[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Joel Johnson]

On 2020-01-21 10:49, Baruch Siach wrote:
> Hi Joel,
> 
> On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote:
>> Add a unique entry for ClearFog Base variant, reflected in the board
>> name and adjusted SerDes topology.
>> 
>> Signed-off-by: Joel Johnson <mrjoel at lixil.net>
>> 
>> ---
>> 
>> v2 changes:
>>   - reworked based on Baruch's run-time TLV EEPROM detection series
>> v3 changes:
>>   - rebased on mvebu merged run-time TLV EEPROM detection series
>>   - minor update to help test regarding runtime detection failures
>> 
>> ---
>>  arch/arm/mach-mvebu/Kconfig        |  2 ++
>>  board/solidrun/clearfog/Kconfig    | 18 ++++++++++++++++++
>>  board/solidrun/clearfog/clearfog.c | 12 ++++++++++++
>>  3 files changed, 32 insertions(+)
>>  create mode 100644 board/solidrun/clearfog/Kconfig
>> 
>> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
>> index bc5eaa5a76..161dee937f 100644
>> --- a/arch/arm/mach-mvebu/Kconfig
>> +++ b/arch/arm/mach-mvebu/Kconfig
>> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX
>>  	default 0
>>  	depends on SECURED_MODE_IMAGE
>> 
>> +source "board/solidrun/clearfog/Kconfig"
>> +
>>  endif
>> diff --git a/board/solidrun/clearfog/Kconfig 
>> b/board/solidrun/clearfog/Kconfig
>> new file mode 100644
>> index 0000000000..936d5918f8
>> --- /dev/null
>> +++ b/board/solidrun/clearfog/Kconfig
>> @@ -0,0 +1,18 @@
>> +menu "ClearFog configuration"
>> +	depends on TARGET_CLEARFOG
>> +
>> +config TARGET_CLEARFOG_BASE
>> +	bool "Use ClearFog Base static configuration"
>> +	help
>> +	  Use the ClearFog Base as the static configuration instead of the
>> +	  default which uses the ClearFog Pro.
>> +
>> +	  Runtime board detection is always attempted and used if available. 
>> The
>> +	  static configuration is used as a fallback in cases where runtime
>> +	  detection is disabled, is not available in hardware, or otherwise 
>> fails.
>> +
>> +	  Only newer revisions of the ClearFog product line support runtime
>> +	  detection via additional EEPROM hardware. This option enables 
>> selecting
>> +	  the Base variant for older hardware revisions.
>> +
>> +endmenu
>> diff --git a/board/solidrun/clearfog/clearfog.c 
>> b/board/solidrun/clearfog/clearfog.c
>> index e268ef55a2..e77b9465d4 100644
>> --- a/board/solidrun/clearfog/clearfog.c
>> +++ b/board/solidrun/clearfog/clearfog.c
>> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = {
>>  	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>>  	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>>  	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>> +#if defined (CONFIG_TARGET_CLEARFOG_BASE)
>> +	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>> +#else
>>  	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>> +#endif
> 
> I'd prefer run-time test instead of '#ifdefs' that IMO make the code 
> harder to
> read. Something like this (build tested only):
> 
> diff --git a/board/solidrun/clearfog/clearfog.c
> b/board/solidrun/clearfog/clearfog.c
> index e268ef55a2a0..202c60cb7841 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map
> **serdes_map_array, u8 *count)
>  {
>  	cf_read_tlv_data();
> 
> -	if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) {
> +	if (sr_product_is(&cf_tlv_data, "Clearfog GTR") ||
> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) {
>  		board_serdes_map[0].serdes_type = PEX0;
>  		board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS;
>  		board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1;
> @@ -172,6 +173,9 @@ int checkboard(void)
>  {
>  	char *board = "ClearFog";
> 
> +	if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
> +		board = "ClearFog Base";
> +
>  	cf_read_tlv_data();
>  	if (strlen(cf_tlv_data.tlv_product_name[0]) > 0)
>  		board = cf_tlv_data.tlv_product_name[0];
> @@ -194,7 +198,8 @@ int board_late_init(void)
>  {
>  	cf_read_tlv_data();
> 
> -	if (sr_product_is(&cf_tlv_data, "Clearfog Base"))
> +	if (sr_product_is(&cf_tlv_data, "Clearfog Base") ||
> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
>  		env_set("fdtfile", "armada-388-clearfog-base.dtb");
>  	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4"))
>  		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");
> 
> What do you think?
> 
> baruch

I'll have to revisit and test to be sure, but I believe the reason I 
didn't go that route is because I wasn't able to get the 
CONFIG_IS_ENABLED macro version to trigger when building SPL, although 
it worked for the main path. I know that was the case for something I 
was working up, but I don't recall whether it was this patch 
specifically or not.

A bit of a nit, but just using the macro isn't really runtime detection, 
however adding the separate code path for naming and serdes manipulation 
dynamically tends that way. I'm already running into several cases (not 
the default build options) where the SPL doesn't fit in the default 128K 
offset size, so I'm leery of adding paths that add actual binary size 
increase. I'm all for switching to CONFIG_IS_ENABLED if I test and can 
get it to work, but still would lean towards just replacing the ifdef in 
place; after all it is meant to be the static configuration, not 
dynamic.

Joel