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

[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.

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
> >
>