[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 at 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,
>