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@vger.kernel.org Cc: Peter Chen peter.chen@nxp.com Cc: Uwe Kleine-König u.kleine-koenig@pengutronix.de Cc: Matthew Starr mstarr@hedonline.com Signed-off-by: Peter Chen peter.chen@nxp.com
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;elseval &= ~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