From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hanjun Guo Subject: Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c Date: Thu, 27 Aug 2015 20:28:15 +0800 Message-ID: <55DF025F.2050102@linaro.org> References: <=fu.wei@linaro.org> <1440435683-7343-1-git-send-email-fu.wei@linaro.org> <1440435683-7343-9-git-send-email-fu.wei@linaro.org> <55DEFC54.4090904@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-watchdog-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Thomas Gleixner Cc: Fu Wei , Suravee Suthikulpanit , Linaro ACPI Mailman List , linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, LKML , linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Wei Fu , G Gregory , Al Stone , Arnd Bergmann , Guenter Roeck , Vipul Gandhi , Wim Van Sebroeck , Jon Masters , Leo Duran , Jonathan Corbet , Mark Rutland , Catalin Marinas , Will Deacon , Rafael Wysocki , dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, Daniel Lezcano List-Id: devicetree@vger.kernel.org On 08/27/2015 08:08 PM, Thomas Gleixner wrote: > On Thu, 27 Aug 2015, Hanjun Guo wrote: >> On 08/26/2015 03:17 AM, Thomas Gleixner wrote: >>> On Wed, 26 Aug 2015, Fu Wei wrote: >>>>>> /* Initialize per-processor generic timer */ >>>>>> -static int __init arch_timer_acpi_init(struct acpi_table_header >>>>>> *table) >>>>>> +void __init arch_timer_acpi_init(void) >>>>>> { >>>>> >>>>> And how is that supposed to work when we have next generation CPUs >>>>> which implement a different timer? You break multisystem kernels that >>>>> way. >> >> Sorry, I think I missed some context here that I don't understand >> why the code here will break multisystem kernels? I'm trying to >> understand the problem here and update the code :) >> >>>> >>>> yes, you are right, If there is a next generation CPUs which >>>> implement a different timer, (maybe) this driver can not work. >>>> we may need a new timer driver. >>>> >>>> But, >>>> (1) for now, aarch64 core always has the arch timer(this timer is >>>> part of aarch64 architecture). >>>> and the existing code make ARM64 kernel "select ARM_ARCH_TIMER " >>>> (2) GTDT is designed for generic timer, so in this call " >>>> arch_timer_acpi_init" we parse the gtdt info. >>>> (3) once we have a ARM64 CPUs which implement a different timer, we >>>> may need to select a right timer in the config stage. >>>> and this timer may not be described in GTDT. So we can implement >>>> another arch_timer_acpi_init by that time in new timer driver.. >>>> if the new time still uses GTDT(or new version GTDT), we may need to >>>> update gtdt.c for new timer by that time. >>> >>> That's simply wrong. You want to build kernels which run on both cpus >>> and the selection of the timer happens at runtime depending on the >>> ACPI info. We do the same thing with device tree. >> >> I think the code can do that if I understand correctly. The code for >> now is that we only support arch timer on ARM64, and this patch set >> is adding SBSA watchdog timer support which need same function in >> arch timer, so we move that function to common place. >> >> We will load the driver (arch timer, memory mapped timer) when the >> ACPI table defines them, which when new timer is coming, that will >> defined in the ACPI table and load the driver as needed. >> >> Please correct me if I misse something, thanks. > > arch_timer_acpi_init() is called from the architecture boot code. So > how is that supposed to work with different timers? > > Are you going to have bla_timer_acpi_init() and foo_timer_acpi_init() > calls as well? > > Why not having a something like DT has: DECLARE_.... > > and the arch_timer_acpi_init() using that to figure out which of the > timers to initialize. Ah, ok, I can fully understand you now, thanks for your patience. Yes, I agree with you, so this is not a problem for this patch, but for the code implementation of previous code. Actually we are on the road to do as you suggested, we introduced something like #define ACPI_DECLARE(table, name, table_id, subtable, data, fn) [1] in the GICv3 and GIC self probe patch set, and I said that infrastructure can be used as clock declare too, we just trying to not add such dependence on that patch set (it's still on discussion), [1]: https://lkml.org/lkml/2015/7/29/236 If that is ok with you, we will introduce similar DECLARE_ thing for clock declare. Thanks Hanjun -- 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