On Mon, Apr 10, 2017 at 05:03:36PM +0200, Marcin Wojtas wrote:
Leif,
Thanks for checking the patch.
gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds
@@ -72,14 +39,7 @@ Indicates lane polarity invert. Example
#ComPhy
- gMarvellTokenSpaceGuid.PcdComPhyChipCount|1
- gMarvellTokenSpaceGuid.PcdChip0ComPhyMaxLanes|6
- gMarvellTokenSpaceGuid.PcdChip0ComPhyBaseAddress|0xF2441000
- gMarvellTokenSpaceGuid.PcdChip0Hpipe3BaseAddress|0xF2120000
- gMarvellTokenSpaceGuid.PcdChip0ComPhyMuxBitCount|4
- gMarvellTokenSpaceGuid.PcdChip0Compatible|L"Cp110"
- gMarvellTokenSpaceGuid.PcdComPhyChipEnabled|{ 0x1 }
I'm a little bit confused by the meaning of this option now. Is it list of devices enabled? Is it the number of devices enabled?
It's an array of enabled devices, similar to PcdPciEAhci, etc. Do you wish some different name here?
Yes please. A Pcd ending in Enabled looks like something that should be a BOOLEAN.
/ Leif
}}
- DEBUG((DEBUG_ERROR, "ComPhy: Empty ChipType string\n"));
- DEBUG((DEBUG_ERROR, "ComPhy: Wrong chip type\n"));
Worth doing the whitespace style fixup when modifying line. Also, I think "Invalid" would be more accurate than "Wrong" in this message.
2x OK.
return EFI_D_ERROR; }
@@ -222,18 +222,37 @@ STATIC VOID InitComPhyConfig ( IN OUT CHIP_COMPHY_CONFIG *ChipConfig,
- IN OUT PCD_LANE_MAP *LaneData
- IN OUT PCD_LANE_MAP *LaneData,
- IN UINT8 Id )
{
- MVHW_COMPHY_DESC *Desc = &mA7k8kComPhyDescTemplate;
- ChipConfig->ChipType = Desc->ComPhyChipType[Id];
- ChipConfig->ComPhyBaseAddr = Desc->ComPhyBaseAddresses[Id];
- ChipConfig->Hpipe3BaseAddr = Desc->ComPhyHpipe3BaseAddresses[Id];
- ChipConfig->LanesCount = Desc->ComPhyLaneCount[Id];
- ChipConfig->MuxBitCount = Desc->ComPhyMuxBitCount[Id];
- /*
- Below macro contains variable name concatenation (used to form PCD's name)
- and that's why invoking it cannot be automated, e.g. using for loop.
- Currently up to 4 ComPhys might be configured.
- and that's why invoking it cannot be automated, e.g. using for loop's
- iterator.
Not sure the above comment change is an improvement. Could just delete those two lines - the code change makes it look a lot less like a basic coding oversight than the previous version.
Ok, I'll remove those 2 lines.
*/
- GetComPhyPcd(ChipConfig, LaneData, 0);
- GetComPhyPcd(ChipConfig, LaneData, 1);
- GetComPhyPcd(ChipConfig, LaneData, 2);
- GetComPhyPcd(ChipConfig, LaneData, 3);
- switch (Id) {
- case 0:
- GetComPhyPcd(ChipConfig, LaneData, 0);
Whitespace.
Ok.
- break;
- case 1:
- GetComPhyPcd(ChipConfig, LaneData, 1);
- break;
- case 2:
- GetComPhyPcd(ChipConfig, LaneData, 2);
- break;
- case 3:
- GetComPhyPcd(ChipConfig, LaneData, 3);
- break;
- }
}
EFI_STATUS @@ -242,29 +261,42 @@ MvComPhyInit ( ) { EFI_STATUS Status;
- CHIP_COMPHY_CONFIG ChipConfig[MAX_CHIPS], *PtrChipCfg;
- PCD_LANE_MAP LaneData[MAX_CHIPS];
- UINT32 Lane, ChipCount, i, MaxComphyCount;
- ChipCount = PcdGet32 (PcdComPhyChipCount);
- InitComPhyConfig(ChipConfig, LaneData);
- CHIP_COMPHY_CONFIG ChipConfig[MVHW_MAX_COMPHY_DEVS], *PtrChipCfg;
- PCD_LANE_MAP LaneData[MVHW_MAX_COMPHY_DEVS];
- UINT32 Lane, MaxComphyCount;
- UINT8 *ComPhyDeviceTable, Index;
- /* Obtain table with enabled ComPhy devices */
- ComPhyDeviceTable = (UINT8 *) PcdGetPtr (PcdComPhyChipEnabled);
Drop space after cast.
Ok.
- if (ComPhyDeviceTable == NULL) {
- DEBUG ((DEBUG_ERROR, "Missing PcdComPhyChipEnabled\n"));
- return EFI_INVALID_PARAMETER;
- }
- if (ChipCount <= 0 || ChipCount > MAX_CHIPS)
- if (PcdGetSize (PcdComPhyChipEnabled) > MVHW_MAX_COMPHY_DEVS) {
- DEBUG ((DEBUG_ERROR, "Wrong PcdComPhyChipEnabled format\n")); return EFI_INVALID_PARAMETER;
- }
- /* Initialize enabled chips */
- for (Index = 0; Index < PcdGetSize (PcdComPhyChipEnabled); Index++) {
- if (!MVHW_DEV_ENABLED (ComPhy, Index)) {
DEBUG ((DEBUG_ERROR, "Skip ComPhy chip %d\n", Index));continue;- }
- for (i = 0; i < ChipCount ; i++) {
- PtrChipCfg = &ChipConfig[i];
PtrChipCfg = &ChipConfig[Index];
InitComPhyConfig(PtrChipCfg, LaneData, Index);
/* 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[i].TypeStr[Lane]);
ParseSerdesTypeString (LaneData[Index].TypeStr[Lane]); PtrChipCfg->MapData[Lane].Speed =
ParseSerdesSpeed (LaneData[i].SpeedValue[Lane]);PtrChipCfg->MapData[Lane].Invert = (UINT32) LaneData[i].InvFlag[Lane];
ParseSerdesSpeed (LaneData[Index].SpeedValue[Lane]);PtrChipCfg->MapData[Lane].Invert = (UINT32) LaneData[Index].InvFlag[Lane];No space after cast.
Ok.
if ((PtrChipCfg->MapData[Lane].Speed == PHY_SPEED_INVALID) || (PtrChipCfg->MapData[Lane].Speed == PHY_SPEED_ERROR) ||@@ -278,7 +310,7 @@ MvComPhyInit (
Status = GetChipComPhyInit (PtrChipCfg); if (EFI_ERROR(Status)) {
DEBUG((DEBUG_ERROR, "ComPhy: Invalid Chip%dType name\n", i));
DEBUG((DEBUG_ERROR, "ComPhy: Invalid Chip%dType name\n", Index));Missing space.
Ok.
Thanks, Marcin