Virtio native support for lib openamp #494
Conversation
arnopo
requested review from
edmooring,
xiaoxiang781216,
arnopo,
wmamills and
tnmysh
lib/include/openamp/virtio.h
Outdated
| * @param[in] dev Pointer to device structure. | ||
| * @param[in] status Value to be set as device status. | ||
| * | ||
| * @return N/A. |
There was a problem hiding this comment.
just remove the line as no return (same for others functions returning void)
danmilea
force-pushed
the
virtio-native
branch
2 times, most recently
from
e23eafa to
194fea5
Compare
There was a problem hiding this comment.
-
New comments on the code ( I take more time to review also the Virtio drivers)
-
Please also run ./script/checkpatch.pl on your commits. Even if no error is returned some coding rule would need to be fixed.
-
Concerning your proposal to integrate this first, then to integrate remoteproc_virtio: optimize the remoteproc virtio transport layer #489, and finally update virtio MMIO, I'm not fan as we would create temporary API. I will create an Issue to describe what I would expect interm of integration steps, probably by splitting current PRs
lib/virtio_mmio/virtio_mmio_drv.c
Outdated
|
|
||
| offset = 0; | ||
|
|
||
| if (!vqueue) { |
lib/virtio_mmio/virtio_serial_drv.c
Outdated
| return ret; | ||
| } | ||
|
|
||
| void virtio_serial_poll_out(const struct virtio_device *vdev, unsigned char out_char) |
There was a problem hiding this comment.
any reason to not return a status?
danmilea
force-pushed
the
virtio-native
branch
2 times, most recently
from
866060a to
6dc9d9e
Compare
| #define VIRTIO_ASSERT(_exp, _msg) metal_assert(_exp) | ||
| #endif /* VIRTIO_DEBUG */ | ||
|
|
||
| #define VRING_ALIGNMENT 4096 |
There was a problem hiding this comment.
This is something that should become configurable. it is not not blocking for this step, can be done later.
lib/include/openamp/virtqueue.h
Outdated
| * | ||
| * @param vq Pointer to VirtIO queue control block | ||
| * | ||
| * @return 1 if virtqueue is full |
lib/include/openamp/virtqueue.h
Outdated
| * | ||
| * @param vq Pointer to VirtIO queue control block | ||
| * | ||
| * @return 1 if virtqueue is empty |
There was a problem hiding this comment.
the result of your test is a boolean , so could be different from 1
Whould better to have something like taht:
"return a non null value if empty else 0"
There was a problem hiding this comment.
The spec states that == returns either 1 or 0.
There was a problem hiding this comment.
Thanks for the spec reference, I copy it here.
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf
6.5.9 Equality operators
Just a concerns if the spec cover 100% of builds. Regarding posts seems not the case. that why specifying 0 or not-0 seems to me more reliable
lib/virtio_mmio/virtio_mmio_drv.c
Outdated
| vring_info->info.num_descs = virtio_mmio_get_max_elem(vdev, idx); | ||
| vring_info->info.align = VRING_ALIGNMENT; | ||
|
|
||
| /* Preallocated virtqueues; vrings can be already declared via VIRTQUEUE_DECLARE */ |
There was a problem hiding this comment.
VIRTQUEUE_DECLARE or VIRTIO_MMIO_VQ_DECLARE?
Do we need to support the preallocated virtqueue in a first step? This part is not cristal clear for me, and would need more review...
I would be in favor of removing them from this PR and introduce them in a separate PR with more details on the usage...
There was a problem hiding this comment.
Sorry not enough clear... I was also speaking about the VIRTIO_MMIO_VQ_DECLARE and VIRTIO_MMIO_VQ_PTR macro. I would cremove them from this PR
danmilea
force-pushed
the
virtio-native
branch
3 times, most recently
from
105eb7e to
bbd278d
Compare
danmilea
force-pushed
the
virtio-native
branch
2 times, most recently
from
52a619f to
d3763b5
Compare
VIRTIO MMIO transport for OpenAMP. Signed-off-by: Dan Milea <[email protected]>
|
@danmilea |
|
@arnopo I created a pull request to rename the offending macro. |
This patch-set introduces "native" virtio MMIO driver support in lib open-amp. The framework currently supports entropy, console and network virtio devices.