Re: [PATCH v2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch

[Josua Mayer <josua@solid-run.com>]

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