* [PATCH 1/1] mfd: da9063: Support SMBus and I2C mode
2021-01-25 12:54 ` [PATCH 1/1] " Mark Jonas
@ 2021-01-25 18:49 ` Jonas Mark (BT-FIR/ENG1-Grb)
2021-01-25 18:58 ` Mark Jonas
2021-01-28 14:50 ` Adam Thomson
2 siblings, 0 replies; 8+ messages in thread
From: Jonas Mark (BT-FIR/ENG1-Grb) @ 2021-01-25 18:49 UTC (permalink / raw)
To: Support Opensource, Lee Jones, Rob Herring
Cc: devicetree, linux-kernel, Adam.Thomson.Opensource,
stwiss.opensource, marek.vasut, RUAN Tingquan (BT-FIR/ENG1-Zhu),
Streidl Hubert (BT-FIR/ENG1-Grb), Jonas Mark (BT-FIR/ENG1-Grb)
Hi,
I also intended to send a cover-letter but it was somehow lost. Here it is:
On an NXP i.MX6 Solo processor we are running an application which makes use of real-time threads (SCHED_RR). In combination with a DA9063 we experienced (rare) random shut-downs and reboots. We found that the issue was caused by a combination of the (default) DA9063 SMBus mode and non-atomic I2C transactions of the i.MX6 I2C driver. Because a transaction could be idle for longer than the SMBus clock time-out due to a real-time thread the DA9063 would time-out and receive the second half of the transaction as an unintended message.
The solution we are giving to review in this patch is to allow using the I2C mode of the DA9063. We kindly ask for feedback and eventually hope for an integration to the mainline.
Because we are on a vendor kernel we were not able to test this patch on the current mainline kernel. Though, we tested a (very similar) patch on our (close to mainline) Linux 4.14 and 5.4 vendor kernels.
Cheers,
Mark
Mark Jonas
Building Technologies, Panel Software Fire (BT-FIR/ENG1-Grb)
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com
Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118
Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante
> -----Ursprüngliche Nachricht-----
> Von: Mark Jonas <mark.jonas@de.bosch.com>
> Gesendet: Montag, 25. Januar 2021 13:55
> An: Support Opensource <support.opensource@diasemi.com>; Lee Jones
> <lee.jones@linaro.org>; Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> Adam.Thomson.Opensource@diasemi.com; stwiss.opensource@diasemi.com;
> marek.vasut@gmail.com; RUAN Tingquan (BT-FIR/ENG1-Zhu)
> <Tingquan.Ruan@cn.bosch.com>; Streidl Hubert (BT-FIR/ENG1-Grb)
> <Hubert.Streidl@de.bosch.com>; Jonas Mark (BT-FIR/ENG1-Grb)
> <Mark.Jonas@de.bosch.com>
> Betreff: [PATCH 1/1] mfd: da9063: Support SMBus and I2C mode
>
> 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 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 makes the I2C mode configurable by device tree. The SMBus
> compliant mode is kept as the default.
>
> Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> ---
> Documentation/devicetree/bindings/mfd/da9063.txt | 7 +++++++
> drivers/mfd/da9063-core.c | 9 +++++++++
> drivers/mfd/da9063-i2c.c | 13 +++++++++++++
> include/linux/mfd/da9063/core.h | 1 +
> include/linux/mfd/da9063/registers.h | 3 +++
> 5 files changed, 33 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt
> b/Documentation/devicetree/bindings/mfd/da9063.txt
> index 8da879935c59..256f2a25fe0a 100644
> --- a/Documentation/devicetree/bindings/mfd/da9063.txt
> +++ b/Documentation/devicetree/bindings/mfd/da9063.txt
> @@ -19,6 +19,12 @@ Required properties:
> - interrupts : IRQ line information.
> - interrupt-controller
>
> +Optional properties:
> +
> +- i2c-mode : Switch serial 2-wire interface into I2C mode. Without this
> + property the PMIC uses the SMBus mode (resets the interface if the
> +clock
> + ceases for a longer time than the SMBus timeout).
> +
> Sub-nodes:
>
> - regulators : This node defines the settings for the LDOs and BUCKs.
> @@ -77,6 +83,7 @@ Example:
> interrupt-parent = <&gpio6>;
> interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> interrupt-controller;
> + i2c-mode;
>
> rtc {
> compatible = "dlg,da9063-rtc";
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c index
> df407c3afce3..baa1e4310c8c 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -162,6 +162,15 @@ int da9063_device_init(struct da9063 *da9063,
> unsigned int irq) {
> int ret;
>
> + if (da9063->i2cmode) {
> + ret = regmap_update_bits(da9063->regmap,
> DA9063_REG_CONFIG_J,
> + DA9063_TWOWIRE_TO, 0);
> + if (ret < 0) {
> + dev_err(da9063->dev, "Cannot enable I2C mode.\n");
> + return -EIO;
> + }
> + }
> +
> ret = da9063_clear_fault_log(da9063);
> if (ret < 0)
> dev_err(da9063->dev, "Cannot clear fault log\n"); diff --git
> a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c index
> 3781d0bb7786..af0bf13ab43e 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -351,6 +351,17 @@ static const struct of_device_id da9063_dt_ids[] = {
> { }
> };
> MODULE_DEVICE_TABLE(of, da9063_dt_ids);
> +
> +static void da9063_i2c_parse_dt(struct i2c_client *client, struct
> +da9063 *da9063) {
> + struct device_node *np = client->dev.of_node;
> +
> + if (of_property_read_bool(np, "i2c-mode"))
> + da9063->i2cmode = true;
> + else
> + da9063->i2cmode = false;
> +}
> +
> static int da9063_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> @@ -366,6 +377,8 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> da9063->chip_irq = i2c->irq;
> da9063->type = id->driver_data;
>
> + da9063_i2c_parse_dt(i2c, da9063);
> +
> ret = da9063_get_device_type(i2c, da9063);
> if (ret)
> return ret;
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index fa7a43f02f27..866864c50f78 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -77,6 +77,7 @@ struct da9063 {
> enum da9063_type type;
> unsigned char variant_code;
> unsigned int flags;
> + bool i2cmode;
>
> /* Control interface */
> struct regmap *regmap;
> 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
> --
> 2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] mfd: da9063: Support SMBus and I2C mode
2021-01-25 12:54 ` [PATCH 1/1] " Mark Jonas
2021-01-25 18:49 ` Jonas Mark (BT-FIR/ENG1-Grb)
@ 2021-01-25 18:58 ` Mark Jonas
2021-01-26 8:23 ` Lee Jones
2021-01-28 14:50 ` Adam Thomson
2 siblings, 1 reply; 8+ messages in thread
From: Mark Jonas @ 2021-01-25 18:58 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Support Opensource
Cc: devicetree, linux-kernel, Adam.Thomson.Opensource,
stwiss.opensource, marek.vasut, RUAN Tingquan (BT-FIR/ENG1-Zhu),
Streidl Hubert (BT-FIR/ENG1-Grb), Jonas Mark (BT-FIR/ENG1-Grb)
Hi,
I also intended to send a cover-letter but it was somehow lost on the
way. Here it is:
On an NXP i.MX6 Solo processor we are running an application which
makes use of real-time threads (SCHED_RR). In combination with a
DA9063 we experienced (rare) random shut-downs and reboots. We found
that the issue was caused by a combination of the (default) DA9063
SMBus mode and non-atomic I2C transactions of the i.MX6 I2C driver.
Because a transaction could be idle for longer than the SMBus clock
time-out due to a real-time thread the DA9063 would time-out and
receive the second half of the transaction as an unintended message.
The solution we are giving to review in this patch is to allow using
the I2C mode of the DA9063. We kindly ask for feedback and eventually
hope for an integration to the mainline.
Because we are on a vendor kernel we were not able to test this patch
on the current mainline kernel. Though, we tested a (very similar)
patch on our (close to mainline) Linux 4.14 and 5.4 vendor kernels.
Cheers,
Mark
On Mon, Jan 25, 2021 at 2:16 PM Mark Jonas <mark.jonas@de.bosch.com> 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 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 makes the I2C mode configurable by device tree. The SMBus
> compliant mode is kept as the default.
>
> Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> ---
> Documentation/devicetree/bindings/mfd/da9063.txt | 7 +++++++
> drivers/mfd/da9063-core.c | 9 +++++++++
> drivers/mfd/da9063-i2c.c | 13 +++++++++++++
> include/linux/mfd/da9063/core.h | 1 +
> include/linux/mfd/da9063/registers.h | 3 +++
> 5 files changed, 33 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt b/Documentation/devicetree/bindings/mfd/da9063.txt
> index 8da879935c59..256f2a25fe0a 100644
> --- a/Documentation/devicetree/bindings/mfd/da9063.txt
> +++ b/Documentation/devicetree/bindings/mfd/da9063.txt
> @@ -19,6 +19,12 @@ Required properties:
> - interrupts : IRQ line information.
> - interrupt-controller
>
> +Optional properties:
> +
> +- i2c-mode : Switch serial 2-wire interface into I2C mode. Without this
> + property the PMIC uses the SMBus mode (resets the interface if the clock
> + ceases for a longer time than the SMBus timeout).
> +
> Sub-nodes:
>
> - regulators : This node defines the settings for the LDOs and BUCKs.
> @@ -77,6 +83,7 @@ Example:
> interrupt-parent = <&gpio6>;
> interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> interrupt-controller;
> + i2c-mode;
>
> rtc {
> compatible = "dlg,da9063-rtc";
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index df407c3afce3..baa1e4310c8c 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -162,6 +162,15 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> {
> int ret;
>
> + if (da9063->i2cmode) {
> + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONFIG_J,
> + DA9063_TWOWIRE_TO, 0);
> + if (ret < 0) {
> + dev_err(da9063->dev, "Cannot enable I2C mode.\n");
> + return -EIO;
> + }
> + }
> +
> ret = da9063_clear_fault_log(da9063);
> if (ret < 0)
> dev_err(da9063->dev, "Cannot clear fault log\n");
> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> index 3781d0bb7786..af0bf13ab43e 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -351,6 +351,17 @@ static const struct of_device_id da9063_dt_ids[] = {
> { }
> };
> MODULE_DEVICE_TABLE(of, da9063_dt_ids);
> +
> +static void da9063_i2c_parse_dt(struct i2c_client *client, struct da9063 *da9063)
> +{
> + struct device_node *np = client->dev.of_node;
> +
> + if (of_property_read_bool(np, "i2c-mode"))
> + da9063->i2cmode = true;
> + else
> + da9063->i2cmode = false;
> +}
> +
> static int da9063_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> @@ -366,6 +377,8 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> da9063->chip_irq = i2c->irq;
> da9063->type = id->driver_data;
>
> + da9063_i2c_parse_dt(i2c, da9063);
> +
> ret = da9063_get_device_type(i2c, da9063);
> if (ret)
> return ret;
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index fa7a43f02f27..866864c50f78 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -77,6 +77,7 @@ struct da9063 {
> enum da9063_type type;
> unsigned char variant_code;
> unsigned int flags;
> + bool i2cmode;
>
> /* Control interface */
> struct regmap *regmap;
> 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
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] mfd: da9063: Support SMBus and I2C mode
2021-01-25 18:58 ` Mark Jonas
@ 2021-01-26 8:23 ` Lee Jones
0 siblings, 0 replies; 8+ messages in thread
From: Lee Jones @ 2021-01-26 8:23 UTC (permalink / raw)
To: Mark Jonas
Cc: Rob Herring, Support Opensource, devicetree, linux-kernel,
Adam.Thomson.Opensource, stwiss.opensource, marek.vasut,
RUAN Tingquan (BT-FIR/ENG1-Zhu), Streidl Hubert (BT-FIR/ENG1-Grb),
Jonas Mark (BT-FIR/ENG1-Grb)
On Mon, 25 Jan 2021, Mark Jonas wrote:
> Hi,
>
> I also intended to send a cover-letter but it was somehow lost on the
> way. Here it is:
>
> On an NXP i.MX6 Solo processor we are running an application which
> makes use of real-time threads (SCHED_RR). In combination with a
> DA9063 we experienced (rare) random shut-downs and reboots. We found
> that the issue was caused by a combination of the (default) DA9063
> SMBus mode and non-atomic I2C transactions of the i.MX6 I2C driver.
> Because a transaction could be idle for longer than the SMBus clock
> time-out due to a real-time thread the DA9063 would time-out and
> receive the second half of the transaction as an unintended message.
>
> The solution we are giving to review in this patch is to allow using
> the I2C mode of the DA9063. We kindly ask for feedback and eventually
> hope for an integration to the mainline.
>
> Because we are on a vendor kernel we were not able to test this patch
> on the current mainline kernel. Though, we tested a (very similar)
> patch on our (close to mainline) Linux 4.14 and 5.4 vendor kernels.
This is the 3rd cover-letter I've received now! :)
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] mfd: da9063: Support SMBus and I2C mode
2021-01-25 12:54 ` [PATCH 1/1] " Mark Jonas
2021-01-25 18:49 ` Jonas Mark (BT-FIR/ENG1-Grb)
2021-01-25 18:58 ` Mark Jonas
@ 2021-01-28 14:50 ` Adam Thomson
2021-01-28 16:09 ` AW: " Jonas Mark (BT-FIR/ENG1-Grb)
2 siblings, 1 reply; 8+ messages in thread
From: Adam Thomson @ 2021-01-28 14:50 UTC (permalink / raw)
To: Mark Jonas, Support Opensource, Lee Jones, Rob Herring
Cc: devicetree, linux-kernel, Adam Thomson, Steve Twiss, marek.vasut,
tingquan.ruan, hubert.streidl
On 25 January 2021 12:55, 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 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 makes the I2C mode configurable by device tree. The SMBus
> compliant mode is kept as the default.
Could we not just check the bus' functionality flags and set this accordingly?
Something like this is already done in regmap-i2c to determine how to access
the device:
https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-i2c.c#L309
This seems cleaner than a new DT property, or will this not work in this
situation?
>
> Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> ---
> Documentation/devicetree/bindings/mfd/da9063.txt | 7 +++++++
> drivers/mfd/da9063-core.c | 9 +++++++++
> drivers/mfd/da9063-i2c.c | 13 +++++++++++++
> include/linux/mfd/da9063/core.h | 1 +
> include/linux/mfd/da9063/registers.h | 3 +++
> 5 files changed, 33 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt
> b/Documentation/devicetree/bindings/mfd/da9063.txt
> index 8da879935c59..256f2a25fe0a 100644
> --- a/Documentation/devicetree/bindings/mfd/da9063.txt
> +++ b/Documentation/devicetree/bindings/mfd/da9063.txt
> @@ -19,6 +19,12 @@ Required properties:
> - interrupts : IRQ line information.
> - interrupt-controller
>
> +Optional properties:
> +
> +- i2c-mode : Switch serial 2-wire interface into I2C mode. Without this
> + property the PMIC uses the SMBus mode (resets the interface if the clock
> + ceases for a longer time than the SMBus timeout).
> +
> Sub-nodes:
>
> - regulators : This node defines the settings for the LDOs and BUCKs.
> @@ -77,6 +83,7 @@ Example:
> interrupt-parent = <&gpio6>;
> interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> interrupt-controller;
> + i2c-mode;
>
> rtc {
> compatible = "dlg,da9063-rtc";
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index df407c3afce3..baa1e4310c8c 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -162,6 +162,15 @@ int da9063_device_init(struct da9063 *da9063, unsigned
> int irq)
> {
> int ret;
>
> + if (da9063->i2cmode) {
> + ret = regmap_update_bits(da9063->regmap,
> DA9063_REG_CONFIG_J,
> + DA9063_TWOWIRE_TO, 0);
> + if (ret < 0) {
> + dev_err(da9063->dev, "Cannot enable I2C mode.\n");
> + return -EIO;
> + }
> + }
> +
> ret = da9063_clear_fault_log(da9063);
> if (ret < 0)
> dev_err(da9063->dev, "Cannot clear fault log\n");
> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> index 3781d0bb7786..af0bf13ab43e 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -351,6 +351,17 @@ static const struct of_device_id da9063_dt_ids[] = {
> { }
> };
> MODULE_DEVICE_TABLE(of, da9063_dt_ids);
> +
> +static void da9063_i2c_parse_dt(struct i2c_client *client, struct da9063 *da9063)
> +{
> + struct device_node *np = client->dev.of_node;
> +
> + if (of_property_read_bool(np, "i2c-mode"))
> + da9063->i2cmode = true;
> + else
> + da9063->i2cmode = false;
> +}
> +
> static int da9063_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> @@ -366,6 +377,8 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> da9063->chip_irq = i2c->irq;
> da9063->type = id->driver_data;
>
> + da9063_i2c_parse_dt(i2c, da9063);
> +
> ret = da9063_get_device_type(i2c, da9063);
> if (ret)
> return ret;
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index fa7a43f02f27..866864c50f78 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -77,6 +77,7 @@ struct da9063 {
> enum da9063_type type;
> unsigned char variant_code;
> unsigned int flags;
> + bool i2cmode;
>
> /* Control interface */
> struct regmap *regmap;
> 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
> --
> 2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread* AW: [PATCH 1/1] mfd: da9063: Support SMBus and I2C mode
2021-01-28 14:50 ` Adam Thomson
@ 2021-01-28 16:09 ` Jonas Mark (BT-FIR/ENG1-Grb)
2021-01-28 20:53 ` Wolfram Sang
0 siblings, 1 reply; 8+ messages in thread
From: Jonas Mark (BT-FIR/ENG1-Grb) @ 2021-01-28 16:09 UTC (permalink / raw)
To: Adam Thomson, Support Opensource, Lee Jones, Rob Herring
Cc: devicetree, linux-kernel, linux-i2c, Steve Twiss, marek.vasut,
RUAN Tingquan (BT-FIR/ENG1-Zhu), Streidl Hubert (BT-FIR/ENG1-Grb),
Jonas Mark (BT-FIR/ENG1-Grb),
festevam, o.rempel, anson.huang
Hi Adam,
> > 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 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 makes the I2C mode configurable by device tree. The SMBus
> > compliant mode is kept as the default.
>
> Could we not just check the bus' functionality flags and set this accordingly?
> Something like this is already done in regmap-i2c to determine how to access
> the device:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-i2c.c#L309
>
> This seems cleaner than a new DT property, or will this not work in this
> situation?
Thank you for the proposal. We were not aware of this possibility.
We experienced the SMBus timeout problem on a i.MX6 Solo. When looking
at the i2c-imx driver we see that it defines a list of functionality.
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
| I2C_FUNC_SMBUS_READ_BLOCK_DATA;
https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-imx.c#L1133
None of that seems to model the inability to perform atomic transactions
within the SMBus timeout. This is either a bug of this specific driver
or maybe the expressiveness of I2C_FUNC_* is not sufficient.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/i2c.h#L88
What flag do you think we could check to find out whether the bus is
able to obey the SMBus timeout or not?
> > Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com>
> > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> > ---
> > Documentation/devicetree/bindings/mfd/da9063.txt | 7 +++++++
> > drivers/mfd/da9063-core.c | 9 +++++++++
> > drivers/mfd/da9063-i2c.c | 13 +++++++++++++
> > include/linux/mfd/da9063/core.h | 1 +
> > include/linux/mfd/da9063/registers.h | 3 +++
> > 5 files changed, 33 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt
> > b/Documentation/devicetree/bindings/mfd/da9063.txt
> > index 8da879935c59..256f2a25fe0a 100644
> > --- a/Documentation/devicetree/bindings/mfd/da9063.txt
> > +++ b/Documentation/devicetree/bindings/mfd/da9063.txt
> > @@ -19,6 +19,12 @@ Required properties:
> > - interrupts : IRQ line information.
> > - interrupt-controller
> >
> > +Optional properties:
> > +
> > +- i2c-mode : Switch serial 2-wire interface into I2C mode. Without
> > +this
> > + property the PMIC uses the SMBus mode (resets the interface if the
> > +clock
> > + ceases for a longer time than the SMBus timeout).
> > +
> > Sub-nodes:
> >
> > - regulators : This node defines the settings for the LDOs and BUCKs.
> > @@ -77,6 +83,7 @@ Example:
> > interrupt-parent = <&gpio6>;
> > interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> > interrupt-controller;
> > + i2c-mode;
> >
> > rtc {
> > compatible = "dlg,da9063-rtc";
> > diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> > index df407c3afce3..baa1e4310c8c 100644
> > --- a/drivers/mfd/da9063-core.c
> > +++ b/drivers/mfd/da9063-core.c
> > @@ -162,6 +162,15 @@ int da9063_device_init(struct da9063 *da9063,
> > unsigned int irq) {
> > int ret;
> >
> > + if (da9063->i2cmode) {
> > + ret = regmap_update_bits(da9063->regmap,
> > DA9063_REG_CONFIG_J,
> > + DA9063_TWOWIRE_TO, 0);
> > + if (ret < 0) {
> > + dev_err(da9063->dev, "Cannot enable I2C mode.\n");
> > + return -EIO;
> > + }
> > + }
> > +
> > ret = da9063_clear_fault_log(da9063);
> > if (ret < 0)
> > dev_err(da9063->dev, "Cannot clear fault log\n"); diff --git
> > a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c index
> > 3781d0bb7786..af0bf13ab43e 100644
> > --- a/drivers/mfd/da9063-i2c.c
> > +++ b/drivers/mfd/da9063-i2c.c
> > @@ -351,6 +351,17 @@ static const struct of_device_id da9063_dt_ids[] = {
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, da9063_dt_ids);
> > +
> > +static void da9063_i2c_parse_dt(struct i2c_client *client, struct
> > +da9063 *da9063) {
> > + struct device_node *np = client->dev.of_node;
> > +
> > + if (of_property_read_bool(np, "i2c-mode"))
> > + da9063->i2cmode = true;
> > + else
> > + da9063->i2cmode = false;
> > +}
> > +
> > static int da9063_i2c_probe(struct i2c_client *i2c,
> > const struct i2c_device_id *id) { @@ -366,6 +377,8
> @@ static
> > int da9063_i2c_probe(struct i2c_client *i2c,
> > da9063->chip_irq = i2c->irq;
> > da9063->type = id->driver_data;
> >
> > + da9063_i2c_parse_dt(i2c, da9063);
> > +
> > ret = da9063_get_device_type(i2c, da9063);
> > if (ret)
> > return ret;
> > diff --git a/include/linux/mfd/da9063/core.h
> > b/include/linux/mfd/da9063/core.h index fa7a43f02f27..866864c50f78
> > 100644
> > --- a/include/linux/mfd/da9063/core.h
> > +++ b/include/linux/mfd/da9063/core.h
> > @@ -77,6 +77,7 @@ struct da9063 {
> > enum da9063_type type;
> > unsigned char variant_code;
> > unsigned int flags;
> > + bool i2cmode;
> >
> > /* Control interface */
> > struct regmap *regmap;
> > 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
> > --
> > 2.25.1
Cheers,
Mark
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/1] mfd: da9063: Support SMBus and I2C mode
2021-01-28 16:09 ` AW: " Jonas Mark (BT-FIR/ENG1-Grb)
@ 2021-01-28 20:53 ` Wolfram Sang
0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2021-01-28 20:53 UTC (permalink / raw)
To: Jonas Mark (BT-FIR/ENG1-Grb)
Cc: Adam Thomson, Support Opensource, Lee Jones, Rob Herring,
devicetree, linux-kernel, linux-i2c, Steve Twiss, marek.vasut,
RUAN Tingquan (BT-FIR/ENG1-Zhu), Streidl Hubert (BT-FIR/ENG1-Grb),
festevam, o.rempel, anson.huang
[-- Attachment #1: Type: text/plain, Size: 801 bytes --]
Hi,
> None of that seems to model the inability to perform atomic transactions
> within the SMBus timeout. This is either a bug of this specific driver
> or maybe the expressiveness of I2C_FUNC_* is not sufficient.
>
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/i2c.h#L88
>
> What flag do you think we could check to find out whether the bus is
> able to obey the SMBus timeout or not?
While not perfect, you can reasonably assume that the bus cannot obey to
SMBus timings when I2C_FUNC_I2C is set. Because in the vast majority of
all cases with I2C_FUNC_I2C, SMBus commands are emulated which is prone
to the latency you described. You'd need a native SMBus controller to
avoid that which usually has not I2C_FUNC_I2C set.
Happy hacking,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread