Re: [platforms: PATCH 01/12] Marvell/Library: ArmadaSoCDescLib: Add GPIO information

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

On Sat, Oct 20, 2018 at 03:57:30AM +0200, Marcin Wojtas wrote:
> This patch introduces new library callback (ArmadaSoCDescGpioGet ()),
> which dynamically allocates and fills MV_SOC_GPIO_DESC structure with
> the SoC description of GPIO controllers.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h | 10 +++++
>  Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 15 ++++++++
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 39 ++++++++++++++++++++
>  3 files changed, 64 insertions(+)
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> index c14b985..85dd67c 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> @@ -22,6 +22,7 @@
>  // Common macros
>  //
>  #define MV_SOC_CP_BASE(Cp)               (0xF2000000 + ((Cp) * 0x2000000))
> +#define MV_SOC_AP_COUNT                  1

I think all of my comments on this patch can be summarised as "what is
an AP in this context"?

The term either needs explicit documenting, or expansion in the macro
names such that documentation is not required.

/
    Leif

>  
>  //
>  // Platform description of AHCI controllers
> @@ -38,6 +39,15 @@
>  #define MV_SOC_COMPHY_MUX_BITS           4
>  
>  //
> +// Platform description of GPIO controllers
> +//
> +#define MV_SOC_AP_GPIO_BASE              0xF06F5040
> +#define MV_SOC_AP_GPIO_PIN_COUNT         20
> +#define MV_SOC_GPIO_PER_CP_COUNT         2
> +#define MV_SOC_CP_GPIO_BASE(Gpio)        (0x440100 + ((Gpio) * 0x40))
> +#define MV_SOC_CP_GPIO_PIN_COUNT(Gpio)   ((Gpio) == 0 ? 32 : 31)
> +
> +//
>  // Platform description of I2C controllers
>  //
>  #define MV_SOC_I2C_PER_CP_COUNT          2
> diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> index cdfb51b..f3d4f80 100644
> --- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> +++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> @@ -46,6 +46,21 @@ ArmadaSoCDescCpBaseGet (
>    );
>  
>  //
> +// GPIO devices description template definition
> +//
> +typedef struct {
> +  UINTN GpioBaseAddress;
> +  UINTN GpioPinCount;
> +} MV_SOC_GPIO_DESC;
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaSoCDescGpioGet (
> +  IN OUT MV_SOC_GPIO_DESC  **GpioDesc,
> +  IN OUT UINTN              *DescCount
> +  );
> +
> +//
>  // I2C
>  //
>  typedef struct {
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> index 6902fda..7db4ec7 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> @@ -74,6 +74,45 @@ ArmadaSoCDescCpBaseGet (
>  
>  EFI_STATUS
>  EFIAPI
> +ArmadaSoCDescGpioGet (
> +  IN OUT MV_SOC_GPIO_DESC  **GpioDesc,
> +  IN OUT UINTN             *DescCount
> +  )
> +{
> +  MV_SOC_GPIO_DESC *Desc;
> +  UINTN CpCount, CpIndex, Index;
> +
> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> +
> +  *DescCount = CpCount * MV_SOC_GPIO_PER_CP_COUNT + MV_SOC_AP_COUNT;
> +  Desc = AllocateZeroPool (*DescCount * sizeof (MV_SOC_GPIO_DESC));
> +  if (Desc == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  *GpioDesc = Desc;
> +
> +  /* AP GPIO controller */
> +  Desc->GpioBaseAddress = MV_SOC_AP_GPIO_BASE;
> +  Desc->GpioPinCount = MV_SOC_AP_GPIO_PIN_COUNT;
> +  Desc++;
> +
> +  /* CP GPIO controllers */
> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> +    for (Index = 0; Index < MV_SOC_GPIO_PER_CP_COUNT; Index++) {
> +      Desc->GpioBaseAddress = MV_SOC_CP_BASE (CpIndex) +
> +                              MV_SOC_CP_GPIO_BASE (Index);
> +      Desc->GpioPinCount = MV_SOC_CP_GPIO_PIN_COUNT (Index);
> +      Desc++;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
>  ArmadaSoCDescI2cGet (
>    IN OUT MV_SOC_I2C_DESC  **I2cDesc,
>    IN OUT UINTN             *DescCount
> -- 
> 2.7.4
>