linux-kernel - Re: [RFC v2 5/7] clocksource/drivers/rtl8186: Add RTL8186 timer driver

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] [day] [month] [year] [list]

Date:   Sun, 18 Nov 2018 02:39:31 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Yasha Cherikovsky <yasha.che3@...il.com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Paul Burton <paul.burton@...s.com>,
        James Hogan <jhogan@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <marc.zyngier@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-mips@...ux-mips.org,
        devicetree@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [RFC v2 5/7] clocksource/drivers/rtl8186: Add RTL8186 timer
 driver


Hi Yasha,

except the few details below, the driver looks good to me.

On 01/10/2018 12:29, Yasha Cherikovsky wrote:
> The Realtek RTL8186 SoC is a MIPS based SoC
> used in some home routers [1][2].
> 
> This adds a driver to handle the built-in timers
> on this SoC.
> 
> Timers 0 and 1 are 24bit timers.
> Timers 2 and 3 are 32bit timers.
> 
> Use Timer2 as clocksource and Timer3 for clockevents.
> Timer2 is also used for sched_clock.
> 
> [1] https://www.linux-mips.org/wiki/Realtek_SOC#Realtek_RTL8186
> [2] https://wikidevi.com/wiki/Realtek_RTL8186
> 
> Signed-off-by: Yasha Cherikovsky <yasha.che3@...il.com>
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> Cc: Paul Burton <paul.burton@...s.com>
> Cc: James Hogan <jhogan@...nel.org>
> Cc: Daniel Lezcano <daniel.lezcano@...aro.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Rob Herring <robh+dt@...nel.org>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: linux-mips@...ux-mips.org
> Cc: devicetree@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> ---
>  drivers/clocksource/Kconfig         |   9 ++
>  drivers/clocksource/Makefile        |   1 +
>  drivers/clocksource/timer-rtl8186.c | 220 ++++++++++++++++++++++++++++
>  3 files changed, 230 insertions(+)
>  create mode 100644 drivers/clocksource/timer-rtl8186.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index dec0dd88ec15..da87f73d0631 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -609,4 +609,13 @@ config ATCPIT100_TIMER
>  	help
>  	  This option enables support for the Andestech ATCPIT100 timers.
>  
> +config RTL8186_TIMER
> +	bool "RTL8186 timer driver"
> +	depends on MACH_RTL8186
> +	depends on COMMON_CLK
> +	select TIMER_OF
> +	select CLKSRC_MMIO
> +	help
> +	  Enables support for the RTL8186 timer driver.
> +

Please, convert this entry like the MTK_TIMER or the SPRD_TIMER.

>  endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 00caf37e52f9..734e8566e1b6 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
>  obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
>  obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
> +obj-$(CONFIG_RTL8186_TIMER)		+= timer-rtl8186.o
> diff --git a/drivers/clocksource/timer-rtl8186.c b/drivers/clocksource/timer-rtl8186.c
> new file mode 100644
> index 000000000000..47ef4b09ad27
> --- /dev/null
> +++ b/drivers/clocksource/timer-rtl8186.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Realtek RTL8186 SoC timer driver.
> + *
> + * Timer0 (24bit): Unused
> + * Timer1 (24bit): Unused
> + * Timer2 (32bit): Used as clocksource
> + * Timer3 (32bit): Used as clock event device
> + *
> + * Copyright (C) 2018 Yasha Cherikovsky
> + */
> +
> +#include <linux/init.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/sched_clock.h>
> +#include <linux/of_clk.h>
> +#include <linux/io.h>
> +
> +#include <asm/time.h>
> +#include <asm/idle.h>

Why do you need those 2 includes above ?

> +#include "timer-of.h"
> +
> +/* Timer registers */
> +#define TCCNR			0x0
> +#define TCIR			0x4
> +#define TC_DATA(t)		(0x10 + 4 * (t))
> +#define TC_CNT(t)		(0x20 + 4 * (t))
> +
> +/* TCCNR register bits */
> +#define TCCNR_TC_EN_BIT(t)		BIT((t) * 2)
> +#define TCCNR_TC_MODE_BIT(t)		BIT((t) * 2 + 1)
> +#define TCCNR_TC_SRC_BIT(t)		BIT((t) + 8)
> +
> +/* TCIR register bits */
> +#define TCIR_TC_IE_BIT(t)		BIT(t)
> +#define TCIR_TC_IP_BIT(t)		BIT((t) + 4)
> +
> +
> +/* Forward declaration */
> +static struct timer_of to;
> +
> +static void __iomem *base;
> +
> +

nit: extra line

> +#define RTL8186_TIMER_MODE_COUNTER	0
> +#define RTL8186_TIMER_MODE_TIMER	1
> +

[ ... ]

Thanks

  -- 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