Re: [PATCH v2 12/20] ppc/ppc405: QOM'ify EBC

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 9150 bytes --]

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),
>
>
>