diff mbox series

Revert "mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch"

Message ID 20250127-am654-mmc-regression-v1-1-d831f9a13ae9@solid-run.com (mailing list archive)
State New
Headers show
Series Revert "mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch" | expand

Commit Message

Josua Mayer Jan. 27, 2025, 1:35 p.m. UTC 
This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.

This commit uses presence of device-tree properties vmmc-supply and
vqmmc-supply for deciding whether to enable a quirk affecting timing of
clock and data.
The intention was to address issues observed with eMMC and SD on AM62
platforms.

This new quirk is however also enabled for AM64 breaking microSD access
on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
causing a regression. During boot microSD initialization now fails with
the error below:

[    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
[    2.115348] mmc1: error -110 whilst initialising SD card

The heuristics for enabling the quirk are clearly not correct as they
break at least one but potentially many existing boards.

Revert the change and restore original behaviour until a more
appropriate method of selecting the quirk is derived.

Fixes: <941a7abd4666> ("mtd: spi-nor: core: replace dummy buswidth from addr to data")
Closes: https://lore.kernel.org/linux-mmc/a70fc9fc-186f-4165-a652-3de50733763a@solid-run.com/
Cc: stable@vger.kernel.org # 6.13
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 drivers/mmc/host/sdhci_am654.c | 30 ------------------------------
 1 file changed, 30 deletions(-)


---
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
change-id: 20250127-am654-mmc-regression-ed289f8967c2

Best regards,

Comments

Josua Mayer Jan. 27, 2025, 1:40 p.m. UTC | #1 
Cc: Judith Mendez <jm@ti.com>

Am 27.01.25 um 14:35 schrieb Josua Mayer:
> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
>
> This commit uses presence of device-tree properties vmmc-supply and
> vqmmc-supply for deciding whether to enable a quirk affecting timing of
> clock and data.
> The intention was to address issues observed with eMMC and SD on AM62
> platforms.
>
> This new quirk is however also enabled for AM64 breaking microSD access
> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
> causing a regression. During boot microSD initialization now fails with
> the error below:
>
> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> [    2.115348] mmc1: error -110 whilst initialising SD card
>
> The heuristics for enabling the quirk are clearly not correct as they
> break at least one but potentially many existing boards.
>
> Revert the change and restore original behaviour until a more
> appropriate method of selecting the quirk is derived.
>
> Fixes: <941a7abd4666> ("mtd: spi-nor: core: replace dummy buswidth from addr to data")
> Closes: https://lore.kernel.org/linux-mmc/a70fc9fc-186f-4165-a652-3de50733763a@solid-run.com/
> Cc: stable@vger.kernel.org # 6.13
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
>  drivers/mmc/host/sdhci_am654.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index b73f673db92bbc042392995e715815e15ace6005..f75c31815ab00d17b5757063521f56ba5643babe 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -155,7 +155,6 @@ struct sdhci_am654_data {
>  	u32 tuning_loop;
>  
>  #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
> -#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>  };
>  
>  struct window {
> @@ -357,29 +356,6 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>  	sdhci_set_clock(host, clock);
>  }
>  
> -static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
> -{
> -	struct sdhci_host *host = mmc_priv(mmc);
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> -	int ret;
> -
> -	if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
> -	    ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> -		if (!IS_ERR(mmc->supply.vqmmc)) {
> -			ret = mmc_regulator_set_vqmmc(mmc, ios);
> -			if (ret < 0) {
> -				pr_err("%s: Switching to 1.8V signalling voltage failed,\n",
> -				       mmc_hostname(mmc));
> -				return -EIO;
> -			}
> -		}
> -		return 0;
> -	}
> -
> -	return sdhci_start_signal_voltage_switch(mmc, ios);
> -}
> -
>  static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>  {
>  	writeb(val, host->ioaddr + reg);
> @@ -868,11 +844,6 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>  	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>  		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>  
> -	/* Suppress v1p8 ena for eMMC and SD with vqmmc supply */
> -	if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
> -	    !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
> -		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
> -
>  	sdhci_get_of_property(pdev);
>  
>  	return 0;
> @@ -969,7 +940,6 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>  		goto err_pltfm_free;
>  	}
>  
> -	host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>  	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>  
>  	pm_runtime_get_noresume(dev);
>
> ---
> base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
> change-id: 20250127-am654-mmc-regression-ed289f8967c2
>
> Best regards,
Adrian Hunter Jan. 27, 2025, 2:03 p.m. UTC | #2 
On 27/01/25 15:35, Josua Mayer wrote:
> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
> 
> This commit uses presence of device-tree properties vmmc-supply and
> vqmmc-supply for deciding whether to enable a quirk affecting timing of
> clock and data.
> The intention was to address issues observed with eMMC and SD on AM62
> platforms.
> 
> This new quirk is however also enabled for AM64 breaking microSD access
> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
> causing a regression. During boot microSD initialization now fails with
> the error below:
> 
> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> [    2.115348] mmc1: error -110 whilst initialising SD card
> 
> The heuristics for enabling the quirk are clearly not correct as they
> break at least one but potentially many existing boards.
> 
> Revert the change and restore original behaviour until a more
> appropriate method of selecting the quirk is derived.
> 
> Fixes: <941a7abd4666> ("mtd: spi-nor: core: replace dummy buswidth from addr to data")

Angle brackets < > should not be present, and the commit
description is incorrect.

> Closes: https://lore.kernel.org/linux-mmc/a70fc9fc-186f-4165-a652-3de50733763a@solid-run.com/
> Cc: stable@vger.kernel.org # 6.13

With a Fixes tag, better to leave out "# 6.13"

> Signed-off-by: Josua Mayer <josua@solid-run.com>

In the absence of an alternative, we have to revert, so:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci_am654.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index b73f673db92bbc042392995e715815e15ace6005..f75c31815ab00d17b5757063521f56ba5643babe 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -155,7 +155,6 @@ struct sdhci_am654_data {
>  	u32 tuning_loop;
>  
>  #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
> -#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>  };
>  
>  struct window {
> @@ -357,29 +356,6 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>  	sdhci_set_clock(host, clock);
>  }
>  
> -static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
> -{
> -	struct sdhci_host *host = mmc_priv(mmc);
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> -	int ret;
> -
> -	if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
> -	    ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> -		if (!IS_ERR(mmc->supply.vqmmc)) {
> -			ret = mmc_regulator_set_vqmmc(mmc, ios);
> -			if (ret < 0) {
> -				pr_err("%s: Switching to 1.8V signalling voltage failed,\n",
> -				       mmc_hostname(mmc));
> -				return -EIO;
> -			}
> -		}
> -		return 0;
> -	}
> -
> -	return sdhci_start_signal_voltage_switch(mmc, ios);
> -}
> -
>  static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>  {
>  	writeb(val, host->ioaddr + reg);
> @@ -868,11 +844,6 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>  	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>  		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>  
> -	/* Suppress v1p8 ena for eMMC and SD with vqmmc supply */
> -	if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
> -	    !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
> -		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
> -
>  	sdhci_get_of_property(pdev);
>  
>  	return 0;
> @@ -969,7 +940,6 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>  		goto err_pltfm_free;
>  	}
>  
> -	host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>  	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>  
>  	pm_runtime_get_noresume(dev);
> 
> ---
> base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
> change-id: 20250127-am654-mmc-regression-ed289f8967c2
> 
> Best regards,
Greg Kroah-Hartman Jan. 27, 2025, 2:12 p.m. UTC | #3 
On Mon, Jan 27, 2025 at 02:35:29PM +0100, Josua Mayer wrote:
> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
> 
> This commit uses presence of device-tree properties vmmc-supply and
> vqmmc-supply for deciding whether to enable a quirk affecting timing of
> clock and data.
> The intention was to address issues observed with eMMC and SD on AM62
> platforms.
> 
> This new quirk is however also enabled for AM64 breaking microSD access
> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
> causing a regression. During boot microSD initialization now fails with
> the error below:
> 
> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> [    2.115348] mmc1: error -110 whilst initialising SD card
> 
> The heuristics for enabling the quirk are clearly not correct as they
> break at least one but potentially many existing boards.
> 
> Revert the change and restore original behaviour until a more
> appropriate method of selecting the quirk is derived.
> 
> Fixes: <941a7abd4666> ("mtd: spi-nor: core: replace dummy buswidth from addr to data")

Please don't use "<>" in the Fixes: line, that's not how the
documentation asks to use it.

thanks,

greg k-h
Josua Mayer Jan. 27, 2025, 8:08 p.m. UTC | #4 
Am 27.01.25 um 15:03 schrieb Adrian Hunter:
> On 27/01/25 15:35, Josua Mayer wrote:
>> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
>>
>> This commit uses presence of device-tree properties vmmc-supply and
>> vqmmc-supply for deciding whether to enable a quirk affecting timing of
>> clock and data.
>> The intention was to address issues observed with eMMC and SD on AM62
>> platforms.
>>
>> This new quirk is however also enabled for AM64 breaking microSD access
>> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
>> causing a regression. During boot microSD initialization now fails with
>> the error below:
>>
>> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
>> [    2.115348] mmc1: error -110 whilst initialising SD card
>>
>> The heuristics for enabling the quirk are clearly not correct as they
>> break at least one but potentially many existing boards.
>>
>> Revert the change and restore original behaviour until a more
>> appropriate method of selecting the quirk is derived.
>>
>> Fixes: <941a7abd4666> ("mtd: spi-nor: core: replace dummy buswidth from addr to data")
> Angle brackets < > should not be present, 
Adrian, Greg, thank you for pointing this out, I will resolve it with v2.
> and the commit
> description is incorrect.

Now I see why checkpatch had complained before adding <>,
Thank you!

>
>> Closes: https://lore.kernel.org/linux-mmc/a70fc9fc-186f-4165-a652-3de50733763a@solid-run.com/
>> Cc: stable@vger.kernel.org # 6.13
> With a Fixes tag, better to leave out "# 6.13"
Okay.
>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
> In the absence of an alternative, we have to revert, so:
I do not understand the quirk sufficiently to suggest an alternative :(.
>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>
>> ---
>>  drivers/mmc/host/sdhci_am654.c | 30 ------------------------------
>>  1 file changed, 30 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index b73f673db92bbc042392995e715815e15ace6005..f75c31815ab00d17b5757063521f56ba5643babe 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -155,7 +155,6 @@ struct sdhci_am654_data {
>>  	u32 tuning_loop;
>>  
>>  #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>> -#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>  };
>>  
>>  struct window {
>> @@ -357,29 +356,6 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>  	sdhci_set_clock(host, clock);
>>  }
>>  
>> -static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>> -{
>> -	struct sdhci_host *host = mmc_priv(mmc);
>> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> -	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>> -	int ret;
>> -
>> -	if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>> -	    ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>> -		if (!IS_ERR(mmc->supply.vqmmc)) {
>> -			ret = mmc_regulator_set_vqmmc(mmc, ios);
>> -			if (ret < 0) {
>> -				pr_err("%s: Switching to 1.8V signalling voltage failed,\n",
>> -				       mmc_hostname(mmc));
>> -				return -EIO;
>> -			}
>> -		}
>> -		return 0;
>> -	}
>> -
>> -	return sdhci_start_signal_voltage_switch(mmc, ios);
>> -}
>> -
>>  static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>  {
>>  	writeb(val, host->ioaddr + reg);
>> @@ -868,11 +844,6 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>  	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>>  		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>>  
>> -	/* Suppress v1p8 ena for eMMC and SD with vqmmc supply */
>> -	if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
>> -	    !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
>> -		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>> -
>>  	sdhci_get_of_property(pdev);
>>  
>>  	return 0;
>> @@ -969,7 +940,6 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>  		goto err_pltfm_free;
>>  	}
>>  
>> -	host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>  	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>  
>>  	pm_runtime_get_noresume(dev);
>>
>> ---
>> base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
>> change-id: 20250127-am654-mmc-regression-ed289f8967c2
>>
>> Best regards,
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index b73f673db92bbc042392995e715815e15ace6005..f75c31815ab00d17b5757063521f56ba5643babe 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -155,7 +155,6 @@  struct sdhci_am654_data {
 	u32 tuning_loop;
 
 #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
-#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
 };
 
 struct window {
@@ -357,29 +356,6 @@  static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
 	sdhci_set_clock(host, clock);
 }
 
-static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
-{
-	struct sdhci_host *host = mmc_priv(mmc);
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
-	int ret;
-
-	if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
-	    ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
-		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = mmc_regulator_set_vqmmc(mmc, ios);
-			if (ret < 0) {
-				pr_err("%s: Switching to 1.8V signalling voltage failed,\n",
-				       mmc_hostname(mmc));
-				return -EIO;
-			}
-		}
-		return 0;
-	}
-
-	return sdhci_start_signal_voltage_switch(mmc, ios);
-}
-
 static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
 {
 	writeb(val, host->ioaddr + reg);
@@ -868,11 +844,6 @@  static int sdhci_am654_get_of_property(struct platform_device *pdev,
 	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
 		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
 
-	/* Suppress v1p8 ena for eMMC and SD with vqmmc supply */
-	if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
-	    !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
-		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
-
 	sdhci_get_of_property(pdev);
 
 	return 0;
@@ -969,7 +940,6 @@  static int sdhci_am654_probe(struct platform_device *pdev)
 		goto err_pltfm_free;
 	}
 
-	host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
 	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
 
 	pm_runtime_get_noresume(dev);