| Message ID | 1463698459-31312-4-git-send-email-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > The way we currently model the RPU subsystem is of quite > limited use. In addition to that, it causes problems for > KVM and for GDB debugging. > > Make the RPU optional by adding a has_rpu property and > default to having it disabled. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > hw/arm/xlnx-zynqmp.c | 50 +++++++++++++++++++++++++++++++++++++------- > include/hw/arm/xlnx-zynqmp.h | 2 ++ > 2 files changed, 44 insertions(+), 8 deletions(-) > > diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c > index 250ecc4..c180206 100644 > --- a/hw/arm/xlnx-zynqmp.c > +++ b/hw/arm/xlnx-zynqmp.c > @@ -83,6 +83,40 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index) > return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index; > } > > +static bool xlnx_zynqmp_get_has_rpu(Object *obj, Error **errp) > +{ > + XlnxZynqMPState *s = XLNX_ZYNQMP(obj); > + > + return s->has_rpu; > +} > + > +static void xlnx_zynqmp_set_has_rpu(Object *obj, bool value, Error **errp) > +{ > + XlnxZynqMPState *s = XLNX_ZYNQMP(obj); > + int i; > + > + if (s->has_rpu == value) { > + /* Nothing to do. */ > + return; > + } > + > + /* We don't support clearing the flag. */ > + if (s->has_rpu) { > + error_setg(errp, "has_rpu is already set to %u", > + s->has_rpu); > + return; > + } > + > + /* Create the Cortex R5s. */ > + for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) { > + object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]), > + "cortex-r5-" TYPE_ARM_CPU); > + object_property_add_child(obj, "rpu-cpu[*]", OBJECT(&s->rpu_cpu[i]), > + &error_abort); > + } Do we have to create them in the set function so we can set properties before realize, or could this be deferred to realize time? thanks -- PMM
On Tue, May 24, 2016 at 05:26:54PM +0100, Peter Maydell wrote: > On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > The way we currently model the RPU subsystem is of quite > > limited use. In addition to that, it causes problems for > > KVM and for GDB debugging. > > > > Make the RPU optional by adding a has_rpu property and > > default to having it disabled. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > hw/arm/xlnx-zynqmp.c | 50 +++++++++++++++++++++++++++++++++++++------- > > include/hw/arm/xlnx-zynqmp.h | 2 ++ > > 2 files changed, 44 insertions(+), 8 deletions(-) > > > > diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c > > index 250ecc4..c180206 100644 > > --- a/hw/arm/xlnx-zynqmp.c > > +++ b/hw/arm/xlnx-zynqmp.c > > @@ -83,6 +83,40 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index) > > return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index; > > } > > > > +static bool xlnx_zynqmp_get_has_rpu(Object *obj, Error **errp) > > +{ > > + XlnxZynqMPState *s = XLNX_ZYNQMP(obj); > > + > > + return s->has_rpu; > > +} > > + > > +static void xlnx_zynqmp_set_has_rpu(Object *obj, bool value, Error **errp) > > +{ > > + XlnxZynqMPState *s = XLNX_ZYNQMP(obj); > > + int i; > > + > > + if (s->has_rpu == value) { > > + /* Nothing to do. */ > > + return; > > + } > > + > > + /* We don't support clearing the flag. */ > > + if (s->has_rpu) { > > + error_setg(errp, "has_rpu is already set to %u", > > + s->has_rpu); > > + return; > > + } > > + > > + /* Create the Cortex R5s. */ > > + for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) { > > + object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]), > > + "cortex-r5-" TYPE_ARM_CPU); > > + object_property_add_child(obj, "rpu-cpu[*]", OBJECT(&s->rpu_cpu[i]), > > + &error_abort); > > + } > > Do we have to create them in the set function so we can > set properties before realize, or could this be deferred > to realize time? Yes, I thought it was recommended to avoid object creation in realize. But creating the PRU in realize works too. Cheers, Edgar
On 24 May 2016 at 17:32, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Tue, May 24, 2016 at 05:26:54PM +0100, Peter Maydell wrote: >> Do we have to create them in the set function so we can >> set properties before realize, or could this be deferred >> to realize time? > > Yes, I thought it was recommended to avoid object creation > in realize. But creating the PRU in realize works too. Well, it is recommended, but only in the sense of "prefer to do it in instance init" :-) I think we prefer not to have complicated behaviours happening on property-set because it's a bit unexpected (but sometimes it's necessary if later property-sets wouldn't work otherwise). thanks -- PMM
On Tue, May 24, 2016 at 05:37:14PM +0100, Peter Maydell wrote: > On 24 May 2016 at 17:32, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Tue, May 24, 2016 at 05:26:54PM +0100, Peter Maydell wrote: > >> Do we have to create them in the set function so we can > >> set properties before realize, or could this be deferred > >> to realize time? > > > > Yes, I thought it was recommended to avoid object creation > > in realize. But creating the PRU in realize works too. > > Well, it is recommended, but only in the sense of "prefer to > do it in instance init" :-) I think we prefer not to have > complicated behaviours happening on property-set because it's > a bit unexpected (but sometimes it's necessary if later > property-sets wouldn't work otherwise). Allright, I'm happy to change it. It definitely simplifies the code. Cheers, Edgar
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index 250ecc4..c180206 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -83,6 +83,40 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index) return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index; } +static bool xlnx_zynqmp_get_has_rpu(Object *obj, Error **errp) +{ + XlnxZynqMPState *s = XLNX_ZYNQMP(obj); + + return s->has_rpu; +} + +static void xlnx_zynqmp_set_has_rpu(Object *obj, bool value, Error **errp) +{ + XlnxZynqMPState *s = XLNX_ZYNQMP(obj); + int i; + + if (s->has_rpu == value) { + /* Nothing to do. */ + return; + } + + /* We don't support clearing the flag. */ + if (s->has_rpu) { + error_setg(errp, "has_rpu is already set to %u", + s->has_rpu); + return; + } + + /* Create the Cortex R5s. */ + for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) { + object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]), + "cortex-r5-" TYPE_ARM_CPU); + object_property_add_child(obj, "rpu-cpu[*]", OBJECT(&s->rpu_cpu[i]), + &error_abort); + } + s->has_rpu = value; +} + static void xlnx_zynqmp_setup_rpu(XlnxZynqMPState *s, const char *boot_cpu, Error **errp) { @@ -118,6 +152,11 @@ static void xlnx_zynqmp_init(Object *obj) XlnxZynqMPState *s = XLNX_ZYNQMP(obj); int i; + object_property_add_bool(obj, "has_rpu", + xlnx_zynqmp_get_has_rpu, + xlnx_zynqmp_set_has_rpu, + &error_abort); + for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) { object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]), "cortex-a53-" TYPE_ARM_CPU); @@ -125,13 +164,6 @@ static void xlnx_zynqmp_init(Object *obj) &error_abort); } - for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) { - object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]), - "cortex-r5-" TYPE_ARM_CPU); - object_property_add_child(obj, "rpu-cpu[*]", OBJECT(&s->rpu_cpu[i]), - &error_abort); - } - object_property_add_link(obj, "ddr-ram", TYPE_MEMORY_REGION, (Object **)&s->ddr_ram, qdev_prop_allow_set_link_before_realize, @@ -290,7 +322,9 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) qdev_connect_gpio_out(DEVICE(&s->apu_cpu[i]), 1, irq); } - xlnx_zynqmp_setup_rpu(s, boot_cpu, errp); + if (s->has_rpu) { + xlnx_zynqmp_setup_rpu(s, boot_cpu, errp); + } if (!s->boot_cpu_ptr) { error_setg(errp, "ZynqMP Boot cpu %s not found", boot_cpu); diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h index 38d4c8c..68f6eb0 100644 --- a/include/hw/arm/xlnx-zynqmp.h +++ b/include/hw/arm/xlnx-zynqmp.h @@ -87,6 +87,8 @@ typedef struct XlnxZynqMPState { /* Has the ARM Security extensions? */ bool secure; + /* Has the RPU subsystem? */ + bool has_rpu; } XlnxZynqMPState; #define XLNX_ZYNQMP_H