[PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Biju Das]

This patch series aims to add support for Compare-Match Timer (TIM)
module found on RZ/V2M SoC.

it is composed of 32 channels and channels 0-7 and 24-32 are
reserved for ISP usage.

Channel 22 is modelled as clock source and Channel 23 is modelled as clock
event driver and the rest of the channels are modelled as counter driver
as it provides

1) counter for counting
2) configurable counter value for generating timer interrupt
3) userspace event for each interrupt.

logs:-
Counter driver:
Counter driver is tested by reading counts and interrupts tested by
counter-example in tools/counter/counter_example.c

Count snapshot value:
3114
Output from counter_example when it triggers interrupts:
Timestamp 0: 24142152969        Count 0: 5
Error Message 0: Success

Clock source:
Clock source driver is tested by clock-source-switch app.
[ 1275.703567] clocksource: Switched to clocksource arch_sys_counter
[ 1275.710189] clocksource: Switched to clocksource a4000b00.timer
[OK]
# Totals: pass[ 1275.718112] clocksource: Switched to clocksource arch_sys_counter
:0 fail:0 xfail:0 xpass:0 skip:0 error:0
# Totals: pass:0 fail:0 xfail:0 xpass:0 skip:0 error:0

Clock event:
clock event driver is tested by changing the rating as 500.

Biju Das (6):
  clk: renesas: r9a09g011: Add TIM clock and reset entries
  dt-bindings: timer: Add RZ/V2M TIM binding
  clocksource/drivers/rzv2m-tim: Add Renesas RZ/V2M compare match
    timer(TIM) driver
  dt-bindings: counter: Add RZ/V2M TIM counter binding
  counter: Add Renesas RZ/V2M TIM counter driver
  arm64: dts: renesas: r9a09g011: Add tim nodes

 .../counter/renesas,rzv2m-tim-cnt.yaml        |  83 +++++
 .../bindings/timer/renesas,rzv2m-tim.yaml     |  83 +++++
 arch/arm64/boot/dts/renesas/r9a09g011.dtsi    | 192 ++++++++++
 drivers/clk/renesas/r9a09g011-cpg.c           |  22 ++
 drivers/clocksource/Kconfig                   |   7 +
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/rzv2m-tim.c               | 330 ++++++++++++++++++
 drivers/counter/Kconfig                       |  10 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/rzv2m-tim-cnt.c               | 312 +++++++++++++++++
 10 files changed, 1041 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/renesas,rzv2m-tim-cnt.yaml
 create mode 100644 Documentation/devicetree/bindings/timer/renesas,rzv2m-tim.yaml
 create mode 100644 drivers/clocksource/rzv2m-tim.c
 create mode 100644 drivers/counter/rzv2m-tim-cnt.c

-- 
2.25.1

[PATCH 5/6] counter: Add Renesas RZ/V2M TIM counter driver

[Biju Das]

Add Renesas RZ/V2M TIM(Compare Match Timer) counter driver. This IP
supports 32 bit counter and it generates an interrupt request
signal every cycle set in the TIM counter.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/counter/Kconfig         |  10 +
 drivers/counter/Makefile        |   1 +
 drivers/counter/rzv2m-tim-cnt.c | 312 ++++++++++++++++++++++++++++++++
 3 files changed, 323 insertions(+)
 create mode 100644 drivers/counter/rzv2m-tim-cnt.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index d388bf26f4dc..c705b557117f 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -39,6 +39,16 @@ config INTERRUPT_CNT
 	  To compile this driver as a module, choose M here: the
 	  module will be called interrupt-cnt.
 
+config RZV2M_TIM_CNT
+	tristate "Renesas RZ/V2M counter driver"
+	depends on ARCH_R9A09G011 || COMPILE_TEST
+	help
+	  Enable support for RZ/V2M counter driver found on Renesas RZ/V2M alike
+	  SoCs. This IP supports 32 bit counter.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rzv2m-tim-cnt.
+
 config STM32_TIMER_CNT
 	tristate "STM32 Timer encoder counter driver"
 	depends on MFD_STM32_TIMERS || COMPILE_TEST
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index b9a369e0d4fc..1f09ea735ddf 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -8,6 +8,7 @@ counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
 
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_INTERRUPT_CNT)		+= interrupt-cnt.o
+obj-$(CONFIG_RZV2M_TIM_CNT)	+= rzv2m-tim-cnt.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
diff --git a/drivers/counter/rzv2m-tim-cnt.c b/drivers/counter/rzv2m-tim-cnt.c
new file mode 100644
index 000000000000..6c3b738a3e39
--- /dev/null
+++ b/drivers/counter/rzv2m-tim-cnt.c
@@ -0,0 +1,312 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2M Compare Match Timer (TIM) driver
+ *
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/counter.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+/* TIM REGISTERS */
+#define RZV2M_TIM_TMD			0x0
+#define RZV2M_TIM_CMD			0x4
+#define RZV2M_TIM_TMCD			0x8
+#define RZV2M_TIM_INTCLR		0xc
+
+#define RZV2M_TIM_TMCD_CS		GENMASK(6, 4)
+#define RZV2M_TIM_TMCD_INTMODE		BIT(2)
+#define RZV2M_TIM_TMCD_CE		BIT(1)
+#define RZV2M_TIM_TMCD_CAE		BIT(0)
+
+#define RZV2M_TIM_INTCLR_INTCLR		BIT(0)
+
+#define RZV2M_TIM_TMCD_CH_EN	(RZV2M_TIM_TMCD_CE | RZV2M_TIM_TMCD_CAE)
+#define RZV2M_TIM_TMCD_START	(RZV2M_TIM_TMCD_CH_EN | RZV2M_TIM_TMCD_INTMODE)
+
+#define F2CYCLE_NSEC(f)		(1000000000 / (f))
+
+/**
+ * struct rzv2m_tim_channel - TIM counter private data
+ *
+ * @dev: Channel device
+ * @base: Base address of the chanel
+ * @rstc: Reset handle
+ * @delay: write intervel for register writes
+ * @ceiling: Cache for ceiling value
+ */
+
+struct rzv2m_tim_channel {
+	struct device *dev;
+	void __iomem *base;
+	struct reset_control *rstc;
+	unsigned long delay;
+	u32 ceiling;
+};
+
+static void rzv2m_tim_cnt_wait_delay(struct rzv2m_tim_channel *ch)
+{
+	/* delay required when changing the register settings */
+	ndelay(ch->delay);
+}
+
+static bool rzv2m_tim_cnt_is_counter_enabled(struct counter_device *counter)
+{
+	struct rzv2m_tim_channel *const ch = counter_priv(counter);
+	unsigned long tmcd;
+
+	tmcd = readl(ch->base + RZV2M_TIM_TMCD);
+	return !!FIELD_GET(RZV2M_TIM_TMCD_CH_EN, tmcd);
+}
+
+static void rzv2m_tim_cnt_stop(struct rzv2m_tim_channel *ch)
+{
+	unsigned long tmcd;
+
+	tmcd = readl(ch->base + RZV2M_TIM_TMCD);
+	if (FIELD_GET(RZV2M_TIM_TMCD_CH_EN, tmcd)) {
+		writel(FIELD_GET(RZV2M_TIM_TMCD_CS, tmcd),
+		       ch->base + RZV2M_TIM_TMCD);
+		rzv2m_tim_cnt_wait_delay(ch);
+	}
+}
+
+static void rzv2m_tim_cnt_start(struct rzv2m_tim_channel *ch)
+{
+	unsigned long tmcd;
+
+	tmcd = readl(ch->base + RZV2M_TIM_TMCD);
+	writel(tmcd | RZV2M_TIM_TMCD_START, ch->base + RZV2M_TIM_TMCD);
+	rzv2m_tim_cnt_wait_delay(ch);
+}
+
+static irqreturn_t rzv2m_tim_cnt_interrupt(int irq, void *dev_id)
+{
+	struct rzv2m_tim_channel *const ch = counter_priv(dev_id);
+
+	writel(RZV2M_TIM_INTCLR_INTCLR, ch->base + RZV2M_TIM_INTCLR);
+	counter_push_event(dev_id, COUNTER_EVENT_OVERFLOW, 0);
+
+	return IRQ_HANDLED;
+}
+
+static int rzv2m_tim_cnt_read(struct counter_device *counter,
+			      struct counter_count *count, u64 *val)
+{
+	struct rzv2m_tim_channel *const ch = counter_priv(counter);
+
+	*val = readl(ch->base + RZV2M_TIM_TMD);
+
+	return 0;
+}
+
+static int rzv2m_tim_cnt_ceiling_read(struct counter_device *counter,
+				      struct counter_count *count,
+				      u64 *ceiling)
+{
+	struct rzv2m_tim_channel *const ch = counter_priv(counter);
+
+	*ceiling = ch->ceiling;
+
+	return 0;
+}
+
+static int rzv2m_tim_cnt_ceiling_write(struct counter_device *counter,
+				       struct counter_count *count,
+				       u64 ceiling)
+{
+	struct rzv2m_tim_channel *const ch = counter_priv(counter);
+	bool enable = rzv2m_tim_cnt_is_counter_enabled(counter);
+
+	if (ceiling > U32_MAX)
+		return -ERANGE;
+
+	if (enable)
+		rzv2m_tim_cnt_stop(ch);
+	ch->ceiling = ceiling;
+	writel(ceiling, ch->base + RZV2M_TIM_CMD);
+	if (enable)
+		rzv2m_tim_cnt_start(ch);
+
+	return 0;
+}
+
+static int rzv2m_tim_cnt_enable_read(struct counter_device *counter,
+				     struct counter_count *count, u8 *enable)
+{
+	struct rzv2m_tim_channel *const ch = counter_priv(counter);
+
+	pm_runtime_get_sync(ch->dev);
+	*enable = rzv2m_tim_cnt_is_counter_enabled(counter);
+	pm_runtime_put(ch->dev);
+
+	return 0;
+}
+
+static int rzv2m_tim_cnt_enable_write(struct counter_device *counter,
+				      struct counter_count *count, u8 enable)
+{
+	struct rzv2m_tim_channel *const ch = counter_priv(counter);
+
+	if (enable) {
+		pm_runtime_get_sync(ch->dev);
+		rzv2m_tim_cnt_start(ch);
+	} else {
+		rzv2m_tim_cnt_stop(ch);
+		pm_runtime_put(ch->dev);
+	}
+
+	return 0;
+}
+
+static const struct counter_ops rzv2m_tim_cnt_ops = {
+	.count_read = rzv2m_tim_cnt_read,
+};
+
+static struct counter_comp rzv2m_tim_cnt_ext[] = {
+	COUNTER_COMP_ENABLE(rzv2m_tim_cnt_enable_read,
+			    rzv2m_tim_cnt_enable_write),
+	COUNTER_COMP_CEILING(rzv2m_tim_cnt_ceiling_read,
+			     rzv2m_tim_cnt_ceiling_write),
+};
+
+static struct counter_count rzv2m_tim_counts[] = {
+	{
+		.id = 0,
+		.name = "Channel Count",
+		.ext = rzv2m_tim_cnt_ext,
+		.num_ext = ARRAY_SIZE(rzv2m_tim_cnt_ext),
+	},
+};
+
+static void rzv2m_tim_cnt_reset_assert_pm_disable(void *data)
+{
+	struct rzv2m_tim_channel *ch = data;
+
+	reset_control_assert(ch->rstc);
+	pm_runtime_disable(ch->dev);
+}
+
+static int rzv2m_tim_cnt_probe(struct platform_device *pdev)
+{
+	struct counter_device *counter;
+	struct rzv2m_tim_channel *ch;
+	unsigned long apb_rate, rate;
+	struct clk *clk;
+	int irq;
+	int ret;
+
+	counter = devm_counter_alloc(&pdev->dev, sizeof(*ch));
+	if (!counter)
+		return -ENOMEM;
+
+	ch = counter_priv(counter);
+	ch->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ch->base))
+		return PTR_ERR(ch->base);
+
+	ch->rstc = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+	if (IS_ERR(ch->rstc))
+		return dev_err_probe(&pdev->dev, PTR_ERR(ch->rstc),
+				     "get reset failed\n");
+
+	ch->dev = &pdev->dev;
+	clk = clk_get(&pdev->dev, "tim");
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+				     "cannot get tim clock\n");
+	rate = clk_get_rate(clk);
+	clk_put(clk);
+	if (!rate)
+		return dev_err_probe(&pdev->dev, -EINVAL, "tim clk rate is 0");
+
+	clk = clk_get(&pdev->dev, "apb");
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+				     "cannot get apb clock\n");
+	apb_rate = clk_get_rate(clk);
+	clk_put(clk);
+	if (!apb_rate)
+		return dev_err_probe(&pdev->dev, -EINVAL, "apb clk rate is 0");
+
+	/* delay = 5 * PCLK + 5 * INCLK */
+	ch->delay = F2CYCLE_NSEC(apb_rate) * 5 + F2CYCLE_NSEC(rate) * 5;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	pm_runtime_enable(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0) {
+		dev_err_probe(&pdev->dev, ret,
+			      "pm_runtime_resume_and_get failed");
+		goto err_pm_disable;
+	}
+
+	reset_control_deassert(ch->rstc);
+	writel(U32_MAX, ch->base + RZV2M_TIM_CMD);
+	writel(FIELD_PREP(RZV2M_TIM_TMCD_CS, 0), ch->base  + RZV2M_TIM_TMCD);
+	pm_runtime_put(&pdev->dev);
+	platform_set_drvdata(pdev, ch);
+
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rzv2m_tim_cnt_reset_assert_pm_disable,
+				       ch);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_request_irq(&pdev->dev, irq,
+			       rzv2m_tim_cnt_interrupt,
+			       0,
+			       dev_name(&pdev->dev), counter);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to request irq%d\n", irq);
+
+	counter->name = dev_name(&pdev->dev);
+	counter->parent = &pdev->dev;
+	counter->ops = &rzv2m_tim_cnt_ops;
+	counter->counts = rzv2m_tim_counts;
+	counter->num_counts = ARRAY_SIZE(rzv2m_tim_counts);
+
+	/* Register Counter device */
+	ret = devm_counter_add(&pdev->dev, counter);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "Failed to add counter\n");
+
+	return 0;
+
+err_pm_disable:
+	pm_runtime_disable(&pdev->dev);
+	return ret;
+}
+
+static const struct of_device_id rzv2m_tim_of_table[] = {
+	{ .compatible = "renesas,rzv2m-tim-cnt", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzv2m_tim_of_table);
+
+static struct platform_driver rzv2m_tim_cnt_device_driver = {
+	.driver = {
+		.name = "rzv2m_tim_cnt",
+		.of_match_table = of_match_ptr(rzv2m_tim_of_table),
+	},
+	.probe = rzv2m_tim_cnt_probe,
+};
+module_platform_driver(rzv2m_tim_cnt_device_driver);
+
+MODULE_DESCRIPTION("Renesas RZ/V2M Compare Match Timer Driver");
+MODULE_ALIAS("platform:rzv2m-tim-counter");
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(COUNTER);
-- 
2.25.1

Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Rob Herring]

On Mon, Dec 05, 2022 at 02:59:49PM +0000, Biju Das wrote:
> This patch series aims to add support for Compare-Match Timer (TIM)
> module found on RZ/V2M SoC.
> 
> it is composed of 32 channels and channels 0-7 and 24-32 are
> reserved for ISP usage.
> 
> Channel 22 is modelled as clock source and Channel 23 is modelled as clock
> event driver and the rest of the channels are modelled as counter driver
> as it provides

Why did you pick those 2 counters for those functions?

Unless the h/w blocks are different, this is an abuse of compatible 
strings. What's the h/w difference that makes you care which counter the 
OS picks? That's what the DT should describe. If any timer will do, just 
let the OS pick.

> 
> 1) counter for counting
> 2) configurable counter value for generating timer interrupt
> 3) userspace event for each interrupt.
> 
> logs:-
> Counter driver:
> Counter driver is tested by reading counts and interrupts tested by
> counter-example in tools/counter/counter_example.c
> 
> Count snapshot value:
> 3114
> Output from counter_example when it triggers interrupts:
> Timestamp 0: 24142152969        Count 0: 5
> Error Message 0: Success
> 
> Clock source:
> Clock source driver is tested by clock-source-switch app.
> [ 1275.703567] clocksource: Switched to clocksource arch_sys_counter
> [ 1275.710189] clocksource: Switched to clocksource a4000b00.timer

Do you have any use case to really switch. Doing so disables the vDSO 
access to the clocksource.

Rob

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Biju Das]

Hi Rob,

Thanks for the feedback.

> Subject: Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support
> 
> On Mon, Dec 05, 2022 at 02:59:49PM +0000, Biju Das wrote:
> > This patch series aims to add support for Compare-Match Timer (TIM)
> > module found on RZ/V2M SoC.
> >
> > it is composed of 32 channels and channels 0-7 and 24-32 are reserved
> > for ISP usage.
> >
> > Channel 22 is modelled as clock source and Channel 23 is modelled as
> > clock event driver and the rest of the channels are modelled as
> > counter driver as it provides
> 
> Why did you pick those 2 counters for those functions?

Currently it uses architecture timer for broadcast timer, so I thought
Since TIM has 24 channels, use 1 channel for broadcast timer and 1
Channel for clock source. But having said that SoC has an aarch64 architecture
clock source strictly speaking we don't need this.

> 
> Unless the h/w blocks are different, this is an abuse of compatible
> strings. What's the h/w difference that makes you care which counter the
> OS picks? That's what the DT should describe. If any timer will do, just
> let the OS pick.

There is no HW difference. Same HW block can be used for mutually exclusive
functionality.

One is for Linux Clock source/event functionality((scheduler tick/broadcast tick etc) and 

the other purpose is to expose count and event ticks from this module to user space,
so that wide range of applications can make use of it.

If it is an abuse of compatible strings for mutually exclusive functionality
, then I would like to drop clock source and use all the channels as 
Either clock events(for broadcast ticks and real time usage??) or as counters.

If this is not OK, then I need to pick one. I will go with counters.

Please share your thoughts.

> 
> >
> > 1) counter for counting
> > 2) configurable counter value for generating timer interrupt
> > 3) userspace event for each interrupt.
> >
> > logs:-
> > Counter driver:
> > Counter driver is tested by reading counts and interrupts tested by
> > counter-example in tools/counter/counter_example.c
> >
> > Count snapshot value:
> > 3114
> > Output from counter_example when it triggers interrupts:
> > Timestamp 0: 24142152969        Count 0: 5
> > Error Message 0: Success
> >
> > Clock source:
> > Clock source driver is tested by clock-source-switch app.
> > [ 1275.703567] clocksource: Switched to clocksource arch_sys_counter
> > [ 1275.710189] clocksource: Switched to clocksource a4000b00.timer
> 
> Do you have any use case to really switch. Doing so disables the vDSO
> access to the clocksource.

Not really. Architecture timer should be sufficient for clocksource.

Cheers,
Biju

Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Geert Uytterhoeven]

Hi Biju,

On Tue, Dec 6, 2022 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support
> > On Mon, Dec 05, 2022 at 02:59:49PM +0000, Biju Das wrote:
> > > This patch series aims to add support for Compare-Match Timer (TIM)
> > > module found on RZ/V2M SoC.
> > >
> > > it is composed of 32 channels and channels 0-7 and 24-32 are reserved
> > > for ISP usage.
> > >
> > > Channel 22 is modelled as clock source and Channel 23 is modelled as
> > > clock event driver and the rest of the channels are modelled as
> > > counter driver as it provides
> >
> > Why did you pick those 2 counters for those functions?
>
> Currently it uses architecture timer for broadcast timer, so I thought
> Since TIM has 24 channels, use 1 channel for broadcast timer and 1
> Channel for clock source. But having said that SoC has an aarch64 architecture
> clock source strictly speaking we don't need this.
>
> > Unless the h/w blocks are different, this is an abuse of compatible
> > strings. What's the h/w difference that makes you care which counter the
> > OS picks? That's what the DT should describe. If any timer will do, just
> > let the OS pick.
>
> There is no HW difference. Same HW block can be used for mutually exclusive
> functionality.
>
> One is for Linux Clock source/event functionality((scheduler tick/broadcast tick etc) and
>
> the other purpose is to expose count and event ticks from this module to user space,
> so that wide range of applications can make use of it.
>
> If it is an abuse of compatible strings for mutually exclusive functionality
> , then I would like to drop clock source and use all the channels as
> Either clock events(for broadcast ticks and real time usage??) or as counters.
>
> If this is not OK, then I need to pick one. I will go with counters.
>
> Please share your thoughts.

Can't you handle this like sh_cmt.c does:

        /*
         * Use the first channel as a clock event device and the second channel
         * as a clock source. If only one channel is available use it for both.
         */

> > > 1) counter for counting
> > > 2) configurable counter value for generating timer interrupt
> > > 3) userspace event for each interrupt.
> > >
> > > logs:-
> > > Counter driver:
> > > Counter driver is tested by reading counts and interrupts tested by
> > > counter-example in tools/counter/counter_example.c
> > >
> > > Count snapshot value:
> > > 3114
> > > Output from counter_example when it triggers interrupts:
> > > Timestamp 0: 24142152969        Count 0: 5
> > > Error Message 0: Success
> > >
> > > Clock source:
> > > Clock source driver is tested by clock-source-switch app.
> > > [ 1275.703567] clocksource: Switched to clocksource arch_sys_counter
> > > [ 1275.710189] clocksource: Switched to clocksource a4000b00.timer
> >
> > Do you have any use case to really switch. Doing so disables the vDSO
> > access to the clocksource.
>
> Not really. Architecture timer should be sufficient for clocksource.

When multiple clocksources are registered, the clocksource
subsystems picks the best one anyway, right?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Thomas Gleixner]

On Tue, Dec 06 2022 at 09:40, Geert Uytterhoeven wrote:
> On Tue, Dec 6, 2022 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>> > Do you have any use case to really switch. Doing so disables the vDSO
>> > access to the clocksource.
>>
>> Not really. Architecture timer should be sufficient for clocksource.
>
> When multiple clocksources are registered, the clocksource
> subsystems picks the best one anyway, right?

As it does for the clock event devices. If there is an architected timer
then that should be always preferred.

No idea why there is a need for the extra hardware and the drivers which
are both never utilized.

Thanks,

        tglx

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support
> 
> Hi Biju,
> 
> On Tue, Dec 6, 2022 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM)
> > > support On Mon, Dec 05, 2022 at 02:59:49PM +0000, Biju Das wrote:
> > > > This patch series aims to add support for Compare-Match Timer
> > > > (TIM) module found on RZ/V2M SoC.
> > > >
> > > > it is composed of 32 channels and channels 0-7 and 24-32 are
> > > > reserved for ISP usage.
> > > >
> > > > Channel 22 is modelled as clock source and Channel 23 is modelled
> > > > as clock event driver and the rest of the channels are modelled as
> > > > counter driver as it provides
> > >
> > > Why did you pick those 2 counters for those functions?
> >
> > Currently it uses architecture timer for broadcast timer, so I thought
> > Since TIM has 24 channels, use 1 channel for broadcast timer and 1
> > Channel for clock source. But having said that SoC has an aarch64
> > architecture clock source strictly speaking we don't need this.
> >
> > > Unless the h/w blocks are different, this is an abuse of compatible
> > > strings. What's the h/w difference that makes you care which counter
> > > the OS picks? That's what the DT should describe. If any timer will
> > > do, just let the OS pick.
> >
> > There is no HW difference. Same HW block can be used for mutually
> > exclusive functionality.
> >
> > One is for Linux Clock source/event functionality((scheduler
> > tick/broadcast tick etc) and
> >
> > the other purpose is to expose count and event ticks from this module
> > to user space, so that wide range of applications can make use of it.
> >
> > If it is an abuse of compatible strings for mutually exclusive
> > functionality , then I would like to drop clock source and use all the
> > channels as Either clock events(for broadcast ticks and real time
> usage??) or as counters.
> >
> > If this is not OK, then I need to pick one. I will go with counters.
> >
> > Please share your thoughts.
> 
> Can't you handle this like sh_cmt.c does:
> 
>         /*
>          * Use the first channel as a clock event device and the second
> channel
>          * as a clock source. If only one channel is available use it for
> both.
>          */

Currently it is handled like above except for the case " If only one channel is available use it for
Both", see patch#3 [1] probe function.
The only difference is here we have separate address space, clocks, and interrupts for each channel.
[1]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20221205145955.391526-4-biju.das.jz@bp.renesas.com/

Our customer BSP, uses this hw module for exposing timer interrupt event to user space
by using custom driver. The same functionality can be achieved through counter driver.
That is the reason, I have added counter driver to expose this functionality as well.

> 
> > > > 1) counter for counting
> > > > 2) configurable counter value for generating timer interrupt
> > > > 3) userspace event for each interrupt.
> > > >
> > > > logs:-
> > > > Counter driver:
> > > > Counter driver is tested by reading counts and interrupts tested
> > > > by counter-example in tools/counter/counter_example.c
> > > >
> > > > Count snapshot value:
> > > > 3114
> > > > Output from counter_example when it triggers interrupts:
> > > > Timestamp 0: 24142152969        Count 0: 5
> > > > Error Message 0: Success
> > > >
> > > > Clock source:
> > > > Clock source driver is tested by clock-source-switch app.
> > > > [ 1275.703567] clocksource: Switched to clocksource
> > > > arch_sys_counter [ 1275.710189] clocksource: Switched to
> > > > clocksource a4000b00.timer
> > >
> > > Do you have any use case to really switch. Doing so disables the
> > > vDSO access to the clocksource.
> >
> > Not really. Architecture timer should be sufficient for clocksource.
> 
> When multiple clocksources are registered, the clocksource subsystems
> picks the best one anyway, right?

Yes, it picks based on the rating.

Cheers,
Biju

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Biju Das]

> Subject: Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support
> 
> On Tue, Dec 06 2022 at 09:40, Geert Uytterhoeven wrote:
> > On Tue, Dec 6, 2022 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> >> > Do you have any use case to really switch. Doing so disables the
> >> > vDSO access to the clocksource.
> >>
> >> Not really. Architecture timer should be sufficient for clocksource.
> >
> > When multiple clocksources are registered, the clocksource subsystems
> > picks the best one anyway, right?
> 
> As it does for the clock event devices. If there is an architected timer
> then that should be always preferred.
> 
> No idea why there is a need for the extra hardware and the drivers which
> are both never utilized.

Maybe, if you use secure OS, this timer hardware can be used there.

If you use PM deep states, Maybe, it could be used as broadcast event for waking up the system??

Cheers,
Biju

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Biju Das]

Hi Thomas Gleixner and Geert,

> Subject: Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support
> 
> On Tue, Dec 06 2022 at 09:40, Geert Uytterhoeven wrote:
> > On Tue, Dec 6, 2022 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> >> > Do you have any use case to really switch. Doing so disables the
> >> > vDSO access to the clocksource.
> >>
> >> Not really. Architecture timer should be sufficient for clocksource.
> >
> > When multiple clocksources are registered, the clocksource subsystems
> > picks the best one anyway, right?
> 
> As it does for the clock event devices. If there is an architected timer
> then that should be always preferred.
> 
> No idea why there is a need for the extra hardware and the drivers which
> are both never utilized.

I got feedback from BSP team for the actual usage of this timer.

Basically, this HW timer is used for measuring the processing time
of DRP-AI accurately compared to the CPU timer normally we use.

The example use cases,
Timer in FREERUN mode, Check the timer value after the restart(1usec)"
Timer in FREERUN mode, Check the timer value after the restart(10000000usec)"

What is the model to be used for this kind of HW usage? Counter or Timer?

I can think of one possible HW usage by using Counter model.
Not sure how timer model can be used for this kind of HW usage??

Eg: we can set ceiling values 1usec and 10000000usec using counter framework
  And that will trigger interrupt events corresponding to the ceiling values
  to user space and user space app can accurately measure the DRP-AI processing time.

Also counter model exposes count values to user space from the counter HW.

Cheers,
Biju

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Thomas Gleixner]

Biju!

On Wed, Dec 07 2022 at 07:52, Biju Das wrote:
>> On Tue, Dec 06 2022 at 09:40, Geert Uytterhoeven wrote:
>> > When multiple clocksources are registered, the clocksource subsystems
>> > picks the best one anyway, right?
>> 
>> As it does for the clock event devices. If there is an architected timer
>> then that should be always preferred.
>> 
>> No idea why there is a need for the extra hardware and the drivers which
>> are both never utilized.
>
> I got feedback from BSP team for the actual usage of this timer.
>
> Basically, this HW timer is used for measuring the processing time
> of DRP-AI accurately compared to the CPU timer normally we use.

How is a slow to access timer with a lower clock frequency more
accurate?

> The example use cases,
> Timer in FREERUN mode, Check the timer value after the restart(1usec)"
> Timer in FREERUN mode, Check the timer value after the restart(10000000usec)"
>
> What is the model to be used for this kind of HW usage? Counter or Timer?
>
> I can think of one possible HW usage by using Counter model.
> Not sure how timer model can be used for this kind of HW usage??
>
> Eg: we can set ceiling values 1usec and 10000000usec using counter framework
>   And that will trigger interrupt events corresponding to the ceiling values
>   to user space and user space app can accurately measure the DRP-AI processing time.
>
> Also counter model exposes count values to user space from the counter HW.

Counter subsystem != clocksource/event subsystem.

We are debating a clocksource/clockevent driver and not a counter
driver, right?

Thanks,

        tglx

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Biju Das]

Hi Thomas Gleixner,

Thanks for the feedback.

> Subject: RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support
> 
> Biju!
> 
> On Wed, Dec 07 2022 at 07:52, Biju Das wrote:
> >> On Tue, Dec 06 2022 at 09:40, Geert Uytterhoeven wrote:
> >> > When multiple clocksources are registered, the clocksource
> >> > subsystems picks the best one anyway, right?
> >>
> >> As it does for the clock event devices. If there is an architected
> >> timer then that should be always preferred.
> >>
> >> No idea why there is a need for the extra hardware and the drivers
> >> which are both never utilized.
> >
> > I got feedback from BSP team for the actual usage of this timer.
> >
> > Basically, this HW timer is used for measuring the processing time of
> > DRP-AI accurately compared to the CPU timer normally we use.
> 
> How is a slow to access timer with a lower clock frequency more accurate?

But our tick frequency for arm64 defconfig is CONFIG_HZ_250=y. So we get timer interrupt
at every 4 msec. 

How do we get timer event interrupt, eg: for 1 microsec?

> 
> > The example use cases,
> > Timer in FREERUN mode, Check the timer value after the restart(1usec)"
> > Timer in FREERUN mode, Check the timer value after the
> restart(10000000usec)"
> >
> > What is the model to be used for this kind of HW usage? Counter or
> Timer?
> >
> > I can think of one possible HW usage by using Counter model.
> > Not sure how timer model can be used for this kind of HW usage??
> >
> > Eg: we can set ceiling values 1usec and 10000000usec using counter
> framework
> >   And that will trigger interrupt events corresponding to the ceiling
> values
> >   to user space and user space app can accurately measure the DRP-AI
> processing time.
> >
> > Also counter model exposes count values to user space from the counter
> HW.
> 
> Counter subsystem != clocksource/event subsystem.
> 
> We are debating a clocksource/clockevent driver and not a counter driver,
> right?

Yes, Rob pointed out we should not misuse the compatibles as I have both
Timer and counter bindings for a given HW timer.

Timer, It can be used as broadcast and highres timer for RT.

Counter, It can be used as measuring the processing time of DRP-AI.

What is the best way to use this hardware to take care of all this use cases? 

So far all the Renesas timers used timer model. But for RZ/V2M the HW usage is different.
The customer BSP mainly uses this timer for measuring the processing time of DRP-AI, 
so the expectation is we should at least have support for this use case.

Cheers,
Biju

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Thomas Gleixner]

Biju!

On Wed, Dec 07 2022 at 11:35, Biju Das wrote:
>> Subject: RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support
>> On Wed, Dec 07 2022 at 07:52, Biju Das wrote:
>> > Basically, this HW timer is used for measuring the processing time of
>> > DRP-AI accurately compared to the CPU timer normally we use.
>> 
>> How is a slow to access timer with a lower clock frequency more accurate?
>
> But our tick frequency for arm64 defconfig is CONFIG_HZ_250=y. So we
> get timer interrupt at every 4 msec.

CONFIG_HIGH_RES_TIMERS=y

> How do we get timer event interrupt, eg: for 1 microsec?

clock_nanosleep(...);

Though 1usec is wishful thinking with either variant of timer hardware.

>> 
>> We are debating a clocksource/clockevent driver and not a counter driver,
>> right?
>
> Yes, Rob pointed out we should not misuse the compatibles as I have both
> Timer and counter bindings for a given HW timer.
>
> Timer, It can be used as broadcast and highres timer for RT.

I buy the broadcast part if you really have a ARM architected timer
which stops in deep idle states. Highres for RT, no! The arm architected
timer works perfectly fine for that.

> Counter, It can be used as measuring the processing time of DRP-AI.

Sigh. You can do that with the architected timer too, especially when
you are going to do the measurement in user space.

clock_gettime(), which uses the VDSO with the architected timer is fast
to access and accurate.

Thanks,

        tglx

Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]

On Wed, Dec 07, 2022 at 05:49:09PM +0100, Thomas Gleixner wrote:
> On Wed, Dec 07 2022 at 11:35, Biju Das wrote:
> > Counter, It can be used as measuring the processing time of DRP-AI.
> 
> Sigh. You can do that with the architected timer too, especially when
> you are going to do the measurement in user space.
> 
> clock_gettime(), which uses the VDSO with the architected timer is fast
> to access and accurate.
> 
> Thanks,
> 
>         tglx

Hi Biju,

It's true that you could implement a Counter driver to achieve what you
want here, but I don't think that's the most apt interface for this
device. Your device is used to measure the processing time of DRP-AI, so
modeling this as a clocksource seems like the right approach to take.

Of course, if there is something missing from clocksource/clockevent
that you need, then it should be added to the subsystem. So let's try to
narrow down exactly what functionality you need.

You gave a Counter use-case example earlier where you can configure the
ceiling value of the timer (e.g. to 1usec or 10000000usec) and push
Counter events on the interrupts that trigger off that that
configuration; the Counter subsystem can logs the current system time
everytime a Counter event is pushed.

Could the same thing be achieved using clockevents framework instead?
With this approach you would register an event to fire in the future
(e.g. 1usec or 10000000usec) and then call clock_gettime() to get the
current system when you're notified of the event. Would this approach
work for your use-case, or is something else missing here?

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Biju Das]

Hi William Breathitt Gray,

Thanks for the feedback.

> Subject: Re: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support
> 
> On Wed, Dec 07, 2022 at 05:49:09PM +0100, Thomas Gleixner wrote:
> > On Wed, Dec 07 2022 at 11:35, Biju Das wrote:
> > > Counter, It can be used as measuring the processing time of DRP-AI.
> >
> > Sigh. You can do that with the architected timer too, especially when
> > you are going to do the measurement in user space.
> >
> > clock_gettime(), which uses the VDSO with the architected timer is
> > fast to access and accurate.
> >
> > Thanks,
> >
> >         tglx
> 
> Hi Biju,
> 
> It's true that you could implement a Counter driver to achieve what you
> want here, but I don't think that's the most apt interface for this
> device. Your device is used to measure the processing time of DRP-AI, so
> modeling this as a clocksource seems like the right approach to take.
> 
> Of course, if there is something missing from clocksource/clockevent that
> you need, then it should be added to the subsystem. So let's try to narrow
> down exactly what functionality you need.
> 
> You gave a Counter use-case example earlier where you can configure the
> ceiling value of the timer (e.g. to 1usec or 10000000usec) and push
> Counter events on the interrupts that trigger off that that configuration;
> the Counter subsystem can logs the current system time everytime a Counter
> event is pushed.
> 
> Could the same thing be achieved using clockevents framework instead?

If I am correct, from this thread discussion, we can make use of architecture timer
with hrtimer, which will give call back events to user space after time expires. 
In the callback, we could call clock_gettime(), which uses the VDSO which is fast
and accurate.

So there is no need to use any extra HW timer.

scheduling tick is 4millisec. so if we want callback at 1 microsec, then we need to 
use clock_nanosleep. Getting 1 microsec callback to user space is challenging as
the scheduling tick is only 4 millisec.

Cheers,
Biju

> With this approach you would register an event to fire in the future (e.g.
> 1usec or 10000000usec) and then call clock_gettime() to get the current
> system when you're notified of the event. Would this approach work for
> your use-case, or is something else missing here?

RE: [PATCH 0/6] Add RZ/V2M Compare-Match Timer (TIM) support

[Thomas Gleixner]

On Sat, Dec 10 2022 at 07:52, Biju Das wrote:
> scheduling tick is 4millisec. so if we want callback at 1 microsec,
> then we need to use clock_nanosleep. Getting 1 microsec callback to
> user space is challenging as the scheduling tick is only 4 millisec.

The tick is only relevant if high resolution timers are disabled because
then hrtimers are expired in the tick. If high resolution timers are
enabled then the hrtimer expiry happens at the exact expiry time.

What's challenging about the 1 microsecond accuracy is that the system
immanent latencies are already in that range. So while the timer fires
exactly, the actual execution of the woken up task in user space is not
exact as that is subject to the worst case sum of latencies in the
system.

Thanks,

        tglx