[RFC PATCH v1 0/2] perf/smmuv3: dts get opt and simplify code

[RFC PATCH v1 0/2] perf/smmuv3: dts get opt and simplify code

[Jay Chen]

This patch set firstly to simplify the code in smmu pmu probe,
and then support to get options in dts

Jay Chen (2):
  perf/smmuv3: To simplify code for ioremap page in pmcg
  perf/smmuv3: To support the dts to get options

 drivers/perf/arm_smmuv3_pmu.c | 45 ++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

-- 
2.27.0


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

[RFC PATCH v1 1/2] perf/smmuv3: To simplify code for ioremap page in pmcg

[Jay Chen]

Use the devm_platform_get_and_ioremap_resource to simplify the code
a bit.

Signed-off-by: Jay Chen <jkchen@linux.alibaba.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 48e28ef93a70..2d09f3e47d12 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -755,8 +755,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	};
 
-	res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	smmu_pmu->reg_base = devm_ioremap_resource(dev, res_0);
+	smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res_0);
 	if (IS_ERR(smmu_pmu->reg_base))
 		return PTR_ERR(smmu_pmu->reg_base);
 
-- 
2.27.0


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

[RFC PATCH v1 2/2] perf/smmuv3: To support the dts to get options

[Jay Chen]

For the smmuv3 pmu for support the dts to get the
options

Signed-off-by: Jay Chen <jkchen@linux.alibaba.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 2d09f3e47d12..44e9f4197444 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -115,6 +115,16 @@ struct smmu_pmu {
 	bool global_filter;
 };
 
+struct smmu_pmu_prop {
+	u32 opt;
+	const char *prop;
+};
+
+static struct smmu_pmu_prop smmu_pmu_options[] = {
+	{ SMMU_PMCG_EVCNTR_RDONLY, "hisilicon,smmu-pmcg-evcntr-rdonly"},
+	{ 0, NULL},
+};
+
 #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
 
 #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
@@ -708,6 +718,7 @@ static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
 		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
 }
 
+#ifdef CONFIG_ACPI
 static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
 {
 	u32 model;
@@ -723,6 +734,26 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
 
 	dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options);
 }
+#else
+static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
+{
+
+}
+#endif
+
+static void smmu_pmu_get_dt_options(struct smmu_pmu *smmu_pmu)
+{
+	int i = 0;
+
+	do {
+		if (of_property_read_bool(smmu_pmu->dev->of_node,
+					  smmu_pmu_options[i].prop)) {
+			smmu_pmu->options |= smmu_pmu_options[i].opt;
+			dev_notice(smmu_pmu->dev, "option mask 0x%x\n",
+				   smmu_pmu->options);
+		}
+	} while (smmu_pmu_options[++i].opt);
+}
 
 static int smmu_pmu_probe(struct platform_device *pdev)
 {
@@ -801,7 +832,10 @@ static int smmu_pmu_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	smmu_pmu_get_acpi_options(smmu_pmu);
+	if (dev->of_node)
+		smmu_pmu_get_dt_options(smmu_pmu);
+	else
+		smmu_pmu_get_acpi_options(smmu_pmu);
 
 	/* Pick one CPU to be the preferred one to use */
 	smmu_pmu->on_cpu = raw_smp_processor_id();
@@ -855,9 +889,15 @@ static void smmu_pmu_shutdown(struct platform_device *pdev)
 	smmu_pmu_disable(&smmu_pmu->pmu);
 }
 
+static const struct of_device_id smmu_pmu_of_match[] = {
+	{ .compatible = "arm-smmu-v3-pmcg", },
+	{ },
+};
+
 static struct platform_driver smmu_pmu_driver = {
 	.driver = {
 		.name = "arm-smmu-v3-pmcg",
+		.of_match_table = smmu_pmu_of_match,
 	},
 	.probe = smmu_pmu_probe,
 	.remove = smmu_pmu_remove,
-- 
2.27.0


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

Re: [RFC PATCH v1 2/2] perf/smmuv3: To support the dts to get options

[Robin Murphy]

On 2020-07-06 12:22, Jay Chen wrote:
> For the smmuv3 pmu for support the dts to get the
> options
> 
> Signed-off-by: Jay Chen <jkchen@linux.alibaba.com>
> ---
>   drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index 2d09f3e47d12..44e9f4197444 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -115,6 +115,16 @@ struct smmu_pmu {
>   	bool global_filter;
>   };
>   
> +struct smmu_pmu_prop {
> +	u32 opt;
> +	const char *prop;
> +};
> +
> +static struct smmu_pmu_prop smmu_pmu_options[] = {
> +	{ SMMU_PMCG_EVCNTR_RDONLY, "hisilicon,smmu-pmcg-evcntr-rdonly"},

We know this kind of thing doesn't scale well - please define a 
compatible string for the specific PMU model.

In fact my hope was that DT compatibles would map to the same model 
values we made up for IORT to pass, such that the *only* 
firmware-specific difference the driver needs to have is whether to 
retrieve the model from platdata or OF match data, then the actual 
quirks are applied commonly.

> +	{ 0, NULL},
> +};
> +
>   #define to_smmu_pmu(p) (container_of(p, struct smmu_pmu, pmu))
>   
>   #define SMMU_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
> @@ -708,6 +718,7 @@ static void smmu_pmu_reset(struct smmu_pmu *smmu_pmu)
>   		       smmu_pmu->reloc_base + SMMU_PMCG_OVSCLR0);
>   }
>   
> +#ifdef CONFIG_ACPI

Why bother stubbing this out? It shouldn't have any build-time 
dependency on ACPI code, and saving a whole 48 bytes from the object 
size (for my local build setup) struggles to justify the source-code 
clutter.

>   static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
>   {
>   	u32 model;
> @@ -723,6 +734,26 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
>   
>   	dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options);
>   }
> +#else
> +static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
> +{
> +
> +}
> +#endif
> +
> +static void smmu_pmu_get_dt_options(struct smmu_pmu *smmu_pmu)
> +{
> +	int i = 0;
> +
> +	do {
> +		if (of_property_read_bool(smmu_pmu->dev->of_node,
> +					  smmu_pmu_options[i].prop)) {
> +			smmu_pmu->options |= smmu_pmu_options[i].opt;
> +			dev_notice(smmu_pmu->dev, "option mask 0x%x\n",
> +				   smmu_pmu->options);
> +		}
> +	} while (smmu_pmu_options[++i].opt);
> +}
>   
>   static int smmu_pmu_probe(struct platform_device *pdev)
>   {
> @@ -801,7 +832,10 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>   		return -EINVAL;
>   	}
>   
> -	smmu_pmu_get_acpi_options(smmu_pmu);
> +	if (dev->of_node)
> +		smmu_pmu_get_dt_options(smmu_pmu);
> +	else
> +		smmu_pmu_get_acpi_options(smmu_pmu);
>   
>   	/* Pick one CPU to be the preferred one to use */
>   	smmu_pmu->on_cpu = raw_smp_processor_id();
> @@ -855,9 +889,15 @@ static void smmu_pmu_shutdown(struct platform_device *pdev)
>   	smmu_pmu_disable(&smmu_pmu->pmu);
>   }
>   
> +static const struct of_device_id smmu_pmu_of_match[] = {
> +	{ .compatible = "arm-smmu-v3-pmcg", },

Please define the DT binding first. IIRC Jean-Philippe wrote some 
patches a while back that never got posted, but I suppose it should be 
YAML now...

> +	{ },
> +};

How about the thing for module autoloading too?

Robin.

> +
>   static struct platform_driver smmu_pmu_driver = {
>   	.driver = {
>   		.name = "arm-smmu-v3-pmcg",
> +		.of_match_table = smmu_pmu_of_match,
>   	},
>   	.probe = smmu_pmu_probe,
>   	.remove = smmu_pmu_remove,
> 

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

Re: [RFC PATCH v1 2/2] perf/smmuv3: To support the dts to get options

[Jean-Philippe Brucker]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

Hi,

On Mon, Jul 06, 2020 at 04:03:34PM +0100, Robin Murphy wrote:
> On 2020-07-06 12:22, Jay Chen wrote:
> > For the smmuv3 pmu for support the dts to get the
> > options
> > 
> > Signed-off-by: Jay Chen <jkchen@linux.alibaba.com>
[...]
> > +static const struct of_device_id smmu_pmu_of_match[] = {
> > +	{ .compatible = "arm-smmu-v3-pmcg", },
> 
> Please define the DT binding first. IIRC Jean-Philippe wrote some patches a
> while back that never got posted, but I suppose it should be YAML now...

Yes, I've never followed through with that because it only supported the
RevC FastModel with non-default model parameters. I attached the binding I
currently have, converted to YAML.

Thanks,
Jean


[-- Attachment #2: 0001-dt-bindings-Add-SMMUv3-PMCG-binding.patch --]
[-- Type: text/plain, Size: 2840 bytes --]

From b117e5b4ce96a5a8327333ab408cf61200850d4f Mon Sep 17 00:00:00 2001
From: Jean-Philippe Brucker <jean-philippe@linaro.org>
Date: Tue, 7 Jul 2020 16:55:16 +0200
Subject: [PATCH] dt-bindings: Add SMMUv3 PMCG binding

Add binding for the SMMUv3 PMU. Each node represents a PMCG, and is placed
as a sibling node of the SMMU. As PMCGs are mainly implementation
defined there is no 1-1 relation between SMMU and PMCG. The SMMU could
have PMU counters for the TCU and each TBU, or a single PMCG.

TODO: although the Linux implementation doesn't need them, it'd be nice
to have links from the PMCG node to its associated SMMU. IORT does offer
this (Node reference) and perhaps it could later help users figure out
which PMCG is which on systems with dozens of SMMU.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 .../bindings/iommu/arm,smmu-v3-pmcg.yaml      | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
new file mode 100644
index 000000000000..23190a617e7e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/arm,smmu-v3-pmcg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM SMMUv3 Performance Monitor Counter Group
+
+maintainers:
+  - Will Deacon <will@kernel.org>
+  - Robin Murphy <Robin.Murphy@arm.com>
+
+description: |+
+  An SMMUv3 may have several Performance Monitor Counter Group (PMCG).
+  They are standalone performance monitoring units that support both
+  architected and IMPLEMENTATION DEFINED event counters.
+
+properties:
+  $nodename:
+    pattern: "^smmu-pmcg@[0-9a-f]*"
+  compatible:
+    const: arm,smmu-v3-pmcg
+
+  reg:
+    minItems: 1
+    maxItems: 2
+
+  interrupts:
+    maxItems: 1
+
+  msi-parent: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |+
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    tcu: smmu-pmcg@2b420000 {
+            compatible = "arm,smmu-v3-pmcg";
+            reg = <0 0x2b420000 0 0x1000>,
+                  <0 0x2b430000 0 0x1000>;
+            interrupts = <GIC_SPI 80 IRQ_TYPE_EDGE_RISING>;
+            msi-parent = <&its 0xff0000>;
+    };
+
+    tbu0: smmu-pmcg@2b440000 {
+            compatible = "arm,smmu-v3-pmcg";
+            reg = <0 0x2b440000 0 0x1000>,
+                  <0 0x2b450000 0 0x1000>;
+            interrupts = <GIC_SPI 81 IRQ_TYPE_EDGE_RISING>;
+            msi-parent = <&its 0xff0000>;
+    };
-- 
2.27.0


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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

Re: [RFC PATCH v1 2/2] perf/smmuv3: To support the dts to get options

[Rob Herring]

On Tue, Jul 07, 2020 at 05:01:14PM +0200, Jean-Philippe Brucker wrote:
> Hi,
> 
> On Mon, Jul 06, 2020 at 04:03:34PM +0100, Robin Murphy wrote:
> > On 2020-07-06 12:22, Jay Chen wrote:
> > > For the smmuv3 pmu for support the dts to get the
> > > options
> > > 
> > > Signed-off-by: Jay Chen <jkchen@linux.alibaba.com>
> [...]
> > > +static const struct of_device_id smmu_pmu_of_match[] = {
> > > +	{ .compatible = "arm-smmu-v3-pmcg", },
> > 
> > Please define the DT binding first. IIRC Jean-Philippe wrote some patches a
> > while back that never got posted, but I suppose it should be YAML now...
> 
> Yes, I've never followed through with that because it only supported the
> RevC FastModel with non-default model parameters. I attached the binding I
> currently have, converted to YAML.
> 
> Thanks,
> Jean
> 

> >From b117e5b4ce96a5a8327333ab408cf61200850d4f Mon Sep 17 00:00:00 2001
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Date: Tue, 7 Jul 2020 16:55:16 +0200
> Subject: [PATCH] dt-bindings: Add SMMUv3 PMCG binding
> 
> Add binding for the SMMUv3 PMU. Each node represents a PMCG, and is placed
> as a sibling node of the SMMU. As PMCGs are mainly implementation
> defined there is no 1-1 relation between SMMU and PMCG. The SMMU could
> have PMU counters for the TCU and each TBU, or a single PMCG.
> 
> TODO: although the Linux implementation doesn't need them, it'd be nice
> to have links from the PMCG node to its associated SMMU. IORT does offer
> this (Node reference) and perhaps it could later help users figure out
> which PMCG is which on systems with dozens of SMMU.

Is the PMCG really a separate block or a new node is just convenient to 
instantiate a driver?

> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  .../bindings/iommu/arm,smmu-v3-pmcg.yaml      | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
> new file mode 100644
> index 000000000000..23190a617e7e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Dual license new bindings.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/arm,smmu-v3-pmcg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMMUv3 Performance Monitor Counter Group
> +
> +maintainers:
> +  - Will Deacon <will@kernel.org>
> +  - Robin Murphy <Robin.Murphy@arm.com>
> +
> +description: |+
> +  An SMMUv3 may have several Performance Monitor Counter Group (PMCG).
> +  They are standalone performance monitoring units that support both
> +  architected and IMPLEMENTATION DEFINED event counters.
> +
> +properties:
> +  $nodename:
> +    pattern: "^smmu-pmcg@[0-9a-f]*"

Should be generic:

pmu@...

(or whatever we've used for PMUs).

> +  compatible:
> +    const: arm,smmu-v3-pmcg

This is correct, but doesn't match the driver.

> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2

More than 1 entry needs a description of what each one is.

A variable number of 'reg' entries generally implies more than 1 
compatible unless the 2nd entry is optional.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  msi-parent: true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    tcu: smmu-pmcg@2b420000 {

Drop unused labels.

> +            compatible = "arm,smmu-v3-pmcg";
> +            reg = <0 0x2b420000 0 0x1000>,
> +                  <0 0x2b430000 0 0x1000>;
> +            interrupts = <GIC_SPI 80 IRQ_TYPE_EDGE_RISING>;
> +            msi-parent = <&its 0xff0000>;
> +    };
> +
> +    tbu0: smmu-pmcg@2b440000 {
> +            compatible = "arm,smmu-v3-pmcg";
> +            reg = <0 0x2b440000 0 0x1000>,
> +                  <0 0x2b450000 0 0x1000>;
> +            interrupts = <GIC_SPI 81 IRQ_TYPE_EDGE_RISING>;
> +            msi-parent = <&its 0xff0000>;
> +    };
> -- 
> 2.27.0
> 

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


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

Re: [RFC PATCH v1 2/2] perf/smmuv3: To support the dts to get options

[Robin Murphy]

On 2020-07-14 00:25, Rob Herring wrote:
> On Tue, Jul 07, 2020 at 05:01:14PM +0200, Jean-Philippe Brucker wrote:
>> Hi,
>>
>> On Mon, Jul 06, 2020 at 04:03:34PM +0100, Robin Murphy wrote:
>>> On 2020-07-06 12:22, Jay Chen wrote:
>>>> For the smmuv3 pmu for support the dts to get the
>>>> options
>>>>
>>>> Signed-off-by: Jay Chen <jkchen@linux.alibaba.com>
>> [...]
>>>> +static const struct of_device_id smmu_pmu_of_match[] = {
>>>> +	{ .compatible = "arm-smmu-v3-pmcg", },
>>>
>>> Please define the DT binding first. IIRC Jean-Philippe wrote some patches a
>>> while back that never got posted, but I suppose it should be YAML now...
>>
>> Yes, I've never followed through with that because it only supported the
>> RevC FastModel with non-default model parameters. I attached the binding I
>> currently have, converted to YAML.
>>
>> Thanks,
>> Jean
>>
> 
>> >From b117e5b4ce96a5a8327333ab408cf61200850d4f Mon Sep 17 00:00:00 2001
>> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> Date: Tue, 7 Jul 2020 16:55:16 +0200
>> Subject: [PATCH] dt-bindings: Add SMMUv3 PMCG binding
>>
>> Add binding for the SMMUv3 PMU. Each node represents a PMCG, and is placed
>> as a sibling node of the SMMU. As PMCGs are mainly implementation
>> defined there is no 1-1 relation between SMMU and PMCG. The SMMU could
>> have PMU counters for the TCU and each TBU, or a single PMCG.
>>
>> TODO: although the Linux implementation doesn't need them, it'd be nice
>> to have links from the PMCG node to its associated SMMU. IORT does offer
>> this (Node reference) and perhaps it could later help users figure out
>> which PMCG is which on systems with dozens of SMMU.
> 
> Is the PMCG really a separate block or a new node is just convenient to
> instantiate a driver?

Yes, PMCGs are their own thing with their own little programming 
interfaces and interrupts, there are typically multiple PMCG instances 
per SMMU, and they may even belong to "non-SMMU" components which 
participate in translation, like PCIe root complexes or devices with 
their own embedded TLBs.

>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> ---
>>   .../bindings/iommu/arm,smmu-v3-pmcg.yaml      | 58 +++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
>> new file mode 100644
>> index 000000000000..23190a617e7e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3-pmcg.yaml
>> @@ -0,0 +1,58 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
> 
> Dual license new bindings.
> 
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iommu/arm,smmu-v3-pmcg.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: ARM SMMUv3 Performance Monitor Counter Group
>> +
>> +maintainers:
>> +  - Will Deacon <will@kernel.org>
>> +  - Robin Murphy <Robin.Murphy@arm.com>
>> +
>> +description: |+
>> +  An SMMUv3 may have several Performance Monitor Counter Group (PMCG).
>> +  They are standalone performance monitoring units that support both
>> +  architected and IMPLEMENTATION DEFINED event counters.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^smmu-pmcg@[0-9a-f]*"
> 
> Should be generic:
> 
> pmu@...
> 
> (or whatever we've used for PMUs).
> 
>> +  compatible:
>> +    const: arm,smmu-v3-pmcg
> 
> This is correct, but doesn't match the driver.

To be fair, this binding wasn't originally written for this particular 
driver patch ;)

>> +
>> +  reg:
>> +    minItems: 1
>> +    maxItems: 2
> 
> More than 1 entry needs a description of what each one is.
> 
> A variable number of 'reg' entries generally implies more than 1
> compatible unless the 2nd entry is optional.

The second is "optional" in terms of the architecture (and thus the 
generic compatible), but fixed for any specific implementation - it's a 
choice of whether the counter registers are in the same page as the 
control registers or in a separate page, but that can be architecturally 
discovered from an ID register in the first page (see the handling of 
SMMU_PMCG_CFGR_RELOC_CTRS if you're interested).
Robin.

>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  msi-parent: true
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |+
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    tcu: smmu-pmcg@2b420000 {
> 
> Drop unused labels.
> 
>> +            compatible = "arm,smmu-v3-pmcg";
>> +            reg = <0 0x2b420000 0 0x1000>,
>> +                  <0 0x2b430000 0 0x1000>;
>> +            interrupts = <GIC_SPI 80 IRQ_TYPE_EDGE_RISING>;
>> +            msi-parent = <&its 0xff0000>;
>> +    };
>> +
>> +    tbu0: smmu-pmcg@2b440000 {
>> +            compatible = "arm,smmu-v3-pmcg";
>> +            reg = <0 0x2b440000 0 0x1000>,
>> +                  <0 0x2b450000 0 0x1000>;
>> +            interrupts = <GIC_SPI 81 IRQ_TYPE_EDGE_RISING>;
>> +            msi-parent = <&its 0xff0000>;
>> +    };
>> -- 
>> 2.27.0
>>
> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

Re: [RFC PATCH v1 0/2] perf/smmuv3: dts get opt and simplify code

[Will Deacon]

On Mon, 6 Jul 2020 19:22:44 +0800, Jay Chen wrote:
> This patch set firstly to simplify the code in smmu pmu probe,
> and then support to get options in dts
> 
> Jay Chen (2):
>   perf/smmuv3: To simplify code for ioremap page in pmcg
>   perf/smmuv3: To support the dts to get options
> 
> [...]

Applied to will (for-next/perf), thanks!

[1/1] perf/smmuv3: To simplify code for ioremap page in pmcg
      https://git.kernel.org/will/c/f011856ce7b6

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

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

Re: [RFC PATCH v1 0/2] perf/smmuv3: dts get opt and simplify code

[Rob Herring]

On Mon, Jul 13, 2020 at 09:46:58PM +0100, Will Deacon wrote:
> On Mon, 6 Jul 2020 19:22:44 +0800, Jay Chen wrote:
> > This patch set firstly to simplify the code in smmu pmu probe,
> > and then support to get options in dts
> > 
> > Jay Chen (2):
> >   perf/smmuv3: To simplify code for ioremap page in pmcg
> >   perf/smmuv3: To support the dts to get options
> > 
> > [...]
> 
> Applied to will (for-next/perf), thanks!

Without a binding document for the undocumented property and 
compatible string!?

Is the 'arm-smmu-v3-pmcg' actually a separate block from the SMMU? Or is 
this just an add a compatible to instantiate a driver?

Rob

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

Re: [RFC PATCH v1 0/2] perf/smmuv3: dts get opt and simplify code

[Will Deacon]

Hi Rob,

On Mon, Jul 13, 2020 at 05:15:55PM -0600, Rob Herring wrote:
> On Mon, Jul 13, 2020 at 09:46:58PM +0100, Will Deacon wrote:
> > On Mon, 6 Jul 2020 19:22:44 +0800, Jay Chen wrote:
> > > This patch set firstly to simplify the code in smmu pmu probe,
> > > and then support to get options in dts
> > > 
> > > Jay Chen (2):
> > >   perf/smmuv3: To simplify code for ioremap page in pmcg
> > >   perf/smmuv3: To support the dts to get options
> > > 
> > > [...]
> > 
> > Applied to will (for-next/perf), thanks!
> 
> Without a binding document for the undocumented property and 
> compatible string!?

No, I only took the second patch ("To simplify code...")! It would be nice
if b4 was a big clearer about that when cherry-picking, but I'm not sure
exactly what it could do. Maybe if it only replied to the patch being
picked, rather than the cover letter?

I'm usually pretty careful about avoiding DT changes without your ack.

> Is the 'arm-smmu-v3-pmcg' actually a separate block from the SMMU? Or is 
> this just an add a compatible to instantiate a driver?

Not quite sure how to answer that one... afaik, the PMCG is a bunch of
distributed counter blocks that may or may not be present and are driven
entirely independently of the rest of the SMMU, which is a lot more
standardised. In fact, I'm not sure why ARM_SMMU_V3_PMU depends on
ARM_SMMU_V3, it builds perfectly fine without that (and with the main
driver disabled).

Will

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

Re: [RFC PATCH v1 0/2] perf/smmuv3: dts get opt and simplify code

[Will Deacon]

On Tue, Jul 14, 2020 at 08:48:01AM +0100, Will Deacon wrote:
> On Mon, Jul 13, 2020 at 05:15:55PM -0600, Rob Herring wrote:
> > On Mon, Jul 13, 2020 at 09:46:58PM +0100, Will Deacon wrote:
> > > On Mon, 6 Jul 2020 19:22:44 +0800, Jay Chen wrote:
> > > > This patch set firstly to simplify the code in smmu pmu probe,
> > > > and then support to get options in dts
> > > > 
> > > > Jay Chen (2):
> > > >   perf/smmuv3: To simplify code for ioremap page in pmcg
> > > >   perf/smmuv3: To support the dts to get options
> > > > 
> > > > [...]
> > > 
> > > Applied to will (for-next/perf), thanks!
> > 
> > Without a binding document for the undocumented property and 
> > compatible string!?
> 
> No, I only took the second patch ("To simplify code...")!

Duh, *first patch*.

Will

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

Re: [RFC PATCH v1 0/2] perf/smmuv3: dts get opt and simplify code

[Rob Herring]

On Tue, Jul 14, 2020 at 1:48 AM Will Deacon <will@kernel.org> wrote:
>
> Hi Rob,
>
> On Mon, Jul 13, 2020 at 05:15:55PM -0600, Rob Herring wrote:
> > On Mon, Jul 13, 2020 at 09:46:58PM +0100, Will Deacon wrote:
> > > On Mon, 6 Jul 2020 19:22:44 +0800, Jay Chen wrote:
> > > > This patch set firstly to simplify the code in smmu pmu probe,
> > > > and then support to get options in dts
> > > >
> > > > Jay Chen (2):
> > > >   perf/smmuv3: To simplify code for ioremap page in pmcg
> > > >   perf/smmuv3: To support the dts to get options
> > > >
> > > > [...]
> > >
> > > Applied to will (for-next/perf), thanks!
> >
> > Without a binding document for the undocumented property and
> > compatible string!?
>
> No, I only took the second patch ("To simplify code...")! It would be nice
> if b4 was a big clearer about that when cherry-picking, but I'm not sure
> exactly what it could do. Maybe if it only replied to the patch being
> picked, rather than the cover letter?

Humm, I thought the default b4 reply did list exactly which patches
were applied. See broonie's replies for example which are using it I
think. Maybe you lose that if not using the default template.

>
> I'm usually pretty careful about avoiding DT changes without your ack.
>
> > Is the 'arm-smmu-v3-pmcg' actually a separate block from the SMMU? Or is
> > this just an add a compatible to instantiate a driver?
>
> Not quite sure how to answer that one... afaik, the PMCG is a bunch of
> distributed counter blocks that may or may not be present and are driven
> entirely independently of the rest of the SMMU, which is a lot more
> standardised. In fact, I'm not sure why ARM_SMMU_V3_PMU depends on
> ARM_SMMU_V3, it builds perfectly fine without that (and with the main
> driver disabled).

Okay, seems that a separate node or nodes is warranted.

Rob

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