diff mbox series

[v3,06/10] dt-bindings: phy: mvebu-utmi: add UTMI PHY bindings

Message ID 20190121112336.23489-7-miquel.raynal@bootlin.com (mailing list archive)
State Superseded
Headers show
Series A3700 USB S2RAM support | expand

Commit Message

Miquel Raynal Jan. 21, 2019, 11:23 a.m. UTC 
Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt

Comments

Rob Herring (Arm) Jan. 21, 2019, 3:44 p.m. UTC | #1 
On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:
> Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Gregory CLEMENT Jan. 21, 2019, 3:47 p.m. UTC | #2 
Hi Rob,
 
 On lun., janv. 21 2019, Rob Herring <robh@kernel.org> wrote:

> On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:
>> Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.
>> 
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
>>

I found "marvell,usb2-host-miscellaneous" too generic, I would prefer
that we call it "marvell,armada-3700-usb2-host-miscellaneous" as we do
for all the other IP.

Gregory

>
> Reviewed-by: Rob Herring <robh@kernel.org>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Rob Herring (Arm) Jan. 22, 2019, 4:08 p.m. UTC | #3 
On Mon, Jan 21, 2019 at 9:49 AM Gregory CLEMENT
<gregory.clement@bootlin.com> wrote:
>
> Hi Rob,
>
>  On lun., janv. 21 2019, Rob Herring <robh@kernel.org> wrote:
>
> > On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:
> >> Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.
> >>
> >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >> ---
> >>  .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
> >>  1 file changed, 38 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
> >>
>
> I found "marvell,usb2-host-miscellaneous" too generic, I would prefer
> that we call it "marvell,armada-3700-usb2-host-miscellaneous" as we do
> for all the other IP.

Ah yes, you are right. Perhaps just '-misc' to shorten it a little bit.

Rob
Miquel Raynal Jan. 22, 2019, 4:13 p.m. UTC | #4 
Hello,

Rob Herring <robh@kernel.org> wrote on Tue, 22 Jan 2019 10:08:41 -0600:

> On Mon, Jan 21, 2019 at 9:49 AM Gregory CLEMENT
> <gregory.clement@bootlin.com> wrote:
> >
> > Hi Rob,
> >
> >  On lun., janv. 21 2019, Rob Herring <robh@kernel.org> wrote:
> >  
> > > On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:  
> > >> Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.
> > >>
> > >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > >> ---
> > >>  .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
> > >>  1 file changed, 38 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
> > >>  
> >
> > I found "marvell,usb2-host-miscellaneous" too generic, I would prefer
> > that we call it "marvell,armada-3700-usb2-host-miscellaneous" as we do
> > for all the other IP.  
> 
> Ah yes, you are right. Perhaps just '-misc' to shorten it a little bit.
> 
> Rob

Sure, no problem.

I will wait for other comments on the rest of the series and then I
will update the bindings/DT patch. Perhaps if I don't get more
feedback I will just resend the concerned patches (6/10 and 9/10).


Thanks,
Miquèl
Thomas Petazzoni Jan. 22, 2019, 5:38 p.m. UTC | #5 
Hello,

On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:

> +- compatible: Should be one of:
> +	      * "marvell,a3700-utmi-host-phy" for the PHY connected to
> +	        the USB2 host-only controller.
> +	      * "marvell,a3700-utmi-otg-phy" for the PHY connected to
> +	        the USB3 and USB2 OTG capable controller.

Do we really need different compatible strings for those ? I assume the
IP block is exactly the same for the two PHYs, right ? If so, we should
use the same compatible string.

Didn't we discuss that we can lookup from the PHY whether the USB
controller it is connected to is in host mode or otg mode ?

> +Example:
> +
> +	usb2_utmi_host_phy: phy@5f000 {
> +		compatible = "marvell,armada-3700-utmi-host-phy";
> +		reg = <0x5f000 0x800>;
> +		marvell,usb-misc-reg = <&usb2_syscon>;
> +		#phy-cells = <0>;
> +	};
> +
> +	usb2_syscon: system-controller@5f800 {
> +		compatible = "marvell,usb2-host-miscellaneous", "syscon";
> +		reg = <0x5f800 0x800>;
> +	};

Those registers are contiguous to the register range of the PHY itself.
What was the criteria used to decide that we need two separate DT nodes
for these ?

Best regards,

Thomas
Miquel Raynal Jan. 22, 2019, 7:16 p.m. UTC | #6 
Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Tue, 22 Jan
2019 18:38:33 +0100:

> Hello,
> 
> On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:
> 
> > +- compatible: Should be one of:
> > +	      * "marvell,a3700-utmi-host-phy" for the PHY connected to
> > +	        the USB2 host-only controller.
> > +	      * "marvell,a3700-utmi-otg-phy" for the PHY connected to
> > +	        the USB3 and USB2 OTG capable controller.  
> 
> Do we really need different compatible strings for those ? I assume the
> IP block is exactly the same for the two PHYs, right ? If so, we should
> use the same compatible string.

For what I understand, the PHYs are different. At least this is how
they are described in the specification. I can list at least two
differences visible in the register set:
* one has OTG registers, the other does not.
* one has charger detection capabilities (and registers), the other
  does not.

> 
> Didn't we discuss that we can lookup from the PHY whether the USB
> controller it is connected to is in host mode or otg mode ?

Yes, but having two different PHYs meant having two different
compatibles, hence there was no need in the driver to use this
lookup approach. Anyway this is an implementation detail and it is
always possible to go back. 

> 
> > +Example:
> > +
> > +	usb2_utmi_host_phy: phy@5f000 {
> > +		compatible = "marvell,armada-3700-utmi-host-phy";
> > +		reg = <0x5f000 0x800>;
> > +		marvell,usb-misc-reg = <&usb2_syscon>;
> > +		#phy-cells = <0>;
> > +	};
> > +
> > +	usb2_syscon: system-controller@5f800 {
> > +		compatible = "marvell,usb2-host-miscellaneous", "syscon";
> > +		reg = <0x5f800 0x800>;
> > +	};  
> 
> Those registers are contiguous to the register range of the PHY itself.
> What was the criteria used to decide that we need two separate DT nodes
> for these ?

Because this area contains a bunch of registers, most of them are
there to manage the PHY, but others are related to the USB controller
(ie. a software reset). I know the current USB controller driver does
not access this area but what if one day we decide to do it?


Thanks,
Miquèl
Thomas Petazzoni Jan. 23, 2019, 1:51 p.m. UTC | #7 
Hello Miquel,

On Tue, 22 Jan 2019 20:16:35 +0100, Miquel Raynal wrote:

> > Do we really need different compatible strings for those ? I assume the
> > IP block is exactly the same for the two PHYs, right ? If so, we should
> > use the same compatible string.  
> 
> For what I understand, the PHYs are different. At least this is how
> they are described in the specification. I can list at least two
> differences visible in the register set:
> * one has OTG registers, the other does not.
> * one has charger detection capabilities (and registers), the other
>   does not.

OK, then indeed it makes sense to have two different compatible strings.

> > Those registers are contiguous to the register range of the PHY itself.
> > What was the criteria used to decide that we need two separate DT nodes
> > for these ?  
> 
> Because this area contains a bunch of registers, most of them are
> there to manage the PHY, but others are related to the USB controller
> (ie. a software reset). I know the current USB controller driver does
> not access this area but what if one day we decide to do it?

OK, if indeed this register area contains register used both for the
PHY and for something else, your DT representation makes sense.

Thanks for those clarifications!

Thomas
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt b/Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
new file mode 100644
index 000000000000..7ffa34ac93e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
@@ -0,0 +1,38 @@ 
+MVEBU A3700 UTMI PHY
+--------------------
+
+USB2 UTMI+ PHY controllers can be found on the following Marvell MVEBU SoCs:
+* Armada 3700
+
+On Armada 3700, there are two USB controllers, one is compatible with the USB2
+and USB3 specifications and supports OTG. The other one is USB2 compliant and
+only supports host mode. Both of these controllers come with a slightly
+different UTMI PHY.
+
+Required Properties:
+
+- compatible: Should be one of:
+	      * "marvell,a3700-utmi-host-phy" for the PHY connected to
+	        the USB2 host-only controller.
+	      * "marvell,a3700-utmi-otg-phy" for the PHY connected to
+	        the USB3 and USB2 OTG capable controller.
+- reg: PHY IP register range.
+- marvell,usb-misc-reg: handle on the "USB miscellaneous registers" shared
+			region covering registers related to both the host
+			controller and the PHY.
+- #phy-cells: Standard property (Documentation: phy-bindings.txt) Should be 0.
+
+
+Example:
+
+	usb2_utmi_host_phy: phy@5f000 {
+		compatible = "marvell,armada-3700-utmi-host-phy";
+		reg = <0x5f000 0x800>;
+		marvell,usb-misc-reg = <&usb2_syscon>;
+		#phy-cells = <0>;
+	};
+
+	usb2_syscon: system-controller@5f800 {
+		compatible = "marvell,usb2-host-miscellaneous", "syscon";
+		reg = <0x5f800 0x800>;
+	};