Re: [platforms PATCH 23/25] Marvell/Drivers: MvBoardDesc: Extend protocol with I2C support

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

On Fri, Jun 08, 2018 at 05:34:21PM +0200, Marcin Wojtas wrote:
> Introduce new callback that can provide information
> about I2C controllers to the I2c driver.
> 
> Extend ArmadaBoardDescLib with new structure MV_BOARD_I2C_DESC,
> for holding board specific data. In further steps it should
> be extended and replace PCD I2C devices' representation with the
> appropriate structures.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> Reviewed-by: Hua Jing <jinghua@marvell.com>
> ---
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c   | 62 ++++++++++++++++++++
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf |  1 +
>  Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h | 11 ++++
>  Silicon/Marvell/Include/Protocol/BoardDesc.h         |  8 +++
>  4 files changed, 82 insertions(+)
> 
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> index 8f3bdfa..a133085 100644
> --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> @@ -96,6 +96,67 @@ MvBoardDescComPhyGet (
>  
>  STATIC
>  EFI_STATUS
> +MvBoardDescI2cGet (
> +  IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> +  IN OUT MV_BOARD_I2C_DESC       **I2cDesc
> +  )
> +{
> +  UINT8 *I2cDeviceTable, I2cCount;

UINTN.

Having gone through this set, this pattern is becoming clearer to me.
But ideally, it would have been obvious from the first instance.

Should not all of these "Tables" be treated simply as enable flags?
Because that's all they seem to be doing.

If you rename this one
  I2cDeviceEnabled
...

> +  UINTN I2cDeviceTableSize, I2cIndex, Index;
> +  MV_BOARD_I2C_DESC *BoardDesc;
> +  MV_SOC_I2C_DESC *SoCDesc;
> +  EFI_STATUS Status;
> +
> +  /* Get SoC data about all available I2C controllers */
> +  Status = ArmadaSoCDescI2cGet (&SoCDesc, &I2cCount);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  /* Obtain table with enabled I2C controllers */
> +  I2cDeviceTable = (UINT8 *)PcdGetPtr (PcdI2cControllersEnabled);
> +  if (I2cDeviceTable == NULL) {
> +    /* No I2C on platform */
> +    return EFI_SUCCESS;
> +  }
> +
> +  I2cDeviceTableSize = PcdGetSize (PcdI2cControllersEnabled);
> +
> +  /* Check if PCD with I2C controllers is correctly defined */
> +  if (I2cDeviceTableSize > I2cCount) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Wrong PcdI2cControllersEnabled format\n",
> +      __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  /* Allocate and fill board description */
> +  BoardDesc = AllocateZeroPool (I2cDeviceTableSize * sizeof (MV_BOARD_I2C_DESC));
> +  if (BoardDesc == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  I2cIndex = 0;
> +  for (Index = 0; Index < I2cDeviceTableSize; Index++) {
> +    if (!MVHW_DEV_ENABLED (I2c, Index)) {

... then this one becomes simply
    if (!I2cDeviceEnabled[Index]) {

Please consider making this change throughout the set.

> +      DEBUG ((DEBUG_INFO, "%a: Skip I2c controller %d\n", __FUNCTION__, Index));
> +      continue;
> +    }
> +
> +    BoardDesc[I2cIndex].SoC = &SoCDesc[Index];
> +    I2cIndex++;
> +  }
> +
> +  BoardDesc->I2cDevCount = I2cIndex;
> +
> +  *I2cDesc = BoardDesc;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
>  MvBoardDescMdioGet (
>    IN MARVELL_BOARD_DESC_PROTOCOL  *This,
>    IN OUT MV_BOARD_MDIO_DESC      **MdioDesc
> @@ -470,6 +531,7 @@ MvBoardDescInitProtocol (
>    )
>  {
>    BoardDescProtocol->BoardDescComPhyGet = MvBoardDescComPhyGet;
> +  BoardDescProtocol->BoardDescI2cGet = MvBoardDescI2cGet;
>    BoardDescProtocol->BoardDescMdioGet = MvBoardDescMdioGet;
>    BoardDescProtocol->BoardDescAhciGet = MvBoardDescAhciGet;
>    BoardDescProtocol->BoardDescSdMmcGet = MvBoardDescSdMmcGet;
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> index 71b7ebd..cc93eba 100644
> --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> @@ -58,6 +58,7 @@
>  
>  [Pcd]
>    gMarvellTokenSpaceGuid.PcdComPhyDevices
> +  gMarvellTokenSpaceGuid.PcdI2cControllersEnabled

Alphabetically ...

>    gMarvellTokenSpaceGuid.PcdPp2Controllers
>    gMarvellTokenSpaceGuid.PcdUtmiControllersEnabled
>    gMarvellTokenSpaceGuid.PcdUtmiPortType
> diff --git a/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h b/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> index 5379679..74361d4 100644
> --- a/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> +++ b/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> @@ -28,6 +28,17 @@ typedef struct {
>  } MV_BOARD_COMPHY_DESC;
>  
>  //
> +// I2C devices per-board description
> +//
> +// TODO - Extend structure with entire
> +// ports description instead of PCDs.

Please drop TODO.

> +//
> +typedef struct {
> +  MV_SOC_I2C_DESC *SoC;
> +  UINT8            I2cDevCount;

UINTN.

(To clarify: this is because the pointer above already forces struct
alignment to the same as UINTN. The only point at which any space
could be saved would be if something with less alignment than UINTN
was placed immediately after it, and even then it would increase code
size for accessing it.)

/
    Leif

> +} MV_BOARD_I2C_DESC;
> +
> +//
>  // MDIO devices per-board description
>  //
>  typedef struct {
> diff --git a/Silicon/Marvell/Include/Protocol/BoardDesc.h b/Silicon/Marvell/Include/Protocol/BoardDesc.h
> index cff802a..0b73d27 100644
> --- a/Silicon/Marvell/Include/Protocol/BoardDesc.h
> +++ b/Silicon/Marvell/Include/Protocol/BoardDesc.h
> @@ -50,6 +50,13 @@ EFI_STATUS
>  
>  typedef
>  EFI_STATUS
> +(EFIAPI *MV_BOARD_DESC_I2C_GET) (
> +  IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> +  IN OUT MV_BOARD_I2C_DESC       **I2cDesc
> +  );
> +
> +typedef
> +EFI_STATUS
>  (EFIAPI *MV_BOARD_DESC_MDIO_GET) (
>    IN MARVELL_BOARD_DESC_PROTOCOL  *This,
>    IN OUT MV_BOARD_MDIO_DESC      **MdioDesc
> @@ -98,6 +105,7 @@ VOID
>  
>  struct _MARVELL_BOARD_DESC_PROTOCOL {
>    MV_BOARD_DESC_COMPHY_GET       BoardDescComPhyGet;
> +  MV_BOARD_DESC_I2C_GET          BoardDescI2cGet;
>    MV_BOARD_DESC_MDIO_GET         BoardDescMdioGet;
>    MV_BOARD_DESC_AHCI_GET         BoardDescAhciGet;
>    MV_BOARD_DESC_SDMMC_GET        BoardDescSdMmcGet;
> -- 
> 2.7.4
>