Message ID | 1469426487-19558-1-git-send-email-joel@jms.id.au |
---|---|
State | Superseded |
Headers | show |
Hi, Am Mon, 25 Jul 2016 15:31:27 +0930 schrieb Joel Stanley: > diff --git a/package/pflash/Config.in b/package/pflash/Config.in > new file mode 100644 > index 000000000000..315989724088 > --- /dev/null > +++ b/package/pflash/Config.in > @@ -0,0 +1,6 @@ > +config BR2_PACKAGE_PFLASH > + bool "pflash" > + depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64 > + help > + PNOR Flash reading and writing tool for OpenPower servers. Used for > + firmware upgrades and extracting diagnostic infromation from flash. please add https://github.com/open-power/skiboot to the help text. > diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk > new file mode 100644 > index 000000000000..ab19ad699866 > --- /dev/null > +++ b/package/pflash/pflash.mk > @@ -0,0 +1,32 @@ > +################################################################################ > +# > +# pflash > +# > +################################################################################ > + > +PFLASH_VERSION = skiboot-5.2.4 > + no newline here. > +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION)) > +PFLASH_INSTALL_STAGING = YES Do other packages need header files from pflash? If not, it is not necessary to install the package to staging. Regards, Bernd
On Mon, Jul 25, 2016 at 3:44 PM, Bernd Kuhls <bernd.kuhls@t-online.de> wrote: > Hi, > > Am Mon, 25 Jul 2016 15:31:27 +0930 schrieb Joel Stanley: > >> diff --git a/package/pflash/Config.in b/package/pflash/Config.in >> new file mode 100644 >> index 000000000000..315989724088 >> --- /dev/null >> +++ b/package/pflash/Config.in >> @@ -0,0 +1,6 @@ >> +config BR2_PACKAGE_PFLASH >> + bool "pflash" >> + depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64 >> + help >> + PNOR Flash reading and writing tool for OpenPower servers. Used for >> + firmware upgrades and extracting diagnostic infromation from flash. > > please add > > https://github.com/open-power/skiboot > > to the help text. Good idea. > >> diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk >> new file mode 100644 >> index 000000000000..ab19ad699866 >> --- /dev/null >> +++ b/package/pflash/pflash.mk >> @@ -0,0 +1,32 @@ >> +################################################################################ >> +# >> +# pflash >> +# >> +################################################################################ >> + >> +PFLASH_VERSION = skiboot-5.2.4 >> + > > no newline here. > >> +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION)) >> +PFLASH_INSTALL_STAGING = YES > > Do other packages need header files from pflash? If not, it is > not necessary to install the package to staging. Thanks for the explanation. We don't install any headers so I can drop this. Thanks for the review! Cheers, Joel > Regards, Bernd > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hello, On Mon, 25 Jul 2016 15:31:27 +0930, Joel Stanley wrote: > diff --git a/package/pflash/Config.in b/package/pflash/Config.in > new file mode 100644 > index 000000000000..315989724088 > --- /dev/null > +++ b/package/pflash/Config.in > @@ -0,0 +1,6 @@ > +config BR2_PACKAGE_PFLASH > + bool "pflash" > + depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64 Is there something that makes it inherently usable only on those architectures? > diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk > new file mode 100644 > index 000000000000..ab19ad699866 > --- /dev/null > +++ b/package/pflash/pflash.mk > @@ -0,0 +1,32 @@ > +################################################################################ > +# > +# pflash > +# > +################################################################################ > + > +PFLASH_VERSION = skiboot-5.2.4 > + > +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION)) So skiboot is a much much larger repository, with lots of other things, and you're building just the external/pflash subdirectory of it. Are you sure we'll never want to have a package for other things in the skiboot repository? > +PFLASH_INSTALL_STAGING = YES You're not specifying any PFLASH_INSTALL_STAGING_CMDS, so this is useless. > +PFLASH_LICENSE = Apache 2.0 > +PFLASH_LICENSE_FILE = LICENCE > + > +PFLASH_MAKE_OPTS += CROSS_COMPILE="$(TARGET_CROSS)" \ > + PFLASH_VERSION=$(PFLASH_VERSION) \ > + DESTDIR=$(TARGET_DIR) \ DESTDIR should only be passed at install time. > + -C $(@D)/external/pflash \ Unneeded trailing \ Also, we prefer the -C option and its argument to be part of the build and install commands themselves. Thanks! Thomas
On Tue, Jul 26, 2016 at 7:37 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Mon, 25 Jul 2016 15:31:27 +0930, Joel Stanley wrote: > >> diff --git a/package/pflash/Config.in b/package/pflash/Config.in >> new file mode 100644 >> index 000000000000..315989724088 >> --- /dev/null >> +++ b/package/pflash/Config.in >> @@ -0,0 +1,6 @@ >> +config BR2_PACKAGE_PFLASH >> + bool "pflash" >> + depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64 > > Is there something that makes it inherently usable only on those > architectures? Yeah, the 'libflash' library selects different backends depending on it's > >> diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk >> new file mode 100644 >> index 000000000000..ab19ad699866 >> --- /dev/null >> +++ b/package/pflash/pflash.mk >> @@ -0,0 +1,32 @@ >> +################################################################################ >> +# >> +# pflash >> +# >> +################################################################################ >> + >> +PFLASH_VERSION = skiboot-5.2.4 >> + >> +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION)) > > So skiboot is a much much larger repository, with lots of other things, > and you're building just the external/pflash subdirectory of it. > > Are you sure we'll never want to have a package for other things in the > skiboot repository? We might want to package other tools in the future. How should I change the packaging of pflash? > >> +PFLASH_INSTALL_STAGING = YES > > You're not specifying any PFLASH_INSTALL_STAGING_CMDS, so this is > useless. Thanks for clearing that up. I've fixed it in v2. > >> +PFLASH_LICENSE = Apache 2.0 >> +PFLASH_LICENSE_FILE = LICENCE >> + >> +PFLASH_MAKE_OPTS += CROSS_COMPILE="$(TARGET_CROSS)" \ >> + PFLASH_VERSION=$(PFLASH_VERSION) \ >> + DESTDIR=$(TARGET_DIR) \ > > DESTDIR should only be passed at install time. Okay. > >> + -C $(@D)/external/pflash \ > > Unneeded trailing \ Okay. > > Also, we prefer the -C option and its argument to be part of the build > and install commands themselves. Even though we would be replicating them for every invocation of make? I think it's better placed in a variable so changes can be made in one place. Cheers, Joel
Hello, On Tue, 26 Jul 2016 11:57:17 +0930, Joel Stanley wrote: > >> diff --git a/package/pflash/Config.in b/package/pflash/Config.in > >> new file mode 100644 > >> index 000000000000..315989724088 > >> --- /dev/null > >> +++ b/package/pflash/Config.in > >> @@ -0,0 +1,6 @@ > >> +config BR2_PACKAGE_PFLASH > >> + bool "pflash" > >> + depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64 > > > > Is there something that makes it inherently usable only on those > > architectures? > > Yeah, the 'libflash' library selects different backends depending on it's OK. But then it makes sense only for pflash itself, and not skitool as the whole I suppose? > > So skiboot is a much much larger repository, with lots of other things, > > and you're building just the external/pflash subdirectory of it. > > > > Are you sure we'll never want to have a package for other things in the > > skiboot repository? > > We might want to package other tools in the future. > > How should I change the packaging of pflash? A top-level BR2_PACKAGE_SKITOOL option, with a package called skitool. And then a sub-option BR2_PACKAGE_SKITOOL_PFLASH, which enables the build/installation of pflash. BR2_PACKAGE_SKITOOL can "select" BR2_PACKAGE_SKITOOL_PFLASH to make sure the package at least installs one thing. > > Also, we prefer the -C option and its argument to be part of the build > > and install commands themselves. > > Even though we would be replicating them for every invocation of make? > I think it's better placed in a variable so changes can be made in one > place. We generally repeat the -C $(@D) in the different invocations, yes. Not sure it's ideal, but that's how we typically do it in most packages. Thomas
diff --git a/package/Config.in b/package/Config.in index 814141f18784..9f873c31fa58 100644 --- a/package/Config.in +++ b/package/Config.in @@ -426,6 +426,7 @@ endmenu source "package/owl-linux/Config.in" source "package/parted/Config.in" source "package/pciutils/Config.in" + source "package/pflash/Config.in" source "package/picocom/Config.in" source "package/pifmrds/Config.in" source "package/powertop/Config.in" diff --git a/package/pflash/0001-pflash-use-atexit-for-musl-compatibility.patch b/package/pflash/0001-pflash-use-atexit-for-musl-compatibility.patch new file mode 100644 index 000000000000..f8ac58eba329 --- /dev/null +++ b/package/pflash/0001-pflash-use-atexit-for-musl-compatibility.patch @@ -0,0 +1,42 @@ +From 7f6b017b8dc3b5f276b30353e0a7186e4e433e7a Mon Sep 17 00:00:00 2001 +From: Joel Stanley <joel@jms.id.au> +Date: Mon, 25 Jul 2016 13:24:37 +0930 +Subject: [PATCH] pflash: use atexit for musl compatibility +To: skiboot@lists.ozlabs.org + +I accidentally built myself a cross-toolchain with the musl libc. It does +not support on_exit which we use to clean up in pflash. + +Instead use atexit with is supported by both uclibc, musl and glibc. + +Signed-off-by: Joel Stanley <joel@jms.id.au> +--- + external/pflash/pflash.c | 5 ++--- + 1 file changed, 2 insertions(+), 3 deletions(-) + +diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c +index c124356fd3d9..27000469052e 100644 +--- a/external/pflash/pflash.c ++++ b/external/pflash/pflash.c +@@ -509,7 +509,7 @@ static void print_help(const char *pname) + printf("\t\tThis message.\n\n"); + } + +-void exiting(int d, void *p) ++void exiting(void) + { + if (need_relock) + arch_flash_set_wrprotect(bl, 1); +@@ -775,8 +775,7 @@ int main(int argc, char *argv[]) + exit(1); + } + +- on_exit(exiting, NULL); +- ++ atexit(exiting); + + rc = blocklevel_get_info(bl, &fl_name, + &fl_total_size, &fl_erase_granule); +-- +2.8.1 + diff --git a/package/pflash/Config.in b/package/pflash/Config.in new file mode 100644 index 000000000000..315989724088 --- /dev/null +++ b/package/pflash/Config.in @@ -0,0 +1,6 @@ +config BR2_PACKAGE_PFLASH + bool "pflash" + depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64 + help + PNOR Flash reading and writing tool for OpenPower servers. Used for + firmware upgrades and extracting diagnostic infromation from flash. diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk new file mode 100644 index 000000000000..ab19ad699866 --- /dev/null +++ b/package/pflash/pflash.mk @@ -0,0 +1,32 @@ +################################################################################ +# +# pflash +# +################################################################################ + +PFLASH_VERSION = skiboot-5.2.4 + +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION)) +PFLASH_INSTALL_STAGING = YES +PFLASH_LICENSE = Apache 2.0 +PFLASH_LICENSE_FILE = LICENCE + +PFLASH_MAKE_OPTS += CROSS_COMPILE="$(TARGET_CROSS)" \ + PFLASH_VERSION=$(PFLASH_VERSION) \ + DESTDIR=$(TARGET_DIR) \ + -C $(@D)/external/pflash \ + +# A makefile bug causes recent versions of pflash to fail setting CC and LD +# based on CROSS_COMPILE. Set CC and LD to remain compatible with those +# versions. +PFLASH_MAKE_OPTS += CC=$(TARGET_CC) LD=$(TARGET_LD) + +define PFLASH_BUILD_CMDS + $(TARGET_MAKE_ENV) $(MAKE) $(PFLASH_MAKE_OPTS) all +endef + +define PFLASH_INSTALL_TARGET_CMDS + $(TARGET_MAKE_ENV) $(MAKE) $(PFLASH_MAKE_OPTS) install +endef + +$(eval $(generic-package))
pflash is a tool for reading and writing to the PNOR on OpenPower servers, for firmware upgrades, and extracting diagnostic information. This includes a fix so that pflash can build with musl, which has been submitted upstream[1]. [1] http://patchwork.ozlabs.org/patch/652168/ Signed-off-by: Joel Stanley <joel@jms.id.au> --- package/Config.in | 1 + ...-pflash-use-atexit-for-musl-compatibility.patch | 42 ++++++++++++++++++++++ package/pflash/Config.in | 6 ++++ package/pflash/pflash.mk | 32 +++++++++++++++++ 4 files changed, 81 insertions(+) create mode 100644 package/pflash/0001-pflash-use-atexit-for-musl-compatibility.patch create mode 100644 package/pflash/Config.in create mode 100644 package/pflash/pflash.mk