[PATCH 0/9] Bug-fixes for a few boards
This series includes fixes to get some rockchip and nvidia boards working again. It also provides a fix (revert) for Beaglebone Black and a devicetree fix for coral (x86).
Simon Glass (9): nvidia: nyan-big: Disable debug UART tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL rockchip: veyron: Add logging for power init power: regulator: Handle autoset in regulators_enable_boot_on() fdt: Correct condition for bloblist existing spl: Allow ATF to work when dcache is disabled rockchip: bob: kevin: Disable dcache in SPL regulator: rk8xx: Fix incorrect parameter Revert "arm: am335x: Enable SPL_OF_CONTROL on some configs"
board/google/veyron/veyron.c | 27 ++++++++++------------ common/spl/spl_atf.c | 3 ++- configs/am335x_boneblack_vboot_defconfig | 1 - configs/chromebook_bob_defconfig | 1 + configs/chromebook_kevin_defconfig | 1 + configs/nyan-big_defconfig | 1 - drivers/power/regulator/regulator-uclass.c | 2 +- drivers/power/regulator/rk8xx.c | 2 +- lib/Kconfig | 8 +++---- lib/fdtdec.c | 12 ++++++++-- 10 files changed, 32 insertions(+), 26 deletions(-)
This cannot be enabled early in boot since some other init is needed. At this point it is unclear exactly what init is needed, so disable the debug UART to avoid a hang.
Signed-off-by: Simon Glass sjg@chromium.org ---
configs/nyan-big_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/nyan-big_defconfig b/configs/nyan-big_defconfig index 1483d17d975..4dec710cf8d 100644 --- a/configs/nyan-big_defconfig +++ b/configs/nyan-big_defconfig @@ -17,7 +17,6 @@ CONFIG_TEGRA124=y CONFIG_TARGET_NYAN_BIG=y CONFIG_TEGRA_GPU=y CONFIG_SYS_LOAD_ADDR=0x82408000 -CONFIG_DEBUG_UART=y CONFIG_FIT=y CONFIG_FIT_BEST_MATCH=y CONFIG_BOOTSTAGE=y
It does not make sense to enable all SHA algorithms unless they are needed. It bloats the code and in this case, causes chromebook_link to fail to build.
Add a condition to TPM to correct this. Note that the original commit combines refactoring and new features, which makes it hard to see what is going on.
Fixes: 97707f12fda tpm: Support boot measurements
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/Kconfig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/Kconfig b/lib/Kconfig index 189e6eb31aa..70b32362ada 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -438,10 +438,10 @@ config TPM bool "Trusted Platform Module (TPM) Support" depends on DM imply DM_RNG - select SHA1 - select SHA256 - select SHA384 - select SHA512 + select SHA1 if EFI_TCG2_PROTOCOL + select SHA256 if EFI_TCG2_PROTOCOL + select SHA384 if EFI_TCG2_PROTOCOL + select SHA512 if EFI_TCG2_PROTOCOL help This enables support for TPMs which can be used to provide security features for your board. The TPM can be connected via LPC or I2C
On 6/5/24 05:25, Simon Glass wrote:
It does not make sense to enable all SHA algorithms unless they are needed. It bloats the code and in this case, causes chromebook_link to fail to build.
Why would chromebook_link fail to build? Is TPM used by U-Boot on that board at all?
Add a condition to TPM to correct this. Note that the original commit combines refactoring and new features, which makes it hard to see what is going on.
Fixes: 97707f12fda tpm: Support boot measurements
Signed-off-by: Simon Glass sjg@chromium.org
lib/Kconfig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/Kconfig b/lib/Kconfig index 189e6eb31aa..70b32362ada 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -438,10 +438,10 @@ config TPM bool "Trusted Platform Module (TPM) Support" depends on DM imply DM_RNG
- select SHA1
- select SHA256
- select SHA384
- select SHA512
- select SHA1 if EFI_TCG2_PROTOCOL
- select SHA256 if EFI_TCG2_PROTOCOL
- select SHA384 if EFI_TCG2_PROTOCOL
- select SHA512 if EFI_TCG2_PROTOCOL
You need to consider CONFIG_MEASURED_BOOT which allows measured boot in the non-UEFI case.
Please, take into account
lib/tpm-v1.c:20: #error "TPM_AUTH_SESSIONS require SHA1 to be configured, too"
This #error should be replaced by a Kconfig constraint.
I would prefer the select statements to be in lib/efi_loader/Kconfig under EFI_TCG2_PROTOCOL.
@Ilias, Eddie:
Why do we require SHA1 which is considered insecure?
Shouldn't we change tpm2_supported_algorithms[] to include only algorithms selected in the configuration? This would allow replacing all the select statements in Simon's patch by imply.
Best regards
Heinrich
help This enables support for TPMs which can be used to provide security features for your board. The TPM can be connected via LPC or I2C
Hi Heinrich
On Wed, 5 Jun 2024 at 07:09, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 6/5/24 05:25, Simon Glass wrote:
It does not make sense to enable all SHA algorithms unless they are needed. It bloats the code and in this case, causes chromebook_link to fail to build.
Why would chromebook_link fail to build? Is TPM used by U-Boot on that board at all?
Add a condition to TPM to correct this. Note that the original commit combines refactoring and new features, which makes it hard to see what is going on.
Fixes: 97707f12fda tpm: Support boot measurements
Signed-off-by: Simon Glass sjg@chromium.org
lib/Kconfig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/Kconfig b/lib/Kconfig index 189e6eb31aa..70b32362ada 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -438,10 +438,10 @@ config TPM bool "Trusted Platform Module (TPM) Support" depends on DM imply DM_RNG
select SHA1select SHA256select SHA384select SHA512
select SHA1 if EFI_TCG2_PROTOCOLselect SHA256 if EFI_TCG2_PROTOCOLselect SHA384 if EFI_TCG2_PROTOCOLselect SHA512 if EFI_TCG2_PROTOCOLYou need to consider CONFIG_MEASURED_BOOT which allows measured boot in the non-UEFI case.
Please, take into account
lib/tpm-v1.c:20: #error "TPM_AUTH_SESSIONS require SHA1 to be configured, too"
This #error should be replaced by a Kconfig constraint.
I would prefer the select statements to be in lib/efi_loader/Kconfig under EFI_TCG2_PROTOCOL.
@Ilias, Eddie:
Why do we require SHA1 which is considered insecure?
Shouldn't we change tpm2_supported_algorithms[] to include only algorithms selected in the configuration? This would allow replacing all the select statements in Simon's patch by imply.
The algorithms used and configured by the TPM are device runtime decisions, not compile-time ones and to my knowledge, there are no devices out there that disable SHA1.
Failing to extend a PCR in an active bank is a security vulnerability. It would allow the unsealing of secrets if an attacker can replay a good set of measurements into an unused bank.
We could potentially fix that in the future since we can configure the TPM active banks on boot, but IIRC we don't support that yet.
Regards /Ilias
Best regards
Heinrich
help This enables support for TPMs which can be used to provide security features for your board. The TPM can be connected via LPC or I2C
Hi Heinrich,
On Tue, 4 Jun 2024 at 22:15, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 6/5/24 05:25, Simon Glass wrote:
It does not make sense to enable all SHA algorithms unless they are needed. It bloats the code and in this case, causes chromebook_link to fail to build.
Why would chromebook_link fail to build?
Because U-Boot suddenly grew a lot due to that commit. It broke out of the bounds of its SPI-flash region.
Is TPM used by U-Boot on that board at all?
Yes, but only for tests.
Add a condition to TPM to correct this. Note that the original commit combines refactoring and new features, which makes it hard to see what is going on.
Fixes: 97707f12fda tpm: Support boot measurements
Signed-off-by: Simon Glass sjg@chromium.org
lib/Kconfig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/Kconfig b/lib/Kconfig index 189e6eb31aa..70b32362ada 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -438,10 +438,10 @@ config TPM bool "Trusted Platform Module (TPM) Support" depends on DM imply DM_RNG
select SHA1select SHA256select SHA384select SHA512
select SHA1 if EFI_TCG2_PROTOCOLselect SHA256 if EFI_TCG2_PROTOCOLselect SHA384 if EFI_TCG2_PROTOCOLselect SHA512 if EFI_TCG2_PROTOCOLYou need to consider CONFIG_MEASURED_BOOT which allows measured boot in the non-UEFI case.
Please, take into account
lib/tpm-v1.c:20: #error "TPM_AUTH_SESSIONS require SHA1 to be configured, too"
This #error should be replaced by a Kconfig constraint.
I would prefer the select statements to be in lib/efi_loader/Kconfig under EFI_TCG2_PROTOCOL.
@Ilias, Eddie:
Why do we require SHA1 which is considered insecure?
Shouldn't we change tpm2_supported_algorithms[] to include only algorithms selected in the configuration? This would allow replacing all the select statements in Simon's patch by imply.
I attempted that in v2.
Best regards
Heinrich
help This enables support for TPMs which can be used to provide security features for your board. The TPM can be connected via LPC or I2C
Regards, Simon
Add better logging for power init so that CONFIG_LOG_ERROR_RETURN can be enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
board/google/veyron/veyron.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c index 32dbcdc4d10..23fe8bf088c 100644 --- a/board/google/veyron/veyron.c +++ b/board/google/veyron/veyron.c @@ -29,44 +29,41 @@ static int veyron_init(void) int ret;
ret = regulator_get_by_platname("vdd_arm", &dev); - if (ret) { - debug("Cannot set regulator name\n"); - return ret; - } + if (ret) + return log_msg_ret("vdd", ret);
/* Slowly raise to max CPU voltage to prevent overshoot */ ret = regulator_set_value(dev, 1200000); if (ret) - return ret; + return log_msg_ret("s12", ret); udelay(175); /* Must wait for voltage to stabilize, 2mV/us */ ret = regulator_set_value(dev, 1400000); if (ret) - return ret; + return log_msg_ret("s14", ret); udelay(100); /* Must wait for voltage to stabilize, 2mV/us */
ret = rockchip_get_clk(&clk.dev); if (ret) - return ret; + return log_msg_ret("clk", ret); clk.id = PLL_APLL; ret = clk_set_rate(&clk, 1800000000); if (IS_ERR_VALUE(ret)) - return ret; + return log_msg_ret("s18", ret);
ret = regulator_get_by_platname("vcc33_sd", &dev); if (ret) { debug("Cannot get regulator name\n"); - return ret; + if (ret) + return log_msg_ret("vcc", ret); }
ret = regulator_set_value(dev, 3300000); if (ret) - return ret; + return log_msg_ret("s33", ret);
ret = regulators_enable_boot_on(false); - if (ret) { - debug("%s: Cannot enable boot on regulators\n", __func__); - return ret; - } + if (ret) + return log_msg_ret("boo", ret);
return 0; } @@ -81,7 +78,7 @@ int board_early_init_r(void) if (!fdt_node_check_compatible(gd->fdt_blob, 0, "google,veyron")) { ret = veyron_init(); if (ret) - return ret; + return log_msg_ret("vey", ret); } #endif /*
Hi Simon,
On 6/5/24 5:25 AM, Simon Glass wrote:
Add better logging for power init so that CONFIG_LOG_ERROR_RETURN can be enabled.
Signed-off-by: Simon Glass sjg@chromium.org
board/google/veyron/veyron.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c index 32dbcdc4d10..23fe8bf088c 100644 --- a/board/google/veyron/veyron.c +++ b/board/google/veyron/veyron.c @@ -29,44 +29,41 @@ static int veyron_init(void) int ret;
ret = regulator_get_by_platname("vdd_arm", &dev);
- if (ret) {
debug("Cannot set regulator name\n");return ret;- }
- if (ret)
return log_msg_ret("vdd", ret);
Those log messages aren't for code in SPL as far as I could tell, is there any reason to make them that small/cryptic?
/* Slowly raise to max CPU voltage to prevent overshoot */ ret = regulator_set_value(dev, 1200000); if (ret)
return ret;
return log_msg_ret("s12", ret);
return ret;
return log_msg_ret("s14", ret);udelay(100); /* Must wait for voltage to stabilize, 2mV/us */
ret = rockchip_get_clk(&clk.dev); if (ret)
return ret;
return log_msg_ret("clk", ret);
return ret;
return log_msg_ret("s18", ret);ret = regulator_get_by_platname("vcc33_sd", &dev); if (ret) { debug("Cannot get regulator name\n");
return ret;
if (ret)return log_msg_ret("vcc", ret);
I think you can just merge the debug and log_msg_ret here?
Otherwise looking good to me,
Cheers, Quentin
HI Quentin,
On Wed, 5 Jun 2024 at 02:36, Quentin Schulz quentin.schulz@cherry.de wrote:
Hi Simon,
On 6/5/24 5:25 AM, Simon Glass wrote:
Add better logging for power init so that CONFIG_LOG_ERROR_RETURN can be enabled.
Signed-off-by: Simon Glass sjg@chromium.org
board/google/veyron/veyron.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c index 32dbcdc4d10..23fe8bf088c 100644 --- a/board/google/veyron/veyron.c +++ b/board/google/veyron/veyron.c @@ -29,44 +29,41 @@ static int veyron_init(void) int ret;
ret = regulator_get_by_platname("vdd_arm", &dev);
if (ret) {debug("Cannot set regulator name\n");return ret;}
if (ret)return log_msg_ret("vdd", ret);Those log messages aren't for code in SPL as far as I could tell, is there any reason to make them that small/cryptic?
Oh yes, it happens early in U-Boot proper before any messages which is probably what confused me.
Re size, they just need to support searching the code base. Long strings mean that many boards fail to boot / hit their limits when CONFIG_LOG_ERROR_RETURN is enabled. So I try to keep them to 3 characters.
/* Slowly raise to max CPU voltage to prevent overshoot */ ret = regulator_set_value(dev, 1200000); if (ret)
return ret;
return log_msg_ret("s12", ret); udelay(175); /* Must wait for voltage to stabilize, 2mV/us */ ret = regulator_set_value(dev, 1400000); if (ret)
return ret;
return log_msg_ret("s14", ret); udelay(100); /* Must wait for voltage to stabilize, 2mV/us */ ret = rockchip_get_clk(&clk.dev); if (ret)
return ret;
return log_msg_ret("clk", ret); clk.id = PLL_APLL; ret = clk_set_rate(&clk, 1800000000); if (IS_ERR_VALUE(ret))
return ret;
return log_msg_ret("s18", ret); ret = regulator_get_by_platname("vcc33_sd", &dev); if (ret) { debug("Cannot get regulator name\n");
return ret;
if (ret)return log_msg_ret("vcc", ret);I think you can just merge the debug and log_msg_ret here?
They are actually different. The debug message is how I actually found this problem.
Otherwise looking good to me,
Cheers, Quentin
Regards, Simon
Hi Simon,
On 6/6/24 5:04 PM, Simon Glass wrote:
HI Quentin,
On Wed, 5 Jun 2024 at 02:36, Quentin Schulz quentin.schulz@cherry.de wrote:
Hi Simon,
On 6/5/24 5:25 AM, Simon Glass wrote:
Add better logging for power init so that CONFIG_LOG_ERROR_RETURN can be enabled.
Signed-off-by: Simon Glass sjg@chromium.org
board/google/veyron/veyron.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c index 32dbcdc4d10..23fe8bf088c 100644 --- a/board/google/veyron/veyron.c +++ b/board/google/veyron/veyron.c @@ -29,44 +29,41 @@ static int veyron_init(void) int ret;
ret = regulator_get_by_platname("vdd_arm", &dev);
if (ret) {debug("Cannot set regulator name\n");return ret;}
if (ret)return log_msg_ret("vdd", ret);Those log messages aren't for code in SPL as far as I could tell, is there any reason to make them that small/cryptic?
Oh yes, it happens early in U-Boot proper before any messages which is probably what confused me.
Re size, they just need to support searching the code base. Long strings mean that many boards fail to boot / hit their limits when CONFIG_LOG_ERROR_RETURN is enabled. So I try to keep them to 3 characters.
git grep vcc/vdd is going to return a lot of matches :)
This is for one board, so there isn't much reason to argue about this, so fine :)
/* Slowly raise to max CPU voltage to prevent overshoot */ ret = regulator_set_value(dev, 1200000); if (ret)
return ret;
return log_msg_ret("s12", ret); udelay(175); /* Must wait for voltage to stabilize, 2mV/us */ ret = regulator_set_value(dev, 1400000); if (ret)
return ret;
return log_msg_ret("s14", ret); udelay(100); /* Must wait for voltage to stabilize, 2mV/us */ ret = rockchip_get_clk(&clk.dev); if (ret)
return ret;
return log_msg_ret("clk", ret); clk.id = PLL_APLL; ret = clk_set_rate(&clk, 1800000000); if (IS_ERR_VALUE(ret))
return ret;
return log_msg_ret("s18", ret); ret = regulator_get_by_platname("vcc33_sd", &dev); if (ret) { debug("Cannot get regulator name\n");
return ret;
if (ret)return log_msg_ret("vcc", ret);I think you can just merge the debug and log_msg_ret here?
They are actually different. The debug message is how I actually found this problem.
I meant:
""" if (ret) { debug("Cannot get regulator name\n"); if (ret) return log_msg_ret("vcc", ret); } """
Nothing changes ret before the third line, so at the very least we could have:
""" if (ret) { debug("Cannot get regulator name\n"); return log_msg_ret("vcc", ret); } """
But i was also suggesting we merge this into:
""" if (ret) return log_msg_ret("Cannot get vcc regulator name", ret); """
But since you seem to want to keep only a few characters, then just removing the debug entirely? Or what's the plan with it? (I see log_msg_ret as a debug message, but I'm probably wrong here?).
Cheers, Quentin
With a recent change, regulators_enable_boot_on() returns an error if a regulator is already set. Check for and handle this situation.
Fixes: d99fb64a98a power: regulator: Only run autoset once for each regulator
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/power/regulator/regulator-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 77d101f262e..d9e1fb68295 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -518,7 +518,7 @@ int regulators_enable_boot_on(bool verbose) dev; uclass_next_device(&dev)) { ret = regulator_autoset(dev); - if (ret == -EMEDIUMTYPE) { + if (ret == -EMEDIUMTYPE || ret == -EALREADY) { ret = 0; continue; }
Hi Simon,
On 6/5/24 5:25 AM, Simon Glass wrote:
With a recent change, regulators_enable_boot_on() returns an error if a regulator is already set. Check for and handle this situation.
Fixes: d99fb64a98a power: regulator: Only run autoset once for each regulator
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Quentin Schulz quentin.schulz@cherry.de
Thanks! Quentin
Hi Simon,
On 2024-06-05 05:25, Simon Glass wrote:
With a recent change, regulators_enable_boot_on() returns an error if a regulator is already set. Check for and handle this situation.
I am guessing this is being hit because of the call in veyron_init() ?
regulators_enable_boot_on() is also called for rockchip boards in the rockchip common boards.c board_init().
Maybe the call to regulators_enable_boot_on() in veyron_init() could be dropped?
Fixes: d99fb64a98a power: regulator: Only run autoset once for each regulator
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Jonas Karlman jonas@kwiboo.se
Regards, Jonas
drivers/power/regulator/regulator-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 77d101f262e..d9e1fb68295 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -518,7 +518,7 @@ int regulators_enable_boot_on(bool verbose) dev; uclass_next_device(&dev)) { ret = regulator_autoset(dev);
if (ret == -EMEDIUMTYPE) {
if (ret == -EMEDIUMTYPE || ret == -EALREADY) { ret = 0; continue;
On some boards, the bloblist is created in SPL once SDRAM is ready. It cannot be accessed until that point, so is not available early in SPL.
Add a condition to avoid a hang in this case.
This fixes a hang in chromebook_coral
Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist")
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
- /* If allowing a bloblist, check that first */ - if (CONFIG_IS_ENABLED(BLOBLIST)) { + /* + * If allowing a bloblist, check that first. This would be better + * handled with an OF_BLOBLIST Kconfig, but that caused far too much + * argument, so add a hack here, used e.g. by chromebook_coral + * The necessary test is whether the previous stage passed a bloblist, + * not whether this one creates one. + */ + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && + (spl_prev_phase() != PHASE_TPL || + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { ret = bloblist_maybe_init(); if (!ret) { gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
Hi Simon,
On Wed, 5 Jun 2024 at 06:26, Simon Glass sjg@chromium.org wrote:
On some boards, the bloblist is created in SPL once SDRAM is ready. It cannot be accessed until that point, so is not available early in SPL.
Add a condition to avoid a hang in this case.
This fixes a hang in chromebook_coral
Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist")
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
/* If allowing a bloblist, check that first */if (CONFIG_IS_ENABLED(BLOBLIST)) {
/** If allowing a bloblist, check that first. This would be better* handled with an OF_BLOBLIST Kconfig, but that caused far too much* argument, so add a hack here, used e.g. by chromebook_coral* The necessary test is whether the previous stage passed a bloblist,* not whether this one creates one.*/if (CONFIG_IS_ENABLED(OF_BLOBLIST) &&(spl_prev_phase() != PHASE_TPL ||!IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
The same condition exists in common/bloblist.c. Carve out a function --e.g
bool can can_enable_bloblist(void) return ....
instead of open coding that
Thanks /Ilias
ret = bloblist_maybe_init(); if (!ret) { gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);-- 2.34.1
Hi Ilias,
On Tue, 4 Jun 2024 at 23:33, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 5 Jun 2024 at 06:26, Simon Glass sjg@chromium.org wrote:
On some boards, the bloblist is created in SPL once SDRAM is ready. It cannot be accessed until that point, so is not available early in SPL.
Add a condition to avoid a hang in this case.
This fixes a hang in chromebook_coral
Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist")
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
/* If allowing a bloblist, check that first */if (CONFIG_IS_ENABLED(BLOBLIST)) {
/** If allowing a bloblist, check that first. This would be better* handled with an OF_BLOBLIST Kconfig, but that caused far too much* argument, so add a hack here, used e.g. by chromebook_coral* The necessary test is whether the previous stage passed a bloblist,* not whether this one creates one.*/if (CONFIG_IS_ENABLED(OF_BLOBLIST) &&(spl_prev_phase() != PHASE_TPL ||!IS_ENABLED(CONFIG_TPL_BLOBLIST))) {The same condition exists in common/bloblist.c. Carve out a function --e.g
bool can can_enable_bloblist(void) return ....
instead of open coding that
Unfortunately it looks like the conditions are different, with the one you mention being:
if (spl_prev_phase() == PHASE_TPL && !IS_ENABLED(CONFIG_TPL_BLOBLIST)) from_addr = false;
(is that the one you mean?)
So I don't think I can combine them into a helper function.
Regards, Simon
Hi Simon,
On Tue, 4 Jun 2024 at 23:27, Simon Glass sjg@chromium.org wrote:
On some boards, the bloblist is created in SPL once SDRAM is ready. It cannot be accessed until that point, so is not available early in SPL.
Add a condition to avoid a hang in this case.
This fixes a hang in chromebook_coral
Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist")
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
/* If allowing a bloblist, check that first */if (CONFIG_IS_ENABLED(BLOBLIST)) {
/** If allowing a bloblist, check that first. This would be better* handled with an OF_BLOBLIST Kconfig, but that caused far toomuch
* argument, so add a hack here, used e.g. by chromebook_coral
I am a bit confused by this comment - It means you will not use OF_BLOBLIST, but actually you are using it below. Is it a typo?
* The necessary test is whether the previous stage passed abloblist,
* not whether this one creates one.*/if (CONFIG_IS_ENABLED(OF_BLOBLIST) &&(spl_prev_phase() != PHASE_TPL ||!IS_ENABLED(CONFIG_TPL_BLOBLIST))) { ret = bloblist_maybe_init(); if (!ret) { gd->fdt_blob =bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
2.34.1
Regards, Raymond
Hi Raymond,
On Mon, 10 Jun 2024 at 13:13, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Tue, 4 Jun 2024 at 23:27, Simon Glass sjg@chromium.org wrote:
On some boards, the bloblist is created in SPL once SDRAM is ready. It cannot be accessed until that point, so is not available early in SPL.
Add a condition to avoid a hang in this case.
This fixes a hang in chromebook_coral
Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist")
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
/* If allowing a bloblist, check that first */if (CONFIG_IS_ENABLED(BLOBLIST)) {
/** If allowing a bloblist, check that first. This would be better* handled with an OF_BLOBLIST Kconfig, but that caused far too much* argument, so add a hack here, used e.g. by chromebook_coralI am a bit confused by this comment - It means you will not use OF_BLOBLIST, but actually you are using it below. Is it a typo?
Basically it would be cleaner to have a separate, phase-specific Kconfig control as to whether the DT can come from the bloblist (I can't remember the Kconfig name I suggested, nor the patch as it was last year sometime). But for now I am adding this hack to get a few boards working again.
* The necessary test is whether the previous stage passed a bloblist,* not whether this one creates one.*/if (CONFIG_IS_ENABLED(OF_BLOBLIST) &&(spl_prev_phase() != PHASE_TPL ||!IS_ENABLED(CONFIG_TPL_BLOBLIST))) { ret = bloblist_maybe_init(); if (!ret) { gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);-- 2.34.1
Regards, Raymond
Regards, Simon
On Tue, 11 Jun 2024 at 13:54, Simon Glass sjg@chromium.org wrote:
Hi Raymond,
On Mon, 10 Jun 2024 at 13:13, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Tue, 4 Jun 2024 at 23:27, Simon Glass sjg@chromium.org wrote:
On some boards, the bloblist is created in SPL once SDRAM is ready. It cannot be accessed until that point, so is not available early in SPL.
Add a condition to avoid a hang in this case.
This fixes a hang in chromebook_coral
Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist")
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
/* If allowing a bloblist, check that first */if (CONFIG_IS_ENABLED(BLOBLIST)) {
/** If allowing a bloblist, check that first. This would be better* handled with an OF_BLOBLIST Kconfig, but that caused far too much* argument, so add a hack here, used e.g. by chromebook_coralI am a bit confused by this comment - It means you will not use OF_BLOBLIST, but actually you are using it below. Is it a typo?
Basically it would be cleaner to have a separate, phase-specific Kconfig control as to whether the DT can come from the bloblist (I can't remember the Kconfig name I suggested, nor the patch as it was last year sometime). But for now I am adding this hack to get a few boards working again.
I am a bit confused. First of all the comment is innapropriate. We went through a lengthy discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we made up our minds. Why are you adding this comment now? Why do code comments have to illustrate your personal opinion -- which was rejected?
Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo?
Thanks /Ilias
* The necessary test is whether the previous stage passed a bloblist,* not whether this one creates one.*/if (CONFIG_IS_ENABLED(OF_BLOBLIST) &&(spl_prev_phase() != PHASE_TPL ||!IS_ENABLED(CONFIG_TPL_BLOBLIST))) { ret = bloblist_maybe_init(); if (!ret) { gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);-- 2.34.1
Regards, Raymond
Regards, Simon
Hi Ilias,
On Tue, 11 Jun 2024 at 08:27, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 11 Jun 2024 at 13:54, Simon Glass sjg@chromium.org wrote:
Hi Raymond,
On Mon, 10 Jun 2024 at 13:13, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Tue, 4 Jun 2024 at 23:27, Simon Glass sjg@chromium.org wrote:
On some boards, the bloblist is created in SPL once SDRAM is ready. It cannot be accessed until that point, so is not available early in SPL.
Add a condition to avoid a hang in this case.
This fixes a hang in chromebook_coral
Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist")
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
/* If allowing a bloblist, check that first */if (CONFIG_IS_ENABLED(BLOBLIST)) {
/** If allowing a bloblist, check that first. This would be better* handled with an OF_BLOBLIST Kconfig, but that caused far too much* argument, so add a hack here, used e.g. by chromebook_coralI am a bit confused by this comment - It means you will not use OF_BLOBLIST, but actually you are using it below. Is it a typo?
Basically it would be cleaner to have a separate, phase-specific Kconfig control as to whether the DT can come from the bloblist (I can't remember the Kconfig name I suggested, nor the patch as it was last year sometime). But for now I am adding this hack to get a few boards working again.
I am a bit confused. First of all the comment is innapropriate. We went through a lengthy discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we made up our minds. Why are you adding this comment now? Why do code comments have to illustrate your personal opinion -- which was rejected?
I'm sorry for the tone of the comment. I am not trying to offend anyone here and I'm happy to change the language. As I probably mentioned at the time, my accepted patch breaks my workflow and several boards. I hope you can understand how frustrating that sort of thing can be. Also, now that I have my lab back up and running and I would like these boards to work again on mainline!
Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo?
Remember, it was a patch you rejected :-)
Regards, Simon
Thanks /Ilias
* The necessary test is whether the previous stage passed a bloblist,* not whether this one creates one.*/if (CONFIG_IS_ENABLED(OF_BLOBLIST) &&(spl_prev_phase() != PHASE_TPL ||!IS_ENABLED(CONFIG_TPL_BLOBLIST))) { ret = bloblist_maybe_init(); if (!ret) { gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);-- 2.34.1
Regards, Raymond
Regards, Simon
[...]
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
/* If allowing a bloblist, check that first */if (CONFIG_IS_ENABLED(BLOBLIST)) {
/** If allowing a bloblist, check that first. This would be better* handled with an OF_BLOBLIST Kconfig, but that caused far too much* argument, so add a hack here, used e.g. by chromebook_coralI am a bit confused by this comment - It means you will not use OF_BLOBLIST, but actually you are using it below. Is it a typo?
Basically it would be cleaner to have a separate, phase-specific Kconfig control as to whether the DT can come from the bloblist (I can't remember the Kconfig name I suggested, nor the patch as it was last year sometime). But for now I am adding this hack to get a few boards working again.
I am a bit confused. First of all the comment is innapropriate. We went through a lengthy discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we made up our minds. Why are you adding this comment now? Why do code comments have to illustrate your personal opinion -- which was rejected?
I'm sorry for the tone of the comment. I am not trying to offend anyone here and I'm happy to change the language.
Yes please a comment explaining why that piece of code is there is far more intuitive.
As I probably mentioned at the time, my accepted patch breaks my workflow and several boards. I hope you can understand how frustrating that sort of thing can be.
Yes, I do and I am fine with a short-term patch that fixes issues, as long as its not too intrusive. There is a very thin line between quick and dirty fixes to spaghetti unreadable code. But we should have comments and/or commit messages indicating that this needs a proper fix
Also, now that I have my lab back up and running and I would like these boards to work again on mainline!
Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo?
Remember, it was a patch you rejected :-)
I don't maintain any of that. I only gave some feedback along the lines of "bloblist was designed to be auto-discoverable, I don't see how adding an explicit Kconfig helps". IIRC we eventually followed what Tom suggested.
In any case, the amount of bike-shedding in the topic is too much. Do you mind explaining the problem in your workflow again? Perhaps we can find a solution that is integrated in bloblist_maybe_init() instead of injecting ifs on when a bloblist should or should not be searched for all over the place.
Regards /Ilias
Regards, Simon
Thanks /Ilias
* The necessary test is whether the previous stage passed a bloblist,* not whether this one creates one.*/if (CONFIG_IS_ENABLED(OF_BLOBLIST) &&(spl_prev_phase() != PHASE_TPL ||!IS_ENABLED(CONFIG_TPL_BLOBLIST))) { ret = bloblist_maybe_init(); if (!ret) { gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);-- 2.34.1
Regards, Raymond
Regards, Simon
Hi Ilias,
On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..b141244e3b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) { int ret = -ENOENT;
/* If allowing a bloblist, check that first */if (CONFIG_IS_ENABLED(BLOBLIST)) {
/** If allowing a bloblist, check that first. This would be better* handled with an OF_BLOBLIST Kconfig, but that caused far too much* argument, so add a hack here, used e.g. by chromebook_coralI am a bit confused by this comment - It means you will not use OF_BLOBLIST, but actually you are using it below. Is it a typo?
Basically it would be cleaner to have a separate, phase-specific Kconfig control as to whether the DT can come from the bloblist (I can't remember the Kconfig name I suggested, nor the patch as it was last year sometime). But for now I am adding this hack to get a few boards working again.
I am a bit confused. First of all the comment is innapropriate. We went through a lengthy discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we made up our minds. Why are you adding this comment now? Why do code comments have to illustrate your personal opinion -- which was rejected?
I'm sorry for the tone of the comment. I am not trying to offend anyone here and I'm happy to change the language.
Yes please a comment explaining why that piece of code is there is far more intuitive.
OK, once we have agreed the below I can do that.
As I probably mentioned at the time, my accepted patch breaks my workflow and several boards. I hope you can understand how frustrating that sort of thing can be.
Yes, I do and I am fine with a short-term patch that fixes issues, as long as its not too intrusive. There is a very thin line between quick and dirty fixes to spaghetti unreadable code. But we should have comments and/or commit messages indicating that this needs a proper fix
I spent a lot of time explaining this last time.
Also, now that I have my lab back up and running and I would like these boards to work again on mainline!
Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo?
Remember, it was a patch you rejected :-)
I don't maintain any of that. I only gave some feedback along the lines of "bloblist was designed to be auto-discoverable, I don't see how adding an explicit Kconfig helps". IIRC we eventually followed what Tom suggested.
I'm not trying to point the finger here. So far the boards are broken in mainline...I'm just trying to fix that,
In any case, the amount of bike-shedding in the topic is too much. Do you mind explaining the problem in your workflow again? Perhaps we can find a solution that is integrated in bloblist_maybe_init() instead of injecting ifs on when a bloblist should or should not be searched for all over the place.
TPL (or SPL) sets up a bloblist with bits of info in it, but no DT (which is in memory-mapped SPI flash) U-Boot proper starts up, it wants to get the bloblist but not hang if the bloblist doesn't have a DT
Regards, Simon
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
Hi Ilias,
On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
> --- > > lib/fdtdec.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index b2c59ab3818..b141244e3b9 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) > { > int ret = -ENOENT; > > - /* If allowing a bloblist, check that first */ > - if (CONFIG_IS_ENABLED(BLOBLIST)) { > + /* > + * If allowing a bloblist, check that first. This would be better > + * handled with an OF_BLOBLIST Kconfig, but that caused far too much > + * argument, so add a hack here, used e.g. by chromebook_coral
I am a bit confused by this comment - It means you will not use OF_BLOBLIST, but actually you are using it below. Is it a typo?
Basically it would be cleaner to have a separate, phase-specific Kconfig control as to whether the DT can come from the bloblist (I can't remember the Kconfig name I suggested, nor the patch as it was last year sometime). But for now I am adding this hack to get a few boards working again.
I am a bit confused. First of all the comment is innapropriate. We went through a lengthy discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we made up our minds. Why are you adding this comment now? Why do code comments have to illustrate your personal opinion -- which was rejected?
I'm sorry for the tone of the comment. I am not trying to offend anyone here and I'm happy to change the language.
Yes please a comment explaining why that piece of code is there is far more intuitive.
OK, once we have agreed the below I can do that.
As I probably mentioned at the time, my accepted patch breaks my workflow and several boards. I hope you can understand how frustrating that sort of thing can be.
Yes, I do and I am fine with a short-term patch that fixes issues, as long as its not too intrusive. There is a very thin line between quick and dirty fixes to spaghetti unreadable code. But we should have comments and/or commit messages indicating that this needs a proper fix
I spent a lot of time explaining this last time.
Also, now that I have my lab back up and running and I would like these boards to work again on mainline!
Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo?
Remember, it was a patch you rejected :-)
I don't maintain any of that. I only gave some feedback along the lines of "bloblist was designed to be auto-discoverable, I don't see how adding an explicit Kconfig helps". IIRC we eventually followed what Tom suggested.
I'm not trying to point the finger here. So far the boards are broken in mainline...I'm just trying to fix that,
In any case, the amount of bike-shedding in the topic is too much. Do you mind explaining the problem in your workflow again? Perhaps we can find a solution that is integrated in bloblist_maybe_init() instead of injecting ifs on when a bloblist should or should not be searched for all over the place.
TPL (or SPL) sets up a bloblist with bits of info in it, but no DT (which is in memory-mapped SPI flash) U-Boot proper starts up, it wants to get the bloblist but not hang if the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a while back. Looking at the code, if the DT isn't found in a bloblist and the feature is enabled, we don't crash. We just print a debug message and continue to find the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Thanks /Ilias
Regards, Simon
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
Hi Ilias,
On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
>> --- >> >> lib/fdtdec.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c >> index b2c59ab3818..b141244e3b9 100644 >> --- a/lib/fdtdec.c >> +++ b/lib/fdtdec.c >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) >> { >> int ret = -ENOENT; >> >> - /* If allowing a bloblist, check that first */ >> - if (CONFIG_IS_ENABLED(BLOBLIST)) { >> + /* >> + * If allowing a bloblist, check that first. This would be better >> + * handled with an OF_BLOBLIST Kconfig, but that caused far too much >> + * argument, so add a hack here, used e.g. by chromebook_coral > > I am a bit confused by this comment - It means you will not use OF_BLOBLIST, > but actually you are using it below. Is it a typo?
Basically it would be cleaner to have a separate, phase-specific Kconfig control as to whether the DT can come from the bloblist (I can't remember the Kconfig name I suggested, nor the patch as it was last year sometime). But for now I am adding this hack to get a few boards working again.
I am a bit confused. First of all the comment is innapropriate. We went through a lengthy discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we made up our minds. Why are you adding this comment now? Why do code comments have to illustrate your personal opinion -- which was rejected?
I'm sorry for the tone of the comment. I am not trying to offend anyone here and I'm happy to change the language.
Yes please a comment explaining why that piece of code is there is far more intuitive.
OK, once we have agreed the below I can do that.
As I probably mentioned at the time, my accepted patch breaks my workflow and several boards. I hope you can understand how frustrating that sort of thing can be.
Yes, I do and I am fine with a short-term patch that fixes issues, as long as its not too intrusive. There is a very thin line between quick and dirty fixes to spaghetti unreadable code. But we should have comments and/or commit messages indicating that this needs a proper fix
I spent a lot of time explaining this last time.
Also, now that I have my lab back up and running and I would like these boards to work again on mainline!
Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo?
Remember, it was a patch you rejected :-)
I don't maintain any of that. I only gave some feedback along the lines of "bloblist was designed to be auto-discoverable, I don't see how adding an explicit Kconfig helps". IIRC we eventually followed what Tom suggested.
I'm not trying to point the finger here. So far the boards are broken in mainline...I'm just trying to fix that,
In any case, the amount of bike-shedding in the topic is too much. Do you mind explaining the problem in your workflow again? Perhaps we can find a solution that is integrated in bloblist_maybe_init() instead of injecting ifs on when a bloblist should or should not be searched for all over the place.
TPL (or SPL) sets up a bloblist with bits of info in it, but no DT (which is in memory-mapped SPI flash) U-Boot proper starts up, it wants to get the bloblist but not hang if the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a while back. Looking at the code, if the DT isn't found in a bloblist and the feature is enabled, we don't crash. We just print a debug message and continue to find the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Because at this point, if there is no bloblist, then it needs to be created. But creating it this early may fail, e.g. since there is no malloc(). The intent of this code is to locate an FDT from an existing bloblist. There is no point in creating a new bloblist here, since it obviously won't have an FDT in it.
Regards, Simon
Hi Simon,
On Fri, 21 Jun 2024 at 10:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
Hi Ilias,
On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
> >> --- > >> > >> lib/fdtdec.c | 12 ++++++++++-- > >> 1 file changed, 10 insertions(+), 2 deletions(-) > >> > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c > >> index b2c59ab3818..b141244e3b9 100644 > >> --- a/lib/fdtdec.c > >> +++ b/lib/fdtdec.c > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) > >> { > >> int ret = -ENOENT; > >> > >> - /* If allowing a bloblist, check that first */ > >> - if (CONFIG_IS_ENABLED(BLOBLIST)) { > >> + /* > >> + * If allowing a bloblist, check that first. This
would be better
> >> + * handled with an OF_BLOBLIST Kconfig, but that
caused far too much
> >> + * argument, so add a hack here, used e.g. by
chromebook_coral
> > > > I am a bit confused by this comment - It means you will not
use OF_BLOBLIST,
> > but actually you are using it below. Is it a typo? > > Basically it would be cleaner to have a separate,
phase-specific
> Kconfig control as to whether the DT can come from the
bloblist (I
> can't remember the Kconfig name I suggested, nor the patch as
it was
> last year sometime). But for now I am adding this hack to get
a few
> boards working again.
I am a bit confused. First of all the comment is innapropriate. We went through a
lengthy
discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in
and we
made up our minds. Why are you adding this comment now? Why do
code
comments have to illustrate your personal opinion -- which was rejected?
I'm sorry for the tone of the comment. I am not trying to offend anyone here and I'm happy to change the language.
Yes please a comment explaining why that piece of code is there is
far
more intuitive.
OK, once we have agreed the below I can do that.
As I probably mentioned at the time, my accepted patch breaks my workflow and several boards. I hope you can understand how frustrating that
sort of
thing can be.
Yes, I do and I am fine with a short-term patch that fixes issues, as long as its not too intrusive. There is a very thin line between
quick
and dirty fixes to spaghetti unreadable code. But we should have comments and/or commit messages indicating that this needs a proper fix
I spent a lot of time explaining this last time.
Also, now that I have my lab back up and running and I would like these boards to work again on mainline!
Grepping for OF_BLOBLIST, I can't find any matches, so is the
above if a typo?
Remember, it was a patch you rejected :-)
I don't maintain any of that. I only gave some feedback along the lines of "bloblist was designed to be auto-discoverable, I don't see how adding an explicit Kconfig helps". IIRC we eventually followed what Tom suggested.
I'm not trying to point the finger here. So far the boards are broken in mainline...I'm just trying to fix that,
In any case, the amount of bike-shedding in the topic is too much. Do you mind explaining the problem in your workflow again? Perhaps we
can
find a solution that is integrated in bloblist_maybe_init() instead
of
injecting ifs on when a bloblist should or should not be searched for all over the place.
TPL (or SPL) sets up a bloblist with bits of info in it, but no DT (which is in memory-mapped SPI flash) U-Boot proper starts up, it wants to get the bloblist but not hang if the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a while
back.
Looking at the code, if the DT isn't found in a bloblist and the feature
is
enabled, we don't crash. We just print a debug message and continue to
find
the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Because at this point, if there is no bloblist, then it needs to be created. But creating it this early may fail, e.g. since there is no malloc(). The intent of this code is to locate an FDT from an existing bloblist. There is no point in creating a new bloblist here, since it obviously won't have an FDT in it.
Can we add a bool arg to bloblist_init for this?
Eg. int bloblist_init(bool allow_malloc);
Regards, Raymond
Hi Raymond,
On Fri, 21 Jun 2024 at 10:40, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 10:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
Hi Ilias,
On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
> > >> --- > > >> > > >> lib/fdtdec.c | 12 ++++++++++-- > > >> 1 file changed, 10 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > >> index b2c59ab3818..b141244e3b9 100644 > > >> --- a/lib/fdtdec.c > > >> +++ b/lib/fdtdec.c > > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) > > >> { > > >> int ret = -ENOENT; > > >> > > >> - /* If allowing a bloblist, check that first */ > > >> - if (CONFIG_IS_ENABLED(BLOBLIST)) { > > >> + /* > > >> + * If allowing a bloblist, check that first. This would be better > > >> + * handled with an OF_BLOBLIST Kconfig, but that caused far too much > > >> + * argument, so add a hack here, used e.g. by chromebook_coral > > > > > > I am a bit confused by this comment - It means you will not use OF_BLOBLIST, > > > but actually you are using it below. Is it a typo? > > > > Basically it would be cleaner to have a separate, phase-specific > > Kconfig control as to whether the DT can come from the bloblist (I > > can't remember the Kconfig name I suggested, nor the patch as it was > > last year sometime). But for now I am adding this hack to get a few > > boards working again. > > I am a bit confused. > First of all the comment is innapropriate. We went through a lengthy > discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we > made up our minds. Why are you adding this comment now? Why do code > comments have to illustrate your personal opinion -- which was > rejected?
I'm sorry for the tone of the comment. I am not trying to offend anyone here and I'm happy to change the language.
Yes please a comment explaining why that piece of code is there is far more intuitive.
OK, once we have agreed the below I can do that.
As I probably mentioned at the time, my accepted patch breaks my workflow and several boards. I hope you can understand how frustrating that sort of thing can be.
Yes, I do and I am fine with a short-term patch that fixes issues, as long as its not too intrusive. There is a very thin line between quick and dirty fixes to spaghetti unreadable code. But we should have comments and/or commit messages indicating that this needs a proper fix
I spent a lot of time explaining this last time.
Also, now that I have my lab back up and running and I would like these boards to work again on mainline!
> > Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo?
Remember, it was a patch you rejected :-)
I don't maintain any of that. I only gave some feedback along the lines of "bloblist was designed to be auto-discoverable, I don't see how adding an explicit Kconfig helps". IIRC we eventually followed what Tom suggested.
I'm not trying to point the finger here. So far the boards are broken in mainline...I'm just trying to fix that,
In any case, the amount of bike-shedding in the topic is too much. Do you mind explaining the problem in your workflow again? Perhaps we can find a solution that is integrated in bloblist_maybe_init() instead of injecting ifs on when a bloblist should or should not be searched for all over the place.
TPL (or SPL) sets up a bloblist with bits of info in it, but no DT (which is in memory-mapped SPI flash) U-Boot proper starts up, it wants to get the bloblist but not hang if the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a while back. Looking at the code, if the DT isn't found in a bloblist and the feature is enabled, we don't crash. We just print a debug message and continue to find the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Because at this point, if there is no bloblist, then it needs to be created. But creating it this early may fail, e.g. since there is no malloc(). The intent of this code is to locate an FDT from an existing bloblist. There is no point in creating a new bloblist here, since it obviously won't have an FDT in it.
Can we add a bool arg to bloblist_init for this? Eg. int bloblist_init(bool allow_malloc);
Yes, we can do anything, but that seems like a hack to me...if we init the bloblist for the first time in the current phase, then it obviously won't have an FDT inside it. It is the unconditional requirement of an FDT which is causing the problems here.
Regards, Simon
Hi all,
On Fri, 21 Jun 2024 at 12:16, Simon Glass sjg@chromium.org wrote:
Hi Raymond,
On Fri, 21 Jun 2024 at 10:40, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 10:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
Hi Ilias,
On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
> > > >> --- > > > >> > > > >> lib/fdtdec.c | 12 ++++++++++-- > > > >> 1 file changed, 10 insertions(+), 2 deletions(-) > > > >> > > > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > >> index b2c59ab3818..b141244e3b9 100644 > > > >> --- a/lib/fdtdec.c > > > >> +++ b/lib/fdtdec.c > > > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) > > > >> { > > > >> int ret = -ENOENT; > > > >> > > > >> - /* If allowing a bloblist, check that first */ > > > >> - if (CONFIG_IS_ENABLED(BLOBLIST)) { > > > >> + /* > > > >> + * If allowing a bloblist, check that first. This would be better > > > >> + * handled with an OF_BLOBLIST Kconfig, but that caused far too much > > > >> + * argument, so add a hack here, used e.g. by chromebook_coral > > > > > > > > I am a bit confused by this comment - It means you will not use OF_BLOBLIST, > > > > but actually you are using it below. Is it a typo? > > > > > > Basically it would be cleaner to have a separate, phase-specific > > > Kconfig control as to whether the DT can come from the bloblist (I > > > can't remember the Kconfig name I suggested, nor the patch as it was > > > last year sometime). But for now I am adding this hack to get a few > > > boards working again. > > > > I am a bit confused. > > First of all the comment is innapropriate. We went through a lengthy > > discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we > > made up our minds. Why are you adding this comment now? Why do code > > comments have to illustrate your personal opinion -- which was > > rejected? > > I'm sorry for the tone of the comment. I am not trying to offend > anyone here and I'm happy to change the language.
Yes please a comment explaining why that piece of code is there is far more intuitive.
OK, once we have agreed the below I can do that.
> As I probably > mentioned at the time, my accepted patch breaks my workflow and > several boards. I hope you can understand how frustrating that sort of > thing can be.
Yes, I do and I am fine with a short-term patch that fixes issues, as long as its not too intrusive. There is a very thin line between quick and dirty fixes to spaghetti unreadable code. But we should have comments and/or commit messages indicating that this needs a proper fix
I spent a lot of time explaining this last time.
> Also, now that I have my lab back up and running and I > would like these boards to work again on mainline! > > > > > Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo? > > Remember, it was a patch you rejected :-)
I don't maintain any of that. I only gave some feedback along the lines of "bloblist was designed to be auto-discoverable, I don't see how adding an explicit Kconfig helps". IIRC we eventually followed what Tom suggested.
I'm not trying to point the finger here. So far the boards are broken in mainline...I'm just trying to fix that,
In any case, the amount of bike-shedding in the topic is too much. Do you mind explaining the problem in your workflow again? Perhaps we can find a solution that is integrated in bloblist_maybe_init() instead of injecting ifs on when a bloblist should or should not be searched for all over the place.
TPL (or SPL) sets up a bloblist with bits of info in it, but no DT (which is in memory-mapped SPI flash) U-Boot proper starts up, it wants to get the bloblist but not hang if the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a while back. Looking at the code, if the DT isn't found in a bloblist and the feature is enabled, we don't crash. We just print a debug message and continue to find the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Because at this point, if there is no bloblist, then it needs to be created. But creating it this early may fail, e.g. since there is no malloc(). The intent of this code is to locate an FDT from an existing bloblist. There is no point in creating a new bloblist here, since it obviously won't have an FDT in it.
Can we add a bool arg to bloblist_init for this? Eg. int bloblist_init(bool allow_malloc);
Yes, we can do anything, but that seems like a hack to me...if we init the bloblist for the first time in the current phase, then it obviously won't have an FDT inside it. It is the unconditional requirement of an FDT which is causing the problems here.
Can we apply this patch, please? I still have several broken boards in my lab due to this.
Regards, Simon
Hi Simon,
On Tue, 30 Jul 2024 at 18:35, Simon Glass sjg@chromium.org wrote:
Hi all,
On Fri, 21 Jun 2024 at 12:16, Simon Glass sjg@chromium.org wrote:
Hi Raymond,
On Fri, 21 Jun 2024 at 10:40, Raymond Mao raymond.mao@linaro.org
wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 10:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
Hi Ilias,
On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote: > > [...] > > > > > >> --- > > > > >> > > > > >> lib/fdtdec.c | 12 ++++++++++-- > > > > >> 1 file changed, 10 insertions(+), 2 deletions(-) > > > > >> > > > > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > > >> index b2c59ab3818..b141244e3b9 100644 > > > > >> --- a/lib/fdtdec.c > > > > >> +++ b/lib/fdtdec.c > > > > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) > > > > >> { > > > > >> int ret = -ENOENT; > > > > >> > > > > >> - /* If allowing a bloblist, check that first */ > > > > >> - if (CONFIG_IS_ENABLED(BLOBLIST)) { > > > > >> + /* > > > > >> + * If allowing a bloblist, check that first.
This would be better
> > > > >> + * handled with an OF_BLOBLIST Kconfig, but
that caused far too much
> > > > >> + * argument, so add a hack here, used e.g. by
chromebook_coral
> > > > > > > > > > I am a bit confused by this comment - It means you will
not use OF_BLOBLIST,
> > > > > but actually you are using it below. Is it a typo? > > > > > > > > Basically it would be cleaner to have a separate,
phase-specific
> > > > Kconfig control as to whether the DT can come from the
bloblist (I
> > > > can't remember the Kconfig name I suggested, nor the
patch as it was
> > > > last year sometime). But for now I am adding this hack to
get a few
> > > > boards working again. > > > > > > I am a bit confused. > > > First of all the comment is innapropriate. We went through
a lengthy
> > > discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom
chimed in and we
> > > made up our minds. Why are you adding this comment now? Why
do code
> > > comments have to illustrate your personal opinion -- which
was
> > > rejected? > > > > I'm sorry for the tone of the comment. I am not trying to
offend
> > anyone here and I'm happy to change the language. > > Yes please a comment explaining why that piece of code is there
is far
> more intuitive.
OK, once we have agreed the below I can do that.
> > > As I probably > > mentioned at the time, my accepted patch breaks my workflow
and
> > several boards. I hope you can understand how frustrating
that sort of
> > thing can be. > > Yes, I do and I am fine with a short-term patch that fixes
issues, as
> long as its not too intrusive. There is a very thin line
between quick
> and dirty fixes to spaghetti unreadable code. But we should have > comments and/or commit messages indicating that this needs a
proper
> fix
I spent a lot of time explaining this last time.
> > > Also, now that I have my lab back up and running and I > > would like these boards to work again on mainline! > > > > > > > > Grepping for OF_BLOBLIST, I can't find any matches, so is
the above if a typo?
> > > > Remember, it was a patch you rejected :-) > > I don't maintain any of that. I only gave some feedback along
the
> lines of "bloblist was designed to be auto-discoverable, I
don't see
> how adding an explicit Kconfig helps". IIRC we eventually
followed
> what Tom suggested.
I'm not trying to point the finger here. So far the boards are
broken
in mainline...I'm just trying to fix that,
> > In any case, the amount of bike-shedding in the topic is too
much. Do
> you mind explaining the problem in your workflow again? Perhaps
we can
> find a solution that is integrated in bloblist_maybe_init()
instead of
> injecting ifs on when a bloblist should or should not be
searched for
> all over the place.
TPL (or SPL) sets up a bloblist with bits of info in it, but no DT (which is in memory-mapped SPI flash) U-Boot proper starts up, it wants to get the bloblist but not
hang if
the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a
while back.
Looking at the code, if the DT isn't found in a bloblist and the
feature is
enabled, we don't crash. We just print a debug message and continue
to find
the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Because at this point, if there is no bloblist, then it needs to be created. But creating it this early may fail, e.g. since there is no malloc(). The intent of this code is to locate an FDT from an existing bloblist. There is no point in creating a new bloblist here, since it obviously won't have an FDT in it.
Can we add a bool arg to bloblist_init for this? Eg. int bloblist_init(bool allow_malloc);
Yes, we can do anything, but that seems like a hack to me...if we init the bloblist for the first time in the current phase, then it obviously won't have an FDT inside it. It is the unconditional requirement of an FDT which is causing the problems here.
Can we apply this patch, please? I still have several broken boards in my lab due to this.
Acked-by: Raymond Mao raymond.mao@linaro.org
Regards, Raymond
Hi Raymond,
On Wed, 31 Jul 2024 at 07:59, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Tue, 30 Jul 2024 at 18:35, Simon Glass sjg@chromium.org wrote:
Hi all,
On Fri, 21 Jun 2024 at 12:16, Simon Glass sjg@chromium.org wrote:
Hi Raymond,
On Fri, 21 Jun 2024 at 10:40, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 10:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote: > Hi Ilias, > > On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas > ilias.apalodimas@linaro.org wrote: > > > > [...] > > > > > > > >> --- > > > > > >> > > > > > >> lib/fdtdec.c | 12 ++++++++++-- > > > > > >> 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > >> > > > > > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > > > >> index b2c59ab3818..b141244e3b9 100644 > > > > > >> --- a/lib/fdtdec.c > > > > > >> +++ b/lib/fdtdec.c > > > > > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) > > > > > >> { > > > > > >> int ret = -ENOENT; > > > > > >> > > > > > >> - /* If allowing a bloblist, check that first */ > > > > > >> - if (CONFIG_IS_ENABLED(BLOBLIST)) { > > > > > >> + /* > > > > > >> + * If allowing a bloblist, check that first. This would be better > > > > > >> + * handled with an OF_BLOBLIST Kconfig, but that caused far too much > > > > > >> + * argument, so add a hack here, used e.g. by chromebook_coral > > > > > > > > > > > > I am a bit confused by this comment - It means you will not use OF_BLOBLIST, > > > > > > but actually you are using it below. Is it a typo? > > > > > > > > > > Basically it would be cleaner to have a separate, phase-specific > > > > > Kconfig control as to whether the DT can come from the bloblist (I > > > > > can't remember the Kconfig name I suggested, nor the patch as it was > > > > > last year sometime). But for now I am adding this hack to get a few > > > > > boards working again. > > > > > > > > I am a bit confused. > > > > First of all the comment is innapropriate. We went through a lengthy > > > > discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we > > > > made up our minds. Why are you adding this comment now? Why do code > > > > comments have to illustrate your personal opinion -- which was > > > > rejected? > > > > > > I'm sorry for the tone of the comment. I am not trying to offend > > > anyone here and I'm happy to change the language. > > > > Yes please a comment explaining why that piece of code is there is far > > more intuitive. > > OK, once we have agreed the below I can do that. > > > > > > As I probably > > > mentioned at the time, my accepted patch breaks my workflow and > > > several boards. I hope you can understand how frustrating that sort of > > > thing can be. > > > > Yes, I do and I am fine with a short-term patch that fixes issues, as > > long as its not too intrusive. There is a very thin line between quick > > and dirty fixes to spaghetti unreadable code. But we should have > > comments and/or commit messages indicating that this needs a proper > > fix > > I spent a lot of time explaining this last time. > > > > > > Also, now that I have my lab back up and running and I > > > would like these boards to work again on mainline! > > > > > > > > > > > Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo? > > > > > > Remember, it was a patch you rejected :-) > > > > I don't maintain any of that. I only gave some feedback along the > > lines of "bloblist was designed to be auto-discoverable, I don't see > > how adding an explicit Kconfig helps". IIRC we eventually followed > > what Tom suggested. > > I'm not trying to point the finger here. So far the boards are broken > in mainline...I'm just trying to fix that, > > > > > In any case, the amount of bike-shedding in the topic is too much. Do > > you mind explaining the problem in your workflow again? Perhaps we can > > find a solution that is integrated in bloblist_maybe_init() instead of > > injecting ifs on when a bloblist should or should not be searched for > > all over the place. > > TPL (or SPL) sets up a bloblist with bits of info in it, but no DT > (which is in memory-mapped SPI flash) > U-Boot proper starts up, it wants to get the bloblist but not hang if > the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a while back. Looking at the code, if the DT isn't found in a bloblist and the feature is enabled, we don't crash. We just print a debug message and continue to find the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Because at this point, if there is no bloblist, then it needs to be created. But creating it this early may fail, e.g. since there is no malloc(). The intent of this code is to locate an FDT from an existing bloblist. There is no point in creating a new bloblist here, since it obviously won't have an FDT in it.
Can we add a bool arg to bloblist_init for this? Eg. int bloblist_init(bool allow_malloc);
Yes, we can do anything, but that seems like a hack to me...if we init the bloblist for the first time in the current phase, then it obviously won't have an FDT inside it. It is the unconditional requirement of an FDT which is causing the problems here.
Can we apply this patch, please? I still have several broken boards in my lab due to this.
Acked-by: Raymond Mao raymond.mao@linaro.org
Thank you.
Regards, Simon
Hi Simon,
On Wed, 31 Jul 2024 at 01:35, Simon Glass sjg@chromium.org wrote:
Hi all,
On Fri, 21 Jun 2024 at 12:16, Simon Glass sjg@chromium.org wrote:
Hi Raymond,
On Fri, 21 Jun 2024 at 10:40, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 10:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
Hi Ilias,
On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote: > > [...] > > > > > >> --- > > > > >> > > > > >> lib/fdtdec.c | 12 ++++++++++-- > > > > >> 1 file changed, 10 insertions(+), 2 deletions(-) > > > > >> > > > > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > > >> index b2c59ab3818..b141244e3b9 100644 > > > > >> --- a/lib/fdtdec.c > > > > >> +++ b/lib/fdtdec.c > > > > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) > > > > >> { > > > > >> int ret = -ENOENT; > > > > >> > > > > >> - /* If allowing a bloblist, check that first */ > > > > >> - if (CONFIG_IS_ENABLED(BLOBLIST)) { > > > > >> + /* > > > > >> + * If allowing a bloblist, check that first. This would be better > > > > >> + * handled with an OF_BLOBLIST Kconfig, but that caused far too much > > > > >> + * argument, so add a hack here, used e.g. by chromebook_coral > > > > > > > > > > I am a bit confused by this comment - It means you will not use OF_BLOBLIST, > > > > > but actually you are using it below. Is it a typo? > > > > > > > > Basically it would be cleaner to have a separate, phase-specific > > > > Kconfig control as to whether the DT can come from the bloblist (I > > > > can't remember the Kconfig name I suggested, nor the patch as it was > > > > last year sometime). But for now I am adding this hack to get a few > > > > boards working again. > > > > > > I am a bit confused. > > > First of all the comment is innapropriate. We went through a lengthy > > > discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we > > > made up our minds. Why are you adding this comment now? Why do code > > > comments have to illustrate your personal opinion -- which was > > > rejected? > > > > I'm sorry for the tone of the comment. I am not trying to offend > > anyone here and I'm happy to change the language. > > Yes please a comment explaining why that piece of code is there is far > more intuitive.
OK, once we have agreed the below I can do that.
> > > As I probably > > mentioned at the time, my accepted patch breaks my workflow and > > several boards. I hope you can understand how frustrating that sort of > > thing can be. > > Yes, I do and I am fine with a short-term patch that fixes issues, as > long as its not too intrusive. There is a very thin line between quick > and dirty fixes to spaghetti unreadable code. But we should have > comments and/or commit messages indicating that this needs a proper > fix
I spent a lot of time explaining this last time.
> > > Also, now that I have my lab back up and running and I > > would like these boards to work again on mainline! > > > > > > > > Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo? > > > > Remember, it was a patch you rejected :-) > > I don't maintain any of that. I only gave some feedback along the > lines of "bloblist was designed to be auto-discoverable, I don't see > how adding an explicit Kconfig helps". IIRC we eventually followed > what Tom suggested.
I'm not trying to point the finger here. So far the boards are broken in mainline...I'm just trying to fix that,
> > In any case, the amount of bike-shedding in the topic is too much. Do > you mind explaining the problem in your workflow again? Perhaps we can > find a solution that is integrated in bloblist_maybe_init() instead of > injecting ifs on when a bloblist should or should not be searched for > all over the place.
TPL (or SPL) sets up a bloblist with bits of info in it, but no DT (which is in memory-mapped SPI flash) U-Boot proper starts up, it wants to get the bloblist but not hang if the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a while back. Looking at the code, if the DT isn't found in a bloblist and the feature is enabled, we don't crash. We just print a debug message and continue to find the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Because at this point, if there is no bloblist, then it needs to be created. But creating it this early may fail, e.g. since there is no malloc(). The intent of this code is to locate an FDT from an existing bloblist. There is no point in creating a new bloblist here, since it obviously won't have an FDT in it.
Can we add a bool arg to bloblist_init for this? Eg. int bloblist_init(bool allow_malloc);
Yes, we can do anything, but that seems like a hack to me...if we init the bloblist for the first time in the current phase, then it obviously won't have an FDT inside it. It is the unconditional requirement of an FDT which is causing the problems here.
Can we apply this patch, please? I still have several broken boards in my lab due to this.
Raymond acked the patch, I don't mind merging it
Regards /Ilias
Regards, Simon
Hi Ilias,
On Wed, 31 Jul 2024 at 14:09, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 31 Jul 2024 at 01:35, Simon Glass sjg@chromium.org wrote:
Hi all,
On Fri, 21 Jun 2024 at 12:16, Simon Glass sjg@chromium.org wrote:
Hi Raymond,
On Fri, 21 Jun 2024 at 10:40, Raymond Mao raymond.mao@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 10:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote: > Hi Ilias, > > On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas > ilias.apalodimas@linaro.org wrote: > > > > [...] > > > > > > > >> --- > > > > > >> > > > > > >> lib/fdtdec.c | 12 ++++++++++-- > > > > > >> 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > >> > > > > > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c > > > > > >> index b2c59ab3818..b141244e3b9 100644 > > > > > >> --- a/lib/fdtdec.c > > > > > >> +++ b/lib/fdtdec.c > > > > > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void) > > > > > >> { > > > > > >> int ret = -ENOENT; > > > > > >> > > > > > >> - /* If allowing a bloblist, check that first */ > > > > > >> - if (CONFIG_IS_ENABLED(BLOBLIST)) { > > > > > >> + /* > > > > > >> + * If allowing a bloblist, check that first. This would be better > > > > > >> + * handled with an OF_BLOBLIST Kconfig, but that caused far too much > > > > > >> + * argument, so add a hack here, used e.g. by chromebook_coral > > > > > > > > > > > > I am a bit confused by this comment - It means you will not use OF_BLOBLIST, > > > > > > but actually you are using it below. Is it a typo? > > > > > > > > > > Basically it would be cleaner to have a separate, phase-specific > > > > > Kconfig control as to whether the DT can come from the bloblist (I > > > > > can't remember the Kconfig name I suggested, nor the patch as it was > > > > > last year sometime). But for now I am adding this hack to get a few > > > > > boards working again. > > > > > > > > I am a bit confused. > > > > First of all the comment is innapropriate. We went through a lengthy > > > > discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed in and we > > > > made up our minds. Why are you adding this comment now? Why do code > > > > comments have to illustrate your personal opinion -- which was > > > > rejected? > > > > > > I'm sorry for the tone of the comment. I am not trying to offend > > > anyone here and I'm happy to change the language. > > > > Yes please a comment explaining why that piece of code is there is far > > more intuitive. > > OK, once we have agreed the below I can do that. > > > > > > As I probably > > > mentioned at the time, my accepted patch breaks my workflow and > > > several boards. I hope you can understand how frustrating that sort of > > > thing can be. > > > > Yes, I do and I am fine with a short-term patch that fixes issues, as > > long as its not too intrusive. There is a very thin line between quick > > and dirty fixes to spaghetti unreadable code. But we should have > > comments and/or commit messages indicating that this needs a proper > > fix > > I spent a lot of time explaining this last time. > > > > > > Also, now that I have my lab back up and running and I > > > would like these boards to work again on mainline! > > > > > > > > > > > Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo? > > > > > > Remember, it was a patch you rejected :-) > > > > I don't maintain any of that. I only gave some feedback along the > > lines of "bloblist was designed to be auto-discoverable, I don't see > > how adding an explicit Kconfig helps". IIRC we eventually followed > > what Tom suggested. > > I'm not trying to point the finger here. So far the boards are broken > in mainline...I'm just trying to fix that, > > > > > In any case, the amount of bike-shedding in the topic is too much. Do > > you mind explaining the problem in your workflow again? Perhaps we can > > find a solution that is integrated in bloblist_maybe_init() instead of > > injecting ifs on when a bloblist should or should not be searched for > > all over the place. > > TPL (or SPL) sets up a bloblist with bits of info in it, but no DT > (which is in memory-mapped SPI flash) > U-Boot proper starts up, it wants to get the bloblist but not hang if > the bloblist doesn't have a DT
Sure, that's reasonable and IIRC that's the design we agreed a while back. Looking at the code, if the DT isn't found in a bloblist and the feature is enabled, we don't crash. We just print a debug message and continue to find the DT as we used. Why do we have to skip the call to bloblist_maybe_init()?
Because at this point, if there is no bloblist, then it needs to be created. But creating it this early may fail, e.g. since there is no malloc(). The intent of this code is to locate an FDT from an existing bloblist. There is no point in creating a new bloblist here, since it obviously won't have an FDT in it.
Can we add a bool arg to bloblist_init for this? Eg. int bloblist_init(bool allow_malloc);
Yes, we can do anything, but that seems like a hack to me...if we init the bloblist for the first time in the current phase, then it obviously won't have an FDT inside it. It is the unconditional requirement of an FDT which is causing the problems here.
Can we apply this patch, please? I still have several broken boards in my lab due to this.
Raymond acked the patch, I don't mind merging it
OK thank you.
There is another one too - see the note at the end.
https://patchwork.ozlabs.org/project/uboot/patch/20240721100940.3202027-6-sj...
Regards, Simon
The dcache may not be enabled in SPL. Add a check to avoid trying to use an undefined function.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/spl/spl_atf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/common/spl/spl_atf.c b/common/spl/spl_atf.c index 3bdd013a35f..9afe6456bc4 100644 --- a/common/spl/spl_atf.c +++ b/common/spl/spl_atf.c @@ -204,7 +204,8 @@ static void __noreturn bl31_entry(uintptr_t bl31_entry, uintptr_t bl32_entry, fdt_addr);
raw_write_daif(SPSR_EXCEPTION_MASK); - dcache_disable(); + if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) + dcache_disable();
atf_entry(bl31_params, (void *)fdt_addr); }
On Tue, Jun 04, 2024 at 09:25:18PM -0600, Simon Glass wrote:
The dcache may not be enabled in SPL. Add a check to avoid trying to use an undefined function.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Tom Rini trini@konsulko.com
This causes a hang, so disable it.
Fixes: 6d8cdfd1536 ("rockchip: spl: Enable caches to speed up checksum validation")
Signed-off-by: Simon Glass sjg@chromium.org ---
configs/chromebook_bob_defconfig | 1 + configs/chromebook_kevin_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/chromebook_bob_defconfig b/configs/chromebook_bob_defconfig index acfe3934104..b2ecfa6050c 100644 --- a/configs/chromebook_bob_defconfig +++ b/configs/chromebook_bob_defconfig @@ -1,5 +1,6 @@ CONFIG_ARM=y CONFIG_SKIP_LOWLEVEL_INIT=y +CONFIG_SPL_SYS_DCACHE_OFF=y CONFIG_COUNTER_FREQUENCY=24000000 CONFIG_ARCH_ROCKCHIP=y CONFIG_TEXT_BASE=0x00200000 diff --git a/configs/chromebook_kevin_defconfig b/configs/chromebook_kevin_defconfig index 95fdb418d82..da748e4f022 100644 --- a/configs/chromebook_kevin_defconfig +++ b/configs/chromebook_kevin_defconfig @@ -2,6 +2,7 @@ CONFIG_ARM=y CONFIG_SKIP_LOWLEVEL_INIT=y CONFIG_COUNTER_FREQUENCY=24000000 CONFIG_ARCH_ROCKCHIP=y +CONFIG_SPL_SYS_DCACHE_OFF=y CONFIG_TEXT_BASE=0x00200000 CONFIG_SPL_GPIO=y CONFIG_NR_DRAM_BANKS=1
Hi Simon,
On 2024-06-05 05:25, Simon Glass wrote:
This causes a hang, so disable it.
When I initially tested this on multiple boards there was some boards that also hanged, that turned out to be an issue in one of the drivers.
If I remember correctly such hang was related to a null pointer dereference or unaligned access in one of the drivers.
Could there be a similar underlying issue for these boards?
Regards, Jonas
Fixes: 6d8cdfd1536 ("rockchip: spl: Enable caches to speed up checksum validation")
Signed-off-by: Simon Glass sjg@chromium.org
configs/chromebook_bob_defconfig | 1 + configs/chromebook_kevin_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/chromebook_bob_defconfig b/configs/chromebook_bob_defconfig index acfe3934104..b2ecfa6050c 100644 --- a/configs/chromebook_bob_defconfig +++ b/configs/chromebook_bob_defconfig @@ -1,5 +1,6 @@ CONFIG_ARM=y CONFIG_SKIP_LOWLEVEL_INIT=y +CONFIG_SPL_SYS_DCACHE_OFF=y CONFIG_COUNTER_FREQUENCY=24000000 CONFIG_ARCH_ROCKCHIP=y CONFIG_TEXT_BASE=0x00200000 diff --git a/configs/chromebook_kevin_defconfig b/configs/chromebook_kevin_defconfig index 95fdb418d82..da748e4f022 100644 --- a/configs/chromebook_kevin_defconfig +++ b/configs/chromebook_kevin_defconfig @@ -2,6 +2,7 @@ CONFIG_ARM=y CONFIG_SKIP_LOWLEVEL_INIT=y CONFIG_COUNTER_FREQUENCY=24000000 CONFIG_ARCH_ROCKCHIP=y +CONFIG_SPL_SYS_DCACHE_OFF=y CONFIG_TEXT_BASE=0x00200000 CONFIG_SPL_GPIO=y CONFIG_NR_DRAM_BANKS=1
Hi Jonas,
On Wed, 5 Jun 2024 at 05:07, Jonas Karlman jonas@kwiboo.se wrote:
Hi Simon,
On 2024-06-05 05:25, Simon Glass wrote:
This causes a hang, so disable it.
When I initially tested this on multiple boards there was some boards that also hanged, that turned out to be an issue in one of the drivers.
If I remember correctly such hang was related to a null pointer dereference or unaligned access in one of the drivers.
Could there be a similar underlying issue for these boards?
Yes it could be. I will take a look.
But given the release date I would like to get this patch in first.
Regards, Simon
Hi Jonas,
On Thu, 6 Jun 2024 at 09:04, Simon Glass sjg@chromium.org wrote:
Hi Jonas,
On Wed, 5 Jun 2024 at 05:07, Jonas Karlman jonas@kwiboo.se wrote:
Hi Simon,
On 2024-06-05 05:25, Simon Glass wrote:
This causes a hang, so disable it.
When I initially tested this on multiple boards there was some boards that also hanged, that turned out to be an issue in one of the drivers.
If I remember correctly such hang was related to a null pointer dereference or unaligned access in one of the drivers.
Could there be a similar underlying issue for these boards?
Yes it could be. I will take a look.
But given the release date I would like to get this patch in first.
I did find a bug in memory sizing, but fixing that was not enough to get the cache running. I am not sure how to debug it, since presumably the code works fine on other rk3399 boards (sadly mine broke a few days ago).
Regards, Simon
A recent change introduced a bug whereby a PMIC device is used in place of the regulator device. Fix it.
This fixes a hang after 'Loading Environment from nowhere... OK' on chromebook_jerry
Fixes: f047e4ab976 ("regulator: rk8xx: add indirection level for some..")
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/power/regulator/rk8xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c index 1bd4605d43a..bffc5d2dd65 100644 --- a/drivers/power/regulator/rk8xx.c +++ b/drivers/power/regulator/rk8xx.c @@ -1240,7 +1240,7 @@ static int ldo_set_suspend_value(struct udevice *dev, int uvolt) int ldo = dev->driver_data - 1; const struct rk8xx_reg_info *info = get_ldo_reg(dev->parent, ldo, uvolt);
- return _ldo_set_suspend_value(dev->parent, info, uvolt); + return _ldo_set_suspend_value(dev, info, uvolt); }
static int nldo_set_suspend_value(struct udevice *dev, int uvolt)
Hi Simon,
On 6/5/24 5:25 AM, Simon Glass wrote:
A recent change introduced a bug whereby a PMIC device is used in place of the regulator device. Fix it.
Good catch, but the suggested fix is only fixing one of those bugs, I've introduced quite a few actually.
I'm sending a patch series to fix these as well.
Please do not merge this patch.
Cheers, Quentin
This is a partial revert which makes boneblack_vboot boot again.
This reverts commit f4b64e9736e73ceec14d51600bed9a8ac48f9fe8.
Signed-off-by: Simon Glass sjg@chromium.org ---
configs/am335x_boneblack_vboot_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am335x_boneblack_vboot_defconfig b/configs/am335x_boneblack_vboot_defconfig index d473a1a793b..3ec4abddd77 100644 --- a/configs/am335x_boneblack_vboot_defconfig +++ b/configs/am335x_boneblack_vboot_defconfig @@ -40,7 +40,6 @@ CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2 # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_DNS2=y CONFIG_OF_CONTROL=y -CONFIG_SPL_OF_CONTROL=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_MMC=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
On Tue, Jun 04, 2024 at 09:25:21PM -0600, Simon Glass wrote:
This is a partial revert which makes boneblack_vboot boot again.
This reverts commit f4b64e9736e73ceec14d51600bed9a8ac48f9fe8.
Signed-off-by: Simon Glass sjg@chromium.org
configs/am335x_boneblack_vboot_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am335x_boneblack_vboot_defconfig b/configs/am335x_boneblack_vboot_defconfig index d473a1a793b..3ec4abddd77 100644 --- a/configs/am335x_boneblack_vboot_defconfig +++ b/configs/am335x_boneblack_vboot_defconfig @@ -40,7 +40,6 @@ CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2 # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_DNS2=y CONFIG_OF_CONTROL=y -CONFIG_SPL_OF_CONTROL=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_MMC=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
So, this change was a while ago. But I did it because some of the drivers, tho I forget which ones exactly, really were not functional without SPL_OF_CONTROL enabled. That said, does am335x_evm_defconfig work in your configuration? I think we have to keep am335x_evm_spiboot because it's so radically different from how the rest of the possible options boot, but I'm not sure we need a vboot defconfig? Or if we do, it should be updated to use the #include logic these days.
Hi Tom,
On Wed, 5 Jun 2024 at 08:42, Tom Rini trini@konsulko.com wrote:
On Tue, Jun 04, 2024 at 09:25:21PM -0600, Simon Glass wrote:
This is a partial revert which makes boneblack_vboot boot again.
This reverts commit f4b64e9736e73ceec14d51600bed9a8ac48f9fe8.
Signed-off-by: Simon Glass sjg@chromium.org
configs/am335x_boneblack_vboot_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am335x_boneblack_vboot_defconfig b/configs/am335x_boneblack_vboot_defconfig index d473a1a793b..3ec4abddd77 100644 --- a/configs/am335x_boneblack_vboot_defconfig +++ b/configs/am335x_boneblack_vboot_defconfig @@ -40,7 +40,6 @@ CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2 # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_DNS2=y CONFIG_OF_CONTROL=y -CONFIG_SPL_OF_CONTROL=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_MMC=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
So, this change was a while ago. But I did it because some of the drivers, tho I forget which ones exactly, really were not functional without SPL_OF_CONTROL enabled. That said, does am335x_evm_defconfig work in your configuration? I think we have to keep am335x_evm_spiboot because it's so radically different from how the rest of the possible options boot, but I'm not sure we need a vboot defconfig? Or if we do, it should be updated to use the #include logic these days.
I don't actually have an am335x_evm board, which is why I'm using bbb. We only have the vboot version, since the non-vboot was removed a while back.
In any case, with this patch, bbb boots again.
Regards, Simon
On Wed, Jun 05, 2024 at 02:08:54PM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 5 Jun 2024 at 08:42, Tom Rini trini@konsulko.com wrote:
On Tue, Jun 04, 2024 at 09:25:21PM -0600, Simon Glass wrote:
This is a partial revert which makes boneblack_vboot boot again.
This reverts commit f4b64e9736e73ceec14d51600bed9a8ac48f9fe8.
Signed-off-by: Simon Glass sjg@chromium.org
configs/am335x_boneblack_vboot_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am335x_boneblack_vboot_defconfig b/configs/am335x_boneblack_vboot_defconfig index d473a1a793b..3ec4abddd77 100644 --- a/configs/am335x_boneblack_vboot_defconfig +++ b/configs/am335x_boneblack_vboot_defconfig @@ -40,7 +40,6 @@ CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2 # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_DNS2=y CONFIG_OF_CONTROL=y -CONFIG_SPL_OF_CONTROL=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_MMC=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
So, this change was a while ago. But I did it because some of the drivers, tho I forget which ones exactly, really were not functional without SPL_OF_CONTROL enabled. That said, does am335x_evm_defconfig work in your configuration? I think we have to keep am335x_evm_spiboot because it's so radically different from how the rest of the possible options boot, but I'm not sure we need a vboot defconfig? Or if we do, it should be updated to use the #include logic these days.
I don't actually have an am335x_evm board, which is why I'm using bbb. We only have the vboot version, since the non-vboot was removed a while back.
In any case, with this patch, bbb boots again.
Yes, the am335x_evm build supports all of the TI (or associated, like beagleboard) platforms. Please give it a try.
Hi Tom,
On Wed, 5 Jun 2024 at 14:15, Tom Rini trini@konsulko.com wrote:
On Wed, Jun 05, 2024 at 02:08:54PM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 5 Jun 2024 at 08:42, Tom Rini trini@konsulko.com wrote:
On Tue, Jun 04, 2024 at 09:25:21PM -0600, Simon Glass wrote:
This is a partial revert which makes boneblack_vboot boot again.
This reverts commit f4b64e9736e73ceec14d51600bed9a8ac48f9fe8.
Signed-off-by: Simon Glass sjg@chromium.org
configs/am335x_boneblack_vboot_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am335x_boneblack_vboot_defconfig b/configs/am335x_boneblack_vboot_defconfig index d473a1a793b..3ec4abddd77 100644 --- a/configs/am335x_boneblack_vboot_defconfig +++ b/configs/am335x_boneblack_vboot_defconfig @@ -40,7 +40,6 @@ CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2 # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_DNS2=y CONFIG_OF_CONTROL=y -CONFIG_SPL_OF_CONTROL=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_MMC=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
So, this change was a while ago. But I did it because some of the drivers, tho I forget which ones exactly, really were not functional without SPL_OF_CONTROL enabled. That said, does am335x_evm_defconfig work in your configuration? I think we have to keep am335x_evm_spiboot because it's so radically different from how the rest of the possible options boot, but I'm not sure we need a vboot defconfig? Or if we do, it should be updated to use the #include logic these days.
I don't actually have an am335x_evm board, which is why I'm using bbb. We only have the vboot version, since the non-vboot was removed a while back.
In any case, with this patch, bbb boots again.
Yes, the am335x_evm build supports all of the TI (or associated, like beagleboard) platforms. Please give it a try.
Yes, that works. So what should we do with the boneblack config? We really should have something that boots.
Regards, Simon
On Thu, Jun 06, 2024 at 09:04:17AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 5 Jun 2024 at 14:15, Tom Rini trini@konsulko.com wrote:
On Wed, Jun 05, 2024 at 02:08:54PM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 5 Jun 2024 at 08:42, Tom Rini trini@konsulko.com wrote:
On Tue, Jun 04, 2024 at 09:25:21PM -0600, Simon Glass wrote:
This is a partial revert which makes boneblack_vboot boot again.
This reverts commit f4b64e9736e73ceec14d51600bed9a8ac48f9fe8.
Signed-off-by: Simon Glass sjg@chromium.org
configs/am335x_boneblack_vboot_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/am335x_boneblack_vboot_defconfig b/configs/am335x_boneblack_vboot_defconfig index d473a1a793b..3ec4abddd77 100644 --- a/configs/am335x_boneblack_vboot_defconfig +++ b/configs/am335x_boneblack_vboot_defconfig @@ -40,7 +40,6 @@ CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2 # CONFIG_CMD_SETEXPR is not set CONFIG_BOOTP_DNS2=y CONFIG_OF_CONTROL=y -CONFIG_SPL_OF_CONTROL=y CONFIG_ENV_OVERWRITE=y CONFIG_ENV_IS_IN_MMC=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
So, this change was a while ago. But I did it because some of the drivers, tho I forget which ones exactly, really were not functional without SPL_OF_CONTROL enabled. That said, does am335x_evm_defconfig work in your configuration? I think we have to keep am335x_evm_spiboot because it's so radically different from how the rest of the possible options boot, but I'm not sure we need a vboot defconfig? Or if we do, it should be updated to use the #include logic these days.
I don't actually have an am335x_evm board, which is why I'm using bbb. We only have the vboot version, since the non-vboot was removed a while back.
In any case, with this patch, bbb boots again.
Yes, the am335x_evm build supports all of the TI (or associated, like beagleboard) platforms. Please give it a try.
Yes, that works. So what should we do with the boneblack config? We really should have something that boots.
We should delete it and make sure the vboot docs are still up to date / correct.
participants (7)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Jonas Karlman -
Quentin Schulz -
Raymond Mao -
Simon Glass -
Tom Rini