> On 22-Jul-2020, at 4:19 PM, Jordan Niethe <jniet...@gmail.com> wrote: > > On Wed, Jul 22, 2020 at 5:55 PM Athira Rajeev > <atraj...@linux.vnet.ibm.com <mailto:atraj...@linux.vnet.ibm.com>> wrote: >> >> >> >> On 22-Jul-2020, at 10:11 AM, Jordan Niethe <jniet...@gmail.com> wrote: >> >> On Sat, Jul 18, 2020 at 1:13 AM Athira Rajeev >> <atraj...@linux.vnet.ibm.com> wrote: >> >> >> From: Madhavan Srinivasan <ma...@linux.ibm.com> >> >> Add power10 feature function to dt_cpu_ftrs.c along >> with a power10 specific init() to initialize pmu sprs, >> sets the oprofile_cpu_type and cpu_features. This will >> enable performance monitoring unit(PMU) for Power10 >> in CPU features with "performance-monitor-power10". >> >> For PowerISA v3.1, BHRB disable is controlled via Monitor Mode >> Control Register A (MMCRA) bit, namely "BHRB Recording Disable >> (BHRBRD)". This patch initializes MMCRA BHRBRD to disable BHRB >> feature at boot for power10. >> >> Signed-off-by: Madhavan Srinivasan <ma...@linux.ibm.com> >> --- >> arch/powerpc/include/asm/reg.h | 3 +++ >> arch/powerpc/kernel/cpu_setup_power.S | 8 ++++++++ >> arch/powerpc/kernel/dt_cpu_ftrs.c | 26 ++++++++++++++++++++++++++ >> 3 files changed, 37 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h >> index 21a1b2d..900ada1 100644 >> --- a/arch/powerpc/include/asm/reg.h >> +++ b/arch/powerpc/include/asm/reg.h >> @@ -1068,6 +1068,9 @@ >> #define MMCR0_PMC2_LOADMISSTIME 0x5 >> #endif >> >> +/* BHRB disable bit for PowerISA v3.10 */ >> +#define MMCRA_BHRB_DISABLE 0x0000002000000000 >> >> Shouldn't this go under SPRN_MMCRA with the other MMCRA_*. >> >> >> >> Hi Jordan >> >> Ok, the definition of MMCRA is under #ifdef for 64 bit . if I move >> definition of MMCRA_BHRB_DISABLE along with other SPR's, I also >> need to define this for 32-bit to satisfy core-book3s to compile as below: >> >> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h >> index 900ada10762c..7e271657b412 100644 >> --- a/arch/powerpc/include/asm/reg.h >> +++ b/arch/powerpc/include/asm/reg.h >> @@ -888,6 +888,8 @@ >> #define MMCRA_SLOT 0x07000000UL /* SLOT bits (37-39) */ >> #define MMCRA_SLOT_SHIFT 24 >> #define MMCRA_SAMPLE_ENABLE 0x00000001UL /* enable sampling */ >> +/* BHRB disable bit for PowerISA v3.10 */ >> +#define MMCRA_BHRB_DISABLE 0x0000002000000000 >> #define POWER6_MMCRA_SDSYNC 0x0000080000000000ULL /* SDAR/SIAR synced */ >> #define POWER6_MMCRA_SIHV 0x0000040000000000ULL >> #define POWER6_MMCRA_SIPR 0x0000020000000000ULL >> @@ -1068,9 +1070,6 @@ >> #define MMCR0_PMC2_LOADMISSTIME 0x5 >> #endif >> >> >> >> -/* BHRB disable bit for PowerISA v3.10 */ >> -#define MMCRA_BHRB_DISABLE 0x0000002000000000 >> - >> /* >> * SPRG usage: >> * >> diff --git a/arch/powerpc/perf/core-book3s.c >> b/arch/powerpc/perf/core-book3s.c >> index 36baae666387..88068f20827c 100644 >> --- a/arch/powerpc/perf/core-book3s.c >> +++ b/arch/powerpc/perf/core-book3s.c >> @@ -94,6 +94,7 @@ static unsigned int freeze_events_kernel = MMCR0_FCS; >> #define SPRN_SIER2 0 >> #define SPRN_SIER3 0 >> #define MMCRA_SAMPLE_ENABLE 0 >> +#define MMCRA_BHRB_DISABLE 0 >> >> >> >> static inline unsigned long perf_ip_adjust(struct pt_regs *regs) >> { >> >> >> >> + >> /* >> * SPRG usage: >> * >> diff --git a/arch/powerpc/kernel/cpu_setup_power.S >> b/arch/powerpc/kernel/cpu_setup_power.S >> index efdcfa7..b8e0d1e 100644 >> --- a/arch/powerpc/kernel/cpu_setup_power.S >> +++ b/arch/powerpc/kernel/cpu_setup_power.S >> @@ -94,6 +94,7 @@ _GLOBAL(__restore_cpu_power8) >> _GLOBAL(__setup_cpu_power10) >> mflr r11 >> bl __init_FSCR_power10 >> + bl __init_PMU_ISA31 >> >> So we set MMCRA here but then aren't we still going to call __init_PMU >> which will overwrite that? >> Would this setting MMCRA also need to be handled in __restore_cpu_power10? >> >> >> Thanks for this nice catch ! When I rebased code initial phase, we didn’t >> had power10 part filled in. >> It was a miss from my side in adding PMu init functions and thanks for >> pointing this out. >> Below patch will call __init_PMU functions in setup and restore. Please >> check if this looks good >> >> -- >> diff --git a/arch/powerpc/kernel/cpu_setup_power.S >> b/arch/powerpc/kernel/cpu_setup_power.S >> index efdcfa714106..e672a6c5fd7c 100644 >> --- a/arch/powerpc/kernel/cpu_setup_power.S >> +++ b/arch/powerpc/kernel/cpu_setup_power.S >> @@ -94,6 +94,9 @@ _GLOBAL(__restore_cpu_power8) >> _GLOBAL(__setup_cpu_power10) >> mflr r11 >> bl __init_FSCR_power10 >> + bl __init_PMU >> + bl __init_PMU_ISA31 >> + bl __init_PMU_HV >> b 1f >> >> _GLOBAL(__setup_cpu_power9) > > Won't you also need to change where the label 1 is: > --- a/arch/powerpc/kernel/cpu_setup_power.S > +++ b/arch/powerpc/kernel/cpu_setup_power.S > @@ -100,8 +100,8 @@ _GLOBAL(__setup_cpu_power10) > _GLOBAL(__setup_cpu_power9) > mflr r11 > bl __init_FSCR > -1: bl __init_PMU > - bl __init_hvmode_206 > + bl __init_PMU > +1: bl __init_hvmode_206 > mtlr r11 > beqlr > li r0,0
HI Jordan I will address these comments and include changes for cpu_setup_power.S in a separate patch as suggested by Michael Ellerman Thanks Athira > >> @@ -124,6 +127,9 @@ _GLOBAL(__setup_cpu_power9) >> _GLOBAL(__restore_cpu_power10) >> mflr r11 >> bl __init_FSCR_power10 >> + bl __init_PMU >> + bl __init_PMU_ISA31 >> + bl __init_PMU_HV >> b 1f >> >> _GLOBAL(__restore_cpu_power9) >> @@ -233,3 +239,10 @@ __init_PMU_ISA207: >> li r5,0 >> mtspr SPRN_MMCRS,r5 >> blr >> + >> +__init_PMU_ISA31: >> + li r5,0 >> + mtspr SPRN_MMCR3,r5 >> + LOAD_REG_IMMEDIATE(r5, MMCRA_BHRB_DISABLE) >> + mtspr SPRN_MMCRA,r5 >> + blr >> >> — >> >> b 1f >> >> _GLOBAL(__setup_cpu_power9) >> @@ -233,3 +234,10 @@ __init_PMU_ISA207: >> li r5,0 >> mtspr SPRN_MMCRS,r5 >> blr >> + >> +__init_PMU_ISA31: >> + li r5,0 >> + mtspr SPRN_MMCR3,r5 >> + LOAD_REG_IMMEDIATE(r5, MMCRA_BHRB_DISABLE) >> + mtspr SPRN_MMCRA,r5 >> + blr >> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c >> b/arch/powerpc/kernel/dt_cpu_ftrs.c >> index 3a40951..f482286 100644 >> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c >> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c >> @@ -450,6 +450,31 @@ static int __init feat_enable_pmu_power9(struct >> dt_cpu_feature *f) >> return 1; >> } >> >> +static void init_pmu_power10(void) >> +{ >> + init_pmu_power9(); >> + >> + mtspr(SPRN_MMCR3, 0); >> + mtspr(SPRN_MMCRA, MMCRA_BHRB_DISABLE); >> +} >> + >> +static int __init feat_enable_pmu_power10(struct dt_cpu_feature *f) >> +{ >> + hfscr_pmu_enable(); >> + >> + init_pmu_power10(); >> + init_pmu_registers = init_pmu_power10; >> + >> + cur_cpu_spec->cpu_features |= CPU_FTR_MMCRA; >> + cur_cpu_spec->cpu_user_features |= >> PPC_FEATURE_PSERIES_PERFMON_COMPAT; >> + >> + cur_cpu_spec->num_pmcs = 6; >> + cur_cpu_spec->pmc_type = PPC_PMC_IBM; >> + cur_cpu_spec->oprofile_cpu_type = "ppc64/power10"; >> + >> + return 1; >> +} >> + >> static int __init feat_enable_tm(struct dt_cpu_feature *f) >> { >> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM >> @@ -639,6 +664,7 @@ struct dt_cpu_feature_match { >> {"pc-relative-addressing", feat_enable, 0}, >> {"machine-check-power9", feat_enable_mce_power9, 0}, >> {"performance-monitor-power9", feat_enable_pmu_power9, 0}, >> + {"performance-monitor-power10", feat_enable_pmu_power10, 0}, >> {"event-based-branch-v3", feat_enable, 0}, >> {"random-number-generator", feat_enable, 0}, >> {"system-call-vectored", feat_disable, 0}, >> -- >> 1.8.3.1