linux-kernel - Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f36ec5d-bb31-4b6c-aa4e-4ec48cb1d067@ti.com>
Date: Fri, 11 Apr 2025 16:55:39 -0500
From: Judith Mendez <jm@...com>
To: Hiago De Franco <hiagofranco@...il.com>,
        Adrian Hunter
	<adrian.hunter@...el.com>
CC: Josua Mayer <josua@...id-run.com>, Ulf Hansson <ulf.hansson@...aro.org>,
        <linux-mmc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Moteen Shah
	<m-shah@...com>,
        Hiago De Franco <hiago.franco@...adex.com>
Subject: Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

Hi Hilago,


On 4/11/25 2:48 PM, Hiago De Franco wrote:
> On Fri, Apr 11, 2025 at 11:37:14AM -0500, Judith Mendez wrote:
>>
>> The reason that patches fixes SD init for you is because in original patch,
>> quirk was applied for both SD and eMMC with the exception of SD for am64x
>> SoC. This patch only applies the quirk for eMMC.
>>
>> We cannot use the original implementation because the logic applying the
>> quirk is based off of vmmc/vqmmc nodes in DT. The assumption was that am64x
>> based boards will only have vmmc node since there is an internal LDO that is
>> used to switch IO signal voltage instead of vqmmc. However, SolidRun
>> HimmingBoard-T board has a different implementation on voltage regulator
>> nodes in DT and the quirk applied to them as well and breaks their SD boot.
>> So we now only apply the quirk for eMMC. Without this quirk applied to SD,
>> am62x SD will continue having some stability issues.
>>
>> ~ Judith
> 
> I got the idea now, thanks for the explanation, I missed in the original
> patches this was only about the eMMC and *not* the SD card. Is there any
> plans to send patches to the SD card as well?
> 
> About that, I was wondering if instead of checking the bus_width to
> apply the fix for the eMMC, we could set a devicetree property to
> explicity apply the quirk. This way, we could also decide to apply it on
> the SD node without checking any bus width. As example:
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
> index 1ea8f64b1b3b..c4423c09e809 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
> @@ -1466,6 +1466,7 @@ &sdhci1 {
>   	vmmc-supply = <&reg_sdhc1_vmmc>;
>   	vqmmc-supply = <&reg_sdhc1_vqmmc>;
>   	ti,fails-without-test-cd;
> +	ti,suppress-v1p8-ena;
>   	status = "disabled";
>   };
>   
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 4e1156a2f1b8..a0485c2bb549 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -87,7 +87,6 @@
>   #define CLOCK_TOO_SLOW_HZ	50000000
>   #define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
>   #define RETRY_TUNING_MAX	10
> -#define BUS_WIDTH_8		8
>   
>   /* Command Queue Host Controller Interface Base address */
>   #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> @@ -844,7 +843,6 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>   	struct device *dev = &pdev->dev;
>   	int drv_strength;
>   	int ret;
> -	u32 bus_width;
>   
>   	if (sdhci_am654->flags & DLL_PRESENT) {
>   		ret = device_property_read_u32(dev, "ti,trm-icp",
> @@ -886,9 +884,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_SIGNAL_ENA for eMMC */
> -	device_property_read_u32(dev, "bus-width", &bus_width);
> -	if (bus_width == BUS_WIDTH_8)
> +	/*
> +	 * Suppress V1P8_SIGNAL_ENA for MMC devices if ti,suppress-v1p8-ena
> +	 * flag is present.
> +	 */
> +	if (device_property_read_bool(dev, "ti,suppress-v1p8-ena"))
>   		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>   
>   	sdhci_get_of_property(pdev);
> 
> Which makes our Kingston SD card work again:
> 
> root@...din-am62-15412807:~# dmesg | grep -i mmc1
> [    1.897055] mmc1: CQHCI version 5.10
> [    2.043673] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> [    2.260610] mmc1: new UHS-I speed SDR104 SDHC card at address 0001
> [    2.268150] mmcblk1: mmc1:0001 SD32G 29.1 GiB
> 
> Sorry if I missed something, would also be a valid solution?

Actually this was one of the previous implementations, I should have 
that original patch somewhere. My understanding was that we do not like 
adding new DT properties if we can find a way to apply the quirk in the 
driver.

If this implementation flies with the maintainers, then we can go back 
to DT property implementation.

Adrian, is this fine with you?

~ Judith