[PATCH net-next 0/5] sky2 driver update

[PATCH net-next 0/5] sky2 driver update

[Stephen Hemminger]

Got inspired to look at what has changed in recent versions
of vendor driver (10.88.1.3).  There were a couple of small things
and some new chip versions.

[PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware

[Stephen Hemminger]

[-- Attachment #1: sky2-rxhash-rxcsum.patch --]
[-- Type: text/plain, Size: 2231 bytes --]

Found when reviewing the vendor driver. Apparently some chip versions
require receive checksumming to be enabled in order for RSS to work.
Rather than silently enabling checksumming which is what the vendor
driver does, return an error to the user instead.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sky2.c	2011-07-07 08:36:10.748003369 -0700
+++ b/drivers/net/sky2.c	2011-07-07 08:36:37.372004595 -0700
@@ -2996,7 +2996,8 @@ static int __devinit sky2_init(struct sk
 		hw->flags = SKY2_HW_GIGABIT
 			| SKY2_HW_NEWER_PHY
 			| SKY2_HW_NEW_LE
-			| SKY2_HW_ADV_POWER_CTL;
+			| SKY2_HW_ADV_POWER_CTL
+			| SKY2_HW_RSS_CHKSUM;
 
 		/* New transmit checksum */
 		if (hw->chip_rev != CHIP_REV_YU_EX_B0)
@@ -3024,7 +3025,7 @@ static int __devinit sky2_init(struct sk
 
 		/* The workaround for status conflicts VLAN tag detection. */
 		if (hw->chip_rev == CHIP_REV_YU_FE2_A0)
-			hw->flags |= SKY2_HW_VLAN_BROKEN;
+			hw->flags |= SKY2_HW_VLAN_BROKEN | SKY2_HW_RSS_CHKSUM;
 		break;
 
 	case CHIP_ID_YUKON_SUPR:
@@ -3033,6 +3034,9 @@ static int __devinit sky2_init(struct sk
 			| SKY2_HW_NEW_LE
 			| SKY2_HW_AUTO_TX_SUM
 			| SKY2_HW_ADV_POWER_CTL;
+
+		if (hw->chip_rev == CHIP_REV_YU_SU_A0)
+			hw->flags |= SKY2_HW_RSS_CHKSUM;
 		break;
 
 	case CHIP_ID_YUKON_UL_2:
@@ -4187,6 +4191,11 @@ static int sky2_set_features(struct net_
 	struct sky2_port *sky2 = netdev_priv(dev);
 	u32 changed = dev->features ^ features;
 
+	/* Some hardware requires receive checksum for RSS to work. */
+	if ( (features & (NETIF_F_RXHASH|NETIF_F_RXCSUM)) == NETIF_F_RXHASH &&
+	     (sky2->hw->flags & SKY2_HW_RSS_CHKSUM))
+		return -EINVAL;
+
 	if (changed & NETIF_F_RXCSUM) {
 		u32 on = features & NETIF_F_RXCSUM;
 		sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
--- a/drivers/net/sky2.h	2011-07-06 21:57:43.155687362 -0700
+++ b/drivers/net/sky2.h	2011-07-07 08:36:37.372004595 -0700
@@ -2281,6 +2281,7 @@ struct sky2_hw {
 #define SKY2_HW_ADV_POWER_CTL	0x00000080	/* additional PHY power regs */
 #define SKY2_HW_RSS_BROKEN	0x00000100
 #define SKY2_HW_VLAN_BROKEN     0x00000200
+#define SKY2_HW_RSS_CHKSUM	0x00000400	/* RSS requires chksum */
 
 	u8	     	     chip_id;
 	u8		     chip_rev;

[PATCH net-next 2/5] sky2: use correct Inter Packet Gap at 10/100mbit

[Stephen Hemminger]

[-- Attachment #1: sky2-100mbit-ipg.patch --]
[-- Type: text/plain, Size: 2390 bytes --]

This is another fix picked out of the vendor driver. The IPG value
in the serial mode register is supposed to be programmed differently
at lower speeds.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sky2.c	2011-07-07 08:36:37.372004595 -0700
+++ b/drivers/net/sky2.c	2011-07-07 08:36:42.420004801 -0700
@@ -714,6 +714,20 @@ static void sky2_phy_power_down(struct s
 	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_OFF);
 }
 
+/* configure IPG according to used link speed */
+static void sky2_set_ipg(struct sky2_port *sky2)
+{
+	u16 reg;
+
+	reg = gma_read16(sky2->hw, sky2->port, GM_SERIAL_MODE);
+	reg &= ~GM_SMOD_IPG_MSK;
+	if (sky2->speed > SPEED_100)
+		reg |= IPG_DATA_VAL(IPG_DATA_DEF_1000);
+	else
+		reg |= IPG_DATA_VAL(IPG_DATA_DEF_10_100);
+	gma_write16(sky2->hw, sky2->port, GM_SERIAL_MODE, reg);
+}
+
 /* Enable Rx/Tx */
 static void sky2_enable_rx_tx(struct sky2_port *sky2)
 {
@@ -882,7 +896,7 @@ static void sky2_mac_init(struct sky2_hw
 
 	/* serial mode register */
 	reg = DATA_BLIND_VAL(DATA_BLIND_DEF) |
-		GM_SMOD_VLAN_ENA | IPG_DATA_VAL(IPG_DATA_DEF);
+		GM_SMOD_VLAN_ENA | IPG_DATA_VAL(IPG_DATA_DEF_1000);
 
 	if (hw->dev[port]->mtu > ETH_DATA_LEN)
 		reg |= GM_SMOD_JUMBO_ENA;
@@ -2053,6 +2067,8 @@ static void sky2_link_up(struct sky2_por
 		[FC_BOTH]	= "both",
 	};
 
+	sky2_set_ipg(sky2);
+
 	sky2_enable_rx_tx(sky2);
 
 	gm_phy_write(hw, port, PHY_MARV_INT_MASK, PHY_M_DEF_MSK);
@@ -2290,8 +2306,11 @@ static int sky2_change_mtu(struct net_de
 	dev->mtu = new_mtu;
 	netdev_update_features(dev);
 
-	mode = DATA_BLIND_VAL(DATA_BLIND_DEF) |
-		GM_SMOD_VLAN_ENA | IPG_DATA_VAL(IPG_DATA_DEF);
+	mode = DATA_BLIND_VAL(DATA_BLIND_DEF) |	GM_SMOD_VLAN_ENA;
+	if (sky2->speed > SPEED_100)
+		mode |= IPG_DATA_VAL(IPG_DATA_DEF_1000);
+	else
+		mode |= IPG_DATA_VAL(IPG_DATA_DEF_10_100);
 
 	if (dev->mtu > ETH_DATA_LEN)
 		mode |= GM_SMOD_JUMBO_ENA;
--- a/drivers/net/sky2.h	2011-07-07 08:36:37.372004595 -0700
+++ b/drivers/net/sky2.h	2011-07-07 08:36:42.420004801 -0700
@@ -1807,10 +1807,11 @@ enum {
 };
 
 #define DATA_BLIND_VAL(x)	(((x)<<11) & GM_SMOD_DATABL_MSK)
-#define DATA_BLIND_DEF		0x04
-
 #define IPG_DATA_VAL(x)		(x & GM_SMOD_IPG_MSK)
-#define IPG_DATA_DEF		0x1e
+
+#define DATA_BLIND_DEF		0x04
+#define IPG_DATA_DEF_1000	0x1e
+#define IPG_DATA_DEF_10_100	0x18
 
 /*	GM_SMI_CTRL			16 bit r/w	SMI Control Register */
 enum {

[PATCH net-next 3/5] sky2: support for new Optima chipsets (EXPERIMENTAL)

[Stephen Hemminger]

[-- Attachment #1: sky2-op2.patch --]
[-- Type: text/plain, Size: 6749 bytes --]

This is a backport from the vendor driver of support for the newer Optima
(Prime and 2) chipsets. It also includes some setup changes for the 
current Optima chip as well. The code and comments intentionally
mirror the vendor sk98lin driver to allow for easier maintenance.

Although this adds support for new chip id's, these chip id's are not
used by any of the current PCI device id's listed in the driver.
The patch is just to get initial infrastructure in place to handle them
when they come.

I don't have access to any of this hardware to actually test it yet.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2011-07-07 08:36:42.420004801 -0700
+++ b/drivers/net/sky2.c	2011-07-07 08:36:44.988004931 -0700
@@ -365,6 +365,17 @@ static void sky2_phy_init(struct sky2_hw
 				gm_phy_write(hw, port, PHY_MARV_FE_SPEC_2, spec);
 			}
 		} else {
+			if (hw->chip_id >= CHIP_ID_YUKON_OPT) {
+				u16 ctrl2 = gm_phy_read(hw, port, PHY_MARV_EXT_CTRL_2);
+
+				/* enable PHY Reverse Auto-Negotiation */
+				ctrl2 |= 1u << 13;
+
+				/* Write PHY changes (SW-reset must follow) */
+				gm_phy_write(hw, port, PHY_MARV_EXT_CTRL_2, ctrl2);
+			}
+
+
 			/* disable energy detect */
 			ctrl &= ~PHY_M_PC_EN_DET_MSK;
 
@@ -626,6 +637,63 @@ static void sky2_phy_init(struct sky2_hw
 		if (ledover)
 			gm_phy_write(hw, port, PHY_MARV_LED_OVER, ledover);
 
+	} else if (hw->chip_id == CHIP_ID_YUKON_PRM &&
+		   (sky2_read8(hw, B2_MAC_CFG) & 0xf) == 0x7) {
+		int i;
+		/* This a phy register setup workaround copied from vendor driver. */
+		static const struct {
+			u16 reg, val;
+		} eee_afe[] = {
+			{ 0x156, 0x58ce },
+			{ 0x153, 0x99eb },
+			{ 0x141, 0x8064 },
+			/* { 0x155, 0x130b },*/
+			{ 0x000, 0x0000 },
+			{ 0x151, 0x8433 },
+			{ 0x14b, 0x8c44 },
+			{ 0x14c, 0x0f90 },
+			{ 0x14f, 0x39aa },
+			/* { 0x154, 0x2f39 },*/
+			{ 0x14d, 0xba33 },
+			{ 0x144, 0x0048 },
+			{ 0x152, 0x2010 },
+			/* { 0x158, 0x1223 },*/
+			{ 0x140, 0x4444 },
+			{ 0x154, 0x2f3b },
+			{ 0x158, 0xb203 },
+			{ 0x157, 0x2029 },
+		};
+
+		/* Start Workaround for OptimaEEE Rev.Z0 */
+		gm_phy_write(hw, port, PHY_MARV_EXT_ADR, 0x00fb);
+
+		gm_phy_write(hw, port,  1, 0x4099);
+		gm_phy_write(hw, port,  3, 0x1120);
+		gm_phy_write(hw, port, 11, 0x113c);
+		gm_phy_write(hw, port, 14, 0x8100);
+		gm_phy_write(hw, port, 15, 0x112a);
+		gm_phy_write(hw, port, 17, 0x1008);
+
+		gm_phy_write(hw, port, PHY_MARV_EXT_ADR, 0x00fc);
+		gm_phy_write(hw, port,  1, 0x20b0);
+
+		gm_phy_write(hw, port, PHY_MARV_EXT_ADR, 0x00ff);
+
+		for (i = 0; i < ARRAY_SIZE(eee_afe); i++) {
+			/* apply AFE settings */
+			gm_phy_write(hw, port, 17, eee_afe[i].val);
+			gm_phy_write(hw, port, 16, eee_afe[i].reg | 1u<<13);
+		}
+
+		/* End Workaround for OptimaEEE */
+		gm_phy_write(hw, port, PHY_MARV_EXT_ADR, 0);
+
+		/* Enable 10Base-Te (EEE) */
+		if (hw->chip_id >= CHIP_ID_YUKON_PRM) {
+			reg = gm_phy_read(hw, port, PHY_MARV_EXT_CTRL);
+			gm_phy_write(hw, port, PHY_MARV_EXT_CTRL,
+				     reg | PHY_M_10B_TE_ENABLE);
+		}
 	}
 
 	/* Enable phy interrupt on auto-negotiation complete (or link up) */
@@ -2959,6 +3027,8 @@ static u32 sky2_mhz(const struct sky2_hw
 	case CHIP_ID_YUKON_SUPR:
 	case CHIP_ID_YUKON_UL_2:
 	case CHIP_ID_YUKON_OPT:
+	case CHIP_ID_YUKON_PRM:
+	case CHIP_ID_YUKON_OP_2:
 		return 125;
 
 	case CHIP_ID_YUKON_FE:
@@ -3064,6 +3134,8 @@ static int __devinit sky2_init(struct sk
 		break;
 
 	case CHIP_ID_YUKON_OPT:
+	case CHIP_ID_YUKON_PRM:
+	case CHIP_ID_YUKON_OP_2:
 		hw->flags = SKY2_HW_GIGABIT
 			| SKY2_HW_NEW_LE
 			| SKY2_HW_ADV_POWER_CTL;
@@ -3163,30 +3235,33 @@ static void sky2_reset(struct sky2_hw *h
 		sky2_pci_write32(hw, PCI_DEV_REG3, P_CLK_MACSEC_DIS);
 	}
 
-	if (hw->chip_id == CHIP_ID_YUKON_OPT) {
+	if (hw->chip_id == CHIP_ID_YUKON_OPT ||
+	    hw->chip_id == CHIP_ID_YUKON_PRM ||
+	    hw->chip_id == CHIP_ID_YUKON_OP_2) {
 		u16 reg;
 		u32 msk;
 
-		if (hw->chip_rev == 0) {
+		if (hw->chip_id == CHIP_ID_YUKON_OPT && hw->chip_rev == 0) {
 			/* disable PCI-E PHY power down (set PHY reg 0x80, bit 7 */
 			sky2_write32(hw, Y2_PEX_PHY_DATA, (0x80UL << 16) | (1 << 7));
 
 			/* set PHY Link Detect Timer to 1.1 second (11x 100ms) */
 			reg = 10;
+
+			/* re-enable PEX PM in PEX PHY debug reg. 8 (clear bit 12) */
+			sky2_write32(hw, Y2_PEX_PHY_DATA, PEX_DB_ACCESS | (0x08UL << 16));
 		} else {
 			/* set PHY Link Detect Timer to 0.4 second (4x 100ms) */
 			reg = 3;
 		}
 
 		reg <<= PSM_CONFIG_REG4_TIMER_PHY_LINK_DETECT_BASE;
+		reg |= PSM_CONFIG_REG4_RST_PHY_LINK_DETECT;
 
 		/* reset PHY Link Detect */
 		sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_ON);
-		sky2_pci_write16(hw, PSM_CONFIG_REG4,
-				 reg | PSM_CONFIG_REG4_RST_PHY_LINK_DETECT);
 		sky2_pci_write16(hw, PSM_CONFIG_REG4, reg);
 
-
 		/* enable PHY Quick Link */
 		msk = sky2_read32(hw, B0_IMSK);
 		msk |= Y2_IS_PHY_QLNK;
@@ -4705,9 +4780,11 @@ static const char *sky2_name(u8 chipid,
 		"UL 2",		/* 0xba */
 		"Unknown",	/* 0xbb */
 		"Optima",	/* 0xbc */
+		"Optima Prime", /* 0xbd */
+		"Optima 2",	/* 0xbe */
 	};
 
-	if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OPT)
+	if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2)
 		strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz);
 	else
 		snprintf(buf, sz, "(chip %#x)", chipid);
--- a/drivers/net/sky2.h	2011-07-07 08:36:42.420004801 -0700
+++ b/drivers/net/sky2.h	2011-07-07 08:36:44.992004932 -0700
@@ -412,7 +412,7 @@ enum {
 	Y2_IS_HW_ERR	= 1<<31,	/* Interrupt HW Error */
 	Y2_IS_STAT_BMU	= 1<<30,	/* Status BMU Interrupt */
 	Y2_IS_ASF	= 1<<29,	/* ASF subsystem Interrupt */
-
+	Y2_IS_CPU_TO	= 1<<28,	/* CPU Timeout */
 	Y2_IS_POLL_CHK	= 1<<27,	/* Check IRQ from polling unit */
 	Y2_IS_TWSI_RDY	= 1<<26,	/* IRQ on end of TWSI Tx */
 	Y2_IS_IRQ_SW	= 1<<25,	/* SW forced IRQ	*/
@@ -547,6 +547,8 @@ enum {
 	CHIP_ID_YUKON_SUPR = 0xb9, /* YUKON-2 Supreme */
 	CHIP_ID_YUKON_UL_2 = 0xba, /* YUKON-2 Ultra 2 */
 	CHIP_ID_YUKON_OPT  = 0xbc, /* YUKON-2 Optima */
+	CHIP_ID_YUKON_PRM  = 0xbd, /* YUKON-2 Optima Prime */
+	CHIP_ID_YUKON_OP_2 = 0xbe, /* YUKON-2 Optima 2 */
 };
 
 enum yukon_xl_rev {
@@ -1420,8 +1422,10 @@ enum {
 	PHY_M_EC_FIB_AN_ENA = 1<<3, /* Fiber Auto-Neg. Enable (88E1011S only) */
 	PHY_M_EC_DTE_D_ENA  = 1<<2, /* DTE Detect Enable (88E1111 only) */
 	PHY_M_EC_TX_TIM_CT  = 1<<1, /* RGMII Tx Timing Control */
-	PHY_M_EC_TRANS_DIS  = 1<<0, /* Transmitter Disable (88E1111 only) */};
+	PHY_M_EC_TRANS_DIS  = 1<<0, /* Transmitter Disable (88E1111 only) */
 
+	PHY_M_10B_TE_ENABLE = 1<<7, /* 10Base-Te Enable (88E8079 and above) */
+};
 #define PHY_M_EC_M_DSC(x)	((u16)(x)<<10 & PHY_M_EC_M_DSC_MSK)
 					/* 00=1x; 01=2x; 10=3x; 11=4x */
 #define PHY_M_EC_S_DSC(x)	((u16)(x)<<8 & PHY_M_EC_S_DSC_MSK)

[PATCH net-next 4/5] sky2: version 1.29

[Stephen Hemminger]

[-- Attachment #1: sky2-version.patch --]
[-- Type: text/plain, Size: 445 bytes --]

Since new hardware chip support was added bump version.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sky2.c	2011-07-07 08:38:11.208009500 -0700
+++ b/drivers/net/sky2.c	2011-07-07 08:38:21.612001463 -0700
@@ -50,7 +50,7 @@
 #include "sky2.h"
 
 #define DRV_NAME		"sky2"
-#define DRV_VERSION		"1.28"
+#define DRV_VERSION		"1.29"
 
 /*
  * The Yukon II chipset takes 64 bit command blocks (called list elements)

[PATCH net-next 5/5] skge/sky2: change config references to Marvell

[Stephen Hemminger]

[-- Attachment #1: marvell.patch --]
[-- Type: text/plain, Size: 1498 bytes --]

Change references to SysKonnect in Kconfig to Marvell since
SysKonnect was acquired by Marvell back in 2002.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/Kconfig	2011-07-07 08:39:18.456011945 -0700
+++ b/drivers/net/Kconfig	2011-07-07 08:43:41.372026416 -0700
@@ -2272,7 +2272,7 @@ config SIS190
 	  will be called sis190.  This is recommended.
 
 config SKGE
-	tristate "SysKonnect GigaEthernet support"
+	tristate "Marvell Yukon Gigabit Ethernet support"
 	depends on PCI
 	select CRC32
 	---help---
@@ -2288,7 +2288,7 @@ config SKGE
 	  Linksys EG1032/EG1064, 3Com 3C940/3C940B, SysKonnect SK-9871/9872.
 
 	  It does not support the newer Yukon2 chipset: a separate driver,
-	  sky2, is provided for Yukon2-based adapters.
+	  sky2, is provided for these adapters.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called skge.  This is recommended.
@@ -2313,7 +2313,7 @@ config SKGE_GENESIS
 	 by making code smaller. If unsure say Y.
 
 config SKY2
-	tristate "SysKonnect Yukon2 support"
+	tristate "Marvell Yukon 2 support"
 	depends on PCI
 	select CRC32
 	---help---
@@ -2323,7 +2323,7 @@ config SKY2
 	  88E8053/88E8055/88E8061/88E8062, SysKonnect SK-9E21D/SK-9S21
 
 	  There is companion driver for the older Marvell Yukon and
-	  Genesis based adapters: skge.
+	  SysKonnect Genesis based adapters: skge.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called sky2.  This is recommended.

Re: [PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware

[Michał Mirosław]

2011/7/7 Stephen Hemminger <shemminger@vyatta.com>:
> Found when reviewing the vendor driver. Apparently some chip versions
> require receive checksumming to be enabled in order for RSS to work.
> Rather than silently enabling checksumming which is what the vendor
> driver does, return an error to the user instead.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
>
> --- a/drivers/net/sky2.c        2011-07-07 08:36:10.748003369 -0700
> +++ b/drivers/net/sky2.c        2011-07-07 08:36:37.372004595 -0700
> @@ -2996,7 +2996,8 @@ static int __devinit sky2_init(struct sk
>                hw->flags = SKY2_HW_GIGABIT
>                        | SKY2_HW_NEWER_PHY
>                        | SKY2_HW_NEW_LE
> -                       | SKY2_HW_ADV_POWER_CTL;
> +                       | SKY2_HW_ADV_POWER_CTL
> +                       | SKY2_HW_RSS_CHKSUM;
>
>                /* New transmit checksum */
>                if (hw->chip_rev != CHIP_REV_YU_EX_B0)
> @@ -3024,7 +3025,7 @@ static int __devinit sky2_init(struct sk
>
>                /* The workaround for status conflicts VLAN tag detection. */
>                if (hw->chip_rev == CHIP_REV_YU_FE2_A0)
> -                       hw->flags |= SKY2_HW_VLAN_BROKEN;
> +                       hw->flags |= SKY2_HW_VLAN_BROKEN | SKY2_HW_RSS_CHKSUM;
>                break;
>
>        case CHIP_ID_YUKON_SUPR:
> @@ -3033,6 +3034,9 @@ static int __devinit sky2_init(struct sk
>                        | SKY2_HW_NEW_LE
>                        | SKY2_HW_AUTO_TX_SUM
>                        | SKY2_HW_ADV_POWER_CTL;
> +
> +               if (hw->chip_rev == CHIP_REV_YU_SU_A0)
> +                       hw->flags |= SKY2_HW_RSS_CHKSUM;
>                break;
>
>        case CHIP_ID_YUKON_UL_2:
> @@ -4187,6 +4191,11 @@ static int sky2_set_features(struct net_
>        struct sky2_port *sky2 = netdev_priv(dev);
>        u32 changed = dev->features ^ features;
>
> +       /* Some hardware requires receive checksum for RSS to work. */
> +       if ( (features & (NETIF_F_RXHASH|NETIF_F_RXCSUM)) == NETIF_F_RXHASH &&
> +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM))
> +               return -EINVAL;
> +
>        if (changed & NETIF_F_RXCSUM) {
>                u32 on = features & NETIF_F_RXCSUM;
>                sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),

Nah. This won't work as the error is not passed to user (except via
dmesg) as there is no way to do it. Failing ndo_set_features() without
updating dev->features will also prevent other unrelated changes to
features set.

This should either force RXCSUM when RXHASH is on or disable RXHASH
when RXCSUM is off (both in sky2_fix_features()). Or maybe enable
RXCSUM with RXHASH but ignore hardware-computed checksum when it's not
requested.

Best Regards,
Michał Mirosław

Re: [PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware

[Stephen Hemminger]

On Thu, 7 Jul 2011 19:04:46 +0200
Michał Mirosław <mirqus@gmail.com> wrote:

> 2011/7/7 Stephen Hemminger <shemminger@vyatta.com>:
> > Found when reviewing the vendor driver. Apparently some chip versions
> > require receive checksumming to be enabled in order for RSS to work.
> > Rather than silently enabling checksumming which is what the vendor
> > driver does, return an error to the user instead.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> >
> > --- a/drivers/net/sky2.c        2011-07-07 08:36:10.748003369 -0700
> > +++ b/drivers/net/sky2.c        2011-07-07 08:36:37.372004595 -0700
> > @@ -2996,7 +2996,8 @@ static int __devinit sky2_init(struct sk
> >                hw->flags = SKY2_HW_GIGABIT
> >                        | SKY2_HW_NEWER_PHY
> >                        | SKY2_HW_NEW_LE
> > -                       | SKY2_HW_ADV_POWER_CTL;
> > +                       | SKY2_HW_ADV_POWER_CTL
> > +                       | SKY2_HW_RSS_CHKSUM;
> >
> >                /* New transmit checksum */
> >                if (hw->chip_rev != CHIP_REV_YU_EX_B0)
> > @@ -3024,7 +3025,7 @@ static int __devinit sky2_init(struct sk
> >
> >                /* The workaround for status conflicts VLAN tag detection. */
> >                if (hw->chip_rev == CHIP_REV_YU_FE2_A0)
> > -                       hw->flags |= SKY2_HW_VLAN_BROKEN;
> > +                       hw->flags |= SKY2_HW_VLAN_BROKEN | SKY2_HW_RSS_CHKSUM;
> >                break;
> >
> >        case CHIP_ID_YUKON_SUPR:
> > @@ -3033,6 +3034,9 @@ static int __devinit sky2_init(struct sk
> >                        | SKY2_HW_NEW_LE
> >                        | SKY2_HW_AUTO_TX_SUM
> >                        | SKY2_HW_ADV_POWER_CTL;
> > +
> > +               if (hw->chip_rev == CHIP_REV_YU_SU_A0)
> > +                       hw->flags |= SKY2_HW_RSS_CHKSUM;
> >                break;
> >
> >        case CHIP_ID_YUKON_UL_2:
> > @@ -4187,6 +4191,11 @@ static int sky2_set_features(struct net_
> >        struct sky2_port *sky2 = netdev_priv(dev);
> >        u32 changed = dev->features ^ features;
> >
> > +       /* Some hardware requires receive checksum for RSS to work. */
> > +       if ( (features & (NETIF_F_RXHASH|NETIF_F_RXCSUM)) == NETIF_F_RXHASH &&
> > +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM))
> > +               return -EINVAL;
> > +
> >        if (changed & NETIF_F_RXCSUM) {
> >                u32 on = features & NETIF_F_RXCSUM;
> >                sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
> 
> Nah. This won't work as the error is not passed to user (except via
> dmesg) as there is no way to do it. Failing ndo_set_features() without
> updating dev->features will also prevent other unrelated changes to
> features set.

I think this needs to be fixed in the netdev core then.
There are bound to be hardware types that can support only some combinations
of features. It should be passed back like all other errors.

Re: [PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware

[Michał Mirosław]

W dniu 7 lipca 2011 19:13 użytkownik Stephen Hemminger
<shemminger@vyatta.com> napisał:
> On Thu, 7 Jul 2011 19:04:46 +0200
> Michał Mirosław <mirqus@gmail.com> wrote:
>
>> 2011/7/7 Stephen Hemminger <shemminger@vyatta.com>:
>> > Found when reviewing the vendor driver. Apparently some chip versions
>> > require receive checksumming to be enabled in order for RSS to work.
>> > Rather than silently enabling checksumming which is what the vendor
>> > driver does, return an error to the user instead.
>> >
>> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>> >
>> >
>> > --- a/drivers/net/sky2.c        2011-07-07 08:36:10.748003369 -0700
>> > +++ b/drivers/net/sky2.c        2011-07-07 08:36:37.372004595 -0700
>> > @@ -2996,7 +2996,8 @@ static int __devinit sky2_init(struct sk
>> >                hw->flags = SKY2_HW_GIGABIT
>> >                        | SKY2_HW_NEWER_PHY
>> >                        | SKY2_HW_NEW_LE
>> > -                       | SKY2_HW_ADV_POWER_CTL;
>> > +                       | SKY2_HW_ADV_POWER_CTL
>> > +                       | SKY2_HW_RSS_CHKSUM;
>> >
>> >                /* New transmit checksum */
>> >                if (hw->chip_rev != CHIP_REV_YU_EX_B0)
>> > @@ -3024,7 +3025,7 @@ static int __devinit sky2_init(struct sk
>> >
>> >                /* The workaround for status conflicts VLAN tag detection. */
>> >                if (hw->chip_rev == CHIP_REV_YU_FE2_A0)
>> > -                       hw->flags |= SKY2_HW_VLAN_BROKEN;
>> > +                       hw->flags |= SKY2_HW_VLAN_BROKEN | SKY2_HW_RSS_CHKSUM;
>> >                break;
>> >
>> >        case CHIP_ID_YUKON_SUPR:
>> > @@ -3033,6 +3034,9 @@ static int __devinit sky2_init(struct sk
>> >                        | SKY2_HW_NEW_LE
>> >                        | SKY2_HW_AUTO_TX_SUM
>> >                        | SKY2_HW_ADV_POWER_CTL;
>> > +
>> > +               if (hw->chip_rev == CHIP_REV_YU_SU_A0)
>> > +                       hw->flags |= SKY2_HW_RSS_CHKSUM;
>> >                break;
>> >
>> >        case CHIP_ID_YUKON_UL_2:
>> > @@ -4187,6 +4191,11 @@ static int sky2_set_features(struct net_
>> >        struct sky2_port *sky2 = netdev_priv(dev);
>> >        u32 changed = dev->features ^ features;
>> >
>> > +       /* Some hardware requires receive checksum for RSS to work. */
>> > +       if ( (features & (NETIF_F_RXHASH|NETIF_F_RXCSUM)) == NETIF_F_RXHASH &&
>> > +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM))
>> > +               return -EINVAL;
>> > +
>> >        if (changed & NETIF_F_RXCSUM) {
>> >                u32 on = features & NETIF_F_RXCSUM;
>> >                sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
>>
>> Nah. This won't work as the error is not passed to user (except via
>> dmesg) as there is no way to do it. Failing ndo_set_features() without
>> updating dev->features will also prevent other unrelated changes to
>> features set.
>
> I think this needs to be fixed in the netdev core then.
> There are bound to be hardware types that can support only some combinations
> of features. It should be passed back like all other errors.

This is what ndo_fix_features callback is for - it should alter passed
feature set to what combinations are permitted for the device's
current state.

Updating of the features is not always initiated by user (it might be
because, e.g. link speed changed) - in those cases the error has no
place to go.

Best Regards,
Michał Mirosław

[PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware (v2)

[Stephen Hemminger]

Found when reviewing the vendor driver. Apparently some chip versions
require receive checksumming to be enabled in order for RSS to work.

Also, if fix_features has to change some settings; put in message
in log in similar manner to netdev_fix_features.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
v2 - enforce the requirement in fix_features rather than set features

--- a/drivers/net/sky2.c	2011-07-07 13:56:03.575420273 -0700
+++ b/drivers/net/sky2.c	2011-07-07 14:03:17.775403938 -0700
@@ -2996,7 +2996,8 @@ static int __devinit sky2_init(struct sk
 		hw->flags = SKY2_HW_GIGABIT
 			| SKY2_HW_NEWER_PHY
 			| SKY2_HW_NEW_LE
-			| SKY2_HW_ADV_POWER_CTL;
+			| SKY2_HW_ADV_POWER_CTL
+			| SKY2_HW_RSS_CHKSUM;
 
 		/* New transmit checksum */
 		if (hw->chip_rev != CHIP_REV_YU_EX_B0)
@@ -3024,7 +3025,7 @@ static int __devinit sky2_init(struct sk
 
 		/* The workaround for status conflicts VLAN tag detection. */
 		if (hw->chip_rev == CHIP_REV_YU_FE2_A0)
-			hw->flags |= SKY2_HW_VLAN_BROKEN;
+			hw->flags |= SKY2_HW_VLAN_BROKEN | SKY2_HW_RSS_CHKSUM;
 		break;
 
 	case CHIP_ID_YUKON_SUPR:
@@ -3033,6 +3034,9 @@ static int __devinit sky2_init(struct sk
 			| SKY2_HW_NEW_LE
 			| SKY2_HW_AUTO_TX_SUM
 			| SKY2_HW_ADV_POWER_CTL;
+
+		if (hw->chip_rev == CHIP_REV_YU_SU_A0)
+			hw->flags |= SKY2_HW_RSS_CHKSUM;
 		break;
 
 	case CHIP_ID_YUKON_UL_2:
@@ -4176,8 +4180,18 @@ static u32 sky2_fix_features(struct net_
 	/* In order to do Jumbo packets on these chips, need to turn off the
 	 * transmit store/forward. Therefore checksum offload won't work.
 	 */
-	if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U)
+	if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U) {
+		netdev_warn(dev, "checksum offload not possible with jumbo frames\n");
 		features &= ~(NETIF_F_TSO|NETIF_F_SG|NETIF_F_ALL_CSUM);
+	}
+
+	/* Some hardware requires receive checksum for RSS to work. */
+	if ( (features & NETIF_F_RXHASH) &&
+	     !(features & NETIF_F_RXCSUM) &&
+	     (sky2->hw->flags & SKY2_HW_RSS_CHKSUM)) {
+		netdev_warn(dev, "receive hashing forces receive checksum\n");
+		features |= NETIF_F_RXCSUM;
+	}
 
 	return features;
 }
--- a/drivers/net/sky2.h	2011-07-07 13:56:03.563420272 -0700
+++ b/drivers/net/sky2.h	2011-07-07 13:56:07.719420117 -0700
@@ -2281,6 +2281,7 @@ struct sky2_hw {
 #define SKY2_HW_ADV_POWER_CTL	0x00000080	/* additional PHY power regs */
 #define SKY2_HW_RSS_BROKEN	0x00000100
 #define SKY2_HW_VLAN_BROKEN     0x00000200
+#define SKY2_HW_RSS_CHKSUM	0x00000400	/* RSS requires chksum */
 
 	u8	     	     chip_id;
 	u8		     chip_rev;

Re: [PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware (v2)

[Michał Mirosław]

2011/7/8 Stephen Hemminger <shemminger@vyatta.com>:
> Found when reviewing the vendor driver. Apparently some chip versions
> require receive checksumming to be enabled in order for RSS to work.
>
> Also, if fix_features has to change some settings; put in message
> in log in similar manner to netdev_fix_features.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
> ---
> v2 - enforce the requirement in fix_features rather than set features
>
> --- a/drivers/net/sky2.c        2011-07-07 13:56:03.575420273 -0700
> +++ b/drivers/net/sky2.c        2011-07-07 14:03:17.775403938 -0700
[...]
> @@ -4176,8 +4180,18 @@ static u32 sky2_fix_features(struct net_
>        /* In order to do Jumbo packets on these chips, need to turn off the
>         * transmit store/forward. Therefore checksum offload won't work.
>         */
> -       if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U)
> +       if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U) {
> +               netdev_warn(dev, "checksum offload not possible with jumbo frames\n");
>                features &= ~(NETIF_F_TSO|NETIF_F_SG|NETIF_F_ALL_CSUM);
> +       }
> +
> +       /* Some hardware requires receive checksum for RSS to work. */
> +       if ( (features & NETIF_F_RXHASH) &&
> +            !(features & NETIF_F_RXCSUM) &&
> +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM)) {
> +               netdev_warn(dev, "receive hashing forces receive checksum\n");
> +               features |= NETIF_F_RXCSUM;
> +       }
>
>        return features;
>  }

I suggest using netdev_dbg() instead of netdev_warn() so it won't spam
dmesg on every features update.

Otherwise, looks good.

Best Regards,
Michał Mirosław

Re: [PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware (v2)

[Stephen Hemminger]

On Fri, 8 Jul 2011 10:45:33 +0200
Michał Mirosław <mirqus@gmail.com> wrote:

> 2011/7/8 Stephen Hemminger <shemminger@vyatta.com>:
> > Found when reviewing the vendor driver. Apparently some chip versions
> > require receive checksumming to be enabled in order for RSS to work.
> >
> > Also, if fix_features has to change some settings; put in message
> > in log in similar manner to netdev_fix_features.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> > ---
> > v2 - enforce the requirement in fix_features rather than set features
> >
> > --- a/drivers/net/sky2.c        2011-07-07 13:56:03.575420273 -0700
> > +++ b/drivers/net/sky2.c        2011-07-07 14:03:17.775403938 -0700
> [...]
> > @@ -4176,8 +4180,18 @@ static u32 sky2_fix_features(struct net_
> >        /* In order to do Jumbo packets on these chips, need to turn off the
> >         * transmit store/forward. Therefore checksum offload won't work.
> >         */
> > -       if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U)
> > +       if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U) {
> > +               netdev_warn(dev, "checksum offload not possible with jumbo frames\n");
> >                features &= ~(NETIF_F_TSO|NETIF_F_SG|NETIF_F_ALL_CSUM);
> > +       }
> > +
> > +       /* Some hardware requires receive checksum for RSS to work. */
> > +       if ( (features & NETIF_F_RXHASH) &&
> > +            !(features & NETIF_F_RXCSUM) &&
> > +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM)) {
> > +               netdev_warn(dev, "receive hashing forces receive checksum\n");
> > +               features |= NETIF_F_RXCSUM;
> > +       }
> >
> >        return features;
> >  }
> 
> I suggest using netdev_dbg() instead of netdev_warn() so it won't spam
> dmesg on every features update.

I was just copying existing policy in netdev_fix_features.
netdev_dbg() is useless because most user turn off debug messages.
Maybe notice or info is more appropriate level here.

Re: [PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware (v2)

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 7 Jul 2011 16:40:00 -0700

> Found when reviewing the vendor driver. Apparently some chip versions
> require receive checksumming to be enabled in order for RSS to work.
> 
> Also, if fix_features has to change some settings; put in message
> in log in similar manner to netdev_fix_features.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> v2 - enforce the requirement in fix_features rather than set features

All 5 patches applied, I changed this patch #1 to use netdev_info()
as per the discussion.