| Message ID | 20190929103638.46038-1-biwen.li@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: pca954x: Add property to skip disabling PCA954x MUX device | expand |
Hello Biwen, > + /* Errata ID E-00013 on board LS2088ARDB and LS2088ARDB: > + * The point here is that you must not disable a mux if there > + * are no pullups on the input or you mess up the I2C. This > + * needs to be put into the DTS really as the kernel cannot > + * know this otherwise. > + */ Can you please explain what a "mess up" is? And also, should we put this new DTS property in related default bindings? If not, are you planning a documentation update for the users to notify them about this?
On 2019-09-29 12:36, Biwen Li wrote: > On some Layerscape boards like LS2085ARDB and LS2088ARDB, > input pull-up resistors on PCA954x MUX device are missing on board, > So, if MUX are disabled after powered-on, input lines will float > leading to incorrect functionality. Hi! Are you saying that the parent bus of the mux is relying on some pull-ups inside the mux? > Hence, PCA954x MUX device should never be turned-off after > power-on. > > Add property to skip disabling PCA954x MUX device > if device tree contains "i2c-mux-never-disable" > for PCA954x device node. > > Errata ID: E-00013 on board LS2085ARDB and LS2088ARDB > (Board revision found on Rev.B, Rev.C and Rev.D) I think you should follow the example of the i2c-mux-gpio driver and implement the idle-state property instead. That is a lot more consistent, assuming it solves the problem at hand? > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > --- > drivers/i2c/muxes/i2c-mux-pca954x.c | 33 +++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c > index 923aa3a5a3dc..ea8aca54d572 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > @@ -93,6 +93,7 @@ struct pca954x { > struct irq_domain *irq; > unsigned int irq_mask; > raw_spinlock_t lock; > + u8 disable_mux; /* do not disable mux if val not 0 */ Awful number of negations there. The name is also backwards given that a non-zero value means that the mux should *not* be disabled. I would have reused the name from the binding. bool never_disable; A bit less confusing... > }; > > /* Provide specs for the PCA954x types we know about */ > @@ -258,6 +259,11 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan) > struct i2c_client *client = data->client; > s8 idle_state; > > + if (data->disable_mux != 0) { Please drop " != 0" and use the variable as a truth value. More instances below... > + data->last_chan = data->chip->nchans; > + return pca954x_reg_write(muxc->parent, client, data->disable_mux); > + } > + > idle_state = READ_ONCE(data->idle_state); > if (idle_state >= 0) > /* Set the mux back to a predetermined channel */ > @@ -462,16 +468,32 @@ static int pca954x_probe(struct i2c_client *client, > } > } > > + /* Errata ID E-00013 on board LS2088ARDB and LS2088ARDB: > + * The point here is that you must not disable a mux if there > + * are no pullups on the input or you mess up the I2C. This > + * needs to be put into the DTS really as the kernel cannot > + * know this otherwise. > + */ > + > + data->disable_mux = np && > + of_property_read_bool(np, "i2c-mux-never-disable") && i2c-mux-never-disable needs to be documented. However see above for my point that you should do it like the i2c-mux-gpio driver. Any usage of idle-state still needs to be documented for pca954x binding. > + data->chip->muxtype == pca954x_ismux ? > + data->chip->enable : 0; Why do you not allow never-disable for switches? Cheers, Peter > + > /* Write the mux register at addr to verify > * that the mux is in fact present. This also > * initializes the mux to disconnected state. > */ > - if (i2c_smbus_write_byte(client, 0) < 0) { > + if (i2c_smbus_write_byte(client, data->disable_mux) < 0) { > dev_warn(dev, "probe failed\n"); > return -ENODEV; > } > > - data->last_chan = 0; /* force the first selection */ > + if (data->disable_mux != 0) > + data->last_chan = data->chip->nchans; > + else > + data->last_chan = 0; /* force the first selection */ > + > data->idle_state = MUX_IDLE_AS_IS; > > idle_disconnect_dt = np && > @@ -531,8 +553,11 @@ static int pca954x_resume(struct device *dev) > struct i2c_mux_core *muxc = i2c_get_clientdata(client); > struct pca954x *data = i2c_mux_priv(muxc); > > - data->last_chan = 0; > - return i2c_smbus_write_byte(client, 0); > + if (data->disable_mux != 0) > + data->last_chan = data->chip->nchans; > + else > + data->last_chan = 0; > + return i2c_smbus_write_byte(client, data->disable_mux); > } > #endif > >
> > On 2019-09-29 12:36, Biwen Li wrote: > > On some Layerscape boards like LS2085ARDB and LS2088ARDB, input > > pull-up resistors on PCA954x MUX device are missing on board, So, if > > MUX are disabled after powered-on, input lines will float leading to > > incorrect functionality. > > Hi! > > Are you saying that the parent bus of the mux is relying on some pull-ups inside > the mux? Yes, as follows: VCC ------- |------------------- | | \ \ /10K resister / 10k resister \ \ | | | | I2C1_SCL ------------------------ I2C1_SCL | ---------------------- --------------------|SCL | ----------------------------------------|SCL | I2C1_SDA | PCA9547 | I2C1_SDA | | PCA9547 | --------------------|SDA | ----------------------------|-----------|SDA | ------------------------ ---------------------- --wrong design(need software fix as above or hardware fix)-- --proper design-- > > > Hence, PCA954x MUX device should never be turned-off after power-on. > > > > Add property to skip disabling PCA954x MUX device if device tree > > contains "i2c-mux-never-disable" > > for PCA954x device node. > > > > Errata ID: E-00013 on board LS2085ARDB and LS2088ARDB (Board revision > > found on Rev.B, Rev.C and Rev.D) > > I think you should follow the example of the i2c-mux-gpio driver and implement > the idle-state property instead. > > That is a lot more consistent, assuming it solves the problem at hand? Got it, thanks, I will try it. > > > > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > > --- > > drivers/i2c/muxes/i2c-mux-pca954x.c | 33 > > +++++++++++++++++++++++++---- > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c > > b/drivers/i2c/muxes/i2c-mux-pca954x.c > > index 923aa3a5a3dc..ea8aca54d572 100644 > > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > > @@ -93,6 +93,7 @@ struct pca954x { > > struct irq_domain *irq; > > unsigned int irq_mask; > > raw_spinlock_t lock; > > + u8 disable_mux; /* do not disable mux if val not 0 */ > > Awful number of negations there. The name is also backwards given that a > non-zero value means that the mux should *not* be disabled. I would have > reused the name from the binding. > > bool never_disable; > > A bit less confusing... Got it,thanks, I will let it clear in v2. > > > }; > > > > /* Provide specs for the PCA954x types we know about */ @@ -258,6 > > +259,11 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, > u32 chan) > > struct i2c_client *client = data->client; > > s8 idle_state; > > > > + if (data->disable_mux != 0) { > > Please drop " != 0" and use the variable as a truth value. More instances below... Got it, I will correct it in v2. > > > + data->last_chan = data->chip->nchans; > > + return pca954x_reg_write(muxc->parent, client, > data->disable_mux); > > + } > > + > > idle_state = READ_ONCE(data->idle_state); > > if (idle_state >= 0) > > /* Set the mux back to a predetermined channel */ @@ > > -462,16 +468,32 @@ static int pca954x_probe(struct i2c_client *client, > > } > > } > > > > + /* Errata ID E-00013 on board LS2088ARDB and LS2088ARDB: > > + * The point here is that you must not disable a mux if there > > + * are no pullups on the input or you mess up the I2C. This > > + * needs to be put into the DTS really as the kernel cannot > > + * know this otherwise. > > + */ > > + > > + data->disable_mux = np && > > + of_property_read_bool(np, "i2c-mux-never-disable") && > > i2c-mux-never-disable needs to be documented. However see above for my > point that you should do it like the i2c-mux-gpio driver. Any usage of idle-state > still needs to be documented for pca954x binding. Got it, I will update the document in v2. > > > + data->chip->muxtype == pca954x_ismux ? > > + data->chip->enable : 0; > > Why do you not allow never-disable for switches? Currently, the hardware bug is on pca9547(LS2085ARDB and LS2088ARDB not populate resistors for pca9547), muxtype of pca9547 is 'pca954x_ismux' > > Cheers, > Peter > > > + > > /* Write the mux register at addr to verify > > * that the mux is in fact present. This also > > * initializes the mux to disconnected state. > > */ > > - if (i2c_smbus_write_byte(client, 0) < 0) { > > + if (i2c_smbus_write_byte(client, data->disable_mux) < 0) { > > dev_warn(dev, "probe failed\n"); > > return -ENODEV; > > } > > > > - data->last_chan = 0; /* force the first selection */ > > + if (data->disable_mux != 0) > > + data->last_chan = data->chip->nchans; > > + else > > + data->last_chan = 0; /* force the first > selection */ > > + > > data->idle_state = MUX_IDLE_AS_IS; > > > > idle_disconnect_dt = np && > > @@ -531,8 +553,11 @@ static int pca954x_resume(struct device *dev) > > struct i2c_mux_core *muxc = i2c_get_clientdata(client); > > struct pca954x *data = i2c_mux_priv(muxc); > > > > - data->last_chan = 0; > > - return i2c_smbus_write_byte(client, 0); > > + if (data->disable_mux != 0) > > + data->last_chan = data->chip->nchans; > > + else > > + data->last_chan = 0; > > + return i2c_smbus_write_byte(client, data->disable_mux); > > } > > #endif > > > >
> > Hello Biwen, > > > + /* Errata ID E-00013 on board LS2088ARDB and LS2088ARDB: > > + * The point here is that you must not disable a mux if there > > + * are no pullups on the input or you mess up the I2C. This > > + * needs to be put into the DTS really as the kernel cannot > > + * know this otherwise. > > + */ > > Can you please explain what a "mess up" is? This is a hardware bug that happened on NXP board LS2085ARDB and LS2088ARDB. So give a software fix for the hardware bug. > > And also, should we put this new DTS property in related default bindings? > > If not, are you planning a documentation update for the users to notify them > about this? I will update bindings document on v2. > > -- > Cengiz Can <cengiz@kernel.wtf>
On 2019-09-30 04:43, Biwen Li wrote: >> >> On 2019-09-29 12:36, Biwen Li wrote: >>> On some Layerscape boards like LS2085ARDB and LS2088ARDB, input >>> pull-up resistors on PCA954x MUX device are missing on board, So, if >>> MUX are disabled after powered-on, input lines will float leading to >>> incorrect functionality. SDA and SCL are not "inputs". They are part of a bus and are both bidirectional signals. Speaking of input signals in an I2C context is ambiguous. >> >> Hi! >> >> Are you saying that the parent bus of the mux is relying on some pull-ups inside >> the mux? > Yes, as follows: > VCC > ------- > |------------------- > | | > \ \ > /10K resister / 10k resister > \ \ > | | > | | > I2C1_SCL ------------------------ I2C1_SCL | ---------------------- > --------------------|SCL | ----------------------------------------|SCL | > I2C1_SDA | PCA9547 | I2C1_SDA | | PCA9547 | > --------------------|SDA | ----------------------------|-----------|SDA | > ------------------------ ---------------------- > --wrong design(need software fix as above or hardware fix)-- --proper design-- Ok, got it (but the "picture" didn't help). >> >>> Hence, PCA954x MUX device should never be turned-off after power-on. >>> >>> Add property to skip disabling PCA954x MUX device if device tree >>> contains "i2c-mux-never-disable" >>> for PCA954x device node. >>> >>> Errata ID: E-00013 on board LS2085ARDB and LS2088ARDB (Board revision >>> found on Rev.B, Rev.C and Rev.D) >> >> I think you should follow the example of the i2c-mux-gpio driver and implement >> the idle-state property instead. >> >> That is a lot more consistent, assuming it solves the problem at hand? > Got it, thanks, I will try it. I'll wait for that patch then, instead of spending more energy on the never-disable approach. Cheers, Peter
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c index 923aa3a5a3dc..ea8aca54d572 100644 --- a/drivers/i2c/muxes/i2c-mux-pca954x.c +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c @@ -93,6 +93,7 @@ struct pca954x { struct irq_domain *irq; unsigned int irq_mask; raw_spinlock_t lock; + u8 disable_mux; /* do not disable mux if val not 0 */ }; /* Provide specs for the PCA954x types we know about */ @@ -258,6 +259,11 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan) struct i2c_client *client = data->client; s8 idle_state; + if (data->disable_mux != 0) { + data->last_chan = data->chip->nchans; + return pca954x_reg_write(muxc->parent, client, data->disable_mux); + } + idle_state = READ_ONCE(data->idle_state); if (idle_state >= 0) /* Set the mux back to a predetermined channel */ @@ -462,16 +468,32 @@ static int pca954x_probe(struct i2c_client *client, } } + /* Errata ID E-00013 on board LS2088ARDB and LS2088ARDB: + * The point here is that you must not disable a mux if there + * are no pullups on the input or you mess up the I2C. This + * needs to be put into the DTS really as the kernel cannot + * know this otherwise. + */ + + data->disable_mux = np && + of_property_read_bool(np, "i2c-mux-never-disable") && + data->chip->muxtype == pca954x_ismux ? + data->chip->enable : 0; + /* Write the mux register at addr to verify * that the mux is in fact present. This also * initializes the mux to disconnected state. */ - if (i2c_smbus_write_byte(client, 0) < 0) { + if (i2c_smbus_write_byte(client, data->disable_mux) < 0) { dev_warn(dev, "probe failed\n"); return -ENODEV; } - data->last_chan = 0; /* force the first selection */ + if (data->disable_mux != 0) + data->last_chan = data->chip->nchans; + else + data->last_chan = 0; /* force the first selection */ + data->idle_state = MUX_IDLE_AS_IS; idle_disconnect_dt = np && @@ -531,8 +553,11 @@ static int pca954x_resume(struct device *dev) struct i2c_mux_core *muxc = i2c_get_clientdata(client); struct pca954x *data = i2c_mux_priv(muxc); - data->last_chan = 0; - return i2c_smbus_write_byte(client, 0); + if (data->disable_mux != 0) + data->last_chan = data->chip->nchans; + else + data->last_chan = 0; + return i2c_smbus_write_byte(client, data->disable_mux); } #endif
On some Layerscape boards like LS2085ARDB and LS2088ARDB, input pull-up resistors on PCA954x MUX device are missing on board, So, if MUX are disabled after powered-on, input lines will float leading to incorrect functionality. Hence, PCA954x MUX device should never be turned-off after power-on. Add property to skip disabling PCA954x MUX device if device tree contains "i2c-mux-never-disable" for PCA954x device node. Errata ID: E-00013 on board LS2085ARDB and LS2088ARDB (Board revision found on Rev.B, Rev.C and Rev.D) Signed-off-by: Biwen Li <biwen.li@nxp.com> --- drivers/i2c/muxes/i2c-mux-pca954x.c | 33 +++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)