| Message ID | 20240415070620.133143-1-richard.xnu.clark@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sdhci: Fix SD card detection issue | expand |
On 15/04/24 10:06, Richard Clark wrote: > The mmc_gpio_get_cd(...) will return 0 called from sdhci_get_cd(...), which means > the card is not present. Actually, the card detection pin is active low by default > according to the SDHCI psec, thus the card detection result is not correct, more SDHCI spec covers the SDHCI lines. GPIO is separate. > specificly below if condition is true in mmc_rescan(...): > ... > if (mmc_card_is_removable(host) && host->ops->get_cd && > host->ops->get_cd(host) == 0) { > ... > goto out; > } > The SD card device will have no chance to be created. > > This commit fixes this detection issue via the MMC_CAP2_CD_ACTIVE_HIGH cap2 flag, > parsed from the 'cd-inverted' property of DT. What hardware / driver is it? > > Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com> > --- > drivers/mmc/host/sdhci.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index c79f73459915..79f33a161ca8 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2483,6 +2483,9 @@ static int sdhci_get_cd(struct mmc_host *mmc) > * Try slot gpio detect, if defined it take precedence > * over build in controller functionality > */ > + if (!(mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)) > + gpio_cd = !gpio_cd; MMC_CAP2_CD_ACTIVE_HIGH is already handled in mmc_gpiod_request_cd(), and this turns an error (gpio_cd < 0) into 0, which is not right. > + > if (gpio_cd >= 0) > return !!gpio_cd; >
On Mon, Apr 15, 2024 at 10:18:39AM +0300, Adrian Hunter wrote: > On 15/04/24 10:06, Richard Clark wrote: > > The mmc_gpio_get_cd(...) will return 0 called from sdhci_get_cd(...), which means > > the card is not present. Actually, the card detection pin is active low by default > > according to the SDHCI psec, thus the card detection result is not correct, more > > SDHCI spec covers the SDHCI lines. GPIO is separate. ... and the key bit of information that should be mentioned is in the case of a GPIO, the GPIO library can be told if a GPIO is active-high or active-low in either firmware or via the GPIO lookup data, and this should be used instead of drivers inventing their own "quirking".
On Mon, Apr 15, 2024 at 3:18 PM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 15/04/24 10:06, Richard Clark wrote: > > The mmc_gpio_get_cd(...) will return 0 called from sdhci_get_cd(...), which means > > the card is not present. Actually, the card detection pin is active low by default > > according to the SDHCI psec, thus the card detection result is not correct, more > > SDHCI spec covers the SDHCI lines. GPIO is separate. > > > specificly below if condition is true in mmc_rescan(...): > > ... > > if (mmc_card_is_removable(host) && host->ops->get_cd && > > host->ops->get_cd(host) == 0) { > > ... > > goto out; > > } > > The SD card device will have no chance to be created. > > > > This commit fixes this detection issue via the MMC_CAP2_CD_ACTIVE_HIGH cap2 flag, > > parsed from the 'cd-inverted' property of DT. > > What hardware / driver is it? sdhci-tegra on Orin. > > > > Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com> > > --- > > drivers/mmc/host/sdhci.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index c79f73459915..79f33a161ca8 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2483,6 +2483,9 @@ static int sdhci_get_cd(struct mmc_host *mmc) > > * Try slot gpio detect, if defined it take precedence > > * over build in controller functionality > > */ > > + if (!(mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)) > > + gpio_cd = !gpio_cd; > > MMC_CAP2_CD_ACTIVE_HIGH is already handled in > mmc_gpiod_request_cd(), and this turns an error (gpio_cd < 0) > into 0, which is not right. But in case of 'cd-inverted' is not specified, the gpio CD pin return 0 which will be explained as card is not present. > > > + > > if (gpio_cd >= 0) > > return !!gpio_cd; > > >
On Mon, Apr 15, 2024 at 3:22 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Mon, Apr 15, 2024 at 10:18:39AM +0300, Adrian Hunter wrote: > > On 15/04/24 10:06, Richard Clark wrote: > > > The mmc_gpio_get_cd(...) will return 0 called from sdhci_get_cd(...), which means > > > the card is not present. Actually, the card detection pin is active low by default > > > according to the SDHCI psec, thus the card detection result is not correct, more > > > > SDHCI spec covers the SDHCI lines. GPIO is separate. > > ... and the key bit of information that should be mentioned is in the > case of a GPIO, the GPIO library can be told if a GPIO is active-high > or active-low in either firmware or via the GPIO lookup data, and this > should be used instead of drivers inventing their own "quirking". > Agree! But unfortunately, it seems I can't find the right place to handle this from either firmware or via the GPIO lookup data. Will be appreciated if any suggestion about that?! > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Apr 15, 2024 at 04:17:14PM +0800, richard clark wrote: > On Mon, Apr 15, 2024 at 3:22 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Mon, Apr 15, 2024 at 10:18:39AM +0300, Adrian Hunter wrote: > > > On 15/04/24 10:06, Richard Clark wrote: > > > > The mmc_gpio_get_cd(...) will return 0 called from sdhci_get_cd(...), which means > > > > the card is not present. Actually, the card detection pin is active low by default > > > > according to the SDHCI psec, thus the card detection result is not correct, more > > > > > > SDHCI spec covers the SDHCI lines. GPIO is separate. > > > > ... and the key bit of information that should be mentioned is in the > > case of a GPIO, the GPIO library can be told if a GPIO is active-high > > or active-low in either firmware or via the GPIO lookup data, and this > > should be used instead of drivers inventing their own "quirking". > > > Agree! But unfortunately, it seems I can't find the right place to > handle this from either firmware or via the GPIO lookup data. Will be > appreciated if any suggestion about that?! If you're using DT, then, for example: cd-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>; is all it takes. If you are using firmware then GPIO lookup data isn't what you should be using. I'm afraid I don't know the ACPI bindings for SDHCI.
+Nvidia guys On 15/04/24 11:11, richard clark wrote: > On Mon, Apr 15, 2024 at 3:18 PM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 15/04/24 10:06, Richard Clark wrote: >>> The mmc_gpio_get_cd(...) will return 0 called from sdhci_get_cd(...), which means >>> the card is not present. Actually, the card detection pin is active low by default >>> according to the SDHCI psec, thus the card detection result is not correct, more >> >> SDHCI spec covers the SDHCI lines. GPIO is separate. >> >>> specificly below if condition is true in mmc_rescan(...): >>> ... >>> if (mmc_card_is_removable(host) && host->ops->get_cd && >>> host->ops->get_cd(host) == 0) { >>> ... >>> goto out; >>> } >>> The SD card device will have no chance to be created. >>> >>> This commit fixes this detection issue via the MMC_CAP2_CD_ACTIVE_HIGH cap2 flag, >>> parsed from the 'cd-inverted' property of DT. >> >> What hardware / driver is it? > sdhci-tegra on Orin. >>> >>> Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com> >>> --- >>> drivers/mmc/host/sdhci.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> index c79f73459915..79f33a161ca8 100644 >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -2483,6 +2483,9 @@ static int sdhci_get_cd(struct mmc_host *mmc) >>> * Try slot gpio detect, if defined it take precedence >>> * over build in controller functionality >>> */ >>> + if (!(mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)) >>> + gpio_cd = !gpio_cd; >> >> MMC_CAP2_CD_ACTIVE_HIGH is already handled in >> mmc_gpiod_request_cd(), and this turns an error (gpio_cd < 0) >> into 0, which is not right. > > But in case of 'cd-inverted' is not specified, the gpio CD pin return > 0 which will be explained as card is not present. >> >>> + >>> if (gpio_cd >= 0) >>> return !!gpio_cd; >>> >>
On Mon, Apr 15, 2024 at 4:23 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Mon, Apr 15, 2024 at 04:17:14PM +0800, richard clark wrote: > > On Mon, Apr 15, 2024 at 3:22 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > > > > On Mon, Apr 15, 2024 at 10:18:39AM +0300, Adrian Hunter wrote: > > > > On 15/04/24 10:06, Richard Clark wrote: > > > > > The mmc_gpio_get_cd(...) will return 0 called from sdhci_get_cd(...), which means > > > > > the card is not present. Actually, the card detection pin is active low by default > > > > > according to the SDHCI psec, thus the card detection result is not correct, more > > > > > > > > SDHCI spec covers the SDHCI lines. GPIO is separate. > > > > > > ... and the key bit of information that should be mentioned is in the > > > case of a GPIO, the GPIO library can be told if a GPIO is active-high > > > or active-low in either firmware or via the GPIO lookup data, and this > > > should be used instead of drivers inventing their own "quirking". > > > > > Agree! But unfortunately, it seems I can't find the right place to > > handle this from either firmware or via the GPIO lookup data. Will be > > appreciated if any suggestion about that?! > > If you're using DT, then, for example: > > cd-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>; > > is all it takes. If you are using firmware then GPIO lookup data isn't > what you should be using. I'm afraid I don't know the ACPI bindings for > SDHCI. > Ah, this seems to be a bug of the Nvidia DT, its cd-gpios=<... 0x00> meaning the GPIO_ACTIVE_HIGH, but the CD gpio value is 0 when the card is inserted. In the kernel v5.10, the sdhci-tegra use below logic as the card present indicator: if (!host->mmc->cd_cap_invert) host->mmc->rem_card_present = (mmc_gpio_get_cd(host->mmc) == 0); But the newer version kernel removes the 'rem_card_present', and the CD gpio still value 0 will be interpreted as the card is not present, thus the issue happens... > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index c79f73459915..79f33a161ca8 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2483,6 +2483,9 @@ static int sdhci_get_cd(struct mmc_host *mmc) * Try slot gpio detect, if defined it take precedence * over build in controller functionality */ + if (!(mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)) + gpio_cd = !gpio_cd; + if (gpio_cd >= 0) return !!gpio_cd;
The mmc_gpio_get_cd(...) will return 0 called from sdhci_get_cd(...), which means the card is not present. Actually, the card detection pin is active low by default according to the SDHCI psec, thus the card detection result is not correct, more specificly below if condition is true in mmc_rescan(...): ... if (mmc_card_is_removable(host) && host->ops->get_cd && host->ops->get_cd(host) == 0) { ... goto out; } The SD card device will have no chance to be created. This commit fixes this detection issue via the MMC_CAP2_CD_ACTIVE_HIGH cap2 flag, parsed from the 'cd-inverted' property of DT. Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com> --- drivers/mmc/host/sdhci.c | 3 +++ 1 file changed, 3 insertions(+)