[U-Boot] [PATCH 07/13] arm: zynq: Use separate device tree instead of embedded

[Michal Simek <monstr@monstr.eu>]

Hi Simon,

On 09/01/2015 01:12 AM, Simon Glass wrote:
> Hi Michal,
> 
> On 31 August 2015 at 08:07, Michal Simek <monstr@monstr.eu> wrote:
>> On 08/31/2015 03:54 PM, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On 31 August 2015 at 05:24, Michal Simek <monstr@monstr.eu> wrote:
>>>> On 08/29/2015 05:10 PM, Simon Glass wrote:
>>>>> Production boards should not use CONFIG_OF_EMBED. Fix this for the Zybo
>>>>> boards.
>>>>
>>>> Zynq boards?
>>>>
>>>> As you see I have enabled OF_EMBED some weeks ago.
>>>> zynq: Make CONFIG_OF_EMBED default case
>>>> 98b532b42079a7ffd617ce0330d6778288b7c535
>>>>
>>>> What's the reason not to use CONFIG_OF_EMBED for production boards?
>>>> Strictly speaking none of these boards are production one.
>>>> I would label them more as refence boards, development boards.
>>>
>>> Well I mean that we should not have boards in the tree that use this
>>> option, It has been a long-standing convention since device tree
>>> support was added - see README.fdt-control. I'll send a patch to add
>>> this note to Kconfig.
>>
>> ok then do we want to support this option? If this shouldn't be enabled
>> in any config in the tree? (Note: someone has to enable that and keep
>> building u-boot to ensure that this feature still work)
> 
> It's a bit like disabling relocation. It is useful when you want to
> load an elf image onto your board with a debugger, without having to
> worry about getting the device tree loaded also. It is normally
> possible to load u-boot-dtb.bin and then load the symbols from u-boot
> separately, but that's the reasoning.


The related problem is that in your commit message you are using correct
line for bif if you use FSBL. But the problem is with the all software
around it and users around.
u-boot-dtb.bin is binary file and you need to keep record where to load
it. If you have elf file you have entry point there.
It means that this change breaks all current users which is something
what is pretty problematic.
I can live without disabling OF_EMBED because I will use it in xilinx
tree but it is just pain which you should be aware of.

And still I can't see the problem to have OF_EMBED enabled for these
zynq boards because all of them are not recommended for production.

> 
>>
>> The reason why I have enabled that was that using u-boot-dtb.img is
>> breaking all users because they have to start to change a lot of things.
>> That's why having OF_EMBED enabled was less painful.
>> Users which use these boards just don't recognize any change when this
>> feature is enabled.
> 
> I see. Well perhaps you could enable the debug UART / do something
> else so that a message will be printed in this case for all boards?
> 
> We could perhaps move to a different scheme:
> 
> u-boot.bin - contains U-Boot and device tree(s)
> u-boot-nodtb.bin - contains plain binary without device tree
> 
> and perhaps this behaviour could even be optional (thus you could
> select it for Zynq). This has come up before. Tegra uses
> u-boot-nodtb-tegra.bin although it is for a different purpose. See
> commits 9972db5c and 984df4ec for the reasoning. I suspect that could
> perhaps be dropped but I am not sure.

The problem is more with using bin formats which lack information about
entry point.

You are using this line and it is completely valid.
[load = 0x04000000,startup=0x04000000]/path/to/u-boot.bin
but most of users are just using
u-boot.elf
and they don't need to care about addresses at all.

I am fully aware that this is around for a long time but it will be the
best to have something like u-boot-dtb.elf or u-boot.elf with DTB
inside. As we discussed above u-boot.elf with DTB inside is when
OF_EMBED is enabled.


>>
>> ...
>>
>>>>> diff --git a/include/configs/zynq-common.h b/include/configs/zynq-common.h
>>>>> index e7ab50a..aa4785f 100644
>>>>> --- a/include/configs/zynq-common.h
>>>>> +++ b/include/configs/zynq-common.h
>>>>> @@ -319,7 +319,11 @@
>>>>>  #define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION     1
>>>>>  #define CONFIG_SPL_LIBDISK_SUPPORT
>>>>>  #define CONFIG_SPL_FAT_SUPPORT
>>>>> -#define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME     "u-boot.img"
>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>> +# define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME     "u-boot-dtb.img"
>>>>> +#else
>>>>> +# define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME     "u-boot.img"
>>>>> +#endif
>>>>>  #endif
>>>>>
>>>>>  /* Disable dcache for SPL just for sure */
>>>>>
>>>>
>>>> this was removed by Masahiro long time ago.
>>>> kconfig: move CONFIG_OF_* to Kconfig
>>>> sha1: 783e6a72b8278d59854ced41a4696c9a14abbb0b
>>>
>>> What was moved?
>>
>> Using different names for u-boot image.
> 
> OK. So are you saying my patch needs to change in this area?

What I am saying is that we patched config to use just one name and
currently you have started to use two again.
OF_CONTROL will be enabled by default that's why only one name should be
enough.
Or even better just move this name to Kconfig and setup one default value.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150901/27565187/attachment.sig>