| Message ID | 20200901211747.20649-1-evan.nimmo@alliedtelesis.co.nz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] i2c: algo-pca: Reapply i2c bus settings after reset | expand |
On Wed, Sep 02, 2020 at 09:17:47AM +1200, Evan Nimmo wrote: > If something goes wrong (such as the SCL being stuck low) then we need > to reset the PCA chip. The issue with this is that on reset we lose all > config settings and the chip ends up in a disabled state which results > in a lock up/high cpu usage. We need to re-apply any configuration that cpu -> CPU (I guess Wolfram can decide with this when applying) > had previously been set and re-enable the chip. FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Evan Nimmo <evan.nimmo@alliedtelesis.co.nz> > --- > changes in v2: > - changed lowercase "pca to uppercase "PCA". > - reworded and reformatted the multiline comment. > - moved the clock frequency KERN_INFO closer to the call that sets this. > - moved the i2c_bus_settings struct to the more generic i2c.h and removed > - the comments indicating this as being for the pca chip. > > drivers/i2c/algos/i2c-algo-pca.c | 36 +++++++++++++++++++++----------- > include/linux/i2c-algo-pca.h | 1 + > include/linux/i2c.h | 14 +++++++++++++ > 3 files changed, 39 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c > index 710fbef9a9c2..8b98b737b499 100644 > --- a/drivers/i2c/algos/i2c-algo-pca.c > +++ b/drivers/i2c/algos/i2c-algo-pca.c > @@ -41,8 +41,22 @@ static void pca_reset(struct i2c_algo_pca_data *adap) > pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET); > pca_outw(adap, I2C_PCA_IND, 0xA5); > pca_outw(adap, I2C_PCA_IND, 0x5A); > + > + /* > + * After a reset we need to re-apply any configuration > + * (calculated in pca_init) to get the bus in a working state. > + */ > + pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IMODE); > + pca_outw(adap, I2C_PCA_IND, adap->bus_settings.mode); > + pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_ISCLL); > + pca_outw(adap, I2C_PCA_IND, adap->bus_settings.tlow); > + pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_ISCLH); > + pca_outw(adap, I2C_PCA_IND, adap->bus_settings.thi); > + > + pca_set_con(adap, I2C_PCA_CON_ENSIO); > } else { > adap->reset_chip(adap->data); > + pca_set_con(adap, I2C_PCA_CON_ENSIO | adap->bus_settings.clock_freq); > } > } > > @@ -423,13 +437,15 @@ static int pca_init(struct i2c_adapter *adap) > " Use the nominal frequency.\n", adap->name); > } > > - pca_reset(pca_data); > - > clock = pca_clock(pca_data); > + > printk(KERN_INFO "%s: Clock frequency is %dkHz\n", > adap->name, freqs[clock]); > > - pca_set_con(pca_data, I2C_PCA_CON_ENSIO | clock); > + /* Store settings as these will be needed when the PCA chip is reset */ > + pca_data->bus_settings.clock_freq = clock; > + > + pca_reset(pca_data); > } else { > int clock; > int mode; > @@ -496,19 +512,15 @@ static int pca_init(struct i2c_adapter *adap) > thi = tlow * min_thi / min_tlow; > } > > + /* Store settings as these will be needed when the PCA chip is reset */ > + pca_data->bus_settings.mode = mode; > + pca_data->bus_settings.tlow = tlow; > + pca_data->bus_settings.thi = thi; > + > pca_reset(pca_data); > > printk(KERN_INFO > "%s: Clock frequency is %dHz\n", adap->name, clock * 100); > - > - pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_IMODE); > - pca_outw(pca_data, I2C_PCA_IND, mode); > - pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_ISCLL); > - pca_outw(pca_data, I2C_PCA_IND, tlow); > - pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_ISCLH); > - pca_outw(pca_data, I2C_PCA_IND, thi); > - > - pca_set_con(pca_data, I2C_PCA_CON_ENSIO); > } > udelay(500); /* 500 us for oscillator to stabilise */ > > diff --git a/include/linux/i2c-algo-pca.h b/include/linux/i2c-algo-pca.h > index d03071732db4..97d1f4cd8e56 100644 > --- a/include/linux/i2c-algo-pca.h > +++ b/include/linux/i2c-algo-pca.h > @@ -64,6 +64,7 @@ struct i2c_algo_pca_data { > * For PCA9665, use the frequency you want here. */ > unsigned int i2c_clock; > unsigned int chip; > + struct i2c_bus_settings bus_settings; > }; > > int i2c_pca_add_bus(struct i2c_adapter *); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index fc55ea41d323..8c5138fbe532 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -724,6 +724,20 @@ struct i2c_adapter { > }; > #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) > > +/** > + * struct i2c_bus_settings - The configured i2c bus settings > + * @mode: Configured i2c bus mode > + * @tlow: Configured SCL LOW period > + * @thi: Configured SCL HIGH period > + * @clock_freq: The configured clock frequency > + */ > +struct i2c_bus_settings { > + int mode; > + int tlow; > + int thi; > + int clock_freq; > +}; > + > static inline void *i2c_get_adapdata(const struct i2c_adapter *adap) > { > return dev_get_drvdata(&adap->dev); > -- > 2.27.0 >
On Wed, Sep 02, 2020 at 11:43:56AM +0300, Andy Shevchenko wrote: > On Wed, Sep 02, 2020 at 09:17:47AM +1200, Evan Nimmo wrote: > > If something goes wrong (such as the SCL being stuck low) then we need > > to reset the PCA chip. The issue with this is that on reset we lose all > > config settings and the chip ends up in a disabled state which results > > in a lock up/high cpu usage. We need to re-apply any configuration that > > cpu -> CPU (I guess Wolfram can decide with this when applying) > > > had previously been set and re-enable the chip. > > FWIW, > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> And actually you forgot Chris' tag... But I dunno if the moving struct definition is something that Chris might not like.
On 2/09/20 8:45 pm, Andy Shevchenko wrote: > On Wed, Sep 02, 2020 at 11:43:56AM +0300, Andy Shevchenko wrote: >> On Wed, Sep 02, 2020 at 09:17:47AM +1200, Evan Nimmo wrote: >>> If something goes wrong (such as the SCL being stuck low) then we need >>> to reset the PCA chip. The issue with this is that on reset we lose all >>> config settings and the chip ends up in a disabled state which results >>> in a lock up/high cpu usage. We need to re-apply any configuration that >> cpu -> CPU (I guess Wolfram can decide with this when applying) >> >>> had previously been set and re-enable the chip. >> FWIW, >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > And actually you forgot Chris' tag... But I dunno if the moving struct > definition is something that Chris might not like. > Fine by me. Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c index 710fbef9a9c2..8b98b737b499 100644 --- a/drivers/i2c/algos/i2c-algo-pca.c +++ b/drivers/i2c/algos/i2c-algo-pca.c @@ -41,8 +41,22 @@ static void pca_reset(struct i2c_algo_pca_data *adap) pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET); pca_outw(adap, I2C_PCA_IND, 0xA5); pca_outw(adap, I2C_PCA_IND, 0x5A); + + /* + * After a reset we need to re-apply any configuration + * (calculated in pca_init) to get the bus in a working state. + */ + pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IMODE); + pca_outw(adap, I2C_PCA_IND, adap->bus_settings.mode); + pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_ISCLL); + pca_outw(adap, I2C_PCA_IND, adap->bus_settings.tlow); + pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_ISCLH); + pca_outw(adap, I2C_PCA_IND, adap->bus_settings.thi); + + pca_set_con(adap, I2C_PCA_CON_ENSIO); } else { adap->reset_chip(adap->data); + pca_set_con(adap, I2C_PCA_CON_ENSIO | adap->bus_settings.clock_freq); } } @@ -423,13 +437,15 @@ static int pca_init(struct i2c_adapter *adap) " Use the nominal frequency.\n", adap->name); } - pca_reset(pca_data); - clock = pca_clock(pca_data); + printk(KERN_INFO "%s: Clock frequency is %dkHz\n", adap->name, freqs[clock]); - pca_set_con(pca_data, I2C_PCA_CON_ENSIO | clock); + /* Store settings as these will be needed when the PCA chip is reset */ + pca_data->bus_settings.clock_freq = clock; + + pca_reset(pca_data); } else { int clock; int mode; @@ -496,19 +512,15 @@ static int pca_init(struct i2c_adapter *adap) thi = tlow * min_thi / min_tlow; } + /* Store settings as these will be needed when the PCA chip is reset */ + pca_data->bus_settings.mode = mode; + pca_data->bus_settings.tlow = tlow; + pca_data->bus_settings.thi = thi; + pca_reset(pca_data); printk(KERN_INFO "%s: Clock frequency is %dHz\n", adap->name, clock * 100); - - pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_IMODE); - pca_outw(pca_data, I2C_PCA_IND, mode); - pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_ISCLL); - pca_outw(pca_data, I2C_PCA_IND, tlow); - pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_ISCLH); - pca_outw(pca_data, I2C_PCA_IND, thi); - - pca_set_con(pca_data, I2C_PCA_CON_ENSIO); } udelay(500); /* 500 us for oscillator to stabilise */ diff --git a/include/linux/i2c-algo-pca.h b/include/linux/i2c-algo-pca.h index d03071732db4..97d1f4cd8e56 100644 --- a/include/linux/i2c-algo-pca.h +++ b/include/linux/i2c-algo-pca.h @@ -64,6 +64,7 @@ struct i2c_algo_pca_data { * For PCA9665, use the frequency you want here. */ unsigned int i2c_clock; unsigned int chip; + struct i2c_bus_settings bus_settings; }; int i2c_pca_add_bus(struct i2c_adapter *); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index fc55ea41d323..8c5138fbe532 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -724,6 +724,20 @@ struct i2c_adapter { }; #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) +/** + * struct i2c_bus_settings - The configured i2c bus settings + * @mode: Configured i2c bus mode + * @tlow: Configured SCL LOW period + * @thi: Configured SCL HIGH period + * @clock_freq: The configured clock frequency + */ +struct i2c_bus_settings { + int mode; + int tlow; + int thi; + int clock_freq; +}; + static inline void *i2c_get_adapdata(const struct i2c_adapter *adap) { return dev_get_drvdata(&adap->dev);
If something goes wrong (such as the SCL being stuck low) then we need to reset the PCA chip. The issue with this is that on reset we lose all config settings and the chip ends up in a disabled state which results in a lock up/high cpu usage. We need to re-apply any configuration that had previously been set and re-enable the chip. Signed-off-by: Evan Nimmo <evan.nimmo@alliedtelesis.co.nz> --- changes in v2: - changed lowercase "pca to uppercase "PCA". - reworded and reformatted the multiline comment. - moved the clock frequency KERN_INFO closer to the call that sets this. - moved the i2c_bus_settings struct to the more generic i2c.h and removed - the comments indicating this as being for the pca chip. drivers/i2c/algos/i2c-algo-pca.c | 36 +++++++++++++++++++++----------- include/linux/i2c-algo-pca.h | 1 + include/linux/i2c.h | 14 +++++++++++++ 3 files changed, 39 insertions(+), 12 deletions(-)