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

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

 



Hello,

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.)

Also for i.MX6/7 the overhead isn't that bad to keep compatibility and
give the possibility to explicitly specify the polarity. So I still
prefer my patch set over your approach (which conceptually matches the
patch created by Matthew).

Best regards
Uwe

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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux