Re: [PATCH v2] ARM: dts: imx: Pass 'chosen' and 'memory' nodes

[Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 01/25/17 14:46, Rob Herring wrote:
> On Wed, Jan 25, 2017 at 3:13 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 01/19/17 08:14, Uwe Kleine-König wrote:
>>> On Thu, Jan 19, 2017 at 02:56:42PM +0000, Russell King - ARM Linux wrote:
>>>> On Thu, Jan 19, 2017 at 03:46:41PM +0100, Uwe Kleine-König wrote:
>>>>> On Thu, Jan 19, 2017 at 12:35:40PM -0200, Fabio Estevam wrote:
>>>>>> Hi Uwe,
>>>>>>
>>>>>> On Thu, Jan 19, 2017 at 12:13 PM, Uwe Kleine-König
>>>>>> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>>>>>>
>>>>>>> Would it be nice to add a comment about why this was added? Something to
>>>>>>> prevent a cleanup like "remove empty nodes and invalid memory
>>>>>>> configurations".
>>>>>>
>>>>>> Do you mean something like this?
>>>>>>
>>>>>> /* "chosen" and "memory" nodes are mandatory */
>>>>>> chosen {};
>>>>>> memory { device_type = "memory"; reg = <0 0>; };
>>>>>
>>>>> Not very helpful comment. Something like:
>>>>>
>>>>>     /*
>>>>>      * The decompressor relies on a pre-existing chosen node to be
>>>>>      * available to insert the command line and merge other ATAGS
>>>>>      * info.
>>>>>      */
>>>>>
>>>>> Is it difficult to fix the decompressor?
>>>>
>>>> ... and that comment would be wrong.  Yes, the decompressor relies on it,
>>>> as do some uboot versions.
>>>
>>> Good that we talked about this, otherwise I would have removed that once
>>> the decompressor is fixed. So the comment must be
>>>
>>>       /*
>>>        * The decompressor and also some versions of U-Boot rely on a
>>>        * pre-existing /chosen node to be available to insert the
>>>        * command line and merge other ATAGS info.
>>>        * Also for U-Boot there must be a pre-existing /memory node.
>>>        */
>>>
>>>>> I didn't understood the breakage regarding the memory node good enough
>>>>> to suggest a comment for that.
>>>>
>>>> A missing memory node appears to prevent some uboot versions supplying
>>>> any kind of memory layout to the kernel, which then causes the kernel to
>>>> crash very early during boot.
>>>>
>>>> Again, this is not using appended DTB - this is using a separately loaded
>>>> DTB in uboot.  uboot fails to update the dtb if these nodes are missing.
>>>>
>>>> Frankly, I think the original change (removing the skeleton.dtsi include)
>>>> was misguided and needs to be reverted - the change is imho built upon an
>>>> incorrect assumption that nothing in skeleton.dtsi is required.  That's
>>>> clearly false.
>>>
>>> I tend to agree as I assume it's not only U-Boot on i.MX but on all ARM
>>> platforms.
>>>
>>> Best regards
>>> Uwe
>>>
>>
>> Adding back the chosen nodes is a bandaid that papers over the actual bug
>> in the decompressor.
>>
>> A comment about Fabio's attempt to fix the decompressor noted a possible
>> issue with the method taken to fix the problem, but then the discussion
>> of fixing the decompressor was dropped and v3 of the patch to add chosen
>> into a bunch of .dtsi files was applied by Shawn.
>>
>> Can someone please help Fabio to create a correct patch to fix the
>> decompressor?
>>
>> Note that the ePAPR v1.1 does require at least one memory node in
> 
> We have a DT spec now, let's stop pointing people to ePAPR.
> 
>> section 3.1, so the portion of v3 of the patch that adds a memory
>> node is valid in terms of following the standard, not just a U-boot
>> requirement.
> 
> I'd have no issue making chosen be required. Any platform with a
> serial console should be setting stdout-path anyway. Just add it to
> the spec. :) We should also add a dtc check as well if we do that.
> 
> Rob
> 

Sounds good to me.

-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html