diff mbox series

[net] net: dsa: microchip: remove IRQF_TRIGGER_FALLING in request_threaded_irq

Message ID 20221213101440.24667-1-arun.ramadoss@microchip.com
State New
Headers
Series [net] net: dsa: microchip: remove IRQF_TRIGGER_FALLING in request_threaded_irq |

Commit Message

Arun Ramadoss Dec. 13, 2022, 10:14 a.m. UTC 
  KSZ swithes used interrupts for detecting the phy link up and down.
During registering the interrupt handler, it used IRQF_TRIGGER_FALLING
flag. But this flag has to be retrieved from device tree instead of hard
coding in the driver, so removing the flag.

Fixes: ff319a644829 ("net: dsa: microchip: move interrupt handling logic from lan937x to ksz_common")
Reported-by: Christian Eggers <ceggers@arri.de>
Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


base-commit: e095493091e850d5292ad01d8fbf5cde1d89ac53

Comments

Paolo Abeni Dec. 15, 2022, 11:29 a.m. UTC | #1 
On Tue, 2022-12-13 at 15:44 +0530, Arun Ramadoss wrote:
> KSZ swithes used interrupts for detecting the phy link up and down.
> During registering the interrupt handler, it used IRQF_TRIGGER_FALLING
> flag. But this flag has to be retrieved from device tree instead of hard
> coding in the driver, 

Out of sheer ignorance, why?

> so removing the flag.

It looks like the device tree currently lack such item, so this is
effecivelly breaking phy linkup/linkdown?

What am I missing?

Thanks!

Paolo
Christian Eggers Dec. 15, 2022, 1:50 p.m. UTC | #2 
Hi Paolo,

On Thursday, 15 December 2022, 12:29:22 CET, Paolo Abeni wrote:
> On Tue, 2022-12-13 at 15:44 +0530, Arun Ramadoss wrote:
> > KSZ swithes used interrupts for detecting the phy link up and down.
> > During registering the interrupt handler, it used IRQF_TRIGGER_FALLING
> > flag. But this flag has to be retrieved from device tree instead of hard
> > coding in the driver, 
> 
> Out of sheer ignorance, why?

As far as I know, some IRQF_ flags should be set through the firmware
(e.g. device tree) instead hard coding them into the driver. On my
platform, I have to use IRQF_TRIGGER_LOW instead of IRQF_TRIGGER_FALLING.
If the value is hard coded into the driver, the value from the driver
will have precedence.

See also: https://stackoverflow.com/a/40051191

> > so removing the flag.
> 
> It looks like the device tree currently lack such item, so this is
> effecivelly breaking phy linkup/linkdown?
What is "the" device tree. Do you mean the device tree for your specific
board, or the example under 
Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml?
The latter doesn't mention the irq at all.

BTW: In my kernel log I get the following messages:

> ksz9477-switch 0-005f: configuring for fixed/rmii link mode
> ksz9477-switch 0-005f lan0 (uninitialized): PHY [dsa-0.0:00] driver [Microchip KSZ9477] (irq=POLL)
> ksz9477-switch 0-005f: Link is Up - 100Mbps/Full - flow control off
> ksz9477-switch 0-005f lan1 (uninitialized): PHY [dsa-0.0:01] driver [Microchip KSZ9477] (irq=POLL)

Should I see something different than "irq=POLL" when an
irq line is provided in the device tree?

regards
Christian
Arun Ramadoss Dec. 16, 2022, 4:07 a.m. UTC | #3 
Hi Christian,

On Thu, 2022-12-15 at 14:50 +0100, Christian Eggers wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi Paolo,
> 
> On Thursday, 15 December 2022, 12:29:22 CET, Paolo Abeni wrote:
> > On Tue, 2022-12-13 at 15:44 +0530, Arun Ramadoss wrote:
> > > KSZ swithes used interrupts for detecting the phy link up and
> > > down.
> > > During registering the interrupt handler, it used
> > > IRQF_TRIGGER_FALLING
> > > flag. But this flag has to be retrieved from device tree instead
> > > of hard
> > > coding in the driver,
> > 
> > Out of sheer ignorance, why?
> 
> As far as I know, some IRQF_ flags should be set through the firmware
> (e.g. device tree) instead hard coding them into the driver. On my
> platform, I have to use IRQF_TRIGGER_LOW instead of
> IRQF_TRIGGER_FALLING.
> If the value is hard coded into the driver, the value from the driver
> will have precedence.
> 
> See also: https://stackoverflow.com/a/40051191
> 
> > > so removing the flag.
> > 
> > It looks like the device tree currently lack such item, so this is
> > effecivelly breaking phy linkup/linkdown?
> 
> What is "the" device tree. Do you mean the device tree for your
> specific
> board, or the example under
> Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml?
> The latter doesn't mention the irq at all.
> 
> BTW: In my kernel log I get the following messages:
> 
> > ksz9477-switch 0-005f: configuring for fixed/rmii link mode
> > ksz9477-switch 0-005f lan0 (uninitialized): PHY [dsa-0.0:00] driver
> > [Microchip KSZ9477] (irq=POLL)
> > ksz9477-switch 0-005f: Link is Up - 100Mbps/Full - flow control off
> > ksz9477-switch 0-005f lan1 (uninitialized): PHY [dsa-0.0:01] driver
> > [Microchip KSZ9477] (irq=POLL)
> 
> Should I see something different than "irq=POLL" when an
> irq line is provided in the device tree?

If the device tree is provided *interrupt controller and interrupt
cells*, the kernel message should print the irq number like (irq=67)
instead of POLL. (67 is random number).
Following is the device tree snippet,

ksz9563: switch@0 {
                 compatible = "microchip,ksz9563";
                 reg = <0>;
                 spi-max-frequency = <44000000>;
                 interrupt-parent = <&pioB>;
                 interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
                 interrupt-controller;
                 #interrupt-cells = <2>;
                 resetb-gpios = <&pioD 18 GPIO_ACTIVE_LOW>;
                 pinctrl-0 = <&pinctrl_spi_ksz_rst>;
                 status = "okay";
	
	....
}



> 
> regards
> Christian
> 
> 
>
patchwork-bot+netdevbpf@kernel.org Dec. 20, 2022, 1:30 a.m. UTC | #4 
Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 13 Dec 2022 15:44:40 +0530 you wrote:
> KSZ swithes used interrupts for detecting the phy link up and down.
> During registering the interrupt handler, it used IRQF_TRIGGER_FALLING
> flag. But this flag has to be retrieved from device tree instead of hard
> coding in the driver, so removing the flag.
> 
> Fixes: ff319a644829 ("net: dsa: microchip: move interrupt handling logic from lan937x to ksz_common")
> Reported-by: Christian Eggers <ceggers@arri.de>
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> 
> [...]

Here is the summary with links:
  - [net] net: dsa: microchip: remove IRQF_TRIGGER_FALLING in request_threaded_irq
    https://git.kernel.org/netdev/net/c/62e027fb0e52

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index d612181b3226..c68f48cd1ec0 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1883,8 +1883,7 @@  static int ksz_irq_common_setup(struct ksz_device *dev, struct ksz_irq *kirq)
 		irq_create_mapping(kirq->domain, n);
 
 	ret = request_threaded_irq(kirq->irq_num, NULL, ksz_irq_thread_fn,
-				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
-				   kirq->name, kirq);
+				   IRQF_ONESHOT, kirq->name, kirq);
 	if (ret)
 		goto out;