diff mbox series

[2/3] arm64: dts: qcom: sc8280xp: Correct USB PHY power domains

Message ID 20231227-topic-8280_pcie_dts-v1-2-13d12b1698ff@linaro.org
State New
Headers
Series SC8280XP preparatory PCIe fixes |

Commit Message

Konrad Dybcio Dec. 27, 2023, 10:28 p.m. UTC 
  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(-)

Comments

Johan Hovold Dec. 29, 2023, 1:01 p.m. UTC | #1 
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
Konrad Dybcio Dec. 29, 2023, 1:06 p.m. UTC | #2 
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
Johan Hovold Dec. 29, 2023, 1:25 p.m. UTC | #3 
[ 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
Konrad Dybcio Dec. 29, 2023, 3:05 p.m. UTC | #4 
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
Johan Hovold Dec. 29, 2023, 3:43 p.m. UTC | #5 
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
Manivannan Sadhasivam Dec. 29, 2023, 5:10 p.m. UTC | #6 
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
>
Johan Hovold Jan. 2, 2024, 8:55 a.m. UTC | #7 
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 mbox series

Patch

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>;