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

[Jamie.Gibbons at microchip.com]

On Tue, 2023-04-25 at 11:44 +0200, Thomas Petazzoni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hello Jamie,
> 
> Thanks for your patch. Here are a few comments below.

Hi Thomas,

Thanks a million for your feedback. I have tidied up and made the
changes you suggested below. I will update all in a v2.

Thanks,
Jamie.
> 
> On Tue, 25 Apr 2023 09:14:43 +0100
> Jamie Gibbons via buildroot <buildroot at 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