Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1212358 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c fu.wei@linaro.org - 2015-08-24 19:10 +0200
    Re: [PATCH v7 8/8] clocksource: simplify ACPI code in  arm_arch_timer.c Thomas Gleixner <tglx@linutronix.de> - 2015-08-24 20:00 +0200
      Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c Fu Wei <fu.wei@linaro.org> - 2015-08-25 19:20 +0200
        Re: [PATCH v7 8/8] clocksource: simplify ACPI code in  arm_arch_timer.c Thomas Gleixner <tglx@linutronix.de> - 2015-08-25 21:20 +0200
          Re: [PATCH v7 8/8] clocksource: simplify ACPI code in  arm_arch_timer.c Thomas Gleixner <tglx@linutronix.de> - 2015-08-27 14:10 +0200
            Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c Hanjun Guo <hanjun.guo@linaro.org> - 2015-08-27 14:30 +0200
              Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c Hanjun Guo <hanjun.guo@linaro.org> - 2015-08-27 15:40 +0200
                Re: [PATCH v7 8/8] clocksource: simplify ACPI code in  arm_arch_timer.c Thomas Gleixner <tglx@linutronix.de> - 2015-08-27 15:50 +0200
                  Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c Fu Wei <fu.wei@linaro.org> - 2015-08-27 16:00 +0200
          Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c Hanjun Guo <hanjun.guo@linaro.org> - 2015-08-27 14:10 +0200

#1212358 — [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c

From: Fu Wei <fu.wei@linaro.org>

The patch update arm_arch_timer driver to use the function
provided by the new GTDT driver of ACPI.
By this way, arm_arch_timer.c can be simplified, and separate
all the ACPI GTDT knowledge from this timer driver.

Signed-off-by: Fu Wei <fu.wei@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/time.c             |  4 +--
 drivers/acpi/gtdt.c                  | 43 ++++++++++++++++++++++++++
 drivers/clocksource/Kconfig          |  1 +
 drivers/clocksource/arm_arch_timer.c | 60 +++++++-----------------------------
 include/clocksource/arm_arch_timer.h |  8 +++++
 include/linux/acpi.h                 |  5 +++
 include/linux/clocksource.h          |  4 +--
 7 files changed, 72 insertions(+), 53 deletions(-)

diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 42f9195..2cabea6 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -75,9 +75,9 @@ void __init time_init(void)
 
 	/*
 	 * Since ACPI or FDT will only one be available in the system,
-	 * we can use acpi_generic_timer_init() here safely
+	 * we can use arch_timer_acpi_init() here safely
 	 */
-	acpi_generic_timer_init();
+	arch_timer_acpi_init();
 
 	arch_timer_rate = arch_timer_get_rate();
 	if (!arch_timer_rate)
diff --git a/drivers/acpi/gtdt.c b/drivers/acpi/gtdt.c
index bbe3a2e..3559babf 100644
--- a/drivers/acpi/gtdt.c
+++ b/drivers/acpi/gtdt.c
@@ -17,6 +17,8 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
+#include <clocksource/arm_arch_timer.h>
+
 static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
 {
 	int trigger, polarity;
@@ -33,6 +35,47 @@ static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
 	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
 }
 
+static struct arch_timer_data __initdata *arch_timer_data_p;
+
+static int __init arch_timer_data_init(struct acpi_table_header *table)
+{
+	struct acpi_table_gtdt *gtdt;
+
+	gtdt = container_of(table, struct acpi_table_gtdt, header);
+
+	arch_timer_data_p->phys_secure_ppi =
+		map_generic_timer_interrupt(gtdt->secure_el1_interrupt,
+					    gtdt->secure_el1_flags);
+
+	arch_timer_data_p->phys_nonsecure_ppi =
+		map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt,
+					    gtdt->non_secure_el1_flags);
+
+	arch_timer_data_p->virt_ppi =
+		map_generic_timer_interrupt(gtdt->virtual_timer_interrupt,
+					    gtdt->virtual_timer_flags);
+
+	arch_timer_data_p->hyp_ppi =
+		map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt,
+					    gtdt->non_secure_el2_flags);
+
+	arch_timer_data_p->c3stop = !(gtdt->non_secure_el1_flags &
+				      ACPI_GTDT_ALWAYS_ON);
+
+	return 0;
+}
+
+/* Initialize the arch_timer_data struct for arm_arch_timer by GTDT info */
+int __init gtdt_arch_timer_data_init(struct arch_timer_data *data)
+{
+	if (acpi_disabled || !data)
+		return -EINVAL;
+
+	arch_timer_data_p = data;
+
+	return acpi_table_parse(ACPI_SIG_GTDT, arch_timer_data_init);
+}
+
 /*
  * Initialize a SBSA generic Watchdog platform device info from GTDT
  * According to SBSA specification the size of refresh and control
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4e57730..e111025 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -119,6 +119,7 @@ config CLKSRC_STM32
 config ARM_ARCH_TIMER
 	bool
 	select CLKSRC_OF if OF
+	select ACPI_GTDT if ACPI
 
 config ARM_ARCH_TIMER_EVTSTREAM
 	bool "Support for ARM architected timer event stream generation"
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 0aa135d..99505bb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -817,68 +817,30 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem",
 		       arch_timer_mem_init);
 
 #ifdef CONFIG_ACPI
-static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
-{
-	int trigger, polarity;
-
-	if (!interrupt)
-		return 0;
-
-	trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
-			: ACPI_LEVEL_SENSITIVE;
-
-	polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
-			: ACPI_ACTIVE_HIGH;
-
-	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
-}
-
 /* Initialize per-processor generic timer */
-static int __init arch_timer_acpi_init(struct acpi_table_header *table)
+void __init arch_timer_acpi_init(void)
 {
-	struct acpi_table_gtdt *gtdt;
+	struct arch_timer_data data;
 
 	if (arch_timers_present & ARCH_CP15_TIMER) {
 		pr_warn("arch_timer: already initialized, skipping\n");
-		return -EINVAL;
+		return;
 	}
 
-	gtdt = container_of(table, struct acpi_table_gtdt, header);
-
 	arch_timers_present |= ARCH_CP15_TIMER;
 
-	arch_timer_ppi[PHYS_SECURE_PPI] =
-		map_generic_timer_interrupt(gtdt->secure_el1_interrupt,
-		gtdt->secure_el1_flags);
-
-	arch_timer_ppi[PHYS_NONSECURE_PPI] =
-		map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt,
-		gtdt->non_secure_el1_flags);
-
-	arch_timer_ppi[VIRT_PPI] =
-		map_generic_timer_interrupt(gtdt->virtual_timer_interrupt,
-		gtdt->virtual_timer_flags);
+	if (gtdt_arch_timer_data_init(&data))
+		return;
 
-	arch_timer_ppi[HYP_PPI] =
-		map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt,
-		gtdt->non_secure_el2_flags);
+	arch_timer_ppi[PHYS_SECURE_PPI] = data.phys_secure_ppi;
+	arch_timer_ppi[PHYS_NONSECURE_PPI] = data.phys_nonsecure_ppi;
+	arch_timer_ppi[VIRT_PPI] = data.virt_ppi;
+	arch_timer_ppi[HYP_PPI] = data.hyp_ppi;
+	/* Always-on capability */
+	arch_timer_c3stop = data.c3stop;
 
 	/* Get the frequency from CNTFRQ */
 	arch_timer_detect_rate(NULL, NULL);
-
-	/* Always-on capability */
-	arch_timer_c3stop = !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);
-
 	arch_timer_init();
-	return 0;
-}
-
-/* Initialize all the generic timers presented in GTDT */
-void __init acpi_generic_timer_init(void)
-{
-	if (acpi_disabled)
-		return;
-
-	acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
 }
 #endif
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 9916d0e..5deed9d 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -43,6 +43,14 @@ enum arch_timer_reg {
 
 #define ARCH_TIMER_EVT_STREAM_FREQ	10000	/* 100us */
 
+struct arch_timer_data {
+	int phys_secure_ppi;
+	int phys_nonsecure_ppi;
+	int virt_ppi;
+	int hyp_ppi;
+	bool c3stop;
+};
+
 #ifdef CONFIG_ARM_ARCH_TIMER
 
 extern u32 arch_timer_get_rate(void);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 57c13df..6d7a290 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -449,6 +449,11 @@ void acpi_walk_dep_device_list(acpi_handle handle);
 struct platform_device *acpi_create_platform_device(struct acpi_device *);
 #define ACPI_PTR(_ptr)	(_ptr)
 
+#ifdef CONFIG_ACPI_GTDT
+struct arch_timer_data;
+int __init gtdt_arch_timer_data_init(struct arch_timer_data *data);
+#endif
+
 #else	/* !CONFIG_ACPI */
 
 #define acpi_disabled 1
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 278dd27..b250b75 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -253,9 +253,9 @@ static inline void clocksource_of_init(void) {}
 #endif
 
 #ifdef CONFIG_ACPI
-void acpi_generic_timer_init(void);
+void arch_timer_acpi_init(void);
 #else
-static inline void acpi_generic_timer_init(void) { }
+static inline void arch_timer_acpi_init(void) { }
 #endif
 
 #endif /* _LINUX_CLOCKSOURCE_H */
-- 
2.4.3

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

[toc] | [next] | [standalone]

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

On Tue, 25 Aug 2015, fu.wei@linaro.org wrote:

You Cc the world and some more on your patch, but you fail to add the
maintainers of the clocksource code to the Cc list. Sigh.

> From: Fu Wei <fu.wei@linaro.org>
> 
> The patch update arm_arch_timer driver to use the function
> provided by the new GTDT driver of ACPI.
> By this way, arm_arch_timer.c can be simplified, and separate
> all the ACPI GTDT knowledge from this timer driver.

That's not a proper changelog and this patch want's to be split in two:

1) Implement the new ACPI function

2) Make use of it
 
> index 0aa135d..99505bb 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -817,68 +817,30 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem",
>  		       arch_timer_mem_init);
>  
>  #ifdef CONFIG_ACPI
> -static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
> -{
> -	int trigger, polarity;
> -
> -	if (!interrupt)
> -		return 0;
> -
> -	trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
> -			: ACPI_LEVEL_SENSITIVE;
> -
> -	polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
> -			: ACPI_ACTIVE_HIGH;
> -
> -	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> -}
> -
>  /* 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.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1213194

Hi Thomas,


On 25 August 2015 at 01:50, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 25 Aug 2015, fu.wei@linaro.org wrote:
>
> You Cc the world and some more on your patch, but you fail to add the
> maintainers of the clocksource code to the Cc list. Sigh.

my fault
So sorry for that, I will add the maintainers of clocksource into cc list.

>
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> The patch update arm_arch_timer driver to use the function
>> provided by the new GTDT driver of ACPI.
>> By this way, arm_arch_timer.c can be simplified, and separate
>> all the ACPI GTDT knowledge from this timer driver.
>
> That's not a proper changelog and this patch want's to be split in two:
>
> 1) Implement the new ACPI function
>
> 2) Make use of it

Yes, you are right , will improve this

>
>> index 0aa135d..99505bb 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -817,68 +817,30 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem",
>>                      arch_timer_mem_init);
>>
>>  #ifdef CONFIG_ACPI
>> -static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
>> -{
>> -     int trigger, polarity;
>> -
>> -     if (!interrupt)
>> -             return 0;
>> -
>> -     trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
>> -                     : ACPI_LEVEL_SENSITIVE;
>> -
>> -     polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
>> -                     : ACPI_ACTIVE_HIGH;
>> -
>> -     return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>> -}
>> -
>>  /* 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.

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.

but before we really have this new timer, I think this code is OK to use.

Do I understand your comment correctly? :-)

>
> Thanks,
>
>         tglx



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
> 
> 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.
 
> but before we really have this new timer, I think this code is OK to use.

I don't think so, but I leave this decision to the ARM64 maintainers.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

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

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214504

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214575

On 08/27/2015 08:28 PM, Hanjun Guo wrote:
> 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.

Or we can drop this patch from this patch set, and clean up this
patch when the ACPI_DECLARE() infrastructure is ready for upstream.

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

[toc] | [prev] | [next] | [standalone]

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

On Thu, 27 Aug 2015, Hanjun Guo wrote:
> > [1]: https://lkml.org/lkml/2015/7/29/236
> > 
> > If that is ok with you, we will introduce similar DECLARE_ thing
> > for clock declare.

Yes.
 
> Or we can drop this patch from this patch set, and clean up this
> patch when the ACPI_DECLARE() infrastructure is ready for upstream.

Works either way. I just noticed that hard coded init thing and
decided to rant about it :)

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1214581

Hi Thomas, Hanjun

On 27 August 2015 at 21:40, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 27 Aug 2015, Hanjun Guo wrote:
>> > [1]: https://lkml.org/lkml/2015/7/29/236
>> >
>> > If that is ok with you, we will introduce similar DECLARE_ thing
>> > for clock declare.
>
> Yes.

Thanks

>
>> Or we can drop this patch from this patch set, and clean up this
>> patch when the ACPI_DECLARE() infrastructure is ready for upstream.
>
> Works either way. I just noticed that hard coded init thing and
> decided to rant about it :)

OK, good idea, this patch will be improve by DECLARE_ thing, then upstream.
drop this from this patchset.

Great thanks for your help


>
> Thanks,
>
>         tglx
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214497

Hi Thomas,

Thanks for the comments, I got some questions and
reply below.

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.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web