Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce i.MX93 support #4310

Closed
wants to merge 3 commits into from
Closed

Introduce i.MX93 support #4310

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
265e1ff
ASoC: SOF: Move sof_ipc3_get_reply() after ipc3_fw_ready()
LaurentiuM1234 Apr 21, 2023
7d779a3
ASoC: SOF: Introduce support for host-initiated SOF_IPC_FW_READY
LaurentiuM1234 Apr 21, 2023
510a8fa
ASoC: SOF: imx: Introduce platform driver for i.MX93
LaurentiuM1234 Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,11 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)

sdev->pdata = plat_data;
sdev->first_boot = true;
/* this will be set to true by platforms for which
* the SOF_IPC_FW_READY sequence needs to be initiated by
* them instead of the FW.
*/
sdev->init_fw_ready = false;
dev_set_drvdata(dev, sdev);

if (sof_core_debug)
Expand Down
13 changes: 12 additions & 1 deletion sound/soc/sof/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,13 @@ int sof_ipc_send_msg(struct snd_sof_dev *sdev, void *msg_data, size_t msg_bytes,
struct snd_sof_ipc_msg *msg;
int ret;

if (ipc->disable_ipc_tx || sdev->fw_state != SOF_FW_BOOT_COMPLETE)
/* if init_fw_ready is set to true then that means the host will
* try to send SOF_IPC_FW_READY to FW before it has been
* confirmed that the FW has booted in which case its state can
* be != SOF_FW_BOOT_COMPLETE.
*/
if (ipc->disable_ipc_tx || (sdev->fw_state != SOF_FW_BOOT_COMPLETE
&& !sdev->init_fw_ready))
return -ENODEV;

/*
Expand Down Expand Up @@ -211,6 +217,11 @@ struct snd_sof_ipc *snd_sof_ipc_init(struct snd_sof_dev *sdev)
if (ops->init && ops->init(sdev))
return NULL;

if (!ops->init_reply_data_buffer && sdev->init_fw_ready) {
dev_err(sdev->dev, "Missing init_reply_data_buffer IPC op\n");
return NULL;
}

ipc->ops = ops;

return ipc;
Expand Down
32 changes: 32 additions & 0 deletions sound/soc/sof/ipc3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,13 @@ static int ipc3_fw_ready(struct snd_sof_dev *sdev, u32 cmd)
return offset;
}

/* when the host sends SOF_IPC_FW_READY, we need to
* skip the reply structure in order to get to the
* sof_ipc_fw_ready data.
*/
if (sdev->init_fw_ready)
offset += sizeof(struct sof_ipc_reply);

dev_dbg(sdev->dev, "DSP is ready 0x%8.8x offset 0x%x\n", cmd, offset);

/* no need to re-check version/ABI for subsequent boots */
Expand Down Expand Up @@ -774,13 +781,18 @@ static int ipc3_fw_ready(struct snd_sof_dev *sdev, u32 cmd)

ipc3_get_windows(sdev);

/* reply buffer is already initialized at this point */
if (sdev->init_fw_ready)
return 0;

return ipc3_init_reply_data_buffer(sdev);
}

static int sof_ipc3_get_reply(struct snd_sof_dev *sdev)
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

{
struct snd_sof_ipc_msg *msg = sdev->msg;
struct sof_ipc_reply *reply;
u32 cmd;
int ret = 0;

/* get the generic reply */
Expand All @@ -802,6 +814,25 @@ static int sof_ipc3_get_reply(struct snd_sof_dev *sdev)
return -EINVAL;
}

if (!ret && sdev->init_fw_ready &&
sdev->fw_state == SOF_FW_BOOT_IN_PROGRESS) {
cmd = reply->hdr.cmd & SOF_GLB_TYPE_MASK;

/* check if host received a reply for the
* SOF_IPC_FW_READY message it has sent to
* the FW.
*/
if (cmd == SOF_IPC_FW_READY) {
ret = ipc3_fw_ready(sdev, cmd);
if (ret < 0)
sof_set_fw_state(sdev, SOF_FW_BOOT_READY_FAILED);
else
sof_set_fw_state(sdev, SOF_FW_BOOT_READY_OK);

return 0;
}
}

if (msg->reply_size > 0) {
if (reply->hdr.size == msg->reply_size) {
ret = 0;
Expand Down Expand Up @@ -1108,4 +1139,5 @@ const struct sof_ipc_ops ipc3_ops = {
.rx_msg = sof_ipc3_rx_msg,
.set_get_data = sof_ipc3_set_get_data,
.get_reply = sof_ipc3_get_reply,
.init_reply_data_buffer = ipc3_init_reply_data_buffer,
};
51 changes: 37 additions & 14 deletions sound/soc/sof/loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be moved inside of the if() case

Copy link
Author

@LaurentiuM1234 LaurentiuM1234 May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@ujfalusi ujfalusi May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)?


init_waitqueue_head(&sdev->boot_wait);

Expand Down Expand Up @@ -145,20 +147,41 @@ int snd_sof_run_firmware(struct snd_sof_dev *sdev)
return ret;
}

/*
* now wait for the DSP to boot. There are 3 possible outcomes:
* 1. Boot wait times out indicating FW boot failure.
* 2. FW boots successfully and fw_ready op succeeds.
* 3. FW boots but fw_ready op fails.
*/
ret = wait_event_timeout(sdev->boot_wait,
sdev->fw_state > SOF_FW_BOOT_IN_PROGRESS,
msecs_to_jiffies(sdev->boot_timeout));
if (ret == 0) {
snd_sof_dsp_dbg_dump(sdev, "Firmware boot failure due to timeout",
SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_MBOX |
SOF_DBG_DUMP_TEXT | SOF_DBG_DUMP_PCI);
return -EIO;
if (!sdev->init_fw_ready) {
/*
* now wait for the DSP to boot. There are 3 possible outcomes:
* 1. Boot wait times out indicating FW boot failure.
* 2. FW boots successfully and fw_ready op succeeds.
* 3. FW boots but fw_ready op fails.
*/
ret = wait_event_timeout(sdev->boot_wait,
sdev->fw_state > SOF_FW_BOOT_IN_PROGRESS,
msecs_to_jiffies(sdev->boot_timeout));
if (ret == 0) {
snd_sof_dsp_dbg_dump(sdev, "Firmware boot failure due to timeout",
SOF_DBG_DUMP_REGS | SOF_DBG_DUMP_MBOX |
SOF_DBG_DUMP_TEXT | SOF_DBG_DUMP_PCI);
return -EIO;
}
} else {
/* initialize IPC reply buffer if need be */
if (!sdev->ipc->max_payload_size) {
ret = snd_sof_ipc_init_reply_data_buffer(sdev);
if (ret < 0)
return ret;
}

/* host needs to initiate SOF_IPC_FW_READY. The
* sof_ipc_fw_ready data that was previously signaled by
* a FW-initiated IPC will come as a reply to host's
* IPC.
*/
hdr.cmd = SOF_IPC_FW_READY;
hdr.size = sizeof(reply);

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

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

@dbaluta dbaluta Apr 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

@LaurentiuM1234 LaurentiuM1234 Apr 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

}

if (sdev->fw_state == SOF_FW_BOOT_READY_FAILED)
Expand Down
24 changes: 24 additions & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,10 @@ struct sof_ipc_pcm_ops;
* @get_reply: Function pointer for fetching the reply to
* sdev->ipc->msg.reply_data
* @rx_msg: Function pointer for handling a received message
* @init_reply_data_buffer: Optional pointer for IPC reply data
* initialization. Used for cases where
* the host needs to initialize the
* SOF_IPC_FW_READY sequence.
*
* Note: both @tx_msg and @set_get_data considered as TX functions and they are
* serialized for the duration of the instructed transfer. A large message sent
Expand All @@ -497,6 +501,17 @@ struct sof_ipc_ops {
bool set);
int (*get_reply)(struct snd_sof_dev *sdev);
void (*rx_msg)(struct snd_sof_dev *sdev);
/* this operation is required for cases where the host might
* want to send the firmware a message before SOF_IPC_FW_READY
* is received.
*
* One of these cases is on i.MX93 platform which requires the
* host to send a SOF_IPC_FW_READY message to firmware in order
* to receive the data expected from SOF_IPC_FW_READY. This time
* said data will be received as a reply so the reply_data
* buffer needs to be prepared.
*/
int (*init_reply_data_buffer)(struct snd_sof_dev *sdev);
};

/* SOF generic IPC data */
Expand Down Expand Up @@ -663,6 +678,10 @@ struct snd_sof_dev {
u16 mclk_id_quirk; /* same size as in IPC3 definitions */

void *private; /* core does not touch this */
/* set to true if the host needs to initiate
* the SOF_IPC_FW_READY sequence.
*/
bool init_fw_ready;
};

/*
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

static inline void snd_sof_ipc_process_reply(struct snd_sof_dev *sdev, u32 msg_id)
{
snd_sof_ipc_get_reply(sdev);
Expand Down