Re: [edk2-platforms: PATCH v2 01/10] Marvell/Armada7k8k: Fix 32-bit compilation

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

On Thu, Aug 15, 2019 at 04:54:05AM +0200, Marcin Wojtas wrote:
> It turned out, that the recently added features broke
> ARM compilation. Fix all issues:
> * Use SIGNATURE_32 only
> * Do not shift address by 32-bit in ICU
> * Limit memory for ARM build to 1GB and stop using non-existent PCD
> 
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h                       |  2 +-
>  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h                       |  2 +-
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c      |  4 ++++
>  Silicon/Marvell/Library/IcuLib/IcuLib.c                                  |  7 ++++++-
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/ARM/ArmPlatformHelper.S | 11 -----------
>  5 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h
> index a6f551b..7ecb4e1 100644
> --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h
> @@ -18,7 +18,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  #include <Uefi/UefiBaseType.h>
>  
> -#define MV_BOARD_DESC_SIGNATURE SIGNATURE_64 ('M', 'V', 'B', 'R', 'D', 'D', 'S', 'C')
> +#define MV_BOARD_DESC_SIGNATURE SIGNATURE_32 ('B', 'D', 'S', 'C')
>  
>  typedef struct {
>    MARVELL_BOARD_DESC_PROTOCOL   BoardDescProtocol;
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> index 1cb006a..600d5db 100644
> --- a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> @@ -23,7 +23,7 @@
>  
>  #include <Uefi/UefiBaseType.h>
>  
> -#define MV_GPIO_SIGNATURE        SIGNATURE_64 ('M', 'V','_','G', 'P', 'I','O',' ')
> +#define MV_GPIO_SIGNATURE        SIGNATURE_32 ('G', 'P', 'I', 'O')

Can you epxlain why you opted to change the size of the signature
instead of the signature field in the structs?

I'm not super happy about this, since we still have UINTN signature,
not UINT32. Since the signature is *not* architecture dependent, it
should have a fixed size.

>  
>  // Marvell MV_GPIO Controller Registers
>  #define MV_GPIO_DATA_OUT_REG     (0x0)
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> index a735fe5..5ff6bb1 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> @@ -36,6 +36,7 @@ GetDramSize (
>    IN OUT UINT64 *MemSize
>    )
>  {
> +#if defined(MDE_CPU_AARCH64)
>    ARM_SMC_ARGS SmcRegs = {0};
>    EFI_STATUS Status;
>  
> @@ -48,6 +49,9 @@ GetDramSize (
>    ArmCallSmc (&SmcRegs);
>  
>    *MemSize = SmcRegs.Arg0;
> +#else
> +  *MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> +#endif

Can you add a comment explaining why a 32-bit SMC call is not
possible?

>  
>    return EFI_SUCCESS;
>  }
> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.c b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> index 343c21b..422388c 100644
> --- a/Silicon/Marvell/Library/IcuLib/IcuLib.c
> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> @@ -185,10 +185,15 @@ IcuConfigure (
>    for (Index = 0; Index < IcuGroupMax; Index++, Msi++) {
>      MmioWrite32 (IcuBase + ICU_SET_SPI_AL (Msi->Group),
>        Msi->SetSpiAddr & 0xFFFFFFFF);
> -    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), Msi->SetSpiAddr >> 32);
>      MmioWrite32 (IcuBase + ICU_CLR_SPI_AL (Msi->Group),
>        Msi->ClrSpiAddr & 0xFFFFFFFF);
> +#if defined(MDE_CPU_AARCH64)
> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), Msi->SetSpiAddr >> 32);
>      MmioWrite32 (IcuBase + ICU_CLR_SPI_AH (Msi->Group), Msi->ClrSpiAddr >> 32);
> +#else
> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), 0);
> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AH (Msi->Group), 0);
> +#endif

I don't understand what's going on here - add a comment?

Best Regards,

Leif

>    }
>  
>    /* Mask all ICU interrupts */
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/ARM/ArmPlatformHelper.S b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/ARM/ArmPlatformHelper.S
> index 4416163..db43b0f 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/ARM/ArmPlatformHelper.S
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/ARM/ArmPlatformHelper.S
> @@ -28,17 +28,6 @@ ASM_FUNC(ArmPlatformPeiBootAction)
>    .err  PcdSystemMemoryBase should be 0x0 on this platform!
>    .endif
>  
> -  .if   FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
> -    //
> -    // Use the low range for UEFI itself. The remaining memory will be mapped
> -    // and added to the GCD map later.
> -    //
> -    ADRL  (r0, mSystemMemoryEnd)
> -    MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> -    mov   r3, #0
> -    strd  r2, r3, [r0]
> -  .endif
> -
>    bx    lr
>  
>  //UINTN
> -- 
> 2.7.4
>