[U-Boot] [RFC PATCH 3/6] rockchip: timer: add device-model timer driver for RK3368 (and similar)

[Dr. Philipp Tomsich]

> On 01 Aug 2017, at 11:48, Simon Glass <sjg at chromium.org> wrote:
> 
> On 28 July 2017 at 10:31, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com> wrote:
>> This adds a device-model driver for the timer block in the RK3368 (and
>> similar devices that share the same timer block, such as the RK3288) for
>> the down-counting (i.e. non-secure) timers.
>> 
>> This allows us to configure U-Boot for the RK3368 in such a way that
>> we can run with the secure timer inaccessible or uninitialised (note
>> that the ARMv8 generic timer does not count, if the secure timer is
>> not enabled).
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>> ---
>> 
>> drivers/timer/Kconfig          |   7 +++
>> drivers/timer/Makefile         |   1 +
>> drivers/timer/rockchip_timer.c | 105 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 113 insertions(+)
>> create mode 100644 drivers/timer/rockchip_timer.c
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> See a few comments below.
> 
>> 
>> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
>> index fb5af4d..5dea404 100644
>> --- a/drivers/timer/Kconfig
>> +++ b/drivers/timer/Kconfig
>> @@ -104,4 +104,11 @@ config AE3XX_TIMER
>>        help
>>          Select this to enable a timer for AE3XX devices.
>> 
>> +config ROCKCHIP_TIMER
>> +        bool "Rockchip timer support"
>> +       depends on TIMER
>> +       help
>> +         Select this to enable support for the timer block found on
> 
> I'm not so keen on the word 'block'. Maybe it should be 'IP block'? Or
> perhaps don't mention block at all and say that this is the timer.
> 
>> +         Rockchip devices.
>> +
>> endmenu
>> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
>> index d16ea53..fa7ce7c 100644
>> --- a/drivers/timer/Makefile
>> +++ b/drivers/timer/Makefile
>> @@ -14,3 +14,4 @@ obj-$(CONFIG_STI_TIMER)               += sti-timer.o
>> obj-$(CONFIG_ARC_TIMER)        += arc_timer.o
>> obj-$(CONFIG_AG101P_TIMER) += ag101p_timer.o
>> obj-$(CONFIG_AE3XX_TIMER) += ae3xx_timer.o
>> +obj-$(CONFIG_ROCKCHIP_TIMER) += rockchip_timer.o
>> diff --git a/drivers/timer/rockchip_timer.c b/drivers/timer/rockchip_timer.c
>> new file mode 100644
>> index 0000000..bffb630
>> --- /dev/null
>> +++ b/drivers/timer/rockchip_timer.c
>> @@ -0,0 +1,105 @@
>> +/*
>> + * Copyright (C) 2017 Theobroma Systems Design und Consulting GmbH
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#define DEBUG
> 
> Can we drop this?
> 
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <mapmem.h>
>> +#include <asm/arch/timer.h>
>> +#include <dt-structs.h>
>> +#include <timer.h>
>> +#include <asm/io.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +struct rockchip_timer_plat {
>> +       struct dtd_rockchip_rk3368_timer dtd;
>> +};
>> +#endif
>> +
>> +/* Driver private data. Contains timer id. Could be either 0 or 1. */
>> +struct rockchip_timer_priv {
>> +       struct rk_timer *timer;
>> +};
>> +
>> +static int rockchip_timer_get_count(struct udevice *dev, u64 *count)
>> +{
>> +       struct rockchip_timer_priv *priv = dev_get_priv(dev);
>> +       uint64_t timebase_h, timebase_l;
>> +       uint64_t cntr;
>> +
>> +       timebase_l = readl(&priv->timer->timer_curr_value0);
>> +       timebase_h = readl(&priv->timer->timer_curr_value1);
>> +
>> +       /* timers are down-counting */
>> +       cntr = timebase_h << 32 | timebase_l;
>> +       *count = 0xffffffffull - cntr;
>> +       return 0;
>> +}
>> +
>> +static int rockchip_clk_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA)
>> +       struct rockchip_timer_priv *priv = dev_get_priv(dev);
>> +
>> +       priv->timer = (struct rk_timer *)devfdt_get_addr(dev);
>> +#endif
>> +
>> +       return 0;
>> +}
>> +
>> +static int rockchip_timer_start(struct udevice *dev)
>> +{
>> +       struct rockchip_timer_priv *priv = dev_get_priv(dev);
>> +
>> +       writel(0, &priv->timer->timer_ctrl_reg);
>> +       writel(0xffffffff, &priv->timer->timer_load_count0);
>> +       writel(0xffffffff, &priv->timer->timer_load_count1);
>> +       writel(1, &priv->timer->timer_ctrl_reg);
>> +
>> +       return 0;
>> +}
>> +
>> +static int rockchip_timer_probe(struct udevice *dev)
>> +{
>> +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>> +       const u32 MHz = 1000000;
>> +
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +       struct rockchip_timer_priv *priv = dev_get_priv(dev);
>> +       struct rockchip_timer_plat *plat = dev_get_platdata(dev);
>> +
>> +       priv->timer = map_sysmem(plat->dtd.reg[1], plat->dtd.reg[3]);
>> +#endif
>> +       uc_priv->clock_rate = 24 * MHz;
> 
> If it is not enabled, doesn't this come from the DT?

Unfortunately not, as the DTS neither defines a parent-clk nor a clock-frequency.
This should indeed not be here — I’ll add a change that fixes the DTS.

Thanks for highlighting this!

>> +
>> +       return rockchip_timer_start(dev);
>> +}
>> +
>> +static const struct timer_ops rockchip_timer_ops = {
>> +       .get_count = rockchip_timer_get_count,
>> +};
>> +
>> +static const struct udevice_id rockchip_timer_ids[] = {
>> +       { .compatible = "rockchip,rk3368-timer" },
>> +       {}
>> +};
>> +
>> +U_BOOT_DRIVER(arc_timer) = {
>> +       .name   = "rockchip_rk3368_timer",
>> +       .id     = UCLASS_TIMER,
>> +       .of_match = rockchip_timer_ids,
>> +       .probe = rockchip_timer_probe,
>> +       .ops    = &rockchip_timer_ops,
>> +       .flags = DM_FLAG_PRE_RELOC,
>> +       .priv_auto_alloc_size = sizeof(struct rockchip_timer_priv),
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +       .platdata_auto_alloc_size = sizeof(struct rockchip_timer_plat),
>> +#endif
>> +       .ofdata_to_platdata = rockchip_clk_ofdata_to_platdata,
>> +};
>> --
>> 2.1.4
>> 
> 
> Regards,
> Simon