From mboxrd@z Thu Jan 1 00:00:00 1970 From: romain.izard.pro@gmail.com (Romain Izard) Date: Thu, 12 Sep 2013 16:10:39 +0000 (UTC) Subject: [PATCH v2] watchdog: sirf: add watchdog driver of CSR SiRFprimaII and SiRFatlasVI References: <1378953828-374-1-git-send-email-Baohua.Song@csr.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2013-09-12, Barry Song <21cnbao@gmail.com> wrote: > 2013/9/12 Romain Izard : >> >> 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. >>> 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? >>> + >>> + 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