This series for the i2c-davinci driver implements both a register dump and
a pulsed-SCL recovery of the bus on those controllers that have ICPFUNC
registers which is, so far, the da850 and da830 based platforms.
I2C "controller timed out" messages were being observed by both myself on the
da850evm and Bastian Ruppert on some custom da850 hardware [1]. Originally I
thought it was the existing pulse-SCL routine but was quickly proven wrong; my
apologies to John Povey and Philby John for jumping to conclusions.
A discussion was spawned on the e2e forums [2] where Brad Griffis was
instrumental in the development of the recovery routine proposed by this patch
series. It was pointed out by him that the da850's i2c controller has the
ability to control the SDA and SCL pins as GPIOS through its ICPFUNC registers.
The recovery routine implemented by the patch series toggles the SCL pin and
reads the SDA state using the ICPFUNC registers.
The changes in this series are staged in increments of features and each patch
depends on the changes introduced by the patch before it with the exception of
patch 2/6 which is largely independent of 1/6.
The real meat of the series in in patch 4/6. First we introduce a register
dump routine in 1/6 since this information was requested immeadiately by Brad
Griffis when the conversation began; then the i2c-davinci platform data
structure's comments are converted to kerneldoc in 2/6. Then in 3/6 a level of
indirection is introduced so that the implementation of the recovery procedure
can be switched at runtime; this level of indirection is used in the subsequent
patch, 4/6, to execute a pulsed-SCL recovery routine using the ICPFUNC
registers if they are present. Where the presence of the registers is indicated
by the platform in a has_pfunc flag in platform data. Finally since all da8xx
platforms' i2c controllers have the ICPFUNC registers, the da8xx utility
function to register i2c controllers is modified to set the flag so that the
new recovery routine is used in all da8xx platforms.
I developed this patch against v2.6.38 but I have also tested it against
v2.6.39-rc1: it applies cleanly and the recovery routine is executed as it was
in v2.6.38.
When creating this series I noticed that there are obvious similarities between
the existing recovery routine implemented by Philby John and John Povey and the
recovery routine proposed in this series. Testing was performed using the
ICPFUNC regsiters to toggle SCL as prescribed by the existing routine and it
was found that the recovery did not work. The method described initially by Brad
Griffis had the initial state of SCL high and delays of 5us with a maximum of
8 pulses with a check of SDA each time as compared to the existing routine with
undetermined SCL initial state, 20us delays and a maximum of 8 pulses. I tested
and found that if I changed the initial state of SCL or the number of pulses
(Bastian had success with 16) that the recovery did not occur as expected;
furthermore Brad pointed out that it was important to check the state of SDA
after each pulse to see if the slave had released the line. Indeed, adding the
check of SDA resulted in a quicker recovery. On my da850evm, at least, the
recovery took only one SCL pulse every time.
It would be nice to consolidate the two recovery routines and -- since they
are gpio-based -- put the shared recovery routine in i2c-algo-bit so it can
also be used by bitbanging i2c masters. I did not undertake the former because
I don't have access to the hardware on which the existing recovery routine was
tested. I did not undertake the latter since 1) it seems premature until the
recovery routines are consolidated (or not) and 2) it would require more
changes of a broad scope which I feared might mire the review process of this
series which is, at its core, a functioning workaround of an observed problem
with da850evm hardware (plus some regsiter dumps). It is my hope that both the
consolidation of the recovery routines and the extraction to common bitbanging
code can be done in a later series.
[1] http://permalink.gmane.org/gmane.linux.davinci/22291
[2] http://e2e.ti.com/support/dsp/omap_applications_processors/f/42/p/99895/350610.aspx
Ben Gardiner (6):
i2c-davinci: register dump before attempted bus recovery
i2c-davinci: convert davinci_i2c_platform_data to kerneldoc
i2c-davinci: introduce a dev-> function pointer for scl pulsing
i2c-davinci: use the DA8xx's ICPFUNC to toggle I2C as gpio
i2c-davinci: if device has pfunc register, dump that group also
da8xx: enable the use of the ICPFUNC in i2c-davinci
arch/arm/mach-davinci/devices-da8xx.c | 6 +
arch/arm/mach-davinci/include/mach/i2c.h | 21 +++-
drivers/i2c/busses/i2c-davinci.c | 155 ++++++++++++++++++++++++++++--
3 files changed, 170 insertions(+), 12 deletions(-)
Produce a dump of the register contents of the I2C controller (at the debug
level) whenever an i2c recovery is initiated.
Signed-off-by: Ben Gardiner <[email protected]>
Cc: Bastian Ruppert <[email protected]>
Cc: Brad Griffis <[email protected]>
Cc: Sekhar Nori <[email protected]>
Cc: Ben Dooks <[email protected]>
---
The motivation for the introduction of this additional debugging information is
that when I2C recovery on da850 began discussion in the e2e forums [1] a
register dump was requested.
[1] http://e2e.ti.com/support/dsp/omap_applications_processors/f/42/p/99895/350610.aspx#350610
drivers/i2c/busses/i2c-davinci.c | 32 ++++++++++++++++++++++++++++++++
1 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 5795c83..7011222 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -133,6 +133,37 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
return __raw_readw(i2c_dev->base + reg);
}
+static void i2c_davinci_dump_regs(struct davinci_i2c_dev *dev)
+{
+ dev_dbg(dev->dev, "PSC = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_PSC_REG));
+ dev_dbg(dev->dev, "CLKL = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_CLKL_REG));
+ dev_dbg(dev->dev, "CLKH = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_CLKH_REG));
+ dev_dbg(dev->dev, "MDR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG));
+
+ dev_dbg(dev->dev, "OAR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_OAR_REG));
+ dev_dbg(dev->dev, "IMR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG));
+ dev_dbg(dev->dev, "STR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG));
+ dev_dbg(dev->dev, "CNT = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_CNT_REG));
+ dev_dbg(dev->dev, "DRR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_DRR_REG));
+ dev_dbg(dev->dev, "SAR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_SAR_REG));
+ dev_dbg(dev->dev, "DXR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_DXR_REG));
+ dev_dbg(dev->dev, "IVR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG));
+ dev_dbg(dev->dev, "EMDR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_EMDR_REG));
+}
+
/* Generate a pulse on the i2c clock pin. */
static void generic_i2c_clock_pulse(unsigned int scl_pin)
{
@@ -157,6 +188,7 @@ static void i2c_recover_bus(struct davinci_i2c_dev *dev)
u32 flag = 0;
struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
+ i2c_davinci_dump_regs(dev);
dev_err(dev->dev, "initiating i2c bus recovery\n");
/* Send NACK to the slave */
flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
--
1.7.1
The davinci_i2c_platform_data struct is mach-specific but still deserves
to be documented using kernel doc.
Signed-off-by: Ben Gardiner<[email protected]>
Cc: Sekhar Nori <[email protected]>
Cc: Ben Dooks <[email protected]>
---
arch/arm/mach-davinci/include/mach/i2c.h | 17 ++++++++++++-----
1 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-davinci/include/mach/i2c.h b/arch/arm/mach-davinci/include/mach/i2c.h
index 2312d19..ab07a44 100644
--- a/arch/arm/mach-davinci/include/mach/i2c.h
+++ b/arch/arm/mach-davinci/include/mach/i2c.h
@@ -12,12 +12,19 @@
#ifndef __ASM_ARCH_I2C_H
#define __ASM_ARCH_I2C_H
-/* All frequencies are expressed in kHz */
+/**
+ * davinci_i2c_platform_data - Platform data for I2C master device on DaVinci
+ *
+ * @bus_freq: standard bus frequency (kHz)
+ * @bus_delay: post-transaction delay (usec)
+ * @sda_pin: GPIO pin ID to use for SDA
+ * @scl_pin: GPIO pin ID to use for SCL
+ */
struct davinci_i2c_platform_data {
- unsigned int bus_freq; /* standard bus frequency (kHz) */
- 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 */
+ unsigned int bus_freq;
+ unsigned int bus_delay;
+ unsigned int sda_pin;
+ unsigned int scl_pin;
};
/* for board setup code */
--
1.7.1
On the da850evm, with the default config of polling keys enabled, several
"controller timed out" errors were observed on the console; as well as the
same error observed on custom hardware when communicating with a i2c
touchscreen device. [1]
Discussion of the causes and potential workarounds began on the e2e forums [2]
where Brad Griffis pointed out that the da850 (and da830) has an i2c controller
whose SCL and SDA may be manipulated as GPIOs by using the ICPFUNC registers
of the i2c controller. He further suggested a means of using this feature to
send clock pulses on the I2C bus to free the frozen slave device.
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.
[1] http://permalink.gmane.org/gmane.linux.davinci/22291
[2] http://e2e.ti.com/support/dsp/omap_applications_processors/f/42/p/99895/350610.aspx
Signed-off-by: Ben Gardiner <[email protected]>
Cc: Bastian Ruppert <[email protected]>
Cc: Brad Griffis <[email protected]>
Cc: Jon Povey <[email protected]>
Cc: Philby John <[email protected]>
Cc: Sekhar Nori <[email protected]>
Cc: Ben Dooks <[email protected]>
---
arch/arm/mach-davinci/include/mach/i2c.h | 6 ++-
drivers/i2c/busses/i2c-davinci.c | 88 ++++++++++++++++++++++++++++++
2 files changed, 93 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-davinci/include/mach/i2c.h b/arch/arm/mach-davinci/include/mach/i2c.h
index ab07a44..858c5c6 100644
--- a/arch/arm/mach-davinci/include/mach/i2c.h
+++ b/arch/arm/mach-davinci/include/mach/i2c.h
@@ -19,12 +19,16 @@
* @bus_delay: post-transaction delay (usec)
* @sda_pin: GPIO pin ID to use for SDA
* @scl_pin: GPIO pin ID to use for SCL
+ * @has_pfunc: set this to true if the i2c controller on your chip has a
+ * ICPFUNC register which allows the SDA and SCL pins to be
+ * controlled as gpio (like in DA850)
*/
struct davinci_i2c_platform_data {
unsigned int bus_freq;
unsigned int bus_delay;
unsigned int sda_pin;
unsigned int scl_pin;
+ bool has_pfunc;
};
/* for board setup code */
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 0a2c697..5bdc98c 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -65,6 +65,11 @@
#define DAVINCI_I2C_IVR_REG 0x28
#define DAVINCI_I2C_EMDR_REG 0x2c
#define DAVINCI_I2C_PSC_REG 0x30
+#define DAVINCI_I2C_PFUNC_REG 0x48
+#define DAVINCI_I2C_PDIR_REG 0x4c
+#define DAVINCI_I2C_PDIN_REG 0x50
+#define DAVINCI_I2C_DSET_REG 0x58
+#define DAVINCI_I2C_DCLR_REG 0x5c
#define DAVINCI_I2C_IVR_AAS 0x07
#define DAVINCI_I2C_IVR_SCD 0x06
@@ -98,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_PFUNC_PFUNC0 BIT(0)
+
+/* set SCL as output when used as GPIO*/
+#define DAVINCI_I2C_PDIR_PDIR0 BIT(0)
+/* set SDA as output when used as GPIO*/
+#define DAVINCI_I2C_PDIR_PDIR1 BIT(1)
+
+/* read SCL GPIO level */
+#define DAVINCI_I2C_PDIN_PDIN0 BIT(0)
+/* read SDA GPIO level */
+#define DAVINCI_I2C_PDIN_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;
@@ -182,6 +210,64 @@ static void generic_i2c_clock_pulse(struct davinci_i2c_dev *dev)
}
}
+static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
+ int val);
+
+/* Generate gpio a pulse on the i2c clock pin. */
+static void i2c_davinci_pfunc_i2c_clock_pulse(struct davinci_i2c_dev *dev)
+{
+ u32 flag = 0;
+ u16 i;
+
+ davinci_i2c_reset_ctrl(dev, 0);
+
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0x00);
+
+ /* SCL output, SDA input */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_PDIR_REG,
+ DAVINCI_I2C_PDIR_PDIR0);
+
+ /* change to GPIO mode */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_PFUNC_REG,
+ DAVINCI_I2C_PFUNC_PFUNC0);
+
+ /* SCL high */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_DSET_REG,
+ DAVINCI_I2C_DSET_PDSET0);
+ udelay(5);
+ for (i = 0; i < 16; i++) {
+ /* SCL low */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_DCLR_REG,
+ DAVINCI_I2C_DCLR_PDCLR0);
+ udelay(5);
+ /* SCL high */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_DSET_REG,
+ DAVINCI_I2C_DSET_PDSET0);
+ udelay(5);
+
+ /* read the state of SDA */
+ flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_PDIN_REG);
+ if (flag & DAVINCI_I2C_PDIN_PDIN1) {
+ dev_dbg(dev->dev, "recovered after %d SCL pulses",
+ i + 1);
+ break;
+ }
+ }
+
+ /* change back to I2C mode */
+ davinci_i2c_write_reg(dev, DAVINCI_I2C_PFUNC_REG, 0);
+
+ /* take the I2C dev out of reset */
+ davinci_i2c_reset_ctrl(dev, 1);
+
+ /* read the state of SDA */
+ flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_PDIN_REG);
+ if (flag & DAVINCI_I2C_PDIN_PDIN1)
+ return;
+
+ dev_err(dev->dev, "I2C slave will not release SDA.\n");
+}
+
/* This routine does i2c bus recovery as specified in the
* i2c protocol Rev. 03 section 3.16 titled "Bus clear"
*/
@@ -756,6 +842,8 @@ static int davinci_i2c_probe(struct platform_device *pdev)
pdata = dev->dev->platform_data;
if (pdata->scl_pin)
dev->pulse_scl = generic_i2c_clock_pulse;
+ else if (pdata->has_pfunc)
+ dev->pulse_scl = i2c_davinci_pfunc_i2c_clock_pulse;
return 0;
--
1.7.1
If the platform has indicated that this I2c controller has the ICPFUNC
regsiters then also dump their contents when producing a debug register dump.
Signed-off-by: Ben Gardiner <[email protected]>
Cc: Bastian Ruppert <[email protected]>
Cc: Brad Griffis <[email protected]>
Cc: Sekhar Nori <[email protected]>
Cc: Ben Dooks <[email protected]>
---
drivers/i2c/busses/i2c-davinci.c | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 5bdc98c..a084e50 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -164,6 +164,8 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
static void i2c_davinci_dump_regs(struct davinci_i2c_dev *dev)
{
+ struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
+
dev_dbg(dev->dev, "PSC = %08x\n",
davinci_i2c_read_reg(dev, DAVINCI_I2C_PSC_REG));
dev_dbg(dev->dev, "CLKL = %08x\n",
@@ -191,6 +193,19 @@ static void i2c_davinci_dump_regs(struct davinci_i2c_dev *dev)
davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG));
dev_dbg(dev->dev, "EMDR = %08x\n",
davinci_i2c_read_reg(dev, DAVINCI_I2C_EMDR_REG));
+
+ if (pdata->has_pfunc) {
+ dev_dbg(dev->dev, "PFUNC = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_PFUNC_REG));
+ dev_dbg(dev->dev, "PDIR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_PDIR_REG));
+ dev_dbg(dev->dev, "PDIN = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_PDIN_REG));
+ dev_dbg(dev->dev, "DSET = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_DSET_REG));
+ dev_dbg(dev->dev, "DCLR = %08x\n",
+ davinci_i2c_read_reg(dev, DAVINCI_I2C_DCLR_REG));
+ }
}
/* Generate a pulse on the i2c clock pin. */
--
1.7.1
Both the da850 and da830 have an I2C controller which has the ICPFUNC
registers. Indicate this for all da830 and da850 boards by setting the
has_pfunc flag true in the da8xx utility setup routine for registering the
I2C controller
Signed-off-by: Ben Gardiner <[email protected]>
Cc: Sekhar Nori <[email protected]>
Cc: Ben Dooks <[email protected]>
---
arch/arm/mach-davinci/devices-da8xx.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index beda8a4..da01558 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance,
else
return -EINVAL;
+ /*
+ * Both the DA850 and DA830 have an I2C controller which has the
+ * ICPFUNC et. al. registers
+ */
+ pdata->has_pfunc = 1;
+
pdev->dev.platform_data = pdata;
return platform_device_register(pdev);
}
--
1.7.1
The current implementation of the I2C recovery routine checks whether
platform data ->scl_pin has been assigned and if so, the generic_i2c_clock_pulse
routine is executed.
In preparation for an alternative recovery routine; introduce a pulse_scl
function pointer to the driver private structure and assign it the value of
generic_i2c_clock_pulse on init if the scl_pin is assigned in platform data.
Signed-off-by: Ben Gardiner<[email protected]>
Cc: Sekhar Nori <[email protected]>
Cc: Ben Dooks <[email protected]>
---
drivers/i2c/busses/i2c-davinci.c | 20 +++++++++++++-------
1 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 7011222..0a2c697 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -110,6 +110,7 @@ struct davinci_i2c_dev {
int stop;
u8 terminate;
struct i2c_adapter adapter;
+ void (*pulse_scl) (struct davinci_i2c_dev *dev);
#ifdef CONFIG_CPU_FREQ
struct completion xfr_complete;
struct notifier_block freq_transition;
@@ -165,16 +166,17 @@ static void i2c_davinci_dump_regs(struct davinci_i2c_dev *dev)
}
/* Generate a pulse on the i2c clock pin. */
-static void generic_i2c_clock_pulse(unsigned int scl_pin)
+static void generic_i2c_clock_pulse(struct davinci_i2c_dev *dev)
{
+ struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
u16 i;
- if (scl_pin) {
+ if (pdata->scl_pin) {
/* Send high and low on the SCL line */
for (i = 0; i < 9; i++) {
- gpio_set_value(scl_pin, 0);
+ gpio_set_value(pdata->scl_pin, 0);
udelay(20);
- gpio_set_value(scl_pin, 1);
+ gpio_set_value(pdata->scl_pin, 1);
udelay(20);
}
}
@@ -186,7 +188,6 @@ static void generic_i2c_clock_pulse(unsigned int scl_pin)
static void i2c_recover_bus(struct davinci_i2c_dev *dev)
{
u32 flag = 0;
- struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
i2c_davinci_dump_regs(dev);
dev_err(dev->dev, "initiating i2c bus recovery\n");
@@ -195,8 +196,8 @@ static void i2c_recover_bus(struct davinci_i2c_dev *dev)
flag |= DAVINCI_I2C_MDR_NACK;
/* write the data into mode register */
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
- if (pdata)
- generic_i2c_clock_pulse(pdata->scl_pin);
+ if (dev->pulse_scl)
+ dev->pulse_scl(dev);
/* Send STOP */
flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
flag |= DAVINCI_I2C_MDR_STP;
@@ -669,6 +670,7 @@ static struct i2c_algorithm i2c_davinci_algo = {
static int davinci_i2c_probe(struct platform_device *pdev)
{
+ struct davinci_i2c_platform_data *pdata;
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
struct resource *mem, *irq, *ioarea;
@@ -751,6 +753,10 @@ static int davinci_i2c_probe(struct platform_device *pdev)
goto err_free_irq;
}
+ pdata = dev->dev->platform_data;
+ if (pdata->scl_pin)
+ dev->pulse_scl = generic_i2c_clock_pulse;
+
return 0;
err_free_irq:
--
1.7.1
Hi Ben,
On Wed, Apr 06, 2011 at 03:08:09, Ben Gardiner wrote:
> Both the da850 and da830 have an I2C controller which has the ICPFUNC
> registers. Indicate this for all da830 and da850 boards by setting the
> has_pfunc flag true in the da8xx utility setup routine for registering the
> I2C controller
>
> Signed-off-by: Ben Gardiner <[email protected]>
> Cc: Sekhar Nori <[email protected]>
> Cc: Ben Dooks <[email protected]>
>
> ---
> arch/arm/mach-davinci/devices-da8xx.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
> index beda8a4..da01558 100644
> --- a/arch/arm/mach-davinci/devices-da8xx.c
> +++ b/arch/arm/mach-davinci/devices-da8xx.c
> @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance,
> else
> return -EINVAL;
>
> + /*
> + * Both the DA850 and DA830 have an I2C controller which has the
> + * ICPFUNC et. al. registers
> + */
> + pdata->has_pfunc = 1;
The I2C driver implements a default platform data
so it should actually be legal for a DA8x board to
pass NULL platform data. In that case this line
will crash. You should either check for pdata to
be NULL or just let each board choose whether it
needs recovery (I think the better option).
Thanks,
Sekhar
Hi Sekhar,
On Wed, Apr 13, 2011 at 11:10 AM, Nori, Sekhar <[email protected]> wrote:
> Hi Ben,
>
> On Wed, Apr 06, 2011 at 03:08:09, Ben Gardiner wrote:
>> Both the da850 and da830 have an I2C controller which has the ICPFUNC
>> registers. Indicate this for all da830 and da850 boards by setting the
>> has_pfunc flag true in the da8xx utility setup routine for registering the
>> I2C controller
>>
>> Signed-off-by: Ben Gardiner <[email protected]>
>> Cc: Sekhar Nori <[email protected]>
>> Cc: Ben Dooks <[email protected]>
>>
>> ---
>> ?arch/arm/mach-davinci/devices-da8xx.c | ? ?6 ++++++
>> ?1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
>> index beda8a4..da01558 100644
>> --- a/arch/arm/mach-davinci/devices-da8xx.c
>> +++ b/arch/arm/mach-davinci/devices-da8xx.c
>> @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance,
>> ? ? ? else
>> ? ? ? ? ? ? ? return -EINVAL;
>>
>> + ? ? /*
>> + ? ? ?* Both the DA850 and DA830 have an I2C controller which has the
>> + ? ? ?* ICPFUNC et. al. registers
>> + ? ? ?*/
>> + ? ? pdata->has_pfunc = 1;
>
> The I2C driver implements a default platform data
> so it should actually be legal for a DA8x board to
> pass NULL platform data. In that case this line
> will crash.
Good catch, thanks.
> [...] You should either check for pdata to
> be NULL or just let each board choose whether it
> needs recovery (I think the better option).
I understand "check for pdata to be NULL." If you think it is the
better option I'd be happy to implement it but I don't understand how
to implement "let each board choose whether it needs recovery."
Best Regards,
Ben Gardiner
---
Nanometrics Inc.
http://www.nanometrics.ca
On Wed, Apr 13, 2011 at 20:52:05, Ben Gardiner wrote:
> >> --- a/arch/arm/mach-davinci/devices-da8xx.c
> >> +++ b/arch/arm/mach-davinci/devices-da8xx.c
> >> @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance,
> >> ? ? ? else
> >> ? ? ? ? ? ? ? return -EINVAL;
> >>
> >> + ? ? /*
> >> + ? ? ?* Both the DA850 and DA830 have an I2C controller which has the
> >> + ? ? ?* ICPFUNC et. al. registers
> >> + ? ? ?*/
> >> + ? ? pdata->has_pfunc = 1;
> >
> > The I2C driver implements a default platform data
> > so it should actually be legal for a DA8x board to
> > pass NULL platform data. In that case this line
> > will crash.
>
> Good catch, thanks.
>
> > [...] You should either check for pdata to
> > be NULL or just let each board choose whether it
> > needs recovery (I think the better option).
>
> I understand "check for pdata to be NULL." If you think it is the
> better option I'd be happy to implement it but I don't understand how
> to implement "let each board choose whether it needs recovery."
This is done by just setting has_pfunc = 1 in the
davinci_i2c_platform_data defined in the board file.
Thanks,
Sekhar
On Wed, Apr 13, 2011 at 11:42 AM, Nori, Sekhar <[email protected]> wrote:
> On Wed, Apr 13, 2011 at 20:52:05, Ben Gardiner wrote:
>> I understand "check for pdata to be NULL." ?If you think it is the
>> better option I'd be happy to implement it but I don't understand how
>> to implement "let each board choose whether it needs recovery."
>
> This is done by just setting has_pfunc = 1 in the
> davinci_i2c_platform_data defined in the board file.
Ok -- now I get it, thanks. I spin a V2 shortly.
Best Regards,
Ben Gardiner
---
Nanometrics Inc.
http://www.nanometrics.ca
Hi Ben,
On Wed, Apr 06, 2011 at 03:08:03, Ben Gardiner wrote:
> This series for the i2c-davinci driver implements both a register dump and
> a pulsed-SCL recovery of the bus on those controllers that have ICPFUNC
> registers which is, so far, the da850 and da830 based platforms.
>
> I2C "controller timed out" messages were being observed by both myself on the
> da850evm and Bastian Ruppert on some custom da850 hardware [1]. Originally I
> thought it was the existing pulse-SCL routine but was quickly proven wrong; my
> apologies to John Povey and Philby John for jumping to conclusions.
>
> A discussion was spawned on the e2e forums [2] where Brad Griffis was
> instrumental in the development of the recovery routine proposed by this patch
> series. It was pointed out by him that the da850's i2c controller has the
> ability to control the SDA and SCL pins as GPIOS through its ICPFUNC registers.
> The recovery routine implemented by the patch series toggles the SCL pin and
> reads the SDA state using the ICPFUNC registers.
[...]
>
> When creating this series I noticed that there are obvious similarities between
> the existing recovery routine implemented by Philby John and John Povey and the
> recovery routine proposed in this series. Testing was performed using the
> ICPFUNC regsiters to toggle SCL as prescribed by the existing routine and it
> was found that the recovery did not work. The method described initially by Brad
> Griffis had the initial state of SCL high and delays of 5us with a maximum
of
> 8 pulses with a check of SDA each time as compared to the existing routine with
> undetermined SCL initial state, 20us delays and a maximum of 8 pulses. I tested
I wonder how these values (5us or 20us) are derived. The document[1]
referred to in the existing code says that "the master should send
9 clock pulses". I think 5us suggested by Brad assumes a 100KHz
bus. If that is so, this number should be derived out of the bus_freq
platform data.
[1] http://www.nxp.com/documents/user_manual/UM10204.pdf
> and found that if I changed the initial state of SCL or the number of pulses
> (Bastian had success with 16) that the recovery did not occur as expected;
Was this testing done with the original PinMuxed GPIO based approach or with
the new internal GPIO based approach?
> furthermore Brad pointed out that it was important to check the state of SDA
> after each pulse to see if the slave had released the line. Indeed, adding the
> check of SDA resulted in a quicker recovery. On my da850evm, at least, the
> recovery took only one SCL pulse every time.
Yes, the document referred to above suggests this too.
I guess the existing recovery mechanism and the new ICPFUNC
based approach shouldn't be functionally different - correct?
In that case, it would really be worthwhile to fix the existing
recovery method as well.
>
> It would be nice to consolidate the two recovery routines and -- since they
> are gpio-based -- put the shared recovery routine in i2c-algo-bit so it can
> also be used by bitbanging i2c masters. I did not undertake the former because
> I don't have access to the hardware on which the existing recovery routine was
> tested.
I think if you see issues with existing code, you should just
go ahead and fix. AFAICT, the existing approach should have
worked for you. You can test on your hardware and those with
other hardware can test your patches as well and send their
acks.
Thanks,
Sekhar
> I did not undertake the latter since 1) it seems premature until the
> recovery routines are consolidated (or not) and 2) it would require more
> changes of a broad scope which I feared might mire the review process of this
> series which is, at its core, a functioning workaround of an observed problem
> with da850evm hardware (plus some regsiter dumps). It is my hope that both the
> consolidation of the recovery routines and the extraction to common bitbanging
> code can be done in a later series.
>
> [1] http://permalink.gmane.org/gmane.linux.davinci/22291
> [2] http://e2e.ti.com/support/dsp/omap_applications_processors/f/42/p/99895/350610.aspx
>
> Ben Gardiner (6):
> i2c-davinci: register dump before attempted bus recovery
> i2c-davinci: convert davinci_i2c_platform_data to kerneldoc
> i2c-davinci: introduce a dev-> function pointer for scl pulsing
> i2c-davinci: use the DA8xx's ICPFUNC to toggle I2C as gpio
> i2c-davinci: if device has pfunc register, dump that group also
> da8xx: enable the use of the ICPFUNC in i2c-davinci
>
> arch/arm/mach-davinci/devices-da8xx.c | 6 +
> arch/arm/mach-davinci/include/mach/i2c.h | 21 +++-
> drivers/i2c/busses/i2c-davinci.c | 155 ++++++++++++++++++++++++++++--
> 3 files changed, 170 insertions(+), 12 deletions(-)
>
>
On Wed, Apr 13, 2011 at 11:10 AM, Nori, Sekhar <[email protected]> wrote:
>
> Hi Ben,
>
> On Wed, Apr 06, 2011 at 03:08:09, Ben Gardiner wrote:
> > Both the da850 and da830 have an I2C controller which has the ICPFUNC
> > registers. Indicate this for all da830 and da850 boards by setting the
> > has_pfunc flag true in the da8xx utility setup routine for registering the
> > I2C controller
> >
> > Signed-off-by: Ben Gardiner <[email protected]>
> > Cc: Sekhar Nori <[email protected]>
> > Cc: Ben Dooks <[email protected]>
> >
> > ---
> > arch/arm/mach-davinci/devices-da8xx.c | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
> > index beda8a4..da01558 100644
> > --- a/arch/arm/mach-davinci/devices-da8xx.c
> > +++ b/arch/arm/mach-davinci/devices-da8xx.c
> > @@ -324,6 +324,12 @@ int __init da8xx_register_i2c(int instance,
> > else
> > return -EINVAL;
> >
> > + /*
> > + * Both the DA850 and DA830 have an I2C controller which has the
> > + * ICPFUNC et. al. registers
> > + */
> > + pdata->has_pfunc = 1;
>
> The I2C driver implements a default platform data
> so it should actually be legal for a DA8x board to
> pass NULL platform data. In that case this line
> will crash. You should either check for pdata to
> be NULL or just let each board choose whether it
> needs recovery (I think the better option).
>
I actually had a problem with using NULL for pdata with davinci I2C
and had submitted a patch here that sort of fell on the floor. The
problem was that the i2c_davinci_calc_clk_dividers is using
platform_data without a check as described above. So at the moment
using NULL doesn't really work, best as I can tell...
https://lkml.org/lkml/2010/9/4/119
FWIW.
-Mike
Hi Mike,
On Thu, Apr 14, 2011 at 04:34:34, Mike Williamson wrote:
> > The I2C driver implements a default platform data
> > so it should actually be legal for a DA8x board to
> > pass NULL platform data. In that case this line
> > will crash. You should either check for pdata to
> > be NULL or just let each board choose whether it
> > needs recovery (I think the better option).
> >
>
> I actually had a problem with using NULL for pdata with davinci I2C
> and had submitted a patch here that sort of fell on the floor. The
> problem was that the i2c_davinci_calc_clk_dividers is using
> platform_data without a check as described above. So at the moment
> using NULL doesn't really work, best as I can tell...
>
> https://lkml.org/lkml/2010/9/4/119
Looking at this mail chain, it looks like Ben was actually OK
with your first submission and just wanted you to update the
patch description saying that dev->platform_data is accessed
later in the code and that is why it needs to be updated.
I liked your first attempt better and may be you should
try another submission with the patch description updated.
Thanks,
Sekhar
Hi Sekhar,
Thank you for reviewing the series.
On Wed, Apr 13, 2011 at 12:25 PM, Nori, Sekhar <[email protected]> wrote:
> Hi Ben,
>
> On Wed, Apr 06, 2011 at 03:08:03, Ben Gardiner wrote:
>> This series for the i2c-davinci driver implements both a register dump and
>> a pulsed-SCL recovery of the bus on those controllers that have ICPFUNC
>> registers which is, so far, the da850 and da830 based platforms.
>>
>> I2C "controller timed out" messages were being observed by both myself on the
>> da850evm and Bastian Ruppert on some custom da850 hardware [1]. Originally I
>> thought it was the existing pulse-SCL routine but was quickly proven wrong; my
>> apologies to John Povey and Philby John for jumping to conclusions.
>>
>> A discussion was spawned on the e2e forums [2] where Brad Griffis was
>> instrumental in the development of the recovery routine proposed by this patch
>> series. It was pointed out by him that the da850's i2c controller has the
>> ability to control the SDA and SCL pins as GPIOS through its ICPFUNC registers.
>> The recovery routine implemented by the patch series toggles the SCL pin and
>> reads the SDA state using the ICPFUNC registers.
>
> [...]
>
>>
>> When creating this series I noticed that there are obvious similarities between
>> the existing recovery routine implemented by Philby John and John Povey and the
>> recovery routine proposed in this series. Testing was performed using the
>> ICPFUNC regsiters to toggle SCL as prescribed by the existing routine and it
>> was found that the recovery did not work. The method described initially by Brad
>> Griffis had the initial state of SCL high and delays of 5us with a maximum
> of
>> 8 pulses with a check of SDA each time as compared to the existing routine with
>> undetermined SCL initial state, 20us delays and a maximum of 8 pulses. I tested
>
> I wonder how these values (5us or 20us) are derived. The document[1]
> referred to in the existing code says that "the master should send
> 9 clock pulses". I think 5us suggested by Brad assumes a 100KHz
> bus. If that is so, this number should be derived out of the bus_freq
> platform data.
>
> [1] http://www.nxp.com/documents/user_manual/UM10204.pdf
I'm not sure how the 20us in the existing method was derived -- I
wonder if Philby John or John Povey could comment? I agree that the
5us suggested by Brad assumes a 100KHz bus -- I could certainly derive
this delay from the bus_freq platform data for the new recovery
procedure since board-da850-evm.c sets bus_freq to 100 kHz.
But I was apprehensive to do so for the existing recovery procedure
since the two (in-tree) boards that are assigning the scl_pin member
are dm355 and dm644x which assign bus_freq values of 400kHz and 20kHz,
respectively. These would require delays of 1.25us and 25us,
respectively; neither of which is the hard-coded 20us. To complicate
matters further, dm644x (20kHz) sets bus_delay to 100us with a comment
that "the msp430 uses a slow bitbanged i2c implementation [...] which
requires 100us of idle after i2c writes [...]." If I derived the
clock-pulse delays from bus_freq the dm644x would probably still work
but if the clock-pulse width was at all important for dm355 then the
recovery procedure could fail there. I don't want to introduce
instabilities into other platforms.
>> and found that if I changed the initial state of SCL or the number of pulses
>> (Bastian had success with 16) that the recovery did not occur as expected;
>
> Was this testing done with the original PinMuxed GPIO based approach or with
> the new internal GPIO based approach?
This testing was performed using the 'new' ICPFUNC-based GPIO toggling.
>> furthermore Brad pointed out that it was important to check the state of SDA
>> after each pulse to see if the slave had released the line. Indeed, adding the
>> check of SDA resulted in a quicker recovery. On my da850evm, at least, the
>> recovery took only one SCL pulse every time.
>
> Yes, the document referred to above suggests this too.
>
> I guess the existing recovery mechanism and the new ICPFUNC
> based approach shouldn't be functionally different - correct?
At the outset I had hoped so too; but what we have in the result is a
method proven to work on the problem hardware here that is now
different than the existing method.
> In that case, it would really be worthwhile to fix the existing
> recovery method as well.
>
>>
>> It would be nice to consolidate the two recovery routines and -- since they
>> are gpio-based -- put the shared recovery routine in i2c-algo-bit so it can
>> also be used by bitbanging i2c masters. I did not undertake the former because
>> I don't have access to the hardware on which the existing recovery routine was
>> tested.
>
> I think if you see issues with existing code, you should just
> go ahead and fix. AFAICT, the existing approach should have
> worked for you. You can test on your hardware and those with
> other hardware can test your patches as well and send their
> acks.
Ok -- I did not want to break behaviour on other hardware with my
changes --but I respect your expert opinion coming from the experience
you have in merging patches as a kernel mach maintainer. I will
re-spin (not immeadiately -- I don't expect to be able to return to
development of this patch series for another couple weeks) another
version, this time replacing the existing recovery procedure with the
tested method here. Also, allowing the individual da8xx boards to
specify whether they enable the recovery procedure as you requested in
reviewing 6/6 of this series. I would still prefer to do the
extraction to i2c-algo-bit in a separate series.
Best Regards,
Ben Gardiner
---
Nanometrics Inc.
http://www.nanometrics.ca
Ben Gardiner wrote:
>>> When creating this series I noticed that there are obvious
>>> similarities between the existing recovery routine implemented by
>>> Philby John and John Povey
> I'm not sure how the 20us in the existing method was derived -- I
> wonder if Philby John or John Povey could comment?
I've been a little bemused about why I am getting credited for I2C
bus recovery work. I don't remember doing any work on that.
I did a couple of patches to fix a race when setting up a TX, but
those are, afaik, unrelated.
All I know about the bus recovery stuff is looking at it a while back
and thinking hmm, that seems to wiggle gpio without changing the
pinmuxing, so it can't possibly work.
--
Jon Povey
[email protected]
Racelogic is a limited company registered in England. Registered number 2743719 .
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB .
The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network
Hi Jon,
On Mon, Apr 18, 2011 at 9:47 PM, Jon Povey <[email protected]> wrote:
> Ben Gardiner wrote:
>
>>>> When creating this series I noticed that there are obvious
>>>> similarities between the existing recovery routine implemented by
>>>> Philby John and John Povey
>
>> I'm not sure how the 20us in the existing method was derived -- I
>> wonder if Philby John or John Povey could comment?
>
> I've been a little bemused about why I am getting credited for I2C
> bus recovery work. I don't remember doing any work on that.
My mistake -- sorry. Also sorry for getting your name wrong repeatedly.
> I did a couple of patches to fix a race when setting up a TX, but
> those are, afaik, unrelated.
In "i2c-davinci: Fix race when setting up for TX" and "i2c-davinci:
Fix TX setup for more SoCs" you mention testing on DM355 -- is there
any you have hardware on which the recovery procedure is executed on
occasion and that you would be available to test modifications to the
current implementation?
> All I know about the bus recovery stuff is looking at it a while back
> and thinking hmm, that seems to wiggle gpio without changing the
> pinmuxing, so it can't possibly work.
:) Probably not then.
Best Regards,
Ben Gardiner
---
Nanometrics Inc.
http://www.nanometrics.ca
Ben Gardiner wrote:
> In "i2c-davinci: Fix race when setting up for TX" and "i2c-davinci:
> Fix TX setup for more SoCs" you mention testing on DM355 -- is there
> any you have hardware on which the recovery procedure is executed on
> occasion and that you would be available to test modifications to the
> current implementation?
Nope, sorry. I've never seen the bus lock up.
--
Jon Povey
[email protected]
Racelogic is a limited company registered in England. Registered number 2743719 .
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB .
The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network