[OpenWrt-Devel] apm821xx: for Meraki MR24 modify BLOCKSIZE to reduce LEB over-allocation

On Meraki MR24, the BLOCKSIZE variable is used to allocate space for the
kernel blob. The LEB size on MR24 is 15.5k (15872 bytes). In the
particular instance observed, it was found that reducing blocksize to
512 bytes resulted in 3 fewer LEBs being allocated to the kernel ubi
volume, with no ill effects.

Signed-off-by: Russell Senior <russell@personaltelco.net>
---
 target/linux/apm821xx/image/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hello,

On Tuesday, September 17, 2019 6:59:28 AM CEST Russell Senior wrote:
> On Meraki MR24, the BLOCKSIZE variable is used to allocate space for the
> kernel blob. The LEB size on MR24 is 15.5k (15872 bytes). In the
> particular instance observed, it was found that reducing blocksize to
> 512 bytes resulted in 3 fewer LEBs being allocated to the kernel ubi
> volume, with no ill effects.
> 
> Signed-off-by: Russell Senior <russell@personaltelco.net>
> ---
>  target/linux/apm821xx/image/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/linux/apm821xx/image/Makefile b/target/linux/apm821xx/image/Makefile
> index 8203de39c5..1aa4e0dad3 100644
> --- a/target/linux/apm821xx/image/Makefile
> +++ b/target/linux/apm821xx/image/Makefile
> @@ -127,7 +127,7 @@ define Device/meraki_mr24
>    DEVICE_PACKAGES := kmod-spi-gpio -swconfig
>    BOARD_NAME := mr24
>    DEVICE_DTS := meraki-mr24
> -  BLOCKSIZE := 63k
> +  BLOCKSIZE := 512
>    IMAGES := sysupgrade.bin
>    DTB_SIZE := 64512
>    IMAGE_SIZE := 8191k

The value looks odd, since UBI volumes are always a multiple of the LEB size
and the documentation we have
<https://openwrt.org/docs/techref/flash.layout#discovery_how_to_find_out>
states that "The erasesize is the block size of the flash [...]".
so in that regard BLOCKSIZE could be 15872 or 16 KiB (if we would stick to
the real, raw value of the flash).

But I don't think a generated image with these parameters would boot anymore.
As the problem here is hidden in "MerakiAdd-dtb" step that is used generate the
KERNEL (for sysupgrade) relies on that  BLOCKSIZE value being a integer divisible
of the 63KiB value. Since this magic value (63KiB) is needed to place the
DTB+kernel at the correct offsets for mkmerakifw.

so, what to do? Because it's possible to get rid of the whole logic as well as the
MR24 portion of the mkmerakifw by refactoring the u-boot boot commands to just load
and boot a multi-file image. The framework is all there since the initramfs image is
already utilizing "MuImage-initramfs". This would save even more since this makes it
possible to also shrink down the DTB as well. As the "raw" information in there is 
just around 10k-15k and the rest of this is fluff. (Some of this fluff is needed by
u-boot though. As it needs some space (probably less than 128 bytes) of this area to
"add" in values for frequencies and ranges. So it can't be completely removed like
with newer u-boots).

So, Would you be willing to do this? 

Regards,
Christian

On Fri, Sep 20, 2019 at 1:35 PM Christian Lamparter <chunkeey@gmail.com>
wrote:

> Hello,
>
> On Tuesday, September 17, 2019 6:59:28 AM CEST Russell Senior wrote:
> > On Meraki MR24, the BLOCKSIZE variable is used to allocate space for the
> > kernel blob. The LEB size on MR24 is 15.5k (15872 bytes). In the
> > particular instance observed, it was found that reducing blocksize to
> > 512 bytes resulted in 3 fewer LEBs being allocated to the kernel ubi
> > volume, with no ill effects.
> >
> > Signed-off-by: Russell Senior <russell@personaltelco.net>
> > ---
> >  target/linux/apm821xx/image/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/linux/apm821xx/image/Makefile
> b/target/linux/apm821xx/image/Makefile
> > index 8203de39c5..1aa4e0dad3 100644
> > --- a/target/linux/apm821xx/image/Makefile
> > +++ b/target/linux/apm821xx/image/Makefile
> > @@ -127,7 +127,7 @@ define Device/meraki_mr24
> >    DEVICE_PACKAGES := kmod-spi-gpio -swconfig
> >    BOARD_NAME := mr24
> >    DEVICE_DTS := meraki-mr24
> > -  BLOCKSIZE := 63k
> > +  BLOCKSIZE := 512
> >    IMAGES := sysupgrade.bin
> >    DTB_SIZE := 64512
> >    IMAGE_SIZE := 8191k
>
> The value looks odd, since UBI volumes are always a multiple of the LEB
> size
> and the documentation we have
> <https://openwrt.org/docs/techref/flash.layout#discovery_how_to_find_out>
> states that "The erasesize is the block size of the flash [...]".
> so in that regard BLOCKSIZE could be 15872 or 16 KiB (if we would stick to
> the real, raw value of the flash).
>

The only thing BLOCKSIZE is used for (afaict, for MR24) is in
bs=$(BLOCKSIZE) while generating the kernel blob (which includes a header
and a device tree binary as well). The DTB_SIZE is to align the device tree
and kernel at particular addresses. The sysupgrade infrastructure
(package/base-files/files/lib/upgrade/nand.sh) on the device deals with ubi
volume creation, and it's going to allocate LEBs to volumes to minimally
accommodate the file that is provided to occupy that volume. We could just
concatenate the kernel with no blocking at all, and it would work fine.
The section you are referring to is talking about the mtd layer, not ubi.


>
> But I don't think a generated image with these parameters would boot
> anymore.
>

Which parameters? I've tested 512 and it boots fine.


> As the problem here is hidden in "MerakiAdd-dtb" step that is used
> generate the
> KERNEL (for sysupgrade) relies on that  BLOCKSIZE value being a integer
> divisible
> of the 63KiB value. Since this magic value (63KiB) is needed to place the
> DTB+kernel at the correct offsets for mkmerakifw.
>

No, the device tree and kernel are placed in the "right spots" (the ones
expected by u-boot) because of the DTB_SIZE variable. There is a header,
the device tree at offset 1k, and the kernel starts at offset 64k.

define Build/MerakiAdd-dtb
        $(call Image/BuildDTB,../dts/$(DEVICE_DTS).dts,$@.dtb)
        ( \
                dd if=$@.dtb bs=$(DTB_SIZE) conv=sync; \
                dd if=$@ bs=$(BLOCKSIZE) conv=sync; \
        ) > $@.new
        @mv $@.new $@
endef

That second dd could be "cat $@" and I think it would work (but haven't
tested), because the blocking of the kernel is not important.


>
> so, what to do? Because it's possible to get rid of the whole logic as
> well as the
> MR24 portion of the mkmerakifw by refactoring the u-boot boot commands to
> just load
> and boot a multi-file image. The framework is all there since the
> initramfs image is
> already utilizing "MuImage-initramfs". This would save even more since
> this makes it
> possible to also shrink down the DTB as well. As the "raw" information in
> there is
> just around 10k-15k and the rest of this is fluff. (Some of this fluff is
> needed by
> u-boot though. As it needs some space (probably less than 128 bytes) of
> this area to
> "add" in values for frequencies and ranges. So it can't be completely
> removed like
> with newer u-boots).
>

It seems like the current u-boot-env variables won't support that and it's
not clear how to guarantee the u-boot-env is set up before you flash the
new image style. It seems much safer to just stick with the current layout
for now.


>
> So, Would you be willing to do this?
>
> Regards,
> Christian
>
>
>

The allocation of LEBs to ubi volumes is handled by the sysupgrade script:

  package/base-files/files/lib/upgrade/nand.sh

and the ubimkvol and or ubirsvol command. Therefore, padding of the
kernel blob is not needed at all, so use cat instead of dd. The
BLOCKSIZE variable was only used in the dd command.  In any case, 63k
made no sense for the way BLOCKSIZE was being used.

63k (64512) does make sense for DTB_SIZE because of the offsets expected
by u-boot given extant u-boot-env variables.

Tested on Meraki MR24.

Signed-off-by: Russell Senior <russell@personaltelco.net>
---

v2: got rid of blocking the kernel blob altogether

 target/linux/apm821xx/image/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/linux/apm821xx/image/Makefile b/target/linux/apm821xx/image/Makefile
index 8203de39c5..108f63cb7a 100644
--- a/target/linux/apm821xx/image/Makefile
+++ b/target/linux/apm821xx/image/Makefile
@@ -58,7 +58,7 @@ define Build/MerakiAdd-dtb
 	$(call Image/BuildDTB,../dts/$(DEVICE_DTS).dts,$@.dtb)
 	( \
 		dd if=$@.dtb bs=$(DTB_SIZE) conv=sync; \
-		dd if=$@ bs=$(BLOCKSIZE) conv=sync; \
+		cat $@ ; \
 	) > $@.new
 	@mv $@.new $@
 endef
@@ -127,7 +127,6 @@ define Device/meraki_mr24
   DEVICE_PACKAGES := kmod-spi-gpio -swconfig
   BOARD_NAME := mr24
   DEVICE_DTS := meraki-mr24
-  BLOCKSIZE := 63k
   IMAGES := sysupgrade.bin
   DTB_SIZE := 64512
   IMAGE_SIZE := 8191k

Chris,

Could you test v2 on a Meraki MX60?  I only have MR24's.

Thanks!