Message-ID: <20250714135641.uwe3jlcv7hcjyep2@skbuf>
Date: Mon, 14 Jul 2025 16:56:41 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Wei Fang <wei.fang@....com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, "F.S. Peng" <fushi.peng@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"richardcochran@...il.com" <richardcochran@...il.com>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Clark Wang <xiaoning.wang@....com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>
Subject: Re: [PATCH net-next 01/12] dt-bindings: ptp: add bindings for NETC
 Timer

On Mon, Jul 14, 2025 at 04:22:51PM +0300, Wei Fang wrote:
> > On Mon, Jul 14, 2025 at 01:43:49PM +0300, Wei Fang wrote:
> > > > On Mon, Jul 14, 2025 at 01:28:04PM +0300, Wei Fang wrote:
> > > > > I do not understand, the property is to indicate which pin the board is
> > > > > used to out PPS signal, as I said earlier, these pins are multiplexed with
> > > > > other devices, so different board design may use different pins to out
> > > > > this PPS signal.
> > > >
> > > > Did you look at the 'pins' API in ptp, as used by other drivers, to set
> > > > a function per pin?
> > >
> > > ptp_set_pinfunc()?
> > 
> > You're in the right area, but ptp_set_pinfunc() is an internal function.
> > I was specifically referring to struct ptp_clock_info :: pin_config, the
> > verify() function, etc.
> 
> I don't think these can meet customer's requirement, the PPS pin depends
> on the board design. If I understand correctly, these can only indicate
> whether the specified pin index is in range, or whether the pin is already
> occupied by another PTP function.
> 
> However, these pins are multiplexed with other devices, such as FLEXIO,
> CAN, etc. If the board is designed to assign this pin to other devices, then
> this pin cannot output the PPS signal. For for this use case, we need to
> specify a PPS pin which can output PPS signal.

Ok, apologies if I misunderstood the purpose of this device tree property
as affecting the function of the NETC 1588 timer IP pins. You gave me
this impression because I followed the code and I saw that "nxp,pps-channel"
is used to select in the PTP driver which FIPER block gets configured to
emit PPS. And I commented that maybe you don't need "nxp,pps-channel" at all,
because:
- PTP_CLK_REQ_PPS doesn't do what you think it does
- PTP_CLK_REQ_PEROUT does use the pin API to describe that one of the
  1588 timer block's pins can be used for the periodic output function

You seem to imply that the "nxp,pps-channel" property affects the
function of the SoC pads, which may be connected to the NETC 1588 timer
block or to some other IP. Nothing in the code I saw suggested this
would be the case, and I still don't see how this is the case - but
anyway, my bad.

In this case, echoing Krzysztof's comments: How come it isn't the system
pinmux driver the one concerned with connecting the SoC pads to the NETC
1588 timer or to FlexIO, CAN etc? The pinmux driver controls the pads,
the NETC timer controls its block's pins, regardless of how they are
routed further.

(reductio ad absurdum) Among the other devices which are muxed to these
SoC pads, why is the NETC 1588 timer responsible of controlling the
muxing, and not FlexIO or CAN? Something is illogical.

> > > > > The PPS interface (echo x > /sys/class/ptp/ptp0/pps_enable) provided
> > > > > by the current PTP framework only supports enabling or disabling the
> > > > > PPS signal. This is obviously limited for PTP devices with multiple
> > channels.
> > > >
> > > > For what we call "PPS" I think you should be looking at the periodic
> > > > output (perout) function. "PPS" is to emit events towards the local
> > > > system.
> > >
> > > The driver supports both PPS and PEROUT.
> > 
> > Ok, I noticed patch 3 but missed patch 4. Anyway, the role of
> > PTP_CLK_REQ_PPS is to emit events which can be monitored on the
> > /dev/ppsN char device. It shouldn't have anything to do with external
> > pins.
> 
> Is there a doc to stating that PTP_CLK_REQ_PPS is not used for external
> pins? As far as I know, some customers/users use PTP_CLK_REQ_PPS to
> output PPS signal to sync with other timer devices, for example, a similar
> property "fsl,pps-channel" was added to fec.yaml by others before.

I'm not sure that what you're asking for is realistic, i.e. an explicit
mention in the documentation that PTP_CLK_REQ_PPS does not affect the
state of external pins. There's an infinity of things that PTP_CLK_REQ_PPS
doesn't affect, it doesn't mean they will all be explicitly documented.

That being said, here's what Documentation/ABI/testing/sysfs-ptp says,
hence my interpretation that they're unrelated:

 97 What:»  »       /sys/class/ptp/ptp<N>/pps_available
 98 Date:»  »       September 2010
 99 Contact:»       Richard Cochran <richardcochran@...il.com>
100 Description:
101 »       »       This file indicates whether the PTP hardware clock
102 »       »       supports a Pulse Per Second to the host CPU. Reading
103 »       »       "1" means that the PPS is supported, while "0" means
104 »       »       not supported.

135 What:»  »       /sys/class/ptp/ptp<N>/pps_enable
136 Date:»  »       September 2010
137 Contact:»       Richard Cochran <richardcochran@...il.com>
138 Description:
139 »       »       This write-only file enables or disables delivery of
140 »       »       PPS events to the Linux PPS subsystem. To enable PPS
141 »       »       events, write a "1" into the file. To disable events,
142 »       »       write a "0" into the file.

Search for which user space programs interact with /dev/ppsN and you'll
see chrony, ntpd, even phc2sys. None of those consciously affects the
state of external pins when they use the Linux PPS API
(https://docs.kernel.org/driver-api/pps.html).

Maybe the PTP maintainer can give you an explicit ACK that PPS is not
supposed to affect pin states, if that helps.

As usual, bad practice in old drivers does not justify bad practice in
new drivers.

Powered by blists - more mailing lists