Re: [Linaro-uefi] [PATCH 1/8] Platforms/Marvell: Create ComPhyLib

On Fri, Jul 15, 2016 at 08:35:10AM +0200, Jan Dąbroś wrote:

Hi Leif,

Thanks for your comments. Please find my responses to your remarks inline. Thanks for answering questions in advance.

Regarding CheckPatch issue - it will be fixed in v2.

Best Regards, Jan

...

...

+Indicates how many different chips are placed on board. So far, up to 4 chips +are supported.

• gMarvellTokenSpaceGuid.PcdIsZVersionChip

+Indicates if Z1 chip version is used.

Does Z1 refer to a pre-release part, something for a different market segment, or...? If it's pre-production, you may want to consider just calling it PcdIsPreProductionChip or something? Not a hard requirement, just a suggestion, but the doc could do with a clarification.

It is a left-over from support for old chip revision. It will be removed in v2.

...

...

+{ 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" }

Are these well understood industry standard terms? If not, could the name of the document where these names come from be pointed out (even if the doc itself is available under NDA only)?

Ok, we will point to proper document or add description in our documentation regarding ComPhy.

...

...

• gMarvellTokenSpaceGuid.PcdChip0ComPhySpeeds

+Indicates PHY speeds in MHz. Currently supported are:

+{ 1250, 1500, 2500, 3000, 3125, 5000, 6000, 6250, 1031 }

Is that last 1031 a typo? (not numerical order, and looks a bit arbitrary)

It stands for 10 312 500 Hz. Renaming it to 10310 will be ok?

Well, whatever makes sense really. If it's a higher frequency than the preceding ones, it should be a higher number, and added in numerical order. 10310 sounds reasonable to me, but then I don't know what the others mean :)

...

...

+#ifndef __COMPHYLIB_H__ +#define __COMPHYLIB_H__

__MVCOMPHYLIB_H__ ?

...

+EFI_STATUS +ComPhyInit (

MvComPhyInit or MarvellComPhyInit?

It should be MvComPhyInit.

...

...

+#include "ComPhyLib.h"

+#define HPIPE_ADDR(base, lane) (base + 0x800 * lane)

Could we get a #define for that 0x800 as well?

Ok.

...

...

+#define COMPHY_RESET_REG 0x120

+#define COMPHY_RESET_SW_OFFSET 14 +#define COMPHY_RESET_SW_MASK (1 << COMPHY_RESET_SW_OFFSET) +#define COMPHY_RESET_CORE_OFFSET 13 +#define COMPHY_RESET_CORE_MASK (1 << COMPHY_RESET_CORE_OFFSET)

+#define COMPHY_PCI_MAC_CTRL 0x200

+#define COMPHY_PCI_EN_OFFSET 0 +#define COMPHY_PCI_EN_MASK (0x1 << COMPHY_PCI_EN_OFFSET) +#define COMPHY_PCI_AXI_CACHE_OFFSET 8 +#define COMPHY_PCI_AXI_CACHE_MASK (0xF << COMPHY_PCI_AXI_CACHE_OFFSET) +#define COMPHY_PCI_COHERENT 0x7 +#define COMPHY_PCI_X1_EN_OFFSET 14 +#define COMPHY_PCI_X1_EN_MASK (0x1 << COMPHY_PCI_X1_EN_OFFSET)

Should any/all of the above defines be made explicitly UL/ULL in order to avoid signed type promotion issues?

Proper prefixes will be added where necessary.

...

...

+STATIC +VOID +ComPhyPcieReleaseSoftReset (

• IN EFI_PHYSICAL_ADDRESS HpipeAddr
• )

+{

• /* Set MAX PLL Calibration */
• RegSet (HpipeAddr + HPIPE_KVCO_CALIB_CTRL_REG,
• 0x1 << HPIPE_KVCO_CALIB_CTRL_MAX_PLL_OFFSET,
• HPIPE_KVCO_CALIB_CTRL_MAX_PLL_MASK);
• RegSet (HpipeAddr + HPIPE_LANE_CONFIG0_REG,
• 0x1 << HPIPE_LANE_CONFIG0_MAX_PLL_OFFSET,
• HPIPE_LANE_CONFIG0_MAX_PLL_MASK);
• RegSet (HpipeAddr + HPIPE_LANE_CONFIG0_REG,
• 0x1 << HPIPE_LANE_CONFIG0_GEN2_PLL_OFFSET,
• HPIPE_LANE_CONFIG0_GEN2_PLL_MASK);
• /* DFE reset sequence */
• RegSet (HpipeAddr + HPIPE_PWR_CTR_REG,
• 0x1 << HPIPE_PWR_CTR_RST_DFE_OFFSET, HPIPE_PWR_CTR_RST_DFE_MASK);
• MicroSecondDelay (10);

Global comment: can we have some comment on all explicit delays as to why that particular delay value was chosen? Even if it's "seems to work", that will help pinpoint potential problem areas when debugging in future. If it's "X cycles at Y HZ", that will give credibility to it not just being "seems to work".

Ok.

...

Also, for each one - are we sure there is no memory barrier requirement? The MmioWrite32 call itself guarantees no automatic ordering semantics.

Thanks, we definitely should consider this. Could you point me to example code in edk2, where such barrier is used?

Well ... for drivers, I would prefer the use of MemoryFence(). There are plenty of examples in edk2, and one in OpenPlatformPkg.

...

...

+STATIC +EFI_STATUS +ComPhyPciePowerUp (

• IN UINT32 Lane,
• IN UINT32 PcieBy4,

Lane looks straightforward enough, but what is the meaning of PcieBy4?

This is flag indicating if first 4 lanes are set to PCIe type. I think one-line comment will be nice here.

...

...

• IN EFI_PHYSICAL_ADDRESS HpipeAddr
• )

+{

• UINT32 StartVal, BreakVal, MasterVal;
• /* Enable CLK 500 */
• RegSet (HpipeAddr + HPIPE_MISC_REG, 0x1 << HPIPE_MISC_CLK500_EN_OFFSET,
• HPIPE_MISC_CLK500_EN_MASK);
• /* Clear lane align off */

off or offset?

Off.

Could it be reworded as "disable clear lane align" or somesuch?

Regards,

Leif

...

...

• if (PcieBy4)
• RegSet (HpipeAddr + HPIPE_LANE_ALIGN_REG,

•  0x0 << HPIPE_LANE_ALIGN_OFF_OFFSET, HPIPE_LANE_ALIGN_OFF_MASK);
  

• /* Reference Frequency Select set 0 (for PCIe 0 = 100Mhz) */
• RegSet (HpipeAddr + HPIPE_PWR_PLL_REG, 0x0 << HPIPE_PWR_PLL_REF_FREQ_OFFSET,
• HPIPE_PWR_PLL_REF_FREQ_MASK);
• /* PHY Mode Select (set PCIe = 0x3) */
• RegSet (HpipeAddr + HPIPE_PWR_PLL_REG, 0x3 << HPIPE_PWR_PLL_PHY_MODE_OFFSET,
• HPIPE_PWR_PLL_PHY_MODE_MASK);
• /* Set PIPE RESET - SW reset for the PIPE */
• RegSet (HpipeAddr + HPIPE_RST_CLK_CTRL_REG,
• 0x1 << HPIPE_RST_CLK_CTRL_PIPE_RST_OFFSET,
• HPIPE_RST_CLK_CTRL_PIPE_RST_MASK);
• /* Set PCIe fixed mode to 8 bit @ 250 Mhz */
• RegSet (HpipeAddr + HPIPE_RST_CLK_CTRL_REG,
• 0x1 << HPIPE_RST_CLK_CTRL_FIXED_PCLK_OFFSET,
• HPIPE_RST_CLK_CTRL_FIXED_PCLK_MASK);
• /* Set 5Gbps for RX and TX */
• RegSet (HpipeAddr + HPIPE_ISOLATE_MODE_REG,
• 0x1 << HPIPE_ISOLATE_MODE_GEN_RX_OFFSET, HPIPE_ISOLATE_MODE_GEN_RX_MASK);
• RegSet (HpipeAddr + HPIPE_ISOLATE_MODE_REG,
• 0x1 << HPIPE_ISOLATE_MODE_GEN_TX_OFFSET, HPIPE_ISOLATE_MODE_GEN_TX_MASK);
• /* Set Max PHY generation setting - 5GBps */
• RegSet (HpipeAddr + HPIPE_INTERFACE_REG,
• 0x1 << HPIPE_INTERFACE_GEN_MAX_OFFSET, HPIPE_INTERFACE_GEN_MAX_MASK);
• /*
• • Set Lane Break/Start/Master:
• • master - Provide RefClock to MAC
• • start - Start of providing RefClock
• • break - Stop passing the RefClock
• */
• if (PcieBy4) {
• /*

• * If By4 Lane 0 - is master and start PHY
  

• * Lane 1-2 - pass refclock to next phy
  

• * Lane 3 - stop passing refclock
  

• */
  

• if (Lane == 0) {

•  StartVal = 0x1;
  

•  BreakVal = 0x0;
  

•  MasterVal = 0x1;
  

• } else if (Lane == 3) {

•  StartVal = 0x0;
  

•  BreakVal = 0x1;
  

•  MasterVal = 0x0;
  

• } else {

•  StartVal = 0x0;
  

•  BreakVal = 0x0;
  

•  MasterVal = 0x0;
  

• }
• } else {
• StartVal = 0x1;
• BreakVal = 0x1;
• MasterVal = 0x1;
• }
• RegSet (HpipeAddr + HPIPE_CLK_SRC_HI_REG,
• StartVal << HPIPE_CLK_SRC_HI_LANE_STRT_OFFSET,
• HPIPE_CLK_SRC_HI_LANE_STRT_MASK);
• RegSet (HpipeAddr + HPIPE_CLK_SRC_HI_REG,
• BreakVal << HPIPE_CLK_SRC_HI_LANE_BREAK_OFFSET,
• HPIPE_CLK_SRC_HI_LANE_BREAK_MASK);
• RegSet (HpipeAddr + HPIPE_CLK_SRC_HI_REG,
• MasterVal << HPIPE_CLK_SRC_HI_LANE_MASTER_OFFSET,
• HPIPE_CLK_SRC_HI_LANE_MASTER_MASK);
• /* For PCIe by4 need to reset after configure all 4 Lanes */
• if (PcieBy4) {
• return EFI_SUCCESS;
• }
• ComPhyPcieReleaseSoftReset(HpipeAddr);
• /* Release PIPE RESET - release PHY from reset */
• RegSet (HpipeAddr + HPIPE_RST_CLK_CTRL_REG,
• 0x0 << HPIPE_RST_CLK_CTRL_PIPE_RST_OFFSET,
• HPIPE_RST_CLK_CTRL_PIPE_RST_MASK);
• MicroSecondDelay (20000);
• /* Return the status of the PLL */
• return (EFI_STATUS) (MmioRead32 (HpipeAddr + HPIPE_LANE_STATUS0_REG) &
• HPIPE_LANE_STATUS0_PCLK_EN_MASK);

+}

The above function is very dense, and quite long. (I certainly can't follow what is actually going on.) Would it be possible to either break out parts into STATIC helper functions or:

• Break it up a bit with empty lines.
• Add a few more detailed multi-line comment blocks.

?

Ok.

...

...

+#define SD_ADDR(base, Lane) (base + 0x1000 * Lane) +#define HPIPE_ADDR(base, Lane) (SD_ADDR(base, Lane) + 0x800) +#define COMPHY_ADDR(base, Lane) (base + 0x28 * Lane)

Some defines for the magic values?

Ok.

...

...

+/*

• • For CP-110 we have 2 Selector registers "PHY Selectors"
• • and " PIPE Selectors".
• • PIPE selector include USB and PCIe options.
• • PHY selector include the Ethernet and SATA options, every Ethernet option
• • has different options, for example: serdes Lane2 had option Eth_port_0
• • that include (SGMII0, XAUI0, RXAUI0, KR)
• */

+COMPHY_MUX_DATA Cp110ComPhyMuxData[] = {

• /* Lane 0 */
• {4, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII2, 0x1},
• {PHY_TYPE_XAUI2, 0x1}, {PHY_TYPE_SATA1, 0x4} } },
• /* Lane 1 */
• {4, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII3, 0x1},
• {PHY_TYPE_XAUI3, 0x1}, {PHY_TYPE_SATA1, 0x4} } },
• /* Lane 2 */
• {6, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII0, 0x1},
• {PHY_TYPE_XAUI0, 0x1}, {PHY_TYPE_RXAUI0, 0x1}, {PHY_TYPE_KR, 0x1},
• {PHY_TYPE_SATA0, 0x4} } },
• /* Lane 3 */
• {8, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII0, 0x1},
• {PHY_TYPE_XAUI0, 0x1}, {PHY_TYPE_RXAUI0, 0x1}, {PHY_TYPE_KR, 0x1},
• {PHY_TYPE_XAUI1, 0x1}, {PHY_TYPE_RXAUI1, 0x1}, {PHY_TYPE_SATA1, 0x4} } },
• /* Lane 4 */
• {7, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_SGMII0, 0x2},
• {PHY_TYPE_XAUI0, 0x1}, {PHY_TYPE_RXAUI0, 0x1}, {PHY_TYPE_KR, 0x1},
• {PHY_TYPE_SGMII2, 0x1}, {PHY_TYPE_XAUI2, 0x1} } },
• /* Lane 5 */
• {6, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_XAUI1, 0x1},
• {PHY_TYPE_RXAUI1, 0x1}, {PHY_TYPE_SGMII3, 0x1}, {PHY_TYPE_XAUI3, 0x1},
• {PHY_TYPE_SATA1, 0x4} } },

+};

+COMPHY_MUX_DATA Cp110ComPhyPipeMuxData[] = {

• /* Lane 0 */
• {2, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_PCIE0, 0x4} } },
• /* Lane 1 */
• {4, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_USB3_HOST0, 0x1},
• {PHY_TYPE_USB3_DEVICE, 0x2}, {PHY_TYPE_PCIE0, 0x4} } },
• /* Lane 2 */
• {3, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_USB3_HOST0, 0x1},
• {PHY_TYPE_PCIE0, 0x4} } },
• /* Lane 3 */
• {3, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_USB3_HOST1, 0x1},
• {PHY_TYPE_PCIE0, 0x4} } },
• /* Lane 4 */
• {4, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_USB3_HOST1, 0x1},
• {PHY_TYPE_USB3_DEVICE, 0x2}, {PHY_TYPE_PCIE1, 0x4} } },
• /* Lane 5 */
• {2, {{PHY_TYPE_UNCONNECTED, 0x0}, {PHY_TYPE_PCIE2, 0x4} } },

+};

+STATIC +EFI_STATUS +ComPhyPciePowerUp (

• IN UINT32 Lane,
• IN UINT32 PcieBy4,
• IN EFI_PHYSICAL_ADDRESS HpipeBase,
• IN EFI_PHYSICAL_ADDRESS ComPhyBase
• )

+{

• EFI_STATUS Status = EFI_SUCCESS;
• UINT32 Mask, Data, PcieClk = 0;
• EFI_PHYSICAL_ADDRESS HpipeAddr = HPIPE_ADDR(HpipeBase, Lane);
• EFI_PHYSICAL_ADDRESS ComPhyAddr = COMPHY_ADDR(ComPhyBase, Lane);
• DEBUG((DEBUG_INFO, "ComPhy: stage: RFU configurations - hard reset "
• "ComPhy\n"));
• /* RFU configurations - hard reset ComPhy */
• Mask = COMMON_PHY_CFG1_PWR_UP_MASK;
• Data = 0x1 << COMMON_PHY_CFG1_PWR_UP_OFFSET;
• Mask |= COMMON_PHY_CFG1_PIPE_SELECT_MASK;
• Data |= 0x1 << COMMON_PHY_CFG1_PIPE_SELECT_OFFSET;
• Mask |= COMMON_PHY_CFG1_PWR_ON_RESET_MASK;
• Data |= 0x0 << COMMON_PHY_CFG1_PWR_ON_RESET_OFFSET;
• Mask |= COMMON_PHY_CFG1_CORE_RSTN_MASK;
• Data |= 0x0 << COMMON_PHY_CFG1_CORE_RSTN_OFFSET;
• Mask |= COMMON_PHY_PHY_MODE_MASK;
• Data |= 0x0 << COMMON_PHY_PHY_MODE_OFFSET;
• RegSet (ComPhyAddr + COMMON_PHY_CFG1_REG, Data, Mask);
• /* Release from hard reset */
• Mask = COMMON_PHY_CFG1_PWR_ON_RESET_MASK;
• Data = 0x1 << COMMON_PHY_CFG1_PWR_ON_RESET_OFFSET;
• Mask |= COMMON_PHY_CFG1_CORE_RSTN_MASK;
• Data |= 0x1 << COMMON_PHY_CFG1_CORE_RSTN_OFFSET;
• RegSet (ComPhyAddr + COMMON_PHY_CFG1_REG, Data, Mask);
• /* Wait 1ms - until band gap and ref clock ready */

Exemplary! :) (But consider whether you need a barrier here.)

...

• MicroSecondDelay (1000);
• /* Start ComPhy Configuration */
• DEBUG((DEBUG_INFO, "ComPhy: stage: ComPhy configuration\n"));
• /* Set PIPE soft reset */
• Mask = HPIPE_RST_CLK_CTRL_PIPE_RST_MASK;
• Data = 0x1 << HPIPE_RST_CLK_CTRL_PIPE_RST_OFFSET;
• /* Set PHY Datapath width mode for V0 */
• Mask |= HPIPE_RST_CLK_CTRL_FIXED_PCLK_MASK;
• Data |= 0x1 << HPIPE_RST_CLK_CTRL_FIXED_PCLK_OFFSET;
• /* Set Data bus width USB mode for V0 */
• Mask |= HPIPE_RST_CLK_CTRL_PIPE_WIDTH_MASK;
• Data |= 0x0 << HPIPE_RST_CLK_CTRL_PIPE_WIDTH_OFFSET;
• /* Set CORE_CLK output frequency for 250Mhz */
• Mask |= HPIPE_RST_CLK_CTRL_CORE_FREQ_SEL_MASK;
• Data |= 0x0 << HPIPE_RST_CLK_CTRL_CORE_FREQ_SEL_OFFSET;
• RegSet (HpipeAddr + HPIPE_RST_CLK_CTRL_REG, Data, Mask);
• /* Set PLL ready delay for 0x2 */
• RegSet (HpipeAddr + HPIPE_CLK_SRC_LO_REG,
• 0x2 << HPIPE_CLK_SRC_LO_PLL_RDY_DL_OFFSET,
• HPIPE_CLK_SRC_LO_PLL_RDY_DL_MASK);
• /* Set PIPE mode interface to PCIe3 - 0x1 */
• RegSet (HpipeAddr + HPIPE_CLK_SRC_HI_REG,
• 0x1 << HPIPE_CLK_SRC_HI_MODE_PIPE_OFFSET, HPIPE_CLK_SRC_HI_MODE_PIPE_MASK);
• /* Config update polarity equalization */
• RegSet (HpipeAddr + HPIPE_LANE_EQ_CFG1_REG,
• 0x1 << HPIPE_CFG_UPDATE_POLARITY_OFFSET, HPIPE_CFG_UPDATE_POLARITY_MASK);
• /* Set PIPE version 4 to mode enable */
• RegSet (HpipeAddr + HPIPE_DFE_CTRL_28_REG,
• 0x1 << HPIPE_DFE_CTRL_28_PIPE4_OFFSET, HPIPE_DFE_CTRL_28_PIPE4_MASK);
• /* Enable PIN clock 100M_125M */
• Mask = HPIPE_MISC_CLK100M_125M_MASK;
• Data = 0x1 << HPIPE_MISC_CLK100M_125M_OFFSET;
• /* Set PIN_TXDCLK_2X Clock Frequency Selection for outputs 500MHz clock */
• Mask |= HPIPE_MISC_TXDCLK_2X_MASK;
• Data |= 0x0 << HPIPE_MISC_TXDCLK_2X_OFFSET;
• /* Enable 500MHz Clock */
• Mask |= HPIPE_MISC_CLK500_EN_MASK;
• Data |= 0x1 << HPIPE_MISC_CLK500_EN_OFFSET;
• if (PcieClk) {
• /* Set reference clock comes from group 1 */
• Mask |= HPIPE_MISC_REFCLK_SEL_MASK;
• Data |= 0x0 << HPIPE_MISC_REFCLK_SEL_OFFSET;
• } else {
• /* Set reference clock comes from group 2 */
• Mask |= HPIPE_MISC_REFCLK_SEL_MASK;
• Data |= 0x1 << HPIPE_MISC_REFCLK_SEL_OFFSET;
• }
• RegSet (HpipeAddr + HPIPE_MISC_REG, Data, Mask);
• if (PcieClk) {
• /* Set reference frequcency select - 0x2 for 25MHz*/
• Mask = HPIPE_PWR_PLL_REF_FREQ_MASK;
• Data = 0x2 << HPIPE_PWR_PLL_REF_FREQ_OFFSET;
• } else {
• /* Set reference frequcency select - 0x0 for 100MHz*/
• Mask = HPIPE_PWR_PLL_REF_FREQ_MASK;
• Data = 0x0 << HPIPE_PWR_PLL_REF_FREQ_OFFSET;
• }
• /* Set PHY mode to PCIe */
• Mask |= HPIPE_PWR_PLL_PHY_MODE_MASK;
• Data |= 0x3 << HPIPE_PWR_PLL_PHY_MODE_OFFSET;
• RegSet (HpipeAddr + HPIPE_PWR_PLL_REG, Data, Mask);
• /*
• • Set the amount of time spent in the LoZ state - set
• • for 0x7 only if the PCIe clock is output
• */
• if (PcieClk)
• RegSet (HpipeAddr + HPIPE_GLOBAL_PM_CTRL,

•  0x7 << HPIPE_GLOBAL_PM_RXDLOZ_WAIT_OFFSET,
  

•  HPIPE_GLOBAL_PM_RXDLOZ_WAIT_MASK);
  

• /* Set Maximal PHY Generation Setting (8Gbps) */
• Mask = HPIPE_INTERFACE_GEN_MAX_MASK;
• Data = 0x2 << HPIPE_INTERFACE_GEN_MAX_OFFSET;
• /* Set Link Train Mode (Tx training control pins are used) */
• Mask |= HPIPE_INTERFACE_LINK_TRAIN_MASK;
• Data |= 0x1 << HPIPE_INTERFACE_LINK_TRAIN_OFFSET;
• RegSet (HpipeAddr + HPIPE_INTERFACE_REG, Data, Mask);
• /* Set Idle_sync enable */
• Mask = HPIPE_PCIE_IDLE_SYNC_MASK;
• Data = 0x1 << HPIPE_PCIE_IDLE_SYNC_OFFSET;
• /* Select bits for PCIE Gen3(32bit) */
• Mask |= HPIPE_PCIE_SEL_BITS_MASK;
• Data |= 0x2 << HPIPE_PCIE_SEL_BITS_OFFSET;
• RegSet (HpipeAddr + HPIPE_PCIE_REG0, Data, Mask);
• /* Enable Tx_adapt_g1 */
• Mask = HPIPE_TX_TRAIN_CTRL_G1_MASK;
• Data = 0x1 << HPIPE_TX_TRAIN_CTRL_G1_OFFSET;
• /* Enable Tx_adapt_gn1 */
• Mask |= HPIPE_TX_TRAIN_CTRL_GN1_MASK;
• Data |= 0x1 << HPIPE_TX_TRAIN_CTRL_GN1_OFFSET;
• /* Disable Tx_adapt_g0 */
• Mask |= HPIPE_TX_TRAIN_CTRL_G0_MASK;
• Data |= 0x0 << HPIPE_TX_TRAIN_CTRL_G0_OFFSET;
• RegSet (HpipeAddr + HPIPE_TX_TRAIN_CTRL_REG, Data, Mask);
• /* Set reg_tx_train_chk_init */
• Mask = HPIPE_TX_TRAIN_CHK_INIT_MASK;
• Data = 0x0 << HPIPE_TX_TRAIN_CHK_INIT_OFFSET;
• /* Enable TX_COE_FM_PIN_PCIE3_EN */
• Mask |= HPIPE_TX_TRAIN_COE_FM_PIN_PCIE3_MASK;
• Data |= 0x1 << HPIPE_TX_TRAIN_COE_FM_PIN_PCIE3_OFFSET;
• RegSet (HpipeAddr + HPIPE_TX_TRAIN_REG, Data, Mask);
• DEBUG((DEBUG_INFO, "ComPhy: stage: ComPhy power up\n"));
• /* Release from PIPE soft reset */
• RegSet (HpipeAddr + HPIPE_RST_CLK_CTRL_REG,
• 0x0 << HPIPE_RST_CLK_CTRL_PIPE_RST_OFFSET,
• HPIPE_RST_CLK_CTRL_PIPE_RST_MASK);
• /* Wait 15ms - for ComPhy calibration done */
• MicroSecondDelay (15000);
• DEBUG((DEBUG_INFO, "ComPhy: stage: Check PLL\n"));
• /* Read Lane status */
• Data = MmioRead32 (HpipeAddr + HPIPE_LANE_STATUS0_REG);
• if ((Data & HPIPE_LANE_STATUS0_PCLK_EN_MASK) == 0) {
• DEBUG((DEBUG_INFO, "ComPhy: Read from reg = %p - value = 0x%x\n",

•  HpipeAddr + HPIPE_LANE_STATUS0_REG, Data));
  

• DEBUG((DEBUG_INFO, "ComPhy: HPIPE_LANE_STATUS0_PCLK_EN_MASK is 0\n"));
• Status = EFI_D_ERROR;
• }
• return Status;

+}

Similar to earlier comment - could this be broken down into some smaller STATIC helper functions or have some multi-line comments inserted to explain the purpose of larger blocks of operations?

Ok.

...

...

+STATIC +UINTN +ComPhySataPowerUp (

• IN UINT32 Lane,
• IN EFI_PHYSICAL_ADDRESS HpipeBase,
• IN EFI_PHYSICAL_ADDRESS ComPhyBase
• )

+{

• EFI_STATUS Status;
• UINT32 Mask, Data;
• 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, Addr;
• SataBase = PcdGet32 (PcdSataBaseAddress);
• if (SataBase == 0) {
• DEBUG((DEBUG_INFO, "ComPhy: SATA address not defined\n"));
• return EFI_D_ERROR;
• }
• DEBUG((DEBUG_INFO, "ComPhy: stage: MAC configuration - power down "
• "ComPhy\n"));

While I'm sure the comment is correct, could we have the function name printed or something so it's obvious it's the "power up" function that is printing "power down"?

Ok.

...

...

• /*
• • MAC configuration - power down ComPhy
• • Use indirect address for vendor specific SATA control register
• */
• RegSet (SataBase + SATA3_VENDOR_ADDRESS,
• SATA_CONTROL_REG << SATA3_VENDOR_ADDR_OFSSET, SATA3_VENDOR_ADDR_MASK);
• /* SATA 0 power down */
• Mask = SATA3_CTRL_SATA0_PD_MASK;
• Data = 0x1 << SATA3_CTRL_SATA0_PD_OFFSET;
• /* SATA 1 power down */
• Mask |= SATA3_CTRL_SATA1_PD_MASK;
• Data |= 0x1 << SATA3_CTRL_SATA1_PD_OFFSET;
• /* SATA SSU disable */
• Mask |= SATA3_CTRL_SATA1_ENABLE_MASK;
• Data |= 0x0 << SATA3_CTRL_SATA1_ENABLE_OFFSET;
• /* SATA port 1 disable */
• Mask |= SATA3_CTRL_SATA_SSU_MASK;
• Data |= 0x0 << SATA3_CTRL_SATA_SSU_OFFSET;
• RegSet (SataBase + SATA3_VENDOR_DATA, Data, Mask);
• DEBUG((DEBUG_INFO, "ComPhy: stage: RFU configurations - hard reset "
• "ComPhy\n"));
• /* RFU configurations - hard reset ComPhy */
• Mask = COMMON_PHY_CFG1_PWR_UP_MASK;
• Data = 0x1 << COMMON_PHY_CFG1_PWR_UP_OFFSET;
• Mask |= COMMON_PHY_CFG1_PIPE_SELECT_MASK;
• Data |= 0x0 << COMMON_PHY_CFG1_PIPE_SELECT_OFFSET;
• Mask |= COMMON_PHY_CFG1_PWR_ON_RESET_MASK;
• Data |= 0x0 << COMMON_PHY_CFG1_PWR_ON_RESET_OFFSET;
• Mask |= COMMON_PHY_CFG1_CORE_RSTN_MASK;
• Data |= 0x0 << COMMON_PHY_CFG1_CORE_RSTN_OFFSET;
• RegSet (ComPhyAddr + COMMON_PHY_CFG1_REG, Data, Mask);
• /* Set select Data width 40Bit - SATA mode only */
• RegSet (ComPhyAddr + COMMON_PHY_CFG6_REG,
• 0x1 << COMMON_PHY_CFG6_IF_40_SEL_OFFSET, COMMON_PHY_CFG6_IF_40_SEL_MASK);
• /* Release from hard reset in SD external */
• Mask = SD_EXTERNAL_CONFIG1_RESET_IN_MASK;
• Data = 0x1 << SD_EXTERNAL_CONFIG1_RESET_IN_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG1_RESET_CORE_MASK;
• Data |= 0x1 << SD_EXTERNAL_CONFIG1_RESET_CORE_OFFSET;
• RegSet (SdIpAddr + SD_EXTERNAL_CONFIG1_REG, Data, Mask);
• /* Wait 1ms - until band gap and ref clock ready */
• MicroSecondDelay (1000);
• DEBUG((DEBUG_INFO, "ComPhy: stage: Comphy configuration\n"));
• /* Start ComPhy Configuration */
• /* Set reference clock to comes from group 1 - choose 25Mhz */
• RegSet (HpipeAddr + HPIPE_MISC_REG,
• 0x0 << HPIPE_MISC_REFCLK_SEL_OFFSET, HPIPE_MISC_REFCLK_SEL_MASK);
• /* Reference frequency select set 1 (for SATA = 25Mhz) */
• Mask = HPIPE_PWR_PLL_REF_FREQ_MASK;
• Data = 0x1 << HPIPE_PWR_PLL_REF_FREQ_OFFSET;
• /* PHY mode select (set SATA = 0x0 */
• Mask |= HPIPE_PWR_PLL_PHY_MODE_MASK;
• Data |= 0x0 << HPIPE_PWR_PLL_PHY_MODE_OFFSET;
• RegSet (HpipeAddr + HPIPE_PWR_PLL_REG, Data, Mask);
• /* Set max PHY generation setting - 6Gbps */
• RegSet (HpipeAddr + HPIPE_INTERFACE_REG,
• 0x2 << HPIPE_INTERFACE_GEN_MAX_OFFSET, HPIPE_INTERFACE_GEN_MAX_MASK);
• /* Set select Data width 40Bit (SEL_BITS[2:0]) */
• RegSet (HpipeAddr + HPIPE_LOOPBACK_REG,
• 0x2 << HPIPE_LOOPBACK_SEL_OFFSET, HPIPE_LOOPBACK_SEL_MASK);
• DEBUG((DEBUG_INFO, "ComPhy: stage: Analog paramters from ETP(HW)\n"));
• /* DFE reset sequence */
• RegSet (HpipeAddr + HPIPE_PWR_CTR_REG,
• 0x1 << HPIPE_PWR_CTR_RST_DFE_OFFSET, HPIPE_PWR_CTR_RST_DFE_MASK);
• RegSet (HpipeAddr + HPIPE_PWR_CTR_REG,
• 0x0 << HPIPE_PWR_CTR_RST_DFE_OFFSET, HPIPE_PWR_CTR_RST_DFE_MASK);
• /* SW reset for interupt logic */
• RegSet (HpipeAddr + HPIPE_PWR_CTR_REG,
• 0x1 << HPIPE_PWR_CTR_SFT_RST_OFFSET, HPIPE_PWR_CTR_SFT_RST_MASK);
• RegSet (HpipeAddr + HPIPE_PWR_CTR_REG,
• 0x0 << HPIPE_PWR_CTR_SFT_RST_OFFSET, HPIPE_PWR_CTR_SFT_RST_MASK);
• DEBUG((DEBUG_INFO, "ComPhy: stage: ComPhy power up\n"));
• /*
• • MAC configuration - power up ComPhy - power up PLL/TX/RX
• • Use indirect address for vendor specific SATA control register
• */
• RegSet (SataBase + SATA3_VENDOR_ADDRESS,
• SATA_CONTROL_REG << SATA3_VENDOR_ADDR_OFSSET, SATA3_VENDOR_ADDR_MASK);
• /* SATA 0 power up */
• Mask = SATA3_CTRL_SATA0_PD_MASK;
• Data = 0x0 << SATA3_CTRL_SATA0_PD_OFFSET;
• /* SATA 1 power up */
• Mask |= SATA3_CTRL_SATA1_PD_MASK;
• Data |= 0x0 << SATA3_CTRL_SATA1_PD_OFFSET;
• /* SATA SSU enable */
• Mask |= SATA3_CTRL_SATA1_ENABLE_MASK;
• Data |= 0x1 << SATA3_CTRL_SATA1_ENABLE_OFFSET;
• /* SATA port 1 enable */
• Mask |= SATA3_CTRL_SATA_SSU_MASK;
• Data |= 0x1 << SATA3_CTRL_SATA_SSU_OFFSET;
• RegSet (SataBase + SATA3_VENDOR_DATA, Data, Mask);
• if (PcdGetBool(PcdIsZVersionChip)) {
• /*

• * Reduce read & write burst size to 64 byte due to bug in
  

• * AP-806-Z Aurora 2 that prohibits writes larger than 64 byte
  

• */
  

• MmioWrite32(SataBase + SATA3_VENDOR_ADDRESS, 0x4);
• Mask = 0x77;
• Data = 0x44; /* 4 = 64 bytes burst */
• RegSet (SataBase + SATA3_VENDOR_DATA, Data, Mask);
• }
• /* MBUS request size and interface select register */
• RegSet (SataBase + SATA3_VENDOR_ADDRESS,
• SATA_MBUS_SIZE_SELECT_REG << SATA3_VENDOR_ADDR_OFSSET,

•  SATA3_VENDOR_ADDR_MASK);
  

• /* Mbus regret enable */
• RegSet (SataBase + SATA3_VENDOR_DATA, 0x1 << SATA_MBUS_REGRET_EN_OFFSET,
• SATA_MBUS_REGRET_EN_MASK);
• DEBUG((DEBUG_INFO, "ComPhy: stage: Check PLL\n"));
• Addr = SdIpAddr + SD_EXTERNAL_STATUS0_REG;
• Data = SD_EXTERNAL_STATUS0_PLL_TX_MASK & SD_EXTERNAL_STATUS0_PLL_RX_MASK;
• Mask = Data;
• Data = PollingWithTimeout (Addr, Data, Mask, 15000);
• if (Data != 0) {
• DEBUG((DEBUG_INFO, "ComPhy: Read from reg = %p - value = 0x%x\n",

•  HpipeAddr + HPIPE_LANE_STATUS0_REG, Data));
  

• DEBUG((DEBUG_ERROR, "ComPhy: SD_EXTERNAL_STATUS0_PLL_TX is %d,"

•  "SD_EXTERNAL_STATUS0_PLL_RX is %d\n",
  

•  (Data & SD_EXTERNAL_STATUS0_PLL_TX_MASK),
  

•  (Data & SD_EXTERNAL_STATUS0_PLL_RX_MASK)));
  

• Status = EFI_D_ERROR;
• }
• return Status;

+}

Also a very long function - although more readable than the others I pointed out.

Ok.

...

...

+STATIC +UINTN +ComPhySgmiiPowerUp (

• IN UINT32 Lane,
• IN UINT32 SgmiiSpeed,
• IN EFI_PHYSICAL_ADDRESS HpipeBase,
• IN EFI_PHYSICAL_ADDRESS ComPhyBase
• )

+{

• EFI_STATUS Status = EFI_SUCCESS;
• UINT32 Mask, Data;
• 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 Addr;
• DEBUG((DEBUG_INFO, "ComPhy: stage: RFU configurations - hard reset "
• "ComPhy\n"));
• /* RFU configurations - hard reset ComPhy */
• Mask = COMMON_PHY_CFG1_PWR_UP_MASK;
• Data = 0x1 << COMMON_PHY_CFG1_PWR_UP_OFFSET;
• Mask |= COMMON_PHY_CFG1_PIPE_SELECT_MASK;
• Data |= 0x0 << COMMON_PHY_CFG1_PIPE_SELECT_OFFSET;
• RegSet (ComPhyAddr + COMMON_PHY_CFG1_REG, Data, Mask);
• /* Select Baud Rate of Comphy And PD_PLL/Tx/Rx */
• Mask = SD_EXTERNAL_CONFIG0_SD_PU_PLL_MASK;
• Data = 0x0 << SD_EXTERNAL_CONFIG0_SD_PU_PLL_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG0_SD_PHY_GEN_RX_MASK;
• Mask |= SD_EXTERNAL_CONFIG0_SD_PHY_GEN_TX_MASK;
• if (SgmiiSpeed == PHY_SPEED_1_25G) {
• Data |= 0x6 << SD_EXTERNAL_CONFIG0_SD_PHY_GEN_RX_OFFSET;
• Data |= 0x6 << SD_EXTERNAL_CONFIG0_SD_PHY_GEN_TX_OFFSET;
• } else {
• /* 3.125G */
• Data |= 0x8 << SD_EXTERNAL_CONFIG0_SD_PHY_GEN_RX_OFFSET;
• Data |= 0x8 << SD_EXTERNAL_CONFIG0_SD_PHY_GEN_TX_OFFSET;
• }
• Mask |= SD_EXTERNAL_CONFIG0_SD_PU_RX_MASK;
• Data |= 0 << SD_EXTERNAL_CONFIG0_SD_PU_RX_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG0_SD_PU_TX_MASK;
• Data |= 0 << SD_EXTERNAL_CONFIG0_SD_PU_TX_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG0_HALF_BUS_MODE_MASK;
• Data |= 1 << SD_EXTERNAL_CONFIG0_HALF_BUS_MODE_OFFSET;
• RegSet (SdIpAddr + SD_EXTERNAL_CONFIG0_REG, Data, Mask);
• /* Release from hard reset */
• Mask = SD_EXTERNAL_CONFIG1_RESET_IN_MASK;
• Data = 0x0 << SD_EXTERNAL_CONFIG1_RESET_IN_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG1_RESET_CORE_MASK;
• Data |= 0x0 << SD_EXTERNAL_CONFIG1_RESET_CORE_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG1_RF_RESET_IN_MASK;
• Data |= 0x0 << SD_EXTERNAL_CONFIG1_RF_RESET_IN_OFFSET;
• RegSet (SdIpAddr + SD_EXTERNAL_CONFIG1_REG, Data, Mask);
• /* Release from hard reset */
• Mask = SD_EXTERNAL_CONFIG1_RESET_IN_MASK;
• Data = 0x1 << SD_EXTERNAL_CONFIG1_RESET_IN_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG1_RESET_CORE_MASK;
• Data |= 0x1 << SD_EXTERNAL_CONFIG1_RESET_CORE_OFFSET;
• RegSet (SdIpAddr+ SD_EXTERNAL_CONFIG1_REG, Data, Mask);
• /* Wait 1ms - until band gap and ref clock ready */
• MicroSecondDelay (1000);
• /* Start ComPhy Configuration */
• DEBUG((DEBUG_INFO, "ComPhy: stage: ComPhy configuration\n"));
• /* Set reference clock */
• Mask = HPIPE_MISC_REFCLK_SEL_MASK;
• Data = 0x0 << HPIPE_MISC_REFCLK_SEL_OFFSET;
• RegSet (HpipeAddr + HPIPE_MISC_REG, Data, Mask);
• /* Power and PLL Control */
• Mask = HPIPE_PWR_PLL_REF_FREQ_MASK;
• Data = 0x1 << HPIPE_PWR_PLL_REF_FREQ_OFFSET;
• Mask |= HPIPE_PWR_PLL_PHY_MODE_MASK;
• Data |= 0x4 << HPIPE_PWR_PLL_PHY_MODE_OFFSET;
• RegSet (HpipeAddr + HPIPE_PWR_PLL_REG, Data, Mask);
• /* Loopback register */
• Mask = HPIPE_LOOPBACK_SEL_MASK;
• Data = 0x1 << HPIPE_LOOPBACK_SEL_OFFSET;
• RegSet (HpipeAddr + HPIPE_LOOPBACK_REG, Data, Mask);
• /* Rx control 1 */
• Mask = HPIPE_RX_CONTROL_1_RXCLK2X_SEL_MASK;
• Data = 0x1 << HPIPE_RX_CONTROL_1_RXCLK2X_SEL_OFFSET;
• Mask |= HPIPE_RX_CONTROL_1_CLK8T_EN_MASK;
• Data |= 0x0 << HPIPE_RX_CONTROL_1_CLK8T_EN_OFFSET;
• RegSet (HpipeAddr + HPIPE_RX_CONTROL_1_REG, Data, Mask);
• /* DTL Control */
• Mask = HPIPE_PWR_CTR_DTL_FLOOP_EN_MASK;
• Data = 0x0 << HPIPE_PWR_CTR_DTL_FLOOP_EN_OFFSET;
• RegSet (HpipeAddr + HPIPE_PWR_CTR_DTL_REG, Data, Mask);
• /* Set analog paramters from ETP(HW) - for now use the default data */
• DEBUG((DEBUG_INFO, "ComPhy: stage: Analog paramters from ETP(HW)\n"));
• RegSet (HpipeAddr + HPIPE_G1_SET_0_REG,
• 0x1 << HPIPE_G1_SET_0_G1_TX_EMPH1_OFFSET, HPIPE_G1_SET_0_G1_TX_EMPH1_MASK);
• DEBUG((DEBUG_INFO, "ComPhy: stage: RFU configurations - Power Up "
• "PLL,Tx,Rx\n"));
• /* SerDes External Configuration */
• Mask = SD_EXTERNAL_CONFIG0_SD_PU_PLL_MASK;
• Data = 0x1 << SD_EXTERNAL_CONFIG0_SD_PU_PLL_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG0_SD_PU_RX_MASK;
• Data |= 0x1 << SD_EXTERNAL_CONFIG0_SD_PU_RX_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG0_SD_PU_TX_MASK;
• Data |= 0x1 << SD_EXTERNAL_CONFIG0_SD_PU_TX_OFFSET;
• RegSet (SdIpAddr + SD_EXTERNAL_CONFIG0_REG, Data, Mask);
• /* Check PLL rx & tx ready */
• Addr = SdIpAddr + SD_EXTERNAL_STATUS0_REG;
• Data = SD_EXTERNAL_STATUS0_PLL_RX_MASK | SD_EXTERNAL_STATUS0_PLL_TX_MASK;
• Mask = Data;
• Data = PollingWithTimeout (Addr, Data, Mask, 15000);
• if (Data != 0) {
• DEBUG((DEBUG_ERROR, "ComPhy: Read from reg = %p - value = 0x%x\n",

•  SdIpAddr + SD_EXTERNAL_STATUS0_REG, Data));
  

• DEBUG((DEBUG_ERROR, "ComPhy: SD_EXTERNAL_STATUS0_PLL_RX is %d, "

•  "SD_EXTERNAL_STATUS0_PLL_TX is %d\n",
  

•  (Data & SD_EXTERNAL_STATUS0_PLL_RX_MASK),
  

•  (Data & SD_EXTERNAL_STATUS0_PLL_TX_MASK)));
  

• Status = EFI_D_ERROR;
• }
• /* RX init */
• Mask = SD_EXTERNAL_CONFIG1_RX_INIT_MASK;
• Data = 0x1 << SD_EXTERNAL_CONFIG1_RX_INIT_OFFSET;
• RegSet (SdIpAddr + SD_EXTERNAL_CONFIG1_REG, Data, Mask);
• /* Check that RX init done */
• Addr = SdIpAddr + SD_EXTERNAL_STATUS0_REG;
• Data = SD_EXTERNAL_STATUS0_RX_INIT_MASK;
• Mask = Data;
• Data = PollingWithTimeout (Addr, Data, Mask, 100);
• if (Data != 0) {
• DEBUG((DEBUG_ERROR, "ComPhy: Read from reg = %p - value = 0x%x\n",

•  SdIpAddr + SD_EXTERNAL_STATUS0_REG, Data));
  

• DEBUG((DEBUG_ERROR, "ComPhy: SD_EXTERNAL_STATUS0_RX_INIT is 0\n"));
• Status = EFI_D_ERROR;
• }
• Mask = SD_EXTERNAL_CONFIG1_RX_INIT_MASK;
• Data = 0x0 << SD_EXTERNAL_CONFIG1_RX_INIT_OFFSET;
• Mask |= SD_EXTERNAL_CONFIG1_RF_RESET_IN_MASK;
• Data |= 0x1 << SD_EXTERNAL_CONFIG1_RF_RESET_IN_OFFSET;
• RegSet (SdIpAddr + SD_EXTERNAL_CONFIG1_REG, Data, Mask);
• return Status;

+}

Long function. STATIC helper functions?

Ok.

...

...

+UINT32 +ParseSerdesSpeed (

• UINT32 Value
• )

+{

• UINT32 i;
• UINT32 ValueTable [] = {0, 1250, 1500, 2500, 3000, 3125,

•                      5000, 6000, 6250, 1031};
  

10310?

As above, I will change to 10310.

...

...

• ChipCount = PcdGet32 (PcdComPhyChipCount);
• GetComPhyPcd(0);
• GetComPhyPcd(1);
• GetComPhyPcd(2);
• GetComPhyPcd(3);

This macro makes it unclear which structure is actually being operated on. Can it add another input parameter so it becomes (ChipConfig, 0) and so on?

Ok, additional parameter will be added.

...

...

+/***** Parsing PCD *****/ +#define GET_TYPE_STRING(id) _PCD_GET_MODE_PTR_PcdChip##id##Compatible +#define GET_LANE_TYPE(id) _PCD_GET_MODE_PTR_PcdChip##id##ComPhyTypes +#define GET_LANE_SPEED(id) _PCD_GET_MODE_PTR_PcdChip##id##ComPhySpeeds +#define GET_LANE_INV(id) _PCD_GET_MODE_PTR_PcdChip##id##ComPhyInvFlags +#define GET_COMPHY_BASE_ADDR(id) _PCD_GET_MODE_64_PcdChip##id##ComPhyBaseAddress +#define GET_HPIPE3_BASE_ADDR(id) _PCD_GET_MODE_64_PcdChip##id##Hpipe3BaseAddress +#define GET_MUX_BIT_COUNT(id) _PCD_GET_MODE_32_PcdChip##id##ComPhyMuxBitCount +#define GET_MAX_LANES(id) _PCD_GET_MODE_32_PcdChip##id##ComPhyMaxLanes

Similarly to what happened in Platforms/Marvell/Library/MppLib/MppLib.c, could these be changed to use PcdGet* instead?

Ok.

...

...

+#define FillLaneMap(id) { \

• ParsePcdString((CHAR16 *) GET_LANE_TYPE(id), ChipConfig[id].LanesCount, NULL, LaneData[id].TypeStr); \
• ParsePcdString((CHAR16 *) GET_LANE_SPEED(id), ChipConfig[id].LanesCount, LaneData[id].SpeedValue, NULL); \
• ParsePcdString((CHAR16 *) GET_LANE_INV(id), ChipConfig[id].LanesCount, LaneData[id].InvFlag, NULL); \

+}

This macro hides LaneData also - so can that be added as well, turning the below into a 3-input macro? If the above is _never_ called externally, the above macro can stay as is, but if so I would like it #undef'd below GetComPhyPcd.

Ok.