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

quoted

quoted

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

Signed-off-by: Buday Csaba <redacted>
---
 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)

quoted

        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.