[PATCH net-next v3 0/3] net: fec: add PPS channel configuration

[PATCH net-next v3 0/3] net: fec: add PPS channel configuration

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Make the FEC Ethernet PPS channel configurable from device tree.

v3 to just add the missing "net-next" subject prefix, sorry about the spam, it
seems like friday morning plus the mid of august heat wave is badly affecting
myself ...

v2: https://lore.kernel.org/all/20240809091844.387824-1-francesco@dolcini.it/
v1: https://lore.kernel.org/all/20240807144349.297342-1-francesco@dolcini.it/

Francesco Dolcini (3):
  dt-bindings: net: fec: add pps channel property
  net: fec: refactor PPS channel configuration
  net: fec: make PPS channel configurable

 Documentation/devicetree/bindings/net/fsl,fec.yaml |  7 +++++++
 drivers/net/ethernet/freescale/fec_ptp.c           | 11 ++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.39.2

[PATCH net-next v3 1/3] dt-bindings: net: fec: add pps channel property

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Add fsl,pps-channel property to specify to which timer instance the PPS
channel is connected to.

Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v3: added net-next subject prefix
v2: no changes
---
 Documentation/devicetree/bindings/net/fsl,fec.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
index 5536c06139ca..24e863fdbdab 100644
--- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
+++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
@@ -183,6 +183,13 @@ properties:
     description:
       Register bits of stop mode control, the format is <&gpr req_gpr req_bit>.
 
+  fsl,pps-channel:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+    description:
+      Specifies to which timer instance the PPS signal is routed.
+    enum: [0, 1, 2, 3]
+
   mdio:
     $ref: mdio.yaml#
     unevaluatedProperties: false
-- 
2.39.2

[PATCH net-next v3 2/3] net: fec: refactor PPS channel configuration

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Preparation patch to allow for PPS channel configuration, no functional
change intended.

Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
v3: added net-next subject prefix
v2: add Reviewed-by
---
 drivers/net/ethernet/freescale/fec_ptp.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 2e4f3e1782a2..25f988b9c7cf 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -84,8 +84,7 @@
 #define FEC_CC_MULT	(1 << 31)
 #define FEC_COUNTER_PERIOD	(1 << 31)
 #define PPS_OUPUT_RELOAD_PERIOD	NSEC_PER_SEC
-#define FEC_CHANNLE_0		0
-#define DEFAULT_PPS_CHANNEL	FEC_CHANNLE_0
+#define DEFAULT_PPS_CHANNEL	0
 
 #define FEC_PTP_MAX_NSEC_PERIOD		4000000000ULL
 #define FEC_PTP_MAX_NSEC_COUNTER	0x80000000ULL
@@ -530,8 +529,9 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
 	unsigned long flags;
 	int ret = 0;
 
+	fep->pps_channel = DEFAULT_PPS_CHANNEL;
+
 	if (rq->type == PTP_CLK_REQ_PPS) {
-		fep->pps_channel = DEFAULT_PPS_CHANNEL;
 		fep->reload_period = PPS_OUPUT_RELOAD_PERIOD;
 
 		ret = fec_ptp_enable_pps(fep, on);
@@ -542,10 +542,9 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
 		if (rq->perout.flags)
 			return -EOPNOTSUPP;
 
-		if (rq->perout.index != DEFAULT_PPS_CHANNEL)
+		if (rq->perout.index != fep->pps_channel)
 			return -EOPNOTSUPP;
 
-		fep->pps_channel = DEFAULT_PPS_CHANNEL;
 		period.tv_sec = rq->perout.period.sec;
 		period.tv_nsec = rq->perout.period.nsec;
 		period_ns = timespec64_to_ns(&period);
-- 
2.39.2

[PATCH net-next v3 3/3] net: fec: make PPS channel configurable

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Depending on the SoC where the FEC is integrated into the PPS channel
might be routed to different timer instances. Make this configurable
from the devicetree.

When the related DT property is not present fallback to the previous
default and use channel 0.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Tested-by: Rafael Beims <rafael.beims@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v3: added net-next subject prefix
v2: add Reviewed|Tested-by
---
 drivers/net/ethernet/freescale/fec_ptp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 25f988b9c7cf..2e05083cbf29 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -529,8 +529,6 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
 	unsigned long flags;
 	int ret = 0;
 
-	fep->pps_channel = DEFAULT_PPS_CHANNEL;
-
 	if (rq->type == PTP_CLK_REQ_PPS) {
 		fep->reload_period = PPS_OUPUT_RELOAD_PERIOD;
 
@@ -712,12 +710,16 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct device_node *np = fep->pdev->dev.of_node;
 	int irq;
 	int ret;
 
 	fep->ptp_caps.owner = THIS_MODULE;
 	strscpy(fep->ptp_caps.name, "fec ptp", sizeof(fep->ptp_caps.name));
 
+	fep->pps_channel = DEFAULT_PPS_CHANNEL;
+	of_property_read_u32(np, "fsl,pps-channel", &fep->pps_channel);
+
 	fep->ptp_caps.max_adj = 250000000;
 	fep->ptp_caps.n_alarm = 0;
 	fep->ptp_caps.n_ext_ts = 0;
-- 
2.39.2

Re: [PATCH net-next v3 1/3] dt-bindings: net: fec: add pps channel property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

On Fri, Aug 09, 2024 at 11:48:02AM +0200, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Add fsl,pps-channel property to specify to which timer instance the PPS
> channel is connected to.

In the driver patch you say "depending on the soc ... might be routed to
different timer instances", why is a soc-specific compatible
insufficient to determine which timer instance is in use?
I think I know what you mean, but I'm not 100%.

That said, the explanation in the driver patch is better than the one
here, so a commit message improvement is required.

Cheers,
Conor.

> 
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v3: added net-next subject prefix
> v2: no changes
> ---
>  Documentation/devicetree/bindings/net/fsl,fec.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> index 5536c06139ca..24e863fdbdab 100644
> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> @@ -183,6 +183,13 @@ properties:
>      description:
>        Register bits of stop mode control, the format is <&gpr req_gpr req_bit>.
>  
> +  fsl,pps-channel:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0
> +    description:
> +      Specifies to which timer instance the PPS signal is routed.
> +    enum: [0, 1, 2, 3]
> +
>    mdio:
>      $ref: mdio.yaml#
>      unevaluatedProperties: false
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v3 1/3] dt-bindings: net: fec: add pps channel property

[Francesco Dolcini]

On Fri, Aug 09, 2024 at 03:27:39PM +0100, Conor Dooley wrote:
> On Fri, Aug 09, 2024 at 11:48:02AM +0200, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Add fsl,pps-channel property to specify to which timer instance the PPS
> > channel is connected to.
> 
> In the driver patch you say "depending on the soc ... might be routed to
> different timer instances", why is a soc-specific compatible
> insufficient to determine which timer instance is in use?
> I think I know what you mean, but I'm not 100%.
> 
> That said, the explanation in the driver patch is better than the one
> here, so a commit message improvement is required.

This was clarified by NXP during the discussion on this series [1] and the
commit messages were not amended to take this new information into
account, my fault.

I would propose something like this here:

```
Add fsl,pps-channel property to select where to connect the PPS
signal. This depends on the internal SoC routing and on the board, for
example on the i.MX8 SoC it can be connected to an external pin (using
channel 1) or to internal eDMA as DMA request (channel 0).
```

Francesco

[1] https://lore.kernel.org/all/ZrPYOWA3FESx197L@lizhi-Precision-Tower-5810/

Re: [PATCH net-next v3 1/3] dt-bindings: net: fec: add pps channel property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

On Fri, Aug 09, 2024 at 04:49:14PM +0200, Francesco Dolcini wrote:
> On Fri, Aug 09, 2024 at 03:27:39PM +0100, Conor Dooley wrote:
> > On Fri, Aug 09, 2024 at 11:48:02AM +0200, Francesco Dolcini wrote:
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > Add fsl,pps-channel property to specify to which timer instance the PPS
> > > channel is connected to.
> > 
> > In the driver patch you say "depending on the soc ... might be routed to
> > different timer instances", why is a soc-specific compatible
> > insufficient to determine which timer instance is in use?
> > I think I know what you mean, but I'm not 100%.
> > 
> > That said, the explanation in the driver patch is better than the one
> > here, so a commit message improvement is required.
> 
> This was clarified by NXP during the discussion on this series [1] and the
> commit messages were not amended to take this new information into
> account, my fault.
> 
> I would propose something like this here:
> 
> ```
> Add fsl,pps-channel property to select where to connect the PPS
> signal. This depends on the internal SoC routing and on the board, for
> example on the i.MX8 SoC it can be connected to an external pin (using
> channel 1) or to internal eDMA as DMA request (channel 0).
> ```

That's definitely better, as it illustrates why the compatible is not
sufficient.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH net-next v3 3/3] net: fec: make PPS channel configurable

[Wei Fang]

> -----Original Message-----
> From: Francesco Dolcini <francesco@dolcini.it>
> Sent: 2024年8月9日 17:48
> To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang
> <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; David S.
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> Richard Cochran <richardcochran@gmail.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>; imx@lists.linux.dev;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Frank Li
> <frank.li@nxp.com>; Rafael Beims <rafael.beims@toradex.com>
> Subject: [PATCH net-next v3 3/3] net: fec: make PPS channel configurable
> 
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Depending on the SoC where the FEC is integrated into the PPS channel might
> be routed to different timer instances. Make this configurable from the
> devicetree.
> 
> When the related DT property is not present fallback to the previous default
> and use channel 0.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Tested-by: Rafael Beims <rafael.beims@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v3: added net-next subject prefix
> v2: add Reviewed|Tested-by
> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index 25f988b9c7cf..2e05083cbf29 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -529,8 +529,6 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
>  	unsigned long flags;
>  	int ret = 0;
> 
> -	fep->pps_channel = DEFAULT_PPS_CHANNEL;
> -
>  	if (rq->type == PTP_CLK_REQ_PPS) {
>  		fep->reload_period = PPS_OUPUT_RELOAD_PERIOD;
> 
> @@ -712,12 +710,16 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> +	struct device_node *np = fep->pdev->dev.of_node;
nit
struct device_node *np = pdev->dev.of_node;

>  	int irq;
>  	int ret;
> 
>  	fep->ptp_caps.owner = THIS_MODULE;
>  	strscpy(fep->ptp_caps.name, "fec ptp", sizeof(fep->ptp_caps.name));
> 
> +	fep->pps_channel = DEFAULT_PPS_CHANNEL;
> +	of_property_read_u32(np, "fsl,pps-channel", &fep->pps_channel);
> +
>  	fep->ptp_caps.max_adj = 250000000;
>  	fep->ptp_caps.n_alarm = 0;
>  	fep->ptp_caps.n_ext_ts = 0;
> --
> 2.39.2

Re: [PATCH net-next v3 0/3] net: fec: add PPS channel configuration

[Csókás Bence]

Hi!
I already proposed this 2 years ago. I was just about to resubmit it and 
found this series.

Link: 
https://lore.kernel.org/netdev/20220803112449.37309-1-csokas.bence@prolan.hu/

What's the status of this? Also, please Cc: me in further 
conversations/revisions as well.

Reviewed-by: "Csókás, Bence" <csokas.bence@prolan.hu>

Thanks,
Bence

On 8/9/24 11:48, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Make the FEC Ethernet PPS channel configurable from device tree.
> 
> v3 to just add the missing "net-next" subject prefix, sorry about the spam, it
> seems like friday morning plus the mid of august heat wave is badly affecting
> myself ...
> 
> v2: https://lore.kernel.org/all/20240809091844.387824-1-francesco@dolcini.it/
> v1: https://lore.kernel.org/all/20240807144349.297342-1-francesco@dolcini.it/
> 
> Francesco Dolcini (3):
>    dt-bindings: net: fec: add pps channel property
>    net: fec: refactor PPS channel configuration
>    net: fec: make PPS channel configurable
> 
>   Documentation/devicetree/bindings/net/fsl,fec.yaml |  7 +++++++
>   drivers/net/ethernet/freescale/fec_ptp.c           | 11 ++++++-----
>   2 files changed, 13 insertions(+), 5 deletions(-)
> 

Re: [PATCH net-next v3 0/3] net: fec: add PPS channel configuration

[Francesco Dolcini]

Hello,

On Tue, Sep 03, 2024 at 04:10:28PM +0200, Csókás Bence wrote:
> What's the status of this? Also, please Cc: me in further
> conversations/revisions as well.

I am going to send a v4 in the next few days to address the comments
on the dt-bindings change and apart of that I hope is good to go.

Francesco

Re: [PATCH net-next v3 0/3] net: fec: add PPS channel configuration

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On 03.09.2024 18:07:00, Francesco Dolcini wrote:
> On Tue, Sep 03, 2024 at 04:10:28PM +0200, Csókás Bence wrote:
> > What's the status of this? Also, please Cc: me in further
> > conversations/revisions as well.
> 
> I am going to send a v4 in the next few days to address the comments
> on the dt-bindings change and apart of that I hope is good to go.

Have you read Richard's feedback on this?

| https://lore.kernel.org/all/YvLJJkV2GRJWl7tA@hoboy.vegasvil.org

There seems to be a standard interface for what you're trying to do,
right?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next v3 0/3] net: fec: add PPS channel configuration

[Csókás Bence]

Hi!

On 9/3/24 18:57, Marc Kleine-Budde wrote:
> On 03.09.2024 18:07:00, Francesco Dolcini wrote:
>> On Tue, Sep 03, 2024 at 04:10:28PM +0200, Csókás Bence wrote:
>>> What's the status of this? Also, please Cc: me in further
>>> conversations/revisions as well.
>>
>> I am going to send a v4 in the next few days to address the comments
>> on the dt-bindings change and apart of that I hope is good to go.
> 
> Have you read Richard's feedback on this?
> 
> | https://lore.kernel.org/all/YvLJJkV2GRJWl7tA@hoboy.vegasvil.org
> 
> There seems to be a standard interface for what you're trying to do,
> right?

Yes, he was opposing the sysfs knobs I added, which is not in this patch 
series. Back then, I got moved to another project so I didn't have the 
time to address it. But once this series is merged, we can work on 
run-time output switching using the API.

> regards,
> Marc

Bence