On 05/07/2018 05:30, Guo Ren wrote:
[ ... ]
>>
>> 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 ...
That is what is unclear for me. percpu or IRQF_PERCPU ?
Have a look at the commit 9995f4f184613fb02ee73092b03545520a72b104,
changelog and the comment in the init function.
Can you give a similar description for this timer ?
[ ... ]
>>> +struct clocksource csky_clocksource = {
>>> + .name = "csky_timer_v1_clksrc",
>>
>> "csky"
> struct clocksource csky_clocksource = {
> .name = "csky,mptimer",
> Hmm?
Yes
[ ... ]
>> 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?
Yep, fine.
>>> +#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)
NATCHIP_STATUS_CLR
NATCHIP_CONTROL_RST
NATCHIP_CONTROL_START
NATCHIP_CONFIG_EN
NATCHIP_CONFIG_IRQ_EN
>> 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?
Fine.
>>> +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.
In the flags, it is specified periodic and oneshot but only the
set_periodic ops is set.
[ ... ]
>>> + 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;
I wanted to be sure it wasn't the latter. In this case, you don't need
parenthesis, so just add the spaces around the '/' operator.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
