| Message ID | 20230404182037.863533-1-sunilvl@ventanamicro.com |
|---|---|
| Headers | show |
| Series | Add basic ACPI support for RISC-V | expand |
On Tue, Apr 04, 2023 at 11:50:14PM +0530, Sunil V L wrote: > Changes since V3: > 1) Added two more driver patches to workaround allmodconfig build failure. btw, you need to fix the issues *before* you enable ACPI, not after.
On Tue, Apr 04, 2023 at 11:50:27PM +0530, Sunil V L wrote: > On ACPI based systems, the information about the hart > like ISA is provided by the RISC-V Hart Capabilities Table (RHCT). > Enable filling up hwcap structure based on the information in RHCT. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > --- > arch/riscv/kernel/cpufeature.c | 39 ++++++++++++++++++++++++++++++---- > 1 file changed, 35 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 63e56ce04162..5d2065b937e5 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -6,6 +6,7 @@ > * Copyright (C) 2017 SiFive > */ > > +#include <linux/acpi.h> > #include <linux/bitmap.h> > #include <linux/ctype.h> > #include <linux/libfdt.h> > @@ -13,6 +14,8 @@ > #include <linux/memory.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > +#include <asm/acpi.h> > #include <asm/alternative.h> > #include <asm/cacheflush.h> > #include <asm/errata_list.h> > @@ -91,6 +94,9 @@ void __init riscv_fill_hwcap(void) > char print_str[NUM_ALPHA_EXTS + 1]; > int i, j, rc; > unsigned long isa2hwcap[26] = {0}; > + struct acpi_table_header *rhct; > + acpi_status status; > + unsigned int cpu; > > isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I; > isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M; > @@ -103,14 +109,36 @@ void __init riscv_fill_hwcap(void) > > bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > > - for_each_of_cpu_node(node) { > + if (!acpi_disabled) { > + status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct); > + if (ACPI_FAILURE(status)) > + return; > + } > + > + for_each_possible_cpu(cpu) { > unsigned long this_hwcap = 0; > DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX); > const char *temp; > > - if (of_property_read_string(node, "riscv,isa", &isa)) { > - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > - continue; > + if (acpi_disabled) { > + node = of_cpu_device_node_get(cpu); > + if (node) { > + rc = of_property_read_string(node, "riscv,isa", &isa); Hmm, after digging in the previous patch, I think this is actually not possible to fail? We already validated it when setting up the mask of possible cpus, but I think leaving the error handling here makes things a lot more obvious. I'd swear I gave you a (conditional) R-b on v3 though, no? Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor. > + of_node_put(node); > + if (rc) { > + pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > + continue; > + } > + } else { > + pr_warn("Unable to find cpu node\n"); > + continue; > + } > + } else { > + rc = acpi_get_riscv_isa(rhct, cpu, &isa); > + if (rc < 0) { > + pr_warn("Unable to get ISA for the hart - %d\n", cpu); > + continue; > + } > } > > temp = isa; > @@ -243,6 +271,9 @@ void __init riscv_fill_hwcap(void) > bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX); > } > > + if (!acpi_disabled && rhct) > + acpi_put_table((struct acpi_table_header *)rhct); > + > /* We don't support systems with F but without D, so mask those out > * here. */ > if ((elf_hwcap & COMPAT_HWCAP_ISA_F) && !(elf_hwcap & COMPAT_HWCAP_ISA_D)) { > -- > 2.34.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Apr 04, 2023 at 11:50:31PM +0530, Sunil V L wrote: > Initialize the timer driver based on RHCT table on ACPI based > platforms. > > Currently, ACPI doesn't support a flag to indicate that the > timer interrupt can wake up the cpu irrespective of its > power state. It will be added in future update. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> My only comment on v3 was about the commit message & mentioning why there was no handling of the timer's ability to wake the cpu, so: Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor. > --- > drivers/clocksource/timer-riscv.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > index cecc4662293b..da3071b387eb 100644 > --- a/drivers/clocksource/timer-riscv.c > +++ b/drivers/clocksource/timer-riscv.c > @@ -10,6 +10,7 @@ > > #define pr_fmt(fmt) "riscv-timer: " fmt > > +#include <linux/acpi.h> > #include <linux/clocksource.h> > #include <linux/clockchips.h> > #include <linux/cpu.h> > @@ -207,3 +208,13 @@ static int __init riscv_timer_init_dt(struct device_node *n) > } > > TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt); > + > +#ifdef CONFIG_ACPI > +static int __init riscv_timer_acpi_init(struct acpi_table_header *table) > +{ > + return riscv_timer_init_common(); > +} > + > +TIMER_ACPI_DECLARE(aclint_mtimer, ACPI_SIG_RHCT, riscv_timer_acpi_init); > + > +#endif > -- > 2.34.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote: > Initialize the ACPI core for RISC-V during boot. > > ACPI tables and interpreter are initialized based on > the information passed from the firmware and the value of > the kernel parameter 'acpi'. > > With ACPI support added for RISC-V, the kernel parameter 'acpi' > is also supported on RISC-V. Hence, update the documentation. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > Acked-by: Conor Dooley <conor.dooley@microchip.com> > + /* Parse the ACPI tables for possible boot-time configuration */ > + acpi_boot_table_init(); > + if (acpi_disabled) { > + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) { > + unflatten_and_copy_device_tree(); > + } else { > + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) > + unflatten_device_tree(); > + else > + pr_err("No DTB found in kernel mappings\n"); > + } > + } else { > + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))); I'm probably forgetting something, but this seems very non-obvious to me: Why are you running early_init_dt_verify() when ACPI is enabled? I think that one deserves a comment so that next time someone looks at this (that doesn't live in ACPI land) they've know exactly why this is like it is. Doubly so since this is likely to change with some of Alex's bits moving the dtb back into the fixmap. Cheers, Conor.
Hey Sunil, This one made me scratch my head for a bit.. On Tue, Apr 04, 2023 at 11:50:37PM +0530, Sunil V L wrote: > With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in > allmodconfig build. The gcc tool chain builds this driver removing the > inline arm64 assembly code. However, clang for RISC-V tries to build > the arm64 assembly and below error is seen. There's actually nothing RISC-V specific about that behaviour, that's just how clang works. Quoting Nathan: "Clang performs semantic analysis (i.e., validates assembly) before dead code elimination, so IS_ENABLED() is not sufficient for avoiding that error." > drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint '+Q' in asm > "+Q" (*((char __iomem *)fun_base)) > ^ > It appears that RISC-V clang is not smart enough to detect > IS_ENABLED(CONFIG_ARM64) and remove the dead code. So I think this statement is just not true, it can remove dead code, but only after it has done the semantic analysis. The reason that this has not been seen before, again quoting Nathan, is: "arm64 and x86_64 both support the Q constraint, we cannot build LoongArch yet (although it does not have support for Q either so same boat as RISC-V), and ia64 is dead/unsupported in LLVM. Those are the only architectures that support ACPI, so I guess that explains why we have seen no issues aside from RISC-V so far." > As a workaround, move this check to preprocessing stage which works > with the RISC-V clang tool chain. I don't think there's much else you can do! Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Perhaps it is also worth adding: Link: https://github.com/ClangBuiltLinux/linux/issues/999 Cheers, Conor. > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > --- > drivers/crypto/hisilicon/qm.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c > index e4c84433a88a..a5f521529ab2 100644 > --- a/drivers/crypto/hisilicon/qm.c > +++ b/drivers/crypto/hisilicon/qm.c > @@ -611,13 +611,9 @@ EXPORT_SYMBOL_GPL(hisi_qm_wait_mb_ready); > static void qm_mb_write(struct hisi_qm *qm, const void *src) > { > void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE; > - unsigned long tmp0 = 0, tmp1 = 0; > > - if (!IS_ENABLED(CONFIG_ARM64)) { > - memcpy_toio(fun_base, src, 16); > - dma_wmb(); > - return; > - } > +#if IS_ENABLED(CONFIG_ARM64) > + unsigned long tmp0 = 0, tmp1 = 0; > > asm volatile("ldp %0, %1, %3\n" > "stp %0, %1, %2\n" > @@ -627,6 +623,11 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src) > "+Q" (*((char __iomem *)fun_base)) > : "Q" (*((char *)src)) > : "memory"); > +#else > + memcpy_toio(fun_base, src, 16); > + dma_wmb(); > +#endif > + > } > > static int qm_mb_nolock(struct hisi_qm *qm, struct qm_mailbox *mailbox) > -- > 2.34.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Apr 4, 2023, at 20:20, Sunil V L wrote: > With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in > allmodconfig build. The gcc tool chain builds this driver removing the > inline arm64 assembly code. However, clang for RISC-V tries to build > the arm64 assembly and below error is seen. > > drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint > '+Q' in asm > "+Q" (*((char __iomem *)fun_base)) > ^ > It appears that RISC-V clang is not smart enough to detect > IS_ENABLED(CONFIG_ARM64) and remove the dead code. > > As a workaround, move this check to preprocessing stage which works > with the RISC-V clang tool chain. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> Your patch looks correct for this particular problem, but I see that there are a couple of other issues in the same function: > - } > +#if IS_ENABLED(CONFIG_ARM64) > + unsigned long tmp0 = 0, tmp1 = 0; > > asm volatile("ldp %0, %1, %3\n" > "stp %0, %1, %2\n" > @@ -627,6 +623,11 @@ static void qm_mb_write(struct hisi_qm *qm, const > void *src) > "+Q" (*((char __iomem *)fun_base)) > : "Q" (*((char *)src)) > : "memory"); For the arm64 version: - the "dmb oshst" barrier needs to come before the stp, not after it, otherwise there is no guarantee that data written to memory is visible by the device when the mailbox gets triggered - The input/output arguments need to be pointers to 128-bit types, either a struct or a __uint128_t - this lacks a byteswap on big-endian kernels > +#else > + memcpy_toio(fun_base, src, 16); > + dma_wmb(); > +#endif This version has the same problems, plus the write is not actually atomic. I wonder if a pair of writeq() calls would just do the right thing here for both arm64 and others, or possibly a writeq() followed by a writeq_relaxed() to avoid the extra dmb() in the middle. Arnd
Hi Conor, On Tue, Apr 04, 2023 at 10:59:41PM +0100, Conor Dooley wrote: > Hey Sunil, > > This one made me scratch my head for a bit.. > > On Tue, Apr 04, 2023 at 11:50:37PM +0530, Sunil V L wrote: > > With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in > > allmodconfig build. The gcc tool chain builds this driver removing the > > inline arm64 assembly code. However, clang for RISC-V tries to build > > the arm64 assembly and below error is seen. > > There's actually nothing RISC-V specific about that behaviour, that's > just how clang works. Quoting Nathan: > "Clang performs semantic analysis (i.e., validates assembly) before > dead code elimination, so IS_ENABLED() is not sufficient for avoiding > that error." > Huh, It never occurred to me that this issue could be known already since I always thought we are hitting this first time since ACPI is enabled only now for RISC-V. Thank you very much!. > > drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint '+Q' in asm > > "+Q" (*((char __iomem *)fun_base)) > > ^ > > It appears that RISC-V clang is not smart enough to detect > > IS_ENABLED(CONFIG_ARM64) and remove the dead code. > > So I think this statement is just not true, it can remove dead code, but > only after it has done the semantic analysis. > Yes, with more details now, let me update the commit message. > The reason that this has not been seen before, again quoting Nathan, is: > "arm64 and x86_64 both support the Q constraint, we cannot build > LoongArch yet (although it does not have support for Q either so same > boat as RISC-V), and ia64 is dead/unsupported in LLVM. Those are the > only architectures that support ACPI, so I guess that explains why we > have seen no issues aside from RISC-V so far." > > > As a workaround, move this check to preprocessing stage which works > > with the RISC-V clang tool chain. > > I don't think there's much else you can do! > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > Perhaps it is also worth adding: > Link: https://github.com/ClangBuiltLinux/linux/issues/999 > Sure, Thank you very much for digging this! Thanks, Sunil
On Tue, Apr 04, 2023 at 09:57:19PM +0100, Conor Dooley wrote: > On Tue, Apr 04, 2023 at 11:50:27PM +0530, Sunil V L wrote: > > On ACPI based systems, the information about the hart > > like ISA is provided by the RISC-V Hart Capabilities Table (RHCT). > > Enable filling up hwcap structure based on the information in RHCT. > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > arch/riscv/kernel/cpufeature.c | 39 ++++++++++++++++++++++++++++++---- > > 1 file changed, 35 insertions(+), 4 deletions(-) > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index 63e56ce04162..5d2065b937e5 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -6,6 +6,7 @@ > > * Copyright (C) 2017 SiFive > > */ > > > > +#include <linux/acpi.h> > > #include <linux/bitmap.h> > > #include <linux/ctype.h> > > #include <linux/libfdt.h> > > @@ -13,6 +14,8 @@ > > #include <linux/memory.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <asm/acpi.h> > > #include <asm/alternative.h> > > #include <asm/cacheflush.h> > > #include <asm/errata_list.h> > > @@ -91,6 +94,9 @@ void __init riscv_fill_hwcap(void) > > char print_str[NUM_ALPHA_EXTS + 1]; > > int i, j, rc; > > unsigned long isa2hwcap[26] = {0}; > > + struct acpi_table_header *rhct; > > + acpi_status status; > > + unsigned int cpu; > > > > isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I; > > isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M; > > @@ -103,14 +109,36 @@ void __init riscv_fill_hwcap(void) > > > > bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > > > > - for_each_of_cpu_node(node) { > > + if (!acpi_disabled) { > > + status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct); > > + if (ACPI_FAILURE(status)) > > + return; > > + } > > + > > + for_each_possible_cpu(cpu) { > > unsigned long this_hwcap = 0; > > DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX); > > const char *temp; > > > > - if (of_property_read_string(node, "riscv,isa", &isa)) { > > - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > > - continue; > > + if (acpi_disabled) { > > + node = of_cpu_device_node_get(cpu); > > + if (node) { > > + rc = of_property_read_string(node, "riscv,isa", &isa); > > Hmm, after digging in the previous patch, I think this is actually not > possible to fail? We already validated it when setting up the mask of > possible cpus, but I think leaving the error handling here makes things > a lot more obvious. > Yeah, do you prefer to merge these patches again since only in this patch, we change the loop to for_each_possible_cpu() from for_each_of_cpu_node() which actually makes riscv_of_processor_hartid() not useful? > I'd swear I gave you a (conditional) R-b on v3 though, no? > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > Thanks, Sunil
On Wed, Apr 05, 2023 at 07:05:42PM +0530, Sunil V L wrote: > On Tue, Apr 04, 2023 at 09:57:19PM +0100, Conor Dooley wrote: > > On Tue, Apr 04, 2023 at 11:50:27PM +0530, Sunil V L wrote: > > > On ACPI based systems, the information about the hart > > > like ISA is provided by the RISC-V Hart Capabilities Table (RHCT). > > > Enable filling up hwcap structure based on the information in RHCT. > > > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > --- > > > arch/riscv/kernel/cpufeature.c | 39 ++++++++++++++++++++++++++++++---- > > > 1 file changed, 35 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > index 63e56ce04162..5d2065b937e5 100644 > > > --- a/arch/riscv/kernel/cpufeature.c > > > +++ b/arch/riscv/kernel/cpufeature.c > > > @@ -6,6 +6,7 @@ > > > * Copyright (C) 2017 SiFive > > > */ > > > > > > +#include <linux/acpi.h> > > > #include <linux/bitmap.h> > > > #include <linux/ctype.h> > > > #include <linux/libfdt.h> > > > @@ -13,6 +14,8 @@ > > > #include <linux/memory.h> > > > #include <linux/module.h> > > > #include <linux/of.h> > > > +#include <linux/of_device.h> > > > +#include <asm/acpi.h> > > > #include <asm/alternative.h> > > > #include <asm/cacheflush.h> > > > #include <asm/errata_list.h> > > > @@ -91,6 +94,9 @@ void __init riscv_fill_hwcap(void) > > > char print_str[NUM_ALPHA_EXTS + 1]; > > > int i, j, rc; > > > unsigned long isa2hwcap[26] = {0}; > > > + struct acpi_table_header *rhct; > > > + acpi_status status; > > > + unsigned int cpu; > > > > > > isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I; > > > isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M; > > > @@ -103,14 +109,36 @@ void __init riscv_fill_hwcap(void) > > > > > > bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > > > > > > - for_each_of_cpu_node(node) { > > > + if (!acpi_disabled) { > > > + status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct); > > > + if (ACPI_FAILURE(status)) > > > + return; > > > + } > > > + > > > + for_each_possible_cpu(cpu) { > > > unsigned long this_hwcap = 0; > > > DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX); > > > const char *temp; > > > > > > - if (of_property_read_string(node, "riscv,isa", &isa)) { > > > - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > > > - continue; > > > + if (acpi_disabled) { > > > + node = of_cpu_device_node_get(cpu); > > > + if (node) { > > > + rc = of_property_read_string(node, "riscv,isa", &isa); > > > > Hmm, after digging in the previous patch, I think this is actually not > > possible to fail? We already validated it when setting up the mask of > > possible cpus, but I think leaving the error handling here makes things > > a lot more obvious. > > > Yeah, do you prefer to merge these patches again since only in this > patch, we change the loop to for_each_possible_cpu() from > for_each_of_cpu_node() which actually makes riscv_of_processor_hartid() > not useful? Yah, all 3 of us mistakenly thought that that was an unrelated cleanup on the last revision, but clearly it is not. Squash it back IMO, sorry for my part in the extra work generated. Cheers, Conor. > > > I'd swear I gave you a (conditional) R-b on v3 though, no? > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
On Tue, Apr 04, 2023 at 10:38:56PM +0100, Conor Dooley wrote: > On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote: > > Initialize the ACPI core for RISC-V during boot. > > > > ACPI tables and interpreter are initialized based on > > the information passed from the firmware and the value of > > the kernel parameter 'acpi'. > > > > With ACPI support added for RISC-V, the kernel parameter 'acpi' > > is also supported on RISC-V. Hence, update the documentation. > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > Acked-by: Conor Dooley <conor.dooley@microchip.com> > > > + /* Parse the ACPI tables for possible boot-time configuration */ > > + acpi_boot_table_init(); > > + if (acpi_disabled) { > > + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) { > > + unflatten_and_copy_device_tree(); > > + } else { > > + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) > > + unflatten_device_tree(); > > + else > > + pr_err("No DTB found in kernel mappings\n"); > > + } > > + } else { > > + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))); > > I'm probably forgetting something, but this seems very non-obvious to > me: > Why are you running early_init_dt_verify() when ACPI is enabled? > I think that one deserves a comment so that next time someone looks at > this (that doesn't live in ACPI land) they've know exactly why this is > like it is. > > Doubly so since this is likely to change with some of Alex's bits moving > the dtb back into the fixmap. > Good question. The kernel creates a tiny DTB even when the FW didn't pass the FDT (ACPI systems). Please see update_fdt(). So, parse_dtb() would have set initial_boot_params to early VA and if we don't call early_init_dt_verify() again with __va, it panics since initial_boot_params can not be translated. Thanks, Sunil
On Wed, Apr 05, 2023 at 08:41:54PM +0530, Sunil V L wrote: > On Tue, Apr 04, 2023 at 10:38:56PM +0100, Conor Dooley wrote: > > On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote: > > > Initialize the ACPI core for RISC-V during boot. > > > > > > ACPI tables and interpreter are initialized based on > > > the information passed from the firmware and the value of > > > the kernel parameter 'acpi'. > > > > > > With ACPI support added for RISC-V, the kernel parameter 'acpi' > > > is also supported on RISC-V. Hence, update the documentation. > > > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > Acked-by: Conor Dooley <conor.dooley@microchip.com> > > > > > + /* Parse the ACPI tables for possible boot-time configuration */ > > > + acpi_boot_table_init(); > > > + if (acpi_disabled) { > > > + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) { > > > + unflatten_and_copy_device_tree(); > > > + } else { > > > + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) > > > + unflatten_device_tree(); > > > + else > > > + pr_err("No DTB found in kernel mappings\n"); > > > + } > > > + } else { > > > + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))); > > > > I'm probably forgetting something, but this seems very non-obvious to > > me: > > Why are you running early_init_dt_verify() when ACPI is enabled? > > I think that one deserves a comment so that next time someone looks at > > this (that doesn't live in ACPI land) they've know exactly why this is > > like it is. > > > > Doubly so since this is likely to change with some of Alex's bits moving > > the dtb back into the fixmap. > > > Good question. The kernel creates a tiny DTB even when the FW didn't > pass the FDT (ACPI systems). Please see update_fdt(). Can you add a comment about this either on-location or in the commit message please? I think this counts as non-obvious behaviour. At least to me it does! Cheers, Conor.
On Wed, Apr 05, 2023 at 03:31:24PM +0100, Conor Dooley wrote: > On Wed, Apr 05, 2023 at 07:05:42PM +0530, Sunil V L wrote: > > On Tue, Apr 04, 2023 at 09:57:19PM +0100, Conor Dooley wrote: > > > On Tue, Apr 04, 2023 at 11:50:27PM +0530, Sunil V L wrote: ... > > > > - if (of_property_read_string(node, "riscv,isa", &isa)) { > > > > - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > > > > - continue; > > > > + if (acpi_disabled) { > > > > + node = of_cpu_device_node_get(cpu); > > > > + if (node) { > > > > + rc = of_property_read_string(node, "riscv,isa", &isa); > > > > > > Hmm, after digging in the previous patch, I think this is actually not > > > possible to fail? We already validated it when setting up the mask of > > > possible cpus, but I think leaving the error handling here makes things > > > a lot more obvious. > > > > > Yeah, do you prefer to merge these patches again since only in this > > patch, we change the loop to for_each_possible_cpu() from > > for_each_of_cpu_node() which actually makes riscv_of_processor_hartid() > > not useful? > > Yah, all 3 of us mistakenly thought that that was an unrelated cleanup > on the last revision, but clearly it is not. > Squash it back IMO, sorry for my part in the extra work generated. Yup, please squash back in. Sorry about that, Sunil! drew
On Tue, Apr 04, 2023 at 11:50:29PM +0530, Sunil V L wrote: > Add support for initializing the RISC-V INTC driver on ACPI > platforms. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > --- > drivers/irqchip/irq-riscv-intc.c | 74 ++++++++++++++++++++++++++------ > 1 file changed, 61 insertions(+), 13 deletions(-) > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > index f229e3e66387..6b476fa356c0 100644 > --- a/drivers/irqchip/irq-riscv-intc.c > +++ b/drivers/irqchip/irq-riscv-intc.c > @@ -6,6 +6,7 @@ > */ > > #define pr_fmt(fmt) "riscv-intc: " fmt > +#include <linux/acpi.h> > #include <linux/atomic.h> > #include <linux/bits.h> > #include <linux/cpu.h> > @@ -112,6 +113,30 @@ static struct fwnode_handle *riscv_intc_hwnode(void) > return intc_domain->fwnode; > } > > +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); > + if (!intc_domain) { > + pr_err("unable to add IRQ domain\n"); > + return -ENXIO; > + } > + > + rc = set_handle_irq(&riscv_intc_irq); > + if (rc) { > + pr_err("failed to set irq handler\n"); > + return rc; > + } > + > + riscv_set_intc_hwnode_fn(riscv_intc_hwnode); > + > + pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > + > + return 0; > +} > + > static int __init riscv_intc_init(struct device_node *node, > struct device_node *parent) > { > @@ -133,24 +158,47 @@ 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); > - if (!intc_domain) { > - pr_err("unable to add IRQ domain\n"); > - return -ENXIO; > - } > - > - rc = set_handle_irq(&riscv_intc_irq); > + rc = riscv_intc_init_common(of_node_to_fwnode(node)); > if (rc) { > - pr_err("failed to set irq handler\n"); > + pr_err("failed to initialize INTC\n"); The ACPI version doesn't output this error when riscv_intc_init_common() fails. It should probably be consistent. Either removing it here, if the errors output within riscv_intc_init_common() are sufficient, or adding it to the ACPI version. > return rc; > } > > - riscv_set_intc_hwnode_fn(riscv_intc_hwnode); > - > - pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > - > return 0; > } > > IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init); > + > +#ifdef CONFIG_ACPI > + > +static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header, > + const unsigned long end) > +{ > + int rc; > + struct fwnode_handle *fn; > + struct acpi_madt_rintc *rintc; > + > + rintc = (struct acpi_madt_rintc *)header; > + > + /* > + * The ACPI MADT will have one INTC for each CPU (or HART) > + * so riscv_intc_acpi_init() function will be called once > + * for each INTC. We only do INTC initialization > + * for the INTC belonging to the boot CPU (or boot HART). > + */ > + if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id()) > + return 0; > + > + fn = irq_domain_alloc_named_fwnode("RISCV-INTC"); > + if (!fn) { > + pr_err("unable to allocate INTC FW node\n"); > + return -ENOMEM; > + } > + > + rc = riscv_intc_init_common(fn); > + return rc; nit: If we don't add the error message here, then rc can be removed and we can just do return riscv_intc_init_common(fn); And, if we remove the error above, then we reduce the return there too. > +} > + > +IRQCHIP_ACPI_DECLARE(riscv_intc, ACPI_MADT_TYPE_RINTC, NULL, > + ACPI_MADT_RINTC_VERSION_V1, riscv_intc_acpi_init); > +#endif > -- > 2.34.1 > Thanks, drew
On Wed, Apr 05, 2023 at 05:48:47PM +0200, Andrew Jones wrote: > On Tue, Apr 04, 2023 at 11:50:29PM +0530, Sunil V L wrote: > > Add support for initializing the RISC-V INTC driver on ACPI > > platforms. > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > --- > > drivers/irqchip/irq-riscv-intc.c | 74 ++++++++++++++++++++++++++------ > > 1 file changed, 61 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c > > index f229e3e66387..6b476fa356c0 100644 > > --- a/drivers/irqchip/irq-riscv-intc.c > > +++ b/drivers/irqchip/irq-riscv-intc.c > > @@ -6,6 +6,7 @@ > > */ > > > > #define pr_fmt(fmt) "riscv-intc: " fmt > > +#include <linux/acpi.h> > > #include <linux/atomic.h> > > #include <linux/bits.h> > > #include <linux/cpu.h> > > @@ -112,6 +113,30 @@ static struct fwnode_handle *riscv_intc_hwnode(void) > > return intc_domain->fwnode; > > } > > > > +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); > > + if (!intc_domain) { > > + pr_err("unable to add IRQ domain\n"); > > + return -ENXIO; > > + } > > + > > + rc = set_handle_irq(&riscv_intc_irq); > > + if (rc) { > > + pr_err("failed to set irq handler\n"); > > + return rc; > > + } > > + > > + riscv_set_intc_hwnode_fn(riscv_intc_hwnode); > > + > > + pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > > + > > + return 0; > > +} > > + > > static int __init riscv_intc_init(struct device_node *node, > > struct device_node *parent) > > { > > @@ -133,24 +158,47 @@ 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); > > - if (!intc_domain) { > > - pr_err("unable to add IRQ domain\n"); > > - return -ENXIO; > > - } > > - > > - rc = set_handle_irq(&riscv_intc_irq); > > + rc = riscv_intc_init_common(of_node_to_fwnode(node)); > > if (rc) { > > - pr_err("failed to set irq handler\n"); > > + pr_err("failed to initialize INTC\n"); > > The ACPI version doesn't output this error when riscv_intc_init_common() > fails. It should probably be consistent. Either removing it here, if the > errors output within riscv_intc_init_common() are sufficient, or adding > it to the ACPI version. > > > return rc; > > } > > > > - riscv_set_intc_hwnode_fn(riscv_intc_hwnode); > > - > > - pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > > - > > return 0; > > } > > > > IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init); > > + > > +#ifdef CONFIG_ACPI > > + > > +static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header, > > + const unsigned long end) > > +{ > > + int rc; > > + struct fwnode_handle *fn; > > + struct acpi_madt_rintc *rintc; > > + > > + rintc = (struct acpi_madt_rintc *)header; > > + > > + /* > > + * The ACPI MADT will have one INTC for each CPU (or HART) > > + * so riscv_intc_acpi_init() function will be called once > > + * for each INTC. We only do INTC initialization > > + * for the INTC belonging to the boot CPU (or boot HART). > > + */ > > + if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id()) > > + return 0; > > + > > + fn = irq_domain_alloc_named_fwnode("RISCV-INTC"); > > + if (!fn) { > > + pr_err("unable to allocate INTC FW node\n"); > > + return -ENOMEM; > > + } > > + > > + rc = riscv_intc_init_common(fn); > > + return rc; > > nit: If we don't add the error message here, then rc can be removed and we > can just do > > return riscv_intc_init_common(fn); > > And, if we remove the error above, then we reduce the return there too. > Make sense. Thanks!. Will update in next revision. Thanks, Sunil
On 2023/4/5 16:16, Arnd Bergmann wrote: > On Tue, Apr 4, 2023, at 20:20, Sunil V L wrote: >> With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in >> allmodconfig build. The gcc tool chain builds this driver removing the >> inline arm64 assembly code. However, clang for RISC-V tries to build >> the arm64 assembly and below error is seen. >> >> drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint >> '+Q' in asm >> "+Q" (*((char __iomem *)fun_base)) >> ^ >> It appears that RISC-V clang is not smart enough to detect >> IS_ENABLED(CONFIG_ARM64) and remove the dead code. >> >> As a workaround, move this check to preprocessing stage which works >> with the RISC-V clang tool chain. >> >> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > Your patch looks correct for this particular problem, but I > see that there are a couple of other issues in the same function: > >> - } >> +#if IS_ENABLED(CONFIG_ARM64) >> + unsigned long tmp0 = 0, tmp1 = 0; >> >> asm volatile("ldp %0, %1, %3\n" >> "stp %0, %1, %2\n" >> @@ -627,6 +623,11 @@ static void qm_mb_write(struct hisi_qm *qm, const >> void *src) >> "+Q" (*((char __iomem *)fun_base)) >> : "Q" (*((char *)src)) >> : "memory"); > > For the arm64 version: > > - the "dmb oshst" barrier needs to come before the stp, not after > it, otherwise there is no guarantee that data written to memory > is visible by the device when the mailbox gets triggered > - The input/output arguments need to be pointers to 128-bit types, > either a struct or a __uint128_t > - this lacks a byteswap on big-endian kernels Sorry for the late reply. - the execution order relies on the data dependency between ldp and stp: load "src" to "tmp0" and "tmp1", then store "tmp0" and "tmp1" to "fun_base"; The "dmb oshst" is used to ensure that the stp instruction has been executed before CPU checking mailbox status. Whether the execution order cannot be guaranteed via data dependency? - The input argument "src" is struct "struct qm_mailbox". - Before call this funcion, the data has been byteswapped. mailbox->w0 = cpu_to_le16((cmd) | ((op) ? 0x1 << QM_MB_OP_SHIFT : 0) | (0x1 << QM_MB_BUSY_SHIFT)); mailbox->queue_num = cpu_to_le16(queue); mailbox->base_l = cpu_to_le32(lower_32_bits(base)); mailbox->base_h = cpu_to_le32(upper_32_bits(base)); mailbox->rsvd = 0; > >> +#else >> + memcpy_toio(fun_base, src, 16); >> + dma_wmb(); >> +#endif > > This version has the same problems, plus the write is not actually > atomic. I wonder if a pair of writeq() calls would just do the > right thing here for both arm64 and others, or possibly a > writeq() followed by a writeq_relaxed() to avoid the extra dmb() > in the middle. > > Arnd > . > We have to do a 128bit atomic write here to trigger a mailbox. The reason is that the PF and related VFs of a hardware cannot write mailbox MMIO at the same time. For this SoC(Kunpeng) which has QM, if the address is 128bit aligned, stp will be atomic. The offset of QM mailbox is 128bit aligned, so it is safe here. Best regards, Weili
On Tue, Apr 11, 2023, at 13:42, Weili Qian wrote: > On 2023/4/5 16:16, Arnd Bergmann wrote: >> On Tue, Apr 4, 2023, at 20:20, Sunil V L wrote: >>> With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in >>> allmodconfig build. The gcc tool chain builds this driver removing the >>> inline arm64 assembly code. However, clang for RISC-V tries to build >>> the arm64 assembly and below error is seen. >>> >>> drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint >>> '+Q' in asm >>> "+Q" (*((char __iomem *)fun_base)) >>> ^ >>> It appears that RISC-V clang is not smart enough to detect >>> IS_ENABLED(CONFIG_ARM64) and remove the dead code. >>> >>> As a workaround, move this check to preprocessing stage which works >>> with the RISC-V clang tool chain. >>> >>> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> >> >> Your patch looks correct for this particular problem, but I >> see that there are a couple of other issues in the same function: >> >>> - } >>> +#if IS_ENABLED(CONFIG_ARM64) >>> + unsigned long tmp0 = 0, tmp1 = 0; >>> >>> asm volatile("ldp %0, %1, %3\n" >>> "stp %0, %1, %2\n" >>> @@ -627,6 +623,11 @@ static void qm_mb_write(struct hisi_qm *qm, const >>> void *src) >>> "+Q" (*((char __iomem *)fun_base)) >>> : "Q" (*((char *)src)) >>> : "memory"); >> >> For the arm64 version: >> >> - the "dmb oshst" barrier needs to come before the stp, not after >> it, otherwise there is no guarantee that data written to memory >> is visible by the device when the mailbox gets triggered >> - The input/output arguments need to be pointers to 128-bit types, >> either a struct or a __uint128_t >> - this lacks a byteswap on big-endian kernels > Sorry for the late reply. > > - the execution order relies on the data dependency between ldp and stp: > load "src" to "tmp0" and "tmp1", then > store "tmp0" and "tmp1" to "fun_base"; Not entirely sure how that data dependency would help serialize the store into the DMA buffer against the device access. The problem here is not the qm_mailbox structure but the data pointed to by the 'u64 base' (e.g. struct qm_eqc *eqc) which may still be in a store buffer waiting to make it to physical memory at the time the mailbox store triggers the DMA from the device. > The "dmb oshst" is used to ensure that the stp instruction has been executed > before CPU checking mailbox status. Whether the execution order > cannot be guaranteed via data dependency? There is no need to have barriers between MMIO operations, they are implicitly serialized already. In this case specifically, the read is even on the same address as the write. Note that the "dmb oshst" does not actually guarantee that the store has made it to the device, as (at least on PCIe semantics) it can be posted, but the read from the same address does guarantee that the write is completed first, and this may be required to ensure that it does not complete after the mutex_unlock(). > - The input argument "src" is struct "struct qm_mailbox". > - Before call this funcion, the data has been byteswapped. > > mailbox->w0 = cpu_to_le16((cmd) | > ((op) ? 0x1 << QM_MB_OP_SHIFT : 0) | > (0x1 << QM_MB_BUSY_SHIFT)); > mailbox->queue_num = cpu_to_le16(queue); > mailbox->base_l = cpu_to_le32(lower_32_bits(base)); > mailbox->base_h = cpu_to_le32(upper_32_bits(base)); > mailbox->rsvd = 0; Right, this bit does look correct. Arnd