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

[Marek Vasut <marex@denx.de>]

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