lkml.org 
[lkml]   [2016]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 04/17] clocksource: Add PLX Technology RPS Timer
    From
    Date
    Hi Neil,

    On 03/03/2016 12:39 PM, Neil Armstrong wrote:
    > Add clocksource and clockevent driver from dual RPS timer.

    please elaborate the log: describe the timer, how it works, is there a
    pointer to doc, freq, etc ...

    > CC: Ma Haijun <mahaijuns@gmail.com>
    > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
    > ---
    > drivers/clocksource/Kconfig | 6 +
    > drivers/clocksource/Makefile | 1 +
    > drivers/clocksource/timer-rps.c | 249 ++++++++++++++++++++++++++++++++++++++++
    > 3 files changed, 256 insertions(+)
    > create mode 100644 drivers/clocksource/timer-rps.c
    >
    > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
    > index 33db740..f79bc0f 100644
    > --- a/drivers/clocksource/Kconfig
    > +++ b/drivers/clocksource/Kconfig
    > @@ -276,6 +276,12 @@ config VF_PIT_TIMER
    > help
    > Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
    >
    > +config CLKSRC_RPS_TIMER
    > + bool
    > + select CLKSRC_MMIO
    > + help
    > + This enables support for the PLX Tech OXNAS RPS timers.
    > +

    Please check DIGICOLOR_TIMER or DW_APB_TIMER or ROCKCHIP_TIMER or
    ARMADA_370_XP_TIMER or ... config section and try to stick to the same
    pattern.

    > config SYS_SUPPORTS_SH_CMT
    > bool
    >
    > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
    > index dc2b899..120bc09 100644
    > --- a/drivers/clocksource/Makefile
    > +++ b/drivers/clocksource/Makefile
    > @@ -46,6 +46,7 @@ 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_CLKSRC_RPS_TIMER) += timer-rps.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-rps.c b/drivers/clocksource/timer-rps.c
    > new file mode 100644
    > index 0000000..79621b8
    > --- /dev/null
    > +++ b/drivers/clocksource/timer-rps.c
    > @@ -0,0 +1,249 @@
    > +/*
    > + * drivers/clocksource/timer-rps.c
    > + *
    > + * Copyright (C) 2009 Oxford Semiconductor Ltd
    > + * Copyright (C) 2013 Ma Haijun <mahaijuns@gmail.com>
    > + * Copyright (C) 2016 Neil Armstrong <narmstrong@baylibre.com>
    > + *
    > + * This program is free software; you can redistribute it and/or modify it
    > + * under the terms and conditions of the GNU General Public License,
    > + * version 2, as published by the Free Software Foundation.
    > + *
    > + * This program is distributed in the hope 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/interrupt.h>
    > +#include <linux/irq.h>
    > +#include <linux/io.h>
    > +#include <linux/clockchips.h>
    > +#include <linux/clk.h>
    > +#include <linux/of_irq.h>
    > +#include <linux/of_address.h>
    > +#include <linux/sched_clock.h>
    > +
    > +/* TIMER1 as tick
    > + * TIMER2 as clocksource
    > + */

    Comment format:

    /*
    * TIMER1 used as tick
    * TIMER2 used as clocksource
    */

    > +enum {
    > + TIMER_LOAD = 0,
    > + TIMER_CURR = 4,
    > + TIMER_CTRL = 8,
    > + TIMER_CLRINT = 0xC,
    > +
    > + TIMER_BITS = 24,
    > +
    > + TIMER_MAX_VAL = (1 << TIMER_BITS) - 1,
    > +
    > + TIMER_PERIODIC = (1 << 6),
    > + TIMER_ENABLE = (1 << 7),
    > +
    > + TIMER_DIV1 = (0 << 2),
    > + TIMER_DIV16 = (1 << 2),
    > + TIMER_DIV256 = (2 << 2),
    > +
    > + TIMER1_OFFSET = 0,
    > + TIMER2_OFFSET = 0x20,
    > +};

    What is the point of having this unorganized enum with duplicate values
    ? and why TIMER_DIV1 = (0 << 2) ?

    Replace by #define.

    > +/* Clockevent */
    > +
    > +static unsigned long timer_period = HZ;
    > +static unsigned timer_prescaler = 1;
    > +static void __iomem *timer_base;

    Encapsulate these variables into a structure and use container_of.

    > +
    > +static irqreturn_t rps_timer_irq(int irq, void *dev_id)
    > +{
    > + struct clock_event_device *evt = dev_id;
    > +
    > + iowrite32(0, timer_base + TIMER_CLRINT);
    > +
    > + evt->event_handler(evt);
    > +
    > + return IRQ_HANDLED;
    > +}
    > +
    > +static void rps_timer_config(unsigned long period, unsigned periodic)
    > +{
    > + uint32_t cfg = 0;
    > +
    > + if (period)
    > + cfg |= TIMER_ENABLE;
    > +
    > + if (periodic)
    > + cfg |= TIMER_PERIODIC;
    > +
    > + switch (timer_prescaler) {
    > + case 1:
    > + cfg |= TIMER_DIV1;
    > + break;
    > + case 16:
    > + cfg |= TIMER_DIV16;
    > + break;
    > + case 256:
    > + cfg |= TIMER_DIV256;
    > + break;
    > + }

    No direct value (1, 16, 256) but macros please.

    > + iowrite32(period, timer_base + TIMER_LOAD);
    > + iowrite32(cfg, timer_base + TIMER_CTRL);
    > +}
    > +
    > +static int rps_timer_shutdown(struct clock_event_device *evt)
    > +{
    > + if (!clockevent_state_periodic(evt))
    > + return 0;
    > +
    > + rps_timer_config(0, 0);
    > +
    > + return 0;
    > +}
    > +
    > +static int rps_timer_set_periodic(struct clock_event_device *evt)
    > +{
    > + rps_timer_config(timer_period, 1);
    > +
    > + return 0;
    > +}
    > +
    > +static int rps_timer_set_oneshot(struct clock_event_device *evt)
    > +{
    > + rps_timer_config(timer_period, 0);
    > +
    > + return 0;
    > +}
    > +
    > +static int rps_timer_next_event(unsigned long delta,
    > + struct clock_event_device *evt)
    > +{
    > + rps_timer_config(delta, 0);
    > +
    > + return 0;
    > +}
    > +
    > +static struct clock_event_device rps_clockevent = {
    > + .name = "rps",
    > + .features = CLOCK_EVT_FEAT_PERIODIC |
    > + CLOCK_EVT_FEAT_ONESHOT,
    > + .tick_resume = rps_timer_shutdown,
    > + .set_state_shutdown = rps_timer_shutdown,
    > + .set_state_periodic = rps_timer_set_periodic,
    > + .set_state_oneshot = rps_timer_set_oneshot,
    > + .set_next_event = rps_timer_next_event,
    > + .rating = 200,
    > +};
    > +
    > +static void __init rps_clockevent_init(void __iomem *base, ulong ref_rate,
    > + int irq)
    > +{
    > + timer_base = base;
    > +
    > + /* Start with prescaler 1 */
    > + timer_prescaler = 1;
    > + timer_period = DIV_ROUND_UP(ref_rate, HZ);
    > +
    > + if (timer_period > TIMER_MAX_VAL) {
    > + timer_prescaler = 16;
    > + timer_period = DIV_ROUND_UP(ref_rate / timer_prescaler, HZ);

    Using macro and bit shifting will be nicer.

    > + }
    > + if (timer_period > TIMER_MAX_VAL) {
    > + timer_prescaler = 256;
    > + timer_period = DIV_ROUND_UP(ref_rate / timer_prescaler, HZ);
    > + }
    > + rps_clockevent.cpumask = cpu_possible_mask;
    > + rps_clockevent.irq = irq;

    So if you are using such cpumask, I suggest you have a look at the
    DYNIRQ flag.

    > + clockevents_config_and_register(&rps_clockevent,
    > + ref_rate / timer_prescaler,
    > + 1,
    > + TIMER_MAX_VAL);
    > +
    > + pr_info("rps: Registered clock event rate %luHz prescaler %d period %lu\n",
    > + ref_rate,
    > + timer_prescaler,
    > + timer_period);
    > +}
    > +
    > +/* Clocksource */
    > +
    > +static void __iomem *timer_curr;
    > +
    > +static u64 notrace rps_read_sched_clock(void)
    > +{
    > + return ~readl_relaxed(timer_curr);
    > +}
    > +
    > +static void __init rps_clocksource_init(void __iomem *base, ulong ref_rate)
    > +{
    > + int ret;
    > + ulong clock_rate;
    > + /* use prescale 16 */
    > + clock_rate = ref_rate / 16;
    > +
    > + iowrite32(TIMER_MAX_VAL, base + TIMER_LOAD);
    > + iowrite32(TIMER_PERIODIC | TIMER_ENABLE | TIMER_DIV16,
    > + base + TIMER_CTRL);
    > +
    > + timer_curr = base + TIMER_CURR;
    > + sched_clock_register(rps_read_sched_clock, TIMER_BITS, clock_rate);
    > + ret = clocksource_mmio_init(base + TIMER_CURR, "rps_clocksource_timer",
    > + clock_rate, 250, TIMER_BITS,
    > + clocksource_mmio_readl_down);
    > + if (ret)
    > + panic("can't register clocksource\n");

    No panic, just an error.

    > + pr_info("rps: Registered clocksource rate %luHz\n", clock_rate);
    > +}
    > +
    > +static struct irqaction rps_timer_irqaction = {
    > + .name = "rps_timer",
    > + .flags = IRQF_TIMER | IRQF_IRQPOLL,
    > + .handler = rps_timer_irq,
    > + .dev_id = &rps_clockevent,
    > +};
    > +
    > +static void __init rps_timer_init(struct device_node *np)
    > +{
    > + struct clk *refclk;
    > + unsigned long ref_rate;
    > + void __iomem *base;
    > + int irq, ret;
    > +
    > + refclk = of_clk_get(np, 0);
    > +
    > + if (IS_ERR(refclk) || clk_prepare_enable(refclk))
    > + panic("rps_timer_init: failed to get refclk\n");

    no panic.

    > + ref_rate = clk_get_rate(refclk);
    > +
    > + base = of_iomap(np, 0);
    > + if (!base)
    > + panic("rps_timer_init: failed to map io\n");
    > +
    > + irq = irq_of_parse_and_map(np, 0);
    > + if (irq < 0)
    > + panic("rps_timer_init: failed to parse IRQ\n");

    if (!irq)
    no panic.

    > +
    > + /* Disable timers */
    > + iowrite32(0, base + TIMER1_OFFSET + TIMER_CTRL);
    > + iowrite32(0, base + TIMER2_OFFSET + TIMER_CTRL);
    > + iowrite32(0, base + TIMER1_OFFSET + TIMER_LOAD);
    > + iowrite32(0, base + TIMER2_OFFSET + TIMER_LOAD);
    > + iowrite32(0, base + TIMER1_OFFSET + TIMER_CLRINT);
    > + iowrite32(0, base + TIMER2_OFFSET + TIMER_CLRINT);
    > +
    > + rps_clocksource_init(base + TIMER2_OFFSET, ref_rate);
    > + rps_clockevent_init(base + TIMER1_OFFSET, ref_rate, irq);
    > +
    > + ret = setup_irq(irq, &rps_timer_irqaction);

    Replace setup_irq by request_irq.

    > + if (ret)
    > + panic("rps_timer_init: failed to request irq\n");
    > +}
    > +
    > +CLOCKSOURCE_OF_DECLARE(nas782x, "plxtech,nas782x-rps-timer", rps_timer_init);


    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

    \
     
     \ /
      Last update: 2016-03-17 17:41    [W:4.764 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site