[STM32F446]: st-flash --area=option read out.bin

[ApiumJacob]

• [x] I made serious effort to avoid creating duplicate or nearly similar issue, related to issue #1155 and #1141
• [X] Programmer/board type: [STLINK/V2]
• [X] Operating system an version: [Linux 5.4.0-74-generic 83~18.04.1-Ubuntu SMP x86_64 x86_64 x86_64 GNU/Linux]
• [X] stlink tools version and/or git commit hash: [v1.7.0 g179650d] built from source
• [X] stlink commandline tool name: [st-flash --area=option read]
• [X] Target chip (and board, if applicable): [STM32F446RE custom hardware]

The command line output differs from values written to a file that is passed on the command line. The following three examples are output of three different runs. I may be interpreting the generated output file incorrectly and this may not be an issue at all. Writing option bytes from the command line seems to work fine, didn't test writing from a file (didn't wan't to brick my chip).

Command line output:

st-flash --area=option read
st-flash 1.7.0
2021-06-22T21:49:10 INFO common.c: F446: 128 KiB SRAM, 4096 KiB flash in at least 128 KiB pages.
8f00bbed

$ st-flash --area=option read out.bin
st-flash 1.7.0
2021-06-22T21:50:20 INFO common.c: F446: 128 KiB SRAM, 4096 KiB flash in at least 128 KiB pages.
2021-06-22T21:50:20 INFO common.c: read from address 0x1fffc000 size 4
$ hexdump out.bin 
0000000 0080 1610                              
0000004

st-flash --area=option read out.bin 4
st-flash 1.7.0
2021-06-22T21:51:52 INFO common.c: F446: 128 KiB SRAM, 4096 KiB flash in at least 128 KiB pages.
2021-06-22T21:51:52 INFO common.c: read from address 0x1fffc000 size 4
$ hexdump out.bin 
0000000 0080 1610                              
0000004

[Nightwalker-87]

@Ant-ON @gszy Can one of you explain this behaviour based on a short review? This might not be a bug, but a improper implementation related to a missing parameter instead...

[gszy]

Well, it seems that when the filename is not provided, the program prints the result of stlink_read_option_bytes32():

https://github.com/stlink-org/stlink/blob/db8f789400d0ac19536228b0bd774ac8f1d60ed6/src/st-flash/flash.c#L210-L215

It results in the following calls:

• stlink_read_option_bytes32()
  • stlink_read_option_bytes_f4()
    • stlink_read_option_control_register_f4()
      • stlink_read_debug32(sl, FLASH_F4_OPTCR, …) (where FLASH_F4_OPTCR = 0x40023c14, which is the actual memory address of the register)
        • sl->backend->read_debug32(sl, FLASH_F4_OPTCR, …)

Based on the STM32F446xx RM, I think 0x8f00bbed looks like a reasonable value.

---

On the other hand, when the filename is provided, the program calls stlink_fread() instead:

https://github.com/stlink-org/stlink/blob/db8f789400d0ac19536228b0bd774ac8f1d60ed6/src/st-flash/flash.c#L207

It results in the following calls:

• stlink_fread()
  • stlink_read(sl, 0x1fffc000, 4, …, …) (where 0x1fffc000 is the memory address of the option bytes)
    • stlink_read_mem32(sl, …, …)
      • sl->backend->read_mem32(sl, …, …)

Not digging too deep into these functions, I wonder if we actually want to read four bytes from 0x1fffc000. If I understand the RM correctly (Table 8 and Table 9), there are actually 16 option bytes:

Byte	Description
0x1fffc000	reserved
0x1fffc001	reserved
0x1fffc002	reserved
0x1fffc003	reserved
0x1fffc004	reserved
0x1fffc005	reserved
0x1fffc006	RDP
0x1fffc007	USER
0x1fffc008	reserved
0x1fffc009	reserved
0x1fffc00a	reserved
0x1fffc00b	reserved
0x1fffc00c	reserved
0x1fffc00d	reserved
0x1fffc00e	SPRMOD; reserved
0x1fffc00f	nWRP

[Nightwalker-87]

Looks to me as if at least the wrong address is read. Why should we read address fields that are reserved? I've read before in another context that this can result in strange behaviour. To me it would be reasonable to read the allocated and defined fields...

[gszy]

We currently seem to read four bytes starting from 0x1fffc000, because that’s the way STM32F446’s option bytes are defined in the lib:

https://github.com/stlink-org/stlink/blob/db8f789400d0ac19536228b0bd774ac8f1d60ed6/src/stlink-lib/chipid.c#L295-L305

I don’t really think the address is wrong, technically we are in the option bytes memory area; but the bytes we are interested in (RDP, USER, SPRMOD, nWRP) are obviously not included in these first four bytes. The easiest solution seems to set option_size to 16.

[Nightwalker-87]

Yes, and even more favourable could be to include an address mask for the valid option byte addresses to every chip-id config file. This may help to find the correct fields with valid data for each individual MCU type.

[Nightwalker-87]

@gszy Please also take note of the latest discussion in #1180 in this context.

[Nightwalker-87]

This currently waits for #1181 to merge.

[Nightwalker-87]

We may continue with this now.

[Nightwalker-87]

@gszy BTW: Where did the .flash_size_reg parameter go? Can we move the comments into the chip-id files as well? (I think so.)

[gszy]

Where did the .flash_size_reg parameter go?

I didn’t copy the flash_size_reg parameter, as I didn’t see it in the existing chip files (before my changes). Here is the relevant part of my script:

fprintf(f, "# Chip-ID file for %s\n#\n", device->description);
fprintf(f, "chip_id 0x%x\n", device->chip_id);
fprintf(f, "description %s\n", device->description);
fprintf(f, "flash_type %d\n", device->flash_type);
fprintf(f, "flash_pagesize 0x%x\n", device->flash_pagesize);
fprintf(f, "sram_size 0x%x\n", device->sram_size);
fprintf(f, "bootrom_base 0x%x\n", device->bootrom_base);
fprintf(f, "bootrom_size 0x%x\n", device->bootrom_size);
fprintf(f, "option_base 0x%x\n", device->option_base);
fprintf(f, "option_size 0x%x\n", device->option_size);

if (device->flags & CHIP_F_HAS_DUAL_BANK)
	strcpy(flags, "dualbank");
if (device->flags & CHIP_F_HAS_SWO_TRACING) {
	if (flags[0] == '\0')
		strcpy(flags, "swo");
	else
		strcat(flags, " swo");
}
if (flags[0] == '\0')
	strcpy(flags, "none");
fprintf(f, "flags %s\n\n", flags);

(/me wonders why GitHub highlights the second strcpy and the strcat in a different colour…)

Can we move the comments into the chip-id files as well? (I think so.)

We can move the comments there, though I’m afraid doing so manually would take less time than writing a script to do so (using some C parser or playing with grep and sed).

[Nightwalker-87]

Yes, of course - that's out of question. I've already started to go through the comments chip by chip. One finds that they are incomplete with missing references to the respective RMs by ST Microelectronics. We would want to have that complete I believe, but this is quite a bit of work to do. It would be great to have some help with this.

We should also check the flash_size_reg again and how it is used.

I'll start a new working branch for this. Hopefully the original issue of this ticket can be addressed there as well with several people working on this together.

[Nightwalker-87]

flash_size_reg is only used twice in the codebase and this is in common.c (Lines 915 & 1617):

       } else if (strcmp (word, "flash_size_reg") == 0) {
            if (sscanf(value, "%i", &ts->flash_size_reg) < 1) {
                fprintf(stderr, "Failed to parse flash size reg\n");
            }

  if (params->flash_size_reg & 2) {
    flash_size = flash_size >> 16;
  }

[Ant-ON]

@Nightwalker-87 The main use of flash_size_reg is https://github.com/stlink-org/stlink/blob/ac633683f16409883df740c44592bfeac6b8124f/src/common.c#L1615

@ApiumJacob described the bug. It is necessary to rewrite this part: https://github.com/stlink-org/stlink/blob/db8f789400d0ac19536228b0bd774ac8f1d60ed6/src/st-flash/flash.c#L203-L217 to something like

            uint32_t option_byte = 0;
            err = stlink_read_option_bytes32(sl, &option_byte);
            if (err == -1) {
                printf("could not read option bytes (%d)\n", err);
                goto on_error;
            } else if (NULL != o.filename) {
                int fd = open(o.filename, O_RDWR | O_TRUNC | O_CREAT | O_BINARY, 00700);
                if (fd == -1) {
                    fprintf(stderr, "open(%s) == -1\n", o.filename);
                    goto on_error;
                }
                write(fd, option_byte, 4);
                close(fd);
            } else {
                printf("%08x\n", option_byte);
            }

This fix does not support writing to ihex. But ihex is more complicated. Bytes can be read from one place (copies in RAM), and must be written to another address (in flash).

[Nightwalker-87]

@Ant-ON @gszy I am looking forward to point out the current state of option byte read/write support for the different MCus supported by this toolset. In this context I've pushed a nightly branch to the repo where the source file option_bytes.c has been revised and restructured to group functions that belong together to improve maintainability.

Can you have a look at it and reconsider contributing to this issue based on your previous thoughts? I'd be pleased if we could use this opportunity to resolve several remaining option byte related issues.

[Nightwalker-87]

@gszy I've set option_size to 16 (0x10) and will push this to a testing branch after including the remarks from @Ant-ON. However somebody would need to address the ihex-case to which I can't contribute.

[Nightwalker-87]

@Nightwalker-87 The main use of flash_size_reg is

https://github.com/stlink-org/stlink/blob/ac633683f16409883df740c44592bfeac6b8124f/src/common.c#L1615

@ApiumJacob described the bug. It is necessary to rewrite this part:

https://github.com/stlink-org/stlink/blob/db8f789400d0ac19536228b0bd774ac8f1d60ed6/src/st-flash/flash.c#L203-L217

to something like

            uint32_t option_byte = 0;
            err = stlink_read_option_bytes32(sl, &option_byte);
            if (err == -1) {
                printf("could not read option bytes (%d)\n", err);
                goto on_error;
            } else if (NULL != o.filename) {
                int fd = open(o.filename, O_RDWR | O_TRUNC | O_CREAT | O_BINARY, 00700);
                if (fd == -1) {
                    fprintf(stderr, "open(%s) == -1\n", o.filename);
                    goto on_error;
                }
                write(fd, option_byte, 4);
                close(fd);
            } else {
                printf("%08x\n", option_byte);
            }

This fix does not support writing to ihex. But ihex is more complicated. Bytes can be read from one place (copies in RAM), and must be written to another address (in flash).

@Ant-ON I tried this out, but it seems incomplete and leads to errors during compilation as well. Can you provide a complete solution based on your original idea?