Message-ID:
 <DU2PR04MB88227686B367CAA509EB8D7D8C272@DU2PR04MB8822.eurprd04.prod.outlook.com>
Date: Fri, 8 Mar 2024 08:30:06 +0000
From: Xu Yang <xu.yang_2@....com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"shawnguo@...nel.org" <shawnguo@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>
CC: "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>, "kernel@...gutronix.de"
	<kernel@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>,
	dl-linux-imx <linux-imx@....com>, "peter.chen@...nel.org"
	<peter.chen@...nel.org>, Jun Li <jun.li@....com>, "linux-usb@...r.kernel.org"
	<linux-usb@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [EXT] 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)

Okay.

>
>
> > @@ -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%23&data=05%7C02%7Cxu.yang_2%40nxp.com%7C2c370c9411664be2924608dc3938b8ac%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C0%7C638448162982019025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj
> oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Aez1Kqo7NsYagBsrTfjys51HUQit0wbHjYg5WigzfzI
> %3D&reserved=0
> > +$schema: http://devicetree.org/meta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cxu.yang_2%40nxp.com%7C2c370c9411664be2924608dc3938b8ac%7C686ea1
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638448162982031177%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=oXRXyhK7NTRb79hXry9qRP14plBJfdApUh
> Z8OHdLB%2FI%3D&reserved=0
> > +
> > +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.

I will remove common parts from old ci-hdrc-usb2.yaml and let ci-hdrc-usb2.yaml
refer to the common yaml too.

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

Okay.

>
>
> > +
> > +allOf:
> > +  - $ref: usb-hcd.yaml#
> > +  - $ref: usb-drd.yaml#
> > +
> > +additionalProperties: true
> > \ No newline at end of file
>
> You have patch errors, fix them.

Okay.

>
> > 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%23&data=05%7C02%7Cxu.yang_2%40nxp.com%7C2c370c9411664be2924608dc3938b8ac%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C638448162982040409%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=1MesqU7ed0dAXtuar%2BNIyV9tOQFahTaDNLxvhgL02
> bw%3D&reserved=0
>
> Filename like compatible.

Okay.

>
> > +$schema: http://devicetree.org/meta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cxu.yang_2%40nxp.com%7C2c370c9411664be2924608dc3938b8ac%7C686ea1
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638448162982048575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=4YG0rLj%2B%2FwsNf7koSMOeiy23Bz7Z7S
> f7B9Z3KcNVvUc%3D&reserved=0
> > +
> > +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.

No. I cannot remove clocks property in this yaml since the usb node may use assigned-clocks
and assigned-clock-parents. Then dtschema will have warning like below patch:
https://lore.kernel.org/all/20230508071837.68552-1-krzysztof.kozlowski@linaro.org/

Thanks,
Xu Yang

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