| Message ID | 20220207122108.3780926-5-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: AVS - Audio DSP for cAVS | expand |
> The boot process involving ROM-code requires specific handling with > 'unstall' operations which are not required post-boot with normal IPC so > separate set of send-message handlers is added for each of the usecases. consider splitting this long sentence and use simpler logic. It's quite unclear how you went from boot to use cases. > +#include "messages.h" avs_messages.h? > > struct avs_dev; > > @@ -18,6 +19,9 @@ struct avs_dsp_ops { > int (* const power)(struct avs_dev *, u32, bool); > int (* const reset)(struct avs_dev *, u32, bool); > int (* const stall)(struct avs_dev *, u32, bool); > + irqreturn_t (* const irq_handler)(int, void *); > + irqreturn_t (* const irq_thread)(int, void *); > + void (* const int_control)(struct avs_dev *, bool); kernel-doc or comments on what the last op might mean? > }; > > #define avs_dsp_op(adev, op, ...) \ > @@ -34,6 +38,18 @@ struct avs_spec { > > const u32 core_init_mask; /* used during DSP boot */ > const u64 attributes; /* bitmask of AVS_PLATATTR_* */ > + const u32 sram_base_offset; > + const u32 sram_window_size; > + > + const u32 rom_status; > + const u32 hipc_req_offset; > + const u32 hipc_req_ext_offset; > + const u32 hipc_req_busy_mask; > + const u32 hipc_ack_offset; > + const u32 hipc_ack_done_mask; > + const u32 hipc_rsp_offset; > + const u32 hipc_rsp_busy_mask; > + const u32 hipc_ctl_offset; is this really desirable to describe the IPC registers, when we know there were 3 generations of Intel IPC registers. this is ipc-1.5 only. > }; > > struct avs_dev { > @@ -42,6 +58,9 @@ struct avs_dev { > > void __iomem *adsp_ba; > const struct avs_spec *spec; > + struct avs_ipc *ipc; > + > + struct completion fw_ready; > }; > > /* from hda_bus to avs_dev */ > @@ -61,4 +80,78 @@ int avs_dsp_core_stall(struct avs_dev *adev, u32 core_mask, bool stall); > int avs_dsp_core_enable(struct avs_dev *adev, u32 core_mask); > int avs_dsp_core_disable(struct avs_dev *adev, u32 core_mask); > > +/* Inter Process Communication */ > + > +struct avs_ipc_msg { > + union { > + u64 header; > + union avs_global_msg glb; > + union avs_reply_msg rsp; > + }; > + void *data; > + size_t size; > +}; > + > +struct avs_ipc { > + struct device *dev; > + > + struct avs_ipc_msg rx; > + u32 default_timeout_ms; > + bool ready; ready for what? This should be described or documented. > + > + bool rx_completed; > + spinlock_t rx_lock; > + struct mutex msg_mutex; checkpatch would tell you to add a comment for spinlock and mutex. it's quite unclear what they might describe and if they are related. > + struct completion done_completion; > + struct completion busy_completion; > +}; > + > +#define AVS_EIPC EREMOTEIO > +/* > + * IPC handlers may return positive value (firmware error code) what denotes > + * successful HOST <-> DSP communication yet failure to process specific request. > + * > + * Below macro converts returned value to linux kernel error code. > + * All IPC callers MUST use it as soon as firmware error code is consumed. > + */ > +#define AVS_IPC_RET(ret) \ > + (((ret) <= 0) ? (ret) : -AVS_EIPC) why not use -EREMOTEIO directly? -AVS_EIPC is not very useful for the reader. And why -EREMOTEIO? I see that you used it in catpt but that's a very surprising code that no one else uses in sound/ > + > +static inline void avs_ipc_err(struct avs_dev *adev, struct avs_ipc_msg *tx, > + const char *name, int error) > +{ > + /* > + * If IPC channel is blocked e.g.: due to ongoing recovery, > + * -EPERM error code is expected and thus it's not an actual error. > + */ > + if (error == -EPERM) > + dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, > + tx->glb.primary, tx->glb.ext.val, error); > + else > + dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, > + tx->glb.primary, tx->glb.ext.val, error); > +} we've used such functions before and the feedback, e.g. from GregKH and Mark Brown, has consistenly been that this is pushing the use of dev_dbg too far. > +#define AVS_IPC_TIMEOUT_MS 300 skl-sst-ipc.c:#define IPC_TIMEOUT_MSECS 3000 that's one order of magniture lower. please add a comment or align. > +static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header) > +{ > + struct avs_ipc *ipc = adev->ipc; > + union avs_reply_msg msg = AVS_MSG(header); > + > + ipc->rx.header = header; > + if (!msg.status) > + memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev), > + ipc->rx.size); it wouldn't hurt to describe that the status determines whether additional information can be read from a mailbox. > +} > + > +static void avs_dsp_process_notification(struct avs_dev *adev, u64 header) > +{ > + struct avs_notify_mod_data mod_data; > + union avs_notify_msg msg = AVS_MSG(header); > + size_t data_size = 0; > + void *data = NULL; > + > + if (!adev->ipc->ready && msg.notify_msg_type != AVS_NOTIFY_FW_READY) { > + dev_dbg(adev->dev, "FW not ready, skip notification: 0x%08x\n", > + msg.primary); can this happen? you should add a comment on what could be sent before the first 'real' sign of life from the DSP. it's also unclear why this dev_dbg() when 'unknown notifications' below are handled as dev_warn() > + return; > + } > + > + /* Calculate notification payload size. */ > + switch (msg.notify_msg_type) { > + case AVS_NOTIFY_FW_READY: > + break; > + > + case AVS_NOTIFY_PHRASE_DETECTED: > + data_size = sizeof(struct avs_notify_voice_data); > + break; > + > + case AVS_NOTIFY_RESOURCE_EVENT: > + data_size = sizeof(struct avs_notify_res_data); > + break; > + > + case AVS_NOTIFY_MODULE_EVENT: > + memcpy_fromio(&mod_data, avs_uplink_addr(adev), sizeof(mod_data)); > + data_size = sizeof(mod_data) + mod_data.data_size; it wouldn't hurt to describe the layout behing this formula. > + break; > + > + default: > + dev_warn(adev->dev, "unknown notification: 0x%08x\n", > + msg.primary); > + break; > + } > + > + if (data_size) { > + data = kmalloc(data_size, GFP_KERNEL); > + if (!data) > + return; > + > + memcpy_fromio(data, avs_uplink_addr(adev), data_size); > + } > + > + /* Perform notification-specific operations. */ > + switch (msg.notify_msg_type) { > + case AVS_NOTIFY_FW_READY: > + dev_dbg(adev->dev, "FW READY 0x%08x\n", msg.primary); > + adev->ipc->ready = true; avs->ipc->fw_ready? > + complete(&adev->fw_ready);> + break; > + > + default: > + break; > + } > + > + kfree(data); > +} > + > +void avs_dsp_process_response(struct avs_dev *adev, u64 header) > +{ > + struct avs_ipc *ipc = adev->ipc; > + > + if (avs_msg_is_reply(header)) { the naming is confusing, it's difficult for me to understand that a 'response' could not be a 'reply'. The two terms are synonyms, aren't they? > + /* Response processing is invoked from IRQ thread. */ > + spin_lock_irq(&ipc->rx_lock); > + avs_dsp_receive_rx(adev, header); > + ipc->rx_completed = true; > + spin_unlock_irq(&ipc->rx_lock); > + } else { > + avs_dsp_process_notification(adev, header); > + } > + > + complete(&ipc->busy_completion); > +} > + > +irqreturn_t avs_dsp_irq_handler(int irq, void *dev_id) > +{ > + struct avs_dev *adev = dev_id; > + struct avs_ipc *ipc = adev->ipc; > + const struct avs_spec *const spec = adev->spec; > + u32 adspis, hipc_rsp, hipc_ack; > + irqreturn_t ret = IRQ_NONE; > + > + adspis = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS); > + if (adspis == UINT_MAX || !(adspis & AVS_ADSP_ADSPIS_IPC)) > + return ret; > + > + hipc_ack = snd_hdac_adsp_readl(adev, spec->hipc_ack_offset); > + hipc_rsp = snd_hdac_adsp_readl(adev, spec->hipc_rsp_offset); > + > + /* DSP acked host's request */ > + if (hipc_ack & spec->hipc_ack_done_mask) { > + /* mask done interrupt */ > + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, > + AVS_ADSP_HIPCCTL_DONE, 0); > + > + complete(&ipc->done_completion); > + > + /* tell DSP it has our attention */ > + snd_hdac_adsp_updatel(adev, spec->hipc_ack_offset, > + spec->hipc_ack_done_mask, > + spec->hipc_ack_done_mask); > + /* unmask done interrupt */ > + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, > + AVS_ADSP_HIPCCTL_DONE, > + AVS_ADSP_HIPCCTL_DONE); does the order between the complete() and the next two register updates matter? I would have updated the registers immediately and signal the completion later. I am also not sure why it's necessary to mask the done interrupt then unmask it. There is nothing that seems to require this masking? Or are you expecting the code blocked on wait_for_completion to be handled with interrupts masked, which could be quite racy? > + ret = IRQ_HANDLED; > + } > + > + /* DSP sent new response to process */ > + if (hipc_rsp & spec->hipc_rsp_busy_mask) { > + /* mask busy interrupt */ > + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, > + AVS_ADSP_HIPCCTL_BUSY, 0); > + > + ret = IRQ_WAKE_THREAD; > + } > + > + return ret; > +} > +static int avs_ipc_wait_busy_completion(struct avs_ipc *ipc, int timeout) > +{ > + int ret; > + > +again: > + ret = wait_for_completion_timeout(&ipc->busy_completion, > + msecs_to_jiffies(timeout)); > + /* > + * DSP could be unresponsive at this point e.g. manifested by > + * EXCEPTION_CAUGHT notification. If so, no point in continuing. EXCEPTION_CAUGHT doesn't seem to be described in this patchset, so not sure what this comment might mean. > + */ > + if (!ipc->ready) > + return -EPERM; > + > + if (!ret) { > + if (!avs_ipc_is_busy(ipc)) > + return -ETIMEDOUT; > + /* > + * Firmware did its job, either notification or reply > + * has been received - now wait until it's processed. > + */ > + wait_for_completion_killable(&ipc->busy_completion); can you elaborate on why wait_for_completion() is not enough? I haven't seen the 'killable' attribute been used by anyone in sound/ > + } > + > + /* Ongoing notification's bottom-half may cause early wakeup */ > + spin_lock(&ipc->rx_lock); > + if (!ipc->rx_completed) { > + /* Reply delayed due to notification. */ > + reinit_completion(&ipc->busy_completion); > + spin_unlock(&ipc->rx_lock); > + goto again; shouldn't there be some counter to avoid potential infinite loops here? > + } > + > + spin_unlock(&ipc->rx_lock); > + return 0; > +} > +static int avs_dsp_do_send_msg(struct avs_dev *adev, struct avs_ipc_msg *request, > + struct avs_ipc_msg *reply, int timeout) > +{ > + struct avs_ipc *ipc = adev->ipc; > + int ret; > + > + if (!ipc->ready) > + return -EPERM; > + > + mutex_lock(&ipc->msg_mutex); > + > + spin_lock(&ipc->rx_lock); > + avs_ipc_msg_init(ipc, reply); > + avs_dsp_send_tx(adev, request); > + spin_unlock(&ipc->rx_lock); > + > + ret = avs_ipc_wait_busy_completion(ipc, timeout); > + if (ret) { > + if (ret == -ETIMEDOUT) { > + dev_crit(adev->dev, "communication severed: %d, rebooting dsp..\n", > + ret); dev_crit() seems over the top if there is a recovery mechanism > + > + avs_ipc_block(ipc); > + } > + goto exit; > + } > + > + ret = ipc->rx.rsp.status; > + if (reply) { > + reply->header = ipc->rx.header; > + if (reply->data && ipc->rx.size) > + memcpy(reply->data, ipc->rx.data, reply->size); > + } > + > +exit: > + mutex_unlock(&ipc->msg_mutex); > + return ret; > +} > + > +static int avs_dsp_send_msg_sequence(struct avs_dev *adev, > + struct avs_ipc_msg *request, > + struct avs_ipc_msg *reply, int timeout, > + bool wake_d0i0, bool schedule_d0ix) the last two arguments are not used. is this intentional? > +{ > + return avs_dsp_do_send_msg(adev, request, reply, timeout); > +} > + > +int avs_dsp_send_msg_timeout(struct avs_dev *adev, struct avs_ipc_msg *request, > + struct avs_ipc_msg *reply, int timeout) > +{ > + return avs_dsp_send_msg_sequence(adev, request, reply, timeout, > + false, false); > +} > + > +int avs_dsp_send_msg(struct avs_dev *adev, struct avs_ipc_msg *request, > + struct avs_ipc_msg *reply) > +{ > + return avs_dsp_send_msg_timeout(adev, request, reply, > + adev->ipc->default_timeout_ms); > +} is there really a 4-level nesting in your helpers? avs_dsp_send_msg avs_dsp_send_msg_timeout avs_dsp_send_msg_sequence avs_dsp_do_send_msg this seems complicated, no? At the very least you should explain what a message and message sequence are, and why this is split this way. > + > +int avs_dsp_send_pm_msg_timeout(struct avs_dev *adev, > + struct avs_ipc_msg *request, > + struct avs_ipc_msg *reply, int timeout, > + bool wake_d0i0) > +{ > + return avs_dsp_send_msg_sequence(adev, request, reply, timeout, > + wake_d0i0, false); > +} so the 'pm' means 'wake-d0i0'? that's far from intuitive. avs_dsp_send_d0i0_msg_timeout() would better describe what you are trying to do. In addition you need an explanation that d0i0 is a *firmware* concept without direct links to the *device* Dx status. > +int avs_dsp_send_pm_msg(struct avs_dev *adev, > + struct avs_ipc_msg *request, > + struct avs_ipc_msg *reply, bool wake_d0i0) > +{ > + return avs_dsp_send_pm_msg_timeout(adev, request, reply, > + adev->ipc->default_timeout_ms, > + wake_d0i0); > +} > +void avs_dsp_interrupt_control(struct avs_dev *adev, bool enable) > +{ > + const struct avs_spec *const spec = adev->spec; > + u32 value; > + > + value = enable ? AVS_ADSP_ADSPIC_IPC : 0; > + snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPIC, > + AVS_ADSP_ADSPIC_IPC, value); > + > + value = enable ? AVS_ADSP_HIPCCTL_DONE : 0; > + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, > + AVS_ADSP_HIPCCTL_DONE, value); > + > + value = enable ? AVS_ADSP_HIPCCTL_BUSY : 0; > + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, > + AVS_ADSP_HIPCCTL_BUSY, value); does the order matter? please add a comment.
On 2022-02-25 1:56 AM, Pierre-Louis Bossart wrote: >> The boot process involving ROM-code requires specific handling with >> 'unstall' operations which are not required post-boot with normal IPC so >> separate set of send-message handlers is added for each of the usecases. > > consider splitting this long sentence and use simpler logic. It's quite > unclear how you went from boot to use cases. Agree, ack. >> +#include "messages.h" > > avs_messages.h? Same as previously, this header is for internal use only and is found within directory already named 'avs'. "avs.h" header covers a wider range of types and that's why its name is generic. All others are specific and thus are not prefixed with "avs_". >> >> struct avs_dev; >> >> @@ -18,6 +19,9 @@ struct avs_dsp_ops { >> int (* const power)(struct avs_dev *, u32, bool); >> int (* const reset)(struct avs_dev *, u32, bool); >> int (* const stall)(struct avs_dev *, u32, bool); >> + irqreturn_t (* const irq_handler)(int, void *); >> + irqreturn_t (* const irq_thread)(int, void *); >> + void (* const int_control)(struct avs_dev *, bool); > > kernel-doc or comments on what the last op might mean? Sure, will add a comment. >> }; >> >> #define avs_dsp_op(adev, op, ...) \ >> @@ -34,6 +38,18 @@ struct avs_spec { >> >> const u32 core_init_mask; /* used during DSP boot */ >> const u64 attributes; /* bitmask of AVS_PLATATTR_* */ >> + const u32 sram_base_offset; >> + const u32 sram_window_size; >> + >> + const u32 rom_status; >> + const u32 hipc_req_offset; >> + const u32 hipc_req_ext_offset; >> + const u32 hipc_req_busy_mask; >> + const u32 hipc_ack_offset; >> + const u32 hipc_ack_done_mask; >> + const u32 hipc_rsp_offset; >> + const u32 hipc_rsp_busy_mask; >> + const u32 hipc_ctl_offset; > > is this really desirable to describe the IPC registers, when we know > there were 3 generations of Intel IPC registers. this is ipc-1.5 only. Indeed, this abstraction could be removed, ack. >> }; >> >> struct avs_dev { >> @@ -42,6 +58,9 @@ struct avs_dev { >> >> void __iomem *adsp_ba; >> const struct avs_spec *spec; >> + struct avs_ipc *ipc; >> + >> + struct completion fw_ready; >> }; >> >> /* from hda_bus to avs_dev */ >> @@ -61,4 +80,78 @@ int avs_dsp_core_stall(struct avs_dev *adev, u32 core_mask, bool stall); >> int avs_dsp_core_enable(struct avs_dev *adev, u32 core_mask); >> int avs_dsp_core_disable(struct avs_dev *adev, u32 core_mask); >> >> +/* Inter Process Communication */ >> + >> +struct avs_ipc_msg { >> + union { >> + u64 header; >> + union avs_global_msg glb; >> + union avs_reply_msg rsp; >> + }; >> + void *data; >> + size_t size; >> +}; >> + >> +struct avs_ipc { >> + struct device *dev; >> + >> + struct avs_ipc_msg rx; >> + u32 default_timeout_ms; >> + bool ready; > > ready for what? This should be described or documented. In the past this field was called "fw_ready" until we have decided to split struct avs_ipc from struct avs_dev. In my opinion "ipc->ready" looks very intuitive in the code, given that it translates to: inter process communication ready! No problem with adding a comment though. >> + >> + bool rx_completed; >> + spinlock_t rx_lock; >> + struct mutex msg_mutex; > > checkpatch would tell you to add a comment for spinlock and mutex. it's > quite unclear what they might describe and if they are related. I'll add a kernel-doc for just like for the ->ready field. >> + struct completion done_completion; >> + struct completion busy_completion; >> +}; >> + >> +#define AVS_EIPC EREMOTEIO >> +/* >> + * IPC handlers may return positive value (firmware error code) what denotes >> + * successful HOST <-> DSP communication yet failure to process specific request. >> + * >> + * Below macro converts returned value to linux kernel error code. >> + * All IPC callers MUST use it as soon as firmware error code is consumed. >> + */ >> +#define AVS_IPC_RET(ret) \ >> + (((ret) <= 0) ? (ret) : -AVS_EIPC) > > why not use -EREMOTEIO directly? -AVS_EIPC is not very useful for the > reader. > > And why -EREMOTEIO? I see that you used it in catpt but that's a very > surprising code that no one else uses in sound/ Well, the question: "Which kernel error should represent an error coming from remote process AKA audio firmware" needed an answer. EREMOTEIO fits the description and so it was chosen. >> + >> +static inline void avs_ipc_err(struct avs_dev *adev, struct avs_ipc_msg *tx, >> + const char *name, int error) >> +{ >> + /* >> + * If IPC channel is blocked e.g.: due to ongoing recovery, >> + * -EPERM error code is expected and thus it's not an actual error. >> + */ >> + if (error == -EPERM) >> + dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, >> + tx->glb.primary, tx->glb.ext.val, error); >> + else >> + dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, >> + tx->glb.primary, tx->glb.ext.val, error); >> +} > > we've used such functions before and the feedback, e.g. from GregKH and > Mark Brown, has consistenly been that this is pushing the use of dev_dbg > too far. In basically all cases the outcome is going to be dev_err(). dev_dbg() is here to help keep DSP-recovery scenario viewer-friendly when checking dmesg. Ideally, there should be no DSP-recoveries to begin with : ) >> +#define AVS_IPC_TIMEOUT_MS 300 > > skl-sst-ipc.c:#define IPC_TIMEOUT_MSECS 3000 > > that's one order of magniture lower. please add a comment or align. > >> +static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header) >> +{ >> + struct avs_ipc *ipc = adev->ipc; >> + union avs_reply_msg msg = AVS_MSG(header); >> + >> + ipc->rx.header = header; >> + if (!msg.status) >> + memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev), >> + ipc->rx.size); > > it wouldn't hurt to describe that the status determines whether > additional information can be read from a mailbox. Isn't that consisted with the behaviour of typical API function? Do not copy memory and return it to the caller if something went wrong along the way? >> +} >> + >> +static void avs_dsp_process_notification(struct avs_dev *adev, u64 header) >> +{ >> + struct avs_notify_mod_data mod_data; >> + union avs_notify_msg msg = AVS_MSG(header); >> + size_t data_size = 0; >> + void *data = NULL; >> + >> + if (!adev->ipc->ready && msg.notify_msg_type != AVS_NOTIFY_FW_READY) { >> + dev_dbg(adev->dev, "FW not ready, skip notification: 0x%08x\n", >> + msg.primary); > > can this happen? > > you should add a comment on what could be sent before the first 'real' > sign of life from the DSP. > > it's also unclear why this dev_dbg() when 'unknown notifications' below > are handled as dev_warn() I would like to say: "no, this situation cannot happen" very much, but that's simply not true. Any notification could be sent prior to FW_READY as the internal queue may not always get flushed between the firmware restoring. Ack on the s/warn/info/ part. >> + return; >> + } >> + >> + /* Calculate notification payload size. */ >> + switch (msg.notify_msg_type) { >> + case AVS_NOTIFY_FW_READY: >> + break; >> + >> + case AVS_NOTIFY_PHRASE_DETECTED: >> + data_size = sizeof(struct avs_notify_voice_data); >> + break; >> + >> + case AVS_NOTIFY_RESOURCE_EVENT: >> + data_size = sizeof(struct avs_notify_res_data); >> + break; >> + >> + case AVS_NOTIFY_MODULE_EVENT: >> + memcpy_fromio(&mod_data, avs_uplink_addr(adev), sizeof(mod_data)); >> + data_size = sizeof(mod_data) + mod_data.data_size; > > it wouldn't hurt to describe the layout behing this formula. The layout is kind of implied by the structure itself but a comment wouldn't hurt, agree. >> + break; >> + >> + default: >> + dev_warn(adev->dev, "unknown notification: 0x%08x\n", >> + msg.primary); >> + break; >> + } >> + >> + if (data_size) { >> + data = kmalloc(data_size, GFP_KERNEL); >> + if (!data) >> + return; >> + >> + memcpy_fromio(data, avs_uplink_addr(adev), data_size); >> + } >> + >> + /* Perform notification-specific operations. */ >> + switch (msg.notify_msg_type) { >> + case AVS_NOTIFY_FW_READY: >> + dev_dbg(adev->dev, "FW READY 0x%08x\n", msg.primary); >> + adev->ipc->ready = true; > > avs->ipc->fw_ready? As I have explained earlier, this was the case until we have separated struct avs_ipc from struct avs_dev. I'll provide a kernel-doc instead. >> + complete(&adev->fw_ready);> + break; >> + >> + default: >> + break; >> + } >> + >> + kfree(data); >> +} >> + >> +void avs_dsp_process_response(struct avs_dev *adev, u64 header) >> +{ >> + struct avs_ipc *ipc = adev->ipc; >> + >> + if (avs_msg_is_reply(header)) { > > the naming is confusing, it's difficult for me to understand that a > 'response' could not be a 'reply'. The two terms are synonyms, aren't they? Those two are not the same from the firmware's point of view and thus they are not the same here. Response is either a reply or a notification. Replies are solicited, a request has been sent beforehand. Notifications are unsolicited, you are not sure when exactly and if at all they arrive. Just so I'm not called a heretic later: yes, from English dictionary point of view these two words are synonyms. In general, wording found in this drivers matches firmware equivalents wherever possible to allow developers to switch between these two worlds with minimal adaptation period possible. >> + /* DSP acked host's request */ >> + if (hipc_ack & spec->hipc_ack_done_mask) { >> + /* mask done interrupt */ >> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >> + AVS_ADSP_HIPCCTL_DONE, 0); >> + >> + complete(&ipc->done_completion); >> + >> + /* tell DSP it has our attention */ >> + snd_hdac_adsp_updatel(adev, spec->hipc_ack_offset, >> + spec->hipc_ack_done_mask, >> + spec->hipc_ack_done_mask); >> + /* unmask done interrupt */ >> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >> + AVS_ADSP_HIPCCTL_DONE, >> + AVS_ADSP_HIPCCTL_DONE); > > does the order between the complete() and the next two register updates > matter? > > I would have updated the registers immediately and signal the completion > later. > > I am also not sure why it's necessary to mask the done interrupt then > unmask it. There is nothing that seems to require this masking? > > Or are you expecting the code blocked on wait_for_completion to be > handled with interrupts masked, which could be quite racy? Given how the things turned out in cAVS, some steps are not always required. Here, we have very strict implementation and so interrupt are masked. I'm unsure if relocating complete() to the bottom would bring any consequences. And no, the code waiting_for_completion is not expecting interrupts to be masked as there is no reply for ROM messages. >> + ret = IRQ_HANDLED; >> + } >> + >> + /* DSP sent new response to process */ >> + if (hipc_rsp & spec->hipc_rsp_busy_mask) { >> + /* mask busy interrupt */ >> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >> + AVS_ADSP_HIPCCTL_BUSY, 0); >> + >> + ret = IRQ_WAKE_THREAD; >> + } >> + >> + return ret; >> +} > >> +static int avs_ipc_wait_busy_completion(struct avs_ipc *ipc, int timeout) >> +{ >> + int ret; >> + >> +again: >> + ret = wait_for_completion_timeout(&ipc->busy_completion, >> + msecs_to_jiffies(timeout)); >> + /* >> + * DSP could be unresponsive at this point e.g. manifested by >> + * EXCEPTION_CAUGHT notification. If so, no point in continuing. > > EXCEPTION_CAUGHT doesn't seem to be described in this patchset, so not > sure what this comment might mean. Comment describes the circumstances for the communication failures and arrival of EXCEPTION_CAUGHT notification is one of them. >> + */ >> + if (!ipc->ready) >> + return -EPERM; >> + >> + if (!ret) { >> + if (!avs_ipc_is_busy(ipc)) >> + return -ETIMEDOUT; >> + /* >> + * Firmware did its job, either notification or reply >> + * has been received - now wait until it's processed. >> + */ >> + wait_for_completion_killable(&ipc->busy_completion); > > can you elaborate on why wait_for_completion() is not enough? I haven't > seen the 'killable' attribute been used by anyone in sound/ This is connected to how firmware handles messaging i.e. via queue. you may get BUSY interrupt caused by a notification while waiting for the reply for your request. Being 'disturbed' by a notification does not mean firmware is dead, it's just busy and so we wait until previous response is processed entirely. >> + } >> + >> + /* Ongoing notification's bottom-half may cause early wakeup */ >> + spin_lock(&ipc->rx_lock); >> + if (!ipc->rx_completed) { >> + /* Reply delayed due to notification. */ >> + reinit_completion(&ipc->busy_completion); >> + spin_unlock(&ipc->rx_lock); >> + goto again; > > shouldn't there be some counter to avoid potential infinite loops here? This is not a bad idea although the counter is going to be high e.g.: 128. With DEBUG-level logs enabled you can get ton of messages before your reply gets finally sent. >> + } >> + >> + spin_unlock(&ipc->rx_lock); >> + return 0; >> +} > >> +static int avs_dsp_do_send_msg(struct avs_dev *adev, struct avs_ipc_msg *request, >> + struct avs_ipc_msg *reply, int timeout) >> +{ >> + struct avs_ipc *ipc = adev->ipc; >> + int ret; >> + >> + if (!ipc->ready) >> + return -EPERM; >> + >> + mutex_lock(&ipc->msg_mutex); >> + >> + spin_lock(&ipc->rx_lock); >> + avs_ipc_msg_init(ipc, reply); >> + avs_dsp_send_tx(adev, request); >> + spin_unlock(&ipc->rx_lock); >> + >> + ret = avs_ipc_wait_busy_completion(ipc, timeout); >> + if (ret) { >> + if (ret == -ETIMEDOUT) { >> + dev_crit(adev->dev, "communication severed: %d, rebooting dsp..\n", >> + ret); > > dev_crit() seems over the top if there is a recovery mechanism There is just one dev_crit() within entire driver and it's there for a reason - communication failure is critical and in practice, should never occur in any scenario on the production hardware. >> + >> + avs_ipc_block(ipc); >> + } >> + goto exit; >> + } >> + >> + ret = ipc->rx.rsp.status; >> + if (reply) { >> + reply->header = ipc->rx.header; >> + if (reply->data && ipc->rx.size) >> + memcpy(reply->data, ipc->rx.data, reply->size); >> + } >> + >> +exit: >> + mutex_unlock(&ipc->msg_mutex); >> + return ret; >> +} >> + >> +static int avs_dsp_send_msg_sequence(struct avs_dev *adev, >> + struct avs_ipc_msg *request, >> + struct avs_ipc_msg *reply, int timeout, >> + bool wake_d0i0, bool schedule_d0ix) > > the last two arguments are not used. is this intentional? Used by the d0ix implementation that is not part of this part. Can relocate. >> +{ >> + return avs_dsp_do_send_msg(adev, request, reply, timeout); >> +} >> + >> +int avs_dsp_send_msg_timeout(struct avs_dev *adev, struct avs_ipc_msg *request, >> + struct avs_ipc_msg *reply, int timeout) >> +{ >> + return avs_dsp_send_msg_sequence(adev, request, reply, timeout, >> + false, false); >> +} >> + >> +int avs_dsp_send_msg(struct avs_dev *adev, struct avs_ipc_msg *request, >> + struct avs_ipc_msg *reply) >> +{ >> + return avs_dsp_send_msg_timeout(adev, request, reply, >> + adev->ipc->default_timeout_ms); >> +} > > is there really a 4-level nesting in your helpers? > > avs_dsp_send_msg > avs_dsp_send_msg_timeout > avs_dsp_send_msg_sequence > avs_dsp_do_send_msg > > this seems complicated, no? > > At the very least you should explain what a message and message sequence > are, and why this is split this way. With d0ix handling added, it becomes clear why such separation exists. I left these parts here to reduce the delta in patches that update this code later on. Can simplify here and update the d0ix implementation accordingly. >> + >> +int avs_dsp_send_pm_msg_timeout(struct avs_dev *adev, >> + struct avs_ipc_msg *request, >> + struct avs_ipc_msg *reply, int timeout, >> + bool wake_d0i0) >> +{ >> + return avs_dsp_send_msg_sequence(adev, request, reply, timeout, >> + wake_d0i0, false); >> +} > > so the 'pm' means 'wake-d0i0'? that's far from intuitive. > > avs_dsp_send_d0i0_msg_timeout() would better describe what you are > trying to do. > > In addition you need an explanation that d0i0 is a *firmware* concept > without direct links to the *device* Dx status. This goes for both, Dx and D0ix related messages. >> +void avs_dsp_interrupt_control(struct avs_dev *adev, bool enable) >> +{ >> + const struct avs_spec *const spec = adev->spec; >> + u32 value; >> + >> + value = enable ? AVS_ADSP_ADSPIC_IPC : 0; >> + snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPIC, >> + AVS_ADSP_ADSPIC_IPC, value); >> + >> + value = enable ? AVS_ADSP_HIPCCTL_DONE : 0; >> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >> + AVS_ADSP_HIPCCTL_DONE, value); >> + >> + value = enable ? AVS_ADSP_HIPCCTL_BUSY : 0; >> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >> + AVS_ADSP_HIPCCTL_BUSY, value); > > does the order matter? please add a comment. interrupt_control() is only used during probing and teardown procedures so the order does not matter. You need all those bits up before firmware loading can even begin. And when you are done with firmware, you zero these out but and the order does not matter either as again, there's none to "talk" to. Note: ADSPIC_IPC is higher in hierarchy than DONE and BUSY.
>>> +static inline void avs_ipc_err(struct avs_dev *adev, struct >>> avs_ipc_msg *tx, >>> + const char *name, int error) >>> +{ >>> + /* >>> + * If IPC channel is blocked e.g.: due to ongoing recovery, >>> + * -EPERM error code is expected and thus it's not an actual error. >>> + */ >>> + if (error == -EPERM) >>> + dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, >>> + tx->glb.primary, tx->glb.ext.val, error); >>> + else >>> + dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, >>> + tx->glb.primary, tx->glb.ext.val, error); >>> +} >> >> we've used such functions before and the feedback, e.g. from GregKH and >> Mark Brown, has consistenly been that this is pushing the use of dev_dbg >> too far. > > > In basically all cases the outcome is going to be dev_err(). dev_dbg() > is here to help keep DSP-recovery scenario viewer-friendly when checking > dmesg. Ideally, there should be no DSP-recoveries to begin with : ) I will refer you to this thread: https://lore.kernel.org/alsa-devel/YGX5AUQi41z52xk8@kroah.com/ > >>> +#define AVS_IPC_TIMEOUT_MS 300 >> >> skl-sst-ipc.c:#define IPC_TIMEOUT_MSECS 3000 >> >> that's one order of magniture lower. please add a comment or align. >> >>> +static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header) >>> +{ >>> + struct avs_ipc *ipc = adev->ipc; >>> + union avs_reply_msg msg = AVS_MSG(header); >>> + >>> + ipc->rx.header = header; >>> + if (!msg.status) >>> + memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev), >>> + ipc->rx.size); >> >> it wouldn't hurt to describe that the status determines whether >> additional information can be read from a mailbox. > > > Isn't that consisted with the behaviour of typical API function? Do not > copy memory and return it to the caller if something went wrong along > the way? oh, I thought this was a case where the header contains all the information, and only in specific cases you need to read stuff from the mailbox. You definitively need to add a comment on whether this is an error handling or a feature. >>> +void avs_dsp_process_response(struct avs_dev *adev, u64 header) >>> +{ >>> + struct avs_ipc *ipc = adev->ipc; >>> + >>> + if (avs_msg_is_reply(header)) { >> >> the naming is confusing, it's difficult for me to understand that a >> 'response' could not be a 'reply'. The two terms are synonyms, aren't >> they? > > > Those two are not the same from the firmware's point of view and thus > they are not the same here. Response is either a reply or a > notification. Replies are solicited, a request has been sent beforehand. > Notifications are unsolicited, you are not sure when exactly and if at > all they arrive. add a comment then. > Just so I'm not called a heretic later: yes, from English dictionary > point of view these two words are synonyms. In general, wording found in > this drivers matches firmware equivalents wherever possible to allow > developers to switch between these two worlds with minimal adaptation > period possible. > >>> + /* DSP acked host's request */ >>> + if (hipc_ack & spec->hipc_ack_done_mask) { >>> + /* mask done interrupt */ >>> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >>> + AVS_ADSP_HIPCCTL_DONE, 0); >>> + >>> + complete(&ipc->done_completion); >>> + >>> + /* tell DSP it has our attention */ >>> + snd_hdac_adsp_updatel(adev, spec->hipc_ack_offset, >>> + spec->hipc_ack_done_mask, >>> + spec->hipc_ack_done_mask); >>> + /* unmask done interrupt */ >>> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >>> + AVS_ADSP_HIPCCTL_DONE, >>> + AVS_ADSP_HIPCCTL_DONE); >> >> does the order between the complete() and the next two register updates >> matter? >> >> I would have updated the registers immediately and signal the completion >> later. >> >> I am also not sure why it's necessary to mask the done interrupt then >> unmask it. There is nothing that seems to require this masking? >> >> Or are you expecting the code blocked on wait_for_completion to be >> handled with interrupts masked, which could be quite racy? > > > Given how the things turned out in cAVS, some steps are not always > required. Here, we have very strict implementation and so interrupt are > masked. > > I'm unsure if relocating complete() to the bottom would bring any > consequences. And no, the code waiting_for_completion is not expecting > interrupts to be masked as there is no reply for ROM messages. it would be just fine to add that the masking is added as an extra precaution, the order does not matter and the code executed after the complete() does not assume any masking. > >>> + ret = IRQ_HANDLED; >>> + } >>> + >>> + /* DSP sent new response to process */ >>> + if (hipc_rsp & spec->hipc_rsp_busy_mask) { >>> + /* mask busy interrupt */ >>> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >>> + AVS_ADSP_HIPCCTL_BUSY, 0); >>> + >>> + ret = IRQ_WAKE_THREAD; >>> + } >>> + >>> + return ret; >>> +} >> >>> +static int avs_ipc_wait_busy_completion(struct avs_ipc *ipc, int >>> timeout) >>> +{ >>> + int ret; >>> + >>> +again: >>> + ret = wait_for_completion_timeout(&ipc->busy_completion, >>> + msecs_to_jiffies(timeout)); >>> + /* >>> + * DSP could be unresponsive at this point e.g. manifested by >>> + * EXCEPTION_CAUGHT notification. If so, no point in continuing. >> >> EXCEPTION_CAUGHT doesn't seem to be described in this patchset, so not >> sure what this comment might mean. > > > Comment describes the circumstances for the communication failures and > arrival of EXCEPTION_CAUGHT notification is one of them. that detail is unnecessary for reviewers. > >>> + */ >>> + if (!ipc->ready) >>> + return -EPERM; >>> + >>> + if (!ret) { >>> + if (!avs_ipc_is_busy(ipc)) >>> + return -ETIMEDOUT; >>> + /* >>> + * Firmware did its job, either notification or reply >>> + * has been received - now wait until it's processed. >>> + */ >>> + wait_for_completion_killable(&ipc->busy_completion); >> >> can you elaborate on why wait_for_completion() is not enough? I haven't >> seen the 'killable' attribute been used by anyone in sound/ > > > This is connected to how firmware handles messaging i.e. via queue. you > may get BUSY interrupt caused by a notification while waiting for the > reply for your request. Being 'disturbed' by a notification does not > mean firmware is dead, it's just busy and so we wait until previous > response is processed entirely. this does not clarify why 'killable' is necessary? > >>> + } >>> + >>> + /* Ongoing notification's bottom-half may cause early wakeup */ >>> + spin_lock(&ipc->rx_lock); >>> + if (!ipc->rx_completed) { >>> + /* Reply delayed due to notification. */ >>> + reinit_completion(&ipc->busy_completion); >>> + spin_unlock(&ipc->rx_lock); >>> + goto again; >> >> shouldn't there be some counter to avoid potential infinite loops here? > > > This is not a bad idea although the counter is going to be high e.g.: > 128. With DEBUG-level logs enabled you can get ton of messages before > your reply gets finally sent. dev_dbg() in interrupts is usually not very helpful. we're trying to move to traces instead. > >>> + } >>> + >>> + spin_unlock(&ipc->rx_lock); >>> + return 0; >>> +} >> >>> +static int avs_dsp_do_send_msg(struct avs_dev *adev, struct >>> avs_ipc_msg *request, >>> + struct avs_ipc_msg *reply, int timeout) >>> +{ >>> + struct avs_ipc *ipc = adev->ipc; >>> + int ret; >>> + >>> + if (!ipc->ready) >>> + return -EPERM; >>> + >>> + mutex_lock(&ipc->msg_mutex); >>> + >>> + spin_lock(&ipc->rx_lock); >>> + avs_ipc_msg_init(ipc, reply); >>> + avs_dsp_send_tx(adev, request); >>> + spin_unlock(&ipc->rx_lock); >>> + >>> + ret = avs_ipc_wait_busy_completion(ipc, timeout); >>> + if (ret) { >>> + if (ret == -ETIMEDOUT) { >>> + dev_crit(adev->dev, "communication severed: %d, >>> rebooting dsp..\n", >>> + ret); >> >> dev_crit() seems over the top if there is a recovery mechanism > > > There is just one dev_crit() within entire driver and it's there for a > reason - communication failure is critical and in practice, should never > occur in any scenario on the production hardware. git grep dev_crit shows mostly this being used for temperature and shorts in codec drivers. that seems more 'critical' than a communication failure that likely does not result in spontaneous combustion.
On 2022-02-25 9:37 PM, Pierre-Louis Bossart wrote: > >>>> +static inline void avs_ipc_err(struct avs_dev *adev, struct >>>> avs_ipc_msg *tx, >>>> + const char *name, int error) >>>> +{ >>>> + /* >>>> + * If IPC channel is blocked e.g.: due to ongoing recovery, >>>> + * -EPERM error code is expected and thus it's not an actual error. >>>> + */ >>>> + if (error == -EPERM) >>>> + dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, >>>> + tx->glb.primary, tx->glb.ext.val, error); >>>> + else >>>> + dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, >>>> + tx->glb.primary, tx->glb.ext.val, error); >>>> +} >>> >>> we've used such functions before and the feedback, e.g. from GregKH and >>> Mark Brown, has consistenly been that this is pushing the use of dev_dbg >>> too far. >> >> >> In basically all cases the outcome is going to be dev_err(). dev_dbg() >> is here to help keep DSP-recovery scenario viewer-friendly when checking >> dmesg. Ideally, there should be no DSP-recoveries to begin with : ) > > I will refer you to this thread: > > https://lore.kernel.org/alsa-devel/YGX5AUQi41z52xk8@kroah.com/ That's an interesting lecture, thanks for sharing the link. Most of the time, we do want to dump an dev_err() if function fails for non-trivial reason. During recovery scenario though, we force-disconnect all the streams before attempting DSP reboot. That results in "wall of red" i.e. error messages. Since we know that all these errors are caused by the disconnection of the streams, there is no real value for flagging them as errors. It's debug-friendly (for a developer addressing the possible problem) to have only important marked as errors in dmesg. Also, avs_ipc_err() has a very specific purpose and is used only by IPC handlers and nowhere else. >> >>>> +#define AVS_IPC_TIMEOUT_MS 300 >>> >>> skl-sst-ipc.c:#define IPC_TIMEOUT_MSECS 3000 >>> >>> that's one order of magniture lower. please add a comment or align. >>> >>>> +static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header) >>>> +{ >>>> + struct avs_ipc *ipc = adev->ipc; >>>> + union avs_reply_msg msg = AVS_MSG(header); >>>> + >>>> + ipc->rx.header = header; >>>> + if (!msg.status) >>>> + memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev), >>>> + ipc->rx.size); >>> >>> it wouldn't hurt to describe that the status determines whether >>> additional information can be read from a mailbox. >> >> >> Isn't that consisted with the behaviour of typical API function? Do not >> copy memory and return it to the caller if something went wrong along >> the way? > > oh, I thought this was a case where the header contains all the > information, and only in specific cases you need to read stuff from the > mailbox. > > You definitively need to add a comment on whether this is an error > handling or a feature. Ack. >>>> +void avs_dsp_process_response(struct avs_dev *adev, u64 header) >>>> +{ >>>> + struct avs_ipc *ipc = adev->ipc; >>>> + >>>> + if (avs_msg_is_reply(header)) { >>> >>> the naming is confusing, it's difficult for me to understand that a >>> 'response' could not be a 'reply'. The two terms are synonyms, aren't >>> they? >> >> >> Those two are not the same from the firmware's point of view and thus >> they are not the same here. Response is either a reply or a >> notification. Replies are solicited, a request has been sent beforehand. >> Notifications are unsolicited, you are not sure when exactly and if at >> all they arrive. > > add a comment then. Ack. >> Just so I'm not called a heretic later: yes, from English dictionary >> point of view these two words are synonyms. In general, wording found in >> this drivers matches firmware equivalents wherever possible to allow >> developers to switch between these two worlds with minimal adaptation >> period possible. > >> >>>> + /* DSP acked host's request */ >>>> + if (hipc_ack & spec->hipc_ack_done_mask) { >>>> + /* mask done interrupt */ >>>> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >>>> + AVS_ADSP_HIPCCTL_DONE, 0); >>>> + >>>> + complete(&ipc->done_completion); >>>> + >>>> + /* tell DSP it has our attention */ >>>> + snd_hdac_adsp_updatel(adev, spec->hipc_ack_offset, >>>> + spec->hipc_ack_done_mask, >>>> + spec->hipc_ack_done_mask); >>>> + /* unmask done interrupt */ >>>> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >>>> + AVS_ADSP_HIPCCTL_DONE, >>>> + AVS_ADSP_HIPCCTL_DONE); >>> >>> does the order between the complete() and the next two register updates >>> matter? >>> >>> I would have updated the registers immediately and signal the completion >>> later. >>> >>> I am also not sure why it's necessary to mask the done interrupt then >>> unmask it. There is nothing that seems to require this masking? >>> >>> Or are you expecting the code blocked on wait_for_completion to be >>> handled with interrupts masked, which could be quite racy? >> >> >> Given how the things turned out in cAVS, some steps are not always >> required. Here, we have very strict implementation and so interrupt are >> masked. >> >> I'm unsure if relocating complete() to the bottom would bring any >> consequences. And no, the code waiting_for_completion is not expecting >> interrupts to be masked as there is no reply for ROM messages. > > it would be just fine to add that the masking is added as an extra > precaution, the order does not matter and the code executed after the > complete() does not assume any masking. Ack. >> >>>> + ret = IRQ_HANDLED; >>>> + } >>>> + >>>> + /* DSP sent new response to process */ >>>> + if (hipc_rsp & spec->hipc_rsp_busy_mask) { >>>> + /* mask busy interrupt */ >>>> + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, >>>> + AVS_ADSP_HIPCCTL_BUSY, 0); >>>> + >>>> + ret = IRQ_WAKE_THREAD; >>>> + } >>>> + >>>> + return ret; >>>> +} >>> >>>> +static int avs_ipc_wait_busy_completion(struct avs_ipc *ipc, int >>>> timeout) >>>> +{ >>>> + int ret; >>>> + >>>> +again: >>>> + ret = wait_for_completion_timeout(&ipc->busy_completion, >>>> + msecs_to_jiffies(timeout)); >>>> + /* >>>> + * DSP could be unresponsive at this point e.g. manifested by >>>> + * EXCEPTION_CAUGHT notification. If so, no point in continuing. >>> >>> EXCEPTION_CAUGHT doesn't seem to be described in this patchset, so not >>> sure what this comment might mean. >> >> >> Comment describes the circumstances for the communication failures and >> arrival of EXCEPTION_CAUGHT notification is one of them. > > that detail is unnecessary for reviewers. Ack. >> >>>> + */ >>>> + if (!ipc->ready) >>>> + return -EPERM; >>>> + >>>> + if (!ret) { >>>> + if (!avs_ipc_is_busy(ipc)) >>>> + return -ETIMEDOUT; >>>> + /* >>>> + * Firmware did its job, either notification or reply >>>> + * has been received - now wait until it's processed. >>>> + */ >>>> + wait_for_completion_killable(&ipc->busy_completion); >>> >>> can you elaborate on why wait_for_completion() is not enough? I haven't >>> seen the 'killable' attribute been used by anyone in sound/ >> >> >> This is connected to how firmware handles messaging i.e. via queue. you >> may get BUSY interrupt caused by a notification while waiting for the >> reply for your request. Being 'disturbed' by a notification does not >> mean firmware is dead, it's just busy and so we wait until previous >> response is processed entirely. > > this does not clarify why 'killable' is necessary? Usage of 'killable' variant adheres to its documentation. Sys calls can terminate the waiter. More user friendly. >> >>>> + } >>>> + >>>> + /* Ongoing notification's bottom-half may cause early wakeup */ >>>> + spin_lock(&ipc->rx_lock); >>>> + if (!ipc->rx_completed) { >>>> + /* Reply delayed due to notification. */ >>>> + reinit_completion(&ipc->busy_completion); >>>> + spin_unlock(&ipc->rx_lock); >>>> + goto again; >>> >>> shouldn't there be some counter to avoid potential infinite loops here? >> >> >> This is not a bad idea although the counter is going to be high e.g.: >> 128. With DEBUG-level logs enabled you can get ton of messages before >> your reply gets finally sent. > > dev_dbg() in interrupts is usually not very helpful. we're trying to > move to traces instead. Wasn't precise enough, I appologize for that. By "DEBUG-level logs" I meant firmware logging, not dev_dbg() on kernel side. When enabled with log level DEBUG, you will get at least 1 message per sys tick, resulting in gigabyte logs in no time. >> >>>> + } >>>> + >>>> + spin_unlock(&ipc->rx_lock); >>>> + return 0; >>>> +} >>> >>>> +static int avs_dsp_do_send_msg(struct avs_dev *adev, struct >>>> avs_ipc_msg *request, >>>> + struct avs_ipc_msg *reply, int timeout) >>>> +{ >>>> + struct avs_ipc *ipc = adev->ipc; >>>> + int ret; >>>> + >>>> + if (!ipc->ready) >>>> + return -EPERM; >>>> + >>>> + mutex_lock(&ipc->msg_mutex); >>>> + >>>> + spin_lock(&ipc->rx_lock); >>>> + avs_ipc_msg_init(ipc, reply); >>>> + avs_dsp_send_tx(adev, request); >>>> + spin_unlock(&ipc->rx_lock); >>>> + >>>> + ret = avs_ipc_wait_busy_completion(ipc, timeout); >>>> + if (ret) { >>>> + if (ret == -ETIMEDOUT) { >>>> + dev_crit(adev->dev, "communication severed: %d, >>>> rebooting dsp..\n", >>>> + ret); >>> >>> dev_crit() seems over the top if there is a recovery mechanism >> >> >> There is just one dev_crit() within entire driver and it's there for a >> reason - communication failure is critical and in practice, should never >> occur in any scenario on the production hardware. > > git grep dev_crit shows mostly this being used for temperature and > shorts in codec drivers. that seems more 'critical' than a communication > failure that likely does not result in spontaneous combustion. Few dev_crit()s can also be found in other components as well. Without audio and graphics there is no real 'user experience'. Abrupt closure of communication between DSP firmware and kernel driver can, and usually is a consequence of either an undefined behaviour (in process running on DSP) or hardware issue. While I can't spare the details here for obvious reasons, not all situations can even be recovered with reboot. That usually depends in which power wells registers reside. The 100% confirmed solution for laptops is removing battery for a second or day to force G3. Considering this, I believe having a single dev_crit() here is justified. Regards, Czarek
On 2022-02-28 4:19 PM, Cezary Rojewski wrote: > Few dev_crit()s can also be found in other components as well. > > Without audio and graphics there is no real 'user experience'. Abrupt > closure of communication between DSP firmware and kernel driver can, and > usually is a consequence of either an undefined behaviour (in process > running on DSP) or hardware issue. While I can't spare the details here > for obvious reasons, not all situations can even be recovered with > reboot. That usually depends in which power wells registers reside. The > 100% confirmed solution for laptops is removing battery for a second or > day to force G3. s/second or day/second or two/ Hope people are not panicking after that typo =) > Considering this, I believe having a single dev_crit() here is justified.
diff --git a/sound/soc/intel/avs/Makefile b/sound/soc/intel/avs/Makefile index 5f7976a95fe2..e243806dd38a 100644 --- a/sound/soc/intel/avs/Makefile +++ b/sound/soc/intel/avs/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0-only -snd-soc-avs-objs := dsp.o +snd-soc-avs-objs := dsp.o ipc.o obj-$(CONFIG_SND_SOC_INTEL_AVS) += snd-soc-avs.o diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h index 7ece210b0777..8620d2f7fee0 100644 --- a/sound/soc/intel/avs/avs.h +++ b/sound/soc/intel/avs/avs.h @@ -11,6 +11,7 @@ #include <linux/device.h> #include <sound/hda_codec.h> +#include "messages.h" struct avs_dev; @@ -18,6 +19,9 @@ struct avs_dsp_ops { int (* const power)(struct avs_dev *, u32, bool); int (* const reset)(struct avs_dev *, u32, bool); int (* const stall)(struct avs_dev *, u32, bool); + irqreturn_t (* const irq_handler)(int, void *); + irqreturn_t (* const irq_thread)(int, void *); + void (* const int_control)(struct avs_dev *, bool); }; #define avs_dsp_op(adev, op, ...) \ @@ -34,6 +38,18 @@ struct avs_spec { const u32 core_init_mask; /* used during DSP boot */ const u64 attributes; /* bitmask of AVS_PLATATTR_* */ + const u32 sram_base_offset; + const u32 sram_window_size; + + const u32 rom_status; + const u32 hipc_req_offset; + const u32 hipc_req_ext_offset; + const u32 hipc_req_busy_mask; + const u32 hipc_ack_offset; + const u32 hipc_ack_done_mask; + const u32 hipc_rsp_offset; + const u32 hipc_rsp_busy_mask; + const u32 hipc_ctl_offset; }; struct avs_dev { @@ -42,6 +58,9 @@ struct avs_dev { void __iomem *adsp_ba; const struct avs_spec *spec; + struct avs_ipc *ipc; + + struct completion fw_ready; }; /* from hda_bus to avs_dev */ @@ -61,4 +80,78 @@ int avs_dsp_core_stall(struct avs_dev *adev, u32 core_mask, bool stall); int avs_dsp_core_enable(struct avs_dev *adev, u32 core_mask); int avs_dsp_core_disable(struct avs_dev *adev, u32 core_mask); +/* Inter Process Communication */ + +struct avs_ipc_msg { + union { + u64 header; + union avs_global_msg glb; + union avs_reply_msg rsp; + }; + void *data; + size_t size; +}; + +struct avs_ipc { + struct device *dev; + + struct avs_ipc_msg rx; + u32 default_timeout_ms; + bool ready; + + bool rx_completed; + spinlock_t rx_lock; + struct mutex msg_mutex; + struct completion done_completion; + struct completion busy_completion; +}; + +#define AVS_EIPC EREMOTEIO +/* + * IPC handlers may return positive value (firmware error code) what denotes + * successful HOST <-> DSP communication yet failure to process specific request. + * + * Below macro converts returned value to linux kernel error code. + * All IPC callers MUST use it as soon as firmware error code is consumed. + */ +#define AVS_IPC_RET(ret) \ + (((ret) <= 0) ? (ret) : -AVS_EIPC) + +static inline void avs_ipc_err(struct avs_dev *adev, struct avs_ipc_msg *tx, + const char *name, int error) +{ + /* + * If IPC channel is blocked e.g.: due to ongoing recovery, + * -EPERM error code is expected and thus it's not an actual error. + */ + if (error == -EPERM) + dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, + tx->glb.primary, tx->glb.ext.val, error); + else + dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, + tx->glb.primary, tx->glb.ext.val, error); +} + +irqreturn_t avs_dsp_irq_handler(int irq, void *dev_id); +irqreturn_t avs_dsp_irq_thread(int irq, void *dev_id); +void avs_dsp_process_response(struct avs_dev *adev, u64 header); +int avs_dsp_send_pm_msg_timeout(struct avs_dev *adev, + struct avs_ipc_msg *request, + struct avs_ipc_msg *reply, int timeout, + bool wake_d0i0); +int avs_dsp_send_pm_msg(struct avs_dev *adev, + struct avs_ipc_msg *request, + struct avs_ipc_msg *reply, bool wake_d0i0); +int avs_dsp_send_msg_timeout(struct avs_dev *adev, + struct avs_ipc_msg *request, + struct avs_ipc_msg *reply, int timeout); +int avs_dsp_send_msg(struct avs_dev *adev, + struct avs_ipc_msg *request, struct avs_ipc_msg *reply); +int avs_dsp_send_rom_msg_timeout(struct avs_dev *adev, + struct avs_ipc_msg *request, int timeout); +int avs_dsp_send_rom_msg(struct avs_dev *adev, struct avs_ipc_msg *request); +void avs_dsp_interrupt_control(struct avs_dev *adev, bool enable); +int avs_ipc_init(struct avs_ipc *ipc, struct device *dev); +void avs_ipc_block(struct avs_ipc *ipc); + #endif /* __SOUND_SOC_INTEL_AVS_H */ diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c new file mode 100644 index 000000000000..69178b5d39b1 --- /dev/null +++ b/sound/soc/intel/avs/ipc.c @@ -0,0 +1,404 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright(c) 2021 Intel Corporation. All rights reserved. +// +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> +// + +#include <linux/slab.h> +#include <sound/hdaudio_ext.h> +#include "avs.h" +#include "messages.h" +#include "registers.h" + +#define AVS_IPC_TIMEOUT_MS 300 + +static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header) +{ + struct avs_ipc *ipc = adev->ipc; + union avs_reply_msg msg = AVS_MSG(header); + + ipc->rx.header = header; + if (!msg.status) + memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev), + ipc->rx.size); +} + +static void avs_dsp_process_notification(struct avs_dev *adev, u64 header) +{ + struct avs_notify_mod_data mod_data; + union avs_notify_msg msg = AVS_MSG(header); + size_t data_size = 0; + void *data = NULL; + + if (!adev->ipc->ready && msg.notify_msg_type != AVS_NOTIFY_FW_READY) { + dev_dbg(adev->dev, "FW not ready, skip notification: 0x%08x\n", + msg.primary); + return; + } + + /* Calculate notification payload size. */ + switch (msg.notify_msg_type) { + case AVS_NOTIFY_FW_READY: + break; + + case AVS_NOTIFY_PHRASE_DETECTED: + data_size = sizeof(struct avs_notify_voice_data); + break; + + case AVS_NOTIFY_RESOURCE_EVENT: + data_size = sizeof(struct avs_notify_res_data); + break; + + case AVS_NOTIFY_MODULE_EVENT: + memcpy_fromio(&mod_data, avs_uplink_addr(adev), sizeof(mod_data)); + data_size = sizeof(mod_data) + mod_data.data_size; + break; + + default: + dev_warn(adev->dev, "unknown notification: 0x%08x\n", + msg.primary); + break; + } + + if (data_size) { + data = kmalloc(data_size, GFP_KERNEL); + if (!data) + return; + + memcpy_fromio(data, avs_uplink_addr(adev), data_size); + } + + /* Perform notification-specific operations. */ + switch (msg.notify_msg_type) { + case AVS_NOTIFY_FW_READY: + dev_dbg(adev->dev, "FW READY 0x%08x\n", msg.primary); + adev->ipc->ready = true; + complete(&adev->fw_ready); + break; + + default: + break; + } + + kfree(data); +} + +void avs_dsp_process_response(struct avs_dev *adev, u64 header) +{ + struct avs_ipc *ipc = adev->ipc; + + if (avs_msg_is_reply(header)) { + /* Response processing is invoked from IRQ thread. */ + spin_lock_irq(&ipc->rx_lock); + avs_dsp_receive_rx(adev, header); + ipc->rx_completed = true; + spin_unlock_irq(&ipc->rx_lock); + } else { + avs_dsp_process_notification(adev, header); + } + + complete(&ipc->busy_completion); +} + +irqreturn_t avs_dsp_irq_handler(int irq, void *dev_id) +{ + struct avs_dev *adev = dev_id; + struct avs_ipc *ipc = adev->ipc; + const struct avs_spec *const spec = adev->spec; + u32 adspis, hipc_rsp, hipc_ack; + irqreturn_t ret = IRQ_NONE; + + adspis = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS); + if (adspis == UINT_MAX || !(adspis & AVS_ADSP_ADSPIS_IPC)) + return ret; + + hipc_ack = snd_hdac_adsp_readl(adev, spec->hipc_ack_offset); + hipc_rsp = snd_hdac_adsp_readl(adev, spec->hipc_rsp_offset); + + /* DSP acked host's request */ + if (hipc_ack & spec->hipc_ack_done_mask) { + /* mask done interrupt */ + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, + AVS_ADSP_HIPCCTL_DONE, 0); + + complete(&ipc->done_completion); + + /* tell DSP it has our attention */ + snd_hdac_adsp_updatel(adev, spec->hipc_ack_offset, + spec->hipc_ack_done_mask, + spec->hipc_ack_done_mask); + /* unmask done interrupt */ + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, + AVS_ADSP_HIPCCTL_DONE, + AVS_ADSP_HIPCCTL_DONE); + ret = IRQ_HANDLED; + } + + /* DSP sent new response to process */ + if (hipc_rsp & spec->hipc_rsp_busy_mask) { + /* mask busy interrupt */ + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, + AVS_ADSP_HIPCCTL_BUSY, 0); + + ret = IRQ_WAKE_THREAD; + } + + return ret; +} + +irqreturn_t avs_dsp_irq_thread(int irq, void *dev_id) +{ + struct avs_dev *adev = dev_id; + union avs_reply_msg msg; + u32 hipct, hipcte; + + hipct = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCT); + hipcte = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCTE); + + /* ensure DSP sent new response to process */ + if (!(hipct & SKL_ADSP_HIPCT_BUSY)) + return IRQ_NONE; + + msg.primary = hipct; + msg.ext.val = hipcte; + avs_dsp_process_response(adev, msg.val); + + /* tell DSP we accepted its message */ + snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCT, + SKL_ADSP_HIPCT_BUSY, SKL_ADSP_HIPCT_BUSY); + /* unmask busy interrupt */ + snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCCTL, + AVS_ADSP_HIPCCTL_BUSY, AVS_ADSP_HIPCCTL_BUSY); + + return IRQ_HANDLED; +} + +static bool avs_ipc_is_busy(struct avs_ipc *ipc) +{ + struct avs_dev *adev = to_avs_dev(ipc->dev); + const struct avs_spec *const spec = adev->spec; + u32 hipc_rsp; + + hipc_rsp = snd_hdac_adsp_readl(adev, spec->hipc_rsp_offset); + return hipc_rsp & spec->hipc_rsp_busy_mask; +} + +static int avs_ipc_wait_busy_completion(struct avs_ipc *ipc, int timeout) +{ + int ret; + +again: + ret = wait_for_completion_timeout(&ipc->busy_completion, + msecs_to_jiffies(timeout)); + /* + * DSP could be unresponsive at this point e.g. manifested by + * EXCEPTION_CAUGHT notification. If so, no point in continuing. + */ + if (!ipc->ready) + return -EPERM; + + if (!ret) { + if (!avs_ipc_is_busy(ipc)) + return -ETIMEDOUT; + /* + * Firmware did its job, either notification or reply + * has been received - now wait until it's processed. + */ + wait_for_completion_killable(&ipc->busy_completion); + } + + /* Ongoing notification's bottom-half may cause early wakeup */ + spin_lock(&ipc->rx_lock); + if (!ipc->rx_completed) { + /* Reply delayed due to notification. */ + reinit_completion(&ipc->busy_completion); + spin_unlock(&ipc->rx_lock); + goto again; + } + + spin_unlock(&ipc->rx_lock); + return 0; +} + +static void avs_ipc_msg_init(struct avs_ipc *ipc, struct avs_ipc_msg *reply) +{ + lockdep_assert_held(&ipc->rx_lock); + + ipc->rx.header = 0; + ipc->rx.size = reply ? reply->size : 0; + ipc->rx_completed = false; + + reinit_completion(&ipc->done_completion); + reinit_completion(&ipc->busy_completion); +} + +static void avs_dsp_send_tx(struct avs_dev *adev, struct avs_ipc_msg *tx) +{ + const struct avs_spec *const spec = adev->spec; + + tx->header |= spec->hipc_req_busy_mask; + + if (tx->size) + memcpy_toio(avs_downlink_addr(adev), tx->data, tx->size); + snd_hdac_adsp_writel(adev, spec->hipc_req_ext_offset, tx->header >> 32); + snd_hdac_adsp_writel(adev, spec->hipc_req_offset, tx->header & UINT_MAX); +} + +static int avs_dsp_do_send_msg(struct avs_dev *adev, struct avs_ipc_msg *request, + struct avs_ipc_msg *reply, int timeout) +{ + struct avs_ipc *ipc = adev->ipc; + int ret; + + if (!ipc->ready) + return -EPERM; + + mutex_lock(&ipc->msg_mutex); + + spin_lock(&ipc->rx_lock); + avs_ipc_msg_init(ipc, reply); + avs_dsp_send_tx(adev, request); + spin_unlock(&ipc->rx_lock); + + ret = avs_ipc_wait_busy_completion(ipc, timeout); + if (ret) { + if (ret == -ETIMEDOUT) { + dev_crit(adev->dev, "communication severed: %d, rebooting dsp..\n", + ret); + + avs_ipc_block(ipc); + } + goto exit; + } + + ret = ipc->rx.rsp.status; + if (reply) { + reply->header = ipc->rx.header; + if (reply->data && ipc->rx.size) + memcpy(reply->data, ipc->rx.data, reply->size); + } + +exit: + mutex_unlock(&ipc->msg_mutex); + return ret; +} + +static int avs_dsp_send_msg_sequence(struct avs_dev *adev, + struct avs_ipc_msg *request, + struct avs_ipc_msg *reply, int timeout, + bool wake_d0i0, bool schedule_d0ix) +{ + return avs_dsp_do_send_msg(adev, request, reply, timeout); +} + +int avs_dsp_send_msg_timeout(struct avs_dev *adev, struct avs_ipc_msg *request, + struct avs_ipc_msg *reply, int timeout) +{ + return avs_dsp_send_msg_sequence(adev, request, reply, timeout, + false, false); +} + +int avs_dsp_send_msg(struct avs_dev *adev, struct avs_ipc_msg *request, + struct avs_ipc_msg *reply) +{ + return avs_dsp_send_msg_timeout(adev, request, reply, + adev->ipc->default_timeout_ms); +} + +int avs_dsp_send_pm_msg_timeout(struct avs_dev *adev, + struct avs_ipc_msg *request, + struct avs_ipc_msg *reply, int timeout, + bool wake_d0i0) +{ + return avs_dsp_send_msg_sequence(adev, request, reply, timeout, + wake_d0i0, false); +} + +int avs_dsp_send_pm_msg(struct avs_dev *adev, + struct avs_ipc_msg *request, + struct avs_ipc_msg *reply, bool wake_d0i0) +{ + return avs_dsp_send_pm_msg_timeout(adev, request, reply, + adev->ipc->default_timeout_ms, + wake_d0i0); +} + +static int avs_dsp_do_send_rom_msg(struct avs_dev *adev, struct avs_ipc_msg *request, + int timeout) +{ + struct avs_ipc *ipc = adev->ipc; + int ret; + + mutex_lock(&ipc->msg_mutex); + + spin_lock(&ipc->rx_lock); + avs_ipc_msg_init(ipc, NULL); + avs_dsp_send_tx(adev, request); + spin_unlock(&ipc->rx_lock); + + /* ROM messages must be sent before main core is unstalled */ + ret = avs_dsp_op(adev, stall, AVS_MAIN_CORE_MASK, false); + if (!ret) { + ret = wait_for_completion_timeout(&ipc->done_completion, + msecs_to_jiffies(timeout)); + ret = ret ? 0 : -ETIMEDOUT; + } + + mutex_unlock(&ipc->msg_mutex); + + return ret; +} + +int avs_dsp_send_rom_msg_timeout(struct avs_dev *adev, + struct avs_ipc_msg *request, int timeout) +{ + return avs_dsp_do_send_rom_msg(adev, request, timeout); +} + +int avs_dsp_send_rom_msg(struct avs_dev *adev, struct avs_ipc_msg *request) +{ + return avs_dsp_send_rom_msg_timeout(adev, request, + adev->ipc->default_timeout_ms); +} + +void avs_dsp_interrupt_control(struct avs_dev *adev, bool enable) +{ + const struct avs_spec *const spec = adev->spec; + u32 value; + + value = enable ? AVS_ADSP_ADSPIC_IPC : 0; + snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPIC, + AVS_ADSP_ADSPIC_IPC, value); + + value = enable ? AVS_ADSP_HIPCCTL_DONE : 0; + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, + AVS_ADSP_HIPCCTL_DONE, value); + + value = enable ? AVS_ADSP_HIPCCTL_BUSY : 0; + snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset, + AVS_ADSP_HIPCCTL_BUSY, value); +} + +int avs_ipc_init(struct avs_ipc *ipc, struct device *dev) +{ + ipc->rx.data = devm_kzalloc(dev, AVS_MAILBOX_SIZE, GFP_KERNEL); + if (!ipc->rx.data) + return -ENOMEM; + + ipc->dev = dev; + ipc->ready = false; + ipc->default_timeout_ms = AVS_IPC_TIMEOUT_MS; + init_completion(&ipc->done_completion); + init_completion(&ipc->busy_completion); + spin_lock_init(&ipc->rx_lock); + mutex_init(&ipc->msg_mutex); + + return 0; +} + +void avs_ipc_block(struct avs_ipc *ipc) +{ + ipc->ready = false; +} diff --git a/sound/soc/intel/avs/messages.h b/sound/soc/intel/avs/messages.h new file mode 100644 index 000000000000..003e634f5547 --- /dev/null +++ b/sound/soc/intel/avs/messages.h @@ -0,0 +1,170 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright(c) 2021 Intel Corporation. All rights reserved. + * + * Authors: Cezary Rojewski <cezary.rojewski@intel.com> + * Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> + */ + +#ifndef __SOUND_SOC_INTEL_AVS_MSGS_H +#define __SOUND_SOC_INTEL_AVS_MSGS_H + +struct avs_dev; + +#define AVS_MAILBOX_SIZE 4096 + +enum avs_msg_target { + AVS_FW_GEN_MSG = 0, + AVS_MOD_MSG = 1 +}; + +enum avs_msg_direction { + AVS_MSG_REQUEST = 0, + AVS_MSG_REPLY = 1 +}; + +enum avs_global_msg_type { + AVS_GLB_NOTIFICATION = 27, +}; + +union avs_global_msg { + u64 val; + struct { + union { + u32 primary; + struct { + u32 rsvd:24; + u32 global_msg_type:5; + u32 msg_direction:1; + u32 msg_target:1; + }; + }; + union { + u32 val; + } ext; + }; +} __packed; + +struct avs_tlv { + u32 type; + u32 length; + u32 value[]; +} __packed; + +union avs_module_msg { + u64 val; + struct { + union { + u32 primary; + struct { + u32 module_id:16; + u32 instance_id:8; + u32 module_msg_type:5; + u32 msg_direction:1; + u32 msg_target:1; + }; + }; + union { + u32 val; + } ext; + }; +} __packed; + +union avs_reply_msg { + u64 val; + struct { + union { + u32 primary; + struct { + u32 status:24; + u32 global_msg_type:5; + u32 msg_direction:1; + u32 msg_target:1; + }; + }; + union { + u32 val; + } ext; + }; +} __packed; + +enum avs_notify_msg_type { + AVS_NOTIFY_PHRASE_DETECTED = 4, + AVS_NOTIFY_RESOURCE_EVENT = 5, + AVS_NOTIFY_FW_READY = 8, + AVS_NOTIFY_MODULE_EVENT = 12, +}; + +union avs_notify_msg { + u64 val; + struct { + union { + u32 primary; + struct { + u32 rsvd:16; + u32 notify_msg_type:8; + u32 global_msg_type:5; + u32 msg_direction:1; + u32 msg_target:1; + }; + }; + union { + u32 val; + } ext; + }; +} __packed; + +#define AVS_MSG(hdr) { .val = hdr } + +#define AVS_GLOBAL_REQUEST(msg_type) \ +{ \ + .global_msg_type = AVS_GLB_##msg_type, \ + .msg_direction = AVS_MSG_REQUEST, \ + .msg_target = AVS_FW_GEN_MSG, \ +} + +#define AVS_MODULE_REQUEST(msg_type) \ +{ \ + .module_msg_type = AVS_MOD_##msg_type, \ + .msg_direction = AVS_MSG_REQUEST, \ + .msg_target = AVS_MOD_MSG, \ +} + +#define AVS_NOTIFICATION(msg_type) \ +{ \ + .notify_msg_type = AVS_NOTIFY_##msg_type,\ + .global_msg_type = AVS_GLB_NOTIFICATION,\ + .msg_direction = AVS_MSG_REPLY, \ + .msg_target = AVS_FW_GEN_MSG, \ +} + +#define avs_msg_is_reply(hdr) \ +({ \ + union avs_reply_msg __msg = AVS_MSG(hdr); \ + __msg.msg_direction == AVS_MSG_REPLY && \ + __msg.global_msg_type != AVS_GLB_NOTIFICATION; \ +}) + +/* Notification types */ + +struct avs_notify_voice_data { + u16 kpd_score; + u16 reserved; +} __packed; + +struct avs_notify_res_data { + u32 resource_type; + u32 resource_id; + u32 event_type; + u32 reserved; + u32 data[6]; +} __packed; + +struct avs_notify_mod_data { + u32 module_instance_id; + u32 event_id; + u32 data_size; + u32 data[]; +} __packed; + +#endif /* __SOUND_SOC_INTEL_AVS_MSGS_H */ diff --git a/sound/soc/intel/avs/registers.h b/sound/soc/intel/avs/registers.h index e0b6c8ffe633..c1db10269c62 100644 --- a/sound/soc/intel/avs/registers.h +++ b/sound/soc/intel/avs/registers.h @@ -12,6 +12,11 @@ /* Intel HD Audio General DSP Registers */ #define AVS_ADSP_GEN_BASE 0x0 #define AVS_ADSP_REG_ADSPCS (AVS_ADSP_GEN_BASE + 0x04) +#define AVS_ADSP_REG_ADSPIC (AVS_ADSP_GEN_BASE + 0x08) +#define AVS_ADSP_REG_ADSPIS (AVS_ADSP_GEN_BASE + 0x0C) + +#define AVS_ADSP_ADSPIC_IPC BIT(0) +#define AVS_ADSP_ADSPIS_IPC BIT(0) #define AVS_ADSPCS_CRST_MASK(cm) (cm) #define AVS_ADSPCS_CSTALL_MASK(cm) ((cm) << 8) @@ -19,4 +24,44 @@ #define AVS_ADSPCS_CPA_MASK(cm) ((cm) << 24) #define AVS_MAIN_CORE_MASK BIT(0) +#define AVS_ADSP_HIPCCTL_BUSY BIT(0) +#define AVS_ADSP_HIPCCTL_DONE BIT(1) + +/* SKL Intel HD Audio Inter-Processor Communication Registers */ +#define SKL_ADSP_IPC_BASE 0x40 +#define SKL_ADSP_REG_HIPCT (SKL_ADSP_IPC_BASE + 0x00) +#define SKL_ADSP_REG_HIPCTE (SKL_ADSP_IPC_BASE + 0x04) +#define SKL_ADSP_REG_HIPCI (SKL_ADSP_IPC_BASE + 0x08) +#define SKL_ADSP_REG_HIPCIE (SKL_ADSP_IPC_BASE + 0x0C) +#define SKL_ADSP_REG_HIPCCTL (SKL_ADSP_IPC_BASE + 0x10) + +#define SKL_ADSP_HIPCI_BUSY BIT(31) +#define SKL_ADSP_HIPCIE_DONE BIT(30) +#define SKL_ADSP_HIPCT_BUSY BIT(31) + +/* Constants used when accessing SRAM, space shared with firmware */ +#define AVS_FW_REG_BASE(adev) ((adev)->spec->sram_base_offset) +#define AVS_FW_REG_STATUS(adev) (AVS_FW_REG_BASE(adev) + 0x0) +#define AVS_FW_REG_ERROR_CODE(adev) (AVS_FW_REG_BASE(adev) + 0x4) + +#define AVS_FW_REGS_SIZE PAGE_SIZE +#define AVS_FW_REGS_WINDOW 0 +/* DSP -> HOST communication window */ +#define AVS_UPLINK_WINDOW AVS_FW_REGS_WINDOW +/* HOST -> DSP communication window */ +#define AVS_DOWNLINK_WINDOW 1 + +/* registry I/O helpers */ +#define avs_sram_offset(adev, window_idx) \ + ((adev)->spec->sram_base_offset + \ + (adev)->spec->sram_window_size * (window_idx)) + +#define avs_sram_addr(adev, window_idx) \ + ((adev)->adsp_ba + avs_sram_offset(adev, window_idx)) + +#define avs_uplink_addr(adev) \ + (avs_sram_addr(adev, AVS_UPLINK_WINDOW) + AVS_FW_REGS_SIZE) +#define avs_downlink_addr(adev) \ + avs_sram_addr(adev, AVS_DOWNLINK_WINDOW) + #endif /* __SOUND_SOC_INTEL_AVS_REGS_H */