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

m-syc · 2020-07-09T14:34:33Z

This PR add support for unaligned read from nrf_qspi flash memory.
User does not pay attention if address and read buffer are DWORD
aligned or read buffer length is multiply of DWORD size.
I packed all the functionality to the read_non_aligned() function.

The main concept is firstly to copy data from aligned address to the
aligned middle of the buffer. Therefore the content in the middle is
shifted to achieve the same index. Finally data from start is copied
to temporary, aligned buffer and "memcpy()" to the read buffer.
The same with the end.

I additionally suggested some test of the feature.

nvlsianpu

Just started looking.
Pleas squash fixing commits to them right fix-target commit.

drivers/flash/nrf_qspi_nor.c

nvlsianpu · 2020-07-10T15:34:07Z

I think test added here might be a seed of general flash driver test suite we can extend in another PR.
I suggest to move it to tests/drivers/flash/flash/ directory. Will comment on changes required in its code.

tests/drivers/flash/nrf_qspi_nor/src/main.c

anangl · 2020-07-13T09:32:17Z

drivers/flash/nrf_qspi_nor.c

+		if (res != NRFX_SUCCESS) {
+			return res;
+		}
+		memcpy(dptr, buf + flash_offset, flash_prefix);


Perhaps using buf[4 - flash_prefix] would be clearer, as you are copying an ending part of the buffer (you could also get rid of flash_offset then).
You could define some label for this 4 (WORD_SIZE?). It is used multiple times in the code and in some places it looks quite magical.

anangl · 2020-07-13T09:38:03Z

tests/drivers/flash/nrf_qspi_nor/src/main.c

+#include <ztest.h>
+#include <drivers/flash.h>
+
+#if (CONFIG_NORDIC_QSPI_NOR - 0)


This test is dedicated to the nrf_qspi_nor driver, isn't it? Is there a point of checking if this driver is enabled?

Doesn't seem to be changed.

07fd283 has a good solution for identifying the generic flash device, could be used here.

anangl · 2020-07-13T09:38:40Z

tests/drivers/flash/nrf_qspi_nor/src/main.c

+#define EXPECTED_SIZE	256
+
+struct device *flash_dev;
+int rc;


What's the point of having this as global variable?

Changed to local.

anangl · 2020-07-13T09:38:48Z

tests/drivers/flash/nrf_qspi_nor/src/main.c

+#define LENGTH		17
+#define EXPECTED_SIZE	256
+
+struct device *flash_dev;


static missing.

anangl · 2020-07-13T09:44:45Z

tests/drivers/flash/nrf_qspi_nor/prj.conf

+CONFIG_ZTEST=y
+CONFIG_NORDIC_QSPI_NOR=y
+CONFIG_FLASH=y
+CONFIG_SERIAL=y


Why do you need it here?

Only in debug purposes. Will delete it.

What was "it"? Nothing seems to have changed.

anangl · 2020-07-13T09:49:39Z

tests/drivers/flash/nrf_qspi_nor/src/main.c

+		ztest_unit_test(test_erase),
+		ztest_unit_test(test_read_after_erase),
+		ztest_unit_test(test_read_unaligned_address)


I don't think I get the idea of this test. What's the point here in erasing a sector and then checking if four bytes of it have the value of 0xff?
I think it would be better to write the pattern to flash only if it is not already written there, in test_setup(). And then in the actual test case just perform the reads.

I don't think I get the idea of this test. What's the point here in erasing a sector and then checking if four bytes of it have the value of 0xff?

It's only my suggestion for testing flash_erase function.

I think it would be better to write the pattern to flash only if it is not already written there, in test_setup(). And then in the actual test case just perform the reads.

Ok, flash_erase only if there is something.

anangl · 2020-07-13T10:00:52Z

tests/drivers/flash/nrf_qspi_nor/src/main.c

+	for (off_t ad_o = 0; ad_o < 4; ad_o++) {
+		/* buffer offset */
+		for (off_t buf_o = 0; buf_o < 4; buf_o++) {
+			rc = flash_read(flash_dev,


The buffer should be cleared before the read. To avoid taking the previous read result as the current one if for some reason the current read request is for example ignored.

drivers/flash/nrf_qspi_nor.c

tests/drivers/flash/boards/nrf52840_flash_soc.conf

drivers/flash/nrf_qspi_nor.c

de-nordic · 2020-07-15T13:52:02Z

tests/drivers/flash/src/main.c

+					page_info.start_offset + ad_o,
+					buf + buf_o, LENGTH);
+			zassert_equal(rc, 0, "Cannot read flash");
+			zassert_equal(memcmp(buf + buf_o,


There is a lot of pointer/offset operations happening, it may by worth to actually use encapsulate test buffer in bigger buffer and prefix it and postfix it with canaries to check if writes beyond the buffer does not happen.

There were read buffer overflow indeed, only in one case. Added canaries around buffer in tests.

drivers/flash/nrf_qspi_nor.c

nvlsianpu · 2020-07-17T09:08:44Z

tests/drivers/flash/src/main.c

+
+void test_main(void)
+{
+	ztest_test_suite(nrf_qspi_nor_test,


nip: change name to flash_driver_test as it is generic, shell pass for any flash device in near feature.

carlescufi · 2020-07-17T10:51:21Z

@pabigot @anangl @de-nordic can you take another look please?

nvlsianpu · 2020-07-17T10:57:39Z

@carlescufi @de-nordic @pabigot @anangl I think this is ready to be merged.

After this PR will be merged we can extend the test-suite added here, to be adapted by other SoC and add more test cases for cover flash API implementation testing well.

de-nordic · 2020-07-17T11:02:02Z

drivers/flash/nrf_qspi_nor.c

+	}
+
+	/* perform shift in RAM */
+	if (flash_prefix - dest_prefix != 0) {


The arithmetic expression should be in parentheses.

de-nordic

Thanks for testing with canaries.
I have left one style issue open, that is lack of parentheses around arithmetic expression, otherwise I think it is ok.

pabigot

Just an initial pass. There are a large number of comments where @MSyc-NS commented four days ago that a change had been made, but no change is visible in the current content. Please verify those then I'll look again.

pabigot · 2020-07-17T14:38:43Z

drivers/flash/nrf_qspi_nor.c

+			dptr[i + shift] = dptr[i];
+		}
+	}
+	else if (shift < 0) {


put '}' and 'else {' on same line.

pabigot · 2020-07-17T14:39:55Z

drivers/flash/nrf_qspi_nor.c

+	}
+
+	/* perform shift in RAM */
+	if (shift > 0) {


So why isn't this using memmove?

pabigot · 2020-07-17T14:44:12Z

tests/drivers/flash/nrf_qspi_nor/prj.conf

+CONFIG_ZTEST=y
+CONFIG_NORDIC_QSPI_NOR=y
+CONFIG_FLASH=y
+CONFIG_SERIAL=y


What was "it"? Nothing seems to have changed.

pabigot · 2020-07-17T14:44:52Z

tests/drivers/flash/nrf_qspi_nor/src/main.c

+#include <ztest.h>
+#include <drivers/flash.h>
+
+#if (CONFIG_NORDIC_QSPI_NOR - 0)


Doesn't seem to be changed.

07fd283 has a good solution for identifying the generic flash device, could be used here.

nvlsianpu · 2020-07-17T15:52:55Z

Looks like fix commits were not squashed properly. Most of @pabigot requests were full filled but need do clear commits history here.
@MSyc-NS Please squash fix commits to proper parent commits.

m-syc · 2020-07-17T17:04:31Z

@pabigot

Just an initial pass. There are a large number of comments where @MSyc-NS commented four days ago that a change had been made, but no change is visible in the current content. Please verify those then I'll look again.

I made changes, but not committed it properly. I'm sorry, it's fixed now.

pabigot

OK; my bad on the previous review: when I see more than two or three commits I look at them individually, which causes confusion when the comments aren't addressed in the same commit.

The detection of the appropriate flash device using the technique in 07fd283 can wait until this test is generalized to other devices.

anangl · 2020-07-17T11:25:07Z

drivers/flash/nrf_qspi_nor.c

+		flash_prefix = size;
+	}
+
+	off_t dest_prefix = (WORD_SIZE - (off_t)dptr % WORD_SIZE) % WORD_SIZE;


This trailing % WORD_SIZE is redundant.

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.

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

Yes, should be. Will add.

anangl · 2020-07-20T06:36:39Z

drivers/flash/nrf_qspi_nor.c

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


Why not __aligned(WORD_SIZE)?

anangl · 2020-07-20T06:50:39Z

drivers/flash/nrf_qspi_nor.c

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


This is only needed when some "middle" has been actually read, so it would be better to place it in the if block above.

anangl · 2020-07-20T06:51:37Z

drivers/flash/nrf_qspi_nor.c

+	}
+
+	/* perform shift in RAM */
+	if ((flash_prefix - dest_prefix) != 0) {


Maybe just flash_prefix != dest_prefix?

anangl · 2020-07-20T07:04:34Z

tests/drivers/flash/src/main.c

+	if (!is_buf_clear) {
+		/* erase page */
+		rc = flash_erase(flash_dev, page_info.start_offset,


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).

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.

anangl · 2020-07-20T07:08:15Z

tests/drivers/flash/src/main.c

+	rc = flash_write(flash_dev,
+			 page_info.start_offset,
+			 expected, EXPECTED_SIZE);


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

anangl · 2020-07-20T09:12:00Z

drivers/flash/nrf_qspi_nor.c

+	uint8_t __aligned(WORD_SIZE) buf[WORD_SIZE * 2];
+	uint8_t *dptr = dest;
+
+	off_t flash_prefix = WORD_SIZE - (addr % WORD_SIZE) % WORD_SIZE;


You need to use parentheses here. Otherwise, the second modulo operation will be performed before the subtraction, and consequently it will be useless.

Suggested change

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

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

Added function read_non_aligned that reads data under non-aligned flash addres to non-aligned read buffer of any size. Signed-off-by: Mateusz Syc <Mateusz.Syc@nordicsemi.no>

Test include non-aligned read in nrf_qspi_nor flash and SoC flash memory to buffer with variable size. It checks all possible variants of alignment and size. Signed-off-by: Mateusz Syc <Mateusz.Syc@nordicsemi.no>

nvlsianpu self-requested a review July 9, 2020 14:35

nvlsianpu added area: Drivers platform: nRF Nordic nRFx area: Flash area: QSPI Quad SPI labels Jul 9, 2020

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Jul 9, 2020

nvlsianpu reviewed Jul 10, 2020

View reviewed changes

drivers/flash/nrf_qspi_nor.c Outdated Show resolved Hide resolved

nvlsianpu requested review from anangl and kamlaz July 10, 2020 11:53

m-syc force-pushed the non_aligned_flash_read branch from a42069b to 7d452f9 Compare July 10, 2020 12:20

nvlsianpu reviewed Jul 10, 2020

View reviewed changes

drivers/flash/nrf_qspi_nor.c Outdated Show resolved Hide resolved

drivers/flash/nrf_qspi_nor.c Outdated Show resolved Hide resolved

nvlsianpu requested a review from carlescufi July 10, 2020 13:01

nvlsianpu reviewed Jul 10, 2020

View reviewed changes

tests/drivers/flash/nrf_qspi_nor/src/main.c Outdated Show resolved Hide resolved

tests/drivers/flash/nrf_qspi_nor/src/main.c Outdated Show resolved Hide resolved

tests/drivers/flash/nrf_qspi_nor/src/main.c Outdated Show resolved Hide resolved

anangl reviewed Jul 13, 2020

View reviewed changes

de-nordic requested changes Jul 14, 2020

View reviewed changes

drivers/flash/nrf_qspi_nor.c Outdated Show resolved Hide resolved

m-syc force-pushed the non_aligned_flash_read branch 2 times, most recently from 9a298af to e15fd3e Compare July 15, 2020 11:30

nvlsianpu reviewed Jul 15, 2020

View reviewed changes

tests/drivers/flash/boards/nrf52840_flash_soc.conf Outdated Show resolved Hide resolved

m-syc force-pushed the non_aligned_flash_read branch 2 times, most recently from be67f20 to db31578 Compare July 15, 2020 13:09

de-nordic reviewed Jul 15, 2020

View reviewed changes

drivers/flash/nrf_qspi_nor.c Outdated Show resolved Hide resolved

de-nordic reviewed Jul 15, 2020

View reviewed changes

m-syc marked this pull request as ready for review July 15, 2020 15:03

m-syc requested review from nashif and pabigot as code owners July 15, 2020 15:03

m-syc force-pushed the non_aligned_flash_read branch from db31578 to 84e643b Compare July 15, 2020 15:22

pabigot reviewed Jul 15, 2020

View reviewed changes

drivers/flash/nrf_qspi_nor.c Outdated Show resolved Hide resolved

m-syc force-pushed the non_aligned_flash_read branch from 62d1f99 to 6d2a369 Compare July 17, 2020 06:45

m-syc requested a review from anangl July 17, 2020 08:33

m-syc requested review from nvlsianpu, de-nordic and pabigot July 17, 2020 08:34

nvlsianpu approved these changes Jul 17, 2020

View reviewed changes

nvlsianpu reviewed Jul 17, 2020

View reviewed changes

nvlsianpu mentioned this pull request Jul 17, 2020

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

Merged

de-nordic reviewed Jul 17, 2020

View reviewed changes

de-nordic approved these changes Jul 17, 2020

View reviewed changes

m-syc force-pushed the non_aligned_flash_read branch from 606b2fd to 15ac353 Compare July 17, 2020 11:12

pabigot requested changes Jul 17, 2020

View reviewed changes

m-syc force-pushed the non_aligned_flash_read branch from 15ac353 to 019dabd Compare July 17, 2020 16:57

pabigot approved these changes Jul 17, 2020

View reviewed changes

anangl reviewed Jul 20, 2020

View reviewed changes

m-syc force-pushed the non_aligned_flash_read branch from 019dabd to eb33fab Compare July 20, 2020 08:52

anangl reviewed Jul 20, 2020

View reviewed changes

Mateusz Syc added 2 commits July 20, 2020 11:25

drivers: flash: nrf_qspi_nor: supports non-aligned read

862c8f9

Added function read_non_aligned that reads data under non-aligned flash addres to non-aligned read buffer of any size. Signed-off-by: Mateusz Syc <Mateusz.Syc@nordicsemi.no>

tests: drivers: flash: added flash tests

1d06546

Test include non-aligned read in nrf_qspi_nor flash and SoC flash memory to buffer with variable size. It checks all possible variants of alignment and size. Signed-off-by: Mateusz Syc <Mateusz.Syc@nordicsemi.no>

m-syc force-pushed the non_aligned_flash_read branch from eb33fab to 1d06546 Compare July 20, 2020 09:28

anangl approved these changes Jul 20, 2020

View reviewed changes

carlescufi merged commit 47bb6a3 into zephyrproject-rtos:master Jul 20, 2020

m-syc deleted the non_aligned_flash_read branch July 20, 2020 11:09

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

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

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

Conversation

m-syc commented Jul 9, 2020 • edited Loading

nvlsianpu left a comment

Choose a reason for hiding this comment

nvlsianpu commented Jul 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Jul 17, 2020

nvlsianpu commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic left a comment

Choose a reason for hiding this comment

pabigot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvlsianpu commented Jul 17, 2020

m-syc commented Jul 17, 2020 • edited Loading

pabigot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-syc commented Jul 9, 2020 •

edited

Loading

m-syc commented Jul 17, 2020 •

edited

Loading