diff mbox series

rtl838x: clean up build instructions

Message ID 20200929162006.236206-1-sander@svanheule.net
State Changes Requested
Headers show
Series rtl838x: clean up build instructions | expand

Commit Message

Sander Vanheule Sept. 29, 2020, 4:20 p.m. UTC
The Device/Default recipe was defined, but never used. Move definitions
that are not device-specific to Device/Default, and use it for
allnet_all-sg8208m. This should make it more straightforward to add new
devices.

The modified uImage recipe can also be simplified, similar to how
ath79/common-netgear.mk does it.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 target/linux/rtl838x/image/Makefile | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

Comments

Adrian Schmutzler Sept. 29, 2020, 5:44 p.m. UTC | #1
Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Sander Vanheule
> Sent: Dienstag, 29. September 2020 18:20
> To: openwrt-devel@lists.openwrt.org
> Cc: Sander Vanheule <sander@svanheule.net>
> Subject: [PATCH] rtl838x: clean up build instructions
> 
> The Device/Default recipe was defined, but never used. Move definitions
> that are not device-specific to Device/Default, and use it for allnet_all-
> sg8208m. This should make it more straightforward to add new devices.
> 
> The modified uImage recipe can also be simplified, similar to how
> ath79/common-netgear.mk does it.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  target/linux/rtl838x/image/Makefile | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/target/linux/rtl838x/image/Makefile
> b/target/linux/rtl838x/image/Makefile
> index eef1fe0a33..0174e47449 100644
> --- a/target/linux/rtl838x/image/Makefile
> +++ b/target/linux/rtl838x/image/Makefile
> @@ -7,36 +7,32 @@ include $(INCLUDE_DIR)/image.mk  KERNEL_LOADADDR
> = 0x80000000  KERNEL_ENTRY = 0x80000400
> 
> -define Build/custom-uimage
> -	mkimage -A $(LINUX_KARCH) \
> -		-O linux -T kernel \
> -		-C gzip -a $(KERNEL_LOADADDR) $(if $(UIMAGE_MAGIC),-M
> $(UIMAGE_MAGIC),) \
> -		-e $(if
> $(KERNEL_ENTRY),$(KERNEL_ENTRY),$(KERNEL_LOADADDR)) \
> -		-n '$(1)' -d $@ $@.new
> -	mv $@.new $@
> -endef
> +DEVICE_VARS += UIMAGE_MAGIC
> 
> +define Build/realtek-uImage
> +	$(call Build/uImage,$(1) $(if $(UIMAGE_MAGIC),-M
> $(UIMAGE_MAGIC),))
> +endef

I like the idea, but don't like this hack with the argument.
Either we can move the if here into image-commands.mk (which won't work if we need UIMAGE_MAGIC only for certain recipes ...), or I'd keep the old one.

> 
>  define Device/Default
>    PROFILES = Default
> -  KERNEL := kernel-bin | append-dtb | gzip | uImage gzip
> -  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | uImage gzip
> +  KERNEL := kernel-bin | append-dtb | gzip | realtek-uImage gzip

So, can we assume that we always need the magic for this target?

> + KERNEL_INITRAMFS := $$(KERNEL)
>    DEVICE_DTS_DIR := ../dts
>    DEVICE_DTS = $$(SOC)_$(1)
>    SUPPORTED_DEVICES := $(subst _,$(comma),$(1))
>    IMAGES := sysupgrade.bin
> -  IMAGE/sysupgrade.bin := append-kernel | pad-to 64k | append-rootfs |
> pad-rootfs | \
> -	append-metadata | check-size
> +  IMAGE/sysupgrade.bin := append-kernel | append-rootfs | \
> +      append-metadata | check-size

I'm not sure whether this is correct, but it should be a separate change anyway.

>  endef
> 
>  define Device/allnet_all-sg8208m
> +  $(Device/Default)

Device/Default is added to all devices by default (if it's defined) automatically!

https://github.com/openwrt/openwrt/blob/master/include/image.mk#L696

So, this would actually add it a second time.

Best

Adrian

>    SOC := rtl8382
>    IMAGE_SIZE := 7168k
>    DEVICE_VENDOR := ALLNET
>    DEVICE_MODEL := ALL-SG8208M
>    UIMAGE_MAGIC := 0x00000006
> -  KERNEL := kernel-bin | append-dtb | gzip | custom-uimage 2.2.2.0
> -  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | custom-uimage
> 2.2.2.0
> +  UIMAGE_NAME := 2.2.2.0
>    DEVICE_PACKAGES := ip-full ip-bridge kmod-gpio-button-hotplug tc  endef
> TARGET_DEVICES += allnet_all-sg8208m
> --
> 2.26.2
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Sander Vanheule Sept. 29, 2020, 7:44 p.m. UTC | #2
Hi Adrian,

On Tue, 2020-09-29 at 19:44 +0200, Adrian Schmutzler wrote:
> > -define Build/custom-uimage
> > -	mkimage -A $(LINUX_KARCH) \
> > -		-O linux -T kernel \
> > -		-C gzip -a $(KERNEL_LOADADDR) $(if $(UIMAGE_MAGIC),-M
> > $(UIMAGE_MAGIC),) \
> > -		-e $(if
> > $(KERNEL_ENTRY),$(KERNEL_ENTRY),$(KERNEL_LOADADDR)) \
> > -		-n '$(1)' -d $@ $@.new
> > -	mv $@.new $@
> > -endef
> > +DEVICE_VARS += UIMAGE_MAGIC
> > 
> > +define Build/realtek-uImage
> > +	$(call Build/uImage,$(1) $(if $(UIMAGE_MAGIC),-M
> > $(UIMAGE_MAGIC),))
> > +endef
> 
> I like the idea, but don't like this hack with the argument.
> Either we can move the if here into image-commands.mk (which won't
> work if we need UIMAGE_MAGIC only for certain recipes ...), or I'd
> keep the old one.

I got this from target/linux/ath79/image/common-netgear.mk, but with
the optional UIMAGE_MAGIC from the original recipe. I do agree it's a
hack.

To check if the above works for both cases, I've tested this with
UIMAGE_MAGIC left undefined. Then I got the standard header magic, as
expected. So I'm not sure what you mean with "won't work if we only
need UIMAGE_MAGIC for certain recipes".

If you have no objections, I can try to turn this change into a
treewide patch so all platforms have access to (optional) custom uImage
magic bytes.

> >  define Device/Default
> >    PROFILES = Default
> > -  KERNEL := kernel-bin | append-dtb | gzip | uImage gzip
> > -  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | uImage gzip
> > +  KERNEL := kernel-bin | append-dtb | gzip | realtek-uImage gzip
> 
> So, can we assume that we always need the magic for this target?

As far as I can tell, the Realtek SDK seems to push OEMs to use a
custom magic. I've checked a few of the vendor firmware files for the
devices listed at https://biot.com/switches/hardware:
* Cisco SF220-24, SG220-28: 0x8380000
* D-Link DGS-1210 (Rev F): 0x12345000
* Netgear: per-device magic (like they do on other platforms)
* Zyxel GS1900-10HP, GS1900-24E: 0x83800000

The default here appears to be 0x8380000, which could go into
Device/Default then.

For devices using an older SDK (with kernel 2.6), gzip compression is
used. On the GS110TPP, with a newer SDK (and 3.18 kernel), lzma
compression is used.

> > + KERNEL_INITRAMFS := $$(KERNEL)
> >    DEVICE_DTS_DIR := ../dts
> >    DEVICE_DTS = $$(SOC)_$(1)
> >    SUPPORTED_DEVICES := $(subst _,$(comma),$(1))
> >    IMAGES := sysupgrade.bin
> > -  IMAGE/sysupgrade.bin := append-kernel | pad-to 64k | append-
> > rootfs |
> > pad-rootfs | \
> > -	append-metadata | check-size
> > +  IMAGE/sysupgrade.bin := append-kernel | append-rootfs | \
> > +      append-metadata | check-size
> 
> I'm not sure whether this is correct, but it should be a separate
> change anyway.

Okay, I'll take it out of this patch and (wait to) introduce it with
another patch.

I've also had a short discussion with Birger about this. The firmware
files provided by the vendors are uImages with an initramfs, so there
doesn't really appear to be any reference for the sysupgrade images
anyway. Persistence is achieved by using a separate config partition┬╣.

[1] https://biot.com/switches/gs110tpp#firmware

> >  endef
> > 
> >  define Device/allnet_all-sg8208m
> > +  $(Device/Default)
> 
> Device/Default is added to all devices by default (if it's defined)
> automatically!
> 
> https://github.com/openwrt/openwrt/blob/master/include/image.mk#L696
> 
> So, this would actually add it a second time.

That's my lack of experience the OpenWrt code shining through again,
I'll take it out.

Best,
Sander
diff mbox series

Patch

diff --git a/target/linux/rtl838x/image/Makefile b/target/linux/rtl838x/image/Makefile
index eef1fe0a33..0174e47449 100644
--- a/target/linux/rtl838x/image/Makefile
+++ b/target/linux/rtl838x/image/Makefile
@@ -7,36 +7,32 @@  include $(INCLUDE_DIR)/image.mk
 KERNEL_LOADADDR = 0x80000000
 KERNEL_ENTRY = 0x80000400
 
-define Build/custom-uimage
-	mkimage -A $(LINUX_KARCH) \
-		-O linux -T kernel \
-		-C gzip -a $(KERNEL_LOADADDR) $(if $(UIMAGE_MAGIC),-M $(UIMAGE_MAGIC),) \
-		-e $(if $(KERNEL_ENTRY),$(KERNEL_ENTRY),$(KERNEL_LOADADDR)) \
-		-n '$(1)' -d $@ $@.new
-	mv $@.new $@
-endef
+DEVICE_VARS += UIMAGE_MAGIC
 
+define Build/realtek-uImage
+	$(call Build/uImage,$(1) $(if $(UIMAGE_MAGIC),-M $(UIMAGE_MAGIC),))
+endef
 
 define Device/Default
   PROFILES = Default
-  KERNEL := kernel-bin | append-dtb | gzip | uImage gzip
-  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | uImage gzip
+  KERNEL := kernel-bin | append-dtb | gzip | realtek-uImage gzip
+  KERNEL_INITRAMFS := $$(KERNEL)
   DEVICE_DTS_DIR := ../dts
   DEVICE_DTS = $$(SOC)_$(1)
   SUPPORTED_DEVICES := $(subst _,$(comma),$(1))
   IMAGES := sysupgrade.bin
-  IMAGE/sysupgrade.bin := append-kernel | pad-to 64k | append-rootfs | pad-rootfs | \
-	append-metadata | check-size
+  IMAGE/sysupgrade.bin := append-kernel | append-rootfs | \
+      append-metadata | check-size
 endef
 
 define Device/allnet_all-sg8208m
+  $(Device/Default)
   SOC := rtl8382
   IMAGE_SIZE := 7168k
   DEVICE_VENDOR := ALLNET
   DEVICE_MODEL := ALL-SG8208M
   UIMAGE_MAGIC := 0x00000006
-  KERNEL := kernel-bin | append-dtb | gzip | custom-uimage 2.2.2.0
-  KERNEL_INITRAMFS := kernel-bin | append-dtb | gzip | custom-uimage 2.2.2.0
+  UIMAGE_NAME := 2.2.2.0
   DEVICE_PACKAGES := ip-full ip-bridge kmod-gpio-button-hotplug tc
 endef
 TARGET_DEVICES += allnet_all-sg8208m