Date: Mon, 18 Mar 2024 21:50:17 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Wadim Mueller <wafgo01@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	NXP Linux Team <linux-imx@....com>,
	Chester Lin <chester62515@...il.com>,
	Andreas Färber <afaerber@...e.de>,
	Matthias Brugger <mbrugger@...e.com>,
	NXP S32 Linux Team <s32@....com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>,
	Richard Cochran <richardcochran@...il.com>,
	Andrew Halaney <ahalaney@...hat.com>,
	Simon Horman <horms@...nel.org>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	Johannes Zink <j.zink@...gutronix.de>,
	Shenwei Wang <shenwei.wang@....com>,
	"Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
	Swee Leong Ching <leong.ching.swee@...el.com>,
	Giuseppe Cavallaro <peppe.cavallaro@...com>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com, linux-clk@...r.kernel.org
Subject: Re: [PATCH 2/3] net: stmmac: Add NXP S32 SoC family support

On Fri, Mar 15, 2024 at 11:27:48PM +0100, Wadim Mueller wrote:
> Add support for NXP S32 SoC family's GMAC to the stmmac network driver. This driver implementation is based on the patchset originally contributed by Chester Lin [1], which itself draws heavily from NXP's downstream implementation [2]. The patchset was never merged.

Please wrap you commit message.

 
> +#include <linux/device.h>
> +#include <linux/ethtool.h>

Is this one needed?

> +static int s32_gmac_init(struct platform_device *pdev, void *priv)
> +{
> +	struct s32_priv_data *gmac = priv;
> +	u32 intf_sel;
> +	int ret;
> +
> +	if (gmac->tx_clk) {
> +		ret = clk_prepare_enable(gmac->tx_clk);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Can't set tx clock\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (gmac->rx_clk) {
> +		ret = clk_prepare_enable(gmac->rx_clk);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Can't set rx clock\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* set interface mode */
> +	if (gmac->ctrl_sts) {
> +		switch (gmac->intf_mode) {
> +		default:
> +			dev_info(
> +				&pdev->dev,
> +				"unsupported mode %u, set the default phy mode.\n",
> +				gmac->intf_mode);
> +			fallthrough;

I would actually return -EINVAL. There is no backwards compatibility
needed here, so force that the mode is always specified.

> +		case PHY_INTERFACE_MODE_SGMII:
> +			dev_info(&pdev->dev, "phy mode set to SGMII\n");

Please don't spam the kernel log. dev_dbg(). 

> +static void s32_fix_speed(void *priv, unsigned int speed, unsigned int mode)
> +{
> +	struct s32_priv_data *gmac = priv;
> +
> +	if (!gmac->tx_clk || !gmac->rx_clk)
> +		return;
> +
> +	/* SGMII mode doesn't support the clock reconfiguration */
> +	if (gmac->intf_mode == PHY_INTERFACE_MODE_SGMII)
> +		return;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		dev_info(gmac->dev, "Set TX clock to 125M\n");

more dev_dbg(). A driver should generally be silent, unless something
goes wrong. It is also questionable if dev_dbg() should be used. Once
the driver actually works, you can throw away a lot of debug
prints. Do you expect problems here in the future?

> +static int s32_config_cache_coherency(struct platform_device *pdev,
> +				      struct plat_stmmacenet_data *plat_dat)
> +{
> +	plat_dat->axi4_ace_ctrl = devm_kzalloc(
> +		&pdev->dev, sizeof(struct stmmac_axi4_ace_ctrl), GFP_KERNEL);
> +
> +	if (!plat_dat->axi4_ace_ctrl)
> +		return -ENOMEM;
> +
> +	plat_dat->axi4_ace_ctrl->tx_ar_reg = (ACE_CONTROL_SIGNALS << 16) |
> +					     (ACE_CONTROL_SIGNALS << 8) |
> +					     ACE_CONTROL_SIGNALS;
> +
> +	plat_dat->axi4_ace_ctrl->rx_aw_reg =
> +		(ACE_CONTROL_SIGNALS << 24) | (ACE_CONTROL_SIGNALS << 16) |
> +		(ACE_CONTROL_SIGNALS << 8) | ACE_CONTROL_SIGNALS;
> +
> +	plat_dat->axi4_ace_ctrl->txrx_awar_reg =
> +		(ACE_PROTECTION << 20) | (ACE_PROTECTION << 16) |
> +		(ACE_CONTROL_SIGNALS << 8) | ACE_CONTROL_SIGNALS;

This looks like magic. Can the various shifts be replaced my #defines?
Comments added? This makes changes in some of the core code. So it
might be better to have a prerequisite patch adding cache coherency
control, with a good commit message explaining it.

> +
> +	return 0;
> +}
> +
> +static int s32_dwmac_probe(struct platform_device *pdev)
> +{
> +	struct plat_stmmacenet_data *plat_dat;
> +	struct stmmac_resources stmmac_res;
> +	struct s32_priv_data *gmac;
> +	struct resource *res;
> +	const char *tx_clk, *rx_clk;
> +	int ret;
> +
> +	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> +	if (ret)
> +		return ret;
> +
> +	gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL);
> +	if (!gmac)
> +		return PTR_ERR(gmac);
> +
> +	gmac->dev = &pdev->dev;
> +
> +	/* S32G control reg */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	gmac->ctrl_sts = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR_OR_NULL(gmac->ctrl_sts)) {
> +		dev_err(&pdev->dev, "S32G config region is missing\n");
> +		return PTR_ERR(gmac->ctrl_sts);
> +	}
> +
> +	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> +	if (IS_ERR(plat_dat))
> +		return PTR_ERR(plat_dat);
> +
> +	plat_dat->bsp_priv = gmac;
> +
> +	switch (plat_dat->phy_interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +		tx_clk = "tx_sgmii";
> +		rx_clk = "rx_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:
> +		tx_clk = "tx_rgmii";
> +		rx_clk = "rx_rgmii";
> +		break;
> +	case PHY_INTERFACE_MODE_RMII:
> +		tx_clk = "tx_rmii";
> +		rx_clk = "rx_rmii";
> +		break;
> +	case PHY_INTERFACE_MODE_MII:
> +		tx_clk = "tx_mii";
> +		rx_clk = "rx_mii";
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "Not supported phy interface mode: [%s]\n",
> +			phy_modes(plat_dat->phy_interface));
> +		return -EINVAL;
> +	};
> +
> +	gmac->intf_mode = plat_dat->phy_interface;
> +
> +	/* DMA cache coherency settings */
> +	if (of_dma_is_coherent(pdev->dev.of_node)) {
> +		ret = s32_config_cache_coherency(pdev, plat_dat);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* tx clock */
> +	gmac->tx_clk = devm_clk_get(&pdev->dev, tx_clk);
> +	if (IS_ERR(gmac->tx_clk)) {
> +		dev_info(&pdev->dev, "tx clock not found\n");
> +		gmac->tx_clk = NULL;

Is the clock really optional?

I would also print the name of the clock which is missing.

> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -324,6 +324,10 @@ static void stmmac_clk_csr_set(struct stmmac_priv *priv)
>  			priv->clk_csr = STMMAC_CSR_150_250M;
>  		else if ((clk_rate >= CSR_F_250M) && (clk_rate <= CSR_F_300M))
>  			priv->clk_csr = STMMAC_CSR_250_300M;
> +		else if ((clk_rate >= CSR_F_300M) && (clk_rate < CSR_F_500M))
> +			priv->clk_csr = STMMAC_CSR_300_500M;
> +		else if ((clk_rate >= CSR_F_500M) && (clk_rate < CSR_F_800M))
> +			priv->clk_csr = STMMAC_CSR_500_800M;

Also seems like something which could be a patch of its own. Ideally
you want lots of small patches which are obviously correct. Part of
being obviously correct is the commit message, which is easier to
write when the patch is small and only does one thing.

      Andrew

Powered by blists - more mailing lists