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

login
register
mail settings
Submitter Mugunthan V N
Date Oct. 16, 2012, 10:45 p.m.
Message ID <1350427518-7230-2-git-send-email-mugunthanvnm@ti.com>
Download mbox | patch
Permalink /patch/191906/
State Deferred
Delegated to: David Miller
Headers show

Comments

Mugunthan V N - Oct. 16, 2012, 10:45 p.m.
Register offsets for slave register are different between the CPSW IP
found in TI814X and AM335X, so this patch adds support to access slave
registers with CPSW versions

Cc: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c |   86 +++++++++++++++++++++++++++++++---------
 1 files changed, 67 insertions(+), 19 deletions(-)
Richard Cochran - Oct. 18, 2012, 2:45 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mugunthan V N - Oct. 22, 2012, 10:39 a.m.
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, October 18, 2012 8:15 AM
> To: N, Mugunthan V
> Cc: netdev@vger.kernel.org; davem@davemloft.net
> Subject: Re: [PATCH 1/6] drivers: net: ethernet: cpsw: add support for
> CPSW register offset changes in different IP version
> 
> On Wed, Oct 17, 2012 at 04:15:13AM +0530, Mugunthan V N wrote:
> >

> > +
> > +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?

This register is used in next version of CPSW. Since I don't use it in
current version I will remove the same in next version patch series.

> 
> > +	[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.

I have taken the code reference from the following driver.
drivers/i2c/busses/i2c-omap.c

Can you refer other better solution to handle this?

> 
> >  static int debug_level;
> >  module_param(debug_level, int, 0);
> >  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");

>  	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.
> 

Since version info from hardware registers can be used to differentiate between
the CPSW versions so I don't think there is a need to provide the same through DT.

Regards
Mugunthan V N

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Cochran - Oct. 22, 2012, 11:23 a.m.
On Mon, Oct 22, 2012 at 10:39:51AM +0000, N, Mugunthan V wrote:
> > This is wasting memory with unused static stables. There is a better
> > way to handle this issue.
> 
> I have taken the code reference from the following driver.
> drivers/i2c/busses/i2c-omap.c

Can't speak for that driver.

BTW the ALE driver is also horribly wasting space with the "struct
ale_control_info ale_controls[ALE_NUM_CONTROLS]" thing.
 
> Can you refer other better solution to handle this?

Yes, I can think of two different ways. Maybe you can think of yet
other ways.

1. For those few registers that are not aligned the same way but have
   the same bit layout (and you actually use in the driver), keep a
   separate pointer in your driver's private struct.

2. Make two different declarations of structs corresponding to two
   register layouts and use a cast in the access function based on
   version.

I object to the tables of offsets because these take up twice the
memory of the registers themselves, even if you don't use all of the
registers.

> > 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.
> > 
> 
> Since version info from hardware registers can be used to differentiate between
> the CPSW versions so I don't think there is a need to provide the same through DT.

I did not say to put the versions into the DT.

What I meant was that there is no need to add code that tests the
version and acts differently, if there are no users of the special
cases.

Thanks,
Richard



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mugunthan V N - Oct. 22, 2012, 12:19 p.m.
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Monday, October 22, 2012 4:54 PM
> To: N, Mugunthan V
> Cc: netdev@vger.kernel.org; davem@davemloft.net
> Subject: Re: [PATCH 1/6] drivers: net: ethernet: cpsw: add support for
> CPSW register offset changes in different IP version
> 
> On Mon, Oct 22, 2012 at 10:39:51AM +0000, N, Mugunthan V wrote:
> > > This is wasting memory with unused static stables. There is a
> better
> > > way to handle this issue.
> >
> > I have taken the code reference from the following driver.
> > drivers/i2c/busses/i2c-omap.c
> 
> Can't speak for that driver.
> 
> BTW the ALE driver is also horribly wasting space with the "struct
> ale_control_info ale_controls[ALE_NUM_CONTROLS]" thing.
> 
> > Can you refer other better solution to handle this?
> 
> Yes, I can think of two different ways. Maybe you can think of yet
> other ways.
> 
> 1. For those few registers that are not aligned the same way but have
>    the same bit layout (and you actually use in the driver), keep a
>    separate pointer in your driver's private struct.
> 
> 2. Make two different declarations of structs corresponding to two
>    register layouts and use a cast in the access function based on
>    version.
> 
> I object to the tables of offsets because these take up twice the
> memory of the registers themselves, even if you don't use all of the
> registers.
> 
> > > 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.
> > >
> >
> > Since version info from hardware registers can be used to
> differentiate between
> > the CPSW versions so I don't think there is a need to provide the
> same through DT.
> 
> I did not say to put the versions into the DT.
> 
> What I meant was that there is no need to add code that tests the
> version and acts differently, if there are no users of the special
> cases.
> 

Only the slave register offsets are different, so once slave register
offset is taken care in CPSW driver, then we can directly use CPSW
when DM81XX base port patches are available for vanilla kernel.
No need to revisit the driver again when bringing up DM81XX.

Regards
Mugunthan V N
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Cochran - Oct. 22, 2012, 12:25 p.m.
On Mon, Oct 22, 2012 at 12:19:44PM +0000, N, Mugunthan V wrote:
> 
> Only the slave register offsets are different, so once slave register
> offset is taken care in CPSW driver, then we can directly use CPSW
> when DM81XX base port patches are available for vanilla kernel.
> No need to revisit the driver again when bringing up DM81XX.

Fine with me. It is really up to you how and when you want to get the
CPSW working, if ever.

But at this rate, it won't be working in v3.7, not even for am335x,
and v3.8 isn't looking too good either.

Oh well,
Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index df55e24..cdf9ecc 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -70,6 +70,11 @@  do {								\
 		dev_notice(priv->dev, format, ## __VA_ARGS__);	\
 } while (0)
 
+#define CPSW_VERSION_1		0x19010a
+#define CPSW_VERSION_2		0x19010c
+#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,
+	[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,
+};
+
 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;
 
 	dev_info(priv->dev, "initializing cpsw version %d.%d (%d)\n",
 		 CPSW_MAJOR_VERSION(reg), CPSW_MINOR_VERSION(reg),