Add PSRAM support #2919

Merged
will-v-pi merged 41 commits from pico-psram into develop 2026-05-21 06:39:00 -05:00
will-v-pi commented 2026-05-01 12:33:03 -05:00 (Migrated from github.com)

This adds a hardware_psram library to the SDK to add PSRAM support, as requested in https://github.com/raspberrypi/pico-sdk/issues/2372

Adds PSRAM defines to board headers for boards that have PSRAM - I may not have captured them all

Has three ways it will detect PSRAM when you link hardware_psram, in order of priority:

  1. If flash_devinfo is setup (eg configured in OTP), it will initialise PSRAM with the flash_devinfo CS1 size and GPIO
  2. If PICO_AUTO_DETECT_PSRAM is set it will attempt to detect PSRAM size and GPIO on CS1
  3. If PICO_PSRAM_SIZE_BYTES is set (eg configured in board header, or with pico_override_psram_size) it will initialise PSRAM with that size and GPIO

Variables can be placed in PSRAM using __psram or __psram_uninitialised macros, and you can also write directly to the memory addresses.

If there are variables placed in PSRAM, it will setup ATRANS to prevent any access to PSRAM addresses greater than the size available (from flash_devinfo, auto-detection, or PICO_PSRAM_SIZE_BYTES), which will cause bus faults. The psram_is_available function should be used before accessing variables in PSRAM when auto-detection is on, to prevent these bus faults.

This support uses flash_devinfo to enable the bootrom support for PSRAM, then adds a callback with flash_set_qmi_cs1_setup_function which runs as part of flash_rp2350_restore_qmi_cs1 to enable quad mode and setup faster timings. The RP2350-E14 workaround is included.

The CS pin auto-detection iterates through all possible CS pins, skipping any that are defined as defaults in board header files with remove_defaults_from_cs_gpios - this is an ugly function, but without it you get some garbled characters on the UART as pin 0 is a possible CS pin. Maybe some of the defaults could be removed from this, if they are unlikely to be affected by this auto-detection?

This has psram_or_malloc and psram_or_free macros, which statically allocate in PSRAM and use that if available, but use malloc if not available - not sure how useful these are, could be removed if we don't want them. They were to demostrate psram_check_address which checks if an address is in available PSRAM, so you can avoid busfaults when using variables in PSRAM.

In passing this also:

  • Removes some Bazel TODOs - CMake gates RP2350 only stuff in src/cmake/rp2_common.cmake
  • Expands RP2350 boot2 linker script by 4 bytes, as there is no checksum
This adds a `hardware_psram` library to the SDK to add PSRAM support, as requested in https://github.com/raspberrypi/pico-sdk/issues/2372 Adds PSRAM defines to board headers for boards that have PSRAM - I may not have captured them all Has three ways it will detect PSRAM when you link `hardware_psram`, in order of priority: 1. If flash_devinfo is setup (eg configured in OTP), it will initialise PSRAM with the flash_devinfo CS1 size and GPIO 2. If `PICO_AUTO_DETECT_PSRAM` is set it will attempt to detect PSRAM size and GPIO on CS1 3. If `PICO_PSRAM_SIZE_BYTES` is set (eg configured in board header, or with `pico_override_psram_size`) it will initialise PSRAM with that size and GPIO Variables can be placed in PSRAM using `__psram` or `__psram_uninitialised` macros, and you can also write directly to the memory addresses. If there are variables placed in PSRAM, it will setup ATRANS to prevent any access to PSRAM addresses greater than the size available (from flash_devinfo, auto-detection, or `PICO_PSRAM_SIZE_BYTES`), which will cause bus faults. The `psram_is_available` function should be used before accessing variables in PSRAM when auto-detection is on, to prevent these bus faults. This support uses flash_devinfo to enable the bootrom support for PSRAM, then adds a callback with `flash_set_qmi_cs1_setup_function` which runs as part of `flash_rp2350_restore_qmi_cs1` to enable quad mode and setup faster timings. The RP2350-E14 workaround is included. The CS pin auto-detection iterates through all possible CS pins, skipping any that are defined as defaults in board header files with `remove_defaults_from_cs_gpios` - this is an ugly function, but without it you get some garbled characters on the UART as pin 0 is a possible CS pin. Maybe some of the defaults could be removed from this, if they are unlikely to be affected by this auto-detection? This has `psram_or_malloc` and `psram_or_free` macros, which statically allocate in PSRAM and use that if available, but use `malloc` if not available - not sure how useful these are, could be removed if we don't want them. They were to demostrate `psram_check_address` which checks if an address is in available PSRAM, so you can avoid busfaults when using variables in PSRAM. In passing this also: - Removes some Bazel TODOs - CMake gates RP2350 only stuff in src/cmake/rp2_common.cmake - Expands RP2350 boot2 linker script by 4 bytes, as there is no checksum
lurch (Migrated from github.com) reviewed 2026-05-05 11:38:50 -05:00
lurch (Migrated from github.com) commented 2026-05-05 11:38:49 -05:00

Does it make sense to include this into an RP2040-specific file?

Does it make sense to include this into an RP2040-specific file?
lurch (Migrated from github.com) reviewed 2026-05-05 11:44:17 -05:00
@ -140,1 +139,4 @@
* implementation of pico_get_unique_id() for an example of this.
*
* \if rp2040_specific
* On RP2040 the chip select index is ignored, as there is only one chip select.
lurch (Migrated from github.com) commented 2026-05-05 11:44:17 -05:00

Should we document that the cs parameter is ignored on RP2040?

Should we document that the `cs` parameter is ignored on RP2040?
lurch (Migrated from github.com) reviewed 2026-05-05 11:48:03 -05:00
@ -140,1 +139,4 @@
* implementation of pico_get_unique_id() for an example of this.
*
* \if rp2040_specific
* On RP2040 the chip select index is ignored, as there is only one chip select.
lurch (Migrated from github.com) commented 2026-05-05 11:48:02 -05:00

Is it worth adding one of the invalid_params_if checks on RP2350 to check that cs < 2 ?

Is it worth adding one of the `invalid_params_if` checks on RP2350 to check that `cs < 2` ?
lurch (Migrated from github.com) reviewed 2026-05-05 11:51:58 -05:00
@ -140,1 +139,4 @@
* implementation of pico_get_unique_id() for an example of this.
*
* \if rp2040_specific
* On RP2040 the chip select index is ignored, as there is only one chip select.
lurch (Migrated from github.com) commented 2026-05-05 11:51:58 -05:00

Perhaps the earlier "Low-level function to execute a serial command on a flash device attached to the QSPI interface." wording needs to be updated to say something like "flash or PSRAM device" ? 🤷

Perhaps the earlier "Low-level function to execute a serial command on a flash device attached to the QSPI interface." wording needs to be updated to say something like "flash or PSRAM device" ? :shrug:
lurch (Migrated from github.com) reviewed 2026-05-05 11:54:00 -05:00
@ -140,1 +139,4 @@
* implementation of pico_get_unique_id() for an example of this.
*
* \if rp2040_specific
* On RP2040 the chip select index is ignored, as there is only one chip select.
lurch (Migrated from github.com) commented 2026-05-05 11:54:00 -05:00
 * \return true if the function was set, false if not (e.g. tried to set a function in flash)
```suggestion * \return true if the function was set, false if not (e.g. tried to set a function in flash) ```
lurch (Migrated from github.com) reviewed 2026-05-05 12:02:29 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:02:29 -05:00

type=bool ?

`type=bool` ?
lurch (Migrated from github.com) reviewed 2026-05-05 12:05:45 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:05:45 -05:00

Isn't the entirety of hardware_psram unusable if we're not running on RP2350?

Isn't the entirety of `hardware_psram` unusable if we're not running on RP2350?
lurch (Migrated from github.com) reviewed 2026-05-05 12:08:54 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:08:54 -05:00

I guess this might be more readable as 133 * MHZ? 😉 Or would that introduce an unwanted header dependency?

I guess this might be more readable as `133 * MHZ`? :wink: Or would that introduce an unwanted header dependency?
lurch (Migrated from github.com) reviewed 2026-05-05 12:10:50 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:10:50 -05:00

IMHO "time in ns of psram" here should be "time of psram in ns" ?

IMHO "time in ns of psram" here should be "time of psram in ns" ?
lurch (Migrated from github.com) reviewed 2026-05-05 12:13:29 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:13:29 -05:00

The defaults described here don't seem to match the #defines below?

The defaults described here don't seem to match the `#define`s below?
lurch (Migrated from github.com) reviewed 2026-05-05 12:15:56 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:15:55 -05:00

Should the description and the function both use "initialised" or "available", rather than mixing the two terms?

Should the description and the function both use "initialised" or "available", rather than mixing the two terms?
lurch (Migrated from github.com) reviewed 2026-05-05 12:19:06 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:19:06 -05:00
 * and should also configure the CS GPIO in flash_devinfo to prevent toggling of the

?

```suggestion * and should also configure the CS GPIO in flash_devinfo to prevent toggling of the ``` ?
lurch (Migrated from github.com) reviewed 2026-05-05 12:20:18 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:20:18 -05:00
 * previously configured GPIO (usually 0, so prints invalid characters to default UART).

?

```suggestion * previously configured GPIO (usually 0, so prints invalid characters to default UART). ``` ?
will-v-pi (Migrated from github.com) reviewed 2026-05-05 12:21:22 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-05 12:21:22 -05:00

Yes, but the default CS pins need to be defined for each platform

Yes, but the default CS pins need to be defined for each platform
lurch (Migrated from github.com) reviewed 2026-05-05 12:30:38 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:30:38 -05:00
 * flash_devinfo or the PSRAM parameters are not set by \ref psram_configure_params or \ref psram_set_params
```suggestion * flash_devinfo or the PSRAM parameters are not set by \ref psram_configure_params or \ref psram_set_params ```
lurch (Migrated from github.com) reviewed 2026-05-05 12:32:53 -05:00
@ -0,0 +241,4 @@
/*! \brief Provide a static PSRAM allocation, or malloc if PSRAM is not available
* \ingroup hardware_psram
*
* This will allocate a static buffer in PSRAM and if available use that, otherwise it will use the heap.
lurch (Migrated from github.com) commented 2026-05-05 12:32:53 -05:00
 * This will allocate a static buffer in PSRAM if available and use that, otherwise it will use the heap.
```suggestion * This will allocate a static buffer in PSRAM if available and use that, otherwise it will use the heap. ```
lurch (Migrated from github.com) reviewed 2026-05-05 12:34:05 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-05 12:34:05 -05:00

Should there be a bunch of \param comments here?

Should there be a bunch of `\param` comments here?
will-v-pi (Migrated from github.com) reviewed 2026-05-05 12:36:16 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-05 12:36:16 -05:00

Yes, but I'm leaving fixing up this API until after it's decided if it's wanted, so it's easy to back out the one commit - I'm not sure how much utility it has

Yes, but I'm leaving fixing up this API until after it's decided if it's wanted, so it's easy to back out the one commit - I'm not sure how much utility it has
will-v-pi (Migrated from github.com) reviewed 2026-05-06 04:02:54 -05:00
will-v-pi (Migrated from github.com) commented 2026-05-06 04:02:54 -05:00

I've added some more RP2040 incls, so this has been removed

I've added some more RP2040 incls, so this has been removed
will-v-pi (Migrated from github.com) reviewed 2026-05-06 04:29:05 -05:00
@ -140,1 +139,4 @@
* implementation of pico_get_unique_id() for an example of this.
*
* \if rp2040_specific
* On RP2040 the chip select index is ignored, as there is only one chip select.
will-v-pi (Migrated from github.com) commented 2026-05-06 04:29:05 -05:00

Perhaps the earlier "Low-level function to execute a serial command on a flash device attached to the QSPI interface." wording needs to be updated to say something like "flash or PSRAM device" ? 🤷

I'll removel the reference to PSRAM, and remove the word "flash" from the top paragraph so it's just "device" and "chip"

> Perhaps the earlier "Low-level function to execute a serial command on a flash device attached to the QSPI interface." wording needs to be updated to say something like "flash or PSRAM device" ? 🤷 I'll removel the reference to PSRAM, and remove the word "flash" from the top paragraph so it's just "device" and "chip"
will-v-pi (Migrated from github.com) reviewed 2026-05-06 04:38:02 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-06 04:38:01 -05:00

Have gone for "available and initialised" in the docs, and left the function name the same

Have gone for "available and initialised" in the docs, and left the function name the same
lurch (Migrated from github.com) reviewed 2026-05-06 07:15:36 -05:00
@ -261,15 +274,16 @@ static void __no_inline_not_in_flash_func(flash_cs_force)(bool high) {
IO_QSPI_GPIO_QSPI_SS_CTRL_OUTOVER_BITS
);
lurch (Migrated from github.com) commented 2026-05-06 07:15:36 -05:00

If the doxygen for flash_do_cmd_cs is explicitly saying "On RP2040 the chip select index is ignored, as there is only one chip select." then should this invalid_params_if be wrapped in a #if !PICO_RP2040 ?
Also, should the same invalid_params_if be added to flash_cs_force ?

If the doxygen for `flash_do_cmd_cs` is explicitly saying "On RP2040 the chip select index is ignored, as there is only one chip select." then should this `invalid_params_if` be wrapped in a `#if !PICO_RP2040` ? Also, should the same `invalid_params_if` be added to `flash_cs_force` ?
lurch (Migrated from github.com) reviewed 2026-05-06 07:19:54 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-06 07:19:53 -05:00

Is what is written here about "during flash programming" also true for "during psram writing"? Or are Flash and PSRAM sufficiently different that less precautions are needed with psram?

Is what is written here about "during flash programming" also true for "during psram writing"? Or are Flash and PSRAM sufficiently different that less precautions are needed with psram?
lurch (Migrated from github.com) reviewed 2026-05-06 07:45:32 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-06 07:45:32 -05:00

Should this be made conditional on RP2350A versus RP2350B? (as GPIO 47 obviously only exists on RP2350B)

Should this be made conditional on RP2350A versus RP2350B? (as GPIO 47 obviously only exists on RP2350B)
lurch (Migrated from github.com) reviewed 2026-05-06 07:49:51 -05:00
@ -0,0 +157,4 @@
/*! \brief Detect PSRAM size
* \ingroup hardware_psram
*
* This will read the ID of the PSRAM chip and return the size based on the ID.
lurch (Migrated from github.com) commented 2026-05-06 07:49:50 -05:00
 * This will read the EID of the PSRAM chip and return the size based on the EID.

(this is just a hunch based on the doxygen for psram_eid_to_size 😉 )

```suggestion * This will read the EID of the PSRAM chip and return the size based on the EID. ``` (this is just a hunch based on the doxygen for `psram_eid_to_size` :wink: )
lurch (Migrated from github.com) reviewed 2026-05-06 07:56:20 -05:00
@ -104,0 +106,4 @@
#
# \param\ TARGET The target to override the FLASH size for
# \param\ FLASH_SIZE The FLASH size to set for the target
function(pico_override_flash_size TARGET FLASH_SIZE)
lurch (Migrated from github.com) commented 2026-05-06 07:56:20 -05:00

It looks like this is only used/useful for testing, so perhaps it makes sense to just add it to kitchen_sink rather than pico_standard_link ? 🤷
(unintentional rhyme 😂 )

It looks like this is only used/useful for testing, so perhaps it makes sense to just add it to `kitchen_sink` rather than `pico_standard_link` ? :shrug: (unintentional rhyme :joy: )
will-v-pi (Migrated from github.com) reviewed 2026-05-06 08:00:30 -05:00
@ -104,0 +106,4 @@
#
# \param\ TARGET The target to override the FLASH size for
# \param\ FLASH_SIZE The FLASH size to set for the target
function(pico_override_flash_size TARGET FLASH_SIZE)
will-v-pi (Migrated from github.com) commented 2026-05-06 08:00:30 -05:00

It could be useful for creating universal binaries, if you wanted to force a smaller flash size to ensure your code always fits in that? Or if you wanted to build for multiple flash sizes in a single build directory?

I mostly added it because I was adding the PSRAM one and it was simple to add flash too.

It could be useful for creating universal binaries, if you wanted to force a smaller flash size to ensure your code always fits in that? Or if you wanted to build for multiple flash sizes in a single build directory? I mostly added it because I was adding the PSRAM one and it was simple to add flash too.
lurch (Migrated from github.com) reviewed 2026-05-06 09:34:15 -05:00
@ -104,0 +106,4 @@
#
# \param\ TARGET The target to override the FLASH size for
# \param\ FLASH_SIZE The FLASH size to set for the target
function(pico_override_flash_size TARGET FLASH_SIZE)
lurch (Migrated from github.com) commented 2026-05-06 09:34:15 -05:00

I'm showing my lack of CMake knowledge here, but do we need a specific override function for this, given that the board-header files all seem to follow the

#ifndef PICO_FLASH_SIZE_BYTES
#define PICO_FLASH_SIZE_BYTES (16 * 1024 * 1024)
#endif

pattern? Or are the override functions only needed because of the way that these values interact with the linker-scripts?


Hmmm, although that also raises an interesting side-question... are there some #defines that check_board_header.py ought to error about if they aren't guarded by an #ifndef clause? Because whilst it checks that the #ifndef matches the enclosed #define, so that this would error:

#ifndef PICO_FLASH_SIZE_BYTE
#define PICO_FLASH_SIZE_BYTES (16 * 1024 * 1024)
#endif

currently both

pico_board_cmake_set_default(PICO_FLASH_SIZE_BYTES, (16 * 1024 * 1024))
#ifndef PICO_FLASH_SIZE_BYTES
#define PICO_FLASH_SIZE_BYTES (16 * 1024 * 1024)
#endif

and

pico_board_cmake_set_default(PICO_FLASH_SIZE_BYTES, (16 * 1024 * 1024))
#define PICO_FLASH_SIZE_BYTES (16 * 1024 * 1024)

are considered equally valid. (And presumably the latter version wouldn't work properly with your pico_override_flash_size function? 🤔 )

I'm showing my lack of CMake knowledge here, but do we _need_ a specific override function for this, given that the board-header files all seem to follow the ```c #ifndef PICO_FLASH_SIZE_BYTES #define PICO_FLASH_SIZE_BYTES (16 * 1024 * 1024) #endif ``` pattern? Or are the override functions only needed because of the way that these values interact with the linker-scripts? ---- Hmmm, although that also raises an interesting side-question... are there some `#define`s that `check_board_header.py` ought to error about if they _aren't_ guarded by an `#ifndef` clause? Because whilst it checks that the `#ifndef` matches the enclosed `#define`, so that this would error: ```c #ifndef PICO_FLASH_SIZE_BYTE #define PICO_FLASH_SIZE_BYTES (16 * 1024 * 1024) #endif ``` currently both ```c pico_board_cmake_set_default(PICO_FLASH_SIZE_BYTES, (16 * 1024 * 1024)) #ifndef PICO_FLASH_SIZE_BYTES #define PICO_FLASH_SIZE_BYTES (16 * 1024 * 1024) #endif ``` and ```c pico_board_cmake_set_default(PICO_FLASH_SIZE_BYTES, (16 * 1024 * 1024)) #define PICO_FLASH_SIZE_BYTES (16 * 1024 * 1024) ``` are considered equally valid. (And presumably the latter version wouldn't work properly with your `pico_override_flash_size` function? :thinking: )
will-v-pi (Migrated from github.com) reviewed 2026-05-06 09:58:46 -05:00
@ -104,0 +106,4 @@
#
# \param\ TARGET The target to override the FLASH size for
# \param\ FLASH_SIZE The FLASH size to set for the target
function(pico_override_flash_size TARGET FLASH_SIZE)
will-v-pi (Migrated from github.com) commented 2026-05-06 09:58:46 -05:00

It's needed to override the pico_flash_region.ld file, which sets the flash size in the linker script - the linker doesn't use PICO_FLASH_SIZE_BYTES.

I think that second version without #ifndef should be marked as invalid - definitely for any define with a pico_board_cmake_set_default, but more generally anything that wants to be overridable. Anything that is definitely fixed can just be a #define (eg the boot2 choice, rp2350 variant), but everything else should be overridable (eg I want to use an external LED because my on board one is broken). But that can probably come in a separate PR to develop, with specific exclusions for the defines which don't need to be #ifndefed.

It's needed to override the `pico_flash_region.ld` file, which sets the flash size in the linker script - the linker doesn't use `PICO_FLASH_SIZE_BYTES`. I think that second version without `#ifndef` should be marked as invalid - definitely for any define with a `pico_board_cmake_set_default`, but more generally anything that wants to be overridable. Anything that is definitely fixed can just be a `#define` (eg the boot2 choice, rp2350 variant), but everything else should be overridable (eg I want to use an external LED because my on board one is broken). But that can probably come in a separate PR to develop, with specific exclusions for the defines which don't need to be `#ifndef`ed.
lurch (Migrated from github.com) reviewed 2026-05-06 10:14:41 -05:00
@ -104,0 +106,4 @@
#
# \param\ TARGET The target to override the FLASH size for
# \param\ FLASH_SIZE The FLASH size to set for the target
function(pico_override_flash_size TARGET FLASH_SIZE)
lurch (Migrated from github.com) commented 2026-05-06 10:14:41 -05:00

But that can probably come in a separate PR to develop, with specific exclusions for the defines which don't need to be #ifndefed.

Yup, that's what I was planning to do, rather than polluting #2924 (and apologies for the off-topic questions in this PR).

EDIT: This #ifndef checking has been done in #2927

> But that can probably come in a separate PR to develop, with specific exclusions for the defines which don't need to be #ifndefed. Yup, that's what I was planning to do, rather than polluting #2924 (and apologies for the off-topic questions in this PR). EDIT: This `#ifndef` checking has been done in #2927
will-v-pi (Migrated from github.com) reviewed 2026-05-06 10:24:11 -05:00
@ -261,15 +274,16 @@ static void __no_inline_not_in_flash_func(flash_cs_force)(bool high) {
IO_QSPI_GPIO_QSPI_SS_CTRL_OUTOVER_BITS
);
will-v-pi (Migrated from github.com) commented 2026-05-06 10:24:11 -05:00

I'll move it into flash_cs_force as that handles both of these issues

I'll move it into `flash_cs_force` as that handles both of these issues
will-v-pi (Migrated from github.com) reviewed 2026-05-06 10:28:26 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-06 10:28:26 -05:00

It's a typo from copying - will use "while running these functions" instead

It's a typo from copying - will use "while running these functions" instead
will-v-pi (Migrated from github.com) reviewed 2026-05-06 10:41:18 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-06 10:41:18 -05:00

Annoyingly that ruins the utility of this for me, as I'm compiling for RP2350A but running on an RP2350B board with PSRAM on CS 47 - but looking at the code, using GPIO 47 on RP2350A will trigger an assertion when PARAM_ASSERTIONS_ENABLED_HARDWARE_GPIO is enabled, so you are correct that this needs to be conditional.

Annoyingly that ruins the utility of this for me, as I'm compiling for RP2350A but running on an RP2350B board with PSRAM on CS 47 - but looking at the code, using GPIO 47 on RP2350A will trigger an assertion when `PARAM_ASSERTIONS_ENABLED_HARDWARE_GPIO` is enabled, so you are correct that this needs to be conditional.
will-v-pi (Migrated from github.com) reviewed 2026-05-06 11:15:48 -05:00
@ -0,0 +157,4 @@
/*! \brief Detect PSRAM size
* \ingroup hardware_psram
*
* This will read the ID of the PSRAM chip and return the size based on the ID.
will-v-pi (Migrated from github.com) commented 2026-05-06 11:15:48 -05:00

EID is part of the ID - it also uses the KGD part, and returns 0 if that doesn't match what is expected, so I will leave this description as-is

EID is part of the ID - it also uses the KGD part, and returns 0 if that doesn't match what is expected, so I will leave this description as-is
lurch (Migrated from github.com) reviewed 2026-05-07 07:03:22 -05:00
@ -138,16 +137,44 @@ void flash_get_unique_id(uint8_t *id_out);
* it is recommended that this function only be used to extract flash metadata
lurch (Migrated from github.com) commented 2026-05-07 07:03:22 -05:00

Should these also be updated to say "device" rather than "flash"?

Should these also be updated to say "device" rather than "flash"?
lurch (Migrated from github.com) reviewed 2026-05-07 07:06:26 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 07:06:26 -05:00

Presumably there'll be a separate PR adding this program to pico-examples ?

Presumably there'll be a separate PR adding this program to `pico-examples` ?
lurch (Migrated from github.com) reviewed 2026-05-07 07:16:05 -05:00
@ -0,0 +171,4 @@
* \ingroup hardware_psram
*
* This runs \ref psram_detect_size() for each CS GPIO in the array in turn,
* and returns the size as soon as a PSRAM chip is detected.
lurch (Migrated from github.com) commented 2026-05-07 07:16:04 -05:00

This may be a stupid question, but would it be useful for the code calling this function to be able to determine which of the CS GPIOs got selected?

This may be a stupid question, but would it be useful for the code calling this function to be able to determine _which_ of the CS GPIOs got selected?
lurch (Migrated from github.com) reviewed 2026-05-07 07:44:37 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 07:44:37 -05:00

Looks like it might make sense to swap these?

        } else if (size_id == 1) {
            psram_size *= 4; // 4 MiB
        } else if (size_id == 0) {
            psram_size *= 2; // 2 MiB

That way the sizes in the comments would be in the order: 1 MiB, 16 MiB, 8 MiB, 4 MiB, 2 MiB

Looks like it might make sense to swap these? ```suggestion } else if (size_id == 1) { psram_size *= 4; // 4 MiB } else if (size_id == 0) { psram_size *= 2; // 2 MiB ``` That way the sizes in the comments would be in the order: 1 MiB, 16 MiB, 8 MiB, 4 MiB, 2 MiB
will-v-pi (Migrated from github.com) reviewed 2026-05-07 07:47:02 -05:00
@ -0,0 +171,4 @@
* \ingroup hardware_psram
*
* This runs \ref psram_detect_size() for each CS GPIO in the array in turn,
* and returns the size as soon as a PSRAM chip is detected.
will-v-pi (Migrated from github.com) commented 2026-05-07 07:47:02 -05:00

It can read the value from flash_devinfo with flash_devinfo_get_cs_gpio(1)

It can read the value from flash_devinfo with `flash_devinfo_get_cs_gpio(1)`
lurch (Migrated from github.com) reviewed 2026-05-07 07:47:58 -05:00
@ -0,0 +28,4 @@
// Currently supports APS6404 and ISSI PSRAM
size_t psram_size = 0;
if (kgd == PICO_DEFAULT_PSRAM_ID) { // Known Good Die - expects 0x5D
lurch (Migrated from github.com) commented 2026-05-07 07:47:57 -05:00

I know nothing about PSRAM, but how likely is it that the code inside this if block would still be correct if somebody changes PICO_DEFAULT_PSRAM_ID to a value other than 0x5D ?

I know nothing about PSRAM, but how likely is it that the code inside this `if` block would still be correct if somebody changes `PICO_DEFAULT_PSRAM_ID` to a value other than `0x5D` ?
will-v-pi (Migrated from github.com) reviewed 2026-05-07 07:51:24 -05:00
@ -0,0 +28,4 @@
// Currently supports APS6404 and ISSI PSRAM
size_t psram_size = 0;
if (kgd == PICO_DEFAULT_PSRAM_ID) { // Known Good Die - expects 0x5D
will-v-pi (Migrated from github.com) commented 2026-05-07 07:51:24 -05:00

I have no idea, as the only chips I've seen so far have had the same KGD value - if it doesn't work they can just write their own psram_eid_to_size as it is a weak function

I have no idea, as the only chips I've seen so far have had the same KGD value - if it doesn't work they can just write their own `psram_eid_to_size` as it is a weak function
lurch (Migrated from github.com) reviewed 2026-05-07 07:53:18 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 07:53:18 -05:00

I guess this ought to check that num <= 4 to prevent line 76 causing a buffer overflow?

I guess this ought to check that `num <= 4` to prevent line 76 causing a buffer overflow?
lurch (Migrated from github.com) reviewed 2026-05-07 07:58:53 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 07:58:53 -05:00

Hmm, I guess if you pass in 3 GPIO pins to this function, and the PSRAM CS is on the second GPIO pin, then the immediate return above means that the GPIO function would be restored on the first GPIO, but not restored on the third GPIO?

Hmm, I guess if you pass in 3 GPIO pins to this function, and the PSRAM CS is on the second GPIO pin, then the immediate `return` above means that the GPIO function would be restored on the first GPIO, but **not** restored on the third GPIO?
will-v-pi (Migrated from github.com) reviewed 2026-05-07 07:59:34 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 07:59:34 -05:00

I'll remove as this doesn't have examples yet - was copied from flash.h

I'll remove as this doesn't have examples yet - was copied from flash.h
lurch (Migrated from github.com) reviewed 2026-05-07 08:02:25 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 08:02:25 -05:00

Having read through the code I can now see what PICO_AUTO_DETECT_PSRAM_CS_SKIP_DEFAULTS is doing, but IMHO the "skip default GPIOs" description here is far too vague.

Having read through the code I can now see what `PICO_AUTO_DETECT_PSRAM_CS_SKIP_DEFAULTS` is doing, but IMHO the "skip default GPIOs" description here is _far_ too vague.
lurch (Migrated from github.com) reviewed 2026-05-07 08:08:43 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 08:08:42 -05:00

Is it possible that this reorders the entries in the (trimmed down) cs_gpios array? Does that matter?

Is it possible that this reorders the entries in the (trimmed down) `cs_gpios` array? Does that matter?
will-v-pi (Migrated from github.com) reviewed 2026-05-07 08:12:38 -05:00
@ -0,0 +1,449 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 08:12:38 -05:00

Yes it reorders them, but I don't think that matters

Yes it reorders them, but I don't think that matters
will-v-pi (Migrated from github.com) reviewed 2026-05-07 08:16:31 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 08:16:31 -05:00

Happy for a suggestion of better wording - I agree "skip default GPIOs" is too vague, it's skipping anything that is defined as a PICO_DEFAULT_XXX_PIN or a CYW43_DEFAULT_PIN_XXX, although that list can probably be trimmed down as I mentioned in the PR description

Happy for a suggestion of better wording - I agree "skip default GPIOs" is too vague, it's skipping anything that is defined as a `PICO_DEFAULT_XXX_PIN` or a `CYW43_DEFAULT_PIN_XXX`, although that list can probably be trimmed down as I mentioned in the PR description
will-v-pi (Migrated from github.com) reviewed 2026-05-07 08:17:12 -05:00
@ -0,0 +1,449 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 08:17:12 -05:00

Yes, I will fix that and restore all GPIOs (the bootrom will then put the chosen GPIO back to CS1 when configuring PSRAM)

Yes, I will fix that and restore all GPIOs (the bootrom will then put the chosen GPIO back to CS1 when configuring PSRAM)
will-v-pi (Migrated from github.com) reviewed 2026-05-07 08:40:09 -05:00
@ -0,0 +1,449 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 08:40:08 -05:00

Actually, I can just use gpio_function_t prev_funcs[num]; to avoid that issue

Actually, I can just use `gpio_function_t prev_funcs[num];` to avoid that issue
lurch (Migrated from github.com) reviewed 2026-05-07 09:26:53 -05:00
@ -0,0 +171,4 @@
* \ingroup hardware_psram
*
* This runs \ref psram_detect_size() for each CS GPIO in the array in turn,
* and returns the size as soon as a PSRAM chip is detected.
lurch (Migrated from github.com) commented 2026-05-07 09:26:52 -05:00

Perhaps this description needs updating now that the implementation has changed 😉

 * stopping as soon as a PSRAM chip is detected.

Also, maybe this function-description should warn that it will "wiggle" the CS GPIOs?

Perhaps this description needs updating now that the implementation has changed :wink: ```suggestion * stopping as soon as a PSRAM chip is detected. ``` Also, maybe this function-description should warn that it will "wiggle" the CS GPIOs?
lurch (Migrated from github.com) reviewed 2026-05-07 09:29:37 -05:00
@ -0,0 +173,4 @@
* This runs \ref psram_detect_size() for each CS GPIO in the array in turn,
* and returns the size as soon as a PSRAM chip is detected.
*
* This will setup the CS GPIO using flash_devinfo if PSRAM is found.
lurch (Migrated from github.com) commented 2026-05-07 09:29:37 -05:00

I think this might be clearer like:

 * If PSRAM is found, the CS GPIO will be configured in flash_devinfo.
I think this might be clearer like: ```suggestion * If PSRAM is found, the CS GPIO will be configured in flash_devinfo. ```
lurch (Migrated from github.com) reviewed 2026-05-07 09:32:20 -05:00
@ -0,0 +179,4 @@
* \param num Number of CS GPIOs in the array
* \return size of PSRAM, or 0 if none found
*/
size_t psram_detect_cs_and_size(uint8_t *cs_gpios, size_t num);
lurch (Migrated from github.com) commented 2026-05-07 09:32:20 -05:00

Maybe this function should assert if PICO_PSRAM_CS_PIN has been set? 🤔

Maybe this function should assert if `PICO_PSRAM_CS_PIN` has been set? :thinking:
lurch (Migrated from github.com) reviewed 2026-05-07 09:37:53 -05:00
@ -0,0 +82,4 @@
gpio_set_function(gpio, GPIO_FUNC_XIP_CS1);
psram_size = psram_detect_size();
if (psram_size > 0) {
// CS GPIO found, so will be left configured in flash_devinfo
lurch (Migrated from github.com) commented 2026-05-07 09:37:52 -05:00

If no PSRAM was found, should we also restore flash_devinfo to the original value?

If **no** PSRAM was found, should we also restore flash_devinfo to the original value?
lurch (Migrated from github.com) reviewed 2026-05-07 09:39:33 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 09:39:32 -05:00

Should this also be saved and then restored, in case external code is relying on the isolation behaviour?

Should this also be saved and then restored, in case external code is relying on the isolation behaviour?
lurch (Migrated from github.com) reviewed 2026-05-07 09:50:39 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 09:50:38 -05:00

Perhaps it makes sense to rename this to e.g. psram_initialized ?

Perhaps it makes sense to rename this to e.g. `psram_initialized` ?
will-v-pi (Migrated from github.com) reviewed 2026-05-07 09:59:02 -05:00
@ -0,0 +179,4 @@
* \param num Number of CS GPIOs in the array
* \return size of PSRAM, or 0 if none found
*/
size_t psram_detect_cs_and_size(uint8_t *cs_gpios, size_t num);
will-v-pi (Migrated from github.com) commented 2026-05-07 09:59:01 -05:00

If you enable auto-detection it ignores PICO_PSRAM_CS_PIN, so you can force auto-detection even if PICO_PSRAM_CS_PIN is defined - all of that is handled in runtime_init_setup_psram.

These individual functions should be callable regardless of the compile definitions, as you may want to run manual auto-detection, so they don't look at the PICO_PSRAM_XXX defines.

If you enable auto-detection it ignores `PICO_PSRAM_CS_PIN`, so you can force auto-detection even if `PICO_PSRAM_CS_PIN` is defined - all of that is handled in `runtime_init_setup_psram`. These individual functions should be callable regardless of the compile definitions, as you may want to run manual auto-detection, so they don't look at the `PICO_PSRAM_XXX` defines.
will-v-pi (Migrated from github.com) reviewed 2026-05-07 10:02:04 -05:00
@ -0,0 +82,4 @@
gpio_set_function(gpio, GPIO_FUNC_XIP_CS1);
psram_size = psram_detect_size();
if (psram_size > 0) {
// CS GPIO found, so will be left configured in flash_devinfo
will-v-pi (Migrated from github.com) commented 2026-05-07 10:02:04 -05:00

We haven't set the size, so the bootrom will be ignoring the flash_devinfo CS1 GPIO anyway, so I think it's fine

We haven't set the size, so the bootrom will be ignoring the flash_devinfo CS1 GPIO anyway, so I think it's fine
lurch (Migrated from github.com) reviewed 2026-05-07 10:03:03 -05:00
@ -0,0 +320,4 @@
}
bool psram_check_address(void* addr) {
uint32_t offset = (uint32_t)addr - (XIP_BASE + flash_devinfo_size_to_bytes(FLASH_DEVINFO_SIZE_MAX));
lurch (Migrated from github.com) commented 2026-05-07 10:03:03 -05:00

Could addr ever be less than (XIP_BASE + flash_devinfo_size_to_bytes(FLASH_DEVINFO_SIZE_MAX)) ?

Could `addr` ever be _less_ than `(XIP_BASE + flash_devinfo_size_to_bytes(FLASH_DEVINFO_SIZE_MAX))` ?
lurch (Migrated from github.com) reviewed 2026-05-07 10:06:55 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 10:06:54 -05:00

Why is setting the PICO_PSRAM_CS_PIN conditional on the value returned by flash_devinfo_get_cs_size ?

Why is setting the `PICO_PSRAM_CS_PIN` conditional on the value returned by `flash_devinfo_get_cs_size` ?
lurch (Migrated from github.com) reviewed 2026-05-07 10:11:35 -05:00
@ -0,0 +346,4 @@
bool __unused flash_devinfo_not_configured = flash_devinfo_get_cs_size(1) == FLASH_DEVINFO_SIZE_NONE;
// Setup flash_devinfo from compile definitions
#if defined(PICO_PSRAM_CS_PIN) && !PICO_AUTO_DETECT_PSRAM_CS
lurch (Migrated from github.com) commented 2026-05-07 10:11:35 -05:00

Maybe #if defined(PICO_PSRAM_CS_PIN) && PICO_AUTO_DETECT_PSRAM_CS ought to be a compile-time error? Or is there a reason why that configuration would be useful?

Same question also for #if defined(PICO_PSRAM_SIZE_BYTES) && PICO_AUTO_DETECT_PSRAM_SIZE

Maybe `#if defined(PICO_PSRAM_CS_PIN) && PICO_AUTO_DETECT_PSRAM_CS` ought to be a compile-time error? Or is there a reason why that configuration would be useful? Same question also for `#if defined(PICO_PSRAM_SIZE_BYTES) && PICO_AUTO_DETECT_PSRAM_SIZE`
will-v-pi (Migrated from github.com) reviewed 2026-05-07 10:16:21 -05:00
@ -0,0 +1,449 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 10:16:21 -05:00

Actually, I think I should use gpio_set_function(gpio, GPIO_FUNC_XIP_CS1); here instead, rather than manually changing the isolation

Actually, I think I should use `gpio_set_function(gpio, GPIO_FUNC_XIP_CS1);` here instead, rather than manually changing the isolation
lurch (Migrated from github.com) reviewed 2026-05-07 10:20:18 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 10:20:17 -05:00

flash_devinfo_get_cs_size(1) appears in this function multiple times - is it worth caching the value into a local variable?

`flash_devinfo_get_cs_size(1)` appears in this function _multiple_ times - is it worth caching the value into a local variable?
will-v-pi (Migrated from github.com) reviewed 2026-05-07 10:25:44 -05:00
@ -0,0 +320,4 @@
}
bool psram_check_address(void* addr) {
uint32_t offset = (uint32_t)addr - (XIP_BASE + flash_devinfo_size_to_bytes(FLASH_DEVINFO_SIZE_MAX));
will-v-pi (Migrated from github.com) commented 2026-05-07 10:25:44 -05:00

Yes, if you're passing it a flash or bootrom address - but in that case it will still return false as offset will be very large

Yes, if you're passing it a flash or bootrom address - but in that case it will still return false as `offset` will be very large
will-v-pi (Migrated from github.com) reviewed 2026-05-07 10:27:31 -05:00
@ -0,0 +1,449 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 10:27:31 -05:00

So that if flash_devinfo is already configured by OTP that is always used - there isn't an equivalent NONE value for the cs_gpio as 0 is valid, so checking the cs_size is the correct thing to do

So that if flash_devinfo is already configured by OTP that is always used - there isn't an equivalent NONE value for the cs_gpio as 0 is valid, so checking the cs_size is the correct thing to do
will-v-pi (Migrated from github.com) reviewed 2026-05-07 10:29:23 -05:00
@ -0,0 +346,4 @@
bool __unused flash_devinfo_not_configured = flash_devinfo_get_cs_size(1) == FLASH_DEVINFO_SIZE_NONE;
// Setup flash_devinfo from compile definitions
#if defined(PICO_PSRAM_CS_PIN) && !PICO_AUTO_DETECT_PSRAM_CS
will-v-pi (Migrated from github.com) commented 2026-05-07 10:29:23 -05:00

Could be useful if you're compiling for a board with PSRAM, but want to enable auto-detection to work on other boards with different CS pins, or boards without PSRAM - enabling auto-detection overrides whatever is defined in the board header, see my priority list in the PR description

Could be useful if you're compiling for a board with PSRAM, but want to enable auto-detection to work on other boards with different CS pins, or boards without PSRAM - enabling auto-detection overrides whatever is defined in the board header, see my priority list in the PR description
lurch (Migrated from github.com) reviewed 2026-05-07 10:33:44 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 10:33:44 -05:00

I found this code pretty hard to read. Instead of setting a clear_start value and then iterating from clear_start to 7, would it make more sense to have a clear_regions value and then iterate from (8 - clear_regions) to 7? (or similar, if I've got the maths slightly wrong)

I found this code pretty hard to read. Instead of setting a `clear_start` value and then iterating from `clear_start` to `7`, would it make more sense to have a `clear_regions` value and then iterate from `(8 - clear_regions)` to `7`? (or similar, if I've got the maths slightly wrong)
lurch (Migrated from github.com) reviewed 2026-05-07 10:41:51 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 10:41:51 -05:00

Might be worth adding a comment that these values are only used by runtime_init_setup_psram and are simply passed as a parameter to psram_detect_cs_and_size? When I first read this header file I incorrectly assumed that psram_detect_cs_and_size was doing some kind of filtering based on these values.

But that raises an interesting question - what happens if psram_detect_cs_and_size is given a GPIO number (e.g. 2) which isn't valid as a PSRAM CS pin?

Might be worth adding a comment that these values are only used by `runtime_init_setup_psram` and are simply passed as a parameter to `psram_detect_cs_and_size`? When I first read this header file I incorrectly assumed that `psram_detect_cs_and_size` was doing some kind of filtering based on these values. But that raises an interesting question - what happens if `psram_detect_cs_and_size` is given a GPIO number (e.g. 2) which isn't valid as a PSRAM CS pin?
lurch (Migrated from github.com) reviewed 2026-05-07 10:44:32 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 10:44:32 -05:00

Maybe something like skip any _DEFAULT_ GPIOs defined in the board-header when auto-detecting psram chip select pin ?

Maybe something like `skip any _DEFAULT_ GPIOs defined in the board-header when auto-detecting psram chip select pin` ?
lurch (Migrated from github.com) reviewed 2026-05-07 10:51:30 -05:00
@ -0,0 +320,4 @@
}
bool psram_check_address(void* addr) {
uint32_t offset = (uint32_t)addr - (XIP_BASE + flash_devinfo_size_to_bytes(FLASH_DEVINFO_SIZE_MAX));
lurch (Migrated from github.com) commented 2026-05-07 10:51:30 -05:00

Ahh yes, I keep forgetting about that behaviour of unsigned arithmetic 😂 (too much time spent writing Python)

Ahh yes, I keep forgetting about that behaviour of unsigned arithmetic :joy: (too much time spent writing Python)
lurch (Migrated from github.com) reviewed 2026-05-07 11:00:23 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 11:00:23 -05:00

Ahhh, so if (upon entering this function) flash_devinfo_get_cs_size(1) != FLASH_DEVINFO_SIZE_NONE then I guess that must mean that the PSRAM was configured in OTP, and so large chunks of this function are effectively redundant, regardless of what other #defines have been set? Might be worth adding a comment about that.

Ahhh, so if (upon entering this function) `flash_devinfo_get_cs_size(1) != FLASH_DEVINFO_SIZE_NONE` then I guess that must mean that the PSRAM was configured in OTP, and so large chunks of this function are effectively redundant, regardless of what other `#define`s have been set? Might be worth adding a comment about that.
lurch (Migrated from github.com) reviewed 2026-05-07 11:01:59 -05:00
@ -0,0 +346,4 @@
bool __unused flash_devinfo_not_configured = flash_devinfo_get_cs_size(1) == FLASH_DEVINFO_SIZE_NONE;
// Setup flash_devinfo from compile definitions
#if defined(PICO_PSRAM_CS_PIN) && !PICO_AUTO_DETECT_PSRAM_CS
lurch (Migrated from github.com) commented 2026-05-07 11:01:59 -05:00

see my priority list in the PR description

Thanks for the expanded PR description, feels like some of that might be useful in psram.h

> see my priority list in the PR description Thanks for the expanded PR description, feels like some of that might be useful in `psram.h`
will-v-pi (Migrated from github.com) reviewed 2026-05-07 11:04:43 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 11:04:43 -05:00

I can move them into psram.c so they're only available there

If you pass an invalid GPIO it will still try, but it will always fail to find PSRAM, as it will be configuring the pin for a different function (GPIO_FUNC_CORESIGHT_TRACE, GPIO_FUNC_GPCK, or just an invalid function) - same as if you passed an invalid function to gpio_set_function

I can move them into `psram.c` so they're only available there If you pass an invalid GPIO it will still try, but it will always fail to find PSRAM, as it will be configuring the pin for a different function (GPIO_FUNC_CORESIGHT_TRACE, GPIO_FUNC_GPCK, or just an invalid function) - same as if you passed an invalid function to `gpio_set_function`
will-v-pi (Migrated from github.com) reviewed 2026-05-07 11:06:56 -05:00
@ -0,0 +1,449 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-07 11:06:55 -05:00

It gets modified at the start, but I can cache it for the last section

It gets modified at the start, but I can cache it for the last section
lurch (Migrated from github.com) reviewed 2026-05-07 11:34:21 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-07 11:34:20 -05:00

Ahh, so no harmful side-effects; good to know 🙂

Ahh, so no harmful side-effects; good to know :slightly_smiling_face:
lurch (Migrated from github.com) reviewed 2026-05-07 11:40:02 -05:00
lurch (Migrated from github.com) commented 2026-05-07 11:40:01 -05:00

Looks like you could do something similar for flash_devinfo_size_to_bytes(psram_flash_devinfo_size) 😉

Looks like you could do something similar for `flash_devinfo_size_to_bytes(psram_flash_devinfo_size)` :wink:
will-v-pi (Migrated from github.com) reviewed 2026-05-08 09:18:42 -05:00
@ -0,0 +1,449 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-08 09:18:42 -05:00

I've rearranged the function to make it clearer what's going on

I've rearranged the function to make it clearer what's going on
will-v-pi (Migrated from github.com) reviewed 2026-05-08 10:06:49 -05:00
@ -0,0 +346,4 @@
bool __unused flash_devinfo_not_configured = flash_devinfo_get_cs_size(1) == FLASH_DEVINFO_SIZE_NONE;
// Setup flash_devinfo from compile definitions
#if defined(PICO_PSRAM_CS_PIN) && !PICO_AUTO_DETECT_PSRAM_CS
will-v-pi (Migrated from github.com) commented 2026-05-08 10:06:49 -05:00

Have added lots of it there

Have added lots of it there
lurch (Migrated from github.com) reviewed 2026-05-08 11:49:46 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-08 11:49:46 -05:00

Seems like this could just be:

        num_cs_gpios = remove_defaults_from_cs_gpios(cs_gpios, num_cs_gpios);

?

Seems like this could just be: ```suggestion num_cs_gpios = remove_defaults_from_cs_gpios(cs_gpios, num_cs_gpios); ``` ?
lurch (Migrated from github.com) reviewed 2026-05-08 11:52:15 -05:00
@ -0,0 +392,4 @@
uint32_t psram_words = (uint32_t)(&__psram_end__ - &__psram_start__);
if (psram_words > psram_word_size) {
// Setup to bus fault for variables that don't fit in available PSRAM
int clear_regions = 0; // Clear no regions by default
lurch (Migrated from github.com) commented 2026-05-08 11:52:15 -05:00

This is more readable now thanks, and I can now see that this "Clear no regions by default" applies when the PSRAM size is 16MB 🙂

This is more readable now thanks, and I can now see that this "Clear no regions by default" applies when the PSRAM size is 16MB :slightly_smiling_face:
lurch (Migrated from github.com) reviewed 2026-05-08 11:55:26 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-08 11:55:25 -05:00

I was confused by this line at first, and thought it was missing a comma after "CS1" 😉 But now that I see what it's saying, would this be clearer?

 *      the flash_devinfo size and GPIO for CS1
I was confused by this line at first, and thought it was missing a comma after "CS1" :wink: But now that I see what it's saying, would this be clearer? ```suggestion * the flash_devinfo size and GPIO for CS1 ```
lurch (Migrated from github.com) reviewed 2026-05-08 11:57:34 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-08 11:57:34 -05:00
 *      on CS1. This will attempt to use all available QMI CS1n GPIOs as chip selects, so they

might be slightly clearer?

```suggestion * on CS1. This will attempt to use all available QMI CS1n GPIOs as chip selects, so they ``` might be _slightly_ clearer?
lurch (Migrated from github.com) reviewed 2026-05-08 12:02:17 -05:00
@ -0,0 +23,4 @@
* 2. If `PICO_AUTO_DETECT_PSRAM` is set it will attempt to detect PSRAM size and CS GPIO
* on CS1. This will attempt to use all available QMI CS1n GPIOs as chip selects, so they
* will be wiggled. By default, it will skip over some which are defined in the board
* header (see `PICO_AUTO_DETECT_PSRAM_CS_SKIP_DEFAULTS`).
lurch (Migrated from github.com) commented 2026-05-08 12:02:17 -05:00

Hmm, I wonder if there's a use-case for being able to say "never attempt to probe this list-of-GPIOs for PSRAM CS" if there are certain pins which the user doesn't want to get wiggled, but which aren't covered by any of the board-header _DEFAULT_ defines? 🤔

Hmm, I wonder if there's a use-case for being able to say "never attempt to probe this list-of-GPIOs for PSRAM CS" if there are certain pins which the user doesn't want to get wiggled, but which _aren't_ covered by any of the board-header `_DEFAULT_` defines? :thinking:
lurch (Migrated from github.com) reviewed 2026-05-08 12:03:01 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-08 12:03:01 -05:00
 *     - If the CS GPIO is known and set in `PICO_PSRAM_CS_PIN`, you can just enable
```suggestion * - If the CS GPIO is known and set in `PICO_PSRAM_CS_PIN`, you can just enable ```
lurch (Migrated from github.com) reviewed 2026-05-08 12:04:16 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-08 12:04:15 -05:00

Should this be

 * Only the `PICO_AUTO_DETECT_PSRAM` and `PICO_AUTO_DETECT_PSRAM_SIZE` methods will verify that PSRAM is present before

?

Should this be ```suggestion * Only the `PICO_AUTO_DETECT_PSRAM` and `PICO_AUTO_DETECT_PSRAM_SIZE` methods will verify that PSRAM is present before ``` ?
lurch (Migrated from github.com) reviewed 2026-05-08 12:11:17 -05:00
@ -0,0 +41,4 @@
* If there are variables placed in PSRAM, XIP will be set up to cause bus faults on any
* access to PSRAM addresses greater than the size available. The \ref psram_check_address
* function should be used before accessing variables in PSRAM when auto-detection is on,
* to prevent these bus faults.
lurch (Migrated from github.com) commented 2026-05-08 12:11:16 -05:00

Should / could the kitchen_sink example demonstrate doing this?

Should / could the kitchen_sink example demonstrate doing this?
lurch (Migrated from github.com) reviewed 2026-05-08 12:18:18 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-08 12:18:18 -05:00
 * you can also read/write the memory addresses directly.

? (I assume that PSRAM isn't write-only 😉 )

```suggestion * you can also read/write the memory addresses directly. ``` ? (I assume that PSRAM isn't write-only :wink: )
lurch (Migrated from github.com) reviewed 2026-05-08 12:22:13 -05:00
@ -0,0 +1,269 @@
/*
lurch (Migrated from github.com) commented 2026-05-08 12:22:13 -05:00

This might be one of my stupid questions, but if you wanted to have variables placed in PSRAM and also read/write PSRAM directly, is there a way of knowing which addresses in PSRAM aren't used by the variables placed in PSRAM?
(or maybe that's something you'd never need to do in practice?)

This might be one of my stupid questions, but if you wanted to have variables placed in PSRAM and **also** read/write PSRAM directly, is there a way of knowing which addresses in PSRAM _aren't_ used by the variables placed in PSRAM? (or maybe that's something you'd never need to do in practice?)
lurch commented 2026-05-09 15:56:03 -05:00 (Migrated from github.com)

Should this PR also be adding hardware_psram to docs/index.h ?
Should line 15 of src/rp2_common/hardware_psram/include/hardware/psram.h be declaring hardware_psram (as used on e.g. line 133) instead of pico_psram?

Should this PR also be adding `hardware_psram` to `docs/index.h` ? Should line 15 of `src/rp2_common/hardware_psram/include/hardware/psram.h` be declaring `hardware_psram` (as used on e.g. line 133) instead of `pico_psram`?
will-v-pi (Migrated from github.com) reviewed 2026-05-11 06:26:20 -05:00
@ -0,0 +41,4 @@
* If there are variables placed in PSRAM, XIP will be set up to cause bus faults on any
* access to PSRAM addresses greater than the size available. The \ref psram_check_address
* function should be used before accessing variables in PSRAM when auto-detection is on,
* to prevent these bus faults.
will-v-pi (Migrated from github.com) commented 2026-05-11 06:26:20 -05:00

Yes, it already tests this using psram_or_malloc which bus faults in the tiny PSRAM version - a demonstration would be more suited to a pico-example

Yes, it already tests this using `psram_or_malloc` which bus faults in the tiny PSRAM version - a demonstration would be more suited to a pico-example
will-v-pi (Migrated from github.com) reviewed 2026-05-11 06:33:04 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-11 06:33:04 -05:00

You can read extern uint32_t __psram_end__; to get the end of the region allocated to variables

You can read `extern uint32_t __psram_end__;` to get the end of the region allocated to variables
will-v-pi (Migrated from github.com) reviewed 2026-05-13 09:55:03 -05:00
@ -0,0 +1,269 @@
/*
will-v-pi (Migrated from github.com) commented 2026-05-13 09:55:03 -05:00

I'll go for this, as it's a sub-method, rather than a different method

 * Only the `PICO_AUTO_DETECT_PSRAM` methods (including `PICO_AUTO_DETECT_PSRAM_SIZE`) will
 * verify that PSRAM is present before using it.
I'll go for this, as it's a sub-method, rather than a different method ``` * Only the `PICO_AUTO_DETECT_PSRAM` methods (including `PICO_AUTO_DETECT_PSRAM_SIZE`) will * verify that PSRAM is present before using it. ```
will-v-pi (Migrated from github.com) reviewed 2026-05-13 10:55:48 -05:00
@ -0,0 +23,4 @@
* 2. If `PICO_AUTO_DETECT_PSRAM` is set it will attempt to detect PSRAM size and CS GPIO
* on CS1. This will attempt to use all available QMI CS1n GPIOs as chip selects, so they
* will be wiggled. By default, it will skip over some which are defined in the board
* header (see `PICO_AUTO_DETECT_PSRAM_CS_SKIP_DEFAULTS`).
will-v-pi (Migrated from github.com) commented 2026-05-13 10:55:47 -05:00

Maybe, and that could be a better way of including the default pins

Maybe, and that could be a better way of including the default pins
lurch (Migrated from github.com) reviewed 2026-05-13 12:19:49 -05:00
@ -0,0 +94,4 @@
}
return psram_size;
}
lurch (Migrated from github.com) commented 2026-05-13 12:19:49 -05:00

"comma separated list..." "type=bool"
🤔 😆

Also, might be better to just remove the default=..., rather than saying default=undefined

"comma separated list..." "type=bool" :thinking: :laughing: Also, might be better to just remove the `default=...`, rather than saying `default=undefined`
lurch (Migrated from github.com) reviewed 2026-05-13 12:23:52 -05:00
@ -0,0 +205,4 @@
}
uint32_t rxdelay = divisor;
if (clock_hz / divisor > 100000000) {
rxdelay += 1;
lurch (Migrated from github.com) commented 2026-05-13 12:23:52 -05:00

Depending on what other #defines are set, I guess this might end up as e.g.

static const uint8_t default_pin_cs_gpios[] = {
    PICO_DEFAULT_UART_TX_PIN,
};

(i.e. with nothing after the last comma). Is that allowed in C?

Depending on what other `#defines` are set, I guess this might end up as e.g. ```c static const uint8_t default_pin_cs_gpios[] = { PICO_DEFAULT_UART_TX_PIN, }; ``` (i.e. with nothing after the last comma). Is that allowed in C?
will-v-pi (Migrated from github.com) reviewed 2026-05-13 12:25:21 -05:00
@ -0,0 +205,4 @@
}
uint32_t rxdelay = divisor;
if (clock_hz / divisor > 100000000) {
rxdelay += 1;
will-v-pi (Migrated from github.com) commented 2026-05-13 12:25:21 -05:00

Yes, trailing commas are fine in C

Yes, trailing commas are fine in C
will-v-pi (Migrated from github.com) reviewed 2026-05-13 12:26:26 -05:00
@ -0,0 +94,4 @@
}
return psram_size;
}
will-v-pi (Migrated from github.com) commented 2026-05-13 12:26:26 -05:00
// PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PINS, comma separated list of extra pins to not check when auto-detecting psram chip select pin, type=list, group=hardware_psram
```suggestion // PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PINS, comma separated list of extra pins to not check when auto-detecting psram chip select pin, type=list, group=hardware_psram ```
lurch (Migrated from github.com) reviewed 2026-05-13 12:43:25 -05:00
@ -0,0 +94,4 @@
}
return psram_size;
}
lurch (Migrated from github.com) commented 2026-05-13 12:43:24 -05:00

Unfortunately extract_configs.py only supports type=int, type=bool and type=enum 😕

Although given that there are so few QMI CS pins, maybe we could just use e.g.

// PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PIN_0
// PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PIN_8
// PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PIN_19
// PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PIN_47

(all with type=bool)
🤷

Unfortunately [extract_configs.py](https://github.com/raspberrypi/pico-sdk/blob/master/tools/extract_configs.py#L101) only supports `type=int`, `type=bool` and `type=enum` :confused: Although given that there are so few QMI CS pins, maybe we could just use e.g. ``` // PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PIN_0 // PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PIN_8 // PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PIN_19 // PICO_CONFIG: PICO_AUTO_DETECT_PSRAM_CS_SKIP_PIN_47 ``` (all with `type=bool`) :shrug:
will-v-pi (Migrated from github.com) reviewed 2026-05-14 05:07:14 -05:00
@ -0,0 +94,4 @@
}
return psram_size;
}
will-v-pi (Migrated from github.com) commented 2026-05-14 05:07:14 -05:00

The extract_cmake_configs.py supports list, so I've copied that support in, and it seems to work fine

The [extract_cmake_configs.py](https://github.com/raspberrypi/pico-sdk/blob/master/tools/extract_cmake_configs.py#L120) supports list, so I've copied that support in, and it seems to work fine
lurch (Migrated from github.com) reviewed 2026-05-14 08:35:56 -05:00
@ -0,0 +1,449 @@
/*
lurch (Migrated from github.com) commented 2026-05-14 08:35:55 -05:00

IIRC we normally put the PICO_CONFIG: entries in the header file? (although this isn't a hard'n'fast rule as we currently have 16 exceptions)

IIRC we normally put the `PICO_CONFIG:` entries in the header file? (although this isn't a hard'n'fast rule as we currently have 16 exceptions)
lurch (Migrated from github.com) reviewed 2026-05-14 08:36:31 -05:00
@ -0,0 +94,4 @@
}
return psram_size;
}
lurch (Migrated from github.com) commented 2026-05-14 08:36:31 -05:00

Nice.

Nice.
kilograham (Migrated from github.com) approved these changes 2026-05-20 17:22:22 -05:00
Sign in to join this conversation.
No reviewers
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-raspberrypi/pico-sdk!2919
No description provided.