Re: [PATCH 2/3] arm64: dts: ti: Introduce J722S family of SoCs

[Nishanth Menon <nm@ti.com>]

On 09:43-20231214, Andrew Davis wrote:
> On 12/14/23 6:27 AM, Nishanth Menon wrote:
> > On 14:07-20231214, Vaishnav Achath wrote:
> > [..]
> > > > Trim this down to what is different from AM62P?
> > > > 
> > > 
> > > Thanks for the review, I will trim this down in next revision, but the above is
> > > just a summary of the main features of this SoC, pointing to AM62P feature set
> > > here seems confusing to me. why does a new user/developer using J722S need to be
> > > aware of the existence of AM62P to just understand a high level summary about
> > > this device?
> > 
> > Since this is a reuse device. Helps with review and focus on deltas.
> > 
> > [...]
> > 
> > > > > +	l2_0: l2-cache0 {
> > > > > +		compatible = "cache";
> > > > > +		cache-unified;
> > > > > +		cache-level = <2>;
> > > > > +		cache-size = <0x80000>;
> > > > > +		cache-line-size = <64>;
> > > > > +		cache-sets = <512>;
> > > > > +	};
> > > > 
> > > > ^^ this is a duplication of am62p5.dtsi? what about the spins with
> > > > different CPUs enabled?
> > > > 
> > > 
> > > Yes it is a duplicate, as of now we are not aware of plan for spins with cores
> > > disabled, so just followed the pattern followed for other Jacinto devices
> > > (J721e, J7200, J721s2, J784s4).
> > 
> > None of the devices have been as close a reuse device as this has been.
> 
> I'd argue J721e and J7200 are more similar in terms of reuse. It was a
> mistake to model them as simple super/subsets of each other, only causes
> confusion later. Let's keep at least this top level file, we will end up
> using it more as more features/deltas are enabled/found.
> 

yes, we do need a top level dtsi for the SoC. just minimize the amount
of duplication.

[...]
> > > 
> > > [    7.492406] platform 79000000.r5f: configured R5F for remoteproc mode
> > > [    7.499887] platform 79000000.r5f: device does not have reserved memory
> > > regions, ret = -22
> > > [    7.508271] k3_r5_rproc bus@f0000:bus@4000000:r5fss@79000000: reserved memory
> > > init failed, ret = -22
> > > [    7.517549] remoteproc remoteproc0: releasing 79000000.r5f
> > > [    7.523338] k3_r5_rproc bus@f0000:bus@4000000:r5fss@79000000:
> > > k3_r5_cluster_rproc_init failed, ret = -22
> > > [    7.532993] k3_r5_rproc: probe of bus@f0000:bus@4000000:r5fss@79000000 failed
> > > with error -22
> > 
> > Yes, and the approach should rather be to disable the remote procs in
> > the board or at the SoC dtsi in a consistent manner. I had previously
> > suggested to do that SoC level (which means at am62p dtsi) since the remoteprocs have direct
> > dependency on how the memory layouts are partitioned in board.dts - but
> > i had asked folks working on remote procs to do that consistently across
> > SoCs. I don't see that having been done so far.
> > 
> 
> I fixed this for a couple SoCs way back last year (7e48b665100ee), seems
> folks kept adding mailboxes/rprocs un-disabled in the base .dtbi for
> new SoCs anyway :( This needs fixed in AM62p .dtsi first, then these
> disables can be removed from here.
> 

Yes, hence blocking it from here on. Cleanup, then add.

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