Re: upd985xx USB slave patch

[Bart Veer <bartv at ecoscentric dot com>]

>>>>> "Andrew" == Andrew Lunn <andrew.lunn@ascom.ch> writes:

    Andrew> Hi Folks

    Andrew> Attached is a patch for the upd985xx USB slave. This patch
    Andrew> fixes a problem when the ep0 maximum payload length is not
    Andrew> 8. RedHat have an internal patch which has not yet been
    Andrew> contributed which changes the default to 64. (This patch
    Andrew> does not require that patch).

    Andrew> The problem occurs when replying with less bytes then
    Andrew> requested. If this happens and the reply is a multiple of
    Andrew> the maximum payload size, a zero byte packet must be
    Andrew> appended. The current code has a hardcoded 8 for the
    Andrew> maximum packet size. This patch changes it to use the CDL
    Andrew> configured value.

    Andrew> The patch also adds some clarification has to how the
    Andrew> maximum payload size is determined from the CDL options.

I agree with the analysis, but am not happy with the solution. The
real problem is with Red Hat's original patch which failed to
distinguish between packet size and transaction size. That might have
been acceptable if it only affected one line of code, but now the
problem is spreading.

My alternative patch below introduces a new configuration option
CYGNUM_DEVS_USB_UPD985XX_EP0_PKTSIZE, to control the size of
individual packets in control message transactions. RXBUFSIZE and
TXBUFSIZE then revert to their original meaning, the maximum
transaction size. PKTSIZE has appropriate constraints, e.g. you can
only configure a packet size that is permitted by the USB
specification.

I'll have to leave it to you to check this patch on actual hardware.

Incidentally, this patch also obviates Red Hat's patch so there is no
need to worry about if or when they'll submit theirs.

Bart

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/usb/nec_upd985xx/current/ChangeLog,v
retrieving revision 1.3
diff -u -r1.3 ChangeLog
--- ChangeLog	27 Oct 2002 15:04:06 -0000	1.3
+++ ChangeLog	1 Dec 2002 15:56:28 -0000
@@ -1,3 +1,9 @@
+2002-12-01  Bart Veer  <bartv@ecoscentric.com>
+
+	* src/usbs_upd985xx.c, cdl/usbs_upd985xx.cdl:
+	Make the control packet size configurable, to work around a
+	problem detected by USB compliance testing.
+
 2002-10-26  Bart Veer  <bartv@ecoscentric.com>
 
 	* src/usbs_upd985xx.c (ep0_rx_dsr):
Index: cdl/usbs_upd985xx.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/usb/nec_upd985xx/current/cdl/usbs_upd985xx.cdl,v
retrieving revision 1.2
diff -u -r1.2 usbs_upd985xx.cdl
--- cdl/usbs_upd985xx.cdl	23 May 2002 23:01:26 -0000	1.2
+++ cdl/usbs_upd985xx.cdl	1 Dec 2002 15:56:30 -0000
@@ -88,10 +89,37 @@
             "
         }
 
+	cdl_option CYGNUM_DEVS_USB_UPD985XX_EP0_PKTSIZE {
+	    display	  "Size of endpoint 0 control packets"
+	    flavor        data
+	    default_value 8
+	    legal_values  { 8 16 32 64 }
+	    description "
+                Control messages on endpoint 0 are split into packets of
+                8, 16, 32 or 64 bytes - these are the values permitted by the
+                USB specification. The same packet size is used for both
+                receives and transmits. This value must also be used for the
+                max_packet_size field of the device descriptor in the
+                application's USB enumeration data.
+
+                According to section 5.5.5 of the USB specification, if a new
+                control message is received before the previous transaction
+                has completed then the previous transaction must be aborted.
+                If that transaction involved transferring data to the host
+                then there is a problem: that data may still be queued for
+                transmission and the NEC USB device appears to provide no way
+                of aborting that transmit. The problem is unlikely to arise
+                with normal usage, but may be detected by compliance
+                testsuites. Increasing the packet size to its maximum value
+                of 64 reduces the probability of failure.
+            "
+	}
+	
 	cdl_option CYGNUM_DEVS_USB_UPD985XX_EP0_TXBUFSIZE {
 	    display       "Size of statically-allocated endpoint 0 transmit buffer"
 	    flavor        data
 	    default_value 256
+	    requires      { CYGNUM_DEVS_USB_UPD985XX_EP0_TXBUFSIZE >= CYGNUM_DEVS_USB_UPD985XX_EP0_PKTSIZE }
 	    description "
 	        The implementation of the support for endpoint 0 uses
 	        a single static buffer to hold the response to the
@@ -109,7 +137,7 @@
 	    display       "Size of statically-allocated endpoint 0 transmit buffer"
 	    flavor        data
 	    default_value 64
-	    requires      { CYGNUM_DEVS_USB_UPD985XX_EP0_RXBUFSIZE >= 8 }
+	    requires      { CYGNUM_DEVS_USB_UPD985XX_EP0_RXBUFSIZE >= CYGNUM_DEVS_USB_UPD985XX_EP0_PKTSIZE }
 	    description "
 	        The implementation of the support for endpoint 0 uses
 	        a single static buffer to hold incoming control messages.
Index: src/usbs_upd985xx.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/usb/nec_upd985xx/current/src/usbs_upd985xx.c,v
retrieving revision 1.3
diff -u -r1.3 usbs_upd985xx.c
--- src/usbs_upd985xx.c	27 Oct 2002 15:04:28 -0000	1.3
+++ src/usbs_upd985xx.c	1 Dec 2002 15:56:38 -0000
@@ -1113,7 +1114,8 @@
 //
 // There is one special case. If the host asked for e.g. a string
 // descriptor and asked for 255 bytes, but the string was only
-// e.g. 32 bytes, then there is a problem. The data will be
+// e.g. 32 bytes, then there is a problem. With a default value
+// for CYGNUM_DEVS_USB_UPD985XX_EP0_PKTSIZE, the data will be
 // transferred as four 8-byte packets, but it is necessary to
 // terminate the transfer with a 0-byte packet. Endpoint 0 always
 // operates in NZLP mode so the hardware will never generate
@@ -1535,7 +1537,7 @@
                     if (actual_length > length) {
                         actual_length = length;
                     } 
-                    if ((length != actual_length) && (0 == (actual_length % 8))) {
+                    if ((length != actual_length) && (0 == (actual_length % CYGNUM_DEVS_USB_UPD985XX_EP0_PKTSIZE))) {
                         ep0.tx_needs_zero_transfer = true;
                     } else {
                         ep0.tx_needs_zero_transfer = false;
@@ -1662,10 +1664,10 @@
     // Start a receive operation for a control message.
     ep0_start_rx(8);
     
-    // The endpoint 0 control register. Sticking with the default
-    // 8-byte packet size seems like a good idea. Setting the
+    // The endpoint 0 control register. The control packet size is
+    // configurable, with a default value of 8. Setting the
     // enabled bit here affects the state as seen by the host.
-    *EP0_CR                     = IBUS_SWAP32(EP0_CR_EP0EN | 8);                    FLUSH_IBUS();
+    *EP0_CR                     = IBUS_SWAP32(EP0_CR_EP0EN | CYGNUM_DEVS_USB_UPD985XX_EP0_PKTSIZE); FLUSH_IBUS();
     
     // The other endpoint registers will be initialized by the appropriate
     // _init() functions. Note that those other _init() functions should