fel: Introduce 'sid-dump' #189

qianfan-Zhao · 2023-02-10T09:34:15Z

A sample output from allwinner T3:

$ ./sunxi-fel sid-dump
chipid          1300000c 02404700 79350400 5c761e0c
in              00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
ssk             00000000 00000000 00000000 00000000
thermal         08320834
ft_zone         00000000 00000000
tvout           010002b0 00f7029b 00f9029f 00fb02a5
rssk            00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
hdcp_hash       00000000 00000000 00000000 00000000
reserved        00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
                00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
                00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
                00000000 00000000 00000000 00000000

apritzel

Hi Qianfan,
thanks for the patch, looks like a useful addition!
Can you please add some more text to the commit message, maybe including a part of the pull request message?
And we would need the new command mentioned in the internal help text, ideally also in the manpage.
Some comments inside.

apritzel · 2023-02-10T22:29:13Z

fel.c

+	for (const sid_section *s = soc_info->sid_sections; s->name; s++) {
+		uint32_t count = s->size_bits / 32;
+
+		if (count > sizeof(buffer) / sizeof(buffer[0])) {


In common.h we have ARRAY_SIZE() for this. But in general this looks a bit odd to check this at all, since there is an upper limit (the 2KB SID storage size) and the input size comes from an internal struct, so we don't need to defend against user input.

apritzel · 2023-02-10T22:34:29Z

fel_lib.c

+bool fel_get_sid(feldev_handle *dev, uint32_t *result, uint32_t offset,
+		 size_t count)
+{
+	if (!dev->soc_info->sid_base) {


This check looks unnecessary, since we check this before calling this function.

OK, removed

apritzel · 2023-02-10T22:36:15Z

fel_lib.c

+{
+	if (!dev->soc_info->sid_base) {
+		for (unsigned i = 0; i < count; i++)
+			result[i] = 0;


First this would be a job for memset(). But why would we want to clear the result before we bail out? In fact I would expect a function to not touch a result buffer in case of an early error. Also the caller doesn't seem to need this, so remove that clearing part.

Both the check for sid_base and the clear function are removed now.

apritzel · 2023-02-10T22:36:40Z

soc_info.c

@@ -507,3 +521,4 @@ void get_soc_name_from_id(soc_name_t buffer, uint32_t soc_id)
 	/* unknown SoC (or name string missing), use the hexadecimal ID */
 	snprintf(buffer, sizeof(soc_name_t) - 1, "0x%04X", soc_id);
 }
+


stray extra line

apritzel

Thanks for the quick changes, looks alright now. Tested on a Bananapi M2 Berry. I also tested with some A64 and H6 maps, will send them later.
I will let this sit for a few more days, in case people have comments.

smaeul · 2023-02-14T01:27:03Z

fel_lib.c

+		 size_t count)
+{
+	fel_readl_n(dev, dev->soc_info->sid_base
+		    + dev->soc_info->sid_offset + offset, result, count);


Should this use the register readout method on SoCs where that is required?

It's the better. I've considered it, but it's too hard to write assembly code.

So it looks like fel_get_sid_registers() can be easily extended to read registers from offset instead of 4 registers from offset 0. Then we could just extend fel_get_sid_root_key() to allow parameters, and use that for both sid-dump and for aw_fel_print_sid(), avoiding a new function.
@qianfan-Zhao do you want to do this? If not, I can do it in a follow up patch.

@apritzel It is hard to me. Could you please submit some patchs after this pr merged?

Ok, will do.

apritzel

Just two minor things I noticed about the output formatting.

apritzel · 2023-02-21T10:58:22Z

fel.c

+		for (uint32_t i = 0; i < count; i++) {
+			if (i > 0 && ((i % 8) == 0)) {
+				putchar('\n');
+				printf("%-16s", "");


Just saw this on a final inspection: can you merge those two into one line:

printf("\n%-16s", "");

Then you can get rid of the brackets as well.

apritzel · 2023-02-21T10:59:22Z

fel.c

+				putchar('\n');
+				printf("%-16s", "");
+			}
+			printf("%08x ", buffer[i]);


if you decrease the indent from 16 to 15 in the tow lines above, you can put the space here first, before the number, and avoid a trailing space on the last word.

The sid memory maps are copied from allwinner 3.10 bsp. Next is a sample output from allwinner T3: $ sunxi-fel sid-dump chipid 1300000c 02c04700 79350400 30791acc in 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 ssk 00000000 00000000 00000000 00000000 thermal 0823081c ft_zone 00000000 00000000 tvout 00ff02ad 00f8029e 00f0028d 00f902a2 rssk 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 hdcp_hash 00000000 00000000 00000000 00000000 reserved 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 Signed-off-by: qianfan Zhao <[email protected]>

apritzel requested changes Feb 10, 2023

View reviewed changes

qianfan-Zhao force-pushed the sid-dump branch from 74b273d to 65192d1 Compare February 11, 2023 02:24

qianfan-Zhao requested a review from apritzel February 13, 2023 00:47

apritzel approved these changes Feb 14, 2023

View reviewed changes

smaeul reviewed Feb 14, 2023

View reviewed changes

apritzel reviewed Feb 21, 2023

View reviewed changes

qianfan-Zhao force-pushed the sid-dump branch from 65192d1 to 23bb103 Compare February 22, 2023 01:34

apritzel merged commit 056b65f into linux-sunxi:master Feb 22, 2023

qianfan-Zhao mentioned this pull request Apr 10, 2024

fel: sid: add sid-writel command #206

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fel: Introduce 'sid-dump' #189

fel: Introduce 'sid-dump' #189

qianfan-Zhao commented Feb 10, 2023 •

edited

Loading

apritzel left a comment •

edited

Loading

apritzel Feb 10, 2023

qianfan-Zhao Feb 11, 2023

apritzel Feb 10, 2023

qianfan-Zhao Feb 11, 2023

apritzel Feb 10, 2023

qianfan-Zhao Feb 11, 2023

apritzel Feb 10, 2023

apritzel left a comment

smaeul Feb 14, 2023

qianfan-Zhao Feb 14, 2023

apritzel Feb 20, 2023

qianfan-Zhao Feb 21, 2023

apritzel Feb 21, 2023

apritzel left a comment

apritzel Feb 21, 2023

qianfan-Zhao Feb 22, 2023

apritzel Feb 21, 2023

qianfan-Zhao Feb 22, 2023

fel: Introduce 'sid-dump' #189

fel: Introduce 'sid-dump' #189

Conversation

qianfan-Zhao commented Feb 10, 2023 • edited Loading

apritzel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apritzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apritzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qianfan-Zhao commented Feb 10, 2023 •

edited

Loading

apritzel left a comment •

edited

Loading