| Message ID | 20161221064603.11830-11-fu.wei@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Dec 21, 2016 at 02:45:58PM +0800, fu.wei@linaro.org wrote: > From: Fu Wei <fu.wei@linaro.org> > > The patch refactor original memory-mapped timer init code: > (1) Refactor "arch_timer_mem_init", make it become a common code for > memory-mapped timer init. > (2) Add a new function "arch_timer_mem_of_init" for DT init. As a general note, please write proper commit messages, describing what the problem is, and why we are making the changes. These bullet points don't add anything to what can be derived from a glance at the code. For this patch, you can use: clocksource: arm_arch_timer: refactor MMIO timer probing Currently the code to probe MMIO architected timers mixes DT parsing with actual poking of hardware. This makes the code harder than necessary to understand, and makes it difficult to add support for probing via ACPI. This patch factors all the DT-specific logic out of arch_timer_mem_init(), into a new function, arch_timer_mem_of_init(). The former pokes the hardware and determines the suitablility of frames based on a datastructure populated by the latter. This cleanly separates the two and will make it possible to add probing using the ACPI GTDT in subsequent patches. [...] > + for_each_available_child_of_node(np, frame_node) { > + int n; > + struct arch_timer_mem_frame *frame = &timer_mem->frame[i]; > + > + if (of_property_read_u32(frame_node, "frame-number", &n)) { > + pr_err("Missing frame-number\n"); > + of_node_put(frame_node); > + goto out; > + } > + frame->frame_nr = n; > + > + if (of_address_to_resource(frame_node, 0, &res)) { > + of_node_put(frame_node); > + goto out; > + } > + frame->cntbase = res.start; > + frame->size = resource_size(&res); > + > + frame->virt_irq = irq_of_parse_and_map(frame_node, > + ARCH_TIMER_VIRT_SPI); > + frame->phys_irq = irq_of_parse_and_map(frame_node, > + ARCH_TIMER_PHYS_SPI); > > - if (!arch_timer_needs_of_probing()) > + if (++i >= ARCH_TIMER_MEM_MAX_FRAMES) > + break; > + } It would be good if we could warn upon seeing more than ARCH_TIMER_MEM_MAX_FRAMES children, since that's obviously an error. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On 17 January 2017 at 02:30, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Dec 21, 2016 at 02:45:58PM +0800, fu.wei@linaro.org wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> The patch refactor original memory-mapped timer init code: >> (1) Refactor "arch_timer_mem_init", make it become a common code for >> memory-mapped timer init. >> (2) Add a new function "arch_timer_mem_of_init" for DT init. > > As a general note, please write proper commit messages, describing what > the problem is, and why we are making the changes. These bullet points > don't add anything to what can be derived from a glance at the code. > > For this patch, you can use: > > clocksource: arm_arch_timer: refactor MMIO timer probing > > Currently the code to probe MMIO architected timers mixes DT parsing > with actual poking of hardware. This makes the code harder than > necessary to understand, and makes it difficult to add support for > probing via ACPI. > > This patch factors all the DT-specific logic out of > arch_timer_mem_init(), into a new function, arch_timer_mem_of_init(). > The former pokes the hardware and determines the suitablility of > frames based on a datastructure populated by the latter. > > This cleanly separates the two and will make it possible to add > probing using the ACPI GTDT in subsequent patches. Great thanks for this upstream tip. I have used your example commit message instead. It will be in v20. > > [...] > >> + for_each_available_child_of_node(np, frame_node) { >> + int n; >> + struct arch_timer_mem_frame *frame = &timer_mem->frame[i]; >> + >> + if (of_property_read_u32(frame_node, "frame-number", &n)) { >> + pr_err("Missing frame-number\n"); >> + of_node_put(frame_node); >> + goto out; >> + } >> + frame->frame_nr = n; >> + >> + if (of_address_to_resource(frame_node, 0, &res)) { >> + of_node_put(frame_node); >> + goto out; >> + } >> + frame->cntbase = res.start; >> + frame->size = resource_size(&res); >> + >> + frame->virt_irq = irq_of_parse_and_map(frame_node, >> + ARCH_TIMER_VIRT_SPI); >> + frame->phys_irq = irq_of_parse_and_map(frame_node, >> + ARCH_TIMER_PHYS_SPI); >> >> - if (!arch_timer_needs_of_probing()) >> + if (++i >= ARCH_TIMER_MEM_MAX_FRAMES) >> + break; >> + } > > It would be good if we could warn upon seeing more than > ARCH_TIMER_MEM_MAX_FRAMES children, since that's obviously an error. OK, NP, will use if (i >= ARCH_TIMER_MEM_MAX_FRAMES) { pr_err(FW_BUG "too many frames, ARMv8 spec only allows 8.\n"); goto out; } at the beginning of this loop. Here will be replaced by i++; Great thanks for your suggestion! > > Thanks, > Mark.
Hi Mark, On 17 January 2017 at 18:30, Fu Wei <fu.wei@linaro.org> wrote: > Hi Mark, > > On 17 January 2017 at 02:30, Mark Rutland <mark.rutland@arm.com> wrote: >> On Wed, Dec 21, 2016 at 02:45:58PM +0800, fu.wei@linaro.org wrote: >>> From: Fu Wei <fu.wei@linaro.org> >>> >>> The patch refactor original memory-mapped timer init code: >>> (1) Refactor "arch_timer_mem_init", make it become a common code for >>> memory-mapped timer init. >>> (2) Add a new function "arch_timer_mem_of_init" for DT init. >> >> As a general note, please write proper commit messages, describing what >> the problem is, and why we are making the changes. These bullet points >> don't add anything to what can be derived from a glance at the code. >> >> For this patch, you can use: >> >> clocksource: arm_arch_timer: refactor MMIO timer probing >> >> Currently the code to probe MMIO architected timers mixes DT parsing >> with actual poking of hardware. This makes the code harder than >> necessary to understand, and makes it difficult to add support for >> probing via ACPI. >> >> This patch factors all the DT-specific logic out of >> arch_timer_mem_init(), into a new function, arch_timer_mem_of_init(). >> The former pokes the hardware and determines the suitablility of >> frames based on a datastructure populated by the latter. >> >> This cleanly separates the two and will make it possible to add >> probing using the ACPI GTDT in subsequent patches. > > Great thanks for this upstream tip. > I have used your example commit message instead. > It will be in v20. > >> >> [...] >> >>> + for_each_available_child_of_node(np, frame_node) { >>> + int n; >>> + struct arch_timer_mem_frame *frame = &timer_mem->frame[i]; >>> + >>> + if (of_property_read_u32(frame_node, "frame-number", &n)) { >>> + pr_err("Missing frame-number\n"); >>> + of_node_put(frame_node); >>> + goto out; >>> + } >>> + frame->frame_nr = n; >>> + >>> + if (of_address_to_resource(frame_node, 0, &res)) { >>> + of_node_put(frame_node); >>> + goto out; >>> + } >>> + frame->cntbase = res.start; >>> + frame->size = resource_size(&res); >>> + >>> + frame->virt_irq = irq_of_parse_and_map(frame_node, >>> + ARCH_TIMER_VIRT_SPI); >>> + frame->phys_irq = irq_of_parse_and_map(frame_node, >>> + ARCH_TIMER_PHYS_SPI); >>> >>> - if (!arch_timer_needs_of_probing()) >>> + if (++i >= ARCH_TIMER_MEM_MAX_FRAMES) >>> + break; >>> + } >> >> It would be good if we could warn upon seeing more than >> ARCH_TIMER_MEM_MAX_FRAMES children, since that's obviously an error. > > OK, NP, will use > > if (i >= ARCH_TIMER_MEM_MAX_FRAMES) { > pr_err(FW_BUG "too many frames, ARMv8 spec only allows 8.\n"); Sorry, this should be "ARM spec only allows 8.\n" Not only ARMv8, but also ARMv7 > goto out; > } > > at the beginning of this loop. > > Here will be replaced by i++; > > Great thanks for your suggestion! > >> >> Thanks, >> Mark. > > > > -- > Best regards, > > Fu Wei > Software Engineer > Red Hat
Fu Wei wrote: > if (i >= ARCH_TIMER_MEM_MAX_FRAMES) { > pr_err(FW_BUG "too many frames, ARMv8 spec only allows 8.\n"); pr_err(FW_BUG "too many frames, ARMv8 spec only allows %u.\n", ARCH_TIMER_MEM_MAX_FRAMES);
On Tue, Jan 17, 2017 at 06:18:12AM -0600, Timur Tabi wrote: > Fu Wei wrote: > >if (i >= ARCH_TIMER_MEM_MAX_FRAMES) { > > pr_err(FW_BUG "too many frames, ARMv8 spec only allows 8.\n"); > > pr_err(FW_BUG "too many frames, ARMv8 spec only allows %u.\n", > ARCH_TIMER_MEM_MAX_FRAMES); While I don't see ARCH_TIMER_MEM_MAX_FRAMES changing, this would be nicer to ensure the result obviously matches. As for wording, I'd perfer: pr_err(FW_BUG "too many frames, only %u are permitted.\n", ARCH_TIMER_MEM_MAX_FRAMES); ... so as to avoid any confusion between spec versions and so on. We can reconsider the message if/when that changes. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On 17 January 2017 at 20:29, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Jan 17, 2017 at 06:18:12AM -0600, Timur Tabi wrote: >> Fu Wei wrote: >> >if (i >= ARCH_TIMER_MEM_MAX_FRAMES) { >> > pr_err(FW_BUG "too many frames, ARMv8 spec only allows 8.\n"); >> >> pr_err(FW_BUG "too many frames, ARMv8 spec only allows %u.\n", >> ARCH_TIMER_MEM_MAX_FRAMES); > > While I don't see ARCH_TIMER_MEM_MAX_FRAMES changing, this would be > nicer to ensure the result obviously matches. > > As for wording, I'd perfer: > > pr_err(FW_BUG "too many frames, only %u are permitted.\n", > ARCH_TIMER_MEM_MAX_FRAMES); OK, will do so. Thanks! > > ... so as to avoid any confusion between spec versions and so on. We can > reconsider the message if/when that changes. > > Thanks, > Mark.
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 9113df8..b10cc26 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -922,17 +922,17 @@ static int __init arch_timer_of_init(struct device_node *np) CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_of_init); CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_of_init); -static int __init arch_timer_mem_init(struct device_node *np) +static int __init arch_timer_mem_init(struct arch_timer_mem *timer_mem) { - struct device_node *frame, *best_frame = NULL; void __iomem *cntctlbase, *base; - unsigned int irq, ret = -EINVAL; + struct arch_timer_mem_frame *best_frame = NULL; + unsigned int irq; u32 cnttidr; + int i, ret; - arch_timers_present |= ARCH_TIMER_TYPE_MEM; - cntctlbase = of_iomap(np, 0); + cntctlbase = ioremap(timer_mem->cntctlbase, timer_mem->size); if (!cntctlbase) { - pr_err("Can't find CNTCTLBase\n"); + pr_err("Can't map CNTCTLBase.\n"); return -ENXIO; } @@ -942,26 +942,18 @@ static int __init arch_timer_mem_init(struct device_node *np) * Try to find a virtual capable frame. Otherwise fall back to a * physical capable frame. */ - for_each_available_child_of_node(np, frame) { - int n; - u32 cntacr; - - if (of_property_read_u32(frame, "frame-number", &n)) { - pr_err("Missing frame-number\n"); - of_node_put(frame); - goto out; - } + for (i = 0; i < timer_mem->num_frames; i++) { + u32 cntacr = CNTACR_RFRQ | CNTACR_RWPT | CNTACR_RPCT | + CNTACR_RWVT | CNTACR_RVOFF | CNTACR_RVCT; + int n = timer_mem->frame[i].frame_nr; /* Try enabling everything, and see what sticks */ - cntacr = CNTACR_RFRQ | CNTACR_RWPT | CNTACR_RPCT | - CNTACR_RWVT | CNTACR_RVOFF | CNTACR_RVCT; writel_relaxed(cntacr, cntctlbase + CNTACR(n)); cntacr = readl_relaxed(cntctlbase + CNTACR(n)); if ((cnttidr & CNTTIDR_VIRT(n)) && !(~cntacr & (CNTACR_RWVT | CNTACR_RVCT))) { - of_node_put(best_frame); - best_frame = frame; + best_frame = &timer_mem->frame[i]; arch_timer_mem_use_virtual = true; break; } @@ -969,51 +961,108 @@ static int __init arch_timer_mem_init(struct device_node *np) if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT)) continue; - of_node_put(best_frame); - best_frame = of_node_get(frame); + best_frame = &timer_mem->frame[i]; } + iounmap(cntctlbase); - ret= -ENXIO; - base = arch_counter_base = of_io_request_and_map(best_frame, 0, - "arch_mem_timer"); - if (IS_ERR(base)) { - pr_err("Can't map frame's registers\n"); - goto out; + if (!best_frame) { + pr_err("Can't find frame for register\n"); + return -EINVAL; } if (arch_timer_mem_use_virtual) - irq = irq_of_parse_and_map(best_frame, ARCH_TIMER_VIRT_SPI); + irq = best_frame->virt_irq; else - irq = irq_of_parse_and_map(best_frame, ARCH_TIMER_PHYS_SPI); + irq = best_frame->phys_irq; - ret = -EINVAL; if (!irq) { pr_err("Frame missing %s irq.\n", arch_timer_mem_use_virtual ? "virt" : "phys"); - goto out; + return -EINVAL; } - /* - * Try to determine the frequency from the device tree, - * if fail, get the frequency from CNTFRQ. - */ - if (!arch_timer_rate && - of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) - arch_timer_mem_detect_rate(base); + if (!request_mem_region(best_frame->cntbase, best_frame->size, + "arch_mem_timer")) + return -EBUSY; + + base = ioremap(best_frame->cntbase, best_frame->size); + if (!base) { + pr_err("Can't map frame's registers\n"); + return -ENXIO; + } + + arch_timer_mem_detect_rate(base); ret = arch_timer_mem_register(base, irq); - if (ret) + if (ret) { + iounmap(base); + return ret; + } + + arch_counter_base = base; + arch_timers_present |= ARCH_TIMER_TYPE_MEM; + + return 0; +} + +static int __init arch_timer_mem_of_init(struct device_node *np) +{ + struct arch_timer_mem *timer_mem; + struct device_node *frame_node; + struct resource res; + int i, ret = -EINVAL; + + timer_mem = kzalloc(sizeof(*timer_mem), GFP_KERNEL); + if (!timer_mem) + return -ENOMEM; + + if (of_address_to_resource(np, 0, &res)) goto out; + timer_mem->cntctlbase = res.start; + timer_mem->size = resource_size(&res); + + i = 0; + for_each_available_child_of_node(np, frame_node) { + int n; + struct arch_timer_mem_frame *frame = &timer_mem->frame[i]; + + if (of_property_read_u32(frame_node, "frame-number", &n)) { + pr_err("Missing frame-number\n"); + of_node_put(frame_node); + goto out; + } + frame->frame_nr = n; + + if (of_address_to_resource(frame_node, 0, &res)) { + of_node_put(frame_node); + goto out; + } + frame->cntbase = res.start; + frame->size = resource_size(&res); + + frame->virt_irq = irq_of_parse_and_map(frame_node, + ARCH_TIMER_VIRT_SPI); + frame->phys_irq = irq_of_parse_and_map(frame_node, + ARCH_TIMER_PHYS_SPI); - if (!arch_timer_needs_of_probing()) + if (++i >= ARCH_TIMER_MEM_MAX_FRAMES) + break; + } + timer_mem->num_frames = i; + + /* Try to determine the frequency from the device tree */ + if (!arch_timer_rate) + of_property_read_u32(np, "clock-frequency", &arch_timer_rate); + + ret = arch_timer_mem_init(timer_mem); + if (!ret && !arch_timer_needs_of_probing()) ret = arch_timer_common_init(); out: - iounmap(cntctlbase); - of_node_put(best_frame); + kfree(timer_mem); return ret; } CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem", - arch_timer_mem_init); + arch_timer_mem_of_init); #ifdef CONFIG_ACPI static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)