[PATCH 07/10] arm: mvebu: clearfog: add SPI offsets

[Joel Johnson]

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