| Message ID | 20220323053524.7009-2-a-govindraju@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AM62: Add support for AM62 USB wrapper driver | expand |
Hi Aswath, On 23/03/2022 07:35, 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> > --- > .../devicetree/bindings/usb/ti,am62-usb.yaml | 98 +++++++++++++++++++ > 1 file changed, 98 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..4bb139d1926d > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml We already have ti,j721e-usb.yaml which is covering am64-usb. We could just add am62 compatible there. Any am62 specific properties could be handled with conditional 'required' statements. cheers, -roger > @@ -0,0 +1,98 @@ > +# 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: Bindings for the 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 phandles 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 > + > + 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 configuring frequency of ref clock input. > + > + '#address-cells': > + const: 2 > + > + '#size-cells': > + const: 2 > + > +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>; > + > + dwc3-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>; > + }; > + };
On 23/03/2022 08:17, Roger Quadros wrote: > Hi Aswath, > > On 23/03/2022 07:35, 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> >> --- >> .../devicetree/bindings/usb/ti,am62-usb.yaml | 98 +++++++++++++++++++ >> 1 file changed, 98 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..4bb139d1926d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > > We already have ti,j721e-usb.yaml which is covering am64-usb. > We could just add am62 compatible there. Please ignore my email. Need to drink more coffee :P I totally missed that is for Cadence controller. cheers, -roger > > Any am62 specific properties could be handled with conditional > 'required' statements. > > cheers, > -roger > >> @@ -0,0 +1,98 @@ >> +# 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: Bindings for the 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 phandles 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 >> + >> + 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 configuring frequency of ref clock input. >> + >> + '#address-cells': >> + const: 2 >> + >> + '#size-cells': >> + const: 2 >> + >> +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>; >> + >> + dwc3-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>; >> + }; >> + };
On 23/03/2022 07:35, 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> > --- > .../devicetree/bindings/usb/ti,am62-usb.yaml | 98 +++++++++++++++++++ > 1 file changed, 98 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..4bb139d1926d > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > @@ -0,0 +1,98 @@ > +# 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: Bindings for the 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 phandles to usb2_refclk s/phandles/phandle > + 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 configuring frequency of ref clock input. > + > + '#address-cells': > + const: 2 > + > + '#size-cells': > + const: 2 > + > +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>; > + > + dwc3-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>; > + }; > + }; Reviewed-by: Roger Quadros <rogerq@kernel.org> cheers, -roger
On 23/03/2022 06:35, 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> > --- > .../devicetree/bindings/usb/ti,am62-usb.yaml | 98 +++++++++++++++++++ > 1 file changed, 98 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..4bb139d1926d > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > @@ -0,0 +1,98 @@ > +# 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: Bindings for the TI's AM62 wrapper module for the Synopsys USBSS-DRD controller Skip the "Bindings for the", just leave the HW description. > + > +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 phandles 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 > + > + 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 configuring frequency of ref clock input. This is a bit strange. The ref clock is the "ref" input clock, right? In such case use clk_set_rate()... Using syscon for managing clocks is not the proper way. Plus all the issues pointed out by Roger. > + > + '#address-cells': > + const: 2 > + > + '#size-cells': > + const: 2 No children allowed? I understand this is a wrapper, which explains why you do not include usb-hcd.yaml schema. But since it is a wrapper, what is it wrapping? > + > +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>; > + > + dwc3-usb@f910000 { Generic node name, so usb. > + compatible = "ti,am62-usb"; Best regards, Krzysztof
Hi Krzysztof, On 23/03/22 14:35, Krzysztof Kozlowski wrote: > On 23/03/2022 06:35, 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> >> --- >> .../devicetree/bindings/usb/ti,am62-usb.yaml | 98 +++++++++++++++++++ >> 1 file changed, 98 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..4bb139d1926d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml >> @@ -0,0 +1,98 @@ >> +# 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: Bindings for the TI's AM62 wrapper module for the Synopsys USBSS-DRD controller > > Skip the "Bindings for the", just leave the HW description. > Will drop it in the respin. >> + >> +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 phandles 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 >> + >> + 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 configuring frequency of ref clock input. > > This is a bit strange. The ref clock is the "ref" input clock, right? In > such case use clk_set_rate()... Using syscon for managing clocks is not > the proper way. > The syscon property is not being used to set the ref clock frequency but is rather being used to indicate the input clock frequency for USB PHY operation. I think the description seems be misleading. I will update it in the respin, to reflect the above description. > Plus all the issues pointed out by Roger. > >> + >> + '#address-cells': >> + const: 2 >> + >> + '#size-cells': >> + const: 2 > > No children allowed? > > I understand this is a wrapper, which explains why you do not include > usb-hcd.yaml schema. But since it is a wrapper, what is it wrapping? > Yes, there is a child node, which would be the dwc3-contoller node. Would adding the child node too in the example help capture this better? >> + >> +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>; >> + >> + dwc3-usb@f910000 { > > Generic node name, so usb. > Will correct this in the respin. Thank you for the review. >> + compatible = "ti,am62-usb"; > > > Best regards, > Krzysztof
On 23/03/2022 10:17, Aswath Govindraju wrote: > Hi Krzysztof, (...) >>> + >>> + 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 configuring frequency of ref clock input. >> >> This is a bit strange. The ref clock is the "ref" input clock, right? In >> such case use clk_set_rate()... Using syscon for managing clocks is not >> the proper way. >> > > The syscon property is not being used to set the ref clock frequency but > is rather being used to indicate the input clock frequency for USB PHY > operation. I think the description seems be misleading. I will update it > in the respin, to reflect the above description. Yes, please, it will help. > >> Plus all the issues pointed out by Roger. >> >>> + >>> + '#address-cells': >>> + const: 2 >>> + >>> + '#size-cells': >>> + const: 2 >> >> No children allowed? >> >> I understand this is a wrapper, which explains why you do not include >> usb-hcd.yaml schema. But since it is a wrapper, what is it wrapping? >> > > Yes, there is a child node, which would be the dwc3-contoller node. > Would adding the child node too in the example help capture this better? Yes, please, because then you will also spot errors when running dt_binding_check. You need patternProperties for "^usb@[0-9a-f]+$" referencing Synopsys schema. Something like: Documentation/devicetree/bindings/usb/samsung,exynos-dwc3.yaml Best regards, Krzysztof
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..4bb139d1926d --- /dev/null +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml @@ -0,0 +1,98 @@ +# 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: Bindings for the 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 phandles 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 + + 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 configuring frequency of ref clock input. + + '#address-cells': + const: 2 + + '#size-cells': + const: 2 + +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>; + + dwc3-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>; + }; + };
Add bindings for the TI's AM62 wrapper module for the Synopsys USBSS-DRD controller. Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> --- .../devicetree/bindings/usb/ti,am62-usb.yaml | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/ti,am62-usb.yaml