[PATCH v2 2/2] ath79: airtight c-75: use second flash chip

[Tomasz Maciej Nowak]

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 at lists.openwrt.org]
>> On Behalf Of Tomasz Maciej Nowak
>> Sent: Dienstag, 15. Dezember 2020 18:17
>> To: openwrt-devel at lists.openwrt.org
>> Cc: Vladimir Georgievsky <vladimir.georgievsky at 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 at yahoo.com>
>> Signed-off-by: Tomasz Maciej Nowak <tmn505 at 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 at 0 {
>> +				label = "firmware";
>> +				reg = <0x0 0x1f90000>;
>> +				compatible = "denx,uimage";
>> +			};
>> +		};
>> +	};
>>  };
>>
>>  &eth0 {
>> @@ -120,10 +137,8 @@
>>  				read-only;
>>  			};
>>
>> -			partition at 60000 {
>> -				label = "firmware";
>> +			concat0: partition at 60000 {
>>  				reg = <0x060000 0xf90000>;
>> -				compatible = "denx,uimage";
>>  			};
>>
>>  			art: partition at ff0000 {
>> @@ -144,8 +159,7 @@
>>  			#address-cells = <1>;
>>  			#size-cells = <1>;
>>
>> -			partition at 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 at 0 {
>>  				reg = <0x0 0x1000000>;
>>  			};
>>  		};

Regards

-- 
TMN