From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::344; helo=mail-wm1-x344.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x344.google.com (mail-wm1-x344.google.com [IPv6:2a00:1450:4864:20::344]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8B80E208D6137 for ; Mon, 11 Feb 2019 11:28:39 -0800 (PST) Received: by mail-wm1-x344.google.com with SMTP id f16so422165wmh.4 for ; Mon, 11 Feb 2019 11:28:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=qWGIDyN3pXo3jcQNL+GKmVTc/wBXfwBCKsPu4iI2vSM=; b=xdvQbCaCJKw5BzTaTqFhQeiFpDm/gVkjc0bVemkAQUJy5LYgzMLrkRclsNs97/j5SG /rPR6VGADuaKgtoQPfkPRb8/n+CUnUE/msqX5Oaq9E40260rlKg4FNnqstwt4Iw1V6jI Nf8ElwrTD9gxZlZ10Hc8GD3TR4eiPD22D43ifs/B39xphUIXrGU8i4wJD1QQ612Sh4bf haKRrpIYlK4JMV5+oSDJgDR5VCq92EwVKqlS+hoBVtjQRhC+yfH/Xtx0iB3EmfRJxNH7 IOPMaclg0H+24VtkLyUIjrQCljOAhS/m/G1XBWr/e2XtyavS38O9zkNK6tId9J61QRYX qMEQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=qWGIDyN3pXo3jcQNL+GKmVTc/wBXfwBCKsPu4iI2vSM=; b=mmppQnlnjQpC08+uKl3lj8mtWh9qZmRPLKDmRNdWoSafGlDDwMkjztI300hNw8Lq7W THcHAJJktlE2viGwy5/xPpNKzbtzLQWu+ExMu4PnM+HS6Uitruz0N53PwL7LHNnx/kAY N5SE6MBhvSBsUNlMKJ0ukMKgHi5RTi/YRQv3jDskmcZ3XsEpDIUiHtUrler6MPvRbhdV E7s5E6MudsSre2P1yCbD3a/ZI8jhnfSLnx164mzKeRNinvEF/lX8BmgBLlt7T0KtazAh QvPXHeOsGY2+xsLq/U4HwtoE6q2TfTEuEEca2dVxOo0q24g/lPXlVE/qYetQL7AfDMq+ iFdA== X-Gm-Message-State: AHQUAuZRhN0q9wkR3V55EsOKkdmSHcxsFb4r09ceL308bjsZ55oSOW9i YIf8XhBCIGguylEL4gQPUcBKoA== X-Google-Smtp-Source: AHgI3IYP/FLrqLyditshp63W9vcHfFZse7ENsCHf2IdbOpLRW/H08+3GMhfp6yNaD49zPvUFbV8xdQ== X-Received: by 2002:a1c:a885:: with SMTP id r127mr873819wme.74.1549913317567; Mon, 11 Feb 2019 11:28:37 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id z26sm118707wml.44.2019.02.11.11.28.35 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 11 Feb 2019 11:28:36 -0800 (PST) Date: Mon, 11 Feb 2019 19:28:34 +0000 From: Leif Lindholm To: Ming Huang 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 Message-ID: <20190211192834.x4h6q2o2wgh4sq56@bivouac.eciton.net> References: <20190201133436.10500-1-ming.huang@linaro.org> <20190201133436.10500-11-ming.huang@linaro.org> MIME-Version: 1.0 In-Reply-To: <20190201133436.10500-11-ming.huang@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms v1 10/16] Hisilicon/D06: Modify for M7 self-Adapte support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Feb 2019 19:28:40 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 > > #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 >