[PATCH] rtl838x: clean up build instructions

[Sander Vanheule]

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