Hi Peng, On Wed, Jul 24, 2019 at 01:17:49AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH] mmd: sdhci: fix non GPIO card detect > > On Tue, Jul 23, 2019 at 04:47:47PM +0530, Faiz Abbas wrote: > > > On 23/07/19 4:16 PM, Baruch Siach wrote: > > > > On Tue, Jul 23, 2019 at 03:35:31PM +0530, Faiz Abbas wrote: > > > >> On 23/07/19 2:39 PM, Baruch Siach wrote: > > > >>> On Tue, Jul 23, 2019 at 02:27:28PM +0530, Faiz Abbas wrote: > > > >>>> On 23/07/19 1:30 PM, Peng Fan wrote: > > > >>>>> + Faiz > > > >>>>> > > > >>>>>> Subject: [PATCH] mmd: sdhci: fix non GPIO card detect > > > >>>>>> > > > >>>>>> Some SD cards do not assert the SDHCI_CARD_PRESENT bit. Only > > > >>>>>> the SDHCI_CARD_DETECT_PIN_LEVEL is enabled. Consider that > > > >>>>>> enough for card detect indication. > > > >>>>>> > > > >>>>>> This fixes SD card access from SPL, since DM_GPIO is not > > > >>>>>> available in SPL code. > > > >>>>>> > > > >>>>>> Fixes: da18c62b6e6a ("mmc: sdhci: Implement SDHCI card detect") > > > >>>>>> Cc: T Karthik Reddy <t.karthik.re...@xilinx.com> > > > >>>>>> Cc: Michal Simek <michal.si...@xilinx.com> > > > >>>>>> Signed-off-by: Baruch Siach <bar...@tkos.co.il> > > > >>>>>> --- > > > >>>>>> drivers/mmc/sdhci.c | 2 +- > > > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >>>>>> > > > >>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index > > > >>>>>> 2779bca93f08..17a28181fcca 100644 > > > >>>>>> --- a/drivers/mmc/sdhci.c > > > >>>>>> +++ b/drivers/mmc/sdhci.c > > > >>>>>> @@ -683,7 +683,7 @@ int sdhci_get_cd(struct udevice *dev) > > > >>>>>> } > > > >>>>>> #endif > > > >>>>>> value = !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & > > > >>>>>> - SDHCI_CARD_PRESENT); > > > >>>>>> + (SDHCI_CARD_PRESENT | > > SDHCI_CARD_DETECT_PIN_LEVEL)); > > > >>>>> > > > >>>>> Faiz, are you fine with this change? > > > >>>> > > > >>>> Not really. The spec is pretty clear that DETECT_PIN_LEVEL is not > > > >>>> to be trusted. Also how does the CARD_PRESENT assertion depend on > > > >>>> the SD card you use? Are you normally muxing the SDCD line to the > > > >>>> IP (for hardware to detect) or are you connecting it as a gpio which > > software must detect? > > > >>> > > > >>> I tested SanDisk 8GB SD card, class 10, UHS1, on Armada 388 based > > > >>> SolidRun Clearfog Base. The SDHCI_PRESENT_STATE register > > > >>> consistently reads 0x01f60000, that is, CARD_PRESENT is disabled, > > DETECT_PIN_LEVEL is enabled. > > > >>> > > > >>> The SD card-detect GPIO is present at the hardware level, but it > > > >>> is not accessible from SPL code because there is currently no > > > >>> SPL_DM_GPIO. The main U-Boot image detects the SD card correctly > > > >>> (once the other MMC patches I posted are applied). > > > >>> > > > >>> Without this patch boot from SD card is broken. What is the right fix > > then? > > > >> > > > >> There are two choices to implement card detect: > > > >> > > > >> 1. Mux the card detect line from the SD card cage directly to the > > > >> host controller and expect PRESENT state register to indicate > > > >> whether card is present or not. > > > >> > > > >> 2. Mux the card detect line as a GPIO and use software > > > >> (dm_gpio_get_value() call) to detect whether card is present or > > > >> not. In that case, PRESENT_STATE[16,17,18] are completely useless > > > >> because there is no card detect line going into the IP. > > > >> > > > >> It seems that you are using #2. What confuses me is how any cards > > > >> are able to assert CARD_DETECT. > > > > > > > > As far as I can see the Armada 388 SD host controller does not > > > > provide option #1. The Clearfog indeed uses option #2. Until commit > > da18c62b6e6a4 ("mmc: > > > > sdhci: Implement SDHCI card detect") it used to work because the > > > > mv_sdhci driver does not implement the get_cd callback, so > > > > card-detect was ignored. Now we have a get_cd implementation at the > > > > sdhci level that returns 0 in SPL because the GPIO is not accessible. > > > > > > > > What would you suggest? > > > > > > You can just add your own get_cd() callback instead of using > > > sdhci_get_cd(). > > > > I can do that, but I would still hit the basic problem. GPIOs are not > > accessible > > from SPL when DM is enabled. I guess that would affect a few other > > platforms. > > > > How about making an exception for SPL? Something like this: > > > > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index > > 654931a82f54..03c132631d23 100644 > > --- a/drivers/mmc/sdhci.c > > +++ b/drivers/mmc/sdhci.c > > @@ -672,6 +672,9 @@ int sdhci_get_cd(struct udevice *dev) > > /* If polling, assume that the card is always present. */ > > if (mmc->cfg->host_caps & MMC_CAP_NEEDS_POLL) > > return 1; > > + /* In SPL we have no DM_GPIO access; assume card is present */ > > I think this assumption is wrong. It is not always true the DM_GPIO not > enabled in SPL.
How can you enable DM_GPIO in SPL? The dm_gpio_get_value() code is under CONFIG_IS_ENABLED(DM_GPIO). The CONFIG_IS_ENABLED definition comment (include/linux/kconfig.h) says this: /* * CONFIG_IS_ENABLED(FOO) evaluates to * 1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y' or 'm', * 1 if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y' or 'm', * 0 otherwise. */ There is no CONFIG_SPL_DM_GPIO in current U-Boot code as of commit ff8c23e784f5. So CONFIG_IS_ENABLED(DM_GPIO) will always evaluate as 0 in SPL. baruch > > + if (IS_ENABLED(CONFIG_SPL_BUILD)) > > + return 1; > > > > #if CONFIG_IS_ENABLED(DM_GPIO) > > value = dm_gpio_get_value(&host->cd_gpio); > > > > What do you think? -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot