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

Tue Apr 1 19:27:01 CEST 2025

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

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.

> 
>>
>>>
>>> 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.

Regards,
Jonas

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