Am 15.01.24 um 08:29 schrieb Krzysztof Kozlowski: > On 14/01/2024 15:16, Josua Mayer wrote: >> 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". > Why? This intentionally should not include specific name. I understand. Which is why I imagined in the other reference had intentionally diverged from that rule. > > Also, wrap your emails at proper length so they will be manageable... > >>> 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. > So if there is a bug, you are going to duplicate it. I was torn between making my own solution, and using recently added and topical (to my submission) code as template. > > Please provide real argument why this is needed, not "I saw it > somewhere", or drop it. Otherwise it's a NAK from me. I will attempt to improve the magic numbers in this whole node, and reconsider the node name. Thanks. > >> 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. > How does dmas matter? What are you talking about? I am trying to establish whether I can use that as example or not. Clearly it is a bad example, and I should try describing it better. > > > > Best regards, > Krzysztof >