On Mon, Mar 20, 2017 at 05:19:57PM +0100, Marcin Wojtas wrote:
Hi Leif,
+DECLARE_A7K8K_STORAGE_TEMPLATE;
/*
- For CP-110 we have 2 Selector registers "PHY Selectors"
- and " PIPE Selectors".
@@ -702,24 +705,37 @@ UINTN ComPhySataPowerUp ( IN UINT32 Lane, IN EFI_PHYSICAL_ADDRESS HpipeBase,
- IN EFI_PHYSICAL_ADDRESS ComPhyBase
- IN EFI_PHYSICAL_ADDRESS ComPhyBase,
- IN UINT32 SataHostId )
{ EFI_STATUS Status;
- UINTN SataArrSize;
- UINT8 *SataArrPtr;
No Ptr suffixes, please.
Ok.
- MVHW_STORAGE_DESC *Desc = &mA7k8kStorageDescTemplate; EFI_PHYSICAL_ADDRESS HpipeAddr = HPIPE_ADDR(HpipeBase, Lane); EFI_PHYSICAL_ADDRESS SdIpAddr = SD_ADDR(HpipeBase, Lane); EFI_PHYSICAL_ADDRESS ComPhyAddr = COMPHY_ADDR(ComPhyBase, Lane);
EFI_PHYSICAL_ADDRESS SataBase;
SataBase = PcdGet32 (PcdSataBaseAddress);
if (SataBase == 0) {
DEBUG((DEBUG_INFO, "ComPhy: SATA address not defined\n"));
return EFI_D_ERROR;
- SataArrSize = PcdGetSize (PcdPciEAhci);
- SataArrPtr = (UINT8 *) PcdGetPtr (PcdPciEAhci);
- if (SataArrPtr == NULL || SataHostId >= SataArrSize) {
- DEBUG((DEBUG_ERROR, "ComPhySata: Sata host %d is undefined\n",
SataHostId));
return EFI_INVALID_PARAMETER; }
if (SataArrPtr[SataHostId] != 1) {
What is "1"? Can we have a descriptive #define instead?
How about adding: #define DEV_ENABLED 1 in the MvHwDescLib.h?
It's Tianocore, so a little more namespacing could make sense. An MV_ or MV_HWDESCLIB_ prefix perhaps?
- /* This SATA host is not enabled in PCD */
- DEBUG((DEBUG_WARN, "ComPhySata: Sata host %d is disabled\n",
SataHostId));
Coding-style wise, there sould be a space between DEBUG and ((. Don't need to change other occurences, but please add it in modified lines.
Ok.
- return EFI_SUCCESS;
- }
- DEBUG((DEBUG_INFO, "ComPhySata: Initialize SATA PHYs\n"));
- DEBUG((DEBUG_INFO, "ComPhySataPowerUp: stage: MAC configuration -
power down ComPhy\n"));
- ComPhySataMacPowerDown (SataBase);
ComPhySataMacPowerDown (Desc->AhciBaseAddresses[SataHostId]);
DEBUG((DEBUG_INFO, "ComPhy: stage: RFU configurations - hard reset
ComPhy\n"));
@@ -735,7 +751,7 @@ ComPhySataPowerUp (
DEBUG((DEBUG_INFO, "ComPhy: stage: ComPhy power up\n"));
- ComPhySataPhyPowerUp (SataBase);
ComPhySataPhyPowerUp (Desc->AhciBaseAddresses[SataHostId]);
DEBUG((DEBUG_INFO, "ComPhy: stage: Check PLL\n"));
@@ -961,11 +977,11 @@ ComPhyMuxCp110 ( ComPhyMapPhyData[Lane].Speed = SerdesMap[Lane].Speed; } PtrChipCfg->MuxData = Cp110ComPhyMuxData;
- ComPhyMuxInit(PtrChipCfg, ComPhyMapPhyData, ComPhyBaseAddr +
- ComPhyMuxInit (PtrChipCfg, ComPhyMapPhyData, ComPhyBaseAddr +
On the other hand, here is a whole series of whitespace-only changes. Please leave these out of this patch. If you're happy to do all these fixups as a separate patch preceding this one, that'd be great. If not, just leave them be - it makes it much harder to spot the actual functional changes.
They will be removed from here.
Thanks.
/ Leif
COMMON_SELECTOR_PHY_OFFSET);PtrChipCfg->MuxData = Cp110ComPhyPipeMuxData;
- ComPhyMuxInit(PtrChipCfg, ComPhyMapPipeData, ComPhyBaseAddr +
ComPhyMuxInit (PtrChipCfg, ComPhyMapPipeData, ComPhyBaseAddr + COMMON_SELECTOR_PIPE_OFFSET);
/* Fix the Type after check the PHY and PIPE configuration */
@@ -992,7 +1008,7 @@ ComPhyCp110Init ( SerdesMap = PtrChipCfg->MapData;
/* Config Comphy mux configuration */
- ComPhyMuxCp110(PtrChipCfg, SerdesMap);
ComPhyMuxCp110 (PtrChipCfg, SerdesMap);
/* Check if the first 4 Lanes configured as By-4 */ for (Lane = 0, PtrComPhyMap = SerdesMap; Lane < 4; Lane++,
PtrComPhyMap++) {
@@ -1014,23 +1030,25 @@ ComPhyCp110Init ( case PHY_TYPE_PCIE1: case PHY_TYPE_PCIE2: case PHY_TYPE_PCIE3:
Status = ComPhyPciePowerUp(Lane, PcieBy4, HpipeBaseAddr,ComPhyBaseAddr);
Status = ComPhyPciePowerUp (Lane, PcieBy4, HpipeBaseAddr,ComPhyBaseAddr);
break; case PHY_TYPE_SATA0: case PHY_TYPE_SATA1:
Status = ComPhySataPowerUp (Lane, HpipeBaseAddr, ComPhyBaseAddr,0);
#define for that 0?
Ok.
break;
Status = ComPhySataPowerUp(Lane, HpipeBaseAddr, ComPhyBaseAddr);
Status = ComPhySataPowerUp (Lane, HpipeBaseAddr, ComPhyBaseAddr,1);
#define for that 1?
Ok.
break; case PHY_TYPE_USB3_HOST0: case PHY_TYPE_USB3_HOST1:
Status = ComphyUsb3PowerUp(Lane, HpipeBaseAddr, ComPhyBaseAddr);
Status = ComphyUsb3PowerUp (Lane, HpipeBaseAddr, ComPhyBaseAddr); break;
Status = ComPhySgmiiPowerUp(Lane, PtrComPhyMap->Speed,HpipeBaseAddr,
Status = ComPhySgmiiPowerUp (Lane, PtrComPhyMap->Speed,HpipeBaseAddr,
ComPhyBaseAddr); break; default:diff --git a/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.inf
b/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.inf
index a7ee1f8..02905a5 100644 --- a/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.inf +++ b/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.inf @@ -105,5 +105,4 @@ gMarvellTokenSpaceGuid.PcdChip3ComPhySpeeds gMarvellTokenSpaceGuid.PcdChip3ComPhyInvFlags
- #SATA
- gMarvellTokenSpaceGuid.PcdSataBaseAddress
- gMarvellTokenSpaceGuid.PcdPciEAhci
diff --git a/Platforms/Marvell/Marvell.dec b/Platforms/Marvell/Marvell.
dec
index 313eaa6..267b655 100644 --- a/Platforms/Marvell/Marvell.dec +++ b/Platforms/Marvell/Marvell.dec @@ -176,9 +176,6 @@ gMarvellTokenSpaceGuid.PcdChip3ComPhySpeeds|{ 0x0 }|VOID*|0x30000176 gMarvellTokenSpaceGuid.PcdChip3ComPhyInvFlags|{ 0x0
}|VOID*|0x30000177
-#SATA
- gMarvellTokenSpaceGuid.PcdSataBaseAddress|0|UINT32|0x4000052
#UtmiPhy gMarvellTokenSpaceGuid.PcdUtmiPhyCount|0|UINT32|0x30000205 gMarvellTokenSpaceGuid.PcdUtmiPhyRegUsbCfg|{ 0x0 }|VOID*|0x30000206 @@ -225,4 +222,3 @@ gMarvellPhyProtocolGuid = { 0x32f48a43, 0x37e3,
0x4acf, { 0x93, 0xc4, 0x3e, 0x57, 0xa7, 0xb0, 0xfb, 0xdc }}
gMarvellSpiMasterProtocolGuid = { 0x23de66a3, 0xf666,
0x4b3e, { 0xaa, 0xa2, 0x68, 0x9b, 0x18, 0xae, 0x2e, 0x19 }}
gMarvellSpiFlashProtocolGuid = { 0x9accb423, 0x5bd2,
0x4fca, { 0x9b, 0x4c, 0x2e, 0x65, 0xfc, 0x25, 0xdf, 0x21 }}
Umm, no.
Ok.
Best regards, Marcin