[PATCH v3 19/31] rockchip: Provide a VPL phase on rk3399

Tue Apr 1 19:56:59 CEST 2025

Hi Jonas,

On Wed, 2 Apr 2025 at 06:27, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2025-04-01 17:42, Simon Glass wrote:
> > Hi Jonas,
> >
> > On Sun, 30 Mar 2025 at 05:46, Jonas Karlman <jonas at kwiboo.se> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 2025-03-29 00:40, Simon Glass wrote:
> >>> Hi Tom,
> >>>
> >>> On Fri, 28 Mar 2025 at 10:16, Jonas Karlman <jonas at kwiboo.se> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 2025-03-28 16:34, Simon Glass wrote:
> >>>>> Add support for this new phase, which runs after TPL. It determines the
> >>>>> state of the machine, then selects which SPL image to use. SDRAM init is
> >>>>> then done in SPL, so that it is updatable.
> >>>>>
> >>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>>>> ---
> >>>>>
> >>>>> (no changes since v2)
> >>>>>
> >>>>> Changes in v2:
> >>>>> - Rewrite help for VPL_ROCKCHIP_COMMON_BOARD
> >>>>> - Skip spl-boot-order.c for VPL (rather than modifying it)
> >>>>>
> >>>>>  arch/arm/include/asm/spl.h               |   1 +
> >>>>>  arch/arm/mach-rockchip/Kconfig           |  25 +++++-
> >>>>>  arch/arm/mach-rockchip/Makefile          |  11 ++-
> >>>>>  arch/arm/mach-rockchip/rk3399/Kconfig    |   9 ++
> >>>>>  arch/arm/mach-rockchip/spl.c             |   3 +
> >>>>>  arch/arm/mach-rockchip/tpl.c             |   2 +-
> >>>>>  arch/arm/mach-rockchip/u-boot-vpl-v8.lds | 107 +++++++++++++++++++++++
> >>>>>  arch/arm/mach-rockchip/vpl.c             |  53 +++++++++++
> >>>>>  common/spl/Kconfig                       |   1 +
> >>>>>  9 files changed, 205 insertions(+), 7 deletions(-)
> >>>>>  create mode 100644 arch/arm/mach-rockchip/u-boot-vpl-v8.lds
> >>>>>  create mode 100644 arch/arm/mach-rockchip/vpl.c
> >>>>>
> >>>>
> >>>> This patch and therefore this series does not apply to the next branch.
> >>>>
> >>>> Are there any depends that are missing or are you only testing based on
> >>>> your own fork?
> >>>
> >>> Until we are ready to apply things, yes I am using the sjg tree. See
> >>> the tags in the cover letters of patches I send. For this one it is:
> >>>
> >>> base-commit: 3f76d803db9b500f43bc534465945a8d2836bb3e
> >>> branch: vbi3
> >>
> >> This base-commit does not even exist in your vbi3 branch :-/
> >
> > Are you sure? The base commit works fine for me:
> >
> > $ pe 3f76d803db9b500f43bc534465945a8d2836bb3e
> > 3f76d803db9 (ci/master) Merge branch 'ci' into 'master'
> > d07049b9f28 (el/ci) arm: Support a separate stack for VPL
> > abfff044f26 spl: Use CONFIG_VAL() to obtain the SPL stack
> > 7a032a125f5 spl: Add an SPL_HAVE_INIT_STACK option
> > 8d319b22019 tpl: Rename TPL_NEEDS_SEPARATE_STACK to TPL_HAVE_INIT_STACK
> > 794124b3258 Merge branch 'ci' into 'master'
> >
> > The branch name is a local branch, not necessarily pushed anywhere. Is
> > there some better terminology that I should use there? Perhaps
> > 'local-branch' ?
>
> Probably, I did a git fetch of you public vbi3 branch:
>
>   git fetch https://sjg.u-boot.org/u-boot/u-boot.git vbi3
>
> And that did not include 3f76d803db9b500f43bc534465945a8d2836bb3e:
>
>   fatal: bad object 3f76d803db9b500f43bc534465945a8d2836bb3e

Oh I see. You should just use 'git fetch' and get the whole tree.

>
> So yes, if you keep a public branch with same name as you say in your
> patches, I would expect it to be somewhat up to date.

OK I'll have to think about how to handle that. I used to name every
branch with a '-working' suffix, to make it clear that it can change
at any time. But I got tired of doing that in the end, so I stopped
for my new tree.

>
> >
> >>
> >>>
> >>> So no, there is nothing else that this series depends on and if you
> >>> can review it I can send a PR for Tom for -next one all necessary
> >>> changes are made.
> >>
> >> Sorry, this does not help, my main concern is testing that this does not
> >> break or affect anything for the remaining Rockchip boards. Something
> >> that is hard to test and validate when patches do not even apply to any
> >> of the mainline master or next branches.
> >>
> >> Sending patches based on a tree that has diverged with around 677 files
> >> changed, 20072 insertions(+), 17023 deletions(-), if my git fu is
> >> correct, and then expect us to test based on this diverged tree does not
> >> help with gaining confidence in this series.
> >>
> >> I can understand keeping a personal tree that will diverge at times,
> >> i.e. my personal tree has ~120-150 patches on top of mainline master.
> >> However, I always try to keep my work-in-progress branches rebased on
> >> top of master or next.
> >>
> >> For v4, please send a series based on mainline master or next branch,
> >> as stated in "General Patch Submission Rules" [1] in the U-Boot docs.
> >>
> >> If the series has some depends, I suggest you also publish a branch
> >> to a public repo with any limited depends applied on top of master or
> >> next along with the patches in your series.
> >
> > For now I am going to wait until you pick up your clean-up of part of
> > my series. Then I can pull that in any try to send the final VBE
> > series again.
>
> I sent out a "rockchip: binman: Use a template for FIT and other
> improvements" series [2] that picked up some of your rockchip binman
> image related patches.
>
> [2] https://patchwork.ozlabs.org/cover/2066701/
>
> >
> > I haven't had any rockchip patches rejected, so we should be able to
> > resolve this.
>
> They have not been properly reviewed (or tested) yet, as they have never
> applied cleanly on any mainline tree, with repeated requests to fix that.
>
> For me to review something, I want to be able to run the code, and run it
> on a code base as close as possible to where it intends to be merged.
>
> E.g. v1 was broken and had hidden depends that you had applied to your
> personal tree, and only later was sent out and now merged into mainline.
>
> I have some other concerns to validate, but have waited on a version
> that can be applied before continuing a review. Please consider this a
> NAK until it can properly be tested (i.e. reviewed) using a branch with
> a minimal diff to mainline master or next.

That's fine, but how about I hold off sending it until you pull in your series?

Regards,
Simon

>
> Regards,
> Jonas
>
> >
> >>
> >> [1] https://docs.u-boot.org/en/latest/develop/sending_patches.html