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

Tue Aug 13 09:35:09 UTC 2019

Hi Andy,

On Sat, 6 Jul 2019 at 18:36, Andy Yan <andyshrk at gmail.com> wrote:
>
> 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.

Right, but the default should be to not relocate, so please use the name above.

Regards,
Simon