Re: [Qemu-devel] [PATCH v2 1/6] arm: Uniquely name imx25 I2C buses.

[Alexey Kardashevskiy]

On 02/12/16 18:55, Cédric Le Goater wrote:
> On 12/02/2016 12:34 AM, Alexey Kardashevskiy wrote:
>> On 01/12/16 23:31, Cédric Le Goater wrote:
>>> On 12/01/2016 01:42 AM, Alastair D'Silva wrote:
>>>> On Wed, 2016-11-30 at 09:18 +0100, Cédric Le Goater wrote:
>>>>> On 11/30/2016 06:36 AM, Alastair D'Silva wrote:
>>>>>> From: Alastair D'Silva <address@hidden>
>>>>>>
>>>>>> The imx25 chip provides 3 i2c buses, but they have all been named
>>>>>> "i2c", which makes it difficult to predict which bus a device will
>>>>>> be connected to when specified on the command line.
>>>>>>
>>>>>> This patch addresses the issue by naming the buses uniquely:
>>>>>>   i2c.0 i2c.1 i2c.2
>>>>>>
>>>>>> Signed-off-by: Alastair D'Silva <address@hidden>
>>>>>> ---
>>>>>>  hw/arm/imx25_pdk.c | 4 +---
>>>>>>  hw/i2c/imx_i2c.c   | 6 +++++-
>>>>>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
>>>>>> index 025b608..c6f04d3 100644
>>>>>> --- a/hw/arm/imx25_pdk.c
>>>>>> +++ b/hw/arm/imx25_pdk.c
>>>>>> @@ -138,9 +138,7 @@ static void imx25_pdk_init(MachineState
>>>>>> *machine)
>>>>>>           * We add it here (only on qtest usage) to be able to do a
>>>>>> bit
>>>>>>           * of simple qtest. See "make check" for details.
>>>>>>           */
>>>>>> -        i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s-
>>>>>>> soc.i2c[0]),
>>>>>> -                                                      "i2c"),
>>>>>> -                         "ds1338", 0x68);
>>>>>> +        i2c_create_slave(s->soc.i2c[0].bus, "ds1338", 0x68);
>>>>>>      }
>>>>>>  }
>>>>>>  
>>>>>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
>>>>>> index 37e5a62..7be10fb 100644
>>>>>> --- a/hw/i2c/imx_i2c.c
>>>>>> +++ b/hw/i2c/imx_i2c.c
>>>>>> @@ -305,12 +305,16 @@ static const VMStateDescription
>>>>>> imx_i2c_vmstate = {
>>>>>>  static void imx_i2c_realize(DeviceState *dev, Error **errp)
>>>>>>  {
>>>>>>      IMXI2CState *s = IMX_I2C(dev);
>>>>>> +    static int bus_count;
>>>>>
>>>>> hmm, the static is ugly :/ 
>>>>>
>>>>> Isn't there other ways to achieve this naming ? 
>>>>>
>>>>> Thanks,
>>>>>
>>>>> C.  
>>>>>
>>>>
>>>> I'm not seeing an obvious way around it. The busses are realized
>>>> independently (so I can't implement what we do with the aspeed i2c
>>>> busses), and it is named before fsl-imx25:fsl_imx25_realize() can apply
>>>> specific properties to the bus.
>>>>
>>>> If you have any suggestions, I'm all ears.
>>>
>>> What about that ? 
>>>
>>>     @@ -310,7 +310,7 @@ static void imx_i2c_realize(DeviceState
>>>                                IMX_I2C_MEM_SIZE);
>>>          sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>>>          sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>>>     -    s->bus = i2c_init_bus(DEVICE(dev), "i2c");
>>>     +    s->bus = i2c_init_bus(DEVICE(dev), NULL);
>>>      }
>>>  
>>>  static void imx_i2c_class_init(ObjectClass *klass, void *data)
>>>
>>> Which should name automatically the I2C objects :
>>
>>
>> If you ever do migration, you'll have to specify "id" in the command line
>> anyway. Even in the example below the buses are numbered in messed order,
>> is that desired effect (may be it is, just asking :) )?
> 
> That's how it comes out with qom-tree. I haven't dug deeper to see 
> how this was implemented.

That's fine, your approach will give unique names, it is just hard to
predict what i2c device will end up connected to which i2c bus, and I
usually want to control this instead of guessing (which will work till some
nonobvious change in QOM :) ).


> 
> C.
> 
>>>
>>>     (qemu) info qom-tree 
>>>     /machine (imx25-pdk-machine)
>>>       /peripheral (container)
>>>       /soc (fsl,imx25)
>>>       /peripheral-anon (container)
>>>       /unattached (container)
>>>         /device[0] (arm926-arm-cpu)
>>>           /unnamed-gpio-in[1] (irq)
>>>           /unnamed-gpio-in[3] (irq)
>>>           /unnamed-gpio-in[2] (irq)
>>>           /unnamed-gpio-in[0] (irq)
>>>
>>>         /device[15] (imx.i2c)
>>>           /imx.i2c[0] (qemu:memory-region)
>>>           /i2c-bus.0 (i2c-bus)
>>>         /device[17] (imx.i2c)
>>>           /imx.i2c[0] (qemu:memory-region)
>>>           /i2c-bus.2 (i2c-bus)
>>>         /device[16] (imx.i2c)
>>>           /imx.i2c[0] (qemu:memory-region)
>>>           /i2c-bus.1 (i2c-bus)
>>>        ....
>>>
>>>
>>> Cheers,
>>>
>>> C. 
>>>
>>
>>
> 


-- 
Alexey