| Message ID | 20221213101440.24667-1-arun.ramadoss@microchip.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 62e027fb0e5293d95e8d36655757ef4687c8795d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: microchip: remove IRQF_TRIGGER_FALLING in request_threaded_irq | expand |
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
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
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 > > >
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 --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;
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