Re: [PATCH] arm64: dts: rockchip: rk3588: remove redundant cd-gpios in sdmmc node

Re: [PATCH] arm64: dts: rockchip: rk3588: remove redundant cd-gpios in sdmmc node

[Heiko Stübner]

Hi Kever,

Am Donnerstag, 1. Februar 2024, 04:46:21 CET schrieb Kever Yang:
> The sdmmc node already have a "&sdmmc_det" for pinctrl which switch the
> GPIO0A4 to sdmmc detect function, no need to define a separate "cd-gpios".

just to make sure, did you test this on actual hardware?
Because there might be differences in behaviour.

> RK3588 has force_jtage feature which is enable JTAG function via sdmmc
> pins automatically when there is no SD card insert, this feature will
> need the GPIO0A4 works in sdmmc_det function like other mmc signal instead
> of GPIO function, or else the force_jtag can not auto be disabled when
> SD card insert.

We disable the jtag switching by default [0] ;-) .
And there are very good reasons for it too:


(1) JTAG is very much a debug feature, that the normal user will not need.
Especially not in a finished product. If a developer is debugging _that_
deep and needs jtag, they can enable it in their debug build.


(2) Randomly enabling features that may compromise security.
Why go through all the hoops of doing things like secure boot, signed
images and everything, just to have the kernel then export direct access
to the hardware on sd-card pins. If one wants to expose JTAG somewhere
this should be conscious choice and devs should not need to fork their
kernel just to shut down unwanted security-critical functionality.


(3) It affects board layouts _not following_ the standard layout.
Nobody is forcing board-designers to use Rockchip's desired pin
for card-detection. Some designer may just select a different pin
or a board could go without card-detect at all - see rk3588-jaguar.
These are both valid use-cases that need to be supported.


Heiko


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6f6878ec6faf16a5f36761c93da6ea9cf09adb33


> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts | 1 -
>  arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts     | 1 -
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts         | 1 -
>  arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts        | 1 -
>  4 files changed, 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> index 3e660ff6cd5ff..1b606ea5b6cf2 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> @@ -444,7 +444,6 @@ &sdhci {
>  &sdmmc {
>  	bus-width = <4>;
>  	cap-sd-highspeed;
> -	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>  	disable-wp;
>  	max-frequency = <150000000>;
>  	no-sdio;
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts b/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
> index 87a0abf95f7d4..67414d72e2b6e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
> @@ -429,7 +429,6 @@ &sdhci {
>  &sdmmc {
>  	bus-width = <4>;
>  	cap-sd-highspeed;
> -	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>  	disable-wp;
>  	max-frequency = <150000000>;
>  	no-sdio;
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index a0e303c3a1dc6..25a82008e4f76 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -371,7 +371,6 @@ &sdmmc {
>  	bus-width = <4>;
>  	cap-mmc-highspeed;
>  	cap-sd-highspeed;
> -	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>  	disable-wp;
>  	sd-uhs-sdr104;
>  	vmmc-supply = <&vcc_3v3_s3>;
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
> index 2002fd0221fa3..00afb90d4eb10 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
> @@ -366,7 +366,6 @@ &sdmmc {
>  	bus-width = <4>;
>  	cap-mmc-highspeed;
>  	cap-sd-highspeed;
> -	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>  	disable-wp;
>  	max-frequency = <150000000>;
>  	no-sdio;
> 

Re: [PATCH] arm64: dts: rockchip: rk3588: remove redundant cd-gpios in sdmmc node

[Kever Yang]

Hi Heiko,

On 2024/2/1 16:41, Heiko Stübner wrote:
> Hi Kever,
>
> Am Donnerstag, 1. Februar 2024, 04:46:21 CET schrieb Kever Yang:
>> The sdmmc node already have a "&sdmmc_det" for pinctrl which switch the
>> GPIO0A4 to sdmmc detect function, no need to define a separate "cd-gpios".
> just to make sure, did you test this on actual hardware?
> Because there might be differences in behaviour.

We use this feature in vendor kernel for many boards.

For mainline support, there are 15 rk3588/rk3588 boards available, and 
10 of them

enable sdmmc node in dts, and 4 boards define "cd-gpios" while the 
hardware do use GPIO0A4,

and 1 board(rk3588-jaguar) using "broken-cd", and the other 5 boards are 
using default "&sdmmc_det"

  with the same hardware design.

If the hardware is using GPIO0A4(SDMMC_DET function IO) for sdmmc 
detect, then no need to define "cd-gpios";

if the hardware is not using GPIO0A4 for sdmmc detect, then the 
"cd-gpios" or "broken-cd" is needed.

So this patch is to sync up to use the "&sdmmc_det" when the IO is using 
the one has SDMMC_DET function.

>> RK3588 has force_jtage feature which is enable JTAG function via sdmmc
>> pins automatically when there is no SD card insert, this feature will
>> need the GPIO0A4 works in sdmmc_det function like other mmc signal instead
>> of GPIO function, or else the force_jtag can not auto be disabled when
>> SD card insert.
> We disable the jtag switching by default [0] ;-) .
> And there are very good reasons for it too:

I know you have disable the force_jtag by default, and I didn't want to 
change this.

As you have said we may need to enable it for debug, we suppose to only 
have to revert

the disable force_jtag patch and then it works without affect the 
default sdmmc function.

The sdmmc function is broken if we enable force_jtag for debug, and this 
patch can fix it.

> (1) JTAG is very much a debug feature, that the normal user will not need.
> Especially not in a finished product. If a developer is debugging _that_
> deep and needs jtag, they can enable it in their debug build.
>
>
> (2) Randomly enabling features that may compromise security.
> Why go through all the hoops of doing things like secure boot, signed
> images and everything, just to have the kernel then export direct access
> to the hardware on sd-card pins. If one wants to expose JTAG somewhere
> this should be conscious choice and devs should not need to fork their
> kernel just to shut down unwanted security-critical functionality.
>
>
> (3) It affects board layouts _not following_ the standard layout.
> Nobody is forcing board-designers to use Rockchip's desired pin
> for card-detection. Some designer may just select a different pin
> or a board could go without card-detect at all - see rk3588-jaguar.

You are right, "cd-gpios" and "broken-cd" are available for boards have 
different design,

and this patch is for the boards with SDMMC_DET(GPIO0A4) as sdmmc 
detect, they should go to

the default "&sdmmc_det" in sdmmc node.


Thanks,

- Kever

> These are both valid use-cases that need to be supported.
>
>
> Heiko
>
>
> [0]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6f6878ec6faf16a5f36761c93da6ea9cf09adb33
>
>
>> ---
>>
>>   arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts | 1 -
>>   arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts     | 1 -
>>   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts         | 1 -
>>   arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts        | 1 -
>>   4 files changed, 4 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
>> index 3e660ff6cd5ff..1b606ea5b6cf2 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
>> @@ -444,7 +444,6 @@ &sdhci {
>>   &sdmmc {
>>   	bus-width = <4>;
>>   	cap-sd-highspeed;
>> -	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>>   	disable-wp;
>>   	max-frequency = <150000000>;
>>   	no-sdio;
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts b/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
>> index 87a0abf95f7d4..67414d72e2b6e 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
>> @@ -429,7 +429,6 @@ &sdhci {
>>   &sdmmc {
>>   	bus-width = <4>;
>>   	cap-sd-highspeed;
>> -	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>>   	disable-wp;
>>   	max-frequency = <150000000>;
>>   	no-sdio;
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> index a0e303c3a1dc6..25a82008e4f76 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> @@ -371,7 +371,6 @@ &sdmmc {
>>   	bus-width = <4>;
>>   	cap-mmc-highspeed;
>>   	cap-sd-highspeed;
>> -	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>>   	disable-wp;
>>   	sd-uhs-sdr104;
>>   	vmmc-supply = <&vcc_3v3_s3>;
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
>> index 2002fd0221fa3..00afb90d4eb10 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
>> @@ -366,7 +366,6 @@ &sdmmc {
>>   	bus-width = <4>;
>>   	cap-mmc-highspeed;
>>   	cap-sd-highspeed;
>> -	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>>   	disable-wp;
>>   	max-frequency = <150000000>;
>>   	no-sdio;
>>
>

Re: [PATCH] arm64: dts: rockchip: rk3588: remove redundant cd-gpios in sdmmc node

[Heiko Stuebner]

On Thu, 1 Feb 2024 11:46:21 +0800, Kever Yang wrote:
> The sdmmc node already have a "&sdmmc_det" for pinctrl which switch the
> GPIO0A4 to sdmmc detect function, no need to define a separate "cd-gpios".
> 
> RK3588 has force_jtage feature which is enable JTAG function via sdmmc
> pins automatically when there is no SD card insert, this feature will
> need the GPIO0A4 works in sdmmc_det function like other mmc signal instead
> of GPIO function, or else the force_jtag can not auto be disabled when
> SD card insert.
> 
> [...]

Applied, thanks!

[1/1] arm64: dts: rockchip: rk3588: remove redundant cd-gpios in sdmmc node
      commit: d859ad305ed19d9a77d8c8ecd22459b73da36ba6

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>