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

[Baruch Siach]

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 */
+	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------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -