Skip to contentSkip to conversation

Add andestech atcpit100 timer

PATCH In review Rick Chen 4 participants 14 messages Dec 12, 2017 – Jan 8, 2018

Subsystems (6)
Networking Network drivers Device tree bindings TTY layer and serial drivers +2 more subsystems
lore
Share & tools
lore

Rick Chen

author
#0 lore raw

v5 cover Add andestech atcpit100 timer

Changelog v5:
- Patch 1/3: Changes
- Patch 2/3: New
- Patch 3/3: Changes

[Patch 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
1 No need to split out the Makefile patch from the actual driver.
Suggested by Arnd Bergmann
2 Add of_clk.name = "PCLK" to be explicit on what we use.
Suggested by Linus Walleij
3 Remove the GENERIC_CLOCKEVENTS from Kconfig.
Suggested by Daniel Lezcano
4 Add depends on NDS32 || COMPILE_TEST in Kconfig
Suggested by Greentime Hu

[Patch 2/3] clocksource/drivers/atcpit100: VDSO support
Why implemented in timer driver, please see details from
https://lkml.org/lkml/2017/12/8/362
[PATCH v3 17/33] nds32: VDSO support.
Suggested by Mark Rutland
Here Mark Rutlan suggested as below:
You should not add properties to arbitrary DT bindings to handle a Linux implementation detail.
Please remove this DT code, and have the drivers for those timer blocks export this information to your vdso code somehow.

[Patch 3/3] dt-bindings: timer: Add andestech atcpit100 timer binding doc
Fix incorrect description about PCLK.
Suggested by Linus Walleij

Rick Chen (3):
clocksource/drivers/atcpit100: Add andestech atcpit100 timer clocksource/drivers/atcpit100: VDSO support
dt-bindings: timer: Add andestech atcpit100 timer binding doc

 .../bindings/timer/andestech,atcpit100-timer.txt   |  33 +++
 drivers/clocksource/Kconfig                        |   7 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/timer-atcpit100.c              | 270 +++++++++++++++++++++
 4 files changed, 311 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt
 create mode 100644 drivers/clocksource/timer-atcpit100.c
Signature · 6 lines
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#1 lore raw

PATCH v5 1/3 clocksource/drivers/atcpit100: Add andestech atcpit100 timer

ATCPIT100 is often used on the Andes architecture, This timer provide 4 PIT channels. Each PIT channel is a multi-function timer, can be configured as 32,16,8 bit timers or PWM as well.

For system timer it will set channel 1 32-bit timer0 as clock source and count downwards until underflow and restart again.

It also set channel 0 32-bit timer0 as clock event and count downwards until condition match. It will generate an interrupt for handling periodically.

Signed-off-by Rick Chen <[email protected]>
Signed-off-by Greentime Hu <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
---
 drivers/clocksource/Kconfig           |   7 +
 drivers/clocksource/Makefile          |   1 +
 drivers/clocksource/timer-atcpit100.c | 255 ++++++++++++++++++++++++++++++++++
 3 files changed, 263 insertions(+)
 create mode 100644 drivers/clocksource/timer-atcpit100.c
drivers/clocksource/Kconfig +7 -0
index cc60620..8c57ef2 100644
@@ -615,4 +615,11 @@ config CLKSRC_ST_LPC
 	  Enable this option to use the Low Power controller timer
 	  as clocksource.
 
+config CLKSRC_ATCPIT100
+	bool "Clocksource for AE3XX platform"
+	depends on NDS32 || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This option enables support for the Andestech AE3XX platform timers.
+
 endmenu
drivers/clocksource/Makefile +1 -0
index 72711f1..7d072f5 100644
@@ -75,3 +75,4 @@ obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
 obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
+obj-$(CONFIG_CLKSRC_ATCPIT100)		+= timer-atcpit100.o
drivers/clocksource/timer-atcpit100.c new file +255 -0

255 lines not on this page · raw message

Signature · 1 line
2.7.4
#2 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Quoted 7 lines · Rick Chen

ATCPIT100 is often used on the Andes architecture, This timer provide 4 PIT channels. Each PIT channel is a multi-function timer, can be configured as 32,16,8 bit timers or PWM as well.

For system timer it will set channel 1 32-bit timer0 as clock source and count downwards until underflow and restart again.

[ ... ]

drivers/clocksource/Kconfig config CLKSRC_ST_LPC · line 623

Quoted 6 lines
drivers/clocksource/Kconfig +6 -0
+config CLKSRC_ATCPIT100
+	bool "Clocksource for AE3XX platform"
+	depends on NDS32 || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This option enables support for the Andestech AE3XX platform timers.

Hi Rick,

the general rule for the Kconfig is:

bool "Clocksource for AE3XX platform" if COMPILE_TEST

and no deps on the platform.

It is up to the platform Kconfig to select the option.

We want here a silent option but make it selectable in case of compilation test coverage.

Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100 to TIMER_ATCPIT100.

Quoted 11 lines
drivers/clocksource/Kconfig +1 -0
+
 endmenu
drivers/clocksource/Makefile +1 -0
index 72711f1..7d072f5 100644
@@ -75,3 +75,4 @@ obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
 obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
+obj-$(CONFIG_CLKSRC_ATCPIT100)		+= timer-atcpit100.o

[ ... ]

drivers/clocksource/Makefile

Quoted 13 lines
drivers/clocksource/Makefile +13 -0
+static struct timer_of to = {
+	.flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE,
+
+	.clkevt = {
+		.name = "atcpit100_tick",
+		.rating = 300,
+		.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+		.set_state_shutdown = atcpit100_clkevt_shutdown,
+		.set_state_periodic = atcpit100_clkevt_set_periodic,
+		.set_state_oneshot = atcpit100_clkevt_set_oneshot,
+		.tick_resume = atcpit100_clkevt_shutdown,
+		.set_next_event = atcpit100_clkevt_next_event,
+		.cpumask = cpu_all_mask,

You may consider CLOCK_EVT_DYN_IRQ
https://lwn.net/Articles/540160/

Quoted 4 lines
drivers/clocksource/Makefile +4 -0
+	},
+
+	.of_irq = {
+		.handler = atcpit100_timer_interrupt,

drivers/clocksource/Makefile

Quoted 10 lines
drivers/clocksource/Makefile +10 -0
+		.flags = IRQF_TIMER | IRQF_IRQPOLL,
+	},
+
+	/*
+	 * FIXME: we currently only support clocking using PCLK
+	 * and using EXTCLK is not supported in the driver.
+	 */
+	.of_clk = {
+		.name = "PCLK",
+	}

What do you mean ? We can't specify several clock names with timer-of?

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#3 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Hi, Daniel:

2017-12-12 18:05 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 22 lines · Rick Chen
Quoted 7 lines

ATCPIT100 is often used on the Andes architecture, This timer provide 4 PIT channels. Each PIT channel is a multi-function timer, can be configured as 32,16,8 bit timers or PWM as well.

For system timer it will set channel 1 32-bit timer0 as clock source and count downwards until underflow and restart again.

[ ... ]

drivers/clocksource/Kconfig config CLKSRC_ST_LPC · line 623

Quoted 6 lines
drivers/clocksource/Kconfig +6 -0
+config CLKSRC_ATCPIT100
+     bool "Clocksource for AE3XX platform"
+     depends on NDS32 || COMPILE_TEST
+     depends on HAS_IOMEM
+     help
+       This option enables support for the Andestech AE3XX platform timers.

Hi Rick,

the general rule for the Kconfig is:

Quoted 8 lines

bool "Clocksource for AE3XX platform" if COMPILE_TEST

and no deps on the platform.

It is up to the platform Kconfig to select the option.

We want here a silent option but make it selectable in case of compilation test coverage.

The way we like to use it is because
1. This timer is a basic component to boot an nds32 CPU and it should be able to select without COMPILE_TEST for nds32 architecture.
2. It seems conflict with debug info. I am not sure if there is another way to debug kernel(with debug info) with COMPILE_TEST and DEBUG_INFO because we need this driver for nds32 architecture.

Symbol: DEBUG_INFO [=n]
Type : boolean
Prompt: Compile the kernel with debug info
Location:

    -> Kernel hacking
      -> Compile-time checks and compiler options

Defined at lib/Kconfig.debug:140
Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]

Quoted 2 lines

Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100 to TIMER_ATCPIT100.

Thanks. We will rename it in the next version patch.

Quoted 14 lines
Quoted 11 lines
drivers/clocksource/Kconfig +1 -0
+
 endmenu
drivers/clocksource/Makefile +1 -0
index 72711f1..7d072f5 100644
@@ -75,3 +75,4 @@ obj-$(CONFIG_H8300_TMR16)           += h8300_timer16.o
 obj-$(CONFIG_H8300_TPU)                      += h8300_tpu.o
 obj-$(CONFIG_CLKSRC_ST_LPC)          += clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)           += numachip.o
+obj-$(CONFIG_CLKSRC_ATCPIT100)               += timer-atcpit100.o

[ ... ]

Quoted 16 lines

drivers/clocksource/Makefile

Quoted 13 lines
drivers/clocksource/Makefile +13 -0
+static struct timer_of to = {
+     .flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE,
+
+     .clkevt = {
+             .name = "atcpit100_tick",
+             .rating = 300,
+             .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+             .set_state_shutdown = atcpit100_clkevt_shutdown,
+             .set_state_periodic = atcpit100_clkevt_set_periodic,
+             .set_state_oneshot = atcpit100_clkevt_set_oneshot,
+             .tick_resume = atcpit100_clkevt_shutdown,
+             .set_next_event = atcpit100_clkevt_next_event,
+             .cpumask = cpu_all_mask,

You may consider CLOCK_EVT_DYN_IRQ
https://lwn.net/Articles/540160/

Thanks.
We will consider to implement this feature once we support SMP or our CPU has local timer.

Quoted 8 lines
Quoted 8 lines
drivers/clocksource/Makefile +8 -0
+     },
+
+     .of_irq = {
+             .handler = atcpit100_timer_interrupt,
+             .flags = IRQF_TIMER | IRQF_IRQPOLL,
+     },
+
+     /*
Quoted 8 lines

drivers/clocksource/Makefile

Quoted 6 lines
drivers/clocksource/Makefile +6 -0
+      * FIXME: we currently only support clocking using PCLK
+      * and using EXTCLK is not supported in the driver.
+      */
+     .of_clk = {
+             .name = "PCLK",
+     }

What do you mean ? We can't specify several clock names with timer-of?

It means we currently support PCLK only.
https://lkml.org/lkml/2017/12/1/316

Thanks.

#4 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Hi,

sorry I missed your answer. Comments below.

Quoted 31 lines · Greentime Hu

Hi, Daniel:

2017-12-12 18:05 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 28 lines · Rick Chen
Quoted 7 lines

ATCPIT100 is often used on the Andes architecture, This timer provide 4 PIT channels. Each PIT channel is a multi-function timer, can be configured as 32,16,8 bit timers or PWM as well.

For system timer it will set channel 1 32-bit timer0 as clock source and count downwards until underflow and restart again.

[ ... ]

drivers/clocksource/Kconfig config CLKSRC_ST_LPC · line 623

Quoted 6 lines
drivers/clocksource/Kconfig +6 -0
+config CLKSRC_ATCPIT100
+     bool "Clocksource for AE3XX platform"
+     depends on NDS32 || COMPILE_TEST
+     depends on HAS_IOMEM
+     help
+       This option enables support for the Andestech AE3XX platform timers.

Hi Rick,

the general rule for the Kconfig is:

bool "Clocksource for AE3XX platform" if COMPILE_TEST

and no deps on the platform.

It is up to the platform Kconfig to select the option.

Quoted 7 lines
Quoted 2 lines

We want here a silent option but make it selectable in case of compilation test coverage.

The way we like to use it is because
1. This timer is a basic component to boot an nds32 CPU and it should be able to select without COMPILE_TEST for nds32 architecture.

Yes, so you don't need it to be selectable, you must select it from the platform's Kconfig.

Quoted 4 lines

2. It seems conflict with debug info. I am not sure if there is another way to debug kernel(with debug info) with COMPILE_TEST and DEBUG_INFO because we need this driver for nds32 architecture.

Quoted 8 lines

Symbol: DEBUG_INFO [=n]
Type : boolean
Prompt: Compile the kernel with debug info
Location:

    -> Kernel hacking
      -> Compile-time checks and compiler options

Defined at lib/Kconfig.debug:140
Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]

The COMPILE_TEST option is only there to allow cross-compilation test coverage, it does not select or unselect the driver in usual way.

If the COMPILE_TEST is enabled, then the option will appear in the menuconfig, so that gives the opportunity to select/unselect it.

Otherwise, the Kconfig's platform selects automatically the driver and the user *can't* unselect it from the menuconfig as it is a silent option and that is certainly what you want.

Quoted 4 lines
Quoted 2 lines

Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100 to TIMER_ATCPIT100.

Thanks. We will rename it in the next version patch.

You just resend an entire series V5 for the architecture. I'm confused, what is the merging path ?

Thanks.
-- Daniel

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#5 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Hi, Daniel:

2018-01-04 21:50 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 41 lines

Hi,

sorry I missed your answer. Comments below.

Quoted 35 lines · Greentime Hu

Hi, Daniel:

2017-12-12 18:05 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 30 lines · Rick Chen
Quoted 7 lines

ATCPIT100 is often used on the Andes architecture, This timer provide 4 PIT channels. Each PIT channel is a multi-function timer, can be configured as 32,16,8 bit timers or PWM as well.

For system timer it will set channel 1 32-bit timer0 as clock source and count downwards until underflow and restart again.

[ ... ]

drivers/clocksource/Kconfig config CLKSRC_ST_LPC · line 623

Quoted 6 lines
drivers/clocksource/Kconfig +6 -0
+config CLKSRC_ATCPIT100
+     bool "Clocksource for AE3XX platform"
+     depends on NDS32 || COMPILE_TEST
+     depends on HAS_IOMEM
+     help
+       This option enables support for the Andestech AE3XX platform timers.

Hi Rick,

the general rule for the Kconfig is:

bool "Clocksource for AE3XX platform" if COMPILE_TEST

and no deps on the platform.

It is up to the platform Kconfig to select the option.

We want here a silent option but make it selectable in case of compilation test coverage.

Quoted 6 lines
Quoted 3 lines

The way we like to use it is because
1. This timer is a basic component to boot an nds32 CPU and it should be able to select without COMPILE_TEST for nds32 architecture.

Yes, so you don't need it to be selectable, you must select it from the platform's Kconfig.

I am not sure that I get your point or not.
We don't have a CONFIG_PLAT_AE3XX.
Do you mean we should create one and select CLKSRC_ATCPIT100 under CONFIG_PLAT_AE3XX?

Quoted 23 lines
Quoted 4 lines

2. It seems conflict with debug info. I am not sure if there is another way to debug kernel(with debug info) with COMPILE_TEST and DEBUG_INFO because we need this driver for nds32 architecture.

Quoted 8 lines

Symbol: DEBUG_INFO [=n]
Type : boolean
Prompt: Compile the kernel with debug info
Location:

    -> Kernel hacking
      -> Compile-time checks and compiler options

Defined at lib/Kconfig.debug:140
Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]

The COMPILE_TEST option is only there to allow cross-compilation test coverage, it does not select or unselect the driver in usual way.

If the COMPILE_TEST is enabled, then the option will appear in the menuconfig, so that gives the opportunity to select/unselect it.

Otherwise, the Kconfig's platform selects automatically the driver and the user *can't* unselect it from the menuconfig as it is a silent option and that is certainly what you want.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100 to TIMER_ATCPIT100.

Thanks. We will rename it in the next version patch.

You just resend an entire series V5 for the architecture. I'm confused, what is the merging path ?

Sorry. I didn't get your point.
We sent the timer patch and the architecture patch together because it would be easier for reviewer to check the vdso implementations. What do you mean about the merging path?

Thanks.

#6 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Quoted 35 lines · Greentime Hu

Hi, Daniel:

2018-01-04 21:50 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 32 lines

Hi,

sorry I missed your answer. Comments below.

Quoted 26 lines · Greentime Hu

Hi, Daniel:

2017-12-12 18:05 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 23 lines · Rick Chen
Quoted 7 lines

ATCPIT100 is often used on the Andes architecture, This timer provide 4 PIT channels. Each PIT channel is a multi-function timer, can be configured as 32,16,8 bit timers or PWM as well.

For system timer it will set channel 1 32-bit timer0 as clock source and count downwards until underflow and restart again.

[ ... ]

drivers/clocksource/Kconfig config CLKSRC_ST_LPC · line 623

Quoted 6 lines
drivers/clocksource/Kconfig +6 -0
+config CLKSRC_ATCPIT100
+     bool "Clocksource for AE3XX platform"
+     depends on NDS32 || COMPILE_TEST
+     depends on HAS_IOMEM
+     help
+       This option enables support for the Andestech AE3XX platform timers.

Hi Rick,

the general rule for the Kconfig is:

bool "Clocksource for AE3XX platform" if COMPILE_TEST

BTW, select TIMER_OF is missing.

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines

and no deps on the platform.

It is up to the platform Kconfig to select the option.

We want here a silent option but make it selectable in case of compilation test coverage.

The way we like to use it is because
1. This timer is a basic component to boot an nds32 CPU and it should be able to select without COMPILE_TEST for nds32 architecture.

Quoted 7 lines
Quoted 2 lines

Yes, so you don't need it to be selectable, you must select it from the platform's Kconfig.

I am not sure that I get your point or not.
We don't have a CONFIG_PLAT_AE3XX.
Do you mean we should create one and select CLKSRC_ATCPIT100 under CONFIG_PLAT_AE3XX?

No. Can't you add in arch/ndis32/Kconfig ?

+select TIMER_ATCPIT100

Like:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50

Quoted 28 lines
Quoted 28 lines
Quoted 4 lines

2. It seems conflict with debug info. I am not sure if there is another way to debug kernel(with debug info) with COMPILE_TEST and DEBUG_INFO because we need this driver for nds32 architecture.

Quoted 8 lines

Symbol: DEBUG_INFO [=n]
Type : boolean
Prompt: Compile the kernel with debug info
Location:

    -> Kernel hacking
      -> Compile-time checks and compiler options

Defined at lib/Kconfig.debug:140
Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]

The COMPILE_TEST option is only there to allow cross-compilation test coverage, it does not select or unselect the driver in usual way.

If the COMPILE_TEST is enabled, then the option will appear in the menuconfig, so that gives the opportunity to select/unselect it.

Otherwise, the Kconfig's platform selects automatically the driver and the user *can't* unselect it from the menuconfig as it is a silent option and that is certainly what you want.

Quoted 4 lines
Quoted 2 lines

Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100 to TIMER_ATCPIT100.

Thanks. We will rename it in the next version patch.

Quoted 7 lines
Quoted 2 lines

You just resend an entire series V5 for the architecture. I'm confused, what is the merging path ?

Sorry. I didn't get your point.
We sent the timer patch and the architecture patch together because it would be easier for reviewer to check the vdso implementations. What do you mean about the merging path?

I received a [Vx y/3] series and I received a [Vx y/39].

The former from Rick Chen means to me "please pick them through your tree".

The latter from you means to me "can you ack the patches so I can merge them through my tree". Note you will have to resend the entire arch series for every single review/comment (that could end up upset the Cc'ed people).

Which one should I review ? I can not track different patchset implementing the same thing. Which one should I comment, review ? Are the comments I did on [Vx y/3] taken into account in the arch series ? etc ...

Please clarify, it is confusing and impossible to review in this situation.

I suggest we stick to the x/3 series, so I can comment it and you can resend a new version without resending the entire arch series. So I can merge it through my tree, and you get it via eg. a shared immutable branch. The arch series will be reduced by 3 patches.

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#7 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Hi, Daniel:

2018-01-05 3:48 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 31 lines · Greentime Hu
Quoted 30 lines

Hi, Daniel:

2018-01-04 21:50 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 27 lines

Hi,

sorry I missed your answer. Comments below.

Quoted 21 lines · Greentime Hu

Hi, Daniel:

2017-12-12 18:05 GMT+08:00 Daniel Lezcano <[email protected]>:

Quoted 18 lines · Rick Chen
Quoted 7 lines

ATCPIT100 is often used on the Andes architecture, This timer provide 4 PIT channels. Each PIT channel is a multi-function timer, can be configured as 32,16,8 bit timers or PWM as well.

For system timer it will set channel 1 32-bit timer0 as clock source and count downwards until underflow and restart again.

[ ... ]

Quoted 6 lines
drivers/clocksource/Kconfig +6 -0
+config CLKSRC_ATCPIT100
+     bool "Clocksource for AE3XX platform"
+     depends on NDS32 || COMPILE_TEST
+     depends on HAS_IOMEM
+     help
+       This option enables support for the Andestech AE3XX platform timers.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

Hi Rick,

the general rule for the Kconfig is:

bool "Clocksource for AE3XX platform" if COMPILE_TEST

BTW, select TIMER_OF is missing.

We don't select here because we select TIMER_OF in arch/nds32/Kconfig I am not sure if I still need to select TIMER_OF here?

Quoted 20 lines
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines

and no deps on the platform.

It is up to the platform Kconfig to select the option.

We want here a silent option but make it selectable in case of compilation test coverage.

The way we like to use it is because
1. This timer is a basic component to boot an nds32 CPU and it should be able to select without COMPILE_TEST for nds32 architecture.

Quoted 7 lines
Quoted 2 lines

Yes, so you don't need it to be selectable, you must select it from the platform's Kconfig.

I am not sure that I get your point or not.
We don't have a CONFIG_PLAT_AE3XX.
Do you mean we should create one and select CLKSRC_ATCPIT100 under CONFIG_PLAT_AE3XX?

Quoted 7 lines

No. Can't you add in arch/ndis32/Kconfig ?

+select TIMER_ATCPIT100

Like:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50

IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in arch/nds32/Kconfig because it is part of SoC instead of CPU. If we change to another SoC with another timer, we need to select another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100. It seems more flexible to be selected in driver layer.

It seems to be the timer is part of the arch to be selected in arch's Kconfig.

arch/arc/Kconfig:       select ARC_TIMERS
arch/arc/Kconfig:       select ARC_TIMERS_64BIT
arch/arm/Kconfig:       select ARM_ARCH_TIMER
arch/arm64/Kconfig:     select ARM_ARCH_TIMER
arch/blackfin/Kconfig:  select BFIN_GPTIMERS
Quoted 50 lines
Quoted 28 lines
Quoted 28 lines
Quoted 4 lines

2. It seems conflict with debug info. I am not sure if there is another way to debug kernel(with debug info) with COMPILE_TEST and DEBUG_INFO because we need this driver for nds32 architecture.

Quoted 8 lines

Symbol: DEBUG_INFO [=n]
Type : boolean
Prompt: Compile the kernel with debug info
Location:

    -> Kernel hacking
      -> Compile-time checks and compiler options

Defined at lib/Kconfig.debug:140
Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]

The COMPILE_TEST option is only there to allow cross-compilation test coverage, it does not select or unselect the driver in usual way.

If the COMPILE_TEST is enabled, then the option will appear in the menuconfig, so that gives the opportunity to select/unselect it.

Otherwise, the Kconfig's platform selects automatically the driver and the user *can't* unselect it from the menuconfig as it is a silent option and that is certainly what you want.

Quoted 4 lines
Quoted 2 lines

Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100 to TIMER_ATCPIT100.

Thanks. We will rename it in the next version patch.

Quoted 7 lines
Quoted 2 lines

You just resend an entire series V5 for the architecture. I'm confused, what is the merging path ?

Sorry. I didn't get your point.
We sent the timer patch and the architecture patch together because it would be easier for reviewer to check the vdso implementations. What do you mean about the merging path?

I received a [Vx y/3] series and I received a [Vx y/39].

The former from Rick Chen means to me "please pick them through your tree".

The latter from you means to me "can you ack the patches so I can merge them through my tree". Note you will have to resend the entire arch series for every single review/comment (that could end up upset the Cc'ed people).

Which one should I review ? I can not track different patchset implementing the same thing. Which one should I comment, review ? Are the comments I did on [Vx y/3] taken into account in the arch series ? etc ...

Quoted 6 lines

Please clarify, it is confusing and impossible to review in this situation.

I suggest we stick to the x/3 series, so I can comment it and you can resend a new version without resending the entire arch series. So I can merge it through my tree, and you get it via eg. a shared immutable branch. The arch series will be reduced by 3 patches.

Thanks for your explanation. :)
I will send these 2 patch set separately next time.

#8 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Quoted 1 line · Greentime Hu

Hi, Daniel:

[ ... ]

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines

[ ... ]

Review

Quoted 6 lines
(quoted diff) +6 -0
+config CLKSRC_ATCPIT100
+     bool "Clocksource for AE3XX platform"
+     depends on NDS32 || COMPILE_TEST
+     depends on HAS_IOMEM
+     help
+       This option enables support for the Andestech AE3XX platform timers.

Hi Rick,

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

the general rule for the Kconfig is:

bool "Clocksource for AE3XX platform" if COMPILE_TEST

BTW, select TIMER_OF is missing.

We don't select here because we select TIMER_OF in arch/nds32/Kconfig I am not sure if I still need to select TIMER_OF here?

Actually, I want the drivers/clocksource/Kconfig to be consistent across all entries. As TIMER_OF is needed by the driver and nothing else, it must be selected in the TIMER entry.

As there are a lot of timers and we do the changes little by little, there are still entries with different format.

It should be something like that:

config ASM9260_TIMER
bool "ASM9260 timer driver" if COMPILE_TEST select CLKSRC_MMIO
select TIMER_OF
help
Enables support for the ASM9260 timer.

Move the select TIMER_OF to the timer option entry.

Quoted 34 lines
Quoted 20 lines
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines

and no deps on the platform.

It is up to the platform Kconfig to select the option.

We want here a silent option but make it selectable in case of compilation test coverage.

The way we like to use it is because
1. This timer is a basic component to boot an nds32 CPU and it should be able to select without COMPILE_TEST for nds32 architecture.

Quoted 7 lines
Quoted 2 lines

Yes, so you don't need it to be selectable, you must select it from the platform's Kconfig.

I am not sure that I get your point or not.
We don't have a CONFIG_PLAT_AE3XX.
Do you mean we should create one and select CLKSRC_ATCPIT100 under CONFIG_PLAT_AE3XX?

Quoted 7 lines

No. Can't you add in arch/ndis32/Kconfig ?

+select TIMER_ATCPIT100

Like:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50

IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in arch/nds32/Kconfig because it is part of SoC instead of CPU. If we change to another SoC with another timer, we need to select another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100. It seems more flexible to be selected in driver layer.

Quoted 6 lines

It seems to be the timer is part of the arch to be selected in arch's Kconfig.

arch/arc/Kconfig:       select ARC_TIMERS
arch/arc/Kconfig:       select ARC_TIMERS_64BIT
arch/arm/Kconfig:       select ARM_ARCH_TIMER
arch/arm64/Kconfig:     select ARM_ARCH_TIMER
arch/blackfin/Kconfig:  select BFIN_GPTIMERS

No, the timer must be selected from the arch/soc's or whatever Kconfig. Not in the clocksource's Kconfig.

eg.

on ARM:

arch/arm/mach-vt8500/Kconfig:   select VT8500_TIMER
arch/arm/mach-bcm/Kconfig:      select BCM_KONA_TIMER
arch/arm/mach-actions/Kconfig:  select OWL_TIMER
arch/arm/mach-digicolor/Kconfig:        select DIGICOLOR_TIMER

etc ...

on ARM64:

arch/arm64/Kconfig.platforms:   select OWL_TIMER
arch/arm64/Kconfig.platforms:       select ARM_TIMER_SP804
arch/arm64/Kconfig.platforms:       select MTK_TIMER

etc ...

Thanks.

-- Daniel

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
maintainer
#9 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Quoted 35 lines · Daniel Lezcano
Quoted 14 lines
Quoted 7 lines

No. Can't you add in arch/ndis32/Kconfig ?

+select TIMER_ATCPIT100

Like:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50

IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in arch/nds32/Kconfig because it is part of SoC instead of CPU. If we change to another SoC with another timer, we need to select another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100. It seems more flexible to be selected in driver layer.

Quoted 6 lines

It seems to be the timer is part of the arch to be selected in arch's Kconfig.

arch/arc/Kconfig:       select ARC_TIMERS
arch/arc/Kconfig:       select ARC_TIMERS_64BIT
arch/arm/Kconfig:       select ARM_ARCH_TIMER
arch/arm64/Kconfig:     select ARM_ARCH_TIMER
arch/blackfin/Kconfig:  select BFIN_GPTIMERS

No, the timer must be selected from the arch/soc's or whatever Kconfig. Not in the clocksource's Kconfig.

eg.

on ARM:

arch/arm/mach-vt8500/Kconfig:   select VT8500_TIMER
arch/arm/mach-bcm/Kconfig:      select BCM_KONA_TIMER
arch/arm/mach-actions/Kconfig:  select OWL_TIMER
arch/arm/mach-digicolor/Kconfig:        select DIGICOLOR_TIMER

etc ...

Quoted 7 lines

on ARM64:

arch/arm64/Kconfig.platforms:   select OWL_TIMER
arch/arm64/Kconfig.platforms:       select ARM_TIMER_SP804
arch/arm64/Kconfig.platforms:       select MTK_TIMER

etc ...

I'd actually prefer to not do it for ARM either: Most other subsystems don't do that, and I don't see a strong reason why clocksource should be special here.

Selecting 'TIMER_OF' from the individual drivers that need it (as you suggest) makes sense, but I think for ARM we treat SoC families as a bit too special, in the end they are for the most part collections of individual hardware blocks that may or may not be present on some chip.

In case of risc-v and nds32, I expect that the separation will be even less visibile in the hardware, as a typical model here is that one company designs SoCs for multiple customers that each have different requirements. Some of them may have one timer and some have another timer or multiple timers, but there is no strict separation between SoC families as I understand. Here we'd be better off not having a per-SoC Kconfig option at all, just a generic defconfig that enables all the drivers that might be used, and integrators can have a defconfig file that only enables the stuff they actually use on a given chip.

Arnd

#10 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Quoted 43 lines · Arnd Bergmann
Quoted 41 lines · Daniel Lezcano
Quoted 14 lines
Quoted 7 lines

No. Can't you add in arch/ndis32/Kconfig ?

+select TIMER_ATCPIT100

Like:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50

IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in arch/nds32/Kconfig because it is part of SoC instead of CPU. If we change to another SoC with another timer, we need to select another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100. It seems more flexible to be selected in driver layer.

Quoted 6 lines

It seems to be the timer is part of the arch to be selected in arch's Kconfig.

arch/arc/Kconfig:       select ARC_TIMERS
arch/arc/Kconfig:       select ARC_TIMERS_64BIT
arch/arm/Kconfig:       select ARM_ARCH_TIMER
arch/arm64/Kconfig:     select ARM_ARCH_TIMER
arch/blackfin/Kconfig:  select BFIN_GPTIMERS

No, the timer must be selected from the arch/soc's or whatever Kconfig. Not in the clocksource's Kconfig.

eg.

on ARM:

arch/arm/mach-vt8500/Kconfig:   select VT8500_TIMER
arch/arm/mach-bcm/Kconfig:      select BCM_KONA_TIMER
arch/arm/mach-actions/Kconfig:  select OWL_TIMER
arch/arm/mach-digicolor/Kconfig:        select DIGICOLOR_TIMER

etc ...

on ARM64:

arch/arm64/Kconfig.platforms:   select OWL_TIMER
arch/arm64/Kconfig.platforms:       select ARM_TIMER_SP804
arch/arm64/Kconfig.platforms:       select MTK_TIMER
Quoted 5 lines
Quoted 1 line

etc ...

I'd actually prefer to not do it for ARM either: Most other subsystems don't do that, and I don't see a strong reason why clocksource should be special here.

The majority doing the opposite does not mean it is right.

Do you know which clock belongs to which board ? Who will unselect a clock ? I'm pretty sure nobody. Everyone relies on the platform Kconfig and expect it to select the right drivers.

We don't expect the hackers to have a deep knowledge of the hardware and the driver dependencies. It is very convenient to not care about that and let the platform's Kconfig to select the right drivers.

And that is the behavior I would like to keep.

Quoted 6 lines

Selecting 'TIMER_OF' from the individual drivers that need it (as you suggest) makes sense, but I think for ARM we treat SoC families as a bit too special, in the end they are for the most part collections of individual hardware blocks that may or may not be present on some chip.

Quoted 10 lines

In case of risc-v and nds32, I expect that the separation will be even less visibile in the hardware, as a typical model here is that one company designs SoCs for multiple customers that each have different requirements. Some of them may have one timer and some have another timer or multiple timers, but there is no strict separation between SoC families as I understand. Here we'd be better off not having a per-SoC Kconfig option at all, just a generic defconfig that enables all the drivers that might be used, and integrators can have a defconfig file that only enables the stuff they actually use on a given chip.

Yes, the result is the same, the option is not showed in the menu.

However, I can understand it could be interesting to have the ability to unselect a driver for experts.

I'm wondering if we can create a bool_expert which shows up only when CONFIG_EXPERT is set.

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
maintainer
#11 lore raw

Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer

Quoted 11 lines · Daniel Lezcano
Quoted 9 lines · Arnd Bergmann
Quoted 2 lines · Daniel Lezcano

etc ...

I'd actually prefer to not do it for ARM either: Most other subsystems don't do that, and I don't see a strong reason why clocksource should be special here.

Quoted 5 lines

The majority doing the opposite does not mean it is right.

Do you know which clock belongs to which board ? Who will unselect a clock ? I'm pretty sure nobody. Everyone relies on the platform Kconfig and expect it to select the right drivers.

The point is that there is no platform specific Kconfig that could select it, as the concept doesn't make a lot of sense here.

If you require the driver to always be selected by the architecture code, it adds a little bit of bloat on systems that don't need it. This is possible, but I think it's preferable to give users a way of tuning a kernel for a particular chip.

Quoted 5 lines

We don't expect the hackers to have a deep knowledge of the hardware and the driver dependencies. It is very convenient to not care about that and let the platform's Kconfig to select the right drivers.

And that is the behavior I would like to keep.

I'm not worried about the driver bloating the kernel here when it isn't disabled, this is no different from enabling the USB controller by default for a board that doesn't have a USB connector, or enabling ten different pinctrl drivers for all members of a chip family even though you know which particular chip you are running on.

Quoted 17 lines
Quoted 6 lines

Selecting 'TIMER_OF' from the individual drivers that need it (as you suggest) makes sense, but I think for ARM we treat SoC families as a bit too special, in the end they are for the most part collections of individual hardware blocks that may or may not be present on some chip.

Quoted 10 lines

In case of risc-v and nds32, I expect that the separation will be even less visibile in the hardware, as a typical model here is that one company designs SoCs for multiple customers that each have different requirements. Some of them may have one timer and some have another timer or multiple timers, but there is no strict separation between SoC families as I understand. Here we'd be better off not having a per-SoC Kconfig option at all, just a generic defconfig that enables all the drivers that might be used, and integrators can have a defconfig file that only enables the stuff they actually use on a given chip.

Quoted 7 lines

Yes, the result is the same, the option is not showed in the menu.

However, I can understand it could be interesting to have the ability to unselect a driver for experts.

I'm wondering if we can create a bool_expert which shows up only when CONFIG_EXPERT is set.

Having a dependency on CONFIG_EXPERT means that a more users will end up having to set that for a shipping system. It's something we can do (even without a special Kconfig keyword), but it should be used carefully for stuff that 99% of the users want to enable.

Why not just:

config CLKSRC_ATCPIT100
bool "Clocksource for AE3XX platform"
depends on NDS32 || COMPILE_TEST
depends on HAS_IOMEM
default NDS32
help
This option enables support for the Andestech AE3XX platform timers.

That way, it's simply enabled on NDS32 by default, but just as easy to disable for systems that don't need it.

Arnd

author
#12 lore raw

PATCH v5 2/3 clocksource/drivers/atcpit100: VDSO support

In reply to Rick Chen

VDSO needs real-time cycle count to ensure the time accuracy. Unlike others, nds32 architecture does not define clock source, hence VDSO needs atcpit100 offering real-time cycle count to derive the correct time.

Signed-off-by Vincent Chen <[email protected]>
Signed-off-by Rick Chen <[email protected]>
Signed-off-by Greentime Hu <[email protected]>
---
 drivers/clocksource/timer-atcpit100.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/clocksource/timer-atcpit100.c +15 -0
index 0077fdb..1be6c0a 100644
@@ -29,6 +29,9 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include "timer-of.h"
+#ifdef CONFIG_NDS32
+#include <asm/vdso_timer_info.h>
+#endif
 
 /*
  * Definition of register offsets

@@ -211,6 +214,14 @@ static u64 notrace atcpit100_timer_sched_read(void)
 	return ~readl(timer_of_base(&to) + CH1_CNT);
 }
 
+#ifdef CONFIG_NDS32
+static void fill_vdso_need_info(void)
+{
+	timer_info.cycle_count_down = true;
+	timer_info.cycle_count_reg_offset = CH1_CNT;
+}
+#endif
+
 static int __init atcpit100_timer_init(struct device_node *node)
 {
 	int ret;

@@ -249,6 +260,10 @@ static int __init atcpit100_timer_init(struct device_node *node)
 	val = readl(base + INT_EN);
 	writel(val | CH0INT0EN, base + INT_EN);
 
+#ifdef CONFIG_NDS32
+	fill_vdso_need_info();
+#endif
+
 	return ret;
 }
Signature · 1 line
2.7.4
author
#13 lore raw

PATCH v5 3/3 dt-bindings: timer: Add andestech atcpit100 timer binding doc

In reply to Rick Chen

Add a document to describe Andestech atcpit100 timer and binding information.

Signed-off-by Rick Chen <[email protected]>
Signed-off-by Greentime Hu <[email protected]>
Acked-by Rob Herring <[email protected]>
---
 .../bindings/timer/andestech,atcpit100-timer.txt   | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt
Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt new file +33 -0
new file mode 100644
index 0000000..14812f68
@@ -0,0 +1,33 @@
+Andestech ATCPIT100 timer
+------------------------------------------------------------------
+ATCPIT100 is a generic IP block from Andes Technology, embedded in
+Andestech AE3XX platforms and other designs.
+
+This timer is a set of compact multi-function timers, which can be
+used as pulse width modulators (PWM) as well as simple timers.
+
+It supports up to 4 PIT channels. Each PIT channel is a
+multi-function timer and provide the following usage scenarios:
+One 32-bit timer
+Two 16-bit timers
+Four 8-bit timers
+One 16-bit PWM
+One 16-bit timer and one 8-bit PWM
+Two 8-bit timer and one 8-bit PWM
+
+Required properties:
+- compatible	: Should be "andestech,atcpit100"
+- reg		: Address and length of the register set
+- interrupts	: Reference to the timer interrupt
+- clocks : a clock to provide the tick rate for "andestech,atcpit100"
+- clock-names : should be "PCLK" for the peripheral clock source.
+
+Examples:
+
+timer0: timer@f0400000 {
+	compatible = "andestech,atcpit100";
+	reg = <0xf0400000 0x1000>;
+	interrupts = <2 4>;
+	clocks = <&apb>;
+	clock-names = "PCLK";
+};
Signature · 1 line
2.7.4