From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::642; helo=mail-pl1-x642.google.com; envelope-from=ming.huang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl1-x642.google.com (mail-pl1-x642.google.com [IPv6:2607:f8b0:4864:20::642]) (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 58FBA202E53B6 for ; Tue, 12 Feb 2019 20:38:36 -0800 (PST) Received: by mail-pl1-x642.google.com with SMTP id s1so561472plp.9 for ; Tue, 12 Feb 2019 20:38:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=u49DqR1KAc5EuyxL3eNtBWXt22t8mTI28B8tILtjLGM=; b=TUuSOp5kuFatk7CsOtQfNl4+SDFkSfnX4BSUvex9dxjh2juXiqnMTU+CoLV4jONXlc fgk4GYZFh5QTE6EpcnemQDiygJPHLobSbNHbyGqRrylY8Txnbc6icL8OWDNdjgLTcsos UbrqSAxyppZBkzmsYgrX8n/5L5gOLMFqQ26HFuvQ15L4mMq0+WDYXxdeIytgMijthS0C o/T0g1s8BB4g6+r5NlbyTocRpn/5lAYdrIDh5PkMaTCZnrUqdUu42PSXx48qh3xmCWQH uIjwRNAIyqXImF6tw7NzLF16h+nBMz117okVFiKQ00Nfsn26AutR4fk8mZssYR0LhlJ/ 5pOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=u49DqR1KAc5EuyxL3eNtBWXt22t8mTI28B8tILtjLGM=; b=CFDIV+uv6tZKQ+i4sXY5mdqy8XuKHNMv74mV4XF7fugnyoKVmSkkfL/lLqNwEQ1TnD WmlGY+aoPRUzwdWigZAdzuDGXH9+2nAOn1FWhHdJOlKu2piI8EzwWT1X2+NFbW3p8O/K Uub3JXbgRbokpm8qOAD9XXVj/IN8RrSt7bYBgDHGD1O/bHxd2uv/rasA5btkOeiPssZ1 157McDDWxstx9n90bvDiDXrlNqW3kjMeC2Tz7atmAhJqgfy6cxQpySn5MiFVVAzPp+VA lCCpGaRUmg7UoMB61LbjETyIMVACQk3m89T4vAddYYgR3i4ZsD/bLc/h6Fy789CkEX25 9mig== X-Gm-Message-State: AHQUAuby7i8RQBsnZ8I9t2uEwnp82KVxNk7l0/59MBg3x4CZqCJxJ8mP h9Yn8RKME7XCdupnoz2HP+4qNQ== X-Google-Smtp-Source: AHgI3IbUla8foeBo6h7091TdoOHZ5Ngik30JWMggx20m7hCPmaYEhRh2TQp1J0hX9DaA8lcZHG7XEQ== X-Received: by 2002:a17:902:bcc2:: with SMTP id o2mr7481325pls.69.1550032715641; Tue, 12 Feb 2019 20:38:35 -0800 (PST) Received: from [10.58.0.74] ([64.64.108.254]) by smtp.gmail.com with ESMTPSA id g20sm27102351pfg.85.2019.02.12.20.38.27 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 12 Feb 2019 20:38:34 -0800 (PST) To: Leif Lindholm 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 References: <20190201133436.10500-1-ming.huang@linaro.org> <20190201133436.10500-11-ming.huang@linaro.org> <20190211192834.x4h6q2o2wgh4sq56@bivouac.eciton.net> <892c3608-33d0-5b15-9d1c-6e8824df4e5d@linaro.org> <20190212154639.55w3ozpve5w3kmr7@bivouac.eciton.net> From: Ming Huang Message-ID: Date: Wed, 13 Feb 2019 12:38:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20190212154639.55w3ozpve5w3kmr7@bivouac.eciton.net> 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: Wed, 13 Feb 2019 04:38:36 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >>>> --- >>>> 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. >> >> 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 >>>>