[2/2] ARM: dts: ls1021a-tsn: Use interrupts for the SGMII PHYs
diff mbox series

Message ID 20191112132010.18274-3-linux@rasmusvillemoes.dk
State New
Headers show
Series
  • ARM: dts: ls1021a: define and use external interrupt lines
Related show

Commit Message

Rasmus Villemoes Nov. 12, 2019, 1:20 p.m. UTC
From: Vladimir Oltean <olteanv@gmail.com>

On the LS1021A-TSN board, the 2 Atheros AR8031 PHYs for eth0 and eth1
have interrupt lines connected to the shared IRQ2_B LS1021A pin.

Switching to interrupts offloads the PHY library from the task of
polling the MDIO status and AN registers (1, 4, 5) every second.

Unfortunately, the BCM5464R quad PHY connected to the switch does not
appear to have an interrupt line routed to the SoC.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 arch/arm/boot/dts/ls1021a-tsn.dts | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Vladimir Oltean Nov. 12, 2019, 1:44 p.m. UTC | #1
On Tue, 12 Nov 2019 at 15:20, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
> From: Vladimir Oltean <olteanv@gmail.com>
>
> On the LS1021A-TSN board, the 2 Atheros AR8031 PHYs for eth0 and eth1
> have interrupt lines connected to the shared IRQ2_B LS1021A pin.
>
> Switching to interrupts offloads the PHY library from the task of
> polling the MDIO status and AN registers (1, 4, 5) every second.
>
> Unfortunately, the BCM5464R quad PHY connected to the switch does not
> appear to have an interrupt line routed to the SoC.
>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  arch/arm/boot/dts/ls1021a-tsn.dts | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts b/arch/arm/boot/dts/ls1021a-tsn.dts
> index 5b7689094b70..135d36461af4 100644
> --- a/arch/arm/boot/dts/ls1021a-tsn.dts
> +++ b/arch/arm/boot/dts/ls1021a-tsn.dts
> @@ -203,11 +203,15 @@
>         /* AR8031 */
>         sgmii_phy1: ethernet-phy@1 {
>                 reg = <0x1>;
> +               /* SGMII1_PHY_INT_B: connected to IRQ2, active low */
> +               interrupts-extended = <&extirq 2 IRQ_TYPE_EDGE_FALLING>;
>         };
>
>         /* AR8031 */
>         sgmii_phy2: ethernet-phy@2 {
>                 reg = <0x2>;
> +               /* SGMII2_PHY_INT_B: connected to IRQ2, active low */
> +               interrupts-extended = <&extirq 2 IRQ_TYPE_EDGE_FALLING>;
>         };
>
>         /* BCM5464 quad PHY */
> --
> 2.23.0
>

+netdev and Andrew for this patch, since the interrupt polarity caught
his attention in v1.
Marc Zyngier Nov. 12, 2019, 1:49 p.m. UTC | #2
On 2019-11-12 14:53, Vladimir Oltean wrote:
> On Tue, 12 Nov 2019 at 15:20, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> From: Vladimir Oltean <olteanv@gmail.com>
>>
>> On the LS1021A-TSN board, the 2 Atheros AR8031 PHYs for eth0 and 
>> eth1
>> have interrupt lines connected to the shared IRQ2_B LS1021A pin.
>>
>> Switching to interrupts offloads the PHY library from the task of
>> polling the MDIO status and AN registers (1, 4, 5) every second.
>>
>> Unfortunately, the BCM5464R quad PHY connected to the switch does 
>> not
>> appear to have an interrupt line routed to the SoC.
>>
>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  arch/arm/boot/dts/ls1021a-tsn.dts | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts 
>> b/arch/arm/boot/dts/ls1021a-tsn.dts
>> index 5b7689094b70..135d36461af4 100644
>> --- a/arch/arm/boot/dts/ls1021a-tsn.dts
>> +++ b/arch/arm/boot/dts/ls1021a-tsn.dts
>> @@ -203,11 +203,15 @@
>>         /* AR8031 */
>>         sgmii_phy1: ethernet-phy@1 {
>>                 reg = <0x1>;
>> +               /* SGMII1_PHY_INT_B: connected to IRQ2, active low 
>> */
>> +               interrupts-extended = <&extirq 2 
>> IRQ_TYPE_EDGE_FALLING>;
>>         };
>>
>>         /* AR8031 */
>>         sgmii_phy2: ethernet-phy@2 {
>>                 reg = <0x2>;
>> +               /* SGMII2_PHY_INT_B: connected to IRQ2, active low 
>> */
>> +               interrupts-extended = <&extirq 2 
>> IRQ_TYPE_EDGE_FALLING>;
>>         };
>>
>>         /* BCM5464 quad PHY */
>> --
>> 2.23.0
>>
>
> +netdev and Andrew for this patch, since the interrupt polarity 
> caught
> his attention in v1.

Certainly, the comments and the interrupt specifier do not match.
Which one is true?

         M.
Vladimir Oltean Nov. 12, 2019, 1:53 p.m. UTC | #3
On Tue, 12 Nov 2019 at 15:49, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2019-11-12 14:53, Vladimir Oltean wrote:
> > On Tue, 12 Nov 2019 at 15:20, Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> >>
> >> From: Vladimir Oltean <olteanv@gmail.com>
> >>
> >> On the LS1021A-TSN board, the 2 Atheros AR8031 PHYs for eth0 and
> >> eth1
> >> have interrupt lines connected to the shared IRQ2_B LS1021A pin.
> >>
> >> Switching to interrupts offloads the PHY library from the task of
> >> polling the MDIO status and AN registers (1, 4, 5) every second.
> >>
> >> Unfortunately, the BCM5464R quad PHY connected to the switch does
> >> not
> >> appear to have an interrupt line routed to the SoC.
> >>
> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> ---
> >>  arch/arm/boot/dts/ls1021a-tsn.dts | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts
> >> b/arch/arm/boot/dts/ls1021a-tsn.dts
> >> index 5b7689094b70..135d36461af4 100644
> >> --- a/arch/arm/boot/dts/ls1021a-tsn.dts
> >> +++ b/arch/arm/boot/dts/ls1021a-tsn.dts
> >> @@ -203,11 +203,15 @@
> >>         /* AR8031 */
> >>         sgmii_phy1: ethernet-phy@1 {
> >>                 reg = <0x1>;
> >> +               /* SGMII1_PHY_INT_B: connected to IRQ2, active low
> >> */
> >> +               interrupts-extended = <&extirq 2
> >> IRQ_TYPE_EDGE_FALLING>;
> >>         };
> >>
> >>         /* AR8031 */
> >>         sgmii_phy2: ethernet-phy@2 {
> >>                 reg = <0x2>;
> >> +               /* SGMII2_PHY_INT_B: connected to IRQ2, active low
> >> */
> >> +               interrupts-extended = <&extirq 2
> >> IRQ_TYPE_EDGE_FALLING>;
> >>         };
> >>
> >>         /* BCM5464 quad PHY */
> >> --
> >> 2.23.0
> >>
> >
> > +netdev and Andrew for this patch, since the interrupt polarity
> > caught
> > his attention in v1.
>
> Certainly, the comments and the interrupt specifier do not match.
> Which one is true?
>
>          M.
> --
> Jazz is not dead. It just smells funny...

The interrupt specifier certainly works. So that points to an issue
with the description. What do you mean, exactly? Does "active low"
mean "level-triggered"? How would you have described this?
Andrew Lunn Nov. 12, 2019, 2:01 p.m. UTC | #4
> > >> +               /* SGMII2_PHY_INT_B: connected to IRQ2, active low
> > >> */
> > >> +               interrupts-extended = <&extirq 2
> > >> IRQ_TYPE_EDGE_FALLING>;

> The interrupt specifier certainly works. So that points to an issue
> with the description. What do you mean, exactly? Does "active low"
> mean "level-triggered"? How would you have described this?

I would expect IRQ_TYPE_ACTIVE_LOW, or whatever it is called. Since
this is a shared interrupt, going on the edge i think opens up a race
condition and interrupts can be missed.

	  Andrew
Marc Zyngier Nov. 12, 2019, 2:04 p.m. UTC | #5
On 2019-11-12 15:03, Vladimir Oltean wrote:
> On Tue, 12 Nov 2019 at 15:49, Marc Zyngier <maz@kernel.org> wrote:
>>
>> On 2019-11-12 14:53, Vladimir Oltean wrote:
>> > On Tue, 12 Nov 2019 at 15:20, Rasmus Villemoes
>> > <linux@rasmusvillemoes.dk> wrote:
>> >>
>> >> From: Vladimir Oltean <olteanv@gmail.com>
>> >>
>> >> On the LS1021A-TSN board, the 2 Atheros AR8031 PHYs for eth0 and
>> >> eth1
>> >> have interrupt lines connected to the shared IRQ2_B LS1021A pin.
>> >>
>> >> Switching to interrupts offloads the PHY library from the task of
>> >> polling the MDIO status and AN registers (1, 4, 5) every second.
>> >>
>> >> Unfortunately, the BCM5464R quad PHY connected to the switch does
>> >> not
>> >> appear to have an interrupt line routed to the SoC.
>> >>
>> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> >> ---
>> >>  arch/arm/boot/dts/ls1021a-tsn.dts | 4 ++++
>> >>  1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts
>> >> b/arch/arm/boot/dts/ls1021a-tsn.dts
>> >> index 5b7689094b70..135d36461af4 100644
>> >> --- a/arch/arm/boot/dts/ls1021a-tsn.dts
>> >> +++ b/arch/arm/boot/dts/ls1021a-tsn.dts
>> >> @@ -203,11 +203,15 @@
>> >>         /* AR8031 */
>> >>         sgmii_phy1: ethernet-phy@1 {
>> >>                 reg = <0x1>;
>> >> +               /* SGMII1_PHY_INT_B: connected to IRQ2, active 
>> low
>> >> */
>> >> +               interrupts-extended = <&extirq 2
>> >> IRQ_TYPE_EDGE_FALLING>;
>> >>         };
>> >>
>> >>         /* AR8031 */
>> >>         sgmii_phy2: ethernet-phy@2 {
>> >>                 reg = <0x2>;
>> >> +               /* SGMII2_PHY_INT_B: connected to IRQ2, active 
>> low
>> >> */
>> >> +               interrupts-extended = <&extirq 2
>> >> IRQ_TYPE_EDGE_FALLING>;
>> >>         };
>> >>
>> >>         /* BCM5464 quad PHY */
>> >> --
>> >> 2.23.0
>> >>
>> >
>> > +netdev and Andrew for this patch, since the interrupt polarity
>> > caught
>> > his attention in v1.
>>
>> Certainly, the comments and the interrupt specifier do not match.
>> Which one is true?
>>
>>          M.
>> --
>> Jazz is not dead. It just smells funny...
>
> The interrupt specifier certainly works. So that points to an issue
> with the description. What do you mean, exactly? Does "active low"
> mean "level-triggered"? How would you have described this?

Active Low definitely implies level triggered. And if that's how it
is described in the TRM, than the interrupt specifier is wrong, and
just *seem to work* because the level goes back to high between two
interrupts.

Also, shared *edge* interrupts do not work, full stop. So I'm pretty
convinced that what you have here is just wrong.

         M.
Vladimir Oltean Nov. 12, 2019, 2:12 p.m. UTC | #6
On Tue, 12 Nov 2019 at 16:04, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2019-11-12 15:03, Vladimir Oltean wrote:
> > On Tue, 12 Nov 2019 at 15:49, Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> On 2019-11-12 14:53, Vladimir Oltean wrote:
> >> > On Tue, 12 Nov 2019 at 15:20, Rasmus Villemoes
> >> > <linux@rasmusvillemoes.dk> wrote:
> >> >>
> >> >> From: Vladimir Oltean <olteanv@gmail.com>
> >> >>
> >> >> On the LS1021A-TSN board, the 2 Atheros AR8031 PHYs for eth0 and
> >> >> eth1
> >> >> have interrupt lines connected to the shared IRQ2_B LS1021A pin.
> >> >>
> >> >> Switching to interrupts offloads the PHY library from the task of
> >> >> polling the MDIO status and AN registers (1, 4, 5) every second.
> >> >>
> >> >> Unfortunately, the BCM5464R quad PHY connected to the switch does
> >> >> not
> >> >> appear to have an interrupt line routed to the SoC.
> >> >>
> >> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> >> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> >> ---
> >> >>  arch/arm/boot/dts/ls1021a-tsn.dts | 4 ++++
> >> >>  1 file changed, 4 insertions(+)
> >> >>
> >> >> diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts
> >> >> b/arch/arm/boot/dts/ls1021a-tsn.dts
> >> >> index 5b7689094b70..135d36461af4 100644
> >> >> --- a/arch/arm/boot/dts/ls1021a-tsn.dts
> >> >> +++ b/arch/arm/boot/dts/ls1021a-tsn.dts
> >> >> @@ -203,11 +203,15 @@
> >> >>         /* AR8031 */
> >> >>         sgmii_phy1: ethernet-phy@1 {
> >> >>                 reg = <0x1>;
> >> >> +               /* SGMII1_PHY_INT_B: connected to IRQ2, active
> >> low
> >> >> */
> >> >> +               interrupts-extended = <&extirq 2
> >> >> IRQ_TYPE_EDGE_FALLING>;
> >> >>         };
> >> >>
> >> >>         /* AR8031 */
> >> >>         sgmii_phy2: ethernet-phy@2 {
> >> >>                 reg = <0x2>;
> >> >> +               /* SGMII2_PHY_INT_B: connected to IRQ2, active
> >> low
> >> >> */
> >> >> +               interrupts-extended = <&extirq 2
> >> >> IRQ_TYPE_EDGE_FALLING>;
> >> >>         };
> >> >>
> >> >>         /* BCM5464 quad PHY */
> >> >> --
> >> >> 2.23.0
> >> >>
> >> >
> >> > +netdev and Andrew for this patch, since the interrupt polarity
> >> > caught
> >> > his attention in v1.
> >>
> >> Certainly, the comments and the interrupt specifier do not match.
> >> Which one is true?
> >>
> >>          M.
> >> --
> >> Jazz is not dead. It just smells funny...
> >
> > The interrupt specifier certainly works. So that points to an issue
> > with the description. What do you mean, exactly? Does "active low"
> > mean "level-triggered"? How would you have described this?
>
> Active Low definitely implies level triggered. And if that's how it
> is described in the TRM, than the interrupt specifier is wrong, and
> just *seem to work* because the level goes back to high between two
> interrupts.
>
> Also, shared *edge* interrupts do not work, full stop. So I'm pretty
> convinced that what you have here is just wrong.
>
>          M.
> --
> Jazz is not dead. It just smells funny...

Ok, I've tested both interrupts with IRQ_TYPE_LEVEL_LOW and they still
work. I'll let Rasmus re-send if there is no trouble with the dtsi
patch. Sorry for the trouble and thanks for teaching me something new.

Cheers,
-Vladimir

Patch
diff mbox series

diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts b/arch/arm/boot/dts/ls1021a-tsn.dts
index 5b7689094b70..135d36461af4 100644
--- a/arch/arm/boot/dts/ls1021a-tsn.dts
+++ b/arch/arm/boot/dts/ls1021a-tsn.dts
@@ -203,11 +203,15 @@ 
 	/* AR8031 */
 	sgmii_phy1: ethernet-phy@1 {
 		reg = <0x1>;
+		/* SGMII1_PHY_INT_B: connected to IRQ2, active low */
+		interrupts-extended = <&extirq 2 IRQ_TYPE_EDGE_FALLING>;
 	};
 
 	/* AR8031 */
 	sgmii_phy2: ethernet-phy@2 {
 		reg = <0x2>;
+		/* SGMII2_PHY_INT_B: connected to IRQ2, active low */
+		interrupts-extended = <&extirq 2 IRQ_TYPE_EDGE_FALLING>;
 	};
 
 	/* BCM5464 quad PHY */