[U-Boot] [PATCH v3 1/2] arm64: zynqmp: spl: install a PMU firmware config object at runtime
Luca Ceresoli
luca at lucaceresoli.net
Sat May 4 16:54:42 UTC 2019
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 <luca at 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 <luca at 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
More information about the U-Boot
mailing list