Calling i2c set speed twice for i2c_mux_bus

[Heiko Schocher]

Hello Michal,

Am 07.05.2020 um 10:18 schrieb Michal Simek:
> On 06. 05. 20 16:47, Simon Glass wrote:
>> Hi Michal,
>>
>> On Tue, 5 May 2020 at 21:43, Simon Glass <sjg at chromium.org> wrote:
>>>
>>> Hi Michal,
>>>
>>> On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek at xilinx.com> wrote:
>>>>
>>>> On 15. 04. 20 8:40, Michal Simek wrote:
>>>>> On 10. 04. 20 10:49, Heiko Schocher wrote:
>>>>>> Hello Michal,
>>>>>>
>>>>>> Am 10.04.2020 um 08:46 schrieb Michal Simek:
>>>>>>> Hi Heiko,
>>>>>>>
>>>>>>> On 10. 04. 20 7:11, Heiko Schocher wrote:
>>>>>>>> Hello Michal,
>>>>>>>>
>>>>>>>> Am 09.04.2020 um 16:03 schrieb Michal Simek:
>>>>>>>>> Hi Heiko and Simon,
>>>>>>>>>
>>>>>>>>> I have find out one bug in i2c class. I am using zcu104 revC board
>>>>>>>>> which
>>>>>>>>> has dts in mainline where i2c1 has i2c mux with some channels.
>>>>>>>>> In DT clock-frequency = <400000>; is specified and it is read in
>>>>>>>>> i2c_post_probe(). But i2c_mux_bus_drv is also UCLASS_I2C which means
>>>>>>>>> that post probe is called for it too. And because clock-frequency
>>>>>>>>> property is not there default 100k is used.
>>>>>>>>>
>>>>>>>>> I think that is bug and should be fixed.
>>>>>>>>> Heiko: Are you aware about this issue?
>>>>>>>>
>>>>>>>> No :-(
>>>>>>>>
>>>>>>>> The question is, is this a bug?
>>>>>>>
>>>>>>> I have never seen clock-frequency property in i2c mux bus node. Also I
>>>>>>> have looked at linux dt binding docs and nothing like that is specified
>>>>>>> there. From quick look also pca954x driver is not reading it.
>>>>>>
>>>>>> Indeed.
>>>>>>
>>>>>>>> Should a i2c bus behind a mux not be able to set his own speed?
>>>>>>>
>>>>>>> Not sure if that make sense but Linux will likely ignore it. I am not
>>>>>>> saying it doesn't make sense but I haven't seen this feature.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>> But may as a feature (or bugfix?) if "clock-frequency" is not there,
>>>>>>>> use the speed of the parent bus...?
>>>>>>>
>>>>>>> I was thinking about this too.
>>>>>>> just c&p quick implementation would look like this. Because it is
>>>>>>> i2c->i2c_mux->i2c. But maybe there is a better way how to do it.
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
>>>>>>> index 2aa3efe8aaa0..982c467deba3 100644
>>>>>>> --- a/drivers/i2c/i2c-uclass.c
>>>>>>> +++ b/drivers/i2c/i2c-uclass.c
>>>>>>> @@ -640,9 +640,21 @@ static int i2c_post_probe(struct udevice *dev)
>>>>>>>    {
>>>>>>>    #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
>>>>>>>           struct dm_i2c_bus *i2c = dev_get_uclass_priv(dev);
>>>>>>> +       int parent_speed = I2C_SPEED_STANDARD_RATE;
>>>>>>> +
>>>>>>> +       if (dev->parent &&
>>>>>>> +           device_get_uclass_id(dev->parent) == UCLASS_I2C_MUX &&
>>>>>>> +           dev->parent->parent &&
>>>>>>> +           device_get_uclass_id(dev->parent->parent) == UCLASS_I2C) {
>>>>>>> +               struct dm_i2c_bus *i2c_parent;
>>>>>>> +
>>>>>>> +               i2c_parent = dev_get_uclass_priv(dev->parent->parent);
>>>>>>> +               parent_speed = i2c_parent->speed_hz;
>>>>>>> +               /* Not sure if make sense to check that parent_speed is
>>>>>>> not 0 */
>>>>>>
>>>>>> I think this check is not needed.
>>>>>>
>>>>>>> +       }
>>>>>>>
>>>>>>>           i2c->speed_hz = dev_read_u32_default(dev, "clock-frequency",
>>>>>>> -                                            I2C_SPEED_STANDARD_RATE);
>>>>>>> +                                            parent_speed);
>>>>>>
>>>>>> Wow, a big if ... may this is clearer (not compile tested)?
>>>>>>
>>>>>> udevice *parent = dev_get_parent(dev);
>>>>>>
>>>>>> if (parent && device_get_uclass_id(parent) == UCLASS_I2C_MUX) {
>>>>>>      udevice *parent2 = dev_get_parent(parent);
>>>>>>      if (parent2 && device_get_uclass_id(parent2) == UCLASS_I2C) {
>>>>>>          struct dm_i2c_bus *i2cp = dev_get_uclass_priv(parent2);
>>>>>>
>>>>>>          parent_speed = i2cp->speed_hz;
>>>>>>      }
>>>>>> }
>>>>>>
>>>>>> but Simon has a deeper DM knowledge, may there is a better approach.
>>>>>
>>>>> Simon: any comment on this one?
>>>>
>>>> Simon: Can you please comment this?
>>>>
>>>
>>> OK will take a look.
>>
>> I wonder if i2c-mux-uclass.c should define a new uclass for muxed I2C
>> buses, something like UCLASS_I2C_MUXED_BUS? Then you can define the
>> behaviour correctly in i2c-mux-uclass.c.
>>
>> An I2C controller is not the same as a muxed bus and perhaps we should
>> be explicitly about the differences. It probably just needs changes to
>> the mux uclass.
> 
> Definitely there need to be some changes in connection to i2c muxes. I
> am aware also about bad behavior when you detect devices.
> Just look at log below and you will see that devices on base i2c bus are
> copied also to subbus (especially listing i2c-mux again looks weird).

Yes, this look like we need here a seperate uclass to handle this correct...

> Thanks,
> Michal
> 
> ZynqMP> i2c bus
> Bus 0:	i2c at ff030000  (active 0)
>     20: gpio at 20, offset len 1, flags 0
>     74: i2c-mux at 74, offset len 1, flags 0
> Bus 1:	i2c at ff030000->i2c-mux at 74->i2c at 0  (active 1)
>     54: eeprom at 54, offset len 1, flags 0
> Bus 2:	i2c at ff030000->i2c-mux at 74->i2c at 1
> Bus 3:	i2c at ff030000->i2c-mux at 74->i2c at 2
> Bus 4:	i2c at ff030000->i2c-mux at 74->i2c at 3
> Bus 5:	i2c at ff030000->i2c-mux at 74->i2c at 5
> Bus 6:	i2c at ff030000->i2c-mux at 74->i2c at 7
> ZynqMP> i2c dev 1
> Setting bus to 1
> ZynqMP> i2c probe
> Valid chip addresses: 20 54 55 56 57 74
> ZynqMP> i2c bus
> Bus 0:	i2c at ff030000  (active 0)
>     20: gpio at 20, offset len 1, flags 0
>     74: i2c-mux at 74, offset len 1, flags 0
> Bus 1:	i2c at ff030000->i2c-mux at 74->i2c at 0  (active 1)
>     54: eeprom at 54, offset len 1, flags 0
>     20: generic_20, offset len 1, flags 0
>     74: generic_74, offset len 1, flags 0
> Bus 2:	i2c at ff030000->i2c-mux at 74->i2c at 1
> Bus 3:	i2c at ff030000->i2c-mux at 74->i2c at 2
> Bus 4:	i2c at ff030000->i2c-mux at 74->i2c at 3
> Bus 5:	i2c at ff030000->i2c-mux at 74->i2c at 5
> Bus 6:	i2c at ff030000->i2c-mux at 74->i2c at 7
> ZynqMP> i2c dev 2
> Setting bus to 2
> ZynqMP> i2c probe
> Valid chip addresses: 20 6C 74
> ZynqMP> i2c bus
> Bus 0:	i2c at ff030000  (active 0)
>     20: gpio at 20, offset len 1, flags 0
>     74: i2c-mux at 74, offset len 1, flags 0
> Bus 1:	i2c at ff030000->i2c-mux at 74->i2c at 0  (active 1)
>     54: eeprom at 54, offset len 1, flags 0
>     20: generic_20, offset len 1, flags 0
>     74: generic_74, offset len 1, flags 0
> Bus 2:	i2c at ff030000->i2c-mux at 74->i2c at 1  (active 2)
>     20: generic_20, offset len 1, flags 0
>     6c: generic_6c, offset len 1, flags 0
>     74: generic_74, offset len 1, flags 0
> Bus 3:	i2c at ff030000->i2c-mux at 74->i2c at 2
> Bus 4:	i2c at ff030000->i2c-mux at 74->i2c at 3
> Bus 5:	i2c at ff030000->i2c-mux at 74->i2c at 5
> Bus 6:	i2c at ff030000->i2c-mux at 74->i2c at 7
> 

:-(

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de