[PATCH net-next 0/3] Add support for 1588 in LAN8814

[PATCH net-next 0/3] Add support for 1588 in LAN8814

[Divya Koppera]

The following patch series contains:
- Fix for concurrent register access, which provides
  atomic access to extended page register reads/writes.
- Provides dt-bindings related to latency and timestamping
  that are required for LAN8814 phy.
- 1588 hardware timestamping support in LAN8814 phy.

Divya Koppera (3):
  net: phy: micrel: Fix concurrent register access
  dt-bindings: net: micrel: Configure latency values and timestamping
    check for LAN8814 phy
  net: phy: micrel: 1588 support for LAN8814 phy

 .../devicetree/bindings/net/micrel.txt        |   17 +
 drivers/net/phy/micrel.c                      | 1114 ++++++++++++++++-
 2 files changed, 1097 insertions(+), 34 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] net: phy: micrel: Fix concurrent register access

[Divya Koppera]

Make Extended page register accessing atomic,
to overcome unexpected output from register
reads/writes.

Fixes: 7c2dcfa295b1 ("net: phy: micrel: Add support for LAN8804 PHY")
Signed-off-by: Divya Koppera<Divya.Koppera@microchip.com>
---
 drivers/net/phy/micrel.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index a7ebcdab415b..281cebc3d00c 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1596,11 +1596,13 @@ static int lanphy_read_page_reg(struct phy_device *phydev, int page, u32 addr)
 {
 	u32 data;
 
-	phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
-	phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
-	phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL,
-		  (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));
-	data = phy_read(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA);
+	phy_lock_mdio_bus(phydev);
+	__phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
+	__phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
+	__phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL,
+		    (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));
+	data = __phy_read(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA);
+	phy_unlock_mdio_bus(phydev);
 
 	return data;
 }
@@ -1608,18 +1610,18 @@ static int lanphy_read_page_reg(struct phy_device *phydev, int page, u32 addr)
 static int lanphy_write_page_reg(struct phy_device *phydev, int page, u16 addr,
 				 u16 val)
 {
-	phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
-	phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
-	phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL,
-		  (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));
+	phy_lock_mdio_bus(phydev);
+	__phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
+	__phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
+	__phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL,
+		    page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC);
 
-	val = phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, val);
-	if (val) {
+	val = __phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, val);
+	if (val != 0)
 		phydev_err(phydev, "Error: phy_write has returned error %d\n",
 			   val);
-		return val;
-	}
-	return 0;
+	phy_unlock_mdio_bus(phydev);
+	return val;
 }
 
 static int lan8814_config_init(struct phy_device *phydev)
-- 
2.17.1

[PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Divya Koppera]

Supports configuring latency values and also adds
check for phy timestamping feature.

Signed-off-by: Divya Koppera<Divya.Koppera@microchip.com>
---
 .../devicetree/bindings/net/micrel.txt          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
index 8d157f0295a5..c5ab62c39133 100644
--- a/Documentation/devicetree/bindings/net/micrel.txt
+++ b/Documentation/devicetree/bindings/net/micrel.txt
@@ -45,3 +45,20 @@ Optional properties:
 
 	In fiber mode, auto-negotiation is disabled and the PHY can only work in
 	100base-fx (full and half duplex) modes.
+
+ - lan8814,ignore-ts: If present the PHY will not support timestamping.
+
+	This option acts as check whether Timestamping is supported by
+	hardware or not. LAN8814 phy support hardware tmestamping.
+
+ - lan8814,latency_rx_10: Configures Latency value of phy in ingress at 10 Mbps.
+
+ - lan8814,latency_tx_10: Configures Latency value of phy in egress at 10 Mbps.
+
+ - lan8814,latency_rx_100: Configures Latency value of phy in ingress at 100 Mbps.
+
+ - lan8814,latency_tx_100: Configures Latency value of phy in egress at 100 Mbps.
+
+ - lan8814,latency_rx_1000: Configures Latency value of phy in ingress at 1000 Mbps.
+
+ - lan8814,latency_tx_1000: Configures Latency value of phy in egress at 1000 Mbps.
-- 
2.17.1

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Andrew Lunn]

On Fri, Mar 04, 2022 at 03:04:17PM +0530, Divya Koppera wrote:
> Supports configuring latency values and also adds
> check for phy timestamping feature.
> 
> Signed-off-by: Divya Koppera<Divya.Koppera@microchip.com>
> ---
>  .../devicetree/bindings/net/micrel.txt          | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> index 8d157f0295a5..c5ab62c39133 100644
> --- a/Documentation/devicetree/bindings/net/micrel.txt
> +++ b/Documentation/devicetree/bindings/net/micrel.txt
> @@ -45,3 +45,20 @@ Optional properties:
>  
>  	In fiber mode, auto-negotiation is disabled and the PHY can only work in
>  	100base-fx (full and half duplex) modes.
> +
> + - lan8814,ignore-ts: If present the PHY will not support timestamping.
> +
> +	This option acts as check whether Timestamping is supported by
> +	hardware or not. LAN8814 phy support hardware tmestamping.

Does this mean the hardware itself cannot tell you it is missing the
needed hardware? What happens when you forget to add this flag? Does
the driver timeout waiting for hardware which does not exists?

> + - lan8814,latency_rx_10: Configures Latency value of phy in ingress at 10 Mbps.
> +
> + - lan8814,latency_tx_10: Configures Latency value of phy in egress at 10 Mbps.
> +
> + - lan8814,latency_rx_100: Configures Latency value of phy in ingress at 100 Mbps.
> +
> + - lan8814,latency_tx_100: Configures Latency value of phy in egress at 100 Mbps.
> +
> + - lan8814,latency_rx_1000: Configures Latency value of phy in ingress at 1000 Mbps.
> +
> + - lan8814,latency_tx_1000: Configures Latency value of phy in egress at 1000 Mbps.

Why does this need to be configured, rather than hard coded? Why would
the latency for a given speed change? I would of thought though you
would take the average length of a PTP packet and divide is by the
link speed.

     Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Fri, Mar 04, 2022 at 01:50:47PM +0100, Andrew Lunn wrote:
> Why does this need to be configured, rather than hard coded? Why would
> the latency for a given speed change? I would of thought though you
> would take the average length of a PTP packet and divide is by the
> link speed.

Latency is unrelated to frame length.

My understanding is that it is VERY tricky to measure PHY latency.
Studies have shown that some PHYs vary by link speed, and some vary
randomly, frame by frame.

So I can understand wanting to configure it.  However, DTS is probably
the wrong place.  The linuxptp user space stack has configuration
variables for this purpose:

       egressLatency
              Specifies  the  difference  in  nanoseconds  between  the actual
              transmission time at the reference plane and the reported trans‐
              mit  time  stamp. This value will be added to egress time stamps
              obtained from the hardware.  The default is 0.

       ingressLatency
              Specifies the difference in nanoseconds between the reported re‐
              ceive  time  stamp  and  the  actual reception time at reference
              plane. This value will be subtracted from  ingress  time  stamps
              obtained from the hardware.  The default is 0.

Thanks,
Richard

RE: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Divya.Koppera]

Hi Richard,

> -----Original Message-----
> From: Richard Cochran <richardcochran@gmail.com>
> Sent: Friday, March 4, 2022 7:26 PM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Divya Koppera - I30481 <Divya.Koppera@microchip.com>;
> netdev@vger.kernel.org; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Madhuri Sripada - I34878
> <Madhuri.Sripada@microchip.com>; Manohar Puri - I30488
> <Manohar.Puri@microchip.com>
> Subject: Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency
> values and timestamping check for LAN8814 phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Fri, Mar 04, 2022 at 01:50:47PM +0100, Andrew Lunn wrote:
> > Why does this need to be configured, rather than hard coded? Why would
> > the latency for a given speed change? I would of thought though you
> > would take the average length of a PTP packet and divide is by the
> > link speed.
> 
> Latency is unrelated to frame length.
> 
> My understanding is that it is VERY tricky to measure PHY latency.
> Studies have shown that some PHYs vary by link speed, and some vary
> randomly, frame by frame.
> 
> So I can understand wanting to configure it.  However, DTS is probably the
> wrong place.  The linuxptp user space stack has configuration variables for this
> purpose:
> 
>        egressLatency
>               Specifies  the  difference  in  nanoseconds  between  the actual
>               transmission time at the reference plane and the reported trans‐
>               mit  time  stamp. This value will be added to egress time stamps
>               obtained from the hardware.  The default is 0.
> 
>        ingressLatency
>               Specifies the difference in nanoseconds between the reported re‐
>               ceive  time  stamp  and  the  actual reception time at reference
>               plane. This value will be subtracted from  ingress  time  stamps
>               obtained from the hardware.  The default is 0.
> 

I will check this and come back with fix if it is applicable.

Thanks,
Divya

> Thanks,
> Richard

RE: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Divya.Koppera]

Hi Andrew,

Thanks for review and comments. Please find reply inline below.

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, March 4, 2022 6:21 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> Cc: netdev@vger.kernel.org; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; richardcochran@gmail.com; linux-
> kernel@vger.kernel.org; UNGLinuxDriver <UNGLinuxDriver@microchip.com>;
> Madhuri Sripada - I34878 <Madhuri.Sripada@microchip.com>; Manohar Puri -
> I30488 <Manohar.Puri@microchip.com>
> Subject: Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency
> values and timestamping check for LAN8814 phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Fri, Mar 04, 2022 at 03:04:17PM +0530, Divya Koppera wrote:
> > Supports configuring latency values and also adds check for phy
> > timestamping feature.
> >
> > Signed-off-by: Divya Koppera<Divya.Koppera@microchip.com>
> > ---
> >  .../devicetree/bindings/net/micrel.txt          | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/micrel.txt
> > b/Documentation/devicetree/bindings/net/micrel.txt
> > index 8d157f0295a5..c5ab62c39133 100644
> > --- a/Documentation/devicetree/bindings/net/micrel.txt
> > +++ b/Documentation/devicetree/bindings/net/micrel.txt
> > @@ -45,3 +45,20 @@ Optional properties:
> >
> >       In fiber mode, auto-negotiation is disabled and the PHY can only work in
> >       100base-fx (full and half duplex) modes.
> > +
> > + - lan8814,ignore-ts: If present the PHY will not support timestamping.
> > +
> > +     This option acts as check whether Timestamping is supported by
> > +     hardware or not. LAN8814 phy support hardware tmestamping.
> 
> Does this mean the hardware itself cannot tell you it is missing the needed
> hardware? What happens when you forget to add this flag? Does the driver
> timeout waiting for hardware which does not exists?
> 

If forgot to add this flag, driver will try to register ptp_clock that needs
access to clock related registers, which in turn fails if those registers doesn't exists.

> > + - lan8814,latency_rx_10: Configures Latency value of phy in ingress at 10
> Mbps.
> > +
> > + - lan8814,latency_tx_10: Configures Latency value of phy in egress at 10
> Mbps.
> > +
> > + - lan8814,latency_rx_100: Configures Latency value of phy in ingress at 100
> Mbps.
> > +
> > + - lan8814,latency_tx_100: Configures Latency value of phy in egress at 100
> Mbps.
> > +
> > + - lan8814,latency_rx_1000: Configures Latency value of phy in ingress at
> 1000 Mbps.
> > +
> > + - lan8814,latency_tx_1000: Configures Latency value of phy in egress at
> 1000 Mbps.
> 
> Why does this need to be configured, rather than hard coded? Why would the
> latency for a given speed change? I would of thought though you would take
> the average length of a PTP packet and divide is by the link speed.
> 

This latency values could be different for different phy's. So hardcoding will not work here.
Yes in our case latency values depends on port speed. It is delay between network medium and 
PTP timestamp point.

>      Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Andrew Lunn]

> > > +
> > > + - lan8814,ignore-ts: If present the PHY will not support timestamping.
> > > +
> > > +     This option acts as check whether Timestamping is supported by
> > > +     hardware or not. LAN8814 phy support hardware tmestamping.
> > 
> > Does this mean the hardware itself cannot tell you it is missing the needed
> > hardware? What happens when you forget to add this flag? Does the driver
> > timeout waiting for hardware which does not exists?
> > 
> 
> If forgot to add this flag, driver will try to register ptp_clock that needs
> access to clock related registers, which in turn fails if those registers doesn't exists.

Thanks for the reply, but you did not answer my question:

  Does this mean the hardware itself cannot tell you it is missing the
  needed hardware?

Don't you have different IDs in register 2 and 3 for those devices
with clock register and those without?

> > > + - lan8814,latency_rx_10: Configures Latency value of phy in ingress at 10
> > Mbps.
> > > +
> > > + - lan8814,latency_tx_10: Configures Latency value of phy in egress at 10
> > Mbps.
> > > +
> > > + - lan8814,latency_rx_100: Configures Latency value of phy in ingress at 100
> > Mbps.
> > > +
> > > + - lan8814,latency_tx_100: Configures Latency value of phy in egress at 100
> > Mbps.
> > > +
> > > + - lan8814,latency_rx_1000: Configures Latency value of phy in ingress at
> > 1000 Mbps.
> > > +
> > > + - lan8814,latency_tx_1000: Configures Latency value of phy in egress at
> > 1000 Mbps.
> > 
> > Why does this need to be configured, rather than hard coded? Why would the
> > latency for a given speed change? I would of thought though you would take
> > the average length of a PTP packet and divide is by the link speed.
> > 
> 
> This latency values could be different for different phy's. So hardcoding will not work here.

But you do actually have hard coded defaults. Those odd hex values i
pointed out.

By different PHYs do you mean different PHY versions? So you can look
at register 2 and 3, determine what PHY it is, and so from that what
defaults should be used? Or do you mean different boards with the same
PHY?

In general, the less tunables you have, the better. If the driver can
figure it out, it is better to not have DT properties. The PHY will
then also work with ACPI and USB etc, where there is no
DT. Implementing the user space API Richard pointed out will also
allow your PHY to work with none DP systems.

> Yes in our case latency values depends on port speed. It is delay between network medium and 
> PTP timestamp point.

What are the units. You generally have the units in the property
name. So e.g. lan8814,latency_tx_1000_ns. If need be, the driver then
converts to whatever value you place into the register.

If you do keep them, please make it clear that these values are
optional, and state what value will be used when the property is not
present.

	Andrew

RE: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Divya.Koppera]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, March 7, 2022 6:39 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> Cc: netdev@vger.kernel.org; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; richardcochran@gmail.com; linux-
> kernel@vger.kernel.org; UNGLinuxDriver <UNGLinuxDriver@microchip.com>;
> Madhuri Sripada - I34878 <Madhuri.Sripada@microchip.com>; Manohar Puri -
> I30488 <Manohar.Puri@microchip.com>
> Subject: Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency
> values and timestamping check for LAN8814 phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > > > +
> > > > + - lan8814,ignore-ts: If present the PHY will not support timestamping.
> > > > +
> > > > +     This option acts as check whether Timestamping is supported by
> > > > +     hardware or not. LAN8814 phy support hardware tmestamping.
> > >
> > > Does this mean the hardware itself cannot tell you it is missing the
> > > needed hardware? What happens when you forget to add this flag? Does
> > > the driver timeout waiting for hardware which does not exists?
> > >
> >
> > If forgot to add this flag, driver will try to register ptp_clock that
> > needs access to clock related registers, which in turn fails if those registers
> doesn't exists.
> 
> Thanks for the reply, but you did not answer my question:
> 
>   Does this mean the hardware itself cannot tell you it is missing the
>   needed hardware?
> 
> Don't you have different IDs in register 2 and 3 for those devices with clock
> register and those without?
> 

The purpose of this option is, if both PHY and MAC supports timestamping then always timestamping is done in PHY.
If timestamping need to be done in MAC we need a way to stop PHY timestamping. If this flag is used then timestamping is taken care by MAC.

> > > > + - lan8814,latency_rx_10: Configures Latency value of phy in
> > > > + ingress at 10
> > > Mbps.
> > > > +
> > > > + - lan8814,latency_tx_10: Configures Latency value of phy in
> > > > + egress at 10
> > > Mbps.
> > > > +
> > > > + - lan8814,latency_rx_100: Configures Latency value of phy in
> > > > + ingress at 100
> > > Mbps.
> > > > +
> > > > + - lan8814,latency_tx_100: Configures Latency value of phy in
> > > > + egress at 100
> > > Mbps.
> > > > +
> > > > + - lan8814,latency_rx_1000: Configures Latency value of phy in
> > > > + ingress at
> > > 1000 Mbps.
> > > > +
> > > > + - lan8814,latency_tx_1000: Configures Latency value of phy in
> > > > + egress at
> > > 1000 Mbps.
> > >
> > > Why does this need to be configured, rather than hard coded? Why
> > > would the latency for a given speed change? I would of thought
> > > though you would take the average length of a PTP packet and divide is by
> the link speed.
> > >
> >
> > This latency values could be different for different phy's. So hardcoding will
> not work here.
> 
> But you do actually have hard coded defaults. Those odd hex values i pointed
> out.
> 
> By different PHYs do you mean different PHY versions? So you can look at
> register 2 and 3, determine what PHY it is, and so from that what defaults
> should be used? Or do you mean different boards with the same PHY?
> 
> In general, the less tunables you have, the better. If the driver can figure it out,
> it is better to not have DT properties. The PHY will then also work with ACPI
> and USB etc, where there is no DT. Implementing the user space API Richard
> pointed out will also allow your PHY to work with none DP systems.
> 

Sorry I answered wrong. Latency values vary depending on the position of PHY in board. 
We have used this PHY in different hardware's, where latency values differs based on PHY positioning. 
So we used latency option in DTS file.
If you have other ideas or I'm wrong please let me know?

> > Yes in our case latency values depends on port speed. It is delay
> > between network medium and PTP timestamp point.
> 
> What are the units. You generally have the units in the property name. So e.g.
> lan8814,latency_tx_1000_ns. If need be, the driver then converts to whatever
> value you place into the register.
> 
> If you do keep them, please make it clear that these values are optional, and
> state what value will be used when the property is not present.
> 

Yes units are Nanoseconds.

>         Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Andrew Lunn]

> > Thanks for the reply, but you did not answer my question:
> > 
> >   Does this mean the hardware itself cannot tell you it is missing the
> >   needed hardware?
> > 
> > Don't you have different IDs in register 2 and 3 for those devices with clock
> > register and those without?
> > 
> 

> The purpose of this option is, if both PHY and MAC supports
> timestamping then always timestamping is done in PHY.  If
> timestamping need to be done in MAC we need a way to stop PHY
> timestamping. If this flag is used then timestamping is taken care
> by MAC.

This is not a valid use of DT, since this is configuration, not
describing the hardware. There has been recent extension in the UAPI
to allow user space to do this configuration. Please look at that
work.

> Sorry I answered wrong. Latency values vary depending on the position of PHY in board. 
> We have used this PHY in different hardware's, where latency values differs based on PHY positioning. 
> So we used latency option in DTS file.
> If you have other ideas or I'm wrong please let me know?

So this is a function of the track length between the MAC and the PHY?
How do you determine these values? There is no point having
configuration values if you don't document how to determine what value
should be used.

       Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Tue, Mar 08, 2022 at 02:54:37PM +0100, Andrew Lunn wrote:
> This is not a valid use of DT, since this is configuration, not
> describing the hardware. There has been recent extension in the UAPI
> to allow user space to do this configuration. Please look at that
> work.

Yes, I had an RFC up that hopefully will merge soon.

In the mean time, just implement the PHC/time stamping in your PHY
driver unconditionally.

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Horatiu Vultur]

Hi Andrew,

The 03/08/2022 14:54, Andrew Lunn wrote:
> 
> > > Thanks for the reply, but you did not answer my question:
> > >
> > >   Does this mean the hardware itself cannot tell you it is missing the
> > >   needed hardware?
> > >
> > > Don't you have different IDs in register 2 and 3 for those devices with clock
> > > register and those without?
> > >
> >
> 
> > The purpose of this option is, if both PHY and MAC supports
> > timestamping then always timestamping is done in PHY.  If
> > timestamping need to be done in MAC we need a way to stop PHY
> > timestamping. If this flag is used then timestamping is taken care
> > by MAC.
> 
> This is not a valid use of DT, since this is configuration, not
> describing the hardware. There has been recent extension in the UAPI
> to allow user space to do this configuration. Please look at that
> work.

Ah ... now we have found Richard patch series.
So we will remove this option and once Richard's patch series will be
accepted we will use that.

> 
> > Sorry I answered wrong. Latency values vary depending on the position of PHY in board.
> > We have used this PHY in different hardware's, where latency values differs based on PHY positioning.
> > So we used latency option in DTS file.
> > If you have other ideas or I'm wrong please let me know?
> 
> So this is a function of the track length between the MAC and the PHY?

Nope.
This latency represents the time it takes for the frame to travel from RJ45
module to the timestamping unit inside the PHY. To be more precisely,
the timestamping unit will do the timestamp when it detects the end of
the start of the frame. So it represents the time from when the frame
reaches the RJ45 to when the end of start of the frame reaches the
timestamping unit inside the PHY.

And because each board manufacture could put the same PHY but in
different places, then each of them would have a different latency.
That is the main reason why we put this latencies in the DT and not put
them inside the driver. Because we think each board manufacture will
need to use different values.

Another reason is that we want the board manufacture to determine these
values and not the end users. I have seen that also Richard commenting
on this, saying that the latencies should not be in DT.
Currently I don't know where else they can be. I know that ptp4l has
these option in SW to update the ingress/egress latencies but if someone
else is running another application, what will they do?

> How do you determine these values?

This is a little bit more complicated.
So first you will need a device that you know already that is
calibrated. Then you connect the device that you want to calibrate to
the calibrated one with a known length cable. We presume that there is a
5ns delay per meter of the cable. And then basically we run ptp4l on
each device where the master will be the calibrated one and the slave
will be the device that will be calibrated. When we run ptp4l we can see
mean path delay, and we subtract the delay introduced by the cable(5ns)
and then we take this value and divided by 2. And then
the result is added to the current rx latency and subtracted from tx
latency.
This is how we have calculated the values.

> There is no point having
> configuration values if you don't document how to determine what value
> should be used.

I agree, we should do a better job at this and also explaining what
these values represent. Definitely we will do that in the next patch.

> 
>        Andrew

-- 
/Horatiu

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Andrew Lunn]

> > So this is a function of the track length between the MAC and the PHY?
> 
> Nope.
> This latency represents the time it takes for the frame to travel from RJ45
> module to the timestamping unit inside the PHY. To be more precisely,
> the timestamping unit will do the timestamp when it detects the end of
> the start of the frame. So it represents the time from when the frame
> reaches the RJ45 to when the end of start of the frame reaches the
> timestamping unit inside the PHY.

I must be missing something here. How do you measure the latency
difference for a 1 meter cable vs a 100m cable? Does 100m cable plus
1cm of track from the RJ45 to the PHY make a difference compared to
100m cable plus 1.5cm of track? Isn't this error all just in the
noise?

   Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Horatiu Vultur]

The 03/08/2022 19:10, Andrew Lunn wrote:
> 
> > > So this is a function of the track length between the MAC and the PHY?
> >
> > Nope.
> > This latency represents the time it takes for the frame to travel from RJ45
> > module to the timestamping unit inside the PHY. To be more precisely,
> > the timestamping unit will do the timestamp when it detects the end of
> > the start of the frame. So it represents the time from when the frame
> > reaches the RJ45 to when the end of start of the frame reaches the
> > timestamping unit inside the PHY.
> 
> I must be missing something here. How do you measure the latency
> difference for a 1 meter cable vs a 100m cable?

In the same way because the end result will be the same.
Lets presume that the cable introduce a 5ns latency per meter.
So, if we use a 1m cable and the mean path delay is 11, then
the latency is 11 - 5.
If we use a 100m cable then the mean path delay will be 506(if is not
506 then is something wrong) then the latency is 506 - 500.

> Does 100m cable plus 1cm of track from the RJ45 to the PHY make a difference
> compared to 100m cable plus 1.5cm of track?

In that case I don't think you will see any difference.

> Isn't this error all just in the noise?

I am not sure I follow this question.

> 
>    Andrew

-- 
/Horatiu

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Andrew Lunn]

On Tue, Mar 08, 2022 at 11:14:04PM +0100, Horatiu Vultur wrote:
> The 03/08/2022 19:10, Andrew Lunn wrote:
> > 
> > > > So this is a function of the track length between the MAC and the PHY?
> > >
> > > Nope.
> > > This latency represents the time it takes for the frame to travel from RJ45
> > > module to the timestamping unit inside the PHY. To be more precisely,
> > > the timestamping unit will do the timestamp when it detects the end of
> > > the start of the frame. So it represents the time from when the frame
> > > reaches the RJ45 to when the end of start of the frame reaches the
> > > timestamping unit inside the PHY.
> > 
> > I must be missing something here. How do you measure the latency
> > difference for a 1 meter cable vs a 100m cable?
> 
> In the same way because the end result will be the same.

The latency from the RJ45 to the PHY will be the same. But the latency
from the link peer PHY to the local PHY will be much more, 500ns. In
order for this RJ45 to PHY delay to be meaningful, don't you also need
to know the length of the cable? Is there a configuration knob
somewhere for the cable length?

I'm assuming the ptp protocol does not try to measure the cable delay,
since if it did, there would be no need to know the RJ45-PHY delay, it
would be part of that.

> > Isn't this error all just in the noise?
> 
> I am not sure I follow this question.

At minimum, you expect to have a 1m cable. The RJ45-PHY track length
is maybe 2cm? So 2% of the overall length. So you are trying to
correct the error this 2% causes. If you have a 100m cable, 0.02% is
RJ45-PHY part that you are trying to correct the error on. These
numbers seem so small, it seems pointless. It only seems to make sense
if you know the length of the cable, and to an accuracy of a few cm.

   Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Wed, Mar 09, 2022 at 12:36:54AM +0100, Andrew Lunn wrote:

> I'm assuming the ptp protocol does not try to measure the cable delay,
> since if it did, there would be no need to know the RJ45-PHY delay, it
> would be part of that.

The PTP does indeed measure the cable delay.  With a well tuned
system, you can tell the copper cable length directly from the
measured delay.

The problem with uncorrected PHY time stamps is that they affect the
boundary point between the node and the network.  A static error there
will create a path asymmetry that can neither be measured nor
corrected by the PTP, and a variable error degrades the time signal.


Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Tue, Mar 08, 2022 at 05:46:47PM -0800, Richard Cochran wrote:
> The problem with uncorrected PHY time stamps is that they affect the
> boundary point between the node and the network.  A static error there
> will create a path asymmetry that can neither be measured nor
> corrected by the PTP, and a variable error degrades the time signal.

And FWIW, the imperfections in the cable *also* introduce path
asymmetry and thus uncorrected offsets.  The twisted pairs are never
exactly the same length in both directions, for example.

However the PHY delays can be in the microseconds, but cable delay
deltas in the nanoseconds, so correcting PHY delay is much more
important.

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Horatiu Vultur]

The 03/09/2022 00:36, Andrew Lunn wrote:
> 
> On Tue, Mar 08, 2022 at 11:14:04PM +0100, Horatiu Vultur wrote:
> > The 03/08/2022 19:10, Andrew Lunn wrote:
> > >
> > > > > So this is a function of the track length between the MAC and the PHY?
> > > >
> > > > Nope.
> > > > This latency represents the time it takes for the frame to travel from RJ45
> > > > module to the timestamping unit inside the PHY. To be more precisely,
> > > > the timestamping unit will do the timestamp when it detects the end of
> > > > the start of the frame. So it represents the time from when the frame
> > > > reaches the RJ45 to when the end of start of the frame reaches the
> > > > timestamping unit inside the PHY.
> > >
> > > I must be missing something here. How do you measure the latency
> > > difference for a 1 meter cable vs a 100m cable?
> >
> > In the same way because the end result will be the same.
> 
> The latency from the RJ45 to the PHY will be the same. But the latency
> from the link peer PHY to the local PHY will be much more, 500ns. In
> order for this RJ45 to PHY delay to be meaningful, don't you also need
> to know the length of the cable? Is there a configuration knob
> somewhere for the cable length?
> 
> I'm assuming the ptp protocol does not try to measure the cable delay,
> since if it did, there would be no need to know the RJ45-PHY delay, it
> would be part of that.
> 
> > > Isn't this error all just in the noise?
> >
> > I am not sure I follow this question.
> 
> At minimum, you expect to have a 1m cable. The RJ45-PHY track length
> is maybe 2cm? So 2% of the overall length. So you are trying to
> correct the error this 2% causes. If you have a 100m cable, 0.02% is
> RJ45-PHY part that you are trying to correct the error on. These
> numbers seem so small, it seems pointless. It only seems to make sense
> if you know the length of the cable, and to an accuracy of a few cm.

I am not trying to adjust for the length of the cable.
If we have the following drawing:

 MAC                     PHY                    RJ45
-----       --------------------------       --------
|   |       |                        |       |       |
|   |<----->|timestamp | FIFO | GPHY |<----->|       |<------> Peer
|   |       |   unit                 |       |       |
-----       --------------------------       --------
                 ^                                   ^
                 |            latency                |
                 -------------------------------------

I am trying to calculate this latency, which includes a 2cm of track +
latency inside the PHY. As Richard mentioned also the PHY introduce some
latency which can be microseconds.

I understand if we consider that this latency should not be in the DT
and be part of the driver because the latency over the 2cm or 1.5cm of track
is almost nothing. But then what about the case when we want to add these
latencies to a MAC? They will depend on the latency inside the PHY so
those should come from DT.

So it really doesn't matter to me if I use a 1m cable or 100m cable.
What it matters is to see that mean path delay will be ~5ns for 1m cable
and ~500ns for 100m cable. And if is not, then I need to update the
register to calculate correctly the latency from RJ45 to timestamp unit
in the PHY.

> 

>    Andrew

-- 
/Horatiu

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Russell King (Oracle)]

On Wed, Mar 09, 2022 at 02:24:43PM +0100, Horatiu Vultur wrote:
> The 03/09/2022 00:36, Andrew Lunn wrote:
> > 
> > On Tue, Mar 08, 2022 at 11:14:04PM +0100, Horatiu Vultur wrote:
> > > The 03/08/2022 19:10, Andrew Lunn wrote:
> > > >
> > > > > > So this is a function of the track length between the MAC and the PHY?
> > > > >
> > > > > Nope.
> > > > > This latency represents the time it takes for the frame to travel from RJ45
> > > > > module to the timestamping unit inside the PHY. To be more precisely,
> > > > > the timestamping unit will do the timestamp when it detects the end of
> > > > > the start of the frame. So it represents the time from when the frame
> > > > > reaches the RJ45 to when the end of start of the frame reaches the
> > > > > timestamping unit inside the PHY.
> > > >
> > > > I must be missing something here. How do you measure the latency
> > > > difference for a 1 meter cable vs a 100m cable?
> > >
> > > In the same way because the end result will be the same.
> > 
> > The latency from the RJ45 to the PHY will be the same. But the latency
> > from the link peer PHY to the local PHY will be much more, 500ns. In
> > order for this RJ45 to PHY delay to be meaningful, don't you also need
> > to know the length of the cable? Is there a configuration knob
> > somewhere for the cable length?
> > 
> > I'm assuming the ptp protocol does not try to measure the cable delay,
> > since if it did, there would be no need to know the RJ45-PHY delay, it
> > would be part of that.
> > 
> > > > Isn't this error all just in the noise?
> > >
> > > I am not sure I follow this question.
> > 
> > At minimum, you expect to have a 1m cable. The RJ45-PHY track length
> > is maybe 2cm? So 2% of the overall length. So you are trying to
> > correct the error this 2% causes. If you have a 100m cable, 0.02% is
> > RJ45-PHY part that you are trying to correct the error on. These
> > numbers seem so small, it seems pointless. It only seems to make sense
> > if you know the length of the cable, and to an accuracy of a few cm.
> 
> I am not trying to adjust for the length of the cable.
> If we have the following drawing:
> 
>  MAC                     PHY                    RJ45
> -----       --------------------------       --------
> |   |       |                        |       |       |
> |   |<----->|timestamp | FIFO | GPHY |<----->|       |<------> Peer
> |   |       |   unit                 |       |       |
> -----       --------------------------       --------
>                  ^                                   ^
>                  |            latency                |
>                  -------------------------------------
> 
> I am trying to calculate this latency, which includes a 2cm of track +
> latency inside the PHY. As Richard mentioned also the PHY introduce some
> latency which can be microseconds.

I think we understand this, and compensating for the delay in the PHY
is quite reasonable, which surely will be a fixed amount irrespective
of the board.

However, Andrew's point is that the latency introduced by the copper
wire between the PHY and the RJ45 is insignificant, so insignificant
it's not worth bothering with - and I agree.

> I understand if we consider that this latency should not be in the DT
> and be part of the driver because the latency over the 2cm or 1.5cm of track
> is almost nothing. But then what about the case when we want to add these
> latencies to a MAC? They will depend on the latency inside the PHY so
> those should come from DT.

If you want to measure it to the MAC, then yes, the latency through
the PHY needs to be considered, and we probably need some new
interfaces inside the kernel so that MAC drivers can query phylib
to discover what the delay is. I don't think this is soemthing that
should be thrown into firmware, since the delay inside the PHY
should be constant (depending on what MAC side interface mode is
selected.)

Having it in firmware means that we're reliant on people ensuring
that they've looked up the right value for the PHY and its interface
mode not just once, but for every board out there - and if an error
is found, it brings up the question whether it should be corrected
on all boards or just one (and then there'll be questions why some
people have chosen randomly different values.)

> So it really doesn't matter to me if I use a 1m cable or 100m cable.
> What it matters is to see that mean path delay will be ~5ns for 1m cable
> and ~500ns for 100m cable. And if is not, then I need to update the
> register to calculate correctly the latency from RJ45 to timestamp unit
> in the PHY.

Does this mean you ask the user how long the cable is? Do you get them
to measure it to the nearest millimeter?

What about the overlap in the RJ45 connectors, and the height of the
pins in the RJ45? Some RJ45 connectors have staggered lengths for
their pins which would affect the true length. What about the total
length of the conductors in the RJ45 socket to the point that the
RJ45 plug makes contact? What happens if production then has to
change the make of RJ45 socket due to supply issues (which given
what is going on in the world at the moment is not unlikely.)

If you care about the 20mm or so on the board, then you ought to care
about all these other factors as well, and I suspect you're going to
be hard pressed to gather all that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Wed, Mar 09, 2022 at 02:55:49PM +0000, Russell King (Oracle) wrote:

> I think we understand this, and compensating for the delay in the PHY
> is quite reasonable, which surely will be a fixed amount irrespective
> of the board.

The PHY delays are not fixed.  They can be variable, even packet to packet.

https://www.researchgate.net/publication/260434179_Measurement_of_egress_and_ingress_delays_of_PTP_clocks

https://www.researchgate.net/publication/265731050_Experimental_verification_of_the_egress_and_ingress_latency_correction_in_PTP_clocks

Some PHYs are well behaved.  Some are not.

In any case, the linuxptp user space stack supports the standardized
method of correcting a system's delay asymmetry.  IMO it makes no
sense to even try to let kernel device drivers correct these delays.
Driver authors will get it wrong, and indeed they have already tried
and failed.  And when the magic numbers change from one kernel release
to another, it only makes the end user's job harder, because they will
have to update their scripts to correct the bogus numbers.

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Horatiu Vultur]

The 03/09/2022 11:52, Richard Cochran wrote:
> 
> On Wed, Mar 09, 2022 at 02:55:49PM +0000, Russell King (Oracle) wrote:
> 
> > I think we understand this, and compensating for the delay in the PHY
> > is quite reasonable, which surely will be a fixed amount irrespective
> > of the board.
> 
> The PHY delays are not fixed.  They can be variable, even packet to packet.
> 
> https://www.researchgate.net/publication/260434179_Measurement_of_egress_and_ingress_delays_of_PTP_clocks
> 
> https://www.researchgate.net/publication/265731050_Experimental_verification_of_the_egress_and_ingress_latency_correction_in_PTP_clocks
> 
> Some PHYs are well behaved.  Some are not.

What about adding only some sane values in the driver like here [1].
And the allow the user to use linuxptp to fine tune all this.

> 
> In any case, the linuxptp user space stack supports the standardized
> method of correcting a system's delay asymmetry.  IMO it makes no
> sense to even try to let kernel device drivers correct these delays.
> Driver authors will get it wrong, and indeed they have already tried
> and failed.  And when the magic numbers change from one kernel release
> to another, it only makes the end user's job harder, because they will
> have to update their scripts to correct the bogus numbers.
> 
> Thanks,
> Richard
> 

[1] https://elixir.bootlin.com/linux/v5.17-rc7/source/drivers/net/phy/mscc/mscc_ptp.c#L245

-- 
/Horatiu

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Fri, Mar 11, 2022 at 03:28:14PM +0100, Horatiu Vultur wrote:

> What about adding only some sane values in the driver like here [1].
> And the allow the user to use linuxptp to fine tune all this.

I mean, that is the point.  Users will surely have to tune it
themselves, second guessing the driver in any case.  So having hard
coded constants in the driver is useless.

Probably even the tuned values will differ by link speed, so having
the per-link speed constants in the driver doesn't help either.

(And yes, linuxptp should offer configuration variables per link
speed, monitor actual link speed, and switch automatically.  So far no
one is demanding that loudly)

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Allan W. Nielsen]

Hi Richard,

On Fri, Mar 11, 2022 at 07:08:42AM -0800, Richard Cochran wrote:
> On Fri, Mar 11, 2022 at 03:28:14PM +0100, Horatiu Vultur wrote:
> 
> > What about adding only some sane values in the driver like here [1].
> > And the allow the user to use linuxptp to fine tune all this.
> 
> I mean, that is the point.  Users will surely have to tune it
> themselves, second guessing the driver in any case.  So having hard
> coded constants in the driver is useless.
> 
> Probably even the tuned values will differ by link speed, so having
> the per-link speed constants in the driver doesn't help either.
> 
> (And yes, linuxptp should offer configuration variables per link
> speed, monitor actual link speed, and switch automatically.  So far no
> one is demanding that loudly)

I did skim through the articles, and as you hinted he does find small
latency differences across different packets. (but as I understood, very
few PHYs was tested).

Also, I know that we (Vitesse -> Microsemi -> Microchip) have been
offering ways to calibrate the individual PHYs in other PTP-SW products.
So, this makes good sense.

With this in mind, I do agree with you that it does not make much sense
to compensate they few cm of PCB tracks without also calibrating for
differences from packet to packet.

But this is not really an argument for not having _default_ values
hard-coded in the driver (or DT, but lets forget about DT for now).
Looking at the default compensation numbers provided in the driver, they
are a lot larger than what we expect to find in calibration.

As pointed out by other, most users will not care about the small error
introduced by the few cm PCB track. My claim is that if we provide
default hard-coded delay values in the driver, most users will not care
about the few ns noise that each packet differs. And those who do care,
have all the hooks and handle to calibrate further by using PTP4L.

If we do not offer default delays directly in the driver, everybody will
have to calibrate all boards just to have decent results, we will not
have a good way to provide default delay numbers, and this will be
different from what is done in other drivers.

I do understand that you have a concern that these numbers may change in
future updates. But this has not been a problem in other drivers doing
the same. But if this is still a concern, we can add a comment to say
that these numbers must be treated as UAPI, and chancing them, may
cause regressions on calibrated PHYs.

Long story short, I can see any real down-sides of adding these delay
numbers, and I see plenty in not doing so.

/Allan

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Sat, Mar 12, 2022 at 08:36:20PM +0100, Allan W. Nielsen wrote:
> With this in mind, I do agree with you that it does not make much sense
> to compensate they few cm of PCB tracks without also calibrating for
> differences from packet to packet.

The PCB traces AFTER the PHY are part of the network, and if they
contribute to path asymmetry, then that can and should be corrected
using the delayAsymmetry configuration variable.
 
> If we do not offer default delays directly in the driver, everybody will
> have to calibrate all boards just to have decent results, we will not
> have a good way to provide default delay numbers, and this will be
> different from what is done in other drivers.

Who says the other drivers are even remotely reasonable?  Not me.
I've been fighting this voodoo engineering all along, but people seem
to ignore me.

> I do understand that you have a concern that these numbers may change in
> future updates. But this has not been a problem in other drivers doing
> the same.

Wrong.  See the git history of the i210 driver.  Also the data sheets.

> But if this is still a concern, we can add a comment to say
> that these numbers must be treated as UAPI, and chancing them, may
> cause regressions on calibrated PHYs.

Comments will be ignored.  And when the next batch of developers comes
along, they will ignore your prohibition.

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Sat, Mar 12, 2022 at 08:36:20PM +0100, Allan W. Nielsen wrote:
> I did skim through the articles, and as you hinted he does find small
> latency differences across different packets. (but as I understood, very
> few PHYs was tested).

There is also previous work cited in those articles.
 
> But this is not really an argument for not having _default_ values
> hard-coded in the driver (or DT, but lets forget about DT for now).

You put them in the DTS.  That means you expect them to need changes.

DTS is the WRONG place for such things.

If your numbers are perfect, then do the corrections in silicon/firmware.

If the numbers aren't 100% perfect, then provide your customers with a
test report providing the recommended numbers.  Include a proper
explanation of the test methodology and assumptions used in your
analysis.  Heck, you can even given them linuxptp config snippets (and
other for other popular PTP stacks, Oregano, ixat, ptpd, etc)

Don't hard code random, changing numbers into kernel drivers.

Thanks,
Richard

RE: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Woojung.Huh]

Hi Richard,

Not sure that it is good idea to reply on not-the-latest thread.

> -----Original Message-----
> From: Richard Cochran <richardcochran@gmail.com>
> Sent: Wednesday, March 9, 2022 2:53 PM
> To: Russell King (Oracle) <linux@armlinux.org.uk>
> Cc: Horatiu Vultur - M31836 <Horatiu.Vultur@microchip.com>; Andrew Lunn
> <andrew@lunn.ch>; Divya Koppera - I30481
> <Divya.Koppera@microchip.com>; netdev@vger.kernel.org;
> hkallweit1@gmail.com; davem@davemloft.net; kuba@kernel.org;
> robh+dt@kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Madhuri Sripada - I34878
> <Madhuri.Sripada@microchip.com>; Manohar Puri - I30488
> <Manohar.Puri@microchip.com>
> Subject: Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency
> values and timestamping check for LAN8814 phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Wed, Mar 09, 2022 at 02:55:49PM +0000, Russell King (Oracle) wrote:
> 
> > I think we understand this, and compensating for the delay in the PHY
> > is quite reasonable, which surely will be a fixed amount irrespective
> > of the board.
> 
> The PHY delays are not fixed.  They can be variable, even packet to packet.
> 
> https://www.researchgate.net/publication/260434179_Measurement_of_e
> gress_and_ingress_delays_of_PTP_clocks
> 
> https://www.researchgate.net/publication/265731050_Experimental_verific
> ation_of_the_egress_and_ingress_latency_correction_in_PTP_clocks
> 
> Some PHYs are well behaved.  Some are not.
> 
> In any case, the linuxptp user space stack supports the standardized
> method of correcting a system's delay asymmetry.  IMO it makes no
> sense to even try to let kernel device drivers correct these delays.
> Driver authors will get it wrong, and indeed they have already tried
> and failed.  And when the magic numbers change from one kernel release
> to another, it only makes the end user's job harder, because they will
> have to update their scripts to correct the bogus numbers.
> 

If you are referring to the delayAsymmetry of ptp4l, I think that is different from this latency value.
delayAsymmetry of ptp4l says "The time difference in nanoseconds of the transmit and receive  paths. 
This value should be positive when the master-to-slave propagation time is longer and negative
when the slave-to-master time is longer. The default is 0 nanoseconds."
In my understanding, master-to-slave uses reference timestamp which is defined in IEEE specs.
   <egressTimestamp> = <egressProvidedTimestamp> + <egressLatency>
   <ingressTimestamp> = <ingressProvidedTimestamp> - <ingressLatency>

These latency is egreeLatency or ingressLatency to get accurate timestamp at reference point from 
timestamp of clock in MAC or PHY.
So, this latency should (hopefully) be not-much-change in the same board after manufactured. 
But, value can be different from design to design and port to port if some path (PHY to RJ45) is longer than others.

This doesn't cover any latency from cable length and/or asymmetry which may come from RJ45-to-RJ45.
But, delayAsymmetry may care cable type/length in application point of view.

Of cause, all values may be small enough to ignore though.
Do I miss something here?

Thanks.
Woojung

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Fri, Mar 11, 2022 at 03:21:58PM +0000, Woojung.Huh@microchip.com wrote:

> If you are referring to the delayAsymmetry of ptp4l,

No, really, I'm not.

       PTP4l(8)                    System Manager's Manual                   PTP4l(8)

       NAME
           ptp4l - PTP Boundary/Ordinary/Transparent Clock

       ...

       egressLatency
              Specifies  the  difference  in  nanoseconds  between  the actual
              transmission time at the reference plane and the reported trans‐
              mit  time  stamp. This value will be added to egress time stamps
              obtained from the hardware.  The default is 0.

       ingressLatency
              Specifies the difference in nanoseconds between the reported re‐
              ceive  time  stamp  and  the  actual reception time at reference
              plane. This value will be subtracted from  ingress  time  stamps
              obtained from the hardware.  The default is 0.

> So, this latency should (hopefully) be not-much-change in the same board after manufactured. 

Please read the papers on this topic.  I posted links in another reply
in this thread.

> Of cause, all values may be small enough to ignore though.
> Do I miss something here?

Yes, you miss the point entirely.  PHY delays are relatively large and
cannot be even measured in some cases.  However, for well behaved
PHYs, the user space stack already covers the configuration.

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Andrew Lunn]

>        PTP4l(8)                    System Manager's Manual                   PTP4l(8)
> 
>        NAME
>            ptp4l - PTP Boundary/Ordinary/Transparent Clock
> 
>        ...
> 
>        egressLatency
>               Specifies  the  difference  in  nanoseconds  between  the actual
>               transmission time at the reference plane and the reported trans‐
>               mit  time  stamp. This value will be added to egress time stamps
>               obtained from the hardware.  The default is 0.
> 
>        ingressLatency
>               Specifies the difference in nanoseconds between the reported re‐
>               ceive  time  stamp  and  the  actual reception time at reference
>               plane. This value will be subtracted from  ingress  time  stamps
>               obtained from the hardware.  The default is 0.
> 

Hi Richard

Do these get passed to the kernel so the hardware can act on them, or
are they used purely in userspace by ptp4l?

If they has passed to the kernel, could we provide a getter as well as
a setter, so the defaults hard coded in the driver can be read back?

	Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Sat, Mar 12, 2022 at 09:04:31PM +0100, Andrew Lunn wrote:
> Do these get passed to the kernel so the hardware can act on them, or
> are they used purely in userspace by ptp4l?

user space only.
 
> If they has passed to the kernel, could we provide a getter as well as
> a setter, so the defaults hard coded in the driver can be read back?

Any hard coded defaults in the kernel are a nuisance.

I mean, do you want user space to say,

   "okay, so I know the correct value is X.  But the drivers may offer
   random values according to kernel version.  So, I'll read out the
   driver value Y, and then apply X-Y."

Insanity.

(not to mention that this fails for older kernels without the proposed
interface)

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Andrew Lunn]

On Sat, Mar 12, 2022 at 06:46:46PM -0800, Richard Cochran wrote:
> On Sat, Mar 12, 2022 at 09:04:31PM +0100, Andrew Lunn wrote:
> > Do these get passed to the kernel so the hardware can act on them, or
> > are they used purely in userspace by ptp4l?
> 
> user space only.
>  
> > If they has passed to the kernel, could we provide a getter as well as
> > a setter, so the defaults hard coded in the driver can be read back?
> 
> Any hard coded defaults in the kernel are a nuisance.
> 
> I mean, do you want user space to say,
> 
>    "okay, so I know the correct value is X.  But the drivers may offer
>    random values according to kernel version.  So, I'll read out the
>    driver value Y, and then apply X-Y."
> 
> Insanity.

No, i would not suggests that at all.

You quoted the man page and it says the default it zero. If there was
an API to ask the driver what correction it is doing, and an API to
offload the delay correction to the hardware, i would simply remove
the comment about the default being zero. If these calls return
-EOPNOTSUPP, then user space stays the same, and does actually use a
default of 0. If offload is supported, you can show the user the
current absolute values, and allow the user to set the absolute
values.

Anyway, it is clear you don't want the driver doing any correction, so
lets stop this discussion.

     Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Allan W. Nielsen]

On Sun, Mar 13, 2022 at 04:07:24PM +0100, Andrew Lunn wrote:
> On Sat, Mar 12, 2022 at 06:46:46PM -0800, Richard Cochran wrote:
> > On Sat, Mar 12, 2022 at 09:04:31PM +0100, Andrew Lunn wrote:
> > > Do these get passed to the kernel so the hardware can act on them, or
> > > are they used purely in userspace by ptp4l?
> >
> > user space only.
I'm wondering if one-step will work if these correction values are not
applied to HW.

> > > If they has passed to the kernel, could we provide a getter as well as
> > > a setter, so the defaults hard coded in the driver can be read back?
> >
> > Any hard coded defaults in the kernel are a nuisance.
> >
> > I mean, do you want user space to say,
> >
> >    "okay, so I know the correct value is X.  But the drivers may offer
> >    random values according to kernel version.  So, I'll read out the
> >    driver value Y, and then apply X-Y."
> >
> > Insanity.
> 
> No, i would not suggests that at all.
> 
> You quoted the man page and it says the default it zero. If there was
> an API to ask the driver what correction it is doing, and an API to
> offload the delay correction to the hardware, i would simply remove
> the comment about the default being zero. If these calls return
> -EOPNOTSUPP, then user space stays the same, and does actually use a
> default of 0. If offload is supported, you can show the user the
> current absolute values, and allow the user to set the absolute
> values.
This sounds like a good approach to me (but I know it is not my opinion
you are asking for).

In all cases, if there is a desire to have such APIs, and let drivers
advertise default compensation values in this way, we can work on that.

> Anyway, it is clear you don't want the driver doing any correction, so
> lets stop this discussion.
> 
>      Andrew

-- 
/Allan

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Sun, Mar 13, 2022 at 08:37:44PM +0100, Allan W. Nielsen wrote:
> On Sun, Mar 13, 2022 at 04:07:24PM +0100, Andrew Lunn wrote:
> > On Sat, Mar 12, 2022 at 06:46:46PM -0800, Richard Cochran wrote:
> > > On Sat, Mar 12, 2022 at 09:04:31PM +0100, Andrew Lunn wrote:
> > > > Do these get passed to the kernel so the hardware can act on them, or
> > > > are they used purely in userspace by ptp4l?
> > >
> > > user space only.
> I'm wondering if one-step will work if these correction values are not
> applied to HW.

They are applied to the time stamps that are available to the
program.  So, no, they obviously won't be applied to one step Sync.
But then again, neither will the driver values.

(You could imagine a HW tstamp unit that includes a correction factor,
for example by adding the egress time stamp value to the correction
field.  But there are no APIs for that, and maybe no HW either.)

(BTW one step is overrated IMO)

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Richard Cochran]

On Sun, Mar 13, 2022 at 04:07:24PM +0100, Andrew Lunn wrote:
> Anyway, it is clear you don't want the driver doing any correction, so
> lets stop this discussion.

Okay.  Just to be clear, as an end user, I've already been burnt by
well meaning but false corrections in the driver.  As maintainer I
must advocate for the end users.

Thanks,
Richard

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Horatiu Vultur]

The 03/09/2022 14:55, Russell King (Oracle) wrote:
> 
> On Wed, Mar 09, 2022 at 02:24:43PM +0100, Horatiu Vultur wrote:
> > The 03/09/2022 00:36, Andrew Lunn wrote:
> > >
> > > On Tue, Mar 08, 2022 at 11:14:04PM +0100, Horatiu Vultur wrote:
> > > > The 03/08/2022 19:10, Andrew Lunn wrote:
> > > > >
> > > > > > > So this is a function of the track length between the MAC and the PHY?
> > > > > >
> > > > > > Nope.
> > > > > > This latency represents the time it takes for the frame to travel from RJ45
> > > > > > module to the timestamping unit inside the PHY. To be more precisely,
> > > > > > the timestamping unit will do the timestamp when it detects the end of
> > > > > > the start of the frame. So it represents the time from when the frame
> > > > > > reaches the RJ45 to when the end of start of the frame reaches the
> > > > > > timestamping unit inside the PHY.
> > > > >
> > > > > I must be missing something here. How do you measure the latency
> > > > > difference for a 1 meter cable vs a 100m cable?
> > > >
> > > > In the same way because the end result will be the same.
> > >
> > > The latency from the RJ45 to the PHY will be the same. But the latency
> > > from the link peer PHY to the local PHY will be much more, 500ns. In
> > > order for this RJ45 to PHY delay to be meaningful, don't you also need
> > > to know the length of the cable? Is there a configuration knob
> > > somewhere for the cable length?
> > >
> > > I'm assuming the ptp protocol does not try to measure the cable delay,
> > > since if it did, there would be no need to know the RJ45-PHY delay, it
> > > would be part of that.
> > >
> > > > > Isn't this error all just in the noise?
> > > >
> > > > I am not sure I follow this question.
> > >
> > > At minimum, you expect to have a 1m cable. The RJ45-PHY track length
> > > is maybe 2cm? So 2% of the overall length. So you are trying to
> > > correct the error this 2% causes. If you have a 100m cable, 0.02% is
> > > RJ45-PHY part that you are trying to correct the error on. These
> > > numbers seem so small, it seems pointless. It only seems to make sense
> > > if you know the length of the cable, and to an accuracy of a few cm.
> >
> > I am not trying to adjust for the length of the cable.
> > If we have the following drawing:
> >
> >  MAC                     PHY                    RJ45
> > -----       --------------------------       --------
> > |   |       |                        |       |       |
> > |   |<----->|timestamp | FIFO | GPHY |<----->|       |<------> Peer
> > |   |       |   unit                 |       |       |
> > -----       --------------------------       --------
> >                  ^                                   ^
> >                  |            latency                |
> >                  -------------------------------------
> >
> > I am trying to calculate this latency, which includes a 2cm of track +
> > latency inside the PHY. As Richard mentioned also the PHY introduce some
> > latency which can be microseconds.
> 
> I think we understand this, and compensating for the delay in the PHY
> is quite reasonable, which surely will be a fixed amount irrespective
> of the board.
> 
> However, Andrew's point is that the latency introduced by the copper
> wire between the PHY and the RJ45 is insignificant, so insignificant
> it's not worth bothering with - and I agree.

OK, that is fine for me, we can ignore the latency introduced by the
copper wire.

> 
> > I understand if we consider that this latency should not be in the DT
> > and be part of the driver because the latency over the 2cm or 1.5cm of track
> > is almost nothing. But then what about the case when we want to add these
> > latencies to a MAC? They will depend on the latency inside the PHY so
> > those should come from DT.
> 
> If you want to measure it to the MAC, then yes, the latency through
> the PHY needs to be considered, and we probably need some new
> interfaces inside the kernel so that MAC drivers can query phylib
> to discover what the delay is.

Does that mean that each PHY needs to implement a new API? Because that
would be a little bit of work to do there.

> I don't think this is soemthing that
> should be thrown into firmware, since the delay inside the PHY
> should be constant (depending on what MAC side interface mode is
> selected.)
> 
> Having it in firmware means that we're reliant on people ensuring
> that they've looked up the right value for the PHY and its interface
> mode not just once, but for every board out there - and if an error
> is found, it brings up the question whether it should be corrected
> on all boards or just one (and then there'll be questions why some
> people have chosen randomly different values.)
> 
> > So it really doesn't matter to me if I use a 1m cable or 100m cable.
> > What it matters is to see that mean path delay will be ~5ns for 1m cable
> > and ~500ns for 100m cable. And if is not, then I need to update the
> > register to calculate correctly the latency from RJ45 to timestamp unit
> > in the PHY.
> 
> Does this mean you ask the user how long the cable is? Do you get them
> to measure it to the nearest millimeter?

My expectation is that the end user should not care about the length of
the cable. He just needs to start ptp4l and have some sane results.
Only the board manufacture were supposed to know the length of the
cable to set their latency values.

> 
> What about the overlap in the RJ45 connectors, and the height of the
> pins in the RJ45? Some RJ45 connectors have staggered lengths for
> their pins which would affect the true length. What about the total
> length of the conductors in the RJ45 socket to the point that the
> RJ45 plug makes contact? What happens if production then has to
> change the make of RJ45 socket due to supply issues (which given
> what is going on in the world at the moment is not unlikely.)

If they don't introduce any significat latency then we can ignore them.

> 
> If you care about the 20mm or so on the board, then you ought to care
> about all these other factors as well, and I suspect you're going to
> be hard pressed to gather all that.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

-- 
/Horatiu

Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

[Rob Herring]

On Fri, Mar 04, 2022 at 03:04:17PM +0530, Divya Koppera wrote:
> Supports configuring latency values and also adds
> check for phy timestamping feature.
> 
> Signed-off-by: Divya Koppera<Divya.Koppera@microchip.com>

should be a space here:       ^

> ---
>  .../devicetree/bindings/net/micrel.txt          | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Please convert this to DT schema.

> 
> diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> index 8d157f0295a5..c5ab62c39133 100644
> --- a/Documentation/devicetree/bindings/net/micrel.txt
> +++ b/Documentation/devicetree/bindings/net/micrel.txt
> @@ -45,3 +45,20 @@ Optional properties:
>  
>  	In fiber mode, auto-negotiation is disabled and the PHY can only work in
>  	100base-fx (full and half duplex) modes.
> +
> + - lan8814,ignore-ts: If present the PHY will not support timestamping.

'lan8814' is not a vendor and the format for properties is 
<vendor>,<propname>.

Is this configuration or lack of h/w feature? IOW, instead of 'will 
not', 'does not' or 'timestamping is disabled.'. As configuration, that 
seems like common property. For (lack of) h/w features, that should be 
implied by the compatible or VID/PID.

> +	This option acts as check whether Timestamping is supported by
> +	hardware or not. LAN8814 phy support hardware tmestamping.
> +
> + - lan8814,latency_rx_10: Configures Latency value of phy in ingress at 10 Mbps.

s/_/-/

What are the units here? Is this a common feature of PHYs?

> +
> + - lan8814,latency_tx_10: Configures Latency value of phy in egress at 10 Mbps.
> +
> + - lan8814,latency_rx_100: Configures Latency value of phy in ingress at 100 Mbps.
> +
> + - lan8814,latency_tx_100: Configures Latency value of phy in egress at 100 Mbps.
> +
> + - lan8814,latency_rx_1000: Configures Latency value of phy in ingress at 1000 Mbps.
> +
> + - lan8814,latency_tx_1000: Configures Latency value of phy in egress at 1000 Mbps.
> -- 
> 2.17.1
> 
> 

[PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814 phy

[Divya Koppera]

Add support for 1588 in LAN8814 phy driver.
It supports 1-step and 2-step timestamping.

Co-developed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
---
 drivers/net/phy/micrel.c | 1088 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 1066 insertions(+), 22 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 281cebc3d00c..81a76322254c 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -28,6 +28,10 @@
 #include <linux/of.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/ptp_clock.h>
+#include <linux/ptp_classify.h>
+#include <linux/net_tstamp.h>
 
 /* Operation Mode Strap Override */
 #define MII_KSZPHY_OMSO				0x16
@@ -79,6 +83,119 @@
 #define LAN8814_INTR_CTRL_REG_POLARITY		BIT(1)
 #define LAN8814_INTR_CTRL_REG_INTR_ENABLE	BIT(0)
 
+/* Represents 1ppm adjustment in 2^32 format with
+ * each nsec contains 4 clock cycles.
+ * The value is calculated as following: (1/1000000)/((2^-32)/4)
+ */
+#define LAN8814_1PPM_FORMAT			17179
+
+#define PTP_RX_MOD				0x024F
+#define PTP_RX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_ BIT(3)
+#define PTP_RX_TIMESTAMP_EN			0x024D
+#define PTP_TX_TIMESTAMP_EN			0x028D
+
+#define PTP_TIMESTAMP_EN_SYNC_			BIT(0)
+#define PTP_TIMESTAMP_EN_DREQ_			BIT(1)
+#define PTP_TIMESTAMP_EN_PDREQ_			BIT(2)
+#define PTP_TIMESTAMP_EN_PDRES_			BIT(3)
+
+#define PTP_RX_LATENCY_1000			0x0224
+#define PTP_TX_LATENCY_1000			0x0225
+
+#define PTP_RX_LATENCY_100			0x0222
+#define PTP_TX_LATENCY_100			0x0223
+
+#define PTP_RX_LATENCY_10			0x0220
+#define PTP_TX_LATENCY_10			0x0221
+
+#define PTP_TX_PARSE_L2_ADDR_EN			0x0284
+#define PTP_RX_PARSE_L2_ADDR_EN			0x0244
+
+#define PTP_TX_PARSE_IP_ADDR_EN			0x0285
+#define PTP_RX_PARSE_IP_ADDR_EN			0x0245
+#define LTC_HARD_RESET				0x023F
+#define LTC_HARD_RESET_				BIT(0)
+
+#define TSU_HARD_RESET				0x02C1
+#define TSU_HARD_RESET_				BIT(0)
+
+#define PTP_CMD_CTL				0x0200
+#define PTP_CMD_CTL_PTP_DISABLE_		BIT(0)
+#define PTP_CMD_CTL_PTP_ENABLE_			BIT(1)
+#define PTP_CMD_CTL_PTP_CLOCK_READ_		BIT(3)
+#define PTP_CMD_CTL_PTP_CLOCK_LOAD_		BIT(4)
+#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_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_MID			0x022A
+#define PTP_CLOCK_READ_SEC_LO			0x022B
+#define PTP_CLOCK_READ_NS_HI			0x022C
+#define PTP_CLOCK_READ_NS_LO			0x022D
+
+#define PTP_OPERATING_MODE			0x0241
+#define PTP_OPERATING_MODE_STANDALONE_		BIT(0)
+
+#define PTP_TX_MOD				0x028F
+#define PTP_TX_MOD_TX_PTP_SYNC_TS_INSERT_	BIT(12)
+#define PTP_TX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_ BIT(3)
+
+#define PTP_RX_PARSE_CONFIG			0x0242
+#define PTP_RX_PARSE_CONFIG_LAYER2_EN_		BIT(0)
+#define PTP_RX_PARSE_CONFIG_IPV4_EN_		BIT(1)
+#define PTP_RX_PARSE_CONFIG_IPV6_EN_		BIT(2)
+
+#define PTP_TX_PARSE_CONFIG			0x0282
+#define PTP_TX_PARSE_CONFIG_LAYER2_EN_		BIT(0)
+#define PTP_TX_PARSE_CONFIG_IPV4_EN_		BIT(1)
+#define PTP_TX_PARSE_CONFIG_IPV6_EN_		BIT(2)
+
+#define PTP_CLOCK_RATE_ADJ_HI			0x020C
+#define PTP_CLOCK_RATE_ADJ_LO			0x020D
+#define PTP_CLOCK_RATE_ADJ_DIR_			BIT(15)
+
+#define PTP_LTC_STEP_ADJ_HI			0x0212
+#define PTP_LTC_STEP_ADJ_LO			0x0213
+#define PTP_LTC_STEP_ADJ_DIR_			BIT(15)
+
+#define LAN8814_INTR_STS_REG			0x0033
+#define LAN8814_INTR_STS_REG_1588_TSU0_		BIT(0)
+#define LAN8814_INTR_STS_REG_1588_TSU1_		BIT(1)
+#define LAN8814_INTR_STS_REG_1588_TSU2_		BIT(2)
+#define LAN8814_INTR_STS_REG_1588_TSU3_		BIT(3)
+
+#define PTP_CAP_INFO				0x022A
+#define PTP_CAP_INFO_TX_TS_CNT_GET_(reg_val)	(((reg_val) & 0x0f00) >> 8)
+#define PTP_CAP_INFO_RX_TS_CNT_GET_(reg_val)	((reg_val) & 0x000f)
+
+#define PTP_TX_EGRESS_SEC_HI			0x0296
+#define PTP_TX_EGRESS_SEC_LO			0x0297
+#define PTP_TX_EGRESS_NS_HI			0x0294
+#define PTP_TX_EGRESS_NS_LO			0x0295
+#define PTP_TX_MSG_HEADER2			0x0299
+
+#define PTP_RX_INGRESS_SEC_HI			0x0256
+#define PTP_RX_INGRESS_SEC_LO			0x0257
+#define PTP_RX_INGRESS_NS_HI			0x0254
+#define PTP_RX_INGRESS_NS_LO			0x0255
+#define PTP_RX_MSG_HEADER2			0x0259
+
+#define PTP_TSU_INT_EN				0x0200
+#define PTP_TSU_INT_EN_PTP_TX_TS_OVRFL_EN_	BIT(3)
+#define PTP_TSU_INT_EN_PTP_TX_TS_EN_		BIT(2)
+#define PTP_TSU_INT_EN_PTP_RX_TS_OVRFL_EN_	BIT(1)
+#define PTP_TSU_INT_EN_PTP_RX_TS_EN_		BIT(0)
+
+#define PTP_TSU_INT_STS				0x0201
+#define PTP_TSU_INT_STS_PTP_TX_TS_OVRFL_INT_	BIT(3)
+#define PTP_TSU_INT_STS_PTP_TX_TS_EN_		BIT(2)
+#define PTP_TSU_INT_STS_PTP_RX_TS_OVRFL_INT_	BIT(1)
+#define PTP_TSU_INT_STS_PTP_RX_TS_EN_		BIT(0)
+
 /* PHY Control 1 */
 #define MII_KSZPHY_CTRL_1			0x1e
 #define KSZ8081_CTRL1_MDIX_STAT			BIT(4)
@@ -108,6 +225,7 @@
 #define MII_KSZPHY_TX_DATA_PAD_SKEW		0x106
 
 #define PS_TO_REG				200
+#define FIFO_SIZE				8
 
 struct kszphy_hw_stat {
 	const char *string;
@@ -128,7 +246,57 @@ struct kszphy_type {
 	bool has_rmii_ref_clk_sel;
 };
 
+/* Shared structure between the PHYs of the same package. */
+struct lan8814_shared_priv {
+	struct phy_device *phydev;
+	struct ptp_clock *ptp_clock;
+	struct ptp_clock_info ptp_clock_info;
+
+	/* Reference counter to how many ports in the package are enabling the
+	 * timestamping
+	 */
+	u8 ref;
+
+	/* Lock for ptp_clock and ref */
+	struct mutex shared_lock;
+};
+
+struct lan8814_ptp_rx_ts {
+	struct list_head list;
+	u32 seconds;
+	u32 nsec;
+	u16 seq_id;
+};
+
+struct kszphy_latencies {
+	u16 rx_10;
+	u16 tx_10;
+	u16 rx_100;
+	u16 tx_100;
+	u16 rx_1000;
+	u16 tx_1000;
+};
+
+struct kszphy_ptp_priv {
+	struct mii_timestamper mii_ts;
+	struct phy_device *phydev;
+
+	struct sk_buff_head tx_queue;
+	struct sk_buff_head rx_queue;
+
+	struct list_head rx_ts_list;
+	/* Lock for Rx ts fifo */
+	spinlock_t rx_ts_lock;
+
+	int hwts_tx_type;
+	enum hwtstamp_rx_filters rx_filter;
+	int layer;
+	int version;
+};
+
 struct kszphy_priv {
+	struct kszphy_ptp_priv ptp_priv;
+	struct kszphy_latencies latencies;
 	const struct kszphy_type *type;
 	int led_mode;
 	bool rmii_ref_clk_sel;
@@ -136,6 +304,14 @@ struct kszphy_priv {
 	u64 stats[ARRAY_SIZE(kszphy_hw_stats)];
 };
 
+static struct kszphy_latencies lan8814_latencies = {
+	.rx_10		= 0x22AA,
+	.tx_10		= 0x2E4A,
+	.rx_100		= 0x092A,
+	.tx_100		= 0x02C1,
+	.rx_1000	= 0x01AD,
+	.tx_1000	= 0x00C9,
+};
 static const struct kszphy_type ksz8021_type = {
 	.led_mode_reg		= MII_KSZPHY_CTRL_2,
 	.has_broadcast_disable	= true,
@@ -1624,29 +1800,667 @@ static int lanphy_write_page_reg(struct phy_device *phydev, int page, u16 addr,
 	return val;
 }
 
-static int lan8814_config_init(struct phy_device *phydev)
+static int lan8814_config_ts_intr(struct phy_device *phydev, bool enable)
 {
-	int val;
+	u16 val = 0;
 
-	/* Reset the PHY */
-	val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
-	val |= LAN8814_QSGMII_SOFT_RESET_BIT;
-	lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
+	if (enable)
+		val = PTP_TSU_INT_EN_PTP_TX_TS_EN_ |
+		      PTP_TSU_INT_EN_PTP_TX_TS_OVRFL_EN_ |
+		      PTP_TSU_INT_EN_PTP_RX_TS_EN_ |
+		      PTP_TSU_INT_EN_PTP_RX_TS_OVRFL_EN_;
 
-	/* Disable ANEG with QSGMII PCS Host side */
-	val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
-	val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
-	lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
+	return lanphy_write_page_reg(phydev, 5, PTP_TSU_INT_EN, val);
+}
 
-	/* MDI-X setting for swap A,B transmit */
-	val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
-	val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
-	val |= LAN8814_ALIGN_TX_A_B_SWAP;
-	lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
+static void lan8814_ptp_rx_ts_get(struct phy_device *phydev,
+				  u32 *seconds, u32 *nano_seconds, u16 *seq_id)
+{
+	*seconds = lanphy_read_page_reg(phydev, 5, PTP_RX_INGRESS_SEC_HI);
+	*seconds = (*seconds << 16) |
+		   lanphy_read_page_reg(phydev, 5, PTP_RX_INGRESS_SEC_LO);
+
+	*nano_seconds = lanphy_read_page_reg(phydev, 5, PTP_RX_INGRESS_NS_HI);
+	*nano_seconds = ((*nano_seconds & 0x3fff) << 16) |
+			lanphy_read_page_reg(phydev, 5, PTP_RX_INGRESS_NS_LO);
+
+	*seq_id = lanphy_read_page_reg(phydev, 5, PTP_RX_MSG_HEADER2);
+}
+
+static void lan8814_ptp_tx_ts_get(struct phy_device *phydev,
+				  u32 *seconds, u32 *nano_seconds, u16 *seq_id)
+{
+	*seconds = lanphy_read_page_reg(phydev, 5, PTP_TX_EGRESS_SEC_HI);
+	*seconds = *seconds << 16 |
+		   lanphy_read_page_reg(phydev, 5, PTP_TX_EGRESS_SEC_LO);
+
+	*nano_seconds = lanphy_read_page_reg(phydev, 5, PTP_TX_EGRESS_NS_HI);
+	*nano_seconds = ((*nano_seconds & 0x3fff) << 16) |
+			lanphy_read_page_reg(phydev, 5, PTP_TX_EGRESS_NS_LO);
+
+	*seq_id = lanphy_read_page_reg(phydev, 5, PTP_TX_MSG_HEADER2);
+}
+
+static int lan8814_ts_info(struct mii_timestamper *mii_ts, struct ethtool_ts_info *info)
+{
+	struct kszphy_ptp_priv *ptp_priv = container_of(mii_ts, struct kszphy_ptp_priv, mii_ts);
+	struct phy_device *phydev = ptp_priv->phydev;
+	struct lan8814_shared_priv *shared = phydev->shared->priv;
+
+	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
+				SOF_TIMESTAMPING_RX_HARDWARE |
+				SOF_TIMESTAMPING_RAW_HARDWARE;
+
+	info->phc_index = ptp_clock_index(shared->ptp_clock);
+
+	info->tx_types =
+		(1 << HWTSTAMP_TX_OFF) |
+		(1 << HWTSTAMP_TX_ON) |
+		(1 << HWTSTAMP_TX_ONESTEP_SYNC);
+
+	info->rx_filters =
+		(1 << HWTSTAMP_FILTER_NONE) |
+		(1 << HWTSTAMP_FILTER_PTP_V1_L4_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
+
+	return 0;
+}
+
+static void lan8814_flush_fifo(struct phy_device *phydev, bool egress)
+{
+	int i;
+
+	for (i = 0; i < FIFO_SIZE; ++i)
+		lanphy_read_page_reg(phydev, 5,
+				     egress ? PTP_TX_MSG_HEADER2 : PTP_RX_MSG_HEADER2);
+
+	/* Read to clear overflow status bit */
+	lanphy_read_page_reg(phydev, 5, PTP_TSU_INT_STS);
+}
+
+static int lan8814_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
+{
+	struct kszphy_ptp_priv *ptp_priv =
+			  container_of(mii_ts, struct kszphy_ptp_priv, mii_ts);
+	struct phy_device *phydev = ptp_priv->phydev;
+	struct lan8814_shared_priv *shared = phydev->shared->priv;
+	struct lan8814_ptp_rx_ts *rx_ts, *tmp;
+	struct hwtstamp_config config;
+	int txcfg = 0, rxcfg = 0;
+	int pkt_ts_enable;
+
+	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	ptp_priv->hwts_tx_type = config.tx_type;
+	ptp_priv->rx_filter = config.rx_filter;
+
+	switch (config.rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		ptp_priv->layer = 0;
+		ptp_priv->version = 0;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+		ptp_priv->layer = PTP_CLASS_L4;
+		ptp_priv->version = PTP_CLASS_V2;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+		ptp_priv->layer = PTP_CLASS_L2;
+		ptp_priv->version = PTP_CLASS_V2;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+		ptp_priv->layer = PTP_CLASS_L4 | PTP_CLASS_L2;
+		ptp_priv->version = PTP_CLASS_V2;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	if (ptp_priv->layer & PTP_CLASS_L2) {
+		rxcfg = PTP_RX_PARSE_CONFIG_LAYER2_EN_;
+		txcfg = PTP_TX_PARSE_CONFIG_LAYER2_EN_;
+	} else if (ptp_priv->layer & PTP_CLASS_L4) {
+		rxcfg |= PTP_RX_PARSE_CONFIG_IPV4_EN_ | PTP_RX_PARSE_CONFIG_IPV6_EN_;
+		txcfg |= PTP_TX_PARSE_CONFIG_IPV4_EN_ | PTP_TX_PARSE_CONFIG_IPV6_EN_;
+	}
+	lanphy_write_page_reg(ptp_priv->phydev, 5, PTP_RX_PARSE_CONFIG, rxcfg);
+	lanphy_write_page_reg(ptp_priv->phydev, 5, PTP_TX_PARSE_CONFIG, txcfg);
+
+	pkt_ts_enable = PTP_TIMESTAMP_EN_SYNC_ | PTP_TIMESTAMP_EN_DREQ_ |
+			PTP_TIMESTAMP_EN_PDREQ_ | PTP_TIMESTAMP_EN_PDRES_;
+	lanphy_write_page_reg(ptp_priv->phydev, 5, PTP_RX_TIMESTAMP_EN, pkt_ts_enable);
+	lanphy_write_page_reg(ptp_priv->phydev, 5, PTP_TX_TIMESTAMP_EN, pkt_ts_enable);
+
+	if (ptp_priv->hwts_tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
+		lanphy_write_page_reg(ptp_priv->phydev, 5, PTP_TX_MOD,
+				      PTP_TX_MOD_TX_PTP_SYNC_TS_INSERT_);
+
+	if (config.rx_filter != HWTSTAMP_FILTER_NONE)
+		lan8814_config_ts_intr(ptp_priv->phydev, true);
+	else
+		lan8814_config_ts_intr(ptp_priv->phydev, false);
+
+	mutex_lock(&shared->shared_lock);
+	if (config.rx_filter != HWTSTAMP_FILTER_NONE)
+		shared->ref++;
+	else
+		shared->ref--;
+
+	if (shared->ref)
+		lanphy_write_page_reg(ptp_priv->phydev, 4, PTP_CMD_CTL,
+				      PTP_CMD_CTL_PTP_ENABLE_);
+	else
+		lanphy_write_page_reg(ptp_priv->phydev, 4, PTP_CMD_CTL,
+				      PTP_CMD_CTL_PTP_DISABLE_);
+	mutex_unlock(&shared->shared_lock);
+
+	/* In case of multiple starts and stops, these needs to be cleared */
+	list_for_each_entry_safe(rx_ts, tmp, &ptp_priv->rx_ts_list, list) {
+		list_del(&rx_ts->list);
+		kfree(rx_ts);
+	}
+	skb_queue_purge(&ptp_priv->rx_queue);
+	skb_queue_purge(&ptp_priv->tx_queue);
+
+	lan8814_flush_fifo(ptp_priv->phydev, false);
+	lan8814_flush_fifo(ptp_priv->phydev, true);
+
+	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? -EFAULT : 0;
+}
+
+static bool is_sync(struct sk_buff *skb, int type)
+{
+	struct ptp_header *hdr;
+
+	hdr = ptp_parse_header(skb, type);
+	if (!hdr)
+		return false;
+
+	return ((ptp_get_msgtype(hdr, type) & 0xf) == 0);
+}
+
+static void lan8814_txtstamp(struct mii_timestamper *mii_ts,
+			     struct sk_buff *skb, int type)
+{
+	struct kszphy_ptp_priv *ptp_priv = container_of(mii_ts, struct kszphy_ptp_priv, mii_ts);
+
+	switch (ptp_priv->hwts_tx_type) {
+	case HWTSTAMP_TX_ONESTEP_SYNC:
+		if (is_sync(skb, type)) {
+			kfree_skb(skb);
+			return;
+		}
+		fallthrough;
+	case HWTSTAMP_TX_ON:
+		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+		skb_queue_tail(&ptp_priv->tx_queue, skb);
+		break;
+	case HWTSTAMP_TX_OFF:
+	default:
+		kfree_skb(skb);
+		break;
+	}
+}
+
+static void lan8814_get_sig_rx(struct sk_buff *skb, u16 *sig)
+{
+	struct ptp_header *ptp_header;
+	u32 type;
+
+	skb_push(skb, ETH_HLEN);
+	type = ptp_classify_raw(skb);
+	ptp_header = ptp_parse_header(skb, type);
+	skb_pull_inline(skb, ETH_HLEN);
+
+	*sig = (__force u16)(ntohs(ptp_header->sequence_id));
+}
+
+static bool lan8814_match_rx_ts(struct kszphy_ptp_priv *ptp_priv,
+				struct sk_buff *skb)
+{
+	struct skb_shared_hwtstamps *shhwtstamps;
+	struct lan8814_ptp_rx_ts *rx_ts, *tmp;
+	unsigned long flags;
+	bool ret = false;
+	u16 skb_sig;
+
+	lan8814_get_sig_rx(skb, &skb_sig);
+
+	/* Iterate over all RX timestamps and match it with the received skbs */
+	spin_lock_irqsave(&ptp_priv->rx_ts_lock, flags);
+	list_for_each_entry_safe(rx_ts, tmp, &ptp_priv->rx_ts_list, list) {
+		/* Check if we found the signature we were looking for. */
+		if (memcmp(&skb_sig, &rx_ts->seq_id, sizeof(rx_ts->seq_id)))
+			continue;
+
+		shhwtstamps = skb_hwtstamps(skb);
+		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+		shhwtstamps->hwtstamp = ktime_set(rx_ts->seconds,
+						  rx_ts->nsec);
+		netif_rx_ni(skb);
+
+		list_del(&rx_ts->list);
+		kfree(rx_ts);
+
+		ret = true;
+		break;
+	}
+	spin_unlock_irqrestore(&ptp_priv->rx_ts_lock, flags);
+
+	return ret;
+}
+
+static bool lan8814_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
+{
+	struct kszphy_ptp_priv *ptp_priv =
+			container_of(mii_ts, struct kszphy_ptp_priv, mii_ts);
+
+	if (ptp_priv->rx_filter == HWTSTAMP_FILTER_NONE ||
+	    type == PTP_CLASS_NONE)
+		return false;
+
+	if ((type & ptp_priv->version) == 0 || (type & ptp_priv->layer) == 0)
+		return false;
+
+	/* If we failed to match then add it to the queue for when the timestamp
+	 * will come
+	 */
+	if (!lan8814_match_rx_ts(ptp_priv, skb))
+		skb_queue_tail(&ptp_priv->rx_queue, skb);
+
+	return true;
+}
+
+static void lan8814_ptp_clock_set(struct phy_device *phydev,
+				  u32 seconds, u32 nano_seconds)
+{
+	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_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_LOAD_);
+}
+
+static void lan8814_ptp_clock_get(struct phy_device *phydev,
+				  u32 *seconds, u32 *nano_seconds)
+{
+	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);
+
+	*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);
+}
+
+static int lan8814_ptpci_gettime64(struct ptp_clock_info *ptpci,
+				   struct timespec64 *ts)
+{
+	struct lan8814_shared_priv *shared = container_of(ptpci, struct lan8814_shared_priv,
+							  ptp_clock_info);
+	struct phy_device *phydev = shared->phydev;
+	u32 nano_seconds;
+	u32 seconds;
+
+	mutex_lock(&shared->shared_lock);
+	lan8814_ptp_clock_get(phydev, &seconds, &nano_seconds);
+	mutex_unlock(&shared->shared_lock);
+	ts->tv_sec = seconds;
+	ts->tv_nsec = nano_seconds;
 
 	return 0;
 }
 
+static int lan8814_ptpci_settime64(struct ptp_clock_info *ptpci,
+				   const struct timespec64 *ts)
+{
+	struct lan8814_shared_priv *shared = container_of(ptpci, struct lan8814_shared_priv,
+							  ptp_clock_info);
+	struct phy_device *phydev = shared->phydev;
+
+	mutex_lock(&shared->shared_lock);
+	lan8814_ptp_clock_set(phydev, ts->tv_sec, ts->tv_nsec);
+	mutex_unlock(&shared->shared_lock);
+
+	return 0;
+}
+
+static void lan8814_ptp_clock_step(struct phy_device *phydev,
+				   s64 time_step_ns)
+{
+	u32 nano_seconds_step;
+	u64 abs_time_step_ns;
+	u32 unsigned_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);
+		nano_seconds += remainder;
+		if (nano_seconds >= 1000000000) {
+			unsigned_seconds++;
+			nano_seconds -= 1000000000;
+		}
+		lan8814_ptp_clock_set(phydev, unsigned_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);
+		nano_seconds_step = remainder;
+		if (nano_seconds < nano_seconds_step) {
+			unsigned_seconds--;
+			nano_seconds += 1000000000;
+		}
+		nano_seconds -= nano_seconds_step;
+		lan8814_ptp_clock_set(phydev, unsigned_seconds,
+				      nano_seconds);
+		return;
+	}
+
+	/* do clock step */
+	if (time_step_ns >= 0) {
+		abs_time_step_ns = (u64)time_step_ns;
+		seconds = (s32)div_u64_rem(abs_time_step_ns, 1000000000,
+					   &remainder);
+		nano_seconds = remainder;
+	} else {
+		abs_time_step_ns = (u64)(-time_step_ns);
+		seconds = -((s32)div_u64_rem(abs_time_step_ns, 1000000000,
+			    &remainder));
+		nano_seconds = remainder;
+		if (nano_seconds > 0) {
+			/* subtracting nano seconds is not allowed
+			 * convert to subtracting from seconds,
+			 * and adding to nanoseconds
+			 */
+			seconds--;
+			nano_seconds = (1000000000 - nano_seconds);
+		}
+	}
+
+	if (nano_seconds > 0) {
+		/* add 8 ns to cover the likely normal increment */
+		nano_seconds += 8;
+	}
+
+	if (nano_seconds >= 1000000000) {
+		/* carry into seconds */
+		seconds++;
+		nano_seconds -= 1000000000;
+	}
+
+	while (seconds) {
+		if (seconds > 0) {
+			u32 adjustment_value = (u32)seconds;
+			u16 adjustment_value_lo, adjustment_value_hi;
+
+			if (adjustment_value > 0xF)
+				adjustment_value = 0xF;
+
+			adjustment_value_lo = adjustment_value & 0xffff;
+			adjustment_value_hi = (adjustment_value >> 16) & 0x3fff;
+
+			lanphy_write_page_reg(phydev, 4, PTP_LTC_STEP_ADJ_LO,
+					      adjustment_value_lo);
+			lanphy_write_page_reg(phydev, 4, PTP_LTC_STEP_ADJ_HI,
+					      PTP_LTC_STEP_ADJ_DIR_ |
+					      adjustment_value_hi);
+			seconds -= ((s32)adjustment_value);
+		} else {
+			u32 adjustment_value = (u32)(-seconds);
+			u16 adjustment_value_lo, adjustment_value_hi;
+
+			if (adjustment_value > 0xF)
+				adjustment_value = 0xF;
+
+			adjustment_value_lo = adjustment_value & 0xffff;
+			adjustment_value_hi = (adjustment_value >> 16) & 0x3fff;
+
+			lanphy_write_page_reg(phydev, 4, PTP_LTC_STEP_ADJ_LO,
+					      adjustment_value_lo);
+			lanphy_write_page_reg(phydev, 4, PTP_LTC_STEP_ADJ_HI,
+					      adjustment_value_hi);
+			seconds += ((s32)adjustment_value);
+		}
+		lanphy_write_page_reg(phydev, 4, PTP_CMD_CTL,
+				      PTP_CMD_CTL_PTP_LTC_STEP_SEC_);
+	}
+	if (nano_seconds) {
+		u16 nano_seconds_lo;
+		u16 nano_seconds_hi;
+
+		nano_seconds_lo = nano_seconds & 0xffff;
+		nano_seconds_hi = (nano_seconds >> 16) & 0x3fff;
+
+		lanphy_write_page_reg(phydev, 4, PTP_LTC_STEP_ADJ_LO,
+				      nano_seconds_lo);
+		lanphy_write_page_reg(phydev, 4, PTP_LTC_STEP_ADJ_HI,
+				      PTP_LTC_STEP_ADJ_DIR_ |
+				      nano_seconds_hi);
+		lanphy_write_page_reg(phydev, 4, PTP_CMD_CTL,
+				      PTP_CMD_CTL_PTP_LTC_STEP_NSEC_);
+	}
+}
+
+static int lan8814_ptpci_adjtime(struct ptp_clock_info *ptpci, s64 delta)
+{
+	struct lan8814_shared_priv *shared = container_of(ptpci, struct lan8814_shared_priv,
+							  ptp_clock_info);
+	struct phy_device *phydev = shared->phydev;
+
+	mutex_lock(&shared->shared_lock);
+	lan8814_ptp_clock_step(phydev, delta);
+	mutex_unlock(&shared->shared_lock);
+
+	return 0;
+}
+
+static int lan8814_ptpci_adjfine(struct ptp_clock_info *ptpci, long scaled_ppm)
+{
+	struct lan8814_shared_priv *shared = container_of(ptpci, struct lan8814_shared_priv,
+							  ptp_clock_info);
+	struct phy_device *phydev = shared->phydev;
+	u16 kszphy_rate_adj_lo, kszphy_rate_adj_hi;
+	bool positive = true;
+	u32 kszphy_rate_adj;
+
+	if (scaled_ppm < 0) {
+		scaled_ppm = -scaled_ppm;
+		positive = false;
+	}
+
+	kszphy_rate_adj = LAN8814_1PPM_FORMAT * (scaled_ppm >> 16);
+	kszphy_rate_adj += (LAN8814_1PPM_FORMAT * (0xffff & scaled_ppm)) >> 16;
+
+	kszphy_rate_adj_lo = kszphy_rate_adj & 0xffff;
+	kszphy_rate_adj_hi = (kszphy_rate_adj >> 16) & 0x3fff;
+
+	if (positive)
+		kszphy_rate_adj_hi |= PTP_CLOCK_RATE_ADJ_DIR_;
+
+	mutex_lock(&shared->shared_lock);
+	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_RATE_ADJ_HI, kszphy_rate_adj_hi);
+	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_RATE_ADJ_LO, kszphy_rate_adj_lo);
+	mutex_unlock(&shared->shared_lock);
+
+	return 0;
+}
+
+static void lan8814_get_sig_tx(struct sk_buff *skb, u16 *sig)
+{
+	struct ptp_header *ptp_header;
+	u32 type;
+
+	type = ptp_classify_raw(skb);
+	ptp_header = ptp_parse_header(skb, type);
+
+	*sig = (__force u16)(ntohs(ptp_header->sequence_id));
+}
+
+static void lan8814_dequeue_tx_skb(struct kszphy_ptp_priv *ptp_priv)
+{
+	struct phy_device *phydev = ptp_priv->phydev;
+	struct skb_shared_hwtstamps shhwtstamps;
+	struct sk_buff *skb, *skb_tmp;
+	unsigned long flags;
+	u32 seconds, nsec;
+	bool ret = false;
+	u16 skb_sig;
+	u16 seq_id;
+
+	lan8814_ptp_tx_ts_get(phydev, &seconds, &nsec, &seq_id);
+
+	spin_lock_irqsave(&ptp_priv->tx_queue.lock, flags);
+	skb_queue_walk_safe(&ptp_priv->tx_queue, skb, skb_tmp) {
+		lan8814_get_sig_tx(skb, &skb_sig);
+
+		if (memcmp(&skb_sig, &seq_id, sizeof(seq_id)))
+			continue;
+
+		__skb_unlink(skb, &ptp_priv->tx_queue);
+		ret = true;
+		break;
+	}
+	spin_unlock_irqrestore(&ptp_priv->tx_queue.lock, flags);
+
+	if (ret) {
+		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+		shhwtstamps.hwtstamp = ktime_set(seconds, nsec);
+		skb_complete_tx_timestamp(skb, &shhwtstamps);
+	}
+}
+
+static void lan8814_get_tx_ts(struct kszphy_ptp_priv *ptp_priv)
+{
+	struct phy_device *phydev = ptp_priv->phydev;
+	u32 reg;
+
+	do {
+		lan8814_dequeue_tx_skb(ptp_priv);
+
+		/* If other timestamps are available in the FIFO,
+		 * process them.
+		 */
+		reg = lanphy_read_page_reg(phydev, 5, PTP_CAP_INFO);
+	} while (PTP_CAP_INFO_TX_TS_CNT_GET_(reg) > 0);
+}
+
+static bool lan8814_match_skb(struct kszphy_ptp_priv *ptp_priv,
+			      struct lan8814_ptp_rx_ts *rx_ts)
+{
+	struct skb_shared_hwtstamps *shhwtstamps;
+	struct sk_buff *skb, *skb_tmp;
+	unsigned long flags;
+	bool ret = false;
+	u16 skb_sig;
+
+	spin_lock_irqsave(&ptp_priv->rx_queue.lock, flags);
+	skb_queue_walk_safe(&ptp_priv->rx_queue, skb, skb_tmp) {
+		lan8814_get_sig_rx(skb, &skb_sig);
+
+		if (memcmp(&skb_sig, &rx_ts->seq_id, sizeof(rx_ts->seq_id)))
+			continue;
+
+		__skb_unlink(skb, &ptp_priv->rx_queue);
+
+		ret = true;
+		break;
+	}
+	spin_unlock_irqrestore(&ptp_priv->rx_queue.lock, flags);
+
+	if (ret) {
+		shhwtstamps = skb_hwtstamps(skb);
+		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+		shhwtstamps->hwtstamp = ktime_set(rx_ts->seconds, rx_ts->nsec);
+		netif_rx_ni(skb);
+	}
+
+	return ret;
+}
+
+static void lan8814_get_rx_ts(struct kszphy_ptp_priv *ptp_priv)
+{
+	struct phy_device *phydev = ptp_priv->phydev;
+	struct lan8814_ptp_rx_ts *rx_ts;
+	unsigned long flags;
+	u32 reg;
+
+	do {
+		rx_ts = kzalloc(sizeof(*rx_ts), GFP_KERNEL);
+		if (!rx_ts)
+			return;
+
+		lan8814_ptp_rx_ts_get(phydev, &rx_ts->seconds, &rx_ts->nsec,
+				      &rx_ts->seq_id);
+
+		/* If we failed to match the skb add it to the queue for when
+		 * the frame will come
+		 */
+		if (!lan8814_match_skb(ptp_priv, rx_ts)) {
+			spin_lock_irqsave(&ptp_priv->rx_ts_lock, flags);
+			list_add(&rx_ts->list, &ptp_priv->rx_ts_list);
+			spin_unlock_irqrestore(&ptp_priv->rx_ts_lock, flags);
+		} else {
+			kfree(rx_ts);
+		}
+
+		/* If other timestamps are available in the FIFO,
+		 * process them.
+		 */
+		reg = lanphy_read_page_reg(phydev, 5, PTP_CAP_INFO);
+	} while (PTP_CAP_INFO_RX_TS_CNT_GET_(reg) > 0);
+}
+
+static void lan8814_handle_ptp_interrupt(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+	struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
+	u16 status;
+
+	status = lanphy_read_page_reg(phydev, 5, PTP_TSU_INT_STS);
+	if (status & PTP_TSU_INT_STS_PTP_TX_TS_EN_)
+		lan8814_get_tx_ts(ptp_priv);
+
+	if (status & PTP_TSU_INT_STS_PTP_RX_TS_EN_)
+		lan8814_get_rx_ts(ptp_priv);
+
+	if (status & PTP_TSU_INT_STS_PTP_TX_TS_OVRFL_INT_) {
+		lan8814_flush_fifo(phydev, true);
+		skb_queue_purge(&ptp_priv->tx_queue);
+	}
+
+	if (status & PTP_TSU_INT_STS_PTP_RX_TS_OVRFL_INT_) {
+		lan8814_flush_fifo(phydev, false);
+		skb_queue_purge(&ptp_priv->rx_queue);
+	}
+}
+
 static int lan8804_config_init(struct phy_device *phydev)
 {
 	int val;
@@ -1668,17 +2482,31 @@ static int lan8804_config_init(struct phy_device *phydev)
 
 static irqreturn_t lan8814_handle_interrupt(struct phy_device *phydev)
 {
+	u16 tsu_irq_status;
 	int irq_status;
 
 	irq_status = phy_read(phydev, LAN8814_INTS);
-	if (irq_status < 0)
-		return IRQ_NONE;
+	if (irq_status > 0 && (irq_status & LAN8814_INT_LINK))
+		phy_trigger_machine(phydev);
 
-	if (!(irq_status & LAN8814_INT_LINK))
+	if (irq_status < 0) {
+		phy_error(phydev);
 		return IRQ_NONE;
+	}
 
-	phy_trigger_machine(phydev);
+	while (1) {
+		tsu_irq_status = lanphy_read_page_reg(phydev, 4,
+						      LAN8814_INTR_STS_REG);
 
+		if (tsu_irq_status > 0 &&
+		    (tsu_irq_status & (LAN8814_INTR_STS_REG_1588_TSU0_ |
+				       LAN8814_INTR_STS_REG_1588_TSU1_ |
+				       LAN8814_INTR_STS_REG_1588_TSU2_ |
+				       LAN8814_INTR_STS_REG_1588_TSU3_)))
+			lan8814_handle_ptp_interrupt(phydev);
+		else
+			break;
+	}
 	return IRQ_HANDLED;
 }
 
@@ -1718,6 +2546,223 @@ static int lan8814_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static void lan8814_ptp_init(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+	struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
+	u32 temp;
+
+	lanphy_write_page_reg(phydev, 5, TSU_HARD_RESET, TSU_HARD_RESET_);
+
+	temp = lanphy_read_page_reg(phydev, 5, PTP_TX_MOD);
+	temp |= PTP_TX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_;
+	lanphy_write_page_reg(phydev, 5, PTP_TX_MOD, temp);
+
+	temp = lanphy_read_page_reg(phydev, 5, PTP_RX_MOD);
+	temp |= PTP_RX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_;
+	lanphy_write_page_reg(phydev, 5, PTP_RX_MOD, temp);
+
+	lanphy_write_page_reg(phydev, 5, PTP_RX_PARSE_CONFIG, 0);
+	lanphy_write_page_reg(phydev, 5, PTP_TX_PARSE_CONFIG, 0);
+
+	/* Removing default registers configs related to L2 and IP */
+	lanphy_write_page_reg(phydev, 5, PTP_TX_PARSE_L2_ADDR_EN, 0);
+	lanphy_write_page_reg(phydev, 5, PTP_RX_PARSE_L2_ADDR_EN, 0);
+	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);
+
+	skb_queue_head_init(&ptp_priv->tx_queue);
+	skb_queue_head_init(&ptp_priv->rx_queue);
+	INIT_LIST_HEAD(&ptp_priv->rx_ts_list);
+	spin_lock_init(&ptp_priv->rx_ts_lock);
+
+	ptp_priv->phydev = phydev;
+
+	ptp_priv->mii_ts.rxtstamp = lan8814_rxtstamp;
+	ptp_priv->mii_ts.txtstamp = lan8814_txtstamp;
+	ptp_priv->mii_ts.hwtstamp = lan8814_hwtstamp;
+	ptp_priv->mii_ts.ts_info  = lan8814_ts_info;
+
+	phydev->mii_ts = &ptp_priv->mii_ts;
+}
+
+static int lan8814_ptp_probe_once(struct phy_device *phydev)
+{
+	struct lan8814_shared_priv *shared = phydev->shared->priv;
+
+	/* Initialise shared lock for clock*/
+	mutex_init(&shared->shared_lock);
+
+	shared->ptp_clock_info.owner = THIS_MODULE;
+	snprintf(shared->ptp_clock_info.name, 30, "%s", phydev->drv->name);
+	shared->ptp_clock_info.max_adj = 31249999;
+	shared->ptp_clock_info.n_alarm = 0;
+	shared->ptp_clock_info.n_ext_ts = 0;
+	shared->ptp_clock_info.n_pins = 0;
+	shared->ptp_clock_info.pps = 0;
+	shared->ptp_clock_info.pin_config = NULL;
+	shared->ptp_clock_info.adjfine = lan8814_ptpci_adjfine;
+	shared->ptp_clock_info.adjtime = lan8814_ptpci_adjtime;
+	shared->ptp_clock_info.gettime64 = lan8814_ptpci_gettime64;
+	shared->ptp_clock_info.settime64 = lan8814_ptpci_settime64;
+	shared->ptp_clock_info.getcrosststamp = NULL;
+
+	shared->ptp_clock = ptp_clock_register(&shared->ptp_clock_info,
+					       &phydev->mdio.dev);
+	if (IS_ERR_OR_NULL(shared->ptp_clock)) {
+		phydev_err(phydev, "ptp_clock_register failed %lu\n",
+			   PTR_ERR(shared->ptp_clock));
+		return -EINVAL;
+	}
+
+	phydev_dbg(phydev, "successfully registered ptp clock\n");
+
+	shared->phydev = phydev;
+
+	/* The EP.4 is shared between all the PHYs in the package and also it
+	 * can be accessed by any of the PHYs
+	 */
+	lanphy_write_page_reg(phydev, 4, LTC_HARD_RESET, LTC_HARD_RESET_);
+	lanphy_write_page_reg(phydev, 4, PTP_OPERATING_MODE,
+			      PTP_OPERATING_MODE_STANDALONE_);
+
+	return 0;
+}
+
+static int lan8814_read_status(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+	struct kszphy_latencies *latencies = &priv->latencies;
+	int err;
+	int regval;
+
+	err = genphy_read_status(phydev);
+	if (err)
+		return err;
+
+	switch (phydev->speed) {
+	case SPEED_1000:
+		lanphy_write_page_reg(phydev, 5, PTP_RX_LATENCY_1000,
+				      latencies->rx_1000);
+		lanphy_write_page_reg(phydev, 5, PTP_TX_LATENCY_1000,
+				      latencies->tx_1000);
+		break;
+	case SPEED_100:
+		lanphy_write_page_reg(phydev, 5, PTP_RX_LATENCY_100,
+				      latencies->rx_100);
+		lanphy_write_page_reg(phydev, 5, PTP_TX_LATENCY_100,
+				      latencies->tx_100);
+		break;
+	case SPEED_10:
+		lanphy_write_page_reg(phydev, 5, PTP_RX_LATENCY_10,
+				      latencies->rx_10);
+		lanphy_write_page_reg(phydev, 5, PTP_TX_LATENCY_10,
+				      latencies->tx_10);
+		break;
+	default:
+		break;
+	}
+
+	/* Make sure the PHY is not broken. Read idle error count,
+	 * and reset the PHY if it is maxed out.
+	 */
+	regval = phy_read(phydev, MII_STAT1000);
+	if ((regval & 0xFF) == 0xFF) {
+		phy_init_hw(phydev);
+		phydev->link = 0;
+		if (phydev->drv->config_intr && phy_interrupt_is_valid(phydev))
+			phydev->drv->config_intr(phydev);
+		return genphy_config_aneg(phydev);
+	}
+
+	return 0;
+}
+
+static int lan8814_config_init(struct phy_device *phydev)
+{
+	int val;
+
+	/* Reset the PHY */
+	val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
+	val |= LAN8814_QSGMII_SOFT_RESET_BIT;
+	lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
+
+	/* Disable ANEG with QSGMII PCS Host side */
+	val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
+	val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
+	lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
+
+	/* MDI-X setting for swap A,B transmit */
+	val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
+	val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
+	val |= LAN8814_ALIGN_TX_A_B_SWAP;
+	lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
+
+	return 0;
+}
+
+static void lan8814_parse_latency(struct phy_device *phydev)
+{
+	const struct device_node *np = phydev->mdio.dev.of_node;
+	struct kszphy_priv *priv = phydev->priv;
+	struct kszphy_latencies *latency = &priv->latencies;
+	u32 val;
+
+	if (!of_property_read_u32(np, "lan8814,latency_rx_10", &val))
+		latency->rx_10 = val;
+	if (!of_property_read_u32(np, "lan8814,latency_tx_10", &val))
+		latency->tx_10 = val;
+	if (!of_property_read_u32(np, "lan8814,latency_rx_100", &val))
+		latency->rx_100 = val;
+	if (!of_property_read_u32(np, "lan8814,latency_tx_100", &val))
+		latency->tx_100 = val;
+	if (!of_property_read_u32(np, "lan8814,latency_rx_1000", &val))
+		latency->rx_1000 = val;
+	if (!of_property_read_u32(np, "lan8814,latency_tx_1000", &val))
+		latency->tx_1000 = val;
+}
+
+static int lan8814_probe(struct phy_device *phydev)
+{
+	const struct device_node *np = phydev->mdio.dev.of_node;
+	struct kszphy_priv *priv;
+	u16 addr;
+	int err;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->led_mode = -1;
+
+	priv->latencies = lan8814_latencies;
+
+	phydev->priv = priv;
+
+	if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
+	    !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING) ||
+	    of_property_read_bool(np, "lan8814,ignore-ts"))
+		return 0;
+
+	/* Strap-in value for PHY address, below register read gives starting
+	 * phy address value
+	 */
+	addr = lanphy_read_page_reg(phydev, 4, 0) & 0x1F;
+	devm_phy_package_join(&phydev->mdio.dev, phydev,
+			      addr, sizeof(struct lan8814_shared_priv));
+
+	if (phy_package_init_once(phydev)) {
+		err = lan8814_ptp_probe_once(phydev);
+		if (err)
+			return err;
+	}
+
+	lan8814_parse_latency(phydev);
+	lan8814_ptp_init(phydev);
+
+	return 0;
+}
+
 static struct phy_driver ksphy_driver[] = {
 {
 	.phy_id		= PHY_ID_KS8737,
@@ -1892,10 +2937,9 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Microchip INDY Gigabit Quad PHY",
 	.config_init	= lan8814_config_init,
-	.driver_data	= &ksz9021_type,
-	.probe		= kszphy_probe,
+	.probe		= lan8814_probe,
 	.soft_reset	= genphy_soft_reset,
-	.read_status	= ksz9031_read_status,
+	.read_status	= lan8814_read_status,
 	.get_sset_count	= kszphy_get_sset_count,
 	.get_strings	= kszphy_get_strings,
 	.get_stats	= kszphy_get_stats,
-- 
2.17.1

Re: [PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814 phy

[Andrew Lunn]

> +static struct kszphy_latencies lan8814_latencies = {
> +	.rx_10		= 0x22AA,
> +	.tx_10		= 0x2E4A,
> +	.rx_100		= 0x092A,
> +	.tx_100		= 0x02C1,
> +	.rx_1000	= 0x01AD,
> +	.tx_1000	= 0x00C9,
> +};

Seems odd to use hex here. Are these the defaults? At minimum, you
need to add these to the binding document, making it clear what
defaults are used. Also, what are the unit here?

> +	/* Make sure the PHY is not broken. Read idle error count,
> +	 * and reset the PHY if it is maxed out.
> +	 */
> +	regval = phy_read(phydev, MII_STAT1000);
> +	if ((regval & 0xFF) == 0xFF) {
> +		phy_init_hw(phydev);
> +		phydev->link = 0;
> +		if (phydev->drv->config_intr && phy_interrupt_is_valid(phydev))
> +			phydev->drv->config_intr(phydev);
> +		return genphy_config_aneg(phydev);
> +	}

Is this related to PTP? Or is the PHY broken in general? This looks
like it should be something submitted to stable.

> +static int lan8814_config_init(struct phy_device *phydev)
> +{
> +	int val;
> +
> +	/* Reset the PHY */
> +	val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> +	val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> +	lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
> +
> +	/* Disable ANEG with QSGMII PCS Host side */
> +	val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
> +	val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
> +	lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
> +
> +	/* MDI-X setting for swap A,B transmit */
> +	val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
> +	val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
> +	val |= LAN8814_ALIGN_TX_A_B_SWAP;
> +	lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);

This does not look related to PTP. If David has not ready merged this,
i would of said you should of submitted this as a separate patch.

> +static void lan8814_parse_latency(struct phy_device *phydev)
> +{
> +	const struct device_node *np = phydev->mdio.dev.of_node;
> +	struct kszphy_priv *priv = phydev->priv;
> +	struct kszphy_latencies *latency = &priv->latencies;
> +	u32 val;
> +
> +	if (!of_property_read_u32(np, "lan8814,latency_rx_10", &val))
> +		latency->rx_10 = val;
> +	if (!of_property_read_u32(np, "lan8814,latency_tx_10", &val))
> +		latency->tx_10 = val;
> +	if (!of_property_read_u32(np, "lan8814,latency_rx_100", &val))
> +		latency->rx_100 = val;
> +	if (!of_property_read_u32(np, "lan8814,latency_tx_100", &val))
> +		latency->tx_100 = val;
> +	if (!of_property_read_u32(np, "lan8814,latency_rx_1000", &val))
> +		latency->rx_1000 = val;
> +	if (!of_property_read_u32(np, "lan8814,latency_tx_1000", &val))
> +		latency->tx_1000 = val;

Are range checks need here? You are reading a u32, but PHY registers
are generally 16 bit.

    Andrew

RE: [PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814 phy

[Divya.Koppera]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, March 4, 2022 6:36 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> Cc: netdev@vger.kernel.org; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; richardcochran@gmail.com; linux-
> kernel@vger.kernel.org; UNGLinuxDriver <UNGLinuxDriver@microchip.com>;
> Madhuri Sripada - I34878 <Madhuri.Sripada@microchip.com>; Manohar Puri -
> I30488 <Manohar.Puri@microchip.com>
> Subject: Re: [PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814
> phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +static struct kszphy_latencies lan8814_latencies = {
> > +     .rx_10          = 0x22AA,
> > +     .tx_10          = 0x2E4A,
> > +     .rx_100         = 0x092A,
> > +     .tx_100         = 0x02C1,
> > +     .rx_1000        = 0x01AD,
> > +     .tx_1000        = 0x00C9,
> > +};
> 
> Seems odd to use hex here. Are these the defaults? At minimum, you need to
> add these to the binding document, making it clear what defaults are used.
> Also, what are the unit here?

Yes Andrew, these are default values. Richard too mentioned about this as below
"However, DTS is probably the wrong place.  The linuxptp user space stack has configuration variables for this purpose:"
I will check regarding this and will come with fix in next patch if its applicable.

> 
> > +     /* Make sure the PHY is not broken. Read idle error count,
> > +      * and reset the PHY if it is maxed out.
> > +      */
> > +     regval = phy_read(phydev, MII_STAT1000);
> > +     if ((regval & 0xFF) == 0xFF) {
> > +             phy_init_hw(phydev);
> > +             phydev->link = 0;
> > +             if (phydev->drv->config_intr && phy_interrupt_is_valid(phydev))
> > +                     phydev->drv->config_intr(phydev);
> > +             return genphy_config_aneg(phydev);
> > +     }
> 
> Is this related to PTP? Or is the PHY broken in general? This looks like it should
> be something submitted to stable.
>

Previously lan8814 phy used kszphy_read_status, we have reused the same function and added new
 Changes related to latency with new function lan8814_read_status.
 
> > +static int lan8814_config_init(struct phy_device *phydev) {
> > +     int val;
> > +
> > +     /* Reset the PHY */
> > +     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> > +     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> > +     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET,
> > + val);
> > +
> > +     /* Disable ANEG with QSGMII PCS Host side */
> > +     val = lanphy_read_page_reg(phydev, 5,
> LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
> > +     val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
> > +     lanphy_write_page_reg(phydev, 5,
> > + LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
> > +
> > +     /* MDI-X setting for swap A,B transmit */
> > +     val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
> > +     val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
> > +     val |= LAN8814_ALIGN_TX_A_B_SWAP;
> > +     lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
> 
> This does not look related to PTP. If David has not ready merged this, i would
> of said you should of submitted this as a separate patch.
> 

This code already present in lan8814 phy code. I think due to movement of function from up to down.
This change reflected here.

> > +static void lan8814_parse_latency(struct phy_device *phydev) {
> > +     const struct device_node *np = phydev->mdio.dev.of_node;
> > +     struct kszphy_priv *priv = phydev->priv;
> > +     struct kszphy_latencies *latency = &priv->latencies;
> > +     u32 val;
> > +
> > +     if (!of_property_read_u32(np, "lan8814,latency_rx_10", &val))
> > +             latency->rx_10 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_tx_10", &val))
> > +             latency->tx_10 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_rx_100", &val))
> > +             latency->rx_100 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_tx_100", &val))
> > +             latency->tx_100 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_rx_1000", &val))
> > +             latency->rx_1000 = val;
> > +     if (!of_property_read_u32(np, "lan8814,latency_tx_1000", &val))
> > +             latency->tx_1000 = val;
> 
> Are range checks need here? You are reading a u32, but PHY registers are
> generally 16 bit.
> 

Yes this is mistake. May we will need to give fix for this in next patch.

>     Andrew

Re: [PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814 phy

[Andrew Lunn]

> > > +     /* Make sure the PHY is not broken. Read idle error count,
> > > +      * and reset the PHY if it is maxed out.
> > > +      */
> > > +     regval = phy_read(phydev, MII_STAT1000);
> > > +     if ((regval & 0xFF) == 0xFF) {
> > > +             phy_init_hw(phydev);
> > > +             phydev->link = 0;
> > > +             if (phydev->drv->config_intr && phy_interrupt_is_valid(phydev))
> > > +                     phydev->drv->config_intr(phydev);
> > > +             return genphy_config_aneg(phydev);
> > > +     }
> > 
> > Is this related to PTP? Or is the PHY broken in general? This looks like it should
> > be something submitted to stable.
> >
> 
> Previously lan8814 phy used kszphy_read_status, we have reused the same function and added new
> Changes related to latency with new function lan8814_read_status.

Ah, ksz9031_read_status() already has this workaround. How important
is the ordering here? Rather than cut/paste the code, why not actually
call ksz9031_read_status() to get the basic link status and then
append the additional information in this function.

> > > +static int lan8814_config_init(struct phy_device *phydev) {
> > > +     int val;
> > > +
> > > +     /* Reset the PHY */
> > > +     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> > > +     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> > > +     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET,
> > > + val);
> > > +
> > > +     /* Disable ANEG with QSGMII PCS Host side */
> > > +     val = lanphy_read_page_reg(phydev, 5,
> > LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
> > > +     val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
> > > +     lanphy_write_page_reg(phydev, 5,
> > > + LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
> > > +
> > > +     /* MDI-X setting for swap A,B transmit */
> > > +     val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
> > > +     val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
> > > +     val |= LAN8814_ALIGN_TX_A_B_SWAP;
> > > +     lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
> > 
> > This does not look related to PTP. If David has not ready merged this, i would
> > of said you should of submitted this as a separate patch.
> > 
> 
> This code already present in lan8814 phy code. I think due to movement of function from up to down.
> This change reflected here.

I don't remember seeing the same code with - at the beginning.  In
general, it is better to have lots of small patches, each being
obviously correct. If you need to move a function earlier/later, do it
in a patch of its own. It is obviously correct, so takes 0 time to
review, and makes the remaining patches simpler, so also easier to
review.

	Andrew

Re: [PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814 phy

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On Fri Mar 04 2022, Divya Koppera wrote:
> Add support for 1588 in LAN8814 phy driver.
> It supports 1-step and 2-step timestamping.
>
> Co-developed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
> ---
>  drivers/net/phy/micrel.c | 1088 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 1066 insertions(+), 22 deletions(-)

[snip]

> +static bool is_sync(struct sk_buff *skb, int type)
> +{
> +	struct ptp_header *hdr;
> +
> +	hdr = ptp_parse_header(skb, type);
> +	if (!hdr)
> +		return false;
> +
> +	return ((ptp_get_msgtype(hdr, type) & 0xf) == 0);

The '& 0xf' is already performed by ptp_get_msgtype() and you can use '==
PTP_MSGTYPE_SYNC' instead of 0.

Second, this seems like the second driver to use is_sync(). The other
one is dp83640. Richard, should it be moved to ptp classify?

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next 3/3] net: phy: micrel: 1588 support for LAN8814 phy

[Richard Cochran]

On Fri, Mar 04, 2022 at 02:46:08PM +0100, Kurt Kanzenbach wrote:

> Second, this seems like the second driver to use is_sync(). The other
> one is dp83640. Richard, should it be moved to ptp classify?

Sure, why not.

Thanks,
Richard

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 4 Mar 2022 15:04:15 +0530 you wrote:
> The following patch series contains:
> - Fix for concurrent register access, which provides
>   atomic access to extended page register reads/writes.
> - Provides dt-bindings related to latency and timestamping
>   that are required for LAN8814 phy.
> - 1588 hardware timestamping support in LAN8814 phy.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: phy: micrel: Fix concurrent register access
    https://git.kernel.org/netdev/net-next/c/4488f6b61480
  - [net-next,2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy
    https://git.kernel.org/netdev/net-next/c/2358dd3fd325
  - [net-next,3/3] net: phy: micrel: 1588 support for LAN8814 phy
    https://git.kernel.org/netdev/net-next/c/ece19502834d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Andrew Lunn]

On Fri, Mar 04, 2022 at 12:50:11PM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This series was applied to netdev/net-next.git (master)
> by David S. Miller <davem@davemloft.net>:

Hi David

Why was this merged?

    Andrew

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Fri, 4 Mar 2022 14:06:54 +0100

> On Fri, Mar 04, 2022 at 12:50:11PM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
>> Hello:
>> 
>> This series was applied to netdev/net-next.git (master)
>> by David S. Miller <davem@davemloft.net>:
> 
> Hi David
> 
> Why was this merged?

Sorry, it seemed satraightforward to me, and I try to get the backlog under 40 patches before
I hand over to Jakub for the day.

If you want to review, reply to the thread immediately saying so, don't wait until you haver time for the
full review.

Thank you.

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Andrew Lunn]

On Fri, Mar 04, 2022 at 01:21:21PM +0000, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Fri, 4 Mar 2022 14:06:54 +0100
> 
> > On Fri, Mar 04, 2022 at 12:50:11PM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> >> Hello:
> >> 
> >> This series was applied to netdev/net-next.git (master)
> >> by David S. Miller <davem@davemloft.net>:
> > 
> > Hi David
> > 
> > Why was this merged?
> 
> Sorry, it seemed satraightforward to me, and I try to get the backlog under 40 patches before
> I hand over to Jakub for the day.
> 
> If you want to review, reply to the thread immediately saying so, don't wait until you haver time for the
> full review.

This patchset was on the list for less than 5 hours before it got
merged. I tend to sleep for 8 to 10 hours. Making it impossible for me
to react any faster. At an absolute minimum, you need to wait 12
hours, if you expect anybody to have a fair chance of being able to
say, hold on, i want to comment on this patchset.

I also don't like the metric of 40 patches backlog. Is the size of
backlog more important than the quality of the patches? Don't we care
about the quality of the code any more? Don't we care about getting
code reviewed any more?

     Andrew

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Fri, 4 Mar 2022 14:47:48 +0100

> On Fri, Mar 04, 2022 at 01:21:21PM +0000, David Miller wrote:
>> From: Andrew Lunn <andrew@lunn.ch>
>> Date: Fri, 4 Mar 2022 14:06:54 +0100
>> 
>> > On Fri, Mar 04, 2022 at 12:50:11PM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
>> >> Hello:
>> >> 
>> >> This series was applied to netdev/net-next.git (master)
>> >> by David S. Miller <davem@davemloft.net>:
>> > 
>> > Hi David
>> > 
>> > Why was this merged?
>> 
>> Sorry, it seemed satraightforward to me, and I try to get the backlog under 40 patches before
>> I hand over to Jakub for the day.
>> 
>> If you want to review, reply to the thread immediately saying so, don't wait until you haver time for the
>> full review.
> 
> This patchset was on the list for less than 5 hours before it got
> merged. I tend to sleep for 8 to 10 hours. Making it impossible for me
> to react any faster. At an absolute minimum, you need to wait 12
> hours, if you expect anybody to have a fair chance of being able to
> say, hold on, i want to comment on this patchset.
> 
> I also don't like the metric of 40 patches backlog. Is the size of
> backlog more important than the quality of the patches? Don't we care
> about the quality of the code any more? Don't we care about getting
> code reviewed any more?

Ok, message received, I'll apply things less aggressively.

Thank you.

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Richard Cochran]

On Fri, Mar 04, 2022 at 01:21:21PM +0000, David Miller wrote:

> Sorry, it seemed satraightforward to me, and I try to get the backlog under 40 patches before
> I hand over to Jakub for the day.

Day by day, it seems like there is more and more of this PTP driver
stuff.  Maybe it is time for me to manage a separate PTP driver tree.
I could get the reviews and acks, then place a PR to netdev or lkml
when ready.

Thoughts?

Thanks,
Richard

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Andrew Lunn]

On Fri, Mar 04, 2022 at 06:06:28AM -0800, Richard Cochran wrote:
> On Fri, Mar 04, 2022 at 01:21:21PM +0000, David Miller wrote:
> 
> > Sorry, it seemed satraightforward to me, and I try to get the backlog under 40 patches before
> > I hand over to Jakub for the day.
> 
> Day by day, it seems like there is more and more of this PTP driver
> stuff.  Maybe it is time for me to manage a separate PTP driver tree.
> I could get the reviews and acks, then place a PR to netdev or lkml
> when ready.
> 
> Thoughts?

Hi Richard

My perception is that you also like to sleep at nights, and cannot
keep up with David rapid pace. So setting up your own tree, collecting
reviews and acks yourself will help you and the quality of the PTP
code. So yes, go for it.  I just think it is wrong you have to do
this.

    Andrew

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Richard Cochran]

On Fri, Mar 04, 2022 at 03:17:19PM +0100, Andrew Lunn wrote:

> My perception is that you also like to sleep at nights, and cannot
> keep up with David rapid pace. So setting up your own tree, collecting
> reviews and acks yourself will help you and the quality of the PTP
> code. So yes, go for it.  I just think it is wrong you have to do
> this.

I agree that PTP drivers are being merged too quickly without enough
review, and my commitment to maintaining a PTP tree will both allow
better review and reduce davem's burden.

So maybe it is right for me to do this after all.

Thanks,
Richard

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Michael Walle]

From: patchwork-bot+netdevbpf@kernel.org

> Here is the summary with links:
>   - [net-next,1/3] net: phy: micrel: Fix concurrent register access
>     https://git.kernel.org/netdev/net-next/c/4488f6b61480
>   - [net-next,2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy
>     https://git.kernel.org/netdev/net-next/c/2358dd3fd325
>   - [net-next,3/3] net: phy: micrel: 1588 support for LAN8814 phy
>     https://git.kernel.org/netdev/net-next/c/ece19502834d

I'm almost afraid to ask.. but will this series be reverted (or
the device tree bindings patch)? There were quite a few remarks, even
about the naming of the properties. So, will it be part of the next
kernel release or will it be reverted?

-michael

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Allan W. Nielsen]

Hi,

On Thu, Mar 17, 2022 at 01:16:50PM +0100, Michael Walle wrote:
> From: patchwork-bot+netdevbpf@kernel.org
> > Here is the summary with links:
> >   - [net-next,1/3] net: phy: micrel: Fix concurrent register access
> >     https://git.kernel.org/netdev/net-next/c/4488f6b61480
> >   - [net-next,2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy
> >     https://git.kernel.org/netdev/net-next/c/2358dd3fd325
> >   - [net-next,3/3] net: phy: micrel: 1588 support for LAN8814 phy
> >     https://git.kernel.org/netdev/net-next/c/ece19502834d
> 
> I'm almost afraid to ask.. but will this series be reverted (or
> the device tree bindings patch)? There were quite a few remarks, even
> about the naming of the properties. So, will it be part of the next
> kernel release or will it be reverted?
Thanks for bringing this up - was about to ask myself.

Not sure what is the normal procedure here.

If not reverted, we can do a patch to remove the dt-bindings (and also
the code in the driver using them). Also, a few other minor comments was
given and we can fix those.

The elefant in the room is the 'lan8814_latencies' structure containing
the default latency values in the driver, which Richard is unhappy with.

Russell indicated that he prefere having these numbers in the driver
rather than hiding them in firmware (lan8814 does not have firmware, so
not an option).

Andrew sugegsted adding additional APIs to let ptp4l control if
time-stamps should be calibrated in HW/Kernel or in userspace. Likely
something like that can be done - but I did not get the impression that
this is what Richard would like to see either.

Also, I would like drivers to come with default latency numbers which
are good enough for most (and the rest will need to calibrate and
compensate further using the hooks and handles in userspace).

What would you like to see - believe you will also be a user of this?

-- 
/Allan

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Andrew Lunn]

On Thu, Mar 17, 2022 at 03:05:59PM +0100, Allan W. Nielsen wrote:
> Hi,
> 
> On Thu, Mar 17, 2022 at 01:16:50PM +0100, Michael Walle wrote:
> > From: patchwork-bot+netdevbpf@kernel.org
> > > Here is the summary with links:
> > >   - [net-next,1/3] net: phy: micrel: Fix concurrent register access
> > >     https://git.kernel.org/netdev/net-next/c/4488f6b61480
> > >   - [net-next,2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy
> > >     https://git.kernel.org/netdev/net-next/c/2358dd3fd325
> > >   - [net-next,3/3] net: phy: micrel: 1588 support for LAN8814 phy
> > >     https://git.kernel.org/netdev/net-next/c/ece19502834d
> > 
> > I'm almost afraid to ask.. but will this series be reverted (or
> > the device tree bindings patch)? There were quite a few remarks, even
> > about the naming of the properties. So, will it be part of the next
> > kernel release or will it be reverted?
> Thanks for bringing this up - was about to ask myself.
> 
> Not sure what is the normal procedure here.

I assume this is in net-next. So we have two weeks of the merge window
followed by around 7 weeks of the -rc in order to clean this up. It is
only when the code is released in a final kernel does it become an
ABI.

> If not reverted, we can do a patch to remove the dt-bindings (and also
> the code in the driver using them). Also, a few other minor comments was
> given and we can fix those.

Patches would be good. Ideally the patches would be posted in the next
couple of weeks, even if we do have a lot longer.

> The elefant in the room is the 'lan8814_latencies' structure containing
> the default latency values in the driver, which Richard is unhappy with.

The important thing is getting the ABI fixed. So the DT properties
need to be removed, etc.

To some extend the corrections are ABI. If the corrections change the
user space configuration also needs to change when trying to get the
best out of the hardware. So depending on how long the elefant is
around, it might make sense to actually do a revert, or at minimum
disabling PTP, so time can be spent implementing new APIs or whatever
is decided.

So i would suggest a two pronged attach:

Fixup patchs
Try to bring the discussion to a close and implement whatever is decided.

   Andrew

Re: [PATCH net-next 0/3] Add support for 1588 in LAN8814

[Allan W. Nielsen]

On Thu, Mar 17, 2022 at 03:38:50PM +0100, Andrew Lunn wrote:
> On Thu, Mar 17, 2022 at 03:05:59PM +0100, Allan W. Nielsen wrote:
> > Hi,
> >
> > On Thu, Mar 17, 2022 at 01:16:50PM +0100, Michael Walle wrote:
> > > From: patchwork-bot+netdevbpf@kernel.org
> > > > Here is the summary with links:
> > > >   - [net-next,1/3] net: phy: micrel: Fix concurrent register access
> > > >     https://git.kernel.org/netdev/net-next/c/4488f6b61480
> > > >   - [net-next,2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy
> > > >     https://git.kernel.org/netdev/net-next/c/2358dd3fd325
> > > >   - [net-next,3/3] net: phy: micrel: 1588 support for LAN8814 phy
> > > >     https://git.kernel.org/netdev/net-next/c/ece19502834d
> > >
> > > I'm almost afraid to ask.. but will this series be reverted (or
> > > the device tree bindings patch)? There were quite a few remarks, even
> > > about the naming of the properties. So, will it be part of the next
> > > kernel release or will it be reverted?
> > Thanks for bringing this up - was about to ask myself.
> >
> > Not sure what is the normal procedure here.
> 
> I assume this is in net-next. So we have two weeks of the merge window
> followed by around 7 weeks of the -rc in order to clean this up. It is
> only when the code is released in a final kernel does it become an
> ABI.
> 
> > If not reverted, we can do a patch to remove the dt-bindings (and also
> > the code in the driver using them). Also, a few other minor comments was
> > given and we can fix those.
> 
> Patches would be good. Ideally the patches would be posted in the next
> couple of weeks, even if we do have a lot longer.
> 
> > The elefant in the room is the 'lan8814_latencies' structure containing
> > the default latency values in the driver, which Richard is unhappy with.
> 
> The important thing is getting the ABI fixed. So the DT properties
> need to be removed, etc.
We will do that.

> To some extend the corrections are ABI. If the corrections change the
> user space configuration also needs to change when trying to get the
> best out of the hardware. So depending on how long the elefant is
> around, it might make sense to actually do a revert, or at minimum
> disabling PTP, so time can be spent implementing new APIs or whatever
> is decided.
ACK.

> So i would suggest a two pronged attach:
> 
> Fixup patchs
> Try to bring the discussion to a close and implement whatever is decided.
Make sense - we will do the fix-ups and try restart the dicussion.

-- 
/Allan