Change in coreboot[master]: soc/ti/am335x: Fix timer implementation

[Sam Lewis (Code Review)]

Sam Lewis has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44383 )

Change subject: soc/ti/am335x: Fix timer implementation
......................................................................

soc/ti/am335x: Fix timer implementation

Implements the monotonic timer using the am335x dmtimer peripheral.

Change-Id: I4736b6d3b6e26370be9e8f369fc02285ad519223
Signed-off-by: Sam Lewis 
---
M src/soc/ti/am335x/Makefile.inc
D src/soc/ti/am335x/dmtimer.c
M src/soc/ti/am335x/dmtimer.h
D src/soc/ti/am335x/monotonic_timer.c
A src/soc/ti/am335x/timer.c
5 files changed, 59 insertions(+), 61 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/44383/1

diff --git a/src/soc/ti/am335x/Makefile.inc b/src/soc/ti/am335x/Makefile.inc
index 06ac1ee..f5ebb7a 100644
--- a/src/soc/ti/am335x/Makefile.inc
+++ b/src/soc/ti/am335x/Makefile.inc
@@ -1,17 +1,14 @@
 bootblock-y	+= bootblock.c
 bootblock-y	+= bootblock_media.c
-bootblock-y	+= dmtimer.c
+bootblock-y	+= timer.c
 bootblock-y	+= gpio.c
 bootblock-y	+= pinmux.c
-bootblock-y	+= monotonic_timer.c

romstage-y	+= nand.c
 romstage-y	+= cbmem.c
-romstage-y	+= dmtimer.c
-romstage-y	+= monotonic_timer.c
+romstage-y	+= timer.c

-ramstage-y	+= dmtimer.c
-ramstage-y	+= monotonic_timer.c
+ramstage-y	+= timer.c
 ramstage-y	+= nand.c
 ramstage-y  += soc.c

diff --git a/src/soc/ti/am335x/dmtimer.c b/src/soc/ti/am335x/dmtimer.c
deleted file mode 100644
index b3aa7a1..0000000
--- a/src/soc/ti/am335x/dmtimer.c
+++ /dev/null
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#include "dmtimer.h"
-
-void dmtimer_start(int num)
-{
-}
-
-uint64_t dmtimer_raw_value(int num)
-{
-	return 0;
-}
diff --git a/src/soc/ti/am335x/dmtimer.h b/src/soc/ti/am335x/dmtimer.h
index ad8515f..c2ecb40 100644
--- a/src/soc/ti/am335x/dmtimer.h
+++ b/src/soc/ti/am335x/dmtimer.h
@@ -5,9 +5,31 @@

#include <stdint.h>

-#define OSC_HZ 24000000
+#define M_OSC_MHZ (24)

-void dmtimer_start(int num);
-uint64_t dmtimer_raw_value(int num);
+struct am335x_dmtimer {
+	uint32_t tidr;
+	uint8_t res1[12];
+	uint32_t tiocp_cfg;
+	uint8_t res2[12];
+	uint32_t irq_eoi;
+	uint32_t irqstatus_raw;
+	uint32_t irqstatus;
+	uint32_t irqenable_set;
+	uint32_t irqenable_clr;
+	uint32_t irqwakeen;
+	uint32_t tclr;
+	uint32_t tcrr;
+	uint32_t tldr;
+	uint32_t ttgr;
+	uint32_t twps;
+	uint32_t tmar;
+	uint32_t tcar1;
+	uint32_t tsicr;
+	uint32_t tcar2;
+};
+
+#define TCLR_ST (0x01 << 0)
+#define TCLR_AR (0x01 << 1)

#endif
diff --git a/src/soc/ti/am335x/monotonic_timer.c b/src/soc/ti/am335x/monotonic_timer.c
deleted file mode 100644
index b57258b..0000000
--- a/src/soc/ti/am335x/monotonic_timer.c
+++ /dev/null
@@ -1,40 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#include <stdint.h>
-#include <delay.h>
-#include <timer.h>
-
-#include "dmtimer.h"
-
-static struct monotonic_counter {
-	int initialized;
-	struct mono_time time;
-	uint64_t last_value;
-} mono_counter;
-
-static const uint32_t clocks_per_usec = OSC_HZ/1000000;
-
-void timer_monotonic_get(struct mono_time *mt)
-{
-	uint64_t current_tick;
-	uint64_t usecs_elapsed;
-
-	if (!mono_counter.initialized) {
-		init_timer();
-		mono_counter.last_value = dmtimer_raw_value(0);
-		mono_counter.initialized = 1;
-	}
-
-	current_tick = dmtimer_raw_value(0);
-	usecs_elapsed = (current_tick - mono_counter.last_value) /
-							clocks_per_usec;
-
-	/* Update current time and tick values only if a full tick occurred. */
-	if (usecs_elapsed) {
-		mono_time_add_usecs(&mono_counter.time, usecs_elapsed);
-		mono_counter.last_value = current_tick;
-	}
-
-	/* Save result. */
-	*mt = mono_counter.time;
-}
diff --git a/src/soc/ti/am335x/timer.c b/src/soc/ti/am335x/timer.c
new file mode 100644
index 0000000..58ab811
--- /dev/null
+++ b/src/soc/ti/am335x/timer.c
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <timer.h>
+#include <delay.h>
+#include <device/mmio.h>
+
+#include "dmtimer.h"
+#include "clock.h"
+
+struct am335x_dmtimer *dmtimer_2 = (struct am335x_dmtimer *)0x48040000;
+
+#define CLKSEL_M_OSC (0x01 << 0)
+
+static uint32_t timer_raw_value(void)
+{
+	return read32(&dmtimer_2->tcrr);
+}
+
+void timer_monotonic_get(struct mono_time *mt)
+{
+	mono_time_set_usecs(mt, timer_raw_value() / M_OSC_MHZ);
+}
+
+void init_timer(void)
+{
+	write32(&am335x_cm_dpll->clksel_timer2_clk, CLKSEL_M_OSC);
+
+	// Start the dmtimer in autoreload mode without any prescalers
+	// With M_OSC at 24MHz, this gives a few minutes before the timer overflows
+	write32(&dmtimer_2->tclr, TCLR_ST | TCLR_AR);
+}

[Sam Lewis (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44383

to look at the new patch set (#2).

Change subject: soc/ti/am335x: Fix timer implementation
......................................................................

soc/ti/am335x: Fix timer implementation

Implements the monotonic timer using the am335x dmtimer peripheral.

Change-Id: I4736b6d3b6e26370be9e8f369fc02285ad519223
Signed-off-by: Sam Lewis 
---
M src/soc/ti/am335x/Makefile.inc
D src/soc/ti/am335x/dmtimer.c
M src/soc/ti/am335x/dmtimer.h
D src/soc/ti/am335x/monotonic_timer.c
A src/soc/ti/am335x/timer.c
5 files changed, 59 insertions(+), 61 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/44383/2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44383 )

Change subject: soc/ti/am335x: Fix timer implementation
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44383/5/src/soc/ti/am335x/timer.c 
File src/soc/ti/am335x/timer.c:

https://review.coreboot.org/c/coreboot/+/44383/5/src/soc/ti/am335x/timer.c@1... 
PS5, Line 10: struct am335x_dmtimer *dmtimer_2 = (struct am335x_dmtimer *)0x48040000;
please use a define or a symbol from the memlayout file to get the address
No need for volatile here?

[Sam Lewis (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44383

to look at the new patch set (#6).

Change subject: soc/ti/am335x: Fix timer implementation
......................................................................

soc/ti/am335x: Fix timer implementation

Implements the monotonic timer using the am335x dmtimer peripheral.

Change-Id: I4736b6d3b6e26370be9e8f369fc02285ad519223
Signed-off-by: Sam Lewis 
---
M src/soc/ti/am335x/Makefile.inc
D src/soc/ti/am335x/dmtimer.c
M src/soc/ti/am335x/dmtimer.h
D src/soc/ti/am335x/monotonic_timer.c
A src/soc/ti/am335x/timer.c
5 files changed, 61 insertions(+), 61 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/44383/6

[Sam Lewis (Code Review)]

Sam Lewis has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44383 )

Change subject: soc/ti/am335x: Fix timer implementation
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44383/5/src/soc/ti/am335x/timer.c 
File src/soc/ti/am335x/timer.c:

https://review.coreboot.org/c/coreboot/+/44383/5/src/soc/ti/am335x/timer.c@1... 
PS5, Line 10: struct am335x_dmtimer *dmtimer_2 = (struct am335x_dmtimer *)0x48040000;
> please use a define or a symbol from the memlayout file to get the address […]

Thanks, have defined a symbol now.

I didn't think volatile is necessary on this as it's only accessed through read32/write32 (which cast the addresses as volatile). This seems to be consistent with the way these sort of struct are used across the tree?

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44383 )

Change subject: soc/ti/am335x: Fix timer implementation
......................................................................

Patch Set 9: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44383 )

Change subject: soc/ti/am335x: Fix timer implementation
......................................................................

soc/ti/am335x: Fix timer implementation

Implements the monotonic timer using the am335x dmtimer peripheral.

Change-Id: I4736b6d3b6e26370be9e8f369fc02285ad519223
Signed-off-by: Sam Lewis 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44383
Reviewed-by: Arthur Heymans 
Tested-by: build bot (Jenkins) 
---
M src/soc/ti/am335x/Makefile.inc
D src/soc/ti/am335x/dmtimer.c
M src/soc/ti/am335x/dmtimer.h
D src/soc/ti/am335x/monotonic_timer.c
A src/soc/ti/am335x/timer.c
5 files changed, 61 insertions(+), 61 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Arthur Heymans: Looks good to me, approved

diff --git a/src/soc/ti/am335x/Makefile.inc b/src/soc/ti/am335x/Makefile.inc
index e744c72..60e00fb 100644
--- a/src/soc/ti/am335x/Makefile.inc
+++ b/src/soc/ti/am335x/Makefile.inc
@@ -1,18 +1,15 @@
 ifeq ($(CONFIG_SOC_TI_AM335X),y)
 bootblock-y	+= bootblock.c
 bootblock-y	+= bootblock_media.c
-bootblock-y	+= dmtimer.c
+bootblock-y	+= timer.c
 bootblock-y	+= gpio.c
 bootblock-y	+= pinmux.c
-bootblock-y	+= monotonic_timer.c

romstage-y	+= nand.c
 romstage-y	+= cbmem.c
-romstage-y	+= dmtimer.c
-romstage-y	+= monotonic_timer.c
+romstage-y	+= timer.c

-ramstage-y	+= dmtimer.c
-ramstage-y	+= monotonic_timer.c
+ramstage-y	+= timer.c
 ramstage-y	+= nand.c
 ramstage-y  += soc.c

diff --git a/src/soc/ti/am335x/dmtimer.c b/src/soc/ti/am335x/dmtimer.c
deleted file mode 100644
index b3aa7a1..0000000
--- a/src/soc/ti/am335x/dmtimer.c
+++ /dev/null
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#include "dmtimer.h"
-
-void dmtimer_start(int num)
-{
-}
-
-uint64_t dmtimer_raw_value(int num)
-{
-	return 0;
-}
diff --git a/src/soc/ti/am335x/dmtimer.h b/src/soc/ti/am335x/dmtimer.h
index ad8515f..56f3d0f 100644
--- a/src/soc/ti/am335x/dmtimer.h
+++ b/src/soc/ti/am335x/dmtimer.h
@@ -5,9 +5,33 @@

#include <stdint.h>

-#define OSC_HZ 24000000
+#define M_OSC_MHZ (24)

-void dmtimer_start(int num);
-uint64_t dmtimer_raw_value(int num);
+struct am335x_dmtimer {
+	uint32_t tidr;
+	uint8_t res1[12];
+	uint32_t tiocp_cfg;
+	uint8_t res2[12];
+	uint32_t irq_eoi;
+	uint32_t irqstatus_raw;
+	uint32_t irqstatus;
+	uint32_t irqenable_set;
+	uint32_t irqenable_clr;
+	uint32_t irqwakeen;
+	uint32_t tclr;
+	uint32_t tcrr;
+	uint32_t tldr;
+	uint32_t ttgr;
+	uint32_t twps;
+	uint32_t tmar;
+	uint32_t tcar1;
+	uint32_t tsicr;
+	uint32_t tcar2;
+};
+
+#define TCLR_ST (0x01 << 0)
+#define TCLR_AR (0x01 << 1)
+
+#define DMTIMER_2 (0x48040000)

#endif
diff --git a/src/soc/ti/am335x/monotonic_timer.c b/src/soc/ti/am335x/monotonic_timer.c
deleted file mode 100644
index b57258b..0000000
--- a/src/soc/ti/am335x/monotonic_timer.c
+++ /dev/null
@@ -1,40 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#include <stdint.h>
-#include <delay.h>
-#include <timer.h>
-
-#include "dmtimer.h"
-
-static struct monotonic_counter {
-	int initialized;
-	struct mono_time time;
-	uint64_t last_value;
-} mono_counter;
-
-static const uint32_t clocks_per_usec = OSC_HZ/1000000;
-
-void timer_monotonic_get(struct mono_time *mt)
-{
-	uint64_t current_tick;
-	uint64_t usecs_elapsed;
-
-	if (!mono_counter.initialized) {
-		init_timer();
-		mono_counter.last_value = dmtimer_raw_value(0);
-		mono_counter.initialized = 1;
-	}
-
-	current_tick = dmtimer_raw_value(0);
-	usecs_elapsed = (current_tick - mono_counter.last_value) /
-							clocks_per_usec;
-
-	/* Update current time and tick values only if a full tick occurred. */
-	if (usecs_elapsed) {
-		mono_time_add_usecs(&mono_counter.time, usecs_elapsed);
-		mono_counter.last_value = current_tick;
-	}
-
-	/* Save result. */
-	*mt = mono_counter.time;
-}
diff --git a/src/soc/ti/am335x/timer.c b/src/soc/ti/am335x/timer.c
new file mode 100644
index 0000000..4ed98a3
--- /dev/null
+++ b/src/soc/ti/am335x/timer.c
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <timer.h>
+#include <delay.h>
+#include <device/mmio.h>
+
+#include "dmtimer.h"
+#include "clock.h"
+
+struct am335x_dmtimer *dmtimer_2 = (struct am335x_dmtimer *)DMTIMER_2;
+
+#define CLKSEL_M_OSC (0x01 << 0)
+
+static uint32_t timer_raw_value(void)
+{
+	return read32(&dmtimer_2->tcrr);
+}
+
+void timer_monotonic_get(struct mono_time *mt)
+{
+	mono_time_set_usecs(mt, timer_raw_value() / M_OSC_MHZ);
+}
+
+void init_timer(void)
+{
+	write32(&am335x_cm_dpll->clksel_timer2_clk, CLKSEL_M_OSC);
+
+	// Start the dmtimer in autoreload mode without any prescalers
+	// With M_OSC at 24MHz, this gives a few minutes before the timer overflows
+	write32(&dmtimer_2->tclr, TCLR_ST | TCLR_AR);
+}