> > +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
