[U-Boot] [PATCH v2 2/3] spl: add relocation support【请注意，邮件由sjg at google.com代发】

Sun Jul 7 00:36:39 UTC 2019

Hi Simon：

Simon Glass <sjg at chromium.org> 于2019年7月7日周日 上午1:16写道：

> Hi Andy,
>
> On Mon, 24 Jun 2019 at 04:37, Andy Yan <andy.yan at rock-chips.com> wrote:
> >
> > Hi Simon:
> >
> > Glad to see you online again.
> >
> > On 2019/6/23 上午3:10, Simon Glass wrote:
> > > Hi Andy,
> > >
> > > On Tue, 28 May 2019 at 09:34, Andy Yan <andy.yan at rock-chips.com>
> wrote:
> > >> Hi Simon:
> > >>
> > >> On 2019/5/23 上午3:39, Simon Glass wrote:
> > >>
> > >> Hi Andy,
> > >>
> > >> On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk at gmail.com> wrote:
> > >>
> > >> Hi Simon:
> > >>
> > >> Simon Glass <sjg at chromium.org> 于2019年5月22日周三 上午8:28写道：
> > >>
> > >> Hi Andy,
> > >>
> > >> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan at rock-chips.com>
> wrote:
> > >>
> > >> Hi Simon:
> > >>
> > >> On 2019/5/20 下午11:35, Simon Glass wrote:
> > >>
> > >> Hi Andy,
> > >>
> > >> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan at rock-chips.com>
> wrote:
> > >>
> > >> Hi Simon:
> > >>
> > >> On 2019/5/19 上午12:26, Simon Glass wrote:
> > >>
> > >> Hi Andy,
> > >>
> > >> Instead of this could you:
> > >>
> > >> - move ATF?
> > >>
> > >> All rockchip based arm64 ATF run from the start 64KB of dram as this
> > >> will give convenient for kernel manage the memory.
> > >>
> > >> On the other hand, change the ATF load address will break the
> > >> compatibility of the exiting firmware.
> > >>
> > >> - change the SPL load address so it is not in the way (since TPL can
> > >> load to any address)
> > >>
> > >> The SPL is loaded by bootrom after TPL back to bootrom, so the load
> > >> address if fixed by bootrom code.
> > >>
> > >> I think you are creating a nightmare here. If you have to do things
> > >> like this for older and smaller SoCs, OK. But it should not be used
> > >> for newer ones that can do things properly.
> > >>
> > >> Most rockchip based SOC sram is small,  even in the future soc
> roadmap,
> > >> this situation  will still exist, larger sram means more cost.
> > >>
> > >> I believe the RK3399 has 192KB. What is the minimum size in new chips?
> > >>
> > >> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328
> without storage drive is 28KB.
> > >> The available sram size for TPL on RK3326 is 10KB,  our another A35
> based IOT SOC has the same limitation.
> > >>
> > >> OK, I see.
> > >>
> > >>
> > >> As for the current spl for rockchip soc in mainline, we use a
> workaround
> > >> by reserve large space at the head of spl(see
> > >> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl
> binary.
> > >>
> > >> Yes.
> > >>
> > >> As for my patch, the spl relocation is disabled default, we only
> enable
> > >> it on necessary platform, so it won't hurt others .
> > >>
> > >> Well it adds more code and complexity. Perhaps it makes sense to add
> > >> this, but I want to understand the need.
> > >>
> > >> The bootrom has so many limitations that it just creates pain.
> > >>
> > >> I know we can build mmc or other storage driver into TPL so we can use
> > >> tpl load spl on some platform that sram is big enough, but there are
> > >> also many rockchip soc has very small sram, so we tend to only do dram
> > >> initialization in tpl, and let bootrom load next stage .
> > >>
> > >> See above
> > >>
> > >> For the consideration of software development, we also want to keep
> TPL
> > >> clean,  only do dram initialization(as it current status), this make
> our
> > >> internal dram team work more simple, they don't need to care about
> other
> > >> modules like mmc.
> > >>
> > >> Yes I understand this, but the boot ROM should be provided as a
> > >> library to call into:
> > >>
> > >> int mmc_read(void *addr, int start_block, int end_block)
> > >> int spi_read(void *addr, int start_block, int end_block)
> > >>
> > >> Then SPL or TPL can use it without all the strange limitations we
> have now.
> > >>
> > >> Since you probably already have these functions somewhere in the boot
> > >> ROM, you could implement this using a function table somewhere in the
> > >> ROM with a magic number before it, so that SPL can find it.
> > >>
> > >> The Bootrom do much more work than directly load the spl binary. It
> will do somthing like checksum, look for the backup when the current image
> is invalid, also including security check when secure boot is enabled. This
> is why we did much work to add back_too_bootrom   mechanism in mainline in
> 2017.
> > >>
> > >> Yes I understand that, but it is also quite inflexible, and creates
> > >> enormous problems with bootloaders.
> > >>
> > >> I am not suggesting that you remove functionality. I am suggesting
> > >> that you allow bootloaders to call into some of it, to reduce the
> > >> problems caused by the inflexible bootrom.
> > >>
> > >>
> > >> I checked with people who write bootrom code these days,  as
> different chip written by different people from different team,  it took a
> bit long time to figure out this.
> > >>
> > >> Yes , bootrom have storage access api like
> mmc_read/read_sfc_nand/read_sfc_nor/nandc_read, but bootrom does  not
> provide a fixed table for these api, and the address for these api are
> different on different chip, this means we have to list the api address
> chip by chip in SPL code. There is another thing, as the bootrom code are
> written by different person, the api interface don't keep constant:
> sfc_nor_read on one chip is sfc_nor_read(void *addr, int start, int len),
> but on another chip is sfc_nor_read(int start, void *addr, int len), this
> make things complicated.
> > >>
> > >> Also as what I mentioned  before, the Bootrom do much more work than
> directly load the spl binary, especially in secure boot mode, but the
> bootrom don't want to export secure related api for security concern .
> > >>
> > >> So this seems not a good choice to reuse bootrom api in spl.
> > > While I understand what you are saying, I don't think it would be hard
> > > to add a little interface layer for each SoC which supports reading
> > > from each type of device, and knows the SoC ROM address to call, and
> > > deals with changing args, etc.
> > >
> > > Really, it should be a very small amount of code. And the benefit is a
> > > large reduction in TPL/SPL code size and a lot less of the insane
> > > hacks we ahve now!
> > >
> >
> > Yes, it's not hard to add a wrapper for every soc from technical side,
> > but it make things complicated. When we get a new soc, we need to check
> > with the bootrom person for the storage api type by type, the we get a
> > long header file nested with ''if chipA, else if chipB, esle if chipC",
> > this looks not nice .
>
> I suggest having one header per SoC.
>
> Also, if you do talk to your chip people, ask them to use a consistent
> API from now on, and put the table of function pointers (and perhaps
> an API version) in the ROM itself.
>
> >
> > What's more , there is one more issue,  bootrom  will not export the
> > secure related api, so we can't  do secure boot with this method.
>
> Why not? Making an API hidden does not help security.
>
> I honestly worry that no one at Rockchip is thinking about how to
> actually implement a bootloader that covers all the chips that you
> ship. Consistency takes effort.
>
> >
> > I still prefer to reuse the existing relocation mechanism.
>
> To keep compatibility with the old closed-source binary?
>
> I feel that you should show some willingness to make things better in
> the future. No other SoC needs relocation in SPL as far as I know.
>

Actually： There are ！
（1）Just do a grep about relocate_code function, you will see powerpc handle
spl relocate themselves.
  (2)   function spl_relocate_stack_gd written by yourself.

What has happened with Rockchip SoCs to cause this?
>

> If you must add relocation please use CONFIG_SPL_RELOCATE since not
> relocating should be the default. And try to put the code hidden away
> in spl_reloc.c or something.
>

I have already reuse the existing  CONFIG_SPL_SKIP_RELOCATE config in my
patch.

> Regards,
> Simon
>