diff mbox series

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

Message ID 20200331234807.432637-3-marex@denx.de
State RFC
Delegated to: Patrick Delaunay
Headers show
Series [1/4,RFC] ARM: stm32: Implement board coding on AV96 | expand

Commit Message

Marek Vasut March 31, 2020, 11:48 p.m. UTC
Adjust the DDR configuration dtsi such that they only generate the
DRAM configuration node, the DDR controller node is moved into the
stm32mp157-u-boot.dtsi itself. This permits including multiple DDR
configuration dtsi files in board DT.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
---
 arch/arm/dts/stm32mp15-ddr.dtsi               | 357 +++++++++++-------
 .../dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi   |   1 +
 .../dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi   |   1 +
 arch/arm/dts/stm32mp157-u-boot.dtsi           |  25 ++
 4 files changed, 246 insertions(+), 138 deletions(-)

Comments

Patrick Delaunay April 7, 2020, 1 p.m. UTC | #1
Dear Marek,


> From: Marek Vasut <marex@denx.de>
> Sent: mercredi 1 avril 2020 01:48
> 
> Adjust the DDR configuration dtsi such that they only generate the DRAM
> configuration node, the DDR controller node is moved into the stm32mp157-u-
> boot.dtsi itself. This permits including multiple DDR configuration dtsi files in
> board DT.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> ---
>  arch/arm/dts/stm32mp15-ddr.dtsi               | 357 +++++++++++-------
>  .../dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi   |   1 +
>  .../dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi   |   1 +
>  arch/arm/dts/stm32mp157-u-boot.dtsi           |  25 ++
>  4 files changed, 246 insertions(+), 138 deletions(-)
> 
> 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@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@DDR_MEM_LABEL {

I think that  "config@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@2 {
		reg = 2;
#include "stm32mp15-ddr3-1x4Gb-1066-binG.dtsi"
	}
	config@3 {
		reg = 3;
#include "stm32mp15-ddr3-2x4Gb-1066-binG.dtsi"
	}
}

This proposal avoids to compare a hardcoded string in SPL...

Regards

Patrick
Marek Vasut April 7, 2020, 8:01 p.m. UTC | #2
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@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@DDR_MEM_LABEL {
> 
> I think that  "config@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@2 {
> 		reg = 2;
> #include "stm32mp15-ddr3-1x4Gb-1066-binG.dtsi"
> 	}
> 	config@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.
Patrick Delaunay April 8, 2020, 10:09 a.m. UTC | #3
Hi,

> From: Marek Vasut <marex@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@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@DDR_MEM_LABEL {
> >
> > I think that  "config@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@2 {
> > 		reg = 2;
> > #include "stm32mp15-ddr3-1x4Gb-1066-binG.dtsi"
> > 	}
> > 	config@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@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
Marek Vasut April 8, 2020, 1:53 p.m. UTC | #4
On 4/8/20 12:09 PM, Patrick DELAUNAY wrote:
> Hi,

Hi,

>> From: Marek Vasut <marex@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@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@DDR_MEM_LABEL {
>>>
>>> I think that  "config@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@2 {
>>> 		reg = 2;
>>> #include "stm32mp15-ddr3-1x4Gb-1066-binG.dtsi"
>>> 	}
>>> 	config@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@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 ?

I wonder, can't we have some sort of macro where you would specify a
compatible string for the DDR config (on which you can match in your
board_stm32mp1_ddr_config_name_match() and the dtsi file to be included,
and the macro would generate the necessary entries in the &ddr {}
controller node ?

E.g. like this:

#include "stm32mp15-ddr.dtsi"
STM32MP15_DDR("vendor,board-1gib", stm32mp15-ddr3-2x4Gb-1066-binG.dtsi);
STM32MP15_DDR("vendor,board-2gib", stm32mp15-ddr3-4x4Gb-1066-binG.dtsi);

and then in board_stm32mp1_ddr_config_name_match()
{
 if (!strcmp(..., "vendor,board-1gib"))
    return 0;
 ...
}
Patrick Delaunay April 9, 2020, 5:05 p.m. UTC | #5
Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: mercredi 8 avril 2020 15:54
> 
> On 4/8/20 12:09 PM, Patrick DELAUNAY wrote:
> > Hi,
> 
> Hi,
> 
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: mardi 7 avril 2020 22:01
> >>
> >> On 4/7/20 3:00 PM, Patrick DELAUNAY wrote:
> >>> Dear Marek,
> >>
> >> Hi,
> >>

[...]

> >>>
> >>> 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"
> >>> }
> >>>
> >>>

[...]

> >>
> >> 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.
> >

[....]

> >
> > 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 ?
> 
> I wonder, can't we have some sort of macro where you would specify a compatible
> string for the DDR config (on which you can match in your
> board_stm32mp1_ddr_config_name_match() and the dtsi file to be included, and
> the macro would generate the necessary entries in the &ddr {} controller node ?
> 
> E.g. like this:
> 
> #include "stm32mp15-ddr.dtsi"
> STM32MP15_DDR("vendor,board-1gib", stm32mp15-ddr3-2x4Gb-1066-binG.dtsi);
> STM32MP15_DDR("vendor,board-2gib", stm32mp15-ddr3-4x4Gb-1066-binG.dtsi);
> 
> and then in board_stm32mp1_ddr_config_name_match()
> {
>  if (!strcmp(..., "vendor,board-1gib"))
>     return 0;
>  ...
> }

Yes, I agree, compatible is the better solution and the binding 

./doc/device-tree-bindings/memory-controllers/st,stm32mp1-ddr.txt becomes

ddr: ddr@0x5A003000{
	compatible = "st,stm32mp1-ddr";
	[...]

	config-1 {
		compatible = "vendor,board-1gib";

		st,mem-name = "..."
		[...]
		st,phy-timing = <...>
	}
	config-2 {
		compatible = "vendor,board-2gib";
		st,mem-name = "..."
		[...]
		st,phy-timing = <...>
	}
	status = "okay";
}

And you match this configuration with compatible.

For the macro, it should be perfect, if it is not too complicate.

Because I afraid that "#include" in macro isn't allowed.

regards

Patrick
Marek Vasut April 10, 2020, 6:36 p.m. UTC | #6
On 4/9/20 7:05 PM, Patrick DELAUNAY wrote:
> Hi Marek,
> 
>> From: Marek Vasut <marex@denx.de>
>> Sent: mercredi 8 avril 2020 15:54
>>
>> On 4/8/20 12:09 PM, Patrick DELAUNAY wrote:
>>> Hi,
>>
>> Hi,
>>
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: mardi 7 avril 2020 22:01
>>>>
>>>> On 4/7/20 3:00 PM, Patrick DELAUNAY wrote:
>>>>> Dear Marek,
>>>>
>>>> Hi,
>>>>
> 
> [...]
> 
>>>>>
>>>>> 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"
>>>>> }
>>>>>
>>>>>
> 
> [...]
> 
>>>>
>>>> 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.
>>>
> 
> [....]
> 
>>>
>>> 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 ?
>>
>> I wonder, can't we have some sort of macro where you would specify a compatible
>> string for the DDR config (on which you can match in your
>> board_stm32mp1_ddr_config_name_match() and the dtsi file to be included, and
>> the macro would generate the necessary entries in the &ddr {} controller node ?
>>
>> E.g. like this:
>>
>> #include "stm32mp15-ddr.dtsi"
>> STM32MP15_DDR("vendor,board-1gib", stm32mp15-ddr3-2x4Gb-1066-binG.dtsi);
>> STM32MP15_DDR("vendor,board-2gib", stm32mp15-ddr3-4x4Gb-1066-binG.dtsi);
>>
>> and then in board_stm32mp1_ddr_config_name_match()
>> {
>>  if (!strcmp(..., "vendor,board-1gib"))
>>     return 0;
>>  ...
>> }
> 
> Yes, I agree, compatible is the better solution and the binding 
> 
> ./doc/device-tree-bindings/memory-controllers/st,stm32mp1-ddr.txt becomes
> 
> ddr: ddr@0x5A003000{
> 	compatible = "st,stm32mp1-ddr";
> 	[...]
> 
> 	config-1 {
> 		compatible = "vendor,board-1gib";
> 
> 		st,mem-name = "..."
> 		[...]
> 		st,phy-timing = <...>
> 	}
> 	config-2 {
> 		compatible = "vendor,board-2gib";
> 		st,mem-name = "..."
> 		[...]
> 		st,phy-timing = <...>
> 	}
> 	status = "okay";
> }
> 
> And you match this configuration with compatible.
> 
> For the macro, it should be perfect, if it is not too complicate.
> 
> Because I afraid that "#include" in macro isn't allowed.

I'll send a V2 now. The compatible string is easy enough.
diff mbox series

Patch

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@5a003000 {
-			u-boot,dm-pre-reloc;
+&ddr {
+	config@DDR_MEM_LABEL {
+		u-boot,dm-pre-reloc;
 
-			compatible = "st,stm32mp1-ddr";
+		st,mem-name = DDR_MEM_NAME;
+		st,mem-speed = <DDR_MEM_SPEED>;
+		st,mem-size = <DDR_MEM_SIZE>;
 
-			reg = <0x5A003000 0x550
-			       0x5A004000 0x234>;
+		st,ctl-reg = <
+			DDR_MSTR
+			DDR_MRCTRL0
+			DDR_MRCTRL1
+			DDR_DERATEEN
+			DDR_DERATEINT
+			DDR_PWRCTL
+			DDR_PWRTMG
+			DDR_HWLPCTL
+			DDR_RFSHCTL0
+			DDR_RFSHCTL3
+			DDR_CRCPARCTL0
+			DDR_ZQCTL0
+			DDR_DFITMG0
+			DDR_DFITMG1
+			DDR_DFILPCFG0
+			DDR_DFIUPD0
+			DDR_DFIUPD1
+			DDR_DFIUPD2
+			DDR_DFIPHYMSTR
+			DDR_ODTMAP
+			DDR_DBG0
+			DDR_DBG1
+			DDR_DBGCMD
+			DDR_POISONCFG
+			DDR_PCCFG
+		>;
 
-			clocks = <&rcc AXIDCG>,
-				 <&rcc DDRC1>,
-				 <&rcc DDRC2>,
-				 <&rcc DDRPHYC>,
-				 <&rcc DDRCAPB>,
-				 <&rcc DDRPHYCAPB>;
+		st,ctl-timing = <
+			DDR_RFSHTMG
+			DDR_DRAMTMG0
+			DDR_DRAMTMG1
+			DDR_DRAMTMG2
+			DDR_DRAMTMG3
+			DDR_DRAMTMG4
+			DDR_DRAMTMG5
+			DDR_DRAMTMG6
+			DDR_DRAMTMG7
+			DDR_DRAMTMG8
+			DDR_DRAMTMG14
+			DDR_ODTCFG
+		>;
 
-			clock-names = "axidcg",
-				      "ddrc1",
-				      "ddrc2",
-				      "ddrphyc",
-				      "ddrcapb",
-				      "ddrphycapb";
+		st,ctl-map = <
+			DDR_ADDRMAP1
+			DDR_ADDRMAP2
+			DDR_ADDRMAP3
+			DDR_ADDRMAP4
+			DDR_ADDRMAP5
+			DDR_ADDRMAP6
+			DDR_ADDRMAP9
+			DDR_ADDRMAP10
+			DDR_ADDRMAP11
+		>;
 
-			st,mem-name = DDR_MEM_NAME;
-			st,mem-speed = <DDR_MEM_SPEED>;
-			st,mem-size = <DDR_MEM_SIZE>;
+		st,ctl-perf = <
+			DDR_SCHED
+			DDR_SCHED1
+			DDR_PERFHPR1
+			DDR_PERFLPR1
+			DDR_PERFWR1
+			DDR_PCFGR_0
+			DDR_PCFGW_0
+			DDR_PCFGQOS0_0
+			DDR_PCFGQOS1_0
+			DDR_PCFGWQOS0_0
+			DDR_PCFGWQOS1_0
+			DDR_PCFGR_1
+			DDR_PCFGW_1
+			DDR_PCFGQOS0_1
+			DDR_PCFGQOS1_1
+			DDR_PCFGWQOS0_1
+			DDR_PCFGWQOS1_1
+		>;
 
-			st,ctl-reg = <
-				DDR_MSTR
-				DDR_MRCTRL0
-				DDR_MRCTRL1
-				DDR_DERATEEN
-				DDR_DERATEINT
-				DDR_PWRCTL
-				DDR_PWRTMG
-				DDR_HWLPCTL
-				DDR_RFSHCTL0
-				DDR_RFSHCTL3
-				DDR_CRCPARCTL0
-				DDR_ZQCTL0
-				DDR_DFITMG0
-				DDR_DFITMG1
-				DDR_DFILPCFG0
-				DDR_DFIUPD0
-				DDR_DFIUPD1
-				DDR_DFIUPD2
-				DDR_DFIPHYMSTR
-				DDR_ODTMAP
-				DDR_DBG0
-				DDR_DBG1
-				DDR_DBGCMD
-				DDR_POISONCFG
-				DDR_PCCFG
-			>;
+		st,phy-reg = <
+			DDR_PGCR
+			DDR_ACIOCR
+			DDR_DXCCR
+			DDR_DSGCR
+			DDR_DCR
+			DDR_ODTCR
+			DDR_ZQ0CR1
+			DDR_DX0GCR
+			DDR_DX1GCR
+			DDR_DX2GCR
+			DDR_DX3GCR
+		>;
 
-			st,ctl-timing = <
-				DDR_RFSHTMG
-				DDR_DRAMTMG0
-				DDR_DRAMTMG1
-				DDR_DRAMTMG2
-				DDR_DRAMTMG3
-				DDR_DRAMTMG4
-				DDR_DRAMTMG5
-				DDR_DRAMTMG6
-				DDR_DRAMTMG7
-				DDR_DRAMTMG8
-				DDR_DRAMTMG14
-				DDR_ODTCFG
-			>;
+		st,phy-timing = <
+			DDR_PTR0
+			DDR_PTR1
+			DDR_PTR2
+			DDR_DTPR0
+			DDR_DTPR1
+			DDR_DTPR2
+			DDR_MR0
+			DDR_MR1
+			DDR_MR2
+			DDR_MR3
+		>;
 
-			st,ctl-map = <
-				DDR_ADDRMAP1
-				DDR_ADDRMAP2
-				DDR_ADDRMAP3
-				DDR_ADDRMAP4
-				DDR_ADDRMAP5
-				DDR_ADDRMAP6
-				DDR_ADDRMAP9
-				DDR_ADDRMAP10
-				DDR_ADDRMAP11
-			>;
+		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
+		>;
 
-			st,ctl-perf = <
-				DDR_SCHED
-				DDR_SCHED1
-				DDR_PERFHPR1
-				DDR_PERFLPR1
-				DDR_PERFWR1
-				DDR_PCFGR_0
-				DDR_PCFGW_0
-				DDR_PCFGQOS0_0
-				DDR_PCFGQOS1_0
-				DDR_PCFGWQOS0_0
-				DDR_PCFGWQOS1_0
-				DDR_PCFGR_1
-				DDR_PCFGW_1
-				DDR_PCFGQOS0_1
-				DDR_PCFGQOS1_1
-				DDR_PCFGWQOS0_1
-				DDR_PCFGWQOS1_1
-			>;
-
-			st,phy-reg = <
-				DDR_PGCR
-				DDR_ACIOCR
-				DDR_DXCCR
-				DDR_DSGCR
-				DDR_DCR
-				DDR_ODTCR
-				DDR_ZQ0CR1
-				DDR_DX0GCR
-				DDR_DX1GCR
-				DDR_DX2GCR
-				DDR_DX3GCR
-			>;
-
-			st,phy-timing = <
-				DDR_PTR0
-				DDR_PTR1
-				DDR_PTR2
-				DDR_DTPR0
-				DDR_DTPR1
-				DDR_DTPR2
-				DDR_MR0
-				DDR_MR1
-				DDR_MR2
-				DDR_MR3
-			>;
-
-			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
-			>;
-
-			status = "okay";
-		};
+		status = "okay";
 	};
 };
+
+#undef DDR_MEM_LABEL
+#undef DDR_MEM_NAME
+#undef DDR_MEM_SPEED
+#undef DDR_MEM_SIZE
+
+#undef DDR_MSTR
+#undef DDR_MRCTRL0
+#undef DDR_MRCTRL1
+#undef DDR_DERATEEN
+#undef DDR_DERATEINT
+#undef DDR_PWRCTL
+#undef DDR_PWRTMG
+#undef DDR_HWLPCTL
+#undef DDR_RFSHCTL0
+#undef DDR_RFSHCTL3
+#undef DDR_RFSHTMG
+#undef DDR_CRCPARCTL0
+#undef DDR_DRAMTMG0
+#undef DDR_DRAMTMG1
+#undef DDR_DRAMTMG2
+#undef DDR_DRAMTMG3
+#undef DDR_DRAMTMG4
+#undef DDR_DRAMTMG5
+#undef DDR_DRAMTMG6
+#undef DDR_DRAMTMG7
+#undef DDR_DRAMTMG8
+#undef DDR_DRAMTMG14
+#undef DDR_ZQCTL0
+#undef DDR_DFITMG0
+#undef DDR_DFITMG1
+#undef DDR_DFILPCFG0
+#undef DDR_DFIUPD0
+#undef DDR_DFIUPD1
+#undef DDR_DFIUPD2
+#undef DDR_DFIPHYMSTR
+#undef DDR_ADDRMAP1
+#undef DDR_ADDRMAP2
+#undef DDR_ADDRMAP3
+#undef DDR_ADDRMAP4
+#undef DDR_ADDRMAP5
+#undef DDR_ADDRMAP6
+#undef DDR_ADDRMAP9
+#undef DDR_ADDRMAP10
+#undef DDR_ADDRMAP11
+#undef DDR_ODTCFG
+#undef DDR_ODTMAP
+#undef DDR_SCHED
+#undef DDR_SCHED1
+#undef DDR_PERFHPR1
+#undef DDR_PERFLPR1
+#undef DDR_PERFWR1
+#undef DDR_DBG0
+#undef DDR_DBG1
+#undef DDR_DBGCMD
+#undef DDR_POISONCFG
+#undef DDR_PCCFG
+#undef DDR_PCFGR_0
+#undef DDR_PCFGW_0
+#undef DDR_PCFGQOS0_0
+#undef DDR_PCFGQOS1_0
+#undef DDR_PCFGWQOS0_0
+#undef DDR_PCFGWQOS1_0
+#undef DDR_PCFGR_1
+#undef DDR_PCFGW_1
+#undef DDR_PCFGQOS0_1
+#undef DDR_PCFGQOS1_1
+#undef DDR_PCFGWQOS0_1
+#undef DDR_PCFGWQOS1_1
+#undef DDR_PGCR
+#undef DDR_PTR0
+#undef DDR_PTR1
+#undef DDR_PTR2
+#undef DDR_ACIOCR
+#undef DDR_DXCCR
+#undef DDR_DSGCR
+#undef DDR_DCR
+#undef DDR_DTPR0
+#undef DDR_DTPR1
+#undef DDR_DTPR2
+#undef DDR_MR0
+#undef DDR_MR1
+#undef DDR_MR2
+#undef DDR_MR3
+#undef DDR_ODTCR
+#undef DDR_ZQ0CR1
+#undef DDR_DX0GCR
+#undef DDR_DX0DLLCR
+#undef DDR_DX0DQTR
+#undef DDR_DX0DQSTR
+#undef DDR_DX1GCR
+#undef DDR_DX1DLLCR
+#undef DDR_DX1DQTR
+#undef DDR_DX1DQSTR
+#undef DDR_DX2GCR
+#undef DDR_DX2DLLCR
+#undef DDR_DX2DQTR
+#undef DDR_DX2DQSTR
+#undef DDR_DX3GCR
+#undef DDR_DX3DLLCR
+#undef DDR_DX3DQTR
+#undef DDR_DX3DQSTR
diff --git a/arch/arm/dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi b/arch/arm/dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi
index 11e8f2bef6..a63be8643c 100644
--- a/arch/arm/dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi
+++ b/arch/arm/dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi
@@ -16,6 +16,7 @@ 
  * address mapping : RBC
  * Tc > + 85C : N
  */
+#define DDR_MEM_LABEL ddr3-1066-888-bin-g-1x4gb-533mhz
 #define DDR_MEM_NAME "DDR3-1066/888 bin G 1x4Gb 533MHz v1.45"
 #define DDR_MEM_SPEED 533000
 #define DDR_MEM_SIZE 0x20000000
diff --git a/arch/arm/dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi b/arch/arm/dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi
index 4b70b60554..6f059269af 100644
--- a/arch/arm/dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi
+++ b/arch/arm/dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi
@@ -16,6 +16,7 @@ 
  * address mapping : RBC
  * Tc > + 85C : N
  */
+#define DDR_MEM_LABEL ddr3-1066-888-bin-g-2x4gb-533mhz
 #define DDR_MEM_NAME "DDR3-1066/888 bin G 2x4Gb 533MHz v1.45"
 #define DDR_MEM_SPEED 533000
 #define DDR_MEM_SIZE 0x40000000
diff --git a/arch/arm/dts/stm32mp157-u-boot.dtsi b/arch/arm/dts/stm32mp157-u-boot.dtsi
index 8f9535a4db..1279589a56 100644
--- a/arch/arm/dts/stm32mp157-u-boot.dtsi
+++ b/arch/arm/dts/stm32mp157-u-boot.dtsi
@@ -36,6 +36,31 @@ 
 
 	soc {
 		u-boot,dm-pre-reloc;
+
+		ddr: ddr@5a003000 {
+			u-boot,dm-pre-reloc;
+
+			compatible = "st,stm32mp1-ddr";
+
+			reg = <0x5A003000 0x550
+			       0x5A004000 0x234>;
+
+			clocks = <&rcc AXIDCG>,
+				 <&rcc DDRC1>,
+				 <&rcc DDRC2>,
+				 <&rcc DDRPHYC>,
+				 <&rcc DDRCAPB>,
+				 <&rcc DDRPHYCAPB>;
+
+			clock-names = "axidcg",
+				      "ddrc1",
+				      "ddrc2",
+				      "ddrphyc",
+				      "ddrcapb",
+				      "ddrphycapb";
+
+			status = "okay";
+		};
 	};
 };