[PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Jing Zhang]

The devm_platform_ioremap_resource() won't let the platform device
claim resource when the ACPI companion device has already claimed it.
If CMN-ANY except CMN600 is ACPI companion device, it will return
-EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
successfully installed.

So let ACPI companion device 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);
 
 	return root->start - cfg->start;
 }
 
-static int arm_cmn600_of_probe(struct device_node *np)
+static int arm_cmn_of_probe(struct platform_device *pdev, struct arm_cmn *cmn)
 {
 	u32 rootnode;
+	int ret;
+
+	cmn->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(cmn->base))
+		return PTR_ERR(cmn->base);
 
-	return of_property_read_u32(np, "arm,root-node", &rootnode) ?: rootnode;
+	/* If of_property_read_u32() return EINVAL, it means that device tree has
+	 * not define root-node, and root-node will return 0, which is compatible
+	 * with cmn-600 and cmn-any.
+	 */
+	ret = of_property_read_u32(pdev->dev.of_node, "arm,root-node", &rootnode);
+	if (ret == -EINVAL)
+		return 0;
+
+	return rootnode;
 }
 
 static int arm_cmn_probe(struct platform_device *pdev)
@@ -2255,16 +2277,11 @@ static int arm_cmn_probe(struct platform_device *pdev)
 	cmn->model = (unsigned long)device_get_match_data(cmn->dev);
 	platform_set_drvdata(pdev, cmn);
 
-	if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
-		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
-	} else {
-		rootnode = 0;
-		cmn->base = devm_platform_ioremap_resource(pdev, 0);
-		if (IS_ERR(cmn->base))
-			return PTR_ERR(cmn->base);
-		if (cmn->model == CMN600)
-			rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
-	}
+	if (has_acpi_companion(cmn->dev))
+		rootnode = arm_cmn_acpi_probe(pdev, cmn);
+	else
+		rootnode = arm_cmn_of_probe(pdev, cmn);
+
 	if (rootnode < 0)
 		return rootnode;
 
-- 
1.8.3.1

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Jing Zhang]

Hi all,

Do you have any comments on this?:)

Thanks,
Jing

在 2023/2/16 下午4:17, Jing Zhang 写道:
> The devm_platform_ioremap_resource() won't let the platform device
> claim resource when the ACPI companion device has already claimed it.
> If CMN-ANY except CMN600 is ACPI companion device, it will return
> -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
> successfully installed.
> 
> So let ACPI companion device 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);
>  
>  	return root->start - cfg->start;
>  }
>  
> -static int arm_cmn600_of_probe(struct device_node *np)
> +static int arm_cmn_of_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>  {
>  	u32 rootnode;
> +	int ret;
> +
> +	cmn->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(cmn->base))
> +		return PTR_ERR(cmn->base);
>  
> -	return of_property_read_u32(np, "arm,root-node", &rootnode) ?: rootnode;
> +	/* If of_property_read_u32() return EINVAL, it means that device tree has
> +	 * not define root-node, and root-node will return 0, which is compatible
> +	 * with cmn-600 and cmn-any.
> +	 */
> +	ret = of_property_read_u32(pdev->dev.of_node, "arm,root-node", &rootnode);
> +	if (ret == -EINVAL)
> +		return 0;
> +
> +	return rootnode;
>  }
>  
>  static int arm_cmn_probe(struct platform_device *pdev)
> @@ -2255,16 +2277,11 @@ static int arm_cmn_probe(struct platform_device *pdev)
>  	cmn->model = (unsigned long)device_get_match_data(cmn->dev);
>  	platform_set_drvdata(pdev, cmn);
>  
> -	if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
> -		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
> -	} else {
> -		rootnode = 0;
> -		cmn->base = devm_platform_ioremap_resource(pdev, 0);
> -		if (IS_ERR(cmn->base))
> -			return PTR_ERR(cmn->base);
> -		if (cmn->model == CMN600)
> -			rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
> -	}
> +	if (has_acpi_companion(cmn->dev))
> +		rootnode = arm_cmn_acpi_probe(pdev, cmn);
> +	else
> +		rootnode = arm_cmn_of_probe(pdev, cmn);
> +
>  	if (rootnode < 0)
>  		return rootnode;
>  

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Jing Zhang]

在 2023/2/16 下午4:17, Jing Zhang 写道:
> The devm_platform_ioremap_resource() won't let the platform device
> claim resource when the ACPI companion device has already claimed it.
> If CMN-ANY except CMN600 is ACPI companion device, it will return
> -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
> successfully installed.
> 
> So let ACPI companion device 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);
>  
>  	return root->start - cfg->start;
>  }
>  
> -static int arm_cmn600_of_probe(struct device_node *np)
> +static int arm_cmn_of_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>  {
>  	u32 rootnode;
> +	int ret;
> +
> +	cmn->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(cmn->base))
> +		return PTR_ERR(cmn->base);
>  
> -	return of_property_read_u32(np, "arm,root-node", &rootnode) ?: rootnode;
> +	/* If of_property_read_u32() return EINVAL, it means that device tree has
> +	 * not define root-node, and root-node will return 0, which is compatible
> +	 * with cmn-600 and cmn-any.
> +	 */
> +	ret = of_property_read_u32(pdev->dev.of_node, "arm,root-node", &rootnode);
> +	if (ret == -EINVAL)
> +		return 0;
> +
> +	return rootnode;
>  }
>  
>  static int arm_cmn_probe(struct platform_device *pdev)
> @@ -2255,16 +2277,11 @@ static int arm_cmn_probe(struct platform_device *pdev)
>  	cmn->model = (unsigned long)device_get_match_data(cmn->dev);
>  	platform_set_drvdata(pdev, cmn);
>  
> -	if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
> -		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
> -	} else {
> -		rootnode = 0;
> -		cmn->base = devm_platform_ioremap_resource(pdev, 0);
> -		if (IS_ERR(cmn->base))
> -			return PTR_ERR(cmn->base);
> -		if (cmn->model == CMN600)
> -			rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
> -	}
> +	if (has_acpi_companion(cmn->dev))
> +		rootnode = arm_cmn_acpi_probe(pdev, cmn);
> +	else
> +		rootnode = arm_cmn_of_probe(pdev, cmn);
> +
>  	if (rootnode < 0)
>  		return rootnode;
>  


ping?

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Will Deacon]

[+Robin and Ilkka, as they contribute most to this driver]

On Thu, Feb 16, 2023 at 04:17:50PM +0800, Jing Zhang wrote:
> The devm_platform_ioremap_resource() won't let the platform device
> claim resource when the ACPI companion device has already claimed it.
> If CMN-ANY except CMN600 is ACPI companion device, it will return
> -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
> successfully installed.
> 
> So let ACPI companion device 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);
>  
>  	return root->start - cfg->start;
>  }
>  
> -static int arm_cmn600_of_probe(struct device_node *np)
> +static int arm_cmn_of_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>  {
>  	u32 rootnode;
> +	int ret;
> +
> +	cmn->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(cmn->base))
> +		return PTR_ERR(cmn->base);
>  
> -	return of_property_read_u32(np, "arm,root-node", &rootnode) ?: rootnode;
> +	/* If of_property_read_u32() return EINVAL, it means that device tree has
> +	 * not define root-node, and root-node will return 0, which is compatible
> +	 * with cmn-600 and cmn-any.
> +	 */
> +	ret = of_property_read_u32(pdev->dev.of_node, "arm,root-node", &rootnode);
> +	if (ret == -EINVAL)
> +		return 0;
> +
> +	return rootnode;
>  }
>  
>  static int arm_cmn_probe(struct platform_device *pdev)
> @@ -2255,16 +2277,11 @@ static int arm_cmn_probe(struct platform_device *pdev)
>  	cmn->model = (unsigned long)device_get_match_data(cmn->dev);
>  	platform_set_drvdata(pdev, cmn);
>  
> -	if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
> -		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
> -	} else {
> -		rootnode = 0;
> -		cmn->base = devm_platform_ioremap_resource(pdev, 0);
> -		if (IS_ERR(cmn->base))
> -			return PTR_ERR(cmn->base);
> -		if (cmn->model == CMN600)
> -			rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
> -	}
> +	if (has_acpi_companion(cmn->dev))
> +		rootnode = arm_cmn_acpi_probe(pdev, cmn);
> +	else
> +		rootnode = arm_cmn_of_probe(pdev, cmn);
> +
>  	if (rootnode < 0)
>  		return rootnode;
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Robin Murphy]

On 2023-03-27 15:05, Will Deacon wrote:
> [+Robin and Ilkka, as they contribute most to this driver]
> 
> On Thu, Feb 16, 2023 at 04:17:50PM +0800, Jing Zhang wrote:
>> The devm_platform_ioremap_resource() won't let the platform device
>> claim resource when the ACPI companion device has already claimed it.
>> If CMN-ANY except CMN600 is ACPI companion device, it will return
>> -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
>> successfully installed.
>>
>> So let ACPI companion device 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.

No, the whole point of CMN-600 probing being a special case is that the 
ACPI and DT bindings for CMN-600 are special cases. In ACPI, only 
ARMHC600 has the two nested memory resources; all the other models 
should only have one memory resource because one is all that is 
meaningful. See table 16 the document[1] in where the description of 
ROOTNODEBASE says "This field is specific to the CMN-600 device object."

Similarly in DT, "arm,root-node" is only required for "arm,cmn-600" - it 
didn't seem worth overcomplicating the schema to actively disallow it 
for other models, but that is supposed to be implied by its description 
as "not relevant for newer CMN/CI products".

If you're hitting this because you've written your ACPI DSDT 
incorrectly, it's a sign that you should fix your DSDT.

Thanks,
Robin.

[1] https://developer.arm.com/documentation/den0093/latest/

>> 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);
>>   
>>   	return root->start - cfg->start;
>>   }
>>   
>> -static int arm_cmn600_of_probe(struct device_node *np)
>> +static int arm_cmn_of_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>>   {
>>   	u32 rootnode;
>> +	int ret;
>> +
>> +	cmn->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(cmn->base))
>> +		return PTR_ERR(cmn->base);
>>   
>> -	return of_property_read_u32(np, "arm,root-node", &rootnode) ?: rootnode;
>> +	/* If of_property_read_u32() return EINVAL, it means that device tree has
>> +	 * not define root-node, and root-node will return 0, which is compatible
>> +	 * with cmn-600 and cmn-any.
>> +	 */
>> +	ret = of_property_read_u32(pdev->dev.of_node, "arm,root-node", &rootnode);
>> +	if (ret == -EINVAL)
>> +		return 0;
>> +
>> +	return rootnode;
>>   }
>>   
>>   static int arm_cmn_probe(struct platform_device *pdev)
>> @@ -2255,16 +2277,11 @@ static int arm_cmn_probe(struct platform_device *pdev)
>>   	cmn->model = (unsigned long)device_get_match_data(cmn->dev);
>>   	platform_set_drvdata(pdev, cmn);
>>   
>> -	if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
>> -		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
>> -	} else {
>> -		rootnode = 0;
>> -		cmn->base = devm_platform_ioremap_resource(pdev, 0);
>> -		if (IS_ERR(cmn->base))
>> -			return PTR_ERR(cmn->base);
>> -		if (cmn->model == CMN600)
>> -			rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
>> -	}
>> +	if (has_acpi_companion(cmn->dev))
>> +		rootnode = arm_cmn_acpi_probe(pdev, cmn);
>> +	else
>> +		rootnode = arm_cmn_of_probe(pdev, cmn);
>> +
>>   	if (rootnode < 0)
>>   		return rootnode;
>>   
>> -- 
>> 1.8.3.1
>>

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Jing Zhang]

在 2023/3/27 下午10:27, Robin Murphy 写道:
> On 2023-03-27 15:05, Will Deacon wrote:
>> [+Robin and Ilkka, as they contribute most to this driver]
>>
>> On Thu, Feb 16, 2023 at 04:17:50PM +0800, Jing Zhang wrote:
>>> The devm_platform_ioremap_resource() won't let the platform device
>>> claim resource when the ACPI companion device has already claimed it.
>>> If CMN-ANY except CMN600 is ACPI companion device, it will return
>>> -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
>>> successfully installed.
>>>
>>> So let ACPI companion device 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.
> 
> No, the whole point of CMN-600 probing being a special case is that the ACPI and DT bindings for CMN-600 are special cases. In ACPI, only ARMHC600 has the two nested memory resources; all the other models should only have one memory resource because one is all that is meaningful. See table 16 the document[1] in where the description of ROOTNODEBASE says "This field is specific to the CMN-600 device object."
> 
> Similarly in DT, "arm,root-node" is only required for "arm,cmn-600" - it didn't seem worth overcomplicating the schema to actively disallow it for other models, but that is supposed to be implied by its description as "not relevant for newer CMN/CI products".
> 
> If you're hitting this because you've written your ACPI DSDT incorrectly, it's a sign that you should fix your DSDT.
> 
> Thanks,
> Robin.
> 
> [1] https://developer.arm.com/documentation/den0093/latest/
> 

Hi Robin,

Yes, I know what you mean, but if a new CMN PMU device design two resources like CMN600, it will fail to load driver.
the specification doesn't explicitly forbid giving two ranges for CMN700 although the example shows just one.
So in our SoC, CMN700 is designed with two resources similar to CMN600, and the overlap of resource addresses makes
the CMN driver unable to match my CMN700. The error log:

[  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 this patch can compatible devices which has two memory resources and one memory resource, why not do it?

I  resend this patch before and add more information about it. This patch still needs to be perfected,
I will improve it in the next version.
Link: https://lore.kernel.org/all/1679587838-80000-1-git-send-email-renyu.zj@linux.alibaba.com/

Thanks,
Jing


>>> 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);
>>>         return root->start - cfg->start;
>>>   }
>>>   -static int arm_cmn600_of_probe(struct device_node *np)
>>> +static int arm_cmn_of_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>>>   {
>>>       u32 rootnode;
>>> +    int ret;
>>> +
>>> +    cmn->base = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(cmn->base))
>>> +        return PTR_ERR(cmn->base);
>>>   -    return of_property_read_u32(np, "arm,root-node", &rootnode) ?: rootnode;
>>> +    /* If of_property_read_u32() return EINVAL, it means that device tree has
>>> +     * not define root-node, and root-node will return 0, which is compatible
>>> +     * with cmn-600 and cmn-any.
>>> +     */
>>> +    ret = of_property_read_u32(pdev->dev.of_node, "arm,root-node", &rootnode);
>>> +    if (ret == -EINVAL)
>>> +        return 0;
>>> +
>>> +    return rootnode;
>>>   }
>>>     static int arm_cmn_probe(struct platform_device *pdev)
>>> @@ -2255,16 +2277,11 @@ static int arm_cmn_probe(struct platform_device *pdev)
>>>       cmn->model = (unsigned long)device_get_match_data(cmn->dev);
>>>       platform_set_drvdata(pdev, cmn);
>>>   -    if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
>>> -        rootnode = arm_cmn600_acpi_probe(pdev, cmn);
>>> -    } else {
>>> -        rootnode = 0;
>>> -        cmn->base = devm_platform_ioremap_resource(pdev, 0);
>>> -        if (IS_ERR(cmn->base))
>>> -            return PTR_ERR(cmn->base);
>>> -        if (cmn->model == CMN600)
>>> -            rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
>>> -    }
>>> +    if (has_acpi_companion(cmn->dev))
>>> +        rootnode = arm_cmn_acpi_probe(pdev, cmn);
>>> +    else
>>> +        rootnode = arm_cmn_of_probe(pdev, cmn);
>>> +
>>>       if (rootnode < 0)
>>>           return rootnode;
>>>   -- 
>>> 1.8.3.1
>>>

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Robin Murphy]

On 2023-03-29 10:48, Jing Zhang wrote:
> 
> 
> 在 2023/3/27 下午10:27, Robin Murphy 写道:
>> On 2023-03-27 15:05, Will Deacon wrote:
>>> [+Robin and Ilkka, as they contribute most to this driver]
>>>
>>> On Thu, Feb 16, 2023 at 04:17:50PM +0800, Jing Zhang wrote:
>>>> The devm_platform_ioremap_resource() won't let the platform device
>>>> claim resource when the ACPI companion device has already claimed it.
>>>> If CMN-ANY except CMN600 is ACPI companion device, it will return
>>>> -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
>>>> successfully installed.
>>>>
>>>> So let ACPI companion device 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.
>>
>> No, the whole point of CMN-600 probing being a special case is that the ACPI and DT bindings for CMN-600 are special cases. In ACPI, only ARMHC600 has the two nested memory resources; all the other models should only have one memory resource because one is all that is meaningful. See table 16 the document[1] in where the description of ROOTNODEBASE says "This field is specific to the CMN-600 device object."
>>
>> Similarly in DT, "arm,root-node" is only required for "arm,cmn-600" - it didn't seem worth overcomplicating the schema to actively disallow it for other models, but that is supposed to be implied by its description as "not relevant for newer CMN/CI products".
>>
>> If you're hitting this because you've written your ACPI DSDT incorrectly, it's a sign that you should fix your DSDT.
>>
>> Thanks,
>> Robin.
>>
>> [1] https://developer.arm.com/documentation/den0093/latest/
>>
> 
> Hi Robin,
> 
> Yes, I know what you mean, but if a new CMN PMU device design two resources like CMN600, it will fail to load driver.

Future CMN models won't do that, though. And if some design way down the 
line does end up wanting multiple memory resources for some other 
reason, the CMN-600 probing code almost certainly won't be the right 
thing anyway. Besides, firmware bindings for new models will be defined 
as necessary when those new models are released, and the driver will be 
updated as necessary at that point to support them, so this is a non-issue.

> the specification doesn't explicitly forbid giving two ranges for CMN700

What do you think "specific to the CMN-600 device object" means, exactly?

> although the example shows just one.
> So in our SoC, CMN700 is designed with two resources similar to CMN600,

Why? Either the second resource is meaningless and the correct answer is 
to remove it from the DSDT, or you've actually modified the hardware 
design to move the CFG node somewhere else, in which case what you have 
is no longer an Arm CMN-700 and thus should not be claiming to be one.

  and the overlap of resource addresses makes
> the CMN driver unable to match my CMN700. The error log:
> 
> [  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 this patch can compatible devices which has two memory resources and one memory resource, why not do it?

The driver supports the standard firmware bindings, because if everyone 
ignored the standard firmware bindings and did their own thing, there 
could not be a standard driver. If we can tell that firmware hasn't 
followed the bindings because it's added random extra resources, how can 
we be sure that *any* of them will actually be the one we're looking for?

Please follow the standard firmware bindings and do not hack 
non-standard behaviours into the driver.

Thanks,
Robin.

> I  resend this patch before and add more information about it. This patch still needs to be perfected,
> I will improve it in the next version.
> Link: https://lore.kernel.org/all/1679587838-80000-1-git-send-email-renyu.zj@linux.alibaba.com/
> 
> Thanks,
> Jing
> 
> 
>>>> 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);
>>>>          return root->start - cfg->start;
>>>>    }
>>>>    -static int arm_cmn600_of_probe(struct device_node *np)
>>>> +static int arm_cmn_of_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>>>>    {
>>>>        u32 rootnode;
>>>> +    int ret;
>>>> +
>>>> +    cmn->base = devm_platform_ioremap_resource(pdev, 0);
>>>> +    if (IS_ERR(cmn->base))
>>>> +        return PTR_ERR(cmn->base);
>>>>    -    return of_property_read_u32(np, "arm,root-node", &rootnode) ?: rootnode;
>>>> +    /* If of_property_read_u32() return EINVAL, it means that device tree has
>>>> +     * not define root-node, and root-node will return 0, which is compatible
>>>> +     * with cmn-600 and cmn-any.
>>>> +     */
>>>> +    ret = of_property_read_u32(pdev->dev.of_node, "arm,root-node", &rootnode);
>>>> +    if (ret == -EINVAL)
>>>> +        return 0;
>>>> +
>>>> +    return rootnode;
>>>>    }
>>>>      static int arm_cmn_probe(struct platform_device *pdev)
>>>> @@ -2255,16 +2277,11 @@ static int arm_cmn_probe(struct platform_device *pdev)
>>>>        cmn->model = (unsigned long)device_get_match_data(cmn->dev);
>>>>        platform_set_drvdata(pdev, cmn);
>>>>    -    if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
>>>> -        rootnode = arm_cmn600_acpi_probe(pdev, cmn);
>>>> -    } else {
>>>> -        rootnode = 0;
>>>> -        cmn->base = devm_platform_ioremap_resource(pdev, 0);
>>>> -        if (IS_ERR(cmn->base))
>>>> -            return PTR_ERR(cmn->base);
>>>> -        if (cmn->model == CMN600)
>>>> -            rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
>>>> -    }
>>>> +    if (has_acpi_companion(cmn->dev))
>>>> +        rootnode = arm_cmn_acpi_probe(pdev, cmn);
>>>> +    else
>>>> +        rootnode = arm_cmn_of_probe(pdev, cmn);
>>>> +
>>>>        if (rootnode < 0)
>>>>            return rootnode;
>>>>    --
>>>> 1.8.3.1
>>>>

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Jing Zhang]

在 2023/3/29 下午6:53, Robin Murphy 写道:
> On 2023-03-29 10:48, Jing Zhang wrote:
>>
>>
>> 在 2023/3/27 下午10:27, Robin Murphy 写道:
>>> On 2023-03-27 15:05, Will Deacon wrote:
>>>> [+Robin and Ilkka, as they contribute most to this driver]
>>>>
>>>> On Thu, Feb 16, 2023 at 04:17:50PM +0800, Jing Zhang wrote:
>>>>> The devm_platform_ioremap_resource() won't let the platform device
>>>>> claim resource when the ACPI companion device has already claimed it.
>>>>> If CMN-ANY except CMN600 is ACPI companion device, it will return
>>>>> -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
>>>>> successfully installed.
>>>>>
>>>>> So let ACPI companion device 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.
>>>
>>> No, the whole point of CMN-600 probing being a special case is that the ACPI and DT bindings for CMN-600 are special cases. In ACPI, only ARMHC600 has the two nested memory resources; all the other models should only have one memory resource because one is all that is meaningful. See table 16 the document[1] in where the description of ROOTNODEBASE says "This field is specific to the CMN-600 device object."
>>>
>>> Similarly in DT, "arm,root-node" is only required for "arm,cmn-600" - it didn't seem worth overcomplicating the schema to actively disallow it for other models, but that is supposed to be implied by its description as "not relevant for newer CMN/CI products".
>>>
>>> If you're hitting this because you've written your ACPI DSDT incorrectly, it's a sign that you should fix your DSDT.
>>>
>>> Thanks,
>>> Robin.
>>>
>>> [1] https://developer.arm.com/documentation/den0093/latest/
>>>
>>
>> Hi Robin,
>>
>> Yes, I know what you mean, but if a new CMN PMU device design two resources like CMN600, it will fail to load driver.
> 
> Future CMN models won't do that, though. And if some design way down the line does end up wanting multiple memory resources for some other reason, the CMN-600 probing code almost certainly won't be the right thing anyway. Besides, firmware bindings for new models will be defined as necessary when those new models are released, and the driver will be updated as necessary at that point to support them, so this is a non-issue.
> 
>> the specification doesn't explicitly forbid giving two ranges for CMN700
> 
> What do you think "specific to the CMN-600 device object" means, exactly?
> 
>> although the example shows just one.
>> So in our SoC, CMN700 is designed with two resources similar to CMN600,
> 
> Why? Either the second resource is meaningless and the correct answer is to remove it from the DSDT, or you've actually modified the hardware design to move the CFG node somewhere else, in which case what you have is no longer an Arm CMN-700 and thus should not be claiming to be one.
> 
>  and the overlap of resource addresses makes
>> the CMN driver unable to match my CMN700. The error log:
>>
>> [  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 this patch can compatible devices which has two memory resources and one memory resource, why not do it?
> 
> The driver supports the standard firmware bindings, because if everyone ignored the standard firmware bindings and did their own thing, there could not be a standard driver. If we can tell that firmware hasn't followed the bindings because it's added random extra resources, how can we be sure that *any* of them will actually be the one we're looking for?
> 
> Please follow the standard firmware bindings and do not hack non-standard behaviours into the driver.
> 

Ok, I think I should remove second resource from my ACPI DSDT. Thank you.

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[Robin Murphy]

On 2023-03-27 15:05, Will Deacon wrote:
> [+Robin and Ilkka, as they contribute most to this driver]
> 
> On Thu, Feb 16, 2023 at 04:17:50PM +0800, Jing Zhang wrote:
>> The devm_platform_ioremap_resource() won't let the platform device
>> claim resource when the ACPI companion device has already claimed it.
>> If CMN-ANY except CMN600 is ACPI companion device, it will return
>> -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
>> successfully installed.
>>
>> So let ACPI companion device 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);
>>   
>>   	return root->start - cfg->start;
>>   }
>>   
>> -static int arm_cmn600_of_probe(struct device_node *np)
>> +static int arm_cmn_of_probe(struct platform_device *pdev, struct arm_cmn *cmn)
>>   {
>>   	u32 rootnode;
>> +	int ret;
>> +
>> +	cmn->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(cmn->base))
>> +		return PTR_ERR(cmn->base);
>>   
>> -	return of_property_read_u32(np, "arm,root-node", &rootnode) ?: rootnode;
>> +	/* If of_property_read_u32() return EINVAL, it means that device tree has
>> +	 * not define root-node, and root-node will return 0, which is compatible
>> +	 * with cmn-600 and cmn-any.
>> +	 */
>> +	ret = of_property_read_u32(pdev->dev.of_node, "arm,root-node", &rootnode);
>> +	if (ret == -EINVAL)
>> +		return 0;

Also NAK to this because it's plain wrong. CMN-600 should fail to probe 
if the property is missing, because assuming an offset of 0 is not correct.

Thanks,
Robin.

>> +
>> +	return rootnode;
>>   }
>>   
>>   static int arm_cmn_probe(struct platform_device *pdev)
>> @@ -2255,16 +2277,11 @@ static int arm_cmn_probe(struct platform_device *pdev)
>>   	cmn->model = (unsigned long)device_get_match_data(cmn->dev);
>>   	platform_set_drvdata(pdev, cmn);
>>   
>> -	if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
>> -		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
>> -	} else {
>> -		rootnode = 0;
>> -		cmn->base = devm_platform_ioremap_resource(pdev, 0);
>> -		if (IS_ERR(cmn->base))
>> -			return PTR_ERR(cmn->base);
>> -		if (cmn->model == CMN600)
>> -			rootnode = arm_cmn600_of_probe(pdev->dev.of_node);
>> -	}
>> +	if (has_acpi_companion(cmn->dev))
>> +		rootnode = arm_cmn_acpi_probe(pdev, cmn);
>> +	else
>> +		rootnode = arm_cmn_of_probe(pdev, cmn);
>> +
>>   	if (rootnode < 0)
>>   		return rootnode;
>>   
>> -- 
>> 1.8.3.1
>>

Re: [PATCH] perf/arm-cmn: Fix and refactor device mapping resource

[YinFengwei]

Hi Robin,

On Mon, Mar 27, 2023 at 03:27:39PM +0100, Robin Murphy wrote:
> On 2023-03-27 15:05, Will Deacon wrote:
> > [+Robin and Ilkka, as they contribute most to this driver]
> > 
> > On Thu, Feb 16, 2023 at 04:17:50PM +0800, Jing Zhang wrote:
> > > The devm_platform_ioremap_resource() won't let the platform device
> > > claim resource when the ACPI companion device has already claimed it.
> > > If CMN-ANY except CMN600 is ACPI companion device, it will return
> > > -EBUSY in devm_platform_ioremap_resource(), and the driver cannot be
> > > successfully installed.
> > > 
> > > So let ACPI companion device 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.
> 
> No, the whole point of CMN-600 probing being a special case is that the ACPI
> and DT bindings for CMN-600 are special cases. In ACPI, only ARMHC600 has
> the two nested memory resources; all the other models should only have one
> memory resource because one is all that is meaningful. See table 16 the
> document[1] in where the description of ROOTNODEBASE says "This field is
> specific to the CMN-600 device object."
> 
> Similarly in DT, "arm,root-node" is only required for "arm,cmn-600" - it
> didn't seem worth overcomplicating the schema to actively disallow it for
> other models, but that is supposed to be implied by its description as "not
> relevant for newer CMN/CI products".
> 
> If you're hitting this because you've written your ACPI DSDT incorrectly,
> it's a sign that you should fix your DSDT.
> 
What about following change? Thanks.

-static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *cmn)
+static int arm_acpi_probe(struct platform_device *pdev, struct arm_cmn *cmn)
 {
-       struct resource *cfg, *root;
+       struct resource *cfg, *root = NULL;

        cfg = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!cfg)
                return -EINVAL;

-       root = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-       if (!root)
-               return -EINVAL;
+       if (cmn->part == PART_CMN600) {
+               /* Per "ACPI for Arm Components" Table 16, CMN600 has
+                * nested root node memory range.
+                */
+               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);
        /*
         * Note that devm_ioremap_resource() is dumb and won't let the platform
         * device claim cfg when the ACPI companion device has already claimed
@@ -2534,7 +2540,7 @@ static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *c
        if (!cmn->base)
                return -ENOMEM;

-       return root->start - cfg->start;
+       return root ? (root->start - cfg->start) : 0;
 }


Regards
Yin, Fengwei