diff mbox series

clocksource: Ingenic: Add SMP/SMT support for sysost driver.

Message ID 1621963596-78952-1-git-send-email-zhouyanjie@wanyeetech.com (mailing list archive)
State Not Applicable
Headers show
Series clocksource: Ingenic: Add SMP/SMT support for sysost driver. | expand

Commit Message

Zhou Yanjie May 25, 2021, 5:26 p.m. UTC
The OST in Ingenic XBurst®2 SoCs such as X2000 and X2100, has a global
timer and two or four percpu timers, add support for the percpu timers.

Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
---
 drivers/clocksource/ingenic-sysost.c | 324 ++++++++++++++++++++++++++---------
 1 file changed, 246 insertions(+), 78 deletions(-)

Comments

Paul Cercueil May 26, 2021, 11:43 a.m. UTC | #1
Hi Zhou,

Le mer., mai 26 2021 at 01:26:36 +0800, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@wanyeetech.com> a écrit :
> The OST in Ingenic XBurst®2 SoCs such as X2000 and X2100, has a 
> global
> timer and two or four percpu timers, add support for the percpu 
> timers.

That means X2100 is a quad-core? :)

> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
>  drivers/clocksource/ingenic-sysost.c | 324 
> ++++++++++++++++++++++++++---------
>  1 file changed, 246 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/clocksource/ingenic-sysost.c 
> b/drivers/clocksource/ingenic-sysost.c
> index e77d584..f35e0a3 100644
> --- a/drivers/clocksource/ingenic-sysost.c
> +++ b/drivers/clocksource/ingenic-sysost.c
> @@ -13,6 +13,8 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/overflow.h>
>  #include <linux/sched_clock.h>
>  #include <linux/slab.h>
>  #include <linux/syscore_ops.h>
> @@ -21,10 +23,14 @@
> 
>  /* OST register offsets */
>  #define OST_REG_OSTCCR			0x00
> +#define OST_REG_OSTER			0x04
>  #define OST_REG_OSTCR			0x08
>  #define OST_REG_OSTFR			0x0c
> +#define OST_REG_OSTCNTH			0x0c
>  #define OST_REG_OSTMR			0x10
> +#define OST_REG_OSTCNTL			0x10
>  #define OST_REG_OST1DFR			0x14
> +#define OST_REG_OSTCNTB			0x14
>  #define OST_REG_OST1CNT			0x18
>  #define OST_REG_OST2CNTL		0x20
>  #define OST_REG_OSTCNT2HBUF		0x24
> @@ -55,13 +61,24 @@
>  #define OSTECR_OST1ENC			BIT(0)
>  #define OSTECR_OST2ENC			BIT(1)
> 
> +enum ingenic_ost_version {
> +	ID_X1000,
> +	ID_X2000,
> +};
> +
>  struct ingenic_soc_info {
> +	enum ingenic_ost_version version;
> +	const struct ingenic_ost_clk_info *clk_info;
> +
>  	unsigned int num_channels;
> +	unsigned int base_offset;
>  };
> 
>  struct ingenic_ost_clk_info {
>  	struct clk_init_data init_data;
> -	u8 ostccr_reg;
> +	unsigned int idx;
> +	u32 ostccr_reg;
> +	u32 ostcntl_reg;
>  };
> 
>  struct ingenic_ost_clk {
> @@ -71,15 +88,27 @@ struct ingenic_ost_clk {
>  	const struct ingenic_ost_clk_info *info;
>  };
> 
> +struct ingenic_ost_timer {
> +	void __iomem *base;
> +	unsigned int cpu;
> +	unsigned int channel;
> +	struct clock_event_device cevt;
> +	struct clk *clk;
> +	char name[20];
> +	spinlock_t	lock;

Use a regular space instead of the tab.

> +};
> +
>  struct ingenic_ost {
>  	void __iomem *base;
>  	const struct ingenic_soc_info *soc_info;
> -	struct clk *clk, *percpu_timer_clk, *global_timer_clk;
> -	struct clock_event_device cevt;
> +	struct clk *clk, *global_timer_clk;
> +	struct device_node *np;
>  	struct clocksource cs;
> -	char name[20];
> 
>  	struct clk_hw_onecell_data *clocks;
> +	struct ingenic_ost_timer __percpu *timers;
> +
> +	int irq;
>  };
> 
>  static struct ingenic_ost *ingenic_ost;
> @@ -94,9 +123,10 @@ static unsigned long 
> ingenic_ost_percpu_timer_recalc_rate(struct clk_hw *hw,
>  {
>  	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>  	const struct ingenic_ost_clk_info *info = ost_clk->info;
> +	struct ingenic_ost_timer *timer = per_cpu_ptr(ost_clk->ost->timers, 
> info->idx);
>  	unsigned int prescale;
> 
> -	prescale = readl(ost_clk->ost->base + info->ostccr_reg);
> +	prescale = readl(timer->base + info->ostccr_reg);
> 
>  	prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> 
> OSTCCR_PRESCALE1_LSB;
> 
> @@ -108,11 +138,12 @@ static unsigned long 
> ingenic_ost_global_timer_recalc_rate(struct clk_hw *hw,
>  {
>  	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>  	const struct ingenic_ost_clk_info *info = ost_clk->info;
> +	struct ingenic_ost_timer *timer = per_cpu_ptr(ost_clk->ost->timers, 
> info->idx);
>  	unsigned int prescale;
> 
> -	prescale = readl(ost_clk->ost->base + info->ostccr_reg);
> +	prescale = readl(timer->base + info->ostccr_reg);
> 
> -	prescale = (prescale & OSTCCR_PRESCALE2_MASK) >> 
> OSTCCR_PRESCALE2_LSB;
> +	prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> 
> OSTCCR_PRESCALE1_LSB;

This looks bogus to me, especially when looking at 
ingenic_ost_global_timer_set_rate().

Also, use FIELD_GET() from <linux/bitfield.h>, then you can drop the 
*_LSB macros (maybe do that in a first patch).

> 
>  	return parent_rate >> (prescale * 2);
>  }
> @@ -147,12 +178,13 @@ static int 
> ingenic_ost_percpu_timer_set_rate(struct clk_hw *hw, unsigned long re
>  {
>  	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>  	const struct ingenic_ost_clk_info *info = ost_clk->info;
> +	struct ingenic_ost_timer *timer = per_cpu_ptr(ost_clk->ost->timers, 
> info->idx);
>  	u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
>  	int val;
> 
> -	val = readl(ost_clk->ost->base + info->ostccr_reg);
> +	val = readl(timer->base + info->ostccr_reg);
>  	val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << 
> OSTCCR_PRESCALE1_LSB);
> -	writel(val, ost_clk->ost->base + info->ostccr_reg);
> +	writel(val, timer->base + info->ostccr_reg);
> 
>  	return 0;
>  }
> @@ -162,12 +194,16 @@ static int 
> ingenic_ost_global_timer_set_rate(struct clk_hw *hw, unsigned long re
>  {
>  	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>  	const struct ingenic_ost_clk_info *info = ost_clk->info;
> +	struct ingenic_ost_timer *timer = per_cpu_ptr(ost_clk->ost->timers, 
> info->idx);
>  	u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
>  	int val;
> 
> -	val = readl(ost_clk->ost->base + info->ostccr_reg);
> -	val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << 
> OSTCCR_PRESCALE2_LSB);
> -	writel(val, ost_clk->ost->base + info->ostccr_reg);
> +	val = readl(timer->base + info->ostccr_reg);
> +	if (ost_clk->ost->soc_info->version >= ID_X2000)
> +		val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << 
> OSTCCR_PRESCALE1_LSB);
> +	else
> +		val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << 
> OSTCCR_PRESCALE2_LSB);
> +	writel(val, timer->base + info->ostccr_reg);
> 
>  	return 0;
>  }
> @@ -186,7 +222,19 @@ static const struct clk_ops 
> ingenic_ost_global_timer_ops = {
> 
>  static const char * const ingenic_ost_clk_parents[] = { "ext" };
> 
> -static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
> +static const struct ingenic_ost_clk_info x1000_ost_clk_info[] = {
> +	[OST_CLK_GLOBAL_TIMER] = {
> +		.init_data = {
> +			.name = "global timer",
> +			.parent_names = ingenic_ost_clk_parents,
> +			.num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
> +			.ops = &ingenic_ost_global_timer_ops,
> +			.flags = CLK_SET_RATE_UNGATE,
> +		},
> +		.ostccr_reg = OST_REG_OSTCCR,
> +		.ostcntl_reg = OST_REG_OST2CNTL,
> +	},

Here you add "global timer" support for the X1000 SoC, correct? Then 
this is unrelated to this commit and should be done separately, I think.

> +
>  	[OST_CLK_PERCPU_TIMER] = {
>  		.init_data = {
>  			.name = "percpu timer",
> @@ -195,9 +243,12 @@ static const struct ingenic_ost_clk_info 
> ingenic_ost_clk_info[] = {
>  			.ops = &ingenic_ost_percpu_timer_ops,
>  			.flags = CLK_SET_RATE_UNGATE,
>  		},
> +		.idx = 0,
>  		.ostccr_reg = OST_REG_OSTCCR,
>  	},
> +};
> 
> +static const struct ingenic_ost_clk_info x2000_ost_clk_info[] = {
>  	[OST_CLK_GLOBAL_TIMER] = {
>  		.init_data = {
>  			.name = "global timer",
> @@ -207,6 +258,31 @@ static const struct ingenic_ost_clk_info 
> ingenic_ost_clk_info[] = {
>  			.flags = CLK_SET_RATE_UNGATE,
>  		},
>  		.ostccr_reg = OST_REG_OSTCCR,
> +		.ostcntl_reg = OST_REG_OSTCNTL,
> +	},
> +
> +	[OST_CLK_PERCPU_TIMER0] = {
> +		.init_data = {
> +			.name = "percpu timer0",
> +			.parent_names = ingenic_ost_clk_parents,
> +			.num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
> +			.ops = &ingenic_ost_percpu_timer_ops,
> +			.flags = CLK_SET_RATE_UNGATE,
> +		},
> +		.idx = 0,
> +		.ostccr_reg = OST_REG_OSTCCR,
> +	},
> +
> +	[OST_CLK_PERCPU_TIMER1] = {
> +		.init_data = {
> +			.name = "percpu timer1",
> +			.parent_names = ingenic_ost_clk_parents,
> +			.num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
> +			.ops = &ingenic_ost_percpu_timer_ops,
> +			.flags = CLK_SET_RATE_UNGATE,
> +		},
> +		.idx = 1,
> +		.ostccr_reg = OST_REG_OSTCCR,
>  	},
>  };
> 
> @@ -215,7 +291,7 @@ static u64 notrace 
> ingenic_ost_global_timer_read_cntl(void)
>  	struct ingenic_ost *ost = ingenic_ost;
>  	unsigned int count;
> 
> -	count = readl(ost->base + OST_REG_OST2CNTL);
> +	count = readl(ost->base + ost->soc_info->clk_info->ostcntl_reg);
> 
>  	return count;
>  }
> @@ -225,16 +301,21 @@ static u64 notrace 
> ingenic_ost_clocksource_read(struct clocksource *cs)
>  	return ingenic_ost_global_timer_read_cntl();
>  }
> 
> -static inline struct ingenic_ost *to_ingenic_ost(struct 
> clock_event_device *evt)
> +static inline struct ingenic_ost_timer *
> +to_ingenic_ost_timer(struct clock_event_device *evt)
>  {
> -	return container_of(evt, struct ingenic_ost, cevt);
> +	return container_of(evt, struct ingenic_ost_timer, cevt);
>  }
> 
>  static int ingenic_ost_cevt_set_state_shutdown(struct 
> clock_event_device *evt)
>  {
> -	struct ingenic_ost *ost = to_ingenic_ost(evt);
> +	struct ingenic_ost_timer *timer = to_ingenic_ost_timer(evt);
> +	struct ingenic_ost *ost = ingenic_ost;

You should avoid referencing the global 'ingenic_ost' variable if you 
can. You could instead add a backpointer to the ingenic_ost structure 
in your 'ingenic_ost_timer'.

> 
> -	writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
> +	if (ost->soc_info->version >= ID_X2000)
> +		writel(0, timer->base + OST_REG_OSTER);
> +	else
> +		writel(OSTECR_OST1ENC, timer->base + OST_REG_OSTECR);
> 
>  	return 0;
>  }
> @@ -242,26 +323,44 @@ static int 
> ingenic_ost_cevt_set_state_shutdown(struct clock_event_device *evt)
>  static int ingenic_ost_cevt_set_next(unsigned long next,
>  				     struct clock_event_device *evt)
>  {
> -	struct ingenic_ost *ost = to_ingenic_ost(evt);
> +	struct ingenic_ost_timer *timer = to_ingenic_ost_timer(evt);
> +	struct ingenic_ost *ost = ingenic_ost;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&timer->lock, flags);
> 
> -	writel((u32)~OSTFR_FFLAG, ost->base + OST_REG_OSTFR);
> -	writel(next, ost->base + OST_REG_OST1DFR);
> -	writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
> -	writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTESR);
> -	writel((u32)~OSTMR_FMASK, ost->base + OST_REG_OSTMR);
> +	writel((u32)~OSTFR_FFLAG, timer->base + OST_REG_OSTFR);
> +	writel(next, timer->base + OST_REG_OST1DFR);
> +	writel(OSTCR_OST1CLR, timer->base + OST_REG_OSTCR);
> +
> +	if (ost->soc_info->version >= ID_X2000) {
> +		writel(OSTESR_OST1ENS, timer->base + OST_REG_OSTER);
> +	} else {
> +		writel(OSTESR_OST1ENS, timer->base + OST_REG_OSTESR);
> +		writel((u32)~OSTMR_FMASK, timer->base + OST_REG_OSTMR);
> +	}
> +
> +	spin_unlock_irqrestore(&timer->lock, flags);
> 
>  	return 0;
>  }
> 
>  static irqreturn_t ingenic_ost_cevt_cb(int irq, void *dev_id)
>  {
> -	struct clock_event_device *evt = dev_id;
> -	struct ingenic_ost *ost = to_ingenic_ost(evt);
> +	struct ingenic_ost *ost = ingenic_ost;
> +	struct ingenic_ost_timer *timer = dev_id;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&timer->lock, flags);

You're doing only one writel() here - I *think* it should be fine 
without the spinlock.

> +
> +	if (ost->soc_info->version >= ID_X2000)
> +		writel(0, timer->base + OST_REG_OSTER);
> +	else
> +		writel(OSTECR_OST1ENC, timer->base + OST_REG_OSTECR);
> 
> -	writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
> +	spin_unlock_irqrestore(&timer->lock, flags);
> 
> -	if (evt->event_handler)
> -		evt->event_handler(evt);
> +	timer->cevt.event_handler(&timer->cevt);
> 
>  	return IRQ_HANDLED;
>  }
> @@ -271,6 +370,7 @@ static int __init 
> ingenic_ost_register_clock(struct ingenic_ost *ost,
>  			struct clk_hw_onecell_data *clocks)
>  {
>  	struct ingenic_ost_clk *ost_clk;
> +	struct ingenic_ost_timer *timer = per_cpu_ptr(ost->timers, 
> info->idx);
>  	int val, err;
> 
>  	ost_clk = kzalloc(sizeof(*ost_clk), GFP_KERNEL);
> @@ -283,9 +383,9 @@ static int __init 
> ingenic_ost_register_clock(struct ingenic_ost *ost,
>  	ost_clk->ost = ost;
> 
>  	/* Reset clock divider */
> -	val = readl(ost->base + info->ostccr_reg);
> -	val &= ~(OSTCCR_PRESCALE1_MASK | OSTCCR_PRESCALE2_MASK);
> -	writel(val, ost->base + info->ostccr_reg);
> +	val = readl(timer->base + info->ostccr_reg);
> +	val &= ~(OSTCCR_PRESCALE1_MASK);
> +	writel(val, timer->base + info->ostccr_reg);
> 
>  	err = clk_hw_register(NULL, &ost_clk->hw);
>  	if (err) {
> @@ -309,57 +409,53 @@ static struct clk * __init 
> ingenic_ost_get_clock(struct device_node *np, int id)
>  	return of_clk_get_from_provider(&args);
>  }
> 
> -static int __init ingenic_ost_percpu_timer_init(struct device_node 
> *np,
> -					 struct ingenic_ost *ost)
> +static int __init ingenic_ost_setup_cevt(unsigned int cpu)
>  {
> -	unsigned int timer_virq, channel = OST_CLK_PERCPU_TIMER;
> +	struct ingenic_ost *ost = ingenic_ost;
> +	struct ingenic_ost_timer *timer = this_cpu_ptr(ost->timers);
>  	unsigned long rate;
>  	int err;
> 
> -	ost->percpu_timer_clk = ingenic_ost_get_clock(np, channel);
> -	if (IS_ERR(ost->percpu_timer_clk))
> -		return PTR_ERR(ost->percpu_timer_clk);
> +	timer->clk = ingenic_ost_get_clock(ost->np, timer->channel);
> +	if (IS_ERR(timer->clk))
> +		return PTR_ERR(timer->clk);
> 
> -	err = clk_prepare_enable(ost->percpu_timer_clk);
> +	err = clk_prepare_enable(timer->clk);
>  	if (err)
>  		goto err_clk_put;
> 
> -	rate = clk_get_rate(ost->percpu_timer_clk);
> +	rate = clk_get_rate(timer->clk);
>  	if (!rate) {
>  		err = -EINVAL;
>  		goto err_clk_disable;
>  	}
> 
> -	timer_virq = of_irq_get(np, 0);
> -	if (!timer_virq) {
> -		err = -EINVAL;
> -		goto err_clk_disable;
> -	}
> +	snprintf(timer->name, sizeof(timer->name), "OST percpu timer%u", 
> cpu);
> 
> -	snprintf(ost->name, sizeof(ost->name), "OST percpu timer");
> +	/* Unmask full comparison match interrupt */
> +	writel((u32)~OSTMR_FMASK, timer->base + OST_REG_OSTMR);
> 
> -	err = request_irq(timer_virq, ingenic_ost_cevt_cb, IRQF_TIMER,
> -			  ost->name, &ost->cevt);
> -	if (err)
> -		goto err_irq_dispose_mapping;
> +	timer->cpu = smp_processor_id();
> +	timer->cevt.cpumask = cpumask_of(smp_processor_id());
> +	timer->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
> +	timer->cevt.name = timer->name;
> +	timer->cevt.rating = 400;
> +	timer->cevt.set_state_shutdown = 
> ingenic_ost_cevt_set_state_shutdown;
> +	timer->cevt.set_next_event = ingenic_ost_cevt_set_next;
> +
> +	clockevents_config_and_register(&timer->cevt, rate, 4, 0xffffffff);
> 
> -	ost->cevt.cpumask = cpumask_of(smp_processor_id());
> -	ost->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
> -	ost->cevt.name = ost->name;
> -	ost->cevt.rating = 400;
> -	ost->cevt.set_state_shutdown = ingenic_ost_cevt_set_state_shutdown;
> -	ost->cevt.set_next_event = ingenic_ost_cevt_set_next;
> +	if (ost->soc_info->version >= ID_X2000)
> +		enable_percpu_irq(ost->irq, IRQ_TYPE_NONE);
> 
> -	clockevents_config_and_register(&ost->cevt, rate, 4, 0xffffffff);
> +	spin_lock_init(&timer->lock);
> 
>  	return 0;
> 
> -err_irq_dispose_mapping:
> -	irq_dispose_mapping(timer_virq);
>  err_clk_disable:
> -	clk_disable_unprepare(ost->percpu_timer_clk);
> +	clk_disable_unprepare(timer->clk);
>  err_clk_put:
> -	clk_put(ost->percpu_timer_clk);
> +	clk_put(timer->clk);
>  	return err;
>  }
> 
> @@ -385,11 +481,14 @@ static int __init 
> ingenic_ost_global_timer_init(struct device_node *np,
>  		goto err_clk_disable;
>  	}
> 
> -	/* Clear counter CNT registers */
> -	writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
> -
> -	/* Enable OST channel */
> -	writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
> +	/* Clear counter CNT registers and enable OST channel */
> +	if (ost->soc_info->version >= ID_X2000) {
> +		writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
> +		writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTER);
> +	} else {
> +		writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
> +		writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
> +	}
> 
>  	cs->name = "ingenic-ost";
>  	cs->rating = 400;
> @@ -411,18 +510,33 @@ static int __init 
> ingenic_ost_global_timer_init(struct device_node *np,
>  }
> 
>  static const struct ingenic_soc_info x1000_soc_info = {
> +	.version = ID_X1000,
> +	.clk_info = x1000_ost_clk_info,
> +
>  	.num_channels = 2,
>  };
> 
> -static const struct of_device_id __maybe_unused 
> ingenic_ost_of_match[] __initconst = {
> -	{ .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info, },
> +static const struct ingenic_soc_info x2000_soc_info = {
> +	.version = ID_X2000,
> +	.clk_info = x2000_ost_clk_info,
> +
> +	.num_channels = 3,
> +	.base_offset = 0x100,
> +};
> +
> +static const struct of_device_id __maybe_unused 
> ingenic_ost_of_matches[] __initconst = {
> +	{ .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info },
> +	{ .compatible = "ingenic,x2000-ost", .data = &x2000_soc_info },
>  	{ /* sentinel */ }
>  };
> 
>  static int __init ingenic_ost_probe(struct device_node *np)
>  {
> -	const struct of_device_id *id = of_match_node(ingenic_ost_of_match, 
> np);
> +	const struct of_device_id *id = 
> of_match_node(ingenic_ost_of_matches, np);

Avoid any changes unrelated to the commit - for instance renaming 
'ingenic_ost_of_match' or removing the comma after the .data field, 
there's just no need and it artificially grows the patch.

Please be extra careful to make the smallest patch possible by not 
adding unrelated changes; smaller patches are easier to review, and are 
more likely to be accepted sooner.

> +	struct ingenic_ost_timer *timer;
>  	struct ingenic_ost *ost;
> +	void __iomem *base;
> +	unsigned int cpu;
>  	unsigned int i;
>  	int ret;
> 
> @@ -430,18 +544,43 @@ static int __init ingenic_ost_probe(struct 
> device_node *np)
>  	if (!ost)
>  		return -ENOMEM;
> 
> +	ost->timers = alloc_percpu(struct ingenic_ost_timer);
> +	if (!ost->timers) {
> +		ret = -ENOMEM;
> +		goto err_free_ost;
> +	}
> +
> +	ost->np = np;
> +	ost->soc_info = id->data;

This was moved but didn't have to, as far as I can see.

> +
>  	ost->base = of_io_request_and_map(np, 0, of_node_full_name(np));
>  	if (IS_ERR(ost->base)) {
>  		pr_err("%s: Failed to map OST registers\n", __func__);
>  		ret = PTR_ERR(ost->base);
> -		goto err_free_ost;
> +		goto err_free_timers;
> +	}
> +
> +	if (ost->soc_info->version >= ID_X2000) {
> +		base = of_io_request_and_map(np, 1, of_node_full_name(np));
> +		if (IS_ERR(base)) {
> +			pr_err("%s: Failed to map OST registers\n", __func__);
> +			ret = PTR_ERR(base);
> +			goto err_free_timers;
> +		}
> +	}
> +
> +	ost->irq = irq_of_parse_and_map(np, 0);
> +	if (ost->irq < 0) {
> +		pr_crit("%s: Cannot to get OST IRQ\n", __func__);
> +		ret = -EINVAL;

ret = ost->irq?

> +		goto err_free_timers;
>  	}
> 
>  	ost->clk = of_clk_get_by_name(np, "ost");
>  	if (IS_ERR(ost->clk)) {
> -		ret = PTR_ERR(ost->clk);
>  		pr_crit("%s: Cannot get OST clock\n", __func__);
> -		goto err_free_ost;
> +		ret = PTR_ERR(ost->clk);
> +		goto err_free_timers;
>  	}
> 
>  	ret = clk_prepare_enable(ost->clk);
> @@ -450,8 +589,6 @@ static int __init ingenic_ost_probe(struct 
> device_node *np)
>  		goto err_put_clk;
>  	}
> 
> -	ost->soc_info = id->data;
> -
>  	ost->clocks = kzalloc(struct_size(ost->clocks, hws, 
> ost->soc_info->num_channels),
>  			      GFP_KERNEL);
>  	if (!ost->clocks) {
> @@ -461,8 +598,20 @@ static int __init ingenic_ost_probe(struct 
> device_node *np)
> 
>  	ost->clocks->num = ost->soc_info->num_channels;
> 
> -	for (i = 0; i < ost->clocks->num; i++) {
> -		ret = ingenic_ost_register_clock(ost, i, &ingenic_ost_clk_info[i], 
> ost->clocks);
> +	for (cpu = 0; cpu < num_possible_cpus(); cpu++) {
> +		timer = per_cpu_ptr(ost->timers, cpu);
> +
> +		if (ost->soc_info->version >= ID_X2000)
> +			timer->base = base + ost->soc_info->base_offset * cpu;
> +		else
> +			timer->base = ost->base;
> +
> +		timer->cpu = cpu;
> +		timer->channel = OST_CLK_PERCPU_TIMER + cpu;
> +	}
> +
> +	for (i = 0; i < num_possible_cpus() + 1; i++) {
> +		ret = ingenic_ost_register_clock(ost, i, 
> &ost->soc_info->clk_info[i], ost->clocks);

This won't work. Your 'clk_info' array only has 2-3 entries. 
num_possible_cpus() returns the maximum number of CPUs that is 
supported by the kernel, which could be way more than the number of 
cores you actually have. I think you want num_online_cpus() here, as 
the number of online CPUs is guaranteed to be lower or equal to the 
actual number of cores.

>  		if (ret) {
>  			pr_crit("%s: Cannot register clock %d\n", __func__, i);
>  			goto err_unregister_ost_clocks;
> @@ -488,6 +637,8 @@ static int __init ingenic_ost_probe(struct 
> device_node *np)
>  	clk_disable_unprepare(ost->clk);
>  err_put_clk:
>  	clk_put(ost->clk);
> +err_free_timers:
> +	free_percpu(ost->timers);
>  err_free_ost:
>  	kfree(ost);
>  	return ret;
> @@ -517,9 +668,25 @@ static int __init ingenic_ost_init(struct 
> device_node *np)
>  		goto err_free_ingenic_ost;
>  	}
> 
> -	ret = ingenic_ost_percpu_timer_init(np, ost);
> -	if (ret)
> +	if (ost->soc_info->version >= ID_X2000)
> +		ret = request_percpu_irq(ost->irq, ingenic_ost_cevt_cb,
> +				  "OST percpu timer", ost->timers);
> +	else
> +		ret = request_irq(ost->irq, ingenic_ost_cevt_cb, IRQF_TIMER,
> +				  "OST percpu timer", ost->timers);
> +
> +	if (ret) {
> +		pr_crit("%s: Unable to request percpu IRQ: %x\n", __func__, ret);

You mean %d?

>  		goto err_ost_global_timer_cleanup;
> +	}
> +
> +	/* Setup clock events on each CPU core */
> +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "Ingenic XBurst: 
> online",
> +				ingenic_ost_setup_cevt, NULL);
> +	if (ret < 0) {
> +		pr_crit("%s: Unable to init percpu timers: %x\n", __func__, ret);

Same here.

Cheers,
-Paul

> +		goto err_ost_global_timer_cleanup;
> +	}
> 
>  	/* Register the sched_clock at the end as there's no way to undo it 
> */
>  	rate = clk_get_rate(ost->global_timer_clk);
> @@ -537,3 +704,4 @@ static int __init ingenic_ost_init(struct 
> device_node *np)
>  }
> 
>  TIMER_OF_DECLARE(x1000_ost,  "ingenic,x1000-ost",  ingenic_ost_init);
> +TIMER_OF_DECLARE(x2000_ost,  "ingenic,x2000-ost",  ingenic_ost_init);
> --
> 2.7.4
>
Zhou Yanjie May 26, 2021, 4:05 p.m. UTC | #2
Hi Paul,

On 2021/5/26 下午7:43, Paul Cercueil wrote:
> Hi Zhou,
>
> Le mer., mai 26 2021 at 01:26:36 +0800, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> The OST in Ingenic XBurst®2 SoCs such as X2000 and X2100, has a global
>> timer and two or four percpu timers, add support for the percpu timers.
>
> That means X2100 is a quad-core? :)


No, the X2100 is still a single-core dual-thread (two logical CPUs). 
According to rumors, the dual-core four-thread processor may be X2800 or 
X2900.


>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>  drivers/clocksource/ingenic-sysost.c | 324 
>> ++++++++++++++++++++++++++---------
>>  1 file changed, 246 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/clocksource/ingenic-sysost.c 
>> b/drivers/clocksource/ingenic-sysost.c
>> index e77d584..f35e0a3 100644
>> --- a/drivers/clocksource/ingenic-sysost.c
>> +++ b/drivers/clocksource/ingenic-sysost.c
>> @@ -13,6 +13,8 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/overflow.h>
>>  #include <linux/sched_clock.h>
>>  #include <linux/slab.h>
>>  #include <linux/syscore_ops.h>
>> @@ -21,10 +23,14 @@
>>
>>  /* OST register offsets */
>>  #define OST_REG_OSTCCR            0x00
>> +#define OST_REG_OSTER            0x04
>>  #define OST_REG_OSTCR            0x08
>>  #define OST_REG_OSTFR            0x0c
>> +#define OST_REG_OSTCNTH            0x0c
>>  #define OST_REG_OSTMR            0x10
>> +#define OST_REG_OSTCNTL            0x10
>>  #define OST_REG_OST1DFR            0x14
>> +#define OST_REG_OSTCNTB            0x14
>>  #define OST_REG_OST1CNT            0x18
>>  #define OST_REG_OST2CNTL        0x20
>>  #define OST_REG_OSTCNT2HBUF        0x24
>> @@ -55,13 +61,24 @@
>>  #define OSTECR_OST1ENC            BIT(0)
>>  #define OSTECR_OST2ENC            BIT(1)
>>
>> +enum ingenic_ost_version {
>> +    ID_X1000,
>> +    ID_X2000,
>> +};
>> +
>>  struct ingenic_soc_info {
>> +    enum ingenic_ost_version version;
>> +    const struct ingenic_ost_clk_info *clk_info;
>> +
>>      unsigned int num_channels;
>> +    unsigned int base_offset;
>>  };
>>
>>  struct ingenic_ost_clk_info {
>>      struct clk_init_data init_data;
>> -    u8 ostccr_reg;
>> +    unsigned int idx;
>> +    u32 ostccr_reg;
>> +    u32 ostcntl_reg;
>>  };
>>
>>  struct ingenic_ost_clk {
>> @@ -71,15 +88,27 @@ struct ingenic_ost_clk {
>>      const struct ingenic_ost_clk_info *info;
>>  };
>>
>> +struct ingenic_ost_timer {
>> +    void __iomem *base;
>> +    unsigned int cpu;
>> +    unsigned int channel;
>> +    struct clock_event_device cevt;
>> +    struct clk *clk;
>> +    char name[20];
>> +    spinlock_t    lock;
>
> Use a regular space instead of the tab.


Sure.


>
>> +};
>> +
>>  struct ingenic_ost {
>>      void __iomem *base;
>>      const struct ingenic_soc_info *soc_info;
>> -    struct clk *clk, *percpu_timer_clk, *global_timer_clk;
>> -    struct clock_event_device cevt;
>> +    struct clk *clk, *global_timer_clk;
>> +    struct device_node *np;
>>      struct clocksource cs;
>> -    char name[20];
>>
>>      struct clk_hw_onecell_data *clocks;
>> +    struct ingenic_ost_timer __percpu *timers;
>> +
>> +    int irq;
>>  };
>>
>>  static struct ingenic_ost *ingenic_ost;
>> @@ -94,9 +123,10 @@ static unsigned long 
>> ingenic_ost_percpu_timer_recalc_rate(struct clk_hw *hw,
>>  {
>>      struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>>      const struct ingenic_ost_clk_info *info = ost_clk->info;
>> +    struct ingenic_ost_timer *timer = 
>> per_cpu_ptr(ost_clk->ost->timers, info->idx);
>>      unsigned int prescale;
>>
>> -    prescale = readl(ost_clk->ost->base + info->ostccr_reg);
>> +    prescale = readl(timer->base + info->ostccr_reg);
>>
>>      prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> 
>> OSTCCR_PRESCALE1_LSB;
>>
>> @@ -108,11 +138,12 @@ static unsigned long 
>> ingenic_ost_global_timer_recalc_rate(struct clk_hw *hw,
>>  {
>>      struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>>      const struct ingenic_ost_clk_info *info = ost_clk->info;
>> +    struct ingenic_ost_timer *timer = 
>> per_cpu_ptr(ost_clk->ost->timers, info->idx);
>>      unsigned int prescale;
>>
>> -    prescale = readl(ost_clk->ost->base + info->ostccr_reg);
>> +    prescale = readl(timer->base + info->ostccr_reg);
>>
>> -    prescale = (prescale & OSTCCR_PRESCALE2_MASK) >> 
>> OSTCCR_PRESCALE2_LSB;
>> +    prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> 
>> OSTCCR_PRESCALE1_LSB;
>
> This looks bogus to me, especially when looking at 
> ingenic_ost_global_timer_set_rate().
>

My mistake, there is indeed a problem here.


> Also, use FIELD_GET() from <linux/bitfield.h>, then you can drop the 
> *_LSB macros (maybe do that in a first patch).
>

Sure.


>>
>>      return parent_rate >> (prescale * 2);
>>  }
>> @@ -147,12 +178,13 @@ static int 
>> ingenic_ost_percpu_timer_set_rate(struct clk_hw *hw, unsigned long re
>>  {
>>      struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>>      const struct ingenic_ost_clk_info *info = ost_clk->info;
>> +    struct ingenic_ost_timer *timer = 
>> per_cpu_ptr(ost_clk->ost->timers, info->idx);
>>      u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
>>      int val;
>>
>> -    val = readl(ost_clk->ost->base + info->ostccr_reg);
>> +    val = readl(timer->base + info->ostccr_reg);
>>      val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << 
>> OSTCCR_PRESCALE1_LSB);
>> -    writel(val, ost_clk->ost->base + info->ostccr_reg);
>> +    writel(val, timer->base + info->ostccr_reg);
>>
>>      return 0;
>>  }
>> @@ -162,12 +194,16 @@ static int 
>> ingenic_ost_global_timer_set_rate(struct clk_hw *hw, unsigned long re
>>  {
>>      struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>>      const struct ingenic_ost_clk_info *info = ost_clk->info;
>> +    struct ingenic_ost_timer *timer = 
>> per_cpu_ptr(ost_clk->ost->timers, info->idx);
>>      u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
>>      int val;
>>
>> -    val = readl(ost_clk->ost->base + info->ostccr_reg);
>> -    val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << 
>> OSTCCR_PRESCALE2_LSB);
>> -    writel(val, ost_clk->ost->base + info->ostccr_reg);
>> +    val = readl(timer->base + info->ostccr_reg);
>> +    if (ost_clk->ost->soc_info->version >= ID_X2000)
>> +        val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << 
>> OSTCCR_PRESCALE1_LSB);
>> +    else
>> +        val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << 
>> OSTCCR_PRESCALE2_LSB);
>> +    writel(val, timer->base + info->ostccr_reg);
>>
>>      return 0;
>>  }
>> @@ -186,7 +222,19 @@ static const struct clk_ops 
>> ingenic_ost_global_timer_ops = {
>>
>>  static const char * const ingenic_ost_clk_parents[] = { "ext" };
>>
>> -static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
>> +static const struct ingenic_ost_clk_info x1000_ost_clk_info[] = {
>> +    [OST_CLK_GLOBAL_TIMER] = {
>> +        .init_data = {
>> +            .name = "global timer",
>> +            .parent_names = ingenic_ost_clk_parents,
>> +            .num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
>> +            .ops = &ingenic_ost_global_timer_ops,
>> +            .flags = CLK_SET_RATE_UNGATE,
>> +        },
>> +        .ostccr_reg = OST_REG_OSTCCR,
>> +        .ostcntl_reg = OST_REG_OST2CNTL,
>> +    },
>
> Here you add "global timer" support for the X1000 SoC, correct? Then 
> this is unrelated to this commit and should be done separately, I think.
>

OK, I will split it to a new patch.


>> +
>>      [OST_CLK_PERCPU_TIMER] = {
>>          .init_data = {
>>              .name = "percpu timer",
>> @@ -195,9 +243,12 @@ static const struct ingenic_ost_clk_info 
>> ingenic_ost_clk_info[] = {
>>              .ops = &ingenic_ost_percpu_timer_ops,
>>              .flags = CLK_SET_RATE_UNGATE,
>>          },
>> +        .idx = 0,
>>          .ostccr_reg = OST_REG_OSTCCR,
>>      },
>> +};
>>
>> +static const struct ingenic_ost_clk_info x2000_ost_clk_info[] = {
>>      [OST_CLK_GLOBAL_TIMER] = {
>>          .init_data = {
>>              .name = "global timer",
>> @@ -207,6 +258,31 @@ static const struct ingenic_ost_clk_info 
>> ingenic_ost_clk_info[] = {
>>              .flags = CLK_SET_RATE_UNGATE,
>>          },
>>          .ostccr_reg = OST_REG_OSTCCR,
>> +        .ostcntl_reg = OST_REG_OSTCNTL,
>> +    },
>> +
>> +    [OST_CLK_PERCPU_TIMER0] = {
>> +        .init_data = {
>> +            .name = "percpu timer0",
>> +            .parent_names = ingenic_ost_clk_parents,
>> +            .num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
>> +            .ops = &ingenic_ost_percpu_timer_ops,
>> +            .flags = CLK_SET_RATE_UNGATE,
>> +        },
>> +        .idx = 0,
>> +        .ostccr_reg = OST_REG_OSTCCR,
>> +    },
>> +
>> +    [OST_CLK_PERCPU_TIMER1] = {
>> +        .init_data = {
>> +            .name = "percpu timer1",
>> +            .parent_names = ingenic_ost_clk_parents,
>> +            .num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
>> +            .ops = &ingenic_ost_percpu_timer_ops,
>> +            .flags = CLK_SET_RATE_UNGATE,
>> +        },
>> +        .idx = 1,
>> +        .ostccr_reg = OST_REG_OSTCCR,
>>      },
>>  };
>>
>> @@ -215,7 +291,7 @@ static u64 notrace 
>> ingenic_ost_global_timer_read_cntl(void)
>>      struct ingenic_ost *ost = ingenic_ost;
>>      unsigned int count;
>>
>> -    count = readl(ost->base + OST_REG_OST2CNTL);
>> +    count = readl(ost->base + ost->soc_info->clk_info->ostcntl_reg);
>>
>>      return count;
>>  }
>> @@ -225,16 +301,21 @@ static u64 notrace 
>> ingenic_ost_clocksource_read(struct clocksource *cs)
>>      return ingenic_ost_global_timer_read_cntl();
>>  }
>>
>> -static inline struct ingenic_ost *to_ingenic_ost(struct 
>> clock_event_device *evt)
>> +static inline struct ingenic_ost_timer *
>> +to_ingenic_ost_timer(struct clock_event_device *evt)
>>  {
>> -    return container_of(evt, struct ingenic_ost, cevt);
>> +    return container_of(evt, struct ingenic_ost_timer, cevt);
>>  }
>>
>>  static int ingenic_ost_cevt_set_state_shutdown(struct 
>> clock_event_device *evt)
>>  {
>> -    struct ingenic_ost *ost = to_ingenic_ost(evt);
>> +    struct ingenic_ost_timer *timer = to_ingenic_ost_timer(evt);
>> +    struct ingenic_ost *ost = ingenic_ost;
>
> You should avoid referencing the global 'ingenic_ost' variable if you 
> can. You could instead add a backpointer to the ingenic_ost structure 
> in your 'ingenic_ost_timer'.
>

Sure, I will try.


>>
>> -    writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
>> +    if (ost->soc_info->version >= ID_X2000)
>> +        writel(0, timer->base + OST_REG_OSTER);
>> +    else
>> +        writel(OSTECR_OST1ENC, timer->base + OST_REG_OSTECR);
>>
>>      return 0;
>>  }
>> @@ -242,26 +323,44 @@ static int 
>> ingenic_ost_cevt_set_state_shutdown(struct clock_event_device *evt)
>>  static int ingenic_ost_cevt_set_next(unsigned long next,
>>                       struct clock_event_device *evt)
>>  {
>> -    struct ingenic_ost *ost = to_ingenic_ost(evt);
>> +    struct ingenic_ost_timer *timer = to_ingenic_ost_timer(evt);
>> +    struct ingenic_ost *ost = ingenic_ost;
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&timer->lock, flags);
>>
>> -    writel((u32)~OSTFR_FFLAG, ost->base + OST_REG_OSTFR);
>> -    writel(next, ost->base + OST_REG_OST1DFR);
>> -    writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
>> -    writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTESR);
>> -    writel((u32)~OSTMR_FMASK, ost->base + OST_REG_OSTMR);
>> +    writel((u32)~OSTFR_FFLAG, timer->base + OST_REG_OSTFR);
>> +    writel(next, timer->base + OST_REG_OST1DFR);
>> +    writel(OSTCR_OST1CLR, timer->base + OST_REG_OSTCR);
>> +
>> +    if (ost->soc_info->version >= ID_X2000) {
>> +        writel(OSTESR_OST1ENS, timer->base + OST_REG_OSTER);
>> +    } else {
>> +        writel(OSTESR_OST1ENS, timer->base + OST_REG_OSTESR);
>> +        writel((u32)~OSTMR_FMASK, timer->base + OST_REG_OSTMR);
>> +    }
>> +
>> +    spin_unlock_irqrestore(&timer->lock, flags);
>>
>>      return 0;
>>  }
>>
>>  static irqreturn_t ingenic_ost_cevt_cb(int irq, void *dev_id)
>>  {
>> -    struct clock_event_device *evt = dev_id;
>> -    struct ingenic_ost *ost = to_ingenic_ost(evt);
>> +    struct ingenic_ost *ost = ingenic_ost;
>> +    struct ingenic_ost_timer *timer = dev_id;
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&timer->lock, flags);
>
> You're doing only one writel() here - I *think* it should be fine 
> without the spinlock.
>

Sure, I will drop it.


>> +
>> +    if (ost->soc_info->version >= ID_X2000)
>> +        writel(0, timer->base + OST_REG_OSTER);
>> +    else
>> +        writel(OSTECR_OST1ENC, timer->base + OST_REG_OSTECR);
>>
>> -    writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
>> +    spin_unlock_irqrestore(&timer->lock, flags);
>>
>> -    if (evt->event_handler)
>> -        evt->event_handler(evt);
>> +    timer->cevt.event_handler(&timer->cevt);
>>
>>      return IRQ_HANDLED;
>>  }
>> @@ -271,6 +370,7 @@ static int __init 
>> ingenic_ost_register_clock(struct ingenic_ost *ost,
>>              struct clk_hw_onecell_data *clocks)
>>  {
>>      struct ingenic_ost_clk *ost_clk;
>> +    struct ingenic_ost_timer *timer = per_cpu_ptr(ost->timers, 
>> info->idx);
>>      int val, err;
>>
>>      ost_clk = kzalloc(sizeof(*ost_clk), GFP_KERNEL);
>> @@ -283,9 +383,9 @@ static int __init 
>> ingenic_ost_register_clock(struct ingenic_ost *ost,
>>      ost_clk->ost = ost;
>>
>>      /* Reset clock divider */
>> -    val = readl(ost->base + info->ostccr_reg);
>> -    val &= ~(OSTCCR_PRESCALE1_MASK | OSTCCR_PRESCALE2_MASK);
>> -    writel(val, ost->base + info->ostccr_reg);
>> +    val = readl(timer->base + info->ostccr_reg);
>> +    val &= ~(OSTCCR_PRESCALE1_MASK);
>> +    writel(val, timer->base + info->ostccr_reg);
>>
>>      err = clk_hw_register(NULL, &ost_clk->hw);
>>      if (err) {
>> @@ -309,57 +409,53 @@ static struct clk * __init 
>> ingenic_ost_get_clock(struct device_node *np, int id)
>>      return of_clk_get_from_provider(&args);
>>  }
>>
>> -static int __init ingenic_ost_percpu_timer_init(struct device_node *np,
>> -                     struct ingenic_ost *ost)
>> +static int __init ingenic_ost_setup_cevt(unsigned int cpu)
>>  {
>> -    unsigned int timer_virq, channel = OST_CLK_PERCPU_TIMER;
>> +    struct ingenic_ost *ost = ingenic_ost;
>> +    struct ingenic_ost_timer *timer = this_cpu_ptr(ost->timers);
>>      unsigned long rate;
>>      int err;
>>
>> -    ost->percpu_timer_clk = ingenic_ost_get_clock(np, channel);
>> -    if (IS_ERR(ost->percpu_timer_clk))
>> -        return PTR_ERR(ost->percpu_timer_clk);
>> +    timer->clk = ingenic_ost_get_clock(ost->np, timer->channel);
>> +    if (IS_ERR(timer->clk))
>> +        return PTR_ERR(timer->clk);
>>
>> -    err = clk_prepare_enable(ost->percpu_timer_clk);
>> +    err = clk_prepare_enable(timer->clk);
>>      if (err)
>>          goto err_clk_put;
>>
>> -    rate = clk_get_rate(ost->percpu_timer_clk);
>> +    rate = clk_get_rate(timer->clk);
>>      if (!rate) {
>>          err = -EINVAL;
>>          goto err_clk_disable;
>>      }
>>
>> -    timer_virq = of_irq_get(np, 0);
>> -    if (!timer_virq) {
>> -        err = -EINVAL;
>> -        goto err_clk_disable;
>> -    }
>> +    snprintf(timer->name, sizeof(timer->name), "OST percpu timer%u", 
>> cpu);
>>
>> -    snprintf(ost->name, sizeof(ost->name), "OST percpu timer");
>> +    /* Unmask full comparison match interrupt */
>> +    writel((u32)~OSTMR_FMASK, timer->base + OST_REG_OSTMR);
>>
>> -    err = request_irq(timer_virq, ingenic_ost_cevt_cb, IRQF_TIMER,
>> -              ost->name, &ost->cevt);
>> -    if (err)
>> -        goto err_irq_dispose_mapping;
>> +    timer->cpu = smp_processor_id();
>> +    timer->cevt.cpumask = cpumask_of(smp_processor_id());
>> +    timer->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
>> +    timer->cevt.name = timer->name;
>> +    timer->cevt.rating = 400;
>> +    timer->cevt.set_state_shutdown = 
>> ingenic_ost_cevt_set_state_shutdown;
>> +    timer->cevt.set_next_event = ingenic_ost_cevt_set_next;
>> +
>> +    clockevents_config_and_register(&timer->cevt, rate, 4, 0xffffffff);
>>
>> -    ost->cevt.cpumask = cpumask_of(smp_processor_id());
>> -    ost->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
>> -    ost->cevt.name = ost->name;
>> -    ost->cevt.rating = 400;
>> -    ost->cevt.set_state_shutdown = ingenic_ost_cevt_set_state_shutdown;
>> -    ost->cevt.set_next_event = ingenic_ost_cevt_set_next;
>> +    if (ost->soc_info->version >= ID_X2000)
>> +        enable_percpu_irq(ost->irq, IRQ_TYPE_NONE);
>>
>> -    clockevents_config_and_register(&ost->cevt, rate, 4, 0xffffffff);
>> +    spin_lock_init(&timer->lock);
>>
>>      return 0;
>>
>> -err_irq_dispose_mapping:
>> -    irq_dispose_mapping(timer_virq);
>>  err_clk_disable:
>> -    clk_disable_unprepare(ost->percpu_timer_clk);
>> +    clk_disable_unprepare(timer->clk);
>>  err_clk_put:
>> -    clk_put(ost->percpu_timer_clk);
>> +    clk_put(timer->clk);
>>      return err;
>>  }
>>
>> @@ -385,11 +481,14 @@ static int __init 
>> ingenic_ost_global_timer_init(struct device_node *np,
>>          goto err_clk_disable;
>>      }
>>
>> -    /* Clear counter CNT registers */
>> -    writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
>> -
>> -    /* Enable OST channel */
>> -    writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
>> +    /* Clear counter CNT registers and enable OST channel */
>> +    if (ost->soc_info->version >= ID_X2000) {
>> +        writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
>> +        writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTER);
>> +    } else {
>> +        writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
>> +        writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
>> +    }
>>
>>      cs->name = "ingenic-ost";
>>      cs->rating = 400;
>> @@ -411,18 +510,33 @@ static int __init 
>> ingenic_ost_global_timer_init(struct device_node *np,
>>  }
>>
>>  static const struct ingenic_soc_info x1000_soc_info = {
>> +    .version = ID_X1000,
>> +    .clk_info = x1000_ost_clk_info,
>> +
>>      .num_channels = 2,
>>  };
>>
>> -static const struct of_device_id __maybe_unused 
>> ingenic_ost_of_match[] __initconst = {
>> -    { .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info, },
>> +static const struct ingenic_soc_info x2000_soc_info = {
>> +    .version = ID_X2000,
>> +    .clk_info = x2000_ost_clk_info,
>> +
>> +    .num_channels = 3,
>> +    .base_offset = 0x100,
>> +};
>> +
>> +static const struct of_device_id __maybe_unused 
>> ingenic_ost_of_matches[] __initconst = {
>> +    { .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info },
>> +    { .compatible = "ingenic,x2000-ost", .data = &x2000_soc_info },
>>      { /* sentinel */ }
>>  };
>>
>>  static int __init ingenic_ost_probe(struct device_node *np)
>>  {
>> -    const struct of_device_id *id = 
>> of_match_node(ingenic_ost_of_match, np);
>> +    const struct of_device_id *id = 
>> of_match_node(ingenic_ost_of_matches, np);
>
> Avoid any changes unrelated to the commit - for instance renaming 
> 'ingenic_ost_of_match' or removing the comma after the .data field, 
> there's just no need and it artificially grows the patch.
>

I thought about it this way: when "ingenic,x2000-ost" is added, there 
are two compatible strings, so it seems more appropriate to use matches 
than match, and removing the comma can reduce the code size a bit 
(though insignificant), I will split them into independent patch.


> Please be extra careful to make the smallest patch possible by not 
> adding unrelated changes; smaller patches are easier to review, and 
> are more likely to be accepted sooner.
>

Sure, thanks!


>> +    struct ingenic_ost_timer *timer;
>>      struct ingenic_ost *ost;
>> +    void __iomem *base;
>> +    unsigned int cpu;
>>      unsigned int i;
>>      int ret;
>>
>> @@ -430,18 +544,43 @@ static int __init ingenic_ost_probe(struct 
>> device_node *np)
>>      if (!ost)
>>          return -ENOMEM;
>>
>> +    ost->timers = alloc_percpu(struct ingenic_ost_timer);
>> +    if (!ost->timers) {
>> +        ret = -ENOMEM;
>> +        goto err_free_ost;
>> +    }
>> +
>> +    ost->np = np;
>> +    ost->soc_info = id->data;
>
> This was moved but didn't have to, as far as I can see.
>

Because we need to determine whether we need to obtain the base of (np, 
1) according to the model below, we need to move "ost->soc_info = 
id->data" here to ensure that relevant information is already stored in 
ost->soc_info when the judgment is made.


>> +
>>      ost->base = of_io_request_and_map(np, 0, of_node_full_name(np));
>>      if (IS_ERR(ost->base)) {
>>          pr_err("%s: Failed to map OST registers\n", __func__);
>>          ret = PTR_ERR(ost->base);
>> -        goto err_free_ost;
>> +        goto err_free_timers;
>> +    }
>> +
>> +    if (ost->soc_info->version >= ID_X2000) {
>> +        base = of_io_request_and_map(np, 1, of_node_full_name(np));
>> +        if (IS_ERR(base)) {
>> +            pr_err("%s: Failed to map OST registers\n", __func__);
>> +            ret = PTR_ERR(base);
>> +            goto err_free_timers;
>> +        }
>> +    }
>> +
>> +    ost->irq = irq_of_parse_and_map(np, 0);
>> +    if (ost->irq < 0) {
>> +        pr_crit("%s: Cannot to get OST IRQ\n", __func__);
>> +        ret = -EINVAL;
>
> ret = ost->irq?


I will change it in the next version.


>
>> +        goto err_free_timers;
>>      }
>>
>>      ost->clk = of_clk_get_by_name(np, "ost");
>>      if (IS_ERR(ost->clk)) {
>> -        ret = PTR_ERR(ost->clk);
>>          pr_crit("%s: Cannot get OST clock\n", __func__);
>> -        goto err_free_ost;
>> +        ret = PTR_ERR(ost->clk);
>> +        goto err_free_timers;
>>      }
>>
>>      ret = clk_prepare_enable(ost->clk);
>> @@ -450,8 +589,6 @@ static int __init ingenic_ost_probe(struct 
>> device_node *np)
>>          goto err_put_clk;
>>      }
>>
>> -    ost->soc_info = id->data;
>> -
>>      ost->clocks = kzalloc(struct_size(ost->clocks, hws, 
>> ost->soc_info->num_channels),
>>                    GFP_KERNEL);
>>      if (!ost->clocks) {
>> @@ -461,8 +598,20 @@ static int __init ingenic_ost_probe(struct 
>> device_node *np)
>>
>>      ost->clocks->num = ost->soc_info->num_channels;
>>
>> -    for (i = 0; i < ost->clocks->num; i++) {
>> -        ret = ingenic_ost_register_clock(ost, i, 
>> &ingenic_ost_clk_info[i], ost->clocks);
>> +    for (cpu = 0; cpu < num_possible_cpus(); cpu++) {
>> +        timer = per_cpu_ptr(ost->timers, cpu);
>> +
>> +        if (ost->soc_info->version >= ID_X2000)
>> +            timer->base = base + ost->soc_info->base_offset * cpu;
>> +        else
>> +            timer->base = ost->base;
>> +
>> +        timer->cpu = cpu;
>> +        timer->channel = OST_CLK_PERCPU_TIMER + cpu;
>> +    }
>> +
>> +    for (i = 0; i < num_possible_cpus() + 1; i++) {
>> +        ret = ingenic_ost_register_clock(ost, i, 
>> &ost->soc_info->clk_info[i], ost->clocks);
>
> This won't work. Your 'clk_info' array only has 2-3 entries. 
> num_possible_cpus() returns the maximum number of CPUs that is 
> supported by the kernel, which could be way more than the number of 
> cores you actually have. I think you want num_online_cpus() here, as 
> the number of online CPUs is guaranteed to be lower or equal to the 
> actual number of cores.


Sure, I will change it in the next version.


>
>>          if (ret) {
>>              pr_crit("%s: Cannot register clock %d\n", __func__, i);
>>              goto err_unregister_ost_clocks;
>> @@ -488,6 +637,8 @@ static int __init ingenic_ost_probe(struct 
>> device_node *np)
>>      clk_disable_unprepare(ost->clk);
>>  err_put_clk:
>>      clk_put(ost->clk);
>> +err_free_timers:
>> +    free_percpu(ost->timers);
>>  err_free_ost:
>>      kfree(ost);
>>      return ret;
>> @@ -517,9 +668,25 @@ static int __init ingenic_ost_init(struct 
>> device_node *np)
>>          goto err_free_ingenic_ost;
>>      }
>>
>> -    ret = ingenic_ost_percpu_timer_init(np, ost);
>> -    if (ret)
>> +    if (ost->soc_info->version >= ID_X2000)
>> +        ret = request_percpu_irq(ost->irq, ingenic_ost_cevt_cb,
>> +                  "OST percpu timer", ost->timers);
>> +    else
>> +        ret = request_irq(ost->irq, ingenic_ost_cevt_cb, IRQF_TIMER,
>> +                  "OST percpu timer", ost->timers);
>> +
>> +    if (ret) {
>> +        pr_crit("%s: Unable to request percpu IRQ: %x\n", __func__, 
>> ret);
>
> You mean %d?


My fault, will change it in next version.


>
>>          goto err_ost_global_timer_cleanup;
>> +    }
>> +
>> +    /* Setup clock events on each CPU core */
>> +    ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "Ingenic XBurst: 
>> online",
>> +                ingenic_ost_setup_cevt, NULL);
>> +    if (ret < 0) {
>> +        pr_crit("%s: Unable to init percpu timers: %x\n", __func__, 
>> ret);
>
> Same here.


Will change it in next version.


Thanks and best regards!


>
> Cheers,
> -Paul
>
>> +        goto err_ost_global_timer_cleanup;
>> +    }
>>
>>      /* Register the sched_clock at the end as there's no way to undo 
>> it */
>>      rate = clk_get_rate(ost->global_timer_clk);
>> @@ -537,3 +704,4 @@ static int __init ingenic_ost_init(struct 
>> device_node *np)
>>  }
>>
>>  TIMER_OF_DECLARE(x1000_ost,  "ingenic,x1000-ost", ingenic_ost_init);
>> +TIMER_OF_DECLARE(x2000_ost,  "ingenic,x2000-ost", ingenic_ost_init);
>> -- 
>> 2.7.4
>>
>
kernel test robot May 27, 2021, 1 a.m. UTC | #3
Hi "周琰杰,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/timers/core]
[also build test WARNING on linux/master linus/master v5.13-rc3 next-20210526]
[cannot apply to daniel.lezcano/clockevents/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Zhou-Yanjie/clocksource-Ingenic-Add-SMP-SMT-support-for-sysost-driver/20210526-012925
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 8ac91e6c6033ebc12c5c1e4aa171b81a662bd70f
config: microblaze-randconfig-s031-20210526 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/d613686c302a63ea0ed3a0b42f89401483642689
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zhou-Yanjie/clocksource-Ingenic-Add-SMP-SMT-support-for-sysost-driver/20210526-012925
        git checkout d613686c302a63ea0ed3a0b42f89401483642689
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=microblaze 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/clocksource/ingenic-sysost.c:676:58: sparse: sparse: incorrect type in argument 5 (different address spaces) @@     expected void *dev @@     got struct ingenic_ost_timer [noderef] __percpu *timers @@
   drivers/clocksource/ingenic-sysost.c:676:58: sparse:     expected void *dev
   drivers/clocksource/ingenic-sysost.c:676:58: sparse:     got struct ingenic_ost_timer [noderef] __percpu *timers

vim +676 drivers/clocksource/ingenic-sysost.c

   646	
   647	static int __init ingenic_ost_init(struct device_node *np)
   648	{
   649		struct ingenic_ost *ost;
   650		unsigned long rate;
   651		int ret;
   652	
   653		ret = ingenic_ost_probe(np);
   654		if (ret) {
   655			pr_crit("%s: Failed to initialize OST clocks: %d\n", __func__, ret);
   656			return ret;
   657		}
   658	
   659		of_node_clear_flag(np, OF_POPULATED);
   660	
   661		ost = ingenic_ost;
   662		if (IS_ERR(ost))
   663			return PTR_ERR(ost);
   664	
   665		ret = ingenic_ost_global_timer_init(np, ost);
   666		if (ret) {
   667			pr_crit("%s: Unable to init global timer: %x\n", __func__, ret);
   668			goto err_free_ingenic_ost;
   669		}
   670	
   671		if (ost->soc_info->version >= ID_X2000)
   672			ret = request_percpu_irq(ost->irq, ingenic_ost_cevt_cb,
   673					  "OST percpu timer", ost->timers);
   674		else
   675			ret = request_irq(ost->irq, ingenic_ost_cevt_cb, IRQF_TIMER,
 > 676					  "OST percpu timer", ost->timers);
   677	
   678		if (ret) {
   679			pr_crit("%s: Unable to request percpu IRQ: %x\n", __func__, ret);
   680			goto err_ost_global_timer_cleanup;
   681		}
   682	
   683		/* Setup clock events on each CPU core */
   684		ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "Ingenic XBurst: online",
   685					ingenic_ost_setup_cevt, NULL);
   686		if (ret < 0) {
   687			pr_crit("%s: Unable to init percpu timers: %x\n", __func__, ret);
   688			goto err_ost_global_timer_cleanup;
   689		}
   690	
   691		/* Register the sched_clock at the end as there's no way to undo it */
   692		rate = clk_get_rate(ost->global_timer_clk);
   693		sched_clock_register(ingenic_ost_global_timer_read_cntl, 32, rate);
   694	
   695		return 0;
   696	
   697	err_ost_global_timer_cleanup:
   698		clocksource_unregister(&ost->cs);
   699		clk_disable_unprepare(ost->global_timer_clk);
   700		clk_put(ost->global_timer_clk);
   701	err_free_ingenic_ost:
   702		kfree(ost);
   703		return ret;
   704	}
   705	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/clocksource/ingenic-sysost.c b/drivers/clocksource/ingenic-sysost.c
index e77d584..f35e0a3 100644
--- a/drivers/clocksource/ingenic-sysost.c
+++ b/drivers/clocksource/ingenic-sysost.c
@@ -13,6 +13,8 @@ 
 #include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/overflow.h>
 #include <linux/sched_clock.h>
 #include <linux/slab.h>
 #include <linux/syscore_ops.h>
@@ -21,10 +23,14 @@ 
 
 /* OST register offsets */
 #define OST_REG_OSTCCR			0x00
+#define OST_REG_OSTER			0x04
 #define OST_REG_OSTCR			0x08
 #define OST_REG_OSTFR			0x0c
+#define OST_REG_OSTCNTH			0x0c
 #define OST_REG_OSTMR			0x10
+#define OST_REG_OSTCNTL			0x10
 #define OST_REG_OST1DFR			0x14
+#define OST_REG_OSTCNTB			0x14
 #define OST_REG_OST1CNT			0x18
 #define OST_REG_OST2CNTL		0x20
 #define OST_REG_OSTCNT2HBUF		0x24
@@ -55,13 +61,24 @@ 
 #define OSTECR_OST1ENC			BIT(0)
 #define OSTECR_OST2ENC			BIT(1)
 
+enum ingenic_ost_version {
+	ID_X1000,
+	ID_X2000,
+};
+
 struct ingenic_soc_info {
+	enum ingenic_ost_version version;
+	const struct ingenic_ost_clk_info *clk_info;
+
 	unsigned int num_channels;
+	unsigned int base_offset;
 };
 
 struct ingenic_ost_clk_info {
 	struct clk_init_data init_data;
-	u8 ostccr_reg;
+	unsigned int idx;
+	u32 ostccr_reg;
+	u32 ostcntl_reg;
 };
 
 struct ingenic_ost_clk {
@@ -71,15 +88,27 @@  struct ingenic_ost_clk {
 	const struct ingenic_ost_clk_info *info;
 };
 
+struct ingenic_ost_timer {
+	void __iomem *base;
+	unsigned int cpu;
+	unsigned int channel;
+	struct clock_event_device cevt;
+	struct clk *clk;
+	char name[20];
+	spinlock_t	lock;
+};
+
 struct ingenic_ost {
 	void __iomem *base;
 	const struct ingenic_soc_info *soc_info;
-	struct clk *clk, *percpu_timer_clk, *global_timer_clk;
-	struct clock_event_device cevt;
+	struct clk *clk, *global_timer_clk;
+	struct device_node *np;
 	struct clocksource cs;
-	char name[20];
 
 	struct clk_hw_onecell_data *clocks;
+	struct ingenic_ost_timer __percpu *timers;
+
+	int irq;
 };
 
 static struct ingenic_ost *ingenic_ost;
@@ -94,9 +123,10 @@  static unsigned long ingenic_ost_percpu_timer_recalc_rate(struct clk_hw *hw,
 {
 	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
 	const struct ingenic_ost_clk_info *info = ost_clk->info;
+	struct ingenic_ost_timer *timer = per_cpu_ptr(ost_clk->ost->timers, info->idx);
 	unsigned int prescale;
 
-	prescale = readl(ost_clk->ost->base + info->ostccr_reg);
+	prescale = readl(timer->base + info->ostccr_reg);
 
 	prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> OSTCCR_PRESCALE1_LSB;
 
@@ -108,11 +138,12 @@  static unsigned long ingenic_ost_global_timer_recalc_rate(struct clk_hw *hw,
 {
 	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
 	const struct ingenic_ost_clk_info *info = ost_clk->info;
+	struct ingenic_ost_timer *timer = per_cpu_ptr(ost_clk->ost->timers, info->idx);
 	unsigned int prescale;
 
-	prescale = readl(ost_clk->ost->base + info->ostccr_reg);
+	prescale = readl(timer->base + info->ostccr_reg);
 
-	prescale = (prescale & OSTCCR_PRESCALE2_MASK) >> OSTCCR_PRESCALE2_LSB;
+	prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> OSTCCR_PRESCALE1_LSB;
 
 	return parent_rate >> (prescale * 2);
 }
@@ -147,12 +178,13 @@  static int ingenic_ost_percpu_timer_set_rate(struct clk_hw *hw, unsigned long re
 {
 	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
 	const struct ingenic_ost_clk_info *info = ost_clk->info;
+	struct ingenic_ost_timer *timer = per_cpu_ptr(ost_clk->ost->timers, info->idx);
 	u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
 	int val;
 
-	val = readl(ost_clk->ost->base + info->ostccr_reg);
+	val = readl(timer->base + info->ostccr_reg);
 	val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << OSTCCR_PRESCALE1_LSB);
-	writel(val, ost_clk->ost->base + info->ostccr_reg);
+	writel(val, timer->base + info->ostccr_reg);
 
 	return 0;
 }
@@ -162,12 +194,16 @@  static int ingenic_ost_global_timer_set_rate(struct clk_hw *hw, unsigned long re
 {
 	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
 	const struct ingenic_ost_clk_info *info = ost_clk->info;
+	struct ingenic_ost_timer *timer = per_cpu_ptr(ost_clk->ost->timers, info->idx);
 	u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
 	int val;
 
-	val = readl(ost_clk->ost->base + info->ostccr_reg);
-	val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << OSTCCR_PRESCALE2_LSB);
-	writel(val, ost_clk->ost->base + info->ostccr_reg);
+	val = readl(timer->base + info->ostccr_reg);
+	if (ost_clk->ost->soc_info->version >= ID_X2000)
+		val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << OSTCCR_PRESCALE1_LSB);
+	else
+		val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << OSTCCR_PRESCALE2_LSB);
+	writel(val, timer->base + info->ostccr_reg);
 
 	return 0;
 }
@@ -186,7 +222,19 @@  static const struct clk_ops ingenic_ost_global_timer_ops = {
 
 static const char * const ingenic_ost_clk_parents[] = { "ext" };
 
-static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
+static const struct ingenic_ost_clk_info x1000_ost_clk_info[] = {
+	[OST_CLK_GLOBAL_TIMER] = {
+		.init_data = {
+			.name = "global timer",
+			.parent_names = ingenic_ost_clk_parents,
+			.num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
+			.ops = &ingenic_ost_global_timer_ops,
+			.flags = CLK_SET_RATE_UNGATE,
+		},
+		.ostccr_reg = OST_REG_OSTCCR,
+		.ostcntl_reg = OST_REG_OST2CNTL,
+	},
+
 	[OST_CLK_PERCPU_TIMER] = {
 		.init_data = {
 			.name = "percpu timer",
@@ -195,9 +243,12 @@  static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
 			.ops = &ingenic_ost_percpu_timer_ops,
 			.flags = CLK_SET_RATE_UNGATE,
 		},
+		.idx = 0,
 		.ostccr_reg = OST_REG_OSTCCR,
 	},
+};
 
+static const struct ingenic_ost_clk_info x2000_ost_clk_info[] = {
 	[OST_CLK_GLOBAL_TIMER] = {
 		.init_data = {
 			.name = "global timer",
@@ -207,6 +258,31 @@  static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
 			.flags = CLK_SET_RATE_UNGATE,
 		},
 		.ostccr_reg = OST_REG_OSTCCR,
+		.ostcntl_reg = OST_REG_OSTCNTL,
+	},
+
+	[OST_CLK_PERCPU_TIMER0] = {
+		.init_data = {
+			.name = "percpu timer0",
+			.parent_names = ingenic_ost_clk_parents,
+			.num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
+			.ops = &ingenic_ost_percpu_timer_ops,
+			.flags = CLK_SET_RATE_UNGATE,
+		},
+		.idx = 0,
+		.ostccr_reg = OST_REG_OSTCCR,
+	},
+
+	[OST_CLK_PERCPU_TIMER1] = {
+		.init_data = {
+			.name = "percpu timer1",
+			.parent_names = ingenic_ost_clk_parents,
+			.num_parents = ARRAY_SIZE(ingenic_ost_clk_parents),
+			.ops = &ingenic_ost_percpu_timer_ops,
+			.flags = CLK_SET_RATE_UNGATE,
+		},
+		.idx = 1,
+		.ostccr_reg = OST_REG_OSTCCR,
 	},
 };
 
@@ -215,7 +291,7 @@  static u64 notrace ingenic_ost_global_timer_read_cntl(void)
 	struct ingenic_ost *ost = ingenic_ost;
 	unsigned int count;
 
-	count = readl(ost->base + OST_REG_OST2CNTL);
+	count = readl(ost->base + ost->soc_info->clk_info->ostcntl_reg);
 
 	return count;
 }
@@ -225,16 +301,21 @@  static u64 notrace ingenic_ost_clocksource_read(struct clocksource *cs)
 	return ingenic_ost_global_timer_read_cntl();
 }
 
-static inline struct ingenic_ost *to_ingenic_ost(struct clock_event_device *evt)
+static inline struct ingenic_ost_timer *
+to_ingenic_ost_timer(struct clock_event_device *evt)
 {
-	return container_of(evt, struct ingenic_ost, cevt);
+	return container_of(evt, struct ingenic_ost_timer, cevt);
 }
 
 static int ingenic_ost_cevt_set_state_shutdown(struct clock_event_device *evt)
 {
-	struct ingenic_ost *ost = to_ingenic_ost(evt);
+	struct ingenic_ost_timer *timer = to_ingenic_ost_timer(evt);
+	struct ingenic_ost *ost = ingenic_ost;
 
-	writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
+	if (ost->soc_info->version >= ID_X2000)
+		writel(0, timer->base + OST_REG_OSTER);
+	else
+		writel(OSTECR_OST1ENC, timer->base + OST_REG_OSTECR);
 
 	return 0;
 }
@@ -242,26 +323,44 @@  static int ingenic_ost_cevt_set_state_shutdown(struct clock_event_device *evt)
 static int ingenic_ost_cevt_set_next(unsigned long next,
 				     struct clock_event_device *evt)
 {
-	struct ingenic_ost *ost = to_ingenic_ost(evt);
+	struct ingenic_ost_timer *timer = to_ingenic_ost_timer(evt);
+	struct ingenic_ost *ost = ingenic_ost;
+	unsigned long flags;
+
+	spin_lock_irqsave(&timer->lock, flags);
 
-	writel((u32)~OSTFR_FFLAG, ost->base + OST_REG_OSTFR);
-	writel(next, ost->base + OST_REG_OST1DFR);
-	writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
-	writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTESR);
-	writel((u32)~OSTMR_FMASK, ost->base + OST_REG_OSTMR);
+	writel((u32)~OSTFR_FFLAG, timer->base + OST_REG_OSTFR);
+	writel(next, timer->base + OST_REG_OST1DFR);
+	writel(OSTCR_OST1CLR, timer->base + OST_REG_OSTCR);
+
+	if (ost->soc_info->version >= ID_X2000) {
+		writel(OSTESR_OST1ENS, timer->base + OST_REG_OSTER);
+	} else {
+		writel(OSTESR_OST1ENS, timer->base + OST_REG_OSTESR);
+		writel((u32)~OSTMR_FMASK, timer->base + OST_REG_OSTMR);
+	}
+
+	spin_unlock_irqrestore(&timer->lock, flags);
 
 	return 0;
 }
 
 static irqreturn_t ingenic_ost_cevt_cb(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = dev_id;
-	struct ingenic_ost *ost = to_ingenic_ost(evt);
+	struct ingenic_ost *ost = ingenic_ost;
+	struct ingenic_ost_timer *timer = dev_id;
+	unsigned long flags;
+
+	spin_lock_irqsave(&timer->lock, flags);
+
+	if (ost->soc_info->version >= ID_X2000)
+		writel(0, timer->base + OST_REG_OSTER);
+	else
+		writel(OSTECR_OST1ENC, timer->base + OST_REG_OSTECR);
 
-	writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
+	spin_unlock_irqrestore(&timer->lock, flags);
 
-	if (evt->event_handler)
-		evt->event_handler(evt);
+	timer->cevt.event_handler(&timer->cevt);
 
 	return IRQ_HANDLED;
 }
@@ -271,6 +370,7 @@  static int __init ingenic_ost_register_clock(struct ingenic_ost *ost,
 			struct clk_hw_onecell_data *clocks)
 {
 	struct ingenic_ost_clk *ost_clk;
+	struct ingenic_ost_timer *timer = per_cpu_ptr(ost->timers, info->idx);
 	int val, err;
 
 	ost_clk = kzalloc(sizeof(*ost_clk), GFP_KERNEL);
@@ -283,9 +383,9 @@  static int __init ingenic_ost_register_clock(struct ingenic_ost *ost,
 	ost_clk->ost = ost;
 
 	/* Reset clock divider */
-	val = readl(ost->base + info->ostccr_reg);
-	val &= ~(OSTCCR_PRESCALE1_MASK | OSTCCR_PRESCALE2_MASK);
-	writel(val, ost->base + info->ostccr_reg);
+	val = readl(timer->base + info->ostccr_reg);
+	val &= ~(OSTCCR_PRESCALE1_MASK);
+	writel(val, timer->base + info->ostccr_reg);
 
 	err = clk_hw_register(NULL, &ost_clk->hw);
 	if (err) {
@@ -309,57 +409,53 @@  static struct clk * __init ingenic_ost_get_clock(struct device_node *np, int id)
 	return of_clk_get_from_provider(&args);
 }
 
-static int __init ingenic_ost_percpu_timer_init(struct device_node *np,
-					 struct ingenic_ost *ost)
+static int __init ingenic_ost_setup_cevt(unsigned int cpu)
 {
-	unsigned int timer_virq, channel = OST_CLK_PERCPU_TIMER;
+	struct ingenic_ost *ost = ingenic_ost;
+	struct ingenic_ost_timer *timer = this_cpu_ptr(ost->timers);
 	unsigned long rate;
 	int err;
 
-	ost->percpu_timer_clk = ingenic_ost_get_clock(np, channel);
-	if (IS_ERR(ost->percpu_timer_clk))
-		return PTR_ERR(ost->percpu_timer_clk);
+	timer->clk = ingenic_ost_get_clock(ost->np, timer->channel);
+	if (IS_ERR(timer->clk))
+		return PTR_ERR(timer->clk);
 
-	err = clk_prepare_enable(ost->percpu_timer_clk);
+	err = clk_prepare_enable(timer->clk);
 	if (err)
 		goto err_clk_put;
 
-	rate = clk_get_rate(ost->percpu_timer_clk);
+	rate = clk_get_rate(timer->clk);
 	if (!rate) {
 		err = -EINVAL;
 		goto err_clk_disable;
 	}
 
-	timer_virq = of_irq_get(np, 0);
-	if (!timer_virq) {
-		err = -EINVAL;
-		goto err_clk_disable;
-	}
+	snprintf(timer->name, sizeof(timer->name), "OST percpu timer%u", cpu);
 
-	snprintf(ost->name, sizeof(ost->name), "OST percpu timer");
+	/* Unmask full comparison match interrupt */
+	writel((u32)~OSTMR_FMASK, timer->base + OST_REG_OSTMR);
 
-	err = request_irq(timer_virq, ingenic_ost_cevt_cb, IRQF_TIMER,
-			  ost->name, &ost->cevt);
-	if (err)
-		goto err_irq_dispose_mapping;
+	timer->cpu = smp_processor_id();
+	timer->cevt.cpumask = cpumask_of(smp_processor_id());
+	timer->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
+	timer->cevt.name = timer->name;
+	timer->cevt.rating = 400;
+	timer->cevt.set_state_shutdown = ingenic_ost_cevt_set_state_shutdown;
+	timer->cevt.set_next_event = ingenic_ost_cevt_set_next;
+
+	clockevents_config_and_register(&timer->cevt, rate, 4, 0xffffffff);
 
-	ost->cevt.cpumask = cpumask_of(smp_processor_id());
-	ost->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
-	ost->cevt.name = ost->name;
-	ost->cevt.rating = 400;
-	ost->cevt.set_state_shutdown = ingenic_ost_cevt_set_state_shutdown;
-	ost->cevt.set_next_event = ingenic_ost_cevt_set_next;
+	if (ost->soc_info->version >= ID_X2000)
+		enable_percpu_irq(ost->irq, IRQ_TYPE_NONE);
 
-	clockevents_config_and_register(&ost->cevt, rate, 4, 0xffffffff);
+	spin_lock_init(&timer->lock);
 
 	return 0;
 
-err_irq_dispose_mapping:
-	irq_dispose_mapping(timer_virq);
 err_clk_disable:
-	clk_disable_unprepare(ost->percpu_timer_clk);
+	clk_disable_unprepare(timer->clk);
 err_clk_put:
-	clk_put(ost->percpu_timer_clk);
+	clk_put(timer->clk);
 	return err;
 }
 
@@ -385,11 +481,14 @@  static int __init ingenic_ost_global_timer_init(struct device_node *np,
 		goto err_clk_disable;
 	}
 
-	/* Clear counter CNT registers */
-	writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
-
-	/* Enable OST channel */
-	writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
+	/* Clear counter CNT registers and enable OST channel */
+	if (ost->soc_info->version >= ID_X2000) {
+		writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
+		writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTER);
+	} else {
+		writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
+		writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
+	}
 
 	cs->name = "ingenic-ost";
 	cs->rating = 400;
@@ -411,18 +510,33 @@  static int __init ingenic_ost_global_timer_init(struct device_node *np,
 }
 
 static const struct ingenic_soc_info x1000_soc_info = {
+	.version = ID_X1000,
+	.clk_info = x1000_ost_clk_info,
+
 	.num_channels = 2,
 };
 
-static const struct of_device_id __maybe_unused ingenic_ost_of_match[] __initconst = {
-	{ .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info, },
+static const struct ingenic_soc_info x2000_soc_info = {
+	.version = ID_X2000,
+	.clk_info = x2000_ost_clk_info,
+
+	.num_channels = 3,
+	.base_offset = 0x100,
+};
+
+static const struct of_device_id __maybe_unused ingenic_ost_of_matches[] __initconst = {
+	{ .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info },
+	{ .compatible = "ingenic,x2000-ost", .data = &x2000_soc_info },
 	{ /* sentinel */ }
 };
 
 static int __init ingenic_ost_probe(struct device_node *np)
 {
-	const struct of_device_id *id = of_match_node(ingenic_ost_of_match, np);
+	const struct of_device_id *id = of_match_node(ingenic_ost_of_matches, np);
+	struct ingenic_ost_timer *timer;
 	struct ingenic_ost *ost;
+	void __iomem *base;
+	unsigned int cpu;
 	unsigned int i;
 	int ret;
 
@@ -430,18 +544,43 @@  static int __init ingenic_ost_probe(struct device_node *np)
 	if (!ost)
 		return -ENOMEM;
 
+	ost->timers = alloc_percpu(struct ingenic_ost_timer);
+	if (!ost->timers) {
+		ret = -ENOMEM;
+		goto err_free_ost;
+	}
+
+	ost->np = np;
+	ost->soc_info = id->data;
+
 	ost->base = of_io_request_and_map(np, 0, of_node_full_name(np));
 	if (IS_ERR(ost->base)) {
 		pr_err("%s: Failed to map OST registers\n", __func__);
 		ret = PTR_ERR(ost->base);
-		goto err_free_ost;
+		goto err_free_timers;
+	}
+
+	if (ost->soc_info->version >= ID_X2000) {
+		base = of_io_request_and_map(np, 1, of_node_full_name(np));
+		if (IS_ERR(base)) {
+			pr_err("%s: Failed to map OST registers\n", __func__);
+			ret = PTR_ERR(base);
+			goto err_free_timers;
+		}
+	}
+
+	ost->irq = irq_of_parse_and_map(np, 0);
+	if (ost->irq < 0) {
+		pr_crit("%s: Cannot to get OST IRQ\n", __func__);
+		ret = -EINVAL;
+		goto err_free_timers;
 	}
 
 	ost->clk = of_clk_get_by_name(np, "ost");
 	if (IS_ERR(ost->clk)) {
-		ret = PTR_ERR(ost->clk);
 		pr_crit("%s: Cannot get OST clock\n", __func__);
-		goto err_free_ost;
+		ret = PTR_ERR(ost->clk);
+		goto err_free_timers;
 	}
 
 	ret = clk_prepare_enable(ost->clk);
@@ -450,8 +589,6 @@  static int __init ingenic_ost_probe(struct device_node *np)
 		goto err_put_clk;
 	}
 
-	ost->soc_info = id->data;
-
 	ost->clocks = kzalloc(struct_size(ost->clocks, hws, ost->soc_info->num_channels),
 			      GFP_KERNEL);
 	if (!ost->clocks) {
@@ -461,8 +598,20 @@  static int __init ingenic_ost_probe(struct device_node *np)
 
 	ost->clocks->num = ost->soc_info->num_channels;
 
-	for (i = 0; i < ost->clocks->num; i++) {
-		ret = ingenic_ost_register_clock(ost, i, &ingenic_ost_clk_info[i], ost->clocks);
+	for (cpu = 0; cpu < num_possible_cpus(); cpu++) {
+		timer = per_cpu_ptr(ost->timers, cpu);
+
+		if (ost->soc_info->version >= ID_X2000)
+			timer->base = base + ost->soc_info->base_offset * cpu;
+		else
+			timer->base = ost->base;
+
+		timer->cpu = cpu;
+		timer->channel = OST_CLK_PERCPU_TIMER + cpu;
+	}
+
+	for (i = 0; i < num_possible_cpus() + 1; i++) {
+		ret = ingenic_ost_register_clock(ost, i, &ost->soc_info->clk_info[i], ost->clocks);
 		if (ret) {
 			pr_crit("%s: Cannot register clock %d\n", __func__, i);
 			goto err_unregister_ost_clocks;
@@ -488,6 +637,8 @@  static int __init ingenic_ost_probe(struct device_node *np)
 	clk_disable_unprepare(ost->clk);
 err_put_clk:
 	clk_put(ost->clk);
+err_free_timers:
+	free_percpu(ost->timers);
 err_free_ost:
 	kfree(ost);
 	return ret;
@@ -517,9 +668,25 @@  static int __init ingenic_ost_init(struct device_node *np)
 		goto err_free_ingenic_ost;
 	}
 
-	ret = ingenic_ost_percpu_timer_init(np, ost);
-	if (ret)
+	if (ost->soc_info->version >= ID_X2000)
+		ret = request_percpu_irq(ost->irq, ingenic_ost_cevt_cb,
+				  "OST percpu timer", ost->timers);
+	else
+		ret = request_irq(ost->irq, ingenic_ost_cevt_cb, IRQF_TIMER,
+				  "OST percpu timer", ost->timers);
+
+	if (ret) {
+		pr_crit("%s: Unable to request percpu IRQ: %x\n", __func__, ret);
 		goto err_ost_global_timer_cleanup;
+	}
+
+	/* Setup clock events on each CPU core */
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "Ingenic XBurst: online",
+				ingenic_ost_setup_cevt, NULL);
+	if (ret < 0) {
+		pr_crit("%s: Unable to init percpu timers: %x\n", __func__, ret);
+		goto err_ost_global_timer_cleanup;
+	}
 
 	/* Register the sched_clock at the end as there's no way to undo it */
 	rate = clk_get_rate(ost->global_timer_clk);
@@ -537,3 +704,4 @@  static int __init ingenic_ost_init(struct device_node *np)
 }
 
 TIMER_OF_DECLARE(x1000_ost,  "ingenic,x1000-ost",  ingenic_ost_init);
+TIMER_OF_DECLARE(x2000_ost,  "ingenic,x2000-ost",  ingenic_ost_init);