[RFC Patch net-next v3 0/3] net: dsa: microchip: lan937x: enable interrupt for internal phy link detection

[RFC Patch net-next v3 0/3] net: dsa: microchip: lan937x: enable interrupt for internal phy link detection

[Arun Ramadoss]

This patch series enables the internal phy link detection for lan937x using the
interrupt method. lan937x acts as the interrupt controller for the internal
ports and phy, the irq_domain is registered for the individual ports and in
turn for the individual port interrupts.

RFC v2 -> v3
- Used the interrupt controller implementation of phy link

Changes in RFC v2
- fixed the compilation issue

Arun Ramadoss (3):
  net: dsa: microchip: use dev_ops->reset instead of exit in
    ksz_switch_register
  net: dsa: microchip: add reference to ksz_device inside the ksz_port
  net: dsa: microchip: lan937x: add interrupt support for port phy link

 drivers/net/dsa/microchip/ksz_common.c   |   5 +-
 drivers/net/dsa/microchip/ksz_common.h   |  17 ++
 drivers/net/dsa/microchip/ksz_spi.c      |   2 +
 drivers/net/dsa/microchip/lan937x_main.c | 332 ++++++++++++++++++++++-
 drivers/net/dsa/microchip/lan937x_reg.h  |  12 +
 5 files changed, 361 insertions(+), 7 deletions(-)

-- 
2.36.1

[RFC Patch net-next v3 1/3] net: dsa: microchip: use dev_ops->reset instead of exit in ksz_switch_register

[Arun Ramadoss]

ksz8_switch_exit, ksz9477_switch_exit and lan937x_switch_exit functions
all call the reset function which is assigned to dev_ops->reset hooks.
So instead of calling the dev_ops->exit in ksz_switch_register during
the error condition of dsa_register_switch, dev_ops->reset is used now.
The dev_ops->exit can be extended in lan937x for freeing up the irq
during the ksz_spi_remove. If we add the irq remove in the exit function
and it is called during the dsa_switch_register error condition, kernel
panic happens since irq is setup only in setup operation. To avoid the
kernel panic, dev_ops->reset is used instead of exit.

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 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 6bd69a7e6809..da9bdf753f7a 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1963,7 +1963,7 @@ int ksz_switch_register(struct ksz_device *dev)
 
 	ret = dsa_register_switch(dev->ds);
 	if (ret) {
-		dev->dev_ops->exit(dev);
+		dev->dev_ops->reset(dev);
 		return ret;
 	}
 
-- 
2.36.1

[RFC Patch net-next v3 2/3] net: dsa: microchip: add reference to ksz_device inside the ksz_port

[Arun Ramadoss]

struct ksz_port doesn't have reference to ksz_device as of now. In order
to find out from which port interrupt has triggered, we need to pass the
struct ksz_port as a host data. When the interrupt is triggered, we can
get the port from which interrupt triggered, but to identify it is phy
interrupt we have to read status register. The regmap structure for
accessing the device register is present in the ksz_device struct. To
access the ksz_device from the ksz_port, the reference is added to it
with port number as well.

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 3 +++
 drivers/net/dsa/microchip/ksz_common.h | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index da9bdf753f7a..8714a20da0e7 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1917,6 +1917,9 @@ int ksz_switch_register(struct ksz_device *dev)
 				     GFP_KERNEL);
 		if (!dev->ports[i].mib.counters)
 			return -ENOMEM;
+
+		dev->ports[i].ksz_dev = dev;
+		dev->ports[i].num = i;
 	}
 
 	/* set the real number of ports */
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 0d9520dc6d2d..c675a58ef298 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -16,6 +16,8 @@
 
 #define KSZ_MAX_NUM_PORTS 8
 
+struct ksz_device;
+
 struct vlan_table {
 	u32 table[3];
 };
@@ -82,6 +84,8 @@ struct ksz_port {
 	u16 max_frame;
 	u32 rgmii_tx_val;
 	u32 rgmii_rx_val;
+	struct ksz_device *ksz_dev;
+	u8 num;
 };
 
 struct ksz_device {
-- 
2.36.1

[RFC Patch net-next v3 3/3] net: dsa: microchip: lan937x: add interrupt support for port phy link

[Arun Ramadoss]

This patch enables the interrupts for internal phy link detection for
LAN937x. The interrupt enable bits are active low. There is global
interrupt mask for each port. And each port has the individual interrupt
mask for TAS. QCI, SGMII, PTP, PHY and ACL.
The first level of interrupt domain is registered for global port
interrupt and second level of interrupt domain for the individual port
interrupts. The phy interrupt is enabled in the lan937x_mdio_register
function. Interrupt from which port is raised will be detected based on
the interrupt host data.

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz_common.h   |  13 +
 drivers/net/dsa/microchip/ksz_spi.c      |   2 +
 drivers/net/dsa/microchip/lan937x_main.c | 332 ++++++++++++++++++++++-
 drivers/net/dsa/microchip/lan937x_reg.h  |  12 +
 4 files changed, 353 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index c675a58ef298..991c110a9cd8 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -13,6 +13,7 @@
 #include <linux/phy.h>
 #include <linux/regmap.h>
 #include <net/dsa.h>
+#include <linux/irq.h>
 
 #define KSZ_MAX_NUM_PORTS 8
 
@@ -65,6 +66,14 @@ struct ksz_chip_data {
 	bool internal_phy[KSZ_MAX_NUM_PORTS];
 };
 
+struct ksz_irq {
+	u16 masked;
+	struct irq_chip chip;
+	struct irq_domain *domain;
+	int nirqs;
+	char name[16];
+};
+
 struct ksz_port {
 	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
 	bool learning;
@@ -85,6 +94,7 @@ struct ksz_port {
 	u32 rgmii_tx_val;
 	u32 rgmii_rx_val;
 	struct ksz_device *ksz_dev;
+	struct ksz_irq irq;
 	u8 num;
 };
 
@@ -103,6 +113,7 @@ struct ksz_device {
 	struct regmap *regmap[3];
 
 	void *priv;
+	int irq;
 
 	struct gpio_desc *reset_gpio;	/* Optional reset GPIO */
 
@@ -124,6 +135,8 @@ struct ksz_device {
 	u16 mirror_tx;
 	u32 features;			/* chip specific features */
 	u16 port_mask;
+	struct mutex lock_irq;		/* IRQ Access */
+	struct ksz_irq girq;
 };
 
 /* List of supported models */
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index 05bd089795f8..7ba897b6f950 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -85,6 +85,8 @@ static int ksz_spi_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	dev->irq = spi->irq;
+
 	ret = ksz_switch_register(dev);
 
 	/* Main DSA driver may not be started yet. */
diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
index daedd2bf20c1..3108f8b8c6fc 100644
--- a/drivers/net/dsa/microchip/lan937x_main.c
+++ b/drivers/net/dsa/microchip/lan937x_main.c
@@ -10,6 +10,8 @@
 #include <linux/of_mdio.h>
 #include <linux/if_bridge.h>
 #include <linux/if_vlan.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/math.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
@@ -18,6 +20,8 @@
 #include "ksz_common.h"
 #include "lan937x.h"
 
+#define LAN937x_PNIRQS 6
+
 static int lan937x_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
 {
 	return regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
@@ -171,6 +175,7 @@ static int lan937x_mdio_register(struct ksz_device *dev)
 	struct device_node *mdio_np;
 	struct mii_bus *bus;
 	int ret;
+	int p;
 
 	mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio");
 	if (!mdio_np) {
@@ -194,6 +199,16 @@ static int lan937x_mdio_register(struct ksz_device *dev)
 
 	ds->slave_mii_bus = bus;
 
+	for (p = 0; p < KSZ_MAX_NUM_PORTS; p++) {
+		if (BIT(p) & ds->phys_mii_mask) {
+			unsigned int irq;
+
+			irq = irq_find_mapping(dev->ports[p].irq.domain,
+					       PORT_SRC_PHY_INT);
+			ds->slave_mii_bus->irq[p] = irq;
+		}
+	}
+
 	ret = devm_of_mdiobus_register(ds->dev, bus, mdio_np);
 	if (ret) {
 		dev_err(ds->dev, "unable to register MDIO bus %s\n",
@@ -383,9 +398,287 @@ void lan937x_setup_rgmii_delay(struct ksz_device *dev, int port)
 	}
 }
 
+int lan937x_switch_init(struct ksz_device *dev)
+{
+	dev->port_mask = (1 << dev->info->port_cnt) - 1;
+
+	return 0;
+}
+
+static void lan937x_girq_mask(struct irq_data *d)
+{
+	struct ksz_device *dev = irq_data_get_irq_chip_data(d);
+	unsigned int n = d->hwirq;
+
+	dev->girq.masked |= (1 << n);
+}
+
+static void lan937x_girq_unmask(struct irq_data *d)
+{
+	struct ksz_device *dev = irq_data_get_irq_chip_data(d);
+	unsigned int n = d->hwirq;
+
+	dev->girq.masked &= ~(1 << n);
+}
+
+static void lan937x_girq_bus_lock(struct irq_data *d)
+{
+	struct ksz_device *dev = irq_data_get_irq_chip_data(d);
+
+	mutex_lock(&dev->lock_irq);
+}
+
+static void lan937x_girq_bus_sync_unlock(struct irq_data *d)
+{
+	struct ksz_device *dev = irq_data_get_irq_chip_data(d);
+	int ret;
+
+	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, dev->girq.masked);
+	if (ret)
+		dev_err(dev->dev, "failed to change IRQ mask\n");
+
+	mutex_unlock(&dev->lock_irq);
+}
+
+static const struct irq_chip lan937x_girq_chip = {
+	.name			= "lan937x-global",
+	.irq_mask		= lan937x_girq_mask,
+	.irq_unmask		= lan937x_girq_unmask,
+	.irq_bus_lock		= lan937x_girq_bus_lock,
+	.irq_bus_sync_unlock	= lan937x_girq_bus_sync_unlock,
+};
+
+static int lan937x_girq_domain_map(struct irq_domain *d,
+				   unsigned int irq, irq_hw_number_t hwirq)
+{
+	struct ksz_device *dev = d->host_data;
+
+	irq_set_chip_data(irq, d->host_data);
+	irq_set_chip_and_handler(irq, &dev->girq.chip, handle_level_irq);
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops lan937x_girq_domain_ops = {
+	.map	= lan937x_girq_domain_map,
+	.xlate	= irq_domain_xlate_twocell,
+};
+
+static void lan937x_girq_free(struct ksz_device *dev)
+{
+	int irq, virq;
+
+	for (irq = 0; irq < dev->girq.nirqs; irq++) {
+		virq = irq_find_mapping(dev->girq.domain, irq);
+		irq_dispose_mapping(virq);
+	}
+
+	irq_domain_remove(dev->girq.domain);
+}
+
+static irqreturn_t lan937x_girq_thread_fn(int irq, void *dev_id)
+{
+	struct ksz_device *dev = dev_id;
+	unsigned int nhandled = 0;
+	unsigned int sub_irq;
+	unsigned int n;
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_SW_INT_STATUS__4, &data);
+	if (ret)
+		goto out;
+
+	if (data & POR_READY_INT) {
+		ret = ksz_write32(dev, REG_SW_INT_STATUS__4, POR_READY_INT);
+		if (ret)
+			goto out;
+	}
+
+	/* Read global interrupt status register */
+	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
+	if (ret)
+		goto out;
+
+	for (n = 0; n < dev->girq.nirqs; ++n) {
+		if (data & (1 << n)) {
+			sub_irq = irq_find_mapping(dev->girq.domain, n);
+			handle_nested_irq(sub_irq);
+			++nhandled;
+		}
+	}
+out:
+	return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
+}
+
+static int lan937x_girq_setup(struct ksz_device *dev)
+{
+	int ret, irq;
+
+	dev->girq.nirqs = dev->info->port_cnt;
+	dev->girq.domain = irq_domain_add_simple(NULL, dev->girq.nirqs, 0,
+						 &lan937x_girq_domain_ops, dev);
+	if (!dev->girq.domain)
+		return -ENOMEM;
+
+	for (irq = 0; irq < dev->girq.nirqs; irq++)
+		irq_create_mapping(dev->girq.domain, irq);
+
+	dev->girq.chip = lan937x_girq_chip;
+	dev->girq.masked = ~0;
+
+	ret = request_threaded_irq(dev->irq, NULL, lan937x_girq_thread_fn,
+				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
+				   dev_name(dev->dev), dev);
+	if (ret)
+		goto out;
+
+	return 0;
+
+out:
+	lan937x_girq_free(dev);
+
+	return ret;
+}
+
+static void lan937x_pirq_mask(struct irq_data *d)
+{
+	struct ksz_port *port = irq_data_get_irq_chip_data(d);
+	unsigned int n = d->hwirq;
+
+	port->irq.masked |= (1 << n);
+}
+
+static void lan937x_pirq_unmask(struct irq_data *d)
+{
+	struct ksz_port *port = irq_data_get_irq_chip_data(d);
+	unsigned int n = d->hwirq;
+
+	port->irq.masked &= ~(1 << n);
+}
+
+static void lan937x_pirq_bus_lock(struct irq_data *d)
+{
+	struct ksz_port *port = irq_data_get_irq_chip_data(d);
+	struct ksz_device *dev = port->ksz_dev;
+
+	mutex_lock(&dev->lock_irq);
+}
+
+static void lan937x_pirq_bus_sync_unlock(struct irq_data *d)
+{
+	struct ksz_port *port = irq_data_get_irq_chip_data(d);
+	struct ksz_device *dev = port->ksz_dev;
+
+	ksz_pwrite8(dev, port->num, REG_PORT_INT_MASK, port->irq.masked);
+	mutex_unlock(&dev->lock_irq);
+}
+
+static const struct irq_chip lan937x_pirq_chip = {
+	.name			= "lan937x-port",
+	.irq_mask		= lan937x_pirq_mask,
+	.irq_unmask		= lan937x_pirq_unmask,
+	.irq_bus_lock		= lan937x_pirq_bus_lock,
+	.irq_bus_sync_unlock	= lan937x_pirq_bus_sync_unlock,
+};
+
+static int lan937x_pirq_domain_map(struct irq_domain *d, unsigned int irq,
+				   irq_hw_number_t hwirq)
+{
+	struct ksz_port *port = d->host_data;
+
+	irq_set_chip_data(irq, d->host_data);
+	irq_set_chip_and_handler(irq, &port->irq.chip, handle_level_irq);
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops lan937x_pirq_domain_ops = {
+	.map	= lan937x_pirq_domain_map,
+	.xlate	= irq_domain_xlate_twocell,
+};
+
+static void lan937x_pirq_free(struct ksz_port *port, u8 p)
+{
+	int irq, virq;
+
+	for (irq = 0; irq < port->irq.nirqs; irq++) {
+		virq = irq_find_mapping(port->irq.domain, irq);
+		irq_dispose_mapping(virq);
+	}
+
+	irq_domain_remove(port->irq.domain);
+}
+
+static irqreturn_t lan937x_pirq_thread_fn(int irq, void *dev_id)
+{
+	struct ksz_port *port = dev_id;
+	unsigned int nhandled = 0;
+	struct ksz_device *dev;
+	unsigned int sub_irq;
+	unsigned int n;
+	u8 data;
+
+	dev = port->ksz_dev;
+
+	/* Read global interrupt status register */
+	ksz_pread8(dev, port->num, REG_PORT_INT_STATUS, &data);
+
+	for (n = 0; n < port->irq.nirqs; ++n) {
+		if (data & (1 << n)) {
+			sub_irq = irq_find_mapping(port->irq.domain, n);
+			handle_nested_irq(sub_irq);
+			++nhandled;
+		}
+	}
+
+	return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
+}
+
+static int lan937x_pirq_setup(struct ksz_device *dev, u8 p)
+{
+	struct ksz_port *port = &dev->ports[p];
+	int ret, irq;
+	int irq_num;
+
+	port->irq.nirqs = LAN937x_PNIRQS;
+	port->irq.domain = irq_domain_add_simple(dev->dev->of_node,
+						 port->irq.nirqs, 0,
+						 &lan937x_pirq_domain_ops,
+						 port);
+	if (!port->irq.domain)
+		return -ENOMEM;
+
+	for (irq = 0; irq < port->irq.nirqs; irq++)
+		irq_create_mapping(port->irq.domain, irq);
+
+	port->irq.chip = lan937x_pirq_chip;
+	port->irq.masked = ~0;
+
+	irq_num = irq_find_mapping(dev->girq.domain, p);
+
+	snprintf(port->irq.name, sizeof(port->irq.name), "port_irq-%d", p);
+
+	ret = request_threaded_irq(irq_num, NULL, lan937x_pirq_thread_fn,
+				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
+				   port->irq.name, port);
+	if (ret)
+		goto out;
+
+	return 0;
+
+out:
+	lan937x_pirq_free(port, p);
+
+	return ret;
+}
+
 int lan937x_setup(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
+	struct dsa_port *dp;
 	int ret;
 
 	/* enable Indirect Access from SPI to the VPHY registers */
@@ -395,10 +688,22 @@ int lan937x_setup(struct dsa_switch *ds)
 		return ret;
 	}
 
+	if (dev->irq > 0) {
+		ret = lan937x_girq_setup(dev);
+		if (ret)
+			return ret;
+
+		dsa_switch_for_each_user_port(dp, dev->ds) {
+			ret = lan937x_pirq_setup(dev, dp->index);
+			if (ret)
+				goto out_girq;
+		}
+	}
+
 	ret = lan937x_mdio_register(dev);
 	if (ret < 0) {
 		dev_err(dev->dev, "failed to register the mdio");
-		return ret;
+		goto out_pirq;
 	}
 
 	/* The VLAN aware is a global setting. Mixed vlan
@@ -424,18 +729,33 @@ int lan937x_setup(struct dsa_switch *ds)
 		    (SW_CLK125_ENB | SW_CLK25_ENB), true);
 
 	return 0;
-}
 
-int lan937x_switch_init(struct ksz_device *dev)
-{
-	dev->port_mask = (1 << dev->info->port_cnt) - 1;
+out_pirq:
+	if (dev->irq > 0) {
+		dsa_switch_for_each_user_port(dp, dev->ds) {
+			lan937x_pirq_free(&dev->ports[dp->index], dp->index);
+		}
+	}
+out_girq:
+	if (dev->irq > 0)
+		lan937x_girq_free(dev);
 
-	return 0;
+	return ret;
 }
 
 void lan937x_switch_exit(struct ksz_device *dev)
 {
+	struct dsa_port *dp;
+
 	lan937x_reset_switch(dev);
+
+	if (dev->irq > 0) {
+		dsa_switch_for_each_user_port(dp, dev->ds) {
+			lan937x_pirq_free(&dev->ports[dp->index], dp->index);
+		}
+
+		lan937x_girq_free(dev);
+	}
 }
 
 MODULE_AUTHOR("Arun Ramadoss <arun.ramadoss@microchip.com>");
diff --git a/drivers/net/dsa/microchip/lan937x_reg.h b/drivers/net/dsa/microchip/lan937x_reg.h
index ba4adaddb3ec..a3c669d86e51 100644
--- a/drivers/net/dsa/microchip/lan937x_reg.h
+++ b/drivers/net/dsa/microchip/lan937x_reg.h
@@ -118,6 +118,18 @@
 /* Port Registers */
 
 /* 0 - Operation */
+#define REG_PORT_INT_STATUS		0x001B
+#define REG_PORT_INT_MASK		0x001F
+
+#define PORT_TAS_INT			BIT(5)
+#define PORT_QCI_INT			BIT(4)
+#define PORT_SGMII_INT			BIT(3)
+#define PORT_PTP_INT			BIT(2)
+#define PORT_PHY_INT			BIT(1)
+#define PORT_ACL_INT			BIT(0)
+
+#define PORT_SRC_PHY_INT		1
+
 #define REG_PORT_CTRL_0			0x0020
 
 #define PORT_MAC_LOOPBACK		BIT(7)
-- 
2.36.1

Re: [RFC Patch net-next v3 1/3] net: dsa: microchip: use dev_ops->reset instead of exit in ksz_switch_register

[Andrew Lunn]

On Tue, Aug 30, 2022 at 04:23:01PM +0530, Arun Ramadoss wrote:
> ksz8_switch_exit, ksz9477_switch_exit and lan937x_switch_exit functions
> all call the reset function which is assigned to dev_ops->reset hooks.
> So instead of calling the dev_ops->exit in ksz_switch_register during
> the error condition of dsa_register_switch, dev_ops->reset is used now.
> The dev_ops->exit can be extended in lan937x for freeing up the irq
> during the ksz_spi_remove. If we add the irq remove in the exit function
> and it is called during the dsa_switch_register error condition, kernel
> panic happens since irq is setup only in setup operation. To avoid the
> kernel panic, dev_ops->reset is used instead of exit.
> 
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---
>  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 6bd69a7e6809..da9bdf753f7a 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1963,7 +1963,7 @@ int ksz_switch_register(struct ksz_device *dev)
>  
>  	ret = dsa_register_switch(dev->ds);
>  	if (ret) {
> -		dev->dev_ops->exit(dev);
> +		dev->dev_ops->reset(dev);
>  		return ret;

I don't like this change. You are supposed to be undoing whatever
ksz_switch_register() has done so far. It appears to of called
ops->init() so far, so calling ops->exit() should be the opposite.

If you are making exit() not the opposite of init() in later patches,
it suggests your code structure is wrong. If exit() is not already the
opposite of init() then fix that.

   Andrew

Re: [RFC Patch net-next v3 2/3] net: dsa: microchip: add reference to ksz_device inside the ksz_port

[Andrew Lunn]

On Tue, Aug 30, 2022 at 04:23:02PM +0530, Arun Ramadoss wrote:
> struct ksz_port doesn't have reference to ksz_device as of now. In order
> to find out from which port interrupt has triggered, we need to pass the
> struct ksz_port as a host data. When the interrupt is triggered, we can
> get the port from which interrupt triggered, but to identify it is phy
> interrupt we have to read status register. The regmap structure for
> accessing the device register is present in the ksz_device struct. To
> access the ksz_device from the ksz_port, the reference is added to it
> with port number as well.
> 
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [RFC Patch net-next v3 3/3] net: dsa: microchip: lan937x: add interrupt support for port phy link

[Andrew Lunn]

> @@ -85,6 +94,7 @@ struct ksz_port {
>  	u32 rgmii_tx_val;
>  	u32 rgmii_rx_val;
>  	struct ksz_device *ksz_dev;
> +	struct ksz_irq irq;

Here irq is of type ksz_irq.

>  	u8 num;
>  };
>  
> @@ -103,6 +113,7 @@ struct ksz_device {
>  	struct regmap *regmap[3];
>  
>  	void *priv;
> +	int irq;

Here it is of type int.

>  
>  	struct gpio_desc *reset_gpio;	/* Optional reset GPIO */
>  
> @@ -124,6 +135,8 @@ struct ksz_device {
>  	u16 mirror_tx;
>  	u32 features;			/* chip specific features */
>  	u16 port_mask;
> +	struct mutex lock_irq;		/* IRQ Access */
> +	struct ksz_irq girq;

And here you have the type ksz_irq called qirq. This is going to be
confusing.

I suggest you make the first one called pirq, for port, and then we
have girq for global, and irq is just a number.

>  static int lan937x_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
>  {
>  	return regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
> @@ -171,6 +175,7 @@ static int lan937x_mdio_register(struct ksz_device *dev)
>  	struct device_node *mdio_np;
>  	struct mii_bus *bus;
>  	int ret;
> +	int p;
>  
>  	mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio");
>  	if (!mdio_np) {
> @@ -194,6 +199,16 @@ static int lan937x_mdio_register(struct ksz_device *dev)
>  
>  	ds->slave_mii_bus = bus;
>  
> +	for (p = 0; p < KSZ_MAX_NUM_PORTS; p++) {
> +		if (BIT(p) & ds->phys_mii_mask) {
> +			unsigned int irq;
> +
> +			irq = irq_find_mapping(dev->ports[p].irq.domain,
> +					       PORT_SRC_PHY_INT);

This could return an error code. You really should check for it, the
irq subsystem is not going to be happy with a negative irq number.

> +			ds->slave_mii_bus->irq[p] = irq;
> +		}
> +	}
> +
>  	ret = devm_of_mdiobus_register(ds->dev, bus, mdio_np);
>  	if (ret) {
>  		dev_err(ds->dev, "unable to register MDIO bus %s\n",

I don't see anywhere you destroy the mappings you created above when
the MDIO bus is unregistered. The equivalent of
mv88e6xxx_g2_irq_mdio_free().


> +static irqreturn_t lan937x_girq_thread_fn(int irq, void *dev_id)
> +{
> +	struct ksz_device *dev = dev_id;
> +	unsigned int nhandled = 0;
> +	unsigned int sub_irq;
> +	unsigned int n;
> +	u32 data;
> +	int ret;
> +
> +	ret = ksz_read32(dev, REG_SW_INT_STATUS__4, &data);
> +	if (ret)
> +		goto out;
> +
> +	if (data & POR_READY_INT) {
> +		ret = ksz_write32(dev, REG_SW_INT_STATUS__4, POR_READY_INT);
> +		if (ret)
> +			goto out;
> +	}

What do these two read/writes do? It seems like you are discarding an
interrupt?


> +
> +	/* Read global interrupt status register */
> +	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
> +	if (ret)
> +		goto out;
> +
> +	for (n = 0; n < dev->girq.nirqs; ++n) {
> +		if (data & (1 << n)) {
> +			sub_irq = irq_find_mapping(dev->girq.domain, n);
> +			handle_nested_irq(sub_irq);
> +			++nhandled;
> +		}
> +	}
> +out:
> +	return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
> +}
> +
> +static irqreturn_t lan937x_pirq_thread_fn(int irq, void *dev_id)
> +{
> +	struct ksz_port *port = dev_id;
> +	unsigned int nhandled = 0;
> +	struct ksz_device *dev;
> +	unsigned int sub_irq;
> +	unsigned int n;
> +	u8 data;
> +
> +	dev = port->ksz_dev;
> +
> +	/* Read global interrupt status register */
> +	ksz_pread8(dev, port->num, REG_PORT_INT_STATUS, &data);

I think global here should be port?

> +
> +	for (n = 0; n < port->irq.nirqs; ++n) {
> +		if (data & (1 << n)) {
> +			sub_irq = irq_find_mapping(port->irq.domain, n);
> +			handle_nested_irq(sub_irq);
> +			++nhandled;
> +		}
> +	}
> +
> +	return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
> +}
> +

>  int lan937x_setup(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
> +	struct dsa_port *dp;
>  	int ret;
>  
>  	/* enable Indirect Access from SPI to the VPHY registers */
> @@ -395,10 +688,22 @@ int lan937x_setup(struct dsa_switch *ds)
>  		return ret;
>  	}
>  
> +	if (dev->irq > 0) {
> +		ret = lan937x_girq_setup(dev);
> +		if (ret)
> +			return ret;
> +
> +		dsa_switch_for_each_user_port(dp, dev->ds) {
> +			ret = lan937x_pirq_setup(dev, dp->index);
> +			if (ret)
> +				goto out_girq;
> +		}
> +	}
> +

>  void lan937x_switch_exit(struct ksz_device *dev)
>  {
> +	struct dsa_port *dp;
> +
>  	lan937x_reset_switch(dev);
> +
> +	if (dev->irq > 0) {
> +		dsa_switch_for_each_user_port(dp, dev->ds) {
> +			lan937x_pirq_free(&dev->ports[dp->index], dp->index);
> +		}
> +
> +		lan937x_girq_free(dev);

This is where your problem with exit vs reset is coming from. You
setup all the interrupt code in setup(). But currently there is no
function which is the opposite of setup(). Generally, it is called
tairdown. Add such a function.

	  Andrew

Re: [RFC Patch net-next v3 3/3] net: dsa: microchip: lan937x: add interrupt support for port phy link

[Arun.Ramadoss]

On Tue, 2022-08-30 at 15:25 +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> > @@ -85,6 +94,7 @@ struct ksz_port {
> >       u32 rgmii_tx_val;
> >       u32 rgmii_rx_val;
> >       struct ksz_device *ksz_dev;
> > +     struct ksz_irq irq;
> 
> Here irq is of type ksz_irq.
> 
> >       u8 num;
> >  };
> > 
> > @@ -103,6 +113,7 @@ struct ksz_device {
> >       struct regmap *regmap[3];
> > 
> >       void *priv;
> > +     int irq;
> 
> Here it is of type int.
> 
> > 
> >       struct gpio_desc *reset_gpio;   /* Optional reset GPIO */
> > 
> > @@ -124,6 +135,8 @@ struct ksz_device {
> >       u16 mirror_tx;
> >       u32 features;                   /* chip specific features */
> >       u16 port_mask;
> > +     struct mutex lock_irq;          /* IRQ Access */
> > +     struct ksz_irq girq;
> 
> And here you have the type ksz_irq called qirq. This is going to be
> confusing.
> 
> I suggest you make the first one called pirq, for port, and then we
> have girq for global, and irq is just a number.

Ok. I will update variable names as per the context it is used.

> 
> >  static int lan937x_cfg(struct ksz_device *dev, u32 addr, u8 bits,
> > bool set)
> >  {
> >       return regmap_update_bits(dev->regmap[0], addr, bits, set ?
> > bits : 0);
> > @@ -171,6 +175,7 @@ static int lan937x_mdio_register(struct
> > ksz_device *dev)
> >       struct device_node *mdio_np;
> >       struct mii_bus *bus;
> >       int ret;
> > +     int p;
> > 
> >       mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio");
> >       if (!mdio_np) {
> > @@ -194,6 +199,16 @@ static int lan937x_mdio_register(struct
> > ksz_device *dev)
> > 
> >       ds->slave_mii_bus = bus;
> > 
> > +     for (p = 0; p < KSZ_MAX_NUM_PORTS; p++) {
> > +             if (BIT(p) & ds->phys_mii_mask) {
> > +                     unsigned int irq;
> > +
> > +                     irq = irq_find_mapping(dev-
> > >ports[p].irq.domain,
> > +                                            PORT_SRC_PHY_INT);
> 
> This could return an error code. You really should check for it, the
> irq subsystem is not going to be happy with a negative irq number.

Ok. I will check the return value for irq_find_mapping.

> 
> > +                     ds->slave_mii_bus->irq[p] = irq;
> > +             }
> > +     }
> > +
> >       ret = devm_of_mdiobus_register(ds->dev, bus, mdio_np);
> >       if (ret) {
> >               dev_err(ds->dev, "unable to register MDIO bus %s\n",
> 
> I don't see anywhere you destroy the mappings you created above when
> the MDIO bus is unregistered. The equivalent of
> mv88e6xxx_g2_irq_mdio_free().

I missed to destroy it. I will add.

> 
> 
> > +static irqreturn_t lan937x_girq_thread_fn(int irq, void *dev_id)
> > +{
> > +     struct ksz_device *dev = dev_id;
> > +     unsigned int nhandled = 0;
> > +     unsigned int sub_irq;
> > +     unsigned int n;
> > +     u32 data;
> > +     int ret;
> > +
> > +     ret = ksz_read32(dev, REG_SW_INT_STATUS__4, &data);
> > +     if (ret)
> > +             goto out;
> > +
> > +     if (data & POR_READY_INT) {
> > +             ret = ksz_write32(dev, REG_SW_INT_STATUS__4,
> > POR_READY_INT);
> > +             if (ret)
> > +                     goto out;
> > +     }
> 
> What do these two read/writes do? It seems like you are discarding an
> interrupt?

This interrupt in Power on reset interrupt. It is enabled by default in
the chip. I am not performing any operation based on POR. So I just
cleared the interrupt. Do I need to disable the POR interrupt in the
setup function, since no operation is performed based on it?

> 
> 
> > +
> > +     /* Read global interrupt status register */
> > +     ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
> > +     if (ret)
> > +             goto out;
> > +
> > +     for (n = 0; n < dev->girq.nirqs; ++n) {
> > +             if (data & (1 << n)) {
> > +                     sub_irq = irq_find_mapping(dev->girq.domain,
> > n);
> > +                     handle_nested_irq(sub_irq);
> > +                     ++nhandled;
> > +             }
> > +     }
> > +out:
> > +     return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
> > +}
> > +
> > +static irqreturn_t lan937x_pirq_thread_fn(int irq, void *dev_id)
> > +{
> > +     struct ksz_port *port = dev_id;
> > +     unsigned int nhandled = 0;
> > +     struct ksz_device *dev;
> > +     unsigned int sub_irq;
> > +     unsigned int n;
> > +     u8 data;
> > +
> > +     dev = port->ksz_dev;
> > +
> > +     /* Read global interrupt status register */
> > +     ksz_pread8(dev, port->num, REG_PORT_INT_STATUS, &data);
> 
> I think global here should be port?

Yes. Copy paste problem. I will update it.

> 
> > +
> > +     for (n = 0; n < port->irq.nirqs; ++n) {
> > +             if (data & (1 << n)) {
> > +                     sub_irq = irq_find_mapping(port->irq.domain,
> > n);
> > +                     handle_nested_irq(sub_irq);
> > +                     ++nhandled;
> > +             }
> > +     }
> > +
> > +     return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
> > +}
> > +
> > 
> >  void lan937x_switch_exit(struct ksz_device *dev)
> >  {
> > +     struct dsa_port *dp;
> > +
> >       lan937x_reset_switch(dev);
> > +
> > +     if (dev->irq > 0) {
> > +             dsa_switch_for_each_user_port(dp, dev->ds) {
> > +                     lan937x_pirq_free(&dev->ports[dp->index], dp-
> > >index);
> > +             }
> > +
> > +             lan937x_girq_free(dev);
> 
> This is where your problem with exit vs reset is coming from. You
> setup all the interrupt code in setup(). But currently there is no
> function which is the opposite of setup(). Generally, it is called
> tairdown. Add such a function.

Yes I was looking for opposite function for setup(). Thanks for
suggestion, I will update the code accordingly and remove the patch 1/3
from the series which replaces the exit with reset.

> 
>           Andrew

Re: [RFC Patch net-next v3 3/3] net: dsa: microchip: lan937x: add interrupt support for port phy link

[Andrew Lunn]

> > > +static irqreturn_t lan937x_girq_thread_fn(int irq, void *dev_id)
> > > +{
> > > +     struct ksz_device *dev = dev_id;
> > > +     unsigned int nhandled = 0;
> > > +     unsigned int sub_irq;
> > > +     unsigned int n;
> > > +     u32 data;
> > > +     int ret;
> > > +
> > > +     ret = ksz_read32(dev, REG_SW_INT_STATUS__4, &data);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     if (data & POR_READY_INT) {
> > > +             ret = ksz_write32(dev, REG_SW_INT_STATUS__4,
> > > POR_READY_INT);
> > > +             if (ret)
> > > +                     goto out;
> > > +     }
> > 
> > What do these two read/writes do? It seems like you are discarding an
> > interrupt?
> 
> This interrupt in Power on reset interrupt. It is enabled by default in
> the chip. I am not performing any operation based on POR. So I just
> cleared the interrupt. Do I need to disable the POR interrupt in the
> setup function, since no operation is performed based on it?

It is pretty normal during interrupt controller creation to first
disable all interrupt sources, then clear the interrupt status
register if that can be done with a single operation, and then
register the interrupt controller with the IRQ core. That way, any
outstanding interrupts don't fire.

	    Andrew