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@xxxxxxxxx>
> >> ---
> >>  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.