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

Groups > linux.kernel > #1512475 > unrolled thread

Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400 driver

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-10-31 12:00 +0100
    Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 19:00 +0100
      [PATCH 7/9] ARC: breakout timer stuff into a seperate header Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        Re: [PATCH 7/9] ARC: breakout timer stuff into a seperate header Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:30 +0100
      [PATCH 5/9] ARC: breakout aux handling into a seperate header Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        RE: [PATCH 5/9] ARC: breakout aux handling into a seperate header Noam Camus <noamca@mellanox.com> - 2016-11-01 10:30 +0100
      [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:20 +0100
          Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 18:50 +0100
            Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 19:00 +0100
      [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether hardware exists ... Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        Re: [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether  hardware exists ... Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:10 +0100
          Re: [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether  hardware exists ... Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 18:50 +0100
      [PATCH 8/9] ARC: timer: rename config options Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
      [PATCH 0/9] Move ARC timer code into drivers/clocksource/ Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
          Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-01 01:50 +0100
          Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-01 21:50 +0100
            Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-01 22:00 +0100
              Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-02 01:30 +0100
                Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-02 02:10 +0100
                  Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 17:50 +0100
                    Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:00 +0100
                      Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 19:00 +0100
                        Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 19:20 +0100
                          Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 19:50 +0100
              Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:40 +0100
                Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 19:20 +0100
                Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 20:00 +0100
        [PATCH 4/9] ARC: time: move time_init() out of the driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
          Re: [PATCH 4/9] ARC: time: move time_init() out of the driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:20 +0100
        [PATCH 6/9] ARC: move mcip.h into include/soc and adjust the includes Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-01 00:00 +0100
          Re: [PATCH 6/9] ARC: move mcip.h into include/soc and adjust the  includes Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:30 +0100
        [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs. inline asm Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-01 00:00 +0100
          Re: [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs.  inline asm Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:10 +0100
            Re: [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs.  inline asm Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 18:50 +0100
        Re: [PATCH 0/9] Move ARC timer code into drivers/clocksource/ Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:30 +0100
    RE: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Noam Camus <noamca@mellanox.com> - 2016-11-01 01:40 +0100
      Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-01 21:10 +0100
        RE: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Noam Camus <noamca@mellanox.com> - 2016-11-08 12:10 +0100
          Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-10 11:40 +0100
            RE: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Noam Camus <noamca@mellanox.com> - 2016-11-10 14:10 +0100

Page 1 of 3  [1] 2 3  Next page →

#1512475 — Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400 driver

On Sun, Oct 23, 2016 at 03:12:28PM +0300, Noam Camus wrote:
> From: Noam Camus <noamca@mellanox.com>
> 
> Till now we used clockevent from generic ARC driver.
> This was enough as long as we worked with simple multicore SoC.
> When we are working with multithread SoC each HW thread can be
> scheduled to receive timer interrupt using timer mask register.
> This patch will provide a way to control clock events per HW thread.
> 
> The design idea is that for each core there is dedicated regirtser

s/regirtser/register/

Hey ! re-read yourself.

> (TSI) serving all 16 HW threads.
> The register is a bitmask with one bit for each HW thread.
> When HW thread wants that next expiration of timer interrupt will
> hit it then the proper bit should be set in this dedicated register.

I'm not sure to understand this sentence. Do you mean each cpu/thread must
set a flag in the TSI register at BIT(phys_id) when they set a timer ?

> When timer expires all HW threads within this core which their bit
> is set at the TSI register will be interrupted.

Does it mean some HW threads will be wake up for nothing ?

eg. 

HWT1 sets the timer to expire within 2 seconds
HWT2 sets the timer to expire within 2 hours

When the first timer expires, that will wake up HWT1 *and* HWT2 ?

The code is a bit confusing because of the very specific design of this timer.

Below more questions for clarification.

> Driver can be used from device tree by:
> compatible = "ezchip,nps400-timer0" <-- for clocksource
> compatible = "ezchip,nps400-timer1" <-- for clockevent
> 
> Note that name convention for timer0/timer1 was taken from legacy
> ARC design. This design is our base before adding HW threads.
> 
> Signed-off-by: Noam Camus <noamca@mellanox.com>
> 
> Change-Id: Ib351e6fc7a6b691293040ae655f202f3cc2c1298
> ---
>  .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
>  .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
>  .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++
>  drivers/clocksource/timer-nps.c                    |  220 +++++++++++++++++++-
>  include/linux/cpuhotplug.h                         |    1 +
>  5 files changed, 248 insertions(+), 20 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
> deleted file mode 100644
> index c8c03d7..0000000
> --- a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -NPS Network Processor
> -
> -Required properties:
> -
> -- compatible :	should be "ezchip,nps400-timer"
> -
> -Clocks required for compatible = "ezchip,nps400-timer":
> -- clocks : Must contain a single entry describing the clock input
> -
> -Example:
> -
> -timer {
> -	compatible = "ezchip,nps400-timer";
> -	clocks = <&sysclk>;
> -};
> diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
> new file mode 100644
> index 0000000..e3cfce8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
> @@ -0,0 +1,17 @@
> +NPS Network Processor
> +
> +Required properties:
> +
> +- compatible :	should be "ezchip,nps400-timer0"
> +
> +Clocks required for compatible = "ezchip,nps400-timer0":
> +- interrupts : The interrupt of the first timer
> +- clocks : Must contain a single entry describing the clock input
> +
> +Example:
> +
> +timer {
> +	compatible = "ezchip,nps400-timer0";
> +	interrupts = <3>;
> +	clocks = <&sysclk>;
> +};
> diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> new file mode 100644
> index 0000000..c0ab419
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> @@ -0,0 +1,15 @@
> +NPS Network Processor
> +
> +Required properties:
> +
> +- compatible :	should be "ezchip,nps400-timer1"
> +
> +Clocks required for compatible = "ezchip,nps400-timer1":
> +- clocks : Must contain a single entry describing the clock input
> +
> +Example:
> +
> +timer {
> +	compatible = "ezchip,nps400-timer1";
> +	clocks = <&sysclk>;
> +};
> diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
> index 6156e54..0757328 100644
> --- a/drivers/clocksource/timer-nps.c
> +++ b/drivers/clocksource/timer-nps.c
> @@ -46,7 +46,7 @@
>  /* This array is per cluster of CPUs (Each NPS400 cluster got 256 CPUs) */
>  static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
>  
> -static unsigned long nps_timer_rate;
> +static unsigned long nps_timer1_freq;

Why declare a global static variable for a local use in nps_setup_clocksource ? 

>  static int nps_get_timer_clk(struct device_node *node,
>  			     unsigned long *timer_freq,
>  			     struct clk *clk)
> @@ -87,10 +87,10 @@ static int __init nps_setup_clocksource(struct device_node *node)
>  			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
>  				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
>  
> -	nps_get_timer_clk(node, &nps_timer_rate, clk);
> +	nps_get_timer_clk(node, &nps_timer1_freq, clk);
>  
> -	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
> -				    nps_timer_rate, 301, 32, nps_clksrc_read);
> +	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "nps-tick",
> +				    nps_timer1_freq, 301, 32, nps_clksrc_read);
>  	if (ret) {
>  		pr_err("Couldn't register clock source.\n");
>  		clk_disable_unprepare(clk);
> @@ -99,5 +99,215 @@ static int __init nps_setup_clocksource(struct device_node *node)
>  	return ret;
>  }
>  
> -CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer1",
>  		       nps_setup_clocksource);
> +
> +#ifdef CONFIG_EZNPS_MTM_EXT
> +#include <soc/nps/mtm.h>
> +
> +/* Timer related Aux registers */
> +#define AUX_REG_TIMER0_TSI	0xFFFFF850	/* timer 0 HW threads mask */
> +#define NPS_REG_TIMER0_LIMIT	0x23		/* timer 0 limit */
> +#define NPS_REG_TIMER0_CTRL	0x22		/* timer 0 control */
> +#define NPS_REG_TIMER0_CNT	0x21		/* timer 0 count */
> +
> +#define TIMER0_CTRL_IE	(1 << 0) /* Interrupt when Count reaches limit */
> +#define TIMER0_CTRL_NH	(1 << 1) /* Count only when CPU NOT halted */

Please, use BIT(nr) macro.

Can you elaborate "Count only when CPU NOT halted" ?

> +static unsigned long nps_timer0_freq;
> +static unsigned long nps_timer0_irq;
> +
> +/*
> + * Arm the timer to interrupt after @cycles
> + */
> +static void nps_clkevent_timer_event_setup(unsigned int cycles)
> +{
> +	write_aux_reg(NPS_REG_TIMER0_LIMIT, cycles);
> +	write_aux_reg(NPS_REG_TIMER0_CNT, 0);   /* start from 0 */
> +
> +	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +}
> +
> +static void nps_clkevent_rm_thread(bool remove_thread)
> +{
> +	unsigned int cflags;
> +	unsigned int enabled_threads;
> +	unsigned long flags;
> +	int thread;
> +
> +	local_irq_save(flags);
> +	hw_schd_save(&cflags);

Can you explain why those two lines are needed ?

> +	enabled_threads = read_aux_reg(AUX_REG_TIMER0_TSI);
> +
> +	/* remove thread from TSI1 */
> +	if (remove_thread) {
> +		thread = read_aux_reg(CTOP_AUX_THREAD_ID);
> +		enabled_threads &= ~(1 << thread);
> +		write_aux_reg(AUX_REG_TIMER0_TSI, enabled_threads);
> +	}
> +
> +	/* Re-arm the timer if needed */
> +	if (!enabled_threads)
> +		write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_NH);
> +	else
> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +
> +	hw_schd_restore(cflags);
> +	local_irq_restore(flags);
> +}
> +
> +static void nps_clkevent_add_thread(bool set_event)
> +{
> +	int thread;
> +	unsigned int cflags, enabled_threads;
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	hw_schd_save(&cflags);
> +
> +	/* add thread to TSI1 */
> +	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
> +	enabled_threads = read_aux_reg(AUX_REG_TIMER0_TSI);
> +	enabled_threads |= (1 << thread);
> +	write_aux_reg(AUX_REG_TIMER0_TSI, enabled_threads);
> +
> +	/* set next timer event */
> +	if (set_event)
> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +
> +	hw_schd_restore(cflags);
> +	local_irq_restore(flags);
> +}
> +
> +static int nps_clkevent_set_next_event(unsigned long delta,
> +				       struct clock_event_device *dev)
> +{
> +	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
> +	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
> +
> +	nps_clkevent_add_thread(true);
> +	chip->irq_unmask(&desc->irq_data);

Can you explain why invoking low level IRQ callbacks is needed here ?

> +	return 0;
> +}
> +
> +/*
> + * Whenever anyone tries to change modes, we just mask interrupts
> + * and wait for the next event to get set.
> + */
> +static int nps_clkevent_timer_shutdown(struct clock_event_device *dev)
> +{
> +	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
> +	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
> +
> +	chip->irq_mask(&desc->irq_data);
> +
> +	return 0;
> +}
> +
> +static int nps_clkevent_set_periodic(struct clock_event_device *dev)
> +{
> +	nps_clkevent_add_thread(false);
> +	if (read_aux_reg(CTOP_AUX_THREAD_ID) == 0)
> +		nps_clkevent_timer_event_setup(nps_timer0_freq / HZ);

Please explain this. I read only CPU0 can set the periodic timer.

> +	return 0;
> +}
> +
> +static int nps_clkevent_set_oneshot(struct clock_event_device *dev)
> +{
> +	nps_clkevent_rm_thread(true);
> +	nps_clkevent_timer_shutdown(dev);
> +
> +	return 0;
> +}
> +
> +static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
> +	.name				=	"NPS Timer0",
> +	.features			=	CLOCK_EVT_FEAT_ONESHOT |
> +						CLOCK_EVT_FEAT_PERIODIC,
> +	.rating				=	300,
> +	.set_next_event			=	nps_clkevent_set_next_event,
> +	.set_state_periodic		=	nps_clkevent_set_periodic,
> +	.set_state_oneshot		=	nps_clkevent_set_oneshot,
> +	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
> +	.set_state_shutdown		=	nps_clkevent_timer_shutdown,
> +	.tick_resume			=	nps_clkevent_timer_shutdown,
> +};
> +
> +static irqreturn_t timer_irq_handler(int irq, void *dev_id)
> +{
> +	/*
> +	 * Note that generic IRQ core could have passed @evt for @dev_id if
> +	 * irq_set_chip_and_handler() asked for handle_percpu_devid_irq()
> +	 */
> +	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
> +	int irq_reenable = clockevent_state_periodic(evt);
> +
> +	nps_clkevent_rm_thread(!irq_reenable);
> +
> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int nps_timer_starting_cpu(unsigned int cpu)
> +{
> +	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
> +
> +	evt->cpumask = cpumask_of(smp_processor_id());
> +
> +	clockevents_config_and_register(evt, nps_timer0_freq, 0, ULONG_MAX);
> +	enable_percpu_irq(nps_timer0_irq, 0);
> +
> +	return 0;
> +}
> +
> +static int nps_timer_dying_cpu(unsigned int cpu)
> +{
> +	disable_percpu_irq(nps_timer0_irq);
> +	return 0;
> +}
> +
> +static int __init nps_setup_clockevent(struct device_node *node)
> +{
> +	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
> +	struct clk *clk;

clk = 0xDEADBEEF

> +	int ret;
> +
> +	nps_timer0_irq = irq_of_parse_and_map(node, 0);
> +	if (nps_timer0_irq <= 0) {
> +		pr_err("clockevent: missing irq");
> +		return -EINVAL;
> +	}
> +
> +	nps_get_timer_clk(node, &nps_timer0_freq, clk);
> +
> +	/* Needs apriori irq_set_percpu_devid() done in intc map function */
> +	ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler,
> +				 "Timer0 (per-cpu-tick)", evt);
> +	if (ret) {
> +		pr_err("Couldn't request irq\n");
> +		clk_disable_unprepare(clk);

clk is on the stack, hence returning back from the function, clk is undefined.

clk_disable_unprepare(0xDEADBEEF) ==> kernel panic

It does not make sense to add the nps_get_timer_clk() function.

Better to have a couple of duplicated lines and properly rollback
from the right place instead of rollbacking supposed actions taken
from inside a function.

> +		return ret;
> +	}
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_NPS_TIMER_STARTING,
> +				"AP_NPS_TIMER_STARTING",
> +				nps_timer_starting_cpu,
> +				nps_timer_dying_cpu);
> +	if (ret) {
> +		pr_err("Failed to setup hotplug state");
> +		clk_disable_unprepare(clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clkevt, "ezchip,nps400-timer0",
> +		       nps_setup_clockevent);
> +#endif /* CONFIG_EZNPS_MTM_EXT */
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 34bd805..9efc1a3 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -60,6 +60,7 @@ enum cpuhp_state {
>  	CPUHP_AP_MARCO_TIMER_STARTING,
>  	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
>  	CPUHP_AP_ARC_TIMER_STARTING,
> +	CPUHP_AP_NPS_TIMER_STARTING,

Oops, wait. Here, arch/arc/kernel/time.c should be moved in
drivers/clocksource and consolidated with this driver.

Very likely, CPUHP_AP_ARC_TIMER_STARTING can be used for all ARC
timers.

>  	CPUHP_AP_KVM_STARTING,
>  	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
>  	CPUHP_AP_KVM_ARM_VGIC_STARTING,
> -- 
> 1.7.1
> 

[toc] | [next] | [standalone]

#1512750

Hi Daniel,

On 10/31/2016 10:03 AM, Noam Camus wrote:
>> Oops, wait. Here, arch/arc/kernel/time.c should be moved in drivers/clocksource and consolidated with this driver.
>> >Very likely, CPUHP_AP_ARC_TIMER_STARTING can be used for all ARC timers.
> Indeed the ARC timer driver served as my inspiration but due to HW threads handling they are not the same.
> Moving drivers from arch/arc to driver/clocksource is not my call (Vineet Gupta is the maintainer of ARC)
> And I think they quiet differ now so consolidation gain is not obvious.

Sorry this was on my todo list at low priority. Anyways took a quick look at it now.

The problem is timer code overlaps pure driver land and ARC specific code. e.g.
CONFIG_ARC_HAS_RTC clocksource needs to check whether hardware exists or not. We
currently conveniently check that from cpuinfo populated at boot time. Now either
I need to export that info as a function (which should be done anyways) - but then
define stub for !ARC. Either that or duplicate the detection code here (orig copy
in arc setup.c)

So doing this exercise cleanly would mean exporting much of
arch/arc/include/asm/{arcregs.h, mcip.h} out of ARC into include/soc/arc/*. Isn't
this kind of going back to include/asm-<arch>/* days ?

Honestly I do want to move the driver in the right place but not sure how to do
this cleanly.

Thoughts ?

-Vineet

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

#1512959 — [PATCH 7/9] ARC: breakout timer stuff into a seperate header

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/arcregs.h |  9 +--------
 arch/arc/kernel/time.c         |  2 +-
 include/soc/arc/timers.h       | 24 ++++++++++++++++++++++++
 3 files changed, 26 insertions(+), 9 deletions(-)
 create mode 100644 include/soc/arc/timers.h

diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
index ccf5dc96713b..a17aa4558832 100644
--- a/arch/arc/include/asm/arcregs.h
+++ b/arch/arc/include/asm/arcregs.h
@@ -20,7 +20,6 @@
 #define ARC_REG_FP_V2_BCR	0xc8	/* ARCv2 FPU */
 #define ARC_REG_SLC_BCR		0xce
 #define ARC_REG_DCCM_BUILD	0x74	/* DCCM size (common) */
-#define ARC_REG_TIMERS_BCR	0x75
 #define ARC_REG_AP_BCR		0x76
 #define ARC_REG_ICCM_BUILD	0x78	/* ICCM size (common) */
 #define ARC_REG_XY_MEM_BCR	0x79
@@ -206,13 +205,7 @@ struct bcr_fp_arcv2 {
 #endif
 };
 
-struct bcr_timer {
-#ifdef CONFIG_CPU_BIG_ENDIAN
-	unsigned int pad2:15, rtsc:1, pad1:5, rtc:1, t1:1, t0:1, ver:8;
-#else
-	unsigned int ver:8, t0:1, t1:1, rtc:1, pad1:5, rtsc:1, pad2:15;
-#endif
-};
+#include <soc/arc/timers.h>
 
 struct bcr_bpu_arcompact {
 #ifdef CONFIG_CPU_BIG_ENDIAN
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index f1ebe45bfcdf..dad3a9933c4c 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -38,8 +38,8 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <asm/irq.h>
-#include <asm/arcregs.h>
 
+#include <soc/arc/timers.h>
 #include <soc/arc/mcip.h>
 
 /* Timer related Aux registers */
diff --git a/include/soc/arc/timers.h b/include/soc/arc/timers.h
new file mode 100644
index 000000000000..79484fafabfa
--- /dev/null
+++ b/include/soc/arc/timers.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2016-17 Synopsys, Inc. (www.synopsys.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __SOC_ARC_TIMERS_H
+#define __SOC_ARC_TIMERS_H
+
+#include <soc/arc/aux.h>
+
+#define ARC_REG_TIMERS_BCR	0x75
+
+struct bcr_timer {
+#ifdef CONFIG_CPU_BIG_ENDIAN
+	unsigned int pad2:15, rtsc:1, pad1:5, rtc:1, t1:1, t0:1, ver:8;
+#else
+	unsigned int ver:8, t0:1, t1:1, rtc:1, pad1:5, rtsc:1, pad2:15;
+#endif
+};
+
+#endif
-- 
2.7.4

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

#1514717 — Re: [PATCH 7/9] ARC: breakout timer stuff into a seperate header

s/seperate/separate/

On Mon, Oct 31, 2016 at 03:48:14PM -0700, Vineet Gupta wrote:
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/include/asm/arcregs.h |  9 +--------
>  arch/arc/kernel/time.c         |  2 +-
>  include/soc/arc/timers.h       | 24 ++++++++++++++++++++++++
>  3 files changed, 26 insertions(+), 9 deletions(-)
>  create mode 100644 include/soc/arc/timers.h
> 
> diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
> index ccf5dc96713b..a17aa4558832 100644
> --- a/arch/arc/include/asm/arcregs.h
> +++ b/arch/arc/include/asm/arcregs.h
> @@ -20,7 +20,6 @@
>  #define ARC_REG_FP_V2_BCR	0xc8	/* ARCv2 FPU */
>  #define ARC_REG_SLC_BCR		0xce
>  #define ARC_REG_DCCM_BUILD	0x74	/* DCCM size (common) */
> -#define ARC_REG_TIMERS_BCR	0x75
>  #define ARC_REG_AP_BCR		0x76
>  #define ARC_REG_ICCM_BUILD	0x78	/* ICCM size (common) */
>  #define ARC_REG_XY_MEM_BCR	0x79
> @@ -206,13 +205,7 @@ struct bcr_fp_arcv2 {
>  #endif
>  };
>  
> -struct bcr_timer {
> -#ifdef CONFIG_CPU_BIG_ENDIAN
> -	unsigned int pad2:15, rtsc:1, pad1:5, rtc:1, t1:1, t0:1, ver:8;
> -#else
> -	unsigned int ver:8, t0:1, t1:1, rtc:1, pad1:5, rtsc:1, pad2:15;
> -#endif
> -};
> +#include <soc/arc/timers.h>
>  
>  struct bcr_bpu_arcompact {
>  #ifdef CONFIG_CPU_BIG_ENDIAN
> diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
> index f1ebe45bfcdf..dad3a9933c4c 100644
> --- a/arch/arc/kernel/time.c
> +++ b/arch/arc/kernel/time.c
> @@ -38,8 +38,8 @@
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <asm/irq.h>
> -#include <asm/arcregs.h>
>  
> +#include <soc/arc/timers.h>
>  #include <soc/arc/mcip.h>
>  
>  /* Timer related Aux registers */
> diff --git a/include/soc/arc/timers.h b/include/soc/arc/timers.h
> new file mode 100644
> index 000000000000..79484fafabfa
> --- /dev/null
> +++ b/include/soc/arc/timers.h
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (C) 2016-17 Synopsys, Inc. (www.synopsys.com)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __SOC_ARC_TIMERS_H
> +#define __SOC_ARC_TIMERS_H
> +
> +#include <soc/arc/aux.h>
> +
> +#define ARC_REG_TIMERS_BCR	0x75
> +
> +struct bcr_timer {
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +	unsigned int pad2:15, rtsc:1, pad1:5, rtc:1, t1:1, t0:1, ver:8;
> +#else
> +	unsigned int ver:8, t0:1, t1:1, rtc:1, pad1:5, rtsc:1, pad2:15;
> +#endif
> +};
> +
> +#endif
> -- 
> 2.7.4
> 

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

#1512960 — [PATCH 5/9] ARC: breakout aux handling into a seperate header

ARC timers use aux registers for programming and this paves way for
moving ARC timer drivers into drivers/clocksource

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/arcregs.h | 85 +-----------------------------------------
 arch/arc/include/asm/mcip.h    |  2 +-
 include/soc/arc/aux.h          | 52 ++++++++++++++++++++++++++
 include/soc/nps/common.h       |  4 +-
 4 files changed, 56 insertions(+), 87 deletions(-)
 create mode 100644 include/soc/arc/aux.h

diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
index 7f3f9f63708c..ccf5dc96713b 100644
--- a/arch/arc/include/asm/arcregs.h
+++ b/arch/arc/include/asm/arcregs.h
@@ -110,90 +110,7 @@
 
 #ifndef __ASSEMBLY__
 
-/*
- ******************************************************************
- *      Inline ASM macros to read/write AUX Regs
- *      Essentially invocation of lr/sr insns from "C"
- */
-
-#if 1
-
-#define read_aux_reg(reg)	__builtin_arc_lr(reg)
-
-/* gcc builtin sr needs reg param to be long immediate */
-#define write_aux_reg(reg_immed, val)		\
-		__builtin_arc_sr((unsigned int)(val), reg_immed)
-
-#else
-
-#define read_aux_reg(reg)		\
-({					\
-	unsigned int __ret;		\
-	__asm__ __volatile__(		\
-	"	lr    %0, [%1]"		\
-	: "=r"(__ret)			\
-	: "i"(reg));			\
-	__ret;				\
-})
-
-/*
- * Aux Reg address is specified as long immediate by caller
- * e.g.
- *    write_aux_reg(0x69, some_val);
- * This generates tightest code.
- */
-#define write_aux_reg(reg_imm, val)	\
-({					\
-	__asm__ __volatile__(		\
-	"	sr   %0, [%1]	\n"	\
-	:				\
-	: "ir"(val), "i"(reg_imm));	\
-})
-
-/*
- * Aux Reg address is specified in a variable
- *  * e.g.
- *      reg_num = 0x69
- *      write_aux_reg2(reg_num, some_val);
- * This has to generate glue code to load the reg num from
- *  memory to a reg hence not recommended.
- */
-#define write_aux_reg2(reg_in_var, val)		\
-({						\
-	unsigned int tmp;			\
-						\
-	__asm__ __volatile__(			\
-	"	ld   %0, [%2]	\n\t"		\
-	"	sr   %1, [%0]	\n\t"		\
-	: "=&r"(tmp)				\
-	: "r"(val), "memory"(&reg_in_var));	\
-})
-
-#endif
-
-#define READ_BCR(reg, into)				\
-{							\
-	unsigned int tmp;				\
-	tmp = read_aux_reg(reg);			\
-	if (sizeof(tmp) == sizeof(into)) {		\
-		into = *((typeof(into) *)&tmp);		\
-	} else {					\
-		extern void bogus_undefined(void);	\
-		bogus_undefined();			\
-	}						\
-}
-
-#define WRITE_AUX(reg, into)				\
-{							\
-	unsigned int tmp;				\
-	if (sizeof(tmp) == sizeof(into)) {		\
-		tmp = (*(unsigned int *)&(into));	\
-		write_aux_reg(reg, tmp);		\
-	} else  {					\
-		extern void bogus_undefined(void);	\
-		bogus_undefined();			\
-	}						\
-}
+#include <soc/arc/aux.h>
 
 /* Helpers */
 #define TO_KB(bytes)		((bytes) >> 10)
diff --git a/arch/arc/include/asm/mcip.h b/arch/arc/include/asm/mcip.h
index c8fbe4114bad..fc28d0944801 100644
--- a/arch/arc/include/asm/mcip.h
+++ b/arch/arc/include/asm/mcip.h
@@ -13,7 +13,7 @@
 
 #ifdef CONFIG_ISA_ARCV2
 
-#include <asm/arcregs.h>
+#include <soc/arc/aux.h>
 
 #define ARC_REG_MCIP_BCR	0x0d0
 #define ARC_REG_MCIP_CMD	0x600
diff --git a/include/soc/arc/aux.h b/include/soc/arc/aux.h
new file mode 100644
index 000000000000..8c41c096a51b
--- /dev/null
+++ b/include/soc/arc/aux.h
@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2016-2017 Synopsys, Inc. (www.synopsys.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __SOC_ARC_AUX_H__
+#define __SOC_ARC_AUX_H__
+
+#ifdef CONFIG_ARC
+
+#define read_aux_reg(r)		__builtin_arc_lr(r)
+
+/* gcc builtin sr needs reg param to be long immediate */
+#define write_aux_reg(r, v)	__builtin_arc_sr((unsigned int)(v), r)
+
+#else
+
+#define read_aux_reg(r)		0
+#define write_aux_reg(r, v)
+
+#endif
+
+#define READ_BCR(reg, into)				\
+{							\
+	unsigned int tmp;				\
+	tmp = read_aux_reg(reg);			\
+	if (sizeof(tmp) == sizeof(into)) {		\
+		into = *((typeof(into) *)&tmp);		\
+	} else {					\
+		extern void bogus_undefined(void);	\
+		bogus_undefined();			\
+	}						\
+}
+
+#define WRITE_AUX(reg, into)				\
+{							\
+	unsigned int tmp;				\
+	if (sizeof(tmp) == sizeof(into)) {		\
+		tmp = (*(unsigned int *)&(into));	\
+		write_aux_reg(reg, tmp);		\
+	} else  {					\
+		extern void bogus_undefined(void);	\
+		bogus_undefined();			\
+	}						\
+}
+
+
+#endif
diff --git a/include/soc/nps/common.h b/include/soc/nps/common.h
index 9b1d43d671a3..b7ba05b3993f 100644
--- a/include/soc/nps/common.h
+++ b/include/soc/nps/common.h
@@ -47,6 +47,8 @@
 
 #ifndef __ASSEMBLY__
 
+#include <soc/arc/aux.h>
+
 /* In order to increase compilation test coverage */
 #ifdef CONFIG_ARC
 static inline void nps_ack_gic(void)
@@ -59,8 +61,6 @@ static inline void nps_ack_gic(void)
 }
 #else
 static inline void nps_ack_gic(void) { }
-#define write_aux_reg(r, v)
-#define read_aux_reg(r) 0
 #endif
 
 /* CPU global ID */
-- 
2.7.4

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

#1513171 — RE: [PATCH 5/9] ARC: breakout aux handling into a seperate header

>From: Vineet Gupta [mailto:Vineet.Gupta1@synopsys.com] 
>Sent: Tuesday, November 1, 2016 12:48 AM

>ARC timers use aux registers for programming and this paves way for moving ARC timer drivers into drivers/clocksource

Maybe in this patch or just another one could you move from timer.c to the new soc header all timer related Aux registers definitions?
This could be used by timer-nps driver.
 
i.e.:
/* Timer related Aux registers */
#define ARC_REG_TIMER0_LIMIT    0x23    /* timer 0 limit */
#define ARC_REG_TIMER0_CTRL     0x22    /* timer 0 control */
#define ARC_REG_TIMER0_CNT      0x21    /* timer 0 count */
#define ARC_REG_TIMER1_LIMIT    0x102   /* timer 1 limit */
#define ARC_REG_TIMER1_CTRL     0x101   /* timer 1 control */
#define ARC_REG_TIMER1_CNT      0x100   /* timer 1 count */

#define TIMER_CTRL_IE   (1 << 0) /* Interrupt when Count reaches limit */
#define TIMER_CTRL_NH   (1 << 1) /* Count only when CPU NOT halted */

#define ARC_TIMER_MAX   0xFFFFFFFF

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

#1512961 — [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 0385df77a697..595d06900061 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -234,11 +234,11 @@ static char *arc_cpu_mumbojumbo(int cpu_id, char *buf, int len)
 		       is_isa_arcompact() ? "ARCompact" : "ARCv2",
 		       IS_AVAIL1(cpu->isa.be, "[Big-Endian]"));
 
-	n += scnprintf(buf + n, len - n, "Timers\t\t: %s%s%s%s\nISA Extn\t: ",
+	n += scnprintf(buf + n, len - n, "Timers\t\t: %s%s%s%s%s%s\nISA Extn\t: ",
 		       IS_AVAIL1(cpu->extn.timer0, "Timer0 "),
 		       IS_AVAIL1(cpu->extn.timer1, "Timer1 "),
-		       IS_AVAIL2(cpu->extn.rtc, "Local-64-bit-Ctr ",
-				 CONFIG_ARC_HAS_RTC));
+		       IS_AVAIL2(cpu->extn.rtc, "RTC [UP 64-bit] ", CONFIG_ARC_HAS_RTC),
+		       IS_AVAIL2(cpu->extn.gfrc, "GFRC [SMP 64-bit] ", CONFIG_ARC_HAS_GFRC));
 
 	n += i = scnprintf(buf + n, len - n, "%s%s%s%s%s",
 			   IS_AVAIL2(cpu->isa.atomic, "atomic ", CONFIG_ARC_HAS_LLSC),
-- 
2.7.4

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

#1514709 — Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers

On Mon, Oct 31, 2016 at 03:48:10PM -0700, Vineet Gupta wrote:
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>

Why not add a message in drivers/clocksource/clksrc-probe.c when a timer inits
successfully ? and then get rid of this code.

> ---
>  arch/arc/kernel/setup.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index 0385df77a697..595d06900061 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -234,11 +234,11 @@ static char *arc_cpu_mumbojumbo(int cpu_id, char *buf, int len)
>  		       is_isa_arcompact() ? "ARCompact" : "ARCv2",
>  		       IS_AVAIL1(cpu->isa.be, "[Big-Endian]"));
>  
> -	n += scnprintf(buf + n, len - n, "Timers\t\t: %s%s%s%s\nISA Extn\t: ",
> +	n += scnprintf(buf + n, len - n, "Timers\t\t: %s%s%s%s%s%s\nISA Extn\t: ",
>  		       IS_AVAIL1(cpu->extn.timer0, "Timer0 "),
>  		       IS_AVAIL1(cpu->extn.timer1, "Timer1 "),
> -		       IS_AVAIL2(cpu->extn.rtc, "Local-64-bit-Ctr ",
> -				 CONFIG_ARC_HAS_RTC));
> +		       IS_AVAIL2(cpu->extn.rtc, "RTC [UP 64-bit] ", CONFIG_ARC_HAS_RTC),
> +		       IS_AVAIL2(cpu->extn.gfrc, "GFRC [SMP 64-bit] ", CONFIG_ARC_HAS_GFRC));
>  
>  	n += i = scnprintf(buf + n, len - n, "%s%s%s%s%s",
>  			   IS_AVAIL2(cpu->isa.atomic, "atomic ", CONFIG_ARC_HAS_LLSC),
> -- 
> 2.7.4
> 

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

#1514739 — Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers

On 11/03/2016 10:09 AM, Daniel Lezcano wrote:
> On Mon, Oct 31, 2016 at 03:48:10PM -0700, Vineet Gupta wrote:
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> 
> Why not add a message in drivers/clocksource/clksrc-probe.c when a timer inits
> successfully ? and then get rid of this code.

At boot I have bunch of cod which prints all the major hardware blocks and thus
would like to print it. We also print if a feature is present but not supported
etc. So I like to keep all of that here.

Also same code is used for /proc/cpuinfo.

> 
>> ---
>>  arch/arc/kernel/setup.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
>> index 0385df77a697..595d06900061 100644
>> --- a/arch/arc/kernel/setup.c
>> +++ b/arch/arc/kernel/setup.c
>> @@ -234,11 +234,11 @@ static char *arc_cpu_mumbojumbo(int cpu_id, char *buf, int len)
>>  		       is_isa_arcompact() ? "ARCompact" : "ARCv2",
>>  		       IS_AVAIL1(cpu->isa.be, "[Big-Endian]"));
>>  
>> -	n += scnprintf(buf + n, len - n, "Timers\t\t: %s%s%s%s\nISA Extn\t: ",
>> +	n += scnprintf(buf + n, len - n, "Timers\t\t: %s%s%s%s%s%s\nISA Extn\t: ",
>>  		       IS_AVAIL1(cpu->extn.timer0, "Timer0 "),
>>  		       IS_AVAIL1(cpu->extn.timer1, "Timer1 "),
>> -		       IS_AVAIL2(cpu->extn.rtc, "Local-64-bit-Ctr ",
>> -				 CONFIG_ARC_HAS_RTC));
>> +		       IS_AVAIL2(cpu->extn.rtc, "RTC [UP 64-bit] ", CONFIG_ARC_HAS_RTC),
>> +		       IS_AVAIL2(cpu->extn.gfrc, "GFRC [SMP 64-bit] ", CONFIG_ARC_HAS_GFRC));
>>  
>>  	n += i = scnprintf(buf + n, len - n, "%s%s%s%s%s",
>>  			   IS_AVAIL2(cpu->isa.atomic, "atomic ", CONFIG_ARC_HAS_LLSC),
>> -- 
>> 2.7.4
>>
> 

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

#1514752 — Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers

On Thu, Nov 03, 2016 at 10:47:19AM -0700, Vineet Gupta wrote:
> On 11/03/2016 10:09 AM, Daniel Lezcano wrote:
> > On Mon, Oct 31, 2016 at 03:48:10PM -0700, Vineet Gupta wrote:
> >> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> > 
> > Why not add a message in drivers/clocksource/clksrc-probe.c when a timer inits
> > successfully ? and then get rid of this code.
> 
> At boot I have bunch of cod which prints all the major hardware blocks and thus
> would like to print it. We also print if a feature is present but not supported
> etc. So I like to keep all of that here.
> 
> Also same code is used for /proc/cpuinfo.

Ok, I see. Thanks for the clarification.

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

#1512964 — [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether hardware exists ...

... don't rely on cpuinfo populated in arc boot code. This paves wat for
moving this code in drivers/clocksource/

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/kernel/time.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index f927b8dc6edd..a2db010cde18 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -118,10 +118,11 @@ static struct clocksource arc_counter_gfrc = {
 
 static int __init arc_cs_setup_gfrc(struct device_node *node)
 {
-	int exists = cpuinfo_arc700[0].extn.gfrc;
+	struct mcip_bcr mp;
 	int ret;
 
-	if (WARN(!exists, "Global-64-bit-Ctr clocksource not detected"))
+	READ_BCR(ARC_REG_MCIP_BCR, mp);
+	if (WARN(!mp.gfrc, "Global-64-bit-Ctr clocksource not detected"))
 		return -ENXIO;
 
 	ret = arc_get_timer_clk(node);
@@ -174,10 +175,11 @@ static struct clocksource arc_counter_rtc = {
 
 static int __init arc_cs_setup_rtc(struct device_node *node)
 {
-	int exists = cpuinfo_arc700[smp_processor_id()].extn.rtc;
+	struct bcr_timer timer;
 	int ret;
 
-	if (WARN(!exists, "Local-64-bit-Ctr clocksource not detected"))
+	READ_BCR(ARC_REG_TIMERS_BCR, timer);
+	if (WARN(!timer.rtc, "Local-64-bit-Ctr clocksource not detected"))
 		return -ENXIO;
 
 	/* Local to CPU hence not usable in SMP */
-- 
2.7.4

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

#1514702 — Re: [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether hardware exists ...

On Mon, Oct 31, 2016 at 03:48:08PM -0700, Vineet Gupta wrote:
> ... don't rely on cpuinfo populated in arc boot code. This paves wat for
> moving this code in drivers/clocksource/
> 
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/kernel/time.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
> index f927b8dc6edd..a2db010cde18 100644
> --- a/arch/arc/kernel/time.c
> +++ b/arch/arc/kernel/time.c
> @@ -118,10 +118,11 @@ static struct clocksource arc_counter_gfrc = {
>  
>  static int __init arc_cs_setup_gfrc(struct device_node *node)
>  {
> -	int exists = cpuinfo_arc700[0].extn.gfrc;
> +	struct mcip_bcr mp;
>  	int ret;
>  
> -	if (WARN(!exists, "Global-64-bit-Ctr clocksource not detected"))
> +	READ_BCR(ARC_REG_MCIP_BCR, mp);
> +	if (WARN(!mp.gfrc, "Global-64-bit-Ctr clocksource not detected"))

Take the opportunity to replace this WARN by a pr_err.

>  		return -ENXIO;
>  
>  	ret = arc_get_timer_clk(node);
> @@ -174,10 +175,11 @@ static struct clocksource arc_counter_rtc = {
>  
>  static int __init arc_cs_setup_rtc(struct device_node *node)
>  {
> -	int exists = cpuinfo_arc700[smp_processor_id()].extn.rtc;
> +	struct bcr_timer timer;
>  	int ret;
>  
> -	if (WARN(!exists, "Local-64-bit-Ctr clocksource not detected"))
> +	READ_BCR(ARC_REG_TIMERS_BCR, timer);
> +	if (WARN(!timer.rtc, "Local-64-bit-Ctr clocksource not detected"))
>  		return -ENXIO;o

Ditto and                    ^^

So the READ_BCR() is only there to check the timer is physically present ?

>  	/* Local to CPU hence not usable in SMP */
> -- 
> 2.7.4
> 

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

#1514745 — Re: [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether hardware exists ...

On 11/03/2016 10:00 AM, Daniel Lezcano wrote:
> On Mon, Oct 31, 2016 at 03:48:08PM -0700, Vineet Gupta wrote:
>> ... don't rely on cpuinfo populated in arc boot code. This paves wat for
>> moving this code in drivers/clocksource/
>>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
>>  arch/arc/kernel/time.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
>> index f927b8dc6edd..a2db010cde18 100644
>> --- a/arch/arc/kernel/time.c
>> +++ b/arch/arc/kernel/time.c
>> @@ -118,10 +118,11 @@ static struct clocksource arc_counter_gfrc = {
>>  
>>  static int __init arc_cs_setup_gfrc(struct device_node *node)
>>  {
>> -	int exists = cpuinfo_arc700[0].extn.gfrc;
>> +	struct mcip_bcr mp;
>>  	int ret;
>>  
>> -	if (WARN(!exists, "Global-64-bit-Ctr clocksource not detected"))
>> +	READ_BCR(ARC_REG_MCIP_BCR, mp);
>> +	if (WARN(!mp.gfrc, "Global-64-bit-Ctr clocksource not detected"))
> 
> Take the opportunity to replace this WARN by a pr_err.

OK.

> 
>>  		return -ENXIO;
>>  
>>  	ret = arc_get_timer_clk(node);
>> @@ -174,10 +175,11 @@ static struct clocksource arc_counter_rtc = {
>>  
>>  static int __init arc_cs_setup_rtc(struct device_node *node)
>>  {
>> -	int exists = cpuinfo_arc700[smp_processor_id()].extn.rtc;
>> +	struct bcr_timer timer;
>>  	int ret;
>>  
>> -	if (WARN(!exists, "Local-64-bit-Ctr clocksource not detected"))
>> +	READ_BCR(ARC_REG_TIMERS_BCR, timer);
>> +	if (WARN(!timer.rtc, "Local-64-bit-Ctr clocksource not detected"))
>>  		return -ENXIO;o
> 
> Ditto and                    ^^
> 
> So the READ_BCR() is only there to check the timer is physically present ?

Yep, due to configurable nature of cores, we have Build Config Registers to detect
at runtime what is present or not. This allows for boot printing at the minimum.
This is defined in arcregs.h and in newly introduced soc/arc/aux.h

> 
>>  	/* Local to CPU hence not usable in SMP */
>> -- 
>> 2.7.4
>>
> 

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

#1512965 — [PATCH 8/9] ARC: timer: rename config options

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/Kconfig                           | 4 ++--
 arch/arc/configs/nsimosci_hs_smp_defconfig | 2 +-
 arch/arc/configs/vdk_hs38_smp_defconfig    | 2 +-
 arch/arc/kernel/setup.c                    | 4 ++--
 arch/arc/kernel/time.c                     | 4 ++--
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index bd204bfa29ed..b4499f97035a 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -410,12 +410,12 @@ config ARC_HAS_DIV_REM
 	bool "Insn: div, divu, rem, remu"
 	default y
 
-config ARC_HAS_RTC
+config ARC_TIMER_RTC
 	bool "Local 64-bit r/o cycle counter"
 	default n
 	depends on !SMP
 
-config ARC_HAS_GFRC
+config ARC_TIMER_GFRC
 	bool "SMP synchronized 64-bit cycle counter"
 	default y
 	depends on SMP
diff --git a/arch/arc/configs/nsimosci_hs_smp_defconfig b/arch/arc/configs/nsimosci_hs_smp_defconfig
index 6da71ba253a9..9782fe512244 100644
--- a/arch/arc/configs/nsimosci_hs_smp_defconfig
+++ b/arch/arc/configs/nsimosci_hs_smp_defconfig
@@ -21,7 +21,7 @@ CONFIG_MODULES=y
 CONFIG_ARC_PLAT_SIM=y
 CONFIG_ISA_ARCV2=y
 CONFIG_SMP=y
-# CONFIG_ARC_HAS_GFRC is not set
+# CONFIG_ARC_TIMER_GFRC is not set
 CONFIG_ARC_BUILTIN_DTB_NAME="nsimosci_hs_idu"
 CONFIG_PREEMPT=y
 # CONFIG_COMPACTION is not set
diff --git a/arch/arc/configs/vdk_hs38_smp_defconfig b/arch/arc/configs/vdk_hs38_smp_defconfig
index 969b206d6c67..a927389d02d8 100644
--- a/arch/arc/configs/vdk_hs38_smp_defconfig
+++ b/arch/arc/configs/vdk_hs38_smp_defconfig
@@ -15,7 +15,7 @@ CONFIG_ARC_PLAT_AXS10X=y
 CONFIG_AXS103=y
 CONFIG_ISA_ARCV2=y
 CONFIG_SMP=y
-# CONFIG_ARC_HAS_GFRC is not set
+# CONFIG_ARC_TIMER_GFRC is not set
 CONFIG_ARC_UBOOT_SUPPORT=y
 CONFIG_ARC_BUILTIN_DTB_NAME="vdk_hs38_smp"
 CONFIG_PREEMPT=y
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 5865bd34a7fa..4855335b956b 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -239,8 +239,8 @@ static char *arc_cpu_mumbojumbo(int cpu_id, char *buf, int len)
 	n += scnprintf(buf + n, len - n, "Timers\t\t: %s%s%s%s%s%s\nISA Extn\t: ",
 		       IS_AVAIL1(cpu->extn.timer0, "Timer0 "),
 		       IS_AVAIL1(cpu->extn.timer1, "Timer1 "),
-		       IS_AVAIL2(cpu->extn.rtc, "RTC [UP 64-bit] ", CONFIG_ARC_HAS_RTC),
-		       IS_AVAIL2(cpu->extn.gfrc, "GFRC [SMP 64-bit] ", CONFIG_ARC_HAS_GFRC));
+		       IS_AVAIL2(cpu->extn.rtc, "RTC [UP 64-bit] ", CONFIG_ARC_TIMER_RTC),
+		       IS_AVAIL2(cpu->extn.gfrc, "GFRC [SMP 64-bit] ", CONFIG_ARC_TIMER_GFRC));
 
 	n += i = scnprintf(buf + n, len - n, "%s%s%s%s%s",
 			   IS_AVAIL2(cpu->isa.atomic, "atomic ", CONFIG_ARC_HAS_LLSC),
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index dad3a9933c4c..676b14b7a9be 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -81,7 +81,7 @@ static int noinline arc_get_timer_clk(struct device_node *node)
 
 /********** Clock Source Device *********/
 
-#ifdef CONFIG_ARC_HAS_GFRC
+#ifdef CONFIG_ARC_TIMER_GFRC
 
 static cycle_t arc_read_gfrc(struct clocksource *cs)
 {
@@ -135,7 +135,7 @@ CLOCKSOURCE_OF_DECLARE(arc_gfrc, "snps,archs-timer-gfrc", arc_cs_setup_gfrc);
 
 #endif
 
-#ifdef CONFIG_ARC_HAS_RTC
+#ifdef CONFIG_ARC_TIMER_RTC
 
 #define AUX_RTC_CTRL	0x103
 #define AUX_RTC_LOW	0x104
-- 
2.7.4

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

#1512966 — [PATCH 0/9] Move ARC timer code into drivers/clocksource/

Hi,

This series addresses the long pending move of ARC timer code into
drivers/clocksource/.

- patches [1-4]/9 are improvements to arc code, paving way for later code motion.
- patches [5-8]/9 refactor the arc headers to be shared between arc and drivers
- patch 9/9 moves out the driver code/build/Kconfig bits

As review progresses/concludes, I'd like to merge [1-8] for this merge window and
clocksource maintainers can pick up the actual switch for 4.10 or even 4.9 as they
prefer.

Thx,
-Vineet

 MAINTAINERS                                        |  1 +
 arch/arc/Kconfig                                   | 12 +--
 arch/arc/configs/nsimosci_hs_smp_defconfig         |  2 +-
 arch/arc/configs/vdk_hs38_smp_defconfig            |  2 +-
 arch/arc/include/asm/arcregs.h                     | 94 +---------------------
 arch/arc/kernel/Makefile                           |  2 +-
 arch/arc/kernel/mcip.c                             |  2 +-
 arch/arc/kernel/setup.c                            | 17 +++-
 arch/arc/plat-axs10x/axs10x.c                      |  2 +-
 drivers/clocksource/Kconfig                        | 24 ++++++
 drivers/clocksource/Makefile                       |  1 +
 .../time.c => drivers/clocksource/arc_timer.c      | 69 +++++-----------
 include/soc/arc/aux.h                              | 52 ++++++++++++
 {arch/arc/include/asm => include/soc/arc}/mcip.h   | 10 +--
 include/soc/arc/timers.h                           | 24 ++++++
 include/soc/nps/common.h                           |  4 +-
 16 files changed, 151 insertions(+), 167 deletions(-)
 rename arch/arc/kernel/time.c => drivers/clocksource/arc_timer.c (83%)
 create mode 100644 include/soc/arc/aux.h
 rename {arch/arc/include/asm => include/soc/arc}/mcip.h (95%)
 create mode 100644 include/soc/arc/timers.h

-- 
2.7.4

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

#1512969 — [PATCH 9/9] clocksource: import ARC timer driver

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 MAINTAINERS                                        |  1 +
 arch/arc/Kconfig                                   | 12 +--------
 arch/arc/kernel/Makefile                           |  2 +-
 drivers/clocksource/Kconfig                        | 24 ++++++++++++++++++
 drivers/clocksource/Makefile                       |  1 +
 .../time.c => drivers/clocksource/arc_timer.c      | 29 ++++++----------------
 6 files changed, 35 insertions(+), 34 deletions(-)
 rename arch/arc/kernel/time.c => drivers/clocksource/arc_timer.c (90%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d838cf49f81..57b56ff1dd68 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11632,6 +11632,7 @@ S:	Supported
 F:	arch/arc/
 F:	Documentation/devicetree/bindings/arc/*
 F:	Documentation/devicetree/bindings/interrupt-controller/snps,arc*
+F:	drivers/clocksource/arc_timer.c
 F:	drivers/tty/serial/arc_uart.c
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/vgupta/arc.git
 
diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index b4499f97035a..8768a509d5e7 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -8,9 +8,9 @@
 
 config ARC
 	def_bool y
+	select ARC_TIMER
 	select ARCH_SUPPORTS_ATOMIC_RMW if ARC_HAS_LLSC
 	select BUILDTIME_EXTABLE_SORT
-	select CLKSRC_OF
 	select CLONE_BACKWARDS
 	select COMMON_CLK
 	select GENERIC_ATOMIC64 if !ISA_ARCV2 || !(ARC_HAS_LL64 && ARC_HAS_LLSC)
@@ -410,16 +410,6 @@ config ARC_HAS_DIV_REM
 	bool "Insn: div, divu, rem, remu"
 	default y
 
-config ARC_TIMER_RTC
-	bool "Local 64-bit r/o cycle counter"
-	default n
-	depends on !SMP
-
-config ARC_TIMER_GFRC
-	bool "SMP synchronized 64-bit cycle counter"
-	default y
-	depends on SMP
-
 config ARC_NUMBER_OF_INTERRUPTS
 	int "Number of interrupts"
 	range 8 240
diff --git a/arch/arc/kernel/Makefile b/arch/arc/kernel/Makefile
index cfcdedf52ff8..8942c5c3b4c5 100644
--- a/arch/arc/kernel/Makefile
+++ b/arch/arc/kernel/Makefile
@@ -8,7 +8,7 @@
 # Pass UTS_MACHINE for user_regset definition
 CFLAGS_ptrace.o		+= -DUTS_MACHINE='"$(UTS_MACHINE)"'
 
-obj-y	:= arcksyms.o setup.o irq.o time.o reset.o ptrace.o process.o devtree.o
+obj-y	:= arcksyms.o setup.o irq.o reset.o ptrace.o process.o devtree.o
 obj-y	+= signal.o traps.o sys.o troubleshoot.o stacktrace.o disasm.o
 obj-$(CONFIG_ISA_ARCOMPACT)		+= entry-compact.o intc-compact.o
 obj-$(CONFIG_ISA_ARCV2)			+= entry-arcv2.o intc-arcv2.o
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index e2c6e43cf8ca..73feaadc1924 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -282,6 +282,30 @@ config CLKSRC_MPS2
 	select CLKSRC_MMIO
 	select CLKSRC_OF
 
+config ARC_TIMER
+	bool "Enable timers for ARC Cores"
+	select CLKSRC_OF if OF
+
+if ARC_TIMER
+
+config ARC_TIMER_RTC
+	bool "64-bit cycle counter in HS38 cores"
+	default y if ARC
+	depends on !SMP
+	help
+	  This counter provides 64-bit resolution vs. the 32-bit TIMER1.
+	  It is implemented inside the core thus can't be used in SMP systems
+
+config ARC_TIMER_GFRC
+	bool "64-bit cycle counter in ARConnect block in HS38x cores"
+	default y if ARC
+	depends on SMP
+	help
+	  This counter can be used as clocksource in SMP HS38 SoCs.
+	  It sits outside the core thus can be used in SMP systems
+
+endif
+
 config ARM_ARCH_TIMER
 	bool
 	select CLKSRC_OF if OF
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index cf87f407f1ad..e78480cb47f4 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -69,3 +69,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_ARC_TIMER)			+= arc_timer.o
diff --git a/arch/arc/kernel/time.c b/drivers/clocksource/arc_timer.c
similarity index 90%
rename from arch/arc/kernel/time.c
rename to drivers/clocksource/arc_timer.c
index 676b14b7a9be..ec37b6c5e903 100644
--- a/arch/arc/kernel/time.c
+++ b/drivers/clocksource/arc_timer.c
@@ -1,32 +1,18 @@
 /*
+ * Copyright (C) 2016-17 Synopsys, Inc. (www.synopsys.com)
  * Copyright (C) 2004, 2007-2010, 2011-2012 Synopsys, Inc. (www.synopsys.com)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
- *
- * vineetg: Jan 1011
- *  -sched_clock( ) no longer jiffies based. Uses the same clocksource
- *   as gtod
- *
- * Rajeshwarr/Vineetg: Mar 2008
- *  -Implemented CONFIG_GENERIC_TIME (rather deleted arch specific code)
- *   for arch independent gettimeofday()
- *  -Implemented CONFIG_GENERIC_CLOCKEVENTS as base for hrtimers
- *
- * Vineetg: Mar 2008: Forked off from time.c which now is time-jiff.c
  */
 
-/* ARC700 has two 32bit independent prog Timers: TIMER0 and TIMER1
- * Each can programmed to go from @count to @limit and optionally
- * interrupt when that happens.
- * A write to Control Register clears the Interrupt
- *
- * We've designated TIMER0 for events (clockevents)
- * while TIMER1 for free running (clocksource)
+/* ARC700 has two 32bit independent prog Timers: TIMER0 and TIMER1, Each can be
+ * programmed to go from @count to @limit and optionally interrupt.
+ * We've designated TIMER0 for clockevents and TIMER1 for clocksource
  *
- * Newer ARC700 cores have 64bit clk fetching RTSC insn, preferred over TIMER1
- * which however is currently broken
+ * ARCv2 based HS38 cores have RTC (in-core) and GFRC (inside ARConnect/MCIP)
+ * which are suitable for UP and SMP based clocksources respectively
  */
 
 #include <linux/interrupt.h>
@@ -37,7 +23,6 @@
 #include <linux/cpu.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
-#include <asm/irq.h>
 
 #include <soc/arc/timers.h>
 #include <soc/arc/mcip.h>
@@ -281,7 +266,7 @@ static irqreturn_t timer_irq_handler(int irq, void *dev_id)
 	 * irq_set_chip_and_handler() asked for handle_percpu_devid_irq()
 	 */
 	struct clock_event_device *evt = this_cpu_ptr(&arc_clockevent_device);
-	int irq_reenable = clockevent_state_periodic(evt);
+	int irq_reenable __maybe_unused = clockevent_state_periodic(evt);
 
 	/*
 	 * Any write to CTRL reg ACks the interrupt, we rewrite the
-- 
2.7.4

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

#1513028 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On 10/31/2016 05:01 PM, kbuild test robot wrote:
> Hi Vineet,
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.9-rc3]
> [cannot apply to arc/for-next]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]
> 
> url:    https://github.com/0day-ci/linux/commits/Vineet-Gupta/Move-ARC-timer-code-into-drivers-clocksource/20161101-065452
> config: ia64-allmodconfig (attached as .config)
> compiler: ia64-linux-gcc (GCC) 6.2.0
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=ia64 
> 
> All error/warnings (new ones prefixed by >>):
> 
>>> >> drivers/clocksource/arc_timer.c:237:19: warning: 'struct clock_event_device' declared inside parameter list will not be visible outside of this definition or declaration
>                struct clock_event_device *dev)
>                       ^~~~~~~~~~~~~~~~~~
>    drivers/clocksource/arc_timer.c:243:45: warning: 'struct clock_event_device' declared inside parameter list will not be visible outside of this definition or declaration

So I'd build tested the series for ARM (32 bit) and x86 (64 bit).
Seems this requires a depends on GENERIC_CLOCKEVENTS. Will fix it along with other
comments !

Thx for reporting.
-Vineet

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

#1513496 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On Mon, Oct 31, 2016 at 03:48:16PM -0700, Vineet Gupta wrote:
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  MAINTAINERS                                        |  1 +
>  arch/arc/Kconfig                                   | 12 +--------
>  arch/arc/kernel/Makefile                           |  2 +-
>  drivers/clocksource/Kconfig                        | 24 ++++++++++++++++++
>  drivers/clocksource/Makefile                       |  1 +
>  .../time.c => drivers/clocksource/arc_timer.c      | 29 ++++++----------------
>  6 files changed, 35 insertions(+), 34 deletions(-)
>  rename arch/arc/kernel/time.c => drivers/clocksource/arc_timer.c (90%)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3d838cf49f81..57b56ff1dd68 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11632,6 +11632,7 @@ S:	Supported
>  F:	arch/arc/
>  F:	Documentation/devicetree/bindings/arc/*
>  F:	Documentation/devicetree/bindings/interrupt-controller/snps,arc*
> +F:	drivers/clocksource/arc_timer.c
>  F:	drivers/tty/serial/arc_uart.c
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/vgupta/arc.git
>  
> diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
> index b4499f97035a..8768a509d5e7 100644
> --- a/arch/arc/Kconfig
> +++ b/arch/arc/Kconfig
> @@ -8,9 +8,9 @@
>  
>  config ARC
>  	def_bool y
> +	select ARC_TIMER
>  	select ARCH_SUPPORTS_ATOMIC_RMW if ARC_HAS_LLSC
>  	select BUILDTIME_EXTABLE_SORT
> -	select CLKSRC_OF
>  	select CLONE_BACKWARDS
>  	select COMMON_CLK
>  	select GENERIC_ATOMIC64 if !ISA_ARCV2 || !(ARC_HAS_LL64 && ARC_HAS_LLSC)
> @@ -410,16 +410,6 @@ config ARC_HAS_DIV_REM
>  	bool "Insn: div, divu, rem, remu"
>  	default y
>  
> -config ARC_TIMER_RTC
> -	bool "Local 64-bit r/o cycle counter"
> -	default n
> -	depends on !SMP
> -
> -config ARC_TIMER_GFRC
> -	bool "SMP synchronized 64-bit cycle counter"
> -	default y
> -	depends on SMP
> -
>  config ARC_NUMBER_OF_INTERRUPTS
>  	int "Number of interrupts"
>  	range 8 240
> diff --git a/arch/arc/kernel/Makefile b/arch/arc/kernel/Makefile
> index cfcdedf52ff8..8942c5c3b4c5 100644
> --- a/arch/arc/kernel/Makefile
> +++ b/arch/arc/kernel/Makefile
> @@ -8,7 +8,7 @@
>  # Pass UTS_MACHINE for user_regset definition
>  CFLAGS_ptrace.o		+= -DUTS_MACHINE='"$(UTS_MACHINE)"'
>  
> -obj-y	:= arcksyms.o setup.o irq.o time.o reset.o ptrace.o process.o devtree.o
> +obj-y	:= arcksyms.o setup.o irq.o reset.o ptrace.o process.o devtree.o
>  obj-y	+= signal.o traps.o sys.o troubleshoot.o stacktrace.o disasm.o
>  obj-$(CONFIG_ISA_ARCOMPACT)		+= entry-compact.o intc-compact.o
>  obj-$(CONFIG_ISA_ARCV2)			+= entry-arcv2.o intc-arcv2.o
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index e2c6e43cf8ca..73feaadc1924 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -282,6 +282,30 @@ config CLKSRC_MPS2
>  	select CLKSRC_MMIO
>  	select CLKSRC_OF
>  
> +config ARC_TIMER
> +	bool "Enable timers for ARC Cores"
> +	select CLKSRC_OF if OF
> +
> +if ARC_TIMER
> +
> +config ARC_TIMER_RTC
> +	bool "64-bit cycle counter in HS38 cores"
> +	default y if ARC
> +	depends on !SMP
> +	help
> +	  This counter provides 64-bit resolution vs. the 32-bit TIMER1.
> +	  It is implemented inside the core thus can't be used in SMP systems
> +
> +config ARC_TIMER_GFRC
> +	bool "64-bit cycle counter in ARConnect block in HS38x cores"
> +	default y if ARC
> +	depends on SMP
> +	help
> +	  This counter can be used as clocksource in SMP HS38 SoCs.
> +	  It sits outside the core thus can be used in SMP systems
> +
> +endif
> +

Please stay consistent with the rest of the Kconfig.

config ARC_TIMER_RTC
	bool "64-bit cycle counter in HS38 cores" if COMPILE_TEST
	select CLKSRC_OF
	help
	  This counter provides 64-bit resolution vs. the 32-bit TIMER1.
	  It is implemented inside the core thus can't be used in SMP systems.

config ARC_TIMER_GFRC
	bool "64-bit cycle counter in ARConnect block in HS38x cores" if COMPILE_TEST
	select CLKSRC_OF
	help
	  This counter can be used as clocksource in SMP HS38 SoCs.
	  It sits outside the core thus can be used in SMP systems


Then in the ARC's Kconfig you select ARC_TIMER_RTC or ARC_TIMER_GFRC depending
it is SMP or not.

One question:

Why ARC_TIMER_RTC can't be used in a SMP system ? Doesn't have each core its
own clocksource ? It seems you are assuming a clocksource can be used on SMP
only if the clocksource is unique and shared across the cores.


>  config ARM_ARCH_TIMER
>  	bool
>  	select CLKSRC_OF if OF
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index cf87f407f1ad..e78480cb47f4 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -69,3 +69,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_ARC_TIMER)			+= arc_timer.o
> diff --git a/arch/arc/kernel/time.c b/drivers/clocksource/arc_timer.c
> similarity index 90%
> rename from arch/arc/kernel/time.c
> rename to drivers/clocksource/arc_timer.c
> index 676b14b7a9be..ec37b6c5e903 100644
> --- a/arch/arc/kernel/time.c
> +++ b/drivers/clocksource/arc_timer.c
> @@ -1,32 +1,18 @@
>  /*
> + * Copyright (C) 2016-17 Synopsys, Inc. (www.synopsys.com)
>   * Copyright (C) 2004, 2007-2010, 2011-2012 Synopsys, Inc. (www.synopsys.com)
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
> - *
> - * vineetg: Jan 1011
> - *  -sched_clock( ) no longer jiffies based. Uses the same clocksource
> - *   as gtod
> - *
> - * Rajeshwarr/Vineetg: Mar 2008
> - *  -Implemented CONFIG_GENERIC_TIME (rather deleted arch specific code)
> - *   for arch independent gettimeofday()
> - *  -Implemented CONFIG_GENERIC_CLOCKEVENTS as base for hrtimers
> - *
> - * Vineetg: Mar 2008: Forked off from time.c which now is time-jiff.c
>   */
>  
> -/* ARC700 has two 32bit independent prog Timers: TIMER0 and TIMER1
> - * Each can programmed to go from @count to @limit and optionally
> - * interrupt when that happens.
> - * A write to Control Register clears the Interrupt
> - *
> - * We've designated TIMER0 for events (clockevents)
> - * while TIMER1 for free running (clocksource)
> +/* ARC700 has two 32bit independent prog Timers: TIMER0 and TIMER1, Each can be
> + * programmed to go from @count to @limit and optionally interrupt.
> + * We've designated TIMER0 for clockevents and TIMER1 for clocksource
>   *
> - * Newer ARC700 cores have 64bit clk fetching RTSC insn, preferred over TIMER1
> - * which however is currently broken
> + * ARCv2 based HS38 cores have RTC (in-core) and GFRC (inside ARConnect/MCIP)
> + * which are suitable for UP and SMP based clocksources respectively
>   */
>  
>  #include <linux/interrupt.h>
> @@ -37,7 +23,6 @@
>  #include <linux/cpu.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
> -#include <asm/irq.h>
>  
>  #include <soc/arc/timers.h>
>  #include <soc/arc/mcip.h>
> @@ -281,7 +266,7 @@ static irqreturn_t timer_irq_handler(int irq, void *dev_id)
>  	 * irq_set_chip_and_handler() asked for handle_percpu_devid_irq()
>  	 */
>  	struct clock_event_device *evt = this_cpu_ptr(&arc_clockevent_device);
> -	int irq_reenable = clockevent_state_periodic(evt);
> +	int irq_reenable __maybe_unused = clockevent_state_periodic(evt);
>  
>  	/*
>  	 * Any write to CTRL reg ACks the interrupt, we rewrite the
> -- 
> 2.7.4
> 

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

#1513499 — Re: [PATCH 9/9] clocksource: import ARC timer driver

Hi Daniel,

On 11/01/2016 01:42 PM, Daniel Lezcano wrote:
> Please stay consistent with the rest of the Kconfig.
> 
> config ARC_TIMER_RTC
> 	bool "64-bit cycle counter in HS38 cores" if COMPILE_TEST
> 	select CLKSRC_OF
> 	help
> 	  This counter provides 64-bit resolution vs. the 32-bit TIMER1.
> 	  It is implemented inside the core thus can't be used in SMP systems.
> 
> config ARC_TIMER_GFRC
> 	bool "64-bit cycle counter in ARConnect block in HS38x cores" if COMPILE_TEST
> 	select CLKSRC_OF
> 	help
> 	  This counter can be used as clocksource in SMP HS38 SoCs.
> 	  It sits outside the core thus can be used in SMP systems
> 

Yes I did so already :-) Although I also added a default y if ARC to both, but as
you say that is better done in ARC Kconfig.

> Then in the ARC's Kconfig you select ARC_TIMER_RTC or ARC_TIMER_GFRC depending
> it is SMP or not.
> 
> One question:
> 
> Why ARC_TIMER_RTC can't be used in a SMP system ? Doesn't have each core its
> own clocksource ? It seems you are assuming a clocksource can be used on SMP
> only if the clocksource is unique and shared across the cores.

Thats what I thought so far. Thing is, the individual core's counters could get
out of sync, simply because non masters cores were halted to begin with and came
up at different points in real time. so a gtod might return different value
depending on what core it landed on. Does clocksource also does ticks broadcasts
and such to keep things in sync ?

Because of the git mv you, diff didn't include bulk of driver code which would
make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ?

Thx,
-Vineet

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

#1513576 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On Tue, Nov 01, 2016 at 01:57:05PM -0700, Vineet Gupta wrote:
> Hi Daniel,
> 
> On 11/01/2016 01:42 PM, Daniel Lezcano wrote:
> > Please stay consistent with the rest of the Kconfig.
> > 
> > config ARC_TIMER_RTC
> > 	bool "64-bit cycle counter in HS38 cores" if COMPILE_TEST
> > 	select CLKSRC_OF
> > 	help
> > 	  This counter provides 64-bit resolution vs. the 32-bit TIMER1.
> > 	  It is implemented inside the core thus can't be used in SMP systems.
> > 
> > config ARC_TIMER_GFRC
> > 	bool "64-bit cycle counter in ARConnect block in HS38x cores" if COMPILE_TEST
> > 	select CLKSRC_OF
> > 	help
> > 	  This counter can be used as clocksource in SMP HS38 SoCs.
> > 	  It sits outside the core thus can be used in SMP systems
> > 
> 
> Yes I did so already :-) Although I also added a default y if ARC to both, but as
> you say that is better done in ARC Kconfig.
> 
> > Then in the ARC's Kconfig you select ARC_TIMER_RTC or ARC_TIMER_GFRC depending
> > it is SMP or not.
> > 
> > One question:
> > 
> > Why ARC_TIMER_RTC can't be used in a SMP system ? Doesn't have each core its
> > own clocksource ? It seems you are assuming a clocksource can be used on SMP
> > only if the clocksource is unique and shared across the cores.
> 
> Thats what I thought so far. Thing is, the individual core's counters could get
> out of sync, simply because non masters cores were halted to begin with and came
> up at different points in real time. so a gtod might return different value
> depending on what core it landed on. Does clocksource also does ticks broadcasts
> and such to keep things in sync ?

Sounds like it is similar than the TSC. Do you agree to have a try by setting
the CONFIG_HAVE_UNSTABLE_SCHED_CLOCK option ?

If you can use those per cpu clocksource, performances on your system may
improve with the sched_clock().
 
> Because of the git mv you, diff didn't include bulk of driver code which would
> make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ?

That means I will review and comment existing code. It is not a problem for me
if you agree to do the changes.

  -- Daniel

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web