Re: [PATCH v2 1/5] driver/perf: Add identifier sysfs file for CMN

[John Garry]

On 04/05/2023 13:15, Robin Murphy wrote:
> > ok, I see, that's what I was asking about in the cmn-700 JSON review; 
> > and from what you say, it is not the case that we always have the same 
> > events for every revision. So we need a more fine-grained identifier.
>
> Yes, it's mostly just a case of new events getting added as the 
> microarchitecture evolves over the product's lifetime,

I might have mentioned this before: Currently userspace matches both the 
PMU sysfs name and this HW identifier for creating these event aliases. 
For fixed events - like those non-IMPDEF ones on the PMCG - userspace 
could just assume those standard ones are implemented and not need to 
check the HW identifier. Maybe this will help with scalability of JSONs 
for CMN PMU implementations. Or even HW identifier wildcard matching. 
However, as you might anticipate, we may need to add some sort of 
userspace support for times when HW implementations get messed up...

> but there has 
> been at least one event ID which had a meaning in very early versions of 
> CMN-600, was subsequently removed, and then got reused for a *different* 
> event a couple of revisions after that.
> Thankfully I believe those 
> earliest versions only ever existed on FPGA internally, and the TRMs 
> were never made public, so upstream doesn't care about that specific case.

ok, good

> > For DT support, I suppose per-revision compat strings could be added, 
> > but I would not be sure what to do about ACPI.
>
> We know the version from the ID registers, that's no problem - it's 
> already used to manage visibility of the sysfs event aliases. In 
> principle we *should* have a model code in CFGM_PERIPH_ID_0 as well, and 
> be able to compose an identifier exactly the same way as 
> smmu_pmu_get_iidr() does in the SMMUv3 PMCG driver, but as I mentioned 
> before I'm not entirely confident in the implementation: I do happen to 
> know what codes have been nominally assigned for each product, but the 
> TRMs claim otherwise 🙁

Thanks,
John