On Fri, Mar 17, 2017 at 01:40:29AM +0100, Marcin Wojtas wrote:

From: Konstantin Porotchkin kostap@marvell.com

Fix compilation warning triggered by GCC 4.9.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Signed-off-by: Marcin Wojtas mw@semihalf.com

---

Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c b/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c index df8731d..07a4e6f 100755 --- a/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c +++ b/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c @@ -980,7 +980,7 @@ ComPhyCp110Init ( IN CHIP_COMPHY_CONFIG *PtrChipCfg ) {

• EFI_STATUS Status;

• EFI_STATUS Status = EFI_SUCCESS; COMPHY_MAP *PtrComPhyMap, *SerdesMap; EFI_PHYSICAL_ADDRESS ComPhyBaseAddr, HpipeBaseAddr; UINT32 ComPhyMaxCount, Lane;

@@ -1043,5 +1043,5 @@ ComPhyCp110Init ( Lane)); }

• return EFI_SUCCESS;

• return Status;

OK, so I never objected to this the first time around, but do we really want to return SUCCESS if processing invalid data? It doesn't even look like the return value is checked...

If you really don't care, make the function return void. If you do, set Status to EFI_UNSUPPORTED, EFI_NOT_FOUND or EFI_INVALID_PARAMETER on the default target of the switch as well.

/ Leif

}

1.8.3.1

On Mon, Mar 20, 2017 at 06:06:36PM +0100, Marcin Wojtas wrote:

Leif,

2017-03-20 16:20 GMT+01:00 Leif Lindholm leif.lindholm@linaro.org:

...

...

UINT32 ComPhyMaxCount, Lane; @@ -1043,5 +1043,5 @@ ComPhyCp110Init ( Lane)); }

• return EFI_SUCCESS;

• return Status;

OK, so I never objected to this the first time around, but do we really want to return SUCCESS if processing invalid data? It doesn't even look like the return value is checked...

If you really don't care, make the function return void. If you do, set Status to EFI_UNSUPPORTED, EFI_NOT_FOUND or EFI_INVALID_PARAMETER on the default target of the switch as well.

I agree returning Status at the very end doesn't make much sense, so I'll remove it. For DEBUG build I'll add and assert in switch default and leave the rest as is -- each lane is initialized independently, so normally I prefer not to crash in case one of them fails. Is it ok for you?

Yeah, that's fine. It's just the current half-way house that looks weird.

/ Leif

On Fri, Mar 17, 2017 at 01:40:30AM +0100, Marcin Wojtas wrote:

From: Konstantin Porotchkin kostap@marvell.com

Extend the COMPHY configuration table by CP-slave entry. Change the chip type to be different for CP-master and CP-slave (Cp110m, Cp110s). This is a preparation commit for adding support for new SoC - Armada 80x0, which comprises two CP110 HW blocks with peripherals.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Signed-off-by: Marcin Wojtas mw@semihalf.com

Reviewed-by: Leif Lindholm leif.lindholm@linaro.org

---

Platforms/Marvell/Armada/Armada70x0.dsc | 2 +- Platforms/Marvell/Library/ComPhyLib/ComPhyLib.c | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Platforms/Marvell/Armada/Armada70x0.dsc b/Platforms/Marvell/Armada/Armada70x0.dsc index 134ab71..83842da 100644 --- a/Platforms/Marvell/Armada/Armada70x0.dsc +++ b/Platforms/Marvell/Armada/Armada70x0.dsc @@ -103,7 +103,7 @@ gMarvellTokenSpaceGuid.PcdChip0ComPhyBaseAddress|0xF2441000 gMarvellTokenSpaceGuid.PcdChip0Hpipe3BaseAddress|0xF2120000 gMarvellTokenSpaceGuid.PcdChip0ComPhyMuxBitCount|4

• gMarvellTokenSpaceGuid.PcdChip0Compatible|L"Cp110"

• gMarvellTokenSpaceGuid.PcdChip0Compatible|L"Cp110m"

#UtmiPhy gMarvellTokenSpaceGuid.PcdUtmiPhyCount|2 diff --git a/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.c b/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.c index 1b45bb7..9efefb2 100644 --- a/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.c +++ b/Platforms/Marvell/Library/ComPhyLib/ComPhyLib.c @@ -46,8 +46,12 @@ CHAR16 * SpeedStringTable [] = {L"-", L"1.25 Gbps", L"1.5 Gbps", L"2.5 Gbps", L"6 Gbps", L"6.25 Gbps", L"10.31 Gbps"}; CHIP_COMPHY_CONFIG ChipCfgTbl[] = {

• {
• .ChipType = L"Cp110",

• { /* CP master */
• .ChipType = L"Cp110m",
• .Init = ComPhyCp110Init
• },
• { /* CP slave */
• .ChipType = L"Cp110s", .Init = ComPhyCp110Init }

};

1.8.3.1

(I realise I'm commenting on code you're only moving around here, but I would still appreciate fixing a few things below.)

On Fri, Mar 17, 2017 at 01:40:31AM +0100, Marcin Wojtas wrote:

From: Konstantin Porotchkin kostap@marvell.com

Move the platform description structure to a common header HwDescLib.h. Extend the structure with entries corresponding to interfaces located on a first CP slave device, so that both CP110 blocks are described. Change the platform description structure type to MVHW_STORAGE_DESC.

This change is a beginning of Armada 7k/8k support improvement, that will allow to store SoC's description in one place, accessible for the drivers and libraries. With that, the PCD description will be cleaner and it will also ease adding support for multiple interfaces of one kind.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Signed-off-by: Marcin Wojtas mw@semihalf.com

---

Platforms/Marvell/Armada/Armada70x0.dsc | 4 +- Platforms/Marvell/Include/Library/MvHwDescLib.h | 78 +++++++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.c | 44 ++------------ 3 files changed, 85 insertions(+), 41 deletions(-) create mode 100644 Platforms/Marvell/Include/Library/MvHwDescLib.h

diff --git a/Platforms/Marvell/Armada/Armada70x0.dsc b/Platforms/Marvell/Armada/Armada70x0.dsc index 83842da..ab8fa3d 100644 --- a/Platforms/Marvell/Armada/Armada70x0.dsc +++ b/Platforms/Marvell/Armada/Armada70x0.dsc @@ -140,8 +140,8 @@ gMarvellTokenSpaceGuid.PcdPp2XlgDevSize|0x1000 #PciEmulation

• gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1 }
• gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1 }

• gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1, 0x0, 0x0 }
• gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1, 0x0 } gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x1, 0x1 }

#ResetLib diff --git a/Platforms/Marvell/Include/Library/MvHwDescLib.h b/Platforms/Marvell/Include/Library/MvHwDescLib.h new file mode 100644 index 0000000..b931969 --- /dev/null +++ b/Platforms/Marvell/Include/Library/MvHwDescLib.h @@ -0,0 +1,78 @@ +/******************************************************************************** +Copyright (C) 2017 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 __MVHWDESCLIB_H__ +#define __MVHWDESCLIB_H__

+#include <Library/NonDiscoverableDeviceRegistrationLib.h>

+// +// Platform storage description +// +typedef struct {

• // XHCI
• UINT8 XhciDevCount;
• UINTN XhciBaseAddresses[4];
• UINTN XhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE XhciDmaType[4];
• // AHCI
• UINT8 AhciDevCount;
• UINTN AhciBaseAddresses[4];
• UINTN AhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE AhciDmaType[4];
• // SDHCI
• UINT8 SdhciDevCount;
• UINTN SdhciBaseAddresses[4];
• UINTN SdhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE SdhciDmaType[4];

+} MVHW_STORAGE_DESC;

What do these hardcoded array sizes signify? What makes 4 guaranteed future-proof?

+#define DECLARE_A7K8K_STORAGE_TEMPLATE \

Is XHCI "storage"?

+STATIC MVHW_STORAGE_DESC mA7k8kStorageDescTemplate = {\

• 4, /* XHCI */\
• { 0xF2500000, 0xF2510000, 0xF4500000, 0xF4510000 },\

These addresses would be a lot nicer as #defines.

• { SIZE_16KB, SIZE_16KB, SIZE_16KB, SIZE_16KB },\
• { NonDiscoverableDeviceDmaTypeCoherent, NonDiscoverableDeviceDmaTypeCoherent,\
• NonDiscoverableDeviceDmaTypeCoherent, NonDiscoverableDeviceDmaTypeCoherent },\
• 2, /* AHCI */\
• { 0xF2540000, 0xF4540000 },\
• { SIZE_8KB, SIZE_8KB },\
• { NonDiscoverableDeviceDmaTypeCoherent, NonDiscoverableDeviceDmaTypeCoherent },\
• 2, /* SDHCI */\
• { 0xF06E0000, 0xF2780000 },\
• { SIZE_1KB, SIZE_1KB },\
• { NonDiscoverableDeviceDmaTypeCoherent, NonDiscoverableDeviceDmaTypeCoherent }\

+}

+#endif /* __MVHWDESCLIB_H__ */ diff --git a/Platforms/Marvell/PciEmulation/PciEmulation.c b/Platforms/Marvell/PciEmulation/PciEmulation.c index 491b886..ffefffb 100644 --- a/Platforms/Marvell/PciEmulation/PciEmulation.c +++ b/Platforms/Marvell/PciEmulation/PciEmulation.c @@ -34,47 +34,13 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include <PiDxe.h> +#include <Library/MvHwDescLib.h>

Insert sorted, please.

#include <Library/DebugLib.h> -#include <Library/NonDiscoverableDeviceRegistrationLib.h> #include <Library/UefiBootServicesTableLib.h> #include <Protocol/EmbeddedExternalDevice.h> -// -// Platform description -// -typedef struct {

• // XHCI
• UINT8 XhciDevCount;
• UINTN XhciBaseAddresses[4];
• UINTN XhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE XhciDmaType[4];
• // AHCI
• UINT8 AhciDevCount;
• UINTN AhciBaseAddresses[4];
• UINTN AhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE AhciDmaType[4];
• // SDHCI
• UINT8 SdhciDevCount;
• UINTN SdhciBaseAddresses[4];
• UINTN SdhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE SdhciDmaType[4];

-} PCIE_PLATFORM_DESC;

-STATIC PCIE_PLATFORM_DESC mA70x0PlatDescTemplate = {

• 2, // XHCI
• { 0xF2500000, 0xF2510000 },
• { SIZE_16KB, SIZE_16KB },
• { NonDiscoverableDeviceDmaTypeCoherent, NonDiscoverableDeviceDmaTypeCoherent },
• 1, // AHCI
• { 0xF2540000 },
• { SIZE_8KB },
• { NonDiscoverableDeviceDmaTypeCoherent },
• 2, // SDHCI
• { 0xF06E0000, 0xF2780000 },
• { SIZE_1KB, SIZE_1KB },
• { NonDiscoverableDeviceDmaTypeCoherent, NonDiscoverableDeviceDmaTypeCoherent }

-}; +DECLARE_A7K8K_STORAGE_TEMPLATE; // // Tables with used devices @@ -93,7 +59,7 @@ EFI_STATUS PciEmulationInitXhci ( ) {

• PCIE_PLATFORM_DESC *Desc = &mA70x0PlatDescTemplate;

• MVHW_STORAGE_DESC *Desc = &mA7k8kStorageDescTemplate; EFI_STATUS Status; UINT8 i;

@@ -130,7 +96,7 @@ EFI_STATUS PciEmulationInitAhci ( ) {

• PCIE_PLATFORM_DESC *Desc = &mA70x0PlatDescTemplate;

• MVHW_STORAGE_DESC *Desc = &mA7k8kStorageDescTemplate; EFI_STATUS Status; UINT8 i;

@@ -167,7 +133,7 @@ EFI_STATUS PciEmulationInitSdhci ( ) {

• PCIE_PLATFORM_DESC *Desc = &mA70x0PlatDescTemplate;

• MVHW_STORAGE_DESC *Desc = &mA7k8kStorageDescTemplate; EFI_STATUS Status; UINT8 i;

1.8.3.1

On Mon, Mar 20, 2017 at 05:15:41PM +0100, Marcin Wojtas wrote:

Hi Leif,

Thanks for your remarks.

...

...

+#ifndef __MVHWDESCLIB_H__ +#define __MVHWDESCLIB_H__

+#include <Library/NonDiscoverableDeviceRegistrationLib.h>

+// +// Platform storage description +// +typedef struct {

• // XHCI
• UINT8 XhciDevCount;
• UINTN XhciBaseAddresses[4];
• UINTN XhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE XhciDmaType[4];
• // AHCI
• UINT8 AhciDevCount;
• UINTN AhciBaseAddresses[4];
• UINTN AhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE AhciDmaType[4];
• // SDHCI
• UINT8 SdhciDevCount;
• UINTN SdhciBaseAddresses[4];
• UINTN SdhciMemSize[4];
• NON_DISCOVERABLE_DEVICE_DMA_TYPE SdhciDmaType[4];

+} MVHW_STORAGE_DESC;

What do these hardcoded array sizes signify? What makes 4 guaranteed future-proof?

Well, 4 is biggest available for all variants of Armada 7k and 8k. I have no idea for future plans, but would you like to go with some bigger, arbitrarily chosen number (like 8)?.

Preferably, I'd see a non-arbitrary number :)

But seeing as this is just a code shuffling exercise, could you at least add a #define of some descriptive name?

...

...

+#define DECLARE_A7K8K_STORAGE_TEMPLATE \

Is XHCI "storage"?

Well, the devices attached to XHCI controller can be called storage. I thought such name would be more meaningful than PCI_EMULATION whatsoever. Do you wish some other particular name here?

I think I'd be happier with PCI_EMULATION, but really we are talking about non-discoverable hardware, so maybe something reflecting that?

...

...

+STATIC MVHW_STORAGE_DESC mA7k8kStorageDescTemplate = {\

• 4, /* XHCI */\
• { 0xF2500000, 0xF2510000, 0xF4500000, 0xF4510000 },\

These addresses would be a lot nicer as #defines.

Ok. Could be in the same header?

Absolutely. Just gives the opportunity to add some description into it - like CTRL0_PORT1, ... And confuses the history less if we manage to merge a typo in an address, and fix it.

/ Leif

...

...

• { SIZE_16KB, SIZE_16KB, SIZE_16KB, SIZE_16KB },\
• { NonDiscoverableDeviceDmaTypeCoherent,

NonDiscoverableDeviceDmaTypeCoherent,\

...

• NonDiscoverableDeviceDmaTypeCoherent,

NonDiscoverableDeviceDmaTypeCoherent },\

...

• 2, /* AHCI */\
• { 0xF2540000, 0xF4540000 },\
• { SIZE_8KB, SIZE_8KB },\
• { NonDiscoverableDeviceDmaTypeCoherent,

NonDiscoverableDeviceDmaTypeCoherent },\

...

• 2, /* SDHCI */\
• { 0xF06E0000, 0xF2780000 },\
• { SIZE_1KB, SIZE_1KB },\
• { NonDiscoverableDeviceDmaTypeCoherent,

NonDiscoverableDeviceDmaTypeCoherent }\

...

+}

+#endif /* __MVHWDESCLIB_H__ */ diff --git a/Platforms/Marvell/PciEmulation/PciEmulation.c

b/Platforms/Marvell/PciEmulation/PciEmulation.c

...

index 491b886..ffefffb 100644 --- a/Platforms/Marvell/PciEmulation/PciEmulation.c +++ b/Platforms/Marvell/PciEmulation/PciEmulation.c @@ -34,47 +34,13 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH

DAMAGE.

...

#include <PiDxe.h>

+#include <Library/MvHwDescLib.h>

Insert sorted, please.

Sure.

Thanks, Marcin

On Fri, Mar 17, 2017 at 01:40:32AM +0100, Marcin Wojtas wrote:

From: Konstantin Porotchkin kostap@marvell.com

Add support for multiple SATA host controllers to SERDES library. Remove single SATA controller base address from the description files and use shared platform description structure. This allows to reduce redundancy and pick existing PCD, used by PciEmulation. On the occasion add minor style fixes surrounding the changes.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Signed-off-by: Marcin Wojtas mw@semihalf.com

---

Platforms/Marvell/Armada/Armada70x0.dsc | 3 -- Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c | 48 ++++++++++++++++------- Platforms/Marvell/Library/ComPhyLib/ComPhyLib.inf | 3 +- Platforms/Marvell/Marvell.dec | 4 -- 4 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/Platforms/Marvell/Armada/Armada70x0.dsc b/Platforms/Marvell/Armada/Armada70x0.dsc index ab8fa3d..126d324 100644 --- a/Platforms/Marvell/Armada/Armada70x0.dsc +++ b/Platforms/Marvell/Armada/Armada70x0.dsc @@ -147,6 +147,3 @@ #ResetLib gMarvellTokenSpaceGuid.PcdResetRegAddress|0xf06f0084 gMarvellTokenSpaceGuid.PcdResetRegMask|0x1

• #SATA
• gMarvellTokenSpaceGuid.PcdSataBaseAddress|0xF2540000

diff --git a/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c b/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c index 07a4e6f..aeed53e 100755 --- a/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c +++ b/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c @@ -33,6 +33,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. *******************************************************************************/ #include "ComPhyLib.h" +#include <Library/MvHwDescLib.h> #define SD_LANE_ADDR_WIDTH 0x1000 #define HPIPE_ADDR_OFFSET 0x800 @@ -41,6 +42,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #define HPIPE_ADDR(base, Lane) (SD_ADDR(base, Lane) + HPIPE_ADDR_OFFSET) #define COMPHY_ADDR(base, Lane) (base + COMPHY_ADDR_LANE_WIDTH * Lane) +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.

• 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?

• /* 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.

• 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.

 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?

•  break;
  

  case PHY_TYPE_SATA2: case PHY_TYPE_SATA3:

•  Status = ComPhySataPowerUp(Lane, HpipeBaseAddr, ComPhyBaseAddr);
  

•  Status = ComPhySataPowerUp (Lane, HpipeBaseAddr, ComPhyBaseAddr, 1);
  

#define for that 1?

   break;
 case PHY_TYPE_USB3_HOST0:
 case PHY_TYPE_USB3_HOST1:

•  Status = ComphyUsb3PowerUp(Lane, HpipeBaseAddr, ComPhyBaseAddr);
  

•  Status = ComphyUsb3PowerUp (Lane, HpipeBaseAddr, ComPhyBaseAddr);
   break;
  

  case PHY_TYPE_SGMII0: case PHY_TYPE_SGMII1: case PHY_TYPE_SGMII2: case PHY_TYPE_SGMII3:

•  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.

-- 1.8.3.1

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;
  

  case PHY_TYPE_SATA2: case PHY_TYPE_SATA3:

•  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;
  

  case PHY_TYPE_SGMII0: case PHY_TYPE_SGMII1: case PHY_TYPE_SGMII2: case PHY_TYPE_SGMII3:

•  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