* Re: [PATCH v2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
2024-09-13 18:54 [PATCH v2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch Judith Mendez
@ 2024-09-16 6:14 ` Adrian Hunter
2024-10-02 23:36 ` Ulf Hansson
2025-01-19 20:15 ` Josua Mayer
2 siblings, 0 replies; 5+ messages in thread
From: Adrian Hunter @ 2024-09-16 6:14 UTC (permalink / raw)
To: Judith Mendez, Ulf Hansson; +Cc: linux-mmc, linux-kernel
On 13/09/24 21:54, Judith Mendez wrote:
> The sdhci_start_signal_voltage_switch function sets
> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
> edge or pos edge of clock.
>
> Due to some eMMC and SD failures seen across am62x platform,
> do not set V1P8_SIGNAL_ENA by default, only enable the bit
> for devices that require this bit in order to switch to 1v8
> voltage for uhs modes.
>
> Signed-off-by: Judith Mendez <jm@ti.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> Changes since v1:
> - Invert quirk logic
> - Simplify sdhci_am654_start_signal_voltage_switch() and call
> sdhci_start_signal_voltage_switch() when the quirk does not apply
> - Simply logic when detecting when quirk should be applied
> ---
> drivers/mmc/host/sdhci_am654.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 0aa3c40ea6ed8..9ff07aadb2d91 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -155,6 +155,7 @@ 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 {
> @@ -356,6 +357,29 @@ 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);
> @@ -844,6 +868,11 @@ 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;
> @@ -940,6 +969,7 @@ 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: cf6444ba528f043398b112ac36e041a4d8685cb1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
2024-09-13 18:54 [PATCH v2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch Judith Mendez
2024-09-16 6:14 ` Adrian Hunter
@ 2024-10-02 23:36 ` Ulf Hansson
2025-01-19 20:15 ` Josua Mayer
2 siblings, 0 replies; 5+ messages in thread
From: Ulf Hansson @ 2024-10-02 23:36 UTC (permalink / raw)
To: Judith Mendez; +Cc: Adrian Hunter, linux-mmc, linux-kernel
On Fri, 13 Sept 2024 at 20:54, Judith Mendez <jm@ti.com> wrote:
>
> The sdhci_start_signal_voltage_switch function sets
> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
> edge or pos edge of clock.
>
> Due to some eMMC and SD failures seen across am62x platform,
> do not set V1P8_SIGNAL_ENA by default, only enable the bit
> for devices that require this bit in order to switch to 1v8
> voltage for uhs modes.
>
> Signed-off-by: Judith Mendez <jm@ti.com>
Applied for next, thanks!
Kind regards
Uffe
> ---
> Changes since v1:
> - Invert quirk logic
> - Simplify sdhci_am654_start_signal_voltage_switch() and call
> sdhci_start_signal_voltage_switch() when the quirk does not apply
> - Simply logic when detecting when quirk should be applied
> ---
> drivers/mmc/host/sdhci_am654.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 0aa3c40ea6ed8..9ff07aadb2d91 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -155,6 +155,7 @@ 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 {
> @@ -356,6 +357,29 @@ 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);
> @@ -844,6 +868,11 @@ 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;
> @@ -940,6 +969,7 @@ 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: cf6444ba528f043398b112ac36e041a4d8685cb1
> --
> 2.46.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
2024-09-13 18:54 [PATCH v2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch Judith Mendez
2024-09-16 6:14 ` Adrian Hunter
2024-10-02 23:36 ` Ulf Hansson
@ 2025-01-19 20:15 ` Josua Mayer
2025-01-27 13:24 ` Josua Mayer
2 siblings, 1 reply; 5+ messages in thread
From: Josua Mayer @ 2025-01-19 20:15 UTC (permalink / raw)
To: Judith Mendez, Ulf Hansson
Cc: Adrian Hunter, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Judith,
Am 13.09.24 um 20:54 schrieb Judith Mendez:
> The sdhci_start_signal_voltage_switch function sets
> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
> edge or pos edge of clock.
>
> Due to some eMMC and SD failures seen across am62x platform,
> do not set V1P8_SIGNAL_ENA by default, only enable the bit
> for devices that require this bit in order to switch to 1v8
> voltage for uhs modes.
I have just tested this patch on downstream TI Linux fork tagged 10.01.10
(v6.6.58) and it breaks sd-card detection on SolidRun AM642 HummingBoard-T
supported in-tree: arch/arm64/boot/dts/ti/k3-am642-hummingboard-t.dts since v6.11.
The board fails during boot from sdcard:
mmc1: error -110 whilst initialising SD card
Hence I suspect something is not okay with the heuristics enabling this quirk.
Kindly note I have not tested it with pure 6.13-rc1 yet.
My downstream workaround in dts is to link a fixed regulator vmmc-supply,
but the upstream dts specifies neither vmmc- nor vqmmc supplies.
>
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
> Changes since v1:
> - Invert quirk logic
> - Simplify sdhci_am654_start_signal_voltage_switch() and call
> sdhci_start_signal_voltage_switch() when the quirk does not apply
> - Simply logic when detecting when quirk should be applied
> ---
> drivers/mmc/host/sdhci_am654.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 0aa3c40ea6ed8..9ff07aadb2d91 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
cut
> @@ -844,6 +868,11 @@ 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;
> +
Was it intentional to apply the quirk both when neither,
and when both supplies are specified?
> sdhci_get_of_property(pdev);
>
> return 0;
> @@ -940,6 +969,7 @@ 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: cf6444ba528f043398b112ac36e041a4d8685cb1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
2025-01-19 20:15 ` Josua Mayer
@ 2025-01-27 13:24 ` Josua Mayer
0 siblings, 0 replies; 5+ messages in thread
From: Josua Mayer @ 2025-01-27 13:24 UTC (permalink / raw)
To: Judith Mendez, Ulf Hansson
Cc: Adrian Hunter, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
Am 19.01.25 um 21:15 schrieb Josua Mayer:
> Hi Judith,
>
> Am 13.09.24 um 20:54 schrieb Judith Mendez:
>> The sdhci_start_signal_voltage_switch function sets
>> V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
>> V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
>> edge or pos edge of clock.
>>
>> Due to some eMMC and SD failures seen across am62x platform,
>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>> for devices that require this bit in order to switch to 1v8
>> voltage for uhs modes.
> I have just tested this patch on downstream TI Linux fork tagged 10.01.10
> (v6.6.58) and it breaks sd-card detection on SolidRun AM642 HummingBoard-T
> supported in-tree: arch/arm64/boot/dts/ti/k3-am642-hummingboard-t.dts since v6.11.
>
> The board fails during boot from sdcard:
> mmc1: error -110 whilst initialising SD card
>
> Hence I suspect something is not okay with the heuristics enabling this quirk.
> Kindly note I have not tested it with pure 6.13-rc1 yet.
>
> My downstream workaround in dts is to link a fixed regulator vmmc-supply,
> but the upstream dts specifies neither vmmc- nor vqmmc supplies.
I just tested v6.13.0 release and it has a regression failing microSD:
[ 1.960216] mmc1: CQHCI version 5.10
[ 2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
[ 2.115348] mmc1: error -110 whilst initialising SD card
After reverting this patch original functionality is restored:
[ 1.952241] mmc1: CQHCI version 5.10
[ 2.003283] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
[ 2.100210] mmc1: new UHS-I speed SDR104 SDHC card at address aaaa
[ 2.107387] mmcblk1: mmc1:aaaa SC16G 14.8 GiB
[ 2.126668] mmcblk1: p1 p2
>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>> Changes since v1:
>> - Invert quirk logic
>> - Simplify sdhci_am654_start_signal_voltage_switch() and call
>> sdhci_start_signal_voltage_switch() when the quirk does not apply
>> - Simply logic when detecting when quirk should be applied
>> ---
>> drivers/mmc/host/sdhci_am654.c | 30 ++++++++++++++++++++++++++++++
>> 1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 0aa3c40ea6ed8..9ff07aadb2d91 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
> cut
>> @@ -844,6 +868,11 @@ 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;
>> +
> Was it intentional to apply the quirk both when neither,
> and when both supplies are specified?
>> sdhci_get_of_property(pdev);
>>
>> return 0;
>> @@ -940,6 +969,7 @@ 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: cf6444ba528f043398b112ac36e041a4d8685cb1
^ permalink raw reply [flat|nested] 5+ messages in thread