From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::22b; helo=mail-wm0-x22b.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) (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 A671E21EA15C9 for ; Fri, 6 Oct 2017 12:48:36 -0700 (PDT) Received: by mail-wm0-x22b.google.com with SMTP id u138so9486183wmu.5 for ; Fri, 06 Oct 2017 12:52:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=wOKnbdeNL0UbRcUCbY1RoBOsfJmMzx6k8z04SUq58fw=; b=hyh7VoqK/bOyfsuCRqMTwrayfPZEt6I/bnT2M7fNGjWmRiDMwWheOMji6qg6nOV380 zagPXv0X4JpNhuL+l2Xy6D6cWRuzvXHvW145UsrxuZ5Tqgjwd+bODqG+gfGn39c2bWEu bQ3qR6DJvvpIjw1EHWN0pxpHAdCPeBMLJCrzc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=wOKnbdeNL0UbRcUCbY1RoBOsfJmMzx6k8z04SUq58fw=; b=NtkAgjISbZ3bgOBwDDpPKOY+jPuwuDX1GXmUVH1cj37JYvB0ItmIMIEsjrknp/c608 wFtqP+wWjeJ2w7ruQXjkzXD8+2i9c9W/fHhaTsfJFKSLM0hnmyOs9ASMIUeS8HriTTBb nNmCJSZpq7BGxAjXEl5t1fq1ifHaCQaUD/dkcz8+4lmae6CjNGW63weNzDvIsS3WOMuK Ed11ToDsmQLzWeJetzKhxF3iqXI/HY1ZqGtObbAZ4+oW/hnZjxUcOipRy1MaXF4L4Sj4 6tohbQOp7wlRQYAg3o0EQv0IPaCEnxAkm7VLPcZAj1xUyVHUWVE+NOY2IhqX7YtIaTJb Zbjg== X-Gm-Message-State: AMCzsaVVtX5ltC4hpGZRncIcDZpDdYVcDg8ojPpwS2u2YsqoIFmIFSsZ 3on6BLXoM7XoIEapSt/5Xo5XBA== X-Google-Smtp-Source: AOwi7QBANTNABYyU132ZLPoTKiIXX4uHQliYn3PQ0XS6ERGxiYmFqmMnVmnG35EcQIz5ZjyP0YvQiQ== X-Received: by 10.28.100.4 with SMTP id y4mr2602122wmb.46.1507319518759; Fri, 06 Oct 2017 12:51:58 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id k105sm3705245wrc.90.2017.10.06.12.51.57 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 06 Oct 2017 12:51:57 -0700 (PDT) Date: Fri, 6 Oct 2017 20:51:55 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan Message-ID: <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> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) 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 19:48:37 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > > 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