2017-10-06 21:51 GMT+02:00 Leif Lindholm <[email protected]>:
> 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 <[email protected]>:
>> > 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 <[email protected]>
>> >> ---
>> >> 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.
Right.
>
>> > 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?
No it doesn't allow to break 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.
>
I need to check it. If it's visible in driver (I guess it is), then
let's go with the preix, to which I lean towards anyway.
Marcin
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
