Skip to contentSkip to conversation

smc911x driver frame alignment patch

PATCH Superseded Valentin Yakovenkov 3 participants 5 messages Apr 23, 2010 – Apr 26, 2010 Network drivers

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 smc911x driver frame alignment patch

SMSC911x chips have 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. 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 at the beginig. So calculated data length is always bigger by 1 word.

It seems that 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.

It seems that fifo underflow occurs only when using byte packing on 32-bit blackfin bus (may be because of very small delay between reads).

drivers/net/smc911x.c +1 -1
diff -r 7dc8ff189175 a/drivers/net/smc911x.c
@@ -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/20100423/fa76177c/attachment.bin

#1 lore raw

PATCH v2 1/1 smc911x driver frame alignment patch

for future reference, you shouldnt put "patch" in the subject name ... i'm not referring to the leading [PATCH], but the trailing "patch".

also, i'm not sure if you're using git send-email because the trail of your patch is missing the "---" marker:
=====================
It seems that fifo underflow occurs only when using byte packing on 32-bit blackfin bus (may be because of very small delay between reads).

diff -r 7dc8ff189175 a/drivers/net/smc911x.c
--- a/drivers/net/smc911x.c Mon Mar 29 11:08:55 2010 +0400
=====================

it should have been like:
=====================
It seems that fifo underflow occurs only when using byte packing on 32-bit blackfin bus (may be because of very small delay between reads).

---
diff -r 7dc8ff189175 a/drivers/net/smc911x.c
--- a/drivers/net/smc911x.c Mon Mar 29 11:08:55 2010 +0400
=====================
-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/20100423/fdd2e76c/attachment.pgp

#2 lore raw

PATCH v2 1/1 smc911x driver frame alignment patch

Quoted 2 lines · Mike Frysinger

for future reference, you shouldnt put "patch" in the subject name ... i'm not referring to the leading [PATCH], but the trailing "patch".

ok, thanx

Quoted 2 lines

also, i'm not sure if you're using git send-email because the trail of your patch is missing the "---" marker:

I don't use git at all. We have mercurial "master repo" which is pulled from local "SVN synced repo" which is synchronized via svn with blackfin.uclinux.org.

This patch is a result of the "hg diff smc911.x" command with manually added signed-off-by line.

Is it so critical to make this patch via git?

Signature · 11 lines
   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/20100423/f69d9d93/attachment.bin 
#3 lore raw

PATCH v2 1/1 smc911x driver frame alignment patch

Quoted 3 lines · On Friday 23 April 2010 12:48:55 Valentin Yakovenkov wrote:

I don't use git at all. We have mercurial "master repo" which is pulled from local "SVN synced repo" which is synchronized via svn with blackfin.uclinux.org.

b.u.o offers git as well if that makes things easier to sync:
http://blackfin.uclinux.org/git/

Quoted 1 line

Is it so critical to make this patch via git?

no, but it is important the patch format matches what git expects. so if you add the --- marker yourself after the s-o-b tag, that's fine.
-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/20100423/d37fb85f/attachment.pgp

Applied
#4 lore raw

PATCH v2 1/1 smc911x driver frame alignment patch

In reply to Valentin Yakovenkov

Valentin,

Quoted 42 lines · Valentin Yakovenkov

SMSC911x chips have 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. 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 at the beginig. So calculated data length is always bigger by 1 word.

It seems that 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.

It seems that fifo underflow occurs only when using byte packing on 32-bit blackfin bus (may be because of very small delay between reads).

diff -r 7dc8ff189175 a/drivers/net/smc911x.c

drivers/net/smc911x.c +0 -0
@@ -220,7 +220,7 @@
 
  		smc911x_reg_write(dev, RX_CFG, 0);

drivers/net/smc911x.c

Quoted 10 lines
drivers/net/smc911x.c +1 -2
 
-		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

Applied to net repo.

thanks,
Ben