[PATCH master 1/3] mci: core: fix fallback when eMMC DDR52 is not possible

[PATCH master 1/3] mci: core: fix fallback when eMMC DDR52 is not possible

[Ahmad Fatoum]

mci_mmc_select_hs_ddr() will try DDR52 and if that fails, revert to
SDR operation. In that case, it returns the bus width, which would
be forwarded as if it were an error code:

  WARNING: mmc2: Card's startup fails with 3

Fix this by translating successful return values to 0 in
mci_startup_mmc().

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/mci-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 1d383e6449e9..57b4c5b99c9c 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -1566,7 +1566,7 @@ static int mci_startup_mmc(struct mci *mci)
 		}
 	}
 
-	return ret;
+	return ret >= MMC_BUS_WIDTH_1 ? 0 : ret;
 }
 
 /**
-- 
2.39.2

[PATCH master 2/3] mci: core: fix fallback when host doesn't support HS200

[Ahmad Fatoum]

If switching the card into HS200 mode via mmc_select_timing() succeeds,
but switching the host fails in mmc_hs200_tuning(), we are left with
wrong timing information, which prevents us from falling back to a lower
speed mode. Restore the high speed timing on failure to resolve this.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/mci-core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 57b4c5b99c9c..af0980308866 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -1551,6 +1551,11 @@ static int mci_startup_mmc(struct mci *mci)
 
 		if (mmc_card_hs200(mci))
 			ret = mmc_hs200_tuning(mci);
+
+		if (ret) {
+			host->timing = MMC_TIMING_MMC_HS;
+			mci_switch(mci, EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS);
+		}
 	}
 
 	if (ret || !IS_ENABLED(CONFIG_MCI_TUNING)) {
-- 
2.39.2

[PATCH master 3/3] mci: core: make execute_tuning mandatory for HS200

[Ahmad Fatoum]

We assume that any host controller that doesn't implement execute_tuning
will have no issue with HS200 operation. This is likely untrue for most
host drivers that we have and it will confuse users:

  - Enable CONFIG_MCI_TUNING, e.g. for i.MX
  - devinfo shows HS200 operation
  - Actually reading blocks will fail with EIO

Therefore, let's make execute_tuning mandatory for HS200. If some driver
doesn't need it, it can always define an empty stub for it.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/mci-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index af0980308866..f6565b2b64dd 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -1349,8 +1349,14 @@ int mci_execute_tuning(struct mci *mci)
 	struct mci_host *host = mci->host;
 	u32 opcode;
 
-	if (!host->execute_tuning)
-		return 0;
+	if (!host->execute_tuning) {
+		/*
+		 * For us, implementing ->execute_tuning is mandatory to
+		 * support higher speed modes
+		 */
+		dev_warn(&mci->dev, "tuning failed: no host diver support\n");
+		return -EOPNOTSUPP;
+	}
 
 	/* Tuning is only supported for MMC / HS200 */
 	if (mmc_card_hs200(mci))
-- 
2.39.2

Re: [PATCH master 1/3] mci: core: fix fallback when eMMC DDR52 is not possible

[Sascha Hauer]

On Tue, Apr 23, 2024 at 08:22:04AM +0200, Ahmad Fatoum wrote:
> mci_mmc_select_hs_ddr() will try DDR52 and if that fails, revert to
> SDR operation. In that case, it returns the bus width, which would
> be forwarded as if it were an error code:
> 
>   WARNING: mmc2: Card's startup fails with 3
> 
> Fix this by translating successful return values to 0 in
> mci_startup_mmc().
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/mci/mci-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index 1d383e6449e9..57b4c5b99c9c 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -1566,7 +1566,7 @@ static int mci_startup_mmc(struct mci *mci)
>  		}
>  	}
>  
> -	return ret;
> +	return ret >= MMC_BUS_WIDTH_1 ? 0 : ret;
>  }

Can we do it like below instead? It makes clear where a MMC_BUS_WIDTH_x
return value is expected.

Sascha


----------------------------------8<---------------------------

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 083d2f4ed1..da892a5f84 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -1558,8 +1558,12 @@ static int mci_startup_mmc(struct mci *mci)
 
 		/* find out maximum bus width and then try DDR if supported */
 		ret = mci_mmc_select_bus_width(mci);
-		if (ret > MMC_BUS_WIDTH_1 && mci->tran_speed == 52000000)
-			ret = mci_mmc_select_hs_ddr(mci);
+		if (ret > MMC_BUS_WIDTH_1) {
+			ret = 0;
+
+			if (mci->tran_speed == 52000000)
+				ret = mci_mmc_select_hs_ddr(mci);
+		}
 
 		if (ret < 0) {
 			dev_warn(&mci->dev, "Changing MMC bus width failed: %d\n", ret);


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master 1/3] mci: core: fix fallback when eMMC DDR52 is not possible

[Ahmad Fatoum]

On 23.04.24 08:44, Sascha Hauer wrote:
> On Tue, Apr 23, 2024 at 08:22:04AM +0200, Ahmad Fatoum wrote:
>> mci_mmc_select_hs_ddr() will try DDR52 and if that fails, revert to
>> SDR operation. In that case, it returns the bus width, which would
>> be forwarded as if it were an error code:
>>
>>   WARNING: mmc2: Card's startup fails with 3
>>
>> Fix this by translating successful return values to 0 in
>> mci_startup_mmc().
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/mci/mci-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
>> index 1d383e6449e9..57b4c5b99c9c 100644
>> --- a/drivers/mci/mci-core.c
>> +++ b/drivers/mci/mci-core.c
>> @@ -1566,7 +1566,7 @@ static int mci_startup_mmc(struct mci *mci)
>>  		}
>>  	}
>>  
>> -	return ret;
>> +	return ret >= MMC_BUS_WIDTH_1 ? 0 : ret;
>>  }
> 
> Can we do it like below instead? It makes clear where a MMC_BUS_WIDTH_x
> return value is expected.

mci_mmc_select_hs_ddr() may return MMC_BUS_WIDTH_*, which isn't addressed
by your patch.

> 
> Sascha
> 
> 
> ----------------------------------8<---------------------------
> 
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index 083d2f4ed1..da892a5f84 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -1558,8 +1558,12 @@ static int mci_startup_mmc(struct mci *mci)
>  
>  		/* find out maximum bus width and then try DDR if supported */
>  		ret = mci_mmc_select_bus_width(mci);
> -		if (ret > MMC_BUS_WIDTH_1 && mci->tran_speed == 52000000)
> -			ret = mci_mmc_select_hs_ddr(mci);
> +		if (ret > MMC_BUS_WIDTH_1) {
> +			ret = 0;
> +
> +			if (mci->tran_speed == 52000000)
> +				ret = mci_mmc_select_hs_ddr(mci);
> +		}
>  
>  		if (ret < 0) {
>  			dev_warn(&mci->dev, "Changing MMC bus width failed: %d\n", ret);
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master 1/3] mci: core: fix fallback when eMMC DDR52 is not possible

[Sascha Hauer]

On Tue, 23 Apr 2024 08:22:04 +0200, Ahmad Fatoum wrote:
> mci_mmc_select_hs_ddr() will try DDR52 and if that fails, revert to
> SDR operation. In that case, it returns the bus width, which would
> be forwarded as if it were an error code:
> 
>   WARNING: mmc2: Card's startup fails with 3
> 
> Fix this by translating successful return values to 0 in
> mci_startup_mmc().
> 
> [...]

Applied, thanks!

[1/3] mci: core: fix fallback when eMMC DDR52 is not possible
      https://git.pengutronix.de/cgit/barebox/commit/?id=02a40f3ed0ac (link may not be stable)
[2/3] mci: core: fix fallback when host doesn't support HS200
      https://git.pengutronix.de/cgit/barebox/commit/?id=ebb5a3204630 (link may not be stable)
[3/3] mci: core: make execute_tuning mandatory for HS200
      https://git.pengutronix.de/cgit/barebox/commit/?id=8ca910b92188 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>