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

sample: openamp: add support for stm32h747i_disco board #38322

Closed
wants to merge 1 commit into from

Conversation

cameled
Copy link
Contributor

@cameled cameled commented Sep 6, 2021

NOTE: This PR currently are not ready, still have some problem with M4 booting.

[EDIT] how to using test this sample on openamp sample

# Build M7 and M4
$ west build -p -b stm32h747i_disco_m7 samples/subsys/ipc/openamp

# flash M7
$ west flash

# flash M4 
$ west flash --build-dir build/openamp_remote-prefix/src/openamp_remote-build

@cameled
Copy link
Contributor Author

cameled commented Sep 6, 2021

@galak @hubertmis Hi, Did you have meet booting problem when working with open-amp and rpmsg_service samples? On stm32h747i_disco board, when run openamp and rpmsg_service sample, M4 are not boot.

stm32h747i_disco have a M7(master) and M4(remote). M4 not support openocd debug, I add led blinky to it's main function, but it's not blink.

I have check the M4 image on flash and zephyr.bin file, it's same.

@cameled cameled force-pushed the rpmsg_serv_747 branch 2 times, most recently from 0e02b86 to 6b969e9 Compare September 6, 2021 04:57
@erwango erwango self-assigned this Sep 6, 2021
@erwango erwango added this to the v3.0.0 milestone Sep 6, 2021
@carlocaione
Copy link
Collaborator

@cameled just to let you know there is a discussion going on whether we want to deprecate rpmsg_service in favour of ipc_service.

@erwango
Copy link
Member

erwango commented Sep 6, 2021

@cameled Is that specific to those samples ?

@cameled
Copy link
Contributor Author

cameled commented Sep 6, 2021

@erwango Yes, I think this issue are not same as #37827
I test 8KB array on blinky sample base on this PR, M4 blink fine.

@cameled
Copy link
Contributor Author

cameled commented Sep 6, 2021

@carlescufi Thanks for you mentioning. open-amp raw interface are not friendly for end user, If the ipc_service have merged, I will try to make stm32h747i_disco support it.

@erwango erwango added the platform: STM32 ST Micro STM32 label Sep 7, 2021
@cameled
Copy link
Contributor Author

cameled commented Sep 8, 2021

@erwango I get once time M4 boot success case with this PR, no matter power-down or reset, M4 will always boot success. After I re-flash the M4 board with the same image, boot failed happened again.

Looks there has some deeper issues that I can't find out.

@cameled
Copy link
Contributor Author

cameled commented Sep 9, 2021

@erwango I disabled the CM4 boot config "BCM4" on STM32Progrmmer->option bytes->User Configuration, then all the things return to normal, no open-amp or large size array(#37827) issues exist.

@erwango
Copy link
Member

erwango commented Sep 9, 2021

@erwango I disabled the CM4 boot config "BCM4" on STM32Progrmmer->option bytes->User Configuration, then all the things return to normal, no open-amp or large size array(#37827) issues exist.

@ABOSTM, can you check this is the expected configuration?
If yes, should we document this, or even implement a check to detect unsupported configurations?

@cameled
Copy link
Contributor Author

cameled commented Sep 9, 2021

Another issue, shared memory not work fine.

If write val1 to sram4(0x38000000) on CM7,

uint32_t *sp = (uint32_t *)0x38000000;
*sp  = val1;

then read val2 from sram4(0x38000000) on CM4.

uint32_t *sp = (uint32_t *)0x38000000;
val2 = *sp;

val1 and val2 are not equal ;-)

@carlocaione
Copy link
Collaborator

Another issue, shared memory not work fine.

If write val1 to sram4(0x38000000) on CM7,

uint32_t *sp = (uint32_t *)0x38000000;
*sp  = val1;

then read val2 from sram4(0x38000000) on CM4.

uint32_t *sp = (uint32_t *)0x38000000;
val2 = *sp;

val1 and val2 are not equal ;-)

Cache?

@cameled
Copy link
Contributor Author

cameled commented Sep 9, 2021

Another issue, shared memory not work fine.
If write val1 to sram4(0x38000000) on CM7,

uint32_t *sp = (uint32_t *)0x38000000;
*sp  = val1;

then read val2 from sram4(0x38000000) on CM4.

uint32_t *sp = (uint32_t *)0x38000000;
val2 = *sp;

val1 and val2 are not equal ;-)

Cache?

Yes, looks like it's a cache issue.
If I add CONFIG_NOCACHE_MEMORY=y to M7 Kconfig, then shared memory work fine.

@cameled
Copy link
Contributor Author

cameled commented Sep 9, 2021

@erwango Finally, open-amp and rpmsg_service samples work fine.

There have two issue block this PR.

  1. M4 boot issue
    Fix by disable the CM4 boot config "BCM4" on STM32Progrmmer->option bytes->User Configuration,
    Fix by @ABOSTM soc: arm: stm32h7: rework STM32H7 dual core boot #38504
  2. M7 memory cache issue
    Fix by add CONFIG_NOCACHE_MEMORY=y to M7 Kconfig(thanks @carlocaione).

@carlocaione
Copy link
Collaborator

For (2) you want to take a look at OpenAMP/open-amp#293

@ABOSTM
Copy link
Collaborator

ABOSTM commented Sep 9, 2021

@erwango I disabled the CM4 boot config "BCM4" on STM32Progrmmer->option bytes->User Configuration, then all the things return to normal, no open-amp or large size array(#37827) issues exist.

@ABOSTM, can you check this is the expected configuration?
If yes, should we document this, or even implement a check to detect unsupported configurations?

It is not the expected behavior, it should work with BCM4 enabled (especially as it is the default configuration out of the box), and it used to work when I worked on this dualcore. So it sounds like a regression

@cameled
Copy link
Contributor Author

cameled commented Sep 14, 2021

@erwango I disabled the CM4 boot config "BCM4" on STM32Progrmmer->option bytes->User Configuration, then all the things return to normal, no open-amp or large size array(#37827) issues exist.

@ABOSTM, can you check this is the expected configuration?
If yes, should we document this, or even implement a check to detect unsupported configurations?

It is not the expected behavior, it should work with BCM4 enabled (especially as it is the default configuration out of the box), and it used to work when I worked on this dualcore. So it sounds like a regression

There have some unknown areas, why open-amp not running code(POST_KERNEL and main) can affect the CM4 booting process.

@lenghonglin
Copy link
Contributor

the pull request is not counting ? @cameled

arnopo
arnopo previously approved these changes Nov 4, 2021
Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

minor comments else LGTM

subsys/ipc/rpmsg_service/rpmsg_backend.c Outdated Show resolved Hide resolved
@erwango
Copy link
Member

erwango commented Nov 5, 2021

@cameled I've had a try on my board and unfortunately I can't get it working.
Here is what I can get on m7 side:

*** Booting Zephyr OS build v2.7.99-1108-g4eec821986ba  ***
Starting application thread!

OpenAMP[master] demo started

On M4:

*** Booting Zephyr OS build v2.7.99-1108-g4eec821986ba  ***
Starting application thread!

OpenAMP[remote] demo started

Trying to debug (m7 side), it seems to be doing right, waiting in arch_cpu_idle.
Any advice on what I can do to see what is going on ?

@cameled
Copy link
Contributor Author

cameled commented Nov 5, 2021

@cameled I've had a try on my board and unfortunately I can't get it working. Here is what I can get on m7 side:

*** Booting Zephyr OS build v2.7.99-1108-g4eec821986ba  ***
Starting application thread!

OpenAMP[master] demo started

On M4:

*** Booting Zephyr OS build v2.7.99-1108-g4eec821986ba  ***
Starting application thread!

OpenAMP[remote] demo started

Trying to debug (m7 side), it seems to be doing right, waiting in arch_cpu_idle. Any advice on what I can do to see what is going on ?

Quick fix:
add CONFIG_NOCACHE_MEMORY=y to CM7, there have a cache issue on M7 Core.

@erwango
Copy link
Member

erwango commented Nov 5, 2021

Quick fix:
add CONFIG_NOCACHE_MEMORY=y to CM7, there have a cache issue on M7 Core.

\o/ working fine now! Thanks
Is there a plan to fix this in another PR ? Otherwise, what about adding this in samples/subsys/ipc/openamp/boards/stm32h747i_disco_m7.conf
This would avoid others to face the same issue.

@cameled
Copy link
Contributor Author

cameled commented Nov 5, 2021

Quick fix:
add CONFIG_NOCACHE_MEMORY=y to CM7, there have a cache issue on M7 Core.

\o/ working fine now! Thanks Is there a plan to fix this in another PR ? Otherwise, what about adding this in samples/subsys/ipc/openamp/boards/stm32h747i_disco_m7.conf This would avoid others to face the same issue.

@carlocaione have merged the openamp cache helpers OpenAMP/open-amp#293 to Zephyr. But there have an interface missing issue report by @lenghonglin #38322 (comment).

@erwango
Copy link
Member

erwango commented Nov 5, 2021

@carlocaione have merged the openamp cache helpers OpenAMP/open-amp#293 to Zephyr. But there have an interface missing issue report by @lenghonglin #38322 (comment).

What about adding CONFIG_NOCACHE_MEMORY with a note specifying this is a temp workaround waiting the issue to be fixed ? Otherwise I'd prefer we avoid to merge current PR if not working as is.

Copy link
Collaborator

@carlocaione carlocaione left a comment

Choose a reason for hiding this comment

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

What about adding CONFIG_NOCACHE_MEMORY with a note specifying this is a temp workaround waiting the issue to be fixed ? Otherwise I'd prefer we avoid to merge current PR if not working as is.

Agreed

@arnopo
Copy link
Collaborator

arnopo commented Nov 5, 2021

What about adding CONFIG_NOCACHE_MEMORY with a note specifying this is a temp workaround waiting the issue to be fixed ? Otherwise I'd prefer we avoid to merge current PR if not working as is.

Agreed

How CONFIG_NOCACHE_MEMORY is working?
Look like an associated no cacheable section can be defined in the linker...
In this case Defining the shared memory in a specific section would make sense, not only has a temporary work around.
As the rpmsg no copy interface is not used, defining the shared memory in a non cacheable area would probably be more efficient.

@cameled
Copy link
Contributor Author

cameled commented Nov 12, 2021

How CONFIG_NOCACHE_MEMORY is working? Look like an associated no cacheable section can be defined in the linker... In this case Defining the shared memory in a specific section would make sense, not only has a temporary work around. As the rpmsg no copy interface is not used, defining the shared memory in a non cacheable area would probably be more efficient.

From ARM Cortex-M7 Reference Manual, section "4.1.3 Initializing and enabling the L1 cache", L1 cache are disabled by default.

stm32h7 enable this at soc_m7.c file with CONFIG_NOCACHE_MEMORY check.

#ifndef CONFIG_NOCACHE_MEMORY
	if (!(SCB->CCR & SCB_CCR_DC_Msk)) {
		SCB_EnableDCache();
	}
#endif /* CONFIG_NOCACHE_MEMORY */

@cameled
Copy link
Contributor Author

cameled commented Nov 12, 2021

@arnopo no cacheable section like #30403? I prefer to implement the cache control for the Cortex-M, which will be a more complete solution. Also found libmetal metal_io_region have mem_flags.

@erwango
Copy link
Member

erwango commented Nov 15, 2021

stm32h7 enable this at soc_m7.c file with CONFIG_NOCACHE_MEMORY check.

This part is indeed uncorrect and I hope #40146 should help providing a better solution.
You can use it for now, but we should keep in mind this is temporary.
Ideally, we should remove usage of NOCACHE_MEMORY which is not proper here and replace with a H7 cache deactivation specific flag.

@cameled
Copy link
Contributor Author

cameled commented Dec 22, 2021

Will update at this weekend.

@cameled
Copy link
Contributor Author

cameled commented Jan 6, 2022

Will update at this weekend.

Sorry for the delay. After check the ipc_service sample, I find there have an ipm channel number requirement.
ipm_stm32_hsem mailbox only have one channel for Tx/Rx.

@carlocaione
Copy link
Collaborator

Sorry for the delay. After check the ipc_service sample, I find there have an ipm channel number requirement.
ipm_stm32_hsem mailbox only have one channel for Tx/Rx.

That's fine. You can use just one single channel for tx/rx.

@cameled cameled changed the title ipc: rpmsg_service: add stm32h747i_disco board support sample: openamp: add support for stm32h747i_disco board Jan 10, 2022
This add openamp sample run on stm32h747i_disco board.

The CM7 on stm32h747 support memory cache. If the cache enabled on CM7,
CM4 will receive incorrect data. This just disable the memory cache
on CM7, future user may want to use more flexible way to fix it.

Signed-off-by: HaiLong Yang <cameledyang@pm.me>
@cameled
Copy link
Contributor Author

cameled commented Jan 10, 2022

Remove rpmsg_service sample support.

Not support ipc_service, it based on mbox api, stm32h747 current not support it.

This just disable the memory cache on CM7, future user may want to use more flexible way to fix it.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Apr 23, 2022
@cameled
Copy link
Contributor Author

cameled commented May 5, 2022

Close this PR.

@cameled cameled closed this May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants