[PATCH 0/4] net: dsa: mv88e6xxx: novice fixes and irq handling

[PATCH 0/4] net: dsa: mv88e6xxx: novice fixes and irq handling

[Uwe Kleine-König]

Hello,

this is a set of patches I created while I tried to understand how DSA
works. I don't claim I already got there and so the first 3 fixes are
trivial only.

I tried to make the switch on the espressobin SBC use the interrupt line
and needed the fourth patch to make this work. Given I don't have access
to the documentation of the Marvell switch, the patch is probably not
correct though (and so doesn't have a S-o-b).

When I first added the irq as:

	interrupt-parent = <&gpiosb>;
	interrupts = <23 IRQ_TYPE_LEVEL_LOW>;

to the switch of the espressobin's dtb, the irq couldn't be used. The
reason is the interaction of several things:

 - On the first try to probe the switch, the driver did:

	irq = of_irq_get(np, 0);
	request_threaded_irq(irq, NULL, func, IRQF_ONESHOT | IRQF_TRIGGER_FALLING, ...);

   and then later aborted with -EPROBE_DEFER because another resource
   wasn't available yet. This correctly undid the request_threaded_irq
   above using free_irq.

 - When the probe routine was entered again, the call to of_irq_get(np, 0)
   failed becauce the irq code remembered that the irq was requested
   using edge triggered logic with:

	irq: type mismatch, failed to map hwirq-23 for gpio!

   (kernel/irq/irqdomain.c, line 801)

Any of the following changes would make the problem disappear:

 - use IRQ_TYPE_EDGE_FALLING in the device tree;
 - drop IRQF_TRIGGER_FALLING from the driver's use of
   request_threaded_irq() (I think, see below); and
 - make the irq code forget the trigger type when the last action is
   removed

I think both the second and the third should be done.

For the third, after a quick look into kernel/irq I didn't find a
suitable place where to do that and so it would be great to get some
feedback from Marc or Thomas here.

Regarding the first one: Is the irq of the switch really edge sensitive?
I didn't see the line reset to 1 after an irq was triggered. (And again,
I don't have access on the documentation of the switch.) When I tested
the second change however the driver still failed because the
gpio controller doesn't support level sensitive irqs. :-|

Looking forward to your comments
Uwe Kleine-Köníg

Uwe Kleine-König (4):
  net: dsa: mv88e6xxx: Fix name of switch 88E6141
  net: dsa: mv88e6xxx: Fix typo in a comment
  net: dsa: mv88e6xxx: Fix interrupt name for g2 irq
  net: dsa: mv88e6xxx: Guess number of g1 irqs

 drivers/net/dsa/mv88e6xxx/chip.c    | 6 ++++--
 drivers/net/dsa/mv88e6xxx/global2.c | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.16.2

[PATCH 1/4] net: dsa: mv88e6xxx: Fix name of switch 88E6141

[Uwe Kleine-König]

The switch name is emitted in the kernel log, so having the right name
there is nice.

Fixes: 1558727a1c1b ("net: dsa: mv88e6xxx: Add support for ethernet switch 88E6141")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 41f872d4ba3c..e2d3df9908e0 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3410,7 +3410,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 	[MV88E6141] = {
 		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6141,
 		.family = MV88E6XXX_FAMILY_6341,
-		.name = "Marvell 88E6341",
+		.name = "Marvell 88E6141",
 		.num_databases = 4096,
 		.num_ports = 6,
 		.num_gpio = 11,
-- 
2.16.2

[PATCH 2/4] net: dsa: mv88e6xxx: fix typo in a comment

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index e2d3df9908e0..38fe875b9631 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4133,7 +4133,7 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	}
 
 	/* Has to be performed before the MDIO bus is created, because
-	 * the PHYs will link there interrupts to these interrupt
+	 * the PHYs will link their interrupts to these interrupt
 	 * controllers
 	 */
 	mutex_lock(&chip->reg_lock);
-- 
2.16.2

[PATCH 3/4] net: dsa: mv88e6xxx: fix interrupt name for g2 irq

[Uwe Kleine-König]

This changes the respective line in /proc/interrupts from

 49:          x          x  mv88e6xxx-g1   7 Edge      mv88e6xxx-g1

to

 49:          x          x  mv88e6xxx-g1   7 Edge      mv88e6xxx-g2

which makes more sense.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/dsa/mv88e6xxx/global2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
index 5f370f1fc7c4..e047f1d355a8 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -1090,7 +1090,7 @@ int mv88e6xxx_g2_irq_setup(struct mv88e6xxx_chip *chip)
 
 	err = request_threaded_irq(chip->device_irq, NULL,
 				   mv88e6xxx_g2_irq_thread_fn,
-				   IRQF_ONESHOT, "mv88e6xxx-g1", chip);
+				   IRQF_ONESHOT, "mv88e6xxx-g2", chip);
 	if (err)
 		goto out;
 
-- 
2.16.2

[PATCH 4/4] net: dsa: mv88e6xxx: guess number of g1 irqs

[Uwe Kleine-König]

I don't have access to the documentation of the marvell switches, but
with this change the switch driver successfully binds on the
espressobin after its device tree makes use of the switch's irq line.
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 38fe875b9631..b3696540e002 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3420,6 +3420,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.global2_addr = 0x1c,
 		.age_time_coeff = 3750,
 		.atu_move_port_mask = 0x1f,
+		.g1_irqs = 9,
 		.g2_irqs = 10,
 		.pvt = true,
 		.multi_chip = true,
@@ -3728,6 +3729,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.global2_addr = 0x1c,
 		.age_time_coeff = 3750,
 		.atu_move_port_mask = 0x1f,
+		.g1_irqs = 9,
 		.g2_irqs = 10,
 		.pvt = true,
 		.multi_chip = true,
-- 
2.16.2

Re: [PATCH 1/4] net: dsa: mv88e6xxx: Fix name of switch 88E6141

[Andrew Lunn]

On Mon, Mar 19, 2018 at 11:05:20AM +0100, Uwe Kleine-K�nig wrote:
> The switch name is emitted in the kernel log, so having the right name
> there is nice.
> 
> Fixes: 1558727a1c1b ("net: dsa: mv88e6xxx: Add support for ethernet switch 88E6141")
> Signed-off-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>

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

    Andrew

Re: [PATCH 2/4] net: dsa: mv88e6xxx: fix typo in a comment

[Andrew Lunn]

On Mon, Mar 19, 2018 at 11:05:21AM +0100, Uwe Kleine-K�nig wrote:
> Signed-off-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>

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

    Andrew

Re: [PATCH 3/4] net: dsa: mv88e6xxx: fix interrupt name for g2 irq

[Andrew Lunn]

On Mon, Mar 19, 2018 at 11:05:22AM +0100, Uwe Kleine-K�nig wrote:
> This changes the respective line in /proc/interrupts from
> 
>  49:          x          x  mv88e6xxx-g1   7 Edge      mv88e6xxx-g1
> 
> to
> 
>  49:          x          x  mv88e6xxx-g1   7 Edge      mv88e6xxx-g2
> 
> which makes more sense.
> 
> Signed-off-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>

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

    Andrew

Re: [PATCH 4/4] net: dsa: mv88e6xxx: guess number of g1 irqs

[Andrew Lunn]

On Mon, Mar 19, 2018 at 11:05:23AM +0100, Uwe Kleine-K�nig wrote:
> I don't have access to the documentation of the marvell switches, but
> with this change the switch driver successfully binds on the
> espressobin after its device tree makes use of the switch's irq line.

DaveM just accepted a patch like this from me.

      Andrew

> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 38fe875b9631..b3696540e002 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -3420,6 +3420,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
>  		.global2_addr = 0x1c,
>  		.age_time_coeff = 3750,
>  		.atu_move_port_mask = 0x1f,
> +		.g1_irqs = 9,
>  		.g2_irqs = 10,
>  		.pvt = true,
>  		.multi_chip = true,
> @@ -3728,6 +3729,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
>  		.global2_addr = 0x1c,
>  		.age_time_coeff = 3750,
>  		.atu_move_port_mask = 0x1f,
> +		.g1_irqs = 9,
>  		.g2_irqs = 10,
>  		.pvt = true,
>  		.multi_chip = true,
> -- 
> 2.16.2
> 

Re: [PATCH 0/4] net: dsa: mv88e6xxx: novice fixes and irq handling

[Andrew Lunn]

On Mon, Mar 19, 2018 at 11:05:19AM +0100, Uwe Kleine-K�nig wrote:
> Hello,
> 
> this is a set of patches I created while I tried to understand how DSA
> works. I don't claim I already got there and so the first 3 fixes are
> trivial only.
> 
> I tried to make the switch on the espressobin SBC use the interrupt line
> and needed the fourth patch to make this work. Given I don't have access
> to the documentation of the Marvell switch, the patch is probably not
> correct though (and so doesn't have a S-o-b).
> 
> When I first added the irq as:
> 
> 	interrupt-parent = <&gpiosb>;
> 	interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> 
> to the switch of the espressobin's dtb, the irq couldn't be used. The
> reason is the interaction of several things:
> 
>  - On the first try to probe the switch, the driver did:
> 
> 	irq = of_irq_get(np, 0);
> 	request_threaded_irq(irq, NULL, func, IRQF_ONESHOT | IRQF_TRIGGER_FALLING, ...);
> 
>    and then later aborted with -EPROBE_DEFER because another resource
>    wasn't available yet. This correctly undid the request_threaded_irq
>    above using free_irq.
> 
>  - When the probe routine was entered again, the call to of_irq_get(np, 0)
>    failed becauce the irq code remembered that the irq was requested
>    using edge triggered logic with:
> 
> 	irq: type mismatch, failed to map hwirq-23 for gpio!
> 
>    (kernel/irq/irqdomain.c, line 801)
> 
> Any of the following changes would make the problem disappear:
> 
>  - use IRQ_TYPE_EDGE_FALLING in the device tree;
>  - drop IRQF_TRIGGER_FALLING from the driver's use of
>    request_threaded_irq() (I think, see below); and
>  - make the irq code forget the trigger type when the last action is
>    removed

Hi Uwe

I think the correct fix here is to use the flag in device tree when
requesting the interrupt. If it says IRQ_TYPE_LEVEL_LOW, then request
it active low.

Thinking about the hardware, active low is probably right, not edge.
It will stay low until all the sources of interrupts are cleared.

   Andrew

Re: [PATCH 0/4] net: dsa: mv88e6xxx: novice fixes and irq handling

[Andrew Lunn]

> 	interrupt-parent = <&gpiosb>;
> 	interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> 
> to the switch of the espressobin's dtb, the irq couldn't be used. The
> reason is the interaction of several things:
> 
>  - On the first try to probe the switch, the driver did:
> 
> 	irq = of_irq_get(np, 0);
> 	request_threaded_irq(irq, NULL, func, IRQF_ONESHOT | IRQF_TRIGGER_FALLING, ...);

Hi Uwe

It looks like we should just drop IRQF_TRIGGER_FALLING from here. The
interrupt trigger will be configured by of_irq_get(). All the current
DT files actually list it as IRQ_TYPE_LEVEL_LOW.

> When I tested the second change however the driver still failed
> because the gpio controller doesn't support level sensitive
> irqs. :-|

Do you have documentation for the SoC? Is it a hardware limitation, or
just missing from the pinctrl driver?

     Andrew

Re: [PATCH 0/4] net: dsa: mv88e6xxx: novice fixes and irq handling

[Uwe Kleine-König]

Hello Andrew,

On Mon, Mar 19, 2018 at 05:37:13PM +0100, Andrew Lunn wrote:
> > When I tested the second change however the driver still failed
> > because the gpio controller doesn't support level sensitive
> > irqs. :-|
> 
> Do you have documentation for the SoC? Is it a hardware limitation, or
> just missing from the pinctrl driver?

I didn't check yet. This is an Armada 3720, the hardware specification
is available from Marvell. I'll take a look after resending my series
with your Reviewed-by tags.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |