Re: [PATCH net-next v1 3/5] arm64: dts: s32n79: add NETC IEP18 ECAM node for ENETC PPM ports

Re: [PATCH net-next v1 3/5] arm64: dts: s32n79: add NETC IEP18 ECAM node for ENETC PPM ports

[netdev-bot+sashiko]

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

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

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).

- [High] dt-bindings: PCI node netc_iep18 uses msi-parent instead of msi-
  map breaking MSI routing
- [Low] The new top-level `nwss-bus` node, which has no unit address, is
  placed between `coss-bus` and `cpus` in…

--

arm64: dts: s32n79: add NETC IEP18 ECAM node for ENETC PPM ports

This commit adds a disabled netc_iep18 PCIe ECAM host node to
s32n79.dtsi, inside a new nwss-bus simple-bus. The node describes the NETC
integrated endpoint bus that hosts the ENETC v4 Pseudo-MAC PF and its VFs.
MSIs go through the GIC-v3 ITS, and DMA is translated by the SMMUv3.

> diff --git a/arch/arm64/boot/dts/freescale/s32n79.dtsi b/arch/arm64/boot/dts/freescale/s32n79.dtsi
> index 94ab58783fdc8..671b807f1c540 100644
> --- a/arch/arm64/boot/dts/freescale/s32n79.dtsi
> +++ b/arch/arm64/boot/dts/freescale/s32n79.dtsi
> @@ -122,6 +122,30 @@ irqsteer_coss: interrupt-controller@4ed00000 {
>  		};
>  	};
>
> +	nwss-bus {

[Severity: Low]
This isn't a bug, but is nwss-bus in the right place?

Documentation/devicetree/bindings/dts-coding-style.rst says:

  "Nodes without unit addresses shall be ordered alpha-numerically by the
  node name."

The other top-level nodes in s32n79.dtsi follow that order: cis-bus,
coss-bus, cpus, firmware, fss-bus, pmu, timer. The new node is inserted
between coss-bus and cpus.

Could it go between fss-bus and pmu instead?

> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges = <0x0 0x48800000 0x0 0x48800000 0x0 0x4b0000>,
> +			 <0x2 0x40000000 0x2 0x40000000 0x0 0x20000000>;

[ ... ]

> +	};
> +
>  	cpus {
>  		#address-cells = <1>;
>  		#size-cells = <0>;

[ ... ]

> @@ -122,6 +122,30 @@ irqsteer_coss: interrupt-controller@4ed00000 {
>  		};
>  	};
>  
> +	nwss-bus {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges = <0x0 0x48800000 0x0 0x48800000 0x0 0x4b0000>,
> +			 <0x2 0x40000000 0x2 0x40000000 0x0 0x20000000>;
> +
> +		netc_iep18: pcie@241200000 {
> +			compatible = "pci-host-ecam-generic";
> +			reg = <0x2 0x41200000 0x0 0x100000>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			device_type = "pci";
> +			bus-range = <0x0 0x0>;
> +			msi-parent = <&its 0x0>;
> +			iommu-map = <0x0 &smmu 0xc0000 0x100>;
> +			dma-coherent;
> +			status = "disabled";
> +
> +			/* All NETC PCIe functions */
> +			ranges = <0x82000000 0x0 0x48800000 0x0 0x48800000 0x0 0x4b0000>;
> +		};
> +	};
> +
>  	cpus {
>  		#address-cells = <1>;
>  		#size-cells = <0>;

[Severity: High]
Does msi-parent with a specifier work for the functions behind this
ECAM host?

For PCI devices, the ITS DeviceID and the per-device MSI domain come
from of_msi_xlate(), which walks up the parent devices trying msi-map
first and then of_check_msi_parent(). of_check_msi_parent() only
accepts an msi-parent phandle with a missing or zero #msi-cells, i.e.
the 1:1 mapping case, and returns -EINVAL when msi_spec.args_count is
non-zero, as it is for <&its 0x0>.

That leaves the MSI controller node unresolved, so
of_msi_map_get_device_domain() finds no domain for the PPM PF and its
VFs, and pci_msi_domain_get_msi_rid() passes the raw RID through
unchanged as the DeviceID that its_pci_msi_prepare() programs. The
0x0 cell is never consumed. Wouldn't MSI setup for these functions
then either fail or target a DeviceID the ITS was never configured
for?

Separately, even if the specifier were honoured, a single msi-parent
cell would collapse all 256 RIDs onto one DeviceID, which doesn't
match the iommu-map giving each RID its own StreamID from 0xc0000.

Should this be an msi-map instead, along the lines of:

  msi-map = <0x0 &its 0xc0000 0x100>;

or do the ITS DeviceIDs for IEP18 differ from the SMMU StreamIDs on
S32N79?

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

[PATCH net-next v1 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.

Rather than hand-building a software node, the driver describes the
fixed link with a self-contained DT overlay (following the lan966x PCI
approach), applied unmodified via of_overlay_fdt_apply() onto the PCI
function's dynamic OF node (CONFIG_PCI_DYNAMIC_OF_NODES). phylink then
picks up the fixed-link node through dev_fwnode(); the real operating
speed is sourced live from PCR[PSPEED].

The series touches three subsystems:
  - net/enetc:  pseudo-MAC overlay 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.

v1: initial submission.

Claudiu Manoil (5):
  net: enetc: Add pseudo-MAC support for ENETCv4 Ports via a DT overlay
  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/Kconfig  |  1 +
 drivers/net/ethernet/freescale/enetc/Makefile |  1 +
 drivers/net/ethernet/freescale/enetc/enetc.c  | 12 +++
 drivers/net/ethernet/freescale/enetc/enetc.h  |  1 +
 .../net/ethernet/freescale/enetc/enetc4_pf.c  | 98 +++++++++++++++++--
 .../freescale/enetc/enetc4_pseudo_mac.dtso    | 32 ++++++
 .../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 +
 12 files changed, 209 insertions(+), 9 deletions(-)
 create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_pseudo_mac.dtso


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.34.1

[PATCH net-next v1 3/5] arm64: dts: s32n79: add NETC IEP18 ECAM node for ENETC PPM ports

[Claudiu Manoil]

Add the netc_iep18 PCIe ECAM host node describing the NETC integrated
endpoint bus allocated to the first Linux cohort on S32N79. The bus is
enumerated via the generic pci-host-ecam-generic driver and hosts the
ENETC v4 Pseudo-MAC (PPM) PF and its VFs, which connect internally to
the on-chip NETC switch (not owned by Linux).

The node is kept disabled at the SoC level; boards that wire up NETC
enable it. MSIs are routed through the GIC-v3 ITS and DMA is translated
by the SMMUv3 (iommu-map).

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 arch/arm64/boot/dts/freescale/s32n79.dtsi | 24 +++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/s32n79.dtsi b/arch/arm64/boot/dts/freescale/s32n79.dtsi
index 94ab58783fdc..671b807f1c54 100644
--- a/arch/arm64/boot/dts/freescale/s32n79.dtsi
+++ b/arch/arm64/boot/dts/freescale/s32n79.dtsi
@@ -122,6 +122,30 @@ irqsteer_coss: interrupt-controller@4ed00000 {
 		};
 	};
 
+	nwss-bus {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges = <0x0 0x48800000 0x0 0x48800000 0x0 0x4b0000>,
+			 <0x2 0x40000000 0x2 0x40000000 0x0 0x20000000>;
+
+		netc_iep18: pcie@241200000 {
+			compatible = "pci-host-ecam-generic";
+			reg = <0x2 0x41200000 0x0 0x100000>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			bus-range = <0x0 0x0>;
+			msi-parent = <&its 0x0>;
+			iommu-map = <0x0 &smmu 0xc0000 0x100>;
+			dma-coherent;
+			status = "disabled";
+
+			/* All NETC PCIe functions */
+			ranges = <0x82000000 0x0 0x48800000 0x0 0x48800000 0x0 0x4b0000>;
+		};
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.34.1