| Message ID | 20200111193639.19022-7-mrjoel@lixil.net |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] arm: mvebu: fix SerDes table alignment | expand |
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 at 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. > +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. > + > endmenu baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
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 at 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
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 <mrjoel at 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
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 + +config SYS_SPI_U_BOOT_OFFS + hex "address of u-boot payload in SPI flash" + default 0x20000 + depends on MVEBU_SPL_BOOT_DEVICE_SPI + endmenu
Add reasonable default SPI offsets and ENV size when configured to boot from SPI flash. Signed-off-by: Joel Johnson <mrjoel at lixil.net> --- board/solidrun/clearfog/Kconfig | 12 ++++++++++++ 1 file changed, 12 insertions(+)