Hi Andy, On Mon, 24 Jun 2019 at 04:37, Andy Yan <andy....@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....@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 <andys...@gmail.com> wrote: > >> > >> Hi Simon: > >> > >> Simon Glass <s...@chromium.org> 于2019年5月22日周三 上午8:28写道： > >> > >> Hi Andy, > >> > >> On Tue, 21 May 2019 at 00:51, Andy Yan <andy....@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....@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. 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. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
