From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Wed, 6 May 2020 08:47:22 -0600 Subject: Calling i2c set speed twice for i2c_mux_bus In-Reply-To: References: <81ef897f-4a16-5d6b-d183-f7bf03ec7aa1@xilinx.com> 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 Hi Michal, On Tue, 5 May 2020 at 21:43, Simon Glass wrote: > > Hi Michal, > > On Tue, 5 May 2020 at 02:26, Michal Simek 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