drivers: flash: sam: Redesign controller to fully utilize flash page layout #64215

bjarki-trackunit · 2023-10-21T20:32:44Z

This PR redesigns the atmel sam flash controller to fully utilize the flash page layout and capabilities of each individual soc.

An example is the ATSAM4E series socs, they have a flash layout like:

|--------------------|
| 8 Kbytes           |  erase block size = 2048
|--------------------|
| 8 Kbytes           |  erase block size = 2048
|--------------------|
| 48 Kbytes          |  erase block size = 4096
|--------------------|
| 64 Kbytes          |  erase block size = 4096
|--------------------|
| ...                |

and a write block size of 8 bytes.

This PR adds a new flash-pages cell property which is used to describe this layout fully:

flash0: flash@400000 {
	compatible = "atmel,sam-flash", "soc-nv-flash";
	write-block-size = <8>;
	flash-pages = <&eefc 8 2048>, <&eefc 252 4096>;
};

Comparing this to the ATSAME70 series

|--------------------|
| 8 Kbytes           |  erase block size = 2048
|--------------------|
| 8 Kbytes           |  erase block size = 2048
|--------------------|
| 112 Kbytes         |  erase block size = 8192
|--------------------|
| 128 Kbytes         |  erase block size = 8192
|--------------------|
| ...                |

and a write block size of 16 bytes.

It is described as

flash0: flash@400000 {
	compatible = "atmel,sam-flash", "soc-nv-flash";
	write-block-size = <16>;
	flash-pages = <&eefc 8 2048>, <&eefc 252 8192>;
};

The redesigned driver is based on the flash_page_layout.c driver to avoid duplicate code. The redesigned flash driver is still a bit naive, it will be expanded with logging and error handling.

Laczen · 2023-10-22T07:13:14Z

Hi @bjarki-trackunit, in other flash drivers this is done by creating the correct page_layout array. For the atmel sam flash there would be two entries: one entry for the smaller pages and one for the larger pages.

bjarki-trackunit · 2023-10-22T10:02:37Z

Hi @bjarki-trackunit, in other flash drivers this is done by creating the correct page_layout array. For the atmel sam flash there would be two entries: one entry for the smaller pages and one for the larger pages.

That is what the redesigned driver is doing internally :)

zephyr/drivers/flash/flash_sam.c

Lines 319 to 333 in 550e9dd

    
           #define SAM_FLASH_DEVICE DT_INST(0, atmel_sam_flash) 
        
           #define SAM_FLASH_PAGES_LAYOUT(node_id, prop, idx)						\ 
        
           	{											\ 
        
           		.pages_count = DT_PHA_BY_IDX(node_id, prop, idx, pages_count),			\ 
        
           		.pages_size = DT_PHA_BY_IDX(node_id, prop, idx, pages_size),			\ 
        
           	} 
        
           #define SAM_FLASH_PAGES_LAYOUTS									\ 
        
           	DT_FOREACH_PROP_ELEM_SEP(SAM_FLASH_DEVICE, flash_pages, SAM_FLASH_PAGES_LAYOUT, (,)) 
        
           #define SAM_FLASH_CONTROLLER(inst)								\ 
        
           	struct flash_pages_layout sam_flash_pages_layouts##inst[] = {				\ 
        
           		SAM_FLASH_PAGES_LAYOUTS								\ 
        
           	};											\

Its using the new cells in the devicetree to create the proper page_layout array :)
For the ATSAM4E, the struct flash_pages_layout array expands to:

struct flash_pages_layout sam_flash_pages_layouts0[] = {
	{
		.pages_count = 8,
		.pages_size = 2048,
	},
	{
		.pages_count = 252,
		.pages_size = 4096,
	},
};

Laczen · 2023-10-22T10:12:31Z

@bjarki-trackunit, is the proposal to move the definition of flash layout from the driver to the dts ?

It would be needed to change "flash-pages" to "erase-blocks".

What is the meaning of "&eefc" ?

bjarki-trackunit · 2023-10-22T10:17:11Z

@bjarki-trackunit, is the proposal to move the definition of flash layout from the driver to the dts ?

Correct, just like write-block-size and read-block-size (which is essentially the erase block size).

It would be needed to change "flash-pages" to "erase-blocks".

I agree with erase-blocks :)

What is the meaning of "&eefc" ?

Its the node label of the flash-controller, it defines the cell properties of the flash-pages property :)

nandojve

Hi @bjarki-trackunit ,

Nice!

It will took time from my side to do a proper review and test the flash_sam.c.

nandojve · 2023-10-22T10:20:09Z

dts/bindings/flash_controller/atmel,sam-flash-controller.yaml

+
+  "#flash-page-cells":
+    type: int
+    const: 2


It will be necessary the documentation with examples. Remember that devicetree should be self explanatory and readers should not go to the datasheet to have a full understanding.

nandojve · 2023-10-22T10:23:16Z

dts/bindings/mtd/atmel,sam-flash.yaml

@@ -0,0 +1,18 @@
+description: Atmel SAM flash area
+
+compatible: "atmel,sam-flash"


provide full documentation with examples

Agreed, wanted to get some feedback first :)

See https://github.com/zephyrproject-rtos/zephyr/compare/550e9ddd81fe23b5d73f7bed237df1c3803ae1f2..1c485944b0a69e4e06bc205ab0eee554ab3a2b03

Laczen · 2024-01-20T13:21:02Z

For external flash, it should be a flash controller, with a flash area, just like internal flash :)

Could it not just be part of the device yaml?

bjarki-trackunit · 2024-01-20T13:33:07Z

For external flash, it should be a flash controller, with a flash area, just like internal flash :)

Could it not just be part of the device yaml?

Well, the device is a flash controller, right?

Laczen · 2024-01-20T14:54:31Z

@bjarki-trackunit, maybe a future addition could be to fall back to an erase-block-size based flash page layout (the pre PR layout) when erase-blocks is not specified.

de-nordic · 2024-01-22T13:32:34Z

For external flash, it should be a flash controller, with a flash area, just like internal flash :)

Could it not just be part of the device yaml?

Well, the device is a flash controller, right?

Unfortunately.

de-nordic

Some nits, but generally OK.

de-nordic · 2024-01-22T13:39:46Z

drivers/flash/flash_sam.c

+	if ((offset >= 0) && ((offset + len) <= config->area_size)) {
+		return true;


with significantly large offset and len the (offset + len) may rollover to something that is <= area_size.

Added a check to validate the addition of offset and len are safe

zephyr/drivers/flash/flash_sam.c

Lines 54 to 67 in a06dfce

static bool sam_flash_validate_offset_len(off_t offset, size_t len)

{

if (offset < 0) {

return false;

}

if ((offset + len) < len) {

return false;

}

return true;

}

de-nordic · 2024-01-22T13:47:50Z

drivers/flash/flash_sam.c

+	if (((offset % config->parameters.write_block_size) == 0) &&
+	    ((len % config->parameters.write_block_size) == 0)) {


'%' is division, so probably takes more cycles than binary operations that could be used here.

Changed to use binary operations as the value will always be aligned with an exponent of base 2

zephyr/drivers/flash/flash_sam.c

Lines 67 to 72 in a06dfce

static bool sam_flash_aligned(size_t value, size_t alignment)

{

return (value & (alignment - 1)) == 0;

}

drivers/flash/flash_sam.c

This commit updates the driver to use the flash layout pages, rewriting it to utilize the flash_page_layout.c driver to avoid duplicate code. Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>

Add log messages to help diagnose and understand flash driver behavior. Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>

Use interrupt to wait for flash controller to finish its command and become ready. Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>

bjarki-trackunit · 2024-01-23T06:10:29Z

Update

I have addressed the issues brought up by @pdgendt and @de-nordic :)

pdgendt

Thanks! Tested on sam4s.

Laczen · 2024-01-23T10:03:25Z

@bjarki-trackunit, I have some annoying remark.

The PR adds a test page_layout that erases and writes to flash. A page_layout test should focus on the routines that are provided by the page_layout api: flash_get_page_info_by_offs(), flash_get_page_info_by_idx() and flash_page_foreach. The correct functionality for each of these routines should be tested and if possible without any erase/write. Also this test should be carried out in userspace if possible (at the moment the flash_page_foreach is not supported for userspace access).

Could the test be renamed into something else so that can leave the name page_layout for the test of the page_layout api?

bjarki-trackunit · 2024-01-23T10:06:49Z

Could the test be renamed into something else so that can leave the name page_layout for the test of the page_layout api?

sure, I have no strong feeling on the name, I could name it erase_write_boundary or just erase_write_validate, or maybe you have a good name?

Laczen · 2024-01-23T10:09:35Z

Could the test be renamed into something else so that can leave the name page_layout for the test of the page_layout api?

sure, I have no strong feeling on the name, I could name it erase_write_boundary or just erase_write_validate, or maybe you have a good name?

What about erase_blocks ?

Add test suite which tests writing to and erasing all pages, of any size defined by the page layout. If the test is built with MCUBOOT, the test will jump into the application, then run write/erase/boundary tests on the bootload partition. If MCUBOOT is not selected, the test will target the slot1 partition instead, performing the same tests. Signed-off-by: Bjarki Arge Andreasen <bjarki@arge-andreasen.me>

bjarki-trackunit · 2024-01-23T10:18:42Z

renamed page_layout test to erase_blocks

Laczen · 2024-01-23T10:20:51Z

renamed page_layout test to erase_blocks

Maybe in the long run we should move all present flash tests under functional and add a separate api folder for api tests (that also verify the userspace access).

bjarki-trackunit force-pushed the rewrite_atmel_sam_flash_controller branch from ec32c8d to 550e9dd Compare October 21, 2023 23:05

bjarki-trackunit requested a review from nandojve October 21, 2023 23:06

bjarki-trackunit marked this pull request as ready for review October 22, 2023 10:05

bjarki-trackunit requested review from galak, nashif and de-nordic as code owners October 22, 2023 10:05

zephyrbot added area: Flash platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) area: Devicetree Binding PR modifies or adds a Device Tree binding labels Oct 22, 2023

zephyrbot requested review from attie-argentum, mnkp, pdgendt and stephanosio October 22, 2023 10:06

zephyrbot assigned de-nordic Oct 22, 2023

bjarki-trackunit requested review from Laczen and removed request for pdgendt, stephanosio, mnkp and attie-argentum October 22, 2023 10:06

bjarki-trackunit assigned nandojve and de-nordic and unassigned de-nordic and nandojve Oct 22, 2023

nandojve requested changes Oct 22, 2023

View reviewed changes

bjarki-trackunit force-pushed the rewrite_atmel_sam_flash_controller branch from 550e9dd to 1c48594 Compare October 22, 2023 12:13

zephyrbot requested a review from attie-argentum October 22, 2023 12:14

de-nordic previously approved these changes Jan 22, 2024

View reviewed changes

bjarki-trackunit dismissed stale reviews from de-nordic, Laczen, pdgendt, and nandojve via be18b9d January 23, 2024 05:46

bjarki-trackunit force-pushed the rewrite_atmel_sam_flash_controller branch 2 times, most recently from be18b9d to 84b6ed9 Compare January 23, 2024 05:52

bjarki-trackunit added 3 commits January 23, 2024 07:06

flash: sam: Rewrite driver to dyncamically adapt to pages

180a58c

This commit updates the driver to use the flash layout pages, rewriting it to utilize the flash_page_layout.c driver to avoid duplicate code. Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>

drivers: flash: sam: Add logging

65d127f

Add log messages to help diagnose and understand flash driver behavior. Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>

drivers: flash: sam: Use interrupt to sync

9b618c9

Use interrupt to wait for flash controller to finish its command and become ready. Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>

bjarki-trackunit force-pushed the rewrite_atmel_sam_flash_controller branch from 84b6ed9 to a06dfce Compare January 23, 2024 06:08

pdgendt previously approved these changes Jan 23, 2024

View reviewed changes

bjarki-trackunit dismissed pdgendt’s stale review via 86b8b55 January 23, 2024 10:18

bjarki-trackunit force-pushed the rewrite_atmel_sam_flash_controller branch from a06dfce to 86b8b55 Compare January 23, 2024 10:18

pdgendt approved these changes Jan 23, 2024

View reviewed changes

Laczen approved these changes Jan 23, 2024

View reviewed changes

de-nordic approved these changes Jan 23, 2024

View reviewed changes

nandojve approved these changes Jan 23, 2024

View reviewed changes

fabiobaltieri merged commit 592647c into zephyrproject-rtos:main Jan 23, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: flash: sam: Redesign controller to fully utilize flash page layout #64215

drivers: flash: sam: Redesign controller to fully utilize flash page layout #64215

bjarki-trackunit commented Oct 21, 2023 •

edited

Laczen commented Oct 22, 2023

bjarki-trackunit commented Oct 22, 2023 •

edited

Laczen commented Oct 22, 2023

bjarki-trackunit commented Oct 22, 2023 •

edited

nandojve left a comment

nandojve Oct 22, 2023

bjarki-trackunit Oct 22, 2023

nandojve Oct 22, 2023

bjarki-trackunit Oct 22, 2023

bjarki-trackunit Oct 22, 2023

Laczen commented Jan 20, 2024

bjarki-trackunit commented Jan 20, 2024

Laczen commented Jan 20, 2024

de-nordic commented Jan 22, 2024

de-nordic left a comment

de-nordic Jan 22, 2024

bjarki-trackunit Jan 23, 2024 •

edited

de-nordic Jan 22, 2024

bjarki-trackunit Jan 23, 2024 •

edited

bjarki-trackunit commented Jan 23, 2024

pdgendt left a comment

Laczen commented Jan 23, 2024

bjarki-trackunit commented Jan 23, 2024

Laczen commented Jan 23, 2024

bjarki-trackunit commented Jan 23, 2024

Laczen commented Jan 23, 2024

		@@ -0,0 +1,18 @@
		description: Atmel SAM flash area

		compatible: "atmel,sam-flash"

		if ((offset >= 0) && ((offset + len) <= config->area_size)) {
		return true;


	static bool sam_flash_validate_offset_len(off_t offset, size_t len)
	{
	if (offset < 0) {
	return false;
	}

	if ((offset + len) < len) {
	return false;
	}

	return true;
	}

		if (((offset % config->parameters.write_block_size) == 0) &&
		((len % config->parameters.write_block_size) == 0)) {


	static bool sam_flash_aligned(size_t value, size_t alignment)
	{
	return (value & (alignment - 1)) == 0;
	}

drivers: flash: sam: Redesign controller to fully utilize flash page layout #64215

drivers: flash: sam: Redesign controller to fully utilize flash page layout #64215

Conversation

bjarki-trackunit commented Oct 21, 2023 • edited

Laczen commented Oct 22, 2023

bjarki-trackunit commented Oct 22, 2023 • edited

Laczen commented Oct 22, 2023

bjarki-trackunit commented Oct 22, 2023 • edited

nandojve left a comment

Choose a reason for hiding this comment

nandojve Oct 22, 2023

Choose a reason for hiding this comment

bjarki-trackunit Oct 22, 2023

Choose a reason for hiding this comment

nandojve Oct 22, 2023

Choose a reason for hiding this comment

bjarki-trackunit Oct 22, 2023

Choose a reason for hiding this comment

bjarki-trackunit Oct 22, 2023

Choose a reason for hiding this comment

Laczen commented Jan 20, 2024

bjarki-trackunit commented Jan 20, 2024

Laczen commented Jan 20, 2024

de-nordic commented Jan 22, 2024

de-nordic left a comment

Choose a reason for hiding this comment

de-nordic Jan 22, 2024

Choose a reason for hiding this comment

bjarki-trackunit Jan 23, 2024 • edited

Choose a reason for hiding this comment

de-nordic Jan 22, 2024

Choose a reason for hiding this comment

bjarki-trackunit Jan 23, 2024 • edited

Choose a reason for hiding this comment

bjarki-trackunit commented Jan 23, 2024

Update

pdgendt left a comment

Choose a reason for hiding this comment

Laczen commented Jan 23, 2024

bjarki-trackunit commented Jan 23, 2024

Laczen commented Jan 23, 2024

bjarki-trackunit commented Jan 23, 2024

Laczen commented Jan 23, 2024

bjarki-trackunit commented Oct 21, 2023 •

edited

bjarki-trackunit commented Oct 22, 2023 •

edited

bjarki-trackunit commented Oct 22, 2023 •

edited

bjarki-trackunit Jan 23, 2024 •

edited

bjarki-trackunit Jan 23, 2024 •

edited