Message-ID: <65bf4f83-43c2-4640-9858-afb96fa1cfc7@ti.com>
Date: Fri, 13 Jun 2025 16:29:11 +0530
From: MD Danish Anwar <danishanwar@...com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Meghana Malladi <m-malladi@...com>, Vignesh Raghavendra <vigneshr@...com>,
        Simon Horman <horms@...nel.org>,
        Guillaume La Roque <glaroque@...libre.com>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Roger Quadros <rogerq@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>, Eric
 Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew+netdev@...n.ch>,
        <linux-arm-kernel@...ts.infradead.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <srk@...com>,
        Roger
 Quadros <rogerq@...com>
Subject: Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload
 support



On 12/06/25 8:40 pm, Vladimir Oltean wrote:
> On Wed, Jun 11, 2025 at 03:10:35PM +0530, MD Danish Anwar wrote:
>>> I am not very positive that even if adding the extra restrictions
>>> discovered here (cycle-time cannot be != IEP_DEFAULT_CYCLE_TIME_NS),
>>> the implementation will work as expected. I am not sure that our image
>>> of "as expected" is the same.
>>>
>>> Given that these don't seem to be hardware limitations, but constraints
>>> imposed by the ICSSG firmware, I guess my suggestion would be to start
>>> with the selftest I mentioned earlier (which may need to be adapted),
>>
>> Yes I am working on running the selftest on ICSSG driver however there
>> are some setup issues that I am encountering. I will try to test this
>> using the selftest.
>>
>>> and use it to get a better picture of the gaps. Then make a plan to fix
>>> them in the firmware, and see what it takes. If it isn't going to be
>>> sufficient to fix the bugs unless major API changes are introduced, then
>>> maybe it doesn't make sense for Linux to support taprio offload on the
>>> buggy firmware versions.
>>>
>>> Or maybe it does (with the appropriate restrictions), but it would still
>>> inspire more trust to see that the developer at least got some version
>>> of the firmware to pass a selftest, and has a valid reference to follow.
>>
>> Sure. I think we can go back to v9 implementation (no extend feature)
>> and add two additional restrictions in the driver.
>>
>> 1. Cycle-time needs to be 1ms
>> 2. Base-time needs to be Multiple of 1ms
>>
>> With these two restrictions we can have the basic taprio support. Once
>> the firmware is fixed and has support for both the above cases, I will
>> modify the driver as needed.
>>
>> I know firmware is very buggy as of now. But we can still start the
>> driver integration and fix these bugs with time.
>>
>> I will try to test the implementation with these two limitations using
>> the selftest and share the logs if it's okay with you to go ahead with
>> these limitations.
>>
>>> Not going to lie, it doesn't look great that we discover during v10 that
>>> taprio offload only works with a cycle time of 1 ms. The schedule is
>>
>> I understand that. Even I got to know about this limitation after my
>> last response to v10
>> (https://lore.kernel.org/all/5e928ff0-e75b-4618-b84c-609138598801@ti.com/)
>>
>>> network-dependent and user-customizable, and maybe the users didn't get
>>> the memo that only 1 ms was tested :-/
>>
>> Let me know if it'll be okay to go ahead with the two limitations
>> mentioned above for now (with selftest done).
>>
>> If it's okay, I will try to send v11 with testing with selftest done as
>> well. Thanks for the continuous feedback.
> 
> I don't want to gate your upstreaming efforts, but a new version with
> just these extra restrictions, and no concrete plan to lift them, will
> be seen with scepticism from reviewers. You can alleviate some of that
> by showing results from a selftest.

We will make a complete plan for fixing these restrictions and I will
update the community.

> 
> The existing selftest uses a 2 ms schedule and a 10 ms schedule. Neither
> of those is supported by your current proposal. You can modify the
> schedules to be compatible with your current firmware, and the selftest
> may pass that way, but I will not be in favor of accepting that change
> upstream, because the cycle time is something that needs to be highly
> adaptive to the network requirements.
> 

I can tweak the script to use 1ms cycle time. I tried to run the script
however I am not able to run the script due to multiple package
dependencies which I am currently trying to resolve.

I checked your commit [1] where you have introduced the tc_taprio.sh
script. You have mentioned,
	"This is specifically intended for NICs with an offloaded data path
(switchdev/DSA) and requires taprio 'flags 2'. Also, $h1 and $h2 must
support hardware timestamping, and $h1 tc-etf offload, for isochron to
work."

My NIC does support offloaded data path (switchdev), taprio 'flags 2'
and hardware timestamping. However we don't support tc-etf offload.

Will it be possible to use this script without the support of tc-etf
offload?

> So to summarize, you can try to move forward with a restricted version
> of this feature, but you will have to be very transparent as to what
> works and what are the next steps, as well as give assurance that you
> intend to keep supporting the current firmware and its API when an
> improved firmware will become available that lifts these restrictions.

We are working on a plan to get full taprio support and I will update
the community accordingly. I want the driver to get upstreamed with
whatever support we have right now and add things later on. If I am able
to verify the `tc_taprio.sh` script then I would share results from
there. If I am not able to verify the script, I will at least try to
mention what we are supporting now and what are the limitations and when
do we plan on addressing those limitations.

[1]
https://lore.kernel.org/all/20250426144859.3128352-5-vladimir.oltean@nxp.com/#r

-- 
Thanks and Regards,
Danish

Powered by blists - more mailing lists