Re: [PATCH 1/6] drivers: net: ethernet: cpsw: add support for CPSW register offset changes in different IP version

[Richard Cochran <richardcochran@gmail.com>]

On Wed, Oct 17, 2012 at 04:15:13AM +0530, Mugunthan V N wrote:
>  
> +#define CPSW_VERSION_1		0x19010a
> +#define CPSW_VERSION_2		0x19010c

Are you sure about these codes? What about 0x19010b?
I could not find them documented anywhere.

> +#define cpsw_slave_reg(priv, slave, reg)				\
> +	((slave)->regs + (priv)->slave_reg_ofs[(reg)])
> +
>  #define CPSW_MAJOR_VERSION(reg)		(reg >> 8 & 0x7)
>  #define CPSW_MINOR_VERSION(reg)		(reg & 0xff)
>  #define CPSW_RTL_VERSION(reg)		((reg >> 11) & 0x1f)
> @@ -117,6 +122,48 @@ do {								\
>  			disable_irq_nosync(priv->irqs_table[i]); \
>  	} while (0);
>  
> +enum CPSW_SLAVE_REG_OFS {
> +	MAX_BLKS,
> +	BLK_CNT,
> +	FLOW_THRESH,
> +	PORT_VLAN,
> +	TX_PRI_MAP,
> +	TS_CTL,
> +	TS_SEQ_LTYPE,
> +	TS_VLAN,
> +	SA_LO,
> +	SA_HI,
> +	PORT_CONTROL,
> +	TS_SEQ_MTYPE,
> +	TS_CONTROL,
> +};
> +
> +static const u32 slave_reg_map_ip_v1[] = {
> +	[MAX_BLKS]	= 0x00,
> +	[BLK_CNT]	= 0x04,
> +	[FLOW_THRESH]	= 0x08,
> +	[PORT_VLAN]	= 0x0c,
> +	[TX_PRI_MAP]	= 0x10,
> +	[TS_CTL]	= 0x14,
> +	[TS_SEQ_LTYPE]	= 0x18,
> +	[TS_VLAN]	= 0x1c,
> +	[SA_LO]		= 0x20,
> +	[SA_HI]		= 0x24,
> +};
> +
> +static const u32 slave_reg_map_ip_v2[] = {
> +	[PORT_CONTROL]	= 0x00,
> +	[TS_CONTROL]	= 0x04,

You don't make use of this register in your driver, so what is the
point?

> +	[MAX_BLKS]	= 0x08,
> +	[BLK_CNT]	= 0x0c,
> +	[FLOW_THRESH]	= 0x10,
> +	[PORT_VLAN]	= 0x14,
> +	[TX_PRI_MAP]	= 0x18,
> +	[TS_SEQ_MTYPE]	= 0x1c,
> +	[SA_LO]		= 0x20,
> +	[SA_HI]		= 0x24,
> +};
> +

This is wasting memory with unused static stables. There is a better
way to handle this issue.

>  static int debug_level;
>  module_param(debug_level, int, 0);
>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
> @@ -146,19 +193,13 @@ struct cpsw_regs {
>  	u32	soft_reset;
>  	u32	stat_port_en;
>  	u32	ptype;
> -};
> -
> -struct cpsw_slave_regs {
> -	u32	max_blks;
> -	u32	blk_cnt;
> -	u32	flow_thresh;
> -	u32	port_vlan;
> -	u32	tx_pri_map;
> -	u32	ts_ctl;
> -	u32	ts_seq_ltype;
> -	u32	ts_vlan;
> -	u32	sa_lo;
> -	u32	sa_hi;
> +	u32	soft_idle;
> +	u32	thru_rate;
> +	u32	gap_thresh;
> +	u32	tx_start_wds;
> +	u32	flow_control;
> +	u32	vlan_ltype;
> +	u32	ts_ltype;
>  };
>  
>  struct cpsw_host_regs {
> @@ -185,7 +226,7 @@ struct cpsw_sliver_regs {
>  };
>  
>  struct cpsw_slave {
> -	struct cpsw_slave_regs __iomem	*regs;
> +	void __iomem			*regs;
>  	struct cpsw_sliver_regs __iomem	*sliver;
>  	int				slave_num;
>  	u32				mac_control;
> @@ -215,6 +256,8 @@ struct cpsw_priv {
>  	struct cpdma_ctlr		*dma;
>  	struct cpdma_chan		*txch, *rxch;
>  	struct cpsw_ale			*ale;
> +	u32				cpsw_version;
> +	u32				*slave_reg_ofs;
>  	/* snapshot of IRQ numbers */
>  	u32 irqs_table[4];
>  	u32 num_irqs;
> @@ -359,8 +402,8 @@ static inline void soft_reset(const char *module, void __iomem *reg)
>  static void cpsw_set_slave_mac(struct cpsw_slave *slave,
>  			       struct cpsw_priv *priv)
>  {
> -	__raw_writel(mac_hi(priv->mac_addr), &slave->regs->sa_hi);
> -	__raw_writel(mac_lo(priv->mac_addr), &slave->regs->sa_lo);
> +	writel(mac_hi(priv->mac_addr), cpsw_slave_reg(priv, slave, SA_HI));
> +	writel(mac_lo(priv->mac_addr), cpsw_slave_reg(priv, slave, SA_LO));
>  }
>  
>  static void _cpsw_adjust_link(struct cpsw_slave *slave,
> @@ -445,8 +488,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
>  	soft_reset(name, &slave->sliver->soft_reset);
>  
>  	/* setup priority mapping */
> -	__raw_writel(RX_PRIORITY_MAPPING, &slave->sliver->rx_pri_map);
> -	__raw_writel(TX_PRIORITY_MAPPING, &slave->regs->tx_pri_map);
> +	writel(RX_PRIORITY_MAPPING, &slave->sliver->rx_pri_map);
> +	writel(TX_PRIORITY_MAPPING, cpsw_slave_reg(priv, slave, TX_PRI_MAP));
>  
>  	/* setup max packet size, and mac address */
>  	__raw_writel(priv->rx_packet_max, &slave->sliver->rx_maxlen);
> @@ -505,7 +548,12 @@ static int cpsw_ndo_open(struct net_device *ndev)
>  
>  	pm_runtime_get_sync(&priv->pdev->dev);
>  
> -	reg = __raw_readl(&priv->regs->id_ver);
> +	reg = readl(&priv->regs->id_ver);
> +	priv->cpsw_version = reg;
> +	if (reg == CPSW_VERSION_1)
> +		priv->slave_reg_ofs = (u32 *)slave_reg_map_ip_v1;
> +	else
> +		priv->slave_reg_ofs = (u32 *)slave_reg_map_ip_v2;

You didn't provide a way to even use this code, like a dts for a
non-am335x board with the older version.

I think it would be better to start off supporting one version and
have that fully working, and then add the older version, but *really*
add it so that it is actually working.

Thanks,
Richard