From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4001:c06::231; helo=mail-io0-x231.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C217A21EA15D3 for ; Fri, 6 Oct 2017 13:05:27 -0700 (PDT) Received: by mail-io0-x231.google.com with SMTP id z187so17071795ioz.12 for ; Fri, 06 Oct 2017 13:08:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=kdXv4tHzxJ5/1BHUyRhFYxl79ym0xpenPtadXtmwfUk=; b=UcQ5FHRrBUqSTcPHg+miePz3u0h+7wI4dIDJXnJR6lK/JZBmiELqB6w/eYr5MmNFPK G4JoDVCWDPprcaBEn4gR+mdoOpEX3lPafaaIaYCrk1mJrngXhjSVU1aMpSDioUczaQBU yaN3T9UdIGqjvCCHGYH2VuWZEZTKqdphG6Soef1CXgxHdun5zFjb6H4EwIAnCYzXve3z Y23q63wJi7SKpmXgnXrkn+ZKlOGRRCgyyBIfIrQZ71B1MbwymyCPP960AApEQv9/3HMg wZyZQ5jY3A/G0AvZNQDfNBH3vnV81phb3hpVmcdT+DOtkuAddOOoO9pws/82redSfp9m XsHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=kdXv4tHzxJ5/1BHUyRhFYxl79ym0xpenPtadXtmwfUk=; b=HE6THKG2NCTRva/DpStTedUxMELYdS0Xjp2oM8twSodQBcKZ5cCpEddOVnyB9G0lEt sA4B6R4Ll8xFXe4ntpCMCD8o2F+py/6xrGpWdXEEaSh1vzSE+qr1DNi2XM6wyXK2U2Ww yOOdW6qYyQaRpZHLHKxflOyHgcYT8VKowstY9S37Dh28T+1blbDIlg9ss3Zjq2M0C1iA sHgVcjGtzx+nNRrInjlyFf4ndBY1aiKQAkoEmrj+5+TKyzc4rqYGhIECerz8xwVgaxv1 zLn7iK5q5YeI0OGesz9SjgNY+ZC8fWzOzX3gstCUyDtuH4avU8L+y5iEYrfOC0CN4Xml iHNA== X-Gm-Message-State: AMCzsaVaOYVRGKLceahMMJNz15v75FfdjQijVYFieJmHssnxNiS1eTHo +1OW84OPgrnqZ4VE1kHsr3smO0bThHEd4nojprH5/A== X-Google-Smtp-Source: AOwi7QCZPLYnLjvc2AAC859TCmpy+qr7hbgWpPxjTy3GDOWUf/9wmj+Btz4nL5vn0lDFVRXCztVWiXEl4TFehTXvcxY= X-Received: by 10.107.130.226 with SMTP id m95mr4035394ioi.273.1507320531152; Fri, 06 Oct 2017 13:08:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.157.141 with HTTP; Fri, 6 Oct 2017 13:08:50 -0700 (PDT) In-Reply-To: <20171006195155.gf2cg3aaqr3dqhoe@bivouac.eciton.net> References: <1507276278-3608-1-git-send-email-mw@semihalf.com> <1507276278-3608-2-git-send-email-mw@semihalf.com> <20171006155215.g63kakcrvhx7ocra@bivouac.eciton.net> <20171006195155.gf2cg3aaqr3dqhoe@bivouac.eciton.net> From: Marcin Wojtas Date: Fri, 6 Oct 2017 22:08:50 +0200 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan Subject: Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Oct 2017 20:05:28 -0000 Content-Type: text/plain; charset="UTF-8" 2017-10-06 21:51 GMT+02:00 Leif Lindholm : > 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 : >> > 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 >> >> --- >> >> 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