| Message ID | 20241028111051.1546143-1-m-malladi@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net: ti: icssg-prueth: Fix 1 PPS sync | expand |
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..
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.
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
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.
> > 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
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.
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; }
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(-) base-commit: 73840ca5ef361f143b89edd5368a1aa8c2979241