Re: [PATCH] hw/riscv: microchip_pfsoc: specify XIP image
On Tue, Jan 12, 2021 at 4:11 AM Vitaly Wool wrote:
>
> Hi Bin,
>
> On Tue, Jan 5, 2021 at 7:27 AM Bin Meng wrote:
> >
> > +Alistair Francis
> >
> > On Sat, Dec 19, 2020 at 8:24 AM Vitaly Wool
> > 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= ...
> > >
> > > Signed-off-by: Vitaly Wool
> > > ---
> > > 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 :)
In general I would say let's avoid machine properties. The more
aligned we can be to general QEMU arguments the better. That way
someone swapping from some other platform shouldn't have too much
trouble getting started.
Ideally we want to use either the device loader or the `-drive`
argument for loading these images. The RISC-V virt machine has -drive
support, maybe that is what you want here instead?
Alistair
>
> ~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,
> > > + 0x1 /* 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
>
Re: [PATCH] hw/riscv: microchip_pfsoc: specify XIP image
Hi Bin,
On Tue, Jan 5, 2021 at 7:27 AM Bin Meng wrote:
>
> +Alistair Francis
>
> On Sat, Dec 19, 2020 at 8:24 AM Vitaly Wool 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= ...
> >
> > Signed-off-by: Vitaly Wool
> > ---
> > 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,
> > + 0x1 /* 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
Re: [PATCH] hw/riscv: microchip_pfsoc: specify XIP image
+Alistair Francis
On Sat, Dec 19, 2020 at 8:24 AM Vitaly Wool 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= ...
>
> Signed-off-by: Vitaly Wool
> ---
> 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.
> - "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,
> + 0x1 /* 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
[PATCH] hw/riscv: microchip_pfsoc: specify XIP image
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= ...
Signed-off-by: Vitaly Wool
---
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),
- "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,
+ 0x1 /* 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)
}
}
+static void microchip_pfsoc_prop_set_xipimage(Object *obj,
+ const char *value,
+ Error **errp)
+{
+MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(obj);
+
+g_free(s->xip_image);
+s->xip_image = g_strdup(value);
+}
+
+static char *microchip_pfsoc_prop_get_xipimage(Object *obj,
+ Error **errp)
+{
+MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(obj);
+
+return g_strdup(s->xip_image);
+}
+
static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void
*data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -536,6 +564,12 @@ static void
microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
* See memory_tests() in mss_ddr.c in the HSS source code.
*/
mc->default_ram_size = 1537 * MiB;
+
+object_class_property_add_str(oc, "xipImage",
+microchip_pfsoc_prop_get_xipimage,
+microchip_pfsoc_prop_set_xipimage);
+object_class_property_set_description(oc, "xipImage",
+"Kernel XIP image to run from QSPI NOR");
}
static const TypeInfo microchip_icicle_kit_machine_typeinfo = {
diff --git a/include/hw/riscv/microchip_pfsoc.h
b/include/hw/riscv/microchip_pfsoc.h
index d0c666aae0..a7b180a5d4 100644
--- a/include/hw/riscv/microchip_pfsoc.h
+++ b/include/hw/riscv/microchip_pfsoc.h
@@ -65,6 +65,7 @@ typedef struct MicrochipIcicleKitState {
/*< public >*/
MicrochipPFSoCState soc;
+char *xip_image;
} MicrochipIcicleKitState;
#define TYPE_MICROCHIP_ICICLE_KIT_MACHINE \
--
2.20.1
