[PATCH 0/6] stmmac: Driver Updates

[PATCH 0/6] stmmac: Driver Updates

[Deepak Sikri]

This patch set modifies the stmmac driver.
Following changes have been done in the patch set.
1. Explicitly have the option to pass checksum offload engine type
through the platform code. This is helpful for stmmac core prior to
revision 3.5
2. Pass the default value of clk_csr scaling through the platform code.
Define the scaling macros to be consistently used across driver and plat
code.
3. Update the stmmac driver to incorporate the CPU freq framework.
4. Update the DMA descriptor status checks for cores prior to rev-3.5.
5. Configure burst related DMA parameter for cores revision beyond
rev-3.6
6. Replace infinite loop in mdio read and write by timeouts

Deepak Sikri (6):
  stmmac: Define CSUM offload engine Types
  stmmac: Define MDC clock selection macros.
  stmmac: Add support for CPU freq notifiers.
  stmmac: Update stmmac descriptor checks for stmmac core prior to
    Rev-3.5.
  stmmac: configure burst related GMAC DMA parameters
  stmmac: Replace infinite loops by timeouts in mdio r/w

 drivers/net/ethernet/stmicro/stmmac/common.h       |    2 +-
 .../net/ethernet/stmicro/stmmac/dwmac1000_dma.c    |   14 ++-
 drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h    |    1 +
 drivers/net/ethernet/stmicro/stmmac/enh_desc.c     |   17 ++-
 drivers/net/ethernet/stmicro/stmmac/norm_desc.c    |    2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |   10 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  138 +++++++++++++++++++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  |   30 ++++-
 include/linux/stmmac.h                             |   36 +++++
 9 files changed, 229 insertions(+), 21 deletions(-)

[PATCH 1/6] stmmac: Define CSUM offload engine Types

[Deepak Sikri]

This patch explicitly defines the CSUM offload engine type which need
(not mandatory) to be passed from the platform code.
STMMAC core supports two check sum offload engine types- Type-1 & Type-2.
Also, there are STMMAC cores that do not have the check sum offload
capabilities.
The behaviour of Type-1 & Type-2 cores related to provision of checksum
increases the packet length for Type-1 cores by 2, as the checksum is appended
at the end of data packet and the same is made accountable in the DMA status.
The STMMAC cores beyond Version-3.5 provide HW interface registers which allows
the user to read the HW capabilities, while to support the previous cores the
information related to HW capabilities has to be provided from the platform
code.
The Type-1 cores which do not have the HW register interface need this
information.

Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
---
 include/linux/stmmac.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 0dddc9e..aa0d99e 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -28,6 +28,18 @@
 
 #include <linux/platform_device.h>
 
+/* Checksum offload engine Types */
+/* STMMAC core supports two check sum offloading engine types
+ * Type-1 & Type-2
+ * These are configurable portion of the MAC core and hence could be
+ * also made off. 
+ * The Type-0 Macro defined below covers the core which do not support
+ * the checksum offloading.
+ */
+#define STMMAC_CSUM_T0	0
+#define STMMAC_CSUM_T1	1
+#define STMMAC_CSUM_T2	2
+
 /* Platfrom data for platform device structure's platform_data field */
 
 struct stmmac_mdio_bus_data {
@@ -57,5 +69,6 @@ struct plat_stmmacenet_data {
 	void (*exit)(struct platform_device *pdev);
 	void *custom_cfg;
 	void *bsp_priv;
+	int csum_off_engine_type;
 };
 #endif
-- 
1.6.0.2

[PATCH 2/6] stmmac: Define MDC clock selection macros.

[Deepak Sikri]

The patch adds the macros to be used for MDC clock selection. The MDC clock
frequency is based on scaled system clock, and has to be confined to a range
of 1-2.5 MHz. Based on the input CSR clock, the scaling factor has to be
selected.
The platform specific code will provide the default value of this scaling
factor, based on the input CSR clock.

Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
---
 include/linux/stmmac.h |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index aa0d99e..7332ed8 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -40,6 +40,29 @@
 #define STMMAC_CSUM_T1	1
 #define STMMAC_CSUM_T2	2
 
+/*
+ * Define the macros for CSR clock range parameters to be passed by
+ * platform code.
+ * This could also be configured at run time using CPU freq framework.
+ */
+
+/* CSR Frequency Access Defines*/
+#define CSR_F_20M	20000000
+#define CSR_F_35M	35000000
+#define CSR_F_60M	60000000
+#define CSR_F_100M	100000000
+#define CSR_F_150M	150000000
+#define CSR_F_250M	50000000
+#define CSR_F_300M	300000000
+
+/* MDC Clock Selection define*/
+#define	STMMAC_CLK_RANGE_60_100M	0	/* MDC = Clk/42 */
+#define	STMMAC_CLK_RANGE_100_150M	1	/* MDC = Clk/62 */
+#define	STMMAC_CLK_RANGE_20_35M		2	/* MDC = Clk/16 */
+#define	STMMAC_CLK_RANGE_35_60M		3	/* MDC = Clk/26 */
+#define	STMMAC_CLK_RANGE_150_250M	4	/* MDC = Clk/102 */
+#define	STMMAC_CLK_RANGE_250_300M	5	/* MDC = Clk/122 */
+
 /* Platfrom data for platform device structure's platform_data field */
 
 struct stmmac_mdio_bus_data {
-- 
1.6.0.2

[PATCH 3/6] stmmac: Add support for CPU freq notifiers.

[Deepak Sikri]

This patch adds in the support for CPU freq notifiers. In case the system freq
is changed using the CPU freq governors, the CSR input clock is also affected.

To maintain the CSR mdio clock within the limits of 1-2.5 Mhz the input clock
has to be properly scaled using the appropriate scaling factors.
This patch looks into the input clock frequency changes and selects the apt
scaling factor once a notifier alert is received.
Additionally the stmmac clock has been defined to support cpu freq notifiers.

Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   10 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  124 ++++++++++++++++++++-
 2 files changed, 130 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 1207400..67b9757 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -22,6 +22,10 @@
 
 #define STMMAC_RESOURCE_NAME   "stmmaceth"
 #define DRV_MODULE_VERSION	"Dec_2011"
+
+#ifdef CONFIG_HAVE_CLK
+#include <linux/clk.h>
+#endif
 #include <linux/stmmac.h>
 #include <linux/phy.h>
 #include "common.h"
@@ -81,6 +85,12 @@ struct stmmac_priv {
 	struct stmmac_counters mmc;
 	struct dma_features dma_cap;
 	int hw_cap_support;
+#ifdef CONFIG_CPU_FREQ
+	struct notifier_block freq_transition;
+#endif
+#ifdef CONFIG_HAVE_CLK
+	struct clk *stmmac_clk;
+#endif
 };
 
 extern int phyaddr;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 96fa2da..001b8f3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -35,6 +35,7 @@
 #include <linux/skbuff.h>
 #include <linux/ethtool.h>
 #include <linux/if_ether.h>
+#include <linux/cpufreq.h>
 #include <linux/crc32.h>
 #include <linux/mii.h>
 #include <linux/if.h>
@@ -180,6 +181,92 @@ static void print_pkt(unsigned char *buf, int len)
 /* minimum number of free TX descriptors required to wake up TX process */
 #define STMMAC_TX_THRESH(x)	(x->dma_tx_size/4)
 
+#ifdef CONFIG_CPU_FREQ
+static inline void stmmac_clk_csr_set(struct stmmac_priv *priv, int clk_rate)
+{
+
+	if ((clk_rate >= CSR_F_20M) && (clk_rate < CSR_F_35M))
+		priv->plat->clk_csr = STMMAC_CLK_RANGE_20_35M;
+	else if ((clk_rate >= CSR_F_35M) && (clk_rate < CSR_F_60M))
+		priv->plat->clk_csr = STMMAC_CLK_RANGE_35_60M;
+	else if ((clk_rate >= CSR_F_60M) && (clk_rate < CSR_F_100M))
+		priv->plat->clk_csr = STMMAC_CLK_RANGE_60_100M;
+	else if ((clk_rate >= CSR_F_100M) && (clk_rate < CSR_F_150M))
+		priv->plat->clk_csr = STMMAC_CLK_RANGE_100_150M;
+	else if ((clk_rate >= CSR_F_150M) && (clk_rate < CSR_F_250M))
+		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;
+	else if ((clk_rate >= CSR_F_250M) && (clk_rate < CSR_F_300M))
+		priv->plat->clk_csr = STMMAC_CLK_RANGE_250_300M;
+	else
+		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;
+
+}
+
+static int stmmac_cpufreq_transition(struct notifier_block *nb,
+		unsigned long val, void *data)
+{
+	struct stmmac_priv *priv;
+	u32 clk_rate;
+
+	priv = container_of(nb, struct stmmac_priv, freq_transition);
+
+	if (val == CPUFREQ_PRECHANGE) {
+		/* Stop TX/RX DMA */
+		priv->hw->dma->stop_tx(priv->ioaddr);
+		priv->hw->dma->stop_rx(priv->ioaddr);
+
+	} else if (val == CPUFREQ_POSTCHANGE) {
+		/* Start DMA Tx/Rx */
+		priv->hw->dma->start_tx(priv->ioaddr);
+		priv->hw->dma->start_rx(priv->ioaddr);
+		priv->hw->dma->enable_dma_transmission(priv->ioaddr);
+
+		if (priv->stmmac_clk) {
+			/*
+			 * Decide on the MDC clock dynamically based on the
+			 * csr clock input.
+			 * This is helpfull in case the cpu frequency is changed
+			 * on the run using the cpu freq framework, and based
+			 * on that the bus frequency is also changed.
+			 * In case the clock framework is not established for an
+			 * architecture, use the default value that has to be
+			 * set through the platform.
+			 */
+			clk_rate = clk_get_rate(priv->stmmac_clk);
+			stmmac_clk_csr_set(priv, clk_rate);
+		}
+	}
+
+	return 0;
+}
+
+static inline void stmmac_cpufreq_register(struct stmmac_priv *priv)
+{
+	priv->freq_transition.notifier_call = stmmac_cpufreq_transition;
+	cpufreq_register_notifier(&priv->freq_transition,
+			CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+static inline void stmmac_cpufreq_deregister(struct stmmac_priv *priv)
+{
+
+	cpufreq_unregister_notifier(&priv->freq_transition,
+			CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+#else
+
+static inline void stmmac_cpufreq_register(struct stmmac_priv *priv)
+{
+	return 0;
+}
+
+static inline void stmmac_cpufreq_deregister(struct stmmac_priv *priv)
+{
+}
+
+#endif
+
 static inline u32 stmmac_tx_avail(struct stmmac_priv *priv)
 {
 	return priv->dirty_tx + priv->dma_tx_size - priv->cur_tx - 1;
@@ -930,10 +1017,14 @@ static int stmmac_open(struct net_device *dev)
 	struct stmmac_priv *priv = netdev_priv(dev);
 	int ret;
 
+#ifdef CONFIG_HAVE_CLK
+	if (priv->stmmac_clk)
+		clk_enable(priv->stmmac_clk);
+#endif
 	/* MAC HW device setup */
 	ret = stmmac_mac_device_setup(dev);
 	if (ret < 0)
-		return ret;
+		goto open_clk_dis;
 
 	stmmac_check_ether_addr(priv);
 
@@ -949,14 +1040,15 @@ static int stmmac_open(struct net_device *dev)
 	if (ret < 0) {
 		pr_debug("%s: MDIO bus (id: %d) registration failed",
 			 __func__, priv->plat->bus_id);
-		return ret;
+		goto open_clk_dis;
 	}
 
 #ifdef CONFIG_STMMAC_TIMER
 	priv->tm = kzalloc(sizeof(struct stmmac_timer *), GFP_KERNEL);
 	if (unlikely(priv->tm == NULL)) {
 		pr_err("%s: ERROR: timer memory alloc failed\n", __func__);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto open_clk_dis;
 	}
 	priv->tm->freq = tmrate;
 
@@ -1093,7 +1185,11 @@ open_error:
 #endif
 	if (priv->phydev)
 		phy_disconnect(priv->phydev);
-
+open_clk_dis:
+#ifdef CONFIG_HAVE_CLK
+	if (priv->stmmac_clk)
+		clk_disable(priv->stmmac_clk);
+#endif
 	return ret;
 }
 
@@ -1145,6 +1241,11 @@ static int stmmac_release(struct net_device *dev)
 #endif
 	stmmac_mdio_unregister(dev);
 
+#ifdef CONFIG_HAVE_CLK
+	if (priv->stmmac_clk)
+		clk_disable(priv->stmmac_clk);
+#endif
+
 	return 0;
 }
 
@@ -1844,6 +1945,16 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
 		goto error;
 	}
 
+#ifdef CONFIG_HAVE_CLK
+	priv->stmmac_clk = clk_get(device, NULL);
+	if (IS_ERR(priv->stmmac_clk)) {
+		ret = PTR_ERR(priv->stmmac_clk);
+		pr_err("%s: ERROR %i clock get \n", __func__, ret);
+		goto error;
+	}
+#endif
+	stmmac_cpufreq_register(priv);
+
 	DBG(probe, DEBUG, "%s: Scatter/Gather: %s - HW checksums: %s\n",
 	    ndev->name, (ndev->features & NETIF_F_SG) ? "on" : "off",
 	    (ndev->features & NETIF_F_IP_CSUM) ? "on" : "off");
@@ -1875,6 +1986,11 @@ int stmmac_dvr_remove(struct net_device *ndev)
 	priv->hw->dma->stop_tx(priv->ioaddr);
 
 	stmmac_set_mac(priv->ioaddr, false);
+	stmmac_cpufreq_deregister(priv);
+#ifdef CONFIG_HAVE_CLK
+	if (priv->stmmac_clk)
+		clk_put(priv->stmmac_clk);
+#endif
 	netif_carrier_off(ndev);
 	unregister_netdev(ndev);
 	free_netdev(ndev);
-- 
1.6.0.2

[PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[Deepak Sikri]

The patch adds in the following support.
1. The stmmac core supporting Type-1 checksum offload engine & prior to
Revision 3.5, append the HW cecksum at the end of payload in case the Rx
checksum engine was used to offload the HW checksum.
There cores did not provide the HW register interface to read the device
capabilities, and hence the plat code provides the core checksum capabilties
that help to identify type-1 cores, and adjust the frame length.

2. Also, the following Frame status checks with the Full checksum offload
Type-2 engine enabled are not backward compatible and are reserved for
STMMAC core revisions prior to 3.5.
Bit18(Eth Frame)	Bit27(Header Csum Error)	Bit28 (Payload Csum Err)
	0			1				1
	0			1				0

These conditions have been treated as no HW checksum support for stmmac core
prior to rev-3.5.

Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h      |    2 +-
 drivers/net/ethernet/stmicro/stmmac/enh_desc.c    |   17 +++++++++++------
 drivers/net/ethernet/stmicro/stmmac/norm_desc.c   |    2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   14 +++++++++++++-
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index d0b814e..12d1565 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -230,7 +230,7 @@ struct stmmac_desc_ops {
 	int (*get_rx_frame_len) (struct dma_desc *p);
 	/* Return the reception status looking at the RDES1 */
 	int (*rx_status) (void *data, struct stmmac_extra_stats *x,
-			  struct dma_desc *p);
+			  struct dma_desc *p, u32 mac_id);
 };
 
 struct stmmac_dma_ops {
diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
index d879763..42026f6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -22,6 +22,7 @@
   Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
 *******************************************************************************/
 
+#include <linux/stmmac.h>
 #include "common.h"
 #include "descs_com.h"
 
@@ -106,7 +107,9 @@ static int enh_desc_get_tx_len(struct dma_desc *p)
 	return p->des01.etx.buffer1_size;
 }
 
-static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err)
+#define GMAC_VERSION_35 0x35
+static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err,
+		u32 mac_id)
 {
 	int ret = good_frame;
 	u32 status = (type << 2 | ipc_err << 1 | payload_err) & 0x7;
@@ -141,16 +144,16 @@ static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err)
 	} else if (status == 0x1) {
 		CHIP_DBG(KERN_ERR
 		    "RX Des0 status: IPv4/6 unsupported IP PAYLOAD.\n");
-		ret = discard_frame;
+		ret = (mac_id >= GMAC_VERSION_35) ? discard_frame : csum_none;
 	} else if (status == 0x3) {
 		CHIP_DBG(KERN_ERR "RX Des0 status: No IPv4, IPv6 frame.\n");
-		ret = discard_frame;
+		ret = (mac_id >= GMAC_VERSION_35) ? discard_frame : csum_none;
 	}
 	return ret;
 }
 
 static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
-				  struct dma_desc *p)
+				  struct dma_desc *p, u32 mac_id)
 {
 	int ret = good_frame;
 	struct net_device_stats *stats = (struct net_device_stats *)data;
@@ -195,9 +198,11 @@ static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
 	/* After a payload csum error, the ES bit is set.
 	 * It doesn't match with the information reported into the databook.
 	 * At any rate, we need to understand if the CSUM hw computation is ok
-	 * and report this info to the upper layers. */
+	 * and report this info to the upper layers.
+	 */
 	ret = enh_desc_coe_rdes0(p->des01.erx.ipc_csum_error,
-		p->des01.erx.frame_type, p->des01.erx.payload_csum_error);
+			p->des01.erx.frame_type,
+			p->des01.erx.payload_csum_error, mac_id);
 
 	if (unlikely(p->des01.erx.dribbling)) {
 		CHIP_DBG(KERN_ERR "GMAC RX: dribbling error\n");
diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
index fda5d2b..057a728 100644
--- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
@@ -72,7 +72,7 @@ static int ndesc_get_tx_len(struct dma_desc *p)
  * In case of success, it returns good_frame because the GMAC device
  * is supposed to be able to compute the csum in HW. */
 static int ndesc_get_rx_status(void *data, struct stmmac_extra_stats *x,
-			       struct dma_desc *p)
+			       struct dma_desc *p, u32 mac_id)
 {
 	int ret = good_frame;
 	struct net_device_stats *stats = (struct net_device_stats *)data;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 001b8f3..3bcdc97 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1103,6 +1103,7 @@ static int stmmac_open(struct net_device *dev)
 	priv->rx_coe = priv->hw->mac->rx_coe(priv->ioaddr);
 	if (priv->rx_coe)
 		pr_info(" Checksum Offload Engine supported\n");
+
 	if (priv->plat->tx_coe)
 		pr_info(" Checksum insertion supported\n");
 
@@ -1436,7 +1437,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
 
 		/* read the status of the incoming frame */
 		status = (priv->hw->desc->rx_status(&priv->dev->stats,
-						    &priv->xstats, p));
+					&priv->xstats, p,
+					priv->hw->synopsys_uid & 0xff));
 		if (unlikely(status == discard_frame))
 			priv->dev->stats.rx_errors++;
 		else {
@@ -1444,6 +1446,16 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
 			int frame_len;
 
 			frame_len = priv->hw->desc->get_rx_frame_len(p);
+			/*
+			 * The type-1 checksume offload engines append
+			 * the checksum at the end of frame, and the
+			 * the two bytes of checksum are added in
+			 * length.
+			 * Adjust for that in the framelen for type-1
+			 * checksum offload engines.
+			 */
+			if (priv->plat->csum_off_engine_type == STMMAC_CSUM_T1)
+				frame_len -= 2;
 			/* ACS is set; GMAC core strips PAD/FCS for IEEE 802.3
 			 * Type frames (LLC/LLC-SNAP) */
 			if (unlikely(status != llc_snap))
-- 
1.6.0.2

[PATCH 5/6] stmmac: configure burst related GMAC DMA parameters

[Deepak Sikri]

SPEAr1340 GMAC is a different version (3.61a) of Synopsys IP where
instead of 4xPBL we have 8xPBL, hence pbl value supplied by platform
data in
	- SPEAr1340 results in 8 * pbl
	- rest devices result in 4 * pbl

Further it is observed that rest of the devices (older version) which
have an AXI wrapper over AHB are limited  to incr 32 burst where as this
can go up to incr 128 in case of SPEAr1340.

Also, with fixed burst configuration we need to program permissible
burst values in newer versions (AXI supported) of gmac. This
AXI_BUS_MODE) register is reserved for earlier versions of gmac and
writing to them has no impact.

Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac1000_dma.c    |   14 +++++++++++++-
 drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h    |    1 +
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
index 4d5402a..ed6ffa3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
@@ -48,7 +48,7 @@ static int dwmac1000_dma_init(void __iomem *ioaddr, int pbl, u32 dma_tx,
 	if (limit < 0)
 		return -EBUSY;
 
-	value = /* DMA_BUS_MODE_FB | */ DMA_BUS_MODE_4PBL |
+	value = DMA_BUS_MODE_FB | DMA_BUS_MODE_4PBL |
 	    ((pbl << DMA_BUS_MODE_PBL_SHIFT) |
 	     (pbl << DMA_BUS_MODE_RPBL_SHIFT));
 
@@ -56,6 +56,18 @@ static int dwmac1000_dma_init(void __iomem *ioaddr, int pbl, u32 dma_tx,
 	value |= DMA_BUS_MODE_DA;	/* Rx has priority over tx */
 #endif
 	writel(value, ioaddr + DMA_BUS_MODE);
+	/*
+	 * We need to program DMA_AXI_BUS_MODE for supported bursts in
+	 * case DMA_BUS_MODE_FB mode is selected
+	 * Note: This is applicable only for revision GMACv3.61a. For
+	 * older version this register is reserved and shall have no
+	 * effect.
+	 * Further we directly write 0xFF to this register. This would
+	 * ensure that all bursts supported by platform is set and those
+	 * which are not supported would remain ineffective.
+	 */
+	if (value & DMA_BUS_MODE_FB)
+		writel(0xFF, ioaddr + DMA_AXI_BUS_MODE);
 
 	/* Mask interrupts by writing to CSR7 */
 	writel(DMA_INTR_DEFAULT_MASK, ioaddr + DMA_INTR_ENA);
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
index 437edac..6e0360f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
@@ -32,6 +32,7 @@
 #define DMA_CONTROL		0x00001018	/* Ctrl (Operational Mode) */
 #define DMA_INTR_ENA		0x0000101c	/* Interrupt Enable */
 #define DMA_MISSED_FRAME_CTR	0x00001020	/* Missed Frame Counter */
+#define DMA_AXI_BUS_MODE       0x00001028      /* AXI Bus Mode */
 #define DMA_CUR_TX_BUF_ADDR	0x00001050	/* Current Host Tx Buffer */
 #define DMA_CUR_RX_BUF_ADDR	0x00001054	/* Current Host Rx Buffer */
 #define DMA_HW_FEATURE		0x00001058	/* HW Feature Register */
-- 
1.6.0.2

[PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w

[Deepak Sikri]

This patch removes the infinite waits from the mdio read and
write interfaces. These infinite waits have been replaced by
the timeout handling. In case if a time out occurs, an error is
returned.

Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c |   30 ++++++++++++++++-----
 1 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 7319532..b6a6fb2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -34,6 +34,20 @@
 #define MII_BUSY 0x00000001
 #define MII_WRITE 0x00000002
 
+static int stmmac_mdio_busy_wait(unsigned long ioaddr, unsigned int mii_addr)
+{
+	unsigned long finish = jiffies + 3 * HZ;
+
+	do {
+		if (readl(ioaddr + mii_addr) & MII_BUSY)
+			cpu_relax();
+		else
+			return 0;
+	} while (!time_after_eq(jiffies, finish));
+
+	return -EBUSY;
+}
+
 /**
  * stmmac_mdio_read
  * @bus: points to the mii_bus structure
@@ -56,9 +70,13 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
 			((phyreg << 6) & (0x000007C0)));
 	regValue |= MII_BUSY | ((priv->plat->clk_csr & 7) << 2);
 
-	do {} while (((readl(priv->ioaddr + mii_address)) & MII_BUSY) == 1);
+	if (stmmac_mdio_busy_wait(priv->ioaddr, mii_address))
+		return -EBUSY;
+
 	writel(regValue, priv->ioaddr + mii_address);
-	do {} while (((readl(priv->ioaddr + mii_address)) & MII_BUSY) == 1);
+
+	if (stmmac_mdio_busy_wait(priv->ioaddr, mii_address))
+		return -EBUSY;
 
 	/* Read the data from the MII data register */
 	data = (int)readl(priv->ioaddr + mii_data);
@@ -88,18 +106,16 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg,
 
 	value |= MII_BUSY | ((priv->plat->clk_csr & 7) << 2);
 
-
 	/* Wait until any existing MII operation is complete */
-	do {} while (((readl(priv->ioaddr + mii_address)) & MII_BUSY) == 1);
+	if (stmmac_mdio_busy_wait(priv->ioaddr, mii_address))
+		return -EBUSY;
 
 	/* Set the MII address register to write */
 	writel(phydata, priv->ioaddr + mii_data);
 	writel(value, priv->ioaddr + mii_address);
 
 	/* Wait until any existing MII operation is complete */
-	do {} while (((readl(priv->ioaddr + mii_address)) & MII_BUSY) == 1);
-
-	return 0;
+	return stmmac_mdio_busy_wait(priv->ioaddr, mii_address);
 }
 
 /**
-- 
1.6.0.2

Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers.

[David Miller]

From: Deepak Sikri <deepak.sikri@st.com>
Date: Fri, 2 Mar 2012 18:25:25 +0530

> +{
> +
> +	if ((clk_rate >= CSR_F_20M) && (clk_rate < CSR_F_35M))
 ...
> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;
> +
> +}

Unnecessary empty lines, remove them.

> +			/*
> +			 * Decide on the MDC clock dynamically based on the
> +			 * csr clock input.

Format comments:

	/* Like
	 * this.
	 */

Not:

	/*
	 * Like
	 * this.
	 */

> +static inline void stmmac_cpufreq_deregister(struct stmmac_priv *priv)
> +{
> +
> +	cpufreq_unregister_notifier(&priv->freq_transition,

Unnecessary empty line.

Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[David Miller]

From: Deepak Sikri <deepak.sikri@st.com>
Date: Fri, 2 Mar 2012 18:25:26 +0530

> +static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err,
> +		u32 mac_id)

Poorly formatted, this should be:

static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err,
			      u32 mac_id)


> +			/*
> +			 * The type-1 checksume offload engines append
> +			 * the checksum at the end of frame, and the

	/* Format comments
	 * like this.
	 */

	/*
	 * Not
	 * like this.
	 */

Re: [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters

[David Miller]

From: Deepak Sikri <deepak.sikri@st.com>
Date: Fri, 2 Mar 2012 18:25:27 +0530

>  	writel(value, ioaddr + DMA_BUS_MODE);
> +	/*
> +	 * We need to program DMA_AXI_BUS_MODE for supported bursts in
> +	 * case DMA_BUS_MODE_FB mode is selected

Please fix comment formatting.

Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[Shiraz Hashim]

Hello David,

On Mon, Mar 05, 2012 at 09:51:55AM +0800, David Miller wrote:
> From: Deepak Sikri <deepak.sikri@st.com>
> > +			/*
> > +			 * The type-1 checksume offload engines append
> > +			 * the checksum at the end of frame, and the
> 
> 	/* Format comments
> 	 * like this.
> 	 */
> 
> 	/*
> 	 * Not
> 	 * like this.
> 	 */

Linux Documentation/CodingStyle mentions following about comments in
chapter 8.

The preferred style for long (multi-line) comments is: 

        /*  
         * This is the preferred style for multi-line
         * comments in the Linux kernel source code.
         * Please use it consistently.
         *   
         * Description:  A column of asterisks on the left side,
         * with beginning and ending almost-blank lines.
         */  

Are we missing something ?

--
regards
Shiraz

Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[David Miller]

From: Shiraz Hashim <shiraz.hashim@st.com>
Date: Mon, 5 Mar 2012 09:31:25 +0530

> Are we missing something ?

It wastes precious screen real-estate with that first line
with zero text in it, therefore I don't want it used in any
code I accept.

Re: [PATCH 1/6] stmmac: Define CSUM offload engine Types

[Giuseppe CAVALLARO]

Hello Deepak

On 3/2/2012 1:55 PM, Deepak SIKRI wrote:
> This patch explicitly defines the CSUM offload engine type which need
> (not mandatory) to be passed from the platform code.
> STMMAC core supports two check sum offload engine types- Type-1 & Type-2.
> Also, there are STMMAC cores that do not have the check sum offload
> capabilities.
> The behaviour of Type-1 & Type-2 cores related to provision of checksum
> increases the packet length for Type-1 cores by 2, as the checksum is appended
> at the end of data packet and the same is made accountable in the DMA status.
> The STMMAC cores beyond Version-3.5 provide HW interface registers which allows
> the user to read the HW capabilities, while to support the previous cores the
> information related to HW capabilities has to be provided from the platform
> code.
> The Type-1 cores which do not have the HW register interface need this
> information.

this patch is useful but I've some notes.

Pay attention that, new GMAC devices have the HW capability register to
understand if the rx_coe_type is TYPE1 or TYPE2.
These values should always override the ones come from the platform.
This check is missing in your code.
Can I ask you to rename csum_off_engine_type as rx_coe_type to be
aligned to the name convention used inside the driver.

Regards
Peppe

> Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
> ---
>  include/linux/stmmac.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index 0dddc9e..aa0d99e 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -28,6 +28,18 @@
>  
>  #include <linux/platform_device.h>
>  
> +/* Checksum offload engine Types */
> +/* STMMAC core supports two check sum offloading engine types
> + * Type-1 & Type-2
> + * These are configurable portion of the MAC core and hence could be
> + * also made off. 
> + * The Type-0 Macro defined below covers the core which do not support
> + * the checksum offloading.
> + */
> +#define STMMAC_CSUM_T0	0
> +#define STMMAC_CSUM_T1	1
> +#define STMMAC_CSUM_T2	2
> +
>  /* Platfrom data for platform device structure's platform_data field */
>  
>  struct stmmac_mdio_bus_data {
> @@ -57,5 +69,6 @@ struct plat_stmmacenet_data {
>  	void (*exit)(struct platform_device *pdev);
>  	void *custom_cfg;
>  	void *bsp_priv;
> +	int csum_off_engine_type;
>  };
>  #endif

Re: [PATCH 2/6] stmmac: Define MDC clock selection macros.

[Giuseppe CAVALLARO]

Hello Deepak

On 3/2/2012 1:55 PM, Deepak SIKRI wrote:
> The patch adds the macros to be used for MDC clock selection. The MDC clock
> frequency is based on scaled system clock, and has to be confined to a range
> of 1-2.5 MHz. Based on the input CSR clock, the scaling factor has to be
> selected.
> The platform specific code will provide the default value of this scaling
> factor, based on the input CSR clock.
> 
> Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
> ---
>  include/linux/stmmac.h |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index aa0d99e..7332ed8 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -40,6 +40,29 @@
>  #define STMMAC_CSUM_T1	1
>  #define STMMAC_CSUM_T2	2
>  
> +/*
> + * Define the macros for CSR clock range parameters to be passed by
> + * platform code.
> + * This could also be configured at run time using CPU freq framework.
> + */
> +
> +/* CSR Frequency Access Defines*/
> +#define CSR_F_20M	20000000
> +#define CSR_F_35M	35000000
> +#define CSR_F_60M	60000000
> +#define CSR_F_100M	100000000
> +#define CSR_F_150M	150000000
> +#define CSR_F_250M	50000000
> +#define CSR_F_300M	300000000


I have some concerns about this patch.

We want to have some defines to help on setting the clk_csr (that is is
a clk divisor).

When you program the "CSR Clock Range" bits in the GMII Address Register
you can also set the bit 5 (not supported in older devices e.g. 3.41a).
In this case, the defines below do not cover all the cases, I mean:

1000 clk_csr_i/4
1001 clk_csr_i/6
1010 clk_csr_i/8
1011 clk_csr_i/10
1100 clk_csr_i/12
1101 clk_csr_i/14
1110 clk_csr_i/16
1111 clk_csr_i/18

> +/* MDC Clock Selection define*/
> +#define	STMMAC_CLK_RANGE_60_100M	0	/* MDC = Clk/42 */
> +#define	STMMAC_CLK_RANGE_100_150M	1	/* MDC = Clk/62 */
> +#define	STMMAC_CLK_RANGE_20_35M		2	/* MDC = Clk/16 */
> +#define	STMMAC_CLK_RANGE_35_60M		3	/* MDC = Clk/26 */
> +#define	STMMAC_CLK_RANGE_150_250M	4	/* MDC = Clk/102 */
> +#define	STMMAC_CLK_RANGE_250_300M	5	/* MDC = Clk/122 */

I suggest you to rename these macros as:

#define STMMAC_CSR_60_100M	0	/* MDC = Clk/42 */
...

Also, macros CSR_F_20M should be totally removed.

Peppe

> +
>  /* Platfrom data for platform device structure's platform_data field */
>  
>  struct stmmac_mdio_bus_data {

Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers.

[Giuseppe CAVALLARO]

On 3/2/2012 1:55 PM, Deepak Sikri wrote:
> This patch adds in the support for CPU freq notifiers. In case the system freq
> is changed using the CPU freq governors, the CSR input clock is also affected.
> 
> To maintain the CSR mdio clock within the limits of 1-2.5 Mhz the input clock
> has to be properly scaled using the appropriate scaling factors.
> This patch looks into the input clock frequency changes and selects the apt
> scaling factor once a notifier alert is received.
> Additionally the stmmac clock has been defined to support cpu freq notifiers.

Hello Deepak

this patch doesn't apply on net.git.

Concerning the stmmac_clk_csr_set it should treat all the other divisor
values as I've just told you in the email #2.

All the defines from the patch #2:

+/* CSR Frequency Access Defines*/
+#define CSR_F_20M	20000000
+#define CSR_F_35M	35000000
+#define CSR_F_60M	60000000
+#define CSR_F_100M	100000000
+#define CSR_F_150M	150000000
+#define CSR_F_250M	50000000
+#define CSR_F_300M	300000000


should be moved from linux/stmmac.h to stmicro/stmmac/common.h

In the end, I kindly ask you to try to remove some of not necessary
#ifdef CONFIG_HAVE_CLK  inside the C file.

Indeed stmmac_clk is be null in case of there is not this support.

Also I wonder if stmmac_clk could be renamed as: stmmac_csr_clk.

Pls, resend the patch against net-git so i'll be happy to continue the
review ;-)

Peppe

> 
> Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   10 ++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  124 ++++++++++++++++++++-
>  2 files changed, 130 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 1207400..67b9757 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -22,6 +22,10 @@
>  
>  #define STMMAC_RESOURCE_NAME   "stmmaceth"
>  #define DRV_MODULE_VERSION	"Dec_2011"
> +
> +#ifdef CONFIG_HAVE_CLK
> +#include <linux/clk.h>
> +#endif
>  #include <linux/stmmac.h>
>  #include <linux/phy.h>
>  #include "common.h"
> @@ -81,6 +85,12 @@ struct stmmac_priv {
>  	struct stmmac_counters mmc;
>  	struct dma_features dma_cap;
>  	int hw_cap_support;
> +#ifdef CONFIG_CPU_FREQ
> +	struct notifier_block freq_transition;
> +#endif
> +#ifdef CONFIG_HAVE_CLK
> +	struct clk *stmmac_clk;
> +#endif
>  };
>  
>  extern int phyaddr;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 96fa2da..001b8f3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -35,6 +35,7 @@
>  #include <linux/skbuff.h>
>  #include <linux/ethtool.h>
>  #include <linux/if_ether.h>
> +#include <linux/cpufreq.h>
>  #include <linux/crc32.h>
>  #include <linux/mii.h>
>  #include <linux/if.h>
> @@ -180,6 +181,92 @@ static void print_pkt(unsigned char *buf, int len)
>  /* minimum number of free TX descriptors required to wake up TX process */
>  #define STMMAC_TX_THRESH(x)	(x->dma_tx_size/4)
>  
> +#ifdef CONFIG_CPU_FREQ
> +static inline void stmmac_clk_csr_set(struct stmmac_priv *priv, int clk_rate)
> +{
> +
> +	if ((clk_rate >= CSR_F_20M) && (clk_rate < CSR_F_35M))
> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_20_35M;
> +	else if ((clk_rate >= CSR_F_35M) && (clk_rate < CSR_F_60M))
> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_35_60M;
> +	else if ((clk_rate >= CSR_F_60M) && (clk_rate < CSR_F_100M))
> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_60_100M;
> +	else if ((clk_rate >= CSR_F_100M) && (clk_rate < CSR_F_150M))
> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_100_150M;
> +	else if ((clk_rate >= CSR_F_150M) && (clk_rate < CSR_F_250M))
> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;
> +	else if ((clk_rate >= CSR_F_250M) && (clk_rate < CSR_F_300M))
> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_250_300M;
> +	else
> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;
> +
> +}
> +
> +static int stmmac_cpufreq_transition(struct notifier_block *nb,
> +		unsigned long val, void *data)
> +{
> +	struct stmmac_priv *priv;
> +	u32 clk_rate;
> +
> +	priv = container_of(nb, struct stmmac_priv, freq_transition);
> +
> +	if (val == CPUFREQ_PRECHANGE) {
> +		/* Stop TX/RX DMA */
> +		priv->hw->dma->stop_tx(priv->ioaddr);
> +		priv->hw->dma->stop_rx(priv->ioaddr);
> +
> +	} else if (val == CPUFREQ_POSTCHANGE) {
> +		/* Start DMA Tx/Rx */
> +		priv->hw->dma->start_tx(priv->ioaddr);
> +		priv->hw->dma->start_rx(priv->ioaddr);
> +		priv->hw->dma->enable_dma_transmission(priv->ioaddr);
> +
> +		if (priv->stmmac_clk) {
> +			/*
> +			 * Decide on the MDC clock dynamically based on the
> +			 * csr clock input.
> +			 * This is helpfull in case the cpu frequency is changed
> +			 * on the run using the cpu freq framework, and based
> +			 * on that the bus frequency is also changed.
> +			 * In case the clock framework is not established for an
> +			 * architecture, use the default value that has to be
> +			 * set through the platform.
> +			 */
> +			clk_rate = clk_get_rate(priv->stmmac_clk);
> +			stmmac_clk_csr_set(priv, clk_rate);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static inline void stmmac_cpufreq_register(struct stmmac_priv *priv)
> +{
> +	priv->freq_transition.notifier_call = stmmac_cpufreq_transition;
> +	cpufreq_register_notifier(&priv->freq_transition,
> +			CPUFREQ_TRANSITION_NOTIFIER);
> +}
> +
> +static inline void stmmac_cpufreq_deregister(struct stmmac_priv *priv)
> +{
> +
> +	cpufreq_unregister_notifier(&priv->freq_transition,
> +			CPUFREQ_TRANSITION_NOTIFIER);
> +}
> +
> +#else
> +
> +static inline void stmmac_cpufreq_register(struct stmmac_priv *priv)
> +{
> +	return 0;
> +}
> +
> +static inline void stmmac_cpufreq_deregister(struct stmmac_priv *priv)
> +{
> +}
> +
> +#endif
> +
>  static inline u32 stmmac_tx_avail(struct stmmac_priv *priv)
>  {
>  	return priv->dirty_tx + priv->dma_tx_size - priv->cur_tx - 1;
> @@ -930,10 +1017,14 @@ static int stmmac_open(struct net_device *dev)
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	int ret;
>  
> +#ifdef CONFIG_HAVE_CLK
> +	if (priv->stmmac_clk)
> +		clk_enable(priv->stmmac_clk);
> +#endif
>  	/* MAC HW device setup */
>  	ret = stmmac_mac_device_setup(dev);
>  	if (ret < 0)
> -		return ret;
> +		goto open_clk_dis;
>  
>  	stmmac_check_ether_addr(priv);
>  
> @@ -949,14 +1040,15 @@ static int stmmac_open(struct net_device *dev)
>  	if (ret < 0) {
>  		pr_debug("%s: MDIO bus (id: %d) registration failed",
>  			 __func__, priv->plat->bus_id);
> -		return ret;
> +		goto open_clk_dis;
>  	}
>  
>  #ifdef CONFIG_STMMAC_TIMER
>  	priv->tm = kzalloc(sizeof(struct stmmac_timer *), GFP_KERNEL);
>  	if (unlikely(priv->tm == NULL)) {
>  		pr_err("%s: ERROR: timer memory alloc failed\n", __func__);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto open_clk_dis;
>  	}
>  	priv->tm->freq = tmrate;
>  
> @@ -1093,7 +1185,11 @@ open_error:
>  #endif
>  	if (priv->phydev)
>  		phy_disconnect(priv->phydev);
> -
> +open_clk_dis:
> +#ifdef CONFIG_HAVE_CLK
> +	if (priv->stmmac_clk)
> +		clk_disable(priv->stmmac_clk);
> +#endif
>  	return ret;
>  }
>  
> @@ -1145,6 +1241,11 @@ static int stmmac_release(struct net_device *dev)
>  #endif
>  	stmmac_mdio_unregister(dev);
>  
> +#ifdef CONFIG_HAVE_CLK
> +	if (priv->stmmac_clk)
> +		clk_disable(priv->stmmac_clk);
> +#endif
> +
>  	return 0;
>  }
>  
> @@ -1844,6 +1945,16 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>  		goto error;
>  	}
>  
> +#ifdef CONFIG_HAVE_CLK
> +	priv->stmmac_clk = clk_get(device, NULL);
> +	if (IS_ERR(priv->stmmac_clk)) {
> +		ret = PTR_ERR(priv->stmmac_clk);
> +		pr_err("%s: ERROR %i clock get \n", __func__, ret);
> +		goto error;
> +	}
> +#endif
> +	stmmac_cpufreq_register(priv);
> +
>  	DBG(probe, DEBUG, "%s: Scatter/Gather: %s - HW checksums: %s\n",
>  	    ndev->name, (ndev->features & NETIF_F_SG) ? "on" : "off",
>  	    (ndev->features & NETIF_F_IP_CSUM) ? "on" : "off");
> @@ -1875,6 +1986,11 @@ int stmmac_dvr_remove(struct net_device *ndev)
>  	priv->hw->dma->stop_tx(priv->ioaddr);
>  
>  	stmmac_set_mac(priv->ioaddr, false);
> +	stmmac_cpufreq_deregister(priv);
> +#ifdef CONFIG_HAVE_CLK
> +	if (priv->stmmac_clk)
> +		clk_put(priv->stmmac_clk);
> +#endif
>  	netif_carrier_off(ndev);
>  	unregister_netdev(ndev);
>  	free_netdev(ndev);

Re: [PATCH 0/6] stmmac: Driver Updates

[Giuseppe CAVALLARO]

Hello Deepak

thanks for these patches.
I've just commented the patches #1 #2 #3.

I've found some problem when apply them in git.net.

I'll look at the patches #4 and #5 tomorrow and then I'll give you my
feedback. These have an impact on old driver versions too.

Patch #6, at first glance looks fine to me. I'll give you mu Acked-by as
soon as I complete the review.

Sorry for the delay

Regards
Peppe

On 3/2/2012 1:55 PM, Deepak SIKRI wrote:
> This patch set modifies the stmmac driver.
> Following changes have been done in the patch set.
> 1. Explicitly have the option to pass checksum offload engine type
> through the platform code. This is helpful for stmmac core prior to
> revision 3.5
> 2. Pass the default value of clk_csr scaling through the platform code.
> Define the scaling macros to be consistently used across driver and plat
> code.
> 3. Update the stmmac driver to incorporate the CPU freq framework.
> 4. Update the DMA descriptor status checks for cores prior to rev-3.5.
> 5. Configure burst related DMA parameter for cores revision beyond
> rev-3.6
> 6. Replace infinite loop in mdio read and write by timeouts
> 
> Deepak Sikri (6):
>   stmmac: Define CSUM offload engine Types
>   stmmac: Define MDC clock selection macros.
>   stmmac: Add support for CPU freq notifiers.
>   stmmac: Update stmmac descriptor checks for stmmac core prior to
>     Rev-3.5.
>   stmmac: configure burst related GMAC DMA parameters
>   stmmac: Replace infinite loops by timeouts in mdio r/w
> 
>  drivers/net/ethernet/stmicro/stmmac/common.h       |    2 +-
>  .../net/ethernet/stmicro/stmmac/dwmac1000_dma.c    |   14 ++-
>  drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h    |    1 +
>  drivers/net/ethernet/stmicro/stmmac/enh_desc.c     |   17 ++-
>  drivers/net/ethernet/stmicro/stmmac/norm_desc.c    |    2 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |   10 ++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  138 +++++++++++++++++++-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  |   30 ++++-
>  include/linux/stmmac.h                             |   36 +++++
>  9 files changed, 229 insertions(+), 21 deletions(-)
> 
> 

Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[Giuseppe CAVALLARO]

Hello Deepak

On 3/2/2012 1:55 PM, Deepak Sikri wrote:
> The patch adds in the following support.
> 1. The stmmac core supporting Type-1 checksum offload engine & prior to
> Revision 3.5, append the HW cecksum at the end of payload in case the Rx
> checksum engine was used to offload the HW checksum.
> There cores did not provide the HW register interface to read the device
> capabilities, and hence the plat code provides the core checksum capabilties
> that help to identify type-1 cores, and adjust the frame length.
> 
> 2. Also, the following Frame status checks with the Full checksum offload
> Type-2 engine enabled are not backward compatible and are reserved for
> STMMAC core revisions prior to 3.5.
> Bit18(Eth Frame)	Bit27(Header Csum Error)	Bit28 (Payload Csum Err)
> 	0			1				1
> 	0			1				0

Type 2 has been introduced starting from the 3.30a and Type 1 maintained
for back-ward compatibility because only available in 3.30.

If we want to actually support Type 1 (I've no HW where test) I guess we
need to review this patch.

First problem I can see in the patch is that, in case of type 1, we have
to properly set the device hw features because full IPC offload is not
supported (e.g. IPv6). This only is true for type 2.

I've just looked at all the MAC data-books starting from the 3.30a to
3.60a and I have seen that all the MACs treat the Receive Checksum
Offload Engine in the same way. I mean, the cores don't append any
payload csum bytes in case of type 2. This is always done for type 1!

Frankly, I prefer to have no define like GMAC_VERSION_35.
I always tried to avoid it :-)... unless there is some critical reason
and we actually need it. Pls, see my comments comments inline below.

> These conditions have been treated as no HW checksum support for stmmac core
> prior to rev-3.5.
> 
> Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/common.h      |    2 +-
>  drivers/net/ethernet/stmicro/stmmac/enh_desc.c    |   17 +++++++++++------
>  drivers/net/ethernet/stmicro/stmmac/norm_desc.c   |    2 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   14 +++++++++++++-
>  4 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index d0b814e..12d1565 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -230,7 +230,7 @@ struct stmmac_desc_ops {
>  	int (*get_rx_frame_len) (struct dma_desc *p);
>  	/* Return the reception status looking at the RDES1 */
>  	int (*rx_status) (void *data, struct stmmac_extra_stats *x,
> -			  struct dma_desc *p);
> +			  struct dma_desc *p, u32 mac_id);
>  };
>  
>  struct stmmac_dma_ops {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> index d879763..42026f6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> @@ -22,6 +22,7 @@
>    Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>  *******************************************************************************/
>  
> +#include <linux/stmmac.h>
>  #include "common.h"
>  #include "descs_com.h"
>  
> @@ -106,7 +107,9 @@ static int enh_desc_get_tx_len(struct dma_desc *p)
>  	return p->des01.etx.buffer1_size;
>  }
>  
> -static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err)
> +#define GMAC_VERSION_35 0x35
> +static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err,
> +		u32 mac_id)
>  {
>  	int ret = good_frame;
>  	u32 status = (type << 2 | ipc_err << 1 | payload_err) & 0x7;
> @@ -141,16 +144,16 @@ static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err)
>  	} else if (status == 0x1) {
>  		CHIP_DBG(KERN_ERR
>  		    "RX Des0 status: IPv4/6 unsupported IP PAYLOAD.\n");
> -		ret = discard_frame;
> +		ret = (mac_id >= GMAC_VERSION_35) ? discard_frame : csum_none;
>  	} else if (status == 0x3) {
>  		CHIP_DBG(KERN_ERR "RX Des0 status: No IPv4, IPv6 frame.\n");
> -		ret = discard_frame;
> +		ret = (mac_id >= GMAC_VERSION_35) ? discard_frame : csum_none;
>  	}
>  	return ret;
>  }

The enh_desc_coe_rdes0 parses the Receive Descriptor 0 When COE (Type
2). It should be onlyinvoked on full csum case.
We also should discard the frames on the latest two cases I mean when:

- IPv4/IPv6 Type frame with no IP header checksum error and the payload
check bypassed, due to an unsupported payload

- A Type frame that is neither IPv4 or IPv6 (the Checksum Offload engine
bypasses checksum completely.)

Also from all the Synopsys databooks I cannot see any difference in the
Table 7.2 when treat the RDES0 bits 0, 5, 7.

So I expect to have no check for the GMAC_VERSION_35 inside the enh desc
file.

>  static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
> -				  struct dma_desc *p)
> +				  struct dma_desc *p, u32 mac_id)
>  {
>  	int ret = good_frame;
>  	struct net_device_stats *stats = (struct net_device_stats *)data;
> @@ -195,9 +198,11 @@ static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
>  	/* After a payload csum error, the ES bit is set.
>  	 * It doesn't match with the information reported into the databook.
>  	 * At any rate, we need to understand if the CSUM hw computation is ok
> -	 * and report this info to the upper layers. */
> +	 * and report this info to the upper layers.
> +	 */

This is useless change in the patch that should be removed in the final
version.

>  	ret = enh_desc_coe_rdes0(p->des01.erx.ipc_csum_error,
> -		p->des01.erx.frame_type, p->des01.erx.payload_csum_error);
> +			p->des01.erx.frame_type,
> +			p->des01.erx.payload_csum_error, mac_id);
>  
>  	if (unlikely(p->des01.erx.dribbling)) {
>  		CHIP_DBG(KERN_ERR "GMAC RX: dribbling error\n");
> diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> index fda5d2b..057a728 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> @@ -72,7 +72,7 @@ static int ndesc_get_tx_len(struct dma_desc *p)
>   * In case of success, it returns good_frame because the GMAC device
>   * is supposed to be able to compute the csum in HW. */
>  static int ndesc_get_rx_status(void *data, struct stmmac_extra_stats *x,
> -			       struct dma_desc *p)
> +			       struct dma_desc *p, u32 mac_id)
>  {
>  	int ret = good_frame;
>  	struct net_device_stats *stats = (struct net_device_stats *)data;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 001b8f3..3bcdc97 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1103,6 +1103,7 @@ static int stmmac_open(struct net_device *dev)
>  	priv->rx_coe = priv->hw->mac->rx_coe(priv->ioaddr);
>  	if (priv->rx_coe)
>  		pr_info(" Checksum Offload Engine supported\n");
> +

do not add useless spaces.

>  	if (priv->plat->tx_coe)
>  		pr_info(" Checksum insertion supported\n");

Here I expect to see more information about the RX COE ;-)

I'd like to see:
  pr_info(" Checksum Offload Engine supported (type %d)\n", ....);

>  
> @@ -1436,7 +1437,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
>  
>  		/* read the status of the incoming frame */
>  		status = (priv->hw->desc->rx_status(&priv->dev->stats,
> -						    &priv->xstats, p));
> +					&priv->xstats, p,
> +					priv->hw->synopsys_uid & 0xff));
>  		if (unlikely(status == discard_frame))
>  			priv->dev->stats.rx_errors++;
>  		else {
> @@ -1444,6 +1446,16 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
>  			int frame_len;
>  
>  			frame_len = priv->hw->desc->get_rx_frame_len(p);
> +			/*
> +			 * The type-1 checksume offload engines append
> +			 * the checksum at the end of frame, and the
> +			 * the two bytes of checksum are added in
> +			 * length.
> +			 * Adjust for that in the framelen for type-1
> +			 * checksum offload engines.
> +			 */
> +			if (priv->plat->csum_off_engine_type == STMMAC_CSUM_T1)
> +				frame_len -= 2;

I'd like to have this inside the core. I mean, get_rx_frame_len returns
the len - 2 in case of type 1.

>  			/* ACS is set; GMAC core strips PAD/FCS for IEEE 802.3
>  			 * Type frames (LLC/LLC-SNAP) */
>  			if (unlikely(status != llc_snap))

Re: [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters

[Giuseppe CAVALLARO]

Hello Deepak,

On 3/2/2012 1:55 PM, Deepak Sikri wrote:
> SPEAr1340 GMAC is a different version (3.61a) of Synopsys IP where
> instead of 4xPBL we have 8xPBL, hence pbl value supplied by platform
> data in
> 	- SPEAr1340 results in 8 * pbl
> 	- rest devices result in 4 * pbl
> 
> Further it is observed that rest of the devices (older version) which
> have an AXI wrapper over AHB are limited  to incr 32 burst where as this
> can go up to incr 128 in case of SPEAr1340.
> 
> Also, with fixed burst configuration we need to program permissible
> burst values in newer versions (AXI supported) of gmac. This
> AXI_BUS_MODE) register is reserved for earlier versions of gmac and
> writing to them has no impact.
> 
> Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> Signed-off-by: Deepak Sikri <deepak.sikri@st.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac1000_dma.c    |   14 +++++++++++++-
>  drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h    |    1 +
>  2 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> index 4d5402a..ed6ffa3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> @@ -48,7 +48,7 @@ static int dwmac1000_dma_init(void __iomem *ioaddr, int pbl, u32 dma_tx,
>  	if (limit < 0)
>  		return -EBUSY;
>  
> -	value = /* DMA_BUS_MODE_FB | */ DMA_BUS_MODE_4PBL |
> +	value = DMA_BUS_MODE_FB | DMA_BUS_MODE_4PBL |
>  	    ((pbl << DMA_BUS_MODE_PBL_SHIFT) |
>  	     (pbl << DMA_BUS_MODE_RPBL_SHIFT));


bit 24 in the Bus Mode Register represents the 8xPBL in databooks newer
than the 3.50a. In older databook it is 4xPBL.
So maybe, the DMA_BUS_MODE_4PBL define should be renamed as
DMA_BUS_PBL_MODE (to be more generic and surrounded by a nice comment
that details its meaning for different devices).

but ...

> @@ -56,6 +56,18 @@ static int dwmac1000_dma_init(void __iomem *ioaddr, int pbl, u32 dma_tx,
>  	value |= DMA_BUS_MODE_DA;	/* Rx has priority over tx */
>  #endif
>  	writel(value, ioaddr + DMA_BUS_MODE);
> +	/*
> +	 * We need to program DMA_AXI_BUS_MODE for supported bursts in
> +	 * case DMA_BUS_MODE_FB mode is selected
> +	 * Note: This is applicable only for revision GMACv3.61a. For
> +	 * older version this register is reserved and shall have no
> +	 * effect.
> +	 * Further we directly write 0xFF to this register. This would
> +	 * ensure that all bursts supported by platform is set and those
> +	 * which are not supported would remain ineffective.
> +	 */
> +	if (value & DMA_BUS_MODE_FB)
> +		writel(0xFF, ioaddr + DMA_AXI_BUS_MODE);

... this patch sets the FB: Fixed Burst (not set on other GMAC!) and
then always writes into the DMA_AXI_BUS_MODE register.

I agree that the DMA_AXI_BUS_MODE Reg is reserved in old devices but I'd
like to have a new platform parameter to do this kind of thing.

We could have .fixed_burst in the linux/stmmac.h (like .pbl).

If it is passed, so we set the DMA_BUS_MODE_FB in the DMA Reg 0.

In this case, we can also set the DMA reg11 but, pls, not using 0xff.
I wonder if you could improve the code adding the defines for this register.

With this approach we have no impact on other GMAC!

peppe

>  	/* Mask interrupts by writing to CSR7 */
>  	writel(DMA_INTR_DEFAULT_MASK, ioaddr + DMA_INTR_ENA);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> index 437edac..6e0360f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> @@ -32,6 +32,7 @@
>  #define DMA_CONTROL		0x00001018	/* Ctrl (Operational Mode) */
>  #define DMA_INTR_ENA		0x0000101c	/* Interrupt Enable */
>  #define DMA_MISSED_FRAME_CTR	0x00001020	/* Missed Frame Counter */
> +#define DMA_AXI_BUS_MODE       0x00001028      /* AXI Bus Mode */
>  #define DMA_CUR_TX_BUF_ADDR	0x00001050	/* Current Host Tx Buffer */
>  #define DMA_CUR_RX_BUF_ADDR	0x00001054	/* Current Host Rx Buffer */
>  #define DMA_HW_FEATURE		0x00001058	/* HW Feature Register */

Re: [PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w

[Giuseppe CAVALLARO]

On 3/2/2012 1:55 PM, Deepak Sikri wrote:
> This patch removes the infinite waits from the mdio read and
> write interfaces. These infinite waits have been replaced by
> the timeout handling. In case if a time out occurs, an error is
> returned.
> 
> Signed-off-by: Deepak Sikri <deepak.sikri@st.com>

Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c |   30 ++++++++++++++++-----
>  1 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index 7319532..b6a6fb2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -34,6 +34,20 @@
>  #define MII_BUSY 0x00000001
>  #define MII_WRITE 0x00000002
>  
> +static int stmmac_mdio_busy_wait(unsigned long ioaddr, unsigned int mii_addr)
> +{
> +	unsigned long finish = jiffies + 3 * HZ;
> +
> +	do {
> +		if (readl(ioaddr + mii_addr) & MII_BUSY)
> +			cpu_relax();
> +		else
> +			return 0;
> +	} while (!time_after_eq(jiffies, finish));
> +
> +	return -EBUSY;
> +}
> +
>  /**
>   * stmmac_mdio_read
>   * @bus: points to the mii_bus structure
> @@ -56,9 +70,13 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
>  			((phyreg << 6) & (0x000007C0)));
>  	regValue |= MII_BUSY | ((priv->plat->clk_csr & 7) << 2);
>  
> -	do {} while (((readl(priv->ioaddr + mii_address)) & MII_BUSY) == 1);
> +	if (stmmac_mdio_busy_wait(priv->ioaddr, mii_address))
> +		return -EBUSY;
> +
>  	writel(regValue, priv->ioaddr + mii_address);
> -	do {} while (((readl(priv->ioaddr + mii_address)) & MII_BUSY) == 1);
> +
> +	if (stmmac_mdio_busy_wait(priv->ioaddr, mii_address))
> +		return -EBUSY;
>  
>  	/* Read the data from the MII data register */
>  	data = (int)readl(priv->ioaddr + mii_data);
> @@ -88,18 +106,16 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg,
>  
>  	value |= MII_BUSY | ((priv->plat->clk_csr & 7) << 2);
>  
> -
>  	/* Wait until any existing MII operation is complete */
> -	do {} while (((readl(priv->ioaddr + mii_address)) & MII_BUSY) == 1);
> +	if (stmmac_mdio_busy_wait(priv->ioaddr, mii_address))
> +		return -EBUSY;
>  
>  	/* Set the MII address register to write */
>  	writel(phydata, priv->ioaddr + mii_data);
>  	writel(value, priv->ioaddr + mii_address);
>  
>  	/* Wait until any existing MII operation is complete */
> -	do {} while (((readl(priv->ioaddr + mii_address)) & MII_BUSY) == 1);
> -
> -	return 0;
> +	return stmmac_mdio_busy_wait(priv->ioaddr, mii_address);
>  }
>  
>  /**

Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers.

[Giuseppe CAVALLARO]

On 3/5/2012 4:05 PM, Giuseppe CAVALLARO wrote:
[snip]
>> +#ifdef CONFIG_CPU_FREQ
>> +static inline void stmmac_clk_csr_set(struct stmmac_priv *priv, int clk_rate)
>> +{
>> +
>> +	if ((clk_rate >= CSR_F_20M) && (clk_rate < CSR_F_35M))
>> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_20_35M;
>> +	else if ((clk_rate >= CSR_F_35M) && (clk_rate < CSR_F_60M))
>> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_35_60M;
>> +	else if ((clk_rate >= CSR_F_60M) && (clk_rate < CSR_F_100M))
>> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_60_100M;
>> +	else if ((clk_rate >= CSR_F_100M) && (clk_rate < CSR_F_150M))
>> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_100_150M;
>> +	else if ((clk_rate >= CSR_F_150M) && (clk_rate < CSR_F_250M))
>> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;
>> +	else if ((clk_rate >= CSR_F_250M) && (clk_rate < CSR_F_300M))
>> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_250_300M;
>> +	else
>> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;


Another note is that you are overriding the priv->plat->clk_csr that was
passed through the platform and used in some case on other architectures.

peppe

Re: [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters

[deepaksi]

Hi David,

On 3/5/2012 7:22 AM, David Miller wrote:
> From: Deepak Sikri<deepak.sikri@st.com>
> Date: Fri, 2 Mar 2012 18:25:27 +0530
>
>>   	writel(value, ioaddr + DMA_BUS_MODE);
>> +	/*
>> +	 * We need to program DMA_AXI_BUS_MODE for supported bursts in
>> +	 * case DMA_BUS_MODE_FB mode is selected
> Please fix comment formatting.
>
This would be fixed.

Thanks
Deepak

Re: [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters

[deepaksi]

Hellp Giuseppe,

>
> bit 24 in the Bus Mode Register represents the 8xPBL in databooks newer
> than the 3.50a. In older databook it is 4xPBL.
> So maybe, the DMA_BUS_MODE_4PBL define should be renamed as
> DMA_BUS_PBL_MODE (to be more generic and surrounded by a nice comment
> that details its meaning for different devices).
>
> but ...

Ok, will do that

>> @@ -56,6 +56,18 @@ static int dwmac1000_dma_init(void __iomem *ioaddr, int pbl, u32 dma_tx,
>>   	value |= DMA_BUS_MODE_DA;	/* Rx has priority over tx */
>>   #endif
>>   	writel(value, ioaddr + DMA_BUS_MODE);
>> +	/*
>> +	 * We need to program DMA_AXI_BUS_MODE for supported bursts in
>> +	 * case DMA_BUS_MODE_FB mode is selected
>> +	 * Note: This is applicable only for revision GMACv3.61a. For
>> +	 * older version this register is reserved and shall have no
>> +	 * effect.
>> +	 * Further we directly write 0xFF to this register. This would
>> +	 * ensure that all bursts supported by platform is set and those
>> +	 * which are not supported would remain ineffective.
>> +	 */
>> +	if (value&  DMA_BUS_MODE_FB)
>> +		writel(0xFF, ioaddr + DMA_AXI_BUS_MODE);
> ... this patch sets the FB: Fixed Burst (not set on other GMAC!) and
> then always writes into the DMA_AXI_BUS_MODE register.
>
> I agree that the DMA_AXI_BUS_MODE Reg is reserved in old devices but I'd
> like to have a new platform parameter to do this kind of thing.
>
> We could have .fixed_burst in the linux/stmmac.h (like .pbl).
>
> If it is passed, so we set the DMA_BUS_MODE_FB in the DMA Reg 0.
>
> In this case, we can also set the DMA reg11 but, pls, not using 0xff.
> I wonder if you could improve the code adding the defines for this register.
>
> With this approach we have no impact on other GMAC!
>
> peppe

Looks good, will use the <include/linux/stmmac.h> for passing the fixed 
burst option, and also
program the DMA register accordingly.

Along with that, we may need to program following things.
- For Fixed burst mode, the fixed burst length that has to be programmed.
- For no-fixed burst mode, the maximum allowed burst length need to be set.

This is in sync with the following information captured from the stmmac 
core manual for the
bit-0  of the DMA AXI Bus Mode register

**********************************************************************
UNDEF: AXI Undefined Burst Length
This bit is read-only bit and indicates the complement (invert) value of 
FB bit
in Register0 (Bus Mode Register[16]).
• When this bit is set to 1, the GMAC-AXI is allowed to perform any burst
length equal to or below the maximum allowed burst length as
programmed in bits[7:1]
• When this bit is set to 0, the GMAC-AXI is allowed to perform only fixed
burst lengths as indicated by BLEN256/128/64/32/16/8/4, or a burst
length of 1.
*********************************************************************

It would be better if additionally we pass the burst length, signifying the
- Fixed burst length for fixed burst mode
- Max Burst length for non-fixed burst mode.


Regards
Deepak

Re: [PATCH 1/6] stmmac: Define CSUM offload engine Types

[deepaksi]

Hello Peppe,

> this patch is useful but I've some notes.
>
> Pay attention that, new GMAC devices have the HW capability register to
> understand if the rx_coe_type is TYPE1 or TYPE2.
> These values should always override the ones come from the platform.
> This check is missing in your code.
> Can I ask you to rename csum_off_engine_type as rx_coe_type to be
> aligned to the name convention used inside the driver.
>
> Regards
> Peppe

Will change the naming convention in this file as well as driver.

Regards
Deepak

Re: [PATCH 2/6] stmmac: Define MDC clock selection macros.

[deepaksi]

Hello Peppe,

>
> I have some concerns about this patch.
>
> We want to have some defines to help on setting the clk_csr (that is is
> a clk divisor).
>
> When you program the "CSR Clock Range" bits in the GMII Address Register
> you can also set the bit 5 (not supported in older devices e.g. 3.41a).
> In this case, the defines below do not cover all the cases, I mean:
>
> 1000 clk_csr_i/4
> 1001 clk_csr_i/6
> 1010 clk_csr_i/8
> 1011 clk_csr_i/10
> 1100 clk_csr_i/12
> 1101 clk_csr_i/14
> 1110 clk_csr_i/16
> 1111 clk_csr_i/18

I agree that these macros have been missed. But lets take the change 
suggested as a separate patch,
as this would then be integrated along with the driver.
The driver by default is considering the 2.5MHz MDIO clock option only. 
In this case we require an extra
variable to differentiate specification which is higher than IEEE spec 
of 2.5MHz.

>> +/* MDC Clock Selection define*/
>> +#define	STMMAC_CLK_RANGE_60_100M	0	/* MDC = Clk/42 */
>> +#define	STMMAC_CLK_RANGE_100_150M	1	/* MDC = Clk/62 */
>> +#define	STMMAC_CLK_RANGE_20_35M		2	/* MDC = Clk/16 */
>> +#define	STMMAC_CLK_RANGE_35_60M		3	/* MDC = Clk/26 */
>> +#define	STMMAC_CLK_RANGE_150_250M	4	/* MDC = Clk/102 */
>> +#define	STMMAC_CLK_RANGE_250_300M	5	/* MDC = Clk/122 */
> I suggest you to rename these macros as:
>
> #define STMMAC_CSR_60_100M	0	/* MDC = Clk/42 */
> ...

Ok

>
> Also, macros CSR_F_20M should be totally removed.

ok

> Peppe
>
>> +
>>   /* Platfrom data for platform device structure's platform_data field */
>>
>>   struct stmmac_mdio_bus_data {
Regards
Deepak

Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers.

[deepaksi]

Hello Peppe,

> Hello Deepak
>
> this patch doesn't apply on net.git.

I would rebase it again on net.git and send
>
> Concerning the stmmac_clk_csr_set it should treat all the other divisor
> values as I've just told you in the email #2.

I looked through the other divisor values also,. The input clock range 
coming through those divisors were
very much overlapping the 2.5 MHz range.

As I had requested to consider the 12.5 MHz MDIO clock provision as a 
separate patch, I would seek
some more discussions on this particular thing.

> All the defines from the patch #2:
>
> +/* CSR Frequency Access Defines*/
> +#define CSR_F_20M	20000000
> +#define CSR_F_35M	35000000
> +#define CSR_F_60M	60000000
> +#define CSR_F_100M	100000000
> +#define CSR_F_150M	150000000
> +#define CSR_F_250M	50000000
> +#define CSR_F_300M	300000000
>
>
> should be moved from linux/stmmac.h to stmicro/stmmac/common.h

ok

>
> In the end, I kindly ask you to try to remove some of not necessary
> #ifdef CONFIG_HAVE_CLK  inside the C file.
>
> Indeed stmmac_clk is be null in case of there is not this support.
>
> Also I wonder if stmmac_clk could be renamed as: stmmac_csr_clk.

Its only at one place that we will need the conditional macro, while we 
do clk_get. Rest of the
macros could be do away with.
I would also rename the stmmac_clk to stmmac_csr_clk.

>
> Pls, resend the patch against net-git so i'll be happy to continue the
> review ;-)
Sure


Regards
Deepak

Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers.

[deepaksi]

Hi David

+		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;
+
+}

> Unnecessary empty lines, remove them.

ok

>> +			/*
>> +			 * Decide on the MDC clock dynamically based on the
>> +			 * csr clock input.
> Format comments:
>
> 	/* Like
> 	 * this.
> 	 */
>
> Not:
>
> 	/*
> 	 * Like
> 	 * this.
> 	 */

ok

>> +static inline void stmmac_cpufreq_deregister(struct stmmac_priv *priv)
>> +{
>> +
>> +	cpufreq_unregister_notifier(&priv->freq_transition,
> Unnecessary empty line.
>

Will remove.

Regards
Deepak

Re: [PATCH 2/6] stmmac: Define MDC clock selection macros.

[Giuseppe CAVALLARO]

On 3/7/2012 7:55 AM, Deepak SIKRI wrote:
> Hello Peppe,
> 
>>
>> I have some concerns about this patch.
>>
>> We want to have some defines to help on setting the clk_csr (that is is
>> a clk divisor).
>>
>> When you program the "CSR Clock Range" bits in the GMII Address Register
>> you can also set the bit 5 (not supported in older devices e.g. 3.41a).
>> In this case, the defines below do not cover all the cases, I mean:
>>
>> 1000 clk_csr_i/4
>> 1001 clk_csr_i/6
>> 1010 clk_csr_i/8
>> 1011 clk_csr_i/10
>> 1100 clk_csr_i/12
>> 1101 clk_csr_i/14
>> 1110 clk_csr_i/16
>> 1111 clk_csr_i/18
> 
> I agree that these macros have been missed. But lets take the change
> suggested as a separate patch,
> as this would then be integrated along with the driver.
> The driver by default is considering the 2.5MHz MDIO clock option only.
> In this case we require an extra
> variable to differentiate specification which is higher than IEEE spec
> of 2.5MHz.

ok, we will have them in a another patch. At any rate, I ask you to add
a FIXME in the code and detail this issue in the patch comment.

> 
>>> +/* MDC Clock Selection define*/
>>> +#define    STMMAC_CLK_RANGE_60_100M    0    /* MDC = Clk/42 */
>>> +#define    STMMAC_CLK_RANGE_100_150M    1    /* MDC = Clk/62 */
>>> +#define    STMMAC_CLK_RANGE_20_35M        2    /* MDC = Clk/16 */
>>> +#define    STMMAC_CLK_RANGE_35_60M        3    /* MDC = Clk/26 */
>>> +#define    STMMAC_CLK_RANGE_150_250M    4    /* MDC = Clk/102 */
>>> +#define    STMMAC_CLK_RANGE_250_300M    5    /* MDC = Clk/122 */
>> I suggest you to rename these macros as:
>>
>> #define STMMAC_CSR_60_100M    0    /* MDC = Clk/42 */
>> ...
> 
> Ok
> 
>>
>> Also, macros CSR_F_20M should be totally removed.
> 
> ok
> 
>> Peppe
>>
>>> +
>>>   /* Platfrom data for platform device structure's platform_data
>>> field */
>>>
>>>   struct stmmac_mdio_bus_data {
> Regards
> Deepak
> 

Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[deepaksi]

Hi Peppe,


> Type 2 has been introduced starting from the 3.30a and Type 1 maintained
> for back-ward compatibility because only available in 3.30.
>
> If we want to actually support Type 1 (I've no HW where test) I guess we
> need to review this patch.
>
> First problem I can see in the patch is that, in case of type 1, we have
> to properly set the device hw features because full IPC offload is not
> supported (e.g. IPv6). This only is true for type 2.
>
> I've just looked at all the MAC data-books starting from the 3.30a to
> 3.60a and I have seen that all the MACs treat the Receive Checksum
> Offload Engine in the same way. I mean, the cores don't append any
> payload csum bytes in case of type 2. This is always done for type 1!
>
> Frankly, I prefer to have no define like GMAC_VERSION_35.
> I always tried to avoid it :-)... unless there is some critical reason
> and we actually need it. Pls, see my comments comments inline below.
There are two issues to be addresses.
1. Issue-1 : For the Type-1 Rx COE, the frame length has to be adjusted 
by 2.
2. The two frame status conditions that have been marked as csum_none 
for the versions 3.3a and
earlier.

I would take them along the review comments below



>>   	} else if (status == 0x1) {
>>   		CHIP_DBG(KERN_ERR
>>   		    "RX Des0 status: IPv4/6 unsupported IP PAYLOAD.\n");
>> -		ret = discard_frame;
>> +		ret = (mac_id>= GMAC_VERSION_35) ? discard_frame : csum_none;
>>   	} else if (status == 0x3) {
>>   		CHIP_DBG(KERN_ERR "RX Des0 status: No IPv4, IPv6 frame.\n");
>> -		ret = discard_frame;
>> +		ret = (mac_id>= GMAC_VERSION_35) ? discard_frame : csum_none;
>>   	}
>>   	return ret;
>>   }
> The enh_desc_coe_rdes0 parses the Receive Descriptor 0 When COE (Type
> 2). It should be onlyinvoked on full csum case.
> We also should discard the frames on the latest two cases I mean when:
>
> - IPv4/IPv6 Type frame with no IP header checksum error and the payload
> check bypassed, due to an unsupported payload
>
> - A Type frame that is neither IPv4 or IPv6 (the Checksum Offload engine
> bypasses checksum completely.)
>
> Also from all the Synopsys databooks I cannot see any difference in the
> Table 7.2 when treat the RDES0 bits 0, 5, 7.
>
> So I expect to have no check for the GMAC_VERSION_35 inside the enh desc
> file.

I can cross verify this on the SPEAr test platform which we had been 
using. We had faced some issue
related to this before also.

>>   static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
>> -				  struct dma_desc *p)
>> +				  struct dma_desc *p, u32 mac_id)
>>   {
>>   	int ret = good_frame;
>>   	struct net_device_stats *stats = (struct net_device_stats *)data;
>> @@ -195,9 +198,11 @@ static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
>>   	/* After a payload csum error, the ES bit is set.
>>   	 * It doesn't match with the information reported into the databook.
>>   	 * At any rate, we need to understand if the CSUM hw computation is ok
>> -	 * and report this info to the upper layers. */
>> +	 * and report this info to the upper layers.
>> +	 */
> This is useless change in the patch that should be removed in the final
> version.

ok

>   	if (priv->rx_coe)
>   		pr_info(" Checksum Offload Engine supported\n");
> +
> do not add useless spaces.

ok

>>   	if (priv->plat->tx_coe)
>>   		pr_info(" Checksum insertion supported\n");
> Here I expect to see more information about the RX COE ;-)
>
> I'd like to see:
>    pr_info(" Checksum Offload Engine supported (type %d)\n", ....);

Sure, will do that

>>
>> @@ -1436,7 +1437,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
>>
>>   		/* read the status of the incoming frame */
>>   		status = (priv->hw->desc->rx_status(&priv->dev->stats,
>> -						&priv->xstats, p));
>> +					&priv->xstats, p,
>> +					priv->hw->synopsys_uid&  0xff));
>>   		if (unlikely(status == discard_frame))
>>   			priv->dev->stats.rx_errors++;
>>   		else {
>> @@ -1444,6 +1446,16 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit)
>>   			int frame_len;
>>
>>   			frame_len = priv->hw->desc->get_rx_frame_len(p);
>> +			/*
>> +			 * The type-1 checksume offload engines append
>> +			 * the checksum at the end of frame, and the
>> +			 * the two bytes of checksum are added in
>> +			 * length.
>> +			 * Adjust for that in the framelen for type-1
>> +			 * checksum offload engines.
>> +			 */
>> +			if (priv->plat->csum_off_engine_type == STMMAC_CSUM_T1)
>> +				frame_len -= 2;
> I'd like to have this inside the core. I mean, get_rx_frame_len returns
> the len - 2 in case of type 1.

Thats fine. Will do that

Regards
Deepak

Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[deepaksi]

Hi David,

On 3/5/2012 7:21 AM, David Miller wrote:
> From: Deepak Sikri<deepak.sikri@st.com>
> Date: Fri, 2 Mar 2012 18:25:26 +0530
>
>> +static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err,
>> +		u32 mac_id)
> Poorly formatted, this should be:
Sorry for that.. Will rectify that

> static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err,
> 			      u32 mac_id)
>
>
>> +			/*
>> +			 * The type-1 checksume offload engines append
>> +			 * the checksum at the end of frame, and the
> 	/* Format comments
> 	 * like this.
> 	 */
>
> 	/*
> 	 * Not
> 	 * like this.
> 	 */
>

Sure..  will do that


Regards
Deepak

Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers.

[deepaksi]

Hi Peppe,

On 3/6/2012 1:34 PM, Giuseppe CAVALLARO wrote:
> On 3/5/2012 4:05 PM, Giuseppe CAVALLARO wrote:
> [snip]
>>> +	else
>>> +		priv->plat->clk_csr = STMMAC_CLK_RANGE_150_250M;
>
> Another note is that you are overriding the priv->plat->clk_csr that was
> passed through the platform and used in some case on other architectures.
>
> peppe

I would cross verify and update the code.

Regards
Deepak

Re: [PATCH 2/6] stmmac: Define MDC clock selection macros.

[deepaksi]

On 3/7/2012 12:49 PM, Giuseppe CAVALLARO wrote:
> On 3/7/2012 7:55 AM, Deepak SIKRI wrote:
>> Hello Peppe,
>>
>>> I have some concerns about this patch.
>>>
>>> We want to have some defines to help on setting the clk_csr (that is is
>>> a clk divisor).
>>>
>>> When you program the "CSR Clock Range" bits in the GMII Address Register
>>> you can also set the bit 5 (not supported in older devices e.g. 3.41a).
>>> In this case, the defines below do not cover all the cases, I mean:
>>>
>>> 1000 clk_csr_i/4
>>> 1001 clk_csr_i/6
>>> 1010 clk_csr_i/8
>>> 1011 clk_csr_i/10
>>> 1100 clk_csr_i/12
>>> 1101 clk_csr_i/14
>>> 1110 clk_csr_i/16
>>> 1111 clk_csr_i/18
>> I agree that these macros have been missed. But lets take the change
>> suggested as a separate patch,
>> as this would then be integrated along with the driver.
>> The driver by default is considering the 2.5MHz MDIO clock option only.
>> In this case we require an extra
>> variable to differentiate specification which is higher than IEEE spec
>> of 2.5MHz.
> ok, we will have them in a another patch. At any rate, I ask you to add
> a FIXME in the code and detail this issue in the patch comment.

Sure, I will do the need full.

>>>> +/* MDC Clock Selection define*/
>>>> +#define    STMMAC_CLK_RANGE_60_100M    0    /* MDC = Clk/42 */
>>>> +#define    STMMAC_CLK_RANGE_100_150M    1    /* MDC = Clk/62 */
>>>> +#define    STMMAC_CLK_RANGE_20_35M        2    /* MDC = Clk/16 */
>>>> +#define    STMMAC_CLK_RANGE_35_60M        3    /* MDC = Clk/26 */
>>>> +#define    STMMAC_CLK_RANGE_150_250M    4    /* MDC = Clk/102 */
>>>> +#define    STMMAC_CLK_RANGE_250_300M    5    /* MDC = Clk/122 */
>>> I suggest you to rename these macros as:
>>>
>>> #define STMMAC_CSR_60_100M    0    /* MDC = Clk/42 */
>>> ...
>> Ok
>>
>>> Also, macros CSR_F_20M should be totally removed.
>> ok
>>
>>> Peppe
>>>
>>>> +
>>>>    /* Platfrom data for platform device structure's platform_data
>>>> field */
>>>>
>>>>    struct stmmac_mdio_bus_data {
>> Regards
>> Deepak
>>

Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5.

[Giuseppe CAVALLARO]

On 3/7/2012 9:25 AM, Deepak SIKRI wrote:
> Hi Peppe,
> 
> 
>> Type 2 has been introduced starting from the 3.30a and Type 1 maintained
>> for back-ward compatibility because only available in 3.30.
>>
>> If we want to actually support Type 1 (I've no HW where test) I guess we
>> need to review this patch.
>>
>> First problem I can see in the patch is that, in case of type 1, we have
>> to properly set the device hw features because full IPC offload is not
>> supported (e.g. IPv6). This only is true for type 2.
>>
>> I've just looked at all the MAC data-books starting from the 3.30a to
>> 3.60a and I have seen that all the MACs treat the Receive Checksum
>> Offload Engine in the same way. I mean, the cores don't append any
>> payload csum bytes in case of type 2. This is always done for type 1!
>>
>> Frankly, I prefer to have no define like GMAC_VERSION_35.
>> I always tried to avoid it :-)... unless there is some critical reason
>> and we actually need it. Pls, see my comments comments inline below.
> There are two issues to be addresses.
> 1. Issue-1 : For the Type-1 Rx COE, the frame length has to be adjusted
> by 2.

agree but this could directly be done in get_rx_frame_len

> 2. The two frame status conditions that have been marked as csum_none
> for the versions 3.3a and
> earlier.

Earlier MACs have no Type 2 and the status condition  enh_desc_coe_rdes0
only is for MAC that has Type 2
> 
> I would take them along the review comments below
> 
> 
> 
>>>       } else if (status == 0x1) {
>>>           CHIP_DBG(KERN_ERR
>>>               "RX Des0 status: IPv4/6 unsupported IP PAYLOAD.\n");
>>> -        ret = discard_frame;
>>> +        ret = (mac_id>= GMAC_VERSION_35) ? discard_frame : csum_none;
>>>       } else if (status == 0x3) {
>>>           CHIP_DBG(KERN_ERR "RX Des0 status: No IPv4, IPv6 frame.\n");
>>> -        ret = discard_frame;
>>> +        ret = (mac_id>= GMAC_VERSION_35) ? discard_frame : csum_none;
>>>       }
>>>       return ret;
>>>   }
>> The enh_desc_coe_rdes0 parses the Receive Descriptor 0 When COE (Type
>> 2). It should be onlyinvoked on full csum case.
>> We also should discard the frames on the latest two cases I mean when:
>>
>> - IPv4/IPv6 Type frame with no IP header checksum error and the payload
>> check bypassed, due to an unsupported payload
>>
>> - A Type frame that is neither IPv4 or IPv6 (the Checksum Offload engine
>> bypasses checksum completely.)
>>
>> Also from all the Synopsys databooks I cannot see any difference in the
>> Table 7.2 when treat the RDES0 bits 0, 5, 7.
>>
>> So I expect to have no check for the GMAC_VERSION_35 inside the enh desc
>> file.
> 
> I can cross verify this on the SPEAr test platform which we had been
> using. We had faced some issue
> related to this before also.
> 
>>>   static int enh_desc_get_rx_status(void *data, struct
>>> stmmac_extra_stats *x,
>>> -                  struct dma_desc *p)
>>> +                  struct dma_desc *p, u32 mac_id)
>>>   {
>>>       int ret = good_frame;
>>>       struct net_device_stats *stats = (struct net_device_stats *)data;
>>> @@ -195,9 +198,11 @@ static int enh_desc_get_rx_status(void *data,
>>> struct stmmac_extra_stats *x,
>>>       /* After a payload csum error, the ES bit is set.
>>>        * It doesn't match with the information reported into the
>>> databook.
>>>        * At any rate, we need to understand if the CSUM hw
>>> computation is ok
>>> -     * and report this info to the upper layers. */
>>> +     * and report this info to the upper layers.
>>> +     */
>> This is useless change in the patch that should be removed in the final
>> version.
> 
> ok
> 
>>       if (priv->rx_coe)
>>           pr_info(" Checksum Offload Engine supported\n");
>> +
>> do not add useless spaces.
> 
> ok
> 
>>>       if (priv->plat->tx_coe)
>>>           pr_info(" Checksum insertion supported\n");
>> Here I expect to see more information about the RX COE ;-)
>>
>> I'd like to see:
>>    pr_info(" Checksum Offload Engine supported (type %d)\n", ....);
> 
> Sure, will do that

:-)

> 
>>>
>>> @@ -1436,7 +1437,8 @@ static int stmmac_rx(struct stmmac_priv *priv,
>>> int limit)
>>>
>>>           /* read the status of the incoming frame */
>>>           status = (priv->hw->desc->rx_status(&priv->dev->stats,
>>> -                        &priv->xstats, p));
>>> +                    &priv->xstats, p,
>>> +                    priv->hw->synopsys_uid&  0xff));
>>>           if (unlikely(status == discard_frame))
>>>               priv->dev->stats.rx_errors++;
>>>           else {
>>> @@ -1444,6 +1446,16 @@ static int stmmac_rx(struct stmmac_priv *priv,
>>> int limit)
>>>               int frame_len;
>>>
>>>               frame_len = priv->hw->desc->get_rx_frame_len(p);
>>> +            /*
>>> +             * The type-1 checksume offload engines append
>>> +             * the checksum at the end of frame, and the
>>> +             * the two bytes of checksum are added in
>>> +             * length.
>>> +             * Adjust for that in the framelen for type-1
>>> +             * checksum offload engines.
>>> +             */
>>> +            if (priv->plat->csum_off_engine_type == STMMAC_CSUM_T1)
>>> +                frame_len -= 2;
>> I'd like to have this inside the core. I mean, get_rx_frame_len returns
>> the len - 2 in case of type 1.
> 
> Thats fine. Will do that

Thanks so much

peppe

> 
> Regards
> Deepak
>