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:30:04 +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: Received: from mail-it0-f47.google.com ([209.85.214.47]:38414 "EHLO mail-it0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751115AbdAQKaF (ORCPT ); Tue, 17 Jan 2017 05:30:05 -0500 Received: by mail-it0-f47.google.com with SMTP id c7so39234420itd.1 for ; Tue, 17 Jan 2017 02:30:05 -0800 (PST) In-Reply-To: <20170116183032.GL5908@leverpostej> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.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@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 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"); 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751334AbdAQKaI (ORCPT ); Tue, 17 Jan 2017 05:30:08 -0500 Received: from mail-it0-f46.google.com ([209.85.214.46]:38414 "EHLO mail-it0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751159AbdAQKaF (ORCPT ); Tue, 17 Jan 2017 05:30:05 -0500 MIME-Version: 1.0 In-Reply-To: <20170116183032.GL5908@leverpostej> 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:30:04 +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 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"); 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: fu.wei@linaro.org (Fu Wei) Date: Tue, 17 Jan 2017 18:30:04 +0800 Subject: [PATCH v19 10/15] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT In-Reply-To: <20170116183032.GL5908@leverpostej> 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 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"); 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