Skip to content

nRF52840_DK: "qspi_api.c" check read/write WORD alignment, set clock frequency divider #8832

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

Merged
merged 1 commit into from
Dec 20, 2018
Merged

nRF52840_DK: "qspi_api.c" check read/write WORD alignment, set clock frequency divider #8832

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
66 changes: 61 additions & 5 deletions targets/TARGET_NORDIC/TARGET_NRF5x/qspi_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ TODO
- dummy cycles
*/

#define MBED_HAL_QSPI_HZ_TO_CONFIG(hz) ((32000000/(hz))-1)
#define MBED_HAL_QSPI_MAX_FREQ 32000000UL

// NRF supported R/W opcodes
Expand All @@ -68,10 +67,15 @@ TODO
#define PP4O_opcode 0x32
#define PP4IO_opcode 0x38

#define SCK_DELAY 0x05
#define WORD_MASK 0x03

static nrf_drv_qspi_config_t config;

// Private helper function to track initialization
static ret_code_t _qspi_drv_init(void);
// Private helper function to set NRF frequency divider
nrf_qspi_frequency_t nrf_frequency(int hz);

qspi_status_t qspi_prepare_command(qspi_t *obj, const qspi_command_t *command, bool write)
{
Expand Down Expand Up @@ -207,8 +211,8 @@ qspi_status_t qspi_init(qspi_t *obj, PinName io0, PinName io1, PinName io2, PinN
config.pins.io3_pin = (uint32_t)io3;
config.irq_priority = SPI_DEFAULT_CONFIG_IRQ_PRIORITY;

config.phy_if.sck_freq = (nrf_qspi_frequency_t)MBED_HAL_QSPI_HZ_TO_CONFIG(hz);
config.phy_if.sck_delay = 0x05;
config.phy_if.sck_freq = nrf_frequency(hz);
config.phy_if.sck_delay = SCK_DELAY;
config.phy_if.dpmen = false;
config.phy_if.spi_mode = mode == 0 ? NRF_QSPI_MODE_0 : NRF_QSPI_MODE_1;

Expand All @@ -232,8 +236,8 @@ qspi_status_t qspi_free(qspi_t *obj)

qspi_status_t qspi_frequency(qspi_t *obj, int hz)
{
config.phy_if.sck_freq = (nrf_qspi_frequency_t)MBED_HAL_QSPI_HZ_TO_CONFIG(hz);
config.phy_if.sck_freq = nrf_frequency(hz);

// use sync version, no handler
ret_code_t ret = _qspi_drv_init();
if (ret == NRF_SUCCESS ) {
Expand All @@ -247,6 +251,11 @@ qspi_status_t qspi_frequency(qspi_t *obj, int hz)

qspi_status_t qspi_write(qspi_t *obj, const qspi_command_t *command, const void *data, size_t *length)
{
// length needs to be rounded up to the next WORD (4 bytes)
if ((*length & WORD_MASK) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the transfer size need to be 4 byte aligned? I don't see this as a requirement in the documentation qspi_api.h.

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out this is a limitation of the NRF52. Thanks for updating the documentation @j3hill

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is a Nordic requirement. In the nRF52840 product specification document (https://infocenter.nordicsemi.com/pdf/nRF52840_OPS_v0.5.pdf ) it states that for qspi read and write operations: "The length must be a multiple of 4 bytes." To help make this clear going forward, I've updated the length comments in "qspi_api.h" to note that support for non 4-byte (WORD) aligned reads/writes is implementation defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

@j3hill - I do not understand the comment: "support for non 4-byte (WORD) aligned reads/writes is implementation defined". 4-bytes alignment is Nordic limitation. Other targets might have other limitations.
Currently qspi_hal has no limitation on read/write granularity - so minimal size is 1byte.
The way I understand it the remark should be something like: "support for any specific target alignment requirement is - target implementation defined"

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont not understand neither rereading this thread. Similar limitations we have in flashing. How do we cover it there? via get_program_page_size() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@donatieng , @0xc0170 - if you're referring to FalshIAP, it is covered by a hal API flash_get_page_size() which is implemented in each target, and surface its requirement to the driver FlashIAP. The FlashIAP driver then handles the program address alignment and granularity requirements with its own internal buffer.

Driver:

_uint32_t FlashIAP::get_page_size() const
{
return flash_get_page_size(&flash);
}

HAL:

_/** Get page size
*

  • The page size defines the writable page size
  • @param obj The flash object
  • @return The size of a page
    */
    uint32_t flash_get_page_size(const flash_t *obj);_

Copy link
Contributor

Choose a reason for hiding this comment

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

@offirko you are referring to the internal flash. The external QSPI flash blockdevice is defined here: https://github.com/ARMmbed/mbed-os/blob/master/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp#L468-L472

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcuschangarm - I know.

I was simply showing @0xc0170 and @donatieng that in FlashIAP the driver handles alignment and write-granularity using its own internal buffer, and based on page_size upstreamed from hal .
My original suggestion was to do the same for QSPI

return QSPI_STATUS_INVALID_PARAMETER;
}

qspi_status_t status = qspi_prepare_command(obj, command, true);
if (status != QSPI_STATUS_OK) {
return status;
Expand All @@ -263,6 +272,11 @@ qspi_status_t qspi_write(qspi_t *obj, const qspi_command_t *command, const void

qspi_status_t qspi_read(qspi_t *obj, const qspi_command_t *command, void *data, size_t *length)
{
// length needs to be rounded up to the next WORD (4 bytes)
if ((*length & WORD_MASK) > 0) {
return QSPI_STATUS_INVALID_PARAMETER;
}

qspi_status_t status = qspi_prepare_command(obj, command, false);
if (status != QSPI_STATUS_OK) {
return status;
Expand Down Expand Up @@ -344,6 +358,48 @@ static ret_code_t _qspi_drv_init(void)
return ret;
}

// Private helper to set NRF frequency divider
nrf_qspi_frequency_t nrf_frequency(int hz)
{
nrf_qspi_frequency_t freq = NRF_QSPI_FREQ_32MDIV16;

// Convert hz to closest NRF frequency divider
if (hz < 2130000)
freq = NRF_QSPI_FREQ_32MDIV16; // 2.0 MHz, minimum supported frequency
else if (hz < 2290000)
freq = NRF_QSPI_FREQ_32MDIV15; // 2.13 MHz
else if (hz < 2460000)
freq = NRF_QSPI_FREQ_32MDIV14; // 2.29 MHz
else if (hz < 2660000)
freq = NRF_QSPI_FREQ_32MDIV13; // 2.46 Mhz
else if (hz < 2900000)
freq = NRF_QSPI_FREQ_32MDIV12; // 2.66 MHz
else if (hz < 3200000)
freq = NRF_QSPI_FREQ_32MDIV11; // 2.9 MHz
else if (hz < 3550000)
freq = NRF_QSPI_FREQ_32MDIV10; // 3.2 MHz
else if (hz < 4000000)
freq = NRF_QSPI_FREQ_32MDIV9; // 3.55 MHz
else if (hz < 4570000)
freq = NRF_QSPI_FREQ_32MDIV8; // 4.0 MHz
else if (hz < 5330000)
freq = NRF_QSPI_FREQ_32MDIV7; // 4.57 MHz
else if (hz < 6400000)
freq = NRF_QSPI_FREQ_32MDIV6; // 5.33 MHz
else if (hz < 8000000)
freq = NRF_QSPI_FREQ_32MDIV5; // 6.4 MHz
else if (hz < 10600000)
freq = NRF_QSPI_FREQ_32MDIV4; // 8.0 MHz
else if (hz < 16000000)
freq = NRF_QSPI_FREQ_32MDIV3; // 10.6 MHz
else if (hz < 32000000)
freq = NRF_QSPI_FREQ_32MDIV2; // 16 MHz
else
freq = NRF_QSPI_FREQ_32MDIV1; // 32 MHz

return freq;
}


#endif

Expand Down