| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d2ad2519-9c3c-1d93-e8ee-adfeea2b416a@ti.com>
Date: Tue, 19 Sep 2023 12:22:18 +0530
From: MD Danish Anwar <danishanwar@...com>
To: Andrew Davis <afd@...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 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.
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.
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>;
>>>>>> + };
>>>>>> +};
>>>>
>>
--
Thanks and Regards,
Danish
Powered by blists - more mailing lists