Re: [PATCH 0/2] perf/smmuv3: Support devicetree

[Robin Murphy <robin.murphy@arm.com>]

On 2021-11-16 15:42, Jean-Philippe Brucker wrote:
> On Tue, Nov 16, 2021 at 12:02:47PM +0000, Robin Murphy wrote:
>> On 2021-11-16 11:35, Jean-Philippe Brucker wrote:
>>> Add devicetree binding for the SMMUv3 PMU, called Performance Monitoring
>>> Counter Group (PMCG) in the spec. Each SMMUv3 implementation can have
>>> multiple independent PMCGs, for example one for the Translation Control
>>> Unit (TCU) and one per Translation Buffer Unit (TBU).
>>>
>>> I previously sent the binding as reply to Jay Chen's thread implementing
>>> device tree support [1]. This posting addresses the comments from that
>>> thread.
>>
>> Ha, I'd also resurrected this and was planning to post it at some point this
>> week[0] - you should have said :)
> 
> Ah sorry about that, I just resent because there was some demand for it at
> Linaro

Heh, no worries - it's not like you were even CC'ed on the thread where 
I only mentioned I *might* do it.

Can I get away with being cheeky and just saying that my review comments 
are the diff between my branch and yours, I wonder...

>>> Patch 1 adds two compatible strings. "arm,smmu-v3-pmcg" is common to all
>>> PMCGs. "hisilicon,smmu-v3-pmcg-hip08" allows to support the same quirk
>>> as IORT for that implementation (see patch 2). We'll probably want to
>>> also introduce compatible strings for each implementation that has
>>> additional perf events. For example the MMU-600 implementation has
>>> different events for TCU and TBU PMCGs [2], but both components have the
>>> same device IDs. So the driver could differentiate them if they had two
>>> distinct compatible strings such as "arm,mmu-600-pmcg-tbu" and
>>> "arm,mmu-600-pmcg-tcu".
>>
>> Actually it only needs a general MMU-600 compatible, since once you know
>> it's an Arm Ltd. implementation, you can assume the pattern for the IMP_DEF
>> ID registers to figure out the rest.
> 
> It might be an error in the MMU-600 spec specifically, both TBU and TCU
> PMU registers have a 0x83 PIDR0, where I think the TBU should be 0x84 (the
> revC model uses that value). It's possible that the implementation
> actually has 0x84 instead.

Yup, it's a mistake in the TRM. I just checked a real MMU-600 and the 
PMU PIDRs match the main TCU/TBU PIDRs as expected. At least the MMU-700 
docs haven't repeated the same error.

Cheers,
Robin.