Re: [PATCH 09/17] clocksource/timer-riscv: New RISC-V Clocksource

[Arnd Bergmann]

On Wed, Jun 7, 2017 at 9:12 AM, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> CC clocksource folks
>
> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@xxxxxxxxxxx> wrote:
>> The RISC-V ISA defines a single RTC as well as an SBI oneshot timer.
>> This timer is present on all RISC-V systems.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@xxxxxxxxxxx>
>> ---
>>  drivers/clocksource/Kconfig       |   8 +++
>>  drivers/clocksource/Makefile      |   1 +
>>  drivers/clocksource/timer-riscv.c | 118 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 127 insertions(+)
>>  create mode 100644 drivers/clocksource/timer-riscv.c
>>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 545d541ae20e..1c2c6e7c7fab 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -612,4 +612,12 @@ config CLKSRC_ST_LPC
>>           Enable this option to use the Low Power controller timer
>>           as clocksource.
>>
>> +config CLKSRC_RISCV
>> +       #bool "Clocksource for the RISC-V platform"
>> +       def_bool y if RISCV
>> +       depends on RISCV

I don't like the commenting out parts of the entry. If there are no
build-time dependencies, you can just make it 'default y' and still allow
users to disabled the driver if they really want to (e.g. on a machine
specific kernel that has a driver for another clocksource), or you
just leave it 'def_bool RISCV'.

>> +
>> +static int riscv_timer_set_oneshot(struct clock_event_device *evt)
>> +{
>> +       /* no-op; only one mode */
>> +       return 0;
>> +}
>> +
>> +static int riscv_timer_set_shutdown(struct clock_event_device *evt)
>> +{
>> +       /* can't stop the clock! */
>> +       return 0;
>> +}

I'd just leave out the empty callbacks, the callers all protect NULL
pointers.

>> +static u64 riscv_rdtime(struct clocksource *cs)
>> +{
>> +       return get_cycles();
>> +}
>> +
>> +static struct clocksource riscv_clocksource = {
>> +       .name = "riscv_clocksource",
>> +       .rating = 300,
>> +       .read = riscv_rdtime,
>> +#ifdef CONFIG_64BITS
>> +       .mask = CLOCKSOURCE_MASK(64),
>> +#else
>> +       .mask = CLOCKSOURCE_MASK(32),
>> +#endif /* CONFIG_64BITS */
>> +       .flags = CLOCK_SOURCE_IS_CONTINUOUS,
>> +};

     ".mask = BITS_PER_LONG" maybe?

>> +void riscv_timer_interrupt(void)
>> +{
>> +       int cpu = smp_processor_id();
>> +       struct clock_event_device *evdev = &per_cpu(clock_event, cpu);
>> +
>> +       evdev->event_handler(evdev);
>> +}
>> +
>> +void __init init_clockevent(void)
>> +{
>> +       int cpu = smp_processor_id();
>> +       struct clock_event_device *ce = &per_cpu(clock_event, cpu);
>> +
>> +       *ce = (struct clock_event_device){
>> +               .name = "riscv_timer_clockevent",
>> +               .features = CLOCK_EVT_FEAT_ONESHOT,
>> +               .rating = 300,
>> +               .cpumask = cpumask_of(cpu),
>> +               .set_next_event = riscv_timer_set_next_event,
>> +               .set_state_oneshot  = riscv_timer_set_oneshot,
>> +               .set_state_shutdown = riscv_timer_set_shutdown,
>> +       };
>> +
>> +       /* Enable timer interrupts */
>> +       csr_set(sie, SIE_STIE);
>> +
>> +       clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff);
>> +}
>> +
>> +static unsigned long __init of_timebase(void)
>> +{
>> +       struct device_node *cpu;
>> +       const __be32 *prop;
>> +
>> +       cpu = of_find_node_by_path("/cpus");
>> +       if (cpu) {
>> +               prop = of_get_property(cpu, "timebase-frequency", NULL);
>> +               if (prop)
>> +                       return be32_to_cpu(*prop);

of_property_read_u32()

>> +       }
>> +
>> +       return 10000000;

The default seems rather arbitrary. Any reason for this particular
number? Maybe it's better to fail if the property is missing.

       Arnd