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

[PATCH v3 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.

For detailed changes can refer to patch commit log.

v3:
 - fix possible build break
 - remove usbmisc_imx95_init()
 - add Rb tag in some patches

v2:
 - apply some suggestions
 - improve wakeup interrupt handling
 - add Rb tag in some patches

Xu Yang (6):
  dt-bindings: usb: chipidea: Add i.MX95 compatible string
    'fsl,imx95-usb'
  dt-bindings: usb: usbmisc-imx: add support for i.MX95 platform
  usb: chipidea: imx: add wakeup interrupt handling
  usb: chipidea: imx: add HSIO Block Control wakeup 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       | 24 ++++++-
 .../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            | 33 +++++++++
 drivers/usb/chipidea/usbmisc_imx.c            | 72 +++++++++++++++++++
 7 files changed, 199 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v3 1/6] dt-bindings: usb: chipidea: Add i.MX95 compatible string 'fsl,imx95-usb'

[Xu Yang]

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.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v3:
 - add Rb tag
Changes in v2:
 - improve interrupts description
 - improve subject and commit message
---
 .../bindings/usb/chipidea,usb2-common.yaml    |  3 +++
 .../bindings/usb/chipidea,usb2-imx.yaml       | 24 ++++++++++++++++++-
 2 files changed, 26 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..51014955ab3c 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,11 @@ properties:
     maxItems: 1
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: USB controller interrupt or combine USB controller
+                     and wakeup interrupts.
+      - description: Wakeup interrupt
 
   clocks:
     minItems: 1
@@ -191,6 +196,7 @@ allOf:
           contains:
             enum:
               - fsl,imx93-usb
+              - fsl,imx95-usb
     then:
       properties:
         clocks:
@@ -238,6 +244,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

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

[Xu Yang]

Add compatible string "fsl,imx95-usbmisc" for i.MX95 platform and
restriction on reg property.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v3:
 - add Rb tag
Changes in v2:
 - improve subject and add Rb tag
---
 .../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

[PATCH v3 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 wakeup interrupt handling for i.MX95 to support various
wakeup events.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
Changes in v3:
 - include <linux/irq.h> to fix possible build issue
Changes in v2:
 - rename irq to wakeup_irq
 - disable irq by default
 - enable irq when suspend, disable irq when resume
---
 drivers/usb/chipidea/ci_hdrc_imx.c | 33 ++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 1a7fc638213e..535a1476a6cf 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/irq.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
@@ -98,6 +99,7 @@ struct ci_hdrc_imx_data {
 	struct clk *clk;
 	struct clk *clk_wakeup;
 	struct imx_usbmisc_data *usbmisc_data;
+	int wakeup_irq;
 	bool supports_runtime_pm;
 	bool override_phy_control;
 	bool in_lpm;
@@ -336,6 +338,16 @@ 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;
+
+	disable_irq_nosync(irq);
+	pm_runtime_resume(&imx_data->ci_pdev->dev);
+
+	return IRQ_HANDLED;
+}
+
 static int ci_hdrc_imx_probe(struct platform_device *pdev)
 {
 	struct ci_hdrc_imx_data *data;
@@ -476,6 +488,16 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
 		data->supports_runtime_pm = true;
 
+	data->wakeup_irq = platform_get_irq_optional(pdev, 1);
+	if (data->wakeup_irq > 0) {
+		ret = devm_request_threaded_irq(dev, data->wakeup_irq,
+				NULL, ci_wakeup_irq_handler,
+				IRQF_ONESHOT | IRQF_NO_AUTOEN,
+				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);
@@ -584,6 +606,7 @@ static int imx_controller_suspend(struct device *dev,
 	}
 
 	imx_disable_unprepare_clks(dev);
+	enable_irq(data->wakeup_irq);
 	if (data->plat_data->flags & CI_HDRC_PMQOS)
 		cpu_latency_qos_remove_request(&data->pm_qos_req);
 
@@ -608,6 +631,9 @@ static int imx_controller_resume(struct device *dev,
 	if (data->plat_data->flags & CI_HDRC_PMQOS)
 		cpu_latency_qos_add_request(&data->pm_qos_req, 0);
 
+	if (!irqd_irq_disabled(irq_get_irq_data(data->wakeup_irq)))
+		disable_irq_nosync(data->wakeup_irq);
+
 	ret = imx_prepare_enable_clks(dev);
 	if (ret)
 		return ret;
@@ -643,6 +669,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->wakeup_irq);
+
 	return ret;
 }
 
@@ -651,6 +681,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->wakeup_irq);
+
 	pinctrl_pm_select_default_state(dev);
 	ret = imx_controller_resume(dev, PMSG_RESUME);
 	if (!ret && data->supports_runtime_pm) {
-- 
2.34.1

[PATCH v3 4/6] usb: chipidea: imx: add HSIO Block Control wakeup 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 wakeup setting for
HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory, then do
wakeup setting as needs.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v3:
 - remove usbmisc_imx95_init(), use usbmisc_imx7d_init()
Changes in v2:
 - add Rb tag
---
 drivers/usb/chipidea/usbmisc_imx.c | 72 ++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
index 1394881fde5f..8c30908c11ed 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,41 @@ 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;
+
+	/* Select 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 const struct usbmisc_ops imx25_usbmisc_ops = {
 	.init = usbmisc_imx25_init,
 	.post = usbmisc_imx25_post,
@@ -1068,6 +1120,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_imx7d_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 +1349,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 +1360,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 +1372,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 v3 5/6] arm64: dts: imx95: add USB2.0 nodes

[Xu Yang]

Add USB2.0 controller and phy nodes.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v3:
 - no changes
Changes in v2:
 - add Rb tag
---
 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 3af13173de4b..48c2033971bf 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 v3 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.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v3:
 - no changes
Changes in v2:
 - reorder regulator node
---
 .../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..a41d542488ed 100644
--- a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
@@ -145,6 +145,15 @@ reg_usdhc2_vmmc: regulator-usdhc2 {
 		off-on-delay-us = <12000>;
 	};
 
+	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;
+	};
+
 	sound-bt-sco {
 		compatible = "simple-audio-card";
 		simple-audio-card,name = "bt-sco-audio";
@@ -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 v3 4/6] usb: chipidea: imx: add HSIO Block Control wakeup setting

[Alexander Stein]

Hi,

Am Donnerstag, 27. Februar 2025, 10:53:46 CET schrieb 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 wakeup setting for
> HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory, then do
> wakeup setting as needs.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v3:
>  - remove usbmisc_imx95_init(), use usbmisc_imx7d_init()
> Changes in v2:
>  - add Rb tag
> ---
>  drivers/usb/chipidea/usbmisc_imx.c | 72 ++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> index 1394881fde5f..8c30908c11ed 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,41 @@ 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;
> +
> +	/* Select 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);

usbmisc->blkctl is NULL if DT does not provide a 2nd IORESOURCE_MEM.

> +
> +	return 0;
> +}
> +
>  static const struct usbmisc_ops imx25_usbmisc_ops = {
>  	.init = usbmisc_imx25_init,
>  	.post = usbmisc_imx25_post,
> @@ -1068,6 +1120,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_imx7d_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 +1349,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 +1360,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 +1372,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);
> +	}
> +

Any chance to ensure imx95 has actually data->blkctl?

Best regards
Alexander

>  	data->ops = of_device_get_match_data(&pdev->dev);
>  	platform_set_drvdata(pdev, data);
>  
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

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

[Alexander Stein]

Hi,

Am Donnerstag, 27. Februar 2025, 10:53:47 CET schrieb Xu Yang:
> Add USB2.0 controller and phy nodes.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

I was able to test this in peripheral mode on TQMa95xxSA + MB-SMARC-2.

Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> # TQMa95xxSA

> ---
> Changes in v3:
>  - no changes
> Changes in v2:
>  - add Rb tag
> ---
>  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 3af13173de4b..48c2033971bf 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>,
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

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

[Xu Yang]

On Thu, Feb 27, 2025 at 04:12:54PM +0100, Alexander Stein wrote:
> Hi,
> 
> Am Donnerstag, 27. Februar 2025, 10:53:46 CET schrieb 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 wakeup setting for
> > HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory, then do
> > wakeup setting as needs.
> > 
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > Reviewed-by: Jun Li <jun.li@nxp.com>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > 
> > ---
> > Changes in v3:
> >  - remove usbmisc_imx95_init(), use usbmisc_imx7d_init()
> > Changes in v2:
> >  - add Rb tag
> > ---
> >  drivers/usb/chipidea/usbmisc_imx.c | 72 ++++++++++++++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> > 
> > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> > index 1394881fde5f..8c30908c11ed 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,41 @@ 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;
> > +
> > +	/* Select 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);
> 
> usbmisc->blkctl is NULL if DT does not provide a 2nd IORESOURCE_MEM.

Good catch. Thanks!

It may return an errno if usbmisc->blkctl is NULL.

> 
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct usbmisc_ops imx25_usbmisc_ops = {
> >  	.init = usbmisc_imx25_init,
> >  	.post = usbmisc_imx25_post,
> > @@ -1068,6 +1120,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_imx7d_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 +1349,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 +1360,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 +1372,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);
> > +	}
> > +
> 
> Any chance to ensure imx95 has actually data->blkctl?

I think let usbmisc_imx95_set_wakeup() return an error should be enough.
Some errors will be printed for user. Also dt-bindings has restriction for
imx95.

Thanks,
Xu Yang

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

[Alexander Stein]

Hi,

Am Freitag, 28. Februar 2025, 03:33:29 CET schrieb Xu Yang:
> On Thu, Feb 27, 2025 at 04:12:54PM +0100, Alexander Stein wrote:
> > Hi,
> > 
> > Am Donnerstag, 27. Februar 2025, 10:53:46 CET schrieb 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 wakeup setting for
> > > HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory, then do
> > > wakeup setting as needs.
> > > 
> > > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > 
> > > ---
> > > Changes in v3:
> > >  - remove usbmisc_imx95_init(), use usbmisc_imx7d_init()
> > > Changes in v2:
> > >  - add Rb tag
> > > ---
> > >  drivers/usb/chipidea/usbmisc_imx.c | 72 ++++++++++++++++++++++++++++++
> > >  1 file changed, 72 insertions(+)
> > > 
> > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> > > index 1394881fde5f..8c30908c11ed 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,41 @@ 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;
> > > +
> > > +	/* Select 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);
> > 
> > usbmisc->blkctl is NULL if DT does not provide a 2nd IORESOURCE_MEM.
> 
> Good catch. Thanks!
> 
> It may return an errno if usbmisc->blkctl is NULL.
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct usbmisc_ops imx25_usbmisc_ops = {
> > >  	.init = usbmisc_imx25_init,
> > >  	.post = usbmisc_imx25_post,
> > > @@ -1068,6 +1120,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_imx7d_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 +1349,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 +1360,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 +1372,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);
> > > +	}
> > > +
> > 
> > Any chance to ensure imx95 has actually data->blkctl?
> 
> I think let usbmisc_imx95_set_wakeup() return an error should be enough.

Better fail early rather than later.

> Some errors will be printed for user. Also dt-bindings has restriction for
> imx95.

That's true, but don't assume the DT is always setup correctly.

Best regards
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

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

[Xu Yang]

On Fri, Feb 28, 2025 at 08:22:19AM +0100, Alexander Stein wrote:
> Hi,
> 
> Am Freitag, 28. Februar 2025, 03:33:29 CET schrieb Xu Yang:
> > On Thu, Feb 27, 2025 at 04:12:54PM +0100, Alexander Stein wrote:
> > > Hi,
> > > 
> > > Am Donnerstag, 27. Februar 2025, 10:53:46 CET schrieb 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 wakeup setting for
> > > > HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory, then do
> > > > wakeup setting as needs.
> > > > 
> > > > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > 
> > > > ---
> > > > Changes in v3:
> > > >  - remove usbmisc_imx95_init(), use usbmisc_imx7d_init()
> > > > Changes in v2:
> > > >  - add Rb tag
> > > > ---
> > > >  drivers/usb/chipidea/usbmisc_imx.c | 72 ++++++++++++++++++++++++++++++
> > > >  1 file changed, 72 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> > > > index 1394881fde5f..8c30908c11ed 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,41 @@ 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;
> > > > +
> > > > +	/* Select 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);
> > > 
> > > usbmisc->blkctl is NULL if DT does not provide a 2nd IORESOURCE_MEM.
> > 
> > Good catch. Thanks!
> > 
> > It may return an errno if usbmisc->blkctl is NULL.
> > 
> > > 
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static const struct usbmisc_ops imx25_usbmisc_ops = {
> > > >  	.init = usbmisc_imx25_init,
> > > >  	.post = usbmisc_imx25_post,
> > > > @@ -1068,6 +1120,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_imx7d_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 +1349,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 +1360,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 +1372,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);
> > > > +	}
> > > > +
> > > 
> > > Any chance to ensure imx95 has actually data->blkctl?
> > 
> > I think let usbmisc_imx95_set_wakeup() return an error should be enough.
> 
> Better fail early rather than later.
> 
> > Some errors will be printed for user. Also dt-bindings has restriction for
> > imx95.
> 
> That's true, but don't assume the DT is always setup correctly.

Sure, I thought about this question again.

If 2nd IORESOURCE_MEM is not provided, the USB controller could still function
normally in most scenarios except when USB wakeup is required. So if the probe()
return a failure just caused by improper wakeup configurations seems not that
necessary. In this case, ensuring the kernel does not panic should suffice.

Therfore, I prefer to skip wakeup setting if blkctl is NULL now.
Do you have any ideas?

Thanks,
Xu Yang

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

[Alexander Stein]

Am Freitag, 28. Februar 2025, 10:16:49 CET schrieb Xu Yang:
> On Fri, Feb 28, 2025 at 08:22:19AM +0100, Alexander Stein wrote:
> > Hi,
> > 
> > Am Freitag, 28. Februar 2025, 03:33:29 CET schrieb Xu Yang:
> > > On Thu, Feb 27, 2025 at 04:12:54PM +0100, Alexander Stein wrote:
> > > > Hi,
> > > > 
> > > > Am Donnerstag, 27. Februar 2025, 10:53:46 CET schrieb 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 wakeup setting for
> > > > > HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory, then do
> > > > > wakeup setting as needs.
> > > > > 
> > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > > > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > > 
> > > > > ---
> > > > > Changes in v3:
> > > > >  - remove usbmisc_imx95_init(), use usbmisc_imx7d_init()
> > > > > Changes in v2:
> > > > >  - add Rb tag
> > > > > ---
> > > > >  drivers/usb/chipidea/usbmisc_imx.c | 72 ++++++++++++++++++++++++++++++
> > > > >  1 file changed, 72 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> > > > > index 1394881fde5f..8c30908c11ed 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,41 @@ 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;
> > > > > +
> > > > > +	/* Select 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);
> > > > 
> > > > usbmisc->blkctl is NULL if DT does not provide a 2nd IORESOURCE_MEM.
> > > 
> > > Good catch. Thanks!
> > > 
> > > It may return an errno if usbmisc->blkctl is NULL.
> > > 
> > > > 
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static const struct usbmisc_ops imx25_usbmisc_ops = {
> > > > >  	.init = usbmisc_imx25_init,
> > > > >  	.post = usbmisc_imx25_post,
> > > > > @@ -1068,6 +1120,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_imx7d_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 +1349,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 +1360,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 +1372,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);
> > > > > +	}
> > > > > +
> > > > 
> > > > Any chance to ensure imx95 has actually data->blkctl?
> > > 
> > > I think let usbmisc_imx95_set_wakeup() return an error should be enough.
> > 
> > Better fail early rather than later.
> > 
> > > Some errors will be printed for user. Also dt-bindings has restriction for
> > > imx95.
> > 
> > That's true, but don't assume the DT is always setup correctly.
> 
> Sure, I thought about this question again.
> 
> If 2nd IORESOURCE_MEM is not provided, the USB controller could still function
> normally in most scenarios except when USB wakeup is required. So if the probe()
> return a failure just caused by improper wakeup configurations seems not that
> necessary. In this case, ensuring the kernel does not panic should suffice.
> 
> Therfore, I prefer to skip wakeup setting if blkctl is NULL now.
> Do you have any ideas?

That also seems reasonable. So in case 2nd IORESOURCE_MEM is missing (only)
for imx95 there should be a warning that wakeup is not possible/supported.
But the driver itself should work without issue.

Best regards
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

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

[Xu Yang]

On Fri, Feb 28, 2025 at 11:41:42AM +0100, Alexander Stein wrote:
> Am Freitag, 28. Februar 2025, 10:16:49 CET schrieb Xu Yang:
> > On Fri, Feb 28, 2025 at 08:22:19AM +0100, Alexander Stein wrote:
> > > Hi,
> > > 
> > > Am Freitag, 28. Februar 2025, 03:33:29 CET schrieb Xu Yang:
> > > > On Thu, Feb 27, 2025 at 04:12:54PM +0100, Alexander Stein wrote:
> > > > > Hi,
> > > > > 
> > > > > Am Donnerstag, 27. Februar 2025, 10:53:46 CET schrieb 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 wakeup setting for
> > > > > > HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory, then do
> > > > > > wakeup setting as needs.
> > > > > > 
> > > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > > > > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > > > > 
> > > > > > ---
> > > > > > Changes in v3:
> > > > > >  - remove usbmisc_imx95_init(), use usbmisc_imx7d_init()
> > > > > > Changes in v2:
> > > > > >  - add Rb tag
> > > > > > ---
> > > > > >  drivers/usb/chipidea/usbmisc_imx.c | 72 ++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 72 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> > > > > > index 1394881fde5f..8c30908c11ed 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,41 @@ 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;
> > > > > > +
> > > > > > +	/* Select 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);
> > > > > 
> > > > > usbmisc->blkctl is NULL if DT does not provide a 2nd IORESOURCE_MEM.
> > > > 
> > > > Good catch. Thanks!
> > > > 
> > > > It may return an errno if usbmisc->blkctl is NULL.
> > > > 
> > > > > 
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static const struct usbmisc_ops imx25_usbmisc_ops = {
> > > > > >  	.init = usbmisc_imx25_init,
> > > > > >  	.post = usbmisc_imx25_post,
> > > > > > @@ -1068,6 +1120,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_imx7d_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 +1349,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 +1360,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 +1372,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);
> > > > > > +	}
> > > > > > +
> > > > > 
> > > > > Any chance to ensure imx95 has actually data->blkctl?
> > > > 
> > > > I think let usbmisc_imx95_set_wakeup() return an error should be enough.
> > > 
> > > Better fail early rather than later.
> > > 
> > > > Some errors will be printed for user. Also dt-bindings has restriction for
> > > > imx95.
> > > 
> > > That's true, but don't assume the DT is always setup correctly.
> > 
> > Sure, I thought about this question again.
> > 
> > If 2nd IORESOURCE_MEM is not provided, the USB controller could still function
> > normally in most scenarios except when USB wakeup is required. So if the probe()
> > return a failure just caused by improper wakeup configurations seems not that
> > necessary. In this case, ensuring the kernel does not panic should suffice.
> > 
> > Therfore, I prefer to skip wakeup setting if blkctl is NULL now.
> > Do you have any ideas?
> 
> That also seems reasonable. So in case 2nd IORESOURCE_MEM is missing (only)
> for imx95 there should be a warning that wakeup is not possible/supported.
> But the driver itself should work without issue.

Okay. Sure.

Thanks,
Xu Yang