Skip to contentSkip to conversation

package/sunxi-tools: add tool selection

PATCH New kaplan2539 at gmail.com 2 participants 2 messages Nov 10, 2018 – Nov 11, 2018 Arm/Allwinner sunXi SoC support

lore
Share & tools
lore

kaplan2539 at gmail.com

author
#0 lore raw

PATCH 1/1 package/sunxi-tools: add tool selection

From: Alex Kaplan <[email protected]>

This patch allows to select which of the sunxi-tools are built and installed in host and target diretories.
This way it is possible to have e.g. sunxi-nand-image-builder among the host tools, or to install sunxi-fel on the target device. The corresponding config options have been added to Config.in and Config.in.host and the sunxi-tools.mk has been adapted respectively. The defaults have been selected to produce the same output as without this patch.

Signed-off-by Alex Kaplan <[email protected]>
---
 package/sunxi-tools/Config.in      | 51 +++++++++++++++++++++++++++++++++++---
 package/sunxi-tools/Config.in.host | 40 ++++++++++++++++++++++++++++++
 package/sunxi-tools/sunxi-tools.mk | 51 ++++++++++++++++++++++++++++++++------
 3 files changed, 130 insertions(+), 12 deletions(-)
package/sunxi-tools/Config.in +47 -4
index 02eba95..83eb982 100644
@@ -1,9 +1,52 @@
 config BR2_PACKAGE_SUNXI_TOOLS
-	bool "sunxi nand-part"
+	bool "sunxi-tools"
 	depends on BR2_arm
 	help
-	  nand-part is part of sunxi-tools for Allwinner A10 (aka
-	  sun4i) and A13 (aka sun5i) based devices. It is a tool to
-	  repartition the internal NAND on sunxi devices.
+	  Tools for Allwinner A10 (aka sun4i) and A13 (aka sun5i)
+	  based devices. This includes fex2bin which can be used to
+	  compile .fex board definition files to the binary script.bin
+	  format required by the linux-sunxi kernel. These tools are
+	  specific for linux-sunxi kernel and do not apply to the
+	  mainline Linux kernel version.
 
 	  http://linux-sunxi.org/Sunxi-tools
+
+if BR2_PACKAGE_SUNXI_TOOLS
+
+# from the "tools" make target
+config BR2_PACKAGE_SUNXI_TOOLS_FEXC
+    bool "sunxi-fexc"
+    default n
+
+config BR2_PACKAGE_SUNXI_TOOLS_BOOTINFO
+    bool "sunxi-bootinfo"
+    default n
+
+config BR2_PACKAGE_SUNXI_TOOLS_FEL
+    bool "sunxi-fel"
+    default n
+
+config BR2_PACKAGE_SUNXI_TOOLS_NAND_PART
+    bool "sunxi-nand-part"
+    default y
+
+config BR2_PACKAGE_SUNXI_TOOLS_PIO
+    bool "sunxi-pio"
+    default n
+
+# from the "target-tools" make target
+config BR2_PACKAGE_SUNXI_TOOLS_MEMINFO
+    bool "sunxi-meminfo"
+    default n
+
+# from the "misc" make target
+config BR2_PACKAGE_SUNXI_TOOLS_PHOENIX_INFO
+    bool "phoenix_info"
+    default n
+
+config BR2_PACKAGE_SUNXI_TOOLS_NAND_IMAGE_BUILDER
+    bool "sunxi-nand-image-builder"
+    default n
+
+endif #BR2_PACKAGE_SUNXI_TOOLS
+
package/sunxi-tools/Config.in.host +40 -0
index 5fab5e6..f02fd40 100644
@@ -10,3 +10,43 @@ config BR2_PACKAGE_HOST_SUNXI_TOOLS
 	  mainline Linux kernel version.
 
 	  http://linux-sunxi.org/Sunxi-tools
+
+if BR2_PACKAGE_HOST_SUNXI_TOOLS
+
+# from the "tools" make target
+config BR2_PACKAGE_HOST_SUNXI_TOOLS_FEXC
+    bool "sunxi-fexc"
+    default y
+
+config BR2_PACKAGE_HOST_SUNXI_TOOLS_BOOTINFO
+    bool "sunxi-bootinfo"
+    default y
+
+config BR2_PACKAGE_HOST_SUNXI_TOOLS_FEL
+    bool "sunxi-fel"
+    default y
+
+config BR2_PACKAGE_HOST_SUNXI_TOOLS_NAND_PART
+    bool "sunxi-nand-part"
+    default y
+
+config BR2_PACKAGE_HOST_SUNXI_TOOLS_PIO
+    bool "sunxi-pio"
+    default y
+
+# from the "target-tools" make target
+config BR2_PACKAGE_HOST_SUNXI_TOOLS_MEMINFO
+    bool "sunxi-meminfo"
+    default n
+
+# from the "misc" make target
+config BR2_PACKAGE_HOST_SUNXI_TOOLS_PHOENIX_INFO
+    bool "phoenix_info"
+    default n
+
+config BR2_PACKAGE_HOST_SUNXI_TOOLS_NAND_IMAGE_BUILDER
+    bool "sunxi-nand-image-builder"
+    default n
+
+endif #BR2_PACKAGE_HOST_SUNXI_TOOLS
+
package/sunxi-tools/sunxi-tools.mk +43 -8

74 lines not on this page · raw message

Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/1 package/sunxi-tools: add tool selection

Hi Alex,

Thank you for your patch. I have a few comments on it. Could you take these
into account and respin?

Quoted 5 lines · On 11/11/2018 00:58, kaplan2539 at gmail.com wrote:

From: Alex Kaplan <[email protected]>

This patch allows to select which of the sunxi-tools are built and installed in host and target diretories.
This way it is possible to have e.g. sunxi-nand-image-builder among the host tools, or to install sunxi-fel on the target device. The corresponding config options have been added to Config.in and Config.in.host and the sunxi-tools.mk has been adapted respectively.

It would probably be better to split this into two patches, one for target and
one for host. However, I'm not so sure even of that:

- for the host, we really don't care about the size; the only reason to make the tools selectable would be to avoid pulling in some (large) dependency. Since host-libusb already is a dependency: just build everything;

- even for the target, it only makes sense to make the individual tools selectable if that makes a significant size difference. I haven't tried it, but I have the impression that these are really small executables. So even for the target, I'm not so sure we really want Config.in options. There, however, it does make sense to have them.

Also, the commit message should be wrapped at 72 columns (which vim does for
you by default).

Quoted 17 lines

The defaults have been selected to produce the same output as without this patch.>

Signed-off-by Alex Kaplan <[email protected]>
---
 package/sunxi-tools/Config.in      | 51 +++++++++++++++++++++++++++++++++++---
 package/sunxi-tools/Config.in.host | 40 ++++++++++++++++++++++++++++++
 package/sunxi-tools/sunxi-tools.mk | 51 ++++++++++++++++++++++++++++++++------
 3 files changed, 130 insertions(+), 12 deletions(-)
package/sunxi-tools/Config.in +1 -1
index 02eba95..83eb982 100644
@@ -1,9 +1,52 @@
 config BR2_PACKAGE_SUNXI_TOOLS
-	bool "sunxi nand-part"
+	bool "sunxi-tools"
 	depends on BR2_arm

package/sunxi-tools/Config.in line 10

Quoted 10 lines
package/sunxi-tools/Config.in +6 -3
 	help
-	  nand-part is part of sunxi-tools for Allwinner A10 (aka
-	  sun4i) and A13 (aka sun5i) based devices. It is a tool to
-	  repartition the internal NAND on sunxi devices.
+	  Tools for Allwinner A10 (aka sun4i) and A13 (aka sun5i)
+	  based devices. This includes fex2bin which can be used to
+	  compile .fex board definition files to the binary script.bin
+	  format required by the linux-sunxi kernel. These tools are
+	  specific for linux-sunxi kernel and do not apply to the
+	  mainline Linux kernel version.

It's a bit weird that now the help text doesn't mention sunxi-nand-part at all
any more. So I would remove most of the help text here, but add help texts to the individual tools.

package/sunxi-tools/Config.in line 16

Quoted 6 lines
package/sunxi-tools/Config.in +4 -0
 	  http://linux-sunxi.org/Sunxi-tools
+
+if BR2_PACKAGE_SUNXI_TOOLS
+
+# from the "tools" make target

I don't think this comment is really useful - which make target it is is more a
.mk issue.

package/sunxi-tools/Config.in line 19

Quoted 3 lines
package/sunxi-tools/Config.in +3 -0
+config BR2_PACKAGE_SUNXI_TOOLS_FEXC
+    bool "sunxi-fexc"
+    default n

The 'default n' is not needed, it's the default.

Also, all these options really should have a help text. For this one, it can be
the text above.

package/sunxi-tools/Config.in line 26

Quoted 7 lines
package/sunxi-tools/Config.in +7 -0
+
+config BR2_PACKAGE_SUNXI_TOOLS_BOOTINFO
+    bool "sunxi-bootinfo"
+    default n
+
+config BR2_PACKAGE_SUNXI_TOOLS_FEL
+    bool "sunxi-fel"

Since this one pulls in a dependency on libusb, you should select it here. And
since libusb has a dependency on threads, that should also be added here:

config BR2_PACKAGE_SUNXI_TOOLS_FEL

	bool "sunxi-fel"
	depends on BR2_TOOLCHAIN_HAS_THREADS # libusb
	select BR2_PACKAGE_LIBUSB
	help
	  ...

comment "sunxi-fel needs a toolchain w/ threads"
depends on !BR2_TOOLCHAIN_HAS_THREADS

Quoted 26 lines
package/sunxi-tools/Config.in +26 -0
+    default n
+
+config BR2_PACKAGE_SUNXI_TOOLS_NAND_PART
+    bool "sunxi-nand-part"
+    default y
+
+config BR2_PACKAGE_SUNXI_TOOLS_PIO
+    bool "sunxi-pio"
+    default n
+
+# from the "target-tools" make target
+config BR2_PACKAGE_SUNXI_TOOLS_MEMINFO
+    bool "sunxi-meminfo"
+    default n
+
+# from the "misc" make target
+config BR2_PACKAGE_SUNXI_TOOLS_PHOENIX_INFO
+    bool "phoenix_info"
+    default n
+
+config BR2_PACKAGE_SUNXI_TOOLS_NAND_IMAGE_BUILDER
+    bool "sunxi-nand-image-builder"
+    default n
+
+endif #BR2_PACKAGE_SUNXI_TOOLS
+

[snip]

package/sunxi-tools/sunxi-tools.mk line 10

Quoted 12 lines
package/sunxi-tools/sunxi-tools.mk +4 -0
index 3a44cf6..2cc4443 100644
@@ -4,6 +4,11 @@
 #
 ################################################################################
 
+#SUNXI_TOOLS_VERSION = 24be7518cbc3622b65d3fc29f892e06bb4800ab9
+
+#v 1.4.2 not officially released yet?!?
+#SUNXI_TOOLS_VERSION = 60ea132fccce2dda22987da6f1694095fb33cb53

Don't include comments like this in a patch you send upstream.

Quoted 15 lines
package/sunxi-tools/sunxi-tools.mk +8 -0
+
 SUNXI_TOOLS_VERSION = v1.4.1
 SUNXI_TOOLS_SITE = $(call github,linux-sunxi,sunxi-tools,$(SUNXI_TOOLS_VERSION))
 SUNXI_TOOLS_LICENSE = GPL-2.0+

@@ -11,25 +16,55 @@ SUNXI_TOOLS_LICENSE_FILES = LICENSE.md
 HOST_SUNXI_TOOLS_DEPENDENCIES = host-libusb host-pkgconf
 FEX2BIN = $(HOST_DIR)/bin/fex2bin
 
+HOST_SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_FEXC)               += sunxi-fexc
+HOST_SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_BOOTINFO)           += sunxi-bootinfo
+HOST_SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_FEL)                += sunxi-fel
+HOST_SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_NAND_PART)          += sunxi-nand-part
+HOST_SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_PIO)                += sunxi-pio
+HOST_SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_MEMINFO)            += sunxi-meminfo
+HOST_SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_PHOENIX_INFO)       += phoenix_info

package/sunxi-tools/sunxi-tools.mk SUNXI_TOOLS_LICENSE_FILES = LICENSE.md · line 35

Quoted 10 lines
package/sunxi-tools/sunxi-tools.mk +10 -0
+HOST_SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_NAND_IMAGE_BUILDER) += sunxi-nand-image-builder
+
+SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_FEXC)               += sunxi-fexc
+SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_BOOTINFO)           += sunxi-bootinfo
+SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_FEL)                += sunxi-fel
+SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_NAND_PART)          += sunxi-nand-part
+SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_PIO)                += sunxi-pio
+SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_MEMINFO)            += sunxi-meminfo
+SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_PHOENIX_INFO)       += phoenix_info
+SUNXI_TOOLS_TARGETS_$(BR2_PACKAGE_HOST_SUNXI_TOOLS_NAND_IMAGE_BUILDER) += sunxi-nand-image-builder

This is a good approach of enumerating the selected tools.

package/sunxi-tools/sunxi-tools.mk SUNXI_TOOLS_LICENSE_FILES = LICENSE.md · line 38

Quoted 3 lines
package/sunxi-tools/sunxi-tools.mk +3 -0
+
+ifeq ($(BR2_PACKAGE_SUNXI_TOOLS_FEL),y)
+	SUNXI_TOOLS_DEPENDENCIES += libusb host-pkgconf

We don't indent within a condition.

Quoted 19 lines
package/sunxi-tools/sunxi-tools.mk +10 -4
+endif
+
+
 define HOST_SUNXI_TOOLS_BUILD_CMDS
-	$(HOST_MAKE_ENV) $(MAKE) CC="$(HOSTCC)" PREFIX=$(HOST_DIR) \
+	$(foreach t,$(HOST_SUNXI_TOOLS_TARGETS_y), \
+	    $(HOST_MAKE_ENV) $(MAKE) CROSS_COMPILE="" CC="$(HOSTCC)" PREFIX=$(HOST_DIR) \
 		EXTRA_CFLAGS="$(HOST_CFLAGS)" LDFLAGS="$(HOST_LDFLAGS)" \
-		-C $(@D) tools
+		-C $(@D) $(t)
+	)
 endef
 
 define HOST_SUNXI_TOOLS_INSTALL_CMDS
-	$(HOST_MAKE_ENV) $(MAKE) PREFIX=$(HOST_DIR) \
-		-C $(@D) install-tools
+	$(foreach t,$(HOST_SUNXI_TOOLS_TARGETS_y), \
+		$(INSTALL) -D -m 0755 $(@D)/$(t) $(HOST_DIR)/usr/bin/$(t)
+	)

package/sunxi-tools/sunxi-tools.mk SUNXI_TOOLS_LICENSE_FILES = LICENSE.md · line 60

Quoted 10 lines
package/sunxi-tools/sunxi-tools.mk +4 -3
 endef
 
 define SUNXI_TOOLS_BUILD_CMDS
-	$(TARGET_MAKE_ENV) $(MAKE) CC="$(TARGET_CC)" PREFIX=/usr \
-		EXTRA_CFLAGS="$(TARGET_CFLAGS)" LDFLAGS="$(TARGET_LDFLAGS)" \
-		-C $(@D) sunxi-nand-part
+	$(foreach t,$(SUNXI_TOOLS_TARGETS_y), \
+		$(TARGET_MAKE_ENV) $(MAKE) CROSS_COMPILE="$(TARGET_CROSS)" CC="$(TARGET_CC)" PREFIX=/usr \
+			EXTRA_CFLAGS="$(TARGET_CFLAGS)" LDFLAGS="$(TARGET_LDFLAGS)" \
+			-C $(@D) $(t)

This will invoke the submake multiple times, but I don't think that that's
needed. Can't you just replace 'sunxi-nand-part' with '$(UNXI_TOOLS_TARGETS_y)'? If you do that, you better check if there's a risk that parallel compilation will break (with these homegrown Makefiles, it sometimes does).

package/sunxi-tools/sunxi-tools.mk SUNXI_TOOLS_LICENSE_FILES = LICENSE.md · line 67

Quoted 8 lines
package/sunxi-tools/sunxi-tools.mk +4 -1
+	)
 endef
 
 define SUNXI_TOOLS_INSTALL_TARGET_CMDS
-	$(INSTALL) -D -m 0755 $(@D)/sunxi-nand-part $(TARGET_DIR)/usr/bin/sunxi-nand-part
+	$(foreach t,$(SUNXI_TOOLS_TARGETS_y), \
+		$(INSTALL) -D -m 0755 $(@D)/$(t) $(TARGET_DIR)/usr/bin/$(t)
+	)

Here, the foreach is indeed the way to go.

Thanks,

Regards,
Arnout

Quoted 4 lines
package/sunxi-tools/sunxi-tools.mk +0 -0
 endef
 
 $(eval $(generic-package))