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:22:13PM +0200, Marcin Wojtas wrote:
> Hi Leif,
> 
> 2017-10-06 17:52 GMT+02:00 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 know, this is why I placed acomment above...

The comment is good, but it's just text next to random hex characters.
It explains what the intent is, but does nothing for emabling review.

> > 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.)
> > ?
> 
> Of course I can do it. Two doubts however:
> 1. Wouldn't line with Pcd swell to too many signs?

Oh, indeed.

> gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes|{COMPHY_SGMII1,
> COMPHY_USB3_HOST0, COMPHY_SFI, COMPHY_SATA1, COMPHY_USB3_HOST1,
> COMPHY_PCIE2}
> Alternatively we can shorten this to:
> gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes|{SGMII1, USB3_HOST0, SFI,
> SATA1, USB3_HOST1, PCIE2}
> Which one do you prefer?

I'm just worried about future name clashes with something else.
I don't think the .dsc format supports wrapping a Pcd definition over
multiple lines?

On average, we already have .dscs that end up with too long lines.
I am not sure I care about the line length limit in .dsc files, to be
honest.
Certainly, my take on line length restrictions is that their intent is
to increase visibility. If at any point breaking a rule improves
visibility, that is still permissible.

> 2. If I define stuff e.g. in the .dsc [Defines] section - will they be
> visible in all modules, i.e. would I be able to remove the definitions
> from the ComPhy header? If yes, I guess the longer version above will
> have to be used...

I will confess my ignorance. Not sure.
If it does, that would mean less duplication, which would be good.
But it would also make a name prefix more important.

/
    Leif