From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 14 Sep 2015 13:42:21 +0200
Subject: [U-Boot] [RFC PATCH 2/2] sunxi: add "fel" boot target
In-Reply-To: <20150914133353.1e1658c4@i7>
References: <1441289520-22749-1-git-send-email-bernhard.nortmann@web.de>
 <1441289520-22749-3-git-send-email-bernhard.nortmann@web.de>
 <55F1CD9E.6080303@redhat.com> <55F29F86.5050901@web.de>
 <20150914133353.1e1658c4@i7>
Message-ID: <55F6B29D.7000309@redhat.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi,

On 14-09-15 12:33, Siarhei Siamashka wrote:
> On Fri, 11 Sep 2015 11:31:50 +0200
> Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
>
>> Hi!
>>
>> Am 10.09.2015 um 20:36 schrieb Hans de Goede:
>>> Hi,
>>>
>>> I would prefer to have this like this:
>>>
>>>      "bootcmd_fel=" \
>>>          "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \
>>>              "echo '(FEL boot)';" \
>>>              "source ${fel_data_addr}; " \
>>>          "fi\0"
>>>
>>
>> Sure, we could do that. I wanted to make clear that ${fel_booted} is
>> independent of a script being present (and thus ${fel_data_addr} set).
>> If the user feels inclined to do so, he might e.g. tweak bootcmd_fel
>> to override some defaults even with no boot.scr involved.
>>
>>> Also if we are not using fel_data_size, then why do we even
>>> have it ?
>>>
>>
>> I thought it unnecessary to restrict ourselves to not being able to
>> pass the size information, and kept it optional deliberately.
>>
>> Admittedly it's pointless in the "standard" case of boot.scr, as that
>> is expected to be an image with a well-defined header (including data
>> size). I could imagine other uses, e.g. a customized fel utility
>> passing uEnv.txt-style data, and integrating that via bootcmd_fel
>> "import -t ${fel_data_addr} ${fel_data_size}".
>
> We could probably have this data represented in the following way
> in the SPL header:
>
> 	union {
> 		struct {
> 			uint32_t fel_boot_script_address;
> 			uint32_t must_be_zero;
> 		};
> 		struct {
> 			uint32_t fel_uenv_txt_address;
> 			uint32_t fel_uenv_txt_size;
> 		};
> 	};
>
> So that if 'fel_uenv_txt_size' is non-zero, then we can do
> "import -t ${fel_uenv_txt_address} ${fel_uenv_txt_size}".
> And if it is zero, then treat this pointer as boot.scr data.
>
> And we don't even need to use the union if we don't want to. This all
> data could be also stored in a straightforward way (at the expense
> of using additional 4 bytes in the SPL header):
>
> 	uint32_t fel_boot_script_address;
> 	uint32_t fel_uenv_txt_address;
> 	uint32_t fel_uenv_txt_size;
>
> And because it takes more storage space, we would need to increase
> the SPL header size. But this can be easily done.
>
>> Personally I like to do this when testing; I find it easier to
>> simply edit a text file (without having to go through a mkimage
>> .scr on each cycle).
>
> Supporting both boot.scr and uEnv.txt for FEL boot seems to be
> reasonably simple to me. You can even do it in a single patch
> series. As Hans suggests, please take care of the boot.scr case
> first. Then maybe introduce uEnv.txt support with an additional
> patch.

I'm still not convinced that support uEnv.txt is necessary at all,
but I agree that if we this having this done through extra fields,
rather then through a union would be better.

Regards,

Hans