| Message ID | 20220220050854.743420-3-apatel@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V IPI Improvements | expand |
On Sun, 20 Feb 2022 05:08:50 +0000, Anup Patel <apatel@ventanamicro.com> wrote: > > Various RISC-V drivers (such as SBI IPI, SBI Timer, SBI PMU, and > KVM RISC-V) don't have associated DT node but these drivers need > standard per-CPU (local) interrupts defined by the RISC-V privileged > specification. > > We add riscv_get_intc_hwnode() in arch/riscv which allows RISC-V > drivers not having DT node to discover INTC hwnode which in-turn > helps these drivers to map per-CPU (local) interrupts provided > by the INTC driver. > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > --- > arch/riscv/include/asm/irq.h | 4 ++++ > arch/riscv/kernel/irq.c | 19 +++++++++++++++++++ > drivers/irqchip/irq-riscv-intc.c | 7 +++++++ > 3 files changed, 30 insertions(+) > > diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h > index e4c435509983..43b9ebfbd943 100644 > --- a/arch/riscv/include/asm/irq.h > +++ b/arch/riscv/include/asm/irq.h > @@ -12,6 +12,10 @@ > > #include <asm-generic/irq.h> > > +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void)); > + > +struct fwnode_handle *riscv_get_intc_hwnode(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..ead92432df8c 100644 > --- a/arch/riscv/kernel/irq.c > +++ b/arch/riscv/kernel/irq.c > @@ -7,9 +7,28 @@ > > #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 *(*__get_intc_node)(void); > + > +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void)) > +{ > + __get_intc_node = fn; > +} > +EXPORT_SYMBOL_GPL(riscv_set_intc_hwnode_fn); We're talking about the root interrupt controller here. How can this ever be implemented as a module? > + > +struct fwnode_handle *riscv_get_intc_hwnode(void) > +{ > + if (__get_intc_node) > + return __get_intc_node(); > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(riscv_get_intc_hwnode); > + > int arch_show_interrupts(struct seq_file *p, int prec) > { > show_ipi_stats(p, prec); > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > index b65bd8878d4f..fa24ecd01d39 100644 > --- a/drivers/irqchip/irq-riscv-intc.c > +++ b/drivers/irqchip/irq-riscv-intc.c > @@ -92,6 +92,11 @@ static const struct irq_domain_ops riscv_intc_domain_ops = { > .xlate = irq_domain_xlate_onecell, > }; > > +static struct fwnode_handle *riscv_intc_hwnode(void) > +{ > + return (intc_domain) ? intc_domain->fwnode : NULL; > +} This makes no sense. Either you have found the interrupt controller and allocated the domain, or you haven't. But you don't register a callback without having found it. And you have totally ignored my previous comments about the multitude of irq domains for the INTC. Either you get rid of all but one and you can register a single fwnode, or you stay with what you have today, You can't have it both ways. M.
On Mon, Feb 21, 2022 at 3:21 PM Marc Zyngier <maz@kernel.org> wrote: > > On Sun, 20 Feb 2022 05:08:50 +0000, > Anup Patel <apatel@ventanamicro.com> wrote: > > > > Various RISC-V drivers (such as SBI IPI, SBI Timer, SBI PMU, and > > KVM RISC-V) don't have associated DT node but these drivers need > > standard per-CPU (local) interrupts defined by the RISC-V privileged > > specification. > > > > We add riscv_get_intc_hwnode() in arch/riscv which allows RISC-V > > drivers not having DT node to discover INTC hwnode which in-turn > > helps these drivers to map per-CPU (local) interrupts provided > > by the INTC driver. > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > arch/riscv/include/asm/irq.h | 4 ++++ > > arch/riscv/kernel/irq.c | 19 +++++++++++++++++++ > > drivers/irqchip/irq-riscv-intc.c | 7 +++++++ > > 3 files changed, 30 insertions(+) > > > > diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h > > index e4c435509983..43b9ebfbd943 100644 > > --- a/arch/riscv/include/asm/irq.h > > +++ b/arch/riscv/include/asm/irq.h > > @@ -12,6 +12,10 @@ > > > > #include <asm-generic/irq.h> > > > > +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void)); > > + > > +struct fwnode_handle *riscv_get_intc_hwnode(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..ead92432df8c 100644 > > --- a/arch/riscv/kernel/irq.c > > +++ b/arch/riscv/kernel/irq.c > > @@ -7,9 +7,28 @@ > > > > #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 *(*__get_intc_node)(void); > > + > > +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void)) > > +{ > > + __get_intc_node = fn; > > +} > > +EXPORT_SYMBOL_GPL(riscv_set_intc_hwnode_fn); > > We're talking about the root interrupt controller here. How can this > ever be implemented as a module? Okay, I will drop the EXPORT() from here. > > > + > > +struct fwnode_handle *riscv_get_intc_hwnode(void) > > +{ > > + if (__get_intc_node) > > + return __get_intc_node(); > > + > > + return NULL; > > +} > > +EXPORT_SYMBOL_GPL(riscv_get_intc_hwnode); > > + > > int arch_show_interrupts(struct seq_file *p, int prec) > > { > > show_ipi_stats(p, prec); > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > index b65bd8878d4f..fa24ecd01d39 100644 > > --- a/drivers/irqchip/irq-riscv-intc.c > > +++ b/drivers/irqchip/irq-riscv-intc.c > > @@ -92,6 +92,11 @@ static const struct irq_domain_ops riscv_intc_domain_ops = { > > .xlate = irq_domain_xlate_onecell, > > }; > > > > +static struct fwnode_handle *riscv_intc_hwnode(void) > > +{ > > + return (intc_domain) ? intc_domain->fwnode : NULL; > > +} > > This makes no sense. Either you have found the interrupt controller > and allocated the domain, or you haven't. But you don't register a > callback without having found it. We are registering this callback after creating the INTC domain. > > And you have totally ignored my previous comments about the multitude > of irq domains for the INTC. Either you get rid of all but one and you > can register a single fwnode, or you stay with what you have today, Only the INTC DT nodes are per-CPU but we are creating only one INTC domain to manage per-CPU IRQs across all CPUs. Regards, Anup > > You can't have it both ways. > > M. > > -- > Without deviation from the norm, progress is not possible.
On Mon, Feb 21, 2022 at 3:21 PM Marc Zyngier <maz@kernel.org> wrote: > > On Sun, 20 Feb 2022 05:08:50 +0000, > Anup Patel <apatel@ventanamicro.com> wrote: > > > > Various RISC-V drivers (such as SBI IPI, SBI Timer, SBI PMU, and > > KVM RISC-V) don't have associated DT node but these drivers need > > standard per-CPU (local) interrupts defined by the RISC-V privileged > > specification. > > > > We add riscv_get_intc_hwnode() in arch/riscv which allows RISC-V > > drivers not having DT node to discover INTC hwnode which in-turn > > helps these drivers to map per-CPU (local) interrupts provided > > by the INTC driver. > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > arch/riscv/include/asm/irq.h | 4 ++++ > > arch/riscv/kernel/irq.c | 19 +++++++++++++++++++ > > drivers/irqchip/irq-riscv-intc.c | 7 +++++++ > > 3 files changed, 30 insertions(+) > > > > diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h > > index e4c435509983..43b9ebfbd943 100644 > > --- a/arch/riscv/include/asm/irq.h > > +++ b/arch/riscv/include/asm/irq.h > > @@ -12,6 +12,10 @@ > > > > #include <asm-generic/irq.h> > > > > +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void)); > > + > > +struct fwnode_handle *riscv_get_intc_hwnode(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..ead92432df8c 100644 > > --- a/arch/riscv/kernel/irq.c > > +++ b/arch/riscv/kernel/irq.c > > @@ -7,9 +7,28 @@ > > > > #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 *(*__get_intc_node)(void); > > + > > +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void)) > > +{ > > + __get_intc_node = fn; > > +} > > +EXPORT_SYMBOL_GPL(riscv_set_intc_hwnode_fn); > > We're talking about the root interrupt controller here. How can this > ever be implemented as a module? > > > + > > +struct fwnode_handle *riscv_get_intc_hwnode(void) > > +{ > > + if (__get_intc_node) > > + return __get_intc_node(); > > + > > + return NULL; > > +} > > +EXPORT_SYMBOL_GPL(riscv_get_intc_hwnode); > > + > > int arch_show_interrupts(struct seq_file *p, int prec) > > { > > show_ipi_stats(p, prec); > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > index b65bd8878d4f..fa24ecd01d39 100644 > > --- a/drivers/irqchip/irq-riscv-intc.c > > +++ b/drivers/irqchip/irq-riscv-intc.c > > @@ -92,6 +92,11 @@ static const struct irq_domain_ops riscv_intc_domain_ops = { > > .xlate = irq_domain_xlate_onecell, > > }; > > > > +static struct fwnode_handle *riscv_intc_hwnode(void) > > +{ > > + return (intc_domain) ? intc_domain->fwnode : NULL; > > +} > > This makes no sense. Either you have found the interrupt controller > and allocated the domain, or you haven't. But you don't register a > callback without having found it. Okay, I will drop the check on intc_domain since we are registering callback after creating a single INTC domain common for all CPUs. > > And you have totally ignored my previous comments about the multitude > of irq domains for the INTC. Either you get rid of all but one and you > can register a single fwnode, or you stay with what you have today, > > You can't have it both ways. > > M. > > -- > Without deviation from the norm, progress is not possible. Regards, Anup
On Mon, 21 Feb 2022 09:55:05 +0000, Anup Patel <anup@brainfault.org> wrote: > > On Mon, Feb 21, 2022 at 3:21 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Sun, 20 Feb 2022 05:08:50 +0000, > > Anup Patel <apatel@ventanamicro.com> wrote: > > > > > > Various RISC-V drivers (such as SBI IPI, SBI Timer, SBI PMU, and > > > KVM RISC-V) don't have associated DT node but these drivers need > > > standard per-CPU (local) interrupts defined by the RISC-V privileged > > > specification. > > > > > > We add riscv_get_intc_hwnode() in arch/riscv which allows RISC-V > > > drivers not having DT node to discover INTC hwnode which in-turn > > > helps these drivers to map per-CPU (local) interrupts provided > > > by the INTC driver. > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > --- > > > arch/riscv/include/asm/irq.h | 4 ++++ > > > arch/riscv/kernel/irq.c | 19 +++++++++++++++++++ > > > drivers/irqchip/irq-riscv-intc.c | 7 +++++++ > > > 3 files changed, 30 insertions(+) > > > [...] > > > index b65bd8878d4f..fa24ecd01d39 100644 > > > --- a/drivers/irqchip/irq-riscv-intc.c > > > +++ b/drivers/irqchip/irq-riscv-intc.c > > > @@ -92,6 +92,11 @@ static const struct irq_domain_ops riscv_intc_domain_ops = { > > > .xlate = irq_domain_xlate_onecell, > > > }; > > > > > > +static struct fwnode_handle *riscv_intc_hwnode(void) > > > +{ > > > + return (intc_domain) ? intc_domain->fwnode : NULL; > > > +} > > > > This makes no sense. Either you have found the interrupt controller > > and allocated the domain, or you haven't. But you don't register a > > callback without having found it. > > We are registering this callback after creating the INTC domain. Then why are you checking for intc_domain being NULL? > > And you have totally ignored my previous comments about the multitude > > of irq domains for the INTC. Either you get rid of all but one and you > > can register a single fwnode, or you stay with what you have today, > > Only the INTC DT nodes are per-CPU but we are creating only one > INTC domain to manage per-CPU IRQs across all CPUs. Ah, there is this guard that is only valid on the boot CPU. Fair enough. M.
diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h index e4c435509983..43b9ebfbd943 100644 --- a/arch/riscv/include/asm/irq.h +++ b/arch/riscv/include/asm/irq.h @@ -12,6 +12,10 @@ #include <asm-generic/irq.h> +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void)); + +struct fwnode_handle *riscv_get_intc_hwnode(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..ead92432df8c 100644 --- a/arch/riscv/kernel/irq.c +++ b/arch/riscv/kernel/irq.c @@ -7,9 +7,28 @@ #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 *(*__get_intc_node)(void); + +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void)) +{ + __get_intc_node = fn; +} +EXPORT_SYMBOL_GPL(riscv_set_intc_hwnode_fn); + +struct fwnode_handle *riscv_get_intc_hwnode(void) +{ + if (__get_intc_node) + return __get_intc_node(); + + return NULL; +} +EXPORT_SYMBOL_GPL(riscv_get_intc_hwnode); + int arch_show_interrupts(struct seq_file *p, int prec) { show_ipi_stats(p, prec); diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c index b65bd8878d4f..fa24ecd01d39 100644 --- a/drivers/irqchip/irq-riscv-intc.c +++ b/drivers/irqchip/irq-riscv-intc.c @@ -92,6 +92,11 @@ static const struct irq_domain_ops riscv_intc_domain_ops = { .xlate = irq_domain_xlate_onecell, }; +static struct fwnode_handle *riscv_intc_hwnode(void) +{ + return (intc_domain) ? intc_domain->fwnode : NULL; +} + static int __init riscv_intc_init(struct device_node *node, struct device_node *parent) { @@ -119,6 +124,8 @@ static int __init riscv_intc_init(struct device_node *node, return -ENXIO; } + riscv_set_intc_hwnode_fn(riscv_intc_hwnode); + rc = set_handle_irq(&riscv_intc_irq); if (rc) { pr_err("failed to set irq handler\n");
Various RISC-V drivers (such as SBI IPI, SBI Timer, SBI PMU, and KVM RISC-V) don't have associated DT node but these drivers need standard per-CPU (local) interrupts defined by the RISC-V privileged specification. We add riscv_get_intc_hwnode() in arch/riscv which allows RISC-V drivers not having DT node to discover INTC hwnode which in-turn helps these drivers to map per-CPU (local) interrupts provided by the INTC driver. Signed-off-by: Anup Patel <apatel@ventanamicro.com> --- arch/riscv/include/asm/irq.h | 4 ++++ arch/riscv/kernel/irq.c | 19 +++++++++++++++++++ drivers/irqchip/irq-riscv-intc.c | 7 +++++++ 3 files changed, 30 insertions(+)