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

Serge Semin
2024-08-01T19:10:01Z

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.

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.

-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
>
>