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

[PATCH net v2] 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>
---

Hello,

This patch is based on net-next tagged next-20241023.
v1:https://lore.kernel.org/all/20241023091213.593351-1-m-malladi@ti.com/
Changes since v1 (v2-v1):
- Use roundup() instead of open coding as suggested by Vadim Fedorenko

Regards,
Meghana.

 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 12 ++++++++++--
 drivers/net/ethernet/ti/icssg/icssg_prueth.h | 11 +++++++++++
 2 files changed, 21 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..6876e8181066 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -411,6 +411,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 +451,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 = icssg_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);
+
+	icssg_writeq(start_offset, emac->prueth->shram.va +
+		     TIMESYNC_FW_WC_SYNCOUT_START_TIME_CYCLECOUNT_OFFSET);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 8722bb4a268a..a4af2dbcca31 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -330,6 +330,17 @@ static inline int prueth_emac_slice(struct prueth_emac *emac)
 extern const struct ethtool_ops icssg_ethtool_ops;
 extern const struct dev_pm_ops prueth_dev_pm_ops;
 
+static inline u64 icssg_readq(const void __iomem *addr)
+{
+	return readl(addr) + ((u64)readl(addr + 4) << 32);
+}
+
+static inline void icssg_writeq(u64 val, void __iomem *addr)
+{
+	writel(lower_32_bits(val), addr);
+	writel(upper_32_bits(val), addr + 4);
+}
+
 /* Classifier helpers */
 void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac);
 void icssg_class_set_host_mac_addr(struct regmap *miig_rt, const u8 *mac);

base-commit: 73840ca5ef361f143b89edd5368a1aa8c2979241
-- 
2.25.1

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

[Vadim Fedorenko]

On 24/10/2024 12:31, 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.
> 
> Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on net-next tagged next-20241023.
> v1:https://lore.kernel.org/all/20241023091213.593351-1-m-malladi@ti.com/
> Changes since v1 (v2-v1):
> - Use roundup() instead of open coding as suggested by Vadim Fedorenko
> 
> Regards,
> Meghana.
> 
>   drivers/net/ethernet/ti/icssg/icssg_prueth.c | 12 ++++++++++--
>   drivers/net/ethernet/ti/icssg/icssg_prueth.h | 11 +++++++++++
>   2 files changed, 21 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..6876e8181066 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -411,6 +411,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 +451,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 = icssg_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);
> +
> +	icssg_writeq(start_offset, emac->prueth->shram.va +
> +		     TIMESYNC_FW_WC_SYNCOUT_START_TIME_CYCLECOUNT_OFFSET);
>   
>   	return 0;
>   }
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index 8722bb4a268a..a4af2dbcca31 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -330,6 +330,17 @@ static inline int prueth_emac_slice(struct prueth_emac *emac)
>   extern const struct ethtool_ops icssg_ethtool_ops;
>   extern const struct dev_pm_ops prueth_dev_pm_ops;
>   
> +static inline u64 icssg_readq(const void __iomem *addr)
> +{
> +	return readl(addr) + ((u64)readl(addr + 4) << 32);
> +}
> +
> +static inline void icssg_writeq(u64 val, void __iomem *addr)
> +{
> +	writel(lower_32_bits(val), addr);
> +	writel(upper_32_bits(val), addr + 4);
> +}
> +
>   /* Classifier helpers */
>   void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac);
>   void icssg_class_set_host_mac_addr(struct regmap *miig_rt, const u8 *mac);
> 
> base-commit: 73840ca5ef361f143b89edd5368a1aa8c2979241

Thanks,

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

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

[Anwar, Md Danish]

Hi Meghana,

On 10/24/2024 5:01 PM, 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.
> 
> Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>

Reviewed-by: MD Danish Anwar <danishanwar@ti.com>

-- 
Thanks and Regards,
Md Danish Anwar

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

[Andrew Lunn]

> +static inline u64 icssg_readq(const void __iomem *addr)
> +{
> +	return readl(addr) + ((u64)readl(addr + 4) << 32);
> +}
> +
> +static inline void icssg_writeq(u64 val, void __iomem *addr)
> +{
> +	writel(lower_32_bits(val), addr);
> +	writel(upper_32_bits(val), addr + 4);
> +}

Could readq() and writeq() be used, rather than your own helpers?

	Andrew

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

[Meghana Malladi]

On 25/10/24 01:25, Andrew Lunn wrote:
>> +static inline u64 icssg_readq(const void __iomem *addr)
>> +{
>> +	return readl(addr) + ((u64)readl(addr + 4) << 32);
>> +}
>> +
>> +static inline void icssg_writeq(u64 val, void __iomem *addr)
>> +{
>> +	writel(lower_32_bits(val), addr);
>> +	writel(upper_32_bits(val), addr + 4);
>> +}
> 
> Could readq() and writeq() be used, rather than your own helpers?
> 
> 	Andrew
> 
The addresses we are trying to read here are not 64-bit aligned, hence 
using our own helpers to read the 64-bit value.

Regards,
Meghana

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

[Andrew Lunn]

On Fri, Oct 25, 2024 at 11:17:44AM +0530, Meghana Malladi wrote:
> 
> 
> On 25/10/24 01:25, Andrew Lunn wrote:
> > > +static inline u64 icssg_readq(const void __iomem *addr)
> > > +{
> > > +	return readl(addr) + ((u64)readl(addr + 4) << 32);
> > > +}
> > > +
> > > +static inline void icssg_writeq(u64 val, void __iomem *addr)
> > > +{
> > > +	writel(lower_32_bits(val), addr);
> > > +	writel(upper_32_bits(val), addr + 4);
> > > +}
> > 
> > Could readq() and writeq() be used, rather than your own helpers?
> > 
> > 	Andrew
> > 
> The addresses we are trying to read here are not 64-bit aligned, hence using
> our own helpers to read the 64-bit value.

Ah, you should document this, because somebody might do a drive by
patch converting this to readq()/write(q).

Alternatively, i think hi_lo_writeq() would work.

	Andrew

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

[Meghana Malladi]

On 25/10/24 18:52, Andrew Lunn wrote:
> On Fri, Oct 25, 2024 at 11:17:44AM +0530, Meghana Malladi wrote:
>>
>>
>> On 25/10/24 01:25, Andrew Lunn wrote:
>>>> +static inline u64 icssg_readq(const void __iomem *addr)
>>>> +{
>>>> +	return readl(addr) + ((u64)readl(addr + 4) << 32);
>>>> +}
>>>> +
>>>> +static inline void icssg_writeq(u64 val, void __iomem *addr)
>>>> +{
>>>> +	writel(lower_32_bits(val), addr);
>>>> +	writel(upper_32_bits(val), addr + 4);
>>>> +}
>>>
>>> Could readq() and writeq() be used, rather than your own helpers?
>>>
>>> 	Andrew
>>>
>> The addresses we are trying to read here are not 64-bit aligned, hence using
>> our own helpers to read the 64-bit value.
> 
> Ah, you should document this, because somebody might do a drive by
> patch converting this to readq()/write(q).
> 
> Alternatively, i think hi_lo_writeq() would work.
> 
> 	Andrew
I tried hi_lo_readq() and hi_lo_writeq(), and it is fitting my 
requirement. Thanks, I will update it.

Regards,
Meghana