| Message ID | 20230601030144.3458136-2-ilkka@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: ampere: Add support for Ampere SoC PMUs | expand |
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. > + 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 :) 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; > };
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; >> }; >
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) { + writeq(val, cspmu->base1 + offset); + } else { + writel(lower_32_bits(val), cspmu->base1 + offset); + writel(upper_32_bits(val), cspmu->base1 + offset + 4); + } } 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; };
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(-)