[PATCH v2] watchdog: sirf: add watchdog driver of CSR SiRFprimaII and SiRFatlasVI

[Barry Song]

2013/9/13 Romain Izard <romain.izard.pro at gmail.com>:
> On 2013-09-12, Barry Song <21cnbao at gmail.com> wrote:
>> 2013/9/12 Romain Izard <romain.izard.pro at gmail.com>:
>>>
>>> Does this really mean that both drivers will be loaded?  From what I
>>> understand of device tree, this means to use the "sirf,prima2-tick"
>>> driver, and then if not available, the "sirf-prima2-wdt" driver.
>>> It would be necessary to either add the watchdog support in the
>>> clocksource driver, or create a separate node in the device tree
>>> (with the same address?) to respect the compatible rules.
>>
>> that means both. the 6th timer can act as a watchdog timer when the
>> Watchdog mode is enabled. all 6 timers have been controlled by timer
>> driver, here it is a more-functionality for timer6.
>>
>
> First, from my reading the device tree specification says that the
> compatible property describes an ordered list from which a single driver
> is used.
>
> After checking with the platform code, I understand why this is possible,
> but I think the current syntax is not good.
>
> The general case for device tree is the following one:
> - Linux parses the device tree on startup with of_platform_populate, and
>   creates a struct platform_device per node in the tree with a
>   compatible string.
> - Creating each device calls device_attach, trying to match and bind the
>   new device with already existing drivers, until one of them succeeds
>   or no driver is left.
> - Registering a new device driver ilater calls driver_attach, trying to
>   match and bind each unbound device with the new driver.
>
> As each device can only be bound to a single driver, the normal case
> is that only one compatible driver can be bound to a device. As noted
> in http://xillybus.com/tutorials/device-tree-zynq-3, what happens when
> multiple drivers are compatible with a node is dependent on the
> initialization order.
>
> But in our specific case, "sirf,prima2-tick" does not describe a normal
> device from the kernel point of view, it describes a clocksource node.
> It is used earlier during the startup sequence to avoid cyclic
> dependency issues between the timers and the driver framework. When the
> node is created later on, there is no driver to match with the node from
> the device tree. This is why in this case, and for all clocksource
> nodes, it will be possible to have a second matching compatible string.
>
> So, since this behaviour is specific for clocksource nodes, and does not
> describe the hardware as device tree should, I believe it would be
> appropriate to use "sirf,prima2-tick" as the compatible string for the
> watchdog driver, binding the device left by the clocksource with it.

ok. make sense. i guess the best name for "sirf,prima2-tick" should be
"sirf,prima2-timer" as tick is a software concept but timer is a
hardware concept.

but i will use prima2-tick for both watchdog and clocksource for the
moment as i don't want to touch two tasks in a patch.

>
>
>>>> diff --git a/arch/arm/configs/prima2_defconfig b/arch/arm/configs/prima2_defconfig
>>>> index 002a1ce..eec56e8 100644
>>>> --- a/arch/arm/configs/prima2_defconfig
>>>> +++ b/arch/arm/configs/prima2_defconfig
>>>> @@ -1,4 +1,3 @@
>>>> -CONFIG_EXPERIMENTAL=y
>>>
>>> CONFIG_EXPERIMENTAL is not related to this change.
>>
>> i think it is just due to kernel upgrade. here just make menuconfig
>> with enabling watchdog and save defconfig, then there are some minor
>> differences here.
>>
> Perhaps would it be better to put it in a different patch?

ok.

>
>>>> +
>>>> +     if (match >= counter)
>>>> +             time_left = match-counter;
>>>> +     else
>>>> +             /* rollover */
>>>> +             time_left = (0xffffffffUL - counter) + match;
>>>
>>> As u32 is a modulo 2^32 type, those values are almost the same (off by one).
>>> time_left = match - counter; will be valid in all cases.
>>>
>
> On one side, we have:
>         time_left = match - counter (mod 2^32)
> On the other side we have:
>         time_left = (0xffffffffUL - counter) + match (mod 2^32)
> <=>     time_left = match - counter + 0xFFFFFFFF (mod 2^32)
> As we are in the modulo 2^32 space, adding or substracting 2^32 does not
> change the result
> <=>     time_left = match - counter + 0xFFFFFFFF - 2^32 (mod 2^32)
> And since 0xFFFFFFFF = 2^32 - 1
>         time_left = match - counter - 1; (mod 2^32)
>
>>>> +
>>>> +     if ((0xffffffffUL - counter) >= timeout_ticks)
>>>> +             counter += timeout_ticks;
>>>> +     else
>>>> +             /* Rollover */
>>>> +             counter = timeout_ticks - (0xffffffffUL - counter);
>>>
>>> modulo 2^32 arithmetic, no need for two cases.
>>
>> you mean just "counter += timeout_ticks" is ok?
>
> Yes, with the same reasoning as previously. Adding or substracting
> 0xffffffff for 32 bit unsigned values is the same as substracting
> or adding 1. But we do not even want this 1, we just want to
> rollover like the timer itself does, because it counts on 32 bit
> unsigned values.
>

well. good.

> --
> Romain Izard
>

-barry