diff mbox

mmc: sdhci: clear interrupt when retune interrupt received

Message ID 1496947916-15785-1-git-send-email-troy.kisky@boundarydevices.com (mailing list archive)
State New, archived
Headers show

Commit Message

Troy Kisky June 8, 2017, 6:51 p.m. UTC 
This lets the loop exit before max_loops reaches 0.

Fixes: f37b20ebc4bc ("mmc: sdhci: add standard hw auto retuning support")

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/mmc/host/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Adrian Hunter June 9, 2017, 12:46 p.m. UTC | #1 
On 08/06/17 21:51, Troy Kisky wrote:
> This lets the loop exit before max_loops reaches 0.

Needs more explanation.

> 
> Fixes: f37b20ebc4bc ("mmc: sdhci: add standard hw auto retuning support")
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/mmc/host/sdhci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ecd0d43..e104194 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2687,7 +2687,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  
>  		/* Clear selected interrupts. */
>  		mask = intmask & (SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK |
> -				  SDHCI_INT_BUS_POWER);
> +				  SDHCI_INT_BUS_POWER | SDHCI_INT_RETUNE);

SDHCI_INT_RETUNE is defined to be read-only so why write to it.

>  		sdhci_writel(host, mask, SDHCI_INT_STATUS);
>  
>  		if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Troy Kisky June 9, 2017, 3:45 p.m. UTC | #2 
On 6/9/2017 5:46 AM, Adrian Hunter wrote:
> On 08/06/17 21:51, Troy Kisky wrote:
>> This lets the loop exit before max_loops reaches 0.
> 
> Needs more explanation.
> 
>>
>> Fixes: f37b20ebc4bc ("mmc: sdhci: add standard hw auto retuning support")
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>  drivers/mmc/host/sdhci.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ecd0d43..e104194 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2687,7 +2687,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>  
>>  		/* Clear selected interrupts. */
>>  		mask = intmask & (SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK |
>> -				  SDHCI_INT_BUS_POWER);
>> +				  SDHCI_INT_BUS_POWER | SDHCI_INT_RETUNE);
> 
> SDHCI_INT_RETUNE is defined to be read-only so why write to it.


SDHCI_INT_RETUNE is bit 12
host/sdhci.h:#define  SDHCI_INT_RETUNE  0x00001000

Which at least from i.mx6/i.mx7 reference manuals, is a write 1 to clear bit(marked as w1c).



> 
>>  		sdhci_writel(host, mask, SDHCI_INT_STATUS);
>>  
>>  		if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter June 12, 2017, 12:33 p.m. UTC | #3 
On 09/06/17 18:45, Troy Kisky wrote:
> On 6/9/2017 5:46 AM, Adrian Hunter wrote:
>> On 08/06/17 21:51, Troy Kisky wrote:
>>> This lets the loop exit before max_loops reaches 0.
>>
>> Needs more explanation.
>>
>>>
>>> Fixes: f37b20ebc4bc ("mmc: sdhci: add standard hw auto retuning support")
>>>
>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>> ---
>>>  drivers/mmc/host/sdhci.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index ecd0d43..e104194 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2687,7 +2687,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>>  
>>>  		/* Clear selected interrupts. */
>>>  		mask = intmask & (SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK |
>>> -				  SDHCI_INT_BUS_POWER);
>>> +				  SDHCI_INT_BUS_POWER | SDHCI_INT_RETUNE);
>>
>> SDHCI_INT_RETUNE is defined to be read-only so why write to it.
> 
> 
> SDHCI_INT_RETUNE is bit 12
> host/sdhci.h:#define  SDHCI_INT_RETUNE  0x00001000
> 
> Which at least from i.mx6/i.mx7 reference manuals, is a write 1 to clear bit(marked as w1c).

So it doesn't work the way it is specified in the SDHCI spec.

It should be harmless to write 1, but you still need to explain how the bit
works on your hardware.  What does "This lets the loop exit before max_loops
reaches 0" mean?

> 
> 
> 
>>
>>>  		sdhci_writel(host, mask, SDHCI_INT_STATUS);
>>>  
>>>  		if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
>>>
>>
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aisheng Dong June 12, 2017, 2:03 p.m. UTC | #4 
> -----Original Message-----

> From: Adrian Hunter [mailto:adrian.hunter@intel.com]

> Sent: Monday, June 12, 2017 8:34 PM

> To: Troy Kisky; A.S. Dong

> Cc: ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; Fabio Estevam;

> gary.bisson@boundarydevices.com

> Subject: Re: [PATCH] mmc: sdhci: clear interrupt when retune interrupt

> received

> 

> On 09/06/17 18:45, Troy Kisky wrote:

> > On 6/9/2017 5:46 AM, Adrian Hunter wrote:

> >> On 08/06/17 21:51, Troy Kisky wrote:

> >>> This lets the loop exit before max_loops reaches 0.

> >>

> >> Needs more explanation.

> >>

> >>>

> >>> Fixes: f37b20ebc4bc ("mmc: sdhci: add standard hw auto retuning

> >>> support")

> >>>

> >>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

> >>> ---

> >>>  drivers/mmc/host/sdhci.c | 2 +-

> >>>  1 file changed, 1 insertion(+), 1 deletion(-)

> >>>

> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c

> >>> index ecd0d43..e104194 100644

> >>> --- a/drivers/mmc/host/sdhci.c

> >>> +++ b/drivers/mmc/host/sdhci.c

> >>> @@ -2687,7 +2687,7 @@ static irqreturn_t sdhci_irq(int irq, void

> >>> *dev_id)

> >>>

> >>>  		/* Clear selected interrupts. */

> >>>  		mask = intmask & (SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK |

> >>> -				  SDHCI_INT_BUS_POWER);

> >>> +				  SDHCI_INT_BUS_POWER | SDHCI_INT_RETUNE);

> >>

> >> SDHCI_INT_RETUNE is defined to be read-only so why write to it.

> >

> >

> > SDHCI_INT_RETUNE is bit 12

> > host/sdhci.h:#define  SDHCI_INT_RETUNE  0x00001000

> >

> > Which at least from i.mx6/i.mx7 reference manuals, is a write 1 to clear

> bit(marked as w1c).

> 

> So it doesn't work the way it is specified in the SDHCI spec.

> 

> It should be harmless to write 1, but you still need to explain how the

> bit works on your hardware.  What does "This lets the loop exit before

> max_loops reaches 0" mean?

> 


Can't remember too much history.
Simply from code inspection, it seems without clear SDHCI_INT_RETUNE,
it will loop max_loops.

Not sure about ""This lets the loop exit before max_loops reaches 0" mean?"

Troy,
Would you help explain it?

Regards
Dong Aisheng

> >

> >

> >

> >>

> >>>  		sdhci_writel(host, mask, SDHCI_INT_STATUS);

> >>>

> >>>  		if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE))

> {

> >>>

> >>

> >>

> >

> >
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ecd0d43..e104194 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2687,7 +2687,7 @@  static irqreturn_t sdhci_irq(int irq, void *dev_id)
 
 		/* Clear selected interrupts. */
 		mask = intmask & (SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK |
-				  SDHCI_INT_BUS_POWER);
+				  SDHCI_INT_BUS_POWER | SDHCI_INT_RETUNE);
 		sdhci_writel(host, mask, SDHCI_INT_STATUS);
 
 		if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {