[PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[Oleksij Rempel]

This patch series is a result of maintaining work on ksz8 part of
microchip driver. It includes stats64 and fdb support. Error handling.
Loopback fix and so on...

Oleksij Rempel (26):
  net: dsa: microchip: add stats64 support for ksz8 series of switches
  net: dsa: microchip: ksz8: ksz8_fdb_dump: fix port validation and VID
    information
  net: dsa: microchip: ksz8: ksz8_fdb_dump: fix not complete fdb
    extraction
  net: dsa: microchip: ksz8: ksz8_fdb_dump: fix time stamp extraction
  net: dsa: microchip: ksz8: ksz8_fdb_dump: do not extract ghost entry
    from empty table
  net: dsa: microchip: ksz8863_smi: fix bulk access
  net: dsa: microchip: ksz8_r_dyn_mac_table(): remove timestamp support
  net: dsa: microchip: make ksz8_r_dyn_mac_table() static
  net: dsa: microchip: ksz8_r_dyn_mac_table(): remove fid support
  net: dsa: microchip: ksz8: refactor ksz8_fdb_dump()
  net: dsa: microchip: ksz8: ksz8_fdb_dump: dump static MAC table
  net: dsa: microchip: ksz8: move static mac table operations to a
    separate functions
  net: dsa: microchip: ksz8: add fdb_add/del support
  net: dsa: microchip: KSZ88x3 fix loopback support
  net: dsa: microchip: ksz8_r_dyn_mac_table(): move main part of the
    code out of if statement
  net: dsa: microchip: ksz8_r_dyn_mac_table(): use ret instead of rc
  net: dsa: microchip: ksz8_r_dyn_mac_table(): ksz: do not return EAGAIN
    on timeout
  net: dsa: microchip: ksz8_r_dyn_mac_table(): return read/write error
    if we got any
  net: dsa: microchip: ksz8_r_dyn_mac_table(): use entries variable to
    signal 0 entries
  net: dsa: microchip: make ksz8_r_sta_mac_table() static
  net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for
    empty entries
  net: dsa: microchip: ksz8_r_sta_mac_table(): make use of error values
    provided by read/write functions
  net: dsa: microchip: make ksz8_w_sta_mac_table() static
  net: dsa: microchip: ksz8_w_sta_mac_table(): make use of error values
    provided by read/write functions
  net: dsa: microchip: remove ksz_port:on variable
  net: dsa: microchip: ksz8: do not force flow control by default

 drivers/net/dsa/microchip/ksz8.h        |  14 +-
 drivers/net/dsa/microchip/ksz8795.c     | 440 +++++++++++++++---------
 drivers/net/dsa/microchip/ksz8795_reg.h |   2 +
 drivers/net/dsa/microchip/ksz8863_smi.c |  10 +-
 drivers/net/dsa/microchip/ksz_common.c  | 100 +++++-
 drivers/net/dsa/microchip/ksz_common.h  |   2 +-
 6 files changed, 377 insertions(+), 191 deletions(-)

-- 
2.30.2

[PATCH v1 01/26] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Oleksij Rempel]

Add stats64 support for ksz8xxx series of switches.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 87 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |  1 +
 2 files changed, 88 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index f39b041765fb..423f944cc34c 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -70,6 +70,43 @@ struct ksz_stats_raw {
 	u64 tx_discards;
 };
 
+struct ksz88xx_stats_raw {
+	u64 rx;
+	u64 rx_hi;
+	u64 rx_undersize;
+	u64 rx_fragments;
+	u64 rx_oversize;
+	u64 rx_jabbers;
+	u64 rx_symbol_err;
+	u64 rx_crc_err;
+	u64 rx_align_err;
+	u64 rx_mac_ctrl;
+	u64 rx_pause;
+	u64 rx_bcast;
+	u64 rx_mcast;
+	u64 rx_ucast;
+	u64 rx_64_or_less;
+	u64 rx_65_127;
+	u64 rx_128_255;
+	u64 rx_256_511;
+	u64 rx_512_1023;
+	u64 rx_1024_1522;
+	u64 tx;
+	u64 tx_hi;
+	u64 tx_late_col;
+	u64 tx_pause;
+	u64 tx_bcast;
+	u64 tx_mcast;
+	u64 tx_ucast;
+	u64 tx_deferred;
+	u64 tx_total_col;
+	u64 tx_exc_col;
+	u64 tx_single_col;
+	u64 tx_mult_col;
+	u64 rx_discards;
+	u64 tx_discards;
+};
+
 static const struct ksz_mib_names ksz88xx_mib_names[] = {
 	{ 0x00, "rx" },
 	{ 0x01, "rx_hi" },
@@ -156,6 +193,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
 	.w_phy = ksz8_w_phy,
 	.r_mib_cnt = ksz8_r_mib_cnt,
 	.r_mib_pkt = ksz8_r_mib_pkt,
+	.r_mib_stat64 = ksz88xx_r_mib_stats64,
 	.freeze_mib = ksz8_freeze_mib,
 	.port_init_cnt = ksz8_port_init_cnt,
 	.fdb_dump = ksz8_fdb_dump,
@@ -1583,6 +1621,55 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 	spin_unlock(&mib->stats64_lock);
 }
 
+void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
+{
+	struct ethtool_pause_stats *pstats;
+	struct rtnl_link_stats64 *stats;
+	struct ksz88xx_stats_raw *raw;
+	struct ksz_port_mib *mib;
+
+	mib = &dev->ports[port].mib;
+	stats = &mib->stats64;
+	pstats = &mib->pause_stats;
+	raw = (struct ksz88xx_stats_raw *)mib->counters;
+
+	spin_lock(&mib->stats64_lock);
+
+	stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast +
+		raw->rx_pause;
+	stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast +
+		raw->tx_pause;
+
+	/* HW counters are counting bytes + FCS which is not acceptable
+	 * for rtnl_link_stats64 interface
+	 */
+	stats->rx_bytes = raw->rx + raw->rx_hi - stats->rx_packets * ETH_FCS_LEN;
+	stats->tx_bytes = raw->tx + raw->tx_hi - stats->tx_packets * ETH_FCS_LEN;
+
+	stats->rx_length_errors = raw->rx_undersize + raw->rx_fragments +
+		raw->rx_oversize;
+
+	stats->rx_crc_errors = raw->rx_crc_err;
+	stats->rx_frame_errors = raw->rx_align_err;
+	stats->rx_dropped = raw->rx_discards;
+	stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors +
+		stats->rx_frame_errors  + stats->rx_dropped;
+
+	stats->tx_window_errors = raw->tx_late_col;
+	stats->tx_fifo_errors = raw->tx_discards;
+	stats->tx_aborted_errors = raw->tx_exc_col;
+	stats->tx_errors = stats->tx_window_errors + stats->tx_fifo_errors +
+		stats->tx_aborted_errors;
+
+	stats->multicast = raw->rx_mcast;
+	stats->collisions = raw->tx_total_col;
+
+	pstats->tx_pause_frames = raw->tx_pause;
+	pstats->rx_pause_frames = raw->rx_pause;
+
+	spin_unlock(&mib->stats64_lock);
+}
+
 static void ksz_get_stats64(struct dsa_switch *ds, int port,
 			    struct rtnl_link_stats64 *s)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index cb27f5a180c7..055d61ff3fb8 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -345,6 +345,7 @@ void ksz_switch_remove(struct ksz_device *dev);
 
 void ksz_init_mib_timer(struct ksz_device *dev);
 void ksz_r_mib_stats64(struct ksz_device *dev, int port);
+void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port);
 void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
 bool ksz_get_gbit(struct ksz_device *dev, int port);
 phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
-- 
2.30.2

[PATCH v1 02/26] net: dsa: microchip: ksz8: ksz8_fdb_dump: fix port validation and VID information

[Oleksij Rempel]

The ksz8_fdb_dump() has multiple issues:
- struct alu_struct was used only to store MAC address.
- port value was misinterpreted as a bit mask
- Filter ID was attempted to use as VID. FID can be used to get VID, but
  currently it is not supported.
- by the attempt to use FID, was used not initialized alu.fid...

Before this patch, "bridge fdb" command was providing random results,
which are fixed by this patch:
- random vid information
- assignment to wrong port

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 003b0ac2854c..e0530bc3bec0 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -958,15 +958,14 @@ int ksz8_fdb_dump(struct ksz_device *dev, int port,
 	u16 entries = 0;
 	u8 timestamp = 0;
 	u8 fid;
-	u8 member;
-	struct alu_struct alu;
+	u8 src_port;
+	u8 mac[ETH_ALEN];
 
 	do {
-		alu.is_static = false;
-		ret = ksz8_r_dyn_mac_table(dev, i, alu.mac, &fid, &member,
+		ret = ksz8_r_dyn_mac_table(dev, i, mac, &fid, &src_port,
 					   &timestamp, &entries);
-		if (!ret && (member & BIT(port))) {
-			ret = cb(alu.mac, alu.fid, alu.is_static, data);
+		if (!ret && port == src_port) {
+			ret = cb(mac, 0, false, data);
 			if (ret)
 				break;
 		}
-- 
2.30.2

[PATCH v1 03/26] net: dsa: microchip: ksz8: ksz8_fdb_dump: fix not complete fdb extraction

[Oleksij Rempel]

Current ksz8_fdb_dump() is able to extract max 249 entries on
the ksz8863/ksz8873 series of switches. This happened due to wrong
bit mask and offset calculation.

With this patch, we will be able to extract all possible 1024 entries.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 423f944cc34c..43d94131417b 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -398,10 +398,10 @@ static const u32 ksz8863_masks[] = {
 	[STATIC_MAC_TABLE_FID]		= GENMASK(29, 26),
 	[STATIC_MAC_TABLE_OVERRIDE]	= BIT(20),
 	[STATIC_MAC_TABLE_FWD_PORTS]	= GENMASK(18, 16),
-	[DYNAMIC_MAC_TABLE_ENTRIES_H]	= GENMASK(5, 0),
+	[DYNAMIC_MAC_TABLE_ENTRIES_H]	= GENMASK(1, 0),
 	[DYNAMIC_MAC_TABLE_MAC_EMPTY]	= BIT(7),
 	[DYNAMIC_MAC_TABLE_NOT_READY]	= BIT(7),
-	[DYNAMIC_MAC_TABLE_ENTRIES]	= GENMASK(31, 28),
+	[DYNAMIC_MAC_TABLE_ENTRIES]	= GENMASK(31, 24),
 	[DYNAMIC_MAC_TABLE_FID]		= GENMASK(19, 16),
 	[DYNAMIC_MAC_TABLE_SRC_PORT]	= GENMASK(21, 20),
 	[DYNAMIC_MAC_TABLE_TIMESTAMP]	= GENMASK(23, 22),
@@ -411,7 +411,7 @@ static u8 ksz8863_shifts[] = {
 	[VLAN_TABLE_MEMBERSHIP_S]	= 16,
 	[STATIC_MAC_FWD_PORTS]		= 16,
 	[STATIC_MAC_FID]		= 22,
-	[DYNAMIC_MAC_ENTRIES_H]		= 3,
+	[DYNAMIC_MAC_ENTRIES_H]		= 8,
 	[DYNAMIC_MAC_ENTRIES]		= 24,
 	[DYNAMIC_MAC_FID]		= 16,
 	[DYNAMIC_MAC_TIMESTAMP]		= 24,
-- 
2.30.2

[PATCH v1 04/26] net: dsa: microchip: ksz8: ksz8_fdb_dump: fix time stamp extraction

[Oleksij Rempel]

FDB fix time stamp extraction. We are using wrong offset, so we will get
not a time stamp.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 43d94131417b..7c28dc1edaa1 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -414,7 +414,7 @@ static u8 ksz8863_shifts[] = {
 	[DYNAMIC_MAC_ENTRIES_H]		= 8,
 	[DYNAMIC_MAC_ENTRIES]		= 24,
 	[DYNAMIC_MAC_FID]		= 16,
-	[DYNAMIC_MAC_TIMESTAMP]		= 24,
+	[DYNAMIC_MAC_TIMESTAMP]		= 22,
 	[DYNAMIC_MAC_SRC_PORT]		= 20,
 };
 
-- 
2.30.2

[PATCH v1 05/26] net: dsa: microchip: ksz8: ksz8_fdb_dump: do not extract ghost entry from empty table

[Oleksij Rempel]

If the dynamic MAC table is empty, we will still extract one outdated
entry. Fix it by using correct bit offset.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7c28dc1edaa1..afb846c18b57 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -399,7 +399,7 @@ static const u32 ksz8863_masks[] = {
 	[STATIC_MAC_TABLE_OVERRIDE]	= BIT(20),
 	[STATIC_MAC_TABLE_FWD_PORTS]	= GENMASK(18, 16),
 	[DYNAMIC_MAC_TABLE_ENTRIES_H]	= GENMASK(1, 0),
-	[DYNAMIC_MAC_TABLE_MAC_EMPTY]	= BIT(7),
+	[DYNAMIC_MAC_TABLE_MAC_EMPTY]	= BIT(2),
 	[DYNAMIC_MAC_TABLE_NOT_READY]	= BIT(7),
 	[DYNAMIC_MAC_TABLE_ENTRIES]	= GENMASK(31, 24),
 	[DYNAMIC_MAC_TABLE_FID]		= GENMASK(19, 16),
-- 
2.30.2

[PATCH v1 06/26] net: dsa: microchip: ksz8863_smi: fix bulk access

[Oleksij Rempel]

Current regmap bulk access is broken, resulting to wrong reads/writes
if ksz_read64/ksz_write64 functions are used.
Mostly this issue was visible by using ksz8_fdb_dump(), which returned
corrupt MAC address.

The reason is that regmap was configured to have max_raw_read/write,
even if ksz8863_mdio_read/write functions are able to handle unlimited
read/write accesses. On ksz_read64 function we are using multiple 32bit
accesses by incrementing each access by 1 instead of 4. Resulting buffer
had 01234567.12345678 instead of 01234567.89abcdef.

We have multiple ways to fix it:
- enable 4 byte alignment for 32bit accesses. Since the HW do not have
  this requirement. It will break driver.
- disable max_raw_* limit.

This patch is removing max_raw_* limit for regmap accesses in ksz8863_smi.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8863_smi.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c
index 2f4623f3bd85..2516c9db7fec 100644
--- a/drivers/net/dsa/microchip/ksz8863_smi.c
+++ b/drivers/net/dsa/microchip/ksz8863_smi.c
@@ -70,6 +70,7 @@ static int ksz8863_mdio_write(void *ctx, const void *data, size_t count)
 				      tmp, val[i]);
 		if (ret < 0)
 			goto out;
+
 	}
 
  out:
@@ -82,22 +83,16 @@ static const struct regmap_bus regmap_smi[] = {
 	{
 		.read = ksz8863_mdio_read,
 		.write = ksz8863_mdio_write,
-		.max_raw_read = 1,
-		.max_raw_write = 1,
 	},
 	{
 		.read = ksz8863_mdio_read,
 		.write = ksz8863_mdio_write,
 		.val_format_endian_default = REGMAP_ENDIAN_BIG,
-		.max_raw_read = 2,
-		.max_raw_write = 2,
 	},
 	{
 		.read = ksz8863_mdio_read,
 		.write = ksz8863_mdio_write,
 		.val_format_endian_default = REGMAP_ENDIAN_BIG,
-		.max_raw_read = 4,
-		.max_raw_write = 4,
 	}
 };
 
@@ -108,7 +103,6 @@ static const struct regmap_config ksz8863_regmap_config[] = {
 		.pad_bits = 24,
 		.val_bits = 8,
 		.cache_type = REGCACHE_NONE,
-		.use_single_read = 1,
 		.lock = ksz_regmap_lock,
 		.unlock = ksz_regmap_unlock,
 	},
@@ -118,7 +112,6 @@ static const struct regmap_config ksz8863_regmap_config[] = {
 		.pad_bits = 24,
 		.val_bits = 16,
 		.cache_type = REGCACHE_NONE,
-		.use_single_read = 1,
 		.lock = ksz_regmap_lock,
 		.unlock = ksz_regmap_unlock,
 	},
@@ -128,7 +121,6 @@ static const struct regmap_config ksz8863_regmap_config[] = {
 		.pad_bits = 24,
 		.val_bits = 32,
 		.cache_type = REGCACHE_NONE,
-		.use_single_read = 1,
 		.lock = ksz_regmap_lock,
 		.unlock = ksz_regmap_unlock,
 	}
-- 
2.30.2

[PATCH v1 07/26] net: dsa: microchip: ksz8_r_dyn_mac_table(): remove timestamp support

[Oleksij Rempel]

We do not use FDB timestamps. So, drop it.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8.h    | 2 +-
 drivers/net/dsa/microchip/ksz8795.c | 7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index ea05abfbd51d..ad8e69051347 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -20,7 +20,7 @@ void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port);
 int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
 int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
-			 u8 *fid, u8 *src_port, u8 *timestamp, u16 *entries);
+			 u8 *fid, u8 *src_port, u16 *entries);
 int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 			 struct alu_struct *alu);
 void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index e0530bc3bec0..d0cfe74d5b13 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -395,7 +395,7 @@ static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
 }
 
 int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
-			 u8 *fid, u8 *src_port, u8 *timestamp, u16 *entries)
+			 u8 *fid, u8 *src_port, u16 *entries)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
@@ -440,8 +440,6 @@ int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 			shifts[DYNAMIC_MAC_FID];
 		*src_port = (data_hi & masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
 			shifts[DYNAMIC_MAC_SRC_PORT];
-		*timestamp = (data_hi & masks[DYNAMIC_MAC_TABLE_TIMESTAMP]) >>
-			shifts[DYNAMIC_MAC_TIMESTAMP];
 
 		mac_addr[5] = (u8)data_lo;
 		mac_addr[4] = (u8)(data_lo >> 8);
@@ -956,14 +954,13 @@ int ksz8_fdb_dump(struct ksz_device *dev, int port,
 	int ret = 0;
 	u16 i = 0;
 	u16 entries = 0;
-	u8 timestamp = 0;
 	u8 fid;
 	u8 src_port;
 	u8 mac[ETH_ALEN];
 
 	do {
 		ret = ksz8_r_dyn_mac_table(dev, i, mac, &fid, &src_port,
-					   &timestamp, &entries);
+					   &entries);
 		if (!ret && port == src_port) {
 			ret = cb(mac, 0, false, data);
 			if (ret)
-- 
2.30.2

Re: [PATCH v1 07/26] net: dsa: microchip: ksz8_r_dyn_mac_table(): remove timestamp support

[Arun.Ramadoss]

Hi Oleksij,

On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> We do not use FDB timestamps. So, drop it.

If the FDB timestamps are not used/to be dropped, then do we need to
have patch 4  * net: dsa: microchip: ksz8: ksz8_fdb_dump: fix time
stamp extraction * which is to fix the offset.

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8.h    | 2 +-
>  drivers/net/dsa/microchip/ksz8795.c | 7 ++-----
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index e0530bc3bec0..d0cfe74d5b13 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -395,7 +395,7 @@ static int ksz8_valid_dyn_entry(struct ksz_device
> *dev, u8 *data)
>  }
> 
>  int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8
> *mac_addr,
> -                        u8 *fid, u8 *src_port, u8 *timestamp, u16
> *entries)
> +                        u8 *fid, u8 *src_port, u16 *entries)
>  {
>         u32 data_hi, data_lo;
>         const u8 *shifts;
> @@ -440,8 +440,6 @@ int ksz8_r_dyn_mac_table(struct ksz_device *dev,
> u16 addr, u8 *mac_addr,
>                         shifts[DYNAMIC_MAC_FID];
>                 *src_port = (data_hi &
> masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
>                         shifts[DYNAMIC_MAC_SRC_PORT];
> -               *timestamp = (data_hi &
> masks[DYNAMIC_MAC_TABLE_TIMESTAMP]) >>
> -                       shifts[DYNAMIC_MAC_TIMESTAMP];

The macros DYNAMIC_MAC_TABLE_TIMESTAMP and DYNAMIC_MAC_TIMESTAMP needs
to be deleted in ksz8795_shifts/masks and ksz8863_shifts/masks
structure in ksz_common.c/h files since it is only in this function.

> 
>                 mac_addr[5] = (u8)data_lo;
>                 mac_addr[4] = (u8)(data_lo >> 8);
> @@ -956,14 +954,13 @@ int ksz8_fdb_dump(struct ksz_device *dev, int
> port,
> 
> --
> 2.30.2
> 

[PATCH v1 08/26] net: dsa: microchip: make ksz8_r_dyn_mac_table() static

[Oleksij Rempel]

ksz8_r_dyn_mac_table() is not used any where outside. So, make it
static.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8.h    | 2 --
 drivers/net/dsa/microchip/ksz8795.c | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index ad8e69051347..96d453af17c5 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -19,8 +19,6 @@ void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port);
 void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port);
 int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
-int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
-			 u8 *fid, u8 *src_port, u16 *entries);
 int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 			 struct alu_struct *alu);
 void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index d0cfe74d5b13..7310abd0d8df 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -394,8 +394,8 @@ static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
 	return 0;
 }
 
-int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
-			 u8 *fid, u8 *src_port, u16 *entries)
+static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
+				u8 *fid, u8 *src_port, u16 *entries)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
-- 
2.30.2

[PATCH v1 09/26] net: dsa: microchip: ksz8_r_dyn_mac_table(): remove fid support

[Oleksij Rempel]

FID != VLAN. FID to VLAN conversation is currently not validated and was
not working before within the FDB dump context. So, drop it until
we have time to implement this functionality.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 7310abd0d8df..736cf4e54333 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -395,7 +395,7 @@ static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
 }
 
 static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
-				u8 *fid, u8 *src_port, u16 *entries)
+				u8 *src_port, u16 *entries)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
@@ -436,8 +436,6 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 			shifts[DYNAMIC_MAC_ENTRIES];
 		*entries = cnt + 1;
 
-		*fid = (data_hi & masks[DYNAMIC_MAC_TABLE_FID]) >>
-			shifts[DYNAMIC_MAC_FID];
 		*src_port = (data_hi & masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
 			shifts[DYNAMIC_MAC_SRC_PORT];
 
@@ -954,12 +952,11 @@ int ksz8_fdb_dump(struct ksz_device *dev, int port,
 	int ret = 0;
 	u16 i = 0;
 	u16 entries = 0;
-	u8 fid;
 	u8 src_port;
 	u8 mac[ETH_ALEN];
 
 	do {
-		ret = ksz8_r_dyn_mac_table(dev, i, mac, &fid, &src_port,
+		ret = ksz8_r_dyn_mac_table(dev, i, mac, &src_port,
 					   &entries);
 		if (!ret && port == src_port) {
 			ret = cb(mac, 0, false, data);
-- 
2.30.2

[PATCH v1 10/26] net: dsa: microchip: ksz8: refactor ksz8_fdb_dump()

[Oleksij Rempel]

After fixing different bugs we can refactor this function:
- be paranoid - read only max possibly amount of entries supported by
  the HW.
- pass error values returned by regmap

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 41 ++++++++++++++-----------
 drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 736cf4e54333..308b46bb2ce5 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -949,26 +949,31 @@ void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port)
 int ksz8_fdb_dump(struct ksz_device *dev, int port,
 		  dsa_fdb_dump_cb_t *cb, void *data)
 {
-	int ret = 0;
-	u16 i = 0;
-	u16 entries = 0;
-	u8 src_port;
-	u8 mac[ETH_ALEN];
+	u16 i, entries = 0;
+	int ret;
 
-	do {
-		ret = ksz8_r_dyn_mac_table(dev, i, mac, &src_port,
-					   &entries);
-		if (!ret && port == src_port) {
-			ret = cb(mac, 0, false, data);
-			if (ret)
-				break;
-		}
-		i++;
-	} while (i < entries);
-	if (i >= entries)
-		ret = 0;
+	for (i = 0; i < KSZ8_DYN_MAC_ENTRIES; i++) {
+		u8 mac[ETH_ALEN];
+		u8 src_port;
 
-	return ret;
+		ret = ksz8_r_dyn_mac_table(dev, i, mac, &src_port, &entries);
+		if (ret == -ENXIO)
+			return 0;
+		if (ret)
+			return ret;
+
+		if (i >= entries)
+			return 0;
+
+		if (port != src_port)
+			continue;
+
+		ret = cb(mac, 0, false, data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 int ksz8_mdb_add(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 7a57c6088f80..0bdceb534192 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -811,5 +811,6 @@
 #define TAIL_TAG_LOOKUP			BIT(7)
 
 #define FID_ENTRIES			128
+#define KSZ8_DYN_MAC_ENTRIES		1024
 
 #endif
-- 
2.30.2

Re: [PATCH v1 10/26] net: dsa: microchip: ksz8: refactor ksz8_fdb_dump()

[Arun.Ramadoss]

Hi Oleksij,

On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> After fixing different bugs we can refactor this function:
> - be paranoid - read only max possibly amount of entries supported by
>   the HW.
> - pass error values returned by regmap
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c     | 41 ++++++++++++++---------
> --
>  drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
>  2 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index 736cf4e54333..308b46bb2ce5 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -949,26 +949,31 @@ void ksz8_flush_dyn_mac_table(struct ksz_device
> *dev, int port)
>  int ksz8_fdb_dump(struct ksz_device *dev, int port,
>                   dsa_fdb_dump_cb_t *cb, void *data)
>  {
> -       int ret = 0;
> -       u16 i = 0;
> -       u16 entries = 0;
> -       u8 src_port;
> -       u8 mac[ETH_ALEN];
> +       u16 i, entries = 0;
> +       int ret;
> 
> -       do {
> -               ret = ksz8_r_dyn_mac_table(dev, i, mac, &src_port,
> -                                          &entries);
> -               if (!ret && port == src_port) {
> -                       ret = cb(mac, 0, false, data);
> -                       if (ret)
> -                               break;
> -               }
> -               i++;
> -       } while (i < entries);
> -       if (i >= entries)
> -               ret = 0;
> +       for (i = 0; i < KSZ8_DYN_MAC_ENTRIES; i++) {
> +               u8 mac[ETH_ALEN];
> +               u8 src_port;

Any specific reason for declaring variable within for loop instead of
outside.

> 
> -       return ret;
> +               ret = ksz8_r_dyn_mac_table(dev, i, mac, &src_port,
> &entries);
> +               if (ret == -ENXIO)
> +                       return 0;
> +               if (ret)
> +                       return ret;
> +
> +               if (i >= entries)
> +                       return 0;
> +
> +               if (port != src_port)
> +                       continue;
> +
> +               ret = cb(mac, 0, false, data);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
>  }
> 
>  int ksz8_mdb_add(struct ksz_device *dev, int port,
> diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h
> b/drivers/net/dsa/microchip/ksz8795_reg.h
> index 7a57c6088f80..0bdceb534192 100644
> --- a/drivers/net/dsa/microchip/ksz8795_reg.h
> +++ b/drivers/net/dsa/microchip/ksz8795_reg.h
> @@ -811,5 +811,6 @@
>  #define TAIL_TAG_LOOKUP                        BIT(7)
> 
>  #define FID_ENTRIES                    128
> +#define KSZ8_DYN_MAC_ENTRIES           1024
> 
>  #endif
> --
> 2.30.2
> 

Re: [PATCH v1 10/26] net: dsa: microchip: ksz8: refactor ksz8_fdb_dump()

[Oleksij Rempel]

On Tue, Nov 29, 2022 at 08:29:47AM +0000, Arun.Ramadoss@microchip.com wrote:
> Hi Oleksij,
> 
> On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > After fixing different bugs we can refactor this function:
> > - be paranoid - read only max possibly amount of entries supported by
> >   the HW.
> > - pass error values returned by regmap
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/ksz8795.c     | 41 ++++++++++++++---------
> > --
> >  drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
> >  2 files changed, 24 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> > b/drivers/net/dsa/microchip/ksz8795.c
> > index 736cf4e54333..308b46bb2ce5 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -949,26 +949,31 @@ void ksz8_flush_dyn_mac_table(struct ksz_device
> > *dev, int port)
> >  int ksz8_fdb_dump(struct ksz_device *dev, int port,
> >                   dsa_fdb_dump_cb_t *cb, void *data)
> >  {
> > -       int ret = 0;
> > -       u16 i = 0;
> > -       u16 entries = 0;
> > -       u8 src_port;
> > -       u8 mac[ETH_ALEN];
> > +       u16 i, entries = 0;
> > +       int ret;
> > 
> > -       do {
> > -               ret = ksz8_r_dyn_mac_table(dev, i, mac, &src_port,
> > -                                          &entries);
> > -               if (!ret && port == src_port) {
> > -                       ret = cb(mac, 0, false, data);
> > -                       if (ret)
> > -                               break;
> > -               }
> > -               i++;
> > -       } while (i < entries);
> > -       if (i >= entries)
> > -               ret = 0;
> > +       for (i = 0; i < KSZ8_DYN_MAC_ENTRIES; i++) {
> > +               u8 mac[ETH_ALEN];
> > +               u8 src_port;
> 
> Any specific reason for declaring variable within for loop instead of
> outside.

No. It is personal preference to declare variables within the scope where
variable is used.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v1 11/26] net: dsa: microchip: ksz8: ksz8_fdb_dump: dump static MAC table

[Oleksij Rempel]

Extend fdb_dump with static MAC table dump.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 308b46bb2ce5..392f6cb1f706 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -952,6 +952,23 @@ int ksz8_fdb_dump(struct ksz_device *dev, int port,
 	u16 i, entries = 0;
 	int ret;
 
+	for (i = 0; i  < dev->info->num_statics; i++) {
+		struct alu_struct alu;
+
+		ret = ksz8_r_sta_mac_table(dev, i, &alu);
+		if (ret == -ENXIO)
+			continue;
+		if (ret)
+			return ret;
+
+		if (!(alu.port_forward & BIT(port)))
+			continue;
+
+		ret = cb(alu.mac, 0, true, data);
+		if (ret)
+			return ret;
+	}
+
 	for (i = 0; i < KSZ8_DYN_MAC_ENTRIES; i++) {
 		u8 mac[ETH_ALEN];
 		u8 src_port;
-- 
2.30.2

[PATCH v1 12/26] net: dsa: microchip: ksz8: move static mac table operations to a separate functions

[Oleksij Rempel]

We need it to reuse the code for add/del_fdb

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 34 +++++++++++++++++++----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 392f6cb1f706..d16dc8e5ed18 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -993,8 +993,8 @@ int ksz8_fdb_dump(struct ksz_device *dev, int port,
 	return 0;
 }
 
-int ksz8_mdb_add(struct ksz_device *dev, int port,
-		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db)
+static int ksz8_add_sta_mac(struct ksz_device *dev, int port,
+			    const unsigned char *addr, u16 vid)
 {
 	struct alu_struct alu;
 	int index;
@@ -1004,8 +1004,8 @@ int ksz8_mdb_add(struct ksz_device *dev, int port,
 	for (index = 0; index < dev->info->num_statics; index++) {
 		if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
 			/* Found one already in static MAC table. */
-			if (!memcmp(alu.mac, mdb->addr, ETH_ALEN) &&
-			    alu.fid == mdb->vid)
+			if (!memcmp(alu.mac, addr, ETH_ALEN) &&
+			    alu.fid == vid)
 				break;
 		/* Remember the first empty entry. */
 		} else if (!empty) {
@@ -1021,23 +1021,23 @@ int ksz8_mdb_add(struct ksz_device *dev, int port,
 	if (index == dev->info->num_statics) {
 		index = empty - 1;
 		memset(&alu, 0, sizeof(alu));
-		memcpy(alu.mac, mdb->addr, ETH_ALEN);
+		memcpy(alu.mac, addr, ETH_ALEN);
 		alu.is_static = true;
 	}
 	alu.port_forward |= BIT(port);
-	if (mdb->vid) {
+	if (vid) {
 		alu.is_use_fid = true;
 
 		/* Need a way to map VID to FID. */
-		alu.fid = mdb->vid;
+		alu.fid = vid;
 	}
 	ksz8_w_sta_mac_table(dev, index, &alu);
 
 	return 0;
 }
 
-int ksz8_mdb_del(struct ksz_device *dev, int port,
-		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db)
+static int ksz8_del_sta_mac(struct ksz_device *dev, int port,
+			    const unsigned char *addr, u16 vid)
 {
 	struct alu_struct alu;
 	int index;
@@ -1045,8 +1045,8 @@ int ksz8_mdb_del(struct ksz_device *dev, int port,
 	for (index = 0; index < dev->info->num_statics; index++) {
 		if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
 			/* Found one already in static MAC table. */
-			if (!memcmp(alu.mac, mdb->addr, ETH_ALEN) &&
-			    alu.fid == mdb->vid)
+			if (!memcmp(alu.mac, addr, ETH_ALEN) &&
+			    alu.fid == vid)
 				break;
 		}
 	}
@@ -1065,6 +1065,18 @@ int ksz8_mdb_del(struct ksz_device *dev, int port,
 	return 0;
 }
 
+int ksz8_mdb_add(struct ksz_device *dev, int port,
+		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db)
+{
+	return ksz8_add_sta_mac(dev, port, mdb->addr, mdb->vid);
+}
+
+int ksz8_mdb_del(struct ksz_device *dev, int port,
+		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db)
+{
+	return ksz8_del_sta_mac(dev, port, mdb->addr, mdb->vid);
+}
+
 int ksz8_port_vlan_filtering(struct ksz_device *dev, int port, bool flag,
 			     struct netlink_ext_ack *extack)
 {
-- 
2.30.2

[PATCH v1 13/26] net: dsa: microchip: ksz8: add fdb_add/del support

[Oleksij Rempel]

Provide access to the static MAC table over fdb_add/del callbacks.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8.h       |  4 ++++
 drivers/net/dsa/microchip/ksz8795.c    | 12 ++++++++++++
 drivers/net/dsa/microchip/ksz_common.c |  2 ++
 3 files changed, 18 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index 96d453af17c5..a28fa7cd4d98 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -30,6 +30,10 @@ void ksz8_freeze_mib(struct ksz_device *dev, int port, bool freeze);
 void ksz8_port_init_cnt(struct ksz_device *dev, int port);
 int ksz8_fdb_dump(struct ksz_device *dev, int port,
 		  dsa_fdb_dump_cb_t *cb, void *data);
+int ksz8_fdb_add(struct ksz_device *dev, int port, const unsigned char *addr,
+		 u16 vid, struct dsa_db db);
+int ksz8_fdb_del(struct ksz_device *dev, int port, const unsigned char *addr,
+		 u16 vid, struct dsa_db db);
 int ksz8_mdb_add(struct ksz_device *dev, int port,
 		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db);
 int ksz8_mdb_del(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index d16dc8e5ed18..208cf4dde397 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1077,6 +1077,18 @@ int ksz8_mdb_del(struct ksz_device *dev, int port,
 	return ksz8_del_sta_mac(dev, port, mdb->addr, mdb->vid);
 }
 
+int ksz8_fdb_add(struct ksz_device *dev, int port, const unsigned char *addr,
+		 u16 vid, struct dsa_db db)
+{
+	return ksz8_add_sta_mac(dev, port, addr, vid);
+}
+
+int ksz8_fdb_del(struct ksz_device *dev, int port, const unsigned char *addr,
+		 u16 vid, struct dsa_db db)
+{
+	return ksz8_del_sta_mac(dev, port, addr, vid);
+}
+
 int ksz8_port_vlan_filtering(struct ksz_device *dev, int port, bool flag,
 			     struct netlink_ext_ack *extack)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index afb846c18b57..171cb0063fbf 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -197,6 +197,8 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
 	.freeze_mib = ksz8_freeze_mib,
 	.port_init_cnt = ksz8_port_init_cnt,
 	.fdb_dump = ksz8_fdb_dump,
+	.fdb_add = ksz8_fdb_add,
+	.fdb_del = ksz8_fdb_del,
 	.mdb_add = ksz8_mdb_add,
 	.mdb_del = ksz8_mdb_del,
 	.vlan_filtering = ksz8_port_vlan_filtering,
-- 
2.30.2

[PATCH v1 14/26] net: dsa: microchip: KSZ88x3 fix loopback support

[Oleksij Rempel]

With current code loopback is not working and selftest will always fail.
Fix register and bit offsets to make loopback on KSZ88x3 switches.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 24 ++++++++++++++++++------
 drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 208cf4dde397..a6d5de41a754 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -625,8 +625,13 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 		if (ret)
 			return ret;
 
-		if (restart & PORT_PHY_LOOPBACK)
-			data |= BMCR_LOOPBACK;
+		if (ksz_is_ksz88x3(dev)) {
+			if (restart & KSZ8873_PORT_PHY_LOOPBACK)
+				data |= BMCR_LOOPBACK;
+		} else {
+			if (restart & PORT_PHY_LOOPBACK)
+				data |= BMCR_LOOPBACK;
+		}
 		if (ctrl & PORT_FORCE_100_MBIT)
 			data |= BMCR_SPEED100;
 		if (ksz_is_ksz88x3(dev)) {
@@ -849,10 +854,17 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 			data |= PORT_FORCE_MDIX;
 		else
 			data &= ~PORT_FORCE_MDIX;
-		if (val & BMCR_LOOPBACK)
-			data |= PORT_PHY_LOOPBACK;
-		else
-			data &= ~PORT_PHY_LOOPBACK;
+		if (ksz_is_ksz88x3(dev)) {
+			if (val & BMCR_LOOPBACK)
+				data |= KSZ8873_PORT_PHY_LOOPBACK;
+			else
+				data &= ~KSZ8873_PORT_PHY_LOOPBACK;
+		} else {
+			if (val & BMCR_LOOPBACK)
+				data |= PORT_PHY_LOOPBACK;
+			else
+				data &= ~PORT_PHY_LOOPBACK;
+		}
 
 		if (data != restart) {
 			ret = ksz_pwrite8(dev, p, regs[P_NEG_RESTART_CTRL],
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 0bdceb534192..08204da7d621 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -262,6 +262,7 @@
 #define PORT_AUTO_MDIX_DISABLE		BIT(2)
 #define PORT_FORCE_MDIX			BIT(1)
 #define PORT_MAC_LOOPBACK		BIT(0)
+#define KSZ8873_PORT_PHY_LOOPBACK	BIT(0)
 
 #define REG_PORT_1_STATUS_2		0x1E
 #define REG_PORT_2_STATUS_2		0x2E
-- 
2.30.2

Re: [PATCH v1 14/26] net: dsa: microchip: KSZ88x3 fix loopback support

[Arun.Ramadoss]

Hi Oleksij,

On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> With current code loopback is not working and selftest will always
> fail.
> Fix register and bit offsets to make loopback on KSZ88x3 switches.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c     | 24 ++++++++++++++++++-----
> -
>  drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index 208cf4dde397..a6d5de41a754 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -625,8 +625,13 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy,
> u16 reg, u16 *val)
>                 if (ret)
>                         return ret;
> 
> -               if (restart & PORT_PHY_LOOPBACK)
> -                       data |= BMCR_LOOPBACK;
> +               if (ksz_is_ksz88x3(dev)) {
> +                       if (restart & KSZ8873_PORT_PHY_LOOPBACK)
> +                               data |= BMCR_LOOPBACK;
> +               } else {
> +                       if (restart & PORT_PHY_LOOPBACK)
> +                               data |= BMCR_LOOPBACK;
> +               }

Can you consider using ksz8795_masks[] and ksz8863_masks[] to check the
loopback. Like if (restart & mask[PHY_LOOPBAK)) to avoid two checks.

>                 if (ctrl & PORT_FORCE_100_MBIT)
>                         data |= BMCR_SPEED100;
>                 if (ksz_is_ksz88x3(dev)) {
> diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h
> b/drivers/net/dsa/microchip/ksz8795_reg.h
> index 0bdceb534192..08204da7d621 100644
> --- a/drivers/net/dsa/microchip/ksz8795_reg.h
> +++ b/drivers/net/dsa/microchip/ksz8795_reg.h
> @@ -262,6 +262,7 @@
>  #define PORT_AUTO_MDIX_DISABLE         BIT(2)
>  #define PORT_FORCE_MDIX                        BIT(1)
>  #define PORT_MAC_LOOPBACK              BIT(0)
> +#define KSZ8873_PORT_PHY_LOOPBACK      BIT(0)
> 
>  #define REG_PORT_1_STATUS_2            0x1E
>  #define REG_PORT_2_STATUS_2            0x2E
> --
> 2.30.2
> 

[PATCH v1 15/26] net: dsa: microchip: ksz8_r_dyn_mac_table(): move main part of the code out of if statement

[Oleksij Rempel]

Start reworking error handling of this function

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 56 +++++++++++++++--------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index a6d5de41a754..2674fd553851 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -402,7 +402,9 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	const u32 *masks;
 	const u16 *regs;
 	u16 ctrl_addr;
+	u64 buf = 0;
 	u8 data;
+	int cnt;
 	int rc;
 
 	shifts = dev->info->shifts;
@@ -418,36 +420,36 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	if (rc == -EAGAIN) {
 		if (addr == 0)
 			*entries = 0;
+		goto unlock_alu;
 	} else if (rc == -ENXIO) {
 		*entries = 0;
-	/* At least one valid entry in the table. */
-	} else {
-		u64 buf = 0;
-		int cnt;
-
-		ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
-		data_hi = (u32)(buf >> 32);
-		data_lo = (u32)buf;
-
-		/* Check out how many valid entry in the table. */
-		cnt = data & masks[DYNAMIC_MAC_TABLE_ENTRIES_H];
-		cnt <<= shifts[DYNAMIC_MAC_ENTRIES_H];
-		cnt |= (data_hi & masks[DYNAMIC_MAC_TABLE_ENTRIES]) >>
-			shifts[DYNAMIC_MAC_ENTRIES];
-		*entries = cnt + 1;
-
-		*src_port = (data_hi & masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
-			shifts[DYNAMIC_MAC_SRC_PORT];
-
-		mac_addr[5] = (u8)data_lo;
-		mac_addr[4] = (u8)(data_lo >> 8);
-		mac_addr[3] = (u8)(data_lo >> 16);
-		mac_addr[2] = (u8)(data_lo >> 24);
-
-		mac_addr[1] = (u8)data_hi;
-		mac_addr[0] = (u8)(data_hi >> 8);
-		rc = 0;
+		goto unlock_alu;
 	}
+
+	ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
+	data_hi = (u32)(buf >> 32);
+	data_lo = (u32)buf;
+
+	/* Check out how many valid entry in the table. */
+	cnt = data & masks[DYNAMIC_MAC_TABLE_ENTRIES_H];
+	cnt <<= shifts[DYNAMIC_MAC_ENTRIES_H];
+	cnt |= (data_hi & masks[DYNAMIC_MAC_TABLE_ENTRIES]) >>
+		shifts[DYNAMIC_MAC_ENTRIES];
+	*entries = cnt + 1;
+
+	*src_port = (data_hi & masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
+		shifts[DYNAMIC_MAC_SRC_PORT];
+
+	mac_addr[5] = (u8)data_lo;
+	mac_addr[4] = (u8)(data_lo >> 8);
+	mac_addr[3] = (u8)(data_lo >> 16);
+	mac_addr[2] = (u8)(data_lo >> 24);
+
+	mac_addr[1] = (u8)data_hi;
+	mac_addr[0] = (u8)(data_hi >> 8);
+	rc = 0;
+
+unlock_alu:
 	mutex_unlock(&dev->alu_mutex);
 
 	return rc;
-- 
2.30.2

[PATCH v1 16/26] net: dsa: microchip: ksz8_r_dyn_mac_table(): use ret instead of rc

[Oleksij Rempel]

Other parts of this file use ret. So, unify it.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 2674fd553851..1677f61b138a 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -405,7 +405,7 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	u64 buf = 0;
 	u8 data;
 	int cnt;
-	int rc;
+	int ret;
 
 	shifts = dev->info->shifts;
 	masks = dev->info->masks;
@@ -416,12 +416,12 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	mutex_lock(&dev->alu_mutex);
 	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
 
-	rc = ksz8_valid_dyn_entry(dev, &data);
-	if (rc == -EAGAIN) {
+	ret = ksz8_valid_dyn_entry(dev, &data);
+	if (ret == -EAGAIN) {
 		if (addr == 0)
 			*entries = 0;
 		goto unlock_alu;
-	} else if (rc == -ENXIO) {
+	} else if (ret == -ENXIO) {
 		*entries = 0;
 		goto unlock_alu;
 	}
@@ -447,12 +447,12 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 
 	mac_addr[1] = (u8)data_hi;
 	mac_addr[0] = (u8)(data_hi >> 8);
-	rc = 0;
+	ret = 0;
 
 unlock_alu:
 	mutex_unlock(&dev->alu_mutex);
 
-	return rc;
+	return ret;
 }
 
 int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
-- 
2.30.2

[PATCH v1 17/26] net: dsa: microchip: ksz8_r_dyn_mac_table(): ksz: do not return EAGAIN on timeout

[Oleksij Rempel]

EAGAIN was not used by previous code and not used by  current code. So,
remove it and use proper error value.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 1677f61b138a..1b067e0cacd5 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -382,7 +382,7 @@ static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
 
 	/* Entry is not ready for accessing. */
 	if (*data & masks[DYNAMIC_MAC_TABLE_NOT_READY]) {
-		return -EAGAIN;
+		return -ETIMEDOUT;
 	/* Entry is ready for accessing. */
 	} else {
 		ksz_read8(dev, regs[REG_IND_DATA_8], data);
@@ -417,15 +417,14 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
 
 	ret = ksz8_valid_dyn_entry(dev, &data);
-	if (ret == -EAGAIN) {
-		if (addr == 0)
-			*entries = 0;
-		goto unlock_alu;
-	} else if (ret == -ENXIO) {
+	if (ret == -ENXIO) {
 		*entries = 0;
 		goto unlock_alu;
 	}
 
+	if (ret)
+		goto unlock_alu;
+
 	ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
 	data_hi = (u32)(buf >> 32);
 	data_lo = (u32)buf;
-- 
2.30.2

[PATCH v1 18/26] net: dsa: microchip: ksz8_r_dyn_mac_table(): return read/write error if we got any

[Oleksij Rempel]

The read/write path may fail. So, return error if we got it.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 1b067e0cacd5..3348f5c25942 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -371,12 +371,16 @@ static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
 	int timeout = 100;
 	const u32 *masks;
 	const u16 *regs;
+	int ret;
 
 	masks = dev->info->masks;
 	regs = dev->info->regs;
 
 	do {
-		ksz_read8(dev, regs[REG_IND_DATA_CHECK], data);
+		ret = ksz_read8(dev, regs[REG_IND_DATA_CHECK], data);
+		if (ret)
+			return ret;
+
 		timeout--;
 	} while ((*data & masks[DYNAMIC_MAC_TABLE_NOT_READY]) && timeout);
 
@@ -385,7 +389,9 @@ static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
 		return -ETIMEDOUT;
 	/* Entry is ready for accessing. */
 	} else {
-		ksz_read8(dev, regs[REG_IND_DATA_8], data);
+		ret = ksz_read8(dev, regs[REG_IND_DATA_8], data);
+		if (ret)
+			return ret;
 
 		/* There is no valid entry in the table. */
 		if (*data & masks[DYNAMIC_MAC_TABLE_MAC_EMPTY])
@@ -414,7 +420,9 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	ctrl_addr = IND_ACC_TABLE(TABLE_DYNAMIC_MAC | TABLE_READ) | addr;
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ret = ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	if (ret)
+		goto unlock_alu;
 
 	ret = ksz8_valid_dyn_entry(dev, &data);
 	if (ret == -ENXIO) {
@@ -425,7 +433,10 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	if (ret)
 		goto unlock_alu;
 
-	ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
+	ret = ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
+	if (ret)
+		goto unlock_alu;
+
 	data_hi = (u32)(buf >> 32);
 	data_lo = (u32)buf;
 
@@ -446,7 +457,6 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 
 	mac_addr[1] = (u8)data_hi;
 	mac_addr[0] = (u8)(data_hi >> 8);
-	ret = 0;
 
 unlock_alu:
 	mutex_unlock(&dev->alu_mutex);
-- 
2.30.2

[PATCH v1 19/26] net: dsa: microchip: ksz8_r_dyn_mac_table(): use entries variable to signal 0 entries

[Oleksij Rempel]

We already have a variable to provide number of entries. So use it,
instead of using error number.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 3348f5c25942..31c77e086a9d 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -385,19 +385,11 @@ static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
 	} while ((*data & masks[DYNAMIC_MAC_TABLE_NOT_READY]) && timeout);
 
 	/* Entry is not ready for accessing. */
-	if (*data & masks[DYNAMIC_MAC_TABLE_NOT_READY]) {
+	if (*data & masks[DYNAMIC_MAC_TABLE_NOT_READY])
 		return -ETIMEDOUT;
-	/* Entry is ready for accessing. */
-	} else {
-		ret = ksz_read8(dev, regs[REG_IND_DATA_8], data);
-		if (ret)
-			return ret;
 
-		/* There is no valid entry in the table. */
-		if (*data & masks[DYNAMIC_MAC_TABLE_MAC_EMPTY])
-			return -ENXIO;
-	}
-	return 0;
+	/* Entry is ready for accessing. */
+	return ksz_read8(dev, regs[REG_IND_DATA_8], data);
 }
 
 static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
@@ -425,13 +417,13 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 		goto unlock_alu;
 
 	ret = ksz8_valid_dyn_entry(dev, &data);
-	if (ret == -ENXIO) {
-		*entries = 0;
+	if (ret)
 		goto unlock_alu;
-	}
 
-	if (ret)
+	if (data & masks[DYNAMIC_MAC_TABLE_MAC_EMPTY]) {
+		*entries = 0;
 		goto unlock_alu;
+	}
 
 	ret = ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
 	if (ret)
@@ -997,8 +989,6 @@ int ksz8_fdb_dump(struct ksz_device *dev, int port,
 		u8 src_port;
 
 		ret = ksz8_r_dyn_mac_table(dev, i, mac, &src_port, &entries);
-		if (ret == -ENXIO)
-			return 0;
 		if (ret)
 			return ret;
 
-- 
2.30.2

[PATCH v1 20/26] net: dsa: microchip: make ksz8_r_sta_mac_table() static

[Oleksij Rempel]

It is used only in ksz8795.c, no need to export it.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8.h    | 2 --
 drivers/net/dsa/microchip/ksz8795.c | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index a28fa7cd4d98..ed72ec626593 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -19,8 +19,6 @@ void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port);
 void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port);
 int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
-int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
-			 struct alu_struct *alu);
 void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
 			  struct alu_struct *alu);
 void ksz8_r_mib_cnt(struct ksz_device *dev, int port, u16 addr, u64 *cnt);
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 31c77e086a9d..1c08103c9f50 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -456,8 +456,8 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	return ret;
 }
 
-int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
-			 struct alu_struct *alu)
+static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
+				struct alu_struct *alu)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
-- 
2.30.2

Re: [PATCH v1 20/26] net: dsa: microchip: make ksz8_r_sta_mac_table() static

[Arun.Ramadoss]

Hi Oleksij,

On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> It is used only in ksz8795.c, no need to export it.

You can consider grouping patch 8, 20 and 23 into single patch. Since
all adding static to function namely ksz8_r_dyn_mac_table,
ksz8_r_sta_mac_table and ksz8_w_sta_mac_table.

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8.h    | 2 --
>  drivers/net/dsa/microchip/ksz8795.c | 4 ++--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8.h
> b/drivers/net/dsa/microchip/ksz8.h
> index a28fa7cd4d98..ed72ec626593 100644
> --- a/drivers/net/dsa/microchip/ksz8.h
> +++ b/drivers/net/dsa/microchip/ksz8.h
> @@ -19,8 +19,6 @@ void ksz8_flush_dyn_mac_table(struct ksz_device
> *dev, int port);
>  void ksz8_port_setup(struct ksz_device *dev, int port, bool
> cpu_port);
>  int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
>  int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
> -int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
> -                        struct alu_struct *alu);
>  void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
>                           struct alu_struct *alu);
>  void ksz8_r_mib_cnt(struct ksz_device *dev, int port, u16 addr, u64
> *cnt);
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index 31c77e086a9d..1c08103c9f50 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -456,8 +456,8 @@ static int ksz8_r_dyn_mac_table(struct ksz_device
> *dev, u16 addr, u8 *mac_addr,
>         return ret;
>  }
> 
> -int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
> -                        struct alu_struct *alu)
> +static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
> +                               struct alu_struct *alu)
>  {
>         u32 data_hi, data_lo;
>         const u8 *shifts;
> --
> 2.30.2
> 

[PATCH v1 21/26] net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for empty entries

[Oleksij Rempel]

This is a preparation for the next patch, to make use of read/write errors.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 94 +++++++++++++++++------------
 1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 1c08103c9f50..b7487be91f67 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -457,7 +457,7 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 }
 
 static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
-				struct alu_struct *alu)
+				struct alu_struct *alu, bool *valid)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
@@ -470,28 +470,32 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 	ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
 	data_hi = data >> 32;
 	data_lo = (u32)data;
-	if (data_hi & (masks[STATIC_MAC_TABLE_VALID] |
-			masks[STATIC_MAC_TABLE_OVERRIDE])) {
-		alu->mac[5] = (u8)data_lo;
-		alu->mac[4] = (u8)(data_lo >> 8);
-		alu->mac[3] = (u8)(data_lo >> 16);
-		alu->mac[2] = (u8)(data_lo >> 24);
-		alu->mac[1] = (u8)data_hi;
-		alu->mac[0] = (u8)(data_hi >> 8);
-		alu->port_forward =
-			(data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
-				shifts[STATIC_MAC_FWD_PORTS];
-		alu->is_override =
-			(data_hi & masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
-		data_hi >>= 1;
-		alu->is_static = true;
-		alu->is_use_fid =
-			(data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ? 1 : 0;
-		alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
-				shifts[STATIC_MAC_FID];
+
+	if (!(data_hi & (masks[STATIC_MAC_TABLE_VALID] |
+			 masks[STATIC_MAC_TABLE_OVERRIDE]))) {
+		*valid = false;
 		return 0;
 	}
-	return -ENXIO;
+
+	alu->mac[5] = (u8)data_lo;
+	alu->mac[4] = (u8)(data_lo >> 8);
+	alu->mac[3] = (u8)(data_lo >> 16);
+	alu->mac[2] = (u8)(data_lo >> 24);
+	alu->mac[1] = (u8)data_hi;
+	alu->mac[0] = (u8)(data_hi >> 8);
+	alu->port_forward =
+		(data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
+			shifts[STATIC_MAC_FWD_PORTS];
+	alu->is_override = (data_hi & masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
+	data_hi >>= 1;
+	alu->is_static = true;
+	alu->is_use_fid = (data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ? 1 : 0;
+	alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
+		shifts[STATIC_MAC_FID];
+
+	*valid = true;
+
+	return 0;
 }
 
 void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
@@ -969,12 +973,13 @@ int ksz8_fdb_dump(struct ksz_device *dev, int port,
 
 	for (i = 0; i  < dev->info->num_statics; i++) {
 		struct alu_struct alu;
+		bool valid;
 
-		ret = ksz8_r_sta_mac_table(dev, i, &alu);
-		if (ret == -ENXIO)
-			continue;
+		ret = ksz8_r_sta_mac_table(dev, i, &alu, &valid);
 		if (ret)
 			return ret;
+		if (!valid)
+			continue;
 
 		if (!(alu.port_forward & BIT(port)))
 			continue;
@@ -1010,20 +1015,25 @@ static int ksz8_add_sta_mac(struct ksz_device *dev, int port,
 			    const unsigned char *addr, u16 vid)
 {
 	struct alu_struct alu;
-	int index;
+	int index, ret;
 	int empty = 0;
 
 	alu.port_forward = 0;
 	for (index = 0; index < dev->info->num_statics; index++) {
-		if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
-			/* Found one already in static MAC table. */
-			if (!memcmp(alu.mac, addr, ETH_ALEN) &&
-			    alu.fid == vid)
-				break;
-		/* Remember the first empty entry. */
-		} else if (!empty) {
-			empty = index + 1;
+		bool valid;
+
+		ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
+		if (ret)
+			return ret;
+		if (!valid) {
+			/* Remember the first empty entry. */
+			if (!empty)
+				empty = index + 1;
+			continue;
 		}
+
+		if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid == vid)
+			break;
 	}
 
 	/* no available entry */
@@ -1053,15 +1063,19 @@ static int ksz8_del_sta_mac(struct ksz_device *dev, int port,
 			    const unsigned char *addr, u16 vid)
 {
 	struct alu_struct alu;
-	int index;
+	int index, ret;
 
 	for (index = 0; index < dev->info->num_statics; index++) {
-		if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
-			/* Found one already in static MAC table. */
-			if (!memcmp(alu.mac, addr, ETH_ALEN) &&
-			    alu.fid == vid)
-				break;
-		}
+		bool valid;
+
+		ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
+		if (ret)
+			return ret;
+		if (!valid)
+			continue;
+
+		if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid == vid)
+			break;
 	}
 
 	/* no available entry */
-- 
2.30.2

Re: [PATCH v1 21/26] net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for empty entries

[Arun.Ramadoss]

On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> This is a preparation for the next patch, to make use of read/write
> errors.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 94 +++++++++++++++++--------
> ----
>  1 file changed, 54 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index 1c08103c9f50..b7487be91f67 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -457,7 +457,7 @@ static int ksz8_r_dyn_mac_table(struct ksz_device
> *dev, u16 addr, u8 *mac_addr,
>  }
> 
>  static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
> -                               struct alu_struct *alu)
> +                               struct alu_struct *alu, bool *valid)
>  {
>         u32 data_hi, data_lo;
>         const u8 *shifts;
> @@ -470,28 +470,32 @@ static int ksz8_r_sta_mac_table(struct
> ksz_device *dev, u16 addr,
>         ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
>         data_hi = data >> 32;
>         data_lo = (u32)data;
> -       if (data_hi & (masks[STATIC_MAC_TABLE_VALID] |
> -                       masks[STATIC_MAC_TABLE_OVERRIDE])) {
> -               alu->mac[5] = (u8)data_lo;
> -               alu->mac[4] = (u8)(data_lo >> 8);
> -               alu->mac[3] = (u8)(data_lo >> 16);
> -               alu->mac[2] = (u8)(data_lo >> 24);
> -               alu->mac[1] = (u8)data_hi;
> -               alu->mac[0] = (u8)(data_hi >> 8);
> -               alu->port_forward =
> -                       (data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS])
> >>
> -                               shifts[STATIC_MAC_FWD_PORTS];
> -               alu->is_override =
> -                       (data_hi & masks[STATIC_MAC_TABLE_OVERRIDE])
> ? 1 : 0;
> -               data_hi >>= 1;
> -               alu->is_static = true;
> -               alu->is_use_fid =
> -                       (data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ?
> 1 : 0;
> -               alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
> -                               shifts[STATIC_MAC_FID];
> +
> +       if (!(data_hi & (masks[STATIC_MAC_TABLE_VALID] |
> +                        masks[STATIC_MAC_TABLE_OVERRIDE]))) {
> +               *valid = false;
>                 return 0;
>         }
> -       return -ENXIO;
> +
> +       alu->mac[5] = (u8)data_lo;
> +       alu->mac[4] = (u8)(data_lo >> 8);
> +       alu->mac[3] = (u8)(data_lo >> 16);
> +       alu->mac[2] = (u8)(data_lo >> 24);
> +       alu->mac[1] = (u8)data_hi;
> +       alu->mac[0] = (u8)(data_hi >> 8);

u64_to_ether_addr macro can be used to store address into array.

> +       alu->port_forward =
> +               (data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
> +                       shifts[STATIC_MAC_FWD_PORTS];



> +       alu->is_override = (data_hi &
> masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
> +       data_hi >>= 1;
> +       alu->is_static = true;
> +       alu->is_use_fid = (data_hi & masks[STATIC_MAC_TABLE_USE_FID])
> ? 1 : 0;
> +       alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
> +               shifts[STATIC_MAC_FID];

Instead of masks and shifts, you consider using
FIELD_GET(STATIC_MAC_TABLE_FID, data_hi);

> +
> +       *valid = true;
> +
> +       return 0;
>  }
> 
>  void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
> @@ -969,12 +973,13 @@ int ksz8_fdb_dump(struct ksz_device *dev, int
> port,
> 
>         for (i = 0; i  < dev->info->num_statics; i++) {
>                 struct alu_struct alu;
> +               bool valid;
> 
> -               ret = ksz8_r_sta_mac_table(dev, i, &alu);
> -               if (ret == -ENXIO)
> -                       continue;
> +               ret = ksz8_r_sta_mac_table(dev, i, &alu, &valid);
>                 if (ret)
>                         return ret;
> +               if (!valid)
> +                       continue;
> 
>                 if (!(alu.port_forward & BIT(port)))
>                         continue;
> @@ -1010,20 +1015,25 @@ static int ksz8_add_sta_mac(struct ksz_device
> *dev, int port,
>                             const unsigned char *addr, u16 vid)
>  {
>         struct alu_struct alu;
> -       int index;
> +       int index, ret;
>         int empty = 0;
> 
>         alu.port_forward = 0;
>         for (index = 0; index < dev->info->num_statics; index++) {
> -               if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
> -                       /* Found one already in static MAC table. */
> -                       if (!memcmp(alu.mac, addr, ETH_ALEN) &&
> -                           alu.fid == vid)
> -                               break;
> -               /* Remember the first empty entry. */
> -               } else if (!empty) {
> -                       empty = index + 1;
> +               bool valid;
> +
> +               ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
> +               if (ret)
> +                       return ret;
> +               if (!valid) {
> +                       /* Remember the first empty entry. */
> +                       if (!empty)
> +                               empty = index + 1;
> +                       continue;
>                 }
> +
> +               if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid ==
> vid)
> +                       break;
>         }
> 
>         /* no available entry */
> @@ -1053,15 +1063,19 @@ static int ksz8_del_sta_mac(struct ksz_device
> *dev, int port,
>                             const unsigned char *addr, u16 vid)
>  {
>         struct alu_struct alu;
> -       int index;
> +       int index, ret;

Variable declaration in separate line.

> 
>         for (index = 0; index < dev->info->num_statics; index++) {
> -               if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
> -                       /* Found one already in static MAC table. */
> -                       if (!memcmp(alu.mac, addr, ETH_ALEN) &&
> -                           alu.fid == vid)
> -                               break;
> -               }
> +               bool valid;
> +
> +               ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
> +               if (ret)
> +                       return ret;
> +               if (!valid)
> +                       continue;
> +
> +               if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid ==
> vid)
> +                       break;
>         }
> 
>         /* no available entry */
> --
> 2.30.2
> 

Re: [PATCH v1 21/26] net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for empty entries

[Oleksij Rempel]

On Tue, Nov 29, 2022 at 05:25:55AM +0000, Arun.Ramadoss@microchip.com wrote:
> On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > This is a preparation for the next patch, to make use of read/write
> > errors.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/ksz8795.c | 94 +++++++++++++++++--------
> > ----
> >  1 file changed, 54 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> > b/drivers/net/dsa/microchip/ksz8795.c
> > index 1c08103c9f50..b7487be91f67 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -457,7 +457,7 @@ static int ksz8_r_dyn_mac_table(struct ksz_device
> > *dev, u16 addr, u8 *mac_addr,
> >  }
> > 
> >  static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
> > -                               struct alu_struct *alu)
> > +                               struct alu_struct *alu, bool *valid)
> >  {
> >         u32 data_hi, data_lo;
> >         const u8 *shifts;
> > @@ -470,28 +470,32 @@ static int ksz8_r_sta_mac_table(struct
> > ksz_device *dev, u16 addr,
> >         ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
> >         data_hi = data >> 32;
> >         data_lo = (u32)data;
> > -       if (data_hi & (masks[STATIC_MAC_TABLE_VALID] |
> > -                       masks[STATIC_MAC_TABLE_OVERRIDE])) {
> > -               alu->mac[5] = (u8)data_lo;
> > -               alu->mac[4] = (u8)(data_lo >> 8);
> > -               alu->mac[3] = (u8)(data_lo >> 16);
> > -               alu->mac[2] = (u8)(data_lo >> 24);
> > -               alu->mac[1] = (u8)data_hi;
> > -               alu->mac[0] = (u8)(data_hi >> 8);
> > -               alu->port_forward =
> > -                       (data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS])
> > >>
> > -                               shifts[STATIC_MAC_FWD_PORTS];
> > -               alu->is_override =
> > -                       (data_hi & masks[STATIC_MAC_TABLE_OVERRIDE])
> > ? 1 : 0;
> > -               data_hi >>= 1;
> > -               alu->is_static = true;
> > -               alu->is_use_fid =
> > -                       (data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ?
> > 1 : 0;
> > -               alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
> > -                               shifts[STATIC_MAC_FID];
> > +
> > +       if (!(data_hi & (masks[STATIC_MAC_TABLE_VALID] |
> > +                        masks[STATIC_MAC_TABLE_OVERRIDE]))) {
> > +               *valid = false;
> >                 return 0;
> >         }
> > -       return -ENXIO;
> > +
> > +       alu->mac[5] = (u8)data_lo;
> > +       alu->mac[4] = (u8)(data_lo >> 8);
> > +       alu->mac[3] = (u8)(data_lo >> 16);
> > +       alu->mac[2] = (u8)(data_lo >> 24);
> > +       alu->mac[1] = (u8)data_hi;
> > +       alu->mac[0] = (u8)(data_hi >> 8);
> 
> u64_to_ether_addr macro can be used to store address into array.

This should not be done in this patch.

> > +       alu->port_forward =
> > +               (data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
> > +                       shifts[STATIC_MAC_FWD_PORTS];
> 
> 
> 
> > +       alu->is_override = (data_hi &
> > masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
> > +       data_hi >>= 1;
> > +       alu->is_static = true;
> > +       alu->is_use_fid = (data_hi & masks[STATIC_MAC_TABLE_USE_FID])
> > ? 1 : 0;
> > +       alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
> > +               shifts[STATIC_MAC_FID];
> 
> Instead of masks and shifts, you consider using
> FIELD_GET(STATIC_MAC_TABLE_FID, data_hi);

I would love to do this, but in this case, complete driver should be
splittet in to KSZ88X3 and KSZ879X portions. FIELD_GET can't work with
dynamic masks and shifts.

I would say, it is not for this patch set.

> > +
> > +       *valid = true;
> > +
> > +       return 0;
> >  }
> > 
> >  void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
> > @@ -969,12 +973,13 @@ int ksz8_fdb_dump(struct ksz_device *dev, int
> > port,
> > 
> >         for (i = 0; i  < dev->info->num_statics; i++) {
> >                 struct alu_struct alu;
> > +               bool valid;
> > 
> > -               ret = ksz8_r_sta_mac_table(dev, i, &alu);
> > -               if (ret == -ENXIO)
> > -                       continue;
> > +               ret = ksz8_r_sta_mac_table(dev, i, &alu, &valid);
> >                 if (ret)
> >                         return ret;
> > +               if (!valid)
> > +                       continue;
> > 
> >                 if (!(alu.port_forward & BIT(port)))
> >                         continue;
> > @@ -1010,20 +1015,25 @@ static int ksz8_add_sta_mac(struct ksz_device
> > *dev, int port,
> >                             const unsigned char *addr, u16 vid)
> >  {
> >         struct alu_struct alu;
> > -       int index;
> > +       int index, ret;
> >         int empty = 0;
> > 
> >         alu.port_forward = 0;
> >         for (index = 0; index < dev->info->num_statics; index++) {
> > -               if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
> > -                       /* Found one already in static MAC table. */
> > -                       if (!memcmp(alu.mac, addr, ETH_ALEN) &&
> > -                           alu.fid == vid)
> > -                               break;
> > -               /* Remember the first empty entry. */
> > -               } else if (!empty) {
> > -                       empty = index + 1;
> > +               bool valid;
> > +
> > +               ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
> > +               if (ret)
> > +                       return ret;
> > +               if (!valid) {
> > +                       /* Remember the first empty entry. */
> > +                       if (!empty)
> > +                               empty = index + 1;
> > +                       continue;
> >                 }
> > +
> > +               if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid ==
> > vid)
> > +                       break;
> >         }
> > 
> >         /* no available entry */
> > @@ -1053,15 +1063,19 @@ static int ksz8_del_sta_mac(struct ksz_device
> > *dev, int port,
> >                             const unsigned char *addr, u16 vid)
> >  {
> >         struct alu_struct alu;
> > -       int index;
> > +       int index, ret;
> 
> Variable declaration in separate line.

Is it a new coding style requirement? I can't find it here:
https://www.kernel.org/doc/html/v6.0/process/coding-style.html

> > 
> >         for (index = 0; index < dev->info->num_statics; index++) {
> > -               if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
> > -                       /* Found one already in static MAC table. */
> > -                       if (!memcmp(alu.mac, addr, ETH_ALEN) &&
> > -                           alu.fid == vid)
> > -                               break;
> > -               }
> > +               bool valid;
> > +
> > +               ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
> > +               if (ret)
> > +                       return ret;
> > +               if (!valid)
> > +                       continue;
> > +
> > +               if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid ==
> > vid)
> > +                       break;
> >         }
> > 
> >         /* no available entry */
> > --
> > 2.30.2
> > 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v1 22/26] net: dsa: microchip: ksz8_r_sta_mac_table(): make use of error values provided by read/write functions

[Oleksij Rempel]

Read/write operations may fail. So, make use of return values.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index b7487be91f67..1de33ceb50de 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -336,19 +336,26 @@ void ksz8_port_init_cnt(struct ksz_device *dev, int port)
 	}
 }
 
-static void ksz8_r_table(struct ksz_device *dev, int table, u16 addr, u64 *data)
+static int ksz8_r_table(struct ksz_device *dev, int table, u16 addr, u64 *data)
 {
 	const u16 *regs;
 	u16 ctrl_addr;
+	int ret;
 
 	regs = dev->info->regs;
 
 	ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
-	ksz_read64(dev, regs[REG_IND_DATA_HI], data);
+	ret = ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	if (ret)
+		goto unlock_alu;
+
+	ret = ksz_read64(dev, regs[REG_IND_DATA_HI], data);
+unlock_alu:
 	mutex_unlock(&dev->alu_mutex);
+
+	return ret;
 }
 
 static void ksz8_w_table(struct ksz_device *dev, int table, u16 addr, u64 data)
@@ -463,11 +470,15 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 	const u8 *shifts;
 	const u32 *masks;
 	u64 data;
+	int ret;
 
 	shifts = dev->info->shifts;
 	masks = dev->info->masks;
 
-	ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
+	ret = ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
+	if (ret)
+		return ret;
+
 	data_hi = data >> 32;
 	data_lo = (u32)data;
 
-- 
2.30.2

[PATCH v1 23/26] net: dsa: microchip: make ksz8_w_sta_mac_table() static

[Oleksij Rempel]

ksz8_w_sta_mac_table() is used only in ksz8795.c. So, make it static.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8.h    | 2 --
 drivers/net/dsa/microchip/ksz8795.c | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index ed72ec626593..e3d1bbde008a 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -19,8 +19,6 @@ void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port);
 void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port);
 int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
-void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
-			  struct alu_struct *alu);
 void ksz8_r_mib_cnt(struct ksz_device *dev, int port, u16 addr, u64 *cnt);
 void ksz8_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
 		    u64 *dropped, u64 *cnt);
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 1de33ceb50de..9c1450782314 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -509,8 +509,8 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 	return 0;
 }
 
-void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
-			  struct alu_struct *alu)
+static void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
+				 struct alu_struct *alu)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
-- 
2.30.2

[PATCH v1 24/26] net: dsa: microchip: ksz8_w_sta_mac_table(): make use of error values provided by read/write functions

[Oleksij Rempel]

Read/write operations may fail. So, make use of return values.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 33 ++++++++++++++++-------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 9c1450782314..ea08bdea193e 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -358,19 +358,26 @@ static int ksz8_r_table(struct ksz_device *dev, int table, u16 addr, u64 *data)
 	return ret;
 }
 
-static void ksz8_w_table(struct ksz_device *dev, int table, u16 addr, u64 data)
+static int ksz8_w_table(struct ksz_device *dev, int table, u16 addr, u64 data)
 {
 	const u16 *regs;
 	u16 ctrl_addr;
+	int ret;
 
 	regs = dev->info->regs;
 
 	ctrl_addr = IND_ACC_TABLE(table) | addr;
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write64(dev, regs[REG_IND_DATA_HI], data);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ret = ksz_write64(dev, regs[REG_IND_DATA_HI], data);
+	if (ret)
+		goto unlock_alu;
+
+	ret = ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+unlock_alu:
 	mutex_unlock(&dev->alu_mutex);
+
+	return ret;
 }
 
 static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
@@ -509,8 +516,8 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 	return 0;
 }
 
-static void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
-				 struct alu_struct *alu)
+static int ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
+				struct alu_struct *alu)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
@@ -538,7 +545,8 @@ static void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
 		data_hi &= ~masks[STATIC_MAC_TABLE_OVERRIDE];
 
 	data = (u64)data_hi << 32 | data_lo;
-	ksz8_w_table(dev, TABLE_STATIC_MAC, addr, data);
+
+	return ksz8_w_table(dev, TABLE_STATIC_MAC, addr, data);
 }
 
 static void ksz8_from_vlan(struct ksz_device *dev, u32 vlan, u8 *fid,
@@ -1065,9 +1073,8 @@ static int ksz8_add_sta_mac(struct ksz_device *dev, int port,
 		/* Need a way to map VID to FID. */
 		alu.fid = vid;
 	}
-	ksz8_w_sta_mac_table(dev, index, &alu);
 
-	return 0;
+	return ksz8_w_sta_mac_table(dev, index, &alu);
 }
 
 static int ksz8_del_sta_mac(struct ksz_device *dev, int port,
@@ -1091,16 +1098,14 @@ static int ksz8_del_sta_mac(struct ksz_device *dev, int port,
 
 	/* no available entry */
 	if (index == dev->info->num_statics)
-		goto exit;
+		return 0;
 
 	/* clear port */
 	alu.port_forward &= ~BIT(port);
 	if (!alu.port_forward)
 		alu.is_static = false;
-	ksz8_w_sta_mac_table(dev, index, &alu);
 
-exit:
-	return 0;
+	return ksz8_w_sta_mac_table(dev, index, &alu);
 }
 
 int ksz8_mdb_add(struct ksz_device *dev, int port,
@@ -1424,9 +1429,7 @@ int ksz8_enable_stp_addr(struct ksz_device *dev)
 	alu.is_override = true;
 	alu.port_forward = dev->info->cpu_ports;
 
-	ksz8_w_sta_mac_table(dev, 0, &alu);
-
-	return 0;
+	return ksz8_w_sta_mac_table(dev, 0, &alu);
 }
 
 int ksz8_setup(struct dsa_switch *ds)
-- 
2.30.2

[PATCH v1 25/26] net: dsa: microchip: remove ksz_port:on variable

[Oleksij Rempel]

The only place where this variable would be set to false is the
ksz8_config_cpu_port() function. But it is done in a bogus way:

 	for (i = 0; i < dev->phy_port_cnt; i++) {
		if (i == dev->phy_port_cnt) <--- will be never executed.
			break;
		p->on = 1;

So, we never have a situation where p->on = 0. In this case, we can just
remove it.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c    | 20 +-------------------
 drivers/net/dsa/microchip/ksz_common.h |  1 -
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index ea08bdea193e..618366fadfb5 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -952,7 +952,6 @@ void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port)
 {
 	u8 learn[DSA_MAX_PORTS];
 	int first, index, cnt;
-	struct ksz_port *p;
 	const u16 *regs;
 
 	regs = dev->info->regs;
@@ -966,9 +965,6 @@ void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port)
 		cnt = dev->info->port_cnt;
 	}
 	for (index = first; index < cnt; index++) {
-		p = &dev->ports[index];
-		if (!p->on)
-			continue;
 		ksz_pread8(dev, index, regs[P_STP_CTRL], &learn[index]);
 		if (!(learn[index] & PORT_LEARN_DISABLE))
 			ksz_pwrite8(dev, index, regs[P_STP_CTRL],
@@ -976,9 +972,6 @@ void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port)
 	}
 	ksz_cfg(dev, S_FLUSH_TABLE_CTRL, SW_FLUSH_DYN_MAC_TABLE, true);
 	for (index = first; index < cnt; index++) {
-		p = &dev->ports[index];
-		if (!p->on)
-			continue;
 		if (!(learn[index] & PORT_LEARN_DISABLE))
 			ksz_pwrite8(dev, index, regs[P_STP_CTRL], learn[index]);
 	}
@@ -1368,25 +1361,14 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
 
 	ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true);
 
-	p = &dev->ports[dev->cpu_port];
-	p->on = 1;
-
 	ksz8_port_setup(dev, dev->cpu_port, true);
 
 	for (i = 0; i < dev->phy_port_cnt; i++) {
-		p = &dev->ports[i];
-
 		ksz_port_stp_state_set(ds, i, BR_STATE_DISABLED);
-
-		/* Last port may be disabled. */
-		if (i == dev->phy_port_cnt)
-			break;
-		p->on = 1;
 	}
 	for (i = 0; i < dev->phy_port_cnt; i++) {
 		p = &dev->ports[i];
-		if (!p->on)
-			continue;
+
 		if (!ksz_is_ksz88x3(dev)) {
 			ksz_pread8(dev, i, regs[P_REMOTE_STATUS], &remote);
 			if (remote & KSZ8_PORT_FIBER_MODE)
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 055d61ff3fb8..504ad07842a0 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -87,7 +87,6 @@ struct ksz_port {
 	int stp_state;
 	struct phy_device phydev;
 
-	u32 on:1;			/* port is not disabled by hardware */
 	u32 fiber:1;			/* port is fiber */
 	u32 force:1;
 	u32 read:1;			/* read MIB counters in background */
-- 
2.30.2

[PATCH v1 26/26] net: dsa: microchip: ksz8: do not force flow control by default

[Oleksij Rempel]

Make flow control configurable by moving it to ksz8_phylink_mac_link_up()

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8.h       |  4 ++
 drivers/net/dsa/microchip/ksz8795.c    | 54 ++++++++++++++++++++++----
 drivers/net/dsa/microchip/ksz_common.c |  1 +
 3 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index e3d1bbde008a..77439d9c5fc2 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -56,5 +56,9 @@ int ksz8_switch_detect(struct ksz_device *dev);
 int ksz8_switch_init(struct ksz_device *dev);
 void ksz8_switch_exit(struct ksz_device *dev);
 int ksz8_change_mtu(struct ksz_device *dev, int port, int mtu);
+void ksz8_phylink_mac_link_up(struct ksz_device *dev, int port,
+			      unsigned int mode, phy_interface_t interface,
+			      struct phy_device *phydev, int speed, int duplex,
+			      bool tx_pause, bool rx_pause);
 
 #endif
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 618366fadfb5..4782747b7b61 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1374,12 +1374,52 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
 			if (remote & KSZ8_PORT_FIBER_MODE)
 				p->fiber = 1;
 		}
-		if (p->fiber)
-			ksz_port_cfg(dev, i, regs[P_STP_CTRL],
-				     PORT_FORCE_FLOW_CTRL, true);
-		else
-			ksz_port_cfg(dev, i, regs[P_STP_CTRL],
-				     PORT_FORCE_FLOW_CTRL, false);
+	}
+}
+
+void ksz8_phylink_mac_link_up(struct ksz_device *dev, int port,
+			      unsigned int mode, phy_interface_t interface,
+			      struct phy_device *phydev, int speed, int duplex,
+			      bool tx_pause, bool rx_pause)
+{
+	struct dsa_switch *ds = dev->ds;
+	struct ksz_port *p;
+	u8 ctrl = 0;
+
+	p = &dev->ports[port];
+
+	if (dsa_upstream_port(ds, port)) {
+		u8 mask = SW_HALF_DUPLEX_FLOW_CTRL | SW_HALF_DUPLEX |
+			SW_FLOW_CTRL | SW_10_MBIT;
+
+		if (duplex) {
+			if (tx_pause && rx_pause)
+				ctrl |= SW_FLOW_CTRL;
+		} else {
+			ctrl |= SW_HALF_DUPLEX;
+			if (tx_pause && rx_pause)
+				ctrl |= SW_HALF_DUPLEX_FLOW_CTRL;
+		}
+
+		if (speed == SPEED_10)
+			ctrl |= SW_10_MBIT;
+
+		ksz_rmw8(dev, REG_SW_CTRL_4, mask, ctrl);
+
+		p->phydev.speed = speed;
+	} else {
+		const u16 *regs = dev->info->regs;
+
+		if (duplex) {
+			if (tx_pause && rx_pause)
+				ctrl |= PORT_FORCE_FLOW_CTRL;
+		} else {
+			if (tx_pause && rx_pause)
+				ctrl |= PORT_BACK_PRESSURE;
+		}
+
+		ksz_rmw8(dev, regs[P_STP_CTRL], PORT_FORCE_FLOW_CTRL |
+			 PORT_BACK_PRESSURE, ctrl);
 	}
 }
 
@@ -1431,8 +1471,6 @@ int ksz8_setup(struct dsa_switch *ds)
 	 */
 	ds->vlan_filtering_is_global = true;
 
-	ksz_cfg(dev, S_REPLACE_VID_CTRL, SW_FLOW_CTRL, true);
-
 	/* Enable automatic fast aging when link changed detected. */
 	ksz_cfg(dev, S_LINK_AGING_CTRL, SW_LINK_AUTO_AGING, true);
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 171cb0063fbf..bfdb4f4f36cc 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -207,6 +207,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
 	.mirror_add = ksz8_port_mirror_add,
 	.mirror_del = ksz8_port_mirror_del,
 	.get_caps = ksz8_get_caps,
+	.phylink_mac_link_up = ksz8_phylink_mac_link_up,
 	.config_cpu_port = ksz8_config_cpu_port,
 	.enable_stp_addr = ksz8_enable_stp_addr,
 	.reset = ksz8_reset_switch,
-- 
2.30.2

Re: [PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[Andrew Lunn]

On Mon, Nov 28, 2022 at 12:59:32PM +0100, Oleksij Rempel wrote:
> This patch series is a result of maintaining work on ksz8 part of
> microchip driver. It includes stats64 and fdb support. Error handling.
> Loopback fix and so on...
> 
> Oleksij Rempel (26):

The netdev FAQ says:

* don’t post large series (> 15 patches), break them up

This seems like something which could easily be broken up.

     Andrew

Re: [PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[Jakub Kicinski]

On Mon, 28 Nov 2022 14:51:47 +0100 Andrew Lunn wrote:
> * don’t post large series (> 15 patches), break them up
> 
> This seems like something which could easily be broken up.

And name the target tree in the subject tag. And make sure it applies
cleanly to that tree (last patch is to blame AFAICT). And don't repost
within 24 hours.

Re: [PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[Jacob Keller]

On 11/28/2022 3:59 AM, Oleksij Rempel wrote:
> This patch series is a result of maintaining work on ksz8 part of
> microchip driver. It includes stats64 and fdb support. Error handling.
> Loopback fix and so on...
> 
> Oleksij Rempel (26):
>    net: dsa: microchip: add stats64 support for ksz8 series of switches
>    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix port validation and VID
>      information
>    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix not complete fdb
>      extraction
>    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix time stamp extraction
>    net: dsa: microchip: ksz8: ksz8_fdb_dump: do not extract ghost entry
>      from empty table
>    net: dsa: microchip: ksz8863_smi: fix bulk access
>    net: dsa: microchip: ksz8_r_dyn_mac_table(): remove timestamp support
>    net: dsa: microchip: make ksz8_r_dyn_mac_table() static
>    net: dsa: microchip: ksz8_r_dyn_mac_table(): remove fid support
>    net: dsa: microchip: ksz8: refactor ksz8_fdb_dump()
>    net: dsa: microchip: ksz8: ksz8_fdb_dump: dump static MAC table
>    net: dsa: microchip: ksz8: move static mac table operations to a
>      separate functions
>    net: dsa: microchip: ksz8: add fdb_add/del support
>    net: dsa: microchip: KSZ88x3 fix loopback support
>    net: dsa: microchip: ksz8_r_dyn_mac_table(): move main part of the
>      code out of if statement
>    net: dsa: microchip: ksz8_r_dyn_mac_table(): use ret instead of rc
>    net: dsa: microchip: ksz8_r_dyn_mac_table(): ksz: do not return EAGAIN
>      on timeout
>    net: dsa: microchip: ksz8_r_dyn_mac_table(): return read/write error
>      if we got any
>    net: dsa: microchip: ksz8_r_dyn_mac_table(): use entries variable to
>      signal 0 entries
>    net: dsa: microchip: make ksz8_r_sta_mac_table() static
>    net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for
>      empty entries
>    net: dsa: microchip: ksz8_r_sta_mac_table(): make use of error values
>      provided by read/write functions
>    net: dsa: microchip: make ksz8_w_sta_mac_table() static
>    net: dsa: microchip: ksz8_w_sta_mac_table(): make use of error values
>      provided by read/write functions
>    net: dsa: microchip: remove ksz_port:on variable
>    net: dsa: microchip: ksz8: do not force flow control by default
> 


My understanding is that we typically limit series to 15 patches. Do you 
have some justification for why this goes over 15 and can't reasonably 
be split into two series?

At a glance it seems like a bunch of smaller cleanups.

Re: [PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[Oleksij Rempel]

On Mon, Nov 28, 2022 at 03:09:19PM -0800, Jacob Keller wrote:
> 
> 
> On 11/28/2022 3:59 AM, Oleksij Rempel wrote:
> > This patch series is a result of maintaining work on ksz8 part of
> > microchip driver. It includes stats64 and fdb support. Error handling.
> > Loopback fix and so on...
> > 
> > Oleksij Rempel (26):
> >    net: dsa: microchip: add stats64 support for ksz8 series of switches
> >    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix port validation and VID
> >      information
> >    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix not complete fdb
> >      extraction
> >    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix time stamp extraction
> >    net: dsa: microchip: ksz8: ksz8_fdb_dump: do not extract ghost entry
> >      from empty table
> >    net: dsa: microchip: ksz8863_smi: fix bulk access
> >    net: dsa: microchip: ksz8_r_dyn_mac_table(): remove timestamp support
> >    net: dsa: microchip: make ksz8_r_dyn_mac_table() static
> >    net: dsa: microchip: ksz8_r_dyn_mac_table(): remove fid support
> >    net: dsa: microchip: ksz8: refactor ksz8_fdb_dump()
> >    net: dsa: microchip: ksz8: ksz8_fdb_dump: dump static MAC table
> >    net: dsa: microchip: ksz8: move static mac table operations to a
> >      separate functions
> >    net: dsa: microchip: ksz8: add fdb_add/del support
> >    net: dsa: microchip: KSZ88x3 fix loopback support
> >    net: dsa: microchip: ksz8_r_dyn_mac_table(): move main part of the
> >      code out of if statement
> >    net: dsa: microchip: ksz8_r_dyn_mac_table(): use ret instead of rc
> >    net: dsa: microchip: ksz8_r_dyn_mac_table(): ksz: do not return EAGAIN
> >      on timeout
> >    net: dsa: microchip: ksz8_r_dyn_mac_table(): return read/write error
> >      if we got any
> >    net: dsa: microchip: ksz8_r_dyn_mac_table(): use entries variable to
> >      signal 0 entries
> >    net: dsa: microchip: make ksz8_r_sta_mac_table() static
> >    net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for
> >      empty entries
> >    net: dsa: microchip: ksz8_r_sta_mac_table(): make use of error values
> >      provided by read/write functions
> >    net: dsa: microchip: make ksz8_w_sta_mac_table() static
> >    net: dsa: microchip: ksz8_w_sta_mac_table(): make use of error values
> >      provided by read/write functions
> >    net: dsa: microchip: remove ksz_port:on variable
> >    net: dsa: microchip: ksz8: do not force flow control by default
> > 
> 
> 
> My understanding is that we typically limit series to 15 patches. Do you
> have some justification for why this goes over 15 and can't reasonably be
> split into two series?
> 
> At a glance it seems like a bunch of smaller cleanups.

The previous patch set got request to do more clean ups:
https://lore.kernel.org/all/20221124101458.3353902-1-o.rempel@pengutronix.de/

I need to show, there are already more patches in the queue.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[Andrew Lunn]

On Tue, Nov 29, 2022 at 06:35:39AM +0100, Oleksij Rempel wrote:
> On Mon, Nov 28, 2022 at 03:09:19PM -0800, Jacob Keller wrote:
> > 
> > 
> > On 11/28/2022 3:59 AM, Oleksij Rempel wrote:
> > > This patch series is a result of maintaining work on ksz8 part of
> > > microchip driver. It includes stats64 and fdb support. Error handling.
> > > Loopback fix and so on...
> > > 
> > > Oleksij Rempel (26):
> > >    net: dsa: microchip: add stats64 support for ksz8 series of switches
> > >    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix port validation and VID
> > >      information
> > >    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix not complete fdb
> > >      extraction
> > >    net: dsa: microchip: ksz8: ksz8_fdb_dump: fix time stamp extraction
> > >    net: dsa: microchip: ksz8: ksz8_fdb_dump: do not extract ghost entry
> > >      from empty table
> > >    net: dsa: microchip: ksz8863_smi: fix bulk access
> > >    net: dsa: microchip: ksz8_r_dyn_mac_table(): remove timestamp support
> > >    net: dsa: microchip: make ksz8_r_dyn_mac_table() static
> > >    net: dsa: microchip: ksz8_r_dyn_mac_table(): remove fid support
> > >    net: dsa: microchip: ksz8: refactor ksz8_fdb_dump()
> > >    net: dsa: microchip: ksz8: ksz8_fdb_dump: dump static MAC table
> > >    net: dsa: microchip: ksz8: move static mac table operations to a
> > >      separate functions
> > >    net: dsa: microchip: ksz8: add fdb_add/del support
> > >    net: dsa: microchip: KSZ88x3 fix loopback support
> > >    net: dsa: microchip: ksz8_r_dyn_mac_table(): move main part of the
> > >      code out of if statement
> > >    net: dsa: microchip: ksz8_r_dyn_mac_table(): use ret instead of rc
> > >    net: dsa: microchip: ksz8_r_dyn_mac_table(): ksz: do not return EAGAIN
> > >      on timeout
> > >    net: dsa: microchip: ksz8_r_dyn_mac_table(): return read/write error
> > >      if we got any
> > >    net: dsa: microchip: ksz8_r_dyn_mac_table(): use entries variable to
> > >      signal 0 entries
> > >    net: dsa: microchip: make ksz8_r_sta_mac_table() static
> > >    net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for
> > >      empty entries
> > >    net: dsa: microchip: ksz8_r_sta_mac_table(): make use of error values
> > >      provided by read/write functions
> > >    net: dsa: microchip: make ksz8_w_sta_mac_table() static
> > >    net: dsa: microchip: ksz8_w_sta_mac_table(): make use of error values
> > >      provided by read/write functions
> > >    net: dsa: microchip: remove ksz_port:on variable
> > >    net: dsa: microchip: ksz8: do not force flow control by default
> > > 
> > 
> > 
> > My understanding is that we typically limit series to 15 patches. Do you
> > have some justification for why this goes over 15 and can't reasonably be
> > split into two series?
> > 
> > At a glance it seems like a bunch of smaller cleanups.
> 
> The previous patch set got request to do more clean ups:
> https://lore.kernel.org/all/20221124101458.3353902-1-o.rempel@pengutronix.de/
> 
> I need to show, there are already more patches in the queue.

There is some psychology involved here. I see 26 patches and decide i
need to allocate 30 minutes to this sometime, and put the review off
until later, without even looking at them. If i get 5 patches, i
probably just do it, knowing i will be finished pretty quickly. My
guess is, 5 patches a day for 5 days will be merged faster than 26
patches in one go.

     Andrew

Re: [PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[Oleksij Rempel]

On Tue, Nov 29, 2022 at 03:15:03PM +0100, Andrew Lunn wrote:
> On Tue, Nov 29, 2022 at 06:35:39AM +0100, Oleksij Rempel wrote:
> > On Mon, Nov 28, 2022 at 03:09:19PM -0800, Jacob Keller wrote:
> > > 
> > > My understanding is that we typically limit series to 15 patches. Do you
> > > have some justification for why this goes over 15 and can't reasonably be
> > > split into two series?
> > > 
> > > At a glance it seems like a bunch of smaller cleanups.
> > 
> > The previous patch set got request to do more clean ups:
> > https://lore.kernel.org/all/20221124101458.3353902-1-o.rempel@pengutronix.de/
> > 
> > I need to show, there are already more patches in the queue.
> 
> There is some psychology involved here. I see 26 patches and decide i
> need to allocate 30 minutes to this sometime, and put the review off
> until later, without even looking at them. If i get 5 patches, i
> probably just do it, knowing i will be finished pretty quickly. My
> guess is, 5 patches a day for 5 days will be merged faster than 26
> patches in one go.

Good point. Thx!

I'll split this patch set.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v1 00/26] net: dsa: microchip: stats64, fdb, error

[Oleksij Rempel]

This patch series is a result of maintaining work on ksz8 part of
microchip driver. It includes stats64 and fdb support. Error handling.
Loopback fix and so on...

Oleksij Rempel (26):
  net: dsa: microchip: add stats64 support for ksz8 series of switches
  net: dsa: microchip: ksz8: ksz8_fdb_dump: fix port validation and VID
    information
  net: dsa: microchip: ksz8: ksz8_fdb_dump: fix not complete fdb
    extraction
  net: dsa: microchip: ksz8: ksz8_fdb_dump: fix time stamp extraction
  net: dsa: microchip: ksz8: ksz8_fdb_dump: do not extract ghost entry
    from empty table
  net: dsa: microchip: ksz8863_smi: fix bulk access
  net: dsa: microchip: ksz8_r_dyn_mac_table(): remove timestamp support
  net: dsa: microchip: make ksz8_r_dyn_mac_table() static
  net: dsa: microchip: ksz8_r_dyn_mac_table(): remove fid support
  net: dsa: microchip: ksz8: refactor ksz8_fdb_dump()
  net: dsa: microchip: ksz8: ksz8_fdb_dump: dump static MAC table
  net: dsa: microchip: ksz8: move static mac table operations to a
    separate functions
  net: dsa: microchip: ksz8: add fdb_add/del support
  net: dsa: microchip: KSZ88x3 fix loopback support
  net: dsa: microchip: ksz8_r_dyn_mac_table(): move main part of the
    code out of if statement
  net: dsa: microchip: ksz8_r_dyn_mac_table(): use ret instead of rc
  net: dsa: microchip: ksz8_r_dyn_mac_table(): ksz: do not return EAGAIN
    on timeout
  net: dsa: microchip: ksz8_r_dyn_mac_table(): return read/write error
    if we got any
  net: dsa: microchip: ksz8_r_dyn_mac_table(): use entries variable to
    signal 0 entries
  net: dsa: microchip: make ksz8_r_sta_mac_table() static
  net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for
    empty entries
  net: dsa: microchip: ksz8_r_sta_mac_table(): make use of error values
    provided by read/write functions
  net: dsa: microchip: make ksz8_w_sta_mac_table() static
  net: dsa: microchip: ksz8_w_sta_mac_table(): make use of error values
    provided by read/write functions
  net: dsa: microchip: remove ksz_port:on variable
  net: dsa: microchip: ksz8: do not force flow control by default

 drivers/net/dsa/microchip/ksz8.h        |  14 +-
 drivers/net/dsa/microchip/ksz8795.c     | 440 +++++++++++++++---------
 drivers/net/dsa/microchip/ksz8795_reg.h |   2 +
 drivers/net/dsa/microchip/ksz8863_smi.c |  10 +-
 drivers/net/dsa/microchip/ksz_common.c  | 100 +++++-
 drivers/net/dsa/microchip/ksz_common.h  |   2 +-
 6 files changed, 377 insertions(+), 191 deletions(-)

-- 
2.30.2

[PATCH v1 14/26] net: dsa: microchip: KSZ88x3 fix loopback support

[Oleksij Rempel]

With current code loopback is not working and selftest will always fail.
Fix register and bit offsets to make loopback on KSZ88x3 switches.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 24 ++++++++++++++++++------
 drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 208cf4dde397..a6d5de41a754 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -625,8 +625,13 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 		if (ret)
 			return ret;
 
-		if (restart & PORT_PHY_LOOPBACK)
-			data |= BMCR_LOOPBACK;
+		if (ksz_is_ksz88x3(dev)) {
+			if (restart & KSZ8873_PORT_PHY_LOOPBACK)
+				data |= BMCR_LOOPBACK;
+		} else {
+			if (restart & PORT_PHY_LOOPBACK)
+				data |= BMCR_LOOPBACK;
+		}
 		if (ctrl & PORT_FORCE_100_MBIT)
 			data |= BMCR_SPEED100;
 		if (ksz_is_ksz88x3(dev)) {
@@ -849,10 +854,17 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 			data |= PORT_FORCE_MDIX;
 		else
 			data &= ~PORT_FORCE_MDIX;
-		if (val & BMCR_LOOPBACK)
-			data |= PORT_PHY_LOOPBACK;
-		else
-			data &= ~PORT_PHY_LOOPBACK;
+		if (ksz_is_ksz88x3(dev)) {
+			if (val & BMCR_LOOPBACK)
+				data |= KSZ8873_PORT_PHY_LOOPBACK;
+			else
+				data &= ~KSZ8873_PORT_PHY_LOOPBACK;
+		} else {
+			if (val & BMCR_LOOPBACK)
+				data |= PORT_PHY_LOOPBACK;
+			else
+				data &= ~PORT_PHY_LOOPBACK;
+		}
 
 		if (data != restart) {
 			ret = ksz_pwrite8(dev, p, regs[P_NEG_RESTART_CTRL],
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 0bdceb534192..08204da7d621 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -262,6 +262,7 @@
 #define PORT_AUTO_MDIX_DISABLE		BIT(2)
 #define PORT_FORCE_MDIX			BIT(1)
 #define PORT_MAC_LOOPBACK		BIT(0)
+#define KSZ8873_PORT_PHY_LOOPBACK	BIT(0)
 
 #define REG_PORT_1_STATUS_2		0x1E
 #define REG_PORT_2_STATUS_2		0x2E
-- 
2.30.2