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

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

---
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]

[1] https://lkml.org/lkml/2022/4/1/231

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 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 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 | 103 +--------------------------------------
 1 file changed, 1 insertion(+), 102 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 19b11e896460..886ea99b3906 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,30 +2614,8 @@ 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;
 	struct kszphy_priv *priv;
 	u16 addr;
 	int err;
@@ -2724,8 +2626,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 +2646,6 @@ static int lan8814_probe(struct phy_device *phydev)
 			return err;
 	}
 
-	lan8814_parse_latency(phydev);
 	lan8814_ptp_init(phydev);
 
 	return 0;
@@ -2928,7 +2827,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 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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 886ea99b3906..fc53b71dc872 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -2629,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 1/3] dt-bindings: net: micrel: Revert latency support and timestamping check

[Andrew Lunn]

On Fri, Apr 01, 2022 at 11:48:03AM +0200, Horatiu Vultur wrote:
> 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

On Fri, Apr 01, 2022 at 11:48:04AM +0200, Horatiu Vultur wrote:
> 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>

Thanks for the revert.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

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

What are the reset defaults of these? I'm just wondering if we should
explicitly set them to 0, so we don't get into a mess where some
vendor bootloader sets values but mainline bootloader does not,
breaking a configuration where the userspace daemon does the correct?

	 Andrew

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

[Andrew Lunn]

On Fri, Apr 01, 2022 at 11:48:05AM +0200, Horatiu Vultur wrote:
> 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Horatiu Vultur]

The 04/01/2022 14:47, Andrew Lunn wrote:
> 
> On Fri, Apr 01, 2022 at 11:48:04AM +0200, Horatiu Vultur wrote:
> > 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>
> 
> Thanks for the revert.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> > -static struct kszphy_latencies lan8814_latencies = {
> > -     .rx_10          = 0x22AA,
> > -     .tx_10          = 0x2E4A,
> > -     .rx_100         = 0x092A,
> > -     .tx_100         = 0x02C1,
> > -     .rx_1000        = 0x01AD,
> > -     .tx_1000        = 0x00C9,
> > -};
> 
> What are the reset defaults of these? 

Those are actually the reset values.

> I'm just wondering if we should
> explicitly set them to 0, so we don't get into a mess where some
> vendor bootloader sets values but mainline bootloader does not,
> breaking a configuration where the userspace daemon does the correct?

It would be fine for me to set them to 0. But then definitely we need a
way to set these latencies from userspace.

> 
>          Andrew

-- 
/Horatiu

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

[Allan W. Nielsen]

On 01.04.2022 15:34, Horatiu Vultur wrote:
>The 04/01/2022 14:47, Andrew Lunn wrote:
>>
>> On Fri, Apr 01, 2022 at 11:48:04AM +0200, Horatiu Vultur wrote:
>> > 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>
>>
>> Thanks for the revert.
>>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>
>> > -static struct kszphy_latencies lan8814_latencies = {
>> > -     .rx_10          = 0x22AA,
>> > -     .tx_10          = 0x2E4A,
>> > -     .rx_100         = 0x092A,
>> > -     .tx_100         = 0x02C1,
>> > -     .rx_1000        = 0x01AD,
>> > -     .tx_1000        = 0x00C9,
>> > -};
>>
>> What are the reset defaults of these?
>
>Those are actually the reset values.
>
>> I'm just wondering if we should
>> explicitly set them to 0, so we don't get into a mess where some
>> vendor bootloader sets values but mainline bootloader does not,
>> breaking a configuration where the userspace daemon does the correct?
>
>It would be fine for me to set them to 0. But then definitely we need a
>way to set these latencies from userspace.
I would like to keep the default values. With default values, you can
get PTP working (accuracy is not great - but it is much better than
if set to zero).

There is no risk of bootloaders to pre-load other values, as the kernel
will reset the PHY, and after reset we will be back to these numbers.

/Allan

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

[Andrew Lunn]

On Fri, Apr 01, 2022 at 03:34:54PM +0200, Horatiu Vultur wrote:
> The 04/01/2022 14:47, Andrew Lunn wrote:
> > 
> > On Fri, Apr 01, 2022 at 11:48:04AM +0200, Horatiu Vultur wrote:
> > > 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>
> > 
> > Thanks for the revert.
> > 
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > 
> > > -static struct kszphy_latencies lan8814_latencies = {
> > > -     .rx_10          = 0x22AA,
> > > -     .tx_10          = 0x2E4A,
> > > -     .rx_100         = 0x092A,
> > > -     .tx_100         = 0x02C1,
> > > -     .rx_1000        = 0x01AD,
> > > -     .tx_1000        = 0x00C9,
> > > -};
> > 
> > What are the reset defaults of these? 
> 
> Those are actually the reset values.

Does the driver do a reset, so we can assume the hardware is actually
using these?

      Andrew

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

[Andrew Lunn]

> I would like to keep the default values. With default values, you can
> get PTP working (accuracy is not great - but it is much better than
> if set to zero).
> 
> There is no risk of bootloaders to pre-load other values, as the kernel
> will reset the PHY, and after reset we will be back to these numbers.

O.K, that is what i wanted to know. It should be reasonable safe to
assume these values, and userspace daemons can apply whatever
correction they want, assuming this is what the hardware is doing.

       Andrew