Hi Andy, On Sat, 6 Jul 2019 at 18:36, Andy Yan <[email protected]> wrote: > > Hi Simon： > > Simon Glass <[email protected]> 于2019年7月7日周日 上午1:16写道： >> >> Hi Andy, >> >> On Mon, 24 Jun 2019 at 04:37, Andy Yan <[email protected]> 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 <[email protected]> wrote: >> > >> Hi Simon: >> > >> >> > >> On 2019/5/23 上午3:39, Simon Glass wrote: >> > >> >> > >> Hi Andy, >> > >> >> > >> On Tue, 21 May 2019 at 19:56, Andy Yan <[email protected]> wrote: >> > >> >> > >> Hi Simon: >> > >> >> > >> Simon Glass <[email protected]> 于2019年5月22日周三 上午8:28写道： >> > >> >> > >> Hi Andy, >> > >> >> > >> On Tue, 21 May 2019 at 00:51, Andy Yan <[email protected]> wrote: >> > >> >> > >> Hi Simon: >> > >> >> > >> On 2019/5/20 下午11:35, Simon Glass wrote: >> > >> >> > >> Hi Andy, >> > >> >> > >> On Mon, 20 May 2019 at 00:34, Andy Yan <[email protected]> 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
