Hi Wang,
Thanks for the patch - comments inline below.
On Sun, Feb 24, 2019 at 05:36:34PM +0800, kernel@xxxxxxxxxx wrote:
> From: Wang Xuerui <wangxuerui@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Tested-by: Wang Xuerui <wangxuerui@xxxxxxxxx>
That should really be a given :) I'd drop the Tested-by: for your own
patch.
> Cc: Huacai Chen <chenhc@xxxxxxxxxx>
> Cc: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> Cc: James Hogan <jhogan@xxxxxxxxxx>
> Cc: Paul Burton <paul.burton@xxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> Cc: linux-mips@xxxxxxxxxxxxxx
> ---
> 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@xxxxxxxxxx> (supporter:CLOCKSOURCE, CLOCKEVENT DRIVERS)
Thomas Gleixner <tglx@xxxxxxxxxxxxx> (supporter:CLOCKSOURCE, CLOCKEVENT DRIVERS)
linux-kernel@xxxxxxxxxxxxxxx (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
>
>
