Re: [Buildroot] [PATCH 1/2] package/microchip-hss-payload-generator: add host package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Jamie,

Thanks for your patch. Here are a few comments below.

On Tue, 25 Apr 2023 09:14:43 +0100
Jamie Gibbons via buildroot <buildroot@buildroot.org> wrote:

>  package/Config.in.host                        |  1 +
>  .../Config.in.host                            |  8 +++++++
>  .../microchip-hss-payload-generator.mk        | 22 +++++++++++++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100644 package/microchip-hss-payload-generator/Config.in.host
>  create mode 100644 package/microchip-hss-payload-generator/microchip-hss-payload-generator.mk

We need the DEVELOPERS file to be updated with a new entry for this
package.

> diff --git a/package/microchip-hss-payload-generator/Config.in.host b/package/microchip-hss-payload-generator/Config.in.host
> new file mode 100644
> index 0000000000..7e0bbad719
> --- /dev/null
> +++ b/package/microchip-hss-payload-generator/Config.in.host
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR
> +	bool "HSS Payload Generator"
> +	help
> +	  Microchip PolarFire SoC Payload Generator. This tool creates a formatted
> +	  payload image for the HSS zero-stage bootloader on PolarFire SoC, given a
> +	  configuration file and a set of ELF binaries. The configuration file is
> +	  used to map the ELF binaries or binary blobs to the individual application
> +	  harts (U54s).

At the end of the help text, we need an empty line, followed by the
homepage of the project web site (Github repo homepage will work fine).

> diff --git a/package/microchip-hss-payload-generator/microchip-hss-payload-generator.mk b/package/microchip-hss-payload-generator/microchip-hss-payload-generator.mk
> new file mode 100644
> index 0000000000..ca5f59e0f1
> --- /dev/null
> +++ b/package/microchip-hss-payload-generator/microchip-hss-payload-generator.mk
> @@ -0,0 +1,22 @@
> +################################################################################
> + #
> + # Microchip Hart Software Services
> + #
> +################################################################################

The formatting of this header is not correct: no spaces before # signs.
Also the string should be just the package name:
"microchip-hss-payload-generator".

> +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_VERSION = v2023.02
> +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_SITE = $(call github,polarfire-soc,hart-software-services,$(HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_VERSION))

Please put the "v" in the _SITE variable, but not version:

HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_VERSION = 2023.02
HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_SITE = $(call github,polarfire-soc,hart-software-services,v$(HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_VERSION))

This way if ever release-monitoring.org is used to detect new versions
of this package, it will work correctly.

> +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_INSTALL_STAGING = NO
> +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_INSTALL_TARGET = YES

Both of these lines are not needed: you are doing a host package.

> +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_LICENSE = MIT
> +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_LICENSE_FILES = LICENSE.md
> +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_DEPENDENCIES = host-elfutils host-libyaml
> +
> +define HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_BUILD_CMDS
> +        $(MAKE) -C $(@D)/tools/hss-payload-generator HOST_INCLUDES=-I$(HOST_DIR)/include/ LDFLAGS="$(HOST_LDFLAGS) -Wl,-rpath,$(HOST_DIR)/lib -L$(HOST_DIR)/lib"

Indentation should be one tab.

Why do you need to pass -Wl,-rpath and -L in addition to HOST_LDFLAGS?
HOST_LDFLAGS is already defined as:

HOST_LDFLAGS  += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib

Overall, this should look like this:

	$(MAKE) -C $(@D)/tools/hss-payload-generator \
		HOST_INCLUDES="$(HOST_CPPFLAGS)" \
		LDFLAGS="$(HOST_LDFLAGS)"

> +endef
> +
> +define HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_INSTALL_CMDS
> +		$(INSTALL) -D -m 755 $(@D)/tools/hss-payload-generator/hss-payload-generator $(HOST_DIR)/bin/hss-payload-generator

Indentation should be one tab. Please split the long line with a \
somewhere (between source and destination paths, probably).

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot