Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 31 Mar 2022 19:20:39 -0500
From:   Rob Herring <robh@kernel.org>
To:     Krzysztof Kozlowski <krzk@kernel.org>
Cc:     Aswath Govindraju <a-govindraju@ti.com>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-usb@vger.kernel.org, Felipe Balbi <balbi@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Roger Quadros <rogerq@kernel.org>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Kishon Vijay Abraham I <kishon@ti.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: usb: Add documentation for AM62 USB
 Wrapper module
Message-ID: <YkZFV4h/vvmKg6iw@robh.at.kernel.org>
References: <20220324073425.18607-1-a-govindraju@ti.com>
 <20220324073425.18607-2-a-govindraju@ti.com>
 <93fe6a41-3b59-2fbc-6f95-833f337815ee@kernel.org>
 <41f79aa5-1e04-53f8-ab21-85fe6039e24e@ti.com>
 <2b33798e-23c2-d4a5-171a-55c28bc40c40@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2b33798e-23c2-d4a5-171a-55c28bc40c40@kernel.org>
Precedence: bulk

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@ti.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@ti.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