[PATCH 3/4] [RFC] ARM: dts: stm32: Rework DDR DT inclusion

[Patrick DELAUNAY]

Hi,

> From: Marek Vasut <marex at denx.de>
> Sent: mardi 7 avril 2020 22:01
> 
> On 4/7/20 3:00 PM, Patrick DELAUNAY wrote:
> > Dear Marek,
> 
> Hi,
> 
> >> diff --git a/arch/arm/dts/stm32mp15-ddr.dtsi
> >> b/arch/arm/dts/stm32mp15-ddr.dtsi index 38f29bb789..50ca7092c4 100644
> >> --- a/arch/arm/dts/stm32mp15-ddr.dtsi
> >> +++ b/arch/arm/dts/stm32mp15-ddr.dtsi
> >> @@ -3,152 +3,233 @@
> >>   * Copyright : STMicroelectronics 2018
> >>   */
> >>
> >> -/ {
> >> -	soc {
> >> -		ddr: ddr at 5a003000 {
> >> -			u-boot,dm-pre-reloc;
> >
> > For information, binding file must be updated also
> > ./doc/device-tree-bindings/memory-controllers/st,stm32mp1-ddr.txt
> >
> > This binding and the helper file " stm32mp15-ddr.dtsi" is common with TF-A.
> >
> >> +&ddr {
> >> +	config at DDR_MEM_LABEL {
> >
> > I think that  "config at text"
> > don't respect the latest device tree rule and a warning is raised with
> > latest dtc version
> >
> > it is now mandatory to value after align @ with reg value
> >
> > config@<reg> {
> > 	reg = <reg>
> > }
> >
> > It is why I prefer a name without meaning here (as config-1 /
> > config-2), And selection is done on st,mem-name
> >
> > config-1 {
> > ....
> > }
> > config-2 {
> > ....
> > }
> >
> >
> > And I want to propose, for DH board with several configuration
> >
> > &ddr  {
> > 	config-1 {
> > #include "stm32mp15-ddr3-1x4Gb-1066-binG.dtsi"
> > 	}
> > 	config-2 {
> > #include "stm32mp15-ddr3-2x4Gb-1066-binG.dtsi"
> > 	}
> > }
> >
> >
> > For ST board with only one configuration (don't change the device
> > tree, config at the same level) &ddr  { #include
> > "stm32mp15-ddr3-1x4Gb-1066-binG.dtsi"
> > }
> >
> >
> > NB: I have a other idea, it is to use the "reg" as config identifier to select
> configuration, as it is done with OTP with "opp-supported-hw"
> > in linux/Documentation/devicetree/bindings/opp/opp.txt
> >
> > And reg can be the identified with your GPIO setting
> >
> > &ddr  {
> > 	config at 2 {
> > 		reg = 2;
> > #include "stm32mp15-ddr3-1x4Gb-1066-binG.dtsi"
> > 	}
> > 	config at 3 {
> > 		reg = 3;
> > #include "stm32mp15-ddr3-2x4Gb-1066-binG.dtsi"
> > 	}
> > }
> >
> > This proposal avoids to compare a hardcoded string in SPL...
> 
> I would much rather prefer to avoid manually writing the config@<foo> parts, that
> should be handled by some macro magic instead. With my proposal, it is not
> necessary at all either.

Ok, but you should keep the support when DDR_MEM_LABEL is not available (the ddr dtsi is generated by ST tools)

I can propose a intermediate solution for the macro :

./arch/arm/dts/stm32mp15-ddr.dtsi

&ddr {
#ifdef DDR_MEM_CONFIG 
config at DDR_MEM_CONFIG {
	reg = < DDR_MEM_CONFIG>
#endif
	
	st,mem-name = DDR_MEM_NAME;
	st,mem-speed = <DDR_MEM_SPEED>;
	st,mem-size = <DDR_MEM_SIZE>;
[....]

	st,phy-cal = <
		DDR_DX0DLLCR
		DDR_DX0DQTR
		DDR_DX0DQSTR
		DDR_DX1DLLCR
		DDR_DX1DQTR
		DDR_DX1DQSTR
		DDR_DX2DLLCR
		DDR_DX2DQTR
		DDR_DX2DQSTR
		DDR_DX3DLLCR
		DDR_DX3DQTR
		DDR_DX3DQSTR
	>;
#ifdef DDR_MEM_CONFIG
}
#endif
}

#ifdef DDR_MEM_CONFIG
#undef DDR_MEM_CONFIG 
#endif

#undef DDR_MEM_LABEL
#undef DDR_MEM_NAME
#undef DDR_MEM_SPEED
#undef DDR_MEM_SIZE
 [....]
#undef DDR_DX3DQTR
#undef DDR_DX3DQSTR


So the file generate by CubeMX don't change =  stm32mp15-ddr3-1x4Gb-1066-binG.dtsi and stm32mp15-ddr3-2x4Gb-1066-binG.dtsi.

The ST board devicetree don't change: the DDR configuration is still in ddr node (as in TF-A)

For your board, the device tree /arch/arm/dts/stm32mp15xx-dhcor-u-boot.dtsi

[...]
#define DDR_MEM_CONFIG 2
#include "stm32mp15-ddr3-1x4Gb-1066-binG.dtsi"

#define DDR_MEM_CONFIG 3
#include "stm32mp15-ddr3-2x4Gb-1066-binG.dtsi"
[...]

And you can directly compare reg value of sub node with ddr3code.

It is more acceptable ?

Patrick