Re: NXP P50XX/e5500 secondary CPUs not onlined with current mainline (was [PATCH 20/21] of: use for_each_of_cpu_node iterator)

[Michael Ellerman]

Rob Herring <robh@xxxxxxxxxx> writes:
> On Wed, Oct 31, 2018 at 7:46 AM Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
>> Rob Herring <robh@xxxxxxxxxx> writes:
>> > Use the for_each_of_cpu_node iterator to iterate over cpu nodes. This
>> > has the side effect of defaulting to iterating using "cpu" node names in
>> > preference to the deprecated (for FDT) device_type == "cpu".
>> >
>> > Cc: Frank Rowand <frowand.list@xxxxxxxxx>
>> > Cc: devicetree@xxxxxxxxxxxxxxx
>> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
>> > ---
>> > Please ack and I will take via the DT tree. This is dependent on the
>> > first 2 patches.
>> >
>> >  drivers/of/base.c    |  2 +-
>> >  drivers/of/of_numa.c | 15 ++-------------
>> >  2 files changed, 3 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/drivers/of/base.c b/drivers/of/base.c
>> > index 6389aeb2f48c..8285c07cab44 100644
>> > --- a/drivers/of/base.c
>> > +++ b/drivers/of/base.c
>> > @@ -389,7 +389,7 @@ struct device_node *of_get_cpu_node(int cpu, unsigned int *thread)
>> >  {
>> >       struct device_node *cpun;
>> >
>> > -     for_each_node_by_type(cpun, "cpu") {
>> > +     for_each_of_cpu_node(cpun) {
>> >               if (arch_find_n_match_cpu_physical_id(cpun, cpu, thread))
>> >                       return cpun;
>> >       }
>>
>> Previously we just looked for any node with a type of "cpu", but now
>> we're using for_each_of_cpu_node(), which does:
>>
>>         for (; next; next = next->sibling) {
>>                 if (!(of_node_name_eq(next, "cpu") ||
>>                       (next->type && !of_node_cmp(next->type, "cpu"))))
>>                         continue;
>>
>>                 if (!__of_device_is_available(next))
>>                         continue;
>>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>>                 if (of_node_get(next))
>>                         break;
>>         }
>>
>>
>> ie. the available check is new.
>>
>> On this machine the 2nd CPU is not marked as available:
>>
>>   root@p5020ds:/proc/device-tree/cpus/PowerPC,e5500@1# lsprop status
>>   status           "disabled"
>>
>> This has the effect of preventing the SMP code from finding the 2nd CPU
>> in order to bring it up (in smp_85xx_start_cpu()). And so only the boot
>> CPU is onlined.
>>
>> The device tree is built from a dts:
>>
>>   arch/powerpc/boot/dts/fsl/p5020si-pre.dtsi
>>
>> But we don't set the status in there, so presumably u-boot is changing
>> the status during boot? (not a u-boot expert).
>
> Ah, status for cpus is a bit different. For most nodes, it should be
> equivalent to the node not being present, but for cpus it means
> offline if disabled. Though ARM platforms have never used it in that
> way.

Aha. We don't use it like that on server CPUs either, so perhaps it's
just a u-boot thing.

>> We could work around this in the platform code presumably, but I'm
>> worried this might break other things as well. You didn't mention the
>> addition of the available check in the change log so I wonder if it was
>> deliberate or just seemed like a good idea?
>
> Just seemed like a good idea...

Yeah fair enough.

> I'll send a patch now dropping those 2 lines.

Awesome, thanks.

cheers