Conversation
tnmysh
changed the title
d975033 to
85ce487
Compare
85ce487 to
433c534
Compare
| RSC_VDEV, VIRTIO_ID_RPMSG_, 31, RPMSG_VDEV_DFEATURES, 0, 0, 0, | ||
| RSC_VDEV, VIRTIO_ID_RPMSG_, 31, RPMSG_VDEV_DFEATURES, 0, | ||
| /* vdev config space len */ | ||
| sizeof(struct rpmsg_virtio_config), 0, |
There was a problem hiding this comment.
config_len here is sizeof(struct rpmsg_virtio_config) = 17 (with the current library struct), while .size in vdev_config below is set to 16, and the Linux v5 driver expects its own sizeof = 13. Three values for one length? Am I missing somehting?
There was a problem hiding this comment.
Hello @iuliana-prodan Thank you for the feedback.
This PR only reflects what was in v4. I haven't modified this PR as per v5. We are still discussing on the linux side, how this structure should look like, and once that is finalized, I will update this PR. IIRC, in v5 we decided that version should be u8, and remove all the reserved fields and make the structure __packed. That is why you see the difference. Please feel free to let me know if you have any other questions.
Thank you.
|
2026-07-15 System Reference call: |
433c534 to
c5ff69b
Compare
|
8/26/26: Tanmay did push some additional changes, but open amp team to wait until Linux side is done before reviewing. |
|
9/8/26: Mathieu has merged the Linux side. Ready for OpenAMP team to review. |
f3394dc to
b360bbb
Compare
|
@arnopo this PR is ready for review. |
|
|
||
| #include "platform_info_common.h" | ||
|
|
||
| #if !defined(_AMD_GENERATED_) && !defined(_AMD_GENERATED_) |
There was a problem hiding this comment.
_AMD_GENERATED_ is tested twice
| .vdev_config = { | ||
| .version = 1, | ||
| .size = RPMSG_VIRTIO_CONFIG_SIZE, | ||
| .h2r_buf_size = RPMSG_RX_BUF_SIZE, /* host to remote, i.e. tx for host */ |
There was a problem hiding this comment.
These are inverted compare to Linux: https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/include/linux/rpmsg/virtio_rpmsg.h?h=for-next#n26
There was a problem hiding this comment.
Yes, it is expected as per the parsing logic in the driver: https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/virtio_rpmsg_bus.c?h=for-next#n846
| #define RX_VRING_SIZE 32 /* number of rx vrings/rpmsg bufs */ | ||
| #define TX_VRING_SIZE 32 /* number of tx vrings/rpmsg bufs */ | ||
|
|
||
| #define SHMEM_SIZE_REQUIRED ((RPMSG_RX_BUF_SIZE * RX_VRING_SIZE) + \ |
There was a problem hiding this comment.
The check counts only rpmsg buffers, but the shared region also carries the two vrings. The comparison should be against the buffer area, not the whole region.
There was a problem hiding this comment.
I think that is the case. So, the "buffer area" = "single buf size" * "num of buf". Here the "number of buf" is defined by RX_VRING_SIZE and TX_VRING_SIZE.
There was a problem hiding this comment.
My bad! your comment still applies. I think the condition should be like:
SHMEM_SIZE_REQUIRED > (SHARED_MEM_SIZE - SHARED_BUF_OFFSET)
So, the rpmsg buffer space starts at SHARED_BUF_OFFSET.
vdev config space can be used for various rpmsg virtio configurations such as rpmsg tx and rx single buf size. Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
Convert debug logs to info logs that prints tx and rx buffer size of the rpmsg buffer. Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
b360bbb to
e7c85bd
Compare
3 participants
Corresponding Linux kernel patch series; https://lore.kernel.org/all/20251114184640.3020427-1-tanmay.shah@amd.com/
open-amp library PR: OpenAMP/open-amp#684