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

[Thomas Gleixner]

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.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/