| Message ID | 1490197714-25415-2-git-send-email-al.kochet@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote: > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > 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@linaro.org> > Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> > --- > 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 >
On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote: > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote: > > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > > > 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? Hi Rob, > > 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. Mmh, thanks for raising this. Conceptually speaking there are two (or more) different entities, the clocksource and the clockevents but they share the same IP block. In the driver timer-sp804.c there is a fragile mechanism with the timer counting assuming the timers are declared in a specific order in the DT (integratorcp.dts). There are multiple nodes, which are IIUC overlapping like what you are describing. I'm a bit puzzled with how this should be done. Do you have a suggestion ? > > interrupts = <19 1>; > > clocks = <&coreclk>; > > clockevent; > > This is not needed. The presence of "interrupts" is enough to say use > this timer for clockevent. Yes, that is true if no drivers was already taking CLOCKSOURCE_OF and initializing the clockevent. The driver will pass through the clocksource_probe function, check the interrupt and bail out because there is an interrupt declared and assume it is a clockevent, so no initialization for the driver. IOW, it is not backward compatible. We need this attribute somehow in order to separate clearly a clocksource or a clockevent with a new implementation. > > }; > > > > 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? Yes, there is already the bindings, but that force to do some hackish initialization. I would like to give the opportunity to declare separately a clocksource and a clockevent, in order to have full control of how this is initialized.
Hi, On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote: > On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote: > > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote: > > > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > > > > > 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? > > Hi Rob, > > > > 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. > > Mmh, thanks for raising this. Conceptually speaking there are two (or more) > different entities, the clocksource and the clockevents but they share the same > IP block. From the DT's PoV, this is one entity, which is the IP block. The clocksource/clockevent concept is a Linux implementation detail. The DT cannot and should not be aware of that. [...] > > > interrupts = <19 1>; > > > clocks = <&coreclk>; > > > clockevent; > > > > This is not needed. The presence of "interrupts" is enough to say use > > this timer for clockevent. > > Yes, that is true if no drivers was already taking CLOCKSOURCE_OF and > initializing the clockevent. The driver will pass through the clocksource_probe > function, check the interrupt and bail out because there is an interrupt > declared and assume it is a clockevent, so no initialization for the driver. > IOW, it is not backward compatible. > > We need this attribute somehow in order to separate clearly a clocksource or a > clockevent with a new implementation. Why? A single IP block can provide the functionality of both (though there are reasons that functionality may be mutually exclusive). What is the benefit of this split? > > > 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? > > Yes, there is already the bindings, but that force to do some hackish > initialization. Here, you are forcing hackish DT changes that do not truly describe HW. How is that better? > I would like to give the opportunity to declare separately a clocksource and a > clockevent, in order to have full control of how this is initialized. To me it sounds like what we need is Linux infrastructure that allows one to register a device as having both clockevent/clocksource functionality. That way, we can choose to do something sane at boot time, and if the user really wants specific devices used in specific ways, they can request that. Thanks, Mark.
On Wed, Mar 29, 2017 at 11:49:11AM +0100, Mark Rutland wrote: > Hi, > > On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote: > > On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote: > > > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote: > > > > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > > > > > > > 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? > > > > Hi Rob, > > > > > > 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. > > > > Mmh, thanks for raising this. Conceptually speaking there are two (or more) > > different entities, the clocksource and the clockevents but they share the same > > IP block. > > From the DT's PoV, this is one entity, which is the IP block. > > The clocksource/clockevent concept is a Linux implementation detail. The > DT cannot and should not be aware of that. > > [...] > > > > > interrupts = <19 1>; > > > > clocks = <&coreclk>; > > > > clockevent; > > > > > > This is not needed. The presence of "interrupts" is enough to say use > > > this timer for clockevent. > > > > Yes, that is true if no drivers was already taking CLOCKSOURCE_OF and > > initializing the clockevent. The driver will pass through the clocksource_probe > > function, check the interrupt and bail out because there is an interrupt > > declared and assume it is a clockevent, so no initialization for the driver. > > IOW, it is not backward compatible. > > > > We need this attribute somehow in order to separate clearly a clocksource or a > > clockevent with a new implementation. > > Why? A single IP block can provide the functionality of both (though > there are reasons that functionality may be mutually exclusive). What is > the benefit of this split? Hi Mark, You can have several timers on the system and may want to use the clockevents from one IP block and the clocksource from another IP block. For example, in the case of a bogus clocksource. Moreover there are some drivers with a node for a clocksource and another one for the clockevent, and the driver is assuming the clockevent is defined first and then the clocksource. eg. arch/arc/boot/dts/abilis_tb10x.dtsi: /* TIMER0 with interrupt for clockevent */ timer0 { compatible = "snps,arc-timer"; interrupts = <3>; interrupt-parent = <&intc>; clocks = <&cpu_clk>; }; /* TIMER1 for free running clocksource */ timer1 { compatible = "snps,arc-timer"; clocks = <&cpu_clk>; }; drivers/clocksource/arc_timer.c: static int __init arc_of_timer_init(struct device_node *np) { static int init_count = 0; int ret; if (!init_count) { init_count = 1; ret = arc_clockevent_setup(np); } else { ret = arc_cs_setup_timer1(np); } return ret; } Even if that works, it is a fragile mechanism. So the purpose of these changes is to provide a stronger timer declaration in order to clearly split in the kernel a clocksource and a clockevent initialization. > > > > 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? > > > > Yes, there is already the bindings, but that force to do some hackish > > initialization. > > Here, you are forcing hackish DT changes that do not truly describe HW. > How is that better? So if this is hackish DT changes, then the existing DTs should be fixed, no? > > I would like to give the opportunity to declare separately a clocksource and a > > clockevent, in order to have full control of how this is initialized. > > To me it sounds like what we need is Linux infrastructure that allows > one to register a device as having both clockevent/clocksource > functionality. That was the idea. Create a macro CLOCKEVENT_OF and CLOCKSOURCE_OF both of them calling their respective init routines. And in addition a TIMER_OF doing both CLOCKEVENT_OF and CLOCKSOURCE_OF. It is the DT which does not allow to do this separation. Would be the following approach more acceptable ? 1. Replace all CLOCKSOURCE_OF by TIMER_OF (just renaming) 2. A node can have a clockevent and|or a clocksource attributes 3. The timer_probe pass a flag to the driver's init function, so this one knows if it should invoke the clockevent/clocksource init functions. No attribute defaults to clocksource|clockevent. That would be backward compatible and will let to create drivers with clutch activated device via DT. Also, it will give the opportunity to the existing drivers to change consolidate their initialization routines. > That way, we can choose to do something sane at boot time, and if the > user really wants specific devices used in specific ways, they can > request that.
On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote: > On Wed, Mar 29, 2017 at 11:49:11AM +0100, Mark Rutland wrote: > > On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote: > > > On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote: > > > > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote: > > You can have several timers on the system and may want to use the clockevents > from one IP block and the clocksource from another IP block. For example, in > the case of a bogus clocksource. I understand this. However, what I was trying to say is that *how* we use a particular device should be a software decision. I have a more concrete suggestion on that below. > Moreover there are some drivers with a node for a clocksource and > another one for the clockevent, and the driver is assuming the clockevent is > defined first and then the clocksource. > > eg. > > arch/arc/boot/dts/abilis_tb10x.dtsi: > > /* TIMER0 with interrupt for clockevent */ > timer0 { > compatible = "snps,arc-timer"; > interrupts = <3>; > interrupt-parent = <&intc>; > clocks = <&cpu_clk>; > }; > > /* TIMER1 for free running clocksource */ > timer1 { > compatible = "snps,arc-timer"; > clocks = <&cpu_clk>; > }; > > drivers/clocksource/arc_timer.c: > > static int __init arc_of_timer_init(struct device_node *np) > { > static int init_count = 0; > int ret; > > if (!init_count) { > init_count = 1; > ret = arc_clockevent_setup(np); > } else { > ret = arc_cs_setup_timer1(np); > } > > return ret; > } > > Even if that works, it is a fragile mechanism. > > So the purpose of these changes is to provide a stronger timer declaration in > order to clearly split in the kernel a clocksource and a clockevent > initialization. I agree that this pattern is not nice. However, I think that splitting devices as this level makes the problem *worse*. Users care that they have a clocksource and a clockevent device. They do not care *which* particular device is used as either. The comments in the DT above are at best misleading. What we need is for the kernel to understand that devices can be both clockevent and clocksource (perhaps mutually exclusively), such that the kernel can decide how to make use of devices. That way, for the above the kernel can figure out that timer0 could be used as clocksource or clockevent, while timer1 can only be used as a clocksource due to the lack of an interrupt. Thus, it can choose to use timer0 as a clockevent, and timer1 and a clocksource. > > > > > 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? > > > > > > Yes, there is already the bindings, but that force to do some hackish > > > initialization. > > > > Here, you are forcing hackish DT changes that do not truly describe HW. > > How is that better? > > So if this is hackish DT changes, then the existing DTs should be fixed, no? Yes. For the above snippet, the only thing that needs to change is the comment. > > > I would like to give the opportunity to declare separately a clocksource and a > > > clockevent, in order to have full control of how this is initialized. > > > > To me it sounds like what we need is Linux infrastructure that allows > > one to register a device as having both clockevent/clocksource > > functionality. > > That was the idea. Create a macro CLOCKEVENT_OF and CLOCKSOURCE_OF both of them > calling their respective init routines. And in addition a TIMER_OF doing both > CLOCKEVENT_OF and CLOCKSOURCE_OF. > > It is the DT which does not allow to do this separation. > > Would be the following approach more acceptable ? > > 1. Replace all CLOCKSOURCE_OF by TIMER_OF (just renaming) I am fine with this renaming. > 2. A node can have a clockevent and|or a clocksource attributes As above, this should not be in the DT given it's describing a (Linux-specific) SW policy and not a HW detail. So I must disagree with this. > 3. The timer_probe pass a flag to the driver's init function, so this one knows > if it should invoke the clockevent/clocksource init functions. > No attribute defaults to clocksource|clockevent. > > That would be backward compatible and will let to create drivers with clutch > activated device via DT. Also, it will give the opportunity to the existing > drivers to change consolidate their initialization routines. I think that if anything, we need a combined clocksource+clockevent device that we register to the core code. That means all clocksource/clockevent drivers have a consolidated routine. Subsequently, core code should determine how specifically to use the device (e.g. based on what other devices are registered, and their capabilities). Thanks, Mark.
On Wed, Mar 29, 2017 at 01:57:14PM +0100, Mark Rutland wrote: > On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote: > > On Wed, Mar 29, 2017 at 11:49:11AM +0100, Mark Rutland wrote: > > > On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote: > > > > On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote: > > > > > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote: > > > > You can have several timers on the system and may want to use the clockevents > > from one IP block and the clocksource from another IP block. For example, in > > the case of a bogus clocksource. > > I understand this. However, what I was trying to say is that *how* we > use a particular device should be a software decision. I have a more > concrete suggestion on that below. > > > Moreover there are some drivers with a node for a clocksource and > > another one for the clockevent, and the driver is assuming the clockevent is > > defined first and then the clocksource. > > > > eg. > > > > arch/arc/boot/dts/abilis_tb10x.dtsi: > > > > /* TIMER0 with interrupt for clockevent */ > > timer0 { > > compatible = "snps,arc-timer"; > > interrupts = <3>; > > interrupt-parent = <&intc>; > > clocks = <&cpu_clk>; > > }; > > > > /* TIMER1 for free running clocksource */ > > timer1 { > > compatible = "snps,arc-timer"; > > clocks = <&cpu_clk>; > > }; > > > > drivers/clocksource/arc_timer.c: > > > > static int __init arc_of_timer_init(struct device_node *np) > > { > > static int init_count = 0; > > int ret; > > > > if (!init_count) { > > init_count = 1; > > ret = arc_clockevent_setup(np); > > } else { > > ret = arc_cs_setup_timer1(np); > > } > > > > return ret; > > } > > > > Even if that works, it is a fragile mechanism. > > > > So the purpose of these changes is to provide a stronger timer declaration in > > order to clearly split in the kernel a clocksource and a clockevent > > initialization. > > I agree that this pattern is not nice. However, I think that splitting > devices as this level makes the problem *worse*. > > Users care that they have a clocksource and a clockevent device. They > do not care *which* particular device is used as either. The comments in > the DT above are at best misleading. Agree. And the driver is assuming the first node is the clockevent and the second one is the clocksource. If the DT invert these nodes, that breaks the driver. > What we need is for the kernel to understand that devices can be both > clockevent and clocksource (perhaps mutually exclusively), such that the > kernel can decide how to make use of devices. > > That way, for the above the kernel can figure out that timer0 could be > used as clocksource or clockevent, while timer1 can only be used as a > clocksource due to the lack of an interrupt. Thus, it can choose to use > timer0 as a clockevent, and timer1 and a clocksource. Well, 'interrupt' gives an indication the timer can be used as a clockevent and clocksource, not the clockevent only. If we take the case of the rockchip, the arm_arch_timer clocksource is stopped when the CPU is clock gated. So specifically, we don't want to use this clocksource but we want to use the arch clockevents because they are better. > > > > > > 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? > > > > > > > > Yes, there is already the bindings, but that force to do some hackish > > > > initialization. > > > > > > Here, you are forcing hackish DT changes that do not truly describe HW. > > > How is that better? > > > > So if this is hackish DT changes, then the existing DTs should be fixed, no? > > Yes. > > For the above snippet, the only thing that needs to change is the > comment. > > > > > I would like to give the opportunity to declare separately a clocksource and a > > > > clockevent, in order to have full control of how this is initialized. > > > > > > To me it sounds like what we need is Linux infrastructure that allows > > > one to register a device as having both clockevent/clocksource > > > functionality. > > > > That was the idea. Create a macro CLOCKEVENT_OF and CLOCKSOURCE_OF both of them > > calling their respective init routines. And in addition a TIMER_OF doing both > > CLOCKEVENT_OF and CLOCKSOURCE_OF. > > > > It is the DT which does not allow to do this separation. > > > > Would be the following approach more acceptable ? > > > > 1. Replace all CLOCKSOURCE_OF by TIMER_OF (just renaming) > > I am fine with this renaming. > > > 2. A node can have a clockevent and|or a clocksource attributes > > As above, this should not be in the DT given it's describing a > (Linux-specific) SW policy and not a HW detail. > > So I must disagree with this. IIUC my discussion with Rob, an attribute is acceptable (btw if 'clocksource'|'clockevent' names are too Linux specific (+1), what about a more generic name like 'tick' and 'time' ?). > > 3. The timer_probe pass a flag to the driver's init function, so this one knows > > if it should invoke the clockevent/clocksource init functions. > > No attribute defaults to clocksource|clockevent. > > > > That would be backward compatible and will let to create drivers with clutch > > activated device via DT. Also, it will give the opportunity to the existing > > drivers to change consolidate their initialization routines. > > I think that if anything, we need a combined clocksource+clockevent > device that we register to the core code. That means all > clocksource/clockevent drivers have a consolidated routine. > > Subsequently, core code should determine how specifically to use the > device (e.g. based on what other devices are registered, and their > capabilities). IMO, the core code is complex enough and that may imply more heuristics. Regarding the number of timers, I do believe it is much better to simply tell which one we want to use via the DT (assuming the DT is a description of the desired hardware, not all the available hardware).
On Wed, Mar 29, 2017 at 03:41:34PM +0200, Daniel Lezcano wrote: > On Wed, Mar 29, 2017 at 01:57:14PM +0100, Mark Rutland wrote: > > On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote: > > > arch/arc/boot/dts/abilis_tb10x.dtsi: > > > > > > /* TIMER0 with interrupt for clockevent */ > > > timer0 { > > > compatible = "snps,arc-timer"; > > > interrupts = <3>; > > > interrupt-parent = <&intc>; > > > clocks = <&cpu_clk>; > > > }; > > > > > > /* TIMER1 for free running clocksource */ > > > timer1 { > > > compatible = "snps,arc-timer"; > > > clocks = <&cpu_clk>; > > > }; > > > > > > drivers/clocksource/arc_timer.c: > > > > > > static int __init arc_of_timer_init(struct device_node *np) > > > { > > > static int init_count = 0; > > > int ret; > > > > > > if (!init_count) { > > > init_count = 1; > > > ret = arc_clockevent_setup(np); > > > } else { > > > ret = arc_cs_setup_timer1(np); > > > } > > > > > > return ret; > > > } > > > > > > So the purpose of these changes is to provide a stronger timer declaration in > > > order to clearly split in the kernel a clocksource and a clockevent > > > initialization. > > > > I agree that this pattern is not nice. However, I think that splitting > > devices as this level makes the problem *worse*. > > > > Users care that they have a clocksource and a clockevent device. They > > do not care *which* particular device is used as either. The comments in > > the DT above are at best misleading. > > Agree. > > And the driver is assuming the first node is the clockevent and the second one > is the clocksource. If the DT invert these nodes, that breaks the driver. Sure, but that is something we can and should fix within Linux. > > What we need is for the kernel to understand that devices can be both > > clockevent and clocksource (perhaps mutually exclusively), such that the > > kernel can decide how to make use of devices. > > > > That way, for the above the kernel can figure out that timer0 could be > > used as clocksource or clockevent, while timer1 can only be used as a > > clocksource due to the lack of an interrupt. Thus, it can choose to use > > timer0 as a clockevent, and timer1 and a clocksource. > > Well, 'interrupt' gives an indication the timer can be used as a clockevent and > clocksource, not the clockevent only. Which is exactly what I said above, when I said: the kernel can figure out that timer0 could be used as clocksource or clockevent Considering both timer0 and timer1 is how we can figure out timer0 must be the clockevent, since timer1 cannot be. > If we take the case of the rockchip, the arm_arch_timer clocksource is stopped > when the CPU is clock gated. So specifically, we don't want to use this > clocksource but we want to use the arch clockevents because they are better. Sure. As I pointed out, we want to consider the holistic details to make the right decision. i.e. the infrastructure should make the choice, not the individual drivers. Consider that the kernel may need to make decisions that differ it a kernel is built wihout certain drivers. That cannot work if the use is allocated in the DT. [...] > > > 2. A node can have a clockevent and|or a clocksource attributes > > > > As above, this should not be in the DT given it's describing a > > (Linux-specific) SW policy and not a HW detail. > > > > So I must disagree with this. > > IIUC my discussion with Rob, an attribute is acceptable (btw if > 'clocksource'|'clockevent' names are too Linux specific (+1), what > about a more generic name like 'tick' and 'time' ?). The *meaning* of these is Linux specific. The naming is irrelevant. > > > 3. The timer_probe pass a flag to the driver's init function, so this one knows > > > if it should invoke the clockevent/clocksource init functions. > > > No attribute defaults to clocksource|clockevent. > > > > > > That would be backward compatible and will let to create drivers with clutch > > > activated device via DT. Also, it will give the opportunity to the existing > > > drivers to change consolidate their initialization routines. > > > > I think that if anything, we need a combined clocksource+clockevent > > device that we register to the core code. That means all > > clocksource/clockevent drivers have a consolidated routine. > > > > Subsequently, core code should determine how specifically to use the > > device (e.g. based on what other devices are registered, and their > > capabilities). > > IMO, the core code is complex enough and that may imply more heuristics. Given that the majority of cases are going to be multiple instances of the same IP, I cannot imagine it is complex to get something that works. A generally optimal configuration may require some heuristics, but that's a different matter to a correct and functional configuration. I must disagree with trying to push that complexity into the DT by means of static SW policy, rather than solving the problem in SW where we have all the information to do so. Thanks, Mark.
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. + +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",