* [PATCH 0/4] usb: chipidea: imx: add HSIC support
@ 2018-10-16 5:01 Peter Chen
2018-10-16 5:01 ` [1/4] " Peter Chen
` (4 more replies)
0 siblings, 5 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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. I have
no HSIC interface board which is supported by upstream kernel, so this
patches are only compiled ok, Frieder Schrempf, would you please
help me test it on your board? Thanks.
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 | 1 +
drivers/usb/chipidea/ci_hdrc_imx.c | 153 ++++++++++++++++++---
drivers/usb/chipidea/ci_hdrc_imx.h | 9 +-
drivers/usb/chipidea/host.c | 98 +++++++++++++
drivers/usb/chipidea/usbmisc_imx.c | 131 ++++++++++++++++++
include/linux/usb/chipidea.h | 3 +
6 files changed, 376 insertions(+), 19 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/4] usb: chipidea: add flag for imx hsic implementation
@ 2018-10-16 5:01 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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 | 24 ++++++++++++++++++++++++
include/linux/usb/chipidea.h | 3 +++
2 files changed, 27 insertions(+)
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index d858a82c4f44..d74a13d7c21c 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -28,6 +28,20 @@ struct ehci_ci_priv {
struct regulator *reg_vbus;
};
+/* This function is used to override WKCN, WKDN, and WKOC */
+static void ci_ehci_override_wakeup_flag(struct ehci_hcd *ehci,
+ u32 __iomem *reg, u32 flags, bool set)
+{
+ u32 val = ehci_readl(ehci, reg);
+
+ if (set)
+ val |= flags;
+ else
+ val &= ~flags;
+
+ ehci_writel(ehci, val, reg);
+}
+
static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
{
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
@@ -170,6 +184,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 +237,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 +270,9 @@ 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);
+ if (ci->platdata->flags & CI_HDRC_IMX_IS_HSIC)
+ ci_ehci_override_wakeup_flag(ehci, reg,
+ PORT_WKDISC_E | PORT_WKCONN_E, false);
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [1/4] usb: chipidea: add flag for imx hsic implementation
@ 2018-10-16 5:01 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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 | 24 ++++++++++++++++++++++++
include/linux/usb/chipidea.h | 3 +++
2 files changed, 27 insertions(+)
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index d858a82c4f44..d74a13d7c21c 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -28,6 +28,20 @@ struct ehci_ci_priv {
struct regulator *reg_vbus;
};
+/* This function is used to override WKCN, WKDN, and WKOC */
+static void ci_ehci_override_wakeup_flag(struct ehci_hcd *ehci,
+ u32 __iomem *reg, u32 flags, bool set)
+{
+ u32 val = ehci_readl(ehci, reg);
+
+ if (set)
+ val |= flags;
+ else
+ val &= ~flags;
+
+ ehci_writel(ehci, val, reg);
+}
+
static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
{
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
@@ -170,6 +184,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 +237,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 +270,9 @@ 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);
+ if (ci->platdata->flags & CI_HDRC_IMX_IS_HSIC)
+ ci_ehci_override_wakeup_flag(ehci, reg,
+ PORT_WKDISC_E | PORT_WKCONN_E, false);
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;
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-16 5:01 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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 | 153 +++++++++++++++++++++++++++++++++----
drivers/usb/chipidea/ci_hdrc_imx.h | 9 ++-
drivers/usb/chipidea/usbmisc_imx.c | 131 +++++++++++++++++++++++++++++++
3 files changed, 274 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 09b37c0d075d..d566771fc77a 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -85,6 +85,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 +248,58 @@ 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:
+ if (!IS_ERR(data->pinctrl) &&
+ !IS_ERR(data->pinctrl_hsic_active)) {
+ ret = pinctrl_select_state(data->pinctrl,
+ data->pinctrl_hsic_active);
+ if (ret)
+ dev_err(dev,
+ "hsic_active select failed, err=%d\n",
+ ret);
+ return ret;
+ }
+ break;
+ case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
+ if (data->usbmisc_data) {
+ ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
+ if (ret)
+ dev_err(dev,
+ "hsic_set_connect failed, err=%d\n",
+ ret);
+ return 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 +310,48 @@ 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);
+ data->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(data->pinctrl)) {
+ dev_dbg(dev, "pinctrl get failed, err=%ld\n",
+ PTR_ERR(data->pinctrl));
+ } else {
+ pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
+ if (IS_ERR(pinctrl_hsic_idle)) {
+ dev_dbg(dev,
+ "pinctrl_hsic_idle lookup failed, err=%ld\n",
+ PTR_ERR(pinctrl_hsic_idle));
+ } else {
+ 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_dbg(dev,
+ "pinctrl_hsic_active lookup failed, err=%ld\n",
+ PTR_ERR(data->pinctrl_hsic_active));
+ }
+
+ ret = imx_get_clks(dev);
if (ret)
return ret;
- ret = imx_prepare_enable_clks(&pdev->dev);
+ ret = imx_prepare_enable_clks(dev);
if (ret)
return ret;
- 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 */
@@ -303,42 +374,72 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
data->supports_runtime_pm = true;
+ 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->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
+ if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto err_clk;
+ } 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));
+ ret = PTR_ERR(data->hsic_pad_regulator);
+ goto err_clk;
+ }
+
+ if (data->hsic_pad_regulator) {
+ ret = regulator_enable(data->hsic_pad_regulator);
+ if (ret) {
+ dev_err(dev,
+ "Fail to enable hsic pad regulator\n");
+ goto err_clk;
+ }
+ }
+ }
+
ret = imx_usbmisc_init(data->usbmisc_data);
if (ret) {
- dev_err(&pdev->dev, "usbmisc init failed, ret=%d\n", ret);
- goto err_clk;
+ dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
+ goto disable_hsic_regulator;
}
- 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);
- goto err_clk;
+ dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
+ ret);
+ goto disable_hsic_regulator;
}
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);
+disable_hsic_regulator:
+ if (data->hsic_pad_regulator)
+ ret = regulator_disable(data->hsic_pad_regulator);
err_clk:
- imx_disable_unprepare_clks(&pdev->dev);
+ imx_disable_unprepare_clks(dev);
return ret;
}
@@ -355,6 +456,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 +470,19 @@ 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__);
+ if (data->usbmisc_data) {
+ 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)
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..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)
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),
+ * each controller has its own non core register region.
+ * And the controllers before than imx7d, the 1st controller
+ * is not HSIC controller.
+ */
+ offset = 0;
+ } else {
+ dev_err(data->dev, "index is error for usbmisc\n");
+ offset = 0;
+ ret = -EINVAL;
+ }
+
+ 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 ret;
+}
+
+static int usbmisc_imx6_hsic_set_clk(struct imx_usbmisc_data *data, bool on)
+{
+ 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) {
+ offset = 0;
+ } else {
+ dev_err(data->dev, "index is error for usbmisc\n");
+ offset = 0;
+ ret = -EINVAL;
+ }
+
+ 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 +477,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 +553,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 +581,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 +592,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 +650,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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-16 5:01 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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 | 153 +++++++++++++++++++++++++++++++++----
drivers/usb/chipidea/ci_hdrc_imx.h | 9 ++-
drivers/usb/chipidea/usbmisc_imx.c | 131 +++++++++++++++++++++++++++++++
3 files changed, 274 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 09b37c0d075d..d566771fc77a 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -85,6 +85,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 +248,58 @@ 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:
+ if (!IS_ERR(data->pinctrl) &&
+ !IS_ERR(data->pinctrl_hsic_active)) {
+ ret = pinctrl_select_state(data->pinctrl,
+ data->pinctrl_hsic_active);
+ if (ret)
+ dev_err(dev,
+ "hsic_active select failed, err=%d\n",
+ ret);
+ return ret;
+ }
+ break;
+ case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
+ if (data->usbmisc_data) {
+ ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
+ if (ret)
+ dev_err(dev,
+ "hsic_set_connect failed, err=%d\n",
+ ret);
+ return 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 +310,48 @@ 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);
+ data->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(data->pinctrl)) {
+ dev_dbg(dev, "pinctrl get failed, err=%ld\n",
+ PTR_ERR(data->pinctrl));
+ } else {
+ pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
+ if (IS_ERR(pinctrl_hsic_idle)) {
+ dev_dbg(dev,
+ "pinctrl_hsic_idle lookup failed, err=%ld\n",
+ PTR_ERR(pinctrl_hsic_idle));
+ } else {
+ 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_dbg(dev,
+ "pinctrl_hsic_active lookup failed, err=%ld\n",
+ PTR_ERR(data->pinctrl_hsic_active));
+ }
+
+ ret = imx_get_clks(dev);
if (ret)
return ret;
- ret = imx_prepare_enable_clks(&pdev->dev);
+ ret = imx_prepare_enable_clks(dev);
if (ret)
return ret;
- 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 */
@@ -303,42 +374,72 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
data->supports_runtime_pm = true;
+ 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->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
+ if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto err_clk;
+ } 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));
+ ret = PTR_ERR(data->hsic_pad_regulator);
+ goto err_clk;
+ }
+
+ if (data->hsic_pad_regulator) {
+ ret = regulator_enable(data->hsic_pad_regulator);
+ if (ret) {
+ dev_err(dev,
+ "Fail to enable hsic pad regulator\n");
+ goto err_clk;
+ }
+ }
+ }
+
ret = imx_usbmisc_init(data->usbmisc_data);
if (ret) {
- dev_err(&pdev->dev, "usbmisc init failed, ret=%d\n", ret);
- goto err_clk;
+ dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
+ goto disable_hsic_regulator;
}
- 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);
- goto err_clk;
+ dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
+ ret);
+ goto disable_hsic_regulator;
}
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);
+disable_hsic_regulator:
+ if (data->hsic_pad_regulator)
+ ret = regulator_disable(data->hsic_pad_regulator);
err_clk:
- imx_disable_unprepare_clks(&pdev->dev);
+ imx_disable_unprepare_clks(dev);
return ret;
}
@@ -355,6 +456,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 +470,19 @@ 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__);
+ if (data->usbmisc_data) {
+ 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)
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..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)
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),
+ * each controller has its own non core register region.
+ * And the controllers before than imx7d, the 1st controller
+ * is not HSIC controller.
+ */
+ offset = 0;
+ } else {
+ dev_err(data->dev, "index is error for usbmisc\n");
+ offset = 0;
+ ret = -EINVAL;
+ }
+
+ 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 ret;
+}
+
+static int usbmisc_imx6_hsic_set_clk(struct imx_usbmisc_data *data, bool on)
+{
+ 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) {
+ offset = 0;
+ } else {
+ dev_err(data->dev, "index is error for usbmisc\n");
+ offset = 0;
+ ret = -EINVAL;
+ }
+
+ 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 +477,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 +553,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 +581,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 +592,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 +650,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",
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 3/4] usb: chipidea: host: override ehci->hub_control
@ 2018-10-16 5:01 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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 | 74 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index d74a13d7c21c..b8e7d7fe3d53 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -234,6 +234,79 @@ 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);
+ ci_ehci_override_wakeup_flag(ehci, status_reg,
+ PORT_WKDISC_E | PORT_WKCONN_E, false);
+ }
+
+ 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);
@@ -305,4 +378,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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [3/4] usb: chipidea: host: override ehci->hub_control
@ 2018-10-16 5:01 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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 | 74 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index d74a13d7c21c..b8e7d7fe3d53 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -234,6 +234,79 @@ 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);
+ ci_ehci_override_wakeup_flag(ehci, status_reg,
+ PORT_WKDISC_E | PORT_WKCONN_E, false);
+ }
+
+ 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);
@@ -305,4 +378,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;
}
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
@ 2018-10-16 5:01 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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 | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 529e51879fb2..10c8d793ea49 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -81,6 +81,7 @@ Optional properties:
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"
+ Or names for HSIC "idle" and "active" pin modes.
- pinctrl-n: alternate pin modes
i.mx specific properties
--
2.14.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
@ 2018-10-16 5:01 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 5:01 UTC (permalink / raw)
To: linux-usb, frieder.schrempf; +Cc: dl-linux-imx, robh+dt, devicetree, 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 | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 529e51879fb2..10c8d793ea49 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -81,6 +81,7 @@ Optional properties:
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"
+ Or names for HSIC "idle" and "active" pin modes.
- pinctrl-n: alternate pin modes
i.mx specific properties
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-16 5:52 ` kbuild test robot
0 siblings, 0 replies; 31+ messages in thread
From: kbuild test robot @ 2018-10-16 5:52 UTC (permalink / raw)
To: Peter Chen
Cc: kbuild-all, linux-usb, frieder.schrempf, dl-linux-imx, robh+dt,
devicetree
[-- Attachment #1: Type: text/plain, Size: 9051 bytes --]
Hi Peter,
I love your patch! Yet something to improve:
[auto build test ERROR on peter.chen-usb/ci-for-usb-next]
[also build test ERROR on v4.19-rc8 next-20181012]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Peter-Chen/usb-chipidea-imx-add-HSIC-support/20181016-130840
base: https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git ci-for-usb-next
config: x86_64-randconfig-x017-201841 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/usb/chipidea/ci_hdrc_imx.c: In function 'ci_hdrc_imx_notify_event':
>> drivers/usb/chipidea/ci_hdrc_imx.c:261:10: error: implicit declaration of function 'pinctrl_select_state'; did you mean 'inc_node_state'? [-Werror=implicit-function-declaration]
ret = pinctrl_select_state(data->pinctrl,
^~~~~~~~~~~~~~~~~~~~
inc_node_state
drivers/usb/chipidea/ci_hdrc_imx.c: In function 'ci_hdrc_imx_probe':
drivers/usb/chipidea/ci_hdrc_imx.c:317:18: error: implicit declaration of function 'devm_pinctrl_get'; did you mean 'devm_clk_get'? [-Werror=implicit-function-declaration]
data->pinctrl = devm_pinctrl_get(dev);
^~~~~~~~~~~~~~~~
devm_clk_get
drivers/usb/chipidea/ci_hdrc_imx.c:317:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
data->pinctrl = devm_pinctrl_get(dev);
^
>> drivers/usb/chipidea/ci_hdrc_imx.c:322:23: error: implicit declaration of function 'pinctrl_lookup_state'; did you mean 'inc_node_state'? [-Werror=implicit-function-declaration]
pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
^~~~~~~~~~~~~~~~~~~~
inc_node_state
drivers/usb/chipidea/ci_hdrc_imx.c:322:21: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
^
drivers/usb/chipidea/ci_hdrc_imx.c:338:29: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
^
cc1: some warnings being treated as errors
vim +261 drivers/usb/chipidea/ci_hdrc_imx.c
250
251 static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
252 {
253 struct device *dev = ci->dev->parent;
254 struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
255 int ret = 0;
256
257 switch (event) {
258 case CI_HDRC_IMX_HSIC_ACTIVE_EVENT:
259 if (!IS_ERR(data->pinctrl) &&
260 !IS_ERR(data->pinctrl_hsic_active)) {
> 261 ret = pinctrl_select_state(data->pinctrl,
262 data->pinctrl_hsic_active);
263 if (ret)
264 dev_err(dev,
265 "hsic_active select failed, err=%d\n",
266 ret);
267 return ret;
268 }
269 break;
270 case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
271 if (data->usbmisc_data) {
272 ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
273 if (ret)
274 dev_err(dev,
275 "hsic_set_connect failed, err=%d\n",
276 ret);
277 return ret;
278 }
279 break;
280 default:
281 break;
282 }
283
284 return ret;
285 }
286
287 static int ci_hdrc_imx_probe(struct platform_device *pdev)
288 {
289 struct ci_hdrc_imx_data *data;
290 struct ci_hdrc_platform_data pdata = {
291 .name = dev_name(&pdev->dev),
292 .capoffset = DEF_CAPOFFSET,
293 .notify_event = ci_hdrc_imx_notify_event,
294 };
295 int ret;
296 const struct of_device_id *of_id;
297 const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
298 struct device_node *np = pdev->dev.of_node;
299 struct device *dev = &pdev->dev;
300 struct pinctrl_state *pinctrl_hsic_idle;
301
302 of_id = of_match_device(ci_hdrc_imx_dt_ids, dev);
303 if (!of_id)
304 return -ENODEV;
305
306 imx_platform_flag = of_id->data;
307
308 data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
309 if (!data)
310 return -ENOMEM;
311
312 platform_set_drvdata(pdev, data);
313 data->usbmisc_data = usbmisc_get_init_data(dev);
314 if (IS_ERR(data->usbmisc_data))
315 return PTR_ERR(data->usbmisc_data);
316
> 317 data->pinctrl = devm_pinctrl_get(dev);
318 if (IS_ERR(data->pinctrl)) {
319 dev_dbg(dev, "pinctrl get failed, err=%ld\n",
320 PTR_ERR(data->pinctrl));
321 } else {
> 322 pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
323 if (IS_ERR(pinctrl_hsic_idle)) {
324 dev_dbg(dev,
325 "pinctrl_hsic_idle lookup failed, err=%ld\n",
326 PTR_ERR(pinctrl_hsic_idle));
327 } else {
328 ret = pinctrl_select_state(data->pinctrl,
329 pinctrl_hsic_idle);
330 if (ret) {
331 dev_err(dev,
332 "hsic_idle select failed, err=%d\n",
333 ret);
334 return ret;
335 }
336 }
337
338 data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
339 "active");
340 if (IS_ERR(data->pinctrl_hsic_active))
341 dev_dbg(dev,
342 "pinctrl_hsic_active lookup failed, err=%ld\n",
343 PTR_ERR(data->pinctrl_hsic_active));
344 }
345
346 ret = imx_get_clks(dev);
347 if (ret)
348 return ret;
349
350 ret = imx_prepare_enable_clks(dev);
351 if (ret)
352 return ret;
353
354 data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
355 if (IS_ERR(data->phy)) {
356 ret = PTR_ERR(data->phy);
357 /* Return -EINVAL if no usbphy is available */
358 if (ret == -ENODEV)
359 ret = -EINVAL;
360 goto err_clk;
361 }
362
363 pdata.usb_phy = data->phy;
364
365 if ((of_device_is_compatible(np, "fsl,imx53-usb") ||
366 of_device_is_compatible(np, "fsl,imx51-usb")) && pdata.usb_phy &&
367 of_usb_get_phy_mode(np) == USBPHY_INTERFACE_MODE_ULPI) {
368 pdata.flags |= CI_HDRC_OVERRIDE_PHY_CONTROL;
369 data->override_phy_control = true;
370 usb_phy_init(pdata.usb_phy);
371 }
372
373 pdata.flags |= imx_platform_flag->flags;
374 if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
375 data->supports_runtime_pm = true;
376
377 if (of_usb_get_phy_mode(dev->of_node) == USBPHY_INTERFACE_MODE_HSIC) {
378 pdata.flags |= CI_HDRC_IMX_IS_HSIC;
379 data->usbmisc_data->hsic = 1;
380 data->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
381 if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
382 ret = -EPROBE_DEFER;
383 goto err_clk;
384 } else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
385 /* no pad regualator is needed */
386 data->hsic_pad_regulator = NULL;
387 } else if (IS_ERR(data->hsic_pad_regulator)) {
388 dev_err(dev, "Get hsic pad regulator error: %ld\n",
389 PTR_ERR(data->hsic_pad_regulator));
390 ret = PTR_ERR(data->hsic_pad_regulator);
391 goto err_clk;
392 }
393
394 if (data->hsic_pad_regulator) {
395 ret = regulator_enable(data->hsic_pad_regulator);
396 if (ret) {
397 dev_err(dev,
398 "Fail to enable hsic pad regulator\n");
399 goto err_clk;
400 }
401 }
402 }
403
404 ret = imx_usbmisc_init(data->usbmisc_data);
405 if (ret) {
406 dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
407 goto disable_hsic_regulator;
408 }
409
410 data->ci_pdev = ci_hdrc_add_device(dev,
411 pdev->resource, pdev->num_resources,
412 &pdata);
413 if (IS_ERR(data->ci_pdev)) {
414 ret = PTR_ERR(data->ci_pdev);
415 if (ret != -EPROBE_DEFER)
416 dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
417 ret);
418 goto disable_hsic_regulator;
419 }
420
421 ret = imx_usbmisc_init_post(data->usbmisc_data);
422 if (ret) {
423 dev_err(dev, "usbmisc post failed, ret=%d\n", ret);
424 goto disable_device;
425 }
426
427 if (data->supports_runtime_pm) {
428 pm_runtime_set_active(dev);
429 pm_runtime_enable(dev);
430 }
431
432 device_set_wakeup_capable(dev, true);
433
434 return 0;
435
436 disable_device:
437 ci_hdrc_remove_device(data->ci_pdev);
438 disable_hsic_regulator:
439 if (data->hsic_pad_regulator)
440 ret = regulator_disable(data->hsic_pad_regulator);
441 err_clk:
442 imx_disable_unprepare_clks(dev);
443 return ret;
444 }
445
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35987 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-16 5:52 ` kbuild test robot
0 siblings, 0 replies; 31+ messages in thread
From: kbuild test robot @ 2018-10-16 5:52 UTC (permalink / raw)
To: Peter Chen
Cc: kbuild-all, linux-usb, frieder.schrempf, dl-linux-imx, robh+dt,
devicetree
Hi Peter,
I love your patch! Yet something to improve:
[auto build test ERROR on peter.chen-usb/ci-for-usb-next]
[also build test ERROR on v4.19-rc8 next-20181012]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Peter-Chen/usb-chipidea-imx-add-HSIC-support/20181016-130840
base: https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git ci-for-usb-next
config: x86_64-randconfig-x017-201841 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/usb/chipidea/ci_hdrc_imx.c: In function 'ci_hdrc_imx_notify_event':
>> drivers/usb/chipidea/ci_hdrc_imx.c:261:10: error: implicit declaration of function 'pinctrl_select_state'; did you mean 'inc_node_state'? [-Werror=implicit-function-declaration]
ret = pinctrl_select_state(data->pinctrl,
^~~~~~~~~~~~~~~~~~~~
inc_node_state
drivers/usb/chipidea/ci_hdrc_imx.c: In function 'ci_hdrc_imx_probe':
drivers/usb/chipidea/ci_hdrc_imx.c:317:18: error: implicit declaration of function 'devm_pinctrl_get'; did you mean 'devm_clk_get'? [-Werror=implicit-function-declaration]
data->pinctrl = devm_pinctrl_get(dev);
^~~~~~~~~~~~~~~~
devm_clk_get
drivers/usb/chipidea/ci_hdrc_imx.c:317:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
data->pinctrl = devm_pinctrl_get(dev);
^
>> drivers/usb/chipidea/ci_hdrc_imx.c:322:23: error: implicit declaration of function 'pinctrl_lookup_state'; did you mean 'inc_node_state'? [-Werror=implicit-function-declaration]
pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
^~~~~~~~~~~~~~~~~~~~
inc_node_state
drivers/usb/chipidea/ci_hdrc_imx.c:322:21: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
^
drivers/usb/chipidea/ci_hdrc_imx.c:338:29: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
^
cc1: some warnings being treated as errors
vim +261 drivers/usb/chipidea/ci_hdrc_imx.c
250
251 static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
252 {
253 struct device *dev = ci->dev->parent;
254 struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
255 int ret = 0;
256
257 switch (event) {
258 case CI_HDRC_IMX_HSIC_ACTIVE_EVENT:
259 if (!IS_ERR(data->pinctrl) &&
260 !IS_ERR(data->pinctrl_hsic_active)) {
> 261 ret = pinctrl_select_state(data->pinctrl,
262 data->pinctrl_hsic_active);
263 if (ret)
264 dev_err(dev,
265 "hsic_active select failed, err=%d\n",
266 ret);
267 return ret;
268 }
269 break;
270 case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
271 if (data->usbmisc_data) {
272 ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
273 if (ret)
274 dev_err(dev,
275 "hsic_set_connect failed, err=%d\n",
276 ret);
277 return ret;
278 }
279 break;
280 default:
281 break;
282 }
283
284 return ret;
285 }
286
287 static int ci_hdrc_imx_probe(struct platform_device *pdev)
288 {
289 struct ci_hdrc_imx_data *data;
290 struct ci_hdrc_platform_data pdata = {
291 .name = dev_name(&pdev->dev),
292 .capoffset = DEF_CAPOFFSET,
293 .notify_event = ci_hdrc_imx_notify_event,
294 };
295 int ret;
296 const struct of_device_id *of_id;
297 const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
298 struct device_node *np = pdev->dev.of_node;
299 struct device *dev = &pdev->dev;
300 struct pinctrl_state *pinctrl_hsic_idle;
301
302 of_id = of_match_device(ci_hdrc_imx_dt_ids, dev);
303 if (!of_id)
304 return -ENODEV;
305
306 imx_platform_flag = of_id->data;
307
308 data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
309 if (!data)
310 return -ENOMEM;
311
312 platform_set_drvdata(pdev, data);
313 data->usbmisc_data = usbmisc_get_init_data(dev);
314 if (IS_ERR(data->usbmisc_data))
315 return PTR_ERR(data->usbmisc_data);
316
> 317 data->pinctrl = devm_pinctrl_get(dev);
318 if (IS_ERR(data->pinctrl)) {
319 dev_dbg(dev, "pinctrl get failed, err=%ld\n",
320 PTR_ERR(data->pinctrl));
321 } else {
> 322 pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
323 if (IS_ERR(pinctrl_hsic_idle)) {
324 dev_dbg(dev,
325 "pinctrl_hsic_idle lookup failed, err=%ld\n",
326 PTR_ERR(pinctrl_hsic_idle));
327 } else {
328 ret = pinctrl_select_state(data->pinctrl,
329 pinctrl_hsic_idle);
330 if (ret) {
331 dev_err(dev,
332 "hsic_idle select failed, err=%d\n",
333 ret);
334 return ret;
335 }
336 }
337
338 data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
339 "active");
340 if (IS_ERR(data->pinctrl_hsic_active))
341 dev_dbg(dev,
342 "pinctrl_hsic_active lookup failed, err=%ld\n",
343 PTR_ERR(data->pinctrl_hsic_active));
344 }
345
346 ret = imx_get_clks(dev);
347 if (ret)
348 return ret;
349
350 ret = imx_prepare_enable_clks(dev);
351 if (ret)
352 return ret;
353
354 data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
355 if (IS_ERR(data->phy)) {
356 ret = PTR_ERR(data->phy);
357 /* Return -EINVAL if no usbphy is available */
358 if (ret == -ENODEV)
359 ret = -EINVAL;
360 goto err_clk;
361 }
362
363 pdata.usb_phy = data->phy;
364
365 if ((of_device_is_compatible(np, "fsl,imx53-usb") ||
366 of_device_is_compatible(np, "fsl,imx51-usb")) && pdata.usb_phy &&
367 of_usb_get_phy_mode(np) == USBPHY_INTERFACE_MODE_ULPI) {
368 pdata.flags |= CI_HDRC_OVERRIDE_PHY_CONTROL;
369 data->override_phy_control = true;
370 usb_phy_init(pdata.usb_phy);
371 }
372
373 pdata.flags |= imx_platform_flag->flags;
374 if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
375 data->supports_runtime_pm = true;
376
377 if (of_usb_get_phy_mode(dev->of_node) == USBPHY_INTERFACE_MODE_HSIC) {
378 pdata.flags |= CI_HDRC_IMX_IS_HSIC;
379 data->usbmisc_data->hsic = 1;
380 data->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
381 if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
382 ret = -EPROBE_DEFER;
383 goto err_clk;
384 } else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
385 /* no pad regualator is needed */
386 data->hsic_pad_regulator = NULL;
387 } else if (IS_ERR(data->hsic_pad_regulator)) {
388 dev_err(dev, "Get hsic pad regulator error: %ld\n",
389 PTR_ERR(data->hsic_pad_regulator));
390 ret = PTR_ERR(data->hsic_pad_regulator);
391 goto err_clk;
392 }
393
394 if (data->hsic_pad_regulator) {
395 ret = regulator_enable(data->hsic_pad_regulator);
396 if (ret) {
397 dev_err(dev,
398 "Fail to enable hsic pad regulator\n");
399 goto err_clk;
400 }
401 }
402 }
403
404 ret = imx_usbmisc_init(data->usbmisc_data);
405 if (ret) {
406 dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
407 goto disable_hsic_regulator;
408 }
409
410 data->ci_pdev = ci_hdrc_add_device(dev,
411 pdev->resource, pdev->num_resources,
412 &pdata);
413 if (IS_ERR(data->ci_pdev)) {
414 ret = PTR_ERR(data->ci_pdev);
415 if (ret != -EPROBE_DEFER)
416 dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
417 ret);
418 goto disable_hsic_regulator;
419 }
420
421 ret = imx_usbmisc_init_post(data->usbmisc_data);
422 if (ret) {
423 dev_err(dev, "usbmisc post failed, ret=%d\n", ret);
424 goto disable_device;
425 }
426
427 if (data->supports_runtime_pm) {
428 pm_runtime_set_active(dev);
429 pm_runtime_enable(dev);
430 }
431
432 device_set_wakeup_capable(dev, true);
433
434 return 0;
435
436 disable_device:
437 ci_hdrc_remove_device(data->ci_pdev);
438 disable_hsic_regulator:
439 if (data->hsic_pad_regulator)
440 ret = regulator_disable(data->hsic_pad_regulator);
441 err_clk:
442 imx_disable_unprepare_clks(dev);
443 return ret;
444 }
445
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-16 6:07 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 6:07 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, linux-usb, frieder.schrempf, dl-linux-imx, robh+dt,
devicetree
> Hi Peter,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on peter.chen-usb/ci-for-usb-next] [also build test ERROR
> on v4.19-rc8 next-20181012] [if your patch is applied to the wrong git tree, please
> drop us a note to help improve the system]
>
> url:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com
> %2F0day-ci%2Flinux%2Fcommits%2FPeter-Chen%2Fusb-chipidea-imx-add-HSIC-
> support%2F20181016-
> 130840&data=02%7C01%7Cpeter.chen%40nxp.com%7C50f2cf2d6d5341bf72
> cc08d6332ba794%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63675
> 2662551616821&sdata=clbCGj%2BrgTz56IKDM0DVVyL3e4q79FsCv3vn%2F7
> TSrQI%3D&reserved=0
> base:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.o
> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpeter.chen%2Fusb.git&data
> =02%7C01%7Cpeter.chen%40nxp.com%7C50f2cf2d6d5341bf72cc08d6332ba794
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636752662551616821
> &sdata=6Bp7JvbXAsJ%2FGZCOn0VmtZQSnWUCYhgqlbG8ZrbzBoE%3D&a
> mp;reserved=0 ci-for-usb-next
> config: x86_64-randconfig-x017-201841 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
Will fix it by adding #include <linux/pinctrl/consumer.h>, it is strange I did not meet this error.
Peter
> All errors (new ones prefixed by >>):
>
> drivers/usb/chipidea/ci_hdrc_imx.c: In function 'ci_hdrc_imx_notify_event':
> >> drivers/usb/chipidea/ci_hdrc_imx.c:261:10: error: implicit
> >> declaration of function 'pinctrl_select_state'; did you mean
> >> 'inc_node_state'? [-Werror=implicit-function-declaration]
> ret = pinctrl_select_state(data->pinctrl,
> ^~~~~~~~~~~~~~~~~~~~
> inc_node_state
> drivers/usb/chipidea/ci_hdrc_imx.c: In function 'ci_hdrc_imx_probe':
> drivers/usb/chipidea/ci_hdrc_imx.c:317:18: error: implicit declaration of function
> 'devm_pinctrl_get'; did you mean 'devm_clk_get'? [-Werror=implicit-function-
> declaration]
> data->pinctrl = devm_pinctrl_get(dev);
> ^~~~~~~~~~~~~~~~
> devm_clk_get
> drivers/usb/chipidea/ci_hdrc_imx.c:317:16: warning: assignment makes pointer
> from integer without a cast [-Wint-conversion]
> data->pinctrl = devm_pinctrl_get(dev);
> ^
> >> drivers/usb/chipidea/ci_hdrc_imx.c:322:23: error: implicit
> >> declaration of function 'pinctrl_lookup_state'; did you mean
> >> 'inc_node_state'? [-Werror=implicit-function-declaration]
> pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
> ^~~~~~~~~~~~~~~~~~~~
> inc_node_state
> drivers/usb/chipidea/ci_hdrc_imx.c:322:21: warning: assignment makes pointer
> from integer without a cast [-Wint-conversion]
> pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
> ^
> drivers/usb/chipidea/ci_hdrc_imx.c:338:29: warning: assignment makes pointer
> from integer without a cast [-Wint-conversion]
> data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
> ^
> cc1: some warnings being treated as errors
>
> vim +261 drivers/usb/chipidea/ci_hdrc_imx.c
>
> 250
> 251 static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
> 252 {
> 253 struct device *dev = ci->dev->parent;
> 254 struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> 255 int ret = 0;
> 256
> 257 switch (event) {
> 258 case CI_HDRC_IMX_HSIC_ACTIVE_EVENT:
> 259 if (!IS_ERR(data->pinctrl) &&
> 260 !IS_ERR(data->pinctrl_hsic_active)) {
> > 261 ret = pinctrl_select_state(data->pinctrl,
> 262 data->pinctrl_hsic_active);
> 263 if (ret)
> 264 dev_err(dev,
> 265 "hsic_active select failed, err=%d\n",
> 266 ret);
> 267 return ret;
> 268 }
> 269 break;
> 270 case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
> 271 if (data->usbmisc_data) {
> 272 ret = imx_usbmisc_hsic_set_connect(data-
> >usbmisc_data);
> 273 if (ret)
> 274 dev_err(dev,
> 275 "hsic_set_connect failed, err=%d\n",
> 276 ret);
> 277 return ret;
> 278 }
> 279 break;
> 280 default:
> 281 break;
> 282 }
> 283
> 284 return ret;
> 285 }
> 286
> 287 static int ci_hdrc_imx_probe(struct platform_device *pdev)
> 288 {
> 289 struct ci_hdrc_imx_data *data;
> 290 struct ci_hdrc_platform_data pdata = {
> 291 .name = dev_name(&pdev->dev),
> 292 .capoffset = DEF_CAPOFFSET,
> 293 .notify_event = ci_hdrc_imx_notify_event,
> 294 };
> 295 int ret;
> 296 const struct of_device_id *of_id;
> 297 const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
> 298 struct device_node *np = pdev->dev.of_node;
> 299 struct device *dev = &pdev->dev;
> 300 struct pinctrl_state *pinctrl_hsic_idle;
> 301
> 302 of_id = of_match_device(ci_hdrc_imx_dt_ids, dev);
> 303 if (!of_id)
> 304 return -ENODEV;
> 305
> 306 imx_platform_flag = of_id->data;
> 307
> 308 data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> 309 if (!data)
> 310 return -ENOMEM;
> 311
> 312 platform_set_drvdata(pdev, data);
> 313 data->usbmisc_data = usbmisc_get_init_data(dev);
> 314 if (IS_ERR(data->usbmisc_data))
> 315 return PTR_ERR(data->usbmisc_data);
> 316
> > 317 data->pinctrl = devm_pinctrl_get(dev);
> 318 if (IS_ERR(data->pinctrl)) {
> 319 dev_dbg(dev, "pinctrl get failed, err=%ld\n",
> 320 PTR_ERR(data->pinctrl));
> 321 } else {
> > 322 pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
> 323 if (IS_ERR(pinctrl_hsic_idle)) {
> 324 dev_dbg(dev,
> 325 "pinctrl_hsic_idle lookup failed, err=%ld\n",
> 326 PTR_ERR(pinctrl_hsic_idle));
> 327 } else {
> 328 ret = pinctrl_select_state(data->pinctrl,
> 329 pinctrl_hsic_idle);
> 330 if (ret) {
> 331 dev_err(dev,
> 332 "hsic_idle select failed, err=%d\n",
> 333 ret);
> 334 return ret;
> 335 }
> 336 }
> 337
> 338 data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
> 339 "active");
> 340 if (IS_ERR(data->pinctrl_hsic_active))
> 341 dev_dbg(dev,
> 342 "pinctrl_hsic_active lookup failed, err=%ld\n",
> 343 PTR_ERR(data->pinctrl_hsic_active));
> 344 }
> 345
> 346 ret = imx_get_clks(dev);
> 347 if (ret)
> 348 return ret;
> 349
> 350 ret = imx_prepare_enable_clks(dev);
> 351 if (ret)
> 352 return ret;
> 353
> 354 data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
> 355 if (IS_ERR(data->phy)) {
> 356 ret = PTR_ERR(data->phy);
> 357 /* Return -EINVAL if no usbphy is available */
> 358 if (ret == -ENODEV)
> 359 ret = -EINVAL;
> 360 goto err_clk;
> 361 }
> 362
> 363 pdata.usb_phy = data->phy;
> 364
> 365 if ((of_device_is_compatible(np, "fsl,imx53-usb") ||
> 366 of_device_is_compatible(np, "fsl,imx51-usb")) && pdata.usb_phy
> &&
> 367 of_usb_get_phy_mode(np) ==
> USBPHY_INTERFACE_MODE_ULPI) {
> 368 pdata.flags |= CI_HDRC_OVERRIDE_PHY_CONTROL;
> 369 data->override_phy_control = true;
> 370 usb_phy_init(pdata.usb_phy);
> 371 }
> 372
> 373 pdata.flags |= imx_platform_flag->flags;
> 374 if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> 375 data->supports_runtime_pm = true;
> 376
> 377 if (of_usb_get_phy_mode(dev->of_node) ==
> USBPHY_INTERFACE_MODE_HSIC) {
> 378 pdata.flags |= CI_HDRC_IMX_IS_HSIC;
> 379 data->usbmisc_data->hsic = 1;
> 380 data->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
> 381 if (PTR_ERR(data->hsic_pad_regulator) == -
> EPROBE_DEFER) {
> 382 ret = -EPROBE_DEFER;
> 383 goto err_clk;
> 384 } else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV)
> {
> 385 /* no pad regualator is needed */
> 386 data->hsic_pad_regulator = NULL;
> 387 } else if (IS_ERR(data->hsic_pad_regulator)) {
> 388 dev_err(dev, "Get hsic pad regulator error: %ld\n",
> 389 PTR_ERR(data->hsic_pad_regulator));
> 390 ret = PTR_ERR(data->hsic_pad_regulator);
> 391 goto err_clk;
> 392 }
> 393
> 394 if (data->hsic_pad_regulator) {
> 395 ret = regulator_enable(data->hsic_pad_regulator);
> 396 if (ret) {
> 397 dev_err(dev,
> 398 "Fail to enable hsic pad regulator\n");
> 399 goto err_clk;
> 400 }
> 401 }
> 402 }
> 403
> 404 ret = imx_usbmisc_init(data->usbmisc_data);
> 405 if (ret) {
> 406 dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> 407 goto disable_hsic_regulator;
> 408 }
> 409
> 410 data->ci_pdev = ci_hdrc_add_device(dev,
> 411 pdev->resource, pdev->num_resources,
> 412 &pdata);
> 413 if (IS_ERR(data->ci_pdev)) {
> 414 ret = PTR_ERR(data->ci_pdev);
> 415 if (ret != -EPROBE_DEFER)
> 416 dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
> 417 ret);
> 418 goto disable_hsic_regulator;
> 419 }
> 420
> 421 ret = imx_usbmisc_init_post(data->usbmisc_data);
> 422 if (ret) {
> 423 dev_err(dev, "usbmisc post failed, ret=%d\n", ret);
> 424 goto disable_device;
> 425 }
> 426
> 427 if (data->supports_runtime_pm) {
> 428 pm_runtime_set_active(dev);
> 429 pm_runtime_enable(dev);
> 430 }
> 431
> 432 device_set_wakeup_capable(dev, true);
> 433
> 434 return 0;
> 435
> 436 disable_device:
> 437 ci_hdrc_remove_device(data->ci_pdev);
> 438 disable_hsic_regulator:
> 439 if (data->hsic_pad_regulator)
> 440 ret = regulator_disable(data->hsic_pad_regulator);
> 441 err_clk:
> 442 imx_disable_unprepare_clks(dev);
> 443 return ret;
> 444 }
> 445
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org
> %2Fpipermail%2Fkbuild-
> all&data=02%7C01%7Cpeter.chen%40nxp.com%7C50f2cf2d6d5341bf72cc08
> d6332ba794%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636752662
> 551616821&sdata=FeXt%2BW7Jnl%2B%2BihbZjlVj5vZddN2C%2Fuf63PeBJv
> MQ6TA%3D&reserved=0 Intel Corporation
^ permalink raw reply [flat|nested] 31+ messages in thread
* [2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-16 6:07 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-16 6:07 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, linux-usb, frieder.schrempf, dl-linux-imx, robh+dt,
devicetree
> Hi Peter,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on peter.chen-usb/ci-for-usb-next] [also build test ERROR
> on v4.19-rc8 next-20181012] [if your patch is applied to the wrong git tree, please
> drop us a note to help improve the system]
>
> url:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com
> %2F0day-ci%2Flinux%2Fcommits%2FPeter-Chen%2Fusb-chipidea-imx-add-HSIC-
> support%2F20181016-
> 130840&data=02%7C01%7Cpeter.chen%40nxp.com%7C50f2cf2d6d5341bf72
> cc08d6332ba794%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63675
> 2662551616821&sdata=clbCGj%2BrgTz56IKDM0DVVyL3e4q79FsCv3vn%2F7
> TSrQI%3D&reserved=0
> base:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.o
> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpeter.chen%2Fusb.git&data
> =02%7C01%7Cpeter.chen%40nxp.com%7C50f2cf2d6d5341bf72cc08d6332ba794
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636752662551616821
> &sdata=6Bp7JvbXAsJ%2FGZCOn0VmtZQSnWUCYhgqlbG8ZrbzBoE%3D&a
> mp;reserved=0 ci-for-usb-next
> config: x86_64-randconfig-x017-201841 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
Will fix it by adding #include <linux/pinctrl/consumer.h>, it is strange I did not meet this error.
Peter
> All errors (new ones prefixed by >>):
>
> drivers/usb/chipidea/ci_hdrc_imx.c: In function 'ci_hdrc_imx_notify_event':
> >> drivers/usb/chipidea/ci_hdrc_imx.c:261:10: error: implicit
> >> declaration of function 'pinctrl_select_state'; did you mean
> >> 'inc_node_state'? [-Werror=implicit-function-declaration]
> ret = pinctrl_select_state(data->pinctrl,
> ^~~~~~~~~~~~~~~~~~~~
> inc_node_state
> drivers/usb/chipidea/ci_hdrc_imx.c: In function 'ci_hdrc_imx_probe':
> drivers/usb/chipidea/ci_hdrc_imx.c:317:18: error: implicit declaration of function
> 'devm_pinctrl_get'; did you mean 'devm_clk_get'? [-Werror=implicit-function-
> declaration]
> data->pinctrl = devm_pinctrl_get(dev);
> ^~~~~~~~~~~~~~~~
> devm_clk_get
> drivers/usb/chipidea/ci_hdrc_imx.c:317:16: warning: assignment makes pointer
> from integer without a cast [-Wint-conversion]
> data->pinctrl = devm_pinctrl_get(dev);
> ^
> >> drivers/usb/chipidea/ci_hdrc_imx.c:322:23: error: implicit
> >> declaration of function 'pinctrl_lookup_state'; did you mean
> >> 'inc_node_state'? [-Werror=implicit-function-declaration]
> pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
> ^~~~~~~~~~~~~~~~~~~~
> inc_node_state
> drivers/usb/chipidea/ci_hdrc_imx.c:322:21: warning: assignment makes pointer
> from integer without a cast [-Wint-conversion]
> pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
> ^
> drivers/usb/chipidea/ci_hdrc_imx.c:338:29: warning: assignment makes pointer
> from integer without a cast [-Wint-conversion]
> data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
> ^
> cc1: some warnings being treated as errors
>
> vim +261 drivers/usb/chipidea/ci_hdrc_imx.c
>
> 250
> 251 static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci, unsigned int event)
> 252 {
> 253 struct device *dev = ci->dev->parent;
> 254 struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> 255 int ret = 0;
> 256
> 257 switch (event) {
> 258 case CI_HDRC_IMX_HSIC_ACTIVE_EVENT:
> 259 if (!IS_ERR(data->pinctrl) &&
> 260 !IS_ERR(data->pinctrl_hsic_active)) {
> > 261 ret = pinctrl_select_state(data->pinctrl,
> 262 data->pinctrl_hsic_active);
> 263 if (ret)
> 264 dev_err(dev,
> 265 "hsic_active select failed, err=%d\n",
> 266 ret);
> 267 return ret;
> 268 }
> 269 break;
> 270 case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
> 271 if (data->usbmisc_data) {
> 272 ret = imx_usbmisc_hsic_set_connect(data-
> >usbmisc_data);
> 273 if (ret)
> 274 dev_err(dev,
> 275 "hsic_set_connect failed, err=%d\n",
> 276 ret);
> 277 return ret;
> 278 }
> 279 break;
> 280 default:
> 281 break;
> 282 }
> 283
> 284 return ret;
> 285 }
> 286
> 287 static int ci_hdrc_imx_probe(struct platform_device *pdev)
> 288 {
> 289 struct ci_hdrc_imx_data *data;
> 290 struct ci_hdrc_platform_data pdata = {
> 291 .name = dev_name(&pdev->dev),
> 292 .capoffset = DEF_CAPOFFSET,
> 293 .notify_event = ci_hdrc_imx_notify_event,
> 294 };
> 295 int ret;
> 296 const struct of_device_id *of_id;
> 297 const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
> 298 struct device_node *np = pdev->dev.of_node;
> 299 struct device *dev = &pdev->dev;
> 300 struct pinctrl_state *pinctrl_hsic_idle;
> 301
> 302 of_id = of_match_device(ci_hdrc_imx_dt_ids, dev);
> 303 if (!of_id)
> 304 return -ENODEV;
> 305
> 306 imx_platform_flag = of_id->data;
> 307
> 308 data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> 309 if (!data)
> 310 return -ENOMEM;
> 311
> 312 platform_set_drvdata(pdev, data);
> 313 data->usbmisc_data = usbmisc_get_init_data(dev);
> 314 if (IS_ERR(data->usbmisc_data))
> 315 return PTR_ERR(data->usbmisc_data);
> 316
> > 317 data->pinctrl = devm_pinctrl_get(dev);
> 318 if (IS_ERR(data->pinctrl)) {
> 319 dev_dbg(dev, "pinctrl get failed, err=%ld\n",
> 320 PTR_ERR(data->pinctrl));
> 321 } else {
> > 322 pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
> 323 if (IS_ERR(pinctrl_hsic_idle)) {
> 324 dev_dbg(dev,
> 325 "pinctrl_hsic_idle lookup failed, err=%ld\n",
> 326 PTR_ERR(pinctrl_hsic_idle));
> 327 } else {
> 328 ret = pinctrl_select_state(data->pinctrl,
> 329 pinctrl_hsic_idle);
> 330 if (ret) {
> 331 dev_err(dev,
> 332 "hsic_idle select failed, err=%d\n",
> 333 ret);
> 334 return ret;
> 335 }
> 336 }
> 337
> 338 data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
> 339 "active");
> 340 if (IS_ERR(data->pinctrl_hsic_active))
> 341 dev_dbg(dev,
> 342 "pinctrl_hsic_active lookup failed, err=%ld\n",
> 343 PTR_ERR(data->pinctrl_hsic_active));
> 344 }
> 345
> 346 ret = imx_get_clks(dev);
> 347 if (ret)
> 348 return ret;
> 349
> 350 ret = imx_prepare_enable_clks(dev);
> 351 if (ret)
> 352 return ret;
> 353
> 354 data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
> 355 if (IS_ERR(data->phy)) {
> 356 ret = PTR_ERR(data->phy);
> 357 /* Return -EINVAL if no usbphy is available */
> 358 if (ret == -ENODEV)
> 359 ret = -EINVAL;
> 360 goto err_clk;
> 361 }
> 362
> 363 pdata.usb_phy = data->phy;
> 364
> 365 if ((of_device_is_compatible(np, "fsl,imx53-usb") ||
> 366 of_device_is_compatible(np, "fsl,imx51-usb")) && pdata.usb_phy
> &&
> 367 of_usb_get_phy_mode(np) ==
> USBPHY_INTERFACE_MODE_ULPI) {
> 368 pdata.flags |= CI_HDRC_OVERRIDE_PHY_CONTROL;
> 369 data->override_phy_control = true;
> 370 usb_phy_init(pdata.usb_phy);
> 371 }
> 372
> 373 pdata.flags |= imx_platform_flag->flags;
> 374 if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> 375 data->supports_runtime_pm = true;
> 376
> 377 if (of_usb_get_phy_mode(dev->of_node) ==
> USBPHY_INTERFACE_MODE_HSIC) {
> 378 pdata.flags |= CI_HDRC_IMX_IS_HSIC;
> 379 data->usbmisc_data->hsic = 1;
> 380 data->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
> 381 if (PTR_ERR(data->hsic_pad_regulator) == -
> EPROBE_DEFER) {
> 382 ret = -EPROBE_DEFER;
> 383 goto err_clk;
> 384 } else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV)
> {
> 385 /* no pad regualator is needed */
> 386 data->hsic_pad_regulator = NULL;
> 387 } else if (IS_ERR(data->hsic_pad_regulator)) {
> 388 dev_err(dev, "Get hsic pad regulator error: %ld\n",
> 389 PTR_ERR(data->hsic_pad_regulator));
> 390 ret = PTR_ERR(data->hsic_pad_regulator);
> 391 goto err_clk;
> 392 }
> 393
> 394 if (data->hsic_pad_regulator) {
> 395 ret = regulator_enable(data->hsic_pad_regulator);
> 396 if (ret) {
> 397 dev_err(dev,
> 398 "Fail to enable hsic pad regulator\n");
> 399 goto err_clk;
> 400 }
> 401 }
> 402 }
> 403
> 404 ret = imx_usbmisc_init(data->usbmisc_data);
> 405 if (ret) {
> 406 dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> 407 goto disable_hsic_regulator;
> 408 }
> 409
> 410 data->ci_pdev = ci_hdrc_add_device(dev,
> 411 pdev->resource, pdev->num_resources,
> 412 &pdata);
> 413 if (IS_ERR(data->ci_pdev)) {
> 414 ret = PTR_ERR(data->ci_pdev);
> 415 if (ret != -EPROBE_DEFER)
> 416 dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
> 417 ret);
> 418 goto disable_hsic_regulator;
> 419 }
> 420
> 421 ret = imx_usbmisc_init_post(data->usbmisc_data);
> 422 if (ret) {
> 423 dev_err(dev, "usbmisc post failed, ret=%d\n", ret);
> 424 goto disable_device;
> 425 }
> 426
> 427 if (data->supports_runtime_pm) {
> 428 pm_runtime_set_active(dev);
> 429 pm_runtime_enable(dev);
> 430 }
> 431
> 432 device_set_wakeup_capable(dev, true);
> 433
> 434 return 0;
> 435
> 436 disable_device:
> 437 ci_hdrc_remove_device(data->ci_pdev);
> 438 disable_hsic_regulator:
> 439 if (data->hsic_pad_regulator)
> 440 ret = regulator_disable(data->hsic_pad_regulator);
> 441 err_clk:
> 442 imx_disable_unprepare_clks(dev);
> 443 return ret;
> 444 }
> 445
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org
> %2Fpipermail%2Fkbuild-
> all&data=02%7C01%7Cpeter.chen%40nxp.com%7C50f2cf2d6d5341bf72cc08
> d6332ba794%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636752662
> 551616821&sdata=FeXt%2BW7Jnl%2B%2BihbZjlVj5vZddN2C%2Fuf63PeBJv
> MQ6TA%3D&reserved=0 Intel Corporation
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
@ 2018-10-16 16:24 ` Fabio Estevam
0 siblings, 0 replies; 31+ messages in thread
From: Fabio Estevam @ 2018-10-16 16:24 UTC (permalink / raw)
To: Peter Chen
Cc: USB list, Schrempf Frieder, NXP Linux Team, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
Hi Peter,
On Tue, Oct 16, 2018 at 2:02 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 | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 529e51879fb2..10c8d793ea49 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -81,6 +81,7 @@ Optional properties:
> 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"
> + Or names for HSIC "idle" and "active" pin modes.
I don't think this description is clear enough.
Could you please add a real dts snippet for the HSIC case instead?
^ permalink raw reply [flat|nested] 31+ messages in thread
* [4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
@ 2018-10-16 16:24 ` Fabio Estevam
0 siblings, 0 replies; 31+ messages in thread
From: Fabio Estevam @ 2018-10-16 16:24 UTC (permalink / raw)
To: Peter Chen
Cc: USB list, Schrempf Frieder, NXP Linux Team, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
Hi Peter,
On Tue, Oct 16, 2018 at 2:02 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 | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 529e51879fb2..10c8d793ea49 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -81,6 +81,7 @@ Optional properties:
> 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"
> + Or names for HSIC "idle" and "active" pin modes.
I don't think this description is clear enough.
Could you please add a real dts snippet for the HSIC case instead?
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
@ 2018-10-17 1:04 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-17 1:04 UTC (permalink / raw)
To: Fabio Estevam
Cc: USB list, Schrempf Frieder, dl-linux-imx, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> On Tue, Oct 16, 2018 at 2:02 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 | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > index 529e51879fb2..10c8d793ea49 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > @@ -81,6 +81,7 @@ Optional properties:
> > 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"
> > + Or names for HSIC "idle" and "active" pin modes.
>
> I don't think this description is clear enough.
>
> Could you please add a real dts snippet for the HSIC case instead?
Ok, I will add example like below at next version.
usb@02184000 { /* USB OTG */ {
...
pinctrl-names = "idle", "active";
pinctrl-0 = <&pinctrl_usbh2_1>;
pinctrl-1 = <&pinctrl_usbh2_2>;
...
};
Peter
^ permalink raw reply [flat|nested] 31+ messages in thread
* [4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
@ 2018-10-17 1:04 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-17 1:04 UTC (permalink / raw)
To: Fabio Estevam
Cc: USB list, Schrempf Frieder, dl-linux-imx, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> On Tue, Oct 16, 2018 at 2:02 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 | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > index 529e51879fb2..10c8d793ea49 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > @@ -81,6 +81,7 @@ Optional properties:
> > 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"
> > + Or names for HSIC "idle" and "active" pin modes.
>
> I don't think this description is clear enough.
>
> Could you please add a real dts snippet for the HSIC case instead?
Ok, I will add example like below at next version.
usb@02184000 { /* USB OTG */ {
...
pinctrl-names = "idle", "active";
pinctrl-0 = <&pinctrl_usbh2_1>;
pinctrl-1 = <&pinctrl_usbh2_2>;
...
};
Peter
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-16 5:01 [PATCH 0/4] usb: chipidea: imx: add HSIC support Peter Chen
` (3 preceding siblings ...)
2018-10-16 5:01 ` [4/4] " Peter Chen
@ 2018-10-17 7:02 ` Frieder Schrempf
2018-10-17 7:23 ` Peter Chen
4 siblings, 1 reply; 31+ messages in thread
From: Frieder Schrempf @ 2018-10-17 7:02 UTC (permalink / raw)
To: Peter Chen, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
Hi Peter,
On 16.10.18 07:01, Peter Chen wrote:
> 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. I have
> no HSIC interface board which is supported by upstream kernel, so this
> patches are only compiled ok, Frieder Schrempf, would you please
> help me test it on your board? Thanks.
Thank you for providing the patch!
I applied it to v4.19-rc8 and tested and the LAN9730 comes up and works
just fine.
Here is how my devicetree looks like:
usbphy_dummy: usbphy_dummy@1 {
compatible = "usb-nop-xceiv";
};
[...]
&usbh2 {
vbus-supply = <®_usb_h2_vbus>;
pinctrl-names = "idle", "active";
pinctrl-0 = <&pinctrl_usbh2_idle>;
pinctrl-1 = <&pinctrl_usbh2_active>;
fsl,usbphy = <&usbphy_dummy>;
phy_type = "hsic";
status = "okay";
#address-cells = <1>;
#size-cells = <0>;
usbnet: smsc@1 {
compatible = "usb424,9730";
/* Filled in by U-Boot */
mac-address = [00 00 00 00 00 00];
reg = <1>;
};
};
[...]
pinctrl_usbh2_idle: usbh2grp-idle {
fsl,pins = <
MX6QDL_PAD_RGMII_TXC__USB_H2_DATA 0x40013030
MX6QDL_PAD_RGMII_TX_CTL__USB_H2_STROBE 0x40013030
>;
};
pinctrl_usbh2_active: usbh2grp-active {
fsl,pins = <
MX6QDL_PAD_RGMII_TXC__USB_H2_DATA 0x40013030
MX6QDL_PAD_RGMII_TX_CTL__USB_H2_STROBE 0x40017030
>;
};
Are there any test cases I should try?
How can I test suspend/resume?
I also have some suggestions for your patch. Please see the separate email.
Thanks,
Frieder
>
> 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 | 1 +
> drivers/usb/chipidea/ci_hdrc_imx.c | 153 ++++++++++++++++++---
> drivers/usb/chipidea/ci_hdrc_imx.h | 9 +-
> drivers/usb/chipidea/host.c | 98 +++++++++++++
> drivers/usb/chipidea/usbmisc_imx.c | 131 ++++++++++++++++++
> include/linux/usb/chipidea.h | 3 +
> 6 files changed, 376 insertions(+), 19 deletions(-)
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-17 7:03 ` Frieder Schrempf
0 siblings, 0 replies; 31+ messages in thread
From: Frieder Schrempf @ 2018-10-17 7:03 UTC (permalink / raw)
To: Peter Chen, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
Hi Peter,
please see my comments below. For reference I also pushed the changes
here: https://github.com/fschrempf/linux/commits/usb-hsic-test
On 16.10.18 07:01, 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>
> ---
> drivers/usb/chipidea/ci_hdrc_imx.c | 153 +++++++++++++++++++++++++++++++++----
> drivers/usb/chipidea/ci_hdrc_imx.h | 9 ++-
> drivers/usb/chipidea/usbmisc_imx.c | 131 +++++++++++++++++++++++++++++++
> 3 files changed, 274 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 09b37c0d075d..d566771fc77a 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -85,6 +85,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 +248,58 @@ 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:
> + 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.
> + ret = pinctrl_select_state(data->pinctrl,
> + data->pinctrl_hsic_active);
> + if (ret)
> + dev_err(dev,
> + "hsic_active select failed, err=%d\n",
> + ret);
> + return ret;
> + }
> + break;
> + case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
> + if (data->usbmisc_data) {
> + ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
> + if (ret)
> + dev_err(dev,
> + "hsic_set_connect failed, err=%d\n",
> + ret);
> + return 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 +310,48 @@ 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);
> + data->pinctrl = devm_pinctrl_get(dev);
> + if (IS_ERR(data->pinctrl)) {
> + dev_dbg(dev, "pinctrl get failed, err=%ld\n",
> + PTR_ERR(data->pinctrl));
> + } else {
> + pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
> + if (IS_ERR(pinctrl_hsic_idle)) {
> + dev_dbg(dev,
> + "pinctrl_hsic_idle lookup failed, err=%ld\n",
> + PTR_ERR(pinctrl_hsic_idle));
> + } else {
> + 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_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);
}
}
> +
> + ret = imx_get_clks(dev);
> if (ret)
> return ret;
>
> - ret = imx_prepare_enable_clks(&pdev->dev);
> + ret = imx_prepare_enable_clks(dev);
> if (ret)
> return ret;
>
> - 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 */
> @@ -303,42 +374,72 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> data->supports_runtime_pm = true;
>
> + 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->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
> + if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto err_clk;
> + } 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));
> + ret = PTR_ERR(data->hsic_pad_regulator);
> + goto err_clk;
> + }
> +
> + if (data->hsic_pad_regulator) {
> + ret = regulator_enable(data->hsic_pad_regulator);
> + if (ret) {
> + dev_err(dev,
> + "Fail to enable hsic pad regulator\n");
> + goto err_clk;
> + }
> + }
> + }
> +
> ret = imx_usbmisc_init(data->usbmisc_data);
> if (ret) {
> - dev_err(&pdev->dev, "usbmisc init failed, ret=%d\n", ret);
> - goto err_clk;
> + dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> + goto disable_hsic_regulator;
> }
>
> - 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);
> - goto err_clk;
> + dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
> + ret);
> + goto disable_hsic_regulator;
> }
>
> 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);
> +disable_hsic_regulator:
> + if (data->hsic_pad_regulator)
> + ret = regulator_disable(data->hsic_pad_regulator);
> err_clk:
> - imx_disable_unprepare_clks(&pdev->dev);
> + imx_disable_unprepare_clks(dev);
> return ret;
> }
>
> @@ -355,6 +456,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 +470,19 @@ 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__);
>
> + if (data->usbmisc_data) {
Why do we have this check here, but not in imx_controller_resume()?
> + 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)
> goto clk_disable;
> }
>
Why don't we have a check for data->usbmisc_data here, but in
imx_controller_suspend() we have one?
> + 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)
> 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, ..."
> + * 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."
Thanks,
Frieder
> + */
> + offset = 0;
> + } else {
> + dev_err(data->dev, "index is error for usbmisc\n");
> + offset = 0;
> + ret = -EINVAL;
> + }
> +
> + 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 ret;
> +}
> +
> +static int usbmisc_imx6_hsic_set_clk(struct imx_usbmisc_data *data, bool on)
> +{
> + 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) {
> + offset = 0;
> + } else {
> + dev_err(data->dev, "index is error for usbmisc\n");
> + offset = 0;
> + ret = -EINVAL;
> + }
> +
> + 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 +477,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 +553,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 +581,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 +592,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 +650,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",
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-17 7:03 ` Frieder Schrempf
0 siblings, 0 replies; 31+ messages in thread
From: Frieder Schrempf @ 2018-10-17 7:03 UTC (permalink / raw)
To: Peter Chen, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
Hi Peter,
please see my comments below. For reference I also pushed the changes
here: https://github.com/fschrempf/linux/commits/usb-hsic-test
On 16.10.18 07:01, 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>
> ---
> drivers/usb/chipidea/ci_hdrc_imx.c | 153 +++++++++++++++++++++++++++++++++----
> drivers/usb/chipidea/ci_hdrc_imx.h | 9 ++-
> drivers/usb/chipidea/usbmisc_imx.c | 131 +++++++++++++++++++++++++++++++
> 3 files changed, 274 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 09b37c0d075d..d566771fc77a 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -85,6 +85,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 +248,58 @@ 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:
> + 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.
> + ret = pinctrl_select_state(data->pinctrl,
> + data->pinctrl_hsic_active);
> + if (ret)
> + dev_err(dev,
> + "hsic_active select failed, err=%d\n",
> + ret);
> + return ret;
> + }
> + break;
> + case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
> + if (data->usbmisc_data) {
> + ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
> + if (ret)
> + dev_err(dev,
> + "hsic_set_connect failed, err=%d\n",
> + ret);
> + return 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 +310,48 @@ 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);
> + data->pinctrl = devm_pinctrl_get(dev);
> + if (IS_ERR(data->pinctrl)) {
> + dev_dbg(dev, "pinctrl get failed, err=%ld\n",
> + PTR_ERR(data->pinctrl));
> + } else {
> + pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
> + if (IS_ERR(pinctrl_hsic_idle)) {
> + dev_dbg(dev,
> + "pinctrl_hsic_idle lookup failed, err=%ld\n",
> + PTR_ERR(pinctrl_hsic_idle));
> + } else {
> + 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_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);
}
}
> +
> + ret = imx_get_clks(dev);
> if (ret)
> return ret;
>
> - ret = imx_prepare_enable_clks(&pdev->dev);
> + ret = imx_prepare_enable_clks(dev);
> if (ret)
> return ret;
>
> - 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 */
> @@ -303,42 +374,72 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> data->supports_runtime_pm = true;
>
> + 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->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
> + if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto err_clk;
> + } 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));
> + ret = PTR_ERR(data->hsic_pad_regulator);
> + goto err_clk;
> + }
> +
> + if (data->hsic_pad_regulator) {
> + ret = regulator_enable(data->hsic_pad_regulator);
> + if (ret) {
> + dev_err(dev,
> + "Fail to enable hsic pad regulator\n");
> + goto err_clk;
> + }
> + }
> + }
> +
> ret = imx_usbmisc_init(data->usbmisc_data);
> if (ret) {
> - dev_err(&pdev->dev, "usbmisc init failed, ret=%d\n", ret);
> - goto err_clk;
> + dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
> + goto disable_hsic_regulator;
> }
>
> - 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);
> - goto err_clk;
> + dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
> + ret);
> + goto disable_hsic_regulator;
> }
>
> 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);
> +disable_hsic_regulator:
> + if (data->hsic_pad_regulator)
> + ret = regulator_disable(data->hsic_pad_regulator);
> err_clk:
> - imx_disable_unprepare_clks(&pdev->dev);
> + imx_disable_unprepare_clks(dev);
> return ret;
> }
>
> @@ -355,6 +456,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 +470,19 @@ 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__);
>
> + if (data->usbmisc_data) {
Why do we have this check here, but not in imx_controller_resume()?
> + 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)
> goto clk_disable;
> }
>
Why don't we have a check for data->usbmisc_data here, but in
imx_controller_suspend() we have one?
> + 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)
> 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, ..."
> + * 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."
Thanks,
Frieder
> + */
> + offset = 0;
> + } else {
> + dev_err(data->dev, "index is error for usbmisc\n");
> + offset = 0;
> + ret = -EINVAL;
> + }
> +
> + 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 ret;
> +}
> +
> +static int usbmisc_imx6_hsic_set_clk(struct imx_usbmisc_data *data, bool on)
> +{
> + 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) {
> + offset = 0;
> + } else {
> + dev_err(data->dev, "index is error for usbmisc\n");
> + offset = 0;
> + ret = -EINVAL;
> + }
> +
> + 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 +477,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 +553,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 +581,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 +592,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 +650,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",
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-17 7:02 ` [PATCH 0/4] usb: chipidea: imx: add HSIC support Frieder Schrempf
@ 2018-10-17 7:23 ` Peter Chen
2018-10-17 9:56 ` Frieder Schrempf
0 siblings, 1 reply; 31+ messages in thread
From: Peter Chen @ 2018-10-17 7:23 UTC (permalink / raw)
To: Frieder Schrempf, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
> On 16.10.18 07:01, Peter Chen wrote:
> > 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. I
> > have no HSIC interface board which is supported by upstream kernel, so
> > this patches are only compiled ok, Frieder Schrempf, would you please
> > help me test it on your board? Thanks.
>
> Thank you for providing the patch!
> I applied it to v4.19-rc8 and tested and the LAN9730 comes up and works just fine.
>
> Here is how my devicetree looks like:
>
> usbphy_dummy: usbphy_dummy@1 {
> compatible = "usb-nop-xceiv";
> };
>
> [...]
>
> &usbh2 {
> vbus-supply = <®_usb_h2_vbus>;
> pinctrl-names = "idle", "active";
> pinctrl-0 = <&pinctrl_usbh2_idle>;
> pinctrl-1 = <&pinctrl_usbh2_active>;
> fsl,usbphy = <&usbphy_dummy>;
> phy_type = "hsic";
> status = "okay";
> #address-cells = <1>;
> #size-cells = <0>;
>
> usbnet: smsc@1 {
> compatible = "usb424,9730";
> /* Filled in by U-Boot */
> mac-address = [00 00 00 00 00 00];
> reg = <1>;
> };
> };
>
> [...]
>
> pinctrl_usbh2_idle: usbh2grp-idle {
> fsl,pins = <
> MX6QDL_PAD_RGMII_TXC__USB_H2_DATA 0x40013030
> MX6QDL_PAD_RGMII_TX_CTL__USB_H2_STROBE 0x40013030
> >;
> };
>
> pinctrl_usbh2_active: usbh2grp-active {
> fsl,pins = <
> MX6QDL_PAD_RGMII_TXC__USB_H2_DATA 0x40013030
> MX6QDL_PAD_RGMII_TX_CTL__USB_H2_STROBE 0x40017030
> >;
> };
>
>
> Are there any test cases I should try?
> How can I test suspend/resume?
>
- System suspend/resume
1. Enable USB wakeup
for i in $(find /sys -name wakeup | grep usb);do echo enabled > $i;echo "echo enabled > $i";done;
2. Let the system enter suspend using below command
echo mem > /sys/power/state
3. And see if there is a wakeup block system entering suspend, and check if USB HSIC works ok
after system resume
- Runtime suspend
1. Enable auto suspend for all USB devices, and check if USBOH3 clock is closed,
make sure do not plug any ethernet cable on the RJ45 port.
/* Enable auto suspend */
for i in $(find /sys -name control | grep usb);do echo auto > $i;echo "echo auto > $i";done;
/* Scripts to see USB clock */
root@imx8qxpmek:~# cat uclk.sh
/home/root/dump-clocks.sh | grep usb
root@imx8qxpmek:~# cat /home/root/dump-clocks.sh
#!/bin/bash
if ! mount|grep -sq '/sys/kernel/debug'; then
mount -t debugfs none /sys/kernel/debug
fi
cat /sys/kernel/debug/clk/clk_summary
2. If USB OH3 clock can close successfully, it means USB HSIC can enter suspend successfully.
3. Plug ethernet cable to see if the link can be on.
The purpose of above two tests is to see if USB HSIC can be suspended.
Thanks.
Peter
> I also have some suggestions for your patch. Please see the separate email.
>
> Thanks,
> Frieder
>
> >
> > 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 | 1 +
> > drivers/usb/chipidea/ci_hdrc_imx.c | 153 ++++++++++++++++++---
> > drivers/usb/chipidea/ci_hdrc_imx.h | 9 +-
> > drivers/usb/chipidea/host.c | 98 +++++++++++++
> > drivers/usb/chipidea/usbmisc_imx.c | 131 ++++++++++++++++++
> > include/linux/usb/chipidea.h | 3 +
> > 6 files changed, 376 insertions(+), 19 deletions(-)
> >
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-17 7:23 ` Peter Chen
@ 2018-10-17 9:56 ` Frieder Schrempf
2018-10-17 11:04 ` Frieder Schrempf
0 siblings, 1 reply; 31+ messages in thread
From: Frieder Schrempf @ 2018-10-17 9:56 UTC (permalink / raw)
To: Peter Chen, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
Hi Peter,
On 17.10.18 09:23, Peter Chen wrote:
>
>> On 16.10.18 07:01, Peter Chen wrote:
>>> 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. I
>>> have no HSIC interface board which is supported by upstream kernel, so
>>> this patches are only compiled ok, Frieder Schrempf, would you please
>>> help me test it on your board? Thanks.
>>
>> Thank you for providing the patch!
>> I applied it to v4.19-rc8 and tested and the LAN9730 comes up and works just fine.
>>
>> Here is how my devicetree looks like:
>>
>> usbphy_dummy: usbphy_dummy@1 {
>> compatible = "usb-nop-xceiv";
>> };
>>
>> [...]
>>
>> &usbh2 {
>> vbus-supply = <®_usb_h2_vbus>;
>> pinctrl-names = "idle", "active";
>> pinctrl-0 = <&pinctrl_usbh2_idle>;
>> pinctrl-1 = <&pinctrl_usbh2_active>;
>> fsl,usbphy = <&usbphy_dummy>;
>> phy_type = "hsic";
>> status = "okay";
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> usbnet: smsc@1 {
>> compatible = "usb424,9730";
>> /* Filled in by U-Boot */
>> mac-address = [00 00 00 00 00 00];
>> reg = <1>;
>> };
>> };
>>
>> [...]
>>
>> pinctrl_usbh2_idle: usbh2grp-idle {
>> fsl,pins = <
>> MX6QDL_PAD_RGMII_TXC__USB_H2_DATA 0x40013030
>> MX6QDL_PAD_RGMII_TX_CTL__USB_H2_STROBE 0x40013030
>> >;
>> };
>>
>> pinctrl_usbh2_active: usbh2grp-active {
>> fsl,pins = <
>> MX6QDL_PAD_RGMII_TXC__USB_H2_DATA 0x40013030
>> MX6QDL_PAD_RGMII_TX_CTL__USB_H2_STROBE 0x40017030
>> >;
>> };
>>
>>
>> Are there any test cases I should try?
>> How can I test suspend/resume?
>>
>
> - System suspend/resume
> 1. Enable USB wakeup
> for i in $(find /sys -name wakeup | grep usb);do echo enabled > $i;echo "echo enabled > $i";done;
> 2. Let the system enter suspend using below command
> echo mem > /sys/power/state
> 3. And see if there is a wakeup block system entering suspend, and check if USB HSIC works ok
> after system resume
System suspend/resume seems to work fine. After resume the ethernet
controller works.
root@imxceet-solo-s-43:~# echo mem > /sys/power/state
PM: suspend entry (deep)
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.001 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
smsc95xx 3-1:1.0 eth1: entering SUSPEND2 mode
PM: suspend devices took 0.050 seconds
Disabling non-boot CPUs ...
usb 3-1: reset high-speed USB device number 2 using ci_hdrc
PM: resume devices took 0.590 seconds
OOM killer enabled.
Restarting tasks ... done.
PM: suspend exit
smsc95xx 3-1:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0x4DE1
fec 2188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>
> - Runtime suspend
> 1. Enable auto suspend for all USB devices, and check if USBOH3 clock is closed,
> make sure do not plug any ethernet cable on the RJ45 port.
>
> /* Enable auto suspend */
> for i in $(find /sys -name control | grep usb);do echo auto > $i;echo "echo auto > $i";done;
This doesn't work. When the port is suspended it gets into a loop of
suspending/resuming endlessly. If i put two logs in
ci_hdrc_imx_runtime_suspend() and ci_hdrc_imx_runtime_resume(), I get this:
[...]
ci_hdrc_imx_runtime_resume:603
ci_hdrc_imx_runtime_suspend:574
ci_hdrc_imx_runtime_resume:603
ci_hdrc_imx_runtime_suspend:574
ci_hdrc_imx_runtime_resume:603
ci_hdrc_imx_runtime_suspend:574
ci_hdrc_imx_runtime_resume:603
ci_hdrc_imx_runtime_suspend:574
[...]
Any ideas how to debug?
Thanks,
Frieder
>
> /* Scripts to see USB clock */
> root@imx8qxpmek:~# cat uclk.sh
> /home/root/dump-clocks.sh | grep usb
> root@imx8qxpmek:~# cat /home/root/dump-clocks.sh
> #!/bin/bash
>
> if ! mount|grep -sq '/sys/kernel/debug'; then
> mount -t debugfs none /sys/kernel/debug
> fi
>
> cat /sys/kernel/debug/clk/clk_summary
>
> 2. If USB OH3 clock can close successfully, it means USB HSIC can enter suspend successfully.
>
> 3. Plug ethernet cable to see if the link can be on.
>
> The purpose of above two tests is to see if USB HSIC can be suspended.
>
> Thanks.
>
> Peter
>
>
>> I also have some suggestions for your patch. Please see the separate email.
>>
>> Thanks,
>> Frieder
>>
>>>
>>> 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 | 1 +
>>> drivers/usb/chipidea/ci_hdrc_imx.c | 153 ++++++++++++++++++---
>>> drivers/usb/chipidea/ci_hdrc_imx.h | 9 +-
>>> drivers/usb/chipidea/host.c | 98 +++++++++++++
>>> drivers/usb/chipidea/usbmisc_imx.c | 131 ++++++++++++++++++
>>> include/linux/usb/chipidea.h | 3 +
>>> 6 files changed, 376 insertions(+), 19 deletions(-)
>>>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-17 9:56 ` Frieder Schrempf
@ 2018-10-17 11:04 ` Frieder Schrempf
2018-10-17 14:59 ` Frieder Schrempf
0 siblings, 1 reply; 31+ messages in thread
From: Frieder Schrempf @ 2018-10-17 11:04 UTC (permalink / raw)
To: Peter Chen, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
On 17.10.18 11:56, Frieder Schrempf wrote:
> Hi Peter,
>
> On 17.10.18 09:23, Peter Chen wrote:
>>> On 16.10.18 07:01, Peter Chen wrote:
>>>> 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. I
>>>> have no HSIC interface board which is supported by upstream kernel, so
>>>> this patches are only compiled ok, Frieder Schrempf, would you please
>>>> help me test it on your board? Thanks.
>>>
>>> Thank you for providing the patch!
>>> I applied it to v4.19-rc8 and tested and the LAN9730 comes up and
>>> works just fine.
>>>
>>> Here is how my devicetree looks like:
>>>
>>> usbphy_dummy: usbphy_dummy@1 {
>>> compatible = "usb-nop-xceiv";
>>> };
>>>
>>> [...]
>>>
>>> &usbh2 {
>>> vbus-supply = <®_usb_h2_vbus>;
>>> pinctrl-names = "idle", "active";
>>> pinctrl-0 = <&pinctrl_usbh2_idle>;
>>> pinctrl-1 = <&pinctrl_usbh2_active>;
>>> fsl,usbphy = <&usbphy_dummy>;
>>> phy_type = "hsic";
>>> status = "okay";
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>>
>>> usbnet: smsc@1 {
>>> compatible = "usb424,9730";
>>> /* Filled in by U-Boot */
>>> mac-address = [00 00 00 00 00 00];
>>> reg = <1>;
>>> };
>>> };
>>>
>>> [...]
>>>
>>> pinctrl_usbh2_idle: usbh2grp-idle {
>>> fsl,pins = <
>>> MX6QDL_PAD_RGMII_TXC__USB_H2_DATA 0x40013030
>>> MX6QDL_PAD_RGMII_TX_CTL__USB_H2_STROBE 0x40013030
>>> >;
>>> };
>>>
>>> pinctrl_usbh2_active: usbh2grp-active {
>>> fsl,pins = <
>>> MX6QDL_PAD_RGMII_TXC__USB_H2_DATA 0x40013030
>>> MX6QDL_PAD_RGMII_TX_CTL__USB_H2_STROBE 0x40017030
>>> >;
>>> };
>>>
>>>
>>> Are there any test cases I should try?
>>> How can I test suspend/resume?
>>>
>>
>> - System suspend/resume
>> 1. Enable USB wakeup
>> for i in $(find /sys -name wakeup | grep usb);do echo enabled >
>> $i;echo "echo enabled > $i";done;
>> 2. Let the system enter suspend using below command
>> echo mem > /sys/power/state
>> 3. And see if there is a wakeup block system entering suspend, and
>> check if USB HSIC works ok
>> after system resume
>
> System suspend/resume seems to work fine. After resume the ethernet
> controller works.
>
> root@imxceet-solo-s-43:~# echo mem > /sys/power/state
> PM: suspend entry (deep)
> PM: Syncing filesystems ... done.
> Freezing user space processes ... (elapsed 0.001 seconds) done.
> OOM killer disabled.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> smsc95xx 3-1:1.0 eth1: entering SUSPEND2 mode
> PM: suspend devices took 0.050 seconds
> Disabling non-boot CPUs ...
> usb 3-1: reset high-speed USB device number 2 using ci_hdrc
> PM: resume devices took 0.590 seconds
> OOM killer enabled.
> Restarting tasks ... done.
> PM: suspend exit
> smsc95xx 3-1:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0x4DE1
> fec 2188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>
>>
>> - Runtime suspend
>> 1. Enable auto suspend for all USB devices, and check if USBOH3 clock
>> is closed,
>> make sure do not plug any ethernet cable on the RJ45 port.
>>
>> /* Enable auto suspend */
>> for i in $(find /sys -name control | grep usb);do echo auto > $i;echo
>> "echo auto > $i";done;
>
> This doesn't work. When the port is suspended it gets into a loop of
> suspending/resuming endlessly. If i put two logs in
> ci_hdrc_imx_runtime_suspend() and ci_hdrc_imx_runtime_resume(), I get this:
>
> [...]
> ci_hdrc_imx_runtime_resume:603
> ci_hdrc_imx_runtime_suspend:574
> ci_hdrc_imx_runtime_resume:603
> ci_hdrc_imx_runtime_suspend:574
> ci_hdrc_imx_runtime_resume:603
> ci_hdrc_imx_runtime_suspend:574
> ci_hdrc_imx_runtime_resume:603
> ci_hdrc_imx_runtime_suspend:574
> [...]
Ok, forget about the loop, this was caused by one of the other ports,
that had issues with overcurrent detection.
But still it doesn't work for the HSIC port.
The HSIC device is stuck in status "suspending" (note: "suspending" and
not "suspended"):
~# cat /sys/bus/usb/devices/usb1/power/runtime_status
suspended
~# cat /sys/bus/usb/devices/usb2/power/runtime_status
suspended
~# cat /sys/bus/usb/devices/usb3/power/runtime_status
active
~# cat /sys/bus/usb/devices/usb3/3-1/power/runtime_status
suspending
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-17 11:04 ` Frieder Schrempf
@ 2018-10-17 14:59 ` Frieder Schrempf
2018-10-18 1:22 ` Peter Chen
0 siblings, 1 reply; 31+ messages in thread
From: Frieder Schrempf @ 2018-10-17 14:59 UTC (permalink / raw)
To: Peter Chen, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
On 17.10.18 13:04, Frieder Schrempf wrote:
> On 17.10.18 11:56, Frieder Schrempf wrote:
[...]
>>> - System suspend/resume
>>> 1. Enable USB wakeup
>>> for i in $(find /sys -name wakeup | grep usb);do echo enabled >
>>> $i;echo "echo enabled > $i";done;
>>> 2. Let the system enter suspend using below command
>>> echo mem > /sys/power/state
>>> 3. And see if there is a wakeup block system entering suspend, and
>>> check if USB HSIC works ok
>>> after system resume
>>
>> System suspend/resume seems to work fine. After resume the ethernet
>> controller works.
>>
>> root@imxceet-solo-s-43:~# echo mem > /sys/power/state
>> PM: suspend entry (deep)
>> PM: Syncing filesystems ... done.
>> Freezing user space processes ... (elapsed 0.001 seconds) done.
>> OOM killer disabled.
>> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>> smsc95xx 3-1:1.0 eth1: entering SUSPEND2 mode
>> PM: suspend devices took 0.050 seconds
>> Disabling non-boot CPUs ...
>> usb 3-1: reset high-speed USB device number 2 using ci_hdrc
>> PM: resume devices took 0.590 seconds
>> OOM killer enabled.
>> Restarting tasks ... done.
>> PM: suspend exit
>> smsc95xx 3-1:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0x4DE1
>> fec 2188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>>
>>>
>>> - Runtime suspend
>>> 1. Enable auto suspend for all USB devices, and check if USBOH3 clock
>>> is closed,
>>> make sure do not plug any ethernet cable on the RJ45 port.
>>>
>>> /* Enable auto suspend */
>>> for i in $(find /sys -name control | grep usb);do echo auto >
>>> $i;echo "echo auto > $i";done;
>>
>> This doesn't work. When the port is suspended it gets into a loop of
>> suspending/resuming endlessly. If i put two logs in
>> ci_hdrc_imx_runtime_suspend() and ci_hdrc_imx_runtime_resume(), I get
>> this:
>>
>> [...]
>> ci_hdrc_imx_runtime_resume:603
>> ci_hdrc_imx_runtime_suspend:574
>> ci_hdrc_imx_runtime_resume:603
>> ci_hdrc_imx_runtime_suspend:574
>> ci_hdrc_imx_runtime_resume:603
>> ci_hdrc_imx_runtime_suspend:574
>> ci_hdrc_imx_runtime_resume:603
>> ci_hdrc_imx_runtime_suspend:574
>> [...]
>
> Ok, forget about the loop, this was caused by one of the other ports,
> that had issues with overcurrent detection.
>
> But still it doesn't work for the HSIC port.
>
> The HSIC device is stuck in status "suspending" (note: "suspending" and
> not "suspended"):
>
> ~# cat /sys/bus/usb/devices/usb1/power/runtime_status
> suspended
> ~# cat /sys/bus/usb/devices/usb2/power/runtime_status
> suspended
> ~# cat /sys/bus/usb/devices/usb3/power/runtime_status
> active
> ~# cat /sys/bus/usb/devices/usb3/3-1/power/runtime_status
> suspending
It seems like this is a problem with the smsc95xx driver. I fixed this
and now the device is suspending properly, when the network interface is
down and resumes when it comes up.
I also checked the USBOH3 clock and its properly switched on and off.
What didn't work is auto suspend and resume on ethernet link down/up,
but this seems to be a restriction of the smsc95xx/usbnet driver.
So all in all this looks good to me. Please let me know if you need any
more information or tests.
Thanks,
Frieder
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-18 1:18 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-18 1:18 UTC (permalink / raw)
To: Frieder Schrempf, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
> > +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
> > +
> > + 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.
> > }
> > @@ -367,9 +470,19 @@ 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__);
> >
> > + 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.
> > + 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)
> > 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.
> > + 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)
> > 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, ..."
>
> > + * 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [2/4] usb: chipidea: imx: add HSIC support
@ 2018-10-18 1:18 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-18 1:18 UTC (permalink / raw)
To: Frieder Schrempf, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
> > +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
> > +
> > + 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.
> > }
> > @@ -367,9 +470,19 @@ 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__);
> >
> > + 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.
> > + 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)
> > 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.
> > + 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)
> > 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, ..."
>
> > + * 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-17 14:59 ` Frieder Schrempf
@ 2018-10-18 1:22 ` Peter Chen
2018-10-18 7:46 ` Frieder Schrempf
0 siblings, 1 reply; 31+ messages in thread
From: Peter Chen @ 2018-10-18 1:22 UTC (permalink / raw)
To: Frieder Schrempf, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
> >>> - System suspend/resume
> >>> 1. Enable USB wakeup
> >>> for i in $(find /sys -name wakeup | grep usb);do echo enabled >
> >>> $i;echo "echo enabled > $i";done; 2. Let the system enter suspend
> >>> using below command echo mem > /sys/power/state 3. And see if there
> >>> is a wakeup block system entering suspend, and check if USB HSIC
> >>> works ok after system resume
> >>
> >> System suspend/resume seems to work fine. After resume the ethernet
> >> controller works.
> >>
> >> root@imxceet-solo-s-43:~# echo mem > /sys/power/state
> >> PM: suspend entry (deep)
> >> PM: Syncing filesystems ... done.
> >> Freezing user space processes ... (elapsed 0.001 seconds) done.
> >> OOM killer disabled.
> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> >> smsc95xx 3-1:1.0 eth1: entering SUSPEND2 mode
> >> PM: suspend devices took 0.050 seconds Disabling non-boot CPUs ...
> >> usb 3-1: reset high-speed USB device number 2 using ci_hdrc
> >> PM: resume devices took 0.590 seconds OOM killer enabled.
> >> Restarting tasks ... done.
> >> PM: suspend exit
> >> smsc95xx 3-1:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0x4DE1 fec
> >> 2188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> >>
> >>>
> >>> - Runtime suspend
> >>> 1. Enable auto suspend for all USB devices, and check if USBOH3
> >>> clock is closed, make sure do not plug any ethernet cable on the
> >>> RJ45 port.
> >>>
> >>> /* Enable auto suspend */
> >>> for i in $(find /sys -name control | grep usb);do echo auto >
> >>> $i;echo "echo auto > $i";done;
> >>
> >> This doesn't work. When the port is suspended it gets into a loop of
> >> suspending/resuming endlessly. If i put two logs in
> >> ci_hdrc_imx_runtime_suspend() and ci_hdrc_imx_runtime_resume(), I get
> >> this:
> >>
> >> [...]
> >> ci_hdrc_imx_runtime_resume:603
> >> ci_hdrc_imx_runtime_suspend:574
> >> ci_hdrc_imx_runtime_resume:603
> >> ci_hdrc_imx_runtime_suspend:574
> >> ci_hdrc_imx_runtime_resume:603
> >> ci_hdrc_imx_runtime_suspend:574
> >> ci_hdrc_imx_runtime_resume:603
> >> ci_hdrc_imx_runtime_suspend:574
> >> [...]
> >
> > Ok, forget about the loop, this was caused by one of the other ports,
> > that had issues with overcurrent detection.
> >
> > But still it doesn't work for the HSIC port.
> >
> > The HSIC device is stuck in status "suspending" (note: "suspending"
> > and not "suspended"):
> >
> > ~# cat /sys/bus/usb/devices/usb1/power/runtime_status
> > suspended
> > ~# cat /sys/bus/usb/devices/usb2/power/runtime_status
> > suspended
> > ~# cat /sys/bus/usb/devices/usb3/power/runtime_status
> > active
> > ~# cat /sys/bus/usb/devices/usb3/3-1/power/runtime_status
> > suspending
>
> It seems like this is a problem with the smsc95xx driver. I fixed this and now the
> device is suspending properly, when the network interface is down and resumes
> when it comes up.
>
> I also checked the USBOH3 clock and its properly switched on and off.
>
> What didn't work is auto suspend and resume on ethernet link down/up, but this
> seems to be a restriction of the smsc95xx/usbnet driver.
>
> So all in all this looks good to me. Please let me know if you need any more
> information or tests.
>
Thanks, Frieder, no more tests are needed.
You could send me your dts changes as patches, I will append it at my v2 patch series.
Peter
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-18 1:22 ` Peter Chen
@ 2018-10-18 7:46 ` Frieder Schrempf
2018-10-18 8:48 ` Peter Chen
0 siblings, 1 reply; 31+ messages in thread
From: Frieder Schrempf @ 2018-10-18 7:46 UTC (permalink / raw)
To: Peter Chen, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
On 18.10.18 03:22, Peter Chen wrote:
>
>>>>> - System suspend/resume
>>>>> 1. Enable USB wakeup
>>>>> for i in $(find /sys -name wakeup | grep usb);do echo enabled >
>>>>> $i;echo "echo enabled > $i";done; 2. Let the system enter suspend
>>>>> using below command echo mem > /sys/power/state 3. And see if there
>>>>> is a wakeup block system entering suspend, and check if USB HSIC
>>>>> works ok after system resume
>>>>
>>>> System suspend/resume seems to work fine. After resume the ethernet
>>>> controller works.
>>>>
>>>> root@imxceet-solo-s-43:~# echo mem > /sys/power/state
>>>> PM: suspend entry (deep)
>>>> PM: Syncing filesystems ... done.
>>>> Freezing user space processes ... (elapsed 0.001 seconds) done.
>>>> OOM killer disabled.
>>>> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>>>> smsc95xx 3-1:1.0 eth1: entering SUSPEND2 mode
>>>> PM: suspend devices took 0.050 seconds Disabling non-boot CPUs ...
>>>> usb 3-1: reset high-speed USB device number 2 using ci_hdrc
>>>> PM: resume devices took 0.590 seconds OOM killer enabled.
>>>> Restarting tasks ... done.
>>>> PM: suspend exit
>>>> smsc95xx 3-1:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0x4DE1 fec
>>>> 2188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>>>>
>>>>>
>>>>> - Runtime suspend
>>>>> 1. Enable auto suspend for all USB devices, and check if USBOH3
>>>>> clock is closed, make sure do not plug any ethernet cable on the
>>>>> RJ45 port.
>>>>>
>>>>> /* Enable auto suspend */
>>>>> for i in $(find /sys -name control | grep usb);do echo auto >
>>>>> $i;echo "echo auto > $i";done;
>>>>
>>>> This doesn't work. When the port is suspended it gets into a loop of
>>>> suspending/resuming endlessly. If i put two logs in
>>>> ci_hdrc_imx_runtime_suspend() and ci_hdrc_imx_runtime_resume(), I get
>>>> this:
>>>>
>>>> [...]
>>>> ci_hdrc_imx_runtime_resume:603
>>>> ci_hdrc_imx_runtime_suspend:574
>>>> ci_hdrc_imx_runtime_resume:603
>>>> ci_hdrc_imx_runtime_suspend:574
>>>> ci_hdrc_imx_runtime_resume:603
>>>> ci_hdrc_imx_runtime_suspend:574
>>>> ci_hdrc_imx_runtime_resume:603
>>>> ci_hdrc_imx_runtime_suspend:574
>>>> [...]
>>>
>>> Ok, forget about the loop, this was caused by one of the other ports,
>>> that had issues with overcurrent detection.
>>>
>>> But still it doesn't work for the HSIC port.
>>>
>>> The HSIC device is stuck in status "suspending" (note: "suspending"
>>> and not "suspended"):
>>>
>>> ~# cat /sys/bus/usb/devices/usb1/power/runtime_status
>>> suspended
>>> ~# cat /sys/bus/usb/devices/usb2/power/runtime_status
>>> suspended
>>> ~# cat /sys/bus/usb/devices/usb3/power/runtime_status
>>> active
>>> ~# cat /sys/bus/usb/devices/usb3/3-1/power/runtime_status
>>> suspending
>>
>> It seems like this is a problem with the smsc95xx driver. I fixed this and now the
>> device is suspending properly, when the network interface is down and resumes
>> when it comes up.
>>
>> I also checked the USBOH3 clock and its properly switched on and off.
>>
>> What didn't work is auto suspend and resume on ethernet link down/up, but this
>> seems to be a restriction of the smsc95xx/usbnet driver.
>>
>> So all in all this looks good to me. Please let me know if you need any more
>> information or tests.
>>
>
> Thanks, Frieder, no more tests are needed.
> You could send me your dts changes as patches, I will append it at my v2 patch series.
My board is currently off-tree so I can't send any patch for the pinmux
settings in devicetree. But I will send a patch with the changes that
should go to imx6qdl.dtsi, imx6sl.dtsi and imx6sx.dtsi. Though I only
tested on i.MX6S.
Thanks,
Frieder
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-18 7:46 ` Frieder Schrempf
@ 2018-10-18 8:48 ` Peter Chen
2018-10-18 8:56 ` Frieder Schrempf
0 siblings, 1 reply; 31+ messages in thread
From: Peter Chen @ 2018-10-18 8:48 UTC (permalink / raw)
To: Frieder Schrempf, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
> >
> > Thanks, Frieder, no more tests are needed.
> > You could send me your dts changes as patches, I will append it at my v2 patch
> series.
>
> My board is currently off-tree so I can't send any patch for the pinmux settings in
> devicetree. But I will send a patch with the changes that should go to imx6qdl.dtsi,
> imx6sl.dtsi and imx6sx.dtsi. Though I only tested on i.MX6S.
>
No, the changes are board level specific, you could not add it into SoC file.
Peter
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-18 8:48 ` Peter Chen
@ 2018-10-18 8:56 ` Frieder Schrempf
2018-10-18 9:04 ` Peter Chen
0 siblings, 1 reply; 31+ messages in thread
From: Frieder Schrempf @ 2018-10-18 8:56 UTC (permalink / raw)
To: Peter Chen, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
On 18.10.18 10:48, Peter Chen wrote:
>
>>>
>>> Thanks, Frieder, no more tests are needed.
>>> You could send me your dts changes as patches, I will append it at my v2 patch
>> series.
>>
>> My board is currently off-tree so I can't send any patch for the pinmux settings in
>> devicetree. But I will send a patch with the changes that should go to imx6qdl.dtsi,
>> imx6sl.dtsi and imx6sx.dtsi. Though I only tested on i.MX6S.
>>
>
> No, the changes are board level specific, you could not add it into SoC file.
Not all changes are board specific. The HSIC-only host controllers can
only be used to interface with on-board chips via HSIC. So they need a
"usb-nop-xceiv" dummy PHY in all use cases.
So if I understand this correctly this should be done in the SoCs dtsi
files like in this patch I just sent: [1].
Please also note that for i.MX7S this is already added in imx7s.dtsi: [2]
Thanks,
Frieder
[1]: https://lkml.org/lkml/2018/10/18/414
[2]:
https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/imx7s.dtsi#L88
^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 0/4] usb: chipidea: imx: add HSIC support
2018-10-18 8:56 ` Frieder Schrempf
@ 2018-10-18 9:04 ` Peter Chen
0 siblings, 0 replies; 31+ messages in thread
From: Peter Chen @ 2018-10-18 9:04 UTC (permalink / raw)
To: Frieder Schrempf, linux-usb; +Cc: dl-linux-imx, robh+dt, devicetree
> >> My board is currently off-tree so I can't send any patch for the
> >> pinmux settings in devicetree. But I will send a patch with the
> >> changes that should go to imx6qdl.dtsi, imx6sl.dtsi and imx6sx.dtsi. Though I
> only tested on i.MX6S.
> >>
> >
> > No, the changes are board level specific, you could not add it into SoC file.
>
> Not all changes are board specific. The HSIC-only host controllers can only be used
> to interface with on-board chips via HSIC. So they need a "usb-nop-xceiv" dummy
> PHY in all use cases.
>
> So if I understand this correctly this should be done in the SoCs dtsi files like in this
> patch I just sent: [1].
>
> Please also note that for i.MX7S this is already added in imx7s.dtsi: [2]
>
Oh, I have misunderstood your meaning, the [1] is needed since it is for SoC.
I thought your board level changes (like pinctrl) would go into SoC dtsi file.
If there are no more comments, I will send v2 next week, feel free to add your
reviewed-by and tested-by tag, thanks.
Peter
>
> [1]:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2
> Flkml%2F2018%2F10%2F18%2F414&data=02%7C01%7Cpeter.chen%40nxp.
> com%7C3c73fdf4ec98477da49308d634d78b39%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C636754497687793991&sdata=aHhMhaj%2FbdTFu0B
> qXi0loiBh%2BHzl%2F%2BTSw3txyjBSNLQ%3D&reserved=0
> [2]:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com
> %2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Farch%2Farm%2Fboot%2Fdts%2Fim
> x7s.dtsi%23L88&data=02%7C01%7Cpeter.chen%40nxp.com%7C3c73fdf4ec9
> 8477da49308d634d78b39%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C636754497687793991&sdata=7cVhAMQpWWYjGHjY8Xh2g0CG3jPaeG8X
> 9K368AooQcc%3D&reserved=0
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2018-10-18 17:05 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-16 5:01 [PATCH 0/4] usb: chipidea: imx: add HSIC support Peter Chen
2018-10-16 5:01 ` [PATCH 1/4] usb: chipidea: add flag for imx hsic implementation Peter Chen
2018-10-16 5:01 ` [1/4] " Peter Chen
2018-10-16 5:01 ` [PATCH 2/4] usb: chipidea: imx: add HSIC support Peter Chen
2018-10-16 5:01 ` [2/4] " Peter Chen
2018-10-16 5:52 ` [PATCH 2/4] " kbuild test robot
2018-10-16 5:52 ` [2/4] " kbuild test robot
2018-10-16 6:07 ` [PATCH 2/4] " Peter Chen
2018-10-16 6:07 ` [2/4] " Peter Chen
2018-10-17 7:03 ` [PATCH 2/4] " Frieder Schrempf
2018-10-17 7:03 ` [2/4] " Frieder Schrempf
2018-10-18 1:18 ` [PATCH 2/4] " Peter Chen
2018-10-18 1:18 ` [2/4] " Peter Chen
2018-10-16 5:01 ` [PATCH 3/4] usb: chipidea: host: override ehci->hub_control Peter Chen
2018-10-16 5:01 ` [3/4] " Peter Chen
2018-10-16 5:01 ` [PATCH 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups Peter Chen
2018-10-16 5:01 ` [4/4] " Peter Chen
2018-10-16 16:24 ` [PATCH 4/4] " Fabio Estevam
2018-10-16 16:24 ` [4/4] " Fabio Estevam
2018-10-17 1:04 ` [PATCH 4/4] " Peter Chen
2018-10-17 1:04 ` [4/4] " Peter Chen
2018-10-17 7:02 ` [PATCH 0/4] usb: chipidea: imx: add HSIC support Frieder Schrempf
2018-10-17 7:23 ` Peter Chen
2018-10-17 9:56 ` Frieder Schrempf
2018-10-17 11:04 ` Frieder Schrempf
2018-10-17 14:59 ` Frieder Schrempf
2018-10-18 1:22 ` Peter Chen
2018-10-18 7:46 ` Frieder Schrempf
2018-10-18 8:48 ` Peter Chen
2018-10-18 8:56 ` Frieder Schrempf
2018-10-18 9:04 ` Peter Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.