This series contains some fixes and improvements for Davinci I2C:
patch 1 - small improvement
patch 2 - fixes "Bus busy" state for some case (was reported long time ago:()
patch 3-5 - converts driver to use I2C bus recovery infrastructure and
adds Davinci I2C bus recovery mechanizm based on using ICPFUNC registers.
These patches are combination of two patches from Ben Gardiner [1] and
Mike Looijmans [2] which i've reworked to use I2C bus recovery infrastructure
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/047305.html
"[PATCH 0/6] i2c-davinci gpio pulsed SCL recovery with ICPFUNC"
[2] https://lkml.org/lkml/2014/2/28/133
"[PATCH] i2c-davinci: Implement a bus recovery that actually works"
CC: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: Santosh Shilimkar <[email protected]>
CC: Murali Karicheri <[email protected]>
Grygorii Strashko (5):
i2c: i2c-davinci: switch to use platform_get_irq
i2c: davinci: query STP always when NACK is received
i2c: recovery: change input parameter to i2c_adapter for
prepare/unprepare_recovery
i2c: davinci: use bus recovery infrastructure
i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery
drivers/i2c/busses/i2c-davinci.c | 196 ++++++++++++++++++++++--------
drivers/i2c/i2c-core.c | 4 +-
include/linux/i2c.h | 4 +-
include/linux/platform_data/i2c-davinci.h | 1 +
4 files changed, 148 insertions(+), 57 deletions(-)
--
1.9.1
According to I2C specification the NACK should be handled as folowing:
"When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then gene rate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer."
[http://www.nxp.com/documents/user_manual/UM10204.pdf]
The same is recomened by TI I2C wiki:
http://processors.wiki.ti.com/index.php/I2C_Tips
Currently, the Davinci I2C driver interrupts I2C trunsfer in case of NACK, but
It queries Stop condition DAVINCI_I2C_MDR_REG.STP=1 only if NACK has been received
during the last message transmitting/recieving.
This may lead to Bus stuck in "Bus Busy" until I2C IP reset (idle/enable) if
during SMBus reading transaction the first I2C message is NACKed.
Hence, fix it by querying Stop condition (STP) always when NACK is received.
This patch fixes Davinci I2C in the same way it was done for OMAP I2C
commit cda2109a26eb ("i2c: omap: query STP always when NACK is received").
More info can be found at:
https://lkml.org/lkml/2013/7/16/159
http://patchwork.ozlabs.org/patch/249790/
CC: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: Santosh Shilimkar <[email protected]>
CC: Murali Karicheri <[email protected]>
Reported-by: Hein Tibosch <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/i2c/busses/i2c-davinci.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 9bbfb8f..2cef115 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
if (msg->flags & I2C_M_IGNORE_NAK)
return msg->len;
- if (stop) {
- w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
- w |= DAVINCI_I2C_MDR_STP;
- davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
- }
+ w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+ w |= DAVINCI_I2C_MDR_STP;
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
return -EREMOTEIO;
}
return -EIO;
--
1.9.1
Having a board where the I2C bus locks up occasionally made it clear
that the bus recovery in the i2c-davinci driver will only work on
some boards, because on regular boards, this will only toggle GPIO
lines that aren't muxed to the actual pins.
The I2C controller on SoCs like da850 (and da830), Keystone 2 has the
built-in capability to bit-bang its lines by using the ICPFUNC registers
of the i2c controller.
Implement the suggested procedure by toggling SCL and checking SDA using
the ICPFUNC registers of the I2C controller when present. Allow platforms
to indicate the presence of the ICPFUNC registers with a has_pfunc platform
data flag and enable this mode for platforms which supports DT
(da850 and Keystone 2 are two SoCs which supports DT now and also
support ICPFUNC registers).
CC: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: Santosh Shilimkar <[email protected]>
CC: Murali Karicheri <[email protected]>
Signed-off-by: Ben Gardiner <[email protected]>
Signed-off-by: Mike Looijmans <[email protected]>
[[email protected]: combined patches from Ben Gardiner and
Mike Looijmans and reimplemented ICPFUNC bus recovery using I2C
bus recovery infrastructure]
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/i2c/busses/i2c-davinci.c | 100 +++++++++++++++++++++++++++++-
include/linux/platform_data/i2c-davinci.h | 1 +
2 files changed, 100 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index db2a2cd..6cf96fb 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -64,6 +64,12 @@
#define DAVINCI_I2C_IVR_REG 0x28
#define DAVINCI_I2C_EMDR_REG 0x2c
#define DAVINCI_I2C_PSC_REG 0x30
+#define DAVINCI_I2C_FUNC_REG 0x48
+#define DAVINCI_I2C_DIR_REG 0x4c
+#define DAVINCI_I2C_DIN_REG 0x50
+#define DAVINCI_I2C_DOUT_REG 0x54
+#define DAVINCI_I2C_DSET_REG 0x58
+#define DAVINCI_I2C_DCLR_REG 0x5c
#define DAVINCI_I2C_IVR_AAS 0x07
#define DAVINCI_I2C_IVR_SCD 0x06
@@ -97,6 +103,29 @@
#define DAVINCI_I2C_IMR_NACK BIT(1)
#define DAVINCI_I2C_IMR_AL BIT(0)
+/* set SDA and SCL as GPIO */
+#define DAVINCI_I2C_FUNC_PFUNC0 BIT(0)
+
+/* set SCL as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR0 BIT(0)
+/* set SDA as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR1 BIT(1)
+
+/* read SCL GPIO level */
+#define DAVINCI_I2C_DIN_PDIN0 BIT(0)
+/* read SDA GPIO level */
+#define DAVINCI_I2C_DIN_PDIN1 BIT(1)
+
+/*set the SCL GPIO high */
+#define DAVINCI_I2C_DSET_PDSET0 BIT(0)
+/*set the SDA GPIO high */
+#define DAVINCI_I2C_DSET_PDSET1 BIT(1)
+
+/* set the SCL GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR0 BIT(0)
+/* set the SDA GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR1 BIT(1)
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem *base;
@@ -256,6 +285,72 @@ static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
.prepare_recovery = davinci_i2c_prepare_recovery,
.unprepare_recovery = davinci_i2c_unprepare_recovery,
};
+
+static void davinci_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+ struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+ if (val)
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_DSET_REG,
+ DAVINCI_I2C_DSET_PDSET0);
+ else
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_DCLR_REG,
+ DAVINCI_I2C_DCLR_PDCLR0);
+}
+
+static int davinci_i2c_get_scl(struct i2c_adapter *adap)
+{
+ struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+ int val;
+
+ /* read the state of SCL */
+ val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+ return val & DAVINCI_I2C_DIN_PDIN0;
+}
+
+static int davinci_i2c_get_sda(struct i2c_adapter *adap)
+{
+ struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+ int val;
+
+ /* read the state of SDA */
+ val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+ return val & DAVINCI_I2C_DIN_PDIN1;
+}
+
+static void davinci_i2c_scl_prepare_recovery(struct i2c_adapter *adap)
+{
+ struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+ davinci_i2c_prepare_recovery(adap);
+
+ /* SCL output, SDA input */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_DIR_REG, DAVINCI_I2C_DIR_PDIR0);
+
+ /* change to GPIO mode */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_FUNC_REG,
+ DAVINCI_I2C_FUNC_PFUNC0);
+}
+
+static void davinci_i2c_scl_unprepare_recovery(struct i2c_adapter *adap)
+{
+ struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+ /* change back to I2C mode */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_FUNC_REG, 0);
+
+ davinci_i2c_unprepare_recovery(adap);
+}
+
+static struct i2c_bus_recovery_info davinci_i2c_scl_recovery_info = {
+ .recover_bus = i2c_generic_scl_recovery,
+ .set_scl = davinci_i2c_set_scl,
+ .get_scl = davinci_i2c_get_scl,
+ .get_sda = davinci_i2c_get_sda,
+ .prepare_recovery = davinci_i2c_scl_prepare_recovery,
+ .unprepare_recovery = davinci_i2c_scl_unprepare_recovery,
+};
+
/*
* Waiting for bus not busy
*/
@@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
&prop))
dev->pdata->bus_freq = prop / 1000;
+ dev->pdata->has_pfunc = true;
} else if (!dev->pdata) {
dev->pdata = &davinci_i2c_platform_data_default;
}
@@ -705,7 +801,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
adap->timeout = DAVINCI_I2C_TIMEOUT;
adap->dev.of_node = pdev->dev.of_node;
- if (dev->pdata->scl_pin) {
+ if (dev->pdata->has_pfunc)
+ adap->bus_recovery_info = &davinci_i2c_scl_recovery_info;
+ else if (dev->pdata->scl_pin) {
adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
diff --git a/include/linux/platform_data/i2c-davinci.h b/include/linux/platform_data/i2c-davinci.h
index 2312d19..89fd347 100644
--- a/include/linux/platform_data/i2c-davinci.h
+++ b/include/linux/platform_data/i2c-davinci.h
@@ -18,6 +18,7 @@ struct davinci_i2c_platform_data {
unsigned int bus_delay; /* post-transaction delay (usec) */
unsigned int sda_pin; /* GPIO pin ID to use for SDA */
unsigned int scl_pin; /* GPIO pin ID to use for SCL */
+ bool has_pfunc; /*chip has a ICPFUNC register */
};
/* for board setup code */
--
1.9.1
This patch converts Davinci I2C driver to use I2C bus recovery
infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add
bus recovery infrastructure").
The i2c_bus_recovery_info is configured for Davinci I2C adapter
only in case if scl_pin is provided in Platform data at least.
Because the controller must be held in reset while doing so, the
recovery routine must re-init the controller. Since this was already
being done after each call to i2c_recover_bus, move those calls into
the recovery_prepare/unprepare routines and as well.
CC: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: Santosh Shilimkar <[email protected]>
CC: Murali Karicheri <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/i2c/busses/i2c-davinci.c | 76 ++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 41 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 2cef115..db2a2cd 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
return readw_relaxed(i2c_dev->base + reg);
}
-/* Generate a pulse on the i2c clock pin. */
-static void davinci_i2c_clock_pulse(unsigned int scl_pin)
-{
- u16 i;
-
- if (scl_pin) {
- /* Send high and low on the SCL line */
- for (i = 0; i < 9; i++) {
- gpio_set_value(scl_pin, 0);
- udelay(20);
- gpio_set_value(scl_pin, 1);
- udelay(20);
- }
- }
-}
-
-/* This routine does i2c bus recovery as specified in the
- * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
- */
-static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
-{
- u32 flag = 0;
- struct davinci_i2c_platform_data *pdata = dev->pdata;
-
- dev_err(dev->dev, "initiating i2c bus recovery\n");
- /* Send NACK to the slave */
- flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
- flag |= DAVINCI_I2C_MDR_NACK;
- /* write the data into mode register */
- davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
- davinci_i2c_clock_pulse(pdata->scl_pin);
- /* Send STOP */
- flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
- flag |= DAVINCI_I2C_MDR_STP;
- davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-}
-
static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
int val)
{
@@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
return 0;
}
+/* This routine does i2c bus recovery as specified in the
+ * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
+ */
+static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+ struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+ dev_err(dev->dev, "initiating i2c bus recovery\n");
+ /* Disable interrupts */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
+
+ /* put I2C into reset */
+ davinci_i2c_reset_ctrl(dev, 0);
+}
+
+static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+ struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+ i2c_davinci_init(dev);
+}
+
+static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
+ .recover_bus = i2c_generic_gpio_recovery,
+ .prepare_recovery = davinci_i2c_prepare_recovery,
+ .unprepare_recovery = davinci_i2c_unprepare_recovery,
+};
/*
* Waiting for bus not busy
*/
@@ -286,8 +276,7 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
return -ETIMEDOUT;
} else {
to_cnt = 0;
- davinci_i2c_recover_bus(dev);
- i2c_davinci_init(dev);
+ i2c_recover_bus(&dev->adapter);
}
}
if (allow_sleep)
@@ -376,8 +365,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
dev->adapter.timeout);
if (r == 0) {
dev_err(dev->dev, "controller timed out\n");
- davinci_i2c_recover_bus(dev);
- i2c_davinci_init(dev);
+ i2c_recover_bus(adap);
dev->buf_len = 0;
return -ETIMEDOUT;
}
@@ -717,6 +705,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
adap->timeout = DAVINCI_I2C_TIMEOUT;
adap->dev.of_node = pdev->dev.of_node;
+ if (dev->pdata->scl_pin) {
+ adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
+ adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
+ adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
+ }
+
adap->nr = pdev->id;
r = i2c_add_numbered_adapter(adap);
if (r) {
--
1.9.1
This patch changes type of input parameter for .prepare/unprepare_recovery()
callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
This allows to simplify implementation of these callbacks and avoid
type conversations from i2c_bus_recovery_info to i2c_adapter.
The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
which contains pointer on it.
CC: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: Santosh Shilimkar <[email protected]>
CC: Murali Karicheri <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/i2c/i2c-core.c | 4 ++--
include/linux/i2c.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..72b6e34 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -563,7 +563,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
int i = 0, val = 1, ret = 0;
if (bri->prepare_recovery)
- bri->prepare_recovery(bri);
+ bri->prepare_recovery(adap);
/*
* By this time SCL is high, as we need to give 9 falling-rising edges
@@ -588,7 +588,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
}
if (bri->unprepare_recovery)
- bri->unprepare_recovery(bri);
+ bri->unprepare_recovery(adap);
return ret;
}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..cf9380f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -404,8 +404,8 @@ struct i2c_bus_recovery_info {
void (*set_scl)(struct i2c_adapter *, int val);
int (*get_sda)(struct i2c_adapter *);
- void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
- void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
+ void (*prepare_recovery)(struct i2c_adapter *);
+ void (*unprepare_recovery)(struct i2c_adapter *);
/* gpio recovery */
int scl_gpio;
--
1.9.1
Switch Davinci I2C driver to use platform_get_irq(), because
- it is not recommened to use
platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
resources any more, as they can be not ready yet in case of DT-booting.
- it makes code simpler
CC: Sekhar Nori <[email protected]>
CC: Kevin Hilman <[email protected]>
CC: Santosh Shilimkar <[email protected]>
CC: Murali Karicheri <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/i2c/busses/i2c-davinci.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..9bbfb8f 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
- struct resource *mem, *irq;
- int r;
+ struct resource *mem;
+ int r, irq;
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
+ return irq;
}
dev = devm_kzalloc(&pdev->dev, sizeof(struct davinci_i2c_dev),
@@ -661,7 +661,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
init_completion(&dev->xfr_complete);
#endif
dev->dev = &pdev->dev;
- dev->irq = irq->start;
+ dev->irq = irq;
dev->pdata = dev_get_platdata(&pdev->dev);
platform_set_drvdata(pdev, dev);
--
1.9.1
Hello Grygorii,
On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
> Switch Davinci I2C driver to use platform_get_irq(), because
> - it is not recommened to use
> platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
> resources any more, as they can be not ready yet in case of DT-booting.
> - it makes code simpler
>
> CC: Sekhar Nori <[email protected]>
> CC: Kevin Hilman <[email protected]>
> CC: Santosh Shilimkar <[email protected]>
> CC: Murali Karicheri <[email protected]>
> Signed-off-by: Grygorii Strashko <[email protected]>
> ---
> drivers/i2c/busses/i2c-davinci.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 4d96147..9bbfb8f 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> {
> struct davinci_i2c_dev *dev;
> struct i2c_adapter *adap;
> - struct resource *mem, *irq;
> - int r;
> + struct resource *mem;
> + int r, irq;
>
> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> - if (!irq) {
> - dev_err(&pdev->dev, "no irq resource?\n");
> - return -ENODEV;
> + irq = platform_get_irq(pdev, 0);
One bad thing about platform_get_irq is its unusual handling of irq=0.
I'm pretty sure you don't want to use this value, so adding something
like:
if (!irq)
irq = -ENXIO
would be welcome because the usual value for "invalid irq" is 0 and not
-ESOMETHING. platform_get_irq is one of the very few functions that
don't adhere to this convention. With handling <= 0 as error your code
is immune to changes in this area. Although I notice that
platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
Apart from your change I wonder if platform_get_irq should handle
of_irq_get returning 0 as an error.
> + if (irq < 0) {
> + dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
Please don't print an error if irq=-EPROBE_DEFER.
> + return irq;
> }
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello,
On Thu, Nov 20, 2014 at 12:03:05PM +0200, Grygorii Strashko wrote:
> According to I2C specification the NACK should be handled as folowing:
s/folowing/follows/
> "When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
> Acknowledge signal. The master can then gene rate either a STOP condition to
s/gene rate/generate/
> abort the transfer, or a repeated START condition to start a new transfer."
> [http://www.nxp.com/documents/user_manual/UM10204.pdf]
The link is nice, but pointing out that this is the i2c spec would be
nice.
> The same is recomened by TI I2C wiki:
s/recomened/recommended/
> http://processors.wiki.ti.com/index.php/I2C_Tips
If the specification tells what to do, there is no need to further
support your claim.
> Currently, the Davinci I2C driver interrupts I2C trunsfer in case of NACK, but
s/trunsfer/transfer/
> It queries Stop condition DAVINCI_I2C_MDR_REG.STP=1 only if NACK has been received
s/It/it/
> during the last message transmitting/recieving.
s/transmitting/transmitted/; s/recieving/received/
I think I don't understand this sentence even with the typos corrected.
Do you want to say:
Currently the Davinci i2c driver interrupts the transfer on receipt of a
NACK but fails to send a STOP in some situations and so makes the bus
stuck.
> This may lead to Bus stuck in "Bus Busy" until I2C IP reset (idle/enable) if
> during SMBus reading transaction the first I2C message is NACKed.
Did you hit this problem, or is this a theoretical issue?
Assuming this is a candidate for stable, adding a Fixes:-footer would be
nice.
> Hence, fix it by querying Stop condition (STP) always when NACK is received.
>
> This patch fixes Davinci I2C in the same way it was done for OMAP I2C
> commit cda2109a26eb ("i2c: omap: query STP always when NACK is received").
>
> More info can be found at:
> https://lkml.org/lkml/2013/7/16/159
> http://patchwork.ozlabs.org/patch/249790/
I'd drop this "more info" paragraph.
> CC: Sekhar Nori <[email protected]>
> CC: Kevin Hilman <[email protected]>
> CC: Santosh Shilimkar <[email protected]>
> CC: Murali Karicheri <[email protected]>
> Reported-by: Hein Tibosch <[email protected]>
Is this Reported-by tag reused from the omap issue?
> Signed-off-by: Grygorii Strashko <[email protected]>
> ---
> drivers/i2c/busses/i2c-davinci.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 9bbfb8f..2cef115 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
> if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
> if (msg->flags & I2C_M_IGNORE_NAK)
> return msg->len;
> - if (stop) {
> - w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> - w |= DAVINCI_I2C_MDR_STP;
> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> - }
> + w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> + w |= DAVINCI_I2C_MDR_STP;
> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
I think this is a good change, but I wonder if the handling of
I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
for the 2nd byte of a 5-byte-message, the transfer supposed to
continue, right? (Hmm, maybe the framework handle this and restarts the
transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
handle this flag?)
Best regards
Uwe
> return -EREMOTEIO;
> }
> return -EIO;
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 11/20/2014 11:48 PM, Uwe Kleine-K?nig wrote:
> Hello Grygorii,
>
> On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
>> Switch Davinci I2C driver to use platform_get_irq(), because
>> - it is not recommened to use
>> platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
>> resources any more, as they can be not ready yet in case of DT-booting.
>> - it makes code simpler
>>
>> CC: Sekhar Nori <[email protected]>
>> CC: Kevin Hilman <[email protected]>
>> CC: Santosh Shilimkar <[email protected]>
>> CC: Murali Karicheri <[email protected]>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-davinci.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 4d96147..9bbfb8f 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>> {
>> struct davinci_i2c_dev *dev;
>> struct i2c_adapter *adap;
>> - struct resource *mem, *irq;
>> - int r;
>> + struct resource *mem;
>> + int r, irq;
>>
>> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> - if (!irq) {
>> - dev_err(&pdev->dev, "no irq resource?\n");
>> - return -ENODEV;
>> + irq = platform_get_irq(pdev, 0);
> One bad thing about platform_get_irq is its unusual handling of irq=0.
> I'm pretty sure you don't want to use this value, so adding something
> like:
>
> if (!irq)
> irq = -ENXIO
>
> would be welcome because the usual value for "invalid irq" is 0 and not
> -ESOMETHING. platform_get_irq is one of the very few functions that
> don't adhere to this convention. With handling <= 0 as error your code
> is immune to changes in this area. Although I notice that
> platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
>
> Apart from your change I wonder if platform_get_irq should handle
> of_irq_get returning 0 as an error.
I think you are right and It seems like, the check for !irq should
be added/restored for OF case in platform_get_irq() too.
Also, I've simulated irq == 0 case - the .probe() failed with error -EINVAL
which is returned by request_threaded_irq() because of !irq_settings_can_request(desc).
i2c_davinci 2530000.i2c: failure requesting irq 0
i2c_davinci: probe of 2530000.i2c failed with error -22
I'm not sure that above will work for everyone because it depends on ARCH_IRQ_INIT_FLAGS
and ARCH_IRQ_INIT_FLAGS = (IRQ_NOREQUEST | IRQ_NOPROBE) for ARM.
>
>> + if (irq < 0) {
>> + dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
> Please don't print an error if irq=-EPROBE_DEFER.
ok.
regards,
-grygorii
Hi Uwe,
On 11/21/2014 12:19 AM, Uwe Kleine-K?nig wrote:
> On Thu, Nov 20, 2014 at 12:03:05PM +0200, Grygorii Strashko wrote:
>> According to I2C specification the NACK should be handled as folowing:
> s/folowing/follows/
>
>> "When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
>> Acknowledge signal. The master can then gene rate either a STOP condition to
> s/gene rate/generate/
>
>> abort the transfer, or a repeated START condition to start a new transfer."
>> [http://www.nxp.com/documents/user_manual/UM10204.pdf]
> The link is nice, but pointing out that this is the i2c spec would be
> nice.
>
>> The same is recomened by TI I2C wiki:
> s/recomened/recommended/
>
>> http://processors.wiki.ti.com/index.php/I2C_Tips
> If the specification tells what to do, there is no need to further
> support your claim.
>
>> Currently, the Davinci I2C driver interrupts I2C trunsfer in case of NACK, but
> s/trunsfer/transfer/
>
>> It queries Stop condition DAVINCI_I2C_MDR_REG.STP=1 only if NACK has been received
> s/It/it/
>
>> during the last message transmitting/recieving.
> s/transmitting/transmitted/; s/recieving/received/
>
> I think I don't understand this sentence even with the typos corrected.
> Do you want to say:
>
> Currently the Davinci i2c driver interrupts the transfer on receipt of a
> NACK but fails to send a STOP in some situations and so makes the bus
> stuck.
>
>> This may lead to Bus stuck in "Bus Busy" until I2C IP reset (idle/enable) if
>> during SMBus reading transaction the first I2C message is NACKed.
> Did you hit this problem, or is this a theoretical issue?
I've hit this issue on OMAP board and the Davinci I2C driver is implemented in a similar way.
Now I've no HW which would allow me to reproduce it on Davinci.
quotes from https://lkml.org/lkml/2013/7/16/235:
>>>
The problem is, that lm75 device is SmBus compatible and its driver has
.detect() function implemented. During detection it tries to scan some
registers which might be not present in current device - in my case
it's tmp105.
For example to read regA in tmp105 following is done:
1) do write in "Index" register (val RegA index) (I2C 1st message)
2) do read (I2C 2d message)
the message 1 is Nacked by lm75 type of device in case if register index is wrong,
but i2c-omap don't send NACK (or STP). As result, bus stack in Bus Busy
state.
For SMBus devices the specification states (http://smbus.org/specs/)
"4.2.Acknowledge (ACK) and not acknowledge (NACK)":
- "The slave device detects an invalid command or invalid data. In this
case the slave device must not acknowledge the received byte. The
master upon detection of this condition must generate a STOP condition
and retry the transaction"
<<<
>
> Assuming this is a candidate for stable, adding a Fixes:-footer would be
> nice.
>
>> Hence, fix it by querying Stop condition (STP) always when NACK is received.
>>
>> This patch fixes Davinci I2C in the same way it was done for OMAP I2C
>> commit cda2109a26eb ("i2c: omap: query STP always when NACK is received").
>>
>> More info can be found at:
>> https://lkml.org/lkml/2013/7/16/159
>> http://patchwork.ozlabs.org/patch/249790/
> I'd drop this "more info" paragraph.
>
>> CC: Sekhar Nori <[email protected]>
>> CC: Kevin Hilman <[email protected]>
>> CC: Santosh Shilimkar <[email protected]>
>> CC: Murali Karicheri <[email protected]>
>> Reported-by: Hein Tibosch <[email protected]>
> Is this Reported-by tag reused from the omap issue?
>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-davinci.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 9bbfb8f..2cef115 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>> if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
>> if (msg->flags & I2C_M_IGNORE_NAK)
>> return msg->len;
>> - if (stop) {
>> - w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> - w |= DAVINCI_I2C_MDR_STP;
>> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>> - }
>> + w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> + w |= DAVINCI_I2C_MDR_STP;
>> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> I think this is a good change, but I wonder if the handling of
> I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
> for the 2nd byte of a 5-byte-message, the transfer supposed to
> continue, right? (Hmm, maybe the framework handle this and restarts the
> transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
> handle this flag?)
Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
change current behavior - davinci driver will interrupt transfer of i2c_msg always
in case of NACK and start transfer of the next i2c_msg (if exist).
In my opinion, Above question is out of scope of this patch.
Thanks for your comments.
Regards,
-grygorii
Hello,
On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
> On 11/21/2014 12:19 AM, Uwe Kleine-K?nig wrote:
> >> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> >> index 9bbfb8f..2cef115 100644
> >> --- a/drivers/i2c/busses/i2c-davinci.c
> >> +++ b/drivers/i2c/busses/i2c-davinci.c
> >> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
> >> if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
> >> if (msg->flags & I2C_M_IGNORE_NAK)
> >> return msg->len;
> >> - if (stop) {
> >> - w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> >> - w |= DAVINCI_I2C_MDR_STP;
> >> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> >> - }
> >> + w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> >> + w |= DAVINCI_I2C_MDR_STP;
> >> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> > I think this is a good change, but I wonder if the handling of
> > I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
> > for the 2nd byte of a 5-byte-message, the transfer supposed to
> > continue, right? (Hmm, maybe the framework handle this and restarts the
> > transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
> > handle this flag?)
>
> Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
> change current behavior - davinci driver will interrupt transfer of i2c_msg always
> in case of NACK and start transfer of the next i2c_msg (if exist).
> In my opinion, Above question is out of scope of this patch.
Yeah right, that's exactly what I thought.
Thinking again I wonder if with your change handling is correct when the
sender wants to do a repeated start. That would need a more detailed
look into the driver.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Fri, Nov 21, 2014 at 5:01 AM, Grygorii Strashko
<[email protected]> wrote:
> On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
>> Hello Grygorii,
>>
>> On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
>>> Switch Davinci I2C driver to use platform_get_irq(), because
>>> - it is not recommened to use
>>> platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
>>> resources any more, as they can be not ready yet in case of DT-booting.
>>> - it makes code simpler
>>>
>>> CC: Sekhar Nori <[email protected]>
>>> CC: Kevin Hilman <[email protected]>
>>> CC: Santosh Shilimkar <[email protected]>
>>> CC: Murali Karicheri <[email protected]>
>>> Signed-off-by: Grygorii Strashko <[email protected]>
>>> ---
>>> drivers/i2c/busses/i2c-davinci.c | 14 +++++++-------
>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>>> index 4d96147..9bbfb8f 100644
>>> --- a/drivers/i2c/busses/i2c-davinci.c
>>> +++ b/drivers/i2c/busses/i2c-davinci.c
>>> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>> {
>>> struct davinci_i2c_dev *dev;
>>> struct i2c_adapter *adap;
>>> - struct resource *mem, *irq;
>>> - int r;
>>> + struct resource *mem;
>>> + int r, irq;
>>>
>>> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>> - if (!irq) {
>>> - dev_err(&pdev->dev, "no irq resource?\n");
>>> - return -ENODEV;
>>> + irq = platform_get_irq(pdev, 0);
>> One bad thing about platform_get_irq is its unusual handling of irq=0.
>> I'm pretty sure you don't want to use this value, so adding something
>> like:
>>
>> if (!irq)
>> irq = -ENXIO
>>
>> would be welcome because the usual value for "invalid irq" is 0 and not
>> -ESOMETHING. platform_get_irq is one of the very few functions that
>> don't adhere to this convention. With handling <= 0 as error your code
>> is immune to changes in this area. Although I notice that
>> platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
>>
>> Apart from your change I wonder if platform_get_irq should handle
>> of_irq_get returning 0 as an error.
>
> I think you are right and It seems like, the check for !irq should
> be added/restored for OF case in platform_get_irq() too.
Changing the return values of platform_get_irq is tricky as it would
potentially break drivers because NO_IRQ can be 0 or -1 depending on
the arch. Drivers checking against specific values of NO_IRQ would
break. We've done some clean-up in this area, but I suspect more is
needed.
Rob
On 11/21/2014 04:03 PM, Rob Herring wrote:
> On Fri, Nov 21, 2014 at 5:01 AM, Grygorii Strashko
> <[email protected]> wrote:
>> On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
>>> Hello Grygorii,
>>>
>>> On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
>>>> Switch Davinci I2C driver to use platform_get_irq(), because
>>>> - it is not recommened to use
>>>> platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
>>>> resources any more, as they can be not ready yet in case of DT-booting.
>>>> - it makes code simpler
>>>>
>>>> CC: Sekhar Nori <[email protected]>
>>>> CC: Kevin Hilman <[email protected]>
>>>> CC: Santosh Shilimkar <[email protected]>
>>>> CC: Murali Karicheri <[email protected]>
>>>> Signed-off-by: Grygorii Strashko <[email protected]>
>>>> ---
>>>> drivers/i2c/busses/i2c-davinci.c | 14 +++++++-------
>>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>>>> index 4d96147..9bbfb8f 100644
>>>> --- a/drivers/i2c/busses/i2c-davinci.c
>>>> +++ b/drivers/i2c/busses/i2c-davinci.c
>>>> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>>> {
>>>> struct davinci_i2c_dev *dev;
>>>> struct i2c_adapter *adap;
>>>> - struct resource *mem, *irq;
>>>> - int r;
>>>> + struct resource *mem;
>>>> + int r, irq;
>>>>
>>>> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>> - if (!irq) {
>>>> - dev_err(&pdev->dev, "no irq resource?\n");
>>>> - return -ENODEV;
>>>> + irq = platform_get_irq(pdev, 0);
>>> One bad thing about platform_get_irq is its unusual handling of irq=0.
>>> I'm pretty sure you don't want to use this value, so adding something
>>> like:
>>>
>>> if (!irq)
>>> irq = -ENXIO
I'll add this check in driver.
>>>
>>> would be welcome because the usual value for "invalid irq" is 0 and not
>>> -ESOMETHING. platform_get_irq is one of the very few functions that
>>> don't adhere to this convention. With handling <= 0 as error your code
>>> is immune to changes in this area. Although I notice that
>>> platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
>>>
>>> Apart from your change I wonder if platform_get_irq should handle
>>> of_irq_get returning 0 as an error.
>>
>> I think you are right and It seems like, the check for !irq should
>> be added/restored for OF case in platform_get_irq() too.
>
> Changing the return values of platform_get_irq is tricky as it would
> potentially break drivers because NO_IRQ can be 0 or -1 depending on
> the arch. Drivers checking against specific values of NO_IRQ would
> break. We've done some clean-up in this area, but I suspect more is
> needed.
Thanks for your comment.
regards,
-grygorii
Hi Uwe,
On 11/21/2014 03:10 PM, Uwe Kleine-K?nig wrote:
> On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
>> On 11/21/2014 12:19 AM, Uwe Kleine-K?nig wrote:
>>>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>>>> index 9bbfb8f..2cef115 100644
>>>> --- a/drivers/i2c/busses/i2c-davinci.c
>>>> +++ b/drivers/i2c/busses/i2c-davinci.c
>>>> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>>> if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
>>>> if (msg->flags & I2C_M_IGNORE_NAK)
>>>> return msg->len;
>>>> - if (stop) {
>>>> - w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>>>> - w |= DAVINCI_I2C_MDR_STP;
>>>> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>>> - }
>>>> + w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>>>> + w |= DAVINCI_I2C_MDR_STP;
>>>> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>> I think this is a good change, but I wonder if the handling of
>>> I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
>>> for the 2nd byte of a 5-byte-message, the transfer supposed to
>>> continue, right? (Hmm, maybe the framework handle this and restarts the
>>> transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
>>> handle this flag?)
>>
>> Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
>> change current behavior - davinci driver will interrupt transfer of i2c_msg always
>> in case of NACK and start transfer of the next i2c_msg (if exist).
>> In my opinion, Above question is out of scope of this patch.
> Yeah right, that's exactly what I thought.
>
> Thinking again I wonder if with your change handling is correct when the
> sender wants to do a repeated start. That would need a more detailed
> look into the driver.
Davinci driver will always abort transfer with error -EREMOTEIO in case if
NACK received from I2C slave device. And the next omap_i2c_xfer() call may
be *not* targeted to the same I2C slave device.
^ if !I2C_M_IGNORE_NAK
This discussion is absolutely similar to https://lkml.org/lkml/2013/7/16/235
So, I'm just copy-pasting my answers from there ;)
regards,
-grygorii
On Thu, Nov 20, 2014 at 12:03:06PM +0200, Grygorii Strashko wrote:
> This patch changes type of input parameter for .prepare/unprepare_recovery()
> callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
> This allows to simplify implementation of these callbacks and avoid
> type conversations from i2c_bus_recovery_info to i2c_adapter.
> The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
> which contains pointer on it.
>
> CC: Sekhar Nori <[email protected]>
> CC: Kevin Hilman <[email protected]>
> CC: Santosh Shilimkar <[email protected]>
> CC: Murali Karicheri <[email protected]>
> Signed-off-by: Grygorii Strashko <[email protected]>
Sounds and looks sensible
Acked-by: Uwe Kleine-K?nig <[email protected]>
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
> This patch converts Davinci I2C driver to use I2C bus recovery
> infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add
> bus recovery infrastructure").
>
> The i2c_bus_recovery_info is configured for Davinci I2C adapter
> only in case if scl_pin is provided in Platform data at least.
s/Platform/platform/
>
> Because the controller must be held in reset while doing so, the
s/Because/As/
> recovery routine must re-init the controller. Since this was already
> being done after each call to i2c_recover_bus, move those calls into
> the recovery_prepare/unprepare routines and as well.
>
> CC: Sekhar Nori <[email protected]>
> CC: Kevin Hilman <[email protected]>
> CC: Santosh Shilimkar <[email protected]>
> CC: Murali Karicheri <[email protected]>
> Signed-off-by: Grygorii Strashko <[email protected]>
> ---
> drivers/i2c/busses/i2c-davinci.c | 76 ++++++++++++++++++----------------------
> 1 file changed, 35 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 2cef115..db2a2cd 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
> return readw_relaxed(i2c_dev->base + reg);
> }
>
> -/* Generate a pulse on the i2c clock pin. */
> -static void davinci_i2c_clock_pulse(unsigned int scl_pin)
> -{
> - u16 i;
> -
> - if (scl_pin) {
> - /* Send high and low on the SCL line */
> - for (i = 0; i < 9; i++) {
> - gpio_set_value(scl_pin, 0);
> - udelay(20);
> - gpio_set_value(scl_pin, 1);
> - udelay(20);
> - }
> - }
> -}
> -
> -/* This routine does i2c bus recovery as specified in the
> - * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
> - */
> -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
> -{
> - u32 flag = 0;
> - struct davinci_i2c_platform_data *pdata = dev->pdata;
> -
> - dev_err(dev->dev, "initiating i2c bus recovery\n");
> - /* Send NACK to the slave */
> - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> - flag |= DAVINCI_I2C_MDR_NACK;
> - /* write the data into mode register */
> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> - davinci_i2c_clock_pulse(pdata->scl_pin);
> - /* Send STOP */
> - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> - flag |= DAVINCI_I2C_MDR_STP;
> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> -}
> -
> static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
> int val)
> {
> @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> return 0;
> }
>
> +/* This routine does i2c bus recovery as specified in the
> + * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
> + */
This comment is wrong. The actual bus clear is implemented by
i2c_generic_gpio_recovery. Also while touching this comment, convert it
to the usual format with /* on its own line. (The file in question has
already both types of comment, so consistency is not a reason to keep it
as is.)
Even though I remember that I reviewed this bus recovery patch (that
resulted in 5f9296ba21b3) back then, I don't remember why it was split
in prepare + recover + unprepare. But that is unrelated to this patch.
> +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
> +{
> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> + dev_err(dev->dev, "initiating i2c bus recovery\n");
> + /* Disable interrupts */
> + davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
> +
> + /* put I2C into reset */
> + davinci_i2c_reset_ctrl(dev, 0);
> +}
> +
> +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
> +{
> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> + i2c_davinci_init(dev);
> +}
> +
> +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
I'd call this only davinci_i2c_recovery_info.
> + .recover_bus = i2c_generic_gpio_recovery,
> + .prepare_recovery = davinci_i2c_prepare_recovery,
> + .unprepare_recovery = davinci_i2c_unprepare_recovery,
> +};
new line here please
> /*
> * Waiting for bus not busy
> */
> @@ -286,8 +276,7 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
> return -ETIMEDOUT;
> } else {
> to_cnt = 0;
> - davinci_i2c_recover_bus(dev);
> - i2c_davinci_init(dev);
> + i2c_recover_bus(&dev->adapter);
> }
> }
> if (allow_sleep)
> @@ -376,8 +365,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
> dev->adapter.timeout);
> if (r == 0) {
> dev_err(dev->dev, "controller timed out\n");
> - davinci_i2c_recover_bus(dev);
> - i2c_davinci_init(dev);
> + i2c_recover_bus(adap);
> dev->buf_len = 0;
> return -ETIMEDOUT;
> }
> @@ -717,6 +705,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> adap->timeout = DAVINCI_I2C_TIMEOUT;
> adap->dev.of_node = pdev->dev.of_node;
>
> + if (dev->pdata->scl_pin) {
> + adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
> + adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
> + adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
> + }
> +
> adap->nr = pdev->id;
> r = i2c_add_numbered_adapter(adap);
> if (r) {
Just another general comment about the driver that doesn't influence the
correctness of this patch: The i2c-davinci driver is quite quick to
reset the bus. I wonder how often this reset triggers. Is the bus in
question less "stable" than others?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On 11/21/2014 09:07 PM, Uwe Kleine-K?nig wrote:
> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
>> This patch converts Davinci I2C driver to use I2C bus recovery
>> infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add
>> bus recovery infrastructure").
>>
>> The i2c_bus_recovery_info is configured for Davinci I2C adapter
>> only in case if scl_pin is provided in Platform data at least.
> s/Platform/platform/
>
>>
>> Because the controller must be held in reset while doing so, the
> s/Because/As/
>
>> recovery routine must re-init the controller. Since this was already
>> being done after each call to i2c_recover_bus, move those calls into
>> the recovery_prepare/unprepare routines and as well.
>>
>> CC: Sekhar Nori <[email protected]>
>> CC: Kevin Hilman <[email protected]>
>> CC: Santosh Shilimkar <[email protected]>
>> CC: Murali Karicheri <[email protected]>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-davinci.c | 76 ++++++++++++++++++----------------------
>> 1 file changed, 35 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 2cef115..db2a2cd 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>> return readw_relaxed(i2c_dev->base + reg);
>> }
>>
>> -/* Generate a pulse on the i2c clock pin. */
>> -static void davinci_i2c_clock_pulse(unsigned int scl_pin)
>> -{
>> - u16 i;
>> -
>> - if (scl_pin) {
>> - /* Send high and low on the SCL line */
>> - for (i = 0; i < 9; i++) {
>> - gpio_set_value(scl_pin, 0);
>> - udelay(20);
>> - gpio_set_value(scl_pin, 1);
>> - udelay(20);
>> - }
>> - }
>> -}
>> -
>> -/* This routine does i2c bus recovery as specified in the
>> - * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>> - */
>> -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
>> -{
>> - u32 flag = 0;
>> - struct davinci_i2c_platform_data *pdata = dev->pdata;
>> -
>> - dev_err(dev->dev, "initiating i2c bus recovery\n");
>> - /* Send NACK to the slave */
>> - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> - flag |= DAVINCI_I2C_MDR_NACK;
>> - /* write the data into mode register */
>> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>> - davinci_i2c_clock_pulse(pdata->scl_pin);
>> - /* Send STOP */
>> - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> - flag |= DAVINCI_I2C_MDR_STP;
>> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>> -}
>> -
>> static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
>> int val)
>> {
>> @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>> return 0;
>> }
>>
>> +/* This routine does i2c bus recovery as specified in the
>> + * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>> + */
> This comment is wrong. The actual bus clear is implemented by
> i2c_generic_gpio_recovery. Also while touching this comment, convert it
> to the usual format with /* on its own line. (The file in question has
> already both types of comment, so consistency is not a reason to keep it
> as is.)
It has been just copy-pasted, but ok.
I'll change this comment as following:
/*
* This routine does i2c bus recovery by using i2c_generic_gpio_recovery
* which is provided by I2C Bus recovery infrastructure.
*/
Is it ok?
>
> Even though I remember that I reviewed this bus recovery patch (that
> resulted in 5f9296ba21b3) back then, I don't remember why it was split
> in prepare + recover + unprepare. But that is unrelated to this patch.
>
>> +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
>> +{
>> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
>> +
>> + dev_err(dev->dev, "initiating i2c bus recovery\n");
>> + /* Disable interrupts */
>> + davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
>> +
>> + /* put I2C into reset */
>> + davinci_i2c_reset_ctrl(dev, 0);
>> +}
>> +
>> +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
>> +{
>> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
>> +
>> + i2c_davinci_init(dev);
>> +}
>> +
>> +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
> I'd call this only davinci_i2c_recovery_info.
No. Pls, see next patch.
>
>> + .recover_bus = i2c_generic_gpio_recovery,
>> + .prepare_recovery = davinci_i2c_prepare_recovery,
>> + .unprepare_recovery = davinci_i2c_unprepare_recovery,
>> +};
> new line here please
:) Ok.
Fixed in next patch.
>
>> /*
>> * Waiting for bus not busy
>> */
>> @@ -286,8 +276,7 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
>> return -ETIMEDOUT;
>> } else {
>> to_cnt = 0;
>> - davinci_i2c_recover_bus(dev);
>> - i2c_davinci_init(dev);
>> + i2c_recover_bus(&dev->adapter);
>> }
>> }
>> if (allow_sleep)
>> @@ -376,8 +365,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>> dev->adapter.timeout);
>> if (r == 0) {
>> dev_err(dev->dev, "controller timed out\n");
>> - davinci_i2c_recover_bus(dev);
>> - i2c_davinci_init(dev);
>> + i2c_recover_bus(adap);
>> dev->buf_len = 0;
>> return -ETIMEDOUT;
>> }
>> @@ -717,6 +705,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>> adap->timeout = DAVINCI_I2C_TIMEOUT;
>> adap->dev.of_node = pdev->dev.of_node;
>>
>> + if (dev->pdata->scl_pin) {
>> + adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
>> + adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
>> + adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
>> + }
>> +
>> adap->nr = pdev->id;
>> r = i2c_add_numbered_adapter(adap);
>> if (r) {
> Just another general comment about the driver that doesn't influence the
> correctness of this patch: The i2c-davinci driver is quite quick to
> reset the bus. I wonder how often this reset triggers. Is the bus in
> question less "stable" than others?
In comparison to ..? :)
regards,
-grygorii
Hello,
On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:
> @@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> &prop))
> dev->pdata->bus_freq = prop / 1000;
> + dev->pdata->has_pfunc = true;
I don't understand this. Why does this ICPFUNC recovery work if the bus
is probed by oftree, but doesn't if not?
> } else if (!dev->pdata) {
> dev->pdata = &davinci_i2c_platform_data_default;
> }
> @@ -705,7 +801,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> adap->timeout = DAVINCI_I2C_TIMEOUT;
> adap->dev.of_node = pdev->dev.of_node;
>
> - if (dev->pdata->scl_pin) {
> + if (dev->pdata->has_pfunc)
> + adap->bus_recovery_info = &davinci_i2c_scl_recovery_info;
> + else if (dev->pdata->scl_pin) {
> adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
> adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
> adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
> diff --git a/include/linux/platform_data/i2c-davinci.h b/include/linux/platform_data/i2c-davinci.h
> index 2312d19..89fd347 100644
> --- a/include/linux/platform_data/i2c-davinci.h
> +++ b/include/linux/platform_data/i2c-davinci.h
> @@ -18,6 +18,7 @@ struct davinci_i2c_platform_data {
> unsigned int bus_delay; /* post-transaction delay (usec) */
> unsigned int sda_pin; /* GPIO pin ID to use for SDA */
> unsigned int scl_pin; /* GPIO pin ID to use for SCL */
> + bool has_pfunc; /*chip has a ICPFUNC register */
Space after /* please
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Grygorii,
On Fri, Nov 21, 2014 at 05:33:37PM +0200, Grygorii Strashko wrote:
> On 11/21/2014 03:10 PM, Uwe Kleine-K?nig wrote:
> > On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
> >> On 11/21/2014 12:19 AM, Uwe Kleine-K?nig wrote:
> >>>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> >>>> index 9bbfb8f..2cef115 100644
> >>>> --- a/drivers/i2c/busses/i2c-davinci.c
> >>>> +++ b/drivers/i2c/busses/i2c-davinci.c
> >>>> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
> >>>> if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
> >>>> if (msg->flags & I2C_M_IGNORE_NAK)
> >>>> return msg->len;
> >>>> - if (stop) {
> >>>> - w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> >>>> - w |= DAVINCI_I2C_MDR_STP;
> >>>> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> >>>> - }
> >>>> + w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> >>>> + w |= DAVINCI_I2C_MDR_STP;
> >>>> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> >>> I think this is a good change, but I wonder if the handling of
> >>> I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
> >>> for the 2nd byte of a 5-byte-message, the transfer supposed to
> >>> continue, right? (Hmm, maybe the framework handle this and restarts the
> >>> transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
> >>> handle this flag?)
> >>
> >> Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
> >> change current behavior - davinci driver will interrupt transfer of i2c_msg always
> >> in case of NACK and start transfer of the next i2c_msg (if exist).
> >> In my opinion, Above question is out of scope of this patch.
> > Yeah right, that's exactly what I thought.
> >
> > Thinking again I wonder if with your change handling is correct when the
> > sender wants to do a repeated start. That would need a more detailed
> > look into the driver.
>
> Davinci driver will always abort transfer with error -EREMOTEIO in case if
> NACK received from I2C slave device. And the next omap_i2c_xfer() call may
> be *not* targeted to the same I2C slave device.
> ^ if !I2C_M_IGNORE_NAK
Does this resolve my concern? I think it doesn't. Also a Sr might well
address another device, doesn't it?
A call to .master_xfer with a message sequence implicitly expects ACKs
from the slave and doesn't tell anything about what should be done on a
NAK. So IMHO you must not send a P when the slave responds with a NAK,
but error out and let the sender decide if it wants to reply with P or
Sr.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Grygorii,
On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote:
> On 11/21/2014 09:07 PM, Uwe Kleine-K?nig wrote:
> > On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
> > Just another general comment about the driver that doesn't influence the
> > correctness of this patch: The i2c-davinci driver is quite quick to
> > reset the bus. I wonder how often this reset triggers. Is the bus in
> > question less "stable" than others?
>
> In comparison to ..? :)
In comparison to other bus drivers in other SoCs. I know this might be
hard to answer. I just wonder where the reason for this has to be
located. Strange hardware? Software bug? Or is this SoC just operating
with strange slaves more often than others?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On 11/23/2014 07:04 PM, Uwe Kleine-K?nig wrote:
> On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:
>> @@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>> if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
>> &prop))
>> dev->pdata->bus_freq = prop / 1000;
>> + dev->pdata->has_pfunc = true;
> I don't understand this. Why does this ICPFUNC recovery work if the bus
> is probed by oftree, but doesn't if not?
I've mentioned this in commit message:
Allow platforms to indicate the presence of the ICPFUNC registers with a has_pfunc
platform data flag and enable this mode for platforms which supports DT (da850 and
Keystone 2 are two SoCs which support DT now and they also support ICPFUNC registers).
I'll add proper comment here.
>
>> } else if (!dev->pdata) {
>> dev->pdata = &davinci_i2c_platform_data_default;
>> }
>> @@ -705,7 +801,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>> adap->timeout = DAVINCI_I2C_TIMEOUT;
>> adap->dev.of_node = pdev->dev.of_node;
>>
>> - if (dev->pdata->scl_pin) {
>> + if (dev->pdata->has_pfunc)
>> + adap->bus_recovery_info = &davinci_i2c_scl_recovery_info;
>> + else if (dev->pdata->scl_pin) {
>> adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
>> adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
>> adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
>> diff --git a/include/linux/platform_data/i2c-davinci.h b/include/linux/platform_data/i2c-davinci.h
>> index 2312d19..89fd347 100644
>> --- a/include/linux/platform_data/i2c-davinci.h
>> +++ b/include/linux/platform_data/i2c-davinci.h
>> @@ -18,6 +18,7 @@ struct davinci_i2c_platform_data {
>> unsigned int bus_delay; /* post-transaction delay (usec) */
>> unsigned int sda_pin; /* GPIO pin ID to use for SDA */
>> unsigned int scl_pin; /* GPIO pin ID to use for SCL */
>> + bool has_pfunc; /*chip has a ICPFUNC register */
> Space after /* please
>
regards,
-grygorii
Hi Uwe,
On 11/23/2014 10:36 PM, Uwe Kleine-K?nig wrote:
> On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote:
>> On 11/21/2014 09:07 PM, Uwe Kleine-K?nig wrote:
>>> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
>>> Just another general comment about the driver that doesn't influence the
>>> correctness of this patch: The i2c-davinci driver is quite quick to
>>> reset the bus. I wonder how often this reset triggers. Is the bus in
>>> question less "stable" than others?
>>
>> In comparison to ..? :)
> In comparison to other bus drivers in other SoCs. I know this might be
> hard to answer. I just wonder where the reason for this has to be
> located. Strange hardware? Software bug? Or is this SoC just operating
> with strange slaves more often than others?
Davinci driver does reset in two cases:
- when I2C transaction isn't completed due to timeout (no irq received)
- when BB is detected
both cases are reasonable, because in 1st case HW state is undefined
in 2d case - Davinci I2C supports only master mode and if BB detected
we need perform some recovery procedure.
Also, this patch doesn't introduce functional changes - it's just code
reworking intended to reuse I2C bus recovery infrastructure
i2c-omap.c - OMAP I2C driver does mostly the same now.
i2c-tegra.c - seems, It will do reset even frequently.
i2c-imx.c - if understand right, it will reinitialize I2C controller
before each transfer, because it enables/disables I2C clocks.
...
So, what i can say here is just "In comparison to ..?" :)
regards,
-grygorii
Hi Uwe,
On 11/23/2014 10:33 PM, Uwe Kleine-K?nig wrote:
> On Fri, Nov 21, 2014 at 05:33:37PM +0200, Grygorii Strashko wrote:
>> On 11/21/2014 03:10 PM, Uwe Kleine-K?nig wrote:
>>> On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
>>>> On 11/21/2014 12:19 AM, Uwe Kleine-K?nig wrote:
>>>>>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>>>>>> index 9bbfb8f..2cef115 100644
>>>>>> --- a/drivers/i2c/busses/i2c-davinci.c
>>>>>> +++ b/drivers/i2c/busses/i2c-davinci.c
>>>>>> @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>>>>> if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
>>>>>> if (msg->flags & I2C_M_IGNORE_NAK)
>>>>>> return msg->len;
>>>>>> - if (stop) {
>>>>>> - w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>>>>>> - w |= DAVINCI_I2C_MDR_STP;
>>>>>> - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>>>>> - }
>>>>>> + w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>>>>>> + w |= DAVINCI_I2C_MDR_STP;
>>>>>> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>>>> I think this is a good change, but I wonder if the handling of
>>>>> I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
>>>>> for the 2nd byte of a 5-byte-message, the transfer supposed to
>>>>> continue, right? (Hmm, maybe the framework handle this and restarts the
>>>>> transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
>>>>> handle this flag?)
>>>>
>>>> Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
>>>> change current behavior - davinci driver will interrupt transfer of i2c_msg always
>>>> in case of NACK and start transfer of the next i2c_msg (if exist).
>>>> In my opinion, Above question is out of scope of this patch.
>>> Yeah right, that's exactly what I thought.
>>>
>>> Thinking again I wonder if with your change handling is correct when the
>>> sender wants to do a repeated start. That would need a more detailed
>>> look into the driver.
>>
>> Davinci driver will always abort transfer with error -EREMOTEIO in case if
>> NACK received from I2C slave device. And the next omap_i2c_xfer() call may
>> be *not* targeted to the same I2C slave device.
>> ^ if !I2C_M_IGNORE_NAK
> Does this resolve my concern? I think it doesn't. Also a Sr might well
> address another device, doesn't it?
>
> A call to .master_xfer with a message sequence implicitly expects ACKs
> from the slave and doesn't tell anything about what should be done on a
> NAK. So IMHO you must not send a P when the slave responds with a NAK,
> but error out and let the sender decide if it wants to reply with P or
> Sr.
Sry, but what should be done is defined by I2C/SMbus specs? Does it?
For SMBus devices, the specification states (http://smbus.org/specs/)
"4.2.Acknowledge (ACK) and not acknowledge (NACK)":
- "The slave device detects an invalid command or invalid data. In this
case the slave device must not acknowledge the received byte. The master
upon detection of this condition must generate a STOP condition and
retry the transaction"
For I2C devices, the specification states [http://www.nxp.com/documents/user_manual/UM10204.pdf]:
"3.1.6 Acknowledge (ACK) and Not Acknowledge (NACK)"
"When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then generate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer."
Let take a look on i2c/smbus xfer:
i2c_lock_adapter(adap)
adap->algo->master_xfer/smbus_xfer()
i2c_unlock_adapter(adap);
|- rt_mutex_unlock(&adapter->bus_lock);
|- task switch
So, there is no guarantee that next xfer will address the same I2C client device,
which, in turn, may lead to BB detection (will lead to BB detection if previous
transfer has been not acknowledged by SMbus client device).
Small summary, I2C core + Davinci I2C driver provide ability to use repeated
start (Sr) only within one I2C transaction - which is a number of write/read
operations specified by i2c_msg array. NACK always interrupts transaction
with -EREMOTEIO.
Also, the I2C core doesn't provide ability to manually send P.
regards,
-grygorii
On 24-11-2014 14:15, Grygorii Strashko wrote:
> Hi Uwe,
> On 11/23/2014 07:04 PM, Uwe Kleine-K?nig wrote:
>> On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:
>>> @@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>> if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
>>> &prop))
>>> dev->pdata->bus_freq = prop / 1000;
>>> + dev->pdata->has_pfunc = true;
>> I don't understand this. Why does this ICPFUNC recovery work if the bus
>> is probed by oftree, but doesn't if not?
> I've mentioned this in commit message:
> Allow platforms to indicate the presence of the ICPFUNC registers with a has_pfunc
> platform data flag and enable this mode for platforms which supports DT (da850 and
> Keystone 2 are two SoCs which support DT now and they also support ICPFUNC registers).
>
> I'll add proper comment here.
Just thinking: What happens if you try to use the ICPFUNC registers on
platforms that don't support it? If the answer is "nothing bad", then
you might as well assume that if the platform doesn't specify its own
GPIOs, you can always try using the ICPFUNC registers to shake the I2C
bus. Better to try and fail than to never try at all...
>
>>> } else if (!dev->pdata) {
>>> dev->pdata = &davinci_i2c_platform_data_default;
>>> }
>>> @@ -705,7 +801,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>> adap->timeout = DAVINCI_I2C_TIMEOUT;
>>> adap->dev.of_node = pdev->dev.of_node;
>>>
>>> - if (dev->pdata->scl_pin) {
>>> + if (dev->pdata->has_pfunc)
>>> + adap->bus_recovery_info = &davinci_i2c_scl_recovery_info;
>>> + else if (dev->pdata->scl_pin) {
>>> adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
>>> adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
>>> adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
>>> diff --git a/include/linux/platform_data/i2c-davinci.h b/include/linux/platform_data/i2c-davinci.h
>>> index 2312d19..89fd347 100644
>>> --- a/include/linux/platform_data/i2c-davinci.h
>>> +++ b/include/linux/platform_data/i2c-davinci.h
>>> @@ -18,6 +18,7 @@ struct davinci_i2c_platform_data {
>>> unsigned int bus_delay; /* post-transaction delay (usec) */
>>> unsigned int sda_pin; /* GPIO pin ID to use for SDA */
>>> unsigned int scl_pin; /* GPIO pin ID to use for SCL */
>>> + bool has_pfunc; /*chip has a ICPFUNC register */
>> Space after /* please
>>
> regards,
> -grygorii
>
>
>
On 11/24/2014 08:13 PM, Mike Looijmans wrote:
> On 24-11-2014 14:15, Grygorii Strashko wrote:
>> Hi Uwe,
>> On 11/23/2014 07:04 PM, Uwe Kleine-K?nig wrote:
>>> On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:
>>>> @@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct
>>>> platform_device *pdev)
>>>> if (!of_property_read_u32(pdev->dev.of_node,
>>>> "clock-frequency",
>>>> &prop))
>>>> dev->pdata->bus_freq = prop / 1000;
>>>> + dev->pdata->has_pfunc = true;
>>> I don't understand this. Why does this ICPFUNC recovery work if the bus
>>> is probed by oftree, but doesn't if not?
>> I've mentioned this in commit message:
>> Allow platforms to indicate the presence of the ICPFUNC registers
>> with a has_pfunc
>> platform data flag and enable this mode for platforms which supports
>> DT (da850 and
>> Keystone 2 are two SoCs which support DT now and they also support
>> ICPFUNC registers).
>>
>> I'll add proper comment here.
>
> Just thinking: What happens if you try to use the ICPFUNC registers on
> platforms that don't support it? If the answer is "nothing bad", then
> you might as well assume that if the platform doesn't specify its own
> GPIOs, you can always try using the ICPFUNC registers to shake the I2C
> bus. Better to try and fail than to never try at all...
>
I think the right answer is !"nothing bad".
My intention was to enable this feature by default, because current DT-compatible
SoCs support it and the possibility that older SoCs will migrate to DT is low.
But now I think that the right way will be to add proper compatible strings
and use them to detect if ICPFUNC registers are supported or not.
[...]
regards,
-grygorii
Hi Grygorii,
On Mon, Nov 24, 2014 at 03:15:58PM +0200, Grygorii Strashko wrote:
> On 11/23/2014 07:04 PM, Uwe Kleine-K?nig wrote:
> > On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:
> >> @@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> >> if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> >> &prop))
> >> dev->pdata->bus_freq = prop / 1000;
> >> + dev->pdata->has_pfunc = true;
> > I don't understand this. Why does this ICPFUNC recovery work if the bus
> > is probed by oftree, but doesn't if not?
>
> I've mentioned this in commit message:
> Allow platforms to indicate the presence of the ICPFUNC registers with a has_pfunc
> platform data flag and enable this mode for platforms which supports DT (da850 and
> Keystone 2 are two SoCs which support DT now and they also support ICPFUNC registers).
Ah, so you assume that in the dt case the pfunc functionality is
available. I didn't understand if it's bad or not if this assumption is
wrong, but I suggest to only use the pfunc stuff if the device node has
a given property (e.g. ti,has_pfunc).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Grygorii,
On Mon, Nov 24, 2014 at 03:34:35PM +0200, Grygorii Strashko wrote:
> On 11/23/2014 10:33 PM, Uwe Kleine-K?nig wrote:
> > A call to .master_xfer with a message sequence implicitly expects ACKs
> > from the slave and doesn't tell anything about what should be done on a
> > NAK. So IMHO you must not send a P when the slave responds with a NAK,
> > but error out and let the sender decide if it wants to reply with P or
> > Sr.
>
> Sry, but what should be done is defined by I2C/SMbus specs? Does it?
> For SMBus devices, the specification states (http://smbus.org/specs/)
> "4.2.Acknowledge (ACK) and not acknowledge (NACK)":
> - "The slave device detects an invalid command or invalid data. In this
> case the slave device must not acknowledge the received byte. The master
> upon detection of this condition must generate a STOP condition and
> retry the transaction"
> For I2C devices, the specification states [http://www.nxp.com/documents/user_manual/UM10204.pdf]:
> "3.1.6 Acknowledge (ACK) and Not Acknowledge (NACK)"
> "When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
> Acknowledge signal. The master can then generate either a STOP condition to
> abort the transfer, or a repeated START condition to start a new transfer."
Yes, that's exactly what I meant. The master has the choice, and the
driver should not eliminate options here.
> Let take a look on i2c/smbus xfer:
> i2c_lock_adapter(adap)
> adap->algo->master_xfer/smbus_xfer()
> i2c_unlock_adapter(adap);
> |- rt_mutex_unlock(&adapter->bus_lock);
> |- task switch
>
> So, there is no guarantee that next xfer will address the same I2C client device,
> which, in turn, may lead to BB detection (will lead to BB detection if previous
> transfer has been not acknowledged by SMbus client device).
That's a valid concern.
> Small summary, I2C core + Davinci I2C driver provide ability to use repeated
> start (Sr) only within one I2C transaction - which is a number of write/read
> operations specified by i2c_msg array. NACK always interrupts transaction
> with -EREMOTEIO.
>
> Also, the I2C core doesn't provide ability to manually send P.
Hmm, Wolfram, what do you think?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Grygorii,
On Mon, Nov 24, 2014 at 03:26:10PM +0200, Grygorii Strashko wrote:
> On 11/23/2014 10:36 PM, Uwe Kleine-K?nig wrote:
> > On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote:
> >> On 11/21/2014 09:07 PM, Uwe Kleine-K?nig wrote:
> >>> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
> >>> Just another general comment about the driver that doesn't influence the
> >>> correctness of this patch: The i2c-davinci driver is quite quick to
> >>> reset the bus. I wonder how often this reset triggers. Is the bus in
> >>> question less "stable" than others?
> >>
> >> In comparison to ..? :)
> > In comparison to other bus drivers in other SoCs. I know this might be
> > hard to answer. I just wonder where the reason for this has to be
> > located. Strange hardware? Software bug? Or is this SoC just operating
> > with strange slaves more often than others?
>
> Davinci driver does reset in two cases:
> - when I2C transaction isn't completed due to timeout (no irq received)
> - when BB is detected
> both cases are reasonable, because in 1st case HW state is undefined
> in 2d case - Davinci I2C supports only master mode and if BB detected
> we need perform some recovery procedure.
note that for multi-master scenarios bus recovery might be bad. So that
has not necessarily something to do with "master mode only".
> Also, this patch doesn't introduce functional changes - it's just code
> reworking intended to reuse I2C bus recovery infrastructure
Yeah this is fine, and my concern shouldn't stop your patch from getting
in as it is an improvement for sure. Still I only had to handle BB once
in my linux lifetime, and that was for a different reason. (Machine
doing massive i2c transfers and so a client was often holding the bus
busy when the watchdog resetted the cpu.) So I just wondered how often
the need for recovery arises.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On 11/24/2014 09:45 PM, Uwe Kleine-K?nig wrote:
> On Mon, Nov 24, 2014 at 03:15:58PM +0200, Grygorii Strashko wrote:
>> On 11/23/2014 07:04 PM, Uwe Kleine-K?nig wrote:
>>> On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:
>>>> @@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>>> if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
>>>> &prop))
>>>> dev->pdata->bus_freq = prop / 1000;
>>>> + dev->pdata->has_pfunc = true;
>>> I don't understand this. Why does this ICPFUNC recovery work if the bus
>>> is probed by oftree, but doesn't if not?
>>
>> I've mentioned this in commit message:
>> Allow platforms to indicate the presence of the ICPFUNC registers with a has_pfunc
>> platform data flag and enable this mode for platforms which supports DT (da850 and
>> Keystone 2 are two SoCs which support DT now and they also support ICPFUNC registers).
> Ah, so you assume that in the dt case the pfunc functionality is
> available. I didn't understand if it's bad or not if this assumption is
> wrong, but I suggest to only use the pfunc stuff if the device node has
> a given property (e.g. ti,has_pfunc).
Agree. I'll add it.
regards,
-grygorii