Re: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

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

 



On Mon, Dec 03, 2018 at 09:02:05AM +0000, PETER CHEN wrote:
>  
> > 
> > On Mon, Dec 03, 2018 at 03:13:01AM +0000, PETER CHEN wrote:
> > > The current OC (Over Current) handling does not consider the default
> > > and bootloader OC setting well, in this commit, we reset OC setting
> > > according to dts value:
> > > - If property "disable-over-current" is set, we will disable OC at
> > > code; otherwise, we will enable OC.
> > > - Since most of USB power control chips are low active for OC, we keep
> > > "over-current-active-high" property unchanging to reduce users effort.
> > > If this property is set, we set OC polarity as high explicitly;
> > > otherwise, we set it as low explicitly.
> > >
> > > Cc: stable <stable@xxxxxxxxxxxxxxx>
> > > Cc: Peter Chen <peter.chen@xxxxxxx>
> > > Cc: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > Cc: Matthew Starr <mstarr@xxxxxxxxxxxxx>
> > > Signed-off-by: Peter Chen <peter.chen@xxxxxxx>
> > > ---
> > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > > drivers/usb/chipidea/usbmisc_imx.c | 27 ++++++++++++++++++++++-----
> > >  3 files changed, 25 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index 56781c329db0..058468f2de8d 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> > *usbmisc_get_init_data(struct device *dev)
> > >  		data->disable_oc = 1;
> > >
> > >  	if (of_find_property(np, "over-current-active-high", NULL))
> > > -		data->oc_polarity = 1;
> > > +		data->oc_polarity_high = 1;
> > >
> > >  	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 fcecab478934..5ea8bb239b38 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > @@ -11,7 +11,8 @@ 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 */
> > > +	/* over current polarity high if oc enabled */
> > > +	unsigned int oc_polarity_high:1;
> > >  	unsigned int evdo:1; /* set external vbus divider option */
> > >  	unsigned int ulpi:1; /* connected to an ULPI phy */
> > >  	unsigned int hsic:1; /* HSIC controlller */ diff --git
> > > a/drivers/usb/chipidea/usbmisc_imx.c
> > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > index 43a15a6e86f5..30d1ada952e1 100644
> > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data
> > *data)
> > >  		val = readl(usbmisc->base);
> > >  		val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
> > >  		val |= (MX25_EHCI_INTERFACE_DIFF_UNI &
> > MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
> > > -		val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
> > > +		val |= MX25_OTG_PM_BIT;
> > > +		if (data->oc_polarity_high)
> > > +			/* High active */
> > > +			val |= MX25_OTG_OCPOL_BIT;
> > > +		else
> > > +			val &= ~MX25_OTG_OCPOL_BIT;
> > > +
> > 
> > I think we shouldn't change the i.MX25 behaviour in a patch that is backported to
> > stable. Even without this I'd put the change to i.MX25 in a separate patch. (As I did
> > in my series.)
> > 
> > Also the change is bad, because you're going from hard-coded active high to active
> > low for all users that don't specify "over-current-active-high".
> > This breaks (I think) most i.MX25 machines.
> > 
> > Affected in mainline are:
> > 
> > 	imx25-pdk.dts (&usbotg + &usbhost1)
> > 	imx25-eukrea-mbimxsd25-baseboard.dts (&usbotg + &usbhost1)
> > 
> > and out of tree probably more. (But maybe these use really active low signalling? I
> > don't know and didn't check.)
> 
> Are you sure the boards you listed works well for OC now?

No I'm not. That's why I added the question in parentesis.

> Both you and Matthew posted patch for OC issue, I think you both
> probably find the OC function works not well. I have checked the
> boards (from imx35) at Freescale/NXP internal, all boards are active
> low for OC. 

I think it is unfortunate to have linux default to active low (in the
absense of an explicit configuration) while the reset default is active
high.

> My intention is to let most of boards work well without adding dts property since most of
> USB power control chip is active low for OC, the OC function should not test well before.
> 
> Your patches do not break current setting, but need the users to add dts property
> to let OC function work (Since most of chip are active low for OC), almost all i.mx
> boards will see the warning message for without setting OC polarity, and must
> add dts property to fix this warning message. 

You seem to consider having to explicitly define the polarity a
downside. I'd say it is an advantage have this information in the
machine description.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |



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

  Powered by Linux