From: Daniel Palmer <hidden> Date: 2021-09-14 10:04:24
In new SigmaStar SoCs they have moved away from having a few
interrupt capable GPIOs and instead have chained yet another
interrupt controller in to provide interrupt support for
all of the GPIOs.
I'm hardly an IRQ expert so I expect I've made a total
mess of this. No one else was going to write this driver
so I had a go.
Daniel Palmer (3):
dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi
irqchip: SigmaStar SSD20xD gpi
ARM: dts: mstar: Add gpi interrupt controller to i2m
.../sstar,ssd20xd-gpi.yaml | 53 +++++
MAINTAINERS | 2 +
arch/arm/boot/dts/mstar-infinity2m.dtsi | 8 +
drivers/irqchip/Kconfig | 11 +
drivers/irqchip/Makefile | 2 +
drivers/irqchip/irq-ssd20xd-gpi.c | 211 ++++++++++++++++++
6 files changed, 287 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml
create mode 100644 drivers/irqchip/irq-ssd20xd-gpi.c
--
2.33.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Daniel Palmer <hidden> Date: 2021-09-14 10:04:26
Add a binding description for the SigmaStar GPIO interrupt
controller, gpi, found on the SSD201 and SSD202D.
Signed-off-by: Daniel Palmer <redacted>
---
.../sstar,ssd20xd-gpi.yaml | 53 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 54 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml
@@ -0,0 +1,53 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/interrupt-controller/sstar,ssd20xd-gpi.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:SigmaStar GPIO Interrupt Controller++maintainers:+-Daniel Palmer <daniel@thingy.jp>++description:|++Newer SigmaStar SoCs from the SSD201/SSD202D have an extra+interrupt controller that is just for handling interrupts+on GPIOs and then routing a single interrupt to the peripheral+interrupt controller instead of only having a few interrupt+capable GPIOs that are routed directly to the peripheral+interrupt controller like older SoCs.++properties:+compatible:+const:sstar,ssd20xd-gpi++interrupt-controller:true++"#interrupt-cells":+const:2++reg:+maxItems:1++interrupts:+maxItems:1++required:+-compatible+-reg+-interrupts++additionalProperties:false++examples:+-|+#include <dt-bindings/interrupt-controller/irq.h>+#include <dt-bindings/interrupt-controller/arm-gic.h>+interrupt-controller@207a00 {+compatible = "sstar,ssd20xd-gpi";+reg = <0x207a00 0x200>;+#interrupt-cells = <2>;+interrupt-controller;+interrupts-extended = <&intc_irq GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;+};+...
From: Daniel Palmer <hidden> Date: 2021-09-14 10:04:28
Add support for the SigmaStar GPIO interrupt controller, gpi,
that is present in SSD201 and SSD202D SoCs.
This routes interrupts from many interrupts into a single
interrupt that is connected to the peripheral interrupt
controller.
Signed-off-by: Daniel Palmer <redacted>
---
MAINTAINERS | 1 +
drivers/irqchip/Kconfig | 11 ++
drivers/irqchip/Makefile | 2 +
drivers/irqchip/irq-ssd20xd-gpi.c | 211 ++++++++++++++++++++++++++++++
4 files changed, 225 insertions(+)
create mode 100644 drivers/irqchip/irq-ssd20xd-gpi.c
From: Daniel Palmer <hidden> Date: 2021-09-14 10:04:31
infinity2m chips have the GPI interrupt controller
for GPIOs so add it to the dtsi for infinity2m.
Signed-off-by: Daniel Palmer <redacted>
---
arch/arm/boot/dts/mstar-infinity2m.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-09-14 15:59:39
On Tue, Sep 14, 2021 at 07:04:12PM +0900, Daniel Palmer wrote:
In new SigmaStar SoCs they have moved away from having a few
interrupt capable GPIOs and instead have chained yet another
interrupt controller in to provide interrupt support for
all of the GPIOs.
I'm hardly an IRQ expert so I expect I've made a total
mess of this. No one else was going to write this driver
so I had a go.
Hi Daniel
How are the GPIOs mapped to the interrupts? Is it a simple 1:1?
The GPIO core has some support for the GPIO drivers to be also
interrupt controllers. So if this interrupt control is dedicated to
GPIO, you would be better to make it part of the GPIO driver.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Daniel Palmer <hidden> Date: 2021-09-15 09:07:05
Hi Andrew,
On Wed, 15 Sept 2021 at 00:59, Andrew Lunn [off-list ref] wrote:
How are the GPIOs mapped to the interrupts? Is it a simple 1:1?
Unfortunately, no.
I wanted to add the GPIO controller part of this to this same series
but there are some patches in flight for that so it would have been
messy.
You can see that here though:
https://github.com/linux-chenxing/linux/commit/88345dc470bf07d36aa1ddab09551ed33a1cfb22
They've really made a mess of this. Their whole GPIO thing is a mess
with no clear logic between the pin names and the register locations
etc.
This IRQ part is no exception. IRQ 0 from this thing isn't for the pin
called GPIO0 or anything sane like that.
The GPIO core has some support for the GPIO drivers to be also
interrupt controllers. So if this interrupt control is dedicated to
GPIO, you would be better to make it part of the GPIO driver.
I don't think so. One reason is the non-linear mapping stuff. A second
reason is this GPIO interrupt controller might handle GPIO interrupts
for multiple GPIO controller blocks.
Finally, in newer chips they've replaced one of the GPIO blocks with a
new IP which will need it's own driver. That GPIO controller still
seems to use this same IRQ block to handle it's interrupts.
So if this code is wrapped into the GPIO driver itself it would end up
duplicated in two GPIO drivers.
Cheers,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-09-15 20:34:38
On Wed, Sep 15, 2021 at 06:06:52PM +0900, Daniel Palmer wrote:
Hi Andrew,
On Wed, 15 Sept 2021 at 00:59, Andrew Lunn [off-list ref] wrote:
quoted
How are the GPIOs mapped to the interrupts? Is it a simple 1:1?
Unfortunately, no.
I wanted to add the GPIO controller part of this to this same series
but there are some patches in flight for that so it would have been
messy.
You can see that here though:
https://github.com/linux-chenxing/linux/commit/88345dc470bf07d36aa1ddab09551ed33a1cfb22
They've really made a mess of this. Their whole GPIO thing is a mess
with no clear logic between the pin names and the register locations
etc.
This IRQ part is no exception. IRQ 0 from this thing isn't for the pin
called GPIO0 or anything sane like that.
O.K. Then it sounds like splitting GPIO and the IRQ makes sense. This
is the sort of thing which is good to put in the cover letter,
explaining why you decided to do it this way.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Daniel Palmer <hidden> Date: 2021-09-20 08:36:38
Hi Andrew,
On Thu, 16 Sept 2021 at 05:34, Andrew Lunn [off-list ref] wrote:
O.K. Then it sounds like splitting GPIO and the IRQ makes sense. This
is the sort of thing which is good to put in the cover letter,
explaining why you decided to do it this way.
Good point. I'll probably need to send a v2 at some point so I'll add
it to the cover letter then.
I have a working driver for the other GPIO IP block that uses this
interrupt controller now so I can link that too.
Thanks,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-20 09:45:24
On Tue, 14 Sep 2021 11:04:14 +0100,
Daniel Palmer [off-list ref] wrote:
quoted hunk
Add support for the SigmaStar GPIO interrupt controller, gpi,
that is present in SSD201 and SSD202D SoCs.
This routes interrupts from many interrupts into a single
interrupt that is connected to the peripheral interrupt
controller.
Signed-off-by: Daniel Palmer <redacted>
---
MAINTAINERS | 1 +
drivers/irqchip/Kconfig | 11 ++
drivers/irqchip/Makefile | 2 +
drivers/irqchip/irq-ssd20xd-gpi.c | 211 ++++++++++++++++++++++++++++++
4 files changed, 225 insertions(+)
create mode 100644 drivers/irqchip/irq-ssd20xd-gpi.c
Is this regmap call atomic? When running this, you are holding a
raw_spinlock already. From what I can see, this is unlikely to work
correctly with the current state of regmap.
quoted hunk
+}++static void ssd20xd_gpi_irq_eoi(struct irq_data *data)+{+ struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data);+ irq_hw_number_t hwirq = irqd_to_hwirq(data);+ int offset_reg = REG_OFFSET(hwirq);+ int offset_bit = BIT_OFFSET(hwirq);++ regmap_update_bits_base(gpi->regmap, REG_ACK + offset_reg,+ offset_bit, offset_bit, NULL, false, true);+}++static int ssd20xd_gpi_set_type_irq(struct irq_data *data, unsigned int flow_type)+{+ irq_hw_number_t hwirq = irqd_to_hwirq(data);+ struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data);+ int offset_reg = REG_OFFSET(hwirq);+ int offset_bit = BIT_OFFSET(hwirq);++ switch (flow_type) {+ case IRQ_TYPE_EDGE_FALLING:+ regmap_update_bits(gpi->regmap, REG_TYPE + offset_reg, offset_bit, offset_bit);+ break;+ case IRQ_TYPE_EDGE_RISING:+ regmap_update_bits(gpi->regmap, REG_TYPE + offset_reg, offset_bit, 0);+ break;+ default:+ return -EINVAL;+ }++ return 0;+}++static struct irq_chip ssd20xd_gpi_chip = {+ .name = "GPI",+ .irq_mask = ssd20xd_gpi_mask_irq,+ .irq_unmask = ssd20xd_gpi_unmask_irq,+ .irq_eoi = ssd20xd_gpi_irq_eoi,+ .irq_set_type = ssd20xd_gpi_set_type_irq,+};++static int ssd20xd_gpi_domain_alloc(struct irq_domain *domain, unsigned int virq,+ unsigned int nr_irqs, void *arg)+{+ struct ssd20xd_gpi *intc = domain->host_data;+ struct irq_fwspec *fwspec = arg;+ int i;++ for (i = 0; i < nr_irqs; i++)+ irq_domain_set_info(domain, virq + i, fwspec->param[0] + i,+ &ssd20xd_gpi_chip, intc, handle_fasteoi_irq, NULL, NULL);++ return 0;+}++static void ssd20xd_gpi_domain_free(struct irq_domain *domain, unsigned int virq,+ unsigned int nr_irqs)+{+ int i;++ for (i = 0; i < nr_irqs; i++) {+ struct irq_data *d = irq_domain_get_irq_data(domain, virq + i);++ irq_set_handler(virq + i, NULL);+ irq_domain_reset_irq_data(d);+ }+}++static const struct irq_domain_ops ssd20xd_gpi_domain_ops = {+ .alloc = ssd20xd_gpi_domain_alloc,+ .free = ssd20xd_gpi_domain_free,+};++static const struct regmap_config ssd20xd_gpi_regmap_config = {+ .reg_bits = 16,+ .val_bits = 16,+ .reg_stride = 4,+};++static void ssd20x_gpi_chainedhandler(struct irq_desc *desc)+{+ struct ssd20xd_gpi *intc = irq_desc_get_handler_data(desc);+ struct irq_chip *chip = irq_desc_get_chip(desc);+ unsigned int irqbit, hwirq, virq, status;+ int i;++ chained_irq_enter(chip, desc);++ for (i = 0; i < NUM_IRQ / IRQS_PER_REG; i++) {+ int offset_reg = STRIDE * i;+ int offset_irq = IRQS_PER_REG * i;++ regmap_read(intc->regmap, REG_STATUS + offset_reg, &status);
Does this act as an ACK in the HW?
quoted hunk
++ while (status) {+ irqbit = __ffs(status);+ hwirq = offset_irq + irqbit;+ virq = irq_find_mapping(intc->domain, hwirq);+ if (virq)+ generic_handle_irq(virq);
Please replace this with generic_handle_domain_irq().
quoted hunk
+ status &= ~BIT(irqbit);+ }+ }++ chained_irq_exit(chip, desc);+}++static int __init ssd20xd_gpi_of_init(struct device_node *node,+ struct device_node *parent)+{+ struct ssd20xd_gpi *intc;+ void __iomem *base;+ int irq, ret;++ intc = kzalloc(sizeof(*intc), GFP_KERNEL);+ if (!intc)+ return -ENOMEM;++ base = of_iomap(node, 0);+ if (!base) {+ ret = -ENODEV;+ goto out_free;+ }++ intc->regmap = regmap_init_mmio(NULL, base, &ssd20xd_gpi_regmap_config);+ if (IS_ERR(intc->regmap)) {+ ret = PTR_ERR(intc->regmap);+ goto out_unmap;+ }++ intc->domain = irq_domain_add_linear(node, NUM_IRQ, &ssd20xd_gpi_domain_ops, intc);+ if (!intc->domain) {+ ret = -ENOMEM;+ goto out_free_regmap;+ }++ irq = of_irq_get(node, 0);+ if (irq <= 0) {+ ret = irq;+ goto out_free_domain;+ }++ irq_set_chained_handler_and_data(irq, ssd20x_gpi_chainedhandler,+ intc);++ return 0;++out_free_domain:+ irq_domain_remove(intc->domain);+out_free_regmap:+ regmap_exit(intc->regmap);+out_unmap:+ iounmap(base);+out_free:+ kfree(intc);+ return ret;+}++IRQCHIP_DECLARE(ssd20xd_gpi, "sstar,ssd20xd-gpi", ssd20xd_gpi_of_init);
Is there any reason why this isn't a standard platform device? In
general, irqchips that are not part of the root hierarchy shouldn't
need this anymore.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Is this regmap call atomic? When running this, you are holding a
raw_spinlock already. From what I can see, this is unlikely to work
correctly with the current state of regmap.
I didn't even think about it. I will check.
quoted
+static void ssd20x_gpi_chainedhandler(struct irq_desc *desc)+{+ struct ssd20xd_gpi *intc = irq_desc_get_handler_data(desc);+ struct irq_chip *chip = irq_desc_get_chip(desc);+ unsigned int irqbit, hwirq, virq, status;+ int i;++ chained_irq_enter(chip, desc);++ for (i = 0; i < NUM_IRQ / IRQS_PER_REG; i++) {+ int offset_reg = STRIDE * i;+ int offset_irq = IRQS_PER_REG * i;++ regmap_read(intc->regmap, REG_STATUS + offset_reg, &status);
Does this act as an ACK in the HW?
Not that I'm aware of. The status registers have the interrupt bits
set until the EOI callback is called from what I can tell.
Technically I think the EOI callback should actually be ACK instead
but from my fuzzy memory with the stack of IRQ controllers that
resulted in a null pointer dereference.
quoted
++ while (status) {+ irqbit = __ffs(status);+ hwirq = offset_irq + irqbit;+ virq = irq_find_mapping(intc->domain, hwirq);+ if (virq)+ generic_handle_irq(virq);
Please replace this with generic_handle_domain_irq().
Is there any reason why this isn't a standard platform device? In
general, irqchips that are not part of the root hierarchy shouldn't
need this anymore.
Is this regmap call atomic? When running this, you are holding a
raw_spinlock already. From what I can see, this is unlikely to work
correctly with the current state of regmap.
I didn't even think about it. I will check.
You may want to enable lockdep to verify that.
quoted
quoted
+static void ssd20x_gpi_chainedhandler(struct irq_desc *desc)+{+ struct ssd20xd_gpi *intc = irq_desc_get_handler_data(desc);+ struct irq_chip *chip = irq_desc_get_chip(desc);+ unsigned int irqbit, hwirq, virq, status;+ int i;++ chained_irq_enter(chip, desc);++ for (i = 0; i < NUM_IRQ / IRQS_PER_REG; i++) {+ int offset_reg = STRIDE * i;+ int offset_irq = IRQS_PER_REG * i;++ regmap_read(intc->regmap, REG_STATUS + offset_reg, &status);
Does this act as an ACK in the HW?
Not that I'm aware of. The status registers have the interrupt bits
set until the EOI callback is called from what I can tell.
Then this doesn't work for edge signalling, as you will lose
interrupts that occur between the handling and EOI.
Technically I think the EOI callback should actually be ACK instead
but from my fuzzy memory with the stack of IRQ controllers that
resulted in a null pointer dereference.
That's probably because you are using the wrong flow handler. You
should turn this irq_eoi into an irq_ack, because that's really what
it is, and use handle_edge_irq() as the flow handler.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Daniel Palmer <hidden> Date: 2021-09-21 04:16:50
Hi Marc,
On Mon, 20 Sept 2021 at 20:24, Marc Zyngier [off-list ref] wrote:
quoted
quoted
Is this regmap call atomic? When running this, you are holding a
raw_spinlock already. From what I can see, this is unlikely to work
correctly with the current state of regmap.
I didn't even think about it. I will check.
You may want to enable lockdep to verify that.
I've just checked with lockdep and while it doesn't complain about
this code it complains about similar code I have somewhere else that's
almost identical (yet another interrupt controller driver I needed to
write..).
So it probably doesn't work properly as you say. I'll fix that up.
quoted
Technically I think the EOI callback should actually be ACK instead
but from my fuzzy memory with the stack of IRQ controllers that
resulted in a null pointer dereference.
That's probably because you are using the wrong flow handler. You
should turn this irq_eoi into an irq_ack, because that's really what
it is, and use handle_edge_irq() as the flow handler.
If I do that (put the ack clearing callback into the ack slot, change
the handler to handle_edge_irq()) I get this explosion:
# gpiomon -r 0 44
[ 23.449571] 8<--- cut here ---
[ 23.452673] Unable to handle kernel NULL pointer dereference at
virtual address 00000000
[ 23.460804] pgd = (ptrval)
[ 23.463534] [00000000] *pgd=23530835, *pte=00000000, *ppte=00000000
[ 23.469868] Internal error: Oops: 80000007 [#1] SMP ARM
[ 23.475128] Modules linked in:
[ 23.478211] CPU: 0 PID: 193 Comm: gpiomon Not tainted 5.15.0-rc2+ #2565
[ 23.484866] Hardware name: MStar/Sigmastar Armv7 (Device Tree)
[ 23.490727] PC is at 0x0
[ 23.493283] LR is at handle_edge_irq+0x88/0xfc
[ 23.497764] pc : [<00000000>] lr : [<c0180694>] psr: a0040193
[ 23.504064] sp : c2405d90 ip : 00000000 fp : c35a786c
[ 23.509318] r10: 00000001 r9 : c1b96fc0 r8 : 00000020
[ 23.514572] r7 : 000000c8 r6 : c35a786c r5 : c35a7818 r4 : c35a7800
[ 23.521135] r3 : 00000000 r2 : 000015d6 r1 : 06f80000 r0 : c35a7818
This one is because the GPIO controller irqchip that is on top of this
doesn't have an ack callback.
So if I set irq_chip_ack_parent as the ack callback I get another explosion:
# gpiomon -r 0 44
[ 22.370689] 8<--- cut here ---
[ 22.373802] Unable to handle kernel NULL pointer dereference at
virtual address 00000018
[ 22.381945] pgd = (ptrval)
[ 22.384685] [00000018] *pgd=235cb835, *pte=00000000, *ppte=00000000
[ 22.391038] Internal error: Oops: 17 [#1] SMP ARM
[ 22.395776] Modules linked in:
[ 22.398860] CPU: 1 PID: 193 Comm: gpiomon Not tainted 5.15.0-rc2+ #2566
[ 22.405515] Hardware name: MStar/Sigmastar Armv7 (Device Tree)
[ 22.411376] PC is at irq_chip_ack_parent+0x8/0x10
[ 22.416120] LR is at __irq_do_set_handler+0x3c/0x11c
[ 22.421119] pc : [<c017f498>] lr : [<c018029c>] psr: a0040093
[ 22.427419] sp : c3505d68 ip : ffffe000 fp : 00000000
[ 22.432673] r10: c0d592d4 r9 : 00000001 r8 : 00000000
[ 22.437927] r7 : c3502618 r6 : 00000000 r5 : c017b9cc r4 : c3502600
[ 22.444489] r3 : 00000000 r2 : c10bb294 r1 : c10bb294 r0 : c26a3440
[ 22.451053] Flags: NzCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment user
[ 22.458317] Control: 10c5387d Table: 235b006a DAC: 00000055
---snip---
[ 22.725196] [<c017f498>] (irq_chip_ack_parent) from [<c018029c>]
(__irq_do_set_handler+0x3c/0x11c)
[ 22.734219] [<c018029c>] (__irq_do_set_handler) from [<c01803b4>]
(__irq_set_handler+0x38/0x50)
[ 22.742976] [<c01803b4>] (__irq_set_handler) from [<c0181880>]
(irq_domain_set_info+0x34/0x48)
[ 22.751649] [<c0181880>] (irq_domain_set_info) from [<c046f838>]
(gpiochip_hierarchy_irq_domain_alloc+0x104/0x228)
[ 22.762069] [<c046f838>] (gpiochip_hierarchy_irq_domain_alloc) from
[<c0182c38>] (__irq_domain_alloc_irqs+0xd8/0x318)
[ 22.772748] [<c0182c38>] (__irq_domain_alloc_irqs) from
[<c01832e8>] (irq_create_fwspec_mapping+0x22c/0x298)
[ 22.782641] [<c01832e8>] (irq_create_fwspec_mapping) from
[<c0470124>] (gpiochip_to_irq+0x60/0x84)
[ 22.791664] [<c0470124>] (gpiochip_to_irq) from [<c046ef18>]
(gpiod_to_irq+0x48/0x60)
[ 22.799552] [<c046ef18>] (gpiod_to_irq) from [<c0477a48>]
(gpio_ioctl+0x1b4/0x420)
[ 22.807178] [<c0477a48>] (gpio_ioctl) from [<c0262e4c>] (vfs_ioctl+0x20/0x38)
[ 22.814371] [<c0262e4c>] (vfs_ioctl) from [<c0263708>] (sys_ioctl+0xb0/0x818)
[ 22.821564] [<c0263708>] (sys_ioctl) from [<c0100060>]
(ret_fast_syscall+0x0/0x1c)
[ 22.829190] Exception stack(0xc3505fa8 to 0xc3505ff0)
[ 22.834273] 5fa0: ???????? ???????? ????????
???????? ???????? ????????
[ 22.842488] 5fc0: ???????? ???????? ???????? ???????? ????????
???????? ???????? ????????
[ 22.850701] 5fe0: ???????? ???????? ???????? ????????
[ 22.855790] Code: e593301c e12fff13 e5900018 e5903010 (e5933018)
[ 22.861919] ---[ end trace 10524aa06eced7e3 ]---
If I do something silly like the following diff the explosion stops
and GPIO interrupt fires correctly each time I press the button it's
connected to:
void irq_chip_ack_parent(struct irq_data *data)
{
data = data->parent_data;
+ if(!data->chip)
+ return;
data->chip->irq_ack(data);
}
EXPORT_SYMBOL_GPL(irq_chip_ack_parent);
I think I got stuck at this, switched it to the eoi handler instead
and then forgot about it.
I'll work out the proper solution for this for v2.
Cheers,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Is this regmap call atomic? When running this, you are holding a
raw_spinlock already. From what I can see, this is unlikely to work
correctly with the current state of regmap.
I didn't even think about it. I will check.
You may want to enable lockdep to verify that.
After some research I think this can be solved by adding
".use_raw_spinlock = true" to the regmap config to force using a
raw_spinlock instead of the default spinlock.
This avoids having a spinlock inside of a raw_spinlock.
lockdep doesn't actually complain about this currently but another
interrupt controller driver I have uses a syscon because the interrupt
registers are mixed in with unrelated stuff.
lockdep is complaining about the spinlock inside of a raw_spinlock
there. So I guess I'll need to add a new DT property for syscon to use
raw_spinlocks for that driver.
Cheers,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-21 08:27:36
On Tue, 21 Sep 2021 05:16:35 +0100,
Daniel Palmer [off-list ref] wrote:
+ Linus.
So if I set irq_chip_ack_parent as the ack callback I get another explosion:
# gpiomon -r 0 44
[ 22.370689] 8<--- cut here ---
[ 22.373802] Unable to handle kernel NULL pointer dereference at
virtual address 00000018
[ 22.381945] pgd = (ptrval)
[ 22.384685] [00000018] *pgd=235cb835, *pte=00000000, *ppte=00000000
[ 22.391038] Internal error: Oops: 17 [#1] SMP ARM
[ 22.395776] Modules linked in:
[ 22.398860] CPU: 1 PID: 193 Comm: gpiomon Not tainted 5.15.0-rc2+ #2566
[ 22.405515] Hardware name: MStar/Sigmastar Armv7 (Device Tree)
[ 22.411376] PC is at irq_chip_ack_parent+0x8/0x10
[ 22.416120] LR is at __irq_do_set_handler+0x3c/0x11c
[ 22.421119] pc : [<c017f498>] lr : [<c018029c>] psr: a0040093
[ 22.427419] sp : c3505d68 ip : ffffe000 fp : 00000000
[ 22.432673] r10: c0d592d4 r9 : 00000001 r8 : 00000000
[ 22.437927] r7 : c3502618 r6 : 00000000 r5 : c017b9cc r4 : c3502600
[ 22.444489] r3 : 00000000 r2 : c10bb294 r1 : c10bb294 r0 : c26a3440
[ 22.451053] Flags: NzCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment user
[ 22.458317] Control: 10c5387d Table: 235b006a DAC: 00000055
---snip---
[ 22.725196] [<c017f498>] (irq_chip_ack_parent) from [<c018029c>]
(__irq_do_set_handler+0x3c/0x11c)
[ 22.734219] [<c018029c>] (__irq_do_set_handler) from [<c01803b4>]
(__irq_set_handler+0x38/0x50)
[ 22.742976] [<c01803b4>] (__irq_set_handler) from [<c0181880>]
(irq_domain_set_info+0x34/0x48)
[ 22.751649] [<c0181880>] (irq_domain_set_info) from [<c046f838>]
(gpiochip_hierarchy_irq_domain_alloc+0x104/0x228)
[ 22.762069] [<c046f838>] (gpiochip_hierarchy_irq_domain_alloc) from
[<c0182c38>] (__irq_domain_alloc_irqs+0xd8/0x318)
[ 22.772748] [<c0182c38>] (__irq_domain_alloc_irqs) from
[<c01832e8>] (irq_create_fwspec_mapping+0x22c/0x298)
[ 22.782641] [<c01832e8>] (irq_create_fwspec_mapping) from
[<c0470124>] (gpiochip_to_irq+0x60/0x84)
[ 22.791664] [<c0470124>] (gpiochip_to_irq) from [<c046ef18>]
(gpiod_to_irq+0x48/0x60)
[ 22.799552] [<c046ef18>] (gpiod_to_irq) from [<c0477a48>]
(gpio_ioctl+0x1b4/0x420)
[ 22.807178] [<c0477a48>] (gpio_ioctl) from [<c0262e4c>] (vfs_ioctl+0x20/0x38)
[ 22.814371] [<c0262e4c>] (vfs_ioctl) from [<c0263708>] (sys_ioctl+0xb0/0x818)
[ 22.821564] [<c0263708>] (sys_ioctl) from [<c0100060>]
(ret_fast_syscall+0x0/0x1c)
[ 22.829190] Exception stack(0xc3505fa8 to 0xc3505ff0)
[ 22.834273] 5fa0: ???????? ???????? ????????
???????? ???????? ????????
[ 22.842488] 5fc0: ???????? ???????? ???????? ???????? ????????
???????? ???????? ????????
[ 22.850701] 5fe0: ???????? ???????? ???????? ????????
[ 22.855790] Code: e593301c e12fff13 e5900018 e5903010 (e5933018)
[ 22.861919] ---[ end trace 10524aa06eced7e3 ]---
This seems to be caused by your GPIO driver installing a flow handler
(via irq_domain_set_info()), which is a bit odd. I would expect that
only the root irqchip in the hierarchy would do that.
At the point where this is called, the hierarchy isn't fully populated
(the irq_domain_alloc_irqs_parent() call comes after that), and
irq_chip_ack_parent() explodes as above.
Linus: is there a reason why the gpiolib insist on setting its own
handler while building the hierarchy? I guess this could be worked
around by swapping the calls to irq_domain_set_info and
irq_domain_alloc_irqs_parent, but having two levels of the hierarchy
competing for the flow handler looks a bit odd.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Sep 21, 2021 at 10:27 AM Marc Zyngier [off-list ref] wrote:
Linus: is there a reason why the gpiolib insist on setting its own
handler while building the hierarchy?
Is it this?
/*
* We set handle_bad_irq because the .set_type() should
* always be invoked and set the right type of handler.
*/
irq_domain_set_info(d,
irq,
hwirq,
gc->irq.chip,
gc,
girq->handler,
NULL, NULL);
irq_set_probe(irq);
(...)
IIUC it's because sometimes, on elder systems (such as ixp4xx) some machines
are still using boardfiles, and drivers are not obtaining IRQs dynamically
from device tree or ACPI, instead they are set up statically at machine
init.
I assume it would otherwise be done as part of ops->translate?
I suppose it could be solved with a patch that take this route only if
we're not using device tree or ACPI?
If this is the totally wrong answer then forgive me... a bit tired.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Rob Herring <robh@kernel.org> Date: 2021-09-21 20:36:06
On Tue, 14 Sep 2021 19:04:13 +0900, Daniel Palmer wrote:
Add a binding description for the SigmaStar GPIO interrupt
controller, gpi, found on the SSD201 and SSD202D.
Signed-off-by: Daniel Palmer <redacted>
---
.../sstar,ssd20xd-gpi.yaml | 53 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 54 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml
From: Daniel Palmer <hidden> Date: 2021-09-30 12:39:16
Hi Linus,
On Wed, 22 Sept 2021 at 03:23, Linus Walleij [off-list ref] wrote:
I suppose it could be solved with a patch that take this route only if
we're not using device tree or ACPI?
Is that something I could do with a small patch in my series or is
there the potential to totally break everyone else's stuff to make
mine work?
Thanks,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-30 13:06:21
[huh, missed this email...]
On Tue, 21 Sep 2021 19:23:04 +0100,
Linus Walleij [off-list ref] wrote:
On Tue, Sep 21, 2021 at 10:27 AM Marc Zyngier [off-list ref] wrote:
quoted
Linus: is there a reason why the gpiolib insist on setting its own
handler while building the hierarchy?
Is it this?
/*
* We set handle_bad_irq because the .set_type() should
* always be invoked and set the right type of handler.
*/
irq_domain_set_info(d,
irq,
hwirq,
gc->irq.chip,
gc,
girq->handler,
NULL, NULL);
irq_set_probe(irq);
(...)
It is its relative position wrt to irq_domain_alloc_irqs_parent() that
has the potential for annoyance. irq_domain_set_info() will trigger an
irq startup, which will explode if the parent level hasn't been
initialised correctly.
IIUC it's because sometimes, on elder systems (such as ixp4xx) some machines
are still using boardfiles, and drivers are not obtaining IRQs dynamically
from device tree or ACPI, instead they are set up statically at machine
init.
I assume it would otherwise be done as part of ops->translate?
No, this is the right spot if you really need to set the handler. But
it should really be after the parent allocation (see below for
something totally untested).
Ultimately, setting the flow handler when there is a parent domain is
a bit odd, as you'd expect the root domain to be in charge of the
overall flow.
Thanks,
M.
@@ -1103,19 +1103,6 @@ static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,}chip_dbg(gc,"found parent hwirq %u\n",parent_hwirq);-/*-*Wesethandle_bad_irqbecausethe.set_type()should-*alwaysbeinvokedandsettherighttypeofhandler.-*/-irq_domain_set_info(d,-irq,-hwirq,-gc->irq.chip,-gc,-girq->handler,-NULL,NULL);-irq_set_probe(irq);-/* This parent only handles asserted level IRQs */parent_arg=girq->populate_parent_alloc_arg(gc,parent_hwirq,parent_type);if(!parent_arg)
@@ -1137,6 +1124,18 @@ static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,parent_hwirq,hwirq);kfree(parent_arg);++if(!ret){+irq_domain_set_info(d,+irq,+hwirq,+gc->irq.chip,+gc,+girq->handler,+NULL,NULL);+irq_set_probe(irq);+}+returnret;}
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-30 13:07:40
On Thu, 30 Sep 2021 13:39:04 +0100,
Daniel Palmer [off-list ref] wrote:
Hi Linus,
On Wed, 22 Sept 2021 at 03:23, Linus Walleij [off-list ref] wrote:
quoted
I suppose it could be solved with a patch that take this route only if
we're not using device tree or ACPI?
Is that something I could do with a small patch in my series or is
there the potential to totally break everyone else's stuff to make
mine work?
Can you give the hack that sits in my reply to Linus a go?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Daniel Palmer <hidden> Date: 2021-09-30 13:37:08
Hi Marc,
On Thu, 30 Sept 2021 at 22:06, Marc Zyngier [off-list ref] wrote:
No, this is the right spot if you really need to set the handler. But
it should really be after the parent allocation (see below for
something totally untested).
Your change resolves the null pointer difference when enabling the
interrupt but when it triggers the below happens.
This might just be my driver so I'll try to debug.
Thanks,
Daniel
# gpiomon -r 0 44
[ 61.770519] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0
[ 61.770646]
[ 61.770659] =============================
[ 61.770670] [ BUG: Invalid wait context ]
[ 61.770683] 5.15.0-rc2+ #2583 Not tainted
[ 61.770702] -----------------------------
[ 61.770712] swapper/0/0 is trying to lock:
[ 61.770729] c1789eb0 (&port_lock_key){-.-.}-{3:3}, at:
serial8250_console_write+0x1b0/0x254
[ 61.770840] other info that might help us debug this:
[ 61.770853] context-{2:2}
[ 61.770868] 2 locks held by swapper/0/0:
[ 61.770889] #0: c10189ec (console_lock){+.+.}-{0:0}, at:
vprintk_emit+0xa4/0x200
[ 61.770986] #1: c1018b44 (console_owner){-...}-{0:0}, at:
console_unlock+0x1e8/0x4b4
[ 61.771080] stack backtrace:
[ 61.771093] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.15.0-rc2+ #2583
[ 61.771130] Hardware name: MStar/Sigmastar Armv7 (Device Tree)
[ 61.771156] [<c010daf0>] (unwind_backtrace) from [<c0109f54>]
(show_stack+0x10/0x14)
[ 61.771235] [<c0109f54>] (show_stack) from [<c09303a0>]
(dump_stack_lvl+0x58/0x70)
[ 61.771312] [<c09303a0>] (dump_stack_lvl) from [<c016fbd8>]
(__lock_acquire+0x384/0x16a0)
[ 61.771394] [<c016fbd8>] (__lock_acquire) from [<c017191c>]
(lock_acquire+0x2a0/0x320)
[ 61.771470] [<c017191c>] (lock_acquire) from [<c093de84>]
(_raw_spin_lock_irqsave+0x5c/0x70)
[ 61.771542] [<c093de84>] (_raw_spin_lock_irqsave) from [<c04cdf98>]
(serial8250_console_write+0x1b0/0x254)
[ 61.771620] [<c04cdf98>] (serial8250_console_write) from
[<c0178068>] (console_unlock+0x3fc/0x4b4)
[ 61.771699] [<c0178068>] (console_unlock) from [<c0179750>]
(vprintk_emit+0x1d0/0x200)
[ 61.771769] [<c0179750>] (vprintk_emit) from [<c017979c>]
(vprintk_default+0x1c/0x24)
[ 61.771840] [<c017979c>] (vprintk_default) from [<c092d178>]
(_printk+0x18/0x28)
[ 61.771914] [<c092d178>] (_printk) from [<c017b9d0>]
(handle_bad_irq+0x44/0x22c)
[ 61.771991] [<c017b9d0>] (handle_bad_irq) from [<c017ade4>]
(handle_irq_desc+0x24/0x34)
[ 61.772066] [<c017ade4>] (handle_irq_desc) from [<c0467274>]
(ssd20xd_gpi_chainedhandler+0xb4/0xc4)
[ 61.772147] [<c0467274>] (ssd20xd_gpi_chainedhandler) from
[<c017ade4>] (handle_irq_desc+0x24/0x34)
[ 61.772223] [<c017ade4>] (handle_irq_desc) from [<c017b544>]
(handle_domain_irq+0x40/0x54)
[ 61.772299] [<c017b544>] (handle_domain_irq) from [<c0465f20>]
(gic_handle_irq+0x60/0x6c)
[ 61.772372] [<c0465f20>] (gic_handle_irq) from [<c0100aac>]
(__irq_svc+0x4c/0x64)
[ 61.772435] Exception stack(0xc1001f20 to 0xc1001f68)
[ 61.772466] 1f20: ???????? ???????? ???????? ???????? ????????
???????? ???????? ????????
[ 61.772490] 1f40: ???????? ???????? ???????? ???????? ????????
???????? ???????? ????????
[ 61.772510] 1f60: ???????? ????????
[ 61.772531] [<c0100aac>] (__irq_svc) from [<c010715c>]
(arch_cpu_idle+0x1c/0x38)
[ 61.772605] [<c010715c>] (arch_cpu_idle) from [<c093dc44>]
(default_idle_call+0x50/0x8c)
[ 61.772677] [<c093dc44>] (default_idle_call) from [<c0155984>]
(do_idle+0xf0/0x25c)
[ 61.772743] [<c0155984>] (do_idle) from [<c0155ea4>]
(cpu_startup_entry+0x18/0x1c)
[ 61.772807] [<c0155ea4>] (cpu_startup_entry) from [<c0f00e58>]
(start_kernel+0x560/0x628)
[ 62.055133] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c
[ 62.061099] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90
[ 62.067585] ->action(): (ptrval)
[ 62.070833] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c
[ 62.077664] unexpected IRQ trap at vector 42
[ 62.082014] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0
[ 62.088411] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c
[ 62.094377] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90
[ 62.100862] ->action(): (ptrval)
[ 62.104111] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c
[ 62.110941] unexpected IRQ trap at vector 42
[ 62.115312] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0
[ 62.121712] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c
[ 62.127675] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90
[ 62.134165] ->action(): (ptrval)
[ 62.137416] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c
[ 62.144246] unexpected IRQ trap at vector 42
[ 62.148588] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0
[ 62.154988] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c
[ 62.160955] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90
[ 62.167440] ->action(): (ptrval)
[ 62.170689] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c
[ 62.177517] unexpected IRQ trap at vector 42
[ 62.181840] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0
[ 62.188237] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c
[ 62.194201] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90
[ 62.200686] ->action(): (ptrval)
[ 62.203934] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c
[ 62.210763] unexpected IRQ trap at vector 42
[ 62.215095] unexpected IRQ trap at vector 42
[ 62.219424] unexpected IRQ trap at vector 42
[ 62.223759] unexpected IRQ trap at vector 42
[ 62.228084] unexpected IRQ trap at vector 42
[ 62.232407] unexpected IRQ trap at vector 42
[ 62.236729] unexpected IRQ trap at vector 42
[ 62.241052] unexpected IRQ trap at vector 42
[ 62.245377] unexpected IRQ trap at vector 42
[ 62.249703] unexpected IRQ trap at vector 42
[ 62.254037] unexpected IRQ trap at vector 42
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-30 13:53:52
On Thu, 30 Sep 2021 14:36:52 +0100,
Daniel Palmer [off-list ref] wrote:
Hi Marc,
On Thu, 30 Sept 2021 at 22:06, Marc Zyngier [off-list ref] wrote:
quoted
No, this is the right spot if you really need to set the handler. But
it should really be after the parent allocation (see below for
something totally untested).
Your change resolves the null pointer difference when enabling the
interrupt but when it triggers the below happens.
This might just be my driver so I'll try to debug.
Thanks,
Daniel
# gpiomon -r 0 44
[ 61.770519] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0
[ 61.770646]
[ 61.770659] =============================
[ 61.770670] [ BUG: Invalid wait context ]
[ 61.770683] 5.15.0-rc2+ #2583 Not tainted
[ 61.770702] -----------------------------
[ 61.770712] swapper/0/0 is trying to lock:
[ 61.770729] c1789eb0 (&port_lock_key){-.-.}-{3:3}, at:
serial8250_console_write+0x1b0/0x254
[ 61.770840] other info that might help us debug this:
[ 61.770853] context-{2:2}
[ 61.770868] 2 locks held by swapper/0/0:
[ 61.770889] #0: c10189ec (console_lock){+.+.}-{0:0}, at:
vprintk_emit+0xa4/0x200
[ 61.770986] #1: c1018b44 (console_owner){-...}-{0:0}, at:
console_unlock+0x1e8/0x4b4
[ 61.771080] stack backtrace:
[ 61.771093] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.15.0-rc2+ #2583
[ 61.771130] Hardware name: MStar/Sigmastar Armv7 (Device Tree)
[ 61.771156] [<c010daf0>] (unwind_backtrace) from [<c0109f54>]
(show_stack+0x10/0x14)
[ 61.771235] [<c0109f54>] (show_stack) from [<c09303a0>]
(dump_stack_lvl+0x58/0x70)
[ 61.771312] [<c09303a0>] (dump_stack_lvl) from [<c016fbd8>]
(__lock_acquire+0x384/0x16a0)
[ 61.771394] [<c016fbd8>] (__lock_acquire) from [<c017191c>]
(lock_acquire+0x2a0/0x320)
[ 61.771470] [<c017191c>] (lock_acquire) from [<c093de84>]
(_raw_spin_lock_irqsave+0x5c/0x70)
[ 61.771542] [<c093de84>] (_raw_spin_lock_irqsave) from [<c04cdf98>]
(serial8250_console_write+0x1b0/0x254)
[ 61.771620] [<c04cdf98>] (serial8250_console_write) from
[<c0178068>] (console_unlock+0x3fc/0x4b4)
[ 61.771699] [<c0178068>] (console_unlock) from [<c0179750>]
(vprintk_emit+0x1d0/0x200)
[ 61.771769] [<c0179750>] (vprintk_emit) from [<c017979c>]
(vprintk_default+0x1c/0x24)
[ 61.771840] [<c017979c>] (vprintk_default) from [<c092d178>]
(_printk+0x18/0x28)
[ 61.771914] [<c092d178>] (_printk) from [<c017b9d0>]
(handle_bad_irq+0x44/0x22c)
[ 61.771991] [<c017b9d0>] (handle_bad_irq) from [<c017ade4>]
(handle_irq_desc+0x24/0x34)
[ 61.772066] [<c017ade4>] (handle_irq_desc) from [<c0467274>]
(ssd20xd_gpi_chainedhandler+0xb4/0xc4)
[ 61.772147] [<c0467274>] (ssd20xd_gpi_chainedhandler) from
[<c017ade4>] (handle_irq_desc+0x24/0x34)
[ 61.772223] [<c017ade4>] (handle_irq_desc) from [<c017b544>]
(handle_domain_irq+0x40/0x54)
[ 61.772299] [<c017b544>] (handle_domain_irq) from [<c0465f20>]
(gic_handle_irq+0x60/0x6c)
[ 61.772372] [<c0465f20>] (gic_handle_irq) from [<c0100aac>]
(__irq_svc+0x4c/0x64)
Somehow, the handler for this interrupt is set to handle_bad_irq(),
which probably isn't what you want. You'll have to find out who sets
this (there is a comment about that in gpiolib.c, but I haven't had a
chance to find where this is coming from).
Do you happen to set it in your driver?
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Daniel Palmer <hidden> Date: 2021-09-30 13:59:39
Hi Marc,
On Thu, 30 Sept 2021 at 22:53, Marc Zyngier [off-list ref] wrote:
Somehow, the handler for this interrupt is set to handle_bad_irq(),
which probably isn't what you want. You'll have to find out who sets
this (there is a comment about that in gpiolib.c, but I haven't had a
chance to find where this is coming from).
Do you happen to set it in your driver?
The gpio driver (gpio-msc313.c) sets it during probe:
gpioirqchip = &gpiochip->irq;
gpioirqchip->chip = &msc313_gpio_irqchip;
gpioirqchip->fwnode = of_node_to_fwnode(dev->of_node);
gpioirqchip->parent_domain = parent_domain;
gpioirqchip->child_to_parent_hwirq = match_data->child_to_parent_hwirq;
gpioirqchip->populate_parent_alloc_arg = match_data->populate_parent_fwspec;
gpioirqchip->handler = handle_bad_irq;
gpioirqchip->default_type = IRQ_TYPE_NONE;
Cheers,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-30 14:11:38
On Thu, 30 Sep 2021 14:59:24 +0100,
Daniel Palmer [off-list ref] wrote:
Hi Marc,
On Thu, 30 Sept 2021 at 22:53, Marc Zyngier [off-list ref] wrote:
quoted
Somehow, the handler for this interrupt is set to handle_bad_irq(),
which probably isn't what you want. You'll have to find out who sets
this (there is a comment about that in gpiolib.c, but I haven't had a
chance to find where this is coming from).
Do you happen to set it in your driver?
Right. I have no idea why this is a requirement, and I would normally
set it to whatever is the normal flow handler on this HW, but this
looks like the GPIO subsystem has some expectations here.
I'll let Linus comment on it.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Right. I have no idea why this is a requirement, and I would normally
set it to whatever is the normal flow handler on this HW, but this
looks like the GPIO subsystem has some expectations here.
I'll let Linus comment on it.
The handle_bad_irq() as default handler is because many GPIO
IRQ controllers in difference from on-chip IRQ controllers support
two levels and two edges of triggers, and there is nothing "normal"
(it is "general purpose" after all) so we need to defer the selection
of a suitable flow handler to .set_type().
With device tree the trigger is often specified in the second cell in the
DTS, so .set_type() will be called for any consumer as part of
the request_irq() and the appropriate handler is set from .set_type().
In my mind this is following the DT (ACPI) usage model of allocating
and initializing resources dynamically as they are requested.
I don't know if this reasoning is wrong in some way, what should we
do otherwise? (Confused...)
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Sep 30, 2021 at 3:37 PM Daniel Palmer [off-list ref] wrote:
On Thu, 30 Sept 2021 at 22:06, Marc Zyngier [off-list ref] wrote:
quoted
No, this is the right spot if you really need to set the handler. But
it should really be after the parent allocation (see below for
something totally untested).
Your change resolves the null pointer difference when enabling the
interrupt but when it triggers the below happens.
This might just be my driver so I'll try to debug.
To me this looks like your IRQ handler is firing for unused IRQs, i.e.
you are getting spurious IRQs.
Are you missing to disable all IRQs as part of the set-up before
registering the GPIO chip? (Usually some registers need to
be written with zeroes.)
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Daniel Palmer <hidden> Date: 2021-10-01 12:31:10
Hi Linus,
On Fri, 1 Oct 2021 at 01:13, Linus Walleij [off-list ref] wrote:
On Thu, Sep 30, 2021 at 3:37 PM Daniel Palmer [off-list ref] wrote:
To me this looks like your IRQ handler is firing for unused IRQs, i.e.
you are getting spurious IRQs.
Are you missing to disable all IRQs as part of the set-up before
registering the GPIO chip? (Usually some registers need to
be written with zeroes.)
I don't think it's something like the wrong IRQ firing as the same
driver was previously working with EOI to clear the interrupt instead
of ACK.
I'll double check though.
Cheers,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Daniel Palmer <hidden> Date: 2021-10-02 03:08:38
Hi Linus,
Sorry for the constant spam on this..
On Fri, 1 Oct 2021 at 01:13, Linus Walleij [off-list ref] wrote:
To me this looks like your IRQ handler is firing for unused IRQs, i.e.
you are getting spurious IRQs.
Are you missing to disable all IRQs as part of the set-up before
registering the GPIO chip? (Usually some registers need to
be written with zeroes.)
Changing the handler to handle_edge_irq() on the gpio side resolves
the issue and gpiomon registers edges from the gpio like it should.
So I think it's an ordering thing. Something like the gpio side sets
the handler to handle_bad_irq() after the irqchip side sets it to
handle_edge_irq().
Thanks for the help.
Cheers,
Daniel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel