Message ID | 20240627043317.3751996-8-chris.packham@alliedtelesis.co.nz (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | mips: Support for RTL9302C | expand |
On Thu, 27 Jun 2024 16:33:15 +1200 Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > +/* Simple internal register functions */ > +static inline void rttm_set_counter(void __iomem *base, unsigned int counter) > +{ > + iowrite32(counter, base + RTTM_CNT); These require #include <asm/io.h> > +/* Aggregated control functions for kernel clock framework */ > +#define RTTM_DEBUG(base) \ > + pr_debug("------------- %d %p\n", \ > + smp_processor_id(), base) #include <linux/printk.h> > +static irqreturn_t rttm_timer_interrupt(int irq, void *dev_id) > +{ > + struct clock_event_device *clkevt = dev_id; > + struct timer_of *to = to_timer_of(clkevt); > + > + rttm_ack_irq(to->of_base.base); > + RTTM_DEBUG(to->of_base.base); > + clkevt->event_handler(clkevt); Although you include "timer-of.h", which includes clockchips.h, please do also explicit #include <linux/clockchips.h> > + rttm_set_period(to->of_base.base, RTTM_TICKS_PER_SEC / HZ); HZ -> linux/jiffies.h, or maybe asm/param.h > +static u64 rttm_read_clocksource(struct clocksource *cs) > +{ > + struct rttm_cs *rcs = container_of(cs, struct rttm_cs, cs); > + > + return (u64)rttm_get_counter(rcs->to.of_base.base); Redundant cast to u64. > + rttm_enable_timer(rcs->to.of_base.base, RTTM_CTRL_TIMER, > + rcs->to.of_clk.rate / RTTM_TICKS_PER_SEC); Is this correct? Sometimes it makes sense to use DIV_ROUND_CLOSEST, but maybe not here. > +static u64 notrace rttm_read_clock(void) > +{ > + return (u64)rttm_get_counter(rttm_cs.to.of_base.base); Redundant cast to u64. > +static int __init rttm_probe(struct device_node *np) > +{ > + int cpu, cpu_rollback; unsigned int? > + struct timer_of *to; > + int clkidx = num_possible_cpus(); linux/cpumask.h, unsigned int Marek
Hi Chris, On Thu, 2024-06-27 at 16:33 +1200, Chris Packham wrote: > The timer/counter block on the Realtek SoCs provides up to 5 timers. It > also includes a watchdog timer but this isn't being used currently (it > will be added as a separate wdt driver). Do you mean the watchdog timer supported by drivers/watchdog/realtek_otto_wdt.c? Or are you referring to another watchdog timer? > One timer will be used per CPU as a local clock event generator. An > additional timer will be used as an overal stable clocksource. > > Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de> > Signed-off-by: Sander Vanheule <sander@svanheule.net> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- For reference, I submitted a driver for the same hardware back in 2022, but didn't manage to follow up to finalize the submission: https://lore.kernel.org/all/cover.1642369117.git.sander@svanheule.net/ > + > +/* Module initialization part. */ > +static DEFINE_PER_CPU(struct timer_of, rttm_to) = { > + .flags = TIMER_OF_BASE | TIMER_OF_CLOCK | > TIMER_OF_IRQ, > + .of_irq = { > + .flags = IRQF_PERCPU | IRQF_TIMER, In the original review of this code, I had some doubts about the use of IRQF_PERCPU. Maybe the people in Cc can shed some light on this. If I understand correctly, the SoC interrupts these timers use are not per-cpu interrupts. (For comparison, AFAICT the MIPS CPU interrupts are) > + .handler = rttm_timer_interrupt, > + }, > + .clkevt = { > + .rating = 400, > + .features = CLOCK_EVT_FEAT_PERIODIC | > CLOCK_EVT_FEAT_ONESHOT, If the use of IRQF_PERCPU is appropriate, I wonder if the driver should also use CLOCK_EVT_FEAT_PERCPU. Best, Sander
Hi Chris,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on tip/timers/core tip/irq/core tip/smp/core linus/master v6.10-rc5 next-20240628]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Chris-Packham/mips-dts-realtek-use-serial-instead-of-uart-in-node-name/20240627-202806
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240627043317.3751996-8-chris.packham%40alliedtelesis.co.nz
patch subject: [PATCH v3 7/9] clocksource: realtek: Add timer driver for rtl-otto platforms
config: s390-randconfig-r133-20240629
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce:
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406300803.mMM0Hpxv-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/io.h:14,
from include/linux/of_address.h:7,
from drivers/clocksource/timer-of.c:10:
drivers/clocksource/timer-of.c: In function 'timer_of_base_exit':
>> arch/s390/include/asm/io.h:29:17: error: implicit declaration of function 'iounmap'; did you mean 'iommu_map'? [-Werror=implicit-function-declaration]
29 | #define iounmap iounmap
| ^~~~~~~
drivers/clocksource/timer-of.c:151:9: note: in expansion of macro 'iounmap'
151 | iounmap(of_base->base);
| ^~~~~~~
cc1: some warnings being treated as errors
vim +29 arch/s390/include/asm/io.h
cd24834130ac65 Jan Glauber 2012-11-29 24
b43b3fff042d08 Baoquan He 2023-07-06 25 /*
b43b3fff042d08 Baoquan He 2023-07-06 26 * I/O memory mapping functions.
b43b3fff042d08 Baoquan He 2023-07-06 27 */
b43b3fff042d08 Baoquan He 2023-07-06 28 #define ioremap_prot ioremap_prot
b43b3fff042d08 Baoquan He 2023-07-06 @29 #define iounmap iounmap
b43b3fff042d08 Baoquan He 2023-07-06 30
On 30/06/24 09:03, Sander Vanheule wrote: > Hi Chris, > > On Thu, 2024-06-27 at 16:33 +1200, Chris Packham wrote: >> The timer/counter block on the Realtek SoCs provides up to 5 timers. It >> also includes a watchdog timer but this isn't being used currently (it >> will be added as a separate wdt driver). > Do you mean the watchdog timer supported by drivers/watchdog/realtek_otto_wdt.c? Or are > you referring to another watchdog timer? Yes realtek_otto_wdt.c. I thought that was only on openwrt but looks like you've already landed that one. I'll reword my message to reflect that. >> One timer will be used per CPU as a local clock event generator. An >> additional timer will be used as an overal stable clocksource. >> >> Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de> >> Signed-off-by: Sander Vanheule <sander@svanheule.net> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- > For reference, I submitted a driver for the same hardware back in 2022, but didn't manage > to follow up to finalize the submission: > > https://lore.kernel.org/all/cover.1642369117.git.sander@svanheule.net/ OK thanks for the link. I'll try and make sure I'm not rehashing things that have already been discussed. >> + >> +/* Module initialization part. */ >> +static DEFINE_PER_CPU(struct timer_of, rttm_to) = { >> + .flags = TIMER_OF_BASE | TIMER_OF_CLOCK | >> TIMER_OF_IRQ, >> + .of_irq = { >> + .flags = IRQF_PERCPU | IRQF_TIMER, > In the original review of this code, I had some doubts about the use of IRQF_PERCPU. Maybe > the people in Cc can shed some light on this. > > If I understand correctly, the SoC interrupts these timers use are not per-cpu interrupts. > (For comparison, AFAICT the MIPS CPU interrupts are) I'm not exactly sure either and I've only got the single core RTL9302 to test with so if there were some difference I may not notice. Eventually we're planning a RTL931x based board so that may be something I can check then. >> + .handler = rttm_timer_interrupt, >> + }, >> + .clkevt = { >> + .rating = 400, >> + .features = CLOCK_EVT_FEAT_PERIODIC | >> CLOCK_EVT_FEAT_ONESHOT, > If the use of IRQF_PERCPU is appropriate, I wonder if the driver should also use > CLOCK_EVT_FEAT_PERCPU. > > > Best, > Sander >
On 27/06/24 23:17, Marek BehĂșn wrote: > On Thu, 27 Jun 2024 16:33:15 +1200 > Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > >> +/* Simple internal register functions */ >> +static inline void rttm_set_counter(void __iomem *base, unsigned int counter) >> +{ >> + iowrite32(counter, base + RTTM_CNT); > These require #include <asm/io.h> linux/io.h I'm guessing. >> +/* Aggregated control functions for kernel clock framework */ >> +#define RTTM_DEBUG(base) \ >> + pr_debug("------------- %d %p\n", \ >> + smp_processor_id(), base) > #include <linux/printk.h> ack >> +static irqreturn_t rttm_timer_interrupt(int irq, void *dev_id) >> +{ >> + struct clock_event_device *clkevt = dev_id; >> + struct timer_of *to = to_timer_of(clkevt); >> + >> + rttm_ack_irq(to->of_base.base); >> + RTTM_DEBUG(to->of_base.base); >> + clkevt->event_handler(clkevt); > Although you include "timer-of.h", which includes clockchips.h, please > do also explicit #include <linux/clockchips.h> > >> + rttm_set_period(to->of_base.base, RTTM_TICKS_PER_SEC / HZ); > HZ -> linux/jiffies.h, or maybe asm/param.h ack >> +static u64 rttm_read_clocksource(struct clocksource *cs) >> +{ >> + struct rttm_cs *rcs = container_of(cs, struct rttm_cs, cs); >> + >> + return (u64)rttm_get_counter(rcs->to.of_base.base); > Redundant cast to u64. ack >> + rttm_enable_timer(rcs->to.of_base.base, RTTM_CTRL_TIMER, >> + rcs->to.of_clk.rate / RTTM_TICKS_PER_SEC); > Is this correct? Sometimes it makes sense to use DIV_ROUND_CLOSEST, but > maybe not here. It's OK for me because the Lexra bus clock is 175mhz so plain division works fine. The docs do say something about a configurable divisor but the range seems fairly limited so I don't think there's a specific need to use DIV_ROUND_CLOSEST. > >> +static u64 notrace rttm_read_clock(void) >> +{ >> + return (u64)rttm_get_counter(rttm_cs.to.of_base.base); > Redundant cast to u64. ack >> +static int __init rttm_probe(struct device_node *np) >> +{ >> + int cpu, cpu_rollback; > unsigned int? ack >> + struct timer_of *to; >> + int clkidx = num_possible_cpus(); > linux/cpumask.h, unsigned int ack > Marek
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index 34faa0320ece..70ba57210862 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -134,6 +134,16 @@ config RDA_TIMER help Enables the support for the RDA Micro timer driver. +config REALTEK_OTTO_TIMER + bool "Clocksource/timer for the Realtek Otto platform" + select TIMER_OF + help + This driver adds support for the timers found in the Realtek RTL83xx + and RTL93xx SoCs series. This includes chips such as RTL8380, RTL8381 + and RTL832, as well as chips from the RTL839x series, such as RTL8390 + RT8391, RTL8392, RTL8393 and RTL8396 and chips of the RTL930x series + such as RTL9301, RTL9302 or RTL9303. + config SUN4I_TIMER bool "Sun4i timer driver" if COMPILE_TEST depends on HAS_IOMEM diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index 4bb856e4df55..22743785299e 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -59,6 +59,7 @@ obj-$(CONFIG_MILBEAUT_TIMER) += timer-milbeaut.o obj-$(CONFIG_SPRD_TIMER) += timer-sprd.o obj-$(CONFIG_NPCM7XX_TIMER) += timer-npcm7xx.o obj-$(CONFIG_RDA_TIMER) += timer-rda.o +obj-$(CONFIG_REALTEK_OTTO_TIMER) += timer-rtl-otto.o obj-$(CONFIG_ARC_TIMERS) += arc_timer.o obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o diff --git a/drivers/clocksource/timer-rtl-otto.c b/drivers/clocksource/timer-rtl-otto.c new file mode 100644 index 000000000000..f1b9d35eacd1 --- /dev/null +++ b/drivers/clocksource/timer-rtl-otto.c @@ -0,0 +1,287 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/clk.h> +#include <linux/clockchips.h> +#include <linux/cpu.h> +#include <linux/cpuhotplug.h> +#include <linux/interrupt.h> +#include <linux/sched_clock.h> +#include "timer-of.h" + +#define RTTM_DATA 0x0 +#define RTTM_CNT 0x4 +#define RTTM_CTRL 0x8 +#define RTTM_INT 0xc + +#define RTTM_CTRL_ENABLE BIT(28) +#define RTTM_INT_PENDING BIT(16) +#define RTTM_INT_ENABLE BIT(20) + +/* + * The Otto platform provides multiple 28 bit timers/counters with the following + * operating logic. If enabled the timer counts up. Per timer one can set a + * maximum counter value as an end marker. If end marker is reached the timer + * fires an interrupt. If the timer "overflows" by reaching the end marker or + * by adding 1 to 0x0fffffff the counter is reset to 0. When this happens and + * the timer is in operating mode COUNTER it stops. In mode TIMER it will + * continue to count up. + */ +#define RTTM_CTRL_COUNTER 0 +#define RTTM_CTRL_TIMER BIT(24) + +#define RTTM_BIT_COUNT 28 +#define RTTM_MIN_DELTA 8 +#define RTTM_MAX_DELTA CLOCKSOURCE_MASK(28) + +/* + * Timers are derived from the LXB clock frequency. Usually this is a fixed + * multiple of the 25 MHz oscillator. The 930X SOC is an exception from that. + * Its LXB clock has only dividers and uses the switch PLL of 2.45 GHz as its + * base. The only meaningful frequencies we can achieve from that are 175.000 + * MHz and 153.125 MHz. The greatest common divisor of all explained possible + * speeds is 3125000. Pin the timers to this 3.125 MHz reference frequency. + */ +#define RTTM_TICKS_PER_SEC 3125000 + +struct rttm_cs { + struct timer_of to; + struct clocksource cs; +}; + +/* Simple internal register functions */ +static inline void rttm_set_counter(void __iomem *base, unsigned int counter) +{ + iowrite32(counter, base + RTTM_CNT); +} + +static inline unsigned int rttm_get_counter(void __iomem *base) +{ + return ioread32(base + RTTM_CNT); +} + +static inline void rttm_set_period(void __iomem *base, unsigned int period) +{ + iowrite32(period, base + RTTM_DATA); +} + +static inline void rttm_disable_timer(void __iomem *base) +{ + iowrite32(0, base + RTTM_CTRL); +} + +static inline void rttm_enable_timer(void __iomem *base, u32 mode, u32 divisor) +{ + iowrite32(RTTM_CTRL_ENABLE | mode | divisor, base + RTTM_CTRL); +} + +static inline void rttm_ack_irq(void __iomem *base) +{ + iowrite32(ioread32(base + RTTM_INT) | RTTM_INT_PENDING, base + RTTM_INT); +} + +static inline void rttm_enable_irq(void __iomem *base) +{ + iowrite32(RTTM_INT_ENABLE, base + RTTM_INT); +} + +static inline void rttm_disable_irq(void __iomem *base) +{ + iowrite32(0, base + RTTM_INT); +} + +/* Aggregated control functions for kernel clock framework */ +#define RTTM_DEBUG(base) \ + pr_debug("------------- %d %p\n", \ + smp_processor_id(), base) + +static irqreturn_t rttm_timer_interrupt(int irq, void *dev_id) +{ + struct clock_event_device *clkevt = dev_id; + struct timer_of *to = to_timer_of(clkevt); + + rttm_ack_irq(to->of_base.base); + RTTM_DEBUG(to->of_base.base); + clkevt->event_handler(clkevt); + + return IRQ_HANDLED; +} + +static void rttm_stop_timer(void __iomem *base) +{ + rttm_disable_timer(base); + rttm_ack_irq(base); +} + +static void rttm_start_timer(struct timer_of *to, u32 mode) +{ + rttm_set_counter(to->of_base.base, 0); + rttm_enable_timer(to->of_base.base, mode, to->of_clk.rate / RTTM_TICKS_PER_SEC); +} + +static int rttm_next_event(unsigned long delta, struct clock_event_device *clkevt) +{ + struct timer_of *to = to_timer_of(clkevt); + + RTTM_DEBUG(to->of_base.base); + rttm_stop_timer(to->of_base.base); + rttm_set_period(to->of_base.base, delta); + rttm_start_timer(to, RTTM_CTRL_COUNTER); + + return 0; +} + +static int rttm_state_oneshot(struct clock_event_device *clkevt) +{ + struct timer_of *to = to_timer_of(clkevt); + + RTTM_DEBUG(to->of_base.base); + rttm_stop_timer(to->of_base.base); + rttm_set_period(to->of_base.base, RTTM_TICKS_PER_SEC / HZ); + rttm_start_timer(to, RTTM_CTRL_COUNTER); + + return 0; +} + +static int rttm_state_periodic(struct clock_event_device *clkevt) +{ + struct timer_of *to = to_timer_of(clkevt); + + RTTM_DEBUG(to->of_base.base); + rttm_stop_timer(to->of_base.base); + rttm_set_period(to->of_base.base, RTTM_TICKS_PER_SEC / HZ); + rttm_start_timer(to, RTTM_CTRL_TIMER); + + return 0; +} + +static int rttm_state_shutdown(struct clock_event_device *clkevt) +{ + struct timer_of *to = to_timer_of(clkevt); + + RTTM_DEBUG(to->of_base.base); + rttm_stop_timer(to->of_base.base); + + return 0; +} + +static void rttm_setup_timer(void __iomem *base) +{ + RTTM_DEBUG(base); + rttm_stop_timer(base); + rttm_set_period(base, 0); +} + +static u64 rttm_read_clocksource(struct clocksource *cs) +{ + struct rttm_cs *rcs = container_of(cs, struct rttm_cs, cs); + + return (u64)rttm_get_counter(rcs->to.of_base.base); +} + +/* Module initialization part. */ +static DEFINE_PER_CPU(struct timer_of, rttm_to) = { + .flags = TIMER_OF_BASE | TIMER_OF_CLOCK | TIMER_OF_IRQ, + .of_irq = { + .flags = IRQF_PERCPU | IRQF_TIMER, + .handler = rttm_timer_interrupt, + }, + .clkevt = { + .rating = 400, + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, + .set_state_periodic = rttm_state_periodic, + .set_state_shutdown = rttm_state_shutdown, + .set_state_oneshot = rttm_state_oneshot, + .set_next_event = rttm_next_event + }, +}; + +static int rttm_enable_clocksource(struct clocksource *cs) +{ + struct rttm_cs *rcs = container_of(cs, struct rttm_cs, cs); + + rttm_disable_irq(rcs->to.of_base.base); + rttm_setup_timer(rcs->to.of_base.base); + rttm_enable_timer(rcs->to.of_base.base, RTTM_CTRL_TIMER, + rcs->to.of_clk.rate / RTTM_TICKS_PER_SEC); + + return 0; +} + +struct rttm_cs rttm_cs = { + .to = { + .flags = TIMER_OF_BASE | TIMER_OF_CLOCK, + }, + .cs = { + .name = "realtek_otto_timer", + .rating = 400, + .mask = CLOCKSOURCE_MASK(RTTM_BIT_COUNT), + .flags = CLOCK_SOURCE_IS_CONTINUOUS, + .read = rttm_read_clocksource, + } +}; + +static u64 notrace rttm_read_clock(void) +{ + return (u64)rttm_get_counter(rttm_cs.to.of_base.base); +} + +static int rttm_cpu_starting(unsigned int cpu) +{ + struct timer_of *to = per_cpu_ptr(&rttm_to, cpu); + + RTTM_DEBUG(to->of_base.base); + to->clkevt.cpumask = cpumask_of(cpu); + irq_force_affinity(to->of_irq.irq, to->clkevt.cpumask); + clockevents_config_and_register(&to->clkevt, RTTM_TICKS_PER_SEC, + RTTM_MIN_DELTA, RTTM_MAX_DELTA); + rttm_enable_irq(to->of_base.base); + + return 0; +} + +static int __init rttm_probe(struct device_node *np) +{ + int cpu, cpu_rollback; + struct timer_of *to; + int clkidx = num_possible_cpus(); + + /* Use the first n timers as per CPU clock event generators */ + for_each_possible_cpu(cpu) { + to = per_cpu_ptr(&rttm_to, cpu); + to->of_irq.index = to->of_base.index = cpu; + if (timer_of_init(np, to)) { + pr_err("setup of timer %d failed\n", cpu); + goto rollback; + } + rttm_setup_timer(to->of_base.base); + } + + /* Activate the n'th + 1 timer as a stable CPU clocksource. */ + to = &rttm_cs.to; + to->of_base.index = clkidx; + timer_of_init(np, to); + if (rttm_cs.to.of_base.base && rttm_cs.to.of_clk.rate) { + rttm_enable_clocksource(&rttm_cs.cs); + clocksource_register_hz(&rttm_cs.cs, RTTM_TICKS_PER_SEC); + sched_clock_register(rttm_read_clock, RTTM_BIT_COUNT, RTTM_TICKS_PER_SEC); + } else + pr_err(" setup of timer %d as clocksource failed", clkidx); + + return cpuhp_setup_state(CPUHP_AP_REALTEK_TIMER_STARTING, + "timer/realtek:online", + rttm_cpu_starting, NULL); +rollback: + pr_err("timer registration failed\n"); + for_each_possible_cpu(cpu_rollback) { + if (cpu_rollback == cpu) + break; + to = per_cpu_ptr(&rttm_to, cpu_rollback); + timer_of_cleanup(to); + } + + return -EINVAL; +} + +TIMER_OF_DECLARE(otto_timer, "realtek,otto-timer", rttm_probe); diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h index 7a5785f405b6..56b744dc1317 100644 --- a/include/linux/cpuhotplug.h +++ b/include/linux/cpuhotplug.h @@ -171,6 +171,7 @@ enum cpuhp_state { CPUHP_AP_ARMADA_TIMER_STARTING, CPUHP_AP_MIPS_GIC_TIMER_STARTING, CPUHP_AP_ARC_TIMER_STARTING, + CPUHP_AP_REALTEK_TIMER_STARTING, CPUHP_AP_RISCV_TIMER_STARTING, CPUHP_AP_CLINT_TIMER_STARTING, CPUHP_AP_CSKY_TIMER_STARTING,