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

drivers: flash: nrf_qspi_nor: unaligned read offset & read lenght #26772

Merged
merged 2 commits into from
Jul 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
97 changes: 76 additions & 21 deletions drivers/flash/nrf_qspi_nor.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
#define QSPI_PROP_AT(prop, idx) DT_PROP_BY_IDX(QSPI_NODE, prop, idx)
#define QSPI_PROP_LEN(prop) DT_PROP_LEN(QSPI_NODE, prop)

#define WORD_SIZE 4

LOG_MODULE_REGISTER(qspi_nor, CONFIG_FLASH_LOG_LEVEL);

static const struct flash_parameters qspi_flash_parameters = {
Expand Down Expand Up @@ -503,27 +505,86 @@ static inline int qspi_nor_read_id(struct device *dev,
return 0;
}

static inline nrfx_err_t read_non_aligned(struct device *dev, off_t addr,
void *dest, size_t size)
{
uint8_t __aligned(WORD_SIZE) buf[WORD_SIZE * 2];
uint8_t *dptr = dest;

off_t flash_prefix = (WORD_SIZE - (addr % WORD_SIZE)) % WORD_SIZE;

if (flash_prefix > size) {
flash_prefix = size;
}

off_t dest_prefix = (WORD_SIZE - (off_t)dptr % WORD_SIZE) % WORD_SIZE;
Copy link
Member

Choose a reason for hiding this comment

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

This trailing % WORD_SIZE is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not redundant; dest_prefix should take the value of [0 ... 3], not [0 ... 4]. If dest_prefix has value of 4, it should be reduced to 0.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, I missed that case.
But what about flash_prefix then, shouldn't it be calculated the same way?

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, should be. Will add.


if (dest_prefix > size) {
dest_prefix = size;
}

off_t flash_suffix = (size - flash_prefix) % WORD_SIZE;
off_t flash_middle = size - flash_prefix - flash_suffix;
off_t dest_middle = size - dest_prefix -
(size - dest_prefix) % WORD_SIZE;

if (flash_middle > dest_middle) {
flash_middle = dest_middle;
flash_suffix = size - flash_prefix - flash_middle;
}

nrfx_err_t res = NRFX_SUCCESS;

/* read from aligned flash to aligned memory */
if (flash_middle != 0) {
res = nrfx_qspi_read(dptr + dest_prefix, flash_middle,
addr + flash_prefix);
qspi_wait_for_completion(dev, res);
if (res != NRFX_SUCCESS) {
return res;
}

/* perform shift in RAM */
if (flash_prefix != dest_prefix) {
memmove(dptr + flash_prefix, dptr + dest_prefix, flash_middle);
}
}

/* read prefix */
if (flash_prefix != 0) {
res = nrfx_qspi_read(buf, WORD_SIZE, addr -
(WORD_SIZE - flash_prefix));
qspi_wait_for_completion(dev, res);
if (res != NRFX_SUCCESS) {
return res;
}
memcpy(dptr, buf + WORD_SIZE - flash_prefix, flash_prefix);
}

/* read suffix */
if (flash_suffix != 0) {
res = nrfx_qspi_read(buf, WORD_SIZE * 2,
addr + flash_prefix + flash_middle);
qspi_wait_for_completion(dev, res);
if (res != NRFX_SUCCESS) {
return res;
}
memcpy(dptr + flash_prefix + flash_middle, buf, flash_suffix);
}

return res;
}

static int qspi_nor_read(struct device *dev, off_t addr, void *dest,
size_t size)
{
void *dptr = dest;
size_t dlen = size;
uint8_t __aligned(4) buf[4];

if (!dest) {
return -EINVAL;
}

/* read size must be non-zero multiple of 4 bytes */
if ((size > 0) && (size < 4U)) {
dest = buf;
size = sizeof(buf);
} else if (((size % 4U) != 0) || (size == 0)) {
return -EINVAL;
}
/* address must be 4-byte aligned */
if ((addr % 4U) != 0) {
return -EINVAL;
/* read size must be non-zero */
if (!size) {
return 0;
}

const struct qspi_nor_config *params = dev->config_info;
Expand All @@ -539,18 +600,12 @@ static int qspi_nor_read(struct device *dev, off_t addr, void *dest,

qspi_lock(dev);

nrfx_err_t res = nrfx_qspi_read(dest, size, addr);

qspi_wait_for_completion(dev, res);
nrfx_err_t res = read_non_aligned(dev, addr, dest, size);

qspi_unlock(dev);

int rc = qspi_get_zephyr_ret_code(res);

if ((rc == 0) && (dest != dptr)) {
memcpy(dptr, dest, dlen);
}

return rc;
}

Expand Down
8 changes: 8 additions & 0 deletions tests/drivers/flash/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.13.1)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(nrf_qspi_nor)

FILE(GLOB app_sources src/*.c)
target_sources(app PRIVATE ${app_sources})
4 changes: 4 additions & 0 deletions tests/drivers/flash/boards/nrf52840_flash_qspi.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Minimal configuration for testing flash driver
# on nrf52840dk_nrf52840 board

CONFIG_NORDIC_QSPI_NOR=y
4 changes: 4 additions & 0 deletions tests/drivers/flash/boards/nrf52840_flash_soc.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Minimal configuration for testing flash driver
# on nrf52840dk_nrf52840 board

CONFIG_MPU_ALLOW_FLASH_WRITE=y
3 changes: 3 additions & 0 deletions tests/drivers/flash/prj.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
CONFIG_TEST=y
CONFIG_ZTEST=y
CONFIG_FLASH=y
136 changes: 136 additions & 0 deletions tests/drivers/flash/src/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
/*
* Copyright (c) 2020 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr.h>
#include <ztest.h>
#include <drivers/flash.h>
#include <devicetree.h>
#include <storage/flash_map.h>

#if (CONFIG_NORDIC_QSPI_NOR - 0)
#define FLASH_DEVICE DT_LABEL(DT_INST(0, nordic_qspi_nor))
#define FLASH_TEST_REGION_OFFSET 0xff000
#define TEST_AREA_MAX DT_PROP(DT_INST(0, nordic_qspi_nor), size)
#else

/* SoC emebded NVM */
#define FLASH_DEVICE DT_CHOSEN_ZEPHYR_FLASH_CONTROLLER_LABEL

#ifdef CONFIG_TRUSTED_EXECUTION_NONSECURE
#define FLASH_TEST_REGION_OFFSET FLASH_AREA_OFFSET(image_1_nonsecure)
#define TEST_AREA_MAX (FLASH_TEST_REGION_OFFSET +\
FLASH_AREA_SIZE(image_1_nonsecure))
#else
#define FLASH_TEST_REGION_OFFSET FLASH_AREA_OFFSET(image_1)
#define TEST_AREA_MAX (FLASH_TEST_REGION_OFFSET + FLASH_AREA_SIZE(image_1))
#endif

#endif

#define EXPECTED_SIZE 256
#define CANARY 0xff

static struct device *flash_dev;
static struct flash_pages_info page_info;
static uint8_t __aligned(4) expected[EXPECTED_SIZE];

static void test_setup(void)
{
int rc;

flash_dev = device_get_binding(FLASH_DEVICE);
const struct flash_parameters *qspi_flash_parameters =
flash_get_parameters(flash_dev);

/* For tests purposes use page (in nrf_qspi_nor page = 64 kB) */
flash_get_page_info_by_offs(flash_dev, FLASH_TEST_REGION_OFFSET,
&page_info);

/* Check if test region is not empty */
uint8_t buf[EXPECTED_SIZE];

rc = flash_read(flash_dev, FLASH_TEST_REGION_OFFSET,
buf, EXPECTED_SIZE);
zassert_equal(rc, 0, "Cannot read flash");

/* Fill test buffer with random data */
for (int i = 0; i < EXPECTED_SIZE; i++) {
expected[i] = i;
}

/* Check if tested region fits in flash */
zassert_true((FLASH_TEST_REGION_OFFSET + EXPECTED_SIZE) < TEST_AREA_MAX,
"Test area exceeds flash size");

/* Check if flash is cleared */
bool is_buf_clear = true;

for (off_t i = 0; i < EXPECTED_SIZE; i++) {
if (buf[i] != qspi_flash_parameters->erase_value) {
is_buf_clear = false;
break;
}
}

if (!is_buf_clear) {
/* erase page */
rc = flash_erase(flash_dev, page_info.start_offset,
Comment on lines +78 to +80
Copy link
Member

Choose a reason for hiding this comment

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

Is it really needed to always have this flash region cleared for this test?
I think you could skip this erasing and writing of the expected pattern to flash when it happens to be already written there (and this will be the case when this test is for example performed several times in a row).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that it is type of the test - erase and check if done properly. It is not necessary, but warn if something is wrong.
I only considered if move it to the separate test.

page_info.size);
zassert_equal(rc, 0, "Flash memory not properly erased");
}

}

static void test_read_unaligned_address(void)
{
int rc;
uint8_t buf[EXPECTED_SIZE];

rc = flash_write(flash_dev,
page_info.start_offset,
expected, EXPECTED_SIZE);
Comment on lines +92 to +94
Copy link
Member

Choose a reason for hiding this comment

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

Won't this be a problem if the expected buffer happens to be not aligned to the word boundary?

zassert_equal(rc, 0, "Cannot write to flash");

/* read buffer length*/
for (off_t len = 0; len < 25; len++) {
/* address offset */
for (off_t ad_o = 0; ad_o < 4; ad_o++) {
/* buffer offset; leave space for buffer guard */
for (off_t buf_o = 1; buf_o < 5; buf_o++) {
/* buffer overflow protection */
buf[buf_o - 1] = CANARY;
buf[buf_o + len] = CANARY;
memset(buf + buf_o, 0, len);
rc = flash_read(flash_dev,
page_info.start_offset + ad_o,
buf + buf_o, len);
zassert_equal(rc, 0, "Cannot read flash");
zassert_equal(memcmp(buf + buf_o,
expected + ad_o,
len),
0, "Flash read failed at len=%d, "
"ad_o=%d, buf_o=%d", len, ad_o, buf_o);
/* check buffer guards */
zassert_equal(buf[buf_o - 1], CANARY,
"Buffer underflow at len=%d, "
"ad_o=%d, buf_o=%d", len, ad_o, buf_o);
zassert_equal(buf[buf_o + len], CANARY,
"Buffer overflow at len=%d, "
"ad_o=%d, buf_o=%d", len, ad_o, buf_o);
}
}
}
}

void test_main(void)
{
ztest_test_suite(flash_driver_test,
ztest_unit_test(test_setup),
ztest_unit_test(test_read_unaligned_address)
);

ztest_run_test_suite(flash_driver_test);
}
9 changes: 9 additions & 0 deletions tests/drivers/flash/testcase.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
tests:
drivers.flash.nrf_qspi_nor:
platform_whitelist: nrf52840dk_nrf52840
tags: flash nrf52 nrf_qspi_fash
extra_args: OVERLAY_CONFIG=boards/nrf52840_flash_qspi.conf
drivers.flash.soc_flash_nrf:
platform_whitelist: nrf52840dk_nrf52840
tags: nrf52 soc_flash_nrf
extra_args: OVERLAY_CONFIG=boards/nrf52840_flash_soc.conf