On Thu, Feb 07, 2019 at 08:07:07PM -0800, tristram...@microchip.com wrote:
> From: Tristram Ha <tristram...@microchip.com>
>
> Add MIB counter reading support.
>
> Signed-off-by: Tristram Ha <tristram...@microchip.com>
> ---
> drivers/net/dsa/microchip/ksz9477.c | 139
> +++++++++++++++++++++++----------
> drivers/net/dsa/microchip/ksz_common.c | 96 +++++++++++++++++++++++
> drivers/net/dsa/microchip/ksz_common.h | 2 +
> drivers/net/dsa/microchip/ksz_priv.h | 7 +-
> 4 files changed, 198 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> index 0fdb22d..4502e13 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -10,6 +10,7 @@
> #include <linux/gpio.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/iopoll.h>
> #include <linux/platform_data/microchip-ksz.h>
> #include <linux/phy.h>
> #include <linux/etherdevice.h>
> @@ -18,8 +19,8 @@
> #include <net/switchdev.h>
>
> #include "ksz_priv.h"
> -#include "ksz_common.h"
> #include "ksz9477_reg.h"
> +#include "ksz_common.h"
>
> static const struct {
> int index;
> @@ -92,6 +93,27 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int
> port, int offset,
> ksz_write32(dev, addr, data);
> }
>
> +#define read8_op(addr) \
> +({ \
> + u8 data; \
> + ksz_read8(dev, addr, &data); \
> + data; \
> +})
> +
> +#define read32_op(addr) \
> +({ \
> + u32 data; \
> + ksz_read32(dev, addr, &data); \
> + data; \
> +})
These two are not used. Please remove them.
> +
> +#define pread32_op(addr) \
> +({ \
> + u32 data; \
> + ksz_pread32(dev, port, addr, &data); \
> + data; \
> +})
It works, but it is not nice, and it makes assumptions about how
readx_poll_timeout is implemented.
> + ret = readx_poll_timeout(pread32_op, REG_PORT_MIB_CTRL_STAT__4, data,
> + !(data & MIB_COUNTER_READ), 10, 1000);
The macro is only used one, and addr is fixed,
REG_PORT_MIB_CTRL_STAT__4. So you can at least replace addr with port,
and rename the macro pread32_stat(port).
> + /* failed to read MIB. get out of loop */
> + if (ret < 0) {
> + dev_dbg(dev->dev, "Failed to get MIB\n");
> + return;
> + }
> +
> + /* count resets upon read */
> + ksz_pread32(dev, port, REG_PORT_MIB_DATA, &data);
> + *cnt += data;
> +}
> +
> +static void ksz9477_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
> + u64 *dropped, u64 *cnt)
> +{
> + addr = ksz9477_mib_names[addr].index;
> + ksz9477_r_mib_cnt(dev, port, addr, cnt);
> +}
> +
> +static void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
> +{
> + struct ksz_port *p = &dev->ports[port];
> + u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
Reverse Christmas tree.
> +
> + /* enable/disable the port for flush/freeze function */
> + mutex_lock(&p->mib.cnt_mutex);
> + ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, val);
> +
> + /* used by MIB counter reading code to know freeze is enabled */
> + p->freeze = freeze;
> + mutex_unlock(&p->mib.cnt_mutex);
> +}
> +static void ksz_mib_read_work(struct work_struct *work)
> +{
> + struct ksz_device *dev =
> + container_of(work, struct ksz_device, mib_read);
> + struct ksz_port *p;
> + struct ksz_port_mib *mib;
> + int i;
> +
> + for (i = 0; i < dev->mib_port_cnt; i++) {
> + p = &dev->ports[i];
> + if (!p->on)
> + continue;
> + mib = &p->mib;
> + mutex_lock(&mib->cnt_mutex);
> +
> + /* read only dropped counters when link is not up */
> + if (p->link_just_down)
> + p->link_just_down = 0;
> + else if (!p->phydev.link)
> + mib->cnt_ptr = dev->reg_mib_cnt;
This link_just_down stuff is not clear at all. Why can the drop
counters not be read when the link is up?
> + port_r_cnt(dev, i);
> + mutex_unlock(&mib->cnt_mutex);
> + }
> +}
