[PATCH v3 00/18] Bug-fixes for a few boards
This series includes fixes to get some rockchip and nvidia boards working again. It also drops the broken Beaglebone Black config and provides a devicetree fix for coral (x86).
Changes in v3: - Use BLOBLIST instead of OF_BLOBLIST - Cut the patch down to bare bones - Split out the refactoring into a separate patch - Drop the non-dcache optimisation, since the cache should normally be on
Changes in v2: - Put the conditions under EFI_TCG2_PROTOCOL - Consider MEASURED_BOOT too - Remove the superfluous if() and drop the debug() as well - Use 'phase' instead of 'stage' - Add new patch to correct memory size in SPL - Drop patch "regulator: rk8xx: Fix incorrect parameter" - Rewrite boneblack patch to onstead drop the target and update docs
Simon Glass (18): binman: Support an assumed size for missing binaries binman: Make Intel ME default to position 0x1000 mkeficapsule: Add a --version argument binman: Collect the version number for mkeficapsule binman: Deal with mkeficapsule being missing binman: Return failure when a usage() message is generated binman: Keep the efi_capsule input file x86: Set up some assumed sizes for binary blobs 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: Ensure memory size is available in RK3399 SPL rockchip: Avoid #ifdefs in RK3399 SPL rockchip: bob: kevin: Disable dcache in SPL Drop the special am335x_boneblack_vboot target
arch/x86/dts/u-boot.dtsi | 5 ++ board/google/veyron/veyron.c | 30 +++---- board/ti/am335x/MAINTAINERS | 1 - boot/Kconfig | 4 + common/spl/spl_atf.c | 3 +- configs/am335x_boneblack_vboot_defconfig | 94 ---------------------- configs/am335x_evm_defconfig | 3 +- configs/chromebook_bob_defconfig | 1 + configs/chromebook_kevin_defconfig | 1 + configs/nyan-big_defconfig | 1 - doc/mkeficapsule.1 | 4 + doc/usage/fit/beaglebone_vboot.rst | 21 +++-- drivers/power/regulator/regulator-uclass.c | 2 +- drivers/ram/rockchip/sdram_rk3399.c | 49 ++++++----- lib/Kconfig | 4 - lib/fdtdec.c | 12 ++- tools/binman/binman.rst | 7 ++ tools/binman/btool/mkeficapsule.py | 3 +- tools/binman/entry.py | 1 + tools/binman/etype/blob.py | 7 +- tools/binman/etype/efi_capsule.py | 5 +- tools/binman/etype/intel_descriptor.py | 2 +- tools/binman/ftest.py | 28 +++++++ tools/binman/test/326_assume_size.dts | 16 ++++ tools/binman/test/327_assume_size_ok.dts | 16 ++++ tools/mkeficapsule.c | 10 ++- 26 files changed, 168 insertions(+), 162 deletions(-) delete mode 100644 configs/am335x_boneblack_vboot_defconfig create mode 100644 tools/binman/test/326_assume_size.dts create mode 100644 tools/binman/test/327_assume_size_ok.dts
Binman has a the useful feature of handling missing external blobs gracefully, including allowing them to be missing, deciding whether the resulting image is functional or not and faking blobs when this is necessary for particular tools (e.g. mkimage).
This feature is widely used in CI. One drawback is that if U-Boot grows too large to fit along with the required blobs, then this is not discovered until someone does a 'real' build which includes the blobs.
Add a 'assume-size' property to entries to allow Binman to reserve a given size for missing external blobs.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
tools/binman/binman.rst | 7 ++++++ tools/binman/entry.py | 1 + tools/binman/etype/blob.py | 7 +++++- tools/binman/ftest.py | 28 ++++++++++++++++++++++++ tools/binman/test/326_assume_size.dts | 16 ++++++++++++++ tools/binman/test/327_assume_size_ok.dts | 16 ++++++++++++++ 6 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/326_assume_size.dts create mode 100644 tools/binman/test/327_assume_size_ok.dts
diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 230e055667f..872e9746c8c 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -711,6 +711,13 @@ missing-msg: information about what needs to be fixed. See missing-blob-help for the message for each tag.
+assume-size: + Sets the assumed size of a blob entry if it is missing. This allows for a + check that the rest of the image fits into the available space, even when + the contents are not available. If the entry is missing, Binman will use + this assumed size for the entry size, including creating a fake file of that + size if requested. + no-expanded: By default binman substitutes entries with expanded versions if available, so that a `u-boot` entry type turns into `u-boot-expanded`, for example. The diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 42e0b7b9145..c1904f8ae69 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -315,6 +315,7 @@ class Entry(object): self.overlap = fdt_util.GetBool(self._node, 'overlap') if self.overlap: self.required_props += ['offset', 'size'] + self.assume_size = fdt_util.GetInt(self._node, 'assume-size', 0)
# This is only supported by blobs and sections at present self.compress = fdt_util.GetString(self._node, 'compress', 'none') diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py index 064fae50365..041e1122953 100644 --- a/tools/binman/etype/blob.py +++ b/tools/binman/etype/blob.py @@ -48,11 +48,16 @@ class Entry_blob(Entry): self.external and (self.optional or self.section.GetAllowMissing())) # Allow the file to be missing if not self._pathname: + if not fake_size and self.assume_size: + fake_size = self.assume_size self._pathname, faked = self.check_fake_fname(self._filename, fake_size) self.missing = True if not faked: - self.SetContents(b'') + content_size = 0 + if self.assume_size: # Ensure we get test coverage on next line + content_size = self.assume_size + self.SetContents(tools.get_bytes(0, content_size)) return True
self.ReadBlobContents() diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 8a44bc051b3..bd0a10ff885 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -7460,5 +7460,33 @@ fdt fdtmap Extract the devicetree blob from the fdtmap with self.assertRaises(ValueError) as e: self._DoReadFile('323_capsule_accept_revert_missing.dts')
+ def test_assume_size(self): + """Test handling of the assume-size property for external blob""" + with self.assertRaises(ValueError) as e: + self._DoTestFile('326_assume_size.dts', allow_missing=True, + allow_fake_blobs=True) + self.assertIn("contents size 0xa (10) exceeds section size 0x9 (9)", + str(e.exception)) + + def test_assume_size_ok(self): + """Test handling of the assume-size where it fits OK""" + with test_util.capture_sys_output() as (stdout, stderr): + self._DoTestFile('327_assume_size_ok.dts', allow_missing=True, + allow_fake_blobs=True) + err = stderr.getvalue() + self.assertRegex( + err, + "Image '.*' has faked external blobs and is non-functional: .*") + + def test_assume_size_no_fake(self): + """Test handling of the assume-size where it fits OK""" + with test_util.capture_sys_output() as (stdout, stderr): + self._DoTestFile('327_assume_size_ok.dts', allow_missing=True) + err = stderr.getvalue() + self.assertRegex( + err, + "Image '.*' is missing external blobs and is non-functional: .*") + + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/326_assume_size.dts b/tools/binman/test/326_assume_size.dts new file mode 100644 index 00000000000..4c5f8b418d8 --- /dev/null +++ b/tools/binman/test/326_assume_size.dts @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <9>; + blob-ext { + filename = "assume_blob"; + assume-size = <10>; + }; + }; +}; diff --git a/tools/binman/test/327_assume_size_ok.dts b/tools/binman/test/327_assume_size_ok.dts new file mode 100644 index 00000000000..00ed726f872 --- /dev/null +++ b/tools/binman/test/327_assume_size_ok.dts @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <10>; + blob-ext { + filename = "assume_blob"; + assume-size = <10>; + }; + }; +};
This cannot ever go at offset 0 since the descriptor is there. Use a better offset for the ME, as used by link and coral, for example.
This matters when we start using assumed sizes for missing blobs.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
tools/binman/etype/intel_descriptor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/binman/etype/intel_descriptor.py b/tools/binman/etype/intel_descriptor.py index 7fe88a9ec1a..3ce967fe81a 100644 --- a/tools/binman/etype/intel_descriptor.py +++ b/tools/binman/etype/intel_descriptor.py @@ -59,7 +59,7 @@ class Entry_intel_descriptor(Entry_blob_ext): if self.missing: # Return zero offsets so that these entries get placed somewhere if self.HasSibling('intel-me'): - info['intel-me'] = [0, None] + info['intel-me'] = [0x1000, None] return info offset = self.data.find(FD_SIGNATURE) if offset == -1:
Tools should have an option to obtain the version, so add this to the mkeficapsule tool.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
doc/mkeficapsule.1 | 4 ++++ tools/mkeficapsule.c | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 index c4c2057d5c7..c3d0f21488a 100644 --- a/doc/mkeficapsule.1 +++ b/doc/mkeficapsule.1 @@ -87,6 +87,10 @@ Generate a firmware revert empty capsule .BI "-o\fR,\fB --capoemflag " Capsule OEM flag, value between 0x0000 to 0xffff
+.TP +.BR -V ", " --version +Print version information and exit. + .TP .BR -h ", " --help Print a help message diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 6a261ff549d..c112ae2de8d 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -21,6 +21,8 @@ #include <gnutls/pkcs7.h> #include <gnutls/abstract.h>
+#include <version.h> + #include "eficapsule.h"
static const char *tool_name = "mkeficapsule"; @@ -28,7 +30,7 @@ static const char *tool_name = "mkeficapsule"; efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
-static const char *opts_short = "g:i:I:v:p:c:m:o:dhARD"; +static const char *opts_short = "g:i:I:v:p:c:m:o:dhARDV";
enum { CAPSULE_NORMAL_BLOB = 0, @@ -70,6 +72,7 @@ static void print_usage(void) "\t-R, --fw-revert firmware revert capsule, takes no GUID, no image blob\n" "\t-o, --capoemflag Capsule OEM Flag, an integer between 0x0000 and 0xffff\n" "\t-D, --dump-capsule dump the contents of the capsule headers\n" + "\t-V, --version show version number\n" "\t-h, --help print a help message\n", tool_name); } @@ -969,6 +972,9 @@ int main(int argc, char **argv) case 'D': capsule_dump = true; break; + case 'V': + printf("mkeficapsule version %s\n", PLAIN_VERSION); + exit(EXIT_SUCCESS); default: print_usage(); exit(EXIT_SUCCESS);
On Fri, 21 Jun 2024 at 02:06, Simon Glass sjg@chromium.org wrote:
Tools should have an option to obtain the version, so add this to the mkeficapsule tool.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
doc/mkeficapsule.1 | 4 ++++ tools/mkeficapsule.c | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 index c4c2057d5c7..c3d0f21488a 100644 --- a/doc/mkeficapsule.1 +++ b/doc/mkeficapsule.1 @@ -87,6 +87,10 @@ Generate a firmware revert empty capsule .BI "-o\fR,\fB --capoemflag " Capsule OEM flag, value between 0x0000 to 0xffff
+.TP +.BR -V ", " --version +Print version information and exit.
.TP .BR -h ", " --help Print a help message diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 6a261ff549d..c112ae2de8d 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -21,6 +21,8 @@ #include <gnutls/pkcs7.h> #include <gnutls/abstract.h>
+#include <version.h>
#include "eficapsule.h"
static const char *tool_name = "mkeficapsule"; @@ -28,7 +30,7 @@ static const char *tool_name = "mkeficapsule"; efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
-static const char *opts_short = "g:i:I:v:p:c:m:o:dhARD"; +static const char *opts_short = "g:i:I:v:p:c:m:o:dhARDV";
enum { CAPSULE_NORMAL_BLOB = 0, @@ -70,6 +72,7 @@ static void print_usage(void) "\t-R, --fw-revert firmware revert capsule, takes no GUID, no image blob\n" "\t-o, --capoemflag Capsule OEM Flag, an integer between 0x0000 and 0xffff\n" "\t-D, --dump-capsule dump the contents of the capsule headers\n"
"\t-V, --version show version number\n" "\t-h, --help print a help message\n", tool_name);} @@ -969,6 +972,9 @@ int main(int argc, char **argv) case 'D': capsule_dump = true; break;
case 'V':printf("mkeficapsule version %s\n", PLAIN_VERSION);exit(EXIT_SUCCESS); default: print_usage(); exit(EXIT_SUCCESS);-- 2.34.1
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Now that this tool has a version number, collect it.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
tools/binman/btool/mkeficapsule.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py index ef1da638df1..f7e5a886849 100644 --- a/tools/binman/btool/mkeficapsule.py +++ b/tools/binman/btool/mkeficapsule.py @@ -33,7 +33,8 @@ class Bintoolmkeficapsule(bintool.Bintool): commandline, or through a config file. """ def __init__(self, name): - super().__init__(name, 'mkeficapsule tool for generating capsules') + super().__init__(name, 'mkeficapsule tool for generating capsules', + r'mkeficapsule version (.*)')
def generate_capsule(self, image_index, image_guid, hardware_instance, payload, output_fname, priv_key, pub_key,
Tools cannot be assumed to be present. Add a check for this with the mkeficpasule tool.
Signed-off-by: Simon Glass sjg@chromium.org Fixes: b617611b27a ("binman: capsule: Add support for generating...") ---
(no changes since v1)
tools/binman/etype/efi_capsule.py | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py index e3203717822..47da5da324b 100644 --- a/tools/binman/etype/efi_capsule.py +++ b/tools/binman/etype/efi_capsule.py @@ -150,6 +150,10 @@ class Entry_efi_capsule(Entry_section): if ret is not None: os.remove(payload) return tools.read_file(capsule_fname) + else: + # Bintool is missing; just use the input data as the output + self.record_missing_bintool(self.mkeficapsule) + return data
def AddBintools(self, btools): self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
The tool must return an error code when invalid arguments are provided, otherwise binman has no way of knowing that anything went wrong.
Correct this.
Signed-off-by: Simon Glass sjg@chromium.org Fixes: fab430be2f4 ("tools: add mkeficapsule command for UEFI...") ---
(no changes since v1)
tools/mkeficapsule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index c112ae2de8d..f28008a0829 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -977,7 +977,7 @@ int main(int argc, char **argv) exit(EXIT_SUCCESS); default: print_usage(); - exit(EXIT_SUCCESS); + exit(EXIT_FAILURE); } }
There is no need to remove input files. It makes it harder to diagnose failures. Keep the payload file.
There is no test for this condition, but one could be added.
Signed-off-by: Simon Glass sjg@chromium.org Fixes: b617611b27a ("binman: capsule: Add support for generating...") ---
(no changes since v1)
tools/binman/etype/efi_capsule.py | 1 - 1 file changed, 1 deletion(-)
diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py index 47da5da324b..03e55cbc4d9 100644 --- a/tools/binman/etype/efi_capsule.py +++ b/tools/binman/etype/efi_capsule.py @@ -148,7 +148,6 @@ class Entry_efi_capsule(Entry_section): self.fw_version, self.oem_flags) if ret is not None: - os.remove(payload) return tools.read_file(capsule_fname) else: # Bintool is missing; just use the input data as the output
On Fri, 21 Jun 2024 at 04:36, Simon Glass sjg@chromium.org wrote:
There is no need to remove input files. It makes it harder to diagnose failures. Keep the payload file.
There is no test for this condition, but one could be added.
Signed-off-by: Simon Glass sjg@chromium.org Fixes: b617611b27a ("binman: capsule: Add support for generating...")
Acked-by: Sughosh Ganu sughosh.ganu@linaro.org
Although I don't think this warrants a Fixes tag. It is not fixing any issue as such.
-sughosh
(no changes since v1)
tools/binman/etype/efi_capsule.py | 1 - 1 file changed, 1 deletion(-)
diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py index 47da5da324b..03e55cbc4d9 100644 --- a/tools/binman/etype/efi_capsule.py +++ b/tools/binman/etype/efi_capsule.py @@ -148,7 +148,6 @@ class Entry_efi_capsule(Entry_section): self.fw_version, self.oem_flags) if ret is not None:
os.remove(payload) return tools.read_file(capsule_fname) else: # Bintool is missing; just use the input data as the output-- 2.34.1
Hi Sughosh,
On Fri, 21 Jun 2024 at 00:14, Sughosh Ganu sughosh.ganu@linaro.org wrote:
On Fri, 21 Jun 2024 at 04:36, Simon Glass sjg@chromium.org wrote:
There is no need to remove input files. It makes it harder to diagnose failures. Keep the payload file.
There is no test for this condition, but one could be added.
Signed-off-by: Simon Glass sjg@chromium.org Fixes: b617611b27a ("binman: capsule: Add support for generating...")
Acked-by: Sughosh Ganu sughosh.ganu@linaro.org
Although I don't think this warrants a Fixes tag. It is not fixing any issue as such.
I'm OK to drop it if you like. Binman is designed to keep its intermittent files in the output dir. We have talked about having a separate dir for 'intermediate' files to avoid cluttering the output dir, but have not done this yet.
Regards, Simon
-sughosh
(no changes since v1)
tools/binman/etype/efi_capsule.py | 1 - 1 file changed, 1 deletion(-)
diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py index 47da5da324b..03e55cbc4d9 100644 --- a/tools/binman/etype/efi_capsule.py +++ b/tools/binman/etype/efi_capsule.py @@ -148,7 +148,6 @@ class Entry_efi_capsule(Entry_section): self.fw_version, self.oem_flags) if ret is not None:
os.remove(payload) return tools.read_file(capsule_fname) else: # Bintool is missing; just use the input data as the output-- 2.34.1
Add assumed sizes so that Binman can check that the U-Boot binaries do not grow too large.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
arch/x86/dts/u-boot.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/x86/dts/u-boot.dtsi b/arch/x86/dts/u-boot.dtsi index e0de3318091..fdd28979e0b 100644 --- a/arch/x86/dts/u-boot.dtsi +++ b/arch/x86/dts/u-boot.dtsi @@ -24,9 +24,11 @@ #ifdef CONFIG_HAVE_INTEL_ME intel-descriptor { filename = CONFIG_FLASH_DESCRIPTOR_FILE; + assume-size = <0x1000>; }; intel-me { filename = CONFIG_INTEL_ME_FILE; + assume-size = <0x1ff000>; }; #endif #ifdef CONFIG_TPL @@ -87,6 +89,7 @@ #ifdef CONFIG_HAVE_MRC intel-mrc { offset = <CFG_X86_MRC_ADDR>; + assume-size = <0x2fc94>; }; #endif #ifdef CONFIG_FSP_VERSION1 @@ -98,6 +101,7 @@ #ifdef CONFIG_FSP_VERSION2 intel-descriptor { filename = CONFIG_FLASH_DESCRIPTOR_FILE; + assume-size = <4096>; }; intel-ifwi { filename = CONFIG_IFWI_INPUT_FILE; @@ -139,6 +143,7 @@ intel-vga { filename = CONFIG_VGA_BIOS_FILE; offset = <CONFIG_VGA_BIOS_ADDR>; + assume-size = <0x10000>; }; #endif #ifdef CONFIG_HAVE_VBT
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 ---
(no changes since v1)
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. That board does use the TPM, but not with measured boot, nor EFI.
Since EFI_TCG2_PROTOCOL already selects these options, we just need to add them to MEASURED_BOOT as well.
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 Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - Put the conditions under EFI_TCG2_PROTOCOL - Consider MEASURED_BOOT too
boot/Kconfig | 4 ++++ lib/Kconfig | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index 6f3096c15a6..b061891e109 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT config MEASURED_BOOT bool "Measure boot images and configuration when booting without EFI" depends on HASH && TPM_V2 + select SHA1 + select SHA256 + select SHA384 + select SHA512 help This option enables measurement of the boot process when booting without UEFI . Measurement involves creating cryptographic hashes diff --git a/lib/Kconfig b/lib/Kconfig index 189e6eb31aa..568892fce44 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -438,10 +438,6 @@ config TPM bool "Trusted Platform Module (TPM) Support" depends on DM imply DM_RNG - select SHA1 - select SHA256 - select SHA384 - select SHA512 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 Simon,
On Fri, 21 Jun 2024 at 02:06, Simon Glass sjg@chromium.org 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. That board does use the TPM, but not with measured boot, nor EFI.
Since EFI_TCG2_PROTOCOL already selects these options, we just need to add them to MEASURED_BOOT as well.
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 Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
There was a discussion in the previous version, bout enabling these on CMD_TPM as they are required.
Thanks /Ilias
Changes in v2:
- Put the conditions under EFI_TCG2_PROTOCOL
- Consider MEASURED_BOOT too
boot/Kconfig | 4 ++++ lib/Kconfig | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index 6f3096c15a6..b061891e109 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT config MEASURED_BOOT bool "Measure boot images and configuration when booting without EFI" depends on HASH && TPM_V2
select SHA1select SHA256select SHA384select SHA512 help This option enables measurement of the boot process when booting without UEFI . Measurement involves creating cryptographic hashesdiff --git a/lib/Kconfig b/lib/Kconfig index 189e6eb31aa..568892fce44 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -438,10 +438,6 @@ config TPM bool "Trusted Platform Module (TPM) Support" depends on DM imply DM_RNG
select SHA1select SHA256select SHA384select SHA512 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-- 2.34.1
On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 02:06, Simon Glass sjg@chromium.org 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. That board does use the TPM, but not with measured boot, nor EFI.
Since EFI_TCG2_PROTOCOL already selects these options, we just need to add them to MEASURED_BOOT as well.
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 Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
There was a discussion in the previous version, bout enabling these on CMD_TPM as they are required.
Or switch this to an imply instead so you can disable it ?
Regards /Ilias
Thanks /Ilias
Hi Ilias,
On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 02:06, Simon Glass sjg@chromium.org 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. That board does use the TPM, but not with measured boot, nor EFI.
Since EFI_TCG2_PROTOCOL already selects these options, we just need to add them to MEASURED_BOOT as well.
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 Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
There was a discussion in the previous version, bout enabling these on CMD_TPM as they are required.
For reference [1]
Or switch this to an imply instead so you can disable it ?
The thing is, we need to be able to easily disable unwanted features. My reading of that thread is that U-Boot doesn't use SHA384/512 from the 'tpm2 pcr_extend' command today. Even if it did, I would think it better to allow the supported algorithms to be controlled. At worst, I would expect a separate option to disable that subcommand which would remove the implies.
Some tpm code recently added puts a table and some functions in tpm-v2.h - could someone fix that? tpm-common.c would be a better place. Also we already have a list of hash algorithms in U-Boot (common/hash.c) so should use that, and respect the fact that certain algos may not be supported.
If there is a requirement that (for a certain situation / protocol) we need to support everything, then let's have a Kconfig for that. It is what my patch was trying to work around, perhaps. For boards which don't care about extending TPM in this way, or don't use the more expensive algorithms, it avoids lots of code bloat.
One of the principles of U-Boot is to allow configurability in these situations. It shouldn't be too hard to have the best of both worlds (supports all reasonable cases by default / allow unwanted cases to be dropped).
So I'll await your reply on the above before figuring out where to go with this. For this release I'd just like to get the board working, but I understand that the TPM stuff is very-much in flux, perhaps in -next.
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20240610145920.3302001-3-sj...
Hi Simon,
On Fri, 21 Jun 2024 at 17:57, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 02:06, Simon Glass sjg@chromium.org 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. That board does use the TPM, but not with measured boot, nor EFI.
Since EFI_TCG2_PROTOCOL already selects these options, we just need to add them to MEASURED_BOOT as well.
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 Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
There was a discussion in the previous version, bout enabling these on CMD_TPM as they are required.
For reference [1]
Or switch this to an imply instead so you can disable it ?
The thing is, we need to be able to easily disable unwanted features. My reading of that thread is that U-Boot doesn't use SHA384/512 from the 'tpm2 pcr_extend' command today.
It does, it was mentioned on the thread you referenced. commit 89aa8463cdf39 ("tpm-v2: allow algorithm name to be configured for pcr_read and pcr_extend")
Even if it did, I would think it better to allow the supported algorithms to be controlled. At worst, I would expect a separate option to disable that subcommand which would remove the implies.
To control the algos you need to compile, you need to control the algorithms the TPM supports. Otherwise, you are leaving security holes. We don't configure the supported PCR banks today iirc.
If we need the fix for those platforms for the release so badly, why don't we just disable the TPM? It's going to be useless and potentially dangerous without SHA support. Any idea why those platforms use it for?
Some tpm code recently added puts a table and some functions in tpm-v2.h - could someone fix that? tpm-common.c would be a better place.
Do you mean commit 954b95e77ef0a8? If that's the one, I don't mind moving it there. Those values though are described in [0], which is primarily for v2.0. Nothing prevents you from using it in v1.2 but all of the values apart from SHA1 are useless. Also, we don't support measured boot with 1.2 anyway so I didn't see the point back then.
Also we already have a list of hash algorithms in U-Boot (common/hash.c) so should use that, >and respect the fact that certain algos may not be supported.
That seems tailored for a very specific reason. Do you want to fill it in with EFI internal definitions? We could adjust some lookup functions which seems common.
If there is a requirement that (for a certain situation / protocol) we need to support everything, then let's have a Kconfig for that.
We do. The difference here is that the *TPM at runtime* dictates what you need. So you can't blindly disable stuff.
It is what my patch was trying to work around, perhaps. For boards which don't care about extending TPM in this way, or don't use the more expensive algorithms, it avoids lots of code bloat.
Ok, I disagree here. That's potentially dangerous. Because someone, might need the TPM, easily misconfigure it, and end up with broken measurements.
One of the principles of U-Boot is to allow configurability in these situations. It shouldn't be too hard to have the best of both worlds (supports all reasonable cases by default / allow unwanted cases to be dropped).
It's not. We can have the TPM configure the supported algorithms once it starts. IMHO that's the proper way to do it. Everything else is just papering over the problem -- which even worse no one will care to fix.
So I'll await your reply on the above before figuring out where to go with this. For this release I'd just like to get the board working, but I understand that the TPM stuff is very-much in flux, perhaps in -next.
This is a very complex problem. Even if we configure the TPMs banks based on the supported algoriths, a previous stage loader might have enabled more. So you end up breaking that in that case...
Let me see if I can configure the TPM on boot....
Cheers /Ilias
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20240610145920.3302001-3-sj...
[0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specificat... -- search for EFI_TCG2_BOOT_HASH_ALG_SHA512
Hi Ilias,
On Fri, 21 Jun 2024 at 09:52, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 17:57, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 21 Jun 2024 at 02:06, Simon Glass sjg@chromium.org 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. That board does use the TPM, but not with measured boot, nor EFI.
Since EFI_TCG2_PROTOCOL already selects these options, we just need to add them to MEASURED_BOOT as well.
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 Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
There was a discussion in the previous version, bout enabling these on CMD_TPM as they are required.
For reference [1]
Or switch this to an imply instead so you can disable it ?
The thing is, we need to be able to easily disable unwanted features. My reading of that thread is that U-Boot doesn't use SHA384/512 from the 'tpm2 pcr_extend' command today.
It does, it was mentioned on the thread you referenced. commit 89aa8463cdf39 ("tpm-v2: allow algorithm name to be configured for pcr_read and pcr_extend")
Even if it did, I would think it better to allow the supported algorithms to be controlled. At worst, I would expect a separate option to disable that subcommand which would remove the implies.
To control the algos you need to compile, you need to control the algorithms the TPM supports. Otherwise, you are leaving security holes. We don't configure the supported PCR banks today iirc.
If we need the fix for those platforms for the release so badly, why don't we just disable the TPM? It's going to be useless and potentially dangerous without SHA support. Any idea why those platforms use it for?
Some tpm code recently added puts a table and some functions in tpm-v2.h - could someone fix that? tpm-common.c would be a better place.
Do you mean commit 954b95e77ef0a8? If that's the one, I don't mind moving it there. Those values though are described in [0], which is primarily for v2.0. Nothing prevents you from using it in v1.2 but all of the values apart from SHA1 are useless. Also, we don't support measured boot with 1.2 anyway so I didn't see the point back then.
Also we already have a list of hash algorithms in U-Boot (common/hash.c) so should use that, >and respect the fact that certain algos may not be supported.
That seems tailored for a very specific reason. Do you want to fill it in with EFI internal definitions? We could adjust some lookup functions which seems common.
If there is a requirement that (for a certain situation / protocol) we need to support everything, then let's have a Kconfig for that.
We do. The difference here is that the *TPM at runtime* dictates what you need. So you can't blindly disable stuff.
It is what my patch was trying to work around, perhaps. For boards which don't care about extending TPM in this way, or don't use the more expensive algorithms, it avoids lots of code bloat.
Ok, I disagree here. That's potentially dangerous. Because someone, might need the TPM, easily misconfigure it, and end up with broken measurements.
One of the principles of U-Boot is to allow configurability in these situations. It shouldn't be too hard to have the best of both worlds (supports all reasonable cases by default / allow unwanted cases to be dropped).
It's not. We can have the TPM configure the supported algorithms once it starts. IMHO that's the proper way to do it. Everything else is just papering over the problem -- which even worse no one will care to fix.
So I'll await your reply on the above before figuring out where to go with this. For this release I'd just like to get the board working, but I understand that the TPM stuff is very-much in flux, perhaps in -next.
This is a very complex problem. Even if we configure the TPMs banks based on the supported algoriths, a previous stage loader might have enabled more. So you end up breaking that in that case...
Let me see if I can configure the TPM on boot....
I just don't understand any of this very well.
My request is, please can you find a way to allow the TPM to be enabled for a board that doesn't use the pcr_extend subcommand with SHA384/512? I just want to get that 5KB back for this board. I'm happy with the defaults being a certain way, just not wanting to require this code for boards which don't need it.
Regards, Simon
Cheers /Ilias
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20240610145920.3302001-3-sj...
[0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specificat... -- search for EFI_TCG2_BOOT_HASH_ALG_SHA512
Add better logging for power init so that CONFIG_LOG_ERROR_RETURN can be enabled.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Quentin Schulz quentin.schulz@cherry.de ---
(no changes since v2)
Changes in v2: - Remove the superfluous if() and drop the debug() as well
board/google/veyron/veyron.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c index 32dbcdc4d10..6d4c9debdee 100644 --- a/board/google/veyron/veyron.c +++ b/board/google/veyron/veyron.c @@ -29,44 +29,38 @@ 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 +75,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 /*
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 Reviewed-by: Jonas Karlman jonas@kwiboo.se Reviewed-by: Quentin Schulz quentin.schulz@cherry.de
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
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 ---
Changes in v3: - Use BLOBLIST instead of OF_BLOBLIST
Changes in v2: - Use 'phase' instead of 'stage'
lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b2c59ab3818..e16d1819449 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 phase passed a bloblist, + * not whether this phase creates one. + */ + if (CONFIG_IS_ENABLED(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);
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 ---
(no changes since v1)
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); }
At present gd->ram_size is 0 in SPL, meaning that it is not possible to enable the cache. Correct this by always populating the RAM size correctly.
This increases code size by about 500 bytes in SPL, since it must call the rather large rockchip_sdram_size() function.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Cut the patch down to bare bones
Changes in v2: - Add new patch to correct memory size in SPL
drivers/ram/rockchip/sdram_rk3399.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c index 02cc4a38cf0..3c4e20f4e80 100644 --- a/drivers/ram/rockchip/sdram_rk3399.c +++ b/drivers/ram/rockchip/sdram_rk3399.c @@ -3142,19 +3142,19 @@ static int rk3399_dmc_init(struct udevice *dev)
static int rk3399_dmc_probe(struct udevice *dev) { + struct dram_info *priv = dev_get_priv(dev); + #if defined(CONFIG_TPL_BUILD) || \ (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) if (rk3399_dmc_init(dev)) return 0; -#else - struct dram_info *priv = dev_get_priv(dev); - +#endif priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF); debug("%s: pmugrf = %p\n", __func__, priv->pmugrf); priv->info.base = CFG_SYS_SDRAM_BASE; priv->info.size = rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2); -#endif + return 0; }
The code here is confusing due to large blocks which are #ifdefed out. Add a function phase_sdram_init() which returns whether SDRAM init should happen in the current phase, using that as needed to control the code flow.
This increases code size by about 500 bytes in SPL when the cache is on, since it must call the rather large rockchip_sdram_size() function.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Split out the refactoring into a separate patch - Drop the non-dcache optimisation, since the cache should normally be on
drivers/ram/rockchip/sdram_rk3399.c | 47 ++++++++++++++++------------- 1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c index 3c4e20f4e80..2f37dd712e7 100644 --- a/drivers/ram/rockchip/sdram_rk3399.c +++ b/drivers/ram/rockchip/sdram_rk3399.c @@ -13,6 +13,7 @@ #include <log.h> #include <ram.h> #include <regmap.h> +#include <spl.h> #include <syscon.h> #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/cru.h> @@ -63,8 +64,6 @@ struct chan_info { };
struct dram_info { -#if defined(CONFIG_TPL_BUILD) || \ - (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) u32 pwrup_srefresh_exit[2]; struct chan_info chan[2]; struct clk ddr_clk; @@ -75,7 +74,6 @@ struct dram_info { struct rk3399_pmusgrf_regs *pmusgrf; struct rk3399_ddr_cic_regs *cic; const struct sdram_rk3399_ops *ops; -#endif struct ram_info info; struct rk3399_pmugrf_regs *pmugrf; }; @@ -92,9 +90,6 @@ struct sdram_rk3399_ops { struct rk3399_sdram_params *params); };
-#if defined(CONFIG_TPL_BUILD) || \ - (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) - struct rockchip_dmc_plat { #if CONFIG_IS_ENABLED(OF_PLATDATA) struct dtd_rockchip_rk3399_dmc dtplat; @@ -191,6 +186,17 @@ struct io_setting { }, };
+/** + * phase_sdram_init() - Check if this is the phase where SDRAM init happens + * + * Returns: true to do SDRAM init in this phase, false to not + */ +static bool phase_sdram_init(void) +{ + return spl_phase() == PHASE_TPL || + (!IS_ENABLED(CONFIG_TPL) && !spl_in_proper()); +} + static struct io_setting * lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5) { @@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev) struct rockchip_dmc_plat *plat = dev_get_plat(dev); int ret;
- if (!CONFIG_IS_ENABLED(OF_REAL)) + if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init()) return 0;
ret = dev_read_u32_array(dev, "rockchip,sdram-params", @@ -3138,22 +3144,24 @@ static int rk3399_dmc_init(struct udevice *dev)
return 0; } -#endif
static int rk3399_dmc_probe(struct udevice *dev) { struct dram_info *priv = dev_get_priv(dev);
-#if defined(CONFIG_TPL_BUILD) || \ - (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) - if (rk3399_dmc_init(dev)) - return 0; -#endif - priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF); - debug("%s: pmugrf = %p\n", __func__, priv->pmugrf); - priv->info.base = CFG_SYS_SDRAM_BASE; - priv->info.size = - rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2); + if (phase_sdram_init()) { + if (rk3399_dmc_init(dev)) + return 0; + } else { + priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF); + debug("%s: pmugrf = %p\n", __func__, priv->pmugrf); + } + + if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) { + priv->info.base = CFG_SYS_SDRAM_BASE; + priv->info.size = + rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2); + }
return 0; } @@ -3181,10 +3189,7 @@ U_BOOT_DRIVER(dmc_rk3399) = { .id = UCLASS_RAM, .of_match = rk3399_dmc_ids, .ops = &rk3399_dmc_ops, -#if defined(CONFIG_TPL_BUILD) || \ - (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) .of_to_plat = rk3399_dmc_of_to_plat, -#endif .probe = rk3399_dmc_probe, .priv_auto = sizeof(struct dram_info), #if defined(CONFIG_TPL_BUILD) || \
Hi Simon,
On 2024-06-21 01:06, Simon Glass wrote:
The code here is confusing due to large blocks which are #ifdefed out. Add a function phase_sdram_init() which returns whether SDRAM init should happen in the current phase, using that as needed to control the code flow.
This increases code size by about 500 bytes in SPL when the cache is on, since it must call the rather large rockchip_sdram_size() function.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
- Split out the refactoring into a separate patch
- Drop the non-dcache optimisation, since the cache should normally be on
drivers/ram/rockchip/sdram_rk3399.c | 47 ++++++++++++++++------------- 1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c index 3c4e20f4e80..2f37dd712e7 100644 --- a/drivers/ram/rockchip/sdram_rk3399.c +++ b/drivers/ram/rockchip/sdram_rk3399.c @@ -13,6 +13,7 @@ #include <log.h> #include <ram.h> #include <regmap.h> +#include <spl.h> #include <syscon.h> #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/cru.h> @@ -63,8 +64,6 @@ struct chan_info { };
struct dram_info { -#if defined(CONFIG_TPL_BUILD) || \
- (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) u32 pwrup_srefresh_exit[2]; struct chan_info chan[2]; struct clk ddr_clk;
@@ -75,7 +74,6 @@ struct dram_info { struct rk3399_pmusgrf_regs *pmusgrf; struct rk3399_ddr_cic_regs *cic; const struct sdram_rk3399_ops *ops; -#endif struct ram_info info; struct rk3399_pmugrf_regs *pmugrf; }; @@ -92,9 +90,6 @@ struct sdram_rk3399_ops { struct rk3399_sdram_params *params); };
-#if defined(CONFIG_TPL_BUILD) || \
- (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
struct rockchip_dmc_plat { #if CONFIG_IS_ENABLED(OF_PLATDATA) struct dtd_rockchip_rk3399_dmc dtplat; @@ -191,6 +186,17 @@ struct io_setting { }, };
+/**
- phase_sdram_init() - Check if this is the phase where SDRAM init happens
- Returns: true to do SDRAM init in this phase, false to not
- */
+static bool phase_sdram_init(void) +{
- return spl_phase() == PHASE_TPL ||
(!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());+}
static struct io_setting * lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5) { @@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev) struct rockchip_dmc_plat *plat = dev_get_plat(dev); int ret;
- if (!CONFIG_IS_ENABLED(OF_REAL))
if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init()) return 0;
ret = dev_read_u32_array(dev, "rockchip,sdram-params",
@@ -3138,22 +3144,24 @@ static int rk3399_dmc_init(struct udevice *dev)
return 0; } -#endif
static int rk3399_dmc_probe(struct udevice *dev) { struct dram_info *priv = dev_get_priv(dev);
-#if defined(CONFIG_TPL_BUILD) || \
- (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))
- if (rk3399_dmc_init(dev))
return 0;-#endif
- priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);
- debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);
- priv->info.base = CFG_SYS_SDRAM_BASE;
- priv->info.size =
rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
- if (phase_sdram_init()) {
if (rk3399_dmc_init(dev))return 0;- } else {
priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);- }
- if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {
This check should be dropped, this driver intent is to initialize dram in first phase (normally TPL), and report the size to any other phase.
The main need for phase_sdram_init() and disable of DCACHE can probably be avoided if bob/kevin can move to use separate TPL and SPL instead of doing both dram init and everything else in SPL.
My best guess is that enabling of caches in SPL cause issue for bob and kevin because they only use SPL not TPL+SPL like (if I am not mistaken) all other Rockchip arm64 targets.
Using SPL-only was not something I tested when caches was enabled in SPL.
Maybe bob/kevin can be changed to also use TPL+SPL similar to all other RK3399 boards?
How U-Boot works on these chromebooks is still a mystery to me.
When coreboot is involved it would only load U-Boot proper and SPL or TPL+SPL would never be involved at all?
If I understand correctly SPL is only involved for bare metal boot, if this is the case then using TPL + back-to-brom to load SPL should probably work fine?, and would align all RK3399 boards to work in a similar way and reduce the need for special care/handling in the code.
Regards, Jonas
priv->info.base = CFG_SYS_SDRAM_BASE;priv->info.size =rockchip_sdram_size((ulong)&priv->pmugrf->os_reg2);}
return 0;
} @@ -3181,10 +3189,7 @@ U_BOOT_DRIVER(dmc_rk3399) = { .id = UCLASS_RAM, .of_match = rk3399_dmc_ids, .ops = &rk3399_dmc_ops, -#if defined(CONFIG_TPL_BUILD) || \
- (!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) .of_to_plat = rk3399_dmc_of_to_plat,
-#endif .probe = rk3399_dmc_probe, .priv_auto = sizeof(struct dram_info), #if defined(CONFIG_TPL_BUILD) || \
Hi Jonas,
On Fri, 21 Jun 2024 at 00:45, Jonas Karlman jonas@kwiboo.se wrote:
Hi Simon,
On 2024-06-21 01:06, Simon Glass wrote:
The code here is confusing due to large blocks which are #ifdefed out. Add a function phase_sdram_init() which returns whether SDRAM init should happen in the current phase, using that as needed to control the code flow.
This increases code size by about 500 bytes in SPL when the cache is on, since it must call the rather large rockchip_sdram_size() function.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
- Split out the refactoring into a separate patch
- Drop the non-dcache optimisation, since the cache should normally be on
drivers/ram/rockchip/sdram_rk3399.c | 47 ++++++++++++++++------------- 1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c index 3c4e20f4e80..2f37dd712e7 100644 --- a/drivers/ram/rockchip/sdram_rk3399.c +++ b/drivers/ram/rockchip/sdram_rk3399.c @@ -13,6 +13,7 @@ #include <log.h> #include <ram.h> #include <regmap.h> +#include <spl.h> #include <syscon.h> #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/cru.h> @@ -63,8 +64,6 @@ struct chan_info { };
struct dram_info { -#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) u32 pwrup_srefresh_exit[2]; struct chan_info chan[2]; struct clk ddr_clk;@@ -75,7 +74,6 @@ struct dram_info { struct rk3399_pmusgrf_regs *pmusgrf; struct rk3399_ddr_cic_regs *cic; const struct sdram_rk3399_ops *ops; -#endif struct ram_info info; struct rk3399_pmugrf_regs *pmugrf; }; @@ -92,9 +90,6 @@ struct sdram_rk3399_ops { struct rk3399_sdram_params *params); };
-#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))struct rockchip_dmc_plat { #if CONFIG_IS_ENABLED(OF_PLATDATA) struct dtd_rockchip_rk3399_dmc dtplat; @@ -191,6 +186,17 @@ struct io_setting { }, };
+/**
- phase_sdram_init() - Check if this is the phase where SDRAM init happens
- Returns: true to do SDRAM init in this phase, false to not
- */
+static bool phase_sdram_init(void) +{
return spl_phase() == PHASE_TPL ||(!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());+}
static struct io_setting * lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5) { @@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev) struct rockchip_dmc_plat *plat = dev_get_plat(dev); int ret;
if (!CONFIG_IS_ENABLED(OF_REAL))
if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init()) return 0; ret = dev_read_u32_array(dev, "rockchip,sdram-params",@@ -3138,22 +3144,24 @@ static int rk3399_dmc_init(struct udevice *dev)
return 0;} -#endif
static int rk3399_dmc_probe(struct udevice *dev) { struct dram_info *priv = dev_get_priv(dev);
-#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))if (rk3399_dmc_init(dev))return 0;-#endif
priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);priv->info.base = CFG_SYS_SDRAM_BASE;priv->info.size =rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
if (phase_sdram_init()) {if (rk3399_dmc_init(dev))return 0;} else {priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);}if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {This check should be dropped, this driver intent is to initialize dram in first phase (normally TPL), and report the size to any other phase.
This whole patch can be dropped :-) Here I am just trying to avoid code-size growth when the cache is off. But yes, it is not needed.
The main need for phase_sdram_init() and disable of DCACHE can probably be avoided if bob/kevin can move to use separate TPL and SPL instead of doing both dram init and everything else in SPL.
My best guess is that enabling of caches in SPL cause issue for bob and kevin because they only use SPL not TPL+SPL like (if I am not mistaken) all other Rockchip arm64 targets.
Using SPL-only was not something I tested when caches was enabled in SPL.
Maybe bob/kevin can be changed to also use TPL+SPL similar to all other RK3399 boards?
How U-Boot works on these chromebooks is still a mystery to me.
When coreboot is involved it would only load U-Boot proper and SPL or TPL+SPL would never be involved at all?
If I understand correctly SPL is only involved for bare metal boot, if this is the case then using TPL + back-to-brom to load SPL should probably work fine?, and would align all RK3399 boards to work in a similar way and reduce the need for special care/handling in the code.
These boards were the first rk3399 support we had in mainline, I believe. Everything else came later but conversions were not done for these existing boards.
I actually intensely dislike the back-to-brom stuff. If the ROM had an actual API (mmc_read(), etc.) then that would be fine. But just jumping back makes it hard to control what is going on.
It is interesting to hear that SPL-only is causing a problem. Do you have any inkling of what that might be? I could take a look at converting the boards to use TPL, but it does seem a bit sideways to the issue here. Is the bootrom doing some magic, perhaps? Is there decent documentation for the boot ROM these days?
Regards, Simon
Hi Simon,
On 2024/6/21 22:57, Simon Glass wrote:
Hi Jonas,
On Fri, 21 Jun 2024 at 00:45, Jonas Karlman jonas@kwiboo.se wrote:
Hi Simon,
On 2024-06-21 01:06, Simon Glass wrote:
The code here is confusing due to large blocks which are #ifdefed out. Add a function phase_sdram_init() which returns whether SDRAM init should happen in the current phase, using that as needed to control the code flow.
This increases code size by about 500 bytes in SPL when the cache is on, since it must call the rather large rockchip_sdram_size() function.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
Split out the refactoring into a separate patch
Drop the non-dcache optimisation, since the cache should normally be on
drivers/ram/rockchip/sdram_rk3399.c | 47 ++++++++++++++++------------- 1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c index 3c4e20f4e80..2f37dd712e7 100644 --- a/drivers/ram/rockchip/sdram_rk3399.c +++ b/drivers/ram/rockchip/sdram_rk3399.c @@ -13,6 +13,7 @@ #include <log.h> #include <ram.h> #include <regmap.h> +#include <spl.h> #include <syscon.h> #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/cru.h> @@ -63,8 +64,6 @@ struct chan_info { };
struct dram_info { -#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) u32 pwrup_srefresh_exit[2]; struct chan_info chan[2]; struct clk ddr_clk;@@ -75,7 +74,6 @@ struct dram_info { struct rk3399_pmusgrf_regs *pmusgrf; struct rk3399_ddr_cic_regs *cic; const struct sdram_rk3399_ops *ops; -#endif struct ram_info info; struct rk3399_pmugrf_regs *pmugrf; }; @@ -92,9 +90,6 @@ struct sdram_rk3399_ops { struct rk3399_sdram_params *params); };
-#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))- struct rockchip_dmc_plat { #if CONFIG_IS_ENABLED(OF_PLATDATA) struct dtd_rockchip_rk3399_dmc dtplat;
@@ -191,6 +186,17 @@ struct io_setting { }, };
+/**
- phase_sdram_init() - Check if this is the phase where SDRAM init happens
- Returns: true to do SDRAM init in this phase, false to not
- */
+static bool phase_sdram_init(void) +{
return spl_phase() == PHASE_TPL ||(!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());+}
- static struct io_setting * lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5) {
@@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev) struct rockchip_dmc_plat *plat = dev_get_plat(dev); int ret;
if (!CONFIG_IS_ENABLED(OF_REAL))
if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init()) return 0; ret = dev_read_u32_array(dev, "rockchip,sdram-params",@@ -3138,22 +3144,24 @@ static int rk3399_dmc_init(struct udevice *dev)
return 0;} -#endif
static int rk3399_dmc_probe(struct udevice *dev) { struct dram_info *priv = dev_get_priv(dev);
-#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))if (rk3399_dmc_init(dev))return 0;-#endif
priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);priv->info.base = CFG_SYS_SDRAM_BASE;priv->info.size =rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
if (phase_sdram_init()) {if (rk3399_dmc_init(dev))return 0;} else {priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);}if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {This check should be dropped, this driver intent is to initialize dram in first phase (normally TPL), and report the size to any other phase.
This whole patch can be dropped :-) Here I am just trying to avoid code-size growth when the cache is off. But yes, it is not needed.
The main need for phase_sdram_init() and disable of DCACHE can probably be avoided if bob/kevin can move to use separate TPL and SPL instead of doing both dram init and everything else in SPL.
My best guess is that enabling of caches in SPL cause issue for bob and kevin because they only use SPL not TPL+SPL like (if I am not mistaken) all other Rockchip arm64 targets.
Using SPL-only was not something I tested when caches was enabled in SPL.
Maybe bob/kevin can be changed to also use TPL+SPL similar to all other RK3399 boards?
How U-Boot works on these chromebooks is still a mystery to me.
When coreboot is involved it would only load U-Boot proper and SPL or TPL+SPL would never be involved at all?
If I understand correctly SPL is only involved for bare metal boot, if this is the case then using TPL + back-to-brom to load SPL should probably work fine?, and would align all RK3399 boards to work in a similar way and reduce the need for special care/handling in the code.
These boards were the first rk3399 support we had in mainline, I believe. Everything else came later but conversions were not done for these existing boards.
I actually intensely dislike the back-to-brom stuff. If the ROM had an actual API (mmc_read(), etc.) then that would be fine. But just jumping back makes it hard to control what is going on.
The logic of BootRom is very simple and very clear: - Read image from boot medium and check the availability(format of rockchip IDB, which is packed by the mkimage rksd/rkspi); - Load TPL/dram init blob to the internal SRAM(size limit by the sram), and jump to the entry; - Back to bootRom with '0' return means the dram init success, bootRom will get next image for dram; - Read SPL image from boot medium to DDR start address(no size limit), and jump to the ddr entry; - The signature verify will be done for all the firmware load by bootRom if the secure boot feature is enabled; This model works for all Rockchip SoCs, although some SoCs have very limit SRAM size(only available for DRAM init) and some other SoCs like rk3399 have bigger SRAM size.
Thanks, - Kever
It is interesting to hear that SPL-only is causing a problem. Do you have any inkling of what that might be? I could take a look at converting the boards to use TPL, but it does seem a bit sideways to the issue here. Is the bootrom doing some magic, perhaps? Is there decent documentation for the boot ROM these days?
Regards, Simon
Hi Kever,
On Fri, 21 Jun 2024 at 21:49, Kever Yang kever.yang@rock-chips.com wrote:
Hi Simon,
On 2024/6/21 22:57, Simon Glass wrote:
Hi Jonas,
On Fri, 21 Jun 2024 at 00:45, Jonas Karlman jonas@kwiboo.se wrote:
Hi Simon,
On 2024-06-21 01:06, Simon Glass wrote:
The code here is confusing due to large blocks which are #ifdefed out. Add a function phase_sdram_init() which returns whether SDRAM init should happen in the current phase, using that as needed to control the code flow.
This increases code size by about 500 bytes in SPL when the cache is on, since it must call the rather large rockchip_sdram_size() function.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
Split out the refactoring into a separate patch
Drop the non-dcache optimisation, since the cache should normally be on
drivers/ram/rockchip/sdram_rk3399.c | 47 ++++++++++++++++------------- 1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c index 3c4e20f4e80..2f37dd712e7 100644 --- a/drivers/ram/rockchip/sdram_rk3399.c +++ b/drivers/ram/rockchip/sdram_rk3399.c @@ -13,6 +13,7 @@ #include <log.h> #include <ram.h> #include <regmap.h> +#include <spl.h> #include <syscon.h> #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/cru.h> @@ -63,8 +64,6 @@ struct chan_info { };
struct dram_info { -#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD)) u32 pwrup_srefresh_exit[2]; struct chan_info chan[2]; struct clk ddr_clk;@@ -75,7 +74,6 @@ struct dram_info { struct rk3399_pmusgrf_regs *pmusgrf; struct rk3399_ddr_cic_regs *cic; const struct sdram_rk3399_ops *ops; -#endif struct ram_info info; struct rk3399_pmugrf_regs *pmugrf; }; @@ -92,9 +90,6 @@ struct sdram_rk3399_ops { struct rk3399_sdram_params *params); };
-#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))- struct rockchip_dmc_plat { #if CONFIG_IS_ENABLED(OF_PLATDATA) struct dtd_rockchip_rk3399_dmc dtplat;
@@ -191,6 +186,17 @@ struct io_setting { }, };
+/**
- phase_sdram_init() - Check if this is the phase where SDRAM init happens
- Returns: true to do SDRAM init in this phase, false to not
- */
+static bool phase_sdram_init(void) +{
return spl_phase() == PHASE_TPL ||(!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());+}
- static struct io_setting * lpddr4_get_io_settings(const struct rk3399_sdram_params *params, u32 mr5) {
@@ -3024,7 +3030,7 @@ static int rk3399_dmc_of_to_plat(struct udevice *dev) struct rockchip_dmc_plat *plat = dev_get_plat(dev); int ret;
if (!CONFIG_IS_ENABLED(OF_REAL))
if (!CONFIG_IS_ENABLED(OF_REAL) || !phase_sdram_init()) return 0; ret = dev_read_u32_array(dev, "rockchip,sdram-params",@@ -3138,22 +3144,24 @@ static int rk3399_dmc_init(struct udevice *dev)
return 0;} -#endif
static int rk3399_dmc_probe(struct udevice *dev) { struct dram_info *priv = dev_get_priv(dev);
-#if defined(CONFIG_TPL_BUILD) || \
(!defined(CONFIG_TPL) && defined(CONFIG_SPL_BUILD))if (rk3399_dmc_init(dev))return 0;-#endif
priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);priv->info.base = CFG_SYS_SDRAM_BASE;priv->info.size =rockchip_sdram_size((phys_addr_t)&priv->pmugrf->os_reg2);
if (phase_sdram_init()) {if (rk3399_dmc_init(dev))return 0;} else {priv->pmugrf = syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF);debug("%s: pmugrf = %p\n", __func__, priv->pmugrf);}if (!CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {This check should be dropped, this driver intent is to initialize dram in first phase (normally TPL), and report the size to any other phase.
This whole patch can be dropped :-) Here I am just trying to avoid code-size growth when the cache is off. But yes, it is not needed.
The main need for phase_sdram_init() and disable of DCACHE can probably be avoided if bob/kevin can move to use separate TPL and SPL instead of doing both dram init and everything else in SPL.
My best guess is that enabling of caches in SPL cause issue for bob and kevin because they only use SPL not TPL+SPL like (if I am not mistaken) all other Rockchip arm64 targets.
Using SPL-only was not something I tested when caches was enabled in SPL.
Maybe bob/kevin can be changed to also use TPL+SPL similar to all other RK3399 boards?
How U-Boot works on these chromebooks is still a mystery to me.
When coreboot is involved it would only load U-Boot proper and SPL or TPL+SPL would never be involved at all?
If I understand correctly SPL is only involved for bare metal boot, if this is the case then using TPL + back-to-brom to load SPL should probably work fine?, and would align all RK3399 boards to work in a similar way and reduce the need for special care/handling in the code.
These boards were the first rk3399 support we had in mainline, I believe. Everything else came later but conversions were not done for these existing boards.
I actually intensely dislike the back-to-brom stuff. If the ROM had an actual API (mmc_read(), etc.) then that would be fine. But just jumping back makes it hard to control what is going on.
The logic of BootRom is very simple and very clear:
- Read image from boot medium and check the availability(format of
rockchip IDB, which is packed by the mkimage rksd/rkspi);
- Load TPL/dram init blob to the internal SRAM(size limit by the sram),
and jump to the entry;
- Back to bootRom with '0' return means the dram init success, bootRom
will get next image for dram;
- Read SPL image from boot medium to DDR start address(no size limit),
and jump to the ddr entry;
- The signature verify will be done for all the firmware load by bootRom
if the secure boot feature is enabled; This model works for all Rockchip SoCs, although some SoCs have very limit SRAM size(only available for DRAM init) and some other SoCs like rk3399 have bigger SRAM size.
Thanks for the info. Is there a document that describes this exactly? Some questions it should cover:
- What is the signature verification? - What offsets are used? - What memory addresses are things loaded to? - What other values of 'back to bootRom' are supported? - Is there a jump table listing available functions for U-Boot to call? - How to call the ROM functions to read blocks from mmc, SPI, etc.
Thanks,
- Kever
It is interesting to hear that SPL-only is causing a problem. Do you have any inkling of what that might be? I could take a look at converting the boards to use TPL, but it does seem a bit sideways to the issue here. Is the bootrom doing some magic, perhaps? Is there decent documentation for the boot ROM these days?
Regards, Simon
Regards, Simon
This causes a hang, so disable it. Unfortunately the RAM-size fix does not resolve the problem and I am unsure what is wrong. As soon as the cache is enabled the board appears to hang.
Fixes: 6d8cdfd1536 ("rockchip: spl: Enable caches to speed up checksum validation")
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
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
Now that am335x_evm boots OK on the Beaglebone black, drop the latter and update the docs to cover the change.
Also add a few updates about 'make fit' and drop the note about the security review, as U-Boot's verified boot has had quite extensive review now.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Tom Rini trini@konsulko.com ---
(no changes since v2)
Changes in v2: - Drop patch "regulator: rk8xx: Fix incorrect parameter" - Rewrite boneblack patch to onstead drop the target and update docs
board/ti/am335x/MAINTAINERS | 1 - configs/am335x_boneblack_vboot_defconfig | 94 ------------------------ configs/am335x_evm_defconfig | 3 +- doc/usage/fit/beaglebone_vboot.rst | 21 +++--- 4 files changed, 12 insertions(+), 107 deletions(-) delete mode 100644 configs/am335x_boneblack_vboot_defconfig
diff --git a/board/ti/am335x/MAINTAINERS b/board/ti/am335x/MAINTAINERS index 219c8715bf1..ed8800a2663 100644 --- a/board/ti/am335x/MAINTAINERS +++ b/board/ti/am335x/MAINTAINERS @@ -3,6 +3,5 @@ M: Tom Rini trini@konsulko.com S: Maintained F: board/ti/am335x/ F: include/configs/am335x_evm.h -F: configs/am335x_boneblack_vboot_defconfig F: configs/am335x_evm_defconfig F: configs/am335x_evm_spiboot_defconfig diff --git a/configs/am335x_boneblack_vboot_defconfig b/configs/am335x_boneblack_vboot_defconfig deleted file mode 100644 index d473a1a793b..00000000000 --- a/configs/am335x_boneblack_vboot_defconfig +++ /dev/null @@ -1,94 +0,0 @@ -CONFIG_ARM=y -CONFIG_ARCH_CPU_INIT=y -# CONFIG_SPL_USE_ARCH_MEMCPY is not set -# CONFIG_SPL_USE_ARCH_MEMSET is not set -CONFIG_ARCH_OMAP2PLUS=y -CONFIG_TI_COMMON_CMD_OPTIONS=y -CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y -CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x4030ff00 -CONFIG_SF_DEFAULT_SPEED=24000000 -CONFIG_DEFAULT_DEVICE_TREE="am335x-boneblack" -CONFIG_AM33XX=y -CONFIG_CLOCK_SYNTHESIZER=y -CONFIG_SPL=y -CONFIG_ENV_OFFSET_REDUND=0x280000 -CONFIG_TIMESTAMP=y -CONFIG_FIT_SIGNATURE=y -CONFIG_FIT_VERBOSE=y -CONFIG_SYS_BOOTM_LEN=0x1000000 -CONFIG_DISTRO_DEFAULTS=y -CONFIG_AUTOBOOT_KEYED=y -CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d seconds\n" -CONFIG_AUTOBOOT_DELAY_STR="d" -CONFIG_AUTOBOOT_STOP_STR=" " -CONFIG_BOOTCOMMAND="run findfdt; run init_console; run finduuid; run distro_bootcmd" -CONFIG_SYS_CONSOLE_INFO_QUIET=y -CONFIG_ARCH_MISC_INIT=y -CONFIG_SPL_SYS_MALLOC=y -CONFIG_SPL_SYS_MALLOC_SIZE=0x800000 -CONFIG_SPL_MUSB_NEW=y -# CONFIG_SPL_NAND_SUPPORT is not set -CONFIG_SPL_NET=y -CONFIG_SPL_NET_VCI_STRING="AM33xx U-Boot SPL" -CONFIG_SPL_OS_BOOT=y -CONFIG_SPL_FALCON_BOOT_MMCSD=y -CONFIG_SYS_MMCSD_RAW_MODE_KERNEL_SECTOR=0x1700 -CONFIG_SYS_MMCSD_RAW_MODE_ARGS_SECTOR=0x1500 -CONFIG_SYS_MMCSD_RAW_MODE_ARGS_SECTORS=0x200 -CONFIG_CMD_SPL=y -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 -CONFIG_SYS_RELOC_GD_ENV_ADDR=y -CONFIG_SYS_MMC_ENV_DEV=1 -CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG=y -CONFIG_VERSION_VARIABLE=y -CONFIG_NET_RETRY_COUNT=10 -CONFIG_BOOTP_SEND_HOSTNAME=y -# CONFIG_SPL_BLK is not set -CONFIG_BOOTCOUNT_LIMIT=y -CONFIG_SYS_BOOTCOUNT_BE=y -CONFIG_DFU_MMC=y -CONFIG_DFU_RAM=y -CONFIG_USB_FUNCTION_FASTBOOT=y -CONFIG_DM_I2C=y -CONFIG_MISC=y -CONFIG_SYS_I2C_EEPROM_ADDR=0x50 -# CONFIG_SPL_DM_MMC is not set -CONFIG_MMC_OMAP_HS=y -CONFIG_MTD=y -CONFIG_DM_SPI_FLASH=y -CONFIG_SPI_FLASH_WINBOND=y -CONFIG_PHY_ATHEROS=y -CONFIG_PHY_SMSC=y -CONFIG_PHY_GIGE=y -CONFIG_MII=y -CONFIG_DRIVER_TI_CPSW=y -CONFIG_DM_PMIC=y -# CONFIG_SPL_DM_PMIC is not set -CONFIG_PMIC_TPS65217=y -CONFIG_SPL_POWER_TPS65910=y -CONFIG_SPI=y -CONFIG_DM_SPI=y -CONFIG_OMAP3_SPI=y -CONFIG_TIMER=y -CONFIG_OMAP_TIMER=y -CONFIG_USB=y -CONFIG_DM_USB_GADGET=y -CONFIG_SPL_DM_USB_GADGET=y -CONFIG_USB_MUSB_HOST=y -CONFIG_USB_MUSB_GADGET=y -CONFIG_USB_MUSB_TI=y -CONFIG_USB_GADGET=y -CONFIG_SPL_USB_GADGET=y -CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments" -CONFIG_USB_GADGET_VENDOR_NUM=0x0451 -CONFIG_USB_GADGET_PRODUCT_NUM=0xd022 -CONFIG_USB_ETHER=y -CONFIG_SPL_USB_ETHER=y -CONFIG_LZO=y diff --git a/configs/am335x_evm_defconfig b/configs/am335x_evm_defconfig index d243cb16e72..cabc181460a 100644 --- a/configs/am335x_evm_defconfig +++ b/configs/am335x_evm_defconfig @@ -13,6 +13,8 @@ CONFIG_AM335X_USB0_PERIPHERAL=y CONFIG_AM335X_USB1=y CONFIG_SPL=y CONFIG_TIMESTAMP=y +CONFIG_FIT_SIGNATURE=y +CONFIG_FIT_VERBOSE=y CONFIG_SPL_LOAD_FIT=y CONFIG_SYS_BOOTM_LEN=0x1000000 CONFIG_DISTRO_DEFAULTS=y @@ -119,5 +121,4 @@ CONFIG_SPL_USB_ETHER=y CONFIG_WDT=y # CONFIG_SPL_WDT is not set CONFIG_DYNAMIC_CRC_TABLE=y -CONFIG_RSA=y CONFIG_LZO=y diff --git a/doc/usage/fit/beaglebone_vboot.rst b/doc/usage/fit/beaglebone_vboot.rst index cd6bb141910..1360c71803c 100644 --- a/doc/usage/fit/beaglebone_vboot.rst +++ b/doc/usage/fit/beaglebone_vboot.rst @@ -67,18 +67,20 @@ a. Set up the environment variable to point to your toolchain. You will need
export CROSS_COMPILE=arm-linux-gnueabi-
-b. Configure and build U-Boot with verified boot enabled:: +b. Configure and build U-Boot with verified boot enabled. Note that we use the +am335x_evm target since it covers all boards based on the AM335x evaluation +board::
export UBOOT=/path/to/u-boot cd $UBOOT # You can add -j10 if you have 10 CPUs to make it faster - make O=b/am335x_boneblack_vboot am335x_boneblack_vboot_config all - export UOUT=$UBOOT/b/am335x_boneblack_vboot + make O=b/am335x_evm am335x_evm_config all + export UOUT=$UBOOT/b/am335x_evm
c. You will now have a U-Boot image::
- file b/am335x_boneblack_vboot/u-boot-dtb.img - b/am335x_boneblack_vboot/u-boot-dtb.img: u-boot legacy uImage, + file b/am335x_evm/u-boot-dtb.img + b/am335x_evm/u-boot-dtb.img: u-boot legacy uImage, U-Boot 2014.07-rc2-00065-g2f69f8, Firmware/ARM, Firmware Image (Not compressed), 395375 bytes, Sat May 31 16:19:04 2014, Load Address: 0x80800000, Entry Point: 0x00000000, @@ -466,7 +468,7 @@ the private key that you signed with so that it can verify any kernels that you sign::
cd $UBOOT - make O=b/am335x_boneblack_vboot EXT_DTB=${WORK}/am335x-boneblack-pubkey.dtb + make O=b/am335x_evm EXT_DTB=${WORK}/am335x-boneblack-pubkey.dtb
Here we are overriding the normal device tree file with our one, which contains the public key. @@ -597,14 +599,11 @@ Further Improvements
Several of the steps here can be easily automated. In particular it would be capital if signing and packaging a kernel were easy, perhaps a simple make -target in the kernel. +target in the kernel. A stating point for this is the 'make image.fit' target +for ARM64 in Linux from v6.9 onwards.
Some mention of how to use multiple .dtb files in a FIT might be useful.
-U-Boot's verified boot mechanism has not had a robust and independent security -review. Such a review should look at the implementation and its resistance to -attacks. - Perhaps the verified boot feature could be integrated into the Amstrom distribution.
participants (5)
-
Ilias Apalodimas -
Jonas Karlman -
Kever Yang -
Simon Glass -
Sughosh Ganu