ngd@marvell.com> # This line is ignored.
From: yongd <redacted>
Subject:[PATCH 0/3] mmc: enable card-detection polling in host driver rather than in sdhci_add_host
In-Reply-To:
Hi all,
As checked, SDHCI_QUICK_BROKEN_CARD_DETECTION was previously introduced by Anton Vorontsov in commit
68d1fb7e229c6f95be4fbbe3eb46b24e41184924. For a removable card (MMC_CAP_NONREMOVABLE is not set),
if this QUICK is set, sdhci_add_host will set MMC_CAP_NEEDS_POLL to enable card-detection polling method,
and sdhci_set_card_detection will not enable host controller card insert/remove interrupts (the host
internal card detection method).
However, we can have some other card detection methods besides host internal card detection and polling
methods. For example, we might use an external GPIO pin for detection. In such case, we will not set
MMC_CAP_NONREMOVABLE since removable, and set SDHCI_QUICK_BROKEN_CARD_DETECTION since we don't use controller
card insert/remove interrupts. But unexpectedly, sdhci_add_host will still enable polling for us. This
is redundant.
So, here comes the following solution.
1st, enable card-detection polling in vendor host driver itself so that it will not rely on sdhci_add_host.
[PATCH 1/3]: for sdhci-esdhc-imx.c
[PATCH 2/3]: for sdhci-s3c.c
And for other vendor host drivers(sdhci-pxav2/3.c, sdhci-pci.c, sdhci-of-esdhc.c), as checked, those who
set SDHCI_QUICK_BROKEN_CARD_DETECTION actually do not set this for enabling polling. So no similar change is
needed.
2nd, remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host, as in [PATCH 3/3].
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Actually I have had some discussion with Anton, and now added more reviewers here since my proposal influences
others. I will appreciate your review and comments. Thanks a lot:-)
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
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 polling method. For
example, if the card detection type is ESDHC_CD_GPIO (external gpio pin
for CD), we will keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set. This is right.
But, just as above said, sdhci_add_host() will also enable polling for such
a card. This is redundant.
On the other hand, for the card with ESDHC_CD_NONE detection type(no CD, neither
controller nor gpio, so use polling), we currently rely on sdhci_add_host() to
enable polling for us.
Here proposed a solution for such an embarrassing case. 1st, this patch will
de-couple polling enabling with sdhci_add_host() by doing this in host driver
itself, just as some other vendors. 2nd, one more patch will remove such improper
MMC_CAP_NEEDS_POLL enabling in sdhci_add_host().
Change-Id: Ia7525009d8fd188e3f0169f225e4a76ff9e94b47
Signed-off-by: yongd <redacted>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
1 file changed, 1 insertion(+)
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 polling method. As a
result, sdhci_add_host() will redundantly enable polling for a card
which actually uses other methods, eg external GPIO.
So, we plan to remove such improper setting in sdhci_add_host(). But
before this, this patch is a MUST, which enables polling in host driver
itself when the detection type is S3C_SDHCI_CD_NONE.
Change-Id: I28bd6765e6c6a9dd6c288ff4ca6ecf7268a2d8ed
Signed-off-by: yongd <redacted>
---
drivers/mmc/host/sdhci-s3c.c | 3 +++
1 file changed, 3 insertions(+)
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 <redacted>
---
drivers/mmc/host/sdhci.c | 4 ----
1 file changed, 4 deletions(-)
@@ -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)){
On Fri, Sep 28, 2012 at 06:28:31PM +0800, yongd wrote:
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 polling method. For
example, if the card detection type is ESDHC_CD_GPIO (external gpio pin
for CD), we will keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set. This is right.
But, just as above said, sdhci_add_host() will also enable polling for such
a card. This is redundant.
At least for sdhci-esdhc-imx, SDHCI_QUIRK_BROKEN_CARD_DETECTION will
be set only when neither ESDHC_CD_GPIO nor ESDHC_CD_CONTROLLER works.
Shawn
quoted hunk
On the other hand, for the card with ESDHC_CD_NONE detection type(no CD, neither
controller nor gpio, so use polling), we currently rely on sdhci_add_host() to
enable polling for us.
Here proposed a solution for such an embarrassing case. 1st, this patch will
de-couple polling enabling with sdhci_add_host() by doing this in host driver
itself, just as some other vendors. 2nd, one more patch will remove such improper
MMC_CAP_NEEDS_POLL enabling in sdhci_add_host().
Change-Id: Ia7525009d8fd188e3f0169f225e4a76ff9e94b47
Signed-off-by: yongd <redacted>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
1 file changed, 1 insertion(+)
Shawn,
Thanks for your comment. And sorry for my so late due to illness:-)
SDHCI_QUIRK_BROKEN_CARD_DETECTION is used for notifying we don't use the host internal card detection method so that we don't need enable/disable those relevant interrupt bits of host(sdhci_set_card_detection in sdhci.c).
And as I double-checked the latest kernel code, actually sdhci-esdhc-imx sets this flag by default, and then will clear it only when the detection type is ESDHC_CD_CONTROLLER. So this aligns with my understanding.
What I want to do is that 1st we shall set MMC_CAP_NEEDS_POLL by our host driver itself and 2nd remove the improper logic in sdhci_add_host() . How do u think? Thanks.
________________________________________
From: Shawn Guo [shawn.guo@linaro.org]
Sent: Sunday, October 07, 2012 22:06
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself
On Fri, Sep 28, 2012 at 06:28:31PM +0800, yongd wrote:
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 polling method. For
example, if the card detection type is ESDHC_CD_GPIO (external gpio pin
for CD), we will keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set. This is right.
But, just as above said, sdhci_add_host() will also enable polling for such
a card. This is redundant.
At least for sdhci-esdhc-imx, SDHCI_QUIRK_BROKEN_CARD_DETECTION will
be set only when neither ESDHC_CD_GPIO nor ESDHC_CD_CONTROLLER works.
Shawn
quoted hunk
On the other hand, for the card with ESDHC_CD_NONE detection type(no CD, neither
controller nor gpio, so use polling), we currently rely on sdhci_add_host() to
enable polling for us.
Here proposed a solution for such an embarrassing case. 1st, this patch will
de-couple polling enabling with sdhci_add_host() by doing this in host driver
itself, just as some other vendors. 2nd, one more patch will remove such improper
MMC_CAP_NEEDS_POLL enabling in sdhci_add_host().
Change-Id: Ia7525009d8fd188e3f0169f225e4a76ff9e94b47
Signed-off-by: yongd <redacted>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
1 file changed, 1 insertion(+)
On Tue, Oct 16, 2012 at 09:01:40PM -0700, Yong Ding wrote:
Shawn,
Thanks for your comment. And sorry for my so late due to illness:-)
SDHCI_QUIRK_BROKEN_CARD_DETECTION is used for notifying we don't use the host internal card detection method so that we don't need enable/disable those relevant interrupt bits of host(sdhci_set_card_detection in sdhci.c).
And as I double-checked the latest kernel code, actually sdhci-esdhc-imx sets this flag by default, and then will clear it only when the detection type is ESDHC_CD_CONTROLLER. So this aligns with my understanding.
What I was saying is the bit will be cleared when the detection type is
ESDHC_CD_CONTROLLE or ESDHC_CD_GPIO. You may have missed the fact that
there is no "break" in case ESDHC_CD_GPIO but a "fall through" comment.
switch (boarddata->cd_type) {
case ESDHC_CD_GPIO:
err = gpio_request_one(boarddata->cd_gpio, GPIOF_IN, "ESDHC_CD");
if (err) {
dev_err(mmc_dev(host->mmc),
"no card-detect pin available!\n");
goto no_card_detect_pin;
}
err = request_irq(gpio_to_irq(boarddata->cd_gpio), cd_irq,
IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
mmc_hostname(host->mmc), host);
if (err) {
dev_err(mmc_dev(host->mmc), "request irq error\n");
goto no_card_detect_irq;
}
/* fall through */
case ESDHC_CD_CONTROLLER:
/* we have a working card_detect back */
host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
break;
case ESDHC_CD_PERMANENT:
host->mmc->caps = MMC_CAP_NONREMOVABLE;
break;
case ESDHC_CD_NONE:
break;
}
Shawn
What I want to do is that 1st we shall set MMC_CAP_NEEDS_POLL by our host driver itself and 2nd remove the improper logic in sdhci_add_host() . How do u think? Thanks.
Shawn,
Thanks. Oh, sorry I really have missed the fact u mentioned. U are right in the current code, the bit will also be cleared for ESDHC_CD_GPIO.
But I think this is improper since for GPIO detection type, we don't use the host controller internal card detection(ESDHC_CD_CONTROLLER), but with SDHCI_QUIRK_BROKEN_CARD_DETECTION cleared, we'll still enable/disable relevant INT bits (in sdhci_set_card_detection in sdhci.c). This is my biggest concern. And I think the SDHCI_QUIRK_BROKEN_CARD_DETECTION shall be purely used to notify whether the host controller detection method is used or not. So even for the ESDHC_CD_GPIO type, we should still set this flag. How do u think?
-----Original Message-----
From: Shawn Guo [mailto:shawn.guo@linaro.org]
Sent: 2012年10月18日 13:51
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself
On Tue, Oct 16, 2012 at 09:01:40PM -0700, Yong Ding wrote:
Shawn,
Thanks for your comment. And sorry for my so late due to illness:-)
SDHCI_QUIRK_BROKEN_CARD_DETECTION is used for notifying we don't use the host internal card detection method so that we don't need enable/disable those relevant interrupt bits of host(sdhci_set_card_detection in sdhci.c).
And as I double-checked the latest kernel code, actually sdhci-esdhc-imx sets this flag by default, and then will clear it only when the detection type is ESDHC_CD_CONTROLLER. So this aligns with my understanding.
What I was saying is the bit will be cleared when the detection type is
ESDHC_CD_CONTROLLE or ESDHC_CD_GPIO. You may have missed the fact that
there is no "break" in case ESDHC_CD_GPIO but a "fall through" comment.
switch (boarddata->cd_type) {
case ESDHC_CD_GPIO:
err = gpio_request_one(boarddata->cd_gpio, GPIOF_IN, "ESDHC_CD");
if (err) {
dev_err(mmc_dev(host->mmc),
"no card-detect pin available!\n");
goto no_card_detect_pin;
}
err = request_irq(gpio_to_irq(boarddata->cd_gpio), cd_irq,
IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
mmc_hostname(host->mmc), host);
if (err) {
dev_err(mmc_dev(host->mmc), "request irq error\n");
goto no_card_detect_irq;
}
/* fall through */
case ESDHC_CD_CONTROLLER:
/* we have a working card_detect back */
host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
break;
case ESDHC_CD_PERMANENT:
host->mmc->caps = MMC_CAP_NONREMOVABLE;
break;
case ESDHC_CD_NONE:
break;
}
Shawn
What I want to do is that 1st we shall set MMC_CAP_NEEDS_POLL by our host driver itself and 2nd remove the improper logic in sdhci_add_host() . How do u think? Thanks.
Can you fix your mailer to use bottom posting rather than top posting,
and have texts wrap around column 70? Otherwise, you message stands
a good chance to be ignored by people.
On Wed, Oct 17, 2012 at 11:27:17PM -0700, Yong Ding wrote:
Shawn,
Thanks. Oh, sorry I really have missed the fact u mentioned. U are right in the current code, the bit will also be cleared for ESDHC_CD_GPIO.
But I think this is improper since for GPIO detection type, we don't use the host controller internal card detection(ESDHC_CD_CONTROLLER), but with SDHCI_QUIRK_BROKEN_CARD_DETECTION cleared, we'll still enable/disable relevant INT bits (in sdhci_set_card_detection in sdhci.c). This is my biggest concern. And I think the SDHCI_QUIRK_BROKEN_CARD_DETECTION shall be purely used to notify whether the host controller detection method is used or not. So even for the ESDHC_CD_GPIO type, we should still set this flag. How do u think?
I'm fine with that. Just remind you a fact you seem missed in case
you need to change sdhci-esdhc-imx.c to adapt the changes you are going
to make on sdhci.c.
Shawn
Shawn,
Thanks, I will update the patch for sdhci-esdhc-imx.c and also fix my mail ASAP:-)
-----Original Message-----
From: Shawn Guo [mailto:shawn.guo@linaro.org]
Sent: 2012年10月18日 14:38
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself
Can you fix your mailer to use bottom posting rather than top posting,
and have texts wrap around column 70? Otherwise, you message stands
a good chance to be ignored by people.
On Wed, Oct 17, 2012 at 11:27:17PM -0700, Yong Ding wrote:
Shawn,
Thanks. Oh, sorry I really have missed the fact u mentioned. U are right in the current code, the bit will also be cleared for ESDHC_CD_GPIO.
But I think this is improper since for GPIO detection type, we don't use the host controller internal card detection(ESDHC_CD_CONTROLLER), but with SDHCI_QUIRK_BROKEN_CARD_DETECTION cleared, we'll still enable/disable relevant INT bits (in sdhci_set_card_detection in sdhci.c). This is my biggest concern. And I think the SDHCI_QUIRK_BROKEN_CARD_DETECTION shall be purely used to notify whether the host controller detection method is used or not. So even for the ESDHC_CD_GPIO type, we should still set this flag. How do u think?
I'm fine with that. Just remind you a fact you seem missed in case
you need to change sdhci-esdhc-imx.c to adapt the changes you are going
to make on sdhci.c.
Shawn