| Message ID | 20250414-perst-v2-2-89247746d755@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: qcom: Move PERST# GPIO & phy retrieval from controller to PCIe bridge node | expand |
On Mon, Apr 14, 2025 at 02:49:00PM +0200, Caleb Connolly wrote: > > > On 4/14/25 07:39, Krishna Chaitanya Chundru wrote: > > Move phy, perst, to root port from the controller node. > > > > Rename perst-gpios to reset-gpios to align with the expected naming > > convention of pci-bus-common.yaml. > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > > --- > > arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 5 ++++- > > arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++- > > arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 5 ++++- > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 ++---- > > 4 files changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts > > index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644 > > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts > > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts > > @@ -709,8 +709,11 @@ &mdss_edp_phy { > > status = "okay"; > > }; > > +&pcie1_port0 { > > + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; > > +}; > > + > > &pcie1 { > > - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; > > pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>; > > pinctrl-names = "default"; > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi > > index 2ba4ea60cb14736c9cfbf9f4a9048f20a4c921f2..ff11d85d015bdab6a90bd8a0eb9113a339866953 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi > > @@ -472,10 +472,13 @@ &pcie1 { > > pinctrl-names = "default"; > > pinctrl-0 = <&pcie1_clkreq_n>, <&ssd_rst_l>, <&pe_wake_odl>; > > - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; > > vddpe-3v3-supply = <&pp3300_ssd>; > > }; > > +&pcie1_port0 { > > + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; > > +}; > > + > > &pm8350c_pwm { > > status = "okay"; > > }; > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > > index 7370aa0dbf0e3f9e7a3e38c3f00686e1d3dcbc9f..3209bb15dfec36299cabae07d34f3dc82db6de77 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > > @@ -414,9 +414,12 @@ &lpass_va_macro { > > vdd-micb-supply = <&vreg_bob>; > > }; > > +&pcie1_port0 { > > + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; > > +}; > > + > > &pcie1 { > > status = "okay"; > > - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; > > vddpe-3v3-supply = <&nvme_3v3_regulator>; > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > > index 0f2caf36910b65c398c9e03800a8ce0a8a1f8fc7..376fabf3b4eac34d75bb79ef902c9d83490c45f7 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > > @@ -2271,9 +2271,6 @@ pcie1: pcie@1c08000 { > > power-domains = <&gcc GCC_PCIE_1_GDSC>; > > - phys = <&pcie1_phy>; > > - phy-names = "pciephy"; > > Isn't this a huge API breakage that will break using new DT with old > kernels? It will also break U-boot which uses upstream DT. > That's right. > This is bad enough, but at least let's break it a clean break by changing > all platforms at once. > Even though converting all platforms is the end goal, I don't think converting all of them in a single patch is going to do any good. > As I understand it, we seem to allow these breakages in DT for now (and this > patch landing will confirm that), but perhaps we could at least track them > somewhere or at acknowledge the breakage with a tag in the commit message > pointing to the relevant dt-bindings patch. > I'll leave the decision of breaking old kernels to DT maintainers, but I'd atleast prefer to use this in upcoming targets. So the binding and driver changes can go any time. - Mani
On 4/15/25 09:59, Dmitry Baryshkov wrote: > On Mon, Apr 14, 2025 at 02:49:00PM +0200, Caleb Connolly wrote: >> >> >> On 4/14/25 07:39, Krishna Chaitanya Chundru wrote: >>> Move phy, perst, to root port from the controller node. >>> >>> Rename perst-gpios to reset-gpios to align with the expected naming >>> convention of pci-bus-common.yaml. >>> >>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> >>> --- >>> arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 5 ++++- >>> arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++- >>> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 5 ++++- >>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 ++---- >>> 4 files changed, 14 insertions(+), 7 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts >>> index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644 >>> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts >>> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts >>> @@ -709,8 +709,11 @@ &mdss_edp_phy { >>> status = "okay"; >>> }; >>> +&pcie1_port0 { >>> + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; >>> +}; >>> + >>> &pcie1 { >>> - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; >>> pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>; >>> pinctrl-names = "default"; >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi >>> index 2ba4ea60cb14736c9cfbf9f4a9048f20a4c921f2..ff11d85d015bdab6a90bd8a0eb9113a339866953 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi >>> @@ -472,10 +472,13 @@ &pcie1 { >>> pinctrl-names = "default"; >>> pinctrl-0 = <&pcie1_clkreq_n>, <&ssd_rst_l>, <&pe_wake_odl>; >>> - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; >>> vddpe-3v3-supply = <&pp3300_ssd>; >>> }; >>> +&pcie1_port0 { >>> + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; >>> +}; >>> + >>> &pm8350c_pwm { >>> status = "okay"; >>> }; >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >>> index 7370aa0dbf0e3f9e7a3e38c3f00686e1d3dcbc9f..3209bb15dfec36299cabae07d34f3dc82db6de77 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >>> @@ -414,9 +414,12 @@ &lpass_va_macro { >>> vdd-micb-supply = <&vreg_bob>; >>> }; >>> +&pcie1_port0 { >>> + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; >>> +}; >>> + >>> &pcie1 { >>> status = "okay"; >>> - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; >>> vddpe-3v3-supply = <&nvme_3v3_regulator>; >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi >>> index 0f2caf36910b65c398c9e03800a8ce0a8a1f8fc7..376fabf3b4eac34d75bb79ef902c9d83490c45f7 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >>> @@ -2271,9 +2271,6 @@ pcie1: pcie@1c08000 { >>> power-domains = <&gcc GCC_PCIE_1_GDSC>; >>> - phys = <&pcie1_phy>; >>> - phy-names = "pciephy"; >> >> Isn't this a huge API breakage that will break using new DT with old >> kernels? It will also break U-boot which uses upstream DT. >> >> This is bad enough, but at least let's break it a clean break by changing >> all platforms at once. > > Up to now as the kernel was the only and the main user, we allowed > forward breakage (new DT breaks with the old kernel), only backwards > compatibility was required (new kernels work with old DT). > > Maybe, as this breaks external projects, we should allow a grace period > and list _both_ properties, dropping them after the LTS? This would be good, especially with some mechanism like below that would allow us to collect and list all the ABI breakages in DT for a given kernel release. Thanks,> >> >> As I understand it, we seem to allow these breakages in DT for now (and this >> patch landing will confirm that), but perhaps we could at least track them >> somewhere or at acknowledge the breakage with a tag in the commit message >> pointing to the relevant dt-bindings patch. >> >> Breaks-dt: https://lore.kernel.org/linux-arm-msm/20250414-perst-v2-1-89247746d755@oss.qualcomm.com >> >> Kind regards, >> >>> - >>> pinctrl-names = "default"; >>> pinctrl-0 = <&pcie1_clkreq_n>; >>> @@ -2284,7 +2281,7 @@ pcie1: pcie@1c08000 { >>> status = "disabled"; >>> - pcie@0 { >>> + pcie1_port0: pcie@0 { >>> device_type = "pci"; >>> reg = <0x0 0x0 0x0 0x0 0x0>; >>> bus-range = <0x01 0xff>; >>> @@ -2292,6 +2289,7 @@ pcie@0 { >>> #address-cells = <3>; >>> #size-cells = <2>; >>> ranges; >>> + phys = <&pcie1_phy>; >>> }; >>> }; >>> >> >> -- >> Caleb (they/them) >> >
diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts index 7a36c90ad4ec8b52f30b22b1621404857d6ef336..3dd58986ad5da0f898537a51715bb5d0fecbe100 100644 --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts @@ -709,8 +709,11 @@ &mdss_edp_phy { status = "okay"; }; +&pcie1_port0 { + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; +}; + &pcie1 { - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>; pinctrl-names = "default"; diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi index 2ba4ea60cb14736c9cfbf9f4a9048f20a4c921f2..ff11d85d015bdab6a90bd8a0eb9113a339866953 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi @@ -472,10 +472,13 @@ &pcie1 { pinctrl-names = "default"; pinctrl-0 = <&pcie1_clkreq_n>, <&ssd_rst_l>, <&pe_wake_odl>; - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; vddpe-3v3-supply = <&pp3300_ssd>; }; +&pcie1_port0 { + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; +}; + &pm8350c_pwm { status = "okay"; }; diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi index 7370aa0dbf0e3f9e7a3e38c3f00686e1d3dcbc9f..3209bb15dfec36299cabae07d34f3dc82db6de77 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi @@ -414,9 +414,12 @@ &lpass_va_macro { vdd-micb-supply = <&vreg_bob>; }; +&pcie1_port0 { + reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; +}; + &pcie1 { status = "okay"; - perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>; vddpe-3v3-supply = <&nvme_3v3_regulator>; diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi index 0f2caf36910b65c398c9e03800a8ce0a8a1f8fc7..376fabf3b4eac34d75bb79ef902c9d83490c45f7 100644 --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -2271,9 +2271,6 @@ pcie1: pcie@1c08000 { power-domains = <&gcc GCC_PCIE_1_GDSC>; - phys = <&pcie1_phy>; - phy-names = "pciephy"; - pinctrl-names = "default"; pinctrl-0 = <&pcie1_clkreq_n>; @@ -2284,7 +2281,7 @@ pcie1: pcie@1c08000 { status = "disabled"; - pcie@0 { + pcie1_port0: pcie@0 { device_type = "pci"; reg = <0x0 0x0 0x0 0x0 0x0>; bus-range = <0x01 0xff>; @@ -2292,6 +2289,7 @@ pcie@0 { #address-cells = <3>; #size-cells = <2>; ranges; + phys = <&pcie1_phy>; }; };
Move phy, perst, to root port from the controller node. Rename perst-gpios to reset-gpios to align with the expected naming convention of pci-bus-common.yaml. Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> --- arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 5 ++++- arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++- arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 5 ++++- arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 ++---- 4 files changed, 14 insertions(+), 7 deletions(-)