[PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver
Eugen.Hristev at microchip.com
Eugen.Hristev at microchip.com
Thu Mar 3 10:31:14 CET 2022
Hello Clement,
Thank you for your patch.
On 2/2/22 4:42 PM, Clément Léger wrote:
> Add a driver for the timer counter block that can be found on sama5d2.
> This driver will be used when booting under OP-TEE since the pit timer
> which is part of the SYSC is secured. Channel 1 & 2 are configured to
> be chained together which allows to have a 64bits counter.
>
> Signed-off-by: Clément Léger <clement.leger at bootlin.com>
> ---
> MAINTAINERS | 1 +
> drivers/timer/Kconfig | 7 ++
> drivers/timer/Makefile | 1 +
> drivers/timer/atmel_tcb_timer.c | 160 ++++++++++++++++++++++++++++++++
> 4 files changed, 169 insertions(+)
> create mode 100644 drivers/timer/atmel_tcb_timer.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dcdd99e368..6ff95aea39 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -343,6 +343,7 @@ F: arch/arm/mach-at91/
> F: board/atmel/
> F: drivers/cpu/at91_cpu.c
> F: drivers/misc/microchip_flexcom.c
> +F: drivers/timer/atmel_tcb_timer.c
> F: include/dt-bindings/mfd/atmel-flexcom.h
> F: drivers/timer/mchp-pit64b-timer.c
>
> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> index 8913142654..9b7bbec654 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -96,6 +96,13 @@ config ATMEL_PIT_TIMER
> it is designed to offer maximum accuracy and efficient management,
> even for systems with long response time.
>
> +config ATMEL_TCB_TIMER
> + bool "Atmel timer counter support"
> + depends on TIMER
> + help
> + Select this to enable the use of the timer counter as a monotonic
> + counter.
maybe you should specify that this is specific to at91 architecture
> +
> config CADENCE_TTC_TIMER
> bool "Cadence TTC (Triple Timer Counter)"
> depends on TIMER
> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
> index e2bd530eb0..58da6c1e84 100644
> --- a/drivers/timer/Makefile
> +++ b/drivers/timer/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_ARC_TIMER) += arc_timer.o
> obj-$(CONFIG_AST_TIMER) += ast_timer.o
> obj-$(CONFIG_ATCPIT100_TIMER) += atcpit100_timer.o
> obj-$(CONFIG_ATMEL_PIT_TIMER) += atmel_pit_timer.o
> +obj-$(CONFIG_ATMEL_TCB_TIMER) += atmel_tcb_timer.o
> obj-$(CONFIG_CADENCE_TTC_TIMER) += cadence-ttc.o
> obj-$(CONFIG_DESIGNWARE_APB_TIMER) += dw-apb-timer.o
> obj-$(CONFIG_MPC83XX_TIMER) += mpc83xx_timer.o
> diff --git a/drivers/timer/atmel_tcb_timer.c b/drivers/timer/atmel_tcb_timer.c
> new file mode 100644
> index 0000000000..6f2c054629
> --- /dev/null
> +++ b/drivers/timer/atmel_tcb_timer.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2022 Microchip Corporation
> + *
> + * Author: Clément Léger <clement.leger at bootlin.com>
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <timer.h>
> +#include <asm/io.h>
> +#include <linux/bitops.h>
> +
> +#define TCB_CHAN(chan) ((chan) * 0x40)
> +
> +#define TCB_CCR(chan) (0x0 + TCB_CHAN(chan))
> +#define TCB_CCR_SWTRG 0x4
> +#define TCB_CCR_CLKEN 0x1
> +
> +#define TCB_CMR(chan) (0x4 + TCB_CHAN(chan))
> +#define TCB_CMR_WAVE BIT(15)
> +#define TCB_CMR_TIMER_CLOCK0 0
> +#define TCB_CMR_XC1 6
> +#define TCB_CMR_ACPA_SET (1 << 16)
> +#define TCB_CMR_ACPC_CLEAR (2 << 18)
> +
> +#define TCB_CV(chan) (0x10 + TCB_CHAN(chan))
> +
> +#define TCB_RA(chan) (0x14 + TCB_CHAN(chan))
> +#define TCB_RB(chan) (0x18 + TCB_CHAN(chan))
> +#define TCB_RC(chan) (0x1c + TCB_CHAN(chan))
> +
> +#define TCB_IER(chan) (0x24 + TCB_CHAN(chan))
> +#define TCB_IER_COVFS 0x1
> +
> +#define TCB_SR(chan) (0x20 + TCB_CHAN(chan))
> +#define TCB_SR_COVFS 0x1
Some of these defines are unused. It is better to remove them for simplicity
> +
> +#define TCB_IDR(chan) (0x28 + TCB_CHAN(chan))
> +
> +#define TCB_BCR 0xc0
> +#define TCB_BCR_SYNC 0x1
> +
> +#define TCB_BMR 0xc4
> +#define TCB_BMR_TC1XC1S_TIOA0 (2 << 2)
> +
> +#define TCB_WPMR 0xe4
> +#define TCB_WPMR_WAKEY 0x54494d
> +
> +struct atmel_tcb_plat {
> + void __iomem *base;
> +};
> +
> +static u64 atmel_tcb_get_count(struct udevice *dev)
> +{
> + struct atmel_tcb_plat *plat = dev_get_plat(dev);
> + u64 cv0 = 0;
> + u64 cv1 = 0;
> +
> + do {
> + cv1 = readl(plat->base + TCB_CV(1));
> + cv0 = readl(plat->base + TCB_CV(0));
> + } while (readl(plat->base + TCB_CV(1)) != cv1);
Are you reading cv1 multiple times without storing the cv1 value ?
And I don't really like this loop. What happens if after reading again
CV(1) it's always different from the stored cv1 value ?
> +
> + cv0 |= cv1 << 32;
> +
> + return cv0;
> +}
> +
> +static void atmel_tcb_configure(void __iomem *base)
> +{
> + /* Disable write protection */
> + writel(TCB_WPMR_WAKEY, base + TCB_WPMR);
> +
> + /* Disable all irqs for both channel 0 & 1 */
> + writel(0xff, base + TCB_IDR(0));
> + writel(0xff, base + TCB_IDR(1));
> +
> + /*
> + * In order to avoid wrapping, use a 64 bit counter by chaining
> + * two channels.
> + * Channel 0 is configured to generate a clock on TIOA0 which is cleared
> + * when reaching 0x80000000 and set when reaching 0.
> + */
> + writel(TCB_CMR_TIMER_CLOCK0 | TCB_CMR_WAVE | TCB_CMR_ACPA_SET
> + | TCB_CMR_ACPC_CLEAR, base + TCB_CMR(0));
> + writel(0x80000000, base + TCB_RC(0));
> + writel(0x1, base + TCB_RA(0));
> + writel(TCB_CCR_CLKEN, base + TCB_CCR(0));
> +
> + /* Channel 1 is configured to use TIOA0 as input */
> + writel(TCB_CMR_XC1 | TCB_CMR_WAVE, base + TCB_CMR(1));
> + writel(TCB_CCR_CLKEN, base + TCB_CCR(1));
> +
> + /* Set XC1 input to be TIOA0 (ie output of Channel 0) */
> + writel(TCB_BMR_TC1XC1S_TIOA0, base + TCB_BMR);
> +
> + /* Sync & start all timers */
> + writel(TCB_BCR_SYNC, base + TCB_BCR);
> +}
> +
> +static int atmel_tcb_probe(struct udevice *dev)
> +{
> + struct atmel_tcb_plat *plat = dev_get_plat(dev);
> + struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> + struct clk clk;
> + ulong clk_rate;
> + int ret;
> +
> + if (!device_is_compatible(dev->parent, "atmel,sama5d2-tcb"))
> + return -EINVAL;
I find it odd that you check the compatible for a parent of this device
when this device is compatible only with 'atmel,tcb-timer' .
I would expect to have a driver specific for atmel,sama5d2-tcb and probe
it accordingly.
> +
> + /* Currently, we only support channel 0 and 1 to be chained */
> + if (dev_read_addr_index(dev, 0) != 0 &&
> + dev_read_addr_index(dev, 1) != 1)
> + return -EINVAL;
In here maybe we should at least emit a debug message to log on the
console, if we do not support different things.
> +
> + ret = clk_get_by_name(dev->parent, "gclk", &clk);
> + if (ret)
> + return -EINVAL;
> +
> + clk_rate = clk_get_rate(&clk);
> + if (!clk_rate)
> + return -EINVAL;
> +
> + uc_priv->clock_rate = clk_rate;
> +
> + atmel_tcb_configure(plat->base);
> +
> + return 0;
> +}
> +
> +static int atmel_tcb_of_to_plat(struct udevice *dev)
> +{
> + struct atmel_tcb_plat *plat = dev_get_plat(dev);
> +
> + plat->base = dev_read_addr_ptr(dev->parent);
It looks like you want to handle the parent in fact.
This does not look very straight forward from my perspective.
Added Claudiu to this thread who knows better the timer blocks as they
are designed in Linux
> +
> + return 0;
> +}
> +
> +static const struct timer_ops atmel_tcb_ops = {
> + .get_count = atmel_tcb_get_count,
> +};
> +
> +static const struct udevice_id atmel_tcb_ids[] = {
> + { .compatible = "atmel,tcb-timer" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(atmel_tcb) = {
> + .name = "atmel_tcb",
> + .id = UCLASS_TIMER,
> + .of_match = atmel_tcb_ids,
> + .of_to_plat = atmel_tcb_of_to_plat,
> + .plat_auto = sizeof(struct atmel_tcb_plat),
> + .probe = atmel_tcb_probe,
> + .ops = &atmel_tcb_ops,
> +};
> --
> 2.34.1
>
More information about the U-Boot
mailing list