Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <14d359c6-8cfc-c426-f264-1dbf79d1f09e@linaro.org>
Date:   Wed, 12 Oct 2022 09:24:09 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.2
Subject: Re: [PATCH 1/6] dt-bindings: usb: ci-hdrc-usb2: convert to yaml
Content-Language: en-US
To:     "Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
        gregkh@linuxfoundation.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, shawnguo@kernel.org,
        s.hauer@pengutronix.de
Cc:     kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
        linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        jun.li@nxp.com, Peng Fan <peng.fan@nxp.com>
References: <20221010101816.298334-1-peng.fan@oss.nxp.com>
 <20221010101816.298334-2-peng.fan@oss.nxp.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20221010101816.298334-2-peng.fan@oss.nxp.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/10/2022 06:18, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Convert the binding to yaml format
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  | 158 ---------
>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 306 ++++++++++++++++++
>  2 files changed, 306 insertions(+), 158 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.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> deleted file mode 100644
> index ba51fb1252b9..000000000000
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ /dev/null
> @@ -1,158 +0,0 @@
> -* USB2 ChipIdea USB controller for ci13xxx
> -
> -Required properties:
> -- compatible: should be one of:
> -	"fsl,imx23-usb"
> -	"fsl,imx27-usb"
> -	"fsl,imx28-usb"
> -	"fsl,imx6q-usb"
> -	"fsl,imx6sl-usb"
> -	"fsl,imx6sx-usb"
> -	"fsl,imx6ul-usb"
> -	"fsl,imx7d-usb"
> -	"fsl,imx7ulp-usb"
> -	"lsi,zevio-usb"
> -	"qcom,ci-hdrc"
> -	"chipidea,usb2"
> -	"xlnx,zynq-usb-2.20a"
> -	"nvidia,tegra20-udc"
> -	"nvidia,tegra30-udc"
> -	"nvidia,tegra114-udc"
> -	"nvidia,tegra124-udc"
> -- reg: base address and length of the registers
> -- interrupts: interrupt for the USB controller
> -
> -Recommended properies:
> -- phy_type: the type of the phy connected to the core. Should be one
> -  of "utmi", "utmi_wide", "ulpi", "serial" or "hsic". Without this
> -  property the PORTSC register won't be touched.
> -- dr_mode: One of "host", "peripheral" or "otg". Defaults to "otg"
> -
> -Deprecated properties:
> -- usb-phy:      phandle for the PHY device. Use "phys" instead.
> -- fsl,usbphy: phandle of usb phy that connects to the port. Use "phys" instead.
> -
> -Optional properties:
> -- clocks: reference to the USB clock
> -- phys: reference to the USB PHY
> -- phy-names: should be "usb-phy"
> -- vbus-supply: reference to the VBUS regulator
> -- maximum-speed: limit the maximum connection speed to "full-speed".
> -- tpl-support: TPL (Targeted Peripheral List) feature for targeted hosts
> -- itc-setting: interrupt threshold control register control, the setting
> -  should be aligned with ITC bits at register USBCMD.
> -- ahb-burst-config: 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.
> -- tx-burst-size-dword: 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.
> -- rx-burst-size-dword: 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.
> -- extcon: 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: the delay time (us) between putting the PHY into
> -  low power mode and gating the PHY clock.
> -- non-zero-ttctrl-ttha: 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: 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.
> -- mux-control-names: Shall be "usb_switch" if mux-controls is specified.
> -- pinctrl-names: 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.
> -- pinctrl-n: alternate pin modes
> -
> -i.mx specific properties
> -- fsl,usbmisc: phandler of non-core register device, with one
> -  argument that indicate usb controller index
> -- disable-over-current: disable over current detect
> -- over-current-active-low: over current signal polarity is active low.
> -- over-current-active-high: over current signal polarity is active high.
> -  It's recommended to specify the over current polarity.
> -- power-active-high: power signal polarity is active high
> -- external-vbus-divider: enables off-chip resistor divider for Vbus
> -- samsung,picophy-pre-emp-curr-control: 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.
> -- samsung,picophy-dc-vol-level-adjust: 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.
> -
> -Example:
> -
> -	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:
> -
> -	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>;
> -
> -		usbnet: ethernet@1 {
> -			compatible = "usb424,9730";
> -			reg = <1>;
> -		};
> -	};
> 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..1c10691c45f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> @@ -0,0 +1,306 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/ci-hdrc-usb2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: USB2 ChipIdea USB controller for ci13xxx Binding
> +
> +maintainers:
> +  - Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - chipidea,usb2
> +          - 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,imx7ulp-usb
> +          - const: fsl,imx6ul-usb
> +      - items:
> +          - const: lsi,zevio-usb

You already have this in first enum.

> +      - items:
> +          - const: xlnx,zynq-usb-2.20a
> +          - const: chipidea,usb2
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +  

Your patches have several white-space issues.

Don't send patches which fail checkpatch.

> +  clocks:
> +    maxItems: 2
> +
> +  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
> +
> +  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
> +
> +  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.
> +
> +  mux-control-names:
> +    description: Shall be "usb_switch" if mux-controls is specified.

Just define the item here.

> +
> +  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.

Describe the items. Otherwise this does not make much sense, because
core allows it.

> +
> +  pinctrl-0:
> +    maxItems: 1
> +
> +  pinctrl-1:
> +    maxItems: 1
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: usb-phy
> +
> +  vbus-supply:
> +    description: reference to the VBUS regulator.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +allOf:
> +  - $ref: usb-drd.yaml#
> +  - $ref: usb-hcd.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:
> +          oneOf:
> +            - items:
> +                - const: default
> +                - enum:
> +                    - host
> +                    - device
> +            - items:
> +                - const: default
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - fsl,imx23-usb
> +            - fsl,imx25-usb
> +            - fsl,imx27-usb
> +            - fsl,imx28-usb
> +            - fsl,imx6q-usb
> +            - fsl,imx6sl-usb
> +            - fsl,imx6sx-usb
> +            - fsl,imx6ul-usb
> +            - fsl,imx7d-usb
> +            - fsl,imx7ulp-usb
> +    then:
> +      properties:
> +        fsl,usbmisc: 

What's the type? Which other schema defined it?

Same applies to several other properties - are you sure they are already
defined by some other schema?
> +...

Best regards,
Krzysztof