[RFC PATCH 0/6] SMMUv3 PMCG IMP DEF event support

[RFC PATCH 0/6] SMMUv3 PMCG IMP DEF event support

[John Garry]

This patchset adds IMP DEF event support for the SMMUv3 PMCG.

It is marked as an RFC as the method to identify the PMCG implementation
may be a quite disliked. And, in general, the series is somewhat
incomplete.

So the background is that the PMCG supports IMP DEF events, yet we have no
method to identify the PMCG to know the IMP DEF events.

A method for identifying the PMCG implementation could be using
PMDEVARCH, but we cannot rely on this being set properly, as whether this
is implemented is not defined in SMMUv3 spec.

Another method would be perf event aliasing, but this method of event
matching is based on CPU id, which would not guarantee same
uniqueness as PMCG implementation.

Yet another method could be to continue using ACPI OEM ID in the IORT
code, but this does not scale. And it is not suitable if we ever add DT
support to the PMCG driver.

The method used in this series is based on matching on the parent SMMUv3
IIDR. We store this IIDR contents in the arm smmu structure as the first
element, which means that we don't have to expose SMMU APIs - this is
the part which may be disliked.

The final two patches switch the pre-existing PMCG model identification
from ACPI OEM ID to the same parent SMMUv3 IIDR matching.

For now, we only consider SMMUv3' nodes being the associated node for
PMCG.

John Garry (6):
  ACPI/IORT: Set PMCG device parent
  iommu/arm-smmu-v3: Record IIDR in arm_smmu_device structure
  perf/smmuv3: Retrieve parent SMMUv3 IIDR
  perf/smmuv3: Support HiSilicon hip08 (hi1620) IMP DEF events
  perf/smmuv3: Match implementation options based on parent SMMU IIDR
  ACPI/IORT: Drop code to set the PMCG software-defined model

 drivers/acpi/arm64/iort.c     | 69 ++++++++++++++--------------
 drivers/iommu/arm-smmu-v3.c   |  5 +++
 drivers/perf/arm_smmuv3_pmu.c | 84 ++++++++++++++++++++++++++++++-----
 include/linux/acpi_iort.h     |  8 ----
 4 files changed, 112 insertions(+), 54 deletions(-)

-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH 1/6] ACPI/IORT: Set PMCG device parent

[John Garry]

In the IORT, a PMCG node includes a node reference to its associated
device.

Set the PMCG platform device parent device for future referencing.

For now, we only consider setting for when the associated component is an
SMMUv3.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/acpi/arm64/iort.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 8569b79e8b58..0b687520c3e7 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1455,7 +1455,7 @@ static __init const struct iort_dev_config *iort_get_dev_cfg(
  * Returns: 0 on success, <0 failure
  */
 static int __init iort_add_platform_device(struct acpi_iort_node *node,
-					   const struct iort_dev_config *ops)
+					   const struct iort_dev_config *ops, struct device *parent)
 {
 	struct fwnode_handle *fwnode;
 	struct platform_device *pdev;
@@ -1466,6 +1466,8 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
 	if (!pdev)
 		return -ENOMEM;
 
+	pdev->dev.parent = parent;
+
 	if (ops->dev_set_proximity) {
 		ret = ops->dev_set_proximity(&pdev->dev, node);
 		if (ret)
@@ -1573,6 +1575,11 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
 static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
 #endif
 
+static int iort_fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev->fwnode == fwnode;
+}
+
 static void __init iort_init_platform_devices(void)
 {
 	struct acpi_iort_node *iort_node, *iort_end;
@@ -1594,11 +1601,34 @@ static void __init iort_init_platform_devices(void)
 				iort_table->length);
 
 	for (i = 0; i < iort->node_count; i++) {
+		struct device *parent = NULL;
+
 		if (iort_node >= iort_end) {
 			pr_err("iort node pointer overflows, bad table\n");
 			return;
 		}
 
+		/* Fixme: handle parent declared in IORT after PMCG */
+		if (iort_node->type == ACPI_IORT_NODE_PMCG) {
+			struct acpi_iort_node *iort_assoc_node;
+			struct acpi_iort_pmcg *pmcg;
+			u32 node_reference;
+
+			pmcg = (struct acpi_iort_pmcg *)iort_node->node_data;
+
+			node_reference = pmcg->node_reference;
+			iort_assoc_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+				 node_reference);
+
+			if (iort_assoc_node->type == ACPI_IORT_NODE_SMMU_V3) {
+				struct fwnode_handle *assoc_fwnode;
+
+				assoc_fwnode = iort_get_fwnode(iort_assoc_node);
+
+				parent = bus_find_device(&platform_bus_type, NULL,
+				      assoc_fwnode, iort_fwnode_match);
+			}
+		}
 		iort_enable_acs(iort_node);
 
 		ops = iort_get_dev_cfg(iort_node);
@@ -1609,7 +1639,7 @@ static void __init iort_init_platform_devices(void)
 
 			iort_set_fwnode(iort_node, fwnode);
 
-			ret = iort_add_platform_device(iort_node, ops);
+			ret = iort_add_platform_device(iort_node, ops, parent);
 			if (ret) {
 				iort_delete_fwnode(iort_node);
 				acpi_free_fwnode_static(fwnode);
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 1/6] ACPI/IORT: Set PMCG device parent

[Hanjun Guo]

Hi John,

On 2019/9/30 22:33, John Garry wrote:
> In the IORT, a PMCG node includes a node reference to its associated
> device.
> 
> Set the PMCG platform device parent device for future referencing.
> 
> For now, we only consider setting for when the associated component is an
> SMMUv3.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/acpi/arm64/iort.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 8569b79e8b58..0b687520c3e7 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1455,7 +1455,7 @@ static __init const struct iort_dev_config *iort_get_dev_cfg(
>   * Returns: 0 on success, <0 failure

...

>   */
>  static int __init iort_add_platform_device(struct acpi_iort_node *node,
> -					   const struct iort_dev_config *ops)
> +					   const struct iort_dev_config *ops, struct device *parent)

Since you added a input for this function, could you please update
the comments of this function as well?

>  {
>  	struct fwnode_handle *fwnode;
>  	struct platform_device *pdev;
> @@ -1466,6 +1466,8 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>  	if (!pdev)
>  		return -ENOMEM;
>  
> +	pdev->dev.parent = parent;
> +
>  	if (ops->dev_set_proximity) {
>  		ret = ops->dev_set_proximity(&pdev->dev, node);
>  		if (ret)
> @@ -1573,6 +1575,11 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
>  static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
>  #endif
>  
> +static int iort_fwnode_match(struct device *dev, const void *fwnode)
> +{
> +	return dev->fwnode == fwnode;
> +}
> +
>  static void __init iort_init_platform_devices(void)
>  {
>  	struct acpi_iort_node *iort_node, *iort_end;
> @@ -1594,11 +1601,34 @@ static void __init iort_init_platform_devices(void)
>  				iort_table->length);
>  
>  	for (i = 0; i < iort->node_count; i++) {
> +		struct device *parent = NULL;
> +
>  		if (iort_node >= iort_end) {
>  			pr_err("iort node pointer overflows, bad table\n");
>  			return;
>  		}
>  
> +		/* Fixme: handle parent declared in IORT after PMCG */
> +		if (iort_node->type == ACPI_IORT_NODE_PMCG) {
> +			struct acpi_iort_node *iort_assoc_node;
> +			struct acpi_iort_pmcg *pmcg;
> +			u32 node_reference;
> +
> +			pmcg = (struct acpi_iort_pmcg *)iort_node->node_data;
> +
> +			node_reference = pmcg->node_reference;
> +			iort_assoc_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
> +				 node_reference);
> +
> +			if (iort_assoc_node->type == ACPI_IORT_NODE_SMMU_V3) {
> +				struct fwnode_handle *assoc_fwnode;
> +
> +				assoc_fwnode = iort_get_fwnode(iort_assoc_node);
> +
> +				parent = bus_find_device(&platform_bus_type, NULL,
> +				      assoc_fwnode, iort_fwnode_match);
> +			}
> +		}

How about using a function to include those new added code to make this
function (iort_init_platform_devices()) a bit cleaner?

>  		iort_enable_acs(iort_node);
>  
>  		ops = iort_get_dev_cfg(iort_node);
> @@ -1609,7 +1639,7 @@ static void __init iort_init_platform_devices(void)
>  
>  			iort_set_fwnode(iort_node, fwnode);
>  
> -			ret = iort_add_platform_device(iort_node, ops);
> +			ret = iort_add_platform_device(iort_node, ops, parent);

This function is called if ops is valid, so retrieve the parent
can be done before this function I think.

Thanks
Hanjun

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 1/6] ACPI/IORT: Set PMCG device parent

[John Garry]

Hi Hanjun,

Thanks for checking this.

>>   */
>>  static int __init iort_add_platform_device(struct acpi_iort_node *node,
>> -					   const struct iort_dev_config *ops)
>> +					   const struct iort_dev_config *ops, struct device *parent)
>
> Since you added a input for this function, could you please update
> the comments of this function as well?

Right, that can be updated. Indeed, the current comment omit the @ops 
argument also.

>
>>  {
>>  	struct fwnode_handle *fwnode;
>>  	struct platform_device *pdev;
>> @@ -1466,6 +1466,8 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>>  	if (!pdev)
>>  		return -ENOMEM;
>>
>> +	pdev->dev.parent = parent;
>> +
>>  	if (ops->dev_set_proximity) {
>>  		ret = ops->dev_set_proximity(&pdev->dev, node);
>>  		if (ret)
>> @@ -1573,6 +1575,11 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
>>  static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
>>  #endif
>>
>> +static int iort_fwnode_match(struct device *dev, const void *fwnode)
>> +{
>> +	return dev->fwnode == fwnode;
>> +}
>> +
>>  static void __init iort_init_platform_devices(void)
>>  {
>>  	struct acpi_iort_node *iort_node, *iort_end;
>> @@ -1594,11 +1601,34 @@ static void __init iort_init_platform_devices(void)
>>  				iort_table->length);
>>
>>  	for (i = 0; i < iort->node_count; i++) {
>> +		struct device *parent = NULL;
>> +
>>  		if (iort_node >= iort_end) {
>>  			pr_err("iort node pointer overflows, bad table\n");
>>  			return;
>>  		}
>>
>> +		/* Fixme: handle parent declared in IORT after PMCG */
>> +		if (iort_node->type == ACPI_IORT_NODE_PMCG) {
>> +			struct acpi_iort_node *iort_assoc_node;
>> +			struct acpi_iort_pmcg *pmcg;
>> +			u32 node_reference;
>> +
>> +			pmcg = (struct acpi_iort_pmcg *)iort_node->node_data;
>> +
>> +			node_reference = pmcg->node_reference;
>> +			iort_assoc_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
>> +				 node_reference);
>> +
>> +			if (iort_assoc_node->type == ACPI_IORT_NODE_SMMU_V3) {
>> +				struct fwnode_handle *assoc_fwnode;
>> +
>> +				assoc_fwnode = iort_get_fwnode(iort_assoc_node);
>> +
>> +				parent = bus_find_device(&platform_bus_type, NULL,
>> +				      assoc_fwnode, iort_fwnode_match);
>> +			}
>> +		}
>
> How about using a function to include those new added code to make this
> function (iort_init_platform_devices()) a bit cleaner?
>

Can do. But I still need to add code to deal with the scenario of the 
parent PMCG not being an SMMU, which is supported in the spec as I recall.

Note that I would not have FW to test that, so maybe can omit support 
for now as long as there's no regression.

>>  		iort_enable_acs(iort_node);
>>
>>  		ops = iort_get_dev_cfg(iort_node);
>> @@ -1609,7 +1639,7 @@ static void __init iort_init_platform_devices(void)
>>
>>  			iort_set_fwnode(iort_node, fwnode);
>>
>> -			ret = iort_add_platform_device(iort_node, ops);
>> +			ret = iort_add_platform_device(iort_node, ops, parent);
>
> This function is called if ops is valid, so retrieve the parent
> can be done before this function I think.

Ah, yes

Thanks,
John

>

> Thanks
> Hanjun
>
>
> .
>


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH 2/6] iommu/arm-smmu-v3: Record IIDR in arm_smmu_device structure

[John Garry]

To allow other devices know the SMMU HW IIDR, record the IIDR contents as
the first member of the arm_smmu_device structure.

In storing as the first member, it saves exposing SMMU APIs, which are
nicely self-contained today.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 40f4757096c3..1ed3ef0f1ec3 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -70,6 +70,8 @@
 #define IDR1_SSIDSIZE			GENMASK(10, 6)
 #define IDR1_SIDSIZE			GENMASK(5, 0)
 
+#define ARM_SMMU_IIDR                  0x18
+
 #define ARM_SMMU_IDR5			0x14
 #define IDR5_STALL_MAX			GENMASK(31, 16)
 #define IDR5_GRAN64K			(1 << 6)
@@ -546,6 +548,7 @@ struct arm_smmu_strtab_cfg {
 
 /* An SMMUv3 instance */
 struct arm_smmu_device {
+	u32						iidr; /* must be first member */
 	struct device			*dev;
 	void __iomem			*base;
 
@@ -3153,6 +3156,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	iommu_device_set_ops(&smmu->iommu, &arm_smmu_ops);
 	iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
 
+	smmu->iidr = readl(smmu->base + ARM_SMMU_IIDR);
+
 	ret = iommu_device_register(&smmu->iommu);
 	if (ret) {
 		dev_err(dev, "Failed to register iommu\n");
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH 3/6] perf/smmuv3: Retrieve parent SMMUv3 IIDR

[John Garry]

To support IMP DEF events per PMCG, retrieve the parent SMMUv3 IIDR. This
will be used as a lookup for the IMP DEF events supported, under the
assumption that a PMCG implementation has the same uniqueness as the
parent SMMUv3. In this, we assume that any PMCG associated with the same
SMMUv3 will have the same IMP DEF events - otherwise, some other
secondary matching would need to be done.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index da71c741cb46..f702898c695d 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -115,6 +115,7 @@ struct smmu_pmu {
 	bool global_filter;
 	u32 global_filter_span;
 	u32 global_filter_sid;
+	u32 parent_iidr;
 };
 
 #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
@@ -551,6 +552,11 @@ static const struct attribute_group *smmu_pmu_attr_grps[] = {
 	NULL
 };
 
+static const struct attribute_group **smmu_pmu_lookup_attr_groups(u32 parent_smmu_iidr)
+{
+	return smmu_pmu_attr_grps;
+}
+
 /*
  * Generic device handlers
  */
@@ -706,11 +712,21 @@ static int smmu_pmu_probe(struct platform_device *pdev)
 	int irq, err;
 	char *name;
 	struct device *dev = &pdev->dev;
+	struct device *parent = dev->parent;
 
 	smmu_pmu = devm_kzalloc(dev, sizeof(*smmu_pmu), GFP_KERNEL);
 	if (!smmu_pmu)
 		return -ENOMEM;
 
+	if (parent) {
+		void *parent_drvdata;
+
+		parent_drvdata = platform_get_drvdata(to_platform_device(parent));
+		if (!parent_drvdata)
+			return -EPROBE_DEFER;
+		smmu_pmu->parent_iidr = *(u32 *)parent_drvdata;
+	}
+
 	smmu_pmu->dev = dev;
 	platform_set_drvdata(pdev, smmu_pmu);
 
@@ -724,7 +740,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
 		.start		= smmu_pmu_event_start,
 		.stop		= smmu_pmu_event_stop,
 		.read		= smmu_pmu_event_read,
-		.attr_groups	= smmu_pmu_attr_grps,
+		.attr_groups	= smmu_pmu_lookup_attr_groups(smmu_pmu->parent_iidr),
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	};
 
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH 4/6] perf/smmuv3: Support HiSilicon hip08 (hi1620) IMP DEF events

[John Garry]

Now that we can identify an PMCG implementation through the parent SMMUv3
implementation, add support for IMP DEF events.

For each new implementation supported, we add a new attr_grps structure
for a particular implementation and do lookup matching based on the
parent SMMUv3 IIDR. This could maybe be optimised in future to reduce
structures required.

For now, only the l1_tlb event is added for HiSilicon hip08 platform.

This platform supports many more IMP DEF events, but I need something
better than the electronically translated description of the event to
support.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 54 ++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index f702898c695d..11f28ba5fae0 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -132,6 +132,8 @@ SMMU_PMU_EVENT_ATTR_EXTRACTOR(filter_stream_id, config1, 0, 31);
 SMMU_PMU_EVENT_ATTR_EXTRACTOR(filter_span, config1, 32, 32);
 SMMU_PMU_EVENT_ATTR_EXTRACTOR(filter_enable, config1, 33, 33);
 
+#define PARENT_SMMU_IIDR_HISI_HIP08 (0x30736)
+
 static inline void smmu_pmu_enable(struct pmu *pmu)
 {
 	struct smmu_pmu *smmu_pmu = to_smmu_pmu(pmu);
@@ -505,6 +507,21 @@ static struct attribute *smmu_pmu_events[] = {
 	NULL
 };
 
+SMMU_EVENT_ATTR(l1_tlb, 0x8a);
+
+static struct attribute *smmu_pmu_events_hip08[] = {
+	&smmu_event_attr_cycles.attr.attr,
+	&smmu_event_attr_transaction.attr.attr,
+	&smmu_event_attr_tlb_miss.attr.attr,
+	&smmu_event_attr_config_cache_miss.attr.attr,
+	&smmu_event_attr_trans_table_walk_access.attr.attr,
+	&smmu_event_attr_config_struct_access.attr.attr,
+	&smmu_event_attr_pcie_ats_trans_rq.attr.attr,
+	&smmu_event_attr_pcie_ats_trans_passed.attr.attr,
+	&smmu_event_attr_l1_tlb.attr.attr,
+	NULL
+};
+
 static umode_t smmu_pmu_event_is_visible(struct kobject *kobj,
 					 struct attribute *attr, int unused)
 {
@@ -514,7 +531,10 @@ static umode_t smmu_pmu_event_is_visible(struct kobject *kobj,
 
 	pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr.attr);
 
-	if (test_bit(pmu_attr->id, smmu_pmu->supported_events))
+	if (pmu_attr->id < SMMU_PMCG_ARCH_MAX_EVENTS &&
+	    test_bit(pmu_attr->id, smmu_pmu->supported_events))
+		return attr->mode;
+	else if (pmu_attr->id >= SMMU_PMCG_ARCH_MAX_EVENTS)
 		return attr->mode;
 
 	return 0;
@@ -526,6 +546,12 @@ static struct attribute_group smmu_pmu_events_group = {
 	.is_visible = smmu_pmu_event_is_visible,
 };
 
+static struct attribute_group smmu_pmu_events_group_hip08 = {
+	.name = "events",
+	.attrs = smmu_pmu_events_hip08,
+	.is_visible = smmu_pmu_event_is_visible,
+};
+
 /* Formats */
 PMU_FORMAT_ATTR(event,		   "config:0-15");
 PMU_FORMAT_ATTR(filter_stream_id,  "config1:0-31");
@@ -552,8 +578,34 @@ static const struct attribute_group *smmu_pmu_attr_grps[] = {
 	NULL
 };
 
+static const struct attribute_group *smmu_pmu_attr_grps_hip08[] = {
+	&smmu_pmu_cpumask_group,
+	&smmu_pmu_events_group_hip08,
+	&smmu_pmu_format_group,
+	NULL
+};
+
+struct smmu_pmu_attr_grps_custom {
+	u32 parent_smmu_iidr;
+	const struct attribute_group **attr_grps;
+} smmu_pmu_attr_custom_grps[] = {
+	{
+		.parent_smmu_iidr = PARENT_SMMU_IIDR_HISI_HIP08,
+		.attr_grps = smmu_pmu_attr_grps_hip08,
+	},
+};
+
 static const struct attribute_group **smmu_pmu_lookup_attr_groups(u32 parent_smmu_iidr)
 {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(smmu_pmu_attr_custom_grps); i++) {
+		struct smmu_pmu_attr_grps_custom *c = &smmu_pmu_attr_custom_grps[i];
+
+		if (c->parent_smmu_iidr == parent_smmu_iidr)
+			return c->attr_grps;
+	}
+
 	return smmu_pmu_attr_grps;
 }
 
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH 5/6] perf/smmuv3: Match implementation options based on parent SMMU IIDR

[John Garry]

Currently we match the implementation options based on the ACPI PLATFORM
OEM ID.

Since we can now match based on the parent SMMUv3 IIDR, switch to this
method.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 11f28ba5fae0..33d1379ae525 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -739,14 +739,10 @@ static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
 		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
 }
 
-static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
+static void smmu_pmu_get_implementation_options(struct smmu_pmu *smmu_pmu)
 {
-	u32 model;
-
-	model = *(u32 *)dev_get_platdata(smmu_pmu->dev);
-
-	switch (model) {
-	case IORT_SMMU_V3_PMCG_HISI_HIP08:
+	switch (smmu_pmu->parent_iidr) {
+	case PARENT_SMMU_IIDR_HISI_HIP08:
 		/* HiSilicon Erratum 162001800 */
 		smmu_pmu->options |= SMMU_PMCG_EVCNTR_RDONLY;
 		break;
@@ -844,7 +840,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	smmu_pmu_get_acpi_options(smmu_pmu);
+	smmu_pmu_get_implementation_options(smmu_pmu);
 
 	/* Pick one CPU to be the preferred one to use */
 	smmu_pmu->on_cpu = raw_smp_processor_id();
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH 6/6] ACPI/IORT: Drop code to set the PMCG software-defined model

[John Garry]

Now that we can identify a PMCG implementation from the parent SMMUv3
IIDR, drop all the code to match based on the ACPI OEM ID.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/acpi/arm64/iort.c | 35 +----------------------------------
 include/linux/acpi_iort.h |  8 --------
 2 files changed, 1 insertion(+), 42 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 0b687520c3e7..d04888cb8cff 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1377,27 +1377,6 @@ static void __init arm_smmu_v3_pmcg_init_resources(struct resource *res,
 				       ACPI_EDGE_SENSITIVE, &res[2]);
 }
 
-static struct acpi_platform_list pmcg_plat_info[] __initdata = {
-	/* HiSilicon Hip08 Platform */
-	{"HISI  ", "HIP08   ", 0, ACPI_SIG_IORT, greater_than_or_equal,
-	 "Erratum #162001800", IORT_SMMU_V3_PMCG_HISI_HIP08},
-	{ }
-};
-
-static int __init arm_smmu_v3_pmcg_add_platdata(struct platform_device *pdev)
-{
-	u32 model;
-	int idx;
-
-	idx = acpi_match_platform_list(pmcg_plat_info);
-	if (idx >= 0)
-		model = pmcg_plat_info[idx].data;
-	else
-		model = IORT_SMMU_V3_PMCG_GENERIC;
-
-	return platform_device_add_data(pdev, &model, sizeof(model));
-}
-
 struct iort_dev_config {
 	const char *name;
 	int (*dev_init)(struct acpi_iort_node *node);
@@ -1408,7 +1387,6 @@ struct iort_dev_config {
 				     struct acpi_iort_node *node);
 	int (*dev_set_proximity)(struct device *dev,
 				    struct acpi_iort_node *node);
-	int (*dev_add_platdata)(struct platform_device *pdev);
 };
 
 static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
@@ -1430,7 +1408,6 @@ static const struct iort_dev_config iort_arm_smmu_v3_pmcg_cfg __initconst = {
 	.name = "arm-smmu-v3-pmcg",
 	.dev_count_resources = arm_smmu_v3_pmcg_count_resources,
 	.dev_init_resources = arm_smmu_v3_pmcg_init_resources,
-	.dev_add_platdata = arm_smmu_v3_pmcg_add_platdata,
 };
 
 static __init const struct iort_dev_config *iort_get_dev_cfg(
@@ -1494,17 +1471,7 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
 	if (ret)
 		goto dev_put;
 
-	/*
-	 * Platform devices based on PMCG nodes uses platform_data to
-	 * pass the hardware model info to the driver. For others, add
-	 * a copy of IORT node pointer to platform_data to be used to
-	 * retrieve IORT data information.
-	 */
-	if (ops->dev_add_platdata)
-		ret = ops->dev_add_platdata(pdev);
-	else
-		ret = platform_device_add_data(pdev, &node, sizeof(node));
-
+	ret = platform_device_add_data(pdev, &node, sizeof(node));
 	if (ret)
 		goto dev_put;
 
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 8e7e2ec37f1b..7a8961e6a8bb 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -14,14 +14,6 @@
 #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)
 #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)
 
-/*
- * PMCG model identifiers for use in smmu pmu driver. Please note
- * that this is purely for the use of software and has nothing to
- * do with hardware or with IORT specification.
- */
-#define IORT_SMMU_V3_PMCG_GENERIC        0x00000000 /* Generic SMMUv3 PMCG */
-#define IORT_SMMU_V3_PMCG_HISI_HIP08     0x00000001 /* HiSilicon HIP08 PMCG */
-
 int iort_register_domain_token(int trans_id, phys_addr_t base,
 			       struct fwnode_handle *fw_node);
 void iort_deregister_domain_token(int trans_id);
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 6/6] ACPI/IORT: Drop code to set the PMCG software-defined model

[Hanjun Guo]

On 2019/9/30 22:33, John Garry wrote:
> Now that we can identify a PMCG implementation from the parent SMMUv3
> IIDR, drop all the code to match based on the ACPI OEM ID.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/acpi/arm64/iort.c | 35 +----------------------------------
>  include/linux/acpi_iort.h |  8 --------
>  2 files changed, 1 insertion(+), 42 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 0b687520c3e7..d04888cb8cff 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1377,27 +1377,6 @@ static void __init arm_smmu_v3_pmcg_init_resources(struct resource *res,
>  				       ACPI_EDGE_SENSITIVE, &res[2]);
>  }
>  
> -static struct acpi_platform_list pmcg_plat_info[] __initdata = {
> -	/* HiSilicon Hip08 Platform */
> -	{"HISI  ", "HIP08   ", 0, ACPI_SIG_IORT, greater_than_or_equal,
> -	 "Erratum #162001800", IORT_SMMU_V3_PMCG_HISI_HIP08},
> -	{ }
> -};
> -
> -static int __init arm_smmu_v3_pmcg_add_platdata(struct platform_device *pdev)
> -{
> -	u32 model;
> -	int idx;
> -
> -	idx = acpi_match_platform_list(pmcg_plat_info);
> -	if (idx >= 0)
> -		model = pmcg_plat_info[idx].data;
> -	else
> -		model = IORT_SMMU_V3_PMCG_GENERIC;
> -
> -	return platform_device_add_data(pdev, &model, sizeof(model));
> -}
> -
>  struct iort_dev_config {
>  	const char *name;
>  	int (*dev_init)(struct acpi_iort_node *node);
> @@ -1408,7 +1387,6 @@ struct iort_dev_config {
>  				     struct acpi_iort_node *node);
>  	int (*dev_set_proximity)(struct device *dev,
>  				    struct acpi_iort_node *node);
> -	int (*dev_add_platdata)(struct platform_device *pdev);
>  };
>  
>  static const struct iort_dev_config iort_arm_smmu_v3_cfg __initconst = {
> @@ -1430,7 +1408,6 @@ static const struct iort_dev_config iort_arm_smmu_v3_pmcg_cfg __initconst = {
>  	.name = "arm-smmu-v3-pmcg",
>  	.dev_count_resources = arm_smmu_v3_pmcg_count_resources,
>  	.dev_init_resources = arm_smmu_v3_pmcg_init_resources,
> -	.dev_add_platdata = arm_smmu_v3_pmcg_add_platdata,
>  };
>  
>  static __init const struct iort_dev_config *iort_get_dev_cfg(
> @@ -1494,17 +1471,7 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>  	if (ret)
>  		goto dev_put;
>  
> -	/*
> -	 * Platform devices based on PMCG nodes uses platform_data to
> -	 * pass the hardware model info to the driver. For others, add
> -	 * a copy of IORT node pointer to platform_data to be used to
> -	 * retrieve IORT data information.
> -	 */
> -	if (ops->dev_add_platdata)
> -		ret = ops->dev_add_platdata(pdev);
> -	else
> -		ret = platform_device_add_data(pdev, &node, sizeof(node));
> -
> +	ret = platform_device_add_data(pdev, &node, sizeof(node));
>  	if (ret)
>  		goto dev_put;
>  
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 8e7e2ec37f1b..7a8961e6a8bb 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -14,14 +14,6 @@
>  #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)
>  #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)
>  
> -/*
> - * PMCG model identifiers for use in smmu pmu driver. Please note
> - * that this is purely for the use of software and has nothing to
> - * do with hardware or with IORT specification.
> - */
> -#define IORT_SMMU_V3_PMCG_GENERIC        0x00000000 /* Generic SMMUv3 PMCG */
> -#define IORT_SMMU_V3_PMCG_HISI_HIP08     0x00000001 /* HiSilicon HIP08 PMCG */

Since only HiSilicon platform has such erratum, and I think it works with
both old version of firmware, I'm fine with removing this erratum framework.

Acked-by: Hanjun Guo <guohanjun@huawei.com>

Thanks
Hanjun

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 6/6] ACPI/IORT: Drop code to set the PMCG software-defined model

[John Garry]

On 15/10/2019 04:06, Hanjun Guo wrote:
>> -/*
>> > - * PMCG model identifiers for use in smmu pmu driver. Please note
>> > - * that this is purely for the use of software and has nothing to
>> > - * do with hardware or with IORT specification.
>> > - */
>> > -#define IORT_SMMU_V3_PMCG_GENERIC        0x00000000 /* Generic SMMUv3 PMCG */
>> > -#define IORT_SMMU_V3_PMCG_HISI_HIP08     0x00000001 /* HiSilicon HIP08 PMCG */
> Since only HiSilicon platform has such erratum, and I think it works with
> both old version of firmware, I'm fine with removing this erratum framework.
>

Yeah, seems a decent change on its own, even without the SMMU PMCG 
driver changes.

But we still need to check on patch "[PATCH RFC 2/6] iommu/arm-smmu-v3: 
Record IIDR in arm_smmu_device structure" to progress any of this.

Will, Robin, Any opinion on that patch?

https://lore.kernel.org/linux-iommu/1569854031-237636-1-git-send-email-john.garry@huawei.com/T/#m1e24771a23ee5426ec94ca2c4ec572642c155a77

> Acked-by: Hanjun Guo <guohanjun@huawei.com>

Cheers,
John

>
> Thanks
> Hanjun
>
>
> .
>


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/6] SMMUv3 PMCG IMP DEF event support

[Robin Murphy]

Hi John,

On 30/09/2019 15:33, John Garry wrote:
> This patchset adds IMP DEF event support for the SMMUv3 PMCG.
> 
> It is marked as an RFC as the method to identify the PMCG implementation
> may be a quite disliked. And, in general, the series is somewhat
> incomplete.
> 
> So the background is that the PMCG supports IMP DEF events, yet we have no
> method to identify the PMCG to know the IMP DEF events.
> 
> A method for identifying the PMCG implementation could be using
> PMDEVARCH, but we cannot rely on this being set properly, as whether this
> is implemented is not defined in SMMUv3 spec.
> 
> Another method would be perf event aliasing, but this method of event
> matching is based on CPU id, which would not guarantee same
> uniqueness as PMCG implementation.
> 
> Yet another method could be to continue using ACPI OEM ID in the IORT
> code, but this does not scale. And it is not suitable if we ever add DT
> support to the PMCG driver.
> 
> The method used in this series is based on matching on the parent SMMUv3
> IIDR. We store this IIDR contents in the arm smmu structure as the first
> element, which means that we don't have to expose SMMU APIs - this is
> the part which may be disliked.
> 
> The final two patches switch the pre-existing PMCG model identification
> from ACPI OEM ID to the same parent SMMUv3 IIDR matching.
> 
> For now, we only consider SMMUv3' nodes being the associated node for
> PMCG.

Two significant concerns right off the bat:

- It seems more common than not for silicon designers to fail to 
implement IIDR correctly, so it's only a matter of time before 
inevitably needing to bring back some firmware-level identifier 
abstraction (if not already - does Hi161x have PMCGs?)

- This seems like a step in entirely the wrong direction for supporting 
PMCGs that reference a Named Component or Root Complex.

Interpreting the Node Reference is definitely a welcome improvement over 
matching table headers, but absent a truly compelling argument to the 
contrary, I'd rather retain the "PMCG model" abstraction in between that 
and the driver itself (especially since those can trivially be hung off 
compatibles once it comes to DT support).

Thanks,
Robin.

> 
> John Garry (6):
>    ACPI/IORT: Set PMCG device parent
>    iommu/arm-smmu-v3: Record IIDR in arm_smmu_device structure
>    perf/smmuv3: Retrieve parent SMMUv3 IIDR
>    perf/smmuv3: Support HiSilicon hip08 (hi1620) IMP DEF events
>    perf/smmuv3: Match implementation options based on parent SMMU IIDR
>    ACPI/IORT: Drop code to set the PMCG software-defined model
> 
>   drivers/acpi/arm64/iort.c     | 69 ++++++++++++++--------------
>   drivers/iommu/arm-smmu-v3.c   |  5 +++
>   drivers/perf/arm_smmuv3_pmu.c | 84 ++++++++++++++++++++++++++++++-----
>   include/linux/acpi_iort.h     |  8 ----
>   4 files changed, 112 insertions(+), 54 deletions(-)
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/6] SMMUv3 PMCG IMP DEF event support

[John Garry]

On 15/10/2019 19:00, Robin Murphy wrote:
> Hi John,
>
> On 30/09/2019 15:33, John Garry wrote:
>> This patchset adds IMP DEF event support for the SMMUv3 PMCG.
>>
>> It is marked as an RFC as the method to identify the PMCG implementation
>> may be a quite disliked. And, in general, the series is somewhat
>> incomplete.
>>
>> So the background is that the PMCG supports IMP DEF events, yet we
>> have no
>> method to identify the PMCG to know the IMP DEF events.
>>
>> A method for identifying the PMCG implementation could be using
>> PMDEVARCH, but we cannot rely on this being set properly, as whether this
>> is implemented is not defined in SMMUv3 spec.
>>
>> Another method would be perf event aliasing, but this method of event
>> matching is based on CPU id, which would not guarantee same
>> uniqueness as PMCG implementation.
>>
>> Yet another method could be to continue using ACPI OEM ID in the IORT
>> code, but this does not scale. And it is not suitable if we ever add DT
>> support to the PMCG driver.
>>
>> The method used in this series is based on matching on the parent SMMUv3
>> IIDR. We store this IIDR contents in the arm smmu structure as the first
>> element, which means that we don't have to expose SMMU APIs - this is
>> the part which may be disliked.
>>
>> The final two patches switch the pre-existing PMCG model identification
>> from ACPI OEM ID to the same parent SMMUv3 IIDR matching.
>>
>> For now, we only consider SMMUv3' nodes being the associated node for
>> PMCG.
>

Hi Robin,

> Two significant concerns right off the bat:
>
> - It seems more common than not for silicon designers to fail to
> implement IIDR correctly, so it's only a matter of time before
> inevitably needing to bring back some firmware-level identifier
> abstraction (if not already - does Hi161x have PMCGs?)

Maybe there's a way that we can switch to this method, and leave the 
door open for an easy way to support firmware-level identifier again, if 
ever needed. I'm not too pushed - this was secondary to just allowing 
the PMCG driver know the associated SMMU model.

And, no, hi161x does not have any PMCGs.

>
> - This seems like a step in entirely the wrong direction for supporting
>.

So to support PMCGs that reference a Named Component or Root Complex, I 
thought that the IORT parsing code would have to do some secondary 
lookup to the associated SMMU, through the Named Component or Root 
Complex node.

What was your idea here?

Note: I do acknowledge that an overall issue is that we assume all PMCG 
IMP DEF events are same for a given SMMU model.

>
> Interpreting the Node Reference is definitely a welcome improvement over
> matching table headers, but absent a truly compelling argument to the
> contrary, I'd rather retain the "PMCG model" abstraction in between that
> and the driver itself (especially since those can trivially be hung off
> compatibles once it comes to DT support).

For DT, I would assume that we just use compatible strings would allow 
us to identify the PMCG model.

On a related matter, is there still a need to deal with scenarios of the 
PMCG being located within the SMMU register map? As you may remember, we 
did have this issue but relocated the PMCG to outside the SMMU register 
map in a later chip rev.

Cheers,
John

>
> Thanks,
> Robin.
>
>>
>> John Garry (6):
>>    ACPI/IORT: Set PMCG device parent
>>    iommu/arm-smmu-v3: Record IIDR in arm_smmu_device structure
>>    perf/smmuv3: Retrieve parent SMMUv3 IIDR
>>    perf/smmuv3: Support HiSilicon hip08 (hi1620) IMP DEF events
>>    perf/smmuv3: Match implementation options based on parent SMMU IIDR
>>    ACPI/IORT: Drop code to set the PMCG software-defined model
>>
>>   drivers/acpi/arm64/iort.c     | 69 ++++++++++++++--------------
>>   drivers/iommu/arm-smmu-v3.c   |  5 +++
>>   drivers/perf/arm_smmuv3_pmu.c | 84 ++++++++++++++++++++++++++++++-----
>>   include/linux/acpi_iort.h     |  8 ----
>>   4 files changed, 112 insertions(+), 54 deletions(-)
>>
>
> .
>


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/6] SMMUv3 PMCG IMP DEF event support

[Robin Murphy]

On 2019-10-16 9:47 am, John Garry wrote:
> On 15/10/2019 19:00, Robin Murphy wrote:
>> Hi John,
>>
>> On 30/09/2019 15:33, John Garry wrote:
>>> This patchset adds IMP DEF event support for the SMMUv3 PMCG.
>>>
>>> It is marked as an RFC as the method to identify the PMCG implementation
>>> may be a quite disliked. And, in general, the series is somewhat
>>> incomplete.
>>>
>>> So the background is that the PMCG supports IMP DEF events, yet we
>>> have no
>>> method to identify the PMCG to know the IMP DEF events.
>>>
>>> A method for identifying the PMCG implementation could be using
>>> PMDEVARCH, but we cannot rely on this being set properly, as whether 
>>> this
>>> is implemented is not defined in SMMUv3 spec.
>>>
>>> Another method would be perf event aliasing, but this method of event
>>> matching is based on CPU id, which would not guarantee same
>>> uniqueness as PMCG implementation.
>>>
>>> Yet another method could be to continue using ACPI OEM ID in the IORT
>>> code, but this does not scale. And it is not suitable if we ever add DT
>>> support to the PMCG driver.
>>>
>>> The method used in this series is based on matching on the parent SMMUv3
>>> IIDR. We store this IIDR contents in the arm smmu structure as the first
>>> element, which means that we don't have to expose SMMU APIs - this is
>>> the part which may be disliked.
>>>
>>> The final two patches switch the pre-existing PMCG model identification
>>> from ACPI OEM ID to the same parent SMMUv3 IIDR matching.
>>>
>>> For now, we only consider SMMUv3' nodes being the associated node for
>>> PMCG.
>>
> 
> Hi Robin,
> 
>> Two significant concerns right off the bat:
>>
>> - It seems more common than not for silicon designers to fail to
>> implement IIDR correctly, so it's only a matter of time before
>> inevitably needing to bring back some firmware-level identifier
>> abstraction (if not already - does Hi161x have PMCGs?)
> 
> Maybe there's a way that we can switch to this method, and leave the 
> door open for an easy way to support firmware-level identifier again, if 
> ever needed. I'm not too pushed - this was secondary to just allowing 
> the PMCG driver know the associated SMMU model.

But that's the part I'm not buying - there's no clear advantage to 
pushing that complexity down into the PMCG driver, vs. leaving the IORT 
code responsible for translating an SMMU model into a PMCG model, yet 
the aforementioned disadvantages jump out right away.

> And, no, hi161x does not have any PMCGs.

Hooray, I guess :)

>>
>> - This seems like a step in entirely the wrong direction for supporting
>> .
> 
> So to support PMCGs that reference a Named Component or Root Complex, I 
> thought that the IORT parsing code would have to do some secondary 
> lookup to the associated SMMU, through the Named Component or Root 
> Complex node.
> 
> What was your idea here?

The associated SMMU has no relevance in that context - the reason for 
the Node Reference to point to a non-SMMU node is for devices that 
implement their own embedded TLB (e.g. AMBA DTI masters) and expose a 
standard PMCG interface to monitor it. It isn't reasonable to expect any 
old PCIe controller or on-chip-accelerator driver to expose a fake SMMU 
IIDR just to keep some other driver happy.

> Note: I do acknowledge that an overall issue is that we assume all PMCG 
> IMP DEF events are same for a given SMMU model.

That assumption does technically fail already - I know MMU-600 has 
different IMP-DEF events for its TCU and TBUs, however as long as we can 
get as far as "this is some part of an MMU-600" the driver should be 
able to figure out the rest (annoyingly it looks like both PMCG types 
expose the same PMCG_ID_REGS information, but they should be 
distinguishable by PMCG_CEIDn).

>> Interpreting the Node Reference is definitely a welcome improvement over
>> matching table headers, but absent a truly compelling argument to the
>> contrary, I'd rather retain the "PMCG model" abstraction in between that
>> and the driver itself (especially since those can trivially be hung off
>> compatibles once it comes to DT support).
> 
> For DT, I would assume that we just use compatible strings would allow 
> us to identify the PMCG model.

Right, that was largely my point - DT probing can start with a PMCG 
model, so it's a lot more logical for ACPI probing to do the same, with 
the actual PMCG model determination hidden away in the ACPI code. That's 
the basis of the current design.

I have been nagging the architects that PMCGs not having their own IIDR 
is an unwelcome hole in the spec, so hopefully this might get a bit 
easier some day.

> On a related matter, is there still a need to deal with scenarios of the 
> PMCG being located within the SMMU register map? As you may remember, we 
> did have this issue but relocated the PMCG to outside the SMMU register 
> map in a later chip rev.

MMU-600 has its TCU PMCG page 0 in the middle of its SMMU page 0 space, 
but given that it's an Arm IP, I expect that when the heat gets turned 
up for making it work, it's most likely to be under me ;)

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/6] SMMUv3 PMCG IMP DEF event support

[John Garry]

Hi Robin,

>>> Two significant concerns right off the bat:
>>>
>>> - It seems more common than not for silicon designers to fail to
>>> implement IIDR correctly, so it's only a matter of time before
>>> inevitably needing to bring back some firmware-level identifier
>>> abstraction (if not already - does Hi161x have PMCGs?)
>>
>> Maybe there's a way that we can switch to this method, and leave the
>> door open for an easy way to support firmware-level identifier again,
>> if ever needed. I'm not too pushed - this was secondary to just
>> allowing the PMCG driver know the associated SMMU model.
>
> But that's the part I'm not buying - there's no clear advantage to
> pushing that complexity down into the PMCG driver, vs. leaving the IORT
> code responsible for translating an SMMU model into a PMCG model, yet
> the aforementioned disadvantages jump out right away.
>

One advantage is that the next piece of quirky hw with a properly 
implemented IIDR does not require a new IORT model.

And today, this handling is only for hi1620, and since we can use hi1620 
IIDR to id it, then it seems good to remove code outside the PMCG driver 
specifically to handle it.

But if you think it's going to be needed again, then it makes sense not 
to remove it.

>> And, no, hi161x does not have any PMCGs.
>
> Hooray, I guess :)
>
>>>
>>> - This seems like a step in entirely the wrong direction for supporting
>>> .
>>
>> So to support PMCGs that reference a Named Component or Root Complex,
>> I thought that the IORT parsing code would have to do some secondary
>> lookup to the associated SMMU, through the Named Component or Root
>> Complex node.
>>
>> What was your idea here?
>
> The associated SMMU has no relevance in that context - the reason for
> the Node Reference to point to a non-SMMU node is for devices that
> implement their own embedded TLB (e.g. AMBA DTI masters) and expose a
> standard PMCG interface to monitor it. It isn't reasonable to expect any
> old PCIe controller or on-chip-accelerator driver to expose a fake SMMU
> IIDR just to keep some other driver happy.

But won't there still be an SMMU associated with the AMBA DTI masters, 
in your example?

It's this SMMU which the PMCG driver would reference as the "parent" 
device, and the IORT parsing would need to do the lookup for this reference.

But then, this becomes something that the DT parsing would need to 
handle also.

>
>> Note: I do acknowledge that an overall issue is that we assume all
>> PMCG IMP DEF events are same for a given SMMU model.
>
> That assumption does technically fail already - I know MMU-600 has
> different IMP-DEF events for its TCU and TBUs, however as long as we can
> get as far as "this is some part of an MMU-600" the driver should be
> able to figure out the rest (annoyingly it looks like both PMCG types
> expose the same PMCG_ID_REGS information, but they should be
> distinguishable by PMCG_CEIDn).

JFYI, PMCG_CEIDn contents for hi1620 are all zero, apart from PMDEVARCH 
and PMDEVTYPE, which are same as arm implementation according to the 
spec - sigh...

>
>>> Interpreting the Node Reference is definitely a welcome improvement over
>>> matching table headers, but absent a truly compelling argument to the
>>> contrary, I'd rather retain the "PMCG model" abstraction in between that
>>> and the driver itself (especially since those can trivially be hung off
>>> compatibles once it comes to DT support).
>>
>> For DT, I would assume that we just use compatible strings would allow
>> us to identify the PMCG model.
>
> Right, that was largely my point - DT probing can start with a PMCG
> model, so it's a lot more logical for ACPI probing to do the same, with
> the actual PMCG model determination hidden away in the ACPI code. That's
> the basis of the current design.
>
> I have been nagging the architects that PMCGs not having their own IIDR
> is an unwelcome hole in the spec, so hopefully this might get a bit
> easier some day.

For sure. The spec reads that the PMCGs may be "independently-designed", 
hence no general id method. I don't get this.

>
>> On a related matter, is there still a need to deal with scenarios of
>> the PMCG being located within the SMMU register map? As you may
>> remember, we did have this issue but relocated the PMCG to outside the
>> SMMU register map in a later chip rev.
>
> MMU-600 has its TCU PMCG page 0 in the middle of its SMMU page 0 space,
> but given that it's an Arm IP, I expect that when the heat gets turned
> up for making it work, it's most likely to be under me ;)

OK, so this is another reason why I thought that having a reference to 
the SMMU device could be useful in terms of solving that problem.

>
> Robin.
>
> .

Thanks again,
John

>


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu