Introduce i.MX93 support #4310

LaurentiuM1234 · 2023-04-19T13:44:30Z

This series of patches introduces support for i.MX93 platform on Linux side. For details on the architecture please see: thesofproject/sof#7192

sound/soc/sof/imx/Kconfig

sound/soc/sof/imx/imx93.c

sound/soc/sof/ipc3.c

sound/soc/sof/ipc.c

plbossart · 2023-04-20T18:04:07Z

sound/soc/sof/loader.c

+
+		ret = sof_ipc_tx_message(sdev->ipc, &hdr, hdr.size, &reply, sizeof(reply));
+		if (ret < 0)
+			return ret;


it's not clear where the fw_state is set in this block? Is the test line 187 even relevant for this case where the firmware is assumed to have always booted?

FWIW Intel had such platforms where the firmware is loaded already in ROM/flash, so this notion of skipping the download is relevant.

Intel may have the issue of skipping the download, but on this new Jailhouse implementation there's more. We're also skipping the "DSP" power on itself, and the firmware starts booting outright before SOF even gets the chance to load -- quite possibly multiple seconds, and if we're doing kernel modules can be as long as 30-60 seconds before the SOF module loads.

I believe this flow change is still relevant because of that. Unless Jailhouse provided a way to delay starting the secondary processor running SOF until the kernel module requested it, that is (there apparently is no such way)

The major problem Jailhouse API has in our case is that is controlled only from userspace via ioctls. We tried proposing them opening the API to be also called in kernel, with little success.

https://groups.google.com/g/jailhouse-dev/c/Vaft0VYnLzY/m/A2TYgLUyAwAJ

it's not clear where the fw_state is set in this block? Is the test line 187 even relevant for this case where the firmware is assumed to have always booted?

FWIW Intel had such platforms where the firmware is loaded already in ROM/flash, so this notion of skipping the download is relevant.

The fw_state is set in sof_ipc3_get_reply. IMO yes it's still relevant as there's no guarantee that the FW is up when module is inserted. A very trivial example for this would be the user inserting the snd_sof_imx93 module before doing the Jailhouse setup. Will have to double-check all my comments in case any of them leads to believing that the FW booting successfully is something guaranteed. All we can guarantee is that IF the FW boot process goes well (or the user doesn't mess something up) then the FW will be up before the platform driver.

In this setup the firmware will never going to send the FW_READY notification or it is going to be randomly sent too early, thus the host is not going to receive it?
Iow, is this an issue only on first boot of the firmware and later when you do runtime_pm suspend/resume or system suspend/resume there will be notification?

It might be simpler to handle this use case buy using different description of the setup:
bool query_fw_ready or no_fw_ready_notification flag for such platforms.
if it is set, skip waiting for the fw_ready and head directly to ""firmware boot complete" state
implement the sof_ipc_ops.post_fw_boot for ipc3 and if the flag is set then query the fw_ready.

This way most of the changes will happen in ipc3.c and probably better layered?

This purpose of this commit is to prepare for the usage of ipc3_fw_ready() inside of sof_ipc3_get_reply(). This will be needed when the host initiates the SOF_IPC_FW_READY sequence. Since ipc3_fw_ready() is static then that means sof_ipc3_get_reply() needs to be defined after it. This commit DOES NOT include any functional changes. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

The purpose of this commit is to allow platforms to initiate the SOF_IPC_FW_READY sequence. This is required for cases where the FW might be up before the platform driver (e.g: on i.MX93). In these cases there's no way for the FW to initiate the SOF_IPC_FW_READY sequence. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

This commit introduces a new platform driver for i.MX93. This platform uses Jailhouse to partition the hardware resources in order to make Zephyr+SOF run on an A55 core. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

paulstelian97 · 2023-04-21T11:57:54Z

sound/soc/sof/ipc3.c

@@ -830,6 +777,60 @@ static int ipc3_fw_ready(struct snd_sof_dev *sdev, u32 cmd)
 	return ipc3_init_reply_data_buffer(sdev);
 }

+static int sof_ipc3_get_reply(struct snd_sof_dev *sdev)


Do you genuinely need to move an entire function? Is there no way to e.g. declare the dependency just before the function?

Such as adding before sof_ipc3_get_reply a line like:

static int ipc3_fw_ready(struct snd_sof_dev *sdev, u32 cmd);

The definition can still be after this function, it doesn't matter to the compiler as long as it can see the declaration. That should also cause less code churn. Up to you if you think this is better than moving the function itself.

Both approaches solve the same problem. If the extra commit doesn't bother anyone, might as well go with the current one.

I think we should go with Paul's suggestion. Moving function around will pollute the history and make investigations harder.

iuliana-prodan

LGTM

ujfalusi · 2023-05-02T14:00:32Z

sound/soc/sof/loader.c

+
+		ret = sof_ipc_tx_message(sdev->ipc, &hdr, hdr.size, &reply, sizeof(reply));
+		if (ret < 0)
+			return ret;


In this setup the firmware will never going to send the FW_READY notification or it is going to be randomly sent too early, thus the host is not going to receive it?
Iow, is this an issue only on first boot of the firmware and later when you do runtime_pm suspend/resume or system suspend/resume there will be notification?

It might be simpler to handle this use case buy using different description of the setup:
bool query_fw_ready or no_fw_ready_notification flag for such platforms.
if it is set, skip waiting for the fw_ready and head directly to ""firmware boot complete" state
implement the sof_ipc_ops.post_fw_boot for ipc3 and if the flag is set then query the fw_ready.

This way most of the changes will happen in ipc3.c and probably better layered?

ujfalusi · 2023-05-02T14:00:55Z

sound/soc/sof/loader.c

@@ -109,6 +109,8 @@ EXPORT_SYMBOL(snd_sof_load_firmware_memcpy);
 int snd_sof_run_firmware(struct snd_sof_dev *sdev)
 {
 	int ret;
+	struct sof_ipc_cmd_hdr hdr;
+	struct sof_ipc_reply reply;


this can be moved inside of the if() case

In this setup the firmware will never going to send the FW_READY notification or it is going to be randomly sent too early, thus the host is not going to receive it?

FW_READY notification does get sent too early (platform driver is not even inserted yet) but I'm not sure if it gets lost since I haven't tested this case (which in hindsight seems like a poor choice on my part).

It might be simpler to handle this use case buy using different description of the setup:
bool query_fw_ready or no_fw_ready_notification flag for such platforms.
if it is set, skip waiting for the fw_ready and head directly to ""firmware boot complete" state
implement the sof_ipc_ops.post_fw_boot for ipc3 and if the flag is set then query the fw_ready.
This way most of the changes will happen in ipc3.c and probably better layered?

IMO init_fw_ready should be just as clear as query_fw_ready. The host basically initialises the FW_READY sequence if init_fw_ready == true. I wouldn't mind changing its name if you really think query_fw_ready adds much more clarity.

Hm, using post_fw_boot would move all the logic from loader to IPC3 but logically speaking it doesn't make much sense IMO. The FW is assumed to have booted successfully yet you still need to query its state in post_fw_boot?

EDIT: Please see thesofproject/sof#7192. The flow is explained here.

@LaurentiuM1234, thanks for the pointer to the diagrams, it is clear now.

In this setup you are not going to reboot Zephyr/SOF unless you do a full system suspend to turn off all A55s, right?
On resume the INMATE will boot earlier as well due to jailhouse.

The first and biggest issue with the current PR is that you introduce IPC3 message sending in generic, IPC neutral code (loader.c), this must be avoided.

Before making any change in architecture, I would check the following:

The firmware sends FW_READY on boot (unless you have disabled it explicitly)

The message is placed in the mailbox, irq triggered (or doorbell rung) but the ROOT CELL did not handled it

When the Linux stack boots you still have the FW_READY message in mailbox

In theory you should be able to read it out by 'faking' an interrupt + ipc rx of a FW_READY

Can you test this?
I would use the snd_sof_dsp_run() platform code via delayed work (or just a direct call?) to craft a message and pretend that you have received the FW_READY.
I would likely check if the FW has booted up (check the mailbox for the FW_READY?), but that is just me over cautious.

If this is working then you don't need changes in core at all as the platform's peculiarities will be handled in platform code.

Sorry for taking so long to reply. See answers below:

The first and biggest issue with the current PR is that you introduce IPC3 message sending in generic, IPC neutral code (loader.c), this must be avoided.

This can easily be solved by moving the IPC3 code from loader.c to imx93_run.

The firmware sends FW_READY on boot (unless you have disabled it explicitly)
The message is placed in the mailbox, irq triggered (or doorbell rung) but the ROOT CELL did not handled it
When the Linux stack boots you still have the FW_READY message in mailbox
In theory you should be able to read it out by 'faking' an interrupt + ipc rx of a FW_READY

Just did a small test in which the firmware sends FW_READY and init_fw_ready=false which resulted in a kernel panic. Seems like the IRQ came during the dsp-ipc driver probe.

In theory you should be able to read it out by 'faking' an interrupt + ipc rx of a FW_READY

What do you mean fake an interrupt? Also, wouldn't it be more natural to just have the host initiate the FW_READY sequence (of course, all the IPC3 code would me moved from loader.c to the imx93 platform driver). Also, support for this flow has been added to the firmware.

Sorry for taking so long to reply. See answers below:

I'm even more sorry for the delay...

The firmware sends FW_READY on boot (unless you have disabled it explicitly)
The message is placed in the mailbox, irq triggered (or doorbell rung) but the ROOT CELL did not handled it
When the Linux stack boots you still have the FW_READY message in mailbox
In theory you should be able to read it out by 'faking' an interrupt + ipc rx of a FW_READY

Just did a small test in which the firmware sends FW_READY and init_fw_ready=false which resulted in a kernel panic.

Kernel should never panic... How come the kernel is crashing? It should be waiting for the FW_READY and just gave up and fail the probe since it thinks that the DSP is not working/

In theory you should be able to read it out by 'faking' an interrupt + ipc rx of a FW_READY

What do you mean fake an interrupt?

In iMX platform code declare a delayed work and schedule it from the .run platform callback.

When the work is executed, check if the FW_READY message is in the mailbox, if it is there then call snd_sof_ipc_msgs_rx() like when you do if you receive a message. The core will only know that there is a message to read which is the FW_READY, so it will kick the statemachine forward.

Also, wouldn't it be more natural to just have the host initiate the FW_READY sequence (of course, all the IPC3 code would me moved from loader.c to the imx93 platform driver). Also, support for this flow has been added to the firmware.

If we are pragmatic, it is not too natural ;)
Host instructs the DSP that the DSP has been booted up and in reply the DSP tells the host that yes, that is correct.
Why complicate the core if this can be handled in platform code nicely (if it can be, that is)?

ujfalusi · 2023-05-02T14:03:02Z

sound/soc/sof/sof-priv.h

@@ -728,6 +747,11 @@ static inline int sof_ipc_tx_message_no_pm_no_reply(struct snd_sof_ipc *ipc, voi
 int sof_ipc_send_msg(struct snd_sof_dev *sdev, void *msg_data, size_t msg_bytes,
 		     size_t reply_bytes);

+static inline int snd_sof_ipc_init_reply_data_buffer(struct snd_sof_dev *sdev)
+{
+	return sdev->ipc->ops->init_reply_data_buffer(sdev);


should check for NULL of init_reply_data_buffer, it is not mandatory ops.

plbossart · 2023-08-04T16:42:17Z

@dbaluta @LaurentiuM1234 is this still relevant? It's been ~4 months, any plans to update this PR?

LaurentiuM1234 · 2023-08-08T06:48:37Z

@dbaluta @LaurentiuM1234 is this still relevant? It's been ~4 months, any plans to update this PR?

Sorry for not updating this PR in such a long time. IMO this is still relevant and needs to be taken care of. ATM I don't have time to take care of this so maybe we can close this and re-open later on?

plbossart · 2023-11-21T17:24:35Z

closing for now since it's been idle since May and there are conflicts. This can be re-opened as needed @LaurentiuM1234 @dbaluta

LaurentiuM1234 mentioned this pull request Apr 19, 2023

[FEATURE] Make SOF with Zephyr run on i.MX93's A55 core with Jailhouse thesofproject/sof#7192

Closed

LaurentiuM1234 force-pushed the imx93_support branch 2 times, most recently from 7a2c92b to 61ae6d4 Compare April 20, 2023 09:47

dbaluta reviewed Apr 20, 2023

View reviewed changes

sound/soc/sof/imx/Kconfig Show resolved Hide resolved

sound/soc/sof/imx/imx93.c Show resolved Hide resolved

plbossart reviewed Apr 20, 2023

View reviewed changes

LaurentiuM1234 force-pushed the imx93_support branch from 61ae6d4 to 510a8fa Compare April 21, 2023 11:47

LaurentiuM1234 added 2 commits April 21, 2023 14:50

ASoC: SOF: imx: Introduce platform driver for i.MX93

510a8fa

This commit introduces a new platform driver for i.MX93. This platform uses Jailhouse to partition the hardware resources in order to make Zephyr+SOF run on an A55 core. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

paulstelian97 reviewed Apr 21, 2023

View reviewed changes

LaurentiuM1234 marked this pull request as ready for review April 21, 2023 17:31

LaurentiuM1234 requested review from lyakh, ranj063, juimonen, lgirdwood, kv2019i, bardliao, ujfalusi, ajitkupandey, kuanhsuncheng and yaochunhung as code owners April 21, 2023 17:31

iuliana-prodan approved these changes May 2, 2023

View reviewed changes

ujfalusi reviewed May 2, 2023

View reviewed changes

plbossart closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce i.MX93 support #4310

Introduce i.MX93 support #4310

LaurentiuM1234 commented Apr 19, 2023

plbossart Apr 20, 2023

paulstelian97 Apr 21, 2023

dbaluta Apr 21, 2023 •

edited

Loading

LaurentiuM1234 Apr 21, 2023 •

edited

Loading

ujfalusi May 2, 2023

paulstelian97 Apr 21, 2023

LaurentiuM1234 Apr 21, 2023

dbaluta May 2, 2023

iuliana-prodan left a comment

ujfalusi May 2, 2023

ujfalusi May 2, 2023

LaurentiuM1234 May 2, 2023 •

edited

Loading

ujfalusi May 3, 2023 •

edited

Loading

LaurentiuM1234 May 15, 2023

ujfalusi May 23, 2023

ujfalusi May 2, 2023

plbossart commented Aug 4, 2023

LaurentiuM1234 commented Aug 8, 2023

plbossart commented Nov 21, 2023

Introduce i.MX93 support #4310

Introduce i.MX93 support #4310

Conversation

LaurentiuM1234 commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

LaurentiuM1234 Apr 21, 2023 • edited Loading

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

iuliana-prodan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentiuM1234 May 2, 2023 • edited Loading

Choose a reason for hiding this comment

ujfalusi May 3, 2023 • edited Loading

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

plbossart commented Aug 4, 2023

LaurentiuM1234 commented Aug 8, 2023

plbossart commented Nov 21, 2023

dbaluta Apr 21, 2023 •

edited

Loading

LaurentiuM1234 Apr 21, 2023 •

edited

Loading

LaurentiuM1234 May 2, 2023 •

edited

Loading

ujfalusi May 3, 2023 •

edited

Loading