On Tue, Apr 24, 2012 at 09:18:02PM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 24, 2012 at 07:12:10PM +0530, viresh kumar wrote: > > On 4/24/12, Andrew Lunn <andrew@xxxxxxx> wrote: > > > Sorry, but still wrong. > > > > > > The clock is optional. If we can find a clock, turn it on. If not, > > > keep going.... > > > > > > You patch causes the missing clock to become a fatal error. > > > > > > This sata_mv exists in multiple forms. It can be part of a SoC. It can > > > also be on a PCI bus. In the PCI form, it is unlikely to have a clk > > > which can be controlled. When built into a SoC, namely one of the > > > Orion family, dove, orion5x, mv78xx0 do not have a clock which can be > > > controlled. However kirkwood does have a clock. > > > > > > So, kirkwood will provide a clock and expects that sata_mv will turn > > > it on. All the other ways of using sata_mv will not provide a clock, > > > but still expect the driver to be happy. > > > > Hmm. What this code does now is: > > If HAVE_CLK is selected, then there must be a clock for the device. Otherwise > > it will always pass. > > > > You want not to return error if a platform does have HAVE_CLK, but doesn't > > have a clock for sata? That would be simple to fix, but want to confirm if this > > is actually required. > > > > @Russell: Can we have your view also? > > Look, it's very very simple. > > As far as drivers are concerned: > > clk_get() returns an opaque cookie which _happens_ to be called 'struct clk'. > Drivers _must_ _not_ dereference or interpret this cookie in any way, apart > from the singular case where they use IS_ERR() to determine if clk_get() > failed, and PTR_ERR() to translate that into an error value. As far as > drivers are concerned _everything_ _else_ is a valid cookie and must > never be treated specially. > > That much I hope is (and has been) totally crystal clear for some time. > > Now, for drivers which use the clk API, and are used on platforms which > have the clk API and those which do not have the CLK API. Those which > do have the clk API define HAVE_CLK. We know how to deal with those, > and that's through having a correct and valid clk API implementation. > > For those which don't, as I've already suggested, we need clk_get() to > return a non-error value. I really don't care what value it returns, > because as far as drivers using the clk API are concerned, they are not > allowed to interpret the value in _any_ _other_ _way_ other than whether > it is an error or not. So NULL is a good value for this. It's a > non-error cookie value, but (void *)1 is also good too. > > Now, the question comes: do we want to provide a dummy API? Yes. How > do we want to enable the provision of the dummy API? Through !HAVE_CLK? > I think that's a sane move, and any driver which _really_ _does_ have a > hard dependency on the clk API (eg, amba-clcd needing the clk API to > control the LCD pixel clock rate) should depend on this symbol. > > As for drivers printing out crap if they don't have the clk API configured, > wtf? What does it matter? If the clk API is not configured, it means > the platform has no control over the clocking, and the clocking is fixed. > So why tell the user of each driver which could have clk API support that > same fact over and over again during the kernel boot process? What do you > expect the user to do about it? Scream at the manufacturer that they > didn't implement a feature found on embedded devices on their swankey > platform? Maybe its not appropriate there? > Hi Russell No problems with what is above. The bit in contention is this > Finally, if a platform has clk API support enabled, and a driver requests > a clock, and clk_get() returns an error, it means the clock was not found. > That's a fatal error for the driver, because it means that something is > wrong in the lookup tables - moreover, it means that _potentially_ someone > screwed up the clk matching and this device has a clock which needs some > control, but wasn't found. I don't think ignoring that kind of error, > even by printing a warning, is a particularly good approach - it seems > to me it makes things fragile. What if this missing clock causes the > bus to your device to ultimately hang? You are correct about lockup. I made a typo, match failed, lateinit turned the clock off, and the device hung on the next access. Is that fragile? It should only happen to somebody porting to a new SoC playing with clks. Anyway, what you want, is that the MV_SATA driver knows if there should be a clock and only then call clk_get(). How do we reliably teach the MV_SATA driver this, so we don't cause an regressions? If this platform_device is for a PCI bus device, there probably won't be a clock. If this is a SoC platform_device is for a Dove, Orion5x, mv78xx0, there won't be a clock. If its a kirkwood SoC platform device, there should be a clock. If its a PowerPC platform_device, i've no idea. Have i missed something? It seems to come down to, a bit of fragile handling of clks, or possibly regressing some PowerPC machines. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
