Messages in this thread | | | From | Anup Patel <> | Date | Thu, 25 Aug 2022 08:21:46 +0530 | Subject | Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores |
| |
On Thu, Aug 25, 2022 at 7:34 AM Heiko Stuebner <heiko@sntech.de> wrote: > > Am Donnerstag, 18. August 2022, 10:24:33 CEST schrieb Anup Patel: > > On Thu, Aug 18, 2022 at 1:03 AM Atish Patra <atishp@atishpatra.org> wrote: > > > > > > On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote: > > > > > > > > With the T-HEAD C9XX cores being designed before or during the ratification > > > > to the SSCOFPMF extension, they implement functionality very similar but > > > > not equal to it. So add some adaptions to allow the C9XX to still handle > > > > its PMU through the regular SBI PMU interface instead of defining new > > > > interfaces or drivers. > > > > > > > > > > IMO, vendor specific workarounds in the generic implementation is not > > > a good idea. > > > If we have to support it, I think we should just put the IRQ number in > > > the DT and parse from the DT. > > > The initial sscofpmf series was based on the DT. It was removed later > > > as there was no need for it at that time. > > > We can always revive it. > > > > > > Regarding the CSR number difference and static key enablement, can we > > > use code patching techniques here as well ? > > > At least all the T-HEAD C9XX core erratas are in one place. > > > > > > The alternate would be just to say T-HEAD C9XX support SSCOFPMF but > > > with erratas. I don't prefer this approach > > > but it keeps the vendor specific checks out of the generic code. > > > > Whether to have a DT node (or not) was already discussed and concluded > > in the past. > > > > We don't need a DT node just to get the IRQ number. The T-HEAD custom > > IRQ number can be derived based on mvendorid. > > Yeah, I remember reading that discussion and thus went with the mvendorid > way in this patch. > > > Also, all these T-HEAD specific changes in SBI PMU driver should be > > implemented as erratas using ALTERNATIVE() macros. > > (1) "All these T-HEAD specific changes ..." > Actually the only T-HEAD-specific change is reading that different CSR > for the overflow information, the rest only makes the irq-number variable
If it is just overflow CSR then it is simpler to do instruction patching in drivers/perf/riscv_pmu_sbi.c itself.
> > (2) ALTERNATIVE macros are working on assembler instructions, so are you > sugesting to replace the generic csr_read() for the overflow-csr with a > custom copy like > > #define sbi_pmu_read_overflow(void) \ > ({ \ > register unsigned long __v; \ > ALT_THEAD_PMU_OVERFLOW(__v); \ > __v; \ > }) > > and then in errata_list.h > > #define ALT_THEAD_PMU_OVERFLOW(__ovl) \ > __asm__ __volatile__ (alternative( > "csrr %0, " __ASM_STR(CSR_SSCOUNTOVF), \ > "csrr %0, " __ASM_STR(THEAD_C9XX_CSR_SCOUNTEROF), THEAD_VENDOR_ID, \ > ERRATA_THEAD_PMU, CONFIG_ERRATA_THEAD_PMU) \ > : "=r" (__ovl) : \ > : "memory"); > > I'm not yet seeing what you're gaining by going with this approach: > - that the overflow-csr is the same bitwise but only at a different > address is specific to the c9xx, other deviants might implement things > completely different > - you're not getting rid of the thead mention > - and we're now duplicating the generic csr-read code > > Is this the preferred way or what am I overlooking?
Yes, better to have special sbi_pmu_read_overflow() in drivers/perf/riscv_pmu_sbi.c itself which is based on ALTERNATIVES.
I am suggesting ALTERNATIVEs only because overflow CSR is accessed in the interrupt handler which is in hot-path when we run "perf record".
Regards, Anup
| |