[SRU] Add support for AMD-Xilinx Kria KD240

Bug #2037407 reported by Talha Can Havadar
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
flash-kernel (Ubuntu)
Status tracked in Noble
Jammy
New
Undecided
Unassigned
Lunar
New
Undecided
Unassigned
Mantic
New
Undecided
Unassigned
Noble
New
Undecided
Unassigned

Bug Description

[Impact]

Add AMD-Xilinx Kria KD240 board support

[Where problems could occur]

The patch adds support for the AMD-Xilinx Kria KD240, but affects the boot script used by:

* ZCU 102, 104, 106, 111
* Kria KR revisions A, B, 1
* Kria KV revisions A, B, Z, 1

[Test Case]

* Enable proposed updates (https://wiki.ubuntu.com/Testing/EnableProposed)
* Update flash-kernel to the version in -proposed: sudo apt install -t jammy-proposed flash-kernel
* Run flash-kernel (if the upgrade didn't already): sudo flash-kernel
* Reboot: sudo reboot
* Ensure system boots successfully

[Regression Potential]

As the patch touches the u-boot script used by several boards (listed above), booting should be re-tested against the potentially affected boards under the proposed package.

Tags: patch
Revision history for this message
Talha Can Havadar (tchavadar) wrote :
tags: added: patch
Revision history for this message
Talha Can Havadar (tchavadar) wrote :

debdiff to the devel

Revision history for this message
Talha Can Havadar (tchavadar) wrote :
Revision history for this message
Andreas Hasenack (ahasenack) wrote (last edit ):

Sorry, I have no idea how this package works, so I can't review the code changes properly, only the syntax of the patch.

This should be reviewed by someone familiar with u-boot, so I'm subscribing Dave Jones to this bug, as he has uploaded this package before.

devel_0.debdiff:
- d/changelog needs to be updated for noble (3.107ubuntu3, and noble release name) Maybe the whole patch could be rebased, as there is an offset in db/all.db
- there is no indication from where these changes are coming from. I know this is a native package, but if there is some upstream repository with PRs and comments about these changes, could they be linked somewhere perhaps? But this is just me asking as someone who is not familiar at all with this package.
- the d/changelog diff is also making changes elsewhere in the file, not just adding an entry on top. That is usually not desirable:
@@ -614,7 +620,7 @@

   * Add riscv64 support (LP: #1966219)
   * Add the following boards to db/all.db
- - SiFive HiFive Unmatched A00
+ - SiFive HiFive Unmatched A00
     - BeagleV Starlight Beta

  -- Heinrich Schuchardt <email address hidden> Tue, 08 Mar 2022 10:05:15 +0100

The other debdiffs also have that stray changelog change.

We will also need a mantic diff, now that noble is open for development already.

You should update the [test case] with what you said in the [regression potential] section, about testing other affected boards.

Revision history for this message
Dave Jones (waveform) wrote :

I've had a look at the devel patch and there's a few things I'm a bit concerned about:

In the new all.db entry the "Machine:" entries both include * wildcards. The all.db database is organized to be in strictly alphabetical order and that means that entries with * wildcards (or really, any accepted wildcard) should never be the first Machine: entry because their sorting order is ambiguous. I realize we've already got several such entries, but I'd really like to see all of these revised to be more like the Pi entries with wildcards, e.g.:

Machine: Raspberry Pi 3 Model B
Machine: Raspberry Pi 3 Model B Rev 1.2
Machine: Raspberry Pi 3 Model B Rev *

Here the first entry has no wildcards and hence can be used to enforce the sort order without ambiguity. They also serve to document the strings that the wildcard is expected to match. In the proposed patch, I'm not sure what sort of strings the "ZynqMP *K24*" and "ZynqMP *KD240*" are going to match.

Secondly, in the revised u-boot script, the support list of Kria boards at the top appears unchanged, but I'd assume "sck-kd-g" should appear in there somewhere?

Continuing with the revised u-boot script, $k24_starter is set to "SMK-K24-" if it's not already in the environment. I'm not entirely clear on what this variable is meant to represent and why the environment's providing it? It would be nice to see this (and, ideally, $card1_name) documented as the "variables expected to be in the environment".

Finally, the patch adds the following test:

  if setexpr model gsub .*$k24_starter.* $model

This doesn't make sense unless $model is also set in the environment. It stands in contrast to the tests immediately following it that rely on $model_test, which is retrieved from the device-tree prior to the tests. Are we sure this is correct?

Leaving as "Incomplete" and unsubscribing ubuntu-sponsors pending correct and clarification of the above.

Revision history for this message
Talha Can Havadar (tchavadar) wrote (last edit ):

Thank you for your time and comments Dave and Andreas, since the flash-kernel has dependency to the kernel and the current kernel for Xilinx boards (which is accessible by public) does not include the dtb files required for KD240 yet. So in anyway we needed to hold this update to be released on flash-kernel until we have the new version of kernel released with correct dtb files.

Meanwhile, I will be working on to resolve the issues you guys mentioned already.

As Dave mentioned, I have updated the mechanism to check the type of the board and added a comment in the header of the bootscript to explain the purpose of `card1_name` variable.

I have updated the all.db entry for Kria family like below;
```
Machine: ZynqMP SMK-K26 Rev1/B/A
Machine: ZynqMP *K26*
Machine: ZynqMP *K24*
Machine: ZynqMP *KD240*
Kernel-Flavors: xilinx-zynqmp
Method: generic
Boot-Script-Path: /boot/firmware/boot.scr.uimg
U-Boot-Script-Name: bootscr.zynqmp
Boot-FIT-Path: /boot/firmware/image.fit
Boot-ITS-File-Name: image-kria.its
Required-Packages: u-boot-tools
```

Is this what you would expect in `Machine` field Dave?

I also have another question, I will prepare a debdiff for mantic and noble as well.

Revision history for this message
Talha Can Havadar (tchavadar) wrote :
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.