Message-ID: <1a626f74-4559-4403-9d88-6a9a462b54c1@linaro.org>
Date: Thu, 29 Feb 2024 16:11:32 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Xu Yang <xu.yang_2@....com>, gregkh@...uxfoundation.org,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, shawnguo@...nel.org,
 conor+dt@...nel.org
Cc: s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
 linux-imx@....com, peter.chen@...nel.org, jun.li@....com,
 linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 04/11] dt-bindings: usb: ci-hdrc-usb2-imx: move imx
 parts to dedicated schema

On 28/02/2024 12:29, Xu Yang wrote:
> As more and more NXP i.MX chips come out, it becomes harder to maintain
> ci-hdrc-usb2.yaml if more stuffs like property restrictions are added to
> this file. This will separate i.MX parts out of ci-hdrc-usb2.yaml and add
> a new schema for NXP ChipIdea USB2 Controller, also add a common schema.
> 
> 1. Copy common ci-hdrc-usb2.yaml properties to a new shared
>    ci-hdrc-usb2-common.yaml schema.
> 2. Move fsl,* compatible devices and imx spefific properties
>    to dedicated binding file ci-hdrc-usb2-imx.yaml.
> 
> Signed-off-by: Xu Yang <xu.yang_2@....com>
> 
> ---
> Changes in v6:
>  - new patch
> Changes in v7:
>  - not remove ci-hdrc-usb2.yaml and move imx parts to ci-hdrc-usb2-imx.yaml
> ---
>  .../bindings/usb/ci-hdrc-usb2-common.yaml     | 197 ++++++++++++++++++
>  .../bindings/usb/ci-hdrc-usb2-imx.yaml        | 197 ++++++++++++++++++
>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 186 -----------------
>  3 files changed, 394 insertions(+), 186 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/ci-hdrc-usb2-common.yaml
>  create mode 100644 Documentation/devicetree/bindings/usb/ci-hdrc-usb2-imx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2-common.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2-common.yaml
> new file mode 100644
> index 000000000000..9f8f2f343dd3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2-common.yaml

Use compatible style for filenames. chipidea,hdrc-usb2-common.yaml
(assuming that "ci" was a shortcut of chipidea)


> @@ -0,0 +1,197 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/ci-hdrc-usb2-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: USB2 ChipIdea USB controller Common Properties
> +
> +maintainers:
> +  - Xu Yang <xu.yang_2@....com>
> +
> +properties:
> +  reg:
> +    minItems: 1
> +    maxItems: 2
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 3
> +
> +  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.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1
> +    items:
> +      - description: vbus extcon
> +      - description: id extcon
> +
> +  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 previous 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.
> +    type: boolean
> +
> +  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.
> +    oneOf:
> +      - items:
> +          - const: idle
> +          - const: active
> +      - items:
> +          - const: default
> +          - enum:
> +              - host
> +              - device
> +      - items:
> +          - const: default
> +
> +  pinctrl-0:
> +    maxItems: 1
> +
> +  pinctrl-1:
> +    maxItems: 1
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: usb-phy
> +
> +  vbus-supply:
> +    description: reference to the VBUS regulator.
> +
> +  usb-phy:
> +    description: phandle for the PHY device. Use "phys" instead.
> +    maxItems: 1
> +    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
> +
> +dependencies:
> +  port: [ usb-role-switch ]
> +  mux-controls: [ mux-control-names ]
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts

I don't see where you remove the properties from the existing schema, so
you now just duplicated a lot.

So again let me describe the task:
1. Create common schema by MOVING the common pieces from the existing
schema. The common schema MUST be referenced by other existing schemas.

2. Create IMX specific schema by moving IMX specific bits. New schema
references common schema.

3. The remaining old ci-hdrc-usb2.yaml does not have as a result common
properties and IMX bits.


> +
> +allOf:
> +  - $ref: usb-hcd.yaml#
> +  - $ref: usb-drd.yaml#
> +
> +additionalProperties: true
> \ No newline at end of file

You have patch errors, fix them.

> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2-imx.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2-imx.yaml
> new file mode 100644
> index 000000000000..50494ce06d07
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2-imx.yaml
> @@ -0,0 +1,197 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/ci-hdrc-usb2-imx.yaml#

Filename like compatible.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP USB2 ChipIdea USB controller
> +
> +maintainers:
> +  - Xu Yang <xu.yang_2@....com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - fsl,imx27-usb
> +      - items:
> +          - enum:
> +              - fsl,imx23-usb
> +              - fsl,imx25-usb
> +              - fsl,imx28-usb
> +              - fsl,imx35-usb
> +              - fsl,imx50-usb
> +              - fsl,imx51-usb
> +              - fsl,imx53-usb
> +              - fsl,imx6q-usb
> +              - fsl,imx6sl-usb
> +              - fsl,imx6sx-usb
> +              - fsl,imx6ul-usb
> +              - fsl,imx7d-usb
> +              - fsl,vf610-usb
> +          - const: fsl,imx27-usb
> +      - items:
> +          - enum:
> +              - fsl,imx8dxl-usb
> +              - fsl,imx8ulp-usb
> +          - const: fsl,imx7ulp-usb
> +          - const: fsl,imx6ul-usb
> +      - items:
> +          - enum:
> +              - fsl,imx8mm-usb
> +              - fsl,imx8mn-usb
> +          - const: fsl,imx7d-usb
> +          - const: fsl,imx27-usb
> +      - items:
> +          - enum:
> +              - fsl,imx6sll-usb
> +              - fsl,imx7ulp-usb
> +          - const: fsl,imx6ul-usb
> +          - const: fsl,imx27-usb
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3


No need to, it's already in common. Drop clocks.

> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 3

Drop property.

> +
> +  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
> +
> +  fsl,picophy-rise-fall-time-adjust:
> +    description:
> +      HS Transmitter Rise/Fall Time Adjustment. Adjust the rise/fall times
> +      of the high-speed transmitter waveform. It has no unit. The rise/fall
> +      time will be increased or decreased by a certain percentage relative
> +      to design default time. (0:-10%; 1:design default; 2:+15%; 3:+20%)
> +      Details can refer to TXRISETUNE0 bit of USBNC_n_PHY_CFG1.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 3
> +    default: 1
> +
> +  fsl,usbphy:
> +    description: phandle of usb phy that connects to the port. Use "phys" instead.
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    deprecated: true

Missing required: block here.

> +
> +allOf:
> +  - $ref: ci-hdrc-usb2-common.yaml#
> +  - if:
> +      properties:
> +        phy_type:
> +          const: hsic
> +      required:
> +        - phy_type
> +    then:
> +      properties:
> +        pinctrl-names:
> +          items:
> +            - const: idle
> +            - const: active
> +
> +required:
> +  - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/imx7d-clock.h>
> +
> +    usb@...10000 {
> +        compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
> +        reg = <0x30b10000 0x200>;
> +        interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clks IMX7D_USB_CTRL_CLK>;
> +        fsl,usbphy = <&usbphynop1>;
> +        fsl,usbmisc = <&usbmisc1 0>;
> +        phy-clkgate-delay-us = <400>;
> +    };
> +
> +  # Example for HSIC:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/imx6qdl-clock.h>
> +
> +    usb@...4400 {
> +        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>;
> +        };
> +    };
> +
> +...


Best regards,
Krzysztof

Powered by blists - more mailing lists