Re: [PATCH v3 5/6] ARM: socfpga: add support for Enclustra AA1

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

Hi!

Sascha Hauer <sha@pengutronix.de> writes:

(...)
>> +++ b/arch/arm/boards/enclustra-aa1/board.c
(...)
>> +	pbl_index = readl(0xFFD06210);
>
> This is ARRIA10_SYSMGR_ROM_INITSWLASTLD, right? Please use it.
>

Sure.

>> +
>> +	pr_debug("Current barebox instance %d\n", pbl_index);
>> +
>> +	switch (pbl_index) {
>> +	case 0:
>> +		flag_barebox1 |= BBU_HANDLER_FLAG_DEFAULT;
>> +		break;
>> +	case 1:
>> +		flag_barebox2 |= BBU_HANDLER_FLAG_DEFAULT;
>> +		break;
>> +	};
>> +
>> +	bbu_register_std_file_update("emmc-barebox1-xload", flag_barebox1,
>> +					"/dev/mmc0.barebox1-xload",
>> +					filetype_socfpga_xload);
>> +
>> +	bbu_register_std_file_update("emmc-barebox1", 0,
>> +					"/dev/mmc0.barebox1",
>> +					filetype_arm_barebox);
>> +
>> +	bbu_register_std_file_update("emmc-barebox2-xload", flag_barebox2,
>> +					"/dev/mmc0.barebox2-xload",
>> +					filetype_socfpga_xload);
>> +
>> +	bbu_register_std_file_update("emmc-barebox2", 0,
>> +					"/dev/mmc0.barebox2",
>> +					filetype_arm_barebox);
>
> Should this be turned into something more intelligent like failsafe
> update?
>

You mean like you did for rockchip? Might be a good idea.

>> +	return 0;
>> +}
>> +postcore_initcall(aa1_init);
>
> You could turn this into a postcore_platform_driver.
>
>> +#define BAREBOX_PART 0
>> +#define BITSTREAM_PART 1
>> +#define BAREBOX1_OFFSET    SZ_1M
>> +#define BAREBOX2_OFFSET    BAREBOX1_OFFSET + SZ_512K
>> +#define BAREBOX3_OFFSET    BAREBOX2_OFFSET + SZ_512K
>> +#define BAREBOX4_OFFSET    BAREBOX3_OFFSET + SZ_512K
>> +#define BITSTREAM1_OFFSET  0x0
>
> From looking into the device tree I would expect BITSTREAM1_OFFSET to be 0x700000.
>

Ack.

>> +#define BITSTREAM2_OFFSET  BITSTREAM1_OFFSET + SZ_32M
>
> You should add braces around the macro definitions to make them safe
> to use.
>

Ack.

>> +
>> +extern char __dtb_z_socfpga_arria10_mercury_aa1_start[];
>> +
>> +#define ARRIA10_STACKTOP	ARRIA10_OCRAM_ADDR + SZ_256K
>> +
>> +ENTRY_FUNCTION_WITHSTACK(start_socfpga_aa1_xload, ARRIA10_STACKTOP, r0, r1, r2)
>> +{
>> +	int pbl_index = 0;
>> +	int barebox = 0;
>> +	int bitstream = 0;
>> +
>> +	arm_cpu_lowlevel_init();
>> +
>> +	relocate_to_current_adr();
>> +
>> +	setup_c();
>> +
>> +	arria10_init(&mainpll_cfg, &perpll_cfg, pinmux);
>> +
>> +	arria10_prepare_mmc(BAREBOX_PART, BITSTREAM_PART);
>> +
>> +	pbl_index = readl(ARRIA10_SYSMGR_ROM_INITSWLASTLD);
>> +
>> +	/* Allow booting from both PBL0 and PBL1 to allow atomic updates.
>> +	 * Bitstreams redundant too and expected to reside in the second
>> +	 * partition.
>> +	 * There is a fixed relation between the PBL/barebox instance and its
>> +	 * bitstream location (offset) that requires to update them together */
>> +	switch (pbl_index) {
>> +	case 0:
>> +		barebox = BAREBOX1_OFFSET;
>> +		bitstream = BITSTREAM1_OFFSET;
>> +		break;
>> +	case 1:
>> +		barebox = BAREBOX2_OFFSET;
>> +		bitstream = BITSTREAM1_OFFSET;
>> +		break;
>> +	case 2:
>> +	case 3:
>> +		/* Left blank for future extension */
>> +		break;
>
> You should either bail out or use a sane default for unhandled cases.
>

Ack.

>> diff --git a/arch/arm/dts/socfpga_arria10_mercury_aa1.dts b/arch/arm/dts/socfpga_arria10_mercury_aa1.dts
>> new file mode 100644
>> index 0000000000..ef3afc9b98
>> --- /dev/null
>> +&mmc {
>> +	bus-width = <8>;
>> +	non-removable;
>> +	disable-wp;
>> +	no-sd;
>> +
>> +	partitions {
>> +		compatible = "fixed-partitions";
>> +		#size-cells = <1>;
>> +		#address-cells = <1>;
>> +
>> +		barebox1_xload: partition@100000 {
>> +			label = "barebox1-xload";
>> +			reg = <0x100000 0x40000>;
>> +		};
>> +
>> +		barebox2_xload: partition@140000 {
>> +			label = "barebox2-xload";
>> +			reg = <0x140000 0x40000>;
>> +		};
>> +
>> +		barebox1: partition@200000 {
>> +			label = "barebox1";
>> +			reg = <0x200000 0x80000>;
>> +		};
>> +
>> +		barebox2: partition@280000 {
>> +			label = "barebox2";
>> +			reg = <0x280000 0x80000>;
>> +		};
>
> It might be worth increasing the size to 1MiB. It's easy to make barebox
> bigger than 512KiB.
>

Sure.


Thanks,
Steffen

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