Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c

[Hanjun Guo <hanjun.guo@linaro.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