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

On 18/03/2024 00:26, Wadim Mueller wrote:

On Sun, Mar 17, 2024 at 03:53:19PM +0100, Krzysztof Kozlowski wrote:

quoted

On 15/03/2024 23:27, Wadim Mueller wrote:

quoted

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

quoted

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@suse.com/#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 <redacted>
---
 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. 

That's not valid reason. You only need to mention where the bindings are
for dwmac.

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

Does not matter. DTS is independent description of hardware. Do you want
to say that without driver support in Linux you could not add the DTS?
No, that's irrelevant.

should be part of one patchset.

But your statement also totally makes sense to me.

You unnecessary grow the CC list - it is already way too big (please
trim it to maintainers only and CC lists) - and make applying more
complicated, e.g. suggesting there is some dependency.

DTS *must* go via arm-soc, not net-next, combining it here increases the
risk it will go via wrong tree.


Best regards,
Krzysztof