[PATCH v5 0/4] add TCB driver for sama5d2

[PATCH v5 0/4] add TCB driver for sama5d2

[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 v5:
- Fixed missing include in .dts file for IRQ defines
- Add aic interrupt controller node to avoid DTC warnings

Changes in v4:
- Add missing interrupts property to tcb node

Changes in v3:
- Remove useless defines
- Add printf in case of unsupported timers specified in device-tree
- Add "syscon" compatible to tcb node

Changes in v2:
- Reworked driver to use existing Linux bindings

Clément Léger (4):
  timer: atmel_tcb_timer: add atmel_tcb driver
  ARM: dts: at91: sama5d2: add AIC node
  ARM: dts: at91: sama5d2: add TCB node
  configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER

 MAINTAINERS                                  |   1 +
 arch/arm/dts/sama5d2.dtsi                    |  27 ++++
 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                        |   8 +
 drivers/timer/Makefile                       |   1 +
 drivers/timer/atmel_tcb_timer.c              | 155 +++++++++++++++++++
 12 files changed, 199 insertions(+)
 create mode 100644 drivers/timer/atmel_tcb_timer.c

-- 
2.34.1

[PATCH v5 1/4] timer: atmel_tcb_timer: add atmel_tcb driver

[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           |   8 ++
 drivers/timer/Makefile          |   1 +
 drivers/timer/atmel_tcb_timer.c | 155 ++++++++++++++++++++++++++++++++
 4 files changed, 165 insertions(+)
 create mode 100644 drivers/timer/atmel_tcb_timer.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0f39bc6bc9..e30389dc3e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -352,6 +352,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..8fad59b81a 100644
--- a/drivers/timer/Kconfig
+++ b/drivers/timer/Kconfig
@@ -96,6 +96,14 @@ 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
+	depends on ARCH_AT91
+	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..b73b367227
--- /dev/null
+++ b/drivers/timer/atmel_tcb_timer.c
@@ -0,0 +1,155 @@
+// 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	BIT(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_RC(chan)		(0x1c + TCB_CHAN(chan))
+
+#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) {
+		printf("Error: only chained timers 0 and 1 are supported\n");
+		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

[PATCH v5 2/4] ARM: dts: at91: sama5d2: add AIC node

[Clément Léger]

When using interrupts property, a global interrupt controller needs to
be added to avoid warnings when compiling device-tree:

 arch/arm/dts/at91-sama5d2_xplained.dtb: Warning (interrupts_property):
    /ahb/apb/timer@f800c000: Missing interrupt-parent

Add AIC node as the sama5d2 global interrupt controller.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 arch/arm/dts/sama5d2.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
index 038cd73c03..dee68dc022 100644
--- a/arch/arm/dts/sama5d2.dtsi
+++ b/arch/arm/dts/sama5d2.dtsi
@@ -3,6 +3,7 @@
 / {
 	model = "Atmel SAMA5D2 family SoC";
 	compatible = "atmel,sama5d2";
+	interrupt-parent = <&aic>;
 
 	aliases {
 		spi0 = &spi0;
@@ -700,6 +701,15 @@
 				clocks = <&h32ck>;
 			};
 
+			aic: interrupt-controller@fc020000 {
+				#interrupt-cells = <3>;
+				compatible = "atmel,sama5d2-aic";
+				interrupt-controller;
+				reg = <0xfc020000 0x200>;
+				atmel,external-irqs = <49>;
+				status = "disabled";
+			};
+
 			watchdog@f8048040 {
 				compatible = "atmel,sama5d4-wdt";
 				reg = <0xf8048040 0x10>;
-- 
2.34.1

[PATCH v5 3/4] ARM: dts: at91: sama5d2: add TCB node

[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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
index dee68dc022..9360c0008c 100644
--- a/arch/arm/dts/sama5d2.dtsi
+++ b/arch/arm/dts/sama5d2.dtsi
@@ -1,4 +1,5 @@
 #include "skeleton.dtsi"
+#include <dt-bindings/interrupt-controller/irq.h>
 
 / {
 	model = "Atmel SAMA5D2 family SoC";
@@ -710,6 +711,22 @@
 				status = "disabled";
 			};
 
+			tcb0: timer@f800c000 {
+				compatible = "atmel,sama5d2-tcb", "simple-mfd",
+					     "syscon";
+				reg = <0xf800c000 0x100>;
+				interrupts = <35 IRQ_TYPE_LEVEL_HIGH 0>;
+				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

[PATCH v5 4/4] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER

[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

Re: [PATCH v5 3/4] ARM: dts: at91: sama5d2: add TCB node

[Eugen.Hristev]

On 3/4/22 1:05 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 | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
> index dee68dc022..9360c0008c 100644
> --- a/arch/arm/dts/sama5d2.dtsi
> +++ b/arch/arm/dts/sama5d2.dtsi
> @@ -1,4 +1,5 @@
>   #include "skeleton.dtsi"
> +#include <dt-bindings/interrupt-controller/irq.h>
> 
>   / {
>          model = "Atmel SAMA5D2 family SoC";
> @@ -710,6 +711,22 @@
>                                  status = "disabled";
>                          };
> 
> +                       tcb0: timer@f800c000 {
> +                               compatible = "atmel,sama5d2-tcb", "simple-mfd",
> +                                            "syscon";

No need to line break the previous line. DT files can have more than 80 
chars per line, it's fine.

If there are no other comments I will fix this when applying, no need to 
send a v6 for now.

> +                               reg = <0xf800c000 0x100>;
> +                               interrupts = <35 IRQ_TYPE_LEVEL_HIGH 0>;
> +                               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
> 

Re: [PATCH v5 2/4] ARM: dts: at91: sama5d2: add AIC node

[Eugen.Hristev]

On 3/4/22 1:05 PM, Clément Léger wrote:
> When using interrupts property, a global interrupt controller needs to
> be added to avoid warnings when compiling device-tree:
> 
>   arch/arm/dts/at91-sama5d2_xplained.dtb: Warning (interrupts_property):
>      /ahb/apb/timer@f800c000: Missing interrupt-parent
> 
> Add AIC node as the sama5d2 global interrupt controller.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>   arch/arm/dts/sama5d2.dtsi | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
> index 038cd73c03..dee68dc022 100644
> --- a/arch/arm/dts/sama5d2.dtsi
> +++ b/arch/arm/dts/sama5d2.dtsi
> @@ -3,6 +3,7 @@
>   / {
>          model = "Atmel SAMA5D2 family SoC";
>          compatible = "atmel,sama5d2";
> +       interrupt-parent = <&aic>;
> 
>          aliases {
>                  spi0 = &spi0;
> @@ -700,6 +701,15 @@
>                                  clocks = <&h32ck>;
>                          };
> 
> +                       aic: interrupt-controller@fc020000 {
> +                               #interrupt-cells = <3>;
> +                               compatible = "atmel,sama5d2-aic";
> +                               interrupt-controller;
> +                               reg = <0xfc020000 0x200>;
> +                               atmel,external-irqs = <49>;
> +                               status = "disabled";
> +                       };

DT nodes should be sorted by base address.
Check patch 3/4 as well.

> +
>                          watchdog@f8048040 {
>                                  compatible = "atmel,sama5d4-wdt";
>                                  reg = <0xf8048040 0x10>;
> --
> 2.34.1
> 

Re: [PATCH v5 3/4] ARM: dts: at91: sama5d2: add TCB node

[Clément Léger]

Le Fri, 4 Mar 2022 14:31:08 +0000,
<Eugen.Hristev@microchip.com> a écrit :

> On 3/4/22 1:05 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 | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> > 
> > diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
> > index dee68dc022..9360c0008c 100644
> > --- a/arch/arm/dts/sama5d2.dtsi
> > +++ b/arch/arm/dts/sama5d2.dtsi
> > @@ -1,4 +1,5 @@
> >   #include "skeleton.dtsi"
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > 
> >   / {
> >          model = "Atmel SAMA5D2 family SoC";
> > @@ -710,6 +711,22 @@
> >                                  status = "disabled";
> >                          };
> > 
> > +                       tcb0: timer@f800c000 {
> > +                               compatible = "atmel,sama5d2-tcb", "simple-mfd",
> > +                                            "syscon";  
> 
> No need to line break the previous line. DT files can have more than 80 
> chars per line, it's fine.

Ok, wasn't sure of that, found some of them that had these line breaks.
Good to know for next patches.

> 
> If there are no other comments I will fix this when applying, no need to 
> send a v6 for now.

Ok, thanks Eugen.

> 
> > +                               reg = <0xf800c000 0x100>;
> > +                               interrupts = <35 IRQ_TYPE_LEVEL_HIGH 0>;
> > +                               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
> >   
> 



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v5 1/4] timer: atmel_tcb_timer: add atmel_tcb driver

[Claudiu.Beznea]

On 04.03.2022 13:05, Clément Léger wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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           |   8 ++
>  drivers/timer/Makefile          |   1 +
>  drivers/timer/atmel_tcb_timer.c | 155 ++++++++++++++++++++++++++++++++
>  4 files changed, 165 insertions(+)
>  create mode 100644 drivers/timer/atmel_tcb_timer.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0f39bc6bc9..e30389dc3e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -352,6 +352,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..8fad59b81a 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -96,6 +96,14 @@ 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
> +       depends on ARCH_AT91
> +       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..b73b367227
> --- /dev/null
> +++ b/drivers/timer/atmel_tcb_timer.c
> @@ -0,0 +1,155 @@
> +// 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

This is not used

> +#define  TCB_CCR_CLKEN         0x1

Here

> +
> +#define TCB_CMR(chan)          (0x4 + TCB_CHAN(chan))
> +#define  TCB_CMR_WAVE          BIT(15)

here

> +#define  TCB_CMR_TIMER_CLOCK0  0

Datasheet names it CLOCK1.

> +#define  TCB_CMR_XC1           6
> +#define  TCB_CMR_ACPA_SET      BIT(16)

here

> +#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_RC(chan)           (0x1c + TCB_CHAN(chan))
> +
> +#define TCB_IDR(chan)          (0x28 + TCB_CHAN(chan))
> +
> +#define TCB_BCR                        0xc0
> +#define  TCB_BCR_SYNC          0x1

and here: there a mixture of using BIT() and using plain bit defines: would
be good to stick to one or the other.

> +
> +#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));

I see this set to zero in Linux. Is it set to 1 to avoid "set when reaching
0" described above?

> +       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"))

Is this necessary?

> +               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) {
> +               printf("Error: only chained timers 0 and 1 are supported\n");
> +               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
> 

Re: [PATCH v5 2/4] ARM: dts: at91: sama5d2: add AIC node

[Claudiu.Beznea]

On 04.03.2022 13:05, Clément Léger wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> When using interrupts property, a global interrupt controller needs to
> be added to avoid warnings when compiling device-tree:
> 
>  arch/arm/dts/at91-sama5d2_xplained.dtb: Warning (interrupts_property):
>     /ahb/apb/timer@f800c000: Missing interrupt-parent
> 
> Add AIC node as the sama5d2 global interrupt controller.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  arch/arm/dts/sama5d2.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/dts/sama5d2.dtsi b/arch/arm/dts/sama5d2.dtsi
> index 038cd73c03..dee68dc022 100644
> --- a/arch/arm/dts/sama5d2.dtsi
> +++ b/arch/arm/dts/sama5d2.dtsi
> @@ -3,6 +3,7 @@
>  / {
>         model = "Atmel SAMA5D2 family SoC";
>         compatible = "atmel,sama5d2";
> +       interrupt-parent = <&aic>;
> 
>         aliases {
>                 spi0 = &spi0;
> @@ -700,6 +701,15 @@
>                                 clocks = <&h32ck>;
>                         };
> 
> +                       aic: interrupt-controller@fc020000 {
> +                               #interrupt-cells = <3>;
> +                               compatible = "atmel,sama5d2-aic";
> +                               interrupt-controller;
> +                               reg = <0xfc020000 0x200>;

Most of the nodes in this DT has compatible and reg at the beginning. For
clarity would be good to keep the rule for this one as well.

> +                               atmel,external-irqs = <49>;
> +                               status = "disabled";
> +                       };
> +
>                         watchdog@f8048040 {
>                                 compatible = "atmel,sama5d4-wdt";
>                                 reg = <0xf8048040 0x10>;
> --
> 2.34.1
> 

Re: [PATCH v5 4/4] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER

[Claudiu.Beznea]

Hi Clément,

On 04.03.2022 13:05, Clément Léger wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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

As far as I can tell, dm_timer_init() will probe the first available timer
if there is no CONFIG_OF_REAL enabled + tick-timer DT entry. With this,
having 2 timers enabled in config might not lead to using TCB all the time
as it might be expected.

Thank you,
Claudiu Beznea

>  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
> 

Re: [PATCH v5 1/4] timer: atmel_tcb_timer: add atmel_tcb driver

[Clément Léger]

Le Mon, 7 Mar 2022 10:57:02 +0000,
<Claudiu.Beznea@microchip.com> a écrit :

> On 04.03.2022 13:05, Clément Léger wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > 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           |   8 ++
> >  drivers/timer/Makefile          |   1 +
> >  drivers/timer/atmel_tcb_timer.c | 155 ++++++++++++++++++++++++++++++++
> >  4 files changed, 165 insertions(+)
> >  create mode 100644 drivers/timer/atmel_tcb_timer.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0f39bc6bc9..e30389dc3e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -352,6 +352,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..8fad59b81a 100644
> > --- a/drivers/timer/Kconfig
> > +++ b/drivers/timer/Kconfig
> > @@ -96,6 +96,14 @@ 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
> > +       depends on ARCH_AT91
> > +       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..b73b367227
> > --- /dev/null
> > +++ b/drivers/timer/atmel_tcb_timer.c
> > @@ -0,0 +1,155 @@
> > +// 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  
> 
> This is not used
> 
> > +#define  TCB_CCR_CLKEN         0x1  
> 
> Here
> 
> > +
> > +#define TCB_CMR(chan)          (0x4 + TCB_CHAN(chan))
> > +#define  TCB_CMR_WAVE          BIT(15)  
> 
> here
> 
> > +#define  TCB_CMR_TIMER_CLOCK0  0  
> 
> Datasheet names it CLOCK1.

Indeed, I'll rename that.

> 
> > +#define  TCB_CMR_XC1           6
> > +#define  TCB_CMR_ACPA_SET      BIT(16)  
> 
> here
> 
> > +#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_RC(chan)           (0x1c + TCB_CHAN(chan))
> > +
> > +#define TCB_IDR(chan)          (0x28 + TCB_CHAN(chan))
> > +
> > +#define TCB_BCR                        0xc0
> > +#define  TCB_BCR_SYNC          0x1  
> 
> and here: there a mixture of using BIT() and using plain bit defines: would
> be good to stick to one or the other.

Acked, I guess I'll go with plain bit defines.

> 
> > +
> > +#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));  
> 
> I see this set to zero in Linux. Is it set to 1 to avoid "set when reaching
> 0" described above?

Yes, do you want me to add a comment ?

> 
> > +       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"))  
> 
> Is this necessary?

If we want to be pedantic about the bindings, yes... THis ensure that
we respect the node hierarchy that have been described in the Linux
bindings.

Thanks for the review,

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v5 4/4] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER

[Clément Léger]

Le Mon, 7 Mar 2022 11:06:17 +0000,
<Claudiu.Beznea@microchip.com> a écrit :

> > 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  
> 
> As far as I can tell, dm_timer_init() will probe the first available timer
> if there is no CONFIG_OF_REAL enabled + tick-timer DT entry. With this,
> having 2 timers enabled in config might not lead to using TCB all the time
> as it might be expected.

Hi Claudiu,

Hum indeed. Should I remove this commit and let the user select the TCB
when needed ?

Thanks,

> 
> Thank you,
> Claudiu Beznea


-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH v5 4/4] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER

[Claudiu.Beznea]

On 07.03.2022 13:10, Clément Léger wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Le Mon, 7 Mar 2022 11:06:17 +0000,
> <Claudiu.Beznea@microchip.com> a écrit :
> 
>>> 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
>>
>> As far as I can tell, dm_timer_init() will probe the first available timer
>> if there is no CONFIG_OF_REAL enabled + tick-timer DT entry. With this,
>> having 2 timers enabled in config might not lead to using TCB all the time
>> as it might be expected.
> 
> Hi Claudiu,
> 
> Hum indeed. Should I remove this commit and let the user select the TCB
> when needed ?

I would select only one timer in config to be able to work also with Linux
DT (if any) as in Linux SAMA5D2 works with TCB as the active/prefered
clocksource/clockevent so it is already available in DT. Eugen, is this OK?

Thank you,
Claudiu Beznea

> 
> Thanks,
> 
>>
>> Thank you,
>> Claudiu Beznea
> 
> 
> --
> Clément Léger,
> Embedded Linux and Kernel engineer at Bootlin
> https://bootlin.com

Re: [PATCH v5 4/4] configs: sama5d2: enable option CONFIG_ATMEL_TCB_TIMER

[Eugen.Hristev]

On 3/7/22 3:06 PM, Claudiu Beznea - M18063 wrote:
> On 07.03.2022 13:10, Clément Léger wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Le Mon, 7 Mar 2022 11:06:17 +0000,
>> <Claudiu.Beznea@microchip.com> a écrit :
>>
>>>> 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
>>>
>>> As far as I can tell, dm_timer_init() will probe the first available timer
>>> if there is no CONFIG_OF_REAL enabled + tick-timer DT entry. With this,
>>> having 2 timers enabled in config might not lead to using TCB all the time
>>> as it might be expected.
>>
>> Hi Claudiu,
>>
>> Hum indeed. Should I remove this commit and let the user select the TCB
>> when needed ?
> 
> I would select only one timer in config to be able to work also with Linux
> DT (if any) as in Linux SAMA5D2 works with TCB as the active/prefered
> clocksource/clockevent so it is already available in DT. Eugen, is this OK?

Hi,

In defconfig you are free to select which driver you want to build. 
(both, only one, or none).
In DT, you have to describe the board and SoC as in Linux.

> 
> Thank you,
> Claudiu Beznea
> 
>>
>> Thanks,
>>
>>>
>>> Thank you,
>>> Claudiu Beznea
>>
>>
>> --
>> Clément Léger,
>> Embedded Linux and Kernel engineer at Bootlin
>> https://bootlin.com
>