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

[Jan Petrous (OSS)]

> -----Original Message-----
> From: Russell King <linux at armlinux.org.uk>
> Sent: Monday, 5 August, 2024 17:09
> To: Jan Petrous (OSS) <jan.petrous at oss.nxp.com>
> Cc: Maxime Coquelin <mcoquelin.stm32 at gmail.com>; Alexandre Torgue
> <alexandre.torgue at foss.st.com>; dl-S32 <S32 at nxp.com>; linux-
> kernel at vger.kernel.org; linux-stm32 at st-md-mailman.stormreply.com; linux-
> arm-kernel at lists.infradead.org; Claudiu Manoil <claudiu.manoil at nxp.com>;
> netdev at 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