[PATCH 0/2] clocksource: move System Tick Counter from 'arch/mips/ralink'

[PATCH 0/2] clocksource: move System Tick Counter from 'arch/mips/ralink'

[Sergio Paracuellos]

Hi Daniel,

System Tick Counter is present in RT3352 and MT7620 Ralink SoCs. This driver has 
been in 'arch/mips/ralink' from the beggining and can be easily moved into a more
accurate place in 'drivers/clocksource' folder. This makes easier to enable it
for compile test targets as well as reduce LOC in architecture specific folders.
Bindings are already mainlined and can be located here [0].

Thanks in advance for your time.

Best regards,
   Sergio Paracuellos

[0]: https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/timer/ralink,cevt-systick.yaml

Sergio Paracuellos (2):
  clocksource: Add Ralink System Tick Counter driver
  MIPS: ralink: remove System Tick Counter driver

 arch/mips/ralink/Kconfig                              |  7 -------
 arch/mips/ralink/Makefile                             |  2 --
 drivers/clocksource/Kconfig                           | 10 ++++++++++
 drivers/clocksource/Makefile                          |  1 +
 .../clocksource/timer-ralink.c                        | 11 ++++-------
 5 files changed, 15 insertions(+), 16 deletions(-)
 rename arch/mips/ralink/cevt-rt3352.c => drivers/clocksource/timer-ralink.c (91%)

-- 
2.25.1

[PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

[Sergio Paracuellos]

System Tick Counter is present on Ralink SoCs RT3352 and MT7620. This
driver has been in 'arch/mips/ralink' directory since the beggining of
Ralink architecture support. However, it can be moved into a more proper
place in 'drivers/clocksource'. Hence add it here adding also support for
compile test targets.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/clocksource/Kconfig        |  10 ++
 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/timer-ralink.c | 150 +++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+)
 create mode 100644 drivers/clocksource/timer-ralink.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 95dd4660b5b6..50339f4d3201 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -753,4 +753,14 @@ config EP93XX_TIMER
 	  Enables support for the Cirrus Logic timer block
 	  EP93XX.
 
+config CLKSRC_RALINK
+	bool "Ralink System Tick Counter"
+	depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
+	default y if SOC_RT305X || SOC_MT7620
+	select CLKSRC_MMIO
+	select TIMER_OF
+	help
+	  Enables support for system tick counter present on
+	  Ralink SoCs RT3352 and MT7620.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 22743785299e..c9214afcb712 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -91,3 +91,4 @@ obj-$(CONFIG_GOLDFISH_TIMER)		+= timer-goldfish.o
 obj-$(CONFIG_GXP_TIMER)			+= timer-gxp.o
 obj-$(CONFIG_CLKSRC_LOONGSON1_PWM)	+= timer-loongson1-pwm.o
 obj-$(CONFIG_EP93XX_TIMER)		+= timer-ep93xx.o
+obj-$(CONFIG_CLKSRC_RALINK)		+= timer-ralink.o
diff --git a/drivers/clocksource/timer-ralink.c b/drivers/clocksource/timer-ralink.c
new file mode 100644
index 000000000000..6ecdb4228f76
--- /dev/null
+++ b/drivers/clocksource/timer-ralink.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Ralink System Tick Counter driver present on RT3352 and MT7620 SoCs.
+ *
+ * Copyright (C) 2013 by John Crispin <john@phrozen.org>
+ */
+
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/reset.h>
+#include <linux/init.h>
+#include <linux/time.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+
+#define SYSTICK_FREQ		(50 * 1000)
+
+#define SYSTICK_CONFIG		0x00
+#define SYSTICK_COMPARE		0x04
+#define SYSTICK_COUNT		0x08
+
+/* route systick irq to mips irq 7 instead of the r4k-timer */
+#define CFG_EXT_STK_EN		0x2
+/* enable the counter */
+#define CFG_CNT_EN		0x1
+
+struct systick_device {
+	void __iomem *membase;
+	struct clock_event_device dev;
+	int irq_requested;
+	int freq_scale;
+};
+
+static int systick_set_oneshot(struct clock_event_device *evt);
+static int systick_shutdown(struct clock_event_device *evt);
+
+static int systick_next_event(unsigned long delta,
+			      struct clock_event_device *evt)
+{
+	struct systick_device *sdev;
+	u32 count;
+
+	sdev = container_of(evt, struct systick_device, dev);
+	count = ioread32(sdev->membase + SYSTICK_COUNT);
+	count = (count + delta) % SYSTICK_FREQ;
+	iowrite32(count, sdev->membase + SYSTICK_COMPARE);
+
+	return 0;
+}
+
+static void systick_event_handler(struct clock_event_device *dev)
+{
+	/* noting to do here */
+}
+
+static irqreturn_t systick_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *dev = (struct clock_event_device *)dev_id;
+
+	dev->event_handler(dev);
+
+	return IRQ_HANDLED;
+}
+
+static struct systick_device systick = {
+	.dev = {
+		/*
+		 * cevt-r4k uses 300, make sure systick
+		 * gets used if available
+		 */
+		.rating			= 310,
+		.features		= CLOCK_EVT_FEAT_ONESHOT,
+		.set_next_event		= systick_next_event,
+		.set_state_shutdown	= systick_shutdown,
+		.set_state_oneshot	= systick_set_oneshot,
+		.event_handler		= systick_event_handler,
+	},
+};
+
+static int systick_shutdown(struct clock_event_device *evt)
+{
+	struct systick_device *sdev;
+
+	sdev = container_of(evt, struct systick_device, dev);
+
+	if (sdev->irq_requested)
+		free_irq(systick.dev.irq, &systick.dev);
+	sdev->irq_requested = 0;
+	iowrite32(0, systick.membase + SYSTICK_CONFIG);
+
+	return 0;
+}
+
+static int systick_set_oneshot(struct clock_event_device *evt)
+{
+	const char *name = systick.dev.name;
+	struct systick_device *sdev;
+	int irq = systick.dev.irq;
+
+	sdev = container_of(evt, struct systick_device, dev);
+
+	if (!sdev->irq_requested) {
+		if (request_irq(irq, systick_interrupt,
+				IRQF_PERCPU | IRQF_TIMER, name, &systick.dev))
+			pr_err("Failed to request irq %d (%s)\n", irq, name);
+	}
+	sdev->irq_requested = 1;
+	iowrite32(CFG_EXT_STK_EN | CFG_CNT_EN,
+		  systick.membase + SYSTICK_CONFIG);
+
+	return 0;
+}
+
+static int __init ralink_systick_init(struct device_node *np)
+{
+	int ret;
+
+	systick.membase = of_iomap(np, 0);
+	if (!systick.membase)
+		return -ENXIO;
+
+	systick.dev.name = np->name;
+	clockevents_calc_mult_shift(&systick.dev, SYSTICK_FREQ, 60);
+	systick.dev.max_delta_ns = clockevent_delta2ns(0x7fff, &systick.dev);
+	systick.dev.max_delta_ticks = 0x7fff;
+	systick.dev.min_delta_ns = clockevent_delta2ns(0x3, &systick.dev);
+	systick.dev.min_delta_ticks = 0x3;
+	systick.dev.irq = irq_of_parse_and_map(np, 0);
+	if (!systick.dev.irq) {
+		pr_err("%pOFn: request_irq failed", np);
+		return -EINVAL;
+	}
+
+	ret = clocksource_mmio_init(systick.membase + SYSTICK_COUNT, np->name,
+				    SYSTICK_FREQ, 301, 16,
+				    clocksource_mmio_readl_up);
+	if (ret)
+		return ret;
+
+	clockevents_register_device(&systick.dev);
+
+	pr_info("%pOFn: running - mult: %d, shift: %d\n",
+			np, systick.dev.mult, systick.dev.shift);
+
+	return 0;
+}
+
+TIMER_OF_DECLARE(systick, "ralink,cevt-systick", ralink_systick_init);
-- 
2.25.1

[PATCH 2/2] MIPS: ralink: remove System Tick Counter driver

[Sergio Paracuellos]

This driver has been moved into a more accurate place in the kernel tree
in 'drivers/clocksource' directory as 'timer-ralink.c'. Hence, remove it
from here so we can  reduce architecture code folder.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/ralink/Kconfig       |   7 --
 arch/mips/ralink/Makefile      |   2 -
 arch/mips/ralink/cevt-rt3352.c | 153 ---------------------------------
 3 files changed, 162 deletions(-)
 delete mode 100644 arch/mips/ralink/cevt-rt3352.c

diff --git a/arch/mips/ralink/Kconfig b/arch/mips/ralink/Kconfig
index 08c012a2591f..910d059ec70b 100644
--- a/arch/mips/ralink/Kconfig
+++ b/arch/mips/ralink/Kconfig
@@ -1,13 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 if RALINK
 
-config CLKEVT_RT3352
-	bool
-	depends on SOC_RT305X || SOC_MT7620
-	default y
-	select TIMER_OF
-	select CLKSRC_MMIO
-
 config RALINK_ILL_ACC
 	bool
 	depends on SOC_RT305X
diff --git a/arch/mips/ralink/Makefile b/arch/mips/ralink/Makefile
index 26fabbdea1f1..0c109eae1953 100644
--- a/arch/mips/ralink/Makefile
+++ b/arch/mips/ralink/Makefile
@@ -10,8 +10,6 @@ ifndef CONFIG_MIPS_GIC
 	obj-y += clk.o timer.o
 endif
 
-obj-$(CONFIG_CLKEVT_RT3352) += cevt-rt3352.o
-
 obj-$(CONFIG_RALINK_ILL_ACC) += ill_acc.o
 
 obj-$(CONFIG_IRQ_INTC) += irq.o
diff --git a/arch/mips/ralink/cevt-rt3352.c b/arch/mips/ralink/cevt-rt3352.c
deleted file mode 100644
index 269d4877d120..000000000000
--- a/arch/mips/ralink/cevt-rt3352.c
+++ /dev/null
@@ -1,153 +0,0 @@
-/*
- * This file is subject to the terms and conditions of the GNU General Public
- * License.  See the file "COPYING" in the main directory of this archive
- * for more details.
- *
- * Copyright (C) 2013 by John Crispin <john@phrozen.org>
- */
-
-#include <linux/clockchips.h>
-#include <linux/clocksource.h>
-#include <linux/interrupt.h>
-#include <linux/reset.h>
-#include <linux/init.h>
-#include <linux/time.h>
-#include <linux/of.h>
-#include <linux/of_irq.h>
-#include <linux/of_address.h>
-
-#include <asm/mach-ralink/ralink_regs.h>
-
-#define SYSTICK_FREQ		(50 * 1000)
-
-#define SYSTICK_CONFIG		0x00
-#define SYSTICK_COMPARE		0x04
-#define SYSTICK_COUNT		0x08
-
-/* route systick irq to mips irq 7 instead of the r4k-timer */
-#define CFG_EXT_STK_EN		0x2
-/* enable the counter */
-#define CFG_CNT_EN		0x1
-
-struct systick_device {
-	void __iomem *membase;
-	struct clock_event_device dev;
-	int irq_requested;
-	int freq_scale;
-};
-
-static int systick_set_oneshot(struct clock_event_device *evt);
-static int systick_shutdown(struct clock_event_device *evt);
-
-static int systick_next_event(unsigned long delta,
-				struct clock_event_device *evt)
-{
-	struct systick_device *sdev;
-	u32 count;
-
-	sdev = container_of(evt, struct systick_device, dev);
-	count = ioread32(sdev->membase + SYSTICK_COUNT);
-	count = (count + delta) % SYSTICK_FREQ;
-	iowrite32(count, sdev->membase + SYSTICK_COMPARE);
-
-	return 0;
-}
-
-static void systick_event_handler(struct clock_event_device *dev)
-{
-	/* noting to do here */
-}
-
-static irqreturn_t systick_interrupt(int irq, void *dev_id)
-{
-	struct clock_event_device *dev = (struct clock_event_device *) dev_id;
-
-	dev->event_handler(dev);
-
-	return IRQ_HANDLED;
-}
-
-static struct systick_device systick = {
-	.dev = {
-		/*
-		 * cevt-r4k uses 300, make sure systick
-		 * gets used if available
-		 */
-		.rating			= 310,
-		.features		= CLOCK_EVT_FEAT_ONESHOT,
-		.set_next_event		= systick_next_event,
-		.set_state_shutdown	= systick_shutdown,
-		.set_state_oneshot	= systick_set_oneshot,
-		.event_handler		= systick_event_handler,
-	},
-};
-
-static int systick_shutdown(struct clock_event_device *evt)
-{
-	struct systick_device *sdev;
-
-	sdev = container_of(evt, struct systick_device, dev);
-
-	if (sdev->irq_requested)
-		free_irq(systick.dev.irq, &systick.dev);
-	sdev->irq_requested = 0;
-	iowrite32(0, systick.membase + SYSTICK_CONFIG);
-
-	return 0;
-}
-
-static int systick_set_oneshot(struct clock_event_device *evt)
-{
-	const char *name = systick.dev.name;
-	struct systick_device *sdev;
-	int irq = systick.dev.irq;
-
-	sdev = container_of(evt, struct systick_device, dev);
-
-	if (!sdev->irq_requested) {
-		if (request_irq(irq, systick_interrupt,
-				IRQF_PERCPU | IRQF_TIMER, name, &systick.dev))
-			pr_err("Failed to request irq %d (%s)\n", irq, name);
-	}
-	sdev->irq_requested = 1;
-	iowrite32(CFG_EXT_STK_EN | CFG_CNT_EN,
-		  systick.membase + SYSTICK_CONFIG);
-
-	return 0;
-}
-
-static int __init ralink_systick_init(struct device_node *np)
-{
-	int ret;
-
-	systick.membase = of_iomap(np, 0);
-	if (!systick.membase)
-		return -ENXIO;
-
-	systick.dev.name = np->name;
-	clockevents_calc_mult_shift(&systick.dev, SYSTICK_FREQ, 60);
-	systick.dev.max_delta_ns = clockevent_delta2ns(0x7fff, &systick.dev);
-	systick.dev.max_delta_ticks = 0x7fff;
-	systick.dev.min_delta_ns = clockevent_delta2ns(0x3, &systick.dev);
-	systick.dev.min_delta_ticks = 0x3;
-	systick.dev.irq = irq_of_parse_and_map(np, 0);
-	if (!systick.dev.irq) {
-		pr_err("%pOFn: request_irq failed", np);
-		return -EINVAL;
-	}
-
-	ret = clocksource_mmio_init(systick.membase + SYSTICK_COUNT, np->name,
-				    SYSTICK_FREQ, 301, 16,
-				    clocksource_mmio_readl_up);
-	if (ret)
-		return ret;
-
-	clockevents_register_device(&systick.dev);
-
-	pr_info("%pOFn: running - mult: %d, shift: %d\n",
-			np, systick.dev.mult, systick.dev.shift);
-
-	return 0;
-}
-
-TIMER_OF_DECLARE(systick, "ralink,cevt-systick", ralink_systick_init);
-- 
2.25.1

Re: [PATCH 0/2] clocksource: move System Tick Counter from 'arch/mips/ralink'

[Sergio Paracuellos]

On Fri, Sep 20, 2024 at 9:53 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Daniel,
>
> System Tick Counter is present in RT3352 and MT7620 Ralink SoCs. This driver has
> been in 'arch/mips/ralink' from the beggining and can be easily moved into a more
> accurate place in 'drivers/clocksource' folder. This makes easier to enable it
> for compile test targets as well as reduce LOC in architecture specific folders.
> Bindings are already mainlined and can be located here [0].
>
> Thanks in advance for your time.
>
> Best regards,
>    Sergio Paracuellos
>
> [0]: https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/timer/ralink,cevt-systick.yaml
>
> Sergio Paracuellos (2):
>   clocksource: Add Ralink System Tick Counter driver
>   MIPS: ralink: remove System Tick Counter driver
>
>  arch/mips/ralink/Kconfig                              |  7 -------
>  arch/mips/ralink/Makefile                             |  2 --
>  drivers/clocksource/Kconfig                           | 10 ++++++++++
>  drivers/clocksource/Makefile                          |  1 +
>  .../clocksource/timer-ralink.c                        | 11 ++++-------
>  5 files changed, 15 insertions(+), 16 deletions(-)
>  rename arch/mips/ralink/cevt-rt3352.c => drivers/clocksource/timer-ralink.c (91%)

Gentle ping on this patch series :-)

Best regards,
    Sergio Paracuellos
>
> --
> 2.25.1
>

Re: [PATCH 0/2] clocksource: move System Tick Counter from 'arch/mips/ralink'

[Daniel Lezcano]

On 28/10/2024 15:14, Sergio Paracuellos wrote:
> On Fri, Sep 20, 2024 at 9:53 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
>>
>> Hi Daniel,
>>
>> System Tick Counter is present in RT3352 and MT7620 Ralink SoCs. This driver has
>> been in 'arch/mips/ralink' from the beggining and can be easily moved into a more
>> accurate place in 'drivers/clocksource' folder. This makes easier to enable it
>> for compile test targets as well as reduce LOC in architecture specific folders.
>> Bindings are already mainlined and can be located here [0].
>>
>> Thanks in advance for your time.
>>
>> Best regards,
>>     Sergio Paracuellos
>>
>> [0]: https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/timer/ralink,cevt-systick.yaml
>>
>> Sergio Paracuellos (2):
>>    clocksource: Add Ralink System Tick Counter driver
>>    MIPS: ralink: remove System Tick Counter driver
>>
>>   arch/mips/ralink/Kconfig                              |  7 -------
>>   arch/mips/ralink/Makefile                             |  2 --
>>   drivers/clocksource/Kconfig                           | 10 ++++++++++
>>   drivers/clocksource/Makefile                          |  1 +
>>   .../clocksource/timer-ralink.c                        | 11 ++++-------
>>   5 files changed, 15 insertions(+), 16 deletions(-)
>>   rename arch/mips/ralink/cevt-rt3352.c => drivers/clocksource/timer-ralink.c (91%)
> 
> Gentle ping on this patch series :-)

I'm on it


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/2] clocksource: move System Tick Counter from 'arch/mips/ralink'

[Sergio Paracuellos]

On Mon, Oct 28, 2024 at 3:16 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 28/10/2024 15:14, Sergio Paracuellos wrote:
> > On Fri, Sep 20, 2024 at 9:53 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> >>
> >> Hi Daniel,
> >>
> >> System Tick Counter is present in RT3352 and MT7620 Ralink SoCs. This driver has
> >> been in 'arch/mips/ralink' from the beggining and can be easily moved into a more
> >> accurate place in 'drivers/clocksource' folder. This makes easier to enable it
> >> for compile test targets as well as reduce LOC in architecture specific folders.
> >> Bindings are already mainlined and can be located here [0].
> >>
> >> Thanks in advance for your time.
> >>
> >> Best regards,
> >>     Sergio Paracuellos
> >>
> >> [0]: https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/timer/ralink,cevt-systick.yaml
> >>
> >> Sergio Paracuellos (2):
> >>    clocksource: Add Ralink System Tick Counter driver
> >>    MIPS: ralink: remove System Tick Counter driver
> >>
> >>   arch/mips/ralink/Kconfig                              |  7 -------
> >>   arch/mips/ralink/Makefile                             |  2 --
> >>   drivers/clocksource/Kconfig                           | 10 ++++++++++
> >>   drivers/clocksource/Makefile                          |  1 +
> >>   .../clocksource/timer-ralink.c                        | 11 ++++-------
> >>   5 files changed, 15 insertions(+), 16 deletions(-)
> >>   rename arch/mips/ralink/cevt-rt3352.c => drivers/clocksource/timer-ralink.c (91%)
> >
> > Gentle ping on this patch series :-)
>
> I'm on it

Awesome, thanks!

>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

[Daniel Lezcano]

On 20/09/2024 09:53, Sergio Paracuellos wrote:
> System Tick Counter is present on Ralink SoCs RT3352 and MT7620. This
> driver has been in 'arch/mips/ralink' directory since the beggining of
> Ralink architecture support. However, it can be moved into a more proper
> place in 'drivers/clocksource'. Hence add it here adding also support for
> compile test targets.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>   drivers/clocksource/Kconfig        |  10 ++
>   drivers/clocksource/Makefile       |   1 +
>   drivers/clocksource/timer-ralink.c | 150 +++++++++++++++++++++++++++++
>   3 files changed, 161 insertions(+)
>   create mode 100644 drivers/clocksource/timer-ralink.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 95dd4660b5b6..50339f4d3201 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -753,4 +753,14 @@ config EP93XX_TIMER
>   	  Enables support for the Cirrus Logic timer block
>   	  EP93XX.
>   
> +config CLKSRC_RALINK

It is a timer

	RALINK_TIMER

> +	bool "Ralink System Tick Counter"

Silent option please if possible.

Let the platform Kconfig selects the driver

> +	depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
> +	default y if SOC_RT305X || SOC_MT7620

You should have something similar the RISCV option, no default option

> +	select CLKSRC_MMIO
> +	select TIMER_OF
> +	help
> +	  Enables support for system tick counter present on
> +	  Ralink SoCs RT3352 and MT7620.
> +
>   endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 22743785299e..c9214afcb712 100644
> --- a/drivers/clocksource/Makefile

You should use git mv

Otherwise the code is like submitting a new driver



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

[Sergio Paracuellos]

Hi Daniel,

Thanks for reviewing this.

On Mon, Oct 28, 2024 at 5:29 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 20/09/2024 09:53, Sergio Paracuellos wrote:
> > System Tick Counter is present on Ralink SoCs RT3352 and MT7620. This
> > driver has been in 'arch/mips/ralink' directory since the beggining of
> > Ralink architecture support. However, it can be moved into a more proper
> > place in 'drivers/clocksource'. Hence add it here adding also support for
> > compile test targets.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >   drivers/clocksource/Kconfig        |  10 ++
> >   drivers/clocksource/Makefile       |   1 +
> >   drivers/clocksource/timer-ralink.c | 150 +++++++++++++++++++++++++++++
> >   3 files changed, 161 insertions(+)
> >   create mode 100644 drivers/clocksource/timer-ralink.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 95dd4660b5b6..50339f4d3201 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -753,4 +753,14 @@ config EP93XX_TIMER
> >         Enables support for the Cirrus Logic timer block
> >         EP93XX.
> >
> > +config CLKSRC_RALINK
>
> It is a timer
>
>         RALINK_TIMER

Sure, I will change to RALINK_TIMER instead.

>
> > +     bool "Ralink System Tick Counter"
>
> Silent option please if possible.
>
> Let the platform Kconfig selects the driver
>
> > +     depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
> > +     default y if SOC_RT305X || SOC_MT7620
>
> You should have something similar the RISCV option, no default option

Sorry, I am not the best with Kconfig so I am not sure what you are
exactly expecting here.
Does the following work for you?

config RALINK_TIMER
   bool "Ralink System Tick Counter" if COMPILE_TEST
   depends on SOC_RT305X || SOC_MT7620
   select CLKSRC_MMIO
   select TIMER_OF
   help
     Enables support for system tick counter present on
     Ralink SoCs RT3352 and MT7620.


>
> > +     select CLKSRC_MMIO
> > +     select TIMER_OF
> > +     help
> > +       Enables support for system tick counter present on
> > +       Ralink SoCs RT3352 and MT7620.
> > +
> >   endmenu
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index 22743785299e..c9214afcb712 100644
> > --- a/drivers/clocksource/Makefile
>
> You should use git mv
>
> Otherwise the code is like submitting a new driver

Ok, i will squash two patches in one performing the git mv then.

Thanks,
    Sergio Paracuellos
>
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

[Daniel Lezcano]

On 28/10/2024 19:04, Sergio Paracuellos wrote:
> Hi Daniel,
> 
> Thanks for reviewing this.
> 
> On Mon, Oct 28, 2024 at 5:29 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 20/09/2024 09:53, Sergio Paracuellos wrote:
>>> System Tick Counter is present on Ralink SoCs RT3352 and MT7620. This
>>> driver has been in 'arch/mips/ralink' directory since the beggining of
>>> Ralink architecture support. However, it can be moved into a more proper
>>> place in 'drivers/clocksource'. Hence add it here adding also support for
>>> compile test targets.
>>>
>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>> ---
>>>    drivers/clocksource/Kconfig        |  10 ++
>>>    drivers/clocksource/Makefile       |   1 +
>>>    drivers/clocksource/timer-ralink.c | 150 +++++++++++++++++++++++++++++
>>>    3 files changed, 161 insertions(+)
>>>    create mode 100644 drivers/clocksource/timer-ralink.c
>>>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index 95dd4660b5b6..50339f4d3201 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -753,4 +753,14 @@ config EP93XX_TIMER
>>>          Enables support for the Cirrus Logic timer block
>>>          EP93XX.
>>>
>>> +config CLKSRC_RALINK
>>
>> It is a timer
>>
>>          RALINK_TIMER
> 
> Sure, I will change to RALINK_TIMER instead.
> 
>>
>>> +     bool "Ralink System Tick Counter"
>>
>> Silent option please if possible.
>>
>> Let the platform Kconfig selects the driver
>>
>>> +     depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
>>> +     default y if SOC_RT305X || SOC_MT7620
>>
>> You should have something similar the RISCV option, no default option
> 
> Sorry, I am not the best with Kconfig so I am not sure what you are
> exactly expecting here.MT7620
> Does the following work for you?
> 
> config RALINK_TIMER
>     bool "Ralink System Tick Counter" if COMPILE_TEST
>     depends on SOC_RT305X || SOC_MT7620
>     select CLKSRC_MMIO
>     select TIMER_OF
>     help
>       Enables support for system tick counter present on
>       Ralink SoCs RT3352 and MT7620.

Basically the idea is to have the platform's Kconfig selecting the 
RALINK_TIMER. If I'm not wrong the Kconfig in arch/riscv/ralink should 
select RALINK_TIMER under the "config SOC_RT305X" and "config 
SOC_MT7620". The block "config CLKEVT_RT3352" has to be removed.

Then this (clocksource) Kconfig option is a silent option. The user 
won't have to figure out which option to enable because that will be 
done directly when selecting RT305X or MT7620.

The only reason to not have it silent is if you really want to opt-out 
this timer because it is not present on a different version of RT305X or 
MT7620.

IOW, this option should be:

config RALINK_TIMER
      bool "Ralink System Tick Counter" if COMPILE_TEST
      select CLKSRC_MMIO
      select TIMER_OF
      help
        Enables support for system tick counter present on
        Ralink SoCs RT3352 and MT7620.

The option COMPILE_TEST is to compile on different platforms, thus 
increasing the compilation test coverage. At the first glance, the 
driver does not seem to pull arch dependent code except definitions 
which look compatible with other archs, so it should be fine.


>>> +     select CLKSRC_MMIO
>>> +     select TIMER_OF
>>> +     help
>>> +       Enables support for system tick counter present on
>>> +       Ralink SoCs RT3352 and MT7620.
>>> +
>>>    endmenu
>>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>>> index 22743785299e..c9214afcb712 100644
>>> --- a/drivers/clocksource/Makefile
>>
>> You should use git mv
>>
>> Otherwise the code is like submitting a new driver
> 
> Ok, i will squash two patches in one performing the git mv then.
> 
> Thanks,
>      Sergio Paracuellos
>>
>>
>>
>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

[Sergio Paracuellos]

Hi Daniel,

Thanks a lot for the detailed explanation. It was really helpful.

On Mon, Oct 28, 2024 at 7:44 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 28/10/2024 19:04, Sergio Paracuellos wrote:
> > Hi Daniel,
> >
> > Thanks for reviewing this.
> >
> > On Mon, Oct 28, 2024 at 5:29 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 20/09/2024 09:53, Sergio Paracuellos wrote:
> >>> System Tick Counter is present on Ralink SoCs RT3352 and MT7620. This
> >>> driver has been in 'arch/mips/ralink' directory since the beggining of
> >>> Ralink architecture support. However, it can be moved into a more proper
> >>> place in 'drivers/clocksource'. Hence add it here adding also support for
> >>> compile test targets.
> >>>
> >>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >>> ---
> >>>    drivers/clocksource/Kconfig        |  10 ++
> >>>    drivers/clocksource/Makefile       |   1 +
> >>>    drivers/clocksource/timer-ralink.c | 150 +++++++++++++++++++++++++++++
> >>>    3 files changed, 161 insertions(+)
> >>>    create mode 100644 drivers/clocksource/timer-ralink.c
> >>>
> >>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> >>> index 95dd4660b5b6..50339f4d3201 100644
> >>> --- a/drivers/clocksource/Kconfig
> >>> +++ b/drivers/clocksource/Kconfig
> >>> @@ -753,4 +753,14 @@ config EP93XX_TIMER
> >>>          Enables support for the Cirrus Logic timer block
> >>>          EP93XX.
> >>>
> >>> +config CLKSRC_RALINK
> >>
> >> It is a timer
> >>
> >>          RALINK_TIMER
> >
> > Sure, I will change to RALINK_TIMER instead.
> >
> >>
> >>> +     bool "Ralink System Tick Counter"
> >>
> >> Silent option please if possible.
> >>
> >> Let the platform Kconfig selects the driver
> >>
> >>> +     depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
> >>> +     default y if SOC_RT305X || SOC_MT7620
> >>
> >> You should have something similar the RISCV option, no default option
> >
> > Sorry, I am not the best with Kconfig so I am not sure what you are
> > exactly expecting here.MT7620
> > Does the following work for you?
> >
> > config RALINK_TIMER
> >     bool "Ralink System Tick Counter" if COMPILE_TEST
> >     depends on SOC_RT305X || SOC_MT7620
> >     select CLKSRC_MMIO
> >     select TIMER_OF
> >     help
> >       Enables support for system tick counter present on
> >       Ralink SoCs RT3352 and MT7620.
>
> Basically the idea is to have the platform's Kconfig selecting the
> RALINK_TIMER. If I'm not wrong the Kconfig in arch/riscv/ralink should
> select RALINK_TIMER under the "config SOC_RT305X" and "config
> SOC_MT7620". The block "config CLKEVT_RT3352" has to be removed.
>
> Then this (clocksource) Kconfig option is a silent option. The user
> won't have to figure out which option to enable because that will be
> done directly when selecting RT305X or MT7620.
>
> The only reason to not have it silent is if you really want to opt-out
> this timer because it is not present on a different version of RT305X or
> MT7620.

Ok, then I don't want to silence it since those ralink's platform
SOC_RT305X and SOC_MT7620 includes other SoCs models that do not have
this timer (rt3050, mt7628 for example). Only models
rt3352 and mt7620 include this. So I guess having this is the correct
thing to do:

config RALINK_TIMER
    bool "Ralink System Tick Counter" if COMPILE_TEST
    depends on SOC_RT305X || SOC_MT7620
    select CLKSRC_MMIO
    select TIMER_OF
    help
       Enables support for system tick counter present on
       Ralink SoCs RT3352 and MT7620.

Are you ok with this?

>
> IOW, this option should be:
>
> config RALINK_TIMER
>       bool "Ralink System Tick Counter" if COMPILE_TEST
>       select CLKSRC_MMIO
>       select TIMER_OF
>       help
>         Enables support for system tick counter present on
>         Ralink SoCs RT3352 and MT7620.
>
> The option COMPILE_TEST is to compile on different platforms, thus
> increasing the compilation test coverage. At the first glance, the
> driver does not seem to pull arch dependent code except definitions
> which look compatible with other archs, so it should be fine.

Yes, there is no arch dependencies since the only include which was
dependent was not really needed and I already got rid of it when I
performed the git mv. I already checked
that the driver is properly compiled for allyesconfig target.

Thanks,
    Sergio Paracuellos

>
>
> >>> +     select CLKSRC_MMIO
> >>> +     select TIMER_OF
> >>> +     help
> >>> +       Enables support for system tick counter present on
> >>> +       Ralink SoCs RT3352 and MT7620.
> >>> +
> >>>    endmenu
> >>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> >>> index 22743785299e..c9214afcb712 100644
> >>> --- a/drivers/clocksource/Makefile
> >>
> >> You should use git mv
> >>
> >> Otherwise the code is like submitting a new driver
> >
> > Ok, i will squash two patches in one performing the git mv then.
> >
> > Thanks,
> >      Sergio Paracuellos
> >>
> >>
> >>
> >> --
> >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> >>
> >> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> >> <http://twitter.com/#!/linaroorg> Twitter |
> >> <http://www.linaro.org/linaro-blog/> Blog
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

[Daniel Lezcano]

On 28/10/2024 20:02, Sergio Paracuellos wrote:
> Hi Daniel,
> 
> Thanks a lot for the detailed explanation. It was really helpful.
> 
> On Mon, Oct 28, 2024 at 7:44 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 28/10/2024 19:04, Sergio Paracuellos wrote:
>>> Hi Daniel,
>>>
>>> Thanks for reviewing this.
>>>
>>> On Mon, Oct 28, 2024 at 5:29 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 20/09/2024 09:53, Sergio Paracuellos wrote:
>>>>> System Tick Counter is present on Ralink SoCs RT3352 and MT7620. This
>>>>> driver has been in 'arch/mips/ralink' directory since the beggining of
>>>>> Ralink architecture support. However, it can be moved into a more proper
>>>>> place in 'drivers/clocksource'. Hence add it here adding also support for
>>>>> compile test targets.
>>>>>
>>>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>>>> ---
>>>>>     drivers/clocksource/Kconfig        |  10 ++
>>>>>     drivers/clocksource/Makefile       |   1 +
>>>>>     drivers/clocksource/timer-ralink.c | 150 +++++++++++++++++++++++++++++
>>>>>     3 files changed, 161 insertions(+)
>>>>>     create mode 100644 drivers/clocksource/timer-ralink.c
>>>>>
>>>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>>>> index 95dd4660b5b6..50339f4d3201 100644
>>>>> --- a/drivers/clocksource/Kconfig
>>>>> +++ b/drivers/clocksource/Kconfig
>>>>> @@ -753,4 +753,14 @@ config EP93XX_TIMER
>>>>>           Enables support for the Cirrus Logic timer block
>>>>>           EP93XX.
>>>>>
>>>>> +config CLKSRC_RALINK
>>>>
>>>> It is a timer
>>>>
>>>>           RALINK_TIMER
>>>
>>> Sure, I will change to RALINK_TIMER instead.
>>>
>>>>
>>>>> +     bool "Ralink System Tick Counter"
>>>>
>>>> Silent option please if possible.
>>>>
>>>> Let the platform Kconfig selects the driver
>>>>
>>>>> +     depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
>>>>> +     default y if SOC_RT305X || SOC_MT7620
>>>>
>>>> You should have something similar the RISCV option, no default option
>>>
>>> Sorry, I am not the best with Kconfig so I am not sure what you are
>>> exactly expecting here.MT7620
>>> Does the following work for you?
>>>
>>> config RALINK_TIMER
>>>      bool "Ralink System Tick Counter" if COMPILE_TEST
>>>      depends on SOC_RT305X || SOC_MT7620
>>>      select CLKSRC_MMIO
>>>      select TIMER_OF
>>>      help
>>>        Enables support for system tick counter present on
>>>        Ralink SoCs RT3352 and MT7620.
>>
>> Basically the idea is to have the platform's Kconfig selecting the
>> RALINK_TIMER. If I'm not wrong the Kconfig in arch/riscv/ralink should
>> select RALINK_TIMER under the "config SOC_RT305X" and "config
>> SOC_MT7620". The block "config CLKEVT_RT3352" has to be removed.
>>
>> Then this (clocksource) Kconfig option is a silent option. The user
>> won't have to figure out which option to enable because that will be
>> done directly when selecting RT305X or MT7620.
>>
>> The only reason to not have it silent is if you really want to opt-out
>> this timer because it is not present on a different version of RT305X or
>> MT7620.
> 
> Ok, then I don't want to silence it since those ralink's platform
> SOC_RT305X and SOC_MT7620 includes other SoCs models that do not have
> this timer (rt3050, mt7628 for example). Only models
> rt3352 and mt7620 include this. So I guess having this is the correct
> thing to do:
> 
> config RALINK_TIMER
>      bool "Ralink System Tick Counter" if COMPILE_TEST
>      depends on SOC_RT305X || SOC_MT7620
>      select CLKSRC_MMIO
>      select TIMER_OF
>      help
>         Enables support for system tick counter present on
>         Ralink SoCs RT3352 and MT7620.
> 
> Are you ok with this?


No because it is a compile test option only. You may want to do the same as:

https://lore.kernel.org/all/20241001-arm64-vexpress-sp804-v3-1-0a2d3f7883e4@kernel.org/

It should be:

config RALINK_TIMER
	bool "Ralink System Tick Counter"
	depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
	select CLKSRC_MMIO
	select TIMER_OF
	help
	  Enables support for system tick counter present on
	  Ralink SoCs RT3352 and MT7620.

so very similar to the initial patch except no default is needed as it 
should be set from SOC_RT305X or SOC_MT7620


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog