Re: [PATCH v1 8/8] ARM: at91: Add xload support to skov-arm9cpu

[Sam Ravnborg <sam@ravnborg.org>]

Hi Ahmad,

On Mon, May 16, 2022 at 01:15:42PM +0200, Ahmad Fatoum wrote:
> Hello Sam,

Thanks for your feedback - very appreciated!

> 
> On 15.05.22 21:38, Sam Ravnborg wrote:
> > This updates skov-arm9cpu with xload support, and we can now
> > use barebox as a replacment for at91bootstrap.
> > 
> > Only boot via SD card is supported.
> > 
> > NOTE: Actual status
> > [x] dbgu support in pbl works (can print)
> > [x] Other init stuff ifdeffed out - from at91bootstrap
> > [ ] Check what the original code used for div/mul - there is some confusion
> > [ ] load barebox.bin and boots it. Right now mount fails
> 
> Is your SD-Card perhaps 2G or smaller? The AT91 PBL MCI functions
> assume high capacity (> 2G). It's a quite ugly thing, but
> finding out whether a card is High-Capacity or not happens
> during init phase and as we don't redo init in PBL...

>From the at91sam9263 datasheet:
"Boot ROM does not support high capacity SDCards."
 
Sounds like a very plausible explanation - and gives me something to go
after.

> High Capacity cards reference start block offset in sectors, while
> older cards use bytes. On i.MX, barebox just reads at offset 0
> and all is good, but on AT91, we need to do random access, so
> we need to decide whether to use sectors or bytes. Currently,
> the driver hardcodes the sector assumption. I found this to be
> the lesser evil to the alternative: having a full MMC stack in PBL. 
> 
> If that's indeed your issue, there's a heuristic possible:
> Try to mount for High-Capacity, if that fails, assume non-high
> capacity and try again. It's not 100%, but it's better than status quo.

I will try to find a nice way to tell that we shall go for a non-high
capacity in the first place.
And then I will dig into the sector versus byte thing afterwards.

> 
> > -static void __bare_init skov_arm9cpu_init(void *fdt)
> > +ENTRY_FUNCTION(start_skov_arm9cpu_xload_mmc, r0, r1, r2)
> >  {
> > -	struct at91sam926x_board_cfg cfg;
> > +	const struct sam92_pmc_config sam92_pmc_config = {
> > +		/* X-tal is 16.000 MHz so 16 / 4 * (31 + 1) = 200 */
> > +		.diva = 14,
> > +		.mula = 171,
> > +	};
> > +	sam9263_lowlevel_init(&sam92_pmc_config);
> 
> This is needlessly fragile. Compiler is within rights to never push
> this to stack and to regenerate a relocation entry here that points
> at .data, which has not yet been relocated.
> 
> > +	arm_setup_stack(AT91SAM9263_SRAM0_BASE + AT91SAM9263_SRAM0_SIZE);
> 
> Both sam9263_lowlevel_init and arm_cpu_lowlevel_init are global functions,
> so LR will be pushed to stack in-between, yet stack is only initialized here
> after.
> 
> Also ENTRY_FUNCTION is __naked on ARM32, so it's a bad idea to do more
> than the absolutely necessary stuff here as GCC can generate very unexpected
> code when it decides to spill to stack inside a naked function.
> 
> We have ENTRY_FUNCTION_WITHSTACK now that removes this footgun.
> Please use that instead.
> 
> >  
> > -	cfg.pio = IOMEM(AT91SAM9263_BASE_PIOD);
> > -	cfg.sdramc = IOMEM(AT91SAM9263_BASE_SDRAMC0);
> > -	cfg.ebi_pio_is_peripha = true;
> > -	cfg.matrix_csa = IOMEM(AT91SAM9263_BASE_MATRIX + AT91SAM9263_MATRIX_EBI0CSA);
> > +	relocate_to_current_adr();
> > +	setup_c();
> 
> My preference would be set up ENTRY_FUNCTION_WITHSTACK, so you don't have
> to write naked code. Call arm_cpu_lowlevel_init() first thing, so you have
> active I-Cache. Then relocate and setup_c and only then do the SoC-specific setup.

Thanks - again very useful feedback. I will go along these lines.
> 
> > +pblb-$(CONFIG_MACH_SKOV_ARM9CPU) += start_skov_arm9cpu_xload_mmc
> > +FILE_barebox-skov-arm9cpu-xload-mmc.img = start_skov_arm9cpu_xload_mmc.pblb
> > +MAX_PBL_MEMORY_SIZE_start_skov_arm9cpu = 0x12000
> > +image-$(CONFIG_MACH_SKOV_ARM9CPU) += barebox-skov-arm9cpu-xload-mmc.img
> > +
> >  pblb-$(CONFIG_MACH_SKOV_ARM9CPU) += start_skov_arm9cpu
> >  FILE_barebox-skov-arm9cpu.img = start_skov_arm9cpu.pblb
> >  MAX_PBL_MEMORY_SIZE_start_skov_arm9cpu = 0x12000
> 
> Unrelated to your patch, but you might know the answer: Why is there a max PBL memory size here?
> AFAIK, you use at91bootstrap to chainload barebox into DRAM, why do you need
> a PBL size limit then?
The limit is from the old days where we tried to squeeze barebox into
the SRAM, so it could be used as the only bootloader - dropping the need
for at91bootstrap.
The new approach where we do a PBL barebox and then a full barebox is
much easier when you have first understood the concept.

I will drop the size restriction in my next patch stack.

We see other at91sam boards with the same restrictions due to the same
history. I think we can safely assume there is no use for barebox as
at91bootstrap and we can drop the size restrictions.
But then I am not happy to edit old boards that I cannot tests.

I have an at91sam9263-ek board and will update the board support
when I have skov-arm9cpu done. Focus is on the skov board first, as I
have more HW to play with here.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox