[PATCH net v3] net: ti: icssg-prueth: Fix 1 PPS sync

[PATCH net v3] net: ti: icssg-prueth: Fix 1 PPS sync

[Meghana Malladi]

The first PPS latch time needs to be calculated by the driver
(in rounded off seconds) and configured as the start time
offset for the cycle. After synchronizing two PTP clocks
running as master/slave, missing this would cause master
and slave to start immediately with some milliseconds
drift which causes the PPS signal to never synchronize with
the PTP master.

Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Reviewed-by: MD Danish Anwar <danishanwar@ti.com>
---

Hello,

This patch is based on net-next tagged next-2024102.
v2:https://lore.kernel.org/all/20241024113140.973928-1-m-malladi@ti.com/
Changes since v2 (v3-v2):
- Use hi_lo_writeq() and hi_lo_readq() instead of own helpers
(icssg_readq() & iccsg_writeq()) as asked by Andrew Lunn <andrew@lunn.ch>
- Collected Reviewed-by tags from Vadim and Danish

Regards,
Meghana.

 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 0556910938fa..678a99882627 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -16,6 +16,7 @@
 #include <linux/if_hsr.h>
 #include <linux/if_vlan.h>
 #include <linux/interrupt.h>
+#include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
@@ -411,6 +412,8 @@ static int prueth_perout_enable(void *clockops_data,
 	struct prueth_emac *emac = clockops_data;
 	u32 reduction_factor = 0, offset = 0;
 	struct timespec64 ts;
+	u64 current_cycle;
+	u64 start_offset;
 	u64 ns_period;
 
 	if (!on)
@@ -449,8 +452,14 @@ static int prueth_perout_enable(void *clockops_data,
 	writel(reduction_factor, emac->prueth->shram.va +
 		TIMESYNC_FW_WC_SYNCOUT_REDUCTION_FACTOR_OFFSET);
 
-	writel(0, emac->prueth->shram.va +
-		TIMESYNC_FW_WC_SYNCOUT_START_TIME_CYCLECOUNT_OFFSET);
+	current_cycle = hi_lo_readq(emac->prueth->shram.va +
+				    TIMESYNC_FW_WC_CYCLECOUNT_OFFSET);
+
+	/* Rounding of current_cycle count to next second */
+	start_offset = roundup(current_cycle, MSEC_PER_SEC);
+
+	hi_lo_writeq(start_offset, emac->prueth->shram.va +
+		     TIMESYNC_FW_WC_SYNCOUT_START_TIME_CYCLECOUNT_OFFSET);
 
 	return 0;
 }

base-commit: 73840ca5ef361f143b89edd5368a1aa8c2979241
-- 
2.25.1

Re: [PATCH net v3] net: ti: icssg-prueth: Fix 1 PPS sync

[Jakub Kicinski]

On Mon, 28 Oct 2024 16:40:52 +0530 Meghana Malladi wrote:
> The first PPS latch time needs to be calculated by the driver
> (in rounded off seconds) and configured as the start time
> offset for the cycle. After synchronizing two PTP clocks
> running as master/slave, missing this would cause master
> and slave to start immediately with some milliseconds
> drift which causes the PPS signal to never synchronize with
> the PTP master.

You're reading a 64b value in chunks, is it not possible that it'd wrap
in between reads? This can be usually detected by reading high twice and
making sure it didn't change.

Please fix or explain in the commit message why this is not a problem..
-- 
pw-bot: cr

Re: [PATCH net v3] net: ti: icssg-prueth: Fix 1 PPS sync

[Malladi, Meghana]

On 11/1/2024 7:29 AM, Jakub Kicinski wrote:
> On Mon, 28 Oct 2024 16:40:52 +0530 Meghana Malladi wrote:
>> The first PPS latch time needs to be calculated by the driver
>> (in rounded off seconds) and configured as the start time
>> offset for the cycle. After synchronizing two PTP clocks
>> running as master/slave, missing this would cause master
>> and slave to start immediately with some milliseconds
>> drift which causes the PPS signal to never synchronize with
>> the PTP master.
> 
> You're reading a 64b value in chunks, is it not possible that it'd wrap
> in between reads? This can be usually detected by reading high twice and
> making sure it didn't change.
> 
> Please fix or explain in the commit message why this is not a problem..
Yes I agree that there might be a wrap if the read isn't atomic. As 
suggested by Andrew I am currently not using custom read where I can 
implement the logic you suggested (reading high twice and making sure if 
didn't change). Can you share me some references where this logic is 
implemented in the kernel, so I can directly use that instead of writing 
custom functions.

Regards,
Meghana.

Re: [PATCH net v3] net: ti: icssg-prueth: Fix 1 PPS sync

[Jakub Kicinski]

On Mon, 4 Nov 2024 16:55:46 +0530 Malladi, Meghana wrote:
> On 11/1/2024 7:29 AM, Jakub Kicinski wrote:
> > On Mon, 28 Oct 2024 16:40:52 +0530 Meghana Malladi wrote:  
> >> The first PPS latch time needs to be calculated by the driver
> >> (in rounded off seconds) and configured as the start time
> >> offset for the cycle. After synchronizing two PTP clocks
> >> running as master/slave, missing this would cause master
> >> and slave to start immediately with some milliseconds
> >> drift which causes the PPS signal to never synchronize with
> >> the PTP master.  
> > 
> > You're reading a 64b value in chunks, is it not possible that it'd wrap
> > in between reads? This can be usually detected by reading high twice and
> > making sure it didn't change.
> > 
> > Please fix or explain in the commit message why this is not a problem..  
> Yes I agree that there might be a wrap if the read isn't atomic. As 
> suggested by Andrew I am currently not using custom read where I can 
> implement the logic you suggested

Right but I think Andrew was commenting on a patch which contained pure
re-implementation of read low / hi with no extra bells or whistles. 

> (reading high twice and making sure if 
> didn't change). Can you share me some references where this logic is 
> implemented in the kernel, so I can directly use that instead of writing 
> custom functions.

I think you need to write a custom one. Example:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/meta/fbnic/fbnic_time.c#n40

Re: [PATCH net v3] net: ti: icssg-prueth: Fix 1 PPS sync

[Malladi, Meghana]

On 11/5/2024 8:20 AM, Jakub Kicinski wrote:
> On Mon, 4 Nov 2024 16:55:46 +0530 Malladi, Meghana wrote:
>> On 11/1/2024 7:29 AM, Jakub Kicinski wrote:
>>> On Mon, 28 Oct 2024 16:40:52 +0530 Meghana Malladi wrote:
>>>> The first PPS latch time needs to be calculated by the driver
>>>> (in rounded off seconds) and configured as the start time
>>>> offset for the cycle. After synchronizing two PTP clocks
>>>> running as master/slave, missing this would cause master
>>>> and slave to start immediately with some milliseconds
>>>> drift which causes the PPS signal to never synchronize with
>>>> the PTP master.
>>>
>>> You're reading a 64b value in chunks, is it not possible that it'd wrap
>>> in between reads? This can be usually detected by reading high twice and
>>> making sure it didn't change.
>>>
>>> Please fix or explain in the commit message why this is not a problem..
>> Yes I agree that there might be a wrap if the read isn't atomic. As
>> suggested by Andrew I am currently not using custom read where I can
>> implement the logic you suggested
> 
> Right but I think Andrew was commenting on a patch which contained pure
> re-implementation of read low / hi with no extra bells or whistles.
> 
>> (reading high twice and making sure if
>> didn't change). Can you share me some references where this logic is
>> implemented in the kernel, so I can directly use that instead of writing
>> custom functions.
> 
> I think you need to write a custom one. Example:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/meta/fbnic/fbnic_time.c#n40
Ok thank you. I will add custom function for this and update the patch.

Re: [PATCH net v3] net: ti: icssg-prueth: Fix 1 PPS sync

[Andrew Lunn]

> > I think you need to write a custom one. Example:
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/meta/fbnic/fbnic_time.c#n40
> Ok thank you. I will add custom function for this and update the patch.

Maybe look around and see if they could be reused by other drivers. If
so, they should be put somewhere central.

	Andrew

Re: [PATCH net v3] net: ti: icssg-prueth: Fix 1 PPS sync

[Malladi, Meghana]

On 11/6/2024 2:30 AM, Andrew Lunn wrote:
>>> I think you need to write a custom one. Example:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/meta/fbnic/fbnic_time.c#n40
>> Ok thank you. I will add custom function for this and update the patch.
> 
> Maybe look around and see if they could be reused by other drivers. If
> so, they should be put somewhere central.
> 
> 	Andrew
I have looked into it, and seems like wherever this is getting used it 
is written as a custom function for that driver. Seems like writing our 
own is inevitable.

- Meghana.