| Message ID | 1454775406-25277-3-git-send-email-noamc@ezchip.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/06/2016 05:16 PM, Noam Camus wrote: > From: Noam Camus <noamc@ezchip.com> > > Add internal tick generator which is shared by all cores. > Each cluster of cores view it through dedicated address. > This is used for SMP system where all CPUs synced by same > clock source. > > Signed-off-by: Noam Camus <noamc@ezchip.com> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: John Stultz <john.stultz@linaro.org> > Acked-by: Vineet Gupta <vgupta@synopsys.com> > --- [ ... ] > +static cycle_t nps_clksrc_read(struct clocksource *clksrc) > +{ > + int cluster = raw_smp_processor_id() >> NPS_CLUSTER_OFFSET; > + > + return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]); > +} > + > +static struct clocksource nps_counter = { > + .name = "EZnps-tick", > + .rating = 301, > + .read = nps_clksrc_read, > + .mask = CLOCKSOURCE_MASK(32), > + .flags = CLOCK_SOURCE_IS_CONTINUOUS, > +}; > + > +static void __init nps_setup_clocksource(struct device_node *node, > + struct clk *clk) > +{ > + struct clocksource *clksrc = &nps_counter; > + int ret, cluster; > + > + for (cluster = 0; cluster < NPS_CLUSTER_NUM; cluster++) > + nps_msu_reg_low_addr[cluster] = > + nps_host_reg((cluster << NPS_CLUSTER_OFFSET), > + NPS_MSU_BLKID, NPS_MSU_TICK_LOW); > + > + ret = clk_prepare_enable(clk); > + if (ret) > + pr_err("Couldn't enable parent clock\n"); > + > + nps_timer_rate = clk_get_rate(clk); If there is an error, you continue the execution of the code. I guess you expect the system to hang in any case with the error in the console, right ? > + ret = clocksource_register_hz(clksrc, nps_timer_rate); You can simplify the driver even more by using clocksource_mmio_init.
>From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] >Sent: Monday, February 08, 2016 4:22 PM >> + ret = clk_prepare_enable(clk); >> + if (ret) >> + pr_err("Couldn't enable parent clock\n"); >> + >> + nps_timer_rate = clk_get_rate(clk); >If there is an error, you continue the execution of the code. I guess you expect the system to hang in any case with the error in the >console, right ? Since our clock is root then returned value will always be valid. I am far from being expert here, but no one checks for clk_get_rate() return value for error. Could you refer to a single place at clocksource drivers that checks for error in the return value. >> + ret = clocksource_register_hz(clksrc, nps_timer_rate); >You can simplify the driver even more by using clocksource_mmio_init. Since my base address depends on cluster number, which CPU is part of, this interface is not much of a use. On top of that it assumes that I am little endian by using readl family accessories. -Noam
On 02/09/2016 01:36 PM, Noam Camus wrote: >> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] Sent: >> Monday, February 08, 2016 4:22 PM > >>> + ret = clk_prepare_enable(clk); + if (ret) + pr_err("Couldn't >>> enable parent clock\n"); + + nps_timer_rate = clk_get_rate(clk); > >> If there is an error, you continue the execution of the code. I >> guess you expect the system to hang in any case with the error in >> the >console, right ? > > Since our clock is root then returned value will always be valid. I > am far from being expert here, but no one checks for clk_get_rate() > return value for error. Could you refer to a single place at > clocksource drivers that checks for error in the return value. Actually I was referring to clk_prepare_enable, clocksource_register_hz. Agree clk_get_rate is always valid. >>> + ret = clocksource_register_hz(clksrc, nps_timer_rate); > >> You can simplify the driver even more by using >> clocksource_mmio_init. > Since my base address depends on cluster number, which CPU is part > of, this interface is not much of a use. On top of that it assumes > that I am little endian by using readl family accessories. Why can't you use ? clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick", nps_timer_rate, 32, nps_clksrc_read);
>From: Daniel Lezcano <daniel.lezcano@linaro.org> >Sent: Tuesday, February 9, 2016 3:38 PM >Actually I was referring to clk_prepare_enable, clocksource_register_hz. >Agree clk_get_rate is always valid. Thanks for making this clear. Any way as you can see I do call pr_err() in case of error just like most drivers around. By "hang" do you mean calling panic()? What if there is another clocksource in DT (even with worse rating)? I still prefer using it then having non workable machine. >> >>> You can simplify the driver even more by using >>> clocksource_mmio_init. >> Since my base address depends on cluster number, which CPU is part >> of, this interface is not much of a use. On top of that it assumes >> that I am little endian by using readl family accessories. >Why can't you use ? >clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick", >nps_timer_rate, 32, nps_clksrc_read); I believe that the simplification is meant for drivers that can actually use the clocksource_mmio..() accessories. Could you explain what is the advantage here, for my case, to use clocksource_mmio driver? Thanks for your patience -Noam
On 02/09/2016 10:47 PM, Noam Camus wrote: >> From: Daniel Lezcano <daniel.lezcano@linaro.org> Sent: Tuesday, >> February 9, 2016 3:38 PM > >> Actually I was referring to clk_prepare_enable, >> clocksource_register_hz. Agree clk_get_rate is always valid. > Thanks for making this clear. Any way as you can see I do call > pr_err() in case of error just like most drivers around. By "hang" do > you mean calling panic()? No. I meant the errors are caught but no action is taken, the execution continues normally as nothing wrong happened. This is why I asked if you expect the host to hang at boot time with the last error as a hint. I was expecting to see a call to clk_disable_unprepare if clocksource_register_hz fails, and returning 'ret' if clk_prepare_enable fails. > What if there is another clocksource in DT (even with worse rating)? > I still prefer using it then having non workable machine. You are right. This is why failing gracefully in the init function is important. >>>> You can simplify the driver even more by using >>>> clocksource_mmio_init. >>> Since my base address depends on cluster number, which CPU is >>> part of, this interface is not much of a use. On top of that it >>> assumes that I am little endian by using readl family >>> accessories. > >> Why can't you use ? > >> clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick", >> nps_timer_rate, 32, nps_clksrc_read); > > I believe that the simplification is meant for drivers that can > actually use the clocksource_mmio..() accessories. Could you explain > what is the advantage here, for my case, to use clocksource_mmio > driver? Using the mmio generic code will save: +static struct clocksource nps_counter = { + .name = "EZnps-tick", + .rating = 301, + .read = nps_clksrc_read, + .mask = CLOCKSOURCE_MASK(32), + .flags = CLOCK_SOURCE_IS_CONTINUOUS, +}; Up to you. -- Daniel
>From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] >Sent: Wednesday, February 10, 2016 12:55 AM >> pr_err() in case of error just like most drivers around. By "hang" do >> you mean calling panic()? >No. I meant the errors are caught but no action is taken, the execution continues normally as nothing wrong happened. This is why I asked if you expect the host to hang at boot time with the last error as a hint. >I was expecting to see a call to clk_disable_unprepare if clocksource_register_hz fails, and returning 'ret' if clk_prepare_enable fails. Ok, I will fix that, and handle gracefull return. Thanks >Using the mmio generic code will save: >+static struct clocksource nps_counter = { >+ .name = "EZnps-tick", >+ .rating = 301, >+ .read = nps_clksrc_read, >+ .mask = CLOCKSOURCE_MASK(32), >+ .flags = CLOCK_SOURCE_IS_CONTINUOUS, >+}; >Up to you. I will do that, thanks again Noam -- <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
diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt new file mode 100644 index 0000000..c8c03d7 --- /dev/null +++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt @@ -0,0 +1,15 @@ +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/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index 2eb5f0e..0d0ba91 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -132,6 +132,15 @@ config CLKSRC_TI_32K This option enables support for Texas Instruments 32.768 Hz clocksource available on many OMAP-like platforms. +config CLKSRC_NPS + bool "NPS400 clocksource driver" if COMPILE_TEST + depends on !PHYS_ADDR_T_64BIT + select CLKSRC_OF if OF + help + NPS400 clocksource support. + Got 64 bit counter with update rate up to 1000MHz. + This counter is accessed via couple of 32 bit memory mapped registers. + config CLKSRC_STM32 bool "Clocksource for STM32 SoCs" if !ARCH_STM32 depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST) diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index 56bd16e..056cffd 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -46,6 +46,7 @@ obj-$(CONFIG_CLKSRC_QCOM) += qcom-timer.o obj-$(CONFIG_MTK_TIMER) += mtk_timer.o obj-$(CONFIG_CLKSRC_PISTACHIO) += time-pistachio.o obj-$(CONFIG_CLKSRC_TI_32K) += timer-ti-32k.o +obj-$(CONFIG_CLKSRC_NPS) += timer-nps.o obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o obj-$(CONFIG_ARM_GLOBAL_TIMER) += arm_global_timer.o diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c new file mode 100644 index 0000000..bf9a490 --- /dev/null +++ b/drivers/clocksource/timer-nps.c @@ -0,0 +1,84 @@ +/* + * Copyright(c) 2015 EZchip Technologies. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + * + * The full GNU General Public License is included in this distribution in + * the file called "COPYING". + */ + +#include <linux/interrupt.h> +#include <linux/clocksource.h> +#include <linux/clockchips.h> +#include <linux/clk.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/cpu.h> +#include <soc/nps/common.h> + +#define NPS_MSU_TICK_LOW 0xC8 +#define NPS_CLUSTER_OFFSET 8 +#define NPS_CLUSTER_NUM 16 + +/* 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 cycle_t nps_clksrc_read(struct clocksource *clksrc) +{ + int cluster = raw_smp_processor_id() >> NPS_CLUSTER_OFFSET; + + return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]); +} + +static struct clocksource nps_counter = { + .name = "EZnps-tick", + .rating = 301, + .read = nps_clksrc_read, + .mask = CLOCKSOURCE_MASK(32), + .flags = CLOCK_SOURCE_IS_CONTINUOUS, +}; + +static void __init nps_setup_clocksource(struct device_node *node, + struct clk *clk) +{ + struct clocksource *clksrc = &nps_counter; + int ret, cluster; + + for (cluster = 0; cluster < NPS_CLUSTER_NUM; cluster++) + nps_msu_reg_low_addr[cluster] = + nps_host_reg((cluster << NPS_CLUSTER_OFFSET), + NPS_MSU_BLKID, NPS_MSU_TICK_LOW); + + ret = clk_prepare_enable(clk); + if (ret) + pr_err("Couldn't enable parent clock\n"); + + nps_timer_rate = clk_get_rate(clk); + + ret = clocksource_register_hz(clksrc, nps_timer_rate); + if (ret) + pr_err("Couldn't register clock source.\n"); +} + +static void __init nps_timer_init(struct device_node *node) +{ + struct clk *clk; + + clk = of_clk_get(node, 0); + if (IS_ERR(clk)) + panic("Can't get timer clock"); + + nps_setup_clocksource(node, clk); +} + +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer", + nps_timer_init);