Add support for probing device from ACPI node.
Each DSU ACPI node defines "cpus" package which
each element is the MPIDR of associated cpu.
Signed-off-by: Tuan Phan <[email protected]>
---
drivers/perf/arm_dsu_pmu.c | 53 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 45 insertions(+), 8 deletions(-)
diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c
index 2622900..6ef762c 100644
--- a/drivers/perf/arm_dsu_pmu.c
+++ b/drivers/perf/arm_dsu_pmu.c
@@ -11,6 +11,7 @@
#define DRVNAME PMUNAME "_pmu"
#define pr_fmt(fmt) DRVNAME ": " fmt
+#include <linux/acpi.h>
#include <linux/bitmap.h>
#include <linux/bitops.h>
#include <linux/bug.h>
@@ -603,18 +604,22 @@ static struct dsu_pmu *dsu_pmu_alloc(struct platform_device *pdev)
}
/**
- * dsu_pmu_dt_get_cpus: Get the list of CPUs in the cluster.
+ * dsu_pmu_get_cpus: Get the list of CPUs in the cluster.
*/
-static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
+static int dsu_pmu_get_cpus(struct platform_device *pdev)
{
+#ifndef CONFIG_ACPI
+ /* Get the list of CPUs from device tree */
int i = 0, n, cpu;
struct device_node *cpu_node;
+ struct dsu_pmu *dsu_pmu =
+ (struct dsu_pmu *) platform_get_drvdata(pdev);
- n = of_count_phandle_with_args(dev, "cpus", NULL);
+ n = of_count_phandle_with_args(pdev->dev.of_node, "cpus", NULL);
if (n <= 0)
return -ENODEV;
for (; i < n; i++) {
- cpu_node = of_parse_phandle(dev, "cpus", i);
+ cpu_node = of_parse_phandle(pdev->dev.of_node, "cpus", i);
if (!cpu_node)
break;
cpu = of_cpu_node_to_id(cpu_node);
@@ -626,9 +631,33 @@ static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
*/
if (cpu < 0)
continue;
- cpumask_set_cpu(cpu, mask);
+ cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
}
return 0;
+#else /* CONFIG_ACPI */
+ int i, cpu, ret;
+ const union acpi_object *obj;
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+ struct dsu_pmu *dsu_pmu =
+ (struct dsu_pmu *) platform_get_drvdata(pdev);
+
+ ret = acpi_dev_get_property(adev, "cpus", ACPI_TYPE_ANY, &obj);
+ if (ret < 0)
+ return -EINVAL;
+
+ if (obj->type != ACPI_TYPE_PACKAGE)
+ return -EINVAL;
+
+ for (i = 0; i < obj->package.count; i++) {
+ /* Each element is the MPIDR of associated cpu */
+ for_each_possible_cpu(cpu) {
+ if (cpu_physical_id(cpu) ==
+ obj->package.elements[i].integer.value)
+ cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
+ }
+ }
+ return 0;
+#endif
}
/*
@@ -683,7 +712,9 @@ static int dsu_pmu_device_probe(struct platform_device *pdev)
if (IS_ERR(dsu_pmu))
return PTR_ERR(dsu_pmu);
- rc = dsu_pmu_dt_get_cpus(pdev->dev.of_node, &dsu_pmu->associated_cpus);
+ platform_set_drvdata(pdev, dsu_pmu);
+
+ rc = dsu_pmu_get_cpus(pdev);
if (rc) {
dev_warn(&pdev->dev, "Failed to parse the CPUs\n");
return rc;
@@ -707,7 +738,6 @@ static int dsu_pmu_device_probe(struct platform_device *pdev)
}
dsu_pmu->irq = irq;
- platform_set_drvdata(pdev, dsu_pmu);
rc = cpuhp_state_add_instance(dsu_pmu_cpuhp_state,
&dsu_pmu->cpuhp_node);
if (rc)
@@ -754,11 +784,19 @@ static const struct of_device_id dsu_pmu_of_match[] = {
{ .compatible = "arm,dsu-pmu", },
{},
};
+MODULE_DEVICE_TABLE(of, dsu_pmu_of_match);
+
+static const struct acpi_device_id dsu_pmu_acpi_match[] = {
+ { "ARMHD500", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(acpi, dsu_pmu_acpi_match);
static struct platform_driver dsu_pmu_driver = {
.driver = {
.name = DRVNAME,
.of_match_table = of_match_ptr(dsu_pmu_of_match),
+ .acpi_match_table = ACPI_PTR(dsu_pmu_acpi_match),
},
.probe = dsu_pmu_device_probe,
.remove = dsu_pmu_device_remove,
@@ -827,7 +865,6 @@ static void __exit dsu_pmu_exit(void)
module_init(dsu_pmu_init);
module_exit(dsu_pmu_exit);
-MODULE_DEVICE_TABLE(of, dsu_pmu_of_match);
MODULE_DESCRIPTION("Perf driver for ARM DynamIQ Shared Unit");
MODULE_AUTHOR("Suzuki K Poulose <[email protected]>");
MODULE_LICENSE("GPL v2");
--
2.7.4
On 2020-03-18 12:28 am, Tuan Phan wrote:
> Add support for probing device from ACPI node.
> Each DSU ACPI node defines "cpus" package which
> each element is the MPIDR of associated cpu.
>
> Signed-off-by: Tuan Phan <[email protected]>
> ---
> drivers/perf/arm_dsu_pmu.c | 53 +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c
> index 2622900..6ef762c 100644
> --- a/drivers/perf/arm_dsu_pmu.c
> +++ b/drivers/perf/arm_dsu_pmu.c
> @@ -11,6 +11,7 @@
> #define DRVNAME PMUNAME "_pmu"
> #define pr_fmt(fmt) DRVNAME ": " fmt
>
> +#include <linux/acpi.h>
> #include <linux/bitmap.h>
> #include <linux/bitops.h>
> #include <linux/bug.h>
> @@ -603,18 +604,22 @@ static struct dsu_pmu *dsu_pmu_alloc(struct platform_device *pdev)
> }
>
> /**
> - * dsu_pmu_dt_get_cpus: Get the list of CPUs in the cluster.
> + * dsu_pmu_get_cpus: Get the list of CPUs in the cluster.
> */
> -static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
> +static int dsu_pmu_get_cpus(struct platform_device *pdev)
> {
> +#ifndef CONFIG_ACPI
So this basically disables DT support entirely for distro kernels... no
thanks.
If generic device properties aren't enough to abstract the differences,
then create separate DT and ACPI init functions, and dispatch to them
from probe depending on whether the device has an OF node or an ACPI
companion. It can't be a build-time decision.
Robin.
> + /* Get the list of CPUs from device tree */
> int i = 0, n, cpu;
> struct device_node *cpu_node;
> + struct dsu_pmu *dsu_pmu =
> + (struct dsu_pmu *) platform_get_drvdata(pdev);
>
> - n = of_count_phandle_with_args(dev, "cpus", NULL);
> + n = of_count_phandle_with_args(pdev->dev.of_node, "cpus", NULL);
> if (n <= 0)
> return -ENODEV;
> for (; i < n; i++) {
> - cpu_node = of_parse_phandle(dev, "cpus", i);
> + cpu_node = of_parse_phandle(pdev->dev.of_node, "cpus", i);
> if (!cpu_node)
> break;
> cpu = of_cpu_node_to_id(cpu_node);
> @@ -626,9 +631,33 @@ static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
> */
> if (cpu < 0)
> continue;
> - cpumask_set_cpu(cpu, mask);
> + cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
> }
> return 0;
> +#else /* CONFIG_ACPI */
> + int i, cpu, ret;
> + const union acpi_object *obj;
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + struct dsu_pmu *dsu_pmu =
> + (struct dsu_pmu *) platform_get_drvdata(pdev);
> +
> + ret = acpi_dev_get_property(adev, "cpus", ACPI_TYPE_ANY, &obj);
> + if (ret < 0)
> + return -EINVAL;
> +
> + if (obj->type != ACPI_TYPE_PACKAGE)
> + return -EINVAL;
> +
> + for (i = 0; i < obj->package.count; i++) {
> + /* Each element is the MPIDR of associated cpu */
> + for_each_possible_cpu(cpu) {
> + if (cpu_physical_id(cpu) ==
> + obj->package.elements[i].integer.value)
> + cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
> + }
> + }
> + return 0;
> +#endif
> }
>
> /*
> @@ -683,7 +712,9 @@ static int dsu_pmu_device_probe(struct platform_device *pdev)
> if (IS_ERR(dsu_pmu))
> return PTR_ERR(dsu_pmu);
>
> - rc = dsu_pmu_dt_get_cpus(pdev->dev.of_node, &dsu_pmu->associated_cpus);
> + platform_set_drvdata(pdev, dsu_pmu);
> +
> + rc = dsu_pmu_get_cpus(pdev);
> if (rc) {
> dev_warn(&pdev->dev, "Failed to parse the CPUs\n");
> return rc;
> @@ -707,7 +738,6 @@ static int dsu_pmu_device_probe(struct platform_device *pdev)
> }
>
> dsu_pmu->irq = irq;
> - platform_set_drvdata(pdev, dsu_pmu);
> rc = cpuhp_state_add_instance(dsu_pmu_cpuhp_state,
> &dsu_pmu->cpuhp_node);
> if (rc)
> @@ -754,11 +784,19 @@ static const struct of_device_id dsu_pmu_of_match[] = {
> { .compatible = "arm,dsu-pmu", },
> {},
> };
> +MODULE_DEVICE_TABLE(of, dsu_pmu_of_match);
> +
> +static const struct acpi_device_id dsu_pmu_acpi_match[] = {
> + { "ARMHD500", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, dsu_pmu_acpi_match);
>
> static struct platform_driver dsu_pmu_driver = {
> .driver = {
> .name = DRVNAME,
> .of_match_table = of_match_ptr(dsu_pmu_of_match),
> + .acpi_match_table = ACPI_PTR(dsu_pmu_acpi_match),
> },
> .probe = dsu_pmu_device_probe,
> .remove = dsu_pmu_device_remove,
> @@ -827,7 +865,6 @@ static void __exit dsu_pmu_exit(void)
> module_init(dsu_pmu_init);
> module_exit(dsu_pmu_exit);
>
> -MODULE_DEVICE_TABLE(of, dsu_pmu_of_match);
> MODULE_DESCRIPTION("Perf driver for ARM DynamIQ Shared Unit");
> MODULE_AUTHOR("Suzuki K Poulose <[email protected]>");
> MODULE_LICENSE("GPL v2");
>
Hello,
Please find my comments below.
On 03/18/2020 12:28 AM, Tuan Phan wrote:
> Add support for probing device from ACPI node.
> Each DSU ACPI node defines "cpus" package which
> each element is the MPIDR of associated cpu.
>
> Signed-off-by: Tuan Phan <[email protected]>
> ---
> drivers/perf/arm_dsu_pmu.c | 53 +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c Tua
> index 2622900..6ef762c 100644
> --- a/drivers/perf/arm_dsu_pmu.c
> +++ b/drivers/perf/arm_dsu_pmu.c
> @@ -11,6 +11,7 @@
> #define DRVNAME PMUNAME "_pmu"
> #define pr_fmt(fmt) DRVNAME ": " fmt
>
> +#include <linux/acpi.h>
> #include <linux/bitmap.h>
> #include <linux/bitops.h>
> #include <linux/bug.h>
> @@ -603,18 +604,22 @@ static struct dsu_pmu *dsu_pmu_alloc(struct platform_device *pdev)
> }
>
> /**
> - * dsu_pmu_dt_get_cpus: Get the list of CPUs in the cluster.
> + * dsu_pmu_get_cpus: Get the list of CPUs in the cluster.
> */
> -static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
> +static int dsu_pmu_get_cpus(struct platform_device *pdev)
> {
> +#ifndef CONFIG_ACPI
> + /* Get the list of CPUs from device tree */
What if we have a kernel with both:
CONFIG_OF=y
CONFIG_ACPI=y
and boot the kernel on a system with DT ? In other words, the decision
to choose the DT vs ACPI must be runtime decision, not buildtime.
See
drivers/hwtracing/coresight/coresight-platform.c:coresight_get_platform_data()
for an example.
> int i = 0, n, cpu;
> struct device_node *cpu_node;
> + struct dsu_pmu *dsu_pmu =
> + (struct dsu_pmu *) platform_get_drvdata(pdev);
>
> - n = of_count_phandle_with_args(dev, "cpus", NULL);
> + n = of_count_phandle_with_args(pdev->dev.of_node, "cpus", NULL);
> if (n <= 0)
> return -ENODEV;
> for (; i < n; i++) {
> - cpu_node = of_parse_phandle(dev, "cpus", i);
> + cpu_node = of_parse_phandle(pdev->dev.of_node, "cpus", i);
> if (!cpu_node)
> break;
> cpu = of_cpu_node_to_id(cpu_node);
> @@ -626,9 +631,33 @@ static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
> */
> if (cpu < 0)
> continue;
> - cpumask_set_cpu(cpu, mask);
> + cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
> }
> return 0;
> +#else /* CONFIG_ACPI */
> + int i, cpu, ret;
> + const union acpi_object *obj;
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + struct dsu_pmu *dsu_pmu =
> + (struct dsu_pmu *) platform_get_drvdata(pdev);
> +
> + ret = acpi_dev_get_property(adev, "cpus", ACPI_TYPE_ANY, &obj);
Is the binding documented somewhere ?
nit: Also, why not :
ret = acpi_dev_get_propert(adev, "cpus", ACPI_TYPE_PACKAGE, &obj);
if (ret < 0)
return ret;
?
> + if (ret < 0)
> + return -EINVAL;
> +
> + if (obj->type != ACPI_TYPE_PACKAGE)
> + return -EINVAL;
> +
> + for (i = 0; i < obj->package.count; i++) {
> + /* Each element is the MPIDR of associated cpu */
> + for_each_possible_cpu(cpu) {
> + if (cpu_physical_id(cpu) ==
> + obj->package.elements[i].integer.value)
> + cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
> + }
> + }
> + return 0;
> +#endif
> }
>
Otherwise looks good to me.
Suzuki
Thanks Suzuki and Robin for quick reply.
Please find my comments below.
> On Mar 18, 2020, at 5:45 PM, Suzuki K Poulose <[email protected]> wrote:
>
> Hello,
>
>
> Please find my comments below.
>
> On 03/18/2020 12:28 AM, Tuan Phan wrote:
>> Add support for probing device from ACPI node.
>> Each DSU ACPI node defines "cpus" package which
>> each element is the MPIDR of associated cpu.
>> Signed-off-by: Tuan Phan <[email protected]>
>> ---
>> drivers/perf/arm_dsu_pmu.c | 53 +++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 45 insertions(+), 8 deletions(-)
>> diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c Tua
>> index 2622900..6ef762c 100644
>> --- a/drivers/perf/arm_dsu_pmu.c
>> +++ b/drivers/perf/arm_dsu_pmu.c
>> @@ -11,6 +11,7 @@
>> #define DRVNAME PMUNAME "_pmu"
>> #define pr_fmt(fmt) DRVNAME ": " fmt
>> +#include <linux/acpi.h>
>> #include <linux/bitmap.h>
>> #include <linux/bitops.h>
>> #include <linux/bug.h>
>> @@ -603,18 +604,22 @@ static struct dsu_pmu *dsu_pmu_alloc(struct platform_device *pdev)
>> }
>> /**
>> - * dsu_pmu_dt_get_cpus: Get the list of CPUs in the cluster.
>> + * dsu_pmu_get_cpus: Get the list of CPUs in the cluster.
>> */
>> -static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
>> +static int dsu_pmu_get_cpus(struct platform_device *pdev)
>> {
>> +#ifndef CONFIG_ACPI
>> + /* Get the list of CPUs from device tree */
>
> What if we have a kernel with both:
>
> CONFIG_OF=y
> CONFIG_ACPI=y
>
> and boot the kernel on a system with DT ? In other words, the decision
> to choose the DT vs ACPI must be runtime decision, not buildtime.
>
> See drivers/hwtracing/coresight/coresight-platform.c:coresight_get_platform_data() for an example.
I will update so it can be detected in runtime.
>> int i = 0, n, cpu;
>> struct device_node *cpu_node;
>> + struct dsu_pmu *dsu_pmu =
>> + (struct dsu_pmu *) platform_get_drvdata(pdev);
>> - n = of_count_phandle_with_args(dev, "cpus", NULL);
>> + n = of_count_phandle_with_args(pdev->dev.of_node, "cpus", NULL);
>> if (n <= 0)
>> return -ENODEV;
>> for (; i < n; i++) {
>> - cpu_node = of_parse_phandle(dev, "cpus", i);
>> + cpu_node = of_parse_phandle(pdev->dev.of_node, "cpus", i);
>> if (!cpu_node)
>> break;
>> cpu = of_cpu_node_to_id(cpu_node);
>> @@ -626,9 +631,33 @@ static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
>> */
>> if (cpu < 0)
>> continue;
>> - cpumask_set_cpu(cpu, mask);
>> + cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
>> }
>> return 0;
>> +#else /* CONFIG_ACPI */
>> + int i, cpu, ret;
>> + const union acpi_object *obj;
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> + struct dsu_pmu *dsu_pmu =
>> + (struct dsu_pmu *) platform_get_drvdata(pdev);
>> +
>
>> + ret = acpi_dev_get_property(adev, "cpus", ACPI_TYPE_ANY, &obj);
>
> Is the binding documented somewhere ?
=> Will add one.
>
>
> nit: Also, why not :
> ret = acpi_dev_get_propert(adev, "cpus", ACPI_TYPE_PACKAGE, &obj);
> if (ret < 0)
> return ret;
> ?
>
>
>> + if (ret < 0)
>> + return -EINVAL;
>> +
>> + if (obj->type != ACPI_TYPE_PACKAGE)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < obj->package.count; i++) {
>
>
>> + /* Each element is the MPIDR of associated cpu */
>> + for_each_possible_cpu(cpu) {
>> + if (cpu_physical_id(cpu) ==
>> + obj->package.elements[i].integer.value)
>> + cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
>> + }
>> + }
>> + return 0;
>> +#endif
>> }
>>
>
> Otherwise looks good to me.
>
> Suzuki
Hi Suzuki,
> On Mar 18, 2020, at 5:45 PM, Suzuki K Poulose <[email protected]> wrote:
>
> Hello,
>
>
> Please find my comments below.
>
> On 03/18/2020 12:28 AM, Tuan Phan wrote:
>> Add support for probing device from ACPI node.
>> Each DSU ACPI node defines "cpus" package which
>> each element is the MPIDR of associated cpu.
>> Signed-off-by: Tuan Phan <[email protected]>
>> ---
>> drivers/perf/arm_dsu_pmu.c | 53 +++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 45 insertions(+), 8 deletions(-)
>> diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c Tua
>> index 2622900..6ef762c 100644
>> --- a/drivers/perf/arm_dsu_pmu.c
>> +++ b/drivers/perf/arm_dsu_pmu.c
>> @@ -11,6 +11,7 @@
>> #define DRVNAME PMUNAME "_pmu"
>> #define pr_fmt(fmt) DRVNAME ": " fmt
>> +#include <linux/acpi.h>
>> #include <linux/bitmap.h>
>> #include <linux/bitops.h>
>> #include <linux/bug.h>
>> @@ -603,18 +604,22 @@ static struct dsu_pmu *dsu_pmu_alloc(struct platform_device *pdev)
>> }
>> /**
>> - * dsu_pmu_dt_get_cpus: Get the list of CPUs in the cluster.
>> + * dsu_pmu_get_cpus: Get the list of CPUs in the cluster.
>> */
>> -static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
>> +static int dsu_pmu_get_cpus(struct platform_device *pdev)
>> {
>> +#ifndef CONFIG_ACPI
>> + /* Get the list of CPUs from device tree */
>
> What if we have a kernel with both:
>
> CONFIG_OF=y
> CONFIG_ACPI=y
>
> and boot the kernel on a system with DT ? In other words, the decision
> to choose the DT vs ACPI must be runtime decision, not buildtime.
>
> See drivers/hwtracing/coresight/coresight-platform.c:coresight_get_platform_data() for an example.
>
>> int i = 0, n, cpu;
>> struct device_node *cpu_node;
>> + struct dsu_pmu *dsu_pmu =
>> + (struct dsu_pmu *) platform_get_drvdata(pdev);
>> - n = of_count_phandle_with_args(dev, "cpus", NULL);
>> + n = of_count_phandle_with_args(pdev->dev.of_node, "cpus", NULL);
>> if (n <= 0)
>> return -ENODEV;
>> for (; i < n; i++) {
>> - cpu_node = of_parse_phandle(dev, "cpus", i);
>> + cpu_node = of_parse_phandle(pdev->dev.of_node, "cpus", i);
>> if (!cpu_node)
>> break;
>> cpu = of_cpu_node_to_id(cpu_node);
>> @@ -626,9 +631,33 @@ static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
>> */
>> if (cpu < 0)
>> continue;
>> - cpumask_set_cpu(cpu, mask);
>> + cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
>> }
>> return 0;
>> +#else /* CONFIG_ACPI */
>> + int i, cpu, ret;
>> + const union acpi_object *obj;
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> + struct dsu_pmu *dsu_pmu =
>> + (struct dsu_pmu *) platform_get_drvdata(pdev);
>> +
>
>> + ret = acpi_dev_get_property(adev, "cpus", ACPI_TYPE_ANY, &obj);
>
> Is the binding documented somewhere ?
>
>
> nit: Also, why not :
> ret = acpi_dev_get_propert(adev, "cpus", ACPI_TYPE_PACKAGE, &obj);
> if (ret < 0)
> return ret;
> ?
=> I couldn’t find the device tree binding document of DSU anywhere. Is It enough
to put a comment describing the acpi binding in the code or need somewhere else?
>
>
>> + if (ret < 0)
>> + return -EINVAL;
>> +
>> + if (obj->type != ACPI_TYPE_PACKAGE)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < obj->package.count; i++) {
>
>
>> + /* Each element is the MPIDR of associated cpu */
>> + for_each_possible_cpu(cpu) {
>> + if (cpu_physical_id(cpu) ==
>> + obj->package.elements[i].integer.value)
>> + cpumask_set_cpu(cpu, &dsu_pmu->associated_cpus);
>> + }
>> + }
>> + return 0;
>> +#endif
>> }
>>
>
> Otherwise looks good to me.
>
> Suzuki
Hi Tuan
On 03/19/2020 10:49 PM, Tuan Phan wrote:
> Hi Suzuki,
>
>> On Mar 18, 2020, at 5:45 PM, Suzuki K Poulose <[email protected]> wrote:
>>
>> Hello,
>>
>>
>> Please find my comments below.
>>
>> On 03/18/2020 12:28 AM, Tuan Phan wrote:
>>> Add support for probing device from ACPI node.
>>> Each DSU ACPI node defines "cpus" package which
>>> each element is the MPIDR of associated cpu.
>>> Signed-off-by: Tuan Phan <[email protected]>
...
>>> +#else /* CONFIG_ACPI */
>>> + int i, cpu, ret;
>>> + const union acpi_object *obj;
>>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>>> + struct dsu_pmu *dsu_pmu =
>>> + (struct dsu_pmu *) platform_get_drvdata(pdev);
>>> +
>>
>>> + ret = acpi_dev_get_property(adev, "cpus", ACPI_TYPE_ANY, &obj);
>>
>> Is the binding documented somewhere ?
>>
>>
>> nit: Also, why not :
>> ret = acpi_dev_get_propert(adev, "cpus", ACPI_TYPE_PACKAGE, &obj);
>> if (ret < 0)
>> return ret;
>> ?
> => I couldn’t find the device tree binding document of DSU anywhere. Is It enough
The DT bindings are here :
Documentation/devicetree/bindings/arm/arm-dsu-pmu.txt
> to put a comment describing the acpi binding in the code or need somewhere else?
The concern here is that we are simply trying to replicate the DT
binding here, especially replacing the CPU phandles with MPIDRs.
I am not an expert in the ACPI bindings, but I prefer ACPI
phandle reference to the CPUs (which is much simpler) to
MPIDRs (which is not that intuitive). And this is the same message
that I got from our ACPI folks.
Irrespective of what we end up with, this must be part of the "ACPI
bindings" document here :
DEN0093 - Generic ACPI for Arm Components x.y Platform Design Document
So that everybody uses the same bindings irrespective of the OS.
You don't need to document the bindings here with the Linux kernel code.
Kind regards
Suzuki