[PATCH 1/1] firmware: provide space for firmware fixups

[Heinrich Schuchardt]

On 01.10.20 12:56, Anup Patel wrote:
>
>
>> -----Original Message-----
>> From: opensbi <opensbi-bounces at lists.infradead.org> On Behalf Of Heinrich
>> Schuchardt
>> Sent: 01 October 2020 15:59
>> To: Atish Patra <Atish.Patra at wdc.com>
>> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>; OpenSBI List
>> <opensbi at lists.infradead.org>; Atish Patra <atishp at atishpatra.org>
>> Subject: [PATCH 1/1] firmware: provide space for firmware fixups
>>
>> We expand the provided device tree by up to 1056 bytes in fixup operations.
>> This space must be allocated in the firmware binary.
>>
>> An alternative approach would be to allocate RAM for a copy of the device
>> tree but we do not have library functions for dynamic memory allocation yet.
>>
>> Without this patch devicetree fixups lead to memory corruption.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>>  firmware/fw_payload.S | 2 ++
>>  scripts/d2c.sh        | 3 +++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/firmware/fw_payload.S b/firmware/fw_payload.S index
>> 9805d8c..fdd082f 100644
>> --- a/firmware/fw_payload.S
>> +++ b/firmware/fw_payload.S
>> @@ -108,6 +108,8 @@ fw_options:
>>  	.globl fdt_bin
>>  fdt_bin:
>>  	.incbin	FW_PAYLOAD_FDT_PATH
>> +	/* space for device tree updates */
>> +	. = . + 0x420;
>>  #endif
>
> We should not add arbitrary padding like this. Instead we should have
> separate FW_PAYLOAD_FDT_PADDING options for config.mk.
>
>>
>>  	.section .payload, "ax", %progbits
>> diff --git a/scripts/d2c.sh b/scripts/d2c.sh index 821a995..1c53a71 100755
>> --- a/scripts/d2c.sh
>> +++ b/scripts/d2c.sh
>> @@ -62,6 +62,9 @@ printf "const char __attribute__((aligned(%s)))
>> %s_start[] = {\n" "${OUTPUT_C_AL
>>
>>  od -v -t x1 -An ${INPUT_PATH} | awk '{for (i=1; i<=NF; i++) printf " 0x%s,", $i;
>> printf "\n"; }'
>>
>> +# Add 1056 bytes for device tree fixups echo dummy | awk '{for (j=1;
>> +j<=0x42; j++) {for (i=1; i<=0x10; i++) printf " 0x00,"; printf "\n"}; }'
>> +
>
> Here again, we should not have arbitrary padding. Instead:
> 1) Add optional parameter to d2c.sh for amount of padding required
> 2) In platform/kendryte/k210/objects.mk, add line "platform-padding-k210.o = 1056"

This numbers 1024 and 32 is defined by lib/utils/fdt/fdt_fixup.c. I
understand why defining constants makes sense. These constants may be
used on platform level but should not be defined there but in an include
or common Makefile.

> 3) Extend "compile_d2c" and make rule to convert .dtb to .c file in toplevel Makfile
>
>>  printf "};\n"
>>
>>  printf "const unsigned long %s_size = sizeof(%s_start);\n"
>> "${OUTPUT_C_PREFIX}" "${OUTPUT_C_PREFIX}"

We already have firmware/fw_payload.S including FW_PAYLOAD_FDT_PATH.
Shouldn't we unify the way device trees are included in the firmware. We
could define a new variable defined by FW_PAYLOAD_FDT_PATH with a
fallback to a platform device tree.

This sort of patch should go in first before addressing the fixup issue.

Best regards

Heinrich

>> --
>> 2.28.0
>>
>>
>> --
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
>
> Overall, this needs to be broken down into more patches:
> 1) Patch for adding FW_PAYLOAD_FDT_PADDING option
> 2) Patch for documenting FW_PAYLOAD_FDT_PADDING
> 3) Patch for optional padding parameter to d2c.sh
> 4) Patch to extend "compile_d2c" and make rule in top-level makefile
> 5) Patch to update platform/kendryte/k210/objects.mk
>
> Regards,
> Anup
>