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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!