Message-ID: <1528407284.1597726.1750858335213.JavaMail.zimbra@couthit.local>
Date: Wed, 25 Jun 2025 19:02:15 +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.

>> +
>> +	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.


Thanks and Regards,
Parvathi.

Powered by blists - more mailing lists