| Message ID | E1a1XXS-00060O-GS@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 25, 2015 at 10:43:02AM +0000, Russell King wrote: > arm_dt_init_cpu_maps() initialises the CPU possible map even when the > boot CPU indicates that it is not SMP capable. This can happen with > SoCs where the SoC may have a single UP-only CPU, or may have a pair of > SMP CPUs - and this is the only difference. > > One solution is to increase the number of DT files by forcing peopl to > properly describe the hardware: this means all the iMX6DL DT files need > to be duplicated for iMX6S and a whole raft of updates to boot loaders > and the like. > > Another solution is to decide that it is inappropriate to initialise the > cpu_possible map with anything but the boot CPU on non-SMP capable > systems. This is implemented by this patch. > > The situation currently may provoke a warning on earlier kernels, > printed during SMP bringup where we attempt to bring CPU1 online inspite > of CPU0 being a UP-only CPU, and this only failing due to the lack of > SMP operations. I think that even if we work around this, we should have a warning regarding the erroneous DT. There are plenty of other things people may get wrong in their DT that we may or may not be able to detect and/or workaround. We should generally discourage relying on fixups and encourage people to do the right thing. > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > arch/arm/kernel/devtree.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c > index 65addcbf5b30..bd72ce91d7a2 100644 > --- a/arch/arm/kernel/devtree.c > +++ b/arch/arm/kernel/devtree.c > @@ -170,15 +170,18 @@ void __init arm_dt_init_cpu_maps(void) > return; > } > > - /* > - * Since the boot CPU node contains proper data, and all nodes have > - * a reg property, the DT CPU list can be considered valid and the > - * logical map created in smp_setup_processor_id() can be overridden > - */ > - for (i = 0; i < cpuidx; i++) { > - set_cpu_possible(i, true); > - cpu_logical_map(i) = tmp_map[i]; > - pr_debug("cpu logical map 0x%x\n", cpu_logical_map(i)); > + if (is_smp()) { Can we not change this to something like: if (!is_smp() && cpuidx >1) { pr_warn("Boot CPU is UP, but DT contains multiple CPUs. Skipping!\n"); } else { Other than that, this looks good to me. If you're happy with the above: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. > + /* > + * Since the boot CPU node contains proper data, and all > + * nodes have a reg property, the DT CPU list can be > + * considered valid and the logical map created in > + * smp_setup_processor_id() can be overridden > + */ > + for (i = 0; i < cpuidx; i++) { > + set_cpu_possible(i, true); > + cpu_logical_map(i) = tmp_map[i]; > + pr_debug("cpu logical map 0x%x\n", cpu_logical_map(i)); > + } > } > } > > -- > 2.1.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Wed, Nov 25, 2015 at 11:35:38AM +0000, Mark Rutland wrote: > On Wed, Nov 25, 2015 at 10:43:02AM +0000, Russell King wrote: > > arm_dt_init_cpu_maps() initialises the CPU possible map even when the > > boot CPU indicates that it is not SMP capable. This can happen with > > SoCs where the SoC may have a single UP-only CPU, or may have a pair of > > SMP CPUs - and this is the only difference. > > > > One solution is to increase the number of DT files by forcing peopl to > > properly describe the hardware: this means all the iMX6DL DT files need > > to be duplicated for iMX6S and a whole raft of updates to boot loaders > > and the like. > > > > Another solution is to decide that it is inappropriate to initialise the > > cpu_possible map with anything but the boot CPU on non-SMP capable > > systems. This is implemented by this patch. > > > > The situation currently may provoke a warning on earlier kernels, > > printed during SMP bringup where we attempt to bring CPU1 online inspite > > of CPU0 being a UP-only CPU, and this only failing due to the lack of > > SMP operations. > > I think that even if we work around this, we should have a warning > regarding the erroneous DT. > > There are plenty of other things people may get wrong in their DT that > we may or may not be able to detect and/or workaround. We should > generally discourage relying on fixups and encourage people to do the > right thing. Well, this is the problem with a static description of the hardware - it very quickly becomes unmanagable when you have lots of different combinations. For the SR platforms, there are already the base vs pro Hummingboard, Cubox-i, Hummingboard edge, Hummingboard Edge, Hummingboard Gate, each the choice of iMX6S, DL, D, Q or more uSOMs. To cover these "properly" we're going to need 24 DT files rather than 12 if we're going to require the CPUs entries to correctly describe the hardware. Given that CPUs are discoverable from the hardware, this is getting to be insane. Yes, there's cases where we need the CPUs described in hardware, but to require the description for all platforms even where CPUs are discoverable is IMHO insane. I think we need a better solution for this, rather than telling people their DT files need to fully describe the hardware, and at the same time, be correct.
Am Mittwoch, den 25.11.2015, 11:49 +0000 schrieb Russell King - ARM Linux: > On Wed, Nov 25, 2015 at 11:35:38AM +0000, Mark Rutland wrote: > > On Wed, Nov 25, 2015 at 10:43:02AM +0000, Russell King wrote: > > > arm_dt_init_cpu_maps() initialises the CPU possible map even when the > > > boot CPU indicates that it is not SMP capable. This can happen with > > > SoCs where the SoC may have a single UP-only CPU, or may have a pair of > > > SMP CPUs - and this is the only difference. > > > > > > One solution is to increase the number of DT files by forcing peopl to > > > properly describe the hardware: this means all the iMX6DL DT files need > > > to be duplicated for iMX6S and a whole raft of updates to boot loaders > > > and the like. > > > > > > Another solution is to decide that it is inappropriate to initialise the > > > cpu_possible map with anything but the boot CPU on non-SMP capable > > > systems. This is implemented by this patch. > > > > > > The situation currently may provoke a warning on earlier kernels, > > > printed during SMP bringup where we attempt to bring CPU1 online inspite > > > of CPU0 being a UP-only CPU, and this only failing due to the lack of > > > SMP operations. > > > > I think that even if we work around this, we should have a warning > > regarding the erroneous DT. > > > > There are plenty of other things people may get wrong in their DT that > > we may or may not be able to detect and/or workaround. We should > > generally discourage relying on fixups and encourage people to do the > > right thing. > > Well, this is the problem with a static description of the hardware - > it very quickly becomes unmanagable when you have lots of different > combinations. For the SR platforms, there are already the base vs > pro Hummingboard, Cubox-i, Hummingboard edge, Hummingboard Edge, > Hummingboard Gate, each the choice of iMX6S, DL, D, Q or more uSOMs. > To cover these "properly" we're going to need 24 DT files rather > than 12 if we're going to require the CPUs entries to correctly > describe the hardware. > > Given that CPUs are discoverable from the hardware, this is getting > to be insane. Yes, there's cases where we need the CPUs described > in hardware, but to require the description for all platforms even > where CPUs are discoverable is IMHO insane. > > I think we need a better solution for this, rather than telling people > their DT files need to fully describe the hardware, and at the same > time, be correct. > What we do in barebox is to look into the SCU to find out how many CPU cores are actually present and fix up the DT passed to the kernel accordingly by deleting any superfluous CPU nodes. This satisfies both the need to keep the required DT source files to a minimum, while also providing correct information to the kernel. Regards, Lucas
On Wed, Nov 25, 2015 at 01:50:31PM +0100, Lucas Stach wrote: > What we do in barebox is to look into the SCU to find out how many CPU > cores are actually present and fix up the DT passed to the kernel > accordingly by deleting any superfluous CPU nodes. This satisfies both > the need to keep the required DT source files to a minimum, while also > providing correct information to the kernel. ... which is a function of the boot loader, but not all boot loaders do this. So, if we're using u-boot, what option do we have? I've never been able to successfully build and boot uboot myself (it always fails one way or another with my toolchains - if I get it to link, it silently fails on the hardware, but if someone else builds it for me from exactly the same source, it works fine) so count me out of fixing uboot in this regard. Note that it's not that the kernel doesn't work, it's just that people are complaining that they see warnings from the kernel during their boot. So, unless we're going to start providing iMX6S and iMX6D specific DT blobs (which will double the number of .dtb files we end up with on iMX6), I think we're stuck with users complaining about this.
On Wed, Nov 25, 2015 at 1:56 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Nov 25, 2015 at 01:50:31PM +0100, Lucas Stach wrote: >> What we do in barebox is to look into the SCU to find out how many CPU >> cores are actually present and fix up the DT passed to the kernel >> accordingly by deleting any superfluous CPU nodes. This satisfies both >> the need to keep the required DT source files to a minimum, while also >> providing correct information to the kernel. > > ... which is a function of the boot loader, but not all boot loaders > do this. So, if we're using u-boot, what option do we have? > > I've never been able to successfully build and boot uboot myself (it > always fails one way or another with my toolchains - if I get it to > link, it silently fails on the hardware, but if someone else builds > it for me from exactly the same source, it works fine) so count me > out of fixing uboot in this regard. > > Note that it's not that the kernel doesn't work, it's just that people > are complaining that they see warnings from the kernel during their > boot. So, unless we're going to start providing iMX6S and iMX6D > specific DT blobs (which will double the number of .dtb files we end > up with on iMX6), I think we're stuck with users complaining about > this. I have been looking into doing some of this cleanup in u-boot recently. More and more this seems like a hack because the device-tree descriptions belong to the kernel. If the bootloader isn't generating them it seems counter intuitive that it should be responsible for altering them so the kernel doesn't complain about incorrect hardware configuration. It seems as though device-tree could benefit from some logic definitions that could load device-tree snippets or overlays depending on the hardware that is probed. Or in the case of SR hardware we just generate 24 device tree files and push this probing back into each bootloader implementation to detect and load accordingly. -Jon
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c index 65addcbf5b30..bd72ce91d7a2 100644 --- a/arch/arm/kernel/devtree.c +++ b/arch/arm/kernel/devtree.c @@ -170,15 +170,18 @@ void __init arm_dt_init_cpu_maps(void) return; } - /* - * Since the boot CPU node contains proper data, and all nodes have - * a reg property, the DT CPU list can be considered valid and the - * logical map created in smp_setup_processor_id() can be overridden - */ - for (i = 0; i < cpuidx; i++) { - set_cpu_possible(i, true); - cpu_logical_map(i) = tmp_map[i]; - pr_debug("cpu logical map 0x%x\n", cpu_logical_map(i)); + if (is_smp()) { + /* + * Since the boot CPU node contains proper data, and all + * nodes have a reg property, the DT CPU list can be + * considered valid and the logical map created in + * smp_setup_processor_id() can be overridden + */ + for (i = 0; i < cpuidx; i++) { + set_cpu_possible(i, true); + cpu_logical_map(i) = tmp_map[i]; + pr_debug("cpu logical map 0x%x\n", cpu_logical_map(i)); + } } }
arm_dt_init_cpu_maps() initialises the CPU possible map even when the boot CPU indicates that it is not SMP capable. This can happen with SoCs where the SoC may have a single UP-only CPU, or may have a pair of SMP CPUs - and this is the only difference. One solution is to increase the number of DT files by forcing peopl to properly describe the hardware: this means all the iMX6DL DT files need to be duplicated for iMX6S and a whole raft of updates to boot loaders and the like. Another solution is to decide that it is inappropriate to initialise the cpu_possible map with anything but the boot CPU on non-SMP capable systems. This is implemented by this patch. The situation currently may provoke a warning on earlier kernels, printed during SMP bringup where we attempt to bring CPU1 online inspite of CPU0 being a UP-only CPU, and this only failing due to the lack of SMP operations. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/kernel/devtree.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)