Patchwork [U-Boot,v2,1/3] USB: make usb_kbd obey USB DMA alignment requirements

login
register
mail settings
Submitter Allen Martin
Date Oct. 23, 2012, 5:47 a.m.
Message ID <1350971254-11412-1-git-send-email-amartin@nvidia.com>
Download mbox | patch
Permalink /patch/193354/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Allen Martin - Oct. 23, 2012, 5:47 a.m.
Change usb_kbd driver to obey alignment requirements for USB DMA on
the buffer used for data transfer.  This is necessary for
architectures that enable dcache and enable USB DMA.

Signed-off-by: Allen Martin <amartin@nvidia.com>
---
 common/usb_kbd.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Marek Vasut - Oct. 23, 2012, 7:26 a.m.
Dear Allen Martin,

> Change usb_kbd driver to obey alignment requirements for USB DMA on
> the buffer used for data transfer.  This is necessary for
> architectures that enable dcache and enable USB DMA.
> 
> Signed-off-by: Allen Martin <amartin@nvidia.com>
> ---
>  common/usb_kbd.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 19f01db..57928d9 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -106,15 +106,15 @@ static const unsigned char usb_kbd_num_keypad[] = {
>  	(USB_KBD_NUMLOCK | USB_KBD_CAPSLOCK | USB_KBD_SCROLLLOCK)
> 
>  struct usb_kbd_pdata {
> +	uint8_t		new[8];
> +	uint8_t		old[8];
> +

Some comment about the alignment won't hurt.

>  	uint32_t	repeat_delay;
> 
>  	uint32_t	usb_in_pointer;
>  	uint32_t	usb_out_pointer;
>  	uint8_t		usb_kbd_buffer[USB_KBD_BUFFER_LEN];
> 
> -	uint8_t		new[8];
> -	uint8_t		old[8];
> -
>  	uint8_t		flags;
>  };
> 
> @@ -426,7 +426,7 @@ static int usb_kbd_probe(struct usb_device *dev,
> unsigned int ifnum)
> 
>  	USB_KBD_PRINTF("USB KBD: found set protocol...\n");
> 
> -	data = malloc(sizeof(struct usb_kbd_pdata));
> +	data = memalign(USB_DMA_MINALIGN, sizeof(struct usb_kbd_pdata));

Don't we have ALLOC_CACHE_ALIGN_BUFFER and such stuff in include/common.h for 
this purpose ?

>  	if (!data) {
>  		printf("USB KBD: Error allocating private data\n");
>  		return 0;

Best regards,
Marek Vasut
Stephen Warren - Oct. 23, 2012, 4:31 p.m.
On 10/23/2012 01:26 AM, Marek Vasut wrote:
> Dear Allen Martin,
> 
>> Change usb_kbd driver to obey alignment requirements for USB DMA on
>> the buffer used for data transfer.  This is necessary for
>> architectures that enable dcache and enable USB DMA.

>> @@ -426,7 +426,7 @@ static int usb_kbd_probe(struct usb_device *dev,
>> unsigned int ifnum)
>>
>>  	USB_KBD_PRINTF("USB KBD: found set protocol...\n");
>>
>> -	data = malloc(sizeof(struct usb_kbd_pdata));
>> +	data = memalign(USB_DMA_MINALIGN, sizeof(struct usb_kbd_pdata));
> 
> Don't we have ALLOC_CACHE_ALIGN_BUFFER and such stuff in include/common.h for 
> this purpose ?

That's for stack-based data structures, whereas this data structure
sticks around for more than the duration of this function.
Stephen Warren - Oct. 23, 2012, 4:51 p.m.
On 10/22/2012 11:47 PM, Allen Martin wrote:
> Change usb_kbd driver to obey alignment requirements for USB DMA on
> the buffer used for data transfer.  This is necessary for
> architectures that enable dcache and enable USB DMA.

The series,
Tested-by: Stephen Warren <swarren@nvidia.com>

BTW, I tested tegra-kbc too, and that does indeed currently work (at
least in my local dev branch based on u-boot/master).

Note that patch 2 has a merge conflict with the following patch in
u-boot-tegra/next, since I assume your series is based on u-boot/master
not u-boot-tegra/next:

799f182 ARM: tegra: use standard variables to define load addresses

It's pretty simple to resolve though.
Allen Martin - Oct. 23, 2012, 4:51 p.m.
On Tue, Oct 23, 2012 at 12:26:31AM -0700, Marek Vasut wrote:
> Dear Allen Martin,
> 
> > Change usb_kbd driver to obey alignment requirements for USB DMA on
> > the buffer used for data transfer.  This is necessary for
> > architectures that enable dcache and enable USB DMA.
> > 
> > Signed-off-by: Allen Martin <amartin@nvidia.com>
> > ---
> >  common/usb_kbd.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> > index 19f01db..57928d9 100644
> > --- a/common/usb_kbd.c
> > +++ b/common/usb_kbd.c
> > @@ -106,15 +106,15 @@ static const unsigned char usb_kbd_num_keypad[] = {
> >  	(USB_KBD_NUMLOCK | USB_KBD_CAPSLOCK | USB_KBD_SCROLLLOCK)
> > 
> >  struct usb_kbd_pdata {
> > +	uint8_t		new[8];
> > +	uint8_t		old[8];
> > +
> 
> Some comment about the alignment won't hurt.

Good idea.

> 
> >  	uint32_t	repeat_delay;
> > 
> >  	uint32_t	usb_in_pointer;
> >  	uint32_t	usb_out_pointer;
> >  	uint8_t		usb_kbd_buffer[USB_KBD_BUFFER_LEN];
> > 
> > -	uint8_t		new[8];
> > -	uint8_t		old[8];
> > -
> >  	uint8_t		flags;
> >  };
> > 
> > @@ -426,7 +426,7 @@ static int usb_kbd_probe(struct usb_device *dev,
> > unsigned int ifnum)
> > 
> >  	USB_KBD_PRINTF("USB KBD: found set protocol...\n");
> > 
> > -	data = malloc(sizeof(struct usb_kbd_pdata));
> > +	data = memalign(USB_DMA_MINALIGN, sizeof(struct usb_kbd_pdata));
> 
> Don't we have ALLOC_CACHE_ALIGN_BUFFER and such stuff in include/common.h for 
> this purpose ?

There seems to be some discrepency here, because ehci-hcd.c uses
USB_DMA_MINALIGN for all cache operations, which may or may not be the
same as ARCH_DMA_MINALIGN, see usb.h:

/*
 * The EHCI spec says that we must align to at least 32 bytes.
 However,
 * some platforms require larger alignment.
 */
#if ARCH_DMA_MINALIGN > 32
#define USB_DMA_MINALIGN        ARCH_DMA_MINALIGN
#else
#define USB_DMA_MINALIGN        32
#endif

For tegra this is fine, because ARCH_DMA_MINALIGN > 32, but grepping
through header files, I see a few:

#define ARCH_DMA_MINALIGN       16

which will definately break.  It looks like all other usb class
drivers use ALLOC_CACHE_ALIGN_BUFFER() though, so for consistency the
usb keyboard driver probably should too, but it seems like a potential
problem.


> 
> >  	if (!data) {
> >  		printf("USB KBD: Error allocating private data\n");
> >  		return 0;
> 
> Best regards,
> Marek Vasut

-Allen
Allen Martin - Oct. 23, 2012, 5:02 p.m.
On Tue, Oct 23, 2012 at 09:51:06AM -0700, Stephen Warren wrote:
> On 10/22/2012 11:47 PM, Allen Martin wrote:
> > Change usb_kbd driver to obey alignment requirements for USB DMA on
> > the buffer used for data transfer.  This is necessary for
> > architectures that enable dcache and enable USB DMA.
> 
> The series,
> Tested-by: Stephen Warren <swarren@nvidia.com>
> 
> BTW, I tested tegra-kbc too, and that does indeed currently work (at
> least in my local dev branch based on u-boot/master).

Yes, I also tried on a seaboard with internal keyboard and it works,
although once the USB keyboard driver loads the internal keyboard
stops working.  I haven't tracked down why, but it seems like a bug I
can live with for now as seaboards with internal keyboards are pretty
rare these days, and how many keyboards do you need in u-boot anyway?
:^)

> 
> Note that patch 2 has a merge conflict with the following patch in
> u-boot-tegra/next, since I assume your series is based on u-boot/master
> not u-boot-tegra/next:
> 
> 799f182 ARM: tegra: use standard variables to define load addresses
> 
> It's pretty simple to resolve though.

Yes, I based it on u-boot/master, once we figure out what trees each
patch is destined for I'll rebase appropriately.

-Allen
Marek Vasut - Oct. 23, 2012, 10:03 p.m.
Dear Allen Martin,

> On Tue, Oct 23, 2012 at 09:51:06AM -0700, Stephen Warren wrote:
> > On 10/22/2012 11:47 PM, Allen Martin wrote:
> > > Change usb_kbd driver to obey alignment requirements for USB DMA on
> > > the buffer used for data transfer.  This is necessary for
> > > architectures that enable dcache and enable USB DMA.
> > 
> > The series,
> > Tested-by: Stephen Warren <swarren@nvidia.com>
> > 
> > BTW, I tested tegra-kbc too, and that does indeed currently work (at
> > least in my local dev branch based on u-boot/master).
> 
> Yes, I also tried on a seaboard with internal keyboard and it works,
> although once the USB keyboard driver loads the internal keyboard
> stops working.  I haven't tracked down why, but it seems like a bug I
> can live with for now as seaboards with internal keyboards are pretty
> rare these days, and how many keyboards do you need in u-boot anyway?

Good thing you pointed it out. Please let's not ignore a bug. How come it 
happens? What happens if you have two usb keyboards connected?

> :^)
> :
> > Note that patch 2 has a merge conflict with the following patch in
> > u-boot-tegra/next, since I assume your series is based on u-boot/master
> > not u-boot-tegra/next:
> > 
> > 799f182 ARM: tegra: use standard variables to define load addresses
> > 
> > It's pretty simple to resolve though.
> 
> Yes, I based it on u-boot/master, once we figure out what trees each
> patch is destined for I'll rebase appropriately.

I pushed u-boot-usb/master today, can you check if it still applies ?

Best regards,
Marek Vasut
Marek Vasut - Oct. 23, 2012, 10:04 p.m.
Dear Allen Martin,

> On Tue, Oct 23, 2012 at 12:26:31AM -0700, Marek Vasut wrote:
> > Dear Allen Martin,
> > 
> > > Change usb_kbd driver to obey alignment requirements for USB DMA on
> > > the buffer used for data transfer.  This is necessary for
> > > architectures that enable dcache and enable USB DMA.
> > > 
> > > Signed-off-by: Allen Martin <amartin@nvidia.com>
> > > ---
> > > 
> > >  common/usb_kbd.c |    8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> > > index 19f01db..57928d9 100644
> > > --- a/common/usb_kbd.c
> > > +++ b/common/usb_kbd.c
> > > @@ -106,15 +106,15 @@ static const unsigned char usb_kbd_num_keypad[] =
> > > {
> > > 
> > >  	(USB_KBD_NUMLOCK | USB_KBD_CAPSLOCK | USB_KBD_SCROLLLOCK)
> > >  
> > >  struct usb_kbd_pdata {
> > > 
> > > +	uint8_t		new[8];
> > > +	uint8_t		old[8];
> > > +
> > 
> > Some comment about the alignment won't hurt.
> 
> Good idea.
> 
> > >  	uint32_t	repeat_delay;
> > >  	
> > >  	uint32_t	usb_in_pointer;
> > >  	uint32_t	usb_out_pointer;
> > >  	uint8_t		usb_kbd_buffer[USB_KBD_BUFFER_LEN];
> > > 
> > > -	uint8_t		new[8];
> > > -	uint8_t		old[8];
> > > -
> > > 
> > >  	uint8_t		flags;
> > >  
> > >  };
> > > 
> > > @@ -426,7 +426,7 @@ static int usb_kbd_probe(struct usb_device *dev,
> > > unsigned int ifnum)
> > > 
> > >  	USB_KBD_PRINTF("USB KBD: found set protocol...\n");
> > > 
> > > -	data = malloc(sizeof(struct usb_kbd_pdata));
> > > +	data = memalign(USB_DMA_MINALIGN, sizeof(struct usb_kbd_pdata));
> > 
> > Don't we have ALLOC_CACHE_ALIGN_BUFFER and such stuff in include/common.h
> > for this purpose ?
> 
> There seems to be some discrepency here, because ehci-hcd.c uses
> USB_DMA_MINALIGN for all cache operations, which may or may not be the
> same as ARCH_DMA_MINALIGN, see usb.h:
> 
> /*
>  * The EHCI spec says that we must align to at least 32 bytes.
>  However,
>  * some platforms require larger alignment.
>  */
> #if ARCH_DMA_MINALIGN > 32
> #define USB_DMA_MINALIGN        ARCH_DMA_MINALIGN
> #else
> #define USB_DMA_MINALIGN        32
> #endif
> 
> For tegra this is fine, because ARCH_DMA_MINALIGN > 32, but grepping
> through header files, I see a few:
> 
> #define ARCH_DMA_MINALIGN       16
> 
> which will definately break.  It looks like all other usb class
> drivers use ALLOC_CACHE_ALIGN_BUFFER() though, so for consistency the
> usb keyboard driver probably should too, but it seems like a potential
> problem.

ALLOC_CACHE_ALIGN_BUFFER is what I had in mind, I think the EHCI driver already 
uses this.

> > >  	if (!data) {
> > >  	
> > >  		printf("USB KBD: Error allocating private data\n");
> > >  		return 0;
> > 
> > Best regards,
> > Marek Vasut
> 
> -Allen

Best regards,
Marek Vasut
Allen Martin - Oct. 24, 2012, 12:39 a.m.
On Tue, Oct 23, 2012 at 03:03:34PM -0700, Marek Vasut wrote:
> Dear Allen Martin,
> 
> > On Tue, Oct 23, 2012 at 09:51:06AM -0700, Stephen Warren wrote:
> > > On 10/22/2012 11:47 PM, Allen Martin wrote:
> > > > Change usb_kbd driver to obey alignment requirements for USB DMA on
> > > > the buffer used for data transfer.  This is necessary for
> > > > architectures that enable dcache and enable USB DMA.
> > > 
> > > The series,
> > > Tested-by: Stephen Warren <swarren@nvidia.com>
> > > 
> > > BTW, I tested tegra-kbc too, and that does indeed currently work (at
> > > least in my local dev branch based on u-boot/master).
> > 
> > Yes, I also tried on a seaboard with internal keyboard and it works,
> > although once the USB keyboard driver loads the internal keyboard
> > stops working.  I haven't tracked down why, but it seems like a bug I
> > can live with for now as seaboards with internal keyboards are pretty
> > rare these days, and how many keyboards do you need in u-boot anyway?
> 
> Good thing you pointed it out. Please let's not ignore a bug. How come it 
> happens? What happens if you have two usb keyboards connected?
> 

I'm pretty sure the USB keyboard driver doesn't support multiple
devices, I see this in drv_usb_kbd_init():

                /* We found a keyboard, check if it is already
                registered. */
                USB_KBD_PRINTF("USB KBD: found set up device.\n");
                old_dev = stdio_get_by_name(DEVNAME);
                if (old_dev) {
                        /* Already registered, just return ok. */
                        USB_KBD_PRINTF("USB KBD: is already
                registered.\n");
                        return 1;
                }

The bug is almost certainly inside the tegra kbd driver, which is why
I'm not terribly concerned about it.  The only boards that use that
driver are inside NVIDIA, and even those are rare.


> > :^)
> > :
> > > Note that patch 2 has a merge conflict with the following patch in
> > > u-boot-tegra/next, since I assume your series is based on u-boot/master
> > > not u-boot-tegra/next:
> > > 
> > > 799f182 ARM: tegra: use standard variables to define load addresses
> > > 
> > > It's pretty simple to resolve though.
> > 
> > Yes, I based it on u-boot/master, once we figure out what trees each
> > patch is destined for I'll rebase appropriately.
> 
> I pushed u-boot-usb/master today, can you check if it still applies ?

Yes, it still applies cleanly to u-boot-usb/master

> 
> Best regards,
> Marek Vasut

-Allen
Allen Martin - Oct. 24, 2012, 12:46 a.m.
On Tue, Oct 23, 2012 at 05:39:50PM -0700, Allen Martin wrote:
> On Tue, Oct 23, 2012 at 03:03:34PM -0700, Marek Vasut wrote:
> > Dear Allen Martin,
> > 
> > > On Tue, Oct 23, 2012 at 09:51:06AM -0700, Stephen Warren wrote:
> > > > On 10/22/2012 11:47 PM, Allen Martin wrote:
> > > > > Change usb_kbd driver to obey alignment requirements for USB DMA on
> > > > > the buffer used for data transfer.  This is necessary for
> > > > > architectures that enable dcache and enable USB DMA.
> > > > 
> > > > The series,
> > > > Tested-by: Stephen Warren <swarren@nvidia.com>
> > > > 
> > > > BTW, I tested tegra-kbc too, and that does indeed currently work (at
> > > > least in my local dev branch based on u-boot/master).
> > > 
> > > Yes, I also tried on a seaboard with internal keyboard and it works,
> > > although once the USB keyboard driver loads the internal keyboard
> > > stops working.  I haven't tracked down why, but it seems like a bug I
> > > can live with for now as seaboards with internal keyboards are pretty
> > > rare these days, and how many keyboards do you need in u-boot anyway?
> > 
> > Good thing you pointed it out. Please let's not ignore a bug. How come it 
> > happens? What happens if you have two usb keyboards connected?
> > 
> 
> I'm pretty sure the USB keyboard driver doesn't support multiple
> devices, I see this in drv_usb_kbd_init():
> 
>                 /* We found a keyboard, check if it is already
>                 registered. */
>                 USB_KBD_PRINTF("USB KBD: found set up device.\n");
>                 old_dev = stdio_get_by_name(DEVNAME);
>                 if (old_dev) {
>                         /* Already registered, just return ok. */
>                         USB_KBD_PRINTF("USB KBD: is already
>                 registered.\n");
>                         return 1;
>                 }
> 
> The bug is almost certainly inside the tegra kbd driver, which is why
> I'm not terribly concerned about it.  The only boards that use that
> driver are inside NVIDIA, and even those are rare.
> 

Ok, I dug into the driver code a little.  It's because the tegra
keyboard driver doesn't support CONSOLE_MUX, so when the USB keyboard
calls into iomux it takes away stdin from the tegra kbd driver, and it
has no way of ever getting it back.  Definately a bug, but unrelated
to this change.

-Allen
Marek Vasut - Oct. 24, 2012, 7:31 a.m.
Dear Allen Martin,

> On Tue, Oct 23, 2012 at 03:03:34PM -0700, Marek Vasut wrote:
> > Dear Allen Martin,
> > 
> > > On Tue, Oct 23, 2012 at 09:51:06AM -0700, Stephen Warren wrote:
> > > > On 10/22/2012 11:47 PM, Allen Martin wrote:
> > > > > Change usb_kbd driver to obey alignment requirements for USB DMA on
> > > > > the buffer used for data transfer.  This is necessary for
> > > > > architectures that enable dcache and enable USB DMA.
> > > > 
> > > > The series,
> > > > Tested-by: Stephen Warren <swarren@nvidia.com>
> > > > 
> > > > BTW, I tested tegra-kbc too, and that does indeed currently work (at
> > > > least in my local dev branch based on u-boot/master).
> > > 
> > > Yes, I also tried on a seaboard with internal keyboard and it works,
> > > although once the USB keyboard driver loads the internal keyboard
> > > stops working.  I haven't tracked down why, but it seems like a bug I
> > > can live with for now as seaboards with internal keyboards are pretty
> > > rare these days, and how many keyboards do you need in u-boot anyway?
> > 
> > Good thing you pointed it out. Please let's not ignore a bug. How come it
> > happens? What happens if you have two usb keyboards connected?
> 
> I'm pretty sure the USB keyboard driver doesn't support multiple
> devices, I see this in drv_usb_kbd_init():
> 
>                 /* We found a keyboard, check if it is already
>                 registered. */
>                 USB_KBD_PRINTF("USB KBD: found set up device.\n");
>                 old_dev = stdio_get_by_name(DEVNAME);
>                 if (old_dev) {
>                         /* Already registered, just return ok. */
>                         USB_KBD_PRINTF("USB KBD: is already
>                 registered.\n");
>                         return 1;
>                 }
> 
> The bug is almost certainly inside the tegra kbd driver, which is why
> I'm not terribly concerned about it.  The only boards that use that
> driver are inside NVIDIA, and even those are rare.
[...]

Good, now please fix the bug. I'm terribly unhappy seeing there is a bug that is 
about to go unfixed.

Best regards,
Marek Vasut
Allen Martin - Oct. 24, 2012, 5:24 p.m.
On Wed, Oct 24, 2012 at 12:31:40AM -0700, Marek Vasut wrote:
> Dear Allen Martin,
> 
> > On Tue, Oct 23, 2012 at 03:03:34PM -0700, Marek Vasut wrote:
> > > Dear Allen Martin,
> > > 
> > > > On Tue, Oct 23, 2012 at 09:51:06AM -0700, Stephen Warren wrote:
> > > > > On 10/22/2012 11:47 PM, Allen Martin wrote:
> > > > > > Change usb_kbd driver to obey alignment requirements for USB DMA on
> > > > > > the buffer used for data transfer.  This is necessary for
> > > > > > architectures that enable dcache and enable USB DMA.
> > > > > 
> > > > > The series,
> > > > > Tested-by: Stephen Warren <swarren@nvidia.com>
> > > > > 
> > > > > BTW, I tested tegra-kbc too, and that does indeed currently work (at
> > > > > least in my local dev branch based on u-boot/master).
> > > > 
> > > > Yes, I also tried on a seaboard with internal keyboard and it works,
> > > > although once the USB keyboard driver loads the internal keyboard
> > > > stops working.  I haven't tracked down why, but it seems like a bug I
> > > > can live with for now as seaboards with internal keyboards are pretty
> > > > rare these days, and how many keyboards do you need in u-boot anyway?
> > > 
> > > Good thing you pointed it out. Please let's not ignore a bug. How come it
> > > happens? What happens if you have two usb keyboards connected?
> > 
> > I'm pretty sure the USB keyboard driver doesn't support multiple
> > devices, I see this in drv_usb_kbd_init():
> > 
> >                 /* We found a keyboard, check if it is already
> >                 registered. */
> >                 USB_KBD_PRINTF("USB KBD: found set up device.\n");
> >                 old_dev = stdio_get_by_name(DEVNAME);
> >                 if (old_dev) {
> >                         /* Already registered, just return ok. */
> >                         USB_KBD_PRINTF("USB KBD: is already
> >                 registered.\n");
> >                         return 1;
> >                 }
> > 
> > The bug is almost certainly inside the tegra kbd driver, which is why
> > I'm not terribly concerned about it.  The only boards that use that
> > driver are inside NVIDIA, and even those are rare.
> [...]
> 
> Good, now please fix the bug. I'm terribly unhappy seeing there is a bug that is 
> about to go unfixed.

I didn't say the bug will go unfixed, I've opened an issue in our
internal bug tracker so it doesn't go forgotten.  It's just a matter
of prioritization.  It's just not important to fix a corner case bug
in a driver that noone outside of NVIDIA can actually use when there
are major functionality holes and regressions (like your change to
serial_assign() that broke serial output on tegra).  I only work on
u-boot on the side, so I have to pick my battles carefully.

-Allen
Marek Vasut - Oct. 24, 2012, 8:17 p.m.
Dear Allen Martin,

[...]

> > [...]
> > 
> > Good, now please fix the bug. I'm terribly unhappy seeing there is a bug
> > that is about to go unfixed.
> 
> I didn't say the bug will go unfixed, I've opened an issue in our
> internal bug tracker so it doesn't go forgotten.  It's just a matter
> of prioritization.

Yes, the bug is really simple to fix, so instead of arguing, please get to work. 
It could have been fixed already!

> It's just not important to fix a corner case bug

I'm sorry, I really need to be careful with wording here. I'm always baffled how 
an engineer can ignore a bug.

> in a driver that noone outside of NVIDIA can actually use when there
> are major functionality holes and regressions (like your change to
> serial_assign() that broke serial output on tegra).

Stop being personal, this hurts and doesn't help. Care to send a patch for this? 
I don't have a working tegra setup (yet), but you can test this. I already 
outlined the fix, so it's just a matter to implement it.

> I only work on
> u-boot on the side, so I have to pick my battles carefully.

I'm glad for any contribution from your end, don't be mistaken. I really 
appreciate it, sorry if my working is sometimes way too blunt or heartless.

> -Allen

Best regards,
Marek Vasut
Stephen Warren - Oct. 24, 2012, 8:41 p.m.
On 10/24/2012 02:17 PM, Marek Vasut wrote:
> Dear Allen Martin,
> 
> [...]
> 
>>> [...]
>>>
>>> Good, now please fix the bug. I'm terribly unhappy seeing there is a bug
>>> that is about to go unfixed.
>>
>> I didn't say the bug will go unfixed, I've opened an issue in our
>> internal bug tracker so it doesn't go forgotten.  It's just a matter
>> of prioritization.
> 
> Yes, the bug is really simple to fix, so instead of arguing, please get to work. 
> It could have been fixed already!

I'm sorry, but that's not a constructive response. If you want it fixed
so badly, I'm sure we'd gratefully receive a patch from you for it.

As Allen pointed out, there are currently more important issues to
concentrate on, such as Tegra not booting at all. I'm sure that once
we've made the system work at all, then we can concentrate on the minor
use-cases that simply aren't causing anyone any problems right now.

Patch

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 19f01db..57928d9 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -106,15 +106,15 @@  static const unsigned char usb_kbd_num_keypad[] = {
 	(USB_KBD_NUMLOCK | USB_KBD_CAPSLOCK | USB_KBD_SCROLLLOCK)
 
 struct usb_kbd_pdata {
+	uint8_t		new[8];
+	uint8_t		old[8];
+
 	uint32_t	repeat_delay;
 
 	uint32_t	usb_in_pointer;
 	uint32_t	usb_out_pointer;
 	uint8_t		usb_kbd_buffer[USB_KBD_BUFFER_LEN];
 
-	uint8_t		new[8];
-	uint8_t		old[8];
-
 	uint8_t		flags;
 };
 
@@ -426,7 +426,7 @@  static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 
 	USB_KBD_PRINTF("USB KBD: found set protocol...\n");
 
-	data = malloc(sizeof(struct usb_kbd_pdata));
+	data = memalign(USB_DMA_MINALIGN, sizeof(struct usb_kbd_pdata));
 	if (!data) {
 		printf("USB KBD: Error allocating private data\n");
 		return 0;