Re: [RESEND PATCH 7/9] hw/riscv: microchip_pfsoc: Map debug memory

[Alistair Francis]

On Tue, Oct 27, 2020 at 7:53 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> Somehow HSS needs to access address 0 [1] for the DDR calibration data
> which is in the chipset's debug memory. Let's map the debug memory.
>
> [1] See the config_copy() calls in various places in ddr_setup() in
>     the HSS source codes.

Really? This is reserved memory that they just read and write to? That's crazy.

If we really need this can you add a comment saying that the
documentation is wrong (again) and that this needs to be here.

>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
>  hw/riscv/microchip_pfsoc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 69117c6000..b9c2f73e7c 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -158,6 +158,7 @@ static void microchip_pfsoc_soc_realize(DeviceState *dev, 
> Error **errp)
>      MicrochipPFSoCState *s = MICROCHIP_PFSOC(dev);
>      const struct MemmapEntry *memmap = microchip_pfsoc_memmap;
>      MemoryRegion *system_memory = get_system_memory();
> +    MemoryRegion *debug_mem = g_new(MemoryRegion, 1);
>      MemoryRegion *e51_dtim_mem = g_new(MemoryRegion, 1);
>      MemoryRegion *l2lim_mem = g_new(MemoryRegion, 1);
>      MemoryRegion *envm_data = g_new(MemoryRegion, 1);
> @@ -177,6 +178,13 @@ static void microchip_pfsoc_soc_realize(DeviceState 
> *dev, Error **errp)
>      qdev_realize(DEVICE(&s->e_cluster), NULL, &error_abort);
>      qdev_realize(DEVICE(&s->u_cluster), NULL, &error_abort);
>
> +    /* Debug */

This doesn't seem right.

Debug should start at 0x0000_0100 (it seems like what is currently in
tree is wrong).

Address 0x00 is marked as reserved in the documentation.

Do you mind updating the memory map to fix the debug address? Then
just add a reserved region as well.

Alistair

> +    memory_region_init_ram(debug_mem, NULL, "microchip.pfsoc.debug_mem",
> +                           memmap[MICROCHIP_PFSOC_DEBUG].size, &error_fatal);
> +    memory_region_add_subregion(system_memory,
> +                                memmap[MICROCHIP_PFSOC_DEBUG].base,
> +                                debug_mem);
> +
>      /* E51 DTIM */
>      memory_region_init_ram(e51_dtim_mem, NULL, 
> "microchip.pfsoc.e51_dtim_mem",
>                             memmap[MICROCHIP_PFSOC_E51_DTIM].size, 
> &error_fatal);
> --
> 2.25.1
>
>