Re: [Buildroot] [PATCH] configs/digilent_genesys_zu_defconfig: add Digilent Genesys ZU board (ZynqMP SoC)

[Luca Ceresoli <luca@lucaceresoli.net>]

Hi Alvaro,

I'm replying here to topics that apply also for v2. I'm sending a
separate review for the rest.

On 03/08/21 10:19, Alvaro Gamez wrote:
> Hi Luca
> 
> El vie, 30 jul 2021 a las 0:21, Luca Ceresoli
> (<luca@lucaceresoli.net>) escribió:
>>
>> Hi Alvaro,
>>
>> thanks for your patch.
>>
>> And apologies for the late reply -- vacation time.
> 
> Don't worry, we're all busy! Thanks for your review.
> 
>> On 15/07/21 08:28, Alvaro Gamez Machado wrote:
>>> This adds support the Digilent Genesys ZU development board.
>>>
>>> Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
>>> ---
>>>  ...ve-redundant-YYLOC-global-declaratio.patch |    52 +
>>>  .../0001-uboot-add-genesys-zu.patch           |    10 +
>>>  board/digilent/genesys-zu/GenesysZU.dts       |  1655 +
>>>  board/digilent/genesys-zu/README.txt          |    92 +
>>>  board/digilent/genesys-zu/genimage.cfg        |    28 +
>>>  board/digilent/genesys-zu/image.its           |    59 +
>>>  board/digilent/genesys-zu/kernel_defconfig    |   414 +
>>>  board/digilent/genesys-zu/pm_cfg_obj.c        |   630 +
>>>  board/digilent/genesys-zu/post-image.sh       |    10 +
>>>  board/digilent/genesys-zu/psu_init_gpl.c      | 21818 +++++++++
>>>  board/digilent/genesys-zu/psu_init_gpl.h      | 37545 ++++++++++++++++
>>
>> These files are huge! You should use the
>> tools/zynqmp_psu_init_minimize.sh tool in the U-Boot sources and submit
>> for Buildroot the reduced files.
> 
> I didn't even know that thing existed, I will surely use it.
> My last patch got rejected by the mailing list due to the big size.
> 
> 
>>> diff --git a/board/digilent/genesys-zu/0001-scripts-dtc-Remove-redundant-YYLOC-global-declaratio.patch b/board/digilent/genesys-zu/0001-scripts-dtc-Remove-redundant-YYLOC-global-declaratio.patch
>>> new file mode 100644
>>> index 0000000000..48f683afbe
>>> --- /dev/null
>>> +++ b/board/digilent/genesys-zu/0001-scripts-dtc-Remove-redundant-YYLOC-global-declaratio.patch
>>> @@ -0,0 +1,52 @@
>>> +From e33a814e772cdc36436c8c188d8c42d019fda639 Mon Sep 17 00:00:00 2001
>>> +From: Dirk Mueller <dmueller@suse.com>
>>> +Date: Tue, 14 Jan 2020 18:53:41 +0100
>>> +Subject: [PATCH] scripts/dtc: Remove redundant YYLOC global declaration
>>> +
>>> +gcc 10 will default to -fno-common, which causes this error at link
>>> +time:
>>> +
>>> +  (.text+0x0): multiple definition of `yylloc'; dtc-lexer.lex.o (symbol from plugin):(.text+0x0): first defined here
>>> +
>>> +This is because both dtc-lexer as well as dtc-parser define the same
>>> +global symbol yyloc. Before with -fcommon those were merged into one
>>> +defintion. The proper solution would be to to mark this as "extern",
>>> +however that leads to:
>>> +
>>> +  dtc-lexer.l:26:16: error: redundant redeclaration of 'yylloc' [-Werror=redundant-decls]
>>> +   26 | extern YYLTYPE yylloc;
>>> +      |                ^~~~~~
>>> +In file included from dtc-lexer.l:24:
>>> +dtc-parser.tab.h:127:16: note: previous declaration of 'yylloc' was here
>>> +  127 | extern YYLTYPE yylloc;
>>> +      |                ^~~~~~
>>> +cc1: all warnings being treated as errors
>>> +
>>> +which means the declaration is completely redundant and can just be
>>> +dropped.
>>> +
>>> +Signed-off-by: Dirk Mueller <dmueller@suse.com>
>>> +Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> +[robh: cherry-pick from upstream]
>>> +Cc: stable@vger.kernel.org
>>> +Signed-off-by: Rob Herring <robh@kernel.org>
>>> +Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
>>
>> If this patch comes from upstream it would be nice to add an URL of
>> where it can be found.
> 
> It's just a linux kernel patch, the one referenced on the first line:
> From e33a814e772cdc36436c8c188d8c42d019fda639 Mon Sep 17 00:00:00 2001
> 
> I can clarify it somewhere else, but where? Just below the --- commit
> line is ok?
> 
>>
>>> diff --git a/board/digilent/genesys-zu/GenesysZU.dts b/board/digilent/genesys-zu/GenesysZU.dts
>>> new file mode 100644
>>> index 0000000000..ddeba4b715
>>> --- /dev/null
>>> +++ b/board/digilent/genesys-zu/GenesysZU.dts
>>
>> Do you plan to submit this DTS to mainline Linux? It would be nice to do
>> it and, in that process, have it properly reviewed. After that it can be
>> removed from Buildroot.
> 
> I really should, but this DTS won't be accepted as is. The proper way
> would be removing
> most of it and just use #include "zynqmp.dtsi". If I have enough time
> this century I really will.

You should _really_ do it in v3. Come on, it's not going to take that
much time! ;)

>>> diff --git a/board/digilent/genesys-zu/uboot_defconfig b/board/digilent/genesys-zu/uboot_defconfig
>>> new file mode 100644
>>> index 0000000000..f22856e184
>>> --- /dev/null
>>> +++ b/board/digilent/genesys-zu/uboot_defconfig
>>
>> As above: any plan to submit this to mainline U-Boot?
> 
> And the answer is yet the same... This defconfig is also not
> acceptable as is for mainline, so
> it would require more work.

Another question is: does any of the defconfigs provided in U-Boot work
for your board? Why not?

-- 
Luca

_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot