Skip to content

adsp: ace: ipc: boot: power: watchdog: A set of changes required by the watchdog #53840

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

Merged
merged 3 commits into from
Feb 20, 2023
+49 −20
Merged

adsp: ace: ipc: boot: power: watchdog: A set of changes required by the watchdog #53840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba27d17
adsp: ace: Non-maskable interrupt handling
softwarecki Dec 27, 2022
229d832
adsp: ipc: Emergency ipc message send
softwarecki Dec 29, 2022
e55d530
adsp: boot: power: Separate watchdog state from core power
softwarecki Jan 9, 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
15 changes: 5 additions & 10 deletions soc/xtensa/intel_adsp/ace/_soc_inthandlers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,12 @@ static inline int _xtensa_handle_one_int3(unsigned int mask)

static inline int _xtensa_handle_one_int5(unsigned int mask)
{
int irq;

if (mask & BIT(8)) {
mask = BIT(8);
irq = 8;
goto handle_irq;
}
/* It is a Non-maskable interrupt handler.
* The non-maskable interrupt have no corresponding bit in INTERRUPT and INTENABLE registers
* so mask parameter is always 0.
*/
_sw_isr_table[8].isr(_sw_isr_table[8].arg);
return 0;
handle_irq:
_sw_isr_table[irq].isr(_sw_isr_table[irq].arg);
return mask;
}

static inline int _xtensa_handle_one_int0(unsigned int mask)
Expand Down
3 changes: 0 additions & 3 deletions soc/xtensa/intel_adsp/ace/include/intel_ace15_mtpm/adsp_boot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ struct dspcs {
#define DSPBR_BATTR_LPSCTL_L1_MIN_WAY BIT(15)
#define DSPBR_BATTR_LPSCTL_BATTR_SLAVE_CORE BIT(16)

#define DSPBR_WDT_RESUME BIT(8)
#define DSPBR_WDT_RESTART_COMMAND 0x76

#define DSPCS (*(volatile struct dspcs *)DSPCS_REG)

#endif /* ZEPHYR_SOC_INTEL_ADSP_BOOT_H_ */
5 changes: 0 additions & 5 deletions soc/xtensa/intel_adsp/ace/multiprocessing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,6 @@ void soc_mp_startup(uint32_t cpu)
/* Prevent idle from powering us off */
DSPCS.bootctl[cpu].bctl |=
DSPBR_BCTL_WAITIPCG | DSPBR_BCTL_WAITIPPG;
/* checking if WDT was stopped during D3 transition */
if (DSPCS.bootctl[cpu].wdtcs & DSPBR_WDT_RESUME) {
DSPCS.bootctl[cpu].wdtcs = DSPBR_WDT_RESUME;
/* TODO: delete this IF when FW starts using imr restore vector */
}
}

void arch_sched_ipi(void)
Expand Down
2 changes: 0 additions & 2 deletions soc/xtensa/intel_adsp/ace/power.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,6 @@ __weak void pm_state_set(enum pm_state state, uint8_t substate_id)
core_desc[cpu].intenable = XTENSA_RSR("INTENABLE");
z_xt_ints_off(0xffffffff);
core_desc[cpu].bctl = DSPCS.bootctl[cpu].bctl;
DSPCS.bootctl[cpu].wdtcs = DSPBR_WDT_RESTART_COMMAND;
DSPCS.bootctl[cpu].bctl &= ~DSPBR_BCTL_WAITIPCG;
soc_cpus_active[cpu] = false;
z_xtensa_cache_flush_inv_all();
Expand Down Expand Up @@ -309,7 +308,6 @@ __weak void pm_state_exit_post_ops(enum pm_state state, uint8_t substate_id)
uint32_t cpu = arch_proc_id();

if (state == PM_STATE_SOFT_OFF) {
DSPCS.bootctl[cpu].wdtcs = DSPBR_WDT_RESUME;
/* restore clock gating state */
DSPCS.bootctl[cpu].bctl |=
(core_desc[0].bctl & DSPBR_BCTL_WAITIPCG);
Expand Down
13 changes: 13 additions & 0 deletions soc/xtensa/intel_adsp/common/include/intel_adsp_ipc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,4 +161,17 @@ bool intel_adsp_ipc_send_message(const struct device *dev,
bool intel_adsp_ipc_send_message_sync(const struct device *dev,
uint32_t data, uint32_t ext_data, k_timeout_t timeout);


/** @brief Send an emergency IPC message.
*
* Sends a message to the other side of an IPC link. The data and ext_data parameters are passed
* using the IDR/IDD registers. Waits in a loop until it is possible to send a message.
*
* @param dev IPC device.
* @param data 30 bits value to transmit with the message (IDR register).
* @param ext_data Extended value to transmit with the message (IDD register).
*/
void intel_adsp_ipc_send_message_emergency(const struct device *dev, uint32_t data,
uint32_t ext_data);

#endif /* ZEPHYR_INCLUDE_INTEL_ADSP_IPC_H */
31 changes: 31 additions & 0 deletions soc/xtensa/intel_adsp/common/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,37 @@ bool intel_adsp_ipc_send_message_sync(const struct device *dev,
return ret;
}

void intel_adsp_ipc_send_message_emergency(const struct device *dev, uint32_t data,
uint32_t ext_data)
{
const struct intel_adsp_ipc_config * const config = dev->config;

volatile struct intel_adsp_ipc * const regs = config->regs;
bool done;

/* check if host is processing message. */
while (regs->idr & INTEL_ADSP_IPC_BUSY) {
k_busy_wait(1);
Copy link
Member

@abonislawski abonislawski Jan 31, 2023

Choose a reason for hiding this comment

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

WAIT_FOR

Copy link
Contributor Author

@softwarecki softwarecki Feb 1, 2023

Choose a reason for hiding this comment

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

This is an emergency function used to send a notification about watchdog timeout on a primary core. We don't want any timeout here.

}

/* check if host has pending acknowledge msg
* Same signal, but on different bits in 1.5
*/
done = IS_ENABLED(CONFIG_SOC_INTEL_CAVS_V15) ? (regs->idd & INTEL_ADSP_IPC_DONE)
: (regs->ida & INTEL_ADSP_IPC_DONE);
if (done) {
/* IPC completion */
if (IS_ENABLED(CONFIG_SOC_INTEL_CAVS_V15)) {
regs->idd = INTEL_ADSP_IPC_DONE;
} else {
regs->ida = INTEL_ADSP_IPC_DONE;
}
}

regs->idd = ext_data;
Copy link
Member

@ceolin ceolin Feb 10, 2023

Choose a reason for hiding this comment

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

is it right ? this is overwriting the value set few lines above for regs->idd (CAVS_V15 build)

Copy link
Contributor Author

@softwarecki softwarecki Feb 10, 2023

Choose a reason for hiding this comment

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

Good question. I made it identical to the intel_adsp_ipc_send_message function, which uses these registers regardless of the platform. I suspect that setting this bit triggers message acknowledgment, and it doesn't need to stay set. Maybe @aborisovich can confirm this?

Copy link

@aborisovich aborisovich Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it looks like by analogy it should work, I'll look into CAVS15 registers asap.

Copy link

@aborisovich aborisovich Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so we have it partially wrong.
IDD register on CAVS1.5 is also DIPCIE register.
DIPCIE register has bit 30 used as done bit. When set, it means that ipc target (host here) had completed the operation. DIPCIE message extension is composed of 29 youngest bits.
So basically in this code, we override DIPCIE bits 30 and 31 using extended message what should not happen.
ext_data should be truncated from the left to contain only 29 youngest bits of the extended message for CAVS1.5.

We can of course assume that application logic (that provides extended message value) had prepared the message in the way that is suitable. Let's say application copied state of IDD 31 and 30 bit to the extended message and then wrote remaining 29 bits content. But this is not a good practice in my opinion.

It works for now so I guess we can merge this PR and provide some fixes in separate PR that would also fix the body of intel_adsp_ipc_send_message function.
Is this ok for you @ceolin ?

regs->idr = data | INTEL_ADSP_IPC_BUSY;
}

#if DT_NODE_EXISTS(INTEL_ADSP_IPC_HOST_DTNODE)

#if defined(CONFIG_SOC_SERIES_INTEL_ACE)
Expand Down