linux-kernel - RE: [PATCH 2/4] devicetree/bindings: add support for CP110 UTMI driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <DM5PR18MB1452A089E28D20BC68495639CAB79@DM5PR18MB1452.namprd18.prod.outlook.com>
Date:   Sun, 31 Jan 2021 15:05:11 +0000
From:   Kostya Porotchkin <kostap@...vell.com>
To:     Lubomir Rintel <lkundrak@...sk>
CC:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "mw@...ihalf.com" <mw@...ihalf.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "jaz@...ihalf.com" <jaz@...ihalf.com>,
        "gregory.clement@...tlin.com" <gregory.clement@...tlin.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        Nadav Haklai <nadavh@...vell.com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "miquel.raynal@...tlin.com" <miquel.raynal@...tlin.com>,
        Stefan Chulski <stefanc@...vell.com>,
        "kishon@...com" <kishon@...com>, Ben Peled <bpeled@...vell.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "sebastian.hesselbarth@...il.com" <sebastian.hesselbarth@...il.com>
Subject: RE: [PATCH 2/4] devicetree/bindings: add support for CP110 UTMI
 driver

Hi, Lubomir,

Thank you for your review!

> On Wed, Jan 27, 2021 at 01:27:17PM +0200, kostap@...vell.com wrote:
> > From: Konstantin Porotchkin <kostap@...vell.com>
> >
> > Add DTS binding for Marvell CP110 UTMI driver
> >
> > Signed-off-by: Konstantin Porotchkin <kostap@...vell.com>
> 
> Any chance you could convert the document to YAML so that it could be used
> for automatic validation?
> 
[KP] I believe it is possible, but probably should be done by a separate patch.
Here I am extending the existing documentation.

> > ---
> >  Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt | 69
> > ++++++++++++++++++--
> >  1 file changed, 63 insertions(+), 6 deletions(-)

...
> >  Required Properties:
> >
> >  - compatible: Should be one of:
> >  	      * "marvell,a3700-utmi-host-phy" for the PHY connected to
> > -	        the USB2 host-only controller.
> > +	        the USB2 host-only controller (for Armada3700 only).
> >  	      * "marvell,a3700-utmi-otg-phy" for the PHY connected to
> > -	        the USB3 and USB2 OTG capable controller.
> > +	        the USB3 and USB2 OTG capable controller (for Armada3700 only.
> > +	      * "marvell,cp110-utmi-phy" (for Armada 7k/8k or CN913x only).
> >  - 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.
> > +			controller and the PHY (for Armada3700 only).
> > +- marvell,system-controller: should contain a phandle to the system
> > +			     controller node (for Armada 7k/8k or CN913x only)
> 
> I guess this is okay, but referring to a syscon is done in a multitude ways across
> various other bindings; with the most popular being just:
> 
>   syscon = <&syscon>;
> 
> Perhaps consider doing that?
[KP] I was not sure that I can use a generic name inside the PHY entry 
if it is not defined as part of the generic PHY properties. 
I just did not see a good example of such in PHY bindings documentation.
If it is legal, I can change this entry name to just "syscon".
> 
> > +- #phy-cells: Standard property (Documentation: phy-bindings.txt.
> > +		Should be 0 (for Armada3700 only).
> > +
> > +
> > +Required properties (child nodes, for Armada 7k/8k/CN913x only):
> > +
> > +- reg: UTMI PHY port ID (0 or 1).
> > +- #phy-cells : Should be 0.
> > +
> > +
> > +Optional Properties (child nodes, for Armada 7k/8k/CN913x only)::
> >
> > +- marvell,cp110-utmi-device-mode: request the driver to connect the UTMI
> PHY
> > +				  port to USB device controller.
> 
> Do you need a separate property for this? Could the driver look at "dr_mode"
> property of the USB controller node to see if it's supposed to be in
> device/peripheral mode?
[KP] Yes, it seems I missed this option. I will try to change the code to support it in version 2.

> 
> >  Example:
> >
> > +Armada3700
> >  	usb2_utmi_host_phy: phy@...00 {
> >  		compatible = "marvell,armada-3700-utmi-host-phy";
> >  		reg = <0x5f000 0x800>;
> > @@ -36,3 +67,29 @@ Example:
> > --
> > 2.17.1
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__lists.infradead.org_mailman_listinfo_linux-2Darm-
> 2Dkernel&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o&m=_ZOAKZShBT3Qj
> uT3RZIld2HoLnlvv6gkbHW9gSvEfI4&s=ggCBpvhDLJ8M6-
> Q41qbt8GRxryUo_mHxLMkUl8Ao5mA&e=