Message-ID: <20250703122008.ygz5udttjdo3l2g4@bryanbrattlof.com>
Date: Thu, 3 Jul 2025 07:20:08 -0500
From: Bryan Brattlof <bb@...com>
To: Paresh Bhagat <p-bhagat@...com>
CC: <nm@...com>, <vigneshr@...com>, <praneeth@...com>, <kristo@...nel.org>,
        <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <khasim@...com>, <v-singh1@...com>,
        <afd@...com>, <devarsht@...com>, <s-vadapalli@...com>,
        <andrew@...n.ch>
Subject: Re: [PATCH v5 4/4] arm64: dts: ti: Add support for AM62D2-EVM

On July  3, 2025 thus sayeth Paresh Bhagat:
> Hi Bryan,
> 
> 
> On 01/07/25 21:55, Bryan Brattlof wrote:
> > On June 27, 2025 thus sayeth Paresh Bhagat:
> > > AM62D-EVM evaluation module (EVM) is a low-cost expandable platform board
> > > designed for AM62D2 SoC from TI. It supports the following interfaces:
> > > 
> > > * 4 GB LPDDR4 RAM
> > > * x2 Gigabit Ethernet expansion connectors
> > > * x4 3.5mm TRS Audio Jack Line In
> > > * x4 3.5mm TRS Audio Jack Line Out
> > > * x2 Audio expansion connectors
> > > * x1 Type-A USB 2.0, x1 Type-C dual-role device (DRD) USB 2.0
> > > * x1 UHS-1 capable micro SD card slot
> > > * 32 GB eMMC Flash
> > > * 512 Mb OSPI NOR flash
> > > * x4 UARTs via USB 2.0-B
> > > * XDS110 for onboard JTAG debug using USB
> > > * Temperature sensors, user push buttons and LEDs
> > > 
> > > Although AM62D2 and AM62A7 differ in peripheral capabilities example
> > > multimedia, VPAC, and display subsystems, the core architecture remains
> > > same. To reduce duplication, AM62D support reuses the AM62A dtsi and the
> > > necessary overrides will be handled in SOC specific dtsi file and a
> > > board specific dts.
> > > 
> > > Add basic support for AM62D2-EVM.
> > > 
> > > Schematics Link - https://www.ti.com/lit/zip/sprcal5
> > > 
> > > Signed-off-by: Paresh Bhagat <p-bhagat@...com>
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@...com>
> > > ---
> > >   arch/arm64/boot/dts/ti/Makefile          |   3 +
> > >   arch/arm64/boot/dts/ti/k3-am62d2-evm.dts | 599 +++++++++++++++++++++++
> > >   arch/arm64/boot/dts/ti/k3-am62d2.dtsi    |  25 +
> > >   3 files changed, 627 insertions(+)
> > >   create mode 100644 arch/arm64/boot/dts/ti/k3-am62d2-evm.dts
> > >   create mode 100644 arch/arm64/boot/dts/ti/k3-am62d2.dtsi
> > > 
> > ...
> > 
> > > diff --git a/arch/arm64/boot/dts/ti/k3-am62d2.dtsi
> > > b/arch/arm64/boot/dts/ti/k3-am62d2.dtsi
> > > new file mode 100644
> > > index 000000000000..70aeb40872a9
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/ti/k3-am62d2.dtsi
> > > @@ -0,0 +1,25 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> > > +/*
> > > + * Device Tree Source for AM62D2 SoC family in Quad core configuration
> > > + *
> > > + * TRM: https://www.ti.com/lit/pdf/sprujd4
> > > + *
> > > + * Copyright (C) 2025 Texas Instruments Incorporated - https://www.ti.com/
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "k3-am62a7.dtsi"
> > If we want to reuse the AM62A chassis I think we should probably reused
> > the AM62AX_IOPAD() macro instead of creating a new one.
> 
> 
> AM62D does not necessarily have the same pin configuration compared to
> AM62A. While it is a macro which could be shareable across many boards, i
> think its preferable we maintain separate definitions to highlight the new
> SoCs. AM62D is a separate package, with some components reused from AM62a.

I guess I don't understand the need to create a new padconfig macro when 
we say, in device tree syntax, the AM62D and AM62A uses the same RTL 
chassis. The pinout will always change with packaging changes but this 
will not change the padconfig MMR layout.

All that said. It's just a name and honestly when you look at all these 
macros we haven't changed the padconfig layout for any K3 chip so it not 
a big deal to me. If it helps people when grepping around i'll relent ;)

> 
> 
> > 
> > > +
> > > +/ {
> > > +	model = "Texas Instruments K3 AM62D SoC";
> > > +	compatible = "ti,am62d2";
> > > +};
> > > +
> > > +&vpu {
> > > +	status = "disabled";
> > > +};
> > > +
> > > +&e5010 {
> > > +	status = "disabled";
> > > +};
> > So I could be a little out of date on the style guidelines here, but my
> > intuition is device trees, much like real trees, can only grow, so we
> > can't inherit the am62a.dtsi and remove things.
> > 
> > My understanding is we have to create a full am62d.dtsi with its
> > features that the am62a.dtsi can extend with the vpu{} and e5010{} nodes
> > 
> > ~Bryan
> 
> 
> Agree we should ideally keep the device trees extending. But in this case it
> will involve changes not only in am62a.dtsi but ideally it will change
> k3-am62a-main.dtsi and k3-am62a-mcu.dtsi as well. This moves us back to
> version 3 of this series
> https://lore.kernel.org/all/20250508091422.288876-1-p-bhagat@ti.com/ where i
> created *common*.dtsi files which looks a bit complex.
> 
> 
> The current method also ensures that customers can start their development
> of a new board with k3-am62d2.dtsi, while maintaining less complexity and is
> a easier to follow approach.

The issue I take with this approach is what does 'status = "disabled"' 
mean now. Historically (for TI SoCs at least) it indicated the node was 
incomplete and would need to be extended in the board.dts to function 
properly. But now we're trying to say for these two nodes the hardware 
doesn't exist on this SoC and bad things will happen if you enable them.

My recommendation is to try to flip this around. The am62a7.dtsi should 
inherit the am62d2.dtsi and add the vpu{} and e5010{} nodes. I agree we 
don't need to try to combine the two as we did in v3 just yet but we 
should try to keep the device trees growing as we inherit things.

~Bryan

> 
> 

Powered by blists - more mailing lists