Messages in this thread | | | From | Anup Patel <> | Date | Sat, 19 Feb 2022 09:08:43 +0530 | Subject | Re: [PATCH v2 2/6] irqchip/riscv-intc: Create domain using named fwnode |
| |
On Thu, Feb 17, 2022 at 8:42 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2022-01-28 05:25, Anup Patel wrote: > > We should create INTC domain using a synthetic fwnode which will allow > > drivers (such as RISC-V SBI IPI driver, RISC-V timer driver, RISC-V > > PMU driver, etc) not having dedicated DT/ACPI node to directly create > > interrupt mapping for standard local interrupt numbers defined by the > > RISC-V privileged specification. > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > arch/riscv/include/asm/irq.h | 2 ++ > > arch/riscv/kernel/irq.c | 13 +++++++++++++ > > drivers/clocksource/timer-clint.c | 13 +++++++------ > > drivers/clocksource/timer-riscv.c | 11 ++--------- > > drivers/irqchip/irq-riscv-intc.c | 12 ++++++++++-- > > drivers/irqchip/irq-sifive-plic.c | 19 +++++++++++-------- > > 6 files changed, 45 insertions(+), 25 deletions(-) > > > > diff --git a/arch/riscv/include/asm/irq.h > > b/arch/riscv/include/asm/irq.h > > index e4c435509983..f85ebaf07505 100644 > > --- a/arch/riscv/include/asm/irq.h > > +++ b/arch/riscv/include/asm/irq.h > > @@ -12,6 +12,8 @@ > > > > #include <asm-generic/irq.h> > > > > +extern struct fwnode_handle *riscv_intc_fwnode(void); > > + > > extern void __init init_IRQ(void); > > > > #endif /* _ASM_RISCV_IRQ_H */ > > diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c > > index 7207fa08d78f..f2fed78ab659 100644 > > --- a/arch/riscv/kernel/irq.c > > +++ b/arch/riscv/kernel/irq.c > > @@ -7,9 +7,22 @@ > > > > #include <linux/interrupt.h> > > #include <linux/irqchip.h> > > +#include <linux/irqdomain.h> > > +#include <linux/module.h> > > #include <linux/seq_file.h> > > #include <asm/smp.h> > > > > +static struct fwnode_handle *intc_fwnode; > > + > > +struct fwnode_handle *riscv_intc_fwnode(void) > > +{ > > + if (!intc_fwnode) > > + intc_fwnode = irq_domain_alloc_named_fwnode("RISCV-INTC"); > > + > > + return intc_fwnode; > > +} > > +EXPORT_SYMBOL_GPL(riscv_intc_fwnode); > > Why is this created outside of the root interrupt controller driver? > Furthermore, why do you need to create a new fwnode the first place? > As far as I can tell, the INTC does have a node, and what you don't > have is the firmware linkage between PMU (an others) and the INTC.
Fair enough, I will update this patch to not create a synthetic fwnode.
The issue is not with INTC driver. We have other drivers and places (such as SBI IPI driver, SBI PMU driver, and KVM RISC-V AIA support) where we don't have a way to locate INTC fwnode.
> > what you should have instead is something like: > > static struct fwnode_handle *(*__get_root_intc_node)(void); > struct fwnode_handle *riscv_get_root_intc_hwnode(void) > { > if (__get_root_intc_node) > return __get_root_intc_node(); > > return NULL; > } > > and the corresponding registration interface.
Thanks, I will follow this suggestion. This is a much better approach and it will avoid touching existing drivers.
> > But either way, something breaks: the INTC has one node per CPU, and > expect one irqdomain per CPU. Having a single fwnode completely breaks > the INTC driver (and probably the irqdomain list, as we don't check for > duplicate entries). > > > diff --git a/drivers/irqchip/irq-riscv-intc.c > > b/drivers/irqchip/irq-riscv-intc.c > > index b65bd8878d4f..26ed62c11768 100644 > > --- a/drivers/irqchip/irq-riscv-intc.c > > +++ b/drivers/irqchip/irq-riscv-intc.c > > @@ -112,8 +112,16 @@ static int __init riscv_intc_init(struct > > device_node *node, > > if (riscv_hartid_to_cpuid(hartid) != smp_processor_id()) > > return 0; > > > > - intc_domain = irq_domain_add_linear(node, BITS_PER_LONG, > > - &riscv_intc_domain_ops, NULL); > > + /* > > + * Create INTC domain using a synthetic fwnode which will allow > > + * drivers (such as RISC-V SBI IPI driver, RISC-V timer driver, > > + * RISC-V PMU driver, etc) not having dedicated DT/ACPI node to > > + * directly create interrupt mapping for standard local interrupt > > + * numbers defined by the RISC-V privileged specification. > > + */ > > + intc_domain = irq_domain_create_linear(riscv_intc_fwnode(), > > + BITS_PER_LONG, > > + &riscv_intc_domain_ops, NULL); > > This is what I'm talking about. It is simply broken. So either you don't > need a per-CPU node (and the DT was bad the first place), or you > absolutely need > one (and the whole 'well-known/default domain' doesn't work at all). > > Either way, this patch is plain wrong.
Okay, I will update this patch with the new approach which you suggested.
Regards, Anup
> > > M. > -- > Jazz is not dead. It just smells funny...
| |