[U-Boot] [PATCH] smsc95xx: align buffers to cache line size
Align buffers passed to the USB code to cache line size so they can be DMAed safely.
Signed-off-by: Ilya Yanok ilya.yanok@cogentembedded.com --- Depens on Marek's patch [1] for DEFINE_CACHE_ALIGN_BUFFER.
[1] http://patchwork.ozlabs.org/patch/169619/
drivers/usb/eth/smsc95xx.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c index c7aebea..c62a8c1 100644 --- a/drivers/usb/eth/smsc95xx.c +++ b/drivers/usb/eth/smsc95xx.c @@ -153,13 +153,15 @@ static int curr_eth_dev; /* index for name of next device detected */ static int smsc95xx_write_reg(struct ueth_data *dev, u32 index, u32 data) { int len; + ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1);
cpu_to_le32s(&data); + tmpbuf[0] = data;
len = usb_control_msg(dev->pusb_dev, usb_sndctrlpipe(dev->pusb_dev, 0), USB_VENDOR_REQUEST_WRITE_REGISTER, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, - 00, index, &data, sizeof(data), USB_CTRL_SET_TIMEOUT); + 00, index, tmpbuf, sizeof(data), USB_CTRL_SET_TIMEOUT); if (len != sizeof(data)) { debug("smsc95xx_write_reg failed: index=%d, data=%d, len=%d", index, data, len); @@ -171,11 +173,13 @@ static int smsc95xx_write_reg(struct ueth_data *dev, u32 index, u32 data) static int smsc95xx_read_reg(struct ueth_data *dev, u32 index, u32 *data) { int len; + ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1);
len = usb_control_msg(dev->pusb_dev, usb_rcvctrlpipe(dev->pusb_dev, 0), USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, - 00, index, data, sizeof(data), USB_CTRL_GET_TIMEOUT); + 00, index, tmpbuf, sizeof(data), USB_CTRL_GET_TIMEOUT); + *data = tmpbuf[0]; if (len != sizeof(data)) { debug("smsc95xx_read_reg failed: index=%d, len=%d", index, len); @@ -664,7 +668,8 @@ static int smsc95xx_send(struct eth_device *eth, void* packet, int length) int actual_len; u32 tx_cmd_a; u32 tx_cmd_b; - unsigned char msg[PKTSIZE + sizeof(tx_cmd_a) + sizeof(tx_cmd_b)]; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, msg, + PKTSIZE + sizeof(tx_cmd_a) + sizeof(tx_cmd_b));
debug("** %s(), len %d, buf %#x\n", __func__, length, (int)msg); if (length > PKTSIZE) @@ -695,7 +700,7 @@ static int smsc95xx_send(struct eth_device *eth, void* packet, int length) static int smsc95xx_recv(struct eth_device *eth) { struct ueth_data *dev = (struct ueth_data *)eth->priv; - static unsigned char recv_buf[AX_RX_URB_SIZE]; + DEFINE_CACHE_ALIGN_BUFFER(unsigned char, recv_buf, AX_RX_URB_SIZE); unsigned char *buf_ptr; int err; int actual_len;
Dear Ilya Yanok,
Align buffers passed to the USB code to cache line size so they can be DMAed safely.
Signed-off-by: Ilya Yanok ilya.yanok@cogentembedded.com
Depens on Marek's patch [1] for DEFINE_CACHE_ALIGN_BUFFER.
Isn't it your patch anyway? You should get the credit for that series, you know ...
btw. this will fail with cache line < 32 .
[1] http://patchwork.ozlabs.org/patch/169619/
drivers/usb/eth/smsc95xx.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c index c7aebea..c62a8c1 100644 --- a/drivers/usb/eth/smsc95xx.c +++ b/drivers/usb/eth/smsc95xx.c @@ -153,13 +153,15 @@ static int curr_eth_dev; /* index for name of next device detected */ static int smsc95xx_write_reg(struct ueth_data *dev, u32 index, u32 data) { int len;
ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1);
cpu_to_le32s(&data);
tmpbuf[0] = data;
len = usb_control_msg(dev->pusb_dev, usb_sndctrlpipe(dev->pusb_dev, 0), USB_VENDOR_REQUEST_WRITE_REGISTER, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
00, index, &data, sizeof(data), USB_CTRL_SET_TIMEOUT);
00, index, tmpbuf, sizeof(data), USB_CTRL_SET_TIMEOUT);@@ -171,11 +173,13 @@ static int smsc95xx_write_reg(struct ueth_data *dev, u32 index, u32 data) static int smsc95xx_read_reg(struct ueth_data *dev, u32 index, u32 *data) { int len;
ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1);
len = usb_control_msg(dev->pusb_dev, usb_rcvctrlpipe(dev->pusb_dev, 0), USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
00, index, data, sizeof(data), USB_CTRL_GET_TIMEOUT);
00, index, tmpbuf, sizeof(data), USB_CTRL_GET_TIMEOUT);- *data = tmpbuf[0]; if (len != sizeof(data)) { debug("smsc95xx_read_reg failed: index=%d, len=%d", index, len);
@@ -664,7 +668,8 @@ static int smsc95xx_send(struct eth_device *eth, void* packet, int length) int actual_len; u32 tx_cmd_a; u32 tx_cmd_b;
- unsigned char msg[PKTSIZE + sizeof(tx_cmd_a) + sizeof(tx_cmd_b)];
ALLOC_CACHE_ALIGN_BUFFER(unsigned char, msg,
PKTSIZE + sizeof(tx_cmd_a) + sizeof(tx_cmd_b));debug("** %s(), len %d, buf %#x\n", __func__, length, (int)msg); if (length > PKTSIZE)
@@ -695,7 +700,7 @@ static int smsc95xx_send(struct eth_device *eth, void* packet, int length) static int smsc95xx_recv(struct eth_device *eth) { struct ueth_data *dev = (struct ueth_data *)eth->priv;
- static unsigned char recv_buf[AX_RX_URB_SIZE];
- DEFINE_CACHE_ALIGN_BUFFER(unsigned char, recv_buf, AX_RX_URB_SIZE); unsigned char *buf_ptr; int err; int actual_len;
Best regards, Marek Vasut
Dear Marek,
On Sun, Jul 8, 2012 at 10:59 PM, Marek Vasut marek.vasut@gmail.com wrote:
btw. this will fail with cache line < 32 .
Hm.. I have to admit I'm not very much into USB specs and I don't have any non-ARMv7 system now to do some testing... But it used to work without any alignment, right? (with disabled dcache, of course) That makes me think that data buffers don't need any alignment (from USB pov, not cache) and 32-byte alignment is required for internal structs only.
Regards, Ilya.
Dear Ilya Yanok,
Dear Marek,
On Sun, Jul 8, 2012 at 10:59 PM, Marek Vasut marek.vasut@gmail.com wrote:
btw. this will fail with cache line < 32 .
Hm.. I have to admit I'm not very much into USB specs and I don't have any non-ARMv7 system now to do some testing... But it used to work without any alignment, right? (with disabled dcache, of course) That makes me think that data buffers don't need any alignment (from USB pov, not cache) and 32-byte alignment is required for internal structs only.
See ehci-r10.pdf ... chapter 3.5 ... the buffer pointer has to be aligned too it seems.
Regards, Ilya.
Best regards, Marek Vasut
Dear Marek,
On Mon, Jul 9, 2012 at 1:31 AM, Marek Vasut marek.vasut@gmail.com wrote:
non-ARMv7 system now to do some testing... But it used to work without any alignment, right? (with disabled dcache,
of
course) That makes me think that data buffers don't need any alignment (from USB pov, not cache) and 32-byte alignment is required for internal structs only.
Hm.. I have to admit I'm not very much into USB specs and I don't have
any See ehci-r10.pdf ... chapter 3.5 ... the buffer pointer has to be aligned too it seems.
But in practice it works without any alignment... ok, you made me read the spec ;) page 55: "For the page 0 current offset interpretation, this field is the byte offset into the current page"
Regards, Ilya.
Dear Ilya Yanok,
Dear Marek,
On Mon, Jul 9, 2012 at 1:31 AM, Marek Vasut marek.vasut@gmail.com wrote:
non-ARMv7 system now to do some testing... But it used to work without any alignment, right? (with disabled dcache,
of
course) That makes me think that data buffers don't need any alignment (from USB pov, not cache) and 32-byte alignment is required for internal structs only.
Hm.. I have to admit I'm not very much into USB specs and I don't have
any See ehci-r10.pdf ... chapter 3.5 ... the buffer pointer has to be aligned too it seems.
But in practice it works without any alignment... ok, you made me read the spec ;) page 55: "For the page 0 current offset interpretation, this field is the byte offset into the current page"
See the thread "[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size"
So, if we compute the size over there correctly (I hope Stefan will send an updated patch soon), we'll still need it to be aligned to ARCH_DMA_MINALIGN so we can flush it. Bah, this is getting quite crazy, the deeper we go, the more bugs we meet!
So, what I'd like to do is to see a patch from Stefan, it was really a good finding! Next up, we should finish this patchset for proper EHCI QH and qTD alignment. And finally, we need generic bounce buffer to use as a protection against crazy people who might want to load stuff to unaligned address.
Regards, Ilya.
Best regards, Marek Vasut
Dear Marek,
On Mon, Jul 9, 2012 at 4:45 AM, Marek Vasut marek.vasut@gmail.com wrote:
But in practice it works without any alignment... ok, you made me read
the
spec ;) page 55: "For the page 0 current offset interpretation, this field is the byte offset into the current page"
See the thread "[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size"
Interesting... but that's a different issue...
So, if we compute the size over there correctly (I hope Stefan will send an updated patch soon), we'll still need it to be aligned to ARCH_DMA_MINALIGN so we can flush it. Bah, this is getting quite crazy, the deeper we go, the more bugs we meet!
Well, of course we need proper alignment for cache stuff (well, actually we can skip this alignment thing for the buffer we will flush as long as all buffers we are going to invalidate are properly aligned/sized... but that's too tricky, personally I'd prefer every DMAed buffer to be cache-line aligned/sized).
And this patch actually adds the alignment for the smsc95xx driver's buffers. In your initial reply you said it will be broken on systems with ARCH_DMA_MINALIGN < 32, so I'm asking what makes you think so?
So, what I'd like to do is to see a patch from Stefan, it was really a good finding! Next up, we should finish this patchset for proper EHCI QH and qTD alignment. And finally, we need generic bounce buffer to use as a protection against crazy people who might want to load stuff to unaligned address.
Good plan.
Regards, Ilya.
Dear Ilya Yanok,
Dear Marek,
On Mon, Jul 9, 2012 at 4:45 AM, Marek Vasut marek.vasut@gmail.com wrote:
But in practice it works without any alignment... ok, you made me read
the
spec ;) page 55: "For the page 0 current offset interpretation, this field is the byte offset into the current page"
See the thread "[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size"
Interesting... but that's a different issue...
So, if we compute the size over there correctly (I hope Stefan will send an updated patch soon), we'll still need it to be aligned to ARCH_DMA_MINALIGN so we can flush it. Bah, this is getting quite crazy, the deeper we go, the more bugs we meet!
Well, of course we need proper alignment for cache stuff (well, actually we can skip this alignment thing for the buffer we will flush as long as all buffers we are going to invalidate are properly aligned/sized... but that's too tricky, personally I'd prefer every DMAed buffer to be cache-line aligned/sized).
And this patch actually adds the alignment for the smsc95xx driver's buffers. In your initial reply you said it will be broken on systems with ARCH_DMA_MINALIGN < 32, so I'm asking what makes you think so?
ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be aligned to 16 bytes for arch with 16 byte cachelines.
So, what I'd like to do is to see a patch from Stefan, it was really a good finding! Next up, we should finish this patchset for proper EHCI QH and qTD alignment. And finally, we need generic bounce buffer to use as a protection against crazy people who might want to load stuff to unaligned address.
Good plan.
Regards, Ilya.
Best regards, Marek Vasut
Hi Marek,
On Tue, Jul 10, 2012 at 6:17 AM, Marek Vasut marek.vasut@gmail.com wrote:
Well, of course we need proper alignment for cache stuff (well, actually
we
can skip this alignment thing for the buffer we will flush as long as all buffers we are going to invalidate are properly aligned/sized... but
that's
too tricky, personally I'd prefer every DMAed buffer to be cache-line aligned/sized).
And this patch actually adds the alignment for the smsc95xx driver's buffers. In your initial reply you said it will be broken on systems with ARCH_DMA_MINALIGN < 32, so I'm asking what makes you think so?
ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be aligned to 16 bytes for arch with 16 byte cachelines.
Yes, and this is exactly what we need.
Regards, Ilya.
Dear Ilya Yanok,
Hi Marek,
On Tue, Jul 10, 2012 at 6:17 AM, Marek Vasut marek.vasut@gmail.com wrote:
Well, of course we need proper alignment for cache stuff (well, actually
we
can skip this alignment thing for the buffer we will flush as long as all buffers we are going to invalidate are properly aligned/sized... but
that's
too tricky, personally I'd prefer every DMAed buffer to be cache-line aligned/sized).
And this patch actually adds the alignment for the smsc95xx driver's buffers. In your initial reply you said it will be broken on systems with ARCH_DMA_MINALIGN < 32, so I'm asking what makes you think so?
ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be aligned to 16 bytes for arch with 16 byte cachelines.
Yes, and this is exactly what we need.
It isn't, EHCI needs it aligned on 32 byte boundary.
Regards, Ilya.
Best regards, Marek Vasut
On Tue, Jul 10, 2012 at 1:34 PM, Marek Vasut marek.vasut@gmail.com wrote:
aligned to 16 bytes for arch with 16 byte cachelines.
Yes, and this is exactly what we need.
ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be
It isn't, EHCI needs it aligned on 32 byte boundary.
It doesn't. I posted the page number with data buffer description in EHCI spec upper in this thread. There is no alignment requirement at all. Controller is fine with any address. _We_ need the buffer to be cache line aligned to perform cache operations safely.
Regards, Ilya.
Dear Ilya Yanok,
On Tue, Jul 10, 2012 at 1:34 PM, Marek Vasut marek.vasut@gmail.com wrote:
aligned to 16 bytes for arch with 16 byte cachelines.
Yes, and this is exactly what we need.
ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be
It isn't, EHCI needs it aligned on 32 byte boundary.
It doesn't. I posted the page number with data buffer description in EHCI spec upper in this thread. There is no alignment requirement at all. Controller is fine with any address. _We_ need the buffer to be cache line aligned to perform cache operations safely.
You're right ... I'm starting to get this mixed with DMA. DMA, USB, caches ... it's starting to melt into one big goo in my head. Sorry.
Regards, Ilya.
Best regards, Marek Vasut
participants (2)
-
Ilya Yanok -
Marek Vasut