[U-Boot] [PATCH v2] arm64: dts: rockchip: Add support for Khadas Edge/Edge-V/Captain boards

[Kever Yang]

Hi Paul,


On 06/20/2019 12:54 AM, Paul Kocialkowski wrote:
> Hi Kever,
>
> Le mercredi 19 juin 2019 à 09:42 +0800, Kever Yang a écrit :
>> Hi Paul,
>>
>>
>> On 06/19/2019 12:12 AM, Mark Kettenis wrote:
>>>> From: Paul Kocialkowski <paul.kocialkowski at bootlin.com>
>>>> Date: Tue, 18 Jun 2019 14:47:33 +0200
>>>>
>>>> Hi Kever,
>>>>
>>>> On Tue, 2019-06-18 at 18:08 +0800, Kever Yang wrote:
>>>>> Hi Paul,
>>>>>
>>>>>
>>>>> On 06/18/2019 05:03 PM, Paul Kocialkowski wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, 2019-06-18 at 14:27 +0530, Jagan Teki wrote:
>>>>>>> On Tue, Jun 18, 2019 at 1:55 PM Paul Kocialkowski
>>>>>>> <paul.kocialkowski at bootlin.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Mon, 2019-06-17 at 15:24 +0800, xieqinick at gmail.com wrote:
>>>>>>>>> From: Nick Xie <nick at khadas.com>
>>>>>>>> Was this tested with SPL support? I don't see DRAM configuration here
>>>>>>>> so it seems that it relies on the non-free rockchip loader.
>>>>>>>>
>>>>>>>> If that is the case, could you please indicate that in the commit
>>>>>>>> message?
>>>>>>>>
>>>>>>>> To maintainers: please do not merge this series before DRAM init and
>>>>>>>> SPL support is available for these boards.
>>>>>>>>
>>>>>>>> It seems that other RK3399 boards were merged without SPL support and
>>>>>>>> sofar, I have the feeling that nobody cared to explain how we got into
>>>>>>>> this broken situation. Please don't merge any more such board.
>>>>>>> fyi: no rk3399 boards were merged w/o SPL. lpddr4 boards were merged
>>>>>>> with TPL-enabled (which was discussed on the threads, if you remember)
>>>>>>> with below boot chain.
>>>>>>>
>>>>>>> rkbin (TPL) -> SPL -> U-Boot proper
>>>>>>>
>>>>>>> Same case for this board as well.
>>>>>> Here is a quote from Philipp Tomsich on the thread we discussed this:
>>>>>>
>>>>>> " On some boards, there will be no TPL and only a SPL stage that will
>>>>>> initialise DRAM (as the move to having TPL on the RK3399 is optional).
>>>>>>
>>>>>> I agree with Paul that the DRAM init should be part of U-Boot whenever
>>>>>> we add new boards and make an open DRAM init a prerequisite. "
>>>>>>
>>>>>> So even if I frequently confuse SPL and TPL, it doesn't change the fact
>>>>>> that no board should be merged without proper DRAM init.
>>>>>>
>>>>>> Please stop pushing for merging these boards. I'm not sure what we
>>>>>> should do about the boards that were merged already without DRAM init,
>>>>>> but maybe they should be reverted.
>>>>> I don't think we have to have full DRAM init source code for each
>>>>> board before we can merge it, I believe most of the board on U-Boot
>>>>> mainline need to removed due to this rule. There are so many boards
>>>>> from different vendor need a binary loader before U-Boot, and I can
>>>>> see only very few drivers for dram at driver/ram/, and I believe rockchip
>>>>> is already the most open vendor on this area.
>>>> Well, I am not talking about full DRAM init source code as in dynamic
>>>> link training. I am talking about having at least static DRAM register
>>>> configuration values, 
>> I can tell you that this is no work for all the boards, you can see how
>> rockchip lpddr4(WIP, send by Jagan) driver works.
> I thought that LPDDR4 works the same as other types of DRAM where we
> have a dtsi array with timings configuration. Of course, some more
> registers need to be set up, but we already have support for that or
> it's quite close (for LPDDR4).
>
>>>> which is present for a good number of rockchip
>>>> boards.
>> No, there is no rockchip board only have static DRAM register
>> configuration values, that maybe happens in other vendor.
> I was implying that, as far as I know, it is the case for DRAM timings
> on Rockchip as well as most of the platforms that I know of. In the
> end, any code handling DRAM will end up writing timings to the
> controller's registers. If the DRAM is part of the PCB and doesn't
> change/move, then the timings don't change in particular.
>
> Is there something specific about Rockchip that makes it require
> different DRAM timings at each boot?
>
>>>> Of course, it would be best if Rockchip would consider releasing this
>>>> source code, 
>> Rockchip already release all the DRAM init source code, including DDR3 ,
>> LPDDR3,
>> and LPDDR4(wip). You can see the driver at driver/ram/rockchip/ for
>> everything,
>> which is not only static register configuration.
>> As I have said, rockchip is already the most open vendor in this area
>> till now, I don't know
>> if you have working on rockchip SoC based boards.
> You are quite right about that, but I was thinking about the code to
> calculate DRAM timings (with link-training and such) which is often not
> available as free software, and I am not aware of Rockchip having
> released that code (but feel free to correct me if I'm wrong).
>
>>>> which would be the easiest and friendliest solution
>>>> towards the community here. Are there internal discussions ongoing
>>>> about this? If not, it would be greatly appreciated to start such
>>>> discussions and clearly identify what the blocking points are.
>>>>
>>>>> I won't use this rule to stop developers contribute there source code,
>>>> This is really sad and I think that Philipp was, like me, inclined to
>>>> go towards the other direction.
>>>>
>>>>> for a board support, we only need the board to have the full documentation
>>>>> about how to setup and boot with upstream U-Boot. and I think the
>>>>> most of people cares more about features in U-Boot proper. Everything
>>>>> before U-Boot proper, you can use TPL/SPL or alternative to use binary
>>>>> from vendor, as you can see all over the U-Boot mainline now, although
>>>>> we encourage people to use full open source TPL/SPL.
>>>>> Specifically for U-Boot Rockchip platform, I would like people to
>>>>> support not only U-Boot
>>>>> proper, but also for full SPL(ATF, OP-TEE support, itb image and other
>>>>> features)
>>>>> support. And for DRAM init,
>>>>> - if this belongs to SPL for this board, you must implement it or else
>>>>> SPL won't work;
>>>>> - if this does not belong to SPL for this board, you need implement full
>>>>> function SPL;
>>>>>     and you can either to have full function TPL with DRAM init(which is
>>>>> prefered)
>>>>>     or alternatively use binary loader from vendor.
>>>> This is not really a technical argument here, more of a policy argument
>>>> that ensures we have full free software support for the boards we
>>>> support, and not only half-cooked support (that will most likely never
>>>> be completed as soon as something that works gets merged). So it is a
>>>> strategical decision, not a strictly pragmatic one.
>>> While having full open source software support for boards is a noble
>>> goal, I think there should be some room for pragmatism here.  A
>>> significant number of u_boot targets rely on closed source components.
>>> In the particular case of RK3399 the situation is better than for
>>> other boards since you can combine the binary loader from the vendor
>>> with mainline U-Boot and mainline ATF to create a firmware where (as
>>> far as we can tell) no closed soure component remains active after
>>> U-Boot and ATF take over control.
>>>
>>>> I think reverting patches adding support for boards with no DRAM
>>>> configuration at all would send a message in the right direction here.
>> As a developer, I agree on this, but as a maintainer, I know too many
>> developers not able to do it and what most of developers need is other
>> features in U-Boot or SPL, and I would like the U-Boot mainline is more
>> active with more and more developers. So I'm afraid I agree with Mark
>> at this time for the policy.
> Maybe we need to provide tools ot make that process easier for everyone
> if it is really that hard. I don't really see what is so special about
> DRAM timings that would imply that a regular developer doing a U-Boot
> bringup couldn't figure things out, aside from the ability to dump said
> timings.
>
>> If all the other SoC platforms can have the same rule for DRAM init driver
>> is a mandatory instead of option, eg. brcom, qcom, mtk, omap, tegra, stm,
>> imx, aml, and all others, then I would very happy to follow the rule.
>> Rockchip is open for open source the DRAM driver, you have to know this
>> is the decision by the vendor, but not any of developers.
>> On rockchip platform, developers no need to concern about the DRAM
>> driver(which is pretty hard for most developers) because rockchip
>> already contribute it.
> Rockchip is indeed in a better position than other vendors where DRAM
> init may not be available (or when it's impossible to run U-Boot right
> after the bootrom and do the DRAM init itself because of e.g. abusive
> signature verification or lack of documentation).
>
> Since there is good DRAM support for Rockchip in place, we have an
> opportunity to push developers to do the right thing and contribute
> full support for the board. To me it is simply a matter of
> acknowledging that bootloader support for a board without DRAM init is
> not useful bootloader support. Since we have the code in place to
> support that, we can take the extra step and require that each board
> contribution be useful in that aspect.
>
>> For the time now, I know there will be full DRAM driver for rockchip SoC,
>> so the SoC/board support could be step by step:
>> U-Boot proper -> U-Boot + SPL(no DRAM init) ->U-Boot + SPL + TPL.
>>
>> As you can see the rockchip LPDDR4 driver send by Jagan, has 99 patches
>> in V2, you can't use static register configuration to do this, and maybe you
>> can't have a workable version if rockchip don't release it, but I don't
>> think it's
>> correct to make all those boards with lpddr4 float outside the mainline
>> support
>> because many developers are using the boards, they can only use vendor
>> branch
>> if the board not support by mainline.
> If mainline U-Boot can't support basic bootloader features such as DRAM
> initialization for these boards, I don't see the point in accepting
> support for them.
>
> It would be like submitting support for a board in Linux with a new CPU
> that is not supported and asking to boot Linux via a non-free shim
> before Linux to put the CPU in a legacy state that Linux can support.
> This would definitely not be okay and I don't see why the same
> shouldn't apply to U-Boot.

Linux build target is Image/zImage, if this Image not work with the board,
eg. hang somewhere during boot, then we say the board support is broken
and result in non-functional boot. And there always some kernel features
depends on bootloader setting, including security setting, some clock init,
some power supply and etc.

>
>> So I think merge those patches already make board work on mainline U-Boot
>> is pretty important for open source community.
> I don't think the patches make the boards work on mainline U-Boot since
> building and installing the resulting U-Boot binaries will result in a
> non-functional boot chain and brick the device. I don't think this is a
> good or safe idea.

In U-Boot, there are 2 or 3 standalone subsystem: U-Boot proper,
SPL, and TPL, and build target is u-boot.bin, u-boot-spl.bin, and
u-boot-tpl.bin.
If the standalone u-boot.bin or u-boot-spl.bin works good with a board and
able to boot into next stage correctly, I don't think these patches can
be consider
as "non-functional boot chain and brick the device". And for example for
armv8,
U-Boot is always as part of the boot chain.

Thanks,
- Kever
>
> Cheers,
>
> Paul
>
>> Thanks,
>> - Kever
>>> Frankly, I don't think that would help.  It would just drive more
>>> people to the vendor U-Boot that has more bugs and includes a vendor
>>> supplied ATF binary.
>>>
>>>>> I'm not sure if you have write a new dram driver for a board, but I know
>>>>> even the board vendor may not have the capability to write the DRAM
>>>>> driver, so this should not stop developers contribute to all other 99%
>>>>> features on U-Boot.
>>>> What they can do is run the non-free blob, dump the registers
>>>> afterwards and then use that in the DRAM configuration dtsi. Perhaps
>>>> one could write up a tool to ease the process if they think the process
>>>> is too much for a regular bringup.
>>>>
>>>> Most of the time, the DRAM chips are soldered so the calibrated values
>>>> have about no reason to change over time and can just be kept as-is.
>>>>
>>>> What do you think?
>>> Hopefully the pending diff to add support for other DRAM types beyond
>>> those that are already supported would make bring us a long way in
>>> that direction.  Maybe one of the existing timings will already work
>>> for the boards that are being discussed here.
>>>
>>
>