On 4/8/20 12:09 PM, Patrick DELAUNAY wrote:
> Hi,
Hi,
>> From: Marek Vasut <ma...@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;
...
}
