[PATCH v3 0/4] usb: chipidea: imx: add HSIC support

[PATCH v3 0/4] usb: chipidea: imx: add HSIC support

[PETER CHEN]

Most of NXP (Freescale) i.mx USB part has HSIC support, in this series,
we add support for them, it should cover all imx6 and imx7d SoC.

@Schrempf, since some code logic have changed, please help to re-test
v3, thanks.

Changes for v3:
- Delete the internal API ci_ehci_override_wakeup_flag, and just use
  register read/write APIs. [Patch 1/4, 3/4]
- Using dedicated API usbmisc_imx6_hsic_get_reg_offset to calculate
  offset for HSIC controller. [Patch 2/4]
- Improve the comments for binding-doc, and add the dts example [Patch 4/4]

Changes for v2:
- Compile error reported by kbuild robot [Patch 2/4]
- Comment from Frieder Schrempf about code structure [Patch 2/4]
- Comment from Fabio about adding example for pinctrl [Patch 4/4]

Peter Chen (4):
  usb: chipidea: add flag for imx hsic implementation
  usb: chipidea: imx: add HSIC support
  usb: chipidea: host: override ehci->hub_control
  doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

 .../devicetree/bindings/usb/ci-hdrc-usb2.txt       |   8 +-
 drivers/usb/chipidea/ci_hdrc_imx.c                 | 140 ++++++++++++++++++---
 drivers/usb/chipidea/ci_hdrc_imx.h                 |   9 +-
 drivers/usb/chipidea/host.c                        |  93 ++++++++++++++
 drivers/usb/chipidea/usbmisc_imx.c                 | 140 +++++++++++++++++++++
 include/linux/usb/chipidea.h                       |   3 +
 6 files changed, 373 insertions(+), 20 deletions(-)

-- 
2.14.1

[PATCH v3 1/4] usb: chipidea: add flag for imx hsic implementation

[PETER CHEN]

NXP (Freecale) imx HSIC design has some special requirements, add
some flags at host code to handle them.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/chipidea/host.c  | 17 +++++++++++++++++
 include/linux/usb/chipidea.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index d858a82c4f44..028a3574266a 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -170,6 +170,11 @@ static int host_start(struct ci_hdrc *ci)
 			otg->host = &hcd->self;
 			hcd->self.otg_port = 1;
 		}
+
+		if (ci->platdata->notify_event &&
+			(ci->platdata->flags & CI_HDRC_IMX_IS_HSIC))
+			ci->platdata->notify_event
+				(ci, CI_HDRC_IMX_HSIC_ACTIVE_EVENT);
 	}
 
 	return ret;
@@ -218,6 +223,8 @@ void ci_hdrc_host_destroy(struct ci_hdrc *ci)
 static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
 {
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	struct device *dev = hcd->self.controller;
+	struct ci_hdrc *ci = dev_get_drvdata(dev);
 	int port;
 	u32 tmp;
 
@@ -249,6 +256,16 @@ static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
 			 * It needs a short delay between set RS bit and PHCD.
 			 */
 			usleep_range(150, 200);
+			/*
+			 * Need to clear WKCN and WKOC for imx HSIC,
+			 * otherwise, there will be wakeup event.
+			 */
+			if (ci->platdata->flags & CI_HDRC_IMX_IS_HSIC) {
+				tmp = ehci_readl(ehci, reg);
+				tmp &= ~(PORT_WKDISC_E | PORT_WKCONN_E);
+				ehci_writel(ehci, tmp, reg);
+			}
+
 			break;
 		}
 	}
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 63758c399e4e..911e05af671e 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -60,9 +60,12 @@ struct ci_hdrc_platform_data {
 #define CI_HDRC_OVERRIDE_RX_BURST	BIT(11)
 #define CI_HDRC_OVERRIDE_PHY_CONTROL	BIT(12) /* Glue layer manages phy */
 #define CI_HDRC_REQUIRES_ALIGNED_DMA	BIT(13)
+#define CI_HDRC_IMX_IS_HSIC		BIT(14)
 	enum usb_dr_mode	dr_mode;
 #define CI_HDRC_CONTROLLER_RESET_EVENT		0
 #define CI_HDRC_CONTROLLER_STOPPED_EVENT	1
+#define CI_HDRC_IMX_HSIC_ACTIVE_EVENT		2
+#define CI_HDRC_IMX_HSIC_SUSPEND_EVENT		3
 	int	(*notify_event) (struct ci_hdrc *ci, unsigned event);
 	struct regulator	*reg_vbus;
 	struct usb_otg_caps	ci_otg_caps;
-- 
2.14.1

Re: [PATCH v3 1/4] usb: chipidea: add flag for imx hsic implementation

[Schrempf Frieder]

On 27.11.18 10:30, PETER CHEN wrote:
> NXP (Freecale) imx HSIC design has some special requirements, add
> some flags at host code to handle them.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>

Tested on i.MX6S with Microchip LAN9730 USB HSIC Ethernet Controller.

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

> ---
>   drivers/usb/chipidea/host.c  | 17 +++++++++++++++++
>   include/linux/usb/chipidea.h |  3 +++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index d858a82c4f44..028a3574266a 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -170,6 +170,11 @@ static int host_start(struct ci_hdrc *ci)
>   			otg->host = &hcd->self;
>   			hcd->self.otg_port = 1;
>   		}
> +
> +		if (ci->platdata->notify_event &&
> +			(ci->platdata->flags & CI_HDRC_IMX_IS_HSIC))
> +			ci->platdata->notify_event
> +				(ci, CI_HDRC_IMX_HSIC_ACTIVE_EVENT);
>   	}
>   
>   	return ret;
> @@ -218,6 +223,8 @@ void ci_hdrc_host_destroy(struct ci_hdrc *ci)
>   static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
>   {
>   	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> +	struct device *dev = hcd->self.controller;
> +	struct ci_hdrc *ci = dev_get_drvdata(dev);
>   	int port;
>   	u32 tmp;
>   
> @@ -249,6 +256,16 @@ static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
>   			 * It needs a short delay between set RS bit and PHCD.
>   			 */
>   			usleep_range(150, 200);
> +			/*
> +			 * Need to clear WKCN and WKOC for imx HSIC,
> +			 * otherwise, there will be wakeup event.
> +			 */
> +			if (ci->platdata->flags & CI_HDRC_IMX_IS_HSIC) {
> +				tmp = ehci_readl(ehci, reg);
> +				tmp &= ~(PORT_WKDISC_E | PORT_WKCONN_E);
> +				ehci_writel(ehci, tmp, reg);
> +			}
> +
>   			break;
>   		}
>   	}
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index 63758c399e4e..911e05af671e 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -60,9 +60,12 @@ struct ci_hdrc_platform_data {
>   #define CI_HDRC_OVERRIDE_RX_BURST	BIT(11)
>   #define CI_HDRC_OVERRIDE_PHY_CONTROL	BIT(12) /* Glue layer manages phy */
>   #define CI_HDRC_REQUIRES_ALIGNED_DMA	BIT(13)
> +#define CI_HDRC_IMX_IS_HSIC		BIT(14)
>   	enum usb_dr_mode	dr_mode;
>   #define CI_HDRC_CONTROLLER_RESET_EVENT		0
>   #define CI_HDRC_CONTROLLER_STOPPED_EVENT	1
> +#define CI_HDRC_IMX_HSIC_ACTIVE_EVENT		2
> +#define CI_HDRC_IMX_HSIC_SUSPEND_EVENT		3
>   	int	(*notify_event) (struct ci_hdrc *ci, unsigned event);
>   	struct regulator	*reg_vbus;
>   	struct usb_otg_caps	ci_otg_caps;
> 

[PATCH v3 2/4] usb: chipidea: imx: add HSIC support

[PETER CHEN]

To support imx HSIC, there are some special requirement:
- The HSIC pad is 1.2v, it may need to supply from external
- The data/strobe pin needs to be pulled down first, and after
  host mode is initialized, the strobe pin needs to be pulled up
- During the USB suspend/resume, special setting is needed

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.c | 140 ++++++++++++++++++++++++++++++++-----
 drivers/usb/chipidea/ci_hdrc_imx.h |   9 ++-
 drivers/usb/chipidea/usbmisc_imx.c | 140 +++++++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 09b37c0d075d..56781c329db0 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -14,6 +14,7 @@
 #include <linux/usb/chipidea.h>
 #include <linux/usb/of.h>
 #include <linux/clk.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "ci.h"
 #include "ci_hdrc_imx.h"
@@ -85,6 +86,9 @@ struct ci_hdrc_imx_data {
 	bool supports_runtime_pm;
 	bool override_phy_control;
 	bool in_lpm;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_hsic_active;
+	struct regulator *hsic_pad_regulator;
 	/* SoC before i.mx6 (except imx23/imx28) needs three clks */
 	bool need_three_clks;
 	struct clk *clk_ipg;
@@ -245,19 +249,49 @@ static void imx_disable_unprepare_clks(struct device *dev)
 	}
 }
 
+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:
+		ret = pinctrl_select_state(data->pinctrl,
+				data->pinctrl_hsic_active);
+		if (ret)
+			dev_err(dev, "hsic_active select failed, err=%d\n",
+				ret);
+		break;
+	case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
+		ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
+		if (ret)
+			dev_err(dev,
+				"hsic_set_connect failed, err=%d\n", ret);
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
 static int ci_hdrc_imx_probe(struct platform_device *pdev)
 {
 	struct ci_hdrc_imx_data *data;
 	struct ci_hdrc_platform_data pdata = {
 		.name		= dev_name(&pdev->dev),
 		.capoffset	= DEF_CAPOFFSET,
+		.notify_event	= ci_hdrc_imx_notify_event,
 	};
 	int ret;
 	const struct of_device_id *of_id;
 	const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
 	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct pinctrl_state *pinctrl_hsic_idle;
 
-	of_id = of_match_device(ci_hdrc_imx_dt_ids, &pdev->dev);
+	of_id = of_match_device(ci_hdrc_imx_dt_ids, dev);
 	if (!of_id)
 		return -ENODEV;
 
@@ -268,19 +302,73 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, data);
-	data->usbmisc_data = usbmisc_get_init_data(&pdev->dev);
+	data->usbmisc_data = usbmisc_get_init_data(dev);
 	if (IS_ERR(data->usbmisc_data))
 		return PTR_ERR(data->usbmisc_data);
 
-	ret = imx_get_clks(&pdev->dev);
+	if (of_usb_get_phy_mode(dev->of_node) == USBPHY_INTERFACE_MODE_HSIC) {
+		pdata.flags |= CI_HDRC_IMX_IS_HSIC;
+		data->usbmisc_data->hsic = 1;
+		data->pinctrl = devm_pinctrl_get(dev);
+		if (IS_ERR(data->pinctrl)) {
+			dev_err(dev, "pinctrl get failed, 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,
+				"pinctrl_hsic_idle lookup failed, 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, "hsic_idle select failed, 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,
+				"pinctrl_hsic_active lookup failed, err=%ld\n",
+					PTR_ERR(data->pinctrl_hsic_active));
+			return PTR_ERR(data->pinctrl_hsic_active);
+		}
+
+		data->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
+		if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
+			return -EPROBE_DEFER;
+		} else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
+			/* no pad regualator is needed */
+			data->hsic_pad_regulator = NULL;
+		} else if (IS_ERR(data->hsic_pad_regulator)) {
+			dev_err(dev, "Get HSIC pad regulator error: %ld\n",
+					PTR_ERR(data->hsic_pad_regulator));
+			return PTR_ERR(data->hsic_pad_regulator);
+		}
+
+		if (data->hsic_pad_regulator) {
+			ret = regulator_enable(data->hsic_pad_regulator);
+			if (ret) {
+				dev_err(dev,
+					"Failed to enable HSIC pad regulator\n");
+				return ret;
+			}
+		}
+	}
+	ret = imx_get_clks(dev);
 	if (ret)
-		return ret;
+		goto disable_hsic_regulator;
 
-	ret = imx_prepare_enable_clks(&pdev->dev);
+	ret = imx_prepare_enable_clks(dev);
 	if (ret)
-		return ret;
+		goto disable_hsic_regulator;
 
-	data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);
+	data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
 	if (IS_ERR(data->phy)) {
 		ret = PTR_ERR(data->phy);
 		/* Return -EINVAL if no usbphy is available */
@@ -305,40 +393,43 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 
 	ret = imx_usbmisc_init(data->usbmisc_data);
 	if (ret) {
-		dev_err(&pdev->dev, "usbmisc init failed, ret=%d\n", ret);
+		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
 		goto err_clk;
 	}
 
-	data->ci_pdev = ci_hdrc_add_device(&pdev->dev,
+	data->ci_pdev = ci_hdrc_add_device(dev,
 				pdev->resource, pdev->num_resources,
 				&pdata);
 	if (IS_ERR(data->ci_pdev)) {
 		ret = PTR_ERR(data->ci_pdev);
 		if (ret != -EPROBE_DEFER)
-			dev_err(&pdev->dev,
-				"ci_hdrc_add_device failed, err=%d\n", ret);
+			dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
+					ret);
 		goto err_clk;
 	}
 
 	ret = imx_usbmisc_init_post(data->usbmisc_data);
 	if (ret) {
-		dev_err(&pdev->dev, "usbmisc post failed, ret=%d\n", ret);
+		dev_err(dev, "usbmisc post failed, ret=%d\n", ret);
 		goto disable_device;
 	}
 
 	if (data->supports_runtime_pm) {
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
 	}
 
-	device_set_wakeup_capable(&pdev->dev, true);
+	device_set_wakeup_capable(dev, true);
 
 	return 0;
 
 disable_device:
 	ci_hdrc_remove_device(data->ci_pdev);
 err_clk:
-	imx_disable_unprepare_clks(&pdev->dev);
+	imx_disable_unprepare_clks(dev);
+disable_hsic_regulator:
+	if (data->hsic_pad_regulator)
+		ret = regulator_disable(data->hsic_pad_regulator);
 	return ret;
 }
 
@@ -355,6 +446,8 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
 	if (data->override_phy_control)
 		usb_phy_shutdown(data->phy);
 	imx_disable_unprepare_clks(&pdev->dev);
+	if (data->hsic_pad_regulator)
+		regulator_disable(data->hsic_pad_regulator);
 
 	return 0;
 }
@@ -367,9 +460,16 @@ static void ci_hdrc_imx_shutdown(struct platform_device *pdev)
 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__);
 
+	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 +500,16 @@ static int __maybe_unused imx_controller_resume(struct device *dev)
 		goto clk_disable;
 	}
 
+	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..43a15a6e86f5 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)
 	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,79 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data *data)
 	return 0;
 }
 
+static int usbmisc_imx6_hsic_get_reg_offset(struct imx_usbmisc_data *data)
+{
+	int offset, ret = 0;
+
+	if (data->index == 2 || data->index == 3) {
+		offset = (data->index - 2) * 4;
+	} else if (data->index == 0) {
+		/*
+		 * For SoCs like i.MX7D and later, each USB controller has
+		 * its own non-core register region. For SoCs before i.MX7D,
+		 * the first two USB controllers are non-HSIC controllers.
+		 */
+		offset = 0;
+	} else {
+		dev_err(data->dev, "index is error for usbmisc\n");
+		ret = -EINVAL;
+	}
+
+	return ret ? ret : offset;
+}
+
+static int usbmisc_imx6_hsic_set_connect(struct imx_usbmisc_data *data)
+{
+	unsigned long flags;
+	u32 val;
+	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
+	int offset;
+
+	spin_lock_irqsave(&usbmisc->lock, flags);
+	offset = usbmisc_imx6_hsic_get_reg_offset(data);
+	if (offset < 0) {
+		spin_unlock_irqrestore(&usbmisc->lock, flags);
+		return offset;
+	}
+
+	val = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET + offset);
+	if (!(val & MX6_BM_HSIC_DEV_CONN))
+		writel(val | MX6_BM_HSIC_DEV_CONN,
+			usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET + offset);
+
+	spin_unlock_irqrestore(&usbmisc->lock, flags);
+
+	return 0;
+}
+
+static int usbmisc_imx6_hsic_set_clk(struct imx_usbmisc_data *data, bool on)
+{
+	unsigned long flags;
+	u32 val;
+	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
+	int offset;
+
+	spin_lock_irqsave(&usbmisc->lock, flags);
+	offset = usbmisc_imx6_hsic_get_reg_offset(data);
+	if (offset < 0) {
+		spin_unlock_irqrestore(&usbmisc->lock, flags);
+		return offset;
+	}
+
+	val = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET + offset);
+	val |= MX6_BM_HSIC_EN | MX6_BM_HSIC_CLK_ON;
+	if (on)
+		val |= MX6_BM_HSIC_CLK_ON;
+	else
+		val &= ~MX6_BM_HSIC_CLK_ON;
+
+	writel(val, usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET + offset);
+	spin_unlock_irqrestore(&usbmisc->lock, flags);
+
+	return 0;
+}
+
+
 static int usbmisc_imx6sx_init(struct imx_usbmisc_data *data)
 {
 	void __iomem *reg = NULL;
@@ -385,6 +486,13 @@ static int usbmisc_imx6sx_init(struct imx_usbmisc_data *data)
 		spin_unlock_irqrestore(&usbmisc->lock, flags);
 	}
 
+	/* For HSIC controller */
+	if (data->hsic) {
+		val = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET);
+		val |= MX6SX_BM_HSIC_AUTO_RESUME;
+		writel(val, usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET);
+	}
+
 	return 0;
 }
 
@@ -454,6 +562,7 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data *data)
 	reg &= ~MX7D_USB_VBUS_WAKEUP_SOURCE_MASK;
 	writel(reg | MX7D_USB_VBUS_WAKEUP_SOURCE_BVALID,
 		 usbmisc->base + MX7D_USBNC_USB_CTRL2);
+
 	spin_unlock_irqrestore(&usbmisc->lock, flags);
 
 	usbmisc_imx7d_set_wakeup(data, false);
@@ -481,6 +590,8 @@ static const struct usbmisc_ops imx53_usbmisc_ops = {
 static const struct usbmisc_ops imx6q_usbmisc_ops = {
 	.set_wakeup = usbmisc_imx6q_set_wakeup,
 	.init = usbmisc_imx6q_init,
+	.hsic_set_connect = usbmisc_imx6_hsic_set_connect,
+	.hsic_set_clk   = usbmisc_imx6_hsic_set_clk,
 };
 
 static const struct usbmisc_ops vf610_usbmisc_ops = {
@@ -490,6 +601,8 @@ static const struct usbmisc_ops vf610_usbmisc_ops = {
 static const struct usbmisc_ops imx6sx_usbmisc_ops = {
 	.set_wakeup = usbmisc_imx6q_set_wakeup,
 	.init = usbmisc_imx6sx_init,
+	.hsic_set_connect = usbmisc_imx6_hsic_set_connect,
+	.hsic_set_clk = usbmisc_imx6_hsic_set_clk,
 };
 
 static const struct usbmisc_ops imx7d_usbmisc_ops = {
@@ -546,6 +659,33 @@ int imx_usbmisc_set_wakeup(struct imx_usbmisc_data *data, bool enabled)
 }
 EXPORT_SYMBOL_GPL(imx_usbmisc_set_wakeup);
 
+int imx_usbmisc_hsic_set_connect(struct imx_usbmisc_data *data)
+{
+	struct imx_usbmisc *usbmisc;
+
+	if (!data)
+		return 0;
+
+	usbmisc = dev_get_drvdata(data->dev);
+	if (!usbmisc->ops->hsic_set_connect || !data->hsic)
+		return 0;
+	return usbmisc->ops->hsic_set_connect(data);
+}
+EXPORT_SYMBOL_GPL(imx_usbmisc_hsic_set_connect);
+
+int imx_usbmisc_hsic_set_clk(struct imx_usbmisc_data *data, bool on)
+{
+	struct imx_usbmisc *usbmisc;
+
+	if (!data)
+		return 0;
+
+	usbmisc = dev_get_drvdata(data->dev);
+	if (!usbmisc->ops->hsic_set_clk || !data->hsic)
+		return 0;
+	return usbmisc->ops->hsic_set_clk(data, on);
+}
+EXPORT_SYMBOL_GPL(imx_usbmisc_hsic_set_clk);
 static const struct of_device_id usbmisc_imx_dt_ids[] = {
 	{
 		.compatible = "fsl,imx25-usbmisc",
-- 
2.14.1

Re: [PATCH v3 2/4] usb: chipidea: imx: add HSIC support

[Schrempf Frieder]

On 27.11.18 10:30, PETER CHEN wrote:
> To support imx HSIC, there are some special requirement:
> - The HSIC pad is 1.2v, it may need to supply from external
> - The data/strobe pin needs to be pulled down first, and after
>    host mode is initialized, the strobe pin needs to be pulled up
> - During the USB suspend/resume, special setting is needed
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>

Tested on i.MX6S with Microchip LAN9730 USB HSIC Ethernet Controller.

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

> ---
>   drivers/usb/chipidea/ci_hdrc_imx.c | 140 ++++++++++++++++++++++++++++++++-----
>   drivers/usb/chipidea/ci_hdrc_imx.h |   9 ++-
>   drivers/usb/chipidea/usbmisc_imx.c | 140 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 270 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 09b37c0d075d..56781c329db0 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -14,6 +14,7 @@
>   #include <linux/usb/chipidea.h>
>   #include <linux/usb/of.h>
>   #include <linux/clk.h>
> +#include <linux/pinctrl/consumer.h>
>   
>   #include "ci.h"
>   #include "ci_hdrc_imx.h"
> @@ -85,6 +86,9 @@ struct ci_hdrc_imx_data {
>   	bool supports_runtime_pm;
>   	bool override_phy_control;
>   	bool in_lpm;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pinctrl_hsic_active;
> +	struct regulator *hsic_pad_regulator;
>   	/* SoC before i.mx6 (except imx23/imx28) needs three clks */
>   	bool need_three_clks;
>   	struct clk *clk_ipg;
> @@ -245,19 +249,49 @@ static void imx_disable_unprepare_clks(struct device *dev)
>   	}
>   }
>   
> +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:
> +		ret = pinctrl_select_state(data->pinctrl,
> +				data->pinctrl_hsic_active);
> +		if (ret)
> +			dev_err(dev, "hsic_active select failed, err=%d\n",
> +				ret);
> +		break;
> +	case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
> +		ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
> +		if (ret)
> +			dev_err(dev,
> +				"hsic_set_connect failed, err=%d\n", ret);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>   static int ci_hdrc_imx_probe(struct platform_device *pdev)
>   {
>   	struct ci_hdrc_imx_data *data;
>   	struct ci_hdrc_platform_data pdata = {
>   		.name		= dev_name(&pdev->dev),
>   		.capoffset	= DEF_CAPOFFSET,
> +		.notify_event	= ci_hdrc_imx_notify_event,
>   	};
>   	int ret;
>   	const struct of_device_id *of_id;
>   	const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
>   	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct pinctrl_state *pinctrl_hsic_idle;
>   
> -	of_id = of_match_device(ci_hdrc_imx_dt_ids, &pdev->dev);
> +	of_id = of_match_device(ci_hdrc_imx_dt_ids, dev);
>   	if (!of_id)
>   		return -ENODEV;
>   
> @@ -268,19 +302,73 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	platform_set_drvdata(pdev, data);
> -	data->usbmisc_data = usbmisc_get_init_data(&pdev->dev);
> +	data->usbmisc_data = usbmisc_get_init_data(dev);
>   	if (IS_ERR(data->usbmisc_data))
>   		return PTR_ERR(data->usbmisc_data);
>   
> -	ret = imx_get_clks(&pdev->dev);
> +	if (of_usb_get_phy_mode(dev->of_node) == USBPHY_INTERFACE_MODE_HSIC) {
> +		pdata.flags |= CI_HDRC_IMX_IS_HSIC;
> +		data->usbmisc_data->hsic = 1;
> +		data->pinctrl = devm_pinctrl_get(dev);
> +		if (IS_ERR(data->pinctrl)) {
> +			dev_err(dev, "pinctrl get failed, 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,
> +				"pinctrl_hsic_idle lookup failed, 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, "hsic_idle select failed, 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,
> +				"pinctrl_hsic_active lookup failed, err=%ld\n",
> +					PTR_ERR(data->pinctrl_hsic_active));
> +			return PTR_ERR(data->pinctrl_hsic_active);
> +		}
> +
> +		data->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
> +		if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
> +			return -EPROBE_DEFER;
> +		} else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
> +			/* no pad regualator is needed */
> +			data->hsic_pad_regulator = NULL;
> +		} else if (IS_ERR(data->hsic_pad_regulator)) {
> +			dev_err(dev, "Get HSIC pad regulator error: %ld\n",
> +					PTR_ERR(data->hsic_pad_regulator));
> +			return PTR_ERR(data->hsic_pad_regulator);
> +		}
> +
> +		if (data->hsic_pad_regulator) {
> +			ret = regulator_enable(data->hsic_pad_regulator);
> +			if (ret) {
> +				dev_err(dev,
> +					"Failed to enable HSIC pad regulator\n");
> +				return ret;
> +			}
> +		}
> +	}
> +	ret = imx_get_clks(dev);
>   	if (ret)
> -		return ret;
> +		goto disable_hsic_regulator;
>   
> -	ret = imx_prepare_enable_clks(&pdev->dev);
> +	ret = imx_prepare_enable_clks(dev);
>   	if (ret)
> -		return ret;
> +		goto disable_hsic_regulator;
>   
> -	data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);
> +	data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
>   	if (IS_ERR(data->phy)) {
>   		ret = PTR_ERR(data->phy);
>   		/* Return -EINVAL if no usbphy is available */
> @@ -305,40 +393,43 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>   
>   	ret = imx_usbmisc_init(data->usbmisc_data);
>   	if (ret) {
> -		dev_err(&pdev->dev, "usbmisc init failed, ret=%d\n", ret);
> +		dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
>   		goto err_clk;
>   	}
>   
> -	data->ci_pdev = ci_hdrc_add_device(&pdev->dev,
> +	data->ci_pdev = ci_hdrc_add_device(dev,
>   				pdev->resource, pdev->num_resources,
>   				&pdata);
>   	if (IS_ERR(data->ci_pdev)) {
>   		ret = PTR_ERR(data->ci_pdev);
>   		if (ret != -EPROBE_DEFER)
> -			dev_err(&pdev->dev,
> -				"ci_hdrc_add_device failed, err=%d\n", ret);
> +			dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
> +					ret);
>   		goto err_clk;
>   	}
>   
>   	ret = imx_usbmisc_init_post(data->usbmisc_data);
>   	if (ret) {
> -		dev_err(&pdev->dev, "usbmisc post failed, ret=%d\n", ret);
> +		dev_err(dev, "usbmisc post failed, ret=%d\n", ret);
>   		goto disable_device;
>   	}
>   
>   	if (data->supports_runtime_pm) {
> -		pm_runtime_set_active(&pdev->dev);
> -		pm_runtime_enable(&pdev->dev);
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);
>   	}
>   
> -	device_set_wakeup_capable(&pdev->dev, true);
> +	device_set_wakeup_capable(dev, true);
>   
>   	return 0;
>   
>   disable_device:
>   	ci_hdrc_remove_device(data->ci_pdev);
>   err_clk:
> -	imx_disable_unprepare_clks(&pdev->dev);
> +	imx_disable_unprepare_clks(dev);
> +disable_hsic_regulator:
> +	if (data->hsic_pad_regulator)
> +		ret = regulator_disable(data->hsic_pad_regulator);
>   	return ret;
>   }
>   
> @@ -355,6 +446,8 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
>   	if (data->override_phy_control)
>   		usb_phy_shutdown(data->phy);
>   	imx_disable_unprepare_clks(&pdev->dev);
> +	if (data->hsic_pad_regulator)
> +		regulator_disable(data->hsic_pad_regulator);
>   
>   	return 0;
>   }
> @@ -367,9 +460,16 @@ static void ci_hdrc_imx_shutdown(struct platform_device *pdev)
>   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__);
>   
> +	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 +500,16 @@ static int __maybe_unused imx_controller_resume(struct device *dev)
>   		goto clk_disable;
>   	}
>   
> +	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..43a15a6e86f5 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)
>   	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,79 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data *data)
>   	return 0;
>   }
>   
> +static int usbmisc_imx6_hsic_get_reg_offset(struct imx_usbmisc_data *data)
> +{
> +	int offset, ret = 0;
> +
> +	if (data->index == 2 || data->index == 3) {
> +		offset = (data->index - 2) * 4;
> +	} else if (data->index == 0) {
> +		/*
> +		 * For SoCs like i.MX7D and later, each USB controller has
> +		 * its own non-core register region. For SoCs before i.MX7D,
> +		 * the first two USB controllers are non-HSIC controllers.
> +		 */
> +		offset = 0;
> +	} else {
> +		dev_err(data->dev, "index is error for usbmisc\n");
> +		ret = -EINVAL;
> +	}
> +
> +	return ret ? ret : offset;
> +}
> +
> +static int usbmisc_imx6_hsic_set_connect(struct imx_usbmisc_data *data)
> +{
> +	unsigned long flags;
> +	u32 val;
> +	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
> +	int offset;
> +
> +	spin_lock_irqsave(&usbmisc->lock, flags);
> +	offset = usbmisc_imx6_hsic_get_reg_offset(data);
> +	if (offset < 0) {
> +		spin_unlock_irqrestore(&usbmisc->lock, flags);
> +		return offset;
> +	}
> +
> +	val = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET + offset);
> +	if (!(val & MX6_BM_HSIC_DEV_CONN))
> +		writel(val | MX6_BM_HSIC_DEV_CONN,
> +			usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET + offset);
> +
> +	spin_unlock_irqrestore(&usbmisc->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int usbmisc_imx6_hsic_set_clk(struct imx_usbmisc_data *data, bool on)
> +{
> +	unsigned long flags;
> +	u32 val;
> +	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
> +	int offset;
> +
> +	spin_lock_irqsave(&usbmisc->lock, flags);
> +	offset = usbmisc_imx6_hsic_get_reg_offset(data);
> +	if (offset < 0) {
> +		spin_unlock_irqrestore(&usbmisc->lock, flags);
> +		return offset;
> +	}
> +
> +	val = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET + offset);
> +	val |= MX6_BM_HSIC_EN | MX6_BM_HSIC_CLK_ON;
> +	if (on)
> +		val |= MX6_BM_HSIC_CLK_ON;
> +	else
> +		val &= ~MX6_BM_HSIC_CLK_ON;
> +
> +	writel(val, usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET + offset);
> +	spin_unlock_irqrestore(&usbmisc->lock, flags);
> +
> +	return 0;
> +}
> +
> +
>   static int usbmisc_imx6sx_init(struct imx_usbmisc_data *data)
>   {
>   	void __iomem *reg = NULL;
> @@ -385,6 +486,13 @@ static int usbmisc_imx6sx_init(struct imx_usbmisc_data *data)
>   		spin_unlock_irqrestore(&usbmisc->lock, flags);
>   	}
>   
> +	/* For HSIC controller */
> +	if (data->hsic) {
> +		val = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET);
> +		val |= MX6SX_BM_HSIC_AUTO_RESUME;
> +		writel(val, usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET);
> +	}
> +
>   	return 0;
>   }
>   
> @@ -454,6 +562,7 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data *data)
>   	reg &= ~MX7D_USB_VBUS_WAKEUP_SOURCE_MASK;
>   	writel(reg | MX7D_USB_VBUS_WAKEUP_SOURCE_BVALID,
>   		 usbmisc->base + MX7D_USBNC_USB_CTRL2);
> +
>   	spin_unlock_irqrestore(&usbmisc->lock, flags);
>   
>   	usbmisc_imx7d_set_wakeup(data, false);
> @@ -481,6 +590,8 @@ static const struct usbmisc_ops imx53_usbmisc_ops = {
>   static const struct usbmisc_ops imx6q_usbmisc_ops = {
>   	.set_wakeup = usbmisc_imx6q_set_wakeup,
>   	.init = usbmisc_imx6q_init,
> +	.hsic_set_connect = usbmisc_imx6_hsic_set_connect,
> +	.hsic_set_clk   = usbmisc_imx6_hsic_set_clk,
>   };
>   
>   static const struct usbmisc_ops vf610_usbmisc_ops = {
> @@ -490,6 +601,8 @@ static const struct usbmisc_ops vf610_usbmisc_ops = {
>   static const struct usbmisc_ops imx6sx_usbmisc_ops = {
>   	.set_wakeup = usbmisc_imx6q_set_wakeup,
>   	.init = usbmisc_imx6sx_init,
> +	.hsic_set_connect = usbmisc_imx6_hsic_set_connect,
> +	.hsic_set_clk = usbmisc_imx6_hsic_set_clk,
>   };
>   
>   static const struct usbmisc_ops imx7d_usbmisc_ops = {
> @@ -546,6 +659,33 @@ int imx_usbmisc_set_wakeup(struct imx_usbmisc_data *data, bool enabled)
>   }
>   EXPORT_SYMBOL_GPL(imx_usbmisc_set_wakeup);
>   
> +int imx_usbmisc_hsic_set_connect(struct imx_usbmisc_data *data)
> +{
> +	struct imx_usbmisc *usbmisc;
> +
> +	if (!data)
> +		return 0;
> +
> +	usbmisc = dev_get_drvdata(data->dev);
> +	if (!usbmisc->ops->hsic_set_connect || !data->hsic)
> +		return 0;
> +	return usbmisc->ops->hsic_set_connect(data);
> +}
> +EXPORT_SYMBOL_GPL(imx_usbmisc_hsic_set_connect);
> +
> +int imx_usbmisc_hsic_set_clk(struct imx_usbmisc_data *data, bool on)
> +{
> +	struct imx_usbmisc *usbmisc;
> +
> +	if (!data)
> +		return 0;
> +
> +	usbmisc = dev_get_drvdata(data->dev);
> +	if (!usbmisc->ops->hsic_set_clk || !data->hsic)
> +		return 0;
> +	return usbmisc->ops->hsic_set_clk(data, on);
> +}
> +EXPORT_SYMBOL_GPL(imx_usbmisc_hsic_set_clk);
>   static const struct of_device_id usbmisc_imx_dt_ids[] = {
>   	{
>   		.compatible = "fsl,imx25-usbmisc",
> 

[PATCH v3 3/4] usb: chipidea: host: override ehci->hub_control

[PETER CHEN]

The chipidea controller has some special requirements during
suspend/resume, override common ehci->hub_control to implement
it.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/chipidea/host.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 028a3574266a..aa4756dab487 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -220,6 +220,81 @@ void ci_hdrc_host_destroy(struct ci_hdrc *ci)
 		host_stop(ci);
 }
 
+/* The below code is based on tegra ehci driver */
+static int ci_ehci_hub_control(
+	struct usb_hcd	*hcd,
+	u16		typeReq,
+	u16		wValue,
+	u16		wIndex,
+	char		*buf,
+	u16		wLength
+)
+{
+	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
+	u32 __iomem	*status_reg;
+	u32		temp;
+	unsigned long	flags;
+	int		retval = 0;
+	struct device *dev = hcd->self.controller;
+	struct ci_hdrc *ci = dev_get_drvdata(dev);
+
+	status_reg = &ehci->regs->port_status[(wIndex & 0xff) - 1];
+
+	spin_lock_irqsave(&ehci->lock, flags);
+
+	if (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_SUSPEND) {
+		temp = ehci_readl(ehci, status_reg);
+		if ((temp & PORT_PE) == 0 || (temp & PORT_RESET) != 0) {
+			retval = -EPIPE;
+			goto done;
+		}
+
+		temp &= ~(PORT_RWC_BITS | PORT_WKCONN_E);
+		temp |= PORT_WKDISC_E | PORT_WKOC_E;
+		ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
+
+		/*
+		 * If a transaction is in progress, there may be a delay in
+		 * suspending the port. Poll until the port is suspended.
+		 */
+		if (ehci_handshake(ehci, status_reg, PORT_SUSPEND,
+						PORT_SUSPEND, 5000))
+			ehci_err(ehci, "timeout waiting for SUSPEND\n");
+
+		if (ci->platdata->flags & CI_HDRC_IMX_IS_HSIC) {
+			if (ci->platdata->notify_event)
+				ci->platdata->notify_event
+					(ci, CI_HDRC_IMX_HSIC_SUSPEND_EVENT);
+
+			temp = ehci_readl(ehci, status_reg);
+			temp &= ~(PORT_WKDISC_E | PORT_WKCONN_E);
+			ehci_writel(ehci, temp, status_reg);
+		}
+
+		set_bit((wIndex & 0xff) - 1, &ehci->suspended_ports);
+		goto done;
+	}
+
+	/*
+	 * After resume has finished, it needs do some post resume
+	 * operation for some SoCs.
+	 */
+	else if (typeReq == ClearPortFeature &&
+					wValue == USB_PORT_FEAT_C_SUSPEND) {
+
+		/* Make sure the resume has finished, it should be finished */
+		if (ehci_handshake(ehci, status_reg, PORT_RESUME, 0, 25000))
+			ehci_err(ehci, "timeout waiting for resume\n");
+	}
+
+	spin_unlock_irqrestore(&ehci->lock, flags);
+
+	/* Handle the hub control events here */
+	return ehci_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
+done:
+	spin_unlock_irqrestore(&ehci->lock, flags);
+	return retval;
+}
 static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
 {
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
@@ -298,4 +373,5 @@ void ci_hdrc_host_driver_init(void)
 	ehci_init_driver(&ci_ehci_hc_driver, &ehci_ci_overrides);
 	orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
 	ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;
+	ci_ehci_hc_driver.hub_control = ci_ehci_hub_control;
 }
-- 
2.14.1

Re: [PATCH v3 3/4] usb: chipidea: host: override ehci->hub_control

[Schrempf Frieder]

On 27.11.18 10:30, PETER CHEN wrote:
> The chipidea controller has some special requirements during
> suspend/resume, override common ehci->hub_control to implement
> it.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>

Tested on i.MX6S with Microchip LAN9730 USB HSIC Ethernet Controller.
I have some minor format fixes below. Apart from that:

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

> ---
>   drivers/usb/chipidea/host.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 028a3574266a..aa4756dab487 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -220,6 +220,81 @@ void ci_hdrc_host_destroy(struct ci_hdrc *ci)
>   		host_stop(ci);
>   }
>   
> +/* The below code is based on tegra ehci driver */
> +static int ci_ehci_hub_control(
> +	struct usb_hcd	*hcd,
> +	u16		typeReq,
> +	u16		wValue,
> +	u16		wIndex,
> +	char		*buf,
> +	u16		wLength
> +)
> +{
> +	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
> +	u32 __iomem	*status_reg;
> +	u32		temp;
> +	unsigned long	flags;
> +	int		retval = 0;
> +	struct device *dev = hcd->self.controller;
> +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> +
> +	status_reg = &ehci->regs->port_status[(wIndex & 0xff) - 1];
> +
> +	spin_lock_irqsave(&ehci->lock, flags);
> +
> +	if (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_SUSPEND) {
> +		temp = ehci_readl(ehci, status_reg);
> +		if ((temp & PORT_PE) == 0 || (temp & PORT_RESET) != 0) {
> +			retval = -EPIPE;
> +			goto done;
> +		}
> +
> +		temp &= ~(PORT_RWC_BITS | PORT_WKCONN_E);
> +		temp |= PORT_WKDISC_E | PORT_WKOC_E;
> +		ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
> +
> +		/*
> +		 * If a transaction is in progress, there may be a delay in
> +		 * suspending the port. Poll until the port is suspended.
> +		 */
> +		if (ehci_handshake(ehci, status_reg, PORT_SUSPEND,
> +						PORT_SUSPEND, 5000))

Indentation:                       ^            ^

> +			ehci_err(ehci, "timeout waiting for SUSPEND\n");
> +
> +		if (ci->platdata->flags & CI_HDRC_IMX_IS_HSIC) {
> +			if (ci->platdata->notify_event)
> +				ci->platdata->notify_event
> +					(ci, CI_HDRC_IMX_HSIC_SUSPEND_EVENT);

I think the open parentheses and the first parameter should be on the 
first line:

				ci->platdata->notify_event(ci,
					CI_HDRC_IMX_HSIC_SUSPEND_EVENT);

> +
> +			temp = ehci_readl(ehci, status_reg);
> +			temp &= ~(PORT_WKDISC_E | PORT_WKCONN_E);
> +			ehci_writel(ehci, temp, status_reg);
> +		}
> +
> +		set_bit((wIndex & 0xff) - 1, &ehci->suspended_ports);
> +		goto done;
> +	}
> +
> +	/*
> +	 * After resume has finished, it needs do some post resume
> +	 * operation for some SoCs.
> +	 */
> +	else if (typeReq == ClearPortFeature &&
> +					wValue == USB_PORT_FEAT_C_SUSPEND) {

Indentation:     ^                      ^

> +
> +		/* Make sure the resume has finished, it should be finished */
> +		if (ehci_handshake(ehci, status_reg, PORT_RESUME, 0, 25000))
> +			ehci_err(ehci, "timeout waiting for resume\n");
> +	}
> +
> +	spin_unlock_irqrestore(&ehci->lock, flags);
> +
> +	/* Handle the hub control events here */
> +	return ehci_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
> +done:
> +	spin_unlock_irqrestore(&ehci->lock, flags);
> +	return retval;
> +}
>   static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
>   {
>   	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> @@ -298,4 +373,5 @@ void ci_hdrc_host_driver_init(void)
>   	ehci_init_driver(&ci_ehci_hc_driver, &ehci_ci_overrides);
>   	orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
>   	ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;
> +	ci_ehci_hc_driver.hub_control = ci_ehci_hub_control;
>   }
> 

RE: [PATCH v3 3/4] usb: chipidea: host: override ehci->hub_control

[PETER CHEN]

 
> On 27.11.18 10:30, PETER CHEN wrote:
> > The chipidea controller has some special requirements during
> > suspend/resume, override common ehci->hub_control to implement it.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> 
> Tested on i.MX6S with Microchip LAN9730 USB HSIC Ethernet Controller.
> I have some minor format fixes below. Apart from that:
> 
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> > ---
> >   drivers/usb/chipidea/host.c | 76
> +++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 76 insertions(+)
> >
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index 028a3574266a..aa4756dab487 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -220,6 +220,81 @@ void ci_hdrc_host_destroy(struct ci_hdrc *ci)
> >   		host_stop(ci);
> >   }
> >
> > +/* The below code is based on tegra ehci driver */ static int
> > +ci_ehci_hub_control(
> > +	struct usb_hcd	*hcd,
> > +	u16		typeReq,
> > +	u16		wValue,
> > +	u16		wIndex,
> > +	char		*buf,
> > +	u16		wLength
> > +)
> > +{
> > +	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
> > +	u32 __iomem	*status_reg;
> > +	u32		temp;
> > +	unsigned long	flags;
> > +	int		retval = 0;
> > +	struct device *dev = hcd->self.controller;
> > +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > +
> > +	status_reg = &ehci->regs->port_status[(wIndex & 0xff) - 1];
> > +
> > +	spin_lock_irqsave(&ehci->lock, flags);
> > +
> > +	if (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_SUSPEND)
> {
> > +		temp = ehci_readl(ehci, status_reg);
> > +		if ((temp & PORT_PE) == 0 || (temp & PORT_RESET) != 0) {
> > +			retval = -EPIPE;
> > +			goto done;
> > +		}
> > +
> > +		temp &= ~(PORT_RWC_BITS | PORT_WKCONN_E);
> > +		temp |= PORT_WKDISC_E | PORT_WKOC_E;
> > +		ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
> > +
> > +		/*
> > +		 * If a transaction is in progress, there may be a delay in
> > +		 * suspending the port. Poll until the port is suspended.
> > +		 */
> > +		if (ehci_handshake(ehci, status_reg, PORT_SUSPEND,
> > +						PORT_SUSPEND, 5000))
> 
> Indentation:                       ^            ^
> 
> > +			ehci_err(ehci, "timeout waiting for SUSPEND\n");
> > +
> > +		if (ci->platdata->flags & CI_HDRC_IMX_IS_HSIC) {
> > +			if (ci->platdata->notify_event)
> > +				ci->platdata->notify_event
> > +					(ci, CI_HDRC_IMX_HSIC_SUSPEND_EVENT);
> 
> I think the open parentheses and the first parameter should be on the first line:
> 
> 				ci->platdata->notify_event(ci,
> 					CI_HDRC_IMX_HSIC_SUSPEND_EVENT);
> 
> > +
> > +			temp = ehci_readl(ehci, status_reg);
> > +			temp &= ~(PORT_WKDISC_E | PORT_WKCONN_E);
> > +			ehci_writel(ehci, temp, status_reg);
> > +		}
> > +
> > +		set_bit((wIndex & 0xff) - 1, &ehci->suspended_ports);
> > +		goto done;
> > +	}
> > +
> > +	/*
> > +	 * After resume has finished, it needs do some post resume
> > +	 * operation for some SoCs.
> > +	 */
> > +	else if (typeReq == ClearPortFeature &&
> > +					wValue == USB_PORT_FEAT_C_SUSPEND)
> {
> 
> Indentation:     ^                      ^
> 
 
Thanks, I will change these code style comments for next revision.

Peter

[PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[PETER CHEN]

For USB HSIC, the data and strobe pin needs to be pulled down
at default, we consider it as "idle" state. When the USB host
is ready to be used, the strobe pin needs to be pulled up,
we consider it as "active" state.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 529e51879fb2..1e5e7ddfb1a5 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -80,7 +80,10 @@ Optional properties:
   controller. It's expected that a mux state of 0 indicates device mode and a
   mux state of 1 indicates host mode.
 - mux-control-names: Shall be "usb_switch" if mux-controls is specified.
-- pinctrl-names: Names for optional pin modes in "default", "host", "device"
+- pinctrl-names: Names for optional pin modes in "default", "host", "device".
+  In case of HSIC-mode, "idle" and "active" pin modes are mandatory. In this
+  case, the "idle" state needs to pull down the data and strobe pin
+  and the "active" state needs to pull up the strobe pin.
 - pinctrl-n: alternate pin modes
 
 i.mx specific properties
@@ -110,4 +113,7 @@ Example:
 		phy-clkgate-delay-us = <400>;
 		mux-controls = <&usb_switch>;
 		mux-control-names = "usb_switch";
+		pinctrl-names = "idle", "active";
+		pinctrl-0 = <&pinctrl_usbh2_1>;
+		pinctrl-1 = <&pinctrl_usbh2_2>;
 	};
-- 
2.14.1

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Fabio Estevam]

Hi Peter,

On Tue, Nov 27, 2018 at 7:31 AM PETER CHEN <peter.chen@nxp.com> wrote:
>
> For USB HSIC, the data and strobe pin needs to be pulled down
> at default, we consider it as "idle" state. When the USB host
> is ready to be used, the strobe pin needs to be pulled up,
> we consider it as "active" state.
>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 529e51879fb2..1e5e7ddfb1a5 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -80,7 +80,10 @@ Optional properties:
>    controller. It's expected that a mux state of 0 indicates device mode and a
>    mux state of 1 indicates host mode.
>  - mux-control-names: Shall be "usb_switch" if mux-controls is specified.
> -- pinctrl-names: Names for optional pin modes in "default", "host", "device"
> +- pinctrl-names: Names for optional pin modes in "default", "host", "device".
> +  In case of HSIC-mode, "idle" and "active" pin modes are mandatory. In this
> +  case, the "idle" state needs to pull down the data and strobe pin
> +  and the "active" state needs to pull up the strobe pin.
>  - pinctrl-n: alternate pin modes
>
>  i.mx specific properties
> @@ -110,4 +113,7 @@ Example:
>                 phy-clkgate-delay-us = <400>;
>                 mux-controls = <&usb_switch>;
>                 mux-control-names = "usb_switch";
> +               pinctrl-names = "idle", "active";
> +               pinctrl-0 = <&pinctrl_usbh2_1>;
> +               pinctrl-1 = <&pinctrl_usbh2_2>;

I would put the "idle" and "active" entries inside a explicit section for HSIC.

Otherwise, it may confuse people using non-HSIC bindings.

RE: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[PETER CHEN]

 
> 
> On Tue, Nov 27, 2018 at 7:31 AM PETER CHEN <peter.chen@nxp.com> wrote:
> >
> > For USB HSIC, the data and strobe pin needs to be pulled down at
> > default, we consider it as "idle" state. When the USB host is ready to
> > be used, the strobe pin needs to be pulled up, we consider it as
> > "active" state.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > index 529e51879fb2..1e5e7ddfb1a5 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > @@ -80,7 +80,10 @@ Optional properties:
> >    controller. It's expected that a mux state of 0 indicates device mode and a
> >    mux state of 1 indicates host mode.
> >  - mux-control-names: Shall be "usb_switch" if mux-controls is specified.
> > -- pinctrl-names: Names for optional pin modes in "default", "host", "device"
> > +- pinctrl-names: Names for optional pin modes in "default", "host", "device".
> > +  In case of HSIC-mode, "idle" and "active" pin modes are mandatory.
> > +In this
> > +  case, the "idle" state needs to pull down the data and strobe pin
> > +  and the "active" state needs to pull up the strobe pin.
> >  - pinctrl-n: alternate pin modes
> >
> >  i.mx specific properties
> > @@ -110,4 +113,7 @@ Example:
> >                 phy-clkgate-delay-us = <400>;
> >                 mux-controls = <&usb_switch>;
> >                 mux-control-names = "usb_switch";
> > +               pinctrl-names = "idle", "active";
> > +               pinctrl-0 = <&pinctrl_usbh2_1>;
> > +               pinctrl-1 = <&pinctrl_usbh2_2>;
> 
> I would put the "idle" and "active" entries inside a explicit section for HSIC.
> 
> Otherwise, it may confuse people using non-HSIC bindings.

It is just an example; the user should check the meaning for optional properties before
using it, like the property "phy-clkgate-delay-us", only imx7d/7s needs it.

Peter

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Schrempf Frieder]

Hi Peter, hi Fabio,

On 30.11.18 03:33, PETER CHEN wrote:
>   
>>
>> On Tue, Nov 27, 2018 at 7:31 AM PETER CHEN <peter.chen@nxp.com> wrote:
>>>
>>> For USB HSIC, the data and strobe pin needs to be pulled down at
>>> default, we consider it as "idle" state. When the USB host is ready to
>>> be used, the strobe pin needs to be pulled up, we consider it as
>>> "active" state.
>>>
>>> Signed-off-by: Peter Chen <peter.chen@nxp.com>
>>> ---
>>>   Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> index 529e51879fb2..1e5e7ddfb1a5 100644
>>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> @@ -80,7 +80,10 @@ Optional properties:
>>>     controller. It's expected that a mux state of 0 indicates device mode and a
>>>     mux state of 1 indicates host mode.
>>>   - mux-control-names: Shall be "usb_switch" if mux-controls is specified.
>>> -- pinctrl-names: Names for optional pin modes in "default", "host", "device"
>>> +- pinctrl-names: Names for optional pin modes in "default", "host", "device".
>>> +  In case of HSIC-mode, "idle" and "active" pin modes are mandatory.
>>> +In this
>>> +  case, the "idle" state needs to pull down the data and strobe pin
>>> +  and the "active" state needs to pull up the strobe pin.
>>>   - pinctrl-n: alternate pin modes
>>>
>>>   i.mx specific properties
>>> @@ -110,4 +113,7 @@ Example:
>>>                  phy-clkgate-delay-us = <400>;
>>>                  mux-controls = <&usb_switch>;
>>>                  mux-control-names = "usb_switch";
>>> +               pinctrl-names = "idle", "active";
>>> +               pinctrl-0 = <&pinctrl_usbh2_1>;
>>> +               pinctrl-1 = <&pinctrl_usbh2_2>;
>>
>> I would put the "idle" and "active" entries inside a explicit section for HSIC.
>>
>> Otherwise, it may confuse people using non-HSIC bindings.
> 
> It is just an example; the user should check the meaning for optional properties before
> using it, like the property "phy-clkgate-delay-us", only imx7d/7s needs it.

There are many other optional properties for this driver and a lot of 
them are not in the given example. Maybe we should just keep the 
pinctrls for HSIC-mode out of the example, too?

Regards,
Frieder

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Fabio Estevam]

Hi Frieder,

On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
<frieder.schrempf@kontron.de> wrote:

> There are many other optional properties for this driver and a lot of
> them are not in the given example. Maybe we should just keep the
> pinctrls for HSIC-mode out of the example, too?

I am just trying to make life easier for those who want to use HSIC
support with chipidea.

Can we just add a real dts snippet example of your board into the
binding document?

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Schrempf Frieder]

Hi Fabio,

On 04.12.18 21:01, Fabio Estevam wrote:
> Hi Frieder,
> 
> On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
> <frieder.schrempf@kontron.de> wrote:
> 
>> There are many other optional properties for this driver and a lot of
>> them are not in the given example. Maybe we should just keep the
>> pinctrls for HSIC-mode out of the example, too?
> 
> I am just trying to make life easier for those who want to use HSIC
> support with chipidea.
> 
> Can we just add a real dts snippet example of your board into the
> binding document?

Sure, here is what I have in my dts:

&usbh2 {
	pinctrl-names = "idle", "active";
	pinctrl-0 = <&pinctrl_usbh2_idle>;
	pinctrl-1 = <&pinctrl_usbh2_active>;
	status = "okay";
	#address-cells = <1>;
	#size-cells = <0>;

	usbnet: smsc@1 {
		compatible = "usb424,9730";
		reg = <1>;
	};
};

@Peter: Can you add this as a second example to the binding documentation?

Thanks,
Frieder

RE: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[PETER CHEN]

 
> On 04.12.18 21:01, Fabio Estevam wrote:
> > Hi Frieder,
> >
> > On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
> > <frieder.schrempf@kontron.de> wrote:
> >
> >> There are many other optional properties for this driver and a lot of
> >> them are not in the given example. Maybe we should just keep the
> >> pinctrls for HSIC-mode out of the example, too?
> >
> > I am just trying to make life easier for those who want to use HSIC
> > support with chipidea.
> >
> > Can we just add a real dts snippet example of your board into the
> > binding document?
> 
> Sure, here is what I have in my dts:
> 
> &usbh2 {
> 	pinctrl-names = "idle", "active";
> 	pinctrl-0 = <&pinctrl_usbh2_idle>;
> 	pinctrl-1 = <&pinctrl_usbh2_active>;
> 	status = "okay";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	usbnet: smsc@1 {
> 		compatible = "usb424,9730";
> 		reg = <1>;
> 	};
> };
> 
> @Peter: Can you add this as a second example to the binding documentation?
> 

So, there are two examples at binding-doc, one for normal, one for HSIC? Fabio, do you
mean that? If DT maintainer agrees it too, I will add it.

Peter

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Fabio Estevam]

On Wed, Dec 5, 2018 at 5:57 AM PETER CHEN <peter.chen@nxp.com> wrote:

> So, there are two examples at binding-doc, one for normal, one for HSIC? Fabio, do you
> mean that? If DT maintainer agrees it too, I will add it.

Yes, I think it will makes things clearer. Thanks

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Rob Herring]

On Wed, Dec 05, 2018 at 07:57:37AM +0000, PETER CHEN wrote:
>  
> > On 04.12.18 21:01, Fabio Estevam wrote:
> > > Hi Frieder,
> > >
> > > On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
> > > <frieder.schrempf@kontron.de> wrote:
> > >
> > >> There are many other optional properties for this driver and a lot of
> > >> them are not in the given example. Maybe we should just keep the
> > >> pinctrls for HSIC-mode out of the example, too?
> > >
> > > I am just trying to make life easier for those who want to use HSIC
> > > support with chipidea.
> > >
> > > Can we just add a real dts snippet example of your board into the
> > > binding document?
> > 
> > Sure, here is what I have in my dts:
> > 
> > &usbh2 {
> > 	pinctrl-names = "idle", "active";
> > 	pinctrl-0 = <&pinctrl_usbh2_idle>;
> > 	pinctrl-1 = <&pinctrl_usbh2_active>;
> > 	status = "okay";
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 
> > 	usbnet: smsc@1 {
> > 		compatible = "usb424,9730";
> > 		reg = <1>;
> > 	};
> > };
> > 
> > @Peter: Can you add this as a second example to the binding documentation?
> > 
> 
> So, there are two examples at binding-doc, one for normal, one for HSIC? Fabio, do you
> mean that? If DT maintainer agrees it too, I will add it.

Okay.

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Schrempf Frieder]

On 05.12.18 08:45, Schrempf Frieder wrote:
> Hi Fabio,
> 
> On 04.12.18 21:01, Fabio Estevam wrote:
>> Hi Frieder,
>>
>> On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
>> <frieder.schrempf@kontron.de> wrote:
>>
>>> There are many other optional properties for this driver and a lot of
>>> them are not in the given example. Maybe we should just keep the
>>> pinctrls for HSIC-mode out of the example, too?
>>
>> I am just trying to make life easier for those who want to use HSIC
>> support with chipidea.
>>
>> Can we just add a real dts snippet example of your board into the
>> binding document?
> 
> Sure, here is what I have in my dts:
> 
> &usbh2 {
> 	pinctrl-names = "idle", "active";
> 	pinctrl-0 = <&pinctrl_usbh2_idle>;
> 	pinctrl-1 = <&pinctrl_usbh2_active>;
> 	status = "okay";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	usbnet: smsc@1 {
> 		compatible = "usb424,9730";
> 		reg = <1>;
> 	};
> };

Or merged with the settings from imx6qdl.dtsi this will look like below. 
Maybe this is better as it is the complete node without depending on 
imx6qdl.dtsi:

usbh2: usb@2184400 {
	compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
	reg = <0x02184400 0x200>;
	interrupts = <0 41 IRQ_TYPE_LEVEL_HIGH>;
	clocks = <&clks IMX6QDL_CLK_USBOH3>;
	fsl,usbphy = <&usbphynop1>;
	fsl,usbmisc = <&usbmisc 2>;
	phy_type = "hsic";
	dr_mode = "host";
	ahb-burst-config = <0x0>;
	tx-burst-size-dword = <0x10>;
	rx-burst-size-dword = <0x10>;
	pinctrl-names = "idle", "active";
  	pinctrl-0 = <&pinctrl_usbh2_idle>;
  	pinctrl-1 = <&pinctrl_usbh2_active>;
	#address-cells = <1>;
  	#size-cells = <0>;

  	usbnet: smsc@1 {
  		compatible = "usb424,9730";
  		reg = <1>;
  	};
};

> 
> @Peter: Can you add this as a second example to the binding documentation?
> 
> Thanks,
> Frieder
>