[PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP S32G/S32R glue

[PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP S32G/S32R glue

[Jan Petrous (OSS)]

NXP S32G2xx/S32G3xx and S32R45 are automotive grade SoCs
that integrate one or two Synopsys DWMAC 5.10/5.20 IPs.

The basic driver supports only RGMII interface.

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../net/ethernet/stmicro/stmmac/dwmac-s32cc.c | 235 ++++++++++++++++++
 3 files changed, 247 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 05cc07b8f48c..31628c363d71 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -153,6 +153,17 @@ config DWMAC_RZN1
 	  This selects the Renesas RZ/N1 SoC glue layer support for
 	  the stmmac device driver. This support can make use of a custom MII
 	  converter PCS device.
+config DWMAC_S32CC
+	tristate "NXP S32G/S32R GMAC support"
+	default ARCH_S32
+	depends on OF && (ARCH_S32 || COMPILE_TEST)
+	help
+	  Support for ethernet controller on NXP S32CC SOCs.
+
+	  This selects NXP SoC glue layer support for the stmmac
+	  device driver. This driver is used for the S32CC series
+	  SOCs GMAC ethernet controller, ie. S32G2xx, S32G3xx and
+	  S32R45.
 
 config DWMAC_SOCFPGA
 	tristate "SOCFPGA dwmac support"
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index c2f0e91f6bf8..089ef3c1c45b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_DWMAC_MESON)	+= dwmac-meson.o dwmac-meson8b.o
 obj-$(CONFIG_DWMAC_QCOM_ETHQOS)	+= dwmac-qcom-ethqos.o
 obj-$(CONFIG_DWMAC_ROCKCHIP)	+= dwmac-rk.o
 obj-$(CONFIG_DWMAC_RZN1)	+= dwmac-rzn1.o
+obj-$(CONFIG_DWMAC_S32CC)	+= dwmac-s32cc.o
 obj-$(CONFIG_DWMAC_SOCFPGA)	+= dwmac-altr-socfpga.o
 obj-$(CONFIG_DWMAC_STARFIVE)	+= dwmac-starfive.o
 obj-$(CONFIG_DWMAC_STI)		+= dwmac-sti.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c
new file mode 100644
index 000000000000..2ef961efa01c
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NXP S32G/R GMAC glue layer
+ *
+ * Copyright 2019-2024 NXP
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/ethtool.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/of_address.h>
+#include <linux/phy.h>
+#include <linux/phylink.h>
+#include <linux/platform_device.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+#define GMAC_TX_RATE_125M	125000000	/* 125MHz */
+#define GMAC_TX_RATE_25M	25000000	/* 25MHz */
+#define GMAC_TX_RATE_2M5	2500000		/* 2.5MHz */
+
+/* SoC PHY interface control register */
+#define PHY_INTF_SEL_MII        0x00
+#define PHY_INTF_SEL_SGMII      0x01
+#define PHY_INTF_SEL_RGMII      0x02
+#define PHY_INTF_SEL_RMII       0x08
+
+struct s32cc_priv_data {
+	void __iomem *ioaddr;
+	void __iomem *ctrl_sts;
+	struct device *dev;
+	phy_interface_t intf_mode;
+	struct clk *tx_clk;
+	struct clk *rx_clk;
+	bool rx_clk_enabled;
+};
+
+static int s32cc_gmac_write_phy_intf_select(struct s32cc_priv_data *gmac)
+{
+	u32 intf_sel;
+
+	switch (gmac->intf_mode) {
+	case PHY_INTERFACE_MODE_SGMII:
+		intf_sel = PHY_INTF_SEL_SGMII;
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		intf_sel = PHY_INTF_SEL_RGMII;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		intf_sel = PHY_INTF_SEL_RMII;
+		break;
+	case PHY_INTERFACE_MODE_MII:
+		intf_sel = PHY_INTF_SEL_MII;
+		break;
+	default:
+		dev_err(gmac->dev, "Unsupported PHY interface: %s\n",
+			phy_modes(gmac->intf_mode));
+		return -EINVAL;
+	}
+
+	writel(intf_sel, gmac->ctrl_sts);
+
+	dev_dbg(gmac->dev, "PHY mode set to %s\n", phy_modes(gmac->intf_mode));
+
+	return 0;
+}
+
+static int s32cc_gmac_init(struct platform_device *pdev, void *priv)
+{
+	struct s32cc_priv_data *gmac = priv;
+	int ret;
+
+	ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
+	if (!ret)
+		ret = clk_prepare_enable(gmac->tx_clk);
+
+	if (ret) {
+		dev_err(&pdev->dev, "Can't set tx clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(gmac->rx_clk);
+	if (ret)
+		dev_dbg(&pdev->dev, "Can't set rx, clock source is disabled.\n");
+	else
+		gmac->rx_clk_enabled = true;
+
+	ret = s32cc_gmac_write_phy_intf_select(gmac);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't set PHY interface mode\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void s32cc_gmac_exit(struct platform_device *pdev, void *priv)
+{
+	struct s32cc_priv_data *gmac = priv;
+
+	clk_disable_unprepare(gmac->tx_clk);
+
+	clk_disable_unprepare(gmac->rx_clk);
+}
+
+static void s32cc_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
+{
+	struct s32cc_priv_data *gmac = priv;
+	int ret;
+
+	if (!gmac->rx_clk_enabled) {
+		ret = clk_prepare_enable(gmac->rx_clk);
+		if (ret) {
+			dev_err(gmac->dev, "Can't set rx clock\n");
+			return;
+		}
+		dev_dbg(gmac->dev, "rx clock enabled\n");
+		gmac->rx_clk_enabled = true;
+	}
+
+	switch (speed) {
+	case SPEED_1000:
+		dev_dbg(gmac->dev, "Set tx clock to 125M\n");
+		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
+		break;
+	case SPEED_100:
+		dev_dbg(gmac->dev, "Set tx clock to 25M\n");
+		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_25M);
+		break;
+	case SPEED_10:
+		dev_dbg(gmac->dev, "Set tx clock to 2.5M\n");
+		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_2M5);
+		break;
+	default:
+		dev_err(gmac->dev, "Unsupported/Invalid speed: %d\n", speed);
+		return;
+	}
+
+	if (ret)
+		dev_err(gmac->dev, "Can't set tx clock\n");
+}
+
+static int s32cc_dwmac_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct plat_stmmacenet_data *plat;
+	struct s32cc_priv_data *gmac;
+	struct stmmac_resources res;
+	int ret;
+
+	gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL);
+	if (!gmac)
+		return PTR_ERR(gmac);
+
+	gmac->dev = &pdev->dev;
+
+	ret = stmmac_get_platform_resources(pdev, &res);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get platform resources\n");
+
+	plat = devm_stmmac_probe_config_dt(pdev, res.mac);
+	if (IS_ERR(plat))
+		return dev_err_probe(dev, PTR_ERR(plat),
+				     "dt configuration failed\n");
+
+	/* PHY interface mode control reg */
+	gmac->ctrl_sts = devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
+	if (IS_ERR_OR_NULL(gmac->ctrl_sts))
+		return dev_err_probe(dev, PTR_ERR(gmac->ctrl_sts),
+				     "S32CC config region is missing\n");
+
+	/* tx clock */
+	gmac->tx_clk = devm_clk_get(&pdev->dev, "tx");
+	if (IS_ERR(gmac->tx_clk))
+		return dev_err_probe(dev, PTR_ERR(gmac->tx_clk),
+				     "tx clock not found\n");
+
+	/* rx clock */
+	gmac->rx_clk = devm_clk_get(&pdev->dev, "rx");
+	if (IS_ERR(gmac->rx_clk))
+		return dev_err_probe(dev, PTR_ERR(gmac->rx_clk),
+				     "rx clock not found\n");
+
+	gmac->intf_mode = plat->phy_interface;
+	gmac->ioaddr = res.addr;
+
+	/* S32CC core feature set */
+	plat->has_gmac4 = true;
+	plat->pmt = 1;
+	plat->flags |= STMMAC_FLAG_SPH_DISABLE;
+	plat->rx_fifo_size = 20480;
+	plat->tx_fifo_size = 20480;
+
+	plat->init = s32cc_gmac_init;
+	plat->exit = s32cc_gmac_exit;
+	plat->fix_mac_speed = s32cc_fix_mac_speed;
+
+	plat->bsp_priv = gmac;
+
+	return stmmac_pltfr_probe(pdev, plat, &res);
+}
+
+static const struct of_device_id s32cc_dwmac_match[] = {
+	{ .compatible = "nxp,s32g2-dwmac" },
+	{ .compatible = "nxp,s32g3-dwmac" },
+	{ .compatible = "nxp,s32r45-dwmac" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, s32cc_dwmac_match);
+
+static struct platform_driver s32cc_dwmac_driver = {
+	.probe		= s32cc_dwmac_probe,
+	.remove_new	= stmmac_pltfr_remove,
+	.driver		= {
+			    .name		= "s32cc-dwmac",
+			    .pm		= &stmmac_pltfr_pm_ops,
+			    .of_match_table = s32cc_dwmac_match,
+	},
+};
+module_platform_driver(s32cc_dwmac_driver);
+
+MODULE_AUTHOR("Jan Petrous (OSS) <jan.petrous@oss.nxp.com>");
+MODULE_DESCRIPTION("NXP S32G/R common chassis GMAC driver");
+MODULE_LICENSE("GPL");
+
-- 
2.45.2

Re: [PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP S32G/S32R glue

[Russell King (Oracle)]

On Sun, Aug 04, 2024 at 08:50:10PM +0000, Jan Petrous (OSS) wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index 05cc07b8f48c..31628c363d71 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -153,6 +153,17 @@ config DWMAC_RZN1
>  	  This selects the Renesas RZ/N1 SoC glue layer support for
>  	  the stmmac device driver. This support can make use of a custom MII
>  	  converter PCS device.

There should be a blank line here.

> +config DWMAC_S32CC
> +	tristate "NXP S32G/S32R GMAC support"
> +	default ARCH_S32
> +	depends on OF && (ARCH_S32 || COMPILE_TEST)
...

> +static void s32cc_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
> +{
> +	struct s32cc_priv_data *gmac = priv;
> +	int ret;
> +
> +	if (!gmac->rx_clk_enabled) {
> +		ret = clk_prepare_enable(gmac->rx_clk);
> +		if (ret) {
> +			dev_err(gmac->dev, "Can't set rx clock\n");
> +			return;
> +		}
> +		dev_dbg(gmac->dev, "rx clock enabled\n");
> +		gmac->rx_clk_enabled = true;
> +	}
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		dev_dbg(gmac->dev, "Set tx clock to 125M\n");
> +		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
> +		break;
> +	case SPEED_100:
> +		dev_dbg(gmac->dev, "Set tx clock to 25M\n");
> +		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_25M);
> +		break;
> +	case SPEED_10:
> +		dev_dbg(gmac->dev, "Set tx clock to 2.5M\n");
> +		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_2M5);
> +		break;
> +	default:
> +		dev_err(gmac->dev, "Unsupported/Invalid speed: %d\n", speed);
> +		return;
> +	}
> +

We seem to have multiple cases of very similar logic in lots of stmmac
platform drivers, and I think it's about time we said no more to this.
So, what I think we should do is as follows:

add the following helper - either in stmmac, or more generically
(phylib? - in which case its name will need changing.)

static long stmmac_get_rgmii_clock(int speed)
{
	switch (speed) {
	case SPEED_10:
		return 2500000;

	case SPEED_100:
		return 25000000;

	case SPEED_1000:
		return 125000000;

	default:
		return -ENVAL;
	}
}

Then, this can become:

	long tx_clk_rate;

	...

	tx_clk_rate = stmmac_get_rgmii_clock(speed);
	if (tx_clk_rate < 0) {
		dev_err(gmac->dev, "Unsupported/Invalid speed: %d\n", speed);
		return;
	}

	ret = clk_set_rate(gmac->tx_clk, tx_clk_rate);

> +	if (ret)
> +		dev_err(gmac->dev, "Can't set tx clock\n");
> +}
> +
> +static int s32cc_dwmac_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct plat_stmmacenet_data *plat;
> +	struct s32cc_priv_data *gmac;
> +	struct stmmac_resources res;
> +	int ret;
> +
> +	gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL);
> +	if (!gmac)
> +		return PTR_ERR(gmac);
> +
> +	gmac->dev = &pdev->dev;
> +
> +	ret = stmmac_get_platform_resources(pdev, &res);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get platform resources\n");
> +
> +	plat = devm_stmmac_probe_config_dt(pdev, res.mac);
> +	if (IS_ERR(plat))
> +		return dev_err_probe(dev, PTR_ERR(plat),
> +				     "dt configuration failed\n");
> +
> +	/* PHY interface mode control reg */
> +	gmac->ctrl_sts = devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
> +	if (IS_ERR_OR_NULL(gmac->ctrl_sts))
> +		return dev_err_probe(dev, PTR_ERR(gmac->ctrl_sts),
> +				     "S32CC config region is missing\n");

Testing with IS_ERR() is all that's required here.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP S32G/S32R glue

[Jan Petrous (OSS)]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Monday, 5 August, 2024 17:09
> To: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; dl-S32 <S32@nxp.com>; linux-
> kernel@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> arm-kernel@lists.infradead.org; Claudiu Manoil <claudiu.manoil@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP
> S32G/S32R glue
> 
> On Sun, Aug 04, 2024 at 08:50:10PM +0000, Jan Petrous (OSS) wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > index 05cc07b8f48c..31628c363d71 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > @@ -153,6 +153,17 @@ config DWMAC_RZN1
> >  	  This selects the Renesas RZ/N1 SoC glue layer support for
> >  	  the stmmac device driver. This support can make use of a custom
> MII
> >  	  converter PCS device.
> 
> There should be a blank line here.
> 

Added to v2. 

> > +config DWMAC_S32CC
> > +	tristate "NXP S32G/S32R GMAC support"
> > +	default ARCH_S32
> > +	depends on OF && (ARCH_S32 || COMPILE_TEST)
> ...
> 
> > +static void s32cc_fix_mac_speed(void *priv, unsigned int speed, unsigned
> int mode)
> > +{
> > +	struct s32cc_priv_data *gmac = priv;
> > +	int ret;
> > +
> > +	if (!gmac->rx_clk_enabled) {
> > +		ret = clk_prepare_enable(gmac->rx_clk);
> > +		if (ret) {
> > +			dev_err(gmac->dev, "Can't set rx clock\n");
> > +			return;
> > +		}
> > +		dev_dbg(gmac->dev, "rx clock enabled\n");
> > +		gmac->rx_clk_enabled = true;
> > +	}
> > +
> > +	switch (speed) {
> > +	case SPEED_1000:
> > +		dev_dbg(gmac->dev, "Set tx clock to 125M\n");
> > +		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
> > +		break;
> > +	case SPEED_100:
> > +		dev_dbg(gmac->dev, "Set tx clock to 25M\n");
> > +		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_25M);
> > +		break;
> > +	case SPEED_10:
> > +		dev_dbg(gmac->dev, "Set tx clock to 2.5M\n");
> > +		ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_2M5);
> > +		break;
> > +	default:
> > +		dev_err(gmac->dev, "Unsupported/Invalid speed: %d\n",
> speed);
> > +		return;
> > +	}
> > +
> 
> We seem to have multiple cases of very similar logic in lots of stmmac
> platform drivers, and I think it's about time we said no more to this.
> So, what I think we should do is as follows:
> 
> add the following helper - either in stmmac, or more generically
> (phylib? - in which case its name will need changing.)
> 
> static long stmmac_get_rgmii_clock(int speed)
> {
> 	switch (speed) {
> 	case SPEED_10:
> 		return 2500000;
> 
> 	case SPEED_100:
> 		return 25000000;
> 
> 	case SPEED_1000:
> 		return 125000000;
> 
> 	default:
> 		return -ENVAL;
> 	}
> }
> 
> Then, this can become:
> 
> 	long tx_clk_rate;
> 
> 	...
> 
> 	tx_clk_rate = stmmac_get_rgmii_clock(speed);
> 	if (tx_clk_rate < 0) {
> 		dev_err(gmac->dev, "Unsupported/Invalid speed: %d\n",
> speed);
> 		return;
> 	}
> 
> 	ret = clk_set_rate(gmac->tx_clk, tx_clk_rate);
> 

OK, added rgmi_clk(speed) helper to the linux/phy.h for general usage
In other drivers.

> > +	if (ret)
> > +		dev_err(gmac->dev, "Can't set tx clock\n");
> > +}
> > +
> > +static int s32cc_dwmac_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct plat_stmmacenet_data *plat;
> > +	struct s32cc_priv_data *gmac;
> > +	struct stmmac_resources res;
> > +	int ret;
> > +
> > +	gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL);
> > +	if (!gmac)
> > +		return PTR_ERR(gmac);
> > +
> > +	gmac->dev = &pdev->dev;
> > +
> > +	ret = stmmac_get_platform_resources(pdev, &res);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to get platform resources\n");
> > +
> > +	plat = devm_stmmac_probe_config_dt(pdev, res.mac);
> > +	if (IS_ERR(plat))
> > +		return dev_err_probe(dev, PTR_ERR(plat),
> > +				     "dt configuration failed\n");
> > +
> > +	/* PHY interface mode control reg */
> > +	gmac->ctrl_sts = devm_platform_get_and_ioremap_resource(pdev, 1,
> NULL);
> > +	if (IS_ERR_OR_NULL(gmac->ctrl_sts))
> > +		return dev_err_probe(dev, PTR_ERR(gmac->ctrl_sts),
> > +				     "S32CC config region is missing\n");
> 
> Testing with IS_ERR() is all that's required here.

Removed _OR_NULL suffix.

Thanks.
/Jan

Re: [PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP S32G/S32R glue

[Simon Horman]

On Sun, Aug 04, 2024 at 08:50:10PM +0000, Jan Petrous (OSS) wrote:
> NXP S32G2xx/S32G3xx and S32R45 are automotive grade SoCs
> that integrate one or two Synopsys DWMAC 5.10/5.20 IPs.
> 
> The basic driver supports only RGMII interface.
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c

...

> +static int s32cc_gmac_init(struct platform_device *pdev, void *priv)
> +{
> +	struct s32cc_priv_data *gmac = priv;
> +	int ret;
> +
> +	ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
> +	if (!ret)
> +		ret = clk_prepare_enable(gmac->tx_clk);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't set tx clock\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(gmac->rx_clk);
> +	if (ret)
> +		dev_dbg(&pdev->dev, "Can't set rx, clock source is disabled.\n");
> +	else
> +		gmac->rx_clk_enabled = true;
> +
> +	ret = s32cc_gmac_write_phy_intf_select(gmac);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't set PHY interface mode\n");

Should operations on tx_clk and rx_clk be unwound here?

Flagged by Smatch.

> +		return ret;
> +	}
> +
> +	return 0;
> +}

...

> +static int s32cc_dwmac_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct plat_stmmacenet_data *plat;
> +	struct s32cc_priv_data *gmac;
> +	struct stmmac_resources res;
> +	int ret;

Please consider arranging local variables in Networking code
in reverse xmas tree order - longest line to shortest.

Flagged by: https://github.com/ecree-solarflare/xmastree

> +
> +	gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL);
> +	if (!gmac)
> +		return PTR_ERR(gmac);

This will return 0, perhaps return -ENOMEM ?

Flagged by Smatch.

...

RE: [PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP S32G/S32R glue

[Jan Petrous (OSS)]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Monday, 5 August, 2024 19:00
> To: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; dl-S32 <S32@nxp.com>; linux-
> kernel@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> arm-kernel@lists.infradead.org; Claudiu Manoil <claudiu.manoil@nxp.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP
> S32G/S32R glue
> 
> On Sun, Aug 04, 2024 at 08:50:10PM +0000, Jan Petrous (OSS) wrote:
> > NXP S32G2xx/S32G3xx and S32R45 are automotive grade SoCs
> > that integrate one or two Synopsys DWMAC 5.10/5.20 IPs.
> >
> > The basic driver supports only RGMII interface.
> >
> > Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c
> 
> ...
> 
> > +static int s32cc_gmac_init(struct platform_device *pdev, void *priv)
> > +{
> > +	struct s32cc_priv_data *gmac = priv;
> > +	int ret;
> > +
> > +	ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
> > +	if (!ret)
> > +		ret = clk_prepare_enable(gmac->tx_clk);
> > +
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Can't set tx clock\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(gmac->rx_clk);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "Can't set rx, clock source is
> disabled.\n");
> > +	else
> > +		gmac->rx_clk_enabled = true;
> > +
> > +	ret = s32cc_gmac_write_phy_intf_select(gmac);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Can't set PHY interface mode\n");
> 
> Should operations on tx_clk and rx_clk be unwound here?
> 
> Flagged by Smatch.

Yes, it shall be there. Added to v2.

> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int s32cc_dwmac_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct plat_stmmacenet_data *plat;
> > +	struct s32cc_priv_data *gmac;
> > +	struct stmmac_resources res;
> > +	int ret;
> 
> Please consider arranging local variables in Networking code
> in reverse xmas tree order - longest line to shortest.
> 
> Flagged by: https://github.com/ecree-solarflare/xmastree
> 

Done for v2.

> > +
> > +	gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL);
> > +	if (!gmac)
> > +		return PTR_ERR(gmac);
> 
> This will return 0, perhaps return -ENOMEM ?
> 
> Flagged by Smatch.

Changed to -ENOMEM.

Thanks for review.
/Jan