ipm: imx: send firmware ready reply #61709

iuliana-prodan · 2023-08-21T13:54:28Z

Send a fw ready reply message as soon as possible.
This is usually used on host side which is waiting for this message in order to establish the communication with the remote processor - see imx_dsp_rproc driver from Linux.

This can be enabled by IPM_IMX_FW_READY_REPLY config, which is by default N.

Set CONFIG_IPM_IMX_FW_READY_REPLY as Y for openamp_rsc_table sample, running on nxp_adsp_imx8m.

carlocaione

While this PR is not problematic per se my impression is that we are kinda cheating here. Wouldn't much better just to introduce a new REMOTEPROC API class / subsystem in Zephyr and use that instead?

drivers/ipm/Kconfig.imx

drivers/ipm/ipm_imx.c

iuliana-prodan · 2023-08-22T09:30:47Z

While this PR is not problematic per se my impression is that we are kinda cheating here. Wouldn't much better just to introduce a new REMOTEPROC API class / subsystem in Zephyr and use that instead?

This is not a standard - to send a fw ready reply after the remote core is up. This is just for imx use-case.

Others remoteproc Linux drivers (e.g. omap_remoteproc) are sending an echo request and this message will wait in the mailbox fifo until the remote core is up.

Others are doing nothing.

Therefore, there is no "one way fits all" for host-remote handshake communication.
In remoteproc there's no confirmation for remote core booted.

carlocaione · 2023-08-22T10:51:32Z

This is not a standard - to send a fw ready reply after the remote core is up. This is just for imx use-case.

Others remoteproc Linux drivers (e.g. omap_remoteproc) are sending an echo request and this message will wait in the mailbox fifo until the remote core is up.

Others are doing nothing.

Therefore, there is no "one way fits all" for host-remote handshake communication. In remoteproc there's no confirmation for remote core booted.

What about the notify hook here https://github.com/OpenAMP/open-amp/blob/5891cb4b2e813d1c9d10def998952b062304e43f/lib/include/openamp/remoteproc.h#L413 ?

I'm not going to NACK this PR, this is fine to me. I'm just trying to understand if there is a generic way to do this.

iuliana-prodan · 2023-08-22T11:30:10Z

This is not a standard - to send a fw ready reply after the remote core is up. This is just for imx use-case.
Others remoteproc Linux drivers (e.g. omap_remoteproc) are sending an echo request and this message will wait in the mailbox fifo until the remote core is up.
Others are doing nothing.
Therefore, there is no "one way fits all" for host-remote handshake communication. In remoteproc there's no confirmation for remote core booted.

What about the notify hook here https://github.com/OpenAMP/open-amp/blob/5891cb4b2e813d1c9d10def998952b062304e43f/lib/include/openamp/remoteproc.h#L413 ?

This might work if I would have openAMP on host side.
In my case, I have just Linux's remoteproc - https://elixir.bootlin.com/linux/latest/source/include/linux/remoteproc.h#L385, and has no notify().
That's why I said that in Linux there is no confirmation of remote core state.

I'm not going to NACK this PR, this is fine to me. I'm just trying to understand if there is a generic way to do this.

In my opinion, the generic way is to remove the limitation, from imx_dsp_rproc, of waiting for a fw_ready reply - this is not standard and is used only for an internal use case. But, there are still open discussions in upstream on how to handle this in Linux - https://patchwork.kernel.org/project/linux-remoteproc/patch/20230712224251.26482-1-iuliana.prodan@oss.nxp.com/

arnopo · 2023-08-22T11:54:36Z

Agree with @carlocaione,

The handshake should be performed at the application level. It probably relies on a mailbox, but

Today, similar mechanism is done in Linux but from the main to the remote processor. When the virtio device status has been updated. A kick (notify) is sent by Linux rpmsg virtio driver to the coprocessor. The Linux remoteproc is in charge of sending this kick through the mailbox. Associated code is available here: https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/virtio_rpmsg_bus.c#L995

In this PR the inverse is requested.
So your Linux kernel driver should probably have to block on start waiting a mailbox kick from the coprocessor before initializing the rpmsg.
The coprocessor application (https://elixir.bootlin.com/zephyr/latest/C/ident/rpmsg_mng_task) would be in charge of sending a notification when it is ready to communicate. And then wait the resource table virtio device status update.

Using a config is nice for compatibility with other hardwares

iuliana-prodan · 2023-08-22T12:47:36Z

Agree with @carlocaione,

The handshake should be performed at the application level. It probably relies on a mailbox, but

Today, similar mechanism is done in Linux but from the main to the remote processor. When the virtio device status has been updated. A kick (notify) is sent by Linux rpmsg virtio driver to the coprocessor. The Linux remoteproc is in charge of sending this kick through the mailbox. Associated code is available here: https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/virtio_rpmsg_bus.c#L995

The i.MX DSP remoteproc (imx_dsp_rproc) driver is implementing the generic kick callback - https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c and is called after starting the remote core - see https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c#L316.
But, this will return error, because the FW_READY flag was not set (https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c#L291).
This is set on rxdb callback (https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c#L496) which uses general purpose interrupts.

In this PR the inverse is requested. So your Linux kernel driver should probably have to block on start waiting a mailbox kick from the coprocessor before initializing the rpmsg.

It does, as mentioned above (https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c#L344)

The coprocessor application (https://elixir.bootlin.com/zephyr/latest/C/ident/rpmsg_mng_task) would be in charge of sending a notification when it is ready to communicate. And then wait the resource table virtio device status update.

So, @arnopo you're saying I should update the openamp_rsc_table sample?
This already sends a notify (using ipm_send), but is using TX interrupts, while in Linux, I'm waiting for GP interrupts.
So I cannot use the IPM API.

Now, in ipm_imx, I'm calling directly from NXP_HAL the MU_TriggerInterrupts() used for GP interrupts.

Using a config is nice for compatibility with other hardwares

iuliana-prodan · 2023-08-22T13:16:46Z

Agree with @carlocaione,
The handshake should be performed at the application level. It probably relies on a mailbox, but
Today, similar mechanism is done in Linux but from the main to the remote processor. When the virtio device status has been updated. A kick (notify) is sent by Linux rpmsg virtio driver to the coprocessor. The Linux remoteproc is in charge of sending this kick through the mailbox. Associated code is available here: https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/virtio_rpmsg_bus.c#L995

The i.MX DSP remoteproc (imx_dsp_rproc) driver is implementing the generic kick callback - https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c and is called after starting the remote core - see https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c#L316. But, this will return error, because the FW_READY flag was not set (https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c#L291). This is set on rxdb callback (https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c#L496) which uses general purpose interrupts.

In this PR the inverse is requested. So your Linux kernel driver should probably have to block on start waiting a mailbox kick from the coprocessor before initializing the rpmsg.

It does, as mentioned above (https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/imx_dsp_rproc.c#L344)

The coprocessor application (https://elixir.bootlin.com/zephyr/latest/C/ident/rpmsg_mng_task) would be in charge of sending a notification when it is ready to communicate. And then wait the resource table virtio device status update.

So, @arnopo you're saying I should update the openamp_rsc_table sample? This already sends a notify (using ipm_send), but is using TX interrupts, while in Linux, I'm waiting for GP interrupts. So I cannot use the IPM API.

I thought a little bit more and I can expand the IPM API with an ipm_notify().
Give me a couple of days and I'll send a new version to this.

Thanks!

PS: If I won't be able to send it by Friday, I'll do it next week since I'll be OOO :)

carlocaione · 2023-08-22T13:22:10Z

I thought a little bit more and I can expand the IPM API with an ipm_notify().

Why? Please don't.

A notify hook has nothing to do with the IPM device class. The IPM API is supposed to support an Inter-Processor communication hardware, a notify hook is something related to your transport protocol, not the hardware.

iuliana-prodan · 2023-08-22T13:29:43Z

I thought a little bit more and I can expand the IPM API with an ipm_notify().

Why? Please don't.

A notify hook has nothing to do with the IPM device class. The IPM API is supposed to support an Inter-Processor communication hardware, a notify hook is something related to your transport protocol, not the hardware.

But then how can I send (actually just set the interrupts I need) the fw reply? This is platform specific.
I add the ipm_notify and this will be implemented where needed - like, for now, only in ipm_imx.

As mention, I cannot use the ipm_send since is using TX interrupts, rather than GP interrupts that are expected in Linux.

carlocaione · 2023-08-22T13:40:12Z

But then how can I send (actually just set the interrupts I need) the fw reply? This is platform specific. I add the ipm_notify and this will be implemented where needed - like, for now, only in ipm_imx.

You realize that we cannot arbitrary extend every API device class because some random platform needs something on top right? This being platform specific is exactly the reason this hook makes no sense in the general API class.

You extend an API when the new API is going to support a wide variety of hardware and platforms and, to me, it looks like this is not the case.

The IPM class is about hardware, how do you use this hardware is not the driver concern really. You want something on top of that, an application, a subsystem, a framework, etc... that is using that driver to do something with it (notify, build packets, etc...).

arnopo · 2023-08-22T14:27:20Z

I thought a little bit more and I can expand the IPM API with an ipm_notify().

Why? Please don't.
A notify hook has nothing to do with the IPM device class. The IPM API is supposed to support an Inter-Processor communication hardware, a notify hook is something related to your transport protocol, not the hardware.

But then how can I send (actually just set the interrupts I need) the fw reply? This is platform specific. I add the ipm_notify and this will be implemented where needed - like, for now, only in ipm_imx.

As mention, I cannot use the ipm_send since is using TX interrupts, rather than GP interrupts that are expected in Linux.

What about considering the GP interrupt as a specific channel of the ipm_imx (or a new instance of the ipm)?
you could implement specific action in your ipm_imx based on the channel ID.

Then in application you could define a config that is set to the channel ID value.
if the CONFIG is set the application calls ipm_send(ipm_handle, 0, CONFIG_OPENAMP_RSC_TABLE_KICK_ID, NULL, 0); that trigs the GP interrupt.

carlocaione · 2023-08-24T14:39:28Z

drivers/ipm/Kconfig

+config IPM_SEND_FW_READY_REPLY
+	hex "Send a FW_READY reply message"
+	default 0xFFFF
+	help
+	  Send FW_READY reply to check for FW boot completion
+


right, so let's say that tomorrow I have a different platform (something custom of course) that needs a new reply type, for example an error messaging kind of thing. Should I add a new Kconfig symbol for that?

This doesn't scale and it is plain wrong.

If you want you can move this into Kconfig.imx, that would be acceptable.

I added this new version as @arnopo suggested - just for all of us to review it :)
But, I prefer the first version of this PR - adding a config just for IPM_IMX.
Since the fw_ready is waited only for IMX we should fix this limitation only for imx.

@arnopo what do you think?

If you want you can move this into Kconfig.imx, that would be acceptable.

Yes, I can add it to imx and not modify the openamp_rsc_table sample (main_remote.c) at all.

@iuliana-prodan
Regarding your patch seems that my suggestion is not my best one... :(

This solution is not cleaner that your solution.

More than that looking at Shengju comment on Linux remote proc mailing list:
"The reason why wait for a reply in IMX DSP driver is to make sure the DSP
boot successfully and the firmware is correct. ", it looks to me that you need to trig the signal whatever the sample you are running on the remote processor.For instance, the "hello word" sample should also work so trig the signal.

In this case the implementation in IPM or in the openamp_rsc_table application seems not the good solutions.

Is it a way to do it at SoC or board level?

I reverted to my previous version (with small changes on commit message and CONFIG description).

I added the config only for i.MX family.
I enable it only for the board I need (right now this is only for the openamp_rsc_table sample compiled for nxp_adsp_imx8m)

arnopo

I am still not convinced that the solution is reliable in the long term (for instance, for a Zephyr project that does not use ipm driver). However, the patch should be able to respond in the short term.

dbaluta · 2023-09-01T07:57:55Z

I am still not convinced that the solution is reliable in the long term (for instance, for a Zephyr project that does not use ipm driver). However, the patch should be able to respond in the short term.

I'm OK with this for now, as it seems there is no general agreement with Linux folks.

On the long run I think we can make use of struct resource_table https://github.com/OpenAMP/open-amp/blob/e63d07d10577b06c5b3fc8e1c20ede74779d3132/lib/include/openamp/remoteproc.h#L4

We could define a new fw_resource_type for example RSC_PLATFORM.

All application will need to fill this struct with information about expected behavior. Like, kernel should wait for firmware to boot (or not).

It looks like the Linux kernel infrastructure is already in place. What we need to figure out is how to define such structure with open-amp and zephyr.

arnopo · 2023-09-01T09:32:34Z

I am still not convinced that the solution is reliable in the long term (for instance, for a Zephyr project that does not use ipm driver). However, the patch should be able to respond in the short term.

I'm OK with this for now, as it seems there is no general agreement with Linux folks.

On the long run I think we can make use of struct resource_table https://github.com/OpenAMP/open-amp/blob/e63d07d10577b06c5b3fc8e1c20ede74779d3132/lib/include/openamp/remoteproc.h#L4

We could define a new fw_resource_type for example RSC_PLATFORM.

All application will need to fill this struct with information about expected behavior. Like, kernel should wait for firmware to boot (or not).

It looks like the Linux kernel infrastructure is already in place. What we need to figure out is how to define such structure with open-amp and zephyr.

Yes that a way to do it. I don't know if that would cover all use cases (XIP, start by U-boot or secure context, Zephyr <->Zephyr IPC, ...). This would also imply that the resource table is defined for all zephyr samples. But also, that we respect cache line to make the resource table R/W for both processors during the initialization phase.

For your information we started a work in openAMP community to integrate virtio drivers/devices for IPC and the virtio-mmio transport in place of the virtio remoteproc (so no resource table). We identify similar handshake need to ensure that the coprocessor is running before probing the Linux virtio-mmio driver.

That's why synchronization using signaling seems to me more generic. But perhaps we will need different solutions to address all hardwares and use cases.

iuliana-prodan · 2023-09-01T09:46:59Z

I am still not convinced that the solution is reliable in the long term (for instance, for a Zephyr project that does not use ipm driver). However, the patch should be able to respond in the short term.

I'm OK with this for now, as it seems there is no general agreement with Linux folks.

On the long run I think we can make use of struct resource_table https://github.com/OpenAMP/open-amp/blob/e63d07d10577b06c5b3fc8e1c20ede74779d3132/lib/include/openamp/remoteproc.h#L4

We could define a new fw_resource_type for example RSC_PLATFORM.

All application will need to fill this struct with information about expected behavior. Like, kernel should wait for firmware to boot (or not).

The resource table, both in Linux and Zephyr, uses fw_rsc_vdev struct (meaning RSC_VDEV).
I cannot just add a new one and use that instead or both - this imply a serious refactorization - in Linux and openAMP.
Maybe, you mean to extend the fw_rsc_vdev struct.

Probably, a solution will be to use the config space from fw_rsc_vdev struct,
But this is nowhere (Linux, Zephyr, OpenAMP) used - the config_len is declared 0 and that's all.

It looks like the Linux kernel infrastructure is already in place. What we need to figure out is how to define such structure with open-amp and zephyr.

Where is this? Can you please give me some links?

There is nothing in place, otherwise I would have used it.

dbaluta · 2023-09-07T12:45:32Z

drivers/ipm/ipm_imx.c

+	 * FW_READY reply.
+	 */
+	MU_Type *base = MU(config);
+


This looks like a false positive.

dleach02 · 2023-09-13T15:07:20Z

@nashif, We appear to have a false-positive on the compliance. What should we do here?

https://github.com/zephyrproject-rtos/zephyr/pull/61709/files#diff-7a9de38974d6ca2f0d251849b385035241af755de445ddee41ab93fb33cd5d25R332

Send a fw ready reply message as soon as possible. This is usually used on host side which is waiting for this message in order to establish the communication with the remote processor - see imx_dsp_rproc driver from Linux. This can be enabled by IPM_IMX_FW_READY_REPLY config, which is by default N. Set CONFIG_IPM_IMX_FW_READY_REPLY as Y for openamp_rsc_table sample, running on nxp_adsp_imx8m. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

iuliana-prodan · 2023-09-26T09:24:46Z

I've added a new version where I've fixed the (false positive) compliance check -https://github.com/zephyrproject-rtos/zephyr/pull/61709/files#diff-7a9de38974d6ca2f0d251849b385035241af755de445ddee41ab93fb33cd5d25R332

cc: @carlocaione @arnopo @nashif @dleach02

carlescufi · 2023-09-26T13:06:48Z

@arnopo could you please take another look?

arnopo

only one minor comment else LGTM

arnopo · 2023-09-26T13:53:08Z

drivers/ipm/ipm_imx.c

+	 * after starting the remote processor, the host is waiting for a
+	 * FW_READY reply.
+	 */
+	MU_Type * base = MU(config);


remove space between * and base

I add an extra space to fix a compliance issue - a false positive one - see also this comment #61709 (comment)

I didn't know what else to do in order to have this merged :)

iuliana-prodan requested a review from nashif as a code owner August 21, 2023 13:54

zephyrbot added area: IPM Inter-Processor Mailbox platform: NXP NXP area: IPC Inter-Process Communication labels Aug 21, 2023

zephyrbot requested review from arnopo, carlocaione, danieldegrasse, dbaluta, dcpleung, decsny, DerekSnell, dleach02, EmilioCBen, manuargue, mmahadevan108 and yvanderv August 21, 2023 13:55

zephyrbot assigned dleach02 Aug 21, 2023

iuliana-prodan force-pushed the fw_ready_reply branch from 2e2f021 to 9740e3c Compare August 21, 2023 15:36

carlocaione requested changes Aug 22, 2023

View reviewed changes

drivers/ipm/Kconfig.imx Outdated Show resolved Hide resolved

drivers/ipm/Kconfig.imx Outdated Show resolved Hide resolved

drivers/ipm/ipm_imx.c Outdated Show resolved Hide resolved

iuliana-prodan force-pushed the fw_ready_reply branch from 9740e3c to 696ac6c Compare August 22, 2023 10:40

zephyrbot requested a review from bperseghetti August 22, 2023 10:41

iuliana-prodan force-pushed the fw_ready_reply branch from 696ac6c to 0cfbd5f Compare August 22, 2023 11:34

iuliana-prodan force-pushed the fw_ready_reply branch from 0cfbd5f to 4c05289 Compare August 22, 2023 12:02

iuliana-prodan force-pushed the fw_ready_reply branch 4 times, most recently from b22aa93 to d565a28 Compare August 24, 2023 14:13

carlocaione requested changes Aug 24, 2023

View reviewed changes

iuliana-prodan force-pushed the fw_ready_reply branch from d565a28 to 1f9d7ab Compare August 31, 2023 16:17

carlocaione previously approved these changes Aug 31, 2023

View reviewed changes

arnopo previously approved these changes Sep 1, 2023

View reviewed changes

dbaluta previously approved these changes Sep 7, 2023

View reviewed changes

dbaluta reviewed Sep 7, 2023

View reviewed changes

drivers/ipm/ipm_imx.c

* FW_READY reply.

*/

MU_Type *base = MU(config);

Copy link

Collaborator

dbaluta Sep 7, 2023

Choose a reason for hiding this comment

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

This looks like a false positive.

iuliana-prodan dismissed stale reviews from dbaluta, arnopo, and carlocaione via 01ab719 September 26, 2023 08:11

iuliana-prodan force-pushed the fw_ready_reply branch from 1f9d7ab to 01ab719 Compare September 26, 2023 08:11

dbaluta approved these changes Sep 26, 2023

View reviewed changes

carlocaione approved these changes Sep 26, 2023

View reviewed changes

carlescufi requested a review from arnopo September 26, 2023 13:06

arnopo reviewed Sep 26, 2023

View reviewed changes

carlescufi merged commit 8148643 into zephyrproject-rtos:main Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipm: imx: send firmware ready reply #61709

ipm: imx: send firmware ready reply #61709

iuliana-prodan commented Aug 21, 2023 •

edited

Loading

carlocaione left a comment

iuliana-prodan commented Aug 22, 2023 •

edited

Loading

carlocaione commented Aug 22, 2023

iuliana-prodan commented Aug 22, 2023

arnopo commented Aug 22, 2023 •

edited

Loading

iuliana-prodan commented Aug 22, 2023

iuliana-prodan commented Aug 22, 2023

carlocaione commented Aug 22, 2023

iuliana-prodan commented Aug 22, 2023 •

edited

Loading

carlocaione commented Aug 22, 2023

arnopo commented Aug 22, 2023

carlocaione Aug 24, 2023

carlocaione Aug 24, 2023

iuliana-prodan Aug 24, 2023

iuliana-prodan Aug 24, 2023

arnopo Aug 24, 2023

iuliana-prodan Aug 31, 2023

arnopo left a comment

dbaluta commented Sep 1, 2023

arnopo commented Sep 1, 2023

iuliana-prodan commented Sep 1, 2023

dbaluta Sep 7, 2023

dleach02 commented Sep 13, 2023

iuliana-prodan commented Sep 26, 2023

carlescufi commented Sep 26, 2023

arnopo left a comment

arnopo Sep 26, 2023

iuliana-prodan Sep 26, 2023

ipm: imx: send firmware ready reply #61709

ipm: imx: send firmware ready reply #61709

Conversation

iuliana-prodan commented Aug 21, 2023 • edited Loading

carlocaione left a comment

Choose a reason for hiding this comment

iuliana-prodan commented Aug 22, 2023 • edited Loading

carlocaione commented Aug 22, 2023

iuliana-prodan commented Aug 22, 2023

arnopo commented Aug 22, 2023 • edited Loading

iuliana-prodan commented Aug 22, 2023

iuliana-prodan commented Aug 22, 2023

carlocaione commented Aug 22, 2023

iuliana-prodan commented Aug 22, 2023 • edited Loading

carlocaione commented Aug 22, 2023

arnopo commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnopo left a comment

Choose a reason for hiding this comment

dbaluta commented Sep 1, 2023

arnopo commented Sep 1, 2023

iuliana-prodan commented Sep 1, 2023

Choose a reason for hiding this comment

dleach02 commented Sep 13, 2023

iuliana-prodan commented Sep 26, 2023

carlescufi commented Sep 26, 2023

arnopo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iuliana-prodan commented Aug 21, 2023 •

edited

Loading

iuliana-prodan commented Aug 22, 2023 •

edited

Loading

arnopo commented Aug 22, 2023 •

edited

Loading

iuliana-prodan commented Aug 22, 2023 •

edited

Loading