On 2/12/2019 11:46 PM, Leif Lindholm wrote:
> On Tue, Feb 12, 2019 at 11:14:43PM +0800, Ming Huang wrote:
>>
>>
>> On 2/12/2019 3:28 AM, Leif Lindholm wrote:
>>> 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.
>>
>> M7 is used for HNS(Hisilicon network system), cpu access the network
>> component via M7.
> 
> Sure. But does customer documentation documentation refer to it as
> "M7"?

I check documentation just now, Integrated Management Processor(IMP) is used,
so, I will change commit titil and message M7 to IMP.

> 
>>>
>>> What are the external dependencies?
>>> Is this addressed by one of the patches for edk2-non-osi?
>>
>> This is depend on M7 firmware which will include in hpm file.
> 
> So we don't get it when using Capsule Update?

Yes.

> 
> What would be the implication of installing system firmware with the
> below change on a system that had not had the corresponding M7
> firmware update?

The HNS will not worked.

Thanks

> 
> /
>     Leif
> 
>>>
>>> More style issues below.
>>>
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Ming Huang <ming.hu...@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.
>>
>> Yes, M7 firmware will read this two sram addresses.
>>
>>>
>>>> +
>>>>  #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?
>>
>> 2 processors, or 2 sockets.
>>
>>>
>>>> +      // P1
>>>
>>> What is P1?
>>
>> The second processor.
>>
>>>
>>>> +      if ((ReadCpldReg (CPU2_SFP2_100G_CARD_OFFSET) & CARD_PRESENT_100G) 
>>>> != 0) {
>>>> +        Cpu2NclConfOffet =  0x20000;
>>>
>>> SIZE_128KB?
>>
>> ok
>>
>>>
>>>> +      } else {
>>>> +        Cpu2NclConfOffet =  0x10000;
>>>
>>> SIZE_64KB?
>>
>> ok
>>
>>>
>>>> +      }
>>>> +      MmioWrite32 (SRAM_NIC_NCL2_OFFSET_FLAG, Cpu2NclConfOffet);
>>>> +      return Cpu2NclConfOffet;
>>>> +    }
>>>> +}
>>>> +
>>>> +UINT32 OemGetNetPortNum (UINT32 Socket)
>>>> +{
>>>> +    if (0 == Socket){
>>>
>>> No jeopardy-comparisons. Please flip that around.
>>
>> All comments will be addressed.
>>
>> Thanks
>>
>>>
>>> /
>>>     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
>>>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to