| Message ID | 20250308200921.1089980-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add GBETH glue layer driver for Renesas RZ/V2H(P) SoC | expand |
On Sat, Mar 08, 2025 at 08:09:21PM +0000, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Add the DWMAC glue layer for the GBETH IP found in the Renesas RZ/V2H(P) > SoC. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v1->v2 > - Dropped __initconst for renesas_gbeth_clks array > - Added clks_config callback > - Dropped STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag as this needs > investigation. I thought you had got to the bottom of this, and it was a bug in your clock driver? > + * The Rx and Tx clocks are supplied as follows for the GBETH IP. > + * > + * Rx / Tx > + * -------+------------- on / off ------- > + * | > + * | Rx-180 / Tx-180 > + * +---- not ---- on / off ------- Thanks for the diagram. > +struct renesas_gbeth { > + struct device *dev; > + void __iomem *regs; > + unsigned int num_clks; > + struct clk *clk_tx_i; > + struct clk_bulk_data *clks; > + struct reset_control *rstc; > +}; If you stored a pointer to struct plat_stmmacenet_data, then you wouldn't need num_clks, clk_tx_i or clks. If you look at dwmac-dwc-qos-eth.c, I recently added a helper (dwc_eth_find_clk()) which could be made generic. You can then include the clk_tx_i clock in the bulk clock, and use the helper to set plat_dat->clk_tx_i. > + plat_dat->flags |= STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY | > + STMMAC_FLAG_EN_TX_LPI_CLOCKGATING | Didn't I send you a patch that provides STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP so we can move towards the PHY saying whether it permits the TX clock to be disabled?
Hi Russell, Thank you for the review. On Sun, Mar 9, 2025 at 8:50 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Sat, Mar 08, 2025 at 08:09:21PM +0000, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Add the DWMAC glue layer for the GBETH IP found in the Renesas RZ/V2H(P) > > SoC. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > v1->v2 > > - Dropped __initconst for renesas_gbeth_clks array > > - Added clks_config callback > > - Dropped STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag as this needs > > investigation. > > I thought you had got to the bottom of this, and it was a bug in your > clock driver? > I have added a fix in the clock driver to ignore CLK_MON bits for external clocks. The main reason for dropping this flag was despite trying the below i.e. adding phy_eee_rx_clock_stop() just before unregister_netdev() in stmmac_dvr_remove() still doesnt stop the Rx clocks. if (ndev->phydev) phy_eee_rx_clock_stop(ndev->phydev, false); Note, on another platform where I can issue a reset to the PHY I issued the reset after unbind operation and monitored the Rx clock using CLK_MON and I noticed they reported Rx clocks were OFF. But on the current platform I cannot issue a reset to the PHY after unbind operation. > > + * The Rx and Tx clocks are supplied as follows for the GBETH IP. > > + * > > + * Rx / Tx > > + * -------+------------- on / off ------- > > + * | > > + * | Rx-180 / Tx-180 > > + * +---- not ---- on / off ------- > > Thanks for the diagram. > > > +struct renesas_gbeth { > > + struct device *dev; > > + void __iomem *regs; > > + unsigned int num_clks; > > + struct clk *clk_tx_i; > > + struct clk_bulk_data *clks; > > + struct reset_control *rstc; > > +}; > > If you stored a pointer to struct plat_stmmacenet_data, then you > wouldn't need num_clks, clk_tx_i or clks. If you look at > dwmac-dwc-qos-eth.c, I recently added a helper (dwc_eth_find_clk()) > which could be made generic. > > You can then include the clk_tx_i clock in the bulk clock, and > use the helper to set plat_dat->clk_tx_i. > Thanks for the pointer, I'll switch to that. > > + plat_dat->flags |= STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY | > > + STMMAC_FLAG_EN_TX_LPI_CLOCKGATING | > > Didn't I send you a patch that provides > STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP so we can move towards the PHY > saying whether it permits the TX clock to be disabled? > I'll rebase my changes on top of [0]. Do you want me to run any specific tests for this? [0] https://patchwork.kernel.org/project/netdevbpf/patch/E1trCPy-005jZf-Ou@rmk-PC.armlinux.org.uk/ Cheers, Prabhakar
On Sun, Mar 09, 2025 at 11:24:57AM +0000, Lad, Prabhakar wrote: > Hi Russell, > > Thank you for the review. > > On Sun, Mar 9, 2025 at 8:50 AM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Sat, Mar 08, 2025 at 08:09:21PM +0000, Prabhakar wrote: > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Add the DWMAC glue layer for the GBETH IP found in the Renesas RZ/V2H(P) > > > SoC. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > v1->v2 > > > - Dropped __initconst for renesas_gbeth_clks array > > > - Added clks_config callback > > > - Dropped STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag as this needs > > > investigation. > > > > I thought you had got to the bottom of this, and it was a bug in your > > clock driver? > > > I have added a fix in the clock driver to ignore CLK_MON bits for > external clocks. The main reason for dropping this flag was despite > trying the below i.e. adding phy_eee_rx_clock_stop() just before > unregister_netdev() in stmmac_dvr_remove() still doesnt stop the Rx > clocks. That's not unexpected, because phy_eee_rx_clock_stop() does not control a clock gate. What phy_eee_rx_clock_stop() does is control the clock stop enable bit in the PHY. Please see IEEE 802.3 section 45.2.3.1.4 and other sections referred to from that section to gain the appropriate understanding. The point of adding the phy_eee_rx_clock_stop() call before stmmac_dvr_remove() was to _test_ (and *only as a test* - a point that I did stress) to see whether preventing the PHY from stopping it's receive clock solved the reset timeout on module reload. This test only makes sense if STMMAC_FLAG_RX_CLK_RUNS_IN_LPI has not been set. As I understand it, you have found the real issue why that occurs, so it seems there is little need to continue with that test if, and only if, everything is now working reliably when removing and re-inserting the module. The key point here is "reliably". The receive side of the link *could* enter or exit LPI at *any* moment - it depends in the link partner. If the PHY has permission to stop it's receive clock, then this might lead to stmmac_reset() timing out because the PHY has stopped it's receive clock _if_ the receive-side LPI persists longer than the reset timeout. At this point, I am not certain what the current situation is. Are you now setting STMMAC_FLAG_RX_CLK_RUNS_IN_LPI because it solves a problem? If the answer is yes, then there is still a bug in the driver that needs to be solved and I've presented several solutions to that. I want to remove STMMAC_FLAG_RX_CLK_RUNS_IN_LPI from the stmmac driver so I'm going to NACK patches that add new uses. Sorry, but we need to solve the root problem, and stop hacking around it with flags to change behaviours.
Hi Russell, On Sun, Mar 9, 2025 at 12:19 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Sun, Mar 09, 2025 at 11:24:57AM +0000, Lad, Prabhakar wrote: > > Hi Russell, > > > > Thank you for the review. > > > > On Sun, Mar 9, 2025 at 8:50 AM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > > > > On Sat, Mar 08, 2025 at 08:09:21PM +0000, Prabhakar wrote: > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > Add the DWMAC glue layer for the GBETH IP found in the Renesas RZ/V2H(P) > > > > SoC. > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > v1->v2 > > > > - Dropped __initconst for renesas_gbeth_clks array > > > > - Added clks_config callback > > > > - Dropped STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag as this needs > > > > investigation. > > > > > > I thought you had got to the bottom of this, and it was a bug in your > > > clock driver? > > > > > I have added a fix in the clock driver to ignore CLK_MON bits for > > external clocks. The main reason for dropping this flag was despite > > trying the below i.e. adding phy_eee_rx_clock_stop() just before > > unregister_netdev() in stmmac_dvr_remove() still doesnt stop the Rx > > clocks. > > That's not unexpected, because phy_eee_rx_clock_stop() does not control > a clock gate. > > What phy_eee_rx_clock_stop() does is control the clock stop enable bit > in the PHY. Please see IEEE 802.3 section 45.2.3.1.4 and other sections > referred to from that section to gain the appropriate understanding. > > The point of adding the phy_eee_rx_clock_stop() call before > stmmac_dvr_remove() was to _test_ (and *only as a test* - a point that > I did stress) to see whether preventing the PHY from stopping it's > receive clock solved the reset timeout on module reload. This test > only makes sense if STMMAC_FLAG_RX_CLK_RUNS_IN_LPI has not been set. > Thank you for your clarification. > As I understand it, you have found the real issue why that occurs, so > it seems there is little need to continue with that test if, and only > if, everything is now working reliably when removing and re-inserting > the module. > > The key point here is "reliably". The receive side of the link *could* > enter or exit LPI at *any* moment - it depends in the link partner. If > the PHY has permission to stop it's receive clock, then this might lead > to stmmac_reset() timing out because the PHY has stopped it's receive > clock _if_ the receive-side LPI persists longer than the reset timeout. > > At this point, I am not certain what the current situation is. Are you > now setting STMMAC_FLAG_RX_CLK_RUNS_IN_LPI because it solves a problem? > If the answer is yes, then there is still a bug in the driver that needs > to be solved and I've presented several solutions to that. > In this series I have dropped the STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag with this unbind/bind works without any issues. > I want to remove STMMAC_FLAG_RX_CLK_RUNS_IN_LPI from the stmmac driver > so I'm going to NACK patches that add new uses. Sorry, but we need to > solve the root problem, and stop hacking around it with flags to change > behaviours. > I have retested the glue driver with and without STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag and in both the cases I can see the unbind/bind operation working without any timeouts (attached are the logs). Ive reworked your suggestions from v2, please let me know if I can send out a v3 without STMMAC_FLAG_RX_CLK_RUNS_IN_LPI and with the new STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP flag added. Cheers, Prabhakar
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig index 3c820ef56775..2c99b23f0faa 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig @@ -131,6 +131,17 @@ config DWMAC_QCOM_ETHQOS This selects the Qualcomm ETHQOS glue layer support for the stmmac device driver. +config DWMAC_RENESAS_GBETH + tristate "Renesas RZ/V2H(P) GBETH support" + default ARCH_RENESAS + depends on OF && (ARCH_RENESAS || COMPILE_TEST) + help + Support for Gigabit Ethernet Interface (GBETH) on Renesas + RZ/V2H(P) SoCs. + + This selects the Renesas RZ/V2H(P) Soc specific glue layer support + for the stmmac device driver. + config DWMAC_ROCKCHIP tristate "Rockchip dwmac support" default ARCH_ROCKCHIP diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile index 594883fb4164..91050215511b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Makefile +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_DWMAC_LPC18XX) += dwmac-lpc18xx.o obj-$(CONFIG_DWMAC_MEDIATEK) += dwmac-mediatek.o obj-$(CONFIG_DWMAC_MESON) += dwmac-meson.o dwmac-meson8b.o obj-$(CONFIG_DWMAC_QCOM_ETHQOS) += dwmac-qcom-ethqos.o +obj-$(CONFIG_DWMAC_RENESAS_GBETH) += dwmac-renesas-gbeth.o obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o obj-$(CONFIG_DWMAC_RZN1) += dwmac-rzn1.o obj-$(CONFIG_DWMAC_S32) += dwmac-s32.o diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c new file mode 100644 index 000000000000..7527387f30a0 --- /dev/null +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c @@ -0,0 +1,165 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * dwmac-renesas-gbeth.c - DWMAC Specific Glue layer for Renesas GBETH + * + * The Rx and Tx clocks are supplied as follows for the GBETH IP. + * + * Rx / Tx + * -------+------------- on / off ------- + * | + * | Rx-180 / Tx-180 + * +---- not ---- on / off ------- + * + * Copyright (C) 2025 Renesas Electronics Corporation + */ + +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/reset.h> + +#include "dwmac4.h" +#include "stmmac_platform.h" + +struct renesas_gbeth { + struct device *dev; + void __iomem *regs; + unsigned int num_clks; + struct clk *clk_tx_i; + struct clk_bulk_data *clks; + struct reset_control *rstc; +}; + +static const char *const renesas_gbeth_clks[] = { + "rx", "rx-180", "tx-180", +}; + +static int renesas_gbeth_clks_config(void *priv, bool enabled) +{ + struct renesas_gbeth *gbeth = priv; + int ret; + + if (enabled) { + ret = reset_control_deassert(gbeth->rstc); + if (ret) { + dev_err(gbeth->dev, "Reset deassert failed\n"); + return ret; + } + + ret = clk_prepare_enable(gbeth->clk_tx_i); + if (ret) { + dev_err(gbeth->dev, "Tx clock enable failed\n"); + reset_control_assert(gbeth->rstc); + return ret; + } + + ret = clk_bulk_prepare_enable(gbeth->num_clks, gbeth->clks); + if (ret) + clk_disable_unprepare(gbeth->clk_tx_i); + } else { + clk_bulk_disable_unprepare(gbeth->num_clks, gbeth->clks); + clk_disable_unprepare(gbeth->clk_tx_i); + ret = reset_control_assert(gbeth->rstc); + if (ret) + dev_err(gbeth->dev, "Reset assert failed\n"); + } + + return ret; +} + +static int renesas_gbeth_probe(struct platform_device *pdev) +{ + struct plat_stmmacenet_data *plat_dat; + struct stmmac_resources stmmac_res; + struct device *dev = &pdev->dev; + struct renesas_gbeth *gbeth; + unsigned int i; + int err; + + err = stmmac_get_platform_resources(pdev, &stmmac_res); + if (err) + return dev_err_probe(dev, err, + "failed to get resources\n"); + + plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac); + if (IS_ERR(plat_dat)) + return dev_err_probe(dev, PTR_ERR(plat_dat), + "dt configuration failed\n"); + + gbeth = devm_kzalloc(dev, sizeof(*gbeth), GFP_KERNEL); + if (!gbeth) + return -ENOMEM; + + plat_dat->clk_tx_i = devm_clk_get(dev, "tx"); + if (IS_ERR(plat_dat->clk_tx_i)) + return dev_err_probe(dev, PTR_ERR(plat_dat->clk_tx_i), + "error getting tx clock\n"); + + gbeth->clk_tx_i = plat_dat->clk_tx_i; + gbeth->num_clks = ARRAY_SIZE(renesas_gbeth_clks); + gbeth->clks = devm_kcalloc(dev, gbeth->num_clks, + sizeof(*gbeth->clks), GFP_KERNEL); + if (!gbeth->clks) + return -ENOMEM; + + for (i = 0; i < gbeth->num_clks; i++) + gbeth->clks[i].id = renesas_gbeth_clks[i]; + + err = devm_clk_bulk_get(dev, gbeth->num_clks, gbeth->clks); + if (err < 0) + return err; + + gbeth->rstc = devm_reset_control_get_exclusive(dev, NULL); + if (IS_ERR(gbeth->rstc)) + return PTR_ERR(gbeth->rstc); + + gbeth->dev = dev; + gbeth->regs = stmmac_res.addr; + plat_dat->bsp_priv = gbeth; + plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate; + plat_dat->clks_config = renesas_gbeth_clks_config; + plat_dat->flags |= STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY | + STMMAC_FLAG_EN_TX_LPI_CLOCKGATING | + STMMAC_FLAG_SPH_DISABLE; + + err = renesas_gbeth_clks_config(gbeth, true); + if (err) + return err; + + err = stmmac_dvr_probe(dev, plat_dat, &stmmac_res); + if (err) { + renesas_gbeth_clks_config(gbeth, false); + return err; + } + + return 0; +} + +static void renesas_gbeth_remove(struct platform_device *pdev) +{ + stmmac_dvr_remove(&pdev->dev); + + renesas_gbeth_clks_config(get_stmmac_bsp_priv(&pdev->dev), false); +} + +static const struct of_device_id renesas_gbeth_match[] = { + { .compatible = "renesas,rzv2h-gbeth", }, + { /* Sentinel */ } +}; +MODULE_DEVICE_TABLE(of, renesas_gbeth_match); + +static struct platform_driver renesas_gbeth_driver = { + .probe = renesas_gbeth_probe, + .remove = renesas_gbeth_remove, + .driver = { + .name = "renesas-gbeth", + .pm = &stmmac_pltfr_pm_ops, + .of_match_table = renesas_gbeth_match, + }, +}; +module_platform_driver(renesas_gbeth_driver); + +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>"); +MODULE_DESCRIPTION("Renesas GBETH DWMAC Specific Glue layer"); +MODULE_LICENSE("GPL");