| Message ID | 20230425081444.1441521-2-jamie.gibbons@microchip.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | *** Add Microchip PolarFire SoC Icicle Kit *** | expand |
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
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@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
Hello Jamie, On Tue, 25 Apr 2023 10:18:41 +0000 <Jamie.Gibbons@microchip.com> wrote: > Thanks a million for your feedback. I have tidied up and made the > changes you suggested below. I will update all in a v2. Please wait a little bit before v2 if possible. I also had a few comments on PATCH 2/2. I hope I'll be able to review it today, if not tomorrow. Best regards, Thomas
Hi Thomas, all, On Tue, 2023-04-25 at 13:08 +0200, Thomas Petazzoni wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Hello Jamie, > > On Tue, 25 Apr 2023 10:18:41 +0000 > <Jamie.Gibbons@microchip.com> wrote: > > > Thanks a million for your feedback. I have tidied up and made the > > changes you suggested below. I will update all in a v2. > > Please wait a little bit before v2 if possible. I also had a few > comments on PATCH 2/2. I hope I'll be able to review it today, if not > tomorrow. Wondering if there is any further feedback on this patch or patch 2/2 in this series from anyone before I send a v2. Thank you in advance, Jamie.
diff --git a/package/Config.in.host b/package/Config.in.host index dcadbfdfc1..42856c09df 100644 --- a/package/Config.in.host +++ b/package/Config.in.host @@ -58,6 +58,7 @@ menu "Host utilities" source "package/mender-artifact/Config.in.host" source "package/meson-tools/Config.in.host" source "package/mfgtools/Config.in.host" + source "package/microchip-hss-payload-generator/Config.in.host" source "package/mkpasswd/Config.in.host" source "package/moby-buildkit/Config.in.host" source "package/mosquitto/Config.in.host" 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). 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 + # +################################################################################ +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)) +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_INSTALL_STAGING = NO +HOST_MICROCHIP_HSS_PAYLOAD_GENERATOR_INSTALL_TARGET = YES +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" +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 +endef + +$(eval $(host-generic-package))