| Message ID | 20231227-topic-8280_pcie_dts-v1-2-13d12b1698ff@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | SC8280XP preparatory PCIe fixes | expand |
On Wed, Dec 27, 2023 at 11:28:27PM +0100, Konrad Dybcio wrote: > The USB GDSCs are only related to the controllers. Are you sure? > The PHYs on the other > hand, are powered by VDD_MX and their specific VDDA_PHY/PLL regulators. > > Fix the power-domains assignment to stop potentially toggling the GDSC > unnecessarily. Again, there's no additional toggling being done here, but yes, this may keep the domains enabled during suspend depending on how the driver is implemented. If that's the concern, then please spell that out too. > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform") May not be needed either. > @@ -2597,7 +2597,7 @@ usb_2_qmpphy0: phy@88ef000 { > <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>; > reset-names = "phy", "phy_phy"; > > - power-domains = <&gcc USB30_MP_GDSC>; > + power-domains = <&rpmhpd SC8280XP_MX>; Johan
On 29.12.2023 14:01, Johan Hovold wrote: > On Wed, Dec 27, 2023 at 11:28:27PM +0100, Konrad Dybcio wrote: >> The USB GDSCs are only related to the controllers. > > Are you sure? That's what I've been told from rather reliable sources. > >> The PHYs on the other >> hand, are powered by VDD_MX and their specific VDDA_PHY/PLL regulators. >> >> Fix the power-domains assignment to stop potentially toggling the GDSC >> unnecessarily. > > Again, there's no additional toggling being done here, but yes, this may > keep the domains enabled during suspend depending on how the driver is > implemented. No, it can actually happen. (Some) QMP PHYs are referenced by the DP hardware. If USB is disabled (or suspended), the DP being active will hold these GDSCs enabled. Konrad > If that's the concern, then please spell that out too. > >> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform") > > May not be needed either. > >> @@ -2597,7 +2597,7 @@ usb_2_qmpphy0: phy@88ef000 { >> <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>; >> reset-names = "phy", "phy_phy"; >> >> - power-domains = <&gcc USB30_MP_GDSC>; >> + power-domains = <&rpmhpd SC8280XP_MX>; > > Johan
[ Please remember to trim your replies and add a newline before your inline comments to make them readable. ] On Fri, Dec 29, 2023 at 02:06:26PM +0100, Konrad Dybcio wrote: > On 29.12.2023 14:01, Johan Hovold wrote: > > On Wed, Dec 27, 2023 at 11:28:27PM +0100, Konrad Dybcio wrote: > >> Fix the power-domains assignment to stop potentially toggling the GDSC > >> unnecessarily. > > > > Again, there's no additional toggling being done here, but yes, this may > > keep the domains enabled during suspend depending on how the driver is > > implemented. > No, it can actually happen. (Some) QMP PHYs are referenced by the > DP hardware. If USB is disabled (or suspended), the DP being active > will hold these GDSCs enabled. That's not a "toggling", is it? Also if the DP controller is a consumer of these PHY's why should it not prevent the PHYs from suspending? Johan
On 29.12.2023 14:25, Johan Hovold wrote: > [ Please remember to trim your replies and add a newline before your > inline comments to make them readable. ] > > On Fri, Dec 29, 2023 at 02:06:26PM +0100, Konrad Dybcio wrote: >> On 29.12.2023 14:01, Johan Hovold wrote: >>> On Wed, Dec 27, 2023 at 11:28:27PM +0100, Konrad Dybcio wrote: > >>>> Fix the power-domains assignment to stop potentially toggling the GDSC >>>> unnecessarily. >>> >>> Again, there's no additional toggling being done here, but yes, this may >>> keep the domains enabled during suspend depending on how the driver is >>> implemented. > >> No, it can actually happen. (Some) QMP PHYs are referenced by the >> DP hardware. If USB is disabled (or suspended), the DP being active >> will hold these GDSCs enabled. > > That's not a "toggling", is it? Also if the DP controller is a consumer of > these PHY's why should it not prevent the PHYs from suspending? As far as I'm concerned, "toggling" is the correct word for "switching it on".. While the PHYs are indeed useful for getting displayport to work, the USB controller itself may not be necessary there, so enabling its power line would be a bit of a waste.. Konrad
On Fri, Dec 29, 2023 at 04:05:18PM +0100, Konrad Dybcio wrote: > On 29.12.2023 14:25, Johan Hovold wrote: > > On Fri, Dec 29, 2023 at 02:06:26PM +0100, Konrad Dybcio wrote: > >> On 29.12.2023 14:01, Johan Hovold wrote: > >>> On Wed, Dec 27, 2023 at 11:28:27PM +0100, Konrad Dybcio wrote: > > > >>>> Fix the power-domains assignment to stop potentially toggling the GDSC > >>>> unnecessarily. > >>> > >>> Again, there's no additional toggling being done here, but yes, this may > >>> keep the domains enabled during suspend depending on how the driver is > >>> implemented. > > > >> No, it can actually happen. (Some) QMP PHYs are referenced by the > >> DP hardware. If USB is disabled (or suspended), the DP being active > >> will hold these GDSCs enabled. > > > > That's not a "toggling", is it? Also if the DP controller is a consumer of > > these PHY's why should it not prevent the PHYs from suspending? > > As far as I'm concerned, "toggling" is the correct word for "switching it > on".. Hmm, this doesn't make sense. The PHY power domain will be disabled when the PHY is suspended, regardless of the DP controller. But sure, a system with USB disabled, would end up with the USB GDSC on. > While the PHYs are indeed useful for getting displayport to work, > the USB controller itself may not be necessary there, so enabling its > power line would be a bit of a waste.. Sure, if the PHYs truly don't need the USB PD then fine, this just doesn't seem to be case for PCIe, or at least the picture isn't as clear as your previous commit message suggested. Johan
On Fri, Dec 29, 2023 at 02:01:06PM +0100, Johan Hovold wrote: > On Wed, Dec 27, 2023 at 11:28:27PM +0100, Konrad Dybcio wrote: > > The USB GDSCs are only related to the controllers. > > Are you sure? > Yes, that's what I was told by UFS and PCIe teams and some of the internal documentation also confirms the same. > > The PHYs on the other > > hand, are powered by VDD_MX and their specific VDDA_PHY/PLL regulators. > > > > Fix the power-domains assignment to stop potentially toggling the GDSC > > unnecessarily. > > Again, there's no additional toggling being done here, but yes, this may > keep the domains enabled during suspend depending on how the driver is > implemented. > > If that's the concern, then please spell that out too. > > > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform") > > May not be needed either. > Fixes tag is indeed needed on this platform and all other platforms too. - Mani > > @@ -2597,7 +2597,7 @@ usb_2_qmpphy0: phy@88ef000 { > > <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>; > > reset-names = "phy", "phy_phy"; > > > > - power-domains = <&gcc USB30_MP_GDSC>; > > + power-domains = <&rpmhpd SC8280XP_MX>; > > Johan >
On Fri, Dec 29, 2023 at 10:40:08PM +0530, Manivannan Sadhasivam wrote: > On Fri, Dec 29, 2023 at 02:01:06PM +0100, Johan Hovold wrote: > > On Wed, Dec 27, 2023 at 11:28:27PM +0100, Konrad Dybcio wrote: > > > The USB GDSCs are only related to the controllers. > > > > Are you sure? > > Yes, that's what I was told by UFS and PCIe teams and some of the internal > documentation also confirms the same. Ok, good. I'm not sure I did a corresponding test of powering on a USB PHY without the corresponding USB GDSC enabled, so perhaps the issue I noted only applies to PCIe. Johan
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi index 72c5818b67f2..4b18a0762ca7 100644 --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi @@ -2597,7 +2597,7 @@ usb_2_qmpphy0: phy@88ef000 { <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>; reset-names = "phy", "phy_phy"; - power-domains = <&gcc USB30_MP_GDSC>; + power-domains = <&rpmhpd SC8280XP_MX>; #clock-cells = <0>; clock-output-names = "usb2_phy0_pipe_clk"; @@ -2621,7 +2621,7 @@ usb_2_qmpphy1: phy@88f1000 { <&gcc GCC_USB3UNIPHY_PHY_MP1_BCR>; reset-names = "phy", "phy_phy"; - power-domains = <&gcc USB30_MP_GDSC>; + power-domains = <&rpmhpd SC8280XP_MX>; #clock-cells = <0>; clock-output-names = "usb2_phy1_pipe_clk"; @@ -3109,7 +3109,7 @@ usb_0_qmpphy: phy@88eb000 { <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>; clock-names = "aux", "ref", "com_aux", "usb3_pipe"; - power-domains = <&gcc USB30_PRIM_GDSC>; + power-domains = <&rpmhpd SC8280XP_MX>; resets = <&gcc GCC_USB3_PHY_PRIM_BCR>, <&gcc GCC_USB4_DP_PHY_PRIM_BCR>; @@ -3162,7 +3162,7 @@ usb_1_qmpphy: phy@8903000 { <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>; clock-names = "aux", "ref", "com_aux", "usb3_pipe"; - power-domains = <&gcc USB30_SEC_GDSC>; + power-domains = <&rpmhpd SC8280XP_MX>; resets = <&gcc GCC_USB3_PHY_SEC_BCR>, <&gcc GCC_USB4_1_DP_PHY_PRIM_BCR>;
The USB GDSCs are only related to the controllers. The PHYs on the other hand, are powered by VDD_MX and their specific VDDA_PHY/PLL regulators. Fix the power-domains assignment to stop potentially toggling the GDSC unnecessarily. Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform") Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)