| Message ID | 1455198537-21791-1-git-send-email-ludovic.desroches@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11 February 2016 at 14:48, Ludovic Desroches <ludovic.desroches@atmel.com> wrote: > Add quirk broken card detection to enable card detection polling. It is > a short term solution until reworking PM stuff. > > If the card detect signal is connected to the sdhci controller and not a > gpio, when runtime PM suspend happens, we have no way to wake up on a card > detect event since these irqs are no more enabled. > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> > Fixes: f5f17813ae9b ("mmc: sdhci-of-at91: add PM support") > --- > drivers/mmc/host/sdhci-of-at91.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > index 9cb86fb..efec736 100644 > --- a/drivers/mmc/host/sdhci-of-at91.c > +++ b/drivers/mmc/host/sdhci-of-at91.c > @@ -45,6 +45,7 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = { > > static const struct sdhci_pltfm_data soc_data_sama5d2 = { > .ops = &sdhci_at91_sama5d2_ops, > + .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION, > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST, > }; > > -- > 2.7.0 > According to the below commit, SDHCI_QUIRK_BROKEN_CARD_DETECTION was invented because of unreliable card detection mechanism inside the sdhci controller. Therefore it required polling to be used, but also to make ->get_cd() to always return 1 in these cases. Although, as I understand it that's not the case here. You can still rely on card detection to work, but as you don't have wakeups you can't fully make use of card detect, when combined with runtime PM. I am not sure we should add more users of SDHCI_QUIRK_BROKEN_CARD_DETECTION, especially since in this case it's not reflecting the capability of the hardware. Can't we think of another way? Kind regards Uffe commit 68d1fb7e229c6f95be4fbbe3eb46b24e41184924 Author: Anton Vorontsov <avorontsov@ru.mvista.com> sdhci: Add support for card-detection polling This patch adds SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk. When specified, sdhci driver will set MMC_CAP_NEEDS_POLL MMC host capability, and won't enable card insert/remove interrupts. This is needed for hosts with unreliable card detection, such as FSL eSDHC. The original eSDHC driver was tring to "debounce" card-detection IRQs by reading present state and disabling particular interrupts. But with this debouncing scheme I noticed that sometimes we miss card insertion/removal events. Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> Signed-off-by: Pierre Ossman <drzeus@drzeus.cx> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 11, 2016 at 04:10:54PM +0100, Ulf Hansson wrote: > On 11 February 2016 at 14:48, Ludovic Desroches > <ludovic.desroches@atmel.com> wrote: > > Add quirk broken card detection to enable card detection polling. It is > > a short term solution until reworking PM stuff. > > > > If the card detect signal is connected to the sdhci controller and not a > > gpio, when runtime PM suspend happens, we have no way to wake up on a card > > detect event since these irqs are no more enabled. > > > > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> > > Fixes: f5f17813ae9b ("mmc: sdhci-of-at91: add PM support") > > --- > > drivers/mmc/host/sdhci-of-at91.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > > index 9cb86fb..efec736 100644 > > --- a/drivers/mmc/host/sdhci-of-at91.c > > +++ b/drivers/mmc/host/sdhci-of-at91.c > > @@ -45,6 +45,7 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = { > > > > static const struct sdhci_pltfm_data soc_data_sama5d2 = { > > .ops = &sdhci_at91_sama5d2_ops, > > + .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION, > > .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST, > > }; > > > > -- > > 2.7.0 > > > > According to the below commit, SDHCI_QUIRK_BROKEN_CARD_DETECTION was > invented because of unreliable card detection mechanism inside the > sdhci controller. > Therefore it required polling to be used, but also to make ->get_cd() > to always return 1 in these cases. > > Although, as I understand it that's not the case here. You can still > rely on card detection to work, but as you don't have wakeups you > can't fully make use of card detect, when combined with runtime PM. > I am not sure we should add more users of > SDHCI_QUIRK_BROKEN_CARD_DETECTION, especially since in this case it's > not reflecting the capability of the hardware. > > Can't we think of another way? Sorry but I am not sure to understand. In the previous thread, you told me to use MMC_CAP_NEEDS_POLL which is set if we have SDHCI_QUIRK_BROKEN_CARD_DETECTION. I was not confortable to do this because as you say it is not reflecting the capability of the hardware. Do you mean that I can simply add MMC_CAP_NEEDS_POLL after sdhci_add_host()? Regards Ludovic > > Kind regards > Uffe > > > commit 68d1fb7e229c6f95be4fbbe3eb46b24e41184924 > Author: Anton Vorontsov <avorontsov@ru.mvista.com> > sdhci: Add support for card-detection polling > > This patch adds SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk. When specified, > sdhci driver will set MMC_CAP_NEEDS_POLL MMC host capability, and won't > enable card insert/remove interrupts. > > This is needed for hosts with unreliable card detection, such as FSL > eSDHC. The original eSDHC driver was tring to "debounce" card-detection > IRQs by reading present state and disabling particular interrupts. But > with this debouncing scheme I noticed that sometimes we miss card > insertion/removal events. > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> > Signed-off-by: Pierre Ossman <drzeus@drzeus.cx> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >> According to the below commit, SDHCI_QUIRK_BROKEN_CARD_DETECTION was >> invented because of unreliable card detection mechanism inside the >> sdhci controller. >> Therefore it required polling to be used, but also to make ->get_cd() >> to always return 1 in these cases. >> >> Although, as I understand it that's not the case here. You can still >> rely on card detection to work, but as you don't have wakeups you >> can't fully make use of card detect, when combined with runtime PM. >> I am not sure we should add more users of >> SDHCI_QUIRK_BROKEN_CARD_DETECTION, especially since in this case it's >> not reflecting the capability of the hardware. >> >> Can't we think of another way? > > Sorry but I am not sure to understand. In the previous thread, you told > me to use MMC_CAP_NEEDS_POLL which is set if we have > SDHCI_QUIRK_BROKEN_CARD_DETECTION. I was not confortable to do this > because as you say it is not reflecting the capability of the hardware. > > Do you mean that I can simply add MMC_CAP_NEEDS_POLL after sdhci_add_host()? Yes, something like that, but... Within this context, I realize that the DT binding "broken-cd" has two different meanings, while comparing the generic MMC bindings towards SDHCI's. That's bad. In the SDHCI case it means, enable MMC_CAP_NEEDS_POLL *and* make ->get_cd() to always return 1 (via adding SDHCI_QUIRK_BROKEN_CARD_DETECTION). In the generic MMC case, it means only to enable MMC_CAP_NEEDS_POLL, which is exactly what you want. Perhaps you wonder why I think it's a good good idea to use DT to decide if MMC_CAP_NEEDS_POLL should be enabled? It allows flexibility for future platforms. For example, there may be platforms adding GPIO card detect support or even cards that's non-removable. I realize that the fix to solve this regression would then mean that sdhci-of-at91 need to clear SDHCI_QUIRK_BROKEN_CARD_DETECTION after parsing the shdci DTB, but then the DTB for your platform also needs an update as the "broken-cd" options needs to be set. Do you think this can work? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 12, 2016 at 12:01:39PM +0100, Ulf Hansson wrote: > >> > >> According to the below commit, SDHCI_QUIRK_BROKEN_CARD_DETECTION was > >> invented because of unreliable card detection mechanism inside the > >> sdhci controller. > >> Therefore it required polling to be used, but also to make ->get_cd() > >> to always return 1 in these cases. > >> > >> Although, as I understand it that's not the case here. You can still > >> rely on card detection to work, but as you don't have wakeups you > >> can't fully make use of card detect, when combined with runtime PM. > >> I am not sure we should add more users of > >> SDHCI_QUIRK_BROKEN_CARD_DETECTION, especially since in this case it's > >> not reflecting the capability of the hardware. > >> > >> Can't we think of another way? > > > > Sorry but I am not sure to understand. In the previous thread, you told > > me to use MMC_CAP_NEEDS_POLL which is set if we have > > SDHCI_QUIRK_BROKEN_CARD_DETECTION. I was not confortable to do this > > because as you say it is not reflecting the capability of the hardware. > > > > Do you mean that I can simply add MMC_CAP_NEEDS_POLL after sdhci_add_host()? > > Yes, something like that, but... > > Within this context, I realize that the DT binding "broken-cd" has two > different meanings, while comparing the generic MMC bindings towards > SDHCI's. That's bad. > > In the SDHCI case it means, enable MMC_CAP_NEEDS_POLL *and* make > ->get_cd() to always return 1 (via adding > SDHCI_QUIRK_BROKEN_CARD_DETECTION). > > In the generic MMC case, it means only to enable MMC_CAP_NEEDS_POLL, > which is exactly what you want. > > Perhaps you wonder why I think it's a good good idea to use DT to > decide if MMC_CAP_NEEDS_POLL should be enabled? > It allows flexibility for future platforms. For example, there may be > platforms adding GPIO card detect support or even cards that's > non-removable. I agree. > > I realize that the fix to solve this regression would then mean that > sdhci-of-at91 need to clear SDHCI_QUIRK_BROKEN_CARD_DETECTION after > parsing the shdci DTB, but then the DTB for your platform also needs > an update as the "broken-cd" options needs to be set. > > Do you think this can work? > It should but as SDHCI_QUIRK_BROKEN_CARD_DETECTION, broken-cd is not reflecting the capability of the hardware. I was thinking about checking non-removable and using mmc_gpio_get_cd() in my runtime_suspend callback. If I have a non removable device or a gpio for card detection then I can disable all clocks and call sdhci_runtime_suspend_host(). If not, I keep enabled the clock for the 'interface', disable the other one and that's all. The controller won't be set as runtime suspended but I would save some power. Does it sounds good? Regards Ludovic -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c index 9cb86fb..efec736 100644 --- a/drivers/mmc/host/sdhci-of-at91.c +++ b/drivers/mmc/host/sdhci-of-at91.c @@ -45,6 +45,7 @@ static const struct sdhci_ops sdhci_at91_sama5d2_ops = { static const struct sdhci_pltfm_data soc_data_sama5d2 = { .ops = &sdhci_at91_sama5d2_ops, + .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION, .quirks2 = SDHCI_QUIRK2_NEED_DELAY_AFTER_INT_CLK_RST, };
Add quirk broken card detection to enable card detection polling. It is a short term solution until reworking PM stuff. If the card detect signal is connected to the sdhci controller and not a gpio, when runtime PM suspend happens, we have no way to wake up on a card detect event since these irqs are no more enabled. Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> Fixes: f5f17813ae9b ("mmc: sdhci-of-at91: add PM support") --- drivers/mmc/host/sdhci-of-at91.c | 1 + 1 file changed, 1 insertion(+)