by Peng Fan

[permalink] [raw]

Subject: RE: [PATCH V3 4/7] dt-bindings: usb: ci-hdrc-usb2: convert to DT schema format

> Subject: Re: [PATCH V3 4/7] dt-bindings: usb: ci-hdrc-usb2: convert to DT
> schema format
>
> On Sun, Feb 26, 2023 at 09:05:36PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > Convert the binding to DT schema format. To fix the dtbs_check error,
> > some properties were also added, such as nvidia,phy, reset-names ulpi.
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 159 -------
> > .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 408 ++++++++++++++++++
> > 2 files changed, 408 insertions(+), 159 deletions(-) delete mode
> > 100644 Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > create mode 100644
> > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
>
> [...]
>
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> > new file mode 100644
> > index 000000000000..b30ed11fa22f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> > @@ -0,0 +1,408 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> >
> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fschemas%2Fusb%2Fci-hdrc-
> usb2.yaml%23&data=05%7C01%7Cpeng
> >
> +.fan%40nxp.com%7Cf46be21afe3e40dce8da08db1907f3ab%7C686ea1d3bc
> 2b4c6fa
> >
> +92cd99c5c301635%7C0%7C0%7C638131294167215776%7CUnknown%7CT
> WFpbGZsb3d8
> >
> +eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> 3D%7C
> >
> +3000%7C%7C%7C&sdata=cKoiZ7ZFPo%2FmIrudifs8NyvMkUhzoHLTWYwTD
> PDuR0I%3D&
> > +reserved=0
> > +$schema:
> >
> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C01%7Cpeng.fan%40nx
> >
> +p.com%7Cf46be21afe3e40dce8da08db1907f3ab%7C686ea1d3bc2b4c6fa92
> cd99c5c
> >
> +301635%7C0%7C0%7C638131294167215776%7CUnknown%7CTWFpbGZsb
> 3d8eyJWIjoiM
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> 0%7C%7
> >
> +C%7C&sdata=Zpqwgma1XzN0Q5ZxRZCF%2BIzvSL6Qjkgrk581nrzazLQ%3D&r
> eserved=
> > +0
> > +
> > +title: USB2 ChipIdea USB controller for ci13xxx Binding
>
> Drop ' for ci13xxx Binding'
>
> > +
> > +maintainers:
> > + - Xu Yang <[email protected]>
> > + - Peng Fan <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - enum:
> > + - chipidea,usb2
> > + - fsl,imx7ulp-usb
> > + - lsi,zevio-usb
> > + - nvidia,tegra20-udc
> > + - nvidia,tegra30-udc
> > + - nvidia,tegra114-udc
> > + - nvidia,tegra124-udc
> > + - qcom,ci-hdrc
> > + - items:
> > + - enum:
> > + - fsl,imx23-usb
> > + - fsl,imx25-usb
> > + - fsl,imx28-usb
> > + - fsl,imx6q-usb
> > + - fsl,imx6sl-usb
> > + - fsl,imx6sx-usb
> > + - fsl,imx6ul-usb
> > + - fsl,imx7d-usb
> > + - const: fsl,imx27-usb
> > + - items:
> > + - const: fsl,imx8mm-usb
> > + - const: fsl,imx7d-usb
>
> But not compatible with imx27?

If I add imx27, there will lots dtbs_check failure.

I am not sure what are the rule here.
Saying A is compatible with B, B is compatible with C, C
is compatible with D, D is compatible with E. should we add?
-const: A
-const: B
-const: C
-const: D
-const: E

Or we could split them such as two?
-const A
-const B
-const C
-----------------
-const C
-const D
-const E

Thanks,
Peng.

>
> > + - items:
> > + - const: fsl,imx7ulp-usb
> > + - const: fsl,imx6ul-usb
> > + - items:
> > + - const: xlnx,zynq-usb-2.20a
> > + - const: chipidea,usb2
> > +
>
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 0
>
> Defined in usb-hcd.yaml, so drop.
>
> > +
> > + reg:
> > + minItems: 1
> > + maxItems: 2
> > +
> > + interrupts:
> > + minItems: 1
> > + maxItems: 2
> > +
> > + clocks:
> > + minItems: 1
> > + maxItems: 2
> > +
> > + clock-names:
> > + minItems: 1
> > + maxItems: 2
> > +
> > + dr_mode: true
> > +
> > + power-domains:
> > + maxItems: 1
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + reset-names:
> > + maxItems: 1
> > +
> > + "#reset-cells":
> > + const: 1
> > +
> > + phy_type: true
> > +
> > + itc-setting:
> > + description:
> > + interrupt threshold control register control, the setting should be
> > + aligned with ITC bits at register USBCMD.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > + ahb-burst-config:
> > + description:
> > + it is vendor dependent, the required value should be aligned with
> > + AHBBRST at SBUSCFG, the range is from 0x0 to 0x7. This property is
> > + used to change AHB burst configuration, check the chipidea spec for
> > + meaning of each value. If this property is not existed, it will use
> > + the reset value.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x0
> > + maximum: 0x7
> > +
> > + tx-burst-size-dword:
> > + description:
> > + it is vendor dependent, the tx burst size in dword (4 bytes), This
> > + register represents the maximum length of a the burst in 32-bit
> > + words while moving data from system memory to the USB bus, the
> value
> > + of this property will only take effect if property "ahb-burst-config"
> > + is set to 0, if this property is missing the reset default of the
> > + hardware implementation will be used.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x0
> > + maximum: 0x20
> > +
> > + rx-burst-size-dword:
> > + description:
> > + it is vendor dependent, the rx burst size in dword (4 bytes), This
> > + register represents the maximum length of a the burst in 32-bit words
> > + while moving data from the USB bus to system memory, the value of
> > + this property will only take effect if property "ahb-burst-config"
> > + is set to 0, if this property is missing the reset default of the
> > + hardware implementation will be used.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x0
> > + maximum: 0x20
> > +
> > + extcon:
> > + description: |
> > + Phandles to external connector devices. First phandle should point
> > + to external connector, which provide "USB" cable events, the second
> > + should point to external connector device, which provide "USB-HOST"
> > + cable events. If one of the external connector devices is not
> > + required, empty <0> phandle should be specified.
> > +
> > + phy-clkgate-delay-us:
> > + description: |
> > + The delay time (us) between putting the PHY into low power mode
> and
> > + gating the PHY clock.
> > +
> > + non-zero-ttctrl-ttha:
> > + description: |
> > + After setting this property, the value of register ttctrl.ttha
> > + will be 0x7f; if not, the value will be 0x0, this is the default
> > + value. It needs to be very carefully for setting this property, it
> > + is recommended that consult with your IC engineer before setting
> > + this value. On the most of chipidea platforms, the "usage_tt" flag
> > + at RTL is 0, so this property only affects siTD.
> > +
> > + If this property is not set, the max packet size is 1023 bytes, and
> > + if the total of packet size for pervious transactions are more than
> > + 256 bytes, it can't accept any transactions within this frame. The
> > + use case is single transaction, but higher frame rate.
> > +
> > + If this property is set, the max packet size is 188 bytes, it can
> > + handle more transactions than above case, it can accept transactions
> > + until it considers the left room size within frame is less than 188
> > + bytes, software needs to make sure it does not send more than 90%
> > + maximum_periodic_data_per_frame. The use case is multiple
> > + transactions, but less frame rate.
> > +
> > + mux-controls:
> > + description: |
> > + The mux control for toggling host/device output of this controller.
> > + It's expected that a mux state of 0 indicates device mode and a mux
> > + state of 1 indicates host mode.
> > + maxItems: 1
> > +
> > + mux-control-names:
> > + const: usb_switch
> > +
> > + pinctrl-names:
> > + description: |
> > + Names for optional pin modes in "default", "host", "device".
> > + In case of HSIC-mode, "idle" and "active" pin modes are mandatory.
> > + In this case, the "idle" state needs to pull down the data and
> > + strobe pin and the "active" state needs to pull up the strobe pin.
>
> items:
> enum: [ default, host, device, idle, active ]
>
> > +
> > + pinctrl-0:
> > + maxItems: 1
> > +
> > + pinctrl-1:
> > + maxItems: 1
> > +
> > + phys:
> > + maxItems: 1
> > +
> > + phy-names:
> > + const: usb-phy
> > +
> > + vbus-supply:
> > + description: reference to the VBUS regulator.
> > +
> > + fsl,usbmisc:
> > + description:
> > + Phandler of non-core register device, with one argument that
> > + indicate usb controller index
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + items:
> > + - items:
> > + - description: phandle to usbmisc node
> > + - description: index of usb controller
> > +
> > + fsl,anatop:
> > + description: phandle for the anatop node.
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > +
> > + disable-over-current:
> > + type: boolean
> > + description: disable over current detect
> > +
> > + over-current-active-low:
> > + type: boolean
> > + description: over current signal polarity is active low
> > +
> > + over-current-active-high:
> > + type: boolean
> > + description: |
> > + Over current signal polarity is active high. It's recommended to
> > + specify the over current polarity.
> > +
> > + power-active-high:
> > + type: boolean
> > + description: power signal polarity is active high
> > +
> > + external-vbus-divider:
> > + type: boolean
> > + description: enables off-chip resistor divider for Vbus
> > +
> > + samsung,picophy-pre-emp-curr-control:
> > + description: |
> > + HS Transmitter Pre-Emphasis Current Control. This signal controls
> > + the amount of current sourced to the USB_OTG*_DP and
> USB_OTG*_DN
> > + pins after a J-to-K or K-to-J transition. The range is from 0x0 to
> > + 0x3, the default value is 0x1. Details can refer to
> TXPREEMPAMPTUNE0
> > + bits of USBNC_n_PHY_CFG1.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x0
> > + maximum: 0x3
> > +
> > + samsung,picophy-dc-vol-level-adjust:
> > + description: |
> > + HS DC Voltage Level Adjustment. Adjust the high-speed transmitter
> DC
> > + level voltage. The range is from 0x0 to 0xf, the default value is
> > + 0x3. Details can refer to TXVREFTUNE0 bits of USBNC_n_PHY_CFG1.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x0
> > + maximum: 0xf
> > +
> > + usb-phy:
> > + description: phandle for the PHY device. Use "phys" instead.
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + deprecated: true
> > +
> > + fsl,usbphy:
> > + description: phandle of usb phy that connects to the port. Use "phys"
> instead.
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + deprecated: true
> > +
> > + nvidia,phy:
> > + description: phandle of usb phy that connects to the port. Use "phys"
> instead.
> > + $ref: /schemas/types.yaml#/definitions/phandle
>
> deprecated: true
>
> > +
> > + port:
> > + description:
> > + Any connector to the data bus of this controller should be modelled
> > + using the OF graph bindings specified, if the "usb-role-switch"
> > + property is used.
> > + $ref: /schemas/graph.yaml#/properties/port
> > +
> > + reset-gpios:
> > + maxItems: 1
> > +
> > + ulpi:
> > + type: object
> > + properties:
> > + phy:
> > + description: The phy child node for Qcom chips.
> > + type: object
> > + $ref: /schemas/phy/qcom,usb-hs-phy.yaml
> > +
>
> > +patternProperties:
> > + "^.*@[0-9a-f]{1,2}$":
> > + description: The hard wired USB devices
> > + type: object
> > + $ref: /schemas/usb/usb-device.yaml
>
> Defined in usb-hcd.yaml, so drop.
>
> > +
> > +dependencies:
> > + port: [ usb-role-switch ]
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > +
> > +allOf:
> > + - $ref: usb-hcd.yaml#
> > + - $ref: usb-drd.yaml#
> > + - if:
> > + properties:
> > + mux-controls:
> > + true
> > + then:
> > + properties:
> > + mux-control-names:
> > + const: usb_switch
> > + - if:
> > + properties:
> > + phy_type:
> > + const: hsic
> > + required:
> > + - phy_type
> > + then:
> > + properties:
> > + pinctrl-names:
> > + items:
> > + - const: idle
> > + - const: active
> > + else:
> > + properties:
> > + pinctrl-names:
> > + minItems: 1
> > + maxItems: 2
> > + oneOf:
> > + - items:
> > + - const: default
> > + - enum:
> > + - host
> > + - device
> > + - items:
> > + - const: default
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - chipidea,usb2
> > + - lsi,zevio-usb
> > + - nvidia,tegra20-udc
> > + - nvidia,tegra30-udc
> > + - nvidia,tegra114-udc
> > + - nvidia,tegra124-udc
> > + - qcom,ci-hdrc
> > + - xlnx,zynq-usb-2.20a
> > + then:
> > + properties:
> > + fsl,usbmisc: false
> > + disable-over-current: false
> > + over-current-active-low: false
> > + over-current-active-high: false
> > + power-active-high: false
> > + external-vbus-divider: false
> > + samsung,picophy-pre-emp-curr-control: false
> > + samsung,picophy-dc-vol-level-adjust: false
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/clock/berlin2.h>
> > +
> > + usb@f7ed0000 {
> > + compatible = "chipidea,usb2";
> > + reg = <0xf7ed0000 0x10000>;
> > + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&chip CLKID_USB0>;
> > + phys = <&usb_phy0>;
> > + phy-names = "usb-phy";
> > + vbus-supply = <&reg_usb0_vbus>;
> > + itc-setting = <0x4>; /* 4 micro-frames */
> > + /* Incremental burst of unspecified length */
> > + ahb-burst-config = <0x0>;
> > + tx-burst-size-dword = <0x10>; /* 64 bytes */
> > + rx-burst-size-dword = <0x10>;
> > + extcon = <0>, <&usb_id>;
> > + phy-clkgate-delay-us = <400>;
> > + mux-controls = <&usb_switch>;
> > + mux-control-names = "usb_switch";
> > + };
> > +
> > + # Example for HSIC:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/clock/imx6qdl-clock.h>
> > +
> > + usb@2184400 {
> > + compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
> > + reg = <0x02184400 0x200>;
> > + interrupts = <0 41 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&clks IMX6QDL_CLK_USBOH3>;
> > + fsl,usbphy = <&usbphynop1>;
> > + fsl,usbmisc = <&usbmisc 2>;
> > + phy_type = "hsic";
> > + dr_mode = "host";
> > + ahb-burst-config = <0x0>;
> > + tx-burst-size-dword = <0x10>;
> > + rx-burst-size-dword = <0x10>;
> > + pinctrl-names = "idle", "active";
> > + pinctrl-0 = <&pinctrl_usbh2_idle>;
> > + pinctrl-1 = <&pinctrl_usbh2_active>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + ethernet@1 {
> > + compatible = "usb424,9730";
> > + reg = <1>;
> > + };
> > + };
> > +
> > +...
> > --
> > 2.37.1
> >

2023-03-04 07:37:37

by Peng Fan (OSS)

[permalink] [raw]

Subject: Re: [PATCH V3 1/7] dt-bindings: usb: usbmisc-imx: convert to DT schema

Hi Rob, Krzysztof,

On 2/28/2023 1:55 PM, Peng Fan wrote:
>> Subject: Re: [PATCH V3 1/7] dt-bindings: usb: usbmisc-imx: convert to DT
>> schema
...
>>> +
>>> +title: Freescale i.MX wrapper module for Chipidea USB2 controller
>>> +
>>> +maintainers:
>>> + - Xu Yang <[email protected]>
>>> + - Peng Fan <[email protected]>
>>> +
>>> +properties:
>>> + compatible:
>>> + oneOf:
>>> + - enum:
>>> + - fsl,imx6q-usbmisc
>>> + - fsl,imx7ulp-usbmisc
>>> + - fsl,vf610-usbmisc
>>> + - items:
>>> + - enum:
>>> + - fsl,imx6ul-usbmisc
>>> + - fsl,imx6sx-usbmisc
>>> + - fsl,imx7d-usbmisc
>>> + - const: fsl,imx6q-usbmisc
>>> + - items:
>>> + - enum:
>>> + - fsl,imx7ulp-usbmisc
>>> + - fsl,imx8mm-usbmisc
>>> + - const: fsl,imx7d-usbmisc
>>
>> So imx8mm is compatible with imx7d, and imx7d is compatible with imx6q,
>> but imx8mm is not compatible with imx6q? That doesn't really make sense.
>> Maybe all 3 compatibles makes sense, but only if s/w understanding only
>> one of the fallback compatibles would function without knowledge of the
>> newer h/w.
>
> I added the list according the current device tree in use. If using
> three compatibles, that would involve device tree upate, and firmware
> update for SR-IR.
>
> My understanding is imx8mm is compatible with imx7d, imx7d is compatible
> with imx6q, then imx8mm is compatible with imx6q. it is just not put
> them under one item. Please correct if my understanding is wrong.
>>

Do you have any guidance here? I am not sure how to proceed on V4.

Thanks,
Peng

>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + '#index-cells':
>>> + const: 1
>>> + description: Cells used to describe usb controller index.
>>
>> Please mark this as deprecated. If it is always 1 cell, then there's no point.
>
> Fix in v4.
>>
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - '#index-cells'
>>
>> And drop as required. That all can be a follow-up patch if you prefer or in
>> this patch is fine. Primarily, I don't want this pattern copied.
>>
>
> Fix in v4.
>
> Thanks,
> Peng.
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + usbmisc@2184800 {
>>> + #index-cells = <1>;
>>> + compatible = "fsl,imx6q-usbmisc";
>>> + reg = <0x02184800 0x200>;
>>> + };
>>> +
>>> +...
>>> diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
>>> b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
>>> deleted file mode 100644
>>> index 29b8f65ff849..000000000000
>>> --- a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
>>> +++ /dev/null
>>> @@ -1,19 +0,0 @@
>>> -* Freescale i.MX non-core registers
>>> -
>>> -Required properties:
>>> -- #index-cells: Cells used to describe usb controller index. Should
>>> be <1>
>>> -- compatible: Should be one of below:
>>> - "fsl,imx6q-usbmisc" for imx6q
>>> - "fsl,vf610-usbmisc" for Vybrid vf610
>>> - "fsl,imx6sx-usbmisc" for imx6sx
>>> - "fsl,imx7d-usbmisc" for imx7d
>>> - "fsl,imx7ulp-usbmisc" for imx7ulp
>>> - "fsl,imx8mm-usbmisc" for imx8mm
>>> -- reg: Should contain registers location and length
>>> -
>>> -Examples:
>>> -usbmisc@2184800 {
>>> - #index-cells = <1>;
>>> - compatible = "fsl,imx6q-usbmisc";
>>> - reg = <0x02184800 0x200>;
>>> -};
>>> --
>>> 2.37.1
>>>