[PATCH net 0/2] net: micrel: Fixes for PHC for lan8814

[PATCH net 0/2] net: micrel: Fixes for PHC for lan8814

[Horatiu Vultur]

There are different issues with the PHC of lan8814.
First issue is similar with the one on lan8841 which it has enabled by
default the check regarding minorVersionPTP in the PTP header frame.
So all the frames compliant to 8021AS will not be timestamped.
The second issue is with setting and getting the time of the PHC. The
issue is seen only when setting a time which is bigger than 2^32.

Horatiu Vultur (2):
  net: micrel: Fix PTP frame parsing for lan8814
  net: micrel: Fix set/get PHC time for lan8814

 drivers/net/phy/micrel.c | 68 +++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH net 1/2] net: micrel: Fix PTP frame parsing for lan8814

[Horatiu Vultur]

The HW has the capability to check each frame if it is a PTP frame,
which domain it is, which ptp frame type it is, different ip address in
the frame. And if one of these checks fail then the frame is not
timestamp. Most of these checks were disabled except checking the field
minorVersionPTP inside the PTP header. Meaning that once a partner sends
a frame compliant to 8021AS which has minorVersionPTP set to 1, then the
frame was not timestamp because the HW expected by default a value of 0
in minorVersionPTP. This is exactly the same issue as on lan8841.
Fix this issue by removing this check so the userspace can decide on this.

Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/phy/micrel.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index bf4053431dcb3..1752eaeadc42e 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -120,6 +120,9 @@
  */
 #define LAN8814_1PPM_FORMAT			17179
 
+#define PTP_RX_VERSION				0x0248
+#define PTP_TX_VERSION				0x0288
+
 #define PTP_RX_MOD				0x024F
 #define PTP_RX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_ BIT(3)
 #define PTP_RX_TIMESTAMP_EN			0x024D
@@ -3150,6 +3153,10 @@ static void lan8814_ptp_init(struct phy_device *phydev)
 	lanphy_write_page_reg(phydev, 5, PTP_TX_PARSE_IP_ADDR_EN, 0);
 	lanphy_write_page_reg(phydev, 5, PTP_RX_PARSE_IP_ADDR_EN, 0);
 
+	/* Disable checking for minorVersionPTP field */
+	lanphy_write_page_reg(phydev, 5, PTP_RX_VERSION, 0xff00);
+	lanphy_write_page_reg(phydev, 5, PTP_TX_VERSION, 0xff00);
+
 	skb_queue_head_init(&ptp_priv->tx_queue);
 	skb_queue_head_init(&ptp_priv->rx_queue);
 	INIT_LIST_HEAD(&ptp_priv->rx_ts_list);
-- 
2.34.1

Re: [PATCH net 1/2] net: micrel: Fix PTP frame parsing for lan8814

[Maxime Chevallier]

Hello Horatiu,

On Sat, 13 Jan 2024 14:15:20 +0100
Horatiu Vultur <horatiu.vultur@microchip.com> wrote:

> The HW has the capability to check each frame if it is a PTP frame,
> which domain it is, which ptp frame type it is, different ip address in
> the frame. And if one of these checks fail then the frame is not
> timestamp. Most of these checks were disabled except checking the field
> minorVersionPTP inside the PTP header. Meaning that once a partner sends
> a frame compliant to 8021AS which has minorVersionPTP set to 1, then the
> frame was not timestamp because the HW expected by default a value of 0
> in minorVersionPTP. This is exactly the same issue as on lan8841.
> Fix this issue by removing this check so the userspace can decide on this.
> 
> Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  drivers/net/phy/micrel.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index bf4053431dcb3..1752eaeadc42e 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -120,6 +120,9 @@
>   */
>  #define LAN8814_1PPM_FORMAT			17179
>  
> +#define PTP_RX_VERSION				0x0248
> +#define PTP_TX_VERSION				0x0288
> +
>  #define PTP_RX_MOD				0x024F
>  #define PTP_RX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_ BIT(3)
>  #define PTP_RX_TIMESTAMP_EN			0x024D
> @@ -3150,6 +3153,10 @@ static void lan8814_ptp_init(struct phy_device *phydev)
>  	lanphy_write_page_reg(phydev, 5, PTP_TX_PARSE_IP_ADDR_EN, 0);
>  	lanphy_write_page_reg(phydev, 5, PTP_RX_PARSE_IP_ADDR_EN, 0);
>  
> +	/* Disable checking for minorVersionPTP field */
> +	lanphy_write_page_reg(phydev, 5, PTP_RX_VERSION, 0xff00);
> +	lanphy_write_page_reg(phydev, 5, PTP_TX_VERSION, 0xff00);

Small nit: This looks a bit like magic values, from the datasheet I
understand the upper byte is the max supported version and the lower
byte is the min supported version, maybe this could be wrapped in
macros ?

> +
>  	skb_queue_head_init(&ptp_priv->tx_queue);
>  	skb_queue_head_init(&ptp_priv->rx_queue);
>  	INIT_LIST_HEAD(&ptp_priv->rx_ts_list);

Besides that,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

Re: [PATCH net 1/2] net: micrel: Fix PTP frame parsing for lan8814

[Horatiu Vultur]

The 01/15/2024 11:06, Maxime Chevallier wrote:
> 
> Hello Horatiu,

Hi Maxime,

> 
> On Sat, 13 Jan 2024 14:15:20 +0100
> Horatiu Vultur <horatiu.vultur@microchip.com> wrote:
> 
> > The HW has the capability to check each frame if it is a PTP frame,
> > which domain it is, which ptp frame type it is, different ip address in
> > the frame. And if one of these checks fail then the frame is not
> > timestamp. Most of these checks were disabled except checking the field
> > minorVersionPTP inside the PTP header. Meaning that once a partner sends
> > a frame compliant to 8021AS which has minorVersionPTP set to 1, then the
> > frame was not timestamp because the HW expected by default a value of 0
> > in minorVersionPTP. This is exactly the same issue as on lan8841.
> > Fix this issue by removing this check so the userspace can decide on this.
> >
> > Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  drivers/net/phy/micrel.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index bf4053431dcb3..1752eaeadc42e 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -120,6 +120,9 @@
> >   */
> >  #define LAN8814_1PPM_FORMAT                  17179
> >
> > +#define PTP_RX_VERSION                               0x0248
> > +#define PTP_TX_VERSION                               0x0288
> > +
> >  #define PTP_RX_MOD                           0x024F
> >  #define PTP_RX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_ BIT(3)
> >  #define PTP_RX_TIMESTAMP_EN                  0x024D
> > @@ -3150,6 +3153,10 @@ static void lan8814_ptp_init(struct phy_device *phydev)
> >       lanphy_write_page_reg(phydev, 5, PTP_TX_PARSE_IP_ADDR_EN, 0);
> >       lanphy_write_page_reg(phydev, 5, PTP_RX_PARSE_IP_ADDR_EN, 0);
> >
> > +     /* Disable checking for minorVersionPTP field */
> > +     lanphy_write_page_reg(phydev, 5, PTP_RX_VERSION, 0xff00);
> > +     lanphy_write_page_reg(phydev, 5, PTP_TX_VERSION, 0xff00);
> 
> Small nit: This looks a bit like magic values, from the datasheet I
> understand the upper byte is the max supported version and the lower
> byte is the min supported version, maybe this could be wrapped in
> macros ?

Your understanding is correct. Yes, I will update this in the next
series.

> 
> > +
> >       skb_queue_head_init(&ptp_priv->tx_queue);
> >       skb_queue_head_init(&ptp_priv->rx_queue);
> >       INIT_LIST_HEAD(&ptp_priv->rx_ts_list);
> 
> Besides that,
> 
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> 
> Thanks,
> 
> Maxime

-- 
/Horatiu

RE: [PATCH net 1/2] net: micrel: Fix PTP frame parsing for lan8814

[Divya.Koppera]

> The HW has the capability to check each frame if it is a PTP frame, which
> domain it is, which ptp frame type it is, different ip address in the frame. And if
> one of these checks fail then the frame is not timestamp. Most of these checks
> were disabled except checking the field minorVersionPTP inside the PTP
> header. Meaning that once a partner sends a frame compliant to 8021AS
> which has minorVersionPTP set to 1, then the frame was not timestamp
> because the HW expected by default a value of 0 in minorVersionPTP. This is
> exactly the same issue as on lan8841.
> Fix this issue by removing this check so the userspace can decide on this.
> 
> Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

Reviewed-by: Divya Koppera <divya.koppera@microchip.com>

[PATCH net 2/2] net: micrel: Fix set/get PHC time for lan8814

[Horatiu Vultur]

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[118.619]: set clock time to 4294967295.000000000 or Sun Feb  7 06:28:15 2106

 # phc_ctl /dev/ptp1 get
 phc_ctl[120.858]: clock time is 1.238620924 or Thu Jan  1 00:00:01 1970

Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 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 1752eaeadc42e..d9eabbb5af37f 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -152,11 +152,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
@@ -2590,35 +2592,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,
@@ -2628,7 +2626,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);
@@ -2658,38 +2656,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

Re: [PATCH net 2/2] net: micrel: Fix set/get PHC time for lan8814

[Maxime Chevallier]

Hello Horatiu,

On Sat, 13 Jan 2024 14:15:21 +0100
Horatiu Vultur <horatiu.vultur@microchip.com> 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[118.619]: set clock time to 4294967295.000000000 or Sun Feb  7 06:28:15 2106
> 
>  # phc_ctl /dev/ptp1 get
>  phc_ctl[120.858]: clock time is 1.238620924 or Thu Jan  1 00:00:01 1970
> 
> Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

This looks fine to me,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

RE: [PATCH net 2/2] net: micrel: Fix set/get PHC time for lan8814

[Divya.Koppera]

> 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[118.619]: set clock time to 4294967295.000000000 or Sun Feb  7
> 06:28:15 2106
> 
>  # phc_ctl /dev/ptp1 get
>  phc_ctl[120.858]: clock time is 1.238620924 or Thu Jan  1 00:00:01 1970
> 
> Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

Changes looks fine to me.

Reviewed-by: Divya Koppera <divya.koppera@microchip.com>