[PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[Csókás, Bence]

This function is used in `fec_ptp_enable_pps()` through
struct cyclecounter read(). Forward declarations make
it clearer, what's happening.

Fixes: 61d5e2a251fb ("fec: Fix timer capture timing in `fec_ptp_enable_pps()`")
Suggested-by: Frank Li <Frank.li@nxp.com>
Link: https://lore.kernel.org/netdev/20240805144754.2384663-1-csokas.bence@prolan.hu/T/#ma6c21ad264016c24612048b1483769eaff8cdf20
Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index e32f6724f568..fdbf61069a05 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -90,6 +90,8 @@
 #define FEC_PTP_MAX_NSEC_PERIOD		4000000000ULL
 #define FEC_PTP_MAX_NSEC_COUNTER	0x80000000ULL
 
+static u64 fec_ptp_read(const struct cyclecounter *cc);
+
 /**
  * fec_ptp_enable_pps
  * @fep: the fec_enet_private structure handle
@@ -136,7 +138,7 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
 		 * NSEC_PER_SEC - ts.tv_nsec. Add the remaining nanoseconds
 		 * to current timer would be next second.
 		 */
-		tempval = fep->cc.read(&fep->cc);
+		tempval = fec_ptp_read(&fep->cc);
 		/* Convert the ptp local counter to 1588 timestamp */
 		ns = timecounter_cyc2time(&fep->tc, tempval);
 		ts = ns_to_timespec64(ns);
-- 
2.34.1

[PATCH resubmit net 2/2] net: fec: Remove duplicated code

[Csókás, Bence]

`fec_ptp_pps_perout()` reimplements logic already
in `fec_ptp_read()`. Replace with function call.

Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index fdbf61069a05..91b0804142e4 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -213,13 +213,7 @@ static int fec_ptp_pps_perout(struct fec_enet_private *fep)
 	timecounter_read(&fep->tc);
 
 	/* Get the current ptp hardware time counter */
-	temp_val = readl(fep->hwp + FEC_ATIME_CTRL);
-	temp_val |= FEC_T_CTRL_CAPTURE;
-	writel(temp_val, fep->hwp + FEC_ATIME_CTRL);
-	if (fep->quirks & FEC_QUIRK_BUG_CAPTURE)
-		udelay(1);
-
-	ptp_hc = readl(fep->hwp + FEC_ATIME);
+	ptp_hc = fec_ptp_read(&fep->cc);
 
 	/* Convert the ptp local counter to 1588 timestamp */
 	curr_time = timecounter_cyc2time(&fep->tc, ptp_hc);
-- 
2.34.1

Re: [PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[Csókás Bence]

Aw, crop, I meant to say -v2 instead of "resubmit"... That's what 
happens if you mindlessly re-use format-patch commands :/ I hope it 
doesn't bother you _too_ much...

On 8/7/24 10:29, Csókás, Bence wrote:
> This function is used in `fec_ptp_enable_pps()` through
> struct cyclecounter read(). Forward declarations make
> it clearer, what's happening.
> 
> Fixes: 61d5e2a251fb ("fec: Fix timer capture timing in `fec_ptp_enable_pps()`")
> Suggested-by: Frank Li <Frank.li@nxp.com>
> Link: https://lore.kernel.org/netdev/20240805144754.2384663-1-csokas.bence@prolan.hu/T/#ma6c21ad264016c24612048b1483769eaff8cdf20
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>

Re: [PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[Andrew Lunn]

On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote:
> This function is used in `fec_ptp_enable_pps()` through
> struct cyclecounter read(). Forward declarations make
> it clearer, what's happening.

In general, forward declarations are not liked. It is better to move
the code to before it is used.

Since this is a minimal fix for stable, lets allow it. But please wait
for net to be merged into net-next, and submit a cleanup patch which
does move fec_ptp_read() earlier and remove the forward declaration.

     Andrew

Re: [PATCH resubmit net 2/2] net: fec: Remove duplicated code

[Andrew Lunn]

On Wed, Aug 07, 2024 at 10:29:19AM +0200, Csókás, Bence wrote:
> `fec_ptp_pps_perout()` reimplements logic already
> in `fec_ptp_read()`. Replace with function call.

This is a cleanup, not a fix. So please target net-next, not net.

    Andrew

---
pw-bot: cr

Re: [PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[Frank Li]

On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote:
> This function is used in `fec_ptp_enable_pps()` through
> struct cyclecounter read(). Forward declarations make
> it clearer, what's happening.
>
> Fixes: 61d5e2a251fb ("fec: Fix timer capture timing in `fec_ptp_enable_pps()`")
> Suggested-by: Frank Li <Frank.li@nxp.com>
> Link: https://lore.kernel.org/netdev/20240805144754.2384663-1-csokas.bence@prolan.hu/T/#ma6c21ad264016c24612048b1483769eaff8cdf20
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index e32f6724f568..fdbf61069a05 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -90,6 +90,8 @@
>  #define FEC_PTP_MAX_NSEC_PERIOD		4000000000ULL
>  #define FEC_PTP_MAX_NSEC_COUNTER	0x80000000ULL
>
> +static u64 fec_ptp_read(const struct cyclecounter *cc);
> +
>  /**
>   * fec_ptp_enable_pps
>   * @fep: the fec_enet_private structure handle
> @@ -136,7 +138,7 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
>  		 * NSEC_PER_SEC - ts.tv_nsec. Add the remaining nanoseconds
>  		 * to current timer would be next second.
>  		 */
> -		tempval = fep->cc.read(&fep->cc);
> +		tempval = fec_ptp_read(&fep->cc);
>  		/* Convert the ptp local counter to 1588 timestamp */
>  		ns = timecounter_cyc2time(&fep->tc, tempval);
>  		ts = ns_to_timespec64(ns);
> --
> 2.34.1
>
>

Re: [PATCH resubmit net 2/2] net: fec: Remove duplicated code

[Frank Li]

On Wed, Aug 07, 2024 at 10:29:19AM +0200, Csókás, Bence wrote:
> `fec_ptp_pps_perout()` reimplements logic already
> in `fec_ptp_read()`. Replace with function call.
>
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index fdbf61069a05..91b0804142e4 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -213,13 +213,7 @@ static int fec_ptp_pps_perout(struct fec_enet_private *fep)
>  	timecounter_read(&fep->tc);
>
>  	/* Get the current ptp hardware time counter */
> -	temp_val = readl(fep->hwp + FEC_ATIME_CTRL);
> -	temp_val |= FEC_T_CTRL_CAPTURE;
> -	writel(temp_val, fep->hwp + FEC_ATIME_CTRL);
> -	if (fep->quirks & FEC_QUIRK_BUG_CAPTURE)
> -		udelay(1);
> -
> -	ptp_hc = readl(fep->hwp + FEC_ATIME);
> +	ptp_hc = fec_ptp_read(&fep->cc);
>
>  	/* Convert the ptp local counter to 1588 timestamp */
>  	curr_time = timecounter_cyc2time(&fep->tc, ptp_hc);
> --
> 2.34.1
>
>

Re: [PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[Simon Horman]

On Wed, Aug 07, 2024 at 03:53:17PM +0200, Andrew Lunn wrote:
> On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote:
> > This function is used in `fec_ptp_enable_pps()` through
> > struct cyclecounter read(). Forward declarations make
> > it clearer, what's happening.
> 
> In general, forward declarations are not liked. It is better to move
> the code to before it is used.
> 
> Since this is a minimal fix for stable, lets allow it. But please wait
> for net to be merged into net-next, and submit a cleanup patch which
> does move fec_ptp_read() earlier and remove the forward declaration.

That makes sense.

However, is this a fix?
It's not clear to me that it is.

And if it is a pre-requisite for patch 2/2,
well that doesn't seem to be a fix.

So in all, I'm somewhat confused.
And wonder if all changes can go via net-next.

Re: [PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[Csókás Bence]

On 8/8/24 11:41, Simon Horman wrote:
> On Wed, Aug 07, 2024 at 03:53:17PM +0200, Andrew Lunn wrote:
>> On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote:
>>> This function is used in `fec_ptp_enable_pps()` through
>>> struct cyclecounter read(). Forward declarations make
>>> it clearer, what's happening.
>>
>> In general, forward declarations are not liked. It is better to move
>> the code to before it is used.
>>
>> Since this is a minimal fix for stable, lets allow it. But please wait
>> for net to be merged into net-next, and submit a cleanup patch which
>> does move fec_ptp_read() earlier and remove the forward declaration.
> 
> That makes sense.
> 
> However, is this a fix?
> It's not clear to me that it is.

Well, it's not clear to me either what constitutes as a "fix" versus 
"just a cleanup". But, whatever floats Andrew's boat...

> And if it is a pre-requisite for patch 2/2,
> well that doesn't seem to be a fix.

It indeed is.

> So in all, I'm somewhat confused.
> And wonder if all changes can go via net-next.

That's probably what will be happening.

Bence

Re: [PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[Simon Horman]

On Thu, Aug 08, 2024 at 11:49:29AM +0200, Csókás Bence wrote:
> On 8/8/24 11:41, Simon Horman wrote:
> > On Wed, Aug 07, 2024 at 03:53:17PM +0200, Andrew Lunn wrote:
> > > On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote:
> > > > This function is used in `fec_ptp_enable_pps()` through
> > > > struct cyclecounter read(). Forward declarations make
> > > > it clearer, what's happening.
> > > 
> > > In general, forward declarations are not liked. It is better to move
> > > the code to before it is used.
> > > 
> > > Since this is a minimal fix for stable, lets allow it. But please wait
> > > for net to be merged into net-next, and submit a cleanup patch which
> > > does move fec_ptp_read() earlier and remove the forward declaration.
> > 
> > That makes sense.
> > 
> > However, is this a fix?
> > It's not clear to me that it is.
> 
> Well, it's not clear to me either what constitutes as a "fix" versus "just a
> cleanup". But, whatever floats Andrew's boat...

Let me state my rule of thumb: a fix addresses a user-visible bug.

> > And if it is a pre-requisite for patch 2/2,
> > well that doesn't seem to be a fix.
> 
> It indeed is.
> 
> > So in all, I'm somewhat confused.
> > And wonder if all changes can go via net-next.
> 
> That's probably what will be happening.

It does seem like the cleanest, and coincidently easiest, path.

Re: [PATCH resubmit net 1/2] net: fec: Forward-declare `fec_ptp_read()`

[Andrew Lunn]

On Thu, Aug 08, 2024 at 12:37:41PM +0100, Simon Horman wrote:
> On Thu, Aug 08, 2024 at 11:49:29AM +0200, Csókás Bence wrote:
> > On 8/8/24 11:41, Simon Horman wrote:
> > > On Wed, Aug 07, 2024 at 03:53:17PM +0200, Andrew Lunn wrote:
> > > > On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote:
> > > > > This function is used in `fec_ptp_enable_pps()` through
> > > > > struct cyclecounter read(). Forward declarations make
> > > > > it clearer, what's happening.
> > > > 
> > > > In general, forward declarations are not liked. It is better to move
> > > > the code to before it is used.
> > > > 
> > > > Since this is a minimal fix for stable, lets allow it. But please wait
> > > > for net to be merged into net-next, and submit a cleanup patch which
> > > > does move fec_ptp_read() earlier and remove the forward declaration.
> > > 
> > > That makes sense.
> > > 
> > > However, is this a fix?
> > > It's not clear to me that it is.
> > 
> > Well, it's not clear to me either what constitutes as a "fix" versus "just a
> > cleanup". But, whatever floats Andrew's boat...
> 
> Let me state my rule of thumb: a fix addresses a user-visible bug.
> 
> > > And if it is a pre-requisite for patch 2/2,
> > > well that doesn't seem to be a fix.
> > 
> > It indeed is.
> > 
> > > So in all, I'm somewhat confused.
> > > And wonder if all changes can go via net-next.
> > 
> > That's probably what will be happening.
> 
> It does seem like the cleanest, and coincidently easiest, path.

If it does not really fix anything, then net-next.

   Andrew