nxp-mcuxpresso / rpmsg-lite

RPMsg implementation for small MCUs
BSD 3-Clause "New" or "Revised" License
241 stars 74 forks source link

Sequencing, master or remote to invoke init first? #27

Closed infn-ke closed 1 year ago

infn-ke commented 2 years ago

In the rpmsg protocol, who should invoke init first? Shall the remote invoke rpmsg_lite_remote_init first and then the master invokes rpmsg_lite_master_init? Or is it the other way around?

Another question, how is this link_state supposed to work? Looks like the master puts link_state to 1 whereas slave puts it to 0. Can this be reliable used somehow to know when both sides are ready?

Hadatko commented 2 years ago

Isn't it a way that remote is waiting for 1 and master for 0? And compiled start value is 0? Then it doesn't matter who will init first If master it will put 0 to 1 and wait for 0. If remote it will wait for 1 and then put to 0... Just guessing, i didn't saw this part of code...

infn-ke commented 2 years ago

Do you mean that rpmsg_lite_master_init and rpmsg_lite_remote_init can be invoked in any order? This link_state is that in heap memory or shared memory? Looks like it ends up in heap, so how is it used then?

Hadatko commented 2 years ago

Do you mean that rpmsg_lite_master_init and rpmsg_lite_remote_init can be invoked in any order? This link_state is that in heap memory or shared memory? Looks like it ends up in heap, so how is it used then?

I will let owner to respond. We did some changes. I think link id is id of synchronizing mechanizm to synchronize init of master and remote. So it can be in heap or stack (same for mechanism) but there will be something in shared memory which will be used for synchronization. Again better to explain from owner @MichalPrincNXP

MichalPrincNXP commented 2 years ago

Hello @infn-ke master side is responsible for all shared memory buffers (VirtIO) initialization, so until the master init is done the remote can't send any message. To ensure that, the link_state is introduced in rpmsg_lite context data. It is set to 0 on remote side during the initialization and once interrupts are enabled on the remote side and the virtqueue kick ISR is serviced the rpmsg_lite_tx_callback() is invoked and the link_state is set to 1. (Note, the virtqueue kick from the master side is done at the end of the rpmsg_lite_master_init function, at the time all VirtIO structures in shared memory are already initialized). To avoid busy loop implementations on the remote side, waiting for the link_state==1, the rpmsg_lite_wait_for_link_up() can be used in the remote application. It's implementation depends on the used env. layer, in case of baremetal the busy loop has to be used, but in case of an RTOS env., the event sync. mechanism is used in waiting for the virtqueue kick interrupt service and the link_state up state).

zhangjk2752 commented 2 years ago

About the link_state, it seems not work well in my environment.
My enviroment : linux is the master and R52 with rpmsg lite is the remote. The mailbox we used is arm pl320. When linux side driver uses "virtqueue_nofity" function to notify the remote side the vdev init is ready, the remote rpmsg lite size just do nothing, didn't run the rpmsg_lite_tx_callback. Because virtqueue_notify send message with vqid0(svq in the linux) will use mailbox0, the mailbox0 channel in the linux is a RX channel. so virtqueue_nofity will have no action(mailbox framwork only init TX mailbox channels). linux code in virtio_rpmsg_bus.c: if (notify) virtqueue_notify(vrp->rvq); //here is rvq, not svq. code in my rproc driver to kick mailbox: `static void xp5_rproc_kick(struct rproc rproc, int vqid) { struct xp5_rproc kproc = rproc->priv; struct device *dev = rproc->dev.parent; mbox_msg_t msg = (mbox_msg_t)vqid; int ret;

printk("in xp5_rproc_kick, vqid=%d\n", vqid);

ret = mbox_send_message(kproc->mbox[vqid], (void *)msg);
if (ret < 0)
    dev_err(dev, "failed to send mailbox message, status = %d\n", ret);

}` As the code above, svq with vqid0 will use mailbox0 to send data, but in linux side , mailbox0 not work as a tx channel.

if we have any other way to set the link_state other than the tx callback, if possiable to use the the vdev's status in the resource table just like openamp did?

zhangjk2752 commented 2 years ago

linux side virtio device ready call: `static inline void virtio_device_ready(struct virtio_device *dev) { unsigned status = dev->config->get_status(dev);

if (dev->config->enable_cbs)
              dev->config->enable_cbs(dev);

BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);

}`

zhangjk2752 commented 2 years ago

linux side virtio device ready call: `static inline void virtio_device_ready(struct virtio_device *dev) { unsigned status = dev->config->get_status(dev);

if (dev->config->enable_cbs)
              dev->config->enable_cbs(dev);

BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);

}`

modified my code to suit the link_state setting flow: static void xp5_rproc_kick(struct rproc rproc, int vqid) { struct xp5_rproc kproc = rproc->priv; struct device *dev = rproc->dev.parent; mbox_msg_t msg = (mbox_msg_t)vqid; int ret;

printk("in xp5_rproc_kick, vqid=%d\n", vqid);

if (vqid % 2 == 0) {
    vqid += 1;
}

ret = mbox_send_message(kproc->mbox[vqid], (void *)msg);
if (ret < 0)
    dev_err(dev, "failed to send mailbox message, status = %d\n", ret);

}

the tx callback can be triggered . So fixed.

MichalPrincNXP commented 2 years ago

Hi @zhangjk2752 , thanks for sharing your approach / Linux adoption on link_state mechanism that is specific to rpmsg_lite component.