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

[Jing Zhang <renyu.zj@linux.alibaba.com>]

在 2023/5/5 下午8:24, 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
> 

Thanks John and Robin, perhaps using CMN's model and revision to form an identifier is currently the most feasible solution?
For example: CMN600_R1P0, CMN600_R1P1. The revision of CMN can be obtained from the revision register.

>>
>>> For DT support, I suppose per-revision compat strings could be added, but I would not be sure what to do about ACPI.
>>

Yes, I also have this doubt. On DT, char* can be passed in through the data field because it is of void* type, but the driver_data
field of ACPI is ulong. In addition, the revision is read out through the register, and I cannot get the revision in
arm_cmn_acpi/of_match[]. We still need to compose the identifier in the probe function. So, I wonder if it is better to use an
additional function to form the identifier instead of passing the identifier through arm_cmn_acpi/of_match[]?

>> 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


Thanks,
Jing