Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

10 messages, 4 authors, 2014-12-11 · open the first message on its own page

Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

From: David Miller <davem@davemloft.net>
Date: 2014-12-09 18:10:58

From: Murali Karicheri <redacted>
Date: Tue, 2 Dec 2014 16:41:42 -0500

quoted hunk

+static void set_pkt_info_le(u32 buff, u32 buff_len, u32 ndesc,
+			    struct knav_dma_desc *desc)
+{
+	desc->buff_len = cpu_to_le32(buff_len);
+	desc->buff = cpu_to_le32(buff);
+	desc->next_desc = cpu_to_le32(ndesc);
+}

The members of knav_dma_desc are "u32", so you are going to get tons of
static checker warnings from trying to assign cpu_to_le32()'s result
(which is a le32) into them.

Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

From: Murali Karicheri <hidden>
Date: 2014-12-10 21:31:33

On 12/09/2014 01:10 PM, David Miller wrote:

From: Murali Karicheri<redacted>
Date: Tue, 2 Dec 2014 16:41:42 -0500

quoted

+static void set_pkt_info_le(u32 buff, u32 buff_len, u32 ndesc,
+			    struct knav_dma_desc *desc)
+{
+	desc->buff_len = cpu_to_le32(buff_len);
+	desc->buff = cpu_to_le32(buff);
+	desc->next_desc = cpu_to_le32(ndesc);
+}

The members of knav_dma_desc are "u32", so you are going to get tons of
static checker warnings from trying to assign cpu_to_le32()'s result
(which is a le32) into them.

David,

Thanks for your comment.

Are you referring to the static code analyser sparse that is invoked 
through?

make C=1 CHECK=sparse

I am running it now.

I started a build of v3.18-rc7 with this option and seeing a lot of
warnings. Am I using the right tool? A snapshot of the output is below

   CHECK   drivers/clk/clk.c
include/linux/err.h:40:16: warning: testing a 'safe expression'
include/linux/err.h:40:16: warning: testing a 'safe expression'
include/linux/err.h:40:16: warning: testing a 'safe expression'
include/linux/err.h:40:16: warning: testing a 'safe expression'
drivers/clk/clk.c:2263:5: warning: symbol '__clk_get' was not declared. 
Should it be static?
drivers/clk/clk.c:2274:6: warning: symbol '__clk_put' was not declared. 
Should it be static?
drivers/clk/clk.c:2647:37: warning: incorrect type in assignment 
(different modifiers)
drivers/clk/clk.c:2647:37:    expected void ( *[usertype] clk_init_cb )( 
... )
drivers/clk/clk.c:2647:37:    got void const *const data
drivers/clk/clk.c:67:22: warning: context imbalance in 'clk_enable_lock' 
- wrong count at exit
drivers/clk/clk.c:92:9: warning: context imbalance in 
'clk_enable_unlock' - unexpected unlock
   CC      drivers/clk/clk.o
   CHECK   drivers/clk/clk-divider.c
drivers/clk/clk-divider.c:362:9: warning: context imbalance in 
'clk_divider_set_rate' - different lock contexts for basic block
   CC      drivers/clk/clk-divider.o
   CHECK   drivers/clk/clk-fixed-factor.c
   CC      drivers/clk/clk-fixed-factor.o
   CHECK   drivers/clk/clk-fixed-rate.c
   CC      drivers/clk/clk-fixed-rate.o
   CHECK   drivers/clk/clk-gate.c
drivers/clk/clk-gate.c:71:9: warning: context imbalance in 
'clk_gate_endisable' - different lock contexts for basic block
   CC      drivers/clk/clk-gate.o
   CHECK   drivers/clk/clk-mux.c
drivers/clk/clk-mux.c:69:12: warning: context imbalance in 
'clk_mux_set_parent' - different lock contexts for basic block
   CC      drivers/clk/clk-mux.o
   CHECK   drivers/clk/clk-composite.c
   CC      drivers/clk/clk-composite.o
   CHECK   drivers/clk/clk-fractional-divider.c
drivers/clk/clk-fractional-divider.c:36:9: warning: context imbalance in 
'clk_fd_recalc_rate' - different lock contexts for basic block
drivers/clk/clk-fractional-divider.c:65:12: warning: context imbalance 
in 'clk_fd_set_rate' - different lock contexts for basic block
   CC      drivers/clk/clk-fractional-divider.o
   CHECK   drivers/clk/clk-gpio-gate.c
drivers/clk/clk-gpio-gate.c:189:13: warning: symbol 
'of_gpio_gate_clk_setup' was not declared. Should it be static?
   CC      drivers/clk/clk-gpio-gate.o
   CHECK   drivers/clk/clk-conf.c
   CC      drivers/clk/clk-conf.o
   CHECK   drivers/clk/keystone/pll.c
   CC      drivers/clk/keystone/pll.o
   CHECK   drivers/clk/keystone/gate.c
drivers/clk/keystone/gate.c:118:12: warning: context imbalance in 
'keystone_clk_enable' - different lock contexts for basic block
drivers/clk/keystone/gate.c:148:9: warning: context imbalance in 
'keystone_clk_disable' - different lock contexts for basic block
   CC      drivers/clk/keystone/gate.o
   LD      drivers/clk/keystone/built-in.o
   LD      drivers/clk/built-in.o
   CHECK   drivers/clocksource/clksrc-of.c
drivers/clocksource/clksrc-of.c:37:27: warning: incorrect type in 
assignment (different modifiers)
drivers/clocksource/clksrc-of.c:37:27:    expected void ( *[usertype] 
init_func )( ... )
drivers/clocksource/clksrc-of.c:37:27:    got void const *const data
   CC      drivers/clocksource/clksrc-of.o
   CHECK   drivers/clocksource/mmio.c
   CC      drivers/clocksource/mmio.o
   CHECK   drivers/clocksource/arm_arch_timer.c
   CC      drivers/clocksource/arm_arch_timer.o
   CHECK   drivers/clocksource/dummy_timer.c
   CC      drivers/clocksource/dummy_timer.o
   CHECK   drivers/clocksource/timer-keystone.c
   CC      drivers/clocksource/timer-keystone.o
   LD      drivers/clocksource/built-in.o
   LD      drivers/crypto/built-in.o
   CHECK   drivers/dma/dmaengine.c
   CC      drivers/dma/dmaengine.o
   CHECK   drivers/dma/virt-dma.c
drivers/dma/virt-dma.c:22:14: warning: symbol 'vchan_tx_submit' was not 
declared. Should it be static?
   CC      drivers/dma/virt-dma.o
   CHECK   drivers/dma/of-dma.c
   CC      drivers/dma/of-dma.o
   CHECK   drivers/dma/edma.c
drivers/dma/edma.c:560:32: warning: symbol 'edma_prep_dma_memcpy' was 
not declared. Should it be static?
drivers/dma/edma.c:1099:6: warning: symbol 'edma_filter_fn' was not 
declared. Should it be static?





-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

From: David Miller <davem@davemloft.net>
Date: 2014-12-11 01:41:17

From: Murali Karicheri <redacted>
Date: Wed, 10 Dec 2014 16:31:02 -0500

Are you referring to the static code analyser sparse that is invoked
through?

You have to explicitly enable endian checking, it's not on by
default.

default enable sparse __CHECK_ENDIAN__ (was: Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver)

From: Joe Perches <joe@perches.com>
Date: 2014-12-11 03:05:06

On Wed, 2014-12-10 at 20:41 -0500, David Miller wrote:

From: Murali Karicheri <redacted>

quoted

Are you referring to the static code analyser sparse that is invoked
through?

You have to explicitly enable endian checking, it's not on by
default.

There don't seem to be thousands of warnings anymore.

Maybe it's time to default enable it when using C=?

from: Documentation/sparse.txt:

The optional make variable CF can be used to pass arguments to sparse.  The
build system passes -Wbitwise to sparse automatically.  To perform endianness
checks, you may define __CHECK_ENDIAN__:

        make C=2 CF="-D__CHECK_ENDIAN__"

These checks are disabled by default as they generate a host of warnings.

Re: default enable sparse __CHECK_ENDIAN__ (was: Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver)

From: Marcel Holtmann <marcel@holtmann.org>
Date: 2014-12-11 11:18:52

Hi Joe,

quoted

Are you referring to the static code analyser sparse that is invoked
through?

You have to explicitly enable endian checking, it's not on by
default.

There don't seem to be thousands of warnings anymore.

Maybe it's time to default enable it when using C=?

from: Documentation/sparse.txt:

The optional make variable CF can be used to pass arguments to sparse.  The
build system passes -Wbitwise to sparse automatically.  To perform endianness
checks, you may define __CHECK_ENDIAN__:

       make C=2 CF="-D__CHECK_ENDIAN__"

These checks are disabled by default as they generate a host of warnings.

actually a few subsystems use this in their Makefile:

	subdir-ccflags-y += -D__CHECK_ENDIAN__

We could start with that to enable endian checks by default in various places.

Regards

Marcel

Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

From: Murali Karicheri <hidden>
Date: 2014-12-11 14:15:08

On 12/10/2014 08:41 PM, David Miller wrote:

From: Murali Karicheri<redacted>
Date: Wed, 10 Dec 2014 16:31:02 -0500

quoted

Are you referring to the static code analyser sparse that is invoked
through?

You have to explicitly enable endian checking, it's not on by
default.

Thanks David and others who responded. Let me do this and resolve any 
warning before the next post.

BTW, could you provide any suggestions that would help us merge this 
series to upstream? This has been sitting on this list for a while now.

Thanks and regards,

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

From: David Miller <davem@davemloft.net>
Date: 2014-12-11 17:01:11

From: Murali Karicheri <redacted>
Date: Thu, 11 Dec 2014 09:14:37 -0500

BTW, could you provide any suggestions that would help us merge this
series to upstream? This has been sitting on this list for a while
now.

You simply have to continue going through the review and revision
process until people no longer find problems with your changes.

This could take several more weeks, you simply must be patient.

Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

From: Murali Karicheri <hidden>
Date: 2014-12-11 17:17:39

On 12/11/2014 12:01 PM, David Miller wrote:

From: Murali Karicheri<redacted>
Date: Thu, 11 Dec 2014 09:14:37 -0500

quoted

BTW, could you provide any suggestions that would help us merge this
series to upstream? This has been sitting on this list for a while
now.

You simply have to continue going through the review and revision
process until people no longer find problems with your changes.

This could take several more weeks, you simply must be patient.

Ok. Thanks. That is encouraging to hear!

Regards,
-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

From: Joe Perches <joe@perches.com>
Date: 2014-12-11 18:35:08

On Thu, 2014-12-11 at 12:17 -0500, Murali Karicheri wrote:

On 12/11/2014 12:01 PM, David Miller wrote:

quoted

From: Murali Karicheri<redacted>
Date: Thu, 11 Dec 2014 09:14:37 -0500

quoted

BTW, could you provide any suggestions that would help us merge this
series to upstream? This has been sitting on this list for a while
now.

You simply have to continue going through the review and revision
process until people no longer find problems with your changes.

This could take several more weeks, you simply must be patient.

Ok. Thanks. That is encouraging to hear!

Perhaps these trivial things might be considered

(uncompiled)
---
 drivers/net/ethernet/ti/netcp_core.c     |  2 +-
 drivers/net/ethernet/ti/netcp_ethss.c    | 83 +++++++++++++++++---------------
 drivers/net/ethernet/ti/netcp_xgbepcsr.c | 48 +++++++++---------
 3 files changed, 71 insertions(+), 62 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 60ad299..8f38fe8 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c

@@ -1094,7 +1094,7 @@ static void netcp_tx_notify(void *arg)
 	napi_schedule(&netcp->tx_napi);
 }
 
-static struct knav_dma_desc*
+static struct knav_dma_desc *
 netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
 {
 	struct knav_dma_desc *desc, *ndesc, *pdesc;

diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c
index 036b886..3757957 100644
--- a/drivers/net/ethernet/ti/netcp_ethss.c
+++ b/drivers/net/ethernet/ti/netcp_ethss.c

@@ -486,21 +486,29 @@ struct netcp_ethtool_stat {
 	int offset;
 };
 
-#define GBE_STATSA_INFO(field)		"GBE_A:"#field, GBE_STATSA_MODULE,\
-				FIELD_SIZEOF(struct gbe_hw_stats, field), \
-				offsetof(struct gbe_hw_stats, field)
-
-#define GBE_STATSB_INFO(field)		"GBE_B:"#field, GBE_STATSB_MODULE,\
-				FIELD_SIZEOF(struct gbe_hw_stats, field), \
-				offsetof(struct gbe_hw_stats, field)
-
-#define GBE_STATSC_INFO(field)		"GBE_C:"#field, GBE_STATSC_MODULE,\
-				FIELD_SIZEOF(struct gbe_hw_stats, field), \
-				offsetof(struct gbe_hw_stats, field)
-
-#define GBE_STATSD_INFO(field)		"GBE_D:"#field, GBE_STATSD_MODULE,\
-				FIELD_SIZEOF(struct gbe_hw_stats, field), \
-				offsetof(struct gbe_hw_stats, field)
+#define GBE_STATSA_INFO(field)						\
+	.desc = "GBE_A:"#field,						\
+	.type = GBE_STATSA_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
+
+#define GBE_STATSB_INFO(field)						\
+	.desc = "GBE_B:"#field,						\
+	.type = GBE_STATSB_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
+
+#define GBE_STATSC_INFO(field)						\
+	.desc = "GBE_C:"#field,						\
+	.type = GBE_STATSC_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
+
+#define GBE_STATSD_INFO(field)						\
+	.desc = "GBE_D:"#field,						\
+	.type = GBE_STATSD_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
 
 static const struct netcp_ethtool_stat gbe13_et_stats[] = {
 	/* GBE module A */

@@ -645,17 +653,23 @@ static const struct netcp_ethtool_stat gbe13_et_stats[] = {
 	{GBE_STATSD_INFO(rx_dma_overruns)},
 };
 
-#define XGBE_STATS0_INFO(field)	"GBE_0:"#field, XGBE_STATS0_MODULE, \
-				FIELD_SIZEOF(struct xgbe_hw_stats, field), \
-				offsetof(struct xgbe_hw_stats, field)
+#define XGBE_STATS0_INFO(field)						\
+	.desc = "GBE_0:"#field,						\
+	.type = GBE_STATS0_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
 
-#define XGBE_STATS1_INFO(field)	"GBE_1:"#field, XGBE_STATS1_MODULE, \
-				FIELD_SIZEOF(struct xgbe_hw_stats, field), \
-				offsetof(struct xgbe_hw_stats, field)
+#define XGBE_STATS1_INFO(field)						\
+	.desc = "GBE_1:"#field,						\
+	.type = GBE_STATS1_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
 
-#define XGBE_STATS2_INFO(field)	"GBE_2:"#field, XGBE_STATS2_MODULE, \
-				FIELD_SIZEOF(struct xgbe_hw_stats, field), \
-				offsetof(struct xgbe_hw_stats, field)
+#define XGBE_STATS2_INFO(field)
+	.desc = "GBE_2:"#field,						\
+	.type = GBE_STATS2_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
 
 static const struct netcp_ethtool_stat xgbe10_et_stats[] = {
 	/* GBE module 0 */

@@ -883,11 +897,11 @@ static void gbe_update_stats_ver14(struct gbe_priv *gbe_dev, uint64_t *data)
 			case GBE_STATSA_MODULE:
 			case GBE_STATSC_MODULE:
 				base = gbe_statsa;
-			break;
+				break;
 			case GBE_STATSB_MODULE:
 			case GBE_STATSD_MODULE:
 				base  = gbe_statsb;
-			break;
+				break;
 			}
 
 			p = base + gbe_dev->et_stats[j].offset;

@@ -1639,11 +1653,8 @@ static void init_secondary_ports(struct gbe_priv *gbe_dev,
 
 	for_each_child_of_node(node, port) {
 		slave = devm_kzalloc(dev, sizeof(*slave), GFP_KERNEL);
-		if (!slave) {
-			dev_err(dev, "memomry alloc failed for secondary port(%s), skipping...\n",
-				port->name);
+		if (!slave)
 			continue;
-		}
 
 		if (init_slave(gbe_dev, slave, port)) {
 			dev_err(dev, "Failed to initialize secondary port(%s), skipping...\n",

@@ -1763,10 +1774,8 @@ static int set_xgbe_ethss10_priv(struct gbe_priv *gbe_dev,
 					  XGBE10_NUM_STAT_ENTRIES *
 					  XGBE10_NUM_SLAVES * sizeof(u64),
 					  GFP_KERNEL);
-	if (!gbe_dev->hw_stats) {
-		dev_err(gbe_dev->dev, "hw_stats memory allocation failed\n");
+	if (!gbe_dev->hw_stats)
 		return -ENOMEM;
-	}
 
 	gbe_dev->ss_version = XGBE_SS_VERSION_10;
 	gbe_dev->sgmii_port_regs = gbe_dev->ss_regs +

@@ -1836,10 +1845,8 @@ static int set_gbe_ethss14_priv(struct gbe_priv *gbe_dev,
 					  GBE13_NUM_HW_STAT_ENTRIES *
 					  GBE13_NUM_SLAVES * sizeof(u64),
 					  GFP_KERNEL);
-	if (!gbe_dev->hw_stats) {
-		dev_err(gbe_dev->dev, "hw_stats memory allocation failed\n");
+	if (!gbe_dev->hw_stats)
 		return -ENOMEM;
-	}
 
 	gbe_dev->ss_version = GBE_SS_VERSION_14;
 	gbe_dev->sgmii_port_regs = regs + GBE13_SGMII_MODULE_OFFSET;

@@ -1995,10 +2002,10 @@ static int gbe_probe(struct netcp_device *netcp_device, struct device *dev,
 		dev_err(gbe_dev->dev, "error initializing ale engine\n");
 		ret = -ENODEV;
 		goto quit;
-	} else {
-		dev_dbg(gbe_dev->dev, "Created a gbe ale engine\n");
 	}
 
+	dev_dbg(gbe_dev->dev, "Created a gbe ale engine\n");
+
 	/* initialize host port */
 	gbe_init_host_port(gbe_dev);

diff --git a/drivers/net/ethernet/ti/netcp_xgbepcsr.c b/drivers/net/ethernet/ti/netcp_xgbepcsr.c
index 33571ac..d93a6a4 100644
--- a/drivers/net/ethernet/ti/netcp_xgbepcsr.c
+++ b/drivers/net/ethernet/ti/netcp_xgbepcsr.c

@@ -14,6 +14,9 @@
  * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  */
+
+#define pr_fmt(fmt) "XGBE: " fmt
+
 #include "netcp.h"
 
 /* XGBE registers */

@@ -43,7 +46,7 @@ struct serdes_cfg {
 	u32 mask;
 };
 
-static struct serdes_cfg cfg_phyb_1p25g_156p25mhz_cmu0[] = {
+static const struct serdes_cfg cfg_phyb_1p25g_156p25mhz_cmu0[] = {
 	{0x0000, 0x00800002, 0x00ff00ff},
 	{0x0014, 0x00003838, 0x0000ffff},
 	{0x0060, 0x1c44e438, 0xffffffff},

@@ -54,7 +57,7 @@ static struct serdes_cfg cfg_phyb_1p25g_156p25mhz_cmu0[] = {
 	{0x0000, 0x00000003, 0x000000ff},
 };
 
-static struct serdes_cfg cfg_phyb_10p3125g_156p25mhz_cmu1[] = {
+static const struct serdes_cfg cfg_phyb_10p3125g_156p25mhz_cmu1[] = {
 	{0x0c00, 0x00030002, 0x00ff00ff},
 	{0x0c14, 0x00005252, 0x0000ffff},
 	{0x0c28, 0x80000000, 0xff000000},

@@ -76,7 +79,7 @@ static struct serdes_cfg cfg_phyb_10p3125g_156p25mhz_cmu1[] = {
 	{0x0c00, 0x00000003, 0x000000ff},
 };
 
-static struct serdes_cfg cfg_phyb_10p3125g_16bit_lane[] = {
+static const struct serdes_cfg cfg_phyb_10p3125g_16bit_lane[] = {
 	{0x0204, 0x00000080, 0x000000ff},
 	{0x0208, 0x0000920d, 0x0000ffff},
 	{0x0204, 0xfc000000, 0xff000000},

@@ -106,7 +109,7 @@ static struct serdes_cfg cfg_phyb_10p3125g_16bit_lane[] = {
 	{0x03cc, 0x00000000, 0x000000ff},
 };
 
-static struct serdes_cfg cfg_phyb_10p3125g_comlane[] = {
+static const struct serdes_cfg cfg_phyb_10p3125g_comlane[] = {
 	{0x0a00, 0x00000800, 0x0000ff00},
 	{0x0a84, 0x00000000, 0x000000ff},
 	{0x0a8c, 0x00130000, 0x00ff0000},

@@ -124,7 +127,7 @@ static struct serdes_cfg cfg_phyb_10p3125g_comlane[] = {
 	{0x0ac0, 0x0000008b, 0x000000ff},
 };
 
-static struct serdes_cfg cfg_cm_c1_c2[] = {
+static const struct serdes_cfg cfg_cm_c1_c2[] = {
 	{0x0208, 0x00000000, 0x00000f00},
 	{0x0208, 0x00000000, 0x0000001f},
 	{0x0204, 0x00000000, 0x00040000},

@@ -185,8 +188,8 @@ static void netcp_xgbe_serdes_com_enable(void __iomem *serdes_regs)
 	}
 }
 
-static void netcp_xgbe_serdes_lane_enable(
-			void __iomem *serdes_regs, int lane)
+static void netcp_xgbe_serdes_lane_enable(void __iomem *serdes_regs,
+					  int lane)
 {
 	/* Set Lane Control Rate */
 	writel(0xe0e9e038, serdes_regs + 0x1fe0 + (4 * lane));

@@ -230,7 +233,7 @@ static int netcp_xgbe_wait_pll_locked(void __iomem *sw_regs)
 		cpu_relax();
 	} while (true);
 
-	pr_err("XGBE serdes not locked: time out.\n");
+	pr_err("serdes not locked: time out\n");
 	return ret;
 }

@@ -292,8 +295,7 @@ static void netcp_xgbe_serdes_reset_cdr(void __iomem *serdes_regs,
 	u32 tmp, dlpf, tbus;
 
 	/*Get the DLPF values */
-	tmp = netcp_xgbe_serdes_read_select_tbus(
-			serdes_regs, lane + 1, 5);
+	tmp = netcp_xgbe_serdes_read_select_tbus(serdes_regs, lane + 1, 5);
 
 	dlpf = tmp >> 2;

@@ -302,10 +304,10 @@ static void netcp_xgbe_serdes_reset_cdr(void __iomem *serdes_regs,
 		mdelay(1);
 		reg_rmw(sig_detect_reg, VAL_SH(0, 1), MASK_WID_SH(2, 1));
 	} else {
-		tbus = netcp_xgbe_serdes_read_select_tbus(serdes_regs, lane +
-							  1, 0xe);
+		tbus = netcp_xgbe_serdes_read_select_tbus(serdes_regs,
+							  lane + 1, 0xe);
 
-		pr_debug("XGBE: CDR centered, DLPF: %4d,%d,%d.\n",
+		pr_debug("CDR centered, DLPF: %4d,%d,%d\n",
 			 tmp >> 2, tmp & 3, (tbus >> 2) & 3);
 	}
 }

@@ -340,13 +342,13 @@ static int netcp_xgbe_check_link_status(void __iomem *serdes_regs,
 		case 0:
 			/* if good link lock the signal detect ON! */
 			if (!loss && blk_lock) {
-				pr_debug("XGBE PCSR Linked Lane: %d\n", i);
+				pr_debug("PCSR Linked Lane: %d\n", i);
 				reg_rmw(sig_detect_reg, VAL_SH(3, 1),
 					MASK_WID_SH(2, 1));
 				current_state[i] = 1;
 			} else if (!blk_lock) {
 				/* if no lock, then reset CDR */
-				pr_debug("XGBE PCSR Recover Lane: %d\n", i);
+				pr_debug("PCSR Recover Lane: %d\n", i);
 				netcp_xgbe_serdes_reset_cdr(serdes_regs,
 							    sig_detect_reg, i);
 			}

@@ -361,10 +363,10 @@ static int netcp_xgbe_check_link_status(void __iomem *serdes_regs,
 			break;
 
 		case 2:
-			if (blk_lock)
+			if (blk_lock) {
 				/* Nope just noise */
 				current_state[i] = 1;
-			else {
+			} else {
 				/* Lost the block lock, reset CDR if it is
 				 * not centered and go back to sync state
 				 */

@@ -375,7 +377,7 @@ static int netcp_xgbe_check_link_status(void __iomem *serdes_regs,
 			break;
 
 		default:
-			pr_err("XGBE: unknown current_state[%d] %d\n",
+			pr_err("unknown current_state[%d] %d\n",
 			       i, current_state[i]);
 			break;
 		}

@@ -417,19 +419,19 @@ static int netcp_xgbe_serdes_check_lane(void __iomem *serdes_regs,
 			break;
 
 		if (lane_down[0])
-			pr_debug("XGBE: detected link down on lane 0\n");
+			pr_debug("detected link down on lane 0\n");
 
 		if (lane_down[1])
-			pr_debug("XGBE: detected link down on lane 1\n");
+			pr_debug("detected link down on lane 1\n");
 
 		if (++retries > 1) {
-			pr_debug("XGBE: timeout waiting for serdes link up\n");
+			pr_debug("timeout waiting for serdes link up\n");
 			return -ETIMEDOUT;
 		}
 		mdelay(100);
 	} while (!link_up);
 
-	pr_debug("XGBE: PCSR link is up\n");
+	pr_debug("PCSR link is up\n");
 	return 0;
 }

@@ -494,7 +496,7 @@ int netcp_xgbe_serdes_init(void __iomem *serdes_regs, void __iomem *xgbe_regs)
 	/* read COMLANE bits 4:0 */
 	val = readl(serdes_regs + 0xa00);
 	if (val & 0x1f) {
-		pr_debug("XGBE: serdes already in operation - reset\n");
+		pr_debug("serdes already in operation - reset\n");
 		netcp_xgbe_reset_serdes(serdes_regs);
 	}
 	return netcp_xgbe_serdes_config(serdes_regs, xgbe_regs);

Re: [PATCH v7 2/3] net: Add Keystone NetCP ethernet driver

From: Murali Karicheri <hidden>
Date: 2014-12-11 20:22:00

On 12/11/2014 01:34 PM, Joe Perches wrote:

On Thu, 2014-12-11 at 12:17 -0500, Murali Karicheri wrote:

quoted

On 12/11/2014 12:01 PM, David Miller wrote:

quoted

From: Murali Karicheri<redacted>
Date: Thu, 11 Dec 2014 09:14:37 -0500

quoted

BTW, could you provide any suggestions that would help us merge this
series to upstream? This has been sitting on this list for a while
now.

You simply have to continue going through the review and revision
process until people no longer find problems with your changes.

This could take several more weeks, you simply must be patient.

Ok. Thanks. That is encouraging to hear!

Perhaps these trivial things might be considered

Joe, David,

I will address the comments and repost a new version.

Murali

quoted hunk

(uncompiled)
---
  drivers/net/ethernet/ti/netcp_core.c     |  2 +-
  drivers/net/ethernet/ti/netcp_ethss.c    | 83 +++++++++++++++++---------------
  drivers/net/ethernet/ti/netcp_xgbepcsr.c | 48 +++++++++---------
  3 files changed, 71 insertions(+), 62 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 60ad299..8f38fe8 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c

@@ -1094,7 +1094,7 @@ static void netcp_tx_notify(void *arg)
  	napi_schedule(&netcp->tx_napi);
  }

-static struct knav_dma_desc*
+static struct knav_dma_desc *
  netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
  {
  	struct knav_dma_desc *desc, *ndesc, *pdesc;

diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c
index 036b886..3757957 100644
--- a/drivers/net/ethernet/ti/netcp_ethss.c
+++ b/drivers/net/ethernet/ti/netcp_ethss.c

@@ -486,21 +486,29 @@ struct netcp_ethtool_stat {
  	int offset;
  };

-#define GBE_STATSA_INFO(field)		"GBE_A:"#field, GBE_STATSA_MODULE,\
-				FIELD_SIZEOF(struct gbe_hw_stats, field), \
-				offsetof(struct gbe_hw_stats, field)
-
-#define GBE_STATSB_INFO(field)		"GBE_B:"#field, GBE_STATSB_MODULE,\
-				FIELD_SIZEOF(struct gbe_hw_stats, field), \
-				offsetof(struct gbe_hw_stats, field)
-
-#define GBE_STATSC_INFO(field)		"GBE_C:"#field, GBE_STATSC_MODULE,\
-				FIELD_SIZEOF(struct gbe_hw_stats, field), \
-				offsetof(struct gbe_hw_stats, field)
-
-#define GBE_STATSD_INFO(field)		"GBE_D:"#field, GBE_STATSD_MODULE,\
-				FIELD_SIZEOF(struct gbe_hw_stats, field), \
-				offsetof(struct gbe_hw_stats, field)
+#define GBE_STATSA_INFO(field)						\
+	.desc = "GBE_A:"#field,						\
+	.type = GBE_STATSA_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
+
+#define GBE_STATSB_INFO(field)						\
+	.desc = "GBE_B:"#field,						\
+	.type = GBE_STATSB_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
+
+#define GBE_STATSC_INFO(field)						\
+	.desc = "GBE_C:"#field,						\
+	.type = GBE_STATSC_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)
+
+#define GBE_STATSD_INFO(field)						\
+	.desc = "GBE_D:"#field,						\
+	.type = GBE_STATSD_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)

  static const struct netcp_ethtool_stat gbe13_et_stats[] = {
  	/* GBE module A */

@@ -645,17 +653,23 @@ static const struct netcp_ethtool_stat gbe13_et_stats[] = {
  	{GBE_STATSD_INFO(rx_dma_overruns)},
  };

-#define XGBE_STATS0_INFO(field)	"GBE_0:"#field, XGBE_STATS0_MODULE, \
-				FIELD_SIZEOF(struct xgbe_hw_stats, field), \
-				offsetof(struct xgbe_hw_stats, field)
+#define XGBE_STATS0_INFO(field)						\
+	.desc = "GBE_0:"#field,						\
+	.type = GBE_STATS0_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)

-#define XGBE_STATS1_INFO(field)	"GBE_1:"#field, XGBE_STATS1_MODULE, \
-				FIELD_SIZEOF(struct xgbe_hw_stats, field), \
-				offsetof(struct xgbe_hw_stats, field)
+#define XGBE_STATS1_INFO(field)						\
+	.desc = "GBE_1:"#field,						\
+	.type = GBE_STATS1_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)

-#define XGBE_STATS2_INFO(field)	"GBE_2:"#field, XGBE_STATS2_MODULE, \
-				FIELD_SIZEOF(struct xgbe_hw_stats, field), \
-				offsetof(struct xgbe_hw_stats, field)
+#define XGBE_STATS2_INFO(field)
+	.desc = "GBE_2:"#field,						\
+	.type = GBE_STATS2_MODULE,					\
+	.size = FIELD_SIZEOF(struct gbe_hw_stats, field),		\
+	.offset = offsetof(struct gbe_hw_stats, field)

  static const struct netcp_ethtool_stat xgbe10_et_stats[] = {
  	/* GBE module 0 */

@@ -883,11 +897,11 @@ static void gbe_update_stats_ver14(struct gbe_priv *gbe_dev, uint64_t *data)
  			case GBE_STATSA_MODULE:
  			case GBE_STATSC_MODULE:
  				base = gbe_statsa;
-			break;
+				break;
  			case GBE_STATSB_MODULE:
  			case GBE_STATSD_MODULE:
  				base  = gbe_statsb;
-			break;
+				break;
  			}

  			p = base + gbe_dev->et_stats[j].offset;

@@ -1639,11 +1653,8 @@ static void init_secondary_ports(struct gbe_priv *gbe_dev,

  	for_each_child_of_node(node, port) {
  		slave = devm_kzalloc(dev, sizeof(*slave), GFP_KERNEL);
-		if (!slave) {
-			dev_err(dev, "memomry alloc failed for secondary port(%s), skipping...\n",
-				port->name);
+		if (!slave)
  			continue;
-		}

  		if (init_slave(gbe_dev, slave, port)) {
  			dev_err(dev, "Failed to initialize secondary port(%s), skipping...\n",

@@ -1763,10 +1774,8 @@ static int set_xgbe_ethss10_priv(struct gbe_priv *gbe_dev,
  					  XGBE10_NUM_STAT_ENTRIES *
  					  XGBE10_NUM_SLAVES * sizeof(u64),
  					  GFP_KERNEL);
-	if (!gbe_dev->hw_stats) {
-		dev_err(gbe_dev->dev, "hw_stats memory allocation failed\n");
+	if (!gbe_dev->hw_stats)
  		return -ENOMEM;
-	}

  	gbe_dev->ss_version = XGBE_SS_VERSION_10;
  	gbe_dev->sgmii_port_regs = gbe_dev->ss_regs +

@@ -1836,10 +1845,8 @@ static int set_gbe_ethss14_priv(struct gbe_priv *gbe_dev,
  					  GBE13_NUM_HW_STAT_ENTRIES *
  					  GBE13_NUM_SLAVES * sizeof(u64),
  					  GFP_KERNEL);
-	if (!gbe_dev->hw_stats) {
-		dev_err(gbe_dev->dev, "hw_stats memory allocation failed\n");
+	if (!gbe_dev->hw_stats)
  		return -ENOMEM;
-	}

  	gbe_dev->ss_version = GBE_SS_VERSION_14;
  	gbe_dev->sgmii_port_regs = regs + GBE13_SGMII_MODULE_OFFSET;

@@ -1995,10 +2002,10 @@ static int gbe_probe(struct netcp_device *netcp_device, struct device *dev,
  		dev_err(gbe_dev->dev, "error initializing ale engine\n");
  		ret = -ENODEV;
  		goto quit;
-	} else {
-		dev_dbg(gbe_dev->dev, "Created a gbe ale engine\n");
  	}

+	dev_dbg(gbe_dev->dev, "Created a gbe ale engine\n");
+
  	/* initialize host port */
  	gbe_init_host_port(gbe_dev);

diff --git a/drivers/net/ethernet/ti/netcp_xgbepcsr.c b/drivers/net/ethernet/ti/netcp_xgbepcsr.c
index 33571ac..d93a6a4 100644
--- a/drivers/net/ethernet/ti/netcp_xgbepcsr.c
+++ b/drivers/net/ethernet/ti/netcp_xgbepcsr.c

@@ -14,6 +14,9 @@
   * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   * GNU General Public License for more details.
   */
+
+#define pr_fmt(fmt) "XGBE: " fmt
+
  #include "netcp.h"

  /* XGBE registers */

@@ -43,7 +46,7 @@ struct serdes_cfg {
  	u32 mask;
  };

-static struct serdes_cfg cfg_phyb_1p25g_156p25mhz_cmu0[] = {
+static const struct serdes_cfg cfg_phyb_1p25g_156p25mhz_cmu0[] = {
  	{0x0000, 0x00800002, 0x00ff00ff},
  	{0x0014, 0x00003838, 0x0000ffff},
  	{0x0060, 0x1c44e438, 0xffffffff},

@@ -54,7 +57,7 @@ static struct serdes_cfg cfg_phyb_1p25g_156p25mhz_cmu0[] = {
  	{0x0000, 0x00000003, 0x000000ff},
  };

-static struct serdes_cfg cfg_phyb_10p3125g_156p25mhz_cmu1[] = {
+static const struct serdes_cfg cfg_phyb_10p3125g_156p25mhz_cmu1[] = {
  	{0x0c00, 0x00030002, 0x00ff00ff},
  	{0x0c14, 0x00005252, 0x0000ffff},
  	{0x0c28, 0x80000000, 0xff000000},

@@ -76,7 +79,7 @@ static struct serdes_cfg cfg_phyb_10p3125g_156p25mhz_cmu1[] = {
  	{0x0c00, 0x00000003, 0x000000ff},
  };

-static struct serdes_cfg cfg_phyb_10p3125g_16bit_lane[] = {
+static const struct serdes_cfg cfg_phyb_10p3125g_16bit_lane[] = {
  	{0x0204, 0x00000080, 0x000000ff},
  	{0x0208, 0x0000920d, 0x0000ffff},
  	{0x0204, 0xfc000000, 0xff000000},

@@ -106,7 +109,7 @@ static struct serdes_cfg cfg_phyb_10p3125g_16bit_lane[] = {
  	{0x03cc, 0x00000000, 0x000000ff},
  };

-static struct serdes_cfg cfg_phyb_10p3125g_comlane[] = {
+static const struct serdes_cfg cfg_phyb_10p3125g_comlane[] = {
  	{0x0a00, 0x00000800, 0x0000ff00},
  	{0x0a84, 0x00000000, 0x000000ff},
  	{0x0a8c, 0x00130000, 0x00ff0000},

@@ -124,7 +127,7 @@ static struct serdes_cfg cfg_phyb_10p3125g_comlane[] = {
  	{0x0ac0, 0x0000008b, 0x000000ff},
  };

-static struct serdes_cfg cfg_cm_c1_c2[] = {
+static const struct serdes_cfg cfg_cm_c1_c2[] = {
  	{0x0208, 0x00000000, 0x00000f00},
  	{0x0208, 0x00000000, 0x0000001f},
  	{0x0204, 0x00000000, 0x00040000},

@@ -185,8 +188,8 @@ static void netcp_xgbe_serdes_com_enable(void __iomem *serdes_regs)
  	}
  }

-static void netcp_xgbe_serdes_lane_enable(
-			void __iomem *serdes_regs, int lane)
+static void netcp_xgbe_serdes_lane_enable(void __iomem *serdes_regs,
+					  int lane)
  {
  	/* Set Lane Control Rate */
  	writel(0xe0e9e038, serdes_regs + 0x1fe0 + (4 * lane));

@@ -230,7 +233,7 @@ static int netcp_xgbe_wait_pll_locked(void __iomem *sw_regs)
  		cpu_relax();
  	} while (true);

-	pr_err("XGBE serdes not locked: time out.\n");
+	pr_err("serdes not locked: time out\n");
  	return ret;
  }

@@ -292,8 +295,7 @@ static void netcp_xgbe_serdes_reset_cdr(void __iomem *serdes_regs,
  	u32 tmp, dlpf, tbus;

  	/*Get the DLPF values */
-	tmp = netcp_xgbe_serdes_read_select_tbus(
-			serdes_regs, lane + 1, 5);
+	tmp = netcp_xgbe_serdes_read_select_tbus(serdes_regs, lane + 1, 5);

  	dlpf = tmp>>  2;

@@ -302,10 +304,10 @@ static void netcp_xgbe_serdes_reset_cdr(void __iomem *serdes_regs,
  		mdelay(1);
  		reg_rmw(sig_detect_reg, VAL_SH(0, 1), MASK_WID_SH(2, 1));
  	} else {
-		tbus = netcp_xgbe_serdes_read_select_tbus(serdes_regs, lane +
-							  1, 0xe);
+		tbus = netcp_xgbe_serdes_read_select_tbus(serdes_regs,
+							  lane + 1, 0xe);

-		pr_debug("XGBE: CDR centered, DLPF: %4d,%d,%d.\n",
+		pr_debug("CDR centered, DLPF: %4d,%d,%d\n",
  			 tmp>>  2, tmp&  3, (tbus>>  2)&  3);
  	}
  }

@@ -340,13 +342,13 @@ static int netcp_xgbe_check_link_status(void __iomem *serdes_regs,
  		case 0:
  			/* if good link lock the signal detect ON! */
  			if (!loss&&  blk_lock) {
-				pr_debug("XGBE PCSR Linked Lane: %d\n", i);
+				pr_debug("PCSR Linked Lane: %d\n", i);
  				reg_rmw(sig_detect_reg, VAL_SH(3, 1),
  					MASK_WID_SH(2, 1));
  				current_state[i] = 1;
  			} else if (!blk_lock) {
  				/* if no lock, then reset CDR */
-				pr_debug("XGBE PCSR Recover Lane: %d\n", i);
+				pr_debug("PCSR Recover Lane: %d\n", i);
  				netcp_xgbe_serdes_reset_cdr(serdes_regs,
  							    sig_detect_reg, i);
  			}

@@ -361,10 +363,10 @@ static int netcp_xgbe_check_link_status(void __iomem *serdes_regs,
  			break;

  		case 2:
-			if (blk_lock)
+			if (blk_lock) {
  				/* Nope just noise */
  				current_state[i] = 1;
-			else {
+			} else {
  				/* Lost the block lock, reset CDR if it is
  				 * not centered and go back to sync state
  				 */

@@ -375,7 +377,7 @@ static int netcp_xgbe_check_link_status(void __iomem *serdes_regs,
  			break;

  		default:
-			pr_err("XGBE: unknown current_state[%d] %d\n",
+			pr_err("unknown current_state[%d] %d\n",
  			       i, current_state[i]);
  			break;
  		}

@@ -417,19 +419,19 @@ static int netcp_xgbe_serdes_check_lane(void __iomem *serdes_regs,
  			break;

  		if (lane_down[0])
-			pr_debug("XGBE: detected link down on lane 0\n");
+			pr_debug("detected link down on lane 0\n");

  		if (lane_down[1])
-			pr_debug("XGBE: detected link down on lane 1\n");
+			pr_debug("detected link down on lane 1\n");

  		if (++retries>  1) {
-			pr_debug("XGBE: timeout waiting for serdes link up\n");
+			pr_debug("timeout waiting for serdes link up\n");
  			return -ETIMEDOUT;
  		}
  		mdelay(100);
  	} while (!link_up);

-	pr_debug("XGBE: PCSR link is up\n");
+	pr_debug("PCSR link is up\n");
  	return 0;
  }

@@ -494,7 +496,7 @@ int netcp_xgbe_serdes_init(void __iomem *serdes_regs, void __iomem *xgbe_regs)
  	/* read COMLANE bits 4:0 */
  	val = readl(serdes_regs + 0xa00);
  	if (val&  0x1f) {
-		pr_debug("XGBE: serdes already in operation - reset\n");
+		pr_debug("serdes already in operation - reset\n");
  		netcp_xgbe_reset_serdes(serdes_regs);
  	}
  	return netcp_xgbe_serdes_config(serdes_regs, xgbe_regs);


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help