| Message ID | 20240110073917.2398826-8-peterlin@andestech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Support Andes PMU extension | expand |
On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin <peterlin@andestech.com> 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. > I don't think we should do that. Any custom implementation that violates the standard RISC-V spec should be an errata not a feature. As per my understanding, a vendor can call an extension custom ISA extension if the same feature is not available in the standard ISA extensions or the mechanism is completely different. It must also not violate any standard spec as well. In this case, a standard sscofpmf is already available. Moreover, both Andes and T-head extensions violate the standard spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which are clearly specified as reserved for standard local interrupts in the AIA specification. Please implementation Andes PMU support as an errata as well similar to T-head > 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> > --- > Changes v1 -> v2: > - New patch > Changes v2 -> v3: > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() > Changes v3 -> v4: > - No change > Changes v4 -> v5: > - Include Guo's Reviewed-by > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD > Changes v5 -> v6: > - Include Conor's Reviewed-by > Changes v6 -> v7: > - No change > --- > arch/riscv/Kconfig.errata | 13 ------------- > arch/riscv/errata/thead/errata.c | 19 ------------------- > arch/riscv/include/asm/errata_list.h | 15 +-------------- > arch/riscv/include/asm/hwcap.h | 1 + > arch/riscv/kernel/cpufeature.c | 1 + > drivers/perf/Kconfig | 13 +++++++++++++ > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- > 7 files changed, 30 insertions(+), 51 deletions(-) > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > index e2c731cfed8c..0d19f47d1018 100644 > --- a/arch/riscv/Kconfig.errata > +++ b/arch/riscv/Kconfig.errata > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO > > If you don't know what to do here, say "Y". > > -config ERRATA_THEAD_PMU > - bool "Apply T-Head PMU errata" > - depends on ERRATA_THEAD && RISCV_PMU_SBI > - default y > - help > - The T-Head C9xx cores implement a PMU overflow extension very > - similar to the core SSCOFPMF extension. > - > - This will apply the overflow errata to handle the non-standard > - behaviour via the regular SBI PMU driver and interface. > - > - If you don't know what to do here, say "Y". > - > endmenu # "CPU errata selection" > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > index 0554ed4bf087..5de5f7209132 100644 > --- a/arch/riscv/errata/thead/errata.c > +++ b/arch/riscv/errata/thead/errata.c > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, > return true; > } > > -static bool errata_probe_pmu(unsigned int stage, > - unsigned long arch_id, unsigned long impid) > -{ > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) > - return false; > - > - /* target-c9xx cores report arch_id and impid as 0 */ > - if (arch_id != 0 || impid != 0) > - return false; > - > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > - return false; > - > - return true; > -} > - > static u32 thead_errata_probe(unsigned int stage, > unsigned long archid, unsigned long impid) > { > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, > if (errata_probe_cmo(stage, archid, impid)) > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); > > - if (errata_probe_pmu(stage, archid, impid)) > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > - > return cpu_req_errata; > } > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > index 4ed21a62158c..9bccc2ba0eb5 100644 > --- a/arch/riscv/include/asm/errata_list.h > +++ b/arch/riscv/include/asm/errata_list.h > @@ -25,8 +25,7 @@ > #ifdef CONFIG_ERRATA_THEAD > #define ERRATA_THEAD_PBMT 0 > #define ERRATA_THEAD_CMO 1 > -#define ERRATA_THEAD_PMU 2 > -#define ERRATA_THEAD_NUMBER 3 > +#define ERRATA_THEAD_NUMBER 2 > #endif > > #ifdef __ASSEMBLY__ > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ > "r"((unsigned long)(_start) + (_size)) \ > : "a0") > > -#define THEAD_C9XX_RV_IRQ_PMU 17 > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > - > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > -asm volatile(ALTERNATIVE( \ > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ > - CONFIG_ERRATA_THEAD_PMU) \ > - : "=r" (__ovl) : \ > - : "memory") > - > #endif /* __ASSEMBLY__ */ > > #endif > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > index 5340f818746b..480f9da7fba7 100644 > --- a/arch/riscv/include/asm/hwcap.h > +++ b/arch/riscv/include/asm/hwcap.h > @@ -80,6 +80,7 @@ > #define RISCV_ISA_EXT_ZFA 71 > #define RISCV_ISA_EXT_ZTSO 72 > #define RISCV_ISA_EXT_ZACAS 73 > +#define RISCV_ISA_EXT_XTHEADPMU 74 > > #define RISCV_ISA_EXT_MAX 128 > #define RISCV_ISA_EXT_INVALID U32_MAX > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index e32591e9da90..4aded5bf8fc3 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), > }; > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig > index 273d67ecf6d2..6cef15ec7c25 100644 > --- a/drivers/perf/Kconfig > +++ b/drivers/perf/Kconfig > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI > full perf feature support i.e. counter overflow, privilege mode > filtering, counter configuration. > > +config THEAD_CUSTOM_PMU > + bool "T-Head custom PMU support" > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI > + default y > + help > + The T-Head C9xx cores implement a PMU overflow extension very > + similar to the core SSCOFPMF extension. > + > + This will patch the overflow CSR and handle the non-standard > + behaviour via the regular SBI PMU driver and interface. > + > + If you don't know what to do here, say "Y". > + > config ARM_PMU_ACPI > depends on ARM_PMU && ACPI > def_bool y > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > index 2edbc37abadf..31ca79846399 100644 > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -20,10 +20,21 @@ > #include <linux/cpu_pm.h> > #include <linux/sched/clock.h> > > -#include <asm/errata_list.h> > #include <asm/sbi.h> > #include <asm/cpufeature.h> > > +#define THEAD_C9XX_RV_IRQ_PMU 17 > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > + > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > +asm volatile(ALTERNATIVE( \ > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > + 0, RISCV_ISA_EXT_XTHEADPMU, \ > + CONFIG_THEAD_CUSTOM_PMU) \ > + : "=r" (__ovl) : \ > + : "memory") > + > #define SYSCTL_NO_USER_ACCESS 0 > #define SYSCTL_USER_ACCESS 1 > #define SYSCTL_LEGACY 2 > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { > riscv_pmu_irq_num = RV_IRQ_PMU; > riscv_pmu_use_irq = true; > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && > - riscv_cached_marchid(0) == 0 && > - riscv_cached_mimpid(0) == 0) { > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; > riscv_pmu_use_irq = true; > } > -- > 2.34.1 > -- Regards, Atish
On Tue, Jan 16, 2024 at 12:55:41PM -0800, Atish Patra wrote: > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin > <peterlin@andestech.com> 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. > > > > I don't think we should do that. Any custom implementation that > violates the standard RISC-V spec should > be an errata not a feature. > As per my understanding, a vendor can call an extension custom ISA > extension if the same feature is not available > in the standard ISA extensions or the mechanism is completely > different. It must also not violate any standard spec as well. > > In this case, a standard sscofpmf is already available. Moreover, both > Andes and T-head extensions violate the standard > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which > are clearly specified as reserved for standard local interrupts > in the AIA specification. I disagree with you here. The Andes implementation predated (IIRC that is what was said in replies to an earlier revision) the Sscofpmf extension and certainly predates the AIA specification. I would be on board with this line of thinking if someone comes along in 2030 with "Zbb but with this one tweak" or where something flies entirely in the face of the standard (like the IOCP cache stuff). The relevant section in the AIA spec seems to say: | Interrupt causes that are standardized by the Privileged Architecture | have major identities in the range 0–15, while numbers 16 and higher are | officially available for platform standards or for custom use. | The Advanced Interrupt Architecture claims further authority over | identity numbers in the ranges 16–23 and 32–47, leaving numbers in the | range 24–31 and all major identities 48 and higher still free for custom | use. I don't see how that can be problematic given the Andes implemtation dates from before AIA was a thing. It would be silly to say that because an optional extension later came along and took over something previously allowed for indiscriminate custom use, that support for that custom extension is not permitted. I may well be missing something here though, you clearly know these specs better than I do, but from what I have read I disagree. > Please implementation Andes PMU support as an errata as well similar to T-head I certainly _do not_ want to see things like this detected via lookup tables of marchid and co in the kernel unless it is absolutely required. We have standard probing mechanisms for feature detection (because to me this _is_ a feature) and they should be used. Additionally, we define what entries in the DT properties mean, and if it is convenient to put "psuedo" extensions into the DT, then we should do so. Getting away from being tied to what RVI decrees was one of the goals of the new properties after all, so that we could use a standard mechanism of DT probing for things like this. Thanks, Conor. > > 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> > > --- > > Changes v1 -> v2: > > - New patch > > Changes v2 -> v3: > > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() > > Changes v3 -> v4: > > - No change > > Changes v4 -> v5: > > - Include Guo's Reviewed-by > > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD > > Changes v5 -> v6: > > - Include Conor's Reviewed-by > > Changes v6 -> v7: > > - No change > > --- > > arch/riscv/Kconfig.errata | 13 ------------- > > arch/riscv/errata/thead/errata.c | 19 ------------------- > > arch/riscv/include/asm/errata_list.h | 15 +-------------- > > arch/riscv/include/asm/hwcap.h | 1 + > > arch/riscv/kernel/cpufeature.c | 1 + > > drivers/perf/Kconfig | 13 +++++++++++++ > > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- > > 7 files changed, 30 insertions(+), 51 deletions(-) > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > index e2c731cfed8c..0d19f47d1018 100644 > > --- a/arch/riscv/Kconfig.errata > > +++ b/arch/riscv/Kconfig.errata > > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO > > > > If you don't know what to do here, say "Y". > > > > -config ERRATA_THEAD_PMU > > - bool "Apply T-Head PMU errata" > > - depends on ERRATA_THEAD && RISCV_PMU_SBI > > - default y > > - help > > - The T-Head C9xx cores implement a PMU overflow extension very > > - similar to the core SSCOFPMF extension. > > - > > - This will apply the overflow errata to handle the non-standard > > - behaviour via the regular SBI PMU driver and interface. > > - > > - If you don't know what to do here, say "Y". > > - > > endmenu # "CPU errata selection" > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > index 0554ed4bf087..5de5f7209132 100644 > > --- a/arch/riscv/errata/thead/errata.c > > +++ b/arch/riscv/errata/thead/errata.c > > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, > > return true; > > } > > > > -static bool errata_probe_pmu(unsigned int stage, > > - unsigned long arch_id, unsigned long impid) > > -{ > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) > > - return false; > > - > > - /* target-c9xx cores report arch_id and impid as 0 */ > > - if (arch_id != 0 || impid != 0) > > - return false; > > - > > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > - return false; > > - > > - return true; > > -} > > - > > static u32 thead_errata_probe(unsigned int stage, > > unsigned long archid, unsigned long impid) > > { > > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, > > if (errata_probe_cmo(stage, archid, impid)) > > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); > > > > - if (errata_probe_pmu(stage, archid, impid)) > > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > > - > > return cpu_req_errata; > > } > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > index 4ed21a62158c..9bccc2ba0eb5 100644 > > --- a/arch/riscv/include/asm/errata_list.h > > +++ b/arch/riscv/include/asm/errata_list.h > > @@ -25,8 +25,7 @@ > > #ifdef CONFIG_ERRATA_THEAD > > #define ERRATA_THEAD_PBMT 0 > > #define ERRATA_THEAD_CMO 1 > > -#define ERRATA_THEAD_PMU 2 > > -#define ERRATA_THEAD_NUMBER 3 > > +#define ERRATA_THEAD_NUMBER 2 > > #endif > > > > #ifdef __ASSEMBLY__ > > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ > > "r"((unsigned long)(_start) + (_size)) \ > > : "a0") > > > > -#define THEAD_C9XX_RV_IRQ_PMU 17 > > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > - > > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > -asm volatile(ALTERNATIVE( \ > > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ > > - CONFIG_ERRATA_THEAD_PMU) \ > > - : "=r" (__ovl) : \ > > - : "memory") > > - > > #endif /* __ASSEMBLY__ */ > > > > #endif > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > index 5340f818746b..480f9da7fba7 100644 > > --- a/arch/riscv/include/asm/hwcap.h > > +++ b/arch/riscv/include/asm/hwcap.h > > @@ -80,6 +80,7 @@ > > #define RISCV_ISA_EXT_ZFA 71 > > #define RISCV_ISA_EXT_ZTSO 72 > > #define RISCV_ISA_EXT_ZACAS 73 > > +#define RISCV_ISA_EXT_XTHEADPMU 74 > > > > #define RISCV_ISA_EXT_MAX 128 > > #define RISCV_ISA_EXT_INVALID U32_MAX > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index e32591e9da90..4aded5bf8fc3 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), > > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), > > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), > > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), > > }; > > > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig > > index 273d67ecf6d2..6cef15ec7c25 100644 > > --- a/drivers/perf/Kconfig > > +++ b/drivers/perf/Kconfig > > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI > > full perf feature support i.e. counter overflow, privilege mode > > filtering, counter configuration. > > > > +config THEAD_CUSTOM_PMU > > + bool "T-Head custom PMU support" > > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI > > + default y > > + help > > + The T-Head C9xx cores implement a PMU overflow extension very > > + similar to the core SSCOFPMF extension. > > + > > + This will patch the overflow CSR and handle the non-standard > > + behaviour via the regular SBI PMU driver and interface. > > + > > + If you don't know what to do here, say "Y". > > + > > config ARM_PMU_ACPI > > depends on ARM_PMU && ACPI > > def_bool y > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > index 2edbc37abadf..31ca79846399 100644 > > --- a/drivers/perf/riscv_pmu_sbi.c > > +++ b/drivers/perf/riscv_pmu_sbi.c > > @@ -20,10 +20,21 @@ > > #include <linux/cpu_pm.h> > > #include <linux/sched/clock.h> > > > > -#include <asm/errata_list.h> > > #include <asm/sbi.h> > > #include <asm/cpufeature.h> > > > > +#define THEAD_C9XX_RV_IRQ_PMU 17 > > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > + > > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > +asm volatile(ALTERNATIVE( \ > > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > + 0, RISCV_ISA_EXT_XTHEADPMU, \ > > + CONFIG_THEAD_CUSTOM_PMU) \ > > + : "=r" (__ovl) : \ > > + : "memory") > > + > > #define SYSCTL_NO_USER_ACCESS 0 > > #define SYSCTL_USER_ACCESS 1 > > #define SYSCTL_LEGACY 2 > > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde > > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { > > riscv_pmu_irq_num = RV_IRQ_PMU; > > riscv_pmu_use_irq = true; > > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && > > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && > > - riscv_cached_marchid(0) == 0 && > > - riscv_cached_mimpid(0) == 0) { > > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && > > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { > > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; > > riscv_pmu_use_irq = true; > > } > > -- > > 2.34.1 > > > > > -- > Regards, > Atish
On Wed, Jan 17, 2024 at 2:26 AM Atish Patra <atishp@atishpatra.org> wrote: > > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin > <peterlin@andestech.com> 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. > > > > I don't think we should do that. Any custom implementation that > violates the standard RISC-V spec should > be an errata not a feature. > As per my understanding, a vendor can call an extension custom ISA > extension if the same feature is not available > in the standard ISA extensions or the mechanism is completely > different. It must also not violate any standard spec as well. I agree with Atish here. There is a well defined encoding space for custom extensions. If a custom extension spills over to standard encoding space then it should be treated as an errata and not a proper custom extension. > > In this case, a standard sscofpmf is already available. Moreover, both > Andes and T-head extensions violate the standard > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which > are clearly specified as reserved for standard local interrupts > in the AIA specification. > > Please implementation Andes PMU support as an errata as well similar to T-head > > > > 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. T-Head has many violations of using standard encoding space. I don't see why this series should be touching T-Head erratas. If Andes custom PMU CSRs are defined in custom encoding space then Andes PMU can be treated as proper custom extension. Regards, Anup > > > > 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> > > --- > > Changes v1 -> v2: > > - New patch > > Changes v2 -> v3: > > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() > > Changes v3 -> v4: > > - No change > > Changes v4 -> v5: > > - Include Guo's Reviewed-by > > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD > > Changes v5 -> v6: > > - Include Conor's Reviewed-by > > Changes v6 -> v7: > > - No change > > --- > > arch/riscv/Kconfig.errata | 13 ------------- > > arch/riscv/errata/thead/errata.c | 19 ------------------- > > arch/riscv/include/asm/errata_list.h | 15 +-------------- > > arch/riscv/include/asm/hwcap.h | 1 + > > arch/riscv/kernel/cpufeature.c | 1 + > > drivers/perf/Kconfig | 13 +++++++++++++ > > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- > > 7 files changed, 30 insertions(+), 51 deletions(-) > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > index e2c731cfed8c..0d19f47d1018 100644 > > --- a/arch/riscv/Kconfig.errata > > +++ b/arch/riscv/Kconfig.errata > > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO > > > > If you don't know what to do here, say "Y". > > > > -config ERRATA_THEAD_PMU > > - bool "Apply T-Head PMU errata" > > - depends on ERRATA_THEAD && RISCV_PMU_SBI > > - default y > > - help > > - The T-Head C9xx cores implement a PMU overflow extension very > > - similar to the core SSCOFPMF extension. > > - > > - This will apply the overflow errata to handle the non-standard > > - behaviour via the regular SBI PMU driver and interface. > > - > > - If you don't know what to do here, say "Y". > > - > > endmenu # "CPU errata selection" > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > index 0554ed4bf087..5de5f7209132 100644 > > --- a/arch/riscv/errata/thead/errata.c > > +++ b/arch/riscv/errata/thead/errata.c > > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, > > return true; > > } > > > > -static bool errata_probe_pmu(unsigned int stage, > > - unsigned long arch_id, unsigned long impid) > > -{ > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) > > - return false; > > - > > - /* target-c9xx cores report arch_id and impid as 0 */ > > - if (arch_id != 0 || impid != 0) > > - return false; > > - > > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > - return false; > > - > > - return true; > > -} > > - > > static u32 thead_errata_probe(unsigned int stage, > > unsigned long archid, unsigned long impid) > > { > > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, > > if (errata_probe_cmo(stage, archid, impid)) > > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); > > > > - if (errata_probe_pmu(stage, archid, impid)) > > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > > - > > return cpu_req_errata; > > } > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > index 4ed21a62158c..9bccc2ba0eb5 100644 > > --- a/arch/riscv/include/asm/errata_list.h > > +++ b/arch/riscv/include/asm/errata_list.h > > @@ -25,8 +25,7 @@ > > #ifdef CONFIG_ERRATA_THEAD > > #define ERRATA_THEAD_PBMT 0 > > #define ERRATA_THEAD_CMO 1 > > -#define ERRATA_THEAD_PMU 2 > > -#define ERRATA_THEAD_NUMBER 3 > > +#define ERRATA_THEAD_NUMBER 2 > > #endif > > > > #ifdef __ASSEMBLY__ > > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ > > "r"((unsigned long)(_start) + (_size)) \ > > : "a0") > > > > -#define THEAD_C9XX_RV_IRQ_PMU 17 > > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > - > > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > -asm volatile(ALTERNATIVE( \ > > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ > > - CONFIG_ERRATA_THEAD_PMU) \ > > - : "=r" (__ovl) : \ > > - : "memory") > > - > > #endif /* __ASSEMBLY__ */ > > > > #endif > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > index 5340f818746b..480f9da7fba7 100644 > > --- a/arch/riscv/include/asm/hwcap.h > > +++ b/arch/riscv/include/asm/hwcap.h > > @@ -80,6 +80,7 @@ > > #define RISCV_ISA_EXT_ZFA 71 > > #define RISCV_ISA_EXT_ZTSO 72 > > #define RISCV_ISA_EXT_ZACAS 73 > > +#define RISCV_ISA_EXT_XTHEADPMU 74 > > > > #define RISCV_ISA_EXT_MAX 128 > > #define RISCV_ISA_EXT_INVALID U32_MAX > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index e32591e9da90..4aded5bf8fc3 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), > > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), > > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), > > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), > > }; > > > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig > > index 273d67ecf6d2..6cef15ec7c25 100644 > > --- a/drivers/perf/Kconfig > > +++ b/drivers/perf/Kconfig > > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI > > full perf feature support i.e. counter overflow, privilege mode > > filtering, counter configuration. > > > > +config THEAD_CUSTOM_PMU > > + bool "T-Head custom PMU support" > > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI > > + default y > > + help > > + The T-Head C9xx cores implement a PMU overflow extension very > > + similar to the core SSCOFPMF extension. > > + > > + This will patch the overflow CSR and handle the non-standard > > + behaviour via the regular SBI PMU driver and interface. > > + > > + If you don't know what to do here, say "Y". > > + > > config ARM_PMU_ACPI > > depends on ARM_PMU && ACPI > > def_bool y > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > index 2edbc37abadf..31ca79846399 100644 > > --- a/drivers/perf/riscv_pmu_sbi.c > > +++ b/drivers/perf/riscv_pmu_sbi.c > > @@ -20,10 +20,21 @@ > > #include <linux/cpu_pm.h> > > #include <linux/sched/clock.h> > > > > -#include <asm/errata_list.h> > > #include <asm/sbi.h> > > #include <asm/cpufeature.h> > > > > +#define THEAD_C9XX_RV_IRQ_PMU 17 > > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > + > > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > +asm volatile(ALTERNATIVE( \ > > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > + 0, RISCV_ISA_EXT_XTHEADPMU, \ > > + CONFIG_THEAD_CUSTOM_PMU) \ > > + : "=r" (__ovl) : \ > > + : "memory") > > + > > #define SYSCTL_NO_USER_ACCESS 0 > > #define SYSCTL_USER_ACCESS 1 > > #define SYSCTL_LEGACY 2 > > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde > > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { > > riscv_pmu_irq_num = RV_IRQ_PMU; > > riscv_pmu_use_irq = true; > > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && > > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && > > - riscv_cached_marchid(0) == 0 && > > - riscv_cached_mimpid(0) == 0) { > > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && > > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { > > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; > > riscv_pmu_use_irq = true; > > } > > -- > > 2.34.1 > > > > > -- > Regards, > Atish > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Jan 16, 2024 at 4:16 PM Conor Dooley <conor@kernel.org> wrote: > > On Tue, Jan 16, 2024 at 12:55:41PM -0800, Atish Patra wrote: > > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin > > <peterlin@andestech.com> 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. > > > > > > > I don't think we should do that. Any custom implementation that > > violates the standard RISC-V spec should > > be an errata not a feature. > > As per my understanding, a vendor can call an extension custom ISA > > extension if the same feature is not available > > in the standard ISA extensions or the mechanism is completely > > different. It must also not violate any standard spec as well. > > > > In this case, a standard sscofpmf is already available. Moreover, both > > Andes and T-head extensions violate the standard > > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which > > are clearly specified as reserved for standard local interrupts > > in the AIA specification. > > I disagree with you here. The Andes implementation predated (IIRC that > is what was said in replies to an earlier revision) the Sscofpmf > extension and certainly predates the AIA specification. I would be on > board with this line of thinking if someone comes along in 2030 with > "Zbb but with this one tweak" or where something flies entirely in the > face of the standard (like the IOCP cache stuff). The relevant section > in the AIA spec seems to say: > | Interrupt causes that are standardized by the Privileged Architecture > | have major identities in the range 0–15, while numbers 16 and higher are > | officially available for platform standards or for custom use. > | The Advanced Interrupt Architecture claims further authority over > | identity numbers in the ranges 16–23 and 32–47, leaving numbers in the > | range 24–31 and all major identities 48 and higher still free for custom > | use. > I don't see how that can be problematic given the Andes implemtation > dates from before AIA was a thing. It would be silly to say that because > an optional extension later came along and took over something previously > allowed for indiscriminate custom use, that support for that custom > extension is not permitted. > AIA is not some optional extension. It defines the RISC-V interrupt architecture going forward and will be the default implementation in the future. IMO, this will be a slippery slope if we start supporting custom implementations to override interrupt ID definitions via custom cpu features. T-head implementation works perfectly fine as an errata and I don't understand why there is a push to make it a cpu feature. We should try to improve the ecosystem for future platforms rather than bending backwards to support older implementations. I understand the push to brand this as a custom extension if current errata/alternative can't support it. But I don't think that's the case here though. Please correct me if I am wrong. > I may well be missing something here though, you clearly know these > specs better than I do, but from what I have read I disagree. > > > Please implementation Andes PMU support as an errata as well similar to T-head > > I certainly _do not_ want to see things like this detected via lookup > tables of marchid and co in the kernel unless it is absolutely required. > We have standard probing mechanisms for feature detection (because to me > this _is_ a feature) and they should be used. Additionally, we define what > entries in the DT properties mean, and if it is convenient to put > "psuedo" extensions into the DT, then we should do so. Getting away from > being tied to what RVI decrees was one of the goals of the new > properties after all, so that we could use a standard mechanism of DT > probing for things like this. > Yes. That's a perfectly valid mechanism for actual custom/vendor ISA extensions. I'm sure we'll have many of those, which will be leveraged via pseudo extensions in the DT. However, these shouldn't co-exist with standard ISA extensions in the namespace in riscv_isa_ext and/or hwprobe. The vendor-specific extensions should be defined under a vendor-specific namespace. This was another issue with this series as well. I didn't raise this topic earlier because I don't think overriding interrupt identities qualifies for a custom ISA extension > Thanks, > Conor. > > > > 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> > > > --- > > > Changes v1 -> v2: > > > - New patch > > > Changes v2 -> v3: > > > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() > > > Changes v3 -> v4: > > > - No change > > > Changes v4 -> v5: > > > - Include Guo's Reviewed-by > > > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD > > > Changes v5 -> v6: > > > - Include Conor's Reviewed-by > > > Changes v6 -> v7: > > > - No change > > > --- > > > arch/riscv/Kconfig.errata | 13 ------------- > > > arch/riscv/errata/thead/errata.c | 19 ------------------- > > > arch/riscv/include/asm/errata_list.h | 15 +-------------- > > > arch/riscv/include/asm/hwcap.h | 1 + > > > arch/riscv/kernel/cpufeature.c | 1 + > > > drivers/perf/Kconfig | 13 +++++++++++++ > > > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- > > > 7 files changed, 30 insertions(+), 51 deletions(-) > > > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > > index e2c731cfed8c..0d19f47d1018 100644 > > > --- a/arch/riscv/Kconfig.errata > > > +++ b/arch/riscv/Kconfig.errata > > > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO > > > > > > If you don't know what to do here, say "Y". > > > > > > -config ERRATA_THEAD_PMU > > > - bool "Apply T-Head PMU errata" > > > - depends on ERRATA_THEAD && RISCV_PMU_SBI > > > - default y > > > - help > > > - The T-Head C9xx cores implement a PMU overflow extension very > > > - similar to the core SSCOFPMF extension. > > > - > > > - This will apply the overflow errata to handle the non-standard > > > - behaviour via the regular SBI PMU driver and interface. > > > - > > > - If you don't know what to do here, say "Y". > > > - > > > endmenu # "CPU errata selection" > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > index 0554ed4bf087..5de5f7209132 100644 > > > --- a/arch/riscv/errata/thead/errata.c > > > +++ b/arch/riscv/errata/thead/errata.c > > > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, > > > return true; > > > } > > > > > > -static bool errata_probe_pmu(unsigned int stage, > > > - unsigned long arch_id, unsigned long impid) > > > -{ > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) > > > - return false; > > > - > > > - /* target-c9xx cores report arch_id and impid as 0 */ > > > - if (arch_id != 0 || impid != 0) > > > - return false; > > > - > > > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > > - return false; > > > - > > > - return true; > > > -} > > > - > > > static u32 thead_errata_probe(unsigned int stage, > > > unsigned long archid, unsigned long impid) > > > { > > > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, > > > if (errata_probe_cmo(stage, archid, impid)) > > > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); > > > > > > - if (errata_probe_pmu(stage, archid, impid)) > > > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > > > - > > > return cpu_req_errata; > > > } > > > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > > index 4ed21a62158c..9bccc2ba0eb5 100644 > > > --- a/arch/riscv/include/asm/errata_list.h > > > +++ b/arch/riscv/include/asm/errata_list.h > > > @@ -25,8 +25,7 @@ > > > #ifdef CONFIG_ERRATA_THEAD > > > #define ERRATA_THEAD_PBMT 0 > > > #define ERRATA_THEAD_CMO 1 > > > -#define ERRATA_THEAD_PMU 2 > > > -#define ERRATA_THEAD_NUMBER 3 > > > +#define ERRATA_THEAD_NUMBER 2 > > > #endif > > > > > > #ifdef __ASSEMBLY__ > > > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ > > > "r"((unsigned long)(_start) + (_size)) \ > > > : "a0") > > > > > > -#define THEAD_C9XX_RV_IRQ_PMU 17 > > > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > - > > > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > -asm volatile(ALTERNATIVE( \ > > > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ > > > - CONFIG_ERRATA_THEAD_PMU) \ > > > - : "=r" (__ovl) : \ > > > - : "memory") > > > - > > > #endif /* __ASSEMBLY__ */ > > > > > > #endif > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > index 5340f818746b..480f9da7fba7 100644 > > > --- a/arch/riscv/include/asm/hwcap.h > > > +++ b/arch/riscv/include/asm/hwcap.h > > > @@ -80,6 +80,7 @@ > > > #define RISCV_ISA_EXT_ZFA 71 > > > #define RISCV_ISA_EXT_ZTSO 72 > > > #define RISCV_ISA_EXT_ZACAS 73 > > > +#define RISCV_ISA_EXT_XTHEADPMU 74 > > > > > > #define RISCV_ISA_EXT_MAX 128 > > > #define RISCV_ISA_EXT_INVALID U32_MAX > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > index e32591e9da90..4aded5bf8fc3 100644 > > > --- a/arch/riscv/kernel/cpufeature.c > > > +++ b/arch/riscv/kernel/cpufeature.c > > > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), > > > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), > > > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), > > > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), > > > }; > > > > > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig > > > index 273d67ecf6d2..6cef15ec7c25 100644 > > > --- a/drivers/perf/Kconfig > > > +++ b/drivers/perf/Kconfig > > > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI > > > full perf feature support i.e. counter overflow, privilege mode > > > filtering, counter configuration. > > > > > > +config THEAD_CUSTOM_PMU > > > + bool "T-Head custom PMU support" > > > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI > > > + default y > > > + help > > > + The T-Head C9xx cores implement a PMU overflow extension very > > > + similar to the core SSCOFPMF extension. > > > + > > > + This will patch the overflow CSR and handle the non-standard > > > + behaviour via the regular SBI PMU driver and interface. > > > + > > > + If you don't know what to do here, say "Y". > > > + > > > config ARM_PMU_ACPI > > > depends on ARM_PMU && ACPI > > > def_bool y > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > > index 2edbc37abadf..31ca79846399 100644 > > > --- a/drivers/perf/riscv_pmu_sbi.c > > > +++ b/drivers/perf/riscv_pmu_sbi.c > > > @@ -20,10 +20,21 @@ > > > #include <linux/cpu_pm.h> > > > #include <linux/sched/clock.h> > > > > > > -#include <asm/errata_list.h> > > > #include <asm/sbi.h> > > > #include <asm/cpufeature.h> > > > > > > +#define THEAD_C9XX_RV_IRQ_PMU 17 > > > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > + > > > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > +asm volatile(ALTERNATIVE( \ > > > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > + 0, RISCV_ISA_EXT_XTHEADPMU, \ > > > + CONFIG_THEAD_CUSTOM_PMU) \ > > > + : "=r" (__ovl) : \ > > > + : "memory") > > > + > > > #define SYSCTL_NO_USER_ACCESS 0 > > > #define SYSCTL_USER_ACCESS 1 > > > #define SYSCTL_LEGACY 2 > > > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde > > > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { > > > riscv_pmu_irq_num = RV_IRQ_PMU; > > > riscv_pmu_use_irq = true; > > > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && > > > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && > > > - riscv_cached_marchid(0) == 0 && > > > - riscv_cached_mimpid(0) == 0) { > > > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && > > > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { > > > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; > > > riscv_pmu_use_irq = true; > > > } > > > -- > > > 2.34.1 > > > > > > > > > -- > > Regards, > > Atish
On Tue, Jan 16, 2024 at 7:35 PM Anup Patel <apatel@ventanamicro.com> wrote: > > On Wed, Jan 17, 2024 at 2:26 AM Atish Patra <atishp@atishpatra.org> wrote: > > > > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin > > <peterlin@andestech.com> 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. > > > > > > > I don't think we should do that. Any custom implementation that > > violates the standard RISC-V spec should > > be an errata not a feature. > > As per my understanding, a vendor can call an extension custom ISA > > extension if the same feature is not available > > in the standard ISA extensions or the mechanism is completely > > different. It must also not violate any standard spec as well. > > I agree with Atish here. There is a well defined encoding space for > custom extensions. > > If a custom extension spills over to standard encoding space then > it should be treated as an errata and not a proper custom extension. > > > > > In this case, a standard sscofpmf is already available. Moreover, both > > Andes and T-head extensions violate the standard > > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which > > are clearly specified as reserved for standard local interrupts > > in the AIA specification. > > > > Please implementation Andes PMU support as an errata as well similar to T-head > > > > > > > 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. > > T-Head has many violations of using standard encoding space. I don't see > why this series should be touching T-Head erratas. > > If Andes custom PMU CSRs are defined in custom encoding space then > Andes PMU can be treated as proper custom extension. > The PMU CSRs are in custom extension space. However, the interrupt ID(18) violates the standard encoding space defined in AIA. > Regards, > Anup > > > > > > > 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> > > > --- > > > Changes v1 -> v2: > > > - New patch > > > Changes v2 -> v3: > > > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() > > > Changes v3 -> v4: > > > - No change > > > Changes v4 -> v5: > > > - Include Guo's Reviewed-by > > > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD > > > Changes v5 -> v6: > > > - Include Conor's Reviewed-by > > > Changes v6 -> v7: > > > - No change > > > --- > > > arch/riscv/Kconfig.errata | 13 ------------- > > > arch/riscv/errata/thead/errata.c | 19 ------------------- > > > arch/riscv/include/asm/errata_list.h | 15 +-------------- > > > arch/riscv/include/asm/hwcap.h | 1 + > > > arch/riscv/kernel/cpufeature.c | 1 + > > > drivers/perf/Kconfig | 13 +++++++++++++ > > > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- > > > 7 files changed, 30 insertions(+), 51 deletions(-) > > > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > > index e2c731cfed8c..0d19f47d1018 100644 > > > --- a/arch/riscv/Kconfig.errata > > > +++ b/arch/riscv/Kconfig.errata > > > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO > > > > > > If you don't know what to do here, say "Y". > > > > > > -config ERRATA_THEAD_PMU > > > - bool "Apply T-Head PMU errata" > > > - depends on ERRATA_THEAD && RISCV_PMU_SBI > > > - default y > > > - help > > > - The T-Head C9xx cores implement a PMU overflow extension very > > > - similar to the core SSCOFPMF extension. > > > - > > > - This will apply the overflow errata to handle the non-standard > > > - behaviour via the regular SBI PMU driver and interface. > > > - > > > - If you don't know what to do here, say "Y". > > > - > > > endmenu # "CPU errata selection" > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > index 0554ed4bf087..5de5f7209132 100644 > > > --- a/arch/riscv/errata/thead/errata.c > > > +++ b/arch/riscv/errata/thead/errata.c > > > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, > > > return true; > > > } > > > > > > -static bool errata_probe_pmu(unsigned int stage, > > > - unsigned long arch_id, unsigned long impid) > > > -{ > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) > > > - return false; > > > - > > > - /* target-c9xx cores report arch_id and impid as 0 */ > > > - if (arch_id != 0 || impid != 0) > > > - return false; > > > - > > > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > > - return false; > > > - > > > - return true; > > > -} > > > - > > > static u32 thead_errata_probe(unsigned int stage, > > > unsigned long archid, unsigned long impid) > > > { > > > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, > > > if (errata_probe_cmo(stage, archid, impid)) > > > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); > > > > > > - if (errata_probe_pmu(stage, archid, impid)) > > > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > > > - > > > return cpu_req_errata; > > > } > > > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > > index 4ed21a62158c..9bccc2ba0eb5 100644 > > > --- a/arch/riscv/include/asm/errata_list.h > > > +++ b/arch/riscv/include/asm/errata_list.h > > > @@ -25,8 +25,7 @@ > > > #ifdef CONFIG_ERRATA_THEAD > > > #define ERRATA_THEAD_PBMT 0 > > > #define ERRATA_THEAD_CMO 1 > > > -#define ERRATA_THEAD_PMU 2 > > > -#define ERRATA_THEAD_NUMBER 3 > > > +#define ERRATA_THEAD_NUMBER 2 > > > #endif > > > > > > #ifdef __ASSEMBLY__ > > > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ > > > "r"((unsigned long)(_start) + (_size)) \ > > > : "a0") > > > > > > -#define THEAD_C9XX_RV_IRQ_PMU 17 > > > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > - > > > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > -asm volatile(ALTERNATIVE( \ > > > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ > > > - CONFIG_ERRATA_THEAD_PMU) \ > > > - : "=r" (__ovl) : \ > > > - : "memory") > > > - > > > #endif /* __ASSEMBLY__ */ > > > > > > #endif > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > index 5340f818746b..480f9da7fba7 100644 > > > --- a/arch/riscv/include/asm/hwcap.h > > > +++ b/arch/riscv/include/asm/hwcap.h > > > @@ -80,6 +80,7 @@ > > > #define RISCV_ISA_EXT_ZFA 71 > > > #define RISCV_ISA_EXT_ZTSO 72 > > > #define RISCV_ISA_EXT_ZACAS 73 > > > +#define RISCV_ISA_EXT_XTHEADPMU 74 > > > > > > #define RISCV_ISA_EXT_MAX 128 > > > #define RISCV_ISA_EXT_INVALID U32_MAX > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > index e32591e9da90..4aded5bf8fc3 100644 > > > --- a/arch/riscv/kernel/cpufeature.c > > > +++ b/arch/riscv/kernel/cpufeature.c > > > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), > > > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), > > > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), > > > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), > > > }; > > > > > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig > > > index 273d67ecf6d2..6cef15ec7c25 100644 > > > --- a/drivers/perf/Kconfig > > > +++ b/drivers/perf/Kconfig > > > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI > > > full perf feature support i.e. counter overflow, privilege mode > > > filtering, counter configuration. > > > > > > +config THEAD_CUSTOM_PMU > > > + bool "T-Head custom PMU support" > > > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI > > > + default y > > > + help > > > + The T-Head C9xx cores implement a PMU overflow extension very > > > + similar to the core SSCOFPMF extension. > > > + > > > + This will patch the overflow CSR and handle the non-standard > > > + behaviour via the regular SBI PMU driver and interface. > > > + > > > + If you don't know what to do here, say "Y". > > > + > > > config ARM_PMU_ACPI > > > depends on ARM_PMU && ACPI > > > def_bool y > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > > index 2edbc37abadf..31ca79846399 100644 > > > --- a/drivers/perf/riscv_pmu_sbi.c > > > +++ b/drivers/perf/riscv_pmu_sbi.c > > > @@ -20,10 +20,21 @@ > > > #include <linux/cpu_pm.h> > > > #include <linux/sched/clock.h> > > > > > > -#include <asm/errata_list.h> > > > #include <asm/sbi.h> > > > #include <asm/cpufeature.h> > > > > > > +#define THEAD_C9XX_RV_IRQ_PMU 17 > > > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > + > > > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > +asm volatile(ALTERNATIVE( \ > > > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > + 0, RISCV_ISA_EXT_XTHEADPMU, \ > > > + CONFIG_THEAD_CUSTOM_PMU) \ > > > + : "=r" (__ovl) : \ > > > + : "memory") > > > + > > > #define SYSCTL_NO_USER_ACCESS 0 > > > #define SYSCTL_USER_ACCESS 1 > > > #define SYSCTL_LEGACY 2 > > > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde > > > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { > > > riscv_pmu_irq_num = RV_IRQ_PMU; > > > riscv_pmu_use_irq = true; > > > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && > > > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && > > > - riscv_cached_marchid(0) == 0 && > > > - riscv_cached_mimpid(0) == 0) { > > > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && > > > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { > > > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; > > > riscv_pmu_use_irq = true; > > > } > > > -- > > > 2.34.1 > > > > > > > > > -- > > Regards, > > Atish > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Jan 17, 2024 at 12:58:21AM -0800, Atish Patra wrote: > On Tue, Jan 16, 2024 at 4:16 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Tue, Jan 16, 2024 at 12:55:41PM -0800, Atish Patra wrote: > > > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin > > > <peterlin@andestech.com> 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. > > > > > > > > > > I don't think we should do that. Any custom implementation that > > > violates the standard RISC-V spec should > > > be an errata not a feature. > > > As per my understanding, a vendor can call an extension custom ISA > > > extension if the same feature is not available > > > in the standard ISA extensions or the mechanism is completely > > > different. It must also not violate any standard spec as well. > > > > > > In this case, a standard sscofpmf is already available. Moreover, both > > > Andes and T-head extensions violate the standard > > > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which > > > are clearly specified as reserved for standard local interrupts > > > in the AIA specification. > > > > I disagree with you here. The Andes implementation predated (IIRC that > > is what was said in replies to an earlier revision) the Sscofpmf > > extension and certainly predates the AIA specification. I would be on > > board with this line of thinking if someone comes along in 2030 with > > "Zbb but with this one tweak" or where something flies entirely in the > > face of the standard (like the IOCP cache stuff). The relevant section > > in the AIA spec seems to say: > > | Interrupt causes that are standardized by the Privileged Architecture > > | have major identities in the range 0–15, while numbers 16 and higher are > > | officially available for platform standards or for custom use. > > | The Advanced Interrupt Architecture claims further authority over > > | identity numbers in the ranges 16–23 and 32–47, leaving numbers in the > > | range 24–31 and all major identities 48 and higher still free for custom > > | use. > > I don't see how that can be problematic given the Andes implemtation > > dates from before AIA was a thing. It would be silly to say that because > > an optional extension later came along and took over something previously > > allowed for indiscriminate custom use, that support for that custom > > extension is not permitted. > > > > AIA is not some optional extension. It defines the RISC-V interrupt > architecture going forward and will be the default implementation > in the future. I don't get you here. It is clearly optional - most (all?) existing systems do not have it and certainly none did when this was designed. The wording above from the AIA spec implies that using 16 and above used to be okay for platform specifics (and I think the relevant section of the priv spec was "Machine Interrupt Registers" which says the same). New extensions coming along should not be allowed to block kernel support for platform specifics that predated their use of permitted "resources". > IMO, this will be a slippery slope if we start > supporting custom implementations to override interrupt ID definitions > via custom cpu features. T-head implementation works perfectly fine as > an errata and I don't understand why > there is a push to make it a cpu feature. I don't mind leaving the existing implementation (T-Head) using this, but I will NAK any additions. > We should try to improve the > ecosystem for future platforms rather than bending > backwards to support older implementations. This is hardly "bending backwards". Thanks, Conor. > I understand the push to brand this as a custom extension if current > errata/alternative can't support it. But I don't think that's the case > here though. Please correct me if I am wrong. > > > I may well be missing something here though, you clearly know these > > specs better than I do, but from what I have read I disagree. > > > > > Please implementation Andes PMU support as an errata as well similar to T-head > > > > I certainly _do not_ want to see things like this detected via lookup > > tables of marchid and co in the kernel unless it is absolutely required. > > We have standard probing mechanisms for feature detection (because to me > > this _is_ a feature) and they should be used. Additionally, we define what > > entries in the DT properties mean, and if it is convenient to put > > "psuedo" extensions into the DT, then we should do so. Getting away from > > being tied to what RVI decrees was one of the goals of the new > > properties after all, so that we could use a standard mechanism of DT > > probing for things like this. > > > > Yes. That's a perfectly valid mechanism for actual custom/vendor ISA extensions. > I'm sure we'll have many of those, which will be leveraged via pseudo > extensions in the DT. > However, these shouldn't co-exist with standard ISA extensions in the > namespace in riscv_isa_ext and/or hwprobe. > The vendor-specific extensions should be defined under a > vendor-specific namespace. > This was another issue with this series as well. I didn't raise this > topic earlier because I don't think overriding interrupt > identities qualifies for a custom ISA extension > > > Thanks, > > Conor. > > > > > > 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> > > > > --- > > > > Changes v1 -> v2: > > > > - New patch > > > > Changes v2 -> v3: > > > > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() > > > > Changes v3 -> v4: > > > > - No change > > > > Changes v4 -> v5: > > > > - Include Guo's Reviewed-by > > > > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD > > > > Changes v5 -> v6: > > > > - Include Conor's Reviewed-by > > > > Changes v6 -> v7: > > > > - No change > > > > --- > > > > arch/riscv/Kconfig.errata | 13 ------------- > > > > arch/riscv/errata/thead/errata.c | 19 ------------------- > > > > arch/riscv/include/asm/errata_list.h | 15 +-------------- > > > > arch/riscv/include/asm/hwcap.h | 1 + > > > > arch/riscv/kernel/cpufeature.c | 1 + > > > > drivers/perf/Kconfig | 13 +++++++++++++ > > > > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- > > > > 7 files changed, 30 insertions(+), 51 deletions(-) > > > > > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > > > index e2c731cfed8c..0d19f47d1018 100644 > > > > --- a/arch/riscv/Kconfig.errata > > > > +++ b/arch/riscv/Kconfig.errata > > > > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO > > > > > > > > If you don't know what to do here, say "Y". > > > > > > > > -config ERRATA_THEAD_PMU > > > > - bool "Apply T-Head PMU errata" > > > > - depends on ERRATA_THEAD && RISCV_PMU_SBI > > > > - default y > > > > - help > > > > - The T-Head C9xx cores implement a PMU overflow extension very > > > > - similar to the core SSCOFPMF extension. > > > > - > > > > - This will apply the overflow errata to handle the non-standard > > > > - behaviour via the regular SBI PMU driver and interface. > > > > - > > > > - If you don't know what to do here, say "Y". > > > > - > > > > endmenu # "CPU errata selection" > > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > > index 0554ed4bf087..5de5f7209132 100644 > > > > --- a/arch/riscv/errata/thead/errata.c > > > > +++ b/arch/riscv/errata/thead/errata.c > > > > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, > > > > return true; > > > > } > > > > > > > > -static bool errata_probe_pmu(unsigned int stage, > > > > - unsigned long arch_id, unsigned long impid) > > > > -{ > > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) > > > > - return false; > > > > - > > > > - /* target-c9xx cores report arch_id and impid as 0 */ > > > > - if (arch_id != 0 || impid != 0) > > > > - return false; > > > > - > > > > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > > > - return false; > > > > - > > > > - return true; > > > > -} > > > > - > > > > static u32 thead_errata_probe(unsigned int stage, > > > > unsigned long archid, unsigned long impid) > > > > { > > > > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, > > > > if (errata_probe_cmo(stage, archid, impid)) > > > > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); > > > > > > > > - if (errata_probe_pmu(stage, archid, impid)) > > > > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > > > > - > > > > return cpu_req_errata; > > > > } > > > > > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > > > index 4ed21a62158c..9bccc2ba0eb5 100644 > > > > --- a/arch/riscv/include/asm/errata_list.h > > > > +++ b/arch/riscv/include/asm/errata_list.h > > > > @@ -25,8 +25,7 @@ > > > > #ifdef CONFIG_ERRATA_THEAD > > > > #define ERRATA_THEAD_PBMT 0 > > > > #define ERRATA_THEAD_CMO 1 > > > > -#define ERRATA_THEAD_PMU 2 > > > > -#define ERRATA_THEAD_NUMBER 3 > > > > +#define ERRATA_THEAD_NUMBER 2 > > > > #endif > > > > > > > > #ifdef __ASSEMBLY__ > > > > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ > > > > "r"((unsigned long)(_start) + (_size)) \ > > > > : "a0") > > > > > > > > -#define THEAD_C9XX_RV_IRQ_PMU 17 > > > > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > > - > > > > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > > -asm volatile(ALTERNATIVE( \ > > > > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ > > > > - CONFIG_ERRATA_THEAD_PMU) \ > > > > - : "=r" (__ovl) : \ > > > > - : "memory") > > > > - > > > > #endif /* __ASSEMBLY__ */ > > > > > > > > #endif > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > > index 5340f818746b..480f9da7fba7 100644 > > > > --- a/arch/riscv/include/asm/hwcap.h > > > > +++ b/arch/riscv/include/asm/hwcap.h > > > > @@ -80,6 +80,7 @@ > > > > #define RISCV_ISA_EXT_ZFA 71 > > > > #define RISCV_ISA_EXT_ZTSO 72 > > > > #define RISCV_ISA_EXT_ZACAS 73 > > > > +#define RISCV_ISA_EXT_XTHEADPMU 74 > > > > > > > > #define RISCV_ISA_EXT_MAX 128 > > > > #define RISCV_ISA_EXT_INVALID U32_MAX > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > > index e32591e9da90..4aded5bf8fc3 100644 > > > > --- a/arch/riscv/kernel/cpufeature.c > > > > +++ b/arch/riscv/kernel/cpufeature.c > > > > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > > > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), > > > > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), > > > > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), > > > > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), > > > > }; > > > > > > > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > > > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig > > > > index 273d67ecf6d2..6cef15ec7c25 100644 > > > > --- a/drivers/perf/Kconfig > > > > +++ b/drivers/perf/Kconfig > > > > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI > > > > full perf feature support i.e. counter overflow, privilege mode > > > > filtering, counter configuration. > > > > > > > > +config THEAD_CUSTOM_PMU > > > > + bool "T-Head custom PMU support" > > > > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI > > > > + default y > > > > + help > > > > + The T-Head C9xx cores implement a PMU overflow extension very > > > > + similar to the core SSCOFPMF extension. > > > > + > > > > + This will patch the overflow CSR and handle the non-standard > > > > + behaviour via the regular SBI PMU driver and interface. > > > > + > > > > + If you don't know what to do here, say "Y". > > > > + > > > > config ARM_PMU_ACPI > > > > depends on ARM_PMU && ACPI > > > > def_bool y > > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > > > index 2edbc37abadf..31ca79846399 100644 > > > > --- a/drivers/perf/riscv_pmu_sbi.c > > > > +++ b/drivers/perf/riscv_pmu_sbi.c > > > > @@ -20,10 +20,21 @@ > > > > #include <linux/cpu_pm.h> > > > > #include <linux/sched/clock.h> > > > > > > > > -#include <asm/errata_list.h> > > > > #include <asm/sbi.h> > > > > #include <asm/cpufeature.h> > > > > > > > > +#define THEAD_C9XX_RV_IRQ_PMU 17 > > > > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > > + > > > > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > > +asm volatile(ALTERNATIVE( \ > > > > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > > + 0, RISCV_ISA_EXT_XTHEADPMU, \ > > > > + CONFIG_THEAD_CUSTOM_PMU) \ > > > > + : "=r" (__ovl) : \ > > > > + : "memory") > > > > + > > > > #define SYSCTL_NO_USER_ACCESS 0 > > > > #define SYSCTL_USER_ACCESS 1 > > > > #define SYSCTL_LEGACY 2 > > > > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde > > > > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { > > > > riscv_pmu_irq_num = RV_IRQ_PMU; > > > > riscv_pmu_use_irq = true; > > > > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && > > > > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && > > > > - riscv_cached_marchid(0) == 0 && > > > > - riscv_cached_mimpid(0) == 0) { > > > > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && > > > > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { > > > > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; > > > > riscv_pmu_use_irq = true; > > > > } > > > > -- > > > > 2.34.1 > > > > > > > > > > > > > -- > > > Regards, > > > Atish > > > > -- > Regards, > Atish
On Wed, Jan 17, 2024 at 1:17 AM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Jan 17, 2024 at 12:58:21AM -0800, Atish Patra wrote: > > On Tue, Jan 16, 2024 at 4:16 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Tue, Jan 16, 2024 at 12:55:41PM -0800, Atish Patra wrote: > > > > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin > > > > <peterlin@andestech.com> 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. > > > > > > > > > > > > > I don't think we should do that. Any custom implementation that > > > > violates the standard RISC-V spec should > > > > be an errata not a feature. > > > > As per my understanding, a vendor can call an extension custom ISA > > > > extension if the same feature is not available > > > > in the standard ISA extensions or the mechanism is completely > > > > different. It must also not violate any standard spec as well. > > > > > > > > In this case, a standard sscofpmf is already available. Moreover, both > > > > Andes and T-head extensions violate the standard > > > > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which > > > > are clearly specified as reserved for standard local interrupts > > > > in the AIA specification. > > > > > > I disagree with you here. The Andes implementation predated (IIRC that > > > is what was said in replies to an earlier revision) the Sscofpmf > > > extension and certainly predates the AIA specification. I would be on > > > board with this line of thinking if someone comes along in 2030 with > > > "Zbb but with this one tweak" or where something flies entirely in the > > > face of the standard (like the IOCP cache stuff). The relevant section > > > in the AIA spec seems to say: > > > | Interrupt causes that are standardized by the Privileged Architecture > > > | have major identities in the range 0–15, while numbers 16 and higher are > > > | officially available for platform standards or for custom use. > > > | The Advanced Interrupt Architecture claims further authority over > > > | identity numbers in the ranges 16–23 and 32–47, leaving numbers in the > > > | range 24–31 and all major identities 48 and higher still free for custom > > > | use. > > > I don't see how that can be problematic given the Andes implemtation > > > dates from before AIA was a thing. It would be silly to say that because > > > an optional extension later came along and took over something previously > > > allowed for indiscriminate custom use, that support for that custom > > > extension is not permitted. > > > > > > > AIA is not some optional extension. It defines the RISC-V interrupt > > architecture going forward and will be the default implementation > > in the future. > > I don't get you here. It is clearly optional - most (all?) existing > systems do not have it and certainly none did when this was designed. That's the current situation. I was saying AIA is not "some" optional extension which most implementations will ignore in the future. In the future, I expect most platforms will implement AIA. > The wording above from the AIA spec implies that using 16 and above used > to be okay for platform specifics (and I think the relevant section of > the priv spec was "Machine Interrupt Registers" which says the same). As your quote also described above, AIA spec says (which overrides the priv spec) "The Advanced Interrupt Architecture claims further authority over identity numbers in the ranges 16–23 and 32–47, leaving numbers in the range 24–31 and all major identities 48 and higher still free for custom use." That means any implementation can be treated as custom (as per AIA spec) if they choose a local interrupt only in between 24-31 or > 48. Now if we choose to ignore the AIA spec and go with the old priv spec statement to decide if a custom implementation violated the standard encoding space, that's a different argument. That means we have to allow any future vendor implementation that violates as well as long as they claim that they designed their chip before AIA was ratified. > New extensions coming along should not be allowed to block kernel > support for platform specifics that predated their use of permitted > "resources". > > > IMO, this will be a slippery slope if we start > > supporting custom implementations to override interrupt ID definitions > > via custom cpu features. T-head implementation works perfectly fine as > > an errata and I don't understand why > > there is a push to make it a cpu feature. > > I don't mind leaving the existing implementation (T-Head) using this, > but I will NAK any additions. > That would be an ideal case where we won't require any additions because all RISC-V vendor implementations comply with the spec. In reality, we may not have that luxury ;) > > We should try to improve the > > ecosystem for future platforms rather than bending > > backwards to support older implementations. > > This is hardly "bending backwards". > > Thanks, > Conor. > > > I understand the push to brand this as a custom extension if current > > errata/alternative can't support it. But I don't think that's the case > > here though. Please correct me if I am wrong. > > > > > I may well be missing something here though, you clearly know these > > > specs better than I do, but from what I have read I disagree. > > > > > > > Please implementation Andes PMU support as an errata as well similar to T-head > > > > > > I certainly _do not_ want to see things like this detected via lookup > > > tables of marchid and co in the kernel unless it is absolutely required. > > > We have standard probing mechanisms for feature detection (because to me > > > this _is_ a feature) and they should be used. Additionally, we define what > > > entries in the DT properties mean, and if it is convenient to put > > > "psuedo" extensions into the DT, then we should do so. Getting away from > > > being tied to what RVI decrees was one of the goals of the new > > > properties after all, so that we could use a standard mechanism of DT > > > probing for things like this. > > > > > > > Yes. That's a perfectly valid mechanism for actual custom/vendor ISA extensions. > > I'm sure we'll have many of those, which will be leveraged via pseudo > > extensions in the DT. > > However, these shouldn't co-exist with standard ISA extensions in the > > namespace in riscv_isa_ext and/or hwprobe. > > The vendor-specific extensions should be defined under a > > vendor-specific namespace. > > This was another issue with this series as well. I didn't raise this > > topic earlier because I don't think overriding interrupt > > identities qualifies for a custom ISA extension > > Any thoughts on vendor specific namespace to avoid mixing standard ISA extensions with vendor specific ones ? > > > Thanks, > > > Conor. > > > > > > > > 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> > > > > > --- > > > > > Changes v1 -> v2: > > > > > - New patch > > > > > Changes v2 -> v3: > > > > > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() > > > > > Changes v3 -> v4: > > > > > - No change > > > > > Changes v4 -> v5: > > > > > - Include Guo's Reviewed-by > > > > > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD > > > > > Changes v5 -> v6: > > > > > - Include Conor's Reviewed-by > > > > > Changes v6 -> v7: > > > > > - No change > > > > > --- > > > > > arch/riscv/Kconfig.errata | 13 ------------- > > > > > arch/riscv/errata/thead/errata.c | 19 ------------------- > > > > > arch/riscv/include/asm/errata_list.h | 15 +-------------- > > > > > arch/riscv/include/asm/hwcap.h | 1 + > > > > > arch/riscv/kernel/cpufeature.c | 1 + > > > > > drivers/perf/Kconfig | 13 +++++++++++++ > > > > > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- > > > > > 7 files changed, 30 insertions(+), 51 deletions(-) > > > > > > > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > > > > index e2c731cfed8c..0d19f47d1018 100644 > > > > > --- a/arch/riscv/Kconfig.errata > > > > > +++ b/arch/riscv/Kconfig.errata > > > > > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO > > > > > > > > > > If you don't know what to do here, say "Y". > > > > > > > > > > -config ERRATA_THEAD_PMU > > > > > - bool "Apply T-Head PMU errata" > > > > > - depends on ERRATA_THEAD && RISCV_PMU_SBI > > > > > - default y > > > > > - help > > > > > - The T-Head C9xx cores implement a PMU overflow extension very > > > > > - similar to the core SSCOFPMF extension. > > > > > - > > > > > - This will apply the overflow errata to handle the non-standard > > > > > - behaviour via the regular SBI PMU driver and interface. > > > > > - > > > > > - If you don't know what to do here, say "Y". > > > > > - > > > > > endmenu # "CPU errata selection" > > > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > > > index 0554ed4bf087..5de5f7209132 100644 > > > > > --- a/arch/riscv/errata/thead/errata.c > > > > > +++ b/arch/riscv/errata/thead/errata.c > > > > > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, > > > > > return true; > > > > > } > > > > > > > > > > -static bool errata_probe_pmu(unsigned int stage, > > > > > - unsigned long arch_id, unsigned long impid) > > > > > -{ > > > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) > > > > > - return false; > > > > > - > > > > > - /* target-c9xx cores report arch_id and impid as 0 */ > > > > > - if (arch_id != 0 || impid != 0) > > > > > - return false; > > > > > - > > > > > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > > > > - return false; > > > > > - > > > > > - return true; > > > > > -} > > > > > - > > > > > static u32 thead_errata_probe(unsigned int stage, > > > > > unsigned long archid, unsigned long impid) > > > > > { > > > > > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, > > > > > if (errata_probe_cmo(stage, archid, impid)) > > > > > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); > > > > > > > > > > - if (errata_probe_pmu(stage, archid, impid)) > > > > > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > > > > > - > > > > > return cpu_req_errata; > > > > > } > > > > > > > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > > > > index 4ed21a62158c..9bccc2ba0eb5 100644 > > > > > --- a/arch/riscv/include/asm/errata_list.h > > > > > +++ b/arch/riscv/include/asm/errata_list.h > > > > > @@ -25,8 +25,7 @@ > > > > > #ifdef CONFIG_ERRATA_THEAD > > > > > #define ERRATA_THEAD_PBMT 0 > > > > > #define ERRATA_THEAD_CMO 1 > > > > > -#define ERRATA_THEAD_PMU 2 > > > > > -#define ERRATA_THEAD_NUMBER 3 > > > > > +#define ERRATA_THEAD_NUMBER 2 > > > > > #endif > > > > > > > > > > #ifdef __ASSEMBLY__ > > > > > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ > > > > > "r"((unsigned long)(_start) + (_size)) \ > > > > > : "a0") > > > > > > > > > > -#define THEAD_C9XX_RV_IRQ_PMU 17 > > > > > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > > > - > > > > > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > > > -asm volatile(ALTERNATIVE( \ > > > > > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > > > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > > > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ > > > > > - CONFIG_ERRATA_THEAD_PMU) \ > > > > > - : "=r" (__ovl) : \ > > > > > - : "memory") > > > > > - > > > > > #endif /* __ASSEMBLY__ */ > > > > > > > > > > #endif > > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > > > index 5340f818746b..480f9da7fba7 100644 > > > > > --- a/arch/riscv/include/asm/hwcap.h > > > > > +++ b/arch/riscv/include/asm/hwcap.h > > > > > @@ -80,6 +80,7 @@ > > > > > #define RISCV_ISA_EXT_ZFA 71 > > > > > #define RISCV_ISA_EXT_ZTSO 72 > > > > > #define RISCV_ISA_EXT_ZACAS 73 > > > > > +#define RISCV_ISA_EXT_XTHEADPMU 74 > > > > > > > > > > #define RISCV_ISA_EXT_MAX 128 > > > > > #define RISCV_ISA_EXT_INVALID U32_MAX > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > > > index e32591e9da90..4aded5bf8fc3 100644 > > > > > --- a/arch/riscv/kernel/cpufeature.c > > > > > +++ b/arch/riscv/kernel/cpufeature.c > > > > > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > > > > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), > > > > > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), > > > > > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), > > > > > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), > > > > > }; > > > > > > > > > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > > > > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig > > > > > index 273d67ecf6d2..6cef15ec7c25 100644 > > > > > --- a/drivers/perf/Kconfig > > > > > +++ b/drivers/perf/Kconfig > > > > > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI > > > > > full perf feature support i.e. counter overflow, privilege mode > > > > > filtering, counter configuration. > > > > > > > > > > +config THEAD_CUSTOM_PMU > > > > > + bool "T-Head custom PMU support" > > > > > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI > > > > > + default y > > > > > + help > > > > > + The T-Head C9xx cores implement a PMU overflow extension very > > > > > + similar to the core SSCOFPMF extension. > > > > > + > > > > > + This will patch the overflow CSR and handle the non-standard > > > > > + behaviour via the regular SBI PMU driver and interface. > > > > > + > > > > > + If you don't know what to do here, say "Y". > > > > > + > > > > > config ARM_PMU_ACPI > > > > > depends on ARM_PMU && ACPI > > > > > def_bool y > > > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > > > > index 2edbc37abadf..31ca79846399 100644 > > > > > --- a/drivers/perf/riscv_pmu_sbi.c > > > > > +++ b/drivers/perf/riscv_pmu_sbi.c > > > > > @@ -20,10 +20,21 @@ > > > > > #include <linux/cpu_pm.h> > > > > > #include <linux/sched/clock.h> > > > > > > > > > > -#include <asm/errata_list.h> > > > > > #include <asm/sbi.h> > > > > > #include <asm/cpufeature.h> > > > > > > > > > > +#define THEAD_C9XX_RV_IRQ_PMU 17 > > > > > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > > > + > > > > > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > > > +asm volatile(ALTERNATIVE( \ > > > > > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > > > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > > > + 0, RISCV_ISA_EXT_XTHEADPMU, \ > > > > > + CONFIG_THEAD_CUSTOM_PMU) \ > > > > > + : "=r" (__ovl) : \ > > > > > + : "memory") > > > > > + > > > > > #define SYSCTL_NO_USER_ACCESS 0 > > > > > #define SYSCTL_USER_ACCESS 1 > > > > > #define SYSCTL_LEGACY 2 > > > > > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde > > > > > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { > > > > > riscv_pmu_irq_num = RV_IRQ_PMU; > > > > > riscv_pmu_use_irq = true; > > > > > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && > > > > > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && > > > > > - riscv_cached_marchid(0) == 0 && > > > > > - riscv_cached_mimpid(0) == 0) { > > > > > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && > > > > > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { > > > > > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; > > > > > riscv_pmu_use_irq = true; > > > > > } > > > > > -- > > > > > 2.34.1 > > > > > > > > > > > > > > > > > -- > > > > Regards, > > > > Atish > > > > > > > > -- > > Regards, > > Atish
On Wed, Jan 17, 2024 at 02:32:59PM -0800, Atish Patra wrote: > On Wed, Jan 17, 2024 at 1:17 AM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, Jan 17, 2024 at 12:58:21AM -0800, Atish Patra wrote: > > > On Tue, Jan 16, 2024 at 4:16 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > > > On Tue, Jan 16, 2024 at 12:55:41PM -0800, Atish Patra wrote: > > > > > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin > > > > > <peterlin@andestech.com> 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. > > > > > > > > > > > > > > > > I don't think we should do that. Any custom implementation that > > > > > violates the standard RISC-V spec should > > > > > be an errata not a feature. > > > > > As per my understanding, a vendor can call an extension custom ISA > > > > > extension if the same feature is not available > > > > > in the standard ISA extensions or the mechanism is completely > > > > > different. It must also not violate any standard spec as well. > > > > > > > > > > In this case, a standard sscofpmf is already available. Moreover, both > > > > > Andes and T-head extensions violate the standard > > > > > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which > > > > > are clearly specified as reserved for standard local interrupts > > > > > in the AIA specification. > > > > > > > > I disagree with you here. The Andes implementation predated (IIRC that > > > > is what was said in replies to an earlier revision) the Sscofpmf > > > > extension and certainly predates the AIA specification. I would be on > > > > board with this line of thinking if someone comes along in 2030 with > > > > "Zbb but with this one tweak" or where something flies entirely in the > > > > face of the standard (like the IOCP cache stuff). The relevant section > > > > in the AIA spec seems to say: > > > > | Interrupt causes that are standardized by the Privileged Architecture > > > > | have major identities in the range 0–15, while numbers 16 and higher are > > > > | officially available for platform standards or for custom use. > > > > | The Advanced Interrupt Architecture claims further authority over > > > > | identity numbers in the ranges 16–23 and 32–47, leaving numbers in the > > > > | range 24–31 and all major identities 48 and higher still free for custom > > > > | use. > > > > I don't see how that can be problematic given the Andes implemtation > > > > dates from before AIA was a thing. It would be silly to say that because > > > > an optional extension later came along and took over something previously > > > > allowed for indiscriminate custom use, that support for that custom > > > > extension is not permitted. > > > > > > > > > > AIA is not some optional extension. It defines the RISC-V interrupt > > > architecture going forward and will be the default implementation > > > in the future. > > > > I don't get you here. It is clearly optional - most (all?) existing > > systems do not have it and certainly none did when this was designed. > > That's the current situation. I was saying AIA is not "some" optional extension > which most implementations will ignore in the future. In the future, I > expect most platforms will implement AIA. In the future all platforms may, but I don't think that that is particularly important here. The systems that we are talking about at the moment do not have AIA. If there are AIA capable systems produced using Andes' IP, one would hope that they implement Sscopmf (or w/e the forgettable extension name is). :fingers_crossed: > > The wording above from the AIA spec implies that using 16 and above used > > to be okay for platform specifics (and I think the relevant section of > > the priv spec was "Machine Interrupt Registers" which says the same). > > As your quote also described above, AIA spec says (which overrides the > priv spec) > > "The Advanced Interrupt Architecture claims further authority over > identity numbers in the ranges 16–23 and 32–47, > leaving numbers in the range 24–31 and all major identities 48 and > higher still free for custom use." > > That means any implementation can be treated as custom (as per AIA > spec) if they choose a local interrupt > only in between 24-31 or > 48. Now if we choose to ignore the AIA spec > and go with the old priv spec statement to > decide if a custom implementation violated the standard encoding > space, that's a different argument. That means we have > to allow any future vendor implementation that violates as well as > long as they claim that they designed their chip before > AIA was ratified. I don't see what the problem with that is. It is completely unreasonable to render custom extensions that used the resources available to them at the time invalid for use in the kernel (unless branded as an erratum) because later on standard extensions co-opted those resources for its own usage. > > New extensions coming along should not be allowed to block kernel > > support for platform specifics that predated their use of permitted > > "resources". > > > > > IMO, this will be a slippery slope if we start > > > supporting custom implementations to override interrupt ID definitions > > > via custom cpu features. T-head implementation works perfectly fine as > > > an errata and I don't understand why > > > there is a push to make it a cpu feature. > > > > I don't mind leaving the existing implementation (T-Head) using this, > > but I will NAK any additions. > > > > That would be an ideal case where we won't require any additions > because all RISC-V vendor implementations > comply with the spec. In reality, we may not have that luxury ;) Where we have no other choice but to use marchid et al for detecting issues with a vendors implementation, then I have no problem with it. If you look at any of the threads where I have objected to the use of them, it's been specifically for the detection of features, not for their use in dealing with implementation issues (like the sifive sfence issues). > > > We should try to improve the > > > ecosystem for future platforms rather than bending > > > backwards to support older implementations. > > > > This is hardly "bending backwards". > > > > > I understand the push to brand this as a custom extension if current > > > errata/alternative can't support it. But I don't think that's the case > > > here though. Please correct me if I am wrong. > > > > > > > I may well be missing something here though, you clearly know these > > > > specs better than I do, but from what I have read I disagree. > > > > > > > > > Please implementation Andes PMU support as an errata as well similar to T-head > > > > > > > > I certainly _do not_ want to see things like this detected via lookup > > > > tables of marchid and co in the kernel unless it is absolutely required. > > > > We have standard probing mechanisms for feature detection (because to me > > > > this _is_ a feature) and they should be used. Additionally, we define what > > > > entries in the DT properties mean, and if it is convenient to put > > > > "psuedo" extensions into the DT, then we should do so. Getting away from > > > > being tied to what RVI decrees was one of the goals of the new > > > > properties after all, so that we could use a standard mechanism of DT > > > > probing for things like this. > > > > > > > > > > Yes. That's a perfectly valid mechanism for actual custom/vendor ISA extensions. > > > I'm sure we'll have many of those, which will be leveraged via pseudo > > > extensions in the DT. > > > However, these shouldn't co-exist with standard ISA extensions in the > > > namespace in riscv_isa_ext and/or hwprobe. > > > The vendor-specific extensions should be defined under a > > > vendor-specific namespace. > > > This was another issue with this series as well. I didn't raise this > > > topic earlier because I don't think overriding interrupt > > > identities qualifies for a custom ISA extension > > > > > Any thoughts on vendor specific namespace to avoid mixing standard ISA > extensions with vendor specific ones ? I don't really care for how it is exposed in hwprobe, you should ask those responsible for the hwprobe interface what they think. If you mean on the DT side, one of the stated goals of the new properties was to put RVI's extensions and vendor extensions on a equal footing. Dunno if that answers your question, Conor.
On Wed, 17 Jan 2024 01:17:23 PST (-0800), Conor Dooley wrote: > On Wed, Jan 17, 2024 at 12:58:21AM -0800, Atish Patra wrote: >> On Tue, Jan 16, 2024 at 4:16 PM Conor Dooley <conor@kernel.org> wrote: >> > >> > On Tue, Jan 16, 2024 at 12:55:41PM -0800, Atish Patra wrote: >> > > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin >> > > <peterlin@andestech.com> 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. >> > > > >> > > >> > > I don't think we should do that. Any custom implementation that >> > > violates the standard RISC-V spec should >> > > be an errata not a feature. >> > > As per my understanding, a vendor can call an extension custom ISA >> > > extension if the same feature is not available >> > > in the standard ISA extensions or the mechanism is completely >> > > different. It must also not violate any standard spec as well. >> > > >> > > In this case, a standard sscofpmf is already available. Moreover, both >> > > Andes and T-head extensions violate the standard >> > > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which >> > > are clearly specified as reserved for standard local interrupts >> > > in the AIA specification. >> > >> > I disagree with you here. The Andes implementation predated (IIRC that >> > is what was said in replies to an earlier revision) the Sscofpmf >> > extension and certainly predates the AIA specification. I would be on >> > board with this line of thinking if someone comes along in 2030 with >> > "Zbb but with this one tweak" or where something flies entirely in the >> > face of the standard (like the IOCP cache stuff). The relevant section >> > in the AIA spec seems to say: >> > | Interrupt causes that are standardized by the Privileged Architecture >> > | have major identities in the range 0–15, while numbers 16 and higher are >> > | officially available for platform standards or for custom use. >> > | The Advanced Interrupt Architecture claims further authority over >> > | identity numbers in the ranges 16–23 and 32–47, leaving numbers in the >> > | range 24–31 and all major identities 48 and higher still free for custom >> > | use. >> > I don't see how that can be problematic given the Andes implemtation >> > dates from before AIA was a thing. It would be silly to say that because >> > an optional extension later came along and took over something previously >> > allowed for indiscriminate custom use, that support for that custom >> > extension is not permitted. >> > >> >> AIA is not some optional extension. It defines the RISC-V interrupt >> architecture going forward and will be the default implementation >> in the future. > > I don't get you here. It is clearly optional - most (all?) existing > systems do not have it and certainly none did when this was designed. > The wording above from the AIA spec implies that using 16 and above used > to be okay for platform specifics (and I think the relevant section of > the priv spec was "Machine Interrupt Registers" which says the same). > New extensions coming along should not be allowed to block kernel > support for platform specifics that predated their use of permitted > "resources". Ya, the AIA stuff is definatley optional. It's not even in the "everything's optional" grey area that's so common in RISC-V land, the AIA is a set of extensions and thus it's optional by design. >> IMO, this will be a slippery slope if we start >> supporting custom implementations to override interrupt ID definitions >> via custom cpu features. T-head implementation works perfectly fine as >> an errata and I don't understand why >> there is a push to make it a cpu feature. > > I don't mind leaving the existing implementation (T-Head) using this, > but I will NAK any additions. I can't quite tell what the difference is here. IMO whether we call something a feature or an errata doesn't really matter any more, it's just whatever framework happens to fit better. >> We should try to improve the >> ecosystem for future platforms rather than bending >> backwards to support older implementations. > > This is hardly "bending backwards". and even if it was, it'd be for real hardware and that's really what we should be focusing on. > Thanks, > Conor. > >> I understand the push to brand this as a custom extension if current >> errata/alternative can't support it. But I don't think that's the case >> here though. Please correct me if I am wrong. >> >> > I may well be missing something here though, you clearly know these >> > specs better than I do, but from what I have read I disagree. >> > >> > > Please implementation Andes PMU support as an errata as well similar to T-head >> > >> > I certainly _do not_ want to see things like this detected via lookup >> > tables of marchid and co in the kernel unless it is absolutely required. >> > We have standard probing mechanisms for feature detection (because to me >> > this _is_ a feature) and they should be used. Additionally, we define what >> > entries in the DT properties mean, and if it is convenient to put >> > "psuedo" extensions into the DT, then we should do so. Getting away from >> > being tied to what RVI decrees was one of the goals of the new >> > properties after all, so that we could use a standard mechanism of DT >> > probing for things like this. >> > >> >> Yes. That's a perfectly valid mechanism for actual custom/vendor ISA extensions. >> I'm sure we'll have many of those, which will be leveraged via pseudo >> extensions in the DT. >> However, these shouldn't co-exist with standard ISA extensions in the >> namespace in riscv_isa_ext and/or hwprobe. >> The vendor-specific extensions should be defined under a >> vendor-specific namespace. >> This was another issue with this series as well. I didn't raise this >> topic earlier because I don't think overriding interrupt >> identities qualifies for a custom ISA extension >> >> > Thanks, >> > Conor. >> > >> > > > 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> >> > > > --- >> > > > Changes v1 -> v2: >> > > > - New patch >> > > > Changes v2 -> v3: >> > > > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() >> > > > Changes v3 -> v4: >> > > > - No change >> > > > Changes v4 -> v5: >> > > > - Include Guo's Reviewed-by >> > > > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD >> > > > Changes v5 -> v6: >> > > > - Include Conor's Reviewed-by >> > > > Changes v6 -> v7: >> > > > - No change >> > > > --- >> > > > arch/riscv/Kconfig.errata | 13 ------------- >> > > > arch/riscv/errata/thead/errata.c | 19 ------------------- >> > > > arch/riscv/include/asm/errata_list.h | 15 +-------------- >> > > > arch/riscv/include/asm/hwcap.h | 1 + >> > > > arch/riscv/kernel/cpufeature.c | 1 + >> > > > drivers/perf/Kconfig | 13 +++++++++++++ >> > > > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- >> > > > 7 files changed, 30 insertions(+), 51 deletions(-) >> > > > >> > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata >> > > > index e2c731cfed8c..0d19f47d1018 100644 >> > > > --- a/arch/riscv/Kconfig.errata >> > > > +++ b/arch/riscv/Kconfig.errata >> > > > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO >> > > > >> > > > If you don't know what to do here, say "Y". >> > > > >> > > > -config ERRATA_THEAD_PMU >> > > > - bool "Apply T-Head PMU errata" >> > > > - depends on ERRATA_THEAD && RISCV_PMU_SBI >> > > > - default y >> > > > - help >> > > > - The T-Head C9xx cores implement a PMU overflow extension very >> > > > - similar to the core SSCOFPMF extension. >> > > > - >> > > > - This will apply the overflow errata to handle the non-standard >> > > > - behaviour via the regular SBI PMU driver and interface. >> > > > - >> > > > - If you don't know what to do here, say "Y". >> > > > - >> > > > endmenu # "CPU errata selection" >> > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c >> > > > index 0554ed4bf087..5de5f7209132 100644 >> > > > --- a/arch/riscv/errata/thead/errata.c >> > > > +++ b/arch/riscv/errata/thead/errata.c >> > > > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, >> > > > return true; >> > > > } >> > > > >> > > > -static bool errata_probe_pmu(unsigned int stage, >> > > > - unsigned long arch_id, unsigned long impid) >> > > > -{ >> > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) >> > > > - return false; >> > > > - >> > > > - /* target-c9xx cores report arch_id and impid as 0 */ >> > > > - if (arch_id != 0 || impid != 0) >> > > > - return false; >> > > > - >> > > > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) >> > > > - return false; >> > > > - >> > > > - return true; >> > > > -} >> > > > - >> > > > static u32 thead_errata_probe(unsigned int stage, >> > > > unsigned long archid, unsigned long impid) >> > > > { >> > > > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, >> > > > if (errata_probe_cmo(stage, archid, impid)) >> > > > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); >> > > > >> > > > - if (errata_probe_pmu(stage, archid, impid)) >> > > > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); >> > > > - >> > > > return cpu_req_errata; >> > > > } >> > > > >> > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h >> > > > index 4ed21a62158c..9bccc2ba0eb5 100644 >> > > > --- a/arch/riscv/include/asm/errata_list.h >> > > > +++ b/arch/riscv/include/asm/errata_list.h >> > > > @@ -25,8 +25,7 @@ >> > > > #ifdef CONFIG_ERRATA_THEAD >> > > > #define ERRATA_THEAD_PBMT 0 >> > > > #define ERRATA_THEAD_CMO 1 >> > > > -#define ERRATA_THEAD_PMU 2 >> > > > -#define ERRATA_THEAD_NUMBER 3 >> > > > +#define ERRATA_THEAD_NUMBER 2 >> > > > #endif >> > > > >> > > > #ifdef __ASSEMBLY__ >> > > > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ >> > > > "r"((unsigned long)(_start) + (_size)) \ >> > > > : "a0") >> > > > >> > > > -#define THEAD_C9XX_RV_IRQ_PMU 17 >> > > > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 >> > > > - >> > > > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ >> > > > -asm volatile(ALTERNATIVE( \ >> > > > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ >> > > > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ >> > > > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ >> > > > - CONFIG_ERRATA_THEAD_PMU) \ >> > > > - : "=r" (__ovl) : \ >> > > > - : "memory") >> > > > - >> > > > #endif /* __ASSEMBLY__ */ >> > > > >> > > > #endif >> > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h >> > > > index 5340f818746b..480f9da7fba7 100644 >> > > > --- a/arch/riscv/include/asm/hwcap.h >> > > > +++ b/arch/riscv/include/asm/hwcap.h >> > > > @@ -80,6 +80,7 @@ >> > > > #define RISCV_ISA_EXT_ZFA 71 >> > > > #define RISCV_ISA_EXT_ZTSO 72 >> > > > #define RISCV_ISA_EXT_ZACAS 73 >> > > > +#define RISCV_ISA_EXT_XTHEADPMU 74 >> > > > >> > > > #define RISCV_ISA_EXT_MAX 128 >> > > > #define RISCV_ISA_EXT_INVALID U32_MAX >> > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> > > > index e32591e9da90..4aded5bf8fc3 100644 >> > > > --- a/arch/riscv/kernel/cpufeature.c >> > > > +++ b/arch/riscv/kernel/cpufeature.c >> > > > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { >> > > > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), >> > > > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), >> > > > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), >> > > > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), >> > > > }; >> > > > >> > > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); >> > > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig >> > > > index 273d67ecf6d2..6cef15ec7c25 100644 >> > > > --- a/drivers/perf/Kconfig >> > > > +++ b/drivers/perf/Kconfig >> > > > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI >> > > > full perf feature support i.e. counter overflow, privilege mode >> > > > filtering, counter configuration. >> > > > >> > > > +config THEAD_CUSTOM_PMU >> > > > + bool "T-Head custom PMU support" >> > > > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI >> > > > + default y >> > > > + help >> > > > + The T-Head C9xx cores implement a PMU overflow extension very >> > > > + similar to the core SSCOFPMF extension. >> > > > + >> > > > + This will patch the overflow CSR and handle the non-standard >> > > > + behaviour via the regular SBI PMU driver and interface. >> > > > + >> > > > + If you don't know what to do here, say "Y". >> > > > + >> > > > config ARM_PMU_ACPI >> > > > depends on ARM_PMU && ACPI >> > > > def_bool y >> > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c >> > > > index 2edbc37abadf..31ca79846399 100644 >> > > > --- a/drivers/perf/riscv_pmu_sbi.c >> > > > +++ b/drivers/perf/riscv_pmu_sbi.c >> > > > @@ -20,10 +20,21 @@ >> > > > #include <linux/cpu_pm.h> >> > > > #include <linux/sched/clock.h> >> > > > >> > > > -#include <asm/errata_list.h> >> > > > #include <asm/sbi.h> >> > > > #include <asm/cpufeature.h> >> > > > >> > > > +#define THEAD_C9XX_RV_IRQ_PMU 17 >> > > > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 >> > > > + >> > > > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ >> > > > +asm volatile(ALTERNATIVE( \ >> > > > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ >> > > > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ >> > > > + 0, RISCV_ISA_EXT_XTHEADPMU, \ >> > > > + CONFIG_THEAD_CUSTOM_PMU) \ >> > > > + : "=r" (__ovl) : \ >> > > > + : "memory") >> > > > + >> > > > #define SYSCTL_NO_USER_ACCESS 0 >> > > > #define SYSCTL_USER_ACCESS 1 >> > > > #define SYSCTL_LEGACY 2 >> > > > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde >> > > > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { >> > > > riscv_pmu_irq_num = RV_IRQ_PMU; >> > > > riscv_pmu_use_irq = true; >> > > > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && >> > > > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && >> > > > - riscv_cached_marchid(0) == 0 && >> > > > - riscv_cached_mimpid(0) == 0) { >> > > > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && >> > > > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { >> > > > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; >> > > > riscv_pmu_use_irq = true; >> > > > } >> > > > -- >> > > > 2.34.1 >> > > > >> > > >> > > >> > > -- >> > > Regards, >> > > Atish >> >> >> >> -- >> Regards, >> Atish
Thank you all for your opinion, In fact, the Sscofpmf has been implemented in Andes cores newer than AX45, which is the last one to support Andes PMU. Additionally, this feature will be switched to Sscofpmf once Andes core supports AIA. On Wed, Jan 17, 2024 at 12:58:21AM -0800, Atish Patra wrote: > On Tue, Jan 16, 2024 at 4:16 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Tue, Jan 16, 2024 at 12:55:41PM -0800, Atish Patra wrote: > > > On Tue, Jan 9, 2024 at 11:40 PM Yu Chien Peter Lin > > > <peterlin@andestech.com> 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. > > > > > > > > > > I don't think we should do that. Any custom implementation that > > > violates the standard RISC-V spec should > > > be an errata not a feature. > > > As per my understanding, a vendor can call an extension custom ISA > > > extension if the same feature is not available > > > in the standard ISA extensions or the mechanism is completely > > > different. It must also not violate any standard spec as well. > > > > > > In this case, a standard sscofpmf is already available. Moreover, both > > > Andes and T-head extensions violate the standard > > > spec by reusing local interrupt numbers (17(Thead) & 18(Andes)) which > > > are clearly specified as reserved for standard local interrupts > > > in the AIA specification. > > > > I disagree with you here. The Andes implementation predated (IIRC that > > is what was said in replies to an earlier revision) the Sscofpmf > > extension and certainly predates the AIA specification. I would be on > > board with this line of thinking if someone comes along in 2030 with > > "Zbb but with this one tweak" or where something flies entirely in the > > face of the standard (like the IOCP cache stuff). The relevant section > > in the AIA spec seems to say: > > | Interrupt causes that are standardized by the Privileged Architecture > > | have major identities in the range 0–15, while numbers 16 and higher are > > | officially available for platform standards or for custom use. > > | The Advanced Interrupt Architecture claims further authority over > > | identity numbers in the ranges 16–23 and 32–47, leaving numbers in the > > | range 24–31 and all major identities 48 and higher still free for custom > > | use. > > I don't see how that can be problematic given the Andes implemtation > > dates from before AIA was a thing. It would be silly to say that because > > an optional extension later came along and took over something previously > > allowed for indiscriminate custom use, that support for that custom > > extension is not permitted. > > > > AIA is not some optional extension. It defines the RISC-V interrupt > architecture going forward and will be the default implementation > in the future. IMO, this will be a slippery slope if we start > supporting custom implementations to override interrupt ID definitions > via custom cpu features. T-head implementation works perfectly fine as > an errata and I don't understand why > there is a push to make it a cpu feature. We should try to improve the > ecosystem for future platforms rather than bending > backwards to support older implementations. > > I understand the push to brand this as a custom extension if current > errata/alternative can't support it. But I don't think that's the case > here though. Please correct me if I am wrong. > > > I may well be missing something here though, you clearly know these > > specs better than I do, but from what I have read I disagree. > > > > > Please implementation Andes PMU support as an errata as well similar to T-head > > > > I certainly _do not_ want to see things like this detected via lookup > > tables of marchid and co in the kernel unless it is absolutely required. > > We have standard probing mechanisms for feature detection (because to me > > this _is_ a feature) and they should be used. Additionally, we define what > > entries in the DT properties mean, and if it is convenient to put > > "psuedo" extensions into the DT, then we should do so. Getting away from > > being tied to what RVI decrees was one of the goals of the new > > properties after all, so that we could use a standard mechanism of DT > > probing for things like this. > > > > Yes. That's a perfectly valid mechanism for actual custom/vendor ISA extensions. > I'm sure we'll have many of those, which will be leveraged via pseudo > extensions in the DT. > However, these shouldn't co-exist with standard ISA extensions in the > namespace in riscv_isa_ext and/or hwprobe. > The vendor-specific extensions should be defined under a > vendor-specific namespace. I will remove T-Head PMU changes from this series. Could you please elaborate a bit more on how should I add a vendor-specific extension? Thanks, Peter Lin > This was another issue with this series as well. I didn't raise this > topic earlier because I don't think overriding interrupt > identities qualifies for a custom ISA extension > > > Thanks, > > Conor. > > > > > > 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> > > > > --- > > > > Changes v1 -> v2: > > > > - New patch > > > > Changes v2 -> v3: > > > > - Removed m{vendor/arch/imp}id checks in pmu_sbi_setup_irqs() > > > > Changes v3 -> v4: > > > > - No change > > > > Changes v4 -> v5: > > > > - Include Guo's Reviewed-by > > > > - Let THEAD_CUSTOM_PMU depend on ARCH_THEAD > > > > Changes v5 -> v6: > > > > - Include Conor's Reviewed-by > > > > Changes v6 -> v7: > > > > - No change > > > > --- > > > > arch/riscv/Kconfig.errata | 13 ------------- > > > > arch/riscv/errata/thead/errata.c | 19 ------------------- > > > > arch/riscv/include/asm/errata_list.h | 15 +-------------- > > > > arch/riscv/include/asm/hwcap.h | 1 + > > > > arch/riscv/kernel/cpufeature.c | 1 + > > > > drivers/perf/Kconfig | 13 +++++++++++++ > > > > drivers/perf/riscv_pmu_sbi.c | 19 ++++++++++++++----- > > > > 7 files changed, 30 insertions(+), 51 deletions(-) > > > > > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > > > index e2c731cfed8c..0d19f47d1018 100644 > > > > --- a/arch/riscv/Kconfig.errata > > > > +++ b/arch/riscv/Kconfig.errata > > > > @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO > > > > > > > > If you don't know what to do here, say "Y". > > > > > > > > -config ERRATA_THEAD_PMU > > > > - bool "Apply T-Head PMU errata" > > > > - depends on ERRATA_THEAD && RISCV_PMU_SBI > > > > - default y > > > > - help > > > > - The T-Head C9xx cores implement a PMU overflow extension very > > > > - similar to the core SSCOFPMF extension. > > > > - > > > > - This will apply the overflow errata to handle the non-standard > > > > - behaviour via the regular SBI PMU driver and interface. > > > > - > > > > - If you don't know what to do here, say "Y". > > > > - > > > > endmenu # "CPU errata selection" > > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > > index 0554ed4bf087..5de5f7209132 100644 > > > > --- a/arch/riscv/errata/thead/errata.c > > > > +++ b/arch/riscv/errata/thead/errata.c > > > > @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, > > > > return true; > > > > } > > > > > > > > -static bool errata_probe_pmu(unsigned int stage, > > > > - unsigned long arch_id, unsigned long impid) > > > > -{ > > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) > > > > - return false; > > > > - > > > > - /* target-c9xx cores report arch_id and impid as 0 */ > > > > - if (arch_id != 0 || impid != 0) > > > > - return false; > > > > - > > > > - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > > > - return false; > > > > - > > > > - return true; > > > > -} > > > > - > > > > static u32 thead_errata_probe(unsigned int stage, > > > > unsigned long archid, unsigned long impid) > > > > { > > > > @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, > > > > if (errata_probe_cmo(stage, archid, impid)) > > > > cpu_req_errata |= BIT(ERRATA_THEAD_CMO); > > > > > > > > - if (errata_probe_pmu(stage, archid, impid)) > > > > - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > > > > - > > > > return cpu_req_errata; > > > > } > > > > > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > > > index 4ed21a62158c..9bccc2ba0eb5 100644 > > > > --- a/arch/riscv/include/asm/errata_list.h > > > > +++ b/arch/riscv/include/asm/errata_list.h > > > > @@ -25,8 +25,7 @@ > > > > #ifdef CONFIG_ERRATA_THEAD > > > > #define ERRATA_THEAD_PBMT 0 > > > > #define ERRATA_THEAD_CMO 1 > > > > -#define ERRATA_THEAD_PMU 2 > > > > -#define ERRATA_THEAD_NUMBER 3 > > > > +#define ERRATA_THEAD_NUMBER 2 > > > > #endif > > > > > > > > #ifdef __ASSEMBLY__ > > > > @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ > > > > "r"((unsigned long)(_start) + (_size)) \ > > > > : "a0") > > > > > > > > -#define THEAD_C9XX_RV_IRQ_PMU 17 > > > > -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > > - > > > > -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > > -asm volatile(ALTERNATIVE( \ > > > > - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > > - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > > - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ > > > > - CONFIG_ERRATA_THEAD_PMU) \ > > > > - : "=r" (__ovl) : \ > > > > - : "memory") > > > > - > > > > #endif /* __ASSEMBLY__ */ > > > > > > > > #endif > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > > index 5340f818746b..480f9da7fba7 100644 > > > > --- a/arch/riscv/include/asm/hwcap.h > > > > +++ b/arch/riscv/include/asm/hwcap.h > > > > @@ -80,6 +80,7 @@ > > > > #define RISCV_ISA_EXT_ZFA 71 > > > > #define RISCV_ISA_EXT_ZTSO 72 > > > > #define RISCV_ISA_EXT_ZACAS 73 > > > > +#define RISCV_ISA_EXT_XTHEADPMU 74 > > > > > > > > #define RISCV_ISA_EXT_MAX 128 > > > > #define RISCV_ISA_EXT_INVALID U32_MAX > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > > index e32591e9da90..4aded5bf8fc3 100644 > > > > --- a/arch/riscv/kernel/cpufeature.c > > > > +++ b/arch/riscv/kernel/cpufeature.c > > > > @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > > > __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), > > > > __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), > > > > __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), > > > > + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), > > > > }; > > > > > > > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > > > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig > > > > index 273d67ecf6d2..6cef15ec7c25 100644 > > > > --- a/drivers/perf/Kconfig > > > > +++ b/drivers/perf/Kconfig > > > > @@ -86,6 +86,19 @@ config RISCV_PMU_SBI > > > > full perf feature support i.e. counter overflow, privilege mode > > > > filtering, counter configuration. > > > > > > > > +config THEAD_CUSTOM_PMU > > > > + bool "T-Head custom PMU support" > > > > + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI > > > > + default y > > > > + help > > > > + The T-Head C9xx cores implement a PMU overflow extension very > > > > + similar to the core SSCOFPMF extension. > > > > + > > > > + This will patch the overflow CSR and handle the non-standard > > > > + behaviour via the regular SBI PMU driver and interface. > > > > + > > > > + If you don't know what to do here, say "Y". > > > > + > > > > config ARM_PMU_ACPI > > > > depends on ARM_PMU && ACPI > > > > def_bool y > > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > > > index 2edbc37abadf..31ca79846399 100644 > > > > --- a/drivers/perf/riscv_pmu_sbi.c > > > > +++ b/drivers/perf/riscv_pmu_sbi.c > > > > @@ -20,10 +20,21 @@ > > > > #include <linux/cpu_pm.h> > > > > #include <linux/sched/clock.h> > > > > > > > > -#include <asm/errata_list.h> > > > > #include <asm/sbi.h> > > > > #include <asm/cpufeature.h> > > > > > > > > +#define THEAD_C9XX_RV_IRQ_PMU 17 > > > > +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 > > > > + > > > > +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ > > > > +asm volatile(ALTERNATIVE( \ > > > > + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ > > > > + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ > > > > + 0, RISCV_ISA_EXT_XTHEADPMU, \ > > > > + CONFIG_THEAD_CUSTOM_PMU) \ > > > > + : "=r" (__ovl) : \ > > > > + : "memory") > > > > + > > > > #define SYSCTL_NO_USER_ACCESS 0 > > > > #define SYSCTL_USER_ACCESS 1 > > > > #define SYSCTL_LEGACY 2 > > > > @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde > > > > if (riscv_isa_extension_available(NULL, SSCOFPMF)) { > > > > riscv_pmu_irq_num = RV_IRQ_PMU; > > > > riscv_pmu_use_irq = true; > > > > - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && > > > > - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && > > > > - riscv_cached_marchid(0) == 0 && > > > > - riscv_cached_mimpid(0) == 0) { > > > > + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && > > > > + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { > > > > riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; > > > > riscv_pmu_use_irq = true; > > > > } > > > > -- > > > > 2.34.1 > > > > > > > > > > > > > -- > > > Regards, > > > Atish > > > > -- > Regards, > Atish
diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata index e2c731cfed8c..0d19f47d1018 100644 --- a/arch/riscv/Kconfig.errata +++ b/arch/riscv/Kconfig.errata @@ -86,17 +86,4 @@ config ERRATA_THEAD_CMO If you don't know what to do here, say "Y". -config ERRATA_THEAD_PMU - bool "Apply T-Head PMU errata" - depends on ERRATA_THEAD && RISCV_PMU_SBI - default y - help - The T-Head C9xx cores implement a PMU overflow extension very - similar to the core SSCOFPMF extension. - - This will apply the overflow errata to handle the non-standard - behaviour via the regular SBI PMU driver and interface. - - If you don't know what to do here, say "Y". - endmenu # "CPU errata selection" diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c index 0554ed4bf087..5de5f7209132 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -53,22 +53,6 @@ static bool errata_probe_cmo(unsigned int stage, return true; } -static bool errata_probe_pmu(unsigned int stage, - unsigned long arch_id, unsigned long impid) -{ - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PMU)) - return false; - - /* target-c9xx cores report arch_id and impid as 0 */ - if (arch_id != 0 || impid != 0) - return false; - - if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) - return false; - - return true; -} - static u32 thead_errata_probe(unsigned int stage, unsigned long archid, unsigned long impid) { @@ -80,9 +64,6 @@ static u32 thead_errata_probe(unsigned int stage, if (errata_probe_cmo(stage, archid, impid)) cpu_req_errata |= BIT(ERRATA_THEAD_CMO); - if (errata_probe_pmu(stage, archid, impid)) - cpu_req_errata |= BIT(ERRATA_THEAD_PMU); - return cpu_req_errata; } diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h index 4ed21a62158c..9bccc2ba0eb5 100644 --- a/arch/riscv/include/asm/errata_list.h +++ b/arch/riscv/include/asm/errata_list.h @@ -25,8 +25,7 @@ #ifdef CONFIG_ERRATA_THEAD #define ERRATA_THEAD_PBMT 0 #define ERRATA_THEAD_CMO 1 -#define ERRATA_THEAD_PMU 2 -#define ERRATA_THEAD_NUMBER 3 +#define ERRATA_THEAD_NUMBER 2 #endif #ifdef __ASSEMBLY__ @@ -147,18 +146,6 @@ asm volatile(ALTERNATIVE_2( \ "r"((unsigned long)(_start) + (_size)) \ : "a0") -#define THEAD_C9XX_RV_IRQ_PMU 17 -#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 - -#define ALT_SBI_PMU_OVERFLOW(__ovl) \ -asm volatile(ALTERNATIVE( \ - "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ - "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ - THEAD_VENDOR_ID, ERRATA_THEAD_PMU, \ - CONFIG_ERRATA_THEAD_PMU) \ - : "=r" (__ovl) : \ - : "memory") - #endif /* __ASSEMBLY__ */ #endif diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h index 5340f818746b..480f9da7fba7 100644 --- a/arch/riscv/include/asm/hwcap.h +++ b/arch/riscv/include/asm/hwcap.h @@ -80,6 +80,7 @@ #define RISCV_ISA_EXT_ZFA 71 #define RISCV_ISA_EXT_ZTSO 72 #define RISCV_ISA_EXT_ZACAS 73 +#define RISCV_ISA_EXT_XTHEADPMU 74 #define RISCV_ISA_EXT_MAX 128 #define RISCV_ISA_EXT_INVALID U32_MAX diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index e32591e9da90..4aded5bf8fc3 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -303,6 +303,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL), __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT), __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT), + __RISCV_ISA_EXT_DATA(xtheadpmu, RISCV_ISA_EXT_XTHEADPMU), }; const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig index 273d67ecf6d2..6cef15ec7c25 100644 --- a/drivers/perf/Kconfig +++ b/drivers/perf/Kconfig @@ -86,6 +86,19 @@ config RISCV_PMU_SBI full perf feature support i.e. counter overflow, privilege mode filtering, counter configuration. +config THEAD_CUSTOM_PMU + bool "T-Head custom PMU support" + depends on ARCH_THEAD && RISCV_ALTERNATIVE && RISCV_PMU_SBI + default y + help + The T-Head C9xx cores implement a PMU overflow extension very + similar to the core SSCOFPMF extension. + + This will patch the overflow CSR and handle the non-standard + behaviour via the regular SBI PMU driver and interface. + + If you don't know what to do here, say "Y". + config ARM_PMU_ACPI depends on ARM_PMU && ACPI def_bool y diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index 2edbc37abadf..31ca79846399 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -20,10 +20,21 @@ #include <linux/cpu_pm.h> #include <linux/sched/clock.h> -#include <asm/errata_list.h> #include <asm/sbi.h> #include <asm/cpufeature.h> +#define THEAD_C9XX_RV_IRQ_PMU 17 +#define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5 + +#define ALT_SBI_PMU_OVERFLOW(__ovl) \ +asm volatile(ALTERNATIVE( \ + "csrr %0, " __stringify(CSR_SSCOUNTOVF), \ + "csrr %0, " __stringify(THEAD_C9XX_CSR_SCOUNTEROF), \ + 0, RISCV_ISA_EXT_XTHEADPMU, \ + CONFIG_THEAD_CUSTOM_PMU) \ + : "=r" (__ovl) : \ + : "memory") + #define SYSCTL_NO_USER_ACCESS 0 #define SYSCTL_USER_ACCESS 1 #define SYSCTL_LEGACY 2 @@ -808,10 +819,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde if (riscv_isa_extension_available(NULL, SSCOFPMF)) { riscv_pmu_irq_num = RV_IRQ_PMU; riscv_pmu_use_irq = true; - } else if (IS_ENABLED(CONFIG_ERRATA_THEAD_PMU) && - riscv_cached_mvendorid(0) == THEAD_VENDOR_ID && - riscv_cached_marchid(0) == 0 && - riscv_cached_mimpid(0) == 0) { + } else if (riscv_isa_extension_available(NULL, XTHEADPMU) && + IS_ENABLED(CONFIG_THEAD_CUSTOM_PMU)) { riscv_pmu_irq_num = THEAD_C9XX_RV_IRQ_PMU; riscv_pmu_use_irq = true; }