| Message ID | 20220401094805.3343464-3-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: micrel: Remove latencies support lan8814 | expand |
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
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
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
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
> 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
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,
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(-)