From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Simek Date: Wed, 15 Apr 2020 08:40:09 +0200 Subject: Calling i2c set speed twice for i2c_mux_bus In-Reply-To: References: Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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? Thanks, Michal