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

login
register
mail settings
Submitter Allen Martin
Date Oct. 23, 2012, 9:37 p.m.
Message ID <1351028274-30301-1-git-send-email-amartin@nvidia.com>
Download mbox | patch
Permalink /patch/193578/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Allen Martin - Oct. 23, 2012, 9:37 p.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>
Tested-by: Stephen Warren <swarren@nvidia.com>
---
v3: add comment about alignemnt requirement
v2: use memalign instead of __align()
---
 common/usb_kbd.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Stephen Warren - Oct. 23, 2012, 9:51 p.m.
On 10/23/2012 03:37 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.

> diff --git a/common/usb_kbd.c b/common/usb_kbd.c

> +/*
> + * This structure must be aligned to USB_DMA_MINALIGN to allow DMA to
> + * buffer "new" below.
> + */
>  struct usb_kbd_pdata {
> +	uint8_t		new[8];
> +	uint8_t		old[8];

Oh, one more thought on this: Those fields should both be aligned, and
their size be aligned too, at least to cache size. In particular, if we
have HW write to new[], then invalidate the cache that contains new[]
because it just did, we want to make sure that fields after new[] (i.e.
old[], ...) don't get invalidate too, in case the cache contained stale
data for those fields. That's one of the things that the (stack-based)
ALLOC_CACHE_ALIGN_BUFFER handles. Perhaps make those two fields
pointers, and point those at a memalign()-allocated blob, where the size
of those blobs are something like ROUND_UP(USB_DMA_MINALIGN, 8)?

Sorry for forgetting about this before!
Allen Martin - Oct. 23, 2012, 9:58 p.m.
On Tue, Oct 23, 2012 at 02:51:53PM -0700, Stephen Warren wrote:
> On 10/23/2012 03:37 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.
> 
> > diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> 
> > +/*
> > + * This structure must be aligned to USB_DMA_MINALIGN to allow DMA to
> > + * buffer "new" below.
> > + */
> >  struct usb_kbd_pdata {
> > +	uint8_t		new[8];
> > +	uint8_t		old[8];
> 
> Oh, one more thought on this: Those fields should both be aligned, and
> their size be aligned too, at least to cache size. In particular, if we
> have HW write to new[], then invalidate the cache that contains new[]
> because it just did, we want to make sure that fields after new[] (i.e.
> old[], ...) don't get invalidate too, in case the cache contained stale
> data for those fields. That's one of the things that the (stack-based)
> ALLOC_CACHE_ALIGN_BUFFER handles. Perhaps make those two fields
> pointers, and point those at a memalign()-allocated blob, where the size
> of those blobs are something like ROUND_UP(USB_DMA_MINALIGN, 8)?
> 
> Sorry for forgetting about this before!

That's a good point.  We also don't want to touch old[] from the CPU
right after we just flushed new[] and dirty the cacheline again.  It
does seem like these should be dynamically allocated, then I can avoid
the memalign() on the whole structure and don't have to worry if a new
member gets added in front of new[] later.

-Allen

Patch

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 19f01db..be5e8bc 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -105,16 +105,20 @@  static const unsigned char usb_kbd_num_keypad[] = {
 #define USB_KBD_LEDMASK		\
 	(USB_KBD_NUMLOCK | USB_KBD_CAPSLOCK | USB_KBD_SCROLLLOCK)
 
+/*
+ * This structure must be aligned to USB_DMA_MINALIGN to allow DMA to
+ * buffer "new" below.
+ */
 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 +430,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;