When setting or getting PHC time, the higher bits of the second time (>32
bits) they were ignored. Meaning that setting some time in the future like
year 2150, it was failing to set this.
The issue can be reproduced like this:
# phc_ctl /dev/ptp1 set 10000000000
phc_ctl[12.290]: set clock time to 10000000000.000000000 or Sat Nov 20 17:46:40 2286
# phc_ctl /dev/ptp1 get
phc_ctl[15.309]: clock time is 1410065411.018055420 or Sun Sep 7 04:50:11 2014
Signed-off-by: Horatiu Vultur <[email protected]>
Reviewed-by: Maxime Chevallier <[email protected]>
Reviewed-by: Divya Koppera <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
Based on discussion here [1], this patch from the series was changed
to target net-next instead of net.
[1] https://lore.kernel.org/netdev/[email protected]/T/#m88b55103ee8c05599f2fa02c1588e195d95d6a49
---
drivers/net/phy/micrel.c | 61 +++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index dad720138baaf..40bea9293ddd7 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -154,11 +154,13 @@
#define PTP_CMD_CTL_PTP_LTC_STEP_SEC_ BIT(5)
#define PTP_CMD_CTL_PTP_LTC_STEP_NSEC_ BIT(6)
+#define PTP_CLOCK_SET_SEC_HI 0x0205
#define PTP_CLOCK_SET_SEC_MID 0x0206
#define PTP_CLOCK_SET_SEC_LO 0x0207
#define PTP_CLOCK_SET_NS_HI 0x0208
#define PTP_CLOCK_SET_NS_LO 0x0209
+#define PTP_CLOCK_READ_SEC_HI 0x0229
#define PTP_CLOCK_READ_SEC_MID 0x022A
#define PTP_CLOCK_READ_SEC_LO 0x022B
#define PTP_CLOCK_READ_NS_HI 0x022C
@@ -2592,35 +2594,31 @@ static bool lan8814_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb
}
static void lan8814_ptp_clock_set(struct phy_device *phydev,
- u32 seconds, u32 nano_seconds)
+ time64_t sec, u32 nsec)
{
- u32 sec_low, sec_high, nsec_low, nsec_high;
-
- sec_low = seconds & 0xffff;
- sec_high = (seconds >> 16) & 0xffff;
- nsec_low = nano_seconds & 0xffff;
- nsec_high = (nano_seconds >> 16) & 0x3fff;
-
- lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_LO, sec_low);
- lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_MID, sec_high);
- lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_LO, nsec_low);
- lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_HI, nsec_high);
+ lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_LO, lower_16_bits(sec));
+ lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_MID, upper_16_bits(sec));
+ lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_HI, upper_32_bits(sec));
+ lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_LO, lower_16_bits(nsec));
+ lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_HI, upper_16_bits(nsec));
lanphy_write_page_reg(phydev, 4, PTP_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_LOAD_);
}
static void lan8814_ptp_clock_get(struct phy_device *phydev,
- u32 *seconds, u32 *nano_seconds)
+ time64_t *sec, u32 *nsec)
{
lanphy_write_page_reg(phydev, 4, PTP_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_READ_);
- *seconds = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_MID);
- *seconds = (*seconds << 16) |
- lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_LO);
+ *sec = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_HI);
+ *sec <<= 16;
+ *sec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_MID);
+ *sec <<= 16;
+ *sec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_LO);
- *nano_seconds = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_HI);
- *nano_seconds = ((*nano_seconds & 0x3fff) << 16) |
- lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_LO);
+ *nsec = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_HI);
+ *nsec <<= 16;
+ *nsec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_LO);
}
static int lan8814_ptpci_gettime64(struct ptp_clock_info *ptpci,
@@ -2630,7 +2628,7 @@ static int lan8814_ptpci_gettime64(struct ptp_clock_info *ptpci,
ptp_clock_info);
struct phy_device *phydev = shared->phydev;
u32 nano_seconds;
- u32 seconds;
+ time64_t seconds;
mutex_lock(&shared->shared_lock);
lan8814_ptp_clock_get(phydev, &seconds, &nano_seconds);
@@ -2660,38 +2658,37 @@ static void lan8814_ptp_clock_step(struct phy_device *phydev,
{
u32 nano_seconds_step;
u64 abs_time_step_ns;
- u32 unsigned_seconds;
+ time64_t set_seconds;
u32 nano_seconds;
u32 remainder;
s32 seconds;
if (time_step_ns > 15000000000LL) {
/* convert to clock set */
- lan8814_ptp_clock_get(phydev, &unsigned_seconds, &nano_seconds);
- unsigned_seconds += div_u64_rem(time_step_ns, 1000000000LL,
- &remainder);
+ lan8814_ptp_clock_get(phydev, &set_seconds, &nano_seconds);
+ set_seconds += div_u64_rem(time_step_ns, 1000000000LL,
+ &remainder);
nano_seconds += remainder;
if (nano_seconds >= 1000000000) {
- unsigned_seconds++;
+ set_seconds++;
nano_seconds -= 1000000000;
}
- lan8814_ptp_clock_set(phydev, unsigned_seconds, nano_seconds);
+ lan8814_ptp_clock_set(phydev, set_seconds, nano_seconds);
return;
} else if (time_step_ns < -15000000000LL) {
/* convert to clock set */
time_step_ns = -time_step_ns;
- lan8814_ptp_clock_get(phydev, &unsigned_seconds, &nano_seconds);
- unsigned_seconds -= div_u64_rem(time_step_ns, 1000000000LL,
- &remainder);
+ lan8814_ptp_clock_get(phydev, &set_seconds, &nano_seconds);
+ set_seconds -= div_u64_rem(time_step_ns, 1000000000LL,
+ &remainder);
nano_seconds_step = remainder;
if (nano_seconds < nano_seconds_step) {
- unsigned_seconds--;
+ set_seconds--;
nano_seconds += 1000000000;
}
nano_seconds -= nano_seconds_step;
- lan8814_ptp_clock_set(phydev, unsigned_seconds,
- nano_seconds);
+ lan8814_ptp_clock_set(phydev, set_seconds, nano_seconds);
return;
}
--
2.34.1
On 26/01/2024 07:30, Horatiu Vultur wrote:
> When setting or getting PHC time, the higher bits of the second time (>32
> bits) they were ignored. Meaning that setting some time in the future like
> year 2150, it was failing to set this.
>
> The issue can be reproduced like this:
>
> # phc_ctl /dev/ptp1 set 10000000000
> phc_ctl[12.290]: set clock time to 10000000000.000000000 or Sat Nov 20 17:46:40 2286
>
> # phc_ctl /dev/ptp1 get
> phc_ctl[15.309]: clock time is 1410065411.018055420 or Sun Sep 7 04:50:11 2014
>
> Signed-off-by: Horatiu Vultur <[email protected]>
> Reviewed-by: Maxime Chevallier <[email protected]>
> Reviewed-by: Divya Koppera <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
>
> ---
> Based on discussion here [1], this patch from the series was changed
> to target net-next instead of net.
>
> [1] https://lore.kernel.org/netdev/[email protected]/T/#m88b55103ee8c05599f2fa02c1588e195d95d6a49
> ---
> drivers/net/phy/micrel.c | 61 +++++++++++++++++++---------------------
> 1 file changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index dad720138baaf..40bea9293ddd7 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -154,11 +154,13 @@
> #define PTP_CMD_CTL_PTP_LTC_STEP_SEC_ BIT(5)
> #define PTP_CMD_CTL_PTP_LTC_STEP_NSEC_ BIT(6)
>
> +#define PTP_CLOCK_SET_SEC_HI 0x0205
> #define PTP_CLOCK_SET_SEC_MID 0x0206
> #define PTP_CLOCK_SET_SEC_LO 0x0207
> #define PTP_CLOCK_SET_NS_HI 0x0208
> #define PTP_CLOCK_SET_NS_LO 0x0209
>
> +#define PTP_CLOCK_READ_SEC_HI 0x0229
> #define PTP_CLOCK_READ_SEC_MID 0x022A
> #define PTP_CLOCK_READ_SEC_LO 0x022B
> #define PTP_CLOCK_READ_NS_HI 0x022C
> @@ -2592,35 +2594,31 @@ static bool lan8814_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb
> }
>
> static void lan8814_ptp_clock_set(struct phy_device *phydev,
> - u32 seconds, u32 nano_seconds)
> + time64_t sec, u32 nsec)
> {
> - u32 sec_low, sec_high, nsec_low, nsec_high;
> -
> - sec_low = seconds & 0xffff;
> - sec_high = (seconds >> 16) & 0xffff;
> - nsec_low = nano_seconds & 0xffff;
> - nsec_high = (nano_seconds >> 16) & 0x3fff;
> -
> - lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_LO, sec_low);
> - lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_MID, sec_high);
> - lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_LO, nsec_low);
> - lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_HI, nsec_high);
> + lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_LO, lower_16_bits(sec));
> + lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_MID, upper_16_bits(sec));
> + lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_HI, upper_32_bits(sec));
> + lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_LO, lower_16_bits(nsec));
> + lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_HI, upper_16_bits(nsec));
>
> lanphy_write_page_reg(phydev, 4, PTP_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_LOAD_);
> }
>
> static void lan8814_ptp_clock_get(struct phy_device *phydev,
> - u32 *seconds, u32 *nano_seconds)
> + time64_t *sec, u32 *nsec)
> {
> lanphy_write_page_reg(phydev, 4, PTP_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_READ_);
>
> - *seconds = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_MID);
> - *seconds = (*seconds << 16) |
> - lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_LO);
> + *sec = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_HI);
> + *sec <<= 16;
> + *sec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_MID);
lanphy_read_page_reg returns int, but only 16 bits have meanings here.
Is it safe to assume that other 16 bits will be zeros always?
There are some more spots of this template in the function.
> + *sec <<= 16;
> + *sec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_LO);
>
> - *nano_seconds = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_HI);
> - *nano_seconds = ((*nano_seconds & 0x3fff) << 16) |
> - lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_LO);
> + *nsec = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_HI);
> + *nsec <<= 16;
> + *nsec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_LO);
> }
>
> static int lan8814_ptpci_gettime64(struct ptp_clock_info *ptpci,
> @@ -2630,7 +2628,7 @@ static int lan8814_ptpci_gettime64(struct ptp_clock_info *ptpci,
> ptp_clock_info);
> struct phy_device *phydev = shared->phydev;
> u32 nano_seconds;
> - u32 seconds;
> + time64_t seconds;
>
> mutex_lock(&shared->shared_lock);
> lan8814_ptp_clock_get(phydev, &seconds, &nano_seconds);
> @@ -2660,38 +2658,37 @@ static void lan8814_ptp_clock_step(struct phy_device *phydev,
> {
> u32 nano_seconds_step;
> u64 abs_time_step_ns;
> - u32 unsigned_seconds;
> + time64_t set_seconds;
> u32 nano_seconds;
> u32 remainder;
> s32 seconds;
>
> if (time_step_ns > 15000000000LL) {
> /* convert to clock set */
> - lan8814_ptp_clock_get(phydev, &unsigned_seconds, &nano_seconds);
> - unsigned_seconds += div_u64_rem(time_step_ns, 1000000000LL,
> - &remainder);
> + lan8814_ptp_clock_get(phydev, &set_seconds, &nano_seconds);
> + set_seconds += div_u64_rem(time_step_ns, 1000000000LL,
> + &remainder);
> nano_seconds += remainder;
> if (nano_seconds >= 1000000000) {
> - unsigned_seconds++;
> + set_seconds++;
> nano_seconds -= 1000000000;
> }
> - lan8814_ptp_clock_set(phydev, unsigned_seconds, nano_seconds);
> + lan8814_ptp_clock_set(phydev, set_seconds, nano_seconds);
> return;
> } else if (time_step_ns < -15000000000LL) {
> /* convert to clock set */
> time_step_ns = -time_step_ns;
>
> - lan8814_ptp_clock_get(phydev, &unsigned_seconds, &nano_seconds);
> - unsigned_seconds -= div_u64_rem(time_step_ns, 1000000000LL,
> - &remainder);
> + lan8814_ptp_clock_get(phydev, &set_seconds, &nano_seconds);
> + set_seconds -= div_u64_rem(time_step_ns, 1000000000LL,
> + &remainder);
> nano_seconds_step = remainder;
> if (nano_seconds < nano_seconds_step) {
> - unsigned_seconds--;
> + set_seconds--;
> nano_seconds += 1000000000;
> }
> nano_seconds -= nano_seconds_step;
> - lan8814_ptp_clock_set(phydev, unsigned_seconds,
> - nano_seconds);
> + lan8814_ptp_clock_set(phydev, set_seconds, nano_seconds);
> return;
> }
>
> > + *sec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_MID);
>
> lanphy_read_page_reg returns int, but only 16 bits have meanings here.
> Is it safe to assume that other 16 bits will be zeros always?
Yes. __phy_read() should only return a negative error code, or a value
which fits in a u16. If any of the top bits are set, its a bug in the
MDIO driver which needs finding and fixing.
Andrew
On 26/01/2024 13:15, Andrew Lunn wrote:
>>> + *sec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_MID);
>>
>> lanphy_read_page_reg returns int, but only 16 bits have meanings here.
>> Is it safe to assume that other 16 bits will be zeros always?
>
> Yes. __phy_read() should only return a negative error code, or a value
> which fits in a u16. If any of the top bits are set, its a bug in the
> MDIO driver which needs finding and fixing.
Got it. Thanks Andrew
Hello:
This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <[email protected]>:
On Fri, 26 Jan 2024 08:30:42 +0100 you wrote:
> When setting or getting PHC time, the higher bits of the second time (>32
> bits) they were ignored. Meaning that setting some time in the future like
> year 2150, it was failing to set this.
>
> The issue can be reproduced like this:
>
> # phc_ctl /dev/ptp1 set 10000000000
> phc_ctl[12.290]: set clock time to 10000000000.000000000 or Sat Nov 20 17:46:40 2286
>
> [...]
Here is the summary with links:
- [net-next] net: micrel: Fix set/get PHC time for lan8814
https://git.kernel.org/netdev/net-next/c/8e41d6644f9a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html