[U-Boot] [PATCH 1/3] usbether: Fixed bug when using with PXA25X chips

[Marek Vasut]

Dear Łukasz Dałek,

> On 17.08.2012 22:48, Marek Vasut wrote:
> > Dear Łukasz Dałek,
> > 
> >> PXA25X chips don't support alternate settings so code in ether.c
> >> disables usage of CDC.
> >> But only code defined between DEV_CONFIG_CDC signals that network is up.
> >> This patch is fixing this bug by addding pxa_connect_gadget() which on
> >> pxa25x chips signals that network is up and do nothing on any other
> >> chips.
> > 
> > You're getting better, it's a good sign you're learning, I'm glad to see
> > it :-)
> 
> It's my first big project in which I got involved so I'm ready for
> criticism.
> 
> >> Signed-off-by: Łukasz Dałek<luk0104 at gmail.com>
> >> ---
> >> 
> >>   drivers/usb/gadget/ether.c |   21 ++++++++++++++++++++-
> >>   1 files changed, 20 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
> >> index d975fb6..964fe2e 100644
> >> --- a/drivers/usb/gadget/ether.c
> >> +++ b/drivers/usb/gadget/ether.c
> >> @@ -44,7 +44,12 @@ extern struct platform_data brd;
> >> 
> >>   unsigned packet_received, packet_sent;
> >> 
> >> -#define DEV_CONFIG_CDC	1
> >> +#ifdef CONFIG_USB_GADGET_PXA2XX
> >> +# undef DEV_CONFIG_CDC
> >> +# define DEV_CONFIG_SUBSET 1
> > 
> > Can't we make it into a config value?
> > 
> > Does this work for you:
> > 1) Add:
> > CONFIG_USB_ETH_SUBSET
> > CONFIG_USB_ETH_CDC
> > 
> > 2) Change this to
> > 
> > #if !defined(...SUBSET) || !defined(...CDC) || !defined(RNDIS)
> > #define DEV_CONFIG_CDC /* preserve default behavior */
> > #endif
> > 
> > #if defined(...SUBSET)
> > #define DEV_CONFIG_SUBSET
> > #endif
> > 
> > #if defined(...CDC)
> > #define DEV_CONFIG_CDC
> > #endif
> > 
> > /* Note that RNDIS is already fixed */
> 
> Ok.
> 
> > -- FROM HERE IT GOES INTO DIFFERENT PATCH --
> > 
> > 3) Replace DEV_CONFIG_CDC with CONFIG_USB_ETH_CDC, remove the last #if
> > defined above
> 
> Which one #if? That one I'd define in previous patch?

Yes, continuous rework so you avoid breakage and keep this bisectable.

> > 4) DTTO for SUBSET
> > 
> > 5) define SUBSET in your header file
> 
> In my board's header file?

Yes

> >> +#else
> >> +# define DEV_CONFIG_CDC	1
> >> +#endif
> >> 
> >>   #define GFP_ATOMIC ((gfp_t) 0)
> >>   #define GFP_KERNEL ((gfp_t) 0)
> >> 
> >> @@ -864,7 +869,9 @@ static struct usb_gadget_strings	stringtab = {
> >> 
> >>   /*====================================================================
> >>   ====
> >> 
> >> ====*/ static u8 control_req[USB_BUFSIZ];
> >> +#if defined(DEV_CONFIG_CDC) || defined(CONFIG_USB_ETH_RNDIS)
> >> 
> >>   static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));
> >> 
> >> +#endif
> > 
> > What trouble do you face here?
> > 
> > Tom, this aligned(4) doesn't look correct, some ALLOC_ALIGNED or friend
> > would help here, right ? :)
> 
> If you don't define DEV_CONFIG_CDC nor ...RNDIS and define
> DEV_CONFIG_SUBSET then it won't compile (because of STATUS_BYTECOUNT).

Fix STATUS_BYTECOUNT then. Why is status_req global at all?

> >>   /**
> >> 
> >> @@ -1252,6 +1259,17 @@ static void rndis_command_complete(struct usb_ep
> >> *ep, struct usb_request *req)
> >> 
> >>   #endif	/* RNDIS */
> >> 
> >> +#ifdef CONFIG_USB_GADGET_PXA2XX
> >> +static inline void pxa_connect_gadget(void)
> >> +{
> >> +	debug("PXA connecting gadget...\n");
> >> +	l_ethdev.network_started = 1;
> >> +	printf("USB network up!\n");
> > 
> > Definitelly not printf(), but is this really PXA specific or is this part
> > of the CDC subset goo?
> 
> I've took pattern from code in this file. What should I use instead of
> printf?

debug() ? I guess we can wrap that function directly into the code anyway and 
drop all the debug output in it altogether, boiling it down only to 
l_ethdev.network_started = 1;

> CDC subset disable some code for PXA which signalise that network was
> started (l_ethdev... = 1) so this is quick-fix for this bug.

This is really sad :-(

> Łukasz Dałek

Best regards,
Marek Vasut