[U-Boot] [PATCH 66/93] arm: Remove ot1200 board

Wed Dec 5 14:16:56 UTC 2018

Am 05.12.2018 um 14:54 schrieb Simon Glass:
> Hi Simon,
> 
> On Wed, 5 Dec 2018 at 06:38, Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com> wrote:
>>
>> On Wed, Dec 5, 2018 at 2:21 PM Simon Glass <sjg at chromium.org> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Sun, 25 Nov 2018 at 23:05, Simon Goldschmidt
>>> <simon.k.r.goldschmidt at gmail.com> wrote:
>>>>
>>>> [I've cut down the CC list a bit due to some gmail warnings]
>>>> On Mon, Nov 26, 2018 at 4:00 AM Simon Glass <sjg at chromium.org> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On Sun, 25 Nov 2018 at 14:09, Simon Goldschmidt
>>>>> <simon.k.r.goldschmidt at gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Nov 22, 2018 at 9:50 PM Simon Glass <sjg at chromium.org> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Thu, 22 Nov 2018 at 10:02, Tom Rini <trini at konsulko.com> wrote:
>>>>>>>>
>>>>>>>> On Thu, Nov 22, 2018 at 03:44:28PM +0100, Simon Goldschmidt wrote:
>>>>>>>>> Am Do., 22. Nov. 2018, 14:44 hat Tom Rini <trini at konsulko.com> geschrieben:
>>>>>>>>>
>>>>>>>>>> On Thu, Nov 22, 2018 at 02:24:49PM +0100, Marek Vasut wrote:
>>>>>>>>>>> On 11/22/2018 01:52 PM, Tom Rini wrote:
>>>>>>>>>>>> On Thu, Nov 22, 2018 at 10:25:14AM +0100, Christian Gmeiner wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Am Mo., 19. Nov. 2018 um 16:56 Uhr schrieb Simon Glass <
>>>>>>>>>> sjg at chromium.org>:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This board has not been converted to CONFIG_DM_BLK by the deadline.
>>>>>>>>>>>>>> Remove it.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> As the board is still mainted I will NAK it for the moment. Are there
>>>>>>>>>>>>> any hints want needs to be done
>>>>>>>>>>>>> to port thie board over to new DM stuff?
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, as a start you need to switch over to using CONFIG_OF_CONTROL and
>>>>>>>>>>>> selecting/providing a dtb file.  I see ot1200 is using DWC_AHSATA which
>>>>>>>>>>>> needs more work, but this is the board-level work that needs doing.
>>>>>>>>>>>
>>>>>>>>>>> Wasn't there a possibility to use platform data in board file instead of
>>>>>>>>>>> DT ? Or is DT mandatory now , including the libfdt overhead ?
>>>>>>>>>>
>>>>>>>>>> In short, DT for U-Boot and platform data for SPL is what's recommended,
>>>>>>>>>> yes.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is a little confusing for me. Socfpga gen5 SPL doesn't do that. And it
>>>>>>>>> seems a little strange or outdated overall.
>>>>>>>>>
>>>>>>>>> Would there be some kind of reference architecture or mach to look at
>>>>>>>>> what's the suggested/up-to-date way to implement SPL? Also regarding code
>>>>>>>>> flow?
>>>>>>>>
>>>>>>>> So, SPL is where things get, ahem, fuzzy.  While I don't want to
>>>>>>>> encourage boundless growth in U-Boot proper, we aren't exactly size
>>>>>>>> constrained (but rather, functional/logical constrained).  But in SPL,
>>>>>>>> yes, we have many platforms with 32/64/128 kilobyte hard limits (and
>>>>>>>> some smaller) and we can't always shove in a "TPL" before SPL either.
>>>>>>>> So in SPL we do make use of platform data instead.  While not the
>>>>>>>> smallest size constraint, am335x_hs_evm is a reasonable thing to look at
>>>>>>>> in this case.
>>>>>>>
>>>>>>> Also 'rock' uses CONFIG_OF_PLATDATA which provides a halfway house -
>>>>>>> still uses DT, but it gets converted into C structs so saves code
>>>>>>> space.
>>>>>>>
>>>>>>> firefly-rk3288 is a pretty good DM/DT example, including SPL.
>>>>>>
>>>>>> I've currently got an issue on socfpga gen5 that could be solved best
>>>>>> by enabling CONFIG_OF_EMBED (mixing const and non-const sections is a
>>>>>> problem for CRC calculation). However, it could probably also solve by
>>>>>> using platform data (but that doesn't work out of the box, yet). The
>>>>>> problem with CONFIG_OF_EMBED is that I think it's OK to enable this
>>>>>> for SPL but I don't like enabling it for U-Boot, so:
>>>>>>
>>>>>> Would it make sense to duplicate the whole "Provider of DTB for OF
>>>>>> control" choice so that it can be OF_EMBED for SPL but different for
>>>>>> U-Boot? Or does it make more sense to convert socfpga gen5 to use
>>>>>> OF_PLATDATA?
>>>>>
>>>>> We should not be using OF_EMBED in in-tree boards or production code.
>>>>
>>>> What's the reason for this? I can understand this for U-Boot, and I
>>>> can understand that it's at least theoretically a bit cleaner for SPL,
>>>> too. But there are some drawbacks when doing this in SPL where code is
>>>> not relocated:
>>>> - you lose the ability to check total size in linker file (which is
>>>> bad for size-constrained platforms: sometimes you notice failure only
>>>> when booting)
>>>
>>> You can add an SPL size check in Makefile.spl if you like.
>>
>> That might be required, yes.
>>
>>>> - you get an inconsistent memory layout regarding read/write: the
>>>> linker places bss at the end but then, DTB follows as const data
>>>
>>> This should be handled by the $(SPL-BIN)-pad.bin file (or by binman if
>>> you are using that).
>>
>> I don't understand that. How does the padding help? I have these
>> sections (roughly):
>> - text: readonly
>> - bss: writable
>> - DTB: readonly, added as post build step after linking
>>
>> How does $(SPL-BIN)-pad.bin help?
> 
> It covers over the BSS section so that the image ends where the DTB
> starts, thus fixing the addressing issue you mentioned. It allows you
> to do this:
> 
> cat u-boot-spl-nodtb.bin u-boot-spl.dtn >-u-boot-spl.bin
> 
>>
>>>> - binary size "on disk" grows due to this inconsistent memory layout
>>>> (since the flat binary includes the DTB, it needs to include the
>>>> zeroed-out bss, too)
>>>
>>> Right, but this is a few bytes. Why does it matter?
>>>
>>>> - "spl/u-boot-spl.hex" created by the default Makefiles does not seem
>>>> to include the DTB
>>>
>>> That might just be a bug.
>>
>> It might, yes. The hex file is currently built from the elf file, so
>> there's no DTB in there.
> 
> OK. Could be worth a patch.
> 
>>
>>>>
>>>>> Can you please explain the issue a bit more?
>>>>
>>>> Of course: socfpga gen5 has a feature where the boot rom can jump to
>>>> SPL in SRAM on warm boot. To ensure SPL is still valid after a reboot,
>>>> the boot rom can check its consistency by calculating a CRC over one
>>>> specified range in SRAM. On first boot, SPL stores its start, length
>>>> and CRC value to special registers for the boot rom. Since the
>>>> contents of bss changes while SPL is running, bss cannot be included
>>>> in this CRC range. (Same goes for the '.data' region, but it's
>>>> possible to build SPL without actually using it.)
>>>
>>> How about calculating that checksum at build time instead? You could
>>> use binman to do that.
>>>
>>>>
>>>> So to ensure the DTB is untouched, I have to make sure it has a lower
>>>> address than the bss section. Using OF_EMBED does this for me. And I
>>>> expect using platform data would work too. Do you have another idea
>>>> how to achieve my goal of combining all write-only sections in SPL
>>>> into one block?
>>>
>>> Yes, do it at build time. Or calculate your CRC before you write any
>>> BSS variables.
>>
>> Creating the correct checksum is not the point. I can do that before using bss.
>>
>> The problem is that on the next boot, this checksum is not valid any
>> more because bss might have changed.
> 
> Right, but just skip the BSS section when checksumming - i.e. checksum
> the code and then the DT but omit the BSS.

Yeah, well, good idea but that's not possible. The boot rom checks this 
checksum on warm restart and it can only check the CRC of one block. And 
of course I canno change the boot rom.

So the only option I have without using OF_EMBED is to drop the CRC of 
the DT, which is not really an option...

Regards,
Simon

>>>> Oh, and I currently count 109 defconfig files containing "OF_EMBED",
>>>> so I wasn't aware that this should not be used. Maybe these platforms
>>>> have similar reasons like I have and would enable OF_EMBED only for
>>>> SPL if they could. At least for socfpga_stratix10 that should work.
>>>
>>> That is very bad news. I'll see about adding a Makefile warning.
>>
>> OK. Looking forward to the discussion that starts then :-)
> 
> Yes...
> 
> Regards,
> Simon
> 