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

openamp: divide shram to TX shram & RX shram #375

Merged
merged 1 commit into from
Oct 14, 2022
Merged

Conversation

Donny9
Copy link
Contributor

@Donny9 Donny9 commented Apr 18, 2022

openamp: divide shram to TX shram & RX shram

Signed-off-by: Guiding Li [email protected]
Signed-off-by: Jiuzhu Dong [email protected]

Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

This needs a detailed explanation as to why the check that the other end is ready is unnecessary.

@Donny9 Donny9 force-pushed the main branch 2 times, most recently from 19e3b69 to 1e42d03 Compare April 18, 2022 08:35
@Donny9
Copy link
Contributor Author

Donny9 commented Apr 18, 2022

This needs a detailed explanation as to why the check that the other end is ready is unnecessary.

Okay.

Patch 1. In the multi core of lower power device, when one of core enters sleep, it needs to put its corresponding share memory into retention mode to save power consumption. Based on the limitations of the chip design, when the CPU to which share memory belongs goes to sleep, the share memory enters the retention mode, and other cores will not be able to access it. When the share memory divides tx shm and rx shm and the core of tx shm and rx shm are different, so that when one CPU sleeps, the other CPU can still access its own tx shm.

Patch 2. Base 1, The call of rproc_virtio_get_status will access the resource table, but the resource table only belongs to the shm of one of the CPUs, so after sleep, the other CPU cannot access it, so we don't to check the status for rpmsg_virtio_get_tx_payload_buffer every times.

@Donny9 Donny9 force-pushed the main branch 2 times, most recently from 827c32b to feca5cb Compare April 18, 2022 09:52
@xiaoxiang781216
Copy link
Collaborator

BTW, the boot already check the status flag:
https://github.com/OpenAMP/open-amp/blob/main/lib/rpmsg/rpmsg_virtio.c#L675-L678
https://github.com/OpenAMP/open-amp/blob/main/lib/rpmsg/rpmsg_virtio.c#L226-L242
so, the check in get_tx_payload is redundant.

@arnopo
Copy link
Collaborator

arnopo commented Apr 19, 2022

To help could you detail your memory mapping to understand in which memory are your shared structures?

  • The resource table
  • The vrings
  • RX buffers
    -TX buffers
  • log trace buffer ( if declared in the resource table ?

which CPU put the memory in retention: both or only one core ( in this case is it the host or the remote processor?)
Thanks in advance

@xiaoxiang781216
Copy link
Collaborator

To help could you detail your memory mapping to understand in which memory are your shared structures?

Let's name CPU A(master) and B(slave).

  • The resource table

SRAM A

  • The vrings

tx: SRAM A
rx: SRAM B

  • RX buffers

SRAM B

-TX buffers

SRAM A

  • log trace buffer ( if declared in the resource table ?

SRAM B

which CPU put the memory in retention: both or only one core ( in this case is it the host or the remote processor?) Thanks in advance

both, SRAM A and B can enter the retention state independently.

@arnopo
Copy link
Collaborator

arnopo commented Apr 20, 2022

Thanks for the details.
As it is a release period we should address this in the beginning of may,
same for the PR #376

@arnopo arnopo requested review from arnopo and edmooring April 20, 2022 07:14
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.

The PR seems to me valid. Few comments/concerns to address.

lib/include/openamp/rpmsg_virtio.h Outdated Show resolved Hide resolved
lib/include/openamp/rpmsg_virtio.h Outdated Show resolved Hide resolved
rpmsg_ns_bind_cb ns_bind_cb,
struct metal_io_region *shm_io,
struct rpmsg_virtio_shm_pool *tx_shpool,
struct rpmsg_virtio_shm_pool *rx_shpool);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The const struct rpmsg_virtio_config *config param is mising

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved
lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved
lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved
@Donny9 Donny9 changed the title openamp: don't need check status when get_tx_payload openamp: divide shram to TX shram & RX shram May 6, 2022
lib/include/openamp/rpmsg_virtio.h Outdated Show resolved Hide resolved
lib/include/openamp/rpmsg_virtio.h Outdated Show resolved Hide resolved
lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved
@Donny9 Donny9 force-pushed the main branch 7 times, most recently from d4f0fed to a0ff158 Compare August 3, 2022 05:45
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Otherwise, this looks reasonable.

lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved
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 fixes on comments and commit message else LGTM

lib/include/openamp/rpmsg_virtio.h Show resolved Hide resolved
lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved
In the multi core of lower power device, when one of core enters sleep,
it needs to put its corresponding share memory into retention mode to
save power consumption. Based on the limitations of the chip design,
when the CPU to which share memory belongs goes to sleep, the share
memory enters the retention mode, and other cores will not be able
to access it. When the share memory divides tx shm and rx shm
and the core of tx shm and rx shm are different, so that when one
CPU sleeps, the other CPU can still access its own tx shm.

Signed-off-by: Guiding Li <[email protected]>
Signed-off-by: Jiuzhu Dong <[email protected]>
@arnopo arnopo requested a review from edmooring October 10, 2022 11:35
@arnopo arnopo added this to the Release V2022.10 milestone Oct 10, 2022
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Looks good to me. I appreciate the commit message explaining why this code is needed.

lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved
@arnopo arnopo merged commit e62c824 into OpenAMP:main Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants