Calling i2c set speed twice for i2c_mux_bus

[Simon Glass <sjg@chromium.org>]

Hi,

On Mon, 11 May 2020 at 08:37, Heiko Schocher <hs@denx.de> wrote:
>
> 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@chromium.org> wrote:
> >>>>>
> >>>>> Hi Michal,
> >>>>>
> >>>>> On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek@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...

I think this should be implemented in sandbox as it is much faster / simpler.

Regards,
SImon