Re: [PATCH 4/5] xilinx: zynqmp: add Beckhoff CX8200

[Michal Simek <michal.simek@amd.com>]

On 9/1/23 11:00, Steffen Dirkwinkel wrote:
> 
> 
> Am 01.09.23 um 09:36 schrieb Michal Simek:
>>
>>
>> On 8/30/23 16:03, Steffen Dirkwinkel wrote:
>>> From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
>>>
>>> This adds support for the Beckhoff CX8200 series of industrial embedded PCs.
>>> There is some information about the device and features here:
>>> https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx8200-arm-cortex-a53/
>>>
>>> Currently supported/tested:
>>> - Boot from microSD
>>> - Ethernet
>>> - USB
>>> - rtc / rtc eeprom
>>> - tpm access
>>> - uart
>>>
>>> Open points:
>>> - adding the psgtr usb phy doesn't work in linux (failed to get pll
>>>    lock)
>>> - fpga loading currently only as u-boot script or pre launch cmd (type
>>>    may be stored in eeprom of rtc so this could be made generic)
>>>
>>> Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
>>> ---
>>>
>>>   arch/arm/dts/Makefile                         |    1 +
>>>   arch/arm/dts/zynqmp-beckhoff-cx8200.dts       |  247 +++
>>>   .../zynqmp-beckhoff-cx8200/psu_init_gpl.c     | 1960 +++++++++++++++++
>>>   configs/xilinx_zynqmp_virt_defconfig          |    2 +-
>>>   4 files changed, 2209 insertions(+), 1 deletion(-)
>>>   create mode 100644 arch/arm/dts/zynqmp-beckhoff-cx8200.dts
>>>   create mode 100644 board/xilinx/zynqmp/zynqmp-beckhoff-cx8200/psu_init_gpl.c
>>
>> First of all xilinx folder is not the right location because Xilinx/AMD is not 
>> manufacturer of this board.
> 
> Yeah, sorry. I saw the avnet board and copied that.
> 
>>
>> Second I am normally pushing back on adding these custom boards because it 
>> just increase time for maintaining.
>> Your last commit was in 2019 but at least you have some commits that I can 
>> trust that you would maintain your board for some time.
> 
> Our last board [1] only had linux/u-boot support as an afterthought. Customers 
> had to buy a special option to set the right boot fuses, so most devices don’t 
> boot u-boot.
> With this board and a second similar zynqmp board (CX9240 [2]) we’ll have u-boot 
> as default (and probably only) bootloader, so we’ll be
> more active. We can also set two of us as maintainers and it would be fine to 
> drop the boards if nobody responds.
> We generally support these industrial boards for long time frames and would like 
> to stay close to mainline instead of maintaining forks.
> 
> The alternative would be to have a downstream u-boot repository on github or 
> somewhere. We’ll still likely have something there for build scripts / firmware 
> builds / integration, but don’t plan to really diverge from upstream u-boot.
> 
> The main advantage of being in upstream u-boot would be that we can trigger 
> internal CI on upstream changes. We can still do that and apply patches, but 
> even simple patches like adding files to a makefile may fail to apply and will 
> need fixing.
> 
> 
>>
>> My biggest question is in what category is your board unique that it should be 
>> added it to the tree?
> 
> Currently the board isn’t really unique. I guess we’d be the only users of the 
> rtc with eeprom and there might be something needed for loading the correct fpga 
> file based on eeprom (this might be done in linux / userspace or even u-boot 
> script though)
> 
> [1] 
> https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx9020-arm-cortex-a8/cx9020.html#tab_productdetails_1
> [2] 
> https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx9240-arm-cortex-a53/cx9240.html

Regarding board itself.
It is DTB - we use OF_SEPARATE/OF_BOARD it means building is easy. I expect you 
have pretty much something in PL that's why your DT is bigger if you don't use 
DT overlays but for fixed design there is actually no reason to use it.

psu_init_gpl. You are adding it to specific folder which match DEVICE_TREE 
variable when you use it. You can actually just copy it to board/xilinx/zynqmp/ 
folder and it will be pick up for your build.

xilinx_zynqmp_virt_defconfig - you likely don't want to use this in your product 
because there are things enabled which you don't use on your board. That's why 
you should tune it for your usage.

regs.init - that's for boot.bin generation with SPL - BOOT_INIT_FILE should be 
used and it can't be wired via generic defconfig anyway.

That pretty much leads to the state that make no sense for you to use 
xilinx_zynqmp_virt_defconfig.

I think we are still supporting defconfig fragments which is the way how to 
maintain your board to be close to mainline as possible.

And fdt_addr change. I think this is for me the most problematic part which 
should be solved. Pretty much all these variables should be moved out and that's 
what we started to work on. But it will take some time to get there.

Definitely there shouldn't be a problem to merge that rtc driver and I would 
prefer if you can stay with your board out of upstream and try to maintain it 
via your build scripts only. And let me know if we can improve/fix something to 
be able to do it in long term. But pretty much all things should be in place 
already to do it without pain. At least I would like you to try it and see if 
there is any major blocker which we can try to fix.

Thanks,
Michal