| Message ID | 20190224093635.1242-2-kernel@xen0n.name (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | MIPS: Loongson: ExtCC clocksource support | expand |
Hi Wang, Thanks for the patch - comments inline below. On Sun, Feb 24, 2019 at 05:36:34PM +0800, kernel@xen0n.name wrote: > From: Wang Xuerui <wangxuerui@qiniu.com> > > The ExtCC (external counter) is a Loongson GS464E-specific hardware > register that ticks every internal bus cycle. Hence, the counter is > shared among cores of the same package and can be considered constant > frequency. The frequency is same as maximum frequency on all Loongson > chips I have access to, and the resolution is very good. > > A previous iteration of the feature directly ripped x86 arch code, thus > unsuitable for mainlining. This time though it's implemented with the > generic clocksource framework for simplicity, but without HPET-assisted > calibration as in x86. Instead, the ExtCC frequency is directly taken > from firmware. > > Blindly trusting firmware-provided values indeed can lead to clock > drifts, of course, but according to own observation on a 3A3000 board > the drift is somewhat acceptable. For example, during a certain test > run, the firmware advertised 1400.020 MHz, which was refined to > 1400.027 MHz by the old ported calibration code. > > The current code is tested on a single-socket Loongson 3A3000 board for > slightly over a week, while the old code incorporating essentially the > same logic was deployed for well over 2 years. Stability is perfect and > responsiveness improved greatly, according to cyclictest. > > The cyclictest benchmark figures are to be filled later. > > Signed-off-by: Wang Xuerui <wangxuerui@qiniu.com> > Tested-by: Wang Xuerui <wangxuerui@qiniu.com> That should really be a given :) I'd drop the Tested-by: for your own patch. > Cc: Huacai Chen <chenhc@lemote.com> > Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> > Cc: James Hogan <jhogan@kernel.org> > Cc: Paul Burton <paul.burton@mips.com> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: linux-mips@linux-mips.org > --- > arch/mips/include/asm/mach-loongson64/extcc.h | 26 +++++++++ > arch/mips/loongson64/Kconfig | 13 +++++ > arch/mips/loongson64/common/Makefile | 5 ++ > arch/mips/loongson64/common/extcc.c | 55 +++++++++++++++++++ > arch/mips/loongson64/common/time.c | 7 +++ > 5 files changed, 106 insertions(+) > create mode 100644 arch/mips/include/asm/mach-loongson64/extcc.h > create mode 100644 arch/mips/loongson64/common/extcc.c > > diff --git a/arch/mips/include/asm/mach-loongson64/extcc.h b/arch/mips/include/asm/mach-loongson64/extcc.h > new file mode 100644 > index 000000000000..9e70f195441e > --- /dev/null > +++ b/arch/mips/include/asm/mach-loongson64/extcc.h > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ASM_MACH_LOONGSON64_EXTCC_H > +#define __ASM_MACH_LOONGSON64_EXTCC_H > + > +extern void extcc_clocksource_init(void); > + > +// TODO: is the sync really needed on GS464E? Or if this is really the case, > +// is a weakly-ordered version of this suitable for coarser granularity, > +// just like in x86? Please use the standard kernel comment style: /* * TODO: ... */ > +static __always_inline u64 __extcc_read_ordered(void) > +{ > + u64 result; > + > + __asm__ __volatile__( > + ".set push\n\t" > + ".set arch=mips32r2\n\t" Generally I'd prefer: ".set\t" MIPS_ISA_LEVEL "\n\t" > + ".set noreorder\n\t" The .set noreorder directive here is redundant - there are no branches or jumps in this inline asm & thus no delay slots for the assembler to reorder instructions into. > + "sync\n\t" > + "rdhwr %0, $30\n\t" > + ".set pop\n" > + : "=r"(result)); > + > + return result; > +} > + > +#endif /* __ASM_MACH_LOONGSON64_EXTCC_H */ > diff --git a/arch/mips/loongson64/Kconfig b/arch/mips/loongson64/Kconfig > index 4c14a11525f4..a7213a25a6ab 100644 > --- a/arch/mips/loongson64/Kconfig > +++ b/arch/mips/loongson64/Kconfig > @@ -123,6 +123,19 @@ config RS780_HPET > > If unsure, say Yes. > > +config LOONGSON_EXTCC_CLKSRC > + bool "GS464E external counter clocksource" > + depends on LOONGSON3_ENHANCEMENT && GENERIC_SCHED_CLOCK > + help > + This option enables the external counter found on GS464E chips to > + be used as clocksource. > + > + The external counter is an internal cycle counter independent of > + processor cores, and provides very good (nanosecond-scale) time > + resolution. > + > + If unsure, say Yes. > + > config LOONGSON_UART_BASE > bool > default y > diff --git a/arch/mips/loongson64/common/Makefile b/arch/mips/loongson64/common/Makefile > index 684624f61f5a..5416c794123f 100644 > --- a/arch/mips/loongson64/common/Makefile > +++ b/arch/mips/loongson64/common/Makefile > @@ -25,3 +25,8 @@ obj-$(CONFIG_CS5536) += cs5536/ > # > > obj-$(CONFIG_SUSPEND) += pm.o > + > +# > +# ExtCC clocksource support > +# > +obj-$(CONFIG_LOONGSON_EXTCC_CLKSRC) += extcc.o > diff --git a/arch/mips/loongson64/common/extcc.c b/arch/mips/loongson64/common/extcc.c > new file mode 100644 > index 000000000000..702cb389856a > --- /dev/null > +++ b/arch/mips/loongson64/common/extcc.c I think this code belongs under drivers/clocksource/, and the relevant maintainers should be copied: $ ./scripts/get_maintainer.pl -f drivers/clocksource Daniel Lezcano <daniel.lezcano@linaro.org> (supporter:CLOCKSOURCE, CLOCKEVENT DRIVERS) Thomas Gleixner <tglx@linutronix.de> (supporter:CLOCKSOURCE, CLOCKEVENT DRIVERS) linux-kernel@vger.kernel.org (open list:CLOCKSOURCE, CLOCKEVENT DRIVERS) > @@ -0,0 +1,55 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/clocksource.h> > +#include <linux/init.h> > +#include <linux/sched_clock.h> > + > +#include <loongson.h> > +#include <extcc.h> > + > +static u64 notrace extcc_read(struct clocksource *cs) > +{ > + return __extcc_read_ordered(); > +} > + > +static u64 notrace extcc_sched_clock(void) > +{ > + return __extcc_read_ordered(); > +} > + > +static struct clocksource extcc_clocksource = { > + .name = "extcc", > + .read = extcc_read, > + .mask = CLOCKSOURCE_MASK(64), > + .flags = CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_VALID_FOR_HRES, > + /* TODO later */ > + .archdata = { .vdso_clock_mode = VDSO_CLOCK_NONE }, > +}; > + > +void __init extcc_clocksource_init(void) > +{ > + /* trust the firmware-provided frequency */ > + u32 extcc_frequency = cpu_clock_freq; > + int ret; > + > + if (extcc_frequency == 0) { > + pr_err("Frequency not specified\n"); > + return; > + } Is this actually possible? The loongson64 implementation of prom_init_env() seems to always assign a non-zero value to cpu_clock_freq. > + > + /* > + * As for the rating, 200+ is good while 300+ is desirable. > + * Use 1GHz as bar for "desirable"; most Loongson processors with support > + * for ExtCC already satisfy this. > + */ > + extcc_clocksource.rating = 200 + extcc_frequency / 10000000; > + > + ret = clocksource_register_hz(&extcc_clocksource, extcc_frequency); > + if (ret < 0) > + pr_warn("Unable to register clocksource\n"); > + > + /* mark extcc as sched clock */ > + sched_clock_register(extcc_sched_clock, 64, extcc_frequency); > +} > + > diff --git a/arch/mips/loongson64/common/time.c b/arch/mips/loongson64/common/time.c > index 0ba53c55ff33..afacc50b7501 100644 > --- a/arch/mips/loongson64/common/time.c > +++ b/arch/mips/loongson64/common/time.c > @@ -16,6 +16,9 @@ > > #include <loongson.h> > #include <cs5536/cs5536_mfgpt.h> > +#ifdef CONFIG_LOONGSON_EXTCC_CLKSRC > +#include <extcc.h> > +#endif Drop the #ifdef for header inclusion - it'll be harmless to include the header even if nothing from it is used. Thanks, Paul > > void __init plat_time_init(void) > { > @@ -27,6 +30,10 @@ void __init plat_time_init(void) > #else > setup_mfgpt0_timer(); > #endif > + > +#ifdef CONFIG_LOONGSON_EXTCC_CLKSRC > + extcc_clocksource_init(); > +#endif > } > > void read_persistent_clock64(struct timespec64 *ts) > -- > 2.20.1 > >
diff --git a/arch/mips/include/asm/mach-loongson64/extcc.h b/arch/mips/include/asm/mach-loongson64/extcc.h new file mode 100644 index 000000000000..9e70f195441e --- /dev/null +++ b/arch/mips/include/asm/mach-loongson64/extcc.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ASM_MACH_LOONGSON64_EXTCC_H +#define __ASM_MACH_LOONGSON64_EXTCC_H + +extern void extcc_clocksource_init(void); + +// TODO: is the sync really needed on GS464E? Or if this is really the case, +// is a weakly-ordered version of this suitable for coarser granularity, +// just like in x86? +static __always_inline u64 __extcc_read_ordered(void) +{ + u64 result; + + __asm__ __volatile__( + ".set push\n\t" + ".set arch=mips32r2\n\t" + ".set noreorder\n\t" + "sync\n\t" + "rdhwr %0, $30\n\t" + ".set pop\n" + : "=r"(result)); + + return result; +} + +#endif /* __ASM_MACH_LOONGSON64_EXTCC_H */ diff --git a/arch/mips/loongson64/Kconfig b/arch/mips/loongson64/Kconfig index 4c14a11525f4..a7213a25a6ab 100644 --- a/arch/mips/loongson64/Kconfig +++ b/arch/mips/loongson64/Kconfig @@ -123,6 +123,19 @@ config RS780_HPET If unsure, say Yes. +config LOONGSON_EXTCC_CLKSRC + bool "GS464E external counter clocksource" + depends on LOONGSON3_ENHANCEMENT && GENERIC_SCHED_CLOCK + help + This option enables the external counter found on GS464E chips to + be used as clocksource. + + The external counter is an internal cycle counter independent of + processor cores, and provides very good (nanosecond-scale) time + resolution. + + If unsure, say Yes. + config LOONGSON_UART_BASE bool default y diff --git a/arch/mips/loongson64/common/Makefile b/arch/mips/loongson64/common/Makefile index 684624f61f5a..5416c794123f 100644 --- a/arch/mips/loongson64/common/Makefile +++ b/arch/mips/loongson64/common/Makefile @@ -25,3 +25,8 @@ obj-$(CONFIG_CS5536) += cs5536/ # obj-$(CONFIG_SUSPEND) += pm.o + +# +# ExtCC clocksource support +# +obj-$(CONFIG_LOONGSON_EXTCC_CLKSRC) += extcc.o diff --git a/arch/mips/loongson64/common/extcc.c b/arch/mips/loongson64/common/extcc.c new file mode 100644 index 000000000000..702cb389856a --- /dev/null +++ b/arch/mips/loongson64/common/extcc.c @@ -0,0 +1,55 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/clocksource.h> +#include <linux/init.h> +#include <linux/sched_clock.h> + +#include <loongson.h> +#include <extcc.h> + +static u64 notrace extcc_read(struct clocksource *cs) +{ + return __extcc_read_ordered(); +} + +static u64 notrace extcc_sched_clock(void) +{ + return __extcc_read_ordered(); +} + +static struct clocksource extcc_clocksource = { + .name = "extcc", + .read = extcc_read, + .mask = CLOCKSOURCE_MASK(64), + .flags = CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_VALID_FOR_HRES, + /* TODO later */ + .archdata = { .vdso_clock_mode = VDSO_CLOCK_NONE }, +}; + +void __init extcc_clocksource_init(void) +{ + /* trust the firmware-provided frequency */ + u32 extcc_frequency = cpu_clock_freq; + int ret; + + if (extcc_frequency == 0) { + pr_err("Frequency not specified\n"); + return; + } + + /* + * As for the rating, 200+ is good while 300+ is desirable. + * Use 1GHz as bar for "desirable"; most Loongson processors with support + * for ExtCC already satisfy this. + */ + extcc_clocksource.rating = 200 + extcc_frequency / 10000000; + + ret = clocksource_register_hz(&extcc_clocksource, extcc_frequency); + if (ret < 0) + pr_warn("Unable to register clocksource\n"); + + /* mark extcc as sched clock */ + sched_clock_register(extcc_sched_clock, 64, extcc_frequency); +} + diff --git a/arch/mips/loongson64/common/time.c b/arch/mips/loongson64/common/time.c index 0ba53c55ff33..afacc50b7501 100644 --- a/arch/mips/loongson64/common/time.c +++ b/arch/mips/loongson64/common/time.c @@ -16,6 +16,9 @@ #include <loongson.h> #include <cs5536/cs5536_mfgpt.h> +#ifdef CONFIG_LOONGSON_EXTCC_CLKSRC +#include <extcc.h> +#endif void __init plat_time_init(void) { @@ -27,6 +30,10 @@ void __init plat_time_init(void) #else setup_mfgpt0_timer(); #endif + +#ifdef CONFIG_LOONGSON_EXTCC_CLKSRC + extcc_clocksource_init(); +#endif } void read_persistent_clock64(struct timespec64 *ts)