Re: [PATCH v7 1/7] clocksource/drivers/clksrc-evt-probe: Describe with the DT both the clocksource and the clockevent

[Rob Herring]

On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> 
> The CLOCKSOURCE_OF_DECLARE() was introduced before the
> CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> clockevent and the clocksource are both initialized in the same init
> routine.
> 
> With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> now split the clocksource and the clockevent init code. However, the
> device tree may specify a single node, so the same node will be passed
> to the clockevent/clocksource's init function, with the same base
> address.
> 
> with this patch it is possible to specify an attribute to the timer's node to
> specify if it is a clocksource or a clockevent and define two timers node.

Daniel and I discussed and agreed against this a while back. What 
changed?

> 
> For example:
> 
>         timer: timer@98400000 {
>                 compatible = "moxa,moxart-timer";
>                 reg = <0x98400000 0x42>;

This overlaps the next node. You can change this to 0x10, but are these 
really 2 independent h/w blocks? Don't design the nodes around the 
current needs of Linux.

>                 interrupts = <19 1>;
>                 clocks = <&coreclk>;
>                 clockevent;

This is not needed. The presence of "interrupts" is enough to say use 
this timer for clockevent.

>         };
> 
>         timer: timer@98400010 {
>                 compatible = "moxa,moxart-timer";
>                 reg = <0x98400010 0x42>;
>                 clocks = <&coreclk>;
>                 clocksource;

Likewise.

>         };
> 
> With this approach, we allow a mechanism to clearly define a clocksource or a
> clockevent without aerobatics we can find around in some drivers:
> 	timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> 	renesas-ostm.c, time-efm32.c, time-lpc32xx.c.

These all already have bindings and work. What problem are you trying to 
solve other than restructuring Linux?

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Signed-off-by: Alexander Kochetkov <al.kochet@xxxxxxxxx>
> ---
>  Documentation/devicetree/bindings/timer/timer.txt |   38 +++++++++++++++++++++
>  drivers/clocksource/clkevt-probe.c                |    7 ++++
>  drivers/clocksource/clksrc-probe.c                |    7 ++++
>  3 files changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/timer.txt b/Documentation/devicetree/bindings/timer/timer.txt
> new file mode 100644
> index 0000000..f1ee0cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/timer.txt
> @@ -0,0 +1,38 @@
> +
> +Specifying timer information for devices
> +========================================
> +
> +The timer can be declared via the macro:
> +
> +CLOCKSOURCE_OF_DECLARE(name, init) if it is a clocksource
> +CLOCKEVENT_OF_DECLARE(name, init) if it is a clockevent
> +
> +The CLOCKSOURCE_OF_DECLARE() was introduced before the
> +CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> +clockevent and the clocksource are both initialized in the same init
> +routine.
> +
> +With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> +now split the clocksource and the clockevent init code. However, the
> +device tree may specify a single node, so the same node will be passed
> +to the clockevent/clocksource's init function, with the same base
> +address. It is possible to specify an attribute to the timer's node to
> +specify if it is a clocksource or a clockevent and define two timers
> +node.

This is all Linux details and doesn't belong in binding docs.

> +
> +Example:
> +
> +	timer: timer@98400000 {
> +		compatible = "moxa,moxart-timer";
> +		reg = <0x98400000 0x42>;
> +		interrupts = <19 1>;
> +		clocks = <&coreclk>;
> +		clockevent;
> +	};
> +
> +	timer: timer@98400010 {
> +		compatible = "moxa,moxart-timer";
> +		reg = <0x98400010 0x42>;
> +		clocks = <&coreclk>;
> +		clocksource;
> +	};
> diff --git a/drivers/clocksource/clkevt-probe.c b/drivers/clocksource/clkevt-probe.c
> index eb89b50..fa02ac1 100644
> --- a/drivers/clocksource/clkevt-probe.c
> +++ b/drivers/clocksource/clkevt-probe.c
> @@ -37,6 +37,13 @@ int __init clockevent_probe(void)
>  
>  		init_func = match->data;
>  
> +		/*
> +		 * The device node describes a clocksource, ignore it
> +		 * as we are in the clockevent init routine.
> +		 */
> +		if (of_property_read_bool(np, "clocksource"))
> +			continue;
> +
>  		ret = init_func(np);
>  		if (ret) {
>  			pr_warn("Failed to initialize '%s' (%d)\n",
> diff --git a/drivers/clocksource/clksrc-probe.c b/drivers/clocksource/clksrc-probe.c
> index bc62be9..ce50f33 100644
> --- a/drivers/clocksource/clksrc-probe.c
> +++ b/drivers/clocksource/clksrc-probe.c
> @@ -38,6 +38,13 @@ void __init clocksource_probe(void)
>  
>  		init_func_ret = match->data;
>  
> +		/*
> +		 * The device node describes a clockevent, ignore it
> +		 * as we are in the clocksource init routine.
> +		 */
> +		if (of_property_read_bool(np, "clockevent"))
> +			continue;
> +
>  		ret = init_func_ret(np);
>  		if (ret) {
>  			pr_err("Failed to initialize '%s': %d",
> -- 
> 1.7.9.5
>