diff mbox series

[1/2] dt-bindings: msm/dp: List supplies in the bindings

Message ID 20220425140619.1.Ibfde5a26a7182c4b478d570c23d2649823ac2cce@changeid (mailing list archive)
State Not Applicable
Headers show
Series dt-bindings: List supplies needed for sc7280 eDP | expand

Commit Message

Doug Anderson April 25, 2022, 9:06 p.m. UTC
We're supposed to list the supplies in the dt bindings but there are
none in the DP controller bindings. Looking at the Linux driver and
existing device trees, we can see that two supplies are expected:
- vdda-0p9-supply
- vdda-1p2-supply

Let's list them both in the bindings. Note that the datasheet for
sc7280 doesn't describe these supplies very verbosely. For the 0p9
supply, for instance, it says "Power for eDP 0.9 V circuits". This
this is obvious from the property name, we don't bother cluttering the
bindings with a description.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../devicetree/bindings/display/msm/dp-controller.yaml      | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Stephen Boyd April 25, 2022, 9:14 p.m. UTC | #1
Quoting Douglas Anderson (2022-04-25 14:06:42)
> We're supposed to list the supplies in the dt bindings but there are
> none in the DP controller bindings. Looking at the Linux driver and
> existing device trees, we can see that two supplies are expected:
> - vdda-0p9-supply
> - vdda-1p2-supply
>
> Let's list them both in the bindings. Note that the datasheet for
> sc7280 doesn't describe these supplies very verbosely. For the 0p9
> supply, for instance, it says "Power for eDP 0.9 V circuits". This
> this is obvious from the property name, we don't bother cluttering the
> bindings with a description.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  .../devicetree/bindings/display/msm/dp-controller.yaml      | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index cd05cfd76536..dba31108db51 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -76,6 +76,9 @@ properties:
>    "#sound-dai-cells":
>      const: 0
>
> +  vdda-0p9-supply: true
> +  vdda-1p2-supply: true
> +
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>      properties:
> @@ -137,6 +140,9 @@ examples:
>
>          power-domains = <&rpmhpd SC7180_CX>;
>
> +        vdda-0p9-supply = <&vdda_usb_ss_dp_core>;

Having 'a' in 'vdda' typically means 'analog' for 'analog' circuits, so
I'd expect this to only matter for the phy that contains the analog
circuitry. It would be great to remove the regulator code from
drivers/gpu/drm/msm/dp/dp_power.c and move the regulator_set_load() call
to the phy driver if possible. Hopefully qcom folks can help clarify
here.

> +        vdda-1p2-supply = <&vdda_usb_ss_dp_1p2>;
Doug Anderson April 25, 2022, 9:18 p.m. UTC | #2
Hi,

On Mon, Apr 25, 2022 at 2:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Douglas Anderson (2022-04-25 14:06:42)
> > We're supposed to list the supplies in the dt bindings but there are
> > none in the DP controller bindings. Looking at the Linux driver and
> > existing device trees, we can see that two supplies are expected:
> > - vdda-0p9-supply
> > - vdda-1p2-supply
> >
> > Let's list them both in the bindings. Note that the datasheet for
> > sc7280 doesn't describe these supplies very verbosely. For the 0p9
> > supply, for instance, it says "Power for eDP 0.9 V circuits". This
> > this is obvious from the property name, we don't bother cluttering the
> > bindings with a description.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  .../devicetree/bindings/display/msm/dp-controller.yaml      | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > index cd05cfd76536..dba31108db51 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > @@ -76,6 +76,9 @@ properties:
> >    "#sound-dai-cells":
> >      const: 0
> >
> > +  vdda-0p9-supply: true
> > +  vdda-1p2-supply: true
> > +
> >    ports:
> >      $ref: /schemas/graph.yaml#/properties/ports
> >      properties:
> > @@ -137,6 +140,9 @@ examples:
> >
> >          power-domains = <&rpmhpd SC7180_CX>;
> >
> > +        vdda-0p9-supply = <&vdda_usb_ss_dp_core>;
>
> Having 'a' in 'vdda' typically means 'analog' for 'analog' circuits, so
> I'd expect this to only matter for the phy that contains the analog
> circuitry. It would be great to remove the regulator code from
> drivers/gpu/drm/msm/dp/dp_power.c and move the regulator_set_load() call
> to the phy driver if possible. Hopefully qcom folks can help clarify
> here.

Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A".
It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the
schematic in front of me someone labeled these pins on the sc7280 with
the "A". ...and the driver looks for a supply with the "a". :-/

It would be good to get clarification from someone with better information.

-Doug
Sankeerth Billakanti May 5, 2022, 6:02 p.m. UTC | #3
Hi Doug,

>>
>> Quoting Douglas Anderson (2022-04-25 14:06:42)
>> > We're supposed to list the supplies in the dt bindings but there are
>> > none in the DP controller bindings. Looking at the Linux driver and
>> > existing device trees, we can see that two supplies are expected:
>> > - vdda-0p9-supply
>> > - vdda-1p2-supply
>> >
>> > Let's list them both in the bindings. Note that the datasheet for
>> > sc7280 doesn't describe these supplies very verbosely. For the 0p9
>> > supply, for instance, it says "Power for eDP 0.9 V circuits". This
>> > this is obvious from the property name, we don't bother cluttering
>> > the bindings with a description.
>> >
>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> > ---
>> >
>> >  .../devicetree/bindings/display/msm/dp-controller.yaml      | 6 ++++++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git
>> > a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> > index cd05cfd76536..dba31108db51 100644
>> > ---
>> > a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.ya
>> > +++ ml
>> > @@ -76,6 +76,9 @@ properties:
>> >    "#sound-dai-cells":
>> >      const: 0
>> >
>> > +  vdda-0p9-supply: true
>> > +  vdda-1p2-supply: true
>> > +
>> >    ports:
>> >      $ref: /schemas/graph.yaml#/properties/ports
>> >      properties:
>> > @@ -137,6 +140,9 @@ examples:
>> >
>> >          power-domains = <&rpmhpd SC7180_CX>;
>> >
>> > +        vdda-0p9-supply = <&vdda_usb_ss_dp_core>;
>>
>> Having 'a' in 'vdda' typically means 'analog' for 'analog' circuits,
>> so I'd expect this to only matter for the phy that contains the analog
>> circuitry. It would be great to remove the regulator code from
>> drivers/gpu/drm/msm/dp/dp_power.c and move the regulator_set_load()
>> call to the phy driver if possible. Hopefully qcom folks can help
>> clarify here.
>
>Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A".
>It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the
>schematic in front of me someone labeled these pins on the sc7280 with the
>"A". ...and the driver looks for a supply with the "a". :-/
>
>It would be good to get clarification from someone with better information.
>
>-Doug

Our internal power grid documents list the regulators as VDD_A_*_1P2 and VDD_A_*_0P9
for all the platforms.

So, as a practice, we put the same name in the DT files. Hence,

Reviewed-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>

Thank you,
Sankeerth
Stephen Boyd May 5, 2022, 6:18 p.m. UTC | #4
Quoting Sankeerth Billakanti (2022-05-05 11:02:36)
> >>
> >> Quoting Douglas Anderson (2022-04-25 14:06:42)
> >>
> >> Having 'a' in 'vdda' typically means 'analog' for 'analog' circuits,
> >> so I'd expect this to only matter for the phy that contains the analog
> >> circuitry. It would be great to remove the regulator code from
> >> drivers/gpu/drm/msm/dp/dp_power.c and move the regulator_set_load()
> >> call to the phy driver if possible. Hopefully qcom folks can help
> >> clarify here.
> >
> >Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A".
> >It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the
> >schematic in front of me someone labeled these pins on the sc7280 with the
> >"A". ...and the driver looks for a supply with the "a". :-/
> >
> >It would be good to get clarification from someone with better information.
> >
> >-Doug
>
> Our internal power grid documents list the regulators as VDD_A_*_1P2 and VDD_A_*_0P9
> for all the platforms.

Do your internal power grid documents indicate what these supplies are
powering? The question is if these supplies power any of the logic
inside the eDP controller or if they only supply power to the analog
circuits in the eDP phy. If it's the eDP phy only then the regulator
usage in the eDP driver should be removed. I would suspect this is the
case because the controller is probably all digital logic and runs at
the typical 1.8V that the rest of the SoC uses. Similarly, these are
voltage references which sound like a PLL reference voltage.

Please clarify this further.

>
> So, as a practice, we put the same name in the DT files. Hence,
>
> Reviewed-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>
Sankeerth Billakanti (QUIC) May 5, 2022, 6:47 p.m. UTC | #5
>Quoting Sankeerth Billakanti (2022-05-05 11:02:36)
>> >>
>> >> Quoting Douglas Anderson (2022-04-25 14:06:42)
>> >>
>> >> Having 'a' in 'vdda' typically means 'analog' for 'analog'
>> >> circuits, so I'd expect this to only matter for the phy that
>> >> contains the analog circuitry. It would be great to remove the
>> >> regulator code from drivers/gpu/drm/msm/dp/dp_power.c and move
>the
>> >> regulator_set_load() call to the phy driver if possible. Hopefully
>> >> qcom folks can help clarify here.
>> >
>> >Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A".
>> >It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the
>> >schematic in front of me someone labeled these pins on the sc7280
>> >with the "A". ...and the driver looks for a supply with the "a". :-/
>> >
>> >It would be good to get clarification from someone with better
>information.
>> >
>> >-Doug
>>
>> Our internal power grid documents list the regulators as VDD_A_*_1P2
>> and VDD_A_*_0P9 for all the platforms.
>
>Do your internal power grid documents indicate what these supplies are
>powering? The question is if these supplies power any of the logic inside the
>eDP controller or if they only supply power to the analog circuits in the eDP
>phy. If it's the eDP phy only then the regulator usage in the eDP driver should
>be removed. I would suspect this is the case because the controller is
>probably all digital logic and runs at the typical 1.8V that the rest of the SoC
>uses. Similarly, these are voltage references which sound like a PLL reference
>voltage.
>
>Please clarify this further.
>

For the DP driver using the usb-dp combo phy, there were cases where the usb driver
was turning off the phy and pll regulators whenever usb-dp concurrent mode need not be supported.
This caused phy and pll to be powered down causing aux transaction failures and display blankouts. 
From then on, it became a practice for the controller driver to vote for the phy and pll regulators also.

>>
>> So, as a practice, we put the same name in the DT files. Hence,
>>
>> Reviewed-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>>
Stephen Boyd May 5, 2022, 7:06 p.m. UTC | #6
Quoting Sankeerth Billakanti (QUIC) (2022-05-05 11:47:20)
> >Quoting Sankeerth Billakanti (2022-05-05 11:02:36)
> >>
> >> Our internal power grid documents list the regulators as VDD_A_*_1P2
> >> and VDD_A_*_0P9 for all the platforms.
> >
> >Do your internal power grid documents indicate what these supplies are
> >powering? The question is if these supplies power any of the logic inside the
> >eDP controller or if they only supply power to the analog circuits in the eDP
> >phy. If it's the eDP phy only then the regulator usage in the eDP driver should
> >be removed. I would suspect this is the case because the controller is
> >probably all digital logic and runs at the typical 1.8V that the rest of the SoC
> >uses. Similarly, these are voltage references which sound like a PLL reference
> >voltage.
> >
> >Please clarify this further.
> >
>
> For the DP driver using the usb-dp combo phy, there were cases where the usb driver
> was turning off the phy and pll regulators whenever usb-dp concurrent mode need not be supported.
> This caused phy and pll to be powered down causing aux transaction failures and display blankouts.
> From then on, it became a practice for the controller driver to vote for the phy and pll regulators also.
>

That sounds like USB-DP combo phy driver had improper regulator power
management where aux transactions from DP didn't keep the power on to
the phy. Where does the power physically go? If the power isn't
physically going to the DP controller it shouldn't be controlled from
the DP controller driver. If the aux bus needs the DP phy enabled, the
DP controller driver should enable the phy power (via phy_power_on()?).
Sankeerth Billakanti (QUIC) May 6, 2022, 1:36 p.m. UTC | #7
>> >> Our internal power grid documents list the regulators as
>> >> VDD_A_*_1P2 and VDD_A_*_0P9 for all the platforms.
>> >
>> >Do your internal power grid documents indicate what these supplies
>> >are powering? The question is if these supplies power any of the
>> >logic inside the eDP controller or if they only supply power to the
>> >analog circuits in the eDP phy. If it's the eDP phy only then the
>> >regulator usage in the eDP driver should be removed. I would suspect
>> >this is the case because the controller is probably all digital logic
>> >and runs at the typical 1.8V that the rest of the SoC uses.
>> >Similarly, these are voltage references which sound like a PLL reference
>voltage.
>> >
>> >Please clarify this further.
>> >
>>
>> For the DP driver using the usb-dp combo phy, there were cases where
>> the usb driver was turning off the phy and pll regulators whenever usb-dp
>concurrent mode need not be supported.
>> This caused phy and pll to be powered down causing aux transaction failures
>and display blankouts.
>> From then on, it became a practice for the controller driver to vote for the
>phy and pll regulators also.
>>
>
>That sounds like USB-DP combo phy driver had improper regulator power
>management where aux transactions from DP didn't keep the power on to
>the phy. Where does the power physically go? If the power isn't physically
>going to the DP controller it shouldn't be controlled from the DP controller
>driver. If the aux bus needs the DP phy enabled, the DP controller driver
>should enable the phy power (via phy_power_on()?).

Yes, it was limitation earlier when we did not have proper interface to interact
with the combo phy.

In this case, the power from the regulators go to the combo phy.

Now that there is an interface for the controller to interact with the
combo phy, the proposal to drop the phy regulator voting from the controller
driver seems reasonable to me.

The phy_power_on() is used for getting the phy out of low power state or getting
it ready for data transfer.

The controller driver needs to enable the phy power via the phy_init() before
any interaction with the sink like the aux transactions or before sending the data.
The controller can disable the regulators via the phy_exit() call.

Thank you,
Sankeerth
Doug Anderson May 11, 2022, 6:21 p.m. UTC | #8
Hi,

On Fri, May 6, 2022 at 6:36 AM Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> >> >> Our internal power grid documents list the regulators as
> >> >> VDD_A_*_1P2 and VDD_A_*_0P9 for all the platforms.
> >> >
> >> >Do your internal power grid documents indicate what these supplies
> >> >are powering? The question is if these supplies power any of the
> >> >logic inside the eDP controller or if they only supply power to the
> >> >analog circuits in the eDP phy. If it's the eDP phy only then the
> >> >regulator usage in the eDP driver should be removed. I would suspect
> >> >this is the case because the controller is probably all digital logic
> >> >and runs at the typical 1.8V that the rest of the SoC uses.
> >> >Similarly, these are voltage references which sound like a PLL reference
> >voltage.
> >> >
> >> >Please clarify this further.
> >> >
> >>
> >> For the DP driver using the usb-dp combo phy, there were cases where
> >> the usb driver was turning off the phy and pll regulators whenever usb-dp
> >concurrent mode need not be supported.
> >> This caused phy and pll to be powered down causing aux transaction failures
> >and display blankouts.
> >> From then on, it became a practice for the controller driver to vote for the
> >phy and pll regulators also.
> >>
> >
> >That sounds like USB-DP combo phy driver had improper regulator power
> >management where aux transactions from DP didn't keep the power on to
> >the phy. Where does the power physically go? If the power isn't physically
> >going to the DP controller it shouldn't be controlled from the DP controller
> >driver. If the aux bus needs the DP phy enabled, the DP controller driver
> >should enable the phy power (via phy_power_on()?).
>
> Yes, it was limitation earlier when we did not have proper interface to interact
> with the combo phy.
>
> In this case, the power from the regulators go to the combo phy.
>
> Now that there is an interface for the controller to interact with the
> combo phy, the proposal to drop the phy regulator voting from the controller
> driver seems reasonable to me.
>
> The phy_power_on() is used for getting the phy out of low power state or getting
> it ready for data transfer.
>
> The controller driver needs to enable the phy power via the phy_init() before
> any interaction with the sink like the aux transactions or before sending the data.
> The controller can disable the regulators via the phy_exit() call.

I can confirm that if I stop providing these regulators to the DP
controller that the screen still comes up. ...but also there are lots
of other things (including the PHY) that power these regulators up...

From offline discussion with folks:

1. It sounds like maybe the code for handling the regulators in the DP
controller leaked in from downstream where the DP driver itself
controls more stuff.

2. We should probably remove these regulators from the DP controller.

3. When we remove this from the DP controller, we'll have to make sure
that the PHY driver calls regulator_set_load() as needed.

Kuogee has volunteered to own this issue and send out patches fixing
the stuff up. So for now, please consider ${SUBJECT} patch abandoned.

-Doug
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index cd05cfd76536..dba31108db51 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -76,6 +76,9 @@  properties:
   "#sound-dai-cells":
     const: 0
 
+  vdda-0p9-supply: true
+  vdda-1p2-supply: true
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
     properties:
@@ -137,6 +140,9 @@  examples:
 
         power-domains = <&rpmhpd SC7180_CX>;
 
+        vdda-0p9-supply = <&vdda_usb_ss_dp_core>;
+        vdda-1p2-supply = <&vdda_usb_ss_dp_1p2>;
+
         ports {
             #address-cells = <1>;
             #size-cells = <0>;