diff mbox series

[net-next,v2,06/11] net: dsa: microchip: ksz9477: basic interrupt support

Message ID 20201112153537.22383-7-ceggers@arri.de (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: microchip: PTP support for KSZ956x | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 87 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Christian Eggers Nov. 12, 2020, 3:35 p.m. UTC
Interrupts are required for TX time stamping. Probably they could also
be used for PHY connection status.

This patch only adds the basic infrastructure for interrupts, no
interrupts are actually enabled nor handled.

ksz9477_reset_switch() must be called before requesting the IRQ (in
ksz9477_init() instead of ksz9477_setup()).

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/net/dsa/microchip/ksz9477_i2c.c  |   2 +
 drivers/net/dsa/microchip/ksz9477_main.c | 103 +++++++++++++++++++++--
 drivers/net/dsa/microchip/ksz9477_spi.c  |   2 +
 include/linux/dsa/ksz_common.h           |   1 +
 4 files changed, 100 insertions(+), 8 deletions(-)

Comments

Vladimir Oltean Nov. 12, 2020, 11:26 p.m. UTC | #1
On Thu, Nov 12, 2020 at 04:35:32PM +0100, Christian Eggers wrote:
> Interrupts are required for TX time stamping. Probably they could also
> be used for PHY connection status.

Do the KSZ switches have an internal PHY? And there's a single interrupt
line, shared between the PTP timestamping engine, and the internal PHY
that is driver by phylib?

> This patch only adds the basic infrastructure for interrupts, no
> interrupts are actually enabled nor handled.
>
> ksz9477_reset_switch() must be called before requesting the IRQ (in
> ksz9477_init() instead of ksz9477_setup()).

A patch can never be "too simple". Maybe you could factor out that code
movement into a separate patch.

> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  drivers/net/dsa/microchip/ksz9477_i2c.c  |   2 +
>  drivers/net/dsa/microchip/ksz9477_main.c | 103 +++++++++++++++++++++--
>  drivers/net/dsa/microchip/ksz9477_spi.c  |   2 +
>  include/linux/dsa/ksz_common.h           |   1 +
>  4 files changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
> index 4e053a25d077..4ed1f503044a 100644
> --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -41,6 +41,8 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c,
>  	if (i2c->dev.platform_data)
>  		dev->pdata = i2c->dev.platform_data;
>
> +	dev->irq = i2c->irq;
> +
>  	ret = ksz9477_switch_register(dev);
>
>  	/* Main DSA driver may not be started yet. */
> diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
> index abfd3802bb51..6b5a981fb21f 100644
> --- a/drivers/net/dsa/microchip/ksz9477_main.c
> +++ b/drivers/net/dsa/microchip/ksz9477_main.c
> @@ -7,7 +7,9 @@
>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/interrupt.h>
>  #include <linux/iopoll.h>
> +#include <linux/irq.h>
>  #include <linux/platform_data/microchip-ksz.h>
>  #include <linux/phy.h>
>  #include <linux/if_bridge.h>
> @@ -1345,19 +1347,12 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  static int ksz9477_setup(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
> -	int ret = 0;
>
>  	dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
>  				       dev->num_vlans, GFP_KERNEL);
>  	if (!dev->vlan_cache)
>  		return -ENOMEM;
>
> -	ret = ksz9477_reset_switch(dev);
> -	if (ret) {
> -		dev_err(ds->dev, "failed to reset switch\n");
> -		return ret;
> -	}
> -
>  	/* Required for port partitioning. */
>  	ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
>  		      true);
> @@ -1535,12 +1530,84 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = {
>  	},
>  };
>
> +static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
> +{
> +	struct ksz_device *dev = dev_id;
> +	u32 data;
> +	int port;
> +	int ret;
> +	irqreturn_t result = IRQ_NONE;

Please keep local variable declaration sorted in the reverse order of
line length. But....

> +
> +	/* Read global port interrupt status register */
> +	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
> +	if (ret)
> +		return result;

...Is there any point at all in keeping the "result" variable?

> +
> +	for (port = 0; port < dev->port_cnt; port++) {
> +		if (data & BIT(port)) {

You can reduce the indentation level by 1 here using:

		if (!(data & BIT(port)))
			continue;

> +			u8 data8;
> +
> +			/* Read port interrupt status register */
> +			ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
> +					&data8);
> +			if (ret)
> +				return result;
> +
> +			/* ToDo: Add specific handling of port interrupts */

Buggy? Please return IRQ_HANDLED, otherwise the system, when bisected to
this commit exactly, will emit interrupts and complain that nobody cared.

> +		}
> +	}
> +
> +	return result;
> +}
> +
> +static int ksz9477_enable_port_interrupts(struct ksz_device *dev)
> +{
> +	u32 data;
> +	int ret;
> +
> +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable port interrupts (0 means enabled) */
> +	data &= ~((1 << dev->port_cnt) - 1);

And what's the " - 1" for?

> +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);

> +}
> +
> +static int ksz9477_disable_port_interrupts(struct ksz_device *dev)
> +{
> +	u32 data;
> +	int ret;
> +
> +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable port interrupts (1 means disabled) */
> +	data |= ((1 << dev->port_cnt) - 1);
> +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

same comments as above.

Also, it's almost as if you want to implement these in the same
function, with a "bool enable"?

> +}
> +
>  static int ksz9477_switch_init(struct ksz_device *dev)
>  {
> -	int i;
> +	int i, ret;
>
>  	dev->ds->ops = &ksz9477_switch_ops;
>
> +	ret = ksz9477_reset_switch(dev);
> +	if (ret) {
> +		dev_err(dev->dev, "failed to reset switch\n");
> +		return ret;
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(ksz9477_switch_chips); i++) {
>  		const struct ksz_chip_data *chip = &ksz9477_switch_chips[i];
>
> @@ -1584,12 +1651,32 @@ static int ksz9477_switch_init(struct ksz_device *dev)
>
>  	/* set the real number of ports */
>  	dev->ds->num_ports = dev->port_cnt;
> +	if (dev->irq > 0) {
> +		unsigned long irqflags = irqd_get_trigger_type(irq_get_irq_data(dev->irq));

What is irqd_get_trigger_type and what does it have to do with the
"irqflags" argument of request_threaded_irq? Where else have you even
seen this?

> +
> +		irqflags |= IRQF_ONESHOT;

And shared maybe?

> +		ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
> +						ksz9477_switch_irq_thread,
> +						irqflags,
> +						dev_name(dev->dev),
> +						dev);
> +		if (ret) {
> +			dev_err(dev->dev, "failed to request IRQ.\n");
> +			return ret;
> +		}
> +
> +		ret = ksz9477_enable_port_interrupts(dev);
> +		if (ret)
> +			return ret;

Could you also clear pending interrupts before enabling the line?

> +	}
>
>  	return 0;
>  }
>
>  static void ksz9477_switch_exit(struct ksz_device *dev)
>  {
> +	if (dev->irq > 0)
> +		ksz9477_disable_port_interrupts(dev);

I think it'd look a bit nicer if you moved this condition into
ksz9477_disable_port_interrupts:

	if (!dev->irq)
		return;

>  	ksz9477_reset_switch(dev);
>  }
>
Christian Eggers Nov. 13, 2020, 6:57 p.m. UTC | #2
On Friday, 13 November 2020, 00:26:17 CET, Vladimir Oltean wrote:
> On Thu, Nov 12, 2020 at 04:35:32PM +0100, Christian Eggers wrote:
> > Interrupts are required for TX time stamping. Probably they could also
> > be used for PHY connection status.
> 
> Do the KSZ switches have an internal PHY? And there's a single interrupt
> line, shared between the PTP timestamping engine, and the internal PHY
> that is driver by phylib?
The device has only one interrupt line (INTRP_N), although there may be
applications which use additionally the GPIO (PPS/PEROUT) output as an
interrupt.

I assume that the PHY driver currently uses polling (as the KSZ9477 driver
used to have no interrupt functionality. Maybe this can be changed in future,
as the KSZ hardware has hierarchical interrupt enable/status registers.

> > This patch only adds the basic infrastructure for interrupts, no
> > interrupts are actually enabled nor handled.
> > 
> > ksz9477_reset_switch() must be called before requesting the IRQ (in
> > ksz9477_init() instead of ksz9477_setup()).
> 
> A patch can never be "too simple". Maybe you could factor out that code
> movement into a separate patch.
I haven't checked yet, but I'll try.

[...]

> > +static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
> > +{
> > +	struct ksz_device *dev = dev_id;
> > +	u32 data;
> > +	int port;
> > +	int ret;
> > +	irqreturn_t result = IRQ_NONE;
> 
> Please keep local variable declaration sorted in the reverse order of
> line length. But....
> 
> > +
> > +	/* Read global port interrupt status register */
> > +	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
> > +	if (ret)
> > +		return result;
> 
> ...Is there any point at all in keeping the "result" variable?
> 
> > +
> > +	for (port = 0; port < dev->port_cnt; port++) {
> > +		if (data & BIT(port)) {
> 
> You can reduce the indentation level by 1 here using:
> 
> 		if (!(data & BIT(port)))
> 			continue;
> 
> > +			u8 data8;
> > +
> > +			/* Read port interrupt status register */
> > +			ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
> > +					&data8);
> > +			if (ret)
> > +				return result;
> > +
> > +			/* ToDo: Add specific handling of port interrupts */
> 
> Buggy? Please return IRQ_HANDLED, otherwise the system, when bisected to
> this commit exactly, will emit interrupts and complain that nobody cared.
Probably this can be kept as it is. The hardware will only emit interrupts
if these have been explicitly enabled. Although the *port* interrupts are
enabled here (and all bits in the "Port Interrupt Mask Register" (section
5.2.1.12) are active after reset), actually no interrupts should be raised as
the ports sub units (PTP, PHY and ACL) don't emit interrupt after reset:
- PHY (section 5.2.2.19): All interrupts are disabled after reset
- PTP (section 5.2.11.11): dito
- ACL (not found): I got never interrupts from here

> 
> > +		}
> > +	}
> > +
> > +	return result;
> > +}
> > +
> > +static int ksz9477_enable_port_interrupts(struct ksz_device *dev)
> > +{
> > +	u32 data;
> > +	int ret;
> > +
> > +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Enable port interrupts (0 means enabled) */
> > +	data &= ~((1 << dev->port_cnt) - 1);
> 
> And what's the " - 1" for?
I build a bitmask where the bits 0..(dev->port_cnt-1) are set... I'll whether
GENMASK() can be used with variable data as argument.
> 
> > +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> 	return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> 
> > +}
> > +
> > +static int ksz9477_disable_port_interrupts(struct ksz_device *dev)
> > +{
> > +	u32 data;
> > +	int ret;
> > +
> > +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Disable port interrupts (1 means disabled) */
> > +	data |= ((1 << dev->port_cnt) - 1);
> > +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> same comments as above.
> 
> Also, it's almost as if you want to implement these in the same
> function, with a "bool enable"?
You are right.

> 
> > +}
> > +
> > 
> >  static int ksz9477_switch_init(struct ksz_device *dev)
> >  {
> > 
> > -	int i;
> > +	int i, ret;
> > 
> >  	dev->ds->ops = &ksz9477_switch_ops;
> > 
> > +	ret = ksz9477_reset_switch(dev);
> > +	if (ret) {
> > +		dev_err(dev->dev, "failed to reset switch\n");
> > +		return ret;
> > +	}
> > +
> > 
> >  	for (i = 0; i < ARRAY_SIZE(ksz9477_switch_chips); i++) {
> >  	
> >  		const struct ksz_chip_data *chip = &ksz9477_switch_chips[i];
> > 
> > @@ -1584,12 +1651,32 @@ static int ksz9477_switch_init(struct ksz_device
> > *dev)> 
> >  	/* set the real number of ports */
> >  	dev->ds->num_ports = dev->port_cnt;
> > 
> > +	if (dev->irq > 0) {
> > +		unsigned long irqflags =
> > irqd_get_trigger_type(irq_get_irq_data(dev->irq));
> What is irqd_get_trigger_type and what does it have to do with the
> "irqflags" argument of request_threaded_irq? Where else have you even
> seen this?
No idea where I originally found this. It's some time ago when I wrote this.

> 
> > +
> > +		irqflags |= IRQF_ONESHOT;
> 
> And shared maybe?
I don't need it. Is there a rule when to add shared? At least the KSZ should 
be able to tell whether it has raised an IRQ or not.

> 
> > +		ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
> > +						ksz9477_switch_irq_thread,
> > +						irqflags,
> > +						dev_name(dev->dev),
> > +						dev);
> > +		if (ret) {
> > +			dev_err(dev->dev, "failed to request IRQ.\n");
> > +			return ret;
> > +		}
> > +
> > +		ret = ksz9477_enable_port_interrupts(dev);
> > +		if (ret)
> > +			return ret;
> 
> Could you also clear pending interrupts before enabling the line?
As the device has just been reset and no concrete interrupts have been enabled,
there should be no need for this.

> 
> > +	}
> > 
> >  	return 0;
> >  
> >  }
> >  
> >  static void ksz9477_switch_exit(struct ksz_device *dev)
> >  {
> > 
> > +	if (dev->irq > 0)
> > +		ksz9477_disable_port_interrupts(dev);
> 
> I think it'd look a bit nicer if you moved this condition into
> ksz9477_disable_port_interrupts:
> 
> 	if (!dev->irq)
> 		return;
> 
> >  	ksz9477_reset_switch(dev);
> >  
> >  }

regards
Christian
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 4e053a25d077..4ed1f503044a 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -41,6 +41,8 @@  static int ksz9477_i2c_probe(struct i2c_client *i2c,
 	if (i2c->dev.platform_data)
 		dev->pdata = i2c->dev.platform_data;
 
+	dev->irq = i2c->irq;
+
 	ret = ksz9477_switch_register(dev);
 
 	/* Main DSA driver may not be started yet. */
diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
index abfd3802bb51..6b5a981fb21f 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -7,7 +7,9 @@ 
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/interrupt.h>
 #include <linux/iopoll.h>
+#include <linux/irq.h>
 #include <linux/platform_data/microchip-ksz.h>
 #include <linux/phy.h>
 #include <linux/if_bridge.h>
@@ -1345,19 +1347,12 @@  static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 static int ksz9477_setup(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
-	int ret = 0;
 
 	dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
 				       dev->num_vlans, GFP_KERNEL);
 	if (!dev->vlan_cache)
 		return -ENOMEM;
 
-	ret = ksz9477_reset_switch(dev);
-	if (ret) {
-		dev_err(ds->dev, "failed to reset switch\n");
-		return ret;
-	}
-
 	/* Required for port partitioning. */
 	ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
 		      true);
@@ -1535,12 +1530,84 @@  static const struct ksz_chip_data ksz9477_switch_chips[] = {
 	},
 };
 
+static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
+{
+	struct ksz_device *dev = dev_id;
+	u32 data;
+	int port;
+	int ret;
+	irqreturn_t result = IRQ_NONE;
+
+	/* Read global port interrupt status register */
+	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
+	if (ret)
+		return result;
+
+	for (port = 0; port < dev->port_cnt; port++) {
+		if (data & BIT(port)) {
+			u8 data8;
+
+			/* Read port interrupt status register */
+			ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
+					&data8);
+			if (ret)
+				return result;
+
+			/* ToDo: Add specific handling of port interrupts */
+		}
+	}
+
+	return result;
+}
+
+static int ksz9477_enable_port_interrupts(struct ksz_device *dev)
+{
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
+	if (ret)
+		return ret;
+
+	/* Enable port interrupts (0 means enabled) */
+	data &= ~((1 << dev->port_cnt) - 1);
+	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_disable_port_interrupts(struct ksz_device *dev)
+{
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
+	if (ret)
+		return ret;
+
+	/* Disable port interrupts (1 means disabled) */
+	data |= ((1 << dev->port_cnt) - 1);
+	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int ksz9477_switch_init(struct ksz_device *dev)
 {
-	int i;
+	int i, ret;
 
 	dev->ds->ops = &ksz9477_switch_ops;
 
+	ret = ksz9477_reset_switch(dev);
+	if (ret) {
+		dev_err(dev->dev, "failed to reset switch\n");
+		return ret;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(ksz9477_switch_chips); i++) {
 		const struct ksz_chip_data *chip = &ksz9477_switch_chips[i];
 
@@ -1584,12 +1651,32 @@  static int ksz9477_switch_init(struct ksz_device *dev)
 
 	/* set the real number of ports */
 	dev->ds->num_ports = dev->port_cnt;
+	if (dev->irq > 0) {
+		unsigned long irqflags = irqd_get_trigger_type(irq_get_irq_data(dev->irq));
+
+		irqflags |= IRQF_ONESHOT;
+		ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
+						ksz9477_switch_irq_thread,
+						irqflags,
+						dev_name(dev->dev),
+						dev);
+		if (ret) {
+			dev_err(dev->dev, "failed to request IRQ.\n");
+			return ret;
+		}
+
+		ret = ksz9477_enable_port_interrupts(dev);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
 
 static void ksz9477_switch_exit(struct ksz_device *dev)
 {
+	if (dev->irq > 0)
+		ksz9477_disable_port_interrupts(dev);
 	ksz9477_reset_switch(dev);
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index 1142768969c2..d2eea9596e53 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -48,6 +48,8 @@  static int ksz9477_spi_probe(struct spi_device *spi)
 	if (spi->dev.platform_data)
 		dev->pdata = spi->dev.platform_data;
 
+	dev->irq = spi->irq;
+
 	ret = ksz9477_switch_register(dev);
 
 	/* Main DSA driver may not be started yet. */
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index 3b22380d85c5..bf57ba4b2132 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/ksz_common.h
@@ -55,6 +55,7 @@  struct ksz_device {
 
 	struct device *dev;
 	struct regmap *regmap[3];
+	int irq;
 
 	void *priv;