Re: [PATCH v4 1/5] arm64: dts: ti: Refactor J784s4 SoC files to a common file

[Manorit Chawdhry]

Hi Siddharth,

On 13:36-20240827, Siddharth Vadapalli wrote:
> On Mon, Aug 19, 2024 at 03:09:35PM +0530, Manorit Chawdhry wrote:
> > Refactor J784s4 SoC files to a common file which uses the
> > superset device to allow reuse in j742s2-evm which uses the subset part.
> > 
> > Reviewed-by: Beleswar Padhi <b-padhi@xxxxxx>
> > Signed-off-by: Manorit Chawdhry <m-chawdhry@xxxxxx>
> > ---
> >  .../arm64/boot/dts/ti/k3-j784s4-j742s2-common.dtsi |  149 +
> >  .../boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi  | 2667 ++++++++++++++++++
> >  ...tsi => k3-j784s4-j742s2-mcu-wakeup-common.dtsi} |    2 +-
> >  ...l.dtsi => k3-j784s4-j742s2-thermal-common.dtsi} |    0
> >  arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi         | 2847 +-------------------
> >  arch/arm64/boot/dts/ti/k3-j784s4.dtsi              |  135 +-
> >  6 files changed, 2913 insertions(+), 2887 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-common.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-common.dtsi
> > new file mode 100644
> > index 000000000000..958054ab1018
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-common.dtsi
> > @@ -0,0 +1,149 @@
> > +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> > +/*
> > + * Device Tree Source for J784S4 SoC Family
> 
> Since the file is already named "...-j784s4-j742s2-...", wouldn't it be
> better to add J742S2 and the link to its TRM here itself rather than
> adding it in patch 4/5?

I would align this, thanks!

> 
> > + *
> > + * TRM (SPRUJ43 JULY 2022): https://www.ti.com/lit/zip/spruj52
> > + *
> > + * Copyright (C) 2022-2024 Texas Instruments Incorporated - https://www.ti.com/
> 
> Since this is a new file and not a moved file, should it simply be "2024"?

The content of this file is copyright since 2022 so it seemed
counter-intuitive to me to remove that copyright, if the flow is to
remove older copyright whenever file is moved then I can do it but need
to know what is followed here.

> 
> > + *
> > + */
> 
> [...]
> 
> > +
> > +
> > +	cbass_main: bus@100000 {
> > +		bootph-all;
> > +		compatible = "simple-bus";
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges = <0x00 0x00100000 0x00 0x00100000 0x00 0x00020000>, /* ctrl mmr */
> > +			 <0x00 0x00600000 0x00 0x00600000 0x00 0x00031100>, /* GPIO */
> > +			 <0x00 0x00700000 0x00 0x00700000 0x00 0x00001000>, /* ESM */
> > +			 <0x00 0x01000000 0x00 0x01000000 0x00 0x0d000000>, /* Most peripherals */
> 
> Since SERDES2 lies in the above range, to be techincally correct, the
> above range should be split up to move SERDES2 out of the common file.

It's a maintainance overhead as whatever support is being added to
j784s4 would have to be explicitely ported to j742s2 by adding ranges, I
know it's technically correct to add j742s2 ranges in a separate file
but it's not worth the maintainance that future development on both the
devices would be bringing so I think keeping it the same would be the
way to go.

> 
> > +			 <0x00 0x04210000 0x00 0x04210000 0x00 0x00010000>, /* VPU0 */
> > +			 <0x00 0x04220000 0x00 0x04220000 0x00 0x00010000>, /* VPU1 */
> > +			 <0x00 0x0d000000 0x00 0x0d000000 0x00 0x00800000>, /* PCIe0 Core*/
> > +			 <0x00 0x0d800000 0x00 0x0d800000 0x00 0x00800000>, /* PCIe1 Core*/
> > +			 <0x00 0x0e000000 0x00 0x0e000000 0x00 0x00800000>, /* PCIe2 Core*/
> > +			 <0x00 0x0e800000 0x00 0x0e800000 0x00 0x00800000>, /* PCIe3 Core*/
> 
> PCIe2 and PCIe3 should be dropped from the common file.
> 
> > +			 <0x00 0x10000000 0x00 0x10000000 0x00 0x08000000>, /* PCIe0 DAT0 */
> > +			 <0x00 0x18000000 0x00 0x18000000 0x00 0x08000000>, /* PCIe1 DAT0 */
> > +			 <0x00 0x64800000 0x00 0x64800000 0x00 0x0070c000>, /* C71_1 */
> > +			 <0x00 0x65800000 0x00 0x65800000 0x00 0x0070c000>, /* C71_2 */
> > +			 <0x00 0x66800000 0x00 0x66800000 0x00 0x0070c000>, /* C71_3 */
> > +			 <0x00 0x67800000 0x00 0x67800000 0x00 0x0070c000>, /* C71_4 */
> > +			 <0x00 0x6f000000 0x00 0x6f000000 0x00 0x00310000>, /* A72 PERIPHBASE */
> > +			 <0x00 0x70000000 0x00 0x70000000 0x00 0x00400000>, /* MSMC RAM */
> > +			 <0x00 0x30000000 0x00 0x30000000 0x00 0x0c400000>, /* MAIN NAVSS */
> > +			 <0x40 0x00000000 0x40 0x00000000 0x01 0x00000000>, /* PCIe0 DAT1 */
> > +			 <0x41 0x00000000 0x41 0x00000000 0x01 0x00000000>, /* PCIe1 DAT1 */
> > +			 <0x42 0x00000000 0x42 0x00000000 0x01 0x00000000>, /* PCIe2 DAT1 */
> > +			 <0x43 0x00000000 0x43 0x00000000 0x01 0x00000000>, /* PCIe3 DAT1 */
> > +			 <0x44 0x00000000 0x44 0x00000000 0x00 0x08000000>, /* PCIe2 DAT0 */
> > +			 <0x44 0x10000000 0x44 0x10000000 0x00 0x08000000>, /* PCIe3 DAT0 */
> 
> PCIe2 and PCIe3 should be dropped from the common file.
> 
> > +			 <0x4e 0x20000000 0x4e 0x20000000 0x00 0x00080000>, /* GPU */
> > +
> > +			 /* MCUSS_WKUP Range */
> > +			 <0x00 0x28380000 0x00 0x28380000 0x00 0x03880000>,
> > +			 <0x00 0x40200000 0x00 0x40200000 0x00 0x00998400>,
> > +			 <0x00 0x40f00000 0x00 0x40f00000 0x00 0x00020000>,
> > +			 <0x00 0x41000000 0x00 0x41000000 0x00 0x00020000>,
> 
> [...]
> 
> > diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi
> > new file mode 100644
> > index 000000000000..04d77c42442d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi
> > @@ -0,0 +1,2667 @@
> > +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> > +/*
> > + * Device Tree Source for J784S4 and J742S2 SoC Family Main Domain peripherals
> 
> Here, you have mentioned J742S2 as well, so to keep it consistent,
> J742S2 should be mentioned in the "k3-j784s4-j742s2-common.dtsi" file as
> well as I have indicated above.
> 
> > + *
> > + * Copyright (C) 2022-2024 Texas Instruments Incorporated - https://www.ti.com/
> 
> Since this is a new file and not a renamed file, shouldn't it be "2024"?
> 
> > + */
> > +
> > +#include <dt-bindings/mux/mux.h>
> > +#include <dt-bindings/phy/phy.h>
> > +#include <dt-bindings/phy/phy-ti.h>
> > +
> 
> [...]
> 
> Regards,
> Siddharth.

Regards,
Manorit