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

Wed Jul 24 04:11:45 UTC 2019

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.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.

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