On 2021/1/29 23:06, Robin Murphy wrote:
> On 2021-01-27 11:32, Zhen Lei wrote:
>> According to the SMMUv3 specification:
>> Each PMCG counter group is represented by one 4KB page (Page 0) with one
>> optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION
>> DEFINED base addresses.
>>
>> This means that the PMCG register spaces may be within the 64KB pages of
>> the SMMUv3 register space. When both the SMMU and PMCG drivers reserve
>> their own resources, a resource conflict occurs.
>>
>> To avoid this conflict, don't reserve the PMCG regions.
>
> I'm still not a fan of this get_and_ioremap notion in general, especially
> when the "helper" function ends up over twice the size of all the code it
> replaces[1], but for the actual functional change here,
OK，I'll change it to [1] and add some comments.
>
> Reviewed-by: Robin Murphy
>
>> Suggested-by: Robin Murphy
>> Signed-off-by: Zhen Lei
>> ---
>> drivers/perf/arm_smmuv3_pmu.c | 27 +--
>> 1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>> index 74474bb322c3f26..e5e505a0804fe53 100644
>> --- a/drivers/perf/arm_smmuv3_pmu.c
>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>> @@ -761,6 +761,29 @@ 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);
>> }
>> +static void __iomem *
>> +smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev,
>> + unsigned int index,
>> + struct resource **res)
>> +{
>> + void __iomem *base;
>> + struct resource *r;
>> +
>> + r = platform_get_resource(pdev, IORESOURCE_MEM, index);
>> + if (!r) {
>> + dev_err(&pdev->dev, "invalid resource\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> + if (res)
>> + *res = r;
>> +
>> + base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
>> + if (!base)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + return base;
>> +}
>> +
>> static int smmu_pmu_probe(struct platform_device *pdev)
>> {
>> struct smmu_pmu *smmu_pmu;
>> @@ -793,7 +816,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>> .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>> };
>> - smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0,
>> &res_0);
>> + smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0);
>> if (IS_ERR(smmu_pmu->reg_base))
>> return PTR_ERR(smmu_pmu->reg_base);
>> @@ -801,7 +824,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>> /* Determine if page 1 is present */
>> if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
>> - smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1);
>> + smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1,
>> NULL);
>> if (IS_ERR(smmu_pmu->reloc_base))
>> return PTR_ERR(smmu_pmu->reloc_base);
>> } else {
>>
>
> [1]
> ->8-
> drivers/perf/arm_smmuv3_pmu.c | 35 +--
> 1 file changed, 9 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index e5e505a0804f..c9adbc7b55a1 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -761,33 +761,10 @@ 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);
> }
>
> -static void __iomem *
> -smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev,
> - unsigned int index,
> - struct resource **res)
> -{
> - void __iomem *base;
> - struct resource *r;
> -
> - r = platform_get_resource(pdev, IORESOURCE_MEM, index);
> - if (!r) {
> - dev_err(&pdev->dev, "invalid resource\n");
> - return ERR_PTR(-EINVAL);
> - }
> - if (res)
> - *res = r;
> -
> - base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> - if (!base)
> - return ERR_PTR(-ENOMEM);
> -
> - return base;
> -}
> -
> static int smmu_pmu_probe(struct platform_device *pdev)
> {
> struct smmu_pmu *smmu_pmu;
> - struct resource *res_0;
> + struct resource *res_0, *res_1;
> u32 cfgr, reg_size;
> u64 ceid_64[2];
> int irq, err;
> @@ -816,7 +793,10 @@ static int smmu_pmu_probe(struct platform_device *pdev)
> .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> };
>
> - smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0);
> + res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res_0)
> + return ERR_PTR(-EINVAL);
> + smmu_pmu->reg_base = devm_ioremap(dev, res_0->start,
> resource_size(res_0));
> if (IS_ERR(smmu_pmu->reg_base))
> return PTR_ERR(smmu_pmu->reg_base);
>
> @@ -824,7 +804,10 @@ static int sm
