Re: [RESEND PATCH] driver/perf: arm-cmn: support ACPI probe

[Jing Zhang <renyu.zj@linux.alibaba.com>]

在 2023/3/24 上午6:33, Ilkka Koskinen 写道:
> 
> Hi Jing,
> 
> On Thu, 23 Mar 2023, Jing Zhang wrote:
>> ACPI companion devices call insert_resource() in platform_device_add()
>> to claim the device resources. If the resources are claimed again before
>> ioremap(), and the addresses of multiple resources overlap, it will
>> return -BUSY, causing the driver to fail to load.
>>
>> For example, the CMN700 on my machine is set with two resources similar
>> to CMN600, and the overlap of resource addresses makes the CMN driver
>> unable to match my CMN700. The error log:
> 
> I thought that the two address ranges were only needed for CMN-600?
> I guess, the specification doesn't explicitly forbid giving two ranges for CMN700 although the example shows just one.
> 

Hi Ilkka,

Yes, on our platform, CMN700 is designed with two resources which describe PERIPHBASE and ROOTNODEBASE respectively.

>>
>> [  12.016837] arm-cmn ARMHC700:00: can't request region for resource [mem 0x40000000-0x4fffffff]
>> [  12.028230] arm-cmn: probe of ARMHC700:00 failed with error -16
>> [  12.036832] arm-cmn ARMHC700:01: can't request region for resource [mem 0x40040000000-0x4004fffffff]
>> [  12.051289] arm-cmn: probe of ARMHC700:01 failed with error -16
>>
>> So let ACPI companion devices call arm_cmn_acpi_probe() and not claim
>> resource again. In addition, the arm_cmn_acpi_probe() and
>> arm_cmn_of_probe() functions are refactored to make them compatible
>> with both CMN600 and CMN-ANY.
>>
>> Fixes: 61ec1d875812 ("perf/arm-cmn: Demarcate CMN-600 specifics")
>> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
>> ---
>> drivers/perf/arm-cmn.c | 57 ++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 37 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>> index 1deb61b..beb3b37 100644
>> --- a/drivers/perf/arm-cmn.c
>> +++ b/drivers/perf/arm-cmn.c
>> @@ -2206,7 +2206,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
>>     return 0;
>> }
>>
>> -static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>> +static int arm_cmn_acpi_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>> {
>>     struct resource *cfg, *root;
>>
>> @@ -2214,12 +2214,21 @@ static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *c
>>     if (!cfg)
>>         return -EINVAL;
>>
>> -    root = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> -    if (!root)
>> -        return -EINVAL;
>> +    /* If ACPI defines more than one resource, such as cmn-600, then there may be
>> +     * a deviation between ROOTNODEBASE and PERIPHBASE, and ROOTNODEBASE can
>> +     * be obtained from the second resource. Otherwise, it can be considered that
>> +     * ROOT NODE BASE is PERIPHBASE. This is compatible with cmn-600 and cmn-any.
>> +     */
>> +    if (pdev->num_resources > 1) {
>> +        root = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +        if (!root)
>> +            return -EINVAL;
>>
>> -    if (!resource_contains(cfg, root))
>> -        swap(cfg, root);
>> +        if (!resource_contains(cfg, root))
>> +            swap(cfg, root);
>> +    } else {
>> +        root = cfg;
>> +    }
>>     /*
>>      * Note that devm_ioremap_resource() is dumb and won't let the platform
>>      * device claim cfg when the ACPI companion device has already claimed
>> @@ -2227,17 +2236,30 @@ static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *c
>>      * appropriate name, we don't really need to do it again here anyway.
>>      */
>>     cmn->base = devm_ioremap(cmn->dev, cfg->start, resource_size(cfg));
>> -    if (!cmn->base)
>> -        return -ENOMEM;
>> +    if (IS_ERR(cmn->base))
>> +        return PTR_ERR(cmn->base);
> 
> I believe, devm_ioremap() returns NULL in case of error
> 

Yes, you are right.


Thanks,
Jing