[PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node

[PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node

[Matthias Schiffer]

While the current Device Trees for TI EVMs configure the PRUSS Ethernet
controller as a toplevel node with names like "icssg1-eth", allowing to
make it a subnode of the ICSSG has a number of advantages:

- It makes sense semantically - the Ethernet controller is running on
  the ICSSG/PRUSS
- Disabling or deleting the ICSSG node implicitly removes the Ethernet
  controller node when it is a child node. This can be relevant on SoCs
  like the AM64x which come in variants with and without ICSSG; e.g., on
  the TQMa64xxL the ICSSG node will be disabled on variants without as a
  bootloader fixup.
  On Linux, this avoids leaving the Ethernet controller in deferred
  state forever while waiting for the ICSSG to become available
  (resulting in a warning on newer kernels)

The node name "ethernet" is chosen as it nicely matches the regular
"ethernet@<reg>" format of many Ethernet controller nodes, and is also
what the prueth binding example (/schemas/net/ti,icssg-prueth.yaml) uses.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
index c402cb2928e89..89dfcf5ce8434 100644
--- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
+++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
@@ -92,6 +92,13 @@ properties:
     description: |
       This property is as per sci-pm-domain.txt.
 
+  ethernet:
+    description: |
+      ICSSG PRUSS Ethernet. Configuration for an Ethernet controller running
+      on the PRU-ICSS.
+    $ref: /schemas/net/ti,icssg-prueth.yaml#
+    type: object
+
 patternProperties:
 
   memories@[a-f0-9]+$:
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 2/2] arm64: dts: ti: k3-am642-tqma64xxl-mbax4xxl: add PRU Ethernet support

[Matthias Schiffer]

Add PRU Ethernet controller and PHY nodes, as it was previously done for
the AM64x EVM Device Trees.

Differing from the EVM, we add the virtual ethernet controller device
below &icssg1 instead of the top level. Besides being slighly more
accurate, this has the advantage that the node is implicitly disabled
when &icssg1 has status = "disabled" (which the TQMa64xxL bootloader
adds as a fixup when running on an AM64x variant without ICSSG support),
thus avoiding leaving the ethernet device in EPROBE_DEFER limbo forever.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

k3-am642-evm.dts uses "ti-pruss/am64x-sr2-*" filenames instead of
"ti-pruss/am65x-sr2-*", however it is not clear to me where these would
come from - I'm not aware of any firmwares named like that.

So far, these firmwares are not in mainline linux-firmware; TI's
reference BSPs include firmware from ti-linux-firmware [1], and the same
"am65x-sr2" firmwares are used on AM65x and AM64x SoCs.

[1] https://git.ti.com/gitweb?p=processor-firmware/ti-linux-firmware.git;a=tree;f=ti-pruss;h=a220bdc6dce5e11845b5c6337ff9b2d329aee196;hb=refs/heads/ti-linux-firmware

 .../dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts    | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts b/arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts
index 1f4dc5ad1696a..0eff392a29b00 100644
--- a/arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts
+++ b/arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts
@@ -24,6 +24,8 @@ / {
 
 	aliases {
 		ethernet0 = &cpsw_port1;
+		ethernet1 = &icssg1_emac0;
+		ethernet2 = &icssg1_emac1;
 		i2c1 = &mcu_i2c0;
 		mmc1 = &sdhci1;
 		serial0 = &mcu_uart0;
@@ -154,6 +156,104 @@ &epwm5 {
 	status = "okay";
 };
 
+&icssg1 {
+	icssg1_eth: ethernet {
+		compatible = "ti,am642-icssg-prueth";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pru_icssg1_rgmii1_pins>, <&pru_icssg1_rgmii2_pins>;
+		interrupt-parent = <&icssg1_intc>;
+		interrupts = <24 0 2>, <25 1 3>;
+		interrupt-names = "tx_ts0", "tx_ts1";
+		dmas = <&main_pktdma 0xc200 15>, /* egress slice 0 */
+		       <&main_pktdma 0xc201 15>, /* egress slice 0 */
+		       <&main_pktdma 0xc202 15>, /* egress slice 0 */
+		       <&main_pktdma 0xc203 15>, /* egress slice 0 */
+		       <&main_pktdma 0xc204 15>, /* egress slice 1 */
+		       <&main_pktdma 0xc205 15>, /* egress slice 1 */
+		       <&main_pktdma 0xc206 15>, /* egress slice 1 */
+		       <&main_pktdma 0xc207 15>, /* egress slice 1 */
+		       <&main_pktdma 0x4200 15>, /* ingress slice 0 */
+		       <&main_pktdma 0x4201 15>; /* ingress slice 1 */
+		dma-names = "tx0-0", "tx0-1", "tx0-2", "tx0-3",
+			    "tx1-0", "tx1-1", "tx1-2", "tx1-3",
+			    "rx0", "rx1";
+		sram = <&oc_sram>;
+		firmware-name = "ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
+				"ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
+				"ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
+				"ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
+				"ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
+				"ti-pruss/am65x-sr2-txpru1-prueth-fw.elf";
+		ti,prus = <&pru1_0>, <&rtu1_0>, <&tx_pru1_0>, <&pru1_1>, <&rtu1_1>, <&tx_pru1_1>;
+		ti,pruss-gp-mux-sel = <2>,	/* MII mode */
+				      <2>,
+				      <2>,
+				      <2>,	/* MII mode */
+				      <2>,
+				      <2>;
+		ti,mii-g-rt = <&icssg1_mii_g_rt>;
+		ti,mii-rt = <&icssg1_mii_rt>;
+		ti,iep = <&icssg1_iep0>,  <&icssg1_iep1>;
+
+		ethernet-ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			icssg1_emac0: port@0 {
+				reg = <0>;
+				phy-handle = <&icssg1_phy0c>;
+				phy-mode = "rgmii-id";
+				/* Filled in by bootloader */
+				local-mac-address = [00 00 00 00 00 00];
+			};
+
+			icssg1_emac1: port@1 {
+				reg = <1>;
+				phy-handle = <&icssg1_phy03>;
+				phy-mode = "rgmii-id";
+				/* Filled in by bootloader */
+				local-mac-address = [00 00 00 00 00 00];
+			};
+		};
+	};
+};
+
+&icssg1_mdio {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pru_icssg1_mdio_pins>;
+	status = "okay";
+
+	/* phy-mode is fixed up to rgmii-rxid by prueth driver to account for
+	 * the SoC integration, so the only rx-internal-delay and no
+	 * tx-internal-delay is set for the PHYs.
+	 */
+
+	icssg1_phy03: ethernet-phy@3 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <0x3>;
+		reset-gpios = <&main_gpio1 47 GPIO_ACTIVE_LOW>;
+		reset-assert-us = <1000>;
+		reset-deassert-us = <1000>;
+		ti,rx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
+		ti,tx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
+		ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
+		ti,clk-output-sel = <DP83867_CLK_O_SEL_OFF>;
+	};
+
+	icssg1_phy0c: ethernet-phy@c {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <0xc>;
+		reset-gpios = <&main_gpio1 51 GPIO_ACTIVE_LOW>;
+		reset-assert-us = <1000>;
+		reset-deassert-us = <1000>;
+		ti,rx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
+		ti,tx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
+		ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
+		ti,clk-output-sel = <DP83867_CLK_O_SEL_OFF>;
+	};
+};
+
+
 &main_gpio0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&main_gpio0_digital_pins>,
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 2/2] arm64: dts: ti: k3-am642-tqma64xxl-mbax4xxl: add PRU Ethernet support

[MD Danish Anwar]

Hi Matthias,

On 19/06/24 4:54 pm, Matthias Schiffer wrote:
> Add PRU Ethernet controller and PHY nodes, as it was previously done for
> the AM64x EVM Device Trees.
> 
> Differing from the EVM, we add the virtual ethernet controller device
> below &icssg1 instead of the top level. Besides being slighly more
> accurate, this has the advantage that the node is implicitly disabled
> when &icssg1 has status = "disabled" (which the TQMa64xxL bootloader
> adds as a fixup when running on an AM64x variant without ICSSG support),
> thus avoiding leaving the ethernet device in EPROBE_DEFER limbo forever.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
> 
> k3-am642-evm.dts uses "ti-pruss/am64x-sr2-*" filenames instead of
> "ti-pruss/am65x-sr2-*", however it is not clear to me where these would
> come from - I'm not aware of any firmwares named like that.
> 

During upstreaming of icssg1-eth node in "k3-am642-evm.dts", it was
raised that the firmware name should be am64x-sr2* instead of am65x-sr2* [1]

> So far, these firmwares are not in mainline linux-firmware; TI's
> reference BSPs include firmware from ti-linux-firmware [1], and the same
> "am65x-sr2" firmwares are used on AM65x and AM64x SoCs.
> 

Yes currently the firmware used for both AM65x and AM64x is am65x-sr2*,
but that firmware name is not taken from device tree. It is directly
encoded in the driver. Plan is to have different firmware for both SoCs
in future and when that happens driver will read the firmware name from
device tree. For that case, the firmware name here is "am64x-sr2*"

[1] https://lore.kernel.org/all/20231207134343.ufiy2owik5kn3y2r@degrease/

> [1] https://git.ti.com/gitweb?p=processor-firmware/ti-linux-firmware.git;a=tree;f=ti-pruss;h=a220bdc6dce5e11845b5c6337ff9b2d329aee196;hb=refs/heads/ti-linux-firmware
> 
>  .../dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts    | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts b/arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts
> index 1f4dc5ad1696a..0eff392a29b00 100644
> --- a/arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am642-tqma64xxl-mbax4xxl.dts
> @@ -24,6 +24,8 @@ / {
>  
>  	aliases {
>  		ethernet0 = &cpsw_port1;
> +		ethernet1 = &icssg1_emac0;
> +		ethernet2 = &icssg1_emac1;
>  		i2c1 = &mcu_i2c0;
>  		mmc1 = &sdhci1;
>  		serial0 = &mcu_uart0;
> @@ -154,6 +156,104 @@ &epwm5 {
>  	status = "okay";
>  };
>  
> +&icssg1 {
> +	icssg1_eth: ethernet {
> +		compatible = "ti,am642-icssg-prueth";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pru_icssg1_rgmii1_pins>, <&pru_icssg1_rgmii2_pins>;
> +		interrupt-parent = <&icssg1_intc>;
> +		interrupts = <24 0 2>, <25 1 3>;
> +		interrupt-names = "tx_ts0", "tx_ts1";
> +		dmas = <&main_pktdma 0xc200 15>, /* egress slice 0 */
> +		       <&main_pktdma 0xc201 15>, /* egress slice 0 */
> +		       <&main_pktdma 0xc202 15>, /* egress slice 0 */
> +		       <&main_pktdma 0xc203 15>, /* egress slice 0 */
> +		       <&main_pktdma 0xc204 15>, /* egress slice 1 */
> +		       <&main_pktdma 0xc205 15>, /* egress slice 1 */
> +		       <&main_pktdma 0xc206 15>, /* egress slice 1 */
> +		       <&main_pktdma 0xc207 15>, /* egress slice 1 */
> +		       <&main_pktdma 0x4200 15>, /* ingress slice 0 */
> +		       <&main_pktdma 0x4201 15>; /* ingress slice 1 */
> +		dma-names = "tx0-0", "tx0-1", "tx0-2", "tx0-3",
> +			    "tx1-0", "tx1-1", "tx1-2", "tx1-3",
> +			    "rx0", "rx1";
> +		sram = <&oc_sram>;
> +		firmware-name = "ti-pruss/am65x-sr2-pru0-prueth-fw.elf",
> +				"ti-pruss/am65x-sr2-rtu0-prueth-fw.elf",
> +				"ti-pruss/am65x-sr2-txpru0-prueth-fw.elf",
> +				"ti-pruss/am65x-sr2-pru1-prueth-fw.elf",
> +				"ti-pruss/am65x-sr2-rtu1-prueth-fw.elf",
> +				"ti-pruss/am65x-sr2-txpru1-prueth-fw.elf";
> +		ti,prus = <&pru1_0>, <&rtu1_0>, <&tx_pru1_0>, <&pru1_1>, <&rtu1_1>, <&tx_pru1_1>;
> +		ti,pruss-gp-mux-sel = <2>,	/* MII mode */
> +				      <2>,
> +				      <2>,
> +				      <2>,	/* MII mode */
> +				      <2>,
> +				      <2>;
> +		ti,mii-g-rt = <&icssg1_mii_g_rt>;
> +		ti,mii-rt = <&icssg1_mii_rt>;
> +		ti,iep = <&icssg1_iep0>,  <&icssg1_iep1>;
> +
> +		ethernet-ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			icssg1_emac0: port@0 {
> +				reg = <0>;
> +				phy-handle = <&icssg1_phy0c>;
> +				phy-mode = "rgmii-id";
> +				/* Filled in by bootloader */
> +				local-mac-address = [00 00 00 00 00 00];
> +			};
> +
> +			icssg1_emac1: port@1 {
> +				reg = <1>;
> +				phy-handle = <&icssg1_phy03>;
> +				phy-mode = "rgmii-id";
> +				/* Filled in by bootloader */
> +				local-mac-address = [00 00 00 00 00 00];
> +			};
> +		};
> +	};
> +};
> +
> +&icssg1_mdio {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pru_icssg1_mdio_pins>;
> +	status = "okay";
> +
> +	/* phy-mode is fixed up to rgmii-rxid by prueth driver to account for
> +	 * the SoC integration, so the only rx-internal-delay and no
> +	 * tx-internal-delay is set for the PHYs.
> +	 */
> +
> +	icssg1_phy03: ethernet-phy@3 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <0x3>;
> +		reset-gpios = <&main_gpio1 47 GPIO_ACTIVE_LOW>;
> +		reset-assert-us = <1000>;
> +		reset-deassert-us = <1000>;
> +		ti,rx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> +		ti,tx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> +		ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> +		ti,clk-output-sel = <DP83867_CLK_O_SEL_OFF>;
> +	};
> +
> +	icssg1_phy0c: ethernet-phy@c {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <0xc>;
> +		reset-gpios = <&main_gpio1 51 GPIO_ACTIVE_LOW>;
> +		reset-assert-us = <1000>;
> +		reset-deassert-us = <1000>;
> +		ti,rx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> +		ti,tx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> +		ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> +		ti,clk-output-sel = <DP83867_CLK_O_SEL_OFF>;
> +	};
> +};
> +
> +
>  &main_gpio0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&main_gpio0_digital_pins>,


-- 
Thanks and Regards,
Danish

Re: [PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node

[Krzysztof Kozlowski]

On 19/06/2024 13:24, Matthias Schiffer wrote:
> While the current Device Trees for TI EVMs configure the PRUSS Ethernet
> controller as a toplevel node with names like "icssg1-eth", allowing to
> make it a subnode of the ICSSG has a number of advantages:

What is ICSSG? The sram or ti,prus from the ethernet schema?

> 
> - It makes sense semantically - the Ethernet controller is running on
>   the ICSSG/PRUSS
> - Disabling or deleting the ICSSG node implicitly removes the Ethernet
>   controller node when it is a child node. This can be relevant on SoCs
>   like the AM64x which come in variants with and without ICSSG; e.g., on
>   the TQMa64xxL the ICSSG node will be disabled on variants without as a
>   bootloader fixup.
>   On Linux, this avoids leaving the Ethernet controller in deferred
>   state forever while waiting for the ICSSG to become available
>   (resulting in a warning on newer kernels)
> 
> The node name "ethernet" is chosen as it nicely matches the regular
> "ethernet@<reg>" format of many Ethernet controller nodes, and is also
> what the prueth binding example (/schemas/net/ti,icssg-prueth.yaml) uses.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
> index c402cb2928e89..89dfcf5ce8434 100644
> --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
> +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
> @@ -92,6 +92,13 @@ properties:
>      description: |
>        This property is as per sci-pm-domain.txt.
>  
> +  ethernet:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      ICSSG PRUSS Ethernet. Configuration for an Ethernet controller running
> +      on the PRU-ICSS.
> +    $ref: /schemas/net/ti,icssg-prueth.yaml#
> +    type: object
> +
>  patternProperties:
>  
>    memories@[a-f0-9]+$:

You are mixing MMIO and non-MMIO nodes. That's odd or even sloppy
design. It immediately raises questions about your bindings.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node

[Matthias Schiffer]

On Thu, 2024-06-20 at 09:24 +0200, Krzysztof Kozlowski wrote:
> On 19/06/2024 13:24, Matthias Schiffer wrote:
> > While the current Device Trees for TI EVMs configure the PRUSS Ethernet
> > controller as a toplevel node with names like "icssg1-eth", allowing to
> > make it a subnode of the ICSSG has a number of advantages:
> 
> What is ICSSG? The sram or ti,prus from the ethernet schema?

ICSSG (Industrial Communication Subsystem (Group?)) is the main device described by the
ti,pruss.yaml binding (ICSS and PRUSS are different variants of similar IP cores); it is the
container for the individual PRU, TXPRU and RTU cores which are referenced by the ti,prus
node of the Ethernet schema.

The entirety of PRU, TXPRU and RTU cores of one ICSSG, each with its own firmware, forms one
Ethernet controller, which is not quite a hardware device, but also not a fully virtual software
device.

The Ethernet controller only exists through the various ICSS subcores, so it doesn't have an MMIO
address of its own. As described, the existing Device Trees define it as a toplevel non-MMIO node;
we propose to allow it as a non-MMIO child node of the ICSSG container instead.

If you consider moving the ethernet node into the ICSSG node a bad approach, we will drop this patch
and try to find a different solution to our issue (the Ethernet device staying in deferred state
forever when the ICSSG node is disabled on Linux).

Best regards,
Matthias



> 
> > 
> > - It makes sense semantically - the Ethernet controller is running on
> >   the ICSSG/PRUSS
> > - Disabling or deleting the ICSSG node implicitly removes the Ethernet
> >   controller node when it is a child node. This can be relevant on SoCs
> >   like the AM64x which come in variants with and without ICSSG; e.g., on
> >   the TQMa64xxL the ICSSG node will be disabled on variants without as a
> >   bootloader fixup.
> >   On Linux, this avoids leaving the Ethernet controller in deferred
> >   state forever while waiting for the ICSSG to become available
> >   (resulting in a warning on newer kernels)
> > 
> > The node name "ethernet" is chosen as it nicely matches the regular
> > "ethernet@<reg>" format of many Ethernet controller nodes, and is also
> > what the prueth binding example (/schemas/net/ti,icssg-prueth.yaml) uses.
> > 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
> > index c402cb2928e89..89dfcf5ce8434 100644
> > --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
> > +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
> > @@ -92,6 +92,13 @@ properties:
> >      description: |
> >        This property is as per sci-pm-domain.txt.
> >  
> > +  ethernet:
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +      ICSSG PRUSS Ethernet. Configuration for an Ethernet controller running
> > +      on the PRU-ICSS.
> > +    $ref: /schemas/net/ti,icssg-prueth.yaml#
> > +    type: object
> > +
> >  patternProperties:
> >  
> >    memories@[a-f0-9]+$:
> 
> You are mixing MMIO and non-MMIO nodes. That's odd or even sloppy
> design. It immediately raises questions about your bindings.
> 
> Best regards,
> Krzysztof
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node

[Krzysztof Kozlowski]

On 20/06/2024 10:26, Matthias Schiffer wrote:
> On Thu, 2024-06-20 at 09:24 +0200, Krzysztof Kozlowski wrote:
>> On 19/06/2024 13:24, Matthias Schiffer wrote:
>>> While the current Device Trees for TI EVMs configure the PRUSS Ethernet
>>> controller as a toplevel node with names like "icssg1-eth", allowing to
>>> make it a subnode of the ICSSG has a number of advantages:
>>
>> What is ICSSG? The sram or ti,prus from the ethernet schema?
> 
> ICSSG (Industrial Communication Subsystem (Group?)) is the main device described by the
> ti,pruss.yaml binding (ICSS and PRUSS are different variants of similar IP cores); it is the
> container for the individual PRU, TXPRU and RTU cores which are referenced by the ti,prus
> node of the Ethernet schema.
> 
> The entirety of PRU, TXPRU and RTU cores of one ICSSG, each with its own firmware, forms one
> Ethernet controller, which is not quite a hardware device, but also not a fully virtual software
> device.

So it is not really child of ICSSG.

> 
> The Ethernet controller only exists through the various ICSS subcores, so it doesn't have an MMIO
> address of its own. As described, the existing Device Trees define it as a toplevel non-MMIO node;
> we propose to allow it as a non-MMIO child node of the ICSSG container instead.
> 
> If you consider moving the ethernet node into the ICSSG node a bad approach, we will drop this patch
> and try to find a different solution to our issue (the Ethernet device staying in deferred state
> forever when the ICSSG node is disabled on Linux).

Just disable the ethernet. That's the expected behavior, I don't get
what is the problem here.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node

[Matthias Schiffer]

On Thu, 2024-06-20 at 10:29 +0200, Krzysztof Kozlowski wrote:
> 
> 
> On 20/06/2024 10:26, Matthias Schiffer wrote:
> > On Thu, 2024-06-20 at 09:24 +0200, Krzysztof Kozlowski wrote:
> > > On 19/06/2024 13:24, Matthias Schiffer wrote:
> > > > While the current Device Trees for TI EVMs configure the PRUSS Ethernet
> > > > controller as a toplevel node with names like "icssg1-eth", allowing to
> > > > make it a subnode of the ICSSG has a number of advantages:
> > > 
> > > What is ICSSG? The sram or ti,prus from the ethernet schema?
> > 
> > ICSSG (Industrial Communication Subsystem (Group?)) is the main device described by the
> > ti,pruss.yaml binding (ICSS and PRUSS are different variants of similar IP cores); it is the
> > container for the individual PRU, TXPRU and RTU cores which are referenced by the ti,prus
> > node of the Ethernet schema.
> > 
> > The entirety of PRU, TXPRU and RTU cores of one ICSSG, each with its own firmware, forms one
> > Ethernet controller, which is not quite a hardware device, but also not a fully virtual software
> > device.
> 
> So it is not really child of ICSSG.
> 
> > 
> > The Ethernet controller only exists through the various ICSS subcores, so it doesn't have an MMIO
> > address of its own. As described, the existing Device Trees define it as a toplevel non-MMIO node;
> > we propose to allow it as a non-MMIO child node of the ICSSG container instead.
> > 
> > If you consider moving the ethernet node into the ICSSG node a bad approach, we will drop this patch
> > and try to find a different solution to our issue (the Ethernet device staying in deferred state
> > forever when the ICSSG node is disabled on Linux).
> 
> Just disable the ethernet. That's the expected behavior, I don't get
> what is the problem here.

If the disabling happens as a fixup in the bootloader, it needs to know the name of the Ethernet
controller node (or iterate through the DTB to find references to the disabled ICSSG node).

The name is currently not used for anything, and not specified in the binding doc; the example uses
"ethernet", which is too unspecific, as there can be multiple ICSSG/PRUs, with each running a
separate Ethernet controller.

Existing Device trees use "icssgX-eth" for an Ethernet controller running on the ICSSG with label
"&icssgX", but labels are a source concept and don't exist in the compiled DTB by default.

I do have an idea for an alternative approach that does not need changes to the DT bindings: The PRU
Ethernet driver could detect that the referenced ti,prus are disabled and not just waiting to be
probed and then fail with ENODEV instead of EPROBE_DEFER.

Best regards,
Matthias


> 
> 
> Best regards,
> Krzysztof
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node

[Krzysztof Kozlowski]

On 20/06/2024 10:48, Matthias Schiffer wrote:
> On Thu, 2024-06-20 at 10:29 +0200, Krzysztof Kozlowski wrote:
>>
>>
>> On 20/06/2024 10:26, Matthias Schiffer wrote:
>>> On Thu, 2024-06-20 at 09:24 +0200, Krzysztof Kozlowski wrote:
>>>> On 19/06/2024 13:24, Matthias Schiffer wrote:
>>>>> While the current Device Trees for TI EVMs configure the PRUSS Ethernet
>>>>> controller as a toplevel node with names like "icssg1-eth", allowing to
>>>>> make it a subnode of the ICSSG has a number of advantages:
>>>>
>>>> What is ICSSG? The sram or ti,prus from the ethernet schema?
>>>
>>> ICSSG (Industrial Communication Subsystem (Group?)) is the main device described by the
>>> ti,pruss.yaml binding (ICSS and PRUSS are different variants of similar IP cores); it is the
>>> container for the individual PRU, TXPRU and RTU cores which are referenced by the ti,prus
>>> node of the Ethernet schema.
>>>
>>> The entirety of PRU, TXPRU and RTU cores of one ICSSG, each with its own firmware, forms one
>>> Ethernet controller, which is not quite a hardware device, but also not a fully virtual software
>>> device.
>>
>> So it is not really child of ICSSG.
>>
>>>
>>> The Ethernet controller only exists through the various ICSS subcores, so it doesn't have an MMIO
>>> address of its own. As described, the existing Device Trees define it as a toplevel non-MMIO node;
>>> we propose to allow it as a non-MMIO child node of the ICSSG container instead.
>>>
>>> If you consider moving the ethernet node into the ICSSG node a bad approach, we will drop this patch
>>> and try to find a different solution to our issue (the Ethernet device staying in deferred state
>>> forever when the ICSSG node is disabled on Linux).
>>
>> Just disable the ethernet. That's the expected behavior, I don't get
>> what is the problem here.
> 
> If the disabling happens as a fixup in the bootloader, it needs to know the name of the Ethernet
> controller node (or iterate through the DTB to find references to the disabled ICSSG node).

Which is already solved for several such cases, including ethernet
devices? Aliases?

> 
> The name is currently not used for anything, and not specified in the binding doc; the example uses
> "ethernet", which is too unspecific, as there can be multiple ICSSG/PRUs, with each running a
> separate Ethernet controller.

Use existing solutions - aliases.

> 
> Existing Device trees use "icssgX-eth" for an Ethernet controller running on the ICSSG with label
> "&icssgX", but labels are a source concept and don't exist in the compiled DTB by default.
> 
> I do have an idea for an alternative approach that does not need changes to the DT bindings: The PRU
> Ethernet driver could detect that the referenced ti,prus are disabled and not just waiting to be
> probed and then fail with ENODEV instead of EPROBE_DEFER.

Sorry, but re-shuffling nodes into incorrect hardware description is not
the workaround for your problem.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: soc: ti: pruss: allow ethernet controller in ICSSG node

[Matthias Schiffer]

On Wed, 2024-06-26 at 09:33 +0200, Krzysztof Kozlowski wrote:
> 
> On 20/06/2024 10:48, Matthias Schiffer wrote:
> > On Thu, 2024-06-20 at 10:29 +0200, Krzysztof Kozlowski wrote:
> > > 
> > > 
> > > On 20/06/2024 10:26, Matthias Schiffer wrote:
> > > > On Thu, 2024-06-20 at 09:24 +0200, Krzysztof Kozlowski wrote:
> > > > > On 19/06/2024 13:24, Matthias Schiffer wrote:
> > > > > > While the current Device Trees for TI EVMs configure the PRUSS Ethernet
> > > > > > controller as a toplevel node with names like "icssg1-eth", allowing to
> > > > > > make it a subnode of the ICSSG has a number of advantages:
> > > > > 
> > > > > What is ICSSG? The sram or ti,prus from the ethernet schema?
> > > > 
> > > > ICSSG (Industrial Communication Subsystem (Group?)) is the main device described by the
> > > > ti,pruss.yaml binding (ICSS and PRUSS are different variants of similar IP cores); it is the
> > > > container for the individual PRU, TXPRU and RTU cores which are referenced by the ti,prus
> > > > node of the Ethernet schema.
> > > > 
> > > > The entirety of PRU, TXPRU and RTU cores of one ICSSG, each with its own firmware, forms one
> > > > Ethernet controller, which is not quite a hardware device, but also not a fully virtual software
> > > > device.
> > > 
> > > So it is not really child of ICSSG.
> > > 
> > > > 
> > > > The Ethernet controller only exists through the various ICSS subcores, so it doesn't have an MMIO
> > > > address of its own. As described, the existing Device Trees define it as a toplevel non-MMIO node;
> > > > we propose to allow it as a non-MMIO child node of the ICSSG container instead.
> > > > 
> > > > If you consider moving the ethernet node into the ICSSG node a bad approach, we will drop this patch
> > > > and try to find a different solution to our issue (the Ethernet device staying in deferred state
> > > > forever when the ICSSG node is disabled on Linux).
> > > 
> > > Just disable the ethernet. That's the expected behavior, I don't get
> > > what is the problem here.
> > 
> > If the disabling happens as a fixup in the bootloader, it needs to know the name of the Ethernet
> > controller node (or iterate through the DTB to find references to the disabled ICSSG node).
> 
> Which is already solved for several such cases, including ethernet
> devices? Aliases?
> 
> > 
> > The name is currently not used for anything, and not specified in the binding doc; the example uses
> > "ethernet", which is too unspecific, as there can be multiple ICSSG/PRUs, with each running a
> > separate Ethernet controller.
> 
> Use existing solutions - aliases.

Understood.

I'm not entirely happy that the bootloader needs to know that it is an Ethernet controller that is
provided by the ICSSG, and there isn't a simple way to say "whatever kind of device that Linux's DTB
loads into the ICSSG should be disabled".

But I guess for most boards there is only a single kind of ICSSG firmware that is used anyways. So
I'm going with the solution you propose for now.

Best regards,
Matthias


> 
> > 
> > Existing Device trees use "icssgX-eth" for an Ethernet controller running on the ICSSG with label
> > "&icssgX", but labels are a source concept and don't exist in the compiled DTB by default.
> > 
> > I do have an idea for an alternative approach that does not need changes to the DT bindings: The PRU
> > Ethernet driver could detect that the referenced ti,prus are disabled and not just waiting to be
> > probed and then fail with ENODEV instead of EPROBE_DEFER.
> 
> Sorry, but re-shuffling nodes into incorrect hardware description is not
> the workaround for your problem.
> 
> Best regards,
> Krzysztof
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/