[U-Boot] [PATCH v3 06/13] rockchip: rk3399: Add Orangepi RK3399 support

Fri Apr 26 16:09:07 UTC 2019

> On 26.04.2019, at 18:08, Paul Kocialkowski <paul.kocialkowski at bootlin.com> wrote:
> 
> Hi,
> 
> Le vendredi 26 avril 2019 à 21:18 +0530, Jagan Teki a écrit :
>> On Fri, Apr 26, 2019 at 9:08 PM Paul Kocialkowski
>> <paul.kocialkowski at bootlin.com> wrote:
>>> Hi,
>>> 
>>> Le vendredi 26 avril 2019 à 20:48 +0530, Jagan Teki a écrit :
>>>> On Fri, Apr 26, 2019 at 8:42 PM Paul Kocialkowski
>>>> <paul.kocialkowski at bootlin.com> wrote:
>>>>> Hi,
>>>>> 
>>>>> On Fri, 2019-04-26 at 20:31 +0530, Jagan Teki wrote:
>>>>>> On Fri, Apr 26, 2019 at 8:24 PM Paul Kocialkowski
>>>>>> <paul.kocialkowski at bootlin.com> wrote:
>>>>>>> Hi,
>>>>>>> 
>>>>>>> On Fri, 2019-04-26 at 20:15 +0530, Jagan Teki wrote:
>>>>>>>> On Fri, Apr 26, 2019 at 8:04 PM Paul Kocialkowski
>>>>>>>> <paul.kocialkowski at bootlin.com> wrote:
>>>>>>>>> Hi,
>>>>>>>>> 
>>>>>>>>> On Fri, 2019-04-26 at 19:57 +0530, Jagan Teki wrote:
>>>>>>>>>> On Fri, Apr 26, 2019 at 7:47 PM Paul Kocialkowski
>>>>>>>>>> <paul.kocialkowski at bootlin.com> wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>> 
>>>>>>>>>>> On Thu, 2019-04-25 at 23:04 +0530, Jagan Teki wrote:
>>>>>>>>>>>> Add initial support for Orangepi RK3399 board.
>>>>>>>>>>>> 
>>>>>>>>>>>> Specification
>>>>>>>>>>>> - Rockchip RK3399
>>>>>>>>>>>> - 2GB/4GB DDR3
>>>>>>>>>>>> - 16GB eMMC
>>>>>>>>>>>> - SD card slot
>>>>>>>>>>> 
>>>>>>>>>>> Looks like you're missing u-boot,dm-pre-reloc to have it working, which
>>>>>>>>>>> will need to be introduced when moving to rk3399-u-boot.dtsi.
>>>>>>>>>> 
>>>>>>>>>> Look like you are confused or doesn't check the patch. This patch have
>>>>>>>>>> rk3399-orangepi-u-boot.dtsi which included rk3399-u-boot.dtsi that has
>>>>>>>>>> u-boot,dm-pre-reloc for sdmmc.
>>>>>>>>> 
>>>>>>>>> Well no, in your v3.1 patch, you no longer have u-boot,dm-pre-reloc in
>>>>>>>>> rk3399-u-boot.dtsi, so you need to add it in the device dts and remove
>>>>>>>>> it in your second series when you add it back to rk3399-u-boot.dtsi.
>>>>>>>> 
>>>>>>>> Which u-boot,dm-pre-reloc are you taking about?
>>>>>>>> 
>>>>>>>> v3.1 has u-boot,dm-pre-reloc for sdmmc, please check it again [1]
>>>>>>>> which were used by subsequent boards on the same series.
>>>>>>>> 
>>>>>>>> The diff between v3 vs v3.1 is like v3 removed u-boot,dm-pre-reloc on
>>>>>>>> existing board dts files thought that it will include
>>>>>>>> rk3399-u-boot.dtsi will automatically, but not ie fixed in v3.1
>>>>>>>> 
>>>>>>>>> It's not okay to submit the board with broken MMC support and fix it in
>>>>>>>>> a subsequent series.
>>>>>>>> 
>>>>>>>> Again, nothing broken. existing boards or dts(i) files are untouched.
>>>>>>>> Added only initial rk3399-u-boot.dtsi with sdmmc u-boot,dm-pre-reloc
>>>>>>>> node to make use of new boards.  and the same reused by next series
>>>>>>>> so-that I can add binman global to all rk3399 boards.
>>>>>>> 
>>>>>>> Okay I think I'm getting there. So the Orangepi uses the new scheme
>>>>>>> (including rk3399-u-boot.dtsi which has u-boot,dm-pre-reloc) while all
>>>>>>> other boards are still using the previous scheme after the series.
>>>>>>> 
>>>>>>> This is very confusing and I really think you should keep u-boot,dm-
>>>>>>> pre-reloc in the orange pi dts file and then make the transition with
>>>>>>> all the other boards. You're mixing multiple logical steps which makes
>>>>>>> it really hard to understand what's going on.
>>>>>>> 
>>>>>>> More to that, introducing the rk3399-u-boot.dtsi is a logical step that
>>>>>>> should be grouped with your second series, not the first one. In your
>>>>>>> first series, boards have u-boot,dm-pre-reloc in their per-device dtsi.
>>>>>>> 
>>>>>>> Could you respin the two series to group changes by logic changes
>>>>>>> instead of the current state of interleaved changes?
>>>>>> 
>>>>>> Okay, to make it clear I can send v4 with v3.1 included and updated
>>>>>> commit log. but I the new series about binman remains same since the
>>>>>> changes on first patch on the series are relevant.
>>>>> 
>>>>> I think it makes no sense to introduce rk3399-u-boot.dts as part of the
>>>>> series currently in v3. You need to remove the patch (currently v3.1)
>>>>> from this series and group it with the new series (binman-related).
>>>>> 
>>>>> Otherwise, you're mixing two unrelated logical changes across two
>>>>> series.
>>>> 
>>>> rk3399-u-boot.dtsi is require for sdboot on that boards, whole idea is
>>>> to untouch Linux dts files and get introduced initial
>>>> rk3399-u-boot.dtsi which doesn't effect any board dts files but use
>>>> new boards.
>>> 
>>> Yes I think I properly understand that now. The problem is that
>>> currently, rk3399 boards do not use rk3399-u-boot.dtsi. In the first
>>> series, you are introducing the file and including it in the newly-
>>> introduced devices, but not in the previous ones. This is inconsistent.
>>> 
>>> Then, in the second series, you are moving existing boards to using
>>> rk3399-u-boot.dtsi, except for the boards you introduced in the first
>>> series. This is inconsistent.
>>> 
>>> Do you see the problem? I feel like you are missing it and only
>>> focusing on the fact that the series works in practice, which is not
>>> the issue.
>> 
>> Not again.
>> 
>> First series, I get introduced rk3399-u-boot.dtsi, and only the new
>> boards are using this file and next series rest of the boards are
>> using which indeed a valid step. what is inconsistent here, I don't
>> understand.
> 
> Yes, what you are describing is exactly the issue. It does not make
> sense to introduce a new common u-boot dtsi for rk3399 and add new
> devices that use this file *before* switching existing devices to the
> new common u-boot dtsi for rk3399 in the same series.

I am with Paul on this one.

Thanks,
Philipp.

> 
> Globally with your series, you are doing 3 things:
> - Introducing a common RK3399 u-boot dtsi with the required pre-reloc
> entries;
> - Adding new RK3399 devices and syncing the dtsi files from Linux;
> - Introducing u-boot-rockchip-with-spl.bin.
> 
> So it should be 3 distinct series, not two. As you can see, you can
> either do step 1 and then step 2 or step 2 and then step 1, but you
> cannot mix parts of step 1 into step 2 and vice-versa.
> 
> Is it clearer explained this way?
> 
> Cheers,
> 
> Paul
> 
>>>> rk3399-u-boot.dtsi is required for binman for adding binman changes,
>>>> and indeed binman series has some issues which can take time to merge
>>>> and other one can do it before.
>>> 
>>> Yes, it can be done before in the first series (so things will not
>>> break), but it should not be done that way for the reason I'm talking
>>> about. Again, the way changes are split and organized is as import as
>>> the technical changes. It's not sufficient that the series works, it
>>> also has to make sense by grouping logical changes.
>> 
>> Don't agree your point, let me send v4 will see what Philipp and other
>> can think off.
>> 
>> thanks for your comments,
>> Jagan.