Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: (EXT) Re: [PATCH] of: skip disabled CPU nodes
To:     Matthias Schiffer <matthias.schiffer@ew.tq-group.com>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200826120254.8902-1-matthias.schiffer@ew.tq-group.com>
 <4dd06b79-1402-d7cf-9676-1f9a9526da12@gmail.com>
 <9eb72c6561333661599411e49072928385629999.camel@ew.tq-group.com>
From:   Frank Rowand <frowand.list@gmail.com>
Message-ID: <ac64852a-7f2a-6005-f914-268670cd4f95@gmail.com>
Date:   Wed, 26 Aug 2020 09:47:40 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <9eb72c6561333661599411e49072928385629999.camel@ew.tq-group.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Rob,

On 2020-08-26 08:54, Matthias Schiffer wrote:
> On Wed, 2020-08-26 at 08:01 -0500, Frank Rowand wrote:
>> On 2020-08-26 07:02, Matthias Schiffer wrote:
>>> Allow disabling CPU nodes using status = "disabled".
>>>
>>> This allows a bootloader to change the number of available CPUs
>>> (for
>>> example when a common DTS is used for SoC variants with different
>>> numbers
>>> of cores) without deleting the nodes altogether (which may require
>>> additional fixups where the CPU nodes are referenced, e.g. a
>>> cooling
>>> map).
>>>
>>> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
>>>>
>>> ---
>>>  drivers/of/base.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index ea44fea99813..d547e9deced1 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct
>>> device_node *prev)
>>>  		of_node_put(node);
>>>  	}
>>>  	for (; next; next = next->sibling) {
>>> +		if (!__of_device_is_available(next))
>>> +			continue;
>>>  		if (!(of_node_name_eq(next, "cpu") ||
>>>  		      __of_node_is_type(next, "cpu")))
>>>  			continue;
>>>
>>
>> The original implementation of of_get_next_cpu_node() had
>> that check, but status disabled for cpu nodes has different
>> semantics than other nodes, and the check broke some systems.
>> The check was removed by c961cb3be906 "of: Fix cpu node
>> iterator to not ignore disabled cpu nodes".
>>
>> It would be useful to document that difference in the
>> header comment of of_get_next_cpu_node().
>>
>> -Frank
> 
> Hmm, I see. This difference in behaviour is quite unfortunate, as I'm
> currently looking for a way to *really* disable a CPU core.
> 
> In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other variants of the
> i.MX8M), there are 4 CPU nodes for the full-featured quad-core version.
> The reduced single- and dual-core versions are currently handled in
> NXP's U-Boot fork by deleting the additional nodes.
> 
> Not doing so causes the kernel to hang for a while when trying to
> online the non-existent cores during boot (at least in linux-imx 5.4 -
> I have not checked a more recent mainline kernel yet), but the deletion
> is non-trivial to do without leaving dangling phandle references.

Any thoughts on implementing another universal property that means
something like "the hardware described by this node does not exist
or is so broken that you better not use it".

Matthias, if Rob thinks that is a good idea, then you should start
with a new proposal that is also sent to
devicetree-spec@vger.kernel.org <devicetree-spec@vger.kernel.org>

-Frank

> 
> Kind regards,
> Matthias
>