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

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

[Baruch Siach]

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@xilinx.com>
Cc: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Baruch Siach <baruch@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));
 	if (mmc->cfg->host_caps & MMC_CAP_CD_ACTIVE_HIGH)
 		return !value;
 	else
-- 
2.20.1

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

[Peng Fan]

+ 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@xilinx.com>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Baruch Siach <baruch@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?

Thanks,
Peng.


>  	if (mmc->cfg->host_caps & MMC_CAP_CD_ACTIVE_HIGH)
>  		return !value;
>  	else
> --
> 2.20.1

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

[Faiz Abbas]

Hi,

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@xilinx.com>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Signed-off-by: Baruch Siach <baruch@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?

Thanks,
Faiz

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

[Baruch Siach]

Hi Faiz,

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@xilinx.com>
> >> Cc: Michal Simek <michal.simek@xilinx.com>
> >> Signed-off-by: Baruch Siach <baruch@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?

baruch

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

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

[Faiz Abbas]

Hi Baruch,

On 23/07/19 2:39 PM, Baruch Siach wrote:
> Hi Faiz,
> 
> 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@xilinx.com>
>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>> Signed-off-by: Baruch Siach <baruch@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.

Thanks,
Faiz

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

[Baruch Siach]

Hi Faiz,

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@xilinx.com>
> >>>> Cc: Michal Simek <michal.simek@xilinx.com>
> >>>> Signed-off-by: Baruch Siach <baruch@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?

baruch

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

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

[Faiz Abbas]

Hi Baruch,

On 23/07/19 4:16 PM, Baruch Siach wrote:
> Hi Faiz,
> 
> 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@xilinx.com>
>>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>>> Signed-off-by: Baruch Siach <baruch@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().

Thanks,
Faiz

[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@xilinx.com>
> >>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
> >>>>>> Signed-off-by: Baruch Siach <baruch@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 -

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

[Peng Fan]

> 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@xilinx.com>
> > >>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
> > >>>>>> Signed-off-by: Baruch Siach <baruch@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&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7C5
> 3b8baab6aff47e5c1ba08d70f73ff94%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636994863201960255&amp;sdata=yB2JSsQelVqgidEChJrFijg
> q1cUinIYdyR9uK3zwxM0%3D&amp;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&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7C53b8baab6aff47e
> 5c1ba08d70f73ff94%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C636994863201960255&amp;sdata=tKC8PWwGmWzKpDih3Sz5WU1U3i07dF
> fkoIVaTFPC1ro%3D&amp;reserved=0 -

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

[Baruch Siach]

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@xilinx.com>
> > > >>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
> > > >>>>>> Signed-off-by: Baruch Siach <baruch@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 -