| Message ID | 20231227-topic-8280_pcie_dts-v1-3-13d12b1698ff@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | SC8280XP preparatory PCIe fixes | expand |
On Wed, Dec 27, 2023 at 11:28:28PM +0100, Konrad Dybcio wrote: > The CLKREQ pin should not be muxed to its active function when the RC > is asleep. You forgot to explain *why* you think this is needed. Note that this is only appears to be done for one upstream Qualcomm SoC (msm8996) currently, and that, notably, there is no driver support for actually changing the pin state. > Add the missing pin sleep states to resolve that. > Fixes: d907fe5acbf1 ("arm64: dts: qcom: sc8280xp-crd: enable WiFi controller") > Fixes: 17e2ccaf65d1 ("arm64: dts: qcom: sc8280xp-crd: enable SDX55 modem") > Fixes: 6a1ec5eca73c ("arm64: dts: qcom: sc8280xp-crd: enable NVMe SSD") So not sure these Fixes tags are warranted either. > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 78 ++++++++++++++++++++----------- > 1 file changed, 51 insertions(+), 27 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts > index ffc4406422ae..58c0c2d10cb3 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts > @@ -530,8 +530,9 @@ &pcie2a { > > vddpe-3v3-supply = <&vreg_nvme>; > > - pinctrl-names = "default"; > - pinctrl-0 = <&pcie2a_default>; > + pinctrl-0 = <&pcie2a_default>, <&pcie2a_clkreq_default>; > + pinctrl-1 = <&pcie2a_default>, <&pcie2a_clkreq_sleep>; > + pinctrl-names = "default", "sleep"; Johan
On Wed, Dec 27, 2023 at 11:28:28PM +0100, Konrad Dybcio wrote: > The CLKREQ pin should not be muxed to its active function when the RC > is asleep. Add the missing pin sleep states to resolve that. > I don't understand why it should not be muxed and wondering what is the actual issue you are seeing that lead to this conclusion. - Mani > Fixes: d907fe5acbf1 ("arm64: dts: qcom: sc8280xp-crd: enable WiFi controller") > Fixes: 17e2ccaf65d1 ("arm64: dts: qcom: sc8280xp-crd: enable SDX55 modem") > Fixes: 6a1ec5eca73c ("arm64: dts: qcom: sc8280xp-crd: enable NVMe SSD") > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 78 ++++++++++++++++++++----------- > 1 file changed, 51 insertions(+), 27 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts > index ffc4406422ae..58c0c2d10cb3 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts > @@ -530,8 +530,9 @@ &pcie2a { > > vddpe-3v3-supply = <&vreg_nvme>; > > - pinctrl-names = "default"; > - pinctrl-0 = <&pcie2a_default>; > + pinctrl-0 = <&pcie2a_default>, <&pcie2a_clkreq_default>; > + pinctrl-1 = <&pcie2a_default>, <&pcie2a_clkreq_sleep>; > + pinctrl-names = "default", "sleep"; > > status = "okay"; > }; > @@ -549,8 +550,9 @@ &pcie3a { > > vddpe-3v3-supply = <&vreg_wwan>; > > - pinctrl-names = "default"; > - pinctrl-0 = <&pcie3a_default>; > + pinctrl-0 = <&pcie3a_default>, <&pcie3a_clkreq_default>; > + pinctrl-1 = <&pcie3a_default>, <&pcie3a_clkreq_sleep>; > + pinctrl-names = "default", "sleep"; > > status = "okay"; > }; > @@ -568,8 +570,9 @@ &pcie4 { > > vddpe-3v3-supply = <&vreg_wlan>; > > - pinctrl-names = "default"; > - pinctrl-0 = <&pcie4_default>; > + pinctrl-0 = <&pcie4_default>, <&pcie4_clkreq_default>; > + pinctrl-1 = <&pcie4_default>, <&pcie4_clkreq_sleep>; > + pinctrl-names = "default", "sleep"; > > status = "okay"; > }; > @@ -835,13 +838,6 @@ nvme_reg_en: nvme-reg-en-state { > }; > > pcie2a_default: pcie2a-default-state { > - clkreq-n-pins { > - pins = "gpio142"; > - function = "pcie2a_clkreq"; > - drive-strength = <2>; > - bias-pull-up; > - }; > - > perst-n-pins { > pins = "gpio143"; > function = "gpio"; > @@ -857,14 +853,21 @@ wake-n-pins { > }; > }; > > - pcie3a_default: pcie3a-default-state { > - clkreq-n-pins { > - pins = "gpio150"; > - function = "pcie3a_clkreq"; > - drive-strength = <2>; > - bias-pull-up; > - }; > + pcie2a_clkreq_default: pcie2a-clkreq-default-state { > + pins = "gpio142"; > + function = "pcie2a_clkreq"; > + drive-strength = <2>; > + bias-pull-up; > + }; > + > + pcie2a_clkreq_sleep: pcie2a-clkreq-sleep-state { > + pins = "gpio142"; > + function = "gpio"; > + drive-strength = <2>; > + bias-pull-up; > + }; > > + pcie3a_default: pcie3a-default-state { > perst-n-pins { > pins = "gpio151"; > function = "gpio"; > @@ -880,14 +883,21 @@ wake-n-pins { > }; > }; > > - pcie4_default: pcie4-default-state { > - clkreq-n-pins { > - pins = "gpio140"; > - function = "pcie4_clkreq"; > - drive-strength = <2>; > - bias-pull-up; > - }; > + pcie3a_clkreq_default: pcie3a-clkreq-default-state { > + pins = "gpio150"; > + function = "pcie3a_clkreq"; > + drive-strength = <2>; > + bias-pull-up; > + }; > > + pcie3a_clkreq_sleep: pcie3a-clkreq-sleep-state { > + pins = "gpio150"; > + function = "gpio"; > + drive-strength = <2>; > + bias-pull-up; > + }; > + > + pcie4_default: pcie4-default-state { > perst-n-pins { > pins = "gpio141"; > function = "gpio"; > @@ -903,6 +913,20 @@ wake-n-pins { > }; > }; > > + pcie4_clkreq_default: pcie4-clkreq-default-state { > + pins = "gpio140"; > + function = "pcie4_clkreq"; > + drive-strength = <2>; > + bias-pull-up; > + }; > + > + pcie4_clkreq_sleep: pcie4-clkreq-sleep-state { > + pins = "gpio140"; > + function = "gpio"; > + drive-strength = <2>; > + bias-pull-up; > + }; > + > sdc2_default_state: sdc2-default-state { > clk-pins { > pins = "sdc2_clk"; > > -- > 2.43.0 > >
On 29.12.2023 18:14, Manivannan Sadhasivam wrote: > On Wed, Dec 27, 2023 at 11:28:28PM +0100, Konrad Dybcio wrote: >> The CLKREQ pin should not be muxed to its active function when the RC >> is asleep. Add the missing pin sleep states to resolve that. >> > > I don't understand why it should not be muxed and wondering what is the actual > issue you are seeing that lead to this conclusion. According to my knowledge, demuxing this pin prevents the RC from getting (possibly erronous) reference clock request signals when running the RC shutdown sequence. Reading this again, the fixes tags don't really fit this commit. What's perhaps more interesting is that on msm8996, Qualcomm vendor kernels used to remove the pull-up on the CLKREQ pin in its sleep state, while on msm8998 through sm8550 they keep it there, always.. Perhaps an oversight? Konrad
On 29.12.2023 13:55, Johan Hovold wrote: > On Wed, Dec 27, 2023 at 11:28:28PM +0100, Konrad Dybcio wrote: >> The CLKREQ pin should not be muxed to its active function when the RC >> is asleep. > > You forgot to explain *why* you think this is needed. You're right, I was in a flurry of patchsending.. > > Note that this is only appears to be done for one upstream Qualcomm SoC > (msm8996) currently, and that, notably, there is no driver support for > actually changing the pin state. Please see my reply to Mani. > >> Add the missing pin sleep states to resolve that. > >> Fixes: d907fe5acbf1 ("arm64: dts: qcom: sc8280xp-crd: enable WiFi controller") >> Fixes: 17e2ccaf65d1 ("arm64: dts: qcom: sc8280xp-crd: enable SDX55 modem") >> Fixes: 6a1ec5eca73c ("arm64: dts: qcom: sc8280xp-crd: enable NVMe SSD") > > So not sure these Fixes tags are warranted either. Agreed! Konrad
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts index ffc4406422ae..58c0c2d10cb3 100644 --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts @@ -530,8 +530,9 @@ &pcie2a { vddpe-3v3-supply = <&vreg_nvme>; - pinctrl-names = "default"; - pinctrl-0 = <&pcie2a_default>; + pinctrl-0 = <&pcie2a_default>, <&pcie2a_clkreq_default>; + pinctrl-1 = <&pcie2a_default>, <&pcie2a_clkreq_sleep>; + pinctrl-names = "default", "sleep"; status = "okay"; }; @@ -549,8 +550,9 @@ &pcie3a { vddpe-3v3-supply = <&vreg_wwan>; - pinctrl-names = "default"; - pinctrl-0 = <&pcie3a_default>; + pinctrl-0 = <&pcie3a_default>, <&pcie3a_clkreq_default>; + pinctrl-1 = <&pcie3a_default>, <&pcie3a_clkreq_sleep>; + pinctrl-names = "default", "sleep"; status = "okay"; }; @@ -568,8 +570,9 @@ &pcie4 { vddpe-3v3-supply = <&vreg_wlan>; - pinctrl-names = "default"; - pinctrl-0 = <&pcie4_default>; + pinctrl-0 = <&pcie4_default>, <&pcie4_clkreq_default>; + pinctrl-1 = <&pcie4_default>, <&pcie4_clkreq_sleep>; + pinctrl-names = "default", "sleep"; status = "okay"; }; @@ -835,13 +838,6 @@ nvme_reg_en: nvme-reg-en-state { }; pcie2a_default: pcie2a-default-state { - clkreq-n-pins { - pins = "gpio142"; - function = "pcie2a_clkreq"; - drive-strength = <2>; - bias-pull-up; - }; - perst-n-pins { pins = "gpio143"; function = "gpio"; @@ -857,14 +853,21 @@ wake-n-pins { }; }; - pcie3a_default: pcie3a-default-state { - clkreq-n-pins { - pins = "gpio150"; - function = "pcie3a_clkreq"; - drive-strength = <2>; - bias-pull-up; - }; + pcie2a_clkreq_default: pcie2a-clkreq-default-state { + pins = "gpio142"; + function = "pcie2a_clkreq"; + drive-strength = <2>; + bias-pull-up; + }; + + pcie2a_clkreq_sleep: pcie2a-clkreq-sleep-state { + pins = "gpio142"; + function = "gpio"; + drive-strength = <2>; + bias-pull-up; + }; + pcie3a_default: pcie3a-default-state { perst-n-pins { pins = "gpio151"; function = "gpio"; @@ -880,14 +883,21 @@ wake-n-pins { }; }; - pcie4_default: pcie4-default-state { - clkreq-n-pins { - pins = "gpio140"; - function = "pcie4_clkreq"; - drive-strength = <2>; - bias-pull-up; - }; + pcie3a_clkreq_default: pcie3a-clkreq-default-state { + pins = "gpio150"; + function = "pcie3a_clkreq"; + drive-strength = <2>; + bias-pull-up; + }; + pcie3a_clkreq_sleep: pcie3a-clkreq-sleep-state { + pins = "gpio150"; + function = "gpio"; + drive-strength = <2>; + bias-pull-up; + }; + + pcie4_default: pcie4-default-state { perst-n-pins { pins = "gpio141"; function = "gpio"; @@ -903,6 +913,20 @@ wake-n-pins { }; }; + pcie4_clkreq_default: pcie4-clkreq-default-state { + pins = "gpio140"; + function = "pcie4_clkreq"; + drive-strength = <2>; + bias-pull-up; + }; + + pcie4_clkreq_sleep: pcie4-clkreq-sleep-state { + pins = "gpio140"; + function = "gpio"; + drive-strength = <2>; + bias-pull-up; + }; + sdc2_default_state: sdc2-default-state { clk-pins { pins = "sdc2_clk";
The CLKREQ pin should not be muxed to its active function when the RC is asleep. Add the missing pin sleep states to resolve that. Fixes: d907fe5acbf1 ("arm64: dts: qcom: sc8280xp-crd: enable WiFi controller") Fixes: 17e2ccaf65d1 ("arm64: dts: qcom: sc8280xp-crd: enable SDX55 modem") Fixes: 6a1ec5eca73c ("arm64: dts: qcom: sc8280xp-crd: enable NVMe SSD") Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 78 ++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 27 deletions(-)