Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 1 Jun 2023 23:47:39 -0700 (PDT) | From | Ilkka Koskinen <> | Subject | Re: [PATCH v2 1/5] perf: arm_cspmu: Support 32-bit accesses to 64-bit registers |
| |
Hi Robin,
On Thu, 1 Jun 2023, Robin Murphy wrote: > On 2023-06-01 04:01, Ilkka Koskinen wrote: >> Split the 64-bit register accesses if 64-bit access is not supported >> by the PMU. >> >> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> >> --- >> drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++-- >> drivers/perf/arm_cspmu/arm_cspmu.h | 1 + >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c >> b/drivers/perf/arm_cspmu/arm_cspmu.c >> index a3f1c410b417..88547a2b73e6 100644 >> --- a/drivers/perf/arm_cspmu/arm_cspmu.c >> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c >> @@ -701,8 +701,12 @@ static void arm_cspmu_write_counter(struct perf_event >> *event, u64 val) >> if (use_64b_counter_reg(cspmu)) { >> offset = counter_offset(sizeof(u64), event->hw.idx); >> - >> - writeq(val, cspmu->base1 + offset); >> + if (!cspmu->impl.split_64bit_access) { > > Could we not just hang this off the 64-bit atomicity property to match the > read path? It doesn't seem like there's much benefit in micro-optimising for > whether the interconnect splits 64-bit accesses into 32-bit bursts vs. just > not accepting them at all.
Sure, I was actually wondering if I could use it or if there could be a really weird hw implementation. I'll change it.
> >> + writeq(val, cspmu->base1 + offset); >> + } else { >> + writel(lower_32_bits(val), cspmu->base1 + offset); >> + writel(upper_32_bits(val), cspmu->base1 + offset + >> 4); > > lo_hi_writeq() - the header's already included for 32-bit build coverage, so > we may as well put it to use :)
Oh, I have missed that function completely. I fix this too.
Cheers, Ilkka
> Thanks, > Robin. > >> + } >> } else { >> offset = counter_offset(sizeof(u32), event->hw.idx); >> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h >> b/drivers/perf/arm_cspmu/arm_cspmu.h >> index 51323b175a4a..c0412cf2bd97 100644 >> --- a/drivers/perf/arm_cspmu/arm_cspmu.h >> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h >> @@ -110,6 +110,7 @@ struct arm_cspmu_impl_ops { >> /* Vendor/implementer descriptor. */ >> struct arm_cspmu_impl { >> u32 pmiidr; >> + bool split_64bit_access; >> struct arm_cspmu_impl_ops ops; >> void *ctx; >> }; >
| ![\](/images/icornerr.gif) |