Message ID | 20231213070301.1684751-1-peterlin@andestech.com |
---|---|
Headers | show |
Series | Support Andes PMU extension | expand |
On Wed, Dec 13, 2023 at 12:34 PM Yu Chien Peter Lin <peterlin@andestech.com> wrote: > > Currently, the implementation of the RISC-V INTC driver uses the > interrupt cause as hardware interrupt number and has a limitation of > supporting a maximum of 64 interrupts. However, according to the > privileged spec, interrupt causes >= 16 are defined for platform use. I disagree with this patch. Even though RISC-V priv sepc allows interrupt causes >= 16, we still need CSRs to manage arbitrary local interrupts Currently, we have following standard CSRs: 1) [m|s]ie and [m|s]ip which are XLEN wide 2) With AIA, we have [m|s]ieh and [m|s]iph for RV32 Clearly, we can only have a XLEN number of standard local interrupts without AIA and 64 local interrupts with AIA. Now for implementations with custom CSRs (such as Andes), we still can't assume infinite local interrupts because HW will have a finite number of custom CSRs. > > This limitation prevents to fully utilize the available local interrupt > sources. Additionally, the interrupt number used on RISC-V are sparse, > with only interrupt numbers 1, 5 and 9 (plus Sscofpmf or T-Head's PMU > interrupt) being currently used for supervisor mode. > > Switch to using irq_domain_create_tree() to create the radix tree > map, so a larger number of hardware interrupts can be handled. > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > Reviewed-by: Charles Ci-Jyun Wu <dminus@andestech.com> > Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com> > --- > Changes v1 -> v2: > - Fixed irq mapping failure checking (suggested by Clément and Anup) > Changes v2 -> v3: > - No change > Changes v3 -> v4: (Suggested by Thomas [1]) > - Use pr_warn_ratelimited instead > - Fix coding style and commit message > Changes v4 -> v5: (Suggested by Thomas) > - Fix commit message > > [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231023004100.2663486-3-peterlin@andestech.com/#25573085 > --- > drivers/irqchip/irq-riscv-intc.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > index e8d01b14ccdd..2fdd40f2a791 100644 > --- a/drivers/irqchip/irq-riscv-intc.c > +++ b/drivers/irqchip/irq-riscv-intc.c > @@ -24,10 +24,9 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs) > { > unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG; > > - if (unlikely(cause >= BITS_PER_LONG)) > - panic("unexpected interrupt cause"); > - > - generic_handle_domain_irq(intc_domain, cause); > + if (generic_handle_domain_irq(intc_domain, cause)) > + pr_warn_ratelimited("Failed to handle interrupt (cause: %ld)\n", > + cause); > } > > /* > @@ -117,8 +116,7 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn) > { > int rc; > > - intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG, > - &riscv_intc_domain_ops, NULL); > + intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL); I disagree with this change based on the reasoning above. Instead of this, we should determine the number of local interrupts based on the type of RISC-V intc: 1) For standard INTC without AIA, we have XLEN (or BITS_PER_LONG) local interrupts 2) For standart INTC with AIA, we have 64 local interrupts 3) For custom INTC (such as Andes), the number of local interrupt should be custom (Andes specific) which can be determined based on compatible string. Also, creating a linear domain with a fixed number of local interrupts ensures that drivers can't map a local interrupt beyond the availability of CSRs to manage it. > if (!intc_domain) { > pr_err("unable to add IRQ domain\n"); > return -ENXIO; > @@ -132,8 +130,6 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn) > > riscv_set_intc_hwnode_fn(riscv_intc_hwnode); > > - pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > - Same as above, we should definitely advertise the type of INTC and number of local interrupts mapped. Regards, Anup > return 0; > } > > -- > 2.34.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Dec 13, 2023 at 12:35 PM Yu Chien Peter Lin <peterlin@andestech.com> wrote: > > Add support for the Andes hart-level interrupt controller. This > controller provides interrupt mask/unmask functions to access the > custom register (SLIE) where the non-standard S-mode local interrupt > enable bits are located. > > To share the riscv_intc_domain_map() with the generic RISC-V INTC and > ACPI, add a chip parameter to riscv_intc_init_common(), so it can be > passed to the irq_domain_set_info() as private data. > > Andes hart-level interrupt controller requires the "andestech,cpu-intc" > compatible string to be present in interrupt-controller of cpu node. > e.g., > > cpu0: cpu@0 { > compatible = "andestech,ax45mp", "riscv"; > ... > cpu0-intc: interrupt-controller { > #interrupt-cells = <0x01>; > compatible = "andestech,cpu-intc", "riscv,cpu-intc"; > interrupt-controller; > }; > }; > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > Reviewed-by: Charles Ci-Jyun Wu <dminus@andestech.com> > Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com> > --- > Changes v1 -> v2: > - New patch > Changes v2 -> v3: > - Return -ENXIO if no valid compatible INTC found > - Allow falling back to generic RISC-V INTC > Changes v3 -> v4: (Suggested by Thomas [1]) > - Add comment to andes irq chip function > - Refine code flow to share with generic RISC-V INTC and ACPI > - Move Andes specific definitions to include/linux/soc/andes/irq.h > Changes v4 -> v5: (Suggested by Thomas) > - Fix commit message > - Subtract ANDES_SLI_CAUSE_BASE from d->hwirq to calculate the value of mask > - Do not set chip_data to the chip itself with irq_domain_set_info() > - Follow reverse fir tree order variable declarations > > [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231019135723.3657156-1-peterlin@andestech.com/ > --- > drivers/irqchip/irq-riscv-intc.c | 53 ++++++++++++++++++++++++++++---- > include/linux/soc/andes/irq.h | 17 ++++++++++ > 2 files changed, 64 insertions(+), 6 deletions(-) > create mode 100644 include/linux/soc/andes/irq.h > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > index 2fdd40f2a791..0b6bf3fb1dba 100644 > --- a/drivers/irqchip/irq-riscv-intc.c > +++ b/drivers/irqchip/irq-riscv-intc.c > @@ -17,6 +17,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/smp.h> > +#include <linux/soc/andes/irq.h> > > static struct irq_domain *intc_domain; > > @@ -46,6 +47,31 @@ static void riscv_intc_irq_unmask(struct irq_data *d) > csr_set(CSR_IE, BIT(d->hwirq)); > } > > +static void andes_intc_irq_mask(struct irq_data *d) > +{ > + /* > + * Andes specific S-mode local interrupt causes (hwirq) > + * are defined as (256 + n) and controlled by n-th bit > + * of SLIE. > + */ > + unsigned int mask = BIT(d->hwirq - ANDES_SLI_CAUSE_BASE); > + > + if (d->hwirq < ANDES_SLI_CAUSE_BASE) > + csr_clear(CSR_IE, mask); > + else > + csr_clear(ANDES_CSR_SLIE, mask); > +} > + > +static void andes_intc_irq_unmask(struct irq_data *d) > +{ > + unsigned int mask = BIT(d->hwirq - ANDES_SLI_CAUSE_BASE); > + > + if (d->hwirq < ANDES_SLI_CAUSE_BASE) > + csr_set(CSR_IE, mask); > + else > + csr_set(ANDES_CSR_SLIE, mask); Clearly, Andes does not have any CSR for: XLEN <= local interrupt <ANDES_SLI_CAUSE_BASE and ANDES_SLI_CAUSE_BASE + XLEN <= local interrupt Regards, Anup > +} > + > static void riscv_intc_irq_eoi(struct irq_data *d) > { > /* > @@ -69,11 +95,20 @@ static struct irq_chip riscv_intc_chip = { > .irq_eoi = riscv_intc_irq_eoi, > }; > > +static struct irq_chip andes_intc_chip = { > + .name = "RISC-V INTC", > + .irq_mask = andes_intc_irq_mask, > + .irq_unmask = andes_intc_irq_unmask, > + .irq_eoi = riscv_intc_irq_eoi, > +}; > + > static int riscv_intc_domain_map(struct irq_domain *d, unsigned int irq, > irq_hw_number_t hwirq) > { > + struct irq_chip *chip = d->host_data; > + > irq_set_percpu_devid(irq); > - irq_domain_set_info(d, irq, hwirq, &riscv_intc_chip, d->host_data, > + irq_domain_set_info(d, irq, hwirq, chip, NULL, > handle_percpu_devid_irq, NULL, NULL); > > return 0; > @@ -112,11 +147,12 @@ static struct fwnode_handle *riscv_intc_hwnode(void) > return intc_domain->fwnode; > } > > -static int __init riscv_intc_init_common(struct fwnode_handle *fn) > +static int __init riscv_intc_init_common(struct fwnode_handle *fn, > + struct irq_chip *chip) > { > int rc; > > - intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL); > + intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, chip); > if (!intc_domain) { > pr_err("unable to add IRQ domain\n"); > return -ENXIO; > @@ -136,8 +172,9 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn) > static int __init riscv_intc_init(struct device_node *node, > struct device_node *parent) > { > - int rc; > + struct irq_chip *chip = &riscv_intc_chip; > unsigned long hartid; > + int rc; > > rc = riscv_of_parent_hartid(node, &hartid); > if (rc < 0) { > @@ -162,10 +199,14 @@ static int __init riscv_intc_init(struct device_node *node, > return 0; > } > > - return riscv_intc_init_common(of_node_to_fwnode(node)); > + if (of_device_is_compatible(node, "andestech,cpu-intc")) > + chip = &andes_intc_chip; > + > + return riscv_intc_init_common(of_node_to_fwnode(node), chip); > } > > IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init); > +IRQCHIP_DECLARE(andes, "andestech,cpu-intc", riscv_intc_init); > > #ifdef CONFIG_ACPI > > @@ -192,7 +233,7 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header, > return -ENOMEM; > } > > - return riscv_intc_init_common(fn); > + return riscv_intc_init_common(fn, &riscv_intc_chip); > } > > IRQCHIP_ACPI_DECLARE(riscv_intc, ACPI_MADT_TYPE_RINTC, NULL, > diff --git a/include/linux/soc/andes/irq.h b/include/linux/soc/andes/irq.h > new file mode 100644 > index 000000000000..f03e68fea261 > --- /dev/null > +++ b/include/linux/soc/andes/irq.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2023 Andes Technology Corporation > + */ > +#ifndef __ANDES_IRQ_H > +#define __ANDES_IRQ_H > + > +/* Andes PMU irq number */ > +#define ANDES_RV_IRQ_PMU 18 > +#define ANDES_SLI_CAUSE_BASE 256 > + > +/* Andes PMU related registers */ > +#define ANDES_CSR_SLIE 0x9c4 > +#define ANDES_CSR_SLIP 0x9c5 > +#define ANDES_CSR_SCOUNTEROF 0x9d4 > + > +#endif /* __ANDES_IRQ_H */ > -- > 2.34.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Dec 13, 2023 at 7:58 PM Anup Patel <apatel@ventanamicro.com> wrote: > > On Wed, Dec 13, 2023 at 12:34 PM Yu Chien Peter Lin > <peterlin@andestech.com> wrote: > > > > Currently, the implementation of the RISC-V INTC driver uses the > > interrupt cause as hardware interrupt number and has a limitation of > > supporting a maximum of 64 interrupts. However, according to the > > privileged spec, interrupt causes >= 16 are defined for platform use. > > I disagree with this patch. > > Even though RISC-V priv sepc allows interrupt causes >= 16, we > still need CSRs to manage arbitrary local interrupts > > Currently, we have following standard CSRs: > 1) [m|s]ie and [m|s]ip which are XLEN wide > 2) With AIA, we have [m|s]ieh and [m|s]iph for RV32 > > Clearly, we can only have a XLEN number of standard local > interrupts without AIA and 64 local interrupts with AIA. > > Now for implementations with custom CSRs (such as Andes), > we still can't assume infinite local interrupts because HW will > have a finite number of custom CSRs. > > > > > This limitation prevents to fully utilize the available local interrupt > > sources. Additionally, the interrupt number used on RISC-V are sparse, > > with only interrupt numbers 1, 5 and 9 (plus Sscofpmf or T-Head's PMU > > interrupt) being currently used for supervisor mode. > > > > Switch to using irq_domain_create_tree() to create the radix tree > > map, so a larger number of hardware interrupts can be handled. > > > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > > Reviewed-by: Charles Ci-Jyun Wu <dminus@andestech.com> > > Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com> > > --- > > Changes v1 -> v2: > > - Fixed irq mapping failure checking (suggested by Clément and Anup) > > Changes v2 -> v3: > > - No change > > Changes v3 -> v4: (Suggested by Thomas [1]) > > - Use pr_warn_ratelimited instead > > - Fix coding style and commit message > > Changes v4 -> v5: (Suggested by Thomas) > > - Fix commit message > > > > [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231023004100.2663486-3-peterlin@andestech.com/#25573085 > > --- > > drivers/irqchip/irq-riscv-intc.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > index e8d01b14ccdd..2fdd40f2a791 100644 > > --- a/drivers/irqchip/irq-riscv-intc.c > > +++ b/drivers/irqchip/irq-riscv-intc.c > > @@ -24,10 +24,9 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs) > > { > > unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG; > > > > - if (unlikely(cause >= BITS_PER_LONG)) > > - panic("unexpected interrupt cause"); > > - > > - generic_handle_domain_irq(intc_domain, cause); > > + if (generic_handle_domain_irq(intc_domain, cause)) > > + pr_warn_ratelimited("Failed to handle interrupt (cause: %ld)\n", > > + cause); > > } > > > > /* > > @@ -117,8 +116,7 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn) > > { > > int rc; > > > > - intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG, > > - &riscv_intc_domain_ops, NULL); > > + intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL); > > I disagree with this change based on the reasoning above. > > Instead of this, we should determine the number of local interrupts > based on the type of RISC-V intc: > 1) For standard INTC without AIA, we have XLEN (or BITS_PER_LONG) > local interrupts > 2) For standart INTC with AIA, we have 64 local interrupts > 3) For custom INTC (such as Andes), the number of local interrupt > should be custom (Andes specific) which can be determined based > on compatible string. > > Also, creating a linear domain with a fixed number of local interrupts > ensures that drivers can't map a local interrupt beyond the availability > of CSRs to manage it. Thinking about this more. We do have a problem because Andes local interrupts are really sparse which is not the case for standard local interrupts I have an alternate suggestion which goes as follows ... We use irq_domain_create_tree() in-place of irq_domain_create_linear() and enforce checks on hwirq in riscv_intc_domain_alloc() to ensure that we only allow hwirq for which we have corresponding standard or custom CSR. To achieve this, riscv_intc_init_common() will have to save the following as static global variables: 1) riscv_intc_nr_irqs: Number of standard local interrupts 2) riscv_intc_custom_base and riscv_intc_custom_nr_irqs: Base and number of custom local interrupts. Using the above static global variables, the riscv_intc_domain_alloc() can return error if one of the following conditions are met: 1) riscv_intc_nr_irqs<= hwirq && hwirq < riscv_intc_custom_base 2) (riscv_intc_custom_base + riscv_intc_custom_nr_irqs) <= hwirq For standard INTC, we can set the static global variable as follows: riscv_intc_nr_irqs = XLEN or BITS_PER_LONG riscv_intc_custom_base = riscv_intc_nr_irqs riscv_intc_custom_nr_irqs = 0 For Andes INTC, we can set the static global variables as follows: riscv_intc_nr_irqs = XLEN or BITS_PER_LONG riscv_intc_custom_base = 256 riscv_intc_custom_nr_irqs = XLEN or BITS_PER_LONG Regards, Anup > > > if (!intc_domain) { > > pr_err("unable to add IRQ domain\n"); > > return -ENXIO; > > @@ -132,8 +130,6 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn) > > > > riscv_set_intc_hwnode_fn(riscv_intc_hwnode); > > > > - pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > > - > > Same as above, we should definitely advertise the type of INTC and > number of local interrupts mapped. > > Regards, > Anup > > > return 0; > > } > > > > -- > > 2.34.1 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Dec 13, 2023 at 03:02:57PM +0800, Yu Chien Peter Lin wrote: > xtheadpmu stands for T-Head Performance Monitor Unit extension. > Based on the added T-Head PMU ISA string, the SBI PMU driver > will make use of the non-standard irq source. > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor. > --- > Changes v4 -> v5: > - New patch > --- > arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi > index aec6401a467b..8c0143f0a01b 100644 > --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi > +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi > @@ -29,7 +29,7 @@ cpu0: cpu@0 { > riscv,isa = "rv64imafdc"; > riscv,isa-base = "rv64i"; > riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", > - "zifencei", "zihpm"; > + "zifencei", "zihpm", "xtheadpmu"; > > cpu0_intc: interrupt-controller { > compatible = "riscv,cpu-intc"; > -- > 2.34.1 >
On Wed, Dec 13, 2023 at 03:02:59PM +0800, Yu Chien Peter Lin wrote: > xtheadpmu stands for T-Head Performance Monitor Unit extension. > Based on the added T-Head PMU ISA string, the SBI PMU driver > will make use of the non-standard irq source. > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor. > --- > Changes v4 -> v5: > - New patch > --- > arch/riscv/boot/dts/thead/th1520.dtsi | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi > index ba4d2c673ac8..2dad2b22824a 100644 > --- a/arch/riscv/boot/dts/thead/th1520.dtsi > +++ b/arch/riscv/boot/dts/thead/th1520.dtsi > @@ -22,7 +22,7 @@ c910_0: cpu@0 { > riscv,isa = "rv64imafdc"; > riscv,isa-base = "rv64i"; > riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", > - "zifencei", "zihpm"; > + "zifencei", "zihpm", "xtheadpmu"; > reg = <0>; > i-cache-block-size = <64>; > i-cache-size = <65536>; > @@ -46,7 +46,7 @@ c910_1: cpu@1 { > riscv,isa = "rv64imafdc"; > riscv,isa-base = "rv64i"; > riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", > - "zifencei", "zihpm"; > + "zifencei", "zihpm", "xtheadpmu"; > reg = <1>; > i-cache-block-size = <64>; > i-cache-size = <65536>; > @@ -70,7 +70,7 @@ c910_2: cpu@2 { > riscv,isa = "rv64imafdc"; > riscv,isa-base = "rv64i"; > riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", > - "zifencei", "zihpm"; > + "zifencei", "zihpm", "xtheadpmu"; > reg = <2>; > i-cache-block-size = <64>; > i-cache-size = <65536>; > @@ -94,7 +94,7 @@ c910_3: cpu@3 { > riscv,isa = "rv64imafdc"; > riscv,isa-base = "rv64i"; > riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", > - "zifencei", "zihpm"; > + "zifencei", "zihpm", "xtheadpmu"; > reg = <3>; > i-cache-block-size = <64>; > i-cache-size = <65536>; > -- > 2.34.1 >
On Wed, Dec 13, 2023 at 03:02:58PM +0800, Yu Chien Peter Lin wrote: > xtheadpmu stands for T-Head Performance Monitor Unit extension. > Based on the added T-Head PMU ISA string, the SBI PMU driver > will make use of the non-standard irq source. > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
On Wed, Dec 13, 2023 at 03:03:00PM +0800, Yu Chien Peter Lin wrote: > xandespmu stands for Andes Performance Monitor Unit extension. > Based on the added Andes PMU ISA string, the SBI PMU driver > will make use of the non-standard irq source. > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
On Wed, Dec 13, 2023 at 03:02:52PM +0800, Yu Chien Peter Lin wrote: > The custom PMU extension aims to support perf event sampling prior > to the ratification of Sscofpmf. Instead of diverting the bits and > register reserved for future standard, a set of custom registers is > added. Hence, we may consider it as a CPU feature rather than an > erratum. > > T-Head cores need to append "xtheadpmu" to the riscv,isa-extensions > for each cpu node in device tree, and enable CONFIG_THEAD_CUSTOM_PMU > for proper functioning as of this commit. > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > Reviewed-by: Guo Ren <guoren@kernel.org> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
On Wed, Dec 13, 2023 at 03:27:25PM +0000, Conor Dooley wrote: > On Wed, Dec 13, 2023 at 03:02:52PM +0800, Yu Chien Peter Lin wrote: > > The custom PMU extension aims to support perf event sampling prior > > to the ratification of Sscofpmf. Instead of diverting the bits and > > register reserved for future standard, a set of custom registers is > > added. Hence, we may consider it as a CPU feature rather than an > > erratum. > > > > T-Head cores need to append "xtheadpmu" to the riscv,isa-extensions > > for each cpu node in device tree, and enable CONFIG_THEAD_CUSTOM_PMU > > for proper functioning as of this commit. > > > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > > Reviewed-by: Guo Ren <guoren@kernel.org> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> I think it is also worth mentioning that the only SoC, to my knowledge, that works with a mainline kernel, and supports the SBI PMU is the D1, and only recently has the OpenSBI port for the SoC been fixed to actually work correctly, and that has apparently not yet made it to a release of OpenSBI, making the "damage" caused by requiring a DT property for PMU support not all that bad since the firmware needs to be changed anyway. Thanks for your work on this, Conor.
On Wed, Dec 13, 2023 at 08:15:28PM +0530, Anup Patel wrote: > On Wed, Dec 13, 2023 at 12:35 PM Yu Chien Peter Lin > <peterlin@andestech.com> wrote: > > > > Add support for the Andes hart-level interrupt controller. This > > controller provides interrupt mask/unmask functions to access the > > custom register (SLIE) where the non-standard S-mode local interrupt > > enable bits are located. > > > > To share the riscv_intc_domain_map() with the generic RISC-V INTC and > > ACPI, add a chip parameter to riscv_intc_init_common(), so it can be > > passed to the irq_domain_set_info() as private data. > > > > Andes hart-level interrupt controller requires the "andestech,cpu-intc" > > compatible string to be present in interrupt-controller of cpu node. > > e.g., > > > > cpu0: cpu@0 { > > compatible = "andestech,ax45mp", "riscv"; > > ... > > cpu0-intc: interrupt-controller { > > #interrupt-cells = <0x01>; > > compatible = "andestech,cpu-intc", "riscv,cpu-intc"; > > interrupt-controller; > > }; > > }; > > > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > > Reviewed-by: Charles Ci-Jyun Wu <dminus@andestech.com> > > Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com> > > --- > > Changes v1 -> v2: > > - New patch > > Changes v2 -> v3: > > - Return -ENXIO if no valid compatible INTC found > > - Allow falling back to generic RISC-V INTC > > Changes v3 -> v4: (Suggested by Thomas [1]) > > - Add comment to andes irq chip function > > - Refine code flow to share with generic RISC-V INTC and ACPI > > - Move Andes specific definitions to include/linux/soc/andes/irq.h > > Changes v4 -> v5: (Suggested by Thomas) > > - Fix commit message > > - Subtract ANDES_SLI_CAUSE_BASE from d->hwirq to calculate the value of mask > > - Do not set chip_data to the chip itself with irq_domain_set_info() > > - Follow reverse fir tree order variable declarations > > > > [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231019135723.3657156-1-peterlin@andestech.com/ > > --- > > drivers/irqchip/irq-riscv-intc.c | 53 ++++++++++++++++++++++++++++---- > > include/linux/soc/andes/irq.h | 17 ++++++++++ > > 2 files changed, 64 insertions(+), 6 deletions(-) > > create mode 100644 include/linux/soc/andes/irq.h > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > index 2fdd40f2a791..0b6bf3fb1dba 100644 > > --- a/drivers/irqchip/irq-riscv-intc.c > > +++ b/drivers/irqchip/irq-riscv-intc.c > > @@ -17,6 +17,7 @@ > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/smp.h> > > +#include <linux/soc/andes/irq.h> > > > > static struct irq_domain *intc_domain; > > > > @@ -46,6 +47,31 @@ static void riscv_intc_irq_unmask(struct irq_data *d) > > csr_set(CSR_IE, BIT(d->hwirq)); > > } > > > > +static void andes_intc_irq_mask(struct irq_data *d) > > +{ > > + /* > > + * Andes specific S-mode local interrupt causes (hwirq) > > + * are defined as (256 + n) and controlled by n-th bit > > + * of SLIE. > > + */ > > + unsigned int mask = BIT(d->hwirq - ANDES_SLI_CAUSE_BASE); > > + > > + if (d->hwirq < ANDES_SLI_CAUSE_BASE) > > + csr_clear(CSR_IE, mask); > > + else > > + csr_clear(ANDES_CSR_SLIE, mask); > > +} > > + > > +static void andes_intc_irq_unmask(struct irq_data *d) > > +{ > > + unsigned int mask = BIT(d->hwirq - ANDES_SLI_CAUSE_BASE); > > + > > + if (d->hwirq < ANDES_SLI_CAUSE_BASE) > > + csr_set(CSR_IE, mask); > > + else > > + csr_set(ANDES_CSR_SLIE, mask); > > Clearly, Andes does not have any CSR for: > XLEN <= local interrupt <ANDES_SLI_CAUSE_BASE > and > ANDES_SLI_CAUSE_BASE + XLEN <= local interrupt Ah, what am I doing here. sorry for that silly patch. Regards, Peter Lin > Regards, > Anup
On Wed, Dec 13, 2023 at 9:15 PM Yu-Chien Peter Lin <peterlin@andestech.com> wrote: > > On Wed, Dec 13, 2023 at 08:15:28PM +0530, Anup Patel wrote: > > On Wed, Dec 13, 2023 at 12:35 PM Yu Chien Peter Lin > > <peterlin@andestech.com> wrote: > > > > > > Add support for the Andes hart-level interrupt controller. This > > > controller provides interrupt mask/unmask functions to access the > > > custom register (SLIE) where the non-standard S-mode local interrupt > > > enable bits are located. > > > > > > To share the riscv_intc_domain_map() with the generic RISC-V INTC and > > > ACPI, add a chip parameter to riscv_intc_init_common(), so it can be > > > passed to the irq_domain_set_info() as private data. > > > > > > Andes hart-level interrupt controller requires the "andestech,cpu-intc" > > > compatible string to be present in interrupt-controller of cpu node. > > > e.g., > > > > > > cpu0: cpu@0 { > > > compatible = "andestech,ax45mp", "riscv"; > > > ... > > > cpu0-intc: interrupt-controller { > > > #interrupt-cells = <0x01>; > > > compatible = "andestech,cpu-intc", "riscv,cpu-intc"; > > > interrupt-controller; > > > }; > > > }; > > > > > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > > > Reviewed-by: Charles Ci-Jyun Wu <dminus@andestech.com> > > > Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com> > > > --- > > > Changes v1 -> v2: > > > - New patch > > > Changes v2 -> v3: > > > - Return -ENXIO if no valid compatible INTC found > > > - Allow falling back to generic RISC-V INTC > > > Changes v3 -> v4: (Suggested by Thomas [1]) > > > - Add comment to andes irq chip function > > > - Refine code flow to share with generic RISC-V INTC and ACPI > > > - Move Andes specific definitions to include/linux/soc/andes/irq.h > > > Changes v4 -> v5: (Suggested by Thomas) > > > - Fix commit message > > > - Subtract ANDES_SLI_CAUSE_BASE from d->hwirq to calculate the value of mask > > > - Do not set chip_data to the chip itself with irq_domain_set_info() > > > - Follow reverse fir tree order variable declarations > > > > > > [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231019135723.3657156-1-peterlin@andestech.com/ > > > --- > > > drivers/irqchip/irq-riscv-intc.c | 53 ++++++++++++++++++++++++++++---- > > > include/linux/soc/andes/irq.h | 17 ++++++++++ > > > 2 files changed, 64 insertions(+), 6 deletions(-) > > > create mode 100644 include/linux/soc/andes/irq.h > > > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > > index 2fdd40f2a791..0b6bf3fb1dba 100644 > > > --- a/drivers/irqchip/irq-riscv-intc.c > > > +++ b/drivers/irqchip/irq-riscv-intc.c > > > @@ -17,6 +17,7 @@ > > > #include <linux/module.h> > > > #include <linux/of.h> > > > #include <linux/smp.h> > > > +#include <linux/soc/andes/irq.h> > > > > > > static struct irq_domain *intc_domain; > > > > > > @@ -46,6 +47,31 @@ static void riscv_intc_irq_unmask(struct irq_data *d) > > > csr_set(CSR_IE, BIT(d->hwirq)); > > > } > > > > > > +static void andes_intc_irq_mask(struct irq_data *d) > > > +{ > > > + /* > > > + * Andes specific S-mode local interrupt causes (hwirq) > > > + * are defined as (256 + n) and controlled by n-th bit > > > + * of SLIE. > > > + */ > > > + unsigned int mask = BIT(d->hwirq - ANDES_SLI_CAUSE_BASE); > > > + > > > + if (d->hwirq < ANDES_SLI_CAUSE_BASE) > > > + csr_clear(CSR_IE, mask); > > > + else > > > + csr_clear(ANDES_CSR_SLIE, mask); > > > +} > > > + > > > +static void andes_intc_irq_unmask(struct irq_data *d) > > > +{ > > > + unsigned int mask = BIT(d->hwirq - ANDES_SLI_CAUSE_BASE); > > > + > > > + if (d->hwirq < ANDES_SLI_CAUSE_BASE) > > > + csr_set(CSR_IE, mask); > > > + else > > > + csr_set(ANDES_CSR_SLIE, mask); > > > > Clearly, Andes does not have any CSR for: > > XLEN <= local interrupt <ANDES_SLI_CAUSE_BASE > > and > > ANDES_SLI_CAUSE_BASE + XLEN <= local interrupt > > Ah, what am I doing here. > sorry for that silly patch. This patch is okay only if we can guarantee that hwirq is within accepted range. For example, riscv_intc_domain_alloc() can deny invalid local interrupts. Regards, Anup
Hi Anup, On Wed, Dec 13, 2023 at 08:49:23PM +0530, Anup Patel wrote: > On Wed, Dec 13, 2023 at 7:58 PM Anup Patel <apatel@ventanamicro.com> wrote: > > > > On Wed, Dec 13, 2023 at 12:34 PM Yu Chien Peter Lin > > <peterlin@andestech.com> wrote: > > > > > > Currently, the implementation of the RISC-V INTC driver uses the > > > interrupt cause as hardware interrupt number and has a limitation of > > > supporting a maximum of 64 interrupts. However, according to the > > > privileged spec, interrupt causes >= 16 are defined for platform use. > > > > I disagree with this patch. > > > > Even though RISC-V priv sepc allows interrupt causes >= 16, we > > still need CSRs to manage arbitrary local interrupts > > > > Currently, we have following standard CSRs: > > 1) [m|s]ie and [m|s]ip which are XLEN wide > > 2) With AIA, we have [m|s]ieh and [m|s]iph for RV32 > > > > Clearly, we can only have a XLEN number of standard local > > interrupts without AIA and 64 local interrupts with AIA. > > > > Now for implementations with custom CSRs (such as Andes), > > we still can't assume infinite local interrupts because HW will > > have a finite number of custom CSRs. > > > > > > > > This limitation prevents to fully utilize the available local interrupt > > > sources. Additionally, the interrupt number used on RISC-V are sparse, > > > with only interrupt numbers 1, 5 and 9 (plus Sscofpmf or T-Head's PMU > > > interrupt) being currently used for supervisor mode. > > > > > > Switch to using irq_domain_create_tree() to create the radix tree > > > map, so a larger number of hardware interrupts can be handled. > > > > > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com> > > > Reviewed-by: Charles Ci-Jyun Wu <dminus@andestech.com> > > > Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com> > > > --- > > > Changes v1 -> v2: > > > - Fixed irq mapping failure checking (suggested by Clément and Anup) > > > Changes v2 -> v3: > > > - No change > > > Changes v3 -> v4: (Suggested by Thomas [1]) > > > - Use pr_warn_ratelimited instead > > > - Fix coding style and commit message > > > Changes v4 -> v5: (Suggested by Thomas) > > > - Fix commit message > > > > > > [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231023004100.2663486-3-peterlin@andestech.com/#25573085 > > > --- > > > drivers/irqchip/irq-riscv-intc.c | 12 ++++-------- > > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > > index e8d01b14ccdd..2fdd40f2a791 100644 > > > --- a/drivers/irqchip/irq-riscv-intc.c > > > +++ b/drivers/irqchip/irq-riscv-intc.c > > > @@ -24,10 +24,9 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs) > > > { > > > unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG; > > > > > > - if (unlikely(cause >= BITS_PER_LONG)) > > > - panic("unexpected interrupt cause"); > > > - > > > - generic_handle_domain_irq(intc_domain, cause); > > > + if (generic_handle_domain_irq(intc_domain, cause)) > > > + pr_warn_ratelimited("Failed to handle interrupt (cause: %ld)\n", > > > + cause); > > > } > > > > > > /* > > > @@ -117,8 +116,7 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn) > > > { > > > int rc; > > > > > > - intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG, > > > - &riscv_intc_domain_ops, NULL); > > > + intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL); > > > > I disagree with this change based on the reasoning above. > > > > Instead of this, we should determine the number of local interrupts > > based on the type of RISC-V intc: > > 1) For standard INTC without AIA, we have XLEN (or BITS_PER_LONG) > > local interrupts > > 2) For standart INTC with AIA, we have 64 local interrupts > > 3) For custom INTC (such as Andes), the number of local interrupt > > should be custom (Andes specific) which can be determined based > > on compatible string. > > > > Also, creating a linear domain with a fixed number of local interrupts > > ensures that drivers can't map a local interrupt beyond the availability > > of CSRs to manage it. > > Thinking about this more. We do have a problem because Andes local > interrupts are really sparse which is not the case for standard local > interrupts > > I have an alternate suggestion which goes as follows ... > > We use irq_domain_create_tree() in-place of irq_domain_create_linear() > and enforce checks on hwirq in riscv_intc_domain_alloc() to ensure that > we only allow hwirq for which we have corresponding standard or custom > CSR. > > To achieve this, riscv_intc_init_common() will have to save the following > as static global variables: > 1) riscv_intc_nr_irqs: Number of standard local interrupts > 2) riscv_intc_custom_base and riscv_intc_custom_nr_irqs: Base and > number of custom local interrupts. > > Using the above static global variables, the riscv_intc_domain_alloc() > can return error if one of the following conditions are met: > 1) riscv_intc_nr_irqs<= hwirq && hwirq < riscv_intc_custom_base > 2) (riscv_intc_custom_base + riscv_intc_custom_nr_irqs) <= hwirq > > For standard INTC, we can set the static global variable as follows: > riscv_intc_nr_irqs = XLEN or BITS_PER_LONG > riscv_intc_custom_base = riscv_intc_nr_irqs > riscv_intc_custom_nr_irqs = 0 > > For Andes INTC, we can set the static global variables as follows: > riscv_intc_nr_irqs = XLEN or BITS_PER_LONG > riscv_intc_custom_base = 256 > riscv_intc_custom_nr_irqs = XLEN or BITS_PER_LONG > > Regards, > Anup Thank you for offering your help on this. I will rework the patch accordingly. Best regards, Peter Lin > > > > > if (!intc_domain) { > > > pr_err("unable to add IRQ domain\n"); > > > return -ENXIO; > > > @@ -132,8 +130,6 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn) > > > > > > riscv_set_intc_hwnode_fn(riscv_intc_hwnode); > > > > > > - pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > > > - > > > > Same as above, we should definitely advertise the type of INTC and > > number of local interrupts mapped. > > > > Regards, > > Anup > > > > > return 0; > > > } > > > > > > -- > > > 2.34.1 > > > > > > > > > _______________________________________________ > > > linux-riscv mailing list > > > linux-riscv@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-riscv