Re: [U-Boot] [PATCH] usbtty/omap: update to current API
On 20:59 Sun 09 Nov , Wolfgang Denk wrote:
Dear Jean-Christophe PLAGNIOL-VILLARD,
In message 1226254923-8378-1-git-send-email-plagnioj@jcrosoft.com you wrote:
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com
Please explain what exactly you are trying to change / fix here.
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e738c56..448defa 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -22,14 +22,13 @@ */
#include <common.h>
+#include <config.h> /* If defined, override Linux identifiers with
* vendor specific ones */"If defined" - what needs to be defined to have any impact here?
And why do we care about Linux identifiers here? That's U-Boot, not Linux.
Yes I may remove the comment because I've allow to not overwrite them
+#ifndef CONFIG_USBD_VENDORID #define CONFIG_USBD_VENDORID 0x0525 /* Linux/NetChip */ +#endif +#ifndef CONFIG_USBD_PRODUCTID_GSERIAL #define CONFIG_USBD_PRODUCTID_GSERIAL 0xa4a6 /* gserial */ +#endif +#ifndef CONFIG_USBD_PRODUCTID_CDCACM #define CONFIG_USBD_PRODUCTID_CDCACM 0xa4a7 /* CDC ACM */ +#endif +#ifndef CONFIG_USBD_MANUFACTURER #define CONFIG_USBD_MANUFACTURER "Das U-Boot" +#endif +#ifndef CONFIG_USBD_PRODUCT_NAME #define CONFIG_USBD_PRODUCT_NAME U_BOOT_VERSION +#endif
+#ifndef CONFIG_USBD_CONFIGURATION_STR #define CONFIG_USBD_CONFIGURATION_STR "TTY via USB" +#endif
All these new config options need to be documented in the README.
this not new config option this is present option which is supposed to be define in the config file or use this defaul one.
I'll take a look on the README anyway and add it if nor present
/* Called to start packet transmission. */ -void udc_endpoint_write (struct usb_endpoint_instance *endpoint) +int udc_endpoint_write (struct usb_endpoint_instance *endpoint) { unsigned short epnum = endpoint->endpoint_address & USB_ENDPOINT_NUMBER_MASK; @@ -1081,6 +1081,8 @@ void udc_endpoint_write (struct usb_endpoint_instance *endpoint) /* deselect the endpoint FIFO */ outw (UDC_EP_Dir | epnum, UDC_EP_NUM); }
- return 0;
}
If the only way to exit the function is by returning 0 at the end, then we should rather leave this as is and have this function return void - otherwise we suggest that different values could be returned which is not the case.
on mpc8xx they do return -1 if failed
and they share to same api
Best Regards, J.
Hello Jean-Christophe,
In message 1226254923-8378-1-git-send-email-plagnioj@jcrosoft.com you wrote:
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c
What are we going to do with this patch?
Kind Regards,
Remy
On 22:00 Wed 26 Nov , Remy Bohmer wrote:
Hello Jean-Christophe,
In message 1226254923-8378-1-git-send-email-plagnioj@jcrosoft.com you wrote:
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c
What are we going to do with this patch?
I'll work on it next week
Best Regards, J.
Dear Jean-Christophe PLAGNIOL-VILLARD,
In message 20081127124413.GD3644@game.jcrosoft.org you wrote:
On 22:00 Wed 26 Nov , Remy Bohmer wrote:
Hello Jean-Christophe,
In message 1226254923-8378-1-git-send-email-plagnioj@jcrosoft.com you wrote:
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c
What are we going to do with this patch?
I'll work on it next week
Next week was a long time ago.
What happened to that patch?
Best regards,
Wolfgang Denk
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com --- the doc about the CONFIG_USBD is already in the README
Best Regards, J. drivers/serial/usbtty.c | 6 ++---- drivers/serial/usbtty.h | 31 +++++++++++++++++++++---------- drivers/usb/usbdcore_omap1510.c | 4 +++- include/usbdcore_omap1510.h | 6 +++--- 4 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e738c56..7eba470 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -22,16 +22,14 @@ */
#include <common.h> - +#include <config.h> #include <circbuf.h> #include <devices.h> #include "usbtty.h" #include "usb_cdc_acm.h" #include "usbdescriptors.h" -#include <config.h> /* If defined, override Linux identifiers with - * vendor specific ones */
-#if 0 +#ifdef DEBUG #define TTYDBG(fmt,args...)\ serial_printf("[%s] %s %d: "fmt, __FILE__,__FUNCTION__,__LINE__,##args) #else diff --git a/drivers/serial/usbtty.h b/drivers/serial/usbtty.h index 71c47bc..ecefde5 100644 --- a/drivers/serial/usbtty.h +++ b/drivers/serial/usbtty.h @@ -24,11 +24,11 @@ #ifndef __USB_TTY_H__ #define __USB_TTY_H__
-#include "usbdcore.h" +#include <usbdcore.h> #if defined(CONFIG_PPC) -#include "usbdcore_mpc8xx.h" +#include <usbdcore_mpc8xx.h> #elif defined(CONFIG_ARM) -#include "usbdcore_omap1510.h" +#include <usbdcore_omap1510.h> #endif
#include <version_autogenerated.h> @@ -36,14 +36,25 @@ /* If no VendorID/ProductID is defined in config.h, pretend to be Linux * DO NOT Reuse this Vendor/Product setup with protocol incompatible devices */
-#define CONFIG_USBD_VENDORID 0x0525 /* Linux/NetChip */ -#define CONFIG_USBD_PRODUCTID_GSERIAL 0xa4a6 /* gserial */ -#define CONFIG_USBD_PRODUCTID_CDCACM 0xa4a7 /* CDC ACM */ -#define CONFIG_USBD_MANUFACTURER "Das U-Boot" -#define CONFIG_USBD_PRODUCT_NAME U_BOOT_VERSION - +#ifndef CONFIG_USBD_VENDORID +#define CONFIG_USBD_VENDORID 0x0525 /* Linux/NetChip */ +#endif +#ifndef CONFIG_USBD_PRODUCTID_GSERIAL +#define CONFIG_USBD_PRODUCTID_GSERIAL 0xa4a6 /* gserial */ +#endif +#ifndef CONFIG_USBD_PRODUCTID_CDCACM +#define CONFIG_USBD_PRODUCTID_CDCACM 0xa4a7 /* CDC ACM */ +#endif +#ifndef CONFIG_USBD_MANUFACTURER +#define CONFIG_USBD_MANUFACTURER "Das U-Boot" +#endif +#ifndef CONFIG_USBD_PRODUCT_NAME +#define CONFIG_USBD_PRODUCT_NAME U_BOOT_VERSION +#endif
-#define CONFIG_USBD_CONFIGURATION_STR "TTY via USB" +#ifndef CONFIG_USBD_CONFIGURATION_STR +#define CONFIG_USBD_CONFIGURATION_STR "TTY via USB" +#endif
#define CONFIG_USBD_SERIAL_OUT_ENDPOINT UDC_OUT_ENDPOINT #define CONFIG_USBD_SERIAL_OUT_PKTSIZE UDC_OUT_PACKET_SIZE diff --git a/drivers/usb/usbdcore_omap1510.c b/drivers/usb/usbdcore_omap1510.c index cb9dc44..6b7b61b 100644 --- a/drivers/usb/usbdcore_omap1510.c +++ b/drivers/usb/usbdcore_omap1510.c @@ -1061,7 +1061,7 @@ void omap1510_udc_noniso_irq (void) */
/* Called to start packet transmission. */ -void udc_endpoint_write (struct usb_endpoint_instance *endpoint) +int udc_endpoint_write (struct usb_endpoint_instance *endpoint) { unsigned short epnum = endpoint->endpoint_address & USB_ENDPOINT_NUMBER_MASK; @@ -1078,6 +1078,8 @@ void udc_endpoint_write (struct usb_endpoint_instance *endpoint) /* deselect the endpoint FIFO */ outw (UDC_EP_Dir | epnum, UDC_EP_NUM); } + + return 0; }
/* Start to initialize h/w stuff */ diff --git a/include/usbdcore_omap1510.h b/include/usbdcore_omap1510.h index 526fcd9..ece0e95 100644 --- a/include/usbdcore_omap1510.h +++ b/include/usbdcore_omap1510.h @@ -168,8 +168,8 @@ #define UDC_IN_ENDPOINT 1 #define UDC_IN_PACKET_SIZE 64 #define UDC_INT_ENDPOINT 5 -#define UDC_INT_PKTSIZE 16 -#define UDC_BULK_PKTSIZE 16 +#define UDC_INT_PACKET_SIZE 16 +#define UDC_BULK_PACKET_SIZE 16
void udc_irq (void); /* Flow control */ @@ -177,7 +177,7 @@ void udc_set_nak(int epid); void udc_unset_nak (int epid);
/* Higher level functions for abstracting away from specific device */ -void udc_endpoint_write(struct usb_endpoint_instance *endpoint); +int udc_endpoint_write(struct usb_endpoint_instance *endpoint);
int udc_init (void);
Hello,
2008/12/7 Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com:
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com
the doc about the CONFIG_USBD is already in the README
Best Regards, J.
Applied to U-boot-usb tree ('next' branch)
Thanks,
Remy
drivers/serial/usbtty.c | 6 ++---- drivers/serial/usbtty.h | 31 +++++++++++++++++++++---------- drivers/usb/usbdcore_omap1510.c | 4 +++- include/usbdcore_omap1510.h | 6 +++--- 4 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e738c56..7eba470 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -22,16 +22,14 @@ */
#include <common.h>
+#include <config.h> #include <circbuf.h> #include <devices.h> #include "usbtty.h" #include "usb_cdc_acm.h" #include "usbdescriptors.h" -#include <config.h> /* If defined, override Linux identifiers with
* vendor specific ones */-#if 0 +#ifdef DEBUG #define TTYDBG(fmt,args...)\ serial_printf("[%s] %s %d: "fmt, __FILE__,__FUNCTION__,__LINE__,##args) #else diff --git a/drivers/serial/usbtty.h b/drivers/serial/usbtty.h index 71c47bc..ecefde5 100644 --- a/drivers/serial/usbtty.h +++ b/drivers/serial/usbtty.h @@ -24,11 +24,11 @@ #ifndef __USB_TTY_H__ #define __USB_TTY_H__
-#include "usbdcore.h" +#include <usbdcore.h> #if defined(CONFIG_PPC) -#include "usbdcore_mpc8xx.h" +#include <usbdcore_mpc8xx.h> #elif defined(CONFIG_ARM) -#include "usbdcore_omap1510.h" +#include <usbdcore_omap1510.h> #endif
#include <version_autogenerated.h> @@ -36,14 +36,25 @@ /* If no VendorID/ProductID is defined in config.h, pretend to be Linux
- DO NOT Reuse this Vendor/Product setup with protocol incompatible devices */
-#define CONFIG_USBD_VENDORID 0x0525 /* Linux/NetChip */ -#define CONFIG_USBD_PRODUCTID_GSERIAL 0xa4a6 /* gserial */ -#define CONFIG_USBD_PRODUCTID_CDCACM 0xa4a7 /* CDC ACM */ -#define CONFIG_USBD_MANUFACTURER "Das U-Boot" -#define CONFIG_USBD_PRODUCT_NAME U_BOOT_VERSION
+#ifndef CONFIG_USBD_VENDORID +#define CONFIG_USBD_VENDORID 0x0525 /* Linux/NetChip */ +#endif +#ifndef CONFIG_USBD_PRODUCTID_GSERIAL +#define CONFIG_USBD_PRODUCTID_GSERIAL 0xa4a6 /* gserial */ +#endif +#ifndef CONFIG_USBD_PRODUCTID_CDCACM +#define CONFIG_USBD_PRODUCTID_CDCACM 0xa4a7 /* CDC ACM */ +#endif +#ifndef CONFIG_USBD_MANUFACTURER +#define CONFIG_USBD_MANUFACTURER "Das U-Boot" +#endif +#ifndef CONFIG_USBD_PRODUCT_NAME +#define CONFIG_USBD_PRODUCT_NAME U_BOOT_VERSION +#endif
-#define CONFIG_USBD_CONFIGURATION_STR "TTY via USB" +#ifndef CONFIG_USBD_CONFIGURATION_STR +#define CONFIG_USBD_CONFIGURATION_STR "TTY via USB" +#endif
#define CONFIG_USBD_SERIAL_OUT_ENDPOINT UDC_OUT_ENDPOINT #define CONFIG_USBD_SERIAL_OUT_PKTSIZE UDC_OUT_PACKET_SIZE diff --git a/drivers/usb/usbdcore_omap1510.c b/drivers/usb/usbdcore_omap1510.c index cb9dc44..6b7b61b 100644 --- a/drivers/usb/usbdcore_omap1510.c +++ b/drivers/usb/usbdcore_omap1510.c @@ -1061,7 +1061,7 @@ void omap1510_udc_noniso_irq (void) */
/* Called to start packet transmission. */ -void udc_endpoint_write (struct usb_endpoint_instance *endpoint) +int udc_endpoint_write (struct usb_endpoint_instance *endpoint) { unsigned short epnum = endpoint->endpoint_address & USB_ENDPOINT_NUMBER_MASK; @@ -1078,6 +1078,8 @@ void udc_endpoint_write (struct usb_endpoint_instance *endpoint) /* deselect the endpoint FIFO */ outw (UDC_EP_Dir | epnum, UDC_EP_NUM); }
return 0;}
/* Start to initialize h/w stuff */ diff --git a/include/usbdcore_omap1510.h b/include/usbdcore_omap1510.h index 526fcd9..ece0e95 100644 --- a/include/usbdcore_omap1510.h +++ b/include/usbdcore_omap1510.h @@ -168,8 +168,8 @@ #define UDC_IN_ENDPOINT 1 #define UDC_IN_PACKET_SIZE 64 #define UDC_INT_ENDPOINT 5 -#define UDC_INT_PKTSIZE 16 -#define UDC_BULK_PKTSIZE 16 +#define UDC_INT_PACKET_SIZE 16 +#define UDC_BULK_PACKET_SIZE 16
void udc_irq (void); /* Flow control */ @@ -177,7 +177,7 @@ void udc_set_nak(int epid); void udc_unset_nak (int epid);
/* Higher level functions for abstracting away from specific device */ -void udc_endpoint_write(struct usb_endpoint_instance *endpoint); +int udc_endpoint_write(struct usb_endpoint_instance *endpoint);
int udc_init (void);
-- 1.5.6.5
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On 11:43 Sun 07 Dec , Remy Bohmer wrote:
Hello,
2008/12/7 Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com:
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com
the doc about the CONFIG_USBD is already in the README
Best Regards, J.
Applied to U-boot-usb tree ('next' branch)
It's a fix not a new features
Best Regards, J.
On Mon, 10 Nov 2008 17:05:00 +0100 Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com wrote:
Please explain what exactly you are trying to change / fix here.
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e738c56..448defa 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -22,14 +22,13 @@ */
#include <common.h>
+#include <config.h> /* If defined, override Linux identifiers with
* vendor specific ones */"If defined" - what needs to be defined to have any impact here?
And why do we care about Linux identifiers here? That's U-Boot, not Linux.
Yes I may remove the comment because I've allow to not overwrite them
Greetings.
The idea there is that the producer of a USB device is supposed to get themselves a VendorID and typically you'd incrementally iterate your ProductID's upwards as you release more devices.
usbtty.h in Das U-Boot "borrows" the Linux VendorID and ProductID because our CDC-ACM implementation and usbtty - "g_serial" implementation is protocol compatible with Linux.
In theory though if one were producing a device that for example had a USB port in place of an RS232 port you'd replace VendorID and ProductID with your own VendorID and ProductID.
You'd then diseminate - for example a Windows .ini file so that you could map the cdc-acm driver that ships with Windows to your USB device.
In order to have any kind of meaningful test of the USB stack though we need a default VendorID AND ProductID - hence the re-use of the Linux identifiers.
You really are supposed to go and get your OWN VendorID if you are producing a USB enabled device.
This is stated in the README albeit not very cogently by me.
<snip>
+#ifndef CONFIG_USBD_CONFIGURATION_STR #define CONFIG_USBD_CONFIGURATION_STR "TTY via USB" +#endif
All these new config options need to be documented in the README.
this not new config option this is present option which is supposed to be define in the config file or use this defaul one.
I'll take a look on the README anyway and add it if nor present
<snip>
/* Called to start packet transmission. */ -void udc_endpoint_write (struct usb_endpoint_instance *endpoint) +int udc_endpoint_write (struct usb_endpoint_instance *endpoint) { unsigned short epnum = endpoint->endpoint_address & USB_ENDPOINT_NUMBER_MASK; @@ -1081,6 +1081,8 @@ void udc_endpoint_write (struct usb_endpoint_instance *endpoint) /* deselect the endpoint FIFO */ outw (UDC_EP_Dir | epnum, UDC_EP_NUM); }
- return 0;
}
If the only way to exit the function is by returning 0 at the end, then we should rather leave this as is and have this function return void - otherwise we suggest that different values could be returned which is not the case.
on mpc8xx they do return -1 if failed
and they share to same api
Indeed. As the API stands the OMAP stuff would have to be updated.
Basically in usbtty.c - what's happening is our terminal has gone to write a USB endpoint but in midst of doing that - he received some data.
Since USB can't just throw away the data he was writing - the RX data just pre-empts the write until a later time.
The REASON we have to deal with the RX event straight away is that it might be a control packet - which the USB defines - if memory serves - as having to be dealt with immediately.
Hence modification of omap as above I believe is consistent with the requirements of the protocol !!!!!
I'd actually ACK this patch - if I read my email more often !
Taking WD's point - there may be a better or more elegant way to preempt the endpoint write - deal with the RX data - and come back later to finish off the write...
For the moment though I'd certainly say that the patch provided bring OMAP inline with what's supposed to be happening in the code - even if the wisdom or elegance of what it's trying to do - i.e. fit in with a bit of code I built ontop of the original usbtty - is questionable !
Cheers, BOD
participants (4)
-
Bryan O'Donoghue -
Jean-Christophe PLAGNIOL-VILLARD -
Remy Bohmer -
Wolfgang Denk