[PATCH 0/3] cpsw interrupt pacing and get/set phy setting implementation

[PATCH 0/3] cpsw interrupt pacing and get/set phy setting implementation

[Mugunthan V N]

This patch serires implements the following features in CPSW driver
* get/set phy link settings
* interrupt pacing

Mugunthan V N (3):
  driver: net: ethernet: cpsw: implement ethtool get/set phy setting
  arm: dts: am33xx: add default ethtool slave to cpsw node
  driver: net: ethernet: cpsw: implement interrupt pacing via ethtool

 Documentation/devicetree/bindings/net/cpsw.txt |    3 +
 arch/arm/boot/dts/am33xx.dtsi                  |    1 +
 drivers/net/ethernet/ti/cpsw.c                 |  127 ++++++++++++++++++++++++
 include/linux/platform_data/cpsw.h             |    1 +
 4 files changed, 132 insertions(+)

-- 
1.7.9.5

[PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Mugunthan V N]

This patch implements get/set of the phy settings via ethtool apis

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 Documentation/devicetree/bindings/net/cpsw.txt |    3 +++
 drivers/net/ethernet/ti/cpsw.c                 |   32 ++++++++++++++++++++++++
 include/linux/platform_data/cpsw.h             |    1 +
 3 files changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index ecfdf75..8d61300 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -20,6 +20,7 @@ Required properties:
 - cpts_clock_shift	: Denominator to convert input clock ticks into nanoseconds
 - phy_id		: Specifies slave phy id
 - mac-address		: Specifies slave MAC address
+- ethtool-active-slave	: Specifies the slave to use for ethtool command
 
 Optional properties:
 - ti,hwmods		: Must be "cpgmac0"
@@ -50,6 +51,7 @@ Examples:
 		cpts_active_slave = <0>;
 		cpts_clock_mult = <0x80000000>;
 		cpts_clock_shift = <29>;
+		ethtool-active-slave = <0>;
 		cpsw_emac0: slave@0 {
 			phy_id = <&davinci_mdio>, <0>;
 			/* Filled in by U-Boot */
@@ -76,6 +78,7 @@ Examples:
 		cpts_active_slave = <0>;
 		cpts_clock_mult = <0x80000000>;
 		cpts_clock_shift = <29>;
+		ethtool-active-slave = <0>;
 		cpsw_emac0: slave@0 {
 			phy_id = <&davinci_mdio>, <0>;
 			/* Filled in by U-Boot */
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 01ffbc4..fa91eec 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1244,12 +1244,41 @@ static int cpsw_get_ts_info(struct net_device *ndev,
 	return 0;
 }
 
+#define cpsw_slave_phy_index(priv)			\
+	((priv->data.dual_emac) ? priv->emac_port :	\
+	priv->data.ethtool_active_slave)
+
+static int cpsw_get_settings(struct net_device *ndev,
+			     struct ethtool_cmd *ecmd)
+{
+	struct cpsw_priv *priv = netdev_priv(ndev);
+	int slave_no = cpsw_slave_phy_index(priv);
+
+	if (priv->slaves[slave_no].phy)
+		return phy_ethtool_gset(priv->slaves[slave_no].phy, ecmd);
+	else
+		return -EOPNOTSUPP;
+}
+
+static int cpsw_set_settings(struct net_device *ndev, struct ethtool_cmd *ecmd)
+{
+	struct cpsw_priv *priv = netdev_priv(ndev);
+	int slave_no = cpsw_slave_phy_index(priv);
+
+	if (priv->slaves[slave_no].phy)
+		return phy_ethtool_sset(priv->slaves[slave_no].phy, ecmd);
+	else
+		return -EOPNOTSUPP;
+}
+
 static const struct ethtool_ops cpsw_ethtool_ops = {
 	.get_drvinfo	= cpsw_get_drvinfo,
 	.get_msglevel	= cpsw_get_msglevel,
 	.set_msglevel	= cpsw_set_msglevel,
 	.get_link	= ethtool_op_get_link,
 	.get_ts_info	= cpsw_get_ts_info,
+	.get_settings	= cpsw_get_settings,
+	.set_settings	= cpsw_set_settings,
 };
 
 static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
@@ -1346,6 +1375,9 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 	if (!of_property_read_u32(node, "dual_emac", &prop))
 		data->dual_emac = prop;
 
+	if (!of_property_read_u32(node, "ethtool-active-slave", &prop))
+		data->ethtool_active_slave = prop;
+
 	/*
 	 * Populate all the child nodes here...
 	 */
diff --git a/include/linux/platform_data/cpsw.h b/include/linux/platform_data/cpsw.h
index 798fb80..e87e5cb 100644
--- a/include/linux/platform_data/cpsw.h
+++ b/include/linux/platform_data/cpsw.h
@@ -39,6 +39,7 @@ struct cpsw_platform_data {
 	u32	mac_control;	/* Mac control register */
 	u16	default_vlan;	/* Def VLAN for ALE lookup in VLAN aware mode*/
 	bool	dual_emac;	/* Enable Dual EMAC mode */
+	u32	ethtool_active_slave; /* ethtool slave */
 };
 
 #endif /* __CPSW_H__ */
-- 
1.7.9.5

[PATCH 2/3] arm: dts: am33xx: add default ethtool slave to cpsw node

[Mugunthan V N]

Signed-off-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
---
 arch/arm/boot/dts/am33xx.dtsi |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 0957645..f8c83a1 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -352,6 +352,7 @@
 			cpts_active_slave = <0>;
 			cpts_clock_mult = <0x80000000>;
 			cpts_clock_shift = <29>;
+			ethtool-active-slave = <0>;
 			reg = <0x4a100000 0x800
 			       0x4a101200 0x100>;
 			#address-cells = <1>;
-- 
1.7.9.5

[PATCH 3/3] driver: net: ethernet: cpsw: implement interrupt pacing via ethtool

[Mugunthan V N]

This patch implements support for interrupt pacing block of CPSW via ethtool

Signed-off-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
---
 drivers/net/ethernet/ti/cpsw.c |   95 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index fa91eec..da7276d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -126,6 +126,13 @@ do {								\
 #define CPSW_FIFO_DUAL_MAC_MODE		(1 << 15)
 #define CPSW_FIFO_RATE_LIMIT_MODE	(2 << 15)
 
+#define CPSW_INTPACEEN		(0x3f << 16)
+#define CPSW_INTPRESCALE_MASK	(0x7FF << 0)
+#define CPSW_CMINTMAX_CNT	63
+#define CPSW_CMINTMIN_CNT	2
+#define CPSW_CMINTMAX_INTVL	(1000 / CPSW_CMINTMIN_CNT)
+#define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
+
 #define cpsw_enable_irq(priv)	\
 	do {			\
 		u32 i;		\
@@ -160,6 +167,15 @@ struct cpsw_wr_regs {
 	u32	rx_en;
 	u32	tx_en;
 	u32	misc_en;
+	u32	mem_allign1[8];
+	u32	rx_thresh_stat;
+	u32	rx_stat;
+	u32	tx_stat;
+	u32	misc_stat;
+	u32	mem_allign2[8];
+	u32	rx_imax;
+	u32	tx_imax;
+
 };
 
 struct cpsw_ss_regs {
@@ -314,6 +330,8 @@ struct cpsw_priv {
 	struct cpsw_host_regs __iomem	*host_port_regs;
 	u32				msg_enable;
 	u32				version;
+	u32				coal_intvl;
+	u32				bus_freq_mhz;
 	struct net_device_stats		stats;
 	int				rx_packet_max;
 	int				host_port;
@@ -612,6 +630,68 @@ static void cpsw_adjust_link(struct net_device *ndev)
 	}
 }
 
+static int cpsw_get_coalesce(struct net_device *ndev,
+				struct ethtool_coalesce *coal)
+{
+	struct cpsw_priv *priv = netdev_priv(ndev);
+
+	coal->rx_coalesce_usecs = priv->coal_intvl;
+	return 0;
+}
+
+static int cpsw_set_coalesce(struct net_device *ndev,
+				struct ethtool_coalesce *coal)
+{
+	struct cpsw_priv *priv = netdev_priv(ndev);
+	u32 int_ctrl;
+	u32 num_interrupts = 0;
+	u32 prescale = 0;
+	u32 addnl_dvdr = 1;
+	u32 coal_intvl = 0;
+
+	if (!coal->rx_coalesce_usecs)
+		return -EINVAL;
+
+	coal_intvl = coal->rx_coalesce_usecs;
+
+	int_ctrl =  readl(&priv->wr_regs->int_control);
+	prescale = priv->bus_freq_mhz * 4;
+
+	if (coal_intvl < CPSW_CMINTMIN_INTVL)
+		coal_intvl = CPSW_CMINTMIN_INTVL;
+
+	if (coal_intvl > CPSW_CMINTMAX_INTVL) {
+		/* Interrupt pacer works with 4us Pulse, we can
+		 * throttle further by dilating the 4us pulse.
+		 */
+		addnl_dvdr = CPSW_INTPRESCALE_MASK / prescale;
+
+		if (addnl_dvdr > 1) {
+			prescale *= addnl_dvdr;
+			if (coal_intvl > (CPSW_CMINTMAX_INTVL * addnl_dvdr))
+				coal_intvl = (CPSW_CMINTMAX_INTVL
+						* addnl_dvdr);
+		} else {
+			addnl_dvdr = 1;
+			coal_intvl = CPSW_CMINTMAX_INTVL;
+		}
+	}
+
+	num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
+	writel(num_interrupts, &priv->wr_regs->rx_imax);
+	writel(num_interrupts, &priv->wr_regs->tx_imax);
+
+	int_ctrl |= CPSW_INTPACEEN;
+	int_ctrl &= (~CPSW_INTPRESCALE_MASK);
+	int_ctrl |= (prescale & CPSW_INTPRESCALE_MASK);
+	writel(int_ctrl, &priv->wr_regs->int_control);
+
+	cpsw_notice(priv, timer, "Set coalesce to %d usecs.\n", coal_intvl);
+	priv->coal_intvl = coal_intvl;
+
+	return 0;
+}
+
 static inline int __show_stat(char *buf, int maxlen, const char *name, u32 val)
 {
 	static char *leader = "........................................";
@@ -834,6 +914,14 @@ static int cpsw_ndo_open(struct net_device *ndev)
 		cpsw_info(priv, ifup, "submitted %d rx descriptors\n", i);
 	}
 
+	/* Enable Interrupt pacing if configured */
+	if (priv->coal_intvl != 0) {
+		struct ethtool_coalesce coal;
+
+		coal.rx_coalesce_usecs = (priv->coal_intvl << 4);
+		cpsw_set_coalesce(ndev, &coal);
+	}
+
 	cpdma_ctlr_start(priv->dma);
 	cpsw_intr_enable(priv);
 	napi_enable(&priv->napi);
@@ -1279,6 +1367,8 @@ static const struct ethtool_ops cpsw_ethtool_ops = {
 	.get_ts_info	= cpsw_get_ts_info,
 	.get_settings	= cpsw_get_settings,
 	.set_settings	= cpsw_set_settings,
+	.get_coalesce	= cpsw_get_coalesce,
+	.set_coalesce	= cpsw_set_coalesce,
 };
 
 static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
@@ -1469,6 +1559,9 @@ static int cpsw_probe_dual_emac(struct platform_device *pdev,
 	priv_sl2->slaves = priv->slaves;
 	priv_sl2->clk = priv->clk;
 
+	priv_sl2->coal_intvl = 0;
+	priv_sl2->bus_freq_mhz = clk_get_rate(priv_sl2->clk) / 1000000;
+
 	priv_sl2->cpsw_res = priv->cpsw_res;
 	priv_sl2->regs = priv->regs;
 	priv_sl2->host_port = priv->host_port;
@@ -1578,6 +1671,8 @@ static int cpsw_probe(struct platform_device *pdev)
 		ret = -ENODEV;
 		goto clean_slave_ret;
 	}
+	priv->coal_intvl = 0;
+	priv->bus_freq_mhz = clk_get_rate(priv->clk) / 1000000;
 
 	priv->cpsw_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!priv->cpsw_res) {
-- 
1.7.9.5

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Peter Korsgaard]

>>>>> "M" == Mugunthan V N <mugunthanvnm@ti.com> writes:

 M> This patch implements get/set of the phy settings via ethtool apis
 M> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
 M> ---
 M>  Documentation/devicetree/bindings/net/cpsw.txt |    3 +++
 M>  drivers/net/ethernet/ti/cpsw.c                 |   32 ++++++++++++++++++++++++
 M>  include/linux/platform_data/cpsw.h             |    1 +
 M>  3 files changed, 36 insertions(+)

 M> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
 M> index ecfdf75..8d61300 100644
 M> --- a/Documentation/devicetree/bindings/net/cpsw.txt
 M> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
 M> @@ -20,6 +20,7 @@ Required properties:
 M>  - cpts_clock_shift	: Denominator to convert input clock ticks into nanoseconds
 M>  - phy_id		: Specifies slave phy id
 M>  - mac-address		: Specifies slave MAC address
 M> +- ethtool-active-slave	: Specifies the slave to use for ethtool command

That again sounds like something Linux specific rather than a hardware
property.

It would be good if all these special things (dual emac mode, vlan
handling, switching) could be handled using the existing kernel
(bridging/vlan) infrastructure, and the driver always just exposing 2
network interfaces instead of these configuration properties.

-- 
Bye, Peter Korsgaard

Re: [PATCH 2/3] arm: dts: am33xx: add default ethtool slave to cpsw node

[Tony Lindgren]

* Mugunthan V N <mugunthanvnm@ti.com> [130307 04:35]:

Can you please add a description and send this separately
so Benoit can queue it.

Thanks,

Tony

> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  arch/arm/boot/dts/am33xx.dtsi |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 0957645..f8c83a1 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -352,6 +352,7 @@
>  			cpts_active_slave = <0>;
>  			cpts_clock_mult = <0x80000000>;
>  			cpts_clock_shift = <29>;
> +			ethtool-active-slave = <0>;
>  			reg = <0x4a100000 0x800
>  			       0x4a101200 0x100>;
>  			#address-cells = <1>;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Mugunthan V N]

On 3/7/2013 6:54 PM, Peter Korsgaard wrote:
>>>>>> "M" == Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org> writes:
>   M> This patch implements get/set of the phy settings via ethtool apis
>   M> Signed-off-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
>   M> ---
>   M>  Documentation/devicetree/bindings/net/cpsw.txt |    3 +++
>   M>  drivers/net/ethernet/ti/cpsw.c                 |   32 ++++++++++++++++++++++++
>   M>  include/linux/platform_data/cpsw.h             |    1 +
>   M>  3 files changed, 36 insertions(+)
>
>   M> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>   M> index ecfdf75..8d61300 100644
>   M> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>   M> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>   M> @@ -20,6 +20,7 @@ Required properties:
>   M>  - cpts_clock_shift	: Denominator to convert input clock ticks into nanoseconds
>   M>  - phy_id		: Specifies slave phy id
>   M>  - mac-address		: Specifies slave MAC address
>   M> +- ethtool-active-slave	: Specifies the slave to use for ethtool command
>
> That again sounds like something Linux specific rather than a hardware
> property.
>
> It would be good if all these special things (dual emac mode, vlan
> handling, switching) could be handled using the existing kernel
> (bridging/vlan) infrastructure, and the driver always just exposing 2
> network interfaces instead of these configuration properties.
>
Switch and Dual Emac modes of operation of CPSW are two different 
features of the
hardware and packet routing between the slaves in the hardware are 
different in
both the modes.

If by default it is brought up as Dual EMAC then hardware switching is 
blocked and
use-cases like IP phone etc cannot be achieved.

Since CPSW as a hardware Switch, it cannot not be handled in existing kernel
feature.

Regards
Mugunthan V N

Re: [PATCH 2/3] arm: dts: am33xx: add default ethtool slave to cpsw node

[Mugunthan V N]

On 3/7/2013 9:43 PM, Tony Lindgren wrote:
> * Mugunthan V N <mugunthanvnm@ti.com> [130307 04:35]:
>
> Can you please add a description and send this separately
> so Benoit can queue it.
>
>
Will send this patch separately after this patch series has been
reviewed and accepted by netdev maintainer.

Regards
Mugunthan V N

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Peter Korsgaard]

>>>>> "Mugunthan" == Mugunthan V N <mugunthanvnm@ti.com> writes:

Hi,

 M> +- ethtool-active-slave	: Specifies the slave to use for ethtool command
 >> 
 >> That again sounds like something Linux specific rather than a hardware
 >> property.
 >> 
 >> It would be good if all these special things (dual emac mode, vlan
 >> handling, switching) could be handled using the existing kernel
 >> (bridging/vlan) infrastructure, and the driver always just exposing 2
 >> network interfaces instead of these configuration properties.

 Mugunthan> Switch and Dual Emac modes of operation of CPSW are two
 Mugunthan> different features of the hardware and packet routing
 Mugunthan> between the slaves in the hardware are different in both the
 Mugunthan> modes.

 Mugunthan> If by default it is brought up as Dual EMAC then hardware
 Mugunthan> switching is blocked and use-cases like IP phone etc cannot
 Mugunthan> be achieved.

Well, you could use the (sw) bridge functionality of the kernel network
stack, but performance naturally wouldn't be as good.

 Mugunthan> Since CPSW as a hardware Switch, it cannot not be handled in
 Mugunthan> existing kernel feature.

Well, we do have net/dsa, which is conceptually quite similar (even
though it has never been extended to hook into the bridging stuff). I
agree that we don't have infrastructure to handle hw like cpsw in a
really good way today, but it would be very nice to move towards it.

-- 
Bye, Peter Korsgaard

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Ben Hutchings]

On Thu, 2013-03-07 at 14:24 +0100, Peter Korsgaard wrote:
> >>>>> "M" == Mugunthan V N <mugunthanvnm@ti.com> writes:
> 
>  M> This patch implements get/set of the phy settings via ethtool apis
>  M> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>  M> ---
>  M>  Documentation/devicetree/bindings/net/cpsw.txt |    3 +++
>  M>  drivers/net/ethernet/ti/cpsw.c                 |   32 ++++++++++++++++++++++++
>  M>  include/linux/platform_data/cpsw.h             |    1 +
>  M>  3 files changed, 36 insertions(+)
> 
>  M> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>  M> index ecfdf75..8d61300 100644
>  M> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>  M> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>  M> @@ -20,6 +20,7 @@ Required properties:
>  M>  - cpts_clock_shift	: Denominator to convert input clock ticks into nanoseconds
>  M>  - phy_id		: Specifies slave phy id
>  M>  - mac-address		: Specifies slave MAC address
>  M> +- ethtool-active-slave	: Specifies the slave to use for ethtool command
> 
> That again sounds like something Linux specific rather than a hardware
> property.

Yes, indeed.  Isn't it redundant with the phy_id?

Ben.

> It would be good if all these special things (dual emac mode, vlan
> handling, switching) could be handled using the existing kernel
> (bridging/vlan) infrastructure, and the driver always just exposing 2
> network interfaces instead of these configuration properties.
> 

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Mugunthan V N]

On 3/8/2013 1:29 AM, Ben Hutchings wrote:
> On Thu, 2013-03-07 at 14:24 +0100, Peter Korsgaard wrote:
>>>>>>> "M" == Mugunthan V N <mugunthanvnm@ti.com> writes:
>>   M> This patch implements get/set of the phy settings via ethtool apis
>>   M> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>   M> ---
>>   M>  Documentation/devicetree/bindings/net/cpsw.txt |    3 +++
>>   M>  drivers/net/ethernet/ti/cpsw.c                 |   32 ++++++++++++++++++++++++
>>   M>  include/linux/platform_data/cpsw.h             |    1 +
>>   M>  3 files changed, 36 insertions(+)
>>
>>   M> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>>   M> index ecfdf75..8d61300 100644
>>   M> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>   M> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>   M> @@ -20,6 +20,7 @@ Required properties:
>>   M>  - cpts_clock_shift	: Denominator to convert input clock ticks into nanoseconds
>>   M>  - phy_id		: Specifies slave phy id
>>   M>  - mac-address		: Specifies slave MAC address
>>   M> +- ethtool-active-slave	: Specifies the slave to use for ethtool command
>>
>> That again sounds like something Linux specific rather than a hardware
>> property.
> Yes, indeed.  Isn't it redundant with the phy_id?
>
> Ben.
phy_id is part of slave data and will be present for both the slaves.
so phy_id cannot be used for get/set phy setting until phy framework
allows to change settings without going through eth interface.

Regards
Mugunthan V N

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Ben Hutchings]

On Fri, 2013-03-08 at 12:53 +0530, Mugunthan V N wrote:
> On 3/8/2013 1:29 AM, Ben Hutchings wrote:
> > On Thu, 2013-03-07 at 14:24 +0100, Peter Korsgaard wrote:
> >>>>>>> "M" == Mugunthan V N <mugunthanvnm@ti.com> writes:
> >>   M> This patch implements get/set of the phy settings via ethtool apis
> >>   M> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> >>   M> ---
> >>   M>  Documentation/devicetree/bindings/net/cpsw.txt |    3 +++
> >>   M>  drivers/net/ethernet/ti/cpsw.c                 |   32 ++++++++++++++++++++++++
> >>   M>  include/linux/platform_data/cpsw.h             |    1 +
> >>   M>  3 files changed, 36 insertions(+)
> >>
> >>   M> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> >>   M> index ecfdf75..8d61300 100644
> >>   M> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> >>   M> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> >>   M> @@ -20,6 +20,7 @@ Required properties:
> >>   M>  - cpts_clock_shift	: Denominator to convert input clock ticks into nanoseconds
> >>   M>  - phy_id		: Specifies slave phy id
> >>   M>  - mac-address		: Specifies slave MAC address
> >>   M> +- ethtool-active-slave	: Specifies the slave to use for ethtool command
> >>
> >> That again sounds like something Linux specific rather than a hardware
> >> property.
> > Yes, indeed.  Isn't it redundant with the phy_id?
> >
> > Ben.
> phy_id is part of slave data and will be present for both the slaves.
> so phy_id cannot be used for get/set phy setting until phy framework
> allows to change settings without going through eth interface.

Now I've looked at the examples in this file, I think I see what you're
getting at.  What confused me is that you're adding to a single list of
properties without a proper distinction of which devices they are
applied to.  It really ought to be properly divided between switch and
'slave' properties.

The 'active slave' property would also be needed for the SIOCGMIIPHY
ioctl and not just ethtool.  But it's really quite arbitrary.  Perhaps
each of them should have their own net device (as with DSA).

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Peter Korsgaard]

>>>>> "Ben" == Ben Hutchings <bhutchings@solarflare.com> writes:

Hi,

 Ben> The 'active slave' property would also be needed for the SIOCGMIIPHY
 Ben> ioctl and not just ethtool.  But it's really quite arbitrary.  Perhaps
 Ben> each of them should have their own net device (as with DSA).

Indeed, I think that would simplify all of this quite a bit.

-- 
Bye, Peter Korsgaard

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Mugunthan V N]

On 3/8/2013 8:23 PM, Ben Hutchings wrote:
> On Fri, 2013-03-08 at 12:53 +0530, Mugunthan V N wrote:
>> On 3/8/2013 1:29 AM, Ben Hutchings wrote:
>>> On Thu, 2013-03-07 at 14:24 +0100, Peter Korsgaard wrote:
>>>>>>>>> "M" == Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org> writes:
>>>>    M> This patch implements get/set of the phy settings via ethtool apis
>>>>    M> Signed-off-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
>>>>    M> ---
>>>>    M>  Documentation/devicetree/bindings/net/cpsw.txt |    3 +++
>>>>    M>  drivers/net/ethernet/ti/cpsw.c                 |   32 ++++++++++++++++++++++++
>>>>    M>  include/linux/platform_data/cpsw.h             |    1 +
>>>>    M>  3 files changed, 36 insertions(+)
>>>>
>>>>    M> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>>>>    M> index ecfdf75..8d61300 100644
>>>>    M> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>>>    M> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>>>    M> @@ -20,6 +20,7 @@ Required properties:
>>>>    M>  - cpts_clock_shift	: Denominator to convert input clock ticks into nanoseconds
>>>>    M>  - phy_id		: Specifies slave phy id
>>>>    M>  - mac-address		: Specifies slave MAC address
>>>>    M> +- ethtool-active-slave	: Specifies the slave to use for ethtool command
>>>>
>>>> That again sounds like something Linux specific rather than a hardware
>>>> property.
>>> Yes, indeed.  Isn't it redundant with the phy_id?
>>>
>>> Ben.
>> phy_id is part of slave data and will be present for both the slaves.
>> so phy_id cannot be used for get/set phy setting until phy framework
>> allows to change settings without going through eth interface.
> Now I've looked at the examples in this file, I think I see what you're
> getting at.  What confused me is that you're adding to a single list of
> properties without a proper distinction of which devices they are
> applied to.  It really ought to be properly divided between switch and
> 'slave' properties.
Will fix this in next patch series.
> The 'active slave' property would also be needed for the SIOCGMIIPHY
> ioctl and not just ethtool.  But it's really quite arbitrary.  Perhaps
> each of them should have their own net device (as with DSA).
But if we have net device for each of the slaves then it is dual EMAC 
which will kill
hardware switching functionality. To achieve switching bridge has to be 
done and
there will be a performance drop as well.

As Peter Korsgaard mentioned we need to have infrastructure to handle CPSW
kind of hardware.

Regards
Mugunthan V N

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Mugunthan V N]

On 3/8/2013 8:34 PM, Peter Korsgaard wrote:
>>>>>> "Ben" == Ben Hutchings <bhutchings-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org> writes:
> Hi,
>
>   Ben> The 'active slave' property would also be needed for the SIOCGMIIPHY
>   Ben> ioctl and not just ethtool.  But it's really quite arbitrary.  Perhaps
>   Ben> each of them should have their own net device (as with DSA).
>
> Indeed, I think that would simplify all of this quite a bit.
>
I will update this in the next patch series

Regards
Mugunthan V N

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Richard Cochran]

On Fri, Mar 08, 2013 at 08:37:00PM +0530, Mugunthan V N wrote:
> 
> As Peter Korsgaard mentioned we need to have infrastructure to handle CPSW
> kind of hardware.

This will be a big job, I think, but I agree that it is worth doing.

I can think of one other switch-with-host-port device. Maybe we should
start off by making a list of actual products and their capabilities,
in order to get an overall idea of what we would need to develop.

Thanks,
Richard

Re: [PATCH 1/3] driver: net: ethernet: cpsw: implement ethtool get/set phy setting

[Peter Korsgaard]

>>>>> "Richard" == Richard Cochran <richardcochran@gmail.com> writes:

Hi,

 >> As Peter Korsgaard mentioned we need to have infrastructure to
 >> handle CPSW kind of hardware.

 Richard> This will be a big job, I think, but I agree that it is worth doing.

 Richard> I can think of one other switch-with-host-port device. Maybe
 Richard> we should start off by making a list of actual products and
 Richard> their capabilities, in order to get an overall idea of what we
 Richard> would need to develop.

Next to the dsa stuff with external switches, I can think of 3 other
devices off the top of my head:

Freescale imx287:
http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=i.MX287

Freescale T1022:
http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=T1020

Ralink RT3502:
http://www.netcheif.com/Reviews/VigorFly200/PDF/RT3052-product%20brief.pdf

-- 
Bye, Peter Korsgaard