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

[Marek Vasut]

On 4/8/20 12:09 PM, Patrick DELAUNAY wrote:
> Hi,

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 ?

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