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