J-Core timer support
Subsystems (4)
Rich Felker
maintainerv8 cover J-Core timer support
Resubmitted with the (incorrect) workaround for rcu_sched stalls removed, since the problem was found in the interrupt controller driver; a separate patch will be submitted for it.
Rich Felker (2):
of: add J-Core timer bindings
clocksource: add J-Core timer/clocksource driver
.../devicetree/bindings/timer/jcore,pit.txt | 24 +++ drivers/clocksource/Kconfig | 10 + drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 231 +++++++++++++++++++++ include/linux/cpuhotplug.h | 1 + 5 files changed, 267 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt create mode 100644 drivers/clocksource/jcore-pit.c
Signature · 1 line
2.10.0
Rich Felker
maintainerPATCH v8 1/2 of: add J-Core timer bindings
--- .../devicetree/bindings/timer/jcore,pit.txt | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt
Documentation/devicetree/bindings/timer/jcore,pit.txt new file +24 -0
@@ -0,0 +1,24 @@ +J-Core Programmable Interval Timer and Clocksource + +Required properties: + +- compatible: Must be "jcore,pit". + +- reg: Memory region(s) for timer/clocksource registers. For SMP, + there should be one region per cpu, indexed by the sequential, + zero-based hardware cpu number. + +- interrupts: An interrupt to assign for the timer. The actual pit + core is integrated with the aic and allows the timer interrupt + assignment to be programmed by software, but this property is + required in order to reserve an interrupt number that doesn't + conflict with other devices. + + +Example: + +timer@200 { + compatible = "jcore,pit"; + reg = < 0x200 0x30 0x500 0x30 >; + interrupts = < 0x48 >; +};
Signature · 7 lines
2.10.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rich Felker
maintainerPATCH v8 2/2 clocksource: add J-Core timer/clocksource driver
In reply to Rich FelkerAt the hardware level, the J-Core PIT is integrated with the interrupt controller, but it is represented as its own device and has an independent programming interface. It provides a 12-bit countdown timer, which is not presently used, and a periodic timer. The interval length for the latter is programmable via a 32-bit throttle register whose units are determined by a bus-period register. The periodic timer is used to implement both periodic and oneshot clock event modes; in oneshot mode the interrupt handler simply disables the timer as soon as it fires.
Despite its device tree node representing an interrupt for the PIT, the actual irq generated is programmable, not hard-wired. The driver is responsible for programming the PIT to generate the hardware irq number that the DT assigns to it.
On SMP configurations, J-Core provides cpu-local instances of the PIT; no broadcast timer is needed. This driver supports the creation of the necessary per-cpu clock_event_device instances.
A nanosecond-resolution clocksource is provided using the J-Core "RTC" registers, which give a 64-bit seconds count and 32-bit nanoseconds that wrap every second. The driver converts these to a full-range 32-bit nanoseconds count.
--- drivers/clocksource/Kconfig | 10 ++ drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++ include/linux/cpuhotplug.h | 1 + 4 files changed, 243 insertions(+) create mode 100644 drivers/clocksource/jcore-pit.c
drivers/clocksource/Kconfig +10 -0
@@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU config SYS_SUPPORTS_EM_STI bool +config CLKSRC_JCORE_PIT + bool "J-Core PIT timer driver" + depends on OF && (SUPERH || COMPILE_TEST) + depends on GENERIC_CLOCKEVENTS + depends on HAS_IOMEM + select CLKSRC_MMIO + help + This enables build of clocksource and clockevent driver for + the integrated PIT in the J-Core synthesizable, open source SoC. + config SH_TIMER_CMT bool "Renesas CMT timer driver" if COMPILE_TEST depends on GENERIC_CLOCKEVENTS
drivers/clocksource/Makefile +1 -0
@@ -5,6 +5,7 @@ obj-$(CONFIG_ATMEL_TCB_CLKSRC) += tcb_clksrc.o obj-$(CONFIG_X86_PM_TIMER) += acpi_pm.o obj-$(CONFIG_SCx200HR_TIMER) += scx200_hrt.o obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC) += cs5535-clockevt.o +obj-$(CONFIG_CLKSRC_JCORE_PIT) += jcore-pit.o obj-$(CONFIG_SH_TIMER_CMT) += sh_cmt.o obj-$(CONFIG_SH_TIMER_MTU2) += sh_mtu2.o obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o
drivers/clocksource/jcore-pit.c new file +231 -0
231 lines not on this page · raw message
include/linux/cpuhotplug.h +1 -0
@@ -52,6 +52,7 @@ enum cpuhp_state {CPUHP_AP_ARM_ARCH_TIMER_STARTING ,CPUHP_AP_ARM_GLOBAL_TIMER_STARTING ,CPUHP_AP_DUMMY_TIMER_STARTING , +CPUHP_AP_JCORE_TIMER_STARTING ,CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING ,CPUHP_AP_ARM_TWD_STARTING , CPUHP_AP_METAG_TIMER_STARTING,
Signature · 1 line
2.10.0
Re: [PATCH v8 2/2] clocksource: add J-Core timer/clocksource driver
Hi Rich,
Quoted 10 lines · Rich Felker
At the hardware level, the J-Core PIT is integrated with the interrupt controller, but it is represented as its own device and has an independent programming interface. It provides a 12-bit countdown timer, which is not presently used, and a periodic timer. The interval length for the latter is programmable via a 32-bit throttle register whose units are determined by a bus-period register. The periodic timer is used to implement both periodic and oneshot clock event modes; in oneshot mode the interrupt handler simply disables the timer as soon as it fires.
Quoted 8 lines
Despite its device tree node representing an interrupt for the PIT, the actual irq generated is programmable, not hard-wired. The driver is responsible for programming the PIT to generate the hardware irq number that the DT assigns to it.
On SMP configurations, J-Core provides cpu-local instances of the PIT; no broadcast timer is needed. This driver supports the creation of the necessary per-cpu
clock_event_deviceinstances.
For my personnal information, why no broadcast timer is needed ?
Are the CPUs on always-on power down ?
Quoted 19 lines
A nanosecond-resolution clocksource is provided using the J-Core "RTC" registers, which give a 64-bit seconds count and 32-bit nanoseconds that wrap every second. The driver converts these to a full-range 32-bit nanoseconds count.
Signed-off-by Rich Felker <[email protected]>--- drivers/clocksource/Kconfig | 10 ++ drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++ include/linux/cpuhotplug.h | 1 + 4 files changed, 243 insertions(+) create mode 100644 drivers/clocksource/jcore-pit.cdrivers/clocksource/Kconfig +0
-0
drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 412
Quoted 6 lines
drivers/clocksource/Kconfig +3
-0@@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU config SYS_SUPPORTS_EM_STI bool +config CLKSRC_JCORE_PIT + bool "J-Core PIT timer driver" + depends on OF && (SUPERH || COMPILE_TEST)
Actually the idea is to have the SUPERH to select this timer, not create a dependency on SUPERH from here.
We don't want to prompt in the configuration menu the drivers because it would be impossible to anyone to know which timer comes with which hardware, so we let the platform to select the timer it needs.
The exception is to enable in order to compile on non-native platform to compile-test a bunch of drivers (eg. compile most of the clocksource / clockevents drivers on a x86 big server).
That's why we should have:
config CLKSRC_JCORE_PIT
bool "J-Core PIT timer driver" if COMPILE_TEST
So the jcore pit driver option appears only if compile test is enabled otherwise it is a silent option and the user doesn't have to deal with it. Having consistent dependency like the other drivers will help future changes to consolidate the Kconfig.
[ ... ]
drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU
Quoted 11 lines
drivers/clocksource/Kconfig +11
-0+#define REG_PITEN 0x00 +#define REG_THROT 0x10 +#define REG_COUNT 0x14 +#define REG_BUSPD 0x18 +#define REG_SECHI 0x20 +#define REG_SECLO 0x24 +#define REG_NSEC 0x28 + +struct jcore_pit { + struct clock_event_device ced; + __iomem void *base;
It is not '__iomem void *' but 'void __iomem *'. This appears multiple times in this code.
drivers/clocksource/Kconfig
Quoted 2 lines
drivers/clocksource/Kconfig +2
-0+ unsigned long periodic_delta; + unsigned cpu;
This field is pointless, 'cpu' is only used for a trace in the init function which has already the cpu has parameter.
drivers/clocksource/Kconfig
Quoted 4 lines
drivers/clocksource/Kconfig +4
-0+ u32 enable_val; +}; + +static __iomem void *jcore_pit_base;
static void __iomem *jcore_pit_base;
Quoted 1 line
+struct
jcore_pit__percpu*jcore_pit_percpu;
static.
[ ... ]
Quoted 2 lines
drivers/clocksource/Kconfig +2
-0+static int __init jcore_pit_init(struct device_node *node) +{
[ ... ]
Quoted 4 lines
drivers/clocksource/Kconfig +4
-0+ /* + * The J-Core PIT is not hard-wired to a particular IRQ, but + * integrated with the interrupt controller such that the IRQ it + * generates is programmable. The programming interface has a
drivers/clocksource/Kconfig
Quoted 10 lines
drivers/clocksource/Kconfig +10
-0+ * legacy field which was an interrupt priority for AIC1, but + * which is OR'd onto bits 2-5 of the generated IRQ number when + * used with J-Core AIC2, so set it to match these bits. + */ + hwirq = irq_get_irq_data(pit_irq)->hwirq; + irqprio = (hwirq >> 2) & PIT_PRIO_MASK; + enable_val = (1U << PIT_ENABLE_SHIFT) + | (hwirq << PIT_IRQ_SHIFT) + | (irqprio << PIT_PRIO_SHIFT); +
Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
Will be the same information available if the irqchip is AIC1 ?
I have to admin I find strange this driver has to invoke irq_get_irq_data(), it is the only one and it sounds even strange it has to be stored per cpu below.
drivers/clocksource/Kconfig
Quoted 1 line
drivers/clocksource/Kconfig +1
-0+ for_each_present_cpu(cpu) {
[ ... ]
drivers/clocksource/Kconfig
Quoted 10 lines
drivers/clocksource/Kconfig +10
-0+ pit->enable_val = enable_val; + } + + cpuhp_setup_state(CPUHP_AP_JCORE_TIMER_STARTING, + "AP_JCORE_TIMER_STARTING", + jcore_pit_local_init, NULL); + + return 0; +} +
Thanks !
-- Daniel
Rich Felker
maintainerRe: [PATCH v8 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 19 lines · Daniel Lezcano
Hi Rich,
Quoted 15 lines · Rich Felker
At the hardware level, the J-Core PIT is integrated with the interrupt controller, but it is represented as its own device and has an independent programming interface. It provides a 12-bit countdown timer, which is not presently used, and a periodic timer. The interval length for the latter is programmable via a 32-bit throttle register whose units are determined by a bus-period register. The periodic timer is used to implement both periodic and oneshot clock event modes; in oneshot mode the interrupt handler simply disables the timer as soon as it fires.
Despite its device tree node representing an interrupt for the PIT, the actual irq generated is programmable, not hard-wired. The driver is responsible for programming the PIT to generate the hardware irq number that the DT assigns to it.
Quoted 5 lines
Quoted 3 lines
On SMP configurations, J-Core provides cpu-local instances of the PIT; no broadcast timer is needed. This driver supports the creation of the necessary per-cpu
clock_event_deviceinstances.For my personnal information, why no broadcast timer is needed ?
Broadcast timer is only needed if you don't have percpu local timers. Early on in SMP development I actually tested with an ipi broadcast timer and performance was noticably worse.
Quoted 1 line
Are the CPUs on always-on power down ?
For now they are always on and don't even have the sleep instruction (i.e. stop cpu clock until interrupt) implemented. Adding sleep will be the first power-saving step, and perhaps the only one for now, since there doesn't seem to be any indication (according to the ppl working on the hardware) that a deeper sleep would provide significant additional savings.
Quoted 29 lines
Quoted 19 lines
A nanosecond-resolution clocksource is provided using the J-Core "RTC" registers, which give a 64-bit seconds count and 32-bit nanoseconds that wrap every second. The driver converts these to a full-range 32-bit nanoseconds count.
Signed-off-by Rich Felker <[email protected]>--- drivers/clocksource/Kconfig | 10 ++ drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++ include/linux/cpuhotplug.h | 1 + 4 files changed, 243 insertions(+) create mode 100644 drivers/clocksource/jcore-pit.cdrivers/clocksource/Kconfig +0
-0drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 412
Quoted 6 lines
drivers/clocksource/Kconfig +3
-0@@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU config SYS_SUPPORTS_EM_STI bool +config CLKSRC_JCORE_PIT + bool "J-Core PIT timer driver" + depends on OF && (SUPERH || COMPILE_TEST)Actually the idea is to have the SUPERH to select this timer, not create a dependency on SUPERH from here.
Quoted 3 lines
We don't want to prompt in the configuration menu the drivers because it would be impossible to anyone to know which timer comes with which hardware, so we let the platform to select the timer it needs.
I thought we discussed this before. For users building a kernel for legacy SH systems, especially in the current state where they're only supported with hard-coded board files rather than device tree, it makes no sense to build drivers for J-core hardware. It would make sense to be on by default for CONFIG_SH_DEVICE_TREE with a compatible CPU selection, but at least at this time, not for SUPERH in general.
Anyway I'd really like to do this non-invasively as long as we have a mix of legacy and new stuff and the legacy stuff is not readily testable. Once all of arch/sh is moved over to device tree, could we revisit this and make all the drivers follow a common policy (on by default if they're associated with boards/SoCs using a matching or compatible CPU model, or something like that, but still able to be disabled manually, since the user might be trying to get a tiny-ish embedded kernel)?
Quoted 6 lines
The exception is to enable in order to compile on non-native platform to compile-test a bunch of drivers (eg. compile most of the clocksource / clockevents drivers on a x86 big server).
That's why we should have:
Quoted 7 lines
config
CLKSRC_JCORE_PIT
bool "J-Core PIT timer driver" ifCOMPILE_TESTSo the jcore pit driver option appears only if compile test is enabled otherwise it is a silent option and the user doesn't have to deal with it. Having consistent dependency like the other drivers will help future changes to consolidate the
Kconfig.
I don't think this matches the user expectation for the arch yet. For now we have a j2_defconfig that enables the drivers and other kernel settings expected to be useful for J-core socs. I want to modernize this all but that's a separate project.
Quoted 16 lines
[ ... ]
drivers/clocksource/Kconfig
Quoted 11 lines
drivers/clocksource/Kconfig +11
-0+#define REG_PITEN 0x00 +#define REG_THROT 0x10 +#define REG_COUNT 0x14 +#define REG_BUSPD 0x18 +#define REG_SECHI 0x20 +#define REG_SECLO 0x24 +#define REG_NSEC 0x28 + +struct jcore_pit { + struct clock_event_device ced; + __iomem void *base;It is not '
__iomemvoid *' but 'void__iomem*'. This appears multiple times in this code.
OK, I'll change that.
Quoted 5 lines
drivers/clocksource/Kconfig
Quoted 2 lines
drivers/clocksource/Kconfig +2
-0+ unsigned long periodic_delta; + unsigned cpu;This field is pointless, 'cpu' is only used for a trace in the init function which has already the cpu has parameter.
OK, will remove. It was needed for the old notify framework but not for cpu hotplug framework, I think.
Quoted 6 lines
drivers/clocksource/Kconfig
Quoted 4 lines
drivers/clocksource/Kconfig +4
-0+ u32 enable_val; +}; + +static __iomem void *jcore_pit_base;static void
__iomem*jcore_pit_base;
OK.
Quoted 3 lines
Quoted 1 line
+struct
jcore_pit__percpu*jcore_pit_percpu;static.
OK.
Quoted 6 lines
[ ... ]
Quoted 2 lines
drivers/clocksource/Kconfig +2
-0+static int __init jcore_pit_init(struct device_node *node) +{[ ... ]
Was there something analogous you wanted me to change here, or just leftover quoting?
Quoted 17 lines
Quoted 4 lines
drivers/clocksource/Kconfig +4
-0+ /* + * The J-Core PIT is not hard-wired to a particular IRQ, but + * integrated with the interrupt controller such that the IRQ it + * generates is programmable. The programming interface has adrivers/clocksource/Kconfig
Quoted 10 lines
drivers/clocksource/Kconfig +10
-0+ * legacy field which was an interrupt priority for AIC1, but + * which is OR'd onto bits 2-5 of the generated IRQ number when + * used with J-Core AIC2, so set it to match these bits. + */ + hwirq = irq_get_irq_data(pit_irq)->hwirq; + irqprio = (hwirq >> 2) & PIT_PRIO_MASK; + enable_val = (1U << PIT_ENABLE_SHIFT) + | (hwirq << PIT_IRQ_SHIFT) + | (irqprio << PIT_PRIO_SHIFT); +Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
Quoted 1 line
Will be the same information available if the irqchip is AIC1 ?
The bit layout of the PIT enable register is:
.....e..ppppiiiiiiii............
where the .'s indicate unrelated/unused bits, e is enable, p is priority, and i is hard irq number.
For the PIT included in AIC1 (obsolete but still in use), any hard irq (trap number) can be programmed via the 8 iiiiiiii bits, and a priority (0-15) is programmable separately in the pppp bits.
For the PIT included in AIC2 (current), the programming interface is equivalent modulo interrupt mapping. This is why a different compatible tag was not used. However only traps 64-127 (the ones actually intended to be used for interrupts, rather than syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are ignored) and the priority pppp is <<2'd and or'd onto the irq number. This was a poor decision made on the hardware engineering side based on a wrong assumption that preserving old priority mapping of outdated software was important, whereas priorities weren't/aren't even being used.
When we do the next round of interrupt controller improvements (AIC3) the PIT programming interface should remain compatible with the driver; likely the priority bits will just be ignored.
If we do want to change the programming interface beyond this at some point (that maay be a good idea, since we have identified several things that are less than ideal for Linux, like the sechi/seclo/ns clocksource), a new compatible tag will be added instead.
Quoted 2 lines
I have to admin I find strange this driver has to invoke
irq_get_irq_data(), it is the only one and it sounds even strange it has to be stored per cpu below.
The timer will not actually generate the irq it's assigned to (or any interrupt at all) unless/until it's programmed for the (hard) irq number. The need to use irq_get_irq_data comes from the fact that the hardware needs the hard irq number, not a virq number, which could in principle be different.
There's probably some argument that could have been made that the irqchip and clocksource/timer driver should have been unified since they're unified in the hardware and have this awkward programming relationship, but that didn't fit the Linux model very well and I think having them factored like this encourages future versions of the hardware to adapt to the model the software wants.
Rich
Re: [PATCH v8 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 24 lines · Rich Felker
Quoted 23 lines · Daniel Lezcano
Hi Rich,
Quoted 18 lines · Rich Felker
At the hardware level, the J-Core PIT is integrated with the interrupt controller, but it is represented as its own device and has an independent programming interface. It provides a 12-bit countdown timer, which is not presently used, and a periodic timer. The interval length for the latter is programmable via a 32-bit throttle register whose units are determined by a bus-period register. The periodic timer is used to implement both periodic and oneshot clock event modes; in oneshot mode the interrupt handler simply disables the timer as soon as it fires.
Despite its device tree node representing an interrupt for the PIT, the actual irq generated is programmable, not hard-wired. The driver is responsible for programming the PIT to generate the hardware irq number that the DT assigns to it.
On SMP configurations, J-Core provides cpu-local instances of the PIT; no broadcast timer is needed. This driver supports the creation of the necessary per-cpu
clock_event_deviceinstances.
Quoted 5 lines
Quoted 1 line
For my personnal information, why no broadcast timer is needed ?
Broadcast timer is only needed if you don't have percpu local timers. Early on in SMP development I actually tested with an ipi broadcast timer and performance was noticably worse.
Obviously. I thought there were another reason related to power management.
Quoted 8 lines
Quoted 1 line
Are the CPUs on always-on power down ?
For now they are always on and don't even have the sleep instruction (i.e. stop cpu clock until interrupt) implemented. Adding sleep will be the first power-saving step, and perhaps the only one for now, since there doesn't seem to be any indication (according to the ppl working on the hardware) that a deeper sleep would provide significant additional savings.
Ok.
However, the 'sleep' state is not, in the power management terminology, the idle state described above. It is called "clock gated" / "Wait for Interrupt".
The 'sleep' state lose the CPU context.
Quoted 33 lines
Quoted 29 lines
Quoted 19 lines
A nanosecond-resolution clocksource is provided using the J-Core "RTC" registers, which give a 64-bit seconds count and 32-bit nanoseconds that wrap every second. The driver converts these to a full-range 32-bit nanoseconds count.
Signed-off-by Rich Felker <[email protected]>--- drivers/clocksource/Kconfig | 10 ++ drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++ include/linux/cpuhotplug.h | 1 + 4 files changed, 243 insertions(+) create mode 100644 drivers/clocksource/jcore-pit.cdrivers/clocksource/Kconfig +0
-0drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 412
Quoted 6 lines
drivers/clocksource/Kconfig +3
-0@@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU config SYS_SUPPORTS_EM_STI bool +config CLKSRC_JCORE_PIT + bool "J-Core PIT timer driver" + depends on OF && (SUPERH || COMPILE_TEST)Actually the idea is to have the SUPERH to select this timer, not create a dependency on SUPERH from here.
Quoted 3 lines
We don't want to prompt in the configuration menu the drivers because it would be impossible to anyone to know which timer comes with which hardware, so we let the platform to select the timer it needs.
Quoted 6 lines
I thought we discussed this before. For users building a kernel for legacy SH systems, especially in the current state where they're only supported with hard-coded board files rather than device tree, it makes no sense to build drivers for J-core hardware. It would make sense to be on by default for
CONFIG_SH_DEVICE_TREEwith a compatible CPU selection, but at least at this time, not for SUPERH in general.
Probably I am missing the point but why the user would have to unselect this driver manually ? The user wants a config file nothing more or a very trivial option. Can you imagine someone can know every single IP block for each boards of the same arch and be able to disable/enable the right ones ?
Quoted 8 lines
Anyway I'd really like to do this non-invasively as long as we have a mix of legacy and new stuff and the legacy stuff is not readily testable. Once all of
arch/shis moved over to device tree, could we revisit this and make all the drivers follow a common policy (on by default if they're associated with boards/SoCs using a matching or compatible CPU model, or something like that, but still able to be disabled manually, since the user might be trying to get a tiny-ish embedded kernel)?
I understand the goal is to have one single configuration and everything DT based and it sounds great but what is missing here is just a subarch, not an option to enable/disable the timer.
Give a try with:
make ARCH=arm multi_v7_defconfig menuconfig
--> System Type
That is what you are looking for, a SUPERH config option selecting all the common options and then a JCORE config option adding the different missing bits, namely the CLKSRC_JCORE_PIT.
Quoted 64 lines
Quoted 6 lines
The exception is to enable in order to compile on non-native platform to compile-test a bunch of drivers (eg. compile most of the clocksource / clockevents drivers on a x86 big server).
That's why we should have:
Quoted 7 lines
config
CLKSRC_JCORE_PIT
bool "J-Core PIT timer driver" ifCOMPILE_TESTSo the jcore pit driver option appears only if compile test is enabled otherwise it is a silent option and the user doesn't have to deal with it. Having consistent dependency like the other drivers will help future changes to consolidate the
Kconfig.I don't think this matches the user expectation for the arch yet. For now we have a
j2_defconfigthat enables the drivers and other kernel settings expected to be useful for J-core socs. I want to modernize this all but that's a separate project.Quoted 16 lines
[ ... ]
drivers/clocksource/Kconfig
Quoted 11 lines
drivers/clocksource/Kconfig +11
-0+#define REG_PITEN 0x00 +#define REG_THROT 0x10 +#define REG_COUNT 0x14 +#define REG_BUSPD 0x18 +#define REG_SECHI 0x20 +#define REG_SECLO 0x24 +#define REG_NSEC 0x28 + +struct jcore_pit { + struct clock_event_device ced; + __iomem void *base;It is not '
__iomemvoid *' but 'void__iomem*'. This appears multiple times in this code.OK, I'll change that.
Quoted 5 lines
drivers/clocksource/Kconfig
Quoted 2 lines
drivers/clocksource/Kconfig +2
-0+ unsigned long periodic_delta; + unsigned cpu;This field is pointless, 'cpu' is only used for a trace in the init function which has already the cpu has parameter.
OK, will remove. It was needed for the old notify framework but not for cpu hotplug framework, I think.
Quoted 6 lines
drivers/clocksource/Kconfig
Quoted 4 lines
drivers/clocksource/Kconfig +4
-0+ u32 enable_val; +}; + +static __iomem void *jcore_pit_base;static void
__iomem*jcore_pit_base;OK.
Quoted 3 lines
Quoted 1 line
+struct
jcore_pit__percpu*jcore_pit_percpu;static.
OK.
Quoted 2 lines
[ ... ]
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
drivers/clocksource/Kconfig +2
-0+static int __init jcore_pit_init(struct device_node *node) +{[ ... ]
Was there something analogous you wanted me to change here, or just leftover quoting?
No, I just cut until the function name in order to give the context for the next comment and then cut inside the function.
Quoted 41 lines
Quoted 17 lines
Quoted 4 lines
drivers/clocksource/Kconfig +4
-0+ /* + * The J-Core PIT is not hard-wired to a particular IRQ, but + * integrated with the interrupt controller such that the IRQ it + * generates is programmable. The programming interface has adrivers/clocksource/Kconfig
Quoted 10 lines
drivers/clocksource/Kconfig +10
-0+ * legacy field which was an interrupt priority for AIC1, but + * which is OR'd onto bits 2-5 of the generated IRQ number when + * used with J-Core AIC2, so set it to match these bits. + */ + hwirq = irq_get_irq_data(pit_irq)->hwirq; + irqprio = (hwirq >> 2) & PIT_PRIO_MASK; + enable_val = (1U << PIT_ENABLE_SHIFT) + | (hwirq << PIT_IRQ_SHIFT) + | (irqprio << PIT_PRIO_SHIFT); +Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
Quoted 1 line
Will be the same information available if the irqchip is AIC1 ?
The bit layout of the PIT enable register is:
.....e..ppppiiiiiiii............where the .'s indicate unrelated/unused bits, e is enable, p is priority, and i is hard irq number.
For the PIT included in AIC1 (obsolete but still in use), any hard irq (trap number) can be programmed via the 8 iiiiiiii bits, and a priority (0-15) is programmable separately in the pppp bits.
For the PIT included in AIC2 (current), the programming interface is equivalent modulo interrupt mapping. This is why a different compatible tag was not used. However only traps 64-127 (the ones actually intended to be used for interrupts, rather than syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are ignored) and the priority pppp is <<2'd and or'd onto the irq number. This was a poor decision made on the hardware engineering side based on a wrong assumption that preserving old priority mapping of outdated software was important, whereas priorities weren't/aren't even being used.
Quoted 8 lines
When we do the next round of interrupt controller improvements (AIC3) the PIT programming interface should remain compatible with the driver; likely the priority bits will just be ignored.
If we do want to change the programming interface beyond this at some point (that maay be a good idea, since we have identified several things that are less than ideal for Linux, like the sechi/seclo/ns clocksource), a new compatible tag will be added instead.
Ok, thanks for the clarification. Can you add your answer as a comment for the bits dance above ?
Quoted 9 lines
Quoted 2 lines
I have to admin I find strange this driver has to invoke
irq_get_irq_data(), it is the only one and it sounds even strange it has to be stored per cpu below.The timer will not actually generate the irq it's assigned to (or any interrupt at all) unless/until it's programmed for the (hard) irq number. The need to use
irq_get_irq_datacomes from the fact that the hardware needs the hard irq number, not a virq number, which could in principle be different.
Quoted 6 lines
There's probably some argument that could have been made that the irqchip and clocksource/timer driver should have been unified since they're unified in the hardware and have this awkward programming relationship, but that didn't fit the Linux model very well and I think having them factored like this encourages future versions of the hardware to adapt to the model the software wants.
Ok, thanks.
-- Daniel
Rich Felker
maintainerRe: [PATCH v8 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 9 lines · Daniel Lezcano
Quoted 8 lines
Quoted 1 line
Are the CPUs on always-on power down ?
For now they are always on and don't even have the sleep instruction (i.e. stop cpu clock until interrupt) implemented. Adding sleep will be the first power-saving step, and perhaps the only one for now, since there doesn't seem to be any indication (according to the ppl working on the hardware) that a deeper sleep would provide significant additional savings.
Quoted 7 lines
Ok.
However, the 'sleep' state is not, in the power management terminology, the idle state described above. It is called "clock gated" / "Wait for Interrupt".
The 'sleep' state lose the CPU context.
I use the term "sleep" because that's the name of the SH instruction mnemonic for the opcode for entering the wait-for-interrupt state. Sorry it's confusing like the "RTC" all over again. :-)
Quoted 40 lines
Quoted 33 lines
Quoted 29 lines
Quoted 19 lines
A nanosecond-resolution clocksource is provided using the J-Core "RTC" registers, which give a 64-bit seconds count and 32-bit nanoseconds that wrap every second. The driver converts these to a full-range 32-bit nanoseconds count.
Signed-off-by Rich Felker <[email protected]>--- drivers/clocksource/Kconfig | 10 ++ drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++ include/linux/cpuhotplug.h | 1 + 4 files changed, 243 insertions(+) create mode 100644 drivers/clocksource/jcore-pit.cdrivers/clocksource/Kconfig +0
-0drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 412
Quoted 6 lines
drivers/clocksource/Kconfig +3
-0@@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU config SYS_SUPPORTS_EM_STI bool +config CLKSRC_JCORE_PIT + bool "J-Core PIT timer driver" + depends on OF && (SUPERH || COMPILE_TEST)Actually the idea is to have the SUPERH to select this timer, not create a dependency on SUPERH from here.
Quoted 3 lines
We don't want to prompt in the configuration menu the drivers because it would be impossible to anyone to know which timer comes with which hardware, so we let the platform to select the timer it needs.
Quoted 6 lines
I thought we discussed this before. For users building a kernel for legacy SH systems, especially in the current state where they're only supported with hard-coded board files rather than device tree, it makes no sense to build drivers for J-core hardware. It would make sense to be on by default for
CONFIG_SH_DEVICE_TREEwith a compatible CPU selection, but at least at this time, not for SUPERH in general.
Quoted 4 lines
Probably I am missing the point but why the user would have to unselect this driver manually ? The user wants a config file nothing more or a very trivial option. Can you imagine someone can know every single IP block for each boards of the same arch and be able to disable/enable the right ones ?
The common case I imagine is just accepting defaults that include more than you need, but for space-constrained (or more likely, fast boot, when the kernel is being loaded from a slow spi-based SD card interface) setups the user might be trying to minimize kernel size and turn off drivers they know they don't want. I'm not so worried about this driver, specifically, because it's small, but I am concerned about the general policy -- when we get rid of all the legacy board files and everything is move to device tree, will the user be stuck including a bunch of Renesas SH drivers when building a kernel the intend to use only on J-core?
Quoted 15 lines
Quoted 8 lines
Anyway I'd really like to do this non-invasively as long as we have a mix of legacy and new stuff and the legacy stuff is not readily testable. Once all of
arch/shis moved over to device tree, could we revisit this and make all the drivers follow a common policy (on by default if they're associated with boards/SoCs using a matching or compatible CPU model, or something like that, but still able to be disabled manually, since the user might be trying to get a tiny-ish embedded kernel)?I understand the goal is to have one single configuration and everything DT based and it sounds great but what is missing here is just a subarch, not an option to enable/disable the timer.
Give a try with:
Quoted 7 lines
make ARCH=arm
multi_v7_defconfigmenuconfig--> System Type
That is what you are looking for, a SUPERH config option selecting all the common options and then a JCORE config option adding the different missing bits, namely the
CLKSRC_JCORE_PIT.
We do have something like "system type" in arch/sh, and it's what I'm trying to deprecate since it's the switch to select between all the hard-coded board files, _or_ device tree.
Since part of the goal of my DT overhaul is to be able (but not forced) to produce kernels that run on a wide range of hardware, rather than having a "system type (select one)" option, what about individual boolean options like:
config JCORE_SOC
bool "Support for J-Core SoCs" select CLKSRC_JCORE_PIT select JCORE_AIC ...
Note that there are other drivers that should probably be optional even if you have JCORE_SOC enabled, like the SPI controller, DMA controller (not implemented yet), Ethernet (not submitted upstream yet), etc. Maybe they could depend on JCORE_SOC and be default-yes but configurable if available?
In any case, the SoC support is supposedly there in the current kernel release (4.8) but not working because of missing essential drivers, so I'd really like to fix that without making the fix dependent on restructuring the arch/sh system type handling, which is an ongoing, independent project for which I'm waiting for help converting and testing the conversions of legacy board support. My preference would be to keep the Kconfig stuff the way I submitted it for now -- j2_defconfig already handles enabling thse right drivers -- and do something more user-friendly as part of the bigger arch overhaul project.
Quoted 45 lines
Quoted 45 lines
Quoted 17 lines
Quoted 4 lines
drivers/clocksource/Kconfig +4
-0+ /* + * The J-Core PIT is not hard-wired to a particular IRQ, but + * integrated with the interrupt controller such that the IRQ it + * generates is programmable. The programming interface has adrivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU
Quoted 10 lines
drivers/clocksource/Kconfig +10
-0+ * legacy field which was an interrupt priority for AIC1, but + * which is OR'd onto bits 2-5 of the generated IRQ number when + * used with J-Core AIC2, so set it to match these bits. + */ + hwirq = irq_get_irq_data(pit_irq)->hwirq; + irqprio = (hwirq >> 2) & PIT_PRIO_MASK; + enable_val = (1U << PIT_ENABLE_SHIFT) + | (hwirq << PIT_IRQ_SHIFT) + | (irqprio << PIT_PRIO_SHIFT); +Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
Quoted 1 line
Will be the same information available if the irqchip is AIC1 ?
The bit layout of the PIT enable register is:
.....e..ppppiiiiiiii............where the .'s indicate unrelated/unused bits, e is enable, p is priority, and i is hard irq number.
For the PIT included in AIC1 (obsolete but still in use), any hard irq (trap number) can be programmed via the 8 iiiiiiii bits, and a priority (0-15) is programmable separately in the pppp bits.
For the PIT included in AIC2 (current), the programming interface is equivalent modulo interrupt mapping. This is why a different compatible tag was not used. However only traps 64-127 (the ones actually intended to be used for interrupts, rather than syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are ignored) and the priority pppp is <<2'd and or'd onto the irq number. This was a poor decision made on the hardware engineering side based on a wrong assumption that preserving old priority mapping of outdated software was important, whereas priorities weren't/aren't even being used.
When we do the next round of interrupt controller improvements (AIC3) the PIT programming interface should remain compatible with the driver; likely the priority bits will just be ignored.
Quoted 7 lines
Quoted 4 lines
If we do want to change the programming interface beyond this at some point (that maay be a good idea, since we have identified several things that are less than ideal for Linux, like the sechi/seclo/ns clocksource), a new compatible tag will be added instead.
Ok, thanks for the clarification. Can you add your answer as a comment for the bits dance above ?
Are you happy with the whole quoted text above as a comment? If so I'm happy to include it verbatim. I would lean towards condensing or omitting the last 2 paragraphs (starting with "When we do...") if that's okay with you since they are not documenting the hw but future plans/policy.
Rich
Re: [PATCH v8 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 110 lines · Rich Felker
Quoted 9 lines · Daniel Lezcano
Quoted 8 lines
Quoted 1 line
Are the CPUs on always-on power down ?
For now they are always on and don't even have the sleep instruction (i.e. stop cpu clock until interrupt) implemented. Adding sleep will be the first power-saving step, and perhaps the only one for now, since there doesn't seem to be any indication (according to the ppl working on the hardware) that a deeper sleep would provide significant additional savings.
Quoted 7 lines
Ok.
However, the 'sleep' state is not, in the power management terminology, the idle state described above. It is called "clock gated" / "Wait for Interrupt".
The 'sleep' state lose the CPU context.
I use the term "sleep" because that's the name of the SH instruction mnemonic for the opcode for entering the wait-for-interrupt state. Sorry it's confusing like the "RTC" all over again. :-)
Quoted 40 lines
Quoted 33 lines
Quoted 29 lines
Quoted 19 lines
A nanosecond-resolution clocksource is provided using the J-Core "RTC" registers, which give a 64-bit seconds count and 32-bit nanoseconds that wrap every second. The driver converts these to a full-range 32-bit nanoseconds count.
Signed-off-by Rich Felker <[email protected]>--- drivers/clocksource/Kconfig | 10 ++ drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++ include/linux/cpuhotplug.h | 1 + 4 files changed, 243 insertions(+) create mode 100644 drivers/clocksource/jcore-pit.cdrivers/clocksource/Kconfig +0
-0drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 412
Quoted 6 lines
drivers/clocksource/Kconfig +3
-0@@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU config SYS_SUPPORTS_EM_STI bool +config CLKSRC_JCORE_PIT + bool "J-Core PIT timer driver" + depends on OF && (SUPERH || COMPILE_TEST)Actually the idea is to have the SUPERH to select this timer, not create a dependency on SUPERH from here.
Quoted 3 lines
We don't want to prompt in the configuration menu the drivers because it would be impossible to anyone to know which timer comes with which hardware, so we let the platform to select the timer it needs.
Quoted 6 lines
I thought we discussed this before. For users building a kernel for legacy SH systems, especially in the current state where they're only supported with hard-coded board files rather than device tree, it makes no sense to build drivers for J-core hardware. It would make sense to be on by default for
CONFIG_SH_DEVICE_TREEwith a compatible CPU selection, but at least at this time, not for SUPERH in general.Quoted 4 lines
Probably I am missing the point but why the user would have to unselect this driver manually ? The user wants a config file nothing more or a very trivial option. Can you imagine someone can know every single IP block for each boards of the same arch and be able to disable/enable the right ones ?
The common case I imagine is just accepting defaults that include more than you need, but for space-constrained (or more likely, fast boot, when the kernel is being loaded from a slow spi-based SD card interface) setups the user might be trying to minimize kernel size and turn off drivers they know they don't want. I'm not so worried about this driver, specifically, because it's small, but I am concerned about the general policy -- when we get rid of all the legacy board files and everything is move to device tree, will the user be stuck including a bunch of Renesas SH drivers when building a kernel the intend to use only on J-core?
Quoted 15 lines
Quoted 8 lines
Anyway I'd really like to do this non-invasively as long as we have a mix of legacy and new stuff and the legacy stuff is not readily testable. Once all of
arch/shis moved over to device tree, could we revisit this and make all the drivers follow a common policy (on by default if they're associated with boards/SoCs using a matching or compatible CPU model, or something like that, but still able to be disabled manually, since the user might be trying to get a tiny-ish embedded kernel)?I understand the goal is to have one single configuration and everything DT based and it sounds great but what is missing here is just a subarch, not an option to enable/disable the timer.
Give a try with:
Quoted 7 lines
make ARCH=arm
multi_v7_defconfigmenuconfig--> System Type
That is what you are looking for, a SUPERH config option selecting all the common options and then a JCORE config option adding the different missing bits, namely the
CLKSRC_JCORE_PIT.We do have something like "system type" in
arch/sh, and it's what I'm trying to deprecate since it's the switch to select between all the hard-coded board files, _or_ device tree.Since part of the goal of my DT overhaul is to be able (but not forced) to produce kernels that run on a wide range of hardware, rather than having a "system type (select one)" option, what about individual boolean options like:
Quoted 5 lines
config
JCORE_SOCbool "Support for J-Core SoCs" select CLKSRC_JCORE_PIT select JCORE_AIC ...
I'm perfectly fine with this.
Quoted 5 lines
Note that there are other drivers that should probably be optional even if you have
JCORE_SOCenabled, like the SPI controller, DMA controller (not implemented yet), Ethernet (not submitted upstream yet), etc. Maybe they could depend onJCORE_SOCand be default-yes but configurable if available?
That sounds fine also.
Quoted 10 lines
In any case, the SoC support is supposedly there in the current kernel release (4.8) but not working because of missing essential drivers, so I'd really like to fix that without making the fix dependent on restructuring the
arch/shsystem type handling, which is an ongoing, independent project for which I'm waiting for help converting and testing the conversions of legacy board support. My preference would be to keep theKconfigstuff the way I submitted it for now --j2_defconfigalready handles enabling thse right drivers -- and do something more user-friendly as part of the bigger arch overhaul project.
I prefer the move the option to config JCORE_SOC. That is not a big deal to add this bool in the sh's Kconfig and select the timer from there.
Quoted 50 lines
Quoted 50 lines
Quoted 49 lines
Quoted 17 lines
Quoted 4 lines
drivers/clocksource/Kconfig +4
-0+ /* + * The J-Core PIT is not hard-wired to a particular IRQ, but + * integrated with the interrupt controller such that the IRQ it + * generates is programmable. The programming interface has adrivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU
Quoted 10 lines
drivers/clocksource/Kconfig +10
-0+ * legacy field which was an interrupt priority for AIC1, but + * which is OR'd onto bits 2-5 of the generated IRQ number when + * used with J-Core AIC2, so set it to match these bits. + */ + hwirq = irq_get_irq_data(pit_irq)->hwirq; + irqprio = (hwirq >> 2) & PIT_PRIO_MASK; + enable_val = (1U << PIT_ENABLE_SHIFT) + | (hwirq << PIT_IRQ_SHIFT) + | (irqprio << PIT_PRIO_SHIFT); +Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
Quoted 1 line
Will be the same information available if the irqchip is AIC1 ?
The bit layout of the PIT enable register is:
.....e..ppppiiiiiiii............where the .'s indicate unrelated/unused bits, e is enable, p is priority, and i is hard irq number.
For the PIT included in AIC1 (obsolete but still in use), any hard irq (trap number) can be programmed via the 8 iiiiiiii bits, and a priority (0-15) is programmable separately in the pppp bits.
For the PIT included in AIC2 (current), the programming interface is equivalent modulo interrupt mapping. This is why a different compatible tag was not used. However only traps 64-127 (the ones actually intended to be used for interrupts, rather than syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are ignored) and the priority pppp is <<2'd and or'd onto the irq number. This was a poor decision made on the hardware engineering side based on a wrong assumption that preserving old priority mapping of outdated software was important, whereas priorities weren't/aren't even being used.
When we do the next round of interrupt controller improvements (AIC3) the PIT programming interface should remain compatible with the driver; likely the priority bits will just be ignored.
If we do want to change the programming interface beyond this at some point (that maay be a good idea, since we have identified several things that are less than ideal for Linux, like the sechi/seclo/ns clocksource), a new compatible tag will be added instead.
Quoted 8 lines
Quoted 2 lines
Ok, thanks for the clarification. Can you add your answer as a comment for the bits dance above ?
Are you happy with the whole quoted text above as a comment? If so I'm happy to include it verbatim. I would lean towards condensing or omitting the last 2 paragraphs (starting with "When we do...") if that's okay with you since they are not documenting the hw but future plans/policy.
Makes sense.
Agree for the verbatim minus the last 2 paragraphs.
-- Daniel
Rich Felker
maintainerRe: [PATCH v8 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 50 lines · Daniel Lezcano
Quoted 23 lines
Quoted 6 lines
I understand the goal is to have one single configuration and everything DT based and it sounds great but what is missing here is just a subarch, not an option to enable/disable the timer.
Give a try with:
Quoted 7 lines
make ARCH=arm
multi_v7_defconfigmenuconfig--> System Type
That is what you are looking for, a SUPERH config option selecting all the common options and then a JCORE config option adding the different missing bits, namely the
CLKSRC_JCORE_PIT.We do have something like "system type" in
arch/sh, and it's what I'm trying to deprecate since it's the switch to select between all the hard-coded board files, _or_ device tree.Since part of the goal of my DT overhaul is to be able (but not forced) to produce kernels that run on a wide range of hardware, rather than having a "system type (select one)" option, what about individual boolean options like:
Quoted 5 lines
config
JCORE_SOCbool "Support for J-Core SoCs" select CLKSRC_JCORE_PIT select JCORE_AIC ...I'm perfectly fine with this.
Quoted 5 lines
Note that there are other drivers that should probably be optional even if you have
JCORE_SOCenabled, like the SPI controller, DMA controller (not implemented yet), Ethernet (not submitted upstream yet), etc. Maybe they could depend onJCORE_SOCand be default-yes but configurable if available?That sounds fine also.
Quoted 10 lines
In any case, the SoC support is supposedly there in the current kernel release (4.8) but not working because of missing essential drivers, so I'd really like to fix that without making the fix dependent on restructuring the
arch/shsystem type handling, which is an ongoing, independent project for which I'm waiting for help converting and testing the conversions of legacy board support. My preference would be to keep theKconfigstuff the way I submitted it for now --j2_defconfigalready handles enabling thse right drivers -- and do something more user-friendly as part of the bigger arch overhaul project.
Quoted 2 lines
I prefer the move the option to config
JCORE_SOC. That is not a big deal to add this bool in the sh'sKconfigand select the timer from there.
OK, I'll do this and add the patch to my planned pull request, and send a corresponding Kconfig patch for the interrupt controller.
Quoted 59 lines
Quoted 50 lines
Quoted 50 lines
Quoted 49 lines
Quoted 17 lines
Quoted 4 lines
drivers/clocksource/jcore-pit.c new file +4
-0+/* +* The J-Core PIT is not hard-wired to a particular IRQ, but +* integrated with the interrupt controller such that the IRQ it +* generates is programmable. The programming interface has a drivers/clocksource/jcore-pit.c line 198
Quoted 10 lines
drivers/clocksource/jcore-pit.c new file +10
-0+* legacy field which was an interrupt priority for AIC1, but +* which is OR'd onto bits 2-5 of the generated IRQ number when +* used with J-Core AIC2, so set it to match these bits. +*/ + hwirq =irq_get_irq_data (pit_irq)->hwirq; + irqprio = (hwirq >>2 ) &PIT_PRIO_MASK ; + enable_val = (1U <<PIT_ENABLE_SHIFT ) + | (hwirq <<PIT_IRQ_SHIFT ) + | (irqprio <<PIT_PRIO_SHIFT ); +Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
Quoted 1 line
Will be the same information available if the irqchip is AIC1 ?
The bit layout of the PIT enable register is:
.....e..ppppiiiiiiii............where the .'s indicate unrelated/unused bits, e is enable, p is priority, and i is hard irq number.
For the PIT included in AIC1 (obsolete but still in use), any hard irq (trap number) can be programmed via the 8 iiiiiiii bits, and a priority (0-15) is programmable separately in the pppp bits.
For the PIT included in AIC2 (current), the programming interface is equivalent modulo interrupt mapping. This is why a different compatible tag was not used. However only traps 64-127 (the ones actually intended to be used for interrupts, rather than syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are ignored) and the priority pppp is <<2'd and or'd onto the irq number. This was a poor decision made on the hardware engineering side based on a wrong assumption that preserving old priority mapping of outdated software was important, whereas priorities weren't/aren't even being used.
When we do the next round of interrupt controller improvements (AIC3) the PIT programming interface should remain compatible with the driver; likely the priority bits will just be ignored.
If we do want to change the programming interface beyond this at some point (that maay be a good idea, since we have identified several things that are less than ideal for Linux, like the sechi/seclo/ns clocksource), a new compatible tag will be added instead.
Quoted 8 lines
Quoted 2 lines
Ok, thanks for the clarification. Can you add your answer as a comment for the bits dance above ?
Are you happy with the whole quoted text above as a comment? If so I'm happy to include it verbatim. I would lean towards condensing or omitting the last 2 paragraphs (starting with "When we do...") if that's okay with you since they are not documenting the hw but future plans/policy.
Quoted 3 lines
Makes sense.
Agree for the verbatim minus the last 2 paragraphs.
OK. I'll prepare a new patch with this and the previously-discussed changes.
Rich
Rich Felker
maintainerRe: [PATCH v8 2/2] clocksource: add J-Core timer/clocksource driver
In reply to Daniel LezcanoQuoted 15 lines · Daniel Lezcano
Quoted 15 lines
Quoted 5 lines
--> System Type
That is what you are looking for, a SUPERH config option selecting all the common options and then a JCORE config option adding the different missing bits, namely the
CLKSRC_JCORE_PIT.We do have something like "system type" in
arch/sh, and it's what I'm trying to deprecate since it's the switch to select between all the hard-coded board files, _or_ device tree.Since part of the goal of my DT overhaul is to be able (but not forced) to produce kernels that run on a wide range of hardware, rather than having a "system type (select one)" option, what about individual boolean options like:
Quoted 7 lines
Quoted 5 lines
config
JCORE_SOCbool "Support for J-Core SoCs" select CLKSRC_JCORE_PIT select JCORE_AIC ...I'm perfectly fine with this.
Does this adequately ensure that dependencies for the clocksource and irq driver are met? If not, what else do I need to do?
Rich
Rich Felker
maintainerRe: [PATCH v8 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 22 lines · Rich Felker
Quoted 21 lines · Daniel Lezcano
Quoted 20 lines
Quoted 5 lines
--> System Type
That is what you are looking for, a SUPERH config option selecting all the common options and then a JCORE config option adding the different missing bits, namely the
CLKSRC_JCORE_PIT.We do have something like "system type" in
arch/sh, and it's what I'm trying to deprecate since it's the switch to select between all the hard-coded board files, _or_ device tree.Since part of the goal of my DT overhaul is to be able (but not forced) to produce kernels that run on a wide range of hardware, rather than having a "system type (select one)" option, what about individual boolean options like:
config
JCORE_SOCbool "Support for J-Core SoCs" select CLKSRC_JCORE_PIT select JCORE_AIC ...
Quoted 4 lines
Quoted 1 line
I'm perfectly fine with this.
Does this adequately ensure that dependencies for the clocksource and irq driver are met? If not, what else do I need to do?
I've checked and it seems the dependencies are always met for arch/sh, at least with device tree enabled (on which JCORE_SOC will depend), so it shouldn't matter.
Rich
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html