[U-Boot] [PATCH] usbtty/omap: update to current API

[Bryan O'Donoghue]

On Mon, 10 Nov 2008 17:05:00 +0100
Jean-Christophe PLAGNIOL-VILLARD <plagnioj at 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