GPIO recovery has been added already for some I2C bus drivers, such as
imx, pxa and at91. These drivers use similar bindings and have more or
less the same code for recovery. For this reason, we aim to move the
GPIO bus recovery implementation to the I2C core so that other drivers
can benefit from it, with small modifications.
This implementation initializes the pinctrl states and the SDA/SCL
GPIOs based on common bindings. The I2C bus drivers can still use
different bindings or other particular recovery steps if needed.
The ugly part with this patch series is the handle of PROBE_DEFER
which could be returned by devm_gpiod_get(). This changes things a
little for i2c_register_adapter() and for this reason this step is
implemented in a sperate patch.
The at91 Microchip driver is the first to use this implementation,
with an AI to move the rest of the drivers in the following steps.
Codrin Ciubotariu (4):
dt-binding: i2c: add generic properties for GPIO bus recovery
i2c: core: add generic I2C GPIO recovery
i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs
i2c: at91: Move to generic GPIO bus recovery
Documentation/devicetree/bindings/i2c/i2c.txt | 10 ++
drivers/i2c/busses/i2c-at91-master.c | 69 +--------
drivers/i2c/i2c-core-base.c | 139 +++++++++++++++++-
include/linux/i2c.h | 11 ++
4 files changed, 158 insertions(+), 71 deletions(-)
--
2.25.1
Multiple I2C bus drivers use similar bindings to obtain information needed
for I2C recovery. For example, for platforms using device-tree, the
properties look something like this:
&i2c {
...
pinctrl-names = "default", "gpio";
// or pinctrl-names = "default", "recovery";
pinctrl-0 = <&pinctrl_i2c_default>;
pinctrl-1 = <&pinctrl_i2c_gpio>;
sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>;
scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
...
}
For this reason, we can add this common initialization in the core. This
way, other I2C bus drivers will be able to support GPIO recovery just by
providing a pointer to platform's pinctrl and calling i2c_recover_bus()
when SDA is stuck low.
Signed-off-by: Codrin Ciubotariu <[email protected]>
---
drivers/i2c/i2c-core-base.c | 119 ++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 11 ++++
2 files changed, 130 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index d1f278f73011..4ee29fec4e93 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -32,6 +32,7 @@
#include <linux/of_device.h>
#include <linux/of.h>
#include <linux/of_irq.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
#include <linux/pm_wakeirq.h>
@@ -179,6 +180,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
int i = 0, scl = 1, ret = 0;
+ if (bri->pinctrl)
+ pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
if (bri->prepare_recovery)
bri->prepare_recovery(adap);
@@ -236,6 +239,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
if (bri->unprepare_recovery)
bri->unprepare_recovery(adap);
+ if (bri->pinctrl)
+ pinctrl_select_state(bri->pinctrl, bri->pins_default);
return ret;
}
@@ -251,6 +256,118 @@ int i2c_recover_bus(struct i2c_adapter *adap)
}
EXPORT_SYMBOL_GPL(i2c_recover_bus);
+static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
+{
+ struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+ struct device *dev = &adap->dev;
+ struct pinctrl *p = bri->pinctrl;
+
+ /*
+ * we can't change states without pinctrl, so remove the states if
+ * available
+ */
+ if (!p) {
+ bri->pins_default = NULL;
+ bri->pins_gpio = NULL;
+ return;
+ }
+
+ if (!bri->pins_default) {
+ bri->pins_default = pinctrl_lookup_state(p,
+ PINCTRL_STATE_DEFAULT);
+ if (IS_ERR(bri->pins_default)) {
+ dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
+ bri->pins_default = NULL;
+
+ goto cleanup_pinctrl;
+ }
+ }
+ if (!bri->pins_gpio) {
+ bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
+ if (IS_ERR(bri->pins_gpio))
+ bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
+
+ if (IS_ERR(bri->pins_gpio)) {
+ dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
+ bri->pins_gpio = NULL;
+
+ goto cleanup_pinctrl;
+ }
+ }
+
+cleanup_pinctrl:
+ /* for pinctrl state changes, we need all the information */
+ if (!bri->pins_default || !bri->pins_gpio) {
+ bri->pinctrl = NULL;
+ bri->pins_default = NULL;
+ bri->pins_gpio = NULL;
+ } else {
+ dev_info(dev, "using pinctrl states for GPIO recovery");
+ }
+}
+
+static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
+{
+ struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+ struct device *dev = &adap->dev;
+ struct gpio_desc *gpiod;
+ int ret = 0;
+
+ /* don't touch the recovery information if the driver is not using
+ * generic SCL recovery
+ */
+ if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
+ return 0;
+
+ /*
+ * pins might be taken as GPIO, so we might as well inform pinctrl about
+ * this and move the state to GPIO
+ */
+ if (bri->pinctrl)
+ pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
+
+ /*
+ * if there is incomplete or no recovery information, see if generic
+ * GPIO recovery is available
+ */
+ if (!bri->scl_gpiod) {
+ gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
+ if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto cleanup_pinctrl_state;
+ }
+ if (!IS_ERR(gpiod)) {
+ bri->scl_gpiod = gpiod;
+ bri->recover_bus = i2c_generic_scl_recovery;
+ dev_info(dev, "using generic GPIOs for recovery\n");
+ }
+ }
+
+ /* SDA GPIOD line is optional, so we care about DEFER only */
+ if (!bri->sda_gpiod) {
+ gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);
+ if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto cleanup_pinctrl_state;
+ }
+ if (!IS_ERR(gpiod))
+ bri->sda_gpiod = gpiod;
+ }
+
+cleanup_pinctrl_state:
+ /* change the state of the pins back to their default state */
+ if (bri->pinctrl)
+ pinctrl_select_state(bri->pinctrl, bri->pins_default);
+
+ return ret;
+}
+
+static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
+{
+ i2c_gpio_init_pinctrl_recovery(adap);
+ return i2c_gpio_init_generic_recovery(adap);
+}
+
static void i2c_init_recovery(struct i2c_adapter *adap)
{
struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
@@ -259,6 +376,8 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
if (!bri)
return;
+ i2c_gpio_init_recovery(adap);
+
if (!bri->recover_bus) {
err_str = "no recover_bus() found";
goto err;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index c10617bb980a..c62c9b48f719 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -611,6 +611,14 @@ struct i2c_timings {
* may configure padmux here for SDA/SCL line or something else they want.
* @scl_gpiod: gpiod of the SCL line. Only required for GPIO recovery.
* @sda_gpiod: gpiod of the SDA line. Only required for GPIO recovery.
+ * @pinctrl: pinctrl used by GPIO recovery to change the state of the I2C pins.
+ * Optional.
+ * @pins_default: default state of SCL/SDA lines, when they are assigned to the
+ * I2C bus. Optional. Populated internally for GPIO recovery, if a state with
+ * the name PINCTRL_STATE_DEFAULT is found and pinctrl is valid.
+ * @pins_gpio: recovery state of SCL/SDA lines, when they are used as GPIOs.
+ * Optional. Populated internally for GPIO recovery, if this state is called
+ * "gpio" or "recovery" and pinctrl is valid.
*/
struct i2c_bus_recovery_info {
int (*recover_bus)(struct i2c_adapter *adap);
@@ -627,6 +635,9 @@ struct i2c_bus_recovery_info {
/* gpio recovery */
struct gpio_desc *scl_gpiod;
struct gpio_desc *sda_gpiod;
+ struct pinctrl *pinctrl;
+ struct pinctrl_state *pins_default;
+ struct pinctrl_state *pins_gpio;
};
int i2c_recover_bus(struct i2c_adapter *adap);
--
2.25.1
The I2C GPIO bus recovery properties consist of two GPIOS and one extra
pinctrl state ("gpio" or "recovery"). Not all are mandatory for recovery.
Signed-off-by: Codrin Ciubotariu <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 438ae123107e..6a644a24fc1c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -77,6 +77,16 @@ wants to support one of the below features, it should adapt these bindings.
this information to detect a stalled bus more reliably, for example.
Can not be combined with 'multi-master'.
+- scl-gpios
+ specify the gpio related to SCL pin. Used for GPIO bus recovery.
+
+- sda-gpios
+ specify the gpio related to SDA pin. Optional for GPIO bus recovery.
+
+- pinctrl
+ add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
+ recovery, call it "gpio" or "recovery" state
+
Required properties (per child device)
--------------------------------------
--
2.25.1
Make the Microchip at91 driver the first to use the generic GPIO bus
recovery support from the I2C core and discard the driver implementation.
Signed-off-by: Codrin Ciubotariu <[email protected]>
---
drivers/i2c/busses/i2c-at91-master.c | 69 ++--------------------------
drivers/i2c/busses/i2c-at91.h | 3 --
2 files changed, 3 insertions(+), 69 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index 363d540a8345..66864f9cf7ac 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -816,79 +816,16 @@ 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_gpio(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)) {
+ rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
- return PTR_ERR(dev->pinctrl);
+ return PTR_ERR(rinfo->pinctrl);
}
-
- dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
- PINCTRL_STATE_DEFAULT);
- dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
- "gpio");
- if (IS_ERR(dev->pinctrl_pins_default) ||
- IS_ERR(dev->pinctrl_pins_gpio)) {
- dev_info(&pdev->dev, "pinctrl states incomplete for recovery\n");
- return -EINVAL;
- }
-
- /*
- * pins will be taken as GPIO, so we might as well inform pinctrl about
- * this and move the state to GPIO
- */
- pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_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)) {
- dev_info(&pdev->dev, "recovery information incomplete\n");
- if (!IS_ERR(rinfo->sda_gpiod)) {
- gpiod_put(rinfo->sda_gpiod);
- rinfo->sda_gpiod = NULL;
- }
- if (!IS_ERR(rinfo->scl_gpiod)) {
- gpiod_put(rinfo->scl_gpiod);
- rinfo->scl_gpiod = NULL;
- }
- pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
- return -EINVAL;
- }
-
- /* change the state of the pins back to their default state */
- pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
-
- dev_info(&pdev->dev, "using scl, sda for recovery\n");
-
- 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;
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
index 7e7b4955ca7f..eae673ae786c 100644
--- a/drivers/i2c/busses/i2c-at91.h
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -157,9 +157,6 @@ struct at91_twi_dev {
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;
--
2.25.1
Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
-EPROBE_DEFER, so we should at least treat that. This ends up with
i2c_register_adapter() to be able to return -EPROBE_DEFER.
Signed-off-by: Codrin Ciubotariu <[email protected]>
---
drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 4ee29fec4e93..f8d9f2048ca8 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
return i2c_gpio_init_generic_recovery(adap);
}
-static void i2c_init_recovery(struct i2c_adapter *adap)
+static int i2c_init_recovery(struct i2c_adapter *adap)
{
struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
char *err_str;
if (!bri)
- return;
+ return 0;
- i2c_gpio_init_recovery(adap);
+ if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
if (!bri->recover_bus) {
err_str = "no recover_bus() found";
@@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
if (gpiod_get_direction(bri->sda_gpiod) == 0)
bri->set_sda = set_sda_gpio_value;
}
- return;
+ return 0;
}
if (bri->recover_bus == i2c_generic_scl_recovery) {
@@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
}
}
- return;
+ return 0;
err:
dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
adap->bus_recovery_info = NULL;
+
+ return 0;
}
static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
@@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
"Failed to create compatibility class link\n");
#endif
- i2c_init_recovery(adap);
+ res = i2c_init_recovery(adap);
+ if (res == -EPROBE_DEFER)
+ goto out_link;
/* create pre-declared device nodes */
of_i2c_register_devices(adap);
@@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
return 0;
+out_link:
+#ifdef CONFIG_I2C_COMPAT
+ class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
+ adap->dev.parent);
+#endif
out_reg:
init_completion(&adap->dev_released);
device_unregister(&adap->dev);
--
2.25.1
On Fri, Jun 19, 2020 at 05:19:00PM +0300, Codrin Ciubotariu wrote:
> GPIO recovery has been added already for some I2C bus drivers, such as
> imx, pxa and at91. These drivers use similar bindings and have more or
> less the same code for recovery. For this reason, we aim to move the
> GPIO bus recovery implementation to the I2C core so that other drivers
> can benefit from it, with small modifications.
> This implementation initializes the pinctrl states and the SDA/SCL
> GPIOs based on common bindings. The I2C bus drivers can still use
> different bindings or other particular recovery steps if needed.
> The ugly part with this patch series is the handle of PROBE_DEFER
> which could be returned by devm_gpiod_get(). This changes things a
> little for i2c_register_adapter() and for this reason this step is
> implemented in a sperate patch.
> The at91 Microchip driver is the first to use this implementation,
> with an AI to move the rest of the drivers in the following steps.
Thanks for doing this! On a first high level review, this looks very
good. I have one question in patch 1. From Tuesday on, I'll be
off-the-net for two weeks. Still I think it will be all good for the 5.9
merge window unless we encounter an unexpected problem. But as said, so
far it is looking good!
> +- pinctrl
> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> + recovery, call it "gpio" or "recovery" state
I think we should stick with "gpio" only. That is what at91 and imx have
in their bindings. pxa uses "recovery" as a pinctrl state name but I
can't find any further use or documentation of that. PXA is not fully
converted to the best of my knowledge, so maybe it is no problem for PXA
to switch to "gpio", too? We should ask Russell King (cced).
Russell, do you object naming the pinctrl state for bus recovery in
the pxa i2c driver from "recovery" to "gpio"?
On Fri, 19 Jun 2020 17:19:01 +0300, Codrin Ciubotariu wrote:
> The I2C GPIO bus recovery properties consist of two GPIOS and one extra
> pinctrl state ("gpio" or "recovery"). Not all are mandatory for recovery.
>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
> ---
> Documentation/devicetree/bindings/i2c/i2c.txt | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>
> > +- pinctrl
> > + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> > + recovery, call it "gpio" or "recovery" state
>
> I think we should stick with "gpio" only. That is what at91 and imx have
> in their bindings. pxa uses "recovery" as a pinctrl state name but I
> can't find any further use or documentation of that. PXA is not fully
> converted to the best of my knowledge, so maybe it is no problem for PXA
> to switch to "gpio", too? We should ask Russell King (cced).
>
> Russell, do you object naming the pinctrl state for bus recovery in
> the pxa i2c driver from "recovery" to "gpio"?
No response, so far. I suggest now to support the "recovery" naming but
mark it as deprecated. Opinions?
On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
> >
> > > +- pinctrl
> > > + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> > > + recovery, call it "gpio" or "recovery" state
> >
> > I think we should stick with "gpio" only. That is what at91 and imx have
> > in their bindings. pxa uses "recovery" as a pinctrl state name but I
> > can't find any further use or documentation of that. PXA is not fully
> > converted to the best of my knowledge, so maybe it is no problem for PXA
> > to switch to "gpio", too? We should ask Russell King (cced).
Fully converted to what? The generic handling where the i2c core layer
handles everything to do with recovery, including the switch between
modes?
i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
GPIO mode, and I don't see a generic driver doing that to avoid causing
any additional glitches on the bus. Given the use case that this recovery
is targetted at, avoiding glitches is very important to keep.
> > Russell, do you object naming the pinctrl state for bus recovery in
> > the pxa i2c driver from "recovery" to "gpio"?
>
> No response, so far. I suggest now to support the "recovery" naming but
> mark it as deprecated. Opinions?
I don't have a preference on the exact naming.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>>>
>>>> +- pinctrl
>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
>>>> + recovery, call it "gpio" or "recovery" state
>>>
>>> I think we should stick with "gpio" only. That is what at91 and imx have
>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
>>> can't find any further use or documentation of that. PXA is not fully
>>> converted to the best of my knowledge, so maybe it is no problem for PXA
>>> to switch to "gpio", too? We should ask Russell King (cced).
>
> Fully converted to what? The generic handling where the i2c core layer
> handles everything to do with recovery, including the switch between
> modes?
>
> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
> GPIO mode, and I don't see a generic driver doing that to avoid causing
> any additional glitches on the bus. Given the use case that this recovery
> is targetted at, avoiding glitches is very important to keep.
Why is it not possbile to handle glitches in a generic way? I guess it
depends on the pinctl, but we could treat a worst-case scenario to
assure the switch between states is done properly.
>
>>> Russell, do you object naming the pinctrl state for bus recovery in
>>> the pxa i2c driver from "recovery" to "gpio"?
>>
>> No response, so far. I suggest now to support the "recovery" naming but
>> mark it as deprecated. Opinions?
>
> I don't have a preference on the exact naming.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
>
On Mon, Jul 27, 2020 at 10:44:57AM +0000, [email protected] wrote:
> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
> >> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
> >>>
> >>>> +- pinctrl
> >>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> >>>> + recovery, call it "gpio" or "recovery" state
> >>>
> >>> I think we should stick with "gpio" only. That is what at91 and imx have
> >>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
> >>> can't find any further use or documentation of that. PXA is not fully
> >>> converted to the best of my knowledge, so maybe it is no problem for PXA
> >>> to switch to "gpio", too? We should ask Russell King (cced).
> >
> > Fully converted to what? The generic handling where the i2c core layer
> > handles everything to do with recovery, including the switch between
> > modes?
> >
> > i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
> > GPIO mode, and I don't see a generic driver doing that to avoid causing
> > any additional glitches on the bus. Given the use case that this recovery
> > is targetted at, avoiding glitches is very important to keep.
>
> Why is it not possbile to handle glitches in a generic way? I guess it
> depends on the pinctl, but we could treat a worst-case scenario to
> assure the switch between states is done properly.
Please look at how i2c-pxa switches between the two, and decide whether
the generic implementation can do the same.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 27.07.2020 13:50, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Mon, Jul 27, 2020 at 10:44:57AM +0000, [email protected] wrote:
>> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
>>>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>>>>>
>>>>>> +- pinctrl
>>>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
>>>>>> + recovery, call it "gpio" or "recovery" state
>>>>>
>>>>> I think we should stick with "gpio" only. That is what at91 and imx have
>>>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
>>>>> can't find any further use or documentation of that. PXA is not fully
>>>>> converted to the best of my knowledge, so maybe it is no problem for PXA
>>>>> to switch to "gpio", too? We should ask Russell King (cced).
>>>
>>> Fully converted to what? The generic handling where the i2c core layer
>>> handles everything to do with recovery, including the switch between
>>> modes?
>>>
>>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
>>> GPIO mode, and I don't see a generic driver doing that to avoid causing
>>> any additional glitches on the bus. Given the use case that this recovery
>>> is targetted at, avoiding glitches is very important to keep.
>>
>> Why is it not possbile to handle glitches in a generic way? I guess it
>> depends on the pinctl, but we could treat a worst-case scenario to
>> assure the switch between states is done properly.
>
> Please look at how i2c-pxa switches between the two, and decide whether
> the generic implementation can do the same.
The handling of glitches from initialization looks generic to me. I see
that there are specific clear/reset routines that are in the
(un)prepare_recovery() callbacks, but these callbacks are not replaced
by the generic i2c recovery and will still be used if given by the
driver. The only thing the generic recovery does is to switch the pinmux
state. We can discuss whether we want to change the pinmux state first
or call the (un)preapre_recovery().
What I had in mind for the generic recovery was to just handle the
common parts that follow the same bindings, which is getting the gpios
and changing the pinmux states before recovering.
Best regards,
Codrin
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
>
On Fri, Jun 19, 2020 at 05:19:02PM +0300, Codrin Ciubotariu wrote:
> Multiple I2C bus drivers use similar bindings to obtain information needed
> for I2C recovery. For example, for platforms using device-tree, the
> properties look something like this:
>
> &i2c {
> ...
> pinctrl-names = "default", "gpio";
> // or pinctrl-names = "default", "recovery";
> pinctrl-0 = <&pinctrl_i2c_default>;
> pinctrl-1 = <&pinctrl_i2c_gpio>;
> sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>;
> scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> ...
> }
>
> For this reason, we can add this common initialization in the core. This
> way, other I2C bus drivers will be able to support GPIO recovery just by
> providing a pointer to platform's pinctrl and calling i2c_recover_bus()
> when SDA is stuck low.
>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
Thanks, it looks a lot like what I had in mind!
> ---
> drivers/i2c/i2c-core-base.c | 119 ++++++++++++++++++++++++++++++++++++
> include/linux/i2c.h | 11 ++++
> 2 files changed, 130 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index d1f278f73011..4ee29fec4e93 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -32,6 +32,7 @@
> #include <linux/of_device.h>
> #include <linux/of.h>
> #include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> #include <linux/pm_domain.h>
> #include <linux/pm_runtime.h>
> #include <linux/pm_wakeirq.h>
> @@ -179,6 +180,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> int i = 0, scl = 1, ret = 0;
>
> + if (bri->pinctrl)
> + pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
I think this should come after 'prepare_recovery'. It may be that
'prepare_recovery' already needs to select the pinctrl state to avoid a
glitch. In this version, there would be a glitch then. If we move it
down, the doubled 'pinctrl_select_state' would be a noop then.
> if (bri->prepare_recovery)
> bri->prepare_recovery(adap);
>
> @@ -236,6 +239,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>
> if (bri->unprepare_recovery)
> bri->unprepare_recovery(adap);
> + if (bri->pinctrl)
> + pinctrl_select_state(bri->pinctrl, bri->pins_default);
Here it is OK and will still work with the PXA version which needs to
select the state on its own.
>
> return ret;
> }
> @@ -251,6 +256,118 @@ int i2c_recover_bus(struct i2c_adapter *adap)
> }
> EXPORT_SYMBOL_GPL(i2c_recover_bus);
>
> +static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> + struct device *dev = &adap->dev;
> + struct pinctrl *p = bri->pinctrl;
> +
> + /*
> + * we can't change states without pinctrl, so remove the states if
> + * available
s/available/populated/ ?
> + */
> + if (!p) {
> + bri->pins_default = NULL;
> + bri->pins_gpio = NULL;
> + return;
> + }
> +
> + if (!bri->pins_default) {
> + bri->pins_default = pinctrl_lookup_state(p,
> + PINCTRL_STATE_DEFAULT);
> + if (IS_ERR(bri->pins_default)) {
> + dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
> + bri->pins_default = NULL;
> +
> + goto cleanup_pinctrl;
I'd leave out the goto here. It is OK to check both parameters, I think.
> + }
> + }
> + if (!bri->pins_gpio) {
> + bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
> + if (IS_ERR(bri->pins_gpio))
> + bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
> +
> + if (IS_ERR(bri->pins_gpio)) {
> + dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
> + bri->pins_gpio = NULL;
> +
> + goto cleanup_pinctrl;
This goto is not needed...
> + }
> + }
> +
> +cleanup_pinctrl:
... and this label can go then. Also nicer to read, I'd say.
> + /* for pinctrl state changes, we need all the information */
> + if (!bri->pins_default || !bri->pins_gpio) {
> + bri->pinctrl = NULL;
> + bri->pins_default = NULL;
> + bri->pins_gpio = NULL;
> + } else {
> + dev_info(dev, "using pinctrl states for GPIO recovery");
> + }
> +}
> +
> +static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> + struct device *dev = &adap->dev;
> + struct gpio_desc *gpiod;
> + int ret = 0;
> +
> + /* don't touch the recovery information if the driver is not using
> + * generic SCL recovery
> + */
Not kernel comment style.
> + if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
> + return 0;
No need for the first condition. 'i2c_generic_scl_recovery' is
definately not NULL :)
> +
> + /*
> + * pins might be taken as GPIO, so we might as well inform pinctrl about
s/might as well/should/
> + * this and move the state to GPIO
> + */
> + if (bri->pinctrl)
> + pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
> +
> + /*
> + * if there is incomplete or no recovery information, see if generic
> + * GPIO recovery is available
> + */
> + if (!bri->scl_gpiod) {
> + gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
> + if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto cleanup_pinctrl_state;
> + }
> + if (!IS_ERR(gpiod)) {
> + bri->scl_gpiod = gpiod;
> + bri->recover_bus = i2c_generic_scl_recovery;
> + dev_info(dev, "using generic GPIOs for recovery\n");
> + }
> + }
I think this extra code from the PXA driver makes sense in case SDA was
released while we were executing this code:
1383 /*
1384 * We have SCL. Pull SCL low and wait a bit so that SDA glitches
1385 * have no effect.
1386 */
1387 gpiod_direction_output(bri->scl_gpiod, 0);
1388 udelay(10);
1389 bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
1390
1391 /* Wait a bit in case of a SDA glitch, and then release SCL. */
1392 udelay(10);
1393 gpiod_direction_output(bri->scl_gpiod, 1);
> +
> + /* SDA GPIOD line is optional, so we care about DEFER only */
> + if (!bri->sda_gpiod) {
> + gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);
> + if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto cleanup_pinctrl_state;
> + }
> + if (!IS_ERR(gpiod))
> + bri->sda_gpiod = gpiod;
> + }
> +
> +cleanup_pinctrl_state:
> + /* change the state of the pins back to their default state */
> + if (bri->pinctrl)
> + pinctrl_select_state(bri->pinctrl, bri->pins_default);
> +
> + return ret;
> +}
> +
Rest looks good! If you have some time for this now, I will make sure to
get it into 5.9. With these minor things fixed, this is good to go, me
thinks.
On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote:
> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
> -EPROBE_DEFER, so we should at least treat that. This ends up with
> i2c_register_adapter() to be able to return -EPROBE_DEFER.
>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
> ---
> drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 4ee29fec4e93..f8d9f2048ca8 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
> return i2c_gpio_init_generic_recovery(adap);
> }
>
> -static void i2c_init_recovery(struct i2c_adapter *adap)
> +static int i2c_init_recovery(struct i2c_adapter *adap)
> {
> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> char *err_str;
>
> if (!bri)
> - return;
> + return 0;
>
> - i2c_gpio_init_recovery(adap);
> + if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
>
> if (!bri->recover_bus) {
> err_str = "no recover_bus() found";
> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
> if (gpiod_get_direction(bri->sda_gpiod) == 0)
> bri->set_sda = set_sda_gpio_value;
> }
> - return;
> + return 0;
This is correct but I think the code flow is/was confusing. Can you drop
this 'return' and use 'else if' for the next code block? I think this is
more readable.
> }
>
> if (bri->recover_bus == i2c_generic_scl_recovery) {
> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
> }
> }
>
> - return;
> + return 0;
> err:
> dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
> adap->bus_recovery_info = NULL;
> +
> + return 0;
'return -EINVAL;' I'd suggest.
> }
>
> static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
> "Failed to create compatibility class link\n");
> #endif
>
> - i2c_init_recovery(adap);
> + res = i2c_init_recovery(adap);
> + if (res == -EPROBE_DEFER)
> + goto out_link;
Please move 'i2c_init_recovery' above the class-link creation. It
shouldn't make a difference but we can skip the extra label and the
ifdeffery.
>
> /* create pre-declared device nodes */
> of_i2c_register_devices(adap);
> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>
> return 0;
>
> +out_link:
> +#ifdef CONFIG_I2C_COMPAT
> + class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
> + adap->dev.parent);
> +#endif
> out_reg:
> init_completion(&adap->dev_released);
> device_unregister(&adap->dev);
> --
> 2.25.1
>
On Fri, Jun 19, 2020 at 05:19:04PM +0300, Codrin Ciubotariu wrote:
> Make the Microchip at91 driver the first to use the generic GPIO bus
> recovery support from the I2C core and discard the driver implementation.
>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
> ---
> drivers/i2c/busses/i2c-at91-master.c | 69 ++--------------------------
> drivers/i2c/busses/i2c-at91.h | 3 --
Nice diffstat! I will try using this new functionality with another
controller next week.
On 02.08.2020 19:54, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:02PM +0300, Codrin Ciubotariu wrote:
>> Multiple I2C bus drivers use similar bindings to obtain information needed
>> for I2C recovery. For example, for platforms using device-tree, the
>> properties look something like this:
>>
>> &i2c {
>> ...
>> pinctrl-names = "default", "gpio";
>> // or pinctrl-names = "default", "recovery";
>> pinctrl-0 = <&pinctrl_i2c_default>;
>> pinctrl-1 = <&pinctrl_i2c_gpio>;
>> sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>;
>> scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> ...
>> }
>>
>> For this reason, we can add this common initialization in the core. This
>> way, other I2C bus drivers will be able to support GPIO recovery just by
>> providing a pointer to platform's pinctrl and calling i2c_recover_bus()
>> when SDA is stuck low.
>>
>> Signed-off-by: Codrin Ciubotariu <[email protected]>
>
> Thanks, it looks a lot like what I had in mind!
>
>> ---
>> drivers/i2c/i2c-core-base.c | 119 ++++++++++++++++++++++++++++++++++++
>> include/linux/i2c.h | 11 ++++
>> 2 files changed, 130 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index d1f278f73011..4ee29fec4e93 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -32,6 +32,7 @@
>> #include <linux/of_device.h>
>> #include <linux/of.h>
>> #include <linux/of_irq.h>
>> +#include <linux/pinctrl/consumer.h>
>> #include <linux/pm_domain.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/pm_wakeirq.h>
>> @@ -179,6 +180,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> int i = 0, scl = 1, ret = 0;
>>
>> + if (bri->pinctrl)
>> + pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
>
> I think this should come after 'prepare_recovery'. It may be that
> 'prepare_recovery' already needs to select the pinctrl state to avoid a
> glitch. In this version, there would be a glitch then. If we move it
> down, the doubled 'pinctrl_select_state' would be a noop then.
Agree
>
>> if (bri->prepare_recovery)
>> bri->prepare_recovery(adap);
>>
>> @@ -236,6 +239,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>>
>> if (bri->unprepare_recovery)
>> bri->unprepare_recovery(adap);
>> + if (bri->pinctrl)
>> + pinctrl_select_state(bri->pinctrl, bri->pins_default);
>
> Here it is OK and will still work with the PXA version which needs to
> select the state on its own.
>
>>
>> return ret;
>> }
>> @@ -251,6 +256,118 @@ int i2c_recover_bus(struct i2c_adapter *adap)
>> }
>> EXPORT_SYMBOL_GPL(i2c_recover_bus);
>>
>> +static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
>> +{
>> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> + struct device *dev = &adap->dev;
>> + struct pinctrl *p = bri->pinctrl;
>> +
>> + /*
>> + * we can't change states without pinctrl, so remove the states if
>> + * available
>
> s/available/populated/ ?
OK
>
>> + */
>> + if (!p) {
>> + bri->pins_default = NULL;
>> + bri->pins_gpio = NULL;
>> + return;
>> + }
>> +
>> + if (!bri->pins_default) {
>> + bri->pins_default = pinctrl_lookup_state(p,
>> + PINCTRL_STATE_DEFAULT);
>> + if (IS_ERR(bri->pins_default)) {
>> + dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
>> + bri->pins_default = NULL;
>> +
>> + goto cleanup_pinctrl;
>
> I'd leave out the goto here. It is OK to check both parameters, I think.
since default state is missing, the next check can be skipped, but I
agree that removing the label makes things easier to read.
>
>> + }
>> + }
>> + if (!bri->pins_gpio) {
>> + bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
>> + if (IS_ERR(bri->pins_gpio))
>> + bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
>> +
>> + if (IS_ERR(bri->pins_gpio)) {
>> + dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
>> + bri->pins_gpio = NULL;
>> +
>> + goto cleanup_pinctrl;
>
> This goto is not needed...
right
>
>> + }
>> + }
>> +
>> +cleanup_pinctrl:
>
> ... and this label can go then. Also nicer to read, I'd say.
I will remove it.
>
>> + /* for pinctrl state changes, we need all the information */
>> + if (!bri->pins_default || !bri->pins_gpio) {
>> + bri->pinctrl = NULL;
>> + bri->pins_default = NULL;
>> + bri->pins_gpio = NULL;
>> + } else {
>> + dev_info(dev, "using pinctrl states for GPIO recovery");
>> + }
>> +}
>> +
>> +static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
>> +{
>> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> + struct device *dev = &adap->dev;
>> + struct gpio_desc *gpiod;
>> + int ret = 0;
>> +
>> + /* don't touch the recovery information if the driver is not using
>> + * generic SCL recovery
>> + */
>
> Not kernel comment style.
Right, sorry about this. Will fix.
>
>> + if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
>> + return 0;
>
> No need for the first condition. 'i2c_generic_scl_recovery' is
> definately not NULL :)
It is not the same thing. Without the first check, it will return 0 if
bri->recover_bus is NULL, which is not what we want. If bri->recover_bus
is NULL (and the pintrl states and gpios are in place) we can set
recover_bus to i2c_generic_scl_recovery and use the generic recovery.
>
>> +
>> + /*
>> + * pins might be taken as GPIO, so we might as well inform pinctrl about
>
> s/might as well/should/
OK
>
>> + * this and move the state to GPIO
>> + */
>> + if (bri->pinctrl)
>> + pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
>> +
>> + /*
>> + * if there is incomplete or no recovery information, see if generic
>> + * GPIO recovery is available
>> + */
>> + if (!bri->scl_gpiod) {
>> + gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
>> + if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
>> + ret = -EPROBE_DEFER;
>> + goto cleanup_pinctrl_state;
>> + }
>> + if (!IS_ERR(gpiod)) {
>> + bri->scl_gpiod = gpiod;
>> + bri->recover_bus = i2c_generic_scl_recovery;
>> + dev_info(dev, "using generic GPIOs for recovery\n");
>> + }
>> + }
>
> I think this extra code from the PXA driver makes sense in case SDA was
> released while we were executing this code:
>
> 1383 /*
> 1384 * We have SCL. Pull SCL low and wait a bit so that SDA glitches
> 1385 * have no effect.
> 1386 */
> 1387 gpiod_direction_output(bri->scl_gpiod, 0);
> 1388 udelay(10);
> 1389 bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
> 1390
> 1391 /* Wait a bit in case of a SDA glitch, and then release SCL. */
> 1392 udelay(10);
> 1393 gpiod_direction_output(bri->scl_gpiod, 1);
I agree. I will add it.
>
>> +
>> + /* SDA GPIOD line is optional, so we care about DEFER only */
>> + if (!bri->sda_gpiod) {
>> + gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);
>> + if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
>> + ret = -EPROBE_DEFER;
>> + goto cleanup_pinctrl_state;
>> + }
>> + if (!IS_ERR(gpiod))
>> + bri->sda_gpiod = gpiod;
>> + }
>> +
>> +cleanup_pinctrl_state:
>> + /* change the state of the pins back to their default state */
>> + if (bri->pinctrl)
>> + pinctrl_select_state(bri->pinctrl, bri->pins_default);
>> +
>> + return ret;
>> +}
>> +
>
> Rest looks good! If you have some time for this now, I will make sure to
> get it into 5.9. With these minor things fixed, this is good to go, me
> thinks.
>
I agree will all your suggestions, except with the removal of if
(bri->recover_bus) . If you agree with this, I can send the next version
and remove the RFC.
Thanks and best regards,
Codrin
On Thu, Jul 30, 2020 at 09:00:36AM +0000, [email protected] wrote:
> On 27.07.2020 13:50, Russell King - ARM Linux admin wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Mon, Jul 27, 2020 at 10:44:57AM +0000, [email protected] wrote:
> >> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
> >>>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
> >>>>>
> >>>>>> +- pinctrl
> >>>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> >>>>>> + recovery, call it "gpio" or "recovery" state
> >>>>>
> >>>>> I think we should stick with "gpio" only. That is what at91 and imx have
> >>>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
> >>>>> can't find any further use or documentation of that. PXA is not fully
> >>>>> converted to the best of my knowledge, so maybe it is no problem for PXA
> >>>>> to switch to "gpio", too? We should ask Russell King (cced).
> >>>
> >>> Fully converted to what? The generic handling where the i2c core layer
> >>> handles everything to do with recovery, including the switch between
> >>> modes?
> >>>
> >>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
> >>> GPIO mode, and I don't see a generic driver doing that to avoid causing
> >>> any additional glitches on the bus. Given the use case that this recovery
> >>> is targetted at, avoiding glitches is very important to keep.
> >>
> >> Why is it not possbile to handle glitches in a generic way? I guess it
> >> depends on the pinctl, but we could treat a worst-case scenario to
> >> assure the switch between states is done properly.
> >
> > Please look at how i2c-pxa switches between the two, and decide whether
> > the generic implementation can do the same.
>
> The handling of glitches from initialization looks generic to me. I see
> that there are specific clear/reset routines that are in the
> (un)prepare_recovery() callbacks, but these callbacks are not replaced
> by the generic i2c recovery and will still be used if given by the
> driver. The only thing the generic recovery does is to switch the pinmux
> state. We can discuss whether we want to change the pinmux state first
> or call the (un)preapre_recovery().
Right, the key point i2c-pxa does is that on prepare:
- read the current state of the SCL and SDA lines and set the GPIO to
reflect those values.
- then switch the pinmux state.
That must be preserved, otherwise if SCL is being held low by the I2C
master, and we switch to GPIO mode, SCL will be released. So the
driver needs to be involved before the pinmux state is changed.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 02.08.2020 20:05, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote:
>> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
>> -EPROBE_DEFER, so we should at least treat that. This ends up with
>> i2c_register_adapter() to be able to return -EPROBE_DEFER.
>>
>> Signed-off-by: Codrin Ciubotariu <[email protected]>
>> ---
>> drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 4ee29fec4e93..f8d9f2048ca8 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
>> return i2c_gpio_init_generic_recovery(adap);
>> }
>>
>> -static void i2c_init_recovery(struct i2c_adapter *adap)
>> +static int i2c_init_recovery(struct i2c_adapter *adap)
>> {
>> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> char *err_str;
>>
>> if (!bri)
>> - return;
>> + return 0;
>>
>> - i2c_gpio_init_recovery(adap);
>> + if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>>
>> if (!bri->recover_bus) {
>> err_str = "no recover_bus() found";
>> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>> if (gpiod_get_direction(bri->sda_gpiod) == 0)
>> bri->set_sda = set_sda_gpio_value;
>> }
>> - return;
>> + return 0;
>
> This is correct but I think the code flow is/was confusing. Can you drop
> this 'return' and use 'else if' for the next code block? I think this is
> more readable.
Ok, it makes sense. Should I make a separate patch for this only?
One more question, should we keep:
if (!bri->set_sda && !bri->get_sda) {
err_str = "either get_sda() or set_sda() needed";
goto err;
}
?
Without {get/set}_sda we won't be able to generate stop commands and
possibly check if the bus is free, but we can still generate the SCL
clock pulses.
>
>> }
>>
>> if (bri->recover_bus == i2c_generic_scl_recovery) {
>> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>> }
>> }
>>
>> - return;
>> + return 0;
>> err:
>> dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
>> adap->bus_recovery_info = NULL;
>> +
>> + return 0;
>
> 'return -EINVAL;' I'd suggest.
OK
>
>> }
>>
>> static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
>> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>> "Failed to create compatibility class link\n");
>> #endif
>>
>> - i2c_init_recovery(adap);
>> + res = i2c_init_recovery(adap);
>> + if (res == -EPROBE_DEFER)
>> + goto out_link;
>
> Please move 'i2c_init_recovery' above the class-link creation. It
> shouldn't make a difference but we can skip the extra label and the
> ifdeffery.
Ok. Perhaps I should also move the debug print with the registered
adapter after calling i2c_init_recovery().
>
>>
>> /* create pre-declared device nodes */
>> of_i2c_register_devices(adap);
>> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>>
>> return 0;
>>
>> +out_link:
>> +#ifdef CONFIG_I2C_COMPAT
>> + class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
>> + adap->dev.parent);
>> +#endif
>> out_reg:
>> init_completion(&adap->dev_released);
>> device_unregister(&adap->dev);
>> --
>> 2.25.1
>>
Do you want me to integrate this patch in the previous one?
Best regards,
Codrin
On 02.08.2020 20:08, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:04PM +0300, Codrin Ciubotariu wrote:
>> Make the Microchip at91 driver the first to use the generic GPIO bus
>> recovery support from the I2C core and discard the driver implementation.
>>
>> Signed-off-by: Codrin Ciubotariu <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-at91-master.c | 69 ++--------------------------
>> drivers/i2c/busses/i2c-at91.h | 3 --
>
> Nice diffstat! I will try using this new functionality with another
> controller next week.
>
Thanks, this would be great! I tested this on a sam9x60, with the HW
feature for the 9 pulses disabled, with a picky audio codec as I2C device.
Please let me know of the result.
Best regards,
Codrin
On 03.08.2020 17:16, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Thu, Jul 30, 2020 at 09:00:36AM +0000, [email protected] wrote:
>> On 27.07.2020 13:50, Russell King - ARM Linux admin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Jul 27, 2020 at 10:44:57AM +0000, [email protected] wrote:
>>>> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
>>>>>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>>>>>>>
>>>>>>>> +- pinctrl
>>>>>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
>>>>>>>> + recovery, call it "gpio" or "recovery" state
>>>>>>>
>>>>>>> I think we should stick with "gpio" only. That is what at91 and imx have
>>>>>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
>>>>>>> can't find any further use or documentation of that. PXA is not fully
>>>>>>> converted to the best of my knowledge, so maybe it is no problem for PXA
>>>>>>> to switch to "gpio", too? We should ask Russell King (cced).
>>>>>
>>>>> Fully converted to what? The generic handling where the i2c core layer
>>>>> handles everything to do with recovery, including the switch between
>>>>> modes?
>>>>>
>>>>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
>>>>> GPIO mode, and I don't see a generic driver doing that to avoid causing
>>>>> any additional glitches on the bus. Given the use case that this recovery
>>>>> is targetted at, avoiding glitches is very important to keep.
>>>>
>>>> Why is it not possbile to handle glitches in a generic way? I guess it
>>>> depends on the pinctl, but we could treat a worst-case scenario to
>>>> assure the switch between states is done properly.
>>>
>>> Please look at how i2c-pxa switches between the two, and decide whether
>>> the generic implementation can do the same.
>>
>> The handling of glitches from initialization looks generic to me. I see
>> that there are specific clear/reset routines that are in the
>> (un)prepare_recovery() callbacks, but these callbacks are not replaced
>> by the generic i2c recovery and will still be used if given by the
>> driver. The only thing the generic recovery does is to switch the pinmux
>> state. We can discuss whether we want to change the pinmux state first
>> or call the (un)preapre_recovery().
>
> Right, the key point i2c-pxa does is that on prepare:
> - read the current state of the SCL and SDA lines and set the GPIO to
> reflect those values.
> - then switch the pinmux state.
>
> That must be preserved, otherwise if SCL is being held low by the I2C
> master, and we switch to GPIO mode, SCL will be released. So the
> driver needs to be involved before the pinmux state is changed.
I understand, and I admit that I didn't see this case. In my mind, the
master would always be in (almost) a reset state before calling for SDA
recovery, so it won't hold any lines.
These steps can't be generic, of course. Also, not all I2C masters have
a way to show the state of its lines. For these masters, one idea would
be to reset them before calling i2c_recover_bus()
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
>
> I agree will all your suggestions, except with the removal of if
> (bri->recover_bus) . If you agree with this, I can send the next version
> and remove the RFC.
Yes, sure. That was a brown paper bag thingie from my side. Please go
ahead!
> > This is correct but I think the code flow is/was confusing. Can you drop
> > this 'return' and use 'else if' for the next code block? I think this is
> > more readable.
>
> Ok, it makes sense. Should I make a separate patch for this only?
I am fine if this is included in this change.
> One more question, should we keep:
> if (!bri->set_sda && !bri->get_sda) {
> err_str = "either get_sda() or set_sda() needed";
> goto err;
> }
> ?
> Without {get/set}_sda we won't be able to generate stop commands and
> possibly check if the bus is free, but we can still generate the SCL
> clock pulses.
My gut feeling says we need to keep it. I can't recall the reason now
and want to send out this answer ASAP. Anyhow, this definately would be
a seperate patch. If you really want to, send a patch, and then I have
to think why we still need it ;)
> Ok. Perhaps I should also move the debug print with the registered
> adapter after calling i2c_init_recovery().
Yes, makes sense.
> Do you want me to integrate this patch in the previous one?
Nope, please keep it seperate.
> > Nice diffstat! I will try using this new functionality with another
> > controller next week.
> >
>
> Thanks, this would be great! I tested this on a sam9x60, with the HW
> feature for the 9 pulses disabled, with a picky audio codec as I2C device.
> Please let me know of the result.
I'll surely let you know.
> Thanks, this would be great! I tested this on a sam9x60, with the HW
> feature for the 9 pulses disabled, with a picky audio codec as I2C device.
> Please let me know of the result.
I can't make use of the feature on the platform I had in mind, sadly. It
doesn't really support switching from/to GPIO pinctrl states. If that
ever changes, I will add bus recovery for that controller, but I think
this is low priority.
On the good side, there are patches which make i2c-mv64xxx another user
of your new mechanism, so everything is well, I think.
On 26.08.2020 09:14, Wolfram Sang wrote:
>
>> Thanks, this would be great! I tested this on a sam9x60, with the HW
>> feature for the 9 pulses disabled, with a picky audio codec as I2C device.
>> Please let me know of the result.
>
> I can't make use of the feature on the platform I had in mind, sadly. It
> doesn't really support switching from/to GPIO pinctrl states. If that
> ever changes, I will add bus recovery for that controller, but I think
> this is low priority.
The pinmux driver needs to have strict set to false, otherwise the
switching is not available, not at this time at least. Perhaps there is
room for improvement here, because the I2C bus is not using the pins
while we are doing GPIO recovery.
>
> On the good side, there are patches which make i2c-mv64xxx another user
> of your new mechanism, so everything is well, I think.
>
I saw them, I will try to take a look.
I am not sure I'll have time the next week to work on what you asked me
regarding sh_mobile and PXA, but I will look into it the week after that.
Sorry about my delayed reply, I was on vacation.
Best regards,
Codrin
Hi Codrin,
> The pinmux driver needs to have strict set to false, otherwise the
> switching is not available, not at this time at least. Perhaps there is
> room for improvement here, because the I2C bus is not using the pins
> while we are doing GPIO recovery.
Our driver doesn't use 'strict'. The thing is that I can't describe a
pinctrl state for GPIO. GPIO is the default state until another function
is requested. Back to GPIO currently means freeing the pin again, so it
defaults back to GPIO. We are currently discussing it. Geert (CCed)
isn't very happy of describing the same pins with 'function = "gpio"'
because the Kernel already knows the mapping, just needs to revert it.
Geert, please correct me if I am wrong.
> I am not sure I'll have time the next week to work on what you asked me
> regarding sh_mobile and PXA, but I will look into it the week after that.
> Sorry about my delayed reply, I was on vacation.
Well, no need for sh_mobile, this is my todo item :) About PXA, well, I
am still happy that you volunteered to do it, so I hope you had a
relaxing vacation!
Happy hacking,
Wolfram