[PATCH net v2 0/2] Fixes for perout configuration in IEP driver

[PATCH net v2 0/2] Fixes for perout configuration in IEP driver

[Meghana Malladi]

IEP driver supports both pps and perout signal generation using testptp
application. Currently the driver is missing to incorporate the perout
signal configuration. This series introduces fixes in the driver to
configure perout signal based on the arguments passed by the perout
request.

v1: https://lore.kernel.org/all/20250211103527.923849-1-m-malladi@ti.com/

Meghana Malladi (2):
  net: ti: icss-iep: Fix pwidth configuration for perout signal
  net: ti: icss-iep: Fix phase offset configuration for perout signal

 drivers/net/ethernet/ti/icssg/icss_iep.c | 44 ++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)


base-commit: 071ed42cff4fcdd89025d966d48eabef59913bf2
-- 
2.43.0

[PATCH net v2 1/2] net: ti: icss-iep: Fix pwidth configuration for perout signal

[Meghana Malladi]

icss_iep_perout_enable_hw() is a common function for generating
both pps and perout signals. When enabling pps, the application needs
to only pass enable/disable argument, whereas for perout it supports
different flags to configure the signal.

But icss_iep_perout_enable_hw() function is missing to hook the
configuration params passed by the app, causing perout to behave
same a pps (except being able to configure the period). As duty cycle
is also one feature which can configured for perout, incorporate this
in the function to get the expected signal.

Fixes: c1e0230eeaab2 ("net: ti: icss-iep: Add IEP driver")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
Changes from v1 (v2-v1):
- replace u32 typecast with div_u64()

 drivers/net/ethernet/ti/icssg/icss_iep.c | 28 ++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index 768578c0d958..0411438a3b5d 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -406,9 +406,16 @@ static void icss_iep_update_to_next_boundary(struct icss_iep *iep, u64 start_ns)
 static int icss_iep_perout_enable_hw(struct icss_iep *iep,
 				     struct ptp_perout_request *req, int on)
 {
+	struct timespec64 ts;
+	u64 ns_width;
 	int ret;
 	u64 cmp;
 
+	/* Calculate width of the signal for PPS/PEROUT handling */
+	ts.tv_sec = req->on.sec;
+	ts.tv_nsec = req->on.nsec;
+	ns_width = timespec64_to_ns(&ts);
+
 	if (iep->ops && iep->ops->perout_enable) {
 		ret = iep->ops->perout_enable(iep->clockops_data, req, on, &cmp);
 		if (ret)
@@ -419,8 +426,9 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep,
 			regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp));
 			if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT)
 				regmap_write(iep->map, ICSS_IEP_CMP1_REG1, upper_32_bits(cmp));
-			/* Configure SYNC, 1ms pulse width */
-			regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, 1000000);
+			/* Configure SYNC, based on req on width */
+			regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG,
+				     div_u64(ns_width, iep->def_inc));
 			regmap_write(iep->map, ICSS_IEP_SYNC0_PERIOD_REG, 0);
 			regmap_write(iep->map, ICSS_IEP_SYNC_START_REG, 0);
 			regmap_write(iep->map, ICSS_IEP_SYNC_CTRL_REG, 0); /* one-shot mode */
@@ -447,6 +455,8 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep,
 				   + req->period.nsec;
 			icss_iep_update_to_next_boundary(iep, start_ns);
 
+			regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG,
+				     div_u64(ns_width, iep->def_inc));
 			/* Enable Sync in single shot mode  */
 			regmap_write(iep->map, ICSS_IEP_SYNC_CTRL_REG,
 				     IEP_SYNC_CTRL_SYNC_N_EN(0) | IEP_SYNC_CTRL_SYNC_EN);
@@ -478,6 +488,12 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
 
 	mutex_lock(&iep->ptp_clk_mutex);
 
+	/* Reject requests with unsupported flags */
+	if (req->flags & ~PTP_PEROUT_DUTY_CYCLE) {
+		ret = -EOPNOTSUPP;
+		goto exit;
+	}
+
 	if (iep->pps_enabled) {
 		ret = -EBUSY;
 		goto exit;
@@ -486,6 +502,12 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
 	if (iep->perout_enabled == !!on)
 		goto exit;
 
+	/* Set default "on" time (1ms) for the signal if not passed by the app */
+	if (!(req->flags & PTP_PEROUT_DUTY_CYCLE)) {
+		req->on.sec = 0;
+		req->on.nsec = NSEC_PER_MSEC;
+	}
+
 	ret = icss_iep_perout_enable_hw(iep, req, on);
 	if (!ret)
 		iep->perout_enabled = !!on;
@@ -572,6 +594,8 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
 		rq.perout.period.nsec = 0;
 		rq.perout.start.sec = ts.tv_sec + 2;
 		rq.perout.start.nsec = 0;
+		rq.perout.on.sec = 0;
+		rq.perout.on.nsec = NSEC_PER_MSEC;
 		ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
 	} else {
 		ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
-- 
2.43.0

[PATCH net v2 2/2] net: ti: icss-iep: Fix phase offset configuration for perout signal

[Meghana Malladi]

icss_iep_perout_enable_hw() is a common function for generating
both pps and perout signals. When enabling pps, the application needs
to only pass enable/disable argument, whereas for perout it supports
different flags to configure the signal.

In case the app passes a valid phase offset value, the signal should
start toggling after that phase offset, else start immediately or
as soon as possible. ICSS_IEP_SYNC_START_REG register take number of
clock cycles to wait before starting the signal after activation time.
Set appropriate value to this register to support phase offset.

Fixes: c1e0230eeaab2 ("net: ti: icss-iep: Add IEP driver")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
Changes from v1 (v2-v1):
- replace u32 typecast with div_u64()

 drivers/net/ethernet/ti/icssg/icss_iep.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index 0411438a3b5d..598dfb963db6 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -407,6 +407,7 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep,
 				     struct ptp_perout_request *req, int on)
 {
 	struct timespec64 ts;
+	u64 ns_start;
 	u64 ns_width;
 	int ret;
 	u64 cmp;
@@ -416,6 +417,14 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep,
 	ts.tv_nsec = req->on.nsec;
 	ns_width = timespec64_to_ns(&ts);
 
+	if (req->flags & PTP_PEROUT_PHASE) {
+		ts.tv_sec = req->phase.sec;
+		ts.tv_nsec = req->phase.nsec;
+		ns_start = timespec64_to_ns(&ts);
+	} else {
+		ns_start = 0;
+	}
+
 	if (iep->ops && iep->ops->perout_enable) {
 		ret = iep->ops->perout_enable(iep->clockops_data, req, on, &cmp);
 		if (ret)
@@ -430,7 +439,8 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep,
 			regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG,
 				     div_u64(ns_width, iep->def_inc));
 			regmap_write(iep->map, ICSS_IEP_SYNC0_PERIOD_REG, 0);
-			regmap_write(iep->map, ICSS_IEP_SYNC_START_REG, 0);
+			regmap_write(iep->map, ICSS_IEP_SYNC_START_REG,
+				     div_u64(ns_start, iep->def_inc));
 			regmap_write(iep->map, ICSS_IEP_SYNC_CTRL_REG, 0); /* one-shot mode */
 			/* Enable CMP 1 */
 			regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
@@ -457,6 +467,8 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep,
 
 			regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG,
 				     div_u64(ns_width, iep->def_inc));
+			regmap_write(iep->map, ICSS_IEP_SYNC_START_REG,
+				     div_u64(ns_start, iep->def_inc));
 			/* Enable Sync in single shot mode  */
 			regmap_write(iep->map, ICSS_IEP_SYNC_CTRL_REG,
 				     IEP_SYNC_CTRL_SYNC_N_EN(0) | IEP_SYNC_CTRL_SYNC_EN);
@@ -489,7 +501,8 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
 	mutex_lock(&iep->ptp_clk_mutex);
 
 	/* Reject requests with unsupported flags */
-	if (req->flags & ~PTP_PEROUT_DUTY_CYCLE) {
+	if (req->flags & ~(PTP_PEROUT_DUTY_CYCLE |
+			  PTP_PEROUT_PHASE)) {
 		ret = -EOPNOTSUPP;
 		goto exit;
 	}
@@ -590,6 +603,7 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
 	if (on) {
 		ns = icss_iep_gettime(iep, NULL);
 		ts = ns_to_timespec64(ns);
+		rq.perout.flags = 0;
 		rq.perout.period.sec = 1;
 		rq.perout.period.nsec = 0;
 		rq.perout.start.sec = ts.tv_sec + 2;
-- 
2.43.0

Re: [PATCH net v2 0/2] Fixes for perout configuration in IEP driver

[Jacob Keller]

On 2/18/2025 10:26 PM, Meghana Malladi wrote:
> IEP driver supports both pps and perout signal generation using testptp
> application. Currently the driver is missing to incorporate the perout
> signal configuration. This series introduces fixes in the driver to
> configure perout signal based on the arguments passed by the perout
> request.
> 

This could be interpreted as a feature implementation rather than a fix.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> v1: https://lore.kernel.org/all/20250211103527.923849-1-m-malladi@ti.com/
> 
> Meghana Malladi (2):
>   net: ti: icss-iep: Fix pwidth configuration for perout signal
>   net: ti: icss-iep: Fix phase offset configuration for perout signal
> 
>  drivers/net/ethernet/ti/icssg/icss_iep.c | 44 ++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> 
> base-commit: 071ed42cff4fcdd89025d966d48eabef59913bf2

Re: [PATCH net v2 0/2] Fixes for perout configuration in IEP driver

[Jakub Kicinski]

On Wed, 19 Feb 2025 15:37:16 -0800 Jacob Keller wrote:
> On 2/18/2025 10:26 PM, Meghana Malladi wrote:
> > IEP driver supports both pps and perout signal generation using testptp
> > application. Currently the driver is missing to incorporate the perout
> > signal configuration. This series introduces fixes in the driver to
> > configure perout signal based on the arguments passed by the perout
> > request.
> >   
> 
> This could be interpreted as a feature implementation rather than a fix.
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Agreed, ideally we should get a patch for net which rejects
all currently (as in - in Linus's tree) unsupported settings.
That would be a fix.

Then once that's merged add support for the new settings in net-next.

Hope that makes sense?
-- 
pw-bot: cr

Re: [PATCH net v2 0/2] Fixes for perout configuration in IEP driver

[Jacob Keller]

On 2/20/2025 5:24 PM, Jakub Kicinski wrote:
> On Wed, 19 Feb 2025 15:37:16 -0800 Jacob Keller wrote:
>> On 2/18/2025 10:26 PM, Meghana Malladi wrote:
>>> IEP driver supports both pps and perout signal generation using testptp
>>> application. Currently the driver is missing to incorporate the perout
>>> signal configuration. This series introduces fixes in the driver to
>>> configure perout signal based on the arguments passed by the perout
>>> request.
>>>   
>>
>> This could be interpreted as a feature implementation rather than a fix.
>>
>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Agreed, ideally we should get a patch for net which rejects
> all currently (as in - in Linus's tree) unsupported settings.
> That would be a fix.
> 
> Then once that's merged add support for the new settings in net-next.
> 
> Hope that makes sense?

+1 on this direction, its important that the driver does not accept
configuration which is incorrect.

Reminds me of my backlog to refactor this whole mess into a
supported_flags field in the PTP ops structure. Maybe it is again time
to revive that.

Thanks,
Jake

Re: [PATCH net v2 0/2] Fixes for perout configuration in IEP driver

[Malladi, Meghana]

On 2/21/2025 6:54 AM, Jakub Kicinski wrote:
> On Wed, 19 Feb 2025 15:37:16 -0800 Jacob Keller wrote:
>> On 2/18/2025 10:26 PM, Meghana Malladi wrote:
>>> IEP driver supports both pps and perout signal generation using testptp
>>> application. Currently the driver is missing to incorporate the perout
>>> signal configuration. This series introduces fixes in the driver to
>>> configure perout signal based on the arguments passed by the perout
>>> request.
>>>    
>>
>> This could be interpreted as a feature implementation rather than a fix.
>>
>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Agreed, ideally we should get a patch for net which rejects
> all currently (as in - in Linus's tree) unsupported settings.
> That would be a fix.
> 
> Then once that's merged add support for the new settings in net-next.
> 
> Hope that makes sense?

I do agree that this can be interpreted as a feature implementation (as 
the bug here is: missing perout driver implementation for which the 
driver claims it supports). I will post the updated patch series as 
suggested by Jakub.

Thanks,
Meghana Malladi.

Plan to validate supported flags in PTP core (Was: Re: [PATCH net v2 0/2] Fixes for perout configuration in IEP driver)

[Jacob Keller]

On 2/21/2025 2:22 PM, Jacob Keller wrote:
> 
> 
> On 2/20/2025 5:24 PM, Jakub Kicinski wrote:
>> On Wed, 19 Feb 2025 15:37:16 -0800 Jacob Keller wrote:
>>> On 2/18/2025 10:26 PM, Meghana Malladi wrote:
>>>> IEP driver supports both pps and perout signal generation using testptp
>>>> application. Currently the driver is missing to incorporate the perout
>>>> signal configuration. This series introduces fixes in the driver to
>>>> configure perout signal based on the arguments passed by the perout
>>>> request.
>>>>   
>>>
>>> This could be interpreted as a feature implementation rather than a fix.
>>>
>>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>>
>> Agreed, ideally we should get a patch for net which rejects
>> all currently (as in - in Linus's tree) unsupported settings.
>> That would be a fix.
>>
>> Then once that's merged add support for the new settings in net-next.
>>
>> Hope that makes sense?
> 
> +1 on this direction, its important that the driver does not accept
> configuration which is incorrect.
> 
> Reminds me of my backlog to refactor this whole mess into a
> supported_flags field in the PTP ops structure. Maybe it is again time
> to revive that.
> 
I've been looking into this the last week or so and I have what i think
is a workable plan, but I'd like to get some feedback before continuing.

I believe we ought to add .supported_extts_flags and
.supported_perout_flags to the ptp_info struct. These will get checked
by the core, to reject commands which set flags not specified here.

For PTP_EXTTS_REQUEST, the PTP_ENABLE_FEATURE flag is always accepted.

The behavior of PTP_RISING_EDGE and PTP_FALLING_EDGE is somewhat
complicated. I think the best way to handle it is to check the
PTP_STRICT_FLAGS. If the driver sets this, then assume they will
validate the flags properly and only enable strict matching modes. In
that case, check against all flags as.

If the driver does not set PTP_STRICT_FLAGS, (For example, drivers which
don't set anything because they forgot), we only allow ENABLE,
RISING_EDGE, and FALLING_EDGE, but not STRICT. This essentially disables
the v2 ioctl, and prevents users from getting strict behavior, (since
the driver did not say it would handle strict behavior!)

For periodic output, its easy, since all the flags are present only with
the v2 ioctl, so we can do a simple & ~supported_flags check.

The bigger question I have is.. what should we do about the existing
drivers which do not bother to check flags at all?

In my search I found these driver files set the n_ext_ts field to a
non-zero value but do not have any flag checks:

>      • drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
>      • drivers/net/ethernet/freescale/enetc/enetc_ptp.c
>      • drivers/net/ethernet/intel/i40e/i40e_ptp.c
>      • drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c
>      • drivers/net/ethernet/renesas/rtsn.c
>      • drivers/net/ethernet/renesas/rtsn.h
>      • drivers/net/ethernet/ti/am65-cpts.c
>      • drivers/net/ethernet/ti/cpts.h
>      • drivers/net/ethernet/ti/icssg/icss_iep.c
>      • drivers/net/ethernet/xscale/ptp_ixp46x.c
>      • drivers/net/phy/bcm-phy-ptp.c
>      • drivers/ptp/ptp_ocp.c
>      • drivers/ptp/ptp_pch.c
>      • drivers/ptp/ptp_qoriq.c

If a user sends the V2 ioctl, they'll happily ignore strict checks and
do who knows what.

With my changes applied, the core would now automatically reject the v2
ioctl.

I believe this is an improvement, but I'm not sure whether we should
just make this change with a net-next feature improvement.

Should I prepare a patch for net which updates the drivers to manually
check and reject requests with flags other than PTP_EXT_TS_V1_FLAGS
first, before sending my changes to the core?

Worse, there are a couple of drivers including one hardware in igb, the
renesas driver, and the lan743x driver which do some checks but which
ultimately don't properly honor the PTP_STRICT_FLAGS.

If I send these all together as separate patches, I end up with more
than 15 patches total, and thats before I've finished investigating the
PTP_PEROUT_REQUEST, which I believe is likely to have a similar number
of issues.

Would a series with individual patches for the 3 special cases + one
patch to handle all the drivers that have no explicit flag check be
acceptable? Or should I do individual patches for each driver and just
break the series up? Or are we ok with just fixing this in next with the
.supported_extts_flags change?

Thanks,
Jake

Re: Plan to validate supported flags in PTP core (Was: Re: [PATCH net v2 0/2] Fixes for perout configuration in IEP driver)

[Jakub Kicinski]

On Fri, 7 Mar 2025 15:48:27 -0800 Jacob Keller wrote:
> Would a series with individual patches for the 3 special cases + one
> patch to handle all the drivers that have no explicit flag check be
> acceptable? Or should I do individual patches for each driver and just
> break the series up? Or are we ok with just fixing this in next with the
> .supported_extts_flags change?

A mass rejection of unsupported settings feels like a net-next material
in general. Handling the more complex cases individually and the rest in
a big patch makes sense.

Re: Plan to validate supported flags in PTP core (Was: Re: [PATCH net v2 0/2] Fixes for perout configuration in IEP driver)

[Jacob Keller]

On 3/10/2025 9:25 AM, Jakub Kicinski wrote:
> On Fri, 7 Mar 2025 15:48:27 -0800 Jacob Keller wrote:
>> Would a series with individual patches for the 3 special cases + one
>> patch to handle all the drivers that have no explicit flag check be
>> acceptable? Or should I do individual patches for each driver and just
>> break the series up? Or are we ok with just fixing this in next with the
>> .supported_extts_flags change?
> 
> A mass rejection of unsupported settings feels like a net-next material
> in general. Handling the more complex cases individually and the rest in
> a big patch makes sense.

Alright. I think the three cases with drivers that check flags but do so
incorrectly are worth going to net. I'll send that series shortly, and
will follow up with the next series that introduces the supported flags
knobs afterwards.

Thanks,
Jake