From: Marek Vasut <marex@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v3 1/2] usb: align buffers at cacheline
Date: Fri, 2 Mar 2012 13:58:03 +0100 [thread overview]
Message-ID: <201203021358.04685.marex@denx.de> (raw)
In-Reply-To: <4F50C217.90101@nvidia.com>
> Hi,
>
> On Friday 02 March 2012 04:13 PM, Marek Vasut wrote:
> >> Hi,
> >>
> >> On Friday 02 March 2012 12:08 AM, Marek Vasut wrote:
> >>>> On Thursday 01 March 2012 03:05 AM, Marek Vasut wrote:
> >>>>>> As DMA expects the buffers to be equal and larger then
> >>>>>> cache lines, This aligns buffers at cacheline.
> >>>>>>
> >>>>>> Signed-off-by: Puneet Saxena<puneets@nvidia.com>
> >>>>>> ---
> >>>>>>
> >>>>>> Changes for V2:
> >>>>>> - Use "ARCH_DMA_MINALIGN" directly
> >>>>>> - Use "ALIGN" to align size as cacheline
> >>>>>> - Removed headers from usb.h
> >>>>>> - Send 8 bytes of device descriptor size to read
> >>>>>>
> >>>>>> Max packet size
> >>>>>>
> >>>>>> scsi.h header is needed to avoid extra memcpy from local
> >>>>>> buffer to global buffer.
> >>>>>>
> >>>>>> Changes for V3:
> >>>>>> - Removed local descriptor elements copy to global descriptor
> >>>>>> elements - Removed "Signed-off-by: Jim Lin<jilin@nvidia.com>"
> >>>>>> from commit
> >>>>>>
> >>>>>> message
> >>>>>>
> >>>>>> common/cmd_usb.c | 3 +-
> >>>>>> common/usb.c | 57
> >>>>>>
> >>>>>> ++++++++++++++++++++++------------------- common/usb_storage.c
> >>>>>>
> >>>>>> | 59 ++++++++++++++++++++---------------------- disk/part_dos.c
> >>>>>> |
> >>>>>> | 2 +-
> >>>>>>
> >>>>>> drivers/usb/host/ehci-hcd.c | 8 ++++++
> >>>>>> include/scsi.h | 4 ++-
> >>>>>> 6 files changed, 73 insertions(+), 60 deletions(-)
> >>>>>>
> >>>>>> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
> >>>>>> index 320667f..bca9d94 100644
> >>>>>> --- a/common/cmd_usb.c
> >>>>>> +++ b/common/cmd_usb.c
> >>>>>> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass,
> >>>>>> unsigned char subclass,
> >>>>>>
> >>>>>> void usb_display_string(struct usb_device *dev, int index)
> >>>>>> {
> >>>>>>
> >>>>>> - char buffer[256];
> >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256);
> >>>>>> +
> >>>>>>
> >>>>>> if (index != 0) {
> >>>>>>
> >>>>>> if (usb_string(dev, index,&buffer[0], 256)> 0)
> >>>>>>
> >>>>>> printf("String: \"%s\"", buffer);
> >>>>>>
> >>>>>> diff --git a/common/usb.c b/common/usb.c
> >>>>>> index 63a11c8..191bc5b 100644
> >>>>>> --- a/common/usb.c
> >>>>>> +++ b/common/usb.c
> >>>>>> @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE];
> >>>>>>
> >>>>>> static int dev_index;
> >>>>>> static int running;
> >>>>>> static int asynch_allowed;
> >>>>>>
> >>>>>> -static struct devrequest setup_packet;
> >>>>>>
> >>>>>> char usb_started; /* flag for the started/stopped USB status */
> >>>>>>
> >>>>>> @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev,
> >>>>>> unsigned int pipe, unsigned short value, unsigned short index,
> >>>>>>
> >>>>>> void *data, unsigned short size, int timeout)
> >>>>>>
> >>>>>> {
> >>>>>>
> >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet,
> >>>>>> + sizeof(struct devrequest));
> >>>>>>
> >>>>>> if ((timeout == 0)&& (!asynch_allowed)) {
> >>>>>>
> >>>>>> /* request for a asynch control pipe is not allowed */
> >>>>>> return -1;
> >>>>>>
> >>>>>> }
> >>>>>>
> >>>>>> /* set setup command */
> >>>>>>
> >>>>>> - setup_packet.requesttype = requesttype;
> >>>>>> - setup_packet.request = request;
> >>>>>> - setup_packet.value = cpu_to_le16(value);
> >>>>>> - setup_packet.index = cpu_to_le16(index);
> >>>>>> - setup_packet.length = cpu_to_le16(size);
> >>>>>> + setup_packet->requesttype = requesttype;
> >>>>>> + setup_packet->request = request;
> >>>>>> + setup_packet->value = cpu_to_le16(value);
> >>>>>> + setup_packet->index = cpu_to_le16(index);
> >>>>>> + setup_packet->length = cpu_to_le16(size);
> >>>>>>
> >>>>>> USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X,
> >>>>>> "
> >
> > \
> >
> >>>>>> "value 0x%X index 0x%X length 0x%X\n",
> >>>>>>
> >>>>>> request, requesttype, value, index, size);
> >>>>>>
> >>>>>> dev->status = USB_ST_NOT_PROC; /*not yet processed */
> >>>>>>
> >>>>>> - submit_control_msg(dev, pipe, data, size,&setup_packet);
> >>>>>> + submit_control_msg(dev, pipe, data, size, setup_packet);
> >>>>>>
> >>>>>> if (timeout == 0)
> >>>>>>
> >>>>>> return (int)size;
> >>>>>>
> >>>>>> @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device
> >>>>>> *dev, unsigned int langid, */
> >>>>>>
> >>>>>> int usb_string(struct usb_device *dev, int index, char *buf,
> >>>>>> size_t size) {
> >>>>>>
> >>>>>> - unsigned char mybuf[USB_BUFSIZ];
> >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ);
> >>>>>>
> >>>>>> unsigned char *tbuf;
> >>>>>> int err;
> >>>>>> unsigned int u, idx;
> >>>>>>
> >>>>>> @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev)
> >>>>>>
> >>>>>> {
> >>>>>>
> >>>>>> int addr, err;
> >>>>>> int tmp;
> >>>>>>
> >>>>>> - unsigned char tmpbuf[USB_BUFSIZ];
> >>>>>> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
> >>>>>>
> >>>>>> /* We still haven't set the Address yet */
> >>>>>> addr = dev->devnum;
> >>>>>>
> >>>>>> @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev)
> >>>>>>
> >>>>>> dev->epmaxpacketin[0] = 64;
> >>>>>> dev->epmaxpacketout[0] = 64;
> >>>>>>
> >>>>>> - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
> >>>>>> + desc->bMaxPacketSize0 = 0;
> >>>>>> + /*8 bytes of the descriptor to read Max packet size*/
> >>>>>> + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc,
> >>>>>> + 8);
> >>>>>
> >>>>> Did some unrelated addition/change creep in here?
> >>>>
> >>>> No, This is the fix for the similar issue as is discussed for string
> >>>> fetch().
> >>>> When the device partially fills the passed buffer and we try to
> >>>> invalidate the partial buffer
> >>>> the cache alignment error crops up.
> >>>>
> >>>> The code in "ehci_submit_async() " invalidates *partially* the passed
> >>>> buffer though we pass aligned buffer after "STD_ASS"
> >>>> is received. Actually it should invalidate only the cached line which
> >>>> is equal(~32) to device desc length.
> >>>>
> >>>> If we pass actual device desc length the cache alignment error does
> >>>> not spew.
> >>>> Note that here we are aligning the buffer still the error comes.
> >>>
> >>> Then please send this fix as a separate patch. And I think ehci_hcd is
> >>> what should be fixed then as I said in the other email, or am I wrong?
> >>
> >> Yes, I will send this fix in separate patch. To address partial
> >> invalidate issue
> >> will send another patch in "ehci_hcd()".
> >
> > Very good! I'm really glad we're working towards the proper solution,
> > thanks for all the effort you put into it!
> >
> >>>>>> if (err< 0) {
> >>>>>>
> >>>>>> USB_PRINTF("usb_new_device: usb_get_descriptor()
> >
> > failed\n");
> >
> >>>>>> return 1;
> >>>>>>
> >>>>>> @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev)
> >>>>>>
> >>>>>> tmp = sizeof(dev->descriptor);
> >>>>>>
> >>>>>> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
> >>>>>>
> >>>>>> - &dev->descriptor, sizeof(dev-
> >>
> >> descriptor));
> >>
> >>>>>> + desc, sizeof(dev->descriptor));
> >>>>>
> >>>>> Won't this change (desc) break anything?
> >>>>
> >>>> Its not breaking any thing. For safer side we could add memcpy to copy
> >>>> from local desc
> >>>> to global desc. What you say?
> >>>
> >>> What do you mean? So you changed the use from some global variable to
> >>> different (local) variable? This might break stuff I fear :-(
> >>
> >> Actually in previous comments it was said to not cache align "struct
> >> usb_device_descriptor descriptor; /* Device Descriptor */ Line:112
> >> usb.h, in header file. So another way is to define cache aligned local
> >> variable and pass it to "usb_get_descriptor". After returning from
> >> "usb_get_descriptor", memcpy this buffer to global variable
> >> "dev->descriptor".
> >> I verified the devices, usb mouse, mass-storage etc... that without this
> >> memcpy to global variable, its not breaking anything.
> >> That's why I avoided memcpy.
> >
> > Oh ... maybe the global variable is unneeded at all and using the local
> > only is OK?
>
> Investigated further and found that "memcpy" is needed to configure Usb
> version, vendor id, prod Id ..etc
> of the device. Its also useful to hook actual device driver and detect
> no. of configuration supported.
>
> I strongly feel we could avoid "memcpy" by making global device desc in
> usb.h Line:112, cache align as done in
> scsi.h,Line:30 in this patch and this is accepted also?
We try to make as many things local as possible.
M
next prev parent reply other threads:[~2012-03-02 12:58 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-17 10:50 [U-Boot] [PATCH] usb: align buffers at cache boundary Puneet Saxena
2012-02-17 17:23 ` Mike Frysinger
2012-02-23 14:25 ` [U-Boot] [PATCH 1/2] usb: align buffers at cacheline Puneet Saxena
2012-02-23 18:15 ` Mike Frysinger
2012-02-24 11:27 ` puneets
2012-02-24 12:42 ` Simon Glass
2012-02-27 15:36 ` [U-Boot] [PATCH v2 " Puneet Saxena
2012-02-27 16:49 ` Marek Vasut
2012-02-27 17:03 ` Simon Glass
2012-02-27 17:11 ` Marek Vasut
2012-02-27 17:27 ` Simon Glass
2012-02-29 14:21 ` [U-Boot] [PATCH v3 " Puneet Saxena
2012-02-29 21:35 ` Marek Vasut
2012-03-01 13:51 ` puneets
2012-03-01 18:38 ` Marek Vasut
2012-03-02 6:56 ` puneets
2012-03-02 10:43 ` Marek Vasut
2012-03-02 12:50 ` puneets
2012-03-02 12:58 ` Marek Vasut [this message]
2012-03-02 13:35 ` [U-Boot] [PATCH v4] " Puneet Saxena
2012-03-02 13:46 ` Marek Vasut
2012-03-02 14:00 ` puneets
2012-03-02 14:41 ` Marek Vasut
2012-03-02 15:21 ` puneets
2012-03-02 15:59 ` Marek Vasut
2012-03-02 16:45 ` Wolfgang Denk
2012-03-05 7:16 ` [U-Boot] [PATCH v5] " Puneet Saxena
2012-03-05 13:24 ` Eric Nelson
2012-03-05 13:35 ` Marek Vasut
2012-03-05 14:46 ` [U-Boot] [PATCH v8] " Puneet Saxena
2012-03-05 15:35 ` Marek Vasut
2012-03-05 18:18 ` Simon Glass
2012-03-06 0:36 ` Marek Vasut
2012-03-06 0:39 ` Marek Vasut
2012-03-06 7:00 ` puneets
2012-03-06 8:22 ` Marek Vasut
2012-03-06 3:07 ` Mike Frysinger
2012-03-07 7:12 ` puneets
2012-03-07 9:20 ` puneets
2012-03-07 22:06 ` Marek Vasut
2012-03-08 11:21 ` puneets
2012-03-08 14:12 ` Marek Vasut
2012-03-09 6:15 ` puneets
2012-03-09 12:03 ` Marek Vasut
2012-03-11 2:35 ` Mike Frysinger
2012-03-14 2:05 ` Marek Vasut
2012-03-16 4:39 ` Marek Vasut
2012-03-16 7:42 ` puneets
2012-03-16 8:52 ` Marek Vasut
2012-03-19 14:29 ` puneets
2012-03-19 14:43 ` Marek Vasut
2012-03-19 15:19 ` Tom Warren
2012-03-19 15:46 ` Marek Vasut
2012-04-02 15:59 ` Tom Warren
2012-04-02 16:11 ` Marek Vasut
2012-04-02 16:16 ` Tom Warren
2012-04-02 16:32 ` Marek Vasut
2012-04-03 6:05 ` puneets
2012-03-05 7:27 ` [U-Boot] [PATCH v6] " Puneet Saxena
2012-03-05 12:03 ` Marek Vasut
2012-03-05 12:21 ` [U-Boot] [PATCH v7] " Puneet Saxena
2012-03-05 12:41 ` Marek Vasut
2012-03-06 3:28 ` [U-Boot] [PATCH v4] " Mike Frysinger
2012-03-06 8:24 ` Marek Vasut
2012-03-06 16:42 ` Mike Frysinger
2012-02-29 14:21 ` [U-Boot] [PATCH v3 2/2] usb: Add CONFIG to fetch string descriptor Puneet Saxena
2012-02-29 21:29 ` Marek Vasut
2012-03-01 11:07 ` puneets
2012-03-01 11:45 ` Marek Vasut
2012-03-01 12:59 ` puneets
2012-03-01 13:13 ` Marek Vasut
2012-03-05 12:48 ` Marek Vasut
2012-03-05 13:14 ` puneets
2012-03-05 21:15 ` Marek Vasut
2012-02-28 9:34 ` [U-Boot] [PATCH v2 1/2] usb: align buffers at cacheline puneets
2012-02-29 21:38 ` Marek Vasut
2012-02-27 15:36 ` [U-Boot] [PATCH v2 2/2] usb: Add CONFIG to fetch string descriptor Puneet Saxena
2012-02-27 18:28 ` Mike Frysinger
2012-02-27 15:37 ` [U-Boot] [PATCH 1/2] usb: align buffers at cacheline puneets
2012-02-23 14:25 ` [U-Boot] [PATCH 2/2] usb: Add quirk "USB_QUIRK_STRING_FETCH_255" Puneet Saxena
2012-02-23 15:20 ` Tom Rini
2012-02-23 16:04 ` Tom Warren
2012-02-24 7:52 ` puneets
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201203021358.04685.marex@denx.de \
--to=marex@denx.de \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox