[PATCH V3 06/12] ata/pata_arasan: Remove conditional compilation of clk code

[PATCH V3 06/12] ata/pata_arasan: Remove conditional compilation of clk code

[Viresh Kumar]

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: linux-ide@vger.kernel.org
---
 drivers/ata/pata_arasan_cf.c |   14 +-------------
 1 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/pata_arasan_cf.c b/drivers/ata/pata_arasan_cf.c
index fc2db2a..09f0729 100644
--- a/drivers/ata/pata_arasan_cf.c
+++ b/drivers/ata/pata_arasan_cf.c
@@ -184,10 +184,8 @@
 struct arasan_cf_dev {
 	/* pointer to ata_host structure */
 	struct ata_host *host;
-	/* clk structure, only if HAVE_CLK is defined */
-#ifdef CONFIG_HAVE_CLK
+	/* clk structure */
 	struct clk *clk;
-#endif
 
 	/* physical base address of controller */
 	dma_addr_t pbase;
@@ -312,13 +310,11 @@ static int cf_init(struct arasan_cf_dev *acdev)
 	unsigned long flags;
 	int ret = 0;
 
-#ifdef CONFIG_HAVE_CLK
 	ret = clk_enable(acdev->clk);
 	if (ret) {
 		dev_dbg(acdev->host->dev, "clock enable failed");
 		return ret;
 	}
-#endif
 
 	spin_lock_irqsave(&acdev->host->lock, flags);
 	/* configure CF interface clock */
@@ -344,9 +340,7 @@ static void cf_exit(struct arasan_cf_dev *acdev)
 	writel(readl(acdev->vbase + OP_MODE) & ~CFHOST_ENB,
 			acdev->vbase + OP_MODE);
 	spin_unlock_irqrestore(&acdev->host->lock, flags);
-#ifdef CONFIG_HAVE_CLK
 	clk_disable(acdev->clk);
-#endif
 }
 
 static void dma_callback(void *dev)
@@ -828,13 +822,11 @@ static int __devinit arasan_cf_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-#ifdef CONFIG_HAVE_CLK
 	acdev->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(acdev->clk)) {
 		dev_warn(&pdev->dev, "Clock not found\n");
 		return PTR_ERR(acdev->clk);
 	}
-#endif
 
 	/* allocate host */
 	host = ata_host_alloc(&pdev->dev, 1);
@@ -899,9 +891,7 @@ static int __devinit arasan_cf_probe(struct platform_device *pdev)
 			&arasan_cf_sht);
 
 free_clk:
-#ifdef CONFIG_HAVE_CLK
 	clk_put(acdev->clk);
-#endif
 	return ret;
 }
 
@@ -912,9 +902,7 @@ static int __devexit arasan_cf_remove(struct platform_device *pdev)
 
 	ata_host_detach(host);
 	cf_exit(acdev);
-#ifdef CONFIG_HAVE_CLK
 	clk_put(acdev->clk);
-#endif
 
 	return 0;
 }
-- 
1.7.9

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

[Viresh Kumar]

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(-)

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;
 }
 
-- 
1.7.9

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

[Andrew Lunn]

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

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

[Lothar Waßmann]

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

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@karo-electronics.de
___________________________________________________________

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

[viresh kumar]

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

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

[Andrew Lunn]

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
   
   

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

[viresh kumar]

On 4/24/12, Andrew Lunn <andrew@lunn.ch> wrote:
>    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.

Whatever i wrote was intentional, because i didn't had this expectation
of yours. :(
Please see if following is fine:

From: Viresh Kumar <viresh.kumar@st.com>
Date: Sat, 21 Apr 2012 11:16:53 +0530
Subject: [PATCH V3 Resend] ata/sata_mv: Remove conditional compilation
of clk code

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 |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 7336d4a..37503b8 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,11 @@ 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

 	/*
 	 * (Re-)program MBUS remapping windows if we are asked to.
@@ -4096,12 +4092,10 @@ 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

 	return rc;
 }
@@ -4117,17 +4111,13 @@ 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
 	return 0;
 }

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

[Andrew Lunn]

> Please see if following is fine:

Hi Viresh

This looks good. Thanks

     Andrew

> 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 |   10 ----------
>  1 files changed, 0 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 7336d4a..37503b8 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,11 @@ 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
> 
>  	/*
>  	 * (Re-)program MBUS remapping windows if we are asked to.
> @@ -4096,12 +4092,10 @@ 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
> 
>  	return rc;
>  }
> @@ -4117,17 +4111,13 @@ 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
>  	return 0;
>  }

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

[viresh kumar]

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

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

[Russell King - ARM Linux]

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?

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

[viresh kumar]

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

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

[Andrew Lunn]

> 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

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

[Lothar Waßmann]

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

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@karo-electronics.de
___________________________________________________________

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

[Andrew Lunn]

> 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

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

[Lothar Waßmann]

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

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@karo-electronics.de
___________________________________________________________

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

[Andrew Lunn]

> 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

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

[Russell King - ARM Linux]

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?

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

[Andrew Lunn]

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