linux-kernel - Re: [PATCH 33/36] AArch64: Generic timers support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <12a7f9b8e6d0b4dc3e3f6fcf91ad2a18@localhost>
Date:	Thu, 12 Jul 2012 12:09:24 +0200
From:	Marc Zyngier <marc.zyngier@....com>
To:	Linus Walleij <linus.ml.walleij@...il.com>
Cc:	Catalin Marinas <catalin.marinas@....com>,
	<linux-kernel@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>,
	Will Deacon <will.deacon@....com>,
	Mike Turquette <mike.turquette@...aro.org>,
	Marc Zyngier <maz@...terjones.org>
Subject: Re: [PATCH 33/36] AArch64: Generic timers support

On Thu, 12 Jul 2012 02:18:42 +0200, Linus Walleij
<linus.ml.walleij@...il.com> wrote:

Hi Linus,

> I'm reviewing the only patch I really understand...
> 
> 2012/7/6 Catalin Marinas <catalin.marinas@....com>:
> 
>> +/* This isn't really used any more */
>> +#define CLOCK_TICK_RATE 1000
> 
> Is it still necessary to even have it there?

It is used as part of the LATCH/TICK_* computation in
include/linux/jiffies.h. It seems that any value could do, actually, and it
only seem to be used in kernel/time/ntp.c. Any guidance on this much
appreciated.

By the way, there is a very interesting comment about this in
arch/ia64/include/asm/timex.h.

>> +       /* Try to determine the frequency from the device tree or
CNTFRQ
>> */
>> +       if (!of_property_read_u32(np, "clock-frequency", &freq))
>> +               arch_timer_rate = freq;
> 
> Have you documented the bindings for this thing?

It has been documented as part of the architected timer patch that has
been merged for 3.5. See
Documentation/devicetree/bindings/arm/arch_timer.txt.

>> +       arch_timer_calibrate();
> 
> Why is the ability to get this from a clk not contemplated?
> I think this will be common. You could make it optional I think,
> just like in the SMP TWD.

_calibrate() is a misnomer here. It should really be _get_freq(). But your
point still stand, and we could indeed use a clk to obtain the frequency.

It should probably be selected in the following order: clock-frequency
(DT), clk, CNTFRQ.

>> diff --git a/include/clocksource/arm_generic.h
>> b/include/clocksource/arm_generic.h
>> new file mode 100644
>> index 0000000..6933f8e
> (...)
>> +#ifndef __ASM_ARCH_TIMER_H
>> +#define __ASM_ARCH_TIMER_H
> 
> This #ifdef name looks confusing, what about
> #ifndef __CLKSOURCE_ARM_GENERIC_H?
> 
> I noticed you already saw the problem with naming it "arch timers"
> and renamed it to "arm generic" so a leftover here :)

Thanks! :-)

        M.
-- 
Fast, cheap, reliable. Pick two.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/