| Message ID | 3C8219BAE02B894A9C69304A12E8AA06CA27C613@HED-Exchange.hed.local (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] usb: chipidea: imx: Allow OC polarity active low | expand |
Hi Matthew,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.20-rc4 next-20181130]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Matthew-Starr/usb-chipidea-imx-Allow-OC-polarity-active-low/20181202-132847
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-x000-201848 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/usb/chipidea/usbmisc_imx.c: In function 'usbmisc_imx7d_init':
>> drivers/usb/chipidea/usbmisc_imx.c:452:2: error: expected '}' before 'else'
else {
^~~~
vim +452 drivers/usb/chipidea/usbmisc_imx.c
434
435 static int usbmisc_imx7d_init(struct imx_usbmisc_data *data)
436 {
437 struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
438 unsigned long flags;
439 u32 reg;
440
441 if (data->index >= 1)
442 return -EINVAL;
443
444 spin_lock_irqsave(&usbmisc->lock, flags);
445 reg = readl(usbmisc->base);
446 if (data->disable_oc) {
447 reg |= MX6_BM_OVER_CUR_DIS;
448 } else if (data->oc_polarity == 1) {
449 /* Low active */
450 reg &= ~(MX6_BM_OVER_CUR_DIS);
451 reg |= MX6_BM_OVER_CUR_POLARITY;
> 452 else {
453 reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
454 }
455 writel(reg, usbmisc->base);
456
457 reg = readl(usbmisc->base + MX7D_USBNC_USB_CTRL2);
458 reg &= ~MX7D_USB_VBUS_WAKEUP_SOURCE_MASK;
459 writel(reg | MX7D_USB_VBUS_WAKEUP_SOURCE_BVALID,
460 usbmisc->base + MX7D_USBNC_USB_CTRL2);
461 spin_unlock_irqrestore(&usbmisc->lock, flags);
462
463 usbmisc_imx7d_set_wakeup(data, false);
464
465 return 0;
466 }
467
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Nov 30, 2018 at 11:29:13PM +0000, Matthew Starr wrote: > The implementation for setting the over current polarity has always been > the over-current-active-high property. The problem with this is there > is no way to enable over current active low polarity since the default > state of the register bit that controls the over current polarity is a 0 > which means active high. This value never actually did anything since > active high is already the default state. Also it is not used in any > device tree source in the kernel. The default register bit state was > verified by checking the i.MX6Q and i.MX7Dual reference manuals. > > The change replaces the over-current-active-high property with the > over-current-active-low property. Without this property the over > current polarity will be active high by default like it always has been. > With the property the over current polarity will be active low. I'd prefer to do the following instead: Keep over-current-active-high in the binding and encourage users to specify (exactly) one of them by issuing a warning if there is none and keep the bit as configured by the bootloader (or by reset if the bootloader didn't touch it). > Signed-off-by: Matthew Starr <mstarr@hedonline.com> > --- > .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 4 ++-- > drivers/usb/chipidea/ci_hdrc_imx.c | 2 +- > drivers/usb/chipidea/usbmisc_imx.c | 12 ++++++++---- > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > index 529e51879fb2..3dee58037839 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > @@ -87,8 +87,8 @@ i.mx specific properties > - fsl,usbmisc: phandler of non-core register device, with one > argument that indicate usb controller index > - 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, > + the default signal polarity is high 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 09b37c0d075d..f7069544fb42 100644 > --- a/drivers/usb/chipidea/ci_hdrc_imx.c > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c > @@ -135,7 +135,7 @@ 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)) > + if (of_find_property(np, "over-current-active-low", NULL)) > data->oc_polarity = 1; > > if (of_find_property(np, "external-vbus-divider", NULL)) > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c > index def80ff547e4..b39ba76b874f 100644 > --- a/drivers/usb/chipidea/usbmisc_imx.c > +++ b/drivers/usb/chipidea/usbmisc_imx.c > @@ -341,10 +341,11 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data *data) > if (data->disable_oc) { > reg |= MX6_BM_OVER_CUR_DIS; > } else if (data->oc_polarity == 1) { > - /* High active */ > - reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY); > - } else { > + /* Low active */ > reg &= ~(MX6_BM_OVER_CUR_DIS); > + reg |= MX6_BM_OVER_CUR_POLARITY; > + } else { > + reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY); > } > writel(reg, usbmisc->base + data->index * 4); > > @@ -445,7 +446,10 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data *data) > if (data->disable_oc) { > reg |= MX6_BM_OVER_CUR_DIS; > } else if (data->oc_polarity == 1) { > - /* High active */ > + /* Low active */ > + reg &= ~(MX6_BM_OVER_CUR_DIS); > + reg |= MX6_BM_OVER_CUR_POLARITY; > + else { > reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY); You break machines here that use active low signaling but don't use the new property (yet). Also it's not nice to inverse the sematic of oc_polarity. Before it was at least intuitively right that = 1 means active high. As I improved my patch while being offline and before I saw your patch, I'm sending out my patch as a better alternative. Best regards Uwe
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt index 529e51879fb2..3dee58037839 100644 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt @@ -87,8 +87,8 @@ i.mx specific properties - fsl,usbmisc: phandler of non-core register device, with one argument that indicate usb controller index - 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, + the default signal polarity is high 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 09b37c0d075d..f7069544fb42 100644 --- a/drivers/usb/chipidea/ci_hdrc_imx.c +++ b/drivers/usb/chipidea/ci_hdrc_imx.c @@ -135,7 +135,7 @@ 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)) + if (of_find_property(np, "over-current-active-low", NULL)) data->oc_polarity = 1; if (of_find_property(np, "external-vbus-divider", NULL)) diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c index def80ff547e4..b39ba76b874f 100644 --- a/drivers/usb/chipidea/usbmisc_imx.c +++ b/drivers/usb/chipidea/usbmisc_imx.c @@ -341,10 +341,11 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data *data) if (data->disable_oc) { reg |= MX6_BM_OVER_CUR_DIS; } else if (data->oc_polarity == 1) { - /* High active */ - reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY); - } else { + /* Low active */ reg &= ~(MX6_BM_OVER_CUR_DIS); + reg |= MX6_BM_OVER_CUR_POLARITY; + } else { + reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY); } writel(reg, usbmisc->base + data->index * 4); @@ -445,7 +446,10 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data *data) if (data->disable_oc) { reg |= MX6_BM_OVER_CUR_DIS; } else if (data->oc_polarity == 1) { - /* High active */ + /* Low active */ + reg &= ~(MX6_BM_OVER_CUR_DIS); + reg |= MX6_BM_OVER_CUR_POLARITY; + else { reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY); } writel(reg, usbmisc->base);
The implementation for setting the over current polarity has always been the over-current-active-high property. The problem with this is there is no way to enable over current active low polarity since the default state of the register bit that controls the over current polarity is a 0 which means active high. This value never actually did anything since active high is already the default state. Also it is not used in any device tree source in the kernel. The default register bit state was verified by checking the i.MX6Q and i.MX7Dual reference manuals. The change replaces the over-current-active-high property with the over-current-active-low property. Without this property the over current polarity will be active high by default like it always has been. With the property the over current polarity will be active low. Signed-off-by: Matthew Starr <mstarr@hedonline.com> --- .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 4 ++-- drivers/usb/chipidea/ci_hdrc_imx.c | 2 +- drivers/usb/chipidea/usbmisc_imx.c | 12 ++++++++---- 3 files changed, 11 insertions(+), 7 deletions(-)