[PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Robert Marko]

Generic SDHCI driver received support for checking the busy status by
polling the DAT[0] level instead of waiting for the worst MMC switch time.

Unfortunately, it appears that this does not work for Xenon controllers
despite being a part of the standard SDHCI registers and the Armada 3720
datasheet itself telling that BIT(20) is useful for detecting the DAT[0]
busy signal.

I have tried increasing the timeout value, but I have newer managed to
catch DAT_LEVEL bits change from 0 at all.

This issue appears to hit most if not all SoC-s supported by Xenon driver,
at least A3720, A8040 and CN9130 have non working eMMC currently.

So, until a better solution is found drop the wait_dat0 OP for Xenon.
I was able to only test it on A3720, but it should work for others as well.

Fixes: 40e6f52454fc ("drivers: mmc: Add wait_dat0 support for sdhci driver")
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 drivers/mmc/xenon_sdhci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/xenon_sdhci.c b/drivers/mmc/xenon_sdhci.c
index e292f2903d..2f8805096c 100644
--- a/drivers/mmc/xenon_sdhci.c
+++ b/drivers/mmc/xenon_sdhci.c
@@ -439,6 +439,8 @@ static const struct sdhci_ops xenon_sdhci_ops = {
 	.set_ios_post = xenon_sdhci_set_ios_post
 };
 
+static struct dm_mmc_ops xenon_mmc_ops;
+
 static int xenon_sdhci_probe(struct udevice *dev)
 {
 	struct xenon_sdhci_plat *plat = dev_get_plat(dev);
@@ -452,6 +454,9 @@ static int xenon_sdhci_probe(struct udevice *dev)
 	host->mmc->dev = dev;
 	upriv->mmc = host->mmc;
 
+	xenon_mmc_ops = sdhci_ops;
+	xenon_mmc_ops.wait_dat0 = NULL;
+
 	/* Set quirks */
 	host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD | SDHCI_QUIRK_32BIT_DMA_ADDR;
 
@@ -568,7 +573,7 @@ U_BOOT_DRIVER(xenon_sdhci_drv) = {
 	.id		= UCLASS_MMC,
 	.of_match	= xenon_sdhci_ids,
 	.of_to_plat = xenon_sdhci_of_to_plat,
-	.ops		= &sdhci_ops,
+	.ops		= &xenon_mmc_ops,
 	.bind		= xenon_sdhci_bind,
 	.probe		= xenon_sdhci_probe,
 	.remove		= xenon_sdhci_remove,
-- 
2.35.1

Re: [PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Pali Rohár]

+ Marek

Xenon eMMC is broken in U-Boot, could you check / verify it on A3720 eMMC based board?

On Friday 11 March 2022 19:14:07 Robert Marko wrote:
> Generic SDHCI driver received support for checking the busy status by
> polling the DAT[0] level instead of waiting for the worst MMC switch time.
> 
> Unfortunately, it appears that this does not work for Xenon controllers
> despite being a part of the standard SDHCI registers and the Armada 3720
> datasheet itself telling that BIT(20) is useful for detecting the DAT[0]
> busy signal.
> 
> I have tried increasing the timeout value, but I have newer managed to
> catch DAT_LEVEL bits change from 0 at all.
> 
> This issue appears to hit most if not all SoC-s supported by Xenon driver,
> at least A3720, A8040 and CN9130 have non working eMMC currently.
> 
> So, until a better solution is found drop the wait_dat0 OP for Xenon.
> I was able to only test it on A3720, but it should work for others as well.
> 
> Fixes: 40e6f52454fc ("drivers: mmc: Add wait_dat0 support for sdhci driver")
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  drivers/mmc/xenon_sdhci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/xenon_sdhci.c b/drivers/mmc/xenon_sdhci.c
> index e292f2903d..2f8805096c 100644
> --- a/drivers/mmc/xenon_sdhci.c
> +++ b/drivers/mmc/xenon_sdhci.c
> @@ -439,6 +439,8 @@ static const struct sdhci_ops xenon_sdhci_ops = {
>  	.set_ios_post = xenon_sdhci_set_ios_post
>  };
>  
> +static struct dm_mmc_ops xenon_mmc_ops;
> +
>  static int xenon_sdhci_probe(struct udevice *dev)
>  {
>  	struct xenon_sdhci_plat *plat = dev_get_plat(dev);
> @@ -452,6 +454,9 @@ static int xenon_sdhci_probe(struct udevice *dev)
>  	host->mmc->dev = dev;
>  	upriv->mmc = host->mmc;
>  
> +	xenon_mmc_ops = sdhci_ops;
> +	xenon_mmc_ops.wait_dat0 = NULL;
> +
>  	/* Set quirks */
>  	host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD | SDHCI_QUIRK_32BIT_DMA_ADDR;
>  
> @@ -568,7 +573,7 @@ U_BOOT_DRIVER(xenon_sdhci_drv) = {
>  	.id		= UCLASS_MMC,
>  	.of_match	= xenon_sdhci_ids,
>  	.of_to_plat = xenon_sdhci_of_to_plat,
> -	.ops		= &sdhci_ops,
> +	.ops		= &xenon_mmc_ops,
>  	.bind		= xenon_sdhci_bind,
>  	.probe		= xenon_sdhci_probe,
>  	.remove		= xenon_sdhci_remove,
> -- 
> 2.35.1
> 

Re: [PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Marek Behún]

On Fri, 11 Mar 2022 19:52:40 +0100
Pali Rohár <pali@kernel.org> wrote:

> + Marek
> 
> Xenon eMMC is broken in U-Boot, could you check / verify it on A3720 eMMC based board?

I can confirm this. I also had to workaround this issue, I reverted the
commit adding support for wait_dat0 in my internal repo.

For now I am pro this solution, so

Reviewed-by: Marek Behún <marek.behun@nic.cz>

Marek

Re: [PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Jaehoon Chung]

On 3/12/22 03:14, Robert Marko wrote:
> Generic SDHCI driver received support for checking the busy status by
> polling the DAT[0] level instead of waiting for the worst MMC switch time.
> 
> Unfortunately, it appears that this does not work for Xenon controllers
> despite being a part of the standard SDHCI registers and the Armada 3720
> datasheet itself telling that BIT(20) is useful for detecting the DAT[0]
> busy signal.
> 
> I have tried increasing the timeout value, but I have newer managed to
> catch DAT_LEVEL bits change from 0 at all.
> 
> This issue appears to hit most if not all SoC-s supported by Xenon driver,
> at least A3720, A8040 and CN9130 have non working eMMC currently.
> 
> So, until a better solution is found drop the wait_dat0 OP for Xenon.
> I was able to only test it on A3720, but it should work for others as well.
> 
> Fixes: 40e6f52454fc ("drivers: mmc: Add wait_dat0 support for sdhci driver")
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
>  drivers/mmc/xenon_sdhci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/xenon_sdhci.c b/drivers/mmc/xenon_sdhci.c
> index e292f2903d..2f8805096c 100644
> --- a/drivers/mmc/xenon_sdhci.c
> +++ b/drivers/mmc/xenon_sdhci.c
> @@ -439,6 +439,8 @@ static const struct sdhci_ops xenon_sdhci_ops = {
>  	.set_ios_post = xenon_sdhci_set_ios_post
>  };
>  
> +static struct dm_mmc_ops xenon_mmc_ops;
> +
>  static int xenon_sdhci_probe(struct udevice *dev)
>  {
>  	struct xenon_sdhci_plat *plat = dev_get_plat(dev);
> @@ -452,6 +454,9 @@ static int xenon_sdhci_probe(struct udevice *dev)
>  	host->mmc->dev = dev;
>  	upriv->mmc = host->mmc;
>  
> +	xenon_mmc_ops = sdhci_ops;
> +	xenon_mmc_ops.wait_dat0 = NULL;
> +
>  	/* Set quirks */
>  	host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD | SDHCI_QUIRK_32BIT_DMA_ADDR;
>  
> @@ -568,7 +573,7 @@ U_BOOT_DRIVER(xenon_sdhci_drv) = {
>  	.id		= UCLASS_MMC,
>  	.of_match	= xenon_sdhci_ids,
>  	.of_to_plat = xenon_sdhci_of_to_plat,
> -	.ops		= &sdhci_ops,
> +	.ops		= &xenon_mmc_ops,
>  	.bind		= xenon_sdhci_bind,
>  	.probe		= xenon_sdhci_probe,
>  	.remove		= xenon_sdhci_remove,

Re: [PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Stefan Roese]

On 3/11/22 19:14, Robert Marko wrote:
> Generic SDHCI driver received support for checking the busy status by
> polling the DAT[0] level instead of waiting for the worst MMC switch time.
> 
> Unfortunately, it appears that this does not work for Xenon controllers
> despite being a part of the standard SDHCI registers and the Armada 3720
> datasheet itself telling that BIT(20) is useful for detecting the DAT[0]
> busy signal.
> 
> I have tried increasing the timeout value, but I have newer managed to
> catch DAT_LEVEL bits change from 0 at all.
> 
> This issue appears to hit most if not all SoC-s supported by Xenon driver,
> at least A3720, A8040 and CN9130 have non working eMMC currently.
> 
> So, until a better solution is found drop the wait_dat0 OP for Xenon.
> I was able to only test it on A3720, but it should work for others as well.
> 
> Fixes: 40e6f52454fc ("drivers: mmc: Add wait_dat0 support for sdhci driver")
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/mmc/xenon_sdhci.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/xenon_sdhci.c b/drivers/mmc/xenon_sdhci.c
> index e292f2903d..2f8805096c 100644
> --- a/drivers/mmc/xenon_sdhci.c
> +++ b/drivers/mmc/xenon_sdhci.c
> @@ -439,6 +439,8 @@ static const struct sdhci_ops xenon_sdhci_ops = {
>   	.set_ios_post = xenon_sdhci_set_ios_post
>   };
>   
> +static struct dm_mmc_ops xenon_mmc_ops;
> +
>   static int xenon_sdhci_probe(struct udevice *dev)
>   {
>   	struct xenon_sdhci_plat *plat = dev_get_plat(dev);
> @@ -452,6 +454,9 @@ static int xenon_sdhci_probe(struct udevice *dev)
>   	host->mmc->dev = dev;
>   	upriv->mmc = host->mmc;
>   
> +	xenon_mmc_ops = sdhci_ops;
> +	xenon_mmc_ops.wait_dat0 = NULL;
> +
>   	/* Set quirks */
>   	host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD | SDHCI_QUIRK_32BIT_DMA_ADDR;
>   
> @@ -568,7 +573,7 @@ U_BOOT_DRIVER(xenon_sdhci_drv) = {
>   	.id		= UCLASS_MMC,
>   	.of_match	= xenon_sdhci_ids,
>   	.of_to_plat = xenon_sdhci_of_to_plat,
> -	.ops		= &sdhci_ops,
> +	.ops		= &xenon_mmc_ops,
>   	.bind		= xenon_sdhci_bind,
>   	.probe		= xenon_sdhci_probe,
>   	.remove		= xenon_sdhci_remove,

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On Mon, Mar 14, 2022 at 06:37:02PM +0900, Jaehoon Chung wrote:
> On 3/12/22 03:14, Robert Marko wrote:
> > Generic SDHCI driver received support for checking the busy status by
> > polling the DAT[0] level instead of waiting for the worst MMC switch time.
> > 
> > Unfortunately, it appears that this does not work for Xenon controllers
> > despite being a part of the standard SDHCI registers and the Armada 3720
> > datasheet itself telling that BIT(20) is useful for detecting the DAT[0]
> > busy signal.
> > 
> > I have tried increasing the timeout value, but I have newer managed to
> > catch DAT_LEVEL bits change from 0 at all.
> > 
> > This issue appears to hit most if not all SoC-s supported by Xenon driver,
> > at least A3720, A8040 and CN9130 have non working eMMC currently.
> > 
> > So, until a better solution is found drop the wait_dat0 OP for Xenon.
> > I was able to only test it on A3720, but it should work for others as well.
> > 
> > Fixes: 40e6f52454fc ("drivers: mmc: Add wait_dat0 support for sdhci driver")
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> 
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Since this is a regression fix, will this be in the PR with the imx fix
as well?  Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Robert Marko]

On Mon, Mar 14, 2022 at 2:10 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Mar 14, 2022 at 06:37:02PM +0900, Jaehoon Chung wrote:
> > On 3/12/22 03:14, Robert Marko wrote:
> > > Generic SDHCI driver received support for checking the busy status by
> > > polling the DAT[0] level instead of waiting for the worst MMC switch time.
> > >
> > > Unfortunately, it appears that this does not work for Xenon controllers
> > > despite being a part of the standard SDHCI registers and the Armada 3720
> > > datasheet itself telling that BIT(20) is useful for detecting the DAT[0]
> > > busy signal.
> > >
> > > I have tried increasing the timeout value, but I have newer managed to
> > > catch DAT_LEVEL bits change from 0 at all.
> > >
> > > This issue appears to hit most if not all SoC-s supported by Xenon driver,
> > > at least A3720, A8040 and CN9130 have non working eMMC currently.
> > >
> > > So, until a better solution is found drop the wait_dat0 OP for Xenon.
> > > I was able to only test it on A3720, but it should work for others as well.
> > >
> > > Fixes: 40e6f52454fc ("drivers: mmc: Add wait_dat0 support for sdhci driver")
> > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> >
> > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>
> Since this is a regression fix, will this be in the PR with the imx fix
> as well?  Thanks!

Hi Tom,
Was this question directed at me or?

Regards,
Robert
>
> --
> Tom



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

On Tue, Mar 15, 2022 at 10:47:57AM +0100, Robert Marko wrote:
> On Mon, Mar 14, 2022 at 2:10 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Mar 14, 2022 at 06:37:02PM +0900, Jaehoon Chung wrote:
> > > On 3/12/22 03:14, Robert Marko wrote:
> > > > Generic SDHCI driver received support for checking the busy status by
> > > > polling the DAT[0] level instead of waiting for the worst MMC switch time.
> > > >
> > > > Unfortunately, it appears that this does not work for Xenon controllers
> > > > despite being a part of the standard SDHCI registers and the Armada 3720
> > > > datasheet itself telling that BIT(20) is useful for detecting the DAT[0]
> > > > busy signal.
> > > >
> > > > I have tried increasing the timeout value, but I have newer managed to
> > > > catch DAT_LEVEL bits change from 0 at all.
> > > >
> > > > This issue appears to hit most if not all SoC-s supported by Xenon driver,
> > > > at least A3720, A8040 and CN9130 have non working eMMC currently.
> > > >
> > > > So, until a better solution is found drop the wait_dat0 OP for Xenon.
> > > > I was able to only test it on A3720, but it should work for others as well.
> > > >
> > > > Fixes: 40e6f52454fc ("drivers: mmc: Add wait_dat0 support for sdhci driver")
> > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > >
> > > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> >
> > Since this is a regression fix, will this be in the PR with the imx fix
> > as well?  Thanks!
> 
> Hi Tom,
> Was this question directed at me or?

Sorry, to Jaehoon or Peng.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] mmc: xenon_sdhci: remove wait_dat0 SDHCI OP

[Jaehoon Chung]

On 3/15/22 21:22, Tom Rini wrote:
> On Tue, Mar 15, 2022 at 10:47:57AM +0100, Robert Marko wrote:
>> On Mon, Mar 14, 2022 at 2:10 PM Tom Rini <trini@konsulko.com> wrote:
>>>
>>> On Mon, Mar 14, 2022 at 06:37:02PM +0900, Jaehoon Chung wrote:
>>>> On 3/12/22 03:14, Robert Marko wrote:
>>>>> Generic SDHCI driver received support for checking the busy status by
>>>>> polling the DAT[0] level instead of waiting for the worst MMC switch time.
>>>>>
>>>>> Unfortunately, it appears that this does not work for Xenon controllers
>>>>> despite being a part of the standard SDHCI registers and the Armada 3720
>>>>> datasheet itself telling that BIT(20) is useful for detecting the DAT[0]
>>>>> busy signal.
>>>>>
>>>>> I have tried increasing the timeout value, but I have newer managed to
>>>>> catch DAT_LEVEL bits change from 0 at all.
>>>>>
>>>>> This issue appears to hit most if not all SoC-s supported by Xenon driver,
>>>>> at least A3720, A8040 and CN9130 have non working eMMC currently.
>>>>>
>>>>> So, until a better solution is found drop the wait_dat0 OP for Xenon.
>>>>> I was able to only test it on A3720, but it should work for others as well.
>>>>>
>>>>> Fixes: 40e6f52454fc ("drivers: mmc: Add wait_dat0 support for sdhci driver")
>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
>>>>
>>>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>
>>> Since this is a regression fix, will this be in the PR with the imx fix
>>> as well?  Thanks!
>>
>> Hi Tom,
>> Was this question directed at me or?
> 
> Sorry, to Jaehoon or Peng.

Applied u-boot-mmc, Thanks! Sorry for late.

Best Regards,
Jaehoon Chung

>