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

Wed Dec 5 16:11:05 UTC 2018

Am 05.12.2018 um 16:55 schrieb Simon Glass:
> Hi Simon,
> 
> On Wed, 5 Dec 2018 at 07:17, Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com> wrote:
>>
>> 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...
> 
> Really it sounds like you want to move the BSS after the DT. We don't
> have a way of doing that at present.
> 
> But I suppose it would be easy enough to arrange, by putting a
> placeholder for the DT in the link script. We just need a variable
> which holds the DT size and then can use '. = . + DT_SIZE' to reserve
> the space.

That's a good idea. I might try that as soon as I find the time to 
continue working on that boot-crc issue.

Thanks,
Simon

> 
> Regards,
> Simon
> 
>>
>> 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
>>>
>>