diff mbox series

[net-next,07/12] net: dsa: rzn1-a5psw: add statistics support

Message ID 20220414122250.158113-8-clement.leger@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series add support for Renesas RZ/N1 ethernet subsystem devices | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/checkpatch warning CHECK: spinlock_t definition without comment
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/source_inline success Was 0 now: 0

Commit Message

Clément Léger April 14, 2022, 12:22 p.m. UTC
Add per-port statistics. This support requries to add a stat lock since
statistics are stored in two 32 bits registers, the hi part one being
global and latched when accessing the lo part.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/rzn1_a5psw.h |   2 +
 2 files changed, 103 insertions(+)

Comments

Vladimir Oltean April 14, 2022, 5:34 p.m. UTC | #1
On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote:
> Add per-port statistics. This support requries to add a stat lock since
> statistics are stored in two 32 bits registers, the hi part one being
> global and latched when accessing the lo part.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---

I think for new drivers Jakub will also want to see the more specific
and less free-form get_stats64, get_eth_mac_stats, get_eth_phy_stats,
get_eth_ctrl_stats ops implemented. Your counters should map nicely over
these.

>  drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/rzn1_a5psw.h |   2 +
>  2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> index 5bee999f7050..7ab7d9054427 100644
> --- a/drivers/net/dsa/rzn1_a5psw.c
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -16,6 +16,59 @@
>  
>  #include "rzn1_a5psw.h"
>  
> +struct a5psw_stats {
> +	u16 offset;
> +	const char *name;
> +};
> +
> +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name}
> +
> +static const struct a5psw_stats a5psw_stats[] = {
> +	STAT_DESC(0x868, "aFrameTransmitted"),
> +	STAT_DESC(0x86C, "aFrameReceived"),
> +	STAT_DESC(0x870, "aFrameCheckSequenceErrors"),
> +	STAT_DESC(0x874, "aAlignmentErrors"),
> +	STAT_DESC(0x878, "aOctetsTransmitted"),
> +	STAT_DESC(0x87C, "aOctetsReceived"),
> +	STAT_DESC(0x880, "aTxPAUSEMACCtrlFrames"),
> +	STAT_DESC(0x884, "aRxPAUSEMACCtrlFrames"),

What does the "a" stand for?

> +	/* If */
> +	STAT_DESC(0x888, "ifInErrors"),
> +	STAT_DESC(0x88C, "ifOutErrors"),
> +	STAT_DESC(0x890, "ifInUcastPkts"),
> +	STAT_DESC(0x894, "ifInMulticastPkts"),
> +	STAT_DESC(0x898, "ifInBroadcastPkts"),
> +	STAT_DESC(0x89C, "ifOutDiscards"),
> +	STAT_DESC(0x8A0, "ifOutUcastPkts"),
> +	STAT_DESC(0x8A4, "ifOutMulticastPkts"),
> +	STAT_DESC(0x8A8, "ifOutBroadcastPkts"),
> +	/* Ether */
> +	STAT_DESC(0x8AC, "etherStatsDropEvents"),
> +	STAT_DESC(0x8B0, "etherStatsOctets"),
> +	STAT_DESC(0x8B4, "etherStatsPkts"),
> +	STAT_DESC(0x8B8, "etherStatsUndersizePkts"),
> +	STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"),

"etherStats" is duplicated here.

> +	STAT_DESC(0x8C0, "etherStatsPkts64Octets"),
> +	STAT_DESC(0x8C4, "etherStatsPkts65to127Octets"),
> +	STAT_DESC(0x8C8, "etherStatsPkts128to255Octets"),
> +	STAT_DESC(0x8CC, "etherStatsPkts256to511Octets"),
> +	STAT_DESC(0x8D0, "etherStatsPkts512to1023Octets"),
> +	STAT_DESC(0x8D4, "etherStatsPkts1024to1518Octets"),
> +	STAT_DESC(0x8D8, "etherStatsPkts1519toXOctets"),
> +	STAT_DESC(0x8DC, "etherStatsJabbers"),
> +	STAT_DESC(0x8E0, "etherStatsFragments"),
> +
> +	STAT_DESC(0x8E8, "VLANReceived"),
> +	STAT_DESC(0x8EC, "VLANTransmitted"),
> +
> +	STAT_DESC(0x910, "aDeferred"),
> +	STAT_DESC(0x914, "aMultipleCollisions"),
> +	STAT_DESC(0x918, "aSingleCollisions"),
> +	STAT_DESC(0x91C, "aLateCollisions"),
> +	STAT_DESC(0x920, "aExcessiveCollisions"),
> +	STAT_DESC(0x924, "aCarrierSenseErrors"),
> +};
> +
>  static void a5psw_reg_writel(struct a5psw *a5psw, int offset, u32 value)
>  {
>  	writel(value, a5psw->base + offset);
> @@ -316,6 +369,50 @@ static void a5psw_port_fast_age(struct dsa_switch *ds, int port)
>  	a5psw_port_fdb_flush(a5psw, port);
>  }
>  
> +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
> +			      uint8_t *data)
> +{
> +	unsigned int u;
> +
> +	if (stringset != ETH_SS_STATS)
> +		return;
> +
> +	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> +		strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name,
> +			ETH_GSTRING_LEN);
> +	}
> +}
> +
> +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> +				    uint64_t *data)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +	u32 reg_lo, reg_hi;
> +	unsigned int u;
> +
> +	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> +		/* A5PSW_STATS_HIWORD is global and thus, access must be
> +		 * exclusive
> +		 */
> +		spin_lock(&a5psw->stats_lock);
> +		reg_lo = a5psw_reg_readl(a5psw, a5psw_stats[u].offset +
> +					 A5PSW_PORT_OFFSET(port));
> +		/* A5PSW_STATS_HIWORD is latched on stat read */
> +		reg_hi = a5psw_reg_readl(a5psw, A5PSW_STATS_HIWORD);
> +
> +		data[u] = ((u64)reg_hi << 32) | reg_lo;
> +		spin_unlock(&a5psw->stats_lock);
> +	}
> +}
> +
> +static int a5psw_get_sset_count(struct dsa_switch *ds, int port, int sset)
> +{
> +	if (sset != ETH_SS_STATS)
> +		return 0;
> +
> +	return ARRAY_SIZE(a5psw_stats);
> +}
> +
>  static int a5psw_setup(struct dsa_switch *ds)
>  {
>  	struct a5psw *a5psw = ds->priv;
> @@ -395,6 +492,9 @@ const struct dsa_switch_ops a5psw_switch_ops = {
>  	.phylink_mac_link_up = a5psw_phylink_mac_link_up,
>  	.port_change_mtu = a5psw_port_change_mtu,
>  	.port_max_mtu = a5psw_port_max_mtu,
> +	.get_sset_count = a5psw_get_sset_count,
> +	.get_strings = a5psw_get_strings,
> +	.get_ethtool_stats = a5psw_get_ethtool_stats,
>  	.set_ageing_time = a5psw_set_ageing_time,
>  	.port_bridge_join = a5psw_port_bridge_join,
>  	.port_bridge_leave = a5psw_port_bridge_leave,
> @@ -580,6 +680,7 @@ static int a5psw_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	a5psw->dev = dev;
> +	spin_lock_init(&a5psw->stats_lock);
>  	spin_lock_init(&a5psw->lk_lock);
>  	spin_lock_init(&a5psw->reg_lock);
>  	a5psw->base = devm_platform_ioremap_resource(pdev, 0);
> diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
> index 2d96a2afbc3a..b34ea549e936 100644
> --- a/drivers/net/dsa/rzn1_a5psw.h
> +++ b/drivers/net/dsa/rzn1_a5psw.h
> @@ -177,6 +177,7 @@
>   * @mdio_freq: MDIO bus frequency requested
>   * @pcs: Array of PCS connected to the switch ports (not for the CPU)
>   * @ds: DSA switch struct
> + * @stats_lock: lock to access statistics (shared HI counter)
>   * @lk_lock: Lock for the lookup table
>   * @reg_lock: Lock for register read-modify-write operation
>   * @flooding_ports: List of ports that should be flooded
> @@ -190,6 +191,7 @@ struct a5psw {
>  	u32 mdio_freq;
>  	struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
>  	struct dsa_switch ds;
> +	spinlock_t stats_lock;
>  	spinlock_t lk_lock;
>  	spinlock_t reg_lock;
>  	u32 flooding_ports;
> -- 
> 2.34.1
>
Andrew Lunn April 14, 2022, 11:16 p.m. UTC | #2
On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote:
> Add per-port statistics. This support requries to add a stat lock since
> statistics are stored in two 32 bits registers, the hi part one being
> global and latched when accessing the lo part.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/rzn1_a5psw.h |   2 +
>  2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> index 5bee999f7050..7ab7d9054427 100644
> --- a/drivers/net/dsa/rzn1_a5psw.c
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -16,6 +16,59 @@
>  
>  #include "rzn1_a5psw.h"
>  
> +struct a5psw_stats {
> +	u16 offset;
> +	const char *name;
> +};
> +
> +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name}
> +
> +static const struct a5psw_stats a5psw_stats[] = {
> +	STAT_DESC(0x868, "aFrameTransmitted"),
> +	STAT_DESC(0x86C, "aFrameReceived"),
> +	STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"),

> +};


> +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
> +			      uint8_t *data)
> +{
> +	unsigned int u;
> +
> +	if (stringset != ETH_SS_STATS)
> +		return;
> +
> +	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> +		strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name,
> +			ETH_GSTRING_LEN);
> +	}

The kernel strncpy() is like the user space one. It does not add a
NULL if the string is longer than ETH_GSTRING_LEN and it needs to
truncate. So there is a danger here.

What you find most drivers do is

struct a5psw_stats {
	u16 offset;
	const char name[ETH_GSTRING_LEN];
};

You should then get a compiler warning/error if you string is ever
longer than allowed. And use memcpy() rather than strcpy(), which is
faster anyway. But you do use up a bit more memory.

> +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> +				    uint64_t *data)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +	u32 reg_lo, reg_hi;
> +	unsigned int u;
> +
> +	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> +		/* A5PSW_STATS_HIWORD is global and thus, access must be
> +		 * exclusive
> +		 */

Could you explain that a bit more. The RTNL lock will prevent two
parallel calls to this function.

> +		spin_lock(&a5psw->stats_lock);
> +		reg_lo = a5psw_reg_readl(a5psw, a5psw_stats[u].offset +
> +					 A5PSW_PORT_OFFSET(port));
> +		/* A5PSW_STATS_HIWORD is latched on stat read */
> +		reg_hi = a5psw_reg_readl(a5psw, A5PSW_STATS_HIWORD);
> +
> +		data[u] = ((u64)reg_hi << 32) | reg_lo;
> +		spin_unlock(&a5psw->stats_lock);
> +	}
> +}

  Andrew
Clément Léger April 15, 2022, 12:04 p.m. UTC | #3
Le Fri, 15 Apr 2022 01:16:11 +0200,
Andrew Lunn <andrew@lunn.ch> a écrit :

> On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote:
> > Add per-port statistics. This support requries to add a stat lock since
> > statistics are stored in two 32 bits registers, the hi part one being
> > global and latched when accessing the lo part.
> > 
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> >  drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/rzn1_a5psw.h |   2 +
> >  2 files changed, 103 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> > index 5bee999f7050..7ab7d9054427 100644
> > --- a/drivers/net/dsa/rzn1_a5psw.c
> > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > @@ -16,6 +16,59 @@
> >  
> >  #include "rzn1_a5psw.h"
> >  
> > +struct a5psw_stats {
> > +	u16 offset;
> > +	const char *name;
> > +};
> > +
> > +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name}
> > +
> > +static const struct a5psw_stats a5psw_stats[] = {
> > +	STAT_DESC(0x868, "aFrameTransmitted"),
> > +	STAT_DESC(0x86C, "aFrameReceived"),
> > +	STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"),  
> 
> > +};  
> 
> 
> > +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
> > +			      uint8_t *data)
> > +{
> > +	unsigned int u;
> > +
> > +	if (stringset != ETH_SS_STATS)
> > +		return;
> > +
> > +	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> > +		strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name,
> > +			ETH_GSTRING_LEN);
> > +	}  
> 
> The kernel strncpy() is like the user space one. It does not add a
> NULL if the string is longer than ETH_GSTRING_LEN and it needs to
> truncate. So there is a danger here.
> 
> What you find most drivers do is
> 
> struct a5psw_stats {
> 	u16 offset;
> 	const char name[ETH_GSTRING_LEN];
> };
> 
> You should then get a compiler warning/error if you string is ever
> longer than allowed. And use memcpy() rather than strcpy(), which is
> faster anyway. But you do use up a bit more memory.

Acked.

> 
> > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> > +				    uint64_t *data)
> > +{
> > +	struct a5psw *a5psw = ds->priv;
> > +	u32 reg_lo, reg_hi;
> > +	unsigned int u;
> > +
> > +	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> > +		/* A5PSW_STATS_HIWORD is global and thus, access must be
> > +		 * exclusive
> > +		 */  
> 
> Could you explain that a bit more. The RTNL lock will prevent two
> parallel calls to this function.

Ok, I wasn't sure of the locking applicable here.
Clément Léger April 15, 2022, 12:42 p.m. UTC | #4
Le Thu, 14 Apr 2022 20:34:44 +0300,
Vladimir Oltean <olteanv@gmail.com> a écrit :

> On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote:
> > Add per-port statistics. This support requries to add a stat lock since
> > statistics are stored in two 32 bits registers, the hi part one being
> > global and latched when accessing the lo part.
> > 
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---  
> 
> I think for new drivers Jakub will also want to see the more specific
> and less free-form get_stats64, get_eth_mac_stats, get_eth_phy_stats,
> get_eth_ctrl_stats ops implemented. Your counters should map nicely over
> these.

Ok, I'll implement these callbacks !

> 
> >  drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/rzn1_a5psw.h |   2 +
> >  2 files changed, 103 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> > index 5bee999f7050..7ab7d9054427 100644
> > --- a/drivers/net/dsa/rzn1_a5psw.c
> > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > @@ -16,6 +16,59 @@
> >  
> >  #include "rzn1_a5psw.h"
> >  
> > +struct a5psw_stats {
> > +	u16 offset;
> > +	const char *name;
> > +};
> > +
> > +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name}
> > +
> > +static const struct a5psw_stats a5psw_stats[] = {
> > +	STAT_DESC(0x868, "aFrameTransmitted"),
> > +	STAT_DESC(0x86C, "aFrameReceived"),
> > +	STAT_DESC(0x870, "aFrameCheckSequenceErrors"),
> > +	STAT_DESC(0x874, "aAlignmentErrors"),
> > +	STAT_DESC(0x878, "aOctetsTransmitted"),
> > +	STAT_DESC(0x87C, "aOctetsReceived"),
> > +	STAT_DESC(0x880, "aTxPAUSEMACCtrlFrames"),
> > +	STAT_DESC(0x884, "aRxPAUSEMACCtrlFrames"),  
> 
> What does the "a" stand for?

That's a mystery :/ I tried to be a normal person and copy/pasted these
from the datasheet ;)
Andrew Lunn April 15, 2022, 1:37 p.m. UTC | #5
> > > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> > > +				    uint64_t *data)
> > > +{
> > > +	struct a5psw *a5psw = ds->priv;
> > > +	u32 reg_lo, reg_hi;
> > > +	unsigned int u;
> > > +
> > > +	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> > > +		/* A5PSW_STATS_HIWORD is global and thus, access must be
> > > +		 * exclusive
> > > +		 */  
> > 
> > Could you explain that a bit more. The RTNL lock will prevent two
> > parallel calls to this function.
> 
> Ok, I wasn't sure of the locking applicable here.

In general, RTNL protects you for any user space management like
operation on the driver. In this case, if you look in net/ethtool, you
will find the IOCTL handler code takes RTNL before calling into the
main IOCTL dispatcher. If you want to be paranoid/document the
assumption, you can add an ASSERT_RTNL().

The semantics for some of the other statistics Vladimir requested can
be slightly different. One of them is in atomic context, because a
spinlock is held. But i don't remember if RTNL is also held. This is
less of an issue for your switch, since it uses MMIO, however many
switches need to perform blocking IO over MDIO, SPI, IC2 etc to get
stats, which you cannot do in atomic context. So they end up returning
cached values.

Look in the mailing list for past discussion for details.

    Andrew
Clément Léger April 15, 2022, 1:44 p.m. UTC | #6
Le Fri, 15 Apr 2022 15:37:38 +0200,
Andrew Lunn <andrew@lunn.ch> a écrit :

> > > > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> > > > +				    uint64_t *data)
> > > > +{
> > > > +	struct a5psw *a5psw = ds->priv;
> > > > +	u32 reg_lo, reg_hi;
> > > > +	unsigned int u;
> > > > +
> > > > +	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> > > > +		/* A5PSW_STATS_HIWORD is global and thus, access must be
> > > > +		 * exclusive
> > > > +		 */    
> > > 
> > > Could you explain that a bit more. The RTNL lock will prevent two
> > > parallel calls to this function.  
> > 
> > Ok, I wasn't sure of the locking applicable here.  
> 
> In general, RTNL protects you for any user space management like
> operation on the driver. In this case, if you look in net/ethtool, you
> will find the IOCTL handler code takes RTNL before calling into the
> main IOCTL dispatcher. If you want to be paranoid/document the
> assumption, you can add an ASSERT_RTNL().

Ok, I'll look at the call stack in details to see what locking is
applied.

> 
> The semantics for some of the other statistics Vladimir requested can
> be slightly different. One of them is in atomic context, because a
> spinlock is held. But i don't remember if RTNL is also held. This is
> less of an issue for your switch, since it uses MMIO, however many
> switches need to perform blocking IO over MDIO, SPI, IC2 etc to get
> stats, which you cannot do in atomic context. So they end up returning
> cached values.
> 
> Look in the mailing list for past discussion for details.

Ok, thanks,

> 
>     Andrew
diff mbox series

Patch

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index 5bee999f7050..7ab7d9054427 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -16,6 +16,59 @@ 
 
 #include "rzn1_a5psw.h"
 
+struct a5psw_stats {
+	u16 offset;
+	const char *name;
+};
+
+#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name}
+
+static const struct a5psw_stats a5psw_stats[] = {
+	STAT_DESC(0x868, "aFrameTransmitted"),
+	STAT_DESC(0x86C, "aFrameReceived"),
+	STAT_DESC(0x870, "aFrameCheckSequenceErrors"),
+	STAT_DESC(0x874, "aAlignmentErrors"),
+	STAT_DESC(0x878, "aOctetsTransmitted"),
+	STAT_DESC(0x87C, "aOctetsReceived"),
+	STAT_DESC(0x880, "aTxPAUSEMACCtrlFrames"),
+	STAT_DESC(0x884, "aRxPAUSEMACCtrlFrames"),
+	/* If */
+	STAT_DESC(0x888, "ifInErrors"),
+	STAT_DESC(0x88C, "ifOutErrors"),
+	STAT_DESC(0x890, "ifInUcastPkts"),
+	STAT_DESC(0x894, "ifInMulticastPkts"),
+	STAT_DESC(0x898, "ifInBroadcastPkts"),
+	STAT_DESC(0x89C, "ifOutDiscards"),
+	STAT_DESC(0x8A0, "ifOutUcastPkts"),
+	STAT_DESC(0x8A4, "ifOutMulticastPkts"),
+	STAT_DESC(0x8A8, "ifOutBroadcastPkts"),
+	/* Ether */
+	STAT_DESC(0x8AC, "etherStatsDropEvents"),
+	STAT_DESC(0x8B0, "etherStatsOctets"),
+	STAT_DESC(0x8B4, "etherStatsPkts"),
+	STAT_DESC(0x8B8, "etherStatsUndersizePkts"),
+	STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"),
+	STAT_DESC(0x8C0, "etherStatsPkts64Octets"),
+	STAT_DESC(0x8C4, "etherStatsPkts65to127Octets"),
+	STAT_DESC(0x8C8, "etherStatsPkts128to255Octets"),
+	STAT_DESC(0x8CC, "etherStatsPkts256to511Octets"),
+	STAT_DESC(0x8D0, "etherStatsPkts512to1023Octets"),
+	STAT_DESC(0x8D4, "etherStatsPkts1024to1518Octets"),
+	STAT_DESC(0x8D8, "etherStatsPkts1519toXOctets"),
+	STAT_DESC(0x8DC, "etherStatsJabbers"),
+	STAT_DESC(0x8E0, "etherStatsFragments"),
+
+	STAT_DESC(0x8E8, "VLANReceived"),
+	STAT_DESC(0x8EC, "VLANTransmitted"),
+
+	STAT_DESC(0x910, "aDeferred"),
+	STAT_DESC(0x914, "aMultipleCollisions"),
+	STAT_DESC(0x918, "aSingleCollisions"),
+	STAT_DESC(0x91C, "aLateCollisions"),
+	STAT_DESC(0x920, "aExcessiveCollisions"),
+	STAT_DESC(0x924, "aCarrierSenseErrors"),
+};
+
 static void a5psw_reg_writel(struct a5psw *a5psw, int offset, u32 value)
 {
 	writel(value, a5psw->base + offset);
@@ -316,6 +369,50 @@  static void a5psw_port_fast_age(struct dsa_switch *ds, int port)
 	a5psw_port_fdb_flush(a5psw, port);
 }
 
+static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
+			      uint8_t *data)
+{
+	unsigned int u;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
+		strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name,
+			ETH_GSTRING_LEN);
+	}
+}
+
+static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
+				    uint64_t *data)
+{
+	struct a5psw *a5psw = ds->priv;
+	u32 reg_lo, reg_hi;
+	unsigned int u;
+
+	for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
+		/* A5PSW_STATS_HIWORD is global and thus, access must be
+		 * exclusive
+		 */
+		spin_lock(&a5psw->stats_lock);
+		reg_lo = a5psw_reg_readl(a5psw, a5psw_stats[u].offset +
+					 A5PSW_PORT_OFFSET(port));
+		/* A5PSW_STATS_HIWORD is latched on stat read */
+		reg_hi = a5psw_reg_readl(a5psw, A5PSW_STATS_HIWORD);
+
+		data[u] = ((u64)reg_hi << 32) | reg_lo;
+		spin_unlock(&a5psw->stats_lock);
+	}
+}
+
+static int a5psw_get_sset_count(struct dsa_switch *ds, int port, int sset)
+{
+	if (sset != ETH_SS_STATS)
+		return 0;
+
+	return ARRAY_SIZE(a5psw_stats);
+}
+
 static int a5psw_setup(struct dsa_switch *ds)
 {
 	struct a5psw *a5psw = ds->priv;
@@ -395,6 +492,9 @@  const struct dsa_switch_ops a5psw_switch_ops = {
 	.phylink_mac_link_up = a5psw_phylink_mac_link_up,
 	.port_change_mtu = a5psw_port_change_mtu,
 	.port_max_mtu = a5psw_port_max_mtu,
+	.get_sset_count = a5psw_get_sset_count,
+	.get_strings = a5psw_get_strings,
+	.get_ethtool_stats = a5psw_get_ethtool_stats,
 	.set_ageing_time = a5psw_set_ageing_time,
 	.port_bridge_join = a5psw_port_bridge_join,
 	.port_bridge_leave = a5psw_port_bridge_leave,
@@ -580,6 +680,7 @@  static int a5psw_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	a5psw->dev = dev;
+	spin_lock_init(&a5psw->stats_lock);
 	spin_lock_init(&a5psw->lk_lock);
 	spin_lock_init(&a5psw->reg_lock);
 	a5psw->base = devm_platform_ioremap_resource(pdev, 0);
diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
index 2d96a2afbc3a..b34ea549e936 100644
--- a/drivers/net/dsa/rzn1_a5psw.h
+++ b/drivers/net/dsa/rzn1_a5psw.h
@@ -177,6 +177,7 @@ 
  * @mdio_freq: MDIO bus frequency requested
  * @pcs: Array of PCS connected to the switch ports (not for the CPU)
  * @ds: DSA switch struct
+ * @stats_lock: lock to access statistics (shared HI counter)
  * @lk_lock: Lock for the lookup table
  * @reg_lock: Lock for register read-modify-write operation
  * @flooding_ports: List of ports that should be flooded
@@ -190,6 +191,7 @@  struct a5psw {
 	u32 mdio_freq;
 	struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
 	struct dsa_switch ds;
+	spinlock_t stats_lock;
 	spinlock_t lk_lock;
 	spinlock_t reg_lock;
 	u32 flooding_ports;