[PATCH v5 0/3] Add andestech atcpit100 timer

[PATCH v5 0/3] Add andestech atcpit100 timer

[Rick Chen]

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

-- 
2.7.4

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

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

[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.

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 <rickchen36@gmail.com>
Signed-off-by: Greentime Hu <green.hu@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 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

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cc60620..8c57ef2 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -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
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 72711f1..7d072f5 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -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
diff --git a/drivers/clocksource/timer-atcpit100.c b/drivers/clocksource/timer-atcpit100.c
new file mode 100644
index 0000000..0077fdb
--- /dev/null
+++ b/drivers/clocksource/timer-atcpit100.c
@@ -0,0 +1,255 @@
+/*
+ *  Andestech ATCPIT100 Timer Device Driver Implementation
+ *
+ * Copyright (C) 2017 Andes Technology Corporation
+ * Rick Chen, Andes Technology Corporation <rick@andestech.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ *
+ */
+
+#include <linux/irq.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/cpufreq.h>
+#include <linux/sched.h>
+#include <linux/sched_clock.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include "timer-of.h"
+
+/*
+ * Definition of register offsets
+ */
+
+/* ID and Revision Register */
+#define ID_REV		0x0
+
+/* Configuration Register */
+#define CFG		0x10
+
+/* Interrupt Enable Register */
+#define INT_EN		0x14
+#define CH_INT_EN(c, i)	((1<<i)<<(4*c))
+#define CH0INT0EN	0x01
+
+/* Interrupt Status Register */
+#define INT_STA		0x18
+#define CH0INT0		0x01
+
+/* Channel Enable Register */
+#define CH_EN		0x1C
+#define CH0TMR0EN	0x1
+#define CH1TMR0EN	0x10
+
+/* Channel 0 , 1 Control Register */
+#define CH0_CTL		(0x20)
+#define CH1_CTL		(0x20 + 0x10)
+
+/* Channel clock source , bit 3 , 0:External clock , 1:APB clock */
+#define APB_CLK		BIT(3)
+
+/* Channel mode , bit 0~2 */
+#define TMR_32		0x1
+#define TMR_16		0x2
+#define TMR_8		0x3
+
+/* Channel 0 , 1 Reload Register */
+#define CH0_REL		(0x24)
+#define CH1_REL		(0x24 + 0x10)
+
+/* Channel 0 , 1 Counter Register */
+#define CH0_CNT		(0x28)
+#define CH1_CNT		(0x28 + 0x10)
+
+#define TIMER_SYNC_TICKS	3
+
+static void atcpit100_ch1_tmr0_en(void __iomem *base)
+{
+	writel(~0, base + CH1_REL);
+	writel(APB_CLK|TMR_32, base + CH1_CTL);
+}
+
+static void atcpit100_ch0_tmr0_en(void __iomem *base)
+{
+	writel(APB_CLK|TMR_32, base + CH0_CTL);
+}
+
+static void atcpit100_clkevt_time_setup(void __iomem *base, unsigned long delay)
+{
+	writel(delay, base + CH0_CNT);
+	writel(delay, base + CH0_REL);
+}
+
+static void atcpit100_timer_clear_interrupt(void __iomem *base)
+{
+	u32 val;
+
+	val = readl(base + INT_STA);
+	writel(val | CH0INT0, base + INT_STA);
+}
+
+static void atcpit100_clocksource_start(void __iomem *base)
+{
+	u32 val;
+
+	val = readl(base + CH_EN);
+	writel(val | CH1TMR0EN, base + CH_EN);
+}
+
+static void atcpit100_clkevt_time_start(void __iomem *base)
+{
+	u32 val;
+
+	val = readl(base + CH_EN);
+	writel(val | CH0TMR0EN, base + CH_EN);
+}
+
+static void atcpit100_clkevt_time_stop(void __iomem *base)
+{
+	u32 val;
+
+	atcpit100_timer_clear_interrupt(base);
+	val = readl(base + CH_EN);
+	writel(val & ~CH0TMR0EN, base + CH_EN);
+}
+
+static int atcpit100_clkevt_next_event(unsigned long evt,
+	struct clock_event_device *clkevt)
+{
+	struct timer_of *to = to_timer_of(clkevt);
+
+	writel(evt, timer_of_base(to) + CH0_REL);
+
+	return 0;
+}
+
+static int atcpit100_clkevt_set_periodic(struct clock_event_device *evt)
+{
+	struct timer_of *to = to_timer_of(evt);
+
+	atcpit100_clkevt_time_setup(timer_of_base(to), timer_of_period(to));
+	atcpit100_clkevt_time_start(timer_of_base(to));
+
+	return 0;
+}
+static int atcpit100_clkevt_shutdown(struct clock_event_device *evt)
+{
+	struct timer_of *to = to_timer_of(evt);
+
+	atcpit100_clkevt_time_stop(timer_of_base(to));
+
+	return 0;
+}
+static int atcpit100_clkevt_set_oneshot(struct clock_event_device *evt)
+{
+	struct timer_of *to = to_timer_of(evt);
+	u32 val;
+
+	writel(~0x0, timer_of_base(to) + CH0_REL);
+	val = readl(timer_of_base(to) + CH_EN);
+	writel(val | CH0TMR0EN, timer_of_base(to) + CH_EN);
+
+	return 0;
+}
+
+static irqreturn_t atcpit100_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	struct timer_of *to = to_timer_of(evt);
+
+	atcpit100_timer_clear_interrupt(timer_of_base(to));
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+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,
+	},
+
+	.of_irq = {
+		.handler = atcpit100_timer_interrupt,
+		.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",
+	}
+};
+
+static u64 notrace atcpit100_timer_sched_read(void)
+{
+	return ~readl(timer_of_base(&to) + CH1_CNT);
+}
+
+static int __init atcpit100_timer_init(struct device_node *node)
+{
+	int ret;
+	u32 val;
+	void __iomem *base;
+
+	ret = timer_of_init(node, &to);
+	if (ret)
+		return ret;
+
+	base = timer_of_base(&to);
+
+	sched_clock_register(atcpit100_timer_sched_read, 32,
+		timer_of_rate(&to));
+
+	ret = clocksource_mmio_init(base + CH1_CNT,
+		node->name, timer_of_rate(&to), 300, 32,
+		clocksource_mmio_readl_down);
+
+	if (ret) {
+		pr_err("Failed to register clocksource\n");
+		return ret;
+	}
+
+	/* clear channel 0 timer0 interrupt */
+	atcpit100_timer_clear_interrupt(base);
+
+	clockevents_config_and_register(&to.clkevt, timer_of_rate(&to),
+					TIMER_SYNC_TICKS, 0xffffffff);
+	atcpit100_ch0_tmr0_en(base);
+	atcpit100_ch1_tmr0_en(base);
+	atcpit100_clocksource_start(base);
+	atcpit100_clkevt_time_start(base);
+
+	/* Enable channel 0 timer0 interrupt */
+	val = readl(base + INT_EN);
+	writel(val | CH0INT0EN, base + INT_EN);
+
+	return ret;
+}
+
+TIMER_OF_DECLARE(atcpit100, "andestech,atcpit100", atcpit100_timer_init);
-- 
2.7.4

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

[Daniel Lezcano]

On 12/12/2017 06:46, Rick Chen wrote:
> 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.

[ ... ]

> +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.

> +
>  endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 72711f1..7d072f5 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -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

[ ... ]

> +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/

> +	},
> +
> +	.of_irq = {
> +		.handler = atcpit100_timer_interrupt,
> +		.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?


-- 
 <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

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

[Greentime Hu]

Hi, Daniel:

2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 12/12/2017 06:46, Rick Chen wrote:
>> 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.
>
> [ ... ]
>
>> +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.


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]


> 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.

>> +
>>  endmenu
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index 72711f1..7d072f5 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -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
>
> [ ... ]
>
>> +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.

>> +     },
>> +
>> +     .of_irq = {
>> +             .handler = atcpit100_timer_interrupt,
>> +             .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?

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

Thanks.

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

[Daniel Lezcano]

Hi,

sorry I missed your answer. Comments below.

On 13/12/2017 07:06, Greentime Hu wrote:
> Hi, Daniel:
> 
> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 12/12/2017 06:46, Rick Chen wrote:
>>> 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.
>>
>> [ ... ]
>>
>>> +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.
> 
> 
> 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.

> 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]

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.

>> 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

-- 
 <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

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

[Greentime Hu]

Hi, Daniel:

2018-01-04 21:50 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>
> Hi,
>
> sorry I missed your answer. Comments below.
>
> On 13/12/2017 07:06, Greentime Hu wrote:
>> Hi, Daniel:
>>
>> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>> On 12/12/2017 06:46, Rick Chen wrote:
>>>> 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.
>>>
>>> [ ... ]
>>>
>>>> +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.
>>
>>
>> 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?

>> 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]
>
> 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.
>
>>> 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.

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

[Daniel Lezcano]

On 04/01/2018 15:06, Greentime Hu wrote:
> Hi, Daniel:
> 
> 2018-01-04 21:50 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>
>> Hi,
>>
>> sorry I missed your answer. Comments below.
>>
>> On 13/12/2017 07:06, Greentime Hu wrote:
>>> Hi, Daniel:
>>>
>>> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>> On 12/12/2017 06:46, Rick Chen wrote:
>>>>> 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.
>>>>
>>>> [ ... ]
>>>>
>>>>> +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.

>>>> 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.
>>
>> 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


>>> 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]
>>
>> 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.
>>
>>>> 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?

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.

-- 
 <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

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

[Greentime Hu]

Hi, Daniel:

2018-01-05 3:48 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 04/01/2018 15:06, Greentime Hu wrote:
>> Hi, Daniel:
>>
>> 2018-01-04 21:50 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>
>>> Hi,
>>>
>>> sorry I missed your answer. Comments below.
>>>
>>> On 13/12/2017 07:06, Greentime Hu wrote:
>>>> Hi, Daniel:
>>>>
>>>> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>>> On 12/12/2017 06:46, Rick Chen wrote:
>>>>>> 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.
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>> +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.

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?

>>>>> 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.
>>>
>>> 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

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

>>>> 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]
>>>
>>> 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.
>>>
>>>>> 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?
>
> 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.

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

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

[Daniel Lezcano]

On 05/01/2018 09:45, Greentime Hu wrote:
> Hi, Daniel:

[ ... ]

>>>>>> [ ... ]
>>>>>>
>>>>>>> +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.
> 
> 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.

>>>>>> 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.
>>>>
>>>> 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
> 
> 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

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


-- 
 <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

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

[Arnd Bergmann]

On Fri, Jan 5, 2018 at 10:31 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>>> 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
>
> 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 ...

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

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

[Daniel Lezcano]

On 08/01/2018 16:26, Arnd Bergmann wrote:
> On Fri, Jan 5, 2018 at 10:31 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>>> 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
>>
>> 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 ...
> 
> 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.

> 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.

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.




-- 
 <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

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

[Arnd Bergmann]

On Mon, Jan 8, 2018 at 5:08 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 08/01/2018 16:26, Arnd Bergmann wrote:
>> On Fri, Jan 5, 2018 at 10:31 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:

>>>
>>> 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.

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.

> 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.

>> 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.
>
> 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

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

[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 <vincentc@andestech.com>
Signed-off-by: Rick Chen <rickchen36@gmail.com>
Signed-off-by: Greentime Hu <green.hu@gmail.com>
---
 drivers/clocksource/timer-atcpit100.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clocksource/timer-atcpit100.c b/drivers/clocksource/timer-atcpit100.c
index 0077fdb..1be6c0a 100644
--- a/drivers/clocksource/timer-atcpit100.c
+++ b/drivers/clocksource/timer-atcpit100.c
@@ -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;
 }
 
-- 
2.7.4

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

[Rick Chen]

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

Signed-off-by: Rick Chen <rickchen36@gmail.com>
Signed-off-by: Greentime Hu <green.hu@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/timer/andestech,atcpit100-timer.txt   | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt

diff --git a/Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt b/Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt
new file mode 100644
index 0000000..14812f68
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt
@@ -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";
+};
-- 
2.7.4