* [v2] usb: chipidea: imx: Allow OC polarity active low
@ 2018-12-02 21:12 Uwe Kleine-König
0 siblings, 0 replies; 3+ messages in thread
From: Uwe Kleine-König @ 2018-12-02 21:12 UTC (permalink / raw)
To: Matthew Starr
Cc: linux-usb, Peter Chen (Peter.Chen@nxp.com),
Fabio Estevam (festevam@gmail.com)
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [v2] usb: chipidea: imx: Allow OC polarity active low
@ 2018-12-02 5:57 kbuild test robot
0 siblings, 0 replies; 3+ messages in thread
From: kbuild test robot @ 2018-12-02 5:57 UTC (permalink / raw)
To: Matthew Starr
Cc: kbuild-all, linux-usb, Peter Chen (Peter.Chen@nxp.com),
Fabio Estevam (festevam@gmail.com),
Uwe Kleine-König
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [v2] usb: chipidea: imx: Allow OC polarity active low
@ 2018-11-30 23:29 Matthew Starr
0 siblings, 0 replies; 3+ messages in thread
From: Matthew Starr @ 2018-11-30 23:29 UTC (permalink / raw)
To: linux-usb
Cc: Peter Chen (Peter.Chen@nxp.com),
Fabio Estevam (festevam@gmail.com),
Uwe Kleine-König
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(-)
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);
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-02 21:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-02 21:12 [v2] usb: chipidea: imx: Allow OC polarity active low Uwe Kleine-König
-- strict thread matches above, loose matches on Subject: below --
2018-12-02 5:57 kbuild test robot
2018-11-30 23:29 Matthew Starr
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.