[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 11:21:14 CET 2022
On 3/3/22 11:59 AM, Clément Léger wrote:
> Le Thu, 3 Mar 2022 09:31:14 +0000,
> <Eugen.Hristev at microchip.com> a écrit :
>
>> 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
>
> Acked.
>
>>
>>> +
>>> +#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 ?
>
> This loops allows to read a "coherent" 64 bits value from two 32 bits
> registers that are handled separatly by the hardware. This is just a
> precaution to avoid reading value while overflowing the 32 bits low
> value.
>
> Lets say the counter contains the following values:
> cv1 = 0
> cv0 = 0xffffffff
>
> If we read cv1, and then cv0, we might potentially read cv1 = 0 and cv0
> = 0 due to counter being incremented instead ov cv1 = 1, cv0 = 0. So
> actually, this check with cv1 will almost never be true and will
> potentially just happens if reading at the exact same time cv0
> overflows.
Ok, I understand, the cv1 is the MSB of the timer, thus it won't change
so fast
>
> I can downgrade to using a 32 bits counter if you want. Please note
> that the same logic is used in Linux driver.
It's fine, keep the 64 bits. If Linux uses the same logic we are fine.
>
>>
>>> +
>>> + 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.
>
> Since we "must" use the Linux bindings, this should be done this way.
> In linux, it is done the same way:
Yes, we must, as bindings is ABI and it should reflect the hardware.
>
> static const struct of_device_id atmel_tcb_of_match[] = {
> ...
> { .compatible = "atmel,sama5d2-tcb", ...},
> { /* sentinel */ }
> };
>
> static int __init tcb_clksrc_init(struct device_node *node)
> {
> [...]
> tc.regs = of_iomap(node->parent, 0);
> if (!tc.regs)
> return -ENXIO;
> [...]
> match = of_match_node(atmel_tcb_of_match, node->parent);
> if (!match)
> return -ENODEV;
> [...]
> }
>
>>
>>> +
>>> + /* 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.
>
> Ok, that would be a good thing indeed.
>
>>
>>> +
>>> + 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
>
> The same thing is done in Linux also (see above). This is caused by the
> specific bindings.
Ok, keep Claudiu in the loop as I would like for him to have the chance
to review this driver.
Thanks !
>
>>
>>> +
>>> + 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
>>>
>>
>
>
>
> --
> Clément Léger,
> Embedded Linux and Kernel engineer at Bootlin
> https://bootlin.com
>
More information about the U-Boot
mailing list