| Message ID | 20181130205309.17232-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: chipidea: imx: unify over-current polarity handling | expand |
Hi Uwe, [Adding Matt] On Fri, Nov 30, 2018 at 6:53 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > The status quo is: > - on i.MX25 the overcurrent polarity is always explicitly configured as > active high which matches the reset default. > - on i.MX6 and i.MX7 the overcurrent polarity is active high after > reset. usbmisc_imx6q_init() and usbmisc_imx7d_init() keep the current > state (probably as setup by the bootloader or still the reset > default) unless the polarity is explicitly configured as active high > which then is configured. (So if the pin is active low and the > bootloader didn't set this up the configuration is wrong.) > > To improve the situation always configure the reset default value unless > the device tree configures the polarity (and then use this one). > > Note that as the reset default is active high on all platforms there is > no need to check for the presence of "over-current-active-high". In the > absence of "over-current-active-low" it doesn't matter if active high is > configured because that's the default or because it is configured > explicitly. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> It seems that Matt is also interested in adding the 'over-current-active-low' property: https://patchwork.kernel.org/patch/10701311/
Hello Fabio, On Fri, Nov 30, 2018 at 07:16:39PM -0200, Fabio Estevam wrote: > [Adding Matt] > > On Fri, Nov 30, 2018 at 6:53 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > The status quo is: > > - on i.MX25 the overcurrent polarity is always explicitly configured as > > active high which matches the reset default. > > - on i.MX6 and i.MX7 the overcurrent polarity is active high after > > reset. usbmisc_imx6q_init() and usbmisc_imx7d_init() keep the current > > state (probably as setup by the bootloader or still the reset > > default) unless the polarity is explicitly configured as active high > > which then is configured. (So if the pin is active low and the > > bootloader didn't set this up the configuration is wrong.) > > > > To improve the situation always configure the reset default value unless > > the device tree configures the polarity (and then use this one). > > > > Note that as the reset default is active high on all platforms there is > > no need to check for the presence of "over-current-active-high". In the > > absence of "over-current-active-low" it doesn't matter if active high is > > configured because that's the default or because it is configured > > explicitly. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > It seems that Matt is also interested in adding the > 'over-current-active-low' property: > https://patchwork.kernel.org/patch/10701311/ Looking at Matt's patch I noticed that I applied my patch on an old kernel version (4.14) :-| But I think Matt's patch is broken because in usbmisc_imx6q_init() in the last else branch MX6_BM_OVER_CUR_POLARITY isn't cleared. Other than that they don't look that different semantically. I will respin my patch on Monday. Best regards Uwe
> -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] > Sent: Friday, November 30, 2018 3:35 PM > > Hello Fabio, > > On Fri, Nov 30, 2018 at 07:16:39PM -0200, Fabio Estevam wrote: > > [Adding Matt] > > > > On Fri, Nov 30, 2018 at 6:53 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > The status quo is: > > > - on i.MX25 the overcurrent polarity is always explicitly configured as > > > active high which matches the reset default. > > > - on i.MX6 and i.MX7 the overcurrent polarity is active high after > > > reset. usbmisc_imx6q_init() and usbmisc_imx7d_init() keep the current > > > state (probably as setup by the bootloader or still the reset > > > default) unless the polarity is explicitly configured as active high > > > which then is configured. (So if the pin is active low and the > > > bootloader didn't set this up the configuration is wrong.) > > > > > > To improve the situation always configure the reset default value unless > > > the device tree configures the polarity (and then use this one). > > > > > > Note that as the reset default is active high on all platforms there is > > > no need to check for the presence of "over-current-active-high". In the > > > absence of "over-current-active-low" it doesn't matter if active high is > > > configured because that's the default or because it is configured > > > explicitly. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > It seems that Matt is also interested in adding the > > 'over-current-active-low' property: > > https://patchwork.kernel.org/patch/10701311/ > > Looking at Matt's patch I noticed that I applied my patch on an old > kernel version (4.14) :-| > > But I think Matt's patch is broken because in usbmisc_imx6q_init() in > the last else branch MX6_BM_OVER_CUR_POLARITY isn't cleared. Other > than > that they don't look that different semantically. > > I will respin my patch on Monday. > Uwe, You are correct that I missed the clearing of MX6_BM_OVER_CUR_POLARITY. Also I don't see any reason why that last else that should clear MX6_BM_OVER_CUR_POLARITY and MX6_BM_OVER_CUR_DIS shouldn't also be in the usbmisc_imx7d_init function too. I just resubmitted my patch based on 4.20-rc4 with the mentioned changes. Matt Starr
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt index 0e03344e2e8b..b733b7c4bfdc 100644 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt @@ -83,6 +83,7 @@ i.mx specific properties - disable-over-current: disable over current detect - over-current-active-high: over current signal polarity is high active, typically over current signal polarity is low active. +- over-current-active-low: over current signal polarity is low active. - external-vbus-divider: enables off-chip resistor divider for Vbus Example: diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c index 5f4a8157fad8..352bd0bfe161 100644 --- a/drivers/usb/chipidea/ci_hdrc_imx.c +++ b/drivers/usb/chipidea/ci_hdrc_imx.c @@ -141,8 +141,13 @@ 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_find_property(np, "over-current-active-high", NULL)) - data->oc_polarity = 1; + /* + * No need to check for "over-current-active-high" as this is the reset + * default for all platforms and this is configured unless "active-low" + * is specified. + */ + if (of_find_property(np, "over-current-active-low", NULL)) + data->oc_polarity_low = 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 d666c9f036ba..dffa34f3777d 100644 --- a/drivers/usb/chipidea/ci_hdrc_imx.h +++ b/drivers/usb/chipidea/ci_hdrc_imx.h @@ -17,7 +17,9 @@ 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 low if oc enabled */ + unsigned int oc_polarity_low:1; + unsigned int evdo:1; /* set external vbus divider option */ unsigned int ulpi:1; /* connected to an ULPI phy */ }; diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c index 9f4a0185dd60..89c1a0c48f80 100644 --- a/drivers/usb/chipidea/usbmisc_imx.c +++ b/drivers/usb/chipidea/usbmisc_imx.c @@ -126,6 +126,15 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data *data) 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); + + /* + * reset default for MX25_OTG_OCPOL_BIT is set (i.e. active + * high). So only unset if explicitly configured and keep set + * otherwise. + */ + if (data->oc_polarity_low) + val &= ~MX25_OTG_OCPOL_BIT; + writel(val, usbmisc->base); break; case 1: @@ -135,6 +144,14 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data *data) val |= (MX25_H1_PM_BIT | MX25_H1_OCPOL_BIT | MX25_H1_TLL_BIT | MX25_H1_USBTE_BIT | MX25_H1_IPPUE_DOWN_BIT); + /* + * reset default for MX25_H1_OCPOL_BIT is set (i.e. active + * high). So only unset if explicitly configured and keep set + * otherwise. + */ + if (data->oc_polarity_low) + val &= ~MX25_H1_OCPOL_BIT; + writel(val, usbmisc->base); break; @@ -340,9 +357,16 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data *data) reg = readl(usbmisc->base + data->index * 4); if (data->disable_oc) { reg |= MX6_BM_OVER_CUR_DIS; - } else if (data->oc_polarity == 1) { - /* High active */ + } else { reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY); + + /* + * reset default for MX6_BM_OVER_CUR_POLARITY is unset (i.e. + * active high). So only set if explicitly configured and keep + * clear otherwise. + */ + if (data->oc_polarity_low) + reg |= MX6_BM_OVER_CUR_POLARITY; } writel(reg, usbmisc->base + data->index * 4); @@ -442,9 +466,16 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data *data) reg = readl(usbmisc->base); if (data->disable_oc) { reg |= MX6_BM_OVER_CUR_DIS; - } else if (data->oc_polarity == 1) { - /* High active */ + } else { reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY); + + /* + * reset default for MX6_BM_OVER_CUR_POLARITY is unset (i.e. + * active high). So only set if explicitly configured and keep + * clear otherwise. + */ + if (data->oc_polarity_low) + reg |= MX6_BM_OVER_CUR_POLARITY; } writel(reg, usbmisc->base);
The status quo is: - on i.MX25 the overcurrent polarity is always explicitly configured as active high which matches the reset default. - on i.MX6 and i.MX7 the overcurrent polarity is active high after reset. usbmisc_imx6q_init() and usbmisc_imx7d_init() keep the current state (probably as setup by the bootloader or still the reset default) unless the polarity is explicitly configured as active high which then is configured. (So if the pin is active low and the bootloader didn't set this up the configuration is wrong.) To improve the situation always configure the reset default value unless the device tree configures the polarity (and then use this one). Note that as the reset default is active high on all platforms there is no need to check for the presence of "over-current-active-high". In the absence of "over-current-active-low" it doesn't matter if active high is configured because that's the default or because it is configured explicitly. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 1 + drivers/usb/chipidea/ci_hdrc_imx.c | 9 ++++- drivers/usb/chipidea/ci_hdrc_imx.h | 4 +- drivers/usb/chipidea/usbmisc_imx.c | 39 +++++++++++++++++-- 4 files changed, 46 insertions(+), 7 deletions(-)