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

From: [email-masked] (Murali Karicheri)
Date: Thu, 11 Dec 2014 15:21:27 -0500

On 12/11/2014 01:34 PM, Joe Perches wrote:
> On Thu, 2014-12-11 at 12:17 -0500, Murali Karicheri wrote:
>> On 12/11/2014 12:01 PM, David Miller wrote:
>>> From: Murali Karicheri<[email-masked]>
>>> 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.
>>
>> 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
>
> (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