Re: [PATCH v2 4/5] arm64: dts: add description for solidrun am642 som and evaluation board

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 12/01/2024 18:12, Josua Mayer wrote:
> Add description for the SolidRun AM642 SoM, and HummingBoard-T
> evaluation board.
> 
...

> +&main_gpio0 {
> +	m2-reset-hog {
> +		gpio-hog;
> +		gpios = <12 GPIO_ACTIVE_LOW>;
> +		output-low; /* deasserted */
> +		line-name = "m2-reset";
> +	};
> +
> +	m1-m2-w-disable1-hog {
> +		gpio-hog;
> +		gpios = <32 GPIO_ACTIVE_LOW>;
> +		output-low; /* deasserted */
> +		line-name = "m1-m2-pcie-w-disable1";
> +	};
> +
> +	m1-m2-w_disable2-hog {

No, underscores are not allowed.

...

> +
> +#include <dt-bindings/net/ti-dp83869.h>
> +
> +/ {
> +	model = "SolidRun AM642 SoM";
> +	compatible = "solidrun,am642-sr-som", "ti,am642";
> +
> +	aliases {
> +		ethernet0 = &cpsw_port1;
> +		ethernet1 = &icssg1_emac0;
> +		ethernet2 = &icssg1_emac1;
> +		mmc0 = &sdhci0;
> +		mmc1 = &sdhci1;
> +		serial2 = &main_uart0;
> +	};
> +
> +	chosen {
> +		/* SoC default UART console */
> +		stdout-path = "serial2:115200n8";
> +	};
> +
> +	/* PRU Ethernet Controller */
> +	icssg1_eth: icssg1-eth {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +		compatible = "ti,am642-icssg-prueth";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pru_rgmii1_pins_default>, <&pru_rgmii2_pins_default>;
> +
> +		sram = <&oc_sram>;
> +		ti,prus = <&pru1_0>, <&rtu1_0>, <&tx_pru1_0>, <&pru1_1>, <&rtu1_1>, <&tx_pru1_1>;
> +		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,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>;
> +
> +		interrupt-parent = <&icssg1_intc>;
> +		interrupts = <24 0 2>, <25 1 3>;

None of these are typical interrupt constants/flags?

> +		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 */
> +		       <&main_pktdma 0x4202 0>, /* mgmnt rsp slice 0 */
> +		       <&main_pktdma 0x4203 0>; /* mgmnt rsp slice 1 */
> +		dma-names = "tx0-0", "tx0-1", "tx0-2", "tx0-3",
> +			    "tx1-0", "tx1-1", "tx1-2", "tx1-3",
> +			    "rx0", "rx1";
> +
> +		status = "okay";

Drop. Didn't you get such comments before?

> +
> +		ethernet-ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			icssg1_emac0: port@0 {
> +				reg = <0>;
> +				ti,syscon-rgmii-delay = <&main_conf 0x4110>;
> +				/* Filled in by bootloader */
> +				local-mac-address = [00 00 00 00 00 00];
> +				phy-handle = <&ethernet_phy2>;
> +				phy-mode = "rgmii-id";
> +				status = "okay";

?

> +			};
> +
> +			icssg1_emac1: port@1 {
> +				reg = <1>;
> +				ti,syscon-rgmii-delay = <&main_conf 0x4114>;
> +				/* Filled in by bootloader */
> +				local-mac-address = [00 00 00 00 00 00];
> +				phy-handle = <&ethernet_phy1>;
> +				phy-mode = "rgmii-id";
> +				status = "okay";

?


....

> +	ethernet_phy0: ethernet-phy@0 {
> +		compatible = "ethernet-phy-id2000.a0f1";
> +		reg = <0>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&ethernet_phy0_pins_default>;
> +		ti,clk-output-sel = <DP83869_CLK_O_SEL_REF_CLK>;
> +		ti,op-mode = <DP83869_RGMII_COPPER_ETHERNET>;
> +		/*
> +		 * Disable interrupts because ISR never clears 0x0040
> +		 *
> +		 * interrupt-parent = <&main_gpio1>;
> +		 * interrupts = <70 IRQ_TYPE_LEVEL_LOW>;
> +		 */
> +		/*
> +		 * Disable HW Reset because clock signal is daisy-chained
> +		 *
> +		 * reset-gpios = <&main_gpio0 84 GPIO_ACTIVE_LOW>;
> +		 * reset-assert-us = <1>;
> +		 * reset-deassert-us = <30>;
> +		 */
> +		 status = "okay";

Drop, this applies everywhere where not needed. You have this in
multiple places...

Best regards,
Krzysztof