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

[Andre Przywara <andre.przywara@arm.com>]

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-bigunclemax@gmail.com/
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