> -----Original Message-----
> From: Serge Semin <fancer.lancer@xxxxxxxxx>
> Sent: 02 August 2024 00:40
> To: Swathi K S <swathi.ks@xxxxxxxxxxx>; Andrew Lunn <andrew@xxxxxxx>
> Cc: krzk@xxxxxxxxxx; robh@xxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; richardcochran@xxxxxxxxx;
> mcoquelin.stm32@xxxxxxxxx; alim.akhtar@xxxxxxxxxxx; linux-
> fsd@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx;
> alexandre.torgue@xxxxxxxxxxx; peppe.cavallaro@xxxxxx;
> joabreu@xxxxxxxxxxxx; rcsekar@xxxxxxxxxxx; ssiddha@xxxxxxxxx;
> jayati.sahu@xxxxxxxxxxx; pankaj.dubey@xxxxxxxxxxx;
> ravi.patel@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx
> 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@xxxxxxxxxxx>
> > Signed-off-by: Suresh Siddha <ssiddha@xxxxxxxxx>
> > Signed-off-by: Swathi K S <swathi.ks@xxxxxxxxxxx>
> > ---
> > .../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
> >
> >
