[PATCH 1/3] rockchip: Drop bootstage stash in TPL and SPL

[PATCH 1/3] rockchip: Drop bootstage stash in TPL and SPL

[Simon Glass]

Unfortunately the IRAM used to stash the bootstage records in TPL
becomes accessible after SPL runs. Presumably this is because of ATF
taking it over.

We could move the stash to another address in SPL, before passing it to
U-Boot proper. But it seems easier to wait until we have support for
standard passage[1] which should not be too far away.

For now, disable it in TPL and SPL.

[1] https://patchwork.ozlabs.org/project/uboot/cover/
    20220117150428.1580273-1-sjg@chromium.org/

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 configs/rockpro64-rk3399_defconfig | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
index dd67f9dff64..49614236819 100644
--- a/configs/rockpro64-rk3399_defconfig
+++ b/configs/rockpro64-rk3399_defconfig
@@ -13,7 +13,6 @@ CONFIG_DM_RESET=y
 CONFIG_ROCKCHIP_RK3399=y
 CONFIG_TARGET_ROCKPRO64_RK3399=y
 CONFIG_SPL_STACK=0x400000
-CONFIG_BOOTSTAGE_STASH_ADDR=0xff8e0000
 CONFIG_DEBUG_UART_BASE=0xFF1A0000
 CONFIG_DEBUG_UART_CLOCK=24000000
 CONFIG_SPL_SPI_FLASH_SUPPORT=y
@@ -21,11 +20,7 @@ CONFIG_SPL_SPI=y
 CONFIG_SYS_LOAD_ADDR=0x800800
 CONFIG_DEBUG_UART=y
 CONFIG_BOOTSTAGE=y
-CONFIG_SPL_BOOTSTAGE=y
-CONFIG_TPL_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
-CONFIG_SPL_BOOTSTAGE_RECORD_COUNT=10
-CONFIG_BOOTSTAGE_STASH=y
 CONFIG_USE_PREBOOT=y
 CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-rockpro64.dtb"
 CONFIG_DISPLAY_BOARDINFO_LATE=y
-- 
2.39.2.637.g21b0678d19-goog

[PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Simon Glass]

This board has moved to standard boot but the old 'distro_bootcmd'
command is still active. Disable DISTRO_DEFAULTS to fix this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 configs/rockpro64-rk3399_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
index 49614236819..fe2415c87c9 100644
--- a/configs/rockpro64-rk3399_defconfig
+++ b/configs/rockpro64-rk3399_defconfig
@@ -19,6 +19,7 @@ CONFIG_SPL_SPI_FLASH_SUPPORT=y
 CONFIG_SPL_SPI=y
 CONFIG_SYS_LOAD_ADDR=0x800800
 CONFIG_DEBUG_UART=y
+# CONFIG_DISTRO_DEFAULTS is not set
 CONFIG_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_USE_PREBOOT=y
-- 
2.39.2.637.g21b0678d19-goog

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On 2023-02-21, Simon Glass wrote:
> This board has moved to standard boot but the old 'distro_bootcmd'
> command is still active. Disable DISTRO_DEFAULTS to fix this.

Works for booting rockpro64-rk3399, thanks!

Tested-by: Vagrant Cascadian <vagrant@debian.org>

> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  configs/rockpro64-rk3399_defconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> index 49614236819..fe2415c87c9 100644
> --- a/configs/rockpro64-rk3399_defconfig
> +++ b/configs/rockpro64-rk3399_defconfig
> @@ -19,6 +19,7 @@ CONFIG_SPL_SPI_FLASH_SUPPORT=y
>  CONFIG_SPL_SPI=y
>  CONFIG_SYS_LOAD_ADDR=0x800800
>  CONFIG_DEBUG_UART=y
> +# CONFIG_DISTRO_DEFAULTS is not set
>  CONFIG_BOOTSTAGE=y
>  CONFIG_BOOTSTAGE_REPORT=y
>  CONFIG_USE_PREBOOT=y
> -- 
> 2.39.2.637.g21b0678d19-goog

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

On 2023-02-21, Vagrant Cascadian wrote:
> On 2023-02-21, Simon Glass wrote:
>> This board has moved to standard boot but the old 'distro_bootcmd'
>> command is still active. Disable DISTRO_DEFAULTS to fix this.
>
> Works for booting rockpro64-rk3399, thanks!

I can also confirm that applying a very similar patch for
pinebook-pro-rk3399 works booting with bootstd.

Seems worth adding if there is a v2 of the patch series.

Alternately, rather than doing this on a board-by-board basis, is there
some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
BOOTSTD?

live well,
  vagrant

>>  configs/rockpro64-rk3399_defconfig | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
>> index 49614236819..fe2415c87c9 100644
>> --- a/configs/rockpro64-rk3399_defconfig
>> +++ b/configs/rockpro64-rk3399_defconfig
>> @@ -19,6 +19,7 @@ CONFIG_SPL_SPI_FLASH_SUPPORT=y
>>  CONFIG_SPL_SPI=y
>>  CONFIG_SYS_LOAD_ADDR=0x800800
>>  CONFIG_DEBUG_UART=y
>> +# CONFIG_DISTRO_DEFAULTS is not set
>>  CONFIG_BOOTSTAGE=y
>>  CONFIG_BOOTSTAGE_REPORT=y
>>  CONFIG_USE_PREBOOT=y
>> -- 
>> 2.39.2.637.g21b0678d19-goog

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
> On 2023-02-21, Vagrant Cascadian wrote:
> > On 2023-02-21, Simon Glass wrote:
> >> This board has moved to standard boot but the old 'distro_bootcmd'
> >> command is still active. Disable DISTRO_DEFAULTS to fix this.
> >
> > Works for booting rockpro64-rk3399, thanks!
> 
> I can also confirm that applying a very similar patch for
> pinebook-pro-rk3399 works booting with bootstd.
> 
> Seems worth adding if there is a v2 of the patch series.
> 
> Alternately, rather than doing this on a board-by-board basis, is there
> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
> BOOTSTD?

I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
DISTRO_DEFAULTS method of select not imply for most things is also I
suspect the right path.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Jonas Karlman]

On 2023-02-22 19:24, Tom Rini wrote:
> On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
>> On 2023-02-21, Vagrant Cascadian wrote:
>>> On 2023-02-21, Simon Glass wrote:
>>>> This board has moved to standard boot but the old 'distro_bootcmd'
>>>> command is still active. Disable DISTRO_DEFAULTS to fix this.
>>>
>>> Works for booting rockpro64-rk3399, thanks!
>>
>> I can also confirm that applying a very similar patch for
>> pinebook-pro-rk3399 works booting with bootstd.
>>
>> Seems worth adding if there is a v2 of the patch series.
>>
>> Alternately, rather than doing this on a board-by-board basis, is there
>> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
>> BOOTSTD?
> 
> I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
> BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
> DISTRO_DEFAULTS method of select not imply for most things is also I
> suspect the right path.
> 

Another option would be to set BOOTSTD_BOOTCOMMAND=y, this is the main
issue with rk3399 at the moment, distro_bootcmd got removed from env,
yet bootcmd is still "run distro_bootcmd" due to DISTRO_DEFAULTS=y.


config BOOTCOMMAND
	default "bootflow scan -lb" if BOOTSTD_BOOTCOMMAND && CMD_BOOTFLOW_FULL
	default "bootflow scan" if BOOTSTD_BOOTCOMMAND && !CMD_BOOTFLOW_FULL
	default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS

config BOOTSTD_BOOTCOMMAND
	default y if !DISTRO_DEFAULTS


Regards,
Jonas

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Simon Glass]

Hi,

On Wed, 22 Feb 2023 at 11:56, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2023-02-22 19:24, Tom Rini wrote:
> > On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
> >> On 2023-02-21, Vagrant Cascadian wrote:
> >>> On 2023-02-21, Simon Glass wrote:
> >>>> This board has moved to standard boot but the old 'distro_bootcmd'
> >>>> command is still active. Disable DISTRO_DEFAULTS to fix this.
> >>>
> >>> Works for booting rockpro64-rk3399, thanks!
> >>
> >> I can also confirm that applying a very similar patch for
> >> pinebook-pro-rk3399 works booting with bootstd.
> >>
> >> Seems worth adding if there is a v2 of the patch series.
> >>
> >> Alternately, rather than doing this on a board-by-board basis, is there
> >> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
> >> BOOTSTD?
> >
> > I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
> > BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
> > DISTRO_DEFAULTS method of select not imply for most things is also I
> > suspect the right path.
> >
>
> Another option would be to set BOOTSTD_BOOTCOMMAND=y, this is the main
> issue with rk3399 at the moment, distro_bootcmd got removed from env,
> yet bootcmd is still "run distro_bootcmd" due to DISTRO_DEFAULTS=y.
>
>
> config BOOTCOMMAND
>         default "bootflow scan -lb" if BOOTSTD_BOOTCOMMAND && CMD_BOOTFLOW_FULL
>         default "bootflow scan" if BOOTSTD_BOOTCOMMAND && !CMD_BOOTFLOW_FULL
>         default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
>
> config BOOTSTD_BOOTCOMMAND
>         default y if !DISTRO_DEFAULTS

That seems reasonable to me, along with a 'depends on BOOTSTD'

At present DISTRO_DEFAULTS is both enabled in defconfig files (about
350) and Kconfig (another 300). For rockchip it is the latter.

It doesn't really make sense to use DISTRO_DEFAULTS when using
BOOTSTD_DEFAULTS. One is 'run distro_bootcmd' and the other is
'bootflow scan', possibly with -lb flags. But we really don't want
people flipping back and forward.

At present, if both are enabled, then DISTRO_DEFAULTS takes
precedence. I think it makes some sense this way, since
DISTRO_DEFAULTS is the status quo and moving to BOOTSTD_DEFAULTS is
being done over time. I just sent a rpi3 series. I can perhaps look at
doing all of rockchip as well, including pinebook-pro. That would
provide some amount of testing once it is in -next.

I'd quite like to get rid of DISTRO_DEFAULTS entirely, perhaps in the
release after this one?

Regards,
Simon

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

On Wed, Feb 22, 2023 at 11:58:37AM -0700, Simon Glass wrote:
> Hi,
> 
> On Wed, 22 Feb 2023 at 11:56, Jonas Karlman <jonas@kwiboo.se> wrote:
> >
> > On 2023-02-22 19:24, Tom Rini wrote:
> > > On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
> > >> On 2023-02-21, Vagrant Cascadian wrote:
> > >>> On 2023-02-21, Simon Glass wrote:
> > >>>> This board has moved to standard boot but the old 'distro_bootcmd'
> > >>>> command is still active. Disable DISTRO_DEFAULTS to fix this.
> > >>>
> > >>> Works for booting rockpro64-rk3399, thanks!
> > >>
> > >> I can also confirm that applying a very similar patch for
> > >> pinebook-pro-rk3399 works booting with bootstd.
> > >>
> > >> Seems worth adding if there is a v2 of the patch series.
> > >>
> > >> Alternately, rather than doing this on a board-by-board basis, is there
> > >> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
> > >> BOOTSTD?
> > >
> > > I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
> > > BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
> > > DISTRO_DEFAULTS method of select not imply for most things is also I
> > > suspect the right path.
> > >
> >
> > Another option would be to set BOOTSTD_BOOTCOMMAND=y, this is the main
> > issue with rk3399 at the moment, distro_bootcmd got removed from env,
> > yet bootcmd is still "run distro_bootcmd" due to DISTRO_DEFAULTS=y.
> >
> >
> > config BOOTCOMMAND
> >         default "bootflow scan -lb" if BOOTSTD_BOOTCOMMAND && CMD_BOOTFLOW_FULL
> >         default "bootflow scan" if BOOTSTD_BOOTCOMMAND && !CMD_BOOTFLOW_FULL
> >         default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
> >
> > config BOOTSTD_BOOTCOMMAND
> >         default y if !DISTRO_DEFAULTS
> 
> That seems reasonable to me, along with a 'depends on BOOTSTD'
> 
> At present DISTRO_DEFAULTS is both enabled in defconfig files (about
> 350) and Kconfig (another 300). For rockchip it is the latter.
> 
> It doesn't really make sense to use DISTRO_DEFAULTS when using
> BOOTSTD_DEFAULTS. One is 'run distro_bootcmd' and the other is
> 'bootflow scan', possibly with -lb flags. But we really don't want
> people flipping back and forward.

It doesn't make linguistic sense to have "distro defaults" and "bootstd
defaults" enabled.  It makes compute sense to have these two symbols
combined as they're nearly identical and have a similar conceptual need
(make sure we have things like bootz/booti/ext4/etc support).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Simon Glass]

Hi Tom,

On Wed, 22 Feb 2023 at 12:01, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Feb 22, 2023 at 11:58:37AM -0700, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 22 Feb 2023 at 11:56, Jonas Karlman <jonas@kwiboo.se> wrote:
> > >
> > > On 2023-02-22 19:24, Tom Rini wrote:
> > > > On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
> > > >> On 2023-02-21, Vagrant Cascadian wrote:
> > > >>> On 2023-02-21, Simon Glass wrote:
> > > >>>> This board has moved to standard boot but the old 'distro_bootcmd'
> > > >>>> command is still active. Disable DISTRO_DEFAULTS to fix this.
> > > >>>
> > > >>> Works for booting rockpro64-rk3399, thanks!
> > > >>
> > > >> I can also confirm that applying a very similar patch for
> > > >> pinebook-pro-rk3399 works booting with bootstd.
> > > >>
> > > >> Seems worth adding if there is a v2 of the patch series.
> > > >>
> > > >> Alternately, rather than doing this on a board-by-board basis, is there
> > > >> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
> > > >> BOOTSTD?
> > > >
> > > > I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
> > > > BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
> > > > DISTRO_DEFAULTS method of select not imply for most things is also I
> > > > suspect the right path.
> > > >
> > >
> > > Another option would be to set BOOTSTD_BOOTCOMMAND=y, this is the main
> > > issue with rk3399 at the moment, distro_bootcmd got removed from env,
> > > yet bootcmd is still "run distro_bootcmd" due to DISTRO_DEFAULTS=y.
> > >
> > >
> > > config BOOTCOMMAND
> > >         default "bootflow scan -lb" if BOOTSTD_BOOTCOMMAND && CMD_BOOTFLOW_FULL
> > >         default "bootflow scan" if BOOTSTD_BOOTCOMMAND && !CMD_BOOTFLOW_FULL
> > >         default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
> > >
> > > config BOOTSTD_BOOTCOMMAND
> > >         default y if !DISTRO_DEFAULTS
> >
> > That seems reasonable to me, along with a 'depends on BOOTSTD'
> >
> > At present DISTRO_DEFAULTS is both enabled in defconfig files (about
> > 350) and Kconfig (another 300). For rockchip it is the latter.
> >
> > It doesn't really make sense to use DISTRO_DEFAULTS when using
> > BOOTSTD_DEFAULTS. One is 'run distro_bootcmd' and the other is
> > 'bootflow scan', possibly with -lb flags. But we really don't want
> > people flipping back and forward.
>
> It doesn't make linguistic sense to have "distro defaults" and "bootstd
> defaults" enabled.  It makes compute sense to have these two symbols
> combined as they're nearly identical and have a similar conceptual need
> (make sure we have things like bootz/booti/ext4/etc support).

Oh, so you are talking about DISTRO_DEFAULTS, not the actual command?
In that case I can do a patch to make them both use a common symbol to
bring in most stuff, although bootstd does not need hush and a few
other things that are needed by distro_bootcmd.

Let me know if that makes sense.

Regards,
Simon

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3500 bytes --]

On Wed, Feb 22, 2023 at 12:03:58PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 22 Feb 2023 at 12:01, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Feb 22, 2023 at 11:58:37AM -0700, Simon Glass wrote:
> > > Hi,
> > >
> > > On Wed, 22 Feb 2023 at 11:56, Jonas Karlman <jonas@kwiboo.se> wrote:
> > > >
> > > > On 2023-02-22 19:24, Tom Rini wrote:
> > > > > On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
> > > > >> On 2023-02-21, Vagrant Cascadian wrote:
> > > > >>> On 2023-02-21, Simon Glass wrote:
> > > > >>>> This board has moved to standard boot but the old 'distro_bootcmd'
> > > > >>>> command is still active. Disable DISTRO_DEFAULTS to fix this.
> > > > >>>
> > > > >>> Works for booting rockpro64-rk3399, thanks!
> > > > >>
> > > > >> I can also confirm that applying a very similar patch for
> > > > >> pinebook-pro-rk3399 works booting with bootstd.
> > > > >>
> > > > >> Seems worth adding if there is a v2 of the patch series.
> > > > >>
> > > > >> Alternately, rather than doing this on a board-by-board basis, is there
> > > > >> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
> > > > >> BOOTSTD?
> > > > >
> > > > > I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
> > > > > BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
> > > > > DISTRO_DEFAULTS method of select not imply for most things is also I
> > > > > suspect the right path.
> > > > >
> > > >
> > > > Another option would be to set BOOTSTD_BOOTCOMMAND=y, this is the main
> > > > issue with rk3399 at the moment, distro_bootcmd got removed from env,
> > > > yet bootcmd is still "run distro_bootcmd" due to DISTRO_DEFAULTS=y.
> > > >
> > > >
> > > > config BOOTCOMMAND
> > > >         default "bootflow scan -lb" if BOOTSTD_BOOTCOMMAND && CMD_BOOTFLOW_FULL
> > > >         default "bootflow scan" if BOOTSTD_BOOTCOMMAND && !CMD_BOOTFLOW_FULL
> > > >         default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
> > > >
> > > > config BOOTSTD_BOOTCOMMAND
> > > >         default y if !DISTRO_DEFAULTS
> > >
> > > That seems reasonable to me, along with a 'depends on BOOTSTD'
> > >
> > > At present DISTRO_DEFAULTS is both enabled in defconfig files (about
> > > 350) and Kconfig (another 300). For rockchip it is the latter.
> > >
> > > It doesn't really make sense to use DISTRO_DEFAULTS when using
> > > BOOTSTD_DEFAULTS. One is 'run distro_bootcmd' and the other is
> > > 'bootflow scan', possibly with -lb flags. But we really don't want
> > > people flipping back and forward.
> >
> > It doesn't make linguistic sense to have "distro defaults" and "bootstd
> > defaults" enabled.  It makes compute sense to have these two symbols
> > combined as they're nearly identical and have a similar conceptual need
> > (make sure we have things like bootz/booti/ext4/etc support).
> 
> Oh, so you are talking about DISTRO_DEFAULTS, not the actual command?
> In that case I can do a patch to make them both use a common symbol to
> bring in most stuff, although bootstd does not need hush and a few
> other things that are needed by distro_bootcmd.
> 
> Let me know if that makes sense.

Yeah, the Kconfig symbols DISTRO_DEFAULTS and BOOTSTD_DEFAULTS need to
be reconciled and combined. This is separate from setting bootcmd to
"run distro_bootcmd", which should be separate, and yes, not be the
chosen symbol when we have bootstd enabled.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Peter Robinson]

On Wed, Feb 22, 2023 at 7:17 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Feb 22, 2023 at 12:03:58PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 22 Feb 2023 at 12:01, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Feb 22, 2023 at 11:58:37AM -0700, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Wed, 22 Feb 2023 at 11:56, Jonas Karlman <jonas@kwiboo.se> wrote:
> > > > >
> > > > > On 2023-02-22 19:24, Tom Rini wrote:
> > > > > > On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
> > > > > >> On 2023-02-21, Vagrant Cascadian wrote:
> > > > > >>> On 2023-02-21, Simon Glass wrote:
> > > > > >>>> This board has moved to standard boot but the old 'distro_bootcmd'
> > > > > >>>> command is still active. Disable DISTRO_DEFAULTS to fix this.
> > > > > >>>
> > > > > >>> Works for booting rockpro64-rk3399, thanks!
> > > > > >>
> > > > > >> I can also confirm that applying a very similar patch for
> > > > > >> pinebook-pro-rk3399 works booting with bootstd.
> > > > > >>
> > > > > >> Seems worth adding if there is a v2 of the patch series.
> > > > > >>
> > > > > >> Alternately, rather than doing this on a board-by-board basis, is there
> > > > > >> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
> > > > > >> BOOTSTD?
> > > > > >
> > > > > > I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
> > > > > > BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
> > > > > > DISTRO_DEFAULTS method of select not imply for most things is also I
> > > > > > suspect the right path.
> > > > > >
> > > > >
> > > > > Another option would be to set BOOTSTD_BOOTCOMMAND=y, this is the main
> > > > > issue with rk3399 at the moment, distro_bootcmd got removed from env,
> > > > > yet bootcmd is still "run distro_bootcmd" due to DISTRO_DEFAULTS=y.
> > > > >
> > > > >
> > > > > config BOOTCOMMAND
> > > > >         default "bootflow scan -lb" if BOOTSTD_BOOTCOMMAND && CMD_BOOTFLOW_FULL
> > > > >         default "bootflow scan" if BOOTSTD_BOOTCOMMAND && !CMD_BOOTFLOW_FULL
> > > > >         default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
> > > > >
> > > > > config BOOTSTD_BOOTCOMMAND
> > > > >         default y if !DISTRO_DEFAULTS
> > > >
> > > > That seems reasonable to me, along with a 'depends on BOOTSTD'
> > > >
> > > > At present DISTRO_DEFAULTS is both enabled in defconfig files (about
> > > > 350) and Kconfig (another 300). For rockchip it is the latter.
> > > >
> > > > It doesn't really make sense to use DISTRO_DEFAULTS when using
> > > > BOOTSTD_DEFAULTS. One is 'run distro_bootcmd' and the other is
> > > > 'bootflow scan', possibly with -lb flags. But we really don't want
> > > > people flipping back and forward.
> > >
> > > It doesn't make linguistic sense to have "distro defaults" and "bootstd
> > > defaults" enabled.  It makes compute sense to have these two symbols
> > > combined as they're nearly identical and have a similar conceptual need
> > > (make sure we have things like bootz/booti/ext4/etc support).
> >
> > Oh, so you are talking about DISTRO_DEFAULTS, not the actual command?
> > In that case I can do a patch to make them both use a common symbol to
> > bring in most stuff, although bootstd does not need hush and a few
> > other things that are needed by distro_bootcmd.
> >
> > Let me know if that makes sense.
>
> Yeah, the Kconfig symbols DISTRO_DEFAULTS and BOOTSTD_DEFAULTS need to
> be reconciled and combined. This is separate from setting bootcmd to
> "run distro_bootcmd", which should be separate, and yes, not be the
> chosen symbol when we have bootstd enabled.

What came out of this decision, the first other rk3399 device I tried,
the rock960, is also broken booting. We really can't have devices
randomly broken all the time and I don't have the time or the devices
to go through and test everything. We should really be getting better
at these mass changes, it doesn't lead to a great user experience!

Peter

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4618 bytes --]

On Thu, Mar 16, 2023 at 11:01:01AM +0000, Peter Robinson wrote:
> On Wed, Feb 22, 2023 at 7:17 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Feb 22, 2023 at 12:03:58PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 22 Feb 2023 at 12:01, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Feb 22, 2023 at 11:58:37AM -0700, Simon Glass wrote:
> > > > > Hi,
> > > > >
> > > > > On Wed, 22 Feb 2023 at 11:56, Jonas Karlman <jonas@kwiboo.se> wrote:
> > > > > >
> > > > > > On 2023-02-22 19:24, Tom Rini wrote:
> > > > > > > On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
> > > > > > >> On 2023-02-21, Vagrant Cascadian wrote:
> > > > > > >>> On 2023-02-21, Simon Glass wrote:
> > > > > > >>>> This board has moved to standard boot but the old 'distro_bootcmd'
> > > > > > >>>> command is still active. Disable DISTRO_DEFAULTS to fix this.
> > > > > > >>>
> > > > > > >>> Works for booting rockpro64-rk3399, thanks!
> > > > > > >>
> > > > > > >> I can also confirm that applying a very similar patch for
> > > > > > >> pinebook-pro-rk3399 works booting with bootstd.
> > > > > > >>
> > > > > > >> Seems worth adding if there is a v2 of the patch series.
> > > > > > >>
> > > > > > >> Alternately, rather than doing this on a board-by-board basis, is there
> > > > > > >> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
> > > > > > >> BOOTSTD?
> > > > > > >
> > > > > > > I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
> > > > > > > BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
> > > > > > > DISTRO_DEFAULTS method of select not imply for most things is also I
> > > > > > > suspect the right path.
> > > > > > >
> > > > > >
> > > > > > Another option would be to set BOOTSTD_BOOTCOMMAND=y, this is the main
> > > > > > issue with rk3399 at the moment, distro_bootcmd got removed from env,
> > > > > > yet bootcmd is still "run distro_bootcmd" due to DISTRO_DEFAULTS=y.
> > > > > >
> > > > > >
> > > > > > config BOOTCOMMAND
> > > > > >         default "bootflow scan -lb" if BOOTSTD_BOOTCOMMAND && CMD_BOOTFLOW_FULL
> > > > > >         default "bootflow scan" if BOOTSTD_BOOTCOMMAND && !CMD_BOOTFLOW_FULL
> > > > > >         default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
> > > > > >
> > > > > > config BOOTSTD_BOOTCOMMAND
> > > > > >         default y if !DISTRO_DEFAULTS
> > > > >
> > > > > That seems reasonable to me, along with a 'depends on BOOTSTD'
> > > > >
> > > > > At present DISTRO_DEFAULTS is both enabled in defconfig files (about
> > > > > 350) and Kconfig (another 300). For rockchip it is the latter.
> > > > >
> > > > > It doesn't really make sense to use DISTRO_DEFAULTS when using
> > > > > BOOTSTD_DEFAULTS. One is 'run distro_bootcmd' and the other is
> > > > > 'bootflow scan', possibly with -lb flags. But we really don't want
> > > > > people flipping back and forward.
> > > >
> > > > It doesn't make linguistic sense to have "distro defaults" and "bootstd
> > > > defaults" enabled.  It makes compute sense to have these two symbols
> > > > combined as they're nearly identical and have a similar conceptual need
> > > > (make sure we have things like bootz/booti/ext4/etc support).
> > >
> > > Oh, so you are talking about DISTRO_DEFAULTS, not the actual command?
> > > In that case I can do a patch to make them both use a common symbol to
> > > bring in most stuff, although bootstd does not need hush and a few
> > > other things that are needed by distro_bootcmd.
> > >
> > > Let me know if that makes sense.
> >
> > Yeah, the Kconfig symbols DISTRO_DEFAULTS and BOOTSTD_DEFAULTS need to
> > be reconciled and combined. This is separate from setting bootcmd to
> > "run distro_bootcmd", which should be separate, and yes, not be the
> > chosen symbol when we have bootstd enabled.
> 
> What came out of this decision, the first other rk3399 device I tried,
> the rock960, is also broken booting. We really can't have devices
> randomly broken all the time and I don't have the time or the devices
> to go through and test everything. We should really be getting better
> at these mass changes, it doesn't lead to a great user experience!

Part of the problem is that Rockchip had a few issues at the end of the
last cycle and then the start of this one. Another issue is that I don't
have a Rockchip platform in my CI loop, but I'm hoping to fix that in
the next week or so.  Is top of tree working on your platforms or no?
They should be, at this point.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] rockchip: Disable DISTRO_DEFAULTS for rockpro64

[Simon Glass]

Hi,

On Thu, 16 Mar 2023 at 12:17, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Mar 16, 2023 at 11:01:01AM +0000, Peter Robinson wrote:
> > On Wed, Feb 22, 2023 at 7:17 PM Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Feb 22, 2023 at 12:03:58PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 22 Feb 2023 at 12:01, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Feb 22, 2023 at 11:58:37AM -0700, Simon Glass wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Wed, 22 Feb 2023 at 11:56, Jonas Karlman <jonas@kwiboo.se> wrote:
> > > > > > >
> > > > > > > On 2023-02-22 19:24, Tom Rini wrote:
> > > > > > > > On Wed, Feb 22, 2023 at 10:19:03AM -0800, Vagrant Cascadian wrote:
> > > > > > > >> On 2023-02-21, Vagrant Cascadian wrote:
> > > > > > > >>> On 2023-02-21, Simon Glass wrote:
> > > > > > > >>>> This board has moved to standard boot but the old 'distro_bootcmd'
> > > > > > > >>>> command is still active. Disable DISTRO_DEFAULTS to fix this.
> > > > > > > >>>
> > > > > > > >>> Works for booting rockpro64-rk3399, thanks!
> > > > > > > >>
> > > > > > > >> I can also confirm that applying a very similar patch for
> > > > > > > >> pinebook-pro-rk3399 works booting with bootstd.
> > > > > > > >>
> > > > > > > >> Seems worth adding if there is a v2 of the patch series.
> > > > > > > >>
> > > > > > > >> Alternately, rather than doing this on a board-by-board basis, is there
> > > > > > > >> some way to disable CONFIG_DISTRO_DEFAULTS when a board is using
> > > > > > > >> BOOTSTD?
> > > > > > > >
> > > > > > > > I think that's possibly a bit dangerous? DISTRO_DEFAULTS and
> > > > > > > > BOOTSTD_DEFAULTS need to be reconciled to a single new symbol, and the
> > > > > > > > DISTRO_DEFAULTS method of select not imply for most things is also I
> > > > > > > > suspect the right path.
> > > > > > > >
> > > > > > >
> > > > > > > Another option would be to set BOOTSTD_BOOTCOMMAND=y, this is the main
> > > > > > > issue with rk3399 at the moment, distro_bootcmd got removed from env,
> > > > > > > yet bootcmd is still "run distro_bootcmd" due to DISTRO_DEFAULTS=y.
> > > > > > >
> > > > > > >
> > > > > > > config BOOTCOMMAND
> > > > > > >         default "bootflow scan -lb" if BOOTSTD_BOOTCOMMAND && CMD_BOOTFLOW_FULL
> > > > > > >         default "bootflow scan" if BOOTSTD_BOOTCOMMAND && !CMD_BOOTFLOW_FULL
> > > > > > >         default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
> > > > > > >
> > > > > > > config BOOTSTD_BOOTCOMMAND
> > > > > > >         default y if !DISTRO_DEFAULTS
> > > > > >
> > > > > > That seems reasonable to me, along with a 'depends on BOOTSTD'
> > > > > >
> > > > > > At present DISTRO_DEFAULTS is both enabled in defconfig files (about
> > > > > > 350) and Kconfig (another 300). For rockchip it is the latter.
> > > > > >
> > > > > > It doesn't really make sense to use DISTRO_DEFAULTS when using
> > > > > > BOOTSTD_DEFAULTS. One is 'run distro_bootcmd' and the other is
> > > > > > 'bootflow scan', possibly with -lb flags. But we really don't want
> > > > > > people flipping back and forward.
> > > > >
> > > > > It doesn't make linguistic sense to have "distro defaults" and "bootstd
> > > > > defaults" enabled.  It makes compute sense to have these two symbols
> > > > > combined as they're nearly identical and have a similar conceptual need
> > > > > (make sure we have things like bootz/booti/ext4/etc support).
> > > >
> > > > Oh, so you are talking about DISTRO_DEFAULTS, not the actual command?
> > > > In that case I can do a patch to make them both use a common symbol to
> > > > bring in most stuff, although bootstd does not need hush and a few
> > > > other things that are needed by distro_bootcmd.
> > > >
> > > > Let me know if that makes sense.
> > >
> > > Yeah, the Kconfig symbols DISTRO_DEFAULTS and BOOTSTD_DEFAULTS need to
> > > be reconciled and combined. This is separate from setting bootcmd to
> > > "run distro_bootcmd", which should be separate, and yes, not be the
> > > chosen symbol when we have bootstd enabled.
> >
> > What came out of this decision, the first other rk3399 device I tried,
> > the rock960, is also broken booting. We really can't have devices
> > randomly broken all the time and I don't have the time or the devices
> > to go through and test everything. We should really be getting better
> > at these mass changes, it doesn't lead to a great user experience!
>
> Part of the problem is that Rockchip had a few issues at the end of the
> last cycle and then the start of this one. Another issue is that I don't
> have a Rockchip platform in my CI loop, but I'm hoping to fix that in
> the next week or so.  Is top of tree working on your platforms or no?
> They should be, at this point.

This series hopes to resolve the issues:

https://patchwork.ozlabs.org/project/uboot/list/?series=344332&state=*

Regards,
Simon

[PATCH 3/3] bootstd: Enable BOOTSTD_DEFAULTS by default

[Simon Glass]

This is needed to enable the boot command used to start standard boot.
Enable it by default. This brings in quite a few features, mostly in
common with DISTRO_DEFAULTS

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/boot/Kconfig b/boot/Kconfig
index 5f491625c82..8759b863b00 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -409,6 +409,7 @@ if BOOTSTD
 config BOOTSTD_DEFAULTS
 	bool "Select some common defaults for standard boot"
 	depends on BOOTSTD
+	default y
 	imply USE_BOOTCOMMAND
 	# Bring in some defaults which are generally needed. Boards can drop
 	# these as needed to save code space. Bootstd does not generally require
-- 
2.39.2.637.g21b0678d19-goog

Re: [PATCH 3/3] bootstd: Enable BOOTSTD_DEFAULTS by default

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On 2023-02-21, Simon Glass wrote:
> This is needed to enable the boot command used to start standard boot.
> Enable it by default. This brings in quite a few features, mostly in
> common with DISTRO_DEFAULTS

Works for booting rockpro64-rk3399, thanks!

Tested-by: Vagrant Cascadian <vagrant@debian.org>

> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  boot/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index 5f491625c82..8759b863b00 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -409,6 +409,7 @@ if BOOTSTD
>  config BOOTSTD_DEFAULTS
>  	bool "Select some common defaults for standard boot"
>  	depends on BOOTSTD
> +	default y
>  	imply USE_BOOTCOMMAND
>  	# Bring in some defaults which are generally needed. Boards can drop
>  	# these as needed to save code space. Bootstd does not generally require
> -- 
> 2.39.2.637.g21b0678d19-goog

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 1/3] rockchip: Drop bootstage stash in TPL and SPL

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

On 2023-02-21, Simon Glass wrote:
> Unfortunately the IRAM used to stash the bootstage records in TPL
> becomes accessible after SPL runs. Presumably this is because of ATF
> taking it over.
>
> We could move the stash to another address in SPL, before passing it to
> U-Boot proper. But it seems easier to wait until we have support for
> standard passage[1] which should not be too far away.
>
> For now, disable it in TPL and SPL.
>
> [1] https://patchwork.ozlabs.org/project/uboot/cover/
>     20220117150428.1580273-1-sjg@chromium.org/
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Works for booting rockpro64-rk3399, thanks!

Tested-by: Vagrant Cascadian <vagrant@debian.org>

>  configs/rockpro64-rk3399_defconfig | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> index dd67f9dff64..49614236819 100644
> --- a/configs/rockpro64-rk3399_defconfig
> +++ b/configs/rockpro64-rk3399_defconfig
> @@ -13,7 +13,6 @@ CONFIG_DM_RESET=y
>  CONFIG_ROCKCHIP_RK3399=y
>  CONFIG_TARGET_ROCKPRO64_RK3399=y
>  CONFIG_SPL_STACK=0x400000
> -CONFIG_BOOTSTAGE_STASH_ADDR=0xff8e0000
>  CONFIG_DEBUG_UART_BASE=0xFF1A0000
>  CONFIG_DEBUG_UART_CLOCK=24000000
>  CONFIG_SPL_SPI_FLASH_SUPPORT=y
> @@ -21,11 +20,7 @@ CONFIG_SPL_SPI=y
>  CONFIG_SYS_LOAD_ADDR=0x800800
>  CONFIG_DEBUG_UART=y
>  CONFIG_BOOTSTAGE=y
> -CONFIG_SPL_BOOTSTAGE=y
> -CONFIG_TPL_BOOTSTAGE=y
>  CONFIG_BOOTSTAGE_REPORT=y
> -CONFIG_SPL_BOOTSTAGE_RECORD_COUNT=10
> -CONFIG_BOOTSTAGE_STASH=y
>  CONFIG_USE_PREBOOT=y
>  CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-rockpro64.dtb"
>  CONFIG_DISPLAY_BOARDINFO_LATE=y
> -- 
> 2.39.2.637.g21b0678d19-goog

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 1/3] rockchip: Drop bootstage stash in TPL and SPL

[Quentin Schulz]

Hi Simon,

On 2/21/23 20:39, Simon Glass wrote:
> Unfortunately the IRAM used to stash the bootstage records in TPL
> becomes accessible after SPL runs. Presumably this is because of ATF

nit: I believe you meant to say inaccessible here?

Cheers,
Quentin