[PATCH 0/2] mv88e6xxx: Poll when no interrupt defined

[PATCH 0/2] mv88e6xxx: Poll when no interrupt defined

[Andrew Lunn]

Not all boards using the mv88e6xxx switches have the interrupt output
connected to a GPIO. On these boards phylib has to poll the PHYs,
rather than use interrupts. Have the driver poll the interrupt status
register, which is more efficient than having phylib do it. And it
enables other switch interrupts to be services.

The Armada 370RD is such a board without a interrupt GPIO. Now that
interrupts work, wire up the PHYs to make use if them.

Gregory: Are you O.K. for the second patch to go through netdev?

Andrew Lunn (2):
  net: dsa: mv88e6xxx: Poll when no interrupt defined
  arm: mvebu: 370-rd: Enable PHY interrupt handling

 arch/arm/boot/dts/armada-370-rd.dts |  32 ++++++++
 drivers/net/dsa/mv88e6xxx/chip.c    | 146 +++++++++++++++++++++++++-----------
 drivers/net/dsa/mv88e6xxx/chip.h    |   3 +
 3 files changed, 138 insertions(+), 43 deletions(-)

-- 
2.15.1

[PATCH 1/2] net: dsa: mv88e6xxx: Poll when no interrupt defined

[Andrew Lunn]

Not all boards have the interrupt output from the switch connected to
a GPIO line. In such cases, phylib has to poll the internal PHYs,
rather than receive an interrupt when there is a change in the link
state. phylib polls once per second, and per PHY reads around 4
words. With a switch typically having 4 internal PHYs, this means 16
MDIO transactions per second.

Rather than performing this phylib level polling, have the driver poll
the interrupt status register. If the status register indicates an
interrupt condition processing of interrupts in the same way as if a
GPIO was used.

Polling 10 times a second places less load on the MDIO bus. But rather
than taking on average 0.5s to detect a link change, it takes less
than 0.05s. Additionally, other interrupts, such as the watchdog, ATU
and VTU violations will be reported.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 146 +++++++++++++++++++++++++++------------
 drivers/net/dsa/mv88e6xxx/chip.h |   3 +
 2 files changed, 106 insertions(+), 43 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index e1b5c5c66fce..24486f96dd39 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -253,9 +253,8 @@ static void mv88e6xxx_g1_irq_unmask(struct irq_data *d)
 	chip->g1_irq.masked &= ~(1 << n);
 }
 
-static irqreturn_t mv88e6xxx_g1_irq_thread_fn(int irq, void *dev_id)
+static irqreturn_t mv88e6xxx_g1_irq_thread_work(struct mv88e6xxx_chip *chip)
 {
-	struct mv88e6xxx_chip *chip = dev_id;
 	unsigned int nhandled = 0;
 	unsigned int sub_irq;
 	unsigned int n;
@@ -280,6 +279,13 @@ static irqreturn_t mv88e6xxx_g1_irq_thread_fn(int irq, void *dev_id)
 	return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
 }
 
+static irqreturn_t mv88e6xxx_g1_irq_thread_fn(int irq, void *dev_id)
+{
+	struct mv88e6xxx_chip *chip = dev_id;
+
+	return mv88e6xxx_g1_irq_thread_work(chip);
+}
+
 static void mv88e6xxx_g1_irq_bus_lock(struct irq_data *d)
 {
 	struct mv88e6xxx_chip *chip = irq_data_get_irq_chip_data(d);
@@ -335,7 +341,7 @@ static const struct irq_domain_ops mv88e6xxx_g1_irq_domain_ops = {
 	.xlate	= irq_domain_xlate_twocell,
 };
 
-static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
+static void mv88e6xxx_g1_irq_free_common(struct mv88e6xxx_chip *chip)
 {
 	int irq, virq;
 	u16 mask;
@@ -344,8 +350,6 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
 	mask &= ~GENMASK(chip->g1_irq.nirqs, 0);
 	mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, mask);
 
-	free_irq(chip->irq, chip);
-
 	for (irq = 0; irq < chip->g1_irq.nirqs; irq++) {
 		virq = irq_find_mapping(chip->g1_irq.domain, irq);
 		irq_dispose_mapping(virq);
@@ -354,7 +358,14 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
 	irq_domain_remove(chip->g1_irq.domain);
 }
 
-static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
+static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
+{
+	mv88e6xxx_g1_irq_free(chip);
+
+	free_irq(chip->irq, chip);
+}
+
+static int mv88e6xxx_g1_irq_setup_common(struct mv88e6xxx_chip *chip)
 {
 	int err, irq, virq;
 	u16 reg, mask;
@@ -387,13 +398,6 @@ static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
 	if (err)
 		goto out_disable;
 
-	err = request_threaded_irq(chip->irq, NULL,
-				   mv88e6xxx_g1_irq_thread_fn,
-				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
-				   dev_name(chip->dev), chip);
-	if (err)
-		goto out_disable;
-
 	return 0;
 
 out_disable:
@@ -411,6 +415,62 @@ static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
 	return err;
 }
 
+static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
+{
+	int err;
+
+	err = mv88e6xxx_g1_irq_setup_common(chip);
+	if (err)
+		return err;
+
+	err = request_threaded_irq(chip->irq, NULL,
+				   mv88e6xxx_g1_irq_thread_fn,
+				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
+				   dev_name(chip->dev), chip);
+	if (err)
+		mv88e6xxx_g1_irq_free_common(chip);
+
+	return err;
+}
+
+static void mv88e6xxx_irq_poll(struct kthread_work *work)
+{
+	struct mv88e6xxx_chip *chip = container_of(work,
+						   struct mv88e6xxx_chip,
+						   irq_poll_work.work);
+	mv88e6xxx_g1_irq_thread_work(chip);
+
+	kthread_queue_delayed_work(chip->kworker, &chip->irq_poll_work,
+				   msecs_to_jiffies(100));
+}
+
+static int mv88e6xxx_irq_poll_setup(struct mv88e6xxx_chip *chip)
+{
+	int err;
+
+	err = mv88e6xxx_g1_irq_setup_common(chip);
+	if (err)
+		return err;
+
+	kthread_init_delayed_work(&chip->irq_poll_work,
+				  mv88e6xxx_irq_poll);
+
+	chip->kworker = kthread_create_worker(0, dev_name(chip->dev));
+	if (IS_ERR(chip->kworker))
+		return PTR_ERR(chip->kworker);
+
+	kthread_queue_delayed_work(chip->kworker, &chip->irq_poll_work,
+				   msecs_to_jiffies(100));
+
+	return 0;
+}
+
+static void mv88e6xxx_irq_poll_free(struct mv88e6xxx_chip *chip)
+{
+	kthread_cancel_delayed_work_sync(&chip->irq_poll_work);
+	kthread_destroy_worker(chip->kworker);
+}
+
 int mv88e6xxx_wait(struct mv88e6xxx_chip *chip, int addr, int reg, u16 mask)
 {
 	int i;
@@ -4034,33 +4094,34 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 		goto out;
 	}
 
-	if (chip->irq > 0) {
-		/* Has to be performed before the MDIO bus is created,
-		 * because the PHYs will link there interrupts to these
-		 * interrupt controllers
-		 */
-		mutex_lock(&chip->reg_lock);
+	/* Has to be performed before the MDIO bus is created, because
+	 * the PHYs will link there interrupts to these interrupt
+	 * controllers
+	 */
+	mutex_lock(&chip->reg_lock);
+	if (chip->irq > 0)
 		err = mv88e6xxx_g1_irq_setup(chip);
-		mutex_unlock(&chip->reg_lock);
-
-		if (err)
-			goto out;
-
-		if (chip->info->g2_irqs > 0) {
-			err = mv88e6xxx_g2_irq_setup(chip);
-			if (err)
-				goto out_g1_irq;
-		}
+	else
+		err = mv88e6xxx_irq_poll_setup(chip);
+	mutex_unlock(&chip->reg_lock);
 
-		err = mv88e6xxx_g1_atu_prob_irq_setup(chip);
-		if (err)
-			goto out_g2_irq;
+	if (err)
+		goto out;
 
-		err = mv88e6xxx_g1_vtu_prob_irq_setup(chip);
+	if (chip->info->g2_irqs > 0) {
+		err = mv88e6xxx_g2_irq_setup(chip);
 		if (err)
-			goto out_g1_atu_prob_irq;
+			goto out_g1_irq;
 	}
 
+	err = mv88e6xxx_g1_atu_prob_irq_setup(chip);
+	if (err)
+		goto out_g2_irq;
+
+	err = mv88e6xxx_g1_vtu_prob_irq_setup(chip);
+	if (err)
+		goto out_g1_atu_prob_irq;
+
 	err = mv88e6xxx_mdios_register(chip, np);
 	if (err)
 		goto out_g1_vtu_prob_irq;
@@ -4074,20 +4135,19 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 out_mdio:
 	mv88e6xxx_mdios_unregister(chip);
 out_g1_vtu_prob_irq:
-	if (chip->irq > 0)
-		mv88e6xxx_g1_vtu_prob_irq_free(chip);
+	mv88e6xxx_g1_vtu_prob_irq_free(chip);
 out_g1_atu_prob_irq:
-	if (chip->irq > 0)
-		mv88e6xxx_g1_atu_prob_irq_free(chip);
+	mv88e6xxx_g1_atu_prob_irq_free(chip);
 out_g2_irq:
-	if (chip->info->g2_irqs > 0 && chip->irq > 0)
+	if (chip->info->g2_irqs > 0)
 		mv88e6xxx_g2_irq_free(chip);
 out_g1_irq:
-	if (chip->irq > 0) {
-		mutex_lock(&chip->reg_lock);
+	mutex_lock(&chip->reg_lock);
+	if (chip->irq > 0)
 		mv88e6xxx_g1_irq_free(chip);
-		mutex_unlock(&chip->reg_lock);
-	}
+	else
+		mv88e6xxx_irq_poll_free(chip);
+	mutex_unlock(&chip->reg_lock);
 out:
 	return err;
 }
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 97d7915f32c7..d6a1391dc268 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -15,6 +15,7 @@
 #include <linux/if_vlan.h>
 #include <linux/irq.h>
 #include <linux/gpio/consumer.h>
+#include <linux/kthread.h>
 #include <linux/phy.h>
 #include <linux/ptp_clock_kernel.h>
 #include <linux/timecounter.h>
@@ -245,6 +246,8 @@ struct mv88e6xxx_chip {
 	int watchdog_irq;
 	int atu_prob_irq;
 	int vtu_prob_irq;
+	struct kthread_worker *kworker;
+	struct kthread_delayed_work irq_poll_work;
 
 	/* GPIO resources */
 	u8 gpio_data[2];
-- 
2.15.1

[PATCH 2/2] arm: mvebu: 370-rd: Enable PHY interrupt handling

[Andrew Lunn]

The Ethernet switch has an embedded interrupt controller. Interrupts
from the embedded PHYs are part of this interrupt controller.
Explicitly list the MDIO bus the embedded PHYs are on, and wire up the
interrupts.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/boot/dts/armada-370-rd.dts | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
index 8b2fa9a49967..c28afb242393 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -56,6 +56,7 @@
 
 /dts-v1/;
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/gpio/gpio.h>
 #include "armada-370.dtsi"
 
@@ -243,6 +244,8 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 		reg = <0x10>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
 
 		ports {
 			#address-cells = <1>;
@@ -278,6 +281,35 @@
 				};
 			};
 		};
+
+		mdio {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			switchphy0: switchphy@0 {
+				reg = <0>;
+				interrupt-parent = <&switch>;
+				interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+			};
+
+			switchphy1: switchphy@1 {
+				reg = <1>;
+				interrupt-parent = <&switch>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
+			};
+
+			switchphy2: switchphy@2 {
+				reg = <2>;
+				interrupt-parent = <&switch>;
+				interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
+			};
+
+			switchphy3: switchphy@3 {
+				reg = <3>;
+				interrupt-parent = <&switch>;
+				interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
+			};
+		};
 	};
 };
 
-- 
2.15.1

Re: [PATCH 0/2] mv88e6xxx: Poll when no interrupt defined

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 22 Feb 2018 22:58:31 +0100

> Not all boards using the mv88e6xxx switches have the interrupt output
> connected to a GPIO. On these boards phylib has to poll the PHYs,
> rather than use interrupts. Have the driver poll the interrupt status
> register, which is more efficient than having phylib do it. And it
> enables other switch interrupts to be services.
> 
> The Armada 370RD is such a board without a interrupt GPIO. Now that
> interrupts work, wire up the PHYs to make use if them.
> 
> Gregory: Are you O.K. for the second patch to go through netdev?

I'll wait for Gregory's response before applying this series.

Re: [PATCH 0/2] mv88e6xxx: Poll when no interrupt defined

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 22 Feb 2018 22:58:31 +0100

> Not all boards using the mv88e6xxx switches have the interrupt output
> connected to a GPIO. On these boards phylib has to poll the PHYs,
> rather than use interrupts. Have the driver poll the interrupt status
> register, which is more efficient than having phylib do it. And it
> enables other switch interrupts to be services.
> 
> The Armada 370RD is such a board without a interrupt GPIO. Now that
> interrupts work, wire up the PHYs to make use if them.
> 
> Gregory: Are you O.K. for the second patch to go through netdev?

Still no response from Gregory after 4+ days, so I'm applying this
to net-next.

Thanks Andrew.

Re: [PATCH 0/2] mv88e6xxx: Poll when no interrupt defined

[Andrew Lunn]

On Mon, Feb 26, 2018 at 11:29:58AM -0500, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Thu, 22 Feb 2018 22:58:31 +0100
> 
> > Not all boards using the mv88e6xxx switches have the interrupt output
> > connected to a GPIO. On these boards phylib has to poll the PHYs,
> > rather than use interrupts. Have the driver poll the interrupt status
> > register, which is more efficient than having phylib do it. And it
> > enables other switch interrupts to be services.
> > 
> > The Armada 370RD is such a board without a interrupt GPIO. Now that
> > interrupts work, wire up the PHYs to make use if them.
> > 
> > Gregory: Are you O.K. for the second patch to go through netdev?
> 
> Still no response from Gregory after 4+ days, so I'm applying this
> to net-next.

Hi David

I pinged Gregory earlier today. He said he was on vacation last week,
and you get to it later today or tomorrow.

    Andrew

Re: [PATCH 0/2] mv88e6xxx: Poll when no interrupt defined

[Gregory CLEMENT]

Hi Andrew,
 
 On jeu., févr. 22 2018, Andrew Lunn <andrew@lunn.ch> wrote:

> Not all boards using the mv88e6xxx switches have the interrupt output
> connected to a GPIO. On these boards phylib has to poll the PHYs,
> rather than use interrupts. Have the driver poll the interrupt status
> register, which is more efficient than having phylib do it. And it
> enables other switch interrupts to be services.
>
> The Armada 370RD is such a board without a interrupt GPIO. Now that
> interrupts work, wire up the PHYs to make use if them.
>
> Gregory: Are you O.K. for the second patch to go through netdev?

Why do you need that the second patch to go through netdev. Is there any
dependency between the 2 patches?

If it is the case does it means that an new kernel won't work with an
old device tree?

Gregory


>
> Andrew Lunn (2):
>   net: dsa: mv88e6xxx: Poll when no interrupt defined
>   arm: mvebu: 370-rd: Enable PHY interrupt handling
>
>  arch/arm/boot/dts/armada-370-rd.dts |  32 ++++++++
>  drivers/net/dsa/mv88e6xxx/chip.c    | 146 +++++++++++++++++++++++++-----------
>  drivers/net/dsa/mv88e6xxx/chip.h    |   3 +
>  3 files changed, 138 insertions(+), 43 deletions(-)
>
> -- 
> 2.15.1
>

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 0/2] mv88e6xxx: Poll when no interrupt defined

[Andrew Lunn]

On Tue, Feb 27, 2018 at 11:24:02AM +0100, Gregory CLEMENT wrote:
> Hi Andrew,
>  
>  On jeu., f�vr. 22 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > Not all boards using the mv88e6xxx switches have the interrupt output
> > connected to a GPIO. On these boards phylib has to poll the PHYs,
> > rather than use interrupts. Have the driver poll the interrupt status
> > register, which is more efficient than having phylib do it. And it
> > enables other switch interrupts to be services.
> >
> > The Armada 370RD is such a board without a interrupt GPIO. Now that
> > interrupts work, wire up the PHYs to make use if them.
> >
> > Gregory: Are you O.K. for the second patch to go through netdev?
> 
> Why do you need that the second patch to go through netdev. Is there any
> dependency between the 2 patches?
> 
> If it is the case does it means that an new kernel won't work with an
> old device tree?

Hi Gregory

There is a runtime dependency between the two. A new device tree blob
will not run on an old kernel. So if you take the second patch alone
via mvebu, the PHYs will stop working, no link up reported.

But an old blob will run on a new kernel. Backwards compatibility is
maintained.

	Andrew

Re: [PATCH 0/2] mv88e6xxx: Poll when no interrupt defined

[Gregory CLEMENT]

Hi Andrew,
 
 On mar., févr. 27 2018, Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Feb 27, 2018 at 11:24:02AM +0100, Gregory CLEMENT wrote:
>> Hi Andrew,
>>  
>>  On jeu., févr. 22 2018, Andrew Lunn <andrew@lunn.ch> wrote:
>> 
>> > Not all boards using the mv88e6xxx switches have the interrupt output
>> > connected to a GPIO. On these boards phylib has to poll the PHYs,
>> > rather than use interrupts. Have the driver poll the interrupt status
>> > register, which is more efficient than having phylib do it. And it
>> > enables other switch interrupts to be services.
>> >
>> > The Armada 370RD is such a board without a interrupt GPIO. Now that
>> > interrupts work, wire up the PHYs to make use if them.
>> >
>> > Gregory: Are you O.K. for the second patch to go through netdev?
>> 
>> Why do you need that the second patch to go through netdev. Is there any
>> dependency between the 2 patches?
>> 
>> If it is the case does it means that an new kernel won't work with an
>> old device tree?
>
> Hi Gregory
>
> There is a runtime dependency between the two. A new device tree blob
> will not run on an old kernel. So if you take the second patch alone
> via mvebu, the PHYs will stop working, no link up reported.
>
> But an old blob will run on a new kernel. Backwards compatibility is
> maintained.

Ok so you wanted to keep the kernel bisctable.

So I'm fine with having this patch in netdev (and I think it is alrdeay
the case).

Gregory

>
> 	Andrew

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com