| Message ID | 20220825193836.54262-6-linux@fw-web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RK3568 PCIe V3 support | expand |
On 25/08/2022 22:38, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> > > Add Nodes to Bananapi-R2-Pro board to support PCIe v3 and > set PCIe related regulators to always on. > > Suggested-by: Peter Geis <pgwipeout@gmail.com> > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > --- > v5: > - rebase on 6.0-rc1 > - add pinctrl for pcie > - fix ngff pwr_en_h gpio for hw ref 1.1 > > v4: > - change u8 lane-map to u32 data-lanes > > v3: > - squash lane-map over bifurcation property > - add comment which slot is M2 and which one if mPCIe > - fixes from Peter: > - drop regulator-always-on/regulator-boot-on from regulators > - increase startup-delay-us for regulators > - set phy-mode on PCIe3-phy > - add num-lanes to PCIe overrides > - add usb node for to PCIe/m2 > - move lane-map from PCIe controller to PCIe-phy > > v2: > - underscores in nodenames > - rockchip,bifurcation to vendor unspecific bifurcation > - fix trailing space > --- > .../boot/dts/rockchip/rk3568-bpi-r2-pro.dts | 117 ++++++++++++++++++ > 1 file changed, 117 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > index 93d383b8be87..40b90c052634 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > @@ -86,6 +86,66 @@ vcc5v0_sys: vcc5v0-sys { > vin-supply = <&dc_12v>; > }; > > + pcie30_avdd0v9: pcie30-avdd0v9 { Use consistent naming, so if other nodes have "regulator" suffix, use it here as well. > + compatible = "regulator-fixed"; > + regulator-name = "pcie30_avdd0v9"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <900000>; > + regulator-max-microvolt = <900000>; > + vin-supply = <&vcc3v3_sys>; > + }; > + > + pcie30_avdd1v8: pcie30-avdd1v8 { Ditto. > + compatible = "regulator-fixed"; > + regulator-name = "pcie30_avdd1v8"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + vin-supply = <&vcc3v3_sys>; > + }; > + > + /* pi6c pcie clock generator feeds both ports */ > + vcc3v3_pi6c_05: vcc3v3-pi6c-05-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "vcc3v3_pcie"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + enable-active-high; > + gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>; > + startup-delay-us = <200000>; > + vin-supply = <&vcc5v0_sys>; > + }; > + Best regards, Krzysztof
Hi > Gesendet: Freitag, 26. August 2022 um 08:50 Uhr > Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > On 25/08/2022 22:38, Frank Wunderlich wrote: > > From: Frank Wunderlich <frank-w@public-files.de> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > > index 93d383b8be87..40b90c052634 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > > @@ -86,6 +86,66 @@ vcc5v0_sys: vcc5v0-sys { > > vin-supply = <&dc_12v>; > > }; > > > > + pcie30_avdd0v9: pcie30-avdd0v9 { > > Use consistent naming, so if other nodes have "regulator" suffix, use it > here as well. only these 3 new have the suffix: vcc3v3_pi6c_05: vcc3v3-pi6c-05-regulator vcc3v3_minipcie: vcc3v3-minipcie-regulator vcc3v3_ngff: vcc3v3-ngff-regulator so i would drop it there... so i end up with (including existing ones to compare): vcc3v3_sys: vcc3v3-sys vcc5v0_sys: vcc5v0-sys pcie30_avdd0v9: pcie30-avdd0v9 pcie30_avdd1v8: pcie30-avdd1v8 vcc3v3_pi6c_05: vcc3v3-pi6c-05 vcc3v3_minipcie: vcc3v3-minipcie vcc3v3_ngff: vcc3v3-ngff vcc5v0_usb: vcc5v0_usb vcc5v0_usb_host: vcc5v0-usb-host vcc5v0_usb_otg: vcc5v0-usb-otg is this ok? maybe swap avdd* and pcie30 part to have voltage in front of function. > > + compatible = "regulator-fixed"; > > + regulator-name = "pcie30_avdd0v9"; > > + regulator-always-on; > > + regulator-boot-on; > > + regulator-min-microvolt = <900000>; > > + regulator-max-microvolt = <900000>; > > + vin-supply = <&vcc3v3_sys>; > > + }; > > + > > + pcie30_avdd1v8: pcie30-avdd1v8 { > > Ditto. > > > > + compatible = "regulator-fixed"; > > + regulator-name = "pcie30_avdd1v8"; > > + regulator-always-on; > > + regulator-boot-on; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > + vin-supply = <&vcc3v3_sys>; > > + }; > > + > > + /* pi6c pcie clock generator feeds both ports */ > > + vcc3v3_pi6c_05: vcc3v3-pi6c-05-regulator { > > + compatible = "regulator-fixed"; > > + regulator-name = "vcc3v3_pcie"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + enable-active-high; > > + gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>; > > + startup-delay-us = <200000>; > > + vin-supply = <&vcc5v0_sys>; > > + }; > > + > > Best regards, > Krzysztof >
On 27/08/2022 11:50, Frank Wunderlich wrote: > Hi > >> Gesendet: Freitag, 26. August 2022 um 08:50 Uhr >> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> >> On 25/08/2022 22:38, Frank Wunderlich wrote: >>> From: Frank Wunderlich <frank-w@public-files.de> > >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts >>> index 93d383b8be87..40b90c052634 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts >>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts >>> @@ -86,6 +86,66 @@ vcc5v0_sys: vcc5v0-sys { >>> vin-supply = <&dc_12v>; >>> }; >>> >>> + pcie30_avdd0v9: pcie30-avdd0v9 { >> >> Use consistent naming, so if other nodes have "regulator" suffix, use it >> here as well. > > only these 3 new have the suffix: > > vcc3v3_pi6c_05: vcc3v3-pi6c-05-regulator > vcc3v3_minipcie: vcc3v3-minipcie-regulator > vcc3v3_ngff: vcc3v3-ngff-regulator > > so i would drop it there... > > so i end up with (including existing ones to compare): > > vcc3v3_sys: vcc3v3-sys > vcc5v0_sys: vcc5v0-sys > pcie30_avdd0v9: pcie30-avdd0v9 > pcie30_avdd1v8: pcie30-avdd1v8 > vcc3v3_pi6c_05: vcc3v3-pi6c-05 > vcc3v3_minipcie: vcc3v3-minipcie > vcc3v3_ngff: vcc3v3-ngff > vcc5v0_usb: vcc5v0_usb > vcc5v0_usb_host: vcc5v0-usb-host > vcc5v0_usb_otg: vcc5v0-usb-otg > > is this ok? > > maybe swap avdd* and pcie30 part to have voltage in front of function. > I prefer all of them have regulator suffix. I think reasonable is also to rename the old ones and then add new ones with suffix. Best regards, Krzysztof
> Gesendet: Samstag, 27. August 2022 um 10:56 Uhr > Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > On 27/08/2022 11:50, Frank Wunderlich wrote: > > Hi > > > >> Gesendet: Freitag, 26. August 2022 um 08:50 Uhr > >> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > >> On 25/08/2022 22:38, Frank Wunderlich wrote: > >>> From: Frank Wunderlich <frank-w@public-files.de> > > > >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > >>> index 93d383b8be87..40b90c052634 100644 > >>> --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > >>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > >>> @@ -86,6 +86,66 @@ vcc5v0_sys: vcc5v0-sys { > >>> vin-supply = <&dc_12v>; > >>> }; > >>> > >>> + pcie30_avdd0v9: pcie30-avdd0v9 { > >> > >> Use consistent naming, so if other nodes have "regulator" suffix, use it > >> here as well. > > > > only these 3 new have the suffix: > > > > vcc3v3_pi6c_05: vcc3v3-pi6c-05-regulator > > vcc3v3_minipcie: vcc3v3-minipcie-regulator > > vcc3v3_ngff: vcc3v3-ngff-regulator > > > > so i would drop it there... > > > > so i end up with (including existing ones to compare): > > > > vcc3v3_sys: vcc3v3-sys > > vcc5v0_sys: vcc5v0-sys > > pcie30_avdd0v9: pcie30-avdd0v9 > > pcie30_avdd1v8: pcie30-avdd1v8 > > vcc3v3_pi6c_05: vcc3v3-pi6c-05 > > vcc3v3_minipcie: vcc3v3-minipcie > > vcc3v3_ngff: vcc3v3-ngff > > vcc5v0_usb: vcc5v0_usb > > vcc5v0_usb_host: vcc5v0-usb-host > > vcc5v0_usb_otg: vcc5v0-usb-otg > > > > is this ok? > > > > maybe swap avdd* and pcie30 part to have voltage in front of function. > > > > I prefer all of them have regulator suffix. I think reasonable is also > to rename the old ones and then add new ones with suffix. ok, will change these to add -regulator in name (not label). and then rename the others in separate Patch outside of the series. so basicly here - pcie30_avdd0v9: pcie30-avdd0v9 { + pcie30_avdd0v9: pcie30-avdd0v9-regulator { - pcie30_avdd1v8: pcie30-avdd1v8 { + pcie30_avdd1v8: pcie30-avdd1v8-regulator { how about the swapping of pcie30 and the avddXvY? In Schematic they are named PCIE30_AVDD_0V9 / PCIE30_AVDD_1V8, so better leave this? avdd0v9-pcie30 will be more similar to the other regulators, but inconsistent with Schematic. regards Frank
On 27/08/2022 12:14, Frank Wunderlich wrote: >> Gesendet: Samstag, 27. August 2022 um 10:56 Uhr >> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > >> On 27/08/2022 11:50, Frank Wunderlich wrote: >>> Hi >>> >>>> Gesendet: Freitag, 26. August 2022 um 08:50 Uhr >>>> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> >>>> On 25/08/2022 22:38, Frank Wunderlich wrote: >>>>> From: Frank Wunderlich <frank-w@public-files.de> >>> >>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts >>>>> index 93d383b8be87..40b90c052634 100644 >>>>> --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts >>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts >>>>> @@ -86,6 +86,66 @@ vcc5v0_sys: vcc5v0-sys { >>>>> vin-supply = <&dc_12v>; >>>>> }; >>>>> >>>>> + pcie30_avdd0v9: pcie30-avdd0v9 { >>>> >>>> Use consistent naming, so if other nodes have "regulator" suffix, use it >>>> here as well. >>> >>> only these 3 new have the suffix: >>> >>> vcc3v3_pi6c_05: vcc3v3-pi6c-05-regulator >>> vcc3v3_minipcie: vcc3v3-minipcie-regulator >>> vcc3v3_ngff: vcc3v3-ngff-regulator >>> >>> so i would drop it there... >>> >>> so i end up with (including existing ones to compare): >>> >>> vcc3v3_sys: vcc3v3-sys >>> vcc5v0_sys: vcc5v0-sys >>> pcie30_avdd0v9: pcie30-avdd0v9 >>> pcie30_avdd1v8: pcie30-avdd1v8 >>> vcc3v3_pi6c_05: vcc3v3-pi6c-05 >>> vcc3v3_minipcie: vcc3v3-minipcie >>> vcc3v3_ngff: vcc3v3-ngff >>> vcc5v0_usb: vcc5v0_usb >>> vcc5v0_usb_host: vcc5v0-usb-host >>> vcc5v0_usb_otg: vcc5v0-usb-otg >>> >>> is this ok? >>> >>> maybe swap avdd* and pcie30 part to have voltage in front of function. >>> >> >> I prefer all of them have regulator suffix. I think reasonable is also >> to rename the old ones and then add new ones with suffix. > > ok, will change these to add -regulator in name (not label). and then rename the others in separate Patch outside of the series. > > so basicly here > - pcie30_avdd0v9: pcie30-avdd0v9 { > + pcie30_avdd0v9: pcie30-avdd0v9-regulator { > - pcie30_avdd1v8: pcie30-avdd1v8 { > + pcie30_avdd1v8: pcie30-avdd1v8-regulator { > > how about the swapping of pcie30 and the avddXvY? In Schematic they are named PCIE30_AVDD_0V9 / PCIE30_AVDD_1V8, so better leave this? > > avdd0v9-pcie30 will be more similar to the other regulators, but inconsistent with Schematic. Does not matter to me - it is still a specific prefix, so whatever you put there it's for you, not for me. Keeping something aligned to schematic - even if not consistently named - makes sense to me. Best regards, Krzysztof
Hi, Am Samstag, 27. August 2022, 11:14:28 CEST schrieb Frank Wunderlich: > > Gesendet: Samstag, 27. August 2022 um 10:56 Uhr > > Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > > > On 27/08/2022 11:50, Frank Wunderlich wrote: > > > Hi > > > > > >> Gesendet: Freitag, 26. August 2022 um 08:50 Uhr > > >> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> > > >> On 25/08/2022 22:38, Frank Wunderlich wrote: > > >>> From: Frank Wunderlich <frank-w@public-files.de> > > > > > >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > > >>> index 93d383b8be87..40b90c052634 100644 > > >>> --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > > >>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > > >>> @@ -86,6 +86,66 @@ vcc5v0_sys: vcc5v0-sys { > > >>> vin-supply = <&dc_12v>; > > >>> }; > > >>> > > >>> + pcie30_avdd0v9: pcie30-avdd0v9 { > > >> > > >> Use consistent naming, so if other nodes have "regulator" suffix, use it > > >> here as well. > > > > > > only these 3 new have the suffix: > > > > > > vcc3v3_pi6c_05: vcc3v3-pi6c-05-regulator > > > vcc3v3_minipcie: vcc3v3-minipcie-regulator > > > vcc3v3_ngff: vcc3v3-ngff-regulator > > > > > > so i would drop it there... > > > > > > so i end up with (including existing ones to compare): > > > > > > vcc3v3_sys: vcc3v3-sys > > > vcc5v0_sys: vcc5v0-sys > > > pcie30_avdd0v9: pcie30-avdd0v9 > > > pcie30_avdd1v8: pcie30-avdd1v8 > > > vcc3v3_pi6c_05: vcc3v3-pi6c-05 > > > vcc3v3_minipcie: vcc3v3-minipcie > > > vcc3v3_ngff: vcc3v3-ngff > > > vcc5v0_usb: vcc5v0_usb > > > vcc5v0_usb_host: vcc5v0-usb-host > > > vcc5v0_usb_otg: vcc5v0-usb-otg > > > > > > is this ok? > > > > > > maybe swap avdd* and pcie30 part to have voltage in front of function. > > > > > > > I prefer all of them have regulator suffix. I think reasonable is also > > to rename the old ones and then add new ones with suffix. > > ok, will change these to add -regulator in name (not label). and then rename the others in separate Patch outside of the series. > > so basicly here > - pcie30_avdd0v9: pcie30-avdd0v9 { > + pcie30_avdd0v9: pcie30-avdd0v9-regulator { > - pcie30_avdd1v8: pcie30-avdd1v8 { > + pcie30_avdd1v8: pcie30-avdd1v8-regulator { > > how about the swapping of pcie30 and the avddXvY? In Schematic they are named PCIE30_AVDD_0V9 / PCIE30_AVDD_1V8, so better leave this? > > avdd0v9-pcie30 will be more similar to the other regulators, but inconsistent with Schematic. now that the phy-driver changes got applied I'll just pick up the remaining patches and do the node-name conversion while applying, so no need to send another revision for it. But of course feel free so send patches for converting the other regulator names. And I'm definitly preferring keeping close to schematic names :-) Heiko
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts index 93d383b8be87..40b90c052634 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts @@ -86,6 +86,66 @@ vcc5v0_sys: vcc5v0-sys { vin-supply = <&dc_12v>; }; + pcie30_avdd0v9: pcie30-avdd0v9 { + compatible = "regulator-fixed"; + regulator-name = "pcie30_avdd0v9"; + regulator-always-on; + regulator-boot-on; + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <900000>; + vin-supply = <&vcc3v3_sys>; + }; + + pcie30_avdd1v8: pcie30-avdd1v8 { + compatible = "regulator-fixed"; + regulator-name = "pcie30_avdd1v8"; + regulator-always-on; + regulator-boot-on; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + vin-supply = <&vcc3v3_sys>; + }; + + /* pi6c pcie clock generator feeds both ports */ + vcc3v3_pi6c_05: vcc3v3-pi6c-05-regulator { + compatible = "regulator-fixed"; + regulator-name = "vcc3v3_pcie"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + enable-active-high; + gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>; + startup-delay-us = <200000>; + vin-supply = <&vcc5v0_sys>; + }; + + /* actually fed by vcc3v3_sys, dependent on pi6c clock generator */ + vcc3v3_minipcie: vcc3v3-minipcie-regulator { + compatible = "regulator-fixed"; + regulator-name = "vcc3v3_minipcie"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + enable-active-high; + gpio = <&gpio0 RK_PC6 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&minipcie_enable_h>; + startup-delay-us = <50000>; + vin-supply = <&vcc3v3_pi6c_05>; + }; + + /* actually fed by vcc3v3_sys, dependent on pi6c clock generator */ + vcc3v3_ngff: vcc3v3-ngff-regulator { + compatible = "regulator-fixed"; + regulator-name = "vcc3v3_ngff"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + enable-active-high; + gpio = <&gpio0 RK_PB7 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&ngffpcie_enable_h>; + startup-delay-us = <50000>; + vin-supply = <&vcc3v3_pi6c_05>; + }; + vcc5v0_usb: vcc5v0_usb { compatible = "regulator-fixed"; regulator-name = "vcc5v0_usb"; @@ -513,6 +573,32 @@ rgmii_phy1: ethernet-phy@0 { }; }; +&pcie30phy { + data-lanes = <1 2>; + phy-supply = <&vcc3v3_pi6c_05>; + status = "okay"; +}; + +&pcie3x1 { + /* M.2 slot */ + num-lanes = <1>; + pinctrl-names = "default"; + pinctrl-0 = <&ngffpcie_reset_h>; + reset-gpios = <&gpio3 RK_PA1 GPIO_ACTIVE_HIGH>; + vpcie3v3-supply = <&vcc3v3_ngff>; + status = "okay"; +}; + +&pcie3x2 { + /* mPCIe slot */ + num-lanes = <1>; + pinctrl-names = "default"; + pinctrl-0 = <&minipcie_reset_h>; + reset-gpios = <&gpio2 RK_PD6 GPIO_ACTIVE_HIGH>; + vpcie3v3-supply = <&vcc3v3_minipcie>; + status = "okay"; +}; + &pinctrl { leds { blue_led_pin: blue-led-pin { @@ -529,6 +615,24 @@ hym8563_int: hym8563-int { }; }; + pcie { + minipcie_enable_h: minipcie-enable-h { + rockchip,pins = <0 RK_PC6 RK_FUNC_GPIO &pcfg_pull_none_drv_level_5>; + }; + + ngffpcie_enable_h: ngffpcie-enable-h { + rockchip,pins = <0 RK_PB7 RK_FUNC_GPIO &pcfg_pull_none_drv_level_5>; + }; + + minipcie_reset_h: minipcie-reset-h { + rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none_drv_level_5>; + }; + + ngffpcie_reset_h: ngffpcie-reset-h { + rockchip,pins = <3 RK_PA1 RK_FUNC_GPIO &pcfg_pull_none_drv_level_5>; + }; + }; + pmic { pmic_int: pmic_int { rockchip,pins = @@ -708,6 +812,19 @@ &usb2phy0_otg { status = "okay"; }; +&usb2phy1 { + /* USB for PCIe/M2 */ + status = "okay"; +}; + +&usb2phy1_host { + status = "okay"; +}; + +&usb2phy1_otg { + status = "okay"; +}; + &vop { assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>; assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;