linux-kernel - Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD

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]
Date:   Mon, 13 Feb 2023 17:18:53 +0100
From:   Frieder Schrempf <frieder.schrempf@...tron.de>
To:     Marco Felsch <m.felsch@...gutronix.de>,
        Frieder Schrempf <frieder@...s.de>
Cc:     devicetree@...r.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>, Marek Vasut <marex@...x.de>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        NXP Linux Team <linux-imx@....com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Heiko Thiery <heiko.thiery@...il.com>,
        Fabio Estevam <festevam@...il.com>
Subject: Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading
 SD_VSEL signal

On 13.02.23 17:15, Marco Felsch wrote:
> Hi Frieder,
> 
> thanks for the patch.
> 
> On 23-02-13, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@...tron.de>
>>
>> This fixes the LDO5 regulator handling of the pca9450 driver by
>> taking the status of the SD_VSEL into account to determine which
>> configuration register is used for the voltage setting.
>>
>> Even without this change there is no functional issue, as the code
>> for switching the voltage in sdhci.c currently switches both, the
>> VSELECT/SD_VSEL signal and the regulator voltage at the same time
>> and doesn't run into an invalid corner case.
>>
>> We should still make sure, that we always use the correct register
>> when controlling the regulator. At least in U-Boot this fixes an
>> actual bug where the wrong IO voltage is used.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@...tron.de>
>> ---
>>  arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts | 6 +++---
>>  arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts       | 6 +++---
>>  arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi   | 1 +
>>  arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi      | 1 +
>>  4 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
>> index 8b16bd68576c..bdcd9cd843c7 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
>> @@ -344,7 +344,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  
>> @@ -357,7 +357,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  
>> @@ -370,7 +370,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  };
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
>> index a079322a3793..ba2a4491cf31 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
>> @@ -321,7 +321,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  
>> @@ -334,7 +334,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  
>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
> 
> The VSELECT pin should be driven by the (u)sdhc core...
> 
>>  		>;
>>  	};
>>  };
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> index 5172883717d1..90daaf54e704 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
>>  				regulator-name = "NVCC_SD (LDO5)";
>>  				regulator-min-microvolt = <1800000>;
>>  				regulator-max-microvolt = <3300000>;
>> +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> 
> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
> muxed as GPIO, which is not the case. So I think that u-boot have a bug
> within the (u)sdhc core.

No, we don't mux the signal as GPIO. We still use the VSLECT mux option,
but enable the SION bit, so we can read back the current state of the
VSELECT signal via the GPIO.