Re: [PATCH net-next v9 2/2] net: ti: icssg_prueth: add TAPRIO offload support

[Anwar, Md Danish]

On 6/3/2024 7:35 PM, Vladimir Oltean wrote:
> On Mon, Jun 03, 2024 at 04:51:00PM +0300, Vladimir Oltean wrote:
>>>>> +static void tas_reset(struct prueth_emac *emac)
>>>>> +{
>>>>> +	struct tas_config *tas = &emac->qos.tas.config;
>>>>> +	int i;
>>>>> +
>>>>> +	for (i = 0; i < TAS_MAX_NUM_QUEUES; i++)
>>>>> +		tas->max_sdu_table.max_sdu[i] = 2048;
>>>>
>>>> Macro + short comment for the magic number, please.
>>>>
>>>
>>> Sure I will add it. Each elements in this array is a 2 byte value
>>> showing the maximum length of frame to be allowed through each gate.
>>
>> Is the queueMaxSDU[] array active even with the TAS being in the reset
>> state? Does this configuration have any impact upon the device MTU?
>> I don't know why 2048 was chosen.
> 
> Another comment here is: in the tc-taprio UAPI, a max-sdu value of 0
> is special and means "no maxSDU limit for this TX queue". You are
> programming the values from taprio straight away to hardware, so,
> assuming there's no bug there, it means that the hardware also
> understands 0 to mean "no maxSDU limit".
> 

I discussed this with the firmware team. They are not treating 0 as
something special (""no maxSDU limit for this TX queue"). They have
limit on every queue. Driver needs to handle the max-sdu size carefully.

> If so, then during tas_reset(), after which the TAS should be disabled,
> why aren't you also using 0 as a default value, but 2048?

As using 0 doesn't mean anything special in firmware. The default value
during reset is kept as the max supported value.

There's also one thing missing here, the max-sdu table in firmware is
updated (by calling tas_update_maxsdu_table()) only once by driver
during tas_reset(). The firmware table should also be updated once
before triggering the list change so that the firmware would know what
are the max-sdu value that user has requested.

If a user request max-sdu as 0 0 0 80 for 4 queues. The driver will
update these values to firmware as PRUETH_MAX_MTU, PRUETH_MAX_MTU,
PRUETH_MAX_MTU, 80.


-- 
Thanks and Regards,
Md Danish Anwar