J-Core timer support
Subsystems (3)
Rich Felker
maintainerv5 cover J-Core timer support
The only difference since v4 should be merging of Arnd Bergmann's patch that addresses the API change whereby init functions can return an error.
Rich
Rich Felker (2):
of: add J-Core timer bindings
clocksource: add J-Core timer/clocksource driver
.../devicetree/bindings/timer/jcore,pit.txt | 25 ++ drivers/clocksource/Kconfig | 8 + drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 283 +++++++++++++++++++++ 4 files changed, 317 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt create mode 100644 drivers/clocksource/jcore-pit.c
Signature · 6 lines
2.8.1 -- 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 v5 1/2 of: add J-Core timer bindings
--- .../devicetree/bindings/timer/jcore,pit.txt | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt
Documentation/devicetree/bindings/timer/jcore,pit.txt new file +25 -0
@@ -0,0 +1,25 @@ +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 (which is also the logical 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 · 1 line
2.8.1
Re: [PATCH v5 1/2] of: add J-Core timer bindings
Quoted 5 lines · Rich Felker
Signed-off-by Rich Felker <[email protected]>--- .../devicetree/bindings/timer/jcore,pit.txt | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt
Rich Felker
maintainerRe: [PATCH v5 1/2] of: add J-Core timer bindings
In reply to Rich FelkerQuoted 12 lines · Rich Felker
Signed-off-by Rich Felker <[email protected]>--- .../devicetree/bindings/timer/jcore,pit.txt | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txtDocumentation/devicetree/bindings/timer/jcore,pit.txt new file +0
-0
Documentation/devicetree/bindings/timer/jcore,pit.txt line 10
Quoted 10 lines
Documentation/devicetree/bindings/timer/jcore,pit.txt new file +10
-0@@ -0,0 +1,25 @@ +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 (which is also the logical cpu + number).
One detail I missed: Mark Rutland asked me to remove the corresponding remark about logical cpu numbers from the AIC binding document, so I think it should be removed here too for the same reason -- it's a Linux implementation detail, not part of the hw binding.
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
Rich Felker
maintainerPATCH v5 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. The code has been tested and works on SMP, but will not be usable without additional J-Core SMP-support patches and appropriate hardware capable of running SMP.
A nanosecond-resolution clocksource is provided using the J-Core "RTC" registers, which give a 64-bit seconds count and 32-bit nanoseconds. The driver converts these to a 64-bit nanoseconds count.
--- drivers/clocksource/Kconfig | 8 ++ drivers/clocksource/Makefile | 1 + drivers/clocksource/jcore-pit.c | 283 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 292 insertions(+) create mode 100644 drivers/clocksource/jcore-pit.c
drivers/clocksource/Kconfig +8 -0
@@ -313,6 +313,14 @@ config SYS_SUPPORTS_SH_TMU config SYS_SUPPORTS_EM_STI bool +config CLKSRC_JCORE_PIT + bool "J-Core PIT timer driver" + depends on GENERIC_CLOCKEVENTS + depends on HAS_IOMEM + 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 +283 -0
283 lines not on this page · raw message
Signature · 1 line
2.8.1
Re: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver
drivers/clocksource/jcore-pit.c line 25
Quoted 6 lines
drivers/clocksource/jcore-pit.c new file +6
-0+ +#define PIT_IRQ_SHIFT 12 +#define PIT_PRIO_SHIFT 20 +#define PIT_ENABLE_SHIFT 26 +#define PIT_IRQ_MASK 0x3f +#define PIT_PRIO_MASK 0xf
Can you please align the numbers as nicely as you did below and please use TABS not spaces to seperate them.
Quoted 10 lines
drivers/clocksource/jcore-pit.c new file +10
-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_clocksource { +struct clocksource cs;
drivers/clocksource/jcore-pit.c line 46
Quoted 10 lines
drivers/clocksource/jcore-pit.c new file +10
-0+__iomem void *base; +}; + +struct jcore_pit { +struct clock_event_device ced; +__iomem void *base; +unsigned long periodic_delta; +unsigned cpu; +u32 enable_val; +};
Again. Please align the struct members as I asked in the other mail.
drivers/clocksource/jcore-pit.c line 59
Quoted 13 lines
drivers/clocksource/jcore-pit.c new file +13
-0+ +struct jcore_pit_nb { +struct notifier_block nb; +struct jcore_pit __percpu *pit_percpu; +}; + +static struct clocksource *jcore_cs; + +static cycle_t jcore_clocksource_read (struct clocksource *cs) +{ +__iomem void *base = + container_of(cs, struct jcore_clocksource, cs)->base; +u32 sechi, seclo, nsec, sechi0, seclo0;
It would be way simpler to read if you avoid that line break by doing:
u32 sechi, seclo, nsec, sechi0, seclo0; __iomem void *base; base = container_of(cs, struct jcore_clocksource, cs)->base;
Hmm?
drivers/clocksource/jcore-pit.c line 71
Quoted 12 lines
drivers/clocksource/jcore-pit.c new file +12
-0+ + sechi =__raw_readl (base +REG_SECHI ); + seclo =__raw_readl (base +REG_SECLO ); +do { + sechi0 = sechi; + seclo0 = seclo; + nsec =__raw_readl (base +REG_NSEC ); + sechi =__raw_readl (base +REG_SECHI ); + seclo =__raw_readl (base +REG_SECLO ); + }while (sechi0 != sechi || seclo0 != seclo); + +return ((u64 )sechi <<32 | seclo) *NSEC_PER_SEC + nsec;
Wow, that's an expensive thing for a hotpath operation. You really don't have binary readout register for that clock thingy?
drivers/clocksource/jcore-pit.c line 76
Quoted 5 lines
drivers/clocksource/jcore-pit.c new file +5
-0+} + +static notrace u64 jcore_sched_clock_read(void) +{ +return jcore_clocksource_read (jcore_cs);
Why don't you stuff the above code into this function?
Quoted 9 lines
drivers/clocksource/jcore-pit.c new file +9
-0+} + +static int jcore_pit_disable (struct jcore_pit *pit) +{ +__raw_writel (0 , pit->base +REG_PITEN ); +return 0 ; +} + +static int jcore_pit_set (unsigned long delta,struct jcore_pit *pit)
drivers/clocksource/jcore-pit.c line 95
Quoted 10 lines
drivers/clocksource/jcore-pit.c new file +10
-0+{ +jcore_pit_disable (pit); +__raw_writel (delta, pit->base +REG_THROT ); +__raw_writel (pit->enable_val, pit->base +REG_PITEN ); +return 0 ; +} + +static int jcore_pit_set_state_shutdown (struct clock_event_device *ced) +{ + struct jcore_pit *pit = container_of(ced, struct jcore_pit, ced);
Newline between declaration and code please....
drivers/clocksource/jcore-pit.c line 142
Quoted 13 lines
drivers/clocksource/jcore-pit.c new file +12
-0+return jcore_pit_disable (pit); +} +static int jcore_pit_cpu_notify (struct notifier_block *self, +unsigned long action,void *hcpu) +{ + struct jcore_pit_nb *nb = container_of(self, struct jcore_pit_nb, nb); +switch (action & ~CPU_TASKS_FROZEN ) { +case CPU_STARTING : +jcore_pit_local_init (this_cpu_ptr (nb->pit_percpu)); +break ; + } +return NOTIFY_OK ;
Please convert this to the new state machine model of cpu hotplug. CPU_STARTING will be gone very soon. Here is an example:
drivers/clocksource/jcore-pit.c line 162
Quoted 6 lines
drivers/clocksource/jcore-pit.c new file +6
-0+static int __init jcore_pit_init (struct device_node *node) +{ +int err; +__iomem void *pit_base; +unsigned pit_irq; +unsigned cpu;
Please put the same data types into a single line
drivers/clocksource/jcore-pit.c line 163
Quoted 1 line
drivers/clocksource/jcore-pit.c new file +1
-0+struct jcore_pit_nb *nb =0 ;
= NULL;
Please
drivers/clocksource/jcore-pit.c line 233
Quoted 5 lines
drivers/clocksource/jcore-pit.c new file +5
-0+struct jcore_clocksource *cs =0 ; +struct jcore_pit __percpu *pit_percpu =0 ; + + err =request_irq (pit_irq, jcore_timer_interrupt, +IRQF_TIMER |IRQF_PERCPU ,"jcore_pit" , pit_percpu);
If you need line breaks, then please keep the arguments aligned:
err = request_irq(pit_irq, jcore_timer_interrupt, IRQF_TIMER | IRQF_PERCPU, "jcore_pit", pit_percpu);
drivers/clocksource/jcore-pit.c line 244
Quoted 6 lines
drivers/clocksource/jcore-pit.c new file +6
-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 +* 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. */
Proper multiline comments please
drivers/clocksource/jcore-pit.c line 279
Quoted 4 lines
drivers/clocksource/jcore-pit.c new file +4
-0+return 0 ; + +out: +pr_err ("Could not initialize J-Core PIT driver\n" );
So you have a pr_err in every error path and that goto just to pr_err the obvious? Well, yes ...
Thanks,
tglx
--
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
maintainerRe: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 9 lines · Thomas Gleixner
drivers/clocksource/jcore-pit.c line 25
Quoted 6 lines
drivers/clocksource/jcore-pit.c new file +6
-0+ +#define PIT_IRQ_SHIFT 12 +#define PIT_PRIO_SHIFT 20 +#define PIT_ENABLE_SHIFT 26 +#define PIT_IRQ_MASK 0x3f +#define PIT_PRIO_MASK 0xfCan you please align the numbers as nicely as you did below and please use TABS not spaces to seperate them.
OK.
Quoted 15 lines
Quoted 15 lines
drivers/clocksource/jcore-pit.c new file +14
-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_clocksource { +struct clocksource cs; +__iomem void *base; +}; + +struct jcore_pit {
Quoted 8 lines
drivers/clocksource/jcore-pit.c line 46
Quoted 6 lines
drivers/clocksource/jcore-pit.c new file +6
-0+struct clock_event_device ced; +__iomem void *base; +unsigned long periodic_delta; +unsigned cpu; +u32 enable_val; +};Again. Please align the struct members as I asked in the other mail.
OK.
Quoted 16 lines
drivers/clocksource/jcore-pit.c line 59
Quoted 13 lines
drivers/clocksource/jcore-pit.c new file +13
-0+ +struct jcore_pit_nb { +struct notifier_block nb; +struct jcore_pit __percpu *pit_percpu; +}; + +static struct clocksource *jcore_cs; + +static cycle_t jcore_clocksource_read (struct clocksource *cs) +{ +__iomem void *base = + container_of(cs, struct jcore_clocksource, cs)->base; +u32 sechi, seclo, nsec, sechi0, seclo0;It would be way simpler to read if you avoid that line break by doing:
Quoted 6 lines
u32 sechi, seclo, nsec, sechi0, seclo0; __iomem void *base; base = container_of(cs, struct jcore_clocksource, cs)->base;Hmm?
Yes, I agree.
Quoted 7 lines
Quoted 7 lines
drivers/clocksource/jcore-pit.c new file +7
-0+ + sechi =__raw_readl (base +REG_SECHI ); + seclo =__raw_readl (base +REG_SECLO ); + do { + sechi0 = sechi; + seclo0 = seclo; + nsec = __raw_readl(base + REG_NSEC);
Quoted 8 lines
drivers/clocksource/jcore-pit.c line 71
Quoted 5 lines
drivers/clocksource/jcore-pit.c new file +5
-0+ sechi =__raw_readl (base +REG_SECHI ); + seclo =__raw_readl (base +REG_SECLO ); + }while (sechi0 != sechi || seclo0 != seclo); + +return ((u64 )sechi <<32 | seclo) *NSEC_PER_SEC + nsec;Wow, that's an expensive thing for a hotpath operation. You really don't have binary readout register for that clock thingy?
Unforunately the clock is in sec64.nsec32 format instead of a flat nanoseconds count. Daniel Lezcano also suggested just using nanoseconds, which would still need some retry and arithmetic for adding seclo*NSEC_PER_SEC (otherwise it's problematic because it wraps at NSEC_PER_SEC rather than at a power of two) but that does put a hard upper bound on tickless sleep time of 4 sec. In practice it probably doesn't matter. Should I try that instead?
Quoted 5 lines
drivers/clocksource/jcore-pit.c line 76
Quoted 3 lines
drivers/clocksource/jcore-pit.c new file +3
-0+static notrace u64 jcore_sched_clock_read(void) +{ +return jcore_clocksource_read (jcore_cs);Why don't you stuff the above code into this function?
I was trying to avoid code duplication, but I could if you think it really matters for performance.
Quoted 11 lines
Quoted 11 lines
drivers/clocksource/jcore-pit.c new file +11
-0+static int jcore_pit_disable (struct jcore_pit *pit) +{ +__raw_writel (0 , pit->base +REG_PITEN ); +return 0 ; +} + +static int jcore_pit_set (unsigned long delta,struct jcore_pit *pit) +{ +jcore_pit_disable (pit); +__raw_writel (delta, pit->base +REG_THROT ); +__raw_writel (pit->enable_val, pit->base +REG_PITEN );
Quoted 8 lines
drivers/clocksource/jcore-pit.c line 95
Quoted 6 lines
drivers/clocksource/jcore-pit.c new file +6
-0+return 0 ; +} + +static int jcore_pit_set_state_shutdown (struct clock_event_device *ced) +{ + struct jcore_pit *pit = container_of(ced, struct jcore_pit, ced);Newline between declaration and code please....
OK.
Quoted 14 lines
drivers/clocksource/jcore-pit.c line 142
Quoted 10 lines
drivers/clocksource/jcore-pit.c new file +10
-0+static int jcore_pit_cpu_notify (struct notifier_block *self, +unsigned long action,void *hcpu) +{ + struct jcore_pit_nb *nb = container_of(self, struct jcore_pit_nb, nb); +switch (action & ~CPU_TASKS_FROZEN ) { +case CPU_STARTING : +jcore_pit_local_init (this_cpu_ptr (nb->pit_percpu)); +break ; + } +return NOTIFY_OK ;Please convert this to the new state machine model of cpu hotplug.
CPU_STARTINGwill be gone very soon. Here is an example:
I don't think that's the commit you wanted to link -- it doesn't show a usage example.
I've asked about this before in another context but didn't get an answer -- I'm a bit concerned that, from what I can tell, the new framework is a big singleton does assumes singletons in all the drivers that use it. In practice it doesn't matter as long as there's only one instance of the pit driver, but this seems architecturally really bad and like it's a time bomb waiting to blow up on us in the future. Am I missing something?
Quoted 8 lines
drivers/clocksource/jcore-pit.c line 162
Quoted 6 lines
drivers/clocksource/jcore-pit.c new file +6
-0+static int __init jcore_pit_init (struct device_node *node) +{ +int err; +__iomem void *pit_base; +unsigned pit_irq; +unsigned cpu;Please put the same data types into a single line
OK.
Quoted 5 lines
drivers/clocksource/jcore-pit.c line 163
Quoted 1 line
drivers/clocksource/jcore-pit.c new file +1
-0+struct jcore_pit_nb *nb =0 ;= NULL;Please
OK.
Quoted 7 lines
drivers/clocksource/jcore-pit.c line 233
Quoted 5 lines
drivers/clocksource/jcore-pit.c new file +5
-0+struct jcore_clocksource *cs =0 ; +struct jcore_pit __percpu *pit_percpu =0 ; + + err =request_irq (pit_irq, jcore_timer_interrupt, +IRQF_TIMER |IRQF_PERCPU ,"jcore_pit" , pit_percpu);If you need line breaks, then please keep the arguments aligned:
OK.
Quoted 11 lines
err = request_irq(pit_irq, jcore_timer_interrupt, IRQF_TIMER | IRQF_PERCPU, "jcore_pit", pit_percpu);drivers/clocksource/jcore-pit.c line 244
Quoted 6 lines
drivers/clocksource/jcore-pit.c new file +6
-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 +* 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. */ Proper multiline comments please
OK.
Quoted 7 lines
drivers/clocksource/jcore-pit.c line 279
Quoted 4 lines
drivers/clocksource/jcore-pit.c new file +4
-0+return 0 ; + +out: +pr_err ("Could not initialize J-Core PIT driver\n" );So you have a
pr_errin every error path and that goto just topr_errthe obvious? Well, yes ...
I can remove it if you prefer. I'd have to look back at how this happened but I suspect some of the individual pr_errs did not exist before.
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
Re: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 6 lines · Rich Felker
Quoted 4 lines · Thomas Gleixner
drivers/clocksource/jcore-pit.c line 71
Quoted 1 line
drivers/clocksource/jcore-pit.c new file +1
-0+return ((u64 )sechi <<32 | seclo) *NSEC_PER_SEC + nsec;Wow, that's an expensive thing for a hotpath operation. You really don't have binary readout register for that clock thingy?
Quoted 7 lines
Unforunately the clock is in sec64.nsec32 format instead of a flat nanoseconds count. Daniel Lezcano also suggested just using nanoseconds, which would still need some retry and arithmetic for adding seclo*NSEC_PER_SEC (otherwise it's problematic because it wraps at
NSEC_PER_SECrather than at a power of two) but that does put a hard upper bound on tickless sleep time of 4 sec. In practice it probably doesn't matter. Should I try that instead?
Up to you. I was just wondering about the MUL.
Quoted 8 lines
Quoted 5 lines
drivers/clocksource/jcore-pit.c line 76
Quoted 3 lines
drivers/clocksource/jcore-pit.c new file +3
-0+static notrace u64 jcore_sched_clock_read(void) +{ +return jcore_clocksource_read (jcore_cs);Why don't you stuff the above code into this function?
I was trying to avoid code duplication, but I could if you think it really matters for performance.
Did not see where the other usage site was. Must have missed that. So keep it as is and please add notrace to jcore_clocksource_read().
Quoted 7 lines
Quoted 4 lines
Please convert this to the new state machine model of cpu hotplug.
CPU_STARTINGwill be gone very soon. Here is an example:I don't think that's the commit you wanted to link -- it doesn't show a usage example.
It's the conversion of a driver from the old to the new style, So it's an example how to move your stuff to the new interface or am I missing something here?
Quoted 7 lines
I've asked about this before in another context but didn't get an answer -- I'm a bit concerned that, from what I can tell, the new framework is a big singleton does assumes singletons in all the drivers that use it. In practice it doesn't matter as long as there's only one instance of the pit driver, but this seems architecturally really bad and like it's a time bomb waiting to blow up on us in the future. Am I missing something?
Most of the users are single instance. We have a dynamic interface for the online callbacks and we might get one for the prep stage as well.
Now for the real core stuff like starting/dying we want explicit states and if a driver really is multi instance then having a private list there is not rocket science. Most of them have an instance list anyway.
Thanks,
tglx
--
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
maintainerRe: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 15 lines · Thomas Gleixner
Quoted 6 lines · Rich Felker
Quoted 4 lines · Thomas Gleixner
drivers/clocksource/jcore-pit.c line 71
Quoted 1 line
drivers/clocksource/jcore-pit.c new file +1
-0+return ((u64 )sechi <<32 | seclo) *NSEC_PER_SEC + nsec;Wow, that's an expensive thing for a hotpath operation. You really don't have binary readout register for that clock thingy?
Quoted 7 lines
Unforunately the clock is in sec64.nsec32 format instead of a flat nanoseconds count. Daniel Lezcano also suggested just using nanoseconds, which would still need some retry and arithmetic for adding seclo*NSEC_PER_SEC (otherwise it's problematic because it wraps at
NSEC_PER_SECrather than at a power of two) but that does put a hard upper bound on tickless sleep time of 4 sec. In practice it probably doesn't matter. Should I try that instead?
Quoted 1 line
Up to you. I was just wondering about the MUL.
Well 2 people have raised the issue now so I think I'll go ahead and try changing it.
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
drivers/clocksource/jcore-pit.c line 76
Quoted 3 lines
drivers/clocksource/jcore-pit.c new file +3
-0+static notrace u64 jcore_sched_clock_read(void) +{ +return jcore_clocksource_read (jcore_cs);Why don't you stuff the above code into this function?
I was trying to avoid code duplication, but I could if you think it really matters for performance.
Did not see where the other usage site was. Must have missed that. So keep it as is and please add notrace to
jcore_clocksource_read().
Since we have to assume a singleton now anyway, I'm just going to switch the caller/callee relationship the other way around and make the sched version use static objects directly. This shought be slightly faster/smaller.
Quoted 11 lines
Quoted 7 lines
Quoted 4 lines
Please convert this to the new state machine model of cpu hotplug.
CPU_STARTINGwill be gone very soon. Here is an example:I don't think that's the commit you wanted to link -- it doesn't show a usage example.
It's the conversion of a driver from the old to the new style, So it's an example how to move your stuff to the new interface or am I missing something here?
I botched the link copy and paste. Sorry for the noise. I see it now.
Quoted 8 lines
Quoted 7 lines
I've asked about this before in another context but didn't get an answer -- I'm a bit concerned that, from what I can tell, the new framework is a big singleton does assumes singletons in all the drivers that use it. In practice it doesn't matter as long as there's only one instance of the pit driver, but this seems architecturally really bad and like it's a time bomb waiting to blow up on us in the future. Am I missing something?
Quoted 6 lines
Most of the users are single instance. We have a dynamic interface for the online callbacks and we might get one for the prep stage as well.
Now for the real core stuff like starting/dying we want explicit states and if a driver really is multi instance then having a private list there is not rocket science. Most of them have an instance list anyway.
For now I'll just remove some of the dynamic allocation I added and simplify assuming a single instance. It doesn't make sense to be incurring costs for the nominal ability to have multiple instances when it doesn't actually work in the new framework (without a lot more per-driver code duplication that really should be in the framework) and when it's not needed anyway.
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
Rich Felker
maintainerRe: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver
In reply to Thomas GleixnerQuoted 14 lines · Thomas Gleixner
drivers/clocksource/jcore-pit.c line 142
Quoted 10 lines
drivers/clocksource/jcore-pit.c new file +10
-0+static int jcore_pit_cpu_notify (struct notifier_block *self, +unsigned long action,void *hcpu) +{ + struct jcore_pit_nb *nb = container_of(self, struct jcore_pit_nb, nb); +switch (action & ~CPU_TASKS_FROZEN ) { +case CPU_STARTING : +jcore_pit_local_init (this_cpu_ptr (nb->pit_percpu)); +break ; + } +return NOTIFY_OK ;Please convert this to the new state machine model of cpu hotplug.
CPU_STARTINGwill be gone very soon. Here is an example:
Trying this change now and I'm getting hangs inside cpuhp_invoke_ap_callback between wake_up_process and wait_for_completion. I suspect it's not possible to run a scheduled task yet because there's no timer yet, or something like that. Do I need further infrastructure from tip that's not upstream yet in order to test this? I just rebased on linus/master and still have the same problem, but none of the other driver changes are in Linus's tree yet. Following the other drivers in tip (again, I don't have any of these in my tree), I put the new state between CPUHP_AP_SCHED_STARTING and CPUHP_AP_NOTIFY_STARTING; is this correct?
Rich
Rich Felker
maintainerRe: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 17 lines · Rich Felker
Quoted 14 lines · Thomas Gleixner
drivers/clocksource/jcore-pit.c line 142
Quoted 10 lines
drivers/clocksource/jcore-pit.c new file +10
-0+static int jcore_pit_cpu_notify (struct notifier_block *self, +unsigned long action,void *hcpu) +{ + struct jcore_pit_nb *nb = container_of(self, struct jcore_pit_nb, nb); +switch (action & ~CPU_TASKS_FROZEN ) { +case CPU_STARTING : +jcore_pit_local_init (this_cpu_ptr (nb->pit_percpu)); +break ; + } +return NOTIFY_OK ;Please convert this to the new state machine model of cpu hotplug.
CPU_STARTINGwill be gone very soon. Here is an example:
Quoted 10 lines
Trying this change now and I'm getting hangs inside
cpuhp_invoke_ap_callbackbetweenwake_up_processandwait_for_completion. I suspect it's not possible to run a scheduled task yet because there's no timer yet, or something like that. Do I need further infrastructure from tip that's not upstream yet in order to test this? I just rebased on linus/master and still have the same problem, but none of the other driver changes are in Linus's tree yet. Following the other drivers in tip (again, I don't have any of these in my tree), I put the new state betweenCPUHP_AP_SCHED_STARTINGandCPUHP_AP_NOTIFY_STARTING; is this correct?
I think it's this bug where the fix is not yet in Linus's tree:
Cherry-picking that made it work. FWIW, it's really hard to do development on top of new infrastructure that's not yet working in any tree that's intended for others to pull, but I made it work and I'll have a new version of the patch for you soon.
Rich
Rich Felker
maintainerRe: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver
Quoted 29 lines · Rich Felker
Quoted 17 lines · Rich Felker
Quoted 14 lines · Thomas Gleixner
drivers/clocksource/jcore-pit.c line 142
Quoted 10 lines
drivers/clocksource/jcore-pit.c new file +10
-0+static int jcore_pit_cpu_notify (struct notifier_block *self, +unsigned long action,void *hcpu) +{ + struct jcore_pit_nb *nb = container_of(self, struct jcore_pit_nb, nb); +switch (action & ~CPU_TASKS_FROZEN ) { +case CPU_STARTING : +jcore_pit_local_init (this_cpu_ptr (nb->pit_percpu)); +break ; + } +return NOTIFY_OK ;Please convert this to the new state machine model of cpu hotplug.
CPU_STARTINGwill be gone very soon. Here is an example:Quoted 10 lines
Trying this change now and I'm getting hangs inside
cpuhp_invoke_ap_callbackbetweenwake_up_processandwait_for_completion. I suspect it's not possible to run a scheduled task yet because there's no timer yet, or something like that. Do I need further infrastructure from tip that's not upstream yet in order to test this? I just rebased on linus/master and still have the same problem, but none of the other driver changes are in Linus's tree yet. Following the other drivers in tip (again, I don't have any of these in my tree), I put the new state betweenCPUHP_AP_SCHED_STARTINGandCPUHP_AP_NOTIFY_STARTING; is this correct?
Quoted 8 lines
I think it's this bug where the fix is not yet in Linus's tree:
Cherry-picking that made it work. FWIW, it's really hard to do development on top of new infrastructure that's not yet working in any tree that's intended for others to pull, but I made it work and I'll have a new version of the patch for you soon.
I hit some more snags with rcu_sched stalls that I thought were my fault, but it looks like it's actually something in the linus/master I had to rebase on (I've tagged the revision locally in case this is something we need to track later). They happen equally with or without my latest changes for the notify_nb -> cpuhp transition and other requested changes to the pit driver.
Rich