Re: [PATCH 2/2] perf: arm_dsu: Support DSU ACPI devices.

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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 <suzuki.poulose@arm.com> 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 <tuanphan@os.amperecomputing.com>

...

>>> +#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