Re: [PATCH] usb: musb: use put_sync_suspend instead of put_sync
Felipe, On Fri, Aug 12, 2011 at 9:40 AM, Pandita, Vikram vikram.pand...@ti.com wrote: On Fri, Aug 12, 2011 at 6:55 AM, Pandita, Vikram vikram.pand...@ti.com wrote: On Fri, Aug 12, 2011 at 2:01 AM, Felipe Balbi ba...@ti.com wrote: Hi, (sorry for top-posting) Moiz, are we going to have another set of this ? The problem was with pm_runtime() functions getting called from atomic context and we did a work around by calling the runtime functions from a work queue. That seems to fix the issue. We will post that out as an alternative to marking the irq_safe and thus causing the parent to never idle. Here is the new post: https://patchwork.kernel.org/patch/1061282/ The patch was posted because of the runtime calls inside musb_gadget_pullup() within interrupt-disable context. This was actually fixed by syncing with a patch in open-source whering actually these calls were done outside the irq_save() calls. https://lkml.org/lkml/2011/7/20/357 Similar problem was also for the run-time pm calls done from the musb_otg_notifications() which is in atomic context. This has been fixed by Vikrams patch. [.] -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Regards Moiz Sonasath Android Kernel Team -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: OTG: Use work_queue in set_vbus for TWL6030 transciever
Todd, On Sat, Jun 25, 2011 at 1:31 AM, Todd Poynor toddpoy...@google.com wrote: On Fri, Jun 24, 2011 at 10:14:03AM -0500, Moiz Sonasath wrote: ... + if (enabled) + twl-vbus_enable = 1; + else + twl-vbus_enable = 0; + Suggest twl-vbus_enable = enabled; /* * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1 * register. This enables boost mode. */ - if (enabled) - twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x40, - CHARGERUSB_CTRL1); - else - twl6030_writeb(twl, TWL_MODULE_MAIN_CHARGE , 0x00, - CHARGERUSB_CTRL1); + + schedule_work(twl-set_vbus_work); + Suggest also moving the comments together with the new location of the code to write CHARGERUSB_CTRL1. Add a cancel_work_sync(twl-set_vbus_work) at twl6030_usb_remove(), prior to kfree(twl). Thank you for the feedback, have posted a V2 of the patch. -- Regards Moiz Sonasath Android Kernel Team -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630
Tony,
-Original Message-
From: Sonasath, Moiz
Sent: Friday, February 26, 2010 1:15 PM
To: linux-omap@vger.kernel.org
Cc: t...@atomide.com; Sonasath, Moiz; Pais, Allen; Pandita, Vikram
Subject: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630
From: Moiz Sonasath m-sonas...@ti.com
This patch disables the newly introduced internal pull-up feature in
OMAP3630, to use only the external HW resitor = 470 Ohm for the
assured functionality in HS mode.
While testing the I2C in High Speed mode, it was discovered that
without a proper pull-up resitor, there is data corruption during
multi-byte transfers. RTC(time_set) test case was used for testing.
From the analysis done, it was concluded that ideally we need a pull-up
of 1.6 K Ohm (recomended) or atleast 470 Ohm or greater for assured
performance in HS mode.
Signed-off-by: Moiz Sonasath m-sonas...@ti.com
Signed-off-by: Allen Pais allen.p...@ti.com
Signed-off-by: Vikram Pandita vikram.pand...@ti.com
I did resend this patch as per our last conversation, Can you please take this
patch in?
---
arch/arm/mach-omap2/i2c.c | 24
arch/arm/plat-omap/include/plat/control.h | 14 ++
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/i2c.c b/arch/arm/mach-omap2/i2c.c
index 789ca8c..2e6eb28 100644
--- a/arch/arm/mach-omap2/i2c.c
+++ b/arch/arm/mach-omap2/i2c.c
@@ -22,6 +22,7 @@
#include plat/cpu.h
#include plat/i2c.h
#include plat/mux.h
+#include plat/control.h
#include mux.h
@@ -52,5 +53,28 @@ int __init omap_register_i2c_bus(int bus_id, u32
clkrate,
omap_mux_init_signal(mux_name, OMAP_PIN_INPUT);
}
+ /* Disable OMAP 3630 internal pull-ups for all I2Ci */
+ if (cpu_is_omap3630()
!(omap_ctrl_readl(OMAP343X_CONTROL_PROG_IO1)
OMAP3630_PRG_I2C1_PULLUPRESX)) {
+
+ u32 prog_io;
+
+ prog_io = omap_ctrl_readl(OMAP343X_CONTROL_PROG_IO1);
+ /* Program (bit 19)=1 to disable internal pull-up on I2C1 */
+ prog_io |= OMAP3630_PRG_I2C1_PULLUPRESX;
+ /* Program (bit 0)=1 to disable internal pull-up on I2C2 */
+ prog_io |= OMAP3630_PRG_I2C2_PULLUPRESX;
+ omap_ctrl_writel(prog_io, OMAP343X_CONTROL_PROG_IO1);
+
+ prog_io = omap_ctrl_readl(OMAP36XX_CONTROL_PROG_IO2);
+ /* Program (bit 7)=1 to disable internal pull-up on I2C3 */
+ prog_io |= OMAP3630_PRG_I2C3_PULLUPRESX;
+ omap_ctrl_writel(prog_io, OMAP36XX_CONTROL_PROG_IO2);
+
+ prog_io = omap_ctrl_readl(OMAP36XX_CONTROL_PROG_IO_WKUP1);
+ /* Program (bit 5)=1 to disable internall pull-up on I2C4(SR)
*/
+ prog_io |= OMAP3630_PRG_SR_PULLUPRESX;
+ omap_ctrl_writel(prog_io, OMAP36XX_CONTROL_PROG_IO_WKUP1);
+ }
+
return omap_plat_register_i2c_bus(bus_id, clkrate, info, len);
}
diff --git a/arch/arm/plat-omap/include/plat/control.h b/arch/arm/plat-
omap/include/plat/control.h
index 2074473..9e58d8e 100644
--- a/arch/arm/plat-omap/include/plat/control.h
+++ b/arch/arm/plat-omap/include/plat/control.h
@@ -169,6 +169,9 @@
#define AM35XX_CONTROL_IP_SW_RESET (OMAP2_CONTROL_GENERAL + 0x0328)
#define AM35XX_CONTROL_IPSS_CLK_CTRL(OMAP2_CONTROL_GENERAL + 0x032C)
+/* 36xx-only CONTROL_GENERAL register offsets */
+#define OMAP36XX_CONTROL_PROG_IO2 (OMAP2_CONTROL_GENERAL + 0x0198)
+
/* 34xx PADCONF register offsets */
#define OMAP343X_PADCONF_ETK(i) (OMAP2_CONTROL_PADCONFS + 0x5a8
+ \
(i)*2)
@@ -200,6 +203,9 @@
#define OMAP343X_CONTROL_WKUP_DEBOBS3 (OMAP343X_CONTROL_GENERAL_WKUP +
0x014)
#define OMAP343X_CONTROL_WKUP_DEBOBS4 (OMAP343X_CONTROL_GENERAL_WKUP +
0x018)
+/* 36xx-only GENERAL_WKUP register offsets */
+#define OMAP36XX_CONTROL_PROG_IO_WKUP1 (OMAP343X_CONTROL_GENERAL_WKUP +
0x020)
+
/* 34xx D2D idle-related pins, handled by PM core */
#define OMAP3_PADCONF_SAD2D_MSTANDBY 0x250
#define OMAP3_PADCONF_SAD2D_IDLEACK0x254
@@ -250,6 +256,8 @@
#define OMAP2_PBIASLITEVMODE0(1 0)
/* CONTROL_PROG_IO1 bits */
+#define OMAP3630_PRG_I2C2_PULLUPRESX(1 0)
+#define OMAP3630_PRG_I2C1_PULLUPRESX (1 19)
#define OMAP3630_PRG_SDMMC1_SPEEDCTRL(1 20)
/* CONTROL_IVA2_BOOTMOD bits */
@@ -257,6 +265,12 @@
#define OMAP3_IVA2_BOOTMOD_MASK (0xf 0)
#define OMAP3_IVA2_BOOTMOD_IDLE (0x1 0)
+/* CONTROL_PROG_IO2 bits on omap3630 */
+#define OMAP3630_PRG_I2C3_PULLUPRESX(1 7)
+
+/* CONTROL_PROG_IO_WKUP1 bits on omap3630 */
+#define OMAP3630_PRG_SR_PULLUPRESX(1 5)
+
/* CONTROL_PADCONF_X bits */
#define OMAP3_PADCONF_WAKEUPEVENT0 (1 15)
#define OMAP3_PADCONF_WAKEUPENABLE0 (1 14)
--
1.5.6.3
--
To unsubscribe from this list: send the line unsubscribe
RE: [PATCH 1/2] omap: Disable TWL4030/5030 I2C1/I2C4 internal pull-ups
Hello Eduardo,
-Original Message-
From: Eduardo Valentin [mailto:eduardo.valen...@nokia.com]
Sent: Wednesday, February 17, 2010 1:46 AM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; sa...@linux.intel.com; t...@atomide.com;
Pais, Allen
Subject: Re: [PATCH 1/2] omap: Disable TWL4030/5030 I2C1/I2C4 internal
pull-ups
Hello Moiz,
On Wed, Feb 17, 2010 at 01:57:21AM +0100, ext Moiz Sonasath wrote:
This patch disables TWL4030/5030 I2C1 adn I2C4(SR) internal pull-up, to
use only the external HW resistor =470 Ohm for the assured
functionality in HS mode.
While testing the I2C in High Speed mode, it was discovered that
without a proper pull-up resistor, there is data corruption during
multi-byte transfer. RTC(time_set) test case was used for testing.
Nice findings!
From the analysis done, it was concluded that ideally we need a
pull-up of 1.6k Ohm(recomended) or atleast 470 Ohm or greater for
assured performance in HS mode.
Signed-off-by: Moiz Sonasath m-sonas...@ti.com
Signed-off-by: Allen Pais allen.p...@ti.com
---
drivers/mfd/twl-core.c | 13 +
include/linux/i2c/twl.h | 15 +++
2 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 2a76065..35ae2ee 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -965,6 +965,7 @@ twl_probe(struct i2c_client *client, const struct
i2c_device_id *id)
int status;
unsignedi;
struct twl4030_platform_data*pdata = client-dev.platform_data;
+ u8 temp;
if (!pdata) {
dev_dbg(client-dev, no platform data?\n);
@@ -1032,6 +1033,18 @@ twl_probe(struct i2c_client *client, const struct
i2c_device_id *id)
goto fail;
}
+ /* Disable TWL4030/TWL5030 I2C Pull-up on I2C1 and I2C4(SR)
interface.
+* Program I2C_SCL_CTRL_PU(bit 0)=0, I2C_SDA_CTRL_PU (bit 2)=0,
+* SR_I2C_SCL_CTRL_PU(bit 4)=0 and SR_I2C_SDA_CTRL_PU(bit 6)=0.
+*/
+
+ if (twl_class_is_4030()) {
Is this run time check sufficient enough to determine if this fix must be
applied?
From what you have described on you patch description, looks like it is
supposed to be
only for HS mode. I don't know, maybe if you bind this to a platform /
board configuration
flag would it be better? I mean, you create a flag inside the platform
data to determine
if this fix must be applied or not. At least for me, having an external pu
resistor
seams to be very board dependent.
From what I observed, having a smaller pull-up (470 OHM) resistor is
affecting HS mode operation. Moreover, irrespective of whatever external
pull-up value we have, if we have internal pull-up enabled, it comes in
parallel to the external pull-up and thereby reduces the effective pull-up on
the line. So it's safe to have a higher pull-up value instead rather than
having the internal pull-up enabled (irrespective of whatever the external
value is). Also, for all the OMAP boards that we have I think we always have
an external pull-up typically 470 ohm and hence if we just depend on that
external resistor without involving any of the internal pull-ups, HS/FS/LS
should just work fine. That was the whole idea.
+ twl_i2c_read_u8(TWL4030_MODULE_INTBR, temp, REG_GPPUPDCTR1);
+ temp = ~(SR_I2C_SDA_CTRL_PU | SR_I2C_SCL_CTRL_PU | \
+ I2C_SDA_CTRL_PU | I2C_SCL_CTRL_PU);
+ twl_i2c_write_u8(TWL4030_MODULE_INTBR, temp, REG_GPPUPDCTR1);
+ }
+
status = add_children(pdata, id-driver_data);
fail:
if (status 0)
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index bf1c5be..fd95eca 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -239,6 +239,21 @@ int twl6030_interrupt_mask(u8 bit_mask, u8 offset);
/*-
-*/
+/*Interface Bit Register (INTBR) offsets
+ *(Use TWL_4030_MODULE_INTBR)
+ */
+
+#define REG_GPPUPDCTR1 0x0F
+
+/*I2C1 and I2C4(SR) SDA/SCL pull-up control bits */
+
+#define I2C_SCL_CTRL_PUBIT(0)
+#define I2C_SDA_CTRL_PUBIT(2)
+#define SR_I2C_SCL_CTRL_PU BIT(4)
+#define SR_I2C_SDA_CTRL_PU BIT(6)
+
+/*-
-*/
+
/*
* Keypad register offsets (use TWL4030_MODULE_KEYPAD)
* ... SIH/interrupt only
--
1.5.6.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Eduardo Valentin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo
RE: [PATCH 1/2] omap: Disable GAIA I2C1/I2C4 internal pull-ups
Tony/Allen/Paul, -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Tony Lindgren Sent: Tuesday, February 02, 2010 10:56 AM To: Pais, Allen Cc: linux-omap@vger.kernel.org Subject: Re: [PATCH 1/2] omap: Disable GAIA I2C1/I2C4 internal pull-ups * Pais, Allen allen.p...@ti.com [100201 18:56]: From: Tony Lindgren [t...@atomide.com] Sent: Monday, February 01, 2010 7:53 PM To: Pais, Allen Cc: linux-omap@vger.kernel.org Subject: Re: [PATCH 1/2] omap: Disable GAIA I2C1/I2C4 internal pull-ups Hi, * Pais, Allen allen.p...@ti.com [100121 02:31]: From 4044fcc9c517e86fbea9f7d3b15d5cf75a767476 Mon Sep 17 00:00:00 2001 From: Allen Pais allen.p...@ti.com Date: Thu, 21 Jan 2010 21:00:04 +0530 Subject: [PATCH 1/2] omap: Disable GAIA I2C1/I2C4 internal pull-ups This patch disables GAIA I2C1 adn I2C4(SR) internal pull-up, to use only the external HW resistor =470 Ohm for the assured functionality in HS mode. While testing the I2C in High Speed mode, it was discovered that without a proper pull-up resistor, there is data corruption during multi-byte transfer. RTC(time_set) test case was used for testing. From the analysis done, it was concluded that ideally we need a pull-up of 1.6k Ohm(recomended) or atleast 470 Ohm or greater for assured performance in HS mode. Does this apply to 3630 only, or also 34xx? Is this safe to do always? [Allen] Yes, it does apply to 36xx only. There is some confusion, this patch holds true for all OMAP34XX, 36XX as well as 44XX. The idea was to rely on only external Pull-up for I2C operation and disable any internal pull-up on any of the connected power IC's: Triton(TWL4030)/GAIA(TWL5030)/Phoenix(TWL6030) I did verify, the register 'REG_GPPUPDCTR1' holds true for Triton(TWL4030)/GAIA(TWL5030), hence for OMAP34XX/36XX. But there is a register change for this PU/PD control on Phoenix(TWL6030) which is used with OMAP44XX. So I might have to modify this patch a little as this code will apply for TWL4030/5030 and have to introduce new code to achieve the intended fix for TWL6030. The second patch of the series: 'omap: 3630: Disable internal pull-ups' applies only for OMAP36XX as it is a new feature introduced in OMAP3630. I have to check if that feature is available on OMAP44XX as well, if so I will modify that patch to extend to OMAP44XX in future. Although Sounds like then this configuration should be passed from the board-*.c file in platform_data as the external pulls depend on the board. BTW, once ready it hould be sent to Samuel Ortiz with linux-omap list Cc'd: [Allen] i'll have it sent to Samuel also. Thanks, we can't merge it yet though, see above. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Regards Moiz Sonasath -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] omap: Disable GAIA I2C1/I2C4 internal pull-ups
Paul, -Original Message- From: Paul Walmsley [mailto:p...@pwsan.com] Sent: Monday, February 01, 2010 9:37 PM To: Pais, Allen Cc: linux-omap@vger.kernel.org; Sonasath, Moiz; t...@atomide.com Subject: Re: [PATCH 1/2] omap: Disable GAIA I2C1/I2C4 internal pull-ups Hi Allen, On Thu, 21 Jan 2010, Pais, Allen wrote: This patch disables GAIA I2C1 adn I2C4(SR) internal pull-up, to use only the external HW resistor =470 Ohm for the assured functionality in HS mode. While testing the I2C in High Speed mode, it was discovered that without a proper pull-up resistor, there is data corruption during multi-byte transfer. RTC(time_set) test case was used for testing. From the analysis done, it was concluded that ideally we need a pull-up of 1.6k Ohm(recomended) or atleast 470 Ohm or greater for assured performance in HS mode. A few more questions. 1. Does this patch also apply to other TI PMICs, e.g., Triton2, Reno, etc? Thanks for raising this important question. This patch does hold true for the following PM IC's: Triton(TWL4030)/GAIA(TWL5030)/Phoenix(TWL6030) I did verify, the register 'REG_GPPUPDCTR1' holds true for Triton(TWL4030)/GAIA(TWL5030), hence for OMAP34XX/36XX. But there is a register change for this PU/PD control on Phoenix(TWL6030) which is used with OMAP44XX. So I might have to modify this patch a little as this code will apply for TWL4030/5030 and have to introduce new code to achieve the intended fix for TWL6030. I don't think so it applies to Reno though, AFAIK its not included in the Triton family and not used with OMAP on any boards. 2. It sounds like, from the patch description, that HS I2C mode is not reliable on boards without external pullups. Shouldn't this patch (or one in the same series) provide some way for mach-omap2/board-*.c files to indicate this to the I2C code, such that HS I2C can be disallowed on those boards? Similarly, for SmartReflex, which has an internal I2C controller but does not use the OMAP I2C driver code, shouldn't there be a similar mechanism for that code? Its not that HS I2C mode is not reliable without external pull-ups but what was observed is that, for reliable operation a HW resitor = 470 Ohm is needed as pull-up. Now if we keep the internal pull-ups on PIC's enabled they come in parallel to the external pull-up and might lead to lower equivalent resistor value ( required 470 Ohm). That was the reason we decided to just rely on the external resistor on OMAP SOM, and disable any internal pull-ups on Power IC as well as on OMAP (36XX/44XX). - Paul -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] omap i2c: add a timeout to the busy waiting
Alex,
-Original Message-
From: Menon, Nishanth
Sent: Thursday, December 17, 2009 7:59 AM
To: Menon, Nishanth; ben-li...@fluff.org; linux-omap@vger.kernel.org;
linux-...@vger.kernel.org; Sonasath, Moiz; Pandita, Vikram
Subject: Re: [PATCH 2/2] omap i2c: add a timeout to the busy waiting
Alexander Shishkin said the following on 12/17/2009 07:01 PM:
On Thu, Dec 17, 2009 at 08:38:39 +0530, Menon, Nishanth wrote:
Alexander Shishkin said the following on 12/16/2009 07:32 PM:
The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
context, which may lead to kernel hangs. The problem can be reproduced
by running the bus with wrong (too high) speed.
Signed-off-by: Alexander Shishkin virtu...@slind.org
CC: linux-...@vger.kernel.org
CC: linux-omap@vger.kernel.org
---
drivers/i2c/busses/i2c-omap.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-
omap.c
index ad8242a..b474c20 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -678,7 +678,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
*/
static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat,
int *err)
{
- while (!(*stat OMAP_I2C_STAT_XUDF)) {
+ unsigned long timeout = 1;
+
+ while (!(*stat OMAP_I2C_STAT_XUDF --timeout)) {
a) timeout without using an actual delay is not a good idea -
consider each OPP - we can go upto 1ghz on 3630,
the actual time for 1 iterations will depend on the MPU speed.
Well, I could calculate the timeout value based on current operating
speed,
I guess. Or a delay. Perhaps OMAP_I2C_TIMEOUT can be used here?
it might be an overkill trying to generate counter based on opp speeds.
wondering if this delay is required in the first place..
The reason why this timeout was not put in place was that if the I2C controller
is functional, the XUDF has to set after XRDY/XDR are set, the only case where
it might not be set is when there is a sudden NACK | AL condition produced on
the bus and the transfer stops (thereby the FIFO is not getting empty). The
code takes care of this error situation so IMHO we can do without a timeout
here.
--- Moiz
Moiz Sonasath / Vikram P, (commit cd086d3a) could probably comment?
Ref: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg15317.html
b) how did you arrive at the 10k iteration limit?
It was random, but then it seemed ok considering the l4 latency.
I had guessed this is an emperical value. It will be good to know the
exact rationale on why a timeout will happen, else we will have all kind
of questions pending trying to figure out if a timeout happened.
if (*stat (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
omap_i2c_ack_stat(dev, *stat (OMAP_I2C_STAT_XRDY |
OMAP_I2C_STAT_XDR));
@@ -689,6 +691,9 @@ static int omap3430_workaround(struct omap_i2c_dev
*dev, u16 *stat, int *err)
*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
}
+ if (!timeout)
+ dev_err(dev-dev, timeout waiting on XUDF bit\n);
+
return 0;
}
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] MUSB: Workaround for Ethernet data alignment issue
Nilkesh,
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
Sent: Monday, December 14, 2009 11:39 AM
To: Patra, Nilkesh; linux-...@vger.kernel.org
Cc: linux-omap@vger.kernel.org; David Brownell; Gadiyar, Anand
Subject: RE: [RFC] MUSB: Workaround for Ethernet data alignment issue
Nilkesh
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Patra,
Nilkesh
Sent: Sunday, December 13, 2009 10:51 PM
To: linux-...@vger.kernel.org
Cc: linux-omap@vger.kernel.org; David Brownell; Gadiyar, Anand
Subject: [RFC] MUSB: Workaround for Ethernet data alignment issue
On the latest version of MUSB, DMA is not working properly if the Data in
the packet doesn't start at
32bit aligned address. This issue was found while using MUSB as g_ether.
The basic ping does not
work, if the data in the Ethernet packet does not start at 32bit aligned
address.
To overcome this issue, we found one solution mentioned in the below
patch. This patch moves data
(skb-data) by 2 bytes backwards in ethernet packet, which is nothing but
skb-head.
I want to know, if there are any better approaches to fix this issue.
I would NAK the first part of patch for following reasons (and suggestions
are inlined):
a) this change is specific to OMAP hw only and so should not try to change
generic files like usb/gadget/u_ether.c
that get compiled for all the architectures having usb.
b) there are two issues to be handled separately ( separate patches are
good for review )
RX side buffer alignments handling
TX side buffer alignments handling
Signed-off-by: Nilkesh Patra nilkesh.pa...@ti.com
---
drivers/usb/gadget/u_ether.c | 14 ++
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index 2fc02bd..432b1bd 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -249,7 +249,6 @@ rx_submit(struct eth_dev *dev, struct usb_request
*req, gfp_t gfp_flags)
* but on at least one, checksumming fails otherwise. Note:
* RNDIS headers involve variable numbers of LE32 values.
*/
-skb_reserve(skb, NET_IP_ALIGN);
NAK. Do not remove this line,
Instead over-ride the NET_IP_ALIGN macro for your architecture.
One suggestion to do so:
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 058e7e9..b16c5f7 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -514,6 +514,10 @@ static inline unsigned long long
__cmpxchg64_mb(volatile void *ptr,
#define arch_align_stack(x) (x)
+#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
+#define NET_IP_ALIGN 0
+#endif
+
#endif /* __KERNEL__ */
#endif
This has been verified to work for OMAP3630.
req-buf = skb-data;
req-length = size;
@@ -500,6 +499,8 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
*skb,
unsigned long flags;
struct usb_ep *in;
u16 cdc_filter;
+int i = 0;
+
spin_lock_irqsave(dev-lock, flags);
if (dev-port_usb) {
@@ -573,9 +574,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
*skb,
length = skb-len;
}
-req-buf = skb-data;
-req-context = skb;
-req-complete = tx_complete;
This change would be to handle TX un-alignments.
So I would prefer to handle it separately ( a separate patch)
Have you explored the option of over-riding #define NET_SKB_PAD ??
/* use zlp framing on tx for strict CDC-Ether conformance,
* though any robust network rx path ignores extra padding.
@@ -585,6 +583,14 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
*skb,
if (!dev-zlp (length % in-maxpacket) == 0)
length++;
+if ((int)skb-data 0x2) {
You can use this wrapper instead:
!IS_ALIGNED(skb-data, 4)
+skb_push(skb, 2);
Where have you made sure that skb-data allocation has this extra space of
two bytes?
+for (i = 0; i length; i++)
+skb-data[i] = skb-data[i+2];
+}
+req-buf = skb-data;
+req-context = skb;
+req-complete = tx_complete;
req-length = length;
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
-
Moiz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: I2C broken on OMAP 2430SDP?
Hello Paul! Thank you for pointing this out. I think we are missing a patch here which was actually sent for up-stream but did not make through as I don't see it on my latest master branch. http://patchwork.kernel.org/patch/49514/ Can you please apply this and see if it helps, I don't have any 2430SDP platform to test. Regards Moiz Sonasath -Original Message- From: Paul Walmsley [mailto:p...@pwsan.com] Sent: Tuesday, December 01, 2009 10:01 AM To: linux-omap@vger.kernel.org Cc: Sonasath, Moiz Subject: I2C broken on OMAP 2430SDP? Hi, just wanted to mention that i2c-omap.c has some problems on the 2430SDP with the linux-omap kernel running Linux 2.6.32-rc8, commit 4355c41a635943d30e9396b95185314343dcb551. This shows up at boot: bio: create slab bio-0 at 0 6i2c_omap i2c_omap.1: bus 1 rev3.6 at 400 kHz 6i2c_omap i2c_omap.2: bus 2 rev3.6 at 2600 kHz 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing gpio IMR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing gpio SIH_CTRL 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing keypad IMR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing keypad SIH_CTRL 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing bci IMR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing madc IMR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing power IMR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing power SIH_CTRL 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing gpio ISR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing gpio ISR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing keypad ISR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing keypad ISR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing bci ISR 4i2c_omap i2c_omap.2: timeout waiting for bus ready 4i2c_omap i2c_omap.2: timeout waiting for bus ready 3twl4030: err -110 initializing bci ISR ... This is with omap_2430sdp_defconfig. - Paul -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency constraint in i2c
-Original Message-
From: Kalle Jokiniemi [mailto:kalle.jokini...@digia.com]
Sent: Tuesday, October 27, 2009 7:02 AM
To: Sonasath, Moiz
Cc: khil...@deeprootsystems.com; linux-omap@vger.kernel.org; Jarkko
Nikula; Paul Walmsley; Menon, Nishanth; Pandita, Vikram; jouni.hogander
Subject: RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency constraint
in i2c
On Fri, 2009-10-23 at 18:53 +0300, Sonasath, Moiz wrote:
Hello Jokiniemi!
-Original Message-
From: Kalle Jokiniemi [mailto:kalle.jokini...@digia.com]
Sent: Wednesday, October 21, 2009 6:51 AM
To: khil...@deeprootsystems.com
Cc: linux-omap@vger.kernel.org; Kalle Jokiniemi; Sonasath, Moiz;
Jarkko
Nikula; Paul Walmsley; Menon, Nishanth
Subject: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency
constraint in
i2c
While waiting for completion of the i2c transfer, the
MPU could hit OFF mode and cause several msecs of
delay that made i2c transfers fail more often. The
extra delays and subsequent re-trys cause i2c clocks
to be active more often. This has also an negative
effect on power consumption.
Created a mechanism for passing and using the
constraint setting function in driver code. The used
mpu wake up latency constraints are now set individually
per bus, and they are calculated based on clock rate
and fifo size.
Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley,
and Nishanth Menon for tuning out the details of
this patch.
Cc: Moiz Sonasath m-sonas...@ti.com
Cc: Jarkko Nikula jhnik...@gmail.com
Cc: Paul Walmsley p...@pwsan.com
Cc: Nishanth Menon n...@ti.com
Signed-off-by: Kalle Jokiniemi kalle.jokini...@digia.com
---
dev-speed = speed;
dev-idle = 1;
@@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev)
*/
dev-fifo_size = (dev-fifo_size / 2);
dev-b_hw = 1; /* Enable hardware fixes */
+
+ /* calculate wakeup latency constraint for MPU */
+ if (dev-set_mpu_wkup_lat != NULL)
+ dev-latency = (100 * dev-fifo_size) /
+(1000 * speed / 8);
}
IMHO, here instead of using 'dev-fifo_size' for calculating the
'dev-latency', we need to use:
1. For RX case, to avoid Reciver overrun:
if (msg-flags I2C_M_RD)
Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in calculating
dev-latency
Because conceptually, RDR/RRDY interrupts are generated when RTRSH is
reached, so we want the MPU to be wake up within the time it takes to
fill the FIFO from RTRSH to FIFO Depth (FIFO full).
2. For TX case, to avoid Transmitter Underflow:
if (!(msg-flags I2C_M_RD))
Use (FIFO THRSH)bytes in calculating dev-latency
Because conceptually, XDR/XRDY interrupts are generated when XTRSH is
reached, so we want the MPU to be wake up within the time it takes to
drain the FIFO from XTRSH to Zero (FIFO empty).
Using, dev-fifo_size instead, works in the present code because we
have a RTRSH/XTRSH = dev-fifo_size/2 = 4 bytes
And therefore,
(FIFO Depth)bytes - (FIFO THRSH)bytes
= 8 - 4 = 4 bytes
But, to make it more generic in future and to make it independent of
any changes in the RTRSH/XTRSH values or FIFO depths in future, we
should use a generic code here.
Well, I don't completely agree with the necessity of preparing for
different rx/tx thresholds. For this to make sense, the i2c-omap driver
should first separate in it's code the use of rx and tx thresholds. If
someone is planning to do that, he/she should anyway update the usage of
fifo_size throughout the code, including the wake up latency setting.
Anyways, attached a patch that separates the mpu wake up latencies for
rx and tx. In case that is needed. Though I'm not for it, since it adds
unneeded complexity.
Yes Kalle, you are right! Not having different RX/TX wake-up latencies will
absolutely work fine with the in-place code as we have both the RX/TX
thresholds same. But, I think in future we might have to play around with
different RX/TX thresholds and so from a conceptually right and generic point
of view it makes sense to incur the cost of the added complexity.
The patch V4 looks perfect to me :)
- Moiz
- Kalle
/* reset ASAP, clearing any IRQs */
diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
new file mode 100644
index 000..1362fba
--- /dev/null
+++ b/include/linux/i2c-omap.h
@@ -0,0 +1,9 @@
+#ifndef __I2C_OMAP_H__
+#define __I2C_OMAP_H__
+
+struct omap_i2c_bus_platform_data {
+ u32 clkrate;
+ void(*set_mpu_wkup_lat)(struct device *dev, int set);
+};
+
+#endif
--
1.5.4.3
Regards
Moiz Sonasath
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More
RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency constraint in i2c
Hello Jokiniemi!
-Original Message-
From: Kalle Jokiniemi [mailto:kalle.jokini...@digia.com]
Sent: Wednesday, October 21, 2009 6:51 AM
To: khil...@deeprootsystems.com
Cc: linux-omap@vger.kernel.org; Kalle Jokiniemi; Sonasath, Moiz; Jarkko
Nikula; Paul Walmsley; Menon, Nishanth
Subject: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency constraint in
i2c
While waiting for completion of the i2c transfer, the
MPU could hit OFF mode and cause several msecs of
delay that made i2c transfers fail more often. The
extra delays and subsequent re-trys cause i2c clocks
to be active more often. This has also an negative
effect on power consumption.
Created a mechanism for passing and using the
constraint setting function in driver code. The used
mpu wake up latency constraints are now set individually
per bus, and they are calculated based on clock rate
and fifo size.
Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley,
and Nishanth Menon for tuning out the details of
this patch.
Cc: Moiz Sonasath m-sonas...@ti.com
Cc: Jarkko Nikula jhnik...@gmail.com
Cc: Paul Walmsley p...@pwsan.com
Cc: Nishanth Menon n...@ti.com
Signed-off-by: Kalle Jokiniemi kalle.jokini...@digia.com
---
dev-speed = speed;
dev-idle = 1;
@@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev)
*/
dev-fifo_size = (dev-fifo_size / 2);
dev-b_hw = 1; /* Enable hardware fixes */
+
+ /* calculate wakeup latency constraint for MPU */
+ if (dev-set_mpu_wkup_lat != NULL)
+ dev-latency = (100 * dev-fifo_size) /
+(1000 * speed / 8);
}
IMHO, here instead of using 'dev-fifo_size' for calculating the
'dev-latency', we need to use:
1. For RX case, to avoid Reciver overrun:
if (msg-flags I2C_M_RD)
Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in calculating
dev-latency
Because conceptually, RDR/RRDY interrupts are generated when RTRSH is reached,
so we want the MPU to be wake up within the time it takes to fill the FIFO from
RTRSH to FIFO Depth (FIFO full).
2. For TX case, to avoid Transmitter Underflow:
if (!(msg-flags I2C_M_RD))
Use (FIFO THRSH)bytes in calculating dev-latency
Because conceptually, XDR/XRDY interrupts are generated when XTRSH is reached,
so we want the MPU to be wake up within the time it takes to drain the FIFO
from XTRSH to Zero (FIFO empty).
Using, dev-fifo_size instead, works in the present code because we have a
RTRSH/XTRSH = dev-fifo_size/2 = 4 bytes
And therefore,
(FIFO Depth)bytes - (FIFO THRSH)bytes
= 8 - 4 = 4 bytes
But, to make it more generic in future and to make it independent of any
changes in the RTRSH/XTRSH values or FIFO depths in future, we should use a
generic code here.
/* reset ASAP, clearing any IRQs */
diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
new file mode 100644
index 000..1362fba
--- /dev/null
+++ b/include/linux/i2c-omap.h
@@ -0,0 +1,9 @@
+#ifndef __I2C_OMAP_H__
+#define __I2C_OMAP_H__
+
+struct omap_i2c_bus_platform_data {
+ u32 clkrate;
+ void(*set_mpu_wkup_lat)(struct device *dev, int set);
+};
+
+#endif
--
1.5.4.3
Regards
Moiz Sonasath
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RESEND] I2C: OMAP: Add missing wakeup events
Hello Aaro! -Original Message- From: Aaro Koskinen [mailto:aaro.koski...@nokia.com] Sent: Tuesday, October 13, 2009 4:52 AM To: Sonasath, Moiz Cc: ben-li...@fluff.org; linux-...@vger.kernel.org; linux- o...@vger.kernel.org Subject: Re: [PATCH RESEND] I2C: OMAP: Add missing wakeup events Hello, Sonasath, Moiz wrote: From: Jagadeesh Bhaskar Pakaravoor j-pakarav...@ti.com Include wake up events for receiver overflow and transmitter underflow in OMAP_I2C_WE_REG configuration. Also fix a small typo. Signed-off-by: Jagadeesh Bhaskar Pakaravoor j-pakarav...@ti.com Signed-off-by: Aaro Koskinen aaro.koski...@nokia.com --- drivers/i2c/busses/i2c-omap.c |5 - 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c- omap.c index 827da08..34ea9ed 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -92,8 +92,10 @@ #define OMAP_I2C_STAT_AL (1 0)/* Arbitration lost int ena */ /* I2C WE wakeup enable register */ -#define OMAP_I2C_WE_XDR_WE(1 14) /* TX drain wakup */ +#define OMAP_I2C_WE_XDR_WE(1 14) /* TX drain wakeup */ #define OMAP_I2C_WE_RDR_WE(1 13) /* RX drain wakeup */ +#define OMAP_I2C_WE_ROVR_WE (1 11) /* RX overflow wakeup */ +#define OMAP_I2C_WE_XUDF_WE (1 10) /* TX underflow wakeup */ These bits are not documented in OMAP3430, they are reserved. How can they be used? Hmm, that's a valid point. I will have to check if I can find more info on the background of this patch. AFAIK, these bits have been introduced in OMAP3630 as it has a new IP block for I2C. But these bits are reserved bits for OMAP3430. A. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RESEND] I2C: OMAP: Add missing wakeup events
Hello Aaro! -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Aaro Koskinen Sent: Monday, October 12, 2009 5:21 AM To: ben-li...@fluff.org; linux-...@vger.kernel.org Cc: linux-omap@vger.kernel.org; j-pakarav...@ti.com Subject: [PATCH RESEND] I2C: OMAP: Add missing wakeup events From: Jagadeesh Bhaskar Pakaravoor j-pakarav...@ti.com Include wake up events for receiver overflow and transmitter underflow in OMAP_I2C_WE_REG configuration. Also fix a small typo. Signed-off-by: Jagadeesh Bhaskar Pakaravoor j-pakarav...@ti.com Signed-off-by: Aaro Koskinen aaro.koski...@nokia.com --- drivers/i2c/busses/i2c-omap.c |5 - 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 827da08..34ea9ed 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -92,8 +92,10 @@ #define OMAP_I2C_STAT_AL (1 0)/* Arbitration lost int ena */ /* I2C WE wakeup enable register */ -#define OMAP_I2C_WE_XDR_WE (1 14) /* TX drain wakup */ +#define OMAP_I2C_WE_XDR_WE (1 14) /* TX drain wakeup */ #define OMAP_I2C_WE_RDR_WE (1 13) /* RX drain wakeup */ +#define OMAP_I2C_WE_ROVR_WE (1 11) /* RX overflow wakeup */ +#define OMAP_I2C_WE_XUDF_WE (1 10) /* TX underflow wakeup */ These bits are not documented in OMAP3430, they are reserved. How can we use them? #define OMAP_I2C_WE_AAS_WE (1 9)/* Address as slave wakeup*/ #define OMAP_I2C_WE_BF_WE(1 8)/* Bus free wakeup */ #define OMAP_I2C_WE_STC_WE (1 6)/* Start condition wakeup */ @@ -104,6 +106,7 @@ #define OMAP_I2C_WE_AL_WE(1 0)/* Arbitration lost wakeup */ #define OMAP_I2C_WE_ALL (OMAP_I2C_WE_XDR_WE | OMAP_I2C_WE_RDR_WE | \ + OMAP_I2C_WE_ROVR_WE | OMAP_I2C_WE_XUDF_WE | \ OMAP_I2C_WE_AAS_WE | OMAP_I2C_WE_BF_WE | \ OMAP_I2C_WE_STC_WE | OMAP_I2C_WE_GC_WE | \ OMAP_I2C_WE_DRDY_WE | OMAP_I2C_WE_ARDY_WE | \ -- 1.6.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Regards Moiz Sonasath -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
Hello Jokiniemi, -Original Message- From: Kalle Jokiniemi [mailto:kalle.jokini...@digia.com] Sent: Wednesday, October 07, 2009 10:01 AM To: Nishanth Menon Cc: Pandita, Vikram; khil...@deeprootsystems.com; Sonasath, Moiz; jhnik...@gmail.com; linux-omap@vger.kernel.org; jouni.hogander Subject: Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c On Wed, 2009-10-07 at 13:49 +0300, Nishanth Menon wrote: Kalle Jokiniemi said the following on 10/07/2009 05:10 AM: extra delays and subsequent re-trys cause i2c clocks to be active more often. This has also an negative effect on power consumption. Added a constraint that allows MPU to wake up in few hundred usecs, which is roughly the average i2c wait period. How did you arrive at the number 500us for the wakeup latency? This was about the average time that I observed i2c-transfers to be (from clk_enable to clk_disable), when off mode was not enabled. just a quick 2cents: I understand that the 500uSec might need to be board dependent - here is why: one theory we have regarding the timeout delay having to be modified on zoom2 was that if the delay was 500uSec, the touch screen controller timesout and pulls of the data in the middle of transfer. You have a good point there. I think this will require changing the way of passing the bus-specific platform data: currently bus-id and bus-rate are passed as function parameters of omap_register_i2c_bus. IMO that would need to change to use just one omap_i2c_bus_platform_data structure that would contain these and the mpu wake up constraint. The other option is to add another parameter to omap_register_i2c_bus, but that does not seem as extensible to me. It's much easier to add even another platform parameter to a struct, than adding a new function parameter. With struct you can always use some known good value in case the parameter is not defined, but in function parameter we need to always modify all the board files. Either way, it will be a time consuming task to go through all those board files, since they currently use those function parameters. If I find that time, I'll do it. BTW, those cpu idle C-state latencies and thresholds could also be board-specific. Reasons: we already set board specific voltage and oscillator setup times and we have different idle/active consumptions (due to differences in peripherals / pad configs etc.), which effect threshold values. Kevin, take note, maybe something to add to TODO in elinux wiki ;) Sometime back, Paul Walmsley mentioned the calculation for coming up with platform dependent wake-up latency to avoid any receiver overrun problems, the link to that is here: http://www.pwsan.com/omap/i2c-wakeup-latency.html Based on this, we should calculate the wake-up latency in the i2c-omap.c file using the following equation: Parameter to pass to MPU Wakeup latency = [(FIFO Depth)bytes - (FIFO THRSH)bytes]/ [I2C byte rate] Where, I2C byte rate = [(dev-speed)/8] bytes/sec This should solve our purpose for any platform. Using this expression in Zoom2 scenario, with FIFO depth = 8 Bytes FIFO THRSH = 4 Bytes I2C byte rate = 100K / 8 = 12500 Bytes/sec (For I2C Bus2 running at 100KHz/sec on which we have the touch screen controller) Parameter to pass to MPU Wakeup latency = 320 usec Which, when applied overcomes the touch screen problem. The reason why even 400 usec solved the issue was because the exit latency for C3 state was 410 usec and so with 400 usec in place, the system will only go to C2 state with exit latency of 18 usec. Following link gives the full discussion that happened on some issue of spurious IRQ's and has a good explanation about this wake-up latency issue by Paul: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg11973.html - Kalle Regards, Nishanth Menon Regards Moiz Sonasath -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Hi Paul,
-Original Message-
From: Paul Walmsley [mailto:p...@pwsan.com]
Sent: Sunday, August 16, 2009 12:39 PM
To: Sonasath, Moiz
Cc: linux-...@vger.kernel.org; linux-omap@vger.kernel.org; Menon,
Nishanth; Pandita, Vikram
Subject: RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Hi Moiz,
On Tue, 11 Aug 2009, Sonasath, Moiz wrote:
1. From the code we have in place now, we are writing
num_bytes=dev-fifosize(=XTRSH+1) bytes in case of XRDY OR
num_bytes=TXSTAT (last few bytes left) in case of XDR (draining feature)
Without the errata we were writing these num_bytes in a while loop one
by one into I2C_DATA reg (TXFIFO).
With the errata we are writing num_bytes in a while loop, but now we
write one byte wait for XUDF bit to get set and then write another byte.
Thereby, we write a byte, wait for it to get out of the TXFIFO and only
then we write the second byte and so on.
While(num_bytes)
{
Write one byte to I2C_DATA Reg
Wait for XUDF to set
}
Ack the interrupt
Doesn't your patch do:
While(num_bytes)
{
Wait for XUDF to set
Write one byte to I2C_DATA Reg
}
Ack the interrupt
?
Yes I think I just disordered the sequence there, but yes this is exactly what
the patch does.
So irrespective of the XTRSH value, the wait time is actually the same.
2. Now if we see it from the perspective of interrupts, we are
generating an interrupt after every chunk of 4 bytes (with XTRSH+1=4)
written to the TXFIFO because we ACK the interrupt after writing a chunk
of 4 bytes (one byte at a time waiting for the XUDF bit to be set in
between each byte).
From the TRM Figure-18-31, in the XRDY path:
Write I2Ci.I2C_DATA register for (XTRSH+1) times and then clear XRDY
interrupt
Thus, XTRSH is actually driving how many chunks of data you write before
generating a next interrupt. So from this point of view it will be more
desirable to make the XTRSH=7 and write chunk of 8 bytes before
generating a new interrupt. But we end up staying a longer time in the
ISR.
Essentially we are looking at a tradeoff between:
1. Lower XTRSH value: More number of interrupts, less time in an ISR
2. Higher XTRSH value: Less number of interrupts, more time in an ISR
Please correct me if I am wrong.
Your analysis looks correct. I suppose my point is dependent on when the
XRDY interrupt occurs if XTRSH = 0.
If it occurs one I2C byte transmission time before the XUDF condition
occurs, then we should just use your busy-waiting patch. I don't think we
can do better than that.
But if the XRDY interrupt occurs at the same time as the shift register
empties, or if there is an undocumented XUDF interrupt that we can use,
then please consider:
For slower I2C bus speeds, e.g., for a 400KHz I2C bus, emptying a byte out
of the transmit FIFO should take about 20 microseconds [1]. Another way
of expressing this is that the duration from when we write a byte to the
I2C FIFO, to when the controller raises the XRDY/XDR interrupt, should be
about 20 microseconds when XTRSH = 0.
We can either spend this time busy-looping in the ISR (the tradeoff #2
that you mention above), or trying to reach WFI and hopefully entering it
(the tradeoff #1 above). If possible, tradeoff #1 seems better. If the
MPU can reach WFI, it will waste less active power, but it will also
trigger the PRCM to shut down any inactive power domains, which might not
need to wake back up when the next system wakeup event occurs.
This should improve over time, from a power efficiency perspective,
as the amount of time the MPU spends trying to reach WFI should decrease
[2].
What do you think?
On having a closer look at the code, I realized that there is a struct i2c_msg
msg[ ] passed to omap_i2c_xfer func in i2c-omap.c driver from the upper
application and this relates to the following assignments:
dev-buf = msg-buf;
dev-buf_len = msg-len;
The code returns from the ISR context only in the following conditions:
-after completing the transfer of dev-len amount of data (ARDY interrupt)
-In case of an error (NACK|AL)
-when the counter is 100 (too much data to send)
So actually, for a given amount of data we spend more or less the same time in
the ISR irrespective of the XTRSH value. I missed this point in my prior
analysis.
With the code in place and for the draining feature to work correctly with the
present code, we want XTRSH=dev-fiffo_size (presently 4).
Now if we make XTRSH=dev-fiffo_size=8 (use the full FIFO size) we can have
slightly faster performance. We have the following code in place:
omap_i2c_isr
{
While(any interrupt)
{
If (NACK|AL|ARDY)
Complete_cmd and return IRQ_HANDLED
If (RRDY|RDR)
RX implemenetation
If (XRDY|XDR
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
-Original Message-
From: linux-i2c-ow...@vger.kernel.org [mailto:linux-i2c-
ow...@vger.kernel.org] On Behalf Of Sonasath, Moiz
Sent: Monday, August 03, 2009 3:20 PM
To: Paul Walmsley
Cc: linux-...@vger.kernel.org; linux-omap@vger.kernel.org; Menon,
Nishanth; Pandita, Vikram
Subject: RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
-Original Message-
From: Paul Walmsley [mailto:p...@pwsan.com]
Sent: Monday, August 03, 2009 2:36 AM
To: Sonasath, Moiz
Cc: linux-...@vger.kernel.org; linux-omap@vger.kernel.org; Menon,
Nishanth; Pandita, Vikram
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Hello Moiz,
A few remaining comments, most of these from an earlier message.
On Tue, 21 Jul 2009, Sonasath, Moiz wrote:
When an XRDY/XDR is hit, wait for XUDF before writing data to
DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.
Do a Busy-wait for XUDF, before writing data to DATA_REG. While
waiting
if there is NACK | AL, set the appropriate error flags, ack the
pending
interrupts and return from the ISR.
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 24 +++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-
omap.c
index 05b5e4c..8deaf87 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
break;
}
+ err = 0;
+complete:
omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
- err = 0;
if (stat OMAP_I2C_STAT_NACK) {
err |= OMAP_I2C_STAT_NACK;
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
data to send\n);
break;
}
+
+ /*
+ * OMAP3430 Errata 1.153: When an XRDY/XDR
+ * is hit, wait for XUDF before writing data
+ * to DATA_REG. Otherwise some data bytes can
+ * be lost while transferring them from the
+ * memory to the I2C interface.
+ */
Based on this description, shouldn't this patch also zero the transmit
FIFO threshold? Consider what the transmit path becomes after this
patch:
1. Fill transmit FIFO
2. Leave ISR wait for interrupt
3. Interrupt happens due to XDR/XRDY (transmit FIFO low-water-mark
reached)
4. Busy-wait until transmit FIFO shift register completely empty
5. If more data to send, go to step #1
i2c-omap.c currently sets the transmit FIFO threshold to 1/2 of the
total
FIFO size[1]. This means that, in the worst case, I2C3, the I2C ISR
will
busy-wait in step 4 for the time it takes 32 bytes to be transmitted.
This is time that the MPU spends doing nothing but spinning, wasting
power. This seems unnecessary and wasteful. The time the driver spends
busy-waiting in the ISR should be reduced to the lowest possible
duration.
To do this, what I suggest that you additionally do in the patch is to
reduce the transit FIFO threshold/low-water-mark, controlled by
I2C_BUF.XTRSH, to the lowest possible value. This should maximize the
time spent between steps 2 and 3 and minimize the time spent between
steps
3 and 5.
Is there a reason why this can't be done?
Yes, this is actually lined up in my list of actions. I will be working on
this to test the functionality and stability of I2C code with the
threshold set to zero.
I did some analysis and testing of the code with threshold set to zero, we
cannot make the threshold zero with the present code in place, as this would
hamper the functionality of the draining feature because in this case the XDR
interrupt will not be triggered.
XDR- when TX FIFO level equal/below the XTRSH AND TXSTAT is less than XTRSH
XRDY- when TX FIFO level equal/below the XTRSH AND TXSTAT is equal/greater
than XTRSH
This in turn causes XRDY to be triggered always (even when there are only last
few bytes left to be transmitted) and therefore the code tries to transmit data
when the upper application is out of data.
+
+ if (cpu_is_omap34xx()) {
Does this erratum apply to the I2C IP block on OMAP2430? It also has
FIFO
transmit capability. It would be ideal if you can find out from the I2C
IP block designers. If you cannot, please consider adding a comment
that
this may also apply to the I2C block on OMAP2430
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
-Original Message-
From: Paul Walmsley [mailto:p...@pwsan.com]
Sent: Monday, August 03, 2009 2:36 AM
To: Sonasath, Moiz
Cc: linux-...@vger.kernel.org; linux-omap@vger.kernel.org; Menon,
Nishanth; Pandita, Vikram
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Hello Moiz,
A few remaining comments, most of these from an earlier message.
On Tue, 21 Jul 2009, Sonasath, Moiz wrote:
When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.
Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
interrupts and return from the ISR.
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 24 +++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-
omap.c
index 05b5e4c..8deaf87 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
break;
}
+ err = 0;
+complete:
omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
- err = 0;
if (stat OMAP_I2C_STAT_NACK) {
err |= OMAP_I2C_STAT_NACK;
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
data to send\n);
break;
}
+
+ /*
+* OMAP3430 Errata 1.153: When an XRDY/XDR
+* is hit, wait for XUDF before writing data
+* to DATA_REG. Otherwise some data bytes can
+* be lost while transferring them from the
+* memory to the I2C interface.
+*/
Based on this description, shouldn't this patch also zero the transmit
FIFO threshold? Consider what the transmit path becomes after this patch:
1. Fill transmit FIFO
2. Leave ISR wait for interrupt
3. Interrupt happens due to XDR/XRDY (transmit FIFO low-water-mark
reached)
4. Busy-wait until transmit FIFO shift register completely empty
5. If more data to send, go to step #1
i2c-omap.c currently sets the transmit FIFO threshold to 1/2 of the total
FIFO size[1]. This means that, in the worst case, I2C3, the I2C ISR will
busy-wait in step 4 for the time it takes 32 bytes to be transmitted.
This is time that the MPU spends doing nothing but spinning, wasting
power. This seems unnecessary and wasteful. The time the driver spends
busy-waiting in the ISR should be reduced to the lowest possible duration.
To do this, what I suggest that you additionally do in the patch is to
reduce the transit FIFO threshold/low-water-mark, controlled by
I2C_BUF.XTRSH, to the lowest possible value. This should maximize the
time spent between steps 2 and 3 and minimize the time spent between steps
3 and 5.
Is there a reason why this can't be done?
Yes, this is actually lined up in my list of actions. I will be working on this
to test the functionality and stability of I2C code with the threshold set to
zero.
+
+ if (cpu_is_omap34xx()) {
Does this erratum apply to the I2C IP block on OMAP2430? It also has FIFO
transmit capability. It would be ideal if you can find out from the I2C
IP block designers. If you cannot, please consider adding a comment that
this may also apply to the I2C block on OMAP2430.
In general it is best to enable these workarounds based on the I2C IP
block's own revision register contents, not the OMAP CPU type. The goal
is to remove all these OMAP-specific cpu_is_omap() macros from
device drivers. For example, what if a future DaVinci part uses the same
I2C IP block?
Yes this is the right way.
I am checking with the IP team and will get back on this action item.
+ while (!(stat
OMAP_I2C_STAT_XUDF)) {
Is there a reason why you can't just reuse the main while() loop in the
ISR, and add a state variable to handle any special casing needed in this
context? That will avoid this separate while() loop.
The problem with using the main while() loop is the counter 'count' associated
with it as I am not sure if the count value of 100 is enough wait time for
allowing the XUDF bit to set and if we can come up with an accurate wait count
to be used there.
The idea is that if the hardware is functional, XUDF bit will be set once the
FIFO and shift registers
[PATCH 0/3] [OMAP:I2C] Small bug fixes and errata 1.153
This patch includes the following: - Bug in reading the RXSTAT/TXSTAT values from I2C_BUFFSTAT - In case of a NACK|ARDY|AL interrupts return from the ISR - OMAP3430 silicon errata 1.153 Signed-off-by: Moiz Sonasathm-sonas...@ti.com Signed-off-by: Vikram panditavikram.pand...@ti.com Moiz Sonasath (3): [OMAP:I2C]Bug in reading the RXSTAT/TXSTAT values from the I2C_BUFFSTAT register [OMAP:I2C]In case of a NACK|ARDY|AL return from the ISR [OMAP:I2C]OMAP3430 Silicon Errata 1.153 drivers/i2c/busses/i2c-omap.c | 42 +--- 1 files changed, 34 insertions(+), 8 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] [OMAP:I2C]Bug in reading the RXSTAT/TXSTAT values from the I2C_BUFFSTAT register
Fix bug in reading the I2C_BUFFSTAT register for getting byte count on RX/TX
interrupt.
On Interrupt: I2C_STAT[RDR],
read 'RXSTAT' from I2C_BUFFSTAT[8-13]
On Interrupt: I2C_STAT[XDR]
read 'TXSTAT' from I2C_BUFFSTAT[0-5]
Signed-off-by: Jagadeesh Pakaravoorj-pakarav...@ti.com
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 14 --
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..d280acf 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -692,9 +692,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
if (dev-fifo_size) {
if (stat OMAP_I2C_STAT_RRDY)
num_bytes = dev-fifo_size;
- else
- num_bytes = omap_i2c_read_reg(dev,
- OMAP_I2C_BUFSTAT_REG);
+ else/* read RXSTAT on RDR interrupt */
+ num_bytes = (omap_i2c_read_reg(dev,
+ OMAP_I2C_BUFSTAT_REG)
+8) 0x3F;
}
while (num_bytes) {
num_bytes--;
@@ -731,9 +732,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
if (dev-fifo_size) {
if (stat OMAP_I2C_STAT_XRDY)
num_bytes = dev-fifo_size;
- else
- num_bytes = omap_i2c_read_reg(dev,
- OMAP_I2C_BUFSTAT_REG);
+ else/* read TXSTAT on XDR interrupt */
+ num_bytes = omap_i2c_read_reg(dev,
+ OMAP_I2C_BUFSTAT_REG)
+0x3F;
}
while (num_bytes) {
num_bytes--;
--
1.5.6.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] [OMAP:I2C]In case of a NACK|ARDY|AL return from the ISR
In case of a NACK or ARDY or AL interrupt, complete the request.
There is no need to service the RRDY/RDR or XRDY/XDR interrupts.
Refer TRM SWPU114: Figure 18-31.I2C Master Transmitter Mode, Interrupt Method,
in F/S and HS Modes
http://focus.ti.com/pdfs/wtbu/SWPU114T_PrelimFinalEPDF_06_25_2009.pdf
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d280acf..05b5e4c 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -685,8 +685,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
err |= OMAP_I2C_STAT_AL;
}
if (stat (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
- OMAP_I2C_STAT_AL))
+ OMAP_I2C_STAT_AL)) {
omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
+ }
if (stat (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
u8 num_bytes = 1;
if (dev-fifo_size) {
--
1.5.6.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.
Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
interrupts and return from the ISR.
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 24 +++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 05b5e4c..8deaf87 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
break;
}
+ err = 0;
+complete:
omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
- err = 0;
if (stat OMAP_I2C_STAT_NACK) {
err |= OMAP_I2C_STAT_NACK;
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
data to send\n);
break;
}
+
+ /*
+* OMAP3430 Errata 1.153: When an XRDY/XDR
+* is hit, wait for XUDF before writing data
+* to DATA_REG. Otherwise some data bytes can
+* be lost while transferring them from the
+* memory to the I2C interface.
+*/
+
+ if (cpu_is_omap34xx()) {
+ while (!(stat
OMAP_I2C_STAT_XUDF)) {
+ if (stat
(OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+
omap_i2c_ack_stat(dev, stat (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
+ err |=
OMAP_I2C_STAT_XUDF;
+ goto complete;
+ }
+ cpu_relax();
+ stat =
omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ }
+ }
+
omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
}
omap_i2c_ack_stat(dev,
--
1.5.6.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Sonasath, Moiz wrote:
When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.
Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
interrupts and return from the ISR.
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram Panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 24 +++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-
omap.c
index 05b5e4c..8deaf87 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
break;
}
+ err = 0;
+complete:
cant we rename this label?
[Moiz]
This path actually completes the IRQ service routine and therefore the
name.
omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
- err = 0;
if (stat OMAP_I2C_STAT_NACK) {
err |= OMAP_I2C_STAT_NACK;
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
data to send\n);
break;
}
+
+ /*
+* OMAP3430 Errata 1.153: When an XRDY/XDR
+* is hit, wait for XUDF before writing data
+* to DATA_REG. Otherwise some data bytes can
+* be lost while transferring them from the
+* memory to the I2C interface.
+*/
+
+ if (cpu_is_omap34xx()) {
+ while (!(stat
OMAP_I2C_STAT_XUDF)) {
+ if (stat
(OMAP_I2C_STAT_NACK |
OMAP_I2C_STAT_AL)) {
+
omap_i2c_ack_stat(dev,
stat (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
generic comment - code nesting is getting overwhelming - we may like to
refactor the isr function at a later date I think..
+ err |=
OMAP_I2C_STAT_XUDF;
why set the err if we wantedly force this to happen?
[Moiz]
The idea here is to let the upper application to know that something went
wrong while waiting for the XUDF bit to be set. This is just for debugging
purpose.
+ goto complete;
+ }
+ cpu_relax();
+ stat =
omap_i2c_read_reg(dev,
OMAP_I2C_STAT_REG);
+ }
this is an infinite while loop + it tries to handle error cases -
essentially do another isr routine inside itself.
[Moiz]
Yes, it is a busy wait loop. But AFAIK while we come to this part of the
code the only thing that can go wrong in the transfer is either the device
would go off suddenly (creating a NACK) or there is an arbitration
lost(AL). This loop takes care of these two error conditions. Apart from
these, if the hardware is functional, the XUDF bit should be set as soon
as the FIFO gets empty.
Correct me if I am missing something.
As of now I am posting my patch without a timeout, later as per the need I will
optimize it with a timeout approach.
How about:
Apply [1] and the following?
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..e3f21af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
}
if (stat (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
u8 num_bytes = 1;
+ if (cpu_is_omap34xx()
+ !(stat OMAP_I2C_STAT_XUDF)){
+ cpu_relax();
+ continue;
+ }
+
if (dev-fifo_size) {
if (stat OMAP_I2C_STAT_XRDY)
num_bytes = dev-fifo_size;
Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Hello Nishant, Comments inlined, Regards Moiz Sonasath -Original Message- From: Menon, Nishanth Sent: Wednesday, July 15, 2009 6:03 PM To: Sonasath, Moiz Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 Sonasath, Moiz had written, on 07/15/2009 05:37 PM, the following: -Original Message- From: Menon, Nishanth Sent: Wednesday, July 15, 2009 5:32 PM please stop top posting.. Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following: I am also not sure, if the count=100; value will be enough time for the XUDF to be set. If not then it will keep running into timeout errors. Do you mean we need a delay for checking again? that should be easy to incorporate - what kind of delay are we speaking of here? do you have a count requirement for handling this? it is essentially the time b/w XRDY to XUNDF.. this should be deterministic rt? AFAIK, the time between XRDY/XDR and setting of XUDF bit is not deterministic, it depends on the I2C bus speed but I am not sure if we can translate the speed into a fixed count number which we can use as a timeout limit. In that case we need to make a balanced assumption of the count value so that we don't fall in the timeout case under normal operation. May be someone can give a pointer here. Here is my attempt at this: XRDY - in this case the FIFO is completely empty - fill it up for XTRSH XDR - FIFO is not completely empty, fill as per TXSTAT reg. if you look at the sequences of events that should happen - a) no previous data transmitted: XRDY, XUNDF b) previous data available, XDR, XRDY, XUNDF The variables are: i) bus speed ii) num bytes to empty (if XRDY then 0, else TXSTAT) Time = (num_bytes+c) * (1/bus_speed) where c = some constant time interval for XRDY-XUNDF.. I think.. One possible strategy: a)Worst case is XTRSH, so, Compute time prior to entry into transmit loop. Set the count as this+ add a constant for additional events b) Add a constant delay when you decide to continue back - not sure if cpu_relax is predictable delay.. [Moiz] Let me just rephrase what you said and from what I see in the TRM, XRDY- when TX FIFO level is equal/below XTRSH and TXSTAT is equal/greater than XTRSH, LH will write number of data bytes specified by XTRSH XDR- when TX FIFO level is equal/below the XTRSH and TXSTAT is less than XTRSH, LH will write number of data bytes specified by TXSTAT XUDF- when shift register and the TX FIFO are empty and there are still some bytes to transmit (DCOUNT not 0) Here we are looking at the wait time between XRDY/XDR and XUDF. Considering worst case scenarios, Wait_time = (time to transmit out XTRSH bytes from the FIFO at the slowest bus speed) + (safeguard delay) The slowest bus speed appears in standard mode (100K bits/sec). Therefore, Wait_time = [(XTRSH*8)*(1/100K)] seconds + constant; Does this look correct? -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/3] [OMAP:I2C]In case of a NACK|ARDY|AL return from the ISR
Hello Nishant,
Comments inlined:
Regards
Moiz Sonasath
Linux Baseport Team, Dallas
214-567-5514
-Original Message-
From: Menon, Nishanth
Sent: Tuesday, July 14, 2009 5:22 PM
To: Sonasath, Moiz; linux-omap@vger.kernel.org
Cc: Kamat, Nishant; Paul Walmsley
Subject: RE: [PATCH 2/3] [OMAP:I2C]In case of a NACK|ARDY|AL return from the ISR
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Sonasath, Moiz
Sent: Tuesday, July 14, 2009 4:20 PM
To: linux-omap@vger.kernel.org
Cc: Kamat, Nishant; Paul Walmsley
Subject: [PATCH 2/3] [OMAP:I2C]In case of a NACK|ARDY|AL return from the
ISR
In case of a NACK or ARDY or AL interrupt, complete the request.
There is no need to service the RRDY/RDR or XRDY/XDR interrupts.
Refer TRM SWPU114: Figure 18-31.I2C Master Transmitter Mode, Interrupt
Method,
in F/S and HS Modes
http://focus.ti.com/pdfs/wtbu/SWPU114T_PrelimFinalEPDF_06_25_2009.pdf
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram Panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d280acf..05b5e4c 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -685,8 +685,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
err |= OMAP_I2C_STAT_AL;
}
if (stat (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
- OMAP_I2C_STAT_AL))
+ OMAP_I2C_STAT_AL)) {
omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
+ }
What is the status of IRQ_STAT register? Who will clear that if we return
immediately from ISR? Isr will be reinvoked and we'd handle the same anyways..
[Moiz]
Correct me if I am wrong, but you are referring to the OMAP_I2C_STAT_REG (which
has the interrupt status information). This is actually ACKED just before going
into the NACK|AL|ARDY error cases. I suppose a few days back you also sent out
a patch to change this to ACK all interrupts other than RRDY/RDR and XRDY/XDR
as we have to ACK these only after we service them.
I think this conflicts with [1]
[Moiz]
Actually, it does not conflict with [1], as [1] changes the later part of the
code allowing this patch to be applied cleanly.
Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Hello Nishant,
Comments inlined:
Regards
Moiz Sonasath
Linux Baseport Team, Dallas
214-567-5514
-Original Message-
From: Menon, Nishanth
Sent: Tuesday, July 14, 2009 6:23 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Sonasath, Moiz wrote:
When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.
Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
interrupts and return from the ISR.
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram Panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 24 +++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 05b5e4c..8deaf87 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
break;
}
+ err = 0;
+complete:
cant we rename this label?
[Moiz]
This path actually completes the IRQ service routine and therefore the name.
omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
- err = 0;
if (stat OMAP_I2C_STAT_NACK) {
err |= OMAP_I2C_STAT_NACK;
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
data to send\n);
break;
}
+
+ /*
+ * OMAP3430 Errata 1.153: When an XRDY/XDR
+ * is hit, wait for XUDF before writing data
+ * to DATA_REG. Otherwise some data bytes can
+ * be lost while transferring them from the
+ * memory to the I2C interface.
+ */
+
+ if (cpu_is_omap34xx()) {
+ while (!(stat
OMAP_I2C_STAT_XUDF)) {
+ if (stat
(OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+
omap_i2c_ack_stat(dev, stat (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
generic comment - code nesting is getting overwhelming - we may like to
refactor the isr function at a later date I think..
+ err |=
OMAP_I2C_STAT_XUDF;
why set the err if we wantedly force this to happen?
[Moiz]
The idea here is to let the upper application to know that something went wrong
while waiting for the XUDF bit to be set. This is just for debugging purpose.
+ goto complete;
+ }
+ cpu_relax();
+ stat =
omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ }
this is an infinite while loop + it tries to handle error cases -
essentially do another isr routine inside itself.
[Moiz]
Yes, it is a busy wait loop. But AFAIK while we come to this part of the code
the only thing that can go wrong in the transfer is either the device would go
off suddenly (creating a NACK) or there is an arbitration lost(AL). This loop
takes care of these two error conditions. Apart from these, if the hardware is
functional, the XUDF bit should be set as soon as the FIFO gets empty.
Correct me if I am missing something.
How about:
Apply [1] and the following?
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..e3f21af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
}
if (stat (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
u8 num_bytes = 1;
+ if (cpu_is_omap34xx()
+ !(stat OMAP_I2C_STAT_XUDF)){
+ cpu_relax();
+ continue;
+ }
+
if (dev-fifo_size) {
if (stat OMAP_I2C_STAT_XRDY)
num_bytes = dev-fifo_size;
Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/
--
To unsubscribe from this list: send the line unsubscribe linux-omap
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Hello Nishant,
Comments inlined,
Regards
Moiz Sonasath
-Original Message-
From: Menon, Nishanth
Sent: Wednesday, July 15, 2009 4:43 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following:
When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.
Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
+}
+cpu_relax();
+stat =
omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+}
this is an infinite while loop + it tries to handle error cases -
essentially do another isr routine inside itself.
[Moiz]
Yes, it is a busy wait loop. But AFAIK while we come to this part of
the code the only thing that can go wrong in the transfer is either
the device would go off suddenly (creating a NACK) or there is an
arbitration lost(AL). This loop takes care of these two error conditions.
Apart from these, if the hardware is functional, the XUDF bit should
be set as soon as the FIFO gets empty.
Correct me if I am missing something.
That is exactly what I was complaining about - why not use the existing
logic above in the code to take care of it as I indicated below? do you
see a problem with the logic I send? it is lesser code and should IMHO
take care of the same issue without the additional 3rd nested while loop
How about:
Apply [1] and the following?
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..e3f21af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
}
if (stat (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
u8 num_bytes = 1;
+ if (cpu_is_omap34xx()
+ !(stat OMAP_I2C_STAT_XUDF)){
+ cpu_relax();
+ continue;
+ }
+
[Moiz]
In this alternate implementation if a NACK|AL is produced while waiting on the
XUDF, it returns from the ISR (as per my patch [2/3]) without ACKING the
XRDY/XDR interrupts which would make it return to the ISR again and again which
looks like a problem. To accommodate this implementation, we will have to ACK
XRDY/XDR before returning from the ISR.
if (dev-fifo_size) {
if (stat OMAP_I2C_STAT_XRDY)
num_bytes = dev-fifo_size;
Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Hello Nishant!
I am also not sure, if the count=100; value will be enough time for the XUDF to
be set. If not then it will keep running into timeout errors.
Regards
Moiz Sonasath
-Original Message-
From: Menon, Nishanth
Sent: Wednesday, July 15, 2009 5:27 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Sonasath, Moiz had written, on 07/15/2009 05:23 PM, the following:
Hello Nishant,
Comments inlined,
Regards
Moiz Sonasath
-Original Message-
From: Menon, Nishanth
Sent: Wednesday, July 15, 2009 4:43 PM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following:
When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.
Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
+ }
+ cpu_relax();
+ stat =
omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ }
this is an infinite while loop + it tries to handle error cases -
essentially do another isr routine inside itself.
[Moiz]
Yes, it is a busy wait loop. But AFAIK while we come to this part of
the code the only thing that can go wrong in the transfer is either
the device would go off suddenly (creating a NACK) or there is an
arbitration lost(AL). This loop takes care of these two error conditions.
Apart from these, if the hardware is functional, the XUDF bit should
be set as soon as the FIFO gets empty.
Correct me if I am missing something.
That is exactly what I was complaining about - why not use the existing
logic above in the code to take care of it as I indicated below? do you
see a problem with the logic I send? it is lesser code and should IMHO
take care of the same issue without the additional 3rd nested while loop
How about:
Apply [1] and the following?
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..e3f21af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
}
if (stat (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
u8 num_bytes = 1;
+ if (cpu_is_omap34xx()
+ !(stat OMAP_I2C_STAT_XUDF)){
+ cpu_relax();
+ continue;
+ }
+
[Moiz]
In this alternate implementation if a NACK|AL is produced while waiting
on the XUDF, it returns from the ISR (as per my patch [2/3])
without ACKING the XRDY/XDR interrupts which would make it return
to the ISR again and again which looks like a problem. To accommodate
this implementation, we will have to ACK XRDY/XDR before returning
from the ISR.
ok, this is due to my [1] patch which should have handled this condition
in the first place. I will rework my original [1] patch and resend it.
Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
Nishant, AFAIK, the time between XRDY/XDR and setting of XUDF bit is not deterministic, it depends on the I2C bus speed but I am not sure if we can translate the speed into a fixed count number which we can use as a timeout limit. In that case we need to make a balanced assumption of the count value so that we don't fall in the timeout case under normal operation. May be someone can give a pointer here. Regards Moiz Sonasath -Original Message- From: Menon, Nishanth Sent: Wednesday, July 15, 2009 5:32 PM To: Sonasath, Moiz Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following: I am also not sure, if the count=100; value will be enough time for the XUDF to be set. If not then it will keep running into timeout errors. Do you mean we need a delay for checking again? that should be easy to incorporate - what kind of delay are we speaking of here? do you have a count requirement for handling this? it is essentially the time b/w XRDY to XUNDF.. this should be deterministic rt? Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] [OMAP:I2C] Small bug fixes and errata 1.153
This patch includes the following: - Bug in reading the RXSTAT/TXSTAT values from I2C_BUFFSTAT - In case of a NACK|ARDY|AL interrupts return from the ISR - OMAP3430 silicon errata 1.153 Signed-off-by: Moiz Sonasathm-sonas...@ti.com Signed-off-by: Vikram panditavikram.pand...@ti.com Moiz Sonasath (3): [OMAP:I2C]Bug in reading the RXSTAT/TXSTAT values from the I2C_BUFFSTAT register [OMAP:I2C]In case of a NACK|ARDY|AL return from the ISR [OMAP:I2C]OMAP3430 Silicon Errata 1.153 drivers/i2c/busses/i2c-omap.c | 42 +--- 1 files changed, 34 insertions(+), 8 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] [OMAP:I2C]Bug in reading the RXSTAT/TXSTAT values from the I2C_BUFFSTAT register
Fix bug in reading the I2C_BUFFSTAT register for getting byte count on RX/TX
interrupt.
On Interrupt: I2C_STAT[RDR],
read 'RXSTAT' from I2C_BUFFSTAT[8-13]
On Interrupt: I2C_STAT[XDR]
read 'TXSTAT' from I2C_BUFFSTAT[0-5]
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Jagadeesh Pakaravoorj-pakarav...@ti.com
Signed-off-by: Vikram anditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 14 --
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..d280acf 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -692,9 +692,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
if (dev-fifo_size) {
if (stat OMAP_I2C_STAT_RRDY)
num_bytes = dev-fifo_size;
- else
- num_bytes = omap_i2c_read_reg(dev,
- OMAP_I2C_BUFSTAT_REG);
+ else/* read RXSTAT on RDR interrupt */
+ num_bytes = (omap_i2c_read_reg(dev,
+ OMAP_I2C_BUFSTAT_REG)
+8) 0x3F;
}
while (num_bytes) {
num_bytes--;
@@ -731,9 +732,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
if (dev-fifo_size) {
if (stat OMAP_I2C_STAT_XRDY)
num_bytes = dev-fifo_size;
- else
- num_bytes = omap_i2c_read_reg(dev,
- OMAP_I2C_BUFSTAT_REG);
+ else/* read TXSTAT on XDR interrupt */
+ num_bytes = (omap_i2c_read_reg(dev,
+ OMAP_I2C_BUFSTAT_REG))
+0x3F;
}
while (num_bytes) {
num_bytes--;
--
1.5.6.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] [OMAP:I2C]In case of a NACK|ARDY|AL return from the ISR
In case of a NACK or ARDY or AL interrupt, complete the request.
There is no need to service the RRDY/RDR or XRDY/XDR interrupts.
Refer TRM SWPU114: Figure 18-31.I2C Master Transmitter Mode, Interrupt Method,
in F/S and HS Modes
http://focus.ti.com/pdfs/wtbu/SWPU114T_PrelimFinalEPDF_06_25_2009.pdf
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram Panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d280acf..05b5e4c 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -685,8 +685,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
err |= OMAP_I2C_STAT_AL;
}
if (stat (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
- OMAP_I2C_STAT_AL))
+ OMAP_I2C_STAT_AL)) {
omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
+ }
if (stat (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
u8 num_bytes = 1;
if (dev-fifo_size) {
--
1.5.6.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153
When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
Otherwise some data bytes can be lost while transferring them from the
memory to the I2C interface.
Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
if there is NACK | AL, set the appropriate error flags, ack the pending
interrupts and return from the ISR.
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
Signed-off-by: Vikram Panditavikram.pand...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 24 +++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 05b5e4c..8deaf87 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
break;
}
+ err = 0;
+complete:
omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
- err = 0;
if (stat OMAP_I2C_STAT_NACK) {
err |= OMAP_I2C_STAT_NACK;
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
data to send\n);
break;
}
+
+ /*
+* OMAP3430 Errata 1.153: When an XRDY/XDR
+* is hit, wait for XUDF before writing data
+* to DATA_REG. Otherwise some data bytes can
+* be lost while transferring them from the
+* memory to the I2C interface.
+*/
+
+ if (cpu_is_omap34xx()) {
+ while (!(stat
OMAP_I2C_STAT_XUDF)) {
+ if (stat
(OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+
omap_i2c_ack_stat(dev, stat (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
+ err |=
OMAP_I2C_STAT_XUDF;
+ goto complete;
+ }
+ cpu_relax();
+ stat =
omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ }
+ }
+
omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
}
omap_i2c_ack_stat(dev,
--
1.5.6.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
Hello Paul!
I am working on this issue and will get back soon.
Regards
Moiz Sonasath
Linux Baseport Team, Dallas
214-567-5514
-Original Message-
From: Paul Walmsley [mailto:p...@pwsan.com]
Sent: Wednesday, July 08, 2009 11:51 AM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Pakaravoor, Jagadeesh; Kamat, Nishant; Pandita,
Vikram
Subject: Re: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata
1.153
Hello Moiz, Nishant, Jagadeesh,
Any plans to respond to these comments?
- Paul
On Mon, 22 Jun 2009, Paul Walmsley wrote:
Moiz, Nishant, Jagadeesh,
Looks like this patch has some serious problems.
On Mon, 22 Jun 2009, Sonasath, Moiz wrote:
This patch includes the workarround for I2C Errata 1.153: When an
XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG
Signed-off-by: Jagadeesh Bhaskar Pakaravoor j-pakarav...@ti.com
Signed-off by: Nishant Kamat nska...@ti.com
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 54
+---
1 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ece0125..e84836b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -632,6 +632,37 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
#define omap_i2c_rev1_isr NULL
#endif
+/* I2C Errata 1.153:
+ * When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
+ * Otherwise some data bytes can be lost while transferring them from the
+ * memory to the I2C interface.
+ */
+
+static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev)
+{
+ u16 xudf;
+ int counter = 500;
Shouldn't the timeout threshold depend on the I2C bus speed and transmit
FIFO threshold?
+
+ /* We are in interrupt context. Wait for XUDF for max 7 msec */
There's no way an interrupt service routine should loop for up to 7
milliseconds. Why does it need to wait this long? Shouldn't this patch
zero the transmit FIFO threshold to minimize the amount of time between
XRDY/XDR and XUDF?
+ xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ while (!(xudf OMAP_I2C_STAT_XUDF) counter--) {
Why is a separate I2C_STAT loop used here? Shouldn't this patch just use
the existing I2C_STAT loop in the ISR and use a state variable to indicate
the current state of the ISR?
+ if (xudf (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_NACK |
+ OMAP_I2C_STAT_AL))
+ return -EINVAL;
+ udelay(10);
udelay() in an ISR is a big red flag. Why is this needed?
+ xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ }
+
+ if (!counter) {
+ /* Clear Tx FIFO */
+ omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
+ OMAP_I2C_BUF_TXFIF_CLR);
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
static irqreturn_t
omap_i2c_isr(int this_irq, void *dev_id)
{
@@ -639,6 +670,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
u16 bits;
u16 stat, w;
int err, count = 0;
+ int error;
if (dev-idle)
return IRQ_NONE;
@@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) bits) {
dev_dbg(dev-dev, IRQ (ISR = 0x%04x)\n, stat);
if (count++ == 100) {
- dev_warn(dev-dev, Too much work in one IRQ\n);
+ dev_dbg(dev-dev, Too much work in one IRQ\n);
break;
}
@@ -715,11 +747,22 @@ omap_i2c_isr(int this_irq, void *dev_id)
OMAP_I2C_BUFSTAT_REG);
}
while (num_bytes) {
- num_bytes--;
w = 0;
if (dev-buf_len) {
+ if (cpu_is_omap34xx()) {
+ /* OMAP3430 Errata 1.153 */
What about this I2C IP block on previous chip versions? Is this problem
also present on 2430, which also has FIFO transmit capability?
+ error =
omap_i2c_wait_for_xudf(dev);
+ if (error) {
+ omap_i2c_ack_stat(dev,
stat
+
(OMAP_I2C_STAT_XRDY |
+
OMAP_I2C_STAT_XDR));
+ dev_err(dev-dev,
Transmit error\n);
+
omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_XUDF
[PATCH] [RFC] Potential bug in defining 'irq field' of 'ifmap structure'
From: Moiz Sonasath m-sonas...@ti.com There seems to be a bug in the ioctl implementation in /kernel/net/core/dev.c dev_ifsioc_locked() case SIOCGIFMAP: ifr-ifr_map.irq = dev-irq; // ?? type mismatch Here ifr-ifr_map.irq) is of type unsigned char dev-irq is of type unsigned int So ifconfig reports a wrong irq number when the dev-irq number is 255. I am confused to see the same typedefs in file: net/if.h Not sure how to make changes for the user side net/if.h file? Signed-off-by: Moiz Sonasath m-sonas...@ti.com Signed-off-by: Vikram Pandita vikram.pand...@ti.com --- include/linux/if.h |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/include/linux/if.h b/include/linux/if.h index 2d89c96..1ac6559 100644 --- a/include/linux/if.h +++ b/include/linux/if.h @@ -126,7 +126,7 @@ struct ifmap unsigned long mem_start; unsigned long mem_end; unsigned short base_addr; - unsigned char irq; + unsigned int irq; unsigned char dma; unsigned char port; /* 3 bytes spare */ -- 1.5.6.3 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
This patch includes the workarround for I2C Errata 1.153: When an XRDY/XDR is
hit, wait for XUDF before writing data to DATA_REG
Signed-off-by: Jagadeesh Bhaskar Pakaravoor j-pakarav...@ti.com
Signed-off by: Nishant Kamat nska...@ti.com
Signed-off-by: Moiz Sonasathm-sonas...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 54 +---
1 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ece0125..e84836b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -632,6 +632,37 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
#define omap_i2c_rev1_isr NULL
#endif
+/* I2C Errata 1.153:
+ * When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
+ * Otherwise some data bytes can be lost while transferring them from the
+ * memory to the I2C interface.
+ */
+
+static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev)
+{
+ u16 xudf;
+ int counter = 500;
+
+ /* We are in interrupt context. Wait for XUDF for max 7 msec */
+ xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ while (!(xudf OMAP_I2C_STAT_XUDF) counter--) {
+ if (xudf (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_NACK |
+ OMAP_I2C_STAT_AL))
+ return -EINVAL;
+ udelay(10);
+ xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ }
+
+ if (!counter) {
+ /* Clear Tx FIFO */
+ omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
+ OMAP_I2C_BUF_TXFIF_CLR);
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
static irqreturn_t
omap_i2c_isr(int this_irq, void *dev_id)
{
@@ -639,6 +670,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
u16 bits;
u16 stat, w;
int err, count = 0;
+ int error;
if (dev-idle)
return IRQ_NONE;
@@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) bits) {
dev_dbg(dev-dev, IRQ (ISR = 0x%04x)\n, stat);
if (count++ == 100) {
- dev_warn(dev-dev, Too much work in one IRQ\n);
+ dev_dbg(dev-dev, Too much work in one IRQ\n);
break;
}
@@ -715,11 +747,22 @@ omap_i2c_isr(int this_irq, void *dev_id)
OMAP_I2C_BUFSTAT_REG);
}
while (num_bytes) {
- num_bytes--;
w = 0;
if (dev-buf_len) {
+ if (cpu_is_omap34xx()) {
+ /* OMAP3430 Errata 1.153 */
+ error =
omap_i2c_wait_for_xudf(dev);
+ if (error) {
+ omap_i2c_ack_stat(dev,
stat
+
(OMAP_I2C_STAT_XRDY |
+
OMAP_I2C_STAT_XDR));
+ dev_err(dev-dev,
Transmit error\n);
+
omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_XUDF);
+
+ return IRQ_HANDLED;
+ }
+ }
w = *dev-buf++;
- dev-buf_len--;
/* Data reg from 2430 is 8 bit wide */
if (!cpu_is_omap2430()
!cpu_is_omap34xx()) {
@@ -728,6 +771,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
dev-buf_len--;
}
}
+ omap_i2c_write_reg(dev,
+ OMAP_I2C_DATA_REG, w);
+ num_bytes--;
+ dev-buf_len--;
} else {
if (stat OMAP_I2C_STAT_XRDY)
dev_err(dev-dev,
@@ -739,7 +786,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
data to send\n);
break;
}
- omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
}
