Re: [PATCH net-next v4] net: ti: icssg_prueth: add TAPRIO offload support

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: MD Danish Anwar <danishanwar@ti.com>
Cc: Andrew Lunn <andrew@lunn.ch>, Roger Quadros <rogerq@kernel.org>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Richard Cochran <richardcochran@gmail.com>,
	Paolo Abeni <pabeni@redhat.com>, Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Simon Horman <horms@kernel.org>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org, srk@ti.com,
	r-gunasekaran@ti.com, linux-arm-kernel@lists.infradead.org,
	Roger Quadros <rogerq@ti.com>,
	Vinicius Costa Gomes <vinicius.gomes@intel.com>
Subject: Re: [PATCH net-next v4] net: ti: icssg_prueth: add TAPRIO offload support
Date: Thu, 25 Apr 2024 15:18:17 +0300	[thread overview]
Message-ID: <20240425121817.r34tpatcs7qqe4oi@skbuf> (raw)
In-Reply-To: <e311f2c9-f396-41ae-b78b-7bf8efafe066@ti.com>

On Thu, Apr 25, 2024 at 05:27:03PM +0530, MD Danish Anwar wrote:
> On 18/01/24 6:57 am, Vladimir Oltean wrote:
> > On Mon, Jan 15, 2024 at 12:24:12PM +0530, MD Danish Anwar wrote:
> >>> I believe the intention is for this code to be run before any taprio
> >>> offload is added, correct? But it is possible for the user to add an
> >>
> >> Yes, the intention here is to run this code before any taprio offload is
> >> added.
> > 
> > Then it is misplaced?
> > 
> 
> Where should I move it then? Perhaps to end of prueth_probe()?
> If this is moved to prueth_probe() then it will mean it's always called.
> If user adds an offloaded Qdisc even while the netdev has not yet been
> brought up, it will not result into any error

Sorry, I just do not have the free time to dig up an email thread from January.
My feedback was that the network stack expects that a tc qdisc can be
programmed even while the netdev is down, and it is expected that the
offload survives even an up-down cycle, link speed changes, etc. Please
use your best judgement on how to accomplish that in the next version.

> >>>> +
> >>>> +	cycle_time = admin_list->cycle_time - 4; /* -4ns to compensate for IEP wraparound time */
> >>>
> >>> Details? Doesn't this make the phase alignment of the schedule diverge
> >>> from what the user expects?
> >>
> >> 4ns is needed to compensate for IEP wraparound time. IEP is the clock
> >> used by ICSSG driver. IEP tick is 4ns and we adjust this 4ns whenever
> >> calculating cycle_time. You may refer to [1] for details on IEP driver.
> > 
> > What is understood by "IEP wraparound time"? Its time wraps around what?
> 
> IEP clock runs at 250 MHz, 1 tick of IEP clock = NSEC_PER_SEC /
> iep->refclk_freq i.e. 1000000000 / 250000000 = 4ns.
> 
> Thus 1 tick of IEP clock is 4ns.
> 
> > It wraps around exactly once every taprio cycle of each port and that's
> > why the cycle-time is compensated, or how does that work?
> > 
> 
> Yes, it wraps around exactly once every taprio cycle and to compensate
> for that we adjust 4ns. Instead of hardcoding I will use a varaible here.
> 
> It is a hardware errata but it is not public yet.

Please leave the relevant details for the compensation workaround in a
comment, even if you do not have a number for the erratum. Many eyes
will be on this code when engineers try to synchronize the port using
PTP and measure the timestamps of scheduled traffic, to compare them
with the expectation.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel