Messages in this thread | | | Date | Thu, 5 Nov 2020 13:27:05 +0000 | From | Mark Rutland <> | Subject | Re: [PATCH v3 1/3] arm64: wrap and generalise counter read functions |
| |
Hi Ionela,
On Thu, Nov 05, 2020 at 12:27:00PM +0000, Ionela Voinescu wrote: > -#ifdef CONFIG_ARM64_AMU_EXTN > +#define COUNTER_READ_STORE(NAME, VAL) \ > +static inline u64 read_##NAME(void) \ > +{ \ > + return VAL; \ > +} \ > +static inline void store_##NAME(void *val) \ > +{ \ > + *(u64 *)val = read_##NAME(); \ > +} > > -#undef pr_fmt > -#define pr_fmt(fmt) "AMU: " fmt > +#ifdef CONFIG_ARM64_AMU_EXTN > +COUNTER_READ_STORE(corecnt, read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0)); > +COUNTER_READ_STORE(constcnt, read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0)); > +#else > +COUNTER_READ_STORE(corecnt, 0); > +COUNTER_READ_STORE(constcnt, 0); > +#endif
I think this level of indirection is confusing (e.g. since you can't grep for `read_constcnt` to find the implementation). I also don't think we need to store_* functions here -- they're only needed by patch 3, and they can be wrapped in helper functions there (which should also solve the warning you mention in the cover letter).
I think if we need these wrappers, it'd be better to have:
#ifdef CONFIG_ARM64_AMU_EXTN #define read_corecnt() read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0) #define read_constcnt() read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0) #else #define read_corecnt() (0UL) #define read_constcnt() (0UL) #endif
Thanks, Mark.
| |