ath79: POC for DT-based expansion-friendly fw #12174
Open
+118
−102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I see a potential issue with this approach, that U-boot sometimes expects ART to be at the end of the flash, for example at Ubiquiti XM boards, and fail to initialize Ethernet. But otherwise, I like the idea. Do you have boot logs, especially from unexpanded device? |
We could initially mark boards like this as having an "expansion-unfriendly bootloader", but expansion-friendly firmware should remain working well on these boards, unexpanded. At least, we could make official firmware work well on some expanded devices again. Expanding a device with an "expansion-unfriendly bootloader" still needs to modify its device tree. We could improve image builder to pack vmlinux and dtb file on the fly , instead of providing pre-packed device-specific kernel image files. (In the past, to generate firmware image for my expanded tl-wdr4900v2, I should manually repack the kernel image for it with a modified dtb file) Bootlog of expanded tl-wdr4900v2: Bootlog of unexpanded tl-wdr4310: |
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
ae17ef6
to
2433df9
Compare
For tp-link devices specifically: It's possible to port and improve the mtdsplit driver from ar71xx to reproduce the old behaviour. Maybe you can try this instead? |
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
2433df9
to
2df1538
Compare
This time special offset values defined in include/linux/mtd/partitions.h of Linux are used to restore the old behavior. Bootlog of expanded tl-wdr4900v2: Bootlog of unexpanded tl-wdr4310: |
|
@InsaneKnight wow, thanks for finding this. This is clever, very clever! |
|
I think that for "classic TP-link" devices we might even go as far, as creating a .dtsi file containing the layout, like this: and include that in all relevant devices. |
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
2df1538
to
57c30a9
Compare
github-actions
bot
added
the
kernel
|
在 2023/3/25 06:47, Lech Perczak 写道:
I think that for "classic TP-link" devices we might even go as far, as creating a .dtsi file containing the layout, like this:
```
&flash {
partitions {
compatible = "fixed-partitions";
#address-cells = <2>;
#size-cells = <1>;
uboot: ***@***.*** {
label = "u-boot";
reg = <0 0x000000 0x020000>;
read-only;
};
***@***.*** {
compatible = "tplink,firmware";
label = "firmware";
// retain 0x10000
// offset = MTDPART_OFS_RETAIN
reg = <(-1) (-3) 0x010000>;
};
art: ***@***.*** {
label = "art";
// offset = MTDPART_OFS_APPEND
reg = <(-1) (-1) 0x010000>;
read-only;
};
};
};
```
and include that in all relevant devices.
Adding relevant defines to Linux upstream somewhere in `include/dt-bindings` could make this cleaner.
Done
|
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
2 times, most recently
from
74e0781
to
c0588bf
Compare
981213
reviewed
Apr 9, 2023
target/linux/generic/hack-5.10/921-add-constants-in-include-linux-mtd-partitions-h.patch
Outdated
Show resolved
Hide resolved
|
@981213 commented on this pull request.
> @@ -0,0 +1,17 @@
+--- /dev/null
++++ b/include/dt-bindings/mtd/partitions.h
+@@ -0,0 +1,14 @@
++/* SPDX-License-Identifier: GPL-2.0-only */
++/*
++ * Device Tree constants identical to those in include/linux/mtd/partitions.h
++ */
++
++#ifndef _DT_BINDINGS_MTD_PARTITIONS_H
++#define _DT_BINDINGS_MTD_PARTITIONS_H
++
++#define MTDPART_OFS_RETAIN (-1) (-3)
Hi!
It isn't a good idea to put two separated values in one definition. It's confusing when reading device tree.
Could you put the original 4 values here instead?
```
#define MTDPART_OFS_RETAIN (-3)
#define MTDPART_OFS_NXTBLK (-2)
#define MTDPART_OFS_APPEND (-1)
#define MTDPART_SIZ_FULL (0)
```
I couldn't. Because in device tree, 64-bit integers should be represented in my way, which I have described in my commit message. Your way can only represent 32-bit integers, which does not work.
|
|
There are codes like this in drivers/mtd/mtdpart.c of Linux:
if (child->part.offset == MTDPART_OFS_APPEND)
if (child->part.offset == MTDPART_OFS_NXTBLK)
if (child->part.offset == MTDPART_OFS_RETAIN)
where `child->part.offset` has uint64_t type, so MTDPART_OFS_APPEND should be treated as (uint64_t)(-1). Other two are similar.
#address-cells = <1>;
reg = <(-1) ...>
can only assign `offset` (uint32_t)(-1), but does ((uint64_t)(uint32_t)(-1) == (uint64_t)(-1))?
|
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
c0588bf
to
49eea6e
Compare
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
49eea6e
to
ad27eb5
Compare
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
ad27eb5
to
f6d7554
Compare
|
> @981213 commented on this pull request.
>
>
>
>> @@ -0,0 +1,17 @@
> +--- /dev/null
> ++++ b/include/dt-bindings/mtd/partitions.h
> +@@ -0,0 +1,14 @@
> ++/* SPDX-License-Identifier: GPL-2.0-only */
> ++/*
> ++ * Device Tree constants identical to those in include/linux/mtd/partitions.h
> ++ */
> ++
> ++#ifndef _DT_BINDINGS_MTD_PARTITIONS_H
> ++#define _DT_BINDINGS_MTD_PARTITIONS_H
> ++
> ++#define MTDPART_OFS_RETAIN (-1) (-3)
>
> Hi!
> It isn't a good idea to put two separated values in one definition. It's confusing when reading device tree.
> Could you put the original 4 values here instead?
> ```
> #define MTDPART_OFS_RETAIN (-3)
> #define MTDPART_OFS_NXTBLK (-2)
> #define MTDPART_OFS_APPEND (-1)
> #define MTDPART_SIZ_FULL (0)
> ```
>
I couldn't. Because in device tree, 64-bit integers should be represented in my way, which I have described in my commit message. Your way can only represent 32-bit integers, which does not work.
Does anyone have a better solution for this?
|
|
> @981213 commented on this pull request.
>
>
>
>> @@ -0,0 +1,17 @@
> +--- /dev/null
> ++++ b/include/dt-bindings/mtd/partitions.h
> +@@ -0,0 +1,14 @@
> ++/* SPDX-License-Identifier: GPL-2.0-only */
> ++/*
> ++ * Device Tree constants identical to those in include/linux/mtd/partitions.h
> ++ */
> ++
> ++#ifndef _DT_BINDINGS_MTD_PARTITIONS_H
> ++#define _DT_BINDINGS_MTD_PARTITIONS_H
> ++
> ++#define MTDPART_OFS_RETAIN (-1) (-3)
>
> Hi!
> It isn't a good idea to put two separated values in one definition. It's confusing when reading device tree.
> Could you put the original 4 values here instead?
> ```
> #define MTDPART_OFS_RETAIN (-3)
> #define MTDPART_OFS_NXTBLK (-2)
> #define MTDPART_OFS_APPEND (-1)
> #define MTDPART_SIZ_FULL (0)
> ```
>
I couldn't. Because in device tree, 64-bit integers should be represented in my way, which I have described in my commit message. Your way can only represent 32-bit integers, which does not work.
Does anyone have a better suggestion for this?
|
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
f6d7554
to
b37d1d7
Compare
|
This PR is now limited only to tplink ath79 targets with "classic flash layout", as tplink_classic_flash_layout.dtsi shows.
|
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
b37d1d7
to
2db5f0f
Compare
|
Is it possible for this patch being included in the next stable release 23.05? If so, what should I do to improve it?
|
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
2db5f0f
to
ac91164
Compare
|
Now device-specific changes are separated into dedicated commits.
|
On former ar71xx, official firmwares could be directly flashed into a device with expanded flash, provided that partitions like "art" have been relocated to the end of the expanded flash, but it does not work well on the current ath79, because the offset of all partitions is set in the device tree. If partitions like "art" are kept in their original offset, flashing an official firmware to an expanded device can work, but extra space beyond the original end will not be usable. However, there are special "offset" and "size" values documented in include/linux/mtd/partitions.h of Linux: // consume as much as possible, leaving size after the end of partition. #define MTDPART_OFS_RETAIN (uint64_t)(-3) // the partition will start at the next erase block. #define MTDPART_OFS_NXTBLK (uint64_t)(-2) // the partition will start where the previous one ended. #define MTDPART_OFS_APPEND (uint64_t)(-1) // the partition will extend to the end of the master MTD device. #define MTDPART_SIZ_FULL (0) When these special values are used, the actual offset and size of a partition will be determined at runtime. As a result, MTDPART_OFS_RETAIN could be used to define the "offset" of the original writable partition. Its "size" is used to retain spaces for read-only partitions after it. (e.g. art) All partitions after it could use MTDPART_OFS_APPEND for its "offset". However, since these special values have uint64_t type, the #address-cells should be 2 and format <high32 low32> should be used, e.g. <(-1) (-3)> for MTDPART_OFS_RETAIN. If official firmwares are built in this way, they will be flashed to a well-expanded device again, and can claim all extra space. Partitions like art could still be relocated to the end of the expanded flash, as many bootloaders expect. The common expansion-friendly layout for "classic" tplink devices is now in tplink_classic_flash_layout.dtsi, which could later be included to the dts file of all such device. Tested on my tplink tl-wdr4310 (unexpanded), and tl-wdr4900v2 (expanded to 16MiB). Signed-off-by: Edward Chow <equu@openmail.cc>
Official firmware image can now be flashed to a device with flash expanded without changing its device tree. Signed-off-by: Edward Chow <equu@openmail.cc>
Official firmware image can now be flashed to a device with flash expanded without changing its device tree. Signed-off-by: Edward Chow <equu@openmail.cc>
Official firmware image can now be flashed to a device with flash expanded without changing its device tree. Signed-off-by: Edward Chow <equu@openmail.cc>
Official firmware image can now be flashed to a device with flash expanded without changing its device tree. Signed-off-by: Edward Chow <equu@openmail.cc>
InsaneKnight
force-pushed
the
wdr4900v2-extensible-flash
branch
from
ac91164
to
6d61b87
Compare
InsaneKnight
commented
Oct 24, 2023
target/linux/generic/hack-5.10/921-add-constants-in-include-linux-mtd-partitions-h.patch
Show resolved
Hide resolved
|
Hi! Would you mind sending the partitions.h patch upstream and see what they think? I don't feel like either approach is good but I don't have any better ideas right now :( |
|
Hi! Would you mind sending the partitions.h patch upstream and see what they think? I don't feel like either approach is good but I don't have any better ideas right now :(
I managed to send the patch here: ***@***.***/
|
I mean sending the kernel patch to devicetree@vger.kernel.org and linux-mtd@lists.infradead.org as an RFC patch for comments. You could check out the detailed process in the kernel documentation: https://docs.kernel.org/process/5.Posting.html#development-posting |
|
@InsaneKnight please CC me in while sending this patch to LKML. |
@Leo-PL |
|
@Leo-PL |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On former ar71xx, official firmwares could be directly flashed into a
device with expanded flash, provided that partitions like "art" have been
relocated to the end of the expanded flash, but it does not work well on
the current ath79, because the offset of all partitions is set in the
device tree. If partitions like "art" are kept in their original offset,
flashing an official firmware to an expanded device can work, but extra
space beyond the original end will not be usable.
However, there are special "offset" and "size" values documented in
include/linux/mtd/partitions.h of Linux:
// consume as much as possible, leaving size after the end of partition.
#define MTDPART_OFS_RETAIN (uint64_t)(-3)
// the partition will start at the next erase block.
#define MTDPART_OFS_NXTBLK (uint64_t)(-2)
// the partition will start where the previous one ended.
#define MTDPART_OFS_APPEND (uint64_t)(-1)
// the partition will extend to the end of the master MTD device.
#define MTDPART_SIZ_FULL (0)
When these special values are used, the actual offset and size of a
partition will be determined at runtime.
As a result, MTDPART_OFS_RETAIN could be used to define the "offset" of the
original writable partition. Its "size" is used to retain spaces for
read-only partitions after it. (e.g. art) All partitions after it could use
MTDPART_OFS_APPEND for its "offset". However, since these special values
have uint64_t type, the #address-cells should be 2 and format
should be used, e.g. <(-1) (-3)> for MTDPART_OFS_RETAIN.
If official firmwares are built in this way, they will be flashed to a
well-expanded device again, and can claim all extra space. Partitions like
art could still be relocated to the end of the expanded flash, as many
bootloaders expect.
Tested on my tplink tl-wdr4310 (unexpanded), and tl-wdr4900v2 (expanded to
16MiB).
Signed-off-by: Edward Chow equu@openmail.cc