| Message ID | 20230419171731.28641-1-frattaroli.nicolas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: rockchip: fix nEXTRST on SOQuartz | expand |
Hi, Am Mittwoch, 19. April 2023, 19:17:31 CEST schrieb Nicolas Frattaroli: > diff --git a/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi b/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > index ce7165d7f1a1..f589a4fdaccb 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > @@ -65,6 +65,17 @@ led_work: led-work { > }; > }; > > + nextrst_pin: nextrst-pin-regulator { > + compatible = "regulator-fixed"; > + enable-active-high; > + gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&nextrst_h>; > + regulator-always-on; > + regulator-boot-on; > + regulator-name = "nextrst"; > + }; > + I agree with the sentiment and of course the rationale of the change, but not necessarily with the implementation ;-) . Why is this done as a regulator? If you want the nextrst line to be high, you could just use a gpio-hog for the line instead of doing a (fake?-)regulator. For example the px30-ringneck.dtsi does something similar: &gpio2 { /* * The Qseven BIOS_DISABLE signal on the PX30-µQ7 keeps the on-module * eMMC powered-down initially (in fact it keeps the reset signal * asserted). BIOS_DISABLE_OVERRIDE pin allows to re-enable eMMC after * the SPL has been booted from SD Card. */ bios-disable-override-hog { gpios = <RK_PB5 GPIO_ACTIVE_LOW>; output-high; line-name = "bios_disable_override"; gpio-hog; }; ... }; Thanks Heiko
Hello, On Friday, 21 April 2023 13:58:58 CEST Heiko Stuebner wrote: > Hi, > > Am Mittwoch, 19. April 2023, 19:17:31 CEST schrieb Nicolas Frattaroli: > > diff --git a/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > > b/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi index > > ce7165d7f1a1..f589a4fdaccb 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > > @@ -65,6 +65,17 @@ led_work: led-work { > > > > }; > > > > }; > > > > + nextrst_pin: nextrst-pin-regulator { > > + compatible = "regulator-fixed"; > > + enable-active-high; > > + gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&nextrst_h>; > > + regulator-always-on; > > + regulator-boot-on; > > + regulator-name = "nextrst"; > > + }; > > + > > I agree with the sentiment and of course the rationale of the change, > but not necessarily with the implementation ;-) . > > Why is this done as a regulator? > > If you want the nextrst line to be high, you could just use a gpio-hog > for the line instead of doing a (fake?-)regulator. Simply put: because I didn't know gpio hogs were a thing. I'll send a V2 to correct this. Thanks for pointing it out! > > For example the px30-ringneck.dtsi does something similar: > > &gpio2 { > /* > * The Qseven BIOS_DISABLE signal on the PX30-µQ7 keeps the > on-module * eMMC powered-down initially (in fact it keeps the reset signal > * asserted). BIOS_DISABLE_OVERRIDE pin allows to re-enable eMMC after * the > SPL has been booted from SD Card. > */ > bios-disable-override-hog { > gpios = <RK_PB5 GPIO_ACTIVE_LOW>; > output-high; > line-name = "bios_disable_override"; > gpio-hog; > }; > ... > }; > > > Thanks > Heiko Kind regards, Nicolas Frattaroli
Am Freitag, 21. April 2023, 14:38:48 CEST schrieb Nicolas Frattaroli: > Hello, > > On Friday, 21 April 2023 13:58:58 CEST Heiko Stuebner wrote: > > Hi, > > > > Am Mittwoch, 19. April 2023, 19:17:31 CEST schrieb Nicolas Frattaroli: > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > > > b/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi index > > > ce7165d7f1a1..f589a4fdaccb 100644 > > > --- a/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > > > +++ b/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi > > > @@ -65,6 +65,17 @@ led_work: led-work { > > > > > > }; > > > > > > }; > > > > > > + nextrst_pin: nextrst-pin-regulator { > > > + compatible = "regulator-fixed"; > > > + enable-active-high; > > > + gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&nextrst_h>; > > > + regulator-always-on; > > > + regulator-boot-on; > > > + regulator-name = "nextrst"; > > > + }; > > > + > > > > I agree with the sentiment and of course the rationale of the change, > > but not necessarily with the implementation ;-) . > > > > Why is this done as a regulator? > > > > If you want the nextrst line to be high, you could just use a gpio-hog > > for the line instead of doing a (fake?-)regulator. > > Simply put: because I didn't know gpio hogs were a thing. I'll send a V2 > to correct this. Thanks for pointing it out! great, thanks :-) Heiko
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-soquartz-cm4.dts b/arch/arm64/boot/dts/rockchip/rk3566-soquartz-cm4.dts index 263ce40770dd..cddf6cd2fecb 100644 --- a/arch/arm64/boot/dts/rockchip/rk3566-soquartz-cm4.dts +++ b/arch/arm64/boot/dts/rockchip/rk3566-soquartz-cm4.dts @@ -28,6 +28,16 @@ vcc_5v: vcc-5v-regulator { regulator-max-microvolt = <5000000>; vin-supply = <&vcc12v_dcin>; }; + + vcc_sd_pwr: vcc-sd-pwr-regulator { + compatible = "regulator-fixed"; + regulator-name = "vcc_sd_pwr"; + regulator-always-on; + regulator-boot-on; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + vin-supply = <&vcc3v3_sys>; + }; }; /* phy for pcie */ @@ -130,13 +140,7 @@ &saradc { }; &sdmmc0 { - vmmc-supply = <&sdmmc_pwr>; - status = "okay"; -}; - -&sdmmc_pwr { - regulator-min-microvolt = <3300000>; - regulator-max-microvolt = <3300000>; + vmmc-supply = <&vcc_sd_pwr>; status = "okay"; }; diff --git a/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi b/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi index ce7165d7f1a1..f589a4fdaccb 100644 --- a/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3566-soquartz.dtsi @@ -65,6 +65,17 @@ led_work: led-work { }; }; + nextrst_pin: nextrst-pin-regulator { + compatible = "regulator-fixed"; + enable-active-high; + gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&nextrst_h>; + regulator-always-on; + regulator-boot-on; + regulator-name = "nextrst"; + }; + sdio_pwrseq: sdio-pwrseq { status = "okay"; compatible = "mmc-pwrseq-simple"; @@ -104,16 +115,6 @@ vcc3v3_sys: vcc3v3-sys-regulator { regulator-max-microvolt = <3300000>; vin-supply = <&vcc5v0_sys>; }; - - sdmmc_pwr: sdmmc-pwr-regulator { - compatible = "regulator-fixed"; - enable-active-high; - gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>; - pinctrl-names = "default"; - pinctrl-0 = <&sdmmc_pwr_h>; - regulator-name = "sdmmc_pwr"; - status = "disabled"; - }; }; &cpu0 { @@ -539,8 +540,8 @@ wifi_enable_h: wifi-enable-h { }; }; - sdmmc-pwr { - sdmmc_pwr_h: sdmmc-pwr-h { + nextrst { + nextrst_h: nextrst-h { rockchip,pins = <0 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>; }; };
In pre-production prototypes (of which I only know one person having one, Peter Geis), GPIO0 pin A5 was tied to the SDMMC power enable pin on the CM4 connector. On all production models, this is not the case; instead, this pin is used for the nEXTRST signal, and the SDMMC power enable pin is always pulled high. Since everyone currently using the SOQuartz device trees will want this change, it is made to the tree without splitting the trees into two separate ones of which users will then inevitably choose the wrong one. This fixes USB and PCIe on a wide variety of CM4IO-compatible boards which either rely on the 3.3V being there or use the nEXTRST signal. Fixes: 5859b5a9c3ac ("arm64: dts: rockchip: add SoQuartz CM4IO dts") Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com> --- .../boot/dts/rockchip/rk3566-soquartz-cm4.dts | 18 +++++++------ .../boot/dts/rockchip/rk3566-soquartz.dtsi | 25 ++++++++++--------- 2 files changed, 24 insertions(+), 19 deletions(-)