Calling i2c set speed twice for i2c_mux_bus

[Heiko Schocher]

Hello Michal,

Am 11.05.2020 um 15:36 schrieb Michal Simek:
> On 07. 05. 20 12:02, Heiko Schocher wrote:
>> 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...
> 
> Are you going to invest to your time to create?

Unfortunately I have no time to look into this soon, and I must search
for a hardware to test...

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