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