[U-Boot] [PATCH] mmd: sdhci: fix non GPIO card detect

Wed Jul 24 01:17:49 UTC 2019

> Subject: Re: [PATCH] mmd: sdhci: fix non GPIO card detect
> 
> Hi Faiz,
> 
> 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.reddy at xilinx.com>
> > >>>>>> Cc: Michal Simek <michal.simek at xilinx.com>
> > >>>>>> Signed-off-by: Baruch Siach <baruch at 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.

Regards,
Peng.

> +	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?
> 
> --
> 
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fbaruch.
> siach.name%2Fblog%2F&data=02%7C01%7Cpeng.fan%40nxp.com%7C5
> 3b8baab6aff47e5c1ba08d70f73ff94%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636994863201960255&sdata=yB2JSsQelVqgidEChJrFijg
> q1cUinIYdyR9uK3zwxM0%3D&reserved=0                  ~. .~
> Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch at tkos.co.il - tel: +972.2.679.5364,
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.tk
> os.co.il&data=02%7C01%7Cpeng.fan%40nxp.com%7C53b8baab6aff47e
> 5c1ba08d70f73ff94%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C636994863201960255&sdata=tKC8PWwGmWzKpDih3Sz5WU1U3i07dF
> fkoIVaTFPC1ro%3D&reserved=0 -