* Calling i2c set speed twice for i2c_mux_bus @ 2020-04-09 14:03 Michal Simek 2020-04-09 16:25 ` Simon Glass 2020-04-10 5:11 ` Heiko Schocher 0 siblings, 2 replies; 15+ messages in thread From: Michal Simek @ 2020-04-09 14:03 UTC (permalink / raw) To: u-boot 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? Simon: I think that make sense to use some flags to skip calling post_probe in this case. Or any suggestion how to fix it would be great. Thanks, Michal ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-04-09 14:03 Calling i2c set speed twice for i2c_mux_bus Michal Simek @ 2020-04-09 16:25 ` Simon Glass 2020-04-10 6:21 ` Michal Simek 2020-04-10 5:11 ` Heiko Schocher 1 sibling, 1 reply; 15+ messages in thread From: Simon Glass @ 2020-04-09 16:25 UTC (permalink / raw) To: u-boot Hi Michal, On Thu, 9 Apr 2020 at 08:03, Michal Simek <michal.simek@xilinx.com> wrote: > > 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? > > Simon: I think that make sense to use some flags to skip calling > post_probe in this case. Or any suggestion how to fix it would be great. Ick, hope not. Can you please point me to the board config and the device tree? Regards, Simon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-04-09 16:25 ` Simon Glass @ 2020-04-10 6:21 ` Michal Simek 0 siblings, 0 replies; 15+ messages in thread From: Michal Simek @ 2020-04-10 6:21 UTC (permalink / raw) To: u-boot Hi Simon, On 09. 04. 20 18:25, Simon Glass wrote: > Hi Michal, > > On Thu, 9 Apr 2020 at 08:03, Michal Simek <michal.simek@xilinx.com> wrote: >> >> 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? >> >> Simon: I think that make sense to use some flags to skip calling >> post_probe in this case. Or any suggestion how to fix it would be great. > > Ick, hope not. Can you please point me to the board config and the device tree? We have switch to generic config for all zynqmp boards. That's why export DEVICE_TREE=zynqmp-zcu104-revC make xilinx_zynqmp_virt_defconfig It means DT arch/arm/dts/zynqmp-zcu104-revC.dts but I expect the same behavior on zynqmp-zcu102-revA and others who have i2c mux driver. Thanks, Michal ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-04-09 14:03 Calling i2c set speed twice for i2c_mux_bus Michal Simek 2020-04-09 16:25 ` Simon Glass @ 2020-04-10 5:11 ` Heiko Schocher 2020-04-10 6:46 ` Michal Simek 1 sibling, 1 reply; 15+ messages in thread From: Heiko Schocher @ 2020-04-10 5:11 UTC (permalink / raw) To: u-boot 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? Should a i2c bus behind a mux not be able to set his own speed? But may as a feature (or bugfix?) if "clock-frequency" is not there, use the speed of the parent bus...? > Simon: I think that make sense to use some flags to skip calling > post_probe in this case. Or any suggestion how to fix it would be great. > > Thanks, > Michal > bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-04-10 5:11 ` Heiko Schocher @ 2020-04-10 6:46 ` Michal Simek 2020-04-10 8:49 ` Heiko Schocher 0 siblings, 1 reply; 15+ messages in thread From: Michal Simek @ 2020-04-10 6:46 UTC (permalink / raw) To: u-boot 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. > 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. > 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 */ + } i2c->speed_hz = dev_read_u32_default(dev, "clock-frequency", - I2C_SPEED_STANDARD_RATE); + parent_speed); return dm_i2c_set_bus_speed(dev, i2c->speed_hz); #else Thanks, Michal ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-04-10 6:46 ` Michal Simek @ 2020-04-10 8:49 ` Heiko Schocher 2020-04-15 6:40 ` Michal Simek 0 siblings, 1 reply; 15+ messages in thread From: Heiko Schocher @ 2020-04-10 8:49 UTC (permalink / raw) To: u-boot 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. > > return dm_i2c_set_bus_speed(dev, i2c->speed_hz); > #else > > Thanks, > Michal Thanks! bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-04-10 8:49 ` Heiko Schocher @ 2020-04-15 6:40 ` Michal Simek 2020-05-05 8:26 ` Michal Simek 0 siblings, 1 reply; 15+ messages in thread From: Michal Simek @ 2020-04-15 6:40 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-04-15 6:40 ` Michal Simek @ 2020-05-05 8:26 ` Michal Simek 2020-05-06 3:43 ` Simon Glass 0 siblings, 1 reply; 15+ messages in thread From: Michal Simek @ 2020-05-05 8:26 UTC (permalink / raw) To: u-boot 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? Thanks, Michal ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-05-05 8:26 ` Michal Simek @ 2020-05-06 3:43 ` Simon Glass 2020-05-06 14:47 ` Simon Glass 0 siblings, 1 reply; 15+ messages in thread From: Simon Glass @ 2020-05-06 3:43 UTC (permalink / raw) To: u-boot Hi Michal, On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek@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. Regards, Simon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-05-06 3:43 ` Simon Glass @ 2020-05-06 14:47 ` Simon Glass 2020-05-07 8:18 ` Michal Simek 0 siblings, 1 reply; 15+ messages in thread From: Simon Glass @ 2020-05-06 14:47 UTC (permalink / raw) To: u-boot Hi Michal, On Tue, 5 May 2020 at 21:43, Simon Glass <sjg@chromium.org> wrote: > > Hi Michal, > > On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek@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. Regards, Simon ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-05-06 14:47 ` Simon Glass @ 2020-05-07 8:18 ` Michal Simek 2020-05-07 10:02 ` Heiko Schocher 0 siblings, 1 reply; 15+ messages in thread From: Michal Simek @ 2020-05-07 8:18 UTC (permalink / raw) To: u-boot On 06. 05. 20 16:47, Simon Glass wrote: > Hi Michal, > > On Tue, 5 May 2020 at 21:43, Simon Glass <sjg@chromium.org> wrote: >> >> Hi Michal, >> >> On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek@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). Thanks, Michal ZynqMP> i2c bus Bus 0: i2c at ff030000 (active 0) 20: gpio at 20, offset len 1, flags 0 74: i2c-mux at 74, offset len 1, flags 0 Bus 1: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 1) 54: eeprom at 54, offset len 1, flags 0 Bus 2: i2c at ff030000->i2c-mux at 74->i2c at 1 Bus 3: i2c at ff030000->i2c-mux at 74->i2c at 2 Bus 4: i2c at ff030000->i2c-mux at 74->i2c at 3 Bus 5: i2c at ff030000->i2c-mux at 74->i2c at 5 Bus 6: i2c at ff030000->i2c-mux at 74->i2c at 7 ZynqMP> i2c dev 1 Setting bus to 1 ZynqMP> i2c probe Valid chip addresses: 20 54 55 56 57 74 ZynqMP> i2c bus Bus 0: i2c at ff030000 (active 0) 20: gpio at 20, offset len 1, flags 0 74: i2c-mux at 74, offset len 1, flags 0 Bus 1: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 1) 54: eeprom at 54, offset len 1, flags 0 20: generic_20, offset len 1, flags 0 74: generic_74, offset len 1, flags 0 Bus 2: i2c at ff030000->i2c-mux at 74->i2c at 1 Bus 3: i2c at ff030000->i2c-mux at 74->i2c at 2 Bus 4: i2c at ff030000->i2c-mux at 74->i2c at 3 Bus 5: i2c at ff030000->i2c-mux at 74->i2c at 5 Bus 6: i2c at ff030000->i2c-mux at 74->i2c at 7 ZynqMP> i2c dev 2 Setting bus to 2 ZynqMP> i2c probe Valid chip addresses: 20 6C 74 ZynqMP> i2c bus Bus 0: i2c at ff030000 (active 0) 20: gpio at 20, offset len 1, flags 0 74: i2c-mux at 74, offset len 1, flags 0 Bus 1: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 1) 54: eeprom at 54, offset len 1, flags 0 20: generic_20, offset len 1, flags 0 74: generic_74, offset len 1, flags 0 Bus 2: i2c at ff030000->i2c-mux at 74->i2c at 1 (active 2) 20: generic_20, offset len 1, flags 0 6c: generic_6c, offset len 1, flags 0 74: generic_74, offset len 1, flags 0 Bus 3: i2c at ff030000->i2c-mux at 74->i2c at 2 Bus 4: i2c at ff030000->i2c-mux at 74->i2c at 3 Bus 5: i2c at ff030000->i2c-mux at 74->i2c at 5 Bus 6: i2c at ff030000->i2c-mux at 74->i2c at 7 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-05-07 8:18 ` Michal Simek @ 2020-05-07 10:02 ` Heiko Schocher 2020-05-11 13:36 ` Michal Simek 0 siblings, 1 reply; 15+ messages in thread From: Heiko Schocher @ 2020-05-07 10:02 UTC (permalink / raw) To: u-boot 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 <sjg@chromium.org> wrote: >>> >>> Hi Michal, >>> >>> On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek@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... > Thanks, > Michal > > ZynqMP> i2c bus > Bus 0: i2c at ff030000 (active 0) > 20: gpio at 20, offset len 1, flags 0 > 74: i2c-mux at 74, offset len 1, flags 0 > Bus 1: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 1) > 54: eeprom at 54, offset len 1, flags 0 > Bus 2: i2c at ff030000->i2c-mux at 74->i2c at 1 > Bus 3: i2c at ff030000->i2c-mux at 74->i2c at 2 > Bus 4: i2c at ff030000->i2c-mux at 74->i2c at 3 > Bus 5: i2c at ff030000->i2c-mux at 74->i2c at 5 > Bus 6: i2c at ff030000->i2c-mux at 74->i2c at 7 > ZynqMP> i2c dev 1 > Setting bus to 1 > ZynqMP> i2c probe > Valid chip addresses: 20 54 55 56 57 74 > ZynqMP> i2c bus > Bus 0: i2c at ff030000 (active 0) > 20: gpio at 20, offset len 1, flags 0 > 74: i2c-mux at 74, offset len 1, flags 0 > Bus 1: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 1) > 54: eeprom at 54, offset len 1, flags 0 > 20: generic_20, offset len 1, flags 0 > 74: generic_74, offset len 1, flags 0 > Bus 2: i2c at ff030000->i2c-mux at 74->i2c at 1 > Bus 3: i2c at ff030000->i2c-mux at 74->i2c at 2 > Bus 4: i2c at ff030000->i2c-mux at 74->i2c at 3 > Bus 5: i2c at ff030000->i2c-mux at 74->i2c at 5 > Bus 6: i2c at ff030000->i2c-mux at 74->i2c at 7 > ZynqMP> i2c dev 2 > Setting bus to 2 > ZynqMP> i2c probe > Valid chip addresses: 20 6C 74 > ZynqMP> i2c bus > Bus 0: i2c at ff030000 (active 0) > 20: gpio at 20, offset len 1, flags 0 > 74: i2c-mux at 74, offset len 1, flags 0 > Bus 1: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 1) > 54: eeprom at 54, offset len 1, flags 0 > 20: generic_20, offset len 1, flags 0 > 74: generic_74, offset len 1, flags 0 > Bus 2: i2c at ff030000->i2c-mux at 74->i2c at 1 (active 2) > 20: generic_20, offset len 1, flags 0 > 6c: generic_6c, offset len 1, flags 0 > 74: generic_74, offset len 1, flags 0 > Bus 3: i2c at ff030000->i2c-mux at 74->i2c at 2 > Bus 4: i2c at ff030000->i2c-mux at 74->i2c at 3 > Bus 5: i2c at ff030000->i2c-mux at 74->i2c at 5 > Bus 6: i2c at ff030000->i2c-mux at 74->i2c at 7 > :-( bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-05-07 10:02 ` Heiko Schocher @ 2020-05-11 13:36 ` Michal Simek 2020-05-11 14:37 ` Heiko Schocher 0 siblings, 1 reply; 15+ messages in thread From: Michal Simek @ 2020-05-11 13:36 UTC (permalink / raw) To: u-boot 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 <sjg@chromium.org> wrote: >>>> >>>> Hi Michal, >>>> >>>> On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek@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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-05-11 13:36 ` Michal Simek @ 2020-05-11 14:37 ` Heiko Schocher 2020-05-11 20:28 ` Simon Glass 0 siblings, 1 reply; 15+ messages in thread From: Heiko Schocher @ 2020-05-11 14:37 UTC (permalink / raw) To: u-boot 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 <sjg@chromium.org> wrote: >>>>> >>>>> Hi Michal, >>>>> >>>>> On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek@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... bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de ^ permalink raw reply [flat|nested] 15+ messages in thread
* Calling i2c set speed twice for i2c_mux_bus 2020-05-11 14:37 ` Heiko Schocher @ 2020-05-11 20:28 ` Simon Glass 0 siblings, 0 replies; 15+ messages in thread From: Simon Glass @ 2020-05-11 20:28 UTC (permalink / raw) To: u-boot Hi, On Mon, 11 May 2020 at 08:37, Heiko Schocher <hs@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 <sjg@chromium.org> wrote: > >>>>> > >>>>> Hi Michal, > >>>>> > >>>>> On Tue, 5 May 2020 at 02:26, Michal Simek <michal.simek@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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-05-11 20:28 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-09 14:03 Calling i2c set speed twice for i2c_mux_bus Michal Simek 2020-04-09 16:25 ` Simon Glass 2020-04-10 6:21 ` Michal Simek 2020-04-10 5:11 ` Heiko Schocher 2020-04-10 6:46 ` Michal Simek 2020-04-10 8:49 ` Heiko Schocher 2020-04-15 6:40 ` Michal Simek 2020-05-05 8:26 ` Michal Simek 2020-05-06 3:43 ` Simon Glass 2020-05-06 14:47 ` Simon Glass 2020-05-07 8:18 ` Michal Simek 2020-05-07 10:02 ` Heiko Schocher 2020-05-11 13:36 ` Michal Simek 2020-05-11 14:37 ` Heiko Schocher 2020-05-11 20:28 ` Simon Glass
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.