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::22a; helo=mail-wm0-x22a.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x22a.google.com (mail-wm0-x22a.google.com [IPv6:2a00:1450:400c:c09::22a]) (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 BFCFA21EA15B6 for ; Fri, 6 Oct 2017 08:48:56 -0700 (PDT) Received: by mail-wm0-x22a.google.com with SMTP id b189so8366116wmd.4 for ; Fri, 06 Oct 2017 08:52:20 -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=WzGW2cw2hHSPokWpV6jmbJ8R/tTDg+bHX158+/WPbeA=; b=dwl5Xfry+DLCEXqGEag3+pu1Fp491UZBZX59dD3NoirCw9z0d/jnLiFdeAwoAJoJRE 57tZvUopR3Gsn3hO7F0dyx6+l1neQwNRvJWhDqqT47BOmWO2xD13lPAYLnHNkpBf6Scq ItMfb86lTCLl5D3j7n9B+sY2PMxBDEit3YAMk= 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=WzGW2cw2hHSPokWpV6jmbJ8R/tTDg+bHX158+/WPbeA=; b=AlQCn3U+KKnfvkc5ciNHsy+ZRdFREMWwG+fHV0TrZrtlAMRWVVSM7DGZ9BhFXHmUgy Kqqa0HtrF7FGzGHqlKXbBOi/9Fx3brtTiJ+rJhmCoMq4ndkrkl/KL4w6T4dAM4nVG6U/ 5s5Huz5N/o2JrSip45+2GHxrZgV6Dr4wSrQR02i1/Yf4nXOOP4xZZuJQf+Oj+TGyOpiD bB3JoarWsprNYYrHclVVSsIY5aeqDAgloYuLCVE2MgbdZ+9JavZgEC1f5dTSISKhVRoS ofP1j9Nrl05sQ5q0C2NJFTDZbLuxbRV+pAd5HyYOpfwG1EcYmukEjrUVbRNUnNNnVUgk T6dQ== X-Gm-Message-State: AMCzsaVCxVAel9cqzA7mVPl8WyMwT7utXJtmg3rtM0Oo5SWHSEX1utWF pvZLaZYA8ob0pjAgMJOUAIVPUw== X-Google-Smtp-Source: AOwi7QAcbCeK3t9t6qeNXxQdo0R0pnzLxcg3pt8JfktlzioNxfbsSXwFUd41d8aS0dt7viBNTGpiLg== X-Received: by 10.28.208.2 with SMTP id h2mr1949039wmg.13.1507305138936; Fri, 06 Oct 2017 08:52:18 -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 200sm2938002wmu.44.2017.10.06.08.52.16 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 06 Oct 2017 08:52:17 -0700 (PDT) Date: Fri, 6 Oct 2017 16:52:15 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, nadavh@marvell.com, neta@marvell.com, kostap@marvell.com, jinghua@marvell.com, jsd@semihalf.com Message-ID: <20171006155215.g63kakcrvhx7ocra@bivouac.eciton.net> References: <1507276278-3608-1-git-send-email-mw@semihalf.com> <1507276278-3608-2-git-send-email-mw@semihalf.com> MIME-Version: 1.0 In-Reply-To: <1507276278-3608-2-git-send-email-mw@semihalf.com> 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 15:48:57 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 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.) ? / Leif > #UtmiPhy > gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2 > diff --git a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.c b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.c > index 3eb5d9f..bf21dca 100644 > --- a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.c > +++ b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.c > @@ -113,47 +113,6 @@ RegSetSilent16( > MmioWrite16 (Addr, RegData); > } > > -/* This function returns enum with SerDesType */ > -UINT32 > -ParseSerdesTypeString ( > - CHAR16* String > - ) > -{ > - UINT32 i; > - > - if (String == NULL) > - return COMPHY_TYPE_INVALID; > - > - for (i = 0; i < COMPHY_TYPE_MAX; i++) { > - if (StrCmp (String, TypeStringTable[i]) == 0) { > - return i; > - } > - } > - > - /* PCD string doesn't match any supported SerDes Type */ > - return COMPHY_TYPE_INVALID; > -} > - > -/* This function converts SerDes speed in MHz to enum with SerDesSpeed */ > -UINT32 > -ParseSerdesSpeed ( > - UINT32 Value > - ) > -{ > - UINT32 i; > - UINT32 ValueTable [] = {0, 1250, 1500, 2500, 3000, 3125, > - 5000, 5156, 6000, 6250, 10310}; > - > - for (i = 0; i < COMPHY_SPEED_MAX; i++) { > - if (Value == ValueTable[i]) { > - return i; > - } > - } > - > - /* PCD SerDes speed value doesn't match any supported SerDes speed */ > - return COMPHY_SPEED_INVALID; > -} > - > CHAR16 * > GetTypeString ( > UINT32 Type > @@ -182,7 +141,8 @@ GetSpeedString ( > > VOID > ComPhyPrint ( > - IN CHIP_COMPHY_CONFIG *PtrChipCfg > + IN CHIP_COMPHY_CONFIG *PtrChipCfg, > + IN UINT8 Index > ) > { > UINT32 Lane; > @@ -191,7 +151,7 @@ ComPhyPrint ( > for (Lane = 0; Lane < PtrChipCfg->LanesCount; Lane++) { > SpeedStr = GetSpeedString(PtrChipCfg->MapData[Lane].Speed); > TypeStr = GetTypeString(PtrChipCfg->MapData[Lane].Type); > - DEBUG((DEBUG_ERROR, "Comphy-%d: %-13s %-10s\n", Lane, TypeStr, SpeedStr)); > + DEBUG ((DEBUG_ERROR, "Comphy%d-%d: %-13s %-10s\n", Index, Lane, TypeStr, SpeedStr)); > } > > DEBUG((DEBUG_ERROR, "\n")); > @@ -238,16 +198,16 @@ InitComPhyConfig ( > */ > switch (Id) { > case 0: > - GetComPhyPcd (ChipConfig, LaneData, 0); > + GetComPhyPcd (LaneData, 0); > break; > case 1: > - GetComPhyPcd (ChipConfig, LaneData, 1); > + GetComPhyPcd (LaneData, 1); > break; > case 2: > - GetComPhyPcd (ChipConfig, LaneData, 2); > + GetComPhyPcd (LaneData, 2); > break; > case 3: > - GetComPhyPcd (ChipConfig, LaneData, 3); > + GetComPhyPcd (LaneData, 3); > break; > } > } > @@ -288,12 +248,9 @@ MvComPhyInit ( > /* Get the count of the SerDes of the specific chip */ > MaxComphyCount = PtrChipCfg->LanesCount; > for (Lane = 0; Lane < MaxComphyCount; Lane++) { > - /* Parse PCD with string indicating SerDes Type */ > - PtrChipCfg->MapData[Lane].Type = > - ParseSerdesTypeString (LaneData[Index].TypeStr[Lane]); > - PtrChipCfg->MapData[Lane].Speed = > - ParseSerdesSpeed (LaneData[Index].SpeedValue[Lane]); > - PtrChipCfg->MapData[Lane].Invert = (UINT32)LaneData[Index].InvFlag[Lane]; > + PtrChipCfg->MapData[Lane].Type = LaneData[Index].Type[Lane]; > + PtrChipCfg->MapData[Lane].Speed = LaneData[Index].SpeedValue[Lane]; > + PtrChipCfg->MapData[Lane].Invert = LaneData[Index].InvFlag[Lane]; > > if ((PtrChipCfg->MapData[Lane].Speed == COMPHY_SPEED_INVALID) || > (PtrChipCfg->MapData[Lane].Speed == COMPHY_SPEED_ERROR) || > @@ -311,7 +268,7 @@ MvComPhyInit ( > return Status; > } > > - ComPhyPrint (PtrChipCfg); > + ComPhyPrint (PtrChipCfg, Index); > > /* PHY power UP sequence */ > PtrChipCfg->Init (PtrChipCfg); > diff --git a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.h b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.h > index 3898978..5899a4a 100644 > --- a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.h > +++ b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.h > @@ -43,7 +43,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > #include > #include > #include > -#include > > #define MAX_LANE_OPTIONS 10 > > @@ -52,14 +51,10 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > #define GET_LANE_SPEED(id) PcdGetPtr(PcdChip##id##ComPhySpeeds) > #define GET_LANE_INV(id) PcdGetPtr(PcdChip##id##ComPhyInvFlags) > > -#define FillLaneMap(chip_struct, lane_struct, id) { \ > - ParsePcdString((CHAR16 *) GET_LANE_TYPE(id), chip_struct[id].LanesCount, NULL, lane_struct[id].TypeStr); \ > - ParsePcdString((CHAR16 *) GET_LANE_SPEED(id), chip_struct[id].LanesCount, lane_struct[id].SpeedValue, NULL); \ > - ParsePcdString((CHAR16 *) GET_LANE_INV(id), chip_struct[id].LanesCount, lane_struct[id].InvFlag, NULL); \ > -} > - > -#define GetComPhyPcd(chip_struct, lane_struct, id) { \ > - FillLaneMap(chip_struct, lane_struct, id); \ > +#define GetComPhyPcd(lane_struct, id) { \ > + lane_struct[id].Type = (UINT8 *)GET_LANE_TYPE(id); \ > + lane_struct[id].SpeedValue = (UINT8 *)GET_LANE_SPEED(id); \ > + lane_struct[id].InvFlag = (UINT8 *)GET_LANE_SPEED(id); \ > } > > /***** ComPhy *****/ > @@ -573,15 +568,15 @@ typedef struct { > } COMPHY_MUX_DATA; > > typedef struct { > - UINT32 Type; > - UINT32 Speed; > - UINT32 Invert; > + UINT8 Type; > + UINT8 Speed; > + UINT8 Invert; > } COMPHY_MAP; > > typedef struct { > - CHAR16 *TypeStr[MAX_LANE_OPTIONS]; > - UINTN SpeedValue[MAX_LANE_OPTIONS]; > - UINTN InvFlag[MAX_LANE_OPTIONS]; > + UINT8 *Type; > + UINT8 *SpeedValue; > + UINT8 *InvFlag; > } PCD_LANE_MAP; > > typedef > diff --git a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf > index e0f4634..c223fe5 100644 > --- a/Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf > +++ b/Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf > @@ -51,7 +51,6 @@ > MemoryAllocationLib > PcdLib > IoLib > - ParsePcdLib > > [Sources.common] > ComPhyLib.c > diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt > index 83ebe9d..47a667d 100644 > --- a/Silicon/Marvell/Documentation/PortingGuide.txt > +++ b/Silicon/Marvell/Documentation/PortingGuide.txt > @@ -57,27 +57,59 @@ Every ComPhy PCD has part where stands for chip ID (order is not > important, but configuration will be set for first PcdComPhyChipCount chips). > > Every chip has 3 ComPhy PCDs and three of them comprise per-board lanes > -settings for this chip. Their format is unicode string, containing settings > -for up to 10 lanes. Setting for each one is separated with semicolon. > -These PCDs together describe outputs of PHY integrated in simple cihp. > -Below is example for the first chip (Chip0). > +settings for this chip. Their format is array of up to 10 values reflecting > +defined numbers for SPEED/TYPE/INVERT, whose description can be found in: > > - - gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes > - (Unicode string indicating PHY types. Currently supported are: > + OpenPlatformPkg/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.h > > - { L"unconnected", L"PCIE0", L"PCIE1", L"PCIE2", L"PCIE3", > - L"SATA0", L"SATA1", L"SATA2", L"SATA3", L"SGMII0", > - L"SGMII1", L"SGMII2", L"SGMII3", L"QSGMII", > - L"USB3_HOST0", L"USB3_HOST1", L"USB3_DEVICE", > - L"XAUI0", L"XAUI1", L"XAUI2", L"XAUI3", L"RXAUI0", > - L"RXAUI1", L"KR" } ) > + - gMarvellTokenSpaceGuid.PcdChip0ComPhyTypes > + (Array of types - currently supported are: > + > + COMPHY_TYPE_PCIE0 1 > + COMPHY_TYPE_PCIE1 2 > + COMPHY_TYPE_PCIE2 3 > + COMPHY_TYPE_PCIE3 4 > + COMPHY_TYPE_SATA0 5 > + COMPHY_TYPE_SATA1 6 > + COMPHY_TYPE_SATA2 7 > + COMPHY_TYPE_SATA3 8 > + COMPHY_TYPE_SGMII0 9 > + COMPHY_TYPE_SGMII1 10 > + COMPHY_TYPE_SGMII2 11 > + COMPHY_TYPE_SGMII3 12 > + COMPHY_TYPE_QSGMII 13 > + COMPHY_TYPE_USB3_HOST0 14 > + COMPHY_TYPE_USB3_HOST1 15 > + COMPHY_TYPE_USB3_DEVICE 16 > + COMPHY_TYPE_XAUI0 17 > + COMPHY_TYPE_XAUI1 18 > + COMPHY_TYPE_XAUI2 19 > + COMPHY_TYPE_XAUI3 20 > + COMPHY_TYPE_RXAUI0 21 > + COMPHY_TYPE_RXAUI1 22 > + COMPHY_TYPE_SFI 23 ) > > - gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds > - (Indicates PHY speeds in MHz. Currently supported are: > - { 1250, 1500, 2500, 3000, 3125, 5000, 6000, 6250, 1031 } ) > + (Array of speeds - currently supported are: > + > + COMPHY_SPEED_1_25G 1 > + COMPHY_SPEED_1_5G 2 > + COMPHY_SPEED_2_5G 3 > + COMPHY_SPEED_3G 4 > + COMPHY_SPEED_3_125G 5 > + COMPHY_SPEED_5G 6 > + COMPHY_SPEED_5_15625G 7 > + COMPHY_SPEED_6G 8 > + COMPHY_SPEED_6_25G 9 > + COMPHY_SPEED_10_3125G 10 ) > > - gMarvellTokenSpaceGuid.PcdChip0ComPhyInvFlags > - (Indicates lane polarity invert) > + (Array of lane inversion types - currently supported are: > + > + COMPHY_POLARITY_NO_INVERT 0 > + COMPHY_POLARITY_TXD_INVERT 1 > + COMPHY_POLARITY_RXD_INVERT 2 > + COMPHY_POLARITY_ALL_INVERT 3 ) > > Example > ------- > -- > 1.8.3.1 >