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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux