Re: [PATCH 3/3] hw/arm/mps2-tz: Implement AN524 memory remapping via machine property

[Philippe Mathieu-Daudé]

On 4/12/21 3:43 PM, Peter Maydell wrote:
> The AN524 FPGA image supports two memory maps, which differ
> in where the QSPI and BRAM are. In the default map, the BRAM
> is at 0x0000_0000, and the QSPI at 0x2800_0000. In the second
> map, they are the other way around.
> 
> In hardware, the initial mapping can be selected by the user
> by writing either "REMAP: BRAM" (the default) or "REMAP: QSPI"
> in the board configuration file. The guest can also dynamically
> change the mapping via the SCC CFG_REG0 register.
> 
> Implement this functionality for QEMU, using a machine property
> "remap" with valid values "BRAM" and "QSPI" to allow the user to set
> the initial mapping, in the same way they can on the FPGA, and
> wiring up the bit from the SCC register to also switch the mapping.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  docs/system/arm/mps2.rst |  10 ++++
>  hw/arm/mps2-tz.c         | 106 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/system/arm/mps2.rst b/docs/system/arm/mps2.rst
> index f83b1517871..8a75beb3a08 100644
> --- a/docs/system/arm/mps2.rst
> +++ b/docs/system/arm/mps2.rst
> @@ -45,3 +45,13 @@ Differences between QEMU and real hardware:
>    flash, but only as simple ROM, so attempting to rewrite the flash
>    from the guest will fail
>  - QEMU does not model the USB controller in MPS3 boards
> +
> +Machine-specific options
> +""""""""""""""""""""""""
> +
> +The following machine-specific options are supported:
> +
> +remap
> +  Supported for ``mps3-an524`` only.
> +  Set ``BRAM``/``QSPI`` to select the initial memory mapping. The
> +  default is ``BRAM``.
> diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
> index 25016e464d9..e9806779326 100644
> --- a/hw/arm/mps2-tz.c
> +++ b/hw/arm/mps2-tz.c
> @@ -55,6 +55,7 @@
>  #include "hw/boards.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/reset.h"
>  #include "hw/misc/unimp.h"
>  #include "hw/char/cmsdk-apb-uart.h"
>  #include "hw/timer/cmsdk-apb-timer.h"
> @@ -72,6 +73,7 @@
>  #include "hw/core/split-irq.h"
>  #include "hw/qdev-clock.h"
>  #include "qom/object.h"
> +#include "hw/irq.h"
>  
>  #define MPS2TZ_NUMIRQ_MAX 96
>  #define MPS2TZ_RAM_MAX 5
> @@ -153,6 +155,9 @@ struct MPS2TZMachineState {
>      SplitIRQ cpu_irq_splitter[MPS2TZ_NUMIRQ_MAX];
>      Clock *sysclk;
>      Clock *s32kclk;
> +
> +    int remap;

Maybe bool, ...

> +    qemu_irq remap_irq;
>  };
>  
>  #define TYPE_MPS2TZ_MACHINE "mps2tz"
> @@ -228,6 +233,10 @@ static const RAMInfo an505_raminfo[] = { {
>      },
>  };
>  
> +/*
> + * Note that the addresses and MPC numbering here should match up
> + * with those used in remap_memory(), which can swap the BRAM and QSPI.
> + */
>  static const RAMInfo an524_raminfo[] = { {
>          .name = "bram",
>          .base = 0x00000000,
> @@ -457,6 +466,7 @@ static MemoryRegion *make_scc(MPS2TZMachineState *mms, 
> void *opaque,
>  
>      object_initialize_child(OBJECT(mms), "scc", scc, TYPE_MPS2_SCC);
>      sccdev = DEVICE(scc);
> +    qdev_prop_set_uint32(sccdev, "scc-cfg0", mms->remap);

... and here:

       qdev_prop_set_uint32(sccdev, "scc-cfg0", mms->remap ? 1 : 0);

as remap is a bit and scc-cfg0 could have other bits set in the future.

>      qdev_prop_set_uint32(sccdev, "scc-cfg4", 0x2);
>      qdev_prop_set_uint32(sccdev, "scc-aid", 0x00200008);
>      qdev_prop_set_uint32(sccdev, "scc-id", mmc->scc_id);
> @@ -573,6 +583,50 @@ static MemoryRegion *make_mpc(MPS2TZMachineState *mms, 
> void *opaque,
>      return sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 0);
>  }
>  
> +static hwaddr boot_mem_base(MPS2TZMachineState *mms)
> +{
> +    /*
> +     * Return the canonical address of the block which will be mapped
> +     * at address 0x0 (i.e. where the vector table is).
> +     * This is usually 0, but if the AN524 alternate memory map is
> +     * enabled it will be the base address of the QSPI block.
> +     */
> +    return mms->remap ? 0x28000000 : 0;
> +}
> +
> +static void remap_memory(MPS2TZMachineState *mms, int map)
> +{
> +    /*
> +     * Remap the memory for the AN524. 'map' is the value of
> +     * SCC CFG_REG0 bit 0, i.e. 0 for the default map and 1
> +     * for the "option 1" mapping where QSPI is at address 0.
> +     *
> +     * Effectively we need to swap around the "upstream" ends of
> +     * MPC 0 and MPC 1.
> +     */
> +    MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
> +    int i;
> +
> +    if (mmc->fpga_type != FPGA_AN524) {
> +        return;
> +    }
> +

This is done in the machine_reset() handler and during QDev realization,
so probably not important, but since it is reachable by an IRQ handler,
maybe it is better (thinking about code copy/pasting) to wrap this with:

       memory_region_transaction_begin();

> +    for (i = 0; i < 2; i++) {
> +        TZMPC *mpc = &mms->mpc[i];
> +        MemoryRegion *upstream = sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 
> 1);
> +        hwaddr addr = (i ^ map) ? 0x28000000 : 0;
> +
> +        memory_region_set_address(upstream, addr);
> +    }

       memory_region_transaction_commit();

> +}
> +
> +static void remap_irq_fn(void *opaque, int n, int level)
> +{
> +    MPS2TZMachineState *mms = opaque;
> +
> +    remap_memory(mms, level);
> +}

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>