Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/ppc/ppc405.h | 16 +++++++++++
hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
2 files changed, 64 insertions(+), 23 deletions(-)
diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
index 1da34a7f10f3..1c7fe07b8084 100644
--- a/hw/ppc/ppc405.h
+++ b/hw/ppc/ppc405.h
@@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
typedef struct Ppc405SoCState Ppc405SoCState;
+/* Peripheral controller */
+#define TYPE_PPC405_EBC "ppc405-ebc"
+OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
+struct Ppc405EbcState {
+ DeviceState parent_obj;
+
+ PowerPCCPU *cpu;
+ uint32_t addr;
+ uint32_t bcr[8];
+ uint32_t bap[8];
+ uint32_t bear;
+ uint32_t besr0;
+ uint32_t besr1;
+ uint32_t cfg;
+};
/* DMA controller */
#define TYPE_PPC405_DMA "ppc405-dma"
@@ -203,6 +218,7 @@ struct Ppc405SoCState {
Ppc405OcmState ocm;
Ppc405GpioState gpio;
Ppc405DmaState dma;
+ Ppc405EbcState ebc;
};
/* PowerPC 405 core */
diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
index 6bd93c1cb90c..0166f3fc36da 100644
--- a/hw/ppc/ppc405_uc.c
+++ b/hw/ppc/ppc405_uc.c
@@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
/*****************************************************************************/
/* Peripheral controller */
-typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
-struct ppc4xx_ebc_t {
- uint32_t addr;
- uint32_t bcr[8];
- uint32_t bap[8];
- uint32_t bear;
- uint32_t besr0;
- uint32_t besr1;
- uint32_t cfg;
-};
-
enum {
EBC0_CFGADDR = 0x012,
EBC0_CFGDATA = 0x013,
@@ -411,10 +400,9 @@ enum {
static uint32_t dcr_read_ebc (void *opaque, int dcrn)
{
- ppc4xx_ebc_t *ebc;
+ Ppc405EbcState *ebc = PPC405_EBC(opaque);
uint32_t ret;
- ebc = opaque;
switch (dcrn) {
case EBC0_CFGADDR:
ret = ebc->addr;
@@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn)
static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
{
- ppc4xx_ebc_t *ebc;
+ Ppc405EbcState *ebc = PPC405_EBC(opaque);
- ebc = opaque;
switch (dcrn) {
case EBC0_CFGADDR:
ebc->addr = val;
@@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
}
}
-static void ebc_reset (void *opaque)
+static void ppc405_ebc_reset(DeviceState *dev)
{
- ppc4xx_ebc_t *ebc;
+ Ppc405EbcState *ebc = PPC405_EBC(dev);
int i;
- ebc = opaque;
ebc->addr = 0x00000000;
ebc->bap[0] = 0x7F8FFE80;
ebc->bcr[0] = 0xFFE28000;
@@ -572,18 +558,46 @@ static void ebc_reset (void *opaque)
ebc->cfg = 0x80400000;
}
-void ppc405_ebc_init(CPUPPCState *env)
+static void ppc405_ebc_realize(DeviceState *dev, Error **errp)
{
- ppc4xx_ebc_t *ebc;
+ Ppc405EbcState *ebc = PPC405_EBC(dev);
+ CPUPPCState *env;
+
+ assert(ebc->cpu);
+
+ env = &ebc->cpu->env;
- ebc = g_new0(ppc4xx_ebc_t, 1);
- qemu_register_reset(&ebc_reset, ebc);
ppc_dcr_register(env, EBC0_CFGADDR,
ebc, &dcr_read_ebc, &dcr_write_ebc);
ppc_dcr_register(env, EBC0_CFGDATA,
ebc, &dcr_read_ebc, &dcr_write_ebc);
}
+static Property ppc405_ebc_properties[] = {
+ DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU,
+ PowerPCCPU *),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ppc405_ebc_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+
+ dc->realize = ppc405_ebc_realize;
+ dc->user_creatable = false;
+ dc->reset = ppc405_ebc_reset;
+ device_class_set_props(dc, ppc405_ebc_properties);
+}
+
+void ppc405_ebc_init(CPUPPCState *env)
+{
+ PowerPCCPU *cpu = env_archcpu(env);
+ DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
+
+ object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
+ qdev_realize_and_unref(dev, NULL, &error_fatal);
+}
+
/*****************************************************************************/
/* DMA controller */
enum {
@@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj)
object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
+
+ object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
}
static void ppc405_soc_realize(DeviceState *dev, Error **errp)
@@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
s->ram_bases, s->ram_sizes, s->do_dram_init);
/* External bus controller */
- ppc405_ebc_init(env);
+ object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu),
+ &error_abort);
+ if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) {
+ return;
+ }
/* DMA controller */
object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu),
@@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data)
static const TypeInfo ppc405_types[] = {
{
+ .name = TYPE_PPC405_EBC,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(Ppc405EbcState),
+ .class_init = ppc405_ebc_class_init,
+ }, {
.name = TYPE_PPC405_DMA,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(Ppc405DmaState),
--
2.37.1
Cedric,
On 8/3/22 10:28, Cédric Le Goater wrote:
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/ppc405.h | 16 +++++++++++
> hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
> 2 files changed, 64 insertions(+), 23 deletions(-)
>
> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> index 1da34a7f10f3..1c7fe07b8084 100644
> --- a/hw/ppc/ppc405.h
> +++ b/hw/ppc/ppc405.h
> @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
>
> typedef struct Ppc405SoCState Ppc405SoCState;
>
> +/* Peripheral controller */
> +#define TYPE_PPC405_EBC "ppc405-ebc"
> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
> +struct Ppc405EbcState {
> + DeviceState parent_obj;
> +
> + PowerPCCPU *cpu;
>
> + uint32_t addr;
> + uint32_t bcr[8];
> + uint32_t bap[8];
> + uint32_t bear;
> + uint32_t besr0;
> + uint32_t besr1;
> + uint32_t cfg;
> +};
>
> /* DMA controller */
> #define TYPE_PPC405_DMA "ppc405-dma"
> @@ -203,6 +218,7 @@ struct Ppc405SoCState {
> Ppc405OcmState ocm;
> Ppc405GpioState gpio;
> Ppc405DmaState dma;
> + Ppc405EbcState ebc;
> };
>
> /* PowerPC 405 core */
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index 6bd93c1cb90c..0166f3fc36da 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
>
> /*****************************************************************************/
> /* Peripheral controller */
> -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
> -struct ppc4xx_ebc_t {
> - uint32_t addr;
> - uint32_t bcr[8];
> - uint32_t bap[8];
> - uint32_t bear;
> - uint32_t besr0;
> - uint32_t besr1;
> - uint32_t cfg;
> -};
> -
> enum {
> EBC0_CFGADDR = 0x012,
> EBC0_CFGDATA = 0x013,
> @@ -411,10 +400,9 @@ enum {
>
> static uint32_t dcr_read_ebc (void *opaque, int dcrn)
> {
> - ppc4xx_ebc_t *ebc;
> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
> uint32_t ret;
>
> - ebc = opaque;
> switch (dcrn) {
> case EBC0_CFGADDR:
> ret = ebc->addr;
> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>
> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
> {
> - ppc4xx_ebc_t *ebc;
> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>
> - ebc = opaque;
> switch (dcrn) {
> case EBC0_CFGADDR:
> ebc->addr = val;
> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
> }
> }
>
> -static void ebc_reset (void *opaque)
> +static void ppc405_ebc_reset(DeviceState *dev)
> {
> - ppc4xx_ebc_t *ebc;
> + Ppc405EbcState *ebc = PPC405_EBC(dev);
> int i;
>
> - ebc = opaque;
> ebc->addr = 0x00000000;
> ebc->bap[0] = 0x7F8FFE80;
> ebc->bcr[0] = 0xFFE28000;
> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque)
> ebc->cfg = 0x80400000;
> }
>
> -void ppc405_ebc_init(CPUPPCState *env)
> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp)
> {
> - ppc4xx_ebc_t *ebc;
> + Ppc405EbcState *ebc = PPC405_EBC(dev);
> + CPUPPCState *env;
> +
> + assert(ebc->cpu);
> +
> + env = &ebc->cpu->env;
>
> - ebc = g_new0(ppc4xx_ebc_t, 1);
> - qemu_register_reset(&ebc_reset, ebc);
> ppc_dcr_register(env, EBC0_CFGADDR,
> ebc, &dcr_read_ebc, &dcr_write_ebc);
> ppc_dcr_register(env, EBC0_CFGDATA,
> ebc, &dcr_read_ebc, &dcr_write_ebc);
> }
>
> +static Property ppc405_ebc_properties[] = {
> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU,
> + PowerPCCPU *),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->realize = ppc405_ebc_realize;
> + dc->user_creatable = false;
> + dc->reset = ppc405_ebc_reset;
> + device_class_set_props(dc, ppc405_ebc_properties);
> +}
> +
> +void ppc405_ebc_init(CPUPPCState *env)
> +{
> + PowerPCCPU *cpu = env_archcpu(env);
> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
> +
> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
This line is breaking the boot of sam460ex:
./qemu-system-ppc64 -display none -M sam460ex
Unexpected error in object_property_find_err() at ../qom/object.c:1304:
qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found
Aborted (core dumped)
I think you meant to link the cpu prop of the EBC obj to the CPU object,
not the cpu prop of the CPU obj to the EBC dev.
This fixes the issue:
$ git diff
diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
index 0166f3fc36..aac3a3f761 100644
--- a/hw/ppc/ppc405_uc.c
+++ b/hw/ppc/ppc405_uc.c
@@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env)
PowerPCCPU *cpu = env_archcpu(env);
DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
- object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
+ object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort);
qdev_realize_and_unref(dev, NULL, &error_fatal);
}
Daniel
> + qdev_realize_and_unref(dev, NULL, &error_fatal);
> +}
> +
> /*****************************************************************************/
> /* DMA controller */
> enum {
> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj)
> object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
>
> object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
> +
> + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
> }
>
> static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> s->ram_bases, s->ram_sizes, s->do_dram_init);
>
> /* External bus controller */
> - ppc405_ebc_init(env);
> + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu),
> + &error_abort);
> + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) {
> + return;
> + }
>
> /* DMA controller */
> object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu),
> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data)
>
> static const TypeInfo ppc405_types[] = {
> {
> + .name = TYPE_PPC405_EBC,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(Ppc405EbcState),
> + .class_init = ppc405_ebc_class_init,
> + }, {
> .name = TYPE_PPC405_DMA,
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(Ppc405DmaState),
On 8/4/22 01:36, Daniel Henrique Barboza wrote:
> Cedric,
>
> On 8/3/22 10:28, Cédric Le Goater wrote:
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> hw/ppc/ppc405.h | 16 +++++++++++
>> hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
>> 2 files changed, 64 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
>> index 1da34a7f10f3..1c7fe07b8084 100644
>> --- a/hw/ppc/ppc405.h
>> +++ b/hw/ppc/ppc405.h
>> @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
>> typedef struct Ppc405SoCState Ppc405SoCState;
>> +/* Peripheral controller */
>> +#define TYPE_PPC405_EBC "ppc405-ebc"
>> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
>> +struct Ppc405EbcState {
>> + DeviceState parent_obj;
>> +
>> + PowerPCCPU *cpu;
>> + uint32_t addr;
>> + uint32_t bcr[8];
>> + uint32_t bap[8];
>> + uint32_t bear;
>> + uint32_t besr0;
>> + uint32_t besr1;
>> + uint32_t cfg;
>> +};
>> /* DMA controller */
>> #define TYPE_PPC405_DMA "ppc405-dma"
>> @@ -203,6 +218,7 @@ struct Ppc405SoCState {
>> Ppc405OcmState ocm;
>> Ppc405GpioState gpio;
>> Ppc405DmaState dma;
>> + Ppc405EbcState ebc;
>> };
>> /* PowerPC 405 core */
>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>> index 6bd93c1cb90c..0166f3fc36da 100644
>> --- a/hw/ppc/ppc405_uc.c
>> +++ b/hw/ppc/ppc405_uc.c
>> @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
>> /*****************************************************************************/
>> /* Peripheral controller */
>> -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
>> -struct ppc4xx_ebc_t {
>> - uint32_t addr;
>> - uint32_t bcr[8];
>> - uint32_t bap[8];
>> - uint32_t bear;
>> - uint32_t besr0;
>> - uint32_t besr1;
>> - uint32_t cfg;
>> -};
>> -
>> enum {
>> EBC0_CFGADDR = 0x012,
>> EBC0_CFGDATA = 0x013,
>> @@ -411,10 +400,9 @@ enum {
>> static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>> {
>> - ppc4xx_ebc_t *ebc;
>> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>> uint32_t ret;
>> - ebc = opaque;
>> switch (dcrn) {
>> case EBC0_CFGADDR:
>> ret = ebc->addr;
>> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
>> {
>> - ppc4xx_ebc_t *ebc;
>> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>> - ebc = opaque;
>> switch (dcrn) {
>> case EBC0_CFGADDR:
>> ebc->addr = val;
>> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
>> }
>> }
>> -static void ebc_reset (void *opaque)
>> +static void ppc405_ebc_reset(DeviceState *dev)
>> {
>> - ppc4xx_ebc_t *ebc;
>> + Ppc405EbcState *ebc = PPC405_EBC(dev);
>> int i;
>> - ebc = opaque;
>> ebc->addr = 0x00000000;
>> ebc->bap[0] = 0x7F8FFE80;
>> ebc->bcr[0] = 0xFFE28000;
>> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque)
>> ebc->cfg = 0x80400000;
>> }
>> -void ppc405_ebc_init(CPUPPCState *env)
>> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp)
>> {
>> - ppc4xx_ebc_t *ebc;
>> + Ppc405EbcState *ebc = PPC405_EBC(dev);
>> + CPUPPCState *env;
>> +
>> + assert(ebc->cpu);
>> +
>> + env = &ebc->cpu->env;
>> - ebc = g_new0(ppc4xx_ebc_t, 1);
>> - qemu_register_reset(&ebc_reset, ebc);
>> ppc_dcr_register(env, EBC0_CFGADDR,
>> ebc, &dcr_read_ebc, &dcr_write_ebc);
>> ppc_dcr_register(env, EBC0_CFGDATA,
>> ebc, &dcr_read_ebc, &dcr_write_ebc);
>> }
>> +static Property ppc405_ebc_properties[] = {
>> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU,
>> + PowerPCCPU *),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> + dc->realize = ppc405_ebc_realize;
>> + dc->user_creatable = false;
>> + dc->reset = ppc405_ebc_reset;
>> + device_class_set_props(dc, ppc405_ebc_properties);
>> +}
>> +
>> +void ppc405_ebc_init(CPUPPCState *env)
>> +{
>> + PowerPCCPU *cpu = env_archcpu(env);
>> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
>> +
>> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
>
> This line is breaking the boot of sam460ex:
>
>
> ./qemu-system-ppc64 -display none -M sam460ex
> Unexpected error in object_property_find_err() at ../qom/object.c:1304:
> qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found
> Aborted (core dumped)
>
>
> I think you meant to link the cpu prop of the EBC obj to the CPU object,
> not the cpu prop of the CPU obj to the EBC dev.
Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't
test :/
Thanks,
C.
>
> This fixes the issue:
>
>
> $ git diff
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index 0166f3fc36..aac3a3f761 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env)
> PowerPCCPU *cpu = env_archcpu(env);
> DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
>
> - object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
> + object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort);
> qdev_realize_and_unref(dev, NULL, &error_fatal);
> }
>
>
> Daniel
>
>
>> + qdev_realize_and_unref(dev, NULL, &error_fatal);
>> +}
>> +
>> /*****************************************************************************/
>> /* DMA controller */
>> enum {
>> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj)
>> object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
>> object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
>> +
>> + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
>> }
>> static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>> s->ram_bases, s->ram_sizes, s->do_dram_init);
>> /* External bus controller */
>> - ppc405_ebc_init(env);
>> + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu),
>> + &error_abort);
>> + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) {
>> + return;
>> + }
>> /* DMA controller */
>> object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu),
>> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data)
>> static const TypeInfo ppc405_types[] = {
>> {
>> + .name = TYPE_PPC405_EBC,
>> + .parent = TYPE_DEVICE,
>> + .instance_size = sizeof(Ppc405EbcState),
>> + .class_init = ppc405_ebc_class_init,
>> + }, {
>> .name = TYPE_PPC405_DMA,
>> .parent = TYPE_SYS_BUS_DEVICE,
>> .instance_size = sizeof(Ppc405DmaState),
On Thu, 4 Aug 2022, Cédric Le Goater wrote:
> On 8/4/22 01:36, Daniel Henrique Barboza wrote:
>> Cedric,
>>
>> On 8/3/22 10:28, Cédric Le Goater wrote:
>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>> hw/ppc/ppc405.h | 16 +++++++++++
>>> hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
>>> 2 files changed, 64 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
>>> index 1da34a7f10f3..1c7fe07b8084 100644
>>> --- a/hw/ppc/ppc405.h
>>> +++ b/hw/ppc/ppc405.h
>>> @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
>>> typedef struct Ppc405SoCState Ppc405SoCState;
>>> +/* Peripheral controller */
>>> +#define TYPE_PPC405_EBC "ppc405-ebc"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
>>> +struct Ppc405EbcState {
>>> + DeviceState parent_obj;
>>> +
>>> + PowerPCCPU *cpu;
>>> + uint32_t addr;
>>> + uint32_t bcr[8];
>>> + uint32_t bap[8];
>>> + uint32_t bear;
>>> + uint32_t besr0;
>>> + uint32_t besr1;
>>> + uint32_t cfg;
>>> +};
>>> /* DMA controller */
>>> #define TYPE_PPC405_DMA "ppc405-dma"
>>> @@ -203,6 +218,7 @@ struct Ppc405SoCState {
>>> Ppc405OcmState ocm;
>>> Ppc405GpioState gpio;
>>> Ppc405DmaState dma;
>>> + Ppc405EbcState ebc;
>>> };
>>> /* PowerPC 405 core */
>>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>>> index 6bd93c1cb90c..0166f3fc36da 100644
>>> --- a/hw/ppc/ppc405_uc.c
>>> +++ b/hw/ppc/ppc405_uc.c
>>> @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
>>>
>>> /*****************************************************************************/
>>> /* Peripheral controller */
>>> -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
>>> -struct ppc4xx_ebc_t {
>>> - uint32_t addr;
>>> - uint32_t bcr[8];
>>> - uint32_t bap[8];
>>> - uint32_t bear;
>>> - uint32_t besr0;
>>> - uint32_t besr1;
>>> - uint32_t cfg;
>>> -};
>>> -
>>> enum {
>>> EBC0_CFGADDR = 0x012,
>>> EBC0_CFGDATA = 0x013,
>>> @@ -411,10 +400,9 @@ enum {
>>> static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>>> {
>>> - ppc4xx_ebc_t *ebc;
>>> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>>> uint32_t ret;
>>> - ebc = opaque;
>>> switch (dcrn) {
>>> case EBC0_CFGADDR:
>>> ret = ebc->addr;
>>> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>>> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
>>> {
>>> - ppc4xx_ebc_t *ebc;
>>> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>>> - ebc = opaque;
>>> switch (dcrn) {
>>> case EBC0_CFGADDR:
>>> ebc->addr = val;
>>> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn,
>>> uint32_t val)
>>> }
>>> }
>>> -static void ebc_reset (void *opaque)
>>> +static void ppc405_ebc_reset(DeviceState *dev)
>>> {
>>> - ppc4xx_ebc_t *ebc;
>>> + Ppc405EbcState *ebc = PPC405_EBC(dev);
>>> int i;
>>> - ebc = opaque;
>>> ebc->addr = 0x00000000;
>>> ebc->bap[0] = 0x7F8FFE80;
>>> ebc->bcr[0] = 0xFFE28000;
>>> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque)
>>> ebc->cfg = 0x80400000;
>>> }
>>> -void ppc405_ebc_init(CPUPPCState *env)
>>> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp)
>>> {
>>> - ppc4xx_ebc_t *ebc;
>>> + Ppc405EbcState *ebc = PPC405_EBC(dev);
>>> + CPUPPCState *env;
>>> +
>>> + assert(ebc->cpu);
>>> +
>>> + env = &ebc->cpu->env;
>>> - ebc = g_new0(ppc4xx_ebc_t, 1);
>>> - qemu_register_reset(&ebc_reset, ebc);
>>> ppc_dcr_register(env, EBC0_CFGADDR,
>>> ebc, &dcr_read_ebc, &dcr_write_ebc);
>>> ppc_dcr_register(env, EBC0_CFGDATA,
>>> ebc, &dcr_read_ebc, &dcr_write_ebc);
>>> }
>>> +static Property ppc405_ebc_properties[] = {
>>> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU,
>>> + PowerPCCPU *),
>>> + DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data)
>>> +{
>>> + DeviceClass *dc = DEVICE_CLASS(oc);
>>> +
>>> + dc->realize = ppc405_ebc_realize;
>>> + dc->user_creatable = false;
>>> + dc->reset = ppc405_ebc_reset;
>>> + device_class_set_props(dc, ppc405_ebc_properties);
>>> +}
>>> +
>>> +void ppc405_ebc_init(CPUPPCState *env)
>>> +{
>>> + PowerPCCPU *cpu = env_archcpu(env);
>>> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
>>> +
>>> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev),
>>> &error_abort);
>>
>> This line is breaking the boot of sam460ex:
>>
>>
>> ./qemu-system-ppc64 -display none -M sam460ex
>> Unexpected error in object_property_find_err() at ../qom/object.c:1304:
>> qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found
>> Aborted (core dumped)
>>
>>
>> I think you meant to link the cpu prop of the EBC obj to the CPU object,
>> not the cpu prop of the CPU obj to the EBC dev.
>
> Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't
> test :/
This patch changes ppc405_ebc_init to a realize method so shouldn't the
sam460ex be changed to create the new object instead of calling
ppc405_ebc_init too instead? Is the only reason the keep ppc405_ebc_init
to add the cpu link? As I noted before it would be nice to get rid of this
link somehow, it would allow dropping this init func and a bunch of
property descriptors where this cpu link is the only object. It should be
possble to get from a QOM object to its parent and the cpu from there but
I could not find out how. Maybe somehow with object_resolve_path() or
object_resolve_path_type() but I don't know QOM enough and did not find
anything in docs. Does somebody know how to do that? Or maybe the paths
are always the same so it could resolve an absolute path. Don't know how
it looks buth something like /machine/soc/cpu or similar to get to the cpu
to get the env. This could work as long as we assume we only have one cpu
but these SoC all have. Then no cpu link is needed and could get rid of a
lot of boilerplate code. Does this make sense?
Regards,
BALATON Zoltan
> Thanks,
>
> C.
>
>>
>> This fixes the issue:
>>
>>
>> $ git diff
>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>> index 0166f3fc36..aac3a3f761 100644
>> --- a/hw/ppc/ppc405_uc.c
>> +++ b/hw/ppc/ppc405_uc.c
>> @@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env)
>> PowerPCCPU *cpu = env_archcpu(env);
>> DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
>>
>> - object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev),
>> &error_abort);
>> + object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu),
>> &error_abort);
>> qdev_realize_and_unref(dev, NULL, &error_fatal);
>> }
>>
>>
>> Daniel
>>
>>
>>> + qdev_realize_and_unref(dev, NULL, &error_fatal);
>>> +}
>>> +
>>>
>>> /*****************************************************************************/
>>> /* DMA controller */
>>> enum {
>>> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj)
>>> object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
>>> object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
>>> +
>>> + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
>>> }
>>> static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>>> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev,
>>> Error **errp)
>>> s->ram_bases, s->ram_sizes, s->do_dram_init);
>>> /* External bus controller */
>>> - ppc405_ebc_init(env);
>>> + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu),
>>> + &error_abort);
>>> + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) {
>>> + return;
>>> + }
>>> /* DMA controller */
>>> object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu),
>>> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc,
>>> void *data)
>>> static const TypeInfo ppc405_types[] = {
>>> {
>>> + .name = TYPE_PPC405_EBC,
>>> + .parent = TYPE_DEVICE,
>>> + .instance_size = sizeof(Ppc405EbcState),
>>> + .class_init = ppc405_ebc_class_init,
>>> + }, {
>>> .name = TYPE_PPC405_DMA,
>>> .parent = TYPE_SYS_BUS_DEVICE,
>>> .instance_size = sizeof(Ppc405DmaState),
>
>
>
[ Resending to all ]
On 8/4/22 14:09, BALATON Zoltan wrote:
> On Thu, 4 Aug 2022, Cédric Le Goater wrote:
>> On 8/4/22 01:36, Daniel Henrique Barboza wrote:
>>> Cedric,
>>>
>>> On 8/3/22 10:28, Cédric Le Goater wrote:
>>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>> hw/ppc/ppc405.h | 16 +++++++++++
>>>> hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
>>>> 2 files changed, 64 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
>>>> index 1da34a7f10f3..1c7fe07b8084 100644
>>>> --- a/hw/ppc/ppc405.h
>>>> +++ b/hw/ppc/ppc405.h
>>>> @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
>>>> typedef struct Ppc405SoCState Ppc405SoCState;
>>>> +/* Peripheral controller */
>>>> +#define TYPE_PPC405_EBC "ppc405-ebc"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
>>>> +struct Ppc405EbcState {
>>>> + DeviceState parent_obj;
>>>> +
>>>> + PowerPCCPU *cpu;
>>>> + uint32_t addr;
>>>> + uint32_t bcr[8];
>>>> + uint32_t bap[8];
>>>> + uint32_t bear;
>>>> + uint32_t besr0;
>>>> + uint32_t besr1;
>>>> + uint32_t cfg;
>>>> +};
>>>> /* DMA controller */
>>>> #define TYPE_PPC405_DMA "ppc405-dma"
>>>> @@ -203,6 +218,7 @@ struct Ppc405SoCState {
>>>> Ppc405OcmState ocm;
>>>> Ppc405GpioState gpio;
>>>> Ppc405DmaState dma;
>>>> + Ppc405EbcState ebc;
>>>> };
>>>> /* PowerPC 405 core */
>>>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>>>> index 6bd93c1cb90c..0166f3fc36da 100644
>>>> --- a/hw/ppc/ppc405_uc.c
>>>> +++ b/hw/ppc/ppc405_uc.c
>>>> @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
>>>> /*****************************************************************************/
>>>> /* Peripheral controller */
>>>> -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
>>>> -struct ppc4xx_ebc_t {
>>>> - uint32_t addr;
>>>> - uint32_t bcr[8];
>>>> - uint32_t bap[8];
>>>> - uint32_t bear;
>>>> - uint32_t besr0;
>>>> - uint32_t besr1;
>>>> - uint32_t cfg;
>>>> -};
>>>> -
>>>> enum {
>>>> EBC0_CFGADDR = 0x012,
>>>> EBC0_CFGDATA = 0x013,
>>>> @@ -411,10 +400,9 @@ enum {
>>>> static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>>>> {
>>>> - ppc4xx_ebc_t *ebc;
>>>> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>>>> uint32_t ret;
>>>> - ebc = opaque;
>>>> switch (dcrn) {
>>>> case EBC0_CFGADDR:
>>>> ret = ebc->addr;
>>>> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>>>> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
>>>> {
>>>> - ppc4xx_ebc_t *ebc;
>>>> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>>>> - ebc = opaque;
>>>> switch (dcrn) {
>>>> case EBC0_CFGADDR:
>>>> ebc->addr = val;
>>>> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
>>>> }
>>>> }
>>>> -static void ebc_reset (void *opaque)
>>>> +static void ppc405_ebc_reset(DeviceState *dev)
>>>> {
>>>> - ppc4xx_ebc_t *ebc;
>>>> + Ppc405EbcState *ebc = PPC405_EBC(dev);
>>>> int i;
>>>> - ebc = opaque;
>>>> ebc->addr = 0x00000000;
>>>> ebc->bap[0] = 0x7F8FFE80;
>>>> ebc->bcr[0] = 0xFFE28000;
>>>> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque)
>>>> ebc->cfg = 0x80400000;
>>>> }
>>>> -void ppc405_ebc_init(CPUPPCState *env)
>>>> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp)
>>>> {
>>>> - ppc4xx_ebc_t *ebc;
>>>> + Ppc405EbcState *ebc = PPC405_EBC(dev);
>>>> + CPUPPCState *env;
>>>> +
>>>> + assert(ebc->cpu);
>>>> +
>>>> + env = &ebc->cpu->env;
>>>> - ebc = g_new0(ppc4xx_ebc_t, 1);
>>>> - qemu_register_reset(&ebc_reset, ebc);
>>>> ppc_dcr_register(env, EBC0_CFGADDR,
>>>> ebc, &dcr_read_ebc, &dcr_write_ebc);
>>>> ppc_dcr_register(env, EBC0_CFGDATA,
>>>> ebc, &dcr_read_ebc, &dcr_write_ebc);
>>>> }
>>>> +static Property ppc405_ebc_properties[] = {
>>>> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU,
>>>> + PowerPCCPU *),
>>>> + DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>> +
>>>> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data)
>>>> +{
>>>> + DeviceClass *dc = DEVICE_CLASS(oc);
>>>> +
>>>> + dc->realize = ppc405_ebc_realize;
>>>> + dc->user_creatable = false;
>>>> + dc->reset = ppc405_ebc_reset;
>>>> + device_class_set_props(dc, ppc405_ebc_properties);
>>>> +}
>>>> +
>>>> +void ppc405_ebc_init(CPUPPCState *env)
>>>> +{
>>>> + PowerPCCPU *cpu = env_archcpu(env);
>>>> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
>>>> +
>>>> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
>>>
>>> This line is breaking the boot of sam460ex:
>>>
>>>
>>> ./qemu-system-ppc64 -display none -M sam460ex
>>> Unexpected error in object_property_find_err() at ../qom/object.c:1304:
>>> qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found
>>> Aborted (core dumped)
>>>
>>>
>>> I think you meant to link the cpu prop of the EBC obj to the CPU object,
>>> not the cpu prop of the CPU obj to the EBC dev.
>>
>> Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't
>> test :/
>
> This patch changes ppc405_ebc_init to a realize method so shouldn't the sam460ex be changed to create the new object instead of calling ppc405_ebc_init too instead?
Sure.
First step was to make sure nothing was broken. I can add some extra
patches in v3 to convert ppc405_ebc_init(), ppc4xx_plb_init() and
ppc4xx_mal_init() in the ppc4x machines. I don't think that would be
too much work. It's a good opportunity to modernize a bit the ppc4xx
machines also.
> Is the only reason the keep ppc405_ebc_init to add the cpu link?
Yes. That's all there is to it really: convert the routines
parameters in object properties.
> As I noted before it would be nice to get rid of this link somehow,
> it would allow dropping this init func and a bunch of property
> descriptors where this cpu link is the only object.
We should introduce a DCR namespace instead and use DCR memory regions
but that's much more work.
> It should be possble to get from a QOM object to its parent and the
> cpu from there but I could not find out how. Maybe somehow with
> object_resolve_path() or object_resolve_path_type() but I don't know
> QOM enough and did not find anything in docs.
>
> Does somebody know how to do that?
One way, would be to introduce a base class DCRDeviceState with a "cpu"
link and a class handler to install the DCR read/write ops. But I think
it's not worth the time and effort. Adding DCR namespace and DCR memory
regions would be better.
Thanks,
C.
> Or maybe the paths are always the same so it could resolve an absolute path. Don't know how it looks buth something like /machine/soc/cpu or similar to get to the cpu to get the env. This could work as long as we assume we only have one cpu but these SoC all have. Then no cpu link is needed and
> could get rid of a lot of boilerplate code. Does this make sense?
>
> Regards,
> BALATON Zoltan
>
>> Thanks,
>>
>> C.
>>
>>>
>>> This fixes the issue:
>>>
>>>
>>> $ git diff
>>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>>> index 0166f3fc36..aac3a3f761 100644
>>> --- a/hw/ppc/ppc405_uc.c
>>> +++ b/hw/ppc/ppc405_uc.c
>>> @@ -594,7 +594,7 @@ void ppc405_ebc_init(CPUPPCState *env)
>>> PowerPCCPU *cpu = env_archcpu(env);
>>> DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
>>>
>>> - object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
>>> + object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort);
>>> qdev_realize_and_unref(dev, NULL, &error_fatal);
>>> }
>>>
>>>
>>> Daniel
>>>
>>>
>>>> + qdev_realize_and_unref(dev, NULL, &error_fatal);
>>>> +}
>>>> +
>>>> /*****************************************************************************/
>>>> /* DMA controller */
>>>> enum {
>>>> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj)
>>>> object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
>>>> object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
>>>> +
>>>> + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
>>>> }
>>>> static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>>>> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>>>> s->ram_bases, s->ram_sizes, s->do_dram_init);
>>>> /* External bus controller */
>>>> - ppc405_ebc_init(env);
>>>> + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu),
>>>> + &error_abort);
>>>> + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) {
>>>> + return;
>>>> + }
>>>> /* DMA controller */
>>>> object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu),
>>>> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data)
>>>> static const TypeInfo ppc405_types[] = {
>>>> {
>>>> + .name = TYPE_PPC405_EBC,
>>>> + .parent = TYPE_DEVICE,
>>>> + .instance_size = sizeof(Ppc405EbcState),
>>>> + .class_init = ppc405_ebc_class_init,
>>>> + }, {
>>>> .name = TYPE_PPC405_DMA,
>>>> .parent = TYPE_SYS_BUS_DEVICE,
>>>> .instance_size = sizeof(Ppc405DmaState),
>>
>>
>>
On Wed, 3 Aug 2022, Cédric Le Goater wrote:
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/ppc405.h | 16 +++++++++++
> hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
> 2 files changed, 64 insertions(+), 23 deletions(-)
>
> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> index 1da34a7f10f3..1c7fe07b8084 100644
> --- a/hw/ppc/ppc405.h
> +++ b/hw/ppc/ppc405.h
> @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
>
> typedef struct Ppc405SoCState Ppc405SoCState;
>
> +/* Peripheral controller */
> +#define TYPE_PPC405_EBC "ppc405-ebc"
> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
> +struct Ppc405EbcState {
> + DeviceState parent_obj;
> +
> + PowerPCCPU *cpu;
>
> + uint32_t addr;
> + uint32_t bcr[8];
> + uint32_t bap[8];
> + uint32_t bear;
> + uint32_t besr0;
> + uint32_t besr1;
> + uint32_t cfg;
> +};
>
> /* DMA controller */
> #define TYPE_PPC405_DMA "ppc405-dma"
> @@ -203,6 +218,7 @@ struct Ppc405SoCState {
> Ppc405OcmState ocm;
> Ppc405GpioState gpio;
> Ppc405DmaState dma;
> + Ppc405EbcState ebc;
> };
>
> /* PowerPC 405 core */
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index 6bd93c1cb90c..0166f3fc36da 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
>
> /*****************************************************************************/
> /* Peripheral controller */
> -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
> -struct ppc4xx_ebc_t {
> - uint32_t addr;
> - uint32_t bcr[8];
> - uint32_t bap[8];
> - uint32_t bear;
> - uint32_t besr0;
> - uint32_t besr1;
> - uint32_t cfg;
> -};
> -
> enum {
> EBC0_CFGADDR = 0x012,
> EBC0_CFGDATA = 0x013,
> @@ -411,10 +400,9 @@ enum {
>
> static uint32_t dcr_read_ebc (void *opaque, int dcrn)
> {
> - ppc4xx_ebc_t *ebc;
> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
> uint32_t ret;
>
> - ebc = opaque;
I think QOM casts are kind of expensive (maybe because we have quo-debug
enabled by default even without --enable-debug and it does additional
checks; I've tried to change this default once but it was thought to be
better to have it enabled). So it's advised to use QOM casts sparingly,
e.g. store the result in a local variable if you need it more than once
and so. Therefore I tend to consider these read/write callbacks that the
object itself registers with itself as the opaque pointer to be internal
to the object and guaranteed to be passed the object pointer so no QOM
cast is necessary and the direct assignment can be kept. This avoids
potential overhead on every register access. Not sure if it's measurable
but I think if an overhead can be avoided it probably should be.
> switch (dcrn) {
> case EBC0_CFGADDR:
> ret = ebc->addr;
> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>
> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
> {
> - ppc4xx_ebc_t *ebc;
> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>
> - ebc = opaque;
> switch (dcrn) {
> case EBC0_CFGADDR:
> ebc->addr = val;
> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
> }
> }
>
> -static void ebc_reset (void *opaque)
> +static void ppc405_ebc_reset(DeviceState *dev)
> {
> - ppc4xx_ebc_t *ebc;
> + Ppc405EbcState *ebc = PPC405_EBC(dev);
In this case the cast is OK as it's casting a different object so it's
needed and also it's infrequently called so should not matter.
> int i;
>
> - ebc = opaque;
> ebc->addr = 0x00000000;
> ebc->bap[0] = 0x7F8FFE80;
> ebc->bcr[0] = 0xFFE28000;
> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque)
> ebc->cfg = 0x80400000;
> }
>
> -void ppc405_ebc_init(CPUPPCState *env)
> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp)
> {
> - ppc4xx_ebc_t *ebc;
> + Ppc405EbcState *ebc = PPC405_EBC(dev);
> + CPUPPCState *env;
> +
> + assert(ebc->cpu);
> +
> + env = &ebc->cpu->env;
>
> - ebc = g_new0(ppc4xx_ebc_t, 1);
> - qemu_register_reset(&ebc_reset, ebc);
> ppc_dcr_register(env, EBC0_CFGADDR,
> ebc, &dcr_read_ebc, &dcr_write_ebc);
> ppc_dcr_register(env, EBC0_CFGDATA,
> ebc, &dcr_read_ebc, &dcr_write_ebc);
> }
>
> +static Property ppc405_ebc_properties[] = {
> + DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU,
> + PowerPCCPU *),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> +
> + dc->realize = ppc405_ebc_realize;
> + dc->user_creatable = false;
> + dc->reset = ppc405_ebc_reset;
> + device_class_set_props(dc, ppc405_ebc_properties);
> +}
> +
> +void ppc405_ebc_init(CPUPPCState *env)
> +{
> + PowerPCCPU *cpu = env_archcpu(env);
> + DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
> +
> + object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
> + qdev_realize_and_unref(dev, NULL, &error_fatal);
> +}
> +
> /*****************************************************************************/
> /* DMA controller */
> enum {
> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj)
> object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
>
> object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
> +
> + object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
> }
>
> static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> s->ram_bases, s->ram_sizes, s->do_dram_init);
>
> /* External bus controller */
> - ppc405_ebc_init(env);
> + object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu),
> + &error_abort);
I wonder if this link to cpu could be avoided somehow? Maybe assuming that
this device and the cpu is part of the same SoC it could get it's parent
and access the cpu field of the parent or if that's not possible adding a
method to the SoC to get it could avoid this link?
Regards,
BALATON Zoltan
> + if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) {
> + return;
> + }
>
> /* DMA controller */
> object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu),
> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data)
>
> static const TypeInfo ppc405_types[] = {
> {
> + .name = TYPE_PPC405_EBC,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(Ppc405EbcState),
> + .class_init = ppc405_ebc_class_init,
> + }, {
> .name = TYPE_PPC405_DMA,
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(Ppc405DmaState),
>
On 04/08/2022 00:04, BALATON Zoltan wrote:
> On Wed, 3 Aug 2022, Cédric Le Goater wrote:
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> hw/ppc/ppc405.h | 16 +++++++++++
>> hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
>> 2 files changed, 64 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
>> index 1da34a7f10f3..1c7fe07b8084 100644
>> --- a/hw/ppc/ppc405.h
>> +++ b/hw/ppc/ppc405.h
>> @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
>>
>> typedef struct Ppc405SoCState Ppc405SoCState;
>>
>> +/* Peripheral controller */
>> +#define TYPE_PPC405_EBC "ppc405-ebc"
>> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
>> +struct Ppc405EbcState {
>> + DeviceState parent_obj;
>> +
>> + PowerPCCPU *cpu;
>>
>> + uint32_t addr;
>> + uint32_t bcr[8];
>> + uint32_t bap[8];
>> + uint32_t bear;
>> + uint32_t besr0;
>> + uint32_t besr1;
>> + uint32_t cfg;
>> +};
>>
>> /* DMA controller */
>> #define TYPE_PPC405_DMA "ppc405-dma"
>> @@ -203,6 +218,7 @@ struct Ppc405SoCState {
>> Ppc405OcmState ocm;
>> Ppc405GpioState gpio;
>> Ppc405DmaState dma;
>> + Ppc405EbcState ebc;
>> };
>>
>> /* PowerPC 405 core */
>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>> index 6bd93c1cb90c..0166f3fc36da 100644
>> --- a/hw/ppc/ppc405_uc.c
>> +++ b/hw/ppc/ppc405_uc.c
>> @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
>>
>> /*****************************************************************************/
>> /* Peripheral controller */
>> -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
>> -struct ppc4xx_ebc_t {
>> - uint32_t addr;
>> - uint32_t bcr[8];
>> - uint32_t bap[8];
>> - uint32_t bear;
>> - uint32_t besr0;
>> - uint32_t besr1;
>> - uint32_t cfg;
>> -};
>> -
>> enum {
>> EBC0_CFGADDR = 0x012,
>> EBC0_CFGDATA = 0x013,
>> @@ -411,10 +400,9 @@ enum {
>>
>> static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>> {
>> - ppc4xx_ebc_t *ebc;
>> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>> uint32_t ret;
>>
>> - ebc = opaque;
>
> I think QOM casts are kind of expensive (maybe because we have quo-debug enabled by
> default even without --enable-debug and it does additional checks; I've tried to
> change this default once but it was thought to be better to have it enabled). So it's
> advised to use QOM casts sparingly, e.g. store the result in a local variable if you
> need it more than once and so. Therefore I tend to consider these read/write
> callbacks that the object itself registers with itself as the opaque pointer to be
> internal to the object and guaranteed to be passed the object pointer so no QOM cast
> is necessary and the direct assignment can be kept. This avoids potential overhead on
> every register access. Not sure if it's measurable but I think if an overhead can be
> avoided it probably should be.
Can you provide any evidence for this? IIRC the efficiency of the QOM cast macros
without --enable-debug was improved several years ago to the point where their impact
is minimal (note: this does not include object_dynamic_cast()). From memory the
previous discussions concluded that whilst the QOM cast did add some runtime
overhead, it was dwarfed by the cost of breaking out of emulation to handle the MMIO
access itself. If something has changed here then that sounds like a bug.
I think it's worth keeping the QOM casts in place unless there is a good reason not
to, simply because they have helped me many times in past catch out refactoring
mistakes. For example I can certainly imagine that the recent PHB series would have
been a lot more painful without having them.
ATB,
Mark.
On Thu, 4 Aug 2022, Mark Cave-Ayland wrote:
> On 04/08/2022 00:04, BALATON Zoltan wrote:
>> On Wed, 3 Aug 2022, Cédric Le Goater wrote:
>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>> hw/ppc/ppc405.h | 16 +++++++++++
>>> hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
>>> 2 files changed, 64 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
>>> index 1da34a7f10f3..1c7fe07b8084 100644
>>> --- a/hw/ppc/ppc405.h
>>> +++ b/hw/ppc/ppc405.h
>>> @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
>>>
>>> typedef struct Ppc405SoCState Ppc405SoCState;
>>>
>>> +/* Peripheral controller */
>>> +#define TYPE_PPC405_EBC "ppc405-ebc"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
>>> +struct Ppc405EbcState {
>>> + DeviceState parent_obj;
>>> +
>>> + PowerPCCPU *cpu;
>>>
>>> + uint32_t addr;
>>> + uint32_t bcr[8];
>>> + uint32_t bap[8];
>>> + uint32_t bear;
>>> + uint32_t besr0;
>>> + uint32_t besr1;
>>> + uint32_t cfg;
>>> +};
>>>
>>> /* DMA controller */
>>> #define TYPE_PPC405_DMA "ppc405-dma"
>>> @@ -203,6 +218,7 @@ struct Ppc405SoCState {
>>> Ppc405OcmState ocm;
>>> Ppc405GpioState gpio;
>>> Ppc405DmaState dma;
>>> + Ppc405EbcState ebc;
>>> };
>>>
>>> /* PowerPC 405 core */
>>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>>> index 6bd93c1cb90c..0166f3fc36da 100644
>>> --- a/hw/ppc/ppc405_uc.c
>>> +++ b/hw/ppc/ppc405_uc.c
>>> @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
>>>
>>> /*****************************************************************************/
>>> /* Peripheral controller */
>>> -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
>>> -struct ppc4xx_ebc_t {
>>> - uint32_t addr;
>>> - uint32_t bcr[8];
>>> - uint32_t bap[8];
>>> - uint32_t bear;
>>> - uint32_t besr0;
>>> - uint32_t besr1;
>>> - uint32_t cfg;
>>> -};
>>> -
>>> enum {
>>> EBC0_CFGADDR = 0x012,
>>> EBC0_CFGDATA = 0x013,
>>> @@ -411,10 +400,9 @@ enum {
>>>
>>> static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>>> {
>>> - ppc4xx_ebc_t *ebc;
>>> + Ppc405EbcState *ebc = PPC405_EBC(opaque);
>>> uint32_t ret;
>>>
>>> - ebc = opaque;
>>
>> I think QOM casts are kind of expensive (maybe because we have quo-debug
>> enabled by default even without --enable-debug and it does additional
>> checks; I've tried to change this default once but it was thought to be
>> better to have it enabled). So it's advised to use QOM casts sparingly,
>> e.g. store the result in a local variable if you need it more than once and
>> so. Therefore I tend to consider these read/write callbacks that the object
>> itself registers with itself as the opaque pointer to be internal to the
>> object and guaranteed to be passed the object pointer so no QOM cast is
>> necessary and the direct assignment can be kept. This avoids potential
>> overhead on every register access. Not sure if it's measurable but I think
>> if an overhead can be avoided it probably should be.
>
> Can you provide any evidence for this? IIRC the efficiency of the QOM cast
> macros without --enable-debug was improved several years ago to the point
> where their impact is minimal (note: this does not include
> object_dynamic_cast()). From memory the previous discussions concluded that
It probably could be measured on a slower machine when something does a
lot of register access but I did not have any concrete numbers to prove it
and in this particular case not sure how often this device is accessed if
it does anything at all. But this is a general remark for all devices. An
IDE device could be accessed a lot of times for example so I generally
try to avoid unnecessary overhead.
AFAIK (which could well be wrong) a QOM cast is optimised down to a simple
cast if qom-debug is disabled. Problem is it's never disabled unless
somebody explicitly compiles with --disable-qom-cast-debug as this is
enabled by default, even in release builds without --enable-debug. At
least that was the case when this was in configure, I don't know where it
went during meson conversion but I think the default haven't changed. With
qom-cast-debug a QOM cast is ultimately calling object_dynamic_cast_assert
in OBJECT_CHECK.
Here is the discussion when I've tried to change this:
https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03371.html
> whilst the QOM cast did add some runtime overhead, it was dwarfed by the cost
> of breaking out of emulation to handle the MMIO access itself. If something
> has changed here then that sounds like a bug.
Not saying it has changed but having something already slow is not an
argument to make it even slower if that additional overhead can be
avoided. Maybe that makes it a little less slow even if the main reason
for slowness is not this.
> I think it's worth keeping the QOM casts in place unless there is a good
> reason not to, simply because they have helped me many times in past catch
> out refactoring mistakes. For example I can certainly imagine that the recent
> PHB series would have been a lot more painful without having them.
A good reason in my opinion is that these are read/write callbacks of the
object whith are registered in the realize method with the object itself
as the opaque parameter which was already QOM cast from the realize
method's device parameter so there's no way these read/wtite callbacks are
called with an unchecked object. Therefore the QOM cast with check is
unnecessary here and we can safely assign it to the appropriate type
without checcking it again at every register access. Because of this, I
always avoid QOM casts in these callback functions as this can only make
things better and unlikely to make it worse.
The QOM casts are warranted in the object methods such as realize or init
that maybe somehow could be called with a wrong object (I'm not sure why
if these are object methods but maybe through a subclass or something) but
not needed in register access callbacks that are internal to the object
and only passed already checked objects.
Regards,
BALATON Zoltan
© 2016 - 2026 Red Hat, Inc.