Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221031200114.987024-1-robertcnelson@gmail.com>
 <20221031200114.987024-2-robertcnelson@gmail.com> <ce9291a0-e138-f0ec-9604-83abd93e72c3@ti.com>
In-Reply-To: <ce9291a0-e138-f0ec-9604-83abd93e72c3@ti.com>
From:   Robert Nelson <robertcnelson@gmail.com>
Date:   Mon, 14 Nov 2022 15:53:46 -0600
Message-ID: <CAOCHtYi-aBwxY3Hivgj+uW71E3XT6q6j0=SfWg+EfJOqQB3_SA@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] arm64: dts: ti: add k3-j721e-beagleboneai64
To:     Andrew Davis <afd@ti.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, Nishanth Menon <nm@ti.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Tero Kristo <kristo@kernel.org>,
        Jason Kridner <jkridner@beagleboard.org>,
        Drew Fustini <drew@beagleboard.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks Andrew, sorry for the delay!

On Wed, Nov 2, 2022 at 11:16 AM Andrew Davis <afd@ti.com> wrote:
>
> On 10/31/22 3:01 PM, Robert Nelson wrote:
> > BeagleBoard.org BeagleBone AI-64 is an open source hardware single
> > board computer based on the Texas Instruments TDA4VM SoC featuring
> > dual-core 2.0GHz Arm Cortex-A72 processor, C7x+MMA and 2 C66x
> > floating-point VLIW DSPs, 3x dual Arm Cortex-R5 co-processors,
> > 2x 6-core Programmable Real-Time Unit and Industrial Communication
> > SubSystem, PowerVR Rogue 8XE GE8430 3D GPU. The board features 4GB
> > DDR4, USB3.0 Type-C, 2x USB SS Type-A, miniDisplayPort, 2x 4-lane
> > CSI, DSI, 16GB eMMC flash, 1G Ethernet, M.2 E-key for WiFi/BT, and
> > BeagleBone expansion headers.
> >
> > This board family can be indentified by the BBONEAI-64-B0 in the
> > at24 eeprom:
> >
> > [aa 55 33 ee 01 37 00 10  2e 00 42 42 4f 4e 45 41 |.U3..7....BBONEA|]
> > [49 2d 36 34 2d 42 30 2d  00 00 42 30 30 30 37 38 |I-64-B0-..B00078|]
> >
> > https://beagleboard.org/ai-64
> > https://git.beagleboard.org/beagleboard/beaglebone-ai-64

> > +
> > +     reserved_memory: reserved-memory {
> > +             #address-cells = <2>;
> > +             #size-cells = <2>;
> > +             ranges;
> > +
> > +             secure_ddr: optee@9e800000 {
> > +                     reg = <0x00 0x9e800000 0x00 0x01800000>;
> > +                     alignment = <0x1000>;
>
> "alignment" property should not be needed here since you cannot
> allocate from this region anyway.

I removed alignment, wondering if these all should be eventually moved
into the common file, with custom applications just updating the
offset's.

> > +
> > +     gpio_keys: gpio-keys {
> > +             compatible = "gpio-keys";
> > +             autorepeat;
>
> Do you need "autorepeat" on these?
>

Removed, no idea where that came from...

> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&sw_pwr_pins_default>;
> > +
> > +             sw_boot: switch-1 {
>
> I don't see anyone referencing these nodes, the "sw_boot" shouldn't be needed.
>
> > +                     label = "BOOT";
> > +                     linux,code = <BTN_0>;
> > +                     gpios = <&wkup_gpio0 0 GPIO_ACTIVE_LOW>;
> > +             };
> > +
> > +             sw_pwr: switch-2 {
>
> NIT (no need to actually change this),
> Would "button"-1/2 be better here, I see on the silkscreen has them as "sw"
> but most would call these buttons if they saw them.

I do like the 'button' label much better, nothing uses these, so i
removed the label name.

>
> > +                     label = "POWER";
> > +                     linux,code = <KEY_POWER>;
> > +                     gpios = <&wkup_gpio0 4 GPIO_ACTIVE_LOW>;
> > +             };
> > +     };
>
> [...]
>
> > +
> > +&main_sdhci2 {
> > +     /* Unused */
> > +     status = "disabled";
> > +};
> > +
>
> For J7x I did not "disable by default" several classes of device
> like this one, since the default pinmux may allow their function.
> Once that is sorted out I'll fix up this DT here and in the spots
> below for you.
>
> BTW, thanks for taking the time to rebase on my series for the
> devices I did disable. Hope that didn't cause too much churn
> on your side :)

I love it!  I prefer everything disabled, and just enable what nodes we need.

> > +
> > +&main_r5fss0_core0 {
> > +     firmware-name = "pdk-ipc/ipc_echo_test_mcu2_0_release_strip.xer5f";
> > +};
> > +
>
> What is this crazy firmware name? These are not in linux-firmware, might
> be better to leave these out until we get these names sorted out and
> upstreamed. (yes I know the same snuck into k3-j721e-sk.dtb but
> it probably isn't correct there either).

Yeap, direct copy from k3-j721e-sk, I'll remove it till we get
everything sorted out..

>
> > +
> > +&mcu_cpsw {
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&mcu_cpsw_pins_default &mcu_mdio_pins_default>;
>
> mcu_mdio_pins_default should belong to the MDIO node below.
>
> > +};
> > +
> > +&davinci_mdio {
>
>
> Right here.
>
> pinctrl-names = "default";
> pinctrl-0 = <&mcu_mdio_pins_default>;

and moved!


> Everything else looks sane enough to me,
>
> Reviewed-by: Andrew Davis <afd@ti.com>

Thanks!

-- 
Robert Nelson
https://rcn-ee.com/