Re: [PATCH edk2-platforms 2/3] Silicon/NXP: LS1043A: Add Serdes Support

["Wasim Khan" <wasim.khan@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Friday, June 5, 2020 6:06 PM
> To: Wasim Khan (OSS) <wasim.khan@oss.nxp.com>
> Cc: devel@edk2.groups.io; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> ard.biesheuvel@arm.com; Wasim Khan <wasim.khan@nxp.com>
> Subject: Re: [PATCH edk2-platforms 2/3] Silicon/NXP: LS1043A: Add Serdes
> Support
> 
> On Tue, Jun 02, 2020 at 00:57:37 +0530, Wasim Khan wrote:
> > From: Wasim Khan <wasim.khan@nxp.com>
> >
> > Based on serdes protocol value in reset configuration word (RCW)
> > different IP blocks gets enabled in HW.
> > Add SoC specific serdes configuration for LS1043A, which can be used
> > by different IPs to know the enabled interfaces and perform the
> > required initialization.
> >
> > Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> > ---
> >  Silicon/NXP/LS1043A/LS1043A.dsc.inc           |  2 +
> >  Silicon/NXP/LS1043A/Library/SocLib/SocLib.inf |  6 ++
> >  Silicon/NXP/Include/Library/SerDes.h          | 28 ++++++++
> >  Silicon/NXP/LS1043A/Include/SocSerDes.h       | 68 ++++++++++++++++++
> >  Silicon/NXP/LS1043A/Library/SocLib/SerDes.c   | 75 ++++++++++++++++++++
> >  5 files changed, 179 insertions(+)
> >
> > diff --git a/Silicon/NXP/LS1043A/LS1043A.dsc.inc
> > b/Silicon/NXP/LS1043A/LS1043A.dsc.inc
> > index e023bfbc7c04..b598b5790b65 100644
> > --- a/Silicon/NXP/LS1043A/LS1043A.dsc.inc
> > +++ b/Silicon/NXP/LS1043A/LS1043A.dsc.inc
> > @@ -12,6 +12,7 @@
> >  [LibraryClasses.common]
> >    SocLib|Silicon/NXP/LS1043A/Library/SocLib/SocLib.inf
> >    SerialPortLib|Silicon/NXP/Library/DUartPortLib/DUartPortLib.inf
> > +
> > + SerDesHelperLib|Silicon/NXP/Library/SerDesHelperLib/SerDesHelperLib.
> > + inf
> >
> >
> >
> #################################################################
> #####
> > ##########
> >  #
> > @@ -37,4 +38,5 @@ [PcdsFixedAtBuild.common]
> >    gNxpQoriqLsTokenSpaceGuid.PcdNumPciController|3
> >    gNxpQoriqLsTokenSpaceGuid.PcdPcieLutBase|0x10000
> >    gNxpQoriqLsTokenSpaceGuid.PcdPcieLutDbg|0x7FC
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSerdesLanes|0x4
> >  ##
> > diff --git a/Silicon/NXP/LS1043A/Library/SocLib/SocLib.inf
> > b/Silicon/NXP/LS1043A/Library/SocLib/SocLib.inf
> > index 3d0f988e1c67..4731bea8b92e 100644
> > --- a/Silicon/NXP/LS1043A/Library/SocLib/SocLib.inf
> > +++ b/Silicon/NXP/LS1043A/Library/SocLib/SocLib.inf
> > @@ -22,6 +22,12 @@ [Packages]
> >  [LibraryClasses]
> >    ChassisLib
> >    DebugLib
> > +  PcdLib
> > +  SerDesHelperLib
> >
> >  [Sources.common]
> >    SocLib.c
> > +  SerDes.c
> 
> Insert alphabetically sorted.
> 
> > +
> > +[FixedPcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSerdesLanes
> > diff --git a/Silicon/NXP/Include/Library/SerDes.h
> > b/Silicon/NXP/Include/Library/SerDes.h
> > new file mode 100644
> > index 000000000000..d846589401c6
> > --- /dev/null
> > +++ b/Silicon/NXP/Include/Library/SerDes.h
> > @@ -0,0 +1,28 @@
> > +/** SerDes.h
> > +  Header file for SoC specific Serdes routines
> > +
> > +  Copyright 2017-2020 NXP
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > +
> > +#ifndef SERDES_H
> > +#define SERDES_H
> > +
> > +VOID
> > +GetSerdesProtocolMaps (
> > +  OUT UINT64               *SerDesPrtclMap
> > +  );
> > +
> > +typedef VOID
> > +(*SERDES_PROBE_LANES_CALLBACK) (
> > +  IN UINT32 LaneProtocol,
> > +  IN VOID *Arg
> > +  );
> > +
> > +VOID
> > +SerDesProbeLanes (
> > +  IN SERDES_PROBE_LANES_CALLBACK SerDesLaneProbeCallback,
> > +  IN VOID                        *Arg
> > +  );
> > +#endif
> > diff --git a/Silicon/NXP/LS1043A/Include/SocSerDes.h
> > b/Silicon/NXP/LS1043A/Include/SocSerDes.h
> > new file mode 100644
> > index 000000000000..5bf93cb32df6
> > --- /dev/null
> > +++ b/Silicon/NXP/LS1043A/Include/SocSerDes.h
> > @@ -0,0 +1,68 @@
> > +/** @file
> > +  SoC Specific header file for Serdes mapping
> > +
> > +  Copyright 2017-2020 NXP
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef SOC_SERDES_H
> > +#define SOC_SERDES_H
> > +
> > +typedef enum {
> > +  NONE = 0,
> > +  PCIE1,
> > +  PCIE2,
> > +  PCIE3,
> > +  SATA,
> > +  SGMII_FM1_DTSEC1,
> > +  SGMII_FM1_DTSEC2,
> > +  SGMII_FM1_DTSEC5,
> > +  SGMII_FM1_DTSEC6,
> > +  SGMII_FM1_DTSEC9,
> > +  SGMII_FM1_DTSEC10,
> > +  QSGMII_FM1_A,
> > +  XFI_FM1_MAC9,
> > +  XFI_FM1_MAC10,
> > +  SGMII_2500_FM1_DTSEC2,
> > +  SGMII_2500_FM1_DTSEC5,
> > +  SGMII_2500_FM1_DTSEC9,
> > +  SGMII_2500_FM1_DTSEC10,
> > +  SERDES_PRTCL_COUNT
> > +} SERDES_PROTOCOL;
> > +
> > +SERDES_CONFIG SerDes1ConfigTbl[] = {
> 
> No data definitions in header files please.
> Also, global variables should have 'm' or 'g' prefix.
> 
> > +        /* SerDes 1 */
> > +  {0x1555, {XFI_FM1_MAC9, PCIE1, PCIE2, PCIE3 } },
> > +  {0x2555, {SGMII_2500_FM1_DTSEC9, PCIE1, PCIE2, PCIE3 } },
> > +  {0x4555, {QSGMII_FM1_A, PCIE1, PCIE2, PCIE3 } },
> > +  {0x4558, {QSGMII_FM1_A,  PCIE1, PCIE2, SATA } },
> > +  {0x1355, {XFI_FM1_MAC9, SGMII_FM1_DTSEC2, PCIE2, PCIE3 } },
> > +  {0x2355, {SGMII_2500_FM1_DTSEC9, SGMII_FM1_DTSEC2, PCIE2, PCIE3 }
> > +},
> > +  {0x3335, {SGMII_FM1_DTSEC9, SGMII_FM1_DTSEC2, SGMII_FM1_DTSEC5,
> > +PCIE3 } },
> > +  {0x3355, {SGMII_FM1_DTSEC9, SGMII_FM1_DTSEC2, PCIE2, PCIE3 } },
> > +  {0x3358, {SGMII_FM1_DTSEC9, SGMII_FM1_DTSEC2, PCIE2, SATA } },
> > +  {0x3555, {SGMII_FM1_DTSEC9, PCIE1, PCIE2, PCIE3 } },
> > +  {0x3558, {SGMII_FM1_DTSEC9, PCIE1, PCIE2, SATA } },
> > +  {0x7000, {PCIE1, PCIE1, PCIE1, PCIE1 } },
> > +  {0x9998, {PCIE1, PCIE2, PCIE3, SATA } },
> > +  {0x6058, {PCIE1, PCIE1, PCIE2, SATA } },
> > +  {0x1455, {XFI_FM1_MAC9, QSGMII_FM1_A, PCIE2, PCIE3 } },
> > +  {0x2455, {SGMII_2500_FM1_DTSEC9, QSGMII_FM1_A, PCIE2, PCIE3 } },
> > +  {0x2255, {SGMII_2500_FM1_DTSEC9, SGMII_2500_FM1_DTSEC2, PCIE2,
> > +PCIE3 } },
> > +  {0x3333, {SGMII_FM1_DTSEC9, SGMII_FM1_DTSEC2, SGMII_FM1_DTSEC5,
> > +SGMII_FM1_DTSEC6 } },
> > +  {0x1460, {XFI_FM1_MAC9, QSGMII_FM1_A, PCIE3, PCIE3 } },
> > +  {0x2460, {SGMII_2500_FM1_DTSEC9, QSGMII_FM1_A, PCIE3, PCIE3 } },
> > +  {0x3460, {SGMII_FM1_DTSEC9, QSGMII_FM1_A, PCIE3, PCIE3 } },
> > +  {0x3455, {SGMII_FM1_DTSEC9, QSGMII_FM1_A, PCIE2, PCIE3 } },
> > +  {0x9960, {PCIE1, PCIE2, PCIE3, PCIE3 } },
> > +  {0x2233, {SGMII_2500_FM1_DTSEC9, SGMII_FM1_DTSEC2,
> > +SGMII_FM1_DTSEC5, SGMII_FM1_DTSEC6 } },
> > +  {0x2533, {SGMII_2500_FM1_DTSEC9, PCIE1, SGMII_FM1_DTSEC5,
> > +SGMII_FM1_DTSEC6 } },
> > +  {}
> > +};
> > +
> > +SERDES_CONFIG *SerDesConfigTbl[] = {
> 
> No data definitions in header files please.
> Also, global variables should have 'm' or 'g' prefix.
> 
> Finally, consider naming. I usually let the abbreviation "Tbl" slip, but in this case
> it is inapproproate. I assume for the previous data definition it is intended to
> mean "Table", and here it is supposed to mean "Tables"? The reader is not
> supposed to have to figure that out.
> 
> > +  SerDes1ConfigTbl
> > +};
> > +#endif
> > diff --git a/Silicon/NXP/LS1043A/Library/SocLib/SerDes.c
> > b/Silicon/NXP/LS1043A/Library/SocLib/SerDes.c
> > new file mode 100644
> > index 000000000000..c27367621168
> > --- /dev/null
> > +++ b/Silicon/NXP/LS1043A/Library/SocLib/SerDes.c
> > @@ -0,0 +1,75 @@
> > +/** SerDes.c
> > +  Provides SoC specific serdes interface
> > +
> > +  Copyright 2017-2020 NXP
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > +
> > +#include <Library/ChassisLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/SerDesHelperLib.h>
> > +#include <Soc.h>
> > +#include <SocSerDes.h>
> > +#include <Uefi.h>
> > +
> > +#define RCWSR_SRDS1_PRTCL_MASK     0xffff0000
> > +#define RCWSR_SRDS1_PRTCL_SHIFT    16
> > +
> > +/**
> > +  Probe all serdes lanes for lane protocol and execute provided callback
> function.
> > +
> > +  @param  SerDesLaneProbeCallback Pointer Callback function to be called
> for Lane protocol
> > +  @param  Arg                     Pointer to Arguments to be passed to callback
> function.
> > +
> > +**/
> > +VOID
> > +SerDesProbeLanes (
> > +  IN SERDES_PROBE_LANES_CALLBACK SerDesLaneProbeCallback,
> > +  IN VOID                        *Arg
> > +  )
> > +{
> > +  UINT32                 SrdsProt;
> 
> SerdesProtocol (or SerDesProtocol, depending on which variant you choose).
> 
> > +  LS1043A_DEVICE_CONFIG  *Dcfg;
> 
> DeviceConfig
> 
> > +
> > +  Dcfg = (LS1043A_DEVICE_CONFIG  *)LS1043A_DCFG_ADDRESS;  SrdsProt =
> > + DcfgRead32 ((UINTN)&Dcfg->RcwSr[4]) & RCWSR_SRDS1_PRTCL_MASK;
> > + SrdsProt >>= RCWSR_SRDS1_PRTCL_SHIFT;
> 
> RCWSR is not a generally understood abbreviation. Please either add a glossary
> section to the start of the file, or expand the abbreviation at every point of use.
> 
> > +
> > +  SerDesInstanceProbeLanes (SRDS_1, SrdsProt,
> > +                                    FixedPcdGet8 (PcdSerdesLanes),
> > +                                    SERDES_PRTCL_COUNT,
> > +                                    SerDesConfigTbl[SRDS_1],
> > +                                    SerDesLaneProbeCallback,
> > +                                    Arg); }
> > +
> > +/**
> > +  Function to return Serdes protocol map for all serdes available on board.
> > +
> > +  @param  SerDesPrtclMap   Pointer to Serdes protocl map.
> > +
> > +**/
> > +VOID
> > +GetSerdesProtocolMaps (
> > +  OUT UINT64   *SerDesPrtclMap
> > +  )
> > +{
> > +  UINT32                 SrdsProt;
> > +  LS1043A_DEVICE_CONFIG  *Dcfg;
> 
> Both comments for previous function apply here too.
> 
> /
>     Leif


Thank you Leif for the review. I am addressing your review comments.


> 
> > +  EFI_STATUS             Status;
> > +
> > +  *SerDesPrtclMap = 0x0;
> > +  Dcfg = (LS1043A_DEVICE_CONFIG  *)LS1043A_DCFG_ADDRESS;  SrdsProt =
> > + DcfgRead32 ((UINTN)&Dcfg->RcwSr[4]) & RCWSR_SRDS1_PRTCL_MASK;
> > + SrdsProt >>= RCWSR_SRDS1_PRTCL_SHIFT;
> > +
> > +  Status = GetSerDesMap (SRDS_1, SrdsProt, FixedPcdGet8 (PcdSerdesLanes),
> > +                          SERDES_PRTCL_COUNT, SerDesConfigTbl[SRDS_1],
> > +                          SerDesPrtclMap);
> > +
> > +  if (Status != EFI_SUCCESS) {
> > +    DEBUG ((DEBUG_ERROR, "%a: failed for Serdes1 \n",__FUNCTION__));
> > +    *SerDesPrtclMap = 0x0;
> > +  }
> > +}
> > --
> > 2.7.4
> >