diff mbox

[04/19] ARM64 / ACPI: Introduce arch_fix_phys_package_id() for cpu topology

Message ID 1406206825-15590-5-git-send-email-hanjun.guo@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Hanjun Guo July 24, 2014, 1 p.m. UTC
arch_fix_phys_package_id() will be called in ACPI core to use
the slot number provided by ACPI to update the physical package
id, then we can get the right value in the "physical id" field
of /proc/cpuinfo.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/topology.h |    2 ++
 arch/arm64/kernel/topology.c      |   14 ++++++++++++++
 2 files changed, 16 insertions(+)

Comments

Mark Brown July 24, 2014, 2:43 p.m. UTC | #1
On Thu, Jul 24, 2014 at 09:00:10PM +0800, Hanjun Guo wrote:

> +/*
> + * Use the CPU slot number provided by ACPI to update the physical
> + * package id when cpuid_topo->cluster_id is not available, then we
> + * can get the right value in the "physical id" field of /proc/cpuinfo.
> + */
> +void arch_fix_phys_package_id(int num, u32 slot)
> +{
> +	struct cpu_topology *cpuid_topo = &cpu_topology[num];
> +
> +	if (cpuid_topo->cluster_id == -1)
> +		cpuid_topo->cluster_id = slot;
> +}
> +EXPORT_SYMBOL_GPL(arch_fix_phys_package_id);

As I said on the previous version of this patch are you sure this runs
at a point between the cluster ID getting initialized to -1 and the slot
being initialized (bear in mind that we now use MPIDR information if
availabe).  I don't understand why we don't just unconditionally use the
value given.
Hanjun Guo July 25, 2014, 10:32 a.m. UTC | #2
On 2014-7-24 22:43, Mark Brown wrote:
> On Thu, Jul 24, 2014 at 09:00:10PM +0800, Hanjun Guo wrote:
> 
>> +/*
>> + * Use the CPU slot number provided by ACPI to update the physical
>> + * package id when cpuid_topo->cluster_id is not available, then we
>> + * can get the right value in the "physical id" field of /proc/cpuinfo.
>> + */
>> +void arch_fix_phys_package_id(int num, u32 slot)
>> +{
>> +	struct cpu_topology *cpuid_topo = &cpu_topology[num];
>> +
>> +	if (cpuid_topo->cluster_id == -1)
>> +		cpuid_topo->cluster_id = slot;
>> +}
>> +EXPORT_SYMBOL_GPL(arch_fix_phys_package_id);
> 
> As I said on the previous version of this patch are you sure this runs
> at a point between the cluster ID getting initialized to -1 and the slot
> being initialized (bear in mind that we now use MPIDR information if
> availabe).  I don't understand why we don't just unconditionally use the
> value given.

Ah, sorry, it should be unconditionally used as you said, and I remember
the comments in last version, but the patch shows not, must be something
wrong, will update it in next version.

Thanks
Hanjun
Sudeep Holla July 28, 2014, 6:51 p.m. UTC | #3
On 24/07/14 14:00, Hanjun Guo wrote:
> arch_fix_phys_package_id() will be called in ACPI core to use
> the slot number provided by ACPI to update the physical package
> id, then we can get the right value in the "physical id" field
> of /proc/cpuinfo.
>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>   arch/arm64/include/asm/topology.h |    2 ++
>   arch/arm64/kernel/topology.c      |   14 ++++++++++++++
>   2 files changed, 16 insertions(+)
>
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index 7ebcd31..2b216d4 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -23,11 +23,13 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
>   void init_cpu_topology(void);
>   void store_cpu_topology(unsigned int cpuid);
>   const struct cpumask *cpu_coregroup_mask(int cpu);
> +void arch_fix_phys_package_id(int num, u32 slot);
>
>   #else
>
>   static inline void init_cpu_topology(void) { }
>   static inline void store_cpu_topology(unsigned int cpuid) { }
> +static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>
>   #endif
>
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 43514f9..c547885 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -281,3 +281,17 @@ void __init init_cpu_topology(void)
>   	if (parse_dt_topology())
>   		reset_cpu_topology();
>   }
> +
> +/*
> + * Use the CPU slot number provided by ACPI to update the physical
> + * package id when cpuid_topo->cluster_id is not available, then we
> + * can get the right value in the "physical id" field of /proc/cpuinfo.
> + */

We don't have "physical id" field in /proc/cpuinfo on ARM64.

> +void arch_fix_phys_package_id(int num, u32 slot)
> +{
> +	struct cpu_topology *cpuid_topo = &cpu_topology[num];
> +
> +	if (cpuid_topo->cluster_id == -1)
> +		cpuid_topo->cluster_id = slot;
> +}
> +EXPORT_SYMBOL_GPL(arch_fix_phys_package_id);
>

The ACPI core uses this function to set the package id as read from
_SUN from the device. As per spec, _SUN is used by OSPM UI to identify
slots for the user. Do we know how will this be used on ARM64 ?

If not clear at this time, better to define it or keep it empty. I see
even x86 does nothing in that function.

Regards,
Sudeep
Hanjun Guo Aug. 1, 2014, 6:35 a.m. UTC | #4
On 2014-7-29 2:51, Sudeep Holla wrote:
> On 24/07/14 14:00, Hanjun Guo wrote:
>> arch_fix_phys_package_id() will be called in ACPI core to use
>> the slot number provided by ACPI to update the physical package
>> id, then we can get the right value in the "physical id" field
>> of /proc/cpuinfo.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>   arch/arm64/include/asm/topology.h |    2 ++
>>   arch/arm64/kernel/topology.c      |   14 ++++++++++++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/topology.h
>> b/arch/arm64/include/asm/topology.h
>> index 7ebcd31..2b216d4 100644
>> --- a/arch/arm64/include/asm/topology.h
>> +++ b/arch/arm64/include/asm/topology.h
>> @@ -23,11 +23,13 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
>>   void init_cpu_topology(void);
>>   void store_cpu_topology(unsigned int cpuid);
>>   const struct cpumask *cpu_coregroup_mask(int cpu);
>> +void arch_fix_phys_package_id(int num, u32 slot);
>>
>>   #else
>>
>>   static inline void init_cpu_topology(void) { }
>>   static inline void store_cpu_topology(unsigned int cpuid) { }
>> +static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>>
>>   #endif
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 43514f9..c547885 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -281,3 +281,17 @@ void __init init_cpu_topology(void)
>>       if (parse_dt_topology())
>>           reset_cpu_topology();
>>   }
>> +
>> +/*
>> + * Use the CPU slot number provided by ACPI to update the physical
>> + * package id when cpuid_topo->cluster_id is not available, then we
>> + * can get the right value in the "physical id" field of /proc/cpuinfo.
>> + */
> 
> We don't have "physical id" field in /proc/cpuinfo on ARM64.

I'm just curious, if there are two or more CPU (SoC) chips and connected
into one system, how to tell one SoC from another?

> 
>> +void arch_fix_phys_package_id(int num, u32 slot)
>> +{
>> +    struct cpu_topology *cpuid_topo = &cpu_topology[num];
>> +
>> +    if (cpuid_topo->cluster_id == -1)
>> +        cpuid_topo->cluster_id = slot;
>> +}
>> +EXPORT_SYMBOL_GPL(arch_fix_phys_package_id);
>>
> 
> The ACPI core uses this function to set the package id as read from
> _SUN from the device. As per spec, _SUN is used by OSPM UI to identify
> slots for the user. Do we know how will this be used on ARM64 ?
> 
> If not clear at this time, better to define it or keep it empty. I see
> even x86 does nothing in that function.

I'm ok with it, will introduce a stub function for it and remove this patch.

Thanks
Hanjun
Sudeep Holla Aug. 1, 2014, 10:48 a.m. UTC | #5
On 01/08/14 07:35, Hanjun Guo wrote:
> On 2014-7-29 2:51, Sudeep Holla wrote:
>> On 24/07/14 14:00, Hanjun Guo wrote:
>>> arch_fix_phys_package_id() will be called in ACPI core to use
>>> the slot number provided by ACPI to update the physical package
>>> id, then we can get the right value in the "physical id" field
>>> of /proc/cpuinfo.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>    arch/arm64/include/asm/topology.h |    2 ++
>>>    arch/arm64/kernel/topology.c      |   14 ++++++++++++++
>>>    2 files changed, 16 insertions(+)
>>>
>>> diff --git a/arch/arm64/include/asm/topology.h
>>> b/arch/arm64/include/asm/topology.h
>>> index 7ebcd31..2b216d4 100644
>>> --- a/arch/arm64/include/asm/topology.h
>>> +++ b/arch/arm64/include/asm/topology.h
>>> @@ -23,11 +23,13 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
>>>    void init_cpu_topology(void);
>>>    void store_cpu_topology(unsigned int cpuid);
>>>    const struct cpumask *cpu_coregroup_mask(int cpu);
>>> +void arch_fix_phys_package_id(int num, u32 slot);
>>>
>>>    #else
>>>
>>>    static inline void init_cpu_topology(void) { }
>>>    static inline void store_cpu_topology(unsigned int cpuid) { }
>>> +static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>>>
>>>    #endif
>>>
>>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>>> index 43514f9..c547885 100644
>>> --- a/arch/arm64/kernel/topology.c
>>> +++ b/arch/arm64/kernel/topology.c
>>> @@ -281,3 +281,17 @@ void __init init_cpu_topology(void)
>>>        if (parse_dt_topology())
>>>            reset_cpu_topology();
>>>    }
>>> +
>>> +/*
>>> + * Use the CPU slot number provided by ACPI to update the physical
>>> + * package id when cpuid_topo->cluster_id is not available, then we
>>> + * can get the right value in the "physical id" field of /proc/cpuinfo.
>>> + */
>>
>> We don't have "physical id" field in /proc/cpuinfo on ARM64.
>
> I'm just curious, if there are two or more CPU (SoC) chips and connected
> into one system, how to tell one SoC from another?
>

There was some discussion on ARM64 /proc/cpuinfo recently[1]. Even ARM
doesn't have this. IMO we might want to present complete MPIDR in future
if really needed.

Not sure, why we need this "physical id" field as these cpuid topology
is already visible via sysfs explicitly(we really don't need that again
elsewhere ?)

Regards,
Sudeep

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/264390.html
diff mbox

Patch

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 7ebcd31..2b216d4 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -23,11 +23,13 @@  extern struct cpu_topology cpu_topology[NR_CPUS];
 void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
+void arch_fix_phys_package_id(int num, u32 slot);
 
 #else
 
 static inline void init_cpu_topology(void) { }
 static inline void store_cpu_topology(unsigned int cpuid) { }
+static inline void arch_fix_phys_package_id(int num, u32 slot) { }
 
 #endif
 
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 43514f9..c547885 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -281,3 +281,17 @@  void __init init_cpu_topology(void)
 	if (parse_dt_topology())
 		reset_cpu_topology();
 }
+
+/*
+ * Use the CPU slot number provided by ACPI to update the physical
+ * package id when cpuid_topo->cluster_id is not available, then we
+ * can get the right value in the "physical id" field of /proc/cpuinfo.
+ */
+void arch_fix_phys_package_id(int num, u32 slot)
+{
+	struct cpu_topology *cpuid_topo = &cpu_topology[num];
+
+	if (cpuid_topo->cluster_id == -1)
+		cpuid_topo->cluster_id = slot;
+}
+EXPORT_SYMBOL_GPL(arch_fix_phys_package_id);