Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 src/drivers/host/timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ uint64_t platform_timer_get(struct timer *timer)
return 0;
}

uint64_t platform_timer_get_noirq(struct timer *timer)
{
return 0;
}

void platform_timer_stop(struct timer *timer)
{
}
6 changes: 6 additions & 0 deletions src/drivers/imx/timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ uint64_t platform_timer_get(struct timer *timer)
return arch_timer_get_system(timer);
}

/* IRQs off in arch_timer_get_system() */
uint64_t platform_timer_get_noirq(struct timer *timer)
{
return arch_timer_get_system(timer);
}

/* get timestamp for host stream DMA position */
void platform_host_timestamp(struct comp_dev *host,
struct sof_ipc_stream_posn *posn)
Expand Down
5 changes: 5 additions & 0 deletions src/drivers/intel/baytrail/timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,11 @@ uint64_t platform_timer_get(struct timer *timer)
return time;
}

uint64_t platform_timer_get_noirq(struct timer *timer)
{
return platform_timer_get(timer);
}

/* get timestamp for host stream DMA position */
void platform_host_timestamp(struct comp_dev *host,
struct sof_ipc_stream_posn *posn)
Expand Down
5 changes: 5 additions & 0 deletions src/drivers/intel/cavs/idc.c
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,11 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode)

case IDC_POWER_UP:
ret = idc_wait_in_blocking_mode(msg->core, idc_is_powered_up);
if (ret < 0) {
tr_err(&idc_tr, "idc_send_msg(), power up core %d failed, reason 0x%x",
msg->core,
mailbox_sw_reg_read(PLATFORM_TRACEP_SECONDARY_CORE(msg->core)));
}
break;
}

Expand Down
66 changes: 59 additions & 7 deletions src/drivers/intel/cavs/timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,22 +36,43 @@ void platform_timer_stop(struct timer *timer)
shim_read(SHIM_DSPWCTCS) & ~SHIM_DSPWCTCS_T0A);
}

/* clock ticks it takes to set clock and enable IRQ */
#define TIMER_OVERHEAD 500

/* global timer lock */
//static spinlock_t timer_lock = {0};

int64_t platform_timer_set(struct timer *timer, uint64_t ticks)
{
uint64_t ticks_now;
uint32_t flags;

/* a tick value of 0 will not generate an IRQ */
if (ticks == 0)
ticks = 1;

/* Check if requested time is not past time */
if (ticks > shim_read64(SHIM_DSPWC))
irq_local_disable(flags);

ticks_now = platform_timer_get(timer);

/* Check if requested time is not past time and include the
* overhead of changing the timer
*/
if (ticks > ticks_now + TIMER_OVERHEAD) {
shim_write64(SHIM_DSPWCT0C, ticks);
} else {
ticks = ticks_now + TIMER_MIN_RECOVER_CYCLES;
if (ticks == 0)
ticks = 1;

shim_write64(SHIM_DSPWCT0C, ticks);
else
shim_write64(SHIM_DSPWCT0C, shim_read64(SHIM_DSPWC) +
TIMER_MIN_RECOVER_CYCLES);
}

/* Enable IRQ */
shim_write(SHIM_DSPWCTCS, SHIM_DSPWCTCS_T0A);

irq_local_enable(flags);

return shim_read64(SHIM_DSPWCT0C);
}

Expand All @@ -63,8 +84,39 @@ void platform_timer_clear(struct timer *timer)

uint64_t platform_timer_get(struct timer *timer)
{
// return arch_timer_get_system(timer);
return (uint64_t)shim_read64(SHIM_DSPWC);
uint32_t hi0;
uint32_t hi1;
uint32_t lo;
uint64_t ticks_now;

/* 64bit reads are non atomic on xtensa so we must
* read a stable value where there is no bit 32 flipping.
* A large delta between reads[0..1] means we have flipped
* and that the value read back in either 0..1] is invalid.
*/
do {
hi0 = shim_read(SHIM_DSPWCH);
lo = shim_read(SHIM_DSPWCL);
hi1 = shim_read(SHIM_DSPWCH);

/* worst case is we perform this twice so 6 * 32b clock reads */
} while (hi0 != hi1);

ticks_now = (((uint64_t)hi0) << 32) | lo;

return ticks_now;
}

uint64_t platform_timer_get_noirq(struct timer *timer)
{
uint32_t flags;
uint64_t ticks_now;

irq_local_disable(flags);
ticks_now = platform_timer_get(timer);
irq_local_enable(flags);

return ticks_now;
}

/* get timestamp for host stream DMA position */
Expand Down
6 changes: 6 additions & 0 deletions src/drivers/intel/haswell/timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ uint64_t platform_timer_get(struct timer *timer)
return arch_timer_get_system(timer);
}

/* IRQs off in arch_timer_get_system() */
uint64_t platform_timer_get_noirq(struct timer *timer)
{
return arch_timer_get_system(timer);
}

/* get timestamp for host stream DMA position */
void platform_host_timestamp(struct comp_dev *host,
struct sof_ipc_stream_posn *posn)
Expand Down
26 changes: 15 additions & 11 deletions src/idc/idc.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <sof/lib/notifier.h>
#include <sof/lib/uuid.h>
#include <sof/platform.h>
#include <arch/lib/wait.h>
#include <sof/schedule/edf_schedule.h>
#include <sof/schedule/ll_schedule.h>
#include <sof/schedule/schedule.h>
Expand Down Expand Up @@ -93,19 +94,22 @@ int idc_wait_in_blocking_mode(uint32_t target_core, bool (*cond)(int))
IDC_TIMEOUT / 1000;

while (!cond(target_core)) {
if (deadline < platform_timer_get(timer)) {
/* safe check in case we've got preempted
* after read
*/
if (cond(target_core))
break;

tr_err(&idc_tr, "idc_wait_in_blocking_mode() error: timeout");
return -ETIME;
}

/* spin here so other core can access IO and timers freely */
idelay(8192);
Copy link
Member

Choose a reason for hiding this comment

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

After quick debug check this looks more complex, actually it is booting fine (trace point 4000 for secondary core too) and entered task_main_secondary_core() successfully

Copy link
Member Author

@lgirdwood lgirdwood Jan 22, 2021

Choose a reason for hiding this comment

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

yeah, was thinking that too - we now have aloop here that lock and unlocks (with IRQs OFF) a lot. We need to clean this up so we

  1. keep the relax
  2. split the platform_timer_get() into locked and unlocked versions (scheduler uses locked, trace uses unlocked). This will use unlocked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fwiw, the mailbox and timer IP reads are on a shared bus (that all cores must use), so this should actually speed up other cores booting since the IO bus has less traffic to block other core IO.

Copy link
Member Author

Choose a reason for hiding this comment

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

@abonislawski difficult to see this one in the logs ? We could have a race here as I've seen red/green in the reulst on different runs. Added more trace to show any errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

@slawblauciak @abonislawski there is nothing in the logs showing why playback fails on the multicore tests. I can only assume we have something fishy going on with the locks - disabling for CI validation.

Copy link
Member Author

Choose a reason for hiding this comment

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

All multicore/multkernel test pass when the locking is removed ! Lets try per core locking as we should only ever have one user of the timer.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mwasko @slawblauciak @abonislawski @keyonjie fyi - this now passes CI now. Multicore would race and mainly show red on CI when IRQs were globally OFF for timer get/set. I will cleanup and merge it tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

guys I've kept recover cycles high here at original value. We can optimise as more debug info is collected.


if (deadline < platform_timer_get(timer))
break;
}

return 0;
/* safe check in case we've got preempted
* after read
*/
if (cond(target_core))
return 0;

tr_err(&idc_tr, "idc_wait_in_blocking_mode() error: timeout");
return -ETIME;
}

/**
Expand Down
4 changes: 4 additions & 0 deletions src/include/sof/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -146,5 +146,9 @@
#endif /* __XCC__ */

#endif /* __ASSEMBLER__ */
#else /* LINKER */

#define ALIGN_UP_INTERNAL(val, align) (((val) + (align) - 1) & ~((align) - 1))

#endif /* LINKER */
#endif /* __SOF_COMMON_H__ */
1 change: 1 addition & 0 deletions src/include/sof/drivers/timer.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ static inline uint64_t timer_get_system(struct timer *timer)
int64_t platform_timer_set(struct timer *timer, uint64_t ticks);
void platform_timer_clear(struct timer *timer);
uint64_t platform_timer_get(struct timer *timer);
uint64_t platform_timer_get_noirq(struct timer *timer);
void platform_timer_start(struct timer *timer);
void platform_timer_stop(struct timer *timer);

Expand Down
2 changes: 2 additions & 0 deletions src/platform/apollolake/include/platform/lib/shim.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@

/* DSP Shim Registers */
#define SHIM_DSPWC 0x20 /* DSP Wall Clock */
#define SHIM_DSPWCL 0x20 /* DSP Wall Clock Low */
#define SHIM_DSPWCH 0x24 /* DSP Wall Clock High */
#define SHIM_DSPWCTCS 0x28 /* DSP Wall Clock Timer Control & Status */
#define SHIM_DSPWCT0C 0x30 /* DSP Wall Clock Timer 0 Compare */
#define SHIM_DSPWCT1C 0x38 /* DSP Wall Clock Timer 1 Compare */
Expand Down
2 changes: 2 additions & 0 deletions src/platform/cannonlake/include/platform/lib/shim.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@

/* DSP Shim Registers */
#define SHIM_DSPWC 0x20 /* DSP Wall Clock */
#define SHIM_DSPWCL 0x20 /* DSP Wall Clock Low */
#define SHIM_DSPWCH 0x24 /* DSP Wall Clock High */
#define SHIM_DSPWCTCS 0x28 /* DSP Wall Clock Timer Control & Status */
#define SHIM_DSPWCT0C 0x30 /* DSP Wall Clock Timer 0 Compare */
#define SHIM_DSPWCT1C 0x38 /* DSP Wall Clock Timer 1 Compare */
Expand Down
2 changes: 2 additions & 0 deletions src/platform/icelake/include/platform/lib/shim.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@

/* DSP Shim Registers */
#define SHIM_DSPWC 0x20 /* DSP Wall Clock */
#define SHIM_DSPWCL 0x20 /* DSP Wall Clock Low */
#define SHIM_DSPWCH 0x24 /* DSP Wall Clock High */
#define SHIM_DSPWCTCS 0x28 /* DSP Wall Clock Timer Control & Status */
#define SHIM_DSPWCT0C 0x30 /* DSP Wall Clock Timer 0 Compare */
#define SHIM_DSPWCT1C 0x38 /* DSP Wall Clock Timer 1 Compare */
Expand Down
2 changes: 2 additions & 0 deletions src/platform/intel/cavs/lib/pm_memory.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ DECLARE_SOF_UUID("pm-memory", pm_mem_uuid, 0x14f25ab6, 0x3a4b, 0x4e5d,

DECLARE_TR_CTX(pm_mem_tr, SOF_UUID(pm_mem_uuid), LOG_LEVEL_INFO);

#if (CAVS_VERSION >= CAVS_VERSION_1_8) || CONFIG_LP_SRAM
/**
* \brief Retrieves memory banks based on start and end pointer.
* \param[in,out] start Start address of memory range.
Expand Down Expand Up @@ -55,6 +56,7 @@ static void memory_banks_get(void *start, void *end, uint32_t base,
*/
*end_bank = ((uintptr_t)end - base) / SRAM_BANK_SIZE - 1;
}
#endif

#if CAVS_VERSION >= CAVS_VERSION_1_8

Expand Down
2 changes: 1 addition & 1 deletion src/platform/suecreek/include/platform/lib/memory.h
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@

/* code loader entry point for base fw */
#define _SRAM_VECBASE_RESET (BOOT_LDR_BSS_BASE + BOOT_LDR_BSS_SIZE)
#define SRAM_VECBASE_RESET ALIGN(_SRAM_VECBASE_RESET,4096)
#define SRAM_VECBASE_RESET ALIGN_UP_INTERNAL(_SRAM_VECBASE_RESET, 4096)

#endif /* __PLATFORM_LIB_MEMORY_H__ */

Expand Down
2 changes: 2 additions & 0 deletions src/platform/suecreek/include/platform/lib/shim.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@

/* DSP Shim Registers */
#define SHIM_DSPWC 0x20 /* DSP Wall Clock */
#define SHIM_DSPWCL 0x20 /* DSP Wall Clock Low */
#define SHIM_DSPWCH 0x24 /* DSP Wall Clock High */
#define SHIM_DSPWCTCS 0x28 /* DSP Wall Clock Timer Control & Status */
#define SHIM_DSPWCT0C 0x30 /* DSP Wall Clock Timer 0 Compare */
#define SHIM_DSPWCT1C 0x38 /* DSP Wall Clock Timer 1 Compare */
Expand Down
2 changes: 2 additions & 0 deletions src/platform/tigerlake/include/platform/lib/shim.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@

/* DSP Shim Registers */
#define SHIM_DSPWC 0x20 /* DSP Wall Clock */
#define SHIM_DSPWCL 0x20 /* DSP Wall Clock Low */
#define SHIM_DSPWCH 0x24 /* DSP Wall Clock High */
#define SHIM_DSPWCTCS 0x28 /* DSP Wall Clock Timer Control & Status */
#define SHIM_DSPWCT0C 0x30 /* DSP Wall Clock Timer 0 Compare */
#define SHIM_DSPWCT1C 0x38 /* DSP Wall Clock Timer 1 Compare */
Expand Down
2 changes: 1 addition & 1 deletion src/schedule/dma_multi_chan_domain.c
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ static bool dma_multi_chan_domain_is_pending(struct ll_schedule_domain *domain,
/* it's too soon for this task */
if (!pipe_task->registrable &&
pipe_task->task.start >
platform_timer_get(timer_get()))
platform_timer_get_noirq(timer_get()))
continue;

notifier_event(&dmas[i].chan[j], NOTIFIER_ID_DMA_IRQ,
Expand Down
4 changes: 2 additions & 2 deletions src/schedule/dma_single_chan_domain.c
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ static void dma_single_chan_domain_set(struct ll_schedule_domain *domain,
goto out;

if (dma_domain->channel_changed) {
domain->last_tick = platform_timer_get(timer_get());
domain->last_tick = platform_timer_get_noirq(timer_get());

dma_domain->channel_changed = false;
} else {
Expand Down Expand Up @@ -517,7 +517,7 @@ static void dma_single_chan_domain_clear(struct ll_schedule_domain *domain)
static bool dma_single_chan_domain_is_pending(struct ll_schedule_domain *domain,
struct task *task, struct comp_dev **comp)
{
return task->start <= platform_timer_get(timer_get());
return task->start <= platform_timer_get_noirq(timer_get());
}

/**
Expand Down
8 changes: 4 additions & 4 deletions src/schedule/ll_schedule.c
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ static int schedule_ll_domain_set(struct ll_schedule_data *sch,
total = atomic_add(&sch->domain->total_num_tasks, 1);
if (total == 0)
/* First task in domain over all cores: actiivate it */
domain_set(sch->domain, platform_timer_get(timer_get()));
domain_set(sch->domain, platform_timer_get_noirq(timer_get()));

if (total == 0 || !registered) {
/* First task on core: count and enable it */
Expand Down Expand Up @@ -366,7 +366,7 @@ static int schedule_ll_task(void *data, struct task *task, uint64_t start,
task->start = sch->domain->ticks_per_ms * start / 1000;

if (sch->domain->synchronous)
task->start += platform_timer_get(timer_get());
task->start += platform_timer_get_noirq(timer_get());
else
task->start += sch->domain->last_tick;

Expand Down Expand Up @@ -467,7 +467,7 @@ static int reschedule_ll_task(void *data, struct task *task, uint64_t start)
time = sch->domain->ticks_per_ms * start / 1000;

if (sch->domain->synchronous)
time += platform_timer_get(timer_get());
time += platform_timer_get_noirq(timer_get());
else
time += sch->domain->last_tick;

Expand Down Expand Up @@ -514,7 +514,7 @@ static void scheduler_free_ll(void *data)
static void ll_scheduler_recalculate_tasks(struct ll_schedule_data *sch,
struct clock_notify_data *clk_data)
{
uint64_t current = platform_timer_get(timer_get());
uint64_t current = platform_timer_get_noirq(timer_get());
struct list_item *tlist;
struct task *task;
uint64_t delta_ms;
Expand Down
4 changes: 2 additions & 2 deletions src/schedule/timer_domain.c
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ static void timer_domain_set(struct ll_schedule_domain *domain, uint64_t start)

ticks_set = platform_timer_set(timer_domain->timer, ticks_req);
#else
uint64_t current = platform_timer_get(timer_domain->timer);
uint64_t current = platform_timer_get_noirq(timer_domain->timer);
uint64_t earliest_next = current + 1 + ZEPHYR_SCHED_COST;
uint64_t ticks_req = domain->last_tick ? start + ticks_tout :
MAX(start, earliest_next);
Expand Down Expand Up @@ -302,7 +302,7 @@ static void timer_domain_clear(struct ll_schedule_domain *domain)
static bool timer_domain_is_pending(struct ll_schedule_domain *domain,
struct task *task, struct comp_dev **comp)
{
return task->start <= platform_timer_get(timer_get());
return task->start <= platform_timer_get_noirq(timer_get());
}

struct ll_schedule_domain *timer_domain_init(struct timer *timer, int clk,
Expand Down