Re: [PATCH net-next v2 4/5] arm64: dts: s32n79-rdb: enable NETC IEP18

Re: [PATCH net-next v2 4/5] arm64: dts: s32n79-rdb: enable NETC IEP18

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] s32n79-rdb.dts enables &netc_iep18 but leaves &smmu disabled,
  even though the bus node says `iommu-map = <0x0 &smmu 0xc0000 0x100>`.
- [Low] At this commit, enabling &netc_iep18 in s32n79-rdb.dts does not
  get the PPM PF to probe.
- [Low] The &netc_iep18 override is appended after &usdhc0 in
  s32n79-rdb.dts.

--

arm64: dts: s32n79-rdb: enable NETC IEP18

This patch enables the NETC v4 ECAM bus #18 (netc_iep18) on the S32N79-RDB
board. The goal is to get the ENETC v4 Pseudo-MAC port (PPM) PF and VF
instances probed.

> diff --git a/arch/arm64/boot/dts/freescale/s32n79-rdb.dts b/arch/arm64/boot/dts/freescale/s32n79-rdb.dts
> index 1feccd61258ef..897ba24ac03ae 100644
> --- a/arch/arm64/boot/dts/freescale/s32n79-rdb.dts
> +++ b/arch/arm64/boot/dts/freescale/s32n79-rdb.dts
> @@ -68,3 +68,7 @@ &usdhc0 {
>  	no-sdio;
>  	status = "okay";
>  };
> +
> +&netc_iep18 {
> +	status = "okay";
> +};

[Severity: Medium]
Should &smmu also be enabled for this board? The netc_iep18 node in
s32n79.dtsi has:

	iommu-map = <0x0 &smmu 0xc0000 0x100>;

The SMMU node is disabled at SoC level, and s32n79-rdb.dts does not
override it:

	smmu: iommu@4fc00000 {
		...
		#iommu-cells = <1>;
		dma-coherent;
		status = "disabled";
	};

DMA setup for the ENETC PF/VFs on this bus goes through
of_dma_configure_id()->of_iommu_configure()->of_map_iommu_id()->
of_iommu_xlate(). of_iommu_xlate() rejects the disabled node:

	if (!of_device_is_available(iommu_spec->np))
		return -ENODEV;

of_dma_configure_id() then falls back to direct DMA and only logs a
dev_dbg() message:

	/* Take all other IOMMU errors to mean we'll just carry on without it */
	dev_dbg(dev, "device is%sbehind an iommu\n",
		!ret ? " " : " not ");

	arch_setup_dma_ops(dev, coherent);

Does this leave every NETC function, including the SR-IOV VFs, doing
untranslated DMA with no iommu_group? If so, the VFs cannot be assigned
through VFIO. Whether DMA works at all would then depend on whether firmware
left the SMMU in bypass or abort mode.

This also seems to contradict the earlier dtsi commit "arm64: dts: s32n79:
add NETC IEP18 ECAM node for ENETC PPM ports", which says:

  DMA is translated by the SMMUv3 (iommu-map)

The last commit in the series does not change this.

[Severity: Low]
Will the PPM PF actually probe at this point in the series? The commit
message says:

  so that the ENETC v4 Pseudo-MAC port (PPM) PF and VF instances are probed.

The ENETC PFs are endpoints on root bus 0 of a pci-host-ecam-generic node
that has no child nodes, so pci_set_of_node() gives them no of_node.

At this commit, pci_bus_add_device() only creates dynamic nodes for
bridges:

	pci_fixup_device(pci_fixup_final, dev);
	if (pci_is_bridge(dev))
		of_pci_make_dev_node(dev);

drivers/pci/quirks.c has no NXP ENETC fixup yet, so
enetc4_link_init()->enetc4_add_ppm_fixed_link() would fail here:

	if (!np)
		return dev_err_probe(priv->dev, -ENODEV,
				     "Missing of_node for Pseudo-MAC port\n");

The last patch in the series, "PCI: Create OF node for NXP ENETC
pseudo-MAC functions", fixes this by adding:

	DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_PHILIPS, 0xe101, of_pci_make_dev_node);
	DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_PHILIPS, 0xe110, of_pci_make_dev_node);

Without that patch applied first, there is a bisection window where the PF
fails to probe.

One gap remains at the end of the series. CONFIG_PCI_DYNAMIC_OF_NODES
(bool, default n) is not set in arch/arm64/configs/defconfig, which has:

	CONFIG_NXP_ENETC4=m
	CONFIG_FSL_ENETC_VF=m

With defconfig, the PF would still fail with -ENODEV. The commit "net:
enetc: Add pseudo-MAC support for ENETCv4 Ports via a fixed-link node"
documents this as an intended runtime dependency rather than a Kconfig
dependency.

[Severity: Low]
This isn't a bug, but should this override go between &irqsteer_coss and
&uart0? Documentation/devicetree/bindings/dts-coding-style.rst says:

  3. When extending nodes in the board DTS via &label, the entries shall be
     ordered either alpha-numerically or by keeping the order from DTSI, where
     the choice depends on the subarchitecture.

The existing overrides in s32n79-rdb.dts are in alphabetical order:
irqsteer_coss, uart0, uart5, uart6, uart7, usdhc0.

Placing &netc_iep18 after &usdhc0 matches neither alphabetical order nor
DTSI order, since netc_iep18 also comes before usdhc0 in s32n79.dtsi.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791908694.git.claudiu.manoil%40nxp.com

[PATCH net-next v2 0/5] net: enetc: S32N79 pseudo-MAC (PPM) networking support

[Claudiu Manoil]

This series adds networking support for the ENETCv4.4 pseudo-MAC (PPM)
ports found on the NXP S32N79 SoC. On S32N79, the ENETCv4 ports are
not wired to any external PHY/MII; instead they are internal "pseudo-MAC"
links into the on-chip NETC switch. These links are proprietary, expose
no standard IEEE interface, and run at a speed fixed at boot time by the
Port PCR[PSPEED] register.

A key use case is a port that has no "ethernet" DT node at all: NETC is
reached over PCI from a host that probes the ENETC, while the internal
NETC switch it connects to is not owned by Linux. Such a port must have
its fixed link synthesized at probe time.

The driver synthesizes the fixed link directly in C using an of_changeset:
of_changeset_create_node() adds a "fixed-link" subnode onto the PCI
function's dynamic OF node (CONFIG_PCI_DYNAMIC_OF_NODES), with the "speed"
property read from PCR[PSPEED] at probe time and "full-duplex" set.
phylink then picks up the fixed-link node through dev_fwnode().

The series touches three subsystems:
  - net/enetc:  pseudo-MAC fixed-link support + probing of 4.4 devices
  - arm64 dts:  NETC IEP18 ECAM node on S32N79 and its RDB board enable
  - PCI:        a quirk creating the OF node for the ENETC PMAC functions

Because this is primarily a networking feature and the pieces are
interdependent at runtime, it would be most convenient to merge the
whole series through the netdev tree.

Patch 1 has a runtime (not Kconfig) dependency on
CONFIG_PCI_DYNAMIC_OF_NODES; absent that, the pseudo-MAC probe path
fails gracefully with -ENODEV.

v2:
 - Switched patch 1 from a DT overlay blob (of_overlay_fdt_apply() + a
   .dtso compiled to .dtbo) to of_changeset_create_node(), per review
   feedback. This drops the embedded overlay externs, the Makefile
   .dtbo wrapping, and the OF_OVERLAY Kconfig dependency. The operating
   speed is now baked directly into the changeset-created fixed-link
   node (read once from PCR[PSPEED] at probe time); the phylink
   get_fixed_state callback is no longer needed and was removed.
 - Patch 2: described why there are two ENETC PF IDs in the commit
   message, as per review.
 - Patch 3: replaced the 'msi-parent' stub property with 'msi-map', as
   per H/W spec.
 - Patch 5: reworded the commit message to match patch 1's fixed-link
   approach (no functional change); added Bjorn's Acked-by.
v1: initial submission.

Claudiu Manoil (5):
  net: enetc: Add pseudo-MAC support for ENETCv4 Ports via a fixed-link
    node
  net: enetc: Enable probing of version 4.4 devices
  arm64: dts: s32n79: add NETC IEP18 ECAM node for ENETC PPM ports
  arm64: dts: s32n79-rdb: enable NETC IEP18
  PCI: Create OF node for NXP ENETC pseudo-MAC functions

 arch/arm64/boot/dts/freescale/s32n79-rdb.dts  |   4 +
 arch/arm64/boot/dts/freescale/s32n79.dtsi     |  24 ++++
 drivers/net/ethernet/freescale/enetc/enetc.c  |  12 ++
 drivers/net/ethernet/freescale/enetc/enetc.h  |   1 +
 .../net/ethernet/freescale/enetc/enetc4_pf.c  | 113 +++++++++++++++++-
 .../net/ethernet/freescale/enetc/enetc_hw.h   |   1 +
 .../freescale/enetc/enetc_pf_common.c         |  41 ++++++-
 .../freescale/enetc/enetc_pf_common.h         |   1 +
 drivers/pci/quirks.c                          |   2 +
 9 files changed, 190 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH net-next v2 4/5] arm64: dts: s32n79-rdb: enable NETC IEP18

[Claudiu Manoil]

Enable the NETC v4 ECAM bus #18 on the S32N79-RDB board so that the
ENETC v4 Pseudo-MAC port (PPM) PF and VF instances are probed.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
v2: none

 arch/arm64/boot/dts/freescale/s32n79-rdb.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/s32n79-rdb.dts b/arch/arm64/boot/dts/freescale/s32n79-rdb.dts
index 1feccd61258e..897ba24ac03a 100644
--- a/arch/arm64/boot/dts/freescale/s32n79-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/s32n79-rdb.dts
@@ -68,3 +68,7 @@ &usdhc0 {
 	no-sdio;
 	status = "okay";
 };
+
+&netc_iep18 {
+	status = "okay";
+};
-- 
2.34.1