| Message ID | 20240415-rzn1-gmac1-v3-0-ab12f2c4401d@bootlin.com |
|---|---|
| Headers | show |
| Series | net: stmmac: Add support for RZN1 GMAC devices | expand |
Hi Romain, Russell On Mon, Apr 15, 2024 at 11:18:42AM +0200, Romain Gantois wrote: > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > Introduce a mechanism whereby platforms can create their PCS instances > prior to the network device being published to userspace, but after > some of the core stmmac initialisation has been completed. This means > that the data structures that platforms need will be available. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 ++++++++++++++ > include/linux/stmmac.h | 2 ++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index fe3498e86de9d..25fa33ae7017b 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -7208,6 +7208,12 @@ static int stmmac_hw_init(struct stmmac_priv *priv) > if (ret) > return ret; > > + if (priv->plat->pcs_init) { > + ret = priv->plat->pcs_init(priv, priv->hw); > + if (ret) > + return ret; > + } > + I am currently working on my Memory-mapped DW XPCS patchset cooking: https://lore.kernel.org/netdev/20231205103559.9605-1-fancer.lancer@gmail.com/ The changes in this series seems to intersect to what is/will be introduced in my patchset. In particular as before I am going to use the "pcs-handle" property for getting the XPCS node. If so what about collecting PCS-related things in a single place. Like this: int stmmac_xpcs_setup(struct net_device *ndev) { ... if (priv->plat->pcs_init) { return priv->plat->pcs_init(priv); /* Romain' part */ } else if (fwnode_property_present(priv->plat->port_node, "pcs-handle")) { /* My DW XPCS part */ } else if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) { /* Currently implemented procedure */ } ... } void stmmac_xpcs_clean(struct net_device *ndev) { ... if (priv->plat->pcs_exit) { priv->plat->pcs_exit(priv); return; } xpcs_destroy(priv->hw->xpcs); priv->hw->xpcs = NULL; } Please see the last two patches in my series: https://lore.kernel.org/netdev/20231205103559.9605-16-fancer.lancer@gmail.com/ https://lore.kernel.org/netdev/20231205103559.9605-17-fancer.lancer@gmail.com/ as a reference of how the changes could be provided. -Serge(y) > /* Get the HW capability (new GMAC newer than 3.50a) */ > priv->hw_cap_support = stmmac_get_hw_features(priv); > if (priv->hw_cap_support) { > @@ -7290,6 +7296,12 @@ static int stmmac_hw_init(struct stmmac_priv *priv) > return 0; > } > > +static void stmmac_hw_exit(struct stmmac_priv *priv) > +{ > + if (priv->plat->pcs_exit) > + priv->plat->pcs_exit(priv, priv->hw); > +} > + > static void stmmac_napi_add(struct net_device *dev) > { > struct stmmac_priv *priv = netdev_priv(dev); > @@ -7804,6 +7816,7 @@ int stmmac_dvr_probe(struct device *device, > priv->hw->pcs != STMMAC_PCS_RTBI) > stmmac_mdio_unregister(ndev); > error_mdio_register: > + stmmac_hw_exit(priv); > stmmac_napi_del(ndev); > error_hw_init: > destroy_workqueue(priv->wq); > @@ -7844,6 +7857,7 @@ void stmmac_dvr_remove(struct device *dev) > if (priv->hw->pcs != STMMAC_PCS_TBI && > priv->hw->pcs != STMMAC_PCS_RTBI) > stmmac_mdio_unregister(ndev); > + stmmac_hw_exit(priv); > destroy_workqueue(priv->wq); > mutex_destroy(&priv->lock); > bitmap_free(priv->af_xdp_zc_qps); > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > index dfa1828cd756a..941fde506e514 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -285,6 +285,8 @@ struct plat_stmmacenet_data { > int (*crosststamp)(ktime_t *device, struct system_counterval_t *system, > void *ctx); > void (*dump_debug_regs)(void *priv); > + int (*pcs_init)(struct stmmac_priv *priv, struct mac_device_info *hw); > + void (*pcs_exit)(struct stmmac_priv *priv, struct mac_device_info *hw); > void *bsp_priv; > struct clk *stmmac_clk; > struct clk *pclk; > > -- > 2.44.0 > >
On Mon, Apr 15, 2024 at 11:18:44AM +0200, Romain Gantois wrote: > [...] > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c > new file mode 100644 > index 0000000000000..e85524c2017cf > --- /dev/null > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c > @@ -0,0 +1,88 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2024 Schneider-Electric > + * > + * Clément Léger <clement.leger@bootlin.com> > + */ > + > +#include <linux/of.h> > +#include <linux/pcs-rzn1-miic.h> > +#include <linux/phylink.h> > +#include <linux/platform_device.h> > + > +#include "stmmac_platform.h" > +#include "stmmac.h" > + > +static int rzn1_dwmac_pcs_init(struct stmmac_priv *priv, > + struct mac_device_info *hw) AFAICS hw is unused, and the mac_device_info instance is reached via the priv pointer. What about dropping the unused argument then? > +{ > + struct device_node *np = priv->device->of_node; > + struct device_node *pcs_node; > + struct phylink_pcs *pcs; > + > + pcs_node = of_parse_phandle(np, "pcs-handle", 0); > + > + if (pcs_node) { > + pcs = miic_create(priv->device, pcs_node); > + of_node_put(pcs_node); > + if (IS_ERR(pcs)) > + return PTR_ERR(pcs); > + > + priv->hw->phylink_pcs = pcs; > + } > + > + return 0; > +} > + > +static void rzn1_dwmac_pcs_exit(struct stmmac_priv *priv, > + struct mac_device_info *hw) ditto. -Serge(y) > +{ > + if (priv->hw->phylink_pcs) > + miic_destroy(priv->hw->phylink_pcs); > +} > + > [...]
Hi Serge, On Tue, 16 Apr 2024, Serge Semin wrote: > I am currently working on my Memory-mapped DW XPCS patchset cooking: > https://lore.kernel.org/netdev/20231205103559.9605-1-fancer.lancer@gmail.com/ > The changes in this series seems to intersect to what is/will be > introduced in my patchset. In particular as before I am going to > use the "pcs-handle" property for getting the XPCS node. If so what > about collecting PCS-related things in a single place. Like this: > > int stmmac_xpcs_setup(struct net_device *ndev) > { > ... > > if (priv->plat->pcs_init) { > return priv->plat->pcs_init(priv); /* Romain' part */ > } else if (fwnode_property_present(priv->plat->port_node, "pcs-handle")) { > /* My DW XPCS part */ > } else if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) { > /* Currently implemented procedure */ > } > > ... > } That seems like a good idea to me, although those setup functions would have to be renamed to stmmac_pcs_setup/exit. Thanks,
On Wed, Apr 17, 2024 at 11:30:09AM +0200, Romain Gantois wrote: > Hi Serge, > > On Tue, 16 Apr 2024, Serge Semin wrote: > > > I am currently working on my Memory-mapped DW XPCS patchset cooking: > > https://lore.kernel.org/netdev/20231205103559.9605-1-fancer.lancer@gmail.com/ > > The changes in this series seems to intersect to what is/will be > > introduced in my patchset. In particular as before I am going to > > use the "pcs-handle" property for getting the XPCS node. If so what > > about collecting PCS-related things in a single place. Like this: > > > > int stmmac_xpcs_setup(struct net_device *ndev) > > { > > ... > > > > if (priv->plat->pcs_init) { > > return priv->plat->pcs_init(priv); /* Romain' part */ > > } else if (fwnode_property_present(priv->plat->port_node, "pcs-handle")) { > > /* My DW XPCS part */ > > } else if (priv->plat->mdio_bus_data && priv->plat->mdio_bus_data->has_xpcs) { > > /* Currently implemented procedure */ > > } > > > > ... > > } > > That seems like a good idea to me, although those setup functions would have to > be renamed to stmmac_pcs_setup/exit. Why not, seeing they will be responsible for any PCS attached to the MAC. -Serge(y) > > Thanks, > > -- > Romain Gantois, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi Romain, On Mon, Apr 15, 2024 at 11:18 AM Romain Gantois <romain.gantois@bootlin.com> wrote: > From: Clément Léger <clement.leger@bootlin.com> > > The r9a06g032 SoC of the RZ/N1 family features two GMAC devices named > GMAC1/2, that are based on Synopsys cores. GMAC1 is connected to a > RGMII/RMII converter that is already described in this device tree. > > Signed-off-by: "Clément Léger" <clement.leger@bootlin.com> > [rgantois: commit log] > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> Thanks for your patch! > --- a/arch/arm/boot/dts/renesas/r9a06g032.dtsi > +++ b/arch/arm/boot/dts/renesas/r9a06g032.dtsi > @@ -316,6 +316,25 @@ dma1: dma-controller@40105000 { > data-width = <8>; > }; > > + gmac1: ethernet@44000000 { > + compatible = "renesas,r9a06g032-gmac", "renesas,rzn1-gmac", "snps,dwmac"; > + reg = <0x44000000 0x2000>; > + interrupt-parent = <&gic>; The surrounding "soc" node already specifies the interrupt parent, so there is no need to repeat that. I could fix that while applying to renesas-devel for v6.10, but it looks like there will be a v4 for the rest of the series anyway? The rest LGTM, so with the above fixed: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > + interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "macirq", "eth_wake_irq", "eth_lpi"; > + clocks = <&sysctrl R9A06G032_HCLK_GMAC0>; > + clock-names = "stmmaceth"; > + power-domains = <&sysctrl>; > + snps,multicast-filter-bins = <256>; > + snps,perfect-filter-entries = <128>; > + tx-fifo-depth = <2048>; > + rx-fifo-depth = <4096>; > + pcs-handle = <&mii_conv1>; > + status = "disabled"; > + }; > + > gmac2: ethernet@44002000 { > compatible = "renesas,r9a06g032-gmac", "renesas,rzn1-gmac", "snps,dwmac"; > reg = <0x44002000 0x2000>; > Gr{oetje,eeting}s, Geert
Hi Geert, On Thu, 18 Apr 2024, Geert Uytterhoeven wrote: > > + gmac1: ethernet@44000000 { > > + compatible = "renesas,r9a06g032-gmac", "renesas,rzn1-gmac", "snps,dwmac"; > > + reg = <0x44000000 0x2000>; > > + interrupt-parent = <&gic>; > > The surrounding "soc" node already specifies the interrupt parent, > so there is no need to repeat that. I could fix that while applying > to renesas-devel for v6.10, but it looks like there will be a v4 for > the rest of the series anyway? Indeed there will be a v4 so I'll fix it. Thanks,
Hi Serge, On Tue, 16 Apr 2024, Serge Semin wrote: > > +static int rzn1_dwmac_pcs_init(struct stmmac_priv *priv, > > > + struct mac_device_info *hw) > > AFAICS hw is unused, and the mac_device_info instance is reached via > the priv pointer. What about dropping the unused argument then? Unfortunately, this is an implementation of the pcs_init() callback, which is also used by socfpga (see patch 4/6 in this series). The socfpga implementations use the hw parameter for both pcs_init() and pcs_exit() so I can't remove it. Thanks,
On Thu, Apr 18, 2024 at 01:57:47PM +0200, Romain Gantois wrote: > Hi Serge, > > On Tue, 16 Apr 2024, Serge Semin wrote: > > > > +static int rzn1_dwmac_pcs_init(struct stmmac_priv *priv, > > > > > + struct mac_device_info *hw) > > > > AFAICS hw is unused, and the mac_device_info instance is reached via > > the priv pointer. What about dropping the unused argument then? > > Unfortunately, this is an implementation of the pcs_init() callback, which is > also used by socfpga (see patch 4/6 in this series). The socfpga implementations > use the hw parameter for both pcs_init() and pcs_exit() so I can't remove it. I had that patch content in mind when was writing my comment. There is no point in passing the hw-pointer there either because you already have the stmmac_priv pointer. There is stmmac_priv::hw field which you can use instead in the same way as you do in this patch. Here is the respective change for your SoCFPGA patch: +static int socfpga_dwmac_pcs_init(struct stmmac_priv *priv, + struct mac_device_info *hw) +{ ... + + priv->hw->phylink_pcs = pcs; + return 0; +} + +static void socfpga_dwmac_pcs_exit(struct stmmac_priv *priv, + struct mac_device_info *hw) +{ + if (priv->hw->phylink_pcs) + lynx_pcs_destroy(priv->hw->phylink_pcs); +} -Serge(y) > > Thanks, > > -- > Romain Gantois, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Thu, 18 Apr 2024, Serge Semin wrote: > On Thu, Apr 18, 2024 at 01:57:47PM +0200, Romain Gantois wrote: > > Hi Serge, > > > > On Tue, 16 Apr 2024, Serge Semin wrote: > > > > > > +static int rzn1_dwmac_pcs_init(struct stmmac_priv *priv, > > > > > > > + struct mac_device_info *hw) > > > > > > AFAICS hw is unused, and the mac_device_info instance is reached via > > > the priv pointer. What about dropping the unused argument then? > > > > > Unfortunately, this is an implementation of the pcs_init() callback, which is > > also used by socfpga (see patch 4/6 in this series). The socfpga implementations > > use the hw parameter for both pcs_init() and pcs_exit() so I can't remove it. > > I had that patch content in mind when was writing my comment. There is > no point in passing the hw-pointer there either because you already > have the stmmac_priv pointer. There is stmmac_priv::hw field which you > can use instead in the same way as you do in this patch. Here is the > respective change for your SoCFPGA patch: > You're right, I'll remove the parameter. Thanks,
Hello everyone, This is version three of my series that adds support for a Gigabit Ethernet controller featured in the Renesas r9a06g032 SoC, of the RZ/N1 family. This GMAC device is based on a Synopsys IP and is compatible with the stmmac driver. My former colleague Clément Léger originally sent a series for this driver, but an issue in bringing up the PCS clock had blocked the upstreaming process. This issue has since been resolved by the following series: https://lore.kernel.org/all/20240326-rxc_bugfix-v6-0-24a74e5c761f@bootlin.com/ This series consists of a devicetree binding describing the RZN1 GMAC controller IP, a node for the GMAC1 device in the r9a06g032 SoC device tree, and the GMAC driver itself which is a glue layer in stmmac. There are also two patches by Russell that improve pcs initialization handling in stmmac. Best Regards, Romain Gantois --- Changes in v3: - Fixed a typo in the socfpga patch - Link to v2: https://lore.kernel.org/r/20240409-rzn1-gmac1-v2-0-79ca45f2fc79@bootlin.com Changes in v2: - Add pcs_init/exit callbacks in stmmac to solve race condition - Use pcs_init/exit callbacks in dwmac_socfpga glue layer - Miscellaneous device tree binding corrections - Link to v1: https://lore.kernel.org/r/20240402-rzn1-gmac1-v1-0-5be2b2894d8c@bootlin.com --- Clément Léger (3): dt-bindings: net: renesas,rzn1-gmac: Document RZ/N1 GMAC support net: stmmac: add support for RZ/N1 GMAC ARM: dts: r9a06g032: describe GMAC1 Russell King (Oracle) (2): net: stmmac: introduce pcs_init/pcs_exit stmmac operations net: stmmac: dwmac-socfpga: use pcs_init/pcs_exit .../devicetree/bindings/net/renesas,rzn1-gmac.yaml | 66 +++++++++++++ MAINTAINERS | 6 ++ arch/arm/boot/dts/renesas/r9a06g032.dtsi | 19 ++++ drivers/net/ethernet/stmicro/stmmac/Kconfig | 12 +++ drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c | 88 +++++++++++++++++ .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 109 +++++++++++---------- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 +++ include/linux/stmmac.h | 2 + 9 files changed, 263 insertions(+), 54 deletions(-) --- base-commit: f1e197a665c2148ebc25fe09c53689e60afea195 change-id: 20240402-rzn1-gmac1-685cf8793d0e Best regards,