| Message ID | 1362659421-11884-2-git-send-email-mugunthanvnm@ti.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
>>>>> "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.
On 3/7/2013 6:54 PM, 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. > > 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 -- 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" == 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.
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. >
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 -- 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
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" == 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.
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@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. 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 -- 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
On 3/8/2013 8:34 PM, Peter Korsgaard wrote: >>>>>> "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. > I will update this in the next patch series 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
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 -- 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" == 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
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__ */
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(+)