From: Leif Lindholm <leif.lindholm@linaro.org>
To: Ming Huang <ming.huang@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 15:46:39 +0000 [thread overview]
Message-ID: <20190212154639.55w3ozpve5w3kmr7@bivouac.eciton.net> (raw)
In-Reply-To: <892c3608-33d0-5b15-9d1c-6e8824df4e5d@linaro.org>
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"?
> >
> > 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?
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?
/
Leif
> >
> > 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:46 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
2019-02-12 15:46 ` Leif Lindholm [this message]
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=20190212154639.55w3ozpve5w3kmr7@bivouac.eciton.net \
--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