Hi Michal, thanks for your review. See my replies below.
On 04/05/19 00:38, Michal Simek wrote: > On 15. 04. 19 9:47, Luca Ceresoli wrote: >> Optionally allow U-Boot to load at the PMU firmware configuration object >> into the Power Management Unit (PMU) on Xilinx ZynqMP. >> >> The configuration object is required by the PMU FW to enable most SoC >> peripherals. So far the only way to boot using U-Boot SPL was to hard-code >> the configuration object in the PMU firmware. Allow a different boot >> process, where the PMU FW is equal for any ZynqMP chip and its >> configuration is passed at runtime by U-Boot SPL. >> >> All the code for Inter-processor communication with the PMU is isolated in >> a new file (pmu_ipc.c). The code is inspired by the same feature as >> implemented in the Xilinx First Stage Bootloader (FSBL) and Arm Trusted >> Firmware: >> >> * >> https://github.com/Xilinx/embeddedsw/blob/fb647e6b4c00f5154eba52a88b948195b6f1dc2b/lib/sw_apps/zynqmp_fsbl/src/xfsbl_misc_drivers.c#L295 >> * >> https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/plat/xilinx/zynqmp/pm_service/pm_api_sys.c#L357 >> >> SPL logs on the console before loading the configuration object: >> >> U-Boot SPL 2018.01 (Mar 20 2019 - 08:12:21) >> Loading PMUFW cfg obj (2008 bytes) >> EL Level: EL3 >> ... >> >> Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net> >> >> --- >> >> Changes RFC v2 -> v3: >> - don't compile pm_cfg_obj.c from source, load it from a binary file >> (suggested by Michal) >> - move IPC code to arch/arm/mach-zynqmp/ and sys_proto.h (Michal) >> >> Changes RFC v1 -> RFC v2: >> - Load the cfg_obj in SPL, not U-Boot proper: this required a complete >> reimplementation since we cannot rely on ATF now >> - Update and refine the Kconfig option help >> --- >> arch/arm/mach-zynqmp/Kconfig | 17 +++ >> arch/arm/mach-zynqmp/Makefile | 4 + >> arch/arm/mach-zynqmp/include/mach/sys_proto.h | 4 + >> arch/arm/mach-zynqmp/pmu_ipc.c | 112 ++++++++++++++++++ >> board/xilinx/zynqmp/Makefile | 12 ++ >> board/xilinx/zynqmp/pm_cfg_obj.S | 17 +++ >> board/xilinx/zynqmp/zynqmp.c | 8 ++ >> 7 files changed, 174 insertions(+) >> create mode 100644 arch/arm/mach-zynqmp/pmu_ipc.c >> create mode 100644 board/xilinx/zynqmp/pm_cfg_obj.S >> >> diff --git a/arch/arm/mach-zynqmp/Kconfig b/arch/arm/mach-zynqmp/Kconfig >> index 9bb5a5c20201..b88d1d313839 100644 >> --- a/arch/arm/mach-zynqmp/Kconfig >> +++ b/arch/arm/mach-zynqmp/Kconfig >> @@ -65,6 +65,23 @@ config PMUFW_INIT_FILE >> Include external PMUFW (Platform Management Unit FirmWare) to >> a Xilinx bootable image (boot.bin). >> >> +config ZYNQMP_LOAD_PM_CFG_OBJ_FILE >> + string "PMU firmware configuration object to load at runtime" >> + help >> + > > remove this empty line. OK. >> diff --git a/arch/arm/mach-zynqmp/include/mach/sys_proto.h >> b/arch/arm/mach-zynqmp/include/mach/sys_proto.h >> index 385c8825f2f6..e2b9a79ed539 100644 >> --- a/arch/arm/mach-zynqmp/include/mach/sys_proto.h >> +++ b/arch/arm/mach-zynqmp/include/mach/sys_proto.h >> @@ -72,4 +72,8 @@ int chip_id(unsigned char id); >> void tcm_init(u8 mode); >> #endif >> >> +#if defined(CONFIG_SPL_BUILD) && defined(ZYNQMP_LOAD_PM_CFG_OBJ) >> +void zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size); >> +#endif > > nit: you can remove that if/endif to open a way to also pass different > configuration object from full u-boot. Good idea. Less #ifdefs is always good as well. >> diff --git a/arch/arm/mach-zynqmp/pmu_ipc.c b/arch/arm/mach-zynqmp/pmu_ipc.c >> new file mode 100644 >> index 000000000000..5feb8568f8de >> --- /dev/null >> +++ b/arch/arm/mach-zynqmp/pmu_ipc.c >> @@ -0,0 +1,112 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Inter-Processor Communication with the Platform Management Unit (PMU) >> + * firmware. >> + * >> + * (C) Copyright 2019 Luca Ceresoli >> + * Luca Ceresoli <l...@lucaceresoli.net> >> + */ >> + >> +#include <common.h> >> +#include <asm/io.h> >> +#include <asm/arch/sys_proto.h> >> + >> +/* IPI bitmasks, register base and register offsets */ >> +#define IPI_BIT_MASK_APU 0x00001 >> +#define IPI_BIT_MASK_PMU0 0x10000 >> +#define IPI_REG_BASE_APU 0xFF300000 >> +#define IPI_REG_BASE_PMU0 0xFF330000 >> +#define IPI_REG_OFFSET_TRIG 0x00 >> +#define IPI_REG_OFFSET_OBR 0x04 >> + >> +/* IPI mailbox buffer offsets */ >> +#define IPI_BUF_BASE_APU 0xFF990400 >> +#define IPI_BUF_OFFSET_TARGET_PMU 0x1C0 >> +#define IPI_BUF_OFFSET_REQ 0x00 >> +#define IPI_BUF_OFFSET_RESP 0x20 >> + >> +#define PMUFW_PAYLOAD_ARG_CNT 8 >> + >> +/* PMUFW commands */ >> +#define PMUFW_CMD_SET_CONFIGURATION 2 >> + >> +static void pmu_ipc_send_request(const u32 *req, size_t req_len) >> +{ >> + u32 *mbx = IPI_BUF_BASE_APU + >> + IPI_BUF_OFFSET_TARGET_PMU + >> + IPI_BUF_OFFSET_REQ; >> + size_t i; >> + >> + for (i = 0; i < req_len; i++) >> + writel(req[i], &mbx[i]); >> +} >> + >> +static void pmu_ipc_read_response(unsigned int *value, size_t count) >> +{ >> + u32 *mbx = IPI_BUF_BASE_APU + >> + IPI_BUF_OFFSET_TARGET_PMU + >> + IPI_BUF_OFFSET_RESP; >> + size_t i; >> + >> + for (i = 0; i < count; i++) >> + value[i] = readl(&mbx[i]); >> +} >> + >> +/** > > This suggest kernel-doc format but it is not. Please run kernel-doc and > check this file. Will do. >> diff --git a/board/xilinx/zynqmp/Makefile b/board/xilinx/zynqmp/Makefile >> index 80f8ca7e1e4b..d7a3cb244521 100644 >> --- a/board/xilinx/zynqmp/Makefile >> +++ b/board/xilinx/zynqmp/Makefile >> @@ -33,6 +33,18 @@ ifneq ($(call ifdef_any_of, >> CONFIG_ZYNQMP_PSU_INIT_ENABLED CONFIG_SPL_BUILD),) >> obj-y += $(init-objs) >> endif >> >> +ifneq ($(CONFIG_ZYNQMP_LOAD_PM_CFG_OBJ_FILE),"") >> +PM_CFG_OBJ_FILE := $(shell cd $(srctree); readlink -f >> $(CONFIG_ZYNQMP_LOAD_PM_CFG_OBJ_FILE)) >> + >> +$(obj)/pm_cfg_obj.bin: $(PM_CFG_OBJ_FILE) FORCE >> + $(call if_changed,shipped) > > What's the reason to copying it? Perhaps none. Will check and fix. >> +$(obj)/pm_cfg_obj.o: $(obj)/pm_cfg_obj.bin >> + >> +CFLAGS_zynqmp.o += -DZYNQMP_LOAD_PM_CFG_OBJ > > I am no fan of passing another object. you have > CONFIG_ZYNQMP_LOAD_PM_CFG_OBJ_FILE already and this can be used instead. Not sure I got your point here. I'm not passing an object, just setting a define (without value). This is used to enable code under #ifdef in C files. >> +obj-$(CONFIG_SPL_BUILD) += pm_cfg_obj.o >> +endif >> + >> obj-$(CONFIG_MMC_SDHCI_ZYNQ) += tap_delays.o >> >> ifndef CONFIG_SPL_BUILD ... >> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c >> index db272478506f..7fcb3e120688 100644 >> --- a/board/xilinx/zynqmp/zynqmp.c >> +++ b/board/xilinx/zynqmp/zynqmp.c >> @@ -327,6 +327,14 @@ int board_early_init_f(void) >> >> int board_init(void) >> { >> +#if defined(CONFIG_SPL_BUILD) && defined(ZYNQMP_LOAD_PM_CFG_OBJ) >> + extern const u32 zynqmp_pm_cfg_obj[]; >> + extern const int zynqmp_pm_cfg_obj_size; > > please put these two to header instead. This was done on purpose to reduce the amount of #ifdefs, and also to not pollute the namespace with two symbols that are not needed outside this function. I don't see the added value of moving them in a .h, but I might be wrong. > Anyway we are almost there. I have tested it on HW and it works. > When this is cleanup I think this should also go to zynqmp pmufw command > to be able to change it at run time directly from u-boot prompt. Sure. Also moving to a mailbox uclass driver is on my todo list. But this is progressing in spare time, so it's all probably going to happen after this patch is accepted. Ok for you? -- Luca _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot