Messages in this thread | | | Subject | Re: [PATCH] clocksource: Ingenic: Add SMP/SMT support for sysost driver. | From | Zhou Yanjie <> | Date | Thu, 27 May 2021 00:05:10 +0800 |
| |
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 >> >
| |