Re: [PATCH] hw/riscv: microchip_pfsoc: specify XIP image

[Vitaly Wool]

Hi Bin,

On Tue, Jan 5, 2021 at 7:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> +Alistair Francis
>
> On Sat, Dec 19, 2020 at 8:24 AM Vitaly Wool <vitaly.wool@konsulko.com> wrote:
> >
> > Add command line parameter to microchip_pfsoc machine to be able
> > to specify XIP kernel image file. To pass over XIP image file, it
> > will be enough to run
> >
> > $ qemu-system-riscv64 -M microchip-icicle-kit,xipImage=<image> ...
> >
> > Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.com>
> > ---
> >  hw/riscv/microchip_pfsoc.c         | 42 +++++++++++++++++++++++++++---
> >  include/hw/riscv/microchip_pfsoc.h |  1 +
> >  2 files changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > index e952b49e8c..04d81d52fe 100644
> > --- a/hw/riscv/microchip_pfsoc.c
> > +++ b/hw/riscv/microchip_pfsoc.c
> > @@ -181,6 +181,7 @@ static void microchip_pfsoc_soc_instance_init(Object 
> > *obj)
> >  static void microchip_pfsoc_soc_realize(DeviceState *dev, Error **errp)
> >  {
> >      MachineState *ms = MACHINE(qdev_get_machine());
> > +    MicrochipIcicleKitState *mks = MICROCHIP_ICICLE_KIT_MACHINE(ms);
> >      MicrochipPFSoCState *s = MICROCHIP_PFSOC(dev);
> >      const struct MemmapEntry *memmap = microchip_pfsoc_memmap;
> >      MemoryRegion *system_memory = get_system_memory();
> > @@ -415,10 +416,19 @@ static void microchip_pfsoc_soc_realize(DeviceState 
> > *dev, Error **errp)
> >                      memmap[MICROCHIP_PFSOC_IOSCB].base);
> >
> >      /* QSPI Flash */
> > -    memory_region_init_rom(qspi_xip_mem, OBJECT(dev),
>
> I believe we only need to change this to memory_region_init_ram(),
> then use the device loader the load the XIP image into this space. The
> remove the need to introduce the XIP image property as you did.

I think it should be possible to use the device loader with rom as
well, and in this case we can skip this patch altogether. However, my
idea was that specifying explicitly it's not just a random data being
loaded but a kernel image in the XIP format would make things clearer.
OTOH, I would then introduce a property that isn't strictly speaking
necessary. I'll let Alistair decide what is the best way to go :)

~Vitaly

>
> > -                           "microchip.pfsoc.qspi_xip",
> > -                           memmap[MICROCHIP_PFSOC_QSPI_XIP].size,
> > -                           &error_fatal);
> > +    if (mks->xip_image) {
> > +        memory_region_init_ram_from_file(qspi_xip_mem, OBJECT(dev),
> > +                                         "microchip.pfsoc.qspi_xip",
> > +                                         
> > memmap[MICROCHIP_PFSOC_QSPI_XIP].size,
> > +                                         0x10000 /* align */, 0 /* 
> > ram_flags */,
> > +                                         mks->xip_image, &error_fatal);
> > +        qspi_xip_mem->readonly = true;
> > +    } else {
> > +        memory_region_init_rom(qspi_xip_mem, OBJECT(dev),
> > +                               "microchip.pfsoc.qspi_xip",
> > +                               memmap[MICROCHIP_PFSOC_QSPI_XIP].size,
> > +                               &error_fatal);
> > +    }
> >      memory_region_add_subregion(system_memory,
> >                                  memmap[MICROCHIP_PFSOC_QSPI_XIP].base,
> >                                  qspi_xip_mem);
> > @@ -517,6 +527,24 @@ static void 
> > microchip_icicle_kit_machine_init(MachineState *machine)
> >      }
> >  }
> >
>
> Regards,
> Bin