diff mbox

[U-Boot,1/4] net: sun8i_emac: Fix DMA alignment issues with the rx / tx buffers

Message ID 1469635835-2063-1-git-send-email-hdegoede@redhat.com
State Superseded
Delegated to: Hans de Goede
Headers show

Commit Message

Hans de Goede July 27, 2016, 4:10 p.m. UTC
This fixes the following CACHE warnings when using sun8i_emac:

=> dhcp
BOOTP broadcast 1
BOOTP broadcast 2
CACHE: Misaligned operation at range [7bf594a8, 7bf59628]
BOOTP broadcast 3
CACHE: Misaligned operation at range [7bf59c90, 7bf59e10]
CACHE: Misaligned operation at range [7bf5a478, 7bf5a5f8]
DHCP client bound to address 10.42.43.80 (1009 ms)

Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
Cc: Amit Singh Tomar <amittomer25@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/net/sun8i_emac.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Amit Tomer July 27, 2016, 5:11 p.m. UTC | #1
Hello,

> index 7c088c3..877859c 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -32,7 +32,8 @@
>
>  #define CONFIG_TX_DESCR_NUM    32
>  #define CONFIG_RX_DESCR_NUM    32
> -#define CONFIG_ETH_BUFSIZE     2024
> +#define CONFIG_ETH_BUFSIZE     2048
> +#define CONFIG_ETH_RXSIZE      2024 /* Note most fit in ETH_BUFSIZE */

As per the following comment made in Linux driver.

/* The datasheet said that each descriptor can transfers up to 4096bytes
+ * But latter, a register documentation reduce that value to 2048
+ * Anyway using 2048 cause strange behaviours and even BSP driver use 2047
+ */

Can we keep CONFIG_ETH_BUFSIZE to 2044 ?

Thanks,
Amit.
Ian Campbell July 27, 2016, 6:20 p.m. UTC | #2
On Wed, 2016-07-27 at 18:10 +0200, Hans de Goede wrote:
> This fixes the following CACHE warnings when using sun8i_emac:
> 
> => dhcp
> BOOTP broadcast 1
> BOOTP broadcast 2
> CACHE: Misaligned operation at range [7bf594a8, 7bf59628]
> BOOTP broadcast 3
> CACHE: Misaligned operation at range [7bf59c90, 7bf59e10]
> CACHE: Misaligned operation at range [7bf5a478, 7bf5a5f8]
> DHCP client bound to address 10.42.43.80 (1009 ms)
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/net/sun8i_emac.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> index 7c088c3..877859c 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -32,7 +32,8 @@
>  
>  #define CONFIG_TX_DESCR_NUM	32
>  #define CONFIG_RX_DESCR_NUM	32
> -#define CONFIG_ETH_BUFSIZE	2024
> +#define CONFIG_ETH_BUFSIZE	2048
> +#define CONFIG_ETH_RXSIZE	2024 /* Note most fit in ETH_BUFSIZE */

s/most/must/?

A comment (perhaps in the commit message rather than the code) as to
how/why RXSIZE and BUFSIZE interact to affect the alignment in the
desired fasion would be useful, since it is non-obvious to me at
least. 

I was about to speculate on the difference of 14 bytes relating to the
Ethernet frame header, but then I realised it's 24 not 14 and deleted
those paragraphs, which I think underscores the need for a comment ;-)

Ian.
Hans de Goede July 28, 2016, 6:37 p.m. UTC | #3
Hi,

On 27-07-16 19:11, Amit Tomer wrote:
> Hello,
>
>> index 7c088c3..877859c 100644
>> --- a/drivers/net/sun8i_emac.c
>> +++ b/drivers/net/sun8i_emac.c
>> @@ -32,7 +32,8 @@
>>
>>  #define CONFIG_TX_DESCR_NUM    32
>>  #define CONFIG_RX_DESCR_NUM    32
>> -#define CONFIG_ETH_BUFSIZE     2024
>> +#define CONFIG_ETH_BUFSIZE     2048
>> +#define CONFIG_ETH_RXSIZE      2024 /* Note most fit in ETH_BUFSIZE */
>
> As per the following comment made in Linux driver.
>
> /* The datasheet said that each descriptor can transfers up to 4096bytes
> + * But latter, a register documentation reduce that value to 2048
> + * Anyway using 2048 cause strange behaviours and even BSP driver use 2047
> + */
>
> Can we keep CONFIG_ETH_BUFSIZE to 2044 ?

Note that my patch is introducing a CONFIG_ETH_RXSIZE and is using that
where ever CONFIG_ETH_BUFSIZE is not used to actually allocate
or cache-flush a buffer.

And no we cannot use 2044 as then the 2nd buffer in the array
still is not cache-aligned.

But using 2048 is fine, as long as we only use it to allocate
buffers, and not actually as the value passed to the hardware
(which is what the new CONFIG_ETH_RXSIZE is for).

I started with just using 2048 and that does indeed not work,
but with the addition of CONFIG_ETH_RXSIZE things work fine.

Regards,

Hans
diff mbox

Patch

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 7c088c3..877859c 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -32,7 +32,8 @@ 
 
 #define CONFIG_TX_DESCR_NUM	32
 #define CONFIG_RX_DESCR_NUM	32
-#define CONFIG_ETH_BUFSIZE	2024
+#define CONFIG_ETH_BUFSIZE	2048
+#define CONFIG_ETH_RXSIZE	2024 /* Note most fit in ETH_BUFSIZE */
 
 #define TX_TOTAL_BUFSIZE	(CONFIG_ETH_BUFSIZE * CONFIG_TX_DESCR_NUM)
 #define RX_TOTAL_BUFSIZE	(CONFIG_ETH_BUFSIZE * CONFIG_RX_DESCR_NUM)
@@ -324,7 +325,7 @@  static void rx_descs_init(struct emac_eth_dev *priv)
 		desc_p->buf_addr = (uintptr_t)&rxbuffs[idx * CONFIG_ETH_BUFSIZE]
 			;
 		desc_p->next = (uintptr_t)&desc_table_p[idx + 1];
-		desc_p->st |= CONFIG_ETH_BUFSIZE;
+		desc_p->st |= CONFIG_ETH_RXSIZE;
 		desc_p->status = BIT(31);
 	}
 
@@ -506,7 +507,7 @@  static int _sun8i_eth_recv(struct emac_eth_dev *priv, uchar **packetp)
 					roundup(data_end,
 						ARCH_DMA_MINALIGN));
 		if (good_packet) {
-			if (length > CONFIG_ETH_BUFSIZE) {
+			if (length > CONFIG_ETH_RXSIZE) {
 				printf("Received packet is too big (len=%d)\n",
 				       length);
 				return -EMSGSIZE;