| Message ID | 20190401165131.23370-16-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhance CP110 COMPHY support | expand |
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
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
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
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 --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";
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(-)