From: Ming Huang <ming.huang@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: linaro-uefi@lists.linaro.org, edk2-devel@lists.01.org,
graeme.gregory@linaro.org, ard.biesheuvel@linaro.org,
michael.d.kinney@intel.com, lersek@redhat.com,
wanghuiqiang@huawei.com, huangming23@huawei.com,
zhangjinsong2@huawei.com, huangdaode@hisilicon.com,
john.garry@huawei.com, xinliang.liu@linaro.org,
zhangfeng56@huawei.com
Subject: Re: [PATCH edk2-platforms v1 10/16] Hisilicon/D06: Modify for M7 self-Adapte support
Date: Tue, 12 Feb 2019 23:14:43 +0800 [thread overview]
Message-ID: <892c3608-33d0-5b15-9d1c-6e8824df4e5d@linaro.org> (raw)
In-Reply-To: <20190211192834.x4h6q2o2wgh4sq56@bivouac.eciton.net>
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.
>
> 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.
>
> 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.
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
>>
next prev parent reply other threads:[~2019-02-12 15:14 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-01 13:34 [PATCH edk2-platforms v1 00/16] Fix issues and improve D0x Ming Huang
2019-02-01 13:34 ` [PATCH edk2-platforms v1 01/16] Hisilicon/D0x: Remove SerdesLib Ming Huang
2019-02-11 15:05 ` Leif Lindholm
2019-02-13 6:36 ` Ming Huang
2019-02-13 9:42 ` Leif Lindholm
2019-02-13 11:39 ` Ming Huang
2019-02-14 10:51 ` Ming Huang
2019-02-01 13:34 ` [PATCH edk2-platforms v1 02/16] Hisilicon/D0x: Add DriverHealthManagerDxe Ming Huang
2019-02-11 15:20 ` Leif Lindholm
2019-02-01 13:34 ` [PATCH edk2-platforms v1 03/16] Hisilicon/D06: Optimize SAS driver for reducing boot time Ming Huang
2019-02-12 15:12 ` Leif Lindholm
2019-02-13 6:01 ` Ming Huang
2019-02-01 13:34 ` [PATCH edk2-platforms v1 04/16] Hisilicon/D06: Fix access variable fail issue Ming Huang
2019-02-12 15:17 ` Leif Lindholm
2019-02-13 2:21 ` Ming Huang
2019-02-01 13:34 ` [PATCH edk2-platforms v1 05/16] Hisilicon/D06: Add more PCIe port INT-x support Ming Huang
2019-02-11 17:05 ` Leif Lindholm
2019-02-12 12:27 ` Ming Huang
2019-02-01 13:34 ` [PATCH edk2-platforms v1 06/16] Hisilicon/D06: Add OemGetCpuFreq to encapsulate difference Ming Huang
2019-02-11 17:15 ` Leif Lindholm
2019-02-13 2:29 ` Ming Huang
2019-02-01 13:34 ` [PATCH edk2-platforms v1 07/16] Hisilicon/D0x: Rename StartupAp() function Ming Huang
2019-02-11 18:04 ` Leif Lindholm
2019-02-01 13:34 ` [PATCH edk2-platforms v1 08/16] Hisilicon/D06: Change HCCS speed from 30G to 26G Ming Huang
2019-02-11 18:36 ` Leif Lindholm
2019-02-12 14:45 ` Ming Huang
2019-02-12 14:59 ` Leif Lindholm
2019-02-01 13:34 ` [PATCH edk2-platforms v1 09/16] Hisilicon/D06: Add PCI_OSC_SUPPORT Ming Huang
2019-02-11 18:51 ` Leif Lindholm
2019-02-13 2:59 ` Ming Huang
2019-02-13 9:08 ` Leif Lindholm
2019-02-01 13:34 ` [PATCH edk2-platforms v1 10/16] Hisilicon/D06: Modify for M7 self-Adapte support Ming Huang
2019-02-11 19:28 ` Leif Lindholm
2019-02-12 15:14 ` Ming Huang [this message]
2019-02-12 15:46 ` Leif Lindholm
2019-02-13 4:38 ` Ming Huang
2019-02-01 13:34 ` [PATCH edk2-platforms v1 11/16] Hisilicon/D06: Add Setup Item "Support DPC" Ming Huang
2019-02-11 19:46 ` Leif Lindholm
2019-02-12 15:22 ` Ming Huang
2019-02-12 15:49 ` Leif Lindholm
2019-02-01 13:34 ` [PATCH edk2-platforms v1 12/16] Hisilicon/D06: Use new flash layout Ming Huang
2019-02-11 14:54 ` Leif Lindholm
2019-02-13 4:43 ` Ming Huang
2019-02-01 13:34 ` [PATCH edk2-platforms v1 13/16] Hisilicon/D06: Remove SECURE_BOOT_ENABLE definition Ming Huang
2019-02-11 19:47 ` Leif Lindholm
2019-02-01 13:34 ` [PATCH edk2-platforms v1 14/16] Hisilicon/D0x: Remove SP805 watchdog pcd Ming Huang
2019-02-11 19:48 ` Leif Lindholm
2019-02-15 14:18 ` [PATCH edk2-platforms v1 00/16] Fix issues and improve D0x Ming Huang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=892c3608-33d0-5b15-9d1c-6e8824df4e5d@linaro.org \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox