Re: [PATCH] clocksource: Ingenic: Add SMP/SMT support for sysost driver.

[Zhou Yanjie <zhouyanjie@wanyeetech.com>]

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
>>
>