diff mbox

[RFC/PATCH,3/3] mmc: sdhci: add asynchronous interrupt support

Message ID 1348024372-32073-4-git-send-email-keyuan.liu@gmail.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Kevin Liu Sept. 19, 2012, 3:12 a.m. UTC 
From: Kevin Liu <kliu5@marvell.com>

If host support asynchronous interrupt and sdio device has enabled it,
then enable/disable asynchronous interrupt on host when enable/disable
sdio irq.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |   16 ++++++++++++++++
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 18 insertions(+), 0 deletions(-)

Comments

Ulf Hansson Sept. 19, 2012, 3:56 p.m. UTC | #1 
Hi Kevin,

On 19 September 2012 05:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
> From: Kevin Liu <kliu5@marvell.com>
>
> If host support asynchronous interrupt and sdio device has enabled it,
> then enable/disable asynchronous interrupt on host when enable/disable
> sdio irq.
>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/host/sdhci.c |   16 ++++++++++++++++
>  drivers/mmc/host/sdhci.h |    2 ++
>  2 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 0e15c79..f6136e2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1575,6 +1575,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>
>  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>  {
> +       u16 ctrl;
> +
>         if (host->flags & SDHCI_DEVICE_DEAD)
>                 goto out;
>
> @@ -1583,6 +1585,16 @@ static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>         else
>                 host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
>
> +       if ((host->mmc->caps2 & MMC_CAP2_ASYNC_INT) &&
> +               (host->mmc->card->cccr.async_int)) {

It is a quite special card bit value you are checking for. Can you be
sure that the cccr struct exist here?

Maybe it could be a good idea to implement something similar as for
example mmc_card_highspeed | mmc_card_set_highspeed functions.
Thus add a new card state, which is set accordingly when this bit is
set and instead check for this state here. In this case you would not
have to check the caps2 here, since that is already done by the
protocol layer.

> +               ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +               if (enable)
> +                       ctrl |= SDHCI_CTRL_ASYNC_INT_ENABLE;
> +               else
> +                       ctrl &= ~SDHCI_CTRL_ASYNC_INT_ENABLE;
> +               sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +       }
> +
>         /* SDIO IRQ will be enabled as appropriate in runtime resume */
>         if (host->runtime_suspended)
>                 goto out;
> @@ -2895,6 +2907,10 @@ int sdhci_add_host(struct sdhci_host *host)
>         else
>                 mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
>
> +       /* Does the host support async int? */
> +       if (caps[0] & SDHCI_CAN_ASYNC_INT)
> +               mmc->caps2 |= MMC_CAP2_ASYNC_INT;
> +
>         /* Initial value for re-tuning timer count */
>         host->tuning_count = (caps[1] & SDHCI_RETUNING_TIMER_COUNT_MASK) >>
>                               SDHCI_RETUNING_TIMER_COUNT_SHIFT;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 97653ea..2e89dac 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -167,6 +167,7 @@
>  #define   SDHCI_CTRL_DRV_TYPE_D                0x0030
>  #define  SDHCI_CTRL_EXEC_TUNING                0x0040
>  #define  SDHCI_CTRL_TUNED_CLK          0x0080
> +#define  SDHCI_CTRL_ASYNC_INT_ENABLE   0x4000
>  #define  SDHCI_CTRL_PRESET_VAL_ENABLE  0x8000
>
>  #define SDHCI_CAPABILITIES     0x40
> @@ -187,6 +188,7 @@
>  #define  SDHCI_CAN_VDD_300     0x02000000
>  #define  SDHCI_CAN_VDD_180     0x04000000
>  #define  SDHCI_CAN_64BIT       0x10000000
> +#define  SDHCI_CAN_ASYNC_INT   0x20000000
>
>  #define  SDHCI_SUPPORT_SDR50   0x00000001
>  #define  SDHCI_SUPPORT_SDR104  0x00000002
> --
> 1.7.0.4
>
> --
> 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

Kind regards
Ulf Hansson
--
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
Kevin Liu Sept. 20, 2012, 11:33 a.m. UTC | #2 
Hi, Ulf

2012/9/19 Ulf Hansson <ulf.hansson@linaro.org>:
> Hi Kevin,
>
> On 19 September 2012 05:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>> From: Kevin Liu <kliu5@marvell.com>
>>
>> If host support asynchronous interrupt and sdio device has enabled it,
>> then enable/disable asynchronous interrupt on host when enable/disable
>> sdio irq.
>>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>  drivers/mmc/host/sdhci.c |   16 ++++++++++++++++
>>  drivers/mmc/host/sdhci.h |    2 ++
>>  2 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 0e15c79..f6136e2 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1575,6 +1575,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>>
>>  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>>  {
>> +       u16 ctrl;
>> +
>>         if (host->flags & SDHCI_DEVICE_DEAD)
>>                 goto out;
>>
>> @@ -1583,6 +1585,16 @@ static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>>         else
>>                 host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
>>
>> +       if ((host->mmc->caps2 & MMC_CAP2_ASYNC_INT) &&
>> +               (host->mmc->card->cccr.async_int)) {
>
> It is a quite special card bit value you are checking for. Can you be
> sure that the cccr struct exist here?
>
The bit async_int is added to struct sdio_cccr by my previous patch 0002.
If EAI is enabled, this bit will be set.

> Maybe it could be a good idea to implement something similar as for
> example mmc_card_highspeed | mmc_card_set_highspeed functions.
> Thus add a new card state, which is set accordingly when this bit is
> set and instead check for this state here. In this case you would not
> have to check the caps2 here, since that is already done by the
> protocol layer.
>
I think you want to replace the check for host->mmc->card->cccr.async_int with
mmc_card_xxx, right? It's a good idea.
It's not reasonable to replace caps2 with such function since they are
for card status
rather than host.
How do you think?

Thanks
Kevin
--
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
Ulf Hansson Sept. 20, 2012, 3:37 p.m. UTC | #3 
On 20 September 2012 13:33, Kevin Liu <keyuan.liu@gmail.com> wrote:
> Hi, Ulf
>
> 2012/9/19 Ulf Hansson <ulf.hansson@linaro.org>:
>> Hi Kevin,
>>
>> On 19 September 2012 05:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>> From: Kevin Liu <kliu5@marvell.com>
>>>
>>> If host support asynchronous interrupt and sdio device has enabled it,
>>> then enable/disable asynchronous interrupt on host when enable/disable
>>> sdio irq.
>>>
>>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>>> ---
>>>  drivers/mmc/host/sdhci.c |   16 ++++++++++++++++
>>>  drivers/mmc/host/sdhci.h |    2 ++
>>>  2 files changed, 18 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 0e15c79..f6136e2 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1575,6 +1575,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>>>
>>>  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>>>  {
>>> +       u16 ctrl;
>>> +
>>>         if (host->flags & SDHCI_DEVICE_DEAD)
>>>                 goto out;
>>>
>>> @@ -1583,6 +1585,16 @@ static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>>>         else
>>>                 host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
>>>
>>> +       if ((host->mmc->caps2 & MMC_CAP2_ASYNC_INT) &&
>>> +               (host->mmc->card->cccr.async_int)) {
>>
>> It is a quite special card bit value you are checking for. Can you be
>> sure that the cccr struct exist here?
>>
> The bit async_int is added to struct sdio_cccr by my previous patch 0002.
> If EAI is enabled, this bit will be set.
>
>> Maybe it could be a good idea to implement something similar as for
>> example mmc_card_highspeed | mmc_card_set_highspeed functions.
>> Thus add a new card state, which is set accordingly when this bit is
>> set and instead check for this state here. In this case you would not
>> have to check the caps2 here, since that is already done by the
>> protocol layer.
>>
> I think you want to replace the check for host->mmc->card->cccr.async_int with
> mmc_card_xxx, right? It's a good idea.

Correct!

> It's not reasonable to replace caps2 with such function since they are
> for card status
> rather than host.
> How do you think?

I mean in the sdio framework, you already consider the new CAP before
setting cccr.async and thus the "mmc_card_xxx" depend on this CAP as
well. This will mean that the host driver does not need to check the
CAP here, mmc_card_xxx is enough.

Kind regards
Ulf Hansson
--
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
Kevin Liu Sept. 21, 2012, 2:19 a.m. UTC | #4 
2012/9/20 Ulf Hansson <ulf.hansson@linaro.org>:
> On 20 September 2012 13:33, Kevin Liu <keyuan.liu@gmail.com> wrote:
>> Hi, Ulf
>>
>> 2012/9/19 Ulf Hansson <ulf.hansson@linaro.org>:
>>> Hi Kevin,
>>>
>>> On 19 September 2012 05:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>>> From: Kevin Liu <kliu5@marvell.com>
>>>>
>>>> If host support asynchronous interrupt and sdio device has enabled it,
>>>> then enable/disable asynchronous interrupt on host when enable/disable
>>>> sdio irq.
>>>>
>>>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci.c |   16 ++++++++++++++++
>>>>  drivers/mmc/host/sdhci.h |    2 ++
>>>>  2 files changed, 18 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index 0e15c79..f6136e2 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -1575,6 +1575,8 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>>>>
>>>>  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>>>>  {
>>>> +       u16 ctrl;
>>>> +
>>>>         if (host->flags & SDHCI_DEVICE_DEAD)
>>>>                 goto out;
>>>>
>>>> @@ -1583,6 +1585,16 @@ static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>>>>         else
>>>>                 host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
>>>>
>>>> +       if ((host->mmc->caps2 & MMC_CAP2_ASYNC_INT) &&
>>>> +               (host->mmc->card->cccr.async_int)) {
>>>
>>> It is a quite special card bit value you are checking for. Can you be
>>> sure that the cccr struct exist here?
>>>
>> The bit async_int is added to struct sdio_cccr by my previous patch 0002.
>> If EAI is enabled, this bit will be set.
>>
>>> Maybe it could be a good idea to implement something similar as for
>>> example mmc_card_highspeed | mmc_card_set_highspeed functions.
>>> Thus add a new card state, which is set accordingly when this bit is
>>> set and instead check for this state here. In this case you would not
>>> have to check the caps2 here, since that is already done by the
>>> protocol layer.
>>>
>> I think you want to replace the check for host->mmc->card->cccr.async_int with
>> mmc_card_xxx, right? It's a good idea.
>
> Correct!
>
>> It's not reasonable to replace caps2 with such function since they are
>> for card status
>> rather than host.
>> How do you think?
>
> I mean in the sdio framework, you already consider the new CAP before
> setting cccr.async and thus the "mmc_card_xxx" depend on this CAP as
> well. This will mean that the host driver does not need to check the
> CAP here, mmc_card_xxx is enough.
>
Righit, I will update the patch.

Thanks
Kevin
--
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
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0e15c79..f6136e2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1575,6 +1575,8 @@  static int sdhci_get_ro(struct mmc_host *mmc)
 
 static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
 {
+	u16 ctrl;
+
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		goto out;
 
@@ -1583,6 +1585,16 @@  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
 	else
 		host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
 
+	if ((host->mmc->caps2 & MMC_CAP2_ASYNC_INT) &&
+		(host->mmc->card->cccr.async_int)) {
+		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (enable)
+			ctrl |= SDHCI_CTRL_ASYNC_INT_ENABLE;
+		else
+			ctrl &= ~SDHCI_CTRL_ASYNC_INT_ENABLE;
+		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+	}
+
 	/* SDIO IRQ will be enabled as appropriate in runtime resume */
 	if (host->runtime_suspended)
 		goto out;
@@ -2895,6 +2907,10 @@  int sdhci_add_host(struct sdhci_host *host)
 	else
 		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
 
+	/* Does the host support async int? */
+	if (caps[0] & SDHCI_CAN_ASYNC_INT)
+		mmc->caps2 |= MMC_CAP2_ASYNC_INT;
+
 	/* Initial value for re-tuning timer count */
 	host->tuning_count = (caps[1] & SDHCI_RETUNING_TIMER_COUNT_MASK) >>
 			      SDHCI_RETUNING_TIMER_COUNT_SHIFT;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 97653ea..2e89dac 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -167,6 +167,7 @@ 
 #define   SDHCI_CTRL_DRV_TYPE_D		0x0030
 #define  SDHCI_CTRL_EXEC_TUNING		0x0040
 #define  SDHCI_CTRL_TUNED_CLK		0x0080
+#define  SDHCI_CTRL_ASYNC_INT_ENABLE	0x4000
 #define  SDHCI_CTRL_PRESET_VAL_ENABLE	0x8000
 
 #define SDHCI_CAPABILITIES	0x40
@@ -187,6 +188,7 @@ 
 #define  SDHCI_CAN_VDD_300	0x02000000
 #define  SDHCI_CAN_VDD_180	0x04000000
 #define  SDHCI_CAN_64BIT	0x10000000
+#define  SDHCI_CAN_ASYNC_INT	0x20000000
 
 #define  SDHCI_SUPPORT_SDR50	0x00000001
 #define  SDHCI_SUPPORT_SDR104	0x00000002