| Message ID | 1351028274-30301-1-git-send-email-amartin@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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!
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
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;