Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 29: #define SECTION_ALIGN 64 /* TODO not mentioned in kernel DOCs */
This is not a page boundary, like your comment below says. […]
Done
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 43: MAX_KERNEL_SIZE >> 20);
nit: is there really a point in printing out the same constant size on every boot?
Done
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 83: "boot a armv7 kernel!\n");
nit: don't break strings (and we can use 96 chars now anyway). […]
Done
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 108: kernel->offset += 32 * MiB;
Doesn't this mean the size you checked in fit_place_mem() isn't necessarily enough anymore (because […]
Done
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 121: lower_mem
Again, just use _dram. All Arm platforms know where their memory starts at compile time.
I guess that assumes that the payload will be loaded at _dram, which might not be the case on qemu as the stages and stage cache resides near _dram. Shouldn't we use the offset from kernel start as it is likely placed aboved the stages and stage cache?
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 135: fdt->size
Actually, I just noticed a bug in lib/fit_payload. […]
done in CB:41201