* [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal
@ 2017-10-06 7:51 Marcin Wojtas
2017-10-06 7:51 ` [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing Marcin Wojtas
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 7:51 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
Hi,
This patchset purpose is complete removal of ParsePcdLib and
cleanup of boards' PCD representation. More details can be
found in the commit logs.
The patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/parsepcd-upstream-r20171005
I'm looking forward to your comments or remarks.
Best regards,
Marcin
Marcin Wojtas (5):
Marvell/Library: ComPhyLib: Remove PCD string parsing
Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib
Marvell/Library: UtmiLib: Move devices description to MvHwDescLib
Marvell/Drivers: Pp2Dxe: Rework PHY handling
Platform/Marvell/Armada: Remove ParsePcdLib
Platform/Marvell/Armada/Armada.dsc.inc | 1 -
Platform/Marvell/Armada/Armada70x0.dsc | 28 ++-
Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 42 ++--
Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf | 3 +-
Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.c | 35 ++-
Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.inf | 3 -
Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c | 122 ++++++-----
Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h | 2 -
Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.inf | 4 +-
Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c | 16 +-
Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h | 2 +-
Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf | 4 +-
Platform/Marvell/Include/Library/MvHwDescLib.h | 95 ++++++++
Platform/Marvell/Include/Library/ParsePcdLib.h | 46 ----
Platform/Marvell/Include/Protocol/Mdio.h | 6 +
Platform/Marvell/Include/Protocol/MvPhy.h | 1 +
Platform/Marvell/Library/ComPhyLib/ComPhyLib.c | 65 +-----
Platform/Marvell/Library/ComPhyLib/ComPhyLib.h | 25 +--
Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf | 1 -
Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c | 228 --------------------
Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf | 50 -----
Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c | 149 ++++++-------
Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h | 1 -
Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf | 11 +-
Platform/Marvell/Marvell.dec | 17 +-
Silicon/Marvell/Documentation/PortingGuide.txt | 146 ++++++++-----
26 files changed, 452 insertions(+), 651 deletions(-)
delete mode 100644 Platform/Marvell/Include/Library/ParsePcdLib.h
delete mode 100644 Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c
delete mode 100644 Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf
--
1.8.3.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing
2017-10-06 7:51 [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Marcin Wojtas
@ 2017-10-06 7:51 ` Marcin Wojtas
2017-10-06 15:52 ` Leif Lindholm
2017-10-06 7:51 ` [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib Marcin Wojtas
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 7:51 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib
2017-10-06 7:51 [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Marcin Wojtas
2017-10-06 7:51 ` [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing Marcin Wojtas
@ 2017-10-06 7:51 ` Marcin Wojtas
2017-10-06 15:56 ` Leif Lindholm
2017-10-06 7:51 ` [platforms: PATCH 3/5] Marvell/Library: UtmiLib: " Marcin Wojtas
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 7:51 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
This patch introduces I2c description, using the new structures
and template in MvHwDescLib. This change enables more flexible
addition of multiple I2c controllers and also allows for
removal of string PCD parsing. Update Armada 70x0 DB description
and PortingGuide accordingly.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada70x0.dsc | 2 +-
Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 42 +++++++++++---------
Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf | 3 +-
Platform/Marvell/Include/Library/MvHwDescLib.h | 25 ++++++++++++
Platform/Marvell/Marvell.dec | 2 +-
Silicon/Marvell/Documentation/PortingGuide.txt | 4 +-
6 files changed, 54 insertions(+), 24 deletions(-)
diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
index 7b03744..d9d126d 100644
--- a/Platform/Marvell/Armada/Armada70x0.dsc
+++ b/Platform/Marvell/Armada/Armada70x0.dsc
@@ -78,7 +78,7 @@
# I2C
gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses|{ 0x50, 0x57, 0x60 }
gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0, 0x0, 0x0 }
- gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|L"0xF2701000;0xF2701100"
+ gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x1, 0x1 }
gMarvellTokenSpaceGuid.PcdEepromI2cAddresses|{ 0x50, 0x57 }
gMarvellTokenSpaceGuid.PcdEepromI2cBuses|{ 0x0, 0x0 }
gMarvellTokenSpaceGuid.PcdI2cClockFrequency|250000000
diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
index fa79ebc..ff8006e 100755
--- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
+++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
@@ -42,12 +42,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <Library/DebugLib.h>
#include <Library/PcdLib.h>
#include <Library/UefiLib.h>
-#include <Library/ParsePcdLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/MvHwDescLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include "MvI2cDxe.h"
+DECLARE_A7K8K_I2C_TEMPLATE;
+
STATIC MV_I2C_BAUD_RATE baud_rate;
STATIC MV_I2C_DEVICE_PATH MvI2cDevicePathProtocol = {
@@ -172,35 +174,39 @@ MvI2cInitialise (
IN EFI_SYSTEM_TABLE *SystemTable
)
{
+ MVHW_I2C_DESC *Desc = &mA7k8kI2cDescTemplate;
+ UINT8 *I2cDeviceTable, Index;
EFI_STATUS Status;
- UINT32 BusCount;
- EFI_PHYSICAL_ADDRESS I2cBaseAddresses[PcdGet32 (PcdI2cBusCount)];
- INTN i;
- BusCount = PcdGet32 (PcdI2cBusCount);
- if (BusCount == 0)
- return EFI_SUCCESS;
+ /* Obtain table with enabled I2c devices */
+ I2cDeviceTable = (UINT8 *)PcdGetPtr (PcdI2cControllers);
+ if (I2cDeviceTable == NULL) {
+ DEBUG ((DEBUG_ERROR, "Missing PcdI2cControllers\n"));
+ return EFI_INVALID_PARAMETER;
+ }
- Status = ParsePcdString (
- (CHAR16 *) PcdGetPtr (PcdI2cBaseAddresses),
- BusCount,
- I2cBaseAddresses,
- NULL
- );
- if (EFI_ERROR(Status))
- return Status;
+ if (PcdGetSize (PcdI2cControllers) > MVHW_MAX_I2C_DEVS) {
+ DEBUG ((DEBUG_ERROR, "Wrong PcdI2cControllers format\n"));
+ return EFI_INVALID_PARAMETER;
+ }
+
+ /* Initialize enabled chips */
+ for (Index = 0; Index < PcdGetSize (PcdI2cControllers); Index++) {
+ if (!MVHW_DEV_ENABLED (I2c, Index)) {
+ DEBUG ((DEBUG_ERROR, "Skip I2c chip %d\n", Index));
+ continue;
+ }
- for (i = 0; i < BusCount; i++) {
Status = MvI2cInitialiseController(
ImageHandle,
SystemTable,
- I2cBaseAddresses[i]
+ Desc->I2cBaseAddresses[Index]
);
if (EFI_ERROR(Status))
return Status;
}
- return Status;
+ return EFI_SUCCESS;
}
STATIC
diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
index 16374ef..c453b4f 100755
--- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
+++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
@@ -55,7 +55,6 @@
UefiLib
UefiDriverEntryPoint
UefiBootServicesTableLib
- ParsePcdLib
[Protocols]
gEfiI2cMasterProtocolGuid
@@ -66,7 +65,7 @@
[Pcd]
gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses
gMarvellTokenSpaceGuid.PcdI2cSlaveBuses
- gMarvellTokenSpaceGuid.PcdI2cBaseAddresses
+ gMarvellTokenSpaceGuid.PcdI2cControllers
gMarvellTokenSpaceGuid.PcdI2cClockFrequency
gMarvellTokenSpaceGuid.PcdI2cBaudRate
gMarvellTokenSpaceGuid.PcdI2cBusCount
diff --git a/Platform/Marvell/Include/Library/MvHwDescLib.h b/Platform/Marvell/Include/Library/MvHwDescLib.h
index 6a86865..e029b50 100644
--- a/Platform/Marvell/Include/Library/MvHwDescLib.h
+++ b/Platform/Marvell/Include/Library/MvHwDescLib.h
@@ -60,6 +60,16 @@ typedef struct {
} MVHW_COMPHY_DESC;
//
+// I2C devices description template definition
+//
+#define MVHW_MAX_I2C_DEVS 4
+
+typedef struct {
+ UINT8 I2cDevCount;
+ UINTN I2cBaseAddresses[MVHW_MAX_I2C_DEVS];
+} MVHW_I2C_DESC;
+
+//
// NonDiscoverable devices description template definition
//
#define MVHW_MAX_XHCI_DEVS 4
@@ -130,6 +140,21 @@ MVHW_COMPHY_DESC mA7k8kComPhyDescTemplate = {\
}
//
+// Platform description of I2C devices
+//
+#define MVHW_CP0_I2C0_BASE 0xF2701000
+#define MVHW_CP0_I2C1_BASE 0xF2701100
+#define MVHW_CP1_I2C0_BASE 0xF4701000
+#define MVHW_CP1_I2C1_BASE 0xF4701100
+
+#define DECLARE_A7K8K_I2C_TEMPLATE \
+STATIC \
+MVHW_I2C_DESC mA7k8kI2cDescTemplate = {\
+ 4,\
+ { MVHW_CP0_I2C0_BASE, MVHW_CP0_I2C1_BASE, MVHW_CP1_I2C0_BASE, MVHW_CP1_I2C1_BASE }\
+}
+
+//
// Platform description of NonDiscoverable devices
//
#define MVHW_CP0_XHCI0_BASE 0xF2500000
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index fc00f1a..25a4258 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -113,7 +113,7 @@
gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0 }|VOID*|0x3000184
gMarvellTokenSpaceGuid.PcdEepromI2cAddresses|{ 0x0 }|VOID*|0x3000050
gMarvellTokenSpaceGuid.PcdEepromI2cBuses|{ 0x0 }|VOID*|0x3000185
- gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|{ 0x0 }|VOID*|0x3000047
+ gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x0 }|VOID*|0x3000047
gMarvellTokenSpaceGuid.PcdI2cClockFrequency|0|UINT32|0x3000048
gMarvellTokenSpaceGuid.PcdI2cBaudRate|0|UINT32|0x3000049
gMarvellTokenSpaceGuid.PcdI2cBusCount|0|UINT32|0x3000183
diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
index 47a667d..1a30c46 100644
--- a/Silicon/Marvell/Documentation/PortingGuide.txt
+++ b/Silicon/Marvell/Documentation/PortingGuide.txt
@@ -188,8 +188,8 @@ In order to enable driver on a new platform, following steps need to be taken:
(buses to which accoring slaves are attached)
- gMarvellTokenSpaceGuid.PcdI2cBusCount|2
(number of SoC's I2C buses)
- - gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|L"0xF2701000;0xF2701100"
- (base addresses of I2C controller buses)
+ - gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x1, 0x1 }
+ (array with used controllers)
- gMarvellTokenSpaceGuid.PcdI2cClockFrequency|200000000
(I2C host controller clock frequency)
- gMarvellTokenSpaceGuid.PcdI2cBaudRate|100000
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [platforms: PATCH 3/5] Marvell/Library: UtmiLib: Move devices description to MvHwDescLib
2017-10-06 7:51 [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Marcin Wojtas
2017-10-06 7:51 ` [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing Marcin Wojtas
2017-10-06 7:51 ` [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib Marcin Wojtas
@ 2017-10-06 7:51 ` Marcin Wojtas
2017-10-06 15:57 ` Leif Lindholm
2017-10-06 7:51 ` [platforms: PATCH 4/5] Marvell/Drivers: Pp2Dxe: Rework PHY handling Marcin Wojtas
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 7:51 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
This patch introduces UTMI description, using the new structures
and template in MvHwDescLib. This change enables more flexible
addition of multiple CP with UTMI PHY's and also significantly
reduces amount of used PCD's for that purpose. Update PortingGuide
documentation accordingly.
This patch replaces string-based description of Utmi on
Armada 70x0 DB with new, reduced format.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada70x0.dsc | 7 +-
Platform/Marvell/Include/Library/MvHwDescLib.h | 47 ++++++
Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c | 149 ++++++++++----------
Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h | 1 -
Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf | 11 +-
Platform/Marvell/Marvell.dec | 7 +-
Silicon/Marvell/Documentation/PortingGuide.txt | 30 ++--
7 files changed, 142 insertions(+), 110 deletions(-)
diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
index d9d126d..04bdf7c 100644
--- a/Platform/Marvell/Armada/Armada70x0.dsc
+++ b/Platform/Marvell/Armada/Armada70x0.dsc
@@ -111,11 +111,8 @@
gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds|{ 0x1, 0x6, 0xA, 0x6, 0x6, 0x6 }
#UtmiPhy
- gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|L"0xF2440420;0xF2440420"
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg|L"0xF2440440;0xF2440444"
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit|L"0xF2580000;0xF2581000"
- gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort|L"0x0;0x1"
+ gMarvellTokenSpaceGuid.PcdUtmiControllers|{ 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0, 0x1 }
#MDIO
gMarvellTokenSpaceGuid.PcdMdioBaseAddress|0xF212A200
diff --git a/Platform/Marvell/Include/Library/MvHwDescLib.h b/Platform/Marvell/Include/Library/MvHwDescLib.h
index e029b50..6ad1bc2 100644
--- a/Platform/Marvell/Include/Library/MvHwDescLib.h
+++ b/Platform/Marvell/Include/Library/MvHwDescLib.h
@@ -117,6 +117,19 @@ typedef struct {
} MVHW_RTC_DESC;
//
+// UTMI PHY's description template definition
+//
+
+typedef struct {
+ UINT8 UtmiDevCount;
+ UINT32 UtmiPhyId[MVHW_MAX_XHCI_DEVS];
+ UINTN UtmiBaseAddresses[MVHW_MAX_XHCI_DEVS];
+ UINTN UtmiConfigAddresses[MVHW_MAX_XHCI_DEVS];
+ UINTN UtmiUsbConfigAddresses[MVHW_MAX_XHCI_DEVS];
+ UINTN UtmiMuxBitCount[MVHW_MAX_XHCI_DEVS];
+} MVHW_UTMI_DESC;
+
+//
// Platform description of CommonPhy devices
//
#define MVHW_CP0_COMPHY_BASE 0xF2441000
@@ -217,4 +230,38 @@ MVHW_RTC_DESC mA7k8kRtcDescTemplate = {\
{ SIZE_4KB, SIZE_4KB }\
}
+//
+// Platform description of UTMI PHY's
+//
+#define MVHW_CP0_UTMI0_BASE 0xF2580000
+#define MVHW_CP0_UTMI0_CFG_BASE 0xF2440440
+#define MVHW_CP0_UTMI0_USB_CFG_BASE 0xF2440420
+#define MVHW_CP0_UTMI0_ID 0x0
+#define MVHW_CP0_UTMI1_BASE 0xF2581000
+#define MVHW_CP0_UTMI1_CFG_BASE 0xF2440444
+#define MVHW_CP0_UTMI1_USB_CFG_BASE 0xF2440420
+#define MVHW_CP0_UTMI1_ID 0x1
+#define MVHW_CP1_UTMI0_BASE 0xF4580000
+#define MVHW_CP1_UTMI0_CFG_BASE 0xF4440440
+#define MVHW_CP1_UTMI0_USB_CFG_BASE 0xF4440420
+#define MVHW_CP1_UTMI0_ID 0x0
+#define MVHW_CP1_UTMI1_BASE 0xF4581000
+#define MVHW_CP1_UTMI1_CFG_BASE 0xF4440444
+#define MVHW_CP1_UTMI1_USB_CFG_BASE 0xF4440420
+#define MVHW_CP1_UTMI1_ID 0x1
+
+#define DECLARE_A7K8K_UTMI_TEMPLATE \
+STATIC \
+MVHW_UTMI_DESC mA7k8kUtmiDescTemplate = {\
+ 4,\
+ { MVHW_CP0_UTMI0_ID, MVHW_CP0_UTMI1_ID,\
+ MVHW_CP1_UTMI0_ID, MVHW_CP1_UTMI1_ID },\
+ { MVHW_CP0_UTMI0_BASE, MVHW_CP0_UTMI1_BASE,\
+ MVHW_CP1_UTMI0_BASE, MVHW_CP1_UTMI1_BASE },\
+ { MVHW_CP0_UTMI0_CFG_BASE, MVHW_CP0_UTMI1_CFG_BASE,\
+ MVHW_CP1_UTMI0_CFG_BASE, MVHW_CP1_UTMI1_CFG_BASE },\
+ { MVHW_CP0_UTMI0_USB_CFG_BASE, MVHW_CP0_UTMI1_USB_CFG_BASE,\
+ MVHW_CP1_UTMI0_USB_CFG_BASE, MVHW_CP1_UTMI1_USB_CFG_BASE }\
+}
+
#endif /* __MVHWDESCLIB_H__ */
diff --git a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c
index 95b5698..b07c038 100644
--- a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c
+++ b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c
@@ -33,12 +33,16 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*******************************************************************************/
#include "UtmiPhyLib.h"
+#include <Library/MvHwDescLib.h>
+
+DECLARE_A7K8K_UTMI_TEMPLATE;
typedef struct {
EFI_PHYSICAL_ADDRESS UtmiBaseAddr;
EFI_PHYSICAL_ADDRESS UsbCfgAddr;
EFI_PHYSICAL_ADDRESS UtmiCfgAddr;
UINT32 UtmiPhyPort;
+ UINT32 PhyId;
} UTMI_PHY_DATA;
STATIC
@@ -236,48 +240,52 @@ UtmiPhyPowerUp (
STATIC
VOID
Cp110UtmiPhyInit (
- IN UINT32 UtmiPhyCount,
IN UTMI_PHY_DATA *UtmiData
)
{
- UINT32 i;
+ EFI_STATUS Status;
- for (i = 0; i < UtmiPhyCount; i++) {
- UtmiPhyPowerDown(i, UtmiData[i].UtmiBaseAddr,
- UtmiData[i].UsbCfgAddr, UtmiData[i].UtmiCfgAddr,
- UtmiData[i].UtmiPhyPort);
- }
+ UtmiPhyPowerDown (
+ UtmiData->PhyId,
+ UtmiData->UtmiBaseAddr,
+ UtmiData->UsbCfgAddr,
+ UtmiData->UtmiCfgAddr,
+ UtmiData->UtmiPhyPort
+ );
/* Power down PLL */
DEBUG((DEBUG_INFO, "UtmiPhy: stage: PHY power down PLL\n"));
- RegSet (UtmiData[0].UsbCfgAddr, 0x0 << UTMI_USB_CFG_PLL_OFFSET,
- UTMI_USB_CFG_PLL_MASK);
-
- for (i = 0; i < UtmiPhyCount; i++) {
- UtmiPhyConfig(i, UtmiData[i].UtmiBaseAddr,
- UtmiData[i].UsbCfgAddr, UtmiData[i].UtmiCfgAddr,
- UtmiData[i].UtmiPhyPort);
+ MmioAnd32 (UtmiData->UsbCfgAddr, ~UTMI_USB_CFG_PLL_MASK);
+
+ UtmiPhyConfig (
+ UtmiData->PhyId,
+ UtmiData->UtmiBaseAddr,
+ UtmiData->UsbCfgAddr,
+ UtmiData->UtmiCfgAddr,
+ UtmiData->UtmiPhyPort
+ );
+
+ Status = UtmiPhyPowerUp (
+ UtmiData->PhyId,
+ UtmiData->UtmiBaseAddr,
+ UtmiData->UsbCfgAddr,
+ UtmiData->UtmiCfgAddr,
+ UtmiData->UtmiPhyPort
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "UtmiPhy: Failed to initialize UTMI PHY %d\n", UtmiData->PhyId));
+ return;
}
- for (i = 0; i < UtmiPhyCount; i++) {
- if (EFI_ERROR(UtmiPhyPowerUp(i, UtmiData[i].UtmiBaseAddr,
- UtmiData[i].UsbCfgAddr, UtmiData[i].UtmiCfgAddr,
- UtmiData[i].UtmiPhyPort))) {
- DEBUG((DEBUG_ERROR, "UtmiPhy: Failed to initialize UTMI PHY %d\n", i));
- continue;
- }
- DEBUG((DEBUG_ERROR, "UTMI PHY %d initialized to ", i));
-
- if (UtmiData[i].UtmiPhyPort == UTMI_PHY_TO_USB_DEVICE0)
- DEBUG((DEBUG_ERROR, "USB Device\n"));
- else
- DEBUG((DEBUG_ERROR, "USB Host%d\n", UtmiData[i].UtmiPhyPort));
- }
+ DEBUG ((DEBUG_ERROR, "UTMI PHY %d initialized to ", UtmiData->PhyId));
+ if (UtmiData->UtmiPhyPort == UTMI_PHY_TO_USB_DEVICE0)
+ DEBUG((DEBUG_ERROR, "USB Device\n"));
+ else
+ DEBUG((DEBUG_ERROR, "USB Host%d\n", UtmiData->UtmiPhyPort));
/* Power up PLL */
DEBUG((DEBUG_INFO, "UtmiPhy: stage: PHY power up PLL\n"));
- RegSet (UtmiData[0].UsbCfgAddr, 0x1 << UTMI_USB_CFG_PLL_OFFSET,
- UTMI_USB_CFG_PLL_MASK);
+ MmioOr32 (UtmiData->UsbCfgAddr, UTMI_USB_CFG_PLL_MASK);
}
EFI_STATUS
@@ -285,69 +293,66 @@ UtmiPhyInit (
VOID
)
{
- EFI_STATUS Status;
- UTMI_PHY_DATA UtmiData[PcdGet32 (PcdUtmiPhyCount)];
- EFI_PHYSICAL_ADDRESS RegUtmiUnit[PcdGet32 (PcdUtmiPhyCount)];
- EFI_PHYSICAL_ADDRESS RegUsbCfg[PcdGet32 (PcdUtmiPhyCount)];
- EFI_PHYSICAL_ADDRESS RegUtmiCfg[PcdGet32 (PcdUtmiPhyCount)];
- UINTN UtmiPort[PcdGet32 (PcdUtmiPhyCount)];
- UINTN i, Count;
-
- Count = PcdGet32 (PcdUtmiPhyCount);
- if (Count == 0) {
+ UTMI_PHY_DATA UtmiData;
+ UINT8 *UtmiDeviceTable, *XhciDeviceTable, *UtmiPortType, Index;
+ MVHW_UTMI_DESC *Desc = &mA7k8kUtmiDescTemplate;
+
+ /* Obtain table with enabled Utmi PHY's*/
+ UtmiDeviceTable = (UINT8 *)PcdGetPtr (PcdUtmiControllers);
+ if (UtmiDeviceTable == NULL) {
/* No UTMI PHY on platform */
return EFI_SUCCESS;
}
- DEBUG((DEBUG_INFO, "UtmiPhy: Initialize USB UTMI PHYs\n"));
- /* Parse UtmiPhy PCDs */
- Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdUtmiPhyRegUtmiUnit),
- Count, RegUtmiUnit, NULL);
- if (EFI_ERROR(Status)) {
- DEBUG((DEBUG_ERROR, "UtmiPhy: Wrong PcdUtmiPhyRegUtmiUnit format\n"));
+ if (PcdGetSize (PcdUtmiControllers) > MVHW_MAX_XHCI_DEVS) {
+ DEBUG ((DEBUG_ERROR, "UTMI: Wrong PcdUtmiControllers format\n"));
return EFI_INVALID_PARAMETER;
}
- Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdUtmiPhyRegUsbCfg),
- Count, RegUsbCfg, NULL);
- if (EFI_ERROR(Status)) {
- DEBUG((DEBUG_ERROR, "UtmiPhy: Wrong PcdUtmiPhyRegUsbCfg format\n"));
+ /* Make sure XHCI controllers table is present */
+ XhciDeviceTable = (UINT8 *)PcdGetPtr (PcdPciEXhci);
+ if (XhciDeviceTable == NULL) {
+ DEBUG ((DEBUG_ERROR, "UTMI: Missing PcdPciEXhci\n"));
return EFI_INVALID_PARAMETER;
}
- Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdUtmiPhyRegUtmiCfg),
- Count, RegUtmiCfg, NULL);
- if (EFI_ERROR(Status)) {
- DEBUG((DEBUG_ERROR, "UtmiPhy: Wrong PcdUtmiPhyRegUtmiCfg format\n"));
+ /* Obtain port type table */
+ UtmiPortType = (UINT8 *)PcdGetPtr (PcdUtmiPortType);
+ if (UtmiPortType == NULL || PcdGetSize (PcdUtmiPortType) != PcdGetSize (PcdUtmiControllers)) {
+ DEBUG ((DEBUG_ERROR, "UTMI: Wrong PcdUtmiPortType format\n"));
return EFI_INVALID_PARAMETER;
}
- Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdUtmiPhyUtmiPort),
- Count, UtmiPort, NULL);
- if (EFI_ERROR(Status)) {
- DEBUG((DEBUG_ERROR, "UtmiPhy: Wrong PcdUtmiPhyUtmiPort format\n"));
- return EFI_INVALID_PARAMETER;
- }
+ /* Initialize enabled chips */
+ for (Index = 0; Index < PcdGetSize (PcdUtmiControllers); Index++) {
+ if (!MVHW_DEV_ENABLED (Utmi, Index)) {
+ continue;
+ }
+
+ /* UTMI PHY without enabled XHCI controller is useless */
+ if (!MVHW_DEV_ENABLED (Xhci, Index)) {
+ DEBUG ((DEBUG_ERROR, "UTMI: Disabled Xhci controller %d\n", Index));
+ return EFI_INVALID_PARAMETER;
+ }
- for (i = 0 ; i < Count ; i++) {
/* Get base address of UTMI phy */
- UtmiData[i].UtmiBaseAddr = RegUtmiUnit[i];
+ UtmiData.UtmiBaseAddr = Desc->UtmiBaseAddresses[Index];
/* Get usb config address */
- UtmiData[i].UsbCfgAddr = RegUsbCfg[i];
+ UtmiData.UsbCfgAddr = Desc->UtmiUsbConfigAddresses[Index];
/* Get UTMI config address */
- UtmiData[i].UtmiCfgAddr = RegUtmiCfg[i];
+ UtmiData.UtmiCfgAddr = Desc->UtmiConfigAddresses[Index];
- /*
- * Get the usb port number, which will be used to check if
- * the utmi connected to host or device
- */
- UtmiData[i].UtmiPhyPort = UtmiPort[i];
- }
+ /* Get UTMI PHY ID */
+ UtmiData.PhyId = Desc->UtmiPhyId[Index];
- /* Currently only Cp110 is supported */
- Cp110UtmiPhyInit (Count, UtmiData);
+ /* Get the usb port type */
+ UtmiData.UtmiPhyPort = UtmiPortType[Index];
+
+ /* Currently only Cp110 is supported */
+ Cp110UtmiPhyInit (&UtmiData);
+ }
return EFI_SUCCESS;
}
diff --git a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h
index f9b4933..0d7d72e 100644
--- a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h
+++ b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h
@@ -42,7 +42,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <Library/MemoryAllocationLib.h>
#include <Library/IoLib.h>
#include <Library/TimerLib.h>
-#include <Library/ParsePcdLib.h>
#define UTMI_USB_CFG_DEVICE_EN_OFFSET 0
#define UTMI_USB_CFG_DEVICE_EN_MASK (0x1 << UTMI_USB_CFG_DEVICE_EN_OFFSET)
diff --git a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
index f1e57f4..6e33cdd 100644
--- a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
+++ b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
@@ -50,15 +50,12 @@
DebugLib
IoLib
MemoryAllocationLib
- ParsePcdLib
PcdLib
[Sources.common]
UtmiPhyLib.c
-[FixedPcd]
- gMarvellTokenSpaceGuid.PcdUtmiPhyCount
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit
- gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort
+[Pcd]
+ gMarvellTokenSpaceGuid.PcdUtmiControllers
+ gMarvellTokenSpaceGuid.PcdUtmiPortType
+ gMarvellTokenSpaceGuid.PcdPciEXhci
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index 25a4258..00d99fa 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -156,11 +156,8 @@
gMarvellTokenSpaceGuid.PcdChip3ComPhyInvFlags|{ 0x0 }|VOID*|0x30000177
#UtmiPhy
- gMarvellTokenSpaceGuid.PcdUtmiPhyCount|0|UINT32|0x30000205
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|{ 0x0 }|VOID*|0x30000206
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg|{ 0x0 }|VOID*|0x30000207
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit|{ 0x0 }|VOID*|0x30000208
- gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort|{ 0x0 }|VOID*|0x30000209
+ gMarvellTokenSpaceGuid.PcdUtmiControllers|{ 0x0 }|VOID*|0x30000206
+ gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0 }|VOID*|0x30000207
#MDIO
gMarvellTokenSpaceGuid.PcdMdioBaseAddress|0|UINT64|0x3000043
diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
index 1a30c46..8a9f603 100644
--- a/Silicon/Marvell/Documentation/PortingGuide.txt
+++ b/Silicon/Marvell/Documentation/PortingGuide.txt
@@ -278,33 +278,23 @@ UTMI PHY configuration
======================
In order to configure UTMI, following PCDs are available:
- - gMarvellTokenSpaceGuid.PcdUtmiPhyCount
- (Indicates how many UTMI PHYs are available on platform)
-
-Next four PCDs are in unicode string format containing settings for all devices
-separated with semicolon.
-
- - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit
- (Indicates base address of the UTMI unit)
-
- - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg
- (Indicates address of USB Configuration register)
+ - gMarvellTokenSpaceGuid.PcdUtmiControllers
+ (Array with used controllers
+ Set to 0x1 for enabled, 0x0 for disabled)
- - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg
- (Indicates address of external UTMI configuration)
+ - gMarvellTokenSpaceGuid.PcdUtmiPortType
+ (Indicates type of the connected USB port:
- - gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort
- (Indicates type of the connected USB port)
+ UTMI_PHY_TO_USB_HOST0 0
+ UTMI_PHY_TO_USB_HOST1 1
+ UTMI_PHY_TO_USB_DEVICE0 2 )
Example
-------
# UtmiPhy
- gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit|L"0xF2580000;0xF2581000"
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|L"0xF2440420;0xF2440420"
- gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg|L"0xF2440440;0xF2440444"
- gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort|L"0x0;0x1"
+ gMarvellTokenSpaceGuid.PcdUtmiControllers|{ 0x1, 0x1 }
+ gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0, 0x1 }
SPI driver configuration
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [platforms: PATCH 4/5] Marvell/Drivers: Pp2Dxe: Rework PHY handling
2017-10-06 7:51 [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Marcin Wojtas
` (2 preceding siblings ...)
2017-10-06 7:51 ` [platforms: PATCH 3/5] Marvell/Library: UtmiLib: " Marcin Wojtas
@ 2017-10-06 7:51 ` Marcin Wojtas
2017-10-06 16:02 ` Leif Lindholm
2017-10-06 7:51 ` [platforms: PATCH 5/5] Platform/Marvell/Armada: Remove ParsePcdLib Marcin Wojtas
2017-10-06 14:29 ` [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Ard Biesheuvel
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 7:51 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
Hitherto PHY handling in Pp2Dxe was not flexible. It allowed for using
only single MDIO controller, which may not be true on Armada 80x0 SoCs.
For this purpose introduce the MDIO description, using the new structures
and template in MvHwDescLib. This change enables addition of multiple
CP110 hardware blocks with MDIO controllers.
This change required different PHY handling and obtaining data over
desired MDIO bus. Now given Pp2 port is matched with the PHY via
its index in gMarvellTokenSpaceGuid.PcdPhyDeviceIds. The PHY itself
is mapped to the MDIO controller, using
gMarvellTokenSpaceGuid.PcdPhy2MdioController. Also obtaining
SMI addresses was moved to the PHY initialization routine.
All above allow for much cleaner and logical PHY description
in the .dsc file.
Update PortingGuide documentation accordingly and Armada 70x0 DB
NIC/PHY description.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada70x0.dsc | 8 +-
Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.c | 35 ++++--
Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.inf | 3 -
Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c | 122 ++++++++++++--------
Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h | 2 -
Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.inf | 4 +-
Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c | 16 +--
Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h | 2 +-
Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf | 4 +-
Platform/Marvell/Include/Library/MvHwDescLib.h | 23 ++++
Platform/Marvell/Include/Protocol/Mdio.h | 6 +
Platform/Marvell/Include/Protocol/MvPhy.h | 1 +
Platform/Marvell/Marvell.dec | 8 +-
Silicon/Marvell/Documentation/PortingGuide.txt | 50 ++++----
14 files changed, 179 insertions(+), 105 deletions(-)
diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
index 04bdf7c..3b75381 100644
--- a/Platform/Marvell/Armada/Armada70x0.dsc
+++ b/Platform/Marvell/Armada/Armada70x0.dsc
@@ -115,18 +115,20 @@
gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0, 0x1 }
#MDIO
- gMarvellTokenSpaceGuid.PcdMdioBaseAddress|0xF212A200
+ gMarvellTokenSpaceGuid.PcdMdioControllers|{ 0x1, 0x0 }
#PHY
- gMarvellTokenSpaceGuid.PcdPhyConnectionTypes|{ 0x8, 0x4, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhy2MdioController|{ 0x0, 0x0 }
gMarvellTokenSpaceGuid.PcdPhyDeviceIds|{ 0x0, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPhySmiAddresses|{ 0x0, 0x1 }
gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg|FALSE
#NET
- gMarvellTokenSpaceGuid.PcdPhySmiAddresses|{ 0xff, 0x0, 0x1 }
gMarvellTokenSpaceGuid.PcdPp2GopIndexes|{ 0x0, 0x2, 0x3 }
gMarvellTokenSpaceGuid.PcdPp2InterfaceAlwaysUp|{ 0x0, 0x0, 0x0 }
gMarvellTokenSpaceGuid.PcdPp2InterfaceSpeed|{ 0x5, 0x3, 0x3 }
+ gMarvellTokenSpaceGuid.PcdPp2PhyConnectionTypes|{ 0x8, 0x4, 0x0 }
+ gMarvellTokenSpaceGuid.PcdPp2PhyIndexes|{ 0xFF, 0x0, 0x1 }
gMarvellTokenSpaceGuid.PcdPp2Port2Controller|{ 0x0, 0x0, 0x0 }
gMarvellTokenSpaceGuid.PcdPp2PortIds|{ 0x0, 0x1, 0x2 }
gMarvellTokenSpaceGuid.PcdPp2Controllers|{ 0x1 }
diff --git a/Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.c b/Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.c
index ae466d7..12aabad 100644
--- a/Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.c
+++ b/Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.c
@@ -46,7 +46,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include "MvMdioDxe.h"
-UINT64 MdioBase = 0;
+DECLARE_A7K8K_MDIO_TEMPLATE;
STATIC
EFI_STATUS
@@ -70,7 +70,7 @@ MdioCheckParam (
STATIC
EFI_STATUS
MdioWaitReady (
- VOID
+ UINT32 MdioBase
)
{
UINT32 Timeout = MVEBU_SMI_TIMEOUT;
@@ -92,7 +92,7 @@ MdioWaitReady (
STATIC
EFI_STATUS
MdioWaitValid (
- VOID
+ UINT32 MdioBase
)
{
UINT32 Timeout = MVEBU_SMI_TIMEOUT;
@@ -116,11 +116,13 @@ EFI_STATUS
MdioOperation (
IN CONST MARVELL_MDIO_PROTOCOL *This,
IN UINT32 PhyAddr,
+ IN UINT32 MdioIndex,
IN UINT32 RegOff,
IN BOOLEAN Write,
IN OUT UINT32 *Data
)
{
+ UINT32 MdioBase = This->BaseAddresses[MdioIndex];
UINT32 MdioReg;
EFI_STATUS Status;
@@ -131,7 +133,7 @@ MdioOperation (
}
/* wait till the SMI is not busy */
- Status = MdioWaitReady ();
+ Status = MdioWaitReady (MdioBase);
if (EFI_ERROR(Status)) {
DEBUG((DEBUG_ERROR, "MdioDxe: MdioWaitReady error\n"));
return Status;
@@ -151,7 +153,7 @@ MdioOperation (
MdioRegWrite32 (MdioReg, MdioBase);
/* make sure that the write transaction is over */
- Status = Write ? MdioWaitReady () : MdioWaitValid ();
+ Status = Write ? MdioWaitReady (MdioBase) : MdioWaitValid (MdioBase);
if (EFI_ERROR(Status)) {
DEBUG((DEBUG_ERROR, "MdioDxe: MdioWaitReady error\n"));
return Status;
@@ -169,6 +171,7 @@ EFI_STATUS
MvMdioRead (
IN CONST MARVELL_MDIO_PROTOCOL *This,
IN UINT32 PhyAddr,
+ IN UINT32 MdioIndex,
IN UINT32 RegOff,
IN UINT32 *Data
)
@@ -178,6 +181,7 @@ MvMdioRead (
Status = MdioOperation (
This,
PhyAddr,
+ MdioIndex,
RegOff,
FALSE,
Data
@@ -190,6 +194,7 @@ EFI_STATUS
MvMdioWrite (
IN CONST MARVELL_MDIO_PROTOCOL *This,
IN UINT32 PhyAddr,
+ IN UINT32 MdioIndex,
IN UINT32 RegOff,
IN UINT32 Data
)
@@ -197,6 +202,7 @@ MvMdioWrite (
return MdioOperation (
This,
PhyAddr,
+ MdioIndex,
RegOff,
TRUE,
&Data
@@ -210,18 +216,27 @@ MvMdioDxeInitialise (
IN EFI_SYSTEM_TABLE *SystemTable
)
{
+ MVHW_MDIO_DESC *Desc = &mA7k8kMdioDescTemplate;
+ UINT8 Index;
MARVELL_MDIO_PROTOCOL *Mdio;
EFI_STATUS Status;
EFI_HANDLE Handle = NULL;
Mdio = AllocateZeroPool (sizeof (MARVELL_MDIO_PROTOCOL));
+ if (Mdio == NULL) {
+ DEBUG ((DEBUG_ERROR, "MdioDxe: Protocol allocation failed\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ /* Obtain base addresses of all possible controllers */
+ for (Index = 0; Index < Desc->MdioDevCount; Index++) {
+ Mdio->BaseAddresses[Index] = Desc->MdioBaseAddresses[Index];
+ }
+
+ Mdio->ControllerCount = Desc->MdioDevCount;
Mdio->Read = MvMdioRead;
Mdio->Write = MvMdioWrite;
- MdioBase = PcdGet64 (PcdMdioBaseAddress);
- if (MdioBase == 0) {
- DEBUG((DEBUG_ERROR, "MdioDxe: PcdMdioBaseAddress not set\n"));
- return EFI_INVALID_PARAMETER;
- }
+
Status = gBS->InstallMultipleProtocolInterfaces (
&Handle,
&gMarvellMdioProtocolGuid, Mdio,
diff --git a/Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.inf b/Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.inf
index faab1f7..d9878eb 100644
--- a/Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.inf
+++ b/Platform/Marvell/Drivers/Net/MvMdioDxe/MvMdioDxe.inf
@@ -62,8 +62,5 @@
[Protocols]
gMarvellMdioProtocolGuid
-[Pcd]
- gMarvellTokenSpaceGuid.PcdMdioBaseAddress
-
[Depex]
TRUE
diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
index aeb6f7a..5821885 100644
--- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
+++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
@@ -41,6 +41,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <Library/DebugLib.h>
#include <Library/IoLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/MvHwDescLib.h>
#include <Library/PcdLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
@@ -51,6 +52,19 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
STATIC MARVELL_MDIO_PROTOCOL *Mdio;
+//
+// Table with available Mdio controllers
+//
+STATIC UINT8 * CONST MdioDeviceTable = PcdGetPtr (PcdMdioControllers);
+//
+// Table with PHY to Mdio controller mappings
+//
+STATIC UINT8 * CONST Phy2MdioController = PcdGetPtr (PcdPhy2MdioController);
+//
+// Table with PHYs' SMI addresses
+//
+STATIC UINT8 * CONST PhySmiAddresses = PcdGetPtr (PcdPhySmiAddresses);
+
STATIC MV_PHY_DEVICE MvPhyDevices[] = {
{ MV_PHY_DEVICE_1512, MvPhyInit1512 },
{ 0, NULL }
@@ -64,18 +78,18 @@ MvPhyStatus (
EFI_STATUS
MvPhyReset (
- IN UINT32 PhyAddr
+ IN PHY_DEVICE *PhyDev
)
{
UINT32 Reg = 0;
INTN timeout = TIMEOUT;
- Mdio->Read(Mdio, PhyAddr, MII_BMCR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMCR, &Reg);
Reg |= BMCR_RESET;
- Mdio->Write(Mdio, PhyAddr, MII_BMCR, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMCR, Reg);
while ((Reg & BMCR_RESET) && timeout--) {
- Mdio->Read(Mdio, PhyAddr, MII_BMCR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMCR, &Reg);
gBS->Stall(1000);
}
@@ -99,7 +113,7 @@ MvPhyM88e1111sConfig (
(PhyDev->Connection == PHY_CONNECTION_RGMII_ID) ||
(PhyDev->Connection == PHY_CONNECTION_RGMII_RXID) ||
(PhyDev->Connection == PHY_CONNECTION_RGMII_TXID)) {
- Mdio->Read(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_CR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_CR, &Reg);
if ((PhyDev->Connection == PHY_CONNECTION_RGMII) ||
(PhyDev->Connection == PHY_CONNECTION_RGMII_ID)) {
@@ -112,9 +126,9 @@ MvPhyM88e1111sConfig (
Reg |= MIIM_88E1111_TX_DELAY;
}
- Mdio->Write(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_CR, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_CR, Reg);
- Mdio->Read(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_SR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_SR, &Reg);
Reg &= ~(MIIM_88E1111_HWCFG_MODE_MASK);
@@ -123,50 +137,50 @@ MvPhyM88e1111sConfig (
else
Reg |= MIIM_88E1111_HWCFG_MODE_COPPER_RGMII;
- Mdio->Write(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_SR, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_SR, Reg);
}
if (PhyDev->Connection == PHY_CONNECTION_SGMII) {
- Mdio->Read(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_SR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_SR, &Reg);
Reg &= ~(MIIM_88E1111_HWCFG_MODE_MASK);
Reg |= MIIM_88E1111_HWCFG_MODE_SGMII_NO_CLK;
Reg |= MIIM_88E1111_HWCFG_FIBER_COPPER_AUTO;
- Mdio->Write(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_SR, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_SR, Reg);
}
if (PhyDev->Connection == PHY_CONNECTION_RTBI) {
- Mdio->Read(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_CR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_CR, &Reg);
Reg |= (MIIM_88E1111_RX_DELAY | MIIM_88E1111_TX_DELAY);
- Mdio->Write(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_CR, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_CR, Reg);
- Mdio->Read(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_SR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_SR, &Reg);
Reg &= ~(MIIM_88E1111_HWCFG_MODE_MASK |
MIIM_88E1111_HWCFG_FIBER_COPPER_RES);
Reg |= 0x7 | MIIM_88E1111_HWCFG_FIBER_COPPER_AUTO;
- Mdio->Write(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_SR, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_SR, Reg);
/* Soft reset */
- MvPhyReset(PhyDev->Addr);
+ MvPhyReset (PhyDev);
- Mdio->Read(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_SR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_SR, &Reg);
Reg &= ~(MIIM_88E1111_HWCFG_MODE_MASK |
MIIM_88E1111_HWCFG_FIBER_COPPER_RES);
Reg |= MIIM_88E1111_HWCFG_MODE_COPPER_RTBI |
MIIM_88E1111_HWCFG_FIBER_COPPER_AUTO;
- Mdio->Write(Mdio, PhyDev->Addr, MIIM_88E1111_PHY_EXT_SR, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1111_PHY_EXT_SR, Reg);
}
- Mdio->Read(Mdio, PhyDev->Addr, MII_BMCR, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMCR, &Reg);
Reg |= (BMCR_ANENABLE | BMCR_ANRESTART);
Reg &= ~BMCR_ISOLATE;
- Mdio->Write(Mdio, PhyDev->Addr, MII_BMCR, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMCR, Reg);
/* Soft reset */
- MvPhyReset(PhyDev->Addr);
+ MvPhyReset (PhyDev);
- MvPhyReset(PhyDev->Addr);
+ MvPhyReset (PhyDev);
return EFI_SUCCESS;
}
@@ -179,7 +193,7 @@ MvPhyParseStatus (
UINT32 Data;
UINT32 Speed;
- Mdio->Read(Mdio, PhyDev->Addr, MIIM_88E1xxx_PHY_STATUS, &Data);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1xxx_PHY_STATUS, &Data);
if ((Data & MIIM_88E1xxx_PHYSTAT_LINK) &&
!(Data & MIIM_88E1xxx_PHYSTAT_SPDDONE)) {
@@ -196,7 +210,7 @@ MvPhyParseStatus (
if ((i++ % 1000) == 0)
DEBUG((DEBUG_ERROR, "."));
gBS->Stall(1000);
- Mdio->Read(Mdio, PhyDev->Addr, MIIM_88E1xxx_PHY_STATUS, &Data);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MIIM_88E1xxx_PHY_STATUS, &Data);
}
DEBUG((DEBUG_ERROR," done\n"));
gBS->Stall(500000);
@@ -241,7 +255,7 @@ MvPhyParseStatus (
STATIC
VOID
MvPhy1512WriteBits (
- IN UINT32 PhyAddr,
+ IN PHY_DEVICE *PhyDev,
IN UINT8 RegNum,
IN UINT16 Offset,
IN UINT16 Len,
@@ -254,19 +268,18 @@ MvPhy1512WriteBits (
else
Mask = (1 << (Len + Offset)) - (1 << Offset);
- Mdio->Read(Mdio, PhyAddr, RegNum, &Reg);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, RegNum, &Reg);
Reg &= ~Mask;
Reg |= Data << Offset;
- Mdio->Write(Mdio, PhyAddr, RegNum, Reg);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, RegNum, Reg);
}
STATIC
EFI_STATUS
MvPhyInit1512 (
IN CONST MARVELL_PHY_PROTOCOL *Snp,
- IN UINT32 PhyAddr,
IN OUT PHY_DEVICE *PhyDev
)
{
@@ -278,28 +291,28 @@ MvPhyInit1512 (
* Marvell Release Notes - Alaska 88E1510/88E1518/88E1512 Rev A0,
* Errata Section 3.1 - needed in SGMII mode.
*/
- Mdio->Write(Mdio, PhyAddr, 22, 0x00ff);
- Mdio->Write(Mdio, PhyAddr, 17, 0x214B);
- Mdio->Write(Mdio, PhyAddr, 16, 0x2144);
- Mdio->Write(Mdio, PhyAddr, 17, 0x0C28);
- Mdio->Write(Mdio, PhyAddr, 16, 0x2146);
- Mdio->Write(Mdio, PhyAddr, 17, 0xB233);
- Mdio->Write(Mdio, PhyAddr, 16, 0x214D);
- Mdio->Write(Mdio, PhyAddr, 17, 0xCC0C);
- Mdio->Write(Mdio, PhyAddr, 16, 0x2159);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 22, 0x00ff);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 17, 0x214B);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 16, 0x2144);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 17, 0x0C28);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 16, 0x2146);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 17, 0xB233);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 16, 0x214D);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 17, 0xCC0C);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 16, 0x2159);
/* Reset page selection and select page 0x12 */
- Mdio->Write(Mdio, PhyAddr, 22, 0x0000);
- Mdio->Write(Mdio, PhyAddr, 22, 0x0012);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 22, 0x0000);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 22, 0x0012);
/* Write HWCFG_MODE = SGMII to Copper */
- MvPhy1512WriteBits(PhyAddr, 20, 0, 3, 1);
+ MvPhy1512WriteBits(PhyDev, 20, 0, 3, 1);
/* Phy reset - necessary after changing mode */
- MvPhy1512WriteBits(PhyAddr, 20, 15, 1, 1);
+ MvPhy1512WriteBits(PhyDev, 20, 15, 1, 1);
/* Reset page selection */
- Mdio->Write(Mdio, PhyAddr, 22, 0x0000);
+ Mdio->Write (Mdio, PhyDev->Addr, PhyDev->MdioIndex, 22, 0x0000);
gBS->Stall(100);
}
@@ -309,7 +322,7 @@ MvPhyInit1512 (
if (!PcdGetBool (PcdPhyStartupAutoneg))
return EFI_SUCCESS;
- Mdio->Read(Mdio, PhyAddr, MII_BMSR, &Data);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
if ((Data & BMSR_ANEGCAPABLE) && !(Data & BMSR_ANEGCOMPLETE)) {
@@ -322,12 +335,12 @@ MvPhyInit1512 (
}
gBS->Stall(1000); /* 1 ms */
- Mdio->Read(Mdio, PhyAddr, MII_BMSR, &Data);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
}
PhyDev->LinkUp = TRUE;
DEBUG((DEBUG_INFO, "MvPhyDxe: link up\n"));
} else {
- Mdio->Read(Mdio, PhyAddr, MII_BMSR, &Data);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
if (Data & BMSR_LSTATUS) {
PhyDev->LinkUp = TRUE;
@@ -345,7 +358,7 @@ MvPhyInit1512 (
EFI_STATUS
MvPhyInit (
IN CONST MARVELL_PHY_PROTOCOL *Snp,
- IN UINT32 PhyAddr,
+ IN UINT32 PhyIndex,
IN PHY_CONNECTION PhyConnection,
IN OUT PHY_DEVICE **OutPhyDev
)
@@ -353,6 +366,7 @@ MvPhyInit (
EFI_STATUS Status;
PHY_DEVICE *PhyDev;
UINT8 *DeviceIds;
+ UINT8 MdioIndex;
INTN i;
Status = gBS->LocateProtocol (
@@ -363,12 +377,20 @@ MvPhyInit (
if (EFI_ERROR(Status))
return Status;
+ MdioIndex = Phy2MdioController[PhyIndex];
+
+ /* Verify correctness of PHY <-> MDIO assignment */
+ if (!MVHW_DEV_ENABLED (Mdio, MdioIndex) || MdioIndex >= Mdio->ControllerCount) {
+ DEBUG ((DEBUG_ERROR, "MvPhyDxe: Incorrect Mdio controller assignment for PHY#%d", PhyIndex));
+ return EFI_INVALID_PARAMETER;
+ }
+
/* perform setup common for all PHYs */
PhyDev = AllocateZeroPool (sizeof (PHY_DEVICE));
- PhyDev->Addr = PhyAddr;
+ PhyDev->Addr = PhySmiAddresses[PhyIndex];
PhyDev->Connection = PhyConnection;
DEBUG((DEBUG_INFO, "MvPhyDxe: PhyAddr is %d, connection %d\n",
- PhyAddr, PhyConnection));
+ PhyDev->Addr, PhyConnection));
*OutPhyDev = PhyDev;
DeviceIds = PcdGetPtr (PcdPhyDeviceIds);
@@ -377,7 +399,7 @@ MvPhyInit (
if (MvPhyDevices[i].DevId == DeviceIds[i]) {
ASSERT (MvPhyDevices[i].DevInit != NULL);
/* proceed with PHY-specific initialization */
- return MvPhyDevices[i].DevInit(Snp, PhyAddr, PhyDev);
+ return MvPhyDevices[i].DevInit (Snp, PhyDev);
}
}
@@ -395,8 +417,8 @@ MvPhyStatus (
{
UINT32 Data;
- Mdio->Read(Mdio, PhyDev->Addr, MII_BMSR, &Data);
- Mdio->Read(Mdio, PhyDev->Addr, MII_BMSR, &Data);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
+ Mdio->Read (Mdio, PhyDev->Addr, PhyDev->MdioIndex, MII_BMSR, &Data);
if ((Data & BMSR_LSTATUS) == 0) {
PhyDev->LinkUp = FALSE;
diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h
index 6bd06c5..0c3d935 100644
--- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h
+++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h
@@ -174,7 +174,6 @@ typedef
EFI_STATUS
(*MV_PHY_DEVICE_INIT) (
IN CONST MARVELL_PHY_PROTOCOL *Snp,
- IN UINT32 PhyAddr,
IN OUT PHY_DEVICE *PhyDev
);
@@ -187,7 +186,6 @@ STATIC
EFI_STATUS
MvPhyInit1512 (
IN CONST MARVELL_PHY_PROTOCOL *Snp,
- IN UINT32 PhyAddr,
IN OUT PHY_DEVICE *PhyDev
);
diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.inf b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.inf
index c262ce4..f96cf35 100644
--- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.inf
+++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.inf
@@ -63,8 +63,10 @@
gMarvellPhyProtocolGuid
[Pcd]
- gMarvellTokenSpaceGuid.PcdPhyConnectionTypes
+ gMarvellTokenSpaceGuid.PcdMdioControllers
+ gMarvellTokenSpaceGuid.PcdPhy2MdioController
gMarvellTokenSpaceGuid.PcdPhyDeviceIds
+ gMarvellTokenSpaceGuid.PcdPhySmiAddresses
gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg
[Depex]
diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
index 620bd5c..2827976 100644
--- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
+++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
@@ -519,14 +519,14 @@ Pp2DxePhyInitialize (
return Status;
}
- if (Pp2Context->Port.PhyAddr == 0xff) {
+ if (Pp2Context->Port.PhyIndex == 0xff) {
/* PHY iniitalization not required */
return EFI_SUCCESS;
}
Status = Pp2Context->Phy->Init(
Pp2Context->Phy,
- Pp2Context->Port.PhyAddr,
+ Pp2Context->Port.PhyIndex,
Pp2Context->Port.PhyInterface,
&Pp2Context->PhyDev
);
@@ -1147,25 +1147,25 @@ Pp2DxeParsePortPcd (
IN INTN Index
)
{
- UINT8 *PortIds, *GopIndexes, *PhyConnectionTypes, *AlwaysUp, *Speed, *PhyAddresses;
+ UINT8 *PortIds, *GopIndexes, *PhyConnectionTypes, *AlwaysUp, *Speed, *PhyIndexes;
PortIds = PcdGetPtr (PcdPp2PortIds);
GopIndexes = PcdGetPtr (PcdPp2GopIndexes);
- PhyConnectionTypes = PcdGetPtr (PcdPhyConnectionTypes);
- PhyAddresses = PcdGetPtr (PcdPhySmiAddresses);
+ PhyConnectionTypes = PcdGetPtr (PcdPp2PhyConnectionTypes);
+ PhyIndexes = PcdGetPtr (PcdPp2PhyIndexes);
AlwaysUp = PcdGetPtr (PcdPp2InterfaceAlwaysUp);
Speed = PcdGetPtr (PcdPp2InterfaceSpeed);
ASSERT (PcdGetSize (PcdPp2GopIndexes) == PcdGetSize (PcdPp2PortIds));
- ASSERT (PcdGetSize (PcdPhyConnectionTypes) == PcdGetSize (PcdPp2PortIds));
+ ASSERT (PcdGetSize (PcdPp2PhyConnectionTypes) == PcdGetSize (PcdPp2PortIds));
ASSERT (PcdGetSize (PcdPp2InterfaceAlwaysUp) == PcdGetSize (PcdPp2PortIds));
ASSERT (PcdGetSize (PcdPp2InterfaceSpeed) == PcdGetSize (PcdPp2PortIds));
- ASSERT (PcdGetSize (PcdPhySmiAddresses) == PcdGetSize (PcdPp2PortIds));
+ ASSERT (PcdGetSize (PcdPp2PhyIndexes) == PcdGetSize (PcdPp2PortIds));
Pp2Context->Port.Id = PortIds[Index];
Pp2Context->Port.GopIndex = GopIndexes[Index];
Pp2Context->Port.PhyInterface = PhyConnectionTypes[Index];
- Pp2Context->Port.PhyAddr = PhyAddresses[Index];
+ Pp2Context->Port.PhyIndex = PhyIndexes[Index];
Pp2Context->Port.AlwaysUp = AlwaysUp[Index];
Pp2Context->Port.Speed = Speed[Index];
}
diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
index cde2995..60f40be 100644
--- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
+++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
@@ -327,7 +327,7 @@ struct Pp2DxePort {
UINT16 RxRingSize;
INT32 PhyInterface;
- UINTN PhyAddr;
+ UINT32 PhyIndex;
BOOLEAN Link;
BOOLEAN Duplex;
BOOLEAN AlwaysUp;
diff --git a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
index 752fcc0..b4568d8 100644
--- a/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
+++ b/Platform/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
@@ -71,12 +71,12 @@
gMarvellPhyProtocolGuid
[Pcd]
- gMarvellTokenSpaceGuid.PcdPhyConnectionTypes
- gMarvellTokenSpaceGuid.PcdPhySmiAddresses
gMarvellTokenSpaceGuid.PcdPp2Controllers
gMarvellTokenSpaceGuid.PcdPp2GopIndexes
gMarvellTokenSpaceGuid.PcdPp2InterfaceAlwaysUp
gMarvellTokenSpaceGuid.PcdPp2InterfaceSpeed
+ gMarvellTokenSpaceGuid.PcdPp2PhyConnectionTypes
+ gMarvellTokenSpaceGuid.PcdPp2PhyIndexes
gMarvellTokenSpaceGuid.PcdPp2Port2Controller
gMarvellTokenSpaceGuid.PcdPp2PortIds
diff --git a/Platform/Marvell/Include/Library/MvHwDescLib.h b/Platform/Marvell/Include/Library/MvHwDescLib.h
index 6ad1bc2..9ae03d0 100644
--- a/Platform/Marvell/Include/Library/MvHwDescLib.h
+++ b/Platform/Marvell/Include/Library/MvHwDescLib.h
@@ -70,6 +70,16 @@ typedef struct {
} MVHW_I2C_DESC;
//
+// MDIO devices description template definition
+//
+#define MVHW_MAX_MDIO_DEVS 2
+
+typedef struct {
+ UINT8 MdioDevCount;
+ UINTN MdioBaseAddresses[MVHW_MAX_MDIO_DEVS];
+} MVHW_MDIO_DESC;
+
+//
// NonDiscoverable devices description template definition
//
#define MVHW_MAX_XHCI_DEVS 4
@@ -168,6 +178,19 @@ MVHW_I2C_DESC mA7k8kI2cDescTemplate = {\
}
//
+// Platform description of MDIO devices
+//
+#define MVHW_CP0_MDIO_BASE 0xF212A200
+#define MVHW_CP1_MDIO_BASE 0xF412A200
+
+#define DECLARE_A7K8K_MDIO_TEMPLATE \
+STATIC \
+MVHW_MDIO_DESC mA7k8kMdioDescTemplate = {\
+ 2,\
+ { MVHW_CP0_MDIO_BASE, MVHW_CP1_MDIO_BASE }\
+}
+
+//
// Platform description of NonDiscoverable devices
//
#define MVHW_CP0_XHCI0_BASE 0xF2500000
diff --git a/Platform/Marvell/Include/Protocol/Mdio.h b/Platform/Marvell/Include/Protocol/Mdio.h
index 10acad4..d077a8f 100644
--- a/Platform/Marvell/Include/Protocol/Mdio.h
+++ b/Platform/Marvell/Include/Protocol/Mdio.h
@@ -35,6 +35,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#ifndef __MDIO_H__
#define __MDIO_H__
+#include <Library/MvHwDescLib.h>
+
#define MARVELL_MDIO_PROTOCOL_GUID { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
typedef struct _MARVELL_MDIO_PROTOCOL MARVELL_MDIO_PROTOCOL;
@@ -44,6 +46,7 @@ EFI_STATUS
(EFIAPI *MARVELL_MDIO_READ) (
IN CONST MARVELL_MDIO_PROTOCOL *This,
IN UINT32 PhyAddr,
+ IN UINT32 MdioIndex,
IN UINT32 RegOff,
IN UINT32 *Data
);
@@ -53,6 +56,7 @@ EFI_STATUS
(EFIAPI *MARVELL_MDIO_WRITE) (
IN CONST MARVELL_MDIO_PROTOCOL *This,
IN UINT32 PhyAddr,
+ IN UINT32 MdioIndex,
IN UINT32 RegOff,
IN UINT32 Data
);
@@ -60,6 +64,8 @@ EFI_STATUS
struct _MARVELL_MDIO_PROTOCOL {
MARVELL_MDIO_READ Read;
MARVELL_MDIO_WRITE Write;
+ UINTN BaseAddresses[MVHW_MAX_MDIO_DEVS];
+ UINTN ControllerCount;
};
extern EFI_GUID gMarvellMdioProtocolGuid;
diff --git a/Platform/Marvell/Include/Protocol/MvPhy.h b/Platform/Marvell/Include/Protocol/MvPhy.h
index a91759a..99c75b3 100644
--- a/Platform/Marvell/Include/Protocol/MvPhy.h
+++ b/Platform/Marvell/Include/Protocol/MvPhy.h
@@ -62,6 +62,7 @@ typedef enum {
typedef struct {
UINT32 Addr;
+ UINT8 MdioIndex;
BOOLEAN LinkUp;
BOOLEAN FullDuplex;
BOOLEAN AutoNegotiation;
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index 00d99fa..cd2f3ad 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -160,19 +160,21 @@
gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0 }|VOID*|0x30000207
#MDIO
- gMarvellTokenSpaceGuid.PcdMdioBaseAddress|0|UINT64|0x3000043
+ gMarvellTokenSpaceGuid.PcdMdioControllers|{ 0x0 }|VOID*|0x3000043
#PHY
- gMarvellTokenSpaceGuid.PcdPhyConnectionTypes|{ 0x0 }|VOID*|0x3000044
+ gMarvellTokenSpaceGuid.PcdPhy2MdioController|{ 0x0 }|VOID*|0x3000027
gMarvellTokenSpaceGuid.PcdPhyDeviceIds|{ 0x0 }|VOID*|0x3000095
+ gMarvellTokenSpaceGuid.PcdPhySmiAddresses|{ 0x0 }|VOID*|0x3000024
gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg|FALSE|BOOLEAN|0x3000070
#NET
- gMarvellTokenSpaceGuid.PcdPhySmiAddresses|{ 0x0 }|VOID*|0x3000024
gMarvellTokenSpaceGuid.PcdPp2Controllers|{ 0x0 }|VOID*|0x3000028
gMarvellTokenSpaceGuid.PcdPp2GopIndexes|{ 0x0 }|VOID*|0x3000029
gMarvellTokenSpaceGuid.PcdPp2InterfaceAlwaysUp|{ 0x0 }|VOID*|0x300002A
gMarvellTokenSpaceGuid.PcdPp2InterfaceSpeed|{ 0x0 }|VOID*|0x300002B
+ gMarvellTokenSpaceGuid.PcdPp2PhyConnectionTypes|{ 0x0 }|VOID*|0x3000044
+ gMarvellTokenSpaceGuid.PcdPp2PhyIndexes|{ 0x0 }|VOID*|0x3000045
gMarvellTokenSpaceGuid.PcdPp2Port2Controller|{ 0x0 }|VOID*|0x300002D
gMarvellTokenSpaceGuid.PcdPp2PortIds|{ 0x0 }|VOID*|0x300002C
diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
index 8a9f603..52968ae 100644
--- a/Silicon/Marvell/Documentation/PortingGuide.txt
+++ b/Silicon/Marvell/Documentation/PortingGuide.txt
@@ -125,25 +125,15 @@ PHY Driver configuration
MvPhyDxe provides basic initialization and status routines for Marvell PHYs.
Currently only 1518 series PHYs are supported. Following PCDs are required:
- - gMarvellTokenSpaceGuid.PcdPhyConnectionTypes
- (list of values corresponding to PHY_CONNECTION enum)
- gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg
(boolean - if true, driver waits for autonegotiation on startup)
- gMarvellTokenSpaceGuid.PcdPhyDeviceIds
(list of values corresponding to MV_PHY_DEVICE_ID enum)
+ - gMarvellTokenSpaceGuid.PcdPhySmiAddresses
+ (addresses of PHY devices)
+ - gMarvellTokenSpaceGuid.PcdPhy2MdioController
+ (Array specifying, which Mdio controller the PHY is attached to)
-PHY_CONNECTION enum type is defined as follows:
-
- typedef enum {
- 0 PHY_CONNECTION_RGMII,
- 1 PHY_CONNECTION_RGMII_ID,
- 2 PHY_CONNECTION_RGMII_TXID,
- 3 PHY_CONNECTION_RGMII_RXID,
- 4 PHY_CONNECTION_SGMII,
- 5 PHY_CONNECTION_RTBI,
- 6 PHY_CONNECTION_XAUI,
- 7 PHY_CONNECTION_RXAUI
- } PHY_CONNECTION;
MV_PHY_DEVICE_ID:
@@ -152,11 +142,8 @@ MV_PHY_DEVICE_ID:
} MV_PHY_DEVICE_ID;
It should be extended when adding support for other PHY models.
-Thus in order to set RGMII for 1st PHY and SGMII for 2nd, PCD should be:
-
- gMarvellTokenSpaceGuid.PcdPhyConnectionTypes|{ 0x0, 0x4 }
-with disabled autonegotiation:
+Disable autonegotiation:
gMarvellTokenSpaceGuid.PcdPhyStartupAutoneg|FALSE
@@ -170,8 +157,9 @@ MDIO configuration
MDIO driver provides access to network PHYs' registers via EFI_MDIO_READ and
EFI_MDIO_WRITE functions (EFI_MDIO_PROTOCOL). Following PCD is required:
- - gMarvellTokenSpaceGuid.PcdMdioBaseAddress
- (base address of SMI management register)
+ - gMarvellTokenSpaceGuid.PcdMdioControllers
+ (Array with used controllers
+ Set to 0x1 for enabled, 0x0 for disabled)
I2C configuration
@@ -248,8 +236,26 @@ are required to operate:
- gMarvellTokenSpaceGuid.PcdPp2Port2Controller
(Array specifying, to which controller the port belongs to)
- - gMarvellTokenSpaceGuid.PcdPhySmiAddresses
- (Addresses of PHY devices)
+ - gMarvellTokenSpaceGuid.PcdPp2PhyConnectionTypes
+ (List of values corresponding to PHY_CONNECTION enum, which
+ is defined as follows:
+
+ typedef enum {
+ 0 PHY_CONNECTION_RGMII,
+ 1 PHY_CONNECTION_RGMII_ID,
+ 2 PHY_CONNECTION_RGMII_TXID,
+ 3 PHY_CONNECTION_RGMII_RXID,
+ 4 PHY_CONNECTION_SGMII,
+ 5 PHY_CONNECTION_RTBI,
+ 6 PHY_CONNECTION_XAUI,
+ 7 PHY_CONNECTION_RXAUI
+ } PHY_CONNECTION; )
+
+ - gMarvellTokenSpaceGuid.PcdPp2PhyIndexes
+ (Array specifying, to which PHY from
+ gMarvellTokenSpaceGuid.PcdPhyDeviceIds is used. If none,
+ e.g. in 10G SFI in-band link detection, 0xFF value must
+ be specified)
- gMarvellTokenSpaceGuid.PcdPp2PortIds
(Identificators of PP2 ports)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [platforms: PATCH 5/5] Platform/Marvell/Armada: Remove ParsePcdLib
2017-10-06 7:51 [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Marcin Wojtas
` (3 preceding siblings ...)
2017-10-06 7:51 ` [platforms: PATCH 4/5] Marvell/Drivers: Pp2Dxe: Rework PHY handling Marcin Wojtas
@ 2017-10-06 7:51 ` Marcin Wojtas
2017-10-06 16:02 ` Leif Lindholm
2017-10-06 14:29 ` [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Ard Biesheuvel
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 7:51 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
Current PCD handling in libraries and drivers allow to get
rid of this code.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada.dsc.inc | 1 -
Platform/Marvell/Include/Library/ParsePcdLib.h | 46 ----
Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c | 228 --------------------
Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf | 50 -----
4 files changed, 325 deletions(-)
diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index 9549091..679ba59 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -33,7 +33,6 @@
ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
MppLib|Platform/Marvell/Library/MppLib/MppLib.inf
- ParsePcdLib|Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf
UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
diff --git a/Platform/Marvell/Include/Library/ParsePcdLib.h b/Platform/Marvell/Include/Library/ParsePcdLib.h
deleted file mode 100644
index a255685..0000000
--- a/Platform/Marvell/Include/Library/ParsePcdLib.h
+++ /dev/null
@@ -1,46 +0,0 @@
-/********************************************************************************
-Copyright (C) 2016 Marvell International Ltd.
-
-Marvell BSD License Option
-
-If you received this File from Marvell, you may opt to use, redistribute and/or
-modify this File under the following licensing terms.
-Redistribution and use in source and binary forms, with or without modification,
-are permitted provided that the following conditions are met:
-
-* Redistributions of source code must retain the above copyright notice,
- this list of conditions and the following disclaimer.
-
-* Redistributions in binary form must reproduce the above copyright
- notice, this list of conditions and the following disclaimer in the
- documentation and/or other materials provided with the distribution.
-
-* Neither the name of Marvell nor the names of its contributors may be
- used to endorse or promote products derived from this software without
- specific prior written permission.
-
-THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
-ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
-WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
-DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
-ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
-(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
-LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
-ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
-SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-*******************************************************************************/
-
-#ifndef __PARSEPCDLIB_H__
-#define __PARSEPCDLIB_H__
-
-EFI_STATUS
-ParsePcdString (
- IN CHAR16 *PcdString,
- IN UINT8 Count,
- OUT UINTN *ValueTable,
- OUT CHAR16 **StrTable
- );
-
-#endif
diff --git a/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c b/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c
deleted file mode 100644
index 9a4be8e..0000000
--- a/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c
+++ /dev/null
@@ -1,228 +0,0 @@
-/********************************************************************************
-Copyright (C) 2016 Marvell International Ltd.
-
-Marvell BSD License Option
-
-If you received this File from Marvell, you may opt to use, redistribute and/or
-modify this File under the following licensing terms.
-Redistribution and use in source and binary forms, with or without modification,
-are permitted provided that the following conditions are met:
-
-* Redistributions of source code must Retain the above copyright notice,
- this list of conditions and the following disclaimer.
-
-* Redistributions in binary form must reproduce the above copyright
- notice, this list of conditions and the following disclaimer in the
- documentation and/or other materials provided with the distribution.
-
-* Neither the name of Marvell nor the names of its contributors may be
- used to endorse or promote products derived from this software without
- specific prior written permission.
-
-THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
-ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
-WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
-DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
-ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
-(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
-LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
-ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
-SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-*******************************************************************************/
-#define CHAR_NULL 0x0000
-
-#include <Library/BaseLib.h>
-#include <Uefi.h>
-#include <Library/UefiLib.h>
-#include <Library/DebugLib.h>
-
-STATIC
-CHAR16
-CharToUpper (
- IN CHAR16 Char
- )
-{
-
- if (Char >= L'a' && Char <= L'z') {
- return (CHAR16) (Char - (L'a' - L'A'));
- }
-
- return Char;
-}
-
-STATIC
-BOOLEAN
-IsDecimalDigitChar (
- IN CHAR16 Char
- )
-{
-
- return (BOOLEAN) (Char >= L'0' && Char <= L'9');
-}
-
-
-STATIC
-UINTN
-HexCharToUintn (
- IN CHAR16 Char
- )
-{
- if (IsDecimalDigitChar (Char)) {
- return Char - L'0';
- }
-
- return (UINTN) (10 + CharToUpper (Char) - L'A');
-}
-
-STATIC
-BOOLEAN
-IsHexDigitCharacter (
- CHAR16 Char
- )
-{
-
- return (BOOLEAN) ((Char >= L'0' && Char <= L'9') || (Char >= L'A' &&
- Char <= L'F') || (Char >= L'a' && Char <= L'f'));
-}
-
-STATIC
-UINTN
-HexStrToUintn (
- CHAR16 *String
- )
-{
- UINTN Result = 0;
-
- if (String == NULL || StrSize(String) == 0) {
- return (UINTN)(-1);
- }
-
- // Ignore spaces and tabs
- while ((*String == L' ') || (*String == L'\t')) {
- String++;
- }
-
- // Ignore leading zeros after spaces
- while (*String == L'0') {
- String++;
- }
-
- if (CharToUpper (*String) != L'X') {
- return (UINTN)(-1);
- }
-
- // Skip 'x'
- String++;
-
- while (IsHexDigitCharacter (*String)) {
- Result <<= 4;
- Result += HexCharToUintn (*String);
- String++;
- }
-
- return (UINTN) Result;
-}
-
-STATIC
-UINTN
-DecimalStrToUintn (
- CHAR16 *String
- )
-{
- UINTN Result = 0;
-
- while (IsDecimalDigitChar (*String)) {
- Result = 10 * Result + (*String - L'0');
- String++;
- }
-
- return Result;
-}
-
-STATIC
-UINTN
-StrToUintn (
- CHAR16 *String
- )
-{
- CHAR16 *Walker;
-
- // Chop off leading spaces
- for (Walker = String; Walker != NULL && *Walker != CHAR_NULL && *Walker == L' '; Walker++);
-
- if (StrnCmp(Walker, L"0x", 2) == 0 || StrnCmp(Walker, L"0X", 2) == 0) {
- return HexStrToUintn (Walker);
- } else {
- return DecimalStrToUintn (Walker);
- }
-}
-
-EFI_STATUS
-ParsePcdString (
- IN CHAR16 *PcdString,
- IN UINT8 Count,
- OUT UINTN *ValueTable,
- OUT CHAR16 **StrTable
- )
-{
- BOOLEAN ValueFlag = FALSE;
- CHAR16 *Walker;
- UINTN i, Tmp = 0;
-
- if (ValueTable != NULL) {
- ValueFlag = TRUE;
- }
-
- // Set pointer at the end of PCD string
- Walker = PcdString + StrLen (PcdString);
- for (i = 0; i < Count; i++) {
- while ((--Walker) >= PcdString) {
- if (*Walker == L';') {
- // Cut off parsed chunk from PCD string by replacing ';' with
- // null-terminator
- *Walker = '\0';
- if (ValueFlag) {
- Tmp = StrToUintn ((Walker + 1));
- if ((UINTN)(-1) == Tmp) {
- return EFI_INVALID_PARAMETER;
- }
- // Entry is parsed from the end to the beginning
- // so fill table in the same manner
- ValueTable[Count - (i + 1)] = Tmp;
- } else {
- StrTable[Count - (i + 1)] = Walker + 1;
- }
- Walker--;
- break;
- }
- if (Walker == PcdString) {
- if (ValueFlag) {
- Tmp = StrToUintn ((Walker));
- if (Tmp == (UINTN)(-1)) {
- return EFI_INVALID_PARAMETER;
- }
- }
- // Last device's entry should be added to the table here.
- // If not, return error
- if (i != (Count - 1)) {
- DEBUG((DEBUG_ERROR, "ParsePcdLib: Please set PCD value for every "
- "device\n"));
- return EFI_INVALID_PARAMETER;
- }
- // We parse from the end to the beginning
- // so fill table in the same manner
- if (ValueFlag) {
- ValueTable[Count - (i + 1)] = Tmp;
- } else {
- StrTable[Count - (i + 1)] = Walker;
- }
- // End both loops
- return EFI_SUCCESS;
- }
- }
- }
-
- return EFI_SUCCESS;
-}
diff --git a/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf b/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf
deleted file mode 100644
index b4db621..0000000
--- a/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf
+++ /dev/null
@@ -1,50 +0,0 @@
-# Copyright (C) 2016 Marvell International Ltd.
-#
-# Marvell BSD License Option
-#
-# If you received this File from Marvell, you may opt to use, redistribute and/or
-# modify this File under the following licensing terms.
-# Redistribution and use in source and binary forms, with or without modification,
-# are permitted provided that the following conditions are met:
-#
-# * Redistributions of source code must retain the above copyright notice,
-# this list of conditions and the following disclaimer.
-#
-# * Redistributions in binary form must reproduce the above copyright
-# notice, this list of conditions and the following disclaimer in the
-# documentation and/or other materials provided with the distribution.
-#
-# * Neither the name of Marvell nor the names of its contributors may be
-# used to endorse or promote products derived from this software without
-# specific prior written permission.
-#
-# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
-# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
-# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
-# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
-# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
-# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
-# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
-# ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
-# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-#
-
-[Defines]
- INF_VERSION = 0x00010005
- BASE_NAME = ParsePcdLib
- FILE_GUID = 698d85a0-a952-453e-b8a4-1d6ea338a38e
- MODULE_TYPE = BASE
- VERSION_STRING = 1.0
- LIBRARY_CLASS = ParsePcdLib
-
-[Packages]
- MdePkg/MdePkg.dec
- MdeModulePkg/MdeModulePkg.dec
-
-[LibraryClasses]
- ArmLib
- DebugLib
-
-[Sources.common]
- ParsePcdLib.c
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal
2017-10-06 7:51 [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Marcin Wojtas
` (4 preceding siblings ...)
2017-10-06 7:51 ` [platforms: PATCH 5/5] Platform/Marvell/Armada: Remove ParsePcdLib Marcin Wojtas
@ 2017-10-06 14:29 ` Ard Biesheuvel
5 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-10-06 14:29 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel@lists.01.org, Leif Lindholm, Nadav Haklai,
Neta Zur Hershkovits, Kostya Porotchkin, Hua Jing,
Jan Dąbroś
On 6 October 2017 at 08:51, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi,
>
> This patchset purpose is complete removal of ParsePcdLib and
> cleanup of boards' PCD representation. More details can be
> found in the commit logs.
>
> The patches are available in the github:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/parsepcd-upstream-r20171005
>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing
2017-10-06 7:51 ` [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing Marcin Wojtas
@ 2017-10-06 15:52 ` Leif Lindholm
2017-10-06 19:22 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-10-06 15:52 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib
2017-10-06 7:51 ` [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib Marcin Wojtas
@ 2017-10-06 15:56 ` Leif Lindholm
2017-10-06 19:24 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-10-06 15:56 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Fri, Oct 06, 2017 at 09:51:15AM +0200, Marcin Wojtas wrote:
> This patch introduces I2c description, using the new structures
> and template in MvHwDescLib. This change enables more flexible
> addition of multiple I2c controllers and also allows for
> removal of string PCD parsing. Update Armada 70x0 DB description
> and PortingGuide accordingly.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Armada/Armada70x0.dsc | 2 +-
> Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 42 +++++++++++---------
> Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf | 3 +-
> Platform/Marvell/Include/Library/MvHwDescLib.h | 25 ++++++++++++
> Platform/Marvell/Marvell.dec | 2 +-
> Silicon/Marvell/Documentation/PortingGuide.txt | 4 +-
> 6 files changed, 54 insertions(+), 24 deletions(-)
>
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 7b03744..d9d126d 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -78,7 +78,7 @@
> # I2C
> gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses|{ 0x50, 0x57, 0x60 }
> gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0, 0x0, 0x0 }
> - gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|L"0xF2701000;0xF2701100"
> + gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x1, 0x1 }
Similarly to the previous patch, the above is a bit opaque.
I guess in this case, this is simply a boolean array saying whether a
specific controller is enabled or not?
If my interpretation is correct, could the Pcd be renamed
PcdI2cControllersEnabled?
/
Leif
> gMarvellTokenSpaceGuid.PcdEepromI2cAddresses|{ 0x50, 0x57 }
> gMarvellTokenSpaceGuid.PcdEepromI2cBuses|{ 0x0, 0x0 }
> gMarvellTokenSpaceGuid.PcdI2cClockFrequency|250000000
> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> index fa79ebc..ff8006e 100755
> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> @@ -42,12 +42,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> #include <Library/DebugLib.h>
> #include <Library/PcdLib.h>
> #include <Library/UefiLib.h>
> -#include <Library/ParsePcdLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/MvHwDescLib.h>
> #include <Library/UefiBootServicesTableLib.h>
>
> #include "MvI2cDxe.h"
>
> +DECLARE_A7K8K_I2C_TEMPLATE;
> +
> STATIC MV_I2C_BAUD_RATE baud_rate;
>
> STATIC MV_I2C_DEVICE_PATH MvI2cDevicePathProtocol = {
> @@ -172,35 +174,39 @@ MvI2cInitialise (
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> + MVHW_I2C_DESC *Desc = &mA7k8kI2cDescTemplate;
> + UINT8 *I2cDeviceTable, Index;
> EFI_STATUS Status;
> - UINT32 BusCount;
> - EFI_PHYSICAL_ADDRESS I2cBaseAddresses[PcdGet32 (PcdI2cBusCount)];
> - INTN i;
>
> - BusCount = PcdGet32 (PcdI2cBusCount);
> - if (BusCount == 0)
> - return EFI_SUCCESS;
> + /* Obtain table with enabled I2c devices */
> + I2cDeviceTable = (UINT8 *)PcdGetPtr (PcdI2cControllers);
> + if (I2cDeviceTable == NULL) {
> + DEBUG ((DEBUG_ERROR, "Missing PcdI2cControllers\n"));
> + return EFI_INVALID_PARAMETER;
> + }
>
> - Status = ParsePcdString (
> - (CHAR16 *) PcdGetPtr (PcdI2cBaseAddresses),
> - BusCount,
> - I2cBaseAddresses,
> - NULL
> - );
> - if (EFI_ERROR(Status))
> - return Status;
> + if (PcdGetSize (PcdI2cControllers) > MVHW_MAX_I2C_DEVS) {
> + DEBUG ((DEBUG_ERROR, "Wrong PcdI2cControllers format\n"));
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + /* Initialize enabled chips */
> + for (Index = 0; Index < PcdGetSize (PcdI2cControllers); Index++) {
> + if (!MVHW_DEV_ENABLED (I2c, Index)) {
> + DEBUG ((DEBUG_ERROR, "Skip I2c chip %d\n", Index));
> + continue;
> + }
>
> - for (i = 0; i < BusCount; i++) {
> Status = MvI2cInitialiseController(
> ImageHandle,
> SystemTable,
> - I2cBaseAddresses[i]
> + Desc->I2cBaseAddresses[Index]
> );
> if (EFI_ERROR(Status))
> return Status;
> }
>
> - return Status;
> + return EFI_SUCCESS;
> }
>
> STATIC
> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
> index 16374ef..c453b4f 100755
> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
> @@ -55,7 +55,6 @@
> UefiLib
> UefiDriverEntryPoint
> UefiBootServicesTableLib
> - ParsePcdLib
>
> [Protocols]
> gEfiI2cMasterProtocolGuid
> @@ -66,7 +65,7 @@
> [Pcd]
> gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses
> gMarvellTokenSpaceGuid.PcdI2cSlaveBuses
> - gMarvellTokenSpaceGuid.PcdI2cBaseAddresses
> + gMarvellTokenSpaceGuid.PcdI2cControllers
> gMarvellTokenSpaceGuid.PcdI2cClockFrequency
> gMarvellTokenSpaceGuid.PcdI2cBaudRate
> gMarvellTokenSpaceGuid.PcdI2cBusCount
> diff --git a/Platform/Marvell/Include/Library/MvHwDescLib.h b/Platform/Marvell/Include/Library/MvHwDescLib.h
> index 6a86865..e029b50 100644
> --- a/Platform/Marvell/Include/Library/MvHwDescLib.h
> +++ b/Platform/Marvell/Include/Library/MvHwDescLib.h
> @@ -60,6 +60,16 @@ typedef struct {
> } MVHW_COMPHY_DESC;
>
> //
> +// I2C devices description template definition
> +//
> +#define MVHW_MAX_I2C_DEVS 4
> +
> +typedef struct {
> + UINT8 I2cDevCount;
> + UINTN I2cBaseAddresses[MVHW_MAX_I2C_DEVS];
> +} MVHW_I2C_DESC;
> +
> +//
> // NonDiscoverable devices description template definition
> //
> #define MVHW_MAX_XHCI_DEVS 4
> @@ -130,6 +140,21 @@ MVHW_COMPHY_DESC mA7k8kComPhyDescTemplate = {\
> }
>
> //
> +// Platform description of I2C devices
> +//
> +#define MVHW_CP0_I2C0_BASE 0xF2701000
> +#define MVHW_CP0_I2C1_BASE 0xF2701100
> +#define MVHW_CP1_I2C0_BASE 0xF4701000
> +#define MVHW_CP1_I2C1_BASE 0xF4701100
> +
> +#define DECLARE_A7K8K_I2C_TEMPLATE \
> +STATIC \
> +MVHW_I2C_DESC mA7k8kI2cDescTemplate = {\
> + 4,\
> + { MVHW_CP0_I2C0_BASE, MVHW_CP0_I2C1_BASE, MVHW_CP1_I2C0_BASE, MVHW_CP1_I2C1_BASE }\
> +}
> +
> +//
> // Platform description of NonDiscoverable devices
> //
> #define MVHW_CP0_XHCI0_BASE 0xF2500000
> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> index fc00f1a..25a4258 100644
> --- a/Platform/Marvell/Marvell.dec
> +++ b/Platform/Marvell/Marvell.dec
> @@ -113,7 +113,7 @@
> gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0 }|VOID*|0x3000184
> gMarvellTokenSpaceGuid.PcdEepromI2cAddresses|{ 0x0 }|VOID*|0x3000050
> gMarvellTokenSpaceGuid.PcdEepromI2cBuses|{ 0x0 }|VOID*|0x3000185
> - gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|{ 0x0 }|VOID*|0x3000047
> + gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x0 }|VOID*|0x3000047
> gMarvellTokenSpaceGuid.PcdI2cClockFrequency|0|UINT32|0x3000048
> gMarvellTokenSpaceGuid.PcdI2cBaudRate|0|UINT32|0x3000049
> gMarvellTokenSpaceGuid.PcdI2cBusCount|0|UINT32|0x3000183
> diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
> index 47a667d..1a30c46 100644
> --- a/Silicon/Marvell/Documentation/PortingGuide.txt
> +++ b/Silicon/Marvell/Documentation/PortingGuide.txt
> @@ -188,8 +188,8 @@ In order to enable driver on a new platform, following steps need to be taken:
> (buses to which accoring slaves are attached)
> - gMarvellTokenSpaceGuid.PcdI2cBusCount|2
> (number of SoC's I2C buses)
> - - gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|L"0xF2701000;0xF2701100"
> - (base addresses of I2C controller buses)
> + - gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x1, 0x1 }
> + (array with used controllers)
> - gMarvellTokenSpaceGuid.PcdI2cClockFrequency|200000000
> (I2C host controller clock frequency)
> - gMarvellTokenSpaceGuid.PcdI2cBaudRate|100000
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 3/5] Marvell/Library: UtmiLib: Move devices description to MvHwDescLib
2017-10-06 7:51 ` [platforms: PATCH 3/5] Marvell/Library: UtmiLib: " Marcin Wojtas
@ 2017-10-06 15:57 ` Leif Lindholm
2017-10-06 19:26 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-10-06 15:57 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Fri, Oct 06, 2017 at 09:51:16AM +0200, Marcin Wojtas wrote:
> This patch introduces UTMI description, using the new structures
> and template in MvHwDescLib. This change enables more flexible
> addition of multiple CP with UTMI PHY's and also significantly
> reduces amount of used PCD's for that purpose. Update PortingGuide
> documentation accordingly.
>
> This patch replaces string-based description of Utmi on
> Armada 70x0 DB with new, reduced format.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Armada/Armada70x0.dsc | 7 +-
> Platform/Marvell/Include/Library/MvHwDescLib.h | 47 ++++++
> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c | 149 ++++++++++----------
> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h | 1 -
> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf | 11 +-
> Platform/Marvell/Marvell.dec | 7 +-
> Silicon/Marvell/Documentation/PortingGuide.txt | 30 ++--
> 7 files changed, 142 insertions(+), 110 deletions(-)
>
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index d9d126d..04bdf7c 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -111,11 +111,8 @@
> gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds|{ 0x1, 0x6, 0xA, 0x6, 0x6, 0x6 }
>
> #UtmiPhy
> - gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|L"0xF2440420;0xF2440420"
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg|L"0xF2440440;0xF2440444"
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit|L"0xF2580000;0xF2581000"
> - gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort|L"0x0;0x1"
> + gMarvellTokenSpaceGuid.PcdUtmiControllers|{ 0x1, 0x1 }
> + gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0, 0x1 }
Like for 1/5, could we have some entries under a [Define] in a .inc
(reasonably the same .inc)?
/
Leif
>
> #MDIO
> gMarvellTokenSpaceGuid.PcdMdioBaseAddress|0xF212A200
> diff --git a/Platform/Marvell/Include/Library/MvHwDescLib.h b/Platform/Marvell/Include/Library/MvHwDescLib.h
> index e029b50..6ad1bc2 100644
> --- a/Platform/Marvell/Include/Library/MvHwDescLib.h
> +++ b/Platform/Marvell/Include/Library/MvHwDescLib.h
> @@ -117,6 +117,19 @@ typedef struct {
> } MVHW_RTC_DESC;
>
> //
> +// UTMI PHY's description template definition
> +//
> +
> +typedef struct {
> + UINT8 UtmiDevCount;
> + UINT32 UtmiPhyId[MVHW_MAX_XHCI_DEVS];
> + UINTN UtmiBaseAddresses[MVHW_MAX_XHCI_DEVS];
> + UINTN UtmiConfigAddresses[MVHW_MAX_XHCI_DEVS];
> + UINTN UtmiUsbConfigAddresses[MVHW_MAX_XHCI_DEVS];
> + UINTN UtmiMuxBitCount[MVHW_MAX_XHCI_DEVS];
> +} MVHW_UTMI_DESC;
> +
> +//
> // Platform description of CommonPhy devices
> //
> #define MVHW_CP0_COMPHY_BASE 0xF2441000
> @@ -217,4 +230,38 @@ MVHW_RTC_DESC mA7k8kRtcDescTemplate = {\
> { SIZE_4KB, SIZE_4KB }\
> }
>
> +//
> +// Platform description of UTMI PHY's
> +//
> +#define MVHW_CP0_UTMI0_BASE 0xF2580000
> +#define MVHW_CP0_UTMI0_CFG_BASE 0xF2440440
> +#define MVHW_CP0_UTMI0_USB_CFG_BASE 0xF2440420
> +#define MVHW_CP0_UTMI0_ID 0x0
> +#define MVHW_CP0_UTMI1_BASE 0xF2581000
> +#define MVHW_CP0_UTMI1_CFG_BASE 0xF2440444
> +#define MVHW_CP0_UTMI1_USB_CFG_BASE 0xF2440420
> +#define MVHW_CP0_UTMI1_ID 0x1
> +#define MVHW_CP1_UTMI0_BASE 0xF4580000
> +#define MVHW_CP1_UTMI0_CFG_BASE 0xF4440440
> +#define MVHW_CP1_UTMI0_USB_CFG_BASE 0xF4440420
> +#define MVHW_CP1_UTMI0_ID 0x0
> +#define MVHW_CP1_UTMI1_BASE 0xF4581000
> +#define MVHW_CP1_UTMI1_CFG_BASE 0xF4440444
> +#define MVHW_CP1_UTMI1_USB_CFG_BASE 0xF4440420
> +#define MVHW_CP1_UTMI1_ID 0x1
> +
> +#define DECLARE_A7K8K_UTMI_TEMPLATE \
> +STATIC \
> +MVHW_UTMI_DESC mA7k8kUtmiDescTemplate = {\
> + 4,\
> + { MVHW_CP0_UTMI0_ID, MVHW_CP0_UTMI1_ID,\
> + MVHW_CP1_UTMI0_ID, MVHW_CP1_UTMI1_ID },\
> + { MVHW_CP0_UTMI0_BASE, MVHW_CP0_UTMI1_BASE,\
> + MVHW_CP1_UTMI0_BASE, MVHW_CP1_UTMI1_BASE },\
> + { MVHW_CP0_UTMI0_CFG_BASE, MVHW_CP0_UTMI1_CFG_BASE,\
> + MVHW_CP1_UTMI0_CFG_BASE, MVHW_CP1_UTMI1_CFG_BASE },\
> + { MVHW_CP0_UTMI0_USB_CFG_BASE, MVHW_CP0_UTMI1_USB_CFG_BASE,\
> + MVHW_CP1_UTMI0_USB_CFG_BASE, MVHW_CP1_UTMI1_USB_CFG_BASE }\
> +}
> +
> #endif /* __MVHWDESCLIB_H__ */
> diff --git a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c
> index 95b5698..b07c038 100644
> --- a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c
> +++ b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c
> @@ -33,12 +33,16 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> *******************************************************************************/
>
> #include "UtmiPhyLib.h"
> +#include <Library/MvHwDescLib.h>
> +
> +DECLARE_A7K8K_UTMI_TEMPLATE;
>
> typedef struct {
> EFI_PHYSICAL_ADDRESS UtmiBaseAddr;
> EFI_PHYSICAL_ADDRESS UsbCfgAddr;
> EFI_PHYSICAL_ADDRESS UtmiCfgAddr;
> UINT32 UtmiPhyPort;
> + UINT32 PhyId;
> } UTMI_PHY_DATA;
>
> STATIC
> @@ -236,48 +240,52 @@ UtmiPhyPowerUp (
> STATIC
> VOID
> Cp110UtmiPhyInit (
> - IN UINT32 UtmiPhyCount,
> IN UTMI_PHY_DATA *UtmiData
> )
> {
> - UINT32 i;
> + EFI_STATUS Status;
>
> - for (i = 0; i < UtmiPhyCount; i++) {
> - UtmiPhyPowerDown(i, UtmiData[i].UtmiBaseAddr,
> - UtmiData[i].UsbCfgAddr, UtmiData[i].UtmiCfgAddr,
> - UtmiData[i].UtmiPhyPort);
> - }
> + UtmiPhyPowerDown (
> + UtmiData->PhyId,
> + UtmiData->UtmiBaseAddr,
> + UtmiData->UsbCfgAddr,
> + UtmiData->UtmiCfgAddr,
> + UtmiData->UtmiPhyPort
> + );
>
> /* Power down PLL */
> DEBUG((DEBUG_INFO, "UtmiPhy: stage: PHY power down PLL\n"));
> - RegSet (UtmiData[0].UsbCfgAddr, 0x0 << UTMI_USB_CFG_PLL_OFFSET,
> - UTMI_USB_CFG_PLL_MASK);
> -
> - for (i = 0; i < UtmiPhyCount; i++) {
> - UtmiPhyConfig(i, UtmiData[i].UtmiBaseAddr,
> - UtmiData[i].UsbCfgAddr, UtmiData[i].UtmiCfgAddr,
> - UtmiData[i].UtmiPhyPort);
> + MmioAnd32 (UtmiData->UsbCfgAddr, ~UTMI_USB_CFG_PLL_MASK);
> +
> + UtmiPhyConfig (
> + UtmiData->PhyId,
> + UtmiData->UtmiBaseAddr,
> + UtmiData->UsbCfgAddr,
> + UtmiData->UtmiCfgAddr,
> + UtmiData->UtmiPhyPort
> + );
> +
> + Status = UtmiPhyPowerUp (
> + UtmiData->PhyId,
> + UtmiData->UtmiBaseAddr,
> + UtmiData->UsbCfgAddr,
> + UtmiData->UtmiCfgAddr,
> + UtmiData->UtmiPhyPort
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "UtmiPhy: Failed to initialize UTMI PHY %d\n", UtmiData->PhyId));
> + return;
> }
>
> - for (i = 0; i < UtmiPhyCount; i++) {
> - if (EFI_ERROR(UtmiPhyPowerUp(i, UtmiData[i].UtmiBaseAddr,
> - UtmiData[i].UsbCfgAddr, UtmiData[i].UtmiCfgAddr,
> - UtmiData[i].UtmiPhyPort))) {
> - DEBUG((DEBUG_ERROR, "UtmiPhy: Failed to initialize UTMI PHY %d\n", i));
> - continue;
> - }
> - DEBUG((DEBUG_ERROR, "UTMI PHY %d initialized to ", i));
> -
> - if (UtmiData[i].UtmiPhyPort == UTMI_PHY_TO_USB_DEVICE0)
> - DEBUG((DEBUG_ERROR, "USB Device\n"));
> - else
> - DEBUG((DEBUG_ERROR, "USB Host%d\n", UtmiData[i].UtmiPhyPort));
> - }
> + DEBUG ((DEBUG_ERROR, "UTMI PHY %d initialized to ", UtmiData->PhyId));
> + if (UtmiData->UtmiPhyPort == UTMI_PHY_TO_USB_DEVICE0)
> + DEBUG((DEBUG_ERROR, "USB Device\n"));
> + else
> + DEBUG((DEBUG_ERROR, "USB Host%d\n", UtmiData->UtmiPhyPort));
>
> /* Power up PLL */
> DEBUG((DEBUG_INFO, "UtmiPhy: stage: PHY power up PLL\n"));
> - RegSet (UtmiData[0].UsbCfgAddr, 0x1 << UTMI_USB_CFG_PLL_OFFSET,
> - UTMI_USB_CFG_PLL_MASK);
> + MmioOr32 (UtmiData->UsbCfgAddr, UTMI_USB_CFG_PLL_MASK);
> }
>
> EFI_STATUS
> @@ -285,69 +293,66 @@ UtmiPhyInit (
> VOID
> )
> {
> - EFI_STATUS Status;
> - UTMI_PHY_DATA UtmiData[PcdGet32 (PcdUtmiPhyCount)];
> - EFI_PHYSICAL_ADDRESS RegUtmiUnit[PcdGet32 (PcdUtmiPhyCount)];
> - EFI_PHYSICAL_ADDRESS RegUsbCfg[PcdGet32 (PcdUtmiPhyCount)];
> - EFI_PHYSICAL_ADDRESS RegUtmiCfg[PcdGet32 (PcdUtmiPhyCount)];
> - UINTN UtmiPort[PcdGet32 (PcdUtmiPhyCount)];
> - UINTN i, Count;
> -
> - Count = PcdGet32 (PcdUtmiPhyCount);
> - if (Count == 0) {
> + UTMI_PHY_DATA UtmiData;
> + UINT8 *UtmiDeviceTable, *XhciDeviceTable, *UtmiPortType, Index;
> + MVHW_UTMI_DESC *Desc = &mA7k8kUtmiDescTemplate;
> +
> + /* Obtain table with enabled Utmi PHY's*/
> + UtmiDeviceTable = (UINT8 *)PcdGetPtr (PcdUtmiControllers);
> + if (UtmiDeviceTable == NULL) {
> /* No UTMI PHY on platform */
> return EFI_SUCCESS;
> }
>
> - DEBUG((DEBUG_INFO, "UtmiPhy: Initialize USB UTMI PHYs\n"));
> - /* Parse UtmiPhy PCDs */
> - Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdUtmiPhyRegUtmiUnit),
> - Count, RegUtmiUnit, NULL);
> - if (EFI_ERROR(Status)) {
> - DEBUG((DEBUG_ERROR, "UtmiPhy: Wrong PcdUtmiPhyRegUtmiUnit format\n"));
> + if (PcdGetSize (PcdUtmiControllers) > MVHW_MAX_XHCI_DEVS) {
> + DEBUG ((DEBUG_ERROR, "UTMI: Wrong PcdUtmiControllers format\n"));
> return EFI_INVALID_PARAMETER;
> }
>
> - Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdUtmiPhyRegUsbCfg),
> - Count, RegUsbCfg, NULL);
> - if (EFI_ERROR(Status)) {
> - DEBUG((DEBUG_ERROR, "UtmiPhy: Wrong PcdUtmiPhyRegUsbCfg format\n"));
> + /* Make sure XHCI controllers table is present */
> + XhciDeviceTable = (UINT8 *)PcdGetPtr (PcdPciEXhci);
> + if (XhciDeviceTable == NULL) {
> + DEBUG ((DEBUG_ERROR, "UTMI: Missing PcdPciEXhci\n"));
> return EFI_INVALID_PARAMETER;
> }
>
> - Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdUtmiPhyRegUtmiCfg),
> - Count, RegUtmiCfg, NULL);
> - if (EFI_ERROR(Status)) {
> - DEBUG((DEBUG_ERROR, "UtmiPhy: Wrong PcdUtmiPhyRegUtmiCfg format\n"));
> + /* Obtain port type table */
> + UtmiPortType = (UINT8 *)PcdGetPtr (PcdUtmiPortType);
> + if (UtmiPortType == NULL || PcdGetSize (PcdUtmiPortType) != PcdGetSize (PcdUtmiControllers)) {
> + DEBUG ((DEBUG_ERROR, "UTMI: Wrong PcdUtmiPortType format\n"));
> return EFI_INVALID_PARAMETER;
> }
>
> - Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdUtmiPhyUtmiPort),
> - Count, UtmiPort, NULL);
> - if (EFI_ERROR(Status)) {
> - DEBUG((DEBUG_ERROR, "UtmiPhy: Wrong PcdUtmiPhyUtmiPort format\n"));
> - return EFI_INVALID_PARAMETER;
> - }
> + /* Initialize enabled chips */
> + for (Index = 0; Index < PcdGetSize (PcdUtmiControllers); Index++) {
> + if (!MVHW_DEV_ENABLED (Utmi, Index)) {
> + continue;
> + }
> +
> + /* UTMI PHY without enabled XHCI controller is useless */
> + if (!MVHW_DEV_ENABLED (Xhci, Index)) {
> + DEBUG ((DEBUG_ERROR, "UTMI: Disabled Xhci controller %d\n", Index));
> + return EFI_INVALID_PARAMETER;
> + }
>
> - for (i = 0 ; i < Count ; i++) {
> /* Get base address of UTMI phy */
> - UtmiData[i].UtmiBaseAddr = RegUtmiUnit[i];
> + UtmiData.UtmiBaseAddr = Desc->UtmiBaseAddresses[Index];
>
> /* Get usb config address */
> - UtmiData[i].UsbCfgAddr = RegUsbCfg[i];
> + UtmiData.UsbCfgAddr = Desc->UtmiUsbConfigAddresses[Index];
>
> /* Get UTMI config address */
> - UtmiData[i].UtmiCfgAddr = RegUtmiCfg[i];
> + UtmiData.UtmiCfgAddr = Desc->UtmiConfigAddresses[Index];
>
> - /*
> - * Get the usb port number, which will be used to check if
> - * the utmi connected to host or device
> - */
> - UtmiData[i].UtmiPhyPort = UtmiPort[i];
> - }
> + /* Get UTMI PHY ID */
> + UtmiData.PhyId = Desc->UtmiPhyId[Index];
>
> - /* Currently only Cp110 is supported */
> - Cp110UtmiPhyInit (Count, UtmiData);
> + /* Get the usb port type */
> + UtmiData.UtmiPhyPort = UtmiPortType[Index];
> +
> + /* Currently only Cp110 is supported */
> + Cp110UtmiPhyInit (&UtmiData);
> + }
>
> return EFI_SUCCESS;
> }
> diff --git a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h
> index f9b4933..0d7d72e 100644
> --- a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h
> +++ b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h
> @@ -42,7 +42,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> #include <Library/MemoryAllocationLib.h>
> #include <Library/IoLib.h>
> #include <Library/TimerLib.h>
> -#include <Library/ParsePcdLib.h>
>
> #define UTMI_USB_CFG_DEVICE_EN_OFFSET 0
> #define UTMI_USB_CFG_DEVICE_EN_MASK (0x1 << UTMI_USB_CFG_DEVICE_EN_OFFSET)
> diff --git a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
> index f1e57f4..6e33cdd 100644
> --- a/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
> +++ b/Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
> @@ -50,15 +50,12 @@
> DebugLib
> IoLib
> MemoryAllocationLib
> - ParsePcdLib
> PcdLib
>
> [Sources.common]
> UtmiPhyLib.c
>
> -[FixedPcd]
> - gMarvellTokenSpaceGuid.PcdUtmiPhyCount
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit
> - gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort
> +[Pcd]
> + gMarvellTokenSpaceGuid.PcdUtmiControllers
> + gMarvellTokenSpaceGuid.PcdUtmiPortType
> + gMarvellTokenSpaceGuid.PcdPciEXhci
> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> index 25a4258..00d99fa 100644
> --- a/Platform/Marvell/Marvell.dec
> +++ b/Platform/Marvell/Marvell.dec
> @@ -156,11 +156,8 @@
> gMarvellTokenSpaceGuid.PcdChip3ComPhyInvFlags|{ 0x0 }|VOID*|0x30000177
>
> #UtmiPhy
> - gMarvellTokenSpaceGuid.PcdUtmiPhyCount|0|UINT32|0x30000205
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|{ 0x0 }|VOID*|0x30000206
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg|{ 0x0 }|VOID*|0x30000207
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit|{ 0x0 }|VOID*|0x30000208
> - gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort|{ 0x0 }|VOID*|0x30000209
> + gMarvellTokenSpaceGuid.PcdUtmiControllers|{ 0x0 }|VOID*|0x30000206
> + gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0 }|VOID*|0x30000207
>
> #MDIO
> gMarvellTokenSpaceGuid.PcdMdioBaseAddress|0|UINT64|0x3000043
> diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
> index 1a30c46..8a9f603 100644
> --- a/Silicon/Marvell/Documentation/PortingGuide.txt
> +++ b/Silicon/Marvell/Documentation/PortingGuide.txt
> @@ -278,33 +278,23 @@ UTMI PHY configuration
> ======================
> In order to configure UTMI, following PCDs are available:
>
> - - gMarvellTokenSpaceGuid.PcdUtmiPhyCount
> - (Indicates how many UTMI PHYs are available on platform)
> -
> -Next four PCDs are in unicode string format containing settings for all devices
> -separated with semicolon.
> -
> - - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit
> - (Indicates base address of the UTMI unit)
> -
> - - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg
> - (Indicates address of USB Configuration register)
> + - gMarvellTokenSpaceGuid.PcdUtmiControllers
> + (Array with used controllers
> + Set to 0x1 for enabled, 0x0 for disabled)
>
> - - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg
> - (Indicates address of external UTMI configuration)
> + - gMarvellTokenSpaceGuid.PcdUtmiPortType
> + (Indicates type of the connected USB port:
>
> - - gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort
> - (Indicates type of the connected USB port)
> + UTMI_PHY_TO_USB_HOST0 0
> + UTMI_PHY_TO_USB_HOST1 1
> + UTMI_PHY_TO_USB_DEVICE0 2 )
>
> Example
> -------
>
> # UtmiPhy
> - gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit|L"0xF2580000;0xF2581000"
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|L"0xF2440420;0xF2440420"
> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg|L"0xF2440440;0xF2440444"
> - gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort|L"0x0;0x1"
> + gMarvellTokenSpaceGuid.PcdUtmiControllers|{ 0x1, 0x1 }
> + gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0, 0x1 }
>
>
> SPI driver configuration
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 4/5] Marvell/Drivers: Pp2Dxe: Rework PHY handling
2017-10-06 7:51 ` [platforms: PATCH 4/5] Marvell/Drivers: Pp2Dxe: Rework PHY handling Marcin Wojtas
@ 2017-10-06 16:02 ` Leif Lindholm
2017-10-06 19:30 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-10-06 16:02 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Fri, Oct 06, 2017 at 09:51:17AM +0200, Marcin Wojtas wrote:
> Hitherto PHY handling in Pp2Dxe was not flexible. It allowed for using
> only single MDIO controller, which may not be true on Armada 80x0 SoCs.
> For this purpose introduce the MDIO description, using the new structures
> and template in MvHwDescLib. This change enables addition of multiple
> CP110 hardware blocks with MDIO controllers.
>
> This change required different PHY handling and obtaining data over
> desired MDIO bus. Now given Pp2 port is matched with the PHY via
> its index in gMarvellTokenSpaceGuid.PcdPhyDeviceIds. The PHY itself
> is mapped to the MDIO controller, using
> gMarvellTokenSpaceGuid.PcdPhy2MdioController. Also obtaining
> SMI addresses was moved to the PHY initialization routine.
> All above allow for much cleaner and logical PHY description
> in the .dsc file.
>
> Update PortingGuide documentation accordingly and Armada 70x0 DB
> NIC/PHY description.
Like for previous patches, can we have:
- symbolic names for things that can be given symbolic names.
- Pcds describing boolean arrays have an "Enabled" appended to their
name?
/
Leif
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 5/5] Platform/Marvell/Armada: Remove ParsePcdLib
2017-10-06 7:51 ` [platforms: PATCH 5/5] Platform/Marvell/Armada: Remove ParsePcdLib Marcin Wojtas
@ 2017-10-06 16:02 ` Leif Lindholm
0 siblings, 0 replies; 21+ messages in thread
From: Leif Lindholm @ 2017-10-06 16:02 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Fri, Oct 06, 2017 at 09:51:18AM +0200, Marcin Wojtas wrote:
> Current PCD handling in libraries and drivers allow to get
> rid of this code.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
No objections here :)
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> Platform/Marvell/Armada/Armada.dsc.inc | 1 -
> Platform/Marvell/Include/Library/ParsePcdLib.h | 46 ----
> Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c | 228 --------------------
> Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf | 50 -----
> 4 files changed, 325 deletions(-)
>
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
> index 9549091..679ba59 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -33,7 +33,6 @@
> ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
> MppLib|Platform/Marvell/Library/MppLib/MppLib.inf
> - ParsePcdLib|Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf
> UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
>
> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> diff --git a/Platform/Marvell/Include/Library/ParsePcdLib.h b/Platform/Marvell/Include/Library/ParsePcdLib.h
> deleted file mode 100644
> index a255685..0000000
> --- a/Platform/Marvell/Include/Library/ParsePcdLib.h
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -/********************************************************************************
> -Copyright (C) 2016 Marvell International Ltd.
> -
> -Marvell BSD License Option
> -
> -If you received this File from Marvell, you may opt to use, redistribute and/or
> -modify this File under the following licensing terms.
> -Redistribution and use in source and binary forms, with or without modification,
> -are permitted provided that the following conditions are met:
> -
> -* Redistributions of source code must retain the above copyright notice,
> - this list of conditions and the following disclaimer.
> -
> -* Redistributions in binary form must reproduce the above copyright
> - notice, this list of conditions and the following disclaimer in the
> - documentation and/or other materials provided with the distribution.
> -
> -* Neither the name of Marvell nor the names of its contributors may be
> - used to endorse or promote products derived from this software without
> - specific prior written permission.
> -
> -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> -ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> -WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> -DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> -ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> -(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> -LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> -ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> -(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> -
> -*******************************************************************************/
> -
> -#ifndef __PARSEPCDLIB_H__
> -#define __PARSEPCDLIB_H__
> -
> -EFI_STATUS
> -ParsePcdString (
> - IN CHAR16 *PcdString,
> - IN UINT8 Count,
> - OUT UINTN *ValueTable,
> - OUT CHAR16 **StrTable
> - );
> -
> -#endif
> diff --git a/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c b/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c
> deleted file mode 100644
> index 9a4be8e..0000000
> --- a/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.c
> +++ /dev/null
> @@ -1,228 +0,0 @@
> -/********************************************************************************
> -Copyright (C) 2016 Marvell International Ltd.
> -
> -Marvell BSD License Option
> -
> -If you received this File from Marvell, you may opt to use, redistribute and/or
> -modify this File under the following licensing terms.
> -Redistribution and use in source and binary forms, with or without modification,
> -are permitted provided that the following conditions are met:
> -
> -* Redistributions of source code must Retain the above copyright notice,
> - this list of conditions and the following disclaimer.
> -
> -* Redistributions in binary form must reproduce the above copyright
> - notice, this list of conditions and the following disclaimer in the
> - documentation and/or other materials provided with the distribution.
> -
> -* Neither the name of Marvell nor the names of its contributors may be
> - used to endorse or promote products derived from this software without
> - specific prior written permission.
> -
> -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> -ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> -WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> -DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> -ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> -(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> -LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> -ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> -(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> -
> -*******************************************************************************/
> -#define CHAR_NULL 0x0000
> -
> -#include <Library/BaseLib.h>
> -#include <Uefi.h>
> -#include <Library/UefiLib.h>
> -#include <Library/DebugLib.h>
> -
> -STATIC
> -CHAR16
> -CharToUpper (
> - IN CHAR16 Char
> - )
> -{
> -
> - if (Char >= L'a' && Char <= L'z') {
> - return (CHAR16) (Char - (L'a' - L'A'));
> - }
> -
> - return Char;
> -}
> -
> -STATIC
> -BOOLEAN
> -IsDecimalDigitChar (
> - IN CHAR16 Char
> - )
> -{
> -
> - return (BOOLEAN) (Char >= L'0' && Char <= L'9');
> -}
> -
> -
> -STATIC
> -UINTN
> -HexCharToUintn (
> - IN CHAR16 Char
> - )
> -{
> - if (IsDecimalDigitChar (Char)) {
> - return Char - L'0';
> - }
> -
> - return (UINTN) (10 + CharToUpper (Char) - L'A');
> -}
> -
> -STATIC
> -BOOLEAN
> -IsHexDigitCharacter (
> - CHAR16 Char
> - )
> -{
> -
> - return (BOOLEAN) ((Char >= L'0' && Char <= L'9') || (Char >= L'A' &&
> - Char <= L'F') || (Char >= L'a' && Char <= L'f'));
> -}
> -
> -STATIC
> -UINTN
> -HexStrToUintn (
> - CHAR16 *String
> - )
> -{
> - UINTN Result = 0;
> -
> - if (String == NULL || StrSize(String) == 0) {
> - return (UINTN)(-1);
> - }
> -
> - // Ignore spaces and tabs
> - while ((*String == L' ') || (*String == L'\t')) {
> - String++;
> - }
> -
> - // Ignore leading zeros after spaces
> - while (*String == L'0') {
> - String++;
> - }
> -
> - if (CharToUpper (*String) != L'X') {
> - return (UINTN)(-1);
> - }
> -
> - // Skip 'x'
> - String++;
> -
> - while (IsHexDigitCharacter (*String)) {
> - Result <<= 4;
> - Result += HexCharToUintn (*String);
> - String++;
> - }
> -
> - return (UINTN) Result;
> -}
> -
> -STATIC
> -UINTN
> -DecimalStrToUintn (
> - CHAR16 *String
> - )
> -{
> - UINTN Result = 0;
> -
> - while (IsDecimalDigitChar (*String)) {
> - Result = 10 * Result + (*String - L'0');
> - String++;
> - }
> -
> - return Result;
> -}
> -
> -STATIC
> -UINTN
> -StrToUintn (
> - CHAR16 *String
> - )
> -{
> - CHAR16 *Walker;
> -
> - // Chop off leading spaces
> - for (Walker = String; Walker != NULL && *Walker != CHAR_NULL && *Walker == L' '; Walker++);
> -
> - if (StrnCmp(Walker, L"0x", 2) == 0 || StrnCmp(Walker, L"0X", 2) == 0) {
> - return HexStrToUintn (Walker);
> - } else {
> - return DecimalStrToUintn (Walker);
> - }
> -}
> -
> -EFI_STATUS
> -ParsePcdString (
> - IN CHAR16 *PcdString,
> - IN UINT8 Count,
> - OUT UINTN *ValueTable,
> - OUT CHAR16 **StrTable
> - )
> -{
> - BOOLEAN ValueFlag = FALSE;
> - CHAR16 *Walker;
> - UINTN i, Tmp = 0;
> -
> - if (ValueTable != NULL) {
> - ValueFlag = TRUE;
> - }
> -
> - // Set pointer at the end of PCD string
> - Walker = PcdString + StrLen (PcdString);
> - for (i = 0; i < Count; i++) {
> - while ((--Walker) >= PcdString) {
> - if (*Walker == L';') {
> - // Cut off parsed chunk from PCD string by replacing ';' with
> - // null-terminator
> - *Walker = '\0';
> - if (ValueFlag) {
> - Tmp = StrToUintn ((Walker + 1));
> - if ((UINTN)(-1) == Tmp) {
> - return EFI_INVALID_PARAMETER;
> - }
> - // Entry is parsed from the end to the beginning
> - // so fill table in the same manner
> - ValueTable[Count - (i + 1)] = Tmp;
> - } else {
> - StrTable[Count - (i + 1)] = Walker + 1;
> - }
> - Walker--;
> - break;
> - }
> - if (Walker == PcdString) {
> - if (ValueFlag) {
> - Tmp = StrToUintn ((Walker));
> - if (Tmp == (UINTN)(-1)) {
> - return EFI_INVALID_PARAMETER;
> - }
> - }
> - // Last device's entry should be added to the table here.
> - // If not, return error
> - if (i != (Count - 1)) {
> - DEBUG((DEBUG_ERROR, "ParsePcdLib: Please set PCD value for every "
> - "device\n"));
> - return EFI_INVALID_PARAMETER;
> - }
> - // We parse from the end to the beginning
> - // so fill table in the same manner
> - if (ValueFlag) {
> - ValueTable[Count - (i + 1)] = Tmp;
> - } else {
> - StrTable[Count - (i + 1)] = Walker;
> - }
> - // End both loops
> - return EFI_SUCCESS;
> - }
> - }
> - }
> -
> - return EFI_SUCCESS;
> -}
> diff --git a/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf b/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf
> deleted file mode 100644
> index b4db621..0000000
> --- a/Platform/Marvell/Library/ParsePcdLib/ParsePcdLib.inf
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -# Copyright (C) 2016 Marvell International Ltd.
> -#
> -# Marvell BSD License Option
> -#
> -# If you received this File from Marvell, you may opt to use, redistribute and/or
> -# modify this File under the following licensing terms.
> -# Redistribution and use in source and binary forms, with or without modification,
> -# are permitted provided that the following conditions are met:
> -#
> -# * Redistributions of source code must retain the above copyright notice,
> -# this list of conditions and the following disclaimer.
> -#
> -# * Redistributions in binary form must reproduce the above copyright
> -# notice, this list of conditions and the following disclaimer in the
> -# documentation and/or other materials provided with the distribution.
> -#
> -# * Neither the name of Marvell nor the names of its contributors may be
> -# used to endorse or promote products derived from this software without
> -# specific prior written permission.
> -#
> -# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> -# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> -# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> -# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> -# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> -# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> -# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> -# ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> -# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> -# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> -#
> -
> -[Defines]
> - INF_VERSION = 0x00010005
> - BASE_NAME = ParsePcdLib
> - FILE_GUID = 698d85a0-a952-453e-b8a4-1d6ea338a38e
> - MODULE_TYPE = BASE
> - VERSION_STRING = 1.0
> - LIBRARY_CLASS = ParsePcdLib
> -
> -[Packages]
> - MdePkg/MdePkg.dec
> - MdeModulePkg/MdeModulePkg.dec
> -
> -[LibraryClasses]
> - ArmLib
> - DebugLib
> -
> -[Sources.common]
> - ParsePcdLib.c
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing
2017-10-06 15:52 ` Leif Lindholm
@ 2017-10-06 19:22 ` Marcin Wojtas
2017-10-06 19:51 ` Leif Lindholm
0 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 19:22 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib
2017-10-06 15:56 ` Leif Lindholm
@ 2017-10-06 19:24 ` Marcin Wojtas
0 siblings, 0 replies; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 19:24 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
2017-10-06 17:56 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Oct 06, 2017 at 09:51:15AM +0200, Marcin Wojtas wrote:
>> This patch introduces I2c description, using the new structures
>> and template in MvHwDescLib. This change enables more flexible
>> addition of multiple I2c controllers and also allows for
>> removal of string PCD parsing. Update Armada 70x0 DB description
>> and PortingGuide accordingly.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Armada/Armada70x0.dsc | 2 +-
>> Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 42 +++++++++++---------
>> Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf | 3 +-
>> Platform/Marvell/Include/Library/MvHwDescLib.h | 25 ++++++++++++
>> Platform/Marvell/Marvell.dec | 2 +-
>> Silicon/Marvell/Documentation/PortingGuide.txt | 4 +-
>> 6 files changed, 54 insertions(+), 24 deletions(-)
>>
>> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
>> index 7b03744..d9d126d 100644
>> --- a/Platform/Marvell/Armada/Armada70x0.dsc
>> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
>> @@ -78,7 +78,7 @@
>> # I2C
>> gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses|{ 0x50, 0x57, 0x60 }
>> gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0, 0x0, 0x0 }
>> - gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|L"0xF2701000;0xF2701100"
>> + gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x1, 0x1 }
>
> Similarly to the previous patch, the above is a bit opaque.
> I guess in this case, this is simply a boolean array saying whether a
> specific controller is enabled or not?
> If my interpretation is correct, could the Pcd be renamed
> PcdI2cControllersEnabled?
>
Sure, no problem.
Thanks,
Marcin
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 3/5] Marvell/Library: UtmiLib: Move devices description to MvHwDescLib
2017-10-06 15:57 ` Leif Lindholm
@ 2017-10-06 19:26 ` Marcin Wojtas
2017-10-06 19:57 ` Leif Lindholm
0 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 19:26 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
2017-10-06 17:57 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Oct 06, 2017 at 09:51:16AM +0200, Marcin Wojtas wrote:
>> This patch introduces UTMI description, using the new structures
>> and template in MvHwDescLib. This change enables more flexible
>> addition of multiple CP with UTMI PHY's and also significantly
>> reduces amount of used PCD's for that purpose. Update PortingGuide
>> documentation accordingly.
>>
>> This patch replaces string-based description of Utmi on
>> Armada 70x0 DB with new, reduced format.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Armada/Armada70x0.dsc | 7 +-
>> Platform/Marvell/Include/Library/MvHwDescLib.h | 47 ++++++
>> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c | 149 ++++++++++----------
>> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h | 1 -
>> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf | 11 +-
>> Platform/Marvell/Marvell.dec | 7 +-
>> Silicon/Marvell/Documentation/PortingGuide.txt | 30 ++--
>> 7 files changed, 142 insertions(+), 110 deletions(-)
>>
>> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
>> index d9d126d..04bdf7c 100644
>> --- a/Platform/Marvell/Armada/Armada70x0.dsc
>> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
>> @@ -111,11 +111,8 @@
>> gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds|{ 0x1, 0x6, 0xA, 0x6, 0x6, 0x6 }
>>
>> #UtmiPhy
>> - gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2
>> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|L"0xF2440420;0xF2440420"
>> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg|L"0xF2440440;0xF2440444"
>> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit|L"0xF2580000;0xF2581000"
>> - gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort|L"0x0;0x1"
>> + gMarvellTokenSpaceGuid.PcdUtmiControllers|{ 0x1, 0x1 }
>> + gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0, 0x1 }
>
> Like for 1/5, could we have some entries under a [Define] in a .inc
> (reasonably the same .inc)?
>
Ok. Any preferences in terms of the name of such file? Unless we can
just put it into Armada.dsc.inc...
Best regards,
Marcin
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 4/5] Marvell/Drivers: Pp2Dxe: Rework PHY handling
2017-10-06 16:02 ` Leif Lindholm
@ 2017-10-06 19:30 ` Marcin Wojtas
0 siblings, 0 replies; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 19:30 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
2017-10-06 18:02 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Oct 06, 2017 at 09:51:17AM +0200, Marcin Wojtas wrote:
>> Hitherto PHY handling in Pp2Dxe was not flexible. It allowed for using
>> only single MDIO controller, which may not be true on Armada 80x0 SoCs.
>> For this purpose introduce the MDIO description, using the new structures
>> and template in MvHwDescLib. This change enables addition of multiple
>> CP110 hardware blocks with MDIO controllers.
>>
>> This change required different PHY handling and obtaining data over
>> desired MDIO bus. Now given Pp2 port is matched with the PHY via
>> its index in gMarvellTokenSpaceGuid.PcdPhyDeviceIds. The PHY itself
>> is mapped to the MDIO controller, using
>> gMarvellTokenSpaceGuid.PcdPhy2MdioController. Also obtaining
>> SMI addresses was moved to the PHY initialization routine.
>> All above allow for much cleaner and logical PHY description
>> in the .dsc file.
>>
>> Update PortingGuide documentation accordingly and Armada 70x0 DB
>> NIC/PHY description.
>
> Like for previous patches, can we have:
> - symbolic names for things that can be given symbolic names.
> - Pcds describing boolean arrays have an "Enabled" appended to their
> name?
>
2x OK.
Marcin.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing
2017-10-06 19:22 ` Marcin Wojtas
@ 2017-10-06 19:51 ` Leif Lindholm
2017-10-06 20:08 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-10-06 19:51 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 3/5] Marvell/Library: UtmiLib: Move devices description to MvHwDescLib
2017-10-06 19:26 ` Marcin Wojtas
@ 2017-10-06 19:57 ` Leif Lindholm
0 siblings, 0 replies; 21+ messages in thread
From: Leif Lindholm @ 2017-10-06 19:57 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
On Fri, Oct 06, 2017 at 09:26:43PM +0200, Marcin Wojtas wrote:
> 2017-10-06 17:57 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Fri, Oct 06, 2017 at 09:51:16AM +0200, Marcin Wojtas wrote:
> >> This patch introduces UTMI description, using the new structures
> >> and template in MvHwDescLib. This change enables more flexible
> >> addition of multiple CP with UTMI PHY's and also significantly
> >> reduces amount of used PCD's for that purpose. Update PortingGuide
> >> documentation accordingly.
> >>
> >> This patch replaces string-based description of Utmi on
> >> Armada 70x0 DB with new, reduced format.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >> Platform/Marvell/Armada/Armada70x0.dsc | 7 +-
> >> Platform/Marvell/Include/Library/MvHwDescLib.h | 47 ++++++
> >> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.c | 149 ++++++++++----------
> >> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.h | 1 -
> >> Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf | 11 +-
> >> Platform/Marvell/Marvell.dec | 7 +-
> >> Silicon/Marvell/Documentation/PortingGuide.txt | 30 ++--
> >> 7 files changed, 142 insertions(+), 110 deletions(-)
> >>
> >> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> >> index d9d126d..04bdf7c 100644
> >> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> >> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> >> @@ -111,11 +111,8 @@
> >> gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds|{ 0x1, 0x6, 0xA, 0x6, 0x6, 0x6 }
> >>
> >> #UtmiPhy
> >> - gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2
> >> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|L"0xF2440420;0xF2440420"
> >> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiCfg|L"0xF2440440;0xF2440444"
> >> - gMarvellTokenSpaceGuid.PcdUtmiPhyRegUtmiUnit|L"0xF2580000;0xF2581000"
> >> - gMarvellTokenSpaceGuid.PcdUtmiPhyUtmiPort|L"0x0;0x1"
> >> + gMarvellTokenSpaceGuid.PcdUtmiControllers|{ 0x1, 0x1 }
> >> + gMarvellTokenSpaceGuid.PcdUtmiPortType|{ 0x0, 0x1 }
> >
> > Like for 1/5, could we have some entries under a [Define] in a .inc
> > (reasonably the same .inc)?
> >
>
> Ok. Any preferences in terms of the name of such file? Unless we can
> just put it into Armada.dsc.inc...
Not really any preference.
Armada.dsc.inc might be fine.
/
Leif
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing
2017-10-06 19:51 ` Leif Lindholm
@ 2017-10-06 20:08 ` Marcin Wojtas
2017-10-07 14:17 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-06 20:08 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing
2017-10-06 20:08 ` Marcin Wojtas
@ 2017-10-07 14:17 ` Marcin Wojtas
2017-10-07 14:57 ` Leif Lindholm
0 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-07 14:17 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing
2017-10-07 14:17 ` Marcin Wojtas
@ 2017-10-07 14:57 ` Leif Lindholm
0 siblings, 0 replies; 21+ messages in thread
From: Leif Lindholm @ 2017-10-07 14:57 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-10-07 14:54 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-06 7:51 [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Marcin Wojtas
2017-10-06 7:51 ` [platforms: PATCH 1/5] Marvell/Library: ComPhyLib: Remove PCD string parsing Marcin Wojtas
2017-10-06 15:52 ` Leif Lindholm
2017-10-06 19:22 ` Marcin Wojtas
2017-10-06 19:51 ` Leif Lindholm
2017-10-06 20:08 ` Marcin Wojtas
2017-10-07 14:17 ` Marcin Wojtas
2017-10-07 14:57 ` Leif Lindholm
2017-10-06 7:51 ` [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib Marcin Wojtas
2017-10-06 15:56 ` Leif Lindholm
2017-10-06 19:24 ` Marcin Wojtas
2017-10-06 7:51 ` [platforms: PATCH 3/5] Marvell/Library: UtmiLib: " Marcin Wojtas
2017-10-06 15:57 ` Leif Lindholm
2017-10-06 19:26 ` Marcin Wojtas
2017-10-06 19:57 ` Leif Lindholm
2017-10-06 7:51 ` [platforms: PATCH 4/5] Marvell/Drivers: Pp2Dxe: Rework PHY handling Marcin Wojtas
2017-10-06 16:02 ` Leif Lindholm
2017-10-06 19:30 ` Marcin Wojtas
2017-10-06 7:51 ` [platforms: PATCH 5/5] Platform/Marvell/Armada: Remove ParsePcdLib Marcin Wojtas
2017-10-06 16:02 ` Leif Lindholm
2017-10-06 14:29 ` [platforms: PATCH 0/5] Armada 7k/8k - ParsePcdLib removal Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox