* [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB
@ 2019-02-23 2:36 Linus Walleij
2019-02-23 9:00 ` Heiner Kallweit
2019-02-23 15:17 ` Andrew Lunn
0 siblings, 2 replies; 6+ messages in thread
From: Linus Walleij @ 2019-02-23 2:36 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: netdev, Linus Walleij, Heiner Kallweit
This fixes a regression introduced by
commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7
"net: phy: replace PHY_HAS_INTERRUPT with a check for
config_intr and ack_interrupt".
This assumes that a PHY cannot trigger interrupt unless
it has .config_intr() or .ack_interrupt() implemented.
A later patch makes the code assume both need to be
implemented for interrupts to be present.
But this PHY (which is inside a DSA) will happily
fire interrupts without either callback.
Implement a dummy callback for .config_intr() and
.ack_interrupt() as a workaround.
Tested on the RTL8366RB on D-Link DIR-685.
Fixes: 0d2e778e38e0 ("net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt")
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/net/phy/realtek.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index c6010fb1aa0f..31372be44570 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -211,6 +211,17 @@ static int rtl8366rb_config_init(struct phy_device *phydev)
return ret;
}
+static int rtl8366rb_ack_interrupt(struct phy_device *phydev)
+{
+ return 0;
+}
+
+static int rtl8366rb_config_intr(struct phy_device *phydev)
+{
+ /* Dummy function to make sure we get interrupts */
+ return 0;
+}
+
static struct phy_driver realtek_drvs[] = {
{
PHY_ID_MATCH_EXACT(0x00008201),
@@ -282,6 +293,8 @@ static struct phy_driver realtek_drvs[] = {
.name = "RTL8366RB Gigabit Ethernet",
.features = PHY_GBIT_FEATURES,
.config_init = &rtl8366rb_config_init,
+ .ack_interrupt = &rtl8366rb_ack_interrupt,
+ .config_intr = &rtl8366rb_config_intr,
.suspend = genphy_suspend,
.resume = genphy_resume,
},
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB
2019-02-23 2:36 [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB Linus Walleij
@ 2019-02-23 9:00 ` Heiner Kallweit
2019-02-23 15:17 ` Andrew Lunn
1 sibling, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2019-02-23 9:00 UTC (permalink / raw)
To: Linus Walleij, Andrew Lunn, Florian Fainelli; +Cc: netdev
On 23.02.2019 03:36, Linus Walleij wrote:
> This fixes a regression introduced by
> commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7
> "net: phy: replace PHY_HAS_INTERRUPT with a check for
> config_intr and ack_interrupt".
>
> This assumes that a PHY cannot trigger interrupt unless
> it has .config_intr() or .ack_interrupt() implemented.
> A later patch makes the code assume both need to be
> implemented for interrupts to be present.
>
> But this PHY (which is inside a DSA) will happily
> fire interrupts without either callback.
>
> Implement a dummy callback for .config_intr() and
> .ack_interrupt() as a workaround.
>
> Tested on the RTL8366RB on D-Link DIR-685.
>
> Fixes: 0d2e778e38e0 ("net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt")
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/net/phy/realtek.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index c6010fb1aa0f..31372be44570 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -211,6 +211,17 @@ static int rtl8366rb_config_init(struct phy_device *phydev)
> return ret;
> }
>
> +static int rtl8366rb_ack_interrupt(struct phy_device *phydev)
> +{
> + return 0;
> +}
> +
> +static int rtl8366rb_config_intr(struct phy_device *phydev)
> +{
> + /* Dummy function to make sure we get interrupts */
> + return 0;
> +}
> +
Maybe we should place these dummy calls in the core. We have this for
other cases already, see e.g. genphy_no_soft_reset. Then these dummies
can be re-used by other drivers that may have the need to do so.
Names could be:
genphy_no_ack_interrupt
genphy_no_config_intr
> static struct phy_driver realtek_drvs[] = {
> {
> PHY_ID_MATCH_EXACT(0x00008201),
> @@ -282,6 +293,8 @@ static struct phy_driver realtek_drvs[] = {
> .name = "RTL8366RB Gigabit Ethernet",
> .features = PHY_GBIT_FEATURES,
> .config_init = &rtl8366rb_config_init,
> + .ack_interrupt = &rtl8366rb_ack_interrupt,
> + .config_intr = &rtl8366rb_config_intr,
> .suspend = genphy_suspend,
> .resume = genphy_resume,
> },
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB
2019-02-23 2:36 [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB Linus Walleij
2019-02-23 9:00 ` Heiner Kallweit
@ 2019-02-23 15:17 ` Andrew Lunn
2019-02-23 23:57 ` Linus Walleij
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2019-02-23 15:17 UTC (permalink / raw)
To: Linus Walleij; +Cc: Florian Fainelli, netdev, Heiner Kallweit
On Sat, Feb 23, 2019 at 03:36:39AM +0100, Linus Walleij wrote:
> This fixes a regression introduced by
> commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7
> "net: phy: replace PHY_HAS_INTERRUPT with a check for
> config_intr and ack_interrupt".
>
> This assumes that a PHY cannot trigger interrupt unless
> it has .config_intr() or .ack_interrupt() implemented.
> A later patch makes the code assume both need to be
> implemented for interrupts to be present.
>
> But this PHY (which is inside a DSA) will happily
> fire interrupts without either callback.
Hi Linus
Can you disable these interrupts?
If you have dummy implementations, what is clearing the interrupt?
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB
2019-02-23 15:17 ` Andrew Lunn
@ 2019-02-23 23:57 ` Linus Walleij
2019-02-24 0:07 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2019-02-23 23:57 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev, Heiner Kallweit
On Sat, Feb 23, 2019 at 4:17 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Sat, Feb 23, 2019 at 03:36:39AM +0100, Linus Walleij wrote:
> > This fixes a regression introduced by
> > commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7
> > "net: phy: replace PHY_HAS_INTERRUPT with a check for
> > config_intr and ack_interrupt".
> >
> > This assumes that a PHY cannot trigger interrupt unless
> > it has .config_intr() or .ack_interrupt() implemented.
> > A later patch makes the code assume both need to be
> > implemented for interrupts to be present.
> >
> > But this PHY (which is inside a DSA) will happily
> > fire interrupts without either callback.
>
> Hi Linus
>
> Can you disable these interrupts?
>
> If you have dummy implementations, what is clearing the interrupt?
They are handled by the irqchip mask/unmask inside
the RTL8366RB, see:
drivers/net/dsa/rtl8366rb.c
So as soon as the phy core request the threaded IRQ
the irqchip will deal with this business on its own.
How exactly the RTL8366RB IRQ machine looks inside
I doubt even Realtek knows themselves, but from
my experiements, they seem all edge triggered,
and the irq will be raised every time an edge occurse
(such as inserting or removing the cable). The "ACK"
happens in hardware when we read the status register
in the nested interrupt handler in rtl8366rb_irq() so no
further registers need to be accessed.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB
2019-02-23 23:57 ` Linus Walleij
@ 2019-02-24 0:07 ` Andrew Lunn
2019-02-24 0:14 ` Linus Walleij
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2019-02-24 0:07 UTC (permalink / raw)
To: Linus Walleij; +Cc: Florian Fainelli, netdev, Heiner Kallweit
> They are handled by the irqchip mask/unmask inside
> the RTL8366RB, see:
> drivers/net/dsa/rtl8366rb.c
>
> So as soon as the phy core request the threaded IRQ
> the irqchip will deal with this business on its own.
>
> How exactly the RTL8366RB IRQ machine looks inside
> I doubt even Realtek knows themselves, but from
> my experiements, they seem all edge triggered,
> and the irq will be raised every time an edge occurse
> (such as inserting or removing the cable). The "ACK"
> happens in hardware when we read the status register
> in the nested interrupt handler in rtl8366rb_irq() so no
> further registers need to be accessed.
Hi Linus
Thanks for the explanation. So dummy functions are fine in this case.
However, in general, i don't think dummy functions will work for a PHY
driver, and may lead to interrupt storms. So it might be better to
have them in the driver, not the core, with comments about why they
are safe.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB
2019-02-24 0:07 ` Andrew Lunn
@ 2019-02-24 0:14 ` Linus Walleij
0 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2019-02-24 0:14 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev, Heiner Kallweit
On Sun, Feb 24, 2019 at 1:07 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > They are handled by the irqchip mask/unmask inside
> > the RTL8366RB, see:
> > drivers/net/dsa/rtl8366rb.c
> >
> > So as soon as the phy core request the threaded IRQ
> > the irqchip will deal with this business on its own.
> >
> > How exactly the RTL8366RB IRQ machine looks inside
> > I doubt even Realtek knows themselves, but from
> > my experiements, they seem all edge triggered,
> > and the irq will be raised every time an edge occurse
> > (such as inserting or removing the cable). The "ACK"
> > happens in hardware when we read the status register
> > in the nested interrupt handler in rtl8366rb_irq() so no
> > further registers need to be accessed.
>
> Hi Linus
>
> Thanks for the explanation. So dummy functions are fine in this case.
>
> However, in general, i don't think dummy functions will work for a PHY
> driver, and may lead to interrupt storms. So it might be better to
> have them in the driver, not the core, with comments about why they
> are safe.
OK shall I just send a v3 moving them back to the driver so we avoid
confusion on which version should be applied?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-24 0:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-23 2:36 [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB Linus Walleij
2019-02-23 9:00 ` Heiner Kallweit
2019-02-23 15:17 ` Andrew Lunn
2019-02-23 23:57 ` Linus Walleij
2019-02-24 0:07 ` Andrew Lunn
2019-02-24 0:14 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).