Message-ID: <2034056567.1653904.1751452157261.JavaMail.zimbra@couthit.local>
Date: Wed, 2 Jul 2025 15:59:17 +0530 (IST)
From: Parvathi Pudi <parvathi@...thit.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: parvathi <parvathi@...thit.com>, danishanwar <danishanwar@...com>, 
	rogerq <rogerq@...nel.org>, andrew+netdev <andrew+netdev@...n.ch>, 
	davem <davem@...emloft.net>, edumazet <edumazet@...gle.com>, 
	kuba <kuba@...nel.org>, pabeni <pabeni@...hat.com>, 
	robh <robh@...nel.org>, krzk+dt <krzk+dt@...nel.org>, 
	conor+dt <conor+dt@...nel.org>, ssantosh <ssantosh@...nel.org>, 
	richardcochran <richardcochran@...il.com>, 
	s hauer <s.hauer@...gutronix.de>, m-karicheri2 <m-karicheri2@...com>, 
	glaroque <glaroque@...libre.com>, afd <afd@...com>, 
	saikrishnag <saikrishnag@...vell.com>, m-malladi <m-malladi@...com>, 
	jacob e keller <jacob.e.keller@...el.com>, 
	diogo ivo <diogo.ivo@...mens.com>, 
	javier carrasco cruz <javier.carrasco.cruz@...il.com>, 
	horms <horms@...nel.org>, s-anna <s-anna@...com>, 
	basharath <basharath@...thit.com>, 
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>, 
	netdev <netdev@...r.kernel.org>, 
	devicetree <devicetree@...r.kernel.org>, 
	linux-kernel <linux-kernel@...r.kernel.org>, 
	pratheesh <pratheesh@...com>, Prajith Jayarajan <prajith@...com>, 
	Vignesh Raghavendra <vigneshr@...com>, praneeth <praneeth@...com>, 
	srk <srk@...com>, rogerq <rogerq@...com>, 
	krishna <krishna@...thit.com>, pmohan <pmohan@...thit.com>, 
	mohan <mohan@...thit.com>
Subject: Re: [PATCH net-next v9 11/11] net: ti: prueth: Adds PTP OC Support
 for AM335x and AM437x

Hi,

> 
>> On 23/06/2025 17:42, Parvathi Pudi wrote:
>>> From: Roger Quadros <rogerq@...com>
>>> 
>>> PRU-ICSS IEP module, which is capable of timestamping RX and
>>> TX packets at HW level, is used for time synchronization by PTP4L.
>>> 
>>> This change includes interaction between firmware/driver and user
>>> application (ptp4l) with required packet timestamps.
>>> 
>>> RX SOF timestamp comes along with packet and firmware will rise
>>> interrupt with TX SOF timestamp after pushing the packet on to the wire.
>>> 
>>> IEP driver available in upstream linux as part of ICSSG assumes 64-bit
>>> timestamp value from firmware.
>>> 
>>> Enhanced the IEP driver to support the legacy 32-bit timestamp
>>> conversion to 64-bit timestamp by using 2 fields as below:
>>> - 32-bit HW timestamp from SOF event in ns
>>> - Seconds value maintained in driver.
>>> 
>>> Currently ordinary clock (OC) configuration has been validated with
>>> Linux ptp4l.
>>> 
>>> Signed-off-by: Roger Quadros <rogerq@...com>
>>> Signed-off-by: Andrew F. Davis <afd@...com>
>>> Signed-off-by: Basharath Hussain Khaja <basharath@...thit.com>
>>> Signed-off-by: Parvathi Pudi <parvathi@...thit.com>
>>> ---
>>>   drivers/net/ethernet/ti/icssg/icss_iep.c     | 155 ++++++++++++++++++-
>>>   drivers/net/ethernet/ti/icssg/icss_iep.h     |  12 ++
>>>   drivers/net/ethernet/ti/icssm/icssm_prueth.c |  56 ++++++-
>>>   drivers/net/ethernet/ti/icssm/icssm_prueth.h |  11 ++
>>>   4 files changed, 230 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c
>>> b/drivers/net/ethernet/ti/icssg/icss_iep.c
>>> index d0850722814e..85e27cc77a3b 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
>>> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
>>> @@ -14,12 +14,15 @@
>>>   #include <linux/of.h>
>>>   #include <linux/of_platform.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/timecounter.h>
>>> +#include <linux/clocksource.h>
>>>   #include <linux/timekeeping.h>
>>>   #include <linux/interrupt.h>
>>>   #include <linux/of_irq.h>
>>>   #include <linux/workqueue.h>
>>>   
>>>   #include "icss_iep.h"
>>> +#include "../icssm/icssm_prueth_ptp.h"
>>>   
>>>   #define IEP_MAX_DEF_INC		0xf
>>>   #define IEP_MAX_COMPEN_INC		0xfff
>>> @@ -53,6 +56,14 @@
>>>   #define IEP_CAP_CFG_CAPNR_1ST_EVENT_EN(n)	BIT(LATCH_INDEX(n))
>>>   #define IEP_CAP_CFG_CAP_ASYNC_EN(n)		BIT(LATCH_INDEX(n) + 10)
>>>   
>>> +#define IEP_TC_DEFAULT_SHIFT         28
>>> +#define IEP_TC_INCR5_MULT            BIT(28)
>>> +
>>> +/* Polling period - how often iep_overflow_check() is called */
>>> +#define IEP_OVERFLOW_CHECK_PERIOD_MS   50
>>> +
>>> +#define TIMESYNC_SECONDS_COUNT_SIZE    6
>>> +
>>>   /**
>>>    * icss_iep_get_count_hi() - Get the upper 32 bit IEP counter
>>>    * @iep: Pointer to structure representing IEP.
>>> @@ -87,6 +98,28 @@ int icss_iep_get_count_low(struct icss_iep *iep)
>>>   }
>>>   EXPORT_SYMBOL_GPL(icss_iep_get_count_low);
>>>   
>>> +static u64 icss_iep_get_count32(struct icss_iep *iep)
>>> +{
>>> +	void __iomem *sram = iep->sram;
>>> +	u64 v_sec = 0;
>>> +	u32 v_ns = 0;
>>> +	u64 v = 0;
>>> +
>>> +	v_ns = icss_iep_get_count_low(iep);
>>> +	memcpy_fromio(&v_sec, sram + TIMESYNC_SECONDS_COUNT_OFFSET,
>>> +		      TIMESYNC_SECONDS_COUNT_SIZE);
>>> +	v = (v_sec * NSEC_PER_SEC) + v_ns;
>> 
>> How can you be sure that the nanoseconds part does belong to the second
>> which was read afterwards? In other words, what is the protection for
>> the sutiation when an overflow happened right after you read ns but
>> before reading of seconds?
>> And another question - you copy 6 bytes of seconds counter directly into
>> the memory. How will it deal with different endianess?
>> 
> 
> We are analyzing further to check the possibility of the race condition.
> We will review and address this in next version.
> 

We have analyzed and added extra handling to address the concern. We will
post the updated patches shortly.

PRU-ICSS operates only in little-endian byte ordering.

>>> +
>>> +	return v;
>>> +}
>>> +
>>> +static u64 icss_iep_cc_read(const struct cyclecounter *cc)
>>> +{
>>> +	struct icss_iep *iep = container_of(cc, struct icss_iep, cc);
>>> +
>>> +	return icss_iep_get_count32(iep);
>>> +}
>>> +
>>>   /**
>>>    * icss_iep_get_ptp_clock_idx() - Get PTP clock index using IEP driver
>>>    * @iep: Pointer to structure representing IEP.
>>> @@ -280,6 +313,78 @@ static void icss_iep_set_slow_compensation_count(struct
>>> icss_iep *iep,
>>>   	regmap_write(iep->map, ICSS_IEP_SLOW_COMPEN_REG, compen_count);
>>>   }
>>>   
>>> +/* PTP PHC operations */
>>> +static int icss_iep_ptp_adjfine_v1(struct ptp_clock_info *ptp, long scaled_ppm)
>>> +{
>>> +	struct icss_iep *iep = container_of(ptp, struct icss_iep, ptp_info);
>>> +	s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
>>> +	struct timespec64 ts;
>>> +	int neg_adj = 0;
>>> +	u32 diff, mult;
>>> +	u64 adj;
>>> +
>>> +	mutex_lock(&iep->ptp_clk_mutex);
>>> +
>>> +	if (ppb < 0) {
>>> +		neg_adj = 1;
>>> +		ppb = -ppb;
>>> +	}
>>> +	mult = iep->cc_mult;
>>> +	adj = mult;
>>> +	adj *= ppb;
>>> +	diff = div_u64(adj, 1000000000ULL);
>>> +
>>> +	ts = ns_to_timespec64(timecounter_read(&iep->tc));
>>> +	pr_debug("iep ptp adjfine check at %lld.%09lu\n", ts.tv_sec,
>>> +		 ts.tv_nsec);
>>> +
>>> +	iep->cc.mult = neg_adj ? mult - diff : mult + diff;
>>> +
>>> +	mutex_unlock(&iep->ptp_clk_mutex);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int icss_iep_ptp_adjtime_v1(struct ptp_clock_info *ptp, s64 delta)
>>> +{
>>> +	struct icss_iep *iep = container_of(ptp, struct icss_iep, ptp_info);
>>> +
>>> +	mutex_lock(&iep->ptp_clk_mutex);
>>> +	timecounter_adjtime(&iep->tc, delta);
>>> +	mutex_unlock(&iep->ptp_clk_mutex);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int icss_iep_ptp_gettimeex_v1(struct ptp_clock_info *ptp,
>>> +				     struct timespec64 *ts,
>>> +				     struct ptp_system_timestamp *sts)
>>> +{
>>> +	struct icss_iep *iep = container_of(ptp, struct icss_iep, ptp_info);
>>> +	u64 ns;
>>> +
>>> +	mutex_lock(&iep->ptp_clk_mutex);
>>> +	ns = timecounter_read(&iep->tc);
>>> +	*ts = ns_to_timespec64(ns);
>>> +	mutex_unlock(&iep->ptp_clk_mutex);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int icss_iep_ptp_settime_v1(struct ptp_clock_info *ptp,
>>> +				   const struct timespec64 *ts)
>>> +{
>>> +	struct icss_iep *iep = container_of(ptp, struct icss_iep, ptp_info);
>>> +	u64 ns;
>>> +
>>> +	mutex_lock(&iep->ptp_clk_mutex);
>>> +	ns = timespec64_to_ns(ts);
>>> +	timecounter_init(&iep->tc, &iep->cc, ns);
>>> +	mutex_unlock(&iep->ptp_clk_mutex);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   /* PTP PHC operations */
>>>   static int icss_iep_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>>>   {
>>> @@ -669,6 +774,17 @@ static int icss_iep_ptp_enable(struct ptp_clock_info *ptp,
>>>   	return -EOPNOTSUPP;
>>>   }
>>>   
>>> +static long icss_iep_overflow_check(struct ptp_clock_info *ptp)
>>> +{
>>> +	struct icss_iep *iep = container_of(ptp, struct icss_iep, ptp_info);
>>> +	unsigned long delay = iep->ovfl_check_period;
>>> +	struct timespec64 ts;
>>> +
>>> +	ts = ns_to_timespec64(timecounter_read(&iep->tc));
>>> +
>>> +	pr_debug("iep overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
>>> +	return (long)delay;
>>> +}
>>>   static struct ptp_clock_info icss_iep_ptp_info = {
>>>   	.owner		= THIS_MODULE,
>>>   	.name		= "ICSS IEP timer",
>>> @@ -680,6 +796,18 @@ static struct ptp_clock_info icss_iep_ptp_info = {
>>>   	.enable		= icss_iep_ptp_enable,
>>>   };
>>>   
>>> +static struct ptp_clock_info icss_iep_ptp_info_v1 = {
>>> +	.owner		= THIS_MODULE,
>>> +	.name		= "ICSS IEP timer",
>>> +	.max_adj	= 10000000,
>>> +	.adjfine	= icss_iep_ptp_adjfine_v1,
>>> +	.adjtime	= icss_iep_ptp_adjtime_v1,
>>> +	.gettimex64	= icss_iep_ptp_gettimeex_v1,
>>> +	.settime64	= icss_iep_ptp_settime_v1,
>>> +	.enable		= icss_iep_ptp_enable,
>>> +	.do_aux_work	= icss_iep_overflow_check,
>>> +};
>>> +
>>>   struct icss_iep *icss_iep_get_idx(struct device_node *np, int idx)
>>>   {
>>>   	struct platform_device *pdev;
>>> @@ -701,6 +829,18 @@ struct icss_iep *icss_iep_get_idx(struct device_node *np,
>>> int idx)
>>>   	if (!iep)
>>>   		return ERR_PTR(-EPROBE_DEFER);
>>>   
>>> +	if (iep->plat_data->iep_rev == IEP_REV_V1_0) {
>>> +		iep->cc.shift = IEP_TC_DEFAULT_SHIFT;
>>> +		iep->cc.mult = IEP_TC_INCR5_MULT;
>>> +
>>> +		iep->cc.read = icss_iep_cc_read;
>>> +		iep->cc.mask = CLOCKSOURCE_MASK(64);
>>> +
>>> +		iep->ovfl_check_period =
>>> +			msecs_to_jiffies(IEP_OVERFLOW_CHECK_PERIOD_MS);
>>> +		iep->cc_mult = iep->cc.mult;
>>> +	}
>>> +
>>>   	device_lock(iep->dev);
>>>   	if (iep->client_np) {
>>>   		device_unlock(iep->dev);
>>> @@ -795,6 +935,10 @@ int icss_iep_init(struct icss_iep *iep, const struct
>>> icss_iep_clockops *clkops,
>>>   		icss_iep_enable(iep);
>>>   	icss_iep_settime(iep, ktime_get_real_ns());
>>>   
>>> +	if (iep->plat_data->iep_rev == IEP_REV_V1_0)
>>> +		timecounter_init(&iep->tc, &iep->cc,
>>> +				 ktime_to_ns(ktime_get_real()));
>>> +
>>>   	iep->ptp_clock = ptp_clock_register(&iep->ptp_info, iep->dev);
>>>   	if (IS_ERR(iep->ptp_clock)) {
>>>   		ret = PTR_ERR(iep->ptp_clock);
>>> @@ -802,6 +946,9 @@ int icss_iep_init(struct icss_iep *iep, const struct
>>> icss_iep_clockops *clkops,
>>>   		dev_err(iep->dev, "Failed to register ptp clk %d\n", ret);
>>>   	}
>>>   
>>> +	if (iep->plat_data->iep_rev == IEP_REV_V1_0)
>>> +		ptp_schedule_worker(iep->ptp_clock, iep->ovfl_check_period);
>>> +
>>>   	return ret;
>>>   }
>>>   EXPORT_SYMBOL_GPL(icss_iep_init);
>>> @@ -879,7 +1026,11 @@ static int icss_iep_probe(struct platform_device *pdev)
>>>   		return PTR_ERR(iep->map);
>>>   	}
>>>   
>>> -	iep->ptp_info = icss_iep_ptp_info;
>>> +	if (iep->plat_data->iep_rev == IEP_REV_V1_0)
>>> +		iep->ptp_info = icss_iep_ptp_info_v1;
>>> +	else
>>> +		iep->ptp_info = icss_iep_ptp_info;
>>> +
>>>   	mutex_init(&iep->ptp_clk_mutex);
>>>   	dev_set_drvdata(dev, iep);
>>>   	icss_iep_disable(iep);
>>> @@ -1004,6 +1155,7 @@ static const struct icss_iep_plat_data
>>> am57xx_icss_iep_plat_data = {
>>>   		[ICSS_IEP_SYNC_START_REG] = 0x19c,
>>>   	},
>>>   	.config = &am654_icss_iep_regmap_config,
>>> +	.iep_rev = IEP_REV_V2_1,
>>>   };
>>>   
>>>   static bool am335x_icss_iep_valid_reg(struct device *dev, unsigned int reg)
>>> @@ -1057,6 +1209,7 @@ static const struct icss_iep_plat_data
>>> am335x_icss_iep_plat_data = {
>>>   		[ICSS_IEP_SYNC_START_REG] = 0x11C,
>>>   	},
>>>   	.config = &am335x_icss_iep_regmap_config,
>>> +	.iep_rev = IEP_REV_V1_0,
>>>   };
>>>   
>>>   static const struct of_device_id icss_iep_of_match[] = {
>>> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.h
>>> b/drivers/net/ethernet/ti/icssg/icss_iep.h
>>> index 0bdca0155abd..f72f1ea9f3c9 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icss_iep.h
>>> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.h
>>> @@ -47,21 +47,29 @@ enum {
>>>   	ICSS_IEP_MAX_REGS,
>>>   };
>>>   
>>> +enum iep_revision {
>>> +	IEP_REV_V1_0 = 0,
>>> +	IEP_REV_V2_1
>>> +};
>>> +
>>>   /**
>>>    * struct icss_iep_plat_data - Plat data to handle SoC variants
>>>    * @config: Regmap configuration data
>>>    * @reg_offs: register offsets to capture offset differences across SoCs
>>>    * @flags: Flags to represent IEP properties
>>> + * @iep_rev: IEP revision identifier.
>>>    */
>>>   struct icss_iep_plat_data {
>>>   	const struct regmap_config *config;
>>>   	u32 reg_offs[ICSS_IEP_MAX_REGS];
>>>   	u32 flags;
>>> +	enum iep_revision iep_rev;
>>>   };
>>>   
>>>   struct icss_iep {
>>>   	struct device *dev;
>>>   	void __iomem *base;
>>> +	void __iomem *sram;
>>>   	const struct icss_iep_plat_data *plat_data;
>>>   	struct regmap *map;
>>>   	struct device_node *client_np;
>>> @@ -70,6 +78,10 @@ struct icss_iep {
>>>   	struct ptp_clock_info ptp_info;
>>>   	struct ptp_clock *ptp_clock;
>>>   	struct mutex ptp_clk_mutex;	/* PHC access serializer */
>>> +	u32 cc_mult; /* for the nominal frequency */
>>> +	struct cyclecounter cc;
>>> +	struct timecounter tc;
>>> +	unsigned long ovfl_check_period;
>>>   	u32 def_inc;
>>>   	s16 slow_cmp_inc;
>>>   	u32 slow_cmp_count;
>>> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>>> b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>>> index 67ee4c72d3d6..7e90f9e71921 100644
>>> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>>> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>>> @@ -39,6 +39,8 @@
>>>   #define TX_START_DELAY		0x40
>>>   #define TX_CLK_DELAY_100M	0x6
>>>   
>>> +#define TIMESYNC_SECONDS_BIT_MASK   0x0000ffffffffffff
>>> +
>>>   static struct prueth_fw_offsets fw_offsets_v2_1;
>>>   
>>>   static void icssm_prueth_set_fw_offsets(struct prueth *prueth)
>>> @@ -642,13 +644,49 @@ irqreturn_t icssm_prueth_ptp_tx_irq_handle(int irq, void
>>> *dev)
>>>   	return IRQ_HANDLED;
>>>   }
>>>   
>>> +/**
>>> + * icssm_iep_get_timestamp_cycles - IEP get timestamp
>>> + * @iep: icss_iep structure
>>> + * @mem: io memory address
>>> + *
>>> + * To convert the 10 byte timestamp from firmware
>>> + * i.e., nanoseconds part from 32-bit IEP counter(4 bytes)
>>> + * seconds part updated by firmware(rev FW_REV1_0) in SRAM
>>> + * (6 bytes) into 64-bit timestamp in ns
>>> + *
>>> + * Return: 64-bit converted timestamp
>>> + */
>>> +u64 icssm_iep_get_timestamp_cycles(struct icss_iep *iep,
>>> +				   void __iomem *mem)
>>> +{
>>> +	u64 cycles, cycles_sec = 0;
>>> +	u32 cycles_ns;
>>> +
>>> +	memcpy_fromio(&cycles_ns, mem, sizeof(cycles_ns));
>>> +	memcpy_fromio(&cycles_sec, mem + 4, sizeof(cycles_sec));
>> 
>> the same question is here - there is a possibility of overflow
>> between these 2 reads...
>> 
> 
> We are analyzing further to check the possibility of the race condition.
> We will review and address this in next version.
> 

We are reading the snapshot of timestamp from local memory.
The timestamp is captured by PRU-ICSS at a hardware level,
so overflow does not apply here.

 
Thanks and Regards,
Parvathi.

Powered by blists - more mailing lists