[PATCH v2] watchdog: sirf: add watchdog driver of CSR SiRFprimaII and SiRFatlasVI
21cnbao at gmail.com
Sat Sep 21 23:07:05 EDT 2013
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
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 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?
>>>> + 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.
> Romain Izard
More information about the linux-arm-kernel