* [PATCH v2 0/3] add TCB driver for sama5d2
@ 2022-02-02 14:42 Clément Léger
2022-02-02 14:42 ` [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver Clément Léger
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Clément Léger @ 2022-02-02 14:42 UTC (permalink / raw)
To: Eugen Hristev, Ludovic Desroches
Cc: u-boot, Nicolas Ferre, Clément Léger
When booting under OP-TEE, the SYSC is secured which means the PIT is
also not accessible by non-secure world. The TCB 0 however is always
available for non-secure world and thus can be used.
This series add a TCB driver and enables it in sama5d2 configs.
---
Changes in v2:
- Reworked driver to use existing Linux bindings
Clément Léger (3):
timer: atmel_tcb_timer: add atmel_tcb driver
dts: sama5d2: add TCB node
configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER
MAINTAINERS | 1 +
arch/arm/dts/sama5d2.dtsi | 15 ++
configs/sama5d2_icp_mmc_defconfig | 1 +
configs/sama5d2_ptc_ek_mmc_defconfig | 1 +
configs/sama5d2_ptc_ek_nandflash_defconfig | 1 +
configs/sama5d2_xplained_emmc_defconfig | 1 +
configs/sama5d2_xplained_mmc_defconfig | 1 +
configs/sama5d2_xplained_qspiflash_defconfig | 1 +
configs/sama5d2_xplained_spiflash_defconfig | 1 +
drivers/timer/Kconfig | 7 +
drivers/timer/Makefile | 1 +
drivers/timer/atmel_tcb_timer.c | 160 +++++++++++++++++++
12 files changed, 191 insertions(+)
create mode 100644 drivers/timer/atmel_tcb_timer.c
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver
2022-02-02 14:42 [PATCH v2 0/3] add TCB driver for sama5d2 Clément Léger
@ 2022-02-02 14:42 ` Clément Léger
2022-03-03 9:31 ` Eugen.Hristev
2022-02-02 14:43 ` [PATCH v2 2/3] dts: sama5d2: add TCB node Clément Léger
2022-02-02 14:43 ` [PATCH v2 3/3] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER Clément Léger
2 siblings, 1 reply; 12+ messages in thread
From: Clément Léger @ 2022-02-02 14:42 UTC (permalink / raw)
To: Eugen Hristev, Ludovic Desroches
Cc: u-boot, Nicolas Ferre, Clément Léger
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@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.
+
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@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
+
+#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);
+
+ 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;
+
+ /* 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;
+
+ 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);
+
+ 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] dts: sama5d2: add TCB node
2022-02-02 14:42 [PATCH v2 0/3] add TCB driver for sama5d2 Clément Léger
2022-02-02 14:42 ` [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver Clément Léger
@ 2022-02-02 14:43 ` Clément Léger
2022-02-04 7:52 ` Eugen.Hristev
2022-02-02 14:43 ` [PATCH v2 3/3] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER Clément Léger
2 siblings, 1 reply; 12+ messages in thread
From: Clément Léger @ 2022-02-02 14:43 UTC (permalink / raw)
To: Eugen Hristev, Ludovic Desroches
Cc: u-boot, Nicolas Ferre, Clément Léger
Add the device-tree node to describe the TCB timer.
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
arch/arm/dts/sama5d2.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
index 038cd73c03..fc6a4fbe4d 100644
--- a/arch/arm/dts/sama5d2.dtsi
+++ b/arch/arm/dts/sama5d2.dtsi
@@ -700,6 +700,21 @@
clocks = <&h32ck>;
};
+ tcb0: timer@f800c000 {
+ compatible = "atmel,sama5d2-tcb", "simple-mfd";
+ reg = <0xf800c000 0x100>;
+
+ clocks = <&tcb0_clk>, <&tcb0_gclk>, <&clk32k>;
+ clock-names = "t0_clk", "gclk", "slow_clk";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ timer0: timer@0 {
+ compatible = "atmel,tcb-timer";
+ reg = <0>, <1>;
+ };
+ };
+
watchdog@f8048040 {
compatible = "atmel,sama5d4-wdt";
reg = <0xf8048040 0x10>;
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER
2022-02-02 14:42 [PATCH v2 0/3] add TCB driver for sama5d2 Clément Léger
2022-02-02 14:42 ` [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver Clément Léger
2022-02-02 14:43 ` [PATCH v2 2/3] dts: sama5d2: add TCB node Clément Léger
@ 2022-02-02 14:43 ` Clément Léger
2 siblings, 0 replies; 12+ messages in thread
From: Clément Léger @ 2022-02-02 14:43 UTC (permalink / raw)
To: Eugen Hristev, Ludovic Desroches
Cc: u-boot, Nicolas Ferre, Clément Léger
This will allow using the TCB timer instead of the PIT one when running
under OP-TEE.
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
configs/sama5d2_icp_mmc_defconfig | 1 +
configs/sama5d2_ptc_ek_mmc_defconfig | 1 +
configs/sama5d2_ptc_ek_nandflash_defconfig | 1 +
configs/sama5d2_xplained_emmc_defconfig | 1 +
configs/sama5d2_xplained_mmc_defconfig | 1 +
configs/sama5d2_xplained_qspiflash_defconfig | 1 +
configs/sama5d2_xplained_spiflash_defconfig | 1 +
7 files changed, 7 insertions(+)
diff --git a/configs/sama5d2_icp_mmc_defconfig b/configs/sama5d2_icp_mmc_defconfig
index 7761a57e0c..2eb2e92110 100644
--- a/configs/sama5d2_icp_mmc_defconfig
+++ b/configs/sama5d2_icp_mmc_defconfig
@@ -83,5 +83,6 @@ CONFIG_ATMEL_USART=y
CONFIG_TIMER=y
CONFIG_SPL_TIMER=y
CONFIG_ATMEL_PIT_TIMER=y
+CONFIG_ATMEL_TCB_TIMER=y
CONFIG_OF_LIBFDT_OVERLAY=y
# CONFIG_EFI_LOADER_HII is not set
diff --git a/configs/sama5d2_ptc_ek_mmc_defconfig b/configs/sama5d2_ptc_ek_mmc_defconfig
index 9f458e100b..2a77f72734 100644
--- a/configs/sama5d2_ptc_ek_mmc_defconfig
+++ b/configs/sama5d2_ptc_ek_mmc_defconfig
@@ -72,6 +72,7 @@ CONFIG_DEBUG_UART_ANNOUNCE=y
CONFIG_ATMEL_USART=y
CONFIG_TIMER=y
CONFIG_ATMEL_PIT_TIMER=y
+CONFIG_ATMEL_TCB_TIMER=y
CONFIG_USB=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_STORAGE=y
diff --git a/configs/sama5d2_ptc_ek_nandflash_defconfig b/configs/sama5d2_ptc_ek_nandflash_defconfig
index 6460ff3dad..790e79019c 100644
--- a/configs/sama5d2_ptc_ek_nandflash_defconfig
+++ b/configs/sama5d2_ptc_ek_nandflash_defconfig
@@ -72,6 +72,7 @@ CONFIG_DEBUG_UART_ANNOUNCE=y
CONFIG_ATMEL_USART=y
CONFIG_TIMER=y
CONFIG_ATMEL_PIT_TIMER=y
+CONFIG_ATMEL_TCB_TIMER=y
CONFIG_USB=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_STORAGE=y
diff --git a/configs/sama5d2_xplained_emmc_defconfig b/configs/sama5d2_xplained_emmc_defconfig
index 844a9cde64..dfe120051f 100644
--- a/configs/sama5d2_xplained_emmc_defconfig
+++ b/configs/sama5d2_xplained_emmc_defconfig
@@ -91,6 +91,7 @@ CONFIG_ATMEL_QSPI=y
CONFIG_TIMER=y
CONFIG_SPL_TIMER=y
CONFIG_ATMEL_PIT_TIMER=y
+CONFIG_ATMEL_TCB_TIMER=y
CONFIG_USB=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_STORAGE=y
diff --git a/configs/sama5d2_xplained_mmc_defconfig b/configs/sama5d2_xplained_mmc_defconfig
index 0de0636587..910ff5c141 100644
--- a/configs/sama5d2_xplained_mmc_defconfig
+++ b/configs/sama5d2_xplained_mmc_defconfig
@@ -93,6 +93,7 @@ CONFIG_ATMEL_QSPI=y
CONFIG_TIMER=y
CONFIG_SPL_TIMER=y
CONFIG_ATMEL_PIT_TIMER=y
+CONFIG_ATMEL_TCB_TIMER=y
CONFIG_USB=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_STORAGE=y
diff --git a/configs/sama5d2_xplained_qspiflash_defconfig b/configs/sama5d2_xplained_qspiflash_defconfig
index a6e002e59e..158ec4c70e 100644
--- a/configs/sama5d2_xplained_qspiflash_defconfig
+++ b/configs/sama5d2_xplained_qspiflash_defconfig
@@ -92,6 +92,7 @@ CONFIG_ATMEL_QSPI=y
CONFIG_TIMER=y
CONFIG_SPL_TIMER=y
CONFIG_ATMEL_PIT_TIMER=y
+CONFIG_ATMEL_TCB_TIMER=y
CONFIG_USB=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_STORAGE=y
diff --git a/configs/sama5d2_xplained_spiflash_defconfig b/configs/sama5d2_xplained_spiflash_defconfig
index 676385fe55..5d946d06cf 100644
--- a/configs/sama5d2_xplained_spiflash_defconfig
+++ b/configs/sama5d2_xplained_spiflash_defconfig
@@ -96,6 +96,7 @@ CONFIG_ATMEL_QSPI=y
CONFIG_TIMER=y
CONFIG_SPL_TIMER=y
CONFIG_ATMEL_PIT_TIMER=y
+CONFIG_ATMEL_TCB_TIMER=y
CONFIG_USB=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_STORAGE=y
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] dts: sama5d2: add TCB node
2022-02-02 14:43 ` [PATCH v2 2/3] dts: sama5d2: add TCB node Clément Léger
@ 2022-02-04 7:52 ` Eugen.Hristev
2022-02-04 8:04 ` Clément Léger
0 siblings, 1 reply; 12+ messages in thread
From: Eugen.Hristev @ 2022-02-04 7:52 UTC (permalink / raw)
To: clement.leger, Ludovic.Desroches; +Cc: u-boot, Nicolas.Ferre
Hello Clement,
Subject should be ARM: dts: [at91:] sama5d2: ...
On 2/2/22 4:43 PM, Clément Léger wrote:
> Add the device-tree node to describe the TCB timer.
>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
> arch/arm/dts/sama5d2.dtsi | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
> index 038cd73c03..fc6a4fbe4d 100644
> --- a/arch/arm/dts/sama5d2.dtsi
> +++ b/arch/arm/dts/sama5d2.dtsi
> @@ -700,6 +700,21 @@
> clocks = <&h32ck>;
> };
>
> + tcb0: timer@f800c000 {
> + compatible = "atmel,sama5d2-tcb", "simple-mfd";
syscon ?
> + reg = <0xf800c000 0x100>;
> +
> + clocks = <&tcb0_clk>, <&tcb0_gclk>, <&clk32k>;
> + clock-names = "t0_clk", "gclk", "slow_clk";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + timer0: timer@0 {
> + compatible = "atmel,tcb-timer";
> + reg = <0>, <1>;
> + };
> + };
> +
I am not happy that the original binding has the interrupts as
'mandatory'. Maybe the binding author did not have more use cases in mind.
Anyway I think that it can go to u-boot without this interrupts property
as it's surely unused and there is no interrupt controller at the moment
in the DT.
If nobody has another opinion that is...
Eugen
> watchdog@f8048040 {
> compatible = "atmel,sama5d4-wdt";
> reg = <0xf8048040 0x10>;
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] dts: sama5d2: add TCB node
2022-02-04 7:52 ` Eugen.Hristev
@ 2022-02-04 8:04 ` Clément Léger
2022-02-04 8:37 ` Eugen.Hristev
0 siblings, 1 reply; 12+ messages in thread
From: Clément Léger @ 2022-02-04 8:04 UTC (permalink / raw)
To: Eugen.Hristev; +Cc: Ludovic.Desroches, u-boot, Nicolas.Ferre
Le Fri, 4 Feb 2022 07:52:26 +0000,
<Eugen.Hristev@microchip.com> a écrit :
> Hello Clement,
>
> Subject should be ARM: dts: [at91:] sama5d2: ...
>
> On 2/2/22 4:43 PM, Clément Léger wrote:
> > Add the device-tree node to describe the TCB timer.
> >
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> > arch/arm/dts/sama5d2.dtsi | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
> > index 038cd73c03..fc6a4fbe4d 100644
> > --- a/arch/arm/dts/sama5d2.dtsi
> > +++ b/arch/arm/dts/sama5d2.dtsi
> > @@ -700,6 +700,21 @@
> > clocks = <&h32ck>;
> > };
> >
> > + tcb0: timer@f800c000 {
> > + compatible = "atmel,sama5d2-tcb", "simple-mfd";
>
> syscon ?
Hi Eugen,
Yes I might add it if needed but in my case, there is no need for it.
>
> > + reg = <0xf800c000 0x100>;
> > +
> > + clocks = <&tcb0_clk>, <&tcb0_gclk>, <&clk32k>;
> > + clock-names = "t0_clk", "gclk", "slow_clk";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + timer0: timer@0 {
> > + compatible = "atmel,tcb-timer";
> > + reg = <0>, <1>;
> > + };
> > + };
> > +
>
> I am not happy that the original binding has the interrupts as
> 'mandatory'. Maybe the binding author did not have more use cases in mind.
> Anyway I think that it can go to u-boot without this interrupts property
> as it's surely unused and there is no interrupt controller at the moment
> in the DT.
I checked the other nodes and indeed, I did not found any 'interrupts'
property so i guess it 'could' be left out... But I can add it for sure.
> If nobody has another opinion that is...
>
> Eugen
>
> > watchdog@f8048040 {
> > compatible = "atmel,sama5d4-wdt";
> > reg = <0xf8048040 0x10>;
> > --
> > 2.34.1
> >
>
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] dts: sama5d2: add TCB node
2022-02-04 8:04 ` Clément Léger
@ 2022-02-04 8:37 ` Eugen.Hristev
2022-02-04 10:40 ` Eugen.Hristev
0 siblings, 1 reply; 12+ messages in thread
From: Eugen.Hristev @ 2022-02-04 8:37 UTC (permalink / raw)
To: clement.leger; +Cc: Ludovic.Desroches, u-boot, Nicolas.Ferre
On 2/4/22 10:04 AM, Clément Léger wrote:
> Le Fri, 4 Feb 2022 07:52:26 +0000,
> <Eugen.Hristev@microchip.com> a écrit :
>
>> Hello Clement,
>>
>> Subject should be ARM: dts: [at91:] sama5d2: ...
>>
>> On 2/2/22 4:43 PM, Clément Léger wrote:
>>> Add the device-tree node to describe the TCB timer.
>>>
>>> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
>>> ---
>>> arch/arm/dts/sama5d2.dtsi | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
>>> index 038cd73c03..fc6a4fbe4d 100644
>>> --- a/arch/arm/dts/sama5d2.dtsi
>>> +++ b/arch/arm/dts/sama5d2.dtsi
>>> @@ -700,6 +700,21 @@
>>> clocks = <&h32ck>;
>>> };
>>>
>>> + tcb0: timer@f800c000 {
>>> + compatible = "atmel,sama5d2-tcb", "simple-mfd";
>>
>> syscon ?
>
> Hi Eugen,
>
> Yes I might add it if needed but in my case, there is no need for it.
True, but the binding says that the compatible *must* be the three
stated... so we must comply to that, regardless if it's a need for it or
not in U-boot.
>
>>
>>> + reg = <0xf800c000 0x100>;
>>> +
>>> + clocks = <&tcb0_clk>, <&tcb0_gclk>, <&clk32k>;
>>> + clock-names = "t0_clk", "gclk", "slow_clk";
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + timer0: timer@0 {
>>> + compatible = "atmel,tcb-timer";
>>> + reg = <0>, <1>;
>>> + };
>>> + };
>>> +
>>
>> I am not happy that the original binding has the interrupts as
>> 'mandatory'. Maybe the binding author did not have more use cases in mind.
>> Anyway I think that it can go to u-boot without this interrupts property
>> as it's surely unused and there is no interrupt controller at the moment
>> in the DT.
>
> I checked the other nodes and indeed, I did not found any 'interrupts'
> property so i guess it 'could' be left out... But I can add it for sure.
I sent a patch in Linux to remove interrupts from mandatory. Let's have
the node without it for now.
Your series will have to wait a little bit anyway for the next merge
window, and a review on the driver.
Thanks again for your patches,
Eugen
>
>> If nobody has another opinion that is...
>>
>> Eugen
>>
>>> watchdog@f8048040 {
>>> compatible = "atmel,sama5d4-wdt";
>>> reg = <0xf8048040 0x10>;
>>> --
>>> 2.34.1
>>>
>>
>
>
>
> --
> Clément Léger,
> Embedded Linux and Kernel engineer at Bootlin
> https://bootlin.com
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] dts: sama5d2: add TCB node
2022-02-04 8:37 ` Eugen.Hristev
@ 2022-02-04 10:40 ` Eugen.Hristev
2022-02-04 15:28 ` Clément Léger
0 siblings, 1 reply; 12+ messages in thread
From: Eugen.Hristev @ 2022-02-04 10:40 UTC (permalink / raw)
To: clement.leger; +Cc: Ludovic.Desroches, u-boot, Nicolas.Ferre
On 2/4/22 10:37 AM, Eugen Hristev - M18282 wrote:
> On 2/4/22 10:04 AM, Clément Léger wrote:
>> Le Fri, 4 Feb 2022 07:52:26 +0000,
>> <Eugen.Hristev@microchip.com> a écrit :
>>
>>> Hello Clement,
>>>
>>> Subject should be ARM: dts: [at91:] sama5d2: ...
>>>
>>> On 2/2/22 4:43 PM, Clément Léger wrote:
>>>> Add the device-tree node to describe the TCB timer.
>>>>
>>>> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
>>>> ---
>>>> arch/arm/dts/sama5d2.dtsi | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
>>>> index 038cd73c03..fc6a4fbe4d 100644
>>>> --- a/arch/arm/dts/sama5d2.dtsi
>>>> +++ b/arch/arm/dts/sama5d2.dtsi
>>>> @@ -700,6 +700,21 @@
>>>> clocks = <&h32ck>;
>>>> };
>>>>
>>>> + tcb0: timer@f800c000 {
>>>> + compatible = "atmel,sama5d2-tcb", "simple-mfd";
>>>
>>> syscon ?
>>
>> Hi Eugen,
>>
>> Yes I might add it if needed but in my case, there is no need for it.
>
> True, but the binding says that the compatible *must* be the three
> stated... so we must comply to that, regardless if it's a need for it or
> not in U-boot.
>
>>
>>>
>>>> + reg = <0xf800c000 0x100>;
>>>> +
>>>> + clocks = <&tcb0_clk>, <&tcb0_gclk>, <&clk32k>;
>>>> + clock-names = "t0_clk", "gclk", "slow_clk";
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> +
>>>> + timer0: timer@0 {
>>>> + compatible = "atmel,tcb-timer";
>>>> + reg = <0>, <1>;
>>>> + };
>>>> + };
>>>> +
>>>
>>> I am not happy that the original binding has the interrupts as
>>> 'mandatory'. Maybe the binding author did not have more use cases in mind.
>>> Anyway I think that it can go to u-boot without this interrupts property
>>> as it's surely unused and there is no interrupt controller at the moment
>>> in the DT.
>>
>> I checked the other nodes and indeed, I did not found any 'interrupts'
>> property so i guess it 'could' be left out... But I can add it for sure.
>
> I sent a patch in Linux to remove interrupts from mandatory. Let's have
> the node without it for now.
>
> Your series will have to wait a little bit anyway for the next merge
> window, and a review on the driver.
Hello Clement,
According to this :
https://lore.kernel.org/linux-arm-kernel/Yf0Bkh4pXKORmNkG@piout.net/
... the interrupt is mandatory.
Eugen
>
> Thanks again for your patches,
>
> Eugen
>
>>
>>> If nobody has another opinion that is...
>>>
>>> Eugen
>>>
>>>> watchdog@f8048040 {
>>>> compatible = "atmel,sama5d4-wdt";
>>>> reg = <0xf8048040 0x10>;
>>>> --
>>>> 2.34.1
>>>>
>>>
>>
>>
>>
>> --
>> Clément Léger,
>> Embedded Linux and Kernel engineer at Bootlin
>> https://bootlin.com
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] dts: sama5d2: add TCB node
2022-02-04 10:40 ` Eugen.Hristev
@ 2022-02-04 15:28 ` Clément Léger
0 siblings, 0 replies; 12+ messages in thread
From: Clément Léger @ 2022-02-04 15:28 UTC (permalink / raw)
To: Eugen.Hristev; +Cc: Ludovic.Desroches, u-boot, Nicolas.Ferre
Le Fri, 4 Feb 2022 10:40:52 +0000,
<Eugen.Hristev@microchip.com> a écrit :
> >
> > I sent a patch in Linux to remove interrupts from mandatory. Let's have
> > the node without it for now.
> >
> > Your series will have to wait a little bit anyway for the next merge
> > window, and a review on the driver.
>
> Hello Clement,
>
> According to this :
>
> https://lore.kernel.org/linux-arm-kernel/Yf0Bkh4pXKORmNkG@piout.net/
>
> ... the interrupt is mandatory.
Hi Eugen,
No worries, I'll add it in a V3 but I will wait for the driver review to
do so.
>
> Eugen
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver
2022-02-02 14:42 ` [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver Clément Léger
@ 2022-03-03 9:31 ` Eugen.Hristev
2022-03-03 9:59 ` Clément Léger
0 siblings, 1 reply; 12+ messages in thread
From: Eugen.Hristev @ 2022-03-03 9:31 UTC (permalink / raw)
To: clement.leger; +Cc: Ludovic.Desroches, u-boot, Nicolas.Ferre, Claudiu.Beznea
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@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@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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver
2022-03-03 9:31 ` Eugen.Hristev
@ 2022-03-03 9:59 ` Clément Léger
2022-03-03 10:21 ` Eugen.Hristev
0 siblings, 1 reply; 12+ messages in thread
From: Clément Léger @ 2022-03-03 9:59 UTC (permalink / raw)
To: Eugen.Hristev; +Cc: Ludovic.Desroches, u-boot, Nicolas.Ferre, Claudiu.Beznea
Le Thu, 3 Mar 2022 09:31:14 +0000,
<Eugen.Hristev@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@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@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver
2022-03-03 9:59 ` Clément Léger
@ 2022-03-03 10:21 ` Eugen.Hristev
0 siblings, 0 replies; 12+ messages in thread
From: Eugen.Hristev @ 2022-03-03 10:21 UTC (permalink / raw)
To: clement.leger; +Cc: Ludovic.Desroches, u-boot, Nicolas.Ferre, Claudiu.Beznea
On 3/3/22 11:59 AM, Clément Léger wrote:
> Le Thu, 3 Mar 2022 09:31:14 +0000,
> <Eugen.Hristev@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@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@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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-03-03 10:21 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-02 14:42 [PATCH v2 0/3] add TCB driver for sama5d2 Clément Léger
2022-02-02 14:42 ` [PATCH v2 1/3] timer: atmel_tcb_timer: add atmel_tcb driver Clément Léger
2022-03-03 9:31 ` Eugen.Hristev
2022-03-03 9:59 ` Clément Léger
2022-03-03 10:21 ` Eugen.Hristev
2022-02-02 14:43 ` [PATCH v2 2/3] dts: sama5d2: add TCB node Clément Léger
2022-02-04 7:52 ` Eugen.Hristev
2022-02-04 8:04 ` Clément Léger
2022-02-04 8:37 ` Eugen.Hristev
2022-02-04 10:40 ` Eugen.Hristev
2022-02-04 15:28 ` Clément Léger
2022-02-02 14:43 ` [PATCH v2 3/3] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER Clément Léger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.