diff mbox series

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

Message ID 20220803132844.2370514-20-clg@kaod.org
State Changes Requested
Headers show
Series ppc: QOM'ify 405 board | expand

Commit Message

Cédric Le Goater Aug. 3, 2022, 1:28 p.m. UTC 
Having an explicit I2C model object will help if one day we want to
add I2C devices on the bus.

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

Comments

BALATON Zoltan Aug. 3, 2022, 11:31 p.m. UTC | #1 
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.

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)) {
>
Cédric Le Goater Aug. 4, 2022, 5:42 a.m. UTC | #2 
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.

Thanks,

C.

> 
> 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)) {
>>
BALATON Zoltan Aug. 4, 2022, 11:21 a.m. UTC | #3 
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)) {
>>> 
>
>
>
Cédric Le Goater Aug. 4, 2022, 2:14 p.m. UTC | #4 
On 8/4/22 13:21, BALATON Zoltan wrote:
> 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.

Adding I2C devices works fine. I also have a hotfoot machine variant
with PCI and network but that's for later.

Thanks,

c.

> 
> 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)) {
>>>>
>>
>>
>>
diff mbox series

Patch

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