sky2 problem on powerpc

sky2 problem on powerpc

[Benjamin Herrenschmidt]

Hi Stephen !

To try to validate some PCIe code we have around, I got myself a
SysKonnect SK-9Exx card (one x1 lane one port and one x4 lanes 2 ports
actually, though I've only tested the first one at this point, based on
a 88E8061 Marvell chip).

I've used the sky2 driver as of a random last week's git snapshot. I'll
double check tomorrow if there have been any update.

So far, I had no success on a couple of different machines, and I was
wondering if you had an idea of what's wrong. Here's what I've seen far:

 - Card is probed fine

sky2 v1.5 addr 0xa0000000 irq 63 Yukon-XL (0xb3) rev 1
sky2 eth2: addr 00:00:5a:72:4c:2a

 - As soon as I ifconfig it up, I get a steady stream of

sky2 eth2: receive descriptor error (hardware problem)
sky2 eth2: Link is down.

mesages in my kernel log, which makes me think that it's unhappy about
the format of the descriptors or something like that (endian issue ? see
below my notes about that). I'm also wondering why it seems to be
flooding me which makes me think the error condition is never cleared on
the chip.

Some of the things I've noticed and tweaked in the driver, without luck
so far:

 - There is that bit when initializing the chip:

#ifdef __BIG_ENDIAN
        /* byte swap descriptors in hardware */
        {
                u32 reg;

  .../...

However, if you look at some of the routines filling up descriptors,
they still do swap the values put into some of the fields like lenght,
address. (Except an incosistency with rx_set_checksum which doesn't swap
whatever it writes to le->addr, though I've tried fixing that without
much success).

Thus I don't quite understand what the above is supposed to do. I've
tried disabling it but it didn't fix the problem. However, that leads me
to wondering wether there might be a deeper problem with descriptor
swapping. What kind of access size and what kind of swapping the chip is
supposedly doing ? 32 bits accesses ? 64 bits accesses ?

Because if you look at the definition for:

struct sky2_rx_le {
	__le32	addr;
	__le16	length;
	u8	ctrl;
	u8	opcode;
} __attribute((packed));

(As an example)

If the chips does LE accesses, then marking "lenght" as being an LE
value isn't enough. It also needs to be swapped with ctrl and opcode.
The layout then becomes:

struct sky2_rx_le {
	__le32	addr;
	u8	opcode;
	u8	ctrl;
	__le16	length;
} __attribute((packed));

Unless the chip does 2x16 bits accesses here but I very much doubt it.
Now, it's possible that this bit about "byteswapping descriptor" is
about doing that address munging, but would it do it without also
byteswapping the content of the individual fields ? That looks
unlikely... Thus I wonder wether we should either keep this HW byteswap
option _and_ remove all the byteswapping of fields when accessing addr
and lenght, or leave the byteswapping in, remove the HW byteswap option
_and_ fixup the structure definitions of the descriptor in the big
endian case ....

I haven't had time to try all those options yet, and I don't have access
to any documentation for that chip, but I wondered if, in the meantime,
you had any other idea about what may be going wrong or see any flaw in
my reasoning above.

Cheers,
Ben.



-- 
VGER BF report: U 0.505334

Re: sky2 problem on powerpc

[Benjamin Herrenschmidt]

> Because if you look at the definition for:
> 
> struct sky2_rx_le {
> 	__le32	addr;
> 	__le16	length;
> 	u8	ctrl;
> 	u8	opcode;
> } __attribute((packed));
> 
> (As an example)
> 
> If the chips does LE accesses, then marking "lenght" as being an LE
> value isn't enough. It also needs to be swapped with ctrl and opcode.
> The layout then becomes:
> 
> struct sky2_rx_le {
> 	__le32	addr;
> 	u8	opcode;
> 	u8	ctrl;
> 	__le16	length;
> } __attribute((packed));

And of course, I'm stupid... the structure layout in memory is well
defined and the chip is accessing it just fine... The problem are
actually related to

 - That special "reverse" descriptor bit one can set in the chip....
That will indeed cause my scenario above to happen by causing the chip
to do a 32 bits byteswap it seems. But if we set this bit, we must also
remove all of the cpu_to_le/le_to_cpu conversions used to access
descriptors.

 - I've decided not to set that bit for now, thus disabled the code
doing so (and thus requiring be/le conversions). We have cases where
they are missing, and thus it's not working.

I'm trying to get it right now, will send a patch if I can get something
working. At one point, I suppose I'll have the choice of either letting
the HW do the 32 bits flip, which involves playing #ifdef __BIG_ENDIAN
with the structures in sky2.h or just ignore that bit and do cpu_to_le*
when needed. I'm tempted to do the later...

Ben



-- 
VGER BF report: U 0.526041

Re: sky2 problem on powerpc

[Benjamin Herrenschmidt]

This fixes sky2 driver on big endian machines. I choose not to use the
hardware byteswap facility as it would have required to have a different
definition of the various ring data structures and it looks ugly :) On
powerpc, there is pretty much no overhead at doing byteswap.

The patch has a couple of places where I reversed 2 assignments, they
are harmless, it was before I figured out that the chip will
(apparently) not access a descriptor before it's been told to do so via
MMIO, and thus the order of the writes to the descriptors is irrelevant
(I was also adding wmb's though I removed them).

There is a couple of places where we were doing a BE and not LE
conversion of a descriptor field (typically in the VLAN code). I'm not
sure what's up there but BE "felt" wrong. I have turned them into LE
conversions but then I haven't tested VLAN, and I might just
misudnerstand what's happening there so I'll let you decide what to do
about those.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/drivers/net/sky2.c
===================================================================
--- linux-work.orig/drivers/net/sky2.c	2006-09-04 17:35:20.000000000 +1000
+++ linux-work/drivers/net/sky2.c	2006-09-04 17:41:50.000000000 +1000
@@ -811,8 +811,9 @@ static void rx_set_checksum(struct sky2_
 	struct sky2_rx_le *le;
 
 	le = sky2_next_rx(sky2);
-	le->addr = (ETH_HLEN << 16) | ETH_HLEN;
+	le->addr = cpu_to_le32((ETH_HLEN << 16) | ETH_HLEN);
 	le->ctrl = 0;
+	le->length = 0;
 	le->opcode = OP_TCPSTART | HW_OWNER;
 
 	sky2_write32(sky2->hw,
@@ -1001,7 +1002,6 @@ static int sky2_rx_start(struct sky2_por
 		sky2_rx_add(sky2, re->mapaddr);
 	}
 
-
 	/*
 	 * The receiver hangs if it receives frames larger than the
 	 * packet buffer. As a workaround, truncate oversize frames, but
@@ -1250,8 +1250,8 @@ static int sky2_xmit_frame(struct sk_buf
 		le = get_tx_le(sky2);
 		le->tx.tso.size = cpu_to_le16(mss);
 		le->tx.tso.rsvd = 0;
-		le->opcode = OP_LRGLEN | HW_OWNER;
 		le->ctrl = 0;
+		le->opcode = OP_LRGLEN | HW_OWNER;
 		sky2->tx_last_mss = mss;
 	}
 
@@ -1262,11 +1262,11 @@ static int sky2_xmit_frame(struct sk_buf
 		if (!le) {
 			le = get_tx_le(sky2);
 			le->tx.addr = 0;
-			le->opcode = OP_VLAN|HW_OWNER;
 			le->ctrl = 0;
+			le->opcode = OP_VLAN|HW_OWNER;
 		} else
 			le->opcode |= OP_VLAN;
-		le->length = cpu_to_be16(vlan_tx_tag_get(skb));
+		le->length = cpu_to_le16(vlan_tx_tag_get(skb));
 		ctrl |= INS_VLAN;
 	}
 #endif
@@ -1955,8 +1955,8 @@ static int sky2_status_intr(struct sky2_
 		dev = hw->dev[le->link];
 
 		sky2 = netdev_priv(dev);
-		length = le->length;
-		status = le->status;
+		length = le16_to_cpu(le->length);
+		status = le32_to_cpu(le->status);
 
 		switch (le->opcode & ~HW_OWNER) {
 		case OP_RXSTAT:
@@ -1972,7 +1972,7 @@ static int sky2_status_intr(struct sky2_
 			if (sky2->vlgrp && (status & GMR_FS_VLAN)) {
 				vlan_hwaccel_receive_skb(skb,
 							 sky2->vlgrp,
-							 be16_to_cpu(sky2->rx_tag));
+							 le16_to_cpu(sky2->rx_tag));
 			} else
 #endif
 				netif_receive_skb(skb);
@@ -2001,7 +2001,7 @@ static int sky2_status_intr(struct sky2_
 		case OP_RXCHKS:
 			skb = sky2->rx_ring[sky2->rx_next].skb;
 			skb->ip_summed = CHECKSUM_HW;
-			skb->csum = le16_to_cpu(status);
+			skb->csum = status & 0xffffu;
 			break;
 
 		case OP_TXINDEXLE:
@@ -3240,6 +3240,7 @@ static int __devinit sky2_probe(struct p
 	struct net_device *dev, *dev1 = NULL;
 	struct sky2_hw *hw;
 	int err, pm_cap, using_dac = 0;
+	u32 reg;
 
 	err = pci_enable_device(pdev);
 	if (err) {
@@ -3303,16 +3304,12 @@ static int __devinit sky2_probe(struct p
 	}
 	hw->pm_cap = pm_cap;
 
-#ifdef __BIG_ENDIAN
-	/* byte swap descriptors in hardware */
-	{
-		u32 reg;
-
-		reg = sky2_pci_read32(hw, PCI_DEV_REG2);
-		reg |= PCI_REV_DESC;
-		sky2_pci_write32(hw, PCI_DEV_REG2, reg);
-	}
-#endif
+	/* Don't let it byte swap descriptors in hardware. Clear the bit
+	 * in case a previous driver have set it
+	 */
+	reg = sky2_pci_read32(hw, PCI_DEV_REG2);
+	reg &= ~PCI_REV_DESC;
+	sky2_pci_write32(hw, PCI_DEV_REG2, reg);
 
 	/* ring for status responses */
 	hw->st_le = pci_alloc_consistent(hw->pdev, STATUS_LE_BYTES,
Index: linux-work/drivers/net/sky2.h
===================================================================
--- linux-work.orig/drivers/net/sky2.h	2006-09-04 17:35:20.000000000 +1000
+++ linux-work/drivers/net/sky2.h	2006-09-04 17:36:44.000000000 +1000
@@ -1785,7 +1785,6 @@ enum {
 };
 
 /* Yukon 2 hardware interface
- * Not tested on big endian
  */
 struct sky2_tx_le {
 	union {



-- 
VGER BF report: S 0.992386

Re: sky2 problem on powerpc

[Segher Boessenkool]

> The patch has a couple of places where I reversed 2 assignments, they
> are harmless, it was before I figured out that the chip will
> (apparently) not access a descriptor before it's been told to do so  
> via
> MMIO, and thus the order of the writes to the descriptors is  
> irrelevant
> (I was also adding wmb's though I removed them).

wmb() between writing the descriptors to main memory and writing
an MMIO to the device to tell it to go fetch them?  That's still
required.  The 970 you tested on won't care though ;-)

> There is a couple of places where we were doing a BE and not LE
> conversion of a descriptor field (typically in the VLAN code). I'm not
> sure what's up there but BE "felt" wrong. I have turned them into LE
> conversions but then I haven't tested VLAN, and I might just
> misudnerstand what's happening there so I'll let you decide what to do
> about those.

VLAN tags are big-endian on the wire, it might well be that these
chips never swizzle that?

> --- linux-work.orig/drivers/net/sky2.c	2006-09-04  
> 17:35:20.000000000 +1000
> +++ linux-work/drivers/net/sky2.c	2006-09-04 17:41:50.000000000 +1000
> @@ -811,8 +811,9 @@ static void rx_set_checksum(struct sky2_
>  	struct sky2_rx_le *le;
>
>  	le = sky2_next_rx(sky2);
> -	le->addr = (ETH_HLEN << 16) | ETH_HLEN;
> +	le->addr = cpu_to_le32((ETH_HLEN << 16) | ETH_HLEN);
>  	le->ctrl = 0;
> +	le->length = 0;
>  	le->opcode = OP_TCPSTART | HW_OWNER;

Is this  le->length  thing a separate bugfix?  Or just overly
protective coding (you write the field later again).

> +	/* Don't let it byte swap descriptors in hardware. Clear the bit
> +	 * in case a previous driver have set it
> +	 */
> +	reg = sky2_pci_read32(hw, PCI_DEV_REG2);
> +	reg &= ~PCI_REV_DESC;
> +	sky2_pci_write32(hw, PCI_DEV_REG2, reg);

Probably not needed, the chip gets a full reset at startup (or so
I hope anyway, heh).


Segher

Re: sky2 problem on powerpc

[Benjamin Herrenschmidt]

On Mon, 2006-09-04 at 23:05 +0200, Segher Boessenkool wrote:
> > The patch has a couple of places where I reversed 2 assignments, they
> > are harmless, it was before I figured out that the chip will
> > (apparently) not access a descriptor before it's been told to do so  
> > via
> > MMIO, and thus the order of the writes to the descriptors is  
> > irrelevant
> > (I was also adding wmb's though I removed them).
> 
> wmb() between writing the descriptors to main memory and writing
> an MMIO to the device to tell it to go fetch them?  That's still
> required.  The 970 you tested on won't care though ;-)

Won't it ? Anyway, you misunderstood, there is a wmb() already in the
right place
in sky2_put_idx() if I understand things correctly (but then I don't
know the chip hardware well) and the ones I originally added weren't
necessary.

> > There is a couple of places where we were doing a BE and not LE
> > conversion of a descriptor field (typically in the VLAN code). I'm not
> > sure what's up there but BE "felt" wrong. I have turned them into LE
> > conversions but then I haven't tested VLAN, and I might just
> > misudnerstand what's happening there so I'll let you decide what to do
> > about those.
> 
> VLAN tags are big-endian on the wire, it might well be that these
> chips never swizzle that?

Maybe yes.

> > --- linux-work.orig/drivers/net/sky2.c	2006-09-04  
> > 17:35:20.000000000 +1000
> > +++ linux-work/drivers/net/sky2.c	2006-09-04 17:41:50.000000000 +1000
> > @@ -811,8 +811,9 @@ static void rx_set_checksum(struct sky2_
> >  	struct sky2_rx_le *le;
> >
> >  	le = sky2_next_rx(sky2);
> > -	le->addr = (ETH_HLEN << 16) | ETH_HLEN;
> > +	le->addr = cpu_to_le32((ETH_HLEN << 16) | ETH_HLEN);
> >  	le->ctrl = 0;
> > +	le->length = 0;
> >  	le->opcode = OP_TCPSTART | HW_OWNER;
> 
> Is this  le->length  thing a separate bugfix?  Or just overly
> protective coding (you write the field later again).

Overly protective coding when I was still looking for what the problem
was (wanted to have nice descriptors in memory without crap in them :)
I'll try without

> > +	/* Don't let it byte swap descriptors in hardware. Clear the bit
> > +	 * in case a previous driver have set it
> > +	 */
> > +	reg = sky2_pci_read32(hw, PCI_DEV_REG2);
> > +	reg &= ~PCI_REV_DESC;
> > +	sky2_pci_write32(hw, PCI_DEV_REG2, reg);
> 
> Probably not needed, the chip gets a full reset at startup (or so
> I hope anyway, heh).

It is needed. I've verified that this bit resists to anything we do in
the driver. So if you load the old driver that sets it, unload it, then
load the fixed driver, you are toast... I'm also taking no risk in case
one has a firmware that sets this bit.

Ben.

Re: sky2 problem on powerpc

[Stephen Hemminger]

On Mon, 04 Sep 2006 17:42:27 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> This fixes sky2 driver on big endian machines. I choose not to use the
> hardware byteswap facility as it would have required to have a different
> definition of the various ring data structures and it looks ugly :) On
> powerpc, there is pretty much no overhead at doing byteswap.
> 
> The patch has a couple of places where I reversed 2 assignments, they
> are harmless, it was before I figured out that the chip will
> (apparently) not access a descriptor before it's been told to do so via
> MMIO, and thus the order of the writes to the descriptors is irrelevant
> (I was also adding wmb's though I removed them).

I'll put a minimized version of this in the next patch set.
There is no need to re order assignments.

> 
> There is a couple of places where we were doing a BE and not LE
> conversion of a descriptor field (typically in the VLAN code). I'm not
> sure what's up there but BE "felt" wrong. I have turned them into LE
> conversions but then I haven't tested VLAN, and I might just
> misudnerstand what's happening there so I'll let you decide what to do
> about those.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

It may not need any swapping, it is hard to tell what the hardware
will do without experimentation.

Re: sky2 problem on powerpc

[Benjamin Herrenschmidt]

> It may not need any swapping, it is hard to tell what the hardware
> will do without experimentation.

Yes... did you have a chance to test the vlan stuff on LE machines
(x86) ? did it work with the BE swapping you were doing ? I've
purposedly removed in my patches the hardware side swapping of the
descriptors, as I explained, thus making the hardware react the same on
ppc and x86. Thus we need the exact same swapping macros on both
platforms).

I know pretty much nothing about vlan so I'm not too much about trying
to check that right now :)

Also, there is still the hw checksum issue .... I need to verify what's
up there, it might be a swapping problem as well... or not. Can you send
me your latest patch set so I can work from there ?

Cheers,
Ben

Re: sky2 problem on powerpc

[Stephen Hemminger]

On Tue, 05 Sep 2006 13:47:52 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> > It may not need any swapping, it is hard to tell what the hardware
> > will do without experimentation.
> 
> Yes... did you have a chance to test the vlan stuff on LE machines
> (x86) ? did it work with the BE swapping you were doing ? I've
> purposedly removed in my patches the hardware side swapping of the
> descriptors, as I explained, thus making the hardware react the same on
> ppc and x86. Thus we need the exact same swapping macros on both
> platforms).


Last time I checked it worked.  Private cable simulating VLAN
from other Linux card.

> I know pretty much nothing about vlan so I'm not too much about trying
> to check that right now :)
> 
> Also, there is still the hw checksum issue .... I need to verify what's
> up there, it might be a swapping problem as well... or not. Can you send
> me your latest patch set so I can work from there ?
> 
> Cheers,
> Ben
> 

Re: sky2 problem on powerpc

[Benjamin Herrenschmidt]

On Mon, 2006-09-04 at 21:15 -0700, Stephen Hemminger wrote:
> On Tue, 05 Sep 2006 13:47:52 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > 
> > > It may not need any swapping, it is hard to tell what the hardware
> > > will do without experimentation.
> > 
> > Yes... did you have a chance to test the vlan stuff on LE machines
> > (x86) ? did it work with the BE swapping you were doing ? I've
> > purposedly removed in my patches the hardware side swapping of the
> > descriptors, as I explained, thus making the hardware react the same on
> > ppc and x86. Thus we need the exact same swapping macros on both
> > platforms).
> 
> 
> Last time I checked it worked.  Private cable simulating VLAN
> from other Linux card.

Ok, so we should probably switch back the vlan bits to BE swapping
macros... 

However, we then have an inconsistency with that bit:

#ifdef SKY2_VLAN_TAG_USED
                case OP_RXVLAN:
                        sky2->rx_tag = length;
                        break;

                case OP_RXCHKSVLAN:
                        sky2->rx_tag = length;
                        /* fall through */
#endif

in sky2_status_intr()

Where we read the lenght field directly without swapping (on the non
patched driver, on the patched driver, lenght will have gone through an
LE swap). That is, if you take the standpoint of a LE machine, you will
read that value as a little endian value while elsewhere, we manipulate
sky2->rx_tag as a BE value... (this is even without my patch)

Ben.

Re: sky2 problem on powerpc

[Stephen Hemminger]

This is the reduced version of your patch, plus I got rid of the union
in tx_le, it is a nuisance.


--- sky2.orig/drivers/net/sky2.c	2006-09-05 13:39:34.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-09-05 13:57:44.000000000 -0700
@@ -809,7 +809,7 @@
 	struct sky2_rx_le *le;
 
 	le = sky2_next_rx(sky2);
-	le->addr = (ETH_HLEN << 16) | ETH_HLEN;
+	le->addr = cpu_to_le32((ETH_HLEN << 16) | ETH_HLEN);
 	le->ctrl = 0;
 	le->opcode = OP_TCPSTART | HW_OWNER;
 
@@ -1227,7 +1227,7 @@
 	/* Send high bits if changed or crosses boundary */
 	if (addr64 != sky2->tx_addr64 || high32(mapping + len) != sky2->tx_addr64) {
 		le = get_tx_le(sky2);
-		le->tx.addr = cpu_to_le32(addr64);
+		le->addr = cpu_to_le32(addr64);
 		le->ctrl = 0;
 		le->opcode = OP_ADDR64 | HW_OWNER;
 		sky2->tx_addr64 = high32(mapping + len);
@@ -1242,8 +1242,7 @@
 
 		if (mss != sky2->tx_last_mss) {
 			le = get_tx_le(sky2);
-			le->tx.tso.size = cpu_to_le16(mss);
-			le->tx.tso.rsvd = 0;
+			le->addr = cpu_to_le32(mss);
 			le->opcode = OP_LRGLEN | HW_OWNER;
 			le->ctrl = 0;
 			sky2->tx_last_mss = mss;
@@ -1256,7 +1255,7 @@
 	if (sky2->vlgrp && vlan_tx_tag_present(skb)) {
 		if (!le) {
 			le = get_tx_le(sky2);
-			le->tx.addr = 0;
+			le->addr = 0;
 			le->opcode = OP_VLAN|HW_OWNER;
 			le->ctrl = 0;
 		} else
@@ -1268,20 +1267,21 @@
 
 	/* Handle TCP checksum offload */
 	if (skb->ip_summed == CHECKSUM_HW) {
-		u16 hdr = skb->h.raw - skb->data;
-		u16 offset = hdr + skb->csum;
+		unsigned offset = skb->h.raw - skb->data;
+		u32 tcpsum;
+
+		tcpsum = offset << 16;		/* sum start */
+		tcpsum |= offset + skb->csum;	/* sum write */
 
 		ctrl = CALSUM | WR_SUM | INIT_SUM | LOCK_SUM;
 		if (skb->nh.iph->protocol == IPPROTO_UDP)
 			ctrl |= UDPTCP;
 
-		if (hdr != sky2->tx_csum_start || offset != sky2->tx_csum_offset) {
-			sky2->tx_csum_start = hdr;
-			sky2->tx_csum_offset = offset;
+		if (tcpsum != sky2->tx_tcpsum) {
+			sky2->tx_tcpsum = tcpsum;
 
 			le = get_tx_le(sky2);
-			le->tx.csum.start = cpu_to_le16(hdr);
-			le->tx.csum.offset = cpu_to_le16(offset);
+			le->addr = cpu_to_le32(tcpsum);
 			le->length = 0;	/* initial checksum value */
 			le->ctrl = 1;	/* one packet */
 			le->opcode = OP_TCPLISW | HW_OWNER;
@@ -1289,7 +1289,7 @@
 	}
 
 	le = get_tx_le(sky2);
-	le->tx.addr = cpu_to_le32((u32) mapping);
+	le->addr = cpu_to_le32((u32) mapping);
 	le->length = cpu_to_le16(len);
 	le->ctrl = ctrl;
 	le->opcode = mss ? (OP_LARGESEND | HW_OWNER) : (OP_PACKET | HW_OWNER);
@@ -1307,14 +1307,14 @@
 		addr64 = high32(mapping);
 		if (addr64 != sky2->tx_addr64) {
 			le = get_tx_le(sky2);
-			le->tx.addr = cpu_to_le32(addr64);
+			le->addr = cpu_to_le32(addr64);
 			le->ctrl = 0;
 			le->opcode = OP_ADDR64 | HW_OWNER;
 			sky2->tx_addr64 = addr64;
 		}
 
 		le = get_tx_le(sky2);
-		le->tx.addr = cpu_to_le32((u32) mapping);
+		le->addr = cpu_to_le32((u32) mapping);
 		le->length = cpu_to_le16(frag->size);
 		le->ctrl = ctrl;
 		le->opcode = OP_BUFFER | HW_OWNER;
@@ -1919,8 +1919,8 @@
 		dev = hw->dev[le->link];
 
 		sky2 = netdev_priv(dev);
-		length = le->length;
-		status = le->status;
+		length = le16_to_cpu(le->length);
+		status = le32_to_cpu(le->status);
 
 		switch (le->opcode & ~HW_OWNER) {
 		case OP_RXSTAT:
@@ -1964,7 +1964,7 @@
 		case OP_RXCHKS:
 			skb = sky2->rx_ring[sky2->rx_next].skb;
 			skb->ip_summed = CHECKSUM_HW;
-			skb->csum = le16_to_cpu(status);
+			skb->csum = status & 0xffff;
 			break;
 
 		case OP_TXINDEXLE:
@@ -3266,12 +3266,13 @@
 	hw->pm_cap = pm_cap;
 
 #ifdef __BIG_ENDIAN
-	/* byte swap descriptors in hardware */
+	/* The sk98lin vendor driver uses hardware byte swapping but
+	 * this driver uses software swapping.
+	 */
 	{
 		u32 reg;
-
 		reg = sky2_pci_read32(hw, PCI_DEV_REG2);
-		reg |= PCI_REV_DESC;
+		reg &= ~PCI_REV_DESC;
 		sky2_pci_write32(hw, PCI_DEV_REG2, reg);
 	}
 #endif
--- sky2.orig/drivers/net/sky2.h	2006-09-05 12:10:13.000000000 -0700
+++ sky2/drivers/net/sky2.h	2006-09-05 13:55:55.000000000 -0700
@@ -1783,21 +1783,9 @@
 	OP_TXINDEXLE	= 0x68,
 };
 
-/* Yukon 2 hardware interface
- * Not tested on big endian
- */
+/* Yukon 2 hardware interface */
 struct sky2_tx_le {
-	union {
-		__le32	addr;
-		struct {
-			__le16	offset;
-			__le16	start;
-		} csum  __attribute((packed));
-		struct {
-			__le16	size;
-			__le16	rsvd;
-		} tso  __attribute((packed));
-	} tx;
+	__le32	addr;
 	__le16	length;	/* also vlan tag or checksum start */
 	u8	ctrl;
 	u8	opcode;
@@ -1843,8 +1831,7 @@
 	u32		     tx_addr64;
 	u16		     tx_pending;
 	u16		     tx_last_mss;
-	u16		     tx_csum_start;
-	u16		     tx_csum_offset;
+	u32		     tx_tcpsum;
 
 	struct ring_info     *rx_ring ____cacheline_aligned_in_smp;
 	struct sky2_rx_le    *rx_le;

Re: sky2 problem on powerpc

[Benjamin Herrenschmidt]

On Tue, 2006-09-05 at 14:36 -0700, Stephen Hemminger wrote:
> This is the reduced version of your patch, plus I got rid of the union
> in tx_le, it is a nuisance.

Thanks. I'll give it a go later today. The remaining nit is the
inconsitent swapping of the vlan tag which is manipulated at BE at times
and LE at others (later hapens in status_intr).

Ben.

> --- sky2.orig/drivers/net/sky2.c	2006-09-05 13:39:34.000000000 -0700
> +++ sky2/drivers/net/sky2.c	2006-09-05 13:57:44.000000000 -0700
> @@ -809,7 +809,7 @@
>  	struct sky2_rx_le *le;
>  
>  	le = sky2_next_rx(sky2);
> -	le->addr = (ETH_HLEN << 16) | ETH_HLEN;
> +	le->addr = cpu_to_le32((ETH_HLEN << 16) | ETH_HLEN);
>  	le->ctrl = 0;
>  	le->opcode = OP_TCPSTART | HW_OWNER;
>  
> @@ -1227,7 +1227,7 @@
>  	/* Send high bits if changed or crosses boundary */
>  	if (addr64 != sky2->tx_addr64 || high32(mapping + len) != sky2->tx_addr64) {
>  		le = get_tx_le(sky2);
> -		le->tx.addr = cpu_to_le32(addr64);
> +		le->addr = cpu_to_le32(addr64);
>  		le->ctrl = 0;
>  		le->opcode = OP_ADDR64 | HW_OWNER;
>  		sky2->tx_addr64 = high32(mapping + len);
> @@ -1242,8 +1242,7 @@
>  
>  		if (mss != sky2->tx_last_mss) {
>  			le = get_tx_le(sky2);
> -			le->tx.tso.size = cpu_to_le16(mss);
> -			le->tx.tso.rsvd = 0;
> +			le->addr = cpu_to_le32(mss);
>  			le->opcode = OP_LRGLEN | HW_OWNER;
>  			le->ctrl = 0;
>  			sky2->tx_last_mss = mss;
> @@ -1256,7 +1255,7 @@
>  	if (sky2->vlgrp && vlan_tx_tag_present(skb)) {
>  		if (!le) {
>  			le = get_tx_le(sky2);
> -			le->tx.addr = 0;
> +			le->addr = 0;
>  			le->opcode = OP_VLAN|HW_OWNER;
>  			le->ctrl = 0;
>  		} else
> @@ -1268,20 +1267,21 @@
>  
>  	/* Handle TCP checksum offload */
>  	if (skb->ip_summed == CHECKSUM_HW) {
> -		u16 hdr = skb->h.raw - skb->data;
> -		u16 offset = hdr + skb->csum;
> +		unsigned offset = skb->h.raw - skb->data;
> +		u32 tcpsum;
> +
> +		tcpsum = offset << 16;		/* sum start */
> +		tcpsum |= offset + skb->csum;	/* sum write */
>  
>  		ctrl = CALSUM | WR_SUM | INIT_SUM | LOCK_SUM;
>  		if (skb->nh.iph->protocol == IPPROTO_UDP)
>  			ctrl |= UDPTCP;
>  
> -		if (hdr != sky2->tx_csum_start || offset != sky2->tx_csum_offset) {
> -			sky2->tx_csum_start = hdr;
> -			sky2->tx_csum_offset = offset;
> +		if (tcpsum != sky2->tx_tcpsum) {
> +			sky2->tx_tcpsum = tcpsum;
>  
>  			le = get_tx_le(sky2);
> -			le->tx.csum.start = cpu_to_le16(hdr);
> -			le->tx.csum.offset = cpu_to_le16(offset);
> +			le->addr = cpu_to_le32(tcpsum);
>  			le->length = 0;	/* initial checksum value */
>  			le->ctrl = 1;	/* one packet */
>  			le->opcode = OP_TCPLISW | HW_OWNER;
> @@ -1289,7 +1289,7 @@
>  	}
>  
>  	le = get_tx_le(sky2);
> -	le->tx.addr = cpu_to_le32((u32) mapping);
> +	le->addr = cpu_to_le32((u32) mapping);
>  	le->length = cpu_to_le16(len);
>  	le->ctrl = ctrl;
>  	le->opcode = mss ? (OP_LARGESEND | HW_OWNER) : (OP_PACKET | HW_OWNER);
> @@ -1307,14 +1307,14 @@
>  		addr64 = high32(mapping);
>  		if (addr64 != sky2->tx_addr64) {
>  			le = get_tx_le(sky2);
> -			le->tx.addr = cpu_to_le32(addr64);
> +			le->addr = cpu_to_le32(addr64);
>  			le->ctrl = 0;
>  			le->opcode = OP_ADDR64 | HW_OWNER;
>  			sky2->tx_addr64 = addr64;
>  		}
>  
>  		le = get_tx_le(sky2);
> -		le->tx.addr = cpu_to_le32((u32) mapping);
> +		le->addr = cpu_to_le32((u32) mapping);
>  		le->length = cpu_to_le16(frag->size);
>  		le->ctrl = ctrl;
>  		le->opcode = OP_BUFFER | HW_OWNER;
> @@ -1919,8 +1919,8 @@
>  		dev = hw->dev[le->link];
>  
>  		sky2 = netdev_priv(dev);
> -		length = le->length;
> -		status = le->status;
> +		length = le16_to_cpu(le->length);
> +		status = le32_to_cpu(le->status);
>  
>  		switch (le->opcode & ~HW_OWNER) {
>  		case OP_RXSTAT:
> @@ -1964,7 +1964,7 @@
>  		case OP_RXCHKS:
>  			skb = sky2->rx_ring[sky2->rx_next].skb;
>  			skb->ip_summed = CHECKSUM_HW;
> -			skb->csum = le16_to_cpu(status);
> +			skb->csum = status & 0xffff;
>  			break;
>  
>  		case OP_TXINDEXLE:
> @@ -3266,12 +3266,13 @@
>  	hw->pm_cap = pm_cap;
>  
>  #ifdef __BIG_ENDIAN
> -	/* byte swap descriptors in hardware */
> +	/* The sk98lin vendor driver uses hardware byte swapping but
> +	 * this driver uses software swapping.
> +	 */
>  	{
>  		u32 reg;
> -
>  		reg = sky2_pci_read32(hw, PCI_DEV_REG2);
> -		reg |= PCI_REV_DESC;
> +		reg &= ~PCI_REV_DESC;
>  		sky2_pci_write32(hw, PCI_DEV_REG2, reg);
>  	}
>  #endif
> --- sky2.orig/drivers/net/sky2.h	2006-09-05 12:10:13.000000000 -0700
> +++ sky2/drivers/net/sky2.h	2006-09-05 13:55:55.000000000 -0700
> @@ -1783,21 +1783,9 @@
>  	OP_TXINDEXLE	= 0x68,
>  };
>  
> -/* Yukon 2 hardware interface
> - * Not tested on big endian
> - */
> +/* Yukon 2 hardware interface */
>  struct sky2_tx_le {
> -	union {
> -		__le32	addr;
> -		struct {
> -			__le16	offset;
> -			__le16	start;
> -		} csum  __attribute((packed));
> -		struct {
> -			__le16	size;
> -			__le16	rsvd;
> -		} tso  __attribute((packed));
> -	} tx;
> +	__le32	addr;
>  	__le16	length;	/* also vlan tag or checksum start */
>  	u8	ctrl;
>  	u8	opcode;
> @@ -1843,8 +1831,7 @@
>  	u32		     tx_addr64;
>  	u16		     tx_pending;
>  	u16		     tx_last_mss;
> -	u16		     tx_csum_start;
> -	u16		     tx_csum_offset;
> +	u32		     tx_tcpsum;
>  
>  	struct ring_info     *rx_ring ____cacheline_aligned_in_smp;
>  	struct sky2_rx_le    *rx_le;