[Skiboot] [PATCH v7 2/2] SBE: Add timer support
Benjamin Herrenschmidt
benh at kernel.crashing.org
Tue Apr 17 14:18:30 AEST 2018
On Mon, 2018-04-16 at 17:18 +0530, Vasant Hegde wrote:
> SBE on P9 provides one shot programmable timer facility. We can use this
> to implement OPAL timers and hence limit the reliance on the Linux
> heartbeat (similar to HW timer facility provided by SLW on P8).
>
> Design:
> - We will continue to run Linux heartbeat.
> - Each chip has SBE. This patch always schedules timer on SBE on master chip.
> - Start timer option starts new timer or modifies an active timer for the
> specified timeout.
> - SBE expects timeout value in microseconds. We track timeout value in TB.
> Hence we convert tb to microseconds before sending request to SBE.
> - We are requesting ack from SBE for timer message. It gaurantees that
> SBE has scheduled timer.
> - Disabling SBE timer
> We expect SBE to send timer expiry interrupt whenever timer expires. We
> wait for 10 more ms before disabling timer.
> In future we can consider below alternative approaches:
> - Presently SBE timer disable is permanent (until we reboot system).
> SBE sends "I'm back" interrupt after reset. We can consider restarting
> timer after SBE reset.
> - Reset SBE and start timer again.
> - Each chip has SBE. On multi chip system we can try to schedule timer
> on different chip.
>
> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
> CC: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> ---
> core/interrupts.c | 3 +-
> core/test/run-timer.c | 8 ++
> core/timer.c | 17 ++++-
> hw/sbe-p9.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/sbe-p9.h | 6 ++
> 5 files changed, 235 insertions(+), 4 deletions(-)
>
> diff --git a/core/interrupts.c b/core/interrupts.c
> index 4452511f0..5d7a68cd5 100644
> --- a/core/interrupts.c
> +++ b/core/interrupts.c
> @@ -26,6 +26,7 @@
> #include <ccan/str/str.h>
> #include <timer.h>
> #include <sbe-p8.h>
> +#include <sbe-p9.h>
>
> /* ICP registers */
> #define ICP_XIRR 0x4 /* 32-bit access */
> @@ -489,7 +490,7 @@ static int64_t opal_handle_interrupt(uint32_t isn, __be64 *outstanding_event_mas
> is->ops->interrupt(is, isn);
>
> /* Check timers if SBE timer isn't working */
> - if (!p8_sbe_timer_ok())
> + if (!p8_sbe_timer_ok() && !p9_sbe_timer_ok())
> check_timers(true);
>
> /* Update output events */
> diff --git a/core/test/run-timer.c b/core/test/run-timer.c
> index 986af28d8..159e007ac 100644
> --- a/core/test/run-timer.c
> +++ b/core/test/run-timer.c
> @@ -4,12 +4,15 @@
>
> #define __TEST__
> #include <timer.h>
> +#include <skiboot.h>
>
> #define mftb() (stamp)
> #define sync()
> #define smt_lowest()
> #define smt_medium()
>
> +enum proc_gen proc_gen = proc_gen_p9;
> +
> static uint64_t stamp, last;
> struct lock;
> static inline void lock_caller(struct lock *l, const char *caller)
> @@ -53,6 +56,11 @@ void p8_sbe_update_timer_expiry(uint64_t new_target)
> /* FIXME: do intersting SLW timer sim */
> }
>
> +void p9_sbe_update_timer_expiry(uint64_t new_target)
> +{
> + (void)new_target;
> +}
> +
> int main(void)
> {
> unsigned int i;
> diff --git a/core/timer.c b/core/timer.c
> index 21f62a492..8eefb74be 100644
> --- a/core/timer.c
> +++ b/core/timer.c
> @@ -5,6 +5,7 @@
> #include <device.h>
> #include <opal.h>
> #include <sbe-p8.h>
> +#include <sbe-p9.h>
>
> #ifdef __TEST__
> #define this_cpu() ((void *)-1)
> @@ -109,8 +110,12 @@ static void __schedule_timer_at(struct timer *t, uint64_t when)
> bail:
> /* Pick up the next timer and upddate the SBE HW timer */
> lt = list_top(&timer_list, struct timer, link);
> - if (lt)
> - p8_sbe_update_timer_expiry(lt->target);
> + if (lt) {
> + if (proc_gen < proc_gen_p9)
> + p8_sbe_update_timer_expiry(lt->target);
> + else
> + p9_sbe_update_timer_expiry(lt->target);
> + }
> }
>
> void schedule_timer_at(struct timer *t, uint64_t when)
> @@ -167,7 +172,11 @@ static void __check_poll_timers(uint64_t now)
> * arbitrarily 1us.
> */
> if (t->running) {
> - p8_sbe_update_timer_expiry(now + usecs_to_tb(1));
> + if (proc_gen < proc_gen_p9)
> + p8_sbe_update_timer_expiry(now + usecs_to_tb(1));
> + else
> + p9_sbe_update_timer_expiry(now + usecs_to_tb(1));
> +
> break;
> }
>
> @@ -266,6 +275,8 @@ void late_init_timers(void)
> */
> if (platform.heartbeat_time) {
> heartbeat = platform.heartbeat_time();
> + } else if (p9_sbe_timer_ok()) {
> + heartbeat = HEARTBEAT_DEFAULT_MS * 10;
> } else if (p8_sbe_timer_ok() || fsp_present()) {
> heartbeat = HEARTBEAT_DEFAULT_MS * 10;
> }
> diff --git a/hw/sbe-p9.c b/hw/sbe-p9.c
> index 9dc487ba2..a5b0706ef 100644
> --- a/hw/sbe-p9.c
> +++ b/hw/sbe-p9.c
> @@ -53,6 +53,7 @@
> #include <sbe-p9.h>
> #include <skiboot.h>
> #include <timebase.h>
> +#include <timer.h>
> #include <trace.h>
> #include <xscom.h>
>
> @@ -80,11 +81,36 @@ struct p9_sbe {
> /* Default SBE chip ID */
> static int sbe_default_chip_id = -1;
>
> +/* Is SBE timer running? */
> +static bool sbe_has_timer = false;
> +static bool sbe_timer_in_progress = false;
> +
> +/* Inflight and next timer in TB */
> +static uint64_t sbe_last_gen_stamp;
> +static uint64_t sbe_timer_target;
> +
> +static bool has_new_target = false;
> +static bool got_timer_interrupt = false;
> +
> +/* Timer lock */
> +struct lock sbe_timer_lock;
> +
> +/*
> + * Minimum timeout value for P9 is 500 microseconds. After that
> + * SBE timer can handle granularity of 1 microsecond.
> + */
> +#define SBE_TIMER_DEFAULT_US 500
> +static uint64_t sbe_timer_def_tb;
> +
> +/* Timer control message */
> +static struct p9_sbe_msg *timer_ctrl_msg;
> +
> #define SBE_STATUS_PRI_SHIFT 0x30
> #define SBE_STATUS_SEC_SHIFT 0x20
>
> /* Forward declaration */
> static void p9_sbe_timeout_poll_one(struct p9_sbe *sbe);
> +static void p9_sbe_timer_schedule(void);
>
> /* bit 0-15 : Primary status code */
> static inline u16 p9_sbe_get_primary_rc(struct p9_sbe_msg *resp)
> @@ -562,6 +588,14 @@ static void __p9_sbe_interrupt(struct p9_sbe *sbe)
> p9_sbe_msg_complete(sbe, msg);
> }
>
> + /* Timer expired */
> + if (data & SBE_HOST_TIMER_EXPIRY) {
> + if (sbe->chip_id == sbe_default_chip_id) {
> + sbe_timer_in_progress = false;
> + got_timer_interrupt = true;
> + }
> + }
> +
> next_msg:
> p9_sbe_process_queue(sbe);
> }
> @@ -578,6 +612,15 @@ void p9_sbe_interrupt(uint32_t chip_id)
> sbe = chip->sbe;
> lock(&sbe->lock);
> __p9_sbe_interrupt(sbe);
> +
> + if (got_timer_interrupt) {
> + got_timer_interrupt = false;
> + /* Drop lock and call timers */
> + unlock(&sbe->lock);
> + check_timers(true);
> + return;
> + }
> +
> unlock(&sbe->lock);
> }
>
> @@ -634,6 +677,55 @@ out:
> unlock(&sbe->lock);
> }
>
> +/*
> + * Check if the timer is working. If at least 10ms elapsed since
> + * last scheduled timer expiry.
> + */
> +static void p9_sbe_timer_poll(struct p9_sbe *sbe)
> +{
> + if (!sbe_has_timer || !sbe_timer_in_progress)
> + return;
> +
> + lock(&sbe->lock);
> + if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(10))
> + != TB_AAFTERB)
> + goto out;
Do you need the lock for the above ?
> + /*
> + * In some cases there will be a delay in calling OPAL interrupt
> + * handler routine (opal_handle_interrupt). In such cases its
> + * possible that SBE has responded, but OPAL didn't act on that.
> + * Hence check for SBE response before disabling timer.
> + */
> + __p9_sbe_interrupt(sbe);
> + if (got_timer_interrupt)
> + goto call_timer;
Again, why do you call that here ?
You just called p9_sbe_timeout_poll_one() for that same SBE, which
already took the lock *and* called __p9_sbe_interrupt.
I would instead of a call-out from p9_sbe_timeout_poll_one() to check
timers.
> + if (!sbe_timer_in_progress)
> + goto out;
> +
> + if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(10))
> + != TB_AAFTERB)
> + goto out;
> +
> + prlog(PR_ERR, "Timer stuck, falling back to OPAL pollers.\n");
> + prlog(PR_ERR, "You will likely have slower I2C and may have "
> + "experienced increased jitter.\n");
> + p9_sbe_reg_dump(sbe_default_chip_id);
> + sbe_has_timer = false;
> + sbe_timer_in_progress = false;
> +
> +out:
> + unlock(&sbe->lock);
> + return;
> +
> +call_timer:
> + got_timer_interrupt = false;
> + /* Drop lock and call timers */
> + unlock(&sbe->lock);
> + check_timers(true);
> +}
> +
> static void p9_sbe_timeout_poll(void *user_data __unused)
> {
> struct p9_sbe *sbe;
> @@ -644,9 +736,119 @@ static void p9_sbe_timeout_poll(void *user_data __unused)
> continue;
> sbe = chip->sbe;
> p9_sbe_timeout_poll_one(sbe);
> + if (sbe->chip_id == sbe_default_chip_id)
> + p9_sbe_timer_poll(sbe);
> }
> }
>
> +static void p9_sbe_timer_resp(struct p9_sbe_msg *msg)
> +{
> + if (msg->state != sbe_msg_done) {
> + prlog(PR_DEBUG, "Failed to schedule timer [chip id %x]\n",
> + sbe_default_chip_id);
> +
> + if (!has_new_target)
> + return;
> + } else {
> + /* Update last scheduled timer value */
> + sbe_last_gen_stamp = mftb() +
> + usecs_to_tb(timer_ctrl_msg->reg[1]);
> + sbe_timer_in_progress = true;
> + }
The common path should be that we don't have a new target,
I would suggest movign the !has_new_target down here, before
the lock. To be race-free, we need to order it with the msg state, so
you do need a sync.
What about the lockless updates to sbe_last_gen_stamp and
sbe_timer_in_progress ? Is that safe?
> + lock(&sbe_timer_lock);
> + if (has_new_target) {
> + has_new_target = false;
> + p9_sbe_timer_schedule();
> + }
> + unlock(&sbe_timer_lock);
> +}
> +
> +static void p9_sbe_timer_schedule(void)
> +{
> + int rc;
> + u32 tick_us = SBE_TIMER_DEFAULT_US;
> + u64 tb_cnt, now = mftb();
> +
> + if (sbe_timer_in_progress) {
> + /* Remaining time of inflight timer <= sbe_timer_def_tb */
> + if ((sbe_last_gen_stamp - now) <= sbe_timer_def_tb)
> + return;
> + }
> +
> + if (now < sbe_timer_target) {
> + /* Calculate how many microseconds from now, rounded up */
> + if ((sbe_timer_target - now) > sbe_timer_def_tb) {
> + tb_cnt = sbe_timer_target - now + usecs_to_tb(1) - 1;
> + tick_us = tb_to_usecs(tb_cnt);
> + }
> + }
> +
> + /* Clear sequence number. p9_sbe_queue_msg will add new sequene ID */
> + timer_ctrl_msg->reg[0] &= ~(PPC_BITMASK(32, 47));
> + /* Update timeout value */
> + timer_ctrl_msg->reg[1] = tick_us;
> + rc = p9_sbe_queue_msg(sbe_default_chip_id, timer_ctrl_msg,
> + p9_sbe_timer_resp);
> + if (rc != OPAL_SUCCESS) {
> + prlog(PR_ERR, "Failed to start timer [chip id = %x]\n",
> + sbe_default_chip_id);
> + return;
> + }
> +}
> +
> +/*
> + * This is called with the timer lock held, so there is no
> + * issue with re-entrancy or concurrence
> + */
> +void p9_sbe_update_timer_expiry(uint64_t new_target)
> +{
> + u64 now = mftb();
> +
> + if (!sbe_has_timer || new_target == sbe_timer_target)
> + return;
> +
> + lock(&sbe_timer_lock);
> + /* Timer message is in flight. Record new timer and schedule later */
> + if (p9_sbe_msg_busy(timer_ctrl_msg) || has_new_target) {
> + if (new_target < now)
> + goto out;
Will the caller handle the case where we just passed the target ? Will
it re-check timers after calling us ?
> + if (new_target > sbe_timer_target)
> + goto out;
> +
> + sbe_timer_target = new_target;
> + has_new_target = true;
> + goto out;
> + }
> +
> + sbe_timer_target = new_target;
> + p9_sbe_timer_schedule();
> +
> +out:
> + unlock(&sbe_timer_lock);
> +}
> +
> +/* Initialize SBE timer */
> +static void p9_sbe_timer_init(void)
> +{
> + timer_ctrl_msg = p9_sbe_mkmsg(SBE_CMD_CONTROL_TIMER,
> + CONTROL_TIMER_START, 0, 0, 0);
> + assert(timer_ctrl_msg);
> +
> + init_lock(&sbe_timer_lock);
> + sbe_has_timer = true;
> + sbe_timer_target = mftb();
> + sbe_last_gen_stamp = ~0ull;
> + sbe_timer_def_tb = usecs_to_tb(SBE_TIMER_DEFAULT_US);
> + prlog(PR_INFO, "Timer facility on chip %x\n", sbe_default_chip_id);
> +}
> +
> +bool p9_sbe_timer_ok(void)
> +{
> + return sbe_has_timer;
> +}
> +
> void p9_sbe_init(void)
> {
> struct dt_node *xn;
> @@ -680,6 +882,9 @@ void p9_sbe_init(void)
> return;
> }
>
> + /* Initiate SBE timer */
> + p9_sbe_timer_init();
> +
> /* Initiate SBE timeout poller */
> opal_add_poller(p9_sbe_timeout_poll, NULL);
> }
> diff --git a/include/sbe-p9.h b/include/sbe-p9.h
> index 329afff80..4b839d8ba 100644
> --- a/include/sbe-p9.h
> +++ b/include/sbe-p9.h
> @@ -231,4 +231,10 @@ extern void p9_sbe_init(void);
> /* SBE interrupt */
> extern void p9_sbe_interrupt(uint32_t chip_id);
>
> +/* Is SBE timer available ? */
> +extern bool p9_sbe_timer_ok(void);
> +
> +/* Update SBE timer expiry */
> +extern void p9_sbe_update_timer_expiry(uint64_t new_target);
> +
> #endif /* __SBE_P9_H */
More information about the Skiboot
mailing list