[U-Boot] [PATCH v3 1/2] arm64: zynqmp: spl: install a PMU firmware config object at runtime

[Luca Ceresoli]

Hi,

On 06/05/19 17:56, Michal Simek wrote:
> Hi,
> 
> snip.
> 
>>>> +$(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.
> 
> Sorry I meant new config option. It should be enough to use CFG_OBJ_FILE
> everywhere and not create another config option which is just used the
> same way.

I totally agree. But I would need to check whether
CONFIG_ZYNQMP_LOAD_PM_CFG_OBJ_FILE is empty or not, and I don't think
the C preprocessor can do that.

>>>> 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.
> 
> Is checkpatch ok with this? I think that it should error out that you
> shouldn't put externs to .c files.

Uhm, no, checkpatch is not ok with this. I disagree, but for sake of
peace in the world I'll move it to a .h file.

-- 
Luca