From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Mon, 11 May 2020 14:28:51 -0600 Subject: Calling i2c set speed twice for i2c_mux_bus In-Reply-To: <7509de6f-b1c2-884a-47dd-19a2aca89ea5@denx.de> References: <81ef897f-4a16-5d6b-d183-f7bf03ec7aa1@xilinx.com> <745534e4-979e-0626-e63e-ef5452eaf003@xilinx.com> <98b0e61c-2f69-1ccc-feb6-71a66e81601f@denx.de> <0b79a01d-0a09-48a3-891c-c266be7c3497@xilinx.com> <7509de6f-b1c2-884a-47dd-19a2aca89ea5@denx.de> 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, On Mon, 11 May 2020 at 08:37, Heiko Schocher 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 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. > >>> > >>> 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