[PATCH v4 06/12] arm: mvebu: clearfog: Add option for 2.5 Gbps SFP

[Joel Johnson]

On 2020-01-27 09:27, Joel Johnson wrote:
> On 2020-01-27 00:20, Baruch Siach wrote:
>> Hi Joel,
>> 
>> On Mon, Jan 27 2020, Joel Johnson wrote:
>>> On 2020-01-26 22:29, Baruch Siach wrote:
>>>> On Mon, Jan 27 2020, Joel Johnson wrote:
>>>>> While newer Linux kernels provide autoconfiguration of SFP, provide
>>>>> an option for setting in U-Boot Kconfig for use prior to booting.
>>>>> 
>>>>> Signed-off-by: Joel Johnson <mrjoel at lixil.net>
>>>>> 
>>>>> ---
>>>>> 
>>>>> v2 changes:
>>>>>   - fixed help indentation
>>>>> v3 changes:
>>>>>   - none
>>>>> v4 changes:
>>>>>   - adjust static SerDes configuration at runtime instead of #ifdef
>>>>> 
>>>>> ---
>>>>>  board/solidrun/clearfog/Kconfig    | 7 +++++++
>>>>>  board/solidrun/clearfog/clearfog.c | 3 +++
>>>>>  2 files changed, 10 insertions(+)
>>>>> 
>>>>> diff --git a/board/solidrun/clearfog/Kconfig
>>>>> b/board/solidrun/clearfog/Kconfig
>>>>> index 936d5918f8..c910e17093 100644
>>>>> --- a/board/solidrun/clearfog/Kconfig
>>>>> +++ b/board/solidrun/clearfog/Kconfig
>>>>> @@ -15,4 +15,11 @@ config TARGET_CLEARFOG_BASE
>>>>>  	  detection via additional EEPROM hardware. This option enables
>>>>> selecting
>>>>>  	  the Base variant for older hardware revisions.
>>>>> 
>>>>> +config CLEARFOG_SFP_25GB
>>>>> +	bool "Enable 2.5 Gbps mode for SFP"
>>>>> +	help
>>>>> +	  Set the SFP module connection to support 2.5 Gbps transfer 
>>>>> speed for
>>>>> the
>>>>> +	  SGMII connection (requires a supporting SFP). By default, 
>>>>> transfer
>>>>> speed
>>>>> +	  of 1.25 Gbps is used, suitable for a more common 1 Gbps SFP 
>>>>> module.
>>>>> +
>>>>>  endmenu
>>>>> diff --git a/board/solidrun/clearfog/clearfog.c
>>>>> b/board/solidrun/clearfog/clearfog.c
>>>>> index 4019e37016..6c5b9a784f 100644
>>>>> --- a/board/solidrun/clearfog/clearfog.c
>>>>> +++ b/board/solidrun/clearfog/clearfog.c
>>>>> @@ -59,6 +59,9 @@ void config_static_serdes_map(void)
>>>>>  		board_serdes_map[4].serdes_speed = SERDES_SPEED_5_GBPS;
>>>>>  		board_serdes_map[4].serdes_mode = SERDES_DEFAULT_MODE;
>>>>>  	}
>>>>> +
>>>>> +	if (IS_ENABLED(CONFIG_CLEARFOG_SFP_25GB))
>>>>> +		board_serdes_map[5].serdes_speed = SERDES_SPEED_3_125_GBPS;
>>>> 
>>>> config_static_serdes_map() is not called only when TLV EEPROM is
>>>> present. So CONFIG_CLEARFOG_SFP_25GB has no effect in that case. 
>>>> This
>>>> code should move to hws_board_topology_load() to avoid that.
>>> 
>>> Yeah, that's what I was trying to ask with the question below 
>>> embedded in
>>> patch 4.
>>> 
>>>     "Note that this approach *does not* currently provide any 
>>> mechanism
>>>     for EEPROM detected boards to have their SFP speed changed or 
>>> switch
>>>     between PCIE/SATA signalling. I'm assuming that will be done 
>>> based on
>>>     hardware detection, but confirmation/acceptance in review would 
>>> be
>>>     appreciated so it can be revisited if needed.
>>> 
>>> After sending the series and thinking about it more, I didn't see any 
>>> other
>>> mechanism by which the configuration would be changed. The question 
>>> was really
>>> to confirm what I had inferred from code, namely that Pro/Base boards 
>>> even
>>> with EEPROM would still have to have a U-Boot built with custom 
>>> options in
>>> order to change the PCIe/SATA or SFP options. I would like to confirm 
>>> with you
>>> that is desired and expected even in the cases of EEPROM supporting 
>>> units. If
>>> that's the case, I'll rework the location of setting, along with 
>>> updating the
>>> help text for those options. Pending your confirmation, I plan to get 
>>> rid of
>>> the separate config_static_serdes_map function and fold it back in, 
>>> with the
>>> SATA and SFP options coming before the board runtime detection 
>>> configuration.
>> 
>> My thinking is that EEPROM TLV is only meant to describe the hardware 
>> as
>> manufactured, and never change. SATA (read mSATA) and SFP are user 
>> setup
>> that TLV can't cover. So I think that SATA and SFP build time 
>> selection
>> should apply regardless of TLV.
>> 
>> In theory it could have been even better to store SATA/SFP selection 
>> in
>> environment variables. But that is not currently possible, since 
>> serdes
>> configuration takes place at SPL phase where the environment is not
>> accessible.
>> 
>> baruch
> 
> Yeah, that makes sense and as I thought further on it is the only
> logical option based at least on what's currently exposed for runtime
> detction. I'll send out the updated series with this change and a few
> code style cleanups after completing the SATA testing, hopefully later
> today.
> 
> Any other items pending for this series? I think it's in good shape,
> pending any further thoughts or discussion on the Pro DT by default
> change.
> 
> Joel

While SATA testing is pending, I'm including the updated resulting 
board/solidrun/clearfog/clearfog.c snippet (without patch separations) 
for review on the updated approach. I recognize that the final "Clearfog 
Base" and static IS_ENABLED check take the same path and could be 
combined, but I prefer to keep them separate to highlight and 
distinguish between the runtime detection and fallback cases.

Joel

void config_cfbase_serdes_map(void)
{
         board_serdes_map[4].serdes_type = USB3_HOST0;
         board_serdes_map[4].serdes_speed = SERDES_SPEED_5_GBPS;
         board_serdes_map[4].serdes_mode = SERDES_DEFAULT_MODE;
}

int hws_board_topology_load(struct serdes_map **serdes_map_array, u8 
*count)
{
         cf_read_tlv_data();

         /* Apply build configuration options before runtime 
configuration */
         if (IS_ENABLED(CONFIG_CLEARFOG_SFP_25GB))
                 board_serdes_map[5].serdes_speed = 
SERDES_SPEED_3_125_GBPS;

         if (IS_ENABLED(CONFIG_CLEARFOG_CON2_SATA) &&
            !IS_ENABLED(CONFIG_TARGET_CLEARFOG_BASE)) {
                 board_serdes_map[4].serdes_type = SATA2;
                 board_serdes_map[4].serdes_speed = SERDES_SPEED_3_GBPS;
                 board_serdes_map[4].serdes_mode = SERDES_DEFAULT_MODE;
                 board_serdes_map[4].swap_rx = 1;
         }

         if (IS_ENABLED(CONFIG_CLEARFOG_CON3_SATA)) {
                 board_serdes_map[2].serdes_type = SATA1;
                 board_serdes_map[2].serdes_speed = SERDES_SPEED_3_GBPS;
                 board_serdes_map[2].serdes_mode = SERDES_DEFAULT_MODE;
                 board_serdes_map[2].swap_rx = 1;
         }

         /* Apply runtime detection changes */
         if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) {
                 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;
         } else if (sr_product_is(&cf_tlv_data, "Clearfog Pro")) {
                 /* handle recognized product as noop, no adjustment 
required */
         } else if (sr_product_is(&cf_tlv_data, "Clearfog Base")) {
                 config_cfbase_serdes_map();
         } else if (IS_ENABLED(CONFIG_TARGET_CLEARFOG_BASE)) {
                 /*
                  * Fallback to static default. Runtime detection either
                  * failed, hardware support not present, EEPROM is 
corrupt,
                  * or an unrecognized product name is present.
                  */
                 config_cfbase_serdes_map();
         }

         *serdes_map_array = board_serdes_map;
         *count = ARRAY_SIZE(board_serdes_map);
         return 0;
}