Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

[Greentime Hu]

Hi, Daniel:

2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>:
> On 12/12/2017 06:46, Rick Chen wrote:
>> ATCPIT100 is often used on the Andes architecture,
>> This timer provide 4 PIT channels. Each PIT channel is a
>> multi-function timer, can be configured as 32,16,8 bit timers
>> or PWM as well.
>>
>> For system timer it will set channel 1 32-bit timer0 as clock
>> source and count downwards until underflow and restart again.
>
> [ ... ]
>
>> +config CLKSRC_ATCPIT100
>> +     bool "Clocksource for AE3XX platform"
>> +     depends on NDS32 || COMPILE_TEST
>> +     depends on HAS_IOMEM
>> +     help
>> +       This option enables support for the Andestech AE3XX platform timers.
>
> Hi Rick,
>
> the general rule for the Kconfig is:
>
> bool "Clocksource for AE3XX platform" if COMPILE_TEST
>
> and no deps on the platform.
>
> It is up to the platform Kconfig to select the option.
>
> We want here a silent option but make it selectable in case of
> compilation test coverage.


The way we like to use it is because
1. This timer is a basic component to boot an nds32 CPU and it should
be able to select without COMPILE_TEST for nds32 architecture.
2. It seems conflict with debug info. I am not sure if there is
another way to debug kernel(with debug info) with COMPILE_TEST and
DEBUG_INFO because we need this driver for nds32 architecture.

Symbol: DEBUG_INFO [=n]
Type  : boolean
Prompt: Compile the kernel with debug info
  Location:
    -> Kernel hacking
      -> Compile-time checks and compiler options
  Defined at lib/Kconfig.debug:140
  Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]


> Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100
> to TIMER_ATCPIT100.

Thanks. We will rename it in the next version patch.

>> +
>>  endmenu
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index 72711f1..7d072f5 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -75,3 +75,4 @@ obj-$(CONFIG_H8300_TMR16)           += h8300_timer16.o
>>  obj-$(CONFIG_H8300_TPU)                      += h8300_tpu.o
>>  obj-$(CONFIG_CLKSRC_ST_LPC)          += clksrc_st_lpc.o
>>  obj-$(CONFIG_X86_NUMACHIP)           += numachip.o
>> +obj-$(CONFIG_CLKSRC_ATCPIT100)               += timer-atcpit100.o
>
> [ ... ]
>
>> +static struct timer_of to = {
>> +     .flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE,
>> +
>> +     .clkevt = {
>> +             .name = "atcpit100_tick",
>> +             .rating = 300,
>> +             .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
>> +             .set_state_shutdown = atcpit100_clkevt_shutdown,
>> +             .set_state_periodic = atcpit100_clkevt_set_periodic,
>> +             .set_state_oneshot = atcpit100_clkevt_set_oneshot,
>> +             .tick_resume = atcpit100_clkevt_shutdown,
>> +             .set_next_event = atcpit100_clkevt_next_event,
>> +             .cpumask = cpu_all_mask,
>
> You may consider CLOCK_EVT_DYN_IRQ
> https://lwn.net/Articles/540160/

Thanks.
We will consider to implement this feature once we support SMP or our
CPU has local timer.

>> +     },
>> +
>> +     .of_irq = {
>> +             .handler = atcpit100_timer_interrupt,
>> +             .flags = IRQF_TIMER | IRQF_IRQPOLL,
>> +     },
>> +
>> +     /*
>> +      * FIXME: we currently only support clocking using PCLK
>> +      * and using EXTCLK is not supported in the driver.
>> +      */
>> +     .of_clk = {
>> +             .name = "PCLK",
>> +     }
>
> What do you mean ? We can't specify several clock names with timer-of?

It means we currently support PCLK only.
https://lkml.org/lkml/2017/12/1/316

Thanks.