quoted

+static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int
+event) {
+	struct device *dev = ci->dev->parent;
+	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
+	int ret = 0;
+
+	switch (event) {
+	case CI_HDRC_IMX_HSIC_ACTIVE_EVENT:
+		if (!IS_ERR(data->pinctrl) &&
+			!IS_ERR(data->pinctrl_hsic_active)) {

If we make the pinctrl mandatory in case of HSIC as proposed below, we don't need
the checks here.

Will delete them

 

quoted

+
+		data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
+								"active");
+		if (IS_ERR(data->pinctrl_hsic_active))
+			dev_dbg(dev,
+				"pinctrl_hsic_active lookup failed, err=%ld\n",
+					PTR_ERR(data->pinctrl_hsic_active));
+	}

In the paragraph above, I think we should make the pinctrl settings mandatory in
case of HSIC and error out if one of them is missing.

Also I think we could make the code more readable by removing the nested
conditions.

Maybe something like this would be better?

if (of_usb_get_phy_mode(dev->of_node) == USBPHY_INTERFACE_MODE_HSIC)
{
	data->pinctrl = devm_pinctrl_get(dev);
	if (IS_ERR(data->pinctrl)) {
		dev_err(dev, "failed to get HSIC pinctrl, err=%ld\n",
			PTR_ERR(data->pinctrl));
		return PTR_ERR(data->pinctrl);
	}

	pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
	if (IS_ERR(pinctrl_hsic_idle)) {
		dev_err(dev, "failed to get HSIC idle pinctrl,"
			     "err=%ld\n", PTR_ERR(pinctrl_hsic_idle));
		return PTR_ERR(pinctrl_hsic_idle);
	}

	ret = pinctrl_select_state(data->pinctrl, pinctrl_hsic_idle);
	if (ret) {
		dev_err(dev, "failed to select HSIC idle pinctrl,"
			     "err=%d\n", ret);
		return ret;
	}

	data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
							 "active");
	if (IS_ERR(data->pinctrl_hsic_active)) {
		dev_err(dev, "failed to get HSIC active pinctrl,"
			     "err=%ld\n",
			     PTR_ERR(data->pinctrl_hsic_active));
		return PTR_ERR(data->pinctrl_hsic_active);
	}
}

Good idea.

 

quoted

  }

@@ -367,9 +470,19 @@ static void ci_hdrc_imx_shutdown(struct

platform_device *pdev)

quoted

  static int __maybe_unused imx_controller_suspend(struct device *dev)
  {
  	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
+	int ret = 0;

  	dev_dbg(dev, "at %s\n", __func__);

+	if (data->usbmisc_data) {

Why do we have this check here, but not in imx_controller_resume()?

I will delete check here since there is NULL point check at imx_usbmisc_hsic_set_clk too.

quoted

+		ret = imx_usbmisc_hsic_set_clk(data->usbmisc_data, false);
+		if (ret) {
+			dev_err(dev,
+				"usbmisc hsic_set_clk failed, ret=%d\n", ret);
+			return ret;
+		}
+	}
+
  	imx_disable_unprepare_clks(dev);
  	data->in_lpm = true;

@@ -400,8 +513,16 @@ static int __maybe_unused

imx_controller_resume(struct device *dev)

quoted

  		goto clk_disable;
  	}

Why don't we have a check for data->usbmisc_data here, but in
imx_controller_suspend() we have one?

Yes, not need.

quoted

+	ret = imx_usbmisc_hsic_set_clk(data->usbmisc_data, true);
+	if (ret) {
+		dev_err(dev, "usbmisc hsic_set_clk failed, ret=%d\n", ret);
+		goto hsic_set_clk_fail;
+	}
+
  	return 0;

+hsic_set_clk_fail:
+	imx_usbmisc_set_wakeup(data->usbmisc_data, true);
  clk_disable:
  	imx_disable_unprepare_clks(dev);
  	return ret;

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h

b/drivers/usb/chipidea/ci_hdrc_imx.h
index 204275f47573..fcecab478934 100644

--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h

@@ -14,10 +14,13 @@ struct imx_usbmisc_data {
  	unsigned int oc_polarity:1; /* over current polarity if oc enabled */
  	unsigned int evdo:1; /* set external vbus divider option */
  	unsigned int ulpi:1; /* connected to an ULPI phy */
+	unsigned int hsic:1; /* HSIC controlller */
  };

-int imx_usbmisc_init(struct imx_usbmisc_data *); -int

imx_usbmisc_init_post(struct imx_usbmisc_data *); -int
imx_usbmisc_set_wakeup(struct imx_usbmisc_data *, bool);
+int imx_usbmisc_init(struct imx_usbmisc_data *data); int
+imx_usbmisc_init_post(struct imx_usbmisc_data *data); int
+imx_usbmisc_set_wakeup(struct imx_usbmisc_data *data, bool enabled);
+int imx_usbmisc_hsic_set_connect(struct imx_usbmisc_data *data); int
+imx_usbmisc_hsic_set_clk(struct imx_usbmisc_data *data, bool on);

  #endif /* __DRIVER_USB_CHIPIDEA_CI_HDRC_IMX_H */ diff --git
a/drivers/usb/chipidea/usbmisc_imx.c
b/drivers/usb/chipidea/usbmisc_imx.c
index def80ff547e4..a66a15075200 100644

--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c

@@ -64,10 +64,22 @@
  #define MX6_BM_OVER_CUR_DIS		BIT(7)
  #define MX6_BM_OVER_CUR_POLARITY	BIT(8)
  #define MX6_BM_WAKEUP_ENABLE		BIT(10)
+#define MX6_BM_UTMI_ON_CLOCK		BIT(13)
  #define MX6_BM_ID_WAKEUP		BIT(16)
  #define MX6_BM_VBUS_WAKEUP		BIT(17)
  #define MX6SX_BM_DPDM_WAKEUP_EN		BIT(29)
  #define MX6_BM_WAKEUP_INTR		BIT(31)
+
+#define MX6_USB_HSIC_CTRL_OFFSET	0x10
+/* Send resume signal without 480Mhz PHY clock */
+#define MX6SX_BM_HSIC_AUTO_RESUME	BIT(23)
+/* set before portsc.suspendM = 1 */
+#define MX6_BM_HSIC_DEV_CONN		BIT(21)
+/* HSIC enable */
+#define MX6_BM_HSIC_EN			BIT(12)
+/* Force HSIC module 480M clock on, even when in Host is in suspend mode */
+#define MX6_BM_HSIC_CLK_ON		BIT(11)
+
  #define MX6_USB_OTG1_PHY_CTRL		0x18
  /* For imx6dql, it is host-only controller, for later imx6, it is otg's */
  #define MX6_USB_OTG2_PHY_CTRL		0x1c

@@ -94,6 +106,10 @@ struct usbmisc_ops {
  	int (*post)(struct imx_usbmisc_data *data);
  	/* It's called when we need to enable/disable usb wakeup */
  	int (*set_wakeup)(struct imx_usbmisc_data *data, bool enabled);
+	/* It's called before setting portsc.suspendM */
+	int (*hsic_set_connect)(struct imx_usbmisc_data *data);
+	/* It's called during suspend/resume */
+	int (*hsic_set_clk)(struct imx_usbmisc_data *data, bool enabled);
  };

  struct imx_usbmisc {

@@ -353,6 +369,18 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data

*data)

quoted

  	writel(reg | MX6_BM_NON_BURST_SETTING,
  			usbmisc->base + data->index * 4);

+	/* For HSIC controller */
+	if (data->hsic) {
+		reg = readl(usbmisc->base + data->index * 4);
+		writel(reg | MX6_BM_UTMI_ON_CLOCK,
+			usbmisc->base + data->index * 4);
+		reg = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET
+			+ (data->index - 2) * 4);
+		reg |= MX6_BM_HSIC_EN | MX6_BM_HSIC_CLK_ON;
+		writel(reg, usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET
+			+ (data->index - 2) * 4);
+	}
+
  	spin_unlock_irqrestore(&usbmisc->lock, flags);

  	usbmisc_imx6q_set_wakeup(data, false); @@ -360,6 +388,70 @@ static
int usbmisc_imx6q_init(struct imx_usbmisc_data *data)
  	return 0;
  }

+static int usbmisc_imx6_hsic_set_connect(struct imx_usbmisc_data
+*data) {
+	unsigned long flags;
+	u32 val, offset;
+	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
+	int ret = 0;
+
+	spin_lock_irqsave(&usbmisc->lock, flags);
+	if (data->index == 2 || data->index == 3) {
+		offset = (data->index - 2) * 4;
+	} else if (data->index == 0) {
+		/*
+		 * For controllers later than imx7d (imx7d is included),

"For SOCs like i.MX7D and later, ..."

quoted

+		 * each controller has its own non core register region.
+		 * And the controllers before than imx7d, the 1st controller
+		 * is not HSIC controller.

"For SOCs before i.MX7D, the first USB controller is non-HSIC."

I will change both.

Thanks.

Peter