On Mon, Oct 01, 2018 at 11:14:14AM +0200, Daniel Lezcano wrote:
> On 01/10/2018 03:35, Guo Ren wrote:
> > This timer is used by SMP system and use mfcr/mtcr instruction
> > to access the regs.
> >
> > Changelog:
> > - Remove #define CPUHP_AP_CSKY_TIMER_STARTING
> > - Add CPUHP_AP_CSKY_TIMER_STARTING in cpuhotplug.h
> > - Support csky mp timer alpha version.
> > - Just use low-counter with 32bit width as clocksource.
> > - Coding convention with upstream feed-back.
> >
> > Signed-off-by: Guo Ren <ren_guo@xxxxxxxxx>
> > ---
> > drivers/clocksource/Kconfig | 8 ++
> > drivers/clocksource/Makefile | 1 +
> > drivers/clocksource/csky_mptimer.c | 176 +++++++++++++++++++++++++++++++++++++
> > include/linux/cpuhotplug.h | 1 +
> > 4 files changed, 186 insertions(+)
> > create mode 100644 drivers/clocksource/csky_mptimer.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index a11f4ba..a28043f 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -620,4 +620,12 @@ config RISCV_TIMER
> > is accessed via both the SBI and the rdcycle instruction. This is
> > required for all RISC-V systems.
> >
> > +config CSKY_MPTIMER
> > + bool "C-SKY Multi Processor Timer"
> > + depends on CSKY
> > + select TIMER_OF
> > + help
> > + Say yes here to enable C-SKY SMP timer driver used for C-SKY SMP
> > + system.
> > +
> > endmenu
>
> The same rule applies here for COMPILE_TEST.
Ok
> And please rename the file: timer-mp-csky.c
Ok
> > + return (u64) mfcr(PTIM_CCVR);
>
> extra space
Ok
>
> > +}
> > +
> > +static u64 clksrc_read(struct clocksource *c)
> > +{
> > + return (u64) mfcr(PTIM_CCVR);
>
> extra space
Ok
> > + */
> > +
>
> extra line
Ok
>
> > + for_each_possible_cpu(cpu) {
> > + to = per_cpu_ptr(&csky_to, cpu);
> > +
> > + if (cpu == 0) {
> > + to->flags |= TIMER_OF_IRQ;
> > + to->of_irq.handler = timer_interrupt;
> > +
> > + ret = timer_of_init(np, to);
> > + if (ret)
> > + return ret;
> > +
> > + rate = timer_of_rate(to);
> > + irq = to->of_irq.irq;
> > + } else {
> > + ret = timer_of_init(np, to);
> > + if (ret)
> > + return ret;
> > +
> > + to->of_clk.rate = rate;
> > + to->of_irq.irq = irq;
> > + }
> > + }
>
>
> Isn't possible to replace this loop which is a bit confusing by:
>
> - no irq flag specified for timer-of
> - request irq before
Ok, but I've a bit different :)
> So we end up with:
>
> irq = irq_of_parse_and_map(np, 0);
> if (irq <= 0)
> return -EINVAL;
panic();
I want a panic here. Return will make debug confused and directly tell
the people where is wrong. It's root-timer for us and it must bootup.
If it's a co-timer, I also think return is good.
>
> ret = request_irq(irq, csky_timer_interrupt,
> IRQF_TIMER | IRQF_PERCPU,
> "csky_mp_timer", &csky_to));
ret = request_percpu_irq(irq, csky_timer_interrupt,
"csky_mp_timer", &csky_to);
I'll use request_percpu_irq the same as in timer_of and I've tested it's
Ok.
> if (ret)
> return ret;
Pls let me panic here.
>
> for_each_possible_cpu(cpu) {
> to = per_cpu_ptr(&csky_to, cpu);
> ret = timer_of_init(np, to);
> if (ret)
> goto rollback;
Pls let me panic here.
> }
Best Regards
Guo Ren
