| Message ID | 20250321134633.2155141-4-cjd@cjdns.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add EcoNet EN751221 MIPS platform support | expand |
Caleb! On Fri, Mar 21 2025 at 13:46, Caleb James DeLisle wrote: > --- > If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this > device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU > is not prepared to handle. If anybody knows how to either disable this > behavior, or handle vectored interrupts without ugly code that breaks > cascading, please let me know and I will implement that and add > MIPS_MT_SMP in a future patchset. This must be addressed before this driver can be merged, but that's a topic for the MIPS wizards and out of my area of expertise, except for the obvious: For a start you can exclude this platform from being enabled in Kconfig when the EI/VI muck is enabled. That's what 'depends on' is for, So this patch clearly should have been tagged with 'RFC'. > +static const struct econet_intc { > + const struct irq_chip chip; > + > + const struct irq_domain_ops domain_ops; > +} econet_intc; Please see https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers Aside of the coding style issues, what's the actual value of this struct? Is there anything which can't be done with: static const struct irq_chip econet_chip = { .name = .... }; static const struct irq_domain_ops econet_domain_ops = { .xlate = .... }; Which avoids the above forward struct declaration completely and does not need any forward declaration at all, neither for the chip nor for the domain. > +static struct { > + void __iomem *membase; > + u8 shadow_interrupts[INTC_IRQ_COUNT]; > +} econet_intc_rai __ro_after_init; > + > +static DEFINE_RAW_SPINLOCK(irq_lock); > + > +static void econet_wreg(u32 reg, u32 val, u32 mask) > +{ > + unsigned long flags; > + u32 v; > + > + raw_spin_lock_irqsave(&irq_lock, flags); Please use guard(raw_spinlock)(&irq_lock); You don't need irqsave when invoked from mask/unmask as the caller guarantees to have interrupts disabled. Then you only need to disable interrupts across the invocation from mask_all(). > + > + v = ioread32(econet_intc_rai.membase + reg); > + v &= ~mask; > + v |= val & mask; > + iowrite32(v, econet_intc_rai.membase + reg); > + > + raw_spin_unlock_irqrestore(&irq_lock, flags); > +} > + > +static void econet_chmask(u32 hwirq, bool unmask) > +{ > + u32 reg; > + u32 mask; > + u32 bit; > + u8 shadow; Search the same document for local variables. > + shadow = econet_intc_rai.shadow_interrupts[hwirq]; > + if (WARN_ON_ONCE(shadow == INTC_IS_SHADOW)) > + return; > + else if (shadow < INTC_NO_SHADOW && smp_processor_id() > 0) > + hwirq = shadow; This is completely undocumented voodoo. Please add comments which explain this properly. > + if (hwirq >= 32) { > + reg = REG_MASK1; > + mask = BIT(hwirq - 32); > + } else { > + reg = REG_MASK0; > + mask = BIT(hwirq); > + } > + bit = (unmask) ? mask : 0; > + econet_wreg(reg, bit, mask); econet_wreg(reg, unmask ? mask : 0, mask); > +} > + > +static void econet_intc_mask(struct irq_data *d) > +{ > + econet_chmask(d->hwirq, false); > +} > + > +static void econet_intc_unmask(struct irq_data *d) > +{ > + econet_chmask(d->hwirq, true); > +} > + > +static void econet_mask_all(void) > +{ with a guard(irq)(); added here you spare the irqsave in the write function. > +static void econet_intc_from_parent(struct irq_desc *desc) > +{ > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct irq_domain *domain; > + u32 pending0; > + u32 pending1; > + > + chained_irq_enter(chip, desc); > + > + pending0 = ioread32(econet_intc_rai.membase + REG_PENDING0); > + pending1 = ioread32(econet_intc_rai.membase + REG_PENDING1); > + > + if (unlikely(!(pending0 | pending1))) { > + spurious_interrupt(); > + goto out; > + } > + > + domain = irq_desc_get_handler_data(desc); > + > + econet_intc_handle_pending(domain, pending0, 0); > + econet_intc_handle_pending(domain, pending1, 32); if (likely(pending0 | pending1) { domain = ... ... } else { spurious_interrupt(); } Makes the goto go away _and_ sets the focus on the likely path and not on the visual clutter of the unlikely one. > +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq) > +{ > + int ret; > + > + if (hwirq >= INTC_IRQ_COUNT) { > + pr_err("%s: hwirq %lu out of range\n", __func__, hwirq); > + return -EINVAL; > + } else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) { > + pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n", > + __func__, hwirq); No newline > + return -EINVAL; > + } Please put a newline here for readability instead. > + if (econet_intc_rai.shadow_interrupts[hwirq] != INTC_NO_SHADOW) { This INTC_IS_SHADOW and INTC_NO_SHADOW logic is beyond confusing without comments. Three month down the road you will ask yourself what the hell this means. > + irq_set_chip_and_handler( > + irq, &econet_intc.chip, handle_percpu_devid_irq); This line break is unreadable. See documentation. If at all this wants to be: irq_set_chip_and_handler(irq, &econet_intc.chip, handle_percpu_devid_irq); But this fits nicely within 100 characters, so get rid of it completely. > + ret = irq_set_percpu_devid(irq); And please add a comment which explains why this magic shadow thing maps to percpu devid interrupts. > + if (ret) { > + pr_warn("%s: Failed irq_set_percpu_devid for %u: %d\n", > + d->name, irq, ret); > + } > + } else { > + irq_set_chip_and_handler( > + irq, &econet_intc.chip, handle_level_irq); Same here. > + } > + irq_set_chip_data(irq, NULL); > + return 0; > +} > + > +static const struct econet_intc econet_intc = { > + .chip = { > + .name = "en751221-intc", > + .irq_unmask = econet_intc_unmask, > + .irq_mask = econet_intc_mask, > + .irq_mask_ack = econet_intc_mask, > + }, > + .domain_ops = { > + .xlate = irq_domain_xlate_onecell, > + .map = econet_intc_map, See documention. > + }, > +}; > + > +static int __init get_shadow_interrupts(struct device_node *node) > +{ > + const char *field = "econet,shadow-interrupts"; > + int n_shadow_interrupts; > + u32 *shadow_interrupts; > + > + n_shadow_interrupts = of_property_count_u32_elems(node, field); > + memset(econet_intc_rai.shadow_interrupts, INTC_NO_SHADOW, > + sizeof(econet_intc_rai.shadow_interrupts)); > + if (n_shadow_interrupts <= 0) { > + return 0; > + } else if (n_shadow_interrupts % 2) { > + pr_err("%pOF: %s count is odd, ignoring\n", node, field); > + return 0; > + } > + shadow_interrupts = kmalloc_array(n_shadow_interrupts, sizeof(u32), > + GFP_KERNEL); u32 *shadow_interrupts __free(kfree) = kmalloc_array(n_shadow_interrupts, sizeof(u32), GFP_KERNEL); Then the return paths don't have to care about this allocation at all. > + if (!shadow_interrupts) > + return -ENOMEM; > + if (of_property_read_u32_array(node, field, > + shadow_interrupts, n_shadow_interrupts) > + ) { Your random choices of coding style and lack of visual seperation by empty newlines really make this hard to digest. > + pr_err("%pOF: Failed to read %s\n", node, field); > + kfree(shadow_interrupts); > + return -EINVAL; > + } The __free() above will reduce this to if (of_property_read_u32_array(node, field, shadow_interrupts, n_shadow_interrupts)) { pr_err("%pOF: Failed to read %s\n", node, field); return -EINVAL; } and removes the kfree() at the end of the function. > + for (int i = 0; i < n_shadow_interrupts; i += 2) { > + u32 shadow = shadow_interrupts[i + 1]; > + u32 target = shadow_interrupts[i]; > + > + if (shadow > INTC_IRQ_COUNT) { > + pr_err("%pOF: %s[%d] shadow(%d) out of range\n", > + node, field, i, shadow); No line break. > + continue; > + } Newline > + if (target >= INTC_IRQ_COUNT) { > + pr_err("%pOF: %s[%d] target(%d) out of range\n", > + node, field, i + 1, target); No line break. > + continue; > + } > + econet_intc_rai.shadow_interrupts[target] = shadow; > + econet_intc_rai.shadow_interrupts[shadow] = INTC_IS_SHADOW; What the heck does any of this mean? This whole shadow magic is hideously incomprehensible. It's amazing that this whole file does not contain a single line of comment. Aside of that how is any of this sanity checked, i.e. so that there are no existing entries overwritten? I assume this blindly relies on the device tree being correct. Fine, but then please document it. > + } > + kfree(shadow_interrupts); > + return 0; > +} > + > +static int __init econet_intc_of_init(struct device_node *node, struct device_node *parent) > +{ > + int ret; > + int irq; > + struct resource res; > + struct irq_domain *domain; Sigh. > + > + domain = irq_domain_add_linear( > + node, INTC_IRQ_COUNT, > + &econet_intc.domain_ops, NULL); Finally my eyes bleed and my mental code pattern matching engine threw a garbage-overload exception. Seriously. Consistent coding style _and_ comments explaining the non-obvious parts of the code are not optional. You want me and others to review your code, so please have the courtesy to provide it in a digestable form. That spares us to point out the obvious, which can be looked up in documentation, and the frustration of staring at incomprehensible undocumented logic. And it spares you the frustration of getting your submission ripped into bits and pieces. Thanks, tglx
Thank you for this review. On 21/03/2025 21:26, Thomas Gleixner wrote: > Caleb! > > On Fri, Mar 21 2025 at 13:46, Caleb James DeLisle wrote: >> --- >> If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this >> device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU >> is not prepared to handle. If anybody knows how to either disable this >> behavior, or handle vectored interrupts without ugly code that breaks >> cascading, please let me know and I will implement that and add >> MIPS_MT_SMP in a future patchset. > This must be addressed before this driver can be merged, but that's a > topic for the MIPS wizards and out of my area of expertise, except for > the obvious: > > For a start you can exclude this platform from being enabled in > Kconfig when the EI/VI muck is enabled. That's what 'depends on' is > for, Maybe my message was misleading everything has been tested and works correctly on multiple SoCs because ECONET_SOC_EN751221 does not select EI/VI. Answering this question will allow me to enable them, thus also getting MIPS_MT_SMP. I could look at forbidding them in the driver, but I'm not sure that's appropriate as this seems like more of an SoC issue than an INTC issue. But I'll follow your guidance. > > So this patch clearly should have been tagged with 'RFC'. Given the patchset works correctly in testing, does this comment stand? > >> +static const struct econet_intc { >> + const struct irq_chip chip; >> + >> + const struct irq_domain_ops domain_ops; >> +} econet_intc; > Please see > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers Thank you, I've been reading a lot of documentation but did not find that one. > > Aside of the coding style issues, what's the actual value of this > struct? Is there anything which can't be done with: > > static const struct irq_chip econet_chip = { > .name = .... > }; > > static const struct irq_domain_ops econet_domain_ops = { > .xlate = .... > }; > > Which avoids the above forward struct declaration completely and does > not need any forward declaration at all, neither for the chip nor for > the domain. My normal instinct lead me to minimize symbols on the top level scope and I didn't realize I was doing it and should check what is typical in kernel code. Will refactor. > >> +static struct { >> + void __iomem *membase; >> + u8 shadow_interrupts[INTC_IRQ_COUNT]; >> +} econet_intc_rai __ro_after_init; >> + >> +static DEFINE_RAW_SPINLOCK(irq_lock); >> + >> +static void econet_wreg(u32 reg, u32 val, u32 mask) >> +{ >> + unsigned long flags; >> + u32 v; >> + >> + raw_spin_lock_irqsave(&irq_lock, flags); > Please use > > guard(raw_spinlock)(&irq_lock); > > You don't need irqsave when invoked from mask/unmask as the caller > guarantees to have interrupts disabled. Then you only need to disable > interrupts across the invocation from mask_all(). Thank you very much, I would not have thought of this. > >> + >> + v = ioread32(econet_intc_rai.membase + reg); >> + v &= ~mask; >> + v |= val & mask; >> + iowrite32(v, econet_intc_rai.membase + reg); >> + >> + raw_spin_unlock_irqrestore(&irq_lock, flags); >> +} >> + >> +static void econet_chmask(u32 hwirq, bool unmask) >> +{ >> + u32 reg; >> + u32 mask; >> + u32 bit; >> + u8 shadow; > Search the same document for local variables. Ok > >> + shadow = econet_intc_rai.shadow_interrupts[hwirq]; >> + if (WARN_ON_ONCE(shadow == INTC_IS_SHADOW)) >> + return; >> + else if (shadow < INTC_NO_SHADOW && smp_processor_id() > 0) >> + hwirq = shadow; > This is completely undocumented voodoo. Please add comments which > explain this properly. Sure thing, I often write and then remove comments because I don't want to be judged for over-explaining. In fact, I love explaining. > >> + if (hwirq >= 32) { >> + reg = REG_MASK1; >> + mask = BIT(hwirq - 32); >> + } else { >> + reg = REG_MASK0; >> + mask = BIT(hwirq); >> + } >> + bit = (unmask) ? mask : 0; >> + econet_wreg(reg, bit, mask); > econet_wreg(reg, unmask ? mask : 0, mask); Ok > >> +} >> + >> +static void econet_intc_mask(struct irq_data *d) >> +{ >> + econet_chmask(d->hwirq, false); >> +} >> + >> +static void econet_intc_unmask(struct irq_data *d) >> +{ >> + econet_chmask(d->hwirq, true); >> +} >> + >> +static void econet_mask_all(void) >> +{ > with a > > guard(irq)(); > > added here you spare the irqsave in the write function. Ok, thanks. > >> +static void econet_intc_from_parent(struct irq_desc *desc) >> +{ >> + struct irq_chip *chip = irq_desc_get_chip(desc); >> + struct irq_domain *domain; >> + u32 pending0; >> + u32 pending1; >> + >> + chained_irq_enter(chip, desc); >> + >> + pending0 = ioread32(econet_intc_rai.membase + REG_PENDING0); >> + pending1 = ioread32(econet_intc_rai.membase + REG_PENDING1); >> + >> + if (unlikely(!(pending0 | pending1))) { >> + spurious_interrupt(); >> + goto out; >> + } >> + >> + domain = irq_desc_get_handler_data(desc); >> + >> + econet_intc_handle_pending(domain, pending0, 0); >> + econet_intc_handle_pending(domain, pending1, 32); > if (likely(pending0 | pending1) { > domain = ... > ... > } else { > spurious_interrupt(); > } > > Makes the goto go away _and_ sets the focus on the likely path and not > on the visual clutter of the unlikely one. Indeed, will fix. > >> +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq) >> +{ >> + int ret; >> + >> + if (hwirq >= INTC_IRQ_COUNT) { >> + pr_err("%s: hwirq %lu out of range\n", __func__, hwirq); >> + return -EINVAL; >> + } else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) { >> + pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n", >> + __func__, hwirq); > No newline If I understand correctly, you prefer: .....interrupt\n", __func__, hwirq); for a 96 char line? > >> + return -EINVAL; >> + } > Please put a newline here for readability instead. Sure thing > >> + if (econet_intc_rai.shadow_interrupts[hwirq] != INTC_NO_SHADOW) { > This INTC_IS_SHADOW and INTC_NO_SHADOW logic is beyond confusing without > comments. Three month down the road you will ask yourself what the hell > this means. I'll add comments and also try to make the code a little more self-evident in v2. > >> + irq_set_chip_and_handler( >> + irq, &econet_intc.chip, handle_percpu_devid_irq); > This line break is unreadable. See documentation. > > If at all this wants to be: > > irq_set_chip_and_handler(irq, &econet_intc.chip, > handle_percpu_devid_irq); > > But this fits nicely within 100 characters, so get rid of it completely. My apologies. It was my intention, but instincts are sneaky and for some reason checkpatch didn't catch it. > >> + ret = irq_set_percpu_devid(irq); > And please add a comment which explains why this magic shadow thing maps > to percpu devid interrupts. Sure thing > >> + if (ret) { >> + pr_warn("%s: Failed irq_set_percpu_devid for %u: %d\n", >> + d->name, irq, ret); >> + } >> + } else { >> + irq_set_chip_and_handler( >> + irq, &econet_intc.chip, handle_level_irq); > Same here. Ok. > >> + } >> + irq_set_chip_data(irq, NULL); >> + return 0; >> +} >> + >> +static const struct econet_intc econet_intc = { >> + .chip = { >> + .name = "en751221-intc", >> + .irq_unmask = econet_intc_unmask, >> + .irq_mask = econet_intc_mask, >> + .irq_mask_ack = econet_intc_mask, >> + }, >> + .domain_ops = { >> + .xlate = irq_domain_xlate_onecell, >> + .map = econet_intc_map, > See documention. I suppose this is tab alignment, but I will in any case make a point of reading it all carefully. > >> + }, >> +}; >> + >> +static int __init get_shadow_interrupts(struct device_node *node) >> +{ >> + const char *field = "econet,shadow-interrupts"; >> + int n_shadow_interrupts; >> + u32 *shadow_interrupts; >> + >> + n_shadow_interrupts = of_property_count_u32_elems(node, field); >> + memset(econet_intc_rai.shadow_interrupts, INTC_NO_SHADOW, >> + sizeof(econet_intc_rai.shadow_interrupts)); >> + if (n_shadow_interrupts <= 0) { >> + return 0; >> + } else if (n_shadow_interrupts % 2) { >> + pr_err("%pOF: %s count is odd, ignoring\n", node, field); >> + return 0; >> + } >> + shadow_interrupts = kmalloc_array(n_shadow_interrupts, sizeof(u32), >> + GFP_KERNEL); > u32 *shadow_interrupts __free(kfree) = > kmalloc_array(n_shadow_interrupts, sizeof(u32), GFP_KERNEL); > > Then the return paths don't have to care about this allocation at all. Very nice, thank you. > >> + if (!shadow_interrupts) >> + return -ENOMEM; >> + if (of_property_read_u32_array(node, field, >> + shadow_interrupts, n_shadow_interrupts) >> + ) { > Your random choices of coding style and lack of visual seperation by > empty newlines really make this hard to digest. Apologies, will restructure. > >> + pr_err("%pOF: Failed to read %s\n", node, field); >> + kfree(shadow_interrupts); >> + return -EINVAL; >> + } > The __free() above will reduce this to > > if (of_property_read_u32_array(node, field, shadow_interrupts, n_shadow_interrupts)) { > pr_err("%pOF: Failed to read %s\n", node, field); > return -EINVAL; > } > > and removes the kfree() at the end of the function. Yes, I wasn't aware of __free() until now. Generally I love this type of thing and use them wherever possible. > >> + for (int i = 0; i < n_shadow_interrupts; i += 2) { >> + u32 shadow = shadow_interrupts[i + 1]; >> + u32 target = shadow_interrupts[i]; >> + >> + if (shadow > INTC_IRQ_COUNT) { >> + pr_err("%pOF: %s[%d] shadow(%d) out of range\n", >> + node, field, i, shadow); > No line break. Ok > >> + continue; >> + } > Newline Ok (I will check for tightly packed if statements and fix all) > >> + if (target >= INTC_IRQ_COUNT) { >> + pr_err("%pOF: %s[%d] target(%d) out of range\n", >> + node, field, i + 1, target); > No line break. Ok > >> + continue; >> + } >> + econet_intc_rai.shadow_interrupts[target] = shadow; >> + econet_intc_rai.shadow_interrupts[shadow] = INTC_IS_SHADOW; > What the heck does any of this mean? This whole shadow magic is > hideously incomprehensible. It's amazing that this whole file does not > contain a single line of comment. > > Aside of that how is any of this sanity checked, i.e. so that there are > no existing entries overwritten? I assume this blindly relies on the > device tree being correct. Fine, but then please document it. I had a nice comment on the oddities of these "shadow interrupts" which I then moved into the DT binding and removed from the source. I'll happily add it back and more. As I said, I actually like explaining, I perhaps erroneously tried to match the terseness of other kernel drivers. > >> + } >> + kfree(shadow_interrupts); >> + return 0; >> +} >> + >> +static int __init econet_intc_of_init(struct device_node *node, struct device_node *parent) >> +{ >> + int ret; >> + int irq; >> + struct resource res; >> + struct irq_domain *domain; > Sigh. Ok I see, reverse fir tree order. > >> + >> + domain = irq_domain_add_linear( >> + node, INTC_IRQ_COUNT, >> + &econet_intc.domain_ops, NULL); > Finally my eyes bleed and my mental code pattern matching engine threw a > garbage-overload exception. > > Seriously. Consistent coding style _and_ comments explaining the > non-obvious parts of the code are not optional. > > You want me and others to review your code, so please have the courtesy > to provide it in a digestable form. > > That spares us to point out the obvious, which can be looked up in > documentation, and the frustration of staring at incomprehensible > undocumented logic. And it spares you the frustration of getting your > submission ripped into bits and pieces. In case of any doubt, I wasn't trying to sneak bad code past you. I read a lot of documentation, though clearly not enough / the right stuff. When I sent this, I didn't know what was left that could be improved. I'm going to read the documentation you linked, then re-read all of this patchset with your comments in mind, and hopefully come back with something much better. Lastly, I very much appreciate your taking the time. Thanks, Caleb > > Thanks, > > tglx > >
On Fri, Mar 21 2025 at 23:20, Caleb James DeLisle wrote: > On 21/03/2025 21:26, Thomas Gleixner wrote: >> Caleb! >> >> On Fri, Mar 21 2025 at 13:46, Caleb James DeLisle wrote: >>> --- >>> If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this >>> device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU >>> is not prepared to handle. If anybody knows how to either disable this >>> behavior, or handle vectored interrupts without ugly code that breaks >>> cascading, please let me know and I will implement that and add >>> MIPS_MT_SMP in a future patchset. >> This must be addressed before this driver can be merged, but that's a >> topic for the MIPS wizards and out of my area of expertise, except for >> the obvious: >> >> For a start you can exclude this platform from being enabled in >> Kconfig when the EI/VI muck is enabled. That's what 'depends on' is >> for, > > Maybe my message was misleading everything has been tested and works correctly > on multiple SoCs because ECONET_SOC_EN751221 does not select EI/VI. Answering > this question will allow me to enable them, thus also getting > MIPS_MT_SMP. It does not select it, but it can be enabled independently or through some other magic config knob, right? And if it gets enabled, then it does not work, right? > I could look at forbidding them in the driver, but I'm not sure that's > appropriate as this seems like more of an SoC issue than an INTC > issue. But I'll follow your guidance. What's not appropriate? If it does not work, then it's very appropriate to do config ECONET depends on !EI && !VI on the principle of least surprise, no? >> So this patch clearly should have been tagged with 'RFC'. > > Given the patchset works correctly in testing, does this comment > stand? Until the EI/VI issue is resolved so that it either works or cannot happen. >>> +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq) >>> +{ >>> + int ret; >>> + >>> + if (hwirq >= INTC_IRQ_COUNT) { >>> + pr_err("%s: hwirq %lu out of range\n", __func__, hwirq); >>> + return -EINVAL; >>> + } else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) { >>> + pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n", >>> + __func__, hwirq); >> No newline > If I understand correctly, you prefer: > .....interrupt\n", __func__, hwirq); > for a 96 char line? You have 100 characters in drivers/irqchip/ >>> + .domain_ops = { >>> + .xlate = irq_domain_xlate_onecell, >>> + .map = econet_intc_map, >> See documention. > I suppose this is tab alignment, but I will in any case make a point > of reading it all carefully. Yes. The aligned tabular view is way simpler to read and parse. Reading is based on pattern recognition. Irregular patterns disturb the reading flow, which means the focus is shifted from understanding to decoding the irregular pattern. > In case of any doubt, I wasn't trying to sneak bad code past you. I did not assume malice here at all. Thanks, tglx
[snip] >> Maybe my message was misleading everything has been tested and works correctly >> on multiple SoCs because ECONET_SOC_EN751221 does not select EI/VI. Answering >> this question will allow me to enable them, thus also getting >> MIPS_MT_SMP. > It does not select it, but it can be enabled independently or through > some other magic config knob, right? And if it gets enabled, then it > does not work, right? Not really true on either point. Firstly, it's not something you can select in the kernel menuconfig, it's selected by the SoC or some core feature of the SoC (e.g. SMP_MT_MIPS). Secondly it does work, it's just that it does what it says it does, and you need to handle those vectored interrupts - no irqchip driver does this. >> I could look at forbidding them in the driver, but I'm not sure that's >> appropriate as this seems like more of an SoC issue than an INTC >> issue. But I'll follow your guidance. > What's not appropriate? If it does not work, then it's very appropriate > to do > > config ECONET > depends on !EI && !VI > > on the principle of least surprise, no? I've spent quite a bit of time studying this, so with respect for your time, let me try to give you a brief summary of what I know and why I submitted this as I did: 1. EI/VI is supported by the intc but it's really a feature of MIPS32r2. In MIPS32r2, the CPU<->intc wire interface allows the CPU to send its interrupts to the intc, and then allows the intc to fire any of of up to 64 interrupts back to the CPU. 2. When enabled, the CPU's internal intc sends its 7 interrupts to the external intc who prioritizes them, renumbers them, and sends them back along with their own. 3. When they come back, the CPU tries to be helpful by dispatching to an offset within a vector table depending on the interrupt number. 4. The real problem with this is IRQ_MIPS_CPU no longer gets its 7 interrupts because they've been renumbered. 5. MIPS 1004Kc uses a standard intc (IRQ_GIC), and they solved this by not really using the feature. Despite having 64 lines, they only send on one and they make the driver poll to find out what's pending. I believe they also return the CPU's 7 interrupts without renumbering. 6. But in 34Kc world there is no standard intc, and AFAICT many (most?) of them fully use the EI/VI feature. 7. If you don't set EI/VI, the processor goes into / stays in legacy mode, so it doesn't send anything to the intc, and everything the intc sends to it is converted to a hit on line 2 - so as long as the intc has some kind of pending register, chaining works. 8. But without EI/VI you can't have MT_SMP so you only get one thread. 9. In every 34Kc SoC I've found in Linux or OpenWRT, EI/VI is conspicuously missing (with one exception). Clearly they had a compelling reason for doing it, and I *think* that reason is because they all faced the same issue as me and solved it the same way. 10. The exception is irq-realtek-rtl, which via an out-of-tree patch[1] was able to enable EI/VI and I have no idea what they're doing, but it appears that their intc hardware is participating, like with GIC. 11. I did implement an EI/VI dispatcher myself and had it working with SMP, but I shelved because it's complex and it's not tightly coupled to the intc driver itself so I concluded that it should be a separate component that works with any intc. The complexity comes from the fact that you need to either route the software interrupts back to IRQ_MIPS_CPU's domain and fix the renumbering, or else implement your own IPI subsystem. So it's my belief that what I'm doing here is standard for 34Kc. The reason I asked the question in the beginning was because I wanted to check my assumptions and know if there's any way I can get SMP without writing this dispatcher. >>> So this patch clearly should have been tagged with 'RFC'. >> Given the patchset works correctly in testing, does this comment >> stand? > Until the EI/VI issue is resolved so that it either works or cannot > happen. All said, if "depends on !EI && !VI" makes you happy then I'm OK to add it. Just what I'm afraid of is being asked to find an authoritative answer to my question before merging, because if nobody decides to jump in with one then this could just be blocked indefinitely. Thanks, Caleb [1]: https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/patches-6.6/314-irqchip-irq-realtek-rtl-add-VPE-support.patch >>>> +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq) >>>> +{ >>>> + int ret; >>>> + >>>> + if (hwirq >= INTC_IRQ_COUNT) { >>>> + pr_err("%s: hwirq %lu out of range\n", __func__, hwirq); >>>> + return -EINVAL; >>>> + } else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) { >>>> + pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n", >>>> + __func__, hwirq); >>> No newline >> If I understand correctly, you prefer: >> .....interrupt\n", __func__, hwirq); >> for a 96 char line? > You have 100 characters in drivers/irqchip/ > >>>> + .domain_ops = { >>>> + .xlate = irq_domain_xlate_onecell, >>>> + .map = econet_intc_map, >>> See documention. >> I suppose this is tab alignment, but I will in any case make a point >> of reading it all carefully. > Yes. The aligned tabular view is way simpler to read and parse. Reading > is based on pattern recognition. Irregular patterns disturb the reading > flow, which means the focus is shifted from understanding to decoding > the irregular pattern. > >> In case of any doubt, I wasn't trying to sneak bad code past you. > I did not assume malice here at all. > > Thanks, > > tglx
On Sun, Mar 23 2025 at 04:06, Caleb James DeLisle wrote: > So it's my belief that what I'm doing here is standard for 34Kc. > > The reason I asked the question in the beginning was because I wanted > to check my assumptions and know if there's any way I can get SMP > without writing this dispatcher. Fair enough. If it just works as is then I don't have any objections and the question vs. SMP has to answered by the MIPS wizards. >>>> So this patch clearly should have been tagged with 'RFC'. >>> Given the patchset works correctly in testing, does this comment >>> stand? >> Until the EI/VI issue is resolved so that it either works or cannot >> happen. > > All said, if "depends on !EI && !VI" makes you happy then I'm OK to add it. It's not about making me happy. I just want to avoid a situation where this causes hard to diagnose issues. > Just what I'm afraid of is being asked to find an authoritative answer to my > question before merging, because if nobody decides to jump in with one > then this could just be blocked indefinitely. Nah. If it works the way you implemented it and you can arguably exclude EI/VI interaction, then there is no reason to delay anything. Thanks, tglx
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index c11b9965c4ad..a591ad3156dc 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -147,6 +147,11 @@ config DW_APB_ICTL select GENERIC_IRQ_CHIP select IRQ_DOMAIN_HIERARCHY +config ECONET_EN751221_INTC + bool + select GENERIC_IRQ_CHIP + select IRQ_DOMAIN + config FARADAY_FTINTC010 bool select IRQ_DOMAIN diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 25e9ad29b8c4..1ee83823928d 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o obj-$(CONFIG_DAVINCI_CP_INTC) += irq-davinci-cp-intc.o obj-$(CONFIG_EXYNOS_IRQ_COMBINER) += exynos-combiner.o +obj-$(CONFIG_ECONET_EN751221_INTC) += irq-econet-en751221.o obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o diff --git a/drivers/irqchip/irq-econet-en751221.c b/drivers/irqchip/irq-econet-en751221.c new file mode 100644 index 000000000000..edbb8a3d6d51 --- /dev/null +++ b/drivers/irqchip/irq-econet-en751221.c @@ -0,0 +1,280 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * EN751221 Interrupt Controller Driver. + * + * Copyright (C) 2025 Caleb James DeLisle <cjd@cjdns.fr> + */ + +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/irqdomain.h> +#include <linux/irqchip.h> +#include <linux/irqchip/chained_irq.h> + +#define INTC_IRQ_COUNT 40 + +#define INTC_NO_SHADOW 0xff +#define INTC_IS_SHADOW 0xfe + +#define REG_MASK0 0x04 +#define REG_MASK1 0x50 +#define REG_PENDING0 0x08 +#define REG_PENDING1 0x54 + +static const struct econet_intc { + const struct irq_chip chip; + + const struct irq_domain_ops domain_ops; +} econet_intc; + +static struct { + void __iomem *membase; + u8 shadow_interrupts[INTC_IRQ_COUNT]; +} econet_intc_rai __ro_after_init; + +static DEFINE_RAW_SPINLOCK(irq_lock); + +static void econet_wreg(u32 reg, u32 val, u32 mask) +{ + unsigned long flags; + u32 v; + + raw_spin_lock_irqsave(&irq_lock, flags); + + v = ioread32(econet_intc_rai.membase + reg); + v &= ~mask; + v |= val & mask; + iowrite32(v, econet_intc_rai.membase + reg); + + raw_spin_unlock_irqrestore(&irq_lock, flags); +} + +static void econet_chmask(u32 hwirq, bool unmask) +{ + u32 reg; + u32 mask; + u32 bit; + u8 shadow; + + shadow = econet_intc_rai.shadow_interrupts[hwirq]; + if (WARN_ON_ONCE(shadow == INTC_IS_SHADOW)) + return; + else if (shadow < INTC_NO_SHADOW && smp_processor_id() > 0) + hwirq = shadow; + + if (hwirq >= 32) { + reg = REG_MASK1; + mask = BIT(hwirq - 32); + } else { + reg = REG_MASK0; + mask = BIT(hwirq); + } + bit = (unmask) ? mask : 0; + + econet_wreg(reg, bit, mask); +} + +static void econet_intc_mask(struct irq_data *d) +{ + econet_chmask(d->hwirq, false); +} + +static void econet_intc_unmask(struct irq_data *d) +{ + econet_chmask(d->hwirq, true); +} + +static void econet_mask_all(void) +{ + econet_wreg(REG_MASK0, 0, ~0); + econet_wreg(REG_MASK1, 0, ~0); +} + +static void econet_intc_handle_pending(struct irq_domain *d, u32 pending, u32 offset) +{ + int hwirq; + + while (pending) { + hwirq = fls(pending) - 1; + generic_handle_domain_irq(d, hwirq + offset); + pending &= ~BIT(hwirq); + } +} + +static void econet_intc_from_parent(struct irq_desc *desc) +{ + struct irq_chip *chip = irq_desc_get_chip(desc); + struct irq_domain *domain; + u32 pending0; + u32 pending1; + + chained_irq_enter(chip, desc); + + pending0 = ioread32(econet_intc_rai.membase + REG_PENDING0); + pending1 = ioread32(econet_intc_rai.membase + REG_PENDING1); + + if (unlikely(!(pending0 | pending1))) { + spurious_interrupt(); + goto out; + } + + domain = irq_desc_get_handler_data(desc); + + econet_intc_handle_pending(domain, pending0, 0); + econet_intc_handle_pending(domain, pending1, 32); + +out: + chained_irq_exit(chip, desc); +} + +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq) +{ + int ret; + + if (hwirq >= INTC_IRQ_COUNT) { + pr_err("%s: hwirq %lu out of range\n", __func__, hwirq); + return -EINVAL; + } else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) { + pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n", + __func__, hwirq); + return -EINVAL; + } + if (econet_intc_rai.shadow_interrupts[hwirq] != INTC_NO_SHADOW) { + irq_set_chip_and_handler( + irq, &econet_intc.chip, handle_percpu_devid_irq); + ret = irq_set_percpu_devid(irq); + if (ret) { + pr_warn("%s: Failed irq_set_percpu_devid for %u: %d\n", + d->name, irq, ret); + } + } else { + irq_set_chip_and_handler( + irq, &econet_intc.chip, handle_level_irq); + } + irq_set_chip_data(irq, NULL); + return 0; +} + +static const struct econet_intc econet_intc = { + .chip = { + .name = "en751221-intc", + .irq_unmask = econet_intc_unmask, + .irq_mask = econet_intc_mask, + .irq_mask_ack = econet_intc_mask, + }, + .domain_ops = { + .xlate = irq_domain_xlate_onecell, + .map = econet_intc_map, + }, +}; + +static int __init get_shadow_interrupts(struct device_node *node) +{ + const char *field = "econet,shadow-interrupts"; + int n_shadow_interrupts; + u32 *shadow_interrupts; + + n_shadow_interrupts = of_property_count_u32_elems(node, field); + memset(econet_intc_rai.shadow_interrupts, INTC_NO_SHADOW, + sizeof(econet_intc_rai.shadow_interrupts)); + if (n_shadow_interrupts <= 0) { + return 0; + } else if (n_shadow_interrupts % 2) { + pr_err("%pOF: %s count is odd, ignoring\n", node, field); + return 0; + } + shadow_interrupts = kmalloc_array(n_shadow_interrupts, sizeof(u32), + GFP_KERNEL); + if (!shadow_interrupts) + return -ENOMEM; + if (of_property_read_u32_array(node, field, + shadow_interrupts, n_shadow_interrupts) + ) { + pr_err("%pOF: Failed to read %s\n", node, field); + kfree(shadow_interrupts); + return -EINVAL; + } + for (int i = 0; i < n_shadow_interrupts; i += 2) { + u32 shadow = shadow_interrupts[i + 1]; + u32 target = shadow_interrupts[i]; + + if (shadow > INTC_IRQ_COUNT) { + pr_err("%pOF: %s[%d] shadow(%d) out of range\n", + node, field, i, shadow); + continue; + } + if (target >= INTC_IRQ_COUNT) { + pr_err("%pOF: %s[%d] target(%d) out of range\n", + node, field, i + 1, target); + continue; + } + econet_intc_rai.shadow_interrupts[target] = shadow; + econet_intc_rai.shadow_interrupts[shadow] = INTC_IS_SHADOW; + } + kfree(shadow_interrupts); + return 0; +} + +static int __init econet_intc_of_init(struct device_node *node, struct device_node *parent) +{ + int ret; + int irq; + struct resource res; + struct irq_domain *domain; + + ret = get_shadow_interrupts(node); + if (ret) + return ret; + + irq = irq_of_parse_and_map(node, 0); + if (!irq) { + pr_err("%pOF: DT: Failed to get IRQ from 'interrupts'\n", node); + return -EINVAL; + } + + if (of_address_to_resource(node, 0, &res)) { + pr_err("%pOF: DT: Failed to get 'reg'\n", node); + ret = -EINVAL; + goto err_dispose_mapping; + } + + if (!request_mem_region(res.start, resource_size(&res), res.name)) { + pr_err("%pOF: Failed to request memory\n", node); + ret = -EBUSY; + goto err_dispose_mapping; + } + + econet_intc_rai.membase = ioremap(res.start, resource_size(&res)); + if (!econet_intc_rai.membase) { + pr_err("%pOF: Failed to remap membase\n", node); + ret = -ENOMEM; + goto err_release; + } + + econet_mask_all(); + + domain = irq_domain_add_linear( + node, INTC_IRQ_COUNT, + &econet_intc.domain_ops, NULL); + if (!domain) { + pr_err("%pOF: Failed to add irqdomain\n", node); + ret = -ENOMEM; + goto err_unmap; + } + + irq_set_chained_handler_and_data(irq, econet_intc_from_parent, domain); + + return 0; + +err_unmap: + iounmap(econet_intc_rai.membase); +err_release: + release_mem_region(res.start, resource_size(&res)); +err_dispose_mapping: + irq_dispose_mapping(irq); + return ret; +} + +IRQCHIP_DECLARE(econet_en751221_intc, "econet,en751221-intc", econet_intc_of_init);
Add a driver for the interrupt controller in the EcoNet EN751221 MIPS SoC. Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr> --- If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU is not prepared to handle. If anybody knows how to either disable this behavior, or handle vectored interrupts without ugly code that breaks cascading, please let me know and I will implement that and add MIPS_MT_SMP in a future patchset. --- drivers/irqchip/Kconfig | 5 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-econet-en751221.c | 280 ++++++++++++++++++++++++++ 3 files changed, 286 insertions(+) create mode 100644 drivers/irqchip/irq-econet-en751221.c