| Message ID | 20230515063200.301026-7-jiawenwu@trustnetic.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Thu, May 18, 2023 at 2:50 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote: > On Wednesday, May 17, 2023 11:01 PM, Andrew Lunn wrote: > > On Wed, May 17, 2023 at 10:55:01AM +0800, Jiawen Wu wrote: ... > > > I should provide gpio_regmap_config.irq_domain if I want to add the gpio_irq_chip. > > > But in this use, GPIO IRQs are requested by SFP driver. How can I get irq_domain > > > before SFP probe? And where do I add IRQ parent handler? > > > > I _think_ you are mixing upstream IRQs and downstream IRQs. > > > > Interrupts are arranged in trees. The CPU itself only has one or two > > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an > > interrupt, you look in the interrupt controller to see what external > > or internal interrupt triggered the CPU interrupt. And that interrupt > > controller might indicate the interrupt came from another interrupt > > controller. Hence the tree structure. And each node in the tree is > > considered an interrupt domain. > > > > A GPIO controller can also be an interrupt controller. It has an > > upstream interrupt, going to the controller above it. And it has > > downstream interrupts, the GPIO lines coming into it which can cause > > an interrupt. And the GPIO interrupt controller is a domain. > > > > So what exactly does gpio_regmap_config.irq_domain mean? Is it the > > domain of the upstream interrupt controller? Is it an empty domain > > structure to be used by the GPIO interrupt controller? It is very > > unlikely to have anything to do with the SFP devices below it. > > Sorry, since I don't know much about interrupt, it is difficult to understand > regmap-irq in a short time. There are many questions about regmap-irq. That's why I Cc'ed to Michael who is the author of gpio-regmap to probably get advice from. > When I want to add an IRQ chip for regmap, for the further irq_domain, > I need to pass a parameter of IRQ, and this IRQ will be requested with handler: > regmap_irq_thread(). Which IRQ does it mean? In the previous code of using > devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but > it was used to set chained handler only. Should the parent be this IRQ? I found > the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko. > > As you said, the interrupt of each tree node has its domain. Can I understand > that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting > them separately is not conflicting? Although I thought so, but after I implement > gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something > on registering gpio-regmap... > > Anyway it is a bit complicated, could I use this version of GPIO implementation if > it's really tough? It's possible but from GPIO subsystem point of view it's discouraged as long as there is no technical impediment to go the regmap way.
> > I _think_ you are mixing upstream IRQs and downstream IRQs. > > > > Interrupts are arranged in trees. The CPU itself only has one or two > > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an > > interrupt, you look in the interrupt controller to see what external > > or internal interrupt triggered the CPU interrupt. And that interrupt > > controller might indicate the interrupt came from another interrupt > > controller. Hence the tree structure. And each node in the tree is > > considered an interrupt domain. > > > > A GPIO controller can also be an interrupt controller. It has an > > upstream interrupt, going to the controller above it. And it has > > downstream interrupts, the GPIO lines coming into it which can cause > > an interrupt. And the GPIO interrupt controller is a domain. > > > > So what exactly does gpio_regmap_config.irq_domain mean? Is it the > > domain of the upstream interrupt controller? Is it an empty domain > > structure to be used by the GPIO interrupt controller? It is very > > unlikely to have anything to do with the SFP devices below it. > > Sorry, since I don't know much about interrupt, it is difficult to understand > regmap-irq in a short time. There are many questions about regmap-irq. > > When I want to add an IRQ chip for regmap, for the further irq_domain, > I need to pass a parameter of IRQ, and this IRQ will be requested with handler: > regmap_irq_thread(). Which IRQ does it mean? That is your upstream IRQ, the interrupt indicating one of your GPIO lines has changed state. > In the previous code of using > devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but > it was used to set chained handler only. Should the parent be this IRQ? I found > the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko. Do you have one MSI-X dedicated for GPIOs. Or is it your general MAC interrupt, and you need to read an interrupt controller register to determine it was GPIOs which triggered the interrupt? If you are getting errors when removing the driver it means you are missing some level of undoing what us done in probe. Are you sure regmap_del_irq_chip() is being called on unload? > As you said, the interrupt of each tree node has its domain. Can I understand > that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting > them separately is not conflicting? Although I thought so, but after I implement > gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something > on registering gpio-regmap... That is probably some sort of naming issue. You might want to add some prints in swnode_find_gpio() and gpiochip_find() to see what it is looking for vs what the name actually is. Andrew
Hi, I'm currently (and for the next three weeks) on vacation. Sorry in advanced if the format of the mail is wrong or similar. I just have access to my mobile. Am 18. Mai 2023 14:27:00 MESZ schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: >On Thu, May 18, 2023 at 2:50 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote: >> On Wednesday, May 17, 2023 11:01 PM, Andrew Lunn wrote: >> > On Wed, May 17, 2023 at 10:55:01AM +0800, Jiawen Wu wrote: > >... > >> > > I should provide gpio_regmap_config.irq_domain if I want to add the gpio_irq_chip. >> > > But in this use, GPIO IRQs are requested by SFP driver. How can I get irq_domain >> > > before SFP probe? And where do I add IRQ parent handler? >> > >> > I _think_ you are mixing upstream IRQs and downstream IRQs. >> > >> > Interrupts are arranged in trees. The CPU itself only has one or two >> > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an >> > interrupt, you look in the interrupt controller to see what external >> > or internal interrupt triggered the CPU interrupt. And that interrupt >> > controller might indicate the interrupt came from another interrupt >> > controller. Hence the tree structure. And each node in the tree is >> > considered an interrupt domain. >> > >> > A GPIO controller can also be an interrupt controller. It has an >> > upstream interrupt, going to the controller above it. And it has >> > downstream interrupts, the GPIO lines coming into it which can cause >> > an interrupt. And the GPIO interrupt controller is a domain. >> > >> > So what exactly does gpio_regmap_config.irq_domain mean? Is it the >> > domain of the upstream interrupt controller? Is it an empty domain >> > structure to be used by the GPIO interrupt controller? It is very >> > unlikely to have anything to do with the SFP devices below it. >> >> Sorry, since I don't know much about interrupt, it is difficult to understand >> regmap-irq in a short time. There are many questions about regmap-irq. > >That's why I Cc'ed to Michael who is the author of gpio-regmap to >probably get advice from. All gpio remap is doing is forwarding the IRQ domain from regmap-irq to the gpio subsystem. It's opaque to gpio-regmap and outside the scope of gpio-regmap. -michael >> When I want to add an IRQ chip for regmap, for the further irq_domain, >> I need to pass a parameter of IRQ, and this IRQ will be requested with handler: >> regmap_irq_thread(). Which IRQ does it mean? In the previous code of using >> devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but >> it was used to set chained handler only. Should the parent be this IRQ? I found >> the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko. >> >> As you said, the interrupt of each tree node has its domain. Can I understand >> that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting >> them separately is not conflicting? Although I thought so, but after I implement >> gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something >> on registering gpio-regmap... >> >> Anyway it is a bit complicated, could I use this version of GPIO implementation if >> it's really tough? > >It's possible but from GPIO subsystem point of view it's discouraged >as long as there is no technical impediment to go the regmap way.
On Thu, May 18, 2023 at 7:03 PM Michael Walle <michael@walle.cc> wrote:
> I'm currently (and for the next three weeks) on vacation. Sorry in advanced if the format of the mail is wrong or similar. I just have access to my mobile.
Have a good one and thank you for chiming in!
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Thursday, May 18, 2023 8:49 PM > To: Jiawen Wu <jiawenwu@trustnetic.com> > Cc: 'Andy Shevchenko' <andy.shevchenko@gmail.com>; netdev@vger.kernel.org; jarkko.nikula@linux.intel.com; > andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; jsd@semihalf.com; Jose.Abreu@synopsys.com; > hkallweit1@gmail.com; linux@armlinux.org.uk; linux-i2c@vger.kernel.org; linux-gpio@vger.kernel.org; mengyuanlou@net-swift.com > Subject: Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket > > > > I _think_ you are mixing upstream IRQs and downstream IRQs. > > > > > > Interrupts are arranged in trees. The CPU itself only has one or two > > > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an > > > interrupt, you look in the interrupt controller to see what external > > > or internal interrupt triggered the CPU interrupt. And that interrupt > > > controller might indicate the interrupt came from another interrupt > > > controller. Hence the tree structure. And each node in the tree is > > > considered an interrupt domain. > > > > > > A GPIO controller can also be an interrupt controller. It has an > > > upstream interrupt, going to the controller above it. And it has > > > downstream interrupts, the GPIO lines coming into it which can cause > > > an interrupt. And the GPIO interrupt controller is a domain. > > > > > > So what exactly does gpio_regmap_config.irq_domain mean? Is it the > > > domain of the upstream interrupt controller? Is it an empty domain > > > structure to be used by the GPIO interrupt controller? It is very > > > unlikely to have anything to do with the SFP devices below it. > > > > Sorry, since I don't know much about interrupt, it is difficult to understand > > regmap-irq in a short time. There are many questions about regmap-irq. > > > > When I want to add an IRQ chip for regmap, for the further irq_domain, > > I need to pass a parameter of IRQ, and this IRQ will be requested with handler: > > regmap_irq_thread(). Which IRQ does it mean? > > That is your upstream IRQ, the interrupt indicating one of your GPIO > lines has changed state. > > > In the previous code of using > > devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but > > it was used to set chained handler only. Should the parent be this IRQ? I found > > the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko. > > Do you have one MSI-X dedicated for GPIOs. Or is it your general MAC > interrupt, and you need to read an interrupt controller register to > determine it was GPIOs which triggered the interrupt? I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK). It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO interrupt is determined, GPIO_INT_STATUS register should be read to determine which GPIO line has changed state. > If you are getting errors when removing the driver it means you are > missing some level of undoing what us done in probe. Are you sure > regmap_del_irq_chip() is being called on unload? I used devm_* all when I registered them. > > As you said, the interrupt of each tree node has its domain. Can I understand > > that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting > > them separately is not conflicting? Although I thought so, but after I implement > > gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something > > on registering gpio-regmap... > > That is probably some sort of naming issue. You might want to add some > prints in swnode_find_gpio() and gpiochip_find() to see what it is > looking for vs what the name actually is. Thanks for the advice, I'll try again today.
On Thursday, May 18, 2023 8:49 PM, Andrew Lunn wrote: > > > I _think_ you are mixing upstream IRQs and downstream IRQs. > > > > > > Interrupts are arranged in trees. The CPU itself only has one or two > > > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an > > > interrupt, you look in the interrupt controller to see what external > > > or internal interrupt triggered the CPU interrupt. And that interrupt > > > controller might indicate the interrupt came from another interrupt > > > controller. Hence the tree structure. And each node in the tree is > > > considered an interrupt domain. > > > > > > A GPIO controller can also be an interrupt controller. It has an > > > upstream interrupt, going to the controller above it. And it has > > > downstream interrupts, the GPIO lines coming into it which can cause > > > an interrupt. And the GPIO interrupt controller is a domain. > > > > > > So what exactly does gpio_regmap_config.irq_domain mean? Is it the > > > domain of the upstream interrupt controller? Is it an empty domain > > > structure to be used by the GPIO interrupt controller? It is very > > > unlikely to have anything to do with the SFP devices below it. > > > > Sorry, since I don't know much about interrupt, it is difficult to understand > > regmap-irq in a short time. There are many questions about regmap-irq. > > > > When I want to add an IRQ chip for regmap, for the further irq_domain, > > I need to pass a parameter of IRQ, and this IRQ will be requested with handler: > > regmap_irq_thread(). Which IRQ does it mean? > > That is your upstream IRQ, the interrupt indicating one of your GPIO > lines has changed state. > > > In the previous code of using > > devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but > > it was used to set chained handler only. Should the parent be this IRQ? I found > > the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko. > > Do you have one MSI-X dedicated for GPIOs. Or is it your general MAC > interrupt, and you need to read an interrupt controller register to > determine it was GPIOs which triggered the interrupt? > > If you are getting errors when removing the driver it means you are > missing some level of undoing what us done in probe. Are you sure > regmap_del_irq_chip() is being called on unload? > > > As you said, the interrupt of each tree node has its domain. Can I understand > > that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting > > them separately is not conflicting? Although I thought so, but after I implement > > gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something > > on registering gpio-regmap... > > That is probably some sort of naming issue. You might want to add some > prints in swnode_find_gpio() and gpiochip_find() to see what it is > looking for vs what the name actually is. It's true for the problem of name, but there is another problem. SFP driver has successfully got gpio_desc, then it failed to get gpio_irq from gpio_desc (with error return -517). I traced the function gpiod_to_irq(): gc = desc->gdev->chip; offset = gpio_chip_hwgpio(desc); if (gc->to_irq) { int retirq = gc->to_irq(gc, offset); /* Zero means NO_IRQ */ if (!retirq) return -ENXIO; return retirq; } 'gc->to_irq = gpiochip_to_irq' was set in [4]gpiochip_irqchip_add_domain(). So: static int gpiochip_to_irq(struct gpio_chip *gc, unsigned int offset) { struct irq_domain *domain = gc->irq.domain; #ifdef CONFIG_GPIOLIB_IRQCHIP /* * Avoid race condition with other code, which tries to lookup * an IRQ before the irqchip has been properly registered, * i.e. while gpiochip is still being brought up. */ if (!gc->irq.initialized) return -EPROBE_DEFER; #endif gc->irq.initialized is set to true at the end of [3]gpiochip_add_irqchip() only. Firstly, it checks if irqchip is NULL: static int gpiochip_add_irqchip(struct gpio_chip *gc, struct lock_class_key *lock_key, struct lock_class_key *request_key) { struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev); struct irq_chip *irqchip = gc->irq.chip; unsigned int type; unsigned int i; if (!irqchip) return 0; The result shows that it was NULL, so gc->irq.initialized = false. Above all, return irq = -EPROBE_DEFER. So let's sort the function calls. In chronological order, [1] calls [2], [2] calls [3], then [1] calls [4]. The irq_chip was added to irq_domain->host_data->irq_chip before [1]. But I don't find where to convert gpio_chip->irq.domain->host_data->irq_chip to gpio_chip->irq.chip, it seems like it should happen after [4] ? But if it wants to use 'gc->to_irq' successfully, it should happen before [3]? [1] gpio_regmap_register() [2] gpiochip_add_data() [3] gpiochip_add_irqchip() [4] gpiochip_irqchip_add_domain() I'm sorry that I described the problem in a confusing way, apologize if I missed some code that caused this confusion.
> I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK). > It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO > interrupt is determined, GPIO_INT_STATUS register should be read to determine > which GPIO line has changed state. So you have another interrupt controller above the GPIO interrupt controller. regmap-gpio is pushing you towards describing this interrupt controller as a Linux interrupt controller. When you look at drivers handling interrupts, most leaf interrupt controllers are not described as Linux interrupt controllers. The driver interrupt handler reads the interrupt status register and internally dispatches to the needed handler. This works well when everything is internal to one driver. However, here, you have two drivers involved, your MAC driver and a GPIO driver instantiated by the MAC driver. So i think you are going to need to described the MAC interrupt controller as a Linux interrupt controller. Take a look at the mv88e6xxx driver, which does this. It has two interrupt controller embedded within it, and they are chained. > > If you are getting errors when removing the driver it means you are > > missing some level of undoing what us done in probe. Are you sure > > regmap_del_irq_chip() is being called on unload? > > I used devm_* all when I registered them. Look at the ordering. Is regmap_del_irq_chip() being called too late? I've had problems like this with the mv88e6xxx driver and its interrupt controllers. I ended up not using devm_ so i had full control over the order things got undone. In that case, the external devices was PHYs, with the PHY interrupt being inside the Ethernet switch, which i exposed using a Linux interrupt controller. Andrew
> It's true for the problem of name, but there is another problem. SFP driver has > successfully got gpio_desc, then it failed to get gpio_irq from gpio_desc (with error > return -517). I traced the function gpiod_to_irq(): -517 is a number you learn after a while. -EPROBE_DEFFER. So the GPIO controller is not fully ready when the SFP driver tries to use it. I guess this is the missing upstream interrupt. You need to get the order correct: Register the MAC interrupt controller Instantiate the regmap-gpio controller Instantiate the I2C bus master Instantiate the SPF devices Instantiate PHYLINK. Andrew
On Friday, May 19, 2023 9:13 PM, Andrew Lunn wrote: > > I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK). > > It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO > > interrupt is determined, GPIO_INT_STATUS register should be read to determine > > which GPIO line has changed state. > > So you have another interrupt controller above the GPIO interrupt > controller. regmap-gpio is pushing you towards describing this > interrupt controller as a Linux interrupt controller. > > When you look at drivers handling interrupts, most leaf interrupt > controllers are not described as Linux interrupt controllers. The > driver interrupt handler reads the interrupt status register and > internally dispatches to the needed handler. This works well when > everything is internal to one driver. > > However, here, you have two drivers involved, your MAC driver and a > GPIO driver instantiated by the MAC driver. So i think you are going > to need to described the MAC interrupt controller as a Linux interrupt > controller. > > Take a look at the mv88e6xxx driver, which does this. It has two > interrupt controller embedded within it, and they are chained. Now I add two interrupt controllers, the first one for the MAC interrupt, and the second one for regmap-gpio. In the second adding flow, irq = irq_find_mapping(txgbe->misc.domain, TXGBE_PX_MISC_GPIO_OFFSET); err = regmap_add_irq_chip_fwnode(fwnode, regmap, irq, 0, 0, chip, &chip_data); and then, config.irq_domain = regmap_irq_get_domain(chip_data); gpio_regmap = gpio_regmap_register(&config); "txgbe->misc.domain" is the MAC interrupt domain. I think this flow should be correct, but still failed to get gpio_irq from gpio_desc with err -517. And I still have doubts about what I said earlier: https://lore.kernel.org/netdev/20230515063200.301026-1-jiawenwu@trustnetic.com/T/#me1be68e1a1e44426ecc0dd8edf0f6b224e50630d There really is nothing wrong with gpiochip_to_irq()?? > > > If you are getting errors when removing the driver it means you are > > > missing some level of undoing what us done in probe. Are you sure > > > regmap_del_irq_chip() is being called on unload? > > > > I used devm_* all when I registered them. > > Look at the ordering. Is regmap_del_irq_chip() being called too late? > I've had problems like this with the mv88e6xxx driver and its > interrupt controllers. I ended up not using devm_ so i had full > control over the order things got undone. In that case, the external > devices was PHYs, with the PHY interrupt being inside the Ethernet > switch, which i exposed using a Linux interrupt controller. I use no devm_ functions to add regmap irq chip, register gpio regmap, and call their del/unregister functions at the position corresponding to release. irq_domain_remove() call trace still exist. [ 104.553182] Call Trace: [ 104.553184] <TASK> [ 104.553185] irq_domain_remove+0x2b/0xe0 [ 104.553190] regmap_del_irq_chip.part.0+0x8a/0x160 [ 104.553196] txgbe_remove_phy+0x57/0x80 [txgbe] [ 104.553201] txgbe_remove+0x2a/0x90 [txgbe] [ 104.553205] pci_device_remove+0x36/0xa0 [ 104.553208] device_release_driver_internal+0xaa/0x140 [ 104.553213] driver_detach+0x44/0x90 [ 104.553215] bus_remove_driver+0x69/0xf0 [ 104.553217] pci_unregister_driver+0x29/0xb0 [ 104.553220] __x64_sys_delete_module+0x145/0x240 [ 104.553223] ? exit_to_user_mode_prepare+0x3c/0x1a0 [ 104.553226] do_syscall_64+0x3b/0x90 [ 104.553230] entry_SYSCALL_64_after_hwframe+0x72/0xdc
On Mon, May 22, 2023 at 06:58:19PM +0800, Jiawen Wu wrote: > On Monday, May 22, 2023 5:01 PM, Jiawen Wu wrote: > > On Friday, May 19, 2023 9:13 PM, Andrew Lunn wrote: > > > > I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK). > > > > It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO > > > > interrupt is determined, GPIO_INT_STATUS register should be read to determine > > > > which GPIO line has changed state. > > > > > > So you have another interrupt controller above the GPIO interrupt > > > controller. regmap-gpio is pushing you towards describing this > > > interrupt controller as a Linux interrupt controller. > > > > > > When you look at drivers handling interrupts, most leaf interrupt > > > controllers are not described as Linux interrupt controllers. The > > > driver interrupt handler reads the interrupt status register and > > > internally dispatches to the needed handler. This works well when > > > everything is internal to one driver. > > > > > > However, here, you have two drivers involved, your MAC driver and a > > > GPIO driver instantiated by the MAC driver. So i think you are going > > > to need to described the MAC interrupt controller as a Linux interrupt > > > controller. > > > > > > Take a look at the mv88e6xxx driver, which does this. It has two > > > interrupt controller embedded within it, and they are chained. > > > > Now I add two interrupt controllers, the first one for the MAC interrupt, > > and the second one for regmap-gpio. In the second adding flow, > > > > irq = irq_find_mapping(txgbe->misc.domain, TXGBE_PX_MISC_GPIO_OFFSET); > > err = regmap_add_irq_chip_fwnode(fwnode, regmap, irq, 0, 0, > > chip, &chip_data); > > > > and then, > > > > config.irq_domain = regmap_irq_get_domain(chip_data); > > gpio_regmap = gpio_regmap_register(&config); > > > > "txgbe->misc.domain" is the MAC interrupt domain. I think this flow should > > be correct, but still failed to get gpio_irq from gpio_desc with err -517. > > > > And I still have doubts about what I said earlier: > > https://lore.kernel.org/netdev/20230515063200.301026-1- > > jiawenwu@trustnetic.com/T/#me1be68e1a1e44426ecc0dd8edf0f6b224e50630d > > > > There really is nothing wrong with gpiochip_to_irq()?? > > There is indeed something wrong in gpiochip_to_irq(), since commit 5467801 ("gpio: > Restrict usage of GPIO chip irq members before initialization"): > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit?id=5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320 > > When I use gpio_regmap_register() to add gpiochip, gpiochip_add_irqchip() will just > return 0 since irqchip = NULL, then gc->irq.initialized = false. As far as I understood your hardware, you need to provide an IRQ chip for your GPIOs. The driver that provides an IRQ chip for GPIO and uses GPIO regmap is drivers/gpio/gpio-sl28cpld.c. So, you need to create a proper IRQ domain tree before calling for GPIO registration. > Cc the committer: Shreeya Patel. You meant "author", right?
> > > > If you are getting errors when removing the driver it means you are > > > > missing some level of undoing what us done in probe. Are you sure > > > > regmap_del_irq_chip() is being called on unload? > > > > > > I used devm_* all when I registered them. > > > > Look at the ordering. Is regmap_del_irq_chip() being called too late? > > I've had problems like this with the mv88e6xxx driver and its > > interrupt controllers. I ended up not using devm_ so i had full > > control over the order things got undone. In that case, the external > > devices was PHYs, with the PHY interrupt being inside the Ethernet > > switch, which i exposed using a Linux interrupt controller. > > I use no devm_ functions to add regmap irq chip, register gpio regmap, > and call their del/unregister functions at the position corresponding to > release. irq_domain_remove() call trace still exist. > > [ 104.553182] Call Trace: > [ 104.553184] <TASK> > [ 104.553185] irq_domain_remove+0x2b/0xe0 > [ 104.553190] regmap_del_irq_chip.part.0+0x8a/0x160 > [ 104.553196] txgbe_remove_phy+0x57/0x80 [txgbe] > [ 104.553201] txgbe_remove+0x2a/0x90 [txgbe] > [ 104.553205] pci_device_remove+0x36/0xa0 > [ 104.553208] device_release_driver_internal+0xaa/0x140 > [ 104.553213] driver_detach+0x44/0x90 > [ 104.553215] bus_remove_driver+0x69/0xf0 > [ 104.553217] pci_unregister_driver+0x29/0xb0 > [ 104.553220] __x64_sys_delete_module+0x145/0x240 > [ 104.553223] ? exit_to_user_mode_prepare+0x3c/0x1a0 > [ 104.553226] do_syscall_64+0x3b/0x90 > [ 104.553230] entry_SYSCALL_64_after_hwframe+0x72/0xdc I think this problem is caused by a conflict calling of irq_domain_remove() between the two functions gpiochip_irqchip_remove() and regmap_del_irq_chip(). The front one is called by gpio_regmap_unregister(). I adjusted the order of release functions, regmap_del_irq_chip() first, then gpio_regmap_unregister(). Log became: [ 383.261168] Call Trace: [ 383.261169] <TASK> [ 383.261170] irq_domain_remove+0x2b/0xe0 [ 383.261174] gpiochip_irqchip_remove+0xf0/0x210 [ 383.261177] gpiochip_remove+0x4a/0x110 [ 383.261181] gpio_regmap_unregister+0x12/0x20 [gpio_regmap] [ 383.261186] txgbe_remove_phy+0x57/0x80 [txgbe] [ 383.261190] txgbe_remove+0x2a/0x90 [txgbe] irq_domain_remove() just free the memory of irq_domain, but its pointer address still exists. So it will be called twice.
> > Anyway it is a bit complicated, could I use this version of GPIO implementation if > > it's really tough? > > It's possible but from GPIO subsystem point of view it's discouraged > as long as there is no technical impediment to go the regmap way. After these days of trying, I guess there are still some bugs on gpio - regmap - irq. It looks like an compatibility issue with gpio_irq_chip and regmap_irq_chip (My rough fixes seems to work). Other than that, it seems to be no way to add interrupt trigger in regmap_irq_thread(), to solve the both-edge problem for my hardware. I'd be willing to use gpio-regmap if above issues worked out, I learned IRQ controller, IRQ domain, etc. , after all. But if not, I'd like to implement GPIO in the original way, it was tested to work. May I? Thanks for all your suggestions.
On Tue, May 23, 2023 at 12:57 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote: > > > Anyway it is a bit complicated, could I use this version of GPIO implementation if > > > it's really tough? > > > > It's possible but from GPIO subsystem point of view it's discouraged > > as long as there is no technical impediment to go the regmap way. > > After these days of trying, I guess there are still some bugs on gpio - regmap - irq. > It looks like an compatibility issue with gpio_irq_chip and regmap_irq_chip (My rough > fixes seems to work). > > Other than that, it seems to be no way to add interrupt trigger in regmap_irq_thread(), > to solve the both-edge problem for my hardware. > > I'd be willing to use gpio-regmap if above issues worked out, I learned IRQ controller, > IRQ domain, etc. , after all. And thank you for all this! Now you may suggest the fixes to the GPIO regmap with all your knowledge of the area. > But if not, I'd like to implement GPIO in the original way, > it was tested to work. May I? Thanks for all your suggestions.
diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig index 90812d76181d..73f4492928c0 100644 --- a/drivers/net/ethernet/wangxun/Kconfig +++ b/drivers/net/ethernet/wangxun/Kconfig @@ -41,6 +41,8 @@ config TXGBE tristate "Wangxun(R) 10GbE PCI Express adapters support" depends on PCI select I2C_DESIGNWARE_PLATFORM + select GPIOLIB_IRQCHIP + select GPIOLIB select REGMAP select COMMON_CLK select LIBWX diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c index 1e8d8b7b0c62..590215303d45 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c @@ -1348,7 +1348,8 @@ void wx_free_irq(struct wx *wx) free_irq(entry->vector, q_vector); } - free_irq(wx->msix_entries[vector].vector, wx); + if (wx->mac.type == wx_mac_em) + free_irq(wx->msix_entries[vector].vector, wx); } EXPORT_SYMBOL(wx_free_irq); diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h index 97bce855bc60..bb8c63bdd5d4 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h @@ -79,7 +79,9 @@ #define WX_GPIO_INTMASK 0x14834 #define WX_GPIO_INTTYPE_LEVEL 0x14838 #define WX_GPIO_POLARITY 0x1483C +#define WX_GPIO_INTSTATUS 0x14844 #define WX_GPIO_EOI 0x1484C +#define WX_GPIO_EXT 0x14850 /*********************** Transmit DMA registers **************************/ /* transmit global control */ @@ -643,6 +645,7 @@ struct wx { bool wol_enabled; bool ncsi_enabled; bool gpio_ctrl; + spinlock_t gpio_lock; /* Tx fast path data */ int num_tx_queues; diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c index e10296abf5b4..ded04e9e136f 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c @@ -82,6 +82,8 @@ static int txgbe_enumerate_functions(struct wx *wx) **/ static void txgbe_irq_enable(struct wx *wx, bool queues) { + wr32(wx, WX_PX_MISC_IEN, TXGBE_PX_MISC_IEN_MASK); + /* unmask interrupt */ wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); if (queues) @@ -129,17 +131,6 @@ static irqreturn_t txgbe_intr(int __always_unused irq, void *data) return IRQ_HANDLED; } -static irqreturn_t txgbe_msix_other(int __always_unused irq, void *data) -{ - struct wx *wx = data; - - /* re-enable the original interrupt state */ - if (netif_running(wx->netdev)) - txgbe_irq_enable(wx, false); - - return IRQ_HANDLED; -} - /** * txgbe_request_msix_irqs - Initialize MSI-X interrupts * @wx: board private structure @@ -171,13 +162,6 @@ static int txgbe_request_msix_irqs(struct wx *wx) } } - err = request_irq(wx->msix_entries[vector].vector, - txgbe_msix_other, 0, netdev->name, wx); - if (err) { - wx_err(wx, "request_irq for msix_other failed: %d\n", err); - goto free_queue_irqs; - } - return 0; free_queue_irqs: diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c index 38830e280031..aa8b4444e77a 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c @@ -2,6 +2,8 @@ /* Copyright (c) 2015 - 2023 Beijing WangXun Technology Co., Ltd. */ #include <linux/platform_device.h> +#include <linux/gpio/machine.h> +#include <linux/gpio/driver.h> #include <linux/gpio/property.h> #include <linux/regmap.h> #include <linux/clkdev.h> @@ -10,6 +12,7 @@ #include <linux/pci.h> #include "../libwx/wx_type.h" +#include "../libwx/wx_hw.h" #include "txgbe_type.h" #include "txgbe_phy.h" @@ -74,6 +77,238 @@ static int txgbe_swnodes_register(struct txgbe *txgbe) return software_node_register_node_group(nodes->group); } +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset) +{ + struct wx *wx = gpiochip_get_data(chip); + int val; + + val = rd32m(wx, WX_GPIO_EXT, BIT(offset)); + + return !!(val & BIT(offset)); +} + +static int txgbe_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) +{ + struct wx *wx = gpiochip_get_data(chip); + u32 val; + + val = rd32(wx, WX_GPIO_DDR); + if (BIT(offset) & val) + return GPIO_LINE_DIRECTION_OUT; + + return GPIO_LINE_DIRECTION_IN; +} + +static int txgbe_gpio_direction_in(struct gpio_chip *chip, unsigned int offset) +{ + struct wx *wx = gpiochip_get_data(chip); + unsigned long flags; + + spin_lock_irqsave(&wx->gpio_lock, flags); + wr32m(wx, WX_GPIO_DDR, BIT(offset), 0); + spin_unlock_irqrestore(&wx->gpio_lock, flags); + + return 0; +} + +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset, + int val) +{ + struct wx *wx = gpiochip_get_data(chip); + unsigned long flags; + u32 set; + + spin_lock_irqsave(&wx->gpio_lock, flags); + set = val ? BIT(offset) : 0; + wr32m(wx, WX_GPIO_DR, BIT(offset), set); + wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset)); + spin_unlock_irqrestore(&wx->gpio_lock, flags); + + return 0; +} + +static void txgbe_gpio_irq_ack(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct wx *wx = gpiochip_get_data(gc); + unsigned long flags; + + spin_lock_irqsave(&wx->gpio_lock, flags); + wr32(wx, WX_GPIO_EOI, BIT(hwirq)); + spin_unlock_irqrestore(&wx->gpio_lock, flags); +} + +static void txgbe_gpio_irq_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct wx *wx = gpiochip_get_data(gc); + unsigned long flags; + + gpiochip_disable_irq(gc, hwirq); + + spin_lock_irqsave(&wx->gpio_lock, flags); + wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), BIT(hwirq)); + spin_unlock_irqrestore(&wx->gpio_lock, flags); +} + +static void txgbe_gpio_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct wx *wx = gpiochip_get_data(gc); + unsigned long flags; + + gpiochip_enable_irq(gc, hwirq); + + spin_lock_irqsave(&wx->gpio_lock, flags); + wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), 0); + spin_unlock_irqrestore(&wx->gpio_lock, flags); +} + +static void txgbe_toggle_trigger(struct gpio_chip *gc, unsigned int offset) +{ + struct wx *wx = gpiochip_get_data(gc); + u32 pol; + int val; + + pol = rd32(wx, WX_GPIO_POLARITY); + val = gc->get(gc, offset); + if (val) + pol &= ~BIT(offset); + else + pol |= BIT(offset); + + wr32(wx, WX_GPIO_POLARITY, pol); +} + +static int txgbe_gpio_set_type(struct irq_data *d, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct wx *wx = gpiochip_get_data(gc); + u32 level, polarity, mask; + unsigned long flags; + + spin_lock_irqsave(&wx->gpio_lock, flags); + + mask = BIT(hwirq); + + if (type & IRQ_TYPE_LEVEL_MASK) { + level = 0; + irq_set_handler_locked(d, handle_level_irq); + } else { + level = mask; + irq_set_handler_locked(d, handle_edge_irq); + } + + if (type == IRQ_TYPE_EDGE_RISING || type == IRQ_TYPE_LEVEL_HIGH) + polarity = mask; + else + polarity = 0; + + wr32m(wx, WX_GPIO_INTEN, mask, mask); + wr32m(wx, WX_GPIO_INTTYPE_LEVEL, mask, level); + if (type == IRQ_TYPE_EDGE_BOTH) + txgbe_toggle_trigger(gc, hwirq); + else + wr32m(wx, WX_GPIO_POLARITY, mask, polarity); + + spin_unlock_irqrestore(&wx->gpio_lock, flags); + + return 0; +} + +static const struct irq_chip txgbe_gpio_irq_chip = { + .name = "txgbe_gpio_irq", + .irq_ack = txgbe_gpio_irq_ack, + .irq_mask = txgbe_gpio_irq_mask, + .irq_unmask = txgbe_gpio_irq_unmask, + .irq_set_type = txgbe_gpio_set_type, + .flags = IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + +static void txgbe_irq_handler(struct irq_desc *desc) +{ + struct irq_chip *chip = irq_desc_get_chip(desc); + struct wx *wx = irq_desc_get_handler_data(desc); + struct txgbe *txgbe = wx->priv; + irq_hw_number_t hwirq; + unsigned long gpioirq; + struct gpio_chip *gc; + + chained_irq_enter(chip, desc); + + gpioirq = rd32(wx, WX_GPIO_INTSTATUS); + + gc = txgbe->gpio; + for_each_set_bit(hwirq, &gpioirq, gc->ngpio) { + int gpio = irq_find_mapping(gc->irq.domain, hwirq); + u32 irq_type = irq_get_trigger_type(gpio); + + generic_handle_irq(gpio); + + if ((irq_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH) + txgbe_toggle_trigger(gc, hwirq); + } + + chained_irq_exit(chip, desc); + + /* unmask interrupt */ + wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); +} + +static int txgbe_gpio_init(struct txgbe *txgbe) +{ + struct gpio_irq_chip *girq; + struct wx *wx = txgbe->wx; + struct gpio_chip *gc; + struct device *dev; + int ret; + + dev = &wx->pdev->dev; + + gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL); + if (!gc) + return -ENOMEM; + + gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x", + (wx->pdev->bus->number << 8) | wx->pdev->devfn); + gc->base = -1; + gc->ngpio = 6; + gc->owner = THIS_MODULE; + gc->parent = dev; + gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]); + gc->get = txgbe_gpio_get; + gc->get_direction = txgbe_gpio_get_direction; + gc->direction_input = txgbe_gpio_direction_in; + gc->direction_output = txgbe_gpio_direction_out; + + girq = &gc->irq; + gpio_irq_chip_set_chip(girq, &txgbe_gpio_irq_chip); + girq->parent_handler = txgbe_irq_handler; + girq->parent_handler_data = wx; + girq->num_parents = 1; + girq->parents = devm_kcalloc(dev, girq->num_parents, + sizeof(*girq->parents), GFP_KERNEL); + if (!girq->parents) + return -ENOMEM; + girq->parents[0] = wx->msix_entries[wx->num_q_vectors].vector; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_bad_irq; + + ret = devm_gpiochip_add_data(dev, gc, wx); + if (ret) + return ret; + + spin_lock_init(&wx->gpio_lock); + txgbe->gpio = gc; + + return 0; +} + static int txgbe_clock_register(struct txgbe *txgbe) { struct pci_dev *pdev = txgbe->wx->pdev; @@ -188,6 +423,12 @@ int txgbe_init_phy(struct txgbe *txgbe) return ret; } + ret = txgbe_gpio_init(txgbe); + if (ret) { + wx_err(txgbe->wx, "failed to init gpio\n"); + goto err_unregister_swnode; + } + ret = txgbe_clock_register(txgbe); if (ret) { wx_err(txgbe->wx, "failed to register clock: %d\n", ret); diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h index fc91e0fc37a6..6c903e4517c7 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h @@ -55,6 +55,28 @@ #define TXGBE_TS_CTL 0x10300 #define TXGBE_TS_CTL_EVAL_MD BIT(31) +/* GPIO register bit */ +#define TXGBE_GPIOBIT_0 BIT(0) /* I:tx fault */ +#define TXGBE_GPIOBIT_1 BIT(1) /* O:tx disabled */ +#define TXGBE_GPIOBIT_2 BIT(2) /* I:sfp module absent */ +#define TXGBE_GPIOBIT_3 BIT(3) /* I:rx signal lost */ +#define TXGBE_GPIOBIT_4 BIT(4) /* O:rate select, 1G(0) 10G(1) */ +#define TXGBE_GPIOBIT_5 BIT(5) /* O:rate select, 1G(0) 10G(1) */ + +/* Extended Interrupt Enable Set */ +#define TXGBE_PX_MISC_ETH_LKDN BIT(8) +#define TXGBE_PX_MISC_DEV_RST BIT(10) +#define TXGBE_PX_MISC_ETH_EVENT BIT(17) +#define TXGBE_PX_MISC_ETH_LK BIT(18) +#define TXGBE_PX_MISC_ETH_AN BIT(19) +#define TXGBE_PX_MISC_INT_ERR BIT(20) +#define TXGBE_PX_MISC_GPIO BIT(26) +#define TXGBE_PX_MISC_IEN_MASK \ + (TXGBE_PX_MISC_ETH_LKDN | TXGBE_PX_MISC_DEV_RST | \ + TXGBE_PX_MISC_ETH_EVENT | TXGBE_PX_MISC_ETH_LK | \ + TXGBE_PX_MISC_ETH_AN | TXGBE_PX_MISC_INT_ERR | \ + TXGBE_PX_MISC_GPIO) + /* I2C registers */ #define TXGBE_I2C_BASE 0x14900 @@ -153,6 +175,7 @@ struct txgbe { struct platform_device *i2c_dev; struct clk_lookup *clock; struct clk *clk; + struct gpio_chip *gpio; }; #endif /* _TXGBE_TYPE_H_ */
Register GPIO chip and handle GPIO IRQ for SFP socket. Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/net/ethernet/wangxun/Kconfig | 2 + drivers/net/ethernet/wangxun/libwx/wx_lib.c | 3 +- drivers/net/ethernet/wangxun/libwx/wx_type.h | 3 + .../net/ethernet/wangxun/txgbe/txgbe_main.c | 20 +- .../net/ethernet/wangxun/txgbe/txgbe_phy.c | 241 ++++++++++++++++++ .../net/ethernet/wangxun/txgbe/txgbe_type.h | 23 ++ 6 files changed, 273 insertions(+), 19 deletions(-)