Re: [Linaro-uefi] [PATCH edk2-platforms v1 10/16] Hisilicon/D06: Modify for M7 self-Adapte support

On Fri, Feb 01, 2019 at 09:34:30PM +0800, Ming Huang wrote:

As new M7(Cortex-M7) firmware support self-adapte, so do not need BIOS to implement some function, remove useless funtions and report CPU0/CPU1 Nic NCL offset to M7.

I don't really care that some other device in the system is a Cortex-A7. What is its function? Is it an SCP, an MCP, ? Please describe its function rather than its implementation.

What are the external dependencies? Is this addressed by one of the patches for edk2-non-osi?

More style issues below.

Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ming Huang ming.huang@linaro.org

---

Platform/Hisilicon/D06/Library/OemNicLib/OemNicLib.c | 272 ++++---------------- 1 file changed, 45 insertions(+), 227 deletions(-)

diff --git a/Platform/Hisilicon/D06/Library/OemNicLib/OemNicLib.c b/Platform/Hisilicon/D06/Library/OemNicLib/OemNicLib.c index aaf990216982..9bf274e1b991 100644 --- a/Platform/Hisilicon/D06/Library/OemNicLib/OemNicLib.c +++ b/Platform/Hisilicon/D06/Library/OemNicLib/OemNicLib.c @@ -21,44 +21,21 @@ #include <Library/OemNicLib.h> #define CPU2_SFP2_100G_CARD_OFFSET 0x25 -#define CPU1_SFP1_LOCATE_OFFSET 0x16 -#define CPU1_SFP0_LOCATE_OFFSET 0x12 -#define CPU2_SFP1_LOCATE_OFFSET 0x21 -#define CPU2_SFP0_LOCATE_OFFSET 0x19 -#define CPU2_SFP2_10G_GE_CARD_OFFSET 0x25 -#define SFP_10G_SPEED 10 -#define SFP_25G_SPEED 25 -#define SFP_100G_SPEED 100 -#define SFP_GE_SPEED 1

-#define SFP_GE_SPEED_VAL_VENDOR_FINISAR 0x0C -#define SFP_GE_SPEED_VAL 0x0D -#define SFP_10G_SPEED_VAL 0x67 -#define SFP_25G_SPEED_VAL 0xFF +#define SOCKET1_NET_PORT_100G 1 +#define SOCKET0_NET_PORT_NUM 4 +#define SOCKET1_NET_PORT_NUM 2 #define CARD_PRESENT_100G (BIT7) -#define CARD_PRESENT_10G (BIT0) -#define SELECT_SFP_BY_INDEX(index) (1 << (index - 1)) -#define SPF_SPEED_OFFSET 12

-#define SFP_DEVICE_ADDRESS 0x50 -#define CPU1_9545_I2C_ADDR 0x70 -#define CPU2_9545_I2C_ADDR 0x71

-#define FIBER_PRESENT 0 -#define CARD_PRESENT 1 -#define I2C_PORT_SFP 4 -#define CPU2_I2C_PORT_SFP 5

-#define SOCKET_0 0 -#define SOCKET_1 1 #define EEPROM_I2C_PORT 4 #define EEPROM_PAGE_SIZE 0x40 #define MAC_ADDR_LEN 6 #define I2C_OFFSET_EEPROM_ETH0 (0xc00) #define I2C_SLAVEADDR_EEPROM (0x52) +#define SRAM_NIC_NCL1_OFFSET_FLAG 0xA0E87FE0 +#define SRAM_NIC_NCL2_OFFSET_FLAG 0xA0E87FE4

Is this just a hard-coded address in SRAM? Where is it specified? I don't think "_FLAG" is the correct name for this #define - this is the actual offset value. So _OFFSET_ADDRESS would be more descriptive.

#pragma pack(1) typedef struct { UINT16 Crc16; @@ -114,204 +91,6 @@ UINT16 CrcTable16[256] = { 0x6E17, 0x7E36, 0x4E55, 0x5E74, 0x2E93, 0x3EB2, 0x0ED1, 0x1EF0, }; -EFI_STATUS -GetSfpSpeed (

• UINT16 Socket,
• UINT16 SfpNum,
• UINT8* FiberSpeed
• )

-{

• EFI_STATUS Status;
• I2C_DEVICE SpdDev;
• UINT8 SfpSelect;
• UINT8 SfpSpeed;
• UINT32 RegAddr;
• UINT16 I2cAddr;
• UINT32 SfpPort;
• SfpSpeed = 0x0;
• if (Socket == SOCKET_1) {
• I2cAddr = CPU2_9545_I2C_ADDR;
• SfpPort = CPU2_I2C_PORT_SFP;
• } else {
• I2cAddr = CPU1_9545_I2C_ADDR;
• SfpPort = I2C_PORT_SFP;
• }
• Status = I2CInit (Socket, SfpPort, Normal);
• if (EFI_ERROR (Status)) {
• DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Socket%d Call I2CInit failed! p1=0x%x.\n",

•        __FUNCTION__, __LINE__, Socket, Status));
  

• return Status;
• }
• SpdDev.Socket = Socket;
• SpdDev.DeviceType = DEVICE_TYPE_SPD;
• SpdDev.Port = SfpPort;
• SpdDev.SlaveDeviceAddress = I2cAddr;
• RegAddr = 0x0;
• SfpSelect = SELECT_SFP_BY_INDEX (SfpNum);
• Status = I2CWrite (&SpdDev, RegAddr, 1, &SfpSelect);
• if (EFI_ERROR (Status)) {
• DEBUG ((DEBUG_ERROR, "I2CWrite Error =%r.\n", Status));
• return Status;
• }
• SpdDev.Socket = Socket;
• SpdDev.DeviceType = DEVICE_TYPE_SPD;
• SpdDev.Port = SfpPort;
• SpdDev.SlaveDeviceAddress = SFP_DEVICE_ADDRESS;
• RegAddr = SPF_SPEED_OFFSET;
• Status = I2CRead (&SpdDev, RegAddr, 1, &SfpSpeed);
• if (EFI_ERROR (Status)) {
• DEBUG ((DEBUG_ERROR, "I2CRead Error =%r.\n", Status));
• return Status;
• }
• DEBUG ((DEBUG_INFO, "BR, Nominal, Nominal signalling rate, SfpSpeed: 0x%x\n",

•     SfpSpeed));
  

• if (SfpSpeed == SFP_10G_SPEED_VAL) {
• *FiberSpeed = SFP_10G_SPEED;
• } else if (SfpSpeed == SFP_25G_SPEED_VAL) {
• *FiberSpeed = SFP_25G_SPEED;
• } else if ((SfpSpeed == SFP_GE_SPEED_VAL) ||

•         (SfpSpeed == SFP_GE_SPEED_VAL_VENDOR_FINISAR)) {
  

• *FiberSpeed = SFP_GE_SPEED;
• }
• return EFI_SUCCESS;

-}

-//Fiber1Type/Fiber2Type/Fiber3Type return: SFP_10G_SPEED, SFP_100G_SPEED, SFP_GE_SPEED -UINT32 -GetCpu2FiberType (

• UINT8* Fiber1Type,
• UINT8* Fiber2Type,
• UINT8* Fiber100Ge
• )

-{

• EFI_STATUS Status;
• UINT16 SfpNum1;
• UINT8 SfpSpeed1;
• UINT16 SfpNum2;
• UINT8 SfpSpeed2;
• SfpNum1 = 0x1;
• SfpSpeed1 = SFP_10G_SPEED;
• SfpNum2 = 0x2;
• SfpSpeed2 = SFP_10G_SPEED;
• *Fiber100Ge = 0x0;
• *Fiber1Type = SFP_10G_SPEED;
• *Fiber2Type = SFP_10G_SPEED;
• if ((ReadCpldReg (CPU2_SFP2_100G_CARD_OFFSET) & CARD_PRESENT_100G) != 0) {
• // 100 Ge card
• *Fiber1Type = SFP_10G_SPEED;
• *Fiber2Type = SFP_10G_SPEED;
• *Fiber100Ge = SFP_100G_SPEED;
• DEBUG ((DEBUG_ERROR,"Detect Fiber SFP_100G is Present, Set 100Ge\n"));
• } else if ((ReadCpldReg (CPU2_SFP2_10G_GE_CARD_OFFSET) & CARD_PRESENT_10G) != 0) {
• *Fiber100Ge = 0x0;
• *Fiber1Type = SFP_10G_SPEED;
• *Fiber2Type = SFP_10G_SPEED;
• if (ReadCpldReg (CPU2_SFP0_LOCATE_OFFSET) == FIBER_PRESENT) {

•  // Fiber detected in CPU2 slot0, read speed via i2c
  

•  Status = GetSfpSpeed (SOCKET_1, SfpNum1, &SfpSpeed1);
  

•  if (EFI_ERROR (Status)) {
  

•    DEBUG((DEBUG_ERROR,
  

•           "Get Socket1 Sfp%d Speed Error: %r.\n",
  

•           SfpNum1,
  

•           Status));
  

•    return Status;
  

•  }
  

•  if (SfpSpeed1 == SFP_25G_SPEED) {
  

•    // P1 don't support 25G, so set speed to 10G
  

•    *Fiber1Type = SFP_10G_SPEED;
  

•  } else {
  

•    *Fiber1Type = SfpSpeed1;
  

•  }
  

• } else {

•  // No fiber, set speed to 10G
  

•  *Fiber1Type = SFP_10G_SPEED;
  

• }
• if (ReadCpldReg (CPU2_SFP1_LOCATE_OFFSET) == FIBER_PRESENT) {

•  // Fiber detected in CPU2 slot1, read speed via i2c
  

•  Status = GetSfpSpeed (SOCKET_1, SfpNum2, &SfpSpeed2);
  

•  if (EFI_ERROR (Status)) {
  

•    DEBUG ((DEBUG_ERROR, "Get Sfp%d Speed Error: %r.\n", SfpNum2, Status));
  

•    return Status;
  

•  }
  

•  if (SfpSpeed2 == SFP_25G_SPEED) {
  

•    *Fiber2Type = SFP_10G_SPEED;
  

•  } else {
  

•    *Fiber2Type = SfpSpeed2;
  

•  }
  

• } else {

•  // No fiber, set speed to 10G
  

•  *Fiber2Type = SFP_10G_SPEED;
  

• }
• } else {
• // 100Ge/10Ge/Ge Fiber is not found.
• *Fiber1Type = SFP_10G_SPEED;
• *Fiber2Type = SFP_10G_SPEED;
• *Fiber100Ge = 0x0;
• }
• return EFI_SUCCESS;

-}

-//Fiber1Type/Fiber2Type return: SFP_10G_SPEED, SFP_25G_SPEED, SFP_GE_SPEED -UINT32 -GetCpu1FiberType (

• UINT8* Fiber1Type,
• UINT8* Fiber2Type
• )

-{

• EFI_STATUS Status;
• UINT16 SfpNum1;
• UINT8 SfpSpeed1;
• UINT16 SfpNum2;
• UINT8 SfpSpeed2;
• SfpNum1 = 0x1;
• SfpSpeed1 = SFP_10G_SPEED;
• SfpNum2 = 0x2;
• SfpSpeed2 = SFP_10G_SPEED;
• *Fiber1Type = SFP_10G_SPEED;
• *Fiber2Type = SFP_10G_SPEED;
• // Fiber detected in CPU1 slot0, read speed via i2c
• if (ReadCpldReg (CPU1_SFP0_LOCATE_OFFSET) == FIBER_PRESENT) {
• Status = GetSfpSpeed (SOCKET_0, SfpNum1, &SfpSpeed1);
• if (EFI_ERROR (Status)) {

•  DEBUG ((DEBUG_ERROR, "Get Socket0 Sfp%d Speed Error: %r.\n",
  

•          SfpNum1, Status));
  

•  return Status;
  

• }
• *Fiber1Type = SfpSpeed1;
• } else {
• *Fiber1Type = SFP_10G_SPEED;
• }
• // Fiber detected in CPU1 slot1, read speed via i2c
• if (ReadCpldReg (CPU1_SFP1_LOCATE_OFFSET) == FIBER_PRESENT) {
• Status = GetSfpSpeed (SOCKET_0, SfpNum2, &SfpSpeed2);
• if (EFI_ERROR (Status)) {

•  *Fiber2Type = SFP_10G_SPEED;
  

•  DEBUG ((DEBUG_ERROR, "Get Sfp%d Speed Error: %r.\n", SfpNum2, Status));
  

•  return Status;
  

• }
• *Fiber2Type = SfpSpeed2;
• } else {
• *Fiber2Type = SFP_10G_SPEED;
• }
• return EFI_SUCCESS;

-}

UINT16 MakeCrcCheckSum ( UINT8 *Buffer, UINT32 Length @@ -567,3 +346,42 @@ OemIsInitEth ( { return TRUE; }

+EFI_STATUS ConfigCDR(UINT32 Socket) +{

• return EFI_SUCCESS;

+}

+UINT32 OemGetNclConfOffset (UINT32 Socket) +{

• UINT32 Cpu1NclConfOffet = 0;

Indentation is 2 spaces, not 4. (Please address throughout.)

• UINT32 Cpu2NclConfOffet = 0;

Also, no initialization on definition. But I see no value in having two variables with complicated names. Just a single one called ConfigurationOffset or whatever.

• if (0 == Socket) {

No jeopardy-comparisons. Please flip that around.

•  MmioWrite32 (SRAM_NIC_NCL1_OFFSET_FLAG, Cpu1NclConfOffet);
  

This line can just write a 0 directly. But it can also use a comment explaining what writing a 0 here achieves.

•  return Cpu1NclConfOffet;
  

And this is effectively just an error return - so can just return 0 directly.

• } else {

No need for the else. You've returned is there was an error. The rest is just the remainder of the function.

•  //2P only
  

What is 2P?

•  // P1
  

What is P1?

•  if ((ReadCpldReg (CPU2_SFP2_100G_CARD_OFFSET) & CARD_PRESENT_100G) != 0) {
  

•    Cpu2NclConfOffet =  0x20000;
  

SIZE_128KB?

•  } else {
  

•    Cpu2NclConfOffet =  0x10000;
  

SIZE_64KB?

•  }
  

•  MmioWrite32 (SRAM_NIC_NCL2_OFFSET_FLAG, Cpu2NclConfOffet);
  

•  return Cpu2NclConfOffet;
  

• }

+}

+UINT32 OemGetNetPortNum (UINT32 Socket) +{

• if (0 == Socket){

No jeopardy-comparisons. Please flip that around.

/ Leif

• return SOCKET0_NET_PORT_NUM;
• }
• if ((ReadCpldReg (CPU2_SFP2_100G_CARD_OFFSET) & CARD_PRESENT_100G) != 0) {

•  return SOCKET1_NET_PORT_100G;
  

• } else {

•  return SOCKET1_NET_PORT_NUM;
  

• }

+}

2.9.5