Skip to content
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.

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

include/drivers/flash: document on the unrestricted alignment of reads #23628

Merged

Conversation

nvlsianpu
Copy link
Collaborator

Unaligned read-out capability become fact among all drivers.
Let's cut this in stone as API requirement.

fixes #16439

Signed-off-by: Andrzej Puzdrowski andrzej.puzdrowski@nordicsemi.no

* Most of flash drivers support unaligned flash access, but some have
* restrictions on the read offset or/and the read size. Please refer to
* the driver implementation to get details on the read alignment requirement.
* Each of flash drivers support unaligned flash access, without any
Copy link
Member

Choose a reason for hiding this comment

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

s/Each of/ All

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@nvlsianpu nvlsianpu force-pushed the doc/flash-driver-read-unalined branch from 6ee8bf3 to 90fafe7 Compare March 20, 2020 13:01
include/drivers/flash.h Outdated Show resolved Hide resolved
@carlescufi
Copy link
Member

Blocked by #25292

@carlescufi
Copy link
Member

carlescufi commented May 19, 2020

API Meeting:

  • @nvlsianpu did you check all the current drivers beyond the qspi?
  • @nvlsianpu did you consider a compile-time option? I seem to recall you did, but I don't remember what the outcome of that dicussion was
  • During the API meeting it was suggested we could default to not supporting unaligned access unless a Kconfig option is enabled, hence the previous question

cc @pabigot @sglass68
This will be scheduled for discussion in the next API meeting.

@pabigot
Copy link
Collaborator

pabigot commented May 19, 2020

Blocked by #25292

I don't think that PR blocks this change, but it highlights that QSPI needs work before the new requirement can be confirmed. That is a minimal change for a very specific function to support using an external flash to store the alternative image for mcuboot (fixing #25289).

Also we should extend the AU promise to not only alignment but memory region: i.e. that it is permitted to writes of data that is located in flash, rather than RAM, which impacts any implementation (like QSPI) that uses DMA that only works on RAM addresses.

sigvartmh added a commit to sigvartmh/fw-nrfconnect-zephyr-1 that referenced this pull request May 29, 2020
Adds the functionality of doing one byte write to QSPI nor
driver. This is achived through the use of stack in the function calls.

Can be removed when requirements in
zephyrproject-rtos/zephyr#23628
are fulfilled in the QSPI NOR driver.

NCSDK-4628

Signed-off-by: Sigvart Hovland <sigvart.m@gmail.com>
tejlmand pushed a commit to nrfconnect/sdk-zephyr that referenced this pull request May 29, 2020
Adds the functionality of doing one byte write to QSPI nor
driver. This is achived through the use of stack in the function calls.

Can be removed when requirements in
zephyrproject-rtos/zephyr#23628
are fulfilled in the QSPI NOR driver.

NCSDK-4628

Signed-off-by: Sigvart Hovland <sigvart.m@gmail.com>
mbolivar-nordic pushed a commit to mbolivar-nordic/sdk-zephyr that referenced this pull request Jun 1, 2020
Adds the functionality of doing one byte write to QSPI nor
driver. This is achived through the use of stack in the function calls.

Can be removed when requirements in
zephyrproject-rtos/zephyr#23628
are fulfilled in the QSPI NOR driver.

NCSDK-4628

Signed-off-by: Sigvart Hovland <sigvart.m@gmail.com>
(cherry picked from commit 9d4faff)
mbolivar-nordic pushed a commit to mbolivar-nordic/sdk-zephyr that referenced this pull request Jun 1, 2020
Adds the functionality of doing one byte write to QSPI nor
driver. This is achived through the use of stack in the function calls.

Can be removed when requirements in
zephyrproject-rtos/zephyr#23628
are fulfilled in the QSPI NOR driver.

NCSDK-4628

Signed-off-by: Sigvart Hovland <sigvart.m@gmail.com>
(cherry picked from commit 9d4faff)
@nvlsianpu
Copy link
Collaborator Author

#26772 going to add unrestricted alignment of reads for nordic qspi flash driver.

Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

I believe the exception for QSPI has been addressed, so this can be updated and merged after other issues addressed.

We should also document the expectations regarding memory region for material written to the device. In particular that even if a device only supports DMA, and DMA requires the source buffer be in RAM, that it must still be permitted to write material where the source buffer is in flash. Or, if that is not required, document that on some devices such writes may fail and will return a particular error code.

@nvlsianpu
Copy link
Collaborator Author

@pabigot

We should also document the expectations regarding memory region for material written to the device. In particular that even if a device only supports DMA, and DMA requires the source buffer be in RAM, that it must still be permitted to write material where the source buffer is in flash. Or, if that is not required, document that on some devices such writes may fail and will return a particular error code.

For being consistent I prefer to force all to support source buffer anywhere.

@nvlsianpu nvlsianpu force-pushed the doc/flash-driver-read-unalined branch from 90fafe7 to 1523997 Compare August 5, 2020 14:54
@nvlsianpu
Copy link
Collaborator Author

@pabigot Added note on write source buffer location.

@carlescufi
Copy link
Member

@pabigot Added note on write source buffer location.

Thanks @nvlsianpu. @pabigot please take another look.

@@ -121,6 +120,8 @@ static inline int z_impl_flash_read(struct device *dev, off_t offset, void *data
/**
* @brief Write buffer into flash memory.
*
* All flash drivers support source buffer located either in RAM or SoC flash.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For complete clarity this should include the same text on alignment and sizes that was added for read.

Copy link
Collaborator Author

@nvlsianpu nvlsianpu Aug 5, 2020

Choose a reason for hiding this comment

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

check once more, propose redaction as I'm not english native

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about for read:

All flash drivers support reads without alignment restrictions on the read offset, the read size, or the destination address.

and for write:

All flash drivers support a source buffer located either in RAM or SoC flash, without alignment restrictions on the source address, or write size or offset.

?

@nvlsianpu nvlsianpu force-pushed the doc/flash-driver-read-unalined branch from 1523997 to 84535a4 Compare August 5, 2020 15:10
Unaligned read-out capability become fact among all drivers.
Let's cut this in stone as API requirement.

fixes zephyrproject-rtos#16439

Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
Document unrestricted source buffer location while writing to
the flash.

Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
@nvlsianpu nvlsianpu force-pushed the doc/flash-driver-read-unalined branch from 84535a4 to 4b099e6 Compare August 5, 2020 15:56
@nashif nashif merged commit 4d4455b into zephyrproject-rtos:master Aug 5, 2020
nvlsianpu added a commit to nvlsianpu/zephyr that referenced this pull request Sep 8, 2020
…ites

Telling that all the driver supports unaligned writes was and
is not true. Drivers only have to support any source buffer.

This fix message introduced by zephyrproject-rtos#23628

Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
carlescufi pushed a commit that referenced this pull request Sep 10, 2020
…ites

Telling that all the driver supports unaligned writes was and
is not true. Drivers only have to support any source buffer.

This fix message introduced by #23628

Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flash: unify read alignment requirements
7 participants