From: Puranjay Mohan <[email protected]>
Add a YAML binding document for the ICSSG Programmable real time unit
based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs
to interface the PRUs and load/run the firmware for supporting ethernet
functionality.
Signed-off-by: Puranjay Mohan <[email protected]>
Signed-off-by: Md Danish Anwar <[email protected]>
---
.../bindings/net/ti,icssg-prueth.yaml | 174 ++++++++++++++++++
1 file changed, 174 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..7659f5fd3132
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
@@ -0,0 +1,174 @@
+# 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:
+ - Puranjay Mohan <[email protected]>
+ - Md Danish Anwar <[email protected]>
+
+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
+
+ sram:
+ 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:
+ type: object
+ properties:
+ '#address-cells':
+ const: 1
+ '#size-cells':
+ const: 0
+
+ patternProperties:
+ ^port@[0-1]$:
+ type: object
+ description: ICSSG PRUETH external ports
+
+ $ref: ethernet-controller.yaml#
+
+ unevaluatedProperties: false
+ additionalProperties: false
+ properties:
+ reg:
+ items:
+ - enum: [0, 1]
+ description: ICSSG PRUETH port number
+
+ ti,syscon-rgmii-delay:
+ $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
+
+ 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:
+ minItems: 2
+ maxItems: 2
+ description: |
+ Interrupt specifiers to TX timestamp IRQ.
+
+ interrupt-names:
+ items:
+ - const: tx_ts0
+ - const: tx_ts1
+
+required:
+ - compatible
+ - sram
+ - ti,mii-g-rt
+ - dmas
+ - dma-names
+ - ethernet-ports
+ - interrupts
+ - interrupt-names
+
+unevaluatedProperties: false
+
+examples:
+ - |
+
+ /* Example k3-am654 base board SR2.0, dual-emac */
+ pruss2_eth: pruss2_eth {
+ compatible = "ti,am654-icssg-prueth";
+ pinctrl-names = "default";
+ pinctrl-0 = <&icssg2_rgmii_pins_default>;
+ 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>;
+ ti,mii-g-rt = <&icssg2_mii_g_rt>;
+ 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";
+ interrupts = <24 0 2>, <25 1 3>;
+ interrupt-names = "tx_ts0", "tx_ts1";
+ ethernet-ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ pruss2_emac0: port@0 {
+ reg = <0>;
+ phy-handle = <&pruss2_eth0_phy>;
+ phy-mode = "rgmii-rxid";
+ interrupts-extended = <&icssg2_intc 24>;
+ ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
+ /* Filled in by bootloader */
+ local-mac-address = [00 00 00 00 00 00];
+ };
+
+ pruss2_emac1: port@1 {
+ reg = <1>;
+ phy-handle = <&pruss2_eth1_phy>;
+ phy-mode = "rgmii-rxid";
+ interrupts-extended = <&icssg2_intc 25>;
+ ti,syscon-rgmii-delay = <&scm_conf 0x4124>;
+ /* Filled in by bootloader */
+ local-mac-address = [00 00 00 00 00 00];
+ };
+ };
+ };
--
2.25.1
On 23/12/2022 12:09, MD Danish Anwar wrote:
> From: Puranjay Mohan <[email protected]>
>
> Add a YAML binding document for the ICSSG Programmable real time unit
> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs
> to interface the PRUs and load/run the firmware for supporting ethernet
> functionality.
>
> Signed-off-by: Puranjay Mohan <[email protected]>
> Signed-off-by: Md Danish Anwar <[email protected]>
> ---
> .../bindings/net/ti,icssg-prueth.yaml | 174 ++++++++++++++++++
> 1 file changed, 174 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..7659f5fd3132
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> @@ -0,0 +1,174 @@
> +# 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
Keep it in one line.
> +
> +maintainers:
> + - Puranjay Mohan <[email protected]>
> + - Md Danish Anwar <[email protected]>
> +
> +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
> +
> + sram:
> + description:
> + phandle to MSMC SRAM node
Where does the definition of this come from? If from nowhere, usually
you need vendor prefix and type/ref.
> +
> + 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:
> + type: object
> + properties:
> + '#address-cells':
> + const: 1
> + '#size-cells':
> + const: 0
> +
> + patternProperties:
> + ^port@[0-1]$:
> + type: object
> + description: ICSSG PRUETH external ports
> +
> + $ref: ethernet-controller.yaml#
> +
> + unevaluatedProperties: false
> + additionalProperties: false
You cannot have both. Did you test the binding? I doubt it...
> + properties:
> + reg:
> + items:
> + - enum: [0, 1]
> + description: ICSSG PRUETH port number
> +
> + ti,syscon-rgmii-delay:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description:
> + phandle to system controller node and register offset
> + to ICSSG control register for RGMII transmit delay
You need to describe the items. And in your own bindings from TI you
will even find example...
> +
> + required:
> + - reg
> +
> + 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
Why do you have so many phandles? Aren't you missing some phys?
> +
> + interrupts:
> + minItems: 2
Drop minItems
> + maxItems: 2
> + description: |
> + Interrupt specifiers to TX timestamp IRQ.
> +
> + interrupt-names:
> + items:
> + - const: tx_ts0
> + - const: tx_ts1
> +
> +required:
> + - compatible
> + - sram
> + - ti,mii-g-rt
Keep same order as in properties:.
> + - dmas
> + - dma-names
> + - ethernet-ports
> + - interrupts
> + - interrupt-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> +
No need for blank line.
> + /* Example k3-am654 base board SR2.0, dual-emac */
> + pruss2_eth: pruss2_eth {
No underscores in node names.
Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "ti,am654-icssg-prueth";
> + pinctrl-names = "default";
> + pinctrl-0 = <&icssg2_rgmii_pins_default>;
> + 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";
I really doubt it was tested... and maybe there will be no testing from
Rob's bot due to dependency.
Please post dt_binding_check testing results.
Best regards,
Krzysztof
> + ethernet-ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + pruss2_emac0: port@0 {
> + reg = <0>;
> + phy-handle = <&pruss2_eth0_phy>;
> + phy-mode = "rgmii-rxid";
That is unusual. Where are the TX delays coming from?
Andrew
On 23/12/2022 16:28, Andrew Lunn wrote:
>> + ethernet-ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + pruss2_emac0: port@0 {
>> + reg = <0>;
>> + phy-handle = <&pruss2_eth0_phy>;
>> + phy-mode = "rgmii-rxid";
>
> That is unusual. Where are the TX delays coming from?
From the below property
+ ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
The TX delay can be enabled/disabled from within the ICSSG block.
If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID
nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled.
This logic is in prueth_config_rgmiidelay() function in the introduced driver.
>
> Andrew
cheers,
-roger
On Mon, Jan 02, 2023 at 03:04:19PM +0200, Roger Quadros wrote:
>
>
> On 23/12/2022 16:28, Andrew Lunn wrote:
> >> + ethernet-ports {
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + pruss2_emac0: port@0 {
> >> + reg = <0>;
> >> + phy-handle = <&pruss2_eth0_phy>;
> >> + phy-mode = "rgmii-rxid";
> >
> > That is unusual. Where are the TX delays coming from?
>
> >From the below property
>
> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
>
> The TX delay can be enabled/disabled from within the ICSSG block.
>
> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID
> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled.
>
> This logic is in prueth_config_rgmiidelay() function in the introduced driver.
What nearly every other MAC driver does is pass the phy-mode to the
PHY and lets the PHY add the delays. I would recommend you do that,
rather than be special and different.
Andrew
On 02/01/2023 20:34, Andrew Lunn wrote:
> On Mon, Jan 02, 2023 at 03:04:19PM +0200, Roger Quadros wrote:
>>
>>
>> On 23/12/2022 16:28, Andrew Lunn wrote:
>>>> + ethernet-ports {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + pruss2_emac0: port@0 {
>>>> + reg = <0>;
>>>> + phy-handle = <&pruss2_eth0_phy>;
>>>> + phy-mode = "rgmii-rxid";
>>>
>>> That is unusual. Where are the TX delays coming from?
>>
>> >From the below property
>>
>> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
>>
>> The TX delay can be enabled/disabled from within the ICSSG block.
>>
>> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID
>> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled.
>>
>> This logic is in prueth_config_rgmiidelay() function in the introduced driver.
>
> What nearly every other MAC driver does is pass the phy-mode to the
> PHY and lets the PHY add the delays. I would recommend you do that,
> rather than be special and different.
If I remember right we couldn't disable MAC TX delay on some earlier silicon
so had to take this route. I don't remember why we couldn't disable it though.
In more recent Silicon Manuals I do see that MAC TX delay can be enabled/disabled.
If this really is the case then we should change to
phy-mode = "rgmii-id";
And let PHY handle the TX+RX delays.
Danish,
could you please make the change and test if it works on current silicon?
cheers,
-roger
> >> On 23/12/2022 16:28, Andrew Lunn wrote:
> >>>> + ethernet-ports {
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> + pruss2_emac0: port@0 {
> >>>> + reg = <0>;
> >>>> + phy-handle = <&pruss2_eth0_phy>;
> >>>> + phy-mode = "rgmii-rxid";
> >>>
> >>> That is unusual. Where are the TX delays coming from?
> >>
> >> >From the below property
> >>
> >> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
> >>
> >> The TX delay can be enabled/disabled from within the ICSSG block.
> >>
> >> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID
> >> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled.
> >>
> >> This logic is in prueth_config_rgmiidelay() function in the introduced driver.
> >
> > What nearly every other MAC driver does is pass the phy-mode to the
> > PHY and lets the PHY add the delays. I would recommend you do that,
> > rather than be special and different.
>
>
> If I remember right we couldn't disable MAC TX delay on some earlier silicon
> so had to take this route. I don't remember why we couldn't disable it though.
>
> In more recent Silicon Manuals I do see that MAC TX delay can be enabled/disabled.
> If this really is the case then we should change to
>
> phy-mode = "rgmii-id";
>
> And let PHY handle the TX+RX delays.
DT describes the board. PHY mode indicates what delays the board
requires, because the board itself is not performing the delays by
using extra long lines. So typically, phy-mode is rgmii-id, indicating
delays need to be added somewhere in both directions.
Who adds the delays is then between the MAC and the PHY. In most
cases, the MAC does nothing, and passes phy-mode to the PHY and the
PHY does it.
But it is also possible for the MAC to do the delay. So if you cannot
actually disable the TX delay in the MAC, that is O.K. But you need to
modify phy-mode you pass to the PHY to indicate the MAC is doing the
delay, otherwise the PHY will additionally do the delay. So your DT
will contain rgmii-id, because that is what the board requires, but
the MAC will pass rmgii-rxid to the PHY, since that is what the PHY
needs to add.
Andrew
On 05/01/2023 19:13, Andrew Lunn wrote:
>>>> On 23/12/2022 16:28, Andrew Lunn wrote:
>>>>>> + ethernet-ports {
>>>>>> + #address-cells = <1>;
>>>>>> + #size-cells = <0>;
>>>>>> + pruss2_emac0: port@0 {
>>>>>> + reg = <0>;
>>>>>> + phy-handle = <&pruss2_eth0_phy>;
>>>>>> + phy-mode = "rgmii-rxid";
>>>>>
>>>>> That is unusual. Where are the TX delays coming from?
>>>>
>>>> >From the below property
>>>>
>>>> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
>>>>
>>>> The TX delay can be enabled/disabled from within the ICSSG block.
>>>>
>>>> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID
>>>> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled.
>>>>
>>>> This logic is in prueth_config_rgmiidelay() function in the introduced driver.
>>>
>>> What nearly every other MAC driver does is pass the phy-mode to the
>>> PHY and lets the PHY add the delays. I would recommend you do that,
>>> rather than be special and different.
>>
>>
>> If I remember right we couldn't disable MAC TX delay on some earlier silicon
>> so had to take this route. I don't remember why we couldn't disable it though.
>>
>> In more recent Silicon Manuals I do see that MAC TX delay can be enabled/disabled.
>> If this really is the case then we should change to
>>
>> phy-mode = "rgmii-id";
>>
>> And let PHY handle the TX+RX delays.
>
> DT describes the board. PHY mode indicates what delays the board
> requires, because the board itself is not performing the delays by
> using extra long lines. So typically, phy-mode is rgmii-id, indicating
> delays need to be added somewhere in both directions.
>
> Who adds the delays is then between the MAC and the PHY. In most
> cases, the MAC does nothing, and passes phy-mode to the PHY and the
> PHY does it.
>
> But it is also possible for the MAC to do the delay. So if you cannot
> actually disable the TX delay in the MAC, that is O.K. But you need to
> modify phy-mode you pass to the PHY to indicate the MAC is doing the
> delay, otherwise the PHY will additionally do the delay. So your DT
> will contain rgmii-id, because that is what the board requires, but
> the MAC will pass rmgii-rxid to the PHY, since that is what the PHY
> needs to add.
Thanks for the explanation. :)
cheers,
-roger
Hi Roger,
On 05/01/23 17:03, Roger Quadros wrote:
> On 02/01/2023 20:34, Andrew Lunn wrote:
>> On Mon, Jan 02, 2023 at 03:04:19PM +0200, Roger Quadros wrote:
>>>
>>>
>>> On 23/12/2022 16:28, Andrew Lunn wrote:
>>>>> + ethernet-ports {
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> + pruss2_emac0: port@0 {
>>>>> + reg = <0>;
>>>>> + phy-handle = <&pruss2_eth0_phy>;
>>>>> + phy-mode = "rgmii-rxid";
>>>>
>>>> That is unusual. Where are the TX delays coming from?
>>>
>>> >From the below property
>>>
>>> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
>>>
>>> The TX delay can be enabled/disabled from within the ICSSG block.
>>>
>>> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID
>>> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled.
>>>
>>> This logic is in prueth_config_rgmiidelay() function in the introduced driver.
>>
>> What nearly every other MAC driver does is pass the phy-mode to the
>> PHY and lets the PHY add the delays. I would recommend you do that,
>> rather than be special and different.
>
>
> If I remember right we couldn't disable MAC TX delay on some earlier silicon
> so had to take this route. I don't remember why we couldn't disable it though.
>
> In more recent Silicon Manuals I do see that MAC TX delay can be enabled/disabled.
> If this really is the case then we should change to
>
> phy-mode = "rgmii-id";
>
> And let PHY handle the TX+RX delays.
>
> Danish,
> could you please make the change and test if it works on current silicon?
>
I changed the phy-mode to "rgmii-id" instead of "rgmii-rxid". I did the testing
on current silicon (AM654x SR 2.0) and it is working fine.
> cheers,
> -roger
Thanks and Regards,
Danish.
Hi Krzysztof,
On 23/12/22 17:01, Krzysztof Kozlowski wrote:
> On 23/12/2022 12:09, MD Danish Anwar wrote:
>> From: Puranjay Mohan <[email protected]>
>>
>> Add a YAML binding document for the ICSSG Programmable real time unit
>> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs
>> to interface the PRUs and load/run the firmware for supporting ethernet
>> functionality.
>>
>> Signed-off-by: Puranjay Mohan <[email protected]>
>> Signed-off-by: Md Danish Anwar <[email protected]>
>> ---
>> .../bindings/net/ti,icssg-prueth.yaml | 174 ++++++++++++++++++
>> 1 file changed, 174 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..7659f5fd3132
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>> @@ -0,0 +1,174 @@
>> +# 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
>
> Keep it in one line.
>
Sure, I'll do it.
>> +
>> +maintainers:
>> + - Puranjay Mohan <[email protected]>
>> + - Md Danish Anwar <[email protected]>
>> +
>> +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
>> +
>> + sram:
>> + description:
>> + phandle to MSMC SRAM node
>
> Where does the definition of this come from? If from nowhere, usually
> you need vendor prefix and type/ref.
>
It is phandle to sram node in SoC DT file. I'll change it to ti,sram as it is
TI specific. I'll also add $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> + 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:
>> + type: object
>> + properties:
>> + '#address-cells':
>> + const: 1
>> + '#size-cells':
>> + const: 0
>> +
>> + patternProperties:
>> + ^port@[0-1]$:
>> + type: object
>> + description: ICSSG PRUETH external ports
>> +
>> + $ref: ethernet-controller.yaml#
>> +
>> + unevaluatedProperties: false
>> + additionalProperties: false
>
> You cannot have both. Did you test the binding? I doubt it...
>
Sorry, must have missed it in testing. I will remove additionalProperties and
just keep unevaluatedProperties.
>> + properties:
>> + reg:
>> + items:
>> + - enum: [0, 1]
>> + description: ICSSG PRUETH port number
>> +
>> + ti,syscon-rgmii-delay:
>> + $ref: /schemas/types.yaml#/definitions/phandle-array
>> + description:
>> + phandle to system controller node and register offset
>> + to ICSSG control register for RGMII transmit delay
>
> You need to describe the items. And in your own bindings from TI you
> will even find example...
>
Sure. I'll describe the items for this.
>> +
>> + required:
>> + - reg
>> +
>> + 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
>
> Why do you have so many phandles? Aren't you missing some phys?
>
That is because control bits were sprinkled around System Control register so
we need to use syscon regmap to access them.
>> +
>> + interrupts:
>> + minItems: 2
>
> Drop minItems
>
Sure. I'll drop minItems.
>> + maxItems: 2
>> + description: |
>> + Interrupt specifiers to TX timestamp IRQ.
>> +
>> + interrupt-names:
>> + items:
>> + - const: tx_ts0
>> + - const: tx_ts1
>> +
>> +required:
>> + - compatible
>> + - sram
>> + - ti,mii-g-rt
>
> Keep same order as in properties:.
>
Sure, I will make sure it has same order as in properties.
>> + - dmas
>> + - dma-names
>> + - ethernet-ports
>> + - interrupts
>> + - interrupt-names
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> + - |
>> +
>
> No need for blank line.
>
I'll remove this blank line.
>> + /* Example k3-am654 base board SR2.0, dual-emac */
>> + pruss2_eth: pruss2_eth {
>
> No underscores in node names.
>
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
I will change the node name 'pruss2_eth' to generic for example 'ethernet'.
>> + compatible = "ti,am654-icssg-prueth";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&icssg2_rgmii_pins_default>;
>> + 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";
>
> I really doubt it was tested... and maybe there will be no testing from
> Rob's bot due to dependency.
>
> Please post dt_binding_check testing results.
>
I had run dt_binding check before posting, must have missed these errors. I
will be careful to check for this errors before posting patches.
I will post new revision after doing the above mentioned changes. I will make
sure to do testing properly.
> Best regards,
> Krzysztof
>
--
Thanks and Regards,
Danish.