Hi Joel, On Sun, Jan 12 2020, Joel Johnson wrote: > 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 <mrj...@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."
U-Boot code has very few duplicate definitions for setting default values. I'll let the maintainers decide on that. > 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? Right. u-boot,spl-payload-offset clobbers the value from spl_spi_get_uboot_offs() in current code. It's just that I find the DT property nicer than duplicated config. But I can't think of a technical reason to prefer DT set offset. baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - bar...@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -