From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lorenzo Pieralisi Subject: Re: [PATCH v24 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver Date: Wed, 19 Apr 2017 16:56:27 +0100 Message-ID: <20170419155627.GA9439@red-moon> References: <20170414184014.8524-1-fu.wei@linaro.org> <20170414184014.8524-10-fu.wei@linaro.org> <20170418172107.GA1401@red-moon> <20170419150725.GH27829@leverpostej> <20170419154554.GJ27829@leverpostej> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20170419154554.GJ27829@leverpostej> Sender: linux-watchdog-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Mark Rutland Cc: linaro-acpi-cunTk1MwBs8s++Sfvej+rw@public.gmane.org, catalin.marinas-5wv7dgnIgG8@public.gmane.org, will.deacon-5wv7dgnIgG8@public.gmane.org, rruigrok-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, wei-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, al.stone-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, tn-nYOzD4b6Jr9Wk0Htik3J/w@public.gmane.org, timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, lenb-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, harba-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, julien.grall-5wv7dgnIgG8@public.gmane.org, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, arnd-r2nGTMty4D4@public.gmane.org, marc.zyngier-5wv7dgnIgG8@public.gmane.org, jcm-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, cov-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, graeme.gregory-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, leo.duran-5C7GfCeVMHo@public.gmane.org, hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org, sudeep.holla-5wv7dgnIgG8@public.gmane.org, christoffer.dall-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org List-Id: linux-acpi@vger.kernel.org On Wed, Apr 19, 2017 at 04:45:54PM +0100, Mark Rutland wrote: > On Wed, Apr 19, 2017 at 04:07:25PM +0100, Mark Rutland wrote: > > On Tue, Apr 18, 2017 at 06:21:07PM +0100, Lorenzo Pieralisi wrote: > > > On Sat, Apr 15, 2017 at 02:40:12AM +0800, fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote: > > > > If yes, why can't it simply be written like this ? > > > > > > for (; i >= 0; i--, gtdt_frame--) { > > > frame = &timer_mem->frame[gtdt_frame->frame_number]; > > > > > > /* not sure this check is actually needed */ > > > if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER) > > > continue; > > > > > > if (frame->phys_irq > 0) > > > acpi_unregister_gsi(gtdt_frame->timer_interrupt); > > > if (frame->virt_irq > 0) > > > acpi_unregister_gsi(gtdt_frame->virtual_timer_interrupt); > > > } > > > > A reverse loop of this form will work. > > > > That requires some restructuring, and care to avoid going out of bounds > > instantaneously with the gtdt_frame--, so as to not invoke nasal demons. > > > > I've attacked this locally, and will send this out after testing. I'll > > drop the new ACPI API patch. > > FWIW, I've set this up so the cleanup path is: > > do { > if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER || > gtdt_frame->frame_number >= ARCH_TIMER_MEM_MAX_FRAMES) > continue; > > frame = &timer_mem->frame[gtdt_frame->frame_number]; > > if (frame->phys_irq > 0) > acpi_unregister_gsi(gtdt_frame->timer_interrupt); > frame->phys_irq = 0; > > if (frame->virt_irq > 0) > acpi_unregister_gsi(gtdt_frame->virtual_timer_interrupt); > frame->virt_irq = 0; > } while (i-- >= 0 && gtdt_frame--); > > ... the zeroing is to account for duplicate frames, which I now check for in > the probe path (as we do for DT). > > Can I take it per your comment on the prior version that with this change I can > take your ack? Yes, thanks for fixing it up, please add my: Acked-by: Lorenzo Pieralisi > I also assume that you're happy for all of the drivers/acpi/arm64/ patches in the > series to go via the clocksource tree? Yes that's how I expect them to go upstream, that's the simplest way. Thanks ! Lorenzo -- 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 S937840AbdDSPz5 (ORCPT ); Wed, 19 Apr 2017 11:55:57 -0400 Received: from foss.arm.com ([217.140.101.70]:42952 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S937792AbdDSPzx (ORCPT ); Wed, 19 Apr 2017 11:55:53 -0400 Date: Wed, 19 Apr 2017 16:56:27 +0100 From: Lorenzo Pieralisi To: Mark Rutland Cc: linaro-acpi@lists.linaro.org, catalin.marinas@arm.com, will.deacon@arm.com, rruigrok@codeaurora.org, wim@iguana.be, fu.wei@linaro.org, wei@redhat.com, al.stone@linaro.org, tn@semihalf.com, timur@codeaurora.org, daniel.lezcano@linaro.org, linux-acpi@vger.kernel.org, linux@roeck-us.net, lenb@kernel.org, harba@codeaurora.org, julien.grall@arm.com, linux-watchdog@vger.kernel.org, arnd@arndb.de, marc.zyngier@arm.com, jcm@redhat.com, cov@codeaurora.org, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, graeme.gregory@linaro.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, leo.duran@amd.com, hanjun.guo@linaro.org, Suravee.Suthikulpanit@amd.com, sudeep.holla@arm.com, christoffer.dall@linaro.org Subject: Re: [PATCH v24 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver Message-ID: <20170419155627.GA9439@red-moon> References: <20170414184014.8524-1-fu.wei@linaro.org> <20170414184014.8524-10-fu.wei@linaro.org> <20170418172107.GA1401@red-moon> <20170419150725.GH27829@leverpostej> <20170419154554.GJ27829@leverpostej> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170419154554.GJ27829@leverpostej> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 19, 2017 at 04:45:54PM +0100, Mark Rutland wrote: > On Wed, Apr 19, 2017 at 04:07:25PM +0100, Mark Rutland wrote: > > On Tue, Apr 18, 2017 at 06:21:07PM +0100, Lorenzo Pieralisi wrote: > > > On Sat, Apr 15, 2017 at 02:40:12AM +0800, fu.wei@linaro.org wrote: > > > > If yes, why can't it simply be written like this ? > > > > > > for (; i >= 0; i--, gtdt_frame--) { > > > frame = &timer_mem->frame[gtdt_frame->frame_number]; > > > > > > /* not sure this check is actually needed */ > > > if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER) > > > continue; > > > > > > if (frame->phys_irq > 0) > > > acpi_unregister_gsi(gtdt_frame->timer_interrupt); > > > if (frame->virt_irq > 0) > > > acpi_unregister_gsi(gtdt_frame->virtual_timer_interrupt); > > > } > > > > A reverse loop of this form will work. > > > > That requires some restructuring, and care to avoid going out of bounds > > instantaneously with the gtdt_frame--, so as to not invoke nasal demons. > > > > I've attacked this locally, and will send this out after testing. I'll > > drop the new ACPI API patch. > > FWIW, I've set this up so the cleanup path is: > > do { > if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER || > gtdt_frame->frame_number >= ARCH_TIMER_MEM_MAX_FRAMES) > continue; > > frame = &timer_mem->frame[gtdt_frame->frame_number]; > > if (frame->phys_irq > 0) > acpi_unregister_gsi(gtdt_frame->timer_interrupt); > frame->phys_irq = 0; > > if (frame->virt_irq > 0) > acpi_unregister_gsi(gtdt_frame->virtual_timer_interrupt); > frame->virt_irq = 0; > } while (i-- >= 0 && gtdt_frame--); > > ... the zeroing is to account for duplicate frames, which I now check for in > the probe path (as we do for DT). > > Can I take it per your comment on the prior version that with this change I can > take your ack? Yes, thanks for fixing it up, please add my: Acked-by: Lorenzo Pieralisi > I also assume that you're happy for all of the drivers/acpi/arm64/ patches in the > series to go via the clocksource tree? Yes that's how I expect them to go upstream, that's the simplest way. Thanks ! Lorenzo From mboxrd@z Thu Jan 1 00:00:00 1970 From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) Date: Wed, 19 Apr 2017 16:56:27 +0100 Subject: [PATCH v24 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver In-Reply-To: <20170419154554.GJ27829@leverpostej> References: <20170414184014.8524-1-fu.wei@linaro.org> <20170414184014.8524-10-fu.wei@linaro.org> <20170418172107.GA1401@red-moon> <20170419150725.GH27829@leverpostej> <20170419154554.GJ27829@leverpostej> Message-ID: <20170419155627.GA9439@red-moon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Apr 19, 2017 at 04:45:54PM +0100, Mark Rutland wrote: > On Wed, Apr 19, 2017 at 04:07:25PM +0100, Mark Rutland wrote: > > On Tue, Apr 18, 2017 at 06:21:07PM +0100, Lorenzo Pieralisi wrote: > > > On Sat, Apr 15, 2017 at 02:40:12AM +0800, fu.wei at linaro.org wrote: > > > > If yes, why can't it simply be written like this ? > > > > > > for (; i >= 0; i--, gtdt_frame--) { > > > frame = &timer_mem->frame[gtdt_frame->frame_number]; > > > > > > /* not sure this check is actually needed */ > > > if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER) > > > continue; > > > > > > if (frame->phys_irq > 0) > > > acpi_unregister_gsi(gtdt_frame->timer_interrupt); > > > if (frame->virt_irq > 0) > > > acpi_unregister_gsi(gtdt_frame->virtual_timer_interrupt); > > > } > > > > A reverse loop of this form will work. > > > > That requires some restructuring, and care to avoid going out of bounds > > instantaneously with the gtdt_frame--, so as to not invoke nasal demons. > > > > I've attacked this locally, and will send this out after testing. I'll > > drop the new ACPI API patch. > > FWIW, I've set this up so the cleanup path is: > > do { > if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER || > gtdt_frame->frame_number >= ARCH_TIMER_MEM_MAX_FRAMES) > continue; > > frame = &timer_mem->frame[gtdt_frame->frame_number]; > > if (frame->phys_irq > 0) > acpi_unregister_gsi(gtdt_frame->timer_interrupt); > frame->phys_irq = 0; > > if (frame->virt_irq > 0) > acpi_unregister_gsi(gtdt_frame->virtual_timer_interrupt); > frame->virt_irq = 0; > } while (i-- >= 0 && gtdt_frame--); > > ... the zeroing is to account for duplicate frames, which I now check for in > the probe path (as we do for DT). > > Can I take it per your comment on the prior version that with this change I can > take your ack? Yes, thanks for fixing it up, please add my: Acked-by: Lorenzo Pieralisi > I also assume that you're happy for all of the drivers/acpi/arm64/ patches in the > series to go via the clocksource tree? Yes that's how I expect them to go upstream, that's the simplest way. Thanks ! Lorenzo