RE: [PATCH v4 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

["Swathi K S" <swathi.ks@samsung.com>]

> -----Original Message-----
> From: Serge Semin <fancer.lancer@gmail.com>
> Sent: 02 August 2024 00:40
> To: Swathi K S <swathi.ks@samsung.com>; Andrew Lunn <andrew@lunn.ch>
> Cc: krzk@kernel.org; robh@kernel.org; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> conor+dt@kernel.org; richardcochran@gmail.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org;
> alexandre.torgue@foss.st.com; peppe.cavallaro@st.com;
> joabreu@synopsys.com; rcsekar@samsung.com; ssiddha@tesla.com;
> jayati.sahu@samsung.com; pankaj.dubey@samsung.com;
> ravi.patel@samsung.com; gost.dev@samsung.com
> Subject: Re: [PATCH v4 2/4] net: stmmac: dwc-qos: Add FSD EQoS support
> 
> Hi Swathi, Andrew
> 
> On Tue, Jul 30, 2024 at 02:46:46PM +0530, Swathi K S wrote:
> > The FSD SoC contains two instance of the Synopsys DWC ethernet QOS IP
> core.
> > The binding that it uses is slightly different from existing ones
> > because of the integration (clocks, resets).
> >
> 
> > For FSD SoC, a mux switch is needed between internal and external
clocks.
> > By default after reset internal clock is used but for receiving
> > packets properly, external clock is needed. Mux switch to external
> > clock happens only when the external clock is present.
> >
> > Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> > Signed-off-by: Suresh Siddha <ssiddha@tesla.com>
> > Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> > ---
> >  .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 90
> +++++++++++++++++++
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 28 +++++-
> >  include/linux/stmmac.h                        |  1 +
> >  3 files changed, 117 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > index ec924c6c76c6..bc97b3b573b7 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/reset.h>
> >  #include <linux/stmmac.h>
> > +#include <linux/regmap.h>
> >
> >  #include "stmmac_platform.h"
> >  #include "dwmac4.h"
> > @@ -37,6 +38,13 @@ struct tegra_eqos {
> >  	struct gpio_desc *reset;
> >  };
> >
> > +struct fsd_eqos_plat_data {
> > +	const struct fsd_eqos_variant *fsd_eqos_inst_var;
> > +	struct clk_bulk_data *clks;
> > +	int num_clks;
> > +	struct device *dev;
> > +};
> > +
> >  static int dwc_eth_dwmac_config_dt(struct platform_device *pdev,
> >  				   struct plat_stmmacenet_data *plat_dat)  {
> @@ -265,6 +273,82 @@
> > static int tegra_eqos_init(struct platform_device *pdev, void *priv)
> >  	return 0;
> >  }
> >
> > +static int dwc_eqos_rxmux_setup(void *priv, bool external) {
> > +	int i = 0;
> > +	struct fsd_eqos_plat_data *plat = priv;
> > +	struct clk *rx1 = NULL;
> > +	struct clk *rx2 = NULL;
> > +	struct clk *rx3 = NULL;
> > +
> > +	for (i = 0; i < plat->num_clks; i++) {
> > +		if (strcmp(plat->clks[i].id, "eqos_rxclk_mux") == 0)
> > +			rx1 = plat->clks[i].clk;
> > +		else if (strcmp(plat->clks[i].id, "eqos_phyrxclk") == 0)
> > +			rx2 = plat->clks[i].clk;
> > +		else if (strcmp(plat->clks[i].id, "dout_peric_rgmii_clk") ==
0)
> > +			rx3 = plat->clks[i].clk;
> > +	}
> > +
> > +	/* doesn't support RX clock mux */
> > +	if (!rx1)
> > +		return 0;
> > +
> > +	if (external)
> > +		return clk_set_parent(rx1, rx2);
> > +	else
> > +		return clk_set_parent(rx1, rx3);
> > +}
> 
> Andrew is right asking about this implementation. It does seem
> questionable:
> 
> 1. AFAIR RGMII Rx clock is supposed to be retrieved the PHY. So the
> eqos_phyrxclk and dout_peric_rgmii_clk are the PHY clocks. Do you have a
> PHY integrated in the SoC? If so you should have defined it as a separate
DT-
> node and moved the clocks definition in there.

In this case, there is no PHY integrated in the SoC.

> 
> 2. Do you really need to perform the "eqos_rxclk_mux" clock re-parenting
on
> each interface open/close? Based on the commit log you don't. So the re-
> parenting can be done in the glue driver or even in the device tree by
means
> of the "assigned-clock-parents" property.

Thanks for the insight, we investigated further and realized that this is
not mandatory. So I will remove the reparenting done in every open/ close in
the updated patchset v5.

-Swathi

> 
> -Serge(y)
> 
> > +
> > +static int fsd_clks_endisable(void *priv, bool enabled) {
> > +	struct fsd_eqos_plat_data *plat = priv;
> > +
> > +	if (enabled) {
> > +		return clk_bulk_prepare_enable(plat->num_clks, plat->clks);
> > +	} else {
> > +		clk_bulk_disable_unprepare(plat->num_clks, plat->clks);
> > +		return 0;
> > +	}
> > +}
> > +
> > +static int fsd_eqos_probe(struct platform_device *pdev,
> > +			  struct plat_stmmacenet_data *data,
> > +			  struct stmmac_resources *res)
> > +{
> > +	struct fsd_eqos_plat_data *priv_plat;
> > +	int ret = 0;
> > +
> > +	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat),
> GFP_KERNEL);
> > +	if (!priv_plat)
> > +		return -ENOMEM;
> > +
> > +	priv_plat->dev = &pdev->dev;
> > +
> > +	ret = devm_clk_bulk_get_all(&pdev->dev, &priv_plat->clks);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret, "No clocks
> available\n");
> > +
> > +	priv_plat->num_clks = ret;
> > +
> > +	data->bsp_priv = priv_plat;
> > +	data->clks_config = fsd_clks_endisable;
> > +	data->rxmux_setup = dwc_eqos_rxmux_setup;
> > +
> > +	ret = fsd_clks_endisable(priv_plat, true);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret, "Unable to enable
> fsd
> > +clock\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static void fsd_eqos_remove(struct platform_device *pdev) {
> > +	struct fsd_eqos_plat_data *priv_plat =
> > +get_stmmac_bsp_priv(&pdev->dev);
> > +
> > +	fsd_clks_endisable(priv_plat, false); }
> > +
> >  static int tegra_eqos_probe(struct platform_device *pdev,
> >  			    struct plat_stmmacenet_data *data,
> >  			    struct stmmac_resources *res)
> > @@ -411,6 +495,11 @@ static const struct dwc_eth_dwmac_data
> tegra_eqos_data = {
> >  	.remove = tegra_eqos_remove,
> >  };
> >
> > +static const struct dwc_eth_dwmac_data fsd_eqos_data = {
> > +	.probe = fsd_eqos_probe,
> > +	.remove = fsd_eqos_remove,
> > +};
> > +
> >  static int dwc_eth_dwmac_probe(struct platform_device *pdev)  {
> >  	const struct dwc_eth_dwmac_data *data; @@ -473,6 +562,7 @@
> static
> > void dwc_eth_dwmac_remove(struct platform_device *pdev)  static const
> > struct of_device_id dwc_eth_dwmac_match[] = {
> >  	{ .compatible = "snps,dwc-qos-ethernet-4.10", .data =
> &dwc_qos_data },
> >  	{ .compatible = "nvidia,tegra186-eqos", .data = &tegra_eqos_data },
> > +	{ .compatible = "tesla,fsd-ethqos", .data = &fsd_eqos_data },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, dwc_eth_dwmac_match); diff --git
> > a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 12689774d755..2ef82edec522 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -4001,6 +4001,12 @@ static int __stmmac_open(struct net_device
> *dev,
> >  	netif_tx_start_all_queues(priv->dev);
> >  	stmmac_enable_all_dma_irq(priv);
> >
> > +	if (priv->plat->rxmux_setup) {
> > +		ret = priv->plat->rxmux_setup(priv->plat->bsp_priv, true);
> > +		if (ret)
> > +			netdev_err(priv->dev, "Rxmux setup failed\n");
> > +	}
> > +
> >  	return 0;
> >
> >  irq_error:
> > @@ -4056,7 +4062,13 @@ static void stmmac_fpe_stop_wq(struct
> > stmmac_priv *priv)  static int stmmac_release(struct net_device *dev)
> > {
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> > -	u32 chan;
> > +	u32 chan, ret;
> > +
> > +	if (priv->plat->rxmux_setup) {
> > +		ret = priv->plat->rxmux_setup(priv->plat->bsp_priv, false);
> > +		if (ret)
> > +			netdev_err(priv->dev, "Rxmux setup failed\n");
> > +	}
> >
> >  	if (device_may_wakeup(priv->device))
> >  		phylink_speed_down(priv->phylink, false); @@ -7848,11
> +7860,17 @@
> > int stmmac_suspend(struct device *dev)  {
> >  	struct net_device *ndev = dev_get_drvdata(dev);
> >  	struct stmmac_priv *priv = netdev_priv(ndev);
> > -	u32 chan;
> > +	u32 chan, ret;
> >
> >  	if (!ndev || !netif_running(ndev))
> >  		return 0;
> >
> > +	if (priv->plat->rxmux_setup) {
> > +		ret = priv->plat->rxmux_setup(priv->plat->bsp_priv, false);
> > +		if (ret)
> > +			netdev_err(priv->dev, "Rxmux setup failed\n");
> > +	}
> > +
> >  	mutex_lock(&priv->lock);
> >
> >  	netif_device_detach(ndev);
> > @@ -8018,6 +8036,12 @@ int stmmac_resume(struct device *dev)
> >  	mutex_unlock(&priv->lock);
> >  	rtnl_unlock();
> >
> > +	if (priv->plat->rxmux_setup) {
> > +		ret = priv->plat->rxmux_setup(priv->plat->bsp_priv, true);
> > +		if (ret)
> > +			netdev_err(priv->dev, "Rxmux setup failed\n");
> > +	}
> > +
> >  	netif_device_attach(ndev);
> >
> >  	return 0;
> > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index
> > 84e13bd5df28..f017b818d421 100644
> > --- a/include/linux/stmmac.h
> > +++ b/include/linux/stmmac.h
> > @@ -264,6 +264,7 @@ struct plat_stmmacenet_data {
> >  	void (*ptp_clk_freq_config)(struct stmmac_priv *priv);
> >  	int (*init)(struct platform_device *pdev, void *priv);
> >  	void (*exit)(struct platform_device *pdev, void *priv);
> > +	int (*rxmux_setup)(void *priv, bool external);
> >  	struct mac_device_info *(*setup)(void *priv);
> >  	int (*clks_config)(void *priv, bool enabled);
> >  	int (*crosststamp)(ktime_t *device, struct system_counterval_t
> > *system,
> > --
> > 2.17.1
> >
> >