Add a common struct clk
in [Kernel]
|
Prev: X86:reboot.c Add some dmi entries to pci_reboot_dmi_table.
Next: ext3: fix non-update ctime when changing the file's permission by setfacl
From: Benjamin Herrenschmidt on 12 Jun 2010 01:20 > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that > makes it impossible to call those functions in interrupt context. And doing clk_enable/clk_disable from interrupt context is a bad idea as well :-) Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Dooks on 13 Jun 2010 18:30 On Fri, Jun 11, 2010 at 12:08:01PM +0200, Lothar Wa?mann wrote: > Hi, > > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that > > > > > makes it impossible to call those functions in interrupt context. > > IMHO if a device generates an irq its clock should already be on. This > > way you don't need to enable or disable a clock in irq context. > > > You may want to disable a clock in the IRQ handler. The VPU driver in > the Freescale BSP for i.MX51 does exactly this. > Anyway I don't see any reason for using a mutex here instead of > spin_lock_irq_save() as all other implementations do. Hmm, then again the VPU driver may just be a bit wrong here. We could protect each clock with a spinlock, but that would end up with a problem of spinning where we have clocks that takes 100s of usec or so to init. See all PLLs on S3C devices, where it can take 100-300uS to get a stable clock out of the device. -- Ben (ben(a)fluff.org, http://www.fluff.org/) 'a smiley only costs 4 bytes' -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Dooks on 13 Jun 2010 18:30 On Fri, Jun 11, 2010 at 10:14:35AM +0200, Lothar Wa?mann wrote: > Hi, > > > > > +static inline int clk_enable(struct clk *clk) > > > > +{ > > > > + int ret = 0; > > > > + > > > > + if (!clk->ops->enable) > > > > + return 0; > > > > + > > > > + mutex_lock(&clk->mutex); > > > > + if (!clk->enable_count) > > > > + ret = clk->ops->enable(clk); > > > > + > > > > + if (!ret) > > > > + clk->enable_count++; > > > > + mutex_unlock(&clk->mutex); > > > > + > > > > + return ret; > > > > +} > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that > makes it impossible to call those functions in interrupt context. I think that is a bad idea, unless you can provide otherwise. These calls can sleep depending on implementation, and thus I would like to ensure that they are marked as might-sleep. Is there any specific reason? If so, we need to add some form of ops where we have _nosleep specificially for this case. -- Ben (ben(a)fluff.org, http://www.fluff.org/) 'a smiley only costs 4 bytes' -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jeremy Kerr on 13 Jun 2010 23:20 Hi Ben, > You also need a warning that even if it protects the clock, it may not > protect any access to the hardware implementing it. Yep, agreed. HW clock implementations are free to acquire the mutex in their ops. > > I believe we need to ensure that clocks are enabled when clk_enable > > returns, so we'll need some mechanism for waiting on the thread doing > > the enable/disable. Since (as you say) some clocks may take 100s of > > microseconds to enable, we'll need a lock that we can hold while > > sleeping. > > Well, mutexes give us that, whilst enabling we hold the mutex. Exactly, that's why I think the mutex option is the best way to go. > > I've just yesterday added the following to my tree, to allow dynamic > > initialisation: > > > > static inline void clk_init(struct clk *clk, const struct clk_ops *ops) > > { > > > > clk->ops = ops; > > clk->enable_count = 0; > > mutex_init(&clk->mutex); > > > > } > > > > So we can do this either way. > > the above is in my view better. By 'the above' do you mean doing the mutex init at registration time, or the clk_init code above? Either way should be fine; delaying the mutex_init until registration will has the nice property of not requiring the clock name to be passed to INIT_CLK. > > I've been debating dropping the get_parent and set_parent ops entirely, > > actually; setting a parent seems to be quite specific to hardware (you > > have to know that a particular clock can be a parent of another clock), > > so it seems like something that we shouldn't expose to drivers through > > this API. For the code that knows the hardware, it can probably access > > the underlying clock types directly. > > Not really, and it is in use with extant drivers, so not easily > removable either. OK, is set_parent used much? I can see the use of get_parent, but calls set_parent need to know specifics of the clock hardware. > > Checking for the ops first allows us to skip the mutex acquire, but I'm > > happy either way. > > erm, sorry, yes, you can check for them before mutex. any chages > should be done with mutex held. Yep. > > Using default ops would mean a couple of things: > > > > 1) we can no longer mark the real ops as const; and > > 2) we can no longer avoid the hard-to-predict indirect branch > > ok, how about people have to mark these as a default non op in their > clock structure, and then error if they try and register a clock with > null ops. anyone changing these to NULL later deserves all the pain and > agony they get. That addresses the first point, but still means we have an unnecessary indirect branch to a function that does nothing. Since I've unlined the code where this happens, the checks for null ops are pretty unobtrusive. If we require all ops to be not-null, then we'll need much larger chunks of code where the ops are defined. I like that you can just set the ops callbacks that you need, and the rest "just works". Cheers, Jeremy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Lothar Waßmann on 14 Jun 2010 02:40
Hi, Benjamin Herrenschmidt writes: > On Fri, 2010-06-11 at 12:08 +0200, Lothar Wa�mann wrote: > > Hi, > > > > > > > > Using a mutex in clk_enable()/clk_disable() is a bad idea, since that > > > > > > makes it impossible to call those functions in interrupt context. > > > IMHO if a device generates an irq its clock should already be on. This > > > way you don't need to enable or disable a clock in irq context. > > > > > You may want to disable a clock in the IRQ handler. The VPU driver in > > the Freescale BSP for i.MX51 does exactly this. > > Anyway I don't see any reason for using a mutex here instead of > > spin_lock_irq_save() as all other implementations do. > > Because you suddenly make it impossible to sleep inside enable/disable ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ??? All implementations so far use spin_lock_irq_save()! How would you be able to sleep with a mutex held? If you hold a lock you must not sleep, no matter what sort of lock it is. Lothar Wa�mann -- ___________________________________________________________ Ka-Ro electronics GmbH | Pascalstra�e 22 | D - 52076 Aachen Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 Gesch�ftsf�hrer: Matthias Kaussen Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 www.karo-electronics.de | info(a)karo-electronics.de ___________________________________________________________ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |