This patch series introduce the kernel i2c-gpio bus recovery mechanism
for the Microchip SoCs. Updated the corresponding dts to add i2c
gpio pinctrl. The bus recovery is configured for the sama5d3/4 xplained
boards in dts.
Kamel Bouhara (4):
dt-bindings: i2c: at91: document optional bus recovery properties
i2c: at91: implement i2c bus recovery
ARM: at91/dt: sama5d3: add i2c gpio pinctrl
ARM: at91/dt: sama5d4: add i2c gpio pinctrl
.../devicetree/bindings/i2c/i2c-at91.txt | 10 +++
arch/arm/boot/dts/sama5d3.dtsi | 33 +++++++++-
arch/arm/boot/dts/sama5d4.dtsi | 33 +++++++++-
drivers/i2c/busses/i2c-at91-master.c | 63 +++++++++++++++++++
drivers/i2c/busses/i2c-at91.h | 8 +++
5 files changed, 141 insertions(+), 6 deletions(-)
--
2.23.0
Implement i2c bus recovery when slaves devices might hold SDA low.
In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
until the slave release SDA.
Signed-off-by: Kamel Bouhara <[email protected]>
---
drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
drivers/i2c/busses/i2c-at91.h | 8 ++++
2 files changed, 71 insertions(+)
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index a3fcc35ffd3b..df5bb93f952d 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -18,11 +18,13 @@
#include <linux/dma-mapping.h>
#include <linux/dmaengine.h>
#include <linux/err.h>
+#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/platform_device.h>
#include <linux/platform_data/dma-atmel.h>
#include <linux/pm_runtime.h>
@@ -768,6 +770,63 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
return ret;
}
+static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
+{
+ struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+
+ pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
+}
+
+static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
+{
+ struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+
+ pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
+}
+
+static int at91_init_twi_recovery_info(struct platform_device *pdev,
+ struct at91_twi_dev *dev)
+{
+ struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+ dev->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
+ dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
+ return PTR_ERR(dev->pinctrl);
+ }
+
+ dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
+ PINCTRL_STATE_DEFAULT);
+ dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
+ "gpio");
+ rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
+ if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
+ GPIOD_OUT_HIGH_OPEN_DRAIN);
+ if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ if (IS_ERR(rinfo->sda_gpiod) ||
+ IS_ERR(rinfo->scl_gpiod) ||
+ IS_ERR(dev->pinctrl_pins_default) ||
+ IS_ERR(dev->pinctrl_pins_gpio)) {
+ dev_info(&pdev->dev, "recovery information incomplete\n");
+ return -EINVAL;
+ }
+
+ dev_info(&pdev->dev, "using scl%s for recovery\n",
+ rinfo->sda_gpiod ? ",sda" : "");
+
+ rinfo->prepare_recovery = at91_prepare_twi_recovery;
+ rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
+ rinfo->recover_bus = i2c_generic_scl_recovery;
+ dev->adapter.bus_recovery_info = rinfo;
+
+ return 0;
+}
+
int at91_twi_probe_master(struct platform_device *pdev,
u32 phy_addr, struct at91_twi_dev *dev)
{
@@ -795,6 +854,10 @@ int at91_twi_probe_master(struct platform_device *pdev,
at91_calc_twi_clock(dev);
+ rc = at91_init_twi_recovery_info(pdev, dev);
+ if (rc == -EPROBE_DEFER)
+ return rc;
+
dev->adapter.algo = &at91_twi_algorithm;
dev->adapter.quirks = &at91_twi_quirks;
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
index 499b506f6128..b89dab55e776 100644
--- a/drivers/i2c/busses/i2c-at91.h
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -141,6 +141,10 @@ struct at91_twi_dev {
u32 fifo_size;
struct at91_twi_dma dma;
bool slave_detected;
+ struct i2c_bus_recovery_info rinfo;
+ struct pinctrl *pinctrl;
+ struct pinctrl_state *pinctrl_pins_default;
+ struct pinctrl_state *pinctrl_pins_gpio;
#ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
unsigned smr;
struct i2c_client *slave;
@@ -158,6 +162,10 @@ void at91_init_twi_bus_master(struct at91_twi_dev *dev);
int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
struct at91_twi_dev *dev);
+void at91_twi_prepare_recovery(struct i2c_adapter *adap);
+void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
+void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
+
#ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
--
2.23.0
Add the i2c gpio pinctrls to support the i2c bus recovery
Signed-off-by: Kamel Bouhara <[email protected]>
---
arch/arm/boot/dts/sama5d3.dtsi | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index f770aace0efd..faf8907d8d7d 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -159,8 +159,11 @@
dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(7)>,
<&dma0 2 AT91_DMA_CFG_PER_ID(8)>;
dma-names = "tx", "rx";
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c0>;
+ pinctrl-1 = <&pinctrl_i2c0_gpio>;
+ sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+ scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
#address-cells = <1>;
#size-cells = <0>;
clocks = <&twi0_clk>;
@@ -174,8 +177,11 @@
dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(9)>,
<&dma0 2 AT91_DMA_CFG_PER_ID(10)>;
dma-names = "tx", "rx";
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c1>;
+ pinctrl-1 = <&pinctrl_i2c1_gpio>;
+ sda-gpios = <&pioC 26 GPIO_ACTIVE_HIGH>;
+ scl-gpios = <&pioC 27 GPIO_ACTIVE_HIGH>;
#address-cells = <1>;
#size-cells = <0>;
clocks = <&twi1_clk>;
@@ -357,8 +363,11 @@
dmas = <&dma1 2 AT91_DMA_CFG_PER_ID(11)>,
<&dma1 2 AT91_DMA_CFG_PER_ID(12)>;
dma-names = "tx", "rx";
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c2>;
+ pinctrl-1 = <&pinctrl_i2c2_gpio>;
+ sda-gpios = <&pioA 18 GPIO_ACTIVE_HIGH>;
+ scl-gpios = <&pioA 19 GPIO_ACTIVE_HIGH>;
#address-cells = <1>;
#size-cells = <0>;
clocks = <&twi2_clk>;
@@ -639,6 +648,12 @@
<AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE /* PA30 periph A TWD0 pin, conflicts with URXD1, ISI_VSYNC */
AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>; /* PA31 periph A TWCK0 pin, conflicts with UTXD1, ISI_HSYNC */
};
+
+ pinctrl_i2c0_gpio: i2c0-gpio {
+ atmel,pins =
+ <AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+ AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+ };
};
i2c1 {
@@ -647,6 +662,12 @@
<AT91_PIOC 26 AT91_PERIPH_B AT91_PINCTRL_NONE /* PC26 periph B TWD1 pin, conflicts with SPI1_NPCS1, ISI_D11 */
AT91_PIOC 27 AT91_PERIPH_B AT91_PINCTRL_NONE>; /* PC27 periph B TWCK1 pin, conflicts with SPI1_NPCS2, ISI_D10 */
};
+
+ pinctrl_i2c1_gpio: i2c1-gpio {
+ atmel,pins =
+ <AT91_PIOC 26 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+ AT91_PIOC 27 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+ };
};
i2c2 {
@@ -655,6 +676,12 @@
<AT91_PIOA 18 AT91_PERIPH_B AT91_PINCTRL_NONE /* TWD2 pin, conflicts with LCDDAT18, ISI_D2 */
AT91_PIOA 19 AT91_PERIPH_B AT91_PINCTRL_NONE>; /* TWCK2 pin, conflicts with LCDDAT19, ISI_D3 */
};
+
+ pinctrl_i2c2_gpio: i2c2-gpio {
+ atmel,pins =
+ <AT91_PIOA 18 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+ AT91_PIOA 19 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+ };
};
isi {
--
2.23.0
Add the i2c gpio pinctrls so the i2c bus recovery option can be enabled
Signed-off-by: Kamel Bouhara <[email protected]>
---
arch/arm/boot/dts/sama5d4.dtsi | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 6ab27a7b388d..34351baab985 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -458,8 +458,11 @@
(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
| AT91_XDMAC_DT_PERID(3))>;
dma-names = "tx", "rx";
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c0>;
+ pinctrl-1 = <&pinctrl_i2c0_gpio>;
+ sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+ scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
#address-cells = <1>;
#size-cells = <0>;
clocks = <&pmc PMC_TYPE_PERIPHERAL 32>;
@@ -477,8 +480,11 @@
(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
| AT91_XDMAC_DT_PERID(5))>;
dma-names = "tx", "rx";
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c1>;
+ pinctrl-1 = <&pinctrl_i2c1_gpio>;
+ sda-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>;
+ scl-gpios = <&pioE 30 GPIO_ACTIVE_HIGH>;
#address-cells = <1>;
#size-cells = <0>;
clocks = <&pmc PMC_TYPE_PERIPHERAL 33>;
@@ -519,8 +525,11 @@
(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
| AT91_XDMAC_DT_PERID(7))>;
dma-names = "tx", "rx";
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c2>;
+ pinctrl-1 = <&pinctrl_i2c2_gpio>;
+ sda-gpios = <&pioB 29 GPIO_ACTIVE_HIGH>;
+ scl-gpios = <&pioB 30 GPIO_ACTIVE_HIGH>;
#address-cells = <1>;
#size-cells = <0>;
clocks = <&pmc PMC_TYPE_PERIPHERAL 34>;
@@ -1122,6 +1131,12 @@
<AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE
AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>;
};
+
+ pinctrl_i2c0_gpio: i2c0-gpio {
+ atmel,pins =
+ <AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+ AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+ };
};
i2c1 {
@@ -1130,6 +1145,12 @@
<AT91_PIOE 29 AT91_PERIPH_C AT91_PINCTRL_NONE /* TWD1, conflicts with UART0 RX and DIBP */
AT91_PIOE 30 AT91_PERIPH_C AT91_PINCTRL_NONE>; /* TWCK1, conflicts with UART0 TX and DIBN */
};
+
+ pinctrl_i2c1_gpio: i2c1-gpio {
+ atmel,pins =
+ <AT91_PIOE 29 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+ AT91_PIOE 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+ };
};
i2c2 {
@@ -1138,6 +1159,12 @@
<AT91_PIOB 29 AT91_PERIPH_A AT91_PINCTRL_NONE /* TWD2, conflicts with RD0 and PWML1 */
AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE>; /* TWCK2, conflicts with RF0 */
};
+
+ pinctrl_i2c2_gpio: i2c2-gpio {
+ atmel,pins =
+ <AT91_PIOB 29 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+ AT91_PIOB 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+ };
};
isi {
--
2.23.0
The at91 I2C controller can support bus recovery by re-assigning SCL
and SDA to gpios. Add the optional pinctrl and gpio properties to do
so.
Signed-off-by: Kamel Bouhara <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c-at91.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
index b7cec17c3daf..8ea2ce5d8610 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
@@ -19,8 +19,13 @@ Optional properties:
capable I2C controllers.
- i2c-sda-hold-time-ns: TWD hold time, only available for "atmel,sama5d4-i2c"
and "atmel,sama5d2-i2c".
+- scl-gpios: specify the gpio related to SCL pin
+- sda-gpios: specify the gpio related to SDA pin
+- pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
+ bus recovery, call it "gpio" state
- Child nodes conforming to i2c bus binding
+
Examples :
i2c0: i2c@fff84000 {
@@ -55,6 +60,11 @@ i2c0: i2c@f8034600 {
clocks = <&flx0>;
atmel,fifo-size = <16>;
i2c-sda-hold-time-ns = <336>;
+ pinctrl-names = "default", "gpio";
+ pinctrl-0 = <&pinctrl_i2c0>;
+ pinctrl-1 = <&pinctrl_i2c0_gpio>;
+ sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+ scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
wm8731: wm8731@1a {
compatible = "wm8731";
--
2.23.0
Hi Kamel,
On 02.10.2019 17:46, Kamel Bouhara wrote:
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> + struct at91_twi_dev *dev)
> +{
> + struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> + dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
You may use IS_ERR_OR_NULL() here.
> + dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> + return PTR_ERR(dev->pinctrl);
> + }
> +
On Fri, Oct 04, 2019 at 09:35:23AM +0000, [email protected] wrote:
> Hi Kamel,
>
> On 02.10.2019 17:46, Kamel Bouhara wrote:
> > +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> > + struct at91_twi_dev *dev)
> > +{
> > + struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> > +
> > + dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> > + if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
>
> You may use IS_ERR_OR_NULL() here.
Can devm_pinctrl_get return NULL? From a quick look, it cannot.
rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
value semantics.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 04.10.2019 23:39, Uwe Kleine-König wrote:
> External E-Mail
>
>
> On Fri, Oct 04, 2019 at 09:35:23AM +0000, [email protected] wrote:
>> Hi Kamel,
>>
>> On 02.10.2019 17:46, Kamel Bouhara wrote:
>>> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
>>> + struct at91_twi_dev *dev)
>>> +{
>>> + struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>>> +
>>> + dev->pinctrl = devm_pinctrl_get(&pdev->dev);
>>> + if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
>>
>> You may use IS_ERR_OR_NULL() here.
>
> Can devm_pinctrl_get return NULL? From a quick look, it cannot.
Looking quickly though it, yes, it seems it can't.
>
> rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
> value semantics.
>
> Best regards
> Uwe
>
On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> External E-Mail
>
>
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
>
Hi Kamel,
Thanks for adding this new feature. As I see patches only for sama5d3 and
sama5d4, I assume it has not been tested with a sama5d2, isn't it?
I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
work if we add .strict = true to pinmux_ops which is something plan for the
future...
Are you able to test these points? It would be nice to be aware of
possible side effects.
Regards
Ludovic
> Signed-off-by: Kamel Bouhara <[email protected]>
> ---
> drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
> drivers/i2c/busses/i2c-at91.h | 8 ++++
> 2 files changed, 71 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index a3fcc35ffd3b..df5bb93f952d 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -18,11 +18,13 @@
> #include <linux/dma-mapping.h>
> #include <linux/dmaengine.h>
> #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
> #include <linux/platform_device.h>
> #include <linux/platform_data/dma-atmel.h>
> #include <linux/pm_runtime.h>
> @@ -768,6 +770,63 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
> return ret;
> }
>
> +static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
> +{
> + struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> + pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> +}
> +
> +static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
> +{
> + struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> + pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> +}
> +
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> + struct at91_twi_dev *dev)
> +{
> + struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> + dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
> + dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> + return PTR_ERR(dev->pinctrl);
> + }
> +
> + dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> + PINCTRL_STATE_DEFAULT);
> + dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> + "gpio");
> + rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> + if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
> + GPIOD_OUT_HIGH_OPEN_DRAIN);
> + if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + if (IS_ERR(rinfo->sda_gpiod) ||
> + IS_ERR(rinfo->scl_gpiod) ||
> + IS_ERR(dev->pinctrl_pins_default) ||
> + IS_ERR(dev->pinctrl_pins_gpio)) {
> + dev_info(&pdev->dev, "recovery information incomplete\n");
> + return -EINVAL;
> + }
> +
> + dev_info(&pdev->dev, "using scl%s for recovery\n",
> + rinfo->sda_gpiod ? ",sda" : "");
> +
> + rinfo->prepare_recovery = at91_prepare_twi_recovery;
> + rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
> + rinfo->recover_bus = i2c_generic_scl_recovery;
> + dev->adapter.bus_recovery_info = rinfo;
> +
> + return 0;
> +}
> +
> int at91_twi_probe_master(struct platform_device *pdev,
> u32 phy_addr, struct at91_twi_dev *dev)
> {
> @@ -795,6 +854,10 @@ int at91_twi_probe_master(struct platform_device *pdev,
>
> at91_calc_twi_clock(dev);
>
> + rc = at91_init_twi_recovery_info(pdev, dev);
> + if (rc == -EPROBE_DEFER)
> + return rc;
> +
> dev->adapter.algo = &at91_twi_algorithm;
> dev->adapter.quirks = &at91_twi_quirks;
>
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> index 499b506f6128..b89dab55e776 100644
> --- a/drivers/i2c/busses/i2c-at91.h
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -141,6 +141,10 @@ struct at91_twi_dev {
> u32 fifo_size;
> struct at91_twi_dma dma;
> bool slave_detected;
> + struct i2c_bus_recovery_info rinfo;
> + struct pinctrl *pinctrl;
> + struct pinctrl_state *pinctrl_pins_default;
> + struct pinctrl_state *pinctrl_pins_gpio;
> #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
> unsigned smr;
> struct i2c_client *slave;
> @@ -158,6 +162,10 @@ void at91_init_twi_bus_master(struct at91_twi_dev *dev);
> int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
> struct at91_twi_dev *dev);
>
> +void at91_twi_prepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
> +
> #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
> void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
> int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
> --
> 2.23.0
>
>
On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > External E-Mail
> >
> >
> > Implement i2c bus recovery when slaves devices might hold SDA low.
> > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > until the slave release SDA.
> >
>
> Hi Kamel,
>
> Thanks for adding this new feature. As I see patches only for sama5d3 and
> sama5d4, I assume it has not been tested with a sama5d2, isn't it?
>
I there a point having it on sama5d2 as the controller already supports
this feature?
> I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> work if we add .strict = true to pinmux_ops which is something plan for the
> future...
>
I don't see why it wouldn't work with strict as this is switching muxing
properly instead of using the pins for two functions at the same time.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Oct 09, 2019 at 04:01:47PM +0200, Alexandre Belloni wrote:
>
> On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> > On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > > External E-Mail
> > >
> > >
> > > Implement i2c bus recovery when slaves devices might hold SDA low.
> > > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > > until the slave release SDA.
> > >
> >
> > Hi Kamel,
> >
> > Thanks for adding this new feature. As I see patches only for sama5d3 and
> > sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> >
>
> I there a point having it on sama5d2 as the controller already supports
> this feature?
>
Right, I was focused on pinctrl and forget we have this feature
supported by the IP.
> > I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> > work if we add .strict = true to pinmux_ops which is something plan for the
> > future...
> >
>
> I don't see why it wouldn't work with strict as this is switching muxing
> properly instead of using the pins for two functions at the same time.
>
Not sure devm_gpiod_get won't fail with strict.
Ludovic
On Wed, Oct 02, 2019 at 04:46:54PM +0200, Kamel Bouhara wrote:
> This patch series introduce the kernel i2c-gpio bus recovery mechanism
> for the Microchip SoCs. Updated the corresponding dts to add i2c
> gpio pinctrl. The bus recovery is configured for the sama5d3/4 xplained
> boards in dts.
Now we have 2 drivers with the same binding and code for using GPIO for
bus recovery. Perhaps all this should be common.
Rob
On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
>
> Signed-off-by: Kamel Bouhara <[email protected]>
Setting up the bus_recovery looks OK. However, I don't see any call to
i2c_recover_bus(), so the bus_recovery is never used. Did you test this
and see an effect?
Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
clear command if SCL or SDA is down" into this series. The crucial thing
for both is when to apply the recovery (at the beginning of a
transfer!). The rest is "just" that some HW needs a bus_recovery_info
for pinctrl/GPIO handling (from this patch), while other HW needs a
bus_recovery_info with a custom recover_bus callback.
Opinions?
> So at the beginning of a new transfer, we should check if SDA (or SCL?)
> is low and, if it's true, only then we should try recover the bus.
Yes, this is the proper time to do it. Remember, I2C does not define a
timeout.
On 22.10.2019 10:59, Kamel Bouhara wrote:
> On 21/10/2019 22:20, Wolfram Sang wrote:
>> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
>>> Implement i2c bus recovery when slaves devices might hold SDA low.
>>> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
>>> until the slave release SDA.
>>>
>>> Signed-off-by: Kamel Bouhara <[email protected]>
>>
>> Setting up the bus_recovery looks OK. However, I don't see any call to
>> i2c_recover_bus(), so the bus_recovery is never used. Did you test this
>> and see an effect?
>>
> Indeed, I guess I mess it up while doing some git stuff, it should be
> called from at91_do_twi_transfer() when the transfer times out...
> I actually tested it and verified the recovery is triggered by pulling
> the SCL to the ground ...
>
>> Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
>> clear command if SCL or SDA is down" into this series. The crucial thing
>> for both is when to apply the recovery (at the beginning of a
>> transfer!). The rest is "just" that some HW needs a bus_recovery_info
>> for pinctrl/GPIO handling (from this patch), while other HW needs a
>> bus_recovery_info with a custom recover_bus callback.
>>
>> Opinions?
>>
> I'm OK to merge the two series.
So at the beginning of a new transfer, we should check if SDA (or SCL?)
is low and, if it's true, only then we should try recover the bus.
Kamel, let me know if I can help with anything.
Best regards,
Codrin
On 24/10/2019 23:07, Wolfram Sang wrote:
>
>> So at the beginning of a new transfer, we should check if SDA (or SCL?)
>> is low and, if it's true, only then we should try recover the bus.
>
> Yes, this is the proper time to do it. Remember, I2C does not define a
> timeout.
>
FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.
I'm trying to fix the designware drivers handling of this at the moment.
--
Regards
Phil Reid
Hi Phil,
yes, this thread is old but a similar issue came up again...
On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid wrote:
> >
> > > So at the beginning of a new transfer, we should check if SDA (or SCL?)
> > > is low and, if it's true, only then we should try recover the bus.
> >
> > Yes, this is the proper time to do it. Remember, I2C does not define a
> > timeout.
> >
>
> FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
> Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.
>
> I'm trying to fix the designware drivers handling of this at the moment.
I wonder what you ended up with? You are right, a single poll is not
enough. It only might be if one applies the new "single-master" binding
for a given bus. If that is not present, my best idea so far is to poll
SDA for the time defined in adapter->timeout and if it is all low, then
initiate a recovery.
All the best,
Wolfram
On 25/08/2020 21:28, Wolfram Sang wrote:
> Hi Phil,
>
> yes, this thread is old but a similar issue came up again...
>
> On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid wrote:
>
>>>
>>>> So at the beginning of a new transfer, we should check if SDA (or SCL?)
>>>> is low and, if it's true, only then we should try recover the bus.
>>>
>>> Yes, this is the proper time to do it. Remember, I2C does not define a
>>> timeout.
>>>
>>
>> FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
>> Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.
>>
>> I'm trying to fix the designware drivers handling of this at the moment.
>
> I wonder what you ended up with? You are right, a single poll is not
> enough. It only might be if one applies the new "single-master" binding
> for a given bus. If that is not present, my best idea so far is to poll
> SDA for the time defined in adapter->timeout and if it is all low, then
> initiate a recovery.
>
On my todo list still.
Our system eventually recovers at the moment and the multi-master bus
doesn't contain anything that's time critical to our systems operation.
--
Regards
Phil Reid
ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
http://www.electromag.com.au
3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: [email protected]