Re: [PATCH 4/4] usb: chipidea: imx: set over current polarity per dts setting

[Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Tue, Jul 19, 2016 at 02:31:41AM +0000, Jun Li wrote:
> Hi
> 
> > -----Original Message-----
> > From: Peter Chen [mailto:hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> > Sent: Tuesday, July 19, 2016 9:57 AM
> > To: Jun Li <jun.li-3arQi8VN3Tc@public.gmane.org>
> > Cc: Peter Chen <peter.chen-3arQi8VN3Tc@public.gmane.org>; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> > shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > Subject: Re: [PATCH 4/4] usb: chipidea: imx: set over current polarity per
> > dts setting
> > 
> > On Mon, Jul 18, 2016 at 07:15:47PM +0800, Li Jun wrote:
> > > With over-current-polarity property added, imx usb over current
> > > polarity can be configed to be low or high active, since the default
> > > setting value(0) is for active high, so keep this setting for those
> > > legacy platforms without this property specified.
> > >
> > > Signed-off-by: Li Jun <jun.li-3arQi8VN3Tc@public.gmane.org>
> > > ---
> > >  drivers/usb/chipidea/ci_hdrc_imx.c |  9 +++++++++
> > > drivers/usb/chipidea/ci_hdrc_imx.h |  1 +
> > > drivers/usb/chipidea/usbmisc_imx.c | 30 +++++++++++++++++++++++++-----
> > >  3 files changed, 35 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index dedc33e..61e712b 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > @@ -140,6 +140,15 @@ static struct imx_usbmisc_data
> > *usbmisc_get_init_data(struct device *dev)
> > >  	if (of_find_property(np, "disable-over-current", NULL))
> > >  		data->disable_oc = 1;
> > >
> > > +	if (!of_property_read_u32(np, "over-current-polarity", &ret))
> > > +		data->oc_polarity = ret ? 1 : 0;
> > > +	else
> > > +		/*
> > > +		 * Keep the oc polarity setting of legacy
> > > +		 * platforms unchanged.
> > > +		 */
> > > +		data->oc_polarity = 1;
> > 
> > We may can't ensure that, since there are lots of i.mx platforms, eg from
> > imx27 to imx7.
> > 
> > If the user does not assign oc polarity at DT, we need to read it from
> > register.
> 
> I suppose old platforms (other than i.mx6&7) should set either
> "over-current-polarity" or "disable-over-current" if want to use
> "data->oc_polarity".
>  

The user may use old dtb and the latest kernel, so the introduced
DT property should not affect old dtb platforms.

> Do you mean read the register value before set "data->oc_polarity" here?

yes, if there is a dts property, read it from DT; else, read it from the register.

if (!of_property_read_u32(np, "over-current-polarity", &ret))
	data->oc_polarity = ret ? 1 : 0;
else
	/*
	 * Keep the oc polarity setting of legacy
	 * platforms unchanged.
	 */
	data->oc_polarity = readl(oc_polarity);

Then, when you set register for oc polarity, you can depend on the
flag.
	reg = readl(usbmisc->base + data->index * 4);
	value = data->oc_polarity ? MX6_BM_OVER_CUR_POLARITY : 0;
	writel(reg | value, usbmisc->base + data->index * 4);
	

> Even with that, as I still can't ensure all i.mx platforms have the same
> mapping:
> 0 <--> active high.
> 1 <--> active low.
> How should I set "data->oc_polarity" accordingly?

Just keep current register value unchanging if there is no dts property.

Peter

> Li Jun
> > 
> > Peter
> > > +
> > >  	if (of_find_property(np, "external-vbus-divider", NULL))
> > >  		data->evdo = 1;
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > index 635717e..409aa5ca8 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > @@ -17,6 +17,7 @@ struct imx_usbmisc_data {
> > >  	int index;
> > >
> > >  	unsigned int disable_oc:1; /* over current detect disabled */
> > > +	unsigned int oc_polarity:1; /* over current polarity if oc enabled
> > > +*/
> > >  	unsigned int evdo:1; /* set external vbus divider option */  };
> > >
> > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c
> > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > index ab8b027..193dbe4 100644
> > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > @@ -56,6 +56,7 @@
> > >
> > >  #define MX6_BM_NON_BURST_SETTING	BIT(1)
> > >  #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_ID_WAKEUP		BIT(16)
> > >  #define MX6_BM_VBUS_WAKEUP		BIT(17)
> > > @@ -266,11 +267,18 @@ static int usbmisc_imx6q_init(struct
> > > imx_usbmisc_data *data)
> > >
> > >  	spin_lock_irqsave(&usbmisc->lock, flags);
> > >
> > > +	reg = readl(usbmisc->base + data->index * 4);
> > >  	if (data->disable_oc) {
> > > -		reg = readl(usbmisc->base + data->index * 4);
> > > -		writel(reg | MX6_BM_OVER_CUR_DIS,
> > > -			usbmisc->base + data->index * 4);
> > > +		reg |= MX6_BM_OVER_CUR_DIS;
> > > +	} else if (data->oc_polarity == 1) {
> > > +		/* High active */
> > > +		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
> > > +	} else {
> > > +		/* Low active */
> > > +		reg &= ~MX6_BM_OVER_CUR_DIS;
> > > +		reg |= MX6_BM_OVER_CUR_POLARITY;
> > >  	}
> > > +	writel(reg, usbmisc->base + data->index * 4);
> > >
> > >  	/* SoC non-burst setting */
> > >  	reg = readl(usbmisc->base + data->index * 4); @@ -365,10 +373,18 @@
> > > static int usbmisc_imx7d_init(struct imx_usbmisc_data *data)
> > >  		return -EINVAL;
> > >
> > >  	spin_lock_irqsave(&usbmisc->lock, flags);
> > > +	reg = readl(usbmisc->base);
> > >  	if (data->disable_oc) {
> > > -		reg = readl(usbmisc->base);
> > > -		writel(reg | MX6_BM_OVER_CUR_DIS, usbmisc->base);
> > > +		reg |= MX6_BM_OVER_CUR_DIS;
> > > +	} else if (data->oc_polarity == 1) {
> > > +		/* High active */
> > > +		reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
> > > +	} else {
> > > +		/* Low active */
> > > +		reg &= ~MX6_BM_OVER_CUR_DIS;
> > > +		reg |= MX6_BM_OVER_CUR_POLARITY;
> > >  	}
> > > +	writel(reg, usbmisc->base);
> > >
> > >  	reg = readl(usbmisc->base + MX7D_USBNC_USB_CTRL2);
> > >  	reg &= ~MX7D_USB_VBUS_WAKEUP_SOURCE_MASK;
> > > @@ -492,6 +508,10 @@ static const struct of_device_id
> > usbmisc_imx_dt_ids[] = {
> > >  		.compatible = "fsl,imx6ul-usbmisc",
> > >  		.data = &imx6sx_usbmisc_ops,
> > >  	},
> > > +	{
> > > +		.compatible = "fsl,imx7d-usbmisc",
> > > +		.data = &imx7d_usbmisc_ops,
> > > +	},
> > >  	{ /* sentinel */ }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
> > > --
> > > 1.9.1
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> > --
> > 
> > Best Regards,
> > Peter Chen

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html