[OpenWrt-Devel] [PATCH] [brcm47xx] Belkin F7DXXXX (BCM47XX based) support for Linux 3.18

[Joseph East]

Thanks for the feedback, will have another attempt at submitting the patch. 

> @@ -184,7 +204,7 @@ mtd_fixtrx(const char *mtd, size_t offset)
>   }
>
>   trx = (struct trx_header *) (buf + offset);
> - if (trx->magic != STORE32_LE(0x30524448)) {
> + if (!is_trx_magic(trx->magic)) {
> Is dropping STORE32_LE safe? Should this be sth like
> le32_to_cpu(trx->magic)
> ?
I think this is a misunderstanding, both otrx.c and trx.c were modified
to have a function named is_trx_magic(). trx.c uses the STORE32_LE macro
whereas otrx uses the cpu_to_le32() / le32_to_cpu() methods, the two
files and is_trx_magic() implementations shouldn't be related otherwise. 

>> diff --git a/target/linux/brcm47xx/base-files/lib/upgrade/platform.sh
>> b/target/linux/brcm47xx/base-files/lib/upgrade/platform.sh
>> index cbadefb..9f12715 100644
>> --- a/target/linux/brcm47xx/base-files/lib/upgrade/platform.sh
>> +++ b/target/linux/brcm47xx/base-files/lib/upgrade/platform.sh
>> @@ -51,6 +51,10 @@ platform_expected_image() {
>>    "Linksys WRT310N V2") echo "cybertan 310N"; return;;
>>    "Linksys WRT610N V1") echo "cybertan 610N"; return;;
>>    "Linksys WRT610N V2") echo "cybertan 610N"; return;;
>> +  "Belkin F7D3301") echo "22031020"; return;;
>> +  "Belkin F7D3302") echo "28090920"; return;;
>> +  "Belkin F7D4302") echo "06101020"; return;;
>> +  "Belkin F7D4401")       echo "17850100"; return;;
> I just think if we should follow "type id" logic like "beltrx
> 22031020". I did that for other formats to make sure there won't be
> collisions like two different formats using the same magic. In case of
> Belkin it's a bit tricky because their every ID could be treated as
> separated format.
Have updated to follow this convention, but also realised that the
F7D4401 won't take the generic squashfs trx image (CFE indicates bad
boot block when flashed). Have made an additional change to platform.sh
to check that if a generic trx is detected, verify that the machine
isn't on a 'generic blacklist'. This is indicated by a third field in
platform_expected_image() which should read 'nogeneric'

>> @@ -124,6 +144,25 @@ platform_check_image() {
>>      error=1
>>     fi
>>    ;;
>> +  "beltrx")
>> +   local dev_board_id=$(platform_expected_image)
>> +   local machine=$(platform_machine)
>> +   local magic=$(get_magic_long "$1")
>> +   echo "Found TRX image with Belkin Magic TRX"
>> +   echo "Anticipating a $machine, checking..."
> You don't really need this $machine. And if you think we should print
> it for whatever reason, propose it as global change for all formats.
> I'm not sure if that's needed however.
Removed echoed comment
>> diff --git
>> a/target/linux/brcm47xx/patches-3.18/032-11-MIPS-BCM47XX-Include-Belkin-F7D4XXX-F7D3XXX-TRX-for-mtd.patch
>> b/target/linux/brcm47xx/patches-3.18/032-11-MIPS-BCM47XX-Include-Belkin-F7D4XXX-F7D3XXX-TRX-for-mtd.patch
>> new file mode 100644
>> index 0000000..8e2c1df
>> --- /dev/null
>> +++
>> b/target/linux/brcm47xx/patches-3.18/032-11-MIPS-BCM47XX-Include-Belkin-F7D4XXX-F7D3XXX-TRX-for-mtd.patch
>> @@ -0,0 +1,43 @@
>> +--- a/drivers/mtd/bcm47xxpart.c
>> ++++ b/drivers/mtd/bcm47xxpart.c
>> +@@ -45,6 +45,10 @@
>> + #define TRX_MAGIC   0x30524448
>> + #define SHSQ_MAGIC   0x71736873 /* shsq (weird ZTE H218N endianness) */
>> + #define UBI_EC_MAGIC   0x23494255 /* UBI# */
>> ++#define BELKIN_F7D3301_MAGIC  0x20100322 /* Belkin TRX */
>> ++#define BELKIN_F7D3302_MAGIC  0x20090928
>> ++#define BELKIN_F7D4302_MAGIC  0x20101006
>> ++#define BELKIN_F7D4401_MAGIC  0x00018517
> Please send this patch for mainline inclusion, base it on top of:
> http://git.infradead.org/l2-mtd.git
> I don't want to have more brcm47xx patches noone will ever upstream ;)
Will try and do so, but would this stop this patch from going through if
it isn't in mainline?

For Hauke's comment -

> +static int is_trx_magic(uint32_t magic) {
> +        magic = cpu_to_le32(magic);
> +    if(use_trx_override) {
> +            if(magic == trx_override) {
> +                    return 1;
> +            }
> +            else {
> +                    return 0;
> +            }
> +    }
> +    else {
> +        if(magic == TRX_MAGIC) {
> +            return 1;
> +        }
> +        else {
> +            return 0;
>          
> Make this function return bool and then you can simplify it like this:
> if(use_trx_override)
> 	return magic == trx_override;
> else
> 	return magic == TRX_MAGIC;
>
I was initially working within the constraints of defined libraries,
otrx.c now includes stdbool.h to allow boolean evaluation and this
simplified version. 
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel