| Message ID | 20191002114508.1089-1-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dt: check MPIDR on MP devices built without SMP | expand |
Am 02.10.19 um 13:45 schrieb Nicolas Saenz Julienne: > Currently, in arm_dt_init_cpu_maps(), the hwid of the boot CPU is read > from MPIDR on SMP devices and set to 0 for non SMP. This value is then > matched with the DT cpu nodes' reg property in order to find the boot > CPU in DT. > > On MP devices build without SMP the cpu DT node contains the expected > MPIDR yet the hwid is set to 0. With this the function fails to match > the cpus and uses the default CPU logical map. Making it impossible to > get the CPU's DT node further down the line. This causes issues with > cpufreq-dt, as it triggers warnings when not finding a suitable DT node > on CPU0. > > Change the way we choose whether to get MPIDR or not. Instead of > depending on SMP check the number of CPUs defined in DT. Anything > 1 > means MPIDR will be available. > > This was seen on a Raspberry Pi 2 build with bcm2835_defconfig. > > Reported-by: "kernelci.org bot" <bot@kernelci.org> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- Tested-by: Stefan Wahren <wahrenst@gmx.net> Thanks
On 10/2/19 4:45 AM, Nicolas Saenz Julienne wrote: > Currently, in arm_dt_init_cpu_maps(), the hwid of the boot CPU is read > from MPIDR on SMP devices and set to 0 for non SMP. This value is then > matched with the DT cpu nodes' reg property in order to find the boot > CPU in DT. The code you change is about the "mpidr" variable, yet in your commit message you refer to "hwid", that is a tad confusing for the reader. > > On MP devices build without SMP the cpu DT node contains the expected > MPIDR yet the hwid is set to 0. With this the function fails to match > the cpus and uses the default CPU logical map. Making it impossible to > get the CPU's DT node further down the line. This causes issues with > cpufreq-dt, as it triggers warnings when not finding a suitable DT node > on CPU0. > > Change the way we choose whether to get MPIDR or not. Instead of > depending on SMP check the number of CPUs defined in DT. Anything > 1 > means MPIDR will be available. Except if someone accidentally wrote their Device Tree such as to have > 1 CPU nodes, yet the CPU is not MP capable and reading the MPIDR register does return the expected value, but that is wrong anyway. > > This was seen on a Raspberry Pi 2 build with bcm2835_defconfig. > > Reported-by: "kernelci.org bot" <bot@kernelci.org> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > arch/arm/kernel/devtree.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c > index 39c978698406..a924fda9abc8 100644 > --- a/arch/arm/kernel/devtree.c > +++ b/arch/arm/kernel/devtree.c > @@ -74,7 +74,7 @@ void __init arm_dt_init_cpu_maps(void) > struct device_node *cpu, *cpus; > int found_method = 0; > u32 i, j, cpuidx = 1; > - u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0; > + u32 mpidr = 0; > > u32 tmp_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID }; > bool bootcpu_valid = false; > @@ -83,6 +83,9 @@ void __init arm_dt_init_cpu_maps(void) > if (!cpus) > return; > > + if (is_smp() || of_get_child_count(cpus) > 1) > + mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; > + > for_each_of_cpu_node(cpu) { > const __be32 *cell; > int prop_bytes; >
On Thu, 2019-10-03 at 11:08 -0700, Florian Fainelli wrote: > On 10/2/19 4:45 AM, Nicolas Saenz Julienne wrote: > > Currently, in arm_dt_init_cpu_maps(), the hwid of the boot CPU is read > > from MPIDR on SMP devices and set to 0 for non SMP. This value is then > > matched with the DT cpu nodes' reg property in order to find the boot > > CPU in DT. > > The code you change is about the "mpidr" variable, yet in your commit > message you refer to "hwid", that is a tad confusing for the reader. Sorry, it's indeed pretty confusing. I'll send a new version with a fixed description if there is no major push back. > > On MP devices build without SMP the cpu DT node contains the expected > > MPIDR yet the hwid is set to 0. With this the function fails to match > > the cpus and uses the default CPU logical map. Making it impossible to > > get the CPU's DT node further down the line. This causes issues with > > cpufreq-dt, as it triggers warnings when not finding a suitable DT node > > on CPU0. > > > > Change the way we choose whether to get MPIDR or not. Instead of > > depending on SMP check the number of CPUs defined in DT. Anything > 1 > > means MPIDR will be available. > > Except if someone accidentally wrote their Device Tree such as to have > > 1 CPU nodes, yet the CPU is not MP capable and reading the MPIDR > register does return the expected value, but that is wrong anyway. An UP device will most likely not have a MPIDR. That said I'm not sure this is always true. As per ARM1176JZ's TRM[1], the RPi1 CPU, if one was to get the MPIDR it would raise an undefined exception. The way I see it's an acceptable outcome as the DT is clearly wrong. Regarda, Nicolas [1] See 3.1.10 Use of the system control coprocessor in http://infocenter.arm.com/help/topic/com.arm.doc.ddi0333h/DDI0333H_arm1176jzs_r0p7_trm.pdf: "Attempting to read from a nonreadable register, or to write to a nonwriteable register causes Undefined exceptions."
On 10/3/19 12:39 PM, Nicolas Saenz Julienne wrote: > On Thu, 2019-10-03 at 11:08 -0700, Florian Fainelli wrote: >> On 10/2/19 4:45 AM, Nicolas Saenz Julienne wrote: >>> Currently, in arm_dt_init_cpu_maps(), the hwid of the boot CPU is read >>> from MPIDR on SMP devices and set to 0 for non SMP. This value is then >>> matched with the DT cpu nodes' reg property in order to find the boot >>> CPU in DT. >> >> The code you change is about the "mpidr" variable, yet in your commit >> message you refer to "hwid", that is a tad confusing for the reader. > > Sorry, it's indeed pretty confusing. I'll send a new version with a fixed > description if there is no major push back. > >>> On MP devices build without SMP the cpu DT node contains the expected >>> MPIDR yet the hwid is set to 0. With this the function fails to match >>> the cpus and uses the default CPU logical map. Making it impossible to >>> get the CPU's DT node further down the line. This causes issues with >>> cpufreq-dt, as it triggers warnings when not finding a suitable DT node >>> on CPU0. >>> >>> Change the way we choose whether to get MPIDR or not. Instead of >>> depending on SMP check the number of CPUs defined in DT. Anything > 1 >>> means MPIDR will be available. >> >> Except if someone accidentally wrote their Device Tree such as to have > >> 1 CPU nodes, yet the CPU is not MP capable and reading the MPIDR >> register does return the expected value, but that is wrong anyway. > > An UP device will most likely not have a MPIDR. That said I'm not sure this is > always true. As per ARM1176JZ's TRM[1], the RPi1 CPU, if one was to get the > MPIDR it would raise an undefined exception. > > The way I see it's an acceptable outcome as the DT is clearly wrong. It is, although you probably want to use of_get_available_child_count() instead of of_get_child_count() since we could imagine that a boot loader or some other boot program mangling the DT could intentionally put a 'status = "disabled"' property on the non-boot CPU node for whatever reason.
On Thu, 2019-10-03 at 16:47 -0700, Florian Fainelli wrote: > On 10/3/19 12:39 PM, Nicolas Saenz Julienne wrote: > > On Thu, 2019-10-03 at 11:08 -0700, Florian Fainelli wrote: > > > On 10/2/19 4:45 AM, Nicolas Saenz Julienne wrote: > > > > Currently, in arm_dt_init_cpu_maps(), the hwid of the boot CPU is read > > > > from MPIDR on SMP devices and set to 0 for non SMP. This value is then > > > > matched with the DT cpu nodes' reg property in order to find the boot > > > > CPU in DT. > > > > > > The code you change is about the "mpidr" variable, yet in your commit > > > message you refer to "hwid", that is a tad confusing for the reader. > > > > Sorry, it's indeed pretty confusing. I'll send a new version with a fixed > > description if there is no major push back. > > > > > > On MP devices build without SMP the cpu DT node contains the expected > > > > MPIDR yet the hwid is set to 0. With this the function fails to match > > > > the cpus and uses the default CPU logical map. Making it impossible to > > > > get the CPU's DT node further down the line. This causes issues with > > > > cpufreq-dt, as it triggers warnings when not finding a suitable DT node > > > > on CPU0. > > > > > > > > Change the way we choose whether to get MPIDR or not. Instead of > > > > depending on SMP check the number of CPUs defined in DT. Anything > 1 > > > > means MPIDR will be available. > > > > > > Except if someone accidentally wrote their Device Tree such as to have > > > > 1 CPU nodes, yet the CPU is not MP capable and reading the MPIDR > > > register does return the expected value, but that is wrong anyway. > > > > An UP device will most likely not have a MPIDR. That said I'm not sure this > > is > > always true. As per ARM1176JZ's TRM[1], the RPi1 CPU, if one was to get the > > MPIDR it would raise an undefined exception. > > > > The way I see it's an acceptable outcome as the DT is clearly wrong. > > It is, although you probably want to use of_get_available_child_count() > instead of of_get_child_count() since we could imagine that a boot > loader or some other boot program mangling the DT could intentionally > put a 'status = "disabled"' property on the non-boot CPU node for > whatever reason. Good point, I'll fix it on v2.
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c index 39c978698406..a924fda9abc8 100644 --- a/arch/arm/kernel/devtree.c +++ b/arch/arm/kernel/devtree.c @@ -74,7 +74,7 @@ void __init arm_dt_init_cpu_maps(void) struct device_node *cpu, *cpus; int found_method = 0; u32 i, j, cpuidx = 1; - u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0; + u32 mpidr = 0; u32 tmp_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID }; bool bootcpu_valid = false; @@ -83,6 +83,9 @@ void __init arm_dt_init_cpu_maps(void) if (!cpus) return; + if (is_smp() || of_get_child_count(cpus) > 1) + mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; + for_each_of_cpu_node(cpu) { const __be32 *cell; int prop_bytes;
Currently, in arm_dt_init_cpu_maps(), the hwid of the boot CPU is read from MPIDR on SMP devices and set to 0 for non SMP. This value is then matched with the DT cpu nodes' reg property in order to find the boot CPU in DT. On MP devices build without SMP the cpu DT node contains the expected MPIDR yet the hwid is set to 0. With this the function fails to match the cpus and uses the default CPU logical map. Making it impossible to get the CPU's DT node further down the line. This causes issues with cpufreq-dt, as it triggers warnings when not finding a suitable DT node on CPU0. Change the way we choose whether to get MPIDR or not. Instead of depending on SMP check the number of CPUs defined in DT. Anything > 1 means MPIDR will be available. This was seen on a Raspberry Pi 2 build with bcm2835_defconfig. Reported-by: "kernelci.org bot" <bot@kernelci.org> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- arch/arm/kernel/devtree.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)