Re: [PATCH v2 19/20] ppc/ppc405: QOM'ify I2C

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

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

On Thu, 4 Aug 2022, Cédric Le Goater wrote:
> On 8/4/22 01:31, BALATON Zoltan wrote:
>> On Wed, 3 Aug 2022, Cédric Le Goater wrote:
>>> Having an explicit I2C model object will help if one day we want to
>>> add I2C devices on the bus.
>> 
>> Same here as with the UIC in previous patch, it's not QOMifying here 
>> either. As for why we may need I2C, on sam460ex the firmware detects RAM 
>> accessing the SPD data over I2C so that could be the reason but it may not 
>> be used here on 405.
>
> You can still plug I2C devices on the PPC405 command line if you want to.

Yes true, I just remembered the reson to bother to implement I2C on 
sam460ex was that the firmware would not work without it so I thought it 
may be the same reason here but we don't have the firmware for this board 
so I don't know. Maybe the original goal was that or the firmware was used 
for testing but then it was unfinished. Anyway, having more accurate 
emulation of the hardware is always good so if this matches real hardware 
then it should stay.

Regards,
BALATON Zoltan

>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>> hw/ppc/ppc405.h    |  2 ++
>>> hw/ppc/ppc405_uc.c | 10 ++++++++--
>>> 2 files changed, 10 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
>>> index d29f738cd2d0..d13624ae309c 100644
>>> --- a/hw/ppc/ppc405.h
>>> +++ b/hw/ppc/ppc405.h
>>> @@ -28,6 +28,7 @@
>>> #include "qom/object.h"
>>> #include "hw/ppc/ppc4xx.h"
>>> #include "hw/intc/ppc-uic.h"
>>> +#include "hw/i2c/ppc4xx_i2c.h"
>>> 
>>> #define PPC405EP_SDRAM_BASE 0x00000000
>>> #define PPC405EP_NVRAM_BASE 0xF0000000
>>> @@ -256,6 +257,7 @@ struct Ppc405SoCState {
>>>     Ppc405OcmState ocm;
>>>     Ppc405GpioState gpio;
>>>     Ppc405DmaState dma;
>>> +    PPC4xxI2CState i2c;
>>>     Ppc405EbcState ebc;
>>>     Ppc405OpbaState opba;
>>>     Ppc405PobState pob;
>>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>>> index 5cd32e22b7ea..8f0caa45f5f7 100644
>>> --- a/hw/ppc/ppc405_uc.c
>>> +++ b/hw/ppc/ppc405_uc.c
>>> @@ -1461,6 +1461,8 @@ static void ppc405_soc_instance_init(Object *obj)
>>> 
>>>     object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
>>> 
>>> +    object_initialize_child(obj, "i2c", &s->i2c, TYPE_PPC4xx_I2C);
>>> +
>>>     object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
>>> 
>>>     object_initialize_child(obj, "opba", &s->opba, TYPE_PPC405_OPBA);
>>> @@ -1569,8 +1571,12 @@ static void ppc405_soc_realize(DeviceState *dev, 
>>> Error **errp)
>>>     }
>>> 
>>>     /* I2C controller */
>>> -    sysbus_create_simple(TYPE_PPC4xx_I2C, 0xef600500,
>>> -                         qdev_get_gpio_in(DEVICE(&s->uic), 2));
>>> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->i2c), errp)) {
>>> +        return;
>>> +    }
>>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c), 0, 0xef600500);
>>> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c), 0,
>>> +                       qdev_get_gpio_in(DEVICE(&s->uic), 2));
>>> 
>>>     /* GPIO */
>>>     if (!sysbus_realize(SYS_BUS_DEVICE(&s->gpio), errp)) {
>>> 
>
>
>