diff mbox series

[15/15] arm64: dts: marvell: Convert 7k/8k usb-phy properties to phy-supply

Message ID 20190401165131.23370-16-miquel.raynal@bootlin.com
State New, archived
Headers show
Series Enhance CP110 COMPHY support | expand

Commit Message

Miquel Raynal April 1, 2019, 4:51 p.m. UTC
Update Aramda 7k/8k DTs to use the phy-supply property of the (recent)
generic PHY framework instead of the (legacy) usb-phy preperty. Both
enable the supply when the PHY is enabled.

The COMPHY nodes only provide SERDES lanes configuration. The power
supply that is represented by the phy-supply property is just a
regulator wired to the USB connector, hence the creation of connector
nodes as child of the COMPHY nodes and the supply attached to it.

Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../arm64/boot/dts/marvell/armada-7040-db.dts | 26 ++++++++++---------
 .../marvell/armada-8040-clearfog-gt-8k.dts    | 13 +++++-----
 .../arm64/boot/dts/marvell/armada-8040-db.dts | 13 +++++-----
 .../boot/dts/marvell/armada-8040-mcbin.dtsi   | 13 +++++-----
 4 files changed, 35 insertions(+), 30 deletions(-)

Comments

Martin Blumenstingl April 2, 2019, 7:35 p.m. UTC | #1
Hi Miquel,

thank you for keeping me in the loop!

On Mon, Apr 1, 2019 at 6:52 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Update Aramda 7k/8k DTs to use the phy-supply property of the (recent)
> generic PHY framework instead of the (legacy) usb-phy preperty. Both
> enable the supply when the PHY is enabled.
>
> The COMPHY nodes only provide SERDES lanes configuration. The power
> supply that is represented by the phy-supply property is just a
> regulator wired to the USB connector, hence the creation of connector
> nodes as child of the COMPHY nodes and the supply attached to it.
shouldn't this also be reflected in the dt-bindings?

> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../arm64/boot/dts/marvell/armada-7040-db.dts | 26 ++++++++++---------
>  .../marvell/armada-8040-clearfog-gt-8k.dts    | 13 +++++-----
>  .../arm64/boot/dts/marvell/armada-8040-db.dts | 13 +++++-----
>  .../boot/dts/marvell/armada-8040-mcbin.dtsi   | 13 +++++-----
>  4 files changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> index 131ce4229db0..4b8df359b1cc 100644
> --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> @@ -45,16 +45,6 @@
>                 enable-active-high;
>                 gpio = <&expander0 1 GPIO_ACTIVE_HIGH>;
>         };
> -
> -       cp0_usb3_0_phy: cp0-usb3-0-phy {
> -               compatible = "usb-nop-xceiv";
> -               vcc-supply = <&cp0_reg_usb3_0_vbus>;
> -       };
> -
> -       cp0_usb3_1_phy: cp0-usb3-1-phy {
> -               compatible = "usb-nop-xceiv";
> -               vcc-supply = <&cp0_reg_usb3_1_vbus>;
> -       };
>  };
>
>  &i2c0 {
> @@ -200,15 +190,27 @@
>         };
>  };
>
> +&cp0_comphy1 {
> +       cp0_usbh0_con: connector {
> +               compatible = "usb-a-connector";
> +               phy-supply = <&cp0_reg_usb3_0_vbus>;
> +       };
> +};
(disclaimer: I don't have any board with a marvell SoC, so I don't
understand how it works and I can't debug it)
I know about the "phy-supply" property inside the PHY node itself
(that would be cp0_comphy1 in this case).
The connector binding does not mention a phy-supply property:
Documentation/devicetree/bindings/connector/usb-connector.txt

I don't understand which driver enables the phy-supply when it's part
of a "connector" child-node.
do you have a hint where I should start looking?


Regards
Martin
Miquel Raynal April 3, 2019, 8:06 a.m. UTC | #2
Hi Martin,

Rob, a few questions for you below :)

Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Tue,
2 Apr 2019 21:35:26 +0200:

> Hi Miquel,
> 
> thank you for keeping me in the loop!
> 
> On Mon, Apr 1, 2019 at 6:52 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Update Aramda 7k/8k DTs to use the phy-supply property of the (recent)
> > generic PHY framework instead of the (legacy) usb-phy preperty. Both
> > enable the supply when the PHY is enabled.
> >
> > The COMPHY nodes only provide SERDES lanes configuration. The power
> > supply that is represented by the phy-supply property is just a
> > regulator wired to the USB connector, hence the creation of connector
> > nodes as child of the COMPHY nodes and the supply attached to it.  
> shouldn't this also be reflected in the dt-bindings?

I don't think it deserves an update in the bindings as this is just an
update for a better hardware representation. The COMPHY block is
already documented, the connector one too. I thought adding a connector
node in the COMPHY node was okay when I read Rob's answer to your
series as this has nothing to do with the IP itself but is just
describing how the board is built. Maybe Rob can confirm this?

> 
> > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../arm64/boot/dts/marvell/armada-7040-db.dts | 26 ++++++++++---------
> >  .../marvell/armada-8040-clearfog-gt-8k.dts    | 13 +++++-----
> >  .../arm64/boot/dts/marvell/armada-8040-db.dts | 13 +++++-----
> >  .../boot/dts/marvell/armada-8040-mcbin.dtsi   | 13 +++++-----
> >  4 files changed, 35 insertions(+), 30 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > index 131ce4229db0..4b8df359b1cc 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > @@ -45,16 +45,6 @@
> >                 enable-active-high;
> >                 gpio = <&expander0 1 GPIO_ACTIVE_HIGH>;
> >         };
> > -
> > -       cp0_usb3_0_phy: cp0-usb3-0-phy {
> > -               compatible = "usb-nop-xceiv";
> > -               vcc-supply = <&cp0_reg_usb3_0_vbus>;
> > -       };
> > -
> > -       cp0_usb3_1_phy: cp0-usb3-1-phy {
> > -               compatible = "usb-nop-xceiv";
> > -               vcc-supply = <&cp0_reg_usb3_1_vbus>;
> > -       };
> >  };
> >
> >  &i2c0 {
> > @@ -200,15 +190,27 @@
> >         };
> >  };
> >
> > +&cp0_comphy1 {
> > +       cp0_usbh0_con: connector {
> > +               compatible = "usb-a-connector";
> > +               phy-supply = <&cp0_reg_usb3_0_vbus>;
> > +       };
> > +};  
> (disclaimer: I don't have any board with a marvell SoC, so I don't
> understand how it works and I can't debug it)
> I know about the "phy-supply" property inside the PHY node itself
> (that would be cp0_comphy1 in this case).
> The connector binding does not mention a phy-supply property:
> Documentation/devicetree/bindings/connector/usb-connector.txt

That's right, can Rob confirm that this is valid? If yes I can add an
optional "phy-supply" property there.

> I don't understand which driver enables the phy-supply when it's part
> of a "connector" child-node.
> do you have a hint where I should start looking?

I honestly have no idea, but apparently it works.


Thanks,
Miquèl
Miquel Raynal April 8, 2019, 8:51 a.m. UTC | #3
Hi Rob,

Gentle ping on the questions below, if you have some time to check.

Thank you very much,
Miquèl

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Wed, 3 Apr 2019
10:06:42 +0200:

> Hi Martin,
> 
> Rob, a few questions for you below :)
> 
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Tue,
> 2 Apr 2019 21:35:26 +0200:
> 
> > Hi Miquel,
> > 
> > thank you for keeping me in the loop!
> > 
> > On Mon, Apr 1, 2019 at 6:52 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > >
> > > Update Aramda 7k/8k DTs to use the phy-supply property of the (recent)
> > > generic PHY framework instead of the (legacy) usb-phy preperty. Both
> > > enable the supply when the PHY is enabled.
> > >
> > > The COMPHY nodes only provide SERDES lanes configuration. The power
> > > supply that is represented by the phy-supply property is just a
> > > regulator wired to the USB connector, hence the creation of connector
> > > nodes as child of the COMPHY nodes and the supply attached to it.    
> > shouldn't this also be reflected in the dt-bindings?  
> 
> I don't think it deserves an update in the bindings as this is just an
> update for a better hardware representation. The COMPHY block is
> already documented, the connector one too. I thought adding a connector
> node in the COMPHY node was okay when I read Rob's answer to your
> series as this has nothing to do with the IP itself but is just
> describing how the board is built. Maybe Rob can confirm this?
> 
> >   
> > > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  .../arm64/boot/dts/marvell/armada-7040-db.dts | 26 ++++++++++---------
> > >  .../marvell/armada-8040-clearfog-gt-8k.dts    | 13 +++++-----
> > >  .../arm64/boot/dts/marvell/armada-8040-db.dts | 13 +++++-----
> > >  .../boot/dts/marvell/armada-8040-mcbin.dtsi   | 13 +++++-----
> > >  4 files changed, 35 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > index 131ce4229db0..4b8df359b1cc 100644
> > > --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > @@ -45,16 +45,6 @@
> > >                 enable-active-high;
> > >                 gpio = <&expander0 1 GPIO_ACTIVE_HIGH>;
> > >         };
> > > -
> > > -       cp0_usb3_0_phy: cp0-usb3-0-phy {
> > > -               compatible = "usb-nop-xceiv";
> > > -               vcc-supply = <&cp0_reg_usb3_0_vbus>;
> > > -       };
> > > -
> > > -       cp0_usb3_1_phy: cp0-usb3-1-phy {
> > > -               compatible = "usb-nop-xceiv";
> > > -               vcc-supply = <&cp0_reg_usb3_1_vbus>;
> > > -       };
> > >  };
> > >
> > >  &i2c0 {
> > > @@ -200,15 +190,27 @@
> > >         };
> > >  };
> > >
> > > +&cp0_comphy1 {
> > > +       cp0_usbh0_con: connector {
> > > +               compatible = "usb-a-connector";
> > > +               phy-supply = <&cp0_reg_usb3_0_vbus>;
> > > +       };
> > > +};    
> > (disclaimer: I don't have any board with a marvell SoC, so I don't
> > understand how it works and I can't debug it)
> > I know about the "phy-supply" property inside the PHY node itself
> > (that would be cp0_comphy1 in this case).
> > The connector binding does not mention a phy-supply property:
> > Documentation/devicetree/bindings/connector/usb-connector.txt  
> 
> That's right, can Rob confirm that this is valid? If yes I can add an
> optional "phy-supply" property there.
> 
> > I don't understand which driver enables the phy-supply when it's part
> > of a "connector" child-node.
> > do you have a hint where I should start looking?  
> 
> I honestly have no idea, but apparently it works.
> 
> 
> Thanks,
> Miquèl
Martin Blumenstingl April 8, 2019, 7:07 p.m. UTC | #4
Hi Miquel,

On Mon, Apr 8, 2019 at 10:51 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Rob,
>
> Gentle ping on the questions below, if you have some time to check.
>
> Thank you very much,
> Miquèl
>
> Miquel Raynal <miquel.raynal@bootlin.com> wrote on Wed, 3 Apr 2019
> 10:06:42 +0200:
>
> > Hi Martin,
> >
> > Rob, a few questions for you below :)
> >
> > Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Tue,
> > 2 Apr 2019 21:35:26 +0200:
> >
> > > Hi Miquel,
> > >
> > > thank you for keeping me in the loop!
> > >
> > > On Mon, Apr 1, 2019 at 6:52 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >
> > > > Update Aramda 7k/8k DTs to use the phy-supply property of the (recent)
> > > > generic PHY framework instead of the (legacy) usb-phy preperty. Both
> > > > enable the supply when the PHY is enabled.
> > > >
> > > > The COMPHY nodes only provide SERDES lanes configuration. The power
> > > > supply that is represented by the phy-supply property is just a
> > > > regulator wired to the USB connector, hence the creation of connector
> > > > nodes as child of the COMPHY nodes and the supply attached to it.
> > > shouldn't this also be reflected in the dt-bindings?
> >
> > I don't think it deserves an update in the bindings as this is just an
> > update for a better hardware representation. The COMPHY block is
> > already documented, the connector one too. I thought adding a connector
> > node in the COMPHY node was okay when I read Rob's answer to your
> > series as this has nothing to do with the IP itself but is just
> > describing how the board is built. Maybe Rob can confirm this?
> >
> > >
> > > > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  .../arm64/boot/dts/marvell/armada-7040-db.dts | 26 ++++++++++---------
> > > >  .../marvell/armada-8040-clearfog-gt-8k.dts    | 13 +++++-----
> > > >  .../arm64/boot/dts/marvell/armada-8040-db.dts | 13 +++++-----
> > > >  .../boot/dts/marvell/armada-8040-mcbin.dtsi   | 13 +++++-----
> > > >  4 files changed, 35 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > > index 131ce4229db0..4b8df359b1cc 100644
> > > > --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > > +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > > @@ -45,16 +45,6 @@
> > > >                 enable-active-high;
> > > >                 gpio = <&expander0 1 GPIO_ACTIVE_HIGH>;
> > > >         };
> > > > -
> > > > -       cp0_usb3_0_phy: cp0-usb3-0-phy {
> > > > -               compatible = "usb-nop-xceiv";
> > > > -               vcc-supply = <&cp0_reg_usb3_0_vbus>;
> > > > -       };
> > > > -
> > > > -       cp0_usb3_1_phy: cp0-usb3-1-phy {
> > > > -               compatible = "usb-nop-xceiv";
> > > > -               vcc-supply = <&cp0_reg_usb3_1_vbus>;
> > > > -       };
> > > >  };
> > > >
> > > >  &i2c0 {
> > > > @@ -200,15 +190,27 @@
> > > >         };
> > > >  };
> > > >
> > > > +&cp0_comphy1 {
> > > > +       cp0_usbh0_con: connector {
> > > > +               compatible = "usb-a-connector";
> > > > +               phy-supply = <&cp0_reg_usb3_0_vbus>;
> > > > +       };
> > > > +};
> > > (disclaimer: I don't have any board with a marvell SoC, so I don't
> > > understand how it works and I can't debug it)
> > > I know about the "phy-supply" property inside the PHY node itself
> > > (that would be cp0_comphy1 in this case).
> > > The connector binding does not mention a phy-supply property:
> > > Documentation/devicetree/bindings/connector/usb-connector.txt
> >
> > That's right, can Rob confirm that this is valid? If yes I can add an
> > optional "phy-supply" property there.
Icenowy (here: [0]) and Chunfeng (here: [1]) are working on this as
far as I can tell
Icenowy is parsing the "vbus-supply" regulator manually from within
the PHY driver, see [2]

the additional info from their patches doesn't fully answer my questions yet.
I CC'ed both to this mail hoping that they can provide additional
insight what the greater picture is (at least in terms of which driver
is controlling the USB VBUS signal) on Allwinner and Mediatek SoCs.


Regards
Martin


[0] https://patchwork.kernel.org/patch/10887839/
[1] https://patchwork.kernel.org/patch/10844251/
[2] https://patchwork.kernel.org/patch/10887841/
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
index 131ce4229db0..4b8df359b1cc 100644
--- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
@@ -45,16 +45,6 @@ 
 		enable-active-high;
 		gpio = <&expander0 1 GPIO_ACTIVE_HIGH>;
 	};
-
-	cp0_usb3_0_phy: cp0-usb3-0-phy {
-		compatible = "usb-nop-xceiv";
-		vcc-supply = <&cp0_reg_usb3_0_vbus>;
-	};
-
-	cp0_usb3_1_phy: cp0-usb3-1-phy {
-		compatible = "usb-nop-xceiv";
-		vcc-supply = <&cp0_reg_usb3_1_vbus>;
-	};
 };
 
 &i2c0 {
@@ -200,15 +190,27 @@ 
 	};
 };
 
+&cp0_comphy1 {
+	cp0_usbh0_con: connector {
+		compatible = "usb-a-connector";
+		phy-supply = <&cp0_reg_usb3_0_vbus>;
+	};
+};
+
 &cp0_usb3_0 {
-	usb-phy = <&cp0_usb3_0_phy>;
 	phys = <&cp0_comphy1 0>;
 	phy-names = "cp0-usb3h0-comphy";
 	status = "okay";
 };
 
+&cp0_comphy4 {
+	cp0_usbh1_con: connector {
+		compatible = "usb-a-connector";
+		phy-supply = <&cp0_reg_usb3_1_vbus>;
+	};
+};
+
 &cp0_usb3_1 {
-	usb-phy = <&cp0_usb3_1_phy>;
 	phys = <&cp0_comphy4 1>;
 	phy-names = "cp0-usb3h1-comphy";
 	status = "okay";
diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
index b3716d206fbf..58b7e859b61d 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
@@ -51,11 +51,6 @@ 
 		status = "okay";
 	};
 
-	usb3h0_phy: usb3_phy0 {
-		compatible = "usb-nop-xceiv";
-		vcc-supply = <&v_5v0_usb3_hst_vbus>;
-	};
-
 	sfp_cp0_eth0: sfp-cp0-eth0 {
 		compatible = "sff,sfp";
 		i2c-bus = <&cp0_i2c1>;
@@ -462,8 +457,14 @@ 
 	};
 };
 
+&cp1_comphy2 {
+	cp1_usbh0_con: connector {
+		compatible = "usb-a-connector";
+		phy-supply = <&v_5v0_usb3_hst_vbus>;
+	};
+};
+
 &cp1_usb3_0 {
-	usb-phy = <&usb3h0_phy>;
 	phys = <&cp1_comphy2 0>;
 	phy-names = "cp1-usb3h0-comphy";
 	status = "okay";
diff --git a/arch/arm64/boot/dts/marvell/armada-8040-db.dts b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
index 91c51188650a..f73cb3a96393 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
@@ -52,11 +52,6 @@ 
 		vcc-supply = <&cp0_reg_usb3_0_vbus>;
 	};
 
-	cp0_usb3_1_phy: cp0-usb3-1-phy {
-		compatible = "usb-nop-xceiv";
-		vcc-supply = <&cp0_reg_usb3_1_vbus>;
-	};
-
 	cp1_reg_usb3_0_vbus: cp1-usb3-0-vbus {
 		compatible = "regulator-fixed";
 		regulator-name = "cp1-usb3h0-vbus";
@@ -167,9 +162,15 @@ 
 	status = "okay";
 };
 
+&cp0_comphy4 {
+	cp0_usbh1_con: connector {
+		compatible = "usb-a-connector";
+		phy-supply = <&cp0_reg_usb3_1_vbus>;
+	};
+};
+
 /* CON10 on CP0 expansion */
 &cp0_usb3_1 {
-	usb-phy = <&cp0_usb3_1_phy>;
 	phys = <&cp0_comphy4 1>;
 	phy-names = "cp0-usb3h1-comphy";
 	status = "okay";
diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtsi b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtsi
index 0639437ab4b4..061533589356 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtsi
@@ -61,11 +61,6 @@ 
 		status = "okay";
 	};
 
-	usb3h0_phy: usb3_phy0 {
-		compatible = "usb-nop-xceiv";
-		vcc-supply = <&v_5v0_usb3_hst_vbus>;
-	};
-
 	sfp_eth0: sfp-eth0 {
 		/* CON15,16 - CPM lane 4 */
 		compatible = "sff,sfp";
@@ -357,9 +352,15 @@ 
 	};
 };
 
+&cp1_comphy2 {
+	cp1_usbh0_con: connector {
+		compatible = "usb-a-connector";
+		phy-supply = <&v_5v0_usb3_hst_vbus>;
+	};
+};
+
 &cp1_usb3_0 {
 	/* CPS Lane 2 - CON7 */
-	usb-phy = <&usb3h0_phy>;
 	phys = <&cp1_comphy2 0>;
 	phy-names = "cp1-usb3h0-comphy";
 	status = "okay";