Re: [PATCH 2/3] net: stmmac: Add NXP S32 SoC family support

[Wadim Mueller]

On Sun, Mar 17, 2024 at 03:53:19PM +0100, Krzysztof Kozlowski wrote:
> On 15/03/2024 23:27, 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.
> > 
> > The S32G2/3 SoCs feature multiple Ethernet interfaces (PFE0, PFE1, PFE2, and GMAC) which can be routed through a SerDes Subsystem, supporting various interfaces such as SGMII and RGMII. However, the current Glue Code lacks support for SerDes routing and pinctrl handling, relying solely on correct settings in U-Boot. Clock settings for this SoC are managed by the ATF Firmware.
> 
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
> 
> Read how commit msg should be wrapped.
> 
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> 
> > 
> > Changes made compared to [1]:
> > 
> >     Rebased onto Linux 6.8-rc7
> >     Consolidated into a single commit
> >     Minor adjustments in naming and usage of dev_err()/dev_info()
> > 
> > Test Environment:
> > The driver has been successfully tested on the official S32G-VNP-RDB3 Reference Design Board from NXP, utilizing an S32G3 SoC. The firmware and U-Boot used were from the BSP39 Release. The official BSP39 Ubuntu 22.04 Release was successfully booted. A network stress test using iperf [3] was also executed without issues.
> > 
> > [1] https://patchwork.kernel.org/project/netdevbpf/patch/20221031101052.14956-6-clin@xxxxxxxx/#25068228
> > [2] https://github.com/nxp-auto-linux/linux/blob/release/bsp39.0-5.15.129-rt/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c
> > [3] https://linux.die.net/man/1/iperf
> > [4] https://github.com/nxp-auto-linux/u-boot
> > [5] https://github.com/nxp-auto-linux/arm-trusted-firmware
> > 
> > Signed-off-by: Wadim Mueller <wafgo01@xxxxxxxxx>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/Kconfig   |  12 +
> >  drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
> 
> That's totally unrelated to DTS. Do not mix independent work in one
> patchset. This targets net-next, not SoC, so please send it as separate
> patchset when net-next reopens, so after merge window.
>

Let me try to explain why I was thinking that both should be part of the
same patchset. 

The DTS file patch [1/3] is refering to a NIC for which there is no
upstream driver (or lets call it better glue code for the real driver) available. 

This patch here is supposed to add support for this driver. So without this part the DT
node named "gmac0" of [1/3] is not of much use. Thats why I was thinking they
should be part of one patchset.

But your statement also totally makes sense to me.

Thanks for the feedback!

> >  drivers/net/ethernet/stmicro/stmmac/common.h  |   3 +
> >  .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 313 ++++++++++++++++++
> >  .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |   9 +
> >  .../net/ethernet/stmicro/stmmac/dwmac4_dma.h  |   3 +
> >  drivers/net/ethernet/stmicro/stmmac/hwif.h    |   5 +
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c |   7 +
> >  include/linux/stmmac.h                        |   9 +
> >  9 files changed, 362 insertions(+)
> >  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > index 85dcda51df05..1cdf2da0251c 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > @@ -142,6 +142,18 @@ config DWMAC_ROCKCHIP
> >  	  This selects the Rockchip RK3288 SoC glue layer support for
> >  	  the stmmac device drive
> 
> 
> ...
> 
> > +
> > +	plat_dat->safety_feat_cfg->tsoee = 1;
> > +	plat_dat->safety_feat_cfg->mrxpee = 1;
> > +	plat_dat->safety_feat_cfg->mestee = 1;
> > +	plat_dat->safety_feat_cfg->mrxee = 1;
> > +	plat_dat->safety_feat_cfg->mtxee = 1;
> > +	plat_dat->safety_feat_cfg->epsi = 1;
> > +	plat_dat->safety_feat_cfg->edpp = 1;
> > +	plat_dat->safety_feat_cfg->prtyen = 1;
> > +	plat_dat->safety_feat_cfg->tmouten = 1;
> > +
> > +	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > +	if (ret)
> > +		goto err_gmac_exit;
> > +
> > +	return 0;
> > +
> > +err_gmac_exit:
> > +	s32_gmac_exit(pdev, plat_dat->bsp_priv);
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id s32_dwmac_match[] = {
> > +	{ .compatible = "nxp,s32-dwmac" },
> 
> 
> Missing bindings.
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
> 
> > +	{}
> > +};
> 
> 
> 
> Best regards,
> Krzysztof
>