On 2020-01-12 03:42, Baruch Siach wrote:
Hi Joel,
On Sat, Jan 11 2020, Joel Johnson wrote:
Add reasonable default SPI offsets and ENV size when configured to boot from SPI flash.
Signed-off-by: Joel Johnson mrjoel@lixil.net
board/solidrun/clearfog/Kconfig | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/board/solidrun/clearfog/Kconfig b/board/solidrun/clearfog/Kconfig index fd880ee591..ce7fcf653f 100644 --- a/board/solidrun/clearfog/Kconfig +++ b/board/solidrun/clearfog/Kconfig @@ -37,4 +37,16 @@ config CLEARFOG_2GB_SOM Enable support for the 2GB RAM SOM variant. If this option is not enabled then the more common 1GB version will be used.
+config ENV_SECT_SIZE
- hex "Environment Sector-Size"
- # Use SPI flash erase block size of 4 KiB
- default 0x1000 if MVEBU_SPL_BOOT_DEVICE_SPI
- # Use optimistic 64 KiB erase block, will vary between actual media
- default 0x10000 if MVEBU_SPL_BOOT_DEVICE_MMC
Duplication of config symbols in platform specific Kconfig goes against common practice. Platform specific values should go in defconfig. Support for Clearfog SPI boot should be in a dedicated defconfig, I think.
Same comment on patches #9 and #10.
On the config symbol duplication, that's what seemed to be advocated by the Kconfig documentation, perhaps I'm reading/interpreting it incorrectly? I found examples of doing it how I did and it seemed to match the documented inteded usage. From https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html: "Default values are not limited to the menu entry where they are defined. This means the default can be defined somewhere else or be overridden by an earlier definition."
On the topic of a separate defconfig for SPI booting the same platform, I have been intentionally trying to avoid that. My use cases include SPI booting, but env in MMC, and MMC booting, but env in SPI - I don't intend to make those default mechanisms since they're admittedly a bit more esoteric, but I would still like them to be easily obtainable using a few custom config definitions at build time.
+config SYS_SPI_U_BOOT_OFFS
- hex "address of u-boot payload in SPI flash"
- default 0x20000
- depends on MVEBU_SPL_BOOT_DEVICE_SPI
It might be better to add u-boot,spl-payload-offset property to the U-Boot specific -u-boot.dtsi file instead.
I hadn't considered that option and it's intriguing, but what would make it objectively better? It seems just an alternate location to put a statically defined value, having it as a custom U-Boot DTS entry doesn't allow runtime changing, does it?
Joel