Re: [PATCH v3 06/13] rockchip: rk3399: Add Orangepi RK3399 support

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

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@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@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?

Mixing logical changes (not to mention spreading them accross series)
is really a no-go and makes reviewing the patches very difficult, as
this thread perfectly illustrates.

It's really not against you personally, but there are such rules that
need to be followed in upstream contributions. They are at least as
essential as the underlying technical work.

Cheers,

Paul

> [1] https://patchwork.ozlabs.org/patch/1091534/
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot