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

[Baruch Siach <baruch@tkos.co.il>]

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@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------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -