> On 26.04.2019, at 18:08, Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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. _______________________________________________ Linux-rockchip mailing list Linux-rockchip@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-rockchip
