Re: [RFC PATCH 08/17] sunxi: introduce NCAT2 generation model

[Sam Edwards <cfsworks@gmail.com>]

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, 
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.

> 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);
+       }

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).

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 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?

Warm regards,
Sam

P.S. My target is the BMC on the Turing Pi 2 board. 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. :)