Creating FIT images is not supported

[Alfonso Sanchez-Beato]

flash-kernel does currently not support creating FIT images method. The attached debdiff (for impish) supports that, and additionally add supports for the Kria SOM platform.

[Loïc Minier (lool)]

thanks for the patch!

this might be easier to review as smaller atomic changes, but perhaps I can still follow around the debdiff :)

1) The its files are data, so they should to /usr/share; I wouldn't encourage local customizations by dropping them in /etc unless that's an important function; a way to give control would be to install a local flash-kernel database entry that overrides the shipped defaults for that hardware; this should be possible with /etc/flash-kernel/db?

2) would you add the new Boot-FIT-Path and Boot-ITS-Path to README?

3) seems "Boot-ITS-Path: /.../foo.its" should be written "U-Boot-ITS-Name: foo.its" and should be searched in the default flash-kernel directory

4) would it be possible to avoid the cp calls for kernel and initrd and just generate the fit image from their locations? the kernels / initrds tend to be big, so I suspect this adds non-trivial time to the run on SD cards

5) mkimage_fit(): why is padding needed? perhaps deserves a comment. Should this be a config?

6) the full path name in DTB-Id is the first one in the database, perhaps this suggests the corresponding deb should be in Required-Packages? would help with d-i usage

6) "cd" looks pretty singular compared to rest of functions; could it be avoided?

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

On Wed, Jun 9, 2021 at 1:55 AM Loïc Minier <email address hidden>
wrote:

> thanks for the patch!
>

Thanks for the review!

>
> this might be easier to review as smaller atomic changes, but perhaps I
> can still follow around the debdiff :)
>

I have this branch based on the auto-imported ubuntu repos (focal branch)
that probably will make your review easier:
https://code.launchpad.net/~alfonsosanchezbeato/ubuntu/+source/flash-kernel/+git/flash-kernel/+ref/kria-support
There is now a commit with the changes you have requested. I have also
created an MP, purely to help with the review (obviously not for merging to
the auto-imported repo):
https://code.launchpad.net/~alfonsosanchezbeato/ubuntu/+source/flash-kernel/+git/flash-kernel/+merge/403942
When things are ready, I will create a new debdiff for impish.

>
> 1) The its files are data, so they should to /usr/share; I wouldn't
> encourage local customizations by dropping them in /etc unless that's an
> important function; a way to give control would be to install a local
> flash-kernel database entry that overrides the shipped defaults for that
> hardware; this should be possible with /etc/flash-kernel/db?
>

I actually fully agree with you, the reason that I've put the "its" file
under /etc is just to be coherent as that's where the boot script files are
stored (/etc/flash-kernel/bootscript/) too. And I think that /usr/share/
would be a better place for those files as well.

So, should I put the "its" file in /usr/share and maybe move the bootscript
files with another debdiff?

>
> 2) would you add the new Boot-FIT-Path and Boot-ITS-Path to README?
>

Done.

>
> 3) seems "Boot-ITS-Path: /.../foo.its" should be written "U-Boot-ITS-
> Name: foo.its" and should be searched in the default flash-kernel
> directory
>

Done.

>
> 4) would it be possible to avoid the cp calls for kernel and initrd and
> just generate the fit image from their locations? the kernels / initrds
> tend to be big, so I suspect this adds non-trivial time to the run on SD
> cards
>

I have now changed things to use macros in the "its" files (@@XXX@@) and
replace them with the file names using sed, so no copy is necessary anymore.

>
> 5) mkimage_fit(): why is padding needed? perhaps deserves a comment.
> Should this be a config?
>

Added Mkimage-FIT-Options option now.

>
> 6) the full path name in DTB-Id is the first one in the database,
> perhaps this suggests the corresponding deb should be in Required-
> Packages? would help with d-i usage
>

I've added xlnx-kria-firmware to Required-Packages.

>
> 6) "cd" looks pretty singular compared to rest of functions; could it be
> avoided?
>

The problem here was that mkimage was searching the files defined in the
.its in the current folder, but with the macros and using absolute paths
this is not needed anymore.

>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1931278
>
> Title:
> Creating FIT images is not supported
>
> Status in flash-kernel package in Ubuntu:
> New
>
> Bug description:
> flash-kernel does currently not support creating FIT images method.
> The attached debdi...

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

@lool I have pushed some more commits to the branch/MP:

* Move .its files to /usr/share/flash-kernel/its
* Remove Mkimage-FIT-Options field (as you suggested, it was not really needed)
* Make sure temp FIT file lives in same filesystem as final (so mv can be "more atomic")

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

@lool I have updated again the branch/MP:
* Use generic instead of fit method and remove the latter
* Add parameter to control copy of the dtb file to /boot/
* Add support for ZCU boards

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

New debdiff after the review.

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "flash-kernel.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Fixed in 3.104ubuntu4 in impish (dev).