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

[Josua Mayer <josua@solid-run.com>]

Am 12.01.24 um 18:22 schrieb Krzysztof Kozlowski:

>> +	/* PRU Ethernet Controller */
>> +	icssg1_eth: icssg1-eth {
> Node names should be generic.
This name intentionally includes the name of the ip block within am64 soc providing software-defined ethernet controller through coprocessors TI call "pru".
> 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?

Yes, but again I can point to an in-tree example of the same structure.
I see no reason for describing the same thing differently in different places.

Please see arch/arm64/boot/dts/ti/k3-am654-idk.dtso
There are only small differences for this feature between am65 and am64.
It's inclusion in the tree was very recent, clearly it was good enough right?
See also my cover letter dtbs_check remark on dmas property.

>
>> +
>> +		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";
> ?
good point, removed all the status from icssg1-eth node.
>
>
> ....
>
>> +	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...
Is this about status, or commented interrupt and reset definitions?

Thank you for the review!

Sincerely
Josua Mayer