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

Hi Neil/Lorenzo,

quoted

-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org]
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
*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.

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
to avoid the conflict? I am not sure this will fix the issue and also about the order
in which SMMU and PMCG devices will be populated will have an impact on this.

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.

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
?).

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

Lorenzo

Thanks,
Shameer
  

quoted

 	default:
 		return NULL;
 	}

@@ -1056,6 +1098,15 @@ static int __init

iort_add_smmu_platform_device(struct acpi_iort_node *node)
 	if (ret)
 		goto dev_put;

+	/* End of init for PMCG */
+	if (node->type == ACPI_IORT_NODE_PMCG) {
+		ret = platform_device_add(pdev);
+		if (ret)
+			goto dev_put;
+
+		return 0;
+	}
+
 	/*
 	 * We expect the dma masks to be equivalent for
 	 * all SMMUs set-ups

@@ -1131,6 +1182,9 @@ static void __init iort_init_platform_devices(void)
 				acpi_free_fwnode_static(fwnode);
 				return;
 			}
+		} else if (iort_node->type == ACPI_IORT_NODE_PMCG) {
+			if (iort_add_smmu_platform_device(iort_node))
+				return;
 		}

 		iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort_node,

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 707dda74..2169b6f 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h

@@ -695,7 +695,8 @@ enum acpi_iort_node_type {
 	ACPI_IORT_NODE_NAMED_COMPONENT = 0x01,
 	ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
 	ACPI_IORT_NODE_SMMU = 0x03,
-	ACPI_IORT_NODE_SMMU_V3 = 0x04
+	ACPI_IORT_NODE_SMMU_V3 = 0x04,
+	ACPI_IORT_NODE_PMCG = 0x05
 };

 struct acpi_iort_id_mapping {

@@ -811,6 +812,12 @@ struct acpi_iort_smmu_v3 {
 #define ACPI_IORT_SMMU_V3_COHACC_OVERRIDE   (1)
 #define ACPI_IORT_SMMU_V3_HTTU_OVERRIDE     (1<<1)

+struct acpi_iort_pmcg {
+	u64 base_address;	/* PMCG base address */
+	u32 overflow_gsiv;
+	u32 node_reference;
+};
+

/****************************************************************
***************
  *
  * IVRS - I/O Virtualization Reporting Structure
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel