-
Notifications
You must be signed in to change notification settings - Fork 342
softperipheral: Add initial sQSPI #1694
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
Conversation
7c47965 to
bb0e307
Compare
4dd7e0e to
a25fe76
Compare
softperipheral/CHANGELOG.rst
Outdated
| See the following list changes: | ||
|
|
||
| * Added: | ||
| * First implementation of Soft Peripheral **sQSPI**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * First implementation of Soft Peripheral **sQSPI**. | |
| * The first implementation of Soft Peripheral sQSPI for the nRF54L15 and nRF54H20 SoCs. |
empty line needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f5a9456
softperipheral/doc/introduction.rst
Outdated
| It also allows to include additional instances of a peripheral in case the platform lacks sufficient hardware peripherals. | ||
|
|
||
| In most cases, the features and performance of a soft peripheral are equivalent to those of a hardware peripheral. | ||
| However, there may be some limitations. For more information, see the corresponding limitations page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| However, there may be some limitations. For more information, see the corresponding limitations page. | |
| However, there may be some limitations. For more information, see the :ref:`sqspi_limitations` page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f5a9456
| v0.1.0 | ||
| ************ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbc - Is a separate versioning needed?
| v0.1.0 | |
| ************ | |
| |NCS| v3.0.0 | |
| ************ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog at root level is meant to track changes for NCS releases, but as we add more softperipherals we will have each of them have their own version
|
|
||
| * Added: | ||
|
|
||
| * TX, RX, and TX/RX :ref:`SPI transfer modes<sqspi_features_spi_modes>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * TX, RX, and TX/RX :ref:`SPI transfer modes<sqspi_features_spi_modes>`. | |
| * TX, RX, and TX/RX :ref:`SPI transfer modes<sqspi_features_spi_modes>` support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f5a9456
|
|
||
| The sQSPI application requires FastPad bias configuration to run at high frequency on SCLK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The sQSPI application requires FastPad bias configuration to run at high frequency on SCLK. | |
| Additionally, the sQSPI application requires FastPad bias configuration to run at high frequency on SCLK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f5a9456
| Memory retention configuration | ||
| ****************************** | ||
|
|
||
| The sQSPI Soft Peripheral requires RAM retention in order to go into the lowest power consumption mode (API call to :c:func:`nrfx_qspi2_deactivate`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The sQSPI Soft Peripheral requires RAM retention in order to go into the lowest power consumption mode (API call to :c:func:`nrfx_qspi2_deactivate`). | |
| The sQSPI Soft Peripheral requires RAM retention in order to go into the lowest power consumption mode, which can be called through the :c:func:`nrfx_qspi2_deactivate` API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f5a9456
| ***************** | ||
|
|
||
| The sQSPI Soft Peripheral operates directly from non-cached RAM. | ||
| Your build environment must reserve the required RAM and ensure that it is readable/writable by both the Application Core and the FLPR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Your build environment must reserve the required RAM and ensure that it is readable/writable by both the Application Core and the FLPR. | |
| Your build environment must reserve the required RAM and ensure that it is readable and writable by both the application core and the FLPR core. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f5a9456
| The build environment described in the :ref:`nrf54h20_porting_guide_code` section must comply with this requirements. | ||
| This includes proper settings in linker scripts, device tree specifications (DTS), and resource allocation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a general remark, shouldn't this appear somewhere at the beginning of this guide?
| The build environment described in the :ref:`nrf54h20_porting_guide_code` section must comply with this requirements. | |
| This includes proper settings in linker scripts, device tree specifications (DTS), and resource allocation. | |
| The build environment described in the :ref:`nrf54h20_porting_guide_code` section must comply with these requirements. | |
| This includes proper settings in linker scripts, device tree specifications (DTS), and resource allocation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f5a9456
| Application Core and FLPR configuration | ||
| *************************************** | ||
|
|
||
| For an sQSPI application to run properly, nRF54L15 must run at highest base clock frequency. The user/integrator should take care of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all suggestions are the same for nRF54L15 (as in case of nRF54H20 section)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f5a9456
| @@ -0,0 +1,231 @@ | |||
| .. _nrf54H20_porting_guide: | |||
|
|
|||
| nRF54H20 porting guide | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, there's a lot of duplication, but when I ran a diff on it, it showed few changes across the doc (additional tables and code-blocks), so if we were to create tabs for it I think it could be confusing. For now I'd leave it as is but I'll keep thinking of some other solution
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
| */ | ||
|
|
||
| #ifndef SP_SQSPI_FIRMWARE_H__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally dislike using "firmware" here. This is really a full VPR image, so I would call it "image".
| #endif | ||
|
|
||
| /** | ||
| * @defgroup nrfx_qspi2 QSPI2 driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename this to something clearer and cleaner. For starters, this is not only QSPI, so it should not be called QSPI. I would suggest xspi or mspi. If we don't do it now, then we will have to do it later, breaking compatibility.
CC @masz-nordic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this name is not perfect. The source of it is here: https://projecttools.nordicsemi.no/nightly/topic/wezen/qspi.html?cp=3_8_0_0_8_20 . So, unless we change the source, I don't think we should change the driver prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest reconsidering further name changes to your APIs:
- for the HW SSI, there is an existing Zephyr driver using registers directly (so no reason for nrfx_qspi2.h)
- we will need a secondary SHIM for any SW peripheral anyways as there is no mechanism in place to distinguish between HW and SW instances on nrfx level (so not possible to reuse SHIM)
- reusing the same names it will not be possible to run in parallel HW and SW instances
- using different APIs and SHIMs will enable sw developments at own paces
- two sources of nrfx_* names may be confusing for customers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps renaming is out of scope for this PR? We took the existing API approved in this PR: https://github.com/nrfconnect/nrfx/pull/724
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. Either we do it now or else we will be stuck with this name forever. I think we should rename the nrfx API and then merge this PR here.
https://projecttools.nordicsemi.no/nightly/topic/wezen/qspi.html?cp=3_8_0_0_8_20 . So, unless we change the source, I don't think we should change the driver prefix.
@hubertmis I disagree, you wrote in https://github.com/nrfconnect/nrfx/pull/724 that this API would serve multiple peripherals with different names, so I think we should rename the nrfx API to something like nrfx_xspi or nrfx_mspi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@barsok I am not quite sure what you are suggesting here. You are happy with the current name or do you think we should change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@barsok I am not quite sure what you are suggesting here. You are happy with the current name or do you think we should change it?
I suggest changing not only the "peripheral" part of the name, but nrfx prefix as well. The reasons are above, most important are confusion for customers and making it even more confusing when nrfx will get a hw version of the same driver. Then nrfxlib and hal_nordic will contain functions with the same names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, you wrote in nrfconnect/nrfx#724 that this API would serve multiple peripherals with different names, so I think we should rename the nrfx API to something like
nrfx_xspiornrfx_mspi
The two peripherals this API currently targets are called QSPI and sQSPI. So far, all nrfx drivers follow the nrfx_peripheralname prefix. As agreed in https://github.com/nrfconnect/nrfx/pull/724 and other discussions, I would prefer not to change this rule now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have dismissed my nack, but I want to state that I think this is a mistake that will confuse users and developers alike.
|
|
||
| #ifndef SQSPI_FIRMWARE_H__ | ||
| #define SQSPI_FIRMWARE_H__ | ||
| #include "sqspi_firmware_v0.1.0.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing this to:
#if defined(NRF54L15_XXAA)
#include "nrf54l15/sqspi_firmware_v0.1.0.h"
#elif defined(NRF54H20_XXAA)
#include "nrf54h20/sqspi_firmware_v0.1.0.h"
#endif
and moving the file one level up (or maybe even place all of this next to nrfx_qspi2.c, I guess only this file is supposed to include this stuff)? This would simplify a bit the integration of the driver.
|
|
||
| Refer to the following detailed descriptions of current limitations: | ||
|
|
||
| * sQSPI does not support slave mode operations; it can only operate as a controller (master). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spi slave has changed to spi peripheral I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my understanding is that the names were controller and target? is this what you mean?
| * If you set the :c:var:`nrfx_qspi2_cfg_t.skip_gpio_cfg` variable to ``true``, the GPIO configuration is not managed by the sQSPI driver and it must be manually handled by the application. | ||
| This is a requirement for the nRF54H20 device. | ||
| * If you set the :c:var:`nrfx_qspi2_cfg_t.skip_pmux_cfg` variable to ``true``, the GPIO multiplexing is not managed by the sQSPI driver and it must be manually handled by the application. | ||
| This is a requirement for the nRF54H20 device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't dts be used for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any comments here @anangl ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This driver does not depend on the .dts infrastructure. You can set skip_pmux_cfg = true and configure the GPIO multiplexing in any way available to the application, potentially using the device tree if that's the proper way in the integration environment (e.g. Zephyr).
annwoj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more suggestions, thanks!
softperipheral/CHANGELOG.rst
Outdated
| ************ | ||
|
|
||
| This is an initial release for Soft Peripherals. | ||
| See the following list changes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| See the following list changes: | |
| See the following list of changes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c5b23b6
softperipheral/doc/introduction.rst
Outdated
| It also allows to include additional instances of a peripheral in case the platform lacks sufficient hardware peripherals. | ||
|
|
||
| In most cases, the features and performance of a soft peripheral are equivalent to those of a hardware peripheral. | ||
| However, there may be some limitations. For more information, see the :ref:`sqspi_limitations` page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| However, there may be some limitations. For more information, see the :ref:`sqspi_limitations` page. | |
| However, there may be some limitations. | |
| For more information, see the :ref:`sqspi_limitations` page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c5b23b6
softperipheral/doc/introduction.rst
Outdated
| Comparison between QSPI and sQSPI support | ||
| ***************************************** | ||
|
|
||
| You should be aware of the differences between sQSPI and QSPI features: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| You should be aware of the differences between sQSPI and QSPI features: | |
| The following section provides a detailed comparison of the features supported by the standard Quad Serial Peripheral Interface (QSPI) and the sQSPI. | |
| The comparison highlights the capabilities and limitations of each interface to aid in technical decision-making for your system design. | |
| For more details on sQSPI support, see the :ref:`sqspi_features` page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c5b23b6
softperipheral/doc/sQSPI/README.rst
Outdated
| sQSPI | ||
| ################ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| sQSPI | |
| ################ | |
| sQSPI | |
| ##### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c5b23b6
| * Full Duplex: SPI reads by switching to receiving data (MISO) after transmitting data (MOSI). | ||
| * Half Duplex: Three-wire setup (MOSI/MISO, SCLK, CSN). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Full Duplex: SPI reads by switching to receiving data (MISO) after transmitting data (MOSI). | |
| * Half Duplex: Three-wire setup (MOSI/MISO, SCLK, CSN). | |
| * Full duplex - SPI reads by switching to receiving data (MISO) after transmitting data (MOSI). | |
| * Half duplex - Three-wire setup (MOSI/MISO, SCLK, CSN). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c5b23b6
| Application core and FLPR configuration | ||
| *************************************** | ||
|
|
||
| For an sQSPI application to run properly, you must adjust the settings for the nRF54H20 SoC to run at highest base clock frequency. The user/integrator should take care of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| For an sQSPI application to run properly, you must adjust the settings for the nRF54H20 SoC to run at highest base clock frequency. The user/integrator should take care of this. | |
| For an sQSPI application to run properly, you must adjust the settings for the nRF54H20 SoC to run at highest base clock frequency. |
These were not addressed for any of the occurrences, please fix
|
|
||
| In as secure configuration, both the application core and the FLPR core of the nRF54H20 device must operate within a secure environment enabled by TrustZone Secure. | ||
|
|
||
| Non-Secure settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Non-Secure settings | |
| Non-secure settings |
|
|
||
| .. _nrf54H20_porting_guide_ram_configuration: | ||
|
|
||
| RAM Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RAM Configuration | |
| RAM configuration |
| Application core and FLPR configuration | ||
| *************************************** | ||
|
|
||
| For an sQSPI application to run properly, you must adjust the settings for the nRF54L15 SoC to run at highest base clock frequency. The user/integrator should take care of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as for nRF54H20, suggestions not applied
| // NOTE: Setting FLPR to secure | ||
| NRF_SPU00_S->PERIPH[0xC].PERM = (SPU_PERIPH_PERM_SECATTR_Secure << SPU_PERIPH_PERM_SECATTR_Pos); | ||
|
|
||
| Non-Secure settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Non-Secure settings | |
| Non-secure settings |
| RAM Configuration | ||
| ***************** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RAM Configuration | |
| ***************** | |
| RAM configuration | |
| ***************** |
softperipheral/doc/sQSPI/timing.rst
Outdated
|
|
||
| Timing parameters | ||
| ***************** | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this section (heading) necessary?
softperipheral/doc/sQSPI/timing.rst
Outdated
| Timing | ||
| ###### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Timing | |
| ###### | |
| Timing parameters | |
| ################# |
c5b23b6 to
706941e
Compare
|
After talking to @annwoj , we've decided to have a separate documentation only PR, the scope of this PR will be limited to source code |
Stale, the API name will be looked at in a separate PR
|
@lopeztel @gbakke I will ack this PR to help you get it in on time but we do have an agreement on revisiting the |
|
Some doc work is still needed ; will be done after rc-1. |
annwoj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, the doc review will continue on a separate PR
|
Doc build needs to be fixed on the manifest PR if this is to be merged: |
softperipheral/CHANGELOG.rst contains the list of changes. Signed-off-by: Luis David Lopez <[email protected]>
softperipheral/CHANGELOG.rst contains the list of changes.