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