diff mbox series

[v2,1/2] dt-bindings: usb: Add documentation for AM62 USB Wrapper module

Message ID 20220324073425.18607-2-a-govindraju@ti.com (mailing list archive)
State Superseded
Headers show
Series AM62: Add support for AM62 USB wrapper driver | expand

Commit Message

Aswath Govindraju March 24, 2022, 7:34 a.m. UTC
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

Comments

Krzysztof Kozlowski March 24, 2022, 11:07 a.m. UTC | #1
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.


Best regards,
Krzysztof
Aswath Govindraju March 24, 2022, 11:40 a.m. UTC | #2
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.

Thanks,
Aswath

> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski March 24, 2022, 11:53 a.m. UTC | #3
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?


Best regards,
Krzysztof
Rob Herring (Arm) April 1, 2022, 12:20 a.m. UTC | #4
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
Aswath Govindraju April 1, 2022, 5:04 a.m. UTC | #5
Hi Rob,

On 01/04/22 05:50, Rob Herring wrote:
> 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.
> 

Thank you for the clarification. Here ID-gpio is directly read by the
wrapper and hence, I have modeled it as a property in the wrapper dt
node. May I know if this wrong and should the modelling be looked at
differently?

Thanks,
Aswath

> 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
Krzysztof Kozlowski April 1, 2022, 7:49 a.m. UTC | #6
On 01/04/2022 07:04, Aswath Govindraju wrote:
> Hi Rob,
> 
> On 01/04/22 05:50, Rob Herring wrote:
>> 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.
>>
> 
> Thank you for the clarification. Here ID-gpio is directly read by the
> wrapper and hence, I have modeled it as a property in the wrapper dt
> node. May I know if this wrong and should the modelling be looked at
> differently?

What do you mean here by "read"? If you refer to the driver reading the
pin value, it is less related to the hardware, right?


Best regards,
Krzysztof
Aswath Govindraju April 1, 2022, 8:05 a.m. UTC | #7
Hi Krzysztof, Rob,

On 01/04/22 13:19, Krzysztof Kozlowski wrote:
> On 01/04/2022 07:04, Aswath Govindraju wrote:
>> Hi Rob,
>>
>> On 01/04/22 05:50, Rob Herring wrote:
>>> 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.
>>>
>>
>> Thank you for the clarification. Here ID-gpio is directly read by the
>> wrapper and hence, I have modeled it as a property in the wrapper dt
>> node. May I know if this wrong and should the modelling be looked at
>> differently?
> 
> What do you mean here by "read"? If you refer to the driver reading the
> pin value, it is less related to the hardware, right?
> 
> 

Thank you for clarification. Now I understood, I was looking at it in
the wrong way and when looking at in in terms hardware I understood the
mistake that I have been doing. So, the id-gpio is actually not an
inherent part of the wrapper but rather something that it gets from the
connector. So, there needs to be different node describing this.

Thanks,
Aswath

> Best regards,
> Krzysztof
diff mbox series

Patch

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
+
+  interrupts:
+    description:
+      interrupt line to be used for detecting changes in VBUS
+    maxItems: 1
+
+  ti,vbus-divider:
+    description:
+      Should be present if USB VBUS line is connected to the
+      VBUS pin of the SoC via a 1/3 voltage divider.
+    type: boolean
+
+  ti,syscon-phy-pll-refclk:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: Phandle to the SYSCON entry
+          - description: USB phy control register offset within SYSCON
+    description:
+      Specifier for conveying frequency of ref clock input, for the
+      operation of USB2PHY.
+
+  '#address-cells':
+    const: 2
+
+  '#size-cells':
+    const: 2
+
+patternProperties:
+  "^usb@[0-9a-f]+$":
+    $ref: snps,dwc3.yaml#
+    description: Required child node
+
+required:
+  - compatible
+  - reg
+  - power-domains
+  - clocks
+  - clock-names
+  - interrupts
+  - ti,syscon-phy-pll-refclk
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      usbss1: usb@f910000 {
+        compatible = "ti,am62-usb";
+        reg = <0x00 0x0f910000 0x00 0x800>;
+        interrupts = <GIC_SPI 234 IRQ_TYPE_LEVEL_HIGH>; /* MISC IRQ */
+        clocks = <&k3_clks 162 3>;
+        clock-names = "ref";
+        ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>;
+        power-domains = <&k3_pds 179 TI_SCI_PD_EXCLUSIVE>;
+        id-gpio = <&main_gpio1 51 GPIO_ACTIVE_LOW>;
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        usb@31100000 {
+          compatible = "snps,dwc3";
+          reg =<0x00 0x31100000 0x00 0x50000>;
+          interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>, /* irq.0 */
+                       <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>; /* irq.0 */
+          interrupt-names = "host", "peripheral";
+          maximum-speed = "high-speed";
+          extcon = <&usbss1>;
+          dr_mode = "otg";
+        };
+      };
+    };