boot0: rework to add heuristics to dram_para printing by libv · Pull Request #3 · apritzel/sunxi-fw

libv · 2024-03-04T20:53:07Z

This reworks everything to be struct based instead of an index in a uint32_t table.

eGON header handling has been reworked.

A valiant attempt was made to add some sensible heuristics to parse dram_para, to enable printing which matches the fex file.

Tested on images for:
a20: Giada ni-a20
a23: q88pro
a31: msi primo 81
a33: astar a33
a523: teclast p26t
a64: bpi m64
a80: generic a80 tv box called cs-q8
a83t: bpi-m3
h3: generic h3 tv box labelled "atv10"
h6: generic h6 tv box
h616: h96max
h616: opi zero3
h700: anbernic 35xxp

apritzel

Hi Luc,
thanks for taking the time to extend and sending this.
So in general this is quite a massive patch, that does multiple things at once and is hard to review or reason about.
So apart from the comments on some IMHO unnecessary changes (inline), I would at least would like to see this patch split up:
First refactor the existing code, if we really need to do so, see below. No further changes, the output would ideally not change.
Then add support for the heuristic, initially just for the two already supported SoCs.
Then add support for the other SoCs.

Thanks,
Andre

apritzel · 2024-03-04T21:08:00Z

sunxi-boot0.c

+
+#define EGON_CHECKSUM_SEED	0x5f0a6c39
+
+struct egon_header {


So why is this reworked as a struct? A struct is a C language construct, and the compiler has some freedom in its layout. Here we are dealing with an externally defined data structure, which I prefer to model as an array.

Because it is a struct in the allwinner code, and a struct vastly improves readability.

Indexes into arrays are a total pain, and impossible to read.

The egon header, and the secondary header are definitely not hw registers, and are (mostly) actual structs in vendor code.

apritzel · 2024-03-04T21:36:06Z

sunxi-boot0.c

+#define EGON_CHECKSUM_SEED	0x5f0a6c39
+
+struct egon_header {
+	uint32_t jump; /* 0x00 - 0x04 */


Giving the offsets (which are the normative information in here) as comments gives away that there is something off.
I understand that using structs to represent hardware register frames or external data structures is somewhat common in many low level project (U-Boot being a prominent example), but in the Linux kernel they are somewhat frowned upon, for instance. I'd like to side with the kernel here.

The offsets are a habit of mine, from all the REing work i have done. They indeed serve no function here.

These structs here are copies, so they are not going to change without us knowing about it (on newer/changed versions), in which case it is easy to add further structs.

Some googling seems to turn up nothing for me on why this is supposedly frowned upon, please provide references.

Oh, re-reading this makes more sense now. I indeed do not access hardware using structs.

But these values are not register space, they are a data structure at the start of an early bit of bootloader binary. This data structure may have been used as a register value table (which i have been fighting all my life, this is why the world has modesetting) in the early days, they are not used as such any more apparently.

We here are using the struct to make this all more readable and hackable.

From the A13 code in https://github.com/allwinner-zh/bootloader it is clear that:
dram_baseaddr is unused.
dram_clk is just the n factor of the pll, and just 5bits of the pll register.
dram_type is used in two different places to poke 2 different registers.
Once combined with dram_io_width and dram_chip_density and other bits to write the SDR_DCR register.
Once combined with the dram_cas value and other bits to set the SDR_MR register.

Only a few of the values in this struct are written to the hw directly.

apritzel · 2024-03-04T21:42:22Z

sunxi-boot0.c

+	void *dram_param;
+	int ret;
+
+	if (memcmp(header->magic, BOOT0_MAGIC_0, 8) > 0) {


When would this ever trigger? output_boot0_info() is only called when we already know this is an eGON image, because we have detected the magic.

You only look for "eGON" in the upper level, i also check for ".BT0" here, hoping to trigger a bug report for ".BT1", as i have not seen one yet.

This statement is apparently false and based on a too cursory reading of the upper level code.

I still retain this check as it makes the header check more complete and self-contained, so future uses of this code (if any) can straight reuse this. This is not a high-performance tool or a high-performance path, so an extra memcmp does not hurt.

apritzel · 2024-03-04T21:55:23Z

sunxi-boot0.c

+ */
+#define DRAM_PARAM_A10_MATCHES "A10/A10s/A13/A20"
+
+struct dram_param_a10 {


I am not very excited about this extra struct definition, for each SoC and just a single use. For a start, the same arguments against structs as above apply here as well, but on top this is really all uint32_t, so really an array would be as good. And if I see it correctly, you just print those members out below, right? So I don't immediately see what those names add here.

The sed/awk-crafted printfs allow for extra comments useful to the user. cfr the h616 printing.

This is also not per SoC, it is per group of SoCs for which this dram para struct was commonly seen. All in all, we have 4.

Since i am verifying some values to figure out which structure is likely relevant, just referencing struct members makes it easy and very readable.

apritzel · 2024-03-04T21:57:02Z

sunxi-boot0.c

+
+	/* This is a base address, should be 0x40000000 */
+	if (param->baseaddr & 0x0FFFFFFF) {
+		fprintf(stream, "%s: wrong baseaddr: 0x%08X\n", message,


It feels a bit odd to print an error message in a predicate function. So if I let this run on an H616 image, it would print all those scary message for all the SoCs it tried first?
One pattern I used elsewhere is to only print if *stream is not NULL, that allows the same function to be used silently, or in verbose mode.

The dram_para validation and printing happens inside if (verbose) {} in the top level function in this file.

You have not run this code it seems. Here is some sample output.

libv@quoth:~/sunxi/opi_zero3$ ../sunxi-fw/sunxi-fw info -v OrangePi_Zero3_Android12_v1.0.img
@ 0: wty: PhoenixSuite image file
header v3.0, 45 images, 1186 MB
...
@ 540: boot0: Allwinner boot0
Found eGON header.
Boot0 Filesize is 60kB.
eGON checksum matches.

Looking for a valid dram parameter structure...
Invalid structure for A10/A10s/A13/A20: wrong baseaddr: 0x00000318
Invalid structure for H6: wrong type: 0x00000008
Invalid structure for A31/A23/A33/A83T/A64/H3: wrong type: 0x00000008

; For H616/H700/A523
[dram para]

dram_clk = 792,
dram_type = 8,
dram_dx_odt = 0x07070707,
dram_dx_dri = 0x0E0E0E0E,
dram_ca_dri = 0x00000E0E,
dram_para0 = 0xAAAAEEEE, ; aka odt_en on H616/H700
dram_para1 = 0x000030FA,
dram_para2 = 0x00000000,
dram_mr0 = 0x0,
dram_mr1 = 0x34,
dram_mr2 = 0x1B,
dram_mr3 = 0x33,
dram_mr4 = 0x3,
dram_mr5 = 0x0,
dram_mr6 = 0x0,
dram_mr11 = 0x4,
dram_mr12 = 0x72,
dram_mr13 = 0x0,
dram_mr14 = 0x9,
dram_mr16 = 0x0,
dram_mr17 = 0x0,
dram_mr22 = 0x24,
dram_tpr0 = 0x00000000,
dram_tpr1 = 0x0,
dram_tpr2 = 0x0,
dram_tpr3 = 0x0,
dram_tpr6 = 0x35808080,
dram_tpr10 = 0x402F6663,
dram_tpr11 = 0x36363535,
dram_tpr12 = 0x10101110,
dram_tpr13 = 0x2080C60,
dram_tpr14 = 0x0, ; should be 0 on anything but A523

I have now extended this checking printing:

Looking for a valid dram parameter structure...
Invalid structure for A10/A10s/A13/A20: wrong baseaddr: 0x00000288
Invalid structure for H6: wrong odt_en: 0x0E0E0E0E
Invalid structure for A31/A23/A33/A83T/A64/H3: wrong odt_en: 0x0E0E0E0E
Parameters seem valid for H616/H700/A523.

This way the reader will see confirmation that the printed result is (likely) valid.

apritzel · 2024-03-04T22:11:57Z

sunxi-boot0.c

+
+	fprintf(stream, "\n; %s\n", DRAM_PARAM_A10_MATCHES);
+	fprintf(stream, "[dram para]\n\n");
+	fprintf(stream, "dram_baseaddr\t   = 0x%x\n", param->baseaddr);


so the pattern you use here is to have: printf("param0\t = %x\n", param0);
Which I am not sure is helpful to spell out for every parameter. That's where the index approach is better, I believe, since it allows to do this in a SoC agnostic loop, programmatically.
Also the underlying "truth" is the offset into the data structure, which is hidden implicitly above, in the definition of struct dram_param_a10.
If you really need to print out each parameter individually for the A10 class SoCs, you could make this easier with:
printf("param0\t = %x\n", dram_para[x]);
which would automatically document the relation between the parameter name and the offset x in one line.

Again, the printfs were sed/awk crafted from the struct. There are only four sets, and a fallback array printing function. Once such printfs are written out, you can add comments to further guide the user of this information, as visible in the h616 version.

I find the printed result easy to read, and meant to mimic the fex file. Much much easier than the sparse matrix currently used.

apritzel · 2024-03-04T22:14:28Z

sunxi-boot0.c


-static int8_t register_mappings[NR_SOC_TYPES][NR_DRAM_PARAMS] = {
-	[SOC_A64] = {
-		[DRAM_CLK] = OFS(0),


if you don't like this format, we can try to write those assignment easier or more readable. I had it originally transposed, so to just enumerate the field names:
{ DRAM_CLK, DRAM_TYPE, DRAM_ZQ, ...
but that turned out to be worse in the actual code.
Maybe this can be revisited if need be.

Again, i find structs cleaner, faster and more readable. It seems that you fundamentally are against structs for parsing any binary, and that we will always disagree on this.

apritzel · 2024-03-04T22:16:07Z

sunxi-boot0.c

+}
+
+/*
+ * If we return 0, we tell the upper layer to skip only 1 sector.


The actual semantic is: returns how many sectors you have forwarded the stream by, so either read or seeked over.

Comment removed in new version.

apritzel · 2024-03-04T23:05:42Z

sunxi-boot0.c

+	}
+
+	/* Return to start of the boot0 data. */
+	ret = fseek(inf, -512, SEEK_CUR);


No. The whole program is designed to work on a stream, not only on seekable files. So no backwards seeking. The input file could be the output of a decompressor, or piped in via network.
The idea is that the sector that was just read is stored in the buffer *sector points to. If you need more data, you can read more or seek forward, but otherwise just use what's already there.

Ok. That explains why mmap() was not used, which is my preferred usage of anything binary.

Fixed by passing the first sector to the checksum code and handling it separately. This separate addition loop now includes the header->checksum skip, so we totally avoid checking for this in the fread() loop.

This will be replaced with dram_param structure validation and fex style printing at the end of this series. Signed-off-by: Luc Verhaegen <libv@skynet.be>

The __maybe_unused header printing functions were used in the creation of this new code, and can be used in future to help debug header changes/issues. So there is no point in making them go away. Signed-off-by: Luc Verhaegen <libv@skynet.be>

Signed-off-by: Luc Verhaegen <libv@skynet.be>

The validation code needs to run before the A31 validation code. Signed-off-by: Luc Verhaegen <libv@skynet.be>

Signed-off-by: Luc Verhaegen <libv@skynet.be>

apritzel requested changes Mar 4, 2024

View reviewed changes

libv added 11 commits March 7, 2024 12:55

boot0: drop dram parsing and print raw uint32s

bde8e4d

This will be replaced with dram_param structure validation and fex style printing at the end of this series. Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: introduce egon header structs

415fc01

The __maybe_unused header printing functions were used in the creation of this new code, and can be used in future to help debug header changes/issues. So there is no point in making them go away. Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: replace 512 with SECTOR_SIZE

e07094a

Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: move checksum calculation out to its own function

d618bf4

Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: rewrite egon0_checksum_verify()

7550b1e

Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: output_boot0_info: move verbose conditional

5cf323a

Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: increase egon header verification

247991e

Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: add dram param checking and printing for a10 and relations

a066d0a

Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: add dram param checking and printing for a31 and relations

77a4e55

Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: add dram param checking and printing for h6 and relations

1f35ff5

The validation code needs to run before the A31 validation code. Signed-off-by: Luc Verhaegen <libv@skynet.be>

boot0: add dram param checking and printing for h616 and relations

1ba5f4c

Signed-off-by: Luc Verhaegen <libv@skynet.be>

libv force-pushed the main branch from feccad1 to 1ba5f4c Compare March 7, 2024 12:07

Conversation

libv commented Mar 4, 2024

Uh oh!

apritzel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

libv Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

libv Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

libv Mar 4, 2024 •

edited

Loading

libv Mar 5, 2024 •

edited

Loading