Date:   Mon, 13 Mar 2023 14:30:37 -0500
From:   Nishanth Menon <nm@ti.com>
To:     Andrew Davis <afd@ti.com>
CC:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>, <linux-kernel@vger.kernel.org>,
        <devicetree@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        Tero Kristo <kristo@kernel.org>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Julien Panis <jpanis@baylibre.com>, Bryan Brattlof <bb@ti.com>,
        Jason Kridner <jkridner@gmail.com>,
        Robert Nelson <robertcnelson@gmail.com>
Subject: Re: [PATCH 2/3] arm64: dts: ti: Add k3-am625-beagleplay
Message-ID: <20230313193037.55l5sj3htr3mcnsn@spinner>
References: <20230311111022.23717-1-nm@ti.com>
 <20230311111022.23717-3-nm@ti.com>
 <da7555ae-b56a-f895-96cc-447bfd1c772d@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <da7555ae-b56a-f895-96cc-447bfd1c772d@ti.com>
Precedence: bulk

On 12:50-20230313, Andrew Davis wrote:
> On 3/11/23 5:10 AM, Nishanth Menon wrote:
> > From: Robert Nelson <robertcnelson@gmail.com>
> > 
[...]

> > +		serial1 = &mcu_uart0;
> > +		serial2 = &main_uart0;
> > +		serial3 = &main_uart5;
> 
> What are we using main_uart5 for, and why does it need to be serial3?

As provided in the documentation (referred to in the dts and commit
message), Mikrobus, serial3 since that is the 3rd of the uart
interfaces exposed on the platform.

[...]

> > +		secure_tfa_ddr: tfa@9e780000 {
> > +			reg = <0x00 0x9e780000 0x00 0x80000>;
> > +			alignment = <0x1000>;
> 
> "alignment" not needed since we cannot allocate from "no-map" regions anyway. Same
> for OP-TEE mem below.

Aargh.. yes... I missed it again.
> 
> > +			no-map;
> > +		};
> > +
> > +		secure_ddr: optee@9e800000 {
> > +			reg = <0x00 0x9e800000 0x00 0x01800000>; /* for OP-TEE */
> 
> "for OP-TEE" comment is probably extra now that the node is named "optee".

Will drop.

[...]
> > +&a53_opp_table {
> > +		/* Requires VDD_CORE to be at 0.85V */
> > +		opp-1400000000 {
> > +			opp-hz = /bits/ 64 <1400000000>;
> > +			opp-supported-hw = <0x01 0x0004>;
> > +		};
> 
> Seems tabed out too far.

Uggh.. will check and fix.

> 
> > +};
> > +
> > +&wkup_i2c0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c_csi_pins_default>;
> > +	clock-frequency = <400000>;
> > +	/* Enable with overlay for camera sensor */
> 
> If we don't want to enable it here, why not move all this to the overlay?

Makes no sense in duplicating (and would be error prone). The pinmux
is always constant, no matter which sensor overlay one uses. So,
maintaining the common configuration in the board dts itself.

[...]

> > +&cpsw3g_mdio {
> > +	/* Workaround for errata i2329 - Use mdio bitbang */
> > +	status = "disabled";
> 
> Should already be disabled, but the comment is nice to have so
> probably okay to keep IMHO.

Yeah - wanted to be a bit explicit here.

Thanks for reviewing..

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D