[OpenWrt-Devel] [PATCH v3 1/2] ramips: fix RBM33G partitioning

[Rafał Miłecki]

On 04.08.2018 15:44, Thibaut VARÈNE wrote:
> This patch improves 5684d087418d176cfdef4e045e1950ca7ba3b09f by correcting
> the partition scheme for the "RouterBoot" section of the flash.
> 
> The partition scheme initially submitted is incorrect and does not reflect
> the actual flash structure.
> 
> The "RouterBoot" section (name matching OEM) is subdivided in several
> static segments, as they are on ar71xx RB devices albeit with different
> offsets and sizes.
> The naming convention from ar71xx has been preserved, except for the
> bootloaders which are named "bootloader1" and "bootloader2" to avoid
> confusion with the master "RouterBoot" partition.
> The preferred 'fixed-partitions' DTS node syntax is used, with nesting
> support as introduced in 2a598bbaa3.
> "partition" is used for node names, with associated "label" to match
> policy set by 6dd94c2781.
> 
> The OEM source code also define a "RouterBootFake" partition at the
> beginning of the secondary flash chip: to avoid trouble if OEM ever makes
> use of that space, it is also defined here.

My review & comments:

1) Adding "RouterBoot" partition
This seems correct as from what I read:
a) It matches how vendor describes / names that flash region
b) It allow to use "rbcfg" tool which expects that partition
c) It allows following OEM dump procedure

2) Nesting partitions in DTS
It's definitely correct as it follows what was agreed by upstream mtd
developers and DT guys (including Rob) after years of discussing that
over and over.
For details see d2ad00eb7879 ("dt-bindings: mtd: explicitly document
nesting partitions descriptions"):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d2ad00eb78792b396a6d012f15d6297a1701b8bc

3) Using "fixed-partitions" for subpartitions
Sounds good since these subpartitions (bootloaders, bios, config) are
likely to be always located as the same regions.
If we ever get a dynamic parser developed and for some reason it will be
more reliable than hardocded offsets & sizes, we can always switch DTS
to specify that parser supported format.

4) Subpartitions of "RouterBoot" partition
Look sane, "read-only;" present where expected.

5) "RouterBootFake" partition
I don't see why it should be defined. If it is empty and it looks like
just a reserved region, we should simply *not* create any partition for
it.
Maybe vendor was using some hacky MTD method for creating "firmware"
partition and it needed to cover region before that with some /fake/
partition?
Anyway it seems to me that one should *not* be added.

I believe I reviewed every part of this patch and at this point only
one change doesn't seem to be needed.

Anything else I missed?

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel