Skip to content

drivers: flash: stm32 xspi flash read with memcopy when executing #93762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+12 −13
Open

drivers: flash: stm32 xspi flash read with memcopy when executing #93762

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions drivers/flash/flash_stm32_xspi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,7 @@ static int flash_stm32_xspi_erase(const struct device *dev, off_t addr,
goto erase_end;
}
}
#endif
#endif /* CONFIG_STM32_MEMMAP */

XSPI_RegularCmdTypeDef cmd_erase = {
.OperationType = HAL_XSPI_OPTYPE_COMMON_CFG,
Expand Down Expand Up @@ -1173,7 +1173,7 @@ static int flash_stm32_xspi_read(const struct device *dev, off_t addr,
{
const struct flash_stm32_xspi_config *dev_cfg = dev->config;
struct flash_stm32_xspi_data *dev_data = dev->data;
int ret;
int ret = 0;

if (!xspi_address_is_valid(dev, addr, size)) {
LOG_ERR("Error: address or size exceeds expected values: "
Expand All @@ -1186,29 +1186,31 @@ static int flash_stm32_xspi_read(const struct device *dev, off_t addr,
return 0;
}

#ifdef CONFIG_STM32_MEMMAP
#if defined(CONFIG_STM32_MEMMAP) || defined(CONFIG_STM32_APP_IN_EXT_FLASH)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if defined(CONFIG_STM32_MEMMAP) || defined(CONFIG_STM32_APP_IN_EXT_FLASH)
#if defined(CONFIG_STM32_MEMMAP) || (defined(CONFIG_STM32_APP_IN_EXT_FLASH) && defined(CONFIG_XIP))

This is needed in my opinion, since the app image can be stored in ext flash but then be executed in RAM (after being copied there by MCUboot). In that case, we are not sure if the Flash is still MemMapped, because the app can exit Memmap mode and use a storage partition in external Flash for ex. Does this make sense?

zephyr/share/sysbuild/image_configurations/MAIN_image_default.cmake

Line 40 in 00e217d

set_config_bool(${ZCMAKE_APPLICATION} CONFIG_XIP n)

But maybe CONFIG_XIP is not the appropriate flag to check, since it is not disabled in SINGLE_APP_RAM_LOAD mode for ex.

zephyr/share/sysbuild/image_configurations/MAIN_image_default.cmake

Line 42 in 00e217d

elseif(SB_CONFIG_MCUBOOT_MODE_SINGLE_APP_RAM_LOAD)

I'm no longer sure of the exact semantics of Zephyr XiP at this point, I think it just means "app is executed where it was last loaded, RAM, Flash, PSRAM...by either flashloader or bootloader" or something close to this.

@nordicjm Any thoughts on this? what am I getting wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waiting for @nordicjm comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will perhaps need to check for MCUboot modes instead, to know if Memmap mode is still required after the image has started executing, see #85254 (comment)

It appears that XiP semantics in Zephyr needs real clarification in the docs & Kconfig help text, especially in relationship to the usage of a bootloader that can move the app image around, and Zephyr Code Relocation as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be honest I think the comment about single loader is wrong, it is executing from RAM, it should have XIP set to n, XIP means running from flash, the help text is clear on that:

        help
          This option allows the kernel to operate with its text and read-only
          sections residing in ROM (or similar read-only memory). Not all boards
          support this option so it must be used with care; you must also
          supply a linker command file when building your image. Enabling this
          option increases both the code and data footprint of the image.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XIP means running from flash, the help text is clear on that

But is the help text still accurate today, compared to when XiP was first introduced?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XIP means running from flash, the help text is clear on that

But is the help text still accurate today, compared to when XiP was first introduced?

yes, you are still running from read only media (one could argue that you could erase the flash sector you are running meaning it's not read only but that's not going to go well)

ARG_UNUSED(dev_cfg);
ARG_UNUSED(dev_data);

xspi_lock_thread(dev);
/*
* When the call is made by an app executing in external flash,
* skip the memory-mapped mode check
*/
#ifdef CONFIG_STM32_MEMMAP

/* Do reads through memory-mapping instead of indirect */
if (!stm32_xspi_is_memorymap(dev)) {
ret = stm32_xspi_set_memorymap(dev);
if (ret != 0) {
LOG_ERR("READ: failed to set memory mapped");
goto read_end;
return ret;
}
}

__ASSERT_NO_MSG(stm32_xspi_is_memorymap(dev));

#endif /* CONFIG_STM32_MEMMAP */
uintptr_t mmap_addr = STM32_XSPI_BASE_ADDRESS + addr;

LOG_DBG("Memory-mapped read from 0x%08lx, len %zu", mmap_addr, size);
memcpy(data, (void *)mmap_addr, size);
ret = 0;
goto read_end;
return ret;
#else
XSPI_RegularCmdTypeDef cmd = xspi_prepare_cmd(dev_cfg->data_mode, dev_cfg->data_rate);

Expand Down Expand Up @@ -1274,13 +1276,10 @@ static int flash_stm32_xspi_read(const struct device *dev, off_t addr,
xspi_lock_thread(dev);

ret = xspi_read_access(dev, &cmd, data, size);
goto read_end;
#endif

read_end:
xspi_unlock_thread(dev);

return ret;
#endif /* CONFIG_STM32_MEMMAP || CONFIG_STM32_APP_IN_EXT_FLASH */
}

/* Function to write the flash (page program) : with possible OCTO/SPI and STR/DTR */
Expand Down