Re: [Qemu-devel] [PATCH 12/17] e500: add i2c controller to CCSR

[Michael Davidsaver]

On 12/05/2017 01:49 AM, David Gibson wrote:
> On Sun, Nov 26, 2017 at 03:59:10PM -0600, Michael Davidsaver wrote:
>> Add i2c controller found on mpc8540,
>> mpc8544, and P2010 (newer ppc, unmodeled).
> 
> This adds it unconditionally.  Are there any E500 models where it
> doesn't exist?

Not in the devices I've looked at, though I certainly haven't looked
at them all.  In fact the mpc8544 has two i2c controllers.

I keep mentioning the P2010 as it is about a decade
newer than the mpc85xx chips.  My _assumption_ is that the commonalities
between these are a reasonable least common denominator.


>>
>> Signed-off-by: Michael Davidsaver 
>> ---
>>  hw/ppc/e500_ccsr.c | 15 +++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/ppc/e500_ccsr.c b/hw/ppc/e500_ccsr.c
>> index c479ed91ee..cd8216daaf 100644
>> --- a/hw/ppc/e500_ccsr.c
>> +++ b/hw/ppc/e500_ccsr.c
>> @@ -46,6 +46,8 @@
>>  #define E500_ERR_DETECT  (0x2e40)
>>  #define E500_ERR_DISABLE (0x2e44)
>>  
>> +#define E500_I2C_OFFSET  (0x3000)
>> +
>>  #define E500_DUART_OFFSET(N) (0x4500 + (N) * 0x100)
>>  
>>  #define E500_PORPLLSR(0xE)
>> @@ -72,6 +74,7 @@ typedef struct {
>>  uint32_t ccb_freq;
>>  
>>  DeviceState *pic;
>> +DeviceState *i2c;
>>  } CCSRState;
>>  
>>  #define TYPE_E500_CCSR "e500-ccsr"
>> @@ -272,6 +275,18 @@ static void e500_ccsr_realize(DeviceState *dev, Error 
>> **errp)
>>  sysbus_mmio_get_region(pic, 0));
>>  /* Note: MPIC internal interrupts are offset by 16 */
>>  
>> +/* attach I2C controller */
>> +ccsr->i2c = qdev_create(NULL, "mpc8540-i2c");
> 
> Ah.. so I think I missed this on many earlier patches.  I believe
> you're not generally supposed to create new subdevices at realize()
> time.  Instead the device should be created at CCSR instance_init()
> time (but the sub device's "realized" property will need to be set to
> 1 from CCSR realize()).
> 
>> +object_property_add_child(qdev_get_machine(), "i2c[*]",
>> +  OBJECT(ccsr->i2c), NULL);
>> +qdev_init_nofail(ccsr->i2c);
>> +memory_region_add_subregion(>iomem, E500_I2C_OFFSET,
>> +sysbus_mmio_get_region(
>> +SYS_BUS_DEVICE(ccsr->i2c), 0));
>> +sysbus_connect_irq(SYS_BUS_DEVICE(ccsr->i2c), 0,
>> +  qdev_get_gpio_in(ccsr->pic, 16 + 27));
>> +
>> +
>>  /* DUARTS */
>>  /* for mpc8540, mpc8544, and P2010 (unmodeled), the DUART reference 
>> clock
>>   * is the CCB clock divided by 16.
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 12/17] e500: add i2c controller to CCSR

[David Gibson]

On Sun, Nov 26, 2017 at 03:59:10PM -0600, Michael Davidsaver wrote:
> Add i2c controller found on mpc8540,
> mpc8544, and P2010 (newer ppc, unmodeled).

This adds it unconditionally.  Are there any E500 models where it
doesn't exist?

> 
> Signed-off-by: Michael Davidsaver 
> ---
>  hw/ppc/e500_ccsr.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/ppc/e500_ccsr.c b/hw/ppc/e500_ccsr.c
> index c479ed91ee..cd8216daaf 100644
> --- a/hw/ppc/e500_ccsr.c
> +++ b/hw/ppc/e500_ccsr.c
> @@ -46,6 +46,8 @@
>  #define E500_ERR_DETECT  (0x2e40)
>  #define E500_ERR_DISABLE (0x2e44)
>  
> +#define E500_I2C_OFFSET  (0x3000)
> +
>  #define E500_DUART_OFFSET(N) (0x4500 + (N) * 0x100)
>  
>  #define E500_PORPLLSR(0xE)
> @@ -72,6 +74,7 @@ typedef struct {
>  uint32_t ccb_freq;
>  
>  DeviceState *pic;
> +DeviceState *i2c;
>  } CCSRState;
>  
>  #define TYPE_E500_CCSR "e500-ccsr"
> @@ -272,6 +275,18 @@ static void e500_ccsr_realize(DeviceState *dev, Error 
> **errp)
>  sysbus_mmio_get_region(pic, 0));
>  /* Note: MPIC internal interrupts are offset by 16 */
>  
> +/* attach I2C controller */
> +ccsr->i2c = qdev_create(NULL, "mpc8540-i2c");

Ah.. so I think I missed this on many earlier patches.  I believe
you're not generally supposed to create new subdevices at realize()
time.  Instead the device should be created at CCSR instance_init()
time (but the sub device's "realized" property will need to be set to
1 from CCSR realize()).

> +object_property_add_child(qdev_get_machine(), "i2c[*]",
> +  OBJECT(ccsr->i2c), NULL);
> +qdev_init_nofail(ccsr->i2c);
> +memory_region_add_subregion(>iomem, E500_I2C_OFFSET,
> +sysbus_mmio_get_region(
> +SYS_BUS_DEVICE(ccsr->i2c), 0));
> +sysbus_connect_irq(SYS_BUS_DEVICE(ccsr->i2c), 0,
> +  qdev_get_gpio_in(ccsr->pic, 16 + 27));
> +
> +
>  /* DUARTS */
>  /* for mpc8540, mpc8544, and P2010 (unmodeled), the DUART reference clock
>   * is the CCB clock divided by 16.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 12/17] e500: add i2c controller to CCSR

[Michael Davidsaver]

Add i2c controller found on mpc8540,
mpc8544, and P2010 (newer ppc, unmodeled).

Signed-off-by: Michael Davidsaver 
---
 hw/ppc/e500_ccsr.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/hw/ppc/e500_ccsr.c b/hw/ppc/e500_ccsr.c
index c479ed91ee..cd8216daaf 100644
--- a/hw/ppc/e500_ccsr.c
+++ b/hw/ppc/e500_ccsr.c
@@ -46,6 +46,8 @@
 #define E500_ERR_DETECT  (0x2e40)
 #define E500_ERR_DISABLE (0x2e44)
 
+#define E500_I2C_OFFSET  (0x3000)
+
 #define E500_DUART_OFFSET(N) (0x4500 + (N) * 0x100)
 
 #define E500_PORPLLSR(0xE)
@@ -72,6 +74,7 @@ typedef struct {
 uint32_t ccb_freq;
 
 DeviceState *pic;
+DeviceState *i2c;
 } CCSRState;
 
 #define TYPE_E500_CCSR "e500-ccsr"
@@ -272,6 +275,18 @@ static void e500_ccsr_realize(DeviceState *dev, Error 
**errp)
 sysbus_mmio_get_region(pic, 0));
 /* Note: MPIC internal interrupts are offset by 16 */
 
+/* attach I2C controller */
+ccsr->i2c = qdev_create(NULL, "mpc8540-i2c");
+object_property_add_child(qdev_get_machine(), "i2c[*]",
+  OBJECT(ccsr->i2c), NULL);
+qdev_init_nofail(ccsr->i2c);
+memory_region_add_subregion(>iomem, E500_I2C_OFFSET,
+sysbus_mmio_get_region(
+SYS_BUS_DEVICE(ccsr->i2c), 0));
+sysbus_connect_irq(SYS_BUS_DEVICE(ccsr->i2c), 0,
+  qdev_get_gpio_in(ccsr->pic, 16 + 27));
+
+
 /* DUARTS */
 /* for mpc8540, mpc8544, and P2010 (unmodeled), the DUART reference clock
  * is the CCB clock divided by 16.
-- 
2.11.0