Hi Lorenzo,

-----Original Message-----
From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
Sent: Tuesday, January 30, 2018 6:00 PM
To: Shameerali Kolothum Thodi <redacted>
Cc: Neil Leeder <redacted>; Mark Langsdorf
[off-list ref]; Jon Masters [off-list ref]; Timur Tabi
[off-list ref]; linux-kernel at vger.kernel.org; Mark Brown
[off-list ref]; Mark Salter [off-list ref]; linux-arm-
kernel at lists.infradead.org; Will Deacon [off-list ref]; Mark
Rutland [off-list ref]; Linuxarm [off-list ref]
Subject: Re: [PATCH 1/2] acpi: arm64: add iort support for PMCG

On Tue, Jan 30, 2018 at 10:39:20AM +0000, Shameerali Kolothum Thodi wrote:

quoted

Hi Neil/Lorenzo,

quoted

-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-

bounces at lists.infradead.org]

quoted

quoted

On Behalf Of Neil Leeder
Sent: Friday, August 04, 2017 8:59 PM
To: Will Deacon <redacted>; Mark Rutland
[off-list ref]
Cc: Mark Langsdorf <redacted>; Jon Masters
[off-list ref]; Timur Tabi [off-list ref]; linux-
kernel at vger.kernel.org; Mark Brown [off-list ref]; Mark Salter
[off-list ref]; nleeder at codeaurora.org; linux-arm-
kernel at lists.infradead.org
Subject: [PATCH 1/2] acpi: arm64: add iort support for PMCG

Add support for the SMMU Performance Monitor Counter Group
information from ACPI. This is in preparation for its use
in the SMMU v3 PMU driver.

[...]

quoted

 static __init
 const struct iort_iommu_config *iort_get_iommu_cfg(struct

acpi_iort_node

quoted

quoted

*node)
 {

@@ -1001,6 +1041,8 @@ const struct iort_iommu_config

*iort_get_iommu_cfg(struct acpi_iort_node *node)
 		return &iort_arm_smmu_v3_cfg;
 	case ACPI_IORT_NODE_SMMU:
 		return &iort_arm_smmu_cfg;
+	case ACPI_IORT_NODE_PMCG:
+		return &iort_arm_smmu_pmcg_cfg;

I understand this series will be revised based on the iort spec update.

This Is to clarify few queries we have with respect to one of our hisilicon
platform which has support for SMMU v3 PMCG. In our implementation
the base address for the PMCG is defined at a IMP DEF address offset
within the SMMUv3 page 0 address space. And as per SMMU spec,

" The Performance Monitor Counter Groups are standalone monitoring
facilities  and, as such, can be implemented in separate components that
are all associated with (but not necessarily part of) an SMMU"

It looks like PMCG can be part of SMMU,  it is not clear whether this kind
of address mapping is forbidden or not. If this is an accepted  scenario, then
the devm_ioremap_resource() call in SMMv3 probe will fail as it reports

conflict.

quoted

As far as I can see, the above code just checks the iort node type is PMCG
and assumes that its SMMU PMCG. As per IORT spec, it make sense to check
the node reference filed and make that call.

And to fix the resource conflict issue we have, is it possible to treat the PMCG
node as the child of the SMMU and call the platform_device_add()

appropriately

quoted

to avoid the conflict? I am not sure this will fix the issue and also about the

order

quoted

in which SMMU and PMCG devices will be populated will have an impact on

this.

quoted

Please let me know your thoughts on this.

I went back and re-read the patches, I think the point here is that the
perf driver (ie PATCH 2 that, by the way, is not maiinline) uses
devm_ioremap_resource() to map the counters and that's what is causing
failures when PMCG is part of SMMUv3 registers.

Thanks for going through this.  No, this is not where we are seeing the failure.
May be I was not clear in my earlier mail. The failure happens in SMMUv3
driver probe function when it calls devm_ioremap_resource().

It is the resources hierarchy that is wrong and in turn, I do not think
devm_request_mem_region() is the right way of requesting it for the
PMCG driver.

I need to look into this but I suspect that's something that should
be handled in the PMCG driver, that has to request the memory region
_differently_ (ie ioremap copes with this overlap - it is the
devm_request_mem_region() in devm_ioremap_resource() that fails, correct
?).

It looks like, in IORT code,

iort_add_platform_device()--> platform_device_add()-->insert_resource(), inserts
both SMMUv3 and PMCG resources into the resource tree and then when the probe
of SMMUv3 is called, it detects the conflict.

[   85.548749] arm-smmu-v3 arm-smmu-v3.0.auto: can't request region for resource [mem 0x148000000-0x14801ffff]                      

Of course, changing devm_ioremap_resource() to devm_ioremap() in SMMv3
driver probe solves the issue for us, but not sure that's the right approach or not.

Thanks,
Shameer

Certainly we need to create in IORT the resources with the correct
hierarchy (I reckon DT does it already if the right device hierarchy is
specified).