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? Thanks, Michal