[PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[Buday Csaba]

The PPS channel selection was incorrectly implemented in the orginal
commit (see fixes). The sysfs interface uses a logical channel index,
and rejects channel numbers greater than zero (n_per_out is 1).
See: period_store() in drivers/ptp/ptp_sysfs.c

On the other hand, the FEC PTP driver was expecting the hardware
channel number, making the periodic output unusable from the sysfs
interface, with the exception of channel 0.

Fix the FEC PTP driver to match the logical channel number of the
sysfs interface.

Fixes: bf8ca67e2167 ("net: fec: refactor PPS channel configuration")

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 7f6b574320716..c1af81002b8fa 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -534,7 +534,7 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
 		if (rq->perout.flags)
 			return -EOPNOTSUPP;
 
-		if (rq->perout.index != fep->pps_channel)
+		if (rq->perout.index != 0)
 			return -EOPNOTSUPP;
 
 		period.tv_sec = rq->perout.period.sec;
-- 
2.39.5

Re: [PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>


> The PPS channel selection was incorrectly implemented in the orginal
                                                           ^^^^^^^
AI: Typo: "orginal" should be "original"

Frank

Re: [PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[Jakub Kicinski]

On Wed, 18 Mar 2026 16:56:15 +0100 Buday Csaba wrote:
> Fixes: bf8ca67e2167 ("net: fec: refactor PPS channel configuration")
> 
> Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>

No empty lines between tags please

RE: [PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[Wei Fang]

> Fixes: bf8ca67e2167 ("net: fec: refactor PPS channel configuration")
> 
> Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index 7f6b574320716..c1af81002b8fa 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -534,7 +534,7 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
>  		if (rq->perout.flags)
>  			return -EOPNOTSUPP;
> 
> -		if (rq->perout.index != fep->pps_channel)
> +		if (rq->perout.index != 0)
>  			return -EOPNOTSUPP;
> 
>  		period.tv_sec = rq->perout.period.sec;
> --
> 2.39.5
> 

I think the correct fix should update the n_per_out value like below.

@@ -756,7 +756,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
        fep->ptp_caps.max_adj = 250000000;
        fep->ptp_caps.n_alarm = 0;
        fep->ptp_caps.n_ext_ts = 0;
-       fep->ptp_caps.n_per_out = 1;
+       fep->ptp_caps.n_per_out = 4;

The commit 566c2d83887f ("net: fec: make PPS channel configurable") added
the property "fsl,pps-channel" to indicate which pulse channel is available. So
we should not fix the pulse output channel to 0.

RE: [PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[Buday Csaba]

>> Fixes: bf8ca67e2167 ("net: fec: refactor PPS channel configuration")
>> 
>> Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
>> ---
>>  drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>> b/drivers/net/ethernet/freescale/fec_ptp.c
>> index 7f6b574320716..c1af81002b8fa 100644
>> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>> @@ -534,7 +534,7 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
>>  		if (rq->perout.flags)
>>  			return -EOPNOTSUPP;
>> 
>> -		if (rq->perout.index != fep->pps_channel)
>> +		if (rq->perout.index != 0)
>>  			return -EOPNOTSUPP;
>> 
>>  		period.tv_sec = rq->perout.period.sec;
>> --
>> 2.39.5
>> 
> 
> I think the correct fix should update the n_per_out value like below.
> 
> @@ -756,7 +756,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>         fep->ptp_caps.max_adj = 250000000;
>         fep->ptp_caps.n_alarm = 0;
>         fep->ptp_caps.n_ext_ts = 0;
> -       fep->ptp_caps.n_per_out = 1;
> +       fep->ptp_caps.n_per_out = 4;
> 
> The commit 566c2d83887f ("net: fec: make PPS channel configurable") added
> the property "fsl,pps-channel" to indicate which pulse channel is available.
> So we should not fix the pulse output channel to 0.
> 

But is it wise to expose four channels, when only one is usable by this 
driver?

This patch does not affect the channel selection. That can be still made
through the DT. The driver uses `fep->pps_channel` everywhere else, while
rq->perout.index is only checked in this single line, and never used
anywhere else.

So the question is whether userspace is supposed to reference the hardware
channel number directly, or the mapping is meant to be left to the devicetree
and the driver internally.

I hope the netdev maintainers and/or Richard Cochran can clarify the
intended purpose of `n_per_out` and the expected channel indexing.

Re: [PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[Francesco Dolcini]

On Wed, Mar 18, 2026 at 04:56:15PM +0100, Buday Csaba wrote:
> The PPS channel selection was incorrectly implemented in the orginal
> commit (see fixes). The sysfs interface uses a logical channel index,
> and rejects channel numbers greater than zero (n_per_out is 1).
> See: period_store() in drivers/ptp/ptp_sysfs.c
> 
> On the other hand, the FEC PTP driver was expecting the hardware
> channel number, making the periodic output unusable from the sysfs
> interface, with the exception of channel 0.
> 
> Fix the FEC PTP driver to match the logical channel number of the
> sysfs interface.
> 
> Fixes: bf8ca67e2167 ("net: fec: refactor PPS channel configuration")

The commit you mention as fixes, it was just a refactor. The behavior
before/after that commit was supposed to be the same.

That commit never implemented "PPS channel selection", as you wrongly
state here.

Francesco

RE: [PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[Wei Fang]

> >> Fixes: bf8ca67e2167 ("net: fec: refactor PPS channel configuration")
> >>
> >> Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
> >> ---
> >>  drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> >> b/drivers/net/ethernet/freescale/fec_ptp.c
> >> index 7f6b574320716..c1af81002b8fa 100644
> >> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> >> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> >> @@ -534,7 +534,7 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
> >>  		if (rq->perout.flags)
> >>  			return -EOPNOTSUPP;
> >>
> >> -		if (rq->perout.index != fep->pps_channel)
> >> +		if (rq->perout.index != 0)
> >>  			return -EOPNOTSUPP;
> >>
> >>  		period.tv_sec = rq->perout.period.sec;
> >> --
> >> 2.39.5
> >>
> >
> > I think the correct fix should update the n_per_out value like below.
> >
> > @@ -756,7 +756,7 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
> >         fep->ptp_caps.max_adj = 250000000;
> >         fep->ptp_caps.n_alarm = 0;
> >         fep->ptp_caps.n_ext_ts = 0;
> > -       fep->ptp_caps.n_per_out = 1;
> > +       fep->ptp_caps.n_per_out = 4;
> >
> > The commit 566c2d83887f ("net: fec: make PPS channel configurable") added
> > the property "fsl,pps-channel" to indicate which pulse channel is available.
> > So we should not fix the pulse output channel to 0.
> >
> 
> But is it wise to expose four channels, when only one is usable by this
> driver?

It's fine to expose 4 channels, the driver can check which channel can output
pulses based on fep->pps_channel. And it will return " -EOPNOTSUPP " is the
channel specified by the command is unavailable.

> 
> This patch does not affect the channel selection. That can be still made
> through the DT. The driver uses `fep->pps_channel` everywhere else, while
> rq->perout.index is only checked in this single line, and never used
> anywhere else.

rq->perout.index indicates which channel is used to output the pulses, See:
https://elixir.bootlin.com/linux/v7.0-rc4/source/Documentation/ABI/testing/sysfs-ptp#L130

But this patch sets rq->perout.index to 0. However, the driver actually uses the
channel specified by fep->pps_channel, which is unreasonable.

> 
> So the question is whether userspace is supposed to reference the hardware
> channel number directly, or the mapping is meant to be left to the devicetree
> and the driver internally.

Based on my understanding of the period parameters in the kernel doc. I
believe that the user space specifies the hardware channel number directly.

> 
> I hope the netdev maintainers and/or Richard Cochran can clarify the
> intended purpose of `n_per_out` and the expected channel indexing.