[PATCH net-next 0/5] sh_eth changes for net-next

[PATCH net-next 0/5] sh_eth changes for net-next

[Ben Hutchings]

Some minor new features and fixes.

These depend in part on the series I sent earlier for net, specifically
"sh_eth: WARN on access to a register not implemented in a particular
chip" depends on "sh_eth: Fix RX recovery on R-Car in case of RX ring
underrun".

Ben.

Ben Hutchings (5):
  sh_eth: Implement multicast statistic based on the RFS8 status bit
  sh_eth: WARN on access to a register not implemented in a particular
    chip
  sh_eth: Implement ethtool register dump operations
  sh_eth: Optionally log RX and TX status for each completed descriptor
  sh_eth: Mitigate lost statistics updates

 drivers/net/ethernet/renesas/sh_eth.c |  259 ++++++++++++++++++++++++++++++---
 drivers/net/ethernet/renesas/sh_eth.h |   23 ++-
 2 files changed, 256 insertions(+), 26 deletions(-)

-- 
1.7.10.4

[PATCH net-next 1/5] sh_eth: Implement multicast statistic based on the RFS8 status bit

[Ben Hutchings]

At least on the R8A7790, RFS8 reflects the RINT8 (multicast) MAC
status flag.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
This should probably wait for confirmation that RFS8 consistently
indicates a multicast frame.

Ben.

 drivers/net/ethernet/renesas/sh_eth.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 07c537d900da..4169f93b5c3c 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1497,6 +1497,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 			netif_receive_skb(skb);
 			ndev->stats.rx_packets++;
 			ndev->stats.rx_bytes += pkt_len;
+			if (desc_status & RD_RFS8)
+				ndev->stats.multicast++;
 		}
 		entry = (++mdp->cur_rx) % mdp->num_rx_ring;
 		rxdesc = &mdp->rx_ring[entry];
-- 
1.7.10.4

[PATCH net-next 2/5] sh_eth: WARN on access to a register not implemented in a particular chip

[Ben Hutchings]

Currently we may silently read/write a register at offset 0.  Change
this to WARN and then ignore the write or read-back all-ones.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |   16 +++++++++++++++-
 drivers/net/ethernet/renesas/sh_eth.h |   14 ++++++++++++--
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 4169f93b5c3c..e21f8ce609cd 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -52,7 +52,12 @@
 		NETIF_MSG_RX_ERR| \
 		NETIF_MSG_TX_ERR)
 
+#define SH_ETH_OFFSET_DEFAULTS			\
+	[0 ... SH_ETH_MAX_REGISTER_OFFSET - 1] = SH_ETH_OFFSET_INVALID
+
 static const u16 sh_eth_offset_gigabit[SH_ETH_MAX_REGISTER_OFFSET] = {
+	SH_ETH_OFFSET_DEFAULTS,
+
 	[EDSR]		= 0x0000,
 	[EDMR]		= 0x0400,
 	[EDTRR]		= 0x0408,
@@ -151,6 +156,8 @@ static const u16 sh_eth_offset_gigabit[SH_ETH_MAX_REGISTER_OFFSET] = {
 };
 
 static const u16 sh_eth_offset_fast_rz[SH_ETH_MAX_REGISTER_OFFSET] = {
+	SH_ETH_OFFSET_DEFAULTS,
+
 	[EDSR]		= 0x0000,
 	[EDMR]		= 0x0400,
 	[EDTRR]		= 0x0408,
@@ -210,6 +217,8 @@ static const u16 sh_eth_offset_fast_rz[SH_ETH_MAX_REGISTER_OFFSET] = {
 };
 
 static const u16 sh_eth_offset_fast_rcar[SH_ETH_MAX_REGISTER_OFFSET] = {
+	SH_ETH_OFFSET_DEFAULTS,
+
 	[ECMR]		= 0x0300,
 	[RFLR]		= 0x0308,
 	[ECSR]		= 0x0310,
@@ -256,6 +265,8 @@ static const u16 sh_eth_offset_fast_rcar[SH_ETH_MAX_REGISTER_OFFSET] = {
 };
 
 static const u16 sh_eth_offset_fast_sh4[SH_ETH_MAX_REGISTER_OFFSET] = {
+	SH_ETH_OFFSET_DEFAULTS,
+
 	[ECMR]		= 0x0100,
 	[RFLR]		= 0x0108,
 	[ECSR]		= 0x0110,
@@ -308,6 +319,8 @@ static const u16 sh_eth_offset_fast_sh4[SH_ETH_MAX_REGISTER_OFFSET] = {
 };
 
 static const u16 sh_eth_offset_fast_sh3_sh2[SH_ETH_MAX_REGISTER_OFFSET] = {
+	SH_ETH_OFFSET_DEFAULTS,
+
 	[EDMR]		= 0x0000,
 	[EDTRR]		= 0x0004,
 	[EDRRR]		= 0x0008,
@@ -1541,7 +1554,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 	/* If we don't need to check status, don't. -KDU */
 	if (!(sh_eth_read(ndev, EDRRR) & EDRRR_R)) {
 		/* fix the values for the next receiving if RDE is set */
-		if (intr_status & EESR_RDE && mdp->reg_offset[RDFAR] != 0) {
+		if (intr_status & EESR_RDE &&
+		    mdp->reg_offset[RDFAR] != SH_ETH_OFFSET_INVALID) {
 			u32 count = (sh_eth_read(ndev, RDFAR) -
 				     sh_eth_read(ndev, RDLAR)) >> 4;
 
diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
index 259d03f353e1..33a360c4fd10 100644
--- a/drivers/net/ethernet/renesas/sh_eth.h
+++ b/drivers/net/ethernet/renesas/sh_eth.h
@@ -543,19 +543,29 @@ static inline void sh_eth_soft_swap(char *src, int len)
 #endif
 }
 
+#define SH_ETH_OFFSET_INVALID	((u16) ~0)
+
 static inline void sh_eth_write(struct net_device *ndev, u32 data,
 				int enum_index)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
+	u16 offset = mdp->reg_offset[enum_index];
+
+	if (WARN_ON(offset == SH_ETH_OFFSET_INVALID))
+		return;
 
-	iowrite32(data, mdp->addr + mdp->reg_offset[enum_index]);
+	iowrite32(data, mdp->addr + offset);
 }
 
 static inline u32 sh_eth_read(struct net_device *ndev, int enum_index)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
+	u16 offset = mdp->reg_offset[enum_index];
+
+	if (WARN_ON(offset == SH_ETH_OFFSET_INVALID))
+		return ~0U;
 
-	return ioread32(mdp->addr + mdp->reg_offset[enum_index]);
+	return ioread32(mdp->addr + offset);
 }
 
 static inline void *sh_eth_tsu_get_offset(struct sh_eth_private *mdp,
-- 
1.7.10.4

[PATCH net-next 3/5] sh_eth: Implement ethtool register dump operations

[Ben Hutchings]

There are many different sets of registers implemented by the
different versions of this controller, and we can only expect this to
get more complicated in future.  Limit how much ethtool needs to know
by including an explicit bitmap of which registers are included in the
dump, allowing room for future growth in the number of possible
registers.

As I don't have datasheets for all of these, I've only included
registers that are:

- defined in all 5 register type arrays, or
- used by the driver, or
- documented in the datasheet I have

Add one new capability flag so we can tell whether the RTRATE
register is implemented.

Delete the TSU_ADRL0 and TSU_ADR{H,L}31 definitions, as they weren't
used and the address table is already assumed to be contiguous.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |  197 +++++++++++++++++++++++++++++++--
 drivers/net/ethernet/renesas/sh_eth.h |    9 +-
 2 files changed, 195 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index e21f8ce609cd..e6248b1c3ad5 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -137,9 +137,6 @@ static const u16 sh_eth_offset_gigabit[SH_ETH_MAX_REGISTER_OFFSET] = {
 	[TSU_POST3]	= 0x0078,
 	[TSU_POST4]	= 0x007c,
 	[TSU_ADRH0]	= 0x0100,
-	[TSU_ADRL0]	= 0x0104,
-	[TSU_ADRH31]	= 0x01f8,
-	[TSU_ADRL31]	= 0x01fc,
 
 	[TXNLCR0]	= 0x0080,
 	[TXALCR0]	= 0x0084,
@@ -206,9 +203,6 @@ static const u16 sh_eth_offset_fast_rz[SH_ETH_MAX_REGISTER_OFFSET] = {
 	[TSU_ADSBSY]	= 0x0060,
 	[TSU_TEN]	= 0x0064,
 	[TSU_ADRH0]	= 0x0100,
-	[TSU_ADRL0]	= 0x0104,
-	[TSU_ADRH31]	= 0x01f8,
-	[TSU_ADRL31]	= 0x01fc,
 
 	[TXNLCR0]	= 0x0080,
 	[TXALCR0]	= 0x0084,
@@ -405,8 +399,6 @@ static const u16 sh_eth_offset_fast_sh3_sh2[SH_ETH_MAX_REGISTER_OFFSET] = {
 	[FWALCR1]	= 0x00b4,
 
 	[TSU_ADRH0]	= 0x0100,
-	[TSU_ADRL0]	= 0x0104,
-	[TSU_ADRL31]	= 0x01fc,
 };
 
 static void sh_eth_rcv_snd_disable(struct net_device *ndev);
@@ -601,6 +593,7 @@ static struct sh_eth_cpu_data sh7757_data = {
 	.no_ade		= 1,
 	.rpadir		= 1,
 	.rpadir_value   = 2 << 16,
+	.rtrate		= 1,
 };
 
 #define SH_GIGA_ETH_BASE	0xfee00000UL
@@ -1942,6 +1935,192 @@ error_exit:
 	return ret;
 }
 
+/* If it is ever necessary to increase SH_ETH_REG_DUMP_MAX_REGS, the
+ * version must be bumped as well.  Just adding registers up to that
+ * limit is fine, as long as the existing register indices don't
+ * change.
+ */
+#define SH_ETH_REG_DUMP_VERSION		1
+#define SH_ETH_REG_DUMP_MAX_REGS	256
+
+static size_t __sh_eth_get_regs(struct net_device *ndev, u32 *buf)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+	struct sh_eth_cpu_data *cd = mdp->cd;
+	u32 *valid_map;
+	size_t len;
+
+	BUILD_BUG_ON(SH_ETH_MAX_REGISTER_OFFSET > SH_ETH_REG_DUMP_MAX_REGS);
+
+	/* Dump starts with a bitmap that tells ethtool which
+	 * registers are defined for this chip.
+	 */
+	len = DIV_ROUND_UP(SH_ETH_REG_DUMP_MAX_REGS, 32);
+	if (buf) {
+		valid_map = buf;
+		buf += len;
+	} else {
+		valid_map = NULL;
+	}
+
+	/* Add a register to the dump, if it has a defined offset.
+	 * This automatically skips most undefined registers, but for
+	 * some it is also necessary to check a capability flag in
+	 * struct sh_eth_cpu_data.
+	 */
+#define mark_reg_valid(reg) valid_map[reg / 32] |= 1U << (reg % 32)
+#define add_reg_from(reg, read_expr) do {				\
+		if (mdp->reg_offset[reg] != SH_ETH_OFFSET_INVALID) {	\
+			if (buf) {					\
+				mark_reg_valid(reg);			\
+				*buf++ = read_expr;			\
+			}						\
+			++len;						\
+		}							\
+	} while (0)
+#define add_reg(reg) add_reg_from(reg, sh_eth_read(ndev, reg))
+#define add_tsu_reg(reg) add_reg_from(reg, sh_eth_tsu_read(mdp, reg))
+
+	add_reg(EDSR);
+	add_reg(EDMR);
+	add_reg(EDTRR);
+	add_reg(EDRRR);
+	add_reg(EESR);
+	add_reg(EESIPR);
+	add_reg(TDLAR);
+	add_reg(TDFAR);
+	add_reg(TDFXR);
+	add_reg(TDFFR);
+	add_reg(RDLAR);
+	add_reg(RDFAR);
+	add_reg(RDFXR);
+	add_reg(RDFFR);
+	add_reg(TRSCER);
+	add_reg(RMFCR);
+	add_reg(TFTR);
+	add_reg(FDR);
+	add_reg(RMCR);
+	add_reg(TFUCR);
+	add_reg(RFOCR);
+	if (cd->rmiimode)
+		add_reg(RMIIMODE);
+	add_reg(FCFTR);
+	if (cd->rpadir)
+		add_reg(RPADIR);
+	if (!cd->no_trimd)
+		add_reg(TRIMD);
+	add_reg(ECMR);
+	add_reg(ECSR);
+	add_reg(ECSIPR);
+	add_reg(PIR);
+	if (!cd->no_psr)
+		add_reg(PSR);
+	add_reg(RDMLR);
+	add_reg(RFLR);
+	add_reg(IPGR);
+	if (cd->apr)
+		add_reg(APR);
+	if (cd->mpr)
+		add_reg(MPR);
+	add_reg(RFCR);
+	add_reg(RFCF);
+	if (cd->tpauser)
+		add_reg(TPAUSER);
+	add_reg(TPAUSECR);
+	add_reg(GECMR);
+	if (cd->bculr)
+		add_reg(BCULR);
+	add_reg(MAHR);
+	add_reg(MALR);
+	add_reg(TROCR);
+	add_reg(CDCR);
+	add_reg(LCCR);
+	add_reg(CNDCR);
+	add_reg(CEFCR);
+	add_reg(FRECR);
+	add_reg(TSFRCR);
+	add_reg(TLFRCR);
+	add_reg(CERCR);
+	add_reg(CEECR);
+	add_reg(MAFCR);
+	if (cd->rtrate)
+		add_reg(RTRATE);
+	if (cd->hw_crc)
+		add_reg(CSMR);
+	if (cd->select_mii)
+		add_reg(RMII_MII);
+	add_reg(ARSTR);
+	if (cd->tsu) {
+		add_tsu_reg(TSU_CTRST);
+		add_tsu_reg(TSU_FWEN0);
+		add_tsu_reg(TSU_FWEN1);
+		add_tsu_reg(TSU_FCM);
+		add_tsu_reg(TSU_BSYSL0);
+		add_tsu_reg(TSU_BSYSL1);
+		add_tsu_reg(TSU_PRISL0);
+		add_tsu_reg(TSU_PRISL1);
+		add_tsu_reg(TSU_FWSL0);
+		add_tsu_reg(TSU_FWSL1);
+		add_tsu_reg(TSU_FWSLC);
+		add_tsu_reg(TSU_QTAG0);
+		add_tsu_reg(TSU_QTAG1);
+		add_tsu_reg(TSU_QTAGM0);
+		add_tsu_reg(TSU_QTAGM1);
+		add_tsu_reg(TSU_FWSR);
+		add_tsu_reg(TSU_FWINMK);
+		add_tsu_reg(TSU_ADQT0);
+		add_tsu_reg(TSU_ADQT1);
+		add_tsu_reg(TSU_VTAG0);
+		add_tsu_reg(TSU_VTAG1);
+		add_tsu_reg(TSU_ADSBSY);
+		add_tsu_reg(TSU_TEN);
+		add_tsu_reg(TSU_POST1);
+		add_tsu_reg(TSU_POST2);
+		add_tsu_reg(TSU_POST3);
+		add_tsu_reg(TSU_POST4);
+		if (mdp->reg_offset[TSU_ADRH0] != SH_ETH_OFFSET_INVALID) {
+			/* This is the start of a table, not just a single
+			 * register.
+			 */
+			if (buf) {
+				unsigned int i;
+
+				mark_reg_valid(TSU_ADRH0);
+				for (i = 0; i < SH_ETH_TSU_CAM_ENTRIES * 2; i++)
+					*buf++ = ioread32(
+						mdp->tsu_addr +
+						mdp->reg_offset[TSU_ADRH0] +
+						i * 4);
+			}
+			len += SH_ETH_TSU_CAM_ENTRIES * 2;
+		}
+	}
+
+#undef mark_reg_valid
+#undef add_reg_from
+#undef add_reg
+#undef add_tsu_reg
+
+	return len * 4;
+}
+
+static int sh_eth_get_regs_len(struct net_device *ndev)
+{
+	return __sh_eth_get_regs(ndev, NULL);
+}
+
+static void sh_eth_get_regs(struct net_device *ndev, struct ethtool_regs *regs,
+			    void *buf)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+
+	regs->version = SH_ETH_REG_DUMP_VERSION;
+
+	pm_runtime_get_sync(&mdp->pdev->dev);
+	__sh_eth_get_regs(ndev, buf);
+	pm_runtime_put_sync(&mdp->pdev->dev);
+}
+
 static int sh_eth_nway_reset(struct net_device *ndev)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
@@ -2087,6 +2266,8 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
 static const struct ethtool_ops sh_eth_ethtool_ops = {
 	.get_settings	= sh_eth_get_settings,
 	.set_settings	= sh_eth_set_settings,
+	.get_regs_len	= sh_eth_get_regs_len,
+	.get_regs	= sh_eth_get_regs,
 	.nway_reset	= sh_eth_nway_reset,
 	.get_msglevel	= sh_eth_get_msglevel,
 	.set_msglevel	= sh_eth_set_msglevel,
diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
index 33a360c4fd10..06dbbe5201cb 100644
--- a/drivers/net/ethernet/renesas/sh_eth.h
+++ b/drivers/net/ethernet/renesas/sh_eth.h
@@ -32,6 +32,10 @@
 #define SH_ETH_TSU_CAM_ENTRIES	32
 
 enum {
+	/* IMPORTANT: To keep ethtool register dump working, add new
+	 * register names immediately before SH_ETH_MAX_REGISTER_OFFSET.
+	 */
+
 	/* E-DMAC registers */
 	EDSR = 0,
 	EDMR,
@@ -131,9 +135,7 @@ enum {
 	TSU_POST3,
 	TSU_POST4,
 	TSU_ADRH0,
-	TSU_ADRL0,
-	TSU_ADRH31,
-	TSU_ADRL31,
+	/* TSU_ADR{H,L}{0..31} are assumed to be contiguous */
 
 	TXNLCR0,
 	TXALCR0,
@@ -491,6 +493,7 @@ struct sh_eth_cpu_data {
 	unsigned select_mii:1;	/* EtherC have RMII_MII (MII select register) */
 	unsigned shift_rd0:1;	/* shift Rx descriptor word 0 right by 16 */
 	unsigned rmiimode:1;	/* EtherC has RMIIMODE register */
+	unsigned rtrate:1;	/* EtherC has RTRATE register */
 };
 
 struct sh_eth_private {
-- 
1.7.10.4

[PATCH net-next 4/5] sh_eth: Optionally log RX and TX status for each completed descriptor

[Ben Hutchings]

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index e6248b1c3ad5..38b9d87b2667 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1414,6 +1414,9 @@ static int sh_eth_txfree(struct net_device *ndev)
 			break;
 		/* TACT bit must be checked before all the following reads */
 		rmb();
+		netif_info(mdp, tx_done, ndev,
+			   "tx entry %d status 0x%08x\n",
+			   entry, edmac_to_cpu(mdp, txdesc->status));
 		/* Free the original skb. */
 		if (mdp->tx_skbuff[entry]) {
 			dma_unmap_single(&ndev->dev, txdesc->addr,
@@ -1459,6 +1462,10 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 		if (--boguscnt < 0)
 			break;
 
+		netif_info(mdp, rx_status, ndev,
+			   "rx entry %d status 0x%08x len %d\n",
+			   entry, desc_status, pkt_len);
+
 		if (!(desc_status & RDFEND))
 			ndev->stats.rx_length_errors++;
 
-- 
1.7.10.4

[PATCH net-next 5/5] sh_eth: Mitigate lost statistics updates

[Ben Hutchings]

The statistics registers have write-clear behaviour, which means we
will lose any increment between the read and write.  Mitigate this by
only clearing when we read a non-zero value, so we will never falsely
report a total of zero.  This also saves time as we only handle
error statistics here and they won't often be incremented.

Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c |   37 ++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 38b9d87b2667..c3bc01ccad3e 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2414,6 +2414,22 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	return NETDEV_TX_OK;
 }
 
+/* The statistics registers have write-clear behaviour, which means we
+ * will lose any increment between the read and write.  We mitigate
+ * this by only clearing when we read a non-zero value, so we will
+ * never falsely report a total of zero.
+ */
+static void
+sh_eth_update_stat(struct net_device *ndev, unsigned long *stat, int reg)
+{
+	u32 delta = sh_eth_read(ndev, reg);
+
+	if (delta) {
+		*stat += delta;
+		sh_eth_write(ndev, 0, reg);
+	}
+}
+
 static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
@@ -2424,21 +2440,18 @@ static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev)
 	if (!mdp->is_opened)
 		return &ndev->stats;
 
-	ndev->stats.tx_dropped += sh_eth_read(ndev, TROCR);
-	sh_eth_write(ndev, 0, TROCR);	/* (write clear) */
-	ndev->stats.collisions += sh_eth_read(ndev, CDCR);
-	sh_eth_write(ndev, 0, CDCR);	/* (write clear) */
-	ndev->stats.tx_carrier_errors += sh_eth_read(ndev, LCCR);
-	sh_eth_write(ndev, 0, LCCR);	/* (write clear) */
+	sh_eth_update_stat(ndev, &ndev->stats.tx_dropped, TROCR);
+	sh_eth_update_stat(ndev, &ndev->stats.collisions, CDCR);
+	sh_eth_update_stat(ndev, &ndev->stats.tx_carrier_errors, LCCR);
 
 	if (sh_eth_is_gether(mdp)) {
-		ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CERCR);
-		sh_eth_write(ndev, 0, CERCR);	/* (write clear) */
-		ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CEECR);
-		sh_eth_write(ndev, 0, CEECR);	/* (write clear) */
+		sh_eth_update_stat(ndev, &ndev->stats.tx_carrier_errors,
+				   CERCR);
+		sh_eth_update_stat(ndev, &ndev->stats.tx_carrier_errors,
+				   CEECR);
 	} else {
-		ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CNDCR);
-		sh_eth_write(ndev, 0, CNDCR);	/* (write clear) */
+		sh_eth_update_stat(ndev, &ndev->stats.tx_carrier_errors,
+				   CNDCR);
 	}
 
 	return &ndev->stats;
-- 
1.7.10.4

Re: [PATCH net-next 2/5] sh_eth: WARN on access to a register not implemented in a particular chip

[Sergei Shtylyov]

Hello.

On 2/26/2015 11:34 PM, Ben Hutchings wrote:

> Currently we may silently read/write a register at offset 0.  Change
> this to WARN and then ignore the write or read-back all-ones.

    I think reading non-existing registers would yield all 0s, not all 1s.
That's not x86. :-)

> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>

    Other than that, seems a great idea.

Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

WBR, Sergei

Re: [PATCH net-next 0/5] sh_eth changes for net-next

[David Miller]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>
Date: Thu, 26 Feb 2015 20:31:05 +0000

> Some minor new features and fixes.
> 
> These depend in part on the series I sent earlier for net, specifically
> "sh_eth: WARN on access to a register not implemented in a particular
> chip" depends on "sh_eth: Fix RX recovery on R-Car in case of RX ring
> underrun".

Ben, in case it isn't clear, I'm waiting for you to respin your sh_eth
bug fix patch series for 'net' based upon the feedback you were given.

The longer it takes for you to resubmit a new version of that, the longer
this patch series is going to rot in patchwork and I really hate that.

Re: [PATCH net-next 0/5] sh_eth changes for net-next

[David Miller]

From: Ben Hutchings <ben.hutchings@codethink.co.uk>
Date: Thu, 26 Feb 2015 20:31:05 +0000

> Some minor new features and fixes.
> 
> These depend in part on the series I sent earlier for net, specifically
> "sh_eth: WARN on access to a register not implemented in a particular
> chip" depends on "sh_eth: Fix RX recovery on R-Car in case of RX ring
> underrun".

Series applied, thanks Ben.

Re: [net-next, 2/5] sh_eth: WARN on access to a register not implemented in a particular chip

[Geert Uytterhoeven]

Replying to a patchwork mbox, as I noticed this is in net-next.

On Thu, 26 Feb 2015, Ben Hutchings wrote:
> Currently we may silently read/write a register at offset 0.  Change
> this to WARN and then ignore the write or read-back all-ones.
> 
> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

While this may be a good idea for debugging...

> --- a/drivers/net/ethernet/renesas/sh_eth.h
> +++ b/drivers/net/ethernet/renesas/sh_eth.h
> @@ -543,19 +543,29 @@ static inline void sh_eth_soft_swap(char *src, int len)
>  #endif
>  }
>  
> +#define SH_ETH_OFFSET_INVALID	((u16) ~0)
> +
>  static inline void sh_eth_write(struct net_device *ndev, u32 data,
>  				int enum_index)
>  {
>  	struct sh_eth_private *mdp = netdev_priv(ndev);
> +	u16 offset = mdp->reg_offset[enum_index];
> +
> +	if (WARN_ON(offset == SH_ETH_OFFSET_INVALID))
> +		return;

... adding WARN_ON() to static inline functions increases code size a lot:

$ size drivers/net/ethernet/renesas/sh_eth.o{.orig,}
   text	   data	    bss	    dec	    hex	filename
  23352	   1136	      0	  24488	   5fa8	drivers/net/ethernet/renesas/sh_eth.o.orig
  27225	   1136	      0	  28361	   6ec9	drivers/net/ethernet/renesas/sh_eth.o
$

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [net-next, 2/5] sh_eth: WARN on access to a register not implemented in a particular chip

[Ben Hutchings]

On Thu, 2015-03-05 at 10:02 +0100, Geert Uytterhoeven wrote:
> Replying to a patchwork mbox, as I noticed this is in net-next.
> 
> On Thu, 26 Feb 2015, Ben Hutchings wrote:
> > Currently we may silently read/write a register at offset 0.  Change
> > this to WARN and then ignore the write or read-back all-ones.
> > 
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> While this may be a good idea for debugging...
> 
> > --- a/drivers/net/ethernet/renesas/sh_eth.h
> > +++ b/drivers/net/ethernet/renesas/sh_eth.h
> > @@ -543,19 +543,29 @@ static inline void sh_eth_soft_swap(char *src, int len)
> >  #endif
> >  }
> >  
> > +#define SH_ETH_OFFSET_INVALID	((u16) ~0)
> > +
> >  static inline void sh_eth_write(struct net_device *ndev, u32 data,
> >  				int enum_index)
> >  {
> >  	struct sh_eth_private *mdp = netdev_priv(ndev);
> > +	u16 offset = mdp->reg_offset[enum_index];
> > +
> > +	if (WARN_ON(offset == SH_ETH_OFFSET_INVALID))
> > +		return;
> 
> ... adding WARN_ON() to static inline functions increases code size a lot:
> 
> $ size drivers/net/ethernet/renesas/sh_eth.o{.orig,}
>    text	   data	    bss	    dec	    hex	filename
>   23352	   1136	      0	  24488	   5fa8	drivers/net/ethernet/renesas/sh_eth.o.orig
>   27225	   1136	      0	  28361	   6ec9	drivers/net/ethernet/renesas/sh_eth.o
> $

Time to un-inline it, maybe?

Ben.

Re: [Linux-kernel] [net-next, 2/5] sh_eth: WARN on access to a register not implemented in a particular chip

[Ben Dooks]

On 05/03/15 13:18, Ben Hutchings wrote:
> On Thu, 2015-03-05 at 10:02 +0100, Geert Uytterhoeven wrote:
>> Replying to a patchwork mbox, as I noticed this is in net-next.
>>
>> On Thu, 26 Feb 2015, Ben Hutchings wrote:
>>> Currently we may silently read/write a register at offset 0.  Change
>>> this to WARN and then ignore the write or read-back all-ones.
>>>
>>> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
>>> Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> While this may be a good idea for debugging...
>>
>>> --- a/drivers/net/ethernet/renesas/sh_eth.h
>>> +++ b/drivers/net/ethernet/renesas/sh_eth.h
>>> @@ -543,19 +543,29 @@ static inline void sh_eth_soft_swap(char *src, int len)
>>>  #endif
>>>  }
>>>  
>>> +#define SH_ETH_OFFSET_INVALID	((u16) ~0)
>>> +
>>>  static inline void sh_eth_write(struct net_device *ndev, u32 data,
>>>  				int enum_index)
>>>  {
>>>  	struct sh_eth_private *mdp = netdev_priv(ndev);

does de-referencing this each time make a difference? Looks like
it would have been easier to pass an "struct sh_eth_private" instead
of the "struct net_device *ndev"

>>> +	u16 offset = mdp->reg_offset[enum_index];
>>> +
>>> +	if (WARN_ON(offset == SH_ETH_OFFSET_INVALID))
>>> +		return;

You could cange the mdp->reg_offset to an mdp->reg_pointer and make
any invalid registers a NULL. This would at-least make it fail on an
invalid access.

>> ... adding WARN_ON() to static inline functions increases code size a lot:
>>
>> $ size drivers/net/ethernet/renesas/sh_eth.o{.orig,}
>>    text	   data	    bss	    dec	    hex	filename
>>   23352	   1136	      0	  24488	   5fa8	drivers/net/ethernet/renesas/sh_eth.o.orig
>>   27225	   1136	      0	  28361	   6ec9	drivers/net/ethernet/renesas/sh_eth.o
>> $
> 
> Time to un-inline it, maybe?
> 
> Ben.
> 
> 
> _______________________________________________
> linux-kernel mailing list
> linux-kernel@lists.codethink.co.uk
> https://ducie-dc1.codethink.co.uk/cgi-bin/mailman/listinfo/linux-kernel
> 


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [Linux-kernel] [net-next, 2/5] sh_eth: WARN on access to a register not implemented in a particular chip

[Sergei Shtylyov]

Hello.

On 03/09/2015 11:50 AM, Ben Dooks wrote:

>>>> Currently we may silently read/write a register at offset 0.  Change
>>>> this to WARN and then ignore the write or read-back all-ones.

>>>> Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
>>>> Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>>> While this may be a good idea for debugging...

>>>> --- a/drivers/net/ethernet/renesas/sh_eth.h
>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.h
>>>> @@ -543,19 +543,29 @@ static inline void sh_eth_soft_swap(char *src, int len)
>>>>   #endif
>>>>   }
>>>>
>>>> +#define SH_ETH_OFFSET_INVALID	((u16) ~0)
>>>> +
>>>>   static inline void sh_eth_write(struct net_device *ndev, u32 data,
>>>>   				int enum_index)
>>>>   {
>>>>   	struct sh_eth_private *mdp = netdev_priv(ndev);

> does de-referencing this each time make a difference? Looks like

   It's not a dereference, it's just a pointer addition.

> it would have been easier to pass an "struct sh_eth_private" instead
> of the "struct net_device *ndev"

    Hm, maybe...

>>>> +	u16 offset = mdp->reg_offset[enum_index];
>>>> +
>>>> +	if (WARN_ON(offset == SH_ETH_OFFSET_INVALID))
>>>> +		return;

> You could cange the mdp->reg_offset to an mdp->reg_pointer and make
> any invalid registers a NULL. This would at-least make it fail on an
> invalid access.

    Interesting idea...

WBR, Sergei