| Message ID | 20220125054217.383482-3-apatel@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V IPI Improvements | expand |
On Tue, 25 Jan 2022 05:42:13 +0000, Anup Patel <apatel@ventanamicro.com> wrote: > > We have quite a few RISC-V drivers (such as RISC-V SBI IPI driver, > RISC-V timer driver, RISC-V PMU driver, etc) which do not have a > dedicated DT/ACPI fwnode. This patch makes intc domain as the default > host so that these drivers can directly create local interrupt mapping > using standardized local interrupt numbers > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > --- > drivers/clocksource/timer-riscv.c | 17 +---------------- > drivers/irqchip/irq-riscv-intc.c | 9 +++++++++ > 2 files changed, 10 insertions(+), 16 deletions(-) > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > index 1767f8bf2013..dd6916ae6365 100644 > --- a/drivers/clocksource/timer-riscv.c > +++ b/drivers/clocksource/timer-riscv.c > @@ -102,8 +102,6 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) > static int __init riscv_timer_init_dt(struct device_node *n) > { > int cpuid, hartid, error; > - struct device_node *child; > - struct irq_domain *domain; > > hartid = riscv_of_processor_hartid(n); > if (hartid < 0) { > @@ -121,20 +119,7 @@ static int __init riscv_timer_init_dt(struct device_node *n) > if (cpuid != smp_processor_id()) > return 0; > > - domain = NULL; > - child = of_get_compatible_child(n, "riscv,cpu-intc"); > - if (!child) { > - pr_err("Failed to find INTC node [%pOF]\n", n); > - return -ENODEV; > - } > - domain = irq_find_host(child); > - of_node_put(child); > - if (!domain) { > - pr_err("Failed to find IRQ domain for node [%pOF]\n", n); > - return -ENODEV; > - } > - > - riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER); > + riscv_clock_event_irq = irq_create_mapping(NULL, RV_IRQ_TIMER); > if (!riscv_clock_event_irq) { > pr_err("Failed to map timer interrupt for node [%pOF]\n", n); > return -ENODEV; > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > index b65bd8878d4f..9f0a7a8a5c4d 100644 > --- a/drivers/irqchip/irq-riscv-intc.c > +++ b/drivers/irqchip/irq-riscv-intc.c > @@ -125,6 +125,15 @@ static int __init riscv_intc_init(struct device_node *node, > return rc; > } > > + /* > + * Make INTC as the default domain which will allow drivers > + * not having dedicated DT/ACPI fwnode (such as RISC-V SBI IPI > + * driver, RISC-V timer driver, RISC-V PMU driver, etc) can > + * directly create local interrupt mapping using standardized > + * local interrupt numbers. > + */ > + irq_set_default_host(intc_domain); No, please. This really is a bad idea. This sort of catch-all have constantly proven to be a nuisance, because they discard all the topology information. Eventually, you realise that you need to know where this is coming from, but it really is too late. I'd rather you *synthesise* a fwnode (like ACPI does) rather then do this. M.
On Tue, Jan 25, 2022 at 11:47 PM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 25 Jan 2022 05:42:13 +0000, > Anup Patel <apatel@ventanamicro.com> wrote: > > > > We have quite a few RISC-V drivers (such as RISC-V SBI IPI driver, > > RISC-V timer driver, RISC-V PMU driver, etc) which do not have a > > dedicated DT/ACPI fwnode. This patch makes intc domain as the default > > host so that these drivers can directly create local interrupt mapping > > using standardized local interrupt numbers > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > --- > > drivers/clocksource/timer-riscv.c | 17 +---------------- > > drivers/irqchip/irq-riscv-intc.c | 9 +++++++++ > > 2 files changed, 10 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > > index 1767f8bf2013..dd6916ae6365 100644 > > --- a/drivers/clocksource/timer-riscv.c > > +++ b/drivers/clocksource/timer-riscv.c > > @@ -102,8 +102,6 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) > > static int __init riscv_timer_init_dt(struct device_node *n) > > { > > int cpuid, hartid, error; > > - struct device_node *child; > > - struct irq_domain *domain; > > > > hartid = riscv_of_processor_hartid(n); > > if (hartid < 0) { > > @@ -121,20 +119,7 @@ static int __init riscv_timer_init_dt(struct device_node *n) > > if (cpuid != smp_processor_id()) > > return 0; > > > > - domain = NULL; > > - child = of_get_compatible_child(n, "riscv,cpu-intc"); > > - if (!child) { > > - pr_err("Failed to find INTC node [%pOF]\n", n); > > - return -ENODEV; > > - } > > - domain = irq_find_host(child); > > - of_node_put(child); > > - if (!domain) { > > - pr_err("Failed to find IRQ domain for node [%pOF]\n", n); > > - return -ENODEV; > > - } > > - > > - riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER); > > + riscv_clock_event_irq = irq_create_mapping(NULL, RV_IRQ_TIMER); > > if (!riscv_clock_event_irq) { > > pr_err("Failed to map timer interrupt for node [%pOF]\n", n); > > return -ENODEV; > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > index b65bd8878d4f..9f0a7a8a5c4d 100644 > > --- a/drivers/irqchip/irq-riscv-intc.c > > +++ b/drivers/irqchip/irq-riscv-intc.c > > @@ -125,6 +125,15 @@ static int __init riscv_intc_init(struct device_node *node, > > return rc; > > } > > > > + /* > > + * Make INTC as the default domain which will allow drivers > > + * not having dedicated DT/ACPI fwnode (such as RISC-V SBI IPI > > + * driver, RISC-V timer driver, RISC-V PMU driver, etc) can > > + * directly create local interrupt mapping using standardized > > + * local interrupt numbers. > > + */ > > + irq_set_default_host(intc_domain); > > No, please. This really is a bad idea. This sort of catch-all have > constantly proven to be a nuisance, because they discard all the > topology information. Eventually, you realise that you need to know > where this is coming from, but it really is too late. > > I'd rather you *synthesise* a fwnode (like ACPI does) rather then do > this. In absence of INTC as the default domain, currently we have various drivers looking up INTC irq_domain from DT (or ACPI). This is quite a bit of duplicate code across various drivers. How about having a irq_domain lookup routine (riscv_intc_find_irq_domain()) exported by the RISC-V INTC driver or arch/riscv ? OR Do you have an alternative suggestion ? Regards, Anup > > M. > > -- > Without deviation from the norm, progress is not possible.
On Wed, 26 Jan 2022 03:16:55 +0000, Anup Patel <anup@brainfault.org> wrote: > > On Tue, Jan 25, 2022 at 11:47 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 25 Jan 2022 05:42:13 +0000, > > Anup Patel <apatel@ventanamicro.com> wrote: > > > > > > We have quite a few RISC-V drivers (such as RISC-V SBI IPI driver, > > > RISC-V timer driver, RISC-V PMU driver, etc) which do not have a > > > dedicated DT/ACPI fwnode. This patch makes intc domain as the default > > > host so that these drivers can directly create local interrupt mapping > > > using standardized local interrupt numbers > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > --- > > > drivers/clocksource/timer-riscv.c | 17 +---------------- > > > drivers/irqchip/irq-riscv-intc.c | 9 +++++++++ > > > 2 files changed, 10 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > > > index 1767f8bf2013..dd6916ae6365 100644 > > > --- a/drivers/clocksource/timer-riscv.c > > > +++ b/drivers/clocksource/timer-riscv.c > > > @@ -102,8 +102,6 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) > > > static int __init riscv_timer_init_dt(struct device_node *n) > > > { > > > int cpuid, hartid, error; > > > - struct device_node *child; > > > - struct irq_domain *domain; > > > > > > hartid = riscv_of_processor_hartid(n); > > > if (hartid < 0) { > > > @@ -121,20 +119,7 @@ static int __init riscv_timer_init_dt(struct device_node *n) > > > if (cpuid != smp_processor_id()) > > > return 0; > > > > > > - domain = NULL; > > > - child = of_get_compatible_child(n, "riscv,cpu-intc"); > > > - if (!child) { > > > - pr_err("Failed to find INTC node [%pOF]\n", n); > > > - return -ENODEV; > > > - } > > > - domain = irq_find_host(child); > > > - of_node_put(child); > > > - if (!domain) { > > > - pr_err("Failed to find IRQ domain for node [%pOF]\n", n); > > > - return -ENODEV; > > > - } > > > - > > > - riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER); > > > + riscv_clock_event_irq = irq_create_mapping(NULL, RV_IRQ_TIMER); > > > if (!riscv_clock_event_irq) { > > > pr_err("Failed to map timer interrupt for node [%pOF]\n", n); > > > return -ENODEV; > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > > index b65bd8878d4f..9f0a7a8a5c4d 100644 > > > --- a/drivers/irqchip/irq-riscv-intc.c > > > +++ b/drivers/irqchip/irq-riscv-intc.c > > > @@ -125,6 +125,15 @@ static int __init riscv_intc_init(struct device_node *node, > > > return rc; > > > } > > > > > > + /* > > > + * Make INTC as the default domain which will allow drivers > > > + * not having dedicated DT/ACPI fwnode (such as RISC-V SBI IPI > > > + * driver, RISC-V timer driver, RISC-V PMU driver, etc) can > > > + * directly create local interrupt mapping using standardized > > > + * local interrupt numbers. > > > + */ > > > + irq_set_default_host(intc_domain); > > > > No, please. This really is a bad idea. This sort of catch-all have > > constantly proven to be a nuisance, because they discard all the > > topology information. Eventually, you realise that you need to know > > where this is coming from, but it really is too late. > > > > I'd rather you *synthesise* a fwnode (like ACPI does) rather then do > > this. > > In absence of INTC as the default domain, currently we have various > drivers looking up INTC irq_domain from DT (or ACPI). This is quite a > bit of duplicate code across various drivers. > > How about having a irq_domain lookup routine (riscv_intc_find_irq_domain()) > exported by the RISC-V INTC driver or arch/riscv ? > OR > Do you have an alternative suggestion ? But *why* don't you provide an interrupt controller node for DT? I really don't think that's outlandish to require. For ACPI, we already have an interface that allows a fwnode to be registered (acpi_set_irq_model) and interrupts mapped (acpi_register_gsi). You should already have all the required tools you need. M.
On Wed, Jan 26, 2022 at 2:31 PM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 26 Jan 2022 03:16:55 +0000, > Anup Patel <anup@brainfault.org> wrote: > > > > On Tue, Jan 25, 2022 at 11:47 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Tue, 25 Jan 2022 05:42:13 +0000, > > > Anup Patel <apatel@ventanamicro.com> wrote: > > > > > > > > We have quite a few RISC-V drivers (such as RISC-V SBI IPI driver, > > > > RISC-V timer driver, RISC-V PMU driver, etc) which do not have a > > > > dedicated DT/ACPI fwnode. This patch makes intc domain as the default > > > > host so that these drivers can directly create local interrupt mapping > > > > using standardized local interrupt numbers > > > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > > --- > > > > drivers/clocksource/timer-riscv.c | 17 +---------------- > > > > drivers/irqchip/irq-riscv-intc.c | 9 +++++++++ > > > > 2 files changed, 10 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > > > > index 1767f8bf2013..dd6916ae6365 100644 > > > > --- a/drivers/clocksource/timer-riscv.c > > > > +++ b/drivers/clocksource/timer-riscv.c > > > > @@ -102,8 +102,6 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) > > > > static int __init riscv_timer_init_dt(struct device_node *n) > > > > { > > > > int cpuid, hartid, error; > > > > - struct device_node *child; > > > > - struct irq_domain *domain; > > > > > > > > hartid = riscv_of_processor_hartid(n); > > > > if (hartid < 0) { > > > > @@ -121,20 +119,7 @@ static int __init riscv_timer_init_dt(struct device_node *n) > > > > if (cpuid != smp_processor_id()) > > > > return 0; > > > > > > > > - domain = NULL; > > > > - child = of_get_compatible_child(n, "riscv,cpu-intc"); > > > > - if (!child) { > > > > - pr_err("Failed to find INTC node [%pOF]\n", n); > > > > - return -ENODEV; > > > > - } > > > > - domain = irq_find_host(child); > > > > - of_node_put(child); > > > > - if (!domain) { > > > > - pr_err("Failed to find IRQ domain for node [%pOF]\n", n); > > > > - return -ENODEV; > > > > - } > > > > - > > > > - riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER); > > > > + riscv_clock_event_irq = irq_create_mapping(NULL, RV_IRQ_TIMER); > > > > if (!riscv_clock_event_irq) { > > > > pr_err("Failed to map timer interrupt for node [%pOF]\n", n); > > > > return -ENODEV; > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > > > index b65bd8878d4f..9f0a7a8a5c4d 100644 > > > > --- a/drivers/irqchip/irq-riscv-intc.c > > > > +++ b/drivers/irqchip/irq-riscv-intc.c > > > > @@ -125,6 +125,15 @@ static int __init riscv_intc_init(struct device_node *node, > > > > return rc; > > > > } > > > > > > > > + /* > > > > + * Make INTC as the default domain which will allow drivers > > > > + * not having dedicated DT/ACPI fwnode (such as RISC-V SBI IPI > > > > + * driver, RISC-V timer driver, RISC-V PMU driver, etc) can > > > > + * directly create local interrupt mapping using standardized > > > > + * local interrupt numbers. > > > > + */ > > > > + irq_set_default_host(intc_domain); > > > > > > No, please. This really is a bad idea. This sort of catch-all have > > > constantly proven to be a nuisance, because they discard all the > > > topology information. Eventually, you realise that you need to know > > > where this is coming from, but it really is too late. > > > > > > I'd rather you *synthesise* a fwnode (like ACPI does) rather then do > > > this. > > > > In absence of INTC as the default domain, currently we have various > > drivers looking up INTC irq_domain from DT (or ACPI). This is quite a > > bit of duplicate code across various drivers. > > > > How about having a irq_domain lookup routine (riscv_intc_find_irq_domain()) > > exported by the RISC-V INTC driver or arch/riscv ? > > OR > > Do you have an alternative suggestion ? > > But *why* don't you provide an interrupt controller node for DT? I > really don't think that's outlandish to require. Historically, all RISC-V SBI related drivers never had any DT/ACPI node because we can always query/discover the SBI functionality at runtime. The mechanism to query/discover SBI IPI, Timer and PMU is through SBI base functions. Also, local interrupts used by these drivers are specified by the RISC-V specification. This means having a DT/ACPI node for these drivers doesn't provide any info. We will be having KVM RISC-V AIA support in future which will not have a DT/ACPI node as well because this can be discovered as a CPU capability and the local interrupt to be used is specified by the RISC-V hypervisor specification. > > For ACPI, we already have an interface that allows a fwnode to be > registered (acpi_set_irq_model) and interrupts mapped > (acpi_register_gsi). The ACPI specification being proposed for RISC-V does not have any details for SBI IPI, Timer, and PMU for the same reasons mentioned above. > > You should already have all the required tools you need. Are you okay if arch/riscv exports riscv_intc_find_irq_domain() ? OR Maybe export riscv_intc_find_irq_domain() from INTC driver ? Regards, Anup > > M. > > -- > Without deviation from the norm, progress is not possible.
On Wed, 26 Jan 2022 10:12:25 +0000, Anup Patel <apatel@ventanamicro.com> wrote: > > On Wed, Jan 26, 2022 at 2:31 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Wed, 26 Jan 2022 03:16:55 +0000, > > Anup Patel <anup@brainfault.org> wrote: > > > > > > On Tue, Jan 25, 2022 at 11:47 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Tue, 25 Jan 2022 05:42:13 +0000, > > > > Anup Patel <apatel@ventanamicro.com> wrote: > > > > > > > > > > We have quite a few RISC-V drivers (such as RISC-V SBI IPI driver, > > > > > RISC-V timer driver, RISC-V PMU driver, etc) which do not have a > > > > > dedicated DT/ACPI fwnode. This patch makes intc domain as the default > > > > > host so that these drivers can directly create local interrupt mapping > > > > > using standardized local interrupt numbers > > > > > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > > > --- > > > > > drivers/clocksource/timer-riscv.c | 17 +---------------- > > > > > drivers/irqchip/irq-riscv-intc.c | 9 +++++++++ > > > > > 2 files changed, 10 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > > > > > index 1767f8bf2013..dd6916ae6365 100644 > > > > > --- a/drivers/clocksource/timer-riscv.c > > > > > +++ b/drivers/clocksource/timer-riscv.c > > > > > @@ -102,8 +102,6 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) > > > > > static int __init riscv_timer_init_dt(struct device_node *n) > > > > > { > > > > > int cpuid, hartid, error; > > > > > - struct device_node *child; > > > > > - struct irq_domain *domain; > > > > > > > > > > hartid = riscv_of_processor_hartid(n); > > > > > if (hartid < 0) { > > > > > @@ -121,20 +119,7 @@ static int __init riscv_timer_init_dt(struct device_node *n) > > > > > if (cpuid != smp_processor_id()) > > > > > return 0; > > > > > > > > > > - domain = NULL; > > > > > - child = of_get_compatible_child(n, "riscv,cpu-intc"); > > > > > - if (!child) { > > > > > - pr_err("Failed to find INTC node [%pOF]\n", n); > > > > > - return -ENODEV; > > > > > - } > > > > > - domain = irq_find_host(child); > > > > > - of_node_put(child); > > > > > - if (!domain) { > > > > > - pr_err("Failed to find IRQ domain for node [%pOF]\n", n); > > > > > - return -ENODEV; > > > > > - } > > > > > - > > > > > - riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER); > > > > > + riscv_clock_event_irq = irq_create_mapping(NULL, RV_IRQ_TIMER); > > > > > if (!riscv_clock_event_irq) { > > > > > pr_err("Failed to map timer interrupt for node [%pOF]\n", n); > > > > > return -ENODEV; > > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > > > > index b65bd8878d4f..9f0a7a8a5c4d 100644 > > > > > --- a/drivers/irqchip/irq-riscv-intc.c > > > > > +++ b/drivers/irqchip/irq-riscv-intc.c > > > > > @@ -125,6 +125,15 @@ static int __init riscv_intc_init(struct device_node *node, > > > > > return rc; > > > > > } > > > > > > > > > > + /* > > > > > + * Make INTC as the default domain which will allow drivers > > > > > + * not having dedicated DT/ACPI fwnode (such as RISC-V SBI IPI > > > > > + * driver, RISC-V timer driver, RISC-V PMU driver, etc) can > > > > > + * directly create local interrupt mapping using standardized > > > > > + * local interrupt numbers. > > > > > + */ > > > > > + irq_set_default_host(intc_domain); > > > > > > > > No, please. This really is a bad idea. This sort of catch-all have > > > > constantly proven to be a nuisance, because they discard all the > > > > topology information. Eventually, you realise that you need to know > > > > where this is coming from, but it really is too late. > > > > > > > > I'd rather you *synthesise* a fwnode (like ACPI does) rather then do > > > > this. > > > > > > In absence of INTC as the default domain, currently we have various > > > drivers looking up INTC irq_domain from DT (or ACPI). This is quite a > > > bit of duplicate code across various drivers. > > > > > > How about having a irq_domain lookup routine (riscv_intc_find_irq_domain()) > > > exported by the RISC-V INTC driver or arch/riscv ? > > > OR > > > Do you have an alternative suggestion ? > > > > But *why* don't you provide an interrupt controller node for DT? I > > really don't think that's outlandish to require. > > Historically, all RISC-V SBI related drivers never had any DT/ACPI > node because we can always query/discover the SBI functionality > at runtime. > > The mechanism to query/discover SBI IPI, Timer and PMU is > through SBI base functions. Also, local interrupts used by these > drivers are specified by the RISC-V specification. This means having > a DT/ACPI node for these drivers doesn't provide any info. > > We will be having KVM RISC-V AIA support in future which will not > have a DT/ACPI node as well because this can be discovered as a > CPU capability and the local interrupt to be used is specified by the > RISC-V hypervisor specification. > > > > > For ACPI, we already have an interface that allows a fwnode to be > > registered (acpi_set_irq_model) and interrupts mapped > > (acpi_register_gsi). > > The ACPI specification being proposed for RISC-V does not have > any details for SBI IPI, Timer, and PMU for the same reasons > mentioned above. Neither does it on the other architectures. And yet we are able to synthesise fwnodes and use the whole of the infrastructure as intended without having to resort to this crap that was only introduced to cope with 20 year old PPC board files. Only dead architectures are using irq_set_default_host(). > > > > > You should already have all the required tools you need. > > Are you okay if arch/riscv exports riscv_intc_find_irq_domain() ? > OR > Maybe export riscv_intc_find_irq_domain() from INTC driver ? Neither. That's just papering over the core problem. Either you start creating fwnodes out of thin air, which is what we do for both x86 and arm64 when using ACPI, or you add support for SBI (or whatever new spec the RISC-V people come up with) as a provider of fwnodes. Anything else looks like a pretty bad regression. Thanks, M.
On Wed, Jan 26, 2022 at 4:17 PM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 26 Jan 2022 10:12:25 +0000, > Anup Patel <apatel@ventanamicro.com> wrote: > > > > On Wed, Jan 26, 2022 at 2:31 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Wed, 26 Jan 2022 03:16:55 +0000, > > > Anup Patel <anup@brainfault.org> wrote: > > > > > > > > On Tue, Jan 25, 2022 at 11:47 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Tue, 25 Jan 2022 05:42:13 +0000, > > > > > Anup Patel <apatel@ventanamicro.com> wrote: > > > > > > > > > > > > We have quite a few RISC-V drivers (such as RISC-V SBI IPI driver, > > > > > > RISC-V timer driver, RISC-V PMU driver, etc) which do not have a > > > > > > dedicated DT/ACPI fwnode. This patch makes intc domain as the default > > > > > > host so that these drivers can directly create local interrupt mapping > > > > > > using standardized local interrupt numbers > > > > > > > > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com> > > > > > > --- > > > > > > drivers/clocksource/timer-riscv.c | 17 +---------------- > > > > > > drivers/irqchip/irq-riscv-intc.c | 9 +++++++++ > > > > > > 2 files changed, 10 insertions(+), 16 deletions(-) > > > > > > > > > > > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > > > > > > index 1767f8bf2013..dd6916ae6365 100644 > > > > > > --- a/drivers/clocksource/timer-riscv.c > > > > > > +++ b/drivers/clocksource/timer-riscv.c > > > > > > @@ -102,8 +102,6 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) > > > > > > static int __init riscv_timer_init_dt(struct device_node *n) > > > > > > { > > > > > > int cpuid, hartid, error; > > > > > > - struct device_node *child; > > > > > > - struct irq_domain *domain; > > > > > > > > > > > > hartid = riscv_of_processor_hartid(n); > > > > > > if (hartid < 0) { > > > > > > @@ -121,20 +119,7 @@ static int __init riscv_timer_init_dt(struct device_node *n) > > > > > > if (cpuid != smp_processor_id()) > > > > > > return 0; > > > > > > > > > > > > - domain = NULL; > > > > > > - child = of_get_compatible_child(n, "riscv,cpu-intc"); > > > > > > - if (!child) { > > > > > > - pr_err("Failed to find INTC node [%pOF]\n", n); > > > > > > - return -ENODEV; > > > > > > - } > > > > > > - domain = irq_find_host(child); > > > > > > - of_node_put(child); > > > > > > - if (!domain) { > > > > > > - pr_err("Failed to find IRQ domain for node [%pOF]\n", n); > > > > > > - return -ENODEV; > > > > > > - } > > > > > > - > > > > > > - riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER); > > > > > > + riscv_clock_event_irq = irq_create_mapping(NULL, RV_IRQ_TIMER); > > > > > > if (!riscv_clock_event_irq) { > > > > > > pr_err("Failed to map timer interrupt for node [%pOF]\n", n); > > > > > > return -ENODEV; > > > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > > > > > index b65bd8878d4f..9f0a7a8a5c4d 100644 > > > > > > --- a/drivers/irqchip/irq-riscv-intc.c > > > > > > +++ b/drivers/irqchip/irq-riscv-intc.c > > > > > > @@ -125,6 +125,15 @@ static int __init riscv_intc_init(struct device_node *node, > > > > > > return rc; > > > > > > } > > > > > > > > > > > > + /* > > > > > > + * Make INTC as the default domain which will allow drivers > > > > > > + * not having dedicated DT/ACPI fwnode (such as RISC-V SBI IPI > > > > > > + * driver, RISC-V timer driver, RISC-V PMU driver, etc) can > > > > > > + * directly create local interrupt mapping using standardized > > > > > > + * local interrupt numbers. > > > > > > + */ > > > > > > + irq_set_default_host(intc_domain); > > > > > > > > > > No, please. This really is a bad idea. This sort of catch-all have > > > > > constantly proven to be a nuisance, because they discard all the > > > > > topology information. Eventually, you realise that you need to know > > > > > where this is coming from, but it really is too late. > > > > > > > > > > I'd rather you *synthesise* a fwnode (like ACPI does) rather then do > > > > > this. > > > > > > > > In absence of INTC as the default domain, currently we have various > > > > drivers looking up INTC irq_domain from DT (or ACPI). This is quite a > > > > bit of duplicate code across various drivers. > > > > > > > > How about having a irq_domain lookup routine (riscv_intc_find_irq_domain()) > > > > exported by the RISC-V INTC driver or arch/riscv ? > > > > OR > > > > Do you have an alternative suggestion ? > > > > > > But *why* don't you provide an interrupt controller node for DT? I > > > really don't think that's outlandish to require. > > > > Historically, all RISC-V SBI related drivers never had any DT/ACPI > > node because we can always query/discover the SBI functionality > > at runtime. > > > > The mechanism to query/discover SBI IPI, Timer and PMU is > > through SBI base functions. Also, local interrupts used by these > > drivers are specified by the RISC-V specification. This means having > > a DT/ACPI node for these drivers doesn't provide any info. > > > > We will be having KVM RISC-V AIA support in future which will not > > have a DT/ACPI node as well because this can be discovered as a > > CPU capability and the local interrupt to be used is specified by the > > RISC-V hypervisor specification. > > > > > > > > For ACPI, we already have an interface that allows a fwnode to be > > > registered (acpi_set_irq_model) and interrupts mapped > > > (acpi_register_gsi). > > > > The ACPI specification being proposed for RISC-V does not have > > any details for SBI IPI, Timer, and PMU for the same reasons > > mentioned above. > > Neither does it on the other architectures. > > And yet we are able to synthesise fwnodes and use the whole of the > infrastructure as intended without having to resort to this crap that > was only introduced to cope with 20 year old PPC board files. > > Only dead architectures are using irq_set_default_host(). Okay, I will drop the idea of using irq_set_default_host() in INTC driver. > > > > > > > > > You should already have all the required tools you need. > > > > Are you okay if arch/riscv exports riscv_intc_find_irq_domain() ? > > OR > > Maybe export riscv_intc_find_irq_domain() from INTC driver ? > > Neither. That's just papering over the core problem. > > Either you start creating fwnodes out of thin air, which is what we do > for both x86 and arm64 when using ACPI, or you add support for SBI (or > whatever new spec the RISC-V people come up with) as a provider of > fwnodes. > > Anything else looks like a pretty bad regression. Actually, SBI spec has been used for quite a few years in RISC-V now. It can be compared with the ARM PSCI spec so it's not a HW description format like DT or ACPI. How about arch/riscv creating an exported riscv_intc_fwnode ? (This riscv_intc_fwnode can be used by various drivers to obtain the INTC irq_domain) Regards, Anup > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c index 1767f8bf2013..dd6916ae6365 100644 --- a/drivers/clocksource/timer-riscv.c +++ b/drivers/clocksource/timer-riscv.c @@ -102,8 +102,6 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) static int __init riscv_timer_init_dt(struct device_node *n) { int cpuid, hartid, error; - struct device_node *child; - struct irq_domain *domain; hartid = riscv_of_processor_hartid(n); if (hartid < 0) { @@ -121,20 +119,7 @@ static int __init riscv_timer_init_dt(struct device_node *n) if (cpuid != smp_processor_id()) return 0; - domain = NULL; - child = of_get_compatible_child(n, "riscv,cpu-intc"); - if (!child) { - pr_err("Failed to find INTC node [%pOF]\n", n); - return -ENODEV; - } - domain = irq_find_host(child); - of_node_put(child); - if (!domain) { - pr_err("Failed to find IRQ domain for node [%pOF]\n", n); - return -ENODEV; - } - - riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER); + riscv_clock_event_irq = irq_create_mapping(NULL, RV_IRQ_TIMER); if (!riscv_clock_event_irq) { pr_err("Failed to map timer interrupt for node [%pOF]\n", n); return -ENODEV; diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c index b65bd8878d4f..9f0a7a8a5c4d 100644 --- a/drivers/irqchip/irq-riscv-intc.c +++ b/drivers/irqchip/irq-riscv-intc.c @@ -125,6 +125,15 @@ static int __init riscv_intc_init(struct device_node *node, return rc; } + /* + * Make INTC as the default domain which will allow drivers + * not having dedicated DT/ACPI fwnode (such as RISC-V SBI IPI + * driver, RISC-V timer driver, RISC-V PMU driver, etc) can + * directly create local interrupt mapping using standardized + * local interrupt numbers. + */ + irq_set_default_host(intc_domain); + cpuhp_setup_state(CPUHP_AP_IRQ_RISCV_STARTING, "irqchip/riscv/intc:starting", riscv_intc_cpu_starting,
We have quite a few RISC-V drivers (such as RISC-V SBI IPI driver, RISC-V timer driver, RISC-V PMU driver, etc) which do not have a dedicated DT/ACPI fwnode. This patch makes intc domain as the default host so that these drivers can directly create local interrupt mapping using standardized local interrupt numbers Signed-off-by: Anup Patel <apatel@ventanamicro.com> --- drivers/clocksource/timer-riscv.c | 17 +---------------- drivers/irqchip/irq-riscv-intc.c | 9 +++++++++ 2 files changed, 10 insertions(+), 16 deletions(-)