Le Thu, 3 Mar 2022 09:31:14 +0000,
<eugen.hris...@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.le...@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.le...@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.
I can downgrade to using a 32 bits counter if you want. Please note
that the same logic is used in Linux driver.
>
> > +
> > + 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:
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.
>
> > +
> > + 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
