Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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(>card_tasklet). Yes, this is not a proper fix even
it works:-)
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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. 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:-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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:-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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 --- 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
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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 yo...@marvell.com --- 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 */ @@
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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 > --- > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself
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 yo...@marvell.com --- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
