Re: [PATCH V9 3/8] clocksource: add C-SKY SMP timer

[Guo Ren]

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