Re: high irqs-off latency caused by USB serial driver

[David Mosberger]

Actually, I just found this commit:

94dfd7edf USB: HCD: support giveback of URB in tasklet context

It won't fix the problem entirely, but I think it should cut the worst-case
irqs-disabled latency about in half, which would be a good improvement.

Does anybody know why HCD_BH is not enabled in ohci-hcd.c?

  --david
--
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: high irqs-off latency caused by USB serial driver

[David Mosberger]

On Tue, Aug 22, 2017 at 3:40 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Tue, Aug 22, 2017 at 02:44:20PM -0600, David Mosberger wrote:

>> > There was an option a while ago to turn USB irqs
>> > into threaded irqs, do those work on your platform?  If so, that might
>> > help you out here.
>>
>> Do you mean this:
>>
>>https://lkml.org/lkml/2008/10/20/465
>>
>> or is there something else/newer?
>
> I think there was something newer than that almost-a-decade-old thread,
> but I don't remember.  Look at what the RT kernel patch does, it might
> be in there if it wasn't merged into the tree already.

Actually, I'm not able to find much in this direction.
Do you have something more specific I could be looking for?

I don't think the RT patches would (readily) work for my case as I have
a (soft-)realtime interrupt handler that depends on other drivers (e.g,.
SPI and DMA).

Looking at the OHCI and EHCI drivers more carefully, even if those
were turned into threaded handlers, the problem would not
(automatically) go away since they implicitly (OHCI) or explicitly (EHCI)
disable (local) interrupt delivery while processing the URBs (in particular,
while giving them back, which can trigger a lot of extra work).

I'd very much appreciate any pointers to any work or thought that
might have gone into improving this situation (in particular: returning
URBs with interrupts enabled).  The USB stack disabling
interrupts for such extended periods of time surely isn't a problem just
in my situation.

  --david
--
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: high irqs-off latency caused by USB serial driver

[David Mosberger]

On Tue, Aug 22, 2017 at 3:40 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Tue, Aug 22, 2017 at 02:44:20PM -0600, David Mosberger wrote:
>> Greg,
>>
>> On Tue, Aug 22, 2017 at 2:25 PM, Greg KH <gre...@linuxfoundation.org> wrote:
>>
>> > USB has always been a big problem with this, the IRQ patch is very long,
>> > and messy and complex.
>>
>> Yeah.
>>
>> > There was an option a while ago to turn USB irqs
>> > into threaded irqs, do those work on your platform?  If so, that might
>> > help you out here.
>>
>> Do you mean this:
>>
>>https://lkml.org/lkml/2008/10/20/465
>>
>> or is there something else/newer?
>
> I think there was something newer than that almost-a-decade-old thread,
> but I don't remember.  Look at what the RT kernel patch does, it might
> be in there if it wasn't merged into the tree already.

OK, thanks for the pointer.

> What are your requirements that this code path is causing you problems?
> Odds are your USB host controller is pretty horrid, any chance to use a
> different chip for it?

We just have some other soft-realtime stuff going on that doesn't like overly
long periods with interrupts disabled.

  --david
--
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: high irqs-off latency caused by USB serial driver

[David Mosberger]

Greg,

On Tue, Aug 22, 2017 at 2:25 PM, Greg KH  wrote:

> USB has always been a big problem with this, the IRQ patch is very long,
> and messy and complex.

Yeah.

> There was an option a while ago to turn USB irqs
> into threaded irqs, do those work on your platform?  If so, that might
> help you out here.

Do you mean this:

   https://lkml.org/lkml/2008/10/20/465

or is there something else/newer?

> The usb-serial path should be really "short" overall, compared with the
> ohci and tty core logic involved.  What USB-serial driver is this that
> you are using here?

It's the FTDI driver (via generic).  I did a quick
back-of-the-envelope calculation
and it looks like by changing usb/serial/generic.c to move most of the interrupt
work to softinterrupt would save about 200 usec out of the 746 usec.  Surely
that'd be an improvement, but moving the entire OHCI path to softirq would be
a much bigger win.

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

high irqs-off latency caused by USB serial driver

[David Mosberger]

Has anyone here looked into reducing the amount of time the USB serial
driver disables interrupts?  On an ARM system, I'm seeing about 746 us
of latency for handling a USB interrupt, which seems rather excessive.
I attached a trace captured with the irqsoff tracer.

Note: the 500MHz ARM cycle counter was used for timestamping
the entry so the reported times have to be doubled to get actual time
in micro-seconds.

>From what I can see:

 o OHCI takes interrupt and frees up an URB that is done
 o USB serial driver submits a new URB (for transmit, I think)
 o another URB is freed (receive, I think), then a new receive URB is
submited

I'm hoping there is something silly going on and things are done at the
hardirq level when they should be done as softirqs, but I just started
looking into this.

Anyone have any helpful thoughts/pointers?

Thanks!

  --david

# tracer: irqsoff
#
# irqsoff latency trace v1.1.5 on 4.9.44+
# 
# latency: 373 us, #188/188, CPU#0 | (M:server VP:0, KP:0, SP:0 HP:0)
#-
#| task: kworker/0:0-1437 (uid:0 nice:0 policy:0 rt_prio:0)
#-
#  => started at: __irq_usr
#  => ended at:   __schedule
#
#
#  _--=> CPU#
# / _-=> irqs-off
#| / _=> need-resched
#|| / _---=> hardirq/softirq
#||| / _--=> preempt-depth
# / delay
#  cmd pid   | time  |   caller
# \   /  |  \|   /
   <...>-18960d... 1650: __irq_usr
   <...>-18960d... 5443: aic5_handle <-__irq_usr
   <...>-18960d... 7472: irq_get_domain_generic_chip <-aic5_handle
   <...>-18960d... 10194: __handle_domain_irq <-aic5_handle
   <...>-18960d... 11685: irq_enter <-__handle_domain_irq
   <...>-18960d.h. 13521: irq_find_mapping <-__handle_domain_irq
   <...>-18960d.h. 15403: generic_handle_irq <-__handle_domain_irq
   <...>-18960d.h. 16673: irq_to_desc <-generic_handle_irq
   <...>-18960d.h. 18662: handle_fasteoi_irq <-generic_handle_irq
   <...>-18960d.h. 20194: irq_may_run <-handle_fasteoi_irq
   <...>-18960d.h. 22426: handle_irq_event <-handle_fasteoi_irq
   <...>-18960d.h. 23743: handle_irq_event_percpu <-handle_irq_event
   <...>-18960d.h. 25072: __handle_irq_event_percpu
<-handle_irq_event_percpu
   <...>-18960d.h. 26492: usb_hcd_irq <-__handle_irq_event_percpu
   <...>-18960d.h. 28284: ohci_irq <-usb_hcd_irq
   <...>-18960d.h. 32109: arm_heavy_mb <-ohci_irq
   <...>-18960d.h. 33430: l2c210_sync <-arm_heavy_mb
   <...>-18960d.h. 37489: add_to_done_list.part.0 <-ohci_irq
   <...>-18960d.h. 39777: add_to_done_list.part.0 <-ohci_irq
   <...>-18960d.h. 41538: ohci_work <-ohci_irq
   <...>-18960d.h. 43671: td_done <-ohci_work
   <...>-18960d.h. 47023: finish_urb <-ohci_work
   <...>-18960d.h. 48342: urb_free_priv <-finish_urb
   <...>-18960d.h. 49684: td_free <-urb_free_priv
   <...>-18960d.h. 51204: dma_pool_free <-td_free
   <...>-18960d.h. 54677: kfree <-urb_free_priv
   <...>-18960d.h. 56661: ___cache_free <-kfree
   <...>-18960d.h. 59101: usb_hcd_unlink_urb_from_ep <-finish_urb
   <...>-18960d.h. 61064: usb_hcd_giveback_urb <-finish_urb
   <...>-18960d.h. 62745: __usb_hcd_giveback_urb <-usb_hcd_giveback_urb
   <...>-18960d.h. 64082: unmap_urb_for_dma <-__usb_hcd_giveback_urb
   <...>-18960d.h. 65456: usb_hcd_unmap_urb_for_dma <-unmap_urb_for_dma
   <...>-18960d.h. 66757: usb_hcd_unmap_urb_setup_for_dma
<-usb_hcd_unmap_urb_for_dma
   <...>-18960d.h. 69135: arm_dma_unmap_page <-usb_hcd_unmap_urb_for_dma
   <...>-18960d.h. 70590: __dma_page_dev_to_cpu <-arm_dma_unmap_page
   <...>-18960d.h. 72810: usb_anchor_suspend_wakeups
<-__usb_hcd_giveback_urb
   <...>-18960d.h. 74157: usb_unanchor_urb <-__usb_hcd_giveback_urb
   <...>-18960d.h. 76333: usb_serial_generic_write_bulk_callback
<-__usb_hcd_giveback_urb
   <...>-18960d.h. 79605: usb_serial_generic_write_start
<-usb_serial_generic_write_bulk_callback
   <...>-18960d.h. 84314: ftdi_prepare_write_buffer
<-usb_serial_generic_write_start
   <...>-18960d.h. 91988: usb_submit_urb <-usb_serial_generic_write_start
   <...>-18960d.h. 96555: usb_hcd_submit_urb <-usb_submit_urb
   <...>-18960d.h. 98117: usb_get_urb <-usb_hcd_submit_urb
   <...>-18960d.h. 99710: usb_hcd_map_urb_for_dma <-usb_hcd_submit_urb
   <...>-18960d.h. 102131: arm_dma_map_page <-usb_hcd_map_urb_for_dma
   <...>-18960d.h. 103581: __dma_page_cpu_to_dev <-arm_dma_map_page
   <...>-18960d.h. 105096: dma_cache_maint_page <-__dma_page_cpu_to_dev
   <...>-18960d.h. 107224: l2c210_clean_range <-__dma_page_cpu_to_dev
   <...>-18960d.h. 110437: ohci_urb_enqueue <-usb_hcd_submit_urb
   <...>-18960d.h. 114226: __kmalloc <-ohci_urb_enqueue
   <...>-18960d.h. 115568: kmalloc_slab <-__kmalloc
   

[PATCH 2/2] drivers: usb: core: Minimize irq disabling in usb_sg_cancel()

[David Mosberger-Tang]

From: David Mosberger <dav...@egauge.net>

Restructure usb_sg_cancel() so we don't have to disable interrupts
while cancelling the URBs.

Suggested-by: Alan Stern <st...@rowland.harvard.edu>
Signed-off-by: David Mosberger <dav...@egauge.net>
---
 drivers/usb/core/message.c | 37 +
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index e13a2e5..ea681f1 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -577,31 +577,28 @@ EXPORT_SYMBOL_GPL(usb_sg_wait);
 void usb_sg_cancel(struct usb_sg_request *io)
 {
unsigned long flags;
+   int i, retval;
 
spin_lock_irqsave(>lock, flags);
+   if (io->status) {
+   spin_unlock_irqrestore(>lock, flags);
+   return;
+   }
+   /* shut everything down */
+   io->status = -ECONNRESET;
+   spin_unlock_irqrestore(>lock, flags);
 
-   /* shut everything down, if it didn't already */
-   if (!io->status) {
-   int i;
+   for (i = io->entries - 1; i >= 0; --i) {
+   usb_block_urb(io->urbs[i]);
 
-   io->status = -ECONNRESET;
-   spin_unlock(>lock);
-   for (i = 0; i < io->entries; i++) {
-   int retval;
-
-   usb_block_urb(io->urbs[i]);
-
-   retval = usb_unlink_urb(io->urbs[i]);
-   if (retval != -EINPROGRESS
-   && retval != -ENODEV
-   && retval != -EBUSY
-   && retval != -EIDRM)
-   dev_warn(>dev->dev, "%s, unlink --> %d\n",
-   __func__, retval);
-   }
-   spin_lock(>lock);
+   retval = usb_unlink_urb(io->urbs[i]);
+   if (retval != -EINPROGRESS
+   && retval != -ENODEV
+   && retval != -EBUSY
+   && retval != -EIDRM)
+   dev_warn(>dev->dev, "%s, unlink --> %d\n",
+__func__, retval);
}
-   spin_unlock_irqrestore(>lock, flags);
 }
 EXPORT_SYMBOL_GPL(usb_sg_cancel);
 
-- 
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 1/2] drivers: usb: core: Don't disable irqs in usb_sg_wait() during URB submit.

[David Mosberger-Tang]

From: David Mosberger <dav...@egauge.net>

usb_submit_urb() may take quite long to execute.  For example, a
single sg list may have 30 or more entries, possibly leading to that
many calls to DMA-map pages.  This can cause interrupt latency of
several hundred micro-seconds.

Avoid the problem by releasing the io->lock spinlock and re-enabling
interrupts before calling usb_submit_urb().  This opens races with
usb_sg_cancel() and sg_complete().  Handle those races by using
usb_block_urb() to stop URBs from being submitted after
usb_sg_cancel() or sg_complete() with error.

Note that usb_unlink_urb() is guaranteed to return -ENODEV if
!io->urbs[i]->dev and since the -ENODEV case is already handled,
we don't have to check for !io->urbs[i]->dev explicitly.

Before this change, reading 512MB from an ext3 filesystem on a USB
memory stick showed a throughput of 12 MB/s with about 500 missed
deadlines.

With this change, reading the same file gave the same throughput but
only one or two missed deadlines.

Signed-off-by: David Mosberger <dav...@egauge.net>
---
 drivers/usb/core/message.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 8e641b5..e13a2e5 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -302,9 +302,10 @@ static void sg_complete(struct urb *urb)
 */
spin_unlock(>lock);
for (i = 0, found = 0; i < io->entries; i++) {
-   if (!io->urbs[i] || !io->urbs[i]->dev)
+   if (!io->urbs[i])
continue;
if (found) {
+   usb_block_urb(io->urbs[i]);
retval = usb_unlink_urb(io->urbs[i]);
if (retval != -EINPROGRESS &&
retval != -ENODEV &&
@@ -515,12 +516,10 @@ void usb_sg_wait(struct usb_sg_request *io)
int retval;
 
io->urbs[i]->dev = io->dev;
-   retval = usb_submit_urb(io->urbs[i], GFP_ATOMIC);
-
-   /* after we submit, let completions or cancellations fire;
-* we handshake using io->status.
-*/
spin_unlock_irq(>lock);
+
+   retval = usb_submit_urb(io->urbs[i], GFP_NOIO);
+
switch (retval) {
/* maybe we retrying will recover */
case -ENXIO:/* hc didn't queue this one */
@@ -590,8 +589,8 @@ void usb_sg_cancel(struct usb_sg_request *io)
for (i = 0; i < io->entries; i++) {
int retval;
 
-   if (!io->urbs[i]->dev)
-   continue;
+   usb_block_urb(io->urbs[i]);
+
retval = usb_unlink_urb(io->urbs[i]);
if (retval != -EINPROGRESS
&& retval != -ENODEV
-- 
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: [PATCH] drivers: usb: core: Don't disable irqs in usb_sg_wait() during URB submit.

[David Mosberger-Tang]

Alan,

Thanks for your feedback!

> This should now be GFP_NOIO.

OK, I updated the patch to fix that.  Good catch.

> Strictly speaking, this loop should run backward.  Then the
> spin_unlock() could be replaced with spin_unlock_irqrestore().

Good idea.  A separate patch for this is included.

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

high interrupt latency due to usb_sg_wait()

[David Mosberger-Tang]

[Second transmission; hopefully this one will go through...]

Alan,

How about the attached patch?  Works for me but definitely needs more
review and testing.

  --david
--
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] drivers: usb: core: Don't disable irqs in usb_sg_wait() during URB submit.

[David Mosberger-Tang]

From: David Mosberger <dav...@egauge.net>

usb_submit_urb() may take quite long to execute.  For example, a
single sg list may have 30 or more entries, possibly leading to that
many calls to DMA-map pages.  This can cause interrupt latency of
several hundred micro-seconds.

Avoid the problem by releasing the io->lock spinlock and re-enabling
interrupts before calling usb_submit_urb().  This opens races with
usb_sg_cancel() and sg_complete().  Handle those races by using
usb_block_urb() to stop URBs from being submitted after
usb_sg_cancel() or sg_complete() with error.

Note that usb_unlink_urb() is guaranteed to return -ENODEV if
!io->urbs[i]->dev and since the -ENODEV case is already handled,
we don't have to check for !io->urbs[i]->dev explicitly.

Before this change, reading 512MB from an ext3 filesystem on a USB
memory stick showed a throughput of 12 MB/s with about 500 missed
deadlines.

With this change, reading the same file gave the same throughput but
only one or two missed deadlines.

Signed-off-by: David Mosberger <dav...@egauge.net>
---
 drivers/usb/core/message.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 8e641b5..8ead329 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -302,9 +302,10 @@ static void sg_complete(struct urb *urb)
 */
spin_unlock(>lock);
for (i = 0, found = 0; i < io->entries; i++) {
-   if (!io->urbs[i] || !io->urbs[i]->dev)
+   if (!io->urbs[i])
continue;
if (found) {
+   usb_block_urb(io->urbs[i]);
retval = usb_unlink_urb(io->urbs[i]);
if (retval != -EINPROGRESS &&
retval != -ENODEV &&
@@ -515,12 +516,10 @@ void usb_sg_wait(struct usb_sg_request *io)
int retval;
 
io->urbs[i]->dev = io->dev;
+   spin_unlock_irq(>lock);
+
retval = usb_submit_urb(io->urbs[i], GFP_ATOMIC);
 
-   /* after we submit, let completions or cancellations fire;
-* we handshake using io->status.
-*/
-   spin_unlock_irq(>lock);
switch (retval) {
/* maybe we retrying will recover */
case -ENXIO:/* hc didn't queue this one */
@@ -590,8 +589,8 @@ void usb_sg_cancel(struct usb_sg_request *io)
for (i = 0; i < io->entries; i++) {
int retval;
 
-   if (!io->urbs[i]->dev)
-   continue;
+   usb_block_urb(io->urbs[i]);
+
retval = usb_unlink_urb(io->urbs[i]);
if (retval != -EINPROGRESS
&& retval != -ENODEV
-- 
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

high interrupt latency due to usb_sg_wait()

[David Mosberger]

We are seeing relatively high interrupt latencies due to usb_sg_wait()
calling usb_submit_urb() with interrupts disabled (as a result of its
spin_lock_irq() call).

As far as I can see, io->lock protects io->status and it's not really
necessary to hold the lock (or disable interrupts) during the
usb_submit_urb() call.

Note that the current code doesn't protect against multiple concurrent
calls to usb_sg_wait(): a second caller of usb_sg_wait() could acquire
io->lock when the first caller drops it after calling usb_submit_urb()
and then it could resubmit the same URBs before the first caller gets
a chance to update io->status.  This is perhaps an outright bug?

Am I missing something?

 --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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] usb: host: max3421-hcd: use time_after()

[David Mosberger]

Acked-by: David Mosberger dav...@egauge.net

On Mon, Dec 15, 2014 at 8:22 AM, Asaf Vertz asaf.ve...@tandemg.com wrote:
 To be future-proof and for better readability the time comparisons are
 modified to use time_after() instead of plain, error-prone math.

 Signed-off-by: Asaf Vertz asaf.ve...@tandemg.com
 ---
  drivers/usb/host/max3421-hcd.c |3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

 diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
 index 6234c75..aaaff94 100644
 --- a/drivers/usb/host/max3421-hcd.c
 +++ b/drivers/usb/host/max3421-hcd.c
 @@ -55,6 +55,7 @@
   * single thread (max3421_spi_thread).
   */

 +#include linux/jiffies.h
  #include linux/module.h
  #include linux/spi/spi.h
  #include linux/usb.h
 @@ -1291,7 +1292,7 @@ max3421_handle_irqs(struct usb_hcd *hcd)
 char sbuf[16 * 16], *dp, *end;
 int i;

 -   if (jiffies - last_time  5*HZ) {
 +   if (time_after(jiffies, last_time + 5*HZ)) {
 dp = sbuf;
 end = sbuf + sizeof(sbuf);
 *dp = '\0';
 --
 1.7.0.4




-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: host: max3421-hcd: Use atomic bitops in lieu of bit fields

[David Mosberger]

From: David Mosberger-Tang dav...@egauge.net

Bit fields are not MP-safe.

Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/host/max3421-hcd.c |   45 ++--
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index 858efcf..f8ecd7d 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -102,6 +102,15 @@ enum scheduling_pass {
SCHED_PASS_DONE
 };
 
+/* Bit numbers for max3421_hcd-todo: */
+enum {
+   ENABLE_IRQ = 0,
+   RESET_HCD,
+   RESET_PORT,
+   CHECK_UNLINK,
+   IOPIN_UPDATE
+};
+
 struct max3421_dma_buf {
u8 data[2];
 };
@@ -146,11 +155,7 @@ struct max3421_hcd {
u8 hien;
u8 mode;
u8 iopins[2];
-   unsigned int do_enable_irq:1;
-   unsigned int do_reset_hcd:1;
-   unsigned int do_reset_port:1;
-   unsigned int do_check_unlink:1;
-   unsigned int do_iopin_update:1;
+   unsigned long todo;
 #ifdef DEBUG
unsigned long err_stat[16];
 #endif
@@ -1165,10 +1170,8 @@ max3421_irq_handler(int irq, void *dev_id)
if (max3421_hcd-spi_thread 
max3421_hcd-spi_thread-state != TASK_RUNNING)
wake_up_process(max3421_hcd-spi_thread);
-   if (!max3421_hcd-do_enable_irq) {
-   max3421_hcd-do_enable_irq = 1;
+   if (!test_and_set_bit(ENABLE_IRQ, max3421_hcd-todo))
disable_irq_nosync(spi-irq);
-   }
return IRQ_HANDLED;
 }
 
@@ -1423,10 +1426,8 @@ max3421_spi_thread(void *dev_id)
spi_wr8(hcd, MAX3421_REG_HIEN, max3421_hcd-hien);
 
set_current_state(TASK_INTERRUPTIBLE);
-   if (max3421_hcd-do_enable_irq) {
-   max3421_hcd-do_enable_irq = 0;
+   if (test_and_clear_bit(ENABLE_IRQ, max3421_hcd-todo))
enable_irq(spi-irq);
-   }
schedule();
__set_current_state(TASK_RUNNING);
}
@@ -1440,23 +1441,18 @@ max3421_spi_thread(void *dev_id)
else if (!max3421_hcd-curr_urb)
i_worked |= max3421_select_and_start_urb(hcd);
 
-   if (max3421_hcd-do_reset_hcd) {
+   if (test_and_clear_bit(RESET_HCD, max3421_hcd-todo))
/* reset the HCD: */
-   max3421_hcd-do_reset_hcd = 0;
i_worked |= max3421_reset_hcd(hcd);
-   }
-   if (max3421_hcd-do_reset_port) {
+   if (test_and_clear_bit(RESET_PORT, max3421_hcd-todo)) {
/* perform a USB bus reset: */
-   max3421_hcd-do_reset_port = 0;
spi_wr8(hcd, MAX3421_REG_HCTL,
BIT(MAX3421_HCTL_BUSRST_BIT));
i_worked = 1;
}
-   if (max3421_hcd-do_check_unlink) {
-   max3421_hcd-do_check_unlink = 0;
+   if (test_and_clear_bit(CHECK_UNLINK, max3421_hcd-todo))
i_worked |= max3421_check_unlink(hcd);
-   }
-   if (max3421_hcd-do_iopin_update) {
+   if (test_and_clear_bit(IOPIN_UPDATE, max3421_hcd-todo)) {
/*
 * IOPINS1/IOPINS2 do not auto-increment, so we can't
 * use spi_wr_buf().
@@ -1469,7 +1465,6 @@ max3421_spi_thread(void *dev_id)
spi_wr8(hcd, MAX3421_REG_IOPINS1 + i, val);
max3421_hcd-iopins[i] = val;
}
-   max3421_hcd-do_iopin_update = 0;
i_worked = 1;
}
}
@@ -1485,7 +1480,7 @@ max3421_reset_port(struct usb_hcd *hcd)
 
max3421_hcd-port_status = ~(USB_PORT_STAT_ENABLE |
  USB_PORT_STAT_LOW_SPEED);
-   max3421_hcd-do_reset_port = 1;
+   set_bit(RESET_PORT, max3421_hcd-todo);
wake_up_process(max3421_hcd-spi_thread);
return 0;
 }
@@ -1498,7 +1493,7 @@ max3421_reset(struct usb_hcd *hcd)
hcd-self.sg_tablesize = 0;
hcd-speed = HCD_USB2;
hcd-self.root_hub-speed = USB_SPEED_FULL;
-   max3421_hcd-do_reset_hcd = 1;
+   set_bit(RESET_HCD, max3421_hcd-todo);
wake_up_process(max3421_hcd-spi_thread);
return 0;
 }
@@ -1590,7 +1585,7 @@ max3421_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, 
int status)
 */
retval = usb_hcd_check_unlink_urb(hcd, urb, status);
if (retval == 0) {
-   max3421_hcd-do_check_unlink = 1;
+   set_bit(CHECK_UNLINK, max3421_hcd-todo);
wake_up_process(max3421_hcd-spi_thread);
}
spin_unlock_irqrestore(max3421_hcd-lock, flags

[PATCH] usb: host: max3421-hcd: Fix max3421_reset_port() to set USB_PORT_STAT_RESET

[David Mosberger]

From: David Mosberger-Tang dav...@egauge.net

Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/host/max3421-hcd.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index f8ecd7d..6dbf1e9 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -1480,6 +1480,7 @@ max3421_reset_port(struct usb_hcd *hcd)
 
max3421_hcd-port_status = ~(USB_PORT_STAT_ENABLE |
  USB_PORT_STAT_LOW_SPEED);
+   max3421_hcd-port_status |= USB_PORT_STAT_RESET;
set_bit(RESET_PORT, max3421_hcd-todo);
wake_up_process(max3421_hcd-spi_thread);
return 0;
-- 
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] usb: host: max3421-hcd: unconditionally use GFP_ATOMIC in max3421_urb_enqueue()

[David Mosberger]

On Thu, Jun 19, 2014 at 1:44 PM, Alexey Khoroshilov
khoroshi...@ispras.ru wrote:
 As far as kzalloc() is called with spinlock held,
 we have to pass GFP_ATOMIC regardless of mem_flags argument.

Good catch, thanks!

 Found by Linux Driver Verification project (linuxtesting.org).

 Signed-off-by: Alexey Khoroshilov khoroshi...@ispras.ru

Acked-by: David Mosberger dav...@egauge.net

 ---
  drivers/usb/host/max3421-hcd.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
 index 858efcfda50b..ed22424dbec7 100644
 --- a/drivers/usb/host/max3421-hcd.c
 +++ b/drivers/usb/host/max3421-hcd.c
 @@ -1551,7 +1551,7 @@ max3421_urb_enqueue(struct usb_hcd *hcd, struct urb 
 *urb, gfp_t mem_flags)
 max3421_ep = urb-ep-hcpriv;
 if (!max3421_ep) {
 /* gets freed in max3421_endpoint_disable: */
 -   max3421_ep = kzalloc(sizeof(struct max3421_ep), mem_flags);
 +   max3421_ep = kzalloc(sizeof(struct max3421_ep), GFP_ATOMIC);
 if (!max3421_ep) {
 retval = -ENOMEM;
 goto out;
 --
 1.9.1




-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: host: max3421-hcd: Fix max3421_reset_port() to set USB_PORT_STAT_RESET

[David Mosberger]

From: David Mosberger-Tang dav...@egauge.net

Signed-off-by: Davidm Mosberger dav...@egauge.net
---
 drivers/usb/host/max3421-hcd.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index f8ecd7d..6dbf1e9 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -1480,6 +1480,7 @@ max3421_reset_port(struct usb_hcd *hcd)
 
max3421_hcd-port_status = ~(USB_PORT_STAT_ENABLE |
  USB_PORT_STAT_LOW_SPEED);
+   max3421_hcd-port_status |= USB_PORT_STAT_RESET;
set_bit(RESET_PORT, max3421_hcd-todo);
wake_up_process(max3421_hcd-spi_thread);
return 0;
-- 
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

[PATCH] usb: host: max3421-hcd: Use atomic bitops in lieu of bit fields

[David Mosberger]

From: David Mosberger-Tang dav...@egauge.net

Bit fields are not MP-safe.

Signed-off-by: Davidm Mosberger dav...@egauge.net
---
 drivers/usb/host/max3421-hcd.c |   45 ++--
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index 858efcf..f8ecd7d 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -102,6 +102,15 @@ enum scheduling_pass {
SCHED_PASS_DONE
 };
 
+/* Bit numbers for max3421_hcd-todo: */
+enum {
+   ENABLE_IRQ = 0,
+   RESET_HCD,
+   RESET_PORT,
+   CHECK_UNLINK,
+   IOPIN_UPDATE
+};
+
 struct max3421_dma_buf {
u8 data[2];
 };
@@ -146,11 +155,7 @@ struct max3421_hcd {
u8 hien;
u8 mode;
u8 iopins[2];
-   unsigned int do_enable_irq:1;
-   unsigned int do_reset_hcd:1;
-   unsigned int do_reset_port:1;
-   unsigned int do_check_unlink:1;
-   unsigned int do_iopin_update:1;
+   unsigned long todo;
 #ifdef DEBUG
unsigned long err_stat[16];
 #endif
@@ -1165,10 +1170,8 @@ max3421_irq_handler(int irq, void *dev_id)
if (max3421_hcd-spi_thread 
max3421_hcd-spi_thread-state != TASK_RUNNING)
wake_up_process(max3421_hcd-spi_thread);
-   if (!max3421_hcd-do_enable_irq) {
-   max3421_hcd-do_enable_irq = 1;
+   if (!test_and_set_bit(ENABLE_IRQ, max3421_hcd-todo))
disable_irq_nosync(spi-irq);
-   }
return IRQ_HANDLED;
 }
 
@@ -1423,10 +1426,8 @@ max3421_spi_thread(void *dev_id)
spi_wr8(hcd, MAX3421_REG_HIEN, max3421_hcd-hien);
 
set_current_state(TASK_INTERRUPTIBLE);
-   if (max3421_hcd-do_enable_irq) {
-   max3421_hcd-do_enable_irq = 0;
+   if (test_and_clear_bit(ENABLE_IRQ, max3421_hcd-todo))
enable_irq(spi-irq);
-   }
schedule();
__set_current_state(TASK_RUNNING);
}
@@ -1440,23 +1441,18 @@ max3421_spi_thread(void *dev_id)
else if (!max3421_hcd-curr_urb)
i_worked |= max3421_select_and_start_urb(hcd);
 
-   if (max3421_hcd-do_reset_hcd) {
+   if (test_and_clear_bit(RESET_HCD, max3421_hcd-todo))
/* reset the HCD: */
-   max3421_hcd-do_reset_hcd = 0;
i_worked |= max3421_reset_hcd(hcd);
-   }
-   if (max3421_hcd-do_reset_port) {
+   if (test_and_clear_bit(RESET_PORT, max3421_hcd-todo)) {
/* perform a USB bus reset: */
-   max3421_hcd-do_reset_port = 0;
spi_wr8(hcd, MAX3421_REG_HCTL,
BIT(MAX3421_HCTL_BUSRST_BIT));
i_worked = 1;
}
-   if (max3421_hcd-do_check_unlink) {
-   max3421_hcd-do_check_unlink = 0;
+   if (test_and_clear_bit(CHECK_UNLINK, max3421_hcd-todo))
i_worked |= max3421_check_unlink(hcd);
-   }
-   if (max3421_hcd-do_iopin_update) {
+   if (test_and_clear_bit(IOPIN_UPDATE, max3421_hcd-todo)) {
/*
 * IOPINS1/IOPINS2 do not auto-increment, so we can't
 * use spi_wr_buf().
@@ -1469,7 +1465,6 @@ max3421_spi_thread(void *dev_id)
spi_wr8(hcd, MAX3421_REG_IOPINS1 + i, val);
max3421_hcd-iopins[i] = val;
}
-   max3421_hcd-do_iopin_update = 0;
i_worked = 1;
}
}
@@ -1485,7 +1480,7 @@ max3421_reset_port(struct usb_hcd *hcd)
 
max3421_hcd-port_status = ~(USB_PORT_STAT_ENABLE |
  USB_PORT_STAT_LOW_SPEED);
-   max3421_hcd-do_reset_port = 1;
+   set_bit(RESET_PORT, max3421_hcd-todo);
wake_up_process(max3421_hcd-spi_thread);
return 0;
 }
@@ -1498,7 +1493,7 @@ max3421_reset(struct usb_hcd *hcd)
hcd-self.sg_tablesize = 0;
hcd-speed = HCD_USB2;
hcd-self.root_hub-speed = USB_SPEED_FULL;
-   max3421_hcd-do_reset_hcd = 1;
+   set_bit(RESET_HCD, max3421_hcd-todo);
wake_up_process(max3421_hcd-spi_thread);
return 0;
 }
@@ -1590,7 +1585,7 @@ max3421_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, 
int status)
 */
retval = usb_hcd_check_unlink_urb(hcd, urb, status);
if (retval == 0) {
-   max3421_hcd-do_check_unlink = 1;
+   set_bit(CHECK_UNLINK, max3421_hcd-todo);
wake_up_process(max3421_hcd-spi_thread);
}
spin_unlock_irqrestore(max3421_hcd-lock, flags

Re: [PATCH 1/1] usb: host: max3421-hcd: Use module_spi_driver

[David Mosberger]

On Thu, May 29, 2014 at 5:51 AM, Sachin Kamat sachin.ka...@linaro.org wrote:
 module_spi_driver simplifies the code by eliminating
 boilerplate code.

Nice!

Acked-by: David Mosberger dav...@egauge.net


 Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
 ---
  drivers/usb/host/max3421-hcd.c |   15 +--
  1 file changed, 1 insertion(+), 14 deletions(-)

 diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
 index 28abda14c5e2..e8568c0f1df3 100644
 --- a/drivers/usb/host/max3421-hcd.c
 +++ b/drivers/usb/host/max3421-hcd.c
 @@ -1919,20 +1919,7 @@ static struct spi_driver max3421_driver = {
 },
  };

 -static int __init
 -max3421_mod_init(void)
 -{
 -   return spi_register_driver(max3421_driver);
 -}
 -
 -static void __exit
 -max3421_mod_exit(void)
 -{
 -   spi_unregister_driver(max3421_driver);
 -}
 -
 -module_init(max3421_mod_init);
 -module_exit(max3421_mod_exit);
 +module_spi_driver(max3421_driver);

  MODULE_DESCRIPTION(DRIVER_DESC);
  MODULE_AUTHOR(David Mosberger dav...@egauge.net);
 --
 1.7.9.5




-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: host: max3421-hcd: Allow platform-data to specify Vbus polarity

[David Mosberger]

From: David Mosberger-Tang dav...@egauge.net

Signed-off-by: Davidm Mosberger dav...@egauge.net
---
 drivers/usb/host/max3421-hcd.c| 6 --
 include/linux/platform_data/max3421-hcd.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index fa5ee8e..83c8355 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -1714,7 +1714,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, 
u16 value, u16 index,
break;
case USB_PORT_FEAT_POWER:
dev_dbg(hcd-self.controller, power-off\n);
-   max3421_gpout_set_value(hcd, pdata-vbus_gpout, 0);
+   max3421_gpout_set_value(hcd, pdata-vbus_gpout,
+   !pdata-vbus_active_level);
/* FALLS THROUGH */
default:
max3421_hcd-port_status = ~(1  value);
@@ -1763,7 +1764,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, 
u16 value, u16 index,
case USB_PORT_FEAT_POWER:
dev_dbg(hcd-self.controller, power-on\n);
max3421_hcd-port_status |= USB_PORT_STAT_POWER;
-   max3421_gpout_set_value(hcd, pdata-vbus_gpout, 1);
+   max3421_gpout_set_value(hcd, pdata-vbus_gpout,
+   pdata-vbus_active_level);
break;
case USB_PORT_FEAT_RESET:
max3421_reset_port(hcd);
diff --git a/include/linux/platform_data/max3421-hcd.h 
b/include/linux/platform_data/max3421-hcd.h
index 4ad4596..0303d19 100644
--- a/include/linux/platform_data/max3421-hcd.h
+++ b/include/linux/platform_data/max3421-hcd.h
@@ -18,6 +18,7 @@
  */
 struct max3421_hcd_platform_data {
u8 vbus_gpout;  /* pin controlling Vbus */
+   u8 vbus_active_level;   /* level that turns on power */
 };
 
 #endif /* MAX3421_HCD_PLAT_H_INCLUDED */
-- 
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: host: max3421-hcd: Fix potential NULL urb dereference

[David Mosberger]

From: David Mosberger-Tang dav...@egauge.net

Reported-by: Dan Carpenter dan.carpen...@oracle.com
Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/host/max3421-hcd.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index dfc74d6..28abda1 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -588,12 +588,14 @@ max3421_next_transfer(struct usb_hcd *hcd, int 
fast_retransmit)
 {
struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
struct urb *urb = max3421_hcd-curr_urb;
-   struct max3421_ep *max3421_ep = urb-ep-hcpriv;
+   struct max3421_ep *max3421_ep;
int cmd = -EINVAL;
 
if (!urb)
return; /* nothing to do */
 
+   max3421_ep = urb-ep-hcpriv;
+
switch (max3421_ep-pkt_state) {
case PKT_STATE_SETUP:
cmd = max3421_ctrl_setup(hcd, urb);
-- 
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

[PATCH] usb: host: max3421-hcd: Fix missing unlock in max3421_urb_enqueue()

[David Mosberger]

From: David Mosberger-Tang dav...@egauge.net

Reported-by: Dan Carpenter dan.carpen...@oracle.com
Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/host/max3421-hcd.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index 28abda1..714f99f 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -1545,8 +1545,10 @@ max3421_urb_enqueue(struct usb_hcd *hcd, struct urb 
*urb, gfp_t mem_flags)
if (!max3421_ep) {
/* gets freed in max3421_endpoint_disable: */
max3421_ep = kzalloc(sizeof(struct max3421_ep), mem_flags);
-   if (!max3421_ep)
-   return -ENOMEM;
+   if (!max3421_ep) {
+   retval = -ENOMEM;
+   goto out;
+   }
max3421_ep-ep = urb-ep;
max3421_ep-last_active = max3421_hcd-frame_number;
urb-ep-hcpriv = max3421_ep;
@@ -1561,6 +1563,7 @@ max3421_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, 
gfp_t mem_flags)
wake_up_process(max3421_hcd-spi_thread);
}
 
+out:
spin_unlock_irqrestore(max3421_hcd-lock, flags);
return retval;
 }
-- 
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: host: max3421-hcd: fix spi_rd8 uses dynamic stack allocation warning

[David Mosberger]

From: David Mosberger-Tang dav...@egauge.net

kmalloc the SPI rx and tx data buffers.  This appears to be the only
portable way to guarantee that the buffers are DMA-safe (e.g., in
separate DMA cache-lines).  This patch makes the spi_rdX()/spi_wrX()
non-reentrant, but that's OK because calls to them are guaranteed to
be serialized by the per-HCD SPI-thread.

Reported-by: kbuild test robot fengguang...@intel.com
Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/host/max3421-hcd.c |   94 +---
 1 file changed, 60 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index 28abda1..fa5ee8e 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -102,6 +102,10 @@ enum scheduling_pass {
SCHED_PASS_DONE
 };
 
+struct max3421_dma_buf {
+   u8 data[2];
+};
+
 struct max3421_hcd {
spinlock_t lock;
 
@@ -124,6 +128,12 @@ struct max3421_hcd {
u8 rev; /* chip revision */
u16 frame_number;
/*
+* kmalloc'd buffers guaranteed to be in separate (DMA)
+* cache-lines:
+*/
+   struct max3421_dma_buf *tx;
+   struct max3421_dma_buf *rx;
+   /*
 * URB we're currently processing.  Must not be reset to NULL
 * unless MAX3421E chip is idle:
 */
@@ -332,51 +342,47 @@ max3421_to_hcd(struct max3421_hcd *max3421_hcd)
 static u8
 spi_rd8(struct usb_hcd *hcd, unsigned int reg)
 {
+   struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
struct spi_device *spi = to_spi_device(hcd-self.controller);
struct spi_transfer transfer;
-   u8 tx_data[1];
-   /*
-* RX data must be in its own cache-line so it stays flushed
-* from the cache until the transfer is complete.  Otherwise,
-* we get stale data from the cache.
-*/
-   u8 rx_data[SMP_CACHE_BYTES] cacheline_aligned;
struct spi_message msg;
 
memset(transfer, 0, sizeof(transfer));
 
spi_message_init(msg);
 
-   tx_data[0] = (field(reg, MAX3421_SPI_REG_SHIFT) |
- field(MAX3421_SPI_DIR_RD, MAX3421_SPI_DIR_SHIFT));
+   max3421_hcd-tx-data[0] =
+   (field(reg, MAX3421_SPI_REG_SHIFT) |
+field(MAX3421_SPI_DIR_RD, MAX3421_SPI_DIR_SHIFT));
 
-   transfer.tx_buf = tx_data;
-   transfer.rx_buf = rx_data;
+   transfer.tx_buf = max3421_hcd-tx-data;
+   transfer.rx_buf = max3421_hcd-rx-data;
transfer.len = 2;
 
spi_message_add_tail(transfer, msg);
spi_sync(spi, msg);
 
-   return rx_data[1];
+   return max3421_hcd-rx-data[1];
 }
 
 static void
 spi_wr8(struct usb_hcd *hcd, unsigned int reg, u8 val)
 {
struct spi_device *spi = to_spi_device(hcd-self.controller);
+   struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
struct spi_transfer transfer;
struct spi_message msg;
-   u8 tx_data[2];
 
memset(transfer, 0, sizeof(transfer));
 
spi_message_init(msg);
 
-   tx_data[0] = (field(reg, MAX3421_SPI_REG_SHIFT) |
- field(MAX3421_SPI_DIR_WR, MAX3421_SPI_DIR_SHIFT));
-   tx_data[1] = val;
+   max3421_hcd-tx-data[0] =
+   (field(reg, MAX3421_SPI_REG_SHIFT) |
+field(MAX3421_SPI_DIR_WR, MAX3421_SPI_DIR_SHIFT));
+   max3421_hcd-tx-data[1] = val;
 
-   transfer.tx_buf = tx_data;
+   transfer.tx_buf = max3421_hcd-tx-data;
transfer.len = 2;
 
spi_message_add_tail(transfer, msg);
@@ -387,18 +393,18 @@ static void
 spi_rd_buf(struct usb_hcd *hcd, unsigned int reg, void *buf, size_t len)
 {
struct spi_device *spi = to_spi_device(hcd-self.controller);
+   struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
struct spi_transfer transfer[2];
struct spi_message msg;
-   u8 cmd;
 
memset(transfer, 0, sizeof(transfer));
 
spi_message_init(msg);
 
-   cmd = (field(reg, MAX3421_SPI_REG_SHIFT) |
-  field(MAX3421_SPI_DIR_RD, MAX3421_SPI_DIR_SHIFT));
-
-   transfer[0].tx_buf = cmd;
+   max3421_hcd-tx-data[0] =
+   (field(reg, MAX3421_SPI_REG_SHIFT) |
+field(MAX3421_SPI_DIR_RD, MAX3421_SPI_DIR_SHIFT));
+   transfer[0].tx_buf = max3421_hcd-tx-data;
transfer[0].len = 1;
 
transfer[1].rx_buf = buf;
@@ -413,18 +419,19 @@ static void
 spi_wr_buf(struct usb_hcd *hcd, unsigned int reg, void *buf, size_t len)
 {
struct spi_device *spi = to_spi_device(hcd-self.controller);
+   struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
struct spi_transfer transfer[2];
struct spi_message msg;
-   u8 cmd;
 
memset(transfer, 0, sizeof(transfer));
 
spi_message_init(msg);
 
-   cmd = (field(reg, MAX3421_SPI_REG_SHIFT) |
-  field(MAX3421_SPI_DIR_WR, MAX3421_SPI_DIR_SHIFT

[PATCH v5 0/1] Add support for using a MAX3421E chip as a host driver.

[David Mosberger]

This patch incorporates Greg KH's feedback for v4 of the patch and
fixes a bug.  Specifically:

+ Now compiles cleanly on 64-bit arches.
+ Now also depends on SPI subsystem.
+ Uses usb_urb_dir_in/usb_urb_dir_out instead of usb_pipein/usb_pipeout,
  respectively, since the latter do not always correctly identify the
  transfer-direction for control pipes, where the direction is determined
  based on the contents of the control-packet.

David Mosberger (1):
  Add support for using a MAX3421E chip as a host driver.

 drivers/usb/Makefile  |1 +
 drivers/usb/host/Kconfig  |   11 +
 drivers/usb/host/Makefile |1 +
 drivers/usb/host/max3421-hcd.c| 1937 +
 include/linux/platform_data/max3421-hcd.h |   23 +
 5 files changed, 1973 insertions(+)
 create mode 100644 drivers/usb/host/max3421-hcd.c
 create mode 100644 include/linux/platform_data/max3421-hcd.h

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

[PATCH v5 1/1] Add support for using a MAX3421E chip as a host driver.

[David Mosberger]

Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/Makefile  |1 +
 drivers/usb/host/Kconfig  |   11 +
 drivers/usb/host/Makefile |1 +
 drivers/usb/host/max3421-hcd.c| 1937 +
 include/linux/platform_data/max3421-hcd.h |   23 +
 5 files changed, 1973 insertions(+)
 create mode 100644 drivers/usb/host/max3421-hcd.c
 create mode 100644 include/linux/platform_data/max3421-hcd.h

diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 1ae2bf3..9bb6721 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_USB_IMX21_HCD)   += host/
 obj-$(CONFIG_USB_FSL_MPH_DR_OF)+= host/
 obj-$(CONFIG_USB_FUSBH200_HCD) += host/
 obj-$(CONFIG_USB_FOTG210_HCD)  += host/
+obj-$(CONFIG_USB_MAX3421_HCD)  += host/
 
 obj-$(CONFIG_USB_C67X00_HCD)   += c67x00/
 
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 3d9e540..81ad340 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -343,6 +343,17 @@ config USB_FOTG210_HCD
  To compile this driver as a module, choose M here: the
  module will be called fotg210-hcd.
 
+config USB_MAX3421_HCD
+   tristate MAX3421 HCD (USB-over-SPI) support
+   depends on USB  SPI
+   ---help---
+ The Maxim MAX3421E chip supports standard USB 2.0-compliant
+ full-speed devices either in host or peripheral mode.  This
+ driver supports the host-mode of the MAX3421E only.
+
+ To compile this driver as a module, choose M here: the module will
+ be called max3421-hcd.
+
 config USB_OHCI_HCD
tristate OHCI HCD (USB 1.1) support
select ISP1301_OMAP if MACH_OMAP_H2 || MACH_OMAP_H3
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 7530468..ea2bec5 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -70,3 +70,4 @@ obj-$(CONFIG_USB_HCD_BCMA)+= bcma-hcd.o
 obj-$(CONFIG_USB_HCD_SSB)  += ssb-hcd.o
 obj-$(CONFIG_USB_FUSBH200_HCD) += fusbh200-hcd.o
 obj-$(CONFIG_USB_FOTG210_HCD)  += fotg210-hcd.o
+obj-$(CONFIG_USB_MAX3421_HCD)  += max3421-hcd.o
diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
new file mode 100644
index 000..dfc74d6
--- /dev/null
+++ b/drivers/usb/host/max3421-hcd.c
@@ -0,0 +1,1937 @@
+/*
+ * MAX3421 Host Controller driver for USB.
+ *
+ * Author: David Mosberger-Tang dav...@egauge.net
+ *
+ * (C) Copyright 2014 David Mosberger-Tang dav...@egauge.net
+ *
+ * MAX3421 is a chip implementing a USB 2.0 Full-/Low-Speed host
+ * controller on a SPI bus.
+ *
+ * Based on:
+ * o MAX3421E datasheet
+ * http://datasheets.maximintegrated.com/en/ds/MAX3421E.pdf
+ * o MAX3421E Programming Guide
+ * http://www.hdl.co.jp/ftpdata/utl-001/AN3785.pdf
+ * o gadget/dummy_hcd.c
+ * For USB HCD implementation.
+ * o Arduino MAX3421 driver
+ *  https://github.com/felis/USB_Host_Shield_2.0/blob/master/Usb.cpp
+ *
+ * This file is licenced under the GPL v2.
+ *
+ * Important note on worst-case (full-speed) packet size constraints
+ * (See USB 2.0 Section 5.6.3 and following):
+ *
+ * - control:64 bytes
+ * - isochronous:  1023 bytes
+ * - interrupt:  64 bytes
+ * - bulk:   64 bytes
+ *
+ * Since the MAX3421 FIFO size is 64 bytes, we do not have to work about
+ * multi-FIFO writes/reads for a single USB packet *except* for isochronous
+ * transfers.  We don't support isochronous transfers at this time, so we
+ * just assume that a USB packet always fits into a single FIFO buffer.
+ *
+ * NOTE: The June 2006 version of MAX3421E Programming Guide
+ * (AN3785) has conflicting info for the RCVDAVIRQ bit:
+ *
+ * The description of RCVDAVIRQ says The CPU *must* clear
+ * this IRQ bit (by writing a 1 to it) before reading the
+ * RCVFIFO data.
+ *
+ * However, the earlier section on Programming BULK-IN
+ * Transfers says * that:
+ *
+ * After the CPU retrieves the data, it clears the
+ * RCVDAVIRQ bit.
+ *
+ * The December 2006 version has been corrected and it consistently
+ * states the second behavior is the correct one.
+ *
+ * Synchronous SPI transactions sleep so we can't perform any such
+ * transactions while holding a spin-lock (and/or while interrupts are
+ * masked).  To achieve this, all SPI transactions are issued from a
+ * single thread (max3421_spi_thread).
+ */
+
+#include linux/module.h
+#include linux/spi/spi.h
+#include linux/usb.h
+#include linux/usb/hcd.h
+
+#include linux/platform_data/max3421-hcd.h
+
+#define DRIVER_DESCMAX3421 USB Host-Controller Driver
+#define DRIVER_VERSION 1.0
+
+/* 11-bit counter that wraps around (USB 2.0 Section 8.3.3): */
+#define USB_MAX_FRAME_NUMBER   0x7ff
+#define USB_MAX_RETRIES3 /* # of retries before error is 
reported */
+
+/*
+ * Max. # of times we're willing to retransmit

Re: [PATCH v4] Add support for using a MAX3421E chip as a host driver.

[David Mosberger]

Greg,

Thanks for taking a look.  Yes, I'll definitely fix those oversights
and send a new patch next week.

Have a good weekend,

  --david

On Thu, Apr 24, 2014 at 2:09 PM, Greg KH gre...@linuxfoundation.org wrote:
 On Fri, Apr 11, 2014 at 11:55:51PM -0600, David Mosberger wrote:
 This is v4 of the patch.  Compared to v3, the only changes are:

   - addition of a platform-data header file which allows a platform
 to define which general-purpose output pin controls Vbus (if any)
   - spi_wr_fifo/spi_rd_fifo got renamed to spi_wr_buf/spi_rd_buf,
 respectively, since that more accurately reflects their function
 (whether or not a FIFO is being written depends on the register
  number).

 Signed-off-by: David Mosberger dav...@egauge.net
 ---
  drivers/usb/Makefile  |1 +
  drivers/usb/host/Kconfig  |   11 +
  drivers/usb/host/Makefile |1 +
  drivers/usb/host/max3421-hcd.c| 1937 
 +
  include/linux/platform_data/max3421-hcd.h |   23 +
  5 files changed, 1973 insertions(+)
  create mode 100644 drivers/usb/host/max3421-hcd.c
  create mode 100644 include/linux/platform_data/max3421-hcd.h

 I get a number of build warnings when building this code:

 drivers/usb/host/max3421-hcd.c: In function 'max3421_transfer_out':
 drivers/usb/host/max3421-hcd.c:570:4: warning: format '%zu' expects argument 
 of type 'size_t', but argument 5 has type 'int' [-Wformat=]
 __func__, max_packet, MAX3421_FIFO_SIZE);

 In file included from 
 /ssd/gregkh-linux/work/usb/arch/x86/include/asm/percpu.h:44:0,
  from 
 /ssd/gregkh-linux/work/usb/arch/x86/include/asm/preempt.h:5,
  from include/linux/preempt.h:18,
  from include/linux/spinlock.h:50,
  from include/linux/seqlock.h:35,
  from include/linux/time.h:5,
  from include/linux/stat.h:18,
  from include/linux/module.h:10,
  from drivers/usb/host/max3421-hcd.c:58:
 include/linux/kernel.h:713:17: warning: comparison of distinct pointer types 
 lacks a cast [enabled by default]
   (void) (_min1 == _min2);  \
  ^
 drivers/usb/host/max3421-hcd.c:574:26: note: in expansion of macro 'min'
   max3421_hcd-curr_len = min((urb-transfer_buffer_length -
   ^
 drivers/usb/host/max3421-hcd.c: In function 'max3421_transfer_in_done':
 drivers/usb/host/max3421-hcd.c:977:4: warning: format '%zu' expects argument 
 of type 'size_t', but argument 5 has type 'int' [-Wformat=]
 __func__, max_packet, MAX3421_FIFO_SIZE);
 ^

 which I could live with and accept a follow-on patch, but then it breaks the 
 build:

 ERROR: spi_register_driver [drivers/usb/host/max3421-hcd.ko] undefined!
 ERROR: spi_sync [drivers/usb/host/max3421-hcd.ko] undefined!
 ERROR: spi_setup [drivers/usb/host/max3421-hcd.ko] undefined!


 It looks like the Kconfig dependancies are not correct.

 Care to fix that up, and resend this patch?  Also, the changelog text above
 needs some work, no one cares what changed from the previous submission once
 the code is in the tree, they just want to know what the code does, put the
 this changed stuff below the cut line.

 thanks,

 greg k-h



-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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 v4] Add support for using a MAX3421E chip as a host driver.

[David Mosberger]

This is v4 of the patch.  Compared to v3, the only changes are:

- addition of a platform-data header file which allows a platform
  to define which general-purpose output pin controls Vbus (if any)
- spi_wr_fifo/spi_rd_fifo got renamed to spi_wr_buf/spi_rd_buf,
  respectively, since that more accurately reflects their function
  (whether or not a FIFO is being written depends on the register
   number).

Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/Makefile  |1 +
 drivers/usb/host/Kconfig  |   11 +
 drivers/usb/host/Makefile |1 +
 drivers/usb/host/max3421-hcd.c| 1937 +
 include/linux/platform_data/max3421-hcd.h |   23 +
 5 files changed, 1973 insertions(+)
 create mode 100644 drivers/usb/host/max3421-hcd.c
 create mode 100644 include/linux/platform_data/max3421-hcd.h

diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 1ae2bf3..9bb6721 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_USB_IMX21_HCD)   += host/
 obj-$(CONFIG_USB_FSL_MPH_DR_OF)+= host/
 obj-$(CONFIG_USB_FUSBH200_HCD) += host/
 obj-$(CONFIG_USB_FOTG210_HCD)  += host/
+obj-$(CONFIG_USB_MAX3421_HCD)  += host/
 
 obj-$(CONFIG_USB_C67X00_HCD)   += c67x00/
 
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 3d9e540..e9cd7d8 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -343,6 +343,17 @@ config USB_FOTG210_HCD
  To compile this driver as a module, choose M here: the
  module will be called fotg210-hcd.
 
+config USB_MAX3421_HCD
+   tristate MAX3421 HCD (USB-over-SPI) support
+   depends on USB
+   ---help---
+ The Maxim MAX3421E chip supports standard USB 2.0-compliant
+ full-speed devices either in host or peripheral mode.  This
+ driver supports the host-mode of the MAX3421E only.
+
+ To compile this driver as a module, choose M here: the module will
+ be called max3421-hcd.
+
 config USB_OHCI_HCD
tristate OHCI HCD (USB 1.1) support
select ISP1301_OMAP if MACH_OMAP_H2 || MACH_OMAP_H3
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 7530468..ea2bec5 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -70,3 +70,4 @@ obj-$(CONFIG_USB_HCD_BCMA)+= bcma-hcd.o
 obj-$(CONFIG_USB_HCD_SSB)  += ssb-hcd.o
 obj-$(CONFIG_USB_FUSBH200_HCD) += fusbh200-hcd.o
 obj-$(CONFIG_USB_FOTG210_HCD)  += fotg210-hcd.o
+obj-$(CONFIG_USB_MAX3421_HCD)  += max3421-hcd.o
diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
new file mode 100644
index 000..fe931ea
--- /dev/null
+++ b/drivers/usb/host/max3421-hcd.c
@@ -0,0 +1,1937 @@
+/*
+ * MAX3421 Host Controller driver for USB.
+ *
+ * Author: David Mosberger-Tang dav...@egauge.net
+ *
+ * (C) Copyright 2014 David Mosberger-Tang dav...@egauge.net
+ *
+ * MAX3421 is a chip implementing a USB 2.0 Full-/Low-Speed host
+ * controller on a SPI bus.
+ *
+ * Based on:
+ * o MAX3421E datasheet
+ * http://datasheets.maximintegrated.com/en/ds/MAX3421E.pdf
+ * o MAX3421E Programming Guide
+ * http://www.hdl.co.jp/ftpdata/utl-001/AN3785.pdf
+ * o gadget/dummy_hcd.c
+ * For USB HCD implementation.
+ * o Arduino MAX3421 driver
+ *  https://github.com/felis/USB_Host_Shield_2.0/blob/master/Usb.cpp
+ *
+ * This file is licenced under the GPL v2.
+ *
+ * Important note on worst-case (full-speed) packet size constraints
+ * (See USB 2.0 Section 5.6.3 and following):
+ *
+ * - control:64 bytes
+ * - isochronous:  1023 bytes
+ * - interrupt:  64 bytes
+ * - bulk:   64 bytes
+ *
+ * Since the MAX3421 FIFO size is 64 bytes, we do not have to work about
+ * multi-FIFO writes/reads for a single USB packet *except* for isochronous
+ * transfers.  We don't support isochronous transfers at this time, so we
+ * just assume that a USB packet always fits into a single FIFO buffer.
+ *
+ * NOTE: The June 2006 version of MAX3421E Programming Guide
+ * (AN3785) has conflicting info for the RCVDAVIRQ bit:
+ *
+ * The description of RCVDAVIRQ says The CPU *must* clear
+ * this IRQ bit (by writing a 1 to it) before reading the
+ * RCVFIFO data.
+ *
+ * However, the earlier section on Programming BULK-IN
+ * Transfers says * that:
+ *
+ * After the CPU retrieves the data, it clears the
+ * RCVDAVIRQ bit.
+ *
+ * The December 2006 version has been corrected and it consistently
+ * states the second behavior is the correct one.
+ *
+ * Synchronous SPI transactions sleep so we can't perform any such
+ * transactions while holding a spin-lock (and/or while interrupts are
+ * masked).  To achieve this, all SPI transactions are issued from a
+ * single thread (max3421_spi_thread).
+ */
+
+#include linux/module.h
+#include

[PATCH v3] usb: host: Add HCD for MAX3421E chip.

[David Mosberger]

Final version of the driver.  This includes support for low-speed devices
connected via full-speed hubs.  I believe this patch incorporates all the
concrete suggestions that have been made on the mailing list.

Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/Makefile   |1 +
 drivers/usb/host/Kconfig   |   11 +
 drivers/usb/host/Makefile  |1 +
 drivers/usb/host/max3421-hcd.c | 1886 
 4 files changed, 1899 insertions(+)
 create mode 100644 drivers/usb/host/max3421-hcd.c

diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 1ae2bf3..9bb6721 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_USB_IMX21_HCD)   += host/
 obj-$(CONFIG_USB_FSL_MPH_DR_OF)+= host/
 obj-$(CONFIG_USB_FUSBH200_HCD) += host/
 obj-$(CONFIG_USB_FOTG210_HCD)  += host/
+obj-$(CONFIG_USB_MAX3421_HCD)  += host/
 
 obj-$(CONFIG_USB_C67X00_HCD)   += c67x00/
 
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 3d9e540..e9cd7d8 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -343,6 +343,17 @@ config USB_FOTG210_HCD
  To compile this driver as a module, choose M here: the
  module will be called fotg210-hcd.
 
+config USB_MAX3421_HCD
+   tristate MAX3421 HCD (USB-over-SPI) support
+   depends on USB
+   ---help---
+ The Maxim MAX3421E chip supports standard USB 2.0-compliant
+ full-speed devices either in host or peripheral mode.  This
+ driver supports the host-mode of the MAX3421E only.
+
+ To compile this driver as a module, choose M here: the module will
+ be called max3421-hcd.
+
 config USB_OHCI_HCD
tristate OHCI HCD (USB 1.1) support
select ISP1301_OMAP if MACH_OMAP_H2 || MACH_OMAP_H3
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 7530468..ea2bec5 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -70,3 +70,4 @@ obj-$(CONFIG_USB_HCD_BCMA)+= bcma-hcd.o
 obj-$(CONFIG_USB_HCD_SSB)  += ssb-hcd.o
 obj-$(CONFIG_USB_FUSBH200_HCD) += fusbh200-hcd.o
 obj-$(CONFIG_USB_FOTG210_HCD)  += fotg210-hcd.o
+obj-$(CONFIG_USB_MAX3421_HCD)  += max3421-hcd.o
diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
new file mode 100644
index 000..d1263d0
--- /dev/null
+++ b/drivers/usb/host/max3421-hcd.c
@@ -0,0 +1,1886 @@
+/*
+ * MAX3421 Host Controller driver for USB.
+ *
+ * Author: David Mosberger-Tang dav...@egauge.net
+ *
+ * (C) Copyright 2014 David Mosberger-Tang dav...@egauge.net
+ *
+ * MAX3421 is a chip implementing a USB 2.0 Full-/Low-Speed host
+ * controller on a SPI bus.
+ *
+ * Based on:
+ * o MAX3421E datasheet
+ * http://datasheets.maximintegrated.com/en/ds/MAX3421E.pdf
+ * o MAX3421E Programming Guide
+ * http://www.hdl.co.jp/ftpdata/utl-001/AN3785.pdf
+ * o gadget/dummy_hcd.c
+ * For USB HCD implementation.
+ * o Arduino MAX3421 driver
+ *  https://github.com/felis/USB_Host_Shield_2.0/blob/master/Usb.cpp
+ *
+ * This file is licenced under the GPL v2.
+ *
+ * Important note on worst-case (full-speed) packet size constraints
+ * (See USB 2.0 Section 5.6.3 and following):
+ *
+ * - control:64 bytes
+ * - isochronous:  1023 bytes
+ * - interrupt:  64 bytes
+ * - bulk:   64 bytes
+ *
+ * Since the MAX3421 FIFO size is 64 bytes, we do not have to work about
+ * multi-FIFO writes/reads for a single USB packet *except* for isochronous
+ * transfers.  We don't support isochronous transfers at this time, so we
+ * just assume that a USB packet always fits into a single FIFO buffer.
+ *
+ * NOTE: The June 2006 version of MAX3421E Programming Guide
+ * (AN3785) has conflicting info for the RCVDAVIRQ bit:
+ *
+ * The description of RCVDAVIRQ says The CPU *must* clear
+ * this IRQ bit (by writing a 1 to it) before reading the
+ * RCVFIFO data.
+ *
+ * However, the earlier section on Programming BULK-IN
+ * Transfers says * that:
+ *
+ * After the CPU retrieves the data, it clears the
+ * RCVDAVIRQ bit.
+ *
+ * The December 2006 version has been corrected and it consistently
+ * states the second behavior is the correct one.
+ *
+ * Synchronous SPI transactions sleep so we can't perform any such
+ * transactions while holding a spin-lock (and/or while interrupts are
+ * masked).  To achieve this, all SPI transactions are issued from a
+ * single thread (max3421_spi_thread).
+ */
+
+#include linux/module.h
+#include linux/spi/spi.h
+#include linux/usb.h
+#include linux/usb/hcd.h
+
+#define DRIVER_DESCMAX3421 USB Host-Controller Driver
+#define DRIVER_VERSION 1.0
+
+/* 11-bit counter that wraps around (USB 2.0 Section 8.3.3): */
+#define USB_MAX_FRAME_NUMBER   0x7ff
+#define USB_MAX_RETRIES3 /* # of retries before error is 
reported */
+
+/*
+ * Max. # of times we're willing

[RFC] mtd: nand: Preparatory patch for adding on-die ECC controller support. This patch adds NAND_ECC_HW_ON_DIE and all other changes to generic code.

[David Mosberger]

Pekon,

Before I go any further with this, could you confirm that what is
below is what you had in mind as far as the generic portion of the
patch is concerned.  If so, I'll go ahead and create the second,
Micron-specific part next.

Thanks,

  --david

PS: This patch adds a one-liner to of_mtd.c that I forgot about previously.

Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/mtd/nand/nand_base.c |   36 +---
 drivers/of/of_mtd.c  |1 +
 include/linux/mtd/nand.h |7 +++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 5826da3..b94e2e9 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3783,22 +3783,46 @@ EXPORT_SYMBOL(nand_scan_ident);
 int nand_scan_tail(struct mtd_info *mtd)
 {
int i;
+   u8 features[ONFI_SUBFEATURE_PARAM_LEN];
struct nand_chip *chip = mtd-priv;
struct nand_ecc_ctrl *ecc = chip-ecc;
struct nand_buffers *nbuf;
 
+   if (chip-onfi_get_features(mtd, chip, ONFI_FEATURE_ADDR_OP_MODE,
+   features) = 0) {
+   if (features[0]  ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC) {
+   /*
+* If the chip has on-die ECC enabled, we kind
+* of have to do the same...
+*/
+   chip-ecc.mode = NAND_ECC_HW_ON_DIE;
+   pr_info(Using on-die ECC\n);
+   }
+   }
+
/* New bad blocks should be marked in OOB, flash-based BBT, or both */
BUG_ON((chip-bbt_options  NAND_BBT_NO_OOB_BBM) 
!(chip-bbt_options  NAND_BBT_USE_FLASH));
 
if (!(chip-options  NAND_OWN_BUFFERS)) {
+   size_t on_die_bufsz = 0;
+
+   if (chip-ecc.mode == NAND_ECC_HW_ON_DIE)
+   on_die_bufsz = 2*(mtd-writesize + mtd-oobsize);
+
nbuf = kzalloc(sizeof(*nbuf) + mtd-writesize
-   + mtd-oobsize * 3, GFP_KERNEL);
+   + mtd-oobsize * 3 + on_die_bufsz, GFP_KERNEL);
if (!nbuf)
return -ENOMEM;
nbuf-ecccalc = (uint8_t *)(nbuf + 1);
nbuf-ecccode = nbuf-ecccalc + mtd-oobsize;
nbuf-databuf = nbuf-ecccode + mtd-oobsize;
+   if (chip-ecc.mode == NAND_ECC_HW_ON_DIE) {
+   nbuf-chkbuf = (nbuf-databuf + mtd-writesize
+   + mtd-oobsize);
+   nbuf-rawbuf = (nbuf-chkbuf + mtd-writesize
+   + mtd-oobsize);
+   }
 
chip-buffers = nbuf;
} else {
@@ -3956,6 +3980,7 @@ int nand_scan_tail(struct mtd_info *mtd)
ecc-strength = ecc-bytes * 8 / fls(8 * ecc-size);
break;
 
+   case NAND_ECC_HW_ON_DIE:
case NAND_ECC_NONE:
pr_warn(NAND_ECC_NONE selected by board driver. 
   This is not recommended!\n);
@@ -4023,8 +4048,13 @@ int nand_scan_tail(struct mtd_info *mtd)
/* Invalidate the pagebuffer reference */
chip-pagebuf = -1;
 
-   /* Large page NAND with SOFT_ECC should support subpage reads */
-   if ((ecc-mode == NAND_ECC_SOFT)  (chip-page_shift  9))
+   /*
+* Large page NAND with SOFT_ECC or on-die ECC should support
+* subpage reads.
+*/
+   if (((ecc-mode == NAND_ECC_SOFT)
+|| (chip-ecc.mode == NAND_ECC_HW_ON_DIE))
+(chip-page_shift  9))
chip-options |= NAND_SUBPAGE_READ;
 
/* Fill in remaining MTD driver data */
diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c
index b7361ed..c844c84 100644
--- a/drivers/of/of_mtd.c
+++ b/drivers/of/of_mtd.c
@@ -23,6 +23,7 @@ static const char *nand_ecc_modes[] = {
[NAND_ECC_HW_SYNDROME]  = hw_syndrome,
[NAND_ECC_HW_OOB_FIRST] = hw_oob_first,
[NAND_ECC_SOFT_BCH] = soft_bch,
+   [NAND_ECC_HW_ON_DIE]= hw_on_die,
 };
 
 /**
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 450d61e..a1cc980 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -115,6 +115,7 @@ typedef enum {
NAND_ECC_HW_SYNDROME,
NAND_ECC_HW_OOB_FIRST,
NAND_ECC_SOFT_BCH,
+   NAND_ECC_HW_ON_DIE,
 } nand_ecc_modes_t;
 
 /*
@@ -214,6 +215,10 @@ struct nand_chip;
 /* Vendor-specific feature address (Micron) */
 #define ONFI_FEATURE_ADDR_READ_RETRY   0x89
 
+/* Vendor-specific array operation mode (Micron) */
+#define ONFI_FEATURE_ADDR_OP_MODE  0x90
+#define ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC 0x08
+
 /* ONFI subfeature parameters length */
 #define ONFI_SUBFEATURE_PARAM_LEN  4
 
@@ -516,6 +521,8 @@ struct nand_buffers {
uint8_t *ecccalc;
uint8_t *ecccode

Re: [PATCH] usb: host: Add MAX3421E HCD support.

[David Mosberger]

Felipe,

On Mon, Mar 24, 2014 at 7:31 AM, Felipe Balbi ba...@ti.com wrote:

 Why do you need to run your IRQ handler when an URQ gets enqueued ? That
 doesn't make much sense :-s

Please see the comments at the start of the file: spi_sync() is
blocking and must not be called while holding spinlocks or with
interrupts disabled.  The purpose of max3421_spi_thread() is to handle
all SPI transactions (implicitly serializing them).  An interrupt is
just one of the reasons to wake up the SPI thread.  Adding URBs,
removing them, or performing a bus reset are other reasons.

You can think of max3421_spi_thread() as implementing the state
machine that would normally be implemented in silicon with a smart
controller such as OHCI or similar.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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] usb: host: Add MAX3421E HCD support.

[David Mosberger]

On Mon, Mar 24, 2014 at 1:35 PM, Felipe Balbi ba...@ti.com wrote:

 looking at the driver, there's still quite a bit of duplication between
 his kthread implementation and what threaded IRQs would give for free...

Well, let's count: hard irq handler:

static irqreturn_t
max3421_irq_handler(int irq, void *dev_id)
{
struct usb_hcd *hcd = dev_id;
struct spi_device *spi = to_spi_device(hcd-self.controller);
struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);

if (max3421_hcd-spi_thread 
max3421_hcd-spi_thread-state != TASK_RUNNING)
wake_up_process(max3421_hcd-spi_thread);
if (!max3421_hcd-do_enable_irq) {
max3421_hcd-do_enable_irq = 1;
disable_irq_nosync(spi-irq);
}
return IRQ_HANDLED;
}

irq-specific part of spi_thread:

   set_current_state(TASK_INTERRUPTIBLE);
   if (max3421_hcd-do_enable_irq) {
max3421_hcd-do_enable_irq = 0;
enable_irq(spi-irq);
   }
   schedule();
   __set_current_state(TASK_RUNNING);

so we're talking about ~22 lines of code.  As I told you before, I did
start out using a threaded irq handler but it had the distinct
disadvantage of not working.

If you have better solution in mind that actually works, please show
us the code.  As the old saying goes: put up or shut up.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: host: Add MAX3421E HCD support.

[David Mosberger]

Sorry, I keep forgetting how bad gmail is in handling patches...

Here is a retransmit in plain-text format (really).

The patch is relative to linux-next.

The Maxim MAX3421E is a USB-over-SPI chip that can operate either in
peripheral or host mode.  This driver adds support for the host-mode
only.  The chip is USB 2.0 compliant with support for low- and
full-speed devices.
---
 drivers/usb/Makefile   |1 +
 drivers/usb/host/Kconfig   |   10 +
 drivers/usb/host/Makefile  |2 +
 drivers/usb/host/max3421.c | 1990 
 4 files changed, 2003 insertions(+)
 create mode 100644 drivers/usb/host/max3421.c

diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 1ae2bf3..c83c406 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_USB_DWC2)+= dwc2/
 obj-$(CONFIG_USB_MON)  += mon/
 
 obj-$(CONFIG_PCI)  += host/
+obj-$(CONFIG_USB_MAX3421_HCD)  += host/
 obj-$(CONFIG_USB_EHCI_HCD) += host/
 obj-$(CONFIG_USB_ISP116X_HCD)  += host/
 obj-$(CONFIG_USB_OHCI_HCD) += host/
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 3d9e540..98fe7f3 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -3,6 +3,16 @@
 #
 comment USB Host Controller Drivers
 
+config USB_MAX3421_HCD
+   tristate MAX3421 HCD (USB-over-SPI) support
+   depends on USB
+   help
+ The Maxim MAX3421E Host Controller Interface supports
+standard USB 2.0-compliant full-speed devices.
+
+ To compile this driver as a module, choose M here: the module will
+be called max3421.
+
 config USB_C67X00_HCD
tristate Cypress C67x00 HCD support
help
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 7530468..afcef27 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -25,6 +25,8 @@ obj-$(CONFIG_USB_WHCI_HCD)+= whci/
 
 obj-$(CONFIG_PCI)  += pci-quirks.o
 
+obj-$(CONFIG_USB_MAX3421_HCD)  += max3421.o
+
 obj-$(CONFIG_USB_EHCI_HCD) += ehci-hcd.o
 obj-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o
 obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)+= ehci-platform.o
diff --git a/drivers/usb/host/max3421.c b/drivers/usb/host/max3421.c
new file mode 100644
index 000..46e2b10
--- /dev/null
+++ b/drivers/usb/host/max3421.c
@@ -0,0 +1,1990 @@
+/*
+ * MAX3421 Host Controller driver for USB.
+ *
+ * Maintainer: David Mosberger-Tang dav...@egauge.net
+ *
+ * (C) Copyright 2014 David Mosberger-Tang dav...@egauge.net
+ *
+ * MAX3421 is a chip implementing a USB 2.0 Full-/Low-Speed host
+ * controller on a SPI bus.
+ *
+ * Based on:
+ * o MAX3421E datasheet
+ * http://datasheets.maximintegrated.com/en/ds/MAX3421E.pdf
+ * o MAX3421E Programming Guide
+ * http://www.hdl.co.jp/ftpdata/utl-001/AN3785.pdf
+ * o gadget/dummy_hcd.c
+ * For USB HCD implementation.
+ * o Arduino MAX3421 driver
+ *  https://github.com/felis/USB_Host_Shield_2.0/blob/master/Usb.cpp
+ *
+ * This file is licenced under the GPL.
+ *
+ * Important note on worst-case (full-speed) packet size constraints
+ * (See USB 2.0 Section 5.6.3 and following):
+ *
+ * - control:64 bytes
+ * - isochronous:  1023 bytes
+ * - interrupt:  64 bytes
+ * - bulk:   64 bytes
+ *
+ * Since the MAX3421 FIFO size is 64 bytes, we do not have to work about
+ * multi-FIFO writes/reads for a single USB packet *except* for isochronous
+ * transfers.  We don't support isochronous transfers at this time, so we
+ * just assume that a USB packet always fits into a single FIFO buffer.
+ *
+ * NOTE: The June 2006 version of MAX3421E Programming Guide
+ * (AN3785) has conflicting info for the RCVDAVIRQ bit:
+ *
+ * The description of RCVDAVIRQ says The CPU *must* clear
+ * this IRQ bit (by writing a 1 to it) before reading the
+ * RCVFIFO data.
+ *
+ * However, the earlier section on Programming BULK-IN
+ * Transfers says * that:
+ *
+ * After the CPU retrieves the data, it clears the
+ * RCVDAVIRQ bit.
+ *
+ * The December 2006 version has been corrected and it consistently
+ * states the second behavior is the correct one.
+ *
+ * Synchronous SPI transactions sleep so we can't perform any such
+ * transactions while holding a spin-lock (and/or while interrupts are
+ * masked).  To achieve this, all SPI transactions are issued from a
+ * single thread (max3421_spi_thread).
+ */
+
+#define TRACE 0
+
+#include linux/module.h
+#include linux/spi/spi.h
+#include linux/usb.h
+#include linux/usb/hcd.h
+
+#define DRIVER_DESCMAX3421 USB Host-Controller Driver
+#define DRIVER_VERSION 0.4
+
+/* 11-bit counter that wraps around (USB 2.0 Section 8.3.3): */
+#define USB_MAX_FRAME_NUMBER   0x7ff
+#define USB_MAX_RETRIES3 /* # of retries before error is 
reported */
+
+/*
+ * Max. # of times we're willing to retransmit a request immediately

Re: [PATCH] usb: host: Add MAX3421E HCD support.

[David Mosberger]

Felipe,

Thanks for your feedback, I'll take that into consideration.

On Fri, Mar 21, 2014 at 9:44 PM, Felipe Balbi ba...@ti.com wrote:

 + max3421_hcd-spi_thread = kthread_run(max3421_spi_thread, hcd,
 +   max3421_spi_thread);

 why do you need this kthread ? For IRQ handling ? use threaded IRQs
 instead.

Is there a clean way to wakeup a threaded IRQ handler without an
interrupt?  Unless I'm missing something, there is not.  I started out
with a threaded handler in fact, but there are other reasons (e.g.,
URB enqueue) which need to wake up the thread.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: host: Add HCD for MAX3421E chip. (rev2)

[David Mosberger]

This revision incorporates virtually all of Felipe's feedback.

David Mosberger (1):
  usb: host: Add HCD for MAX3421E chip.

 drivers/usb/Makefile   |1 +
 drivers/usb/host/Kconfig   |   11 +
 drivers/usb/host/Makefile  |1 +
 drivers/usb/host/max3421-hcd.c | 1857 
 4 files changed, 1870 insertions(+)
 create mode 100644 drivers/usb/host/max3421-hcd.c

-- 
1.7.9.5

From b567d881406007457ab30c310a705e66a87c9967 Mon Sep 17 00:00:00 2001
From: David Mosberger dav...@egauge.net
Date: Fri, 21 Mar 2014 22:39:15 -0600
Subject: [PATCH] usb: host: Add HCD for MAX3421E chip.

Signed-off-by: David Mosberger dav...@egauge.net
---
 drivers/usb/Makefile   |1 +
 drivers/usb/host/Kconfig   |   11 +
 drivers/usb/host/Makefile  |1 +
 drivers/usb/host/max3421-hcd.c | 1857 
 4 files changed, 1870 insertions(+)
 create mode 100644 drivers/usb/host/max3421-hcd.c

diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 1ae2bf3..9bb6721 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_USB_IMX21_HCD)   += host/
 obj-$(CONFIG_USB_FSL_MPH_DR_OF)+= host/
 obj-$(CONFIG_USB_FUSBH200_HCD) += host/
 obj-$(CONFIG_USB_FOTG210_HCD)  += host/
+obj-$(CONFIG_USB_MAX3421_HCD)  += host/
 
 obj-$(CONFIG_USB_C67X00_HCD)   += c67x00/
 
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 3d9e540..e9cd7d8 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -343,6 +343,17 @@ config USB_FOTG210_HCD
  To compile this driver as a module, choose M here: the
  module will be called fotg210-hcd.
 
+config USB_MAX3421_HCD
+   tristate MAX3421 HCD (USB-over-SPI) support
+   depends on USB
+   ---help---
+ The Maxim MAX3421E chip supports standard USB 2.0-compliant
+ full-speed devices either in host or peripheral mode.  This
+ driver supports the host-mode of the MAX3421E only.
+
+ To compile this driver as a module, choose M here: the module will
+ be called max3421-hcd.
+
 config USB_OHCI_HCD
tristate OHCI HCD (USB 1.1) support
select ISP1301_OMAP if MACH_OMAP_H2 || MACH_OMAP_H3
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 7530468..ea2bec5 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -70,3 +70,4 @@ obj-$(CONFIG_USB_HCD_BCMA)+= bcma-hcd.o
 obj-$(CONFIG_USB_HCD_SSB)  += ssb-hcd.o
 obj-$(CONFIG_USB_FUSBH200_HCD) += fusbh200-hcd.o
 obj-$(CONFIG_USB_FOTG210_HCD)  += fotg210-hcd.o
+obj-$(CONFIG_USB_MAX3421_HCD)  += max3421-hcd.o
diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
new file mode 100644
index 000..d63a042
--- /dev/null
+++ b/drivers/usb/host/max3421-hcd.c
@@ -0,0 +1,1857 @@
+/*
+ * MAX3421 Host Controller driver for USB.
+ *
+ * Author: David Mosberger-Tang dav...@egauge.net
+ *
+ * (C) Copyright 2014 David Mosberger-Tang dav...@egauge.net
+ *
+ * MAX3421 is a chip implementing a USB 2.0 Full-/Low-Speed host
+ * controller on a SPI bus.
+ *
+ * Based on:
+ * o MAX3421E datasheet
+ * http://datasheets.maximintegrated.com/en/ds/MAX3421E.pdf
+ * o MAX3421E Programming Guide
+ * http://www.hdl.co.jp/ftpdata/utl-001/AN3785.pdf
+ * o gadget/dummy_hcd.c
+ * For USB HCD implementation.
+ * o Arduino MAX3421 driver
+ *  https://github.com/felis/USB_Host_Shield_2.0/blob/master/Usb.cpp
+ *
+ * This file is licenced under the GPL v2.
+ *
+ * Important note on worst-case (full-speed) packet size constraints
+ * (See USB 2.0 Section 5.6.3 and following):
+ *
+ * - control:64 bytes
+ * - isochronous:  1023 bytes
+ * - interrupt:  64 bytes
+ * - bulk:   64 bytes
+ *
+ * Since the MAX3421 FIFO size is 64 bytes, we do not have to work about
+ * multi-FIFO writes/reads for a single USB packet *except* for isochronous
+ * transfers.  We don't support isochronous transfers at this time, so we
+ * just assume that a USB packet always fits into a single FIFO buffer.
+ *
+ * NOTE: The June 2006 version of MAX3421E Programming Guide
+ * (AN3785) has conflicting info for the RCVDAVIRQ bit:
+ *
+ * The description of RCVDAVIRQ says The CPU *must* clear
+ * this IRQ bit (by writing a 1 to it) before reading the
+ * RCVFIFO data.
+ *
+ * However, the earlier section on Programming BULK-IN
+ * Transfers says * that:
+ *
+ * After the CPU retrieves the data, it clears the
+ * RCVDAVIRQ bit.
+ *
+ * The December 2006 version has been corrected and it consistently
+ * states the second behavior is the correct one.
+ *
+ * Synchronous SPI transactions sleep so we can't perform any such
+ * transactions while holding a spin-lock (and/or while interrupts are
+ * masked).  To achieve this, all SPI transactions are issued from a
+ * single thread (max3421_spi_thread

Re: MAX3421E: device giving NAKs forever?

[David Mosberger]

Alan,

On Fri, Mar 14, 2014 at 8:02 AM, Alan Stern st...@rowland.harvard.edu wrote:
 On Thu, 13 Mar 2014, David Mosberger wrote:

 To answer my own question: it appears that USB peripherals return NAKs
 not only when the peripheral is not ready to accept the data, but also
 when the peripheral doesn't know what to do with the data.

 No, that's not right.  If the device doesn't know what to do with the
 data (for example, if it doesn't recognize a particular SETUP request),
 it replies with a STALL.

Please re-read my statement again.  I said that infinite NAKs is one
way for a peripheral to say, I don't know what to do with this.  I
didn't say it's the only way to handle errors.  I certainly would have
much preferred to get a STALL for my situation because it'd have made
it very clear what the offending command was.

 Probably what happens is that the device doesn't queue an internal
 request for an OUT transfer until it knows that it needs some data.
 Without a request queued, the device's USB controller doesn't have
 any place to store the data sent by the host computer, so it NAKs the
 transfer.

 It really is saying I'm not ready to accept this data.

Sure, that sounds very plausible.

I didn't say the device was doing anything wrong.  I just have seen
several other questions from developers about infinite NAKs but never
an answer as to what would cause that, so I wanted to explain it for
the benefit of others.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

Alan,

On Fri, Mar 14, 2014 at 9:34 AM, Alan Stern st...@rowland.harvard.edu wrote:

 Now, I'm not saying what the device did was correct.  According to
 section 6.6.1 of the Bulk-Only Transport Mass Storage spec, when the
 device expects a CBW packet but gets something else, it is supposed to
 accept the packet (ACK, not NAK), stall the bulk-IN endpoint, and
 either stall or accept and discard all further bulk-OUT data.

Ah, now that's interesting.  I tried like half a dozen mass storage
devices, not one of them STALLed.
It'd have been so much easier to find this problem if I had gotten a
STALL for the corrupted OUT.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

After thinking about this some more, the MAX3421E behavior could be
triggered if a write to the SNDFIFO is not followed by a BULK_OUT
command to the HXFR register.  My driver always issues BULK_OUT after
writing the SNDFIFO so this should never happen, but a corrupted SPI
transfer could do this.

Also, and perhaps more plausible for my driver, after an OUT transfer
gets a response other than ACK (e.g., NAK or error), the MAX3421E
doesn't unload that FIFO (assuming that you'll want to retransmit the
data).  My driver never retransmits the data immediately, so I think
it has to issue a dummy write to the SNDBC register to switch back to
the original FIFO.  I know I tried that at one point and it didn't fix
the issue, but I should try this again as it seems the most plausible
explanation.

  --david

On Thu, Mar 13, 2014 at 10:46 AM, David Mosberger dav...@egauge.net wrote:
 OK, I finally know where the problem is coming from!  The MAX3421E
 chip uses double-buffering.  Specifically, it has two 64-byte send
 FIFOs.  You write up to 64 bytes to a send FIFO by repeatedly writing
 to SPI register 2 (SNDFIFO).  Then you tell the chip how many bytes
 you just put in the FIFO by writing SPI register 7 (the
 send-byte-count or SNDBC register).  Writing SNDBC is supposed to
 switch the FIFO to the USB-side so it can be transmitted on the USB
 bus.

 Unfortunately, it seems that under certain circumstances, writing the
 SNDBC fails to properly switch the FIFOs and we end up sending data to
 the USB bus from the wrong FIFO.

 In the USB mass-storage error situation we're seeing, the driver was
 trying to send a 31-byte USBC command and we see that command coming
 over the SPI-bus just fine.  However, on the USB-side, the MAX3421E
 chip instead writes a 64-byte packet full of zeroes (which is the data
 we were transmitting before).  The mass-storage peripheral afterwards
 NAKs any OUT request because it never saw the new SCSI WRITE_10
 command that was encapsulated in the USBC command.

 The work-around for now is to write outgoing packets twice, so that
 both FIFOs contain the same data.  With that workaround, we have been
 able to dd 5MB blocks of data repeatedly without any issues (dd
 if=/dev/zero of=/dev/sda1 count=5000 bs=1024).

 I should mention this is with rev 0x12 of the MAX3421E chip.  The
 current rev is 0x13 so we'll try with that chip in the next few days.
 However, we are not aware of any erratas for rev 0x12 that would
 explain this behavior.

 Also, for the record, we ran the SPI bus at only 4MHz for this testing
 so we could reliably capture the data with the Saleae Logic.  Giving
 this low frequency and the fact that the Saleae was able to capture
 the correct data, I do not think that SPI corruption is to blame.  We
 saw the same error occur even with SPI at 1MHz.

 I have the full trace data if anyone is interested.  It's captures the
 complete test (from loading the max3421 driver to when the error
 occurs), so it's 55MiB in size, so I can't attach it to email.

   --david

 On Thu, Mar 13, 2014 at 8:55 AM, David Mosberger dav...@egauge.net wrote:
 Yeah, sorry, the READ_10s were a total red herring.  They're there
 because I forgot to specify bs=1024. ;-(

 I'll try to capture better traces today and if they look interesting,
 make them available.

   --david
 --
 eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.976



 --
 eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768



-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

OK, I was able to confirm this: switching the FIFO back through a
dummy write after a NAK/error-response fixes the problem I was seeing,
so it truly was a driver-bug, not a chip bug.  My apologies to Maxim!
;-)

  --david
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

Yeah, sorry, the READ_10s were a total red herring.  They're there
because I forgot to specify bs=1024. ;-(

I'll try to capture better traces today and if they look interesting,
make them available.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.976
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

OK, I finally know where the problem is coming from!  The MAX3421E
chip uses double-buffering.  Specifically, it has two 64-byte send
FIFOs.  You write up to 64 bytes to a send FIFO by repeatedly writing
to SPI register 2 (SNDFIFO).  Then you tell the chip how many bytes
you just put in the FIFO by writing SPI register 7 (the
send-byte-count or SNDBC register).  Writing SNDBC is supposed to
switch the FIFO to the USB-side so it can be transmitted on the USB
bus.

Unfortunately, it seems that under certain circumstances, writing the
SNDBC fails to properly switch the FIFOs and we end up sending data to
the USB bus from the wrong FIFO.

In the USB mass-storage error situation we're seeing, the driver was
trying to send a 31-byte USBC command and we see that command coming
over the SPI-bus just fine.  However, on the USB-side, the MAX3421E
chip instead writes a 64-byte packet full of zeroes (which is the data
we were transmitting before).  The mass-storage peripheral afterwards
NAKs any OUT request because it never saw the new SCSI WRITE_10
command that was encapsulated in the USBC command.

The work-around for now is to write outgoing packets twice, so that
both FIFOs contain the same data.  With that workaround, we have been
able to dd 5MB blocks of data repeatedly without any issues (dd
if=/dev/zero of=/dev/sda1 count=5000 bs=1024).

I should mention this is with rev 0x12 of the MAX3421E chip.  The
current rev is 0x13 so we'll try with that chip in the next few days.
However, we are not aware of any erratas for rev 0x12 that would
explain this behavior.

Also, for the record, we ran the SPI bus at only 4MHz for this testing
so we could reliably capture the data with the Saleae Logic.  Giving
this low frequency and the fact that the Saleae was able to capture
the correct data, I do not think that SPI corruption is to blame.  We
saw the same error occur even with SPI at 1MHz.

I have the full trace data if anyone is interested.  It's captures the
complete test (from loading the max3421 driver to when the error
occurs), so it's 55MiB in size, so I can't attach it to email.

  --david

On Thu, Mar 13, 2014 at 8:55 AM, David Mosberger dav...@egauge.net wrote:
 Yeah, sorry, the READ_10s were a total red herring.  They're there
 because I forgot to specify bs=1024. ;-(

 I'll try to capture better traces today and if they look interesting,
 make them available.

   --david
 --
 eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.976



-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

Alan,

On Thu, Mar 13, 2014 at 11:12 AM, Alan Stern st...@rowland.harvard.edu wrote:

 Okay.  Maybe this would have been easier to see if you had been writing
 actual data instead of just a lot of zeros; the errors would have shown
 up when you checked the received data (incorrect checksum or something
 like that).

Perhaps.  The error obviously doesn't happen very easily as otherwise
it'd have failed much earlier.  I'll be waiting to hear from MAXIM and
to try out rev 0x13 before spending time on understanding what
triggers the error.

 The work-around for now is to write outgoing packets twice, so that
 both FIFOs contain the same data.  With that workaround, we have been
 able to dd 5MB blocks of data repeatedly without any issues (dd
 if=/dev/zero of=/dev/sda1 count=5000 bs=1024).

 That sounds like you would sacrifice a lot of speed, because you are
 effectively using single buffering instead of double buffering.

The double-buffering is really only useful for isochronous
transactions (which we don't have a need for and which the driver
doesn't support), since only those can be  64 bytes.  The way the
double-buffering is implemented doesn't let us overlap transactions in
any way, so the only loss is added CPU overhead and SPI-bus time.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

To answer my own question: it appears that USB peripherals return NAKs
not only when the peripheral is not ready to accept the data, but also
when the peripheral doesn't know what to do with the data.  So an
infinite series of NAKs basically is just the device's way of saying:
I don't know what the heck to do with the data you keep sending me.

I expected to get an error result for such a case, but I can see why
sending a NAK may be the most natural response for the device.

Anyhow, hopefully this will be helpful for others in the future
(perhaps it's obvious, but it wasn't to me... ;-).

  --david

On Fri, Mar 7, 2014 at 6:16 PM, David Mosberger dav...@egauge.net wrote:
 So the MAX3421E driver is working quite well but one problem I'm
 seeing is that after running devices for a while, they seem to get
 into a mode where a bulk out transfer gets stuck soliciting and
 endless stream of NAKs.  The MAX3421E retries NAK'd transfers in the
 next frame again, only to get the same response, forever.  I see this
 both with a mass storage device and a WIFI adapter (which is
 specifically advertised as being USB 1.1 compatible).  Anybody have
 any ideas where that might be coming from?

   --david
 --
 eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768



-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

On Wed, Mar 12, 2014 at 6:21 AM, Peter Stuge pe...@stuge.se wrote:
 David Mosberger wrote:
 I couldn't figure out how to force UHCI onto an EHCI chip

 I suggested removing the ehci_hcd driver. Did that work?

Nope.  UHCI was loaded but it didn't recognize any UHCI-compatible
chips so I was left without any USB devices (not even keyboard).

 but I did find I had some old IOGEAR USB 1.1 extenders (USB-over-CAT5
 cable) and with those, the device does switch into full-speed mode on my
 computer:

 It might not be comparable.

I just need proof that the devices can be operated properly with
full-speed transactions only.  As long as it does that, I should be
fine.

  --david

 [  886.371122] usb 1-1.3.1.1.4.2: USB disconnect, device number 15
 [  950.960459] usb 1-1.2: new full-speed USB device number 16 using ehci-pci

 Looks like it's still using the ehci driver.


 You could use an FX2 data logger like the Logic or any of the 15 USD
 boards off eBay (the Logic does nothing more than they do) together
 with http://sigrok.org/ and the libsigrokdecode USB protocol decoder
 for protocol analysis.

 It's obviously not a Beagle 480 but could be more than sufficient for
 full speed.


 //Peter



-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

So, quick question to the collective linux-usb wisdom: when I collect
a USB trace on my work-computer while running the command:

 dd if=/dev/zero of=/dev/sdX1

I see the same WRITE_10 commands of 122,880 bytes (240 sectors), but
the glaring difference is that each such WRITE_10 command seems to be
followed by ~ 27 READ_10 commands reading 1KB (2 sectors), whereas
with the MAX3421 driver, I see consecutive WRITE_10 commands.  Anybody
know where those READ_10 commands are coming from?  Is it a cheap way
to poll the device if it's ready for the next block without having to
use expensive OUT transactions that get NAK'd?  I'll obviously
investigate some more, but that's the first obvious difference I have
noticed.

  --david

On Tue, Mar 11, 2014 at 7:10 PM, David Mosberger dav...@egauge.net wrote:
 I couldn't figure out how to force UHCI onto an EHCI chip but I did
 find I had some old IOGEAR USB 1.1 extenders (USB-over-CAT5 cable)
 and with those, the device does switch into full-speed mode on my
 computer:

 [  886.371122] usb 1-1.3.1.1.4.2: USB disconnect, device number 15
 [  950.960459] usb 1-1.2: new full-speed USB device number 16 using ehci-pci
 [  951.055170] usb 1-1.2: not running at top speed; connect to a high speed 
 hub
 [  951.061791] usb 1-1.2: New USB device found, idVendor=058f, idProduct=6387
 [  951.061797] usb 1-1.2: New USB device strings: Mfr=1, Product=2,
 SerialNumber=3
 [  951.061802] usb 1-1.2: Product: Mass Storage
 [  951.061805] usb 1-1.2: Manufacturer: Generic
 [  951.061809] usb 1-1.2: SerialNumber: BCABB02D
 [  951.062390] scsi5 : usb-storage 1-1.2:1.0
 [  952.060285] scsi 5:0:0:0: Direct-Access Generic  Flash Disk
   8.07 PQ: 0 ANSI: 4
 [  952.061585] sd 5:0:0:0: Attached scsi generic sg3 type 0
 [  952.064648] sd 5:0:0:0: [sdc] 1968128 512-byte logical blocks:
 (1.00 GB/961 MiB)
 [  952.065793] sd 5:0:0:0: [sdc] Write Protect is off
 [  952.065801] sd 5:0:0:0: [sdc] Mode Sense: 23 00 00 00
 [  952.067012] sd 5:0:0:0: [sdc] Write cache: disabled, read cache:
 enabled, doesn't support DPO or FUA
 [  952.076695]  sdc: sdc1
 [  952.080629] sd 5:0:0:0: [sdc] Attached SCSI removable disk

 With this setup, I can write to the device just fine:

 dd if=/dev/zero of=/dev/sdc1 count=1
 1+0 records in
 1+0 records out
 512 bytes (5.1 MB) copied, 15.1192 s, 339 kB/s

 No infinite NAK issue.

 Still scratching my head...

   --david

 On Tue, Mar 11, 2014 at 1:00 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Tue, 11 Mar 2014, David Mosberger wrote:

  It looks like the host controller is behaving correctly, which means
  the fault is in the device.  Have you tried plugging this device into a
  regular Linux PC and running the same test?

 Yup, works fine at least at least at hispeed.  I suppose I should try
 the enable UHCI only trick to see if I can test the device at
 full-speed on my computer.

 Yes, definitely, so that you are testing under the same conditions.

 Alan Stern




 --
 eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768



-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

On Wed, Mar 12, 2014 at 2:53 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Wed, 12 Mar 2014, David Mosberger wrote:

 I see the same WRITE_10 commands of 122,880 bytes (240 sectors), but
 the glaring difference is that each such WRITE_10 command seems to be
 followed by ~ 27 READ_10 commands reading 1KB (2 sectors), whereas
 with the MAX3421 driver, I see consecutive WRITE_10 commands.  Anybody
 know where those READ_10 commands are coming from?

 I'd guess it's some sort of readahead.  Not that it makes much sense to
 read sectors that are about to be overwritten.

 Did those READ commands occur before you started running dd?

Definitely not.

I attached a log of a good (working) dd, showing only the USBC
transactions.  The '\n ( commands are READ_10, the \n * commands
are WRITE_10.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
0.1075367,DATA1U S B C '28' } '0' '0' '0' '4' '0' '0' '128' '0' \n 
( '0' '0' '0' '164' '134' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','11620'
0.11749112500,DATA0U S B C '29' } '0' '0' '0' '4' '0' '0' '128' '0' \n 
( '0' '0' '0' '164' '136' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','18350'
0.12037962500,DATA1U S B C '30' } '0' '0' '0' '4' '0' '0' '128' '0' \n 
( '0' '0' '0' '164' '138' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','14500'
0.12338658333,DATA0U S B C '31' } '0' '0' '0' '4' '0' '0' '128' '0' \n 
( '0' '0' '0' '164' '140' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','12868'
0.13044325000,DATA0U S B C ! } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '144' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','35792'
0.13647270833,DATA0U S B C # } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '148' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','65082'
0.1395163,DATA1U S B C $ } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '150' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','2766'
0.14351329167,DATA0U S B C % } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '152' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','24580'
0.1493710,DATA0U S B C ' } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '156' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','5614'
0.15239312500,DATA1U S B C ( } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '158' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','46821'
0.15538312500,DATA0U S B C ) } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '160' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','56528'
0.15837558333,DATA1U S B C * } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '162' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','41946'
0.16438162500,DATA1U S B C COMMA } '0' '0' '0' '4' '0' '0' '128' '0' \n 
( '0' '0' '0' '164' '166' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','24014'
0.17036941667,DATA1U S B C . } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '170' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','18446'
0.17337262500,DATA0U S B C / } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '172' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','17134'
0.17638162500,DATA1U S B C 0 } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '174' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','19995'
0.1793798,DATA0U S B C 1 } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '176' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','58501'
0.18537245833,DATA0U S B C 3 } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '180' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','37231'
0.1913778,DATA0U S B C 5 } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '184' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','3921'
0.19437162500,DATA1U S B C 6 } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '186' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','28763'
0.19737454167,DATA0U S B C 7 } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '188' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','31419'
0.20038425000,DATA1U S B C 8 } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '190' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','55728'
0.20637004167,DATA1U S B C : } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '194' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','3546'
0.21338287500,DATA1U S B C  } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '198' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','62414'
0.21635858333,DATA0U S B C = } '0' '0' '0' '224' '1' '0' '0' '0' \n * 
'0' '0' '0' '146' '30' '0' '0' '240' '0' '0' '0' '0' '0' '0' '0','56423'
0.37253541667,DATA1U S B C  } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '200' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','24069'
0.3775197,DATA0U S B C ? } '0' '0' '0' '4' '0' '0' '128' '0' \n ( 
'0' '0' '0' '164' '202' '0' '0' '2' '0' '0' '0' '0' '0' '0' '0','25840'
0.38553287500

Re: MAX3421E: device giving NAKs forever?

[David Mosberger]

Thanks for the tips!  Yes, the saleae is very simple so it may not do
the trick.  I am also concerned about the very basic trigger
capabilities, but I'm hopeful that we can simply collect a long enough
trace and then find the interesting spots after the fact.  The
MAX3421E also has a couple of gpio ports and since we can detect the
error condition in software, we could signal the error condition on
the gpio port and use that to stop sampling.  In the worst case, we
can return it (sparkfun is just around the corner... ;-).

  --david

On Tue, Mar 11, 2014 at 4:16 AM, David Laight david.lai...@aculab.com wrote:
 From: Felipe Balbi
 On Mon, Mar 10, 2014 at 02:15:36PM -0600, David Mosberger wrote:
  I was thinking of the Salea Logic http://www.saleae.com/logic ($150).
   Are you aware of any limitations for this one that would get in the way?

 for full-speed USB it should work, I guess. Never tried though. Can it
 handle differential signalling ?

 That looks like a simple logic analyser.
 Even if it can trace the usb signals, it is unlikely to be useful
 for looking at the USB data - in particular it probably can't trigger
 on anything inside the packets.

 We've one of these http://www.mqp.com/usb500.htm but I can't recommend
 it since I can't manage to get it to trigger sensibly on anything.
 Being able to centre the trace on an error event would be nice!
 Possibly I just need to RTFM :-)

 David






-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

On Tue, Mar 11, 2014 at 12:43 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Tue, 11 Mar 2014, David Mosberger wrote:

 So I hooked up a Seleae Logic and I still can't find anything obvious
 that's wrong.  I attached the decoded USB trace for the last WRITE10
 request that's ends up with infinite NAKs.

 After receiving 8 contiguous NAKs, my driver prints some kernel
 messages and a command log trace, so that's why after the 8th NAK you
 see nothing more going on.  In reality, the OUT commands keep getting
 NAKd after the kernel messages are printed.

 From what I can see, usb-storage sends a valid WRITE10 command
 indicating that it will write 61440 bytes.

 Actually it says that it will write 122880 bytes.

Ah, OK.

  After ~147x 64-byte writes
 (~9408 bytes) the NAKs start.  Previous WRITE10s with same length work
 fine.

 Anybody sees anything wrong in this trace?

 It looks like the host controller is behaving correctly, which means
 the fault is in the device.  Have you tried plugging this device into a
 regular Linux PC and running the same test?

Yup, works fine at least at least at hispeed.  I suppose I should try
the enable UHCI only trick to see if I can test the device at
full-speed on my computer.

  --david

 Alan Stern




-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

I couldn't figure out how to force UHCI onto an EHCI chip but I did
find I had some old IOGEAR USB 1.1 extenders (USB-over-CAT5 cable)
and with those, the device does switch into full-speed mode on my
computer:

[  886.371122] usb 1-1.3.1.1.4.2: USB disconnect, device number 15
[  950.960459] usb 1-1.2: new full-speed USB device number 16 using ehci-pci
[  951.055170] usb 1-1.2: not running at top speed; connect to a high speed hub
[  951.061791] usb 1-1.2: New USB device found, idVendor=058f, idProduct=6387
[  951.061797] usb 1-1.2: New USB device strings: Mfr=1, Product=2,
SerialNumber=3
[  951.061802] usb 1-1.2: Product: Mass Storage
[  951.061805] usb 1-1.2: Manufacturer: Generic
[  951.061809] usb 1-1.2: SerialNumber: BCABB02D
[  951.062390] scsi5 : usb-storage 1-1.2:1.0
[  952.060285] scsi 5:0:0:0: Direct-Access Generic  Flash Disk
  8.07 PQ: 0 ANSI: 4
[  952.061585] sd 5:0:0:0: Attached scsi generic sg3 type 0
[  952.064648] sd 5:0:0:0: [sdc] 1968128 512-byte logical blocks:
(1.00 GB/961 MiB)
[  952.065793] sd 5:0:0:0: [sdc] Write Protect is off
[  952.065801] sd 5:0:0:0: [sdc] Mode Sense: 23 00 00 00
[  952.067012] sd 5:0:0:0: [sdc] Write cache: disabled, read cache:
enabled, doesn't support DPO or FUA
[  952.076695]  sdc: sdc1
[  952.080629] sd 5:0:0:0: [sdc] Attached SCSI removable disk

With this setup, I can write to the device just fine:

dd if=/dev/zero of=/dev/sdc1 count=1
1+0 records in
1+0 records out
512 bytes (5.1 MB) copied, 15.1192 s, 339 kB/s

No infinite NAK issue.

Still scratching my head...

  --david

On Tue, Mar 11, 2014 at 1:00 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Tue, 11 Mar 2014, David Mosberger wrote:

  It looks like the host controller is behaving correctly, which means
  the fault is in the device.  Have you tried plugging this device into a
  regular Linux PC and running the same test?

 Yup, works fine at least at least at hispeed.  I suppose I should try
 the enable UHCI only trick to see if I can test the device at
 full-speed on my computer.

 Yes, definitely, so that you are testing under the same conditions.

 Alan Stern




-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E: device giving NAKs forever?

[David Mosberger]

[Resend in plain text mode...]

Thanks for taking a look!

I thought that there might be an bug in re-transmitting the OUT requests
that are being NAK'd indefinitely, but a different flash drive works much
longer and with that drive, I see many OUT requests that get NAK'd a couple
of times, but eventually go through fine.  So I think OUT re-transmissions
are fine.  That drive still fails eventually with infinite NAKs, but this
one on a bulk-IN transaction.

I agree, it's time to get a bus analyzer.

  --david

On Sun, Mar 9, 2014 at 9:11 AM, Alan Stern st...@rowland.harvard.edu wrote:
 On Fri, 7 Mar 2014, David Mosberger wrote:

 Felipe,

 Thanks for the tip about usbmon --- that looks interesting.  Of
 course, as luck would have it, turning on usbmon changes behavior:
 dd'ing to a mass-storage device (/dev/sda1) used to fail after
 ~500KiB.  With usbmon, it fails only after about 2MiB.

 I attached two logs: first one is the usb-storage debug output without
 usbmon (fails after about 500KB of writing to /dev/sda1), second is
 with usbmon (fails about 2MiB of writing to /dev/sda1).

 I didn't see anything unusual in the logs.  It looks like you'll have
 to use a bus analyzer to see the actual packets on the wire.

 Alan Stern




-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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

MAX3421E: device giving NAKs forever?

[David Mosberger]

So the MAX3421E driver is working quite well but one problem I'm
seeing is that after running devices for a while, they seem to get
into a mode where a bulk out transfer gets stuck soliciting and
endless stream of NAKs.  The MAX3421E retries NAK'd transfers in the
next frame again, only to get the same response, forever.  I see this
both with a mass storage device and a WIFI adapter (which is
specifically advertised as being USB 1.1 compatible).  Anybody have
any ideas where that might be coming from?

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E Linux driver?

[David Mosberger]

On Mon, Mar 3, 2014 at 2:40 PM, Daniel Mack dan...@zonque.org wrote:

 1. Your patch has a large number of style issues, which
 scripts/checkpatch.pl will tell you more about.

Sure, Im not really at a point of worrying about style, but that's
easy enough to change.

 2. Is there any good reason for not using the regmap abstraction
 framework for the SPI layer? It has convenience functions for bit
 fiddling, a register cache and a nice debugging interface through
 debugfs, among other things. See include/linux/regmap.h.

I have looked at it only briefly but my impression is that it'd add a
lot of complexity and generality that is unnecessary for this case.
If somebody wanted to cook up a patch to show what it would look like
with regmap, I'd certainly be happy to look at it, though.

 Also, out of curiosity: which SPI master controller are you using in
 your setup? Is the whole thing actually fast enough for real-world
 applications? Which data rates are you able to get through on
 mass-storage devices for instance?

Yes, it's fast enough.  We just need 1Mbps at a reasonable CPU
utilization and that's what we're getting even now (no attempt to
optimize).

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.976
--
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: MAX3421E Linux driver?

[David Mosberger]

On Sun, Mar 2, 2014 at 10:04 AM, Peter Stuge pe...@stuge.se wrote:

 David Mosberger wrote:
  +++ b/drivers/usb/host/Kconfig
  @@ -4,6 +4,16 @@
   comment USB Host Controller Drivers
depends on USB
 
  +config USB_MAX3421_HCD
  +   tristate MAX3421 HCD (USB-over-SPI) support
  +   depends on USB
  +   help
  + The Maxim MAX3421E Host Controller Interface supports
  +  standard USB 1.1 high-speed hardware.

 I'd suggest supports USB 2.0-compliant full-speed devices instead.

Yeah, that comment was old and wrong.  I fixed that per your suggestion, thanks.

 And before you assume that high-speed devices (flash drives, wlan, etc)
 don't work correctly because of something your driver does I would
 strongly recommend to perform the same tests using another full-speed
 HC, e.g. by using uhci_hcd as the only hcd, disabling ehci_hcd, on
 EHCI hardware.

 I've seen high-speed-capable devices work quite poorly at full-speed;
 who cares about correctness when there is performance?

At first, we had problems recognizing high-speed devices, but that
must have been a silly bug since it got fixed without me even trying.
Certainly there can be cases where high-speed devices wouldn't work
properly at full speed, but I don't think I'm ready to blame the
devices over my driver just yet.  I may change my mind on this, of
course. ;-)

  --david

-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E Linux driver?

[David Mosberger]

Well, here is a quick-and-dirty proof-of-concept.  Warning: it's ugly
and you might go blind.  Having said that, the code works well enough
to detect
all USB devices I tried and HID devices as well as USB mass storage
seem to work fine.

I'm not terribly familiar with the details of USB and/or the Linux kernel's
USB implementation so the driver is probably doing a couple of really dumb
things.  I marked a couple of places with XXX where I'm particularly
interested in feedback.

Also, one thing that seems tricky is NAK-handling: some of the devices we
tested with have large receive buffers and they end up returning NAK for a
long time.  So the strategy as implemented is to allow up to 4 NAKs and
after that, it sleeps for 1ms for each NAK.  I'm not sure how fancier
OHCDs handle such cases.

  --david

On Thu, Jan 30, 2014 at 5:48 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Thu, Jan 30, 2014 at 05:34:59PM -0700, David Mosberger wrote:
 We have a need to graft a USB Host controller onto a SPI bus (never
 mind the wisdom of doing that, we have little
 choice in this particular instance).

 The Cypress CY7C67300 would fit the bill and has a Linux driver, but
 is probably too complex and definitely
 too expensive for our needs.

 The Maxim MAX3421E is the other option, but it has no Linux driver.
 The chip basically implements OHCI over SPI
 rather than PCI.  Anybody know of any particular reason why there is
 no Linux driver?  Has anyone already written or
 attempted to write an OHCI-over-SPI driver for MAX3421E?

 I don't think anybody has attempted it. If you wanna do it, we're happy
 to review the driver.

 cheers

 --
 balbi



-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index f5ed3d7..2e270cc 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_USB_DWC3)+= dwc3/
 obj-$(CONFIG_USB_MON)  += mon/
 
 obj-$(CONFIG_PCI)  += host/
+obj-$(CONFIG_USB_MAX3421_HCD)  += host/
 obj-$(CONFIG_USB_EHCI_HCD) += host/
 obj-$(CONFIG_USB_ISP116X_HCD)  += host/
 obj-$(CONFIG_USB_OHCI_HCD) += host/
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index d6bb128..5b226de 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -4,6 +4,16 @@
 comment USB Host Controller Drivers
depends on USB
 
+config USB_MAX3421_HCD
+   tristate MAX3421 HCD (USB-over-SPI) support
+   depends on USB
+   help
+ The Maxim MAX3421E Host Controller Interface supports
+standard USB 1.1 high-speed hardware.
+
+ To compile this driver as a module, choose M here: the module will
+be called max3421.
+
 config USB_C67X00_HCD
tristate Cypress C67x00 HCD support
depends on USB
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 1eb4c30..1dfb836 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -23,6 +23,8 @@ obj-$(CONFIG_USB_WHCI_HCD)+= whci/
 
 obj-$(CONFIG_PCI)  += pci-quirks.o
 
+obj-$(CONFIG_USB_MAX3421_HCD)  += max3421.o
+
 obj-$(CONFIG_USB_EHCI_HCD) += ehci-hcd.o
 obj-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o
 obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)+= ehci-platform.o
diff --git a/drivers/usb/host/max3421.c b/drivers/usb/host/max3421.c
new file mode 100644
index 000..61df0e6
--- /dev/null
+++ b/drivers/usb/host/max3421.c
@@ -0,0 +1,1406 @@
+/*
+ * MAX3421 Host Controller driver for USB.
+ *
+ * Maintainer: David Mosberger-Tang dav...@egauge.net
+ *
+ * (C) Copyright 2014 David Mosberger-Tang dav...@egauge.net
+ *
+ * MAX3421 is a chip implementing a USB 2.0 Full-/Low-Speed host
+ * controller on a SPI bus.
+ *
+ * Based on:
+ * o MAX3421E datasheet
+ * http://datasheets.maximintegrated.com/en/ds/MAX3421E.pdf
+ * o MAX3421E Programming Guide
+ * http://www.hdl.co.jp/ftpdata/utl-001/AN3785.pdf
+ * o gadget/dummy_hcd.c
+ * For USB HCD implementation.
+ * o Arduino MAX3421 driver
+ * https://github.com/felis/USB_Host_Shield_2.0/blob/master/Usb.cpp
+ *
+ * This file is licenced under the GPL.
+ */
+
+#include linux/module.h
+#include linux/scatterlist.h
+#include linux/spi/spi.h
+#include linux/usb.h
+#include linux/usb/hcd.h
+
+#define DRIVER_DESCMAX3421 USB Host-Controller Driver
+#define DRIVER_VERSION 0.0
+
+#define POWER_BUDGET   500 // in mA; use 8 for low-power port testing
+
+// Port-change mask:
+#define PORT_C_MASK((USB_PORT_STAT_C_CONNECTION |  \
+ USB_PORT_STAT_C_ENABLE |  \
+ USB_PORT_STAT_C_SUSPEND | \
+ USB_PORT_STAT_C_OVERCURRENT | \
+ USB_PORT_STAT_C_RESET)  16)
+
+enum max3421_rh_state {
+   MAX3421_RH_RESET,
+   MAX3421_RH_SUSPENDED,
+   MAX3421_RH_RUNNING
+};
+
+struct max3421_hcd {
+   spinlock_t

MAX3421E Linux driver?

[David Mosberger]

We have a need to graft a USB Host controller onto a SPI bus (never
mind the wisdom of doing that, we have little
choice in this particular instance).

The Cypress CY7C67300 would fit the bill and has a Linux driver, but
is probably too complex and definitely
too expensive for our needs.

The Maxim MAX3421E is the other option, but it has no Linux driver.
The chip basically implements OHCI over SPI
rather than PCI.  Anybody know of any particular reason why there is
no Linux driver?  Has anyone already written or
attempted to write an OHCI-over-SPI driver for MAX3421E?

Best regards,

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E Linux driver?

[David Mosberger]

Peter,

On Thu, Jan 30, 2014 at 6:09 PM, Peter Stuge pe...@stuge.se wrote:
 David Mosberger wrote:
 The Maxim MAX3421E is the other option, but it has no Linux driver.

 You could look at http://arduino.cc/en/Main/ArduinoBoardADK for a
 reference that might even work.

We are aware of Arduino but the point of our project is to be able to
use normal Linux USB drivers.  As far as I know, Arduino has it's own
(limited) USB stack.  Perhaps I'm missing something?

  --david



 //Peter



-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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: MAX3421E Linux driver?

[David Mosberger]

Great find, I'm impressed!  Yeah, probably not much hope of contacting
the author (it's from 2008), but it sounds like the basics were pretty
straight-forward.  Good to know.

  --david

On Thu, Jan 30, 2014 at 6:54 PM,  si...@mungewell.org wrote:
 We have a need to graft a USB Host controller onto a SPI bus (never
 mind the wisdom of doing that, we have little
 choice in this particular instance).

 The Cypress CY7C67300 would fit the bill and has a Linux driver, but
 is probably too complex and definitely
 too expensive for our needs.

 The Maxim MAX3421E is the other option, but it has no Linux driver.

 Since the device sounds very interesting, I started googling around to
 find this comment:
 http://hary040314.blog.163.com/blog/static/77162442200822125922706/
 --
 Today ,I add max3421e to linux kernel 2.6.13 as usb host controller on
 platform s3c2440 successful.

 For now,I can mount mobile hard disk to sysfs of linux,and list all the
 files and folders on the mobile hard disk.

 But the speed is slow,and even worse,I can't exchange files larger than 8M
 through it.I thought ,the road to success finally ahead is long.
 --

 The rest of the page is Chinese(?), but it might suggest that someone got
 something working at one time
 Simon




-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768
--
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