Leif, 2017-10-06 22:08 GMT+02:00 Marcin Wojtas <[email protected]>: > > 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. >
The macros defined under [Defines] section are visible only within .fdf and .dsc files. I tried both 'DEFINE' and 'EDK_GLOBAL' statements, but the library couldn't use it and we have to define macros in the header anyway. So we have to make a choice - do you wish to use COMPHY_ prefix in PCD values? It may give 130+ signs in such case. Best regards, Marcin _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
