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:c0b::22e; helo=mail-it0-x22e.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22e.google.com (mail-it0-x22e.google.com [IPv6:2607:f8b0:4001:c0b::22e]) (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 5647621FC748D for ; Sat, 7 Oct 2017 07:14:00 -0700 (PDT) Received: by mail-it0-x22e.google.com with SMTP id 72so7904315itk.3 for ; Sat, 07 Oct 2017 07:17:25 -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=9zNRn78uRyzca9ss+YEcDBgU5aECQf49wM9/8pZv6xg=; b=opom5gkcIGoN9Q717VhaS5pgRjxSMIkYqTdIQ+nOp7lVrIHlFJfW8E9Tq6Iejb7Yb6 BRGDk+eywQ442enHm8S9VbXlrhDjCpx/6Wi3eDRjWRuyS+fV/tly3wRmVoGbbnhXmvn+ dOW+dv+O2KKp1+ntNPrpMFEeOLqiadWXSImt4yjDSsW43VanPAirX5GDv7JYt1Z2Kq8d en/fBZ+T3VnePc2V05UATS7d44Jp2h5QWmQMRH5m+72SDi5YzIAfOH5XBgO1L81Iypup UPo3KhQUhtzREE0Vo+iQqm4lN0Vro54ngRXypw0dFkNp6k11wh/rH4GQRyeY3tp2VIrB rkjA== 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=9zNRn78uRyzca9ss+YEcDBgU5aECQf49wM9/8pZv6xg=; b=Jw0JEf73JXTry66LwcYsPVO2h7chuRfODwE7OJ+WozqzSQpKqeJ8A+cceZTf/1Ozkl dmjFItnFm3GI0E01X8FFp4s3xUkM7W/zrFRD4EEnyjH7GHw3aWWjztA9QirfxOCAqgJ4 FK+LOAKlTvAXolE3Cz7r3xZT3TIKD/0ZORracwSU7rVKrE/ZLQNf/z0pbTGo/acwyrTb YL8Vnz4Aov2imP7hVHzytagXuMahDbA0ZXiop6adyIeqGOy8apZYmEbG/KHUxs7/Wbjn 5kTZP3ot8qpdh6My9f8b8aIvmvp/VnyIXQKXhEB8bnfZprEn4ytFHiXH08grPX6qkM3w 0qsw== X-Gm-Message-State: AMCzsaX9mANeEbTtHhYxs0LNeRgEMVuyDKn94B+LJ/3ERjLuYFPqDH4c tJq3YPFFBnoPmQBkhgGKoO45fpjQNhxkGZybWi0ffA== X-Google-Smtp-Source: AOwi7QBr4FFehuGXLiCsSFRGq2kySQOjLHyv3CPzLh14yxV0CiFGIFaN+wE9vQI+a5MzYY/rGCaCP48tyP5yT3OIZ4Q= X-Received: by 10.36.211.201 with SMTP id n192mr6845912itg.65.1507385844183; Sat, 07 Oct 2017 07:17:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.157.141 with HTTP; Sat, 7 Oct 2017 07:17:23 -0700 (PDT) In-Reply-To: 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: Sat, 7 Oct 2017 16:17:23 +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: Sat, 07 Oct 2017 14:14:00 -0000 Content-Type: text/plain; charset="UTF-8" Leif, 2017-10-06 22:08 GMT+02:00 Marcin Wojtas : > > 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. > 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