linux-kernel - Re: [PATCH v2 1/2] dt-bindings: usb: Add documentation for AM62 USB Wrapper module

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Mar 2022 19:20:39 -0500
From:   Rob Herring <robh@...nel.org>
To:     Krzysztof Kozlowski <krzk@...nel.org>
Cc:     Aswath Govindraju <a-govindraju@...com>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-usb@...r.kernel.org, Felipe Balbi <balbi@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Roger Quadros <rogerq@...nel.org>,
        Vignesh Raghavendra <vigneshr@...com>,
        Kishon Vijay Abraham I <kishon@...com>
Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add documentation for AM62 USB
 Wrapper module

On Thu, Mar 24, 2022 at 12:53:08PM +0100, Krzysztof Kozlowski wrote:
> On 24/03/2022 12:40, Aswath Govindraju wrote:
> > Hi Krzysztof,
> > 
> > On 24/03/22 16:37, Krzysztof Kozlowski wrote:
> >> On 24/03/2022 08:34, Aswath Govindraju wrote:
> >>> Add bindings for the TI's AM62 wrapper module for the Synopsys USBSS-DRD
> >>> controller.
> >>>
> >>> Signed-off-by: Aswath Govindraju <a-govindraju@...com>
> >>> ---
> >>>
> >>> Changes since v1:
> >>> - made correction in grammer of clocks property description
> >>>   and added maxItems in the interrupts property based on comments
> >>>   received from Roger
> >>> - corrected the title, fixed the description of
> >>>   ti,syscon-phy-pll-refclk, added pattern properties and child node
> >>>   in the example based on the comments from Krzysztof.
> >>>
> >>>  .../devicetree/bindings/usb/ti,am62-usb.yaml  | 117 ++++++++++++++++++
> >>>  1 file changed, 117 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/usb/ti,am62-usb.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml
> >>> new file mode 100644
> >>> index 000000000000..452bfdc6fb09
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml
> >>> @@ -0,0 +1,117 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/usb/ti,am62-usb.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: TI's AM62 wrapper module for the Synopsys USBSS-DRD controller
> >>> +
> >>> +maintainers:
> >>> +  - Aswath Govindraju <a-govindraju@...com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: ti,am62-usb
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  ranges: true
> >>> +
> >>> +  power-domains:
> >>> +    description:
> >>> +      PM domain provider node and an args specifier containing
> >>> +      the USB ISO device id value. See,
> >>> +      Documentation/devicetree/bindings/soc/ti/sci-pm-domain.yaml
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    description: Clock phandle to usb2_refclk
> >>> +    maxItems: 1
> >>> +
> >>> +  clock-names:
> >>> +    items:
> >>> +      - const: ref
> >>> +
> >>> +  id-gpio:
> >>> +    description:
> >>> +      GPIO to be used as ID pin
> >>> +    maxItems: 1
> >>
> >> I have doubts about this. If you USB controller handles the ID pin, then
> >> probably this should be moved to usb-connector.yaml. I did not see
> >> id-gpio in any other USB controller blocks.
> >>
> > 
> > Yes, the USB wrapper handles the ID pin operation only. It also reads
> > the status of VBUS by reading a register from its MMR and not using a
> > gpio. After evaluating the role the based on the states if id pin and
> > VBUS, this role is communicated to the dwc3 core driver using extcon.
> > There is no way for the dwc3 driver to detect the role on its own.
> > 
> > 
> > The usb-connector(drivers/usb/common/usb-conn-gpio.c) driver, seems to
> > be implemented for driving the VBUS, based on ID and VBUS pin status.
> > However, in case of the above implementation we need to communicate the
> > detected role to the dwc3 core driver. Also, the wrapper does not
> > control VBUS but it is the dwc3 core driver that drives the VBUS.
> > Therefore, I think the usb-connector implementation cannot be used here.
> 
> I don't think about usb-conn-gpio.c but using the binding generic
> binding for usb-X-connector and define a connector with ID.
> 
> Actually Rob could help here.
> 
> Rob,
> Should the id-gpio be modeled as a property in this glue/wrapper driver
> or rather as part of usb-connector child node?

That's a simple question. Where does the ID GPIO signal go to? The 
connector, so it goes in the connector node.

If we have a driver for the usb-connector node, that's news to me. Not 
that we couldn't, but that has nothing to do with designing the binding.

Rob