Re: [PATCH edk2-platforms v2 1/3] Silicon/NXP/Library: Implement SerDesHelperLib

["Leif Lindholm" <leif@nuviainc.com>]

On Sun, Jun 07, 2020 at 18:45:48 +0530, Wasim Khan wrote:
> From: Wasim Khan <wasim.khan@nxp.com>
> 
> Implement SerDesHelperLib to provide helper functions which
> can be used for SoC specific SerDes configuration.
> 
> Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> ---
> 
> Notes:
>     Changes in V2:
>     - Addressed review comments for structure, variable and function names
>     - Using BIT0 instead of 0x1u
> 
>  Silicon/NXP/NxpQoriqLs.dec                              |   1 +
>  Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.inf |  28 ++++
>  Silicon/NXP/Include/Library/SerDesHelperLib.h           |  64 ++++++++
>  Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.c   | 164 ++++++++++++++++++++
>  4 files changed, 257 insertions(+)
> 
> diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> index d4d3057af509..d09a1ae194be 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -35,6 +35,7 @@ [PcdsFixedAtBuild.common]
>    gNxpQoriqLsTokenSpaceGuid.PcdNumPciController|0|UINT32|0x00000501
>    gNxpQoriqLsTokenSpaceGuid.PcdPcieLutBase|0x0|UINT32|0x00000502
>    gNxpQoriqLsTokenSpaceGuid.PcdPcieLutDbg|0x0|UINT32|0x00000503
> +  gNxpQoriqLsTokenSpaceGuid.PcdSerDesLanes|0x0|UINT8|0x00000504
>  
>  [PcdsDynamic.common]
>    gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALSE|BOOLEAN|0x00000600
> diff --git a/Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.inf b/Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.inf
> new file mode 100644
> index 000000000000..7a781620e449
> --- /dev/null
> +++ b/Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.inf
> @@ -0,0 +1,28 @@
> +## @file
> +#
> +#  Copyright 2020 NXP
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = SerDesHelperLib
> +  FILE_GUID                      = 2930e932-a700-41e8-80f9-f1a2dedd2c4f
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = SerDesHelperLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  Silicon/NXP/NxpQoriqLs.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  PcdLib
> +
> +[Sources.common]
> +  SerDesHelperLib.c
> +
> +[FixedPcd]
> +  gNxpQoriqLsTokenSpaceGuid.PcdSerDesLanes
> diff --git a/Silicon/NXP/Include/Library/SerDesHelperLib.h b/Silicon/NXP/Include/Library/SerDesHelperLib.h
> new file mode 100644
> index 000000000000..377f020e0b3a
> --- /dev/null
> +++ b/Silicon/NXP/Include/Library/SerDesHelperLib.h
> @@ -0,0 +1,64 @@
> +/** SerDesHelperLib.h
> +  The Header file for SerDesHelperLib
> +
> +  Copyright 2020 NXP
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef SERDES_HELPER_LIB_H
> +#define SERDES_HELPER_LIB_H
> +
> +#include <Uefi.h>
> +#include <Library/SerDes.h>
> +
> +typedef struct {
> +  UINT16 Protocol;
> +  UINT8  SerDesLane[FixedPcdGet8 (PcdSerDesLanes)];
> +} SERDES_CONFIG;
> +
> +typedef enum {
> +  SERDES_1  = 0,
> +  SERDES_2,
> +  SERDES_3,
> +  SERDES_MAX
> +} SERDES_NUMBER;
> +
> +UINT32
> +GetSerDesProtocol (
> +  IN  INTN            SerDes,
> +  IN  INTN            SerDesProtocol,
> +  IN  INTN            Lane,
> +  IN  UINT32          SerDesMaxProtocol,
> +  IN  SERDES_CONFIG   *Config
> +  );
> +
> +EFI_STATUS
> +IsSerDesProtocolValid (
> +  IN  INTN           SerDes,
> +  IN  UINT32         SerDesProtocol,
> +  IN  UINT8          SerDesNumLanes,
> +  IN  SERDES_CONFIG  *Config
> +  );
> +
> +EFI_STATUS
> +GetSerDesMap (
> +  IN  UINT32                    SerDes,
> +  IN  UINT32                    SerDesProtocol,
> +  IN  UINT8                     SerDesNumLanes,
> +  IN  UINT32                    SerDesMaxProtocol,
> +  IN  SERDES_CONFIG             *Config,
> +  OUT UINT64                    *SerDesProtocolMap
> +  );
> +
> +VOID
> +SerDesInstanceProbeLanes (
> +  IN  UINT32                      SerDes,
> +  IN  UINT32                      SerDesProtocol,
> +  IN  UINT8                       SerDesNumLanes,
> +  IN  UINT32                      SerDesMaxProtocol,
> +  IN  SERDES_CONFIG               *Config,
> +  IN  SERDES_PROBE_LANES_CALLBACK SerDesLaneProbeCallback,
> +  IN  VOID                        *Arg
> +  );
> +#endif
> diff --git a/Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.c b/Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.c
> new file mode 100644
> index 000000000000..52c27a0fd49d
> --- /dev/null
> +++ b/Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.c
> @@ -0,0 +1,164 @@
> +/** SerDes.c
> +  Provides SoC specific SerDes interface
> +
> +  Copyright 2020 NXP
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Library/DebugLib.h>
> +#include <Library/SerDesHelperLib.h>
> +
> +/**
> +  Function to get SerDes Lane protocol corresponding to
> +  SerDes protocol.
> +
> +  @param  SerDes              SerDes number.
> +  @param  SerDesProtocol      SerDes protocol number.
> +  @param  Lane                SerDes Lane number.
> +  @param  SerDesMaxProtocol   Max SerDes protocol number.
> +  @param  Config              SerDes Configuration.
> +
> +  @return SerDes Lane protocol.
> +
> +**/
> +UINT32
> +GetSerDesProtocol (
> +  IN  INTN            SerDes,
> +  IN  INTN            SerDesProtocol,
> +  IN  INTN            Lane,
> +  IN  UINT32          SerDesMaxProtocol,
> +  IN  SERDES_CONFIG   *Config
> +  )
> +{
> +  while (Config->Protocol) {
> +    if (Config->Protocol == SerDesProtocol) {
> +      return Config->SerDesLane[Lane];
> +    }
> +    Config++;
> +  }
> +
> +  return SerDesMaxProtocol;
> +}
> +
> +/**
> +  Function to validate input SerDes protocol.
> +
> +  @param  SerDes              SerDes number.
> +  @param  SerDesProtocol      SerDes protocol number.
> +  @param  SerDesNumLanes      Number of SerDes Lanes.
> +  @param  Config              SerDes Configuration.
> +
> +  @return EFI_NOT_FOUND     SerDes Protocol not a valid protocol.
> +  @return EFI_SUCCESS       SerDes Protocol is a valid protocol.
> +
> +**/
> +EFI_STATUS
> +IsSerDesProtocolValid (
> +  IN  INTN           SerDes,
> +  IN  UINT32         SerDesProtocol,
> +  IN  UINT8          SerDesNumLanes,
> +  IN  SERDES_CONFIG  *Config
> +  )
> +{
> +  UINT8 Count;
> +
> +  while (Config->Protocol) {
> +    if (Config->Protocol == SerDesProtocol) {
> +      DEBUG ((DEBUG_INFO, "Protocol: %x Matched with the one in Table\n", SerDesProtocol));
> +      break;
> +    }
> +    Config++;
> +  }
> +
> +  if (!Config->Protocol) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  for (Count = 0; Count < SerDesNumLanes; Count++) {
> +    if (Config->SerDesLane[Count] != 0) {
> +      return EFI_SUCCESS;
> +    }
> +  }
> +
> +  return EFI_NOT_FOUND;
> +}
> +
> +/**
> +  Get Lane protocol on provided SerDes Lane and execute callback function.
> +
> +  @param  SerDes                  SerDes number.
> +  @param  SerDesProtocol          SerDes protocol number.
> +  @param  SerDesNumLanes          Number of SerDes Lanes.
> +  @param  SerDesMaxProtocol       Max SerDes protocol number.
> +  @param  Config                  SerDes Configuration.
> +  @param  SerDesLaneProbeCallback Pointer Callback function to be called for Lane protocol
> +  @param  Arg                     Pointer to Arguments to be passed to callback function.
> +**/
> +VOID
> +SerDesInstanceProbeLanes (
> +  IN  UINT32                      SerDes,
> +  IN  UINT32                      SerDesProtocol,
> +  IN  UINT8                       SerDesNumLanes,
> +  IN  UINT32                      SerDesMaxProtocol,
> +  IN  SERDES_CONFIG               *Config,
> +  IN  SERDES_PROBE_LANES_CALLBACK SerDesLaneProbeCallback,
> +  IN  VOID                        *Arg
> +  )
> +{
> +  INT8    Lane;
> +  UINT32  LaneProtocol;
> +
> +  // Invoke callback for all lanes in the SerDes instance:
> +  for (Lane = 0; Lane < SerDesNumLanes; Lane++) {
> +    LaneProtocol = GetSerDesProtocol (SerDes, SerDesProtocol, Lane, SerDesMaxProtocol, Config);
> +    ASSERT (LaneProtocol < SerDesMaxProtocol);
> +    if (LaneProtocol != 0x0) {
> +      SerDesLaneProbeCallback (LaneProtocol, Arg);
> +    }
> +  }
> +}
> +
> +/**
> +  Function to fill SerDes map information.
> +
> +  @param  SerDes              SerDes number.
> +  @param  SerDesProtocol      SerDes protocol number.
> +  @param  SerDesNumLanes      Number of SerDes Lanes.
> +  @param  SerDesMaxProtocol   Max SerDes protocol number.
> +  @param  Config              SerDes Configuration.
> +  @param  SerDesProtocolMap   Output SerDes protocol map of enabled devices.
> +
> +**/
> +EFI_STATUS
> +GetSerDesMap (
> +  IN  UINT32                    SerDes,
> +  IN  UINT32                    SerDesProtocol,
> +  IN  UINT8                     SerDesNumLanes,
> +  IN  UINT32                    SerDesMaxProtocol,
> +  IN  SERDES_CONFIG             *Config,
> +  OUT UINT64                    *SerDesProtocolMap
> +  )
> +{
> +  INTN                   Lane;
> +  EFI_STATUS             Status;
> +  UINT32                 LanePrtcl;

Still some Prtcl left in this function. Other than that I'm happy with
this. With that fixed:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

If that's the only comment I have on v2, I can fix that up before
committing. If I ask for a v3 for 2 or 3/3, could you please include
this update?


> +
> +  Status = IsSerDesProtocolValid (SerDes, SerDesProtocol, SerDesNumLanes, Config);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: SERDES%d[PRTCL] = 0x%x is not valid, Status = %r \n",
> +            __FUNCTION__, SerDes + 1, SerDesProtocol, Status));
> +    return Status;
> +  }
> +
> +  for (Lane = 0; Lane < SerDesNumLanes; Lane++) {
> +    LanePrtcl = GetSerDesProtocol (SerDes, SerDesProtocol, Lane, SerDesMaxProtocol, Config);
> +    if (LanePrtcl >= SerDesMaxProtocol) {
> +      DEBUG ((DEBUG_ERROR, "Unknown SerDes lane protocol %d\n", LanePrtcl));
> +      return EFI_NO_MAPPING;
> +    }
> +    *SerDesProtocolMap |= (BIT0 << (LanePrtcl));
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> -- 
> 2.7.4
>