[Linaro-uefi,27/27] D03/D05: Change to access EEPROM data by checking page boundary

Message ID 1478785950-24197-27-git-send-email-heyi.guo@linaro.org
State New
Headers show

Commit Message

gary guo Nov. 10, 2016, 1:52 p.m.
We would get incorrect data when accessing multiple bytes at one time
and going across EEPROM page boundary, so we split data when the length
greater than page boundary.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Peicong Li <lipeicong@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 .../Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c  | 35 ++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

Comments

Leif Lindholm Nov. 14, 2016, 2:59 p.m. | #1
On Thu, Nov 10, 2016 at 09:52:30PM +0800, Heyi Guo wrote:
> We would get incorrect data when accessing multiple bytes at one time
> and going across EEPROM page boundary, so we split data when the length
> greater than page boundary.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Peicong Li <lipeicong@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  .../Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c  | 35 ++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c b/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
> index 994ed6a..eaabb1d 100644
> --- a/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
> +++ b/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
> @@ -26,6 +26,7 @@
>  #include <Library/I2CLib.h>
>  
>  #define EEPROM_I2C_PORT 6
> +#define EEPROM_PAGE_SIZE 0x40
>  
>  EFI_STATUS
>  EFIAPI OemGetMac2P (IN OUT EFI_MAC_ADDRESS *Mac, IN UINTN Port);
> @@ -110,6 +111,8 @@ EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr)
>      UINT16     I2cOffset;
>      UINT16     crc16;
>      NIC_MAC_ADDRESS stMacDesc = {0};
> +    UINT16     RemainderMacOffset;
> +    UINT16     LessSizeOfPage;
>  
>      Status = I2CInit(0, EEPROM_I2C_PORT, Normal);
>      if (EFI_ERROR(Status))
> @@ -124,7 +127,20 @@ EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr)
>      stI2cDev.Port = EEPROM_I2C_PORT;
>      stI2cDev.SlaveDeviceAddress = I2C_SLAVEADDR_EEPROM;
>      stI2cDev.Socket = 0;
> -    Status = I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
> +    RemainderMacOffset = I2cOffset % EEPROM_PAGE_SIZE;
> +    LessSizeOfPage = EEPROM_PAGE_SIZE - RemainderMacOffset;
> +    //The length of NIC_MAC_ADDRESS is 10 bytes long,
> +    //It surly less than EEPROM page size, so we could
> +    //code as bellow, check the address whether across the page boundary,
> +    //and split the data when across page boundary.
> +    if (sizeof(NIC_MAC_ADDRESS) <= LessSizeOfPage) {
> +      Status |= I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
> +    } else {
> +      Status |= I2CRead(&stI2cDev, I2cOffset, LessSizeOfPage, (UINT8 *)&stMacDesc);

No need for |=. Status is always clear if we get here.

> +      if (!(EFI_ERROR(Status))) {
> +      Status |= I2CRead(&stI2cDev, I2cOffset + LessSizeOfPage, sizeof(NIC_MAC_ADDRESS) - LessSizeOfPage, (UINT8 *)&stMacDesc + LessSizeOfPage);
> +      }

Indentation.
Also, would make sense to wrap this long line.

Could write as

    if (sizeof(NIC_MAC_ADDRESS) <= LessSizeOfPage) {
      Status = I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
    } else {
      Status = I2CRead(&stI2cDev, I2cOffset, LessSizeOfPage, (UINT8 *)&stMacDesc);
      Status |= I2CRead(
                        &stI2cDev,
                        I2cOffset + LessSizeOfPage,
                        sizeof(NIC_MAC_ADDRESS) - LessSizeOfPage,
		        (UINT8 *)&stMacDesc + LessSizeOfPage
		       );
    }

Could also keep the "if (!EFI_ERROR" bit, if there is a problem with
performing a second access if the first failed. But there is no point
with the |= then, since we know Status is 0 if the code is executed.

> +    }
>      if (EFI_ERROR(Status))
>      {
>          DEBUG((EFI_D_ERROR, "[%a]:[%dL] Call I2cRead failed! p1=0x%x.\n", __FUNCTION__, __LINE__, Status));
> @@ -153,6 +169,8 @@ EFI_STATUS OemSetMacE2prom(IN UINT32 Port, IN UINT8 *pucAddr)
>  
>  
>      stMacDesc.MacLen = MAC_ADDR_LEN;
> +    UINT16     RemainderMacOffset;
> +    UINT16     LessSizeOfPage;
>      gBS->CopyMem((VOID *)(stMacDesc.Mac), (VOID *)pucAddr, MAC_ADDR_LEN);
>  
>      stMacDesc.Crc16  = make_crc_checksum((UINT8 *)&(stMacDesc.MacLen), sizeof(stMacDesc.MacLen) + MAC_ADDR_LEN);
> @@ -170,7 +188,20 @@ EFI_STATUS OemSetMacE2prom(IN UINT32 Port, IN UINT8 *pucAddr)
>      stI2cDev.Port = EEPROM_I2C_PORT;
>      stI2cDev.SlaveDeviceAddress = I2C_SLAVEADDR_EEPROM;
>      stI2cDev.Socket = 0;
> -    Status = I2CWrite(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
> +    RemainderMacOffset = I2cOffset % EEPROM_PAGE_SIZE;
> +    LessSizeOfPage = EEPROM_PAGE_SIZE - RemainderMacOffset;
> +    //The length of NIC_MAC_ADDRESS is 10 bytes long,
> +    //It surly less than EEPROM page size, so we could
> +    //code as bellow, check the address whether across the page boundary,
> +    //and split the data when across page boundary.
> +    if (sizeof(NIC_MAC_ADDRESS) <= LessSizeOfPage) {
> +      Status |= I2CWrite(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
> +    } else {
> +      Status |= I2CWrite(&stI2cDev, I2cOffset, LessSizeOfPage, (UINT8 *)&stMacDesc);
> +      if (!(EFI_ERROR(Status))) {
> +      Status |= I2CWrite(&stI2cDev, I2cOffset + LessSizeOfPage, sizeof(NIC_MAC_ADDRESS) - LessSizeOfPage, (UINT8 *)&stMacDesc + LessSizeOfPage);


Identical comments as for the Get function.

> +      }
> +    }
>      if (EFI_ERROR(Status))
>      {
>          DEBUG((EFI_D_ERROR, "[%a]:[%dL] Call I2cWrite failed! p1=0x%x.\n", __FUNCTION__, __LINE__, Status));
> -- 
> 1.9.1
>

Patch

diff --git a/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c b/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
index 994ed6a..eaabb1d 100644
--- a/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
+++ b/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c
@@ -26,6 +26,7 @@ 
 #include <Library/I2CLib.h>
 
 #define EEPROM_I2C_PORT 6
+#define EEPROM_PAGE_SIZE 0x40
 
 EFI_STATUS
 EFIAPI OemGetMac2P (IN OUT EFI_MAC_ADDRESS *Mac, IN UINTN Port);
@@ -110,6 +111,8 @@  EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr)
     UINT16     I2cOffset;
     UINT16     crc16;
     NIC_MAC_ADDRESS stMacDesc = {0};
+    UINT16     RemainderMacOffset;
+    UINT16     LessSizeOfPage;
 
     Status = I2CInit(0, EEPROM_I2C_PORT, Normal);
     if (EFI_ERROR(Status))
@@ -124,7 +127,20 @@  EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr)
     stI2cDev.Port = EEPROM_I2C_PORT;
     stI2cDev.SlaveDeviceAddress = I2C_SLAVEADDR_EEPROM;
     stI2cDev.Socket = 0;
-    Status = I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
+    RemainderMacOffset = I2cOffset % EEPROM_PAGE_SIZE;
+    LessSizeOfPage = EEPROM_PAGE_SIZE - RemainderMacOffset;
+    //The length of NIC_MAC_ADDRESS is 10 bytes long,
+    //It surly less than EEPROM page size, so we could
+    //code as bellow, check the address whether across the page boundary,
+    //and split the data when across page boundary.
+    if (sizeof(NIC_MAC_ADDRESS) <= LessSizeOfPage) {
+      Status |= I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
+    } else {
+      Status |= I2CRead(&stI2cDev, I2cOffset, LessSizeOfPage, (UINT8 *)&stMacDesc);
+      if (!(EFI_ERROR(Status))) {
+      Status |= I2CRead(&stI2cDev, I2cOffset + LessSizeOfPage, sizeof(NIC_MAC_ADDRESS) - LessSizeOfPage, (UINT8 *)&stMacDesc + LessSizeOfPage);
+      }
+    }
     if (EFI_ERROR(Status))
     {
         DEBUG((EFI_D_ERROR, "[%a]:[%dL] Call I2cRead failed! p1=0x%x.\n", __FUNCTION__, __LINE__, Status));
@@ -153,6 +169,8 @@  EFI_STATUS OemSetMacE2prom(IN UINT32 Port, IN UINT8 *pucAddr)
 
 
     stMacDesc.MacLen = MAC_ADDR_LEN;
+    UINT16     RemainderMacOffset;
+    UINT16     LessSizeOfPage;
     gBS->CopyMem((VOID *)(stMacDesc.Mac), (VOID *)pucAddr, MAC_ADDR_LEN);
 
     stMacDesc.Crc16  = make_crc_checksum((UINT8 *)&(stMacDesc.MacLen), sizeof(stMacDesc.MacLen) + MAC_ADDR_LEN);
@@ -170,7 +188,20 @@  EFI_STATUS OemSetMacE2prom(IN UINT32 Port, IN UINT8 *pucAddr)
     stI2cDev.Port = EEPROM_I2C_PORT;
     stI2cDev.SlaveDeviceAddress = I2C_SLAVEADDR_EEPROM;
     stI2cDev.Socket = 0;
-    Status = I2CWrite(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
+    RemainderMacOffset = I2cOffset % EEPROM_PAGE_SIZE;
+    LessSizeOfPage = EEPROM_PAGE_SIZE - RemainderMacOffset;
+    //The length of NIC_MAC_ADDRESS is 10 bytes long,
+    //It surly less than EEPROM page size, so we could
+    //code as bellow, check the address whether across the page boundary,
+    //and split the data when across page boundary.
+    if (sizeof(NIC_MAC_ADDRESS) <= LessSizeOfPage) {
+      Status |= I2CWrite(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc);
+    } else {
+      Status |= I2CWrite(&stI2cDev, I2cOffset, LessSizeOfPage, (UINT8 *)&stMacDesc);
+      if (!(EFI_ERROR(Status))) {
+      Status |= I2CWrite(&stI2cDev, I2cOffset + LessSizeOfPage, sizeof(NIC_MAC_ADDRESS) - LessSizeOfPage, (UINT8 *)&stMacDesc + LessSizeOfPage);
+      }
+    }
     if (EFI_ERROR(Status))
     {
         DEBUG((EFI_D_ERROR, "[%a]:[%dL] Call I2cWrite failed! p1=0x%x.\n", __FUNCTION__, __LINE__, Status));