Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for TI AM62x Starter Kit EVM (SK-AM62B-P1) board on Common Torizon #215

Open
wants to merge 4 commits into
base: scarthgap-7.x.y
Choose a base branch
from

Conversation

lucas-akira
Copy link
Contributor

@lucas-akira lucas-akira commented Jan 28, 2025

Add initial Torizon OS support for the AM62x Starter Kit Evaluation Module via Common Torizon distro, using the TI BSP. In addition to this layer (meta-toradex-torizon), the following Yocto layers are necessary in order to build the image:

  • meta-virtualization
  • meta-networking
  • meta-python
  • meta-oe
  • meta-filesystems
  • meta-ti-extras
  • meta-ti-bsp
  • meta-arm
  • meta-arm-toolchain
  • meta
  • meta-poky
  • meta-yocto-bsp
  • meta-updater

DISTRO should be set to common-torizon
MACHINE should be set to am62xx-evm

In conf/local.conf append the following line at the end of the file:

include conf/machine/include/am62xx-evm.inc

Build the image with:

bitbake torizon-docker

Copy link
Collaborator

@jsrc27 jsrc27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments/questions.

@@ -0,0 +1,228 @@
kernel_image_type=@@KERNEL_IMAGETYPE@@
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking is the only difference in this file and the normal uEnv.txt.in file the otaroot and the board_fixups?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otaroot is set to 2 instead of 1, and I've changed board_fixups so that fdtfile gets set with the DTB name without the ti/ prefix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, for the sake of maintenance, I think it would be better to patch the default uEnv.txt from meta-toradex-torizon rather than duplicate the whole file here. Opinions?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah my initial questions was kinda touching on this since uEnv.txt is rather large and it is a little clunky to copy it over if the differences are minimal.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should add a "replace variable" to this uEnv.txt, so that we can easily add to the environment through the recipe!

But this could also be a technical debt on this PR, with a ticket on our backlog (at the top!) for us to fix this and not bloat too much the PR. What you guys thing, @jsrc27, @rborn-tx?

Copy link
Contributor

@rborn-tx rborn-tx Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do it right now since we've already decided to do something similar with the boot script (on the other thread).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I've followed the suggestion to patch uEnv.txt in my latest PR update. For board_fixups I created a new variable called extra_board_fixups to add the am62xx-evm-specific DTB fixup.

conf/machine/include/am62xx-evm.inc Show resolved Hide resolved
EdTheBearded
EdTheBearded previously approved these changes Jan 30, 2025
@EdTheBearded EdTheBearded requested a review from jsrc27 January 30, 2025 12:12
Copy link
Contributor

@rborn-tx rborn-tx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. I've have raised a couple of questions before approving it though.

Comment on lines 25 to 30
if test -n "${loadaddr}"
then
ext4load ${devtype} ${devnum}:2 ${loadaddr} /boot/loader/uEnv.txt; env import -t ${loadaddr} ${filesize}
else
ext4load ${devtype} ${devnum}:2 ${scriptaddr} /boot/loader/uEnv.txt; env import -t ${scriptaddr} ${filesize}
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of maintenance, I think it would be better to patch the default boot.cmd from meta-toradex-torizon rather than duplicate the whole file here given that the difference is minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, how would the patch be applied? One way I can imagine is patching during the do_compile() function in the u-boot-distro-boot recipe, more or less like this:

UENV_TXT_PARTITION ?= "1"
UENV_TXT_PARTITION:am62xx = "2"
[...]
do_compile() {
    sed -e 's/@@UENV_TXT_PARTITION@@/${UENV_TXT_PARTITION}/' \
        -e 's/@@KERNEL_BOOTCMD@@/${KERNEL_BOOTCMD}/' \
        [...]
        "${WORKDIR}/boot.cmd.in" > boot.cmd
[...]

And we would add the variable to the default boot.cmd.in like this:

if test -n "${loadaddr}"
then
    ext4load ${devtype} ${devnum}:@@UENV_TXT_PARTITION@@ ${loadaddr} /boot/loader/uEnv.txt; env import -t ${loadaddr} ${filesize}
else
    ext4load ${devtype} ${devnum}:@@UENV_TXT_PARTITION@@ ${scriptaddr} /boot/loader/uEnv.txt; env import -t ${scriptaddr} ${filesize}
fi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, that's the idea. As you noted, today both uEnv.txt and boot.cmd are produced by transforming the corresponding input files with sed (work done by do_compile()). Here we would add new replacements to those transformations.

As for the OE variable name I'd suggest something indicating this is the partition number used by U-Boot for booting (maybe UBOOT_BOOT_PARTITION, UBOOT_BOOT_PARTITION_NUMBER or similar).

As a note, if our script used information from distro-boot (variable ${distro_bootpart}), then this replacement wouldn't even be needed and the boot script would be more generic. For reference, see

https://github.com/u-boot/u-boot/blob/ac3eeb154257f422d87c3e4ad5414c350238c3b9/include/config_distro_bootcmd.h#L521.

I'm not sure why we don't do that, but that's a another story...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!
Specially now since we'll have all sorts of devices, having this parameterized seems like a good idea.
And I'd do kinda like you suggested @lucas-akira, with a default value in this recipe, and the actual override done in the machine.inc file, since I don't this this value would be shared with our AM62 device.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, that's the idea. As you noted, today both uEnv.txt and boot.cmd are produced by transforming the corresponding input files with sed (work done by do_compile()). Here we would add new replacements to those transformations.

As for the OE variable name I'd suggest something indicating this is the partition number used by U-Boot for booting (maybe UBOOT_BOOT_PARTITION, UBOOT_BOOT_PARTITION_NUMBER or similar).

@rborn-tx I see. I agree that it makes more sense to just patch the differences with sed, especially given that the recipe already has this logic in place. I'll do this then, along with your suggestion to name the variable UBOOT_BOOT_PARTITION_NUMBER.

and the actual override done in the machine.inc file, since I don't this this value would be shared with our AM62 device.

@EdTheBearded You're right! I'll put the override in the machine.inc file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the commit with the suggestions. @EdTheBearded @rborn-tx please take a look and see if it's OK.

@@ -0,0 +1,228 @@
kernel_image_type=@@KERNEL_IMAGETYPE@@
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, for the sake of maintenance, I think it would be better to patch the default uEnv.txt from meta-toradex-torizon rather than duplicate the whole file here. Opinions?

Add logic to properly install .dtbo files if any are listed in the
KERNEL_DEVICETREE variable. They will be included in
/boot/ostree/torizon-<hash>/dtb/overlays/.

A new variable, KERNEL_DEVICETREE_OVERLAY_BOOT, can be used to specify
which .dtbo files in KERNEL_DEVICETREE will be applied during boot time.

Signed-off-by: Lucas Akira Morishita <[email protected]>
Add initial Torizon OS support for the Texas Instruments AM62x starter
kit evaluation module with PMIC (SK-AM62B-P1).

The resulting .wic image file should be flashed to an SD card and the
EVM should be configured for SD card boot mode.

Related-to: TOR-3714

Signed-off-by: Lucas Akira Morishita <[email protected]>
@@ -18,4 +18,6 @@ BBFILES_DYNAMIC += "\
toradex-bsp-common-layer:${LAYERDIR}/dynamic-layers/meta-toradex-bsp-common/*/*/*.bbappend \
toradex-ti-layer:${LAYERDIR}/dynamic-layers/meta-toradex-ti/*/*/*.bb \
toradex-ti-layer:${LAYERDIR}/dynamic-layers/meta-toradex-ti/*/*/*.bbappend \
meta-ti-bsp:${LAYERDIR}/dynamic-layers/meta-ti-bsp/*/*/*.bb \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, this is really minor, but could we order these? 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? You mean put the meta-ti-bsp lines in alphabetical order i.e. like below?

    intel:${LAYERDIR}/dynamic-layers/meta-intel/*/*/*.bb \
    intel:${LAYERDIR}/dynamic-layers/meta-intel/*/*/*.bbappend \
+   meta-ti-bsp:${LAYERDIR}/dynamic-layers/meta-ti-bsp/*/*/*.bb \
+   meta-ti-bsp:${LAYERDIR}/dynamic-layers/meta-ti-bsp/*/*/*.bbappend \
    toradex-bsp-common-layer:${LAYERDIR}/dynamic-layers/meta-toradex-bsp-common/*/*/*.bb \
    toradex-bsp-common-layer:${LAYERDIR}/dynamic-layers/meta-toradex-bsp-common/*/*/*.bbappend \
    toradex-ti-layer:${LAYERDIR}/dynamic-layers/meta-toradex-ti/*/*/*.bb \
    toradex-ti-layer:${LAYERDIR}/dynamic-layers/meta-toradex-ti/*/*/*.bbappend \

@@ -95,7 +97,7 @@ board_fixups=if test "${board}" = "verdin-imx8mm"; then \
elif test "${fdtfile}" = "imx8mm-verdin-wifi-v1.1-dev.dtb"; then \
env set fdtfile imx8mm-verdin-wifi-dev.dtb; \
fi; \
fi || true
fi || if test -n "${extra_board_fixups}"; then run extra_board_fixups; fi || true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line ever running?
Maybe we just rename extra_board_fixups to something else and add it straight into bootcmd_run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think U-Boot runs the if statement for this board. I created a Torizon OS image with the above change and it's able to boot successfully.

If I change uEnv.txt and set the extra_board_fixups variable to an empty string, the fixup isn't made i.e. U-Boot can't find the DTB due to the ti/ prefix in the filename:

7049 bytes read in 21 ms (327.1 KiB/s)
Failed to load '/boot/boot/ostree/torizon-d27d6b32eccb464636330b8ea50c3e45abe251ff93956934e96b97c16cd3042d/dtb/ti/k3-am625-sk.dtb'
133 bytes read in 21 ms (5.9 KiB/s)
libfdt fdt_check_header(): FDT_ERR_BADMAGIC
No FDT memory address configured. Please configure
the FDT address via "fdt addr <address>" command.
Aborting!
Applying Overlay: k3-am62x-sk-csi2-imx219.dtbo
2361 bytes read in 22 ms (104.5 KiB/s)
No FDT memory address configured. Please configure
the FDT address via "fdt addr <address>" command.
Aborting!
No FDT memory address configured. Please configure
the FDT address via "fdt addr <address>" command.
Aborting!
Applying Overlay: k3-am62x-sk-csi2-ov5640.dtbo
2332 bytes read in 21 ms (108.4 KiB/s)
No FDT memory address configured. Please configure
the FDT address via "fdt addr <address>" command.
Aborting!
[...]
21029376 bytes read in 849 ms (23.6 MiB/s)
31239767 bytes read in 1318 ms (22.6 MiB/s)
ERROR: Did not find a cmdline Flattened Device Tree
Could not find a valid device tree

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant having it in uenv like

uenv_extra_configs=@@UENV_EXTRA_CONFIGS@@
...
bootcmd_run=... && run uenv_extra_configs && ...

And in the recipe having a default like

UENV_EXTRA_CONFIGS ?= "true"

This way this is a separate u-boot command that we're always calling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, currently the only case where the command doesn't run is when the board is a Verdin iMX8M Mini, as it has its own DTB fixups hardcoded there.

But I guess it makes sense to have a more general command that always gets executed. I'll change it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants