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 }
#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 <Library/MvComPhyLib.h>
#include <Library/IoLib.h>
#include <Library/TimerLib.h>
-#include <Library/ParsePcdLib.h>
#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 <Num> part where <Num> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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 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 <Library/MvComPhyLib.h>
> #include <Library/IoLib.h>
> #include <Library/TimerLib.h>
> -#include <Library/ParsePcdLib.h>
>
> #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 <Num> part where <Num> 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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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...
>
> 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?
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?
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...
Best regards,
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
> > 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Sat, Oct 07, 2017 at 04:17:23PM +0200, Marcin Wojtas wrote:
> > > 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.
I would still prefer the COMPHY_ prefix.
It's a shame about the duplication, but this will still enhance
readability/reviewability.
/
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.