| Message ID | 1490609570-3571-1-git-send-email-thomasscariah@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello, On 03/27/2017 03:12 AM, Thomas Scariah wrote: > From: "Scariah, Thomas" <thomas.scariah@harman.com> > > Added functions to support ethtool to print the phy statistics and error > information along with other ethtool statistics. This will help ethtool > information to know the error is from physical layer or MAC layer. > This is an enahancement for ethtool to accommodate phy statistics It sounds like your patch should actually be 3 different patches: - add helper function to the core PHY library - add statistics support to the Micrel PHY driver - hook the proper ndo operations in cpsw to allow querying the PHY driver's statistics > > Signed-off-by: Thomas Scariah <thomasscariah@gmail.com> > --- > drivers/net/ethernet/ti/cpsw.c | 16 ++++++++++++- > drivers/net/phy/micrel.c | 50 ++++++++++++++++++++++++++++++++++++++++ > drivers/net/phy/phy.c | 23 ++++++++++++++++++ > include/linux/phy.h | 14 +++++++++++ > 4 files changed, 102 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > index 3f48bb9..26d2dc5 100644 > --- a/drivers/net/ethernet/ti/cpsw.c > +++ b/drivers/net/ethernet/ti/cpsw.c > @@ -1001,9 +1001,16 @@ update_return: > > static int cpsw_get_sset_count(struct net_device *ndev, int sset) > { > + struct cpsw_priv *priv = netdev_priv(ndev); > + int slave_no = cpsw_slave_index(priv); > + int count; > + > switch (sset) { > case ETH_SS_STATS: > - return CPSW_STATS_LEN; > + count = CPSW_STATS_LEN; > + count += phy_ethtool_get_sset_count(priv->slaves[slave_no].phy, > + sset); > + return count; > default: > return -EOPNOTSUPP; > } We already have a way to obtain PHY specific statistics through the ETH_SS_PHY_STATS string set, cannot we use that here too? It certainly makes it easier to overlay cpsw statistics with PHY device statistics, but when Andrew added support for that, AFAIR this was made intentionally separate. > @@ -1011,6 +1018,8 @@ static int cpsw_get_sset_count(struct net_device *ndev, int sset) > > static void cpsw_get_strings(struct net_device *ndev, u32 stringset, u8 *data) > { > + struct cpsw_priv *priv = netdev_priv(ndev); > + int slave_no = cpsw_slave_index(priv); > u8 *p = data; > int i; > > @@ -1021,6 +1030,8 @@ static void cpsw_get_strings(struct net_device *ndev, u32 stringset, u8 *data) > ETH_GSTRING_LEN); > p += ETH_GSTRING_LEN; > } > + phy_ethtool_get_strings(priv->slaves[slave_no].phy, > + stringset, p); Same here. > break; > } > } > @@ -1031,6 +1042,7 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev, > struct cpsw_priv *priv = netdev_priv(ndev); > struct cpdma_chan_stats rx_stats; > struct cpdma_chan_stats tx_stats; > + int slave_no = cpsw_slave_index(priv); > u32 val; > u8 *p; > int i, ret; > @@ -1067,6 +1079,8 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev, > } > } > > + phy_ethtool_get_stats(priv->slaves[slave_no].phy, stats, > + &data[CPSW_STATS_LEN]); And here. > pm_runtime_put(&priv->pdev->dev); > } >
> > static int cpsw_get_sset_count(struct net_device *ndev, int sset) > > { > > + struct cpsw_priv *priv = netdev_priv(ndev); > > + int slave_no = cpsw_slave_index(priv); > > + int count; > > + > > switch (sset) { > > case ETH_SS_STATS: > > - return CPSW_STATS_LEN; > > + count = CPSW_STATS_LEN; > > + count += phy_ethtool_get_sset_count(priv->slaves[slave_no].phy, > > + sset); > > + return count; > > default: > > return -EOPNOTSUPP; > > } > > We already have a way to obtain PHY specific statistics through the > ETH_SS_PHY_STATS string set, cannot we use that here too? It certainly > makes it easier to overlay cpsw statistics with PHY device statistics, > but when Andrew added support for that, AFAIR this was made > intentionally separate. I don't particularly like this. It makes the cpsw driver different to all other drivers, in that it returns both MAC and PHY statistics. If you are going to do this, please do it a higher level so that it applies to all Ethernet drivers, not just cpsw. But personally, i don't see the point of this change. What is wrong with ethtool --phy-statistics? Andrew
On Mon, Mar 27, 2017 at 09:44:37AM -0700, Florian Fainelli wrote: > Hello, > > On 03/27/2017 03:12 AM, Thomas Scariah wrote: > > From: "Scariah, Thomas" <thomas.scariah@harman.com> > > > > Added functions to support ethtool to print the phy statistics and error > > information along with other ethtool statistics. This will help ethtool > > information to know the error is from physical layer or MAC layer. > > This is an enahancement for ethtool to accommodate phy statistics Hi Thomas What tree is this against? net-next already has get_phy_stats, get_strings, and get_sset_count in struct phy_driver. Andrew
Hello Andrew, > > Hello, > > > > On 03/27/2017 03:12 AM, Thomas Scariah wrote: > > > From: "Scariah, Thomas" <thomas.scariah@harman.com> > > > > > > Added functions to support ethtool to print the phy statistics and > > > error information along with other ethtool statistics. This will > > > help ethtool information to know the error is from physical layer or MAC layer. > > > This is an enahancement for ethtool to accommodate phy statistics > Hi Thomas > > What tree is this against? net-next already has get_phy_stats, get_strings, and get_sset_count in struct >phy_driver. As I have mentioned I have developed against Kernel 4.4.14 and the similar functionality was not available. Regards and Thanks Thomas Scariah -----Original Message----- From: Andrew Lunn [mailto:andrew@lunn.ch] Sent: Monday, March 27, 2017 11:09 PM To: Florian Fainelli <f.fainelli@gmail.com> Cc: Thomas Scariah <thomasscariah@gmail.com>; nsekhar@ti.com; grygorii.strashko@ti.com; davem@davemloft.net; drivshin@allworx.com; mugunthanvnm@ti.com; ivan.khoronzhuk@linaro.org; Scariah, Thomas <Thomas.Scariah@harman.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org Subject: [EXTERNAL] Re: [PATCH 1/1] ethtool : added get_phy_stats, get_strings,get_sset_count On Mon, Mar 27, 2017 at 09:44:37AM -0700, Florian Fainelli wrote: > Hello, > > On 03/27/2017 03:12 AM, Thomas Scariah wrote: > > From: "Scariah, Thomas" <thomas.scariah@harman.com> > > > > Added functions to support ethtool to print the phy statistics and > > error information along with other ethtool statistics. This will > > help ethtool information to know the error is from physical layer or MAC layer. > > This is an enahancement for ethtool to accommodate phy statistics Hi Thomas What tree is this against? net-next already has get_phy_stats, get_strings, and get_sset_count in struct phy_driver. Andrew
On Tue, Mar 28, 2017 at 11:56:40AM +0530, Thomas Scariah wrote: > Hello Florian/Andrew, > > > On Mon, Mar 27, 2017 at 10:14 PM, Florian Fainelli <f.fainelli@gmail.com> > wrote: > > > Hello, > > > > On 03/27/2017 03:12 AM, Thomas Scariah wrote: > > > From: "Scariah, Thomas" <thomas.scariah@harman.com> > > > > > > Added functions to support ethtool to print the phy statistics and error > > > information along with other ethtool statistics. This will help ethtool > > > information to know the error is from physical layer or MAC layer. > > > This is an enahancement for ethtool to accommodate phy statistics > > > > It sounds like your patch should actually be 3 different patches: > > > > - add helper function to the core PHY library > > - add statistics support to the Micrel PHY driver > > - hook the proper ndo operations in cpsw to allow querying the PHY > > driver's statistics > > > > Yes agreed. This can be 3 different patches . > Anyway the latest kernel already has the same change. > I have developed this patch against Kernel 4.4.14 and the similar > functionality was not available. Hi Thomas O.K, using v4.4 explains a lot. However, we only accept patches against: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git I still suggest you only do the PHY driver parts, and don't touch the MAC driver. Andrew
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index 3f48bb9..26d2dc5 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -1001,9 +1001,16 @@ update_return: static int cpsw_get_sset_count(struct net_device *ndev, int sset) { + struct cpsw_priv *priv = netdev_priv(ndev); + int slave_no = cpsw_slave_index(priv); + int count; + switch (sset) { case ETH_SS_STATS: - return CPSW_STATS_LEN; + count = CPSW_STATS_LEN; + count += phy_ethtool_get_sset_count(priv->slaves[slave_no].phy, + sset); + return count; default: return -EOPNOTSUPP; } @@ -1011,6 +1018,8 @@ static int cpsw_get_sset_count(struct net_device *ndev, int sset) static void cpsw_get_strings(struct net_device *ndev, u32 stringset, u8 *data) { + struct cpsw_priv *priv = netdev_priv(ndev); + int slave_no = cpsw_slave_index(priv); u8 *p = data; int i; @@ -1021,6 +1030,8 @@ static void cpsw_get_strings(struct net_device *ndev, u32 stringset, u8 *data) ETH_GSTRING_LEN); p += ETH_GSTRING_LEN; } + phy_ethtool_get_strings(priv->slaves[slave_no].phy, + stringset, p); break; } } @@ -1031,6 +1042,7 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev, struct cpsw_priv *priv = netdev_priv(ndev); struct cpdma_chan_stats rx_stats; struct cpdma_chan_stats tx_stats; + int slave_no = cpsw_slave_index(priv); u32 val; u8 *p; int i, ret; @@ -1067,6 +1079,8 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev, } } + phy_ethtool_get_stats(priv->slaves[slave_no].phy, stats, + &data[CPSW_STATS_LEN]); pm_runtime_put(&priv->pdev->dev); } diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index e13ad6c..492acbf 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -287,6 +287,53 @@ static int kszphy_config_init(struct phy_device *phydev) return 0; } +struct ks8051_phy_stat { + char name[ETH_GSTRING_LEN]; + u16 offset; +}; + +static const struct ks8051_phy_stat ks8051_phy_stats[] = { + { "PHY RX Error Counter", 0x15 }, +}; + +#define KS8051_STATS_LEN ARRAY_SIZE(ks8051_phy_stats) + +static int ks8051_get_sset_count(struct phy_device *phydev, int sset) +{ + switch (sset) { + case ETH_SS_STATS: + return KS8051_STATS_LEN; + default: + return 0; + } +} + +static void ks8051_get_strings(struct phy_device *phydev, u32 stringset, + u8 *data) +{ + int i; + + switch (stringset) { + case ETH_SS_STATS: + for (i = 0; i < KS8051_STATS_LEN; i++) + memcpy(data + i * ETH_GSTRING_LEN, + ks8051_phy_stats[i].name, ETH_GSTRING_LEN); + break; + } +} + +static void ks8051_get_phy_stats(struct phy_device *phydev, + struct ethtool_stats *stats, u64 *data) +{ + u32 i; + u32 val; + + for (i = 0; i < KS8051_STATS_LEN; i++) { + val = phy_read(phydev, ks8051_phy_stats[i].offset); + data[i] = val; + } +} + static int ksz9021_load_values_from_of(struct phy_device *phydev, const struct device_node *of_node, u16 reg, @@ -724,6 +771,9 @@ static struct phy_driver ksphy_driver[] = { .probe = kszphy_probe, .config_init = kszphy_config_init, .config_aneg = genphy_config_aneg, + .get_phy_stats = ks8051_get_phy_stats, + .get_strings = ks8051_get_strings, + .get_sset_count = ks8051_get_sset_count, .read_status = genphy_read_status, .ack_interrupt = kszphy_ack_interrupt, .config_intr = kszphy_config_intr, diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 47cd306..25b66cd 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1262,3 +1262,26 @@ void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) phydev->drv->get_wol(phydev, wol); } EXPORT_SYMBOL(phy_ethtool_get_wol); + +int phy_ethtool_get_sset_count(struct phy_device *phydev, int sset) +{ + if (phydev->drv->get_sset_count) + return phydev->drv->get_sset_count(phydev, sset); + return 0; +} +EXPORT_SYMBOL(phy_ethtool_get_sset_count); + +void phy_ethtool_get_strings(struct phy_device *phydev, u32 stringset, u8 *data) +{ + if (phydev->drv->get_strings) + phydev->drv->get_strings(phydev, stringset, data); +} +EXPORT_SYMBOL(phy_ethtool_get_strings); + +void phy_ethtool_get_stats(struct phy_device *phydev, + struct ethtool_stats *stats, u64 *data) +{ + if (phydev->drv->get_phy_stats) + phydev->drv->get_phy_stats(phydev, stats, data); +} +EXPORT_SYMBOL(phy_ethtool_get_stats); diff --git a/include/linux/phy.h b/include/linux/phy.h index 05fde31..4f14368 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -589,6 +589,15 @@ struct phy_driver { int (*module_eeprom)(struct phy_device *dev, struct ethtool_eeprom *ee, u8 *data); + /* Request for phy statitsics information and printing statistics + * information from ethtool stats. So Rx stats, Tx stats and error + * information can be read from + */ + void (*get_phy_stats)(struct phy_device *, struct ethtool_stats *, + u64 *); + void (*get_strings)(struct phy_device *, u32 stringset, u8 *); + int (*get_sset_count)(struct phy_device *, int); + struct device_driver driver; }; #define to_phy_driver(d) container_of(d, struct phy_driver, driver) @@ -816,6 +825,11 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data); int phy_ethtool_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol); void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol); +int phy_ethtool_get_sset_count(struct phy_device *phydev, int sset); +void phy_ethtool_get_strings(struct phy_device *phydev, u32 stringset, + u8 *data); +void phy_ethtool_get_stats(struct phy_device *phydev, + struct ethtool_stats *stats, u64 *data); int __init mdio_bus_init(void); void mdio_bus_exit(void);