Re: [PATCH V2] mmc: omap_hsmmc: Fix sleep too long in ISR context.
Hi,
[ added some people from TI ]
On 8/7/2013 6:05 PM, majianpeng wrote:
V2:
clean up code.
V1:
www.mail-archive.com/linux-omap@vger.../msg93239.html‎
We found a problem when we removed a working sd card that the irqaction
of omap_hsmmc can sleep to 3.6s. This cause our watchdog to work.
In func omap_hsmmc_reset_controller_fsm, it should watch a 0-1
transition.But avoiding endless waiting, it used loops_per_jiffy as the timer.
Tried on a OMAP4460:
This can easily be replicated: just withdraw an SD-card and the kernel
will get blocked during more than 3 seconds.
Calling OMAP_HSMMC_READ() in this loop makes it last so long.
The function waits for a 0=1, followed by a 1=0 transition.
The value of 1 always comes, but in most cases the code is just too
slow to detect it. The first loop will only stop when (i == limit)
The code is:
while ((!(OMAP_HSMMC_READ(host-base, SYSCTL) bit))
(i++ limit))
cpu_relax();
But generanly loops_per_jiffy as a timer,it should like:
while(i++ limit)
cpu_relax();
I found for the long time case, the while-opeation stoped because 'i ==
limit'.
Because added some code, so the duration became too longer than
MMC_TIMEOU_US(20ms).
The software can't monitor the transition of hardware for thi case.
Becasue those codes in ISR context, it can't use timer_before/after.
I divived the time into 1ms and used udelay(1) to instead.
It will cause do additional udelay(1).But from my test,it looks good.
Reported-by: Yuzheng Ma mayuzh...@kedacom.com
Tested-by: Yuzheng Ma mayuzh...@kedacom.com
Reviewed-by: Hein Tibosch hein_tibo...@yahoo.es
Signed-off-by: Jianpeng Ma majianp...@gmail.com
---
drivers/mmc/host/omap_hsmmc.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 1865321..bbda5ed 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -973,9 +973,8 @@ static inline void omap_hsmmc_dbg_report_irq(struct
omap_hsmmc_host *host,
static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host
*host,
unsigned long bit)
{
- unsigned long i = 0;
- unsigned long limit = (loops_per_jiffy *
- msecs_to_jiffies(MMC_TIMEOUT_MS));
+ /*change to 1ms,so we can use udelay(1)*/
+ unsigned long limit = MMC_TIMEOUT_MS * 1000;
OMAP_HSMMC_WRITE(host-base, SYSCTL,
OMAP_HSMMC_READ(host-base, SYSCTL) | bit);
Checked here: the SRC-bit indeed becomes high.
After the test 'features HSMMC_HAS_UPDATED_RESET', the bit has
become low again already.
Changing to order of statements worked for me, but for Jianpeng Ma
this didn't work (timings are 'on the edge').
This reset function is also called while mounting a card and then 'SRC'
will be high long enough (more than 100 loops). It is after withdrawing
the card that the reset is performed extremely fast.
@@ -984,17 +983,15 @@ static inline void
omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
* OMAP4 ES2 and greater has an updated reset logic.
* Monitor a 0-1 transition first
*/
- if (mmc_slot(host).features HSMMC_HAS_UPDATED_RESET) {
- while ((!(OMAP_HSMMC_READ(host-base, SYSCTL) bit))
- (i++ limit))
- cpu_relax();
- }
- i = 0;
-
- while ((OMAP_HSMMC_READ(host-base, SYSCTL) bit)
- (i++ limit))
- cpu_relax();
-
+ if (mmc_slot(host).features HSMMC_HAS_UPDATED_RESET)
+ while (!(OMAP_HSMMC_READ(host-base, SYSCTL) bit)
+ limit--)
+ udelay(1);
In this way, the loop will only last about 'MMC_TIMEOUT_MS' ms
and my WDT doesn't get triggered :-)
+ limit = MMC_TIMEOUT_MS * 1000;
+ while ((OMAP_HSMMC_READ(host-base, SYSCTL) bit) limit--)
+ udelay(1);
+
if (OMAP_HSMMC_READ(host-base, SYSCTL) bit)
dev_err(mmc_dev(host-mmc),
Timeout waiting on controller reset in %s\n,
Anybody an opinion on this?
Thanks, Hein
--
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] mmc: omap_hsmmc: Fix sleep too long in ISR context.
Hi Jianpeng Ma,
On 8/1/2013 10:18 AM, majianpeng wrote:
We found a problem when we removed a working sd card that the irqaction
of omap_hsmmc can sleep to 3.6s. This cause our watchdog to work.
In func omap_hsmmc_reset_controller_fsm, it should watch a 0-1
transition.It used loops_per_jiffy as the timer.
The code is:
while ((!(OMAP_HSMMC_READ(host-base, SYSCTL) bit))
(i++ limit))
cpu_relax();
But the loops_per_jiffy is:
while(i++ limit)
cpu_relax();
It add some codes so the time became long.
Becasue those codes in ISR context, it can't use timer_before/after.
I divived the time into 1ms and used udelay(1) to instead.
It will cause do additional udelay(1).But from my test,it looks good.
Reported-by: Yuzheng Ma mayuzh...@kedacom.com
Tested-by: Yuzheng Ma mayuzh...@kedacom.com
Signed-off-by: Jianpeng Ma majianp...@gmail.com
---
drivers/mmc/host/omap_hsmmc.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 1865321..96daca1 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -977,6 +977,8 @@ static inline void omap_hsmmc_reset_controller_fsm(struct
omap_hsmmc_host *host,
unsigned long limit = (loops_per_jiffy *
msecs_to_jiffies(MMC_TIMEOUT_MS));
+ /*Divided time into us for unit 1,we can use udelay(1)*/
+ i = limit / (MMC_TIMEOUT_MS * 1000);
'limit' is a number of loops, which you now divide by 20,000?
To get uS, you could just change:
- unsigned long limit = (loops_per_jiffy *
- msecs_to_jiffies(MMC_TIMEOUT_MS));
+ unsigned long limit = 1000 * MMC_TIMEOUT_MS;
and make this amount of loops using udelay().
OMAP_HSMMC_WRITE(host-base, SYSCTL,
OMAP_HSMMC_READ(host-base, SYSCTL) | bit);
@@ -985,15 +987,19 @@ static inline void
omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
* Monitor a 0-1 transition first
*/
if (mmc_slot(host).features HSMMC_HAS_UPDATED_RESET) {
- while ((!(OMAP_HSMMC_READ(host-base, SYSCTL) bit))
- (i++ limit))
- cpu_relax();
I still don't see why any of these loops could last 3.6 seconds?
Yes the __raw_readl() will add some time, but so much?
I'd like to see which value you get for 'limit' on your machine
Would PM play a role? Or cpu-freq, and 'loops_per_jiffy' isn't updated
on time?
+ while (i--) {
+ if ((OMAP_HSMMC_READ(host-base, SYSCTL) bit))
+ break;
+ udelay(1);
In earlier threads, the use of udelay was disliked because it's a waste
of cpu cycles. The desired bit in SYSCTL will change, while udelay()
is still making many useless loops.
+ }
}
- i = 0;
- while ((OMAP_HSMMC_READ(host-base, SYSCTL) bit)
- (i++ limit))
- cpu_relax();
+ i = limit / (MMC_TIMEOUT_MS * 1000);
+ while (i--) {
+ if (!(OMAP_HSMMC_READ(host-base, SYSCTL) bit))
+ break;
+ udealy(1);
+ }
if (OMAP_HSMMC_READ(host-base, SYSCTL) bit)
dev_err(mmc_dev(host-mmc),
Hein
--
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] i2c-omap: always send stop after nack
Hi Vikram,
On a OMAP4460, i2c-bus-3:
A driver (lm75) is causing many 'timeout waiting for bus ready' errors.
SDA remains high (as it should), but SCL remains low after a NACK.
The bus becomes _unusable for other clients_.
While probing, lm75 writes a command, followed by a read + stop,
but the write command is NACK'd. The chip does accept other writes/reads,
it just refuses to ack invalid commands.
Can you tell me if the patch below would make any sense? Or is it the
responsibility of the client to reset the i2c_smbus?
In case of M_IGNORE_NAK there is no problem, a STOP will follow after the
last command.
Thanks, Hein
Signed-off-by: Hein Tibosch hein_tibo...@yahoo.es
---
drivers/i2c/busses/i2c-omap.c |8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e02f9e3..2f7a712 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -615,11 +615,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (dev-cmd_err OMAP_I2C_STAT_NACK) {
if (msg-flags I2C_M_IGNORE_NAK)
return 0;
- if (stop) {
- w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
- w |= OMAP_I2C_CON_STP;
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
- }
+ w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
+ w |= OMAP_I2C_CON_STP;
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
return -EREMOTEIO;
}
return -EIO;
--
1.7.9.5
--
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] i2c-omap: always send stop after nack
On 7/16/2013 5:03 PM, Felipe Balbi wrote: Hi, On Tue, Jul 16, 2013 at 04:19:35PM +0800, Hein Tibosch wrote: Hi Vikram, On a OMAP4460, i2c-bus-3: A driver (lm75) is causing many 'timeout waiting for bus ready' errors. SDA remains high (as it should), but SCL remains low after a NACK. The bus becomes _unusable for other clients_. While probing, lm75 writes a command, followed by a read + stop, but the write command is NACK'd. The chip does accept other writes/reads, it just refuses to ack invalid commands. Can you tell me if the patch below would make any sense? Or is it the responsibility of the client to reset the i2c_smbus? patch below breaks repeated start. Hi, No, after the NACK, no more commands are being processed, including a repeated start. omap_i2c_xfer() returns -EREMOTEIO without ever freeing the bus. The bus is left in an impossible state with SCL constantly low and all next commands (to different chips) will therefore get a -ETIMEDOUT With this patch, the bus will become idle again and new commands can be processed normally Hein -- 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] i2c-omap: always send stop after nack
Hi Grygorii, Filipe, On 7/16/2013 9:00 PM, Felipe Balbi wrote: On Tue, Jul 16, 2013 at 03:08:04PM +0300, Grygorii Strashko wrote: Hi Felipe, On 07/16/2013 02:27 PM, Felipe Balbi wrote: Hi, On Tue, Jul 16, 2013 at 02:01:11PM +0300, Grygorii Strashko wrote: On a OMAP4460, i2c-bus-3: A driver (lm75) is causing many 'timeout waiting for bus ready' errors. SDA remains high (as it should), but SCL remains low after a NACK. The bus becomes _unusable for other clients_. While probing, lm75 writes a command, followed by a read + stop, but the write command is NACK'd. The chip does accept other writes/reads, it just refuses to ack invalid commands. In case the NACK may not be ignored, I believe it is correct to break out of the loop and send a stop for 2 reasons: 1. all chips, including the target chip, will know that the current transaction is finished 2. to set CLK high again, which prevents numerous timeouts on subsequent transactions As a test I've set 'I2C_M_IGNORE_NAK' for all .detect messages sent by the lm75 driver. Now the chip (tmp105) will provide read data as expected after the _repeated start_, even though it NACK'd the preceding WR command. And also the detection trick does work now, addresses 4,5,6,7 return the same read data as was returned from the last valid address 2. Felipe: Which means your original patch starts to make a lot more sense. I wonder is this is really what we should be doing though - breaking out of the loop, I mean. So yes, we have to break the loop as the caller is not interested in processing any further commands. In i2c/algos/i2c-algo-bit.c, bit_xfer() works exactly the same way: break the loop (unless IGNORE_NAK) and call unconditionally i2c_stop(). It looks like TI's i2c-davinci will have the same problem as i2c-omap, and will need the same change. Grygorii, if you submit the patch, please add my Signed-off-by: Hein Tibosch hein_tibo...@yahoo.es cause you were earlier to notice and fix this problem. Hein -- 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: omapfb-main.c: check result of simple_strtoul
On 5/10/2012 4:23 PM, Tomi Valkeinen wrote:
On Wed, 2012-05-09 at 22:47 +0800, Hein Tibosch wrote:
fbnum = simple_strtoul(p, p, 10);
-if (p == param)
+if (p == start)
correct?
Yes, looks like a correct fix. I'll cook up a patch.
How did you encounter the bug? What was the outcome?
I was reading the source, adding some printk's to get more debugging.
It would fail to detect a syntax error like:
omapfb.vram=0:2M,:5M
If I may ask: why aren't mode and modes filled in:
/sys/devices/platform/omapfb/graphics/fb0 ?
Only modes will get an entry when a new mode becomes active.
I thought of adding something like this in hdmi.c:
+static void omap_to_fbmode (struct fb_videomode *fb_mode, const struct
hdmi_config *cfg)
+{
+ char name[64];
+ unsigned hsync;
+ unsigned vsync;
+ unsigned refresh;
+ snprintf (name, sizeof name, %d_%s_%d.%d,
+ cfg-cm.code,
+ cfg-cm.mode == HDMI_HDMI ? hdmi : dvi,
+ cfg-timings.x_res,
+ cfg-timings.y_res);
+
+ hsync = cfg-timings.hfp + cfg-timings.hsw + cfg-timings.hbp +
cfg-timings.x_res;
+ vsync = cfg-timings.vfp + cfg-timings.vsw + cfg-timings.vbp +
cfg-timings.y_res;
+ refresh = (unsigned) ((1000U * cfg-timings.pixel_clock + (hsync *
vsync-1) ) / (hsync * vsync));
+
+ fb_mode-name = kstrdup(name, GFP_KERNEL); /* optional */
+ fb_mode-refresh = refresh; /* optional */
+ fb_mode-xres = cfg-timings.x_res;
+ fb_mode-yres = cfg-timings.y_res;
+ fb_mode-pixclock = DIV_ROUND_UP (10UL,
cfg-timings.pixel_clock);
+ fb_mode-left_margin = cfg-timings.hfp;
+ fb_mode-right_margin = cfg-timings.hbp;
+ fb_mode-upper_margin = cfg-timings.vfp;
+ fb_mode-lower_margin = cfg-timings.vbp;
+ fb_mode-hsync_len = cfg-timings.hsw;
+ fb_mode-vsync_len = cfg-timings.vsw;
+ fb_mode-sync =
+ (cfg-timings.vsync_pol ? FB_SYNC_VERT_HIGH_ACT : 0) |
+ (cfg-timings.hsync_pol ? FB_SYNC_HOR_HIGH_ACT : 0);
+ fb_mode-vmode = 0;
+ fb_mode-flag = 0;
+}
+
+int omap2_add_video_modes (struct fb_info *fbi)
+{
+ struct fb_videomode fb_mode;
+ int i;
+ int count = 0;
+
+ INIT_LIST_HEAD(fbi-modelist);
+
+ for (i = 0; i ARRAY_SIZE(cea_timings); i++) {
+ omap_to_fbmode (fb_mode, cea_timings[i]);
+ fb_add_videomode(fb_mode, fbi-modelist);
+ count++;
+ }
+ for (i = 0; i ARRAY_SIZE(vesa_timings); i++) {
+ omap_to_fbmode (fb_mode, vesa_timings[i]);
+ fb_add_videomode(fb_mode, fbi-modelist);
+ count++;
+ }
+ return count;
+}
and call it from omapfb-main.c:
static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)
{
...
clear_fb_info(fbi);
fbdev-fbs[i] = fbi;
+ omap2_add_video_modes (fbi);
With this patch I get 34 working entries from cat modes:
U:1024x768p-59
U:1280x800p-67
U:1680x1050p-59
U:1400x1050p-59
U:1280x768p-59
U:1920x1080p-60
U:1366x768p-59
...
which makes testing a bit easier.
But maybe I'm reinventing some wheel?
Hein
--
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
omapfb-main.c: check result of simple_strtoul
Tomi,
In drivers/video/omap2/omapfb/omapfb-main.c:
static int omapfb_parse_vram_param(const char *param, int max_entries,
unsigned long *sizes, unsigned long *paddrs)
{
int fbnum;
unsigned long size;
unsigned long paddr = 0;
char *p, *start;
start = (char *)param;
while (1) {
p = start;
fbnum = simple_strtoul(p, p, 10);
- if (p == param)
+ if (p == start)
correct?
Hein
--
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
