| Message ID | 20201215171709.3082-2-tmn505@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] ath79: add support for AirTight C-75 | expand |
Hi again, one comment and a slightly conceptual question: > -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] > On Behalf Of Tomasz Maciej Nowak > Sent: Dienstag, 15. Dezember 2020 18:17 > To: openwrt-devel@lists.openwrt.org > Cc: Vladimir Georgievsky <vladimir.georgievsky@yahoo.com> > Subject: [PATCH v2 2/2] ath79: airtight c-75: use second flash chip > > The flash capacity is divided in two flash chips and currently only first is used. > Increase available space for OpenWrt by additional 16 MiB using mtd-concat > driver. Because U-Boot might not be able to load kernel image spanned > through two flash chips, the size of kernel is limited to space available on first > chip. > > Cc: Vladimir Georgievsky <vladimir.georgievsky@yahoo.com> > Signed-off-by: Tomasz Maciej Nowak <tmn505@gmail.com> > --- > v1 -> v2 > > - add kernel size constraints > > .../linux/ath79/dts/qca9550_airtight_c-75.dts | 24 +++++++++++++++---- > target/linux/ath79/image/generic.mk | 3 ++- > 2 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/target/linux/ath79/dts/qca9550_airtight_c-75.dts > b/target/linux/ath79/dts/qca9550_airtight_c-75.dts > index 34d4c32b3562..c380a109c96b 100644 > --- a/target/linux/ath79/dts/qca9550_airtight_c-75.dts > +++ b/target/linux/ath79/dts/qca9550_airtight_c-75.dts > @@ -41,6 +41,23 @@ > linux,default-trigger = "phy1tpt"; > }; > }; > + > + mtd-concat { I think I will change the node name to virtual_flash, as that's the most common case so far and I personally like it more. > + compatible = "mtd-concat"; > + devices = <&concat0 &concat1>; > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "firmware"; > + reg = <0x0 0x1f90000>; > + compatible = "denx,uimage"; > + }; > + }; > + }; > }; > > ð0 { > @@ -120,10 +137,8 @@ > read-only; > }; > > - partition@60000 { > - label = "firmware"; > + concat0: partition@60000 { > reg = <0x060000 0xf90000>; > - compatible = "denx,uimage"; > }; > > art: partition@ff0000 { > @@ -144,8 +159,7 @@ > #address-cells = <1>; > #size-cells = <1>; > > - partition@0 { > - label = "opt"; I wonder what's best practice here: Many devices keep a label here and just use names like "firmware1", "firmware2" or similar. Does it make sense to keep the concatenated partitions available under such a name (because the user might want to do something with it) or would it be better to remove the label and thus "hide" the partition (because the user might want to do something with it)? Best Adrian > + concat1: partition@0 { > reg = <0x0 0x1000000>; > }; > }; > diff --git a/target/linux/ath79/image/generic.mk > b/target/linux/ath79/image/generic.mk > index 177caafa2253..bdc35823c66c 100644 > --- a/target/linux/ath79/image/generic.mk > +++ b/target/linux/ath79/image/generic.mk > @@ -246,7 +246,8 @@ define Device/airtight_c-75 > DEVICE_ALT1_VENDOR := WatchGuard > DEVICE_ALT1_MODEL := AP320 > DEVICE_PACKAGES := ath10k-firmware-qca988x kmod-ath10k-ct kmod- > usb2 > - IMAGE_SIZE := 15936k > + IMAGE_SIZE := 32320k > + KERNEL_SIZE := 15936k > endef > TARGET_DEVICES += airtight_c-75 > > -- > 2.29.2 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
W dniu 16.12.2020 o 16:49, Adrian Schmutzler pisze: > Hi again, > > one comment and a slightly conceptual question: > >> -----Original Message----- >> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] >> On Behalf Of Tomasz Maciej Nowak >> Sent: Dienstag, 15. Dezember 2020 18:17 >> To: openwrt-devel@lists.openwrt.org >> Cc: Vladimir Georgievsky <vladimir.georgievsky@yahoo.com> >> Subject: [PATCH v2 2/2] ath79: airtight c-75: use second flash chip >> >> The flash capacity is divided in two flash chips and currently only first is used. >> Increase available space for OpenWrt by additional 16 MiB using mtd-concat >> driver. Because U-Boot might not be able to load kernel image spanned >> through two flash chips, the size of kernel is limited to space available on first >> chip. >> >> Cc: Vladimir Georgievsky <vladimir.georgievsky@yahoo.com> >> Signed-off-by: Tomasz Maciej Nowak <tmn505@gmail.com> >> --- >> v1 -> v2 >> >> - add kernel size constraints >> >> .../linux/ath79/dts/qca9550_airtight_c-75.dts | 24 +++++++++++++++---- >> target/linux/ath79/image/generic.mk | 3 ++- >> 2 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/target/linux/ath79/dts/qca9550_airtight_c-75.dts >> b/target/linux/ath79/dts/qca9550_airtight_c-75.dts >> index 34d4c32b3562..c380a109c96b 100644 >> --- a/target/linux/ath79/dts/qca9550_airtight_c-75.dts >> +++ b/target/linux/ath79/dts/qca9550_airtight_c-75.dts >> @@ -41,6 +41,23 @@ >> linux,default-trigger = "phy1tpt"; >> }; >> }; >> + >> + mtd-concat { > > I think I will change the node name to virtual_flash, as that's the most common case so far and I personally like it more. I'm fine with Your proposal. > >> + compatible = "mtd-concat"; >> + devices = <&concat0 &concat1>; >> + >> + partitions { >> + compatible = "fixed-partitions"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + partition@0 { >> + label = "firmware"; >> + reg = <0x0 0x1f90000>; >> + compatible = "denx,uimage"; >> + }; >> + }; >> + }; >> }; >> >> ð0 { >> @@ -120,10 +137,8 @@ >> read-only; >> }; >> >> - partition@60000 { >> - label = "firmware"; >> + concat0: partition@60000 { >> reg = <0x060000 0xf90000>; >> - compatible = "denx,uimage"; >> }; >> >> art: partition@ff0000 { >> @@ -144,8 +159,7 @@ >> #address-cells = <1>; >> #size-cells = <1>; >> >> - partition@0 { >> - label = "opt"; > > I wonder what's best practice here: > > Many devices keep a label here and just use names like "firmware1", "firmware2" or similar. This example doesn't explicitly state that these are part of single firmware, users could think there's dual boot system. More appropriate names would be "firmware_part1" and "firmware_part2". > > Does it make sense to keep the concatenated partitions available under such a name (because the user might want to do something with it) or would it be better to remove the label and thus "hide" the partition (because the user might want to do something with it)? In case of no labels, mtd system assigns it from node name (minus the address), so now we have two partitions named "partition", which is not optimal. Worse is that the mtd-concat driver does not hide or mark the concatenated partitions as read-only. I could erase the "partition" on second chip without problems. I'll set labels for both partitions as "reserved1" and "reserved2", so that'll make it a bit more clearer to not touch them. Fell free to adjust them to Your liking, I won't complain. > > Best > > Adrian > >> + concat1: partition@0 { >> reg = <0x0 0x1000000>; >> }; >> }; Regards
diff --git a/target/linux/ath79/dts/qca9550_airtight_c-75.dts b/target/linux/ath79/dts/qca9550_airtight_c-75.dts index 34d4c32b3562..c380a109c96b 100644 --- a/target/linux/ath79/dts/qca9550_airtight_c-75.dts +++ b/target/linux/ath79/dts/qca9550_airtight_c-75.dts @@ -41,6 +41,23 @@ linux,default-trigger = "phy1tpt"; }; }; + + mtd-concat { + compatible = "mtd-concat"; + devices = <&concat0 &concat1>; + + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "firmware"; + reg = <0x0 0x1f90000>; + compatible = "denx,uimage"; + }; + }; + }; }; ð0 { @@ -120,10 +137,8 @@ read-only; }; - partition@60000 { - label = "firmware"; + concat0: partition@60000 { reg = <0x060000 0xf90000>; - compatible = "denx,uimage"; }; art: partition@ff0000 { @@ -144,8 +159,7 @@ #address-cells = <1>; #size-cells = <1>; - partition@0 { - label = "opt"; + concat1: partition@0 { reg = <0x0 0x1000000>; }; }; diff --git a/target/linux/ath79/image/generic.mk b/target/linux/ath79/image/generic.mk index 177caafa2253..bdc35823c66c 100644 --- a/target/linux/ath79/image/generic.mk +++ b/target/linux/ath79/image/generic.mk @@ -246,7 +246,8 @@ define Device/airtight_c-75 DEVICE_ALT1_VENDOR := WatchGuard DEVICE_ALT1_MODEL := AP320 DEVICE_PACKAGES := ath10k-firmware-qca988x kmod-ath10k-ct kmod-usb2 - IMAGE_SIZE := 15936k + IMAGE_SIZE := 32320k + KERNEL_SIZE := 15936k endef TARGET_DEVICES += airtight_c-75
The flash capacity is divided in two flash chips and currently only first is used. Increase available space for OpenWrt by additional 16 MiB using mtd-concat driver. Because U-Boot might not be able to load kernel image spanned through two flash chips, the size of kernel is limited to space available on first chip. Cc: Vladimir Georgievsky <vladimir.georgievsky@yahoo.com> Signed-off-by: Tomasz Maciej Nowak <tmn505@gmail.com> --- v1 -> v2 - add kernel size constraints .../linux/ath79/dts/qca9550_airtight_c-75.dts | 24 +++++++++++++++---- target/linux/ath79/image/generic.mk | 3 ++- 2 files changed, 21 insertions(+), 6 deletions(-)