Re: [PATCH 1/4] clocksource: mmp: add mmp timer driver

[Chao Xie]

>From: Mark Rutland [mailto:mark.rutland@xxxxxxx] 
>Sent: 2015å2æ2æ 18:35
>To: Chao Xie
>Cc: daniel.lezcano@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; haojian.zhuang@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH 1/4] clocksource: mmp: add mmp timer driver
>
>On Mon, Feb 02, 2015 at 08:31:36AM +0000, Chao Xie wrote:
>> From: Chao Xie <chao.xie@xxxxxxxxxxx>
>> 
>> MMP timer is attached to APB bus, It has the following limitation.
>> 1. When get count of timer counter, it need some delay
>>    to get a stable count.
>> 2. When set match register, it need disable the counter
>>    first, and enable it after set the match register.
>>    The disabling need wait for 2 clock cycle to take
>>    effect.
>> 
>> To improve above #1, shadow register for count is added.
>> To improve above #2, CRSR register is added for quick updating.
>> 
>> So there are three types of timer.
>> timer1: old timer.
>> timer2: old timer with shadow register.
>> timer3: old timer with shadow and CRSR register.
>> 
>> This timer driver will be used for many SOCes.
>> 1. pxa168, pxa910, pxa988 pxa1088 have only timer1.
>> 2. pxa1L88, pxa1U88 have timer1 and timer3.
>> 3. pxa1928 has timer 2.
>> 
>> The driver supports DT and non-DT initialization.
>> The driver supports UP/SMP SOCes and 64 bit core.
>> 
>> Signed-off-by: Chao Xie <chao.xie@xxxxxxxxxxx>
>> ---
>>  .../devicetree/bindings/arm/mrvl/timer.txt         |  61 +-
>>  drivers/clocksource/Makefile                       |   1 +
>>  drivers/clocksource/timer-mmp.c                    | 837 +++++++++++++++++++++
>>  include/linux/mmp_timer.h                          |  40 +
>>  4 files changed, 933 insertions(+), 6 deletions(-)  create mode 
>> 100644 drivers/clocksource/timer-mmp.c  create mode 100644 
>> include/linux/mmp_timer.h
>> 
>> diff --git a/Documentation/devicetree/bindings/arm/mrvl/timer.txt 
>> b/Documentation/devicetree/bindings/arm/mrvl/timer.txt
>> index 9a6e251..b49cb3e 100644
>> --- a/Documentation/devicetree/bindings/arm/mrvl/timer.txt
>> +++ b/Documentation/devicetree/bindings/arm/mrvl/timer.txt
>> @@ -1,13 +1,62 @@
>>  * Marvell MMP Timer controller
>> 
>> +Each timer have multiple counters, so the timer DT need include 
>> +counter's description.
>> +
>> +1. Timer
>> +
>>  Required properties:
>> -- compatible : Should be "mrvl,mmp-timer".
>> +- compatible : Should be "marvell,mmp-timer".
>>  - reg : Address and length of the register set of timer controller.
>> -- interrupts : Should be the interrupt number.
>> +- clocks : The first clock is the fequency of the apb bus that the 
>> +timer
>> +  attached to. The second clock is the fast clock frequency of the timer.
>> +  This frequency and fast clock are used to calculated delay
>> +  loops for clock operations.
>
>It might be a good idea to use clock-names for "apb" and "fast" and use them in the driver.

>


Yes. I will fix it.

>> +
>> +Optional properties:
>> +- marvell,timer-has-crsr : This timer has CRSR register.
>> +- marvell,timer-has-shadow : This timer has shadow register.
>> +
>> +2. Counter
>
>The coutner nodes appear to be sub-nodes of the timer. That should be stated explicitly.

>


I will fix it.


>> +>> +Required properties:
>> +- compatible : It can be
>> +      "marvell,timer-counter-clkevt" : The counter is used for clock event
>> +                                       device.
>> +      "marvell,timer-counter-clksrc" : The counter is used for clock source.
>> +      "marvell,timer-counter-delay" : The counter is used for delay timer.
>
>These are all Linux-internal details. I don't see why we should need separate strings; Linux should be able to allocate these as appropriate (and a single timer should be able to be both a clocksource and a delay timer).
>
>Is there any reason you envisage for having separate strings here? It douesn't make sense to me to do so.
>
>> +- marvell,timer-counter-id : The counter index in this timer.
>
>It sounds like you could use reg for this, unless you have other sub-nodes you'll need to instantiate?
>
>> 
>> -Example:
>> -       timer0: timer@d4014000 {
>> -               compatible = "mrvl,mmp-timer";
>> -               reg = <0xd4014000 0x100>;
>> +Optional properties:
>> +- marvell,fast-clock : whether the counter use the fast-clock for counting.
>
>It looks below like this is a policy decision, rather than a description of the HW. When would you select the fast clock and when would you not?
>Why can't the driver figure this out?

>


The name is a little confused. There are two types of fast clock inputs in all the SOCes. From vctcxo or PLL1.
In some low power mode, PLL1 will be shutdown, and timer can not use this clock because it need wake up the cores when time out.
Timer driver can not get the information that whether this fast clock is coming from a clock source that will be shutdown in the low
power mode or not.
So i need tell it in the device tree that the fast clock is still active in the low power mode.


>> +- interrupts : The interrupt for clock event device.>> +  Only valid for "marvell,timer-counter-clkevt".
>> +- marvell,timer-counter-cpu : which CPU the counter is bound. Only 
>> +valid for
>> +  "marvell,timer-counter-clkevt".
>
>This sounds like a policy decision, rather than a description of the HW.
>
>Additionally, the number usage seems to be a Linux logical ID, rather than a physical CPU ID. This is broken.
>
>> +- marvell,timer-counter-broadcast : When this counter acts as clock 
>> +event
>> +  device. It is broadcast clock event device.
>> +  Only valid for "marvell,timer-counter-clkevt".
>
>This is a Linux-internal detail. There shouldn't be a binding for this.
>
>> +- marvell,timer-counter-nodynirq : When this counter acts as 
>> +broadcast clock
>> +  event device, it cannot switch the IRQ of broadcast clock event to any CPU.
>> +  Only valid for "marvell,timer-counter-clkevt".
>
>Likewise. Why can't you change the affinity?
>
>For all of the above properties, it sounds like what you actually need/want to do is to check if the number of available timers >= num_possible_cpus(), and if so, (dynamically) allocate a counter to each CPU. If not, set up a timer as a broadcast source (with dynirq).

>
i understand what you suggested
I want to make a driver for a timer, not for timers.
So if the device tree has
timer0 {
}
timer1 {
}
The driver will be probed twice, and it does not need care the relationship between these timers.


To do what your suggested, the device tree will be
timers {
ÂÂÂÂtimer0 {
ÂÂÂÂ};
ÂÂÂÂtimer1 {
ÂÂÂÂ};
}
The driver is for timers, and only be probed once, and it will consider the relationship between the timers. For example, how much counter can be allocated for per-cpu local timer.
If above device tree is accepted, i can change the driver.

>Thanks,
>Mark.