Re: [PATCH 2/3] clocksource: Add irq pipelined clock events

[Philippe Gerum <rpm@xenomai.org>]

Thanks for sharing this work. Quick review, more later.

shannmu <shanmu1901@gmail.com> writes:

> This adds pipelined clock events for irqchip.
>
> Signed-off-by: shannmu <shanmu1901@gmail.com>
> ---
>  drivers/clocksource/timer-clint.c |  9 +++---
>  drivers/clocksource/timer-riscv.c | 46 ++++++++++++++++---------------
>  drivers/irqchip/irq-riscv-intc.c  |  1 +
>  drivers/irqchip/irq-sifive-plic.c |  7 +++--
>  4 files changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> index 9a55e733ae99..de3eb6937d6e 100644
> --- a/drivers/clocksource/timer-clint.c
> +++ b/drivers/clocksource/timer-clint.c
> @@ -111,17 +111,16 @@ static struct clocksource clint_clocksource = {
>  static int clint_clock_next_event(unsigned long delta,
>  				   struct clock_event_device *ce)
>  {
> -	void __iomem *r = clint_timer_cmp +
> -			  cpuid_to_hartid_map(smp_processor_id());
> +	void __iomem *r = clint_timer_cmp + cpuid_to_hartid_map(smp_processor_id());
>

Noisy whitespace change.

> -	csr_set(CSR_IE, IE_TIE);
>  	writeq_relaxed(clint_get_cycles64() + delta, r);
> +	csr_set(CSR_IE, IE_TIE);

I understand that you needed that change to get the timer event delivery
working in pipelined mode, however could you explain the rationale
behind this difference with the upstream implementation, assuming that
hard irqs should be disabled on entry to the clock event handler when
running on the oob stage?

>  
>  static int riscv_clock_next_event(unsigned long delta,
> -		struct clock_event_device *ce)
> +				  struct clock_event_device *ce)
>  {
>  	u64 next_tval = get_cycles64() + delta;
>  
> -	csr_set(CSR_IE, IE_TIE);
>  	if (static_branch_likely(&riscv_sstc_available)) {
>  #if defined(CONFIG_32BIT)
>  		csr_write(CSR_STIMECMP, next_tval & 0xFFFFFFFF);
> @@ -47,15 +46,17 @@ static int riscv_clock_next_event(unsigned long delta,
>  	} else
>  		sbi_set_timer(next_tval);
>  
> +	csr_set(CSR_IE, IE_TIE);
> +

Ditto.

>  	return 0;
>  }
>  
>  static unsigned int riscv_clock_event_irq;
>  static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
> -	.name			= "riscv_timer_clockevent",
> -	.features		= CLOCK_EVT_FEAT_ONESHOT,
> -	.rating			= 100,
> -	.set_next_event		= riscv_clock_next_event,
> +	.name = "riscv_timer_clockevent",
> +	.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PIPELINE,
> +	.rating = 100,
> +	.set_next_event = riscv_clock_next_event,
>  };

Some noisy whitespace changes here as well (might cause useless merge
conflicts when rebasing).

>  
>  /*
> @@ -74,11 +75,11 @@ static u64 notrace riscv_sched_clock(void)
>  }
>  
>  static struct clocksource riscv_clocksource = {
> -	.name		= "riscv_clocksource",
> -	.rating		= 400,
> -	.mask		= CLOCKSOURCE_MASK(64),
> -	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> -	.read		= riscv_clocksource_rdtime,
> +	.name = "riscv_clocksource",
> +	.rating = 400,
> +	.mask = CLOCKSOURCE_MASK(64),
> +	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +	.read = riscv_clocksource_rdtime,

Ditto.

>  
>  	riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER);
>  	if (!riscv_clock_event_irq) {
> -		pr_err("Failed to map timer interrupt for node [%pfwP]\n", intc_fwnode);
> +		pr_err("Failed to map timer interrupt for node [%pfwP]\n",
> +		       intc_fwnode);
>  		return -ENODEV;
>  	}
>

Again.

> @@ -152,9 +154,8 @@ static int __init riscv_timer_init_common(void)
>  
>  	sched_clock_register(riscv_sched_clock, 64, riscv_timebase);
>  
> -	error = request_percpu_irq(riscv_clock_event_irq,
> -				    riscv_timer_interrupt,
> -				    "riscv-timer", &riscv_clock_event);
> +	error = request_percpu_irq(riscv_clock_event_irq, riscv_timer_interrupt,
> +				   "riscv-timer", &riscv_clock_event);

Same.

>  	if (error) {
>  		pr_err("registering percpu irq failed [%d]\n", error);
>  		return error;
> @@ -166,8 +167,9 @@ static int __init riscv_timer_init_common(void)
>  	}
>  
>  	error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
> -			 "clockevents/riscv/timer:starting",
> -			 riscv_timer_starting_cpu, riscv_timer_dying_cpu);
> +				  "clockevents/riscv/timer:starting",
> +				  riscv_timer_starting_cpu,
> +				  riscv_timer_dying_cpu);

>  	if (error)
>  		pr_err("cpu hp setup state failed for RISCV timer [%d]\n",
>  		       error);
> @@ -183,8 +185,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>  
>  	error = riscv_of_processor_hartid(n, &hartid);
>  	if (error < 0) {
> -		pr_warn("Invalid hartid for node [%pOF] error = [%lu]\n",
> -			n, hartid);
> +		pr_warn("Invalid hartid for node [%pOF] error = [%lu]\n", n,
> +			hartid);
>  		return error;
>  	}
>  
> @@ -199,8 +201,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>  
>  	child = of_find_compatible_node(NULL, NULL, "riscv,timer");
>  	if (child) {
> -		riscv_timer_cannot_wake_cpu = of_property_read_bool(child,
> -					"riscv,timer-cannot-wake-cpu");
> +		riscv_timer_cannot_wake_cpu = of_property_read_bool(
> +			child, "riscv,timer-cannot-wake-cpu");
>  		of_node_put(child);
>  	}
>

etc.

>  static int riscv_intc_domain_map(struct irq_domain *d, unsigned int irq,
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index bf0b40b0fad4..9c03993ddbba 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -197,8 +197,8 @@ static struct irq_chip plic_edge_chip = {
>  	.irq_set_affinity = plic_set_affinity,
>  #endif
>  	.irq_set_type	= plic_irq_set_type,
> -	.flags		= IRQCHIP_SKIP_SET_WAKE |
> -			  IRQCHIP_AFFINITY_PRE_STARTUP,
> +	.flags		= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_AFFINITY_PRE_STARTUP |
> +		IRQCHIP_PIPELINE_SAFE,
>  };
>  
>  static struct irq_chip plic_chip = {
> @@ -213,7 +213,8 @@ static struct irq_chip plic_chip = {
>  #endif
>  	.irq_set_type	= plic_irq_set_type,
>  	.flags		= IRQCHIP_SKIP_SET_WAKE |
> -			  IRQCHIP_AFFINITY_PRE_STARTUP,
> +			  IRQCHIP_AFFINITY_PRE_STARTUP |
> +			  IRQCHIP_PIPELINE_SAFE,
>  };
>  
>  static int plic_irq_set_type(struct irq_data *d, unsigned int type)

This enable_lock is shared between the in-band and oob stages (via
plic_toggle() on EOI and plic_set_affinity() at the very least. We need
a hard spinlock to replace the original raw spinlock (API to use it is
unchanged).

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index bf0b40b0fad4b..a3fa11a96645d 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -79,7 +79,7 @@ struct plic_handler {
 	 * Protect mask operations on the registers given that we can't
 	 * assume atomic memory operations work on them.
 	 */
-	raw_spinlock_t		enable_lock;
+	hard_spinlock_t		enable_lock;
 	void __iomem		*enable_base;
 	u32			*enable_save;
 	struct plic_priv	*priv;

-- 
Philippe.