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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Sam,

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

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.

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

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

Cheers,
Ahmad



-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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