[U-Boot] [PATCH] smc911x driver frame alignment patch

[U-Boot] [PATCH] smc911x driver frame alignment patch

[Valentin Yakovenkov]

Wrong alignment in smc911x driver when reading a frame from fifo.
Neither smc911x chip nor U-Boot doesn't use IP-alignment, so we don't
need to add anything here.

Signed-off-by: Valentin Yakovenkov <yakovenkov@niistt.ru>

diff -r 7dc8ff189175 a/drivers/net/smc911x.c
--- a/drivers/net/smc911x.c	Mon Mar 29 11:08:55 2010 +0400
+++ b/drivers/net/smc911x.c	Mon Apr 19 10:46:02 2010 +0400
@@ -220,7 +220,7 @@

 		smc911x_reg_write(dev, RX_CFG, 0);

-		tmplen = (pktlen + 2+ 3) / 4;
+		tmplen = (pktlen + 3) / 4;
 		while (tmplen--)
 			*data++ = pkt_data_pull(dev, RX_DATA_FIFO);


--
  WBR, Valentin
  CJSC "NII STT", Russia, Smolensk
  http://www.niistt.ru


-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3609 bytes
Desc: S/MIME Cryptographic Signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100421/65da43af/attachment.bin 

[U-Boot] [PATCH] smc911x driver frame alignment patch

[Ben Warren]

Mike,

I know you use this driver a lot.  Please comment on this patch.

On 4/21/2010 12:56 AM, Valentin Yakovenkov wrote:
> Wrong alignment in smc911x driver when reading a frame from fifo.
> Neither smc911x chip nor U-Boot doesn't use IP-alignment, so we don't
> need to add anything here.
>
> Signed-off-by: Valentin Yakovenkov<yakovenkov@niistt.ru>
>
> diff -r 7dc8ff189175 a/drivers/net/smc911x.c
> --- a/drivers/net/smc911x.c	Mon Mar 29 11:08:55 2010 +0400
> +++ b/drivers/net/smc911x.c	Mon Apr 19 10:46:02 2010 +0400
> @@ -220,7 +220,7 @@
>
>   		smc911x_reg_write(dev, RX_CFG, 0);
>
> -		tmplen = (pktlen + 2+ 3) / 4;
> +		tmplen = (pktlen + 3) / 4;
>   		while (tmplen--)
>   			*data++ = pkt_data_pull(dev, RX_DATA_FIFO);
>
>
> --
>    WBR, Valentin
>    CJSC "NII STT", Russia, Smolensk
>    http://www.niistt.ru
>    

regards,
Ben

[U-Boot] [PATCH] smc911x driver frame alignment patch

[Mike Frysinger]

On Wednesday 21 April 2010 13:52:27 Ben Warren wrote:
> On 4/21/2010 12:56 AM, Valentin Yakovenkov wrote:
> > Wrong alignment in smc911x driver when reading a frame from fifo.
> > Neither smc911x chip nor U-Boot doesn't use IP-alignment, so we don't
> > need to add anything here.
>
> I know you use this driver a lot.  Please comment on this patch.

i really havent a clue what this change is doing, and the changelog doesnt 
make much sense to me.  too many negatives perhaps ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100421/01eced77/attachment.pgp 

[U-Boot] [PATCH] smc911x driver frame alignment patch

[Valentin Yakovenkov]

21.04.2010 23:52, Mike Frysinger wrote:
>>> Wrong alignment in smc911x driver when reading a frame from fifo.
>>> Neither smc911x chip nor U-Boot doesn't use IP-alignment, so we don't
>>> need to add anything here.
>>
>> I know you use this driver a lot.  Please comment on this patch.
>
> i really havent a clue what this change is doing, and the changelog doesnt
> make much sense to me.  too many negatives perhaps ...

SMSC911x chips has alignment function to allow frame payload data (which 
comes after 14-bytes ethernet header) to be aligned at some boundary 
when reading it from fifo (usually - 4 bytes boundary).
This is done by inserting fake zeros bytes BEFORE actual frame data when 
reading from SMSC's fifo.
This function controlled by RX_CFG register. There are bits that 
represents amount of fake bytes to be inserted.

Linux uses alignment of 4 bytes. Ethernet frame header is 14 bytes long, 
so we need to add 2 fake bytes to get payload data aligned at 4-bytes 
boundary.
Linux driver does this by adding IP_ALIGNMENT constant (defined at 
skb.h) when calculating fifo data length. But all network subsystem of 
Linux uses this constant too when calculating different offsets.

But u-boot does not use any packet data alignment, so we don't need to 
add anything when calculating fifo data length.
Moreover, driver zeros the RX_CFG register just one line up, so chip 
does not insert any fake data. After calculating data length we always 
got 1 more word to read.

So, at almost every packet read we get an underflow condition at fifo 
and possible corruption of data. Especially at continuous transfers, 
such as tftp.

Just after removing this magic addition, I've got tftp transfer speed as 
it aught to be at 100Mbps. It was really slow before.

Sorry for my english, I'm just a bad russian boy %)

-- 
   WBR, Valentin
   CJSC "NII STT", Russia, Smolensk
   http://www.niistt.ru

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3609 bytes
Desc: S/MIME Cryptographic Signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100422/51ceb733/attachment.bin 

[U-Boot] [PATCH] smc911x driver frame alignment patch

[Mike Frysinger]

On Thursday 22 April 2010 15:12:19 Valentin Yakovenkov wrote:
> 21.04.2010 23:52, Mike Frysinger wrote:
> >>> Wrong alignment in smc911x driver when reading a frame from fifo.
> >>> Neither smc911x chip nor U-Boot doesn't use IP-alignment, so we don't
> >>> need to add anything here.
> >> 
> >> I know you use this driver a lot.  Please comment on this patch.
> > 
> > i really havent a clue what this change is doing, and the changelog
> > doesnt make much sense to me.  too many negatives perhaps ...
> 
> SMSC911x chips has alignment function to allow frame payload data (which
> comes after 14-bytes ethernet header) to be aligned at some boundary
> when reading it from fifo (usually - 4 bytes boundary).
> This is done by inserting fake zeros bytes BEFORE actual frame data when
> reading from SMSC's fifo.
> This function controlled by RX_CFG register. There are bits that
> represents amount of fake bytes to be inserted.
> 
> Linux uses alignment of 4 bytes. Ethernet frame header is 14 bytes long,
> so we need to add 2 fake bytes to get payload data aligned at 4-bytes
> boundary.
> Linux driver does this by adding IP_ALIGNMENT constant (defined at
> skb.h) when calculating fifo data length. But all network subsystem of
> Linux uses this constant too when calculating different offsets.
> 
> But u-boot does not use any packet data alignment, so we don't need to
> add anything when calculating fifo data length.
> Moreover, driver zeros the RX_CFG register just one line up, so chip
> does not insert any fake data. After calculating data length we always
> got 1 more word to read.
> 
> So, at almost every packet read we get an underflow condition at fifo
> and possible corruption of data. Especially at continuous transfers,
> such as tftp.
> 
> Just after removing this magic addition, I've got tftp transfer speed as
> it aught to be at 100Mbps. It was really slow before.

i would send the patch again with this info in the changelog.  however, at 
least on my board, i see no speed difference with this patch.  i get about 
2.8MiB/s on my bf548-ezkit with and without your change.  so, it doesnt break 
anything that i can see, nor have i been experiencing any problems before, so 
i dont have a problem with the patch being merged (once a better changelog is 
added).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100422/315ce0d6/attachment.pgp 

[U-Boot] [PATCH] smc911x driver frame alignment patch

[Valentin Yakovenkov]

22.04.2010 23:43, Mike Frysinger wrote:

> i would send the patch again with this info in the changelog.  however, at
> least on my board, i see no speed difference with this patch.  i get about
> 2.8MiB/s on my bf548-ezkit with and without your change.  so, it doesnt break
> anything that i can see, nor have i been experiencing any problems before, so
> i dont have a problem with the patch being merged (once a better changelog is
> added).

I think it's because of we're using Byte Packing enabled and 32-bit 
reads, but smsc9221 bus is 16-bit wide.

bf548-ezkit config defines CONFIG_SMC911X_16_BIT, but ours - 
CONFIG_SMC911X_32_BIT.

This is the only difference i've found.

-- 
   WBR, Valentin
   CJSC "NII STT", Russia, Smolensk
   http://www.niistt.ru

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3609 bytes
Desc: S/MIME Cryptographic Signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100422/ee3877dc/attachment.bin 

[U-Boot] [PATCH] smc911x driver frame alignment patch

[Ben Warren]

Valentin,

On Thu, Apr 22, 2010 at 12:53 PM, Valentin Yakovenkov
<yakovenkov@niistt.ru>wrote:

> 22.04.2010 23:43, Mike Frysinger wrote:
>
>  i would send the patch again with this info in the changelog.  however, at
>> least on my board, i see no speed difference with this patch.  i get about
>> 2.8MiB/s on my bf548-ezkit with and without your change.  so, it doesnt
>> break
>> anything that i can see, nor have i been experiencing any problems before,
>> so
>> i dont have a problem with the patch being merged (once a better changelog
>> is
>> added).
>>
>
> I think it's because of we're using Byte Packing enabled and 32-bit reads,
> but smsc9221 bus is 16-bit wide.
>
> bf548-ezkit config defines CONFIG_SMC911X_16_BIT, but ours -
> CONFIG_SMC911X_32_BIT.
>
> This is the only difference i've found.
>
>
> Thanks for the explanation and test results.  Please re-submit with this
info in the changelog and I'll pull it in.

regards,
Ben