On Wed, 17 May 2023 01:43:12 +0100 Andre Przywara andre.przywara@arm.com wrote:
+Maksim, as he was interested in the U-Boot series as well and had some plans for SPI-NOR booting, IIUC.
Cheers, Andre
On Tue, 16 May 2023 17:53:38 -0600 Sam Edwards cfsworks@gmail.com wrote:
Hi Sam,
On 5/16/23 15:08, Andre Przywara wrote:
This whole memory map is somewhat of a legacy. Apart from a few addresses for the SPL needs we shouldn't have those defines at all. Some symbols are needed because there are other macros using them, although these then are eventually unused. I have some patches to remove most of the symbols, and patch 14/17 demonstrates some idea how to pin this down to what's really needed.
For this particular case: this was copied from the H6 memory map, some addresses are just plain wrong for the D1 family. I will try to remove them as much as possible, leaving only the ones needed in.
I see - the only "tangible" concern I had was the access to prcm->res_cal_ctrl done in arch/arm/mach-sunxi/clock_sun50i_h6.c:clock_init_safe
This doesn't appear to upset the silicon but also doesn't seem necessary either -- and with how tight of a memory footprint SPL has to fit into,
What's the particular concern here? Compared to the A64 we are pretty cool: it's Thumb2 code and we are at around 27KB, at least with my toolchain. And I haven't tried, but I am pretty sure the BROM loads more than 32K, as it does on the H6 and H616 already. The U-Boot build system and the code already supports this - we rely on this for the H616 - so we can lift the limit anytime, if really needed.
I wanted to check whether this was just something undocumented or dead code that needed to be removed. It sounds like it's mostly the latter.
I haven't checked if the vendor boot0 does this. I am pretty sure there is a PRCM block, it's just regularly not mentioned in the manuals.
So where did you see problems? If you would (wrongly) reference PortL somewhere in SPL GPIO code, it would use a wrong pointer, but at least the code would still compile fine, wouldn't it?
The specific patch I had to apply (to arch/arm/mach-sunxi/board.c) was: /* Update PIO power bias configuration by copy hardware detected value */ val = readl(SUNXI_PIO_BASE + SUN50I_H6_GPIO_POW_MOD_VAL); writel(val, SUNXI_PIO_BASE + SUN50I_H6_GPIO_POW_MOD_SEL);
val = readl(SUNXI_R_PIO_BASE + SUN50I_H6_GPIO_POW_MOD_VAL);writel(val, SUNXI_R_PIO_BASE + SUN50I_H6_GPIO_POW_MOD_SEL);
if (SUNXI_R_PIO_BASE) {val = readl(SUNXI_R_PIO_BASE + SUN50I_H6_GPIO_POW_MOD_VAL);writel(val, SUNXI_R_PIO_BASE + SUN50I_H6_GPIO_POW_MOD_SEL);}Ah, I see, I indeed missed that. We seem to define all symbols anyway, so we can even lose the #ifdef and use proper if's here. Will incorporate that in the next drop.
With SUNXI_R_PIO_BASE being 0, this was actually attempting to write to BROM. This might also be something that doesn't really upset the silicon, though: my debug environment is a concolic emulator I quickly hacked up to trace MMIO accesses, and it flagged the write to BROM as an error. It was easier to patch the SPL than to have the emulator ignore the error (and verify that the T113 was cool with it).
Ah yeah, the Allwinner interconnect is pretty relaxed about those things: accesses to addresses with no device behind them are usually ignored (RAZ/WI), where other platform might throw an external abort. Writes to ROM areas are ignored as well.
Since this kind of extraneous/erroneous init code tends to remain undetected when the symbols they need are dummied-out like this, I figured I'd give a nudge in the direction of instead *removing* the symbols where appropriate and fixing whatever breaks -- especially since we really need to be thrifty about SPL size. But that might also be something that happens in a later cleanup pass when the patchset is being prepared for upstream inclusion. :)
P.S. Could you try the github post? Then compiled and booted fine for me, and includes the DRAM code as well now: https://github.com/apritzel/u-boot/commits/t113s-mq-r-WIP
Ooh, more up-to-date code, thanks for the link! I'll switch to using this instead going forward. My pulls from that branch might be relatively infrequent
Don't worry, I won't push to this anymore.
since I'm also working on some patches for better Clang compatibility concurrent with the efforts here. Is this email thread a good venue for feedback against that branch or would you prefer that I use GitHub issues instead?
Please use this thread here, if you find something still wrong in the branch. I just pushed it to github since someone asked for a fixed and complete version, and I didn't have time to prepare a proper post again.
I will hopefully post a proper version for upstreaming in the next days.
Warm regards, Sam
P.S. My target is the BMC on the Turing Pi 2 board.
Ah, interesting, didn't know that this is now a BMC - for a SoC from Allwinner's arch nemesis Rockchip ;-)
They have the same SoC and (apparently) UART console configuration, but the differences end there: in particular, my target supports boot from either/both microSD+SPI-NAND. I might have to start pushing for room for SPI drivers in the SPL soon. :)
We already have SPI(-NOR) booting support, check arch/arm/mach-sunxi/spl_spi_sunxi.c. This code is very small, and just needs to be updated to cover the D1/T113 SPI controller, which is slightly different. See https://lore.kernel.org/linux-arm-kernel/20230507150345.1971083-1-bigunclema... for the Linux SPI bits. Regarding SPI-*NAND*: there is https://patchwork.ozlabs.org/user/todo/uboot/?series=322733 which is supposed to allow loading U-Boot proper from SPI-NAND. I haven't tested it yet, and wasn't overly happy with the refactoring, but would appreciate any kind of review or test.
Cheers, Andre