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

Fri Apr 26 16:53:47 UTC 2019

On Fri, Apr 26, 2019 at 10:10 PM Paul Kocialkowski
<paul.kocialkowski at bootlin.com> wrote:
>
> Hi,
>
> Le vendredi 26 avril 2019 à 21:46 +0530, Jagan Teki a écrit :
> > On Fri, Apr 26, 2019 at 9:38 PM 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.
> >
> > How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> > patch says it is an initial one and it is bit hard to add all at once.
> > ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> > unaffected to any of the existing dts files.
>
> Again, it's not a technical issue. Your proposal works and has no
> technical issue. The issue is in how the commits are grouped together.
> Patch series need to make logical sense. One patch series should
> accomplish one change, and each patch represents a step of that change.

It's about how you think. say if I wouldn't send the binman, I'm sure
this kind of discussion may not happen. In first mail you said
"something broken and how it can repair next" that indeed doesn't make
any sense of without understanding the whole series of changes.

Any new changes would come up with initial version, that what we
usually supported, ie what we did -u-boot.dtsi changes to i.MX6 and
developers are using the same is adding one after another.

>
> This is how upstream contributions work, and it's a powerful way to
> allow both efficient code review and good code quality. We want to keep
> things as simple, explicit and well-described as possible, so that
> things are easy for reviewers and as many people as possible can
> understand the issue and share their thoughts about it.

Yes, we adopt these principles and that what we are really working.

>
> It is all part of communication with others as part of a community.
> It is definitely an implicit rule that is not written down somewhere
> precisely, but that's the social contract between developers that work
> on a common software project.
>
> In that, contributing to upstream is different than baseline tech
> company standards, because you have to take extra steps to describe
> your work and explain it to others. You must make sure you give them
> all the comfort they need to painlessly understand what you did.

I hope all my communication was relevant to the topics, I can even
given the example how things were moved.

>
> It plays a great deal in having series accepted without going through
> very long discussions and numerous iterations. The less questions
> maintainers will have about your work, the better. They may disagree
> with the decisions you took, but these discussions are purely technical
> and can be resolved quickly.

Pure technical, yes ie what I thought off. no problem for me for long
discussion atleast people can understand how things went.

Jagan.