From 6e3a89f449fa5b4c0153990a64124211197f426a Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Mon, 30 Mar 2020 23:21:13 +0100 Subject: [PATCH 1/5] fdt/wrappers: Generalise fdtw_read_array() Currently our fdtw_read_array() implementation requires the length of the property to exactly match the requested size, which makes it less flexible for parsing generic device trees. Also the name is slightly misleading, since we treat the cells of the array as 32 bit unsigned integers, performing the endianess conversion. To fix those issues and align the code more with other DT users (Linux kernel or U-Boot), rename the function to "fdt_read_uint32_array", and relax the length check to only check if the property covers at least the number of cells we request. This also changes the variable names to be more in-line with other DT users, and switches to the proper data types. This makes this function more useful in later patches. Change-Id: Id86f4f588ffcb5106d4476763ecdfe35a735fa6c Signed-off-by: Andre Przywara --- common/fdt_wrappers.c | 31 +++++++++---------- include/common/fdt_wrappers.h | 4 +-- .../board/fvp/fconf/fconf_hw_config_getter.c | 4 +-- plat/arm/board/fvp/jmptbl.i | 1 + plat/arm/board/juno/jmptbl.i | 1 + plat/arm/common/fconf/arm_fconf_io.c | 3 +- plat/arm/common/fconf/arm_fconf_sp.c | 4 +-- 7 files changed, 24 insertions(+), 24 deletions(-) diff --git a/common/fdt_wrappers.c b/common/fdt_wrappers.c index ca5b4556d..5afa14271 100644 --- a/common/fdt_wrappers.c +++ b/common/fdt_wrappers.c @@ -65,38 +65,35 @@ int fdtw_read_cells(const void *dtb, int node, const char *prop, /* * Read cells from a given property of the given node. Any number of 32-bit - * cells of the property can be read. The fdt pointer is updated. Returns 0 on - * success, and -1 on error. + * cells of the property can be read. Returns 0 on success, or a negative + * FDT error value otherwise. */ -int fdtw_read_array(const void *dtb, int node, const char *prop, - unsigned int cells, void *value) +int fdt_read_uint32_array(const void *dtb, int node, const char *prop_name, + unsigned int cells, uint32_t *value) { - const uint32_t *value_ptr; + const fdt32_t *prop; int value_len; assert(dtb != NULL); - assert(prop != NULL); + assert(prop_name != NULL); assert(value != NULL); assert(node >= 0); /* Access property and obtain its length (in bytes) */ - value_ptr = fdt_getprop_namelen(dtb, node, prop, (int)strlen(prop), - &value_len); - if (value_ptr == NULL) { - WARN("Couldn't find property %s in dtb\n", prop); - return -1; + prop = fdt_getprop(dtb, node, prop_name, &value_len); + if (prop == NULL) { + WARN("Couldn't find property %s in dtb\n", prop_name); + return -FDT_ERR_NOTFOUND; } - /* Verify that property length accords with cell length */ - if (NCELLS((unsigned int)value_len) != cells) { + /* Verify that property length can fill the entire array. */ + if (NCELLS((unsigned int)value_len) < cells) { WARN("Property length mismatch\n"); - return -1; + return -FDT_ERR_BADVALUE; } - uint32_t *dst = value; - for (unsigned int i = 0U; i < cells; i++) { - dst[i] = fdt32_to_cpu(value_ptr[i]); + value[i] = fdt32_to_cpu(prop[i]); } return 0; diff --git a/include/common/fdt_wrappers.h b/include/common/fdt_wrappers.h index f467958b7..e3158f1c5 100644 --- a/include/common/fdt_wrappers.h +++ b/include/common/fdt_wrappers.h @@ -14,8 +14,8 @@ int fdtw_read_cells(const void *dtb, int node, const char *prop, unsigned int cells, void *value); -int fdtw_read_array(const void *dtb, int node, const char *prop, - unsigned int cells, void *value); +int fdt_read_uint32_array(const void *dtb, int node, const char *prop_name, + unsigned int cells, uint32_t *value); int fdtw_read_string(const void *dtb, int node, const char *prop, char *str, size_t size); int fdtw_write_inplace_cells(void *dtb, int node, const char *prop, diff --git a/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c b/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c index 2952cde80..bac1f1587 100644 --- a/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c +++ b/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c @@ -18,7 +18,7 @@ int fconf_populate_gicv3_config(uintptr_t config) { int err; int node; - int addr[20]; + uint32_t addr[20]; /* Necessary to work with libfdt APIs */ const void *hw_config_dtb = (const void *)config; @@ -42,7 +42,7 @@ int fconf_populate_gicv3_config(uintptr_t config) <0x0 0x2c02f000 0 0x2000>; // GICV */ - err = fdtw_read_array(hw_config_dtb, node, "reg", 20, &addr); + err = fdt_read_uint32_array(hw_config_dtb, node, "reg", 20, addr); if (err < 0) { ERROR("FCONF: Failed to read reg property of GIC node\n"); } diff --git a/plat/arm/board/fvp/jmptbl.i b/plat/arm/board/fvp/jmptbl.i index 0c93d0aab..6469e2995 100644 --- a/plat/arm/board/fvp/jmptbl.i +++ b/plat/arm/board/fvp/jmptbl.i @@ -15,6 +15,7 @@ # fdt fdt_getprop_namelen patch rom rom_lib_init +fdt fdt_getprop fdt fdt_getprop_namelen fdt fdt_setprop_inplace fdt fdt_check_header diff --git a/plat/arm/board/juno/jmptbl.i b/plat/arm/board/juno/jmptbl.i index b1b9ed463..66d6e4592 100644 --- a/plat/arm/board/juno/jmptbl.i +++ b/plat/arm/board/juno/jmptbl.i @@ -15,6 +15,7 @@ # fdt fdt_getprop_namelen patch rom rom_lib_init +fdt fdt_getprop fdt fdt_getprop_namelen fdt fdt_setprop_inplace fdt fdt_check_header diff --git a/plat/arm/common/fconf/arm_fconf_io.c b/plat/arm/common/fconf/arm_fconf_io.c index 6ebc467ed..26e51b2a3 100644 --- a/plat/arm/common/fconf/arm_fconf_io.c +++ b/plat/arm/common/fconf/arm_fconf_io.c @@ -241,7 +241,8 @@ int fconf_populate_arm_io_policies(uintptr_t config) /* Locate the uuid cells and read the value for all the load info uuid */ for (i = 0; i < FCONF_ARM_IO_UUID_NUMBER; i++) { uuid_ptr = pool_alloc(&fconf_arm_uuids_pool); - err = fdtw_read_array(dtb, node, load_info[i].name, 4, &uuid_helper.word); + err = fdt_read_uint32_array(dtb, node, load_info[i].name, + 4, uuid_helper.word); if (err < 0) { WARN("FCONF: Read cell failed for %s\n", load_info[i].name); return err; diff --git a/plat/arm/common/fconf/arm_fconf_sp.c b/plat/arm/common/fconf/arm_fconf_sp.c index 9b6fa9b1f..c46ecb114 100644 --- a/plat/arm/common/fconf/arm_fconf_sp.c +++ b/plat/arm/common/fconf/arm_fconf_sp.c @@ -44,8 +44,8 @@ int fconf_populate_arm_sp(uintptr_t config) } fdt_for_each_subnode(sp_node, dtb, node) { - err = fdtw_read_array(dtb, sp_node, "uuid", 4, - &uuid_helper.word); + err = fdt_read_uint32_array(dtb, sp_node, "uuid", 4, + uuid_helper.word); if (err < 0) { ERROR("FCONF: cannot read SP uuid\n"); return -1; From 52a616b48c617fe8721106f29f2910ca4681afea Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Thu, 26 Mar 2020 12:51:21 +0000 Subject: [PATCH 2/5] plat/stm32: Use generic fdt_read_uint32_array() implementation The device tree parsing code for the STM32 platform is using its own FDT helper functions, some of them being rather generic. In particular the existing fdt_read_uint32_array() implementation is now almost identical to the new generic code in fdt_wrappers.c, so we can remove the ST specific version and adjust the existing callers. Compared to the original ST implementation the new version takes a pointer to the DTB as the first argument, and also swaps the order of the number of cells and the pointer. Change-Id: Id06b0f1ba4db1ad1f733be40e82c34f46638551a Signed-off-by: Andre Przywara --- drivers/st/clk/stm32mp1_clk.c | 22 ++++++++++------ drivers/st/clk/stm32mp_clkfunc.c | 7 ++--- drivers/st/ddr/stm32mp1_ram.c | 7 ++--- include/drivers/st/stm32mp_clkfunc.h | 4 +-- plat/st/common/include/stm32mp_dt.h | 2 -- plat/st/common/stm32mp_dt.c | 38 +++------------------------- plat/st/stm32mp1/platform.mk | 3 ++- 7 files changed, 29 insertions(+), 54 deletions(-) diff --git a/drivers/st/clk/stm32mp1_clk.c b/drivers/st/clk/stm32mp1_clk.c index 0cc87cc71..a16f36dcc 100644 --- a/drivers/st/clk/stm32mp1_clk.c +++ b/drivers/st/clk/stm32mp1_clk.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -1600,20 +1601,25 @@ int stm32mp1_clk_init(void) bool pll4_preserve = false; bool pll4_bootrom = false; const fdt32_t *pkcs_cell; + void *fdt; + + if (fdt_get_address(&fdt) == 0) { + return false; + } /* Check status field to disable security */ if (!fdt_get_rcc_secure_status()) { mmio_write_32(rcc_base + RCC_TZCR, 0); } - ret = fdt_rcc_read_uint32_array("st,clksrc", clksrc, - (uint32_t)CLKSRC_NB); + ret = fdt_rcc_read_uint32_array("st,clksrc", (uint32_t)CLKSRC_NB, + clksrc); if (ret < 0) { return -FDT_ERR_NOTFOUND; } - ret = fdt_rcc_read_uint32_array("st,clkdiv", clkdiv, - (uint32_t)CLKDIV_NB); + ret = fdt_rcc_read_uint32_array("st,clkdiv", (uint32_t)CLKDIV_NB, + clkdiv); if (ret < 0) { return -FDT_ERR_NOTFOUND; } @@ -1628,8 +1634,8 @@ int stm32mp1_clk_init(void) continue; } - ret = fdt_read_uint32_array(plloff[i], "cfg", - pllcfg[i], (int)PLLCFG_NB); + ret = fdt_read_uint32_array(fdt, plloff[i], "cfg", + (int)PLLCFG_NB, pllcfg[i]); if (ret < 0) { return -FDT_ERR_NOTFOUND; } @@ -1800,8 +1806,8 @@ int stm32mp1_clk_init(void) if (ret != 0) { return ret; } - ret = fdt_read_uint32_array(plloff[i], "csg", csg, - (uint32_t)PLLCSG_NB); + ret = fdt_read_uint32_array(fdt, plloff[i], "csg", + (uint32_t)PLLCSG_NB, csg); if (ret == 0) { stm32mp1_pll_csg(i, csg); } else if (ret != -FDT_ERR_NOTFOUND) { diff --git a/drivers/st/clk/stm32mp_clkfunc.c b/drivers/st/clk/stm32mp_clkfunc.c index 87c8e2b84..6404d9933 100644 --- a/drivers/st/clk/stm32mp_clkfunc.c +++ b/drivers/st/clk/stm32mp_clkfunc.c @@ -10,6 +10,7 @@ #include +#include #include #include @@ -200,8 +201,8 @@ uint32_t fdt_rcc_read_addr(void) * @param count: number of parameters to be read * @return: 0 on succes or a negative value on error */ -int fdt_rcc_read_uint32_array(const char *prop_name, - uint32_t *array, uint32_t count) +int fdt_rcc_read_uint32_array(const char *prop_name, uint32_t count, + uint32_t *array) { int node; void *fdt; @@ -215,7 +216,7 @@ int fdt_rcc_read_uint32_array(const char *prop_name, return -FDT_ERR_NOTFOUND; } - return fdt_read_uint32_array(node, prop_name, array, count); + return fdt_read_uint32_array(fdt, node, prop_name, count, array); } /* diff --git a/drivers/st/ddr/stm32mp1_ram.c b/drivers/st/ddr/stm32mp1_ram.c index 40cd4554f..273dec0df 100644 --- a/drivers/st/ddr/stm32mp1_ram.c +++ b/drivers/st/ddr/stm32mp1_ram.c @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -223,10 +224,10 @@ static int stm32mp1_ddr_setup(void) INFO("RAM: %s\n", config.info.name); for (idx = 0; idx < ARRAY_SIZE(param); idx++) { - ret = fdt_read_uint32_array(node, param[idx].name, + ret = fdt_read_uint32_array(fdt, node, param[idx].name, + param[idx].size, (void *)((uintptr_t)&config + - param[idx].offset), - param[idx].size); + param[idx].offset)); VERBOSE("%s: %s[0x%x] = %d\n", __func__, param[idx].name, param[idx].size, ret); diff --git a/include/drivers/st/stm32mp_clkfunc.h b/include/drivers/st/stm32mp_clkfunc.h index 076916730..0902f445d 100644 --- a/include/drivers/st/stm32mp_clkfunc.h +++ b/include/drivers/st/stm32mp_clkfunc.h @@ -21,8 +21,8 @@ uint32_t fdt_osc_read_uint32_default(enum stm32mp_osc_id osc_id, int fdt_get_rcc_node(void *fdt); uint32_t fdt_rcc_read_addr(void); -int fdt_rcc_read_uint32_array(const char *prop_name, - uint32_t *array, uint32_t count); +int fdt_rcc_read_uint32_array(const char *prop_name, uint32_t count, + uint32_t *array); int fdt_rcc_subnode_offset(const char *name); const fdt32_t *fdt_rcc_read_prop(const char *prop_name, int *lenp); bool fdt_get_rcc_secure_status(void); diff --git a/plat/st/common/include/stm32mp_dt.h b/plat/st/common/include/stm32mp_dt.h index a29d9148d..92f3d6528 100644 --- a/plat/st/common/include/stm32mp_dt.h +++ b/plat/st/common/include/stm32mp_dt.h @@ -30,8 +30,6 @@ bool fdt_check_node(int node); uint8_t fdt_get_status(int node); uint32_t fdt_read_uint32_default(int node, const char *prop_name, uint32_t dflt_value); -int fdt_read_uint32_array(int node, const char *prop_name, - uint32_t *array, uint32_t count); int fdt_get_reg_props_by_name(int node, const char *name, uintptr_t *base, size_t *size); int dt_set_stdout_pinctrl(void); diff --git a/plat/st/common/stm32mp_dt.c b/plat/st/common/stm32mp_dt.c index acb323cf2..6b76424ae 100644 --- a/plat/st/common/stm32mp_dt.c +++ b/plat/st/common/stm32mp_dt.c @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -154,39 +155,6 @@ uint32_t fdt_read_uint32_default(int node, const char *prop_name, return fdt32_to_cpu(*cuint); } -/******************************************************************************* - * This function reads a series of parameters in a node property - * (generic use of fdt library). - * It reads the values inside the device tree, from property name and node. - * The number of parameters is also indicated as entry parameter. - * Returns 0 on success and a negative FDT error code on failure. - * If success, values are stored at the third parameter address. - ******************************************************************************/ -int fdt_read_uint32_array(int node, const char *prop_name, uint32_t *array, - uint32_t count) -{ - const fdt32_t *cuint; - int len; - uint32_t i; - - cuint = fdt_getprop(fdt, node, prop_name, &len); - if (cuint == NULL) { - return -FDT_ERR_NOTFOUND; - } - - if ((uint32_t)len != (count * sizeof(uint32_t))) { - return -FDT_ERR_BADLAYOUT; - } - - for (i = 0; i < ((uint32_t)len / sizeof(uint32_t)); i++) { - *array = fdt32_to_cpu(*cuint); - array++; - cuint++; - } - - return 0; -} - /******************************************************************************* * This function fills reg node info (base & size) with an index found by * checking the reg-names node. @@ -396,7 +364,7 @@ uintptr_t dt_get_ddrctrl_base(void) assert((fdt_get_node_parent_address_cells(node) == 1) && (fdt_get_node_parent_size_cells(node) == 1)); - if (fdt_read_uint32_array(node, "reg", array, 4) < 0) { + if (fdt_read_uint32_array(fdt, node, "reg", 4, array) < 0) { return 0; } @@ -421,7 +389,7 @@ uintptr_t dt_get_ddrphyc_base(void) assert((fdt_get_node_parent_address_cells(node) == 1) && (fdt_get_node_parent_size_cells(node) == 1)); - if (fdt_read_uint32_array(node, "reg", array, 4) < 0) { + if (fdt_read_uint32_array(fdt, node, "reg", 4, array) < 0) { return 0; } diff --git a/plat/st/stm32mp1/platform.mk b/plat/st/stm32mp1/platform.mk index 5ce7a9c4f..b0ba82abf 100644 --- a/plat/st/stm32mp1/platform.mk +++ b/plat/st/stm32mp1/platform.mk @@ -62,7 +62,8 @@ DTC_FLAGS += -Wno-unit_address_vs_reg include lib/libfdt/libfdt.mk -PLAT_BL_COMMON_SOURCES := plat/st/common/stm32mp_common.c \ +PLAT_BL_COMMON_SOURCES := common/fdt_wrappers.c \ + plat/st/common/stm32mp_common.c \ plat/st/stm32mp1/stm32mp1_private.c PLAT_BL_COMMON_SOURCES += drivers/st/uart/aarch32/stm32_console.S From ff4e6c35c9f3f0b1d190b5d3761a13d701af6925 Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Thu, 26 Mar 2020 11:22:37 +0000 Subject: [PATCH 3/5] fdt/wrappers: Replace fdtw_read_cells() implementation Our fdtw_read_cells() implementation goes to great lengths to sanity-check every parameter and result, but leaves a big hole open: The size of the storage the value pointer points at needs to match the number of cells given. This can't be easily checked at compile time, since we lose the size information by using a void pointer. Regardless the current usage of this function is somewhat wrong anyways, since we use it on single-element, fixed-length properties only, for which the DT binding specifies the size. Typically we use those functions dealing with a number of cells in DT context to deal with *dynamically* sized properties, which depend on other properties (#size-cells, #clock-cells, ...), to specify the number of cells needed. Another problem with the current implementation is the use of ambiguously sized types (uintptr_t, size_t) together with a certain expectation about their size. In general there is no relation between the length of a DT property and the bitness of the code that parses the DTB: AArch64 code could encounter 32-bit addresses (where the physical address space is limited to 4GB [1]), while AArch32 code could read 64-bit sized properties (/memory nodes on LPAE systems, [2]). To make this more clear, fix the potential issues and also align more with other DT users (Linux and U-Boot), introduce functions to explicitly read uint32 and uint64 properties. As the other DT consumers, we do this based on the generic "read array" function. Convert all users to use either of those two new functions, and make sure we never use a pointer to anything other than uint32_t or uint64_t variables directly. This reveals (and fixes) a bug in plat_spmd_manifest.c, where we write 4 bytes into a uint16_t variable (passed via a void pointer). Also we change the implementation of the function to better align with other libfdt users, by using the right types (fdt32_t) and common variable names (*prop, prop_names). [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi#n874 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ecx-2000.dts Change-Id: I718de960515117ac7a3331a1b177d2ec224a3890 Signed-off-by: Andre Przywara --- common/fdt_wrappers.c | 70 ++++++------------- include/common/fdt_wrappers.h | 6 +- lib/fconf/fconf_dyn_cfg_getter.c | 12 +++- lib/fconf/fconf_tbbr_getter.c | 12 ++-- .../board/fvp/fconf/fconf_hw_config_getter.c | 9 +-- plat/arm/common/fconf/arm_fconf_sp.c | 5 +- plat/common/plat_spmd_manifest.c | 16 +++-- 7 files changed, 58 insertions(+), 72 deletions(-) diff --git a/common/fdt_wrappers.c b/common/fdt_wrappers.c index 5afa14271..394f3b0ca 100644 --- a/common/fdt_wrappers.c +++ b/common/fdt_wrappers.c @@ -14,55 +14,6 @@ #include #include -/* - * Read cells from a given property of the given node. At most 2 cells of the - * property are read, and pointer is updated. Returns 0 on success, and -1 upon - * error - */ -int fdtw_read_cells(const void *dtb, int node, const char *prop, - unsigned int cells, void *value) -{ - const uint32_t *value_ptr; - uint32_t hi = 0, lo; - int value_len; - - assert(dtb != NULL); - assert(prop != NULL); - assert(value != NULL); - assert(node >= 0); - - /* We expect either 1 or 2 cell property */ - assert(cells <= 2U); - - /* Access property and obtain its length (in bytes) */ - value_ptr = fdt_getprop_namelen(dtb, node, prop, (int)strlen(prop), - &value_len); - if (value_ptr == NULL) { - WARN("Couldn't find property %s in dtb\n", prop); - return -1; - } - - /* Verify that property length accords with cell length */ - if (NCELLS((unsigned int)value_len) != cells) { - WARN("Property length mismatch\n"); - return -1; - } - - if (cells == 2U) { - hi = fdt32_to_cpu(*value_ptr); - value_ptr++; - } - - lo = fdt32_to_cpu(*value_ptr); - - if (cells == 2U) - *((uint64_t *) value) = ((uint64_t) hi << 32) | lo; - else - *((uint32_t *) value) = lo; - - return 0; -} - /* * Read cells from a given property of the given node. Any number of 32-bit * cells of the property can be read. Returns 0 on success, or a negative @@ -99,6 +50,27 @@ int fdt_read_uint32_array(const void *dtb, int node, const char *prop_name, return 0; } +int fdt_read_uint32(const void *dtb, int node, const char *prop_name, + uint32_t *value) +{ + return fdt_read_uint32_array(dtb, node, prop_name, 1, value); +} + +int fdt_read_uint64(const void *dtb, int node, const char *prop_name, + uint64_t *value) +{ + uint32_t array[2] = {0, 0}; + int ret; + + ret = fdt_read_uint32_array(dtb, node, prop_name, 2, array); + if (ret < 0) { + return ret; + } + + *value = ((uint64_t)array[0] << 32) | array[1]; + return 0; +} + /* * Read bytes from a given property of the given node. Any number of * bytes of the property can be read. The fdt pointer is updated. diff --git a/include/common/fdt_wrappers.h b/include/common/fdt_wrappers.h index e3158f1c5..e28dee142 100644 --- a/include/common/fdt_wrappers.h +++ b/include/common/fdt_wrappers.h @@ -12,8 +12,10 @@ /* Number of cells, given total length in bytes. Each cell is 4 bytes long */ #define NCELLS(len) ((len) / 4U) -int fdtw_read_cells(const void *dtb, int node, const char *prop, - unsigned int cells, void *value); +int fdt_read_uint32(const void *dtb, int node, const char *prop_name, + uint32_t *value); +int fdt_read_uint64(const void *dtb, int node, const char *prop_name, + uint64_t *value); int fdt_read_uint32_array(const void *dtb, int node, const char *prop_name, unsigned int cells, uint32_t *value); int fdtw_read_string(const void *dtb, int node, const char *prop, diff --git a/lib/fconf/fconf_dyn_cfg_getter.c b/lib/fconf/fconf_dyn_cfg_getter.c index 317d3e5d3..03aaf9bb6 100644 --- a/lib/fconf/fconf_dyn_cfg_getter.c +++ b/lib/fconf/fconf_dyn_cfg_getter.c @@ -57,26 +57,32 @@ int fconf_populate_dtb_registry(uintptr_t config) } fdt_for_each_subnode(child, dtb, node) { + uint32_t val32; + uint64_t val64; + dtb_info = pool_alloc(&dtb_info_pool); /* Read configuration dtb information */ - rc = fdtw_read_cells(dtb, child, "load-address", 2, &dtb_info->config_addr); + rc = fdt_read_uint64(dtb, child, "load-address", &val64); if (rc < 0) { ERROR("FCONF: Incomplete configuration property in dtb-registry.\n"); return rc; } + dtb_info->config_addr = (uintptr_t)val64; - rc = fdtw_read_cells(dtb, child, "max-size", 1, &dtb_info->config_max_size); + rc = fdt_read_uint32(dtb, child, "max-size", &val32); if (rc < 0) { ERROR("FCONF: Incomplete configuration property in dtb-registry.\n"); return rc; } + dtb_info->config_max_size = val32; - rc = fdtw_read_cells(dtb, child, "id", 1, &dtb_info->config_id); + rc = fdt_read_uint32(dtb, child, "id", &val32); if (rc < 0) { ERROR("FCONF: Incomplete configuration property in dtb-registry.\n"); return rc; } + dtb_info->config_id = val32; VERBOSE("FCONF: dyn_cfg.dtb_registry cell found with:\n"); VERBOSE("\tload-address = %lx\n", dtb_info->config_addr); diff --git a/lib/fconf/fconf_tbbr_getter.c b/lib/fconf/fconf_tbbr_getter.c index a4d61d8cd..21278019e 100644 --- a/lib/fconf/fconf_tbbr_getter.c +++ b/lib/fconf/fconf_tbbr_getter.c @@ -17,6 +17,8 @@ int fconf_populate_tbbr_dyn_config(uintptr_t config) { int err; int node; + uint64_t val64; + uint32_t val32; /* As libfdt use void *, we can't avoid this cast */ const void *dtb = (void *)config; @@ -30,7 +32,7 @@ int fconf_populate_tbbr_dyn_config(uintptr_t config) } /* Locate the disable_auth cell and read the value */ - err = fdtw_read_cells(dtb, node, "disable_auth", 1, &tbbr_dyn_config.disable_auth); + err = fdt_read_uint32(dtb, node, "disable_auth", &tbbr_dyn_config.disable_auth); if (err < 0) { WARN("FCONF: Read cell failed for `disable_auth`\n"); return err; @@ -48,19 +50,19 @@ int fconf_populate_tbbr_dyn_config(uintptr_t config) #endif /* Retrieve the Mbed TLS heap details from the DTB */ - err = fdtw_read_cells(dtb, node, - "mbedtls_heap_addr", 2, &tbbr_dyn_config.mbedtls_heap_addr); + err = fdt_read_uint64(dtb, node, "mbedtls_heap_addr", &val64); if (err < 0) { ERROR("FCONF: Read cell failed for mbedtls_heap\n"); return err; } + tbbr_dyn_config.mbedtls_heap_addr = (void *)(uintptr_t)val64; - err = fdtw_read_cells(dtb, node, - "mbedtls_heap_size", 1, &tbbr_dyn_config.mbedtls_heap_size); + err = fdt_read_uint32(dtb, node, "mbedtls_heap_size", &val32); if (err < 0) { ERROR("FCONF: Read cell failed for mbedtls_heap\n"); return err; } + tbbr_dyn_config.mbedtls_heap_size = val32; VERBOSE("FCONF:tbbr.disable_auth cell found with value = %d\n", tbbr_dyn_config.disable_auth); diff --git a/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c b/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c index bac1f1587..44e9d0135 100644 --- a/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c +++ b/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c @@ -51,8 +51,9 @@ int fconf_populate_gicv3_config(uintptr_t config) int fconf_populate_topology(uintptr_t config) { - int err, node, cluster_node, core_node, thread_node, max_pwr_lvl = 0; + int err, node, cluster_node, core_node, thread_node; uint32_t cluster_count = 0, max_cpu_per_cluster = 0, total_cpu_count = 0; + uint32_t max_pwr_lvl = 0; /* Necessary to work with libfdt APIs */ const void *hw_config_dtb = (const void *)config; @@ -64,7 +65,7 @@ int fconf_populate_topology(uintptr_t config) return node; } - err = fdtw_read_cells(hw_config_dtb, node, "max-pwr-lvl", 1, &max_pwr_lvl); + err = fdt_read_uint32(hw_config_dtb, node, "max-pwr-lvl", &max_pwr_lvl); if (err < 0) { /* * Some legacy FVP dts may not have this property. Assign the default @@ -74,7 +75,7 @@ int fconf_populate_topology(uintptr_t config) max_pwr_lvl = 2; } - assert((uint32_t)max_pwr_lvl <= MPIDR_AFFLVL2); + assert(max_pwr_lvl <= MPIDR_AFFLVL2); /* Find the offset of the "cpus" node */ node = fdt_path_offset(hw_config_dtb, "/cpus"); @@ -156,7 +157,7 @@ int fconf_populate_topology(uintptr_t config) return -1; } - soc_topology.plat_max_pwr_level = (uint32_t)max_pwr_lvl; + soc_topology.plat_max_pwr_level = max_pwr_lvl; soc_topology.plat_cluster_count = cluster_count; soc_topology.cluster_cpu_count = max_cpu_per_cluster; soc_topology.plat_cpu_count = total_cpu_count; diff --git a/plat/arm/common/fconf/arm_fconf_sp.c b/plat/arm/common/fconf/arm_fconf_sp.c index c46ecb114..1b09bc8e5 100644 --- a/plat/arm/common/fconf/arm_fconf_sp.c +++ b/plat/arm/common/fconf/arm_fconf_sp.c @@ -29,6 +29,7 @@ int fconf_populate_arm_sp(uintptr_t config) int sp_node, node, err; union uuid_helper_t uuid_helper; unsigned int index = 0; + uint32_t val32; const unsigned int sp_start_index = MAX_NUMBER_IDS - MAX_SP_IDS; /* As libfdt use void *, we can't avoid this cast */ @@ -53,12 +54,12 @@ int fconf_populate_arm_sp(uintptr_t config) arm_sp.uuids[index] = uuid_helper; - err = fdtw_read_cells(dtb, sp_node, "load-address", 1, - &arm_sp.load_addr[index]); + err = fdt_read_uint32(dtb, sp_node, "load-address", &val32); if (err < 0) { ERROR("FCONF: cannot read SP load address\n"); return -1; } + arm_sp.load_addr[index] = val32; VERBOSE("FCONF: %s UUID %x-%x-%x-%x load_addr=%lx\n", __func__, diff --git a/plat/common/plat_spmd_manifest.c b/plat/common/plat_spmd_manifest.c index f0aa27cfb..8330356ae 100644 --- a/plat/common/plat_spmd_manifest.c +++ b/plat/common/plat_spmd_manifest.c @@ -21,41 +21,43 @@ static int manifest_parse_attribute(spmc_manifest_sect_attribute_t *attr, const void *fdt, int node) { + uint32_t val32; int rc = 0; assert(attr && fdt); - rc = fdtw_read_cells(fdt, node, "maj_ver", 1, &attr->major_version); + rc = fdt_read_uint32(fdt, node, "maj_ver", &attr->major_version); if (rc) { ERROR("Missing SPCI major version in SPM core manifest.\n"); return -ENOENT; } - rc = fdtw_read_cells(fdt, node, "min_ver", 1, &attr->minor_version); + rc = fdt_read_uint32(fdt, node, "min_ver", &attr->minor_version); if (rc) { ERROR("Missing SPCI minor version in SPM core manifest.\n"); return -ENOENT; } - rc = fdtw_read_cells(fdt, node, "spmc_id", 1, &attr->spmc_id); + rc = fdt_read_uint32(fdt, node, "spmc_id", &val32); if (rc) { ERROR("Missing SPMC ID in manifest.\n"); return -ENOENT; } + attr->spmc_id = val32; - rc = fdtw_read_cells(fdt, node, "exec_state", 1, &attr->exec_state); + rc = fdt_read_uint32(fdt, node, "exec_state", &attr->exec_state); if (rc) NOTICE("Execution state not specified in SPM core manifest.\n"); - rc = fdtw_read_cells(fdt, node, "binary_size", 1, &attr->binary_size); + rc = fdt_read_uint32(fdt, node, "binary_size", &attr->binary_size); if (rc) NOTICE("Binary size not specified in SPM core manifest.\n"); - rc = fdtw_read_cells(fdt, node, "load_address", 2, &attr->load_address); + rc = fdt_read_uint64(fdt, node, "load_address", &attr->load_address); if (rc) NOTICE("Load address not specified in SPM core manifest.\n"); - rc = fdtw_read_cells(fdt, node, "entrypoint", 2, &attr->entrypoint); + rc = fdt_read_uint64(fdt, node, "entrypoint", &attr->entrypoint); if (rc) NOTICE("Entrypoint not specified in SPM core manifest.\n"); From be858cffa91fbcd5b8657200fbec1667c65bb1b7 Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Thu, 26 Mar 2020 11:50:33 +0000 Subject: [PATCH 4/5] plat/stm32: Implement fdt_read_uint32_default() as a wrapper The STM32 platform code uses its own set of FDT helper functions, although some of them are fairly generic. Remove the implementation of fdt_read_uint32_default() and implement it on top of the newly introduced fdt_read_uint32() function, then convert all users over. This also fixes two callers, which were slightly abusing the "default" semantic. Change-Id: I570533362b4846e58dd797a92347de3e0e5abb75 Signed-off-by: Andre Przywara --- common/fdt_wrappers.c | 13 +++++++++++++ drivers/st/clk/stm32mp1_clk.c | 9 ++++++--- drivers/st/clk/stm32mp_clkfunc.c | 3 ++- drivers/st/ddr/stm32mp1_ram.c | 8 ++++---- include/common/fdt_wrappers.h | 2 ++ plat/st/common/include/stm32mp_dt.h | 2 -- plat/st/common/stm32mp_dt.c | 22 +--------------------- 7 files changed, 28 insertions(+), 31 deletions(-) diff --git a/common/fdt_wrappers.c b/common/fdt_wrappers.c index 394f3b0ca..57e3bcd40 100644 --- a/common/fdt_wrappers.c +++ b/common/fdt_wrappers.c @@ -56,6 +56,19 @@ int fdt_read_uint32(const void *dtb, int node, const char *prop_name, return fdt_read_uint32_array(dtb, node, prop_name, 1, value); } +uint32_t fdt_read_uint32_default(const void *dtb, int node, + const char *prop_name, uint32_t dflt_value) +{ + uint32_t ret = dflt_value; + int err = fdt_read_uint32(dtb, node, prop_name, &ret); + + if (err < 0) { + return dflt_value; + } + + return ret; +} + int fdt_read_uint64(const void *dtb, int node, const char *prop_name, uint64_t *value) { diff --git a/drivers/st/clk/stm32mp1_clk.c b/drivers/st/clk/stm32mp1_clk.c index a16f36dcc..540c66aa3 100644 --- a/drivers/st/clk/stm32mp1_clk.c +++ b/drivers/st/clk/stm32mp1_clk.c @@ -1240,7 +1240,8 @@ static bool stm32mp1_check_pll_conf(enum stm32mp1_pll_id pll_id, uintptr_t clksrc_address = rcc_base + (clksrc >> 4); unsigned long refclk; uint32_t ifrge = 0U; - uint32_t src, value, fracv; + uint32_t src, value, fracv = 0; + void *fdt; /* Check PLL output */ if (mmio_read_32(pllxcr) != RCC_PLLNCR_PLLON) { @@ -1279,7 +1280,9 @@ static bool stm32mp1_check_pll_conf(enum stm32mp1_pll_id pll_id, } /* Fractional configuration */ - fracv = fdt_read_uint32_default(plloff, "frac", 0); + if (fdt_get_address(&fdt) == 1) { + fracv = fdt_read_uint32_default(fdt, plloff, "frac", 0); + } value = fracv << RCC_PLLNFRACR_FRACV_SHIFT; value |= RCC_PLLNFRACR_FRACLE; @@ -1800,7 +1803,7 @@ int stm32mp1_clk_init(void) continue; } - fracv = fdt_read_uint32_default(plloff[i], "frac", 0); + fracv = fdt_read_uint32_default(fdt, plloff[i], "frac", 0); ret = stm32mp1_pll_config(i, pllcfg[i], fracv); if (ret != 0) { diff --git a/drivers/st/clk/stm32mp_clkfunc.c b/drivers/st/clk/stm32mp_clkfunc.c index 6404d9933..e87ab1ba7 100644 --- a/drivers/st/clk/stm32mp_clkfunc.c +++ b/drivers/st/clk/stm32mp_clkfunc.c @@ -151,7 +151,8 @@ uint32_t fdt_osc_read_uint32_default(enum stm32mp_osc_id osc_id, continue; } - return fdt_read_uint32_default(subnode, prop_name, dflt_value); + return fdt_read_uint32_default(fdt, subnode, prop_name, + dflt_value); } return dflt_value; diff --git a/drivers/st/ddr/stm32mp1_ram.c b/drivers/st/ddr/stm32mp1_ram.c index 273dec0df..b21c8949f 100644 --- a/drivers/st/ddr/stm32mp1_ram.c +++ b/drivers/st/ddr/stm32mp1_ram.c @@ -206,13 +206,13 @@ static int stm32mp1_ddr_setup(void) return -EINVAL; } - config.info.speed = fdt_read_uint32_default(node, "st,mem-speed", 0); - if (!config.info.speed) { + ret = fdt_read_uint32(fdt, node, "st,mem-speed", &config.info.speed); + if (ret < 0) { VERBOSE("%s: no st,mem-speed\n", __func__); return -EINVAL; } - config.info.size = fdt_read_uint32_default(node, "st,mem-size", 0); - if (!config.info.size) { + ret = fdt_read_uint32(fdt, node, "st,mem-size", &config.info.size); + if (ret < 0) { VERBOSE("%s: no st,mem-size\n", __func__); return -EINVAL; } diff --git a/include/common/fdt_wrappers.h b/include/common/fdt_wrappers.h index e28dee142..852fe1bf8 100644 --- a/include/common/fdt_wrappers.h +++ b/include/common/fdt_wrappers.h @@ -14,6 +14,8 @@ int fdt_read_uint32(const void *dtb, int node, const char *prop_name, uint32_t *value); +uint32_t fdt_read_uint32_default(const void *dtb, int node, + const char *prop_name, uint32_t dflt_value); int fdt_read_uint64(const void *dtb, int node, const char *prop_name, uint64_t *value); int fdt_read_uint32_array(const void *dtb, int node, const char *prop_name, diff --git a/plat/st/common/include/stm32mp_dt.h b/plat/st/common/include/stm32mp_dt.h index 92f3d6528..91a8d67c8 100644 --- a/plat/st/common/include/stm32mp_dt.h +++ b/plat/st/common/include/stm32mp_dt.h @@ -28,8 +28,6 @@ int dt_open_and_check(void); int fdt_get_address(void **fdt_addr); bool fdt_check_node(int node); uint8_t fdt_get_status(int node); -uint32_t fdt_read_uint32_default(int node, const char *prop_name, - uint32_t dflt_value); int fdt_get_reg_props_by_name(int node, const char *name, uintptr_t *base, size_t *size); int dt_set_stdout_pinctrl(void); diff --git a/plat/st/common/stm32mp_dt.c b/plat/st/common/stm32mp_dt.c index 6b76424ae..c76b03358 100644 --- a/plat/st/common/stm32mp_dt.c +++ b/plat/st/common/stm32mp_dt.c @@ -135,26 +135,6 @@ static int fdt_get_node_parent_size_cells(int node) } #endif -/******************************************************************************* - * This function reads a value of a node property (generic use of fdt - * library). - * Returns value if success, and a default value if property not found. - * Default value is passed as parameter. - ******************************************************************************/ -uint32_t fdt_read_uint32_default(int node, const char *prop_name, - uint32_t dflt_value) -{ - const fdt32_t *cuint; - int lenp; - - cuint = fdt_getprop(fdt, node, prop_name, &lenp); - if (cuint == NULL) { - return dflt_value; - } - - return fdt32_to_cpu(*cuint); -} - /******************************************************************************* * This function fills reg node info (base & size) with an index found by * checking the reg-names node. @@ -343,7 +323,7 @@ uint32_t dt_get_ddr_size(void) return 0; } - return fdt_read_uint32_default(node, "st,mem-size", 0); + return fdt_read_uint32_default(fdt, node, "st,mem-size", 0); } /******************************************************************************* From 364ad245a24efce0732245f050a80b9b702fc7b2 Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Thu, 26 Mar 2020 11:57:43 +0000 Subject: [PATCH 5/5] arm: fconf: Fix GICv3 dynamic configuration At the moment the fconf_populate_gicv3_config() implementation is somewhat incomplete: First it actually fails to store the retrieved information (the local addr[] array is going nowhere), but also it makes quite some assumptions about the device tree passed to it: it needs to use two address-cells and two size-cells, and also requires all five register regions to be specified, where actually only the first two are mandatory according to the binding (and needed by our code). Fix this by introducing a proper generic function to retrieve "reg" property information from a DT node: We retrieve the #address-cells and #size-cells properties from the parent node, then use those to extract the right values from the "reg" property. The function takes an index to select one region of a reg property. This is loosely based on the STM32 implementation using "reg-names", which we will subsume in a follow-up patch. Change-Id: Ia59bfdf80aea4e36876c7b6ed4d153e303f482e8 Signed-off-by: Andre Przywara --- common/fdt_wrappers.c | 50 +++++++++++++++++++ include/common/fdt_wrappers.h | 2 + .../board/fvp/fconf/fconf_hw_config_getter.c | 29 ++++++----- .../fvp/include/fconf_hw_config_getter.h | 4 +- plat/arm/board/fvp/jmptbl.i | 3 ++ plat/arm/board/juno/jmptbl.i | 1 + 6 files changed, 75 insertions(+), 14 deletions(-) diff --git a/common/fdt_wrappers.c b/common/fdt_wrappers.c index 57e3bcd40..842d71339 100644 --- a/common/fdt_wrappers.c +++ b/common/fdt_wrappers.c @@ -226,3 +226,53 @@ int fdtw_write_inplace_bytes(void *dtb, int node, const char *prop, return err; } + +static uint64_t fdt_read_prop_cells(const fdt32_t *prop, int nr_cells) +{ + uint64_t reg = fdt32_to_cpu(prop[0]); + + if (nr_cells > 1) { + reg = (reg << 32) | fdt32_to_cpu(prop[1]); + } + + return reg; +} + +int fdt_get_reg_props_by_index(const void *dtb, int node, int index, + uintptr_t *base, size_t *size) +{ + const fdt32_t *prop; + int parent, len; + int ac, sc; + int cell; + + parent = fdt_parent_offset(dtb, node); + if (parent < 0) { + return -FDT_ERR_BADOFFSET; + } + + ac = fdt_address_cells(dtb, parent); + sc = fdt_size_cells(dtb, parent); + + cell = index * (ac + sc); + + prop = fdt_getprop(dtb, node, "reg", &len); + if (prop == NULL) { + WARN("Couldn't find \"reg\" property in dtb\n"); + return -FDT_ERR_NOTFOUND; + } + + if (((cell + ac + sc) * (int)sizeof(uint32_t)) > len) { + return -FDT_ERR_BADVALUE; + } + + if (base != NULL) { + *base = (uintptr_t)fdt_read_prop_cells(&prop[cell], ac); + } + + if (size != NULL) { + *size = (size_t)fdt_read_prop_cells(&prop[cell + ac], sc); + } + + return 0; +} diff --git a/include/common/fdt_wrappers.h b/include/common/fdt_wrappers.h index 852fe1bf8..7a6b59893 100644 --- a/include/common/fdt_wrappers.h +++ b/include/common/fdt_wrappers.h @@ -28,5 +28,7 @@ int fdtw_read_bytes(const void *dtb, int node, const char *prop, unsigned int length, void *value); int fdtw_write_inplace_bytes(void *dtb, int node, const char *prop, unsigned int length, const void *data); +int fdt_get_reg_props_by_index(const void *dtb, int node, int index, + uintptr_t *base, size_t *size); #endif /* FDT_WRAPPERS_H */ diff --git a/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c b/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c index 44e9d0135..f1d9b93f7 100644 --- a/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c +++ b/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c @@ -18,7 +18,7 @@ int fconf_populate_gicv3_config(uintptr_t config) { int err; int node; - uint32_t addr[20]; + uintptr_t addr; /* Necessary to work with libfdt APIs */ const void *hw_config_dtb = (const void *)config; @@ -33,19 +33,24 @@ int fconf_populate_gicv3_config(uintptr_t config) WARN("FCONF: Unable to locate node with arm,gic-v3 compatible property\n"); return 0; } - /* Read the reg cell holding base address of GIC controller modules - A sample reg cell array is shown here: - reg = <0x0 0x2f000000 0 0x10000>, // GICD - <0x0 0x2f100000 0 0x200000>, // GICR - <0x0 0x2c000000 0 0x2000>, // GICC - <0x0 0x2c010000 0 0x2000>, // GICH - <0x0 0x2c02f000 0 0x2000>; // GICV - */ - - err = fdt_read_uint32_array(hw_config_dtb, node, "reg", 20, addr); + /* The GICv3 DT binding holds at least two address/size pairs, + * the first describing the distributor, the second the redistributors. + * See: bindings/interrupt-controller/arm,gic-v3.yaml + */ + err = fdt_get_reg_props_by_index(hw_config_dtb, node, 0, &addr, NULL); if (err < 0) { - ERROR("FCONF: Failed to read reg property of GIC node\n"); + ERROR("FCONF: Failed to read GICD reg property of GIC node\n"); + return err; } + gicv3_config.gicd_base = addr; + + err = fdt_get_reg_props_by_index(hw_config_dtb, node, 1, &addr, NULL); + if (err < 0) { + ERROR("FCONF: Failed to read GICR reg property of GIC node\n"); + } else { + gicv3_config.gicr_base = addr; + } + return err; } diff --git a/plat/arm/board/fvp/include/fconf_hw_config_getter.h b/plat/arm/board/fvp/include/fconf_hw_config_getter.h index cab832f68..a9e569e78 100644 --- a/plat/arm/board/fvp/include/fconf_hw_config_getter.h +++ b/plat/arm/board/fvp/include/fconf_hw_config_getter.h @@ -15,8 +15,8 @@ #define hw_config__topology_getter(prop) soc_topology.prop struct gicv3_config_t { - void *gicd_base; - void *gicr_base; + uintptr_t gicd_base; + uintptr_t gicr_base; }; struct hw_topology_t { diff --git a/plat/arm/board/fvp/jmptbl.i b/plat/arm/board/fvp/jmptbl.i index 6469e2995..50a5ba4d9 100644 --- a/plat/arm/board/fvp/jmptbl.i +++ b/plat/arm/board/fvp/jmptbl.i @@ -25,6 +25,9 @@ fdt fdt_first_subnode fdt fdt_next_subnode fdt fdt_path_offset fdt fdt_subnode_offset +fdt fdt_address_cells +fdt fdt_size_cells +fdt fdt_parent_offset mbedtls mbedtls_asn1_get_alg mbedtls mbedtls_asn1_get_alg_null mbedtls mbedtls_asn1_get_bitstring_null diff --git a/plat/arm/board/juno/jmptbl.i b/plat/arm/board/juno/jmptbl.i index 66d6e4592..eb4399ec0 100644 --- a/plat/arm/board/juno/jmptbl.i +++ b/plat/arm/board/juno/jmptbl.i @@ -23,6 +23,7 @@ fdt fdt_node_offset_by_compatible fdt fdt_setprop_inplace_namelen_partial fdt fdt_first_subnode fdt fdt_next_subnode +fdt fdt_parent_offset mbedtls mbedtls_asn1_get_alg mbedtls mbedtls_asn1_get_alg_null mbedtls mbedtls_asn1_get_bitstring_null