Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Oct 06, 2017 at 09:51:14AM +0200, Marcin Wojtas wrote:
> Simplify obtaining lane data, using arrays with direct enum values,
> rather than strings. This is another step to completely remove
> ParsePcdLib.
> 
> This patch replaces string-based description of ComPhy lanes
> on Armada 70x0 DB with the enum values of type and speed.
> PortingGuide is updated accordingly.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Armada/Armada70x0.dsc           | 11 +++-
>  Platform/Marvell/Library/ComPhyLib/ComPhyLib.c   | 65 ++++----------------
>  Platform/Marvell/Library/ComPhyLib/ComPhyLib.h   | 25 +++-----
>  Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf |  1 -
>  Silicon/Marvell/Documentation/PortingGuide.txt   | 62 ++++++++++++++-----
>  5 files changed, 77 insertions(+), 87 deletions(-)
> 
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 467dfa3..7b03744 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -100,8 +100,15 @@
>  
>    #ComPhy
>    gMarvellTokenSpaceGuid.PcdComPhyDevices|{ 0x1 }
> -  gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes|L"SGMII1;USB3_HOST0;SFI;SATA1;USB3_HOST1;PCIE2"
> -  gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds|L"1250;5000;10310;5000;5000;5000"
> +  # ComPhy0
> +  # 0: SGMII1        1.25 Gbps
> +  # 1: USB3_HOST0    5 Gbps
> +  # 2: SFI           10.31 Gbps
> +  # 3: SATA1         5 Gbps
> +  # 4: USB3_HOST1    5 Gbps
> +  # 5: PCIE2         5 Gbps
> +  gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes|{ 0xA, 0xE, 0x17, 0x6, 0xF, 0x3 }
> +  gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds|{ 0x1, 0x6, 0xA, 0x6, 0x6, 0x6 }

So, I'm really pleased with this patchset, but these two lines above
are its weak spot.

I am not sure about the best way to deal with this, but some way of
getting human readable strings above would be ideal (even if it blows
the line length out of the water).

Could you create a dsc.inc (or .inc.dsc as suggested somewhere
recently) that holds only a [Defines] section declaring things like
  COMPHY_SGMII1 = 0xA
  COMPHY_1_25G  = 0x1

And so on. (Basically all the ones defined in the document below.)
?

/
    Leif

>    #UtmiPhy
>    gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2
> diff --git a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.c b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.c
> index 3eb5d9f..bf21dca 100644
> --- a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.c
> +++ b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.c
> @@ -113,47 +113,6 @@ RegSetSilent16(
>    MmioWrite16 (Addr, RegData);
>  }
>  
> -/* This function returns enum with SerDesType */
> -UINT32
> -ParseSerdesTypeString (
> -  CHAR16* String
> -  )
> -{
> -  UINT32 i;
> -
> -  if (String == NULL)
> -    return COMPHY_TYPE_INVALID;
> -
> -  for (i = 0; i < COMPHY_TYPE_MAX; i++) {
> -    if (StrCmp (String, TypeStringTable[i]) == 0) {
> -      return i;
> -    }
> -  }
> -
> -  /* PCD string doesn't match any supported SerDes Type */
> -  return COMPHY_TYPE_INVALID;
> -}
> -
> -/* This function converts SerDes speed in MHz to enum with SerDesSpeed */
> -UINT32
> -ParseSerdesSpeed (
> -  UINT32 Value
> -  )
> -{
> -  UINT32 i;
> -  UINT32 ValueTable [] = {0, 1250, 1500, 2500, 3000, 3125,
> -                          5000, 5156, 6000, 6250, 10310};
> -
> -  for (i = 0; i < COMPHY_SPEED_MAX; i++) {
> -    if (Value == ValueTable[i]) {
> -      return i;
> -    }
> -  }
> -
> -  /* PCD SerDes speed value doesn't match any supported SerDes speed */
> -  return COMPHY_SPEED_INVALID;
> -}
> -
>  CHAR16 *
>  GetTypeString (
>    UINT32 Type
> @@ -182,7 +141,8 @@ GetSpeedString (
>  
>  VOID
>  ComPhyPrint (
> -  IN CHIP_COMPHY_CONFIG *PtrChipCfg
> +  IN CHIP_COMPHY_CONFIG *PtrChipCfg,
> +  IN UINT8 Index
>    )
>  {
>    UINT32 Lane;
> @@ -191,7 +151,7 @@ ComPhyPrint (
>    for (Lane = 0; Lane < PtrChipCfg->LanesCount; Lane++) {
>      SpeedStr = GetSpeedString(PtrChipCfg->MapData[Lane].Speed);
>      TypeStr = GetTypeString(PtrChipCfg->MapData[Lane].Type);
> -    DEBUG((DEBUG_ERROR, "Comphy-%d: %-13s %-10s\n", Lane, TypeStr, SpeedStr));
> +    DEBUG ((DEBUG_ERROR, "Comphy%d-%d: %-13s %-10s\n", Index, Lane, TypeStr, SpeedStr));
>    }
>  
>    DEBUG((DEBUG_ERROR, "\n"));
> @@ -238,16 +198,16 @@ InitComPhyConfig (
>     */
>    switch (Id) {
>    case 0:
> -    GetComPhyPcd (ChipConfig, LaneData, 0);
> +    GetComPhyPcd (LaneData, 0);
>      break;
>    case 1:
> -    GetComPhyPcd (ChipConfig, LaneData, 1);
> +    GetComPhyPcd (LaneData, 1);
>      break;
>    case 2:
> -    GetComPhyPcd (ChipConfig, LaneData, 2);
> +    GetComPhyPcd (LaneData, 2);
>      break;
>    case 3:
> -    GetComPhyPcd (ChipConfig, LaneData, 3);
> +    GetComPhyPcd (LaneData, 3);
>      break;
>    }
>  }
> @@ -288,12 +248,9 @@ MvComPhyInit (
>      /* Get the count of the SerDes of the specific chip */
>      MaxComphyCount = PtrChipCfg->LanesCount;
>      for (Lane = 0; Lane < MaxComphyCount; Lane++) {
> -      /* Parse PCD with string indicating SerDes Type */
> -      PtrChipCfg->MapData[Lane].Type =
> -        ParseSerdesTypeString (LaneData[Index].TypeStr[Lane]);
> -      PtrChipCfg->MapData[Lane].Speed =
> -        ParseSerdesSpeed (LaneData[Index].SpeedValue[Lane]);
> -      PtrChipCfg->MapData[Lane].Invert = (UINT32)LaneData[Index].InvFlag[Lane];
> +      PtrChipCfg->MapData[Lane].Type = LaneData[Index].Type[Lane];
> +      PtrChipCfg->MapData[Lane].Speed = LaneData[Index].SpeedValue[Lane];
> +      PtrChipCfg->MapData[Lane].Invert = LaneData[Index].InvFlag[Lane];
>  
>        if ((PtrChipCfg->MapData[Lane].Speed == COMPHY_SPEED_INVALID) ||
>            (PtrChipCfg->MapData[Lane].Speed == COMPHY_SPEED_ERROR) ||
> @@ -311,7 +268,7 @@ MvComPhyInit (
>       return Status;
>      }
>  
> -    ComPhyPrint (PtrChipCfg);
> +    ComPhyPrint (PtrChipCfg, Index);
>  
>      /* PHY power UP sequence */
>      PtrChipCfg->Init (PtrChipCfg);
> diff --git a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.h b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.h
> index 3898978..5899a4a 100644
> --- a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.h
> +++ b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.h
> @@ -43,7 +43,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #include <Library/MvComPhyLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/TimerLib.h>
> -#include <Library/ParsePcdLib.h>
>  
>  #define MAX_LANE_OPTIONS          10
>  
> @@ -52,14 +51,10 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #define GET_LANE_SPEED(id)        PcdGetPtr(PcdChip##id##ComPhySpeeds)
>  #define GET_LANE_INV(id)          PcdGetPtr(PcdChip##id##ComPhyInvFlags)
>  
> -#define FillLaneMap(chip_struct, lane_struct, id) { \
> -  ParsePcdString((CHAR16 *) GET_LANE_TYPE(id), chip_struct[id].LanesCount, NULL, lane_struct[id].TypeStr);     \
> -  ParsePcdString((CHAR16 *) GET_LANE_SPEED(id), chip_struct[id].LanesCount, lane_struct[id].SpeedValue, NULL); \
> -  ParsePcdString((CHAR16 *) GET_LANE_INV(id), chip_struct[id].LanesCount, lane_struct[id].InvFlag, NULL);      \
> -}
> -
> -#define GetComPhyPcd(chip_struct, lane_struct, id) {               \
> -  FillLaneMap(chip_struct, lane_struct, id);                       \
> +#define GetComPhyPcd(lane_struct, id) {                      \
> +  lane_struct[id].Type = (UINT8 *)GET_LANE_TYPE(id);         \
> +  lane_struct[id].SpeedValue = (UINT8 *)GET_LANE_SPEED(id);  \
> +  lane_struct[id].InvFlag = (UINT8 *)GET_LANE_SPEED(id);     \
>  }
>  
>  /***** ComPhy *****/
> @@ -573,15 +568,15 @@ typedef struct {
>  } COMPHY_MUX_DATA;
>  
>  typedef struct {
> -  UINT32 Type;
> -  UINT32 Speed;
> -  UINT32 Invert;
> +  UINT8 Type;
> +  UINT8 Speed;
> +  UINT8 Invert;
>  } COMPHY_MAP;
>  
>  typedef struct {
> -  CHAR16 *TypeStr[MAX_LANE_OPTIONS];
> -  UINTN SpeedValue[MAX_LANE_OPTIONS];
> -  UINTN InvFlag[MAX_LANE_OPTIONS];
> +  UINT8 *Type;
> +  UINT8 *SpeedValue;
> +  UINT8 *InvFlag;
>  } PCD_LANE_MAP;
>  
>  typedef
> diff --git a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
> index e0f4634..c223fe5 100644
> --- a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
> +++ b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
> @@ -51,7 +51,6 @@
>    MemoryAllocationLib
>    PcdLib
>    IoLib
> -  ParsePcdLib
>  
>  [Sources.common]
>    ComPhyLib.c
> diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
> index 83ebe9d..47a667d 100644
> --- a/Silicon/Marvell/Documentation/PortingGuide.txt
> +++ b/Silicon/Marvell/Documentation/PortingGuide.txt
> @@ -57,27 +57,59 @@ Every ComPhy PCD has <Num> part where <Num> stands for chip ID (order is not
>  important, but configuration will be set for first PcdComPhyChipCount chips).
>  
>  Every chip has 3 ComPhy PCDs and three of them comprise per-board lanes
> -settings for this chip. Their format is unicode string, containing settings
> -for up to 10 lanes. Setting for each one is separated with semicolon.
> -These PCDs together describe outputs of PHY integrated in simple cihp.
> -Below is example for the first chip (Chip0).
> +settings for this chip. Their format is array of up to 10 values reflecting
> +defined numbers for SPEED/TYPE/INVERT, whose description can be found in:
>  
> -  - gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes
> -	(Unicode string indicating PHY types. Currently supported are:
> +  OpenPlatformPkg/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.h
>  
> -		{ L"unconnected", L"PCIE0", L"PCIE1", L"PCIE2", L"PCIE3",
> -		  L"SATA0", L"SATA1", L"SATA2", L"SATA3", L"SGMII0",
> -		  L"SGMII1", L"SGMII2", L"SGMII3", L"QSGMII",
> -		  L"USB3_HOST0", L"USB3_HOST1", L"USB3_DEVICE",
> -		  L"XAUI0", L"XAUI1", L"XAUI2", L"XAUI3", L"RXAUI0",
> -		  L"RXAUI1", L"KR" } )
> +  - gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes
> +	(Array of types - currently supported are:
> +
> +	COMPHY_TYPE_PCIE0                            1
> +	COMPHY_TYPE_PCIE1                            2
> +	COMPHY_TYPE_PCIE2                            3
> +	COMPHY_TYPE_PCIE3                            4
> +	COMPHY_TYPE_SATA0                            5
> +	COMPHY_TYPE_SATA1                            6
> +	COMPHY_TYPE_SATA2                            7
> +	COMPHY_TYPE_SATA3                            8
> +	COMPHY_TYPE_SGMII0                           9
> +	COMPHY_TYPE_SGMII1                           10
> +	COMPHY_TYPE_SGMII2                           11
> +	COMPHY_TYPE_SGMII3                           12
> +	COMPHY_TYPE_QSGMII                           13
> +	COMPHY_TYPE_USB3_HOST0                       14
> +	COMPHY_TYPE_USB3_HOST1                       15
> +	COMPHY_TYPE_USB3_DEVICE                      16
> +	COMPHY_TYPE_XAUI0                            17
> +	COMPHY_TYPE_XAUI1                            18
> +	COMPHY_TYPE_XAUI2                            19
> +	COMPHY_TYPE_XAUI3                            20
> +	COMPHY_TYPE_RXAUI0                           21
> +	COMPHY_TYPE_RXAUI1                           22
> +	COMPHY_TYPE_SFI                              23 )
>  
>    - gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds
> -	(Indicates PHY speeds in MHz. Currently supported are:
> -		{ 1250, 1500, 2500, 3000, 3125, 5000, 6000, 6250, 1031 }  )
> +	(Array of speeds - currently supported are:
> +
> +	COMPHY_SPEED_1_25G                           1
> +	COMPHY_SPEED_1_5G                            2
> +	COMPHY_SPEED_2_5G                            3
> +	COMPHY_SPEED_3G                              4
> +	COMPHY_SPEED_3_125G                          5
> +	COMPHY_SPEED_5G                              6
> +	COMPHY_SPEED_5_15625G                        7
> +	COMPHY_SPEED_6G                              8
> +	COMPHY_SPEED_6_25G                           9
> +	COMPHY_SPEED_10_3125G                        10 )
>  
>    - gMarvellTokenSpaceGuid.PcdChip0ComPhyInvFlags
> -	(Indicates lane polarity invert)
> +	(Array of lane inversion types - currently supported are:
> +
> +	COMPHY_POLARITY_NO_INVERT                    0
> +	COMPHY_POLARITY_TXD_INVERT                   1
> +	COMPHY_POLARITY_RXD_INVERT                   2
> +	COMPHY_POLARITY_ALL_INVERT                   3 )
>  
>  Example
>  -------
> -- 
> 1.8.3.1
>