[02/11] dt-bindings: usb: Add NVIDIA Tegra XUSB host controller binding

  Add device-tree binding documentation for the XUSB host controller present
on Tegra194 and Tegra234 SoC. This controller supports the USB 3.1
specification.

Signed-off-by: Wayne Chang <waynec@nvidia.com>
---
 .../bindings/usb/nvidia,tegra-xhci.yaml       | 213 ++++++++++++++++++
 1 file changed, 213 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.yaml
  

On Mon, 24 Oct 2022 15:41:19 +0800, Wayne Chang wrote:
> Add device-tree binding documentation for the XUSB host controller present
> on Tegra194 and Tegra234 SoC. This controller supports the USB 3.1
> specification.
> 
> Signed-off-by: Wayne Chang <waynec@nvidia.com>
> ---
>  .../bindings/usb/nvidia,tegra-xhci.yaml       | 213 ++++++++++++++++++
>  1 file changed, 213 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.example.dts:36.27-28 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
  

On Mon, Oct 24, 2022 at 03:41:19PM +0800, Wayne Chang wrote:
> Add device-tree binding documentation for the XUSB host controller present
> on Tegra194 and Tegra234 SoC. This controller supports the USB 3.1
> specification.
> 
> Signed-off-by: Wayne Chang <waynec@nvidia.com>
> ---
>  .../bindings/usb/nvidia,tegra-xhci.yaml       | 213 ++++++++++++++++++
>  1 file changed, 213 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.yaml b/Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.yaml
> new file mode 100644
> index 000000000000..d261a419a04f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.yaml
> @@ -0,0 +1,213 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/usb/nvidia,tegra-xhci.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Device tree binding for NVIDIA Tegra XUSB host controller

Drop 'Device tree binding for '

> +
> +description:
> +  The Tegra XHCI controller supports both USB 2.0 HighSpeed/FullSpeed and
> +  USB 3.1 SuperSpeed protocols.
> +
> +maintainers:
> +  - Wayne Chang <waynec@nvidia.com>
> +

Ref to usb-xhci.yaml? Or usb-hcd.yaml.

> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - nvidia,tegra194-xusb # For Tegra194
> +          - nvidia,tegra234-xusb # For Tegra234

The comment is kind of redundant.

> +
> +  reg:
> +    minItems: 2
> +    items:
> +      - description: XUSB host controller registers
> +      - description: XUSB host PCI Config registers
> +      - description: XUSB host bar2 registers

Drop 'XUSB host '

> +
> +  reg-names:
> +    minItems: 2
> +    items:
> +      - const: hcd
> +      - const: fpci
> +      - const: bar2
> +
> +  interrupts:
> +    items:
> +      - description: Must contain the XUSB host interrupt.
> +      - description: Must contain the XUSB mbox interrupt.

Drop 'Must contain the '

> +
> +  clocks:
> +    items:
> +      - description: Clock to enable core XUSB host clock.
> +      - description: Clock to enable XUSB falcon clock.
> +      - description: Clock to enable XUSB super speed clock.
> +      - description: Clock to enable XUSB super speed dev clock.
> +      - description: Clock to enable XUSB high speed dev clock.
> +      - description: Clock to enable XUSB full speed dev clock.
> +      - description: Clock to enable XUSB UTMI PLL clock.
> +      - description: Clock to enable core XUSB dev clock.
> +      - description: Clock to enable XUSB PLLE clock.

Drop 'Clock to enable '

> +
> +  clock-names:
> +    items:
> +      - const: xusb_host
> +      - const: xusb_falcon_src
> +      - const: xusb_ss
> +      - const: xusb_ss_src
> +      - const: xusb_hs_src
> +      - const: xusb_fs_src
> +      - const: pll_u_480m
> +      - const: clk_m
> +      - const: pll_e
> +
> +  interconnects:
> +    items:
> +      - description: memory read client
> +      - description: memory write client
> +
> +  interconnect-names:
> +    items:
> +      - const: dma-mem # read
> +      - const: write
> +
> +  iommus:
> +    maxItems: 1
> +
> +  power-domains:
> +    items:
> +      - description: XUSBC(host) power-domain
> +      - description: XUSBA(superspeed) power-domain
> +
> +  power-domain-names:
> +    items:
> +      - const: xusb_host
> +      - const: xusb_ss

Drop 'xusb_'.

> +
> +  nvidia,xusb-padctl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to the XUSB pad controller that is used to configure the USB pads
> +      used by the XUDC controller.
> +
> +  phys:
> +    minItems: 1
> +    maxItems: 8
> +    description:
> +      Must contain an entry for each entry in phy-names.
> +      See ../phy/phy-bindings.txt for details.

Drop description.

> +
> +  phy-names:
> +    minItems: 1
> +    maxItems: 8
> +    items:
> +      anyOf:
> +        - const: usb2-0
> +        - const: usb2-1
> +        - const: usb2-2
> +        - const: usb2-3
> +        - const: usb3-0
> +        - const: usb3-1
> +        - const: usb3-2
> +        - const: usb3-3
> +
> +  dma-coherent:
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - power-domain-names
> +  - nvidia,xusb-padctl
> +  - phys
> +  - phy-names
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra194-xusb
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2
> +        reg-names:
> +          minItems: 2
> +        clocks:
> +          minItems: 9
> +        clock-names:
> +          minItems: 9
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra234-xusb
> +    then:
> +      properties:
> +        reg:
> +          minItems: 3
> +        reg-names:
> +          minItems: 3
> +        clocks:
> +          minItems: 9
> +        clock-names:
> +          minItems: 9

Same number of items, why are clocks in the if/then?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/tegra234-gpio.h>
> +    #include <dt-bindings/clock/tegra234-clock.h>
> +    #include <dt-bindings/memory/tegra234-mc.h>
> +    #include <dt-bindings/power/tegra234-powergate.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    usb@3610000 {
> +      compatible = "nvidia,tegra234-xusb";
> +      reg = <0x03610000 0x40000>,
> +            <0x03600000 0x10000>,
> +            <0x03650000 0x10000>;
> +      reg-names = "hcd", "fpci", "bar2";
> +
> +      interrupts = <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>,
> +             <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
> +
> +      clocks = <&bpmp TEGRA234_CLK_XUSB_CORE_HOST>,
> +         <&bpmp TEGRA234_CLK_XUSB_FALCON>,
> +         <&bpmp TEGRA234_CLK_XUSB_CORE_SS>,
> +         <&bpmp TEGRA234_CLK_XUSB_SS>,
> +         <&bpmp TEGRA234_CLK_CLK_M>,
> +         <&bpmp TEGRA234_CLK_XUSB_FS>,
> +         <&bpmp TEGRA234_CLK_UTMIP_PLL>,
> +         <&bpmp TEGRA234_CLK_CLK_M>,
> +         <&bpmp TEGRA234_CLK_PLLE>;
> +      clock-names = "xusb_host", "xusb_falcon_src",
> +              "xusb_ss", "xusb_ss_src", "xusb_hs_src",
> +              "xusb_fs_src", "pll_u_480m", "clk_m",
> +              "pll_e";
> +      interconnects = <&mc TEGRA234_MEMORY_CLIENT_XUSB_HOSTR &emc>,
> +          <&mc TEGRA234_MEMORY_CLIENT_XUSB_HOSTW &emc>;
> +      interconnect-names = "dma-mem", "write";
> +      iommus = <&smmu_niso1 TEGRA234_SID_XUSB_HOST>;
> +
> +      power-domains = <&bpmp TEGRA234_POWER_DOMAIN_XUSBC>,
> +          <&bpmp TEGRA234_POWER_DOMAIN_XUSBA>;
> +      power-domain-names = "xusb_host", "xusb_ss";
> +
> +      nvidia,xusb-padctl = <&xusb_padctl>;
> +
> +      phys =  <&pad_lanes_usb2_0>;
> +      phy-names = "usb2-0";
> +
> +    };
> -- 
> 2.25.1
> 
>
  

On 24/10/2022 14:30, Rob Herring wrote:
> On Mon, 24 Oct 2022 15:41:19 +0800, Wayne Chang wrote:
>> Add device-tree binding documentation for the XUSB host controller present
>> on Tegra194 and Tegra234 SoC. This controller supports the USB 3.1
>> specification.
>>
>> Signed-off-by: Wayne Chang <waynec@nvidia.com>
>> ---
>>   .../bindings/usb/nvidia,tegra-xhci.yaml       | 213 ++++++++++++++++++
>>   1 file changed, 213 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.yaml
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.example.dts:36.27-28 syntax error
> FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/usb/nvidia,tegra-xhci.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1492: dt_binding_check] Error 2


I believe that this is because another DT patch [0] is missing (see 
patch 0/11). Thierry has just picked this up for -next and so hopefully 
will resolve this.

Cheers
Jon

[0] 
https://lore.kernel.org/all/20221003125141.123759-1-jonathanh@nvidia.com/
  

Thanks for the review.
All the changes will be updated in the next patchset except the 
power-domain-name.

On 10/24/22 22:54, Rob Herring wrote:
> External email: Use caution opening links or attachments
> 
> 
>> +description:
>> +  The Tegra XHCI controller supports both USB 2.0 HighSpeed/FullSpeed and
>> +  USB 3.1 SuperSpeed protocols.
>> +
>> +maintainers:
>> +  - Wayne Chang <waynec@nvidia.com>
>> +
> 
> Ref to usb-xhci.yaml? Or usb-hcd.yaml.
> 
It should be usb-xhci.yaml. thanks.

>> +  power-domain-names:
>> +    items:
>> +      - const: xusb_host
>> +      - const: xusb_ss
> 
> Drop 'xusb_'.

The properties are constant and we use the name to get the power domain.

	tegra->genpd_dev_host = dev_pm_domain_attach_by_name(dev, "xusb_host");
	
	tegra->genpd_dev_ss = dev_pm_domain_attach_by_name(dev, "xusb_ss");
	
we might not be able to drop the xusb_

thanks,
Wayne.
  

On 25/10/2022 04:02, Wayne Chang wrote:
> 
>>> +  power-domain-names:
>>> +    items:
>>> +      - const: xusb_host
>>> +      - const: xusb_ss
>>
>> Drop 'xusb_'.
> 
> The properties are constant and we use the name to get the power domain.
> 
> 	tegra->genpd_dev_host = dev_pm_domain_attach_by_name(dev, "xusb_host");
> 	
> 	tegra->genpd_dev_ss = dev_pm_domain_attach_by_name(dev, "xusb_ss");
> 	
> we might not be able to drop the xusb_

These are new bindings, so why do say they are "constant"? New bindings
means you did not use them. If you used them before bindings... what can
we say? Don't?

Best regards,
Krzysztof
  

On 28/10/2022 03:19, Krzysztof Kozlowski wrote:
> On 25/10/2022 04:02, Wayne Chang wrote:
>>
>>>> +  power-domain-names:
>>>> +    items:
>>>> +      - const: xusb_host
>>>> +      - const: xusb_ss
>>>
>>> Drop 'xusb_'.
>>
>> The properties are constant and we use the name to get the power domain.
>>
>> 	tegra->genpd_dev_host = dev_pm_domain_attach_by_name(dev, "xusb_host");
>> 	
>> 	tegra->genpd_dev_ss = dev_pm_domain_attach_by_name(dev, "xusb_ss");
>> 	
>> we might not be able to drop the xusb_
> 
> These are new bindings, so why do say they are "constant"? New bindings
> means you did not use them. If you used them before bindings... what can
> we say? Don't?

Not exactly. However, what we should do here is convert the legacy 
binding doc [0] and replace with this one. But yes we are stuck with the 
'xusb_host' naming.

Jon

[0] Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt
  

On 28/10/2022 10:25, Jon Hunter wrote:
> 
> On 28/10/2022 03:19, Krzysztof Kozlowski wrote:
>> On 25/10/2022 04:02, Wayne Chang wrote:
>>>
>>>>> +  power-domain-names:
>>>>> +    items:
>>>>> +      - const: xusb_host
>>>>> +      - const: xusb_ss
>>>>
>>>> Drop 'xusb_'.
>>>
>>> The properties are constant and we use the name to get the power domain.
>>>
>>>     tegra->genpd_dev_host = dev_pm_domain_attach_by_name(dev, 
>>> "xusb_host");
>>>
>>>     tegra->genpd_dev_ss = dev_pm_domain_attach_by_name(dev, "xusb_ss");
>>>
>>> we might not be able to drop the xusb_
>>
>> These are new bindings, so why do say they are "constant"? New bindings
>> means you did not use them. If you used them before bindings... what can
>> we say? Don't?
> 
> Not exactly. However, what we should do here is convert the legacy 
> binding doc [0] and replace with this one. But yes we are stuck with the 
> 'xusb_host' naming.


Thierry already has a patch to do this [0]. So we should fix that up and 
included in this series.

Jon

[0] 
https://lore.kernel.org/linux-tegra/20211209165339.614498-3-thierry.reding@gmail.com/
  

On Fri, Oct 28, 2022 at 12:07:38PM +0100, Jon Hunter wrote:
> 
> On 28/10/2022 10:25, Jon Hunter wrote:
> > 
> > On 28/10/2022 03:19, Krzysztof Kozlowski wrote:
> > > On 25/10/2022 04:02, Wayne Chang wrote:
> > > > 
> > > > > > +  power-domain-names:
> > > > > > +    items:
> > > > > > +      - const: xusb_host
> > > > > > +      - const: xusb_ss
> > > > > 
> > > > > Drop 'xusb_'.
> > > > 
> > > > The properties are constant and we use the name to get the power domain.
> > > > 
> > > >     tegra->genpd_dev_host = dev_pm_domain_attach_by_name(dev,
> > > > "xusb_host");
> > > > 
> > > >     tegra->genpd_dev_ss = dev_pm_domain_attach_by_name(dev, "xusb_ss");
> > > > 
> > > > we might not be able to drop the xusb_
> > > 
> > > These are new bindings, so why do say they are "constant"? New bindings
> > > means you did not use them. If you used them before bindings... what can
> > > we say? Don't?
> > 
> > Not exactly. However, what we should do here is convert the legacy
> > binding doc [0] and replace with this one. But yes we are stuck with the
> > 'xusb_host' naming.
> 
> 
> Thierry already has a patch to do this [0]. So we should fix that up and
> included in this series.
> 
> Jon
> 
> [0] https://lore.kernel.org/linux-tegra/20211209165339.614498-3-thierry.reding@gmail.com/

I have a v2 with at least some of the comments addressed. I'll go
through them again and send it out. If we can get that reviewed, it can
be included in this series and the Tegra234 addition be applied on top.

Thierry
  

On 10/28/22 19:30, Thierry Reding wrote:
> On Fri, Oct 28, 2022 at 12:07:38PM +0100, Jon Hunter wrote:
>> On 28/10/2022 10:25, Jon Hunter wrote:
>>> On 28/10/2022 03:19, Krzysztof Kozlowski wrote:
>>>> On 25/10/2022 04:02, Wayne Chang wrote:
>>>>>>> +  power-domain-names:
>>>>>>> +    items:
>>>>>>> +      - const: xusb_host
>>>>>>> +      - const: xusb_ss
>>>>>> Drop 'xusb_'.
>>>>> The properties are constant and we use the name to get the power domain.
>>>>>
>>>>>      tegra->genpd_dev_host = dev_pm_domain_attach_by_name(dev,
>>>>> "xusb_host");
>>>>>
>>>>>      tegra->genpd_dev_ss = dev_pm_domain_attach_by_name(dev, "xusb_ss");
>>>>>
>>>>> we might not be able to drop the xusb_
>>>> These are new bindings, so why do say they are "constant"? New bindings
>>>> means you did not use them. If you used them before bindings... what can
>>>> we say? Don't?
>>> Not exactly. However, what we should do here is convert the legacy
>>> binding doc [0] and replace with this one. But yes we are stuck with the
>>> 'xusb_host' naming.
>>
>> Thierry already has a patch to do this [0]. So we should fix that up and
>> included in this series.
>>
>> Jon
>>
>> [0]https://lore.kernel.org/linux-tegra/20211209165339.614498-3-thierry.reding@gmail.com/
> I have a v2 with at least some of the comments addressed. I'll go
> through them again and send it out. If we can get that reviewed, it can
> be included in this series and the Tegra234 addition be applied on top.

Thanks for the help, Thierry. I'll update the binding after your change 
got updated.

> 
> Thierry

thanks,
Wayne.