[RFC] hwmon: f71882fg: Add watchdog API for F71808E and F71889
in [Kernel]
|
Prev: x86,pat Update the page flags for memtype atomically instead of using memtype_lock. -V3
Next: how to debug a kernel that does not power off?
From: Giel van Schijndel on 23 Mar 2010 19:30 On Wed, Mar 24, 2010 at 12:12:16AM +0100, Giel van Schijndel wrote: > Implement the watchdog API for the Fintek F71808E. Looking at the F71889FG datasheet I think implementing watchdog support for that chip can be accomplished rather easily. I think it's as simple as adding the pin initialisation (multi function select registers 0x2a and 0x2b) sequence to the switch statement in watchdog_start(). That being said; I don't have physical access to that chip myself, thus I couldn't actually test it (unlike the F71808E, for which I've already tested these patches). -- Met vriendelijke groet, With kind regards, Giel van Schijndel
From: Giel van Schijndel on 24 Mar 2010 05:40 On Wed, Mar 24, 2010 at 09:37:43AM +0100, Hans de Goede wrote: > Nack: > As the watchdog has its own SIO logical device number, it should > have a separate driver, not have support glued to the hwmon driver. Thus, if I understand correctly, you would suggest for me to implement a new driver in drivers/watchdog/ to implement this driver? -- Met vriendelijke groet, With kind regards, Giel van Schijndel
From: Giel van Schijndel on 24 Mar 2010 11:40 On Wed, Mar 24, 2010 at 11:33:00AM +0100, Hans de Goede wrote: > On 03/24/2010 10:36 AM, Giel van Schijndel wrote: >>On Wed, Mar 24, 2010 at 09:37:43AM +0100, Hans de Goede wrote: >>> Nack: >>> As the watchdog has its own SIO logical device number, it should >>> have a separate driver, not have support glued to the hwmon driver. >> >> Thus, if I understand correctly, you would suggest for me to >> implement a new driver in drivers/watchdog/ to implement this >> feature? > > Yes, correct. So I've now started out working on moving the watchdog code to a separate driver. I/O port conflict However, this driver would have to lock the SIO port range as well. Unlike the hardware monitor, however, the watchdog (VID controller on some datasheets) doesn't appear to provide for an alternative I/O port mapping. Meaning the wathdog driver would have to maintain a permanent hold on the SIO port range. This would thus interfere with the operation of the f71882fg driver. I.e. it would prevent the device probing stage from working, thus preventing it from loading *after* my in-development watchdog driver. Alternative logical device port mapping Regarding address mapping: the 16 bit SIO_REG_ADDR(0x60) register does exist for this logical device, though according to the datasheet it is set to 0x0000 by default. Reading the value from hardware it claims to be mapped on 0x0AE0, trying to read from that port range (using f71882fg_read8) yields nothing more than 0xFF for any register mentioned in the datasheet. Hardware monitor alternate port range Further, when looking at the way that SIO_REG_ADDR is currently used by the f71882fg driver I get more confused: > *hwmon_addr = superio_inw(sioaddr, SIO_REG_ADDR); > /* error checking: *hwmon_addr != 0 */ > *hwmon_addr &= ~(REGION_LENGTH - 1); /* Ignore 3 LSB */ For the F71808E and F71889, which both have 0x295, for hardware monitor base address that code ^^ combined with the addition of ADDR_REG_OFFSET and DATA_REG_OFFSET (see f71882fg_(read|write)8) all of this basically amounts to mangling 0x295 -> 0x290 -> (0x295,0x296). So my question: is there any particular reason for performing this address mangling? Mostly: is there anything there that I should try replicating in order to get mapping of the watchdog device on an alternate port range working? PS Perhaps this would be easier to converse about if you had the datasheet? -- Met vriendelijke groet, With kind regards, Giel van Schijndel
From: Giel van Schijndel on 24 Mar 2010 16:40 On Wed, Mar 24, 2010 at 05:20:59PM +0100, Hans de Goede wrote: > On 03/24/2010 04:51 PM, Alan Cox wrote: >>> hold on the SIO port range. This would thus interfere with the >>> operation of the f71882fg driver. I.e. it would prevent the device >>> probing stage from working, thus preventing it from loading *after* >>> my in-development watchdog driver. >> >> There are two ways to deal with that really >> >> 1. Add a multi-function driver - it finds the chip and claims the >> port regions and then provides methods for locked access to them as >> well as creating other device instances that the drivers map to >> (probably platform devices ?) which in turn trigger the >> loading/binding of the relevant low level devices. >> >> 2. Fix the kernel request_resource stuff to support a sleeping non >> exclusive resource so request/free of regions can be used as a >> resource semaphore by co-operative devices. >> >> #2 is actually not hard but when I did the patch originally it then >> wasn't needed by the driver I had in mind for other reasons. >> >> See http://groups.google.com/group/linux.kernel/msg/1425fc2aad32e6ea >> >> Maybe its worth resurrecting ? > > Or, a bit more specific solution would be to resurrect the superio > lock coordinator patches, which were written (but never merged) 2 > years ago to solve exactly this problem: > http://lists.lm-sensors.org/pipermail/lm-sensors/2008-July/023743.html When performing some searches I find messages going back to at least september 2006 [1] [2]. With multiple occurences of these patches being "dusted off". They never got applied though, and for that (*not* applying them) I cannot find any reason. Is there any? Or did people just become uninterested and let the patches "collect dust"? Then regarding Alan's patch. The fact that it is a *lot* simpler than Jim Cromie's SuperIO locks coordinator is IMHO a significant advantage over the latter. Furthermore, "lock coordinator" seems like a bad name to me, since those patches seem especially concerned with centralising the way that SuperIO devices are probed for. Thus if the only thing required is to serialize resource access I think plain-ol' locking (with the ability to block on the lock, rather than polling for it). [1] http://lists.lm-sensors.org/pipermail/lm-sensors/2006-June/016476.html [2] http://lkml.org/lkml/2006/9/14/20 -- Met vriendelijke groet, With kind regards, Giel van Schijndel
From: Giel van Schijndel on 25 Mar 2010 05:00
On Wed, Mar 24, 2010 at 03:51:51PM +0000, Alan Cox wrote: >> hold on the SIO port range. This would thus interfere with the >> operation of the f71882fg driver. I.e. it would prevent the device >> probing stage from working, thus preventing it from loading *after* >> my in-development watchdog driver. > > There are two ways to deal with that really > > ... snip ... > > 2. Fix the kernel request_resource stuff to support a sleeping non > exclusive resource so request/free of regions can be used as a resource > semaphore by co-operative devices. > > #2 is actually not hard but when I did the patch originally it then > wasn't needed by the driver I had in mind for other reasons. > > See http://groups.google.com/group/linux.kernel/msg/1425fc2aad32e6ea > > Maybe its worth resurrecting ? In this particular case I most definitely think it's worth resurrecting. Using your patch (I had to change the IORESOURCE_MUXED constant's value though) I can completely solve the I/O sharing issue for the f71882fg + watchdog case with only a single line modification to the f71882fg driver. One question regarding your patch though: how will it function when driver (A) locks an I/O region using request_region() then driver (B) comes along and tries to lock it using request_muxed_region()? One problem I imagine might occur is that driver (A)'s 'struct resource' doesn't have the IORESOURCE_MUXED flags set, thus simply won't wake up driver (B) on the 'muxed_resource_wait' queue. -- Met vriendelijke groet, With kind regards, Giel van Schijndel |