[PATCH net v2 0/3] net: phy: micrel: Remove latencies support lan8814

[PATCH net v2 0/3] net: phy: micrel: Remove latencies support lan8814

[Horatiu Vultur]

Remove the latencies support both from the PHY driver and from the DT.
The IP already has some default latencies values which can be used to get
decent results. It has the following values(defined in ns):
rx-1000mbit: 429
tx-1000mbit: 201
rx-100mbit:  2346
tx-100mbit:  705

v0->v1:
- fix the split of the patches, there was a compiling error between patch 2 and
  patch 3.

---
But to get better results the following values needs to be set:
rx-1000mbit: 459
tx-1000mbit: 171
rx-100mbit:  1706
tx-100mbit:  1345

We are proposing to use ethtool to set these latencies, the RFC can be found
here[1]

Horatiu Vultur (3):
  dt-bindings: net: micrel: Revert latency support and timestamping
    check
  net: phy: micrel: Remove latency from driver
  net: phy: micrel: Remove DT option lan8814,ignore-ts

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

-- 
2.33.0

[PATCH net v2 1/3] dt-bindings: net: micrel: Revert latency support and timestamping check

[Horatiu Vultur]

Revert latency support from binding.
Based on the discussion[1], the DT is the wrong place to have the
lantecies for the PHY.

[1] https://lkml.org/lkml/2022/3/4/325

Fixes: 2358dd3fd325fc ("dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../devicetree/bindings/net/micrel.txt          | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
index c5ab62c39133..8d157f0295a5 100644
--- a/Documentation/devicetree/bindings/net/micrel.txt
+++ b/Documentation/devicetree/bindings/net/micrel.txt
@@ -45,20 +45,3 @@ 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.33.0

[PATCH net v2 2/3] net: phy: micrel: Remove latency from driver

[Horatiu Vultur]

Based on the discussions here[1], the PHY driver is the wrong place
to set the latencies, therefore remove them.

[1] https://lkml.org/lkml/2022/3/4/325

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

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 19b11e896460..a873df07ad24 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -99,15 +99,6 @@
 #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
 
@@ -268,15 +259,6 @@ struct lan8814_ptp_rx_ts {
 	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;
@@ -296,7 +278,6 @@ struct kszphy_ptp_priv {
 
 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;
@@ -304,14 +285,6 @@ 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,
@@ -2618,55 +2591,6 @@ static int lan8814_ptp_probe_once(struct phy_device *phydev)
 	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;
@@ -2690,27 +2614,6 @@ static int lan8814_config_init(struct phy_device *phydev)
 	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;
@@ -2724,8 +2627,6 @@ static int lan8814_probe(struct phy_device *phydev)
 
 	priv->led_mode = -1;
 
-	priv->latencies = lan8814_latencies;
-
 	phydev->priv = priv;
 
 	if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
@@ -2746,7 +2647,6 @@ static int lan8814_probe(struct phy_device *phydev)
 			return err;
 	}
 
-	lan8814_parse_latency(phydev);
 	lan8814_ptp_init(phydev);
 
 	return 0;
@@ -2928,7 +2828,7 @@ static struct phy_driver ksphy_driver[] = {
 	.config_init	= lan8814_config_init,
 	.probe		= lan8814_probe,
 	.soft_reset	= genphy_soft_reset,
-	.read_status	= lan8814_read_status,
+	.read_status	= ksz9031_read_status,
 	.get_sset_count	= kszphy_get_sset_count,
 	.get_strings	= kszphy_get_strings,
 	.get_stats	= kszphy_get_stats,
-- 
2.33.0

[PATCH net v2 3/3] net: phy: micrel: Remove DT option lan8814,ignore-ts

[Horatiu Vultur]

When the PHY and the MAC are capable of doing timestamping, the PHY has
priority. Therefore the DT option lan8814,ignore-ts was added such that
the PHY will not expose a PHC so then the timestamping was done in the
MAC. This is not the correct approach of doing it, therefore remove
this.

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

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index a873df07ad24..fc53b71dc872 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -2616,7 +2616,6 @@ static int lan8814_config_init(struct phy_device *phydev)
 
 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;
@@ -2630,8 +2629,7 @@ static int lan8814_probe(struct phy_device *phydev)
 	phydev->priv = priv;
 
 	if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
-	    !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING) ||
-	    of_property_read_bool(np, "lan8814,ignore-ts"))
+	    !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
 		return 0;
 
 	/* Strap-in value for PHY address, below register read gives starting
-- 
2.33.0

Re: [PATCH net v2 0/3] net: phy: micrel: Remove latencies support lan8814

[Andrew Lunn]

On Fri, Apr 01, 2022 at 01:05:19PM +0200, Horatiu Vultur wrote:
> Remove the latencies support both from the PHY driver and from the DT.
> The IP already has some default latencies values which can be used to get
> decent results. It has the following values(defined in ns):
> rx-1000mbit: 429
> tx-1000mbit: 201
> rx-100mbit:  2346
> tx-100mbit:  705

So one alternative option here is that ptp4l looks at

/sys/class/net/<ifname>/phydev/phy_id

to identify the PHY, listens to netlink messages to determine the link
speed and then applies the correction itself in user space. That gives
you a pretty generic solution, works for any existing PHY and pretty
much any existing kernel version.  And if you want board specific
values you can override them in the ptp4l configuration file.

       Andrew

Re: [PATCH net v2 0/3] net: phy: micrel: Remove latencies support lan8814

[Allan W. Nielsen]

On 01.04.2022 14:57, Andrew Lunn wrote:
>On Fri, Apr 01, 2022 at 01:05:19PM +0200, Horatiu Vultur wrote:
>> Remove the latencies support both from the PHY driver and from the DT.
>> The IP already has some default latencies values which can be used to get
>> decent results. It has the following values(defined in ns):
>> rx-1000mbit: 429
>> tx-1000mbit: 201
>> rx-100mbit:  2346
>> tx-100mbit:  705
>
>So one alternative option here is that ptp4l looks at
>
>/sys/class/net/<ifname>/phydev/phy_id
>
>to identify the PHY, listens to netlink messages to determine the link
>speed and then applies the correction itself in user space. That gives
>you a pretty generic solution, works for any existing PHY and pretty
>much any existing kernel version.  And if you want board specific
>values you can override them in the ptp4l configuration file.
I think it is good to have both options. If you want PTP4L to compensate
in user-space, do not call the tunable, if you want to HW to compensate,
call the tunable (this is useful both for users using ptp4l and other
ptpimplementations).

If system behaves strange, it is easy to see what delays has been
applied.

We are planning on creating a small proejct, which go through all PHYs
in the current system. It shall check a config file to see if the user
has configured interface specific numbers, then apply them, other wise
see if we have default numbers based on the PHY-ID (like you describe).

Idea is to run this at boot as a one-off. It will apply the adjustments
on all speeds, all capable PHYs regardless of they PHY-Timestamping is
used or not.

BTW: If there is a desire, we can add a flag to this tool which can set
all the delay-values to zero.

Just for the record: It is not that I do not like PTP4L - it is by far
the best PTP implementation I have seen. But I'm keen on having a
solution that also works for non-ptp4l users.

/Allan

Re: [PATCH net v2 0/3] net: phy: micrel: Remove latencies support lan8814

[Andrew Lunn]

On Fri, Apr 01, 2022 at 04:11:20PM +0200, Allan W. Nielsen wrote:
> On 01.04.2022 14:57, Andrew Lunn wrote:
> > On Fri, Apr 01, 2022 at 01:05:19PM +0200, Horatiu Vultur wrote:
> > > Remove the latencies support both from the PHY driver and from the DT.
> > > The IP already has some default latencies values which can be used to get
> > > decent results. It has the following values(defined in ns):
> > > rx-1000mbit: 429
> > > tx-1000mbit: 201
> > > rx-100mbit:  2346
> > > tx-100mbit:  705
> > 
> > So one alternative option here is that ptp4l looks at
> > 
> > /sys/class/net/<ifname>/phydev/phy_id
> > 
> > to identify the PHY, listens to netlink messages to determine the link
> > speed and then applies the correction itself in user space. That gives
> > you a pretty generic solution, works for any existing PHY and pretty
> > much any existing kernel version.  And if you want board specific
> > values you can override them in the ptp4l configuration file.
>
> I think it is good to have both options. If you want PTP4L to compensate
> in user-space, do not call the tunable, if you want to HW to compensate,
> call the tunable (this is useful both for users using ptp4l and other
> ptpimplementations).
>
> If system behaves strange, it is easy to see what delays has been
> applied.

I don't actually think that is true. How wound i know that

> > > rx-1000mbit: 429
> > > tx-1000mbit: 201
> > > rx-100mbit:  2346
> > > tx-100mbit:  705

are the default values? I cannot just look at them and obviously see
they are not the default values. I would need to learn what the
default values are of every PHY in linux which allows the PHY to
perform a correction.

Are you also saying that ptp4l needs to read the values from the
driver, calculate the differ from the defaults, and then apply that
difference to the correction specified in the configuration file it
will apply in userspace?

Does the PTP API enforce mutual exclusion for a device? Can there be
multiple applications running on an interface, some which assume the
hardware is configured to perform corrections and some which will
apply the correction in user space?

Richard?

	Andrew

Re: [PATCH net v2 0/3] net: phy: micrel: Remove latencies support lan8814

[Russell King (Oracle)]

On Fri, Apr 01, 2022 at 02:57:46PM +0200, Andrew Lunn wrote:
> On Fri, Apr 01, 2022 at 01:05:19PM +0200, Horatiu Vultur wrote:
> > Remove the latencies support both from the PHY driver and from the DT.
> > The IP already has some default latencies values which can be used to get
> > decent results. It has the following values(defined in ns):
> > rx-1000mbit: 429
> > tx-1000mbit: 201
> > rx-100mbit:  2346
> > tx-100mbit:  705
> 
> So one alternative option here is that ptp4l looks at
> 
> /sys/class/net/<ifname>/phydev/phy_id

That doesn't work for Clause 45 PHYs, only Clause 22 PHYs. If we want
userspace to know which PHY it is, we need a proper interface that
exports all the 31 Clause 45 IDs (each mmd's registers 2/3) as well, as
well as the Clause 45 package ID (registers 14/15).

-- 
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 v2 0/3] net: phy: micrel: Remove latencies support lan8814

[Richard Cochran]

On Fri, Apr 01, 2022 at 04:39:10PM +0200, Andrew Lunn wrote:

> Are you also saying that ptp4l needs to read the values from the
> driver, calculate the differ from the defaults, and then apply that
> difference to the correction specified in the configuration file it
> will apply in userspace?

Personally I wouldn't bother with that.  At the end of day, users who
care about sub-microsecond performance will need to calibrate their
particular setup.  The output of the calibration will be the system
delay asymmetry correction.  That number will be applied in ONE place,
namely the user space PTP stack.  Breaking it up into little bits is
just extra work for no benefit.

That is why I'm against any of this driver nonsense.  The only purpose
of putting values in to the driver is to unpleasantly surprise the end
users after kernel upgrade.

If this driver defaults + run time query/setting stuff goes mainline,
I'll never use it.

> Does the PTP API enforce mutual exclusion for a device? Can there be
> multiple applications running on an interface, some which assume the
> hardware is configured to perform corrections and some which will
> apply the correction in user space?

There is no mutual exclusion at the kernel API.  The main hindrance is
the SIOCSHWTSTAMP ioctl which is at the device level.  The setting
applies system wide and is a root caps operation.  There is the "Get"
variant that allows co-operation but does not enforce it.

Thanks,
Richard

Re: [PATCH net v2 0/3] net: phy: micrel: Remove latencies support lan8814

[patchwork-bot+netdevbpf]

Hello:

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

On Fri, 1 Apr 2022 13:05:19 +0200 you wrote:
> Remove the latencies support both from the PHY driver and from the DT.
> The IP already has some default latencies values which can be used to get
> decent results. It has the following values(defined in ns):
> rx-1000mbit: 429
> tx-1000mbit: 201
> rx-100mbit:  2346
> tx-100mbit:  705
> 
> [...]

Here is the summary with links:
  - [net,v2,1/3] dt-bindings: net: micrel: Revert latency support and timestamping check
    https://git.kernel.org/netdev/net/c/b117c88df0e3
  - [net,v2,2/3] net: phy: micrel: Remove latency from driver
    https://git.kernel.org/netdev/net/c/b814403a8cd8
  - [net,v2,3/3] net: phy: micrel: Remove DT option lan8814,ignore-ts
    https://git.kernel.org/netdev/net/c/76e9ccd68943

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