* [PATCH] mmc: omap_hsmmc: Fix sleep too long in ISR context.
@ 2013-08-01 2:18 majianpeng
2013-08-01 8:22 ` Hein Tibosch
0 siblings, 1 reply; 3+ messages in thread
From: majianpeng @ 2013-08-01 2:18 UTC (permalink / raw)
To: balajitk, cjb; +Cc: mayuzheng, linux-mmc, linux-omap, linux-kernel
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 <mayuzheng@kedacom.com>
Tested-by: Yuzheng Ma <mayuzheng@kedacom.com>
Signed-off-by: Jianpeng Ma <majianpeng@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);
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();
+ while (i--) {
+ if ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+ break;
+ udelay(1);
+ }
}
- 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),
--
1.8.1.2
Thanks!
Jianpeng Ma
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] mmc: omap_hsmmc: Fix sleep too long in ISR context.
2013-08-01 2:18 [PATCH] mmc: omap_hsmmc: Fix sleep too long in ISR context majianpeng
@ 2013-08-01 8:22 ` Hein Tibosch
2013-08-01 8:40 ` majianpeng
0 siblings, 1 reply; 3+ messages in thread
From: Hein Tibosch @ 2013-08-01 8:22 UTC (permalink / raw)
To: majianpeng
Cc: balajitk, cjb, mayuzheng, linux-mmc, linux-omap, linux-kernel,
Felipe Balbi
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 <mayuzheng@kedacom.com>
> Tested-by: Yuzheng Ma <mayuzheng@kedacom.com>
> Signed-off-by: Jianpeng Ma <majianpeng@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Re: [PATCH] mmc: omap_hsmmc: Fix sleep too long in ISR context.
2013-08-01 8:22 ` Hein Tibosch
@ 2013-08-01 8:40 ` majianpeng
0 siblings, 0 replies; 3+ messages in thread
From: majianpeng @ 2013-08-01 8:40 UTC (permalink / raw)
To: Hein Tibosch
Cc: balajitk, cjb, mayuzheng, linux-mmc, linux-omap, linux-kernel,
Felipe Balbi
>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 <mayuzheng@kedacom.com>
>> Tested-by: Yuzheng Ma <mayuzheng@kedacom.com>
>> Signed-off-by: Jianpeng Ma <majianpeng@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;
>
Yes, you are right.
>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?
From my test, i found it don't monitor a 0->1 transtion.That is the last result is 'i = limit'.
The later while opertion stop also because 'i = limit'.
The basic reason is the hardware.
It write bit and monitor 1-->0---1>---0.But we start monitor we only got 0 and the value don't change.
Maybe the transition is 1-->0.So the monitor can't work but still waste cpu.
>
>> + 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.
Yes, but it at most wast 1us.
Jianpeng Ma
>
>> + }
>> }
>> - 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
Thanks!
Jianpeng Ma
>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 <mayuzheng@kedacom.com>
>> Tested-by: Yuzheng Ma <mayuzheng@kedacom.com>
>> Signed-off-by: Jianpeng Ma <majianpeng@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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-08-01 9:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-01 2:18 [PATCH] mmc: omap_hsmmc: Fix sleep too long in ISR context majianpeng
2013-08-01 8:22 ` Hein Tibosch
2013-08-01 8:40 ` majianpeng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).