Re: [PATCH v2] net/cxgbe: read firmware configuration file from filesystem

[Ferruh Yigit <ferruh.yigit@xilinx.com>]

On 5/16/2022 12:56 PM, Rahul Lakkireddy wrote:
> On Monday, May 05/16/22, 2022 at 12:06:01 +0100, Ferruh Yigit wrote:
>> On 5/16/2022 11:27 AM, Rahul Lakkireddy wrote:
>>> Add support to read firmware configuration file from
>>> /lib/firmware/cxgb4/ path in the filesystem. The firmware
>>> config file is used to enable or disable NIC features before
>>> firmware initialization to help retrieve better debug data to
>>> analyze firmware init failures. The config file can also
>>> be used to redistribute resources, like queues, TCAMs, etc.,
>>> from disabled physical functions (PFs) to main PF, before
>>> firmware init.
>>
>> Hi Rahul,
>>
>> Please find comments below.
>>
>> Also can you please send a new version for both 4/5 and 5/5 (this patch)
>> from original series?
>>
> 
> I had already posted v2 for 4/5 from original series at below location.
> 
> https://patches.dpdk.org/project/dpdk/patch/61ae717665c2f4e38712652bec2f9d0fe5ca7d32.1651842841.git.rahul.lakkireddy@chelsio.com/
> 
> Do you want me to send a new patch series with 4/5 and 5/5 in a
> single patch series?
> 

Nope, I missed above patch, it is good as separate patches.

>>>
>>> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
>>> ---
>>> v2:
>>> - Update cxgbe pmd doc about the firmware config file
>>> - Update commit message to explain more about firmware config file
>>>
>>>    doc/guides/nics/cxgbe.rst               |  28 ++
>>>    drivers/net/cxgbe/base/t4fw_interface.h |   1 +
>>>    drivers/net/cxgbe/cxgbe_main.c          | 329 ++++++++++++++++--------
>>>    3 files changed, 245 insertions(+), 113 deletions(-)
>>>
>>> diff --git a/doc/guides/nics/cxgbe.rst b/doc/guides/nics/cxgbe.rst
>>> index a1d30c488b..fc8a5751f1 100644
>>> --- a/doc/guides/nics/cxgbe.rst
>>> +++ b/doc/guides/nics/cxgbe.rst
>>> @@ -838,3 +838,31 @@ to configure the mtu of all the ports with a single command.
>>>         testpmd> port stop all
>>>         testpmd> port config all max-pkt-len 9000
>>> +
>>> +Hardware Configuration and Debugging
>>> +------------------------------------
>>> +
>>> +Firmware Configuration File
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +To enable or disable Chelsio NIC features before firmware initialization,
>>> +the Chelsio firmware configuration file can be placed in following
>>> +directory. The CXGBE PMD will search and pick up the firmware
>>> +configuration file during the Chelsio NIC probe, before initializing
>>> +the firmware.
>>> +
>>
>> Does it worth to mention what happens if the FW config file doesn't exist?
>> Or mention from FW config file load order, as you described in mail list, to
>> understand the relation with 'cxgbtool'?
>>
> 
> Will do.
> 
>>> +.. code-block:: console
>>> +
>>> +   cp <path_to_config_file>/t6-config.txt /lib/firmware/cxgb4/t6-config.txt
>>> +
>>
>> There is also 't5-config.txt' in the code.
>>
> 
> Yes, t5-config.txt is for Chelsio T5 NIC series and t6-config.txt
> is for Chelsio T6 NIC series. Will update t5-config.txt as well.
> 
>>> +The firmware configuration file is mainly intended to be used to debug
>>> +firmware initialization failures. It can also be used to redistribute
>>> +NIC resources from disabled physical functions (PFs) to main PF before
>>> +initializing firmware.
>>> +
>>> +.. warning::
>>> +
>>> +   Note that the Chelsio firmware configuration file contains very low
>>> +   level details that is specific to the Chelsio NIC. Hence, the
>>> +   firmware configuration file must not be modified without expert
>>> +   guidance from Chelsio support team.
>>
>> Will it be too much detail to document what config can be changed via this
>> FW config?
>>
> 
> Yes, this will be difficult to document. The config file just looks like
> register=value pairs and are often dependent on each other based on
> the selected configuration. A portion of sample config file is given
> below. I'm not sure how to document this kind of info.
> 
> [global]
>      reg[0x100c] = 0x22222222
>      reg[0x10a0] = 0x01040810
>      reg[0x1044] = 4096
>      reg[0x1048] = 65536
>      reg[0x104c] = 1536
>      reg[0x1050] = 9024
>      reg[0x1054] = 9216
>      reg[0x1058] = 2048
>      reg[0x105c] = 128
>      reg[0x1060] = 8192
>      reg[0x1064] = 16384
> [...]
> 

Above won't be much useful to user anyway (without description of the 
values), if there is already some documentation a link to that can be 
good, otherwise OK to skip this.