Re: [platforms: PATCH 2/5] Marvell/Drivers: MvI2cDxe: Move devices description to MvHwDescLib

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Oct 06, 2017 at 09:51:15AM +0200, Marcin Wojtas wrote:
> This patch introduces I2c description, using the new structures
> and template in MvHwDescLib. This change enables more flexible
> addition of multiple I2c controllers and also allows for
> removal of string PCD parsing. Update Armada 70x0 DB description
> and PortingGuide accordingly.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Armada/Armada70x0.dsc             |  2 +-
>  Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c   | 42 +++++++++++---------
>  Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf |  3 +-
>  Platform/Marvell/Include/Library/MvHwDescLib.h     | 25 ++++++++++++
>  Platform/Marvell/Marvell.dec                       |  2 +-
>  Silicon/Marvell/Documentation/PortingGuide.txt     |  4 +-
>  6 files changed, 54 insertions(+), 24 deletions(-)
> 
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 7b03744..d9d126d 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -78,7 +78,7 @@
>    # I2C
>    gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses|{ 0x50, 0x57, 0x60 }
>    gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0, 0x0, 0x0 }
> -  gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|L"0xF2701000;0xF2701100"
> +  gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x1, 0x1 }

Similarly to the previous patch, the above is a bit opaque.
I guess in this case, this is simply a boolean array saying whether a
specific controller is enabled or not?
If my interpretation is correct, could the Pcd be renamed
PcdI2cControllersEnabled?

/
    Leif

>    gMarvellTokenSpaceGuid.PcdEepromI2cAddresses|{ 0x50, 0x57 }
>    gMarvellTokenSpaceGuid.PcdEepromI2cBuses|{ 0x0, 0x0 }
>    gMarvellTokenSpaceGuid.PcdI2cClockFrequency|250000000
> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> index fa79ebc..ff8006e 100755
> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> @@ -42,12 +42,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #include <Library/DebugLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/UefiLib.h>
> -#include <Library/ParsePcdLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Library/MvHwDescLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  
>  #include "MvI2cDxe.h"
>  
> +DECLARE_A7K8K_I2C_TEMPLATE;
> +
>  STATIC MV_I2C_BAUD_RATE baud_rate;
>  
>  STATIC MV_I2C_DEVICE_PATH MvI2cDevicePathProtocol = {
> @@ -172,35 +174,39 @@ MvI2cInitialise (
>    IN EFI_SYSTEM_TABLE  *SystemTable
>    )
>  {
> +  MVHW_I2C_DESC *Desc = &mA7k8kI2cDescTemplate;
> +  UINT8 *I2cDeviceTable, Index;
>    EFI_STATUS Status;
> -  UINT32 BusCount;
> -  EFI_PHYSICAL_ADDRESS I2cBaseAddresses[PcdGet32 (PcdI2cBusCount)];
> -  INTN i;
>  
> -  BusCount = PcdGet32 (PcdI2cBusCount);
> -  if (BusCount == 0)
> -    return EFI_SUCCESS;
> +  /* Obtain table with enabled I2c devices */
> +  I2cDeviceTable = (UINT8 *)PcdGetPtr (PcdI2cControllers);
> +  if (I2cDeviceTable == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Missing PcdI2cControllers\n"));
> +    return EFI_INVALID_PARAMETER;
> +  }
>  
> -  Status = ParsePcdString (
> -      (CHAR16 *) PcdGetPtr (PcdI2cBaseAddresses),
> -      BusCount,
> -      I2cBaseAddresses,
> -      NULL
> -      );
> -  if (EFI_ERROR(Status))
> -    return Status;
> +  if (PcdGetSize (PcdI2cControllers) > MVHW_MAX_I2C_DEVS) {
> +    DEBUG ((DEBUG_ERROR, "Wrong PcdI2cControllers format\n"));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  /* Initialize enabled chips */
> +  for (Index = 0; Index < PcdGetSize (PcdI2cControllers); Index++) {
> +    if (!MVHW_DEV_ENABLED (I2c, Index)) {
> +      DEBUG ((DEBUG_ERROR, "Skip I2c chip %d\n", Index));
> +      continue;
> +    }
>  
> -  for (i = 0; i < BusCount; i++) {
>      Status = MvI2cInitialiseController(
>          ImageHandle,
>          SystemTable,
> -        I2cBaseAddresses[i]
> +        Desc->I2cBaseAddresses[Index]
>          );
>      if (EFI_ERROR(Status))
>        return Status;
>    }
>  
> -  return Status;
> +  return EFI_SUCCESS;
>  }
>  
>  STATIC
> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
> index 16374ef..c453b4f 100755
> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
> @@ -55,7 +55,6 @@
>    UefiLib
>    UefiDriverEntryPoint
>    UefiBootServicesTableLib
> -  ParsePcdLib
>  
>  [Protocols]
>    gEfiI2cMasterProtocolGuid
> @@ -66,7 +65,7 @@
>  [Pcd]
>    gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses
>    gMarvellTokenSpaceGuid.PcdI2cSlaveBuses
> -  gMarvellTokenSpaceGuid.PcdI2cBaseAddresses
> +  gMarvellTokenSpaceGuid.PcdI2cControllers
>    gMarvellTokenSpaceGuid.PcdI2cClockFrequency
>    gMarvellTokenSpaceGuid.PcdI2cBaudRate
>    gMarvellTokenSpaceGuid.PcdI2cBusCount
> diff --git a/Platform/Marvell/Include/Library/MvHwDescLib.h b/Platform/Marvell/Include/Library/MvHwDescLib.h
> index 6a86865..e029b50 100644
> --- a/Platform/Marvell/Include/Library/MvHwDescLib.h
> +++ b/Platform/Marvell/Include/Library/MvHwDescLib.h
> @@ -60,6 +60,16 @@ typedef struct {
>  } MVHW_COMPHY_DESC;
>  
>  //
> +// I2C devices description template definition
> +//
> +#define MVHW_MAX_I2C_DEVS         4
> +
> +typedef struct {
> +  UINT8 I2cDevCount;
> +  UINTN I2cBaseAddresses[MVHW_MAX_I2C_DEVS];
> +} MVHW_I2C_DESC;
> +
> +//
>  // NonDiscoverable devices description template definition
>  //
>  #define MVHW_MAX_XHCI_DEVS         4
> @@ -130,6 +140,21 @@ MVHW_COMPHY_DESC mA7k8kComPhyDescTemplate = {\
>  }
>  
>  //
> +// Platform description of I2C devices
> +//
> +#define MVHW_CP0_I2C0_BASE       0xF2701000
> +#define MVHW_CP0_I2C1_BASE       0xF2701100
> +#define MVHW_CP1_I2C0_BASE       0xF4701000
> +#define MVHW_CP1_I2C1_BASE       0xF4701100
> +
> +#define DECLARE_A7K8K_I2C_TEMPLATE \
> +STATIC \
> +MVHW_I2C_DESC mA7k8kI2cDescTemplate = {\
> +  4,\
> +  { MVHW_CP0_I2C0_BASE, MVHW_CP0_I2C1_BASE, MVHW_CP1_I2C0_BASE, MVHW_CP1_I2C1_BASE }\
> +}
> +
> +//
>  // Platform description of NonDiscoverable devices
>  //
>  #define MVHW_CP0_XHCI0_BASE        0xF2500000
> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> index fc00f1a..25a4258 100644
> --- a/Platform/Marvell/Marvell.dec
> +++ b/Platform/Marvell/Marvell.dec
> @@ -113,7 +113,7 @@
>    gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0 }|VOID*|0x3000184
>    gMarvellTokenSpaceGuid.PcdEepromI2cAddresses|{ 0x0 }|VOID*|0x3000050
>    gMarvellTokenSpaceGuid.PcdEepromI2cBuses|{ 0x0 }|VOID*|0x3000185
> -  gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|{ 0x0 }|VOID*|0x3000047
> +  gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x0 }|VOID*|0x3000047
>    gMarvellTokenSpaceGuid.PcdI2cClockFrequency|0|UINT32|0x3000048
>    gMarvellTokenSpaceGuid.PcdI2cBaudRate|0|UINT32|0x3000049
>    gMarvellTokenSpaceGuid.PcdI2cBusCount|0|UINT32|0x3000183
> diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
> index 47a667d..1a30c46 100644
> --- a/Silicon/Marvell/Documentation/PortingGuide.txt
> +++ b/Silicon/Marvell/Documentation/PortingGuide.txt
> @@ -188,8 +188,8 @@ In order to enable driver on a new platform, following steps need to be taken:
>  		(buses to which accoring slaves are attached)
>  	- gMarvellTokenSpaceGuid.PcdI2cBusCount|2
>  		(number of SoC's I2C buses)
> -	- gMarvellTokenSpaceGuid.PcdI2cBaseAddresses|L"0xF2701000;0xF2701100"
> -		(base addresses of I2C controller buses)
> +	- gMarvellTokenSpaceGuid.PcdI2cControllers|{ 0x1, 0x1 }
> +		(array with used controllers)
>  	- gMarvellTokenSpaceGuid.PcdI2cClockFrequency|200000000
>  		(I2C host controller clock frequency)
>  	- gMarvellTokenSpaceGuid.PcdI2cBaudRate|100000
> -- 
> 1.8.3.1
>