Re: [PATCH V3 07/12] ata/sata_mv: Remove conditional compilation of clk code

[Andrew Lunn <andrew@lunn.ch>]

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@lunn.ch> 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