raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.26k stars 838 forks source link

IS25LP***D flash uID size is wrong in SDK and produce duplicated flash uID #1641

Open eNovware opened 4 months ago

eNovware commented 4 months ago

Hi world,

I'm working with custom RP2040 boards with IS25LP***D flash chip (usually IS25LP016D)

I've made custom board file and all works fine.

The trouble is with the uID returned using _pico_get_unique_boardid( ) and _pico_get_unique_board_idstring( ) functions.

_pico_unique_board_idt is 8 bytes long but the uID from IS25LP***D is 16 bytes long, so we have a lot of same uID.

Maybe could I override PICO_UNIQUE_BOARD_ID_SIZE_BYTES in my board file ? ... any idea to solve that ?

Thansk for reading. Sebastien.

peterharperuk commented 4 months ago

@kilograham Is there a reason why PICO_UNIQUE_BOARD_ID_SIZE_BYTES is not overridable or was it an oversight?

eNovware commented 4 months ago

Looks like this ID size is near hard-coded in _src/rp2_common/hardwareflash/flash.c

This should be linked to flash selected instead.

Maybe it's easier in my case to try to read ID with custom function but since flash store my code ... probably not as simple as to write it ...

eNovware commented 4 months ago

What about something like this

void flash_get_unique_id_IS25LP(uint8_t *id_out)
{
  const int IS25LP_FLASH_RUID_CMD 0x4b
  const int IS25LP_FLASH_RUID_DUMMY_BYTES 4
  const int IS25LP_FLASH_RUID_DATA_BYTES 16
  const int IS25LP_FLASH_RUID_TOTAL_BYTES (1 + IS25LP_FLASH_RUID_DUMMY_BYTES + IS25LP_FLASH_RUID_DATA_BYTES)
  //
  uint8_t txbuf[IS25LP_FLASH_RUID_TOTAL_BYTES] = {0};
  uint8_t rxbuf[IS25LP_FLASH_RUID_TOTAL_BYTES] = {0};
  //
  txbuf[0] = IS25LP_FLASH_RUID_CMD;
  flash_do_cmd(txbuf, rxbuf, IS25LP_FLASH_RUID_TOTAL_BYTES);
  //
  for (int i = 0; i < IS25LP_FLASH_RUID_DATA_BYTES; i++)
    id_out[i] = rxbuf[i + 1 + IS25LP_FLASH_RUID_DUMMY_BYTES];
}

But looks like calls to that kind of low-level flash RW must to be done as soon à possible.

The fisrt line in my main() is stdio_init_all(), should I call flash_get_unique_id_IS25LP() before or just after it ? ( or does stdio_init_all() in main() is already too late in CPU initialization ?!! )

Thanks.

lurch commented 4 months ago

Looks like this code was initially added in https://github.com/raspberrypi/pico-sdk/commit/c1196e9af61edd374db42412a8b720d15afc152f which says "serial NOR flash devices which have a 64-bit unique ID as a standard feature" ?

EDIT: Ahh, but https://www.issi.com/WW/pdf/25LP-WP016D.pdf indeed says "128 bit Unique ID for Each Device".

eNovware commented 4 months ago

That's the point : some flash have 128b uID and other 64b, but SDK actually only read 64b uID, so all (a lot of !) 128b flashs chip have same uID.

lurch commented 4 months ago

...which I assume means that when the code was first written, we were only testing with flash chips with a 64b uID :wink:

eNovware commented 4 months ago

I fully understand why it's done as it's done, but how could I get my full uID ?

Does the function in upper post have any chance to work or do I have to wait for SDK to fix this ?

The SDK doc say : << [...] _executing flash code on the second core concurrently will be fatal. To avoid these pitfalls it is recommended that this function only be used to extract flash metadata during startup, before the main application begins to run: see the implementation of pico_get_uniqueid() for an example of this. >>

Pretty terrifying !!!

I can run this once in my main() before or after the _stdio_initall(), but is it safe ? ( really safe I mean, not just "it will works most of the time" :) )

kilograham commented 4 months ago

you can call it (without additional protection) if you aren't running anything on core 1 (which you won't be unless/until you started something yourself)

peterharperuk commented 4 months ago

A quick look at the datasheet suggests that might work. You should be ok calling it in main - nothing runs on core1 by default. Yes, you'd need a change to the sdk for a proper fix.