[RFC][PATCH 2/5 v2] usb: dwc2: Workaround case where GOTGCTL state is wrong

[John Stultz]

When removing a USB-A to USB-otg adapter cable, we get a change
status irq, and then in dwc2_conn_id_status_change, we
erroniously see the GOTGCTL_CONID_B flag set. This causes us to
get  stuck in the "while (!dwc2_is_device_mode(hsotg))" loop,
spitting out "Waiting for Peripheral Mode, Mode=Host" warnings
until it fails out many seconds later.

This patch works around the issue by re-reading the GOTGCTL
state to check if the GOTGCTL_CONID_B is still set and if not
restarting the change status logic.

I suspect this isn't the best solution, but it seems to work
well for me.

Feedback would be greatly appreciated!

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Vardan Mikayelyan 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Acked-by: John Youn 
Reviewed-by: Vardan Mikayelyan 
Signed-off-by: John Stultz 
---
v2: Rework goto logic suggested by Vardan, and add a comment
---
 drivers/usb/dwc2/hcd.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 911c3b3..b60307a 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3241,6 +3241,14 @@ static void dwc2_conn_id_status_change(struct 
work_struct *work)
 dwc2_is_host_mode(hsotg) ? "Host" :
 "Peripheral");
usleep_range(2, 4);
+   /*
+* Sometimes the initial GOTGCTRL read is wrong, so
+* check it again and jump to host mode if that was
+* the case.
+*/
+   gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+   if (!(gotgctl & GOTGCTL_CONID_B))
+   goto host;
if (++count > 250)
break;
}
@@ -3255,6 +3263,7 @@ static void dwc2_conn_id_status_change(struct work_struct 
*work)
spin_unlock_irqrestore(>lock, flags);
dwc2_hsotg_core_connect(hsotg);
} else {
+host:
/* A-Device connector (Host Mode) */
dev_dbg(hsotg->dev, "connId A\n");
while (!dwc2_is_host_mode(hsotg)) {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 4/5 v2] usb: dwc2: Avoid suspending if we're in gadget mode

[John Stultz]

I've found when booting HiKey with the usb gadget cable attached
if I then try to connect via adb, I get an infinite spew of:

dwc2 f72c.usb: dwc2_hsotg_ep_sethalt(ep ffc0790ecb18 ep1out, 0)
dwc2 f72c.usb: dwc2_hsotg_ep_sethalt(ep ffc0790eca18 ep1in, 0)

It seems that the usb autosuspend is suspending the bus shortly
after bootup when the gadget cable is attached. So when adbd
then tries to use the device, it doesn't work and it then tries
to restart it over and over via the ep_sethalt calls (via
FUNCTIONFS_CLEAR_HALT ioctl).

Chen Yu suggested this patch to avoid suspending if we're
in device mode, and it avoids the problem.

This doesn't remove the need for the previous patch, to resume
the port when we switch to gadget mode from host mode. But it
does seem to resolve the issue.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Vardan Mikayelyan 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Suggested-by: Chen Yu 
Signed-off-by: John Stultz 
---
 drivers/usb/dwc2/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index a089946..6e4ec8a 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4383,6 +4383,9 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
if (!HCD_HW_ACCESSIBLE(hcd))
goto unlock;
 
+   if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
+   goto unlock;
+
if (!hsotg->params.hibernation)
goto skip_power_saving;
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 1/5 v2] usb: dwc2: Avoid sleeping while holding hsotg->lock

[John Stultz]

Basically when plugging in various cables in different orders, I'm
occasionally seeing the following BUG splat:

[   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x0002
[   86.219164] usb 1-1: USB disconnect, device number 9
[   86.226845] Preemption disabled at:[   86.230218]
[] dwc2_conn_id_status_change+0x120/0x250
[   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: GW
 4.9.0-rc8-00051-gd5a7979-dirty #1702
[   86.246836] Hardware name: HiKey Development Board (DT)
[   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change
[   86.257279] Call trace:
[   86.259771] [] dump_backtrace+0x0/0x1a0
[   86.265210] [] show_stack+0x14/0x20
[   86.270308] [] dump_stack+0x90/0xb0
[   86.275401] [] __schedule_bug+0x6c/0xb8
[   86.280841] [] __schedule+0x4f8/0x5b0
[   86.286099] [] schedule+0x38/0xa0
[   86.291017] [] schedule_hrtimeout_range_clock+0x8c/0xf0
[   86.297846] [] schedule_hrtimeout_range+0x10/0x18
[   86.304150] [] usleep_range+0x50/0x58
[   86.309418] [] dwc2_wait_for_mode.isra.4+0x54/0xd0
[   86.315815] [] dwc2_core_reset+0xe0/0x168
[   86.321431] [] dwc2_hsotg_core_init_disconnected+0x2c/0x310
[   86.328602] [] dwc2_conn_id_status_change+0x130/0x250
[   86.335254] [] process_one_work+0x118/0x370
[   86.341035] [] worker_thread+0x48/0x498
[   86.346473] [] kthread+0xd0/0xe8
[   86.351299] [] ret_from_fork+0x10/0x50

This seems to be caused by the dwc2_wait_for_mode() calling
usleep_range() while the hstog->lock spinlock is held, since
we take that before calling dwc2_hsotg_core_init_disconnected().

This patch avoids the issue by adding an extra argument to
dwc2_core_reset(), as suggested by John Youn, which allows us to
skip the waiting, which should be unnecessary when calling from
dwc2_hsotg_core_init_disconnected().

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Vardan Mikayelyan 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Signed-off-by: John Stultz 
---
 drivers/usb/dwc2/core.c   | 6 +++---
 drivers/usb/dwc2/core.h   | 2 +-
 drivers/usb/dwc2/gadget.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 11d8ae9..2d0dac2 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -313,7 +313,7 @@ static bool dwc2_iddig_filter_enabled(struct dwc2_hsotg 
*hsotg)
  * Do core a soft reset of the core.  Be careful with this because it
  * resets all the internal state machines of the core.
  */
-int dwc2_core_reset(struct dwc2_hsotg *hsotg)
+int dwc2_core_reset(struct dwc2_hsotg *hsotg, bool skip_wait)
 {
u32 greset;
int count = 0;
@@ -369,7 +369,7 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
}
} while (!(greset & GRSTCTL_AHBIDLE));
 
-   if (wait_for_host_mode)
+   if (wait_for_host_mode && !skip_wait)
dwc2_wait_for_mode(hsotg, true);
 
return 0;
@@ -500,7 +500,7 @@ int dwc2_core_reset_and_force_dr_mode(struct dwc2_hsotg 
*hsotg)
 {
int retval;
 
-   retval = dwc2_core_reset(hsotg);
+   retval = dwc2_core_reset(hsotg, false);
if (retval)
return retval;
 
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 9548d3e..eb2d628 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1101,7 +1101,7 @@ static inline bool dwc2_is_hs_iot(struct dwc2_hsotg 
*hsotg)
  * The following functions support initialization of the core driver component
  * and the DWC_otg controller
  */
-extern int dwc2_core_reset(struct dwc2_hsotg *hsotg);
+extern int dwc2_core_reset(struct dwc2_hsotg *hsotg, bool skip_wait);
 extern int dwc2_core_reset_and_force_dr_mode(struct dwc2_hsotg *hsotg);
 extern int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg);
 extern int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, bool restore);
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index c55db4a..cbba471 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3158,7 +3158,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg 
*hsotg,
kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
 
if (!is_usb_reset)
-   if (dwc2_core_reset(hsotg))
+   if (dwc2_core_reset(hsotg, true))
return;
 
/*
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 3/5 v2] usb: dwc2: Force port resume on switching to device mode

[John Stultz]

From: Chen Yu 

We've seen failures when switching between host and gadget mode,
which was diagnosed as being caused due to the bus being
auto-suspended when we switched.

So this patch forces a port resume when switching to device
mode if the bus is suspended.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Vardan Mikayelyan 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Signed-off-by: Chen Yu 
Signed-off-by: John Stultz 
---
 drivers/usb/dwc2/hcd.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index b60307a..a089946 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -54,6 +54,8 @@
 #include "core.h"
 #include "hcd.h"
 
+static void dwc2_port_resume(struct dwc2_hsotg *hsotg);
+
 /*
  * =
  *  Host Core Layer Functions
@@ -3235,6 +3237,11 @@ static void dwc2_conn_id_status_change(struct 
work_struct *work)
if (gotgctl & GOTGCTL_CONID_B) {
/* Wait for switch to device mode */
dev_dbg(hsotg->dev, "connId B\n");
+   if (hsotg->bus_suspended) {
+   dev_info(hsotg->dev,
+"Do port resume before switching to device 
mode\n");
+   dwc2_port_resume(hsotg);
+   }
while (!dwc2_is_device_mode(hsotg)) {
dev_info(hsotg->dev,
 "Waiting for Peripheral Mode, Mode=%s\n",
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 5/5 v2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220

[John Stultz]

From: Chen Yu 

The Hi6220's usb controller is limited in that it does not
support "Split Transactions", so it does not support communicating
with low-speed and full-speed devices behind a high-speed hub.

Thus it requires a quirk so that we can manually drop the usb
speed when low/full-speed are attached, and bump back to high
speed when they are removed.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Vardan Mikayelyan 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Signed-off-by: Chen Yu 
[jstultz: Reworked to simplify the patch, and made
 commit log to be more specific about the issue]
Signed-off-by: John Stultz 
---
v2:
* Fix build issue reported by kbuildbot
* Rework to avoid using new dts entry suggested by RobH
* Further tweaks from Chen Yu to try to address comments from
  John Youn
* Further simplified logic
v3:
* Reworked to adapt around changes that landed in 4.10-rc
---
 drivers/usb/dwc2/core.h   |  7 ++
 drivers/usb/dwc2/hcd.c| 60 +++
 drivers/usb/dwc2/params.c | 19 +++
 3 files changed, 86 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index eb2d628..4074752 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -444,6 +444,11 @@ enum dwc2_ep0_state {
  * in DWORDS with possible values from from
  * 16-32768 (default: 256, 256, 256, 256, 768,
  * 768, 768, 768, 0, 0, 0, 0, 0, 0, 0).
+ * @change_speed_quirk: Change speed configuration to DWC2_SPEED_PARAM_FULL
+ *  while full speed device connect. And change speed
+ *  back to DWC2_SPEED_PARAM_HIGH while device is gone.
+ * 0 - No (default)
+ * 1 - Yes
  *
  * The following parameters may be specified when starting the module. These
  * parameters define how the DWC_otg controller should be configured. A
@@ -516,6 +521,8 @@ struct dwc2_core_params {
u16 g_rx_fifo_size;
u16 g_np_tx_fifo_size;
u32 g_tx_fifo_size[MAX_EPS_CHANNELS];
+
+   int change_speed_quirk;
 };
 
 /**
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 6e4ec8a..81997d2 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4876,6 +4876,61 @@ static void _dwc2_hcd_clear_tt_buffer_complete(struct 
usb_hcd *hcd,
spin_unlock_irqrestore(>lock, flags);
 }
 
+/*
+ * HPRT0_SPD_HIGH_SPEED: high speed
+ * HPRT0_SPD_FULL_SPEED: full speed
+ */
+static void dwc2_change_bus_speed(struct usb_hcd *hcd, int speed)
+{
+   struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
+
+   if (hsotg->params.speed == speed)
+   return;
+
+   hsotg->params.speed = speed;
+   queue_work(hsotg->wq_otg, >wf_otg);
+}
+
+static void dwc2_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
+{
+   struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
+
+   if (!hsotg->params.change_speed_quirk)
+   return;
+
+   /*
+* On removal, set speed to default high-speed.
+*/
+   if (udev->parent && udev->parent->speed > USB_SPEED_UNKNOWN &&
+   udev->parent->speed < USB_SPEED_HIGH) {
+   dev_info(hsotg->dev, "Set speed to default high-speed\n");
+   dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
+   }
+}
+
+static int dwc2_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
+{
+   struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
+
+   if (!hsotg->params.change_speed_quirk)
+   return 0;
+
+   if (udev->speed == USB_SPEED_HIGH) {
+   dev_info(hsotg->dev, "Set speed to high-speed\n");
+   dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
+   } else if ((udev->speed == USB_SPEED_FULL ||
+   udev->speed == USB_SPEED_LOW)) {
+   /*
+* Change speed setting to full-speed if there's
+* a full-speed or low-speed device plugged in.
+*/
+   dev_info(hsotg->dev, "Set speed to full-speed\n");
+   dwc2_change_bus_speed(hcd, HPRT0_SPD_FULL_SPEED);
+   }
+
+   return 0;
+}
+
 static struct hc_driver dwc2_hc_driver = {
.description = "dwc2_hsotg",
.product_desc = "DWC OTG Controller",
@@ -5031,6 +5086,11 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
dev_warn(hsotg->dev, "can't set coherent DMA mask\n");
}
 
+   if (hsotg->params.change_speed_quirk) {

[RFC][PATCH 0/5 v2] Fixes and workarounds for dwc2 on HiKey board

[John Stultz]

I just wanted to re-send my current queue of patches for dwc2
controller on the HiKey board, as my last patchset ended up
colliding with a number of changes that landed in the 4.10-rc
merge window. 

I've fixed things up and validated these against HiKey. I also
made a small change to one of the patches as suggested by Vardan.

This does exclude my patchset[1] to add extcon support to dwc2,
which John Youn suspects a pending rework of the dwc2 fifo init
logic might make unnecssary (however, no such changes appeared
to have landed in 4.10-rc, so I'm still using that patchset in
my testing).

I'm also out for the holidays after today, and while I suspect
others are likely going to be out as well, I figured I'd send
these out for a chance at review, rather then sit on them for
two weeks. I'll resend after the new year addressing any
feedback I do get though.

Feedback would be greatly appreciated!

thanks
-john

[1] https://lkml.org/lkml/2016/12/6/69

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Vardan Mikayelyan 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org

Chen Yu (2):
  usb: dwc2: Force port resume on switching to device mode
  usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220

John Stultz (3):
  usb: dwc2: Avoid sleeping while holding hsotg->lock
  usb: dwc2: Workaround case where GOTGCTL state is wrong
  usb: dwc2: Avoid suspending if we're in gadget mode

 drivers/usb/dwc2/core.c   |  6 ++--
 drivers/usb/dwc2/core.h   |  9 +-
 drivers/usb/dwc2/gadget.c |  2 +-
 drivers/usb/dwc2/hcd.c| 79 +++
 drivers/usb/dwc2/params.c | 19 
 5 files changed, 110 insertions(+), 5 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: dwc3: gadget: Fix full speed mode

[John Youn]

On 12/7/2016 7:06 PM, John Youn wrote:
> On 12/7/2016 4:44 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Roger Quadros  writes:
> Roger Quadros  writes:
>> DCFG.DEVSPD == 0x3 is not valid and we need to set
>> DCFG.DEVSPD to 0x1 for full speed mode.
>
> seems like it has been made invalid somewhere between 1.73a and
> 2.60a. Can you figure it out from Documentation why and when it was made
> invalid? We might need revision checks here.
>

 I'll try to dig out more.
>>>
>>> I couldn't figure out more information on this. The changelogs in the TRMs
>>> don't capture this change and I don't have access to all TRM versions
>>> so I can't say which version it got changed and why.
>>>
>>> Can we please involve someone from Synopsis to provide this information?
>>> Thanks.
>>
>> John, could you help us with this query? We'd like to understand why one
>> of the FULLSPEED modes got removed. Do we need a revision check or can
>> we assume that the other mode was never supposed to be used?
>>
> 
> Full speed is 0x1. 0x3 may still work due to how the bits are
> checked. But it definitely should be 0x1.
> 
> I'm not sure if it was 0x3 before. I still need to confirm whether
> that was the case or not and if so why.
> 

Hi Felipe,

According to the old databook, 0x3 was for FS in 48MHz mode for 1.1
transceiver, which was never supported. UTMI FS was still specified as
0x1 in those old databooks.

Regards,
John


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] usb: musb: blackfin: fix unused warnings

[Jérémy Lefaure]

I found three functions in blackfin.c defined but not used in some
configurations. These patches fix these compilation warnings.

[PATCH 1/2] usb: musb: blackfin: fix unused warnings on suspend/resume
[PATCH 2/2] usb: musb: blackfin: add bfin_fifo_offset in bfin_ops
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] usb: musb: blackfin: add bfin_fifo_offset in bfin_ops

[Jérémy Lefaure]

Commit cc92f6818f6e ("usb: musb: Populate new IO functions for
blackfin") defines bfin_fifo_offset function without using it:

drivers/usb/musb/blackfin.c:36:12: warning: ‘bfin_fifo_offset’ defined
but not used [-Wunused-function]
 static u32 bfin_fifo_offset(u8 epnum)
 ^~~~

Adding bfin_fifo_offset to bfin_ops fixes this warning and allows musb
core to call this function instead of default_fifo_offset.

Signed-off-by: Jérémy Lefaure 
---
 drivers/usb/musb/blackfin.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
index f43edc43268b..4418574a36a1 100644
--- a/drivers/usb/musb/blackfin.c
+++ b/drivers/usb/musb/blackfin.c
@@ -469,6 +469,7 @@ static const struct musb_platform_ops bfin_ops = {
.init   = bfin_musb_init,
.exit   = bfin_musb_exit,
 
+   .fifo_offset= bfin_fifo_offset,
.readb  = bfin_readb,
.writeb = bfin_writeb,
.readw  = bfin_readw,
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] usb: musb: blackfin: fix unused warnings on suspend/resume

[Jérémy Lefaure]

When CONFIG_PM_SLEEP is disabled, SIMPLE_DEV_PM_OPS does not use
bfin_resume and bfin_suspend even if CONFIG_PM is enabled:

drivers/usb/musb/blackfin.c:602:12: warning: ‘bfin_resume’ defined but
not used [-Wunused-function]
 static int bfin_resume(struct device *dev)
^~~
drivers/usb/musb/blackfin.c:585:12: warning: ‘bfin_suspend’ defined but
not used [-Wunused-function]
 static int bfin_suspend(struct device *dev)
^~~~

The preprocessor condition should be on CONFIG_PM_SLEEP, not on CONFIG_PM.
However it is better to mark these functions as __maybe_unused.

Signed-off-by: Jérémy Lefaure 
---
 drivers/usb/musb/blackfin.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
index 310238c6b5cd..f43edc43268b 100644
--- a/drivers/usb/musb/blackfin.c
+++ b/drivers/usb/musb/blackfin.c
@@ -580,8 +580,7 @@ static int bfin_remove(struct platform_device *pdev)
return 0;
 }
 
-#ifdef CONFIG_PM
-static int bfin_suspend(struct device *dev)
+static int __maybe_unused bfin_suspend(struct device *dev)
 {
struct bfin_glue*glue = dev_get_drvdata(dev);
struct musb *musb = glue_to_musb(glue);
@@ -598,7 +597,7 @@ static int bfin_suspend(struct device *dev)
return 0;
 }
 
-static int bfin_resume(struct device *dev)
+static int __maybe_unused bfin_resume(struct device *dev)
 {
struct bfin_glue*glue = dev_get_drvdata(dev);
struct musb *musb = glue_to_musb(glue);
@@ -607,7 +606,6 @@ static int bfin_resume(struct device *dev)
 
return 0;
 }
-#endif
 
 static SIMPLE_DEV_PM_OPS(bfin_pm_ops, bfin_suspend, bfin_resume);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: c67x00: remove unused variable

[Sudip Mukherjee]

The pointer urbp was only assigned some address but was never used.

Signed-off-by: Sudip Mukherjee 
---
 drivers/usb/c67x00/c67x00-sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/c67x00/c67x00-sched.c 
b/drivers/usb/c67x00/c67x00-sched.c
index 7311ed6..0b68cd6 100644
--- a/drivers/usb/c67x00/c67x00-sched.c
+++ b/drivers/usb/c67x00/c67x00-sched.c
@@ -966,13 +966,11 @@ static void c67x00_handle_successful_td(struct c67x00_hcd 
*c67x00,
 static void c67x00_handle_isoc(struct c67x00_hcd *c67x00, struct c67x00_td *td)
 {
struct urb *urb = td->urb;
-   struct c67x00_urb_priv *urbp;
int cnt;
 
if (!urb)
return;
 
-   urbp = urb->hcpriv;
cnt = td->privdata;
 
if (td->status & TD_ERROR_MASK)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: atm: cxacru: remove impossible condition

[Sudip Mukherjee]

The variable index is unsigned and so it can never be less than 0.

Signed-off-by: Sudip Mukherjee 
---
 drivers/usb/atm/cxacru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index f9fe86b6..d65a64c 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -474,7 +474,7 @@ static ssize_t cxacru_sysfs_store_adsl_config(struct device 
*dev,
ret = sscanf(buf + pos, "%x=%x%n", , , );
if (ret < 2)
return -EINVAL;
-   if (index < 0 || index > 0x7f)
+   if (index > 0x7f)
return -EINVAL;
if (tmp < 0 || tmp > len - pos)
return -EINVAL;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: usb/core: warning in usb_create_ep_devs/sysfs_create_dir_ns

[Alan Stern]

On Mon, 12 Dec 2016, Andrey Konovalov wrote:

> Hi!
> 
> While running the syzkaller fuzzer I've got the following error report.
> 
> On commit 3c49de52d5647cda8b42c4255cf8a29d1e22eff5 (Dev 2).
> 
> WARNING: CPU: 2 PID: 865 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x8a/0xa0
> gadgetfs: disconnected
> sysfs: cannot create duplicate filename
> '/devices/platform/dummy_hcd.0/usb2/2-1/2-1:64.0/ep_05'
> Kernel panic - not syncing: panic_on_warn set ...
> 
> CPU: 2 PID: 865 Comm: kworker/2:1 Not tainted 4.9.0-rc7+ #34
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Workqueue: usb_hub_wq hub_event
>  88006bee64c8 81f96b8a 0001 11000d7dcc2c
>  ed000d7dcc24 0001 41b58ab3 8598b510
>  81f968f8 850fee20 85cff020 dc00
> Call Trace:
>  [< inline >] __dump_stack lib/dump_stack.c:15
>  [] dump_stack+0x292/0x398 lib/dump_stack.c:51
>  [] panic+0x1cb/0x3a9 kernel/panic.c:179
>  [] __warn+0x1c4/0x1e0 kernel/panic.c:542
>  [] warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:565
>  [] sysfs_warn_dup+0x8a/0xa0 fs/sysfs/dir.c:30
>  [] sysfs_create_dir_ns+0x178/0x1d0 fs/sysfs/dir.c:59
>  [< inline >] create_dir lib/kobject.c:71
>  [] kobject_add_internal+0x227/0xa60 lib/kobject.c:229
>  [< inline >] kobject_add_varg lib/kobject.c:366
>  [] kobject_add+0x139/0x220 lib/kobject.c:411
>  [] device_add+0x353/0x1660 drivers/base/core.c:1088
>  [] device_register+0x1d/0x20 drivers/base/core.c:1206
>  [] usb_create_ep_devs+0x163/0x260
> drivers/usb/core/endpoint.c:195
>  [] create_intf_ep_devs+0x13b/0x200
> drivers/usb/core/message.c:1030
>  [] usb_set_configuration+0x1083/0x18d0
> drivers/usb/core/message.c:1937

Hi, Andrey:

Please check whether the patch below fixes this problem.

Alan Stern



Index: usb-4.x/drivers/usb/core/config.c
===
--- usb-4.x.orig/drivers/usb/core/config.c
+++ usb-4.x/drivers/usb/core/config.c
@@ -234,6 +234,16 @@ static int usb_parse_endpoint(struct dev
if (ifp->desc.bNumEndpoints >= num_ep)
goto skip_to_next_endpoint_or_interface_descriptor;
 
+   /* Check for duplicate endpoint addresses */
+   for (i = 0; i < ifp->desc.bNumEndpoints; ++i) {
+   if (ifp->endpoint[i].desc.bEndpointAddress ==
+   d->bEndpointAddress) {
+   dev_warn(ddev, "config %d interface %d altsetting %d 
has a duplicate endpoint with address 0x%X, skipping\n",
+   cfgno, inum, asnum, d->bEndpointAddress);
+   goto skip_to_next_endpoint_or_interface_descriptor;
+   }
+   }
+
endpoint = >endpoint[ifp->desc.bNumEndpoints];
++ifp->desc.bNumEndpoints;
 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] usb: musb: da8xx: Add support of suspend / resume

[Bin Liu]

On Fri, Dec 16, 2016 at 11:46:13AM +0100, Alexandre Bailon wrote:
> There is no suspend / resume support for da8xx.
> Add the PM methods to da8xx glue.
> In addition, introduce a new quirk to preserve the session
> (i.e not clear the devctl register).
> Clearing devctl will power off vbus on da8xx platform
> (when it is host mode) and then devices will be disconnected on resume.
> 
> Changes in v2:
> * Rebased the patches on top off this series:
>   http://www.spinics.net/lists/linux-usb/msg150856.html
> 
> Alexandre Bailon (3):
>   usb: musb: da8xx: Add support of suspend / resume
>   usb: musb: Add a quirk to preserve the session during suspend
>   usb: musb: da8xx: Fix host mode suspend

Applied. Thanks.
-Bin.

> 
>  drivers/usb/musb/da8xx.c | 32 +++-
>  drivers/usb/musb/musb_core.c |  3 ++-
>  drivers/usb/musb/musb_core.h |  1 +
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> -- 
> 2.7.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings

[Johan Hovold]

On Fri, Dec 16, 2016 at 05:13:50PM +0100, Johan Hovold wrote:
> On Fri, Dec 16, 2016 at 08:04:18AM -0800, Russell Senior wrote:
> > Sorry, I got distracted.  I'm back now.  Do you want me to test your
> > 13 patch series?  And what is that on top of?
> 
> Yes, please. It's against my usb-next branch.
> 
> I'll send you a couple of diagnostics patches as well shortly.

I ended up pushing a new branch, ch341 to my tree

git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git

which contains the full series as well as four debug patches (on top of
usb-next).

Could you first try and see if the series works with some register dumps
enabled:

79e475a77796 dbg: ch341: add divisor and lcr debugging

If that works, could you try the next commit and see if all still works

ee5a27b51e95 dbg: ch341: enable rx timer

Either way, could you also try the next commit which again tries to use
the init-command for all devices:

a06b45d910e3 dbg: ch341: use init for all devices

and if that does not work, the final commit tries to use the init
command but always use the chip default LCR:

6edc2830abe1 dbg: ch341: always use default LCR

As you know there are, at least two levels of "working" here

1. Your device works and you can change baud rate, but you're
   stuck with 8N1

2. Changing LCR also works (e.g. switching back and forth
   between say 5N1 and 8N1 also works).

When testing a new commit, always make sure to disconnect and reconnect
the device first.

Logs from all tests would be helpful (e.g. to see what effects the
various commands has on the registers).

Just let me know if you have any questions.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type

[Felipe Balbi]

Hi,

Jerry Huang  writes:
> Hi, Balbi,
>> -Original Message-
>> From: Felipe Balbi [mailto:ba...@kernel.org]
>> Sent: Friday, December 16, 2016 7:44 PM
>> To: Jerry Huang ; gre...@linuxfoundation.org
>> Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Rajesh Bhagat
>> 
>> Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
>> 
>> 
>> Hi,
>> 
>> Jerry Huang  writes:
>> >> there's no need for that. This patch is in good format. I do have a
>> >> question,
>> >> however: how do you know this will work for all users? Burst size is
>> >> a function of how wide the interconnect where dwc3 is attached to, is.
>> > So I need to generate one new property in usb node to identify my
>> platform?
>> 
>> Well, we probably need a property to be passed, yes. But let's go through it
>> all first :-)
>
> I think "snps,quirk-frame-length-adjustment" is one good reference,
> which can pass the required value to driver from DTS file.

that's not for burst increment, though.

>> >> You could very well be degrading performance for some users here. Can
>> >> you send me the result of the following commands *without* this patch
>> applied?
>> >>
>> >> # mkdir -p /d
>> >> # mount -t debugfs none /d
>> >> # cat /d/*dwc3*/regdump
>> >>
>> > Below is the regdump:
>> > root@ls1043ardb:/d/300.usb3# cat regdump
>> > GSBUSCFG0 = 0x00100080
>> 
>> so you already have INCR256 here. There's one note in the databook which
>> just caught my attention. It states the following:
>> 
>>  "Undefined burst length has priority over all other burst lenghts."
>> 
>> This means that setting both INCR16 and undefined INCR is unnecessary.
> When bit0 = 1 (Undefined Length INCR Burst Type Enable), which means:
>  1: INCR (undefined length) burst mode
> - AHB configurations: HBURST uses SINGLE or INCR
> of any length less than or equal to the largest-enabled
> burst length of INCR4/8/16/32/64/128/256.
> - AXI configurations: ARLEN/AWLEN uses any length
> less than or equal to the largest-enabled burst length
> of INCR4/8/16/32/64/128/256.

interesting, it doesn't describe what happens to OCP or PCI.

> So, after enable undefined length INCR burst and enable INCR16,
> controller will use less than or equal to 16byte.
>
>> Only Undefined INCR will be taken into consideration. Can you check with
>> your HW engineers what's the largest burst the interconnect is supposed to
>> support?
> I will check it with IP designer.

cool, thanks :-)

>> > GSBUSCFG1 = 0x0700
>> 
>> 8 AXI pipelined requests
>> 
>> > GSNPSID = 0x5533280a
>> 
>> 2.80a cool :-)
>> 
>> I'll check these settings on my platform as well and see if there's any 
>> setting
>> which would improve transfer speed. This is a very good idea, btw, but we
>> need to be careful about how to play with it.
>> 
>> --
>> balbi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type

[Jerry Huang]

Hi, Balbi,
> -Original Message-
> From: Felipe Balbi [mailto:ba...@kernel.org]
> Sent: Friday, December 16, 2016 7:44 PM
> To: Jerry Huang ; gre...@linuxfoundation.org
> Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Rajesh Bhagat
> 
> Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
> 
> 
> Hi,
> 
> Jerry Huang  writes:
> >> there's no need for that. This patch is in good format. I do have a
> >> question,
> >> however: how do you know this will work for all users? Burst size is
> >> a function of how wide the interconnect where dwc3 is attached to, is.
> > So I need to generate one new property in usb node to identify my
> platform?
> 
> Well, we probably need a property to be passed, yes. But let's go through it
> all first :-)
I think "snps,quirk-frame-length-adjustment" is one good reference, which can 
pass the required value to driver from DTS file.

> >> You could very well be degrading performance for some users here. Can
> >> you send me the result of the following commands *without* this patch
> applied?
> >>
> >> # mkdir -p /d
> >> # mount -t debugfs none /d
> >> # cat /d/*dwc3*/regdump
> >>
> > Below is the regdump:
> > root@ls1043ardb:/d/300.usb3# cat regdump
> > GSBUSCFG0 = 0x00100080
> 
> so you already have INCR256 here. There's one note in the databook which
> just caught my attention. It states the following:
> 
>   "Undefined burst length has priority over all other burst lenghts."
> 
> This means that setting both INCR16 and undefined INCR is unnecessary.
When bit0 = 1 (Undefined Length INCR Burst Type Enable), which means:
 1: INCR (undefined length) burst mode
- AHB configurations: HBURST uses SINGLE or INCR
of any length less than or equal to the largest-enabled
burst length of INCR4/8/16/32/64/128/256.
- AXI configurations: ARLEN/AWLEN uses any length
less than or equal to the largest-enabled burst length
of INCR4/8/16/32/64/128/256.
So, after enable undefined length INCR burst and enable INCR16, controller will 
use less than or equal to 16byte.

> Only Undefined INCR will be taken into consideration. Can you check with
> your HW engineers what's the largest burst the interconnect is supposed to
> support?
I will check it with IP designer.

> > GSBUSCFG1 = 0x0700
> 
> 8 AXI pipelined requests
> 
> > GSNPSID = 0x5533280a
> 
> 2.80a cool :-)
> 
> I'll check these settings on my platform as well and see if there's any 
> setting
> which would improve transfer speed. This is a very good idea, btw, but we
> need to be careful about how to play with it.
> 
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings

[Johan Hovold]

On Fri, Dec 16, 2016 at 08:04:18AM -0800, Russell Senior wrote:
> Sorry, I got distracted.  I'm back now.  Do you want me to test your
> 13 patch series?  And what is that on top of?

Yes, please. It's against my usb-next branch.

I'll send you a couple of diagnostics patches as well shortly.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings

[Russell Senior]

Sorry, I got distracted.  I'm back now.  Do you want me to test your
13 patch series?  And what is that on top of?

Thanks

On Fri, Dec 16, 2016 at 6:46 AM, Johan Hovold  wrote:
> On Fri, Dec 16, 2016 at 01:19:05PM +, Aidan Thornton wrote:
>> On Wed, Dec 14, 2016 at 3:28 PM, Johan Hovold  wrote:
>> > The ch341 driver is based on reverse-engineering and contains some bits
>> > which appear to be redundant and some which appear incompatible which
>> > certain devices.
>> >
>> > Specifically, some CH340 devices seem unable to change the initial line
>> > settings, which have so far been set to 5N1. Let's use a more reasonable
>> > 8N1 default instead.
>> >
>> > Note that we also need to set the ENABLE_RX bit or receive will be
>> > disabled after a reset during resume.
>>
>> Lost track a little of the testing, but I don't think you ever got
>> Russel to test whether this worked using a driver that tried to change
>> this through direct register writes which seem to be the only thing
>> that has any effect on this strange hardware variant. Would be a
>> little surprised if it didn't at this point - changing the line
>> settings that way after initialization certainly works well enough to
>> cause us all a headache.
>
> I believe he did test this against my usb-linus branch, which did not
> have your recent changes. IIRC both removing that first LCR write and
> changing it to 0xc3 worked, but maybe you're right and we only verified
> removing it.
>
> In the same setup, changing the word size after successful
> initialisation using direct register writes did not work however, and
> the device appeared stuck at 5N1 or 8N1 depending on if the initial 0x50
> LCR write was left in or not.
>
>> If so, I suggest it might be clearer to drop this patch altogether in
>> favour of patch 10 in the series, since the underlying problem is that
>> setting the baudrate and LCR register didn't work and patch 10 fixes
>> that.
>
> These two patches are definitely related, and I struggled a bit about
> how to order things as I wanted to find a way to backport this minimal
> change (from 5N1 to 8N1) without having to depend on the upcoming
> changes in 4.10 (your changes) as that may be enough to enable support
> in older kernels.
>
> I'll respin this, once we learn more about how those quirky devices
> work.
>
> Thanks,
> Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings

[Johan Hovold]

On Fri, Dec 16, 2016 at 01:19:05PM +, Aidan Thornton wrote:
> On Wed, Dec 14, 2016 at 3:28 PM, Johan Hovold  wrote:
> > The ch341 driver is based on reverse-engineering and contains some bits
> > which appear to be redundant and some which appear incompatible which
> > certain devices.
> >
> > Specifically, some CH340 devices seem unable to change the initial line
> > settings, which have so far been set to 5N1. Let's use a more reasonable
> > 8N1 default instead.
> >
> > Note that we also need to set the ENABLE_RX bit or receive will be
> > disabled after a reset during resume.
> 
> Lost track a little of the testing, but I don't think you ever got
> Russel to test whether this worked using a driver that tried to change
> this through direct register writes which seem to be the only thing
> that has any effect on this strange hardware variant. Would be a
> little surprised if it didn't at this point - changing the line
> settings that way after initialization certainly works well enough to
> cause us all a headache.

I believe he did test this against my usb-linus branch, which did not
have your recent changes. IIRC both removing that first LCR write and
changing it to 0xc3 worked, but maybe you're right and we only verified
removing it.

In the same setup, changing the word size after successful
initialisation using direct register writes did not work however, and
the device appeared stuck at 5N1 or 8N1 depending on if the initial 0x50
LCR write was left in or not.

> If so, I suggest it might be clearer to drop this patch altogether in
> favour of patch 10 in the series, since the underlying problem is that
> setting the baudrate and LCR register didn't work and patch 10 fixes
> that.

These two patches are definitely related, and I struggled a bit about
how to order things as I wanted to find a way to backport this minimal
change (from 5N1 to 8N1) without having to depend on the upcoming
changes in 4.10 (your changes) as that may be enough to enable support
in older kernels.

I'll respin this, once we learn more about how those quirky devices
work.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/13] USB: serial: ch341: fix baud rate and line-control handling

[Johan Hovold]

On Fri, Dec 16, 2016 at 12:39:45PM +, Aidan Thornton wrote:
> On Thu, Dec 15, 2016 at 3:58 PM, Johan Hovold  wrote:
> > On Wed, Dec 14, 2016 at 04:28:07PM +0100, Johan Hovold wrote:
> >> Some CH341 devices appear to require the use of the init vendor command
> >> to set the baud rate and line-control register. Specifically, while
> >> using direct register updates for speed and LCR seem to update those
> >> settings correctly, not using the init command causes received data to
> >> be buffered until a full endpoint-size packet (32 bytes) have been
> >> received (i.e. the init command has some undocumented side-effect we
> >> need).
> >
> > Turns out it is bit 7 in the divisor register which needs to be set in
> > order for CH341A to not buffer incoming data this way. If set, direct
> > register updates works also for these devices.
> >
> > This bit is only set since 4e46c410e050 ("USB: serial: ch341:
> > reinitialize chip on reconfiguration") (in -next), which switched to
> > using the init-vendor command.
> >
> > So it seems we could use a common implementation after all...
> 
> It would certainly simplify matters a lot if it turns out that was the
> only problem with using direct register writes after all.
> 
> The removal of the second CH341_REQ_SERIAL_INIT by patch 2 of my
> series and subsequent removal of it from baudrate setting too does
> presumably mean that some register which was being set to a non-zero
> value isn't anymore. (The one cryptically described as "enable
> SFR_UART Control register and timer" in the vendor driver.)

I think I've managed to decipher that: the control register comment is
about setting bit 7 in wValue which causes the provided LCR value in the
high byte to take effect. If left unset, the LCR is instead reset to its
default value -- and this was in fact something that we unknowingly
relied on before your recent change that removed the second init.
Without, all devices would have been stuck at 5N1 due to that initial
LCR write.

The UART timer thing, may be about that buffering effect I describe
above, but could be something different (as it seems to apply to the
0x9c wValue, but who knows).

> While that doesn't seem to have any negative effect that I can spot so
> far, surely it must do something?

Heh. Possibly, but if we don't notice any difference I think it comes
down to whether we can get Russel's device to play nice with it somehow.
I've also asked if the vendor can provide some insight.

> > -static int ch341_init_set_baudrate(struct usb_device *dev,
> > +static int ch341_set_baudrate_lcr(struct usb_device *dev,
> >struct ch341_private *priv, unsigned 
> > ctrl)
> 
> Also, since you're renaming a bunch of stuff it might be worth
> renaming the last parameter of this from "ctrl" to "lcr" too at some
> point for consistency, and the same with the corresponding variable in
> ch341_set_termios.

Good point, will do.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: dwc3: pci: sometimes 'add' means 'set'

[Felipe Balbi]

even though the API is called
platform_device_add_properties(), it actually
replaces any previously set properties and only
maintains the last properties set.

This means that we need to duplicate
"linux,sysdev_is_parent" in all other occasions
instead of having it in a single place.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/dwc3-pci.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 2b73339f286b..1eca4c543667 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -73,20 +73,11 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 {
struct platform_device  *dwc3 = dwc->dwc3;
struct pci_dev  *pdev = dwc->pci;
-   int ret;
-
-   struct property_entry sysdev_property[] = {
-   PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
-   { },
-   };
-
-   ret = platform_device_add_properties(dwc3, sysdev_property);
-   if (ret)
-   return ret;
 
if (pdev->vendor == PCI_VENDOR_ID_AMD &&
pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
struct property_entry properties[] = {
+   PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
PROPERTY_ENTRY_U8("snps,lpm-nyet-threshold", 0xf),
PROPERTY_ENTRY_BOOL("snps,u2exit_lfps_quirk"),
@@ -115,6 +106,7 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
int ret;
 
struct property_entry properties[] = {
+   PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
PROPERTY_ENTRY_STRING("dr-mode", "peripheral"),
{ }
};
@@ -164,6 +156,7 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 pdev->device == PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI ||
 pdev->device == PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31)) {
struct property_entry properties[] = {
+   PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
PROPERTY_ENTRY_BOOL("snps,usb3_lpm_capable"),
PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
PROPERTY_ENTRY_BOOL("snps,dis_enblslpm_quirk"),
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings

[Aidan Thornton]

On Wed, Dec 14, 2016 at 3:28 PM, Johan Hovold  wrote:
> The ch341 driver is based on reverse-engineering and contains some bits
> which appear to be redundant and some which appear incompatible which
> certain devices.
>
> Specifically, some CH340 devices seem unable to change the initial line
> settings, which have so far been set to 5N1. Let's use a more reasonable
> 8N1 default instead.
>
> Note that we also need to set the ENABLE_RX bit or receive will be
> disabled after a reset during resume.

Lost track a little of the testing, but I don't think you ever got
Russel to test whether this worked using a driver that tried to change
this through direct register writes which seem to be the only thing
that has any effect on this strange hardware variant. Would be a
little surprised if it didn't at this point - changing the line
settings that way after initialization certainly works well enough to
cause us all a headache.

If so, I suggest it might be clearer to drop this patch altogether in
favour of patch 10 in the series, since the underlying problem is that
setting the baudrate and LCR register didn't work and patch 10 fixes
that.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/13] USB: serial: ch341: fix baud rate and line-control handling

[Aidan Thornton]

On Thu, Dec 15, 2016 at 3:58 PM, Johan Hovold  wrote:
> On Wed, Dec 14, 2016 at 04:28:07PM +0100, Johan Hovold wrote:
>> Some CH341 devices appear to require the use of the init vendor command
>> to set the baud rate and line-control register. Specifically, while
>> using direct register updates for speed and LCR seem to update those
>> settings correctly, not using the init command causes received data to
>> be buffered until a full endpoint-size packet (32 bytes) have been
>> received (i.e. the init command has some undocumented side-effect we
>> need).
>
> Turns out it is bit 7 in the divisor register which needs to be set in
> order for CH341A to not buffer incoming data this way. If set, direct
> register updates works also for these devices.
>
> This bit is only set since 4e46c410e050 ("USB: serial: ch341:
> reinitialize chip on reconfiguration") (in -next), which switched to
> using the init-vendor command.
>
> So it seems we could use a common implementation after all...

It would certainly simplify matters a lot if it turns out that was the
only problem with using direct register writes after all.

The removal of the second CH341_REQ_SERIAL_INIT by patch 2 of my
series and subsequent removal of it from baudrate setting too does
presumably mean that some register which was being set to a non-zero
value isn't anymore. (The one cryptically described as "enable
SFR_UART Control register and timer" in the vendor driver.) While that
doesn't seem to have any negative effect that I can spot so far,
surely it must do something?

> -static int ch341_init_set_baudrate(struct usb_device *dev,
> +static int ch341_set_baudrate_lcr(struct usb_device *dev,
>struct ch341_private *priv, unsigned ctrl)

Also, since you're renaming a bunch of stuff it might be worth
renaming the last parameter of this from "ctrl" to "lcr" too at some
point for consistency, and the same with the corresponding variable in
ch341_set_termios.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] USB3/DWC3: Add definition for global soc bus configuration register

[Changming Huang]

Add the macro definition for global soc bus configuration register 0/1

Signed-off-by: Changming Huang 
---
Changes in v2:
  - split the patch
  - add more macro definition for soc bus configuration register

 drivers/usb/dwc3/core.h |   26 ++
 1 file changed, 26 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index de5a857..065aa6f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -161,6 +161,32 @@
 
 /* Bit fields */
 
+/* Global SoC Bus Configuration Register 0 */
+#define AXI3_CACHE_TYPE_AW 0x8 /* write allocate */
+#define AXI3_CACHE_TYPE_AR 0x4 /* read allocate */
+#define AXI3_CACHE_TYPE_SNP0x2 /* cacheable */
+#define AXI3_CACHE_TYPE_BUF0x1 /* bufferable */
+#define DWC3_GSBUSCFG0_DATARD_SHIFT28
+#define DWC3_GSBUSCFG0_DESCRD_SHIFT24
+#define DWC3_GSBUSCFG0_DATAWR_SHIFT20
+#define DWC3_GSBUSCFG0_DESCWR_SHIFT16
+#define DWC3_GSBUSCFG0_SNP_MASK0x
+#define DWC3_GSBUSCFG0_DATABIGEND  (1 << 11)
+#define DWC3_GSBUSCFG0_DESCBIGEND  (1 << 10)
+#define DWC3_GSBUSCFG0_INCR256BRSTENA  (1 << 7) /* INCR256 burst */
+#define DWC3_GSBUSCFG0_INCR128BRSTENA  (1 << 6) /* INCR128 burst */
+#define DWC3_GSBUSCFG0_INCR64BRSTENA   (1 << 5) /* INCR64 burst */
+#define DWC3_GSBUSCFG0_INCR32BRSTENA   (1 << 4) /* INCR32 burst */
+#define DWC3_GSBUSCFG0_INCR16BRSTENA   (1 << 3) /* INCR16 burst */
+#define DWC3_GSBUSCFG0_INCR8BRSTENA(1 << 2) /* INCR8 burst */
+#define DWC3_GSBUSCFG0_INCR4BRSTENA(1 << 1) /* INCR4 burst */
+#define DWC3_GSBUSCFG0_INCRBRSTENA (1 << 0) /* undefined length enable */
+#define DWC3_GSBUSCFG0_INCRBRST_MASK   0xff
+
+/* Global SoC Bus Configuration Register 1 */
+#define DWC3_GSBUSCFG1_1KPAGEENA   (1 << 12) /* 1K page boundary enable */
+#define DWC3_GSBUSCFG1_PTRANSLIMIT_MASK0xf00
+
 /* Global Debug Queue/FIFO Space Available Register */
 #define DWC3_GDBGFIFOSPACE_NUM(n)  ((n) & 0x1f)
 #define DWC3_GDBGFIFOSPACE_TYPE(n) (((n) << 5) & 0x1e0)
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type

[Felipe Balbi]

Hi,

Jerry Huang  writes:
>> there's no need for that. This patch is in good format. I do have a question,
>> however: how do you know this will work for all users? Burst size is a 
>> function
>> of how wide the interconnect where dwc3 is attached to, is.
> So I need to generate one new property in usb node to identify my platform?

Well, we probably need a property to be passed, yes. But let's go
through it all first :-)

>> You could very well be degrading performance for some users here. Can you
>> send me the result of the following commands *without* this patch applied?
>> 
>> # mkdir -p /d
>> # mount -t debugfs none /d
>> # cat /d/*dwc3*/regdump
>> 
> Below is the regdump:
> root@ls1043ardb:/d/300.usb3# cat regdump
> GSBUSCFG0 = 0x00100080

so you already have INCR256 here. There's one note in the databook which
just caught my attention. It states the following:

"Undefined burst length has priority over all other burst lenghts."

This means that setting both INCR16 and undefined INCR is
unnecessary. Only Undefined INCR will be taken into consideration. Can
you check with your HW engineers what's the largest burst the
interconnect is supposed to support?

> GSBUSCFG1 = 0x0700

8 AXI pipelined requests

> GSNPSID = 0x5533280a

2.80a cool :-)

I'll check these settings on my platform as well and see if there's any
setting which would improve transfer speed. This is a very good idea,
btw, but we need to be careful about how to play with it.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH v2 0/3] usb: musb: da8xx: Add support of suspend / resume

[Alexandre Bailon]

There is no suspend / resume support for da8xx.
Add the PM methods to da8xx glue.
In addition, introduce a new quirk to preserve the session
(i.e not clear the devctl register).
Clearing devctl will power off vbus on da8xx platform
(when it is host mode) and then devices will be disconnected on resume.

Changes in v2:
* Rebased the patches on top off this series:
  http://www.spinics.net/lists/linux-usb/msg150856.html

Alexandre Bailon (3):
  usb: musb: da8xx: Add support of suspend / resume
  usb: musb: Add a quirk to preserve the session during suspend
  usb: musb: da8xx: Fix host mode suspend

 drivers/usb/musb/da8xx.c | 32 +++-
 drivers/usb/musb/musb_core.c |  3 ++-
 drivers/usb/musb/musb_core.h |  1 +
 3 files changed, 34 insertions(+), 2 deletions(-)

-- 
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] usb: musb: da8xx: Fix host mode suspend

[Alexandre Bailon]

On da8xx, VBUS is not maintained during suspend when musb is in host mode.
On resume, all the connected devices will be disconnected and then will
be enumerated again.
This happens because MUSB_DEVCTL is cleared during suspend.

Use the quirk MUSB_PRESERVE_SESSION to preseve MUSB_DEVCTL during suspend.

Signed-off-by: Alexandre Bailon 
---
 drivers/usb/musb/da8xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 7384179..69e3b40 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -457,7 +457,8 @@ static inline u8 get_vbus_power(struct device *dev)
 }
 
 static const struct musb_platform_ops da8xx_ops = {
-   .quirks = MUSB_DMA_CPPI | MUSB_INDEXED_EP,
+   .quirks = MUSB_DMA_CPPI | MUSB_INDEXED_EP |
+ MUSB_PRESERVE_SESSION,
.init   = da8xx_musb_init,
.exit   = da8xx_musb_exit,
 
-- 
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] usb: musb: Add a quirk to preserve the session during suspend

[Alexandre Bailon]

On da8xx, VBUS is not maintained during suspend when musb is in host mode.
On resume, all the connected devices will be disconnected and then will
be enumerated again.
This happens because MUSB_DEVCTL is cleared during suspend.
Add a quirk to not clear MUSB_DEVCTL and then preserve the  session during
a suspend.

Signed-off-by: Alexandre Bailon 
---
 drivers/usb/musb/musb_core.c | 3 ++-
 drivers/usb/musb/musb_core.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 56a4b97..c32024e 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2657,7 +2657,8 @@ static int musb_suspend(struct device *dev)
 
musb_platform_disable(musb);
musb_disable_interrupts(musb);
-   musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
+   if (!(musb->io.quirks & MUSB_PRESERVE_SESSION))
+   musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
WARN_ON(!list_empty(>pending_list));
 
spin_lock_irqsave(>lock, flags);
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index a611e2f..d9de3ca 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -172,6 +172,7 @@ struct musb_io;
  */
 struct musb_platform_ops {
 
+#define MUSB_PRESERVE_SESSION  BIT(7)
 #define MUSB_DMA_UX500 BIT(6)
 #define MUSB_DMA_CPPI41BIT(5)
 #define MUSB_DMA_CPPI  BIT(4)
-- 
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] usb: musb: da8xx: Add support of suspend / resume

[Alexandre Bailon]

Implement PM methods specifics for da8xx glue.
The only thing to do is to power off the phy.
As the registers are in retention during suspend,
there is no need to save them.

Signed-off-by: Alexandre Bailon 
---
 drivers/usb/musb/da8xx.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 26766a5..7384179 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -577,6 +577,34 @@ static int da8xx_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int da8xx_suspend(struct device *dev)
+{
+   int ret;
+   struct da8xx_glue *glue = dev_get_drvdata(dev);
+
+   ret = phy_power_off(glue->phy);
+   if (ret)
+   return ret;
+   clk_disable_unprepare(glue->clk);
+
+   return 0;
+}
+
+static int da8xx_resume(struct device *dev)
+{
+   int ret;
+   struct da8xx_glue *glue = dev_get_drvdata(dev);
+
+   ret = clk_prepare_enable(glue->clk);
+   if (ret)
+   return ret;
+   return phy_power_on(glue->phy);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(da8xx_pm_ops, da8xx_suspend, da8xx_resume);
+
 #ifdef CONFIG_OF
 static const struct of_device_id da8xx_id_table[] = {
{
@@ -592,6 +620,7 @@ static struct platform_driver da8xx_driver = {
.remove = da8xx_remove,
.driver = {
.name   = "musb-da8xx",
+   .pm = _pm_ops,
.of_match_table = of_match_ptr(da8xx_id_table),
},
 };
-- 
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: musb: Add a quirk to preserve MUSB_DEVCTL during suspend

[Alexandre Bailon]

On 12/16/2016 03:36 AM, Bin Liu wrote:
> On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:
>> On da8xx, VBUS is not maintained during suspend when musb is in host mode.
>> On resume, all the connected devices will be disconnected and then will
>> be enumerated again.
>> This happens because MUSB_DEVCTL is cleared during suspend.
>> Add a quirk to preserve MUSB_DEVCTL during a suspend.
>>
>> Signed-off-by: Alexandre Bailon 
>> ---
>>  drivers/usb/musb/musb_core.c | 13 +++--
>>  drivers/usb/musb/musb_core.h |  1 +
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>> index 9e22646..7e2cd98 100644
>> --- a/drivers/usb/musb/musb_core.c
>> +++ b/drivers/usb/musb/musb_core.c
>> @@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb *musb)
>>  
>>  }
>>  
>> -static void musb_generic_disable(struct musb *musb)
>> +static void musb_generic_disable(struct musb *musb, bool suspend)
> 
> I don't think I like it, so made a change [1] to remove this function,
> since it only has two lines of code.
> 
>>  {
>>  void __iomem*mbase = musb->mregs;
>>  
>>  musb_disable_interrupts(musb);
>>  
>>  /* off */
>> -musb_writeb(mbase, MUSB_DEVCTL, 0);
>> +if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))
>> +musb_writeb(mbase, MUSB_DEVCTL, 0);
> 
> So now you can move this quirk check into musb_suspend(),
> 
>>  }
>>  
>>  /*
>> @@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)
>>  {
>>  /* stop IRQs, timers, ... */
>>  musb_platform_disable(musb);
>> -musb_generic_disable(musb);
>> +musb_generic_disable(musb, false);
>>  musb_dbg(musb, "HDRC disabled");
>>  
>>  /* FIXME
>> @@ -2310,7 +2311,7 @@ musb_init_controller(struct device *dev, int nIrq, 
>> void __iomem *ctrl)
>>  
>>  /* be sure interrupts are disabled before connecting ISR */
>>  musb_platform_disable(musb);
>> -musb_generic_disable(musb);
>> +musb_generic_disable(musb, false);
>>  
>>  /* Init IRQ workqueue before request_irq */
>>  INIT_DELAYED_WORK(>irq_work, musb_irq_work);
>> @@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)
>>  musb_gadget_cleanup(musb);
>>  spin_lock_irqsave(>lock, flags);
>>  musb_platform_disable(musb);
>> -musb_generic_disable(musb);
>> +musb_generic_disable(musb, false);
>>  spin_unlock_irqrestore(>lock, flags);
>>  musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
>>  pm_runtime_dont_use_autosuspend(musb->controller);
>> @@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)
>>  unsigned long   flags;
>>  
>>  musb_platform_disable(musb);
>> -musb_generic_disable(musb);
>> +musb_generic_disable(musb, true);
>>  WARN_ON(!list_empty(>pending_list));
> 
> and all the changes above are no longer needed. Can you please revise
> this patch based on [1]?
> 
>>  
>>  spin_lock_irqsave(>lock, flags);
>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
>> index a611e2f..22961ef 100644
>> --- a/drivers/usb/musb/musb_core.h
>> +++ b/drivers/usb/musb/musb_core.h
>> @@ -172,6 +172,7 @@ struct musb_io;
>>   */
>>  struct musb_platform_ops {
>>  
>> +#define MUSB_PRESERVE_DEVCTLBIT(7)
> 
> Does it miss a TAB before BIT to align with follows?
I have checked and I don't think there is any missing TAB.

Regards,
Alexandre
> 
>>  #define MUSB_DMA_UX500  BIT(6)
>>  #define MUSB_DMA_CPPI41 BIT(5)
>>  #define MUSB_DMA_CPPI   BIT(4)
>> -- 
>> 2.7.3
>>
> 
> [1] http://www.spinics.net/lists/linux-usb/msg150856.html
> 
> Regards,
> -Bin.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type

[Jerry Huang]

> -Original Message-
> From: Felipe Balbi [mailto:ba...@kernel.org]
> Sent: Friday, December 16, 2016 5:17 PM
> To: Jerry Huang ; Jerry Huang
> ; gre...@linuxfoundation.org
> Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Rajesh Bhagat
> 
> Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
> 
> 
> Hi,
> 
> Jerry Huang  writes:
> >> -Original Message-
> >> From: Changming Huang [mailto:jerry.hu...@nxp.com]
> >> Sent: Tuesday, December 13, 2016 5:06 PM
> >> To: ba...@kernel.org; gre...@linuxfoundation.org
> >> Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Jerry
> >> Huang ; Rajesh Bhagat 
> >> Subject: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
> >>
> >> While enabling undefined length INCR burst type and INCR16 burst
> >> type, get better write performance on NXP Layerscape platform:
> >> around 3% improvement (from 364MB/s to 375MB/s).
> >>
> >> Signed-off-by: Changming Huang 
> >> Signed-off-by: Rajesh Bhagat 
> >> ---
> >>  drivers/usb/dwc3/core.c |6 ++
> >>  drivers/usb/dwc3/core.h |   13 +
> >>  2 files changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> >> fea4469..0e11891 100644
> >> --- a/drivers/usb/dwc3/core.c
> >> +++ b/drivers/usb/dwc3/core.c
> >> @@ -621,6 +621,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >>goto err0;
> >>}
> >>
> >> +  /* Enable Undefined Length INCR Burst Type and Enable INCR16
> >> Burst */
> >> +  reg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> >> +  reg &= ~DWC3_GSBUSCFG0_INCRBRSTMASK;
> >> +  reg |= DWC3_GSBUSCFG0_INCR16BRSTENA |
> >> DWC3_GSBUSCFG0_INCRBRSTENA;
> >> +  dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, reg);
> >> +
> >>/*
> >> * Write Linux Version Code to our GUID register so it's easy to figure
> >> * out which kernel version a bug was found.
> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
> >> 6b60e42..8bfdb77 100644
> >> --- a/drivers/usb/dwc3/core.h
> >> +++ b/drivers/usb/dwc3/core.h
> >> @@ -156,6 +156,19 @@
> >>
> >>  /* Bit fields */
> >>
> >> +/* Global SoC Bus Configuration Register 0 */
> >> +#define DWC3_GSBUSCFG0_DATABIGEND (1 << 11)
> >> +#define DWC3_GSBUSCFG0_DESCBIGEND (1 << 10)
> >> +#define DWC3_GSBUSCFG0_INCR256BRSTENA (1 << 7)
> >> +#define DWC3_GSBUSCFG0_INCR128BRSTENA (1 << 6)
> >> +#define DWC3_GSBUSCFG0_INCR64BRSTENA  (1 << 5)
> >> +#define DWC3_GSBUSCFG0_INCR32BRSTENA  (1 << 4)
> >> +#define DWC3_GSBUSCFG0_INCR16BRSTENA  (1 << 3)
> >> +#define DWC3_GSBUSCFG0_INCR8BRSTENA   (1 << 2)
> >> +#define DWC3_GSBUSCFG0_INCR4BRSTENA   (1 << 1)
> >> +#define DWC3_GSBUSCFG0_INCRBRSTENA(1 << 0)
> >> +#define DWC3_GSBUSCFG0_INCRBRSTMASK   0xff
> >> +
> >>  /* Global Debug Queue/FIFO Space Available Register */
> >>  #define DWC3_GDBGFIFOSPACE_NUM(n) ((n) & 0x1f)
> >>  #define DWC3_GDBGFIFOSPACE_TYPE(n)(((n) << 5) & 0x1e0)
> >> --
> > I will split this patch to two, one is for the performance tune, the
> > other for macro definition in header file.
> 
> there's no need for that. This patch is in good format. I do have a question,
> however: how do you know this will work for all users? Burst size is a 
> function
> of how wide the interconnect where dwc3 is attached to, is.
So I need to generate one new property in usb node to identify my platform?

> You could very well be degrading performance for some users here. Can you
> send me the result of the following commands *without* this patch applied?
> 
> # mkdir -p /d
> # mount -t debugfs none /d
> # cat /d/*dwc3*/regdump
> 
Below is the regdump:
root@ls1043ardb:/d/300.usb3# cat regdump
GSBUSCFG0 = 0x00100080
GSBUSCFG1 = 0x0700
GTXTHRCFG = 0x
GRXTHRCFG = 0x
GCTL = 0x30c11004
GEVTEN = 0x
GSTS = 0x3e81
GUCTL1 = 0x018a
GSNPSID = 0x5533280a
GGPIO = 0x
GUID = 0x00040900
GUCTL = 0x02008010
GBUSERRADDR0 = 0x
GBUSERRADDR1 = 0x
GPRTBIMAP0 = 0x
GPRTBIMAP1 = 0x
GHWPARAMS0 = 0x4020400a
GHWPARAMS1 = 0x81e0c93b
GHWPARAMS2 = 0x0130280a
GHWPARAMS3 = 0x04108485
GHWPARAMS4 = 0x47822004
GHWPARAMS5 = 0x04204108
GHWPARAMS6 = 0x09049c20
GHWPARAMS7 = 0x0308044d
GDBGFIFOSPACE = 0x0082
GDBGLTSSM = 0x4042
GPRTBIMAP_HS0 = 0x
GPRTBIMAP_HS1 = 0x
GPRTBIMAP_FS0 = 0x
GPRTBIMAP_FS1 = 0x
GUSB2PHYCFG(0) = 0x40102440
GUSB2PHYCFG(1) = 0x
GUSB2PHYCFG(2) = 0x
GUSB2PHYCFG(3) = 0x
GUSB2PHYCFG(4) = 0x
GUSB2PHYCFG(5) = 0x
GUSB2PHYCFG(6) = 0x
GUSB2PHYCFG(7) = 0x
GUSB2PHYCFG(8) = 0x
GUSB2PHYCFG(9) = 0x
GUSB2PHYCFG(10) = 0x
GUSB2PHYCFG(11) = 0x
GUSB2PHYCFG(12) = 0x
GUSB2PHYCFG(13) = 0x

[PATCH v2 2/2] USB3/DWC3: Enable undefined length INCR burst type

[Changming Huang]

While enabling undefined length INCR burst type and INCR16 burst type,
get better write performance on NXP Layerscape platform:
around 3% improvement (from 364MB/s to 375MB/s).

Signed-off-by: Changming Huang 
Signed-off-by: Rajesh Bhagat 
---
Changs in v2:
  - split patch
  - create one new function to handle soc bus configuration register

 drivers/usb/dwc3/core.c |   16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 369bab1..699a409 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -991,6 +991,20 @@ static void dwc3_get_properties(struct dwc3 *dwc)
dwc->imod_interval = 0;
 }
 
+/* set global soc bus configuration registers */
+static void dwc3_set_soc_bus_cfg(struct dwc3 *dwc)
+{
+   u32 cfg;
+
+   cfg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
+
+   /* Enable Undefined Length INCR Burst Type and Enable INCR16 Burst */
+   cfg &= ~DWC3_GSBUSCFG0_INCRBRST_MASK;
+   cfg |= DWC3_GSBUSCFG0_INCR16BRSTENA | DWC3_GSBUSCFG0_INCRBRSTENA;
+
+   dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);
+}
+
 /* check whether the core supports IMOD */
 bool dwc3_has_imod(struct dwc3 *dwc)
 {
@@ -1134,6 +1148,8 @@ static int dwc3_probe(struct platform_device *pdev)
goto err4;
}
 
+   dwc3_set_soc_bus_cfg(dwc);
+
dwc3_check_params(dwc);
 
ret = dwc3_core_init_mode(dwc);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type

[Felipe Balbi]

Hi,

Jerry Huang  writes:
>> -Original Message-
>> From: Changming Huang [mailto:jerry.hu...@nxp.com]
>> Sent: Tuesday, December 13, 2016 5:06 PM
>> To: ba...@kernel.org; gre...@linuxfoundation.org
>> Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Jerry Huang
>> ; Rajesh Bhagat 
>> Subject: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
>> 
>> While enabling undefined length INCR burst type and INCR16 burst type, get
>> better write performance on NXP Layerscape platform:
>> around 3% improvement (from 364MB/s to 375MB/s).
>> 
>> Signed-off-by: Changming Huang 
>> Signed-off-by: Rajesh Bhagat 
>> ---
>>  drivers/usb/dwc3/core.c |6 ++
>>  drivers/usb/dwc3/core.h |   13 +
>>  2 files changed, 19 insertions(+)
>> 
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
>> fea4469..0e11891 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -621,6 +621,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>  goto err0;
>>  }
>> 
>> +/* Enable Undefined Length INCR Burst Type and Enable INCR16
>> Burst */
>> +reg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
>> +reg &= ~DWC3_GSBUSCFG0_INCRBRSTMASK;
>> +reg |= DWC3_GSBUSCFG0_INCR16BRSTENA |
>> DWC3_GSBUSCFG0_INCRBRSTENA;
>> +dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, reg);
>> +
>>  /*
>>   * Write Linux Version Code to our GUID register so it's easy to figure
>>   * out which kernel version a bug was found.
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
>> 6b60e42..8bfdb77 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -156,6 +156,19 @@
>> 
>>  /* Bit fields */
>> 
>> +/* Global SoC Bus Configuration Register 0 */
>> +#define DWC3_GSBUSCFG0_DATABIGEND   (1 << 11)
>> +#define DWC3_GSBUSCFG0_DESCBIGEND   (1 << 10)
>> +#define DWC3_GSBUSCFG0_INCR256BRSTENA   (1 << 7)
>> +#define DWC3_GSBUSCFG0_INCR128BRSTENA   (1 << 6)
>> +#define DWC3_GSBUSCFG0_INCR64BRSTENA(1 << 5)
>> +#define DWC3_GSBUSCFG0_INCR32BRSTENA(1 << 4)
>> +#define DWC3_GSBUSCFG0_INCR16BRSTENA(1 << 3)
>> +#define DWC3_GSBUSCFG0_INCR8BRSTENA (1 << 2)
>> +#define DWC3_GSBUSCFG0_INCR4BRSTENA (1 << 1)
>> +#define DWC3_GSBUSCFG0_INCRBRSTENA  (1 << 0)
>> +#define DWC3_GSBUSCFG0_INCRBRSTMASK 0xff
>> +
>>  /* Global Debug Queue/FIFO Space Available Register */
>>  #define DWC3_GDBGFIFOSPACE_NUM(n)   ((n) & 0x1f)
>>  #define DWC3_GDBGFIFOSPACE_TYPE(n)  (((n) << 5) & 0x1e0)
>> --
> I will split this patch to two, one is for the performance tune, the
> other for macro definition in header file.

there's no need for that. This patch is in good format. I do have a
question, however: how do you know this will work for all users? Burst
size is a function of how wide the interconnect where dwc3 is attached
to, is.

You could very well be degrading performance for some users here. Can
you send me the result of the following commands *without* this patch
applied?

# mkdir -p /d
# mount -t debugfs none /d
# cat /d/*dwc3*/regdump

-- 
balbi


signature.asc
Description: PGP signature

[PATCH v2] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Peter Rosin]

The gpiod_get* function family does not want the -gpio suffix.
Use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional.
The descriptor based APIs handle active high/low automatically.
The vbus-gpios are output, request enable while getting the gpio.
Don't try to get any vbus-gpios for ports outside num-ports.

WTF? Big sigh.

Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio APIs")
Signed-off-by: Peter Rosin 
---

Hi!

Ok, I looked again and found one more problem with that garbage patch
(which did not affect me since I have just one port). You might want to
pick this up before v4.10-rc1, it's not nice to regress in the area of
over-current detection...

v1 -> v2 changes:
- use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional

Cheers,
peda

 drivers/usb/host/ohci-at91.c | 24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index 1b2e09c32c6b..66ec407df2db 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -43,7 +43,6 @@ struct at91_usbh_data {
struct gpio_desc *overcurrent_pin[AT91_MAX_USBH_PORTS];
u8 ports;   /* number of ports on root hub 
*/
u8 overcurrent_supported;
-   u8 vbus_pin_active_low[AT91_MAX_USBH_PORTS];
u8 overcurrent_status[AT91_MAX_USBH_PORTS];
u8 overcurrent_changed[AT91_MAX_USBH_PORTS];
 };
@@ -268,8 +267,7 @@ static void ohci_at91_usb_set_power(struct at91_usbh_data 
*pdata, int port, int
if (!valid_port(port))
return;
 
-   gpiod_set_value(pdata->vbus_pin[port],
-   pdata->vbus_pin_active_low[port] ^ enable);
+   gpiod_set_value(pdata->vbus_pin[port], enable);
 }
 
 static int ohci_at91_usb_get_power(struct at91_usbh_data *pdata, int port)
@@ -277,8 +275,7 @@ static int ohci_at91_usb_get_power(struct at91_usbh_data 
*pdata, int port)
if (!valid_port(port))
return -EINVAL;
 
-   return gpiod_get_value(pdata->vbus_pin[port]) ^
-  pdata->vbus_pin_active_low[port];
+   return gpiod_get_value(pdata->vbus_pin[port]);
 }
 
 /*
@@ -535,18 +532,17 @@ static int ohci_hcd_at91_drv_probe(struct platform_device 
*pdev)
pdata->ports = ports;
 
at91_for_each_port(i) {
-   pdata->vbus_pin[i] = devm_gpiod_get_optional(>dev,
-"atmel,vbus-gpio",
-GPIOD_IN);
+   if (i >= pdata->ports)
+   break;
+
+   pdata->vbus_pin[i] =
+   devm_gpiod_get_index_optional(>dev, "atmel,vbus",
+ i, GPIOD_OUT_HIGH);
if (IS_ERR(pdata->vbus_pin[i])) {
err = PTR_ERR(pdata->vbus_pin[i]);
dev_err(>dev, "unable to claim gpio \"vbus\": 
%d\n", err);
continue;
}
-
-   pdata->vbus_pin_active_low[i] = 
gpiod_get_value(pdata->vbus_pin[i]);
-
-   ohci_at91_usb_set_power(pdata, i, 1);
}
 
at91_for_each_port(i) {
@@ -554,8 +550,8 @@ static int ohci_hcd_at91_drv_probe(struct platform_device 
*pdev)
break;
 
pdata->overcurrent_pin[i] =
-   devm_gpiod_get_optional(>dev,
-   "atmel,oc-gpio", GPIOD_IN);
+   devm_gpiod_get_index_optional(>dev, "atmel,oc",
+ i, GPIOD_IN);
if (IS_ERR(pdata->overcurrent_pin[i])) {
err = PTR_ERR(pdata->overcurrent_pin[i]);
dev_err(>dev, "unable to claim gpio 
\"overcurrent\": %d\n", err);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html