[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[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.

[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;
-- 
1.7.9.5

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[Marek Vasut]

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);
>  	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;

Best regards,
Marek Vasut

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[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.

Regards, Ilya.

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[Marek Vasut]

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

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[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"

Regards, Ilya.

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[Marek Vasut]

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

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[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?


> 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.

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[Marek Vasut]

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

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[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.

Regards, Ilya.

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[Marek Vasut]

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

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[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.

Regards, Ilya.

[U-Boot] [PATCH] smsc95xx: align buffers to cache line size

[Marek Vasut]

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