Hi Krzysztof, Thanks for the comments. On 06/02/23 13:20, Krzysztof Kozlowski wrote: > On 06/02/2023 07:07, MD Danish Anwar wrote: >> From: Puranjay Mohan <p-mohan@xxxxxx> >> >> Add a YAML binding document for the ICSSG Programmable real time unit >> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs > > You add a binding for the hardware, not for driver. > >> to interface the PRUs and load/run the firmware for supporting ethernet >> functionality. > > Subject: drop second/last, redundant "driver bindings". The > "dt-bindings" prefix is already stating that these are bindings. > Sure I will modify the subject and commit description. >> >> Signed-off-by: Puranjay Mohan <p-mohan@xxxxxx> >> Signed-off-by: Md Danish Anwar <danishanwar@xxxxxx> >> --- >> .../bindings/net/ti,icssg-prueth.yaml | 179 ++++++++++++++++++ >> 1 file changed, 179 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> new file mode 100644 >> index 000000000000..e4dee01a272a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> @@ -0,0 +1,179 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/ti,icssg-prueth.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Texas Instruments ICSSG PRUSS Ethernet >> + >> +maintainers: >> + - Md Danish Anwar <danishanwar@xxxxxx> >> + >> +description: >> + Ethernet based on the Programmable Real-Time >> + Unit and Industrial Communication Subsystem. >> + >> +allOf: >> + - $ref: /schemas/remoteproc/ti,pru-consumer.yaml# >> + >> +properties: >> + compatible: >> + enum: >> + - ti,am654-icssg-prueth # for AM65x SoC family >> + >> + ti,sram: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + phandle to MSMC SRAM node >> + >> + dmas: >> + maxItems: 10 >> + >> + dma-names: >> + items: >> + - const: tx0-0 >> + - const: tx0-1 >> + - const: tx0-2 >> + - const: tx0-3 >> + - const: tx1-0 >> + - const: tx1-1 >> + - const: tx1-2 >> + - const: tx1-3 >> + - const: rx0 >> + - const: rx1 >> + >> + ethernet-ports: > > Bring some order or logic in the order of the properties. Keep the > ethernet-ports as last property. > Sure, I will re-arrange them. >> + type: object >> + additionalProperties: false > > Blank line > >> + properties: >> + '#address-cells': >> + const: 1 >> + '#size-cells': >> + const: 0 >> + >> + patternProperties: >> + ^port@[0-1]$: >> + type: object >> + description: ICSSG PRUETH external ports >> + At least one ethernet port is required. Should I add the below line here for this? minItems: 1 >> Drop blank line > >> + $ref: ethernet-controller.yaml# >> + > > Drop blank line > >> + unevaluatedProperties: false >> + >> + properties: >> + reg: >> + items: >> + - enum: [0, 1] >> + description: ICSSG PRUETH port number >> + >> + interrupts-extended: > > Just "interrupts" I will drop / add blank lines and change this to just "interrupts". >> + maxItems: 1 >> + >> + ti,syscon-rgmii-delay: >> + items: >> + - items: >> + - description: phandle to system controller node >> + - description: The offset to ICSSG control register >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: >> + phandle to system controller node and register offset >> + to ICSSG control register for RGMII transmit delay >> + >> + required: >> + - reg > > required for ethernet-ports - at least one port is required, isn't it? Yes, at least one ethernet port is required. Should I add "minItems: 1" in patternProperties section or should I add a new required section in patternProperties and mention something like port@0 as required? >> + >> + ti,mii-g-rt: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + phandle to MII_G_RT module's syscon regmap. >> + >> + ti,mii-rt: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + phandle to MII_RT module's syscon regmap >> + >> + interrupts: >> + maxItems: 2 >> + description: | >> + Interrupt specifiers to TX timestamp IRQ. >> + >> + interrupt-names: >> + items: >> + - const: tx_ts0 >> + - const: tx_ts1 >> + >> +required: >> + - compatible >> + - ti,sram >> + - dmas >> + - dma-names >> + - ethernet-ports >> + - ti,mii-g-rt >> + - interrupts >> + - interrupt-names >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + /* Example k3-am654 base board SR2.0, dual-emac */ >> + pruss2_eth: ethernet { >> + compatible = "ti,am654-icssg-prueth"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&icssg2_rgmii_pins_default>; >> + ti,sram = <&msmc_ram>; >> + >> + ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>, >> + <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>; >> + firmware-name = "ti-pruss/am65x-pru0-prueth-fw.elf", >> + "ti-pruss/am65x-rtu0-prueth-fw.elf", >> + "ti-pruss/am65x-txpru0-prueth-fw.elf", >> + "ti-pruss/am65x-pru1-prueth-fw.elf", >> + "ti-pruss/am65x-rtu1-prueth-fw.elf", >> + "ti-pruss/am65x-txpru1-prueth-fw.elf"; >> + ti,pruss-gp-mux-sel = <2>, /* MII mode */ >> + <2>, >> + <2>, >> + <2>, /* MII mode */ >> + <2>, >> + <2>; >> + dmas = <&main_udmap 0xc300>, /* egress slice 0 */ >> + <&main_udmap 0xc301>, /* egress slice 0 */ >> + <&main_udmap 0xc302>, /* egress slice 0 */ >> + <&main_udmap 0xc303>, /* egress slice 0 */ >> + <&main_udmap 0xc304>, /* egress slice 1 */ >> + <&main_udmap 0xc305>, /* egress slice 1 */ >> + <&main_udmap 0xc306>, /* egress slice 1 */ >> + <&main_udmap 0xc307>, /* egress slice 1 */ >> + <&main_udmap 0x4300>, /* ingress slice 0 */ >> + <&main_udmap 0x4301>; /* ingress slice 1 */ >> + dma-names = "tx0-0", "tx0-1", "tx0-2", "tx0-3", >> + "tx1-0", "tx1-1", "tx1-2", "tx1-3", >> + "rx0", "rx1"; >> + ti,mii-g-rt = <&icssg2_mii_g_rt>; >> + interrupts = <24 0 2>, <25 1 3>; > > Aren't you open-coding some IRQ flags? > These values are not open coded here. These values are the expected values for interrupt node. Here, Cell 1 -> PRU System event number Cell 2 -> PRU channel Cell 3 -> PRU host_event (target) This is documented in detail in Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml I also missed mentioning "interrupt-parent" in example section. I will add interrupt-parent in next revision. interrupt-parent = <&icssg2_intc>; >> + interrupt-names = "tx_ts0", "tx_ts1"; >> + ethernet-ports { > > > Best regards, > Krzysztof > -- Thanks and Regards, Danish.
