Patchwork [V3,07/12] ata/sata_mv: Remove conditional compilation of clk code

login
register
mail settings
Submitter Viresh KUMAR
Date April 24, 2012, 11:21 a.m.
Message ID <abbcf661dc58948794839eb0fba1d5a0950ec848.1335266056.git.viresh.kumar@st.com>
Download mbox | patch
Permalink /patch/154658/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Viresh KUMAR - April 24, 2012, 11:21 a.m.
With addition of dummy clk_*() calls for non CONFIG_HAVE_CLK cases in clk.h,
there is no need to have clk code enclosed in #ifdef CONFIG_HAVE_CLK, #endif
macros.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: linux-ide@vger.kernel.org
---
 drivers/ata/sata_mv.c |   32 ++++++++++----------------------
 1 files changed, 10 insertions(+), 22 deletions(-)
Andrew Lunn - April 24, 2012, noon
> -#if defined(CONFIG_HAVE_CLK)
>  	hpriv->clk = clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(hpriv->clk))
> -		dev_notice(&pdev->dev, "cannot get clkdev\n");
> -	else
> -		clk_enable(hpriv->clk);
> -#endif
> +	if (IS_ERR(hpriv->clk)) {
> +		dev_err(&pdev->dev, "cannot get clkdev\n");
> +		return PTR_ERR(hpriv->clk);
> +	}
> +
> +	clk_enable(hpriv->clk);

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.

    Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lothar Waßmann - April 24, 2012, 12:51 p.m.
Hi,

Andrew Lunn writes:
> > -#if defined(CONFIG_HAVE_CLK)
> >  	hpriv->clk = clk_get(&pdev->dev, NULL);
> > -	if (IS_ERR(hpriv->clk))
> > -		dev_notice(&pdev->dev, "cannot get clkdev\n");
> > -	else
> > -		clk_enable(hpriv->clk);
> > -#endif
> > +	if (IS_ERR(hpriv->clk)) {
> > +		dev_err(&pdev->dev, "cannot get clkdev\n");
> > +		return PTR_ERR(hpriv->clk);
> > +	}
> > +
> > +	clk_enable(hpriv->clk);
> 
> 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.
> 
The clock API should accept NULL pointers as valid clocks and
treat them as NOP.
Thus drivers wouldn't have to worry about whether they actually got a
clock to manage or if their clocks are just dummies.


Lothar Waßmann
viresh kumar - April 24, 2012, 1:42 p.m.
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?

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Lunn - April 24, 2012, 2:29 p.m.
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.
> 

-#if defined(CONFIG_HAVE_CLK)
    hpriv->clk = clk_get(&pdev->dev, NULL);
-   if (IS_ERR(hpriv->clk))
-           dev_notice(&pdev->dev, "cannot get clkdev\n");
-   else
-           clk_enable(hpriv->clk);
-#endif
+   if (IS_ERR(hpriv->clk)) {
+           dev_err(&pdev->dev, "cannot get clkdev\n");
+           return PTR_ERR(hpriv->clk);
+   }
+
+   clk_enable(hpriv->clk);

There are three use cases....

1) The platform does not implement HAVE_CLK. So we are using your NOP
   operations.

   clk_get() returns NULL.

   IS_ERR(NULL) is false. So it keeps going, calls clk_enable() which is
   also NOP and the driver is happy.

2) The platform does have HAVE_CLK. So we are using driver/clk/
   code. There is no clk defined for the device, since there is no
   controllable clk. Its using the PCI clock, or some hard wired
   internal SoC clock.

   clk_get() returns ERR_PTR(-ENOENT)

   IS_ERR(hpriv->clk) is true, you output a dev_err() and the device
   probe fails.

   This is wrong. Not having the clk is not fatal. The old code would
   carefully not call clk_enable(), since it has an error pointer, not
   have a valid clk, and would also not call clk_disable during
   removal.

3) The platform does have HAVE_CLK. So we are using driver/clk/ code.
   There is however a gateable clock driving this lump of silicon.

   clk_get() returns a valid pointer to a clk.
   IS_ERR(hpriv->clk) is false, so it keeps going.
   clk_enable() is called and the driver is happy.

   Well, actually, his brings up a new issues 

static int __clk_enable(struct clk *clk)
{
        int ret = 0;

        if (!clk)
                return 0;

        if (WARN_ON(clk->prepare_count == 0))
                return -ESHUTDOWN;

   this should actually be clk_prepare_enable(). Did you see my
   patches adding generic clk framework support for Orion. I fixed
   this as part of that patch set.

Anyway.... You asked:

> 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.

Correct.

	Andrew
   
   
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
viresh kumar - April 24, 2012, 5:05 p.m.
On 4/24/12, Andrew Lunn <andrew@lunn.ch> wrote:
> static int __clk_enable(struct clk *clk)
> {
>         int ret = 0;
>
>         if (!clk)
>                 return 0;
>
>         if (WARN_ON(clk->prepare_count == 0))
>                 return -ESHUTDOWN;
>
>    this should actually be clk_prepare_enable(). Did you see my
>    patches adding generic clk framework support for Orion. I fixed
>    this as part of that patch set.

Sorry for missing it earlier. Ya i know it must be clk_prepare_enable(),
but it wasn't motive of my patchset.

Sorry, i haven't gone through your patches for Orion.

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux - April 24, 2012, 8:18 p.m.
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?

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
viresh kumar - April 25, 2012, 3:02 a.m.
On Wed, Apr 25, 2012 at 1:48 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> Look, it's very very simple.

Thanks for explaining.

> 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?

This is what i was thinking too and thats why floated this version of patch.
But as Andrew said, clk API support is enabled for them, but still they
don't have a clk for sata. To get this working, there are two solutions:
- Create dummy clk for sata for that platform, so clk_get doesn't fail.
- Check for error before every call to clk APIs after clk_get().

Andrew favored the second one. Which means, even on platforms
with clk API defined and clk enable required, if there are some issues
with lookup table, and clk_get() fails, system may hang when registers
are accessed. For this i favored first one.

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Lunn - April 25, 2012, 5:28 a.m.
> This is what i was thinking too and thats why floated this version of patch.
> But as Andrew said, clk API support is enabled for them, but still they
> don't have a clk for sata. To get this working, there are two solutions:
> - Create dummy clk for sata for that platform, so clk_get doesn't fail.
> - Check for error before every call to clk APIs after clk_get().
> 
> Andrew favored the second one. Which means, even on platforms
> with clk API defined and clk enable required, if there are some issues
> with lookup table, and clk_get() fails, system may hang when registers
> are accessed. For this i favored first one.

I'm not too sure how you are going to achieve 1)

config SATA_MV
        tristate "Marvell SATA support"
        help
          This option enables support for the Marvell Serial ATA family.
          Currently supports 88SX[56]0[48][01] PCI(-X) chips,
          as well as the newer [67]042 PCI-X/PCIe and SOC devices.

So this driver can be used with anything which has a PCI(-X) or PCIe
bus, or Orion SoC and a few PowerPC SoCs.

The SoCs are not too bad, we know which ones they are and we can add
dummy entries to their device tree. However, how do you want to handle
the PCI devices? Create the dummy entry somewhere in the middle of the
PCI core?

Thought not.

A comment to:
> - Check for error before every call to clk APIs after clk_get().

There are only two calls. clk_prepare_enable() in the probe and
clk_disable_unprepare() in the remove function.

	Andrew


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lothar Waßmann - April 25, 2012, 6:43 a.m.
Hi,

Andrew Lunn writes:
> > This is what i was thinking too and thats why floated this version of patch.
> > But as Andrew said, clk API support is enabled for them, but still they
> > don't have a clk for sata. To get this working, there are two solutions:
> > - Create dummy clk for sata for that platform, so clk_get doesn't fail.
> > - Check for error before every call to clk APIs after clk_get().
> > 
> > Andrew favored the second one. Which means, even on platforms
> > with clk API defined and clk enable required, if there are some issues
> > with lookup table, and clk_get() fails, system may hang when registers
> > are accessed. For this i favored first one.
> 
> I'm not too sure how you are going to achieve 1)
> 
> config SATA_MV
>         tristate "Marvell SATA support"
>         help
>           This option enables support for the Marvell Serial ATA family.
>           Currently supports 88SX[56]0[48][01] PCI(-X) chips,
>           as well as the newer [67]042 PCI-X/PCIe and SOC devices.
> 
> So this driver can be used with anything which has a PCI(-X) or PCIe
> bus, or Orion SoC and a few PowerPC SoCs.
> 
> The SoCs are not too bad, we know which ones they are and we can add
> dummy entries to their device tree. However, how do you want to handle
> the PCI devices? Create the dummy entry somewhere in the middle of the
> PCI core?
> 
> Thought not.
> 
AFAICT the PCI archs do not have HAVE_CLK enabled. Thus they will have
a dummy clk API in place that provides the driver with a non-error
clk_get() return value.


Lothar Waßmann
Andrew Lunn - April 25, 2012, 7:14 a.m.
> AFAICT the PCI archs do not have HAVE_CLK enabled. Thus they will have
> a dummy clk API in place that provides the driver with a non-error
> clk_get() return value.
 
Hi Lothar

Please could you explain this a little bit more. What exactly is a PCI
arch?

Most of the Orion SoC have PCIe busses. Does this make them an PCI
arch? Because they also have HAVE_CLK. There are more SoCs with PCI.

      Thanks
	Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lothar Waßmann - April 25, 2012, 8:35 a.m.
Andrew Lunn writes:
> > AFAICT the PCI archs do not have HAVE_CLK enabled. Thus they will have
> > a dummy clk API in place that provides the driver with a non-error
> > clk_get() return value.
>  
> Hi Lothar
> 
> Please could you explain this a little bit more. What exactly is a PCI
> arch?
> 
> Most of the Orion SoC have PCIe busses. Does this make them an PCI
> arch? Because they also have HAVE_CLK. There are more SoCs with PCI.
> 
If an arch has HAVE_CLK enabled it must provide valid clocks (be it
dummy clocks) for all devices it supports.


Lothar Waßmann
Andrew Lunn - April 25, 2012, 9:31 a.m.
> If an arch has HAVE_CLK enabled it must provide valid clocks (be it
> dummy clocks) for all devices it supports.

So, lets take the theoretical exaple of a unicore32 PUV3

config ARCH_PUV3
        def_bool y
        select CPU_UCV2
        select GENERIC_CLOCKEVENTS
        select HAVE_CLK
        select ARCH_REQUIRE_GPIOLIB
        select ARCH_HAS_CPUFREQ

Seems like this somewhat unknown, to me at least, architecture, also
supports PCI. So i plug in an HP Adaptec AIC8120 SATA host bus adapter
into a spare PCI slot. This uses the Marvell 88SX6041, which the
SATA_MV driver supports.

Should i expect that the unicore32 PUV3 has created a dummy clk for
this case?

Thanks
	Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux - April 25, 2012, 10:37 a.m.
On Wed, Apr 25, 2012 at 11:31:09AM +0200, Andrew Lunn wrote:
> > If an arch has HAVE_CLK enabled it must provide valid clocks (be it
> > dummy clocks) for all devices it supports.
> 
> So, lets take the theoretical exaple of a unicore32 PUV3
> 
> config ARCH_PUV3
>         def_bool y
>         select CPU_UCV2
>         select GENERIC_CLOCKEVENTS
>         select HAVE_CLK
>         select ARCH_REQUIRE_GPIOLIB
>         select ARCH_HAS_CPUFREQ
> 
> Seems like this somewhat unknown, to me at least, architecture, also
> supports PCI. So i plug in an HP Adaptec AIC8120 SATA host bus adapter
> into a spare PCI slot. This uses the Marvell 88SX6041, which the
> SATA_MV driver supports.
> 
> Should i expect that the unicore32 PUV3 has created a dummy clk for
> this case?

Are you going to bother answering the detailed email I sent describing
my POV on this case?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Lunn - April 25, 2012, 11:24 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 7336d4a..2b37b93 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -551,9 +551,7 @@  struct mv_host_priv {
 	u32			irq_mask_offset;
 	u32			unmask_all_irqs;
 
-#if defined(CONFIG_HAVE_CLK)
 	struct clk		*clk;
-#endif
 	/*
 	 * These consistent DMA memory pools give us guaranteed
 	 * alignment for hardware-accessed data structures,
@@ -4063,13 +4061,13 @@  static int mv_platform_probe(struct platform_device *pdev)
 				   resource_size(res));
 	hpriv->base -= SATAHC0_REG_BASE;
 
-#if defined(CONFIG_HAVE_CLK)
 	hpriv->clk = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(hpriv->clk))
-		dev_notice(&pdev->dev, "cannot get clkdev\n");
-	else
-		clk_enable(hpriv->clk);
-#endif
+	if (IS_ERR(hpriv->clk)) {
+		dev_err(&pdev->dev, "cannot get clkdev\n");
+		return PTR_ERR(hpriv->clk);
+	}
+
+	clk_enable(hpriv->clk);
 
 	/*
 	 * (Re-)program MBUS remapping windows if we are asked to.
@@ -4096,12 +4094,8 @@  static int mv_platform_probe(struct platform_device *pdev)
 		return 0;
 
 err:
-#if defined(CONFIG_HAVE_CLK)
-	if (!IS_ERR(hpriv->clk)) {
-		clk_disable(hpriv->clk);
-		clk_put(hpriv->clk);
-	}
-#endif
+	clk_disable(hpriv->clk);
+	clk_put(hpriv->clk);
 
 	return rc;
 }
@@ -4117,17 +4111,11 @@  err:
 static int __devexit mv_platform_remove(struct platform_device *pdev)
 {
 	struct ata_host *host = platform_get_drvdata(pdev);
-#if defined(CONFIG_HAVE_CLK)
 	struct mv_host_priv *hpriv = host->private_data;
-#endif
 	ata_host_detach(host);
 
-#if defined(CONFIG_HAVE_CLK)
-	if (!IS_ERR(hpriv->clk)) {
-		clk_disable(hpriv->clk);
-		clk_put(hpriv->clk);
-	}
-#endif
+	clk_disable(hpriv->clk);
+	clk_put(hpriv->clk);
 	return 0;
 }