Calling i2c set speed twice for i2c_mux_bus

[Heiko Schocher]

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.

> 
>          return dm_i2c_set_bus_speed(dev, i2c->speed_hz);
>   #else
> 
> Thanks,
> Michal

Thanks!

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