| 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
| ||
|
Message-ID: <560DACD8.8000301@linaro.org>
Date: Thu, 1 Oct 2015 23:59:52 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Felipe Balbi <balbi@...com>, Tony Lindgren <tony@...mide.com>
Cc: Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
Linux ARM Kernel Mailing List
<linux-arm-kernel@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
tglx@...utronix.de, John Stultz <john.stultz@...aro.org>
Subject: Re: [RFC/PATCH 09/11] clocksource: add TI 32.768 Hz counter driver
Hi Felipe,
On 09/29/2015 10:44 PM, Felipe Balbi wrote:
> Introduce a new clocksource driver for Texas
> Instruments 32.768 Hz device which is available
> on most OMAP-like devices.
>
> Signed-off-by: Felipe Balbi <balbi@...com>
> ---
[ ... ]
> +config CLKSRC_TI_32K
> + bool "Texas Instruments 32.768 Hz Clocksource"
> + depends on OF && ARCH_OMAP2PLUS
> + select CLKSRC_OF
> + help
> + This option enables support for Texas Instruments 32.768 Hz clocksource
> + available on many OMAP-like platforms.
> +
It is the omap's Kconfig which should select the timer, not the user to
enable the timer.
It should be something like:
config CLKSRC_TI_32K
bool "Texas Instruments 32.768 Hz Clocksource" if COMPILE_TEST
select CLKSRC_OF if OF
help
This option enables support for Texas Instruments 32.768 Hz
clocksource available on many OMAP-like platforms.
And in the omap's Kconfig:
select CLKSRC_TI_32K
> config CLKSRC_STM32
> bool "Clocksource for STM32 SoCs" if !ARCH_STM32
> depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST)
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 5c00863c3e33..749abc3665b3 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_VF_PIT_TIMER) += vf_pit_timer.o
> obj-$(CONFIG_CLKSRC_QCOM) += qcom-timer.o
> obj-$(CONFIG_MTK_TIMER) += mtk_timer.o
> obj-$(CONFIG_CLKSRC_PISTACHIO) += time-pistachio.o
> +obj-$(CONFIG_CLKSRC_TI_32K) += timer-ti-32k.o
>
> obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
> obj-$(CONFIG_ARM_GLOBAL_TIMER) += arm_global_timer.o
> diff --git a/drivers/clocksource/timer-ti-32k.c b/drivers/clocksource/timer-ti-32k.c
> new file mode 100644
> index 000000000000..10ccce2eb645
> --- /dev/null
> +++ b/drivers/clocksource/timer-ti-32k.c
> @@ -0,0 +1,121 @@
> +/**
> + * timer-ti-32k.c - OMAP2 32k Timer Support
> + *
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/time.h>
> +#include <linux/interrupt.h>
> +#include <linux/err.h>
> +#include <linux/irq.h>
> +#include <linux/sched_clock.h>
> +#include <linux/clocksource.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#include <asm/mach/time.h>
Can you check all these headers are needed ? I don't think interrupt.h,
or slab.h or irq.h, etc ... are needed.
A few nits below:
> +/* OMAP2_32KSYNCNT_CR_OFF: offset of 32ksync counter register */
I am not sure this comment gives some interesting indication.
> +#define OMAP2_32KSYNCNT_REV_OFF 0x0
> +#define OMAP2_32KSYNCNT_REV_SCHEME (0x3 << 30)
> +#define OMAP2_32KSYNCNT_CR_OFF_LOW 0x10
> +#define OMAP2_32KSYNCNT_CR_OFF_HIGH 0x30
> +
> +/*
> + * 32KHz clocksource ... always available, on pretty most chips except
> + * OMAP 730 and 1510. Other timers could be used as clocksources, with
> + * higher resolution in free-running counter modes (e.g. 12 MHz xtal),
> + * but systems won't necessarily want to spend resources that way.
> + */
> +static void __iomem *sync32k_cnt_reg;
> +
> +/**
> + * omap_read_persistent_clock64 - Return time from a persistent clock.
> + *
> + * Reads the time from a source which isn't disabled during PM, the
> + * 32k sync timer. Convert the cycles elapsed since last read into
> + * nsecs and adds to a monotonically increasing timespec64.
> + */
This comment above should be moved right before the function it
describes and in order to comply with the kernel-doc format the
parameters must be documented also:
...
* @ts: ....
...
> +static struct timespec64 persistent_ts;
> +static cycles_t cycles;
> +static unsigned int persistent_mult, persistent_shift;
> +
> +static u64 notrace omap_32k_read_sched_clock(void)
> +{
> + return readl_relaxed(sync32k_cnt_reg);
> +}
> +
> +static void omap_read_persistent_clock64(struct timespec64 *ts)
> +{
> + unsigned long long nsecs;
> + cycles_t last_cycles;
> +
> + last_cycles = cycles;
> + cycles = sync32k_cnt_reg ? readl_relaxed(sync32k_cnt_reg) : 0;
This test is pointless because 'sync32k_cnt_reg' is always different
from zero regarding the init routine, no ?
> +
> + nsecs = clocksource_cyc2ns(cycles - last_cycles,
> + persistent_mult, persistent_shift);
> +
> + timespec64_add_ns(&persistent_ts, nsecs);
> +
> + *ts = persistent_ts;
> +}
> +
> +static void __init ti_32k_timer_init(struct device_node *np)
> +{
> + void __iomem *vbase;
> + int ret;
> +
> + vbase = of_iomap(np, 0);
> + if (!vbase) {
> + pr_err("Can't ioremap 32k timer base\n");
> + return;
> + }
> +
> + /*
> + * 32k sync Counter IP register offsets vary between the
> + * highlander version and the legacy ones.
> + * The 'SCHEME' bits(30-31) of the revision register is used
> + * to identify the version.
> + */
Please fix comment length.
> + if (readl_relaxed(vbase + OMAP2_32KSYNCNT_REV_OFF) &
> + OMAP2_32KSYNCNT_REV_SCHEME)
> + sync32k_cnt_reg = vbase + OMAP2_32KSYNCNT_CR_OFF_HIGH;
> + else
> + sync32k_cnt_reg = vbase + OMAP2_32KSYNCNT_CR_OFF_LOW;
> +
> + /*
> + * 120000 rough estimate from the calculations in
> + * __clocksource_update_freq_scale.
> + */
Fix comment length also.
> + clocks_calc_mult_shift(&persistent_mult, &persistent_shift,
> + 32768, NSEC_PER_SEC, 120000);
> +
> + ret = clocksource_mmio_init(sync32k_cnt_reg, "32k_counter", 32768,
> + 250, 32, clocksource_mmio_readl_up);
> + if (ret) {
> + pr_err("32k_counter: can't register clocksource\n");
> + return;
> + }
> +
> + sched_clock_register(omap_32k_read_sched_clock, 32, 32768);
> + register_persistent_clock(NULL, omap_read_persistent_clock64);
I will let John Stultz to have a look at this part because I have doubt
regarding the usage of the persistent clock.
-- Daniel
--
<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
--
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/
Powered by blists - more mailing lists