When an external device generated a level based interrupt then the
interrupt controller could miss the interrupt. The reason is that the
interrupt controller can detect only link changes.
In the following example, if there is a PHY that generates an interrupt
then the following would happen. The GPIO detected that the interrupt
line changed, and then the 'ocelot_irq_handler' was called. Here it
detects which GPIO line saw the change and for that will call the
following:
1. irq_mask
2. phy interrupt routine
3. irq_eoi
4. irq_unmask
And this works fine for simple cases, but if the PHY generates many
interrupts, for example when doing PTP timestamping, then the following
could happen. Again the function 'ocelot_irq_handler' will be called
and then from here the following could happen:
1. irq_mask
2. phy interrupt routine
3. irq_eoi
4. irq_unmask
Right before step 3(irq_eoi), the PHY will generate another interrupt.
Now the interrupt controller will acknowledge the change in the
interrupt line. So we miss the interrupt.
A solution will be to use 'handle_level_irq' instead of
'handle_fasteoi_irq', because for this will change routine order of
handling the interrupt.
1. irq_mask
2. irq_ack
3. phy interrupt routine
4. irq_unmask
And now if the PHY will generate a new interrupt before irq_unmask, the
interrupt controller will detect this because it already acknowledge the
change in interrupt line at step 2(irq_ack).
But this is not the full solution because there is another issue. In
case there are 2 PHYs that share the interrupt line. For example phy1
generates an interrupt, then the following can happen:
1.irq_mask
2.irq_ack
3.phy0 interrupt routine
4.phy1 interrupt routine
5.irq_unmask
In case phy0 will generate an interrupt while clearing the interrupt
source in phy1, then the interrupt line will be kept down by phy0. So
the interrupt controller will not see any changes in the interrupt line.
The solution here is to update 'irq_unmask' such that it can detect if
the interrupt line is still active or not. And if it is active then call
again the procedure to clear the interrupts. But we don't want to do it
every time, only if we know that the interrupt controller has not seen
already that the interrupt line has changed.
While at this, add support also for IRQ_TYPE_LEVEL_LOW.
Fixes: be36abb71d878f ("pinctrl: ocelot: add support for interrupt controller")
Signed-off-by: Horatiu Vultur <[email protected]>
---
v2->v3:
- fix more grammer mistakes
- use temporary variable for BIT(gpio % 32)
- use own workqueue to queue work, such when the driver is removed all
the pending work is done
v1->v2:
- fix grammer mistakes
- remove redundant checks for type in ocelot_irq_set_type
- split assignment and declaration of work variable
- use irqd_get_trigger_type instead of getting trigger from action
---
drivers/pinctrl/pinctrl-ocelot.c | 111 +++++++++++++++++++++++++++----
1 file changed, 97 insertions(+), 14 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index c5fd154990c8..c7df8c5fe585 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -331,6 +331,7 @@ struct ocelot_pinctrl {
const struct ocelot_pincfg_data *pincfg_data;
struct ocelot_pmx_func func[FUNC_MAX];
u8 stride;
+ struct workqueue_struct *wq;
};
struct ocelot_match_data {
@@ -338,6 +339,11 @@ struct ocelot_match_data {
struct ocelot_pincfg_data pincfg_data;
};
+struct ocelot_irq_work {
+ struct work_struct irq_work;
+ struct irq_desc *irq_desc;
+};
+
#define LUTON_P(p, f0, f1) \
static struct ocelot_pin_caps luton_pin_##p = { \
.pin = p, \
@@ -1813,6 +1819,75 @@ static void ocelot_irq_mask(struct irq_data *data)
gpiochip_disable_irq(chip, gpio);
}
+static void ocelot_irq_work(struct work_struct *work)
+{
+ struct ocelot_irq_work *w = container_of(work, struct ocelot_irq_work, irq_work);
+ struct irq_chip *parent_chip = irq_desc_get_chip(w->irq_desc);
+ struct gpio_chip *chip = irq_desc_get_chip_data(w->irq_desc);
+ struct irq_data *data = irq_desc_get_irq_data(w->irq_desc);
+ unsigned int gpio = irqd_to_hwirq(data);
+
+ local_irq_disable();
+ chained_irq_enter(parent_chip, w->irq_desc);
+ generic_handle_domain_irq(chip->irq.domain, gpio);
+ chained_irq_exit(parent_chip, w->irq_desc);
+ local_irq_enable();
+
+ kfree(w);
+}
+
+static void ocelot_irq_unmask_level(struct irq_data *data)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+ struct ocelot_pinctrl *info = gpiochip_get_data(chip);
+ struct irq_desc *desc = irq_data_to_desc(data);
+ unsigned int gpio = irqd_to_hwirq(data);
+ unsigned int bit = BIT(gpio % 32);
+ bool ack = false, active = false;
+ u8 trigger_level;
+ int val;
+
+ trigger_level = irqd_get_trigger_type(data);
+
+ /* Check if the interrupt line is still active. */
+ regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
+ if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
+ (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
+ active = true;
+
+ /*
+ * Check if the interrupt controller has seen any changes in the
+ * interrupt line.
+ */
+ regmap_read(info->map, REG(OCELOT_GPIO_INTR, info, gpio), &val);
+ if (val & bit)
+ ack = true;
+
+ /* Enable the interrupt now */
+ gpiochip_enable_irq(chip, gpio);
+ regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
+ bit, bit);
+
+ /*
+ * In case the interrupt line is still active and the interrupt
+ * controller has not seen any changes in the interrupt line, then it
+ * means that there happen another interrupt while the line was active.
+ * So we missed that one, so we need to kick the interrupt again
+ * handler.
+ */
+ if (active && !ack) {
+ struct ocelot_irq_work *work;
+
+ work = kmalloc(sizeof(*work), GFP_ATOMIC);
+ if (!work)
+ return;
+
+ work->irq_desc = desc;
+ INIT_WORK(&work->irq_work, ocelot_irq_work);
+ queue_work(info->wq, &work->irq_work);
+ }
+}
+
static void ocelot_irq_unmask(struct irq_data *data)
{
struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
@@ -1836,13 +1911,12 @@ static void ocelot_irq_ack(struct irq_data *data)
static int ocelot_irq_set_type(struct irq_data *data, unsigned int type);
-static struct irq_chip ocelot_eoi_irqchip = {
+static struct irq_chip ocelot_level_irqchip = {
.name = "gpio",
.irq_mask = ocelot_irq_mask,
- .irq_eoi = ocelot_irq_ack,
- .irq_unmask = ocelot_irq_unmask,
- .flags = IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
- IRQCHIP_IMMUTABLE,
+ .irq_ack = ocelot_irq_ack,
+ .irq_unmask = ocelot_irq_unmask_level,
+ .flags = IRQCHIP_IMMUTABLE,
.irq_set_type = ocelot_irq_set_type,
GPIOCHIP_IRQ_RESOURCE_HELPERS
};
@@ -1859,14 +1933,9 @@ static struct irq_chip ocelot_irqchip = {
static int ocelot_irq_set_type(struct irq_data *data, unsigned int type)
{
- type &= IRQ_TYPE_SENSE_MASK;
-
- if (!(type & (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH)))
- return -EINVAL;
-
- if (type & IRQ_TYPE_LEVEL_HIGH)
- irq_set_chip_handler_name_locked(data, &ocelot_eoi_irqchip,
- handle_fasteoi_irq, NULL);
+ if (type & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+ irq_set_chip_handler_name_locked(data, &ocelot_level_irqchip,
+ handle_level_irq, NULL);
if (type & IRQ_TYPE_EDGE_BOTH)
irq_set_chip_handler_name_locked(data, &ocelot_irqchip,
handle_edge_irq, NULL);
@@ -1996,6 +2065,10 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
if (!info->desc)
return -ENOMEM;
+ info->wq = alloc_ordered_workqueue("ocelot_ordered", 0);
+ if (!info->wq)
+ return -ENOMEM;
+
info->pincfg_data = &data->pincfg_data;
reset = devm_reset_control_get_optional_shared(dev, "switch");
@@ -2018,7 +2091,7 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
dev_err(dev, "Failed to create regmap\n");
return PTR_ERR(info->map);
}
- dev_set_drvdata(dev, info->map);
+ dev_set_drvdata(dev, info);
info->dev = dev;
/* Pinconf registers */
@@ -2043,6 +2116,15 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
return 0;
}
+static int ocelot_pinctrl_remove(struct platform_device *pdev)
+{
+ struct ocelot_pinctrl *info = platform_get_drvdata(pdev);
+
+ destroy_workqueue(info->wq);
+
+ return 0;
+}
+
static struct platform_driver ocelot_pinctrl_driver = {
.driver = {
.name = "pinctrl-ocelot",
@@ -2050,6 +2132,7 @@ static struct platform_driver ocelot_pinctrl_driver = {
.suppress_bind_attrs = true,
},
.probe = ocelot_pinctrl_probe,
+ .remove = ocelot_pinctrl_remove,
};
module_platform_driver(ocelot_pinctrl_driver);
MODULE_LICENSE("Dual MIT/GPL");
--
2.33.0
On Fri, Sep 9, 2022 at 5:55 PM Horatiu Vultur
<[email protected]> wrote:
Thanks for an update, my comments below.
...
> - dev_set_drvdata(dev, info->map);
> + dev_set_drvdata(dev, info);
I would also change it to platform_set_drvdata() to keep symmetry with
->remove().
...
> +static int ocelot_pinctrl_remove(struct platform_device *pdev)
> +{
> + struct ocelot_pinctrl *info = platform_get_drvdata(pdev);
> + destroy_workqueue(info->wq);
Is it a synchronous operation? Anyway, what does guarantee that after
this no other task can schedule a new work due to unmasking an
interrupt? I think you need to be sure your device is quiescent before
killing that workqueue. Something like synchronize_irq() +
disable_irq() or equivalent? (I don't know for sure, you need to
investigate it yourself and find the best suitable way).
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
On Fri, Sep 9, 2022 at 4:55 PM Horatiu Vultur
<[email protected]> wrote:
> When an external device generated a level based interrupt then the
> interrupt controller could miss the interrupt. The reason is that the
> interrupt controller can detect only link changes.
>
> In the following example, if there is a PHY that generates an interrupt
> then the following would happen. The GPIO detected that the interrupt
> line changed, and then the 'ocelot_irq_handler' was called. Here it
> detects which GPIO line saw the change and for that will call the
> following:
> 1. irq_mask
> 2. phy interrupt routine
> 3. irq_eoi
> 4. irq_unmask
>
> And this works fine for simple cases, but if the PHY generates many
> interrupts, for example when doing PTP timestamping, then the following
> could happen. Again the function 'ocelot_irq_handler' will be called
> and then from here the following could happen:
> 1. irq_mask
> 2. phy interrupt routine
> 3. irq_eoi
> 4. irq_unmask
>
> Right before step 3(irq_eoi), the PHY will generate another interrupt.
> Now the interrupt controller will acknowledge the change in the
> interrupt line. So we miss the interrupt.
>
> A solution will be to use 'handle_level_irq' instead of
> 'handle_fasteoi_irq', because for this will change routine order of
> handling the interrupt.
> 1. irq_mask
> 2. irq_ack
> 3. phy interrupt routine
> 4. irq_unmask
>
> And now if the PHY will generate a new interrupt before irq_unmask, the
> interrupt controller will detect this because it already acknowledge the
> change in interrupt line at step 2(irq_ack).
>
> But this is not the full solution because there is another issue. In
> case there are 2 PHYs that share the interrupt line. For example phy1
> generates an interrupt, then the following can happen:
> 1.irq_mask
> 2.irq_ack
> 3.phy0 interrupt routine
> 4.phy1 interrupt routine
> 5.irq_unmask
>
> In case phy0 will generate an interrupt while clearing the interrupt
> source in phy1, then the interrupt line will be kept down by phy0. So
> the interrupt controller will not see any changes in the interrupt line.
> The solution here is to update 'irq_unmask' such that it can detect if
> the interrupt line is still active or not. And if it is active then call
> again the procedure to clear the interrupts. But we don't want to do it
> every time, only if we know that the interrupt controller has not seen
> already that the interrupt line has changed.
>
> While at this, add support also for IRQ_TYPE_LEVEL_LOW.
>
> Fixes: be36abb71d878f ("pinctrl: ocelot: add support for interrupt controller")
> Signed-off-by: Horatiu Vultur <[email protected]>
This v3 patch applied for fixes so we get some rotation in linux-next
and get the Ocelot kernel in working order.
Should it even be tagged for stable?
Andy had some further things to think about, consider these
for possible further patching.
Yours,
Linus Walleij
The 09/09/2022 18:09, Andy Shevchenko wrote:
>
> On Fri, Sep 9, 2022 at 5:55 PM Horatiu Vultur
> <[email protected]> wrote:
>
> Thanks for an update, my comments below.
Thanks for all the help and sorry for late reply.
>
> ...
>
> > - dev_set_drvdata(dev, info->map);
> > + dev_set_drvdata(dev, info);
>
> I would also change it to platform_set_drvdata() to keep symmetry with
> ->remove().
Yes, I will change this.
>
> ...
>
> > +static int ocelot_pinctrl_remove(struct platform_device *pdev)
> > +{
> > + struct ocelot_pinctrl *info = platform_get_drvdata(pdev);
>
> > + destroy_workqueue(info->wq);
>
> Is it a synchronous operation? Anyway, what does guarantee that after
> this no other task can schedule a new work due to unmasking an
> interrupt? I think you need to be sure your device is quiescent before
> killing that workqueue. Something like synchronize_irq() +
> disable_irq() or equivalent? (I don't know for sure, you need to
> investigate it yourself and find the best suitable way).
I have look at descriptions of the functions (synchronize_irq(),
disable_irq()) and I think is enough to use only disable_irq().
I also tried something but it didn't have the expected result so I would
need to look more into this. I tried to use disable_irq on returned irq
inside ocelot_gpiochip_register but I was still getting interrupts after
that.
Also I was thinking actually to use gpiochip_remove() here in
ocelot_pinctrl_remove() before calling destroy_workqueue(). But then I
might have problems inside ocelot_irq_work(). I need to check more this.
>
> > + return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
--
/Horatiu
The 09/14/2022 15:02, Linus Walleij wrote:
>
> On Fri, Sep 9, 2022 at 4:55 PM Horatiu Vultur
> <[email protected]> wrote:
>
> > Fixes: be36abb71d878f ("pinctrl: ocelot: add support for interrupt controller")
> > Signed-off-by: Horatiu Vultur <[email protected]>
>
> This v3 patch applied for fixes so we get some rotation in linux-next
> and get the Ocelot kernel in working order.
> Should it even be tagged for stable?
Thanks for applying the patch!
It would be great to go also in stable. I have tried to apply it on
5.19, 5.15, 5.10, 5.4, 4.19 but it failed on all of them because of
merge conflicts.
Should I send separate patch for each stable tree?
>
> Andy had some further things to think about, consider these
> for possible further patching.
>
> Yours,
> Linus Walleij
--
/Horatiu
On Thu, Sep 15, 2022 at 1:18 PM Horatiu Vultur
<[email protected]> wrote:
> It would be great to go also in stable. I have tried to apply it on
> 5.19, 5.15, 5.10, 5.4, 4.19 but it failed on all of them because of
> merge conflicts.
> Should I send separate patch for each stable tree?
You should indicate to the stable maintainers that it should go into
stable (option 2), then prepare backports IIUC. See:
https://www.kernel.org/doc/html/v6.0-rc5/process/stable-kernel-rules.html
Yours,
Linus Walleij
Hi Horatiu,
> When an external device generated a level based interrupt then the
> interrupt controller could miss the interrupt. The reason is that the
> interrupt controller can detect only link changes.
>
> In the following example, if there is a PHY that generates an interrupt
> then the following would happen. The GPIO detected that the interrupt
> line changed, and then the 'ocelot_irq_handler' was called. Here it
> detects which GPIO line saw the change and for that will call the
> following:
> 1. irq_mask
> 2. phy interrupt routine
> 3. irq_eoi
> 4. irq_unmask
>
> And this works fine for simple cases, but if the PHY generates many
> interrupts, for example when doing PTP timestamping, then the following
> could happen. Again the function 'ocelot_irq_handler' will be called
> and then from here the following could happen:
> 1. irq_mask
> 2. phy interrupt routine
> 3. irq_eoi
> 4. irq_unmask
>
> Right before step 3(irq_eoi), the PHY will generate another interrupt.
> Now the interrupt controller will acknowledge the change in the
> interrupt line. So we miss the interrupt.
>
> A solution will be to use 'handle_level_irq' instead of
> 'handle_fasteoi_irq', because for this will change routine order of
> handling the interrupt.
> 1. irq_mask
> 2. irq_ack
> 3. phy interrupt routine
> 4. irq_unmask
>
> And now if the PHY will generate a new interrupt before irq_unmask, the
> interrupt controller will detect this because it already acknowledge the
> change in interrupt line at step 2(irq_ack).
>
> But this is not the full solution because there is another issue. In
> case there are 2 PHYs that share the interrupt line. For example phy1
> generates an interrupt, then the following can happen:
> 1.irq_mask
> 2.irq_ack
> 3.phy0 interrupt routine
> 4.phy1 interrupt routine
> 5.irq_unmask
>
> In case phy0 will generate an interrupt while clearing the interrupt
> source in phy1, then the interrupt line will be kept down by phy0. So
> the interrupt controller will not see any changes in the interrupt line.
> The solution here is to update 'irq_unmask' such that it can detect if
> the interrupt line is still active or not. And if it is active then call
> again the procedure to clear the interrupts. But we don't want to do it
> every time, only if we know that the interrupt controller has not seen
> already that the interrupt line has changed.
>
> While at this, add support also for IRQ_TYPE_LEVEL_LOW.
Our board has a shared active low interrupt line, connected to a quad PHY
LAN8814 and two GPY215 PHYs. I've gave this a try but it doesn't seem to
work. It seems the interrupt fires multiple times. If I plug a cable in
one of the LAN8814 ports, I see that the interrupt count in
/proc/interrupts has increased by two. If I use a GPY215 port, I see about
40 interrupts firing.
I've verified that there is only one low pulse on the interrupt line. I've
noticed though, that the number of interrupts seem to be correlating with
the length of the low pulse.
-michael
On Tue, Sep 20, 2022 at 2:06 PM Michael Walle <[email protected]> wrote:
> Our board has a shared active low interrupt line, connected to a quad PHY
> LAN8814 and two GPY215 PHYs. I've gave this a try but it doesn't seem to
> work. It seems the interrupt fires multiple times. If I plug a cable in
> one of the LAN8814 ports, I see that the interrupt count in
> /proc/interrupts has increased by two. If I use a GPY215 port, I see about
> 40 interrupts firing.
A lot of interrupts firing is very typical for level IRQs.
So I assume these are wire-OR, i.e. exploiting open drain with
a pull-up resistor.
Just checking: since these drivers obviously must pass pass
IRQF_SHARED, have you also made sure that each driver also
will properly return IRQ_HANDLED if the interrupt was for them
(triggered by "their" hardware) but IRQ_NONE if the interrupt was
not for them (triggered by something else)?
The IRQ core relies on all drivers to do the right thing here.
Otherwise the IRQ will just re-fire until someone/something
manages to properly handle it and drive the line high again.
A typical case would be the LAN8814 driver having been probed
first, thus its IRQ handler will be visited first, and always returning
IRQ_HANDLED thereby "stealing" the irq from everyone else.
Another possible problem is if you don't have an external pull-up
resistor and you need some pin config to enable pull-up on the
SoC input line. This will generate a lot of IRQs.
A third problem would be that the line need time to rise.
But that should be uncommon.
Yours,
Linus Walleij
Am 2022-09-20 14:28, schrieb Linus Walleij:
> On Tue, Sep 20, 2022 at 2:06 PM Michael Walle <[email protected]> wrote:
>
>> Our board has a shared active low interrupt line, connected to a quad
>> PHY
>> LAN8814 and two GPY215 PHYs. I've gave this a try but it doesn't seem
>> to
>> work. It seems the interrupt fires multiple times. If I plug a cable
>> in
>> one of the LAN8814 ports, I see that the interrupt count in
>> /proc/interrupts has increased by two. If I use a GPY215 port, I see
>> about
>> 40 interrupts firing.
>
> A lot of interrupts firing is very typical for level IRQs.
Common but wrong? Except in the error case, /proc/interrupts
was always reliable on our boards :)
> So I assume these are wire-OR, i.e. exploiting open drain with
> a pull-up resistor.
Yes, the usual shared line interrupts.
> Just checking: since these drivers obviously must pass pass
> IRQF_SHARED, have you also made sure that each driver also
> will properly return IRQ_HANDLED if the interrupt was for them
> (triggered by "their" hardware) but IRQ_NONE if the interrupt was
> not for them (triggered by something else)?
Thanks, I'll check it.
> The IRQ core relies on all drivers to do the right thing here.
>
> Otherwise the IRQ will just re-fire until someone/something
> manages to properly handle it and drive the line high again.
>
> A typical case would be the LAN8814 driver having been probed
> first, thus its IRQ handler will be visited first, and always returning
> IRQ_HANDLED thereby "stealing" the irq from everyone else.
>
> Another possible problem is if you don't have an external pull-up
> resistor and you need some pin config to enable pull-up on the
> SoC input line. This will generate a lot of IRQs.
I've checked with a scope, the levels and edges look good.
> A third problem would be that the line need time to rise.
> But that should be uncommon.
I haven't looked at the code of this patch, but obiously it
is emulating the level triggered behavior with just a pin change
interrupt. There might also be something wrong there, too.
-michael
Am 2022-09-20 14:34, schrieb Michael Walle:
> Am 2022-09-20 14:28, schrieb Linus Walleij:
>> Just checking: since these drivers obviously must pass pass
>> IRQF_SHARED, have you also made sure that each driver also
>> will properly return IRQ_HANDLED if the interrupt was for them
>> (triggered by "their" hardware) but IRQ_NONE if the interrupt was
>> not for them (triggered by something else)?
>
> Thanks, I'll check it.
The gpy driver seems to handle that correctly. The micrel one introduced
a regression some time ago. I've send a fix in the meantime [1], but it
doesn't help for the multiple interrupts here.
-michael
[1]
https://lore.kernel.org/netdev/[email protected]/
The 09/20/2022 14:06, Michael Walle wrote:
>
> Hi Horatiu,
Hi Walle,
>
> > When an external device generated a level based interrupt then the
> > interrupt controller could miss the interrupt. The reason is that the
> > interrupt controller can detect only link changes.
> >
> > In the following example, if there is a PHY that generates an interrupt
> > then the following would happen. The GPIO detected that the interrupt
> > line changed, and then the 'ocelot_irq_handler' was called. Here it
> > detects which GPIO line saw the change and for that will call the
> > following:
> > 1. irq_mask
> > 2. phy interrupt routine
> > 3. irq_eoi
> > 4. irq_unmask
> >
> > And this works fine for simple cases, but if the PHY generates many
> > interrupts, for example when doing PTP timestamping, then the following
> > could happen. Again the function 'ocelot_irq_handler' will be called
> > and then from here the following could happen:
> > 1. irq_mask
> > 2. phy interrupt routine
> > 3. irq_eoi
> > 4. irq_unmask
> >
> > Right before step 3(irq_eoi), the PHY will generate another interrupt.
> > Now the interrupt controller will acknowledge the change in the
> > interrupt line. So we miss the interrupt.
> >
> > A solution will be to use 'handle_level_irq' instead of
> > 'handle_fasteoi_irq', because for this will change routine order of
> > handling the interrupt.
> > 1. irq_mask
> > 2. irq_ack
> > 3. phy interrupt routine
> > 4. irq_unmask
> >
> > And now if the PHY will generate a new interrupt before irq_unmask, the
> > interrupt controller will detect this because it already acknowledge the
> > change in interrupt line at step 2(irq_ack).
> >
> > But this is not the full solution because there is another issue. In
> > case there are 2 PHYs that share the interrupt line. For example phy1
> > generates an interrupt, then the following can happen:
> > 1.irq_mask
> > 2.irq_ack
> > 3.phy0 interrupt routine
> > 4.phy1 interrupt routine
> > 5.irq_unmask
> >
> > In case phy0 will generate an interrupt while clearing the interrupt
> > source in phy1, then the interrupt line will be kept down by phy0. So
> > the interrupt controller will not see any changes in the interrupt line.
> > The solution here is to update 'irq_unmask' such that it can detect if
> > the interrupt line is still active or not. And if it is active then call
> > again the procedure to clear the interrupts. But we don't want to do it
> > every time, only if we know that the interrupt controller has not seen
> > already that the interrupt line has changed.
> >
> > While at this, add support also for IRQ_TYPE_LEVEL_LOW.
>
> Our board has a shared active low interrupt line, connected to a quad PHY
> LAN8814 and two GPY215 PHYs. I've gave this a try but it doesn't seem to
> work. It seems the interrupt fires multiple times. If I plug a cable in
> one of the LAN8814 ports, I see that the interrupt count in
> /proc/interrupts has increased by two. If I use a GPY215 port, I see about
> 40 interrupts firing.
I really don't know why would see 40 interrupts on GPY215. But I can
explain why you see 2 interrupts with LAN8814 ports.
The reason is that the interrupt controller in the pinctrl detects edges
and not levels. So if we take an example: the PHY will generate an
interrupt by pulling down the line. Then pinctrl detects this change in
the line so will start interrupt handler rutine. It would mask, ack,
call the PHY interrupt routine. At this point when the PHY clears the
interrupt source, the interrupt line will be high. So the interrupt
controller will see also this change. Then when the interrupt controller
will unmask the interrupt, it would generate a new one. And this is the
second interrupt.
I didn't know that is a big issue to get another interrupt. Because
before it was possible to miss interrupts, so I thought it was a pretty
fair trade.
Below I have a diff that I tried with LAN8814 PHYs and I could see that
count in /proc/interrupts is increasing correctly.
>
> I've verified that there is only one low pulse on the interrupt line. I've
> noticed though, that the number of interrupts seem to be correlating with
> the length of the low pulse.
---
diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index c7df8c5fe5854..105771ff82e62 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -1863,19 +1863,28 @@ static void ocelot_irq_unmask_level(struct irq_data *data)
if (val & bit)
ack = true;
+ /* Try to clear any rising edges */
+ if (!active && ack)
+ regmap_write_bits(info->map, REG(OCELOT_GPIO_INTR, info, gpio),
+ bit, bit);
+
/* Enable the interrupt now */
gpiochip_enable_irq(chip, gpio);
regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
bit, bit);
/*
- * In case the interrupt line is still active and the interrupt
- * controller has not seen any changes in the interrupt line, then it
- * means that there happen another interrupt while the line was active.
+ * In case the interrupt line is still active then it means that
+ * there happen another interrupt while the line was active.
* So we missed that one, so we need to kick the interrupt again
* handler.
*/
- if (active && !ack) {
+ regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
+ if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
+ (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
+ active = true;
+
+ if (active) {
struct ocelot_irq_work *work;
work = kmalloc(sizeof(*work), GFP_ATOMIC);
---
>
> -michael
--
/Horatiu
Hi Horatiu,
Am 2022-09-20 21:30, schrieb Horatiu Vultur:
> The 09/20/2022 14:06, Michael Walle wrote:
>> > When an external device generated a level based interrupt then the
>> > interrupt controller could miss the interrupt. The reason is that the
>> > interrupt controller can detect only link changes.
>> >
>> > In the following example, if there is a PHY that generates an interrupt
>> > then the following would happen. The GPIO detected that the interrupt
>> > line changed, and then the 'ocelot_irq_handler' was called. Here it
>> > detects which GPIO line saw the change and for that will call the
>> > following:
>> > 1. irq_mask
>> > 2. phy interrupt routine
>> > 3. irq_eoi
>> > 4. irq_unmask
>> >
>> > And this works fine for simple cases, but if the PHY generates many
>> > interrupts, for example when doing PTP timestamping, then the following
>> > could happen. Again the function 'ocelot_irq_handler' will be called
>> > and then from here the following could happen:
>> > 1. irq_mask
>> > 2. phy interrupt routine
>> > 3. irq_eoi
>> > 4. irq_unmask
>> >
>> > Right before step 3(irq_eoi), the PHY will generate another interrupt.
>> > Now the interrupt controller will acknowledge the change in the
>> > interrupt line. So we miss the interrupt.
>> >
>> > A solution will be to use 'handle_level_irq' instead of
>> > 'handle_fasteoi_irq', because for this will change routine order of
>> > handling the interrupt.
>> > 1. irq_mask
>> > 2. irq_ack
>> > 3. phy interrupt routine
>> > 4. irq_unmask
>> >
>> > And now if the PHY will generate a new interrupt before irq_unmask, the
>> > interrupt controller will detect this because it already acknowledge the
>> > change in interrupt line at step 2(irq_ack).
>> >
>> > But this is not the full solution because there is another issue. In
>> > case there are 2 PHYs that share the interrupt line. For example phy1
>> > generates an interrupt, then the following can happen:
>> > 1.irq_mask
>> > 2.irq_ack
>> > 3.phy0 interrupt routine
>> > 4.phy1 interrupt routine
>> > 5.irq_unmask
>> >
>> > In case phy0 will generate an interrupt while clearing the interrupt
>> > source in phy1, then the interrupt line will be kept down by phy0. So
>> > the interrupt controller will not see any changes in the interrupt line.
>> > The solution here is to update 'irq_unmask' such that it can detect if
>> > the interrupt line is still active or not. And if it is active then call
>> > again the procedure to clear the interrupts. But we don't want to do it
>> > every time, only if we know that the interrupt controller has not seen
>> > already that the interrupt line has changed.
>> >
>> > While at this, add support also for IRQ_TYPE_LEVEL_LOW.
>>
>> Our board has a shared active low interrupt line, connected to a quad
>> PHY
>> LAN8814 and two GPY215 PHYs. I've gave this a try but it doesn't seem
>> to
>> work. It seems the interrupt fires multiple times. If I plug a cable
>> in
>> one of the LAN8814 ports, I see that the interrupt count in
>> /proc/interrupts has increased by two. If I use a GPY215 port, I see
>> about
>> 40 interrupts firing.
>
> I really don't know why would see 40 interrupts on GPY215.
The GPY215 seems to be broken in this regard and hold the interrupt
line asserted even after its interrupt status register is read.
> But I can
> explain why you see 2 interrupts with LAN8814 ports.
> The reason is that the interrupt controller in the pinctrl detects
> edges
> and not levels. So if we take an example: the PHY will generate an
> interrupt by pulling down the line. Then pinctrl detects this change in
> the line so will start interrupt handler rutine. It would mask, ack,
> call the PHY interrupt routine. At this point when the PHY clears the
> interrupt source, the interrupt line will be high. So the interrupt
> controller will see also this change. Then when the interrupt
> controller
> will unmask the interrupt, it would generate a new one. And this is the
> second interrupt.
> I didn't know that is a big issue to get another interrupt. Because
> before it was possible to miss interrupts, so I thought it was a pretty
> fair trade.
Seeing 20 was definitely fishy, seeing two instead of one maybe not
so much. I guess it will create one spurious interrupt if none of
the registered handlers will care.
OTOH, the code below won't work in all cases anyway, right? It's just
best effort.
> Below I have a diff that I tried with LAN8814 PHYs and I could see that
> count in /proc/interrupts is increasing correctly.
>
>> I've verified that there is only one low pulse on the interrupt line.
>> I've
>> noticed though, that the number of interrupts seem to be correlating
>> with
>> the length of the low pulse.
> ---
> diff --git a/drivers/pinctrl/pinctrl-ocelot.c
> b/drivers/pinctrl/pinctrl-ocelot.c
> index c7df8c5fe5854..105771ff82e62 100644
> --- a/drivers/pinctrl/pinctrl-ocelot.c
> +++ b/drivers/pinctrl/pinctrl-ocelot.c
> @@ -1863,19 +1863,28 @@ static void ocelot_irq_unmask_level(struct
> irq_data *data)
> if (val & bit)
> ack = true;
>
> + /* Try to clear any rising edges */
> + if (!active && ack)
> + regmap_write_bits(info->map, REG(OCELOT_GPIO_INTR, info, gpio),
> + bit, bit);
Might we lose interrupts here, if the line would go active again right
after the read of the line state and before reading the "ack" bit?
> +
> /* Enable the interrupt now */
> gpiochip_enable_irq(chip, gpio);
> regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
> bit, bit);
>
> /*
> - * In case the interrupt line is still active and the interrupt
> - * controller has not seen any changes in the interrupt line, then it
> - * means that there happen another interrupt while the line was
> active.
> + * In case the interrupt line is still active then it means that
> + * there happen another interrupt while the line was active.
> * So we missed that one, so we need to kick the interrupt again
> * handler.
> */
> - if (active && !ack) {
> + regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
> + if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
> + (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
> + active = true;
Why do you read the line state twice? What happens if the line state
changes right after you've read it?
> +
> + if (active) {
> struct ocelot_irq_work *work;
>
> work = kmalloc(sizeof(*work), GFP_ATOMIC);
So yes, maybe the trade-off that there will be two interrupts are
better than this additional patch. But it should be documented
somewhere, even if it's just a comment in this driver.
-michael
The 10/06/2022 13:43, Michael Walle wrote:
Hi Walle,
> Seeing 20 was definitely fishy, seeing two instead of one maybe not
> so much. I guess it will create one spurious interrupt if none of
> the registered handlers will care.
>
> OTOH, the code below won't work in all cases anyway, right? It's just
> best effort.
I was expecting to work in all cases, but if you found some cases that
would not work, please point them out.
>
> > Below I have a diff that I tried with LAN8814 PHYs and I could see that
> > count in /proc/interrupts is increasing correctly.
> >
> > > I've verified that there is only one low pulse on the interrupt line.
> > > I've
> > > noticed though, that the number of interrupts seem to be correlating
> > > with
> > > the length of the low pulse.
> > ---
> > diff --git a/drivers/pinctrl/pinctrl-ocelot.c
> > b/drivers/pinctrl/pinctrl-ocelot.c
> > index c7df8c5fe5854..105771ff82e62 100644
> > --- a/drivers/pinctrl/pinctrl-ocelot.c
> > +++ b/drivers/pinctrl/pinctrl-ocelot.c
> > @@ -1863,19 +1863,28 @@ static void ocelot_irq_unmask_level(struct
> > irq_data *data)
> > if (val & bit)
> > ack = true;
> >
> > + /* Try to clear any rising edges */
> > + if (!active && ack)
> > + regmap_write_bits(info->map, REG(OCELOT_GPIO_INTR, info, gpio),
> > + bit, bit);
>
> Might we lose interrupts here, if the line would go active again right
> after the read of the line state and before reading the "ack" bit?
We lose the interrupt here, as the HW will not generate another one
but at later point we read again the line status. And if the line is
active then we kick again the interrupt handler again.
>
> > +
> > /* Enable the interrupt now */
> > gpiochip_enable_irq(chip, gpio);
> > regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
> > bit, bit);
> >
> > /*
> > - * In case the interrupt line is still active and the interrupt
> > - * controller has not seen any changes in the interrupt line, then it
> > - * means that there happen another interrupt while the line was
> > active.
> > + * In case the interrupt line is still active then it means that
> > + * there happen another interrupt while the line was active.
> > * So we missed that one, so we need to kick the interrupt again
> > * handler.
> > */
> > - if (active && !ack) {
> > + regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
> > + if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
> > + (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
> > + active = true;
>
> Why do you read the line state twice? What happens if the line state
> changes right after you've read it?
Here we need to read again the status because we might have clear the
ack of interrupt.
If the line becomes active right after this read, then the HW will
generate another interrupt as the interrupt is enabled and ack is
cleared.
>
> > +
> > + if (active) {
> > struct ocelot_irq_work *work;
> >
> > work = kmalloc(sizeof(*work), GFP_ATOMIC);
>
> So yes, maybe the trade-off that there will be two interrupts are
> better than this additional patch. But it should be documented
> somewhere, even if it's just a comment in this driver.
>
> -michael
--
/Horatiu
Hi Horatiu,
Am 2022-10-07 11:49, schrieb Horatiu Vultur:
> The 10/06/2022 13:43, Michael Walle wrote:
>
> Hi Walle,
>
>> Seeing 20 was definitely fishy, seeing two instead of one maybe not
>> so much. I guess it will create one spurious interrupt if none of
>> the registered handlers will care.
>>
>> OTOH, the code below won't work in all cases anyway, right? It's just
>> best effort.
>
> I was expecting to work in all cases, but if you found some cases that
> would not work, please point them out.
>
>>
>> > Below I have a diff that I tried with LAN8814 PHYs and I could see that
>> > count in /proc/interrupts is increasing correctly.
>> >
>> > > I've verified that there is only one low pulse on the interrupt line.
>> > > I've
>> > > noticed though, that the number of interrupts seem to be correlating
>> > > with
>> > > the length of the low pulse.
>> > ---
>> > diff --git a/drivers/pinctrl/pinctrl-ocelot.c
>> > b/drivers/pinctrl/pinctrl-ocelot.c
>> > index c7df8c5fe5854..105771ff82e62 100644
>> > --- a/drivers/pinctrl/pinctrl-ocelot.c
>> > +++ b/drivers/pinctrl/pinctrl-ocelot.c
>> > @@ -1863,19 +1863,28 @@ static void ocelot_irq_unmask_level(struct
>> > irq_data *data)
>> > if (val & bit)
>> > ack = true;
>> >
>> > + /* Try to clear any rising edges */
>> > + if (!active && ack)
>> > + regmap_write_bits(info->map, REG(OCELOT_GPIO_INTR, info, gpio),
>> > + bit, bit);
>>
>> Might we lose interrupts here, if the line would go active again right
>> after the read of the line state and before reading the "ack" bit?
>
> We lose the interrupt here, as the HW will not generate another one
> but at later point we read again the line status. And if the line is
> active then we kick again the interrupt handler again.
Ahh, thanks for explaining. That also explains the read below.
Will you send a proper patch?
-michael
>
>>
>> > +
>> > /* Enable the interrupt now */
>> > gpiochip_enable_irq(chip, gpio);
>> > regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
>> > bit, bit);
>> >
>> > /*
>> > - * In case the interrupt line is still active and the interrupt
>> > - * controller has not seen any changes in the interrupt line, then it
>> > - * means that there happen another interrupt while the line was
>> > active.
>> > + * In case the interrupt line is still active then it means that
>> > + * there happen another interrupt while the line was active.
>> > * So we missed that one, so we need to kick the interrupt again
>> > * handler.
>> > */
>> > - if (active && !ack) {
>> > + regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
>> > + if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
>> > + (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
>> > + active = true;
>>
>> Why do you read the line state twice? What happens if the line state
>> changes right after you've read it?
>
> Here we need to read again the status because we might have clear the
> ack of interrupt.
> If the line becomes active right after this read, then the HW will
> generate another interrupt as the interrupt is enabled and ack is
> cleared.
>
>>
>> > +
>> > + if (active) {
>> > struct ocelot_irq_work *work;
>> >
>> > work = kmalloc(sizeof(*work), GFP_ATOMIC);
>>
>> So yes, maybe the trade-off that there will be two interrupts are
>> better than this additional patch. But it should be documented
>> somewhere, even if it's just a comment in this driver.
>>
>> -michael
The 10/13/2022 09:30, Michael Walle wrote:
Hi Michael,
> > We lose the interrupt here, as the HW will not generate another one
> > but at later point we read again the line status. And if the line is
> > active then we kick again the interrupt handler again.
>
> Ahh, thanks for explaining. That also explains the read below.
>
> Will you send a proper patch?
No worries. Yes, I will do that.
>
> -michael
>
> >
> > >
> > > > +
> > > > /* Enable the interrupt now */
> > > > gpiochip_enable_irq(chip, gpio);
> > > > regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
> > > > bit, bit);
> > > >
> > > > /*
> > > > - * In case the interrupt line is still active and the interrupt
> > > > - * controller has not seen any changes in the interrupt line, then it
> > > > - * means that there happen another interrupt while the line was
> > > > active.
> > > > + * In case the interrupt line is still active then it means that
> > > > + * there happen another interrupt while the line was active.
> > > > * So we missed that one, so we need to kick the interrupt again
> > > > * handler.
> > > > */
> > > > - if (active && !ack) {
> > > > + regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
> > > > + if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
> > > > + (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
> > > > + active = true;
> > >
> > > Why do you read the line state twice? What happens if the line state
> > > changes right after you've read it?
> >
> > Here we need to read again the status because we might have clear the
> > ack of interrupt.
> > If the line becomes active right after this read, then the HW will
> > generate another interrupt as the interrupt is enabled and ack is
> > cleared.
> >
> > >
> > > > +
> > > > + if (active) {
> > > > struct ocelot_irq_work *work;
> > > >
> > > > work = kmalloc(sizeof(*work), GFP_ATOMIC);
> > >
> > > So yes, maybe the trade-off that there will be two interrupts are
> > > better than this additional patch. But it should be documented
> > > somewhere, even if it's just a comment in this driver.
> > >
> > > -michael
--
/Horatiu