[U-Boot] [RFC v2 2/2] arm64: zynqmp: spl: install a PMU firmware config object at runtime

[Mike Looijmans]

On 03-04-19 23:18, Luca Ceresoli wrote:
> Hi Mike,
> 
> On 03/04/19 13:24, Mike Looijmans wrote:
>> On 29-03-19 13:22, Luca Ceresoli wrote:
>>> Hi Michal,
>>>
>>> thanks for the feedback.
>>>
>>> On 27/03/19 16:03, Michal Simek wrote:
>>>> On 21. 03. 19 16:48, 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
>>>>>
>>>>> The load is logged on the console during boot:
>>>>>
>>>>>     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>
>>> [...]
>>>>> diff --git a/board/xilinx/zynqmp/pmu_ipc.c b/board/xilinx/zynqmp/pmu_ipc.c
>>>>> new file mode 100644
>>>>> index 000000000000..6306d33d6f17
>>>>> --- /dev/null
>>>>> +++ b/board/xilinx/zynqmp/pmu_ipc.c
>>>>> @@ -0,0 +1,116 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/*
>>>>> + * Inter-Processor Communication with the Platform Management Unit (PMU)
>>>>> + * firmware.
>>> [...]
>>>>> +static int pmu_ipc_request(const u32 *req, size_t req_len,
>>>>> +			   u32 *res, size_t res_maxlen)
>>>>> +{
>>>>> +	u32 status;
>>>>> +
>>>>> +	if (req_len > PMUFW_PAYLOAD_ARG_CNT ||
>>>>> +	    res_maxlen > PMUFW_PAYLOAD_ARG_CNT)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	pmu_ipc_send_request(req, req_len);
>>>>> +
>>>>> +	/* Raise Inter-Processor Interrupt to PMU and wait for response */
>>>>> +	writel(IPI_BIT_MASK_PMU0, IPI_REG_BASE_APU + IPI_REG_OFFSET_TRIG);
>>>>> +	do {
>>>>> +		status = readl(IPI_REG_BASE_APU + IPI_REG_OFFSET_OBR);
>>>>> +	} while (status & IPI_BIT_MASK_PMU0);
>>>>> +
>>>>> +	pmu_ipc_read_response(res, res_maxlen);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>
>>>> All above should be mailbox driver. It means this should go to
>>>> drivers/mailbox and be split to mbox send/recv functions.
>>>
>>> Oh, wow, there's a mailbox uclass! I'll have a look into it.
>>>
>>>> But I have no problem to use this configuration in the first patch and
>>>> move to mbox driver in separate patch.
>>>
>>> Good to know, I'll use this option in case it takes too long to make it
>>> a proper mailbox driver.
>>>
>>>>> +/**
>>>>> + * Send a configuration object to the PMU firmware.
>>>>> + *
>>>>> + * @cfg_obj Pointer to the configuration object
>>>>> + * @size    Size of @cfg_obj in bytes
>>>>> + */
>>>>> +void zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size)
>>>>> +{
>>>>> +	const u32 *ocm = (u32 *)CONFIG_SPL_TEXT_BASE;
>>>>> +	const u32 request[] = {
>>>>> +		PMUFW_CMD_SET_CONFIGURATION,
>>>>> +		CONFIG_SPL_TEXT_BASE
>>>>> +	};
>>>>> +	u32 response;
>>>>> +	int err;
>>>>> +
>>>>> +	printf("Loading PMUFW cfg obj (%ld bytes)\n", size);
>>>>> +
>>>>> +	memcpy(ocm, cfg_obj, size);
>>>>> +
>>>>> +	err = pmu_ipc_request(request,  ARRAY_SIZE(request), &response, 1);
>>>>> +	if (err)
>>>>> +		panic("Cannot load PMUFW configuration object (%d)\n", err);
>>>>> +	if (response != 0)
>>>>> +		panic("PMUFW returned 0x%08x status!\n", response);
>>>>> +}
>>>>
>>>> And this can stay here or go to arch/arm/mach-zynq/
>>>
>>> Ok, I'll move it to arch/arm/mach-zynq/pmu.c or so.
>>>
>>> I assume "zynq" here means the whole zynq family, including zynqmp.
>>>
>>>>> diff --git a/board/xilinx/zynqmp/pmu_ipc.h b/board/xilinx/zynqmp/pmu_ipc.h
>>>>> new file mode 100644
>>>>> index 000000000000..37bb72c1b20a
>>>>> --- /dev/null
>>>>> +++ b/board/xilinx/zynqmp/pmu_ipc.h
>>>>> @@ -0,0 +1,14 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>>>> +/*
>>>>> + * (C) Copyright 2019 Luca Ceresoli
>>>>> + * Luca Ceresoli <luca at lucaceresoli.net>
>>>>> + */
>>>>> +
>>>>> +#ifndef __ZYNQMP_PMU_IPC_H__
>>>>> +#define __ZYNQMP_PMU_IPC_H__
>>>>> +
>>>>> +#include <linux/types.h>
>>>>> +
>>>>> +void zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size);
>>>>> +
>>>>
>>>>
>>>> arch/arm/mach-zynqmp/include/mach/sys_proto.h should be fine.
>>>
>>> Ok, but I guess you mean s/zynqmp/zynq/, as above.
>>>
>>>>> +#endif /* __ZYNQMP_PMU_IPC_H__ */
>>>>> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
>>>>> index 5e1d2116bc32..1d5e25961863 100644
>>>>> --- a/board/xilinx/zynqmp/zynqmp.c
>>>>> +++ b/board/xilinx/zynqmp/zynqmp.c
>>>>> @@ -4,6 +4,8 @@
>>>>>     * Michal Simek <michal.simek at xilinx.com>
>>>>>     */
>>>>>    
>>>>> +#include "pmu_ipc.h"
>>>>> +
>>>>>    #include <common.h>
>>>>>    #include <sata.h>
>>>>>    #include <ahci.h>
>>>>> @@ -302,6 +304,10 @@ static char *zynqmp_get_silicon_idcode_name(void)
>>>>>    }
>>>>>    #endif
>>>>>    
>>>>> +#ifdef ZYNQMP_LOAD_PM_CFG_OBJ
>>>>> +#include CONFIG_ZYNQMP_LOAD_PM_CFG_OBJ_FILE
>>>>> +#endif
>>>>> +
>>>>>    int board_early_init_f(void)
>>>>>    {
>>>>>    	int ret = 0;
>>>>> @@ -332,6 +338,11 @@ int board_early_init_f(void)
>>>>>    
>>>>>    int board_init(void)
>>>>>    {
>>>>> +#if defined(CONFIG_SPL_BUILD) && defined(ZYNQMP_LOAD_PM_CFG_OBJ)
>>>>> +	zynqmp_pmufw_load_config_object(XPm_ConfigObject,
>>>>> +					sizeof(XPm_ConfigObject));
>>>>> +#endif
>>>>
>>>> As we discussed over IRC. I think that this should be simply bin
>>>> firmware file compare to C built by u-boot.
>>>
>>> Sure. I have a working prototype that uses a binary blob. It still needs
>>> a decent way to produce a blob and to be updated according to your review.
>>
>> It should be doable to write a Python script to parse the C file and create an
>> equivalent binary (using "struct" module) which is just an array of integers
>> in the end. That avoids the need for a microblaze C compiler...
> 
> There's no need for a microblaze compiler. pm_cfg_obj.c is simply
> declaring a u32 array and some #defines, any C compiler is enough.
> 
> That said, my current solution (a trivial main.c that compiles the u32
> array and outputs it to a binary file) is not nice at all, and it
> requires a pm_defs.h file.
> 
> The python script you mention looks way better from a user perspective,
> although the parsing might be a bit fragile. I'll consider it, thanks
> for the suggestion.
> 
> In the past I even prototyped a python script that parses the Vivado
> .xpr project file and produces a pm_cfg_obj.c. It avoided the need to
> run the Xilinx XSDK just to generate pm_cfg_obj.c. It might also be
> extended to produce a .bin directly, or a self-standing .c file that
> doesn't need pm_defs.h, thus removing any licensing issue. But it never
> grew complete to handle all cases. Obvious, since *I* don't even know
> all of the cases. :)


Another approach would be to simply create and include a "god mode" config 
object that just allows access to all periferals. As far as I can see, such a 
config object would just work on all boards. There's nothing really board 
specific in the config object, and it's rather lame anyway to have to go and 
compile a new bootloader just because you want to use a SPI controller...

The config object I compile into the pmu firmware is of that type anyway.