Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Hello Pablo,
On Mon, 2010-09-13 at 16:23 +0200, Pablo Bitton wrote:
Hi Philby,
i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
char allow_sleep)
{
unsigned long timeout;
+ static u16 to_cnt;
timeout = jiffies + dev-adapter.timeout;
while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
DAVINCI_I2C_STR_BB) {
- if (time_after(jiffies, timeout)) {
- dev_warn(dev-dev,
-timeout waiting for bus
ready\n);
- return -ETIMEDOUT;
+ if (to_cnt = DAVINCI_I2C_MAX_TRIES) {
+ if (time_after(jiffies, timeout)) {
+ dev_warn(dev-dev,
+ timeout waiting for bus ready
\n);
+ to_cnt++;
+ return -ETIMEDOUT;
+ } else {
+ to_cnt = 0;
+ i2c_recover_bus(dev);
+ i2c_davinci_init(dev);
+ }
}
if (allow_sleep)
schedule_timeout(1);
The resulting loop has the following drawbacks:
1) If to_cnt reaches DAVINCI_I2C_MAX_TRIES+1 (which it currently
can't, see 2) and the i2c bus collapses,
the kernel will be stuck in the loop forever, especially if
allow_sleep is false.
I do not understand how to_cnt can reach DAVINCI_I2C_MAX_TRIES+1. You
seem to be contradicting your own statement, you also say that this
cannot happen!!
2) The to_cnt static var never increments beyond 1.
Precisely.
It's initialized to zero and then kept at zero until the timeout
expires.
When the timeout expires, to_cnt is incremented once, until the
next call to the function, where it will be zeroed again.
How then can your 1st assumption hold good?
3) Do we really want to retry recovering the bus thousands of times,
until the timeout arrives?
It seems to me that if the bus recovery procedure didn't help
after one or two tries, it probably never will.
Once a bus recovery is initiated, we are in the last phase of a recovery
and if that fails the user is left with no other choice but to reset the
target. From the very beginning the idea was to try until timeout.
Wouldn't you have a working system rather than to have to reset the
target?
I also have the following nitpicks:
a) The timeout variable actually holds the finish time.
Well, that's just aesthetic makeover. I could say the timeout is 10
seconds and the finish time is 10 seconds, it sounds the same to me.
b) The i2c_recover_bus function uses dev_err to report a bus recovery
process,
but if all recovery attempts failed and the timeout was reached,
only dev_warn is used to report the timeout.
Agreed. But your patch does not reflect this change.
Other than that the patch is very helpful, thanks a lot.
Below is my suggestion for the wait_bus_not_busy function. My patch
has been tested on a DM6446.
All in all, your patch gives multiple checkpatch errors/warnings (spaces
instead of tabs etc). You have missed out parts of the code present in
the pristine kernel and so will not cleanly apply. To me, there are two
things that are of interest in your patch. First is you got rid of the
static variable definition and the other is usage of dev_err. I fail to
understand your assumption that the kernel will be stuck in the loop
forever, hence I cannot tell how useful this patch really is.
Signed-off-by: Pablo Bitton pablo.bit...@gmail.com
--
diff --git a/drivers/i2c/busses/i2c-davinci.c
b/drivers/i2c/busses/i2c-davinci.c
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -220,16 +261,24 @@ static int i2c_davinci_init(struct
davinci_i2c_dev *dev)
static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
char allow_sleep)
{
- unsigned long timeout;
+ unsigned long finish_time;
You have missed out on this line static u16 to_cnt;
+ unsigned long to_cnt = 0;
- timeout = jiffies + dev-adapter.timeout;
+ finish_time = jiffies + dev-adapter.timeout;
+ /* While bus busy */
while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
DAVINCI_I2C_STR_BB) {
Your patch misses out on this line.
if (to_cnt = DAVINCI_I2C_MAX_TRIES) {
Shouldn't you show removing it in the patch?
- if (time_after(jiffies, timeout)) {
+ if (time_after(jiffies, finish_time)) {
dev_warn(dev-dev,
timeout waiting
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Hi Philby,
On Wed, Jan 27, 2010 at 1:41 AM, Kevin Hilman
khil...@deeprootsystems.comwrote:
From: Philby John pj...@in.mvista.com
Come out of i2c time out condition by following the
bus recovery procedure outlined in the i2c protocol v3 spec.
The kernel must be robust enough to gracefully recover
from i2c bus failure without having to reset the machine.
This is done by first NACKing the slave, pulsing the SCL
line 9 times and then sending the stop command.
This patch has been tested on a DM6446 and DM355
Signed-off-by: Philby John pj...@in.mvista.com
Signed-off-by: Srinivasan, Nageswari nagesw...@ti.com
Acked-by: Kevin Hilman khil...@deeprootsystems.com
---
drivers/i2c/busses/i2c-davinci.c | 57
+++--
1 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c
b/drivers/i2c/busses/i2c-davinci.c
index 35f9daa..5459065 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include linux/platform_device.h
#include linux/io.h
#include linux/cpufreq.h
+#include linux/gpio.h
#include mach/hardware.h
#include mach/i2c.h
@@ -43,6 +44,7 @@
/* - global defines --- */
#define DAVINCI_I2C_TIMEOUT(1*HZ)
+#define DAVINCI_I2C_MAX_TRIES 2
#define I2C_DAVINCI_INTR_ALL(DAVINCI_I2C_IMR_AAS | \
DAVINCI_I2C_IMR_SCD | \
DAVINCI_I2C_IMR_ARDY | \
@@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct
davinci_i2c_dev *i2c_dev, int reg)
return __raw_readw(i2c_dev-base + reg);
}
+/* Generate a pulse on the i2c clock pin. */
+static void generic_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 i2c_recover_bus(struct davinci_i2c_dev *dev)
+{
+ u32 flag = 0;
+ struct davinci_i2c_platform_data *pdata = dev-dev-platform_data;
+
+ 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);
+ if (pdata)
+ generic_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)
{
@@ -235,14 +275,22 @@ static int i2c_davinci_wait_bus_not_busy(struct
davinci_i2c_dev *dev,
char allow_sleep)
{
unsigned long timeout;
+ static u16 to_cnt;
timeout = jiffies + dev-adapter.timeout;
while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
DAVINCI_I2C_STR_BB) {
- if (time_after(jiffies, timeout)) {
- dev_warn(dev-dev,
-timeout waiting for bus ready\n);
- return -ETIMEDOUT;
+ if (to_cnt = DAVINCI_I2C_MAX_TRIES) {
+ if (time_after(jiffies, timeout)) {
+ dev_warn(dev-dev,
+ timeout waiting for bus ready\n);
+ to_cnt++;
+ return -ETIMEDOUT;
+ } else {
+ to_cnt = 0;
+ i2c_recover_bus(dev);
+ i2c_davinci_init(dev);
+ }
}
if (allow_sleep)
schedule_timeout(1);
The resulting loop has the following drawbacks:
1) If to_cnt reaches DAVINCI_I2C_MAX_TRIES+1 (which it currently can't, see
2) and the i2c bus collapses,
the kernel will be stuck in the loop forever, especially if allow_sleep
is false.
2) The to_cnt static var never increments beyond 1. It's initialized to zero
and then kept at zero until the timeout expires.
When the timeout expires, to_cnt is incremented once, until the next
call to the function, where it will be zeroed again.
3) Do we really want to retry recovering the bus thousands of times, until
the timeout arrives?
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
On 03/17/2010 02:20 AM, Kevin Hilman wrote:
Philby Johnpj...@mvista.com writes:
On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
Hi Philby,
On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
Hello Sekhar,
[...]
+/* Generate a pulse on the i2c clock pin. */
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
I now think that the previous versions were incorrect since
davinci_cfg_reg() does not set the scl or sda pins for gpio
functionality. Instead they set them as scl or sda which is not what
we want at the time of pulsing. The previous versions used
gpio_set_value() in disable_i2c_pins() and then called
davinci_cfg_reg(). After which it called pulse_i2c_clock().
Please correct me if my interpretation of the code is incorrect.
Can we get some resolution here?
I have a queue of davinci i2c patches waiting to go upstream, and I'd
like to get them in for 2.6.35.
The current i2c series is in the 'davinci-i2c' branch of davinci git.
To quote Sekhar, ...Right. It is only an enhancement (and only good
to have at that). This should not stop the current patch from getting
in. So this patch is good to make it to 2.6.35
Please submit an updated version so we can get this stuff upstream.
Right, the enhancement has now been sent.
Regards,
Philby
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Hi Philby,
On Wed, Mar 17, 2010 at 16:58:44, Philby John wrote:
On 03/17/2010 02:20 AM, Kevin Hilman wrote:
Philby Johnpj...@mvista.com writes:
On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
Hi Philby,
On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
Hello Sekhar,
[...]
+/* Generate a pulse on the i2c clock pin. */
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
I now think that the previous versions were incorrect since
davinci_cfg_reg() does not set the scl or sda pins for gpio
functionality. Instead they set them as scl or sda which is not what
we want at the time of pulsing. The previous versions used
gpio_set_value() in disable_i2c_pins() and then called
davinci_cfg_reg(). After which it called pulse_i2c_clock().
Please correct me if my interpretation of the code is incorrect.
Can we get some resolution here?
I have a queue of davinci i2c patches waiting to go upstream, and I'd
like to get them in for 2.6.35.
The current i2c series is in the 'davinci-i2c' branch of davinci git.
To quote Sekhar, ...Right. It is only an enhancement (and only good
to have at that). This should not stop the current patch from getting
in. So this patch is good to make it to 2.6.35
How about the using the free data format method that Brad was
suggesting [1]? That sounds like a much simpler approach than the
GPIO muxing method that this patch is adopting.
I think we should try that method before accepting this approach
as the final one.
Thanks,
Sekhar
[1] http://www.mail-archive.com/linux-...@vger.kernel.org/msg02800.html
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
On 03/17/2010 06:48 PM, Nori, Sekhar wrote:
Hi Philby,
On Wed, Mar 17, 2010 at 16:58:44, Philby John wrote:
On 03/17/2010 02:20 AM, Kevin Hilman wrote:
Philby Johnpj...@mvista.com writes:
On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
Hi Philby,
On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
Hello Sekhar,
[...]
+/* Generate a pulse on the i2c clock pin. */
+static void generic_i2c_clock_pulse(unsigned int scl_pin)
+{
+ u16 i;
+
+ if (scl_pin) {
+ /* Send high and low on the SCL line */
+ for (i = 0; i9; i++) {
+ gpio_set_value(scl_pin, 0);
+ udelay(20);
+ gpio_set_value(scl_pin, 1);
+ udelay(20);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
I now think that the previous versions were incorrect since
davinci_cfg_reg() does not set the scl or sda pins for gpio
functionality. Instead they set them as scl or sda which is not what
we want at the time of pulsing. The previous versions used
gpio_set_value() in disable_i2c_pins() and then called
davinci_cfg_reg(). After which it called pulse_i2c_clock().
Please correct me if my interpretation of the code is incorrect.
Can we get some resolution here?
I have a queue of davinci i2c patches waiting to go upstream, and I'd
like to get them in for 2.6.35.
The current i2c series is in the 'davinci-i2c' branch of davinci git.
To quote Sekhar, ...Right. It is only an enhancement (and only good
to have at that). This should not stop the current patch from getting
in. So this patch is good to make it to 2.6.35
How about the using the free data format method that Brad was
suggesting [1]? That sounds like a much simpler approach than the
GPIO muxing method that this patch is adopting.
I can't seem to agree with your argument. The recovery method adopted
here is part of the standard i2c v3 spec.
I think we should try that method before accepting this approach
as the final one.
It would be wiser allowing this one to go in, since a lot of
reviewing/testing has already been done. Later on we could try the free
data format and hope that this would work. Otherwise, we would still be
without a fix until free data format is done.
Regards,
Philby
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Philby John pj...@mvista.com writes:
On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
Hi Philby,
On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
Hello Sekhar,
[...]
+/* Generate a pulse on the i2c clock pin. */
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
I now think that the previous versions were incorrect since
davinci_cfg_reg() does not set the scl or sda pins for gpio
functionality. Instead they set them as scl or sda which is not what
we want at the time of pulsing. The previous versions used
gpio_set_value() in disable_i2c_pins() and then called
davinci_cfg_reg(). After which it called pulse_i2c_clock().
Please correct me if my interpretation of the code is incorrect.
Can we get some resolution here?
I have a queue of davinci i2c patches waiting to go upstream, and I'd
like to get them in for 2.6.35.
The current i2c series is in the 'davinci-i2c' branch of davinci git.
Please submit an updated version so we can get this stuff upstream.
Kevin
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
-Original Message- From: Philby John [mailto:pj...@mvista.com] Sent: Monday, March 08, 2010 7:37 AM To: Griffis, Brad Cc: Nori, Sekhar; davinci-linux-open-source@linux.davincidsp.com; linux- i...@vger.kernel.org Subject: Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus I did go through your document, but what does free data format mean? It would be good to expand the procedure that enables you to move into this mode. If this wouldn't require modfying pinmux settings shouldn't it be part of the core i2c implementation? Just to make sure my point was clear, I think the GPIO method is simpler/easier if you're just looking at a single device and assuming that device has muxed the I2C with GPIO. That said, my method is a little more complicated/convoluted when looking at a single device, but I think the code would scale better across the entire Davinci tree since it requires no knowledge of whether the pin-muxing option is available and how the pin-muxing is implemented for that particular device. You enter the free data format mode by setting the FDF bit in the ICMDR register. It is described in Section 2.6.3 of the OMAP-L138 I2C Guide: http://www.ti.com/lit/sprufl9 The advantage of using this mode is that you would not require any device-specific pin-muxing knowledge/changes. The entire recovery can occur within the context of the I2C controller. So to do a read in the free data format mode you would write ICMDR.FDF = 1 (free data format), ICMDR.TRX = 0 (read), ICMDR.STT = 1 (start), ICMDR.STP = 1. This should cause it to clock in 8 bits of data plus an ack, freeing up the bus. Brad ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
On 03/05/2010 08:50 PM, Griffis, Brad wrote: Right. I was also hoping to rid of cpu_is_xxx usage. The only other way I could think of is to add pinmux index into i2c platform data struct. What do you think is the best approach? I think passing pinmux index through platform data is fair. Thanks, Sekhar I recently was told of a clever solution for this issue which I documented here: http://wiki.davincidsp.com/index.php/I2C_Tips#External_Slave_Device_Hanging_the_Bus_by_Holding_SDA_Low Basically the solution was to switch to free data format and perform a read. This will cause the I2C to start toggling SCL. I mention it here because I think the free data format mode is available on most processors. (The only device I know that does NOT support free data format is OMAP35x.) You might have a lot less processor-specific code with this approach and it would be applicable to more devices. I don't have any time to write the code/patch myself, but I thought I would at least toss the idea out there. Brad I did go through your document, but what does free data format mean? It would be good to expand the procedure that enables you to move into this mode. If this wouldn't require modfying pinmux settings shouldn't it be part of the core i2c implementation? At present we follow the i2c spec. recovery procedure which you explained in method 1, and as per AN10216-01 I2C Manual is ... •SDA line is then non usable anymore because of the “Slave-Transmitter”mode. •Methods to recover the SDA line are: –Reset the slave device (assuming the device has a Reset pin) –Use a bus recovery sequence to leave the “Slave-Transmitter” mode •Bus recovery sequence is done as following: 1-Send 9 clock pulses on SCL line 2-Ask the master to keep SDA High until the “Slave-Transmitter” releases the SDA line to perform the ACK operation 3-Keeping SDA High during the ACK means that the “Master-Receiver”does not acknowledge the previous byte receive 4-The “Slave-Transmitter” then goes in an idle state 5-The master then sends a STOP command initializing completely the bus Regards, Philby ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Right. I was also hoping to rid of cpu_is_xxx usage. The only other way I could think of is to add pinmux index into i2c platform data struct. What do you think is the best approach? I think passing pinmux index through platform data is fair. Thanks, Sekhar I recently was told of a clever solution for this issue which I documented here: http://wiki.davincidsp.com/index.php/I2C_Tips#External_Slave_Device_Hanging_the_Bus_by_Holding_SDA_Low Basically the solution was to switch to free data format and perform a read. This will cause the I2C to start toggling SCL. I mention it here because I think the free data format mode is available on most processors. (The only device I know that does NOT support free data format is OMAP35x.) You might have a lot less processor-specific code with this approach and it would be applicable to more devices. I don't have any time to write the code/patch myself, but I thought I would at least toss the idea out there. Brad ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
On 02/08/2010 09:33 PM, Nori, Sekhar wrote:
On Mon, Feb 08, 2010 at 20:43:27, Philby John wrote:
Hello Sekhar,
On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
Don't seem to remember having the code in the old versions at least
not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
enable_i2c_pins() were discarded as the i2c protocol spec. did not
specify the need. Moreover bus recovered without it. (Tested on DM355
and Dm6446).
Yes, I was referring to the davinci_cfg_reg() calls in
{disable|enable}_i2c_pins() functions. Per the specification
of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
it is to be used as GPIO controlled by GPIO module. It may
have worked on couple of devices but cannot be guaranteed to
work on all DaVinci devices (esp. DA8XX ones).
I think that using davinci_cfg_reg() in generic_i2c_clock_pulse() is the
wrong place to put it. This would require adding davinci_cfg_reg() for
all know davinci platforms. The i2c recovery procedure is correct to
assume that it owns the SCL line at that very moment.
Instead I believe pinmuxing using davinci_cfg_reg(), should be done way
early, just like we do for DM6446 in devices.c -- davinci_init_i2c(),
for all other platforms. What I could do in function
generic_i2c_clock_pulse() is, set SCL to output, and use gpio_request()
by checking REVID2 register value (0x6) for DA8xx and 0x5 for others.
But, the pins should remain as I2C pins till you actually
hit a bus lock-up. That's when you need to convert them to GPIO
pins and start the recovery by pulsing SCL.
It you make them GPIO right at the start, they wont be usable
as I2C pins for normal transfers?
Right. I was also hoping to rid of cpu_is_xxx usage. The only other way
I could think of is to add pinmux index into i2c platform data struct.
What do you think is the best approach?
Regards,
Philby
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
On Tue, Feb 09, 2010 at 15:45:15, Philby John wrote:
On 02/08/2010 09:33 PM, Nori, Sekhar wrote:
On Mon, Feb 08, 2010 at 20:43:27, Philby John wrote:
Hello Sekhar,
On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
Don't seem to remember having the code in the old versions at least
not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
enable_i2c_pins() were discarded as the i2c protocol spec. did not
specify the need. Moreover bus recovered without it. (Tested on DM355
and Dm6446).
Yes, I was referring to the davinci_cfg_reg() calls in
{disable|enable}_i2c_pins() functions. Per the specification
of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
it is to be used as GPIO controlled by GPIO module. It may
have worked on couple of devices but cannot be guaranteed to
work on all DaVinci devices (esp. DA8XX ones).
I think that using davinci_cfg_reg() in generic_i2c_clock_pulse() is the
wrong place to put it. This would require adding davinci_cfg_reg() for
all know davinci platforms. The i2c recovery procedure is correct to
assume that it owns the SCL line at that very moment.
Instead I believe pinmuxing using davinci_cfg_reg(), should be done way
early, just like we do for DM6446 in devices.c -- davinci_init_i2c(),
for all other platforms. What I could do in function
generic_i2c_clock_pulse() is, set SCL to output, and use gpio_request()
by checking REVID2 register value (0x6) for DA8xx and 0x5 for others.
But, the pins should remain as I2C pins till you actually
hit a bus lock-up. That's when you need to convert them to GPIO
pins and start the recovery by pulsing SCL.
It you make them GPIO right at the start, they wont be usable
as I2C pins for normal transfers?
Right. I was also hoping to rid of cpu_is_xxx usage. The only other way
I could think of is to add pinmux index into i2c platform data struct.
What do you think is the best approach?
I think passing pinmux index through platform data is fair.
Thanks,
Sekhar
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Hi Philby,
On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
Hello Sekhar,
[...]
+/* Generate a pulse on the i2c clock pin. */
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
Don't seem to remember having the code in the old versions at least
not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
enable_i2c_pins() were discarded as the i2c protocol spec. did not
specify the need. Moreover bus recovered without it. (Tested on DM355
and Dm6446).
Yes, I was referring to the davinci_cfg_reg() calls in
{disable|enable}_i2c_pins() functions. Per the specification
of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
it is to be used as GPIO controlled by GPIO module. It may
have worked on couple of devices but cannot be guaranteed to
work on all DaVinci devices (esp. DA8XX ones).
Couple of good to haves:
[...]
The I2C peripheral on da8xx itself contains a mode where its
pins could be used as GPIO - so no need for SoC level muxing
and need for the platform data. This seems to be missing from
DM355 though. Thankfully there is a revision id within the I2C
memory map which will help you differentiate the two cases
(revision 0x5 vs 0x6)
I did not entirely follow your above statement. Will usage of
gpio_request() solve the problem for da8xx and DM355 or does it
require a if else condition?
You require special casing for I2C version 0x6.
Have a look here: http://focus.ti.com/lit/ug/sprufl9a/sprufl9a.pdf
I was referring to registers described in sections
3.15-3.20
A reminder that the present code will
only work for DM6446 and DM355. I do not have a DA8xx to test specific
functionality if it were to be added.
Right. It is only an enhancement (and only good
to have at that). This should not stop the current
patch from getting in.
Thanks,
Sekhar
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Hello Sekhar,
On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
Don't seem to remember having the code in the old versions at least
not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
enable_i2c_pins() were discarded as the i2c protocol spec. did not
specify the need. Moreover bus recovered without it. (Tested on DM355
and Dm6446).
Yes, I was referring to the davinci_cfg_reg() calls in
{disable|enable}_i2c_pins() functions. Per the specification
of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
it is to be used as GPIO controlled by GPIO module. It may
have worked on couple of devices but cannot be guaranteed to
work on all DaVinci devices (esp. DA8XX ones).
I think that using davinci_cfg_reg() in generic_i2c_clock_pulse() is the
wrong place to put it. This would require adding davinci_cfg_reg() for
all know davinci platforms. The i2c recovery procedure is correct to
assume that it owns the SCL line at that very moment.
Instead I believe pinmuxing using davinci_cfg_reg(), should be done way
early, just like we do for DM6446 in devices.c -- davinci_init_i2c(),
for all other platforms. What I could do in function
generic_i2c_clock_pulse() is, set SCL to output, and use gpio_request()
by checking REVID2 register value (0x6) for DA8xx and 0x5 for others.
Let me know what you think.
Regards,
Philby
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
On Mon, Feb 08, 2010 at 20:43:27, Philby John wrote:
Hello Sekhar,
On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
Don't seem to remember having the code in the old versions at least
not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
enable_i2c_pins() were discarded as the i2c protocol spec. did not
specify the need. Moreover bus recovered without it. (Tested on DM355
and Dm6446).
Yes, I was referring to the davinci_cfg_reg() calls in
{disable|enable}_i2c_pins() functions. Per the specification
of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
it is to be used as GPIO controlled by GPIO module. It may
have worked on couple of devices but cannot be guaranteed to
work on all DaVinci devices (esp. DA8XX ones).
I think that using davinci_cfg_reg() in generic_i2c_clock_pulse() is the
wrong place to put it. This would require adding davinci_cfg_reg() for
all know davinci platforms. The i2c recovery procedure is correct to
assume that it owns the SCL line at that very moment.
Instead I believe pinmuxing using davinci_cfg_reg(), should be done way
early, just like we do for DM6446 in devices.c -- davinci_init_i2c(),
for all other platforms. What I could do in function
generic_i2c_clock_pulse() is, set SCL to output, and use gpio_request()
by checking REVID2 register value (0x6) for DA8xx and 0x5 for others.
But, the pins should remain as I2C pins till you actually
hit a bus lock-up. That's when you need to convert them to GPIO
pins and start the recovery by pulsing SCL.
It you make them GPIO right at the start, they wont be usable
as I2C pins for normal transfers?
Thanks,
Sekhar
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Hello Sekhar,
My apologies for the late mail. Had trouble with our mail server and I
seem to have lost mails. Pulling this thread from the list. Comments
inline...
On Mon, Feb 1, 2010 at 11:27 AM, Nori, Sekhar nsek...@ti.com wrote:
Hi Philby,
On Wed, Jan 27, 2010 at 05:11:33, Kevin Hilman wrote:
From: Philby John pj...@in.mvista.com
Come out of i2c time out condition by following the
bus recovery procedure outlined in the i2c protocol v3 spec.
The kernel must be robust enough to gracefully recover
from i2c bus failure without having to reset the machine.
This is done by first NACKing the slave, pulsing the SCL
line 9 times and then sending the stop command.
This patch has been tested on a DM6446 and DM355
Signed-off-by: Philby John pj...@in.mvista.com
Signed-off-by: Srinivasan, Nageswari nagesw...@ti.com
Acked-by: Kevin Hilman khil...@deeprootsystems.com
---
drivers/i2c/busses/i2c-davinci.c | 57
+++--
1 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c
b/drivers/i2c/busses/i2c-davinci.c
index 35f9daa..5459065 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include linux/platform_device.h
#include linux/io.h
#include linux/cpufreq.h
+#include linux/gpio.h
#include mach/hardware.h
#include mach/i2c.h
@@ -43,6 +44,7 @@
/* - global defines --- */
#define DAVINCI_I2C_TIMEOUT (1*HZ)
+#define DAVINCI_I2C_MAX_TRIES 2
#define I2C_DAVINCI_INTR_ALL (DAVINCI_I2C_IMR_AAS | \
DAVINCI_I2C_IMR_SCD | \
DAVINCI_I2C_IMR_ARDY | \
@@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct
davinci_i2c_dev *i2c_dev, int reg)
return __raw_readw(i2c_dev-base + reg);
}
+/* Generate a pulse on the i2c clock pin. */
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
Don't seem to remember having the code in the old versions at least
not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
enable_i2c_pins() were discarded as the i2c protocol spec. did not
specify the need. Moreover bus recovered without it. (Tested on DM355
and Dm6446).
Couple of good to haves:
It will be good to do a gpio_request() before using the pins
as GPIO - though I can see it may have been deemed unnecessary
- the pins are owned by I2C already - even so it may help catch
system configuration errors in later platforms.
Yes, I could use gpio_request() in generic_i2c_clock_pulse().
The I2C peripheral on da8xx itself contains a mode where its
pins could be used as GPIO - so no need for SoC level muxing
and need for the platform data. This seems to be missing from
DM355 though. Thankfully there is a revision id within the I2C
memory map which will help you differentiate the two cases
(revision 0x5 vs 0x6)
I did not entirely follow your above statement. Will usage of
gpio_request() solve the problem for da8xx and DM355 or does it
require a if else condition? A reminder that the present code will
only work for DM6446 and DM355. I do not have a DA8xx to test specific
functionality if it were to be added.
Regards,
Philby
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Nori, Sekhar nsek...@ti.com writes:
Hi Philby,
On Wed, Jan 27, 2010 at 05:11:33, Kevin Hilman wrote:
From: Philby John pj...@in.mvista.com
Come out of i2c time out condition by following the
bus recovery procedure outlined in the i2c protocol v3 spec.
The kernel must be robust enough to gracefully recover
from i2c bus failure without having to reset the machine.
This is done by first NACKing the slave, pulsing the SCL
line 9 times and then sending the stop command.
This patch has been tested on a DM6446 and DM355
Signed-off-by: Philby John pj...@in.mvista.com
Signed-off-by: Srinivasan, Nageswari nagesw...@ti.com
Acked-by: Kevin Hilman khil...@deeprootsystems.com
---
drivers/i2c/busses/i2c-davinci.c | 57
+++--
1 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c
b/drivers/i2c/busses/i2c-davinci.c
index 35f9daa..5459065 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include linux/platform_device.h
#include linux/io.h
#include linux/cpufreq.h
+#include linux/gpio.h
#include mach/hardware.h
#include mach/i2c.h
@@ -43,6 +44,7 @@
/* - global defines --- */
#define DAVINCI_I2C_TIMEOUT (1*HZ)
+#define DAVINCI_I2C_MAX_TRIES2
#define I2C_DAVINCI_INTR_ALL(DAVINCI_I2C_IMR_AAS | \
DAVINCI_I2C_IMR_SCD | \
DAVINCI_I2C_IMR_ARDY | \
@@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct
davinci_i2c_dev *i2c_dev, int reg)
return __raw_readw(i2c_dev-base + reg);
}
+/* Generate a pulse on the i2c clock pin. */
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
Couple of good to haves:
It will be good to do a gpio_request() before using the pins
as GPIO - though I can see it may have been deemed unnecessary
- the pins are owned by I2C already - even so it may help catch
system configuration errors in later platforms.
The I2C peripheral on da8xx itself contains a mode where its
pins could be used as GPIO - so no need for SoC level muxing
and need for the platform data. This seems to be missing from
DM355 though. Thankfully there is a revision id within the I2C
memory map which will help you differentiate the two cases
(revision 0x5 vs 0x6)
Philby,
Please update with comments from Sekhar soon so I can get this one queued
for 2.6.34.
Kevin
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus
Hi Philby,
On Wed, Jan 27, 2010 at 05:11:33, Kevin Hilman wrote:
From: Philby John pj...@in.mvista.com
Come out of i2c time out condition by following the
bus recovery procedure outlined in the i2c protocol v3 spec.
The kernel must be robust enough to gracefully recover
from i2c bus failure without having to reset the machine.
This is done by first NACKing the slave, pulsing the SCL
line 9 times and then sending the stop command.
This patch has been tested on a DM6446 and DM355
Signed-off-by: Philby John pj...@in.mvista.com
Signed-off-by: Srinivasan, Nageswari nagesw...@ti.com
Acked-by: Kevin Hilman khil...@deeprootsystems.com
---
drivers/i2c/busses/i2c-davinci.c | 57 +++--
1 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c
b/drivers/i2c/busses/i2c-davinci.c
index 35f9daa..5459065 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include linux/platform_device.h
#include linux/io.h
#include linux/cpufreq.h
+#include linux/gpio.h
#include mach/hardware.h
#include mach/i2c.h
@@ -43,6 +44,7 @@
/* - global defines --- */
#define DAVINCI_I2C_TIMEOUT (1*HZ)
+#define DAVINCI_I2C_MAX_TRIES2
#define I2C_DAVINCI_INTR_ALL(DAVINCI_I2C_IMR_AAS | \
DAVINCI_I2C_IMR_SCD | \
DAVINCI_I2C_IMR_ARDY | \
@@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct
davinci_i2c_dev *i2c_dev, int reg)
return __raw_readw(i2c_dev-base + reg);
}
+/* Generate a pulse on the i2c clock pin. */
+static void generic_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);
+ }
Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.
Couple of good to haves:
It will be good to do a gpio_request() before using the pins
as GPIO - though I can see it may have been deemed unnecessary
- the pins are owned by I2C already - even so it may help catch
system configuration errors in later platforms.
The I2C peripheral on da8xx itself contains a mode where its
pins could be used as GPIO - so no need for SoC level muxing
and need for the platform data. This seems to be missing from
DM355 though. Thankfully there is a revision id within the I2C
memory map which will help you differentiate the two cases
(revision 0x5 vs 0x6)
Thanks,
Sekhar
+ }
+}
+
+/* This routine does i2c bus recovery as specified in the
+ * i2c protocol Rev. 03 section 3.16 titled Bus clear
+ */
+static void i2c_recover_bus(struct davinci_i2c_dev *dev)
+{
+ u32 flag = 0;
+ struct davinci_i2c_platform_data *pdata = dev-dev-platform_data;
+
+ 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);
+ if (pdata)
+ generic_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)
{
@@ -235,14 +275,22 @@ static int i2c_davinci_wait_bus_not_busy(struct
davinci_i2c_dev *dev,
char allow_sleep)
{
unsigned long timeout;
+ static u16 to_cnt;
timeout = jiffies + dev-adapter.timeout;
while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
DAVINCI_I2C_STR_BB) {
- if (time_after(jiffies, timeout)) {
- dev_warn(dev-dev,
- timeout waiting for bus ready\n);
- return -ETIMEDOUT;
+ if (to_cnt = DAVINCI_I2C_MAX_TRIES) {
+ if (time_after(jiffies, timeout)) {
+ dev_warn(dev-dev,
+ timeout waiting for bus ready\n);
+ to_cnt++;
+ return -ETIMEDOUT;
+ } else {
+ to_cnt = 0;
+ i2c_recover_bus(dev);
+ i2c_davinci_init(dev);
+ }
