Re: [platforms: PATCH 3/3] Marvell/Armada7k8k: Read DRAM settings from ARM-TF

[Marcin Wojtas <mw@semihalf.com>]

Hi Leif,

pon., 21 sty 2019 o 12:51 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Mon, Jan 21, 2019 at 11:52:11AM +0100, Marcin Wojtas wrote:
> > From: Grzegorz Jaszczyk <jaz@semihalf.com>
> >
> > The memory controller registers are marked as secure in the latest
> > ARM-TF for Armada SoCs. It is available however get the DRAM
> > information via SiP services in the EL3, so use it instead
> > of accessing the registers directly.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf  |  3 ++
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h | 27 ++--------
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c | 55 +++++---------------
> >  3 files changed, 21 insertions(+), 64 deletions(-)
> >
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
> > index e888566..0c7f320 100644
> > --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
> > @@ -41,12 +41,15 @@
> >  [Packages]
> >    ArmPkg/ArmPkg.dec
> >    ArmPlatformPkg/ArmPlatformPkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> >    MdeModulePkg/MdeModulePkg.dec
> >    MdePkg/MdePkg.dec
> >    Silicon/Marvell/Marvell.dec
> >
> >  [LibraryClasses]
> > +  ArmadaSoCDescLib
> >    ArmLib
> > +  ArmSmcLib
> >    DebugLib
> >    MemoryAllocationLib
> >    MppLib
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> > index cc30e4a..8a46df6 100644
> > --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> > @@ -47,27 +47,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >  #define DRAM_REMAP_TARGET \
> >            (MmioRead32 (CCU_MC_RTBR_REG) << TARGET_BASE_OFFS)
> >
> > -#define DRAM_CH0_MMAP_LOW_REG(cs)       (0xf0020200 + (cs) * 0x8)
> > -#define DRAM_CS_VALID_ENABLED_MASK      0x1
> > -#define DRAM_AREA_LENGTH_OFFS           16
> > -#define DRAM_AREA_LENGTH_MASK           (0x1f << DRAM_AREA_LENGTH_OFFS)
> > -#define DRAM_START_ADDRESS_L_OFFS       23
> > -#define DRAM_START_ADDRESS_L_MASK       (0x1ff << DRAM_START_ADDRESS_L_OFFS)
> > -#define DRAM_CH0_MMAP_HIGH_REG(cs)      (0xf0020204 + (cs) * 0x8)
> > -#define DRAM_START_ADDR_HTOL_OFFS       32
> > +/* Armada7k8k North Bridge index */
> > +#define ARMADA7K8K_AP806_INDEX          0
> >
> > -#define DRAM_MAX_CS_NUM                 8
> > -
> > -#define DRAM_CS_ENABLED(Cs) \
> > -          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)) & DRAM_CS_VALID_ENABLED_MASK)
> > -#define GET_DRAM_REGION_BASE(Cs) \
> > -          ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG ((Cs))) << \
> > -           DRAM_START_ADDR_HTOL_OFFS) | \
> > -          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)) & DRAM_START_ADDRESS_L_MASK);
> > -#define GET_DRAM_REGION_SIZE_CODE(Cs) \
> > -          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG ((Cs))) & \
> > -           DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS
> > -#define DRAM_REGION_SIZE_EVEN(C)        (((C) >= 7) && ((C) <= 26))
> > -#define GET_DRAM_REGION_SIZE_EVEN(C)    ((UINT64)1 << ((C) + 16))
> > -#define DRAM_REGION_SIZE_ODD(C)         ((C) <= 4)
> > -#define GET_DRAM_REGION_SIZE_ODD(C)     ((UINT64)0x18000000 << (C))
> > +/* Firmware related definition used for SMC calls */
> > +#define MV_SIP_DRAM_SIZE                0x82000010
>
> Hmm...
> This would end us up with having Marvell SMC calls spread across
> multiple files. Could you insert an intermediate patch which breaks out
> the ones from Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h into a
> separate (MarvellSmc.h?) include file?

How about MvSmc.h ? I try to use 'Mv' prefix consistently, especially
when adding new code.

>
> If you could further add _SMC_ID to the #defines (like in edk2
> ArmPkg/Include/IndustryStandard/ArmStdSmc.h), that would make me very
> happy. (I'd be happy for you to drop _SIP, or keep it either side of
> the addition, as per your preference - we don't seem to have precedent
> here yet.)
>

How about below:
#define MV_SMC_ID_COMPHY_POWER_ON       0x82000001
#define MV_SMC_ID_COMPHY_POWER_OFF     0x82000002
#define MV_SMC_ID_COMPHY_PLL_LOCK          0x82000003
?

Thanks,
Marcin


> Regards,
>
> Leif
>
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > index 2a4f5ad..62e8467 100644
> > --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > @@ -33,11 +33,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >  *******************************************************************************/
> >
> >  #include <Base.h>
> > +#include <IndustryStandard/ArmStdSmc.h>
> >  #include <Library/ArmPlatformLib.h>
> > +#include <Library/ArmSmcLib.h>
> >  #include <Library/DebugLib.h>
> >  #include <Library/HobLib.h>
> >  #include <Library/IoLib.h>
> >  #include <Library/MemoryAllocationLib.h>
> > +#include <Protocol/BoardDesc.h>
> >
> >  #include "Armada7k8kLibMem.h"
> >
> > @@ -57,49 +60,19 @@ GetDramSize (
> >    IN OUT UINT64 *MemSize
> >    )
> >  {
> > -  UINT64 BaseAddr;
> > -  UINT8 RegionCode;
> > -  UINT8 Cs;
> > -
> > -  *MemSize = 0;
> > -
> > -  for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
> > -
> > -    /* Exit loop on first disabled DRAM CS */
> > -    if (!DRAM_CS_ENABLED (Cs)) {
> > -      break;
> > -    }
> > -
> > -    /*
> > -     * Sanity check for base address of next DRAM block.
> > -     * Only continuous space will be used.
> > -     */
> > -    BaseAddr = GET_DRAM_REGION_BASE (Cs);
> > -    if (BaseAddr != *MemSize) {
> > -      DEBUG ((DEBUG_ERROR,
> > -        "%a: DRAM blocks are not contiguous, limit size to 0x%llx\n",
> > -        __FUNCTION__,
> > -        *MemSize));
> > -      return EFI_SUCCESS;
> > -    }
> > -
> > -    /* Decode area length for current CS from register value */
> > -    RegionCode = GET_DRAM_REGION_SIZE_CODE (Cs);
> > -
> > -    if (DRAM_REGION_SIZE_EVEN (RegionCode)) {
> > -      *MemSize += GET_DRAM_REGION_SIZE_EVEN (RegionCode);
> > -    } else if (DRAM_REGION_SIZE_ODD (RegionCode)) {
> > -      *MemSize += GET_DRAM_REGION_SIZE_ODD (RegionCode);
> > -    } else {
> > -      DEBUG ((DEBUG_ERROR,
> > -        "%a: Invalid memory region code (0x%x) for CS#%d\n",
> > -        __FUNCTION__,
> > -        RegionCode,
> > -        Cs));
> > -      return EFI_INVALID_PARAMETER;
> > -    }
> > +  ARM_SMC_ARGS SmcRegs = {0};
> > +  EFI_STATUS Status;
> > +
> > +  SmcRegs.Arg0 = MV_SIP_DRAM_SIZE;
> > +  Status = ArmadaSoCAp8xxBaseGet (&SmcRegs.Arg1, ARMADA7K8K_AP806_INDEX);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> >    }
> >
> > +  ArmCallSmc (&SmcRegs);
> > +
> > +  *MemSize = SmcRegs.Arg0;
> > +
> >    return EFI_SUCCESS;
> >  }
> >
> > --
> > 2.7.4
> >