| Message ID | 20210208152758.13093-1-mark.jonas@de.bosch.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] mfd: da9063: Support SMBus and I2C mode | expand |
Hi, > > From: Hubert Streidl <hubert.streidl@de.bosch.com> > > > > By default the PMIC DA9063 2-wire interface is SMBus compliant. This > > means the PMIC will automatically reset the interface when the clock > > signal ceases for more than the SMBus timeout of 35 ms. > > > > If the I2C driver / device is not capable of creating atomic I2C > > transactions, a context change can cause a ceasing of the clock signal. > > This can happen if for example a real-time thread is scheduled. Then > > the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently > > a write message could end up in the wrong register. This could cause > > unpredictable system behavior. > > > > The DA9063 PMIC also supports an I2C compliant mode for the 2-wire > > interface. This mode does not reset the interface when the clock > > signal ceases. Thus the problem depicted above does not occur. > > > > This patch tests for the bus functionality "I2C_FUNC_I2C". It can > > reasonably be assumed that the bus cannot obey SMBus timings if this > > functionality is set. SMBus commands most probably are emulated in > > this case which is prone to the latency issue described above. > > > > This patch enables the I2C bus mode if I2C_FUNC_I2C is set or > > otherwise enables the SMBus mode for a native SMBus controller which > > doesn't have I2C_FUNC_I2C set. > > > > Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com> > > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> > > Thanks for your efforts. Looks sensible to me, so: > > Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> Is the patch already on the way upstream? Is https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/log/?h=for-mfd-next the right place to watch for mainlining progress? Thanks, Mark
> Is the patch already on the way upstream?
Can't really speak for Lee here, but during the merge window patches are
usually not applied. So, in something like 2 weeks, usually collecting
for the next cycle begins. Looking at the CC list, I think you added all
the relevant people, so it seems all good.
On Sat, 20 Feb 2021, Wolfram Sang wrote: > > > Is the patch already on the way upstream? > > Can't really speak for Lee here, but during the merge window patches are > usually not applied. So, in something like 2 weeks, usually collecting > for the next cycle begins. Looking at the CC list, I think you added all > the relevant people, so it seems all good. Wolfram is correct. I'm not planning on taking patches again until -rc1 is out. Patch looks okay at first glance though. A proper review will be provided in due course. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On Mon, 08 Feb 2021, Mark Jonas wrote: > From: Hubert Streidl <hubert.streidl@de.bosch.com> > > By default the PMIC DA9063 2-wire interface is SMBus compliant. This > means the PMIC will automatically reset the interface when the clock > signal ceases for more than the SMBus timeout of 35 ms. > > If the I2C driver / device is not capable of creating atomic I2C > transactions, a context change can cause a ceasing of the clock signal. > This can happen if for example a real-time thread is scheduled. Then > the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently > a write message could end up in the wrong register. This could cause > unpredictable system behavior. > > The DA9063 PMIC also supports an I2C compliant mode for the 2-wire > interface. This mode does not reset the interface when the clock > signal ceases. Thus the problem depicted above does not occur. > > This patch tests for the bus functionality "I2C_FUNC_I2C". It can > reasonably be assumed that the bus cannot obey SMBus timings if > this functionality is set. SMBus commands most probably are emulated > in this case which is prone to the latency issue described above. > > This patch enables the I2C bus mode if I2C_FUNC_I2C is set or > otherwise enables the SMBus mode for a native SMBus controller > which doesn't have I2C_FUNC_I2C set. > > Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com> > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> > --- > Changes in v4: > - Remove logging of selected 2-wire bus mode. > > Changes in v3: > - busmode now contains the correct bit DA9063_TWOWIRE_TO > > Changes in v2: > - Implement proposal by Adam Thomson and Wolfram Sang to check for > functionality I2C_FUNC_I2C instead of introducing a new DT property. > > drivers/mfd/da9063-i2c.c | 11 +++++++++++ > include/linux/mfd/da9063/registers.h | 3 +++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c > index 3781d0bb7786..9450c95a3741 100644 > --- a/drivers/mfd/da9063-i2c.c > +++ b/drivers/mfd/da9063-i2c.c > @@ -355,6 +355,7 @@ static int da9063_i2c_probe(struct i2c_client *i2c, > const struct i2c_device_id *id) > { > struct da9063 *da9063; > + unsigned int busmode; > int ret; > > da9063 = devm_kzalloc(&i2c->dev, sizeof(struct da9063), GFP_KERNEL); > @@ -442,6 +443,16 @@ static int da9063_i2c_probe(struct i2c_client *i2c, > return ret; > } > > + busmode = i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C) ? > + 0 : DA9063_TWOWIRE_TO; Nit: I find ternaries like this tend to complicate matters and harm readability rather than the converse. > + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J, > + DA9063_TWOWIRE_TO, busmode); > + if (ret < 0) { > + dev_err(da9063->dev, "Failed to set 2-wire bus mode.\n"); > + return -EIO; > + } I'm a little confused by this. It's likely just me, but I would still like some clarification. So you write to the TWOWIRE register despite whether I2C is operable not, which I guess is fine. But what if I2C is disabled and the update fails. You seem to complain to the user that a failure occurred and return an error even if the configuration is invalid in the first place. Would it not be better to encapsulate the update inside the functionality check? > return da9063_device_init(da9063, i2c->irq); > } > > diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h > index 1dbabf1b3cb8..6e0f66a2e727 100644 > --- a/include/linux/mfd/da9063/registers.h > +++ b/include/linux/mfd/da9063/registers.h > @@ -1037,6 +1037,9 @@ > #define DA9063_NONKEY_PIN_AUTODOWN 0x02 > #define DA9063_NONKEY_PIN_AUTOFLPRT 0x03 > > +/* DA9063_REG_CONFIG_J (addr=0x10F) */ > +#define DA9063_TWOWIRE_TO 0x40 > + > /* DA9063_REG_MON_REG_5 (addr=0x116) */ > #define DA9063_MON_A8_IDX_MASK 0x07 > #define DA9063_MON_A8_IDX_NONE 0x00 -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Hi Lee, Thank you for having a look at the patch. > > From: Hubert Streidl <hubert.streidl@de.bosch.com> > > > > By default the PMIC DA9063 2-wire interface is SMBus compliant. This > > means the PMIC will automatically reset the interface when the clock > > signal ceases for more than the SMBus timeout of 35 ms. > > > > If the I2C driver / device is not capable of creating atomic I2C > > transactions, a context change can cause a ceasing of the clock signal. > > This can happen if for example a real-time thread is scheduled. Then > > the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently > > a write message could end up in the wrong register. This could cause > > unpredictable system behavior. > > > > The DA9063 PMIC also supports an I2C compliant mode for the 2-wire > > interface. This mode does not reset the interface when the clock > > signal ceases. Thus the problem depicted above does not occur. > > > > This patch tests for the bus functionality "I2C_FUNC_I2C". It can > > reasonably be assumed that the bus cannot obey SMBus timings if > > this functionality is set. SMBus commands most probably are emulated > > in this case which is prone to the latency issue described above. > > > > This patch enables the I2C bus mode if I2C_FUNC_I2C is set or > > otherwise enables the SMBus mode for a native SMBus controller > > which doesn't have I2C_FUNC_I2C set. > > > > Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com> > > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> > > --- > > Changes in v4: > > - Remove logging of selected 2-wire bus mode. > > > > Changes in v3: > > - busmode now contains the correct bit DA9063_TWOWIRE_TO > > > > Changes in v2: > > - Implement proposal by Adam Thomson and Wolfram Sang to check for > > functionality I2C_FUNC_I2C instead of introducing a new DT property. > > > > drivers/mfd/da9063-i2c.c | 11 +++++++++++ > > include/linux/mfd/da9063/registers.h | 3 +++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c > > index 3781d0bb7786..9450c95a3741 100644 > > --- a/drivers/mfd/da9063-i2c.c > > +++ b/drivers/mfd/da9063-i2c.c > > @@ -355,6 +355,7 @@ static int da9063_i2c_probe(struct i2c_client *i2c, > > const struct i2c_device_id *id) > > { > > struct da9063 *da9063; > > + unsigned int busmode; > > int ret; > > > > da9063 = devm_kzalloc(&i2c->dev, sizeof(struct da9063), GFP_KERNEL); > > @@ -442,6 +443,16 @@ static int da9063_i2c_probe(struct i2c_client *i2c, > > return ret; > > } > > > > + busmode = i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C) ? > > + 0 : DA9063_TWOWIRE_TO; > > Nit: I find ternaries like this tend to complicate matters and > harm readability rather than the converse. We can send an update of the patch if required. > > + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J, > > + DA9063_TWOWIRE_TO, busmode); > > + if (ret < 0) { > > + dev_err(da9063->dev, "Failed to set 2-wire bus mode.\n"); > > + return -EIO; > > + } > > I'm a little confused by this. It's likely just me, but I would still > like some clarification. > > So you write to the TWOWIRE register despite whether I2C is operable > not, which I guess is fine. In our understanding at this point the I2C / SMBus is definitely operable. Otherwise the call to da9063_get_device_type() would have already failed because it reads the chip ID via I2C / SMBus. > But what if I2C is disabled and the update fails. You seem to complain > to the user that a failure occurred and return an error even if the > configuration is invalid in the first place. > > Would it not be better to encapsulate the update inside the > functionality check? Do you mean i2c_check_functionality() with "functionality check"? I understood that this function is part of the I2C / SMBus subsystem. It checks available features of the I2C / SMBus controller. As proposed during review of this patch we check for I2C_FUNC_I2C to determine whether the controller can do SMBus or if it is limited to I2C functionality. If the controller can only do I2C then the DA9063 shall not expect SMBus. By default the DA9063 assumes that it is connected to an SMBus. Thus, even with our patch there are potentially still two (get chip ID, set 2-wire mode) accesses to the DA9063 by an I2C controller but the DA9063 assumes SMBus. Yet, our patch closes the window of opportunity for something bad to happen from maybe one accesse per second to two accesses over the complete lifetime of the driver. I think this is already pretty good. If you have a concrete proposal we could try to improve further. But one write access for setting the twowire mode will always be there. And without the call to da9063_get_device_type() this would also have to be "hard-coded" without the use of the regmap. I consider our patch being already that much better than the current state that it is worth taking it mainline. Without the patch our system triggers the fault during normal operation. Even with heavy stress testing we have not been able to trigger the fault once our patch was applied. > > return da9063_device_init(da9063, i2c->irq); > > } > > > > diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h > > index 1dbabf1b3cb8..6e0f66a2e727 100644 > > --- a/include/linux/mfd/da9063/registers.h > > +++ b/include/linux/mfd/da9063/registers.h > > @@ -1037,6 +1037,9 @@ > > #define DA9063_NONKEY_PIN_AUTODOWN 0x02 > > #define DA9063_NONKEY_PIN_AUTOFLPRT 0x03 > > > > +/* DA9063_REG_CONFIG_J (addr=0x10F) */ > > +#define DA9063_TWOWIRE_TO 0x40 > > + > > /* DA9063_REG_MON_REG_5 (addr=0x116) */ > > #define DA9063_MON_A8_IDX_MASK 0x07 > > #define DA9063_MON_A8_IDX_NONE 0x00 I am currently out of office. In case of change requirements we'll most likely send an updated patch mid of next week. Cheers, Mark
On Tue, 09 Mar 2021, Mark Jonas wrote: > Hi Lee, > > Thank you for having a look at the patch. > > > > From: Hubert Streidl <hubert.streidl@de.bosch.com> > > > > > > By default the PMIC DA9063 2-wire interface is SMBus compliant. This > > > means the PMIC will automatically reset the interface when the clock > > > signal ceases for more than the SMBus timeout of 35 ms. > > > > > > If the I2C driver / device is not capable of creating atomic I2C > > > transactions, a context change can cause a ceasing of the clock signal. > > > This can happen if for example a real-time thread is scheduled. Then > > > the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently > > > a write message could end up in the wrong register. This could cause > > > unpredictable system behavior. > > > > > > The DA9063 PMIC also supports an I2C compliant mode for the 2-wire > > > interface. This mode does not reset the interface when the clock > > > signal ceases. Thus the problem depicted above does not occur. > > > > > > This patch tests for the bus functionality "I2C_FUNC_I2C". It can > > > reasonably be assumed that the bus cannot obey SMBus timings if > > > this functionality is set. SMBus commands most probably are emulated > > > in this case which is prone to the latency issue described above. > > > > > > This patch enables the I2C bus mode if I2C_FUNC_I2C is set or > > > otherwise enables the SMBus mode for a native SMBus controller > > > which doesn't have I2C_FUNC_I2C set. > > > > > > Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com> > > > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> > > > --- > > > Changes in v4: > > > - Remove logging of selected 2-wire bus mode. > > > > > > Changes in v3: > > > - busmode now contains the correct bit DA9063_TWOWIRE_TO > > > > > > Changes in v2: > > > - Implement proposal by Adam Thomson and Wolfram Sang to check for > > > functionality I2C_FUNC_I2C instead of introducing a new DT property. > > > > > > drivers/mfd/da9063-i2c.c | 11 +++++++++++ > > > include/linux/mfd/da9063/registers.h | 3 +++ > > > 2 files changed, 14 insertions(+) > > > > > > diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c > > > index 3781d0bb7786..9450c95a3741 100644 > > > --- a/drivers/mfd/da9063-i2c.c > > > +++ b/drivers/mfd/da9063-i2c.c > > > @@ -355,6 +355,7 @@ static int da9063_i2c_probe(struct i2c_client *i2c, > > > const struct i2c_device_id *id) > > > { > > > struct da9063 *da9063; > > > + unsigned int busmode; > > > int ret; > > > > > > da9063 = devm_kzalloc(&i2c->dev, sizeof(struct da9063), GFP_KERNEL); > > > @@ -442,6 +443,16 @@ static int da9063_i2c_probe(struct i2c_client *i2c, > > > return ret; > > > } > > > > > > + busmode = i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C) ? > > > + 0 : DA9063_TWOWIRE_TO; > > > > Nit: I find ternaries like this tend to complicate matters and > > harm readability rather than the converse. > > We can send an update of the patch if required. > > > > + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J, > > > + DA9063_TWOWIRE_TO, busmode); > > > + if (ret < 0) { > > > + dev_err(da9063->dev, "Failed to set 2-wire bus mode.\n"); > > > + return -EIO; > > > + } > > > > I'm a little confused by this. It's likely just me, but I would still > > like some clarification. > > > > So you write to the TWOWIRE register despite whether I2C is operable > > not, which I guess is fine. > > In our understanding at this point the I2C / SMBus is definitely > operable. Otherwise the call to da9063_get_device_type() would have > already failed because it reads the chip ID via I2C / SMBus. > > > But what if I2C is disabled and the update fails. You seem to complain > > to the user that a failure occurred and return an error even if the > > configuration is invalid in the first place. > > > > Would it not be better to encapsulate the update inside the > > functionality check? > > Do you mean i2c_check_functionality() with "functionality check"? I > understood that this function is part of the I2C / SMBus subsystem. It > checks available features of the I2C / SMBus controller. As proposed > during review of this patch we check for I2C_FUNC_I2C to determine > whether the controller can do SMBus or if it is limited to I2C > functionality. If the controller can only do I2C then the DA9063 shall > not expect SMBus. > > By default the DA9063 assumes that it is connected to an SMBus. Thus, > even with our patch there are potentially still two (get chip ID, set > 2-wire mode) accesses to the DA9063 by an I2C controller but the > DA9063 assumes SMBus. Yet, our patch closes the window of opportunity > for something bad to happen from maybe one accesse per second to two > accesses over the complete lifetime of the driver. I think this is > already pretty good. > > If you have a concrete proposal we could try to improve further. But > one write access for setting the twowire mode will always be there. > And without the call to da9063_get_device_type() this would also have > to be "hard-coded" without the use of the regmap. > > I consider our patch being already that much better than the current > state that it is worth taking it mainline. Without the patch our > system triggers the fault during normal operation. Even with heavy > stress testing we have not been able to trigger the fault once our > patch was applied. This is my suggestion: /* If SMBus Mode is not available, enter Two-Wire Mode */ if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) { ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J, DA9063_TWOWIRE_TO, DA9063_TWOWIRE_TO); if (ret < 0) { dev_err(da9063->dev, "Failed to set Two-Wire Bus Mode\n"); return -EIO; } } > > > return da9063_device_init(da9063, i2c->irq); > > > } > > > > > > diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h > > > index 1dbabf1b3cb8..6e0f66a2e727 100644 > > > --- a/include/linux/mfd/da9063/registers.h > > > +++ b/include/linux/mfd/da9063/registers.h > > > @@ -1037,6 +1037,9 @@ > > > #define DA9063_NONKEY_PIN_AUTODOWN 0x02 > > > #define DA9063_NONKEY_PIN_AUTOFLPRT 0x03 > > > > > > +/* DA9063_REG_CONFIG_J (addr=0x10F) */ > > > +#define DA9063_TWOWIRE_TO 0x40 > > > + > > > /* DA9063_REG_MON_REG_5 (addr=0x116) */ > > > #define DA9063_MON_A8_IDX_MASK 0x07 > > > #define DA9063_MON_A8_IDX_NONE 0x00 > > I am currently out of office. In case of change requirements we'll > most likely send an updated patch mid of next week. > -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Hi Lee, Tidy, but I've noticed the logic got inverted along the way: > On Tue 09 Mar 2021, Lee Jones wrote: > On Tue, 09 Mar 2021, Mark Jonas wrote: > This is my suggestion: > > /* If SMBus Mode is not available, enter Two-Wire Mode */ > if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) { Determine bus *is* I2C, so assume SMBus timings not supported... if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) { > ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J, > DA9063_TWOWIRE_TO, DA9063_TWOWIRE_TO); ...and *clear* the (currently set by default) timeout bit: DA9063_TWOWIRE_TO, 0); > if (ret < 0) { > dev_err(da9063->dev, "Failed to set Two-Wire Bus > Mode\n"); > return -EIO; > } > } I think you're right to exclude a case; vendor motivation to override the TO default seems inherently trustworthy. Best regards, Adam
On Tue, 09 Mar 2021, Adam Ward wrote: > Hi Lee, > > Tidy, but I've noticed the logic got inverted along the way: > > > On Tue 09 Mar 2021, Lee Jones wrote: > > On Tue, 09 Mar 2021, Mark Jonas wrote: > > This is my suggestion: > > > > /* If SMBus Mode is not available, enter Two-Wire Mode */ > > if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) { > > Determine bus *is* I2C, so assume SMBus timings not supported... > if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) { > > > ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J, > > DA9063_TWOWIRE_TO, DA9063_TWOWIRE_TO); > > ...and *clear* the (currently set by default) timeout bit: > DA9063_TWOWIRE_TO, 0); Thanks for checking this. You're right. The example was provided as a back-of-a-fag-packet design, not a fully tested (or even compiled) solution. It took me 2 mins to type out. > > if (ret < 0) { > > dev_err(da9063->dev, "Failed to set Two-Wire Bus > > Mode\n"); > > return -EIO; > > } > > } > > I think you're right to exclude a case; vendor motivation to > override the TO default seems inherently trustworthy. A default value is a default value. :) -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c index 3781d0bb7786..9450c95a3741 100644 --- a/drivers/mfd/da9063-i2c.c +++ b/drivers/mfd/da9063-i2c.c @@ -355,6 +355,7 @@ static int da9063_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct da9063 *da9063; + unsigned int busmode; int ret; da9063 = devm_kzalloc(&i2c->dev, sizeof(struct da9063), GFP_KERNEL); @@ -442,6 +443,16 @@ static int da9063_i2c_probe(struct i2c_client *i2c, return ret; } + busmode = i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C) ? + 0 : DA9063_TWOWIRE_TO; + + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J, + DA9063_TWOWIRE_TO, busmode); + if (ret < 0) { + dev_err(da9063->dev, "Failed to set 2-wire bus mode.\n"); + return -EIO; + } + return da9063_device_init(da9063, i2c->irq); } diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h index 1dbabf1b3cb8..6e0f66a2e727 100644 --- a/include/linux/mfd/da9063/registers.h +++ b/include/linux/mfd/da9063/registers.h @@ -1037,6 +1037,9 @@ #define DA9063_NONKEY_PIN_AUTODOWN 0x02 #define DA9063_NONKEY_PIN_AUTOFLPRT 0x03 +/* DA9063_REG_CONFIG_J (addr=0x10F) */ +#define DA9063_TWOWIRE_TO 0x40 + /* DA9063_REG_MON_REG_5 (addr=0x116) */ #define DA9063_MON_A8_IDX_MASK 0x07 #define DA9063_MON_A8_IDX_NONE 0x00