[PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

[PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

[yongd]

From: yongd <yongd@marvell.com>

In the current code logic, sdhci_add_host() will enable the polling
method (set MMC_CAP_NEEDS_POLL) for a removable card (MMC_CAP_
NONREMOVABLE is not set) whose host's internal card detection method
is disabled for some reason (SDHCI_QUIRK_BROKEN_CARD_DETECTION is set).

However, this is improper since we can have some other card detection
methods besides host internal card detection and the polling method.
For example, we might use an external GPIO pin's interrupt to detect
a removable SD card, so we shall set SDHCI_QUIRK_BROKEN_CARD_DETECTION
since we don't use the internal card detection, and not set MMC_CAP_
NONREMOVABLE since the SD card is physically removable. As a result,
with the current code, the polling method will also be enabled.
Apparently, this is redundant and not what we want.

And the better one to decide whether we use polling or not should be
the host driver itself. Actually, some host driver has already been
like this. Eg, in drivers/mmc/host/Au1xmmc.c, polling will be enabled
only after the board-specific card detection can't be set up successfully.

Change-Id: I27774488a7b9191d7bc39699fd7d62ee21bbf157
Signed-off-by: yongd <yongd@marvell.com>
---
 drivers/mmc/host/sdhci.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0e15c79..900d5f4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2840,10 +2840,6 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (caps[0] & SDHCI_CAN_DO_HISPD)
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
 
-	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
-	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
-		mmc->caps |= MMC_CAP_NEEDS_POLL;
-
 	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
 	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
 	if (IS_ERR(host->vqmmc)) {
-- 
1.7.9.5

Re: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

[Anton Vorontsov]

On Tue, Sep 25, 2012 at 02:34:07PM +0800, yongd wrote:
> From: yongd <yongd@marvell.com>
[...]
> And the better one to decide whether we use polling or not should be
> the host driver itself. Actually, some host driver has already been
> like this. Eg, in drivers/mmc/host/Au1xmmc.c, polling will be enabled
> only after the board-specific card detection can't be set up successfully.

I guess it's not that simple. If you remove this, you have to add
appropriate CAP_NEEDS_POLL for these drivers:

 linux/drivers/mmc/host$ git grep -l SDHCI_QUIRK_BROKEN_CARD_DETECTION | xargs grep -L NEEDS_POLL
 sdhci-esdhc-imx.c
 sdhci-of-esdhc.c
 sdhci-pci.c
 sdhci-pxav2.c
 sdhci-pxav3.c
 sdhci-s3c.c

> Change-Id: I27774488a7b9191d7bc39699fd7d62ee21bbf157
> Signed-off-by: yongd <yongd@marvell.com>
> ---
>  drivers/mmc/host/sdhci.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 0e15c79..900d5f4 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2840,10 +2840,6 @@ int sdhci_add_host(struct sdhci_host *host)
>  	if (caps[0] & SDHCI_CAN_DO_HISPD)
>  		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>  
> -	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
> -	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
> -		mmc->caps |= MMC_CAP_NEEDS_POLL;
> -
>  	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>  	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
>  	if (IS_ERR(host->vqmmc)) {
> -- 
> 1.7.9.5

RE: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

[Yong Ding]

Anton,
Thanks a lot for your comments. I've just reviewed those host drivers under drivers/mmc/host who use SDHCI_QUIRK_BROKEN_CARD_DETECTION. And here are the details,

1. sdhci-pxav2/3.c
Actually they set "SDHCI_QUIRK_BROKEN_CARD_DETECTION and MMC_CAP_NONREMOVABLE" together for a permanent card who is always wired to host and with power always on, eg, emmc card. So, they don't set SDHCI_QUIRK_BROKEN_CARD_DETECTION for polling:-)

2. sdhci-s3c.c
If the card detection type is S3C_SDHCI_CD_NONE, they are expecting polling method to detect card. And in the current code logic, sdhci-s3c.c does not set MMC_CAP_NEEDS_POLL itself, but depends on the code which I am preferring to remove. So, you are right this is not so that simple.

3. sdhci-esdhc-imx.c
By default, it sets SDHCI_QUIRK_BROKEN_CARD_DETECTION for all hosts. 
And then if the card detection type is ESDHC_CD_CONTROLLER(indicating we use the host internal card detection), it will clear this flag. No issue.
If the cd_type is ESDHC_CD_PERMANENT, it will still keep the set of the flag. This is just exactly the same case as sdhci-pxav2/3.c. No issue.
If the cd_type is ESDHC_CD_GPIO(using external GPIO for CD), it will keep the set of this flag, and I think this is the right logic since we don't use host internal card detection. But this will eventually causes the enabling of polling method, which is rightly the case mentioned in my patch comment. A +1 for my patch.
If the cd_type is ESDHC_CD_NONE, this is the same case as sdhci-s3c.c. A +1 for your guess/concern:-)

4. sdhci-of-esdhc.c and sdhci-pci.c
Cased are already included in the above 3 samples. So nothing more here.

So, in all, u are right if with my current patch, some host drivers need some improvement to add MMC_CAP_NEEDS_POLL when it is actually needed. But I think this shall be the right way to follow. Or, we might enable polling for some cases in which it is unnecessary, and maybe this is a potential issue-bomb. How do u think?


Best Wishes,

Yong Ding
Operating Systems Engineering,
Application Processor Systems Engineering

> -----Original Message-----
> From: Anton Vorontsov [mailto:anton.vorontsov@linaro.org]
> Sent: Tuesday, September 25, 2012 3:04 PM
> To: Yong Ding
> Cc: Chris Ball; linux-mmc@vger.kernel.org; Daniel Drake; Zhangfei Gao
> Subject: Re: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in
> sdhci_add_host
> 
> On Tue, Sep 25, 2012 at 02:34:07PM +0800, yongd wrote:
> > From: yongd <yongd@marvell.com>
> [...]
> > And the better one to decide whether we use polling or not should be
> > the host driver itself. Actually, some host driver has already been
> > like this. Eg, in drivers/mmc/host/Au1xmmc.c, polling will be enabled
> > only after the board-specific card detection can't be set up
> successfully.
> 
> I guess it's not that simple. If you remove this, you have to add
> appropriate CAP_NEEDS_POLL for these drivers:
> 
>  linux/drivers/mmc/host$ git grep -l SDHCI_QUIRK_BROKEN_CARD_DETECTION
> | xargs grep -L NEEDS_POLL
>  sdhci-esdhc-imx.c
>  sdhci-of-esdhc.c
>  sdhci-pci.c
>  sdhci-pxav2.c
>  sdhci-pxav3.c
>  sdhci-s3c.c
> 
> > Change-Id: I27774488a7b9191d7bc39699fd7d62ee21bbf157
> > Signed-off-by: yongd <yongd@marvell.com>
> > ---
> >  drivers/mmc/host/sdhci.c |    4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 0e15c79..900d5f4 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -2840,10 +2840,6 @@ int sdhci_add_host(struct sdhci_host *host)
> >  	if (caps[0] & SDHCI_CAN_DO_HISPD)
> >  		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> >
> > -	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
> > -	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
> > -		mmc->caps |= MMC_CAP_NEEDS_POLL;
> > -
> >  	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS
> */
> >  	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
> >  	if (IS_ERR(host->vqmmc)) {
> > --
> > 1.7.9.5

Re: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

[Anton Vorontsov]

On Tue, Sep 25, 2012 at 02:13:10AM -0700, Yong Ding wrote:
[...]
> So, in all, u are right if with my current patch, some host drivers need
> some improvement to add MMC_CAP_NEEDS_POLL when it is actually needed.
> But I think this shall be the right way to follow. Or, we might enable
> polling for some cases in which it is unnecessary, and maybe this is a
> potential issue-bomb. How do u think?

I think if you carefully review and fixup all the drivers, it will be
fine.

But you'd have to add MMC_CAP_NEEDS_POLL into drivers' code (while w/ the
quirk it's less lines of code for drivers).

So, here is another idea: how about something like this

#define SDHCI_QUIRK_NEEDS_POLL \
	(SDHCI_QUIRK_BROKEN_CARD_DETECTION | (1 << NN))

And changing the logic to:

	if ((host->quirks & SDHCI_QUIRK_NEEDS_POLL) &&
	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
		mmc->caps |= MMC_CAP_NEEDS_POLL;

And then, you'd just convert all the current drivers to
SDHCI_QUIRK_NEEDS_POLL (which would be 100% safe), and for your driver,
you'd only set SDHCI_QUIRK_BROKEN_CARD_DETECTION.

Thanks,
Anton.

RE: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

[Yong Ding]

Anton,
For adding a new QUICK which is something like SDHCI_QUICK_NEEDS_POLL, I think it is unnecessary since it actually has the same meaning as MMC_CAP_NEEDS_POLLING. Actually, we can directly set MMC_CAP_NEEDS_POLL in host driver.

So I prefer to review all the host drivers, and proposed relevant changes. And some vendors already have been like this(setting polling by themselves). And as u can see, I have already sent the new patch set. Pls also help review. Thanks a lot.


Best Wishes,

Yong Ding
Operating Systems Engineering,
Application Processor Systems Engineering

-----Original Message-----
From: Anton Vorontsov [mailto:anton.vorontsov@linaro.org] 
Sent: Thursday, September 27, 2012 7:50 AM
To: Yong Ding
Cc: Chris Ball; linux-mmc@vger.kernel.org; Daniel Drake; Zhangfei Gao
Subject: Re: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

On Tue, Sep 25, 2012 at 02:13:10AM -0700, Yong Ding wrote:
[...]
> So, in all, u are right if with my current patch, some host drivers need
> some improvement to add MMC_CAP_NEEDS_POLL when it is actually needed.
> But I think this shall be the right way to follow. Or, we might enable
> polling for some cases in which it is unnecessary, and maybe this is a
> potential issue-bomb. How do u think?

I think if you carefully review and fixup all the drivers, it will be
fine.

But you'd have to add MMC_CAP_NEEDS_POLL into drivers' code (while w/ the
quirk it's less lines of code for drivers).

So, here is another idea: how about something like this

#define SDHCI_QUIRK_NEEDS_POLL \
	(SDHCI_QUIRK_BROKEN_CARD_DETECTION | (1 << NN))

And changing the logic to:

	if ((host->quirks & SDHCI_QUIRK_NEEDS_POLL) &&
	    !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
		mmc->caps |= MMC_CAP_NEEDS_POLL;

And then, you'd just convert all the current drivers to
SDHCI_QUIRK_NEEDS_POLL (which would be 100% safe), and for your driver,
you'd only set SDHCI_QUIRK_BROKEN_CARD_DETECTION.

Thanks,
Anton.