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

login
register
mail settings
Submitter Łukasz Dałek
Date Aug. 17, 2012, 8:31 p.m.
Message ID <1345235499-32556-1-git-send-email-luk0104@gmail.com>
Download mbox | patch
Permalink /patch/178356/
State Superseded
Headers show

Comments

Łukasz Dałek - Aug. 17, 2012, 8:31 p.m.
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.

Signed-off-by: Łukasz Dałek <luk0104@gmail.com>
---
 drivers/usb/gadget/ether.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)
Marek Vasut - Aug. 17, 2012, 8:48 p.m.
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 :-)

> Signed-off-by: Łukasz Dałek <luk0104@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 */

-- FROM HERE IT GOES INTO DIFFERENT PATCH --

3) Replace DEV_CONFIG_CDC with CONFIG_USB_ETH_CDC, remove the last #if defined 
above

4) DTTO for SUBSET

5) define SUBSET in your header file

is it clear?


> +#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 ? :)

> 
>  /**
> @@ -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?

> +}
> +#else
> +static inline void pxa_connect_gadget(void) { }
> +#endif
> +
>  /*
>   * The setup() callback implements all the ep0 functionality that's not
>   * handled lower down.  CDC has a number of less-common features:
> @@ -1352,6 +1370,7 @@ eth_setup(struct usb_gadget *gadget, const struct
> usb_ctrlrequest *ctrl) if (gadget_is_pxa(gadget)) {
>  			value = eth_set_config(dev, DEV_CONFIG_VALUE,
>  						GFP_ATOMIC);
> +			pxa_connect_gadget();
>  			goto done_set_intf;
>  		}

Best regards,
Marek Vasut
Łukasz Dałek - Aug. 17, 2012, 10:07 p.m.
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@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?

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

>> +#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).

>>   /**
>> @@ -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?
CDC subset disable some code for PXA which signalise that network was
started (l_ethdev... = 1) so this is quick-fix for this bug.

Łukasz Dałek
Marek Vasut - Aug. 17, 2012, 11:32 p.m.
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@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

Patch

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
+#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
 
 
 /**
@@ -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");
+}
+#else
+static inline void pxa_connect_gadget(void) { }
+#endif
+
 /*
  * The setup() callback implements all the ep0 functionality that's not
  * handled lower down.  CDC has a number of less-common features:
@@ -1352,6 +1370,7 @@  eth_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 		if (gadget_is_pxa(gadget)) {
 			value = eth_set_config(dev, DEV_CONFIG_VALUE,
 						GFP_ATOMIC);
+			pxa_connect_gadget();
 			goto done_set_intf;
 		}