| Message ID | 20240201034621.1970279-1-kever.yang@rock-chips.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: rk3588: remove redundant cd-gpios in sdmmc node | expand |
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; >
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; >> >
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,
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;
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. Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- 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(-)