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

[Marcin Wojtas <mw@semihalf.com>]

Hi Leif,

śr., 14 lis 2018 o 02:10 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> 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.

Isn't a glossary on top of this file:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/8ff9b13675a401588d3cc999aef9891838047b18/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h#L13
and C file:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/8ff9b13675a401588d3cc999aef9891838047b18/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c#L13
sufficient?

Thanks,
Marcin

>
> >
> >  //
> >  // 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
> >