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

From: "Cédric Le Goater" <clg@kaod.org>
To: BALATON Zoltan <balaton@eik.bme.hu>
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>,
	"list@suse.de:PowerPC" <qemu-ppc@nongnu.org>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH v2 12/20] ppc/ppc405: QOM'ify EBC
Date: Thu, 4 Aug 2022 18:31:08 +0200	[thread overview]
Message-ID: <7ecefd72-b799-8a8c-51fd-28730a12ebf1@kaod.org> (raw)
In-Reply-To: <1e6be2f3-4c7a-2432-5034-fa012c662df@eik.bme.hu>

[ Replying to all ]

On 8/4/22 16:26, BALATON Zoltan wrote:
> On Thu, 4 Aug 2022, Cédric Le Goater wrote:
>> 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.
> 
> OK, I did not mean to impose too much additional work but if a legacy init function only has one caller left maybe it's a good time to get rid of it at the same time when you converted the other caller. Those which have more users left could be addressed later if it would be too much work.
> 
>>> 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.
> 
> Maybe that's a more complete solution but I think we could make it simpler even without going there yet.
> 
>>> 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.
> 
> Is this really necessary? Why do we have a qom-tree if objects can't navigate it? I think it should be possible to get to the cpu object from these soc devices without needing a link property for that. Ideally it should be able to get "../cpu" or shouldn't something like object_resolve_path_type("/machine", TYPE_POWERPC_CPU, NULL) return the cpu object? 

Yes. We could. I find it cleaner to use a property.

It is true that "cpu" is only used at realize time to install the DCR
handlers in the DCR table of the CPU. So there is not much value in
keeping a pointer to the CPU under the device state struct. Once the
DCR handlers are installed. We are done.

> (Assiming there's only one which is true for these SoCs.) Then you could do this in the realize method and store the cpu in the state struct of the soc device 

The CPU is already under the 405 SoC device, or you mean any SoC device ?
I am not sure I am following. Send a code example !

Thanks,

C.

> so it does not have to be looked up every time or set externally via a property? That way no cpu link property is needed for all these soc devices only that they are added to a soc that already has a cpu added before.
>
> 
> We do have the parent field in the Object struct so you could theoretically just do object_resolve_path_at(obj->parent, "cpu") but all those fields are marked private and I could not find out what's the preferred way to get to this then. Could somebody with more knowledge about QOM chime in please?
> 
> Regards,
> BALATON Zoltan