Hi all,
Sorry for my so late. And eventually these updated patches are here for your
review. Thanks in advance.
For patch 1, add SDHCI_QUIRK_BROKEN_CARD_DETECTION setting for ESDHC_CD_GPIO type,
or the host controller detection interrupts will be redundantly enabled. And its
comment is correspondingly updated.
For patch 2, broaden SDHCI_QUIRK_BROKEN_CARD_DETECTION setting for all detection
types except S3C_SDHCI_CD_INTERNAL. Then also update the comment.
For patch 3, no update.
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 should keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set, or host
internal card detection interrupt will still be enabled. So, here comes
the 1st change in this patch to keep SDHCI_QUIRK_BROKEN_CARD_DETECTION
set for ESDHC_CD_GPIO type. But, after the 1st change, just as above
said, sdhci_add_host() will still 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 propose the 2nd change in this patch 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 <[email protected]>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index effc2ac..ff201a5 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -557,7 +557,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
dev_err(mmc_dev(host->mmc), "request irq error\n");
goto no_card_detect_irq;
}
- /* fall through */
+ break;
case ESDHC_CD_CONTROLLER:
/* we have a working card_detect back */
@@ -569,6 +569,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
break;
case ESDHC_CD_NONE:
+ host->mmc->caps = MMC_CAP_NEEDS_POLL;
break;
}
--
1.7.9.5
SDHCI_QUIRK_BROKEN_CARD_DETECTION is used to disable host internal
card detection method. So it should be set for all card detection types
except S3C_SDHCI_CD_INTERNAL, or host internal card detection interrupts
will be redundantly enabled. So, the 1st change of this patch expands such
flag setting.
Moreover, we need move further after the above change.
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.
Apparently, this is arbitrary since we can have some other detection types.
As a result, sdhci_add_host() will redundantly enable polling for a card with
S3C_SDHCI_CD_GPIO/S3C_SDHCI_CD_EXTERNAL type.
So, we plan to remove such improper setting in sdhci_add_host(). But
before this, this 2nd change of this patch is a MUST, which enables polling
in host driver itself for S3C_SDHCI_CD_NONE type. Currently, we rely on the
above improper logic for this.
Change-Id: I28bd6765e6c6a9dd6c288ff4ca6ecf7268a2d8ed
Signed-off-by: yongd <[email protected]>
---
drivers/mmc/host/sdhci-s3c.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index a093e2e..a669616 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -683,10 +683,12 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
/* Samsung SoCs need BROKEN_ADMA_ZEROLEN_DESC */
host->quirks |= SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC;
- if (pdata->cd_type == S3C_SDHCI_CD_NONE ||
- pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
+ if (pdata->cd_type != S3C_SDHCI_CD_INTERNAL)
host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+ if (pdata->cd_type == S3C_SDHCI_CD_NONE)
+ host->mmc->caps = MMC_CAP_NEEDS_POLL;
+
if (pdata->cd_type == S3C_SDHCI_CD_PERMANENT)
host->mmc->caps = MMC_CAP_NONREMOVABLE;
--
1.7.9.5
On Tue, Oct 30, 2012 at 05:30:00PM +0800, yongd wrote:
> Sorry for my so late. And eventually these updated patches are here for your
> review. Thanks in advance.
> For patch 1, add SDHCI_QUIRK_BROKEN_CARD_DETECTION setting for ESDHC_CD_GPIO type,
> or the host controller detection interrupts will be redundantly enabled. And its
> comment is correspondingly updated.
> For patch 2, broaden SDHCI_QUIRK_BROKEN_CARD_DETECTION setting for all detection
> types except S3C_SDHCI_CD_INTERNAL. Then also update the comment.
> For patch 3, no update.
They all look OK, but note that I did not check drivers' logic, so I hope
you carefully reviewed all the drivers yourself. :)
Unfortunately, you decided to go the dangerous way, instead of taking my
100%-safe approach (i.e.
http://www.mail-archive.com/[email protected]/msg16555.html ),
which, to me, still makes more sense (of course :).
Anyways, I'm not opposed either, so hopefully it works:
Reviewed-by: Anton Vorontsov <[email protected]>
Thanks!
Anton.
On 2012年10月31日 07:11, Anton Vorontsov wrote:
> On Tue, Oct 30, 2012 at 05:30:00PM +0800, yongd wrote:
>> Sorry for my so late. And eventually these updated patches are here for your
>> review. Thanks in advance.
>> For patch 1, add SDHCI_QUIRK_BROKEN_CARD_DETECTION setting for ESDHC_CD_GPIO type,
>> or the host controller detection interrupts will be redundantly enabled. And its
>> comment is correspondingly updated.
>> For patch 2, broaden SDHCI_QUIRK_BROKEN_CARD_DETECTION setting for all detection
>> types except S3C_SDHCI_CD_INTERNAL. Then also update the comment.
>> For patch 3, no update.
> They all look OK, but note that I did not check drivers' logic, so I hope
> you carefully reviewed all the drivers yourself. :)
>
> Unfortunately, you decided to go the dangerous way, instead of taking my
> 100%-safe approach (i.e.
> http://www.mail-archive.com/[email protected]/msg16555.html ),
> which, to me, still makes more sense (of course :).
>
> Anyways, I'm not opposed either, so hopefully it works:
>
> Reviewed-by: Anton Vorontsov <[email protected]>
>
> Thanks!
> Anton.
Anton,
Thanks for your kindly review:-)
Just as I replied to u in the past (maybe got missed due to my previous
improper mail setting.
Thanks for Shawn Guo again:-)), I don't think your approach is proper.
Pls forgive me, ha ha...
Something like SDHCI_QUIRK_NEEDS_POLL means all the other detection
types are unavailable
except polling, and then if the card is removable, polling will be
enabled. Based on such meaning,
host driver will need set this flag if the host and card are of the
case. That is exactly of the detection
type like ESDHC_CD_NONE or S3C_SDHCI_CD_NONE, etc. So, why not directly
set the existing
MMC_CAP_NEEDS_POLL in host driver itself (just like Au1xmmc.c, etc)? I
think it is better than achieving
this indirectly through one more flag and some more logic code. Yes, one
more line for each host driver,
but it is very clear and save one unnecessary flag, isn't it?
BTW, currently not all SDHCI_QUIRK_BROKEN_CARD_DETECTION flags in other
host drivers are set to
notify we shall use polling since this flag has its own pure usage. So
we can not simply convert them
to the new flag u mentioned. Hope I've gotten your idea:-)
So I think my patches are the proper way:-) BTW, I've carefully review
those patches. Thanks for your
reminding.
Shawn,
Could u pls help review those patches again? Thanks in advance:-)
On 2012年10月31日 07:11, Anton Vorontsov wrote:
> On Tue, Oct 30, 2012 at 05:30:00PM +0800, yongd wrote:
>> Sorry for my so late. And eventually these updated patches are here for your
>> review. Thanks in advance.
>> For patch 1, add SDHCI_QUIRK_BROKEN_CARD_DETECTION setting for ESDHC_CD_GPIO type,
>> or the host controller detection interrupts will be redundantly enabled. And its
>> comment is correspondingly updated.
>> For patch 2, broaden SDHCI_QUIRK_BROKEN_CARD_DETECTION setting for all detection
>> types except S3C_SDHCI_CD_INTERNAL. Then also update the comment.
>> For patch 3, no update.
> They all look OK, but note that I did not check drivers' logic, so I hope
> you carefully reviewed all the drivers yourself. :)
>
> Unfortunately, you decided to go the dangerous way, instead of taking my
> 100%-safe approach (i.e.
> http://www.mail-archive.com/[email protected]/msg16555.html ),
> which, to me, still makes more sense (of course :).
>
> Anyways, I'm not opposed either, so hopefully it works:
>
> Reviewed-by: Anton Vorontsov <[email protected]>
>
> Thanks!
> Anton.
Anton,
Thanks for your kindly review:-)
Just as I replied to u in the past (maybe got missed due to my previous
improper mail setting.
Thanks for Shawn Guo again:-)), I don't think your approach is proper.
Pls forgive me, ha ha...
Something like SDHCI_QUIRK_NEEDS_POLL means all the other detection
types are unavailable
except polling, and then if the card is removable, polling will be
enabled. Based on such meaning,
host driver will need set this flag if the host and card are of the
case. That is exactly of the detection
type like ESDHC_CD_NONE or S3C_SDHCI_CD_NONE, etc. So, why not directly
set the existing
MMC_CAP_NEEDS_POLL in host driver itself (just like Au1xmmc.c, etc)? I
think it is better than achieving
this indirectly through one more flag and some more logic code. Yes, one
more line for each host driver,
but it is very clear and save one unnecessary flag, isn't it?
BTW, currently not all SDHCI_QUIRK_BROKEN_CARD_DETECTION flags in other
host drivers are set to
notify we shall use polling since this flag has its own pure usage. So
we can not simply convert them
to the new flag u mentioned. Hope I've gotten your idea:-)
So I think my patches are the proper way:-) BTW, I've carefully review
those patches. Thanks for your
reminding.
Shawn,
Could u pls help review those patches again? Thanks in advance.
On Tue, Oct 30, 2012 at 05:30:01PM +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 should keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set, or host
> internal card detection interrupt will still be enabled. So, here comes
> the 1st change in this patch to keep SDHCI_QUIRK_BROKEN_CARD_DETECTION
> set for ESDHC_CD_GPIO type. But, after the 1st change, just as above
> said, sdhci_add_host() will still enable polling for such a card.This
> is redundant.
>
This actually results in an unpleasant rather than redundant behavior.
Right after this patch gets applied and before patch #3 gets applied,
driver sdhci-esdhc-imx will use polling even for ESDHC_CD_GPIO case.
> 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 propose the 2nd change in this patch for such an embarrassing case.
Besides above, this is another sign that the patch (and series) should
be better partitioned.
> 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
Remove this, please.
> Signed-off-by: yongd <[email protected]>
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index effc2ac..ff201a5 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -557,7 +557,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
> dev_err(mmc_dev(host->mmc), "request irq error\n");
> goto no_card_detect_irq;
> }
> - /* fall through */
> + break;
The current sdhci-esdhc-imx implementation clears flag
SDHCI_QUIRK_BROKEN_CARD_DETECTION even for ESDHC_CD_GPIO case
and then emulate flag SDHCI_CARD_PRESENT by reading CD gpio state in
esdhc_readl_le(). Obviously, simply setting the flag for gpio case
does not provide an equal behavior as before.
Shawn
>
> case ESDHC_CD_CONTROLLER:
> /* we have a working card_detect back */
> @@ -569,6 +569,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
> break;
>
> case ESDHC_CD_NONE:
> + host->mmc->caps = MMC_CAP_NEEDS_POLL;
> break;
> }
>
> --
> 1.7.9.5
>
On 2012年10月31日 23:20, Shawn Guo wrote:
> On Tue, Oct 30, 2012 at 05:30:01PM +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 should keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set, or host
>> internal card detection interrupt will still be enabled. So, here comes
>> the 1st change in this patch to keep SDHCI_QUIRK_BROKEN_CARD_DETECTION
>> set for ESDHC_CD_GPIO type. But, after the 1st change, just as above
>> said, sdhci_add_host() will still enable polling for such a card.This
>> is redundant.
>>
> This actually results in an unpleasant rather than redundant behavior.
> Right after this patch gets applied and before patch #3 gets applied,
> driver sdhci-esdhc-imx will use polling even for ESDHC_CD_GPIO case.
Shawn,
I got it. So how do you think of such below partition/reorganization?
Patch-1:
For sdhci-esdhc-imx.c, only add MMC_CAP_NEEDS_POLL for ESDHC_CD_NONE. With that
improper logic of sdhci_add_host(), this is redundant, but shall be better
than something unpleasant you mentioned.
Patch-2:
For sdhci-s3c.c, do exactly the same thing as Patch-1.
Patch-3:
For sdhci.c, remove that improper logic of sdhci_add_host().
Patch-4:
For sdhci-esdhc-imx.c, set SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO.
Patch-5:
For sdhci-s3c.c, broaden SDHCI_QUIRK_BROKEN_CARD_DETECTION range for all detection
types except S3C_SDHCI_CD_INTERNAL.
>> 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 propose the 2nd change in this patch for such an embarrassing case.
> Besides above, this is another sign that the patch (and series) should
> be better partitioned.
>
>> 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
> Remove this, please.
Shawn,
I got it, and will remove such thing in updated patches. Thanks.
>> Signed-off-by: yongd <[email protected]>
>> ---
>> drivers/mmc/host/sdhci-esdhc-imx.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index effc2ac..ff201a5 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -557,7 +557,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>> dev_err(mmc_dev(host->mmc), "request irq error\n");
>> goto no_card_detect_irq;
>> }
>> - /* fall through */
>> + break;
> The current sdhci-esdhc-imx implementation clears flag
> SDHCI_QUIRK_BROKEN_CARD_DETECTION even for ESDHC_CD_GPIO case
> and then emulate flag SDHCI_CARD_PRESENT by reading CD gpio state in
> esdhc_readl_le(). Obviously, simply setting the flag for gpio case
> does not provide an equal behavior as before.
>
> Shawn
>
Shawn,
Yes, not equal as before. But you just remind me of one more improper place in our current sdhci.c.
Let's review those lines in sdhci_request() which are added by Anton long long ago in commit
68d1fb7e229c6f95be4fbbe3eb46b24e41184924(sdhci: Add support for card-detection polling),
/* If polling, assume that the card is always present. */
if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
present = true;
else
present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
SDHCI_CARD_PRESENT;
Here before sending command, if we can confirm the card dose not exist, we will return quickly without
sending this command.This is a good optimization. But if polling, we can't do such checking, so we can
only assume the card is always present.
Here is another improper example of using SDHCI_QUIRK_BROKEN_CARD_DETECTION. Exactly the same as
sdhci_add_host(), it thinks only polling and host internal card detection methods exist.
Maybe we can determine whether and how we can do such card-existence-checking optimization based on our
detection type directly rather than an indirect flag which should have its own pure meaning. But Let's
do such similar further improvement in future since currently with my patch of setting
SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, functionality is OK as before. How do u think?
>>
>> case ESDHC_CD_CONTROLLER:
>> /* we have a working card_detect back */
>> @@ -569,6 +569,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>> break;
>>
>> case ESDHC_CD_NONE:
>> + host->mmc->caps = MMC_CAP_NEEDS_POLL;
>> break;
>> }
>>
>> --
>> 1.7.9.5
>>
On Fri, Nov 02, 2012 at 08:37:41PM +0800, yongd wrote:
> I got it. So how do you think of such below partition/reorganization?
>
It looks fine to me for imx.
> Patch-1:
> For sdhci-esdhc-imx.c, only add MMC_CAP_NEEDS_POLL for ESDHC_CD_NONE. With that
> improper logic of sdhci_add_host(), this is redundant, but shall be better
> than something unpleasant you mentioned.
>
> Patch-2:
> For sdhci-s3c.c, do exactly the same thing as Patch-1.
>
> Patch-3:
> For sdhci.c, remove that improper logic of sdhci_add_host().
>
> Patch-4:
> For sdhci-esdhc-imx.c, set SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO.
>
> Patch-5:
> For sdhci-s3c.c, broaden SDHCI_QUIRK_BROKEN_CARD_DETECTION range for all detection
> types except S3C_SDHCI_CD_INTERNAL.
<snip>
> Yes, not equal as before. But you just remind me of one more improper place in our current sdhci.c.
> Let's review those lines in sdhci_request() which are added by Anton long long ago in commit
> 68d1fb7e229c6f95be4fbbe3eb46b24e41184924(sdhci: Add support for card-detection polling),
>
> /* If polling, assume that the card is always present. */
> if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
> present = true;
> else
> present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
> SDHCI_CARD_PRESENT;
>
> Here before sending command, if we can confirm the card dose not exist, we will return quickly without
> sending this command.This is a good optimization. But if polling, we can't do such checking, so we can
> only assume the card is always present.
> Here is another improper example of using SDHCI_QUIRK_BROKEN_CARD_DETECTION. Exactly the same as
> sdhci_add_host(), it thinks only polling and host internal card detection methods exist.
> Maybe we can determine whether and how we can do such card-existence-checking optimization based on our
> detection type directly rather than an indirect flag which should have its own pure meaning. But Let's
> do such similar further improvement in future since currently with my patch of setting
> SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, functionality is OK as before. How do u think?
>
I'm not sure it will function same as before. When I was testing your
v2 series, I can not see card removal message with removing card, but
can see it show up together with the next card inserting message.
Shawn
On 2012年11月05日 09:54, Shawn Guo wrote:
> On Fri, Nov 02, 2012 at 08:37:41PM +0800, yongd wrote:
>> I got it. So how do you think of such below partition/reorganization?
>>
> It looks fine to me for imx.
Ok. I will update like this way if we have no other issues. Thanks.
>
>> Patch-1:
>> For sdhci-esdhc-imx.c, only add MMC_CAP_NEEDS_POLL for ESDHC_CD_NONE. With that
>> improper logic of sdhci_add_host(), this is redundant, but shall be better
>> than something unpleasant you mentioned.
>>
>> Patch-2:
>> For sdhci-s3c.c, do exactly the same thing as Patch-1.
>>
>> Patch-3:
>> For sdhci.c, remove that improper logic of sdhci_add_host().
>>
>> Patch-4:
>> For sdhci-esdhc-imx.c, set SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO.
>>
>> Patch-5:
>> For sdhci-s3c.c, broaden SDHCI_QUIRK_BROKEN_CARD_DETECTION range for all detection
>> types except S3C_SDHCI_CD_INTERNAL.
> <snip>
>
>> Yes, not equal as before. But you just remind me of one more improper place in our current sdhci.c.
>> Let's review those lines in sdhci_request() which are added by Anton long long ago in commit
>> 68d1fb7e229c6f95be4fbbe3eb46b24e41184924(sdhci: Add support for card-detection polling),
>>
>> /* If polling, assume that the card is always present. */
>> if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
>> present = true;
>> else
>> present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
>> SDHCI_CARD_PRESENT;
>>
>> Here before sending command, if we can confirm the card dose not exist, we will return quickly without
>> sending this command.This is a good optimization. But if polling, we can't do such checking, so we can
>> only assume the card is always present.
>> Here is another improper example of using SDHCI_QUIRK_BROKEN_CARD_DETECTION. Exactly the same as
>> sdhci_add_host(), it thinks only polling and host internal card detection methods exist.
>> Maybe we can determine whether and how we can do such card-existence-checking optimization based on our
>> detection type directly rather than an indirect flag which should have its own pure meaning. But Let's
>> do such similar further improvement in future since currently with my patch of setting
>> SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, functionality is OK as before. How do u think?
>>
> I'm not sure it will function same as before. When I was testing your
> v2 series, I can not see card removal message with removing card, but
> can see it show up together with the next card inserting message.
>
> Shawn
>
Shawn,
From the code logic, without SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, when your card (using
GPIO detection) is removed, we can know the card's absence through the fake CARD_PRESENT flag in esdhc_readl_le().
As a result, we can quickly return ENOMEDIUM error without command sending. Finally mmc_rescan can know the card
is removed.
On the other hand, with SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, we will just send MMC_SEND_STATUS
command (cd_irq->sdhci_tasklet_card->mmc_recan->mmc_sd_detect->mmc_sd_alive), and we will find error since the card
is already gone. Finally, we shall also know the card is removed.
This is really strange why the removal message shows up together with the next card inserting message.
Could u pls tell us more about what actually happened when the card is removed with my patches? Did the GPIO interrupt
happen? Was mmc_rescan() called? Did mmc_sd_detect() finally find error when it sent MMC_SEND_STATUS? What step did
the card removing procedure arrive at? Really really thanks a lot:-)
On Mon, Nov 05, 2012 at 11:34:49AM +0800, yongd wrote:
> From the code logic, without SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, when your card (using
> GPIO detection) is removed, we can know the card's absence through the fake CARD_PRESENT flag in esdhc_readl_le().
> As a result, we can quickly return ENOMEDIUM error without command sending. Finally mmc_rescan can know the card
> is removed.
>
Yes, that's the existing implementation, which does not require retry
sending MMC_SEND_STATUS to know if card is removed.
> On the other hand, with SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, we will just send MMC_SEND_STATUS
> command (cd_irq->sdhci_tasklet_card->mmc_recan->mmc_sd_detect->mmc_sd_alive), and we will find error since the card
> is already gone. Finally, we shall also know the card is removed.
>
> This is really strange why the removal message shows up together with the next card inserting message.
> Could u pls tell us more about what actually happened when the card is removed with my patches? Did the GPIO interrupt
> happen? Was mmc_rescan() called? Did mmc_sd_detect() finally find error when it sent MMC_SEND_STATUS? What step did
> the card removing procedure arrive at? Really really thanks a lot:-)
>
I just tracked it down a little bit. Interesting enough, it depends on
how I remove the card. If I do it slowly, when the gpio interrupt
triggers the MMC_SEND_STATUS query, the command can still retrieve
the card status successfully. Thus driver does not detect the card
removal. If I remove the card from slot quickly, I can see the removal
message.
With your patch series, in ESDHC_CD_GPIO case the driver will have
to send MMC_SEND_STATUS (with retry) to card for knowing its presence.
This is also an unpleasant behavior comparing to the existing code.
I think we should query gpio state to know card presence for this case.
Shawn
On 2012年11月05日 20:48, Shawn Guo wrote:
> On Mon, Nov 05, 2012 at 11:34:49AM +0800, yongd wrote:
>> From the code logic, without SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, when your card (using
>> GPIO detection) is removed, we can know the card's absence through the fake CARD_PRESENT flag in esdhc_readl_le().
>> As a result, we can quickly return ENOMEDIUM error without command sending. Finally mmc_rescan can know the card
>> is removed.
>>
> Yes, that's the existing implementation, which does not require retry
> sending MMC_SEND_STATUS to know if card is removed.
>
>> On the other hand, with SDHCI_QUIRK_BROKEN_CARD_DETECTION for ESDHC_CD_GPIO, we will just send MMC_SEND_STATUS
>> command (cd_irq->sdhci_tasklet_card->mmc_recan->mmc_sd_detect->mmc_sd_alive), and we will find error since the card
>> is already gone. Finally, we shall also know the card is removed.
>>
>> This is really strange why the removal message shows up together with the next card inserting message.
>> Could u pls tell us more about what actually happened when the card is removed with my patches? Did the GPIO interrupt
>> happen? Was mmc_rescan() called? Did mmc_sd_detect() finally find error when it sent MMC_SEND_STATUS? What step did
>> the card removing procedure arrive at? Really really thanks a lot:-)
>>
> I just tracked it down a little bit. Interesting enough, it depends on
> how I remove the card. If I do it slowly, when the gpio interrupt
> triggers the MMC_SEND_STATUS query, the command can still retrieve
> the card status successfully. Thus driver does not detect the card
> removal. If I remove the card from slot quickly, I can see the removal
> message.
Shawn,
Thanks for your interesting input.
From your info, we can see that on your platform, those pins (including
power, clk, DATA) necessary for MMC_SEND_STATUS transaction still keep
connected for some time just after the GPIO's level changes due to card
removable. And if we remove the card very slowly, such time duration can be
such long that the MMC_SEND_STATUS query can still succeed.
So I think we can add a proper delay(maybe 100ms) before the gpio interrupt
triggers the MMC_SEND_STATUS query, and maybe this can probably fix this issue:-)
> With your patch series, in ESDHC_CD_GPIO case the driver will have
> to send MMC_SEND_STATUS (with retry) to card for knowing its presence.
> This is also an unpleasant behavior comparing to the existing code.
> I think we should query gpio state to know card presence for this case.
>
> Shawn
>
Anyway, I 100% agree with you that for a ESDHC_CD_GPIO card, we shall query gpio
state to know such card's presence rather than sending MMC_SEND_STATUS rudely.
But just as I mentioned before, I don't think using SDHCI_QUIRK_BROKEN_CARD_DETECTION
as the flag to determine whether and how we can know card's presence before sending
command is a proper way.
I haven't gotten any good idea. Do u have any idea on this?
On Tue, Nov 06, 2012 at 04:49:42PM +0800, yongd wrote:
> From your info, we can see that on your platform, those pins (including
> power, clk, DATA) necessary for MMC_SEND_STATUS transaction still keep
> connected for some time just after the GPIO's level changes due to card
> removable. And if we remove the card very slowly, such time duration can be
> such long that the MMC_SEND_STATUS query can still succeed.
>
I was not removing the card as slowly as you think. It's actually
a normal speed. That's why I thought your patch breaks the
card-detection functionality before I found the cause.
> So I think we can add a proper delay(maybe 100ms) before the gpio interrupt
> triggers the MMC_SEND_STATUS query, and maybe this can probably fix this issue:-)
>
I do not think it's a proper fixing.
<snip>
> Anyway, I 100% agree with you that for a ESDHC_CD_GPIO card, we shall query gpio
> state to know such card's presence rather than sending MMC_SEND_STATUS rudely.
>
> But just as I mentioned before, I don't think using SDHCI_QUIRK_BROKEN_CARD_DETECTION
> as the flag to determine whether and how we can know card's presence before sending
> command is a proper way.
>
> I haven't gotten any good idea. Do u have any idea on this?
>
I guess what we need is to call mmc_gpio_get_cd() trying to know card's
presence before sending MMC_SEND_STATUS command. sdhci-esdhc-imx
driver will surely need some changes to cope with that.
Shawn
On 2012年11月06日 20:52, Shawn Guo wrote:
> On Tue, Nov 06, 2012 at 04:49:42PM +0800, yongd wrote:
>> From your info, we can see that on your platform, those pins (including
>> power, clk, DATA) necessary for MMC_SEND_STATUS transaction still keep
>> connected for some time just after the GPIO's level changes due to card
>> removable. And if we remove the card very slowly, such time duration can be
>> such long that the MMC_SEND_STATUS query can still succeed.
>>
> I was not removing the card as slowly as you think. It's actually
> a normal speed. That's why I thought your patch breaks the
> card-detection functionality before I found the cause.
>
>> So I think we can add a proper delay(maybe 100ms) before the gpio interrupt
>> triggers the MMC_SEND_STATUS query, and maybe this can probably fix this issue:-)
>>
> I do not think it's a proper fixing.
Anyway, u can try such delay like msleep(100) in cd_irq() before calling
tasklet_schedule(&sdhost->card_tasklet). Yes, this is not a proper fix even
it works:-)
>
> <snip>
>
>> Anyway, I 100% agree with you that for a ESDHC_CD_GPIO card, we shall query gpio
>> state to know such card's presence rather than sending MMC_SEND_STATUS rudely.
>>
>> But just as I mentioned before, I don't think using SDHCI_QUIRK_BROKEN_CARD_DETECTION
>> as the flag to determine whether and how we can know card's presence before sending
>> command is a proper way.
>>
>> I haven't gotten any good idea. Do u have any idea on this?
>>
> I guess what we need is to call mmc_gpio_get_cd() trying to know card's
> presence before sending MMC_SEND_STATUS command. sdhci-esdhc-imx
> driver will surely need some changes to cope with that.
>
> Shawn
>
Yes, gpio card detection should better use the existing framework
offered by slot-gpio.
Then the fake-card-present will be unnecessary. BTW, sdhci-s3c.c also
dose not use slot-gpio,
and then it also adds some tricky logic for gpio detection. U can check
sdhci_s3c_notify_change(),
which dynamically set/clear SDHCI_QUIRK_BROKEN_CARD_DETECTION. This
patch adding
mmc_gpio_get_cd(), bec9d4e5939987053169a9bb48fc58b6a2d3e237, mentioned
this 1stly.
But using SDHCI_QUIRK_BROKEN_CARD_DETECTION to do such judging in
sdhci_request()
is still not proper. I think this is the root causing such above
workarounds.
So I am thinking of adding a new operation like get_card_presence into
sdhci_ops,
and then different host drivers can implement differently by themselves,
eg, for
sdhci-esdhc-imx.c,
static bool esdhc_get_card_presence(struct sdhci_host *host)
{
bool present = true;
if (detection_type == ESDHC_CD_CONTROLLER)
present = sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT;
else if (detection_type == ESDHC_CD_GPIO) {
if (gpio_get_value(boarddata->cd_gpio))
/* no card, if a valid gpio says so... */
present = false;
}
return present;
}
But this will also cause lots of host drivers corresponding changes. Oh,
still inconvenient:-(
Any better ideas?