Message-ID: <20240731151820.tf6tzgj2wrc5vh5j@uda0497581>
Date: Wed, 31 Jul 2024 20:48:20 +0530
From: Manorit Chawdhry <m-chawdhry@...com>
To: Andrew Davis <afd@...com>
CC: Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
        Tero
 Kristo <kristo@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof
 Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Udit Kumar <u-kumar1@...com>,
        Neha Malcom
 Francis <n-francis@...com>,
        Aniket Limaye <a-limaye@...com>
Subject: Re: [PATCH v2 2/3] arm64: dts: ti: Introduce J742S2 SoC family

Hi Andrew,

On 10:03-20240731, Andrew Davis wrote:
> On 7/31/24 9:58 AM, Manorit Chawdhry wrote:
> > Hi Andrew,
> > 
> > On 09:37-20240731, Andrew Davis wrote:
> > > On 7/31/24 8:57 AM, Manorit Chawdhry wrote:
> > > > Hi Nishanth,
> > > > 
> > > > On 06:06-20240731, Nishanth Menon wrote:
> > > > > On 09:49-20240731, Manorit Chawdhry wrote:
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include "k3-j784s4.dtsi"
> > > > > > > > +
> > > > > > > > +/ {
> > > > > > > > +	model = "Texas Instruments K3 J742S2 SoC";
> > > > > > > > +	compatible = "ti,j742s2";
> > > > > > > > +
> > > > > > > > +	cpus {
> > > > > > > > +		cpu-map {
> > > > > > > > +			/delete-node/ cluster1;
> > > > > > > > +		};
> > > > > > > > +	};
> > > > > > > > +
> > > > > > > > +	/delete-node/ cpu4;
> > > > > > > > +	/delete-node/ cpu5;
> > > > > > > > +	/delete-node/ cpu6;
> > > > > > > > +	/delete-node/ cpu7;
> > > > > > > 
> > > > > > > I suggest refactoring by renaming the dtsi files as common and split out
> > > > > > > j784s4 similar to j722s/am62p rather than using /delete-node/
> > > > > > > 
> > > > > > 
> > > > > > I don't mind the suggestion Nishanth if there is a reason behind it.
> > > > > > Could you tell why we should not be using /delete-node/?
> > > > > > 
> > > > > 
> > > > > Maintenance, readability and sustenance are the reasons. This is a
> > > > > optimized die. It will end up having it's own changes in property
> > > > > and integration details. While reuse is necessary, modifying the
> > > > > properties with overrides and /delete-nodes/ creates maintenance
> > > > > challenges down the road. We already went down this road with am62p
> > > > > reuse with j722s, and eventually determined split and reuse is the
> > > > > best option. See [1] for additional guidance.
> > > > > 
> > > > > 
> > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/dts-coding-style.rst#n189
> > > > 
> > > > Thank you for giving some reasoning, would do the needful!
> > > > 
> > > 
> > > This refactor will require some interesting naming for the
> > > common SoC files. Based on your name for the EVM, I'm guessing
> > > you will go with
> > 
> > One other reason I was trying to avoid that and going with
> > /delete-node/. For such a small delta change tbh, this churn doesn't
> > feel worth the effort to me and is just gonna create confusion.
> > 
> > EVM one was required as Rob did raise an interesting point and we did
> > require a soc file that wasn't existing with the previous patchset but
> > now for deleting just 4 cpus and 1 dsp, am gonna have to rename all the
> > files, change the hierarchical structure, add all the cpus again with
> > some weird naming for the file as don't know if some other soc is gonna
> > come up in future so don't wanna clutter the file names as well with
> > j784s4-j742s2-j7xxx.dtsi which is just gonna create another set of mess
> > in future.
> > 
> 
> Which is why I would suggesting getting the name picked and agreed on
> here before you start doing the renames (renames for .dtsi files are not
> a problem, only the final .dtb names seem to require stability as the
> bootloader tend to load them by name, and those are not changing)
> 
> What is wrong with just k3-j784s4-common.dtsi? All future spins of
> this base device can include from this file. Every spin doesn't need
> to be in the common file's name.

Yeah, was gonna go with that file only right now, but now would I have

- k3-j784s4-mcu-wakeup-common.dtsi ( this is not required at this stage,
but ig for consistency better to now itself )
- k3-j784s4-main-common.dtsi ( all dsps excluding c7x_3 )
- k3-j784s4-thermal-common.dtsi ( not required again but consistency )
- k3-j784s4-common.dtsi ( all this won't have the cpu but will have all
						  other ranges including for the last dsp and all )
- k3-j784s4.dtsi ( have 8 cores )
- k3-j784s4-main.dtsi ( have an additional dsp )
- k3-j742s2.dtsi ( have 4 cores )
- k3-j742s2-main.dtsi ( have firmware name overrides )

I do find it confusing while developing but mostly people would have to get
used to developing in common files and hoping that things should be
okay. 

Regards,
Manorit
> 
> Andrew
> 
> > Regards,
> > Manorit
> > 
> > > 
> > > k3-j784s4-common.dtsi
> > > 
> > > included from the real k3-j784s4.dtsi and the new k3-j742s2.dtsi?
> > > 
> > > Too bad the Jacinto SoC names don't use a hierarchical naming. :(
> > > 
> > > J7<family><part><spin><etc>..
> > > 
> > > Andrew
> > > 
> > > > Regards,
> > > > Manorit
> > > > 
> > > > > 
> > > > > -- 
> > > > > Regards,
> > > > > Nishanth Menon
> > > > > Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
> > > > 

Powered by blists - more mailing lists