Atmel SAM: Introduce SMC driver #42739

pdgendt · 2022-02-11T14:17:50Z

Add support for the ATSAM4SA16C variant and add a driver for the Atmel SMC/EBI module.

Tested on a custom board

nandojve · 2022-02-13T20:15:13Z

Thank you for this contribution. It seems a very clean implementation. I hope can review soon when merge window is open again.
I'm missing few things:

It is required a board in tree be configured with this driver to allow CI tests. Is it possible enable IS66WV51216DALL support at SAM4S, or even SAM4E? Please, let us know at least what is your use case.
It is necessary create a test. It doesn't need to be so elaborated but without it won't be possible build the driver at CI with a valid config. My suggestion is add as a sample at samples/drivers/smc_<use case> where use case can be sram, sdram etc.

It will be necessary add an entry as smc at board yaml file to enable twister tests. Be aware that smc will be the filter entry in the test case.

zephyr/boards/arm/sam4s_xplained/sam4s_xplained.yaml

Lines 9 to 16 in 655b0d2

    
           supported: 
        
             - gpio 
        
             - spi 
        
             - watchdog 
        
             - xplained_gpio 
        
             - xplained_i2c 
        
             - xplained_serial 
        
             - xplained_spi

I think SAM4S and SAM4E have same SMC IP, could you confirm? If yes, could you add SMC node for SAM4E too.
How users will mapped memory?

codecov-commenter · 2022-02-14T01:13:55Z

Codecov Report

Merging #42739 (655b0d2) into main (32c4bd3) will increase coverage by 0.30%.
The diff coverage is 36.84%.

❗ Current head 655b0d2 differs from pull request most recent head c1e1d8d. Consider uploading reports for the commit c1e1d8d to get more accurate results

@@            Coverage Diff             @@
##             main   #42739      +/-   ##
==========================================
+ Coverage   50.10%   50.41%   +0.30%     
==========================================
  Files         632      632              
  Lines       77529    77530       +1     
  Branches    18102    18101       -1     
==========================================
+ Hits        38844    39084     +240     
+ Misses      32257    31981     -276     
- Partials     6428     6465      +37

Impacted Files	Coverage Δ
drivers/ethernet/eth_native_posix.c	`35.13% <ø> (ø)`
include/drivers/ptp_clock.h	`100.00% <ø> (ø)`
include/net/buf.h	`98.75% <ø> (ø)`
subsys/bluetooth/host/gatt.c	`8.65% <0.00%> (-0.01%)`	⬇️
subsys/net/ip/tcp.c	`52.98% <33.33%> (-0.12%)`	⬇️
lib/os/fdtable.c	`60.97% <100.00%> (+2.71%)`	⬆️
lib/posix/pthread_cond.c	`75.00% <100.00%> (-5.00%)`	⬇️
lib/posix/pthread_mutex.c	`64.00% <0.00%> (-13.34%)`	⬇️
subsys/fs/littlefs_fs.c	`70.47% <0.00%> (+0.49%)`	⬆️
subsys/settings/src/settings.c	`66.08% <0.00%> (+1.73%)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a17eeb2...c1e1d8d. Read the comment docs.

pdgendt · 2022-02-14T09:22:10Z

Hi @nandojve, thank you for the response. I've update the PR, please check my responses below.

It is required a board in tree be configured with this driver to allow CI tests. Is it possible enable IS66WV51216DALL support at SAM4S, or even SAM4E? Please, let us know at least what is your use case.

I've added the IS66WV51216DALL to the sam4s_xplained board, however I don't have access to this HW, can you/someone test the changes made? I've taken IDs/addresses/timings from datasheets and example projects.

It is necessary create a test. It doesn't need to be so elaborated but without it won't be possible build the driver at CI with a valid config. My suggestion is add as a sample at samples/drivers/smc_<use case> where use case can be sram, sdram etc.

Is this still required if it is added to an existing board where other tests will probably already cover this?

It will be necessary add an entry as smc at board yaml file to enable twister tests. Be aware that smc will be the filter entry in the test case.

zephyr/boards/arm/sam4s_xplained/sam4s_xplained.yaml

Lines 9 to 16 in 655b0d2

supported:

- gpio

- spi

- watchdog

- xplained_gpio

- xplained_i2c

- xplained_serial

- xplained_spi

Added.

I think SAM4S and SAM4E have same SMC IP, could you confirm? If yes, could you add SMC node for SAM4E too.

Added based on the SAM4E datasheet, unable to test due to lack of HW.

How users will mapped memory?

I've updated the yaml file on how to add the memory to the dts.

pdgendt · 2022-02-14T09:30:28Z

Rebased on main

nandojve

Hi @pdgendt ,

It will be necessary add a test at tests/drivers/memc. My suggestion is do something simple like write a pattern, read back and compare. This could be a generic test that may be used for any future memc_sram driver inclusive.

boards/arm/sam4s_xplained/sam4s_xplained.yaml

boards/arm/sam4s_xplained/sam4s_xplained_defconfig

drivers/memc/CMakeLists.txt

drivers/memc/memc_sam_smc.c

pdgendt · 2022-02-18T09:03:48Z

It will be necessary add a test at tests/drivers/memc. My suggestion is do something simple like write a pattern, read back and compare. This could be a generic test that may be used for any future memc_sram driver inclusive.

Hey @nandojve,

I was looking into this, and there's a test for stm32_sdram which could be reused. However this requires some more effort for the linker sections to be more generic.

Looking at #42008 and #40422 we could rework that existing test once those PRs are merged.

nandojve

I was looking into this, and there's a test for stm32_sdram which could be reused. However this requires some more effort for the linker sections to be more generic.

If it is possible, better! Project prefers reuse instead add new ones.

Looking at #42008 and #40422 we could rework that existing test once those PRs are merged.

Your PR will be conditioned to be merged after those.

I was looking this controller and my initial thought was that SMC should handle SDRAM too. After looking datasheets I noted that SAM uses a dedicated controller for SDRAM. So, in this case, we have two distinct drivers and those are independently.

On that research I noted that SAMS7/E7/V7 uses same SMC ip. It is up to you add that nodes like SAM4E. There is a minimal change in the driver that could be added which will allow SAMS7/E7/V7 have 16 bit access instead only 8. If you don't mind add this, you can upstream the whole feature set. This is a plus not a block from my side.

boards/arm/sam4s_xplained/sam4s_xplained.yaml

dts/bindings/memory-controllers/atmel,sam-smc.yaml

pdgendt · 2022-02-21T09:57:23Z

Looking at #42008 and #40422 we could rework that existing test once those PRs are merged.

Your PR will be conditioned to be merged after those.

So we wait a bit, or?

I was looking this controller and my initial thought was that SMC should handle SDRAM too. After looking datasheets I noted that SAM uses a dedicated controller for SDRAM. So, in this case, we have two distinct drivers and those are independently.

Do you mean on the SMC module as well?

On that research I noted that SAMS7/E7/V7 uses same SMC ip. It is up to you add that nodes like SAM4E. There is a minimal change in the driver that could be added which will allow SAMS7/E7/V7 have 16 bit access instead only 8. If you don't mind add this, you can upstream the whole feature set. This is a plus not a block from my side.

I currently have limited time to add these additional SoCs, and no hardware to test. The SAM4E looked easy, but I also have no way to test this. I guess if someone needs them, it should be easy to test and upstream.

nandojve · 2022-02-21T16:22:29Z

Your PR will be conditioned to be merged after those.

So we wait a bit, or?

Yes, to reuse.

Do you mean on the SMC module as well?

As far I understood SMC not support SDRAM. SDRAM is handled by a dedicated controller. At end, EBI is a set of one or two controllers (SMC + SDRAM).

mbolivar-nordic

Still keeping a -1 on for now because it's not clear to me if this is a correct use of default.

dts/bindings/memory-controllers/atmel,sam-smc.yaml

nandojve

Hi @pdgendt ,

I think it is better but still missing important information. I tried highlight few points to you.

dts/bindings/memory-controllers/atmel,sam-smc.yaml

nandojve

Hi @pdgendt ,

It still not clear. Please, see my suggestions and adapt as you wish.

dts/bindings/memory-controllers/atmel,sam-smc.yaml

pdgendt · 2022-08-02T06:46:47Z

Thanks @nandojve, for all your efforts to help out!

I updated the suggestions you made, but adjusted the example timings to the datasheet specifications.
A minimum setup time of 5ns and puls time of 45ns where swapped in your description?

nandojve · 2022-08-04T09:46:25Z

Hi @pdgendt ,

Im OK with changes. Lets ask to @mbolivar-nordic to check if fits.

BTW, we need fix flash stuff

nandojve · 2022-08-07T13:40:35Z

BTW, we need fix flash stuff

Fixed by #48762

Add soc support for sam4sa16c variant Signed-off-by: Pieter De Gendt <[email protected]>

Add a driver to support external memory connected to the SMC port for Atmel SAM devices. Signed-off-by: Pieter De Gendt <[email protected]>

Add pinctrl definitions for the SMC peripheral and add the module to the sam4s soc. Signed-off-by: Pieter De Gendt <[email protected]>

Add pinctrl definitions for the SMC peripheral (8) and add the module to the sam4e soc. IDs and addresses are taken from the datasheet. Signed-off-by: Pieter De Gendt <[email protected]>

This commit enables the SRAM controller is66wv51216dbll connected to the SMC/EBI bus. Half of the memory is made available (512KB) on chip select 0, the other part is (512KB) on chip select 1. The SMC timings are taken from the Microchip studio example SMC_SRAM_EXAMPLE for the sam4s_xplained board. Signed-off-by: Pieter De Gendt <[email protected]>

* Reworked the stm32 SDRAM test to be more generic * Added SRAM entries * Moved to new ztest API Signed-off-by: Pieter De Gendt <[email protected]>

nandojve · 2022-08-15T15:39:25Z

Hi @mbolivar-nordic ,

No more CI erros and @pdgendt made few changes. I was wondering if you can revisit this PR.

mbolivar-nordic

@pdgendt thanks for the big improvements. I'm glad to see the properties go to required. The default feature has its uses, but it is easy to abuse and I'm glad for your and @nandojve figuring out a better way forward.

talih0 · 2024-01-06T17:21:16Z

@pdgendt, @nandojve, I'm working on a similar driver for Infineon XMC4xxx.
A problem I've come across is that it's not possible to use zephyr_code_relocate() to move data, bss to external RAM with these type of drivers.

The driver set up happens with POST_KERNEL priority, but
bss_zeroing_relocation(); and data_copy_xip_relocation() are called very early at boot up. This driver should have the same problem.

Did you come across something like this?

pdgendt · 2024-01-22T10:30:53Z

@pdgendt, @nandojve, I'm working on a similar driver for Infineon XMC4xxx. A problem I've come across is that it's not possible to use zephyr_code_relocate() to move data, bss to external RAM with these type of drivers.

The driver set up happens with POST_KERNEL priority, but bss_zeroing_relocation(); and data_copy_xip_relocation() are called very early at boot up. This driver should have the same problem.

Did you come across something like this?

Hey @talih0, I did not need relocating to external RAM, so I can't really share my experience there. If this is something you want to be looked at, I would suggest creating a new issue. Comments on merged PRs are easily overlooked :)

talih0 · 2024-01-23T03:16:15Z

thanks @pdgendt for following up, I'll create an issue.

pdgendt requested review from galak and gmarull as code owners February 11, 2022 14:17

github-actions bot added area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Feb 11, 2022

pdgendt added the platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) label Feb 11, 2022

pdgendt force-pushed the atmel-smc branch from 3599663 to c1e1d8d Compare February 11, 2022 14:25

pdgendt requested a review from nandojve February 11, 2022 15:24

nandojve requested review from mnkp and stephanosio February 13, 2022 19:53

nandojve added this to the v3.1.0 milestone Feb 13, 2022

pdgendt force-pushed the atmel-smc branch from c1e1d8d to 909b1ff Compare February 14, 2022 09:16

github-actions bot added the area: Boards label Feb 14, 2022

pdgendt force-pushed the atmel-smc branch 2 times, most recently from 76e1960 to d3587e7 Compare February 14, 2022 09:30

pdgendt force-pushed the atmel-smc branch from d3587e7 to 051b308 Compare February 14, 2022 09:39

pdgendt requested a review from nashif as a code owner February 14, 2022 09:39

github-actions bot added the area: Documentation label Feb 14, 2022

nandojve requested changes Feb 17, 2022

View reviewed changes

pdgendt force-pushed the atmel-smc branch from 051b308 to 9a4a6b1 Compare February 18, 2022 07:58

nandojve changed the title ~~sam4s: Introduce SMC driver~~ Atmel SAM: Introduce SMC driver Feb 19, 2022

nandojve requested changes Feb 19, 2022

View reviewed changes

boards/arm/sam4s_xplained/sam4s_xplained.yaml Outdated Show resolved Hide resolved

dts/bindings/memory-controllers/atmel,sam-smc.yaml Outdated Show resolved Hide resolved

dts/bindings/memory-controllers/atmel,sam-smc.yaml Outdated Show resolved Hide resolved

pdgendt force-pushed the atmel-smc branch from 9a4a6b1 to 08d35c2 Compare February 21, 2022 09:52

pdgendt requested a review from nandojve February 21, 2022 10:13

pdgendt requested a review from nandojve July 26, 2022 08:50

mbolivar-nordic suggested changes Jul 28, 2022

View reviewed changes

dts/bindings/memory-controllers/atmel,sam-smc.yaml Outdated Show resolved Hide resolved

dts/bindings/memory-controllers/atmel,sam-smc.yaml Outdated Show resolved Hide resolved

pdgendt force-pushed the atmel-smc branch from eecbd0b to de200f2 Compare August 1, 2022 07:26

pdgendt requested review from galak and mbolivar-nordic August 1, 2022 09:44

nandojve requested changes Aug 1, 2022

View reviewed changes

pdgendt force-pushed the atmel-smc branch from de200f2 to 3d9c7cb Compare August 1, 2022 15:23

nandojve requested changes Aug 1, 2022

View reviewed changes

pdgendt force-pushed the atmel-smc branch from 3d9c7cb to 155517c Compare August 2, 2022 06:43

pdgendt force-pushed the atmel-smc branch from 155517c to 678bb82 Compare August 2, 2022 06:51

pdgendt requested a review from nandojve August 2, 2022 06:56

pdgendt force-pushed the atmel-smc branch from 678bb82 to 015e5f0 Compare August 4, 2022 06:29

pdgendt added 6 commits August 11, 2022 17:11

soc: arm: atmel_sam: sam4s: add support for sam4sa16c

f254a53

Add soc support for sam4sa16c variant Signed-off-by: Pieter De Gendt <[email protected]>

drivers: memc: Introduce Atmel SAM SMC/EBI driver

3876142

Add a driver to support external memory connected to the SMC port for Atmel SAM devices. Signed-off-by: Pieter De Gendt <[email protected]>

dts: sam4s: Add SMC peripheral

634fbd7

Add pinctrl definitions for the SMC peripheral and add the module to the sam4s soc. Signed-off-by: Pieter De Gendt <[email protected]>

dts: sam4e: Add SMC peripheral

6ea2196

Add pinctrl definitions for the SMC peripheral (8) and add the module to the sam4e soc. IDs and addresses are taken from the datasheet. Signed-off-by: Pieter De Gendt <[email protected]>

tests: drivers: memc: Rework S(D)RAM test with ztest

d398163

* Reworked the stm32 SDRAM test to be more generic * Added SRAM entries * Moved to new ztest API Signed-off-by: Pieter De Gendt <[email protected]>

pdgendt force-pushed the atmel-smc branch from 015e5f0 to d398163 Compare August 11, 2022 15:11

mbolivar-nordic approved these changes Aug 15, 2022

View reviewed changes

nandojve approved these changes Aug 16, 2022

View reviewed changes

carlescufi merged commit 1dc69c0 into zephyrproject-rtos:main Aug 16, 2022

pdgendt deleted the atmel-smc branch August 16, 2022 11:06

Atmel SAM: Introduce SMC driver #42739

Atmel SAM: Introduce SMC driver #42739

Uh oh!

Conversation

pdgendt commented Feb 11, 2022

Uh oh!

nandojve commented Feb 13, 2022

Uh oh!

codecov-commenter commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pdgendt commented Feb 14, 2022

Uh oh!

pdgendt commented Feb 14, 2022

Uh oh!

nandojve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdgendt commented Feb 18, 2022

Uh oh!

nandojve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdgendt commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nandojve commented Feb 21, 2022

Uh oh!

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nandojve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nandojve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdgendt commented Aug 2, 2022

Uh oh!

nandojve commented Aug 4, 2022

Uh oh!

nandojve commented Aug 7, 2022

Uh oh!

nandojve commented Aug 15, 2022

Uh oh!

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

talih0 commented Jan 6, 2024

Uh oh!

pdgendt commented Jan 22, 2024

Uh oh!

talih0 commented Jan 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov-commenter commented Feb 14, 2022 •

edited

Loading

pdgendt commented Feb 21, 2022 •

edited

Loading