Patchwork [RFC,v3,06/13] ahci-platform: Add support for devices with more then 1 clock

login
register
mail settings
Submitter Hans de Goede
Date Jan. 18, 2014, 11:48 p.m.
Message ID <1390088935-7193-7-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/312348/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Hans de Goede - Jan. 18, 2014, 11:48 p.m.
The allwinner-sun4i AHCI controller needs 2 clocks to be enabled and the
imx AHCI controller needs 3 clocks to be enabled.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |  1 +
 drivers/ata/ahci.h                                 |  3 +-
 drivers/ata/ahci_platform.c                        | 95 +++++++++++++++-------
 3 files changed, 67 insertions(+), 32 deletions(-)
Russell King - ARM Linux - Jan. 19, 2014, 12:38 p.m.
On Sun, Jan 19, 2014 at 12:48:48AM +0100, Hans de Goede wrote:
> +static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
> +{
> +	int c, rc;
> +
> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {

	for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) {

> +		rc = clk_prepare_enable(hpriv->clks[c]);
> +		if (rc) {
> +			dev_err(dev, "clock prepare enable failed");
> +			goto disable_unprepare_clk;
> +		}
> +	}
> +	return 0;
> +
> +disable_unprepare_clk:
> +	while (--c >= 0)
> +		clk_disable_unprepare(hpriv->clks[c]);
> +	return rc;
> +}
> +
> +static void ahci_disable_clks(struct ahci_host_priv *hpriv)
> +{
> +	int c;
> +
> +	for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
> +		if (hpriv->clks[c])

		if (!IS_ERR(hpriv->clks[c]))

> +			clk_disable_unprepare(hpriv->clks[c]);
> +}
> +
> +static void ahci_put_clks(struct ahci_host_priv *hpriv)
> +{
> +	int c;
> +
> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)

	for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++)

> +		clk_put(hpriv->clks[c]);
> +}

Better still for this one, consider using devm_clk_get() - in which case
the above is even more important to get right.  We really should have
a devm_of_clk_get() too.
Hans de Goede - Jan. 19, 2014, 7:20 p.m.
Hi,

On 01/19/2014 01:38 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:48AM +0100, Hans de Goede wrote:
>> +static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
>> +{
>> +	int c, rc;
>> +
>> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
>
> 	for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) {

That won't work, hpriv->clks == NULL for clks entries which are not used,
and before we get into a discussion about leaving any PTR_ERR returns
from clk_get in-place. I've had similar discussions when doing similar
changes to ohci-platform.c and ehci-platform.c and there the conclusion
was that "if (clk)" is just much more nice to read then "if (!IS_ERR(clk))",
I would like to avoid having the same discussion again.

More-over all clk_foo() methods check for and will safely handle clk == NULL,
and will crash and burn with clk == IS_ERR(clk).

I've chosen to still explicitly check for clk == NULL as that makes it much more
clear when reading the code that clk maybe NULL.


>> +		rc = clk_prepare_enable(hpriv->clks[c]);
>> +		if (rc) {
>> +			dev_err(dev, "clock prepare enable failed");
>> +			goto disable_unprepare_clk;
>> +		}
>> +	}
>> +	return 0;
>> +
>> +disable_unprepare_clk:
>> +	while (--c >= 0)
>> +		clk_disable_unprepare(hpriv->clks[c]);
>> +	return rc;
>> +}
>> +
>> +static void ahci_disable_clks(struct ahci_host_priv *hpriv)
>> +{
>> +	int c;
>> +
>> +	for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
>> +		if (hpriv->clks[c])
>
> 		if (!IS_ERR(hpriv->clks[c]))
>

Idem.

>> +			clk_disable_unprepare(hpriv->clks[c]);
>> +}
>> +
>> +static void ahci_put_clks(struct ahci_host_priv *hpriv)
>> +{
>> +	int c;
>> +
>> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
>
> 	for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++)
>

Idem.

>> +		clk_put(hpriv->clks[c]);
>> +}
>
> Better still for this one, consider using devm_clk_get() - in which case
> the above is even more important to get right.

The above depends on how errors are handled when calling clk_get (or variants),
which in the case of this patch is such that hpriv->clks[i] == NULL when not
present.

> We really should have a devm_of_clk_get() too.

Agreed, but that seems something for another patch-set, this one is big
enough as is.

Regards,

Hans
--
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/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 89de156..3ced07d 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -10,6 +10,7 @@  Required properties:
 
 Optional properties:
 - dma-coherent      : Present if dma operations are coherent
+- clocks            : a list of phandle + clock specifier pairs
 
 Example:
         sata@ffe08000 {
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 8f60f33..7950b3a 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -52,6 +52,7 @@ 
 enum {
 	AHCI_MAX_PORTS		= 32,
 	AHCI_MAX_SG		= 168, /* hardware max is 64K */
+	AHCI_MAX_CLKS		= 3,
 	AHCI_DMA_BOUNDARY	= 0xffffffff,
 	AHCI_MAX_CMDS		= 32,
 	AHCI_CMD_SZ		= 32,
@@ -321,7 +322,7 @@  struct ahci_host_priv {
 	u32 			em_loc; /* enclosure management location */
 	u32			em_buf_sz;	/* EM buffer size in byte */
 	u32			em_msg_type;	/* EM message type */
-	struct clk		*clk;		/* Only for platforms supporting clk */
+	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
 	void			*plat_data;	/* Other platform data */
 	/* Optional ahci_start_engine override */
 	void			(*start_engine)(struct ata_port *ap);
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 33ac7a4..75a3d47 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -87,6 +87,42 @@  static struct scsi_host_template ahci_platform_sht = {
 	AHCI_SHT("ahci_platform"),
 };
 
+static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
+{
+	int c, rc;
+
+	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
+		rc = clk_prepare_enable(hpriv->clks[c]);
+		if (rc) {
+			dev_err(dev, "clock prepare enable failed");
+			goto disable_unprepare_clk;
+		}
+	}
+	return 0;
+
+disable_unprepare_clk:
+	while (--c >= 0)
+		clk_disable_unprepare(hpriv->clks[c]);
+	return rc;
+}
+
+static void ahci_disable_clks(struct ahci_host_priv *hpriv)
+{
+	int c;
+
+	for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
+		if (hpriv->clks[c])
+			clk_disable_unprepare(hpriv->clks[c]);
+}
+
+static void ahci_put_clks(struct ahci_host_priv *hpriv)
+{
+	int c;
+
+	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
+		clk_put(hpriv->clks[c]);
+}
+
 static int ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -97,10 +133,8 @@  static int ahci_probe(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv;
 	struct ata_host *host;
 	struct resource *mem;
-	int irq;
-	int n_ports;
-	int i;
-	int rc;
+	struct clk *clk;
+	int i, irq, max_clk, n_ports, rc;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {
@@ -131,17 +165,26 @@  static int ahci_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	hpriv->clk = clk_get(dev, NULL);
-	if (IS_ERR(hpriv->clk)) {
-		dev_err(dev, "can't get clock\n");
-	} else {
-		rc = clk_prepare_enable(hpriv->clk);
-		if (rc) {
-			dev_err(dev, "clock prepare enable failed");
-			goto free_clk;
+	max_clk = dev->of_node ? AHCI_MAX_CLKS : 1;
+	for (i = 0; i < max_clk; i++) {
+		if (i == 0)
+			clk = clk_get(dev, NULL); /* For old platform init */
+		else
+			clk = of_clk_get(dev->of_node, i);
+
+		if (IS_ERR(clk)) {
+			rc = PTR_ERR(clk);
+			if (rc == -EPROBE_DEFER)
+				goto free_clk;
+			break;
 		}
+		hpriv->clks[i] = clk;
 	}
 
+	rc = ahci_enable_clks(dev, hpriv);
+	if (rc)
+		goto free_clk;
+
 	/*
 	 * Some platforms might need to prepare for mmio region access,
 	 * which could be done in the following init call. So, the mmio
@@ -222,11 +265,9 @@  pdata_exit:
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
 disable_unprepare_clk:
-	if (!IS_ERR(hpriv->clk))
-		clk_disable_unprepare(hpriv->clk);
+	ahci_disable_clks(hpriv);
 free_clk:
-	if (!IS_ERR(hpriv->clk))
-		clk_put(hpriv->clk);
+	ahci_put_clks(hpriv);
 	return rc;
 }
 
@@ -239,10 +280,8 @@  static void ahci_host_stop(struct ata_host *host)
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
 
-	if (!IS_ERR(hpriv->clk)) {
-		clk_disable_unprepare(hpriv->clk);
-		clk_put(hpriv->clk);
-	}
+	ahci_disable_clks(hpriv);
+	ahci_put_clks(hpriv);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -277,8 +316,7 @@  static int ahci_suspend(struct device *dev)
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
 
-	if (!IS_ERR(hpriv->clk))
-		clk_disable_unprepare(hpriv->clk);
+	ahci_disable_clks(hpriv);
 
 	return 0;
 }
@@ -290,13 +328,9 @@  static int ahci_resume(struct device *dev)
 	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
 
-	if (!IS_ERR(hpriv->clk)) {
-		rc = clk_prepare_enable(hpriv->clk);
-		if (rc) {
-			dev_err(dev, "clock prepare enable failed");
-			return rc;
-		}
-	}
+	rc = ahci_enable_clks(dev, hpriv);
+	if (rc)
+		return rc;
 
 	if (pdata && pdata->resume) {
 		rc = pdata->resume(dev);
@@ -320,8 +354,7 @@  pdata_suspend:
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
 disable_unprepare_clk:
-	if (!IS_ERR(hpriv->clk))
-		clk_disable_unprepare(hpriv->clk);
+	ahci_disable_clks(hpriv);
 
 	return rc;
 }