From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fu Wei Subject: Re: [PATCH v19 10/15] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT Date: Tue, 17 Jan 2017 18:39:18 +0800 Message-ID: References: <20161221064603.11830-1-fu.wei@linaro.org> <20161221064603.11830-11-fu.wei@linaro.org> <20170116183032.GL5908@leverpostej> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: Sender: linux-watchdog-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Mark Rutland Cc: "Rafael J. Wysocki" , Len Brown , Daniel Lezcano , Thomas Gleixner , Marc Zyngier , Lorenzo Pieralisi , Sudeep Holla , Hanjun Guo , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Linaro ACPI Mailman List , Linux Kernel Mailing List , ACPI Devel Maling List , rruigrok-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, "Abdulhamid, Harb" , Christopher Covington , Timur Tabi , G Gregory , Al Stone , Jon Masters List-Id: linux-acpi@vger.kernel.org Hi Mark, On 17 January 2017 at 18:30, Fu Wei wrote: > Hi Mark, > > On 17 January 2017 at 02:30, Mark Rutland wrote: >> On Wed, Dec 21, 2016 at 02:45:58PM +0800, fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote: >>> From: Fu Wei >>> >>> 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 -- Best regards, Fu Wei Software Engineer Red Hat -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751704AbdAQKsT (ORCPT ); Tue, 17 Jan 2017 05:48:19 -0500 Received: from mail-io0-f182.google.com ([209.85.223.182]:35626 "EHLO mail-io0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751673AbdAQKsQ (ORCPT ); Tue, 17 Jan 2017 05:48:16 -0500 MIME-Version: 1.0 In-Reply-To: References: <20161221064603.11830-1-fu.wei@linaro.org> <20161221064603.11830-11-fu.wei@linaro.org> <20170116183032.GL5908@leverpostej> From: Fu Wei Date: Tue, 17 Jan 2017 18:39:18 +0800 Message-ID: Subject: Re: [PATCH v19 10/15] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT To: Mark Rutland Cc: "Rafael J. Wysocki" , Len Brown , Daniel Lezcano , Thomas Gleixner , Marc Zyngier , Lorenzo Pieralisi , Sudeep Holla , Hanjun Guo , linux-arm-kernel@lists.infradead.org, Linaro ACPI Mailman List , Linux Kernel Mailing List , ACPI Devel Maling List , rruigrok@codeaurora.org, "Abdulhamid, Harb" , Christopher Covington , Timur Tabi , G Gregory , Al Stone , Jon Masters , Wei Huang , Arnd Bergmann , Catalin Marinas , Will Deacon , Suravee Suthikulpanit , Leo Duran , Wim Van Sebroeck , Guenter Roeck , linux-watchdog@vger.kernel.org, Tomasz Nowicki , Christoffer Dall , Julien Grall Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mark, On 17 January 2017 at 18:30, Fu Wei wrote: > Hi Mark, > > On 17 January 2017 at 02:30, Mark Rutland wrote: >> On Wed, Dec 21, 2016 at 02:45:58PM +0800, fu.wei@linaro.org wrote: >>> From: Fu Wei >>> >>> 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 -- Best regards, Fu Wei Software Engineer Red Hat From mboxrd@z Thu Jan 1 00:00:00 1970 From: fu.wei@linaro.org (Fu Wei) Date: Tue, 17 Jan 2017 18:39:18 +0800 Subject: [PATCH v19 10/15] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT In-Reply-To: References: <20161221064603.11830-1-fu.wei@linaro.org> <20161221064603.11830-11-fu.wei@linaro.org> <20170116183032.GL5908@leverpostej> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Mark, On 17 January 2017 at 18:30, Fu Wei wrote: > Hi Mark, > > On 17 January 2017 at 02:30, Mark Rutland wrote: >> On Wed, Dec 21, 2016 at 02:45:58PM +0800, fu.wei at linaro.org wrote: >>> From: Fu Wei >>> >>> 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 -- Best regards, Fu Wei Software Engineer Red Hat