From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fu Wei Subject: Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver Date: Fri, 7 Apr 2017 02:07:40 +0800 Message-ID: References: <20170331175105.8370-1-fu.wei@linaro.org> <20170331175105.8370-10-fu.wei@linaro.org> <20170405183808.GB27550@leverpostej> <20170406172410.GA11871@leverpostej> <20170406175252.GB11871@leverpostej> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20170406175252.GB11871@leverpostej> 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 7 April 2017 at 01:52, Mark Rutland wrote: > On Fri, Apr 07, 2017 at 01:39:09AM +0800, Fu Wei wrote: >> On 7 April 2017 at 01:24, Mark Rutland wrote: >> > On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote: >> >> On 6 April 2017 at 02:38, Mark Rutland wrote: >> >> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote: > >> > However, I would prefer to simplify this such that we only free the >> > IRQs in the error path. >> > >> > We should be able to iterate over all freams, freeing any non-zero >> > interrupt, since !valid frames shouldn't have non-zero interrupts. >> >> Yes, that is what I am doing : >> >> if (!frame->valid) >> continue; > > What I meant was that we won't look at the frame->valid flag at all; > only the interrupts. e.g. > > for (int i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) { > if (frame->phys_irq > 0) > free_the_phys_irq_somehow(); > if (frame->virt_irq > 0) > free_the_virt_irq_somehow(); > } Since we use "kcalloc" to allocate struct arch_timer_mem, this should be OK. > > ... where we somehow figure out the GSI, or we introduce an api like > unregister_gsi_for_irq(irq). Yes, If you are OK with introducing a new API , this problem is solved :-) > > Since the !valid frames should all have zero for their interrupt fields, > no special handling is necessary. > > That way, we only free the IRQs in one place, it's obvious that we > consistently free all of them, etc. > >> Lorenzo addressed the API issue, we may can fix it by getting GSI info >> from DT, then register it until we figure the best frame. >> It may need some big change in DT code > > I'd prefer to keep this constrained to the ACPI code. ;) > > Thanks, > Mark. -- 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 S932445AbdDFSHw (ORCPT ); Thu, 6 Apr 2017 14:07:52 -0400 Received: from mail-it0-f41.google.com ([209.85.214.41]:38602 "EHLO mail-it0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752617AbdDFSHl (ORCPT ); Thu, 6 Apr 2017 14:07:41 -0400 MIME-Version: 1.0 In-Reply-To: <20170406175252.GB11871@leverpostej> References: <20170331175105.8370-1-fu.wei@linaro.org> <20170331175105.8370-10-fu.wei@linaro.org> <20170405183808.GB27550@leverpostej> <20170406172410.GA11871@leverpostej> <20170406175252.GB11871@leverpostej> From: Fu Wei Date: Fri, 7 Apr 2017 02:07:40 +0800 Message-ID: Subject: Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver 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 7 April 2017 at 01:52, Mark Rutland wrote: > On Fri, Apr 07, 2017 at 01:39:09AM +0800, Fu Wei wrote: >> On 7 April 2017 at 01:24, Mark Rutland wrote: >> > On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote: >> >> On 6 April 2017 at 02:38, Mark Rutland wrote: >> >> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu.wei@linaro.org wrote: > >> > However, I would prefer to simplify this such that we only free the >> > IRQs in the error path. >> > >> > We should be able to iterate over all freams, freeing any non-zero >> > interrupt, since !valid frames shouldn't have non-zero interrupts. >> >> Yes, that is what I am doing : >> >> if (!frame->valid) >> continue; > > What I meant was that we won't look at the frame->valid flag at all; > only the interrupts. e.g. > > for (int i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) { > if (frame->phys_irq > 0) > free_the_phys_irq_somehow(); > if (frame->virt_irq > 0) > free_the_virt_irq_somehow(); > } Since we use "kcalloc" to allocate struct arch_timer_mem, this should be OK. > > ... where we somehow figure out the GSI, or we introduce an api like > unregister_gsi_for_irq(irq). Yes, If you are OK with introducing a new API , this problem is solved :-) > > Since the !valid frames should all have zero for their interrupt fields, > no special handling is necessary. > > That way, we only free the IRQs in one place, it's obvious that we > consistently free all of them, etc. > >> Lorenzo addressed the API issue, we may can fix it by getting GSI info >> from DT, then register it until we figure the best frame. >> It may need some big change in DT code > > I'd prefer to keep this constrained to the ACPI code. ;) > > 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: Fri, 7 Apr 2017 02:07:40 +0800 Subject: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver In-Reply-To: <20170406175252.GB11871@leverpostej> References: <20170331175105.8370-1-fu.wei@linaro.org> <20170331175105.8370-10-fu.wei@linaro.org> <20170405183808.GB27550@leverpostej> <20170406172410.GA11871@leverpostej> <20170406175252.GB11871@leverpostej> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Mark, On 7 April 2017 at 01:52, Mark Rutland wrote: > On Fri, Apr 07, 2017 at 01:39:09AM +0800, Fu Wei wrote: >> On 7 April 2017 at 01:24, Mark Rutland wrote: >> > On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote: >> >> On 6 April 2017 at 02:38, Mark Rutland wrote: >> >> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu.wei at linaro.org wrote: > >> > However, I would prefer to simplify this such that we only free the >> > IRQs in the error path. >> > >> > We should be able to iterate over all freams, freeing any non-zero >> > interrupt, since !valid frames shouldn't have non-zero interrupts. >> >> Yes, that is what I am doing : >> >> if (!frame->valid) >> continue; > > What I meant was that we won't look at the frame->valid flag at all; > only the interrupts. e.g. > > for (int i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) { > if (frame->phys_irq > 0) > free_the_phys_irq_somehow(); > if (frame->virt_irq > 0) > free_the_virt_irq_somehow(); > } Since we use "kcalloc" to allocate struct arch_timer_mem, this should be OK. > > ... where we somehow figure out the GSI, or we introduce an api like > unregister_gsi_for_irq(irq). Yes, If you are OK with introducing a new API , this problem is solved :-) > > Since the !valid frames should all have zero for their interrupt fields, > no special handling is necessary. > > That way, we only free the IRQs in one place, it's obvious that we > consistently free all of them, etc. > >> Lorenzo addressed the API issue, we may can fix it by getting GSI info >> from DT, then register it until we figure the best frame. >> It may need some big change in DT code > > I'd prefer to keep this constrained to the ACPI code. ;) > > Thanks, > Mark. -- Best regards, Fu Wei Software Engineer Red Hat