public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Marcin Wojtas <mw@semihalf.com>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	 nadavh@marvell.com, Neta Zur Hershkovits <neta@marvell.com>,
	 Kostya Porotchkin <kostap@marvell.com>,
	Hua Jing <jinghua@marvell.com>,
	 semihalf-dabros-jan <jsd@semihalf.com>
Subject: Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing
Date: Sat, 7 Oct 2017 16:17:23 +0200	[thread overview]
Message-ID: <CAPv3WKcNFmj3S-+PH0YJevQRKSbf-=ii4+CJd9-td1DEE=p+tA@mail.gmail.com> (raw)
In-Reply-To: <CAPv3WKcUxLbvrsan5E+Sn62_eNNMVHHVQ1W_8WpunDfpiaqGPw@mail.gmail.com>

Leif,

2017-10-06 22:08 GMT+02:00 Marcin Wojtas <mw@semihalf.com>:
>
> 2017-10-06 21:51 GMT+02:00 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.
>
> 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


  reply	other threads:[~2017-10-07 14:14 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-06  7:51 [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Marcin Wojtas
2017-10-06  7:51 ` [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing Marcin Wojtas
2017-10-06 15:52   ` Leif Lindholm
2017-10-06 19:22     ` Marcin Wojtas
2017-10-06 19:51       ` Leif Lindholm
2017-10-06 20:08         ` Marcin Wojtas
2017-10-07 14:17           ` Marcin Wojtas [this message]
2017-10-07 14:57             ` Leif Lindholm
2017-10-06  7:51 ` [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib Marcin Wojtas
2017-10-06 15:56   ` Leif Lindholm
2017-10-06 19:24     ` Marcin Wojtas
2017-10-06  7:51 ` [platforms: PATCH 3/5] Marvell/Library: UtmiLib: " Marcin Wojtas
2017-10-06 15:57   ` Leif Lindholm
2017-10-06 19:26     ` Marcin Wojtas
2017-10-06 19:57       ` Leif Lindholm
2017-10-06  7:51 ` [platforms: PATCH 4/5] Marvell/Drivers: Pp2Dxe: Rework PHY handling Marcin Wojtas
2017-10-06 16:02   ` Leif Lindholm
2017-10-06 19:30     ` Marcin Wojtas
2017-10-06  7:51 ` [platforms: PATCH 5/5] Platform/Marvell/Armada: Remove ParsePcdLib Marcin Wojtas
2017-10-06 16:02   ` Leif Lindholm
2017-10-06 14:29 ` [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPv3WKcNFmj3S-+PH0YJevQRKSbf-=ii4+CJd9-td1DEE=p+tA@mail.gmail.com' \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox