Calling i2c set speed twice for i2c_mux_bus

[Simon Glass]

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.

Regards,
Simon