On 01/02/2019 03:36, Joseph Lo wrote:
> Add support for the Tegra210 timer that runs at oscillator clock
> (TMR10-TMR13). We need these timers to work as clock event device and to
> replace the ARMv8 architected timer due to it can't survive across the
> power cycle of the CPU core or CPUPORESET signal. So it can't be a wake-up
> source when CPU suspends in power down state.
>
> Also convert the original driver to use timer-of API.
It may have been nice to split this into 2 patches to make it easier to
see what is going on but not a big deal.
> Cc: Daniel Lezcano <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> Signed-off-by: Joseph Lo <[email protected]>
> Acked-by: Thierry Reding <[email protected]>
> ---
> v5:
> * add ack tag from Thierry
> v4:
> * merge timer-tegra210.c in previous version into timer-tegra20.c
> v3:
> * use timer-of API
> v2:
> * add error clean-up code
> ---
> drivers/clocksource/Kconfig | 2 +-
> drivers/clocksource/timer-tegra20.c | 369 ++++++++++++++++++++--------
> include/linux/cpuhotplug.h | 1 +
> 3 files changed, 272 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index a9e26f6a81a1..6af78534a285 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -131,7 +131,7 @@ config SUN5I_HSTIMER
> config TEGRA_TIMER
> bool "Tegra timer driver" if COMPILE_TEST
> select CLKSRC_MMIO
> - depends on ARM
> + select TIMER_OF
> help
> Enables support for the Tegra driver.
>
> diff --git a/drivers/clocksource/timer-tegra20.c b/drivers/clocksource/timer-tegra20.c
> index 4293943f4e2b..96a809341c9b 100644
> --- a/drivers/clocksource/timer-tegra20.c
> +++ b/drivers/clocksource/timer-tegra20.c
> @@ -15,21 +15,24 @@
> *
> */
>
> -#include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> +#include <linux/delay.h>
> #include <linux/err.h>
> -#include <linux/time.h>
> #include <linux/interrupt.h>
> -#include <linux/irq.h>
> -#include <linux/clockchips.h>
> -#include <linux/clocksource.h>
> -#include <linux/clk.h>
> -#include <linux/io.h>
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
> -#include <linux/sched_clock.h>
> -#include <linux/delay.h>
> +#include <linux/percpu.h>
> +#include <linux/syscore_ops.h>
> +#include <linux/time.h>
> +
> +#include "timer-of.h"
>
> +#ifdef CONFIG_ARM
> #include <asm/mach/time.h>
> +#endif
>
> #define RTC_SECONDS 0x08
> #define RTC_SHADOW_SECONDS 0x0c
> @@ -43,70 +46,147 @@
> #define TIMER2_BASE 0x8
> #define TIMER3_BASE 0x50
> #define TIMER4_BASE 0x58
> -
> -#define TIMER_PTV 0x0
> -#define TIMER_PCR 0x4
> -
> +#define TIMER10_BASE 0x90
> +
> +#define TIMER_PTV 0x0
> +#define TIMER_PTV_EN BIT(31)
> +#define TIMER_PTV_PER BIT(30)
> +#define TIMER_PCR 0x4
> +#define TIMER_PCR_INTR_CLR BIT(30)
> +
> +#ifdef CONFIG_ARM
> +#define TIMER_BASE TIMER3_BASE
> +#else
> +#define TIMER_BASE TIMER10_BASE
> +#endif
> +#define TIMER10_IRQ_IDX 10
> +#define TIMER_FOR_CPU(cpu) (TIMER_BASE + (cpu) * 8)
> +#define IRQ_IDX_FOR_CPU(cpu) (TIMER10_IRQ_IDX + cpu)
TIMER10_IRQ_IDX and IRQ_IDX_FOR_CPU are only applicable to ARM64 and so
we should probably not defined for ARM to avoid any confusion.
Furthermore, a lot of these TIMERx_BASE definitions are unused AFAICT.
Would be good to get rid of these.
Maybe we could just have ...
+#ifdef CONFIG_ARM
+#define TIMER_CPU0 3
+#else
+#define TIMER_CPU0 10
+#endif
+#define TIMER_BASE_FOR_CPU(cpu) ((TIMER_CPU0 + cpu) * 8)
+#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
Otherwise looks good to me.
Cheers
Jon
--
nvpublic
On 2/1/19 8:44 PM, Jon Hunter wrote:
>
> On 01/02/2019 03:36, Joseph Lo wrote:
>> Add support for the Tegra210 timer that runs at oscillator clock
>> (TMR10-TMR13). We need these timers to work as clock event device and to
>> replace the ARMv8 architected timer due to it can't survive across the
>> power cycle of the CPU core or CPUPORESET signal. So it can't be a wake-up
>> source when CPU suspends in power down state.
>>
>> Also convert the original driver to use timer-of API.
>
> It may have been nice to split this into 2 patches to make it easier to
> see what is going on but not a big deal.
>
>> Cc: Daniel Lezcano <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Joseph Lo <[email protected]>
>> Acked-by: Thierry Reding <[email protected]>
>> ---
>> v5:
>> * add ack tag from Thierry
>> v4:
>> * merge timer-tegra210.c in previous version into timer-tegra20.c
>> v3:
>> * use timer-of API
>> v2:
>> * add error clean-up code
>> ---
>> drivers/clocksource/Kconfig | 2 +-
>> drivers/clocksource/timer-tegra20.c | 369 ++++++++++++++++++++--------
>> include/linux/cpuhotplug.h | 1 +
>> 3 files changed, 272 insertions(+), 100 deletions(-)
>>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index a9e26f6a81a1..6af78534a285 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -131,7 +131,7 @@ config SUN5I_HSTIMER
>> config TEGRA_TIMER
>> bool "Tegra timer driver" if COMPILE_TEST
>> select CLKSRC_MMIO
>> - depends on ARM
>> + select TIMER_OF
>> help
>> Enables support for the Tegra driver.
>>
>> diff --git a/drivers/clocksource/timer-tegra20.c b/drivers/clocksource/timer-tegra20.c
>> index 4293943f4e2b..96a809341c9b 100644
>> --- a/drivers/clocksource/timer-tegra20.c
>> +++ b/drivers/clocksource/timer-tegra20.c
>> @@ -15,21 +15,24 @@
>> *
>> */
>>
>> -#include <linux/init.h>
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/cpu.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/delay.h>
>> #include <linux/err.h>
>> -#include <linux/time.h>
>> #include <linux/interrupt.h>
>> -#include <linux/irq.h>
>> -#include <linux/clockchips.h>
>> -#include <linux/clocksource.h>
>> -#include <linux/clk.h>
>> -#include <linux/io.h>
>> #include <linux/of_address.h>
>> #include <linux/of_irq.h>
>> -#include <linux/sched_clock.h>
>> -#include <linux/delay.h>
>> +#include <linux/percpu.h>
>> +#include <linux/syscore_ops.h>
>> +#include <linux/time.h>
>> +
>> +#include "timer-of.h"
>>
>> +#ifdef CONFIG_ARM
>> #include <asm/mach/time.h>
>> +#endif
>>
>> #define RTC_SECONDS 0x08
>> #define RTC_SHADOW_SECONDS 0x0c
>> @@ -43,70 +46,147 @@
>> #define TIMER2_BASE 0x8
>> #define TIMER3_BASE 0x50
>> #define TIMER4_BASE 0x58
>> -
>> -#define TIMER_PTV 0x0
>> -#define TIMER_PCR 0x4
>> -
>> +#define TIMER10_BASE 0x90
>> +
>> +#define TIMER_PTV 0x0
>> +#define TIMER_PTV_EN BIT(31)
>> +#define TIMER_PTV_PER BIT(30)
>> +#define TIMER_PCR 0x4
>> +#define TIMER_PCR_INTR_CLR BIT(30)
>> +
>> +#ifdef CONFIG_ARM
>> +#define TIMER_BASE TIMER3_BASE
>> +#else
>> +#define TIMER_BASE TIMER10_BASE
>> +#endif
>> +#define TIMER10_IRQ_IDX 10
>> +#define TIMER_FOR_CPU(cpu) (TIMER_BASE + (cpu) * 8)
>> +#define IRQ_IDX_FOR_CPU(cpu) (TIMER10_IRQ_IDX + cpu)
>
> TIMER10_IRQ_IDX and IRQ_IDX_FOR_CPU are only applicable to ARM64 and so
> we should probably not defined for ARM to avoid any confusion.
Okay, will do.
>
> Furthermore, a lot of these TIMERx_BASE definitions are unused AFAICT.
> Would be good to get rid of these.
Okay.
>
> Maybe we could just have ...
>
> +#ifdef CONFIG_ARM
> +#define TIMER_CPU0 3
> +#else
> +#define TIMER_CPU0 10
> +#endif
> +#define TIMER_BASE_FOR_CPU(cpu) ((TIMER_CPU0 + cpu) * 8)
> +#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
>
This can't get the timer base address. I think you mean ...
+#ifdef CONFIG_ARM
+#define TIMER_CPU0 0x50 /* TIMER3 */
+#else
+#define TIMER_CPU0 0x90 /* TIMER10 */
+#endif
+#define TIMER_BASE_FOR_CPU(cpu) (TIMER_CPU0 + (cpu) * 8)
This doesn't need.
+#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
Will fix above accordingly and adding your ack tag.
Thanks,
Joseph
> Otherwise looks good to me.
>
> Cheers
> Jon
>
On 01/02/2019 14:39, Joseph Lo wrote:
> On 2/1/19 8:44 PM, Jon Hunter wrote:
>>
>> On 01/02/2019 03:36, Joseph Lo wrote:
>>> Add support for the Tegra210 timer that runs at oscillator clock
>>> (TMR10-TMR13). We need these timers to work as clock event device and to
>>> replace the ARMv8 architected timer due to it can't survive across the
>>> power cycle of the CPU core or CPUPORESET signal. So it can't be a
>>> wake-up
>>> source when CPU suspends in power down state.
>>>
>>> Also convert the original driver to use timer-of API.
>>
>> It may have been nice to split this into 2 patches to make it easier to
>> see what is going on but not a big deal.
>>
>>> Cc: Daniel Lezcano <[email protected]>
>>> Cc: Thomas Gleixner <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Joseph Lo <[email protected]>
>>> Acked-by: Thierry Reding <[email protected]>
>>> ---
>>> v5:
>>> * add ack tag from Thierry
>>> v4:
>>> * merge timer-tegra210.c in previous version into timer-tegra20.c
>>> v3:
>>> * use timer-of API
>>> v2:
>>> * add error clean-up code
>>> ---
>>> drivers/clocksource/Kconfig | 2 +-
>>> drivers/clocksource/timer-tegra20.c | 369 ++++++++++++++++++++--------
>>> include/linux/cpuhotplug.h | 1 +
>>> 3 files changed, 272 insertions(+), 100 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index a9e26f6a81a1..6af78534a285 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -131,7 +131,7 @@ config SUN5I_HSTIMER
>>> config TEGRA_TIMER
>>> bool "Tegra timer driver" if COMPILE_TEST
>>> select CLKSRC_MMIO
>>> - depends on ARM
>>> + select TIMER_OF
>>> help
>>> Enables support for the Tegra driver.
>>> diff --git a/drivers/clocksource/timer-tegra20.c
>>> b/drivers/clocksource/timer-tegra20.c
>>> index 4293943f4e2b..96a809341c9b 100644
>>> --- a/drivers/clocksource/timer-tegra20.c
>>> +++ b/drivers/clocksource/timer-tegra20.c
>>> @@ -15,21 +15,24 @@
>>> *
>>> */
>>> -#include <linux/init.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/clockchips.h>
>>> +#include <linux/cpu.h>
>>> +#include <linux/cpumask.h>
>>> +#include <linux/delay.h>
>>> #include <linux/err.h>
>>> -#include <linux/time.h>
>>> #include <linux/interrupt.h>
>>> -#include <linux/irq.h>
>>> -#include <linux/clockchips.h>
>>> -#include <linux/clocksource.h>
>>> -#include <linux/clk.h>
>>> -#include <linux/io.h>
>>> #include <linux/of_address.h>
>>> #include <linux/of_irq.h>
>>> -#include <linux/sched_clock.h>
>>> -#include <linux/delay.h>
>>> +#include <linux/percpu.h>
>>> +#include <linux/syscore_ops.h>
>>> +#include <linux/time.h>
>>> +
>>> +#include "timer-of.h"
>>> +#ifdef CONFIG_ARM
>>> #include <asm/mach/time.h>
>>> +#endif
>>> #define RTC_SECONDS 0x08
>>> #define RTC_SHADOW_SECONDS 0x0c
>>> @@ -43,70 +46,147 @@
>>> #define TIMER2_BASE 0x8
>>> #define TIMER3_BASE 0x50
>>> #define TIMER4_BASE 0x58
>>> -
>>> -#define TIMER_PTV 0x0
>>> -#define TIMER_PCR 0x4
>>> -
>>> +#define TIMER10_BASE 0x90
>>> +
>>> +#define TIMER_PTV 0x0
>>> +#define TIMER_PTV_EN BIT(31)
>>> +#define TIMER_PTV_PER BIT(30)
>>> +#define TIMER_PCR 0x4
>>> +#define TIMER_PCR_INTR_CLR BIT(30)
>>> +
>>> +#ifdef CONFIG_ARM
>>> +#define TIMER_BASE TIMER3_BASE
>>> +#else
>>> +#define TIMER_BASE TIMER10_BASE
>>> +#endif
>>> +#define TIMER10_IRQ_IDX 10
>>> +#define TIMER_FOR_CPU(cpu) (TIMER_BASE + (cpu) * 8)
>>> +#define IRQ_IDX_FOR_CPU(cpu) (TIMER10_IRQ_IDX + cpu)
>>
>> TIMER10_IRQ_IDX and IRQ_IDX_FOR_CPU are only applicable to ARM64 and so
>> we should probably not defined for ARM to avoid any confusion.
> Okay, will do.
>>
>> Furthermore, a lot of these TIMERx_BASE definitions are unused AFAICT.
>> Would be good to get rid of these.
>
> Okay.
>>
>> Maybe we could just have ...
>>
>> +#ifdef CONFIG_ARM
>> +#define TIMER_CPU0 3
>> +#else
>> +#define TIMER_CPU0 10
>> +#endif
>> +#define TIMER_BASE_FOR_CPU(cpu) ((TIMER_CPU0 + cpu) * 8)
>> +#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
>>
> This can't get the timer base address. I think you mean ...
>
> +#ifdef CONFIG_ARM
> +#define TIMER_CPU0 0x50 /* TIMER3 */
> +#else
> +#define TIMER_CPU0 0x90 /* TIMER10 */
> +#endif
> +#define TIMER_BASE_FOR_CPU(cpu) (TIMER_CPU0 + (cpu) * 8)
Ah I see.
> This doesn't need.
> +#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
How come? Don't you still need to know the timer index for a given CPU?
Cheers
Jon
--
nvpublic
On 2/1/19 11:43 PM, Jon Hunter wrote:
>
>
> On 01/02/2019 14:39, Joseph Lo wrote:
>> On 2/1/19 8:44 PM, Jon Hunter wrote:
>>>
>>> On 01/02/2019 03:36, Joseph Lo wrote:
>>>> Add support for the Tegra210 timer that runs at oscillator clock
>>>> (TMR10-TMR13). We need these timers to work as clock event device and to
>>>> replace the ARMv8 architected timer due to it can't survive across the
>>>> power cycle of the CPU core or CPUPORESET signal. So it can't be a
>>>> wake-up
>>>> source when CPU suspends in power down state.
>>>>
>>>> Also convert the original driver to use timer-of API.
>>>
>>> It may have been nice to split this into 2 patches to make it easier to
>>> see what is going on but not a big deal.
>>>
>>>> Cc: Daniel Lezcano <[email protected]>
>>>> Cc: Thomas Gleixner <[email protected]>
>>>> Cc: [email protected]
>>>> Signed-off-by: Joseph Lo <[email protected]>
>>>> Acked-by: Thierry Reding <[email protected]>
>>>> ---
snip.
>>>
>> This can't get the timer base address. I think you mean ...
>>
>> +#ifdef CONFIG_ARM
>> +#define TIMER_CPU0 0x50 /* TIMER3 */
>> +#else
>> +#define TIMER_CPU0 0x90 /* TIMER10 */
>> +#endif
>> +#define TIMER_BASE_FOR_CPU(cpu) (TIMER_CPU0 + (cpu) * 8)
>
> Ah I see.
>
>> This doesn't need.
>> +#define TIMER_FOR_CPU(cpu) (TIMER_CPU0 + cpu)
> How come? Don't you still need to know the timer index for a given CPU?
>
Doesn't need. TIMER_BASE_FOR_CPU is enough. Other use cases are well
handled by timer-of API. :)
Thanks,
Joseph