[PATCH 0/6] add USB2.0 support for i.MX95-19x19 EVK board

[PATCH 0/6] add USB2.0 support for i.MX95-19x19 EVK board

[Xu Yang]

The i.MX95-19x19 EVK board features a USB 2.0 Type-A port, with this
series primarily introducing USB 2.0 support. In the i.MX95 architecture,
the USB wake-up handling mechanism is integrated within the HSIO block
control module, utilizing a dedicated wake-up interrupt. Therefore, we
also implemented corresponding wake-up logic code to properly manage this
functionality.

Xu Yang (6):
  dt-bindings: usb: chipidea: add compatible for i.MX95 platform
  dt-bindings: usb: usbmisc-imx: add compatible for i.MX95 platform
  usb: chipidea: imx: add wakeup interrupt handling
  usb: chipidea: imx: add HSIO Block Control wakup setting
  arm64: dts: imx95: add USB2.0 nodes
  arm64: dts: imx95-19x19-evk: enable USB2.0 node

 .../bindings/usb/chipidea,usb2-common.yaml    |   3 +
 .../bindings/usb/chipidea,usb2-imx.yaml       |  26 ++++-
 .../devicetree/bindings/usb/fsl,usbmisc.yaml  |  23 +++-
 .../boot/dts/freescale/imx95-19x19-evk.dts    |  16 +++
 arch/arm64/boot/dts/freescale/imx95.dtsi      |  30 +++++
 drivers/usb/chipidea/ci_hdrc_imx.c            |  42 +++++++
 drivers/usb/chipidea/usbmisc_imx.c            | 107 ++++++++++++++++++
 7 files changed, 245 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/6] dt-bindings: usb: chipidea: add compatible for i.MX95 platform

[Xu Yang]

The i.MX95 USB2.0 controller is basically compatible with i.MX7d, except
it needs a second interrupt for wakeup handling. This will add compatible
for i.MX95 platform and restriciton on interrupt property. Besides, this
will also add iommus property.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 .../bindings/usb/chipidea,usb2-common.yaml    |  3 +++
 .../bindings/usb/chipidea,usb2-imx.yaml       | 26 ++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml b/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml
index d2a7d2ecf48a..10020af15afc 100644
--- a/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml
+++ b/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml
@@ -42,6 +42,9 @@ properties:
 
   phy_type: true
 
+  iommus:
+    maxItems: 1
+
   itc-setting:
     description:
       interrupt threshold control register control, the setting should be
diff --git a/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml b/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml
index 8f6136f5d72e..3678804a3743 100644
--- a/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml
+++ b/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml
@@ -41,6 +41,7 @@ properties:
               - fsl,imx8mm-usb
               - fsl,imx8mn-usb
               - fsl,imx93-usb
+              - fsl,imx95-usb
           - const: fsl,imx7d-usb
           - const: fsl,imx27-usb
       - items:
@@ -54,7 +55,13 @@ properties:
     maxItems: 1
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: USB controller interrupt
+      - description: Wakeup interrupt
+    description:
+      One interrupt for USB controller and wakeup interrupt combined
+      case or two interrupts for individual USB controller and wakeup
 
   clocks:
     minItems: 1
@@ -191,6 +198,7 @@ allOf:
           contains:
             enum:
               - fsl,imx93-usb
+              - fsl,imx95-usb
     then:
       properties:
         clocks:
@@ -238,6 +246,22 @@ allOf:
           maxItems: 1
         clock-names: false
 
+  # imx95 soc use two interrupts
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx95-usb
+    then:
+      properties:
+        interrupts:
+          minItems: 2
+    else:
+      properties:
+        interrupts:
+          maxItems: 1
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

Re: [PATCH 1/6] dt-bindings: usb: chipidea: add compatible for i.MX95 platform

[Frank Li]

On Wed, Feb 19, 2025 at 05:30:59PM +0800, Xu Yang wrote:
> The i.MX95 USB2.0 controller is basically compatible with i.MX7d, except
> it needs a second interrupt for wakeup handling. This will add compatible
> for i.MX95 platform and restriciton on interrupt property. Besides, this
                          ^ typo,

run check-patch --code-spell

> will also add iommus property.

dt-bindings: usb: Add i.MX95 compatible string 'fsl,imx95-usb'

The i.MX95 USB2.0 controller is mostly compatible with i.MX7D, except it
requires a second interrupt for wakeup handling. Add the compatible string
for the i.MX95 platform, add the iommus property, and enforce the
interrupt property restriction. Keep the same restriction for existing
compatible strings.

Fix below nit.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

Frank


>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  .../bindings/usb/chipidea,usb2-common.yaml    |  3 +++
>  .../bindings/usb/chipidea,usb2-imx.yaml       | 26 ++++++++++++++++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml b/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml
> index d2a7d2ecf48a..10020af15afc 100644
> --- a/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml
> +++ b/Documentation/devicetree/bindings/usb/chipidea,usb2-common.yaml
> @@ -42,6 +42,9 @@ properties:
>
>    phy_type: true
>
> +  iommus:
> +    maxItems: 1
> +
>    itc-setting:
>      description:
>        interrupt threshold control register control, the setting should be
> diff --git a/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml b/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml
> index 8f6136f5d72e..3678804a3743 100644
> --- a/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml
> +++ b/Documentation/devicetree/bindings/usb/chipidea,usb2-imx.yaml
> @@ -41,6 +41,7 @@ properties:
>                - fsl,imx8mm-usb
>                - fsl,imx8mn-usb
>                - fsl,imx93-usb
> +              - fsl,imx95-usb
>            - const: fsl,imx7d-usb
>            - const: fsl,imx27-usb
>        - items:
> @@ -54,7 +55,13 @@ properties:
>      maxItems: 1
>
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: USB controller interrupt
> +      - description: Wakeup interrupt
> +    description:
> +      One interrupt for USB controller and wakeup interrupt combined
> +      case or two interrupts for individual USB controller and wakeup

Needn't descrition here. Items: descrption already provide enough
informaiton.  Or you can change first

description: USB controller interrupt Or combine USB controller and wakeup
interrupts.


>
>    clocks:
>      minItems: 1
> @@ -191,6 +198,7 @@ allOf:
>            contains:
>              enum:
>                - fsl,imx93-usb
> +              - fsl,imx95-usb
>      then:
>        properties:
>          clocks:
> @@ -238,6 +246,22 @@ allOf:
>            maxItems: 1
>          clock-names: false
>
> +  # imx95 soc use two interrupts
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,imx95-usb
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 2
> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +
>  unevaluatedProperties: false
>
>  examples:
> --
> 2.34.1
>

Re: [PATCH 1/6] dt-bindings: usb: chipidea: add compatible for i.MX95 platform

[Xu Yang]

On Wed, Feb 19, 2025 at 12:58:10PM -0500, Frank Li wrote:
> On Wed, Feb 19, 2025 at 05:30:59PM +0800, Xu Yang wrote:
> > The i.MX95 USB2.0 controller is basically compatible with i.MX7d, except
> > it needs a second interrupt for wakeup handling. This will add compatible
> > for i.MX95 platform and restriciton on interrupt property. Besides, this
>                           ^ typo,
> 
> run check-patch --code-spell
> 
> > will also add iommus property.
> 
> dt-bindings: usb: Add i.MX95 compatible string 'fsl,imx95-usb'
> 
> The i.MX95 USB2.0 controller is mostly compatible with i.MX7D, except it
> requires a second interrupt for wakeup handling. Add the compatible string
> for the i.MX95 platform, add the iommus property, and enforce the
> interrupt property restriction. Keep the same restriction for existing
> compatible strings.

Okay.

> 
> Fix below nit.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
> Frank
> 
> 
> >

[...]

> > +    description:
> > +      One interrupt for USB controller and wakeup interrupt combined
> > +      case or two interrupts for individual USB controller and wakeup
> 
> Needn't descrition here. Items: descrption already provide enough
> informaiton.  Or you can change first
> 
> description: USB controller interrupt Or combine USB controller and wakeup
> interrupts.

Okay. will do.

Thanks,
Xu Yang

[PATCH 2/6] dt-bindings: usb: usbmisc-imx: add compatible for i.MX95 platform

[Xu Yang]

The i.MX95 USB2.0 controller is basically compatible with i.MX7D,
except it needs use hsio block control for wakeup setting. This will
add compatible for i.MX95 platform and restriciton on reg property.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 .../devicetree/bindings/usb/fsl,usbmisc.yaml  | 23 ++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml b/Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml
index 0a6e7ac1b37e..019435540df0 100644
--- a/Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml
+++ b/Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml
@@ -34,6 +34,7 @@ properties:
               - fsl,imx8mm-usbmisc
               - fsl,imx8mn-usbmisc
               - fsl,imx8ulp-usbmisc
+              - fsl,imx95-usbmisc
           - const: fsl,imx7d-usbmisc
           - const: fsl,imx6q-usbmisc
       - items:
@@ -45,7 +46,10 @@ properties:
     maxItems: 1
 
   reg:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Base and length of the Wrapper module register
+      - description: Base and length of the HSIO Block Control register
 
   '#index-cells':
     const: 1
@@ -56,6 +60,23 @@ required:
   - compatible
   - reg
 
+allOf:
+  # imx95 soc needs use HSIO Block Control
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx95-usbmisc
+    then:
+      properties:
+        reg:
+          minItems: 2
+    else:
+      properties:
+        reg:
+          maxItems: 1
+
 additionalProperties: false
 
 examples:
-- 
2.34.1

Re: [PATCH 2/6] dt-bindings: usb: usbmisc-imx: add compatible for i.MX95 platform

[Frank Li]

On Wed, Feb 19, 2025 at 05:31:00PM +0800, Xu Yang wrote:
> The i.MX95 USB2.0 controller is basically compatible with i.MX7D,
> except it needs use hsio block control for wakeup setting. This will
> add compatible for i.MX95 platform and restriciton on reg property.

Nit: Simple said
Add compatible ...

Better mention "fsl,imx95-usbmisc" at subject.
add compatible string "fsl,imx95-usbmisc" for ...

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  .../devicetree/bindings/usb/fsl,usbmisc.yaml  | 23 ++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml b/Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml
> index 0a6e7ac1b37e..019435540df0 100644
> --- a/Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml
> +++ b/Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml
> @@ -34,6 +34,7 @@ properties:
>                - fsl,imx8mm-usbmisc
>                - fsl,imx8mn-usbmisc
>                - fsl,imx8ulp-usbmisc
> +              - fsl,imx95-usbmisc
>            - const: fsl,imx7d-usbmisc
>            - const: fsl,imx6q-usbmisc
>        - items:
> @@ -45,7 +46,10 @@ properties:
>      maxItems: 1
>
>    reg:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Base and length of the Wrapper module register
> +      - description: Base and length of the HSIO Block Control register
>
>    '#index-cells':
>      const: 1
> @@ -56,6 +60,23 @@ required:
>    - compatible
>    - reg
>
> +allOf:
> +  # imx95 soc needs use HSIO Block Control
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,imx95-usbmisc
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2
> +    else:
> +      properties:
> +        reg:
> +          maxItems: 1
> +
>  additionalProperties: false
>
>  examples:
> --
> 2.34.1
>

Re: [PATCH 2/6] dt-bindings: usb: usbmisc-imx: add compatible for i.MX95 platform

[Xu Yang]

On Wed, Feb 19, 2025 at 01:01:30PM -0500, Frank Li wrote:
> On Wed, Feb 19, 2025 at 05:31:00PM +0800, Xu Yang wrote:
> > The i.MX95 USB2.0 controller is basically compatible with i.MX7D,
> > except it needs use hsio block control for wakeup setting. This will
> > add compatible for i.MX95 platform and restriciton on reg property.
> 
> Nit: Simple said
> Add compatible ...
> 
> Better mention "fsl,imx95-usbmisc" at subject.
> add compatible string "fsl,imx95-usbmisc" for ...

Okay.

Thanks,
Xu Yang

> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 

Re: [PATCH 2/6] dt-bindings: usb: usbmisc-imx: add compatible for i.MX95 platform

[Rob Herring]

On Thu, Feb 20, 2025 at 06:32:09PM +0800, Xu Yang wrote:
> On Wed, Feb 19, 2025 at 01:01:30PM -0500, Frank Li wrote:
> > On Wed, Feb 19, 2025 at 05:31:00PM +0800, Xu Yang wrote:
> > > The i.MX95 USB2.0 controller is basically compatible with i.MX7D,
> > > except it needs use hsio block control for wakeup setting. This will
> > > add compatible for i.MX95 platform and restriciton on reg property.
> > 
> > Nit: Simple said
> > Add compatible ...
> > 
> > Better mention "fsl,imx95-usbmisc" at subject.
> > add compatible string "fsl,imx95-usbmisc" for ...

The patch does a bit more than just add a compatible. 'Add i.MX95 
support' is really enough. The diff is clear it is adding a compatible.

Rob

[PATCH 3/6] usb: chipidea: imx: add wakeup interrupt handling

[Xu Yang]

In previous imx platform, normal USB controller interrupt and wakeup
interrupt are bound to one irq line. However, it changes on latest
i.MX95 platform since it has a dedicated irq line for wakeup interrupt.
This will add wakup interrupt handling for i.MX95 to support various
wakeup events.

Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.c | 42 ++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 1a7fc638213e..5779568ebcf6 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -98,9 +98,12 @@ struct ci_hdrc_imx_data {
 	struct clk *clk;
 	struct clk *clk_wakeup;
 	struct imx_usbmisc_data *usbmisc_data;
+	/* wakeup interrupt*/
+	int irq;
 	bool supports_runtime_pm;
 	bool override_phy_control;
 	bool in_lpm;
+	bool wakeup_pending;
 	struct pinctrl *pinctrl;
 	struct pinctrl_state *pinctrl_hsic_active;
 	struct regulator *hsic_pad_regulator;
@@ -336,6 +339,24 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
 	return ret;
 }
 
+static irqreturn_t ci_wakeup_irq_handler(int irq, void *data)
+{
+	struct ci_hdrc_imx_data *imx_data = data;
+
+	if (imx_data->in_lpm) {
+		if (imx_data->wakeup_pending)
+			return IRQ_HANDLED;
+
+		disable_irq_nosync(irq);
+		imx_data->wakeup_pending = true;
+		pm_runtime_resume(&imx_data->ci_pdev->dev);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
 static int ci_hdrc_imx_probe(struct platform_device *pdev)
 {
 	struct ci_hdrc_imx_data *data;
@@ -476,6 +497,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
 		data->supports_runtime_pm = true;
 
+	data->irq = platform_get_irq_optional(pdev, 1);
+	if (data->irq > 0) {
+		ret = devm_request_threaded_irq(dev, data->irq,
+				NULL, ci_wakeup_irq_handler,
+				IRQF_ONESHOT, pdata.name, data);
+		if (ret)
+			goto err_clk;
+	}
+
 	ret = imx_usbmisc_init(data->usbmisc_data);
 	if (ret) {
 		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
@@ -621,6 +651,11 @@ static int imx_controller_resume(struct device *dev,
 		goto clk_disable;
 	}
 
+	if (data->wakeup_pending) {
+		data->wakeup_pending = false;
+		enable_irq(data->irq);
+	}
+
 	return 0;
 
 clk_disable:
@@ -643,6 +678,10 @@ static int ci_hdrc_imx_suspend(struct device *dev)
 		return ret;
 
 	pinctrl_pm_select_sleep_state(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(data->irq);
+
 	return ret;
 }
 
@@ -651,6 +690,9 @@ static int ci_hdrc_imx_resume(struct device *dev)
 	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
 	int ret;
 
+	if (device_may_wakeup(dev))
+		disable_irq_wake(data->irq);
+
 	pinctrl_pm_select_default_state(dev);
 	ret = imx_controller_resume(dev, PMSG_RESUME);
 	if (!ret && data->supports_runtime_pm) {
-- 
2.34.1

Re: [PATCH 3/6] usb: chipidea: imx: add wakeup interrupt handling

[Frank Li]

On Wed, Feb 19, 2025 at 05:31:01PM +0800, Xu Yang wrote:
> In previous imx platform, normal USB controller interrupt and wakeup
> interrupt are bound to one irq line. However, it changes on latest
> i.MX95 platform since it has a dedicated irq line for wakeup interrupt.
> This will add wakup interrupt handling for i.MX95 to support various
> wakeup events.
>
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_imx.c | 42 ++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 1a7fc638213e..5779568ebcf6 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -98,9 +98,12 @@ struct ci_hdrc_imx_data {
>  	struct clk *clk;
>  	struct clk *clk_wakeup;
>  	struct imx_usbmisc_data *usbmisc_data;
> +	/* wakeup interrupt*/
> +	int irq;

use "wakeup_irq" to avoid confuse with normal controller irq?

>  	bool supports_runtime_pm;
>  	bool override_phy_control;
>  	bool in_lpm;
> +	bool wakeup_pending;
>  	struct pinctrl *pinctrl;
>  	struct pinctrl_state *pinctrl_hsic_active;
>  	struct regulator *hsic_pad_regulator;
> @@ -336,6 +339,24 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
>  	return ret;
>  }
>
> +static irqreturn_t ci_wakeup_irq_handler(int irq, void *data)
> +{
> +	struct ci_hdrc_imx_data *imx_data = data;
> +
> +	if (imx_data->in_lpm) {
> +		if (imx_data->wakeup_pending)
> +			return IRQ_HANDLED;
> +
> +		disable_irq_nosync(irq);
> +		imx_data->wakeup_pending = true;
> +		pm_runtime_resume(&imx_data->ci_pdev->dev);

Not sure why need pm_runtime_resume here? There are not access register
here.

> +
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
>  static int ci_hdrc_imx_probe(struct platform_device *pdev)
>  {
>  	struct ci_hdrc_imx_data *data;
> @@ -476,6 +497,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
>  		data->supports_runtime_pm = true;
>
> +	data->irq = platform_get_irq_optional(pdev, 1);
> +	if (data->irq > 0) {
> +		ret = devm_request_threaded_irq(dev, data->irq,
> +				NULL, ci_wakeup_irq_handler,
> +				IRQF_ONESHOT, pdata.name, data);
> +		if (ret)
> +			goto err_clk;
> +	}
> +
>  	ret = imx_usbmisc_init(data->usbmisc_data);
>  	if (ret) {
>  		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> @@ -621,6 +651,11 @@ static int imx_controller_resume(struct device *dev,
>  		goto clk_disable;
>  	}
>
> +	if (data->wakeup_pending) {
> +		data->wakeup_pending = false;
> +		enable_irq(data->irq);
> +	}
> +
>  	return 0;
>
>  clk_disable:
> @@ -643,6 +678,10 @@ static int ci_hdrc_imx_suspend(struct device *dev)
>  		return ret;
>
>  	pinctrl_pm_select_sleep_state(dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(data->irq);
> +
>  	return ret;
>  }
>
> @@ -651,6 +690,9 @@ static int ci_hdrc_imx_resume(struct device *dev)
>  	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
>  	int ret;
>
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(data->irq);
> +

Look like only want enable wakeup irq between suspend and resume. There are
some questions to understand hehavor.

1 driver probe --> 2. hdrc suspend -->3 hdrc resume --> 4 controller resume

1--2,  look like wakeup irq is enabled. maybe controller have some bit to
disable issue wakeup irq, otherwise flood irq will happen because you check
lpm in irq handler.

after 2. wakeup irq enable,  if wakeup irq happen, system resume.
ci_hdrc_imx_resume() only clear a flags, not any sync problem.

But sequence imx_controller_resume() and ci_wakeup_irq_handler() may not
guaranteed.

If ci_wakeup_irq_handler() call first, ci_wakeup_irq_handler() disable
itself. imx_controller_resume() will enable wakeup irq, which will be same
state 1--2. but if imx_controller_resume() run firstly,
ci_wakeup_irq_handler() will disable wakeup irq, which difference state
with 1--2.

If there are a bit(WAKEUP_EN) in controller can control wakeup irq
enable/disable,

will below logic be simple?

ci_hdrc_imx_suspend()
{
	writel(WAKEUP_EN, ...);
	....
}

ci_wakeup_irq_handler()
{
	...
	pm_runtime_resume(&imx_data->ci_pdev->dev);
	writel(~WAKEUP_EN, ...);
	...
}

Frank

>  	pinctrl_pm_select_default_state(dev);
>  	ret = imx_controller_resume(dev, PMSG_RESUME);
>  	if (!ret && data->supports_runtime_pm) {
> --
> 2.34.1
>

Re: [PATCH 3/6] usb: chipidea: imx: add wakeup interrupt handling

[Xu Yang]

On Wed, Feb 19, 2025 at 03:26:42PM -0500, Frank Li wrote:
> On Wed, Feb 19, 2025 at 05:31:01PM +0800, Xu Yang wrote:
> > In previous imx platform, normal USB controller interrupt and wakeup
> > interrupt are bound to one irq line. However, it changes on latest
> > i.MX95 platform since it has a dedicated irq line for wakeup interrupt.
> > This will add wakup interrupt handling for i.MX95 to support various
> > wakeup events.
> >
> > Reviewed-by: Jun Li <jun.li@nxp.com>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/chipidea/ci_hdrc_imx.c | 42 ++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 1a7fc638213e..5779568ebcf6 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -98,9 +98,12 @@ struct ci_hdrc_imx_data {
> >  	struct clk *clk;
> >  	struct clk *clk_wakeup;
> >  	struct imx_usbmisc_data *usbmisc_data;
> > +	/* wakeup interrupt*/
> > +	int irq;
> 
> use "wakeup_irq" to avoid confuse with normal controller irq?

It doesn't matter. It can't be confused since the driver is different. The
controller driver is core.c. This glue driver is ci_hdrc_imx.c.

> 
> >  	bool supports_runtime_pm;
> >  	bool override_phy_control;
> >  	bool in_lpm;
> > +	bool wakeup_pending;
> >  	struct pinctrl *pinctrl;
> >  	struct pinctrl_state *pinctrl_hsic_active;
> >  	struct regulator *hsic_pad_regulator;
> > @@ -336,6 +339,24 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
> >  	return ret;
> >  }
> >
> > +static irqreturn_t ci_wakeup_irq_handler(int irq, void *data)
> > +{
> > +	struct ci_hdrc_imx_data *imx_data = data;
> > +
> > +	if (imx_data->in_lpm) {
> > +		if (imx_data->wakeup_pending)
> > +			return IRQ_HANDLED;
> > +
> > +		disable_irq_nosync(irq);
> > +		imx_data->wakeup_pending = true;
> > +		pm_runtime_resume(&imx_data->ci_pdev->dev);
> 
> Not sure why need pm_runtime_resume here? There are not access register
> here.

It's needed for runtime resume case.
When wakeup event happens, wakeup irq will be triggered. To let controller
resume back, we need enable USB controller clock to trigger controller
irq. So we call pm_runtime_resume() to resume controller's parent back
and the parent device will enable clocks.

> 
> > +
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> >  static int ci_hdrc_imx_probe(struct platform_device *pdev)
> >  {
> >  	struct ci_hdrc_imx_data *data;
> > @@ -476,6 +497,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> >  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> >  		data->supports_runtime_pm = true;
> >
> > +	data->irq = platform_get_irq_optional(pdev, 1);
> > +	if (data->irq > 0) {
> > +		ret = devm_request_threaded_irq(dev, data->irq,
> > +				NULL, ci_wakeup_irq_handler,
> > +				IRQF_ONESHOT, pdata.name, data);
> > +		if (ret)
> > +			goto err_clk;
> > +	}
> > +
> >  	ret = imx_usbmisc_init(data->usbmisc_data);
> >  	if (ret) {
> >  		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> > @@ -621,6 +651,11 @@ static int imx_controller_resume(struct device *dev,
> >  		goto clk_disable;
> >  	}
> >
> > +	if (data->wakeup_pending) {
> > +		data->wakeup_pending = false;
> > +		enable_irq(data->irq);
> > +	}
> > +
> >  	return 0;
> >
> >  clk_disable:
> > @@ -643,6 +678,10 @@ static int ci_hdrc_imx_suspend(struct device *dev)
> >  		return ret;
> >
> >  	pinctrl_pm_select_sleep_state(dev);
> > +
> > +	if (device_may_wakeup(dev))
> > +		enable_irq_wake(data->irq);
> > +
> >  	return ret;
> >  }
> >
> > @@ -651,6 +690,9 @@ static int ci_hdrc_imx_resume(struct device *dev)
> >  	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> >  	int ret;
> >
> > +	if (device_may_wakeup(dev))
> > +		disable_irq_wake(data->irq);
> > +
> 
> Look like only want enable wakeup irq between suspend and resume. There are
> some questions to understand hehavor.
> 
> 1 driver probe --> 2. hdrc suspend -->3 hdrc resume --> 4 controller resume
> 
> 1--2,  look like wakeup irq is enabled. maybe controller have some bit to
> disable issue wakeup irq, otherwise flood irq will happen because you check
> lpm in irq handler.

It's not true.

We has a bit WAKE_ENABLE to enable/disable wakeup irq. This bit will only be
enabled when do suspend() and disabled when do resume(). So before suspend()
called, the wakeup irq can't be triggered. No flood irq too.

> 
> after 2. wakeup irq enable,  if wakeup irq happen, system resume.
> ci_hdrc_imx_resume() only clear a flags, not any sync problem.
> 
> But sequence imx_controller_resume() and ci_wakeup_irq_handler() may not
> guaranteed.
> 
> If ci_wakeup_irq_handler() call first, ci_wakeup_irq_handler() disable
> itself. imx_controller_resume() will enable wakeup irq, which will be same
> state 1--2. but if imx_controller_resume() run firstly,

It's not true too. Because WAKE_ENABLE is disabled after resume().

> ci_wakeup_irq_handler() will disable wakeup irq, which difference state
> with 1--2.
> 
> If there are a bit(WAKEUP_EN) in controller can control wakeup irq
> enable/disable,

Yes, WAKE_ENABLE bit. It's already used:

ci_hdrc_imx_suspend()
  imx_controller_suspend() -> enable WAKE_ENABLE

ci_hdrc_imx_resume()
  imx_controller_resume() -> disable WAKE_ENABLE

Thanks,
Xu Yang

> 
> will below logic be simple?
> 
> ci_hdrc_imx_suspend()
> {
> 	writel(WAKEUP_EN, ...);
> 	....
> }
> 
> ci_wakeup_irq_handler()
> {
> 	...
> 	pm_runtime_resume(&imx_data->ci_pdev->dev);
> 	writel(~WAKEUP_EN, ...);
> 	...
> }
> 
> Frank
> 
> >  	pinctrl_pm_select_default_state(dev);
> >  	ret = imx_controller_resume(dev, PMSG_RESUME);
> >  	if (!ret && data->supports_runtime_pm) {
> > --
> > 2.34.1
> >

Re: [PATCH 3/6] usb: chipidea: imx: add wakeup interrupt handling

[Frank Li]

On Fri, Feb 21, 2025 at 11:23:48AM +0800, Xu Yang wrote:
> On Wed, Feb 19, 2025 at 03:26:42PM -0500, Frank Li wrote:
> > On Wed, Feb 19, 2025 at 05:31:01PM +0800, Xu Yang wrote:
> > > In previous imx platform, normal USB controller interrupt and wakeup
> > > interrupt are bound to one irq line. However, it changes on latest
> > > i.MX95 platform since it has a dedicated irq line for wakeup interrupt.
> > > This will add wakup interrupt handling for i.MX95 to support various
> > > wakeup events.
> > >
> > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > ---
> > >  drivers/usb/chipidea/ci_hdrc_imx.c | 42 ++++++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index 1a7fc638213e..5779568ebcf6 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > @@ -98,9 +98,12 @@ struct ci_hdrc_imx_data {
> > >  	struct clk *clk;
> > >  	struct clk *clk_wakeup;
> > >  	struct imx_usbmisc_data *usbmisc_data;
> > > +	/* wakeup interrupt*/
> > > +	int irq;
> >
> > use "wakeup_irq" to avoid confuse with normal controller irq?
>
> It doesn't matter. It can't be confused since the driver is different. The
> controller driver is core.c. This glue driver is ci_hdrc_imx.c.
>
> >
> > >  	bool supports_runtime_pm;
> > >  	bool override_phy_control;
> > >  	bool in_lpm;
> > > +	bool wakeup_pending;
> > >  	struct pinctrl *pinctrl;
> > >  	struct pinctrl_state *pinctrl_hsic_active;
> > >  	struct regulator *hsic_pad_regulator;
> > > @@ -336,6 +339,24 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
> > >  	return ret;
> > >  }
> > >
> > > +static irqreturn_t ci_wakeup_irq_handler(int irq, void *data)
> > > +{
> > > +	struct ci_hdrc_imx_data *imx_data = data;
> > > +
> > > +	if (imx_data->in_lpm) {
> > > +		if (imx_data->wakeup_pending)
> > > +			return IRQ_HANDLED;
> > > +
> > > +		disable_irq_nosync(irq);
> > > +		imx_data->wakeup_pending = true;
> > > +		pm_runtime_resume(&imx_data->ci_pdev->dev);
> >
> > Not sure why need pm_runtime_resume here? There are not access register
> > here.
>
> It's needed for runtime resume case.
> When wakeup event happens, wakeup irq will be triggered. To let controller
> resume back, we need enable USB controller clock to trigger controller
> irq. So we call pm_runtime_resume() to resume controller's parent back
> and the parent device will enable clocks.

Please add comment here. why need in_lpm if we can make sure irq only
enable during suspend/resume pharse?

>
> >
> > > +
> > > +		return IRQ_HANDLED;
> > > +	}
> > > +
> > > +	return IRQ_NONE;
> > > +}
> > > +
> > >  static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > >  {
> > >  	struct ci_hdrc_imx_data *data;
> > > @@ -476,6 +497,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > >  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> > >  		data->supports_runtime_pm = true;
> > >
> > > +	data->irq = platform_get_irq_optional(pdev, 1);
> > > +	if (data->irq > 0) {
> > > +		ret = devm_request_threaded_irq(dev, data->irq,
> > > +				NULL, ci_wakeup_irq_handler,
> > > +				IRQF_ONESHOT, pdata.name, data);
> > > +		if (ret)
> > > +			goto err_clk;
> > > +	}
> > > +
> > >  	ret = imx_usbmisc_init(data->usbmisc_data);
> > >  	if (ret) {
> > >  		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> > > @@ -621,6 +651,11 @@ static int imx_controller_resume(struct device *dev,
> > >  		goto clk_disable;
> > >  	}
> > >
> > > +	if (data->wakeup_pending) {
> > > +		data->wakeup_pending = false;
> > > +		enable_irq(data->irq);
> > > +	}
> > > +
> > >  	return 0;
> > >
> > >  clk_disable:
> > > @@ -643,6 +678,10 @@ static int ci_hdrc_imx_suspend(struct device *dev)
> > >  		return ret;
> > >
> > >  	pinctrl_pm_select_sleep_state(dev);
> > > +
> > > +	if (device_may_wakeup(dev))
> > > +		enable_irq_wake(data->irq);
> > > +
> > >  	return ret;
> > >  }
> > >
> > > @@ -651,6 +690,9 @@ static int ci_hdrc_imx_resume(struct device *dev)
> > >  	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> > >  	int ret;
> > >
> > > +	if (device_may_wakeup(dev))
> > > +		disable_irq_wake(data->irq);
> > > +
> >
> > Look like only want enable wakeup irq between suspend and resume. There are
> > some questions to understand hehavor.
> >
> > 1 driver probe --> 2. hdrc suspend -->3 hdrc resume --> 4 controller resume
> >
> > 1--2,  look like wakeup irq is enabled. maybe controller have some bit to
> > disable issue wakeup irq, otherwise flood irq will happen because you check
> > lpm in irq handler.
>
> It's not true.
>
> We has a bit WAKE_ENABLE to enable/disable wakeup irq. This bit will only be
> enabled when do suspend() and disabled when do resume(). So before suspend()
> called, the wakeup irq can't be triggered. No flood irq too.
>
> >
> > after 2. wakeup irq enable,  if wakeup irq happen, system resume.
> > ci_hdrc_imx_resume() only clear a flags, not any sync problem.
> >
> > But sequence imx_controller_resume() and ci_wakeup_irq_handler() may not
> > guaranteed.
> >
> > If ci_wakeup_irq_handler() call first, ci_wakeup_irq_handler() disable
> > itself. imx_controller_resume() will enable wakeup irq, which will be same
> > state 1--2. but if imx_controller_resume() run firstly,
>
> It's not true too. Because WAKE_ENABLE is disabled after resume().
>
> > ci_wakeup_irq_handler() will disable wakeup irq, which difference state
> > with 1--2.
> >
> > If there are a bit(WAKEUP_EN) in controller can control wakeup irq
> > enable/disable,
>
> Yes, WAKE_ENABLE bit. It's already used:
>
> ci_hdrc_imx_suspend()
>   imx_controller_suspend() -> enable WAKE_ENABLE
>
> ci_hdrc_imx_resume()
>   imx_controller_resume() -> disable WAKE_ENABLE

Okay,

I think wakeup_pending is not neccesary. ci_wakeup_irq_handler() can
simple disable WAKE_ENABLE.

Frank

>
> Thanks,
> Xu Yang
>
> >
> > will below logic be simple?
> >
> > ci_hdrc_imx_suspend()
> > {
> > 	writel(WAKEUP_EN, ...);
> > 	....
> > }
> >
> > ci_wakeup_irq_handler()
> > {
> > 	...
> > 	pm_runtime_resume(&imx_data->ci_pdev->dev);
> > 	writel(~WAKEUP_EN, ...);
> > 	...
> > }
> >
> > Frank
> >
> > >  	pinctrl_pm_select_default_state(dev);
> > >  	ret = imx_controller_resume(dev, PMSG_RESUME);
> > >  	if (!ret && data->supports_runtime_pm) {
> > > --
> > > 2.34.1
> > >

Re: [PATCH 3/6] usb: chipidea: imx: add wakeup interrupt handling

[Xu Yang]

On Fri, Feb 21, 2025 at 10:23:26AM -0500, Frank Li wrote:
> On Fri, Feb 21, 2025 at 11:23:48AM +0800, Xu Yang wrote:
> > On Wed, Feb 19, 2025 at 03:26:42PM -0500, Frank Li wrote:
> > > On Wed, Feb 19, 2025 at 05:31:01PM +0800, Xu Yang wrote:
> > > > In previous imx platform, normal USB controller interrupt and wakeup
> > > > interrupt are bound to one irq line. However, it changes on latest
> > > > i.MX95 platform since it has a dedicated irq line for wakeup interrupt.
> > > > This will add wakup interrupt handling for i.MX95 to support various
> > > > wakeup events.
> > > >
> > > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > ---
> > > >  drivers/usb/chipidea/ci_hdrc_imx.c | 42 ++++++++++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > index 1a7fc638213e..5779568ebcf6 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > @@ -98,9 +98,12 @@ struct ci_hdrc_imx_data {
> > > >  	struct clk *clk;
> > > >  	struct clk *clk_wakeup;
> > > >  	struct imx_usbmisc_data *usbmisc_data;
> > > > +	/* wakeup interrupt*/
> > > > +	int irq;
> > >
> > > use "wakeup_irq" to avoid confuse with normal controller irq?
> >
> > It doesn't matter. It can't be confused since the driver is different. The
> > controller driver is core.c. This glue driver is ci_hdrc_imx.c.
> >
> > >
> > > >  	bool supports_runtime_pm;
> > > >  	bool override_phy_control;
> > > >  	bool in_lpm;
> > > > +	bool wakeup_pending;
> > > >  	struct pinctrl *pinctrl;
> > > >  	struct pinctrl_state *pinctrl_hsic_active;
> > > >  	struct regulator *hsic_pad_regulator;
> > > > @@ -336,6 +339,24 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
> > > >  	return ret;
> > > >  }
> > > >
> > > > +static irqreturn_t ci_wakeup_irq_handler(int irq, void *data)
> > > > +{
> > > > +	struct ci_hdrc_imx_data *imx_data = data;
> > > > +
> > > > +	if (imx_data->in_lpm) {
> > > > +		if (imx_data->wakeup_pending)
> > > > +			return IRQ_HANDLED;
> > > > +
> > > > +		disable_irq_nosync(irq);
> > > > +		imx_data->wakeup_pending = true;
> > > > +		pm_runtime_resume(&imx_data->ci_pdev->dev);
> > >
> > > Not sure why need pm_runtime_resume here? There are not access register
> > > here.
> >
> > It's needed for runtime resume case.
> > When wakeup event happens, wakeup irq will be triggered. To let controller
> > resume back, we need enable USB controller clock to trigger controller
> > irq. So we call pm_runtime_resume() to resume controller's parent back
> > and the parent device will enable clocks.
> 
> Please add comment here. why need in_lpm if we can make sure irq only
> enable during suspend/resume pharse?

I have checked again, in_lpm checking is not needed. I will remove the
if condition later.

> 
> >
> > >
> > > > +
> > > > +		return IRQ_HANDLED;
> > > > +	}
> > > > +
> > > > +	return IRQ_NONE;
> > > > +}
> > > > +
> > > >  static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > >  {
> > > >  	struct ci_hdrc_imx_data *data;
> > > > @@ -476,6 +497,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > >  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> > > >  		data->supports_runtime_pm = true;
> > > >
> > > > +	data->irq = platform_get_irq_optional(pdev, 1);
> > > > +	if (data->irq > 0) {
> > > > +		ret = devm_request_threaded_irq(dev, data->irq,
> > > > +				NULL, ci_wakeup_irq_handler,
> > > > +				IRQF_ONESHOT, pdata.name, data);
> > > > +		if (ret)
> > > > +			goto err_clk;
> > > > +	}
> > > > +
> > > >  	ret = imx_usbmisc_init(data->usbmisc_data);
> > > >  	if (ret) {
> > > >  		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> > > > @@ -621,6 +651,11 @@ static int imx_controller_resume(struct device *dev,
> > > >  		goto clk_disable;
> > > >  	}
> > > >
> > > > +	if (data->wakeup_pending) {
> > > > +		data->wakeup_pending = false;
> > > > +		enable_irq(data->irq);
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >
> > > >  clk_disable:
> > > > @@ -643,6 +678,10 @@ static int ci_hdrc_imx_suspend(struct device *dev)
> > > >  		return ret;
> > > >
> > > >  	pinctrl_pm_select_sleep_state(dev);
> > > > +
> > > > +	if (device_may_wakeup(dev))
> > > > +		enable_irq_wake(data->irq);
> > > > +
> > > >  	return ret;
> > > >  }
> > > >
> > > > @@ -651,6 +690,9 @@ static int ci_hdrc_imx_resume(struct device *dev)
> > > >  	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> > > >  	int ret;
> > > >
> > > > +	if (device_may_wakeup(dev))
> > > > +		disable_irq_wake(data->irq);
> > > > +
> > >
> > > Look like only want enable wakeup irq between suspend and resume. There are
> > > some questions to understand hehavor.
> > >
> > > 1 driver probe --> 2. hdrc suspend -->3 hdrc resume --> 4 controller resume
> > >
> > > 1--2,  look like wakeup irq is enabled. maybe controller have some bit to
> > > disable issue wakeup irq, otherwise flood irq will happen because you check
> > > lpm in irq handler.
> >
> > It's not true.
> >
> > We has a bit WAKE_ENABLE to enable/disable wakeup irq. This bit will only be
> > enabled when do suspend() and disabled when do resume(). So before suspend()
> > called, the wakeup irq can't be triggered. No flood irq too.
> >
> > >
> > > after 2. wakeup irq enable,  if wakeup irq happen, system resume.
> > > ci_hdrc_imx_resume() only clear a flags, not any sync problem.
> > >
> > > But sequence imx_controller_resume() and ci_wakeup_irq_handler() may not
> > > guaranteed.
> > >
> > > If ci_wakeup_irq_handler() call first, ci_wakeup_irq_handler() disable
> > > itself. imx_controller_resume() will enable wakeup irq, which will be same
> > > state 1--2. but if imx_controller_resume() run firstly,
> >
> > It's not true too. Because WAKE_ENABLE is disabled after resume().
> >
> > > ci_wakeup_irq_handler() will disable wakeup irq, which difference state
> > > with 1--2.
> > >
> > > If there are a bit(WAKEUP_EN) in controller can control wakeup irq
> > > enable/disable,
> >
> > Yes, WAKE_ENABLE bit. It's already used:
> >
> > ci_hdrc_imx_suspend()
> >   imx_controller_suspend() -> enable WAKE_ENABLE
> >
> > ci_hdrc_imx_resume()
> >   imx_controller_resume() -> disable WAKE_ENABLE
> 
> Okay,
> 
> I think wakeup_pending is not neccesary. ci_wakeup_irq_handler() can
> simple disable WAKE_ENABLE.

Right, wakeup_pending can be removed. However, it's not that simple to
control WAKE_ENABLE in ci_hdrc_imx.c due to imx_controller_susepnd/resume()
do more things expect enable/disable WAKE_ENABLE bit. And usbmisc.c doesn't
export a function to directly control WAKE_ENABLE. Therefore, I still need
to use enable/disable_irq() for simplicity.

Due to this wakeup irq is only used in suspend case, I would like to add
IRQF_NO_AUTOEN flag to request_threaded_irq(), then enable irq in
ci_hdrc_imx_suspend() and disable irq in ci_hdrc_imx_resume(). 

For example:

ci_wakeup_irq_handler()
{
	disable_irq_nosync(data->irq);
	...
}

ci_hdrc_imx_suspend()
{
	...
	enable_irq(data->irq);	
}

ci_hdrc_imx_resume()
{
	if (irq disabled)
		disable_irq_nosync(data->irq);
	...
}

Do you think if it's feasible and better than current patch?

Thanks,
Xu Yang

Re: [PATCH 3/6] usb: chipidea: imx: add wakeup interrupt handling

[Frank Li]

On Mon, Feb 24, 2025 at 07:07:51PM +0800, Xu Yang wrote:
> On Fri, Feb 21, 2025 at 10:23:26AM -0500, Frank Li wrote:
> > On Fri, Feb 21, 2025 at 11:23:48AM +0800, Xu Yang wrote:
> > > On Wed, Feb 19, 2025 at 03:26:42PM -0500, Frank Li wrote:
> > > > On Wed, Feb 19, 2025 at 05:31:01PM +0800, Xu Yang wrote:
> > > > > In previous imx platform, normal USB controller interrupt and wakeup
> > > > > interrupt are bound to one irq line. However, it changes on latest
> > > > > i.MX95 platform since it has a dedicated irq line for wakeup interrupt.
> > > > > This will add wakup interrupt handling for i.MX95 to support various
> > > > > wakeup events.
> > > > >
> > > > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > > ---
> > > > >  drivers/usb/chipidea/ci_hdrc_imx.c | 42 ++++++++++++++++++++++++++++++
> > > > >  1 file changed, 42 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > > index 1a7fc638213e..5779568ebcf6 100644
> > > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > > @@ -98,9 +98,12 @@ struct ci_hdrc_imx_data {
> > > > >  	struct clk *clk;
> > > > >  	struct clk *clk_wakeup;
> > > > >  	struct imx_usbmisc_data *usbmisc_data;
> > > > > +	/* wakeup interrupt*/
> > > > > +	int irq;
> > > >
> > > > use "wakeup_irq" to avoid confuse with normal controller irq?
> > >
> > > It doesn't matter. It can't be confused since the driver is different. The
> > > controller driver is core.c. This glue driver is ci_hdrc_imx.c.
> > >
> > > >
> > > > >  	bool supports_runtime_pm;
> > > > >  	bool override_phy_control;
> > > > >  	bool in_lpm;
> > > > > +	bool wakeup_pending;
> > > > >  	struct pinctrl *pinctrl;
> > > > >  	struct pinctrl_state *pinctrl_hsic_active;
> > > > >  	struct regulator *hsic_pad_regulator;
> > > > > @@ -336,6 +339,24 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
> > > > >  	return ret;
> > > > >  }
> > > > >
> > > > > +static irqreturn_t ci_wakeup_irq_handler(int irq, void *data)
> > > > > +{
> > > > > +	struct ci_hdrc_imx_data *imx_data = data;
> > > > > +
> > > > > +	if (imx_data->in_lpm) {
> > > > > +		if (imx_data->wakeup_pending)
> > > > > +			return IRQ_HANDLED;
> > > > > +
> > > > > +		disable_irq_nosync(irq);
> > > > > +		imx_data->wakeup_pending = true;
> > > > > +		pm_runtime_resume(&imx_data->ci_pdev->dev);
> > > >
> > > > Not sure why need pm_runtime_resume here? There are not access register
> > > > here.
> > >
> > > It's needed for runtime resume case.
> > > When wakeup event happens, wakeup irq will be triggered. To let controller
> > > resume back, we need enable USB controller clock to trigger controller
> > > irq. So we call pm_runtime_resume() to resume controller's parent back
> > > and the parent device will enable clocks.
> >
> > Please add comment here. why need in_lpm if we can make sure irq only
> > enable during suspend/resume pharse?
>
> I have checked again, in_lpm checking is not needed. I will remove the
> if condition later.
>
> >
> > >
> > > >
> > > > > +
> > > > > +		return IRQ_HANDLED;
> > > > > +	}
> > > > > +
> > > > > +	return IRQ_NONE;
> > > > > +}
> > > > > +
> > > > >  static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > > >  {
> > > > >  	struct ci_hdrc_imx_data *data;
> > > > > @@ -476,6 +497,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > > >  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> > > > >  		data->supports_runtime_pm = true;
> > > > >
> > > > > +	data->irq = platform_get_irq_optional(pdev, 1);
> > > > > +	if (data->irq > 0) {
> > > > > +		ret = devm_request_threaded_irq(dev, data->irq,
> > > > > +				NULL, ci_wakeup_irq_handler,
> > > > > +				IRQF_ONESHOT, pdata.name, data);
> > > > > +		if (ret)
> > > > > +			goto err_clk;
> > > > > +	}
> > > > > +
> > > > >  	ret = imx_usbmisc_init(data->usbmisc_data);
> > > > >  	if (ret) {
> > > > >  		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> > > > > @@ -621,6 +651,11 @@ static int imx_controller_resume(struct device *dev,
> > > > >  		goto clk_disable;
> > > > >  	}
> > > > >
> > > > > +	if (data->wakeup_pending) {
> > > > > +		data->wakeup_pending = false;
> > > > > +		enable_irq(data->irq);
> > > > > +	}
> > > > > +
> > > > >  	return 0;
> > > > >
> > > > >  clk_disable:
> > > > > @@ -643,6 +678,10 @@ static int ci_hdrc_imx_suspend(struct device *dev)
> > > > >  		return ret;
> > > > >
> > > > >  	pinctrl_pm_select_sleep_state(dev);
> > > > > +
> > > > > +	if (device_may_wakeup(dev))
> > > > > +		enable_irq_wake(data->irq);
> > > > > +
> > > > >  	return ret;
> > > > >  }
> > > > >
> > > > > @@ -651,6 +690,9 @@ static int ci_hdrc_imx_resume(struct device *dev)
> > > > >  	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> > > > >  	int ret;
> > > > >
> > > > > +	if (device_may_wakeup(dev))
> > > > > +		disable_irq_wake(data->irq);
> > > > > +
> > > >
> > > > Look like only want enable wakeup irq between suspend and resume. There are
> > > > some questions to understand hehavor.
> > > >
> > > > 1 driver probe --> 2. hdrc suspend -->3 hdrc resume --> 4 controller resume
> > > >
> > > > 1--2,  look like wakeup irq is enabled. maybe controller have some bit to
> > > > disable issue wakeup irq, otherwise flood irq will happen because you check
> > > > lpm in irq handler.
> > >
> > > It's not true.
> > >
> > > We has a bit WAKE_ENABLE to enable/disable wakeup irq. This bit will only be
> > > enabled when do suspend() and disabled when do resume(). So before suspend()
> > > called, the wakeup irq can't be triggered. No flood irq too.
> > >
> > > >
> > > > after 2. wakeup irq enable,  if wakeup irq happen, system resume.
> > > > ci_hdrc_imx_resume() only clear a flags, not any sync problem.
> > > >
> > > > But sequence imx_controller_resume() and ci_wakeup_irq_handler() may not
> > > > guaranteed.
> > > >
> > > > If ci_wakeup_irq_handler() call first, ci_wakeup_irq_handler() disable
> > > > itself. imx_controller_resume() will enable wakeup irq, which will be same
> > > > state 1--2. but if imx_controller_resume() run firstly,
> > >
> > > It's not true too. Because WAKE_ENABLE is disabled after resume().
> > >
> > > > ci_wakeup_irq_handler() will disable wakeup irq, which difference state
> > > > with 1--2.
> > > >
> > > > If there are a bit(WAKEUP_EN) in controller can control wakeup irq
> > > > enable/disable,
> > >
> > > Yes, WAKE_ENABLE bit. It's already used:
> > >
> > > ci_hdrc_imx_suspend()
> > >   imx_controller_suspend() -> enable WAKE_ENABLE
> > >
> > > ci_hdrc_imx_resume()
> > >   imx_controller_resume() -> disable WAKE_ENABLE
> >
> > Okay,
> >
> > I think wakeup_pending is not neccesary. ci_wakeup_irq_handler() can
> > simple disable WAKE_ENABLE.
>
> Right, wakeup_pending can be removed. However, it's not that simple to
> control WAKE_ENABLE in ci_hdrc_imx.c due to imx_controller_susepnd/resume()
> do more things expect enable/disable WAKE_ENABLE bit. And usbmisc.c doesn't
> export a function to directly control WAKE_ENABLE. Therefore, I still need
> to use enable/disable_irq() for simplicity.
>
> Due to this wakeup irq is only used in suspend case, I would like to add
> IRQF_NO_AUTOEN flag to request_threaded_irq(), then enable irq in
> ci_hdrc_imx_suspend() and disable irq in ci_hdrc_imx_resume().
>
> For example:
>
> ci_wakeup_irq_handler()
> {
> 	disable_irq_nosync(data->irq);
> 	...
> }
>
> ci_hdrc_imx_suspend()
> {
> 	...
> 	enable_irq(data->irq);
> }
>
> ci_hdrc_imx_resume()
> {
> 	if (irq disabled)
> 		disable_irq_nosync(data->irq);
> 	...
> }
>
> Do you think if it's feasible and better than current patch?

look better. Does ci_wakeup_irq_handler() always happen before
ci_hdrc_imx_resume()?

Frank

>
> Thanks,
> Xu Yang

Re: [PATCH 3/6] usb: chipidea: imx: add wakeup interrupt handling

[Xu Yang]

On Mon, Feb 24, 2025 at 10:58:18AM -0500, Frank Li wrote:
> On Mon, Feb 24, 2025 at 07:07:51PM +0800, Xu Yang wrote:
> > On Fri, Feb 21, 2025 at 10:23:26AM -0500, Frank Li wrote:
> > > On Fri, Feb 21, 2025 at 11:23:48AM +0800, Xu Yang wrote:
> > > > On Wed, Feb 19, 2025 at 03:26:42PM -0500, Frank Li wrote:
> > > > > On Wed, Feb 19, 2025 at 05:31:01PM +0800, Xu Yang wrote:
> > > > > > In previous imx platform, normal USB controller interrupt and wakeup
> > > > > > interrupt are bound to one irq line. However, it changes on latest
> > > > > > i.MX95 platform since it has a dedicated irq line for wakeup interrupt.
> > > > > > This will add wakup interrupt handling for i.MX95 to support various
> > > > > > wakeup events.
> > > > > >
> > > > > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > > > ---
> > > > > >  drivers/usb/chipidea/ci_hdrc_imx.c | 42 ++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 42 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > > > index 1a7fc638213e..5779568ebcf6 100644
> > > > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > > > @@ -98,9 +98,12 @@ struct ci_hdrc_imx_data {
> > > > > >  	struct clk *clk;
> > > > > >  	struct clk *clk_wakeup;
> > > > > >  	struct imx_usbmisc_data *usbmisc_data;
> > > > > > +	/* wakeup interrupt*/
> > > > > > +	int irq;
> > > > >
> > > > > use "wakeup_irq" to avoid confuse with normal controller irq?
> > > >
> > > > It doesn't matter. It can't be confused since the driver is different. The
> > > > controller driver is core.c. This glue driver is ci_hdrc_imx.c.
> > > >
> > > > >
> > > > > >  	bool supports_runtime_pm;
> > > > > >  	bool override_phy_control;
> > > > > >  	bool in_lpm;
> > > > > > +	bool wakeup_pending;
> > > > > >  	struct pinctrl *pinctrl;
> > > > > >  	struct pinctrl_state *pinctrl_hsic_active;
> > > > > >  	struct regulator *hsic_pad_regulator;
> > > > > > @@ -336,6 +339,24 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
> > > > > >  	return ret;
> > > > > >  }
> > > > > >
> > > > > > +static irqreturn_t ci_wakeup_irq_handler(int irq, void *data)
> > > > > > +{
> > > > > > +	struct ci_hdrc_imx_data *imx_data = data;
> > > > > > +
> > > > > > +	if (imx_data->in_lpm) {
> > > > > > +		if (imx_data->wakeup_pending)
> > > > > > +			return IRQ_HANDLED;
> > > > > > +
> > > > > > +		disable_irq_nosync(irq);
> > > > > > +		imx_data->wakeup_pending = true;
> > > > > > +		pm_runtime_resume(&imx_data->ci_pdev->dev);
> > > > >
> > > > > Not sure why need pm_runtime_resume here? There are not access register
> > > > > here.
> > > >
> > > > It's needed for runtime resume case.
> > > > When wakeup event happens, wakeup irq will be triggered. To let controller
> > > > resume back, we need enable USB controller clock to trigger controller
> > > > irq. So we call pm_runtime_resume() to resume controller's parent back
> > > > and the parent device will enable clocks.
> > >
> > > Please add comment here. why need in_lpm if we can make sure irq only
> > > enable during suspend/resume pharse?
> >
> > I have checked again, in_lpm checking is not needed. I will remove the
> > if condition later.
> >
> > >
> > > >
> > > > >
> > > > > > +
> > > > > > +		return IRQ_HANDLED;
> > > > > > +	}
> > > > > > +
> > > > > > +	return IRQ_NONE;
> > > > > > +}
> > > > > > +
> > > > > >  static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > > > >  {
> > > > > >  	struct ci_hdrc_imx_data *data;
> > > > > > @@ -476,6 +497,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> > > > > >  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> > > > > >  		data->supports_runtime_pm = true;
> > > > > >
> > > > > > +	data->irq = platform_get_irq_optional(pdev, 1);
> > > > > > +	if (data->irq > 0) {
> > > > > > +		ret = devm_request_threaded_irq(dev, data->irq,
> > > > > > +				NULL, ci_wakeup_irq_handler,
> > > > > > +				IRQF_ONESHOT, pdata.name, data);
> > > > > > +		if (ret)
> > > > > > +			goto err_clk;
> > > > > > +	}
> > > > > > +
> > > > > >  	ret = imx_usbmisc_init(data->usbmisc_data);
> > > > > >  	if (ret) {
> > > > > >  		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> > > > > > @@ -621,6 +651,11 @@ static int imx_controller_resume(struct device *dev,
> > > > > >  		goto clk_disable;
> > > > > >  	}
> > > > > >
> > > > > > +	if (data->wakeup_pending) {
> > > > > > +		data->wakeup_pending = false;
> > > > > > +		enable_irq(data->irq);
> > > > > > +	}
> > > > > > +
> > > > > >  	return 0;
> > > > > >
> > > > > >  clk_disable:
> > > > > > @@ -643,6 +678,10 @@ static int ci_hdrc_imx_suspend(struct device *dev)
> > > > > >  		return ret;
> > > > > >
> > > > > >  	pinctrl_pm_select_sleep_state(dev);
> > > > > > +
> > > > > > +	if (device_may_wakeup(dev))
> > > > > > +		enable_irq_wake(data->irq);
> > > > > > +
> > > > > >  	return ret;
> > > > > >  }
> > > > > >
> > > > > > @@ -651,6 +690,9 @@ static int ci_hdrc_imx_resume(struct device *dev)
> > > > > >  	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> > > > > >  	int ret;
> > > > > >
> > > > > > +	if (device_may_wakeup(dev))
> > > > > > +		disable_irq_wake(data->irq);
> > > > > > +
> > > > >
> > > > > Look like only want enable wakeup irq between suspend and resume. There are
> > > > > some questions to understand hehavor.
> > > > >
> > > > > 1 driver probe --> 2. hdrc suspend -->3 hdrc resume --> 4 controller resume
> > > > >
> > > > > 1--2,  look like wakeup irq is enabled. maybe controller have some bit to
> > > > > disable issue wakeup irq, otherwise flood irq will happen because you check
> > > > > lpm in irq handler.
> > > >
> > > > It's not true.
> > > >
> > > > We has a bit WAKE_ENABLE to enable/disable wakeup irq. This bit will only be
> > > > enabled when do suspend() and disabled when do resume(). So before suspend()
> > > > called, the wakeup irq can't be triggered. No flood irq too.
> > > >
> > > > >
> > > > > after 2. wakeup irq enable,  if wakeup irq happen, system resume.
> > > > > ci_hdrc_imx_resume() only clear a flags, not any sync problem.
> > > > >
> > > > > But sequence imx_controller_resume() and ci_wakeup_irq_handler() may not
> > > > > guaranteed.
> > > > >
> > > > > If ci_wakeup_irq_handler() call first, ci_wakeup_irq_handler() disable
> > > > > itself. imx_controller_resume() will enable wakeup irq, which will be same
> > > > > state 1--2. but if imx_controller_resume() run firstly,
> > > >
> > > > It's not true too. Because WAKE_ENABLE is disabled after resume().
> > > >
> > > > > ci_wakeup_irq_handler() will disable wakeup irq, which difference state
> > > > > with 1--2.
> > > > >
> > > > > If there are a bit(WAKEUP_EN) in controller can control wakeup irq
> > > > > enable/disable,
> > > >
> > > > Yes, WAKE_ENABLE bit. It's already used:
> > > >
> > > > ci_hdrc_imx_suspend()
> > > >   imx_controller_suspend() -> enable WAKE_ENABLE
> > > >
> > > > ci_hdrc_imx_resume()
> > > >   imx_controller_resume() -> disable WAKE_ENABLE
> > >
> > > Okay,
> > >
> > > I think wakeup_pending is not neccesary. ci_wakeup_irq_handler() can
> > > simple disable WAKE_ENABLE.
> >
> > Right, wakeup_pending can be removed. However, it's not that simple to
> > control WAKE_ENABLE in ci_hdrc_imx.c due to imx_controller_susepnd/resume()
> > do more things expect enable/disable WAKE_ENABLE bit. And usbmisc.c doesn't
> > export a function to directly control WAKE_ENABLE. Therefore, I still need
> > to use enable/disable_irq() for simplicity.
> >
> > Due to this wakeup irq is only used in suspend case, I would like to add
> > IRQF_NO_AUTOEN flag to request_threaded_irq(), then enable irq in
> > ci_hdrc_imx_suspend() and disable irq in ci_hdrc_imx_resume().
> >
> > For example:
> >
> > ci_wakeup_irq_handler()
> > {
> > 	disable_irq_nosync(data->irq);
> > 	...
> > }
> >
> > ci_hdrc_imx_suspend()
> > {
> > 	...
> > 	enable_irq(data->irq);
> > }
> >
> > ci_hdrc_imx_resume()
> > {
> > 	if (irq disabled)
> > 		disable_irq_nosync(data->irq);
> > 	...
> > }
> >
> > Do you think if it's feasible and better than current patch?
> 
> look better. Does ci_wakeup_irq_handler() always happen before
> ci_hdrc_imx_resume()?

It depends on whether a wakeup event happens.
If wakeup event comes, then wakeup irq will be triggered and 
ci_wakeup_irq_handler() will be called before ci_hdrc_imx_resume().
If not, only ci_hdrc_imx_resume() will be called.

Thanks,
Xu Yang

[PATCH 4/6] usb: chipidea: imx: add HSIO Block Control wakup setting

[Xu Yang]

On i.MX95 platform, USB wakeup setting is controlled by HSIO Block
Control:

HSIO Block Control Overview:
- The HSIO block control include configuration and status registers that
  provide miscellaneous top-level controls for clocking, beat limiter
  enables, wakeup signal enables and interrupt status for the PCIe and USB
  interfaces.

The wakeup function of HSIO blkctl is basically same as non-core, except
improvements about power lost cases. This will add the wakup setting for
HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory. then do
wakeup setting as needs.

Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/chipidea/usbmisc_imx.c | 107 +++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
index 1394881fde5f..f723f8670bb8 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -139,6 +139,22 @@
 #define MX6_USB_OTG_WAKEUP_BITS (MX6_BM_WAKEUP_ENABLE | MX6_BM_VBUS_WAKEUP | \
 				 MX6_BM_ID_WAKEUP | MX6SX_BM_DPDM_WAKEUP_EN)
 
+/*
+ * HSIO Block Control Register
+ */
+
+#define BLKCTL_USB_WAKEUP_CTRL		0x0
+#define BLKCTL_OTG_WAKE_ENABLE		BIT(31)
+#define BLKCTL_OTG_VBUS_SESSVALID	BIT(4)
+#define BLKCTL_OTG_ID_WAKEUP_EN		BIT(2)
+#define BLKCTL_OTG_VBUS_WAKEUP_EN	BIT(1)
+#define BLKCTL_OTG_DPDM_WAKEUP_EN	BIT(0)
+
+#define BLKCTL_WAKEUP_SOURCE		(BLKCTL_OTG_WAKE_ENABLE	   | \
+					 BLKCTL_OTG_ID_WAKEUP_EN   | \
+					 BLKCTL_OTG_VBUS_WAKEUP_EN | \
+					 BLKCTL_OTG_DPDM_WAKEUP_EN)
+
 struct usbmisc_ops {
 	/* It's called once when probe a usb device */
 	int (*init)(struct imx_usbmisc_data *data);
@@ -159,6 +175,7 @@ struct usbmisc_ops {
 
 struct imx_usbmisc {
 	void __iomem *base;
+	void __iomem *blkctl;
 	spinlock_t lock;
 	const struct usbmisc_ops *ops;
 };
@@ -1016,6 +1033,76 @@ static int usbmisc_imx6sx_power_lost_check(struct imx_usbmisc_data *data)
 		return 0;
 }
 
+static u32 usbmisc_blkctl_wakeup_setting(struct imx_usbmisc_data *data)
+{
+	u32 wakeup_setting = BLKCTL_WAKEUP_SOURCE;
+
+	if (data->ext_id || data->available_role != USB_DR_MODE_OTG)
+		wakeup_setting &= ~BLKCTL_OTG_ID_WAKEUP_EN;
+
+	if (data->ext_vbus || data->available_role == USB_DR_MODE_HOST)
+		wakeup_setting &= ~BLKCTL_OTG_VBUS_WAKEUP_EN;
+
+	/* Selet session valid as VBUS wakeup source */
+	wakeup_setting |= BLKCTL_OTG_VBUS_SESSVALID;
+
+	return wakeup_setting;
+}
+
+static int usbmisc_imx95_set_wakeup(struct imx_usbmisc_data *data, bool enabled)
+{
+	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&usbmisc->lock, flags);
+	val = readl(usbmisc->blkctl + BLKCTL_USB_WAKEUP_CTRL);
+	val &= ~BLKCTL_WAKEUP_SOURCE;
+
+	if (enabled)
+		val |= usbmisc_blkctl_wakeup_setting(data);
+
+	writel(val, usbmisc->blkctl + BLKCTL_USB_WAKEUP_CTRL);
+	spin_unlock_irqrestore(&usbmisc->lock, flags);
+
+	return 0;
+}
+
+static int usbmisc_imx95_init(struct imx_usbmisc_data *data)
+{
+	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
+	unsigned long flags;
+	u32 reg;
+
+	if (data->index >= 1)
+		return -EINVAL;
+
+	spin_lock_irqsave(&usbmisc->lock, flags);
+	reg = readl(usbmisc->base);
+
+	if (data->disable_oc) {
+		reg |= MX6_BM_OVER_CUR_DIS;
+	} else {
+		reg &= ~MX6_BM_OVER_CUR_DIS;
+
+		if (data->oc_pol_configured && data->oc_pol_active_low)
+			reg |= MX6_BM_OVER_CUR_POLARITY;
+		else if (data->oc_pol_configured)
+			reg &= ~MX6_BM_OVER_CUR_POLARITY;
+	}
+
+	if (data->pwr_pol == 1)
+		reg |= MX6_BM_PWR_POLARITY;
+
+	writel(reg, usbmisc->base);
+	spin_unlock_irqrestore(&usbmisc->lock, flags);
+
+	/* use HSIO blkctl wakeup as source, disable usbmisc setting*/
+	usbmisc_imx7d_set_wakeup(data, false);
+
+	return 0;
+}
+
 static const struct usbmisc_ops imx25_usbmisc_ops = {
 	.init = usbmisc_imx25_init,
 	.post = usbmisc_imx25_post,
@@ -1068,6 +1155,14 @@ static const struct usbmisc_ops imx7ulp_usbmisc_ops = {
 	.power_lost_check = usbmisc_imx7d_power_lost_check,
 };
 
+static const struct usbmisc_ops imx95_usbmisc_ops = {
+	.init = usbmisc_imx95_init,
+	.set_wakeup = usbmisc_imx95_set_wakeup,
+	.charger_detection = imx7d_charger_detection,
+	.power_lost_check = usbmisc_imx7d_power_lost_check,
+	.vbus_comparator_on = usbmisc_imx7d_vbus_comparator_on,
+};
+
 static inline bool is_imx53_usbmisc(struct imx_usbmisc_data *data)
 {
 	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
@@ -1289,6 +1384,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = {
 		.compatible = "fsl,imx8ulp-usbmisc",
 		.data = &imx7ulp_usbmisc_ops,
 	},
+	{
+		.compatible = "fsl,imx95-usbmisc",
+		.data = &imx95_usbmisc_ops,
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
@@ -1296,6 +1395,7 @@ MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
 static int usbmisc_imx_probe(struct platform_device *pdev)
 {
 	struct imx_usbmisc *data;
+	struct resource *res;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -1307,6 +1407,13 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
 	if (IS_ERR(data->base))
 		return PTR_ERR(data->base);
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res) {
+		data->blkctl = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(data->blkctl))
+			return PTR_ERR(data->blkctl);
+	}
+
 	data->ops = of_device_get_match_data(&pdev->dev);
 	platform_set_drvdata(pdev, data);
 
-- 
2.34.1

Re: [PATCH 4/6] usb: chipidea: imx: add HSIO Block Control wakup setting

[Frank Li]

On Wed, Feb 19, 2025 at 05:31:02PM +0800, Xu Yang wrote:
> On i.MX95 platform, USB wakeup setting is controlled by HSIO Block
> Control:
>
> HSIO Block Control Overview:
> - The HSIO block control include configuration and status registers that
>   provide miscellaneous top-level controls for clocking, beat limiter
>   enables, wakeup signal enables and interrupt status for the PCIe and USB
>   interfaces.
>
> The wakeup function of HSIO blkctl is basically same as non-core, except
> improvements about power lost cases. This will add the wakup setting for
> HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory. then do
> wakeup setting as needs.
>
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/usb/chipidea/usbmisc_imx.c | 107 +++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
>
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> index 1394881fde5f..f723f8670bb8 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -139,6 +139,22 @@
>  #define MX6_USB_OTG_WAKEUP_BITS (MX6_BM_WAKEUP_ENABLE | MX6_BM_VBUS_WAKEUP | \
>  				 MX6_BM_ID_WAKEUP | MX6SX_BM_DPDM_WAKEUP_EN)
>
> +/*
> + * HSIO Block Control Register
> + */
> +
> +#define BLKCTL_USB_WAKEUP_CTRL		0x0
> +#define BLKCTL_OTG_WAKE_ENABLE		BIT(31)
> +#define BLKCTL_OTG_VBUS_SESSVALID	BIT(4)
> +#define BLKCTL_OTG_ID_WAKEUP_EN		BIT(2)
> +#define BLKCTL_OTG_VBUS_WAKEUP_EN	BIT(1)
> +#define BLKCTL_OTG_DPDM_WAKEUP_EN	BIT(0)
> +
> +#define BLKCTL_WAKEUP_SOURCE		(BLKCTL_OTG_WAKE_ENABLE	   | \
> +					 BLKCTL_OTG_ID_WAKEUP_EN   | \
> +					 BLKCTL_OTG_VBUS_WAKEUP_EN | \
> +					 BLKCTL_OTG_DPDM_WAKEUP_EN)
> +
>  struct usbmisc_ops {
>  	/* It's called once when probe a usb device */
>  	int (*init)(struct imx_usbmisc_data *data);
> @@ -159,6 +175,7 @@ struct usbmisc_ops {
>
>  struct imx_usbmisc {
>  	void __iomem *base;
> +	void __iomem *blkctl;
>  	spinlock_t lock;
>  	const struct usbmisc_ops *ops;
>  };
> @@ -1016,6 +1033,76 @@ static int usbmisc_imx6sx_power_lost_check(struct imx_usbmisc_data *data)
>  		return 0;
>  }
>
> +static u32 usbmisc_blkctl_wakeup_setting(struct imx_usbmisc_data *data)
> +{
> +	u32 wakeup_setting = BLKCTL_WAKEUP_SOURCE;
> +
> +	if (data->ext_id || data->available_role != USB_DR_MODE_OTG)
> +		wakeup_setting &= ~BLKCTL_OTG_ID_WAKEUP_EN;
> +
> +	if (data->ext_vbus || data->available_role == USB_DR_MODE_HOST)
> +		wakeup_setting &= ~BLKCTL_OTG_VBUS_WAKEUP_EN;
> +
> +	/* Selet session valid as VBUS wakeup source */
> +	wakeup_setting |= BLKCTL_OTG_VBUS_SESSVALID;
> +
> +	return wakeup_setting;
> +}
> +
> +static int usbmisc_imx95_set_wakeup(struct imx_usbmisc_data *data, bool enabled)
> +{
> +	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&usbmisc->lock, flags);
> +	val = readl(usbmisc->blkctl + BLKCTL_USB_WAKEUP_CTRL);
> +	val &= ~BLKCTL_WAKEUP_SOURCE;
> +
> +	if (enabled)
> +		val |= usbmisc_blkctl_wakeup_setting(data);
> +
> +	writel(val, usbmisc->blkctl + BLKCTL_USB_WAKEUP_CTRL);
> +	spin_unlock_irqrestore(&usbmisc->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int usbmisc_imx95_init(struct imx_usbmisc_data *data)
> +{
> +	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
> +	unsigned long flags;
> +	u32 reg;
> +
> +	if (data->index >= 1)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&usbmisc->lock, flags);
> +	reg = readl(usbmisc->base);
> +
> +	if (data->disable_oc) {
> +		reg |= MX6_BM_OVER_CUR_DIS;
> +	} else {
> +		reg &= ~MX6_BM_OVER_CUR_DIS;
> +
> +		if (data->oc_pol_configured && data->oc_pol_active_low)
> +			reg |= MX6_BM_OVER_CUR_POLARITY;
> +		else if (data->oc_pol_configured)
> +			reg &= ~MX6_BM_OVER_CUR_POLARITY;
> +	}
> +
> +	if (data->pwr_pol == 1)
> +		reg |= MX6_BM_PWR_POLARITY;
> +
> +	writel(reg, usbmisc->base);
> +	spin_unlock_irqrestore(&usbmisc->lock, flags);
> +
> +	/* use HSIO blkctl wakeup as source, disable usbmisc setting*/
> +	usbmisc_imx7d_set_wakeup(data, false);
> +
> +	return 0;
> +}
> +
>  static const struct usbmisc_ops imx25_usbmisc_ops = {
>  	.init = usbmisc_imx25_init,
>  	.post = usbmisc_imx25_post,
> @@ -1068,6 +1155,14 @@ static const struct usbmisc_ops imx7ulp_usbmisc_ops = {
>  	.power_lost_check = usbmisc_imx7d_power_lost_check,
>  };
>
> +static const struct usbmisc_ops imx95_usbmisc_ops = {
> +	.init = usbmisc_imx95_init,
> +	.set_wakeup = usbmisc_imx95_set_wakeup,
> +	.charger_detection = imx7d_charger_detection,
> +	.power_lost_check = usbmisc_imx7d_power_lost_check,
> +	.vbus_comparator_on = usbmisc_imx7d_vbus_comparator_on,
> +};
> +
>  static inline bool is_imx53_usbmisc(struct imx_usbmisc_data *data)
>  {
>  	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
> @@ -1289,6 +1384,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = {
>  		.compatible = "fsl,imx8ulp-usbmisc",
>  		.data = &imx7ulp_usbmisc_ops,
>  	},
> +	{
> +		.compatible = "fsl,imx95-usbmisc",
> +		.data = &imx95_usbmisc_ops,
> +	},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
> @@ -1296,6 +1395,7 @@ MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
>  static int usbmisc_imx_probe(struct platform_device *pdev)
>  {
>  	struct imx_usbmisc *data;
> +	struct resource *res;
>
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
> @@ -1307,6 +1407,13 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
>  	if (IS_ERR(data->base))
>  		return PTR_ERR(data->base);
>
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (res) {
> +		data->blkctl = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(data->blkctl))
> +			return PTR_ERR(data->blkctl);
> +	}
> +
>  	data->ops = of_device_get_match_data(&pdev->dev);
>  	platform_set_drvdata(pdev, data);
>
> --
> 2.34.1
>

[PATCH 5/6] arm64: dts: imx95: add USB2.0 nodes

[Xu Yang]

Add USB2.0 controller and phy nodes.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx95.dtsi | 30 ++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
index 0b64a1df3c7a..a7c87b9843bd 100644
--- a/arch/arm64/boot/dts/freescale/imx95.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
@@ -463,6 +463,13 @@ its: msi-controller@48040000 {
 		};
 	};
 
+	usbphynop: usbphynop {
+		compatible = "usb-nop-xceiv";
+		clocks = <&scmi_clk IMX95_CLK_HSIO>;
+		clock-names = "main_clk";
+		#phy-cells = <0>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <2>;
@@ -1579,6 +1586,29 @@ usb3_phy: phy@4c1f0040 {
 			status = "disabled";
 		};
 
+		usb2: usb@4c200000 {
+			compatible = "fsl,imx95-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
+			reg = <0x0 0x4c200000 0x0 0x200>;
+			interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&scmi_clk IMX95_CLK_HSIO>,
+				 <&scmi_clk IMX95_CLK_32K>;
+			clock-names = "usb_ctrl_root", "usb_wakeup";
+			iommus = <&smmu 0xf>;
+			phys = <&usbphynop>;
+			power-domains = <&scmi_devpd IMX95_PD_HSIO_TOP>;
+			fsl,usbmisc = <&usbmisc 0>;
+			status = "disabled";
+		};
+
+		usbmisc: usbmisc@4c200200 {
+			compatible = "fsl,imx95-usbmisc", "fsl,imx7d-usbmisc",
+				     "fsl,imx6q-usbmisc";
+			reg = <0x0 0x4c200200 0x0 0x200>,
+			      <0x0 0x4c010014 0x0 0x04>;
+			#index-cells = <1>;
+		};
+
 		pcie0: pcie@4c300000 {
 			compatible = "fsl,imx95-pcie";
 			reg = <0 0x4c300000 0 0x10000>,
-- 
2.34.1

Re: [PATCH 5/6] arm64: dts: imx95: add USB2.0 nodes

[Frank Li]

On Wed, Feb 19, 2025 at 05:31:03PM +0800, Xu Yang wrote:
> Add USB2.0 controller and phy nodes.
>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx95.dtsi | 30 ++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
> index 0b64a1df3c7a..a7c87b9843bd 100644
> --- a/arch/arm64/boot/dts/freescale/imx95.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
> @@ -463,6 +463,13 @@ its: msi-controller@48040000 {
>  		};
>  	};
>
> +	usbphynop: usbphynop {
> +		compatible = "usb-nop-xceiv";
> +		clocks = <&scmi_clk IMX95_CLK_HSIO>;
> +		clock-names = "main_clk";
> +		#phy-cells = <0>;
> +	};
> +
>  	soc {
>  		compatible = "simple-bus";
>  		#address-cells = <2>;
> @@ -1579,6 +1586,29 @@ usb3_phy: phy@4c1f0040 {
>  			status = "disabled";
>  		};
>
> +		usb2: usb@4c200000 {
> +			compatible = "fsl,imx95-usb", "fsl,imx7d-usb", "fsl,imx27-usb";
> +			reg = <0x0 0x4c200000 0x0 0x200>;
> +			interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&scmi_clk IMX95_CLK_HSIO>,
> +				 <&scmi_clk IMX95_CLK_32K>;
> +			clock-names = "usb_ctrl_root", "usb_wakeup";
> +			iommus = <&smmu 0xf>;
> +			phys = <&usbphynop>;
> +			power-domains = <&scmi_devpd IMX95_PD_HSIO_TOP>;
> +			fsl,usbmisc = <&usbmisc 0>;
> +			status = "disabled";
> +		};
> +
> +		usbmisc: usbmisc@4c200200 {
> +			compatible = "fsl,imx95-usbmisc", "fsl,imx7d-usbmisc",
> +				     "fsl,imx6q-usbmisc";
> +			reg = <0x0 0x4c200200 0x0 0x200>,
> +			      <0x0 0x4c010014 0x0 0x04>;
> +			#index-cells = <1>;
> +		};
> +
>  		pcie0: pcie@4c300000 {
>  			compatible = "fsl,imx95-pcie";
>  			reg = <0 0x4c300000 0 0x10000>,
> --
> 2.34.1
>

[PATCH 6/6] arm64: dts: imx95-19x19-evk: enable USB2.0 node

[Xu Yang]

On this board, USB2.0 is a host-only port, add vbus regulator node
and enable USB2.0 node.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 .../arm64/boot/dts/freescale/imx95-19x19-evk.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
index 25ac331f0318..0505cfe2778f 100644
--- a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
@@ -133,6 +133,15 @@ reg_slot_pwr: regulator-slot-pwr {
 		enable-active-high;
 	};
 
+	reg_usb_vbus: regulator-vbus {
+		compatible = "regulator-fixed";
+		regulator-name = "USB_VBUS";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&i2c7_pcal6524 3 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
 	reg_usdhc2_vmmc: regulator-usdhc2 {
 		compatible = "regulator-fixed";
 		pinctrl-names = "default";
@@ -461,6 +470,13 @@ &sai3 {
 	status = "okay";
 };
 
+&usb2 {
+	dr_mode = "host";
+	disable-over-current;
+	vbus-supply = <&reg_usb_vbus>;
+	status = "okay";
+};
+
 &usb3 {
 	status = "okay";
 };
-- 
2.34.1

Re: [PATCH 6/6] arm64: dts: imx95-19x19-evk: enable USB2.0 node

[Frank Li]

On Wed, Feb 19, 2025 at 05:31:04PM +0800, Xu Yang wrote:
> On this board, USB2.0 is a host-only port, add vbus regulator node
> and enable USB2.0 node.
>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  .../arm64/boot/dts/freescale/imx95-19x19-evk.dts | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
> index 25ac331f0318..0505cfe2778f 100644
> --- a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
> @@ -133,6 +133,15 @@ reg_slot_pwr: regulator-slot-pwr {
>  		enable-active-high;
>  	};
>
> +	reg_usb_vbus: regulator-vbus {
> +		compatible = "regulator-fixed";
> +		regulator-name = "USB_VBUS";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpio = <&i2c7_pcal6524 3 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +

try alphabet order, regulator-vbus should after regulator-usdhc2

Reviewed-by: Frank Li <Frank.Li@nxp.com>


>  	reg_usdhc2_vmmc: regulator-usdhc2 {
>  		compatible = "regulator-fixed";
>  		pinctrl-names = "default";
> @@ -461,6 +470,13 @@ &sai3 {
>  	status = "okay";
>  };
>
> +&usb2 {
> +	dr_mode = "host";
> +	disable-over-current;
> +	vbus-supply = <&reg_usb_vbus>;
> +	status = "okay";
> +};
> +
>  &usb3 {
>  	status = "okay";
>  };
> --
> 2.34.1
>

Re: [PATCH 6/6] arm64: dts: imx95-19x19-evk: enable USB2.0 node

[Xu Yang]

On Wed, Feb 19, 2025 at 03:39:08PM -0500, Frank Li wrote:
> On Wed, Feb 19, 2025 at 05:31:04PM +0800, Xu Yang wrote:
> > On this board, USB2.0 is a host-only port, add vbus regulator node
> > and enable USB2.0 node.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  .../arm64/boot/dts/freescale/imx95-19x19-evk.dts | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
> > index 25ac331f0318..0505cfe2778f 100644
> > --- a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
> > @@ -133,6 +133,15 @@ reg_slot_pwr: regulator-slot-pwr {
> >  		enable-active-high;
> >  	};
> >
> > +	reg_usb_vbus: regulator-vbus {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "USB_VBUS";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		gpio = <&i2c7_pcal6524 3 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +	};
> > +
> 
> try alphabet order, regulator-vbus should after regulator-usdhc2

Okay, will do.

Thanks,
Xu Yang

> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
> 
> >  	reg_usdhc2_vmmc: regulator-usdhc2 {
> >  		compatible = "regulator-fixed";
> >  		pinctrl-names = "default";
> > @@ -461,6 +470,13 @@ &sai3 {
> >  	status = "okay";
> >  };
> >
> > +&usb2 {
> > +	dr_mode = "host";
> > +	disable-over-current;
> > +	vbus-supply = <&reg_usb_vbus>;
> > +	status = "okay";
> > +};
> > +
> >  &usb3 {
> >  	status = "okay";
> >  };
> > --
> > 2.34.1
> >