From: Guo Ren <ren_guo@c-sky.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org,
tglx@linutronix.de, jason@lakedaemon.net, arnd@arndb.de,
c-sky_gcc_upstream@c-sky.com, gnu-csky@mentor.com,
thomas.petazzoni@bootlin.com, wbx@uclibc-ng.org,
green.hu@gmail.com
Subject: Re: [PATCH V2 18/19] clocksource: add C-SKY clocksource drivers
Date: Thu, 5 Jul 2018 11:30:21 +0800 [thread overview]
Message-ID: <20180705033020.GA8023@guoren> (raw)
In-Reply-To: <bc765809-2d97-5ff8-d2ac-65479ce91ef2@linaro.org>
On Wed, Jul 04, 2018 at 07:05:05PM +0200, Daniel Lezcano wrote:
> > create mode 100644 drivers/clocksource/timer-csky-v1.c
> > create mode 100644 drivers/clocksource/timer-nationalchip.c
>
> Provide two separates patches, one for each timer.
Ok.
> > +obj-$(CONFIG_CSKY) += timer-csky-v1.o timer-nationalchip.o
>
> Split this in two.
>
> CONFIG_TIMER_CSKY += timer-csky.o
>
> Note, no v1.
>
> CONFIG_TIMER_NATCHIP += timer-natchip.o
>
>
> And in the Kconfig add the silent timer option.
>
> config TIMER_CSKY
> bool
>
> config TIMER_NATCHIP
> bool
>
> (If you want to keep the nationalchip name, I'm fine with that).
Ok, NATCHIP & timer-natchip :)
> > +static int csky_timer_irq;
> > +static int csky_timer_rate;
>
> You can get the value from the timer-of in all the places it is needed.
Ok, I could remove them.
But in csky_timer_v1_init: ret = timer_of_init(np, to)
We only init 1th cpu's timer_of struct, and others just static inited by:
DEFINE_PER_CPU(struct timer_of, csky_to) = {
.flags = TIMER_OF_CLOCK | TIMER_OF_IRQ,
.clkevt = {
.name = "C-SKY SMP Timer V1",
.rating = 300,
.features = CLOCK_EVT_FEAT_PERCPU | CLOCK_EVT_FEAT_ONESHOT,
.set_state_shutdown = csky_timer_shutdown,
.set_state_oneshot = csky_timer_oneshot,
.set_state_oneshot_stopped = csky_timer_oneshot_stopped,
.set_next_event = csky_timer_set_next_event,
},
.of_irq = {
.handler = timer_interrupt,
.flags = IRQF_TIMER,
.percpu = 1,
},
};
So I still need "for_each_cpu(cpu, cpu_possible_mask)" to init every
csky_to ...
> > +
> > + .clkevt = {
> > + .name = "C-SKY SMP Timer V1",
>
> If the node name is nice enough, you can discard this. See below
> TIMER_OF_DECLARE comment.
Ok, remove it :)
> > +static int csky_timer_starting_cpu(unsigned int cpu)
> > +{
> > + struct timer_of *to = this_cpu_ptr(&csky_to);
>
> per_cpu_ptr(&csky_to, cpu);
Ok, thx for the tip.
> > + to->clkevt.cpumask = cpumask_of(smp_processor_id());
>
> cpumask_of(cpu); ?
Yes.
> > + clockevents_config_and_register(&to->clkevt, csky_timer_rate, 0, ULONG_MAX);
>
> I suggest to choose something different than zero for the min_delta.
Agree, I'll use 1 :)
clockevents_config_and_register(&to->clkevt, csky_timer_rate, 1, ULONG_MAX);
> > +struct clocksource csky_clocksource = {
> > + .name = "csky_timer_v1_clksrc",
>
> "csky"
struct clocksource csky_clocksource = {
.name = "csky,mptimer",
Hmm?
> > + csky_clksrc_init();
>
> inline the function here. It is not worth to add a function for a couple
> of lines which is called one time.
Ok
> > +TIMER_OF_DECLARE(csky_timer_v1, "csky,timer-v1", csky_timer_v1_init);
>
> Stick to the hardware name.
>
> eg.
>
> TIMER_OF_DECLARE(csky_610, "csky,ck610-timer", csky_timer_init);
> TIMER_OF_DECLARE(csky_807, "csky,ck807-timer", csky_timer_init);
>
> (Beside /proc/interrupts will show the node name which states clearly
> what timer it is).
>
> ...
>
> v1, v2, etc ... has no sense here.
TIMER_OF_DECLARE(csky_610, "nationachip,ck610-timer", natchip_timer_init);
TIMER_OF_DECLARE(csky_807, "csky,ck807-timer", csky_timer_init);
TIMER_OF_DECLARE(csky_810, "csky,ck810-timer", csky_timer_init);
TIMER_OF_DECLARE(csky_860, "csky,ck860-timer", csky_timer_init);
TIMER_OF_DECLARE(csky_860mp, "csky,ck860-mptimer", csky_mptimer_init);
Hmm?
> > +#define STATUS_clr BIT(0)
> > +
> > +#define CONTRL_rst BIT(0)
> > +#define CONTRL_start BIT(1)
> > +
> > +#define CONFIG_en BIT(0)
> > +#define CONFIG_irq_en BIT(1)
>
> Prefix the macros with a shortened timer name and don't mix lower case
> and uppercase.
Ok.
#define STATUS_CLR BIT(0)
#define CONTRL_RST BIT(0)
#define CONTRL_START BIT(1)
#define CONFIG_EN BIT(0)
#define CONFIG_IRQ_EN BIT(1)
> NC_ is too short, something like NATCHIP may be better.
Ok, good name.
> > +static irqreturn_t timer_interrupt(int irq, void *dev)
>
> Fix the function name.
static irqreturn_t natchip_timer_interrupt(int irq, void *dev)
Hmm?
> > +static struct timer_of to = {
> > + .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
> > +
> > + .clkevt = {
> > + .name = TIMER_NAME,
>
> Let the node name.
Ok, remove it.
> > + .rating = 300,
> > + .features = CLOCK_EVT_FEAT_DYNIRQ | CLOCK_EVT_FEAT_PERIODIC |
> > + CLOCK_EVT_FEAT_ONESHOT,
> > + .set_state_shutdown = nc_timer_shutdown,
> > + .set_state_periodic = nc_timer_set_periodic,
> > + .set_next_event = nc_timer_set_next_event,
>
> set_oneshot ?
Yes oneshort, but also could support periodic. But in fact, it only
works with oneshort.
> > + .cpumask = cpu_possible_mask,
> > + },
> > +
> > + .of_irq = {
> > + .handler = timer_interrupt,
> > + .flags = IRQF_TIMER | IRQF_IRQPOLL,
> > + },
> > +};
> > +
> > +static u64 notrace nc_sched_clock_read(void)
> > +{
> > + void __iomem *base;
> > +
> > + base = timer_of_base(&to) + CLKSRC_OFFSET;
> > +
> > + return (u64) readl_relaxed(base + TIMER_VALUE);
> > +}
> > +
> > +static void nc_timer_set_div(void __iomem *base)
> > +{
> > + unsigned int div;
> > +
> > + div = timer_of_rate(&to)/TIMER_FREQ - 1;
>
> space ' / '
>
> Is it
> (timer_of_rate(&to) / TIMER_FREQ) - 1
> or
> timer_of_rate(&to) / (TIMER_FREQ - 1)
>
> ?
Thx, I'll modify it like this:
div = (timer_of_rate(&to) / TIMER_FREQ) - 1;
> > + clocksource_mmio_init(base + TIMER_VALUE, "nationalchip", TIMER_FREQ, 200, 32,
> > + clocksource_mmio_readl_up);
>
> return code check ?
Ok, add return code check.
> > +TIMER_OF_DECLARE(nc_timer, "nationalchip,timer-v1", nc_timer_init);
>
> same comment than cksy timer.
Ok.
Guo Ren
next prev parent reply other threads:[~2018-07-05 3:30 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-01 17:30 [PATCH V2 00/19] C-SKY(csky) Linux Kernel Port Guo Ren
2018-07-01 17:30 ` [PATCH V2 01/19] csky: Build infrastructure Guo Ren
2018-07-01 21:01 ` Randy Dunlap
2018-07-02 1:13 ` Guo Ren
2018-07-03 3:33 ` Rob Herring
2018-07-03 9:14 ` Guo Ren
2018-07-03 16:03 ` Arnd Bergmann
2018-07-04 11:40 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 02/19] csky: defconfig Guo Ren
2018-07-03 3:16 ` Rob Herring
2018-07-03 8:31 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 03/19] csky: Kernel booting Guo Ren
2018-07-01 17:30 ` [PATCH V2 04/19] csky: Exception handling Guo Ren
2018-07-01 17:30 ` [PATCH V2 05/19] csky: System Call Guo Ren
2018-07-03 19:53 ` Arnd Bergmann
2018-07-04 11:49 ` Guo Ren
2018-07-04 21:04 ` Arnd Bergmann
2018-07-05 5:38 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 06/19] csky: Cache and TLB routines Guo Ren
2018-07-05 17:40 ` Peter Zijlstra
2018-07-07 11:51 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 07/19] csky: MMU and page table management Guo Ren
2018-07-02 13:29 ` Christoph Hellwig
2018-07-03 2:53 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 08/19] csky: Process management and Signal Guo Ren
2018-07-01 17:30 ` [PATCH V2 09/19] csky: VDSO and rt_sigreturn Guo Ren
2018-07-01 17:30 ` [PATCH V2 10/19] csky: IRQ handling Guo Ren
2018-07-01 17:30 ` [PATCH V2 11/19] csky: Atomic operations Guo Ren
2018-07-05 17:50 ` Peter Zijlstra
2018-07-06 11:01 ` Guo Ren
2018-07-06 11:56 ` Peter Zijlstra
2018-07-06 12:17 ` Peter Zijlstra
2018-07-07 8:08 ` Guo Ren
2018-07-07 20:10 ` Andrea Parri
2018-07-08 1:05 ` Guo Ren
2018-07-07 7:42 ` Guo Ren
2018-07-07 19:54 ` Andrea Parri
2018-07-08 0:39 ` Guo Ren
2018-07-05 17:59 ` Peter Zijlstra
2018-07-06 11:44 ` Guo Ren
2018-07-06 12:03 ` Peter Zijlstra
2018-07-06 13:01 ` Peter Zijlstra
2018-07-06 14:06 ` Guo Ren
2018-07-05 18:00 ` Peter Zijlstra
2018-07-06 11:48 ` Guo Ren
2018-07-06 12:05 ` Peter Zijlstra
2018-07-06 13:46 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 12/19] csky: ELF and module probe Guo Ren
2018-07-01 17:30 ` [PATCH V2 13/19] csky: Library functions Guo Ren
2018-07-03 20:04 ` Arnd Bergmann
2018-07-04 10:51 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 14/19] csky: User access Guo Ren
2018-07-01 17:30 ` [PATCH V2 15/19] csky: Debug and Ptrace GDB Guo Ren
2018-07-01 17:30 ` [PATCH V2 16/19] csky: SMP support Guo Ren
2018-07-05 18:05 ` Peter Zijlstra
2018-07-06 6:07 ` Guo Ren
2018-07-06 9:39 ` Peter Zijlstra
2018-07-06 13:22 ` Guo Ren
2018-07-06 5:24 ` Mark Rutland
2018-07-06 11:32 ` Guo Ren
2018-07-06 11:43 ` Mark Rutland
2018-07-06 12:26 ` Guo Ren
2018-07-06 16:21 ` Mark Rutland
2018-07-07 6:16 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 17/19] csky: Misc headers Guo Ren
2018-07-01 17:30 ` [PATCH V2 18/19] clocksource: add C-SKY clocksource drivers Guo Ren
2018-07-01 17:34 ` Guo Ren
2018-07-03 9:39 ` Thomas Gleixner
2018-07-04 10:49 ` Guo Ren
2018-07-04 14:35 ` Thomas Gleixner
2018-07-05 5:03 ` Guo Ren
2018-07-04 17:05 ` Daniel Lezcano
2018-07-05 3:30 ` Guo Ren [this message]
2018-07-05 9:23 ` Daniel Lezcano
2018-07-06 5:57 ` Guo Ren
2018-07-01 17:30 ` [PATCH V2 19/19] irqchip: add C-SKY irqchip drivers Guo Ren
2018-07-03 3:27 ` Rob Herring
2018-07-03 7:38 ` Guo Ren
2018-07-03 9:28 ` Thomas Gleixner
2018-07-04 5:08 ` Guo Ren
2018-07-04 6:43 ` Thomas Gleixner
2018-07-04 11:58 ` Guo Ren
2018-07-11 9:51 ` [PATCH V2 00/19] C-SKY(csky) Linux Kernel Port David Howells
2018-07-12 12:51 ` Guo Ren
2018-07-12 16:04 ` Sandra Loosemore
2018-07-13 1:30 ` Guo Ren
2018-07-13 10:23 ` David Howells
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180705033020.GA8023@guoren \
--to=ren_guo@c-sky.com \
--cc=arnd@arndb.de \
--cc=c-sky_gcc_upstream@c-sky.com \
--cc=daniel.lezcano@linaro.org \
--cc=gnu-csky@mentor.com \
--cc=green.hu@gmail.com \
--cc=jason@lakedaemon.net \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=thomas.petazzoni@bootlin.com \
--cc=wbx@uclibc-ng.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).