Message-ID: <f1fe8dab-350d-002e-c922-58e5912bd76f@ti.com>
Date:   Tue, 19 Sep 2023 10:27:33 -0500
From:   Andrew Davis <afd@...com>
To:     MD Danish Anwar <danishanwar@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Nishanth Menon <nm@...com>
CC:     Conor Dooley <conor+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Tero Kristo <kristo@...nel.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <srk@...com>,
        <r-gunasekaran@...com>
Subject: Re: [PATCH 2/3] arm64: dts: ti: am654-base-board: add ICSSG2 Ethernet
 support

On 9/19/23 1:52 AM, MD Danish Anwar wrote:
> On 15/09/23 21:36, Andrew Davis wrote:
>> On 9/13/23 1:05 AM, MD Danish Anwar wrote:
>>> On 12/09/23 20:28, Andrew Davis wrote:
>>>> On 9/12/23 3:29 AM, MD Danish Anwar wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> On 11/09/23 18:35, Andrew Davis wrote:
>>>>>> On 9/11/23 2:12 AM, MD Danish Anwar wrote:
>>>>>>> ICSSG2 provides dual Gigabit Ethernet support.
>>>>>>>
>>>>>>> For support SR2.0 ICSSG Ethernet firmware:
>>>>>>> - provide different firmware blobs and use TX_PRU.
>>>>>>> - IEP0 is used as PTP Hardware Clock and can only be used for one
>>>>>>> port.
>>>>>>> - TX timestamp notification comes via INTC interrupt.
>>>>>>>
>>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>>>>>>> ---
>>>>>>>      .../arm64/boot/dts/ti/k3-am654-base-board.dts | 123
>>>>>>> ++++++++++++++++++
>>>>>>
>>>>>> Adding this to the base dts? What if I want to use my PRUs for
>>>>>> something
>>>>>> else? These "application nodes" define a single usecase out of many
>>>>>> possible, and should IMHO always be in overlays so users can select
>>>>>> which
>>>>>> they want easily.
>>>>>>
>>>>>
>>>>> The base board (AM654x-EVM) has two Ethernet ports dedicated for ICSSG.
>>>>> The expectation is that when a user boots up AM654x-EVM, ICSSG is
>>>>> supported on those two ports by default. If the icssg nodes are not
>>>>> added to base dts then by default the two Ethernet ports will have no
>>>>> functionality.
>>>>>
>>>>
>>>> This is *your* default use-case for these PRUs, mine might be different.
>>>> I can agree that most might want this use-case too and this is the one
>>>> intended as the demo for these ports on this board. What I am saying is
>>>> that when one wants to use these PRUs for something else, having this
>>>> one application baked into the base DTB makes it very difficult to
>>>> switch.
>>>>
>>>
>>> This is the intended use case. The base board has two ICSSG Ethernet
>>> ports. My understanding is that device trees describe the hardware.
>>>
>>
>> Correct, but you are not describing hardware here, you are describing
>> software. Yes it is software that uses hardware so you are listing a
>> bunch of hardware too, but the core is a firmware.
>>
>>> The base board dts should describe the base board hardware which has the
>>> two ICSSG ports. So the base board dts should contain nodes to enable
>>> those two ports.
>>>
>>> Any hardware component that is not present in the base board should be
>>> applied as an overlay.
>>>
>>
>> Correct again, the firmware is not baked into the base board, that is
>> loaded by U-Boot/Linux at runtime and can be selected.
>>
>>> For example in the AM654x-IDK, we have extra IDK card applied on the
>>> base board. This IDK card contains 4 ICSSG Ethernet ports. The nodes for
>>> these 4 ICSSG port should go in an overlay i.e. k3-am654-idk.dtso which
>>> I am doing as part of the patch 3 of this series.
>>>
>>> My understanding is that any hardware component that is part of the base
>>> board should be described in base-board.dts. Any hardware component that
>>> is not part of the base board and is added by extra cards should be
>>> described in overlays.
>>>
>>>>> The primary use case is that ICSSG should support on those two ports in
>>>>> AM654x-EVM by default. The user should not need to apply any overlay to
>>>>> get the two ports working. So In order to achieve that I think it is OK
>>>>> to add the ICSSG nodes in the base board dts file.
>>>>>
>>>>
>>>> A user does not need to apply an overlay to use these, this application
>>>> node overlay can be applied at build time. You can even rename the base
>>>> .dtb to be the one that has this overlay applied by default.
>>>>
>>>> Take a look at k3-am654-gp-evm.dtb, it is a composite DTB built from
>>>> the "base-board" DTB and the "rocktech-rk101-panel" DTBO applied on top.
>>>> This combination is what we call and sell as the "GP EVM", and you
>>>> can use it by booting the "k3-am654-gp-evm.dtb". Now let's say you
>>>> want to use a different panel, all you need to do is take the base-
>>>> board and apply a different panel overlay. Had we hard-coded the
>>>> "default" panel into the base-board DTS then a user with a different
>>>> panel would have to go and edit the DTS to remove all the rk101-panel
>>>> bits.
>>>>
>>>
>>> This is one way to do it. But I still think the best way to do this is
>>> to have the ICSSG nodes in base board dts as the ICSSG hardware is
>>> present on the base board.
>>>
>>
>> So again, you are not describing the hardware, you are describing a
>> *use of* the hardware. This node describes what firmware to load and
>> what bits of hardware that firmware should use to get some end result,
>> but I could just as easily use a different firmware and give it different
>> links to different hardware bits and make it into something else. No
>> physical changes to the hardware needed.
>>
>>>>> If user wants to use PRUs for something else, we can have overlay for
>>>>> those. But we should not need to apply any overlay to achieve the
>>>>> primary functionality i.e. ICSSG working in the two dedicated ICSSG
>>>>> Ethernet ports.
>>>>>
>>>>
>>>> They could *not* simply add a different overlay for their usecase as
>>>> you have baked your usecase into the base DTB. Their overlay would
>>>> have to have a bunch of /delete-node/ junk to remove your "defaults".
>>>>
>>>
>>> This patch adds one node "icssg2-eth" which uses six PRUs. If user wants
>>> to use PRUs for something else they can add "/delete-node/ &icssg2_eth"
>>> in the overlay.
>>>
>>
>> /delete-node/ is usually an indication some layering was done wrong,
>> it shouldn't be needed in most cases to delete nodes. And that is
>> my point here, I don't want to have to delete your use-case in
>> every overlay file that uses the PRUs differently than you. Your
>> use-case should simply be an overlay too, then all I have to do is
>> apply my overlay instead of yours without deletes.
>>
> 
> Sure I understand this. But my expectation is as soon as you boot gp-evm
> or base-board, you should be able to see the two ICSSG ports and they
> should be working properly.
> 
> If I move the ICSSG2 nodes from k3-am654-base-board.dtb to some overlay
> and make k3-am654-gp-evm-dtb to have this overlay applied by default
> using below then it will require to change u-boot as well.
> 
> The only reason I am adding these ICSSG nodes to k3-am654-base-board.dtb
> is because k3-am654-base-board.dtb is used by default to boot the board.

Seems we are making an ABI around the names of dtb files, guess that
has always been kinda true.

> If I move ICSSG2 nodes to some dtbo and generate k3-am654-gp-evm-dtb
> with that dtbo then we will need to use k3-am654-gp-evm-dtb as default
> while booting AM65x GP EVM
> 
> diff --git a/board/ti/am65x/am65x.env b/board/ti/am65x/am65x.env
> index 755bff2707..f9249cb7f2 100644
> --- a/board/ti/am65x/am65x.env
> +++ b/board/ti/am65x/am65x.env
> @@ -6,7 +6,7 @@
>   #endif
> 
>   findfdt=
> -       setenv name_fdt ti/k3-am654-base-board.dtb;
> +       setenv name_fdt ti/k3-am654-gp-evm-dtb;
>          setenv fdtfile ${name_fdt}
>   name_kern=Image
>   console=ttyS2,115200n8
> 
> If this is okay with you, I can go ahead and move ICSSG2 nodes to some
> overlay.
> 

That is fine with me, booting the raw -base-board dtb by default
in u-boot always seemed wrong to me anyway.

Another option is to rename the dts file and have the file called
k3-am654-base-board.dtb be made from that + this new overlay,
that way no change is needed in u-boot.

Andrew

> The ICSSG2 node will be part of k3-am654-icssg2.dtso
> The ICSSG0 and ICSSG1 nodes will be part of k3-am654-idk.dtso
> 
> Let me know if this looks OK to you.
> 
>> Andrew
>>
>>>> As above, if this is the "primary functionality" then have this
>>>> overlay applied by default:
>>>>
>>>> --- a/arch/arm64/boot/dts/ti/Makefile
>>>> +++ b/arch/arm64/boot/dts/ti/Makefile
>>>> @@ -42,7 +42,7 @@ dtb-$(CONFIG_ARCH_K3) +=
>>>> k3-am642-tqma64xxl-mbax4xxl-sdcard.dtb
>>>>    dtb-$(CONFIG_ARCH_K3) += k3-am642-tqma64xxl-mbax4xxl-wlan.dtb
>>>>      # Boards with AM65x SoC
>>>> -k3-am654-gp-evm-dtbs := k3-am654-base-board.dtb
>>>> k3-am654-base-board-rocktech-rk101-panel.dtbo
>>>> +k3-am654-gp-evm-dtbs := k3-am654-base-board.dtb
>>>> k3-am654-base-board-rocktech-rk101-panel.dtbo
>>>> k3-am654-base-board-prueth.dtbo
>>>>    dtb-$(CONFIG_ARCH_K3) += k3-am6528-iot2050-basic.dtb
>>>>    dtb-$(CONFIG_ARCH_K3) += k3-am6528-iot2050-basic-pg2.dtb
>>>>    dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced.dtb
>>>>
>>>> Andrew
>>>>
>>>>>
>>>>>> Andrew
>>>>>>
>>>>>>>      1 file changed, 123 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
>>>>>>> b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
>>>>>>> index f5c26e9fba98..5cf9546ff9f7 100644
>>>>>>> --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
>>>>>>> +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
>>>>>>> @@ -25,6 +25,8 @@ aliases {
>>>>>>>              ethernet0 = &cpsw_port1;
>>>>>>>              mmc0 = &sdhci0;
>>>>>>>              mmc1 = &sdhci1;
>>>>>>> +        ethernet1 = &icssg2_emac0;
>>>>>>> +        ethernet2 = &icssg2_emac1;
>>>>>>>          };
>>>>>>>            chosen {
>>>>>>> @@ -144,6 +146,72 @@ vtt_supply: regulator-3 {
>>>>>>>              vin-supply = <&vcc3v3_io>;
>>>>>>>              gpio = <&wkup_gpio0 28 GPIO_ACTIVE_HIGH>;
>>>>>>>          };
>>>>>>> +
>>>>>>> +    /* Dual Ethernet application node on PRU-ICSSG2 */
>>>>>>> +    icssg2_eth: icssg2-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-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 = <&icssg2_mii_g_rt>;
>>>>>>> +        ti,mii-rt = <&icssg2_mii_rt>;
>>>>>>> +        ti,iep = <&icssg2_iep0>, <&icssg2_iep1>;
>>>>>>> +
>>>>>>> +        interrupt-parent = <&icssg2_intc>;
>>>>>>> +        interrupts = <24 0 2>, <25 1 3>;
>>>>>>> +        interrupt-names = "tx_ts0", "tx_ts1";
>>>>>>> +
>>>>>>> +        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";
>>>>>>> +        ethernet-ports {
>>>>>>> +            #address-cells = <1>;
>>>>>>> +            #size-cells = <0>;
>>>>>>> +            icssg2_emac0: port@0 {
>>>>>>> +                reg = <0>;
>>>>>>> +                phy-handle = <&icssg2_phy0>;
>>>>>>> +                phy-mode = "rgmii-id";
>>>>>>> +                ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
>>>>>>> +                /* Filled in by bootloader */
>>>>>>> +                local-mac-address = [00 00 00 00 00 00];
>>>>>>> +            };
>>>>>>> +            icssg2_emac1: port@1 {
>>>>>>> +                reg = <1>;
>>>>>>> +                phy-handle = <&icssg2_phy1>;
>>>>>>> +                phy-mode = "rgmii-id";
>>>>>>> +                ti,syscon-rgmii-delay = <&scm_conf 0x4124>;
>>>>>>> +                /* Filled in by bootloader */
>>>>>>> +                local-mac-address = [00 00 00 00 00 00];
>>>>>>> +            };
>>>>>>> +        };
>>>>>>> +    };
>>>>>>>      };
>>>>>>>        &wkup_pmx0 {
>>>>>>> @@ -300,6 +368,43 @@ usb1_pins_default: usb1-default-pins {
>>>>>>>                  AM65X_IOPAD(0x02c0, PIN_OUTPUT, 0) /* (AC8)
>>>>>>> USB1_DRVVBUS */
>>>>>>>              >;
>>>>>>>          };
>>>>>>> +
>>>>>>> +    icssg2_mdio_pins_default: icssg2-mdio-default-pins {
>>>>>>> +        pinctrl-single,pins = <
>>>>>>> +            AM65X_IOPAD(0x0094, PIN_INPUT, 2) /* (AC19)
>>>>>>> PRG2_PRU0_GPO7.PRG2_MDIO0_MDIO */
>>>>>>> +            AM65X_IOPAD(0x00c8, PIN_OUTPUT, 2) /* (AE15)
>>>>>>> PRG2_PRU1_GPO7.PRG2_MDIO0_MDC */
>>>>>>> +        >;
>>>>>>> +    };
>>>>>>> +
>>>>>>> +    icssg2_rgmii_pins_default: icssg2-rgmii-default-pins {
>>>>>>> +        pinctrl-single,pins = <
>>>>>>> +            AM65X_IOPAD(0x00ac, PIN_INPUT, 2) /* (AH15)
>>>>>>> PRG2_PRU1_GPO0.PRG2_RGMII2_RD0 */
>>>>>>> +            AM65X_IOPAD(0x00b0, PIN_INPUT, 2) /* (AC16)
>>>>>>> PRG2_PRU1_GPO1.PRG2_RGMII2_RD1 */
>>>>>>> +            AM65X_IOPAD(0x00b4, PIN_INPUT, 2) /* (AD17)
>>>>>>> PRG2_PRU1_GPO2.PRG2_RGMII2_RD2 */
>>>>>>> +            AM65X_IOPAD(0x00b8, PIN_INPUT, 2) /* (AH14)
>>>>>>> PRG2_PRU1_GPO3.PRG2_RGMII2_RD3 */
>>>>>>> +            AM65X_IOPAD(0x00cc, PIN_OUTPUT, 2) /* (AD15)
>>>>>>> PRG2_PRU1_GPO8.PRG2_RGMII2_TD0 */
>>>>>>> +            AM65X_IOPAD(0x00d0, PIN_OUTPUT, 2) /* (AF14)
>>>>>>> PRG2_PRU1_GPO9.PRG2_RGMII2_TD1 */
>>>>>>> +            AM65X_IOPAD(0x00d4, PIN_OUTPUT, 2) /* (AC15)
>>>>>>> PRG2_PRU1_GPO10.PRG2_RGMII2_TD2 */
>>>>>>> +            AM65X_IOPAD(0x00d8, PIN_OUTPUT, 2) /* (AD14)
>>>>>>> PRG2_PRU1_GPO11.PRG2_RGMII2_TD3 */
>>>>>>> +            AM65X_IOPAD(0x00dc, PIN_INPUT, 2) /* (AE14)
>>>>>>> PRG2_PRU1_GPO16.PRG2_RGMII2_TXC */
>>>>>>> +            AM65X_IOPAD(0x00c4, PIN_OUTPUT, 2) /* (AC17)
>>>>>>> PRG2_PRU1_GPO6.PRG2_RGMII2_TX_CTL */
>>>>>>> +            AM65X_IOPAD(0x00c0, PIN_INPUT, 2) /* (AG15)
>>>>>>> PRG2_PRU1_GPO5.PRG2_RGMII2_RXC */
>>>>>>> +            AM65X_IOPAD(0x00bc, PIN_INPUT, 2) /* (AG14)
>>>>>>> PRG2_PRU1_GPO4.PRG2_RGMII2_RX_CTL */
>>>>>>> +
>>>>>>> +            AM65X_IOPAD(0x0078, PIN_INPUT, 2) /* (AF18)
>>>>>>> PRG2_PRU0_GPO0.PRG2_RGMII1_RD0 */
>>>>>>> +            AM65X_IOPAD(0x007c, PIN_INPUT, 2) /* (AE18)
>>>>>>> PRG2_PRU0_GPO1.PRG2_RGMII1_RD1 */
>>>>>>> +            AM65X_IOPAD(0x0080, PIN_INPUT, 2) /* (AH17)
>>>>>>> PRG2_PRU0_GPO2.PRG2_RGMII1_RD2 */
>>>>>>> +            AM65X_IOPAD(0x0084, PIN_INPUT, 2) /* (AG18)
>>>>>>> PRG2_PRU0_GPO3.PRG2_RGMII1_RD3 */
>>>>>>> +            AM65X_IOPAD(0x0098, PIN_OUTPUT, 2) /* (AH16)
>>>>>>> PRG2_PRU0_GPO8.PRG2_RGMII1_TD0 */
>>>>>>> +            AM65X_IOPAD(0x009c, PIN_OUTPUT, 2) /* (AG16)
>>>>>>> PRG2_PRU0_GPO9.PRG2_RGMII1_TD1 */
>>>>>>> +            AM65X_IOPAD(0x00a0, PIN_OUTPUT, 2) /* (AF16)
>>>>>>> PRG2_PRU0_GPO10.PRG2_RGMII1_TD2 */
>>>>>>> +            AM65X_IOPAD(0x00a4, PIN_OUTPUT, 2) /* (AE16)
>>>>>>> PRG2_PRU0_GPO11.PRG2_RGMII1_TD3 */
>>>>>>> +            AM65X_IOPAD(0x00a8, PIN_INPUT, 2) /* (AD16)
>>>>>>> PRG2_PRU0_GPO16.PRG2_RGMII1_TXC */
>>>>>>> +            AM65X_IOPAD(0x0090, PIN_OUTPUT, 2) /* (AE17)
>>>>>>> PRG2_PRU0_GPO6.PRG2_RGMII1_TX_CTL */
>>>>>>> +            AM65X_IOPAD(0x008c, PIN_INPUT, 2) /* (AF17)
>>>>>>> PRG2_PRU0_GPO5.PRG2_RGMII1_RXC */
>>>>>>> +            AM65X_IOPAD(0x0088, PIN_INPUT, 2) /* (AG17)
>>>>>>> PRG2_PRU0_GPO4.PRG2_RGMII1_RX_CTL */
>>>>>>> +        >;
>>>>>>> +    };
>>>>>>>      };
>>>>>>>        &main_pmx1 {
>>>>>>> @@ -621,3 +726,21 @@ &cpsw_port1 {
>>>>>>>      &dss {
>>>>>>>          status = "disabled";
>>>>>>>      };
>>>>>>> +
>>>>>>> +&icssg2_mdio {
>>>>>>> +    status = "okay";
>>>>>>> +    pinctrl-names = "default";
>>>>>>> +    pinctrl-0 = <&icssg2_mdio_pins_default>;
>>>>>>> +
>>>>>>> +    icssg2_phy0: ethernet-phy@0 {
>>>>>>> +        reg = <0>;
>>>>>>> +        ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
>>>>>>> +        ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
>>>>>>> +    };
>>>>>>> +
>>>>>>> +    icssg2_phy1: ethernet-phy@3 {
>>>>>>> +        reg = <3>;
>>>>>>> +        ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
>>>>>>> +        ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
>>>>>>> +    };
>>>>>>> +};
>>>>>
>>>
> 

Powered by blists - more mailing lists