* [PATCH 0/6] stmmac: Driver Updates @ 2012-03-02 12:55 Deepak Sikri 2012-03-02 12:55 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Deepak Sikri 2012-03-05 15:31 ` [PATCH 0/6] stmmac: Driver Updates Giuseppe CAVALLARO 0 siblings, 2 replies; 32+ messages in thread From: Deepak Sikri @ 2012-03-02 12:55 UTC (permalink / raw) To: peppe.cavallaro; +Cc: spear-devel, netdev, 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(-) ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/6] stmmac: Define CSUM offload engine Types 2012-03-02 12:55 [PATCH 0/6] stmmac: Driver Updates Deepak Sikri @ 2012-03-02 12:55 ` Deepak Sikri 2012-03-02 12:55 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Deepak Sikri 2012-03-05 14:13 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Giuseppe CAVALLARO 2012-03-05 15:31 ` [PATCH 0/6] stmmac: Driver Updates Giuseppe CAVALLARO 1 sibling, 2 replies; 32+ messages in thread From: Deepak Sikri @ 2012-03-02 12:55 UTC (permalink / raw) To: peppe.cavallaro; +Cc: spear-devel, netdev, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/6] stmmac: Define MDC clock selection macros. 2012-03-02 12:55 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Deepak Sikri @ 2012-03-02 12:55 ` Deepak Sikri 2012-03-02 12:55 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers Deepak Sikri 2012-03-05 14:34 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Giuseppe CAVALLARO 2012-03-05 14:13 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Giuseppe CAVALLARO 1 sibling, 2 replies; 32+ messages in thread From: Deepak Sikri @ 2012-03-02 12:55 UTC (permalink / raw) To: peppe.cavallaro; +Cc: spear-devel, netdev, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/6] stmmac: Add support for CPU freq notifiers. 2012-03-02 12:55 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Deepak Sikri @ 2012-03-02 12:55 ` Deepak Sikri 2012-03-02 12:55 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 Deepak Sikri ` (2 more replies) 2012-03-05 14:34 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Giuseppe CAVALLARO 1 sibling, 3 replies; 32+ messages in thread From: Deepak Sikri @ 2012-03-02 12:55 UTC (permalink / raw) To: peppe.cavallaro; +Cc: spear-devel, netdev, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5. 2012-03-02 12:55 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers Deepak Sikri @ 2012-03-02 12:55 ` Deepak Sikri 2012-03-02 12:55 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters Deepak Sikri ` (2 more replies) 2012-03-05 1:50 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers David Miller 2012-03-05 15:05 ` Giuseppe CAVALLARO 2 siblings, 3 replies; 32+ messages in thread From: Deepak Sikri @ 2012-03-02 12:55 UTC (permalink / raw) To: peppe.cavallaro; +Cc: spear-devel, netdev, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters 2012-03-02 12:55 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 Deepak Sikri @ 2012-03-02 12:55 ` Deepak Sikri 2012-03-02 12:55 ` [PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w Deepak Sikri ` (2 more replies) 2012-03-05 1:51 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 David Miller 2012-03-06 7:10 ` Giuseppe CAVALLARO 2 siblings, 3 replies; 32+ messages in thread From: Deepak Sikri @ 2012-03-02 12:55 UTC (permalink / raw) To: peppe.cavallaro Cc: spear-devel, netdev, Deepak Sikri, Shiraz Hashim, Vikas Manocha 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w 2012-03-02 12:55 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters Deepak Sikri @ 2012-03-02 12:55 ` Deepak Sikri 2012-03-06 7:55 ` Giuseppe CAVALLARO 2012-03-05 1:52 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters David Miller 2012-03-06 7:43 ` Giuseppe CAVALLARO 2 siblings, 1 reply; 32+ messages in thread From: Deepak Sikri @ 2012-03-02 12:55 UTC (permalink / raw) To: peppe.cavallaro; +Cc: spear-devel, netdev, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w 2012-03-02 12:55 ` [PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w Deepak Sikri @ 2012-03-06 7:55 ` Giuseppe CAVALLARO 0 siblings, 0 replies; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-06 7:55 UTC (permalink / raw) To: Deepak Sikri; +Cc: spear-devel, netdev 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); > } > > /** ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters 2012-03-02 12:55 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters Deepak Sikri 2012-03-02 12:55 ` [PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w Deepak Sikri @ 2012-03-05 1:52 ` David Miller 2012-03-07 5:39 ` deepaksi 2012-03-06 7:43 ` Giuseppe CAVALLARO 2 siblings, 1 reply; 32+ messages in thread From: David Miller @ 2012-03-05 1:52 UTC (permalink / raw) To: deepak.sikri Cc: peppe.cavallaro, spear-devel, netdev, shiraz.hashim, vikas.manocha 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters 2012-03-05 1:52 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters David Miller @ 2012-03-07 5:39 ` deepaksi 0 siblings, 0 replies; 32+ messages in thread From: deepaksi @ 2012-03-07 5:39 UTC (permalink / raw) To: David Miller Cc: Peppe CAVALLARO, spear-devel, netdev, Shiraz HASHIM, Vikas MANOCHA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters 2012-03-02 12:55 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters Deepak Sikri 2012-03-02 12:55 ` [PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w Deepak Sikri 2012-03-05 1:52 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters David Miller @ 2012-03-06 7:43 ` Giuseppe CAVALLARO 2012-03-07 6:18 ` deepaksi 2 siblings, 1 reply; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-06 7:43 UTC (permalink / raw) To: Deepak Sikri; +Cc: spear-devel, netdev, Shiraz Hashim, Vikas Manocha 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 */ ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters 2012-03-06 7:43 ` Giuseppe CAVALLARO @ 2012-03-07 6:18 ` deepaksi 0 siblings, 0 replies; 32+ messages in thread From: deepaksi @ 2012-03-07 6:18 UTC (permalink / raw) To: Giuseppe CAVALLARO; +Cc: spear-devel, netdev, Shiraz HASHIM, Vikas MANOCHA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5. 2012-03-02 12:55 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 Deepak Sikri 2012-03-02 12:55 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters Deepak Sikri @ 2012-03-05 1:51 ` David Miller 2012-03-05 4:01 ` Shiraz Hashim 2012-03-07 8:26 ` deepaksi 2012-03-06 7:10 ` Giuseppe CAVALLARO 2 siblings, 2 replies; 32+ messages in thread From: David Miller @ 2012-03-05 1:51 UTC (permalink / raw) To: deepak.sikri; +Cc: peppe.cavallaro, spear-devel, netdev 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. */ ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5. 2012-03-05 1:51 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 David Miller @ 2012-03-05 4:01 ` Shiraz Hashim 2012-03-05 4:59 ` David Miller 2012-03-07 8:26 ` deepaksi 1 sibling, 1 reply; 32+ messages in thread From: Shiraz Hashim @ 2012-03-05 4:01 UTC (permalink / raw) To: David Miller; +Cc: Deepak SIKRI, Peppe CAVALLARO, spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5. 2012-03-05 4:01 ` Shiraz Hashim @ 2012-03-05 4:59 ` David Miller 0 siblings, 0 replies; 32+ messages in thread From: David Miller @ 2012-03-05 4:59 UTC (permalink / raw) To: shiraz.hashim; +Cc: deepak.sikri, peppe.cavallaro, spear-devel, netdev 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5. 2012-03-05 1:51 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 David Miller 2012-03-05 4:01 ` Shiraz Hashim @ 2012-03-07 8:26 ` deepaksi 1 sibling, 0 replies; 32+ messages in thread From: deepaksi @ 2012-03-07 8:26 UTC (permalink / raw) To: David Miller; +Cc: Peppe CAVALLARO, spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5. 2012-03-02 12:55 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 Deepak Sikri 2012-03-02 12:55 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters Deepak Sikri 2012-03-05 1:51 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 David Miller @ 2012-03-06 7:10 ` Giuseppe CAVALLARO 2012-03-07 8:25 ` deepaksi 2 siblings, 1 reply; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-06 7:10 UTC (permalink / raw) To: Deepak Sikri; +Cc: spear-devel, netdev 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)) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5. 2012-03-06 7:10 ` Giuseppe CAVALLARO @ 2012-03-07 8:25 ` deepaksi 2012-03-07 8:45 ` Giuseppe CAVALLARO 0 siblings, 1 reply; 32+ messages in thread From: deepaksi @ 2012-03-07 8:25 UTC (permalink / raw) To: Giuseppe CAVALLARO; +Cc: spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5. 2012-03-07 8:25 ` deepaksi @ 2012-03-07 8:45 ` Giuseppe CAVALLARO 0 siblings, 0 replies; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-07 8:45 UTC (permalink / raw) To: Deepak SIKRI; +Cc: spear-devel, netdev 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 > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers. 2012-03-02 12:55 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers Deepak Sikri 2012-03-02 12:55 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 Deepak Sikri @ 2012-03-05 1:50 ` David Miller 2012-03-07 7:18 ` deepaksi 2012-03-05 15:05 ` Giuseppe CAVALLARO 2 siblings, 1 reply; 32+ messages in thread From: David Miller @ 2012-03-05 1:50 UTC (permalink / raw) To: deepak.sikri; +Cc: peppe.cavallaro, spear-devel, netdev 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers. 2012-03-05 1:50 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers David Miller @ 2012-03-07 7:18 ` deepaksi 0 siblings, 0 replies; 32+ messages in thread From: deepaksi @ 2012-03-07 7:18 UTC (permalink / raw) To: David Miller; +Cc: Peppe CAVALLARO, spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers. 2012-03-02 12:55 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers Deepak Sikri 2012-03-02 12:55 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 Deepak Sikri 2012-03-05 1:50 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers David Miller @ 2012-03-05 15:05 ` Giuseppe CAVALLARO 2012-03-06 8:04 ` Giuseppe CAVALLARO 2012-03-07 7:17 ` deepaksi 2 siblings, 2 replies; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-05 15:05 UTC (permalink / raw) To: Deepak Sikri; +Cc: spear-devel, netdev 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); ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers. 2012-03-05 15:05 ` Giuseppe CAVALLARO @ 2012-03-06 8:04 ` Giuseppe CAVALLARO 2012-03-07 8:28 ` deepaksi 2012-03-07 7:17 ` deepaksi 1 sibling, 1 reply; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-06 8:04 UTC (permalink / raw) To: Deepak Sikri; +Cc: spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers. 2012-03-06 8:04 ` Giuseppe CAVALLARO @ 2012-03-07 8:28 ` deepaksi 0 siblings, 0 replies; 32+ messages in thread From: deepaksi @ 2012-03-07 8:28 UTC (permalink / raw) To: Giuseppe CAVALLARO; +Cc: spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] stmmac: Add support for CPU freq notifiers. 2012-03-05 15:05 ` Giuseppe CAVALLARO 2012-03-06 8:04 ` Giuseppe CAVALLARO @ 2012-03-07 7:17 ` deepaksi 1 sibling, 0 replies; 32+ messages in thread From: deepaksi @ 2012-03-07 7:17 UTC (permalink / raw) To: Giuseppe CAVALLARO; +Cc: spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] stmmac: Define MDC clock selection macros. 2012-03-02 12:55 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Deepak Sikri 2012-03-02 12:55 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers Deepak Sikri @ 2012-03-05 14:34 ` Giuseppe CAVALLARO 2012-03-07 6:55 ` deepaksi 1 sibling, 1 reply; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-05 14:34 UTC (permalink / raw) To: Deepak SIKRI; +Cc: spear-devel, netdev 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 { ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] stmmac: Define MDC clock selection macros. 2012-03-05 14:34 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Giuseppe CAVALLARO @ 2012-03-07 6:55 ` deepaksi 2012-03-07 7:19 ` Giuseppe CAVALLARO 0 siblings, 1 reply; 32+ messages in thread From: deepaksi @ 2012-03-07 6:55 UTC (permalink / raw) To: Giuseppe CAVALLARO; +Cc: spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] stmmac: Define MDC clock selection macros. 2012-03-07 6:55 ` deepaksi @ 2012-03-07 7:19 ` Giuseppe CAVALLARO 2012-03-07 8:30 ` deepaksi 0 siblings, 1 reply; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-07 7:19 UTC (permalink / raw) To: Deepak SIKRI; +Cc: spear-devel, netdev 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 > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] stmmac: Define MDC clock selection macros. 2012-03-07 7:19 ` Giuseppe CAVALLARO @ 2012-03-07 8:30 ` deepaksi 0 siblings, 0 replies; 32+ messages in thread From: deepaksi @ 2012-03-07 8:30 UTC (permalink / raw) To: Giuseppe CAVALLARO; +Cc: spear-devel, netdev 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 >> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/6] stmmac: Define CSUM offload engine Types 2012-03-02 12:55 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Deepak Sikri 2012-03-02 12:55 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Deepak Sikri @ 2012-03-05 14:13 ` Giuseppe CAVALLARO 2012-03-07 6:50 ` deepaksi 1 sibling, 1 reply; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-05 14:13 UTC (permalink / raw) To: Deepak SIKRI; +Cc: spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/6] stmmac: Define CSUM offload engine Types 2012-03-05 14:13 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Giuseppe CAVALLARO @ 2012-03-07 6:50 ` deepaksi 0 siblings, 0 replies; 32+ messages in thread From: deepaksi @ 2012-03-07 6:50 UTC (permalink / raw) To: Giuseppe CAVALLARO; +Cc: spear-devel, netdev 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/6] stmmac: Driver Updates 2012-03-02 12:55 [PATCH 0/6] stmmac: Driver Updates Deepak Sikri 2012-03-02 12:55 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Deepak Sikri @ 2012-03-05 15:31 ` Giuseppe CAVALLARO 1 sibling, 0 replies; 32+ messages in thread From: Giuseppe CAVALLARO @ 2012-03-05 15:31 UTC (permalink / raw) To: Deepak SIKRI; +Cc: spear-devel, netdev 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(-) > > ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2012-03-07 8:47 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-03-02 12:55 [PATCH 0/6] stmmac: Driver Updates Deepak Sikri 2012-03-02 12:55 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Deepak Sikri 2012-03-02 12:55 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Deepak Sikri 2012-03-02 12:55 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers Deepak Sikri 2012-03-02 12:55 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 Deepak Sikri 2012-03-02 12:55 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters Deepak Sikri 2012-03-02 12:55 ` [PATCH 6/6] stmmac: Replace infinite loops by timeouts in mdio r/w Deepak Sikri 2012-03-06 7:55 ` Giuseppe CAVALLARO 2012-03-05 1:52 ` [PATCH 5/6] stmmac: configure burst related GMAC DMA parameters David Miller 2012-03-07 5:39 ` deepaksi 2012-03-06 7:43 ` Giuseppe CAVALLARO 2012-03-07 6:18 ` deepaksi 2012-03-05 1:51 ` [PATCH 4/6] stmmac: Update stmmac descriptor checks for stmmac core prior to Rev-3.5 David Miller 2012-03-05 4:01 ` Shiraz Hashim 2012-03-05 4:59 ` David Miller 2012-03-07 8:26 ` deepaksi 2012-03-06 7:10 ` Giuseppe CAVALLARO 2012-03-07 8:25 ` deepaksi 2012-03-07 8:45 ` Giuseppe CAVALLARO 2012-03-05 1:50 ` [PATCH 3/6] stmmac: Add support for CPU freq notifiers David Miller 2012-03-07 7:18 ` deepaksi 2012-03-05 15:05 ` Giuseppe CAVALLARO 2012-03-06 8:04 ` Giuseppe CAVALLARO 2012-03-07 8:28 ` deepaksi 2012-03-07 7:17 ` deepaksi 2012-03-05 14:34 ` [PATCH 2/6] stmmac: Define MDC clock selection macros Giuseppe CAVALLARO 2012-03-07 6:55 ` deepaksi 2012-03-07 7:19 ` Giuseppe CAVALLARO 2012-03-07 8:30 ` deepaksi 2012-03-05 14:13 ` [PATCH 1/6] stmmac: Define CSUM offload engine Types Giuseppe CAVALLARO 2012-03-07 6:50 ` deepaksi 2012-03-05 15:31 ` [PATCH 0/6] stmmac: Driver Updates Giuseppe CAVALLARO
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).