[PATCH v6 00/18] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver

[PATCH v6 00/18] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver

[Hans de Goede]

Hi all,

Here is v6 of my patchset for adding ahci-sunxi support. This has been
tested with Allwinner A10, Allwinner A20 and Freeware imx6x SoCs, including
suspend / resume. Note that since my last revision the ahci_imx driver has
also grown imx53 sata support, it would be good if some-one could test that
with this series.

History:

v1, by Olliver Schinagl:
This was using the approach of having a platform device which probe method
creates a new child platform device which gets driven by ahci_platform.c,
as done by ahci_imx.c .

v2, by Hans de Goede:
Stand-alone platform driver based on Olliver's work

v3, by Hans de Goede:
patch-series, with 4 different parts
a) Make ahci_platform.c more generic, handle more then 1 clk, target pwr
   regulator
b) New ahci-sunxi code only populating ahci_platform_data, passed to
   ahci_platform.c to of_device_id matching.
c) Refactor ahci-imx code to work the same as the new ahci-sunxi code, this
   is the reason why v3 is an RFC, I'm waiting for the wandboard I ordered to
   arrive so that I can actually test this.
d) dts bindings for the sunxi ahci parts

v4, by Hans de Goede:
patch-series, with 5 different parts:
a) Make ahci_platform.c more generic, handle more then 1 clk, target pwr
   regulator
b) Turn parts of ahci_platform.c into a library for use by other drivers
c) New ahci-sunxi driver using the ahci_platform.c library functionality
d) Refactor ahci-imx code to work the same as the new ahci-sunxi code
e) dts bindings for the sunxi ahci parts

v5:
v4 + the following changes:
1) fsl,imx6q driver is now tested
2) fixed suspend / resume on fsl,imx6q
3) Modifed devicetree node naming to match dt spec
4) Reworked the busy waiting code in the sunxi-phy handling as suggested by
   Russell King

v6:
v5 rebased on top of 3.14-rc3 + the following changes
1) Added Roger Quadros' generic phy support series
2) Added a "ARM: sun4i: dt: Remove grouping + simple-bus for regulators" dts
   patch

Tejun, can you please add patches 1-15 to your ata tree for 3.15 ?

Maxime, can you please add patch 16-18 to your dts tree for 3.15 ?

Thanks & Regards,

Hans 

[PATCH v6 01/18] libahci: Allow drivers to override start_engine

[Hans de Goede]

Allwinner A10 and A20 ARM SoCs have an AHCI sata controller which needs a
special register to be poked before starting the DMA engine.

This register gets reset on an ahci_stop_engine call, so there is no other
place then ahci_start_engine where this poking can be done.

This commit allows drivers to override ahci_start_engine behavior for use by
the Allwinner AHCI driver (and potentially other drivers in the future).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci.c          |  6 ++++--
 drivers/ata/ahci.h          |  3 ++-
 drivers/ata/libahci.c       | 27 ++++++++++++++++++---------
 drivers/ata/sata_highbank.c |  3 ++-
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index dc2756f..eda68b4 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -564,6 +564,7 @@ static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 				 unsigned long deadline)
 {
 	struct ata_port *ap = link->ap;
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	bool online;
 	int rc;
 
@@ -574,7 +575,7 @@ static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
 				 deadline, &online, NULL);
 
-	ahci_start_engine(ap);
+	hpriv->start_engine(ap);
 
 	DPRINTK("EXIT, rc=%d, class=%u\n", rc, *class);
 
@@ -589,6 +590,7 @@ static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
 {
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
 	struct ata_taskfile tf;
 	bool online;
@@ -604,7 +606,7 @@ static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
 	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
 				 deadline, &online, NULL);
 
-	ahci_start_engine(ap);
+	hpriv->start_engine(ap);
 
 	/* The pseudo configuration device on SIMG4726 attached to
 	 * ASUS P5W-DH Deluxe doesn't send signature FIS after
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efd..2c04211 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -323,6 +323,8 @@ struct ahci_host_priv {
 	u32			em_msg_type;	/* EM message type */
 	struct clk		*clk;		/* Only for platforms supporting clk */
 	void			*plat_data;	/* Other platform data */
+	/* Optional ahci_start_engine override */
+	void			(*start_engine)(struct ata_port *ap);
 };
 
 extern int ahci_ignore_sss;
@@ -357,7 +359,6 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 		      int (*check_ready)(struct ata_link *link));
 
 int ahci_stop_engine(struct ata_port *ap);
-void ahci_start_engine(struct ata_port *ap);
 int ahci_check_ready(struct ata_link *link);
 int ahci_kick_engine(struct ata_port *ap);
 int ahci_port_resume(struct ata_port *ap);
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 36605ab..c550b5c 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -69,6 +69,7 @@ static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state,
 
 static int ahci_scr_read(struct ata_link *link, unsigned int sc_reg, u32 *val);
 static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val);
+static void ahci_start_engine(struct ata_port *ap);
 static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc);
 static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc);
 static int ahci_port_start(struct ata_port *ap);
@@ -500,6 +501,9 @@ void ahci_save_initial_config(struct device *dev,
 	hpriv->cap = cap;
 	hpriv->cap2 = cap2;
 	hpriv->port_map = port_map;
+
+	if (!hpriv->start_engine)
+		hpriv->start_engine = ahci_start_engine;
 }
 EXPORT_SYMBOL_GPL(ahci_save_initial_config);
 
@@ -565,7 +569,7 @@ static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
 	return -EINVAL;
 }
 
-void ahci_start_engine(struct ata_port *ap)
+static void ahci_start_engine(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 tmp;
@@ -576,7 +580,6 @@ void ahci_start_engine(struct ata_port *ap)
 	writel(tmp, port_mmio + PORT_CMD);
 	readl(port_mmio + PORT_CMD); /* flush */
 }
-EXPORT_SYMBOL_GPL(ahci_start_engine);
 
 int ahci_stop_engine(struct ata_port *ap)
 {
@@ -766,7 +769,7 @@ static void ahci_start_port(struct ata_port *ap)
 
 	/* enable DMA */
 	if (!(hpriv->flags & AHCI_HFLAG_DELAY_ENGINE))
-		ahci_start_engine(ap);
+		hpriv->start_engine(ap);
 
 	/* turn on LEDs */
 	if (ap->flags & ATA_FLAG_EM) {
@@ -1234,7 +1237,7 @@ int ahci_kick_engine(struct ata_port *ap)
 
 	/* restart engine */
  out_restart:
-	ahci_start_engine(ap);
+	hpriv->start_engine(ap);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_kick_engine);
@@ -1426,6 +1429,7 @@ static int ahci_hardreset(struct ata_link *link, unsigned int *class,
 	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
 	struct ata_taskfile tf;
 	bool online;
@@ -1443,7 +1447,7 @@ static int ahci_hardreset(struct ata_link *link, unsigned int *class,
 	rc = sata_link_hardreset(link, timing, deadline, &online,
 				 ahci_check_ready);
 
-	ahci_start_engine(ap);
+	hpriv->start_engine(ap);
 
 	if (online)
 		*class = ahci_dev_classify(ap);
@@ -2007,10 +2011,12 @@ static void ahci_thaw(struct ata_port *ap)
 
 void ahci_error_handler(struct ata_port *ap)
 {
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+
 	if (!(ap->pflags & ATA_PFLAG_FROZEN)) {
 		/* restart engine */
 		ahci_stop_engine(ap);
-		ahci_start_engine(ap);
+		hpriv->start_engine(ap);
 	}
 
 	sata_pmp_error_handler(ap);
@@ -2031,6 +2037,7 @@ static void ahci_post_internal_cmd(struct ata_queued_cmd *qc)
 
 static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep)
 {
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	void __iomem *port_mmio = ahci_port_base(ap);
 	struct ata_device *dev = ap->link.device;
 	u32 devslp, dm, dito, mdat, deto;
@@ -2094,7 +2101,7 @@ static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep)
 		   PORT_DEVSLP_ADSE);
 	writel(devslp, port_mmio + PORT_DEVSLP);
 
-	ahci_start_engine(ap);
+	hpriv->start_engine(ap);
 
 	/* enable device sleep feature for the drive */
 	err_mask = ata_dev_set_feature(dev,
@@ -2106,6 +2113,7 @@ static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep)
 
 static void ahci_enable_fbs(struct ata_port *ap)
 {
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	struct ahci_port_priv *pp = ap->private_data;
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 fbs;
@@ -2134,11 +2142,12 @@ static void ahci_enable_fbs(struct ata_port *ap)
 	} else
 		dev_err(ap->host->dev, "Failed to enable FBS\n");
 
-	ahci_start_engine(ap);
+	hpriv->start_engine(ap);
 }
 
 static void ahci_disable_fbs(struct ata_port *ap)
 {
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	struct ahci_port_priv *pp = ap->private_data;
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 fbs;
@@ -2166,7 +2175,7 @@ static void ahci_disable_fbs(struct ata_port *ap)
 		pp->fbs_enabled = false;
 	}
 
-	ahci_start_engine(ap);
+	hpriv->start_engine(ap);
 }
 
 static void ahci_pmp_attach(struct ata_port *ap)
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index 870b11e..b3b18d1 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -403,6 +403,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 	static const unsigned long timing[] = { 5, 100, 500};
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
 	struct ata_taskfile tf;
 	bool online;
@@ -431,7 +432,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 			break;
 	} while (!online && retry--);
 
-	ahci_start_engine(ap);
+	hpriv->start_engine(ap);
 
 	if (online)
 		*class = ahci_dev_classify(ap);
-- 
1.8.5.3

[PATCH v6 01/18] libahci: Allow drivers to override start_engine

[Tejun Heo]

Hello,

On Wed, Feb 19, 2014 at 01:01:43PM +0100, Hans de Goede wrote:
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 2289efd..2c04211 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -323,6 +323,8 @@ struct ahci_host_priv {
>  	u32			em_msg_type;	/* EM message type */
>  	struct clk		*clk;		/* Only for platforms supporting clk */
>  	void			*plat_data;	/* Other platform data */
> +	/* Optional ahci_start_engine override */
> +	void			(*start_engine)(struct ata_port *ap);

Can you please add that this gets initialized to the default during
save_initial_config and can be overridden anytime before the host is
activated?

> @@ -500,6 +501,9 @@ void ahci_save_initial_config(struct device *dev,
>  	hpriv->cap = cap;
>  	hpriv->cap2 = cap2;
>  	hpriv->port_map = port_map;
> +
> +	if (!hpriv->start_engine)
> +		hpriv->start_engine = ahci_start_engine;

Please update the function comment accordingly.

> -void ahci_start_engine(struct ata_port *ap)
> +static void ahci_start_engine(struct ata_port *ap)
>  {
>  	void __iomem *port_mmio = ahci_port_base(ap);
>  	u32 tmp;
> @@ -576,7 +580,6 @@ void ahci_start_engine(struct ata_port *ap)
>  	writel(tmp, port_mmio + PORT_CMD);
>  	readl(port_mmio + PORT_CMD); /* flush */
>  }
> -EXPORT_SYMBOL_GPL(ahci_start_engine);

Why are we making ahci_start_engine() static?  Wouldn't there likely
be users who would do more things before or after ahci_start_engine()?
Also, why is there no explanation of this change in the patch
description?

Thanks.

-- 
tejun

[PATCH v6 02/18] libahci: Move ahci_host_priv declaration to include/linux/ahci.h

[Hans de Goede]

With the ahci-platform.c changes later in this patch-set, some
arch/arm/mach-foo/*.c sata drivers need access to ahci_host_priv, so move
its declaration outside of drivers/ata.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci.h   | 20 +-------------------
 include/linux/ahci.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 19 deletions(-)
 create mode 100644 include/linux/ahci.h

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2c04211..0f80129 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -35,7 +35,7 @@
 #ifndef _AHCI_H
 #define _AHCI_H
 
-#include <linux/clk.h>
+#include <linux/ahci.h>
 #include <linux/libata.h>
 
 /* Enclosure Management Control */
@@ -309,24 +309,6 @@ struct ahci_port_priv {
 	char			*irq_desc;	/* desc in /proc/interrupts */
 };
 
-struct ahci_host_priv {
-	void __iomem *		mmio;		/* bus-independent mem map */
-	unsigned int		flags;		/* AHCI_HFLAG_* */
-	u32			cap;		/* cap to use */
-	u32			cap2;		/* cap2 to use */
-	u32			port_map;	/* port map to use */
-	u32			saved_cap;	/* saved initial cap */
-	u32			saved_cap2;	/* saved initial cap2 */
-	u32			saved_port_map;	/* saved initial port_map */
-	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 */
-	void			*plat_data;	/* Other platform data */
-	/* Optional ahci_start_engine override */
-	void			(*start_engine)(struct ata_port *ap);
-};
-
 extern int ahci_ignore_sss;
 
 extern struct device_attribute *ahci_shost_attrs[];
diff --git a/include/linux/ahci.h b/include/linux/ahci.h
new file mode 100644
index 0000000..3499d44
--- /dev/null
+++ b/include/linux/ahci.h
@@ -0,0 +1,46 @@
+/*
+ *  ahci.h - Common AHCI SATA definitions and declarations
+ *
+ *  Maintained by:  Tejun Heo <tj@kernel.org>
+ *                  Please ALWAYS copy linux-ide at vger.kernel.org
+ *                  on emails.
+ *
+ *  Copyright 2004-2005 Red Hat, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2, or (at your option)
+ *  any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ */
+
+#ifndef __LINUX_AHCI_H__
+#define __LINUX_AHCI_H__
+
+#include <linux/clk.h>
+
+struct ata_port;
+
+struct ahci_host_priv {
+	void __iomem		*mmio;		/* bus-independent mem map */
+	unsigned int		flags;		/* AHCI_HFLAG_* */
+	u32			cap;		/* cap to use */
+	u32			cap2;		/* cap2 to use */
+	u32			port_map;	/* port map to use */
+	u32			saved_cap;	/* saved initial cap */
+	u32			saved_cap2;	/* saved initial cap2 */
+	u32			saved_port_map;	/* saved initial port_map */
+	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;		/* Optional */
+	void			*plat_data;	/* Other platform data */
+	/* Optional ahci_start_engine override */
+	void			(*start_engine)(struct ata_port *ap);
+};
+
+#endif
-- 
1.8.5.3

[PATCH v6 03/18] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method

[Hans de Goede]

Some ahci_platform_data->init methods need access to the ahci_host_priv data.

When calling ahci_platform_data->init the ata_host has not been allocated yet,
so access to ahci_host_priv through the dev argument is not possible.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/mach-davinci/devices-da8xx.c |  5 +++--
 arch/arm/mach-spear/spear1340.c       |  2 +-
 drivers/ata/ahci_imx.c                | 12 ++++++------
 drivers/ata/ahci_platform.c           |  2 +-
 include/linux/ahci_platform.h         |  3 ++-
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index 0486cdf..c4e97da 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -14,6 +14,7 @@
 #include <linux/platform_device.h>
 #include <linux/dma-contiguous.h>
 #include <linux/serial_8250.h>
+#include <linux/ahci.h>
 #include <linux/ahci_platform.h>
 #include <linux/clk.h>
 #include <linux/reboot.h>
@@ -1061,7 +1062,7 @@ static unsigned long da850_sata_xtal[] = {
 	KHZ_TO_HZ(60000),
 };
 
-static int da850_sata_init(struct device *dev, void __iomem *addr)
+static int da850_sata_init(struct device *dev, struct ahci_host_priv *hpriv)
 {
 	int i, ret;
 	unsigned int val;
@@ -1096,7 +1097,7 @@ static int da850_sata_init(struct device *dev, void __iomem *addr)
 		SATA_PHY_TXSWING(3) |
 		SATA_PHY_ENPLL(1);
 
-	__raw_writel(val, addr + SATA_P0PHYCR_REG);
+	__raw_writel(val, hpriv->mmio + SATA_P0PHYCR_REG);
 
 	return 0;
 
diff --git a/arch/arm/mach-spear/spear1340.c b/arch/arm/mach-spear/spear1340.c
index 3fb6834..9e2f3ac 100644
--- a/arch/arm/mach-spear/spear1340.c
+++ b/arch/arm/mach-spear/spear1340.c
@@ -77,7 +77,7 @@
 			SPEAR1340_MIPHY_PLL_RATIO_TOP(25))
 
 /* SATA device registration */
-static int sata_miphy_init(struct device *dev, void __iomem *addr)
+static int sata_miphy_init(struct device *dev, struct ahci_host_priv *hpriv)
 {
 	writel(SPEAR1340_SATA_CFG_VAL, SPEAR1340_PCIE_SATA_CFG);
 	writel(SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK,
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index dd4d6f7..ebfd1d6 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -168,7 +168,7 @@ static const struct ata_port_info ahci_imx_port_info = {
 	.port_ops	= &ahci_imx_ops,
 };
 
-static int imx_sata_init(struct device *dev, void __iomem *mmio)
+static int imx_sata_init(struct device *dev, struct ahci_host_priv *hpriv)
 {
 	int ret = 0;
 	unsigned int reg_val;
@@ -185,19 +185,19 @@ static int imx_sata_init(struct device *dev, void __iomem *mmio)
 	 * Implement the port0.
 	 * Get the ahb clock rate, and configure the TIMER1MS register.
 	 */
-	reg_val = readl(mmio + HOST_CAP);
+	reg_val = readl(hpriv->mmio + HOST_CAP);
 	if (!(reg_val & HOST_CAP_SSS)) {
 		reg_val |= HOST_CAP_SSS;
-		writel(reg_val, mmio + HOST_CAP);
+		writel(reg_val, hpriv->mmio + HOST_CAP);
 	}
-	reg_val = readl(mmio + HOST_PORTS_IMPL);
+	reg_val = readl(hpriv->mmio + HOST_PORTS_IMPL);
 	if (!(reg_val & 0x1)) {
 		reg_val |= 0x1;
-		writel(reg_val, mmio + HOST_PORTS_IMPL);
+		writel(reg_val, hpriv->mmio + HOST_PORTS_IMPL);
 	}
 
 	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
-	writel(reg_val, mmio + HOST_TIMER1MS);
+	writel(reg_val, hpriv->mmio + HOST_TIMER1MS);
 
 	return 0;
 }
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 4b231ba..434ab89 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -149,7 +149,7 @@ static int ahci_probe(struct platform_device *pdev)
 	 * returned successfully.
 	 */
 	if (pdata && pdata->init) {
-		rc = pdata->init(dev, hpriv->mmio);
+		rc = pdata->init(dev, hpriv);
 		if (rc)
 			goto disable_unprepare_clk;
 	}
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 73a2500..737fe38 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -19,9 +19,10 @@
 
 struct device;
 struct ata_port_info;
+struct ahci_host_priv;
 
 struct ahci_platform_data {
-	int (*init)(struct device *dev, void __iomem *addr);
+	int (*init)(struct device *dev, struct ahci_host_priv *hpriv);
 	void (*exit)(struct device *dev);
 	int (*suspend)(struct device *dev);
 	int (*resume)(struct device *dev);
-- 
1.8.5.3

[PATCH v6 04/18] ahci-platform: Add support for devices with more then 1 clock

[Hans de Goede]

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_platform.c                        | 97 +++++++++++++++-------
 include/linux/ahci.h                               |  4 +-
 include/linux/ahci_platform.h                      |  3 +
 4 files changed, 73 insertions(+), 32 deletions(-)

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 at ffe08000 {
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 434ab89..aaa0c08 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -87,6 +87,44 @@ static struct scsi_host_template ahci_platform_sht = {
 	AHCI_SHT("ahci_platform"),
 };
 
+
+int ahci_platform_enable_clks(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)
+			goto disable_unprepare_clk;
+	}
+	return 0;
+
+disable_unprepare_clk:
+	while (--c >= 0)
+		clk_disable_unprepare(hpriv->clks[c]);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);
+
+void ahci_platform_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]);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
+
+
+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 +135,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 +167,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 +267,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 +282,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 +318,7 @@ static int ahci_suspend(struct device *dev)
 	if (pdata && pdata->suspend)
 		return pdata->suspend(dev);
 
-	if (!IS_ERR(hpriv->clk))
-		clk_disable_unprepare(hpriv->clk);
+	ahci_disable_clks(hpriv);
 
 	return 0;
 }
@@ -290,13 +330,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);
@@ -317,8 +353,7 @@ static int ahci_resume(struct device *dev)
 	return 0;
 
 disable_unprepare_clk:
-	if (!IS_ERR(hpriv->clk))
-		clk_disable_unprepare(hpriv->clk);
+	ahci_disable_clks(hpriv);
 
 	return rc;
 }
diff --git a/include/linux/ahci.h b/include/linux/ahci.h
index 3499d44..19970b0 100644
--- a/include/linux/ahci.h
+++ b/include/linux/ahci.h
@@ -23,6 +23,8 @@
 
 #include <linux/clk.h>
 
+#define AHCI_MAX_CLKS		3
+
 struct ata_port;
 
 struct ahci_host_priv {
@@ -37,7 +39,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;		/* Optional */
+	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/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 737fe38..0071d0b 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -31,4 +31,7 @@ struct ahci_platform_data {
 	unsigned int mask_port_map;
 };
 
+int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
+void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
+
 #endif /* _AHCI_PLATFORM_H */
-- 
1.8.5.3

[PATCH v6 04/18] ahci-platform: Add support for devices with more then 1 clock

[Tejun Heo]

On Wed, Feb 19, 2014 at 01:01:46PM +0100, Hans de Goede wrote:
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 434ab89..aaa0c08 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -87,6 +87,44 @@ static struct scsi_host_template ahci_platform_sht = {
>  	AHCI_SHT("ahci_platform"),
>  };
>  
> +
> +int ahci_platform_enable_clks(struct ahci_host_priv *hpriv)

Throughout the series, there are mixtures of one and two blank lines
in front of functions, let's just do one consistently.  Also, can you
please add proper function comments on top of the exported functions?

> +{
> +	int c, rc;
> +
> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
> +		rc = clk_prepare_enable(hpriv->clks[c]);
> +		if (rc)
> +			goto disable_unprepare_clk;
> +	}
> +	return 0;
> +
> +disable_unprepare_clk:
> +	while (--c >= 0)
> +		clk_disable_unprepare(hpriv->clks[c]);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);
> +
> +void ahci_platform_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]);
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
> +
> +
> +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]);
> +}

Urgh, clk can't do devm?  ahci_platform itself should be able to build
devmized interface using devres_alloc() or devm_add_action().
Eh.... maybe later.

> @@ -131,17 +167,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;
>  	}

Wouldn't it be nice to have some eplanation on why clk init path
diverges depending on whether dev->of_node is NULL or not?  In
general, the patchset seems dearth on comment side.

Thanks.

-- 
tejun

[PATCH v6 04/18] ahci-platform: Add support for devices with more then 1 clock

[Ben Dooks]

On 19/02/14 15:52, Tejun Heo wrote:
> On Wed, Feb 19, 2014 at 01:01:46PM +0100, Hans de Goede wrote:
>> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
>> index 434ab89..aaa0c08 100644
>> --- a/drivers/ata/ahci_platform.c
>> +++ b/drivers/ata/ahci_platform.c
>> @@ -87,6 +87,44 @@ static struct scsi_host_template ahci_platform_sht = {
>>   	AHCI_SHT("ahci_platform"),
>>   };
>>
>> +
>> +int ahci_platform_enable_clks(struct ahci_host_priv *hpriv)
>
> Throughout the series, there are mixtures of one and two blank lines
> in front of functions, let's just do one consistently.  Also, can you
> please add proper function comments on top of the exported functions?
>
>> +{
>> +	int c, rc;
>> +
>> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
>> +		rc = clk_prepare_enable(hpriv->clks[c]);
>> +		if (rc)
>> +			goto disable_unprepare_clk;
>> +	}
>> +	return 0;
>> +
>> +disable_unprepare_clk:
>> +	while (--c >= 0)
>> +		clk_disable_unprepare(hpriv->clks[c]);
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);
>> +
>> +void ahci_platform_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]);
>> +}
>> +EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
>> +
>> +
>> +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]);
>> +}
>
> Urgh, clk can't do devm?  ahci_platform itself should be able to build
> devmized interface using devres_alloc() or devm_add_action().
> Eh.... maybe later.

There is devm_clk_get()


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH v6 05/18] ahci-platform: Add support for an optional regulator for sata-target power

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |  1 +
 drivers/ata/ahci_platform.c                        | 35 ++++++++++++++++++++--
 include/linux/ahci.h                               |  2 ++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 3ced07d..1ac807f 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -11,6 +11,7 @@ Required properties:
 Optional properties:
 - dma-coherent      : Present if dma operations are coherent
 - clocks            : a list of phandle + clock specifier pairs
+- target-supply     : regulator for SATA target power
 
 Example:
         sata at ffe08000 {
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index aaa0c08..2f319e9 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -167,6 +167,13 @@ static int ahci_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
+	if (IS_ERR(hpriv->target_pwr)) {
+		if (PTR_ERR(hpriv->target_pwr) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		hpriv->target_pwr = NULL;
+	}
+
 	max_clk = dev->of_node ? AHCI_MAX_CLKS : 1;
 	for (i = 0; i < max_clk; i++) {
 		if (i == 0)
@@ -183,9 +190,15 @@ static int ahci_probe(struct platform_device *pdev)
 		hpriv->clks[i] = clk;
 	}
 
+	if (hpriv->target_pwr) {
+		rc = regulator_enable(hpriv->target_pwr);
+		if (rc)
+			goto free_clk;
+	}
+
 	rc = ahci_enable_clks(dev, hpriv);
 	if (rc)
-		goto free_clk;
+		goto disable_regulator;
 
 	/*
 	 * Some platforms might need to prepare for mmio region access,
@@ -268,6 +281,9 @@ pdata_exit:
 		pdata->exit(dev);
 disable_unprepare_clk:
 	ahci_disable_clks(hpriv);
+disable_regulator:
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
 free_clk:
 	ahci_put_clks(hpriv);
 	return rc;
@@ -284,6 +300,9 @@ static void ahci_host_stop(struct ata_host *host)
 
 	ahci_disable_clks(hpriv);
 	ahci_put_clks(hpriv);
+
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -320,6 +339,9 @@ static int ahci_suspend(struct device *dev)
 
 	ahci_disable_clks(hpriv);
 
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
+
 	return 0;
 }
 
@@ -330,9 +352,15 @@ static int ahci_resume(struct device *dev)
 	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
 
+	if (hpriv->target_pwr) {
+		rc = regulator_enable(hpriv->target_pwr);
+		if (rc)
+			return rc;
+	}
+
 	rc = ahci_enable_clks(dev, hpriv);
 	if (rc)
-		return rc;
+		goto disable_regulator;
 
 	if (pdata && pdata->resume) {
 		rc = pdata->resume(dev);
@@ -354,6 +382,9 @@ static int ahci_resume(struct device *dev)
 
 disable_unprepare_clk:
 	ahci_disable_clks(hpriv);
+disable_regulator:
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
 
 	return rc;
 }
diff --git a/include/linux/ahci.h b/include/linux/ahci.h
index 19970b0..ac69cdc 100644
--- a/include/linux/ahci.h
+++ b/include/linux/ahci.h
@@ -22,6 +22,7 @@
 #define __LINUX_AHCI_H__
 
 #include <linux/clk.h>
+#include <linux/regulator/consumer.h>
 
 #define AHCI_MAX_CLKS		3
 
@@ -40,6 +41,7 @@ struct ahci_host_priv {
 	u32			em_buf_sz;	/* EM buffer size in byte */
 	u32			em_msg_type;	/* EM message type */
 	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
+	struct regulator	*target_pwr;	/* Optional */
 	void			*plat_data;	/* Other platform data */
 	/* Optional ahci_start_engine override */
 	void			(*start_engine)(struct ata_port *ap);
-- 
1.8.5.3

[PATCH v6 05/18] ahci-platform: Add support for an optional regulator for sata-target power

[Tejun Heo]

On Wed, Feb 19, 2014 at 01:01:47PM +0100, Hans de Goede wrote:
> @@ -268,6 +281,9 @@ pdata_exit:
>  		pdata->exit(dev);
>  disable_unprepare_clk:
>  	ahci_disable_clks(hpriv);
> +disable_regulator:
> +	if (hpriv->target_pwr)
> +		regulator_disable(hpriv->target_pwr);

The same thing with clks, I'd much prefer to see ahci_platform doing
devres wrapping so that its consumers wouldn't have to worry about
tearing stuff down.

Thanks.

-- 
tejun

[PATCH v6 06/18] ahci-platform: Add enable_ / disable_resources helper functions

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_platform.c   | 83 ++++++++++++++++++++++++-------------------
 include/linux/ahci_platform.h |  2 ++
 2 files changed, 48 insertions(+), 37 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 2f319e9..1cce7a2 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -117,6 +117,39 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
 EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
 
 
+int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
+{
+	int rc;
+
+	if (hpriv->target_pwr) {
+		rc = regulator_enable(hpriv->target_pwr);
+		if (rc)
+			return rc;
+	}
+
+	rc = ahci_platform_enable_clks(hpriv);
+	if (rc)
+		goto disable_regulator;
+
+	return 0;
+
+disable_regulator:
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
+
+void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
+{
+	ahci_platform_disable_clks(hpriv);
+
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
+
+
 static void ahci_put_clks(struct ahci_host_priv *hpriv)
 {
 	int c;
@@ -190,15 +223,9 @@ static int ahci_probe(struct platform_device *pdev)
 		hpriv->clks[i] = clk;
 	}
 
-	if (hpriv->target_pwr) {
-		rc = regulator_enable(hpriv->target_pwr);
-		if (rc)
-			goto free_clk;
-	}
-
-	rc = ahci_enable_clks(dev, hpriv);
+	rc = ahci_platform_enable_resources(hpriv);
 	if (rc)
-		goto disable_regulator;
+		goto free_clk;
 
 	/*
 	 * Some platforms might need to prepare for mmio region access,
@@ -209,7 +236,7 @@ static int ahci_probe(struct platform_device *pdev)
 	if (pdata && pdata->init) {
 		rc = pdata->init(dev, hpriv);
 		if (rc)
-			goto disable_unprepare_clk;
+			goto disable_resources;
 	}
 
 	ahci_save_initial_config(dev, hpriv,
@@ -279,11 +306,8 @@ static int ahci_probe(struct platform_device *pdev)
 pdata_exit:
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
-disable_unprepare_clk:
-	ahci_disable_clks(hpriv);
-disable_regulator:
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
 free_clk:
 	ahci_put_clks(hpriv);
 	return rc;
@@ -298,11 +322,8 @@ static void ahci_host_stop(struct ata_host *host)
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
 
-	ahci_disable_clks(hpriv);
+	ahci_platform_disable_resources(hpriv);
 	ahci_put_clks(hpriv);
-
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -337,10 +358,7 @@ static int ahci_suspend(struct device *dev)
 	if (pdata && pdata->suspend)
 		return pdata->suspend(dev);
 
-	ahci_disable_clks(hpriv);
-
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_resources(hpriv);
 
 	return 0;
 }
@@ -352,26 +370,20 @@ static int ahci_resume(struct device *dev)
 	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
 
-	if (hpriv->target_pwr) {
-		rc = regulator_enable(hpriv->target_pwr);
-		if (rc)
-			return rc;
-	}
-
-	rc = ahci_enable_clks(dev, hpriv);
+	rc = ahci_platform_enable_resources(hpriv);
 	if (rc)
-		goto disable_regulator;
+		return rc;
 
 	if (pdata && pdata->resume) {
 		rc = pdata->resume(dev);
 		if (rc)
-			goto disable_unprepare_clk;
+			goto disable_resources;
 	}
 
 	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
 		rc = ahci_reset_controller(host);
 		if (rc)
-			goto disable_unprepare_clk;
+			goto disable_resources;
 
 		ahci_init_controller(host);
 	}
@@ -380,11 +392,8 @@ static int ahci_resume(struct device *dev)
 
 	return 0;
 
-disable_unprepare_clk:
-	ahci_disable_clks(hpriv);
-disable_regulator:
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
 
 	return rc;
 }
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 0071d0b..5e5f85e 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -33,5 +33,7 @@ struct ahci_platform_data {
 
 int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
+int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
+void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
 
 #endif /* _AHCI_PLATFORM_H */
-- 
1.8.5.3

[PATCH v6 06/18] ahci-platform: Add enable_ / disable_resources helper functions

[Tejun Heo]

On Wed, Feb 19, 2014 at 01:01:48PM +0100, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/ata/ahci_platform.c   | 83 ++++++++++++++++++++++++-------------------
>  include/linux/ahci_platform.h |  2 ++
>  2 files changed, 48 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 2f319e9..1cce7a2 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -117,6 +117,39 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
>  EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
>  
>  
> +int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
> +{
> +	int rc;
> +
> +	if (hpriv->target_pwr) {
> +		rc = regulator_enable(hpriv->target_pwr);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	rc = ahci_platform_enable_clks(hpriv);
> +	if (rc)
> +		goto disable_regulator;
> +
> +	return 0;
> +
> +disable_regulator:
> +	if (hpriv->target_pwr)
> +		regulator_disable(hpriv->target_pwr);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
> +
> +void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
> +{
> +	ahci_platform_disable_clks(hpriv);
> +
> +	if (hpriv->target_pwr)
> +		regulator_disable(hpriv->target_pwr);
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
> +
> +

Ditto with comments, double blank lines and devres wrapping.

Thanks.

-- 
tejun

[PATCH v6 07/18] ahci-platform: "Library-ise" ahci_probe functionality

[Hans de Goede]

ahci_probe consists of 3 steps:
1) Get resources (get mmio, clks, regulator)
2) Enable resources, handled by ahci_platform_enable_resouces
3) The more or less standard ahci-host controller init sequence

This commit refactors step 1 and 3 into separate functions, so the platform
drivers for AHCI implementations which need a specific order in step 2,
and / or need to do some custom register poking at some time, can re-use
ahci-platform.c code without needing to copy and paste it.

Note that ahci_platform_init_host's prototype takes the 3 non function
members of ahci_platform_data as arguments, the idea is that drivers using
the new exported utility functions will not use ahci_platform_data at all,
and hopefully in the future ahci_platform_data can go away entirely.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_platform.c   | 158 ++++++++++++++++++++++++------------------
 include/linux/ahci_platform.h |  14 ++++
 2 files changed, 106 insertions(+), 66 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 1cce7a2..b260ebe 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -150,60 +150,31 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
 EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
 
 
-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 ahci_host_priv *ahci_platform_get_resources(
+	struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
-	const struct platform_device_id *id = platform_get_device_id(pdev);
-	struct ata_port_info pi = ahci_port_info[id ? id->driver_data : 0];
-	const struct ata_port_info *ppi[] = { &pi, NULL };
 	struct ahci_host_priv *hpriv;
-	struct ata_host *host;
-	struct resource *mem;
 	struct clk *clk;
-	int i, irq, max_clk, n_ports, rc;
-
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!mem) {
-		dev_err(dev, "no mmio space\n");
-		return -EINVAL;
-	}
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		dev_err(dev, "no irq\n");
-		return -EINVAL;
-	}
-
-	if (pdata && pdata->ata_port_info)
-		pi = *pdata->ata_port_info;
+	int i, max_clk, rc;
 
 	hpriv = devm_kzalloc(dev, sizeof(*hpriv), GFP_KERNEL);
 	if (!hpriv) {
 		dev_err(dev, "can't alloc ahci_host_priv\n");
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 	}
 
-	hpriv->flags |= (unsigned long)pi.private_data;
-
-	hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem));
+	hpriv->mmio = devm_ioremap_resource(dev,
+			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
 	if (!hpriv->mmio) {
-		dev_err(dev, "can't map %pR\n", mem);
-		return -ENOMEM;
+		dev_err(dev, "no mmio space\n");
+		return ERR_PTR(-EINVAL);
 	}
 
 	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
 	if (IS_ERR(hpriv->target_pwr)) {
 		if (PTR_ERR(hpriv->target_pwr) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+			return ERR_PTR(-EPROBE_DEFER);
 		hpriv->target_pwr = NULL;
 	}
 
@@ -223,27 +194,48 @@ static int ahci_probe(struct platform_device *pdev)
 		hpriv->clks[i] = clk;
 	}
 
-	rc = ahci_platform_enable_resources(hpriv);
-	if (rc)
-		goto free_clk;
+	return hpriv;
 
-	/*
-	 * Some platforms might need to prepare for mmio region access,
-	 * which could be done in the following init call. So, the mmio
-	 * region shouldn't be accessed before init (if provided) has
-	 * returned successfully.
-	 */
-	if (pdata && pdata->init) {
-		rc = pdata->init(dev, hpriv);
-		if (rc)
-			goto disable_resources;
-	}
+free_clk:
+	while (--i >= 0)
+		clk_put(hpriv->clks[i]);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_get_resources);
+
+void ahci_platform_put_resources(struct ahci_host_priv *hpriv)
+{
+	int c;
+
+	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
+		clk_put(hpriv->clks[c]);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_put_resources);
+
+
+int ahci_platform_init_host(struct platform_device *pdev,
+			    struct ahci_host_priv *hpriv,
+			    const struct ata_port_info *pi_template,
+			    unsigned int force_port_map,
+			    unsigned int mask_port_map)
+{
+	struct device *dev = &pdev->dev;
+	struct ata_port_info pi = *pi_template;
+	const struct ata_port_info *ppi[] = { &pi, NULL };
+	struct ata_host *host;
+	int i, irq, n_ports, rc;
 
-	ahci_save_initial_config(dev, hpriv,
-		pdata ? pdata->force_port_map : 0,
-		pdata ? pdata->mask_port_map  : 0);
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		dev_err(dev, "no irq\n");
+		return -EINVAL;
+	}
 
 	/* prepare host */
+	hpriv->flags |= (unsigned long)pi.private_data;
+
+	ahci_save_initial_config(dev, hpriv, force_port_map, mask_port_map);
+
 	if (hpriv->cap & HOST_CAP_NCQ)
 		pi.flags |= ATA_FLAG_NCQ;
 
@@ -260,10 +252,8 @@ static int ahci_probe(struct platform_device *pdev)
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
 	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
-	if (!host) {
-		rc = -ENOMEM;
-		goto pdata_exit;
-	}
+	if (!host)
+		return -ENOMEM;
 
 	host->private_data = hpriv;
 
@@ -278,7 +268,8 @@ static int ahci_probe(struct platform_device *pdev)
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-		ata_port_desc(ap, "mmio %pR", mem);
+		ata_port_desc(ap, "mmio %pR",
+			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
 		ata_port_desc(ap, "port 0x%x", 0x100 + ap->port_no * 0x80);
 
 		/* set enclosure management message type */
@@ -292,13 +283,48 @@ static int ahci_probe(struct platform_device *pdev)
 
 	rc = ahci_reset_controller(host);
 	if (rc)
-		goto pdata_exit;
+		return rc;
 
 	ahci_init_controller(host);
 	ahci_print_info(host, "platform");
 
-	rc = ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
-			       &ahci_platform_sht);
+	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
+				 &ahci_platform_sht);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_init_host);
+
+static int ahci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct platform_device_id *id = platform_get_device_id(pdev);
+	struct ahci_host_priv *hpriv;
+	int rc;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	rc = ahci_platform_enable_resources(hpriv);
+	if (rc)
+		goto put_resources;
+
+	/*
+	 * Some platforms might need to prepare for mmio region access,
+	 * which could be done in the following init call. So, the mmio
+	 * region shouldn't be accessed before init (if provided) has
+	 * returned successfully.
+	 */
+	if (pdata && pdata->init) {
+		rc = pdata->init(dev, hpriv);
+		if (rc)
+			goto disable_resources;
+	}
+
+	rc = ahci_platform_init_host(pdev, hpriv,
+				     &ahci_port_info[id ? id->driver_data : 0],
+				     pdata ? pdata->force_port_map : 0,
+				     pdata ? pdata->mask_port_map  : 0);
 	if (rc)
 		goto pdata_exit;
 
@@ -308,8 +334,8 @@ pdata_exit:
 		pdata->exit(dev);
 disable_resources:
 	ahci_platform_disable_resources(hpriv);
-free_clk:
-	ahci_put_clks(hpriv);
+put_resources:
+	ahci_platform_put_resources(hpriv);
 	return rc;
 }
 
@@ -323,7 +349,7 @@ static void ahci_host_stop(struct ata_host *host)
 		pdata->exit(dev);
 
 	ahci_platform_disable_resources(hpriv);
-	ahci_put_clks(hpriv);
+	ahci_platform_put_resources(hpriv);
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 5e5f85e..1dc7602 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -20,7 +20,13 @@
 struct device;
 struct ata_port_info;
 struct ahci_host_priv;
+struct platform_device;
 
+/*
+ * Note ahci_platform_data is deprecated. New drivers which need to override
+ * any of these, should instead declare there own platform_driver struct, and
+ * use ahci_platform* functions in their own probe, suspend and resume methods.
+ */
 struct ahci_platform_data {
 	int (*init)(struct device *dev, struct ahci_host_priv *hpriv);
 	void (*exit)(struct device *dev);
@@ -35,5 +41,13 @@ int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
 int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
+struct ahci_host_priv *ahci_platform_get_resources(
+	struct platform_device *pdev);
+void ahci_platform_put_resources(struct ahci_host_priv *hpriv);
+int ahci_platform_init_host(struct platform_device *pdev,
+			    struct ahci_host_priv *hpriv,
+			    const struct ata_port_info *pi_template,
+			    unsigned int force_port_map,
+			    unsigned int mask_port_map);
 
 #endif /* _AHCI_PLATFORM_H */
-- 
1.8.5.3

[PATCH v6 07/18] ahci-platform: "Library-ise" ahci_probe functionality

[Tejun Heo]

On Wed, Feb 19, 2014 at 01:01:49PM +0100, Hans de Goede wrote:
> ahci_probe consists of 3 steps:
> 1) Get resources (get mmio, clks, regulator)
> 2) Enable resources, handled by ahci_platform_enable_resouces
> 3) The more or less standard ahci-host controller init sequence
> 
> This commit refactors step 1 and 3 into separate functions, so the platform
> drivers for AHCI implementations which need a specific order in step 2,
> and / or need to do some custom register poking at some time, can re-use
> ahci-platform.c code without needing to copy and paste it.
> 
> Note that ahci_platform_init_host's prototype takes the 3 non function
> members of ahci_platform_data as arguments, the idea is that drivers using
> the new exported utility functions will not use ahci_platform_data at all,
> and hopefully in the future ahci_platform_data can go away entirely.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Heh, the lack of comments is getting kinda impressive. :)

Thanks.

-- 
tejun

[PATCH v6 08/18] ahci-platform: "Library-ise" suspend / resume functionality

[Hans de Goede]

Split suspend / resume code into host suspend / resume functionality and
resource enable / disabling phases, and export the new suspend_ / resume_host
functions.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_platform.c   | 55 +++++++++++++++++++++++++++++++------------
 include/linux/ahci_platform.h |  5 ++++
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index b260ebe..ba93930 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -353,14 +353,12 @@ static void ahci_host_stop(struct ata_host *host)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int ahci_suspend(struct device *dev)
+int ahci_platform_suspend_host(struct device *dev)
 {
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
 	u32 ctl;
-	int rc;
 
 	if (hpriv->flags & AHCI_HFLAG_NO_SUSPEND) {
 		dev_err(dev, "firmware update required for suspend/resume\n");
@@ -377,7 +375,37 @@ static int ahci_suspend(struct device *dev)
 	writel(ctl, mmio + HOST_CTL);
 	readl(mmio + HOST_CTL); /* flush */
 
-	rc = ata_host_suspend(host, PMSG_SUSPEND);
+	return ata_host_suspend(host, PMSG_SUSPEND);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);
+
+int ahci_platform_resume_host(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	int rc;
+
+	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
+		rc = ahci_reset_controller(host);
+		if (rc)
+			return rc;
+
+		ahci_init_controller(host);
+	}
+
+	ata_host_resume(host);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ahci_platform_resume_host);
+
+int ahci_platform_suspend(struct device *dev)
+{
+	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	int rc;
+
+	rc = ahci_platform_suspend_host(dev);
 	if (rc)
 		return rc;
 
@@ -388,8 +416,9 @@ static int ahci_suspend(struct device *dev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ahci_platform_suspend);
 
-static int ahci_resume(struct device *dev)
+int ahci_platform_resume(struct device *dev)
 {
 	struct ahci_platform_data *pdata = dev_get_platdata(dev);
 	struct ata_host *host = dev_get_drvdata(dev);
@@ -406,15 +435,9 @@ static int ahci_resume(struct device *dev)
 			goto disable_resources;
 	}
 
-	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
-		rc = ahci_reset_controller(host);
-		if (rc)
-			goto disable_resources;
-
-		ahci_init_controller(host);
-	}
-
-	ata_host_resume(host);
+	rc = ahci_platform_resume_host(dev);
+	if (rc)
+		goto disable_resources;
 
 	return 0;
 
@@ -423,9 +446,11 @@ disable_resources:
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(ahci_platform_resume);
 #endif
 
-static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
+static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend,
+			 ahci_platform_resume);
 
 static const struct of_device_id ahci_of_match[] = {
 	{ .compatible = "snps,spear-ahci", },
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 1dc7602..7c683c7 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -50,4 +50,9 @@ int ahci_platform_init_host(struct platform_device *pdev,
 			    unsigned int force_port_map,
 			    unsigned int mask_port_map);
 
+int ahci_platform_suspend_host(struct device *dev);
+int ahci_platform_resume_host(struct device *dev);
+int ahci_platform_suspend(struct device *dev);
+int ahci_platform_resume(struct device *dev);
+
 #endif /* _AHCI_PLATFORM_H */
-- 
1.8.5.3

[PATCH v6 09/18] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform

[Hans de Goede]

From: Olliver Schinagl <oliver@schinagl.nl>

This patch adds support for the ahci sata controler found on Allwinner A10
and A20 SoCs to the ahci_platform driver.

Orignally written by Olliver Schinagl using the approach of having a platform
device which probe method creates a new child platform device which gets
driven by ahci_platform.c, as done by ahci_imx.c .

Refactored by Hans de Goede to add most of the non sunxi specific functionality
to ahci_platform.c and use a platform_data pointer from of_device_id for the
sunxi specific bits.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |  15 +-
 drivers/ata/Kconfig                                |   9 +
 drivers/ata/Makefile                               |   1 +
 drivers/ata/ahci_sunxi.c                           | 251 +++++++++++++++++++++
 4 files changed, 273 insertions(+), 3 deletions(-)
 create mode 100644 drivers/ata/ahci_sunxi.c

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 1ac807f..499bfed 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -4,7 +4,9 @@ SATA nodes are defined to describe on-chip Serial ATA controllers.
 Each SATA controller should have its own node.
 
 Required properties:
-- compatible        : compatible list, contains "snps,spear-ahci"
+- compatible        : compatible list, one of "snps,spear-ahci",
+                      "snps,exynos5440-ahci", "ibm,476gtr-ahci", or
+                      "allwinner,sun4i-a10-ahci"
 - interrupts        : <interrupt mapping for SATA IRQ>
 - reg               : <registers mapping>
 
@@ -13,10 +15,17 @@ Optional properties:
 - clocks            : a list of phandle + clock specifier pairs
 - target-supply     : regulator for SATA target power
 
-Example:
+Examples:
         sata at ffe08000 {
 		compatible = "snps,spear-ahci";
 		reg = <0xffe08000 0x1000>;
 		interrupts = <115>;
-
         };
+
+	ahci: sata at 01c18000 {
+		compatible = "allwinner,sun4i-a10-ahci";
+		reg = <0x01c18000 0x1000>;
+		interrupts = <56>;
+		clocks = <&pll6 0>, <&ahb_gates 25>;
+		target-supply = <&reg_ahci_5v>;
+	};
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 4e73772..cc67cc0 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -106,6 +106,15 @@ config AHCI_IMX
 
 	  If unsure, say N.
 
+config AHCI_SUNXI
+	tristate "Allwinner sunxi AHCI SATA support"
+	depends on ARCH_SUNXI && SATA_AHCI_PLATFORM
+	help
+	  This option enables support for the Allwinner sunxi SoC's
+	  onboard AHCI SATA.
+
+	  If unsure, say N.
+
 config SATA_FSL
 	tristate "Freescale 3.0Gbps SATA support"
 	depends on FSL_SOC
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 46518c6..246050b 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
 obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
 obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
 obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o
+obj-$(CONFIG_AHCI_SUNXI)	+= ahci_sunxi.o
 
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
diff --git a/drivers/ata/ahci_sunxi.c b/drivers/ata/ahci_sunxi.c
new file mode 100644
index 0000000..568a211
--- /dev/null
+++ b/drivers/ata/ahci_sunxi.c
@@ -0,0 +1,251 @@
+/*
+ * Allwinner sunxi AHCI SATA platform driver
+ * Copyright 2013 Olliver Schinagl <oliver@schinagl.nl>
+ * Copyright 2014 Hans de Goede <hdegoede@redhat.com>
+ *
+ * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
+ * Based on code from Allwinner Technology Co., Ltd. <www.allwinnertech.com>,
+ * Daniel Wang <danielwang@allwinnertech.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/ahci_platform.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include "ahci.h"
+
+#define AHCI_BISTAFR 0x00a0
+#define AHCI_BISTCR 0x00a4
+#define AHCI_BISTFCTR 0x00a8
+#define AHCI_BISTSR 0x00ac
+#define AHCI_BISTDECR 0x00b0
+#define AHCI_DIAGNR0 0x00b4
+#define AHCI_DIAGNR1 0x00b8
+#define AHCI_OOBR 0x00bc
+#define AHCI_PHYCS0R 0x00c0
+#define AHCI_PHYCS1R 0x00c4
+#define AHCI_PHYCS2R 0x00c8
+#define AHCI_TIMER1MS 0x00e0
+#define AHCI_GPARAM1R 0x00e8
+#define AHCI_GPARAM2R 0x00ec
+#define AHCI_PPARAMR 0x00f0
+#define AHCI_TESTR 0x00f4
+#define AHCI_VERSIONR 0x00f8
+#define AHCI_IDR 0x00fc
+#define AHCI_RWCR 0x00fc
+#define AHCI_P0DMACR 0x0170
+#define AHCI_P0PHYCR 0x0178
+#define AHCI_P0PHYSR 0x017c
+
+static void sunxi_clrbits(void __iomem *reg, u32 clr_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val &= ~(clr_val);
+	writel(reg_val, reg);
+}
+
+static void sunxi_setbits(void __iomem *reg, u32 set_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val |= set_val;
+	writel(reg_val, reg);
+}
+
+static void sunxi_clrsetbits(void __iomem *reg, u32 clr_val, u32 set_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val &= ~(clr_val);
+	reg_val |= set_val;
+	writel(reg_val, reg);
+}
+
+static u32 sunxi_getbits(void __iomem *reg, u8 mask, u8 shift)
+{
+	return (readl(reg) >> shift) & mask;
+}
+
+static int ahci_sunxi_phy_init(struct device *dev, void __iomem *reg_base)
+{
+	u32 reg_val;
+	int timeout;
+
+	/* This magic is from the original code */
+	writel(0, reg_base + AHCI_RWCR);
+	mdelay(5);
+
+	sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(19));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS0R,
+			 (0x7 << 24),
+			 (0x5 << 24) | BIT(23) | BIT(18));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS1R,
+			 (0x3 << 16) | (0x1f << 8) | (0x3 << 6),
+			 (0x2 << 16) | (0x6 << 8) | (0x2 << 6));
+	sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(28) | BIT(15));
+	sunxi_clrbits(reg_base + AHCI_PHYCS1R, BIT(19));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS0R,
+			 (0x7 << 20), (0x3 << 20));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS2R,
+			 (0x1f << 5), (0x19 << 5));
+	mdelay(5);
+
+	sunxi_setbits(reg_base + AHCI_PHYCS0R, (0x1 << 19));
+
+	timeout = 250; /* Power up takes aprox 50 us */
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
+		if (reg_val == 0x02)
+			break;
+
+		if (--timeout == 0) {
+			dev_err(dev, "PHY power up failed.\n");
+			return -EIO;
+		}
+		udelay(1);
+	} while (1);
+
+	sunxi_setbits(reg_base + AHCI_PHYCS2R, (0x1 << 24));
+
+	timeout = 100; /* Calibration takes aprox 10 us */
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS2R, 0x1, 24);
+		if (reg_val == 0x00)
+			break;
+
+		if (--timeout == 0) {
+			dev_err(dev, "PHY calibration failed.\n");
+			return -EIO;
+		}
+		udelay(1);
+	} while (1);
+
+	mdelay(15);
+
+	writel(0x7, reg_base + AHCI_RWCR);
+
+	return 0;
+}
+
+static void ahci_sunxi_start_engine(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+
+	/* Setup DMA before DMA start */
+	sunxi_clrsetbits(hpriv->mmio + AHCI_P0DMACR, 0x0000ff00, 0x00004400);
+
+	/* Start DMA */
+	sunxi_setbits(port_mmio + PORT_CMD, PORT_CMD_START);
+}
+
+static const struct ata_port_info ahci_sunxi_port_info = {
+	AHCI_HFLAGS(AHCI_HFLAG_32BIT_ONLY | AHCI_HFLAG_NO_MSI |
+			  AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
+	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_platform_ops,
+};
+
+static int ahci_sunxi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ahci_host_priv *hpriv;
+	int rc;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	hpriv->start_engine = ahci_sunxi_start_engine;
+
+	rc = ahci_platform_enable_resources(hpriv);
+	if (rc)
+		goto put_resources;
+
+	rc = ahci_sunxi_phy_init(dev, hpriv->mmio);
+	if (rc)
+		goto disable_resources;
+
+	rc = ahci_platform_init_host(pdev, hpriv, &ahci_sunxi_port_info, 0, 0);
+	if (rc)
+		goto disable_resources;
+
+	return 0;
+
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
+put_resources:
+	ahci_platform_put_resources(hpriv);
+	return rc;
+}
+
+#ifdef CONFIG_PM_SLEEP
+int ahci_sunxi_resume(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	int rc;
+
+	rc = ahci_platform_enable_resources(hpriv);
+	if (rc)
+		return rc;
+
+	rc = ahci_sunxi_phy_init(dev, hpriv->mmio);
+	if (rc)
+		goto disable_resources;
+
+	rc = ahci_platform_resume_host(dev);
+	if (rc)
+		goto disable_resources;
+
+	return 0;
+
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
+	return rc;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(ahci_sunxi_pm_ops, ahci_platform_suspend,
+			 ahci_sunxi_resume);
+
+static const struct of_device_id ahci_sunxi_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-ahci", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ahci_sunxi_of_match);
+
+static struct platform_driver ahci_sunxi_driver = {
+	.probe = ahci_sunxi_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "ahci-sunxi",
+		.owner = THIS_MODULE,
+		.of_match_table = ahci_sunxi_of_match,
+		.pm = &ahci_sunxi_pm_ops,
+	},
+};
+module_platform_driver(ahci_sunxi_driver);
+
+MODULE_DESCRIPTION("Allwinner sunxi AHCI SATA driver");
+MODULE_AUTHOR("Olliver Schinagl <oliver@schinagl.nl>");
+MODULE_LICENSE("GPL");
-- 
1.8.5.3

[PATCH v6 10/18] ahci-imx: Port to library-ised ahci_platform

[Hans de Goede]

This avoids the ugliness of creating a nested platform device from probe.

While moving it around anyways, move the mk6q phy init code from probe
to imx_sata_enable, as the phy needs to be re-initialized on resume too,
otherwise the drive won't be recognized after resume.

Tested on a wandboard i.mx6 quad.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |   9 +-
 drivers/ata/ahci_imx.c                             | 338 ++++++++-------------
 2 files changed, 141 insertions(+), 206 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 499bfed..d86e854 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -5,8 +5,9 @@ Each SATA controller should have its own node.
 
 Required properties:
 - compatible        : compatible list, one of "snps,spear-ahci",
-                      "snps,exynos5440-ahci", "ibm,476gtr-ahci", or
-                      "allwinner,sun4i-a10-ahci"
+                      "snps,exynos5440-ahci", "ibm,476gtr-ahci",
+                      "allwinner,sun4i-a10-ahci", "fsl,imx53-ahci" or
+                      "fsl,imx6q-ahci"
 - interrupts        : <interrupt mapping for SATA IRQ>
 - reg               : <registers mapping>
 
@@ -15,6 +16,10 @@ Optional properties:
 - clocks            : a list of phandle + clock specifier pairs
 - target-supply     : regulator for SATA target power
 
+"fsl,imx53-ahci", "fsl,imx6q-ahci" required properties:
+- clocks            : must contain the sata, sata_ref and ahb clocks
+- clock-names       : must contain "ahb" for the ahb clock
+
 Examples:
         sata at ffe08000 {
 		compatible = "snps,spear-ahci";
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index ebfd1d6..7d51ee2 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -42,13 +42,7 @@ enum ahci_imx_type {
 struct imx_ahci_priv {
 	struct platform_device *ahci_pdev;
 	enum ahci_imx_type type;
-
-	/* i.MX53 clock */
-	struct clk *sata_gate_clk;
-	/* Common clock */
-	struct clk *sata_ref_clk;
 	struct clk *ahb_clk;
-
 	struct regmap *gpr;
 	bool no_device;
 	bool first_time;
@@ -58,28 +52,52 @@ static int ahci_imx_hotplug;
 module_param_named(hotplug, ahci_imx_hotplug, int, 0644);
 MODULE_PARM_DESC(hotplug, "AHCI IMX hot-plug support (0=Don't support, 1=support)");
 
-static int imx_sata_clock_enable(struct device *dev)
+static void ahci_imx_host_stop(struct ata_host *host);
+
+static int imx_sata_enable(struct ahci_host_priv *hpriv)
 {
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
+	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
 	int ret;
 
-	if (imxpriv->type == AHCI_IMX53) {
-		ret = clk_prepare_enable(imxpriv->sata_gate_clk);
-		if (ret < 0) {
-			dev_err(dev, "prepare-enable sata_gate clock err:%d\n",
-				ret);
+	if (imxpriv->no_device)
+		return 0;
+
+	if (hpriv->target_pwr) {
+		ret = regulator_enable(hpriv->target_pwr);
+		if (ret)
 			return ret;
-		}
 	}
 
-	ret = clk_prepare_enable(imxpriv->sata_ref_clk);
-	if (ret < 0) {
-		dev_err(dev, "prepare-enable sata_ref clock err:%d\n",
-			ret);
-		goto clk_err;
-	}
+	ret = ahci_platform_enable_clks(hpriv);
+	if (ret < 0)
+		goto disable_regulator;
 
 	if (imxpriv->type == AHCI_IMX6Q) {
+		/*
+		 * set PHY Paremeters, two steps to configure the GPR13,
+		 * one write for rest of parameters, mask of first write
+		 * is 0x07ffffff, and the other one write for setting
+		 * the mpll_clk_en.
+		 */
+		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
+				   IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK |
+				   IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK |
+				   IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK |
+				   IMX6Q_GPR13_SATA_SPD_MODE_MASK |
+				   IMX6Q_GPR13_SATA_MPLL_SS_EN |
+				   IMX6Q_GPR13_SATA_TX_ATTEN_MASK |
+				   IMX6Q_GPR13_SATA_TX_BOOST_MASK |
+				   IMX6Q_GPR13_SATA_TX_LVL_MASK |
+				   IMX6Q_GPR13_SATA_MPLL_CLK_EN |
+				   IMX6Q_GPR13_SATA_TX_EDGE_RATE,
+				   IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB |
+				   IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M |
+				   IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F |
+				   IMX6Q_GPR13_SATA_SPD_MODE_3P0G |
+				   IMX6Q_GPR13_SATA_MPLL_SS_EN |
+				   IMX6Q_GPR13_SATA_TX_ATTEN_9_16 |
+				   IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB |
+				   IMX6Q_GPR13_SATA_TX_LVL_1_025_V);
 		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
 				   IMX6Q_GPR13_SATA_MPLL_CLK_EN,
 				   IMX6Q_GPR13_SATA_MPLL_CLK_EN);
@@ -89,15 +107,19 @@ static int imx_sata_clock_enable(struct device *dev)
 
 	return 0;
 
-clk_err:
-	if (imxpriv->type == AHCI_IMX53)
-		clk_disable_unprepare(imxpriv->sata_gate_clk);
+disable_regulator:
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
+
 	return ret;
 }
 
-static void imx_sata_clock_disable(struct device *dev)
+static void imx_sata_disable(struct ahci_host_priv *hpriv)
 {
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
+	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
+
+	if (imxpriv->no_device)
+		return;
 
 	if (imxpriv->type == AHCI_IMX6Q) {
 		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
@@ -105,10 +127,10 @@ static void imx_sata_clock_disable(struct device *dev)
 				   !IMX6Q_GPR13_SATA_MPLL_CLK_EN);
 	}
 
-	clk_disable_unprepare(imxpriv->sata_ref_clk);
+	ahci_platform_disable_clks(hpriv);
 
-	if (imxpriv->type == AHCI_IMX53)
-		clk_disable_unprepare(imxpriv->sata_gate_clk);
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
 }
 
 static void ahci_imx_error_handler(struct ata_port *ap)
@@ -118,7 +140,7 @@ static void ahci_imx_error_handler(struct ata_port *ap)
 	struct ata_host *host = dev_get_drvdata(ap->dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(ap->dev->parent);
+	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
 
 	ahci_error_handler(ap);
 
@@ -136,7 +158,7 @@ static void ahci_imx_error_handler(struct ata_port *ap)
 	 */
 	reg_val = readl(mmio + PORT_PHY_CTL);
 	writel(reg_val | PORT_PHY_CTL_PDDQ_LOC, mmio + PORT_PHY_CTL);
-	imx_sata_clock_disable(ap->dev);
+	imx_sata_disable(hpriv);
 	imxpriv->no_device = true;
 }
 
@@ -144,7 +166,9 @@ static int ahci_imx_softreset(struct ata_link *link, unsigned int *class,
 		       unsigned long deadline)
 {
 	struct ata_port *ap = link->ap;
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(ap->dev->parent);
+	struct ata_host *host = dev_get_drvdata(ap->dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
 	int ret = -EIO;
 
 	if (imxpriv->type == AHCI_IMX53)
@@ -156,7 +180,8 @@ static int ahci_imx_softreset(struct ata_link *link, unsigned int *class,
 }
 
 static struct ata_port_operations ahci_imx_ops = {
-	.inherits	= &ahci_platform_ops,
+	.inherits	= &ahci_ops,
+	.host_stop	= ahci_imx_host_stop,
 	.error_handler	= ahci_imx_error_handler,
 	.softreset	= ahci_imx_softreset,
 };
@@ -168,79 +193,6 @@ static const struct ata_port_info ahci_imx_port_info = {
 	.port_ops	= &ahci_imx_ops,
 };
 
-static int imx_sata_init(struct device *dev, struct ahci_host_priv *hpriv)
-{
-	int ret = 0;
-	unsigned int reg_val;
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
-
-	ret = imx_sata_clock_enable(dev);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
-	 * and IP vendor specific register HOST_TIMER1MS.
-	 * Configure CAP_SSS (support stagered spin up).
-	 * Implement the port0.
-	 * Get the ahb clock rate, and configure the TIMER1MS register.
-	 */
-	reg_val = readl(hpriv->mmio + HOST_CAP);
-	if (!(reg_val & HOST_CAP_SSS)) {
-		reg_val |= HOST_CAP_SSS;
-		writel(reg_val, hpriv->mmio + HOST_CAP);
-	}
-	reg_val = readl(hpriv->mmio + HOST_PORTS_IMPL);
-	if (!(reg_val & 0x1)) {
-		reg_val |= 0x1;
-		writel(reg_val, hpriv->mmio + HOST_PORTS_IMPL);
-	}
-
-	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
-	writel(reg_val, hpriv->mmio + HOST_TIMER1MS);
-
-	return 0;
-}
-
-static void imx_sata_exit(struct device *dev)
-{
-	imx_sata_clock_disable(dev);
-}
-
-static int imx_ahci_suspend(struct device *dev)
-{
-	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
-
-	/*
-	 * If no_device is set, The CLKs had been gated off in the
-	 * initialization so don't do it again here.
-	 */
-	if (!imxpriv->no_device)
-		imx_sata_clock_disable(dev);
-
-	return 0;
-}
-
-static int imx_ahci_resume(struct device *dev)
-{
-	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
-	int ret = 0;
-
-	if (!imxpriv->no_device)
-		ret = imx_sata_clock_enable(dev);
-
-	return ret;
-}
-
-static struct ahci_platform_data imx_sata_pdata = {
-	.init		= imx_sata_init,
-	.exit		= imx_sata_exit,
-	.ata_port_info	= &ahci_imx_port_info,
-	.suspend	= imx_ahci_suspend,
-	.resume		= imx_ahci_resume,
-
-};
-
 static const struct of_device_id imx_ahci_of_match[] = {
 	{ .compatible = "fsl,imx53-ahci", .data = (void *)AHCI_IMX53 },
 	{ .compatible = "fsl,imx6q-ahci", .data = (void *)AHCI_IMX6Q },
@@ -251,151 +203,129 @@ MODULE_DEVICE_TABLE(of, imx_ahci_of_match);
 static int imx_ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct resource *mem, *irq, res[2];
 	const struct of_device_id *of_id;
-	enum ahci_imx_type type;
-	const struct ahci_platform_data *pdata = NULL;
+	struct ahci_host_priv *hpriv;
 	struct imx_ahci_priv *imxpriv;
-	struct device *ahci_dev;
-	struct platform_device *ahci_pdev;
+	unsigned int reg_val;
 	int ret;
 
 	of_id = of_match_device(imx_ahci_of_match, dev);
 	if (!of_id)
 		return -EINVAL;
 
-	type = (enum ahci_imx_type)of_id->data;
-	pdata = &imx_sata_pdata;
-
 	imxpriv = devm_kzalloc(dev, sizeof(*imxpriv), GFP_KERNEL);
-	if (!imxpriv) {
-		dev_err(dev, "can't alloc ahci_host_priv\n");
+	if (!imxpriv)
 		return -ENOMEM;
-	}
-
-	ahci_pdev = platform_device_alloc("ahci", -1);
-	if (!ahci_pdev)
-		return -ENODEV;
-
-	ahci_dev = &ahci_pdev->dev;
-	ahci_dev->parent = dev;
 
 	imxpriv->no_device = false;
 	imxpriv->first_time = true;
-	imxpriv->type = type;
-
+	imxpriv->type = (enum ahci_imx_type)of_id->data;
 	imxpriv->ahb_clk = devm_clk_get(dev, "ahb");
 	if (IS_ERR(imxpriv->ahb_clk)) {
 		dev_err(dev, "can't get ahb clock.\n");
-		ret = PTR_ERR(imxpriv->ahb_clk);
-		goto err_out;
+		return PTR_ERR(imxpriv->ahb_clk);
 	}
 
-	if (type == AHCI_IMX53) {
-		imxpriv->sata_gate_clk = devm_clk_get(dev, "sata_gate");
-		if (IS_ERR(imxpriv->sata_gate_clk)) {
-			dev_err(dev, "can't get sata_gate clock.\n");
-			ret = PTR_ERR(imxpriv->sata_gate_clk);
-			goto err_out;
+	if (imxpriv->type == AHCI_IMX6Q) {
+		imxpriv->gpr = syscon_regmap_lookup_by_compatible(
+							"fsl,imx6q-iomuxc-gpr");
+		if (IS_ERR(imxpriv->gpr)) {
+			dev_err(dev,
+				"failed to find fsl,imx6q-iomux-gpr regmap\n");
+			return PTR_ERR(imxpriv->gpr);
 		}
 	}
 
-	imxpriv->sata_ref_clk = devm_clk_get(dev, "sata_ref");
-	if (IS_ERR(imxpriv->sata_ref_clk)) {
-		dev_err(dev, "can't get sata_ref clock.\n");
-		ret = PTR_ERR(imxpriv->sata_ref_clk);
-		goto err_out;
-	}
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
 
-	imxpriv->ahci_pdev = ahci_pdev;
-	platform_set_drvdata(pdev, imxpriv);
+	hpriv->plat_data = imxpriv;
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!mem || !irq) {
-		dev_err(dev, "no mmio/irq resource\n");
-		ret = -ENOMEM;
-		goto err_out;
+	ret = imx_sata_enable(hpriv);
+	if (ret)
+		goto put_resources;
+
+	/*
+	 * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
+	 * and IP vendor specific register HOST_TIMER1MS.
+	 * Configure CAP_SSS (support stagered spin up).
+	 * Implement the port0.
+	 * Get the ahb clock rate, and configure the TIMER1MS register.
+	 */
+	reg_val = readl(hpriv->mmio + HOST_CAP);
+	if (!(reg_val & HOST_CAP_SSS)) {
+		reg_val |= HOST_CAP_SSS;
+		writel(reg_val, hpriv->mmio + HOST_CAP);
+	}
+	reg_val = readl(hpriv->mmio + HOST_PORTS_IMPL);
+	if (!(reg_val & 0x1)) {
+		reg_val |= 0x1;
+		writel(reg_val, hpriv->mmio + HOST_PORTS_IMPL);
 	}
 
-	res[0] = *mem;
-	res[1] = *irq;
+	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
+	writel(reg_val, hpriv->mmio + HOST_TIMER1MS);
 
-	ahci_dev->coherent_dma_mask = DMA_BIT_MASK(32);
-	ahci_dev->dma_mask = &ahci_dev->coherent_dma_mask;
-	ahci_dev->of_node = dev->of_node;
+	ret = ahci_platform_init_host(pdev, hpriv, &ahci_imx_port_info, 0, 0);
+	if (ret)
+		goto disable;
 
-	if (type == AHCI_IMX6Q) {
-		imxpriv->gpr = syscon_regmap_lookup_by_compatible(
-							"fsl,imx6q-iomuxc-gpr");
-		if (IS_ERR(imxpriv->gpr)) {
-			dev_err(dev,
-				"failed to find fsl,imx6q-iomux-gpr regmap\n");
-			ret = PTR_ERR(imxpriv->gpr);
-			goto err_out;
-		}
+	return 0;
 
-		/*
-		 * Set PHY Paremeters, two steps to configure the GPR13,
-		 * one write for rest of parameters, mask of first write
-		 * is 0x07fffffe, and the other one write for setting
-		 * the mpll_clk_en happens in imx_sata_clock_enable().
-		 */
-		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
-				   IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK |
-				   IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK |
-				   IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK |
-				   IMX6Q_GPR13_SATA_SPD_MODE_MASK |
-				   IMX6Q_GPR13_SATA_MPLL_SS_EN |
-				   IMX6Q_GPR13_SATA_TX_ATTEN_MASK |
-				   IMX6Q_GPR13_SATA_TX_BOOST_MASK |
-				   IMX6Q_GPR13_SATA_TX_LVL_MASK |
-				   IMX6Q_GPR13_SATA_MPLL_CLK_EN |
-				   IMX6Q_GPR13_SATA_TX_EDGE_RATE,
-				   IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB |
-				   IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M |
-				   IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F |
-				   IMX6Q_GPR13_SATA_SPD_MODE_3P0G |
-				   IMX6Q_GPR13_SATA_MPLL_SS_EN |
-				   IMX6Q_GPR13_SATA_TX_ATTEN_9_16 |
-				   IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB |
-				   IMX6Q_GPR13_SATA_TX_LVL_1_025_V);
-	}
+disable:
+	imx_sata_disable(hpriv);
+put_resources:
+	ahci_platform_put_resources(hpriv);
+	return ret;
+}
 
-	ret = platform_device_add_resources(ahci_pdev, res, 2);
-	if (ret)
-		goto err_out;
+static void ahci_imx_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
 
-	ret = platform_device_add_data(ahci_pdev, pdata, sizeof(*pdata));
-	if (ret)
-		goto err_out;
+	imx_sata_disable(hpriv);
+	ahci_platform_put_resources(hpriv);
+}
+
+static int imx_ahci_suspend(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	int ret;
 
-	ret = platform_device_add(ahci_pdev);
-	if (ret) {
-err_out:
-		platform_device_put(ahci_pdev);
+	ret = ahci_platform_suspend_host(dev);
+	if (ret)
 		return ret;
-	}
+
+	imx_sata_disable(hpriv);
 
 	return 0;
 }
 
-static int imx_ahci_remove(struct platform_device *pdev)
+static int imx_ahci_resume(struct device *dev)
 {
-	struct imx_ahci_priv *imxpriv = platform_get_drvdata(pdev);
-	struct platform_device *ahci_pdev = imxpriv->ahci_pdev;
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	int ret;
 
-	platform_device_unregister(ahci_pdev);
-	return 0;
+	ret = imx_sata_enable(hpriv);
+	if (ret)
+		return ret;
+
+	return ahci_platform_resume_host(dev);
 }
 
+static SIMPLE_DEV_PM_OPS(ahci_imx_pm_ops, imx_ahci_suspend, imx_ahci_resume);
+
 static struct platform_driver imx_ahci_driver = {
 	.probe = imx_ahci_probe,
-	.remove = imx_ahci_remove,
+	.remove = ata_platform_remove_one,
 	.driver = {
 		.name = "ahci-imx",
 		.owner = THIS_MODULE,
 		.of_match_table = imx_ahci_of_match,
+		.pm = &ahci_imx_pm_ops,
 	},
 };
 module_platform_driver(imx_ahci_driver);
-- 
1.8.5.3

[PATCH v6 11/18] ata: ahci_platform: Add DT compatible for Synopsis DWC AHCI controller

[Hans de Goede]

From: Roger Quadros <rogerq@ti.com>

Add compatible string "snps,dwc-ahci", which should be used
for Synopsis Designware SATA cores. e.g. on TI OMAP5 and DRA7 platforms.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index ba93930..962ec25 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -456,6 +456,7 @@ static const struct of_device_id ahci_of_match[] = {
 	{ .compatible = "snps,spear-ahci", },
 	{ .compatible = "snps,exynos5440-ahci", },
 	{ .compatible = "ibm,476gtr-ahci", },
+	{ .compatible = "snps,dwc-ahci", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ahci_of_match);
-- 
1.8.5.3

[PATCH v6 12/18] ata: ahci_platform: Update DT compatible list

[Hans de Goede]

From: Roger Quadros <rogerq@ti.com>

The ahci_platform driver supports "snps,dwc-ahci".
Add this to the DT binding information.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index d86e854..48b285f 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -6,8 +6,8 @@ Each SATA controller should have its own node.
 Required properties:
 - compatible        : compatible list, one of "snps,spear-ahci",
                       "snps,exynos5440-ahci", "ibm,476gtr-ahci",
-                      "allwinner,sun4i-a10-ahci", "fsl,imx53-ahci" or
-                      "fsl,imx6q-ahci"
+                      "allwinner,sun4i-a10-ahci", "fsl,imx53-ahci"
+                      "fsl,imx6q-ahci" or "snps,dwc-ahci"
 - interrupts        : <interrupt mapping for SATA IRQ>
 - reg               : <registers mapping>
 
-- 
1.8.5.3

[PATCH v6 13/18] ata: ahci_platform: Manage SATA PHY

[Hans de Goede]

From: Roger Quadros <rogerq@ti.com>

Some platforms have a PHY hooked up to the
SATA controller. The PHY needs to be initialized
and powered up for SATA to work. We do that
using the PHY framework.

CC: Balaji T K <balajitk@ti.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/Kconfig         |  1 +
 drivers/ata/ahci_platform.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/ahci.h        |  2 ++
 3 files changed, 43 insertions(+)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index cc67cc0..96176f4 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -91,6 +91,7 @@ config SATA_AHCI
 
 config SATA_AHCI_PLATFORM
 	tristate "Platform AHCI SATA support"
+	depends on GENERIC_PHY || !GENERIC_PHY
 	help
 	  This option enables support for Platform AHCI Serial ATA
 	  controllers.
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 962ec25..073931a 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
+#include <linux/phy/phy.h>
 #include "ahci.h"
 
 static void ahci_host_stop(struct ata_host *host);
@@ -131,8 +132,23 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
 	if (rc)
 		goto disable_regulator;
 
+	if (hpriv->phy) {
+		rc = phy_init(hpriv->phy);
+		if (rc)
+			goto disable_clks;
+
+		rc = phy_power_on(hpriv->phy);
+		if (rc) {
+			phy_exit(hpriv->phy);
+			goto disable_clks;
+		}
+	}
+
 	return 0;
 
+disable_clks:
+	ahci_platform_disable_clks(hpriv);
+
 disable_regulator:
 	if (hpriv->target_pwr)
 		regulator_disable(hpriv->target_pwr);
@@ -142,6 +158,11 @@ EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
 
 void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
 {
+	if (hpriv->phy) {
+		phy_power_off(hpriv->phy);
+		phy_exit(hpriv->phy);
+	}
+
 	ahci_platform_disable_clks(hpriv);
 
 	if (hpriv->target_pwr)
@@ -194,6 +215,25 @@ struct ahci_host_priv *ahci_platform_get_resources(
 		hpriv->clks[i] = clk;
 	}
 
+	hpriv->phy = devm_phy_get(dev, "sata-phy");
+	if (IS_ERR(hpriv->phy)) {
+		rc = PTR_ERR(hpriv->phy);
+		switch (rc) {
+		case -ENODEV:
+		case -ENOSYS:
+			/* continue normally */
+			hpriv->phy = NULL;
+			break;
+
+		case -EPROBE_DEFER:
+			goto free_clk;
+
+		default:
+			dev_err(dev, "couldn't get sata-phy\n");
+			goto free_clk;
+		}
+	}
+
 	return hpriv;
 
 free_clk:
diff --git a/include/linux/ahci.h b/include/linux/ahci.h
index ac69cdc..8fbe3d7 100644
--- a/include/linux/ahci.h
+++ b/include/linux/ahci.h
@@ -23,6 +23,7 @@
 
 #include <linux/clk.h>
 #include <linux/regulator/consumer.h>
+#include <linux/phy/phy.h>
 
 #define AHCI_MAX_CLKS		3
 
@@ -42,6 +43,7 @@ struct ahci_host_priv {
 	u32			em_msg_type;	/* EM message type */
 	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
 	struct regulator	*target_pwr;	/* Optional */
+	struct phy		*phy;		/* If platform uses phy */
 	void			*plat_data;	/* Other platform data */
 	/* Optional ahci_start_engine override */
 	void			(*start_engine)(struct ata_port *ap);
-- 
1.8.5.3

[PATCH v6 14/18] ata: ahci_platform: Add 'struct device' argument to ahci_platform_put_resources()

[Hans de Goede]

From: Roger Quadros <rogerq@ti.com>

There is no easy way to access 'struct device' from 'struct ahci_host_priv'
so add an explicit argument for 'struct device'. We will need it
to call Runtime PM APIs.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_imx.c        | 5 +++--
 drivers/ata/ahci_platform.c   | 7 ++++---
 drivers/ata/ahci_sunxi.c      | 2 +-
 include/linux/ahci_platform.h | 3 ++-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 7d51ee2..5cdaf0b 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -276,16 +276,17 @@ static int imx_ahci_probe(struct platform_device *pdev)
 disable:
 	imx_sata_disable(hpriv);
 put_resources:
-	ahci_platform_put_resources(hpriv);
+	ahci_platform_put_resources(dev, hpriv);
 	return ret;
 }
 
 static void ahci_imx_host_stop(struct ata_host *host)
 {
+	struct device *dev = host->dev;
 	struct ahci_host_priv *hpriv = host->private_data;
 
 	imx_sata_disable(hpriv);
-	ahci_platform_put_resources(hpriv);
+	ahci_platform_put_resources(dev, hpriv);
 }
 
 static int imx_ahci_suspend(struct device *dev)
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 073931a..f8ef780 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -243,7 +243,8 @@ free_clk:
 }
 EXPORT_SYMBOL_GPL(ahci_platform_get_resources);
 
-void ahci_platform_put_resources(struct ahci_host_priv *hpriv)
+void ahci_platform_put_resources(struct device *dev,
+				 struct ahci_host_priv *hpriv)
 {
 	int c;
 
@@ -375,7 +376,7 @@ pdata_exit:
 disable_resources:
 	ahci_platform_disable_resources(hpriv);
 put_resources:
-	ahci_platform_put_resources(hpriv);
+	ahci_platform_put_resources(dev, hpriv);
 	return rc;
 }
 
@@ -389,7 +390,7 @@ static void ahci_host_stop(struct ata_host *host)
 		pdata->exit(dev);
 
 	ahci_platform_disable_resources(hpriv);
-	ahci_platform_put_resources(hpriv);
+	ahci_platform_put_resources(dev, hpriv);
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/ata/ahci_sunxi.c b/drivers/ata/ahci_sunxi.c
index 568a211..219d77f 100644
--- a/drivers/ata/ahci_sunxi.c
+++ b/drivers/ata/ahci_sunxi.c
@@ -194,7 +194,7 @@ static int ahci_sunxi_probe(struct platform_device *pdev)
 disable_resources:
 	ahci_platform_disable_resources(hpriv);
 put_resources:
-	ahci_platform_put_resources(hpriv);
+	ahci_platform_put_resources(dev, hpriv);
 	return rc;
 }
 
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 7c683c7..719678c 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -43,7 +43,8 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
 struct ahci_host_priv *ahci_platform_get_resources(
 	struct platform_device *pdev);
-void ahci_platform_put_resources(struct ahci_host_priv *hpriv);
+void ahci_platform_put_resources(struct device *dev,
+				 struct ahci_host_priv *hpriv);
 int ahci_platform_init_host(struct platform_device *pdev,
 			    struct ahci_host_priv *hpriv,
 			    const struct ata_port_info *pi_template,
-- 
1.8.5.3

[PATCH v6 15/18] ata: ahci_platform: runtime resume the device before use

[Hans de Goede]

From: Roger Quadros <rogerq@ti.com>

On OMAP platforms the device needs to be runtime resumed before
it can be accessed. The OMAP HWMOD framework takes care of
enabling the module and its resources based on the
device's runtime PM state.

In this patch we runtime resume during .probe() and runtime suspend
during .remove() (i.e. ahci_host_stop()).

We also update the runtime PM state during .resume().

CC: Balaji T K <balajitk@ti.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_platform.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index f8ef780..bb2e7ab 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -24,6 +24,7 @@
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
 #include <linux/phy/phy.h>
+#include <linux/pm_runtime.h>
 #include "ahci.h"
 
 static void ahci_host_stop(struct ata_host *host);
@@ -234,6 +235,9 @@ struct ahci_host_priv *ahci_platform_get_resources(
 		}
 	}
 
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
 	return hpriv;
 
 free_clk:
@@ -248,6 +252,9 @@ void ahci_platform_put_resources(struct device *dev,
 {
 	int c;
 
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+
 	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
 		clk_put(hpriv->clks[c]);
 }
@@ -480,6 +487,11 @@ int ahci_platform_resume(struct device *dev)
 	if (rc)
 		goto disable_resources;
 
+	/* We resumed so update PM runtime state */
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	return 0;
 
 disable_resources:
-- 
1.8.5.3

[PATCH v6 16/18] ARM: sun4i: dt: Remove grouping + simple-bus compatible for regulators

[Hans de Goede]

According to Documentation/devicetree/bindings/regulator/regulator.txt
regulator nodes should not be placed under 'simple-bus'.

Mark Rutland also explains about it at:
http://www.spinics.net/lists/linux-usb/msg101497.html

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/boot/dts/sun4i-a10-a1000.dts     | 22 +++++++++-------------
 arch/arm/boot/dts/sun4i-a10-hackberry.dts | 18 +++++++-----------
 2 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
index d4b081d..cbd2e13 100644
--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
@@ -80,18 +80,14 @@
 		};
 	};
 
-	regulators {
-		compatible = "simple-bus";
-
-		reg_emac_3v3: emac-3v3 {
-			compatible = "regulator-fixed";
-			pinctrl-names = "default";
-			pinctrl-0 = <&emac_power_pin_a1000>;
-			regulator-name = "emac-3v3";
-			regulator-min-microvolt = <3300000>;
-			regulator-max-microvolt = <3300000>;
-			enable-active-high;
-			gpio = <&pio 7 15 0>;
-		};
+	reg_emac_3v3: emac-3v3 {
+		compatible = "regulator-fixed";
+		pinctrl-names = "default";
+		pinctrl-0 = <&emac_power_pin_a1000>;
+		regulator-name = "emac-3v3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		enable-active-high;
+		gpio = <&pio 7 15 0>;
 	};
 };
diff --git a/arch/arm/boot/dts/sun4i-a10-hackberry.dts b/arch/arm/boot/dts/sun4i-a10-hackberry.dts
index 3a1595f..6692d336 100644
--- a/arch/arm/boot/dts/sun4i-a10-hackberry.dts
+++ b/arch/arm/boot/dts/sun4i-a10-hackberry.dts
@@ -54,16 +54,12 @@
 		};
 	};
 
-	regulators {
-		compatible = "simple-bus";
-
-		reg_emac_3v3: emac-3v3 {
-			compatible = "regulator-fixed";
-			regulator-name = "emac-3v3";
-			regulator-min-microvolt = <3300000>;
-			regulator-max-microvolt = <3300000>;
-			enable-active-high;
-			gpio = <&pio 7 19 0>;
-		};
+	reg_emac_3v3: emac-3v3 {
+		compatible = "regulator-fixed";
+		regulator-name = "emac-3v3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		enable-active-high;
+		gpio = <&pio 7 19 0>;
 	};
 };
-- 
1.8.5.3

[PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support

[Hans de Goede]

From: Oliver Schinagl <oliver@schinagl.nl>

This patch adds sunxi sata support to A10 boards that have such a connector.
Some boards also feature a regulator via a GPIO and support for this is also
added.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/boot/dts/sun4i-a10-a1000.dts      |  4 ++++
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts |  6 +++++
 arch/arm/boot/dts/sun4i-a10.dtsi           |  8 +++++++
 arch/arm/boot/dts/sunxi-ahci-reg.dtsi      | 36 ++++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi

diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
index cbd2e13..d6ec839 100644
--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
@@ -35,6 +35,10 @@
 			};
 		};
 
+		ahci: sata at 01c18000 {
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
 			emac_power_pin_a1000: emac_power_pin at 0 {
 				allwinner,pins = "PH15";
diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
index b139ee6..6df237d8 100644
--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
@@ -12,6 +12,7 @@
 
 /dts-v1/;
 /include/ "sun4i-a10.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
 
 / {
 	model = "Cubietech Cubieboard";
@@ -33,6 +34,11 @@
 			};
 		};
 
+		ahci: sata at 01c18000 {
+			target-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
 			led_pins_cubieboard: led_pins at 0 {
 				allwinner,pins = "PH20", "PH21";
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 336dbec..454077a 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -338,6 +338,14 @@
 			#size-cells = <0>;
 		};
 
+		ahci: sata at 01c18000 {
+			compatible = "allwinner,sun4i-a10-ahci";
+			reg = <0x01c18000 0x1000>;
+			interrupts = <56>;
+			clocks = <&pll6 0>, <&ahb_gates 25>;
+			status = "disabled";
+		};
+
 		intc: interrupt-controller at 01c20400 {
 			compatible = "allwinner,sun4i-ic";
 			reg = <0x01c20400 0x400>;
diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
new file mode 100644
index 0000000..7072af1
--- /dev/null
+++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
@@ -0,0 +1,36 @@
+/*
+ * sunxi boards sata target power supply common code
+ *
+ * Copyright 2014 - Hans de Goede <hdegoede@redhat.com>
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/ {
+	soc at 01c00000 {
+		pio: pinctrl at 01c20800 {
+			ahci_pwr_pin_a: ahci_pwr_pin at 0 {
+				allwinner,pins = "PB8";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+		};
+	};
+
+	reg_ahci_5v: ahci-5v {
+		compatible = "regulator-fixed";
+		pinctrl-names = "default";
+		pinctrl-0 = <&ahci_pwr_pin_a>;
+		regulator-name = "ahci-5v";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		enable-active-high;
+		gpio = <&pio 1 8 0>;
+	};
+};
-- 
1.8.5.3

[PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support

[Maxime Ripard]

Hi Hans,

On Wed, Feb 19, 2014 at 01:01:59PM +0100, Hans de Goede wrote:
> From: Oliver Schinagl <oliver@schinagl.nl>
> 
> This patch adds sunxi sata support to A10 boards that have such a connector.
> Some boards also feature a regulator via a GPIO and support for this is also
> added.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  arch/arm/boot/dts/sun4i-a10-a1000.dts      |  4 ++++
>  arch/arm/boot/dts/sun4i-a10-cubieboard.dts |  6 +++++
>  arch/arm/boot/dts/sun4i-a10.dtsi           |  8 +++++++
>  arch/arm/boot/dts/sunxi-ahci-reg.dtsi      | 36 ++++++++++++++++++++++++++++++
>  4 files changed, 54 insertions(+)
>  create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> 
> diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
> index cbd2e13..d6ec839 100644
> --- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
> +++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
> @@ -35,6 +35,10 @@
>  			};
>  		};
>  
> +		ahci: sata at 01c18000 {
> +			status = "okay";
> +		};
> +
>  		pinctrl at 01c20800 {
>  			emac_power_pin_a1000: emac_power_pin at 0 {
>  				allwinner,pins = "PH15";
> diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> index b139ee6..6df237d8 100644
> --- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> +++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> @@ -12,6 +12,7 @@
>  
>  /dts-v1/;
>  /include/ "sun4i-a10.dtsi"
> +/include/ "sunxi-ahci-reg.dtsi"
>  
>  / {
>  	model = "Cubietech Cubieboard";
> @@ -33,6 +34,11 @@
>  			};
>  		};
>  
> +		ahci: sata at 01c18000 {
> +			target-supply = <&reg_ahci_5v>;
> +			status = "okay";
> +		};
> +
>  		pinctrl at 01c20800 {
>  			led_pins_cubieboard: led_pins at 0 {
>  				allwinner,pins = "PH20", "PH21";
> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> index 336dbec..454077a 100644
> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> @@ -338,6 +338,14 @@
>  			#size-cells = <0>;
>  		};
>  
> +		ahci: sata at 01c18000 {
> +			compatible = "allwinner,sun4i-a10-ahci";
> +			reg = <0x01c18000 0x1000>;
> +			interrupts = <56>;
> +			clocks = <&pll6 0>, <&ahb_gates 25>;
> +			status = "disabled";
> +		};
> +
>  		intc: interrupt-controller at 01c20400 {
>  			compatible = "allwinner,sun4i-ic";
>  			reg = <0x01c20400 0x400>;
> diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> new file mode 100644
> index 0000000..7072af1
> --- /dev/null
> +++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> @@ -0,0 +1,36 @@
> +/*
> + * sunxi boards sata target power supply common code


Since IIRC we have pretty much the same needs for the USB, can't we
just drop the SATA specific mention and use it as the common DTSI for
the usual regulators?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140221/e4884b34/attachment.sig>

[PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support

[Hans de Goede]

Hi Maxime,

On 02/21/2014 07:15 PM, Maxime Ripard wrote:
> Hi Hans,
>
> On Wed, Feb 19, 2014 at 01:01:59PM +0100, Hans de Goede wrote:
>> From: Oliver Schinagl <oliver@schinagl.nl>
>>
>> This patch adds sunxi sata support to A10 boards that have such a connector.
>> Some boards also feature a regulator via a GPIO and support for this is also
>> added.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   arch/arm/boot/dts/sun4i-a10-a1000.dts      |  4 ++++
>>   arch/arm/boot/dts/sun4i-a10-cubieboard.dts |  6 +++++
>>   arch/arm/boot/dts/sun4i-a10.dtsi           |  8 +++++++
>>   arch/arm/boot/dts/sunxi-ahci-reg.dtsi      | 36 ++++++++++++++++++++++++++++++
>>   4 files changed, 54 insertions(+)
>>   create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi
>>
>> diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
>> index cbd2e13..d6ec839 100644
>> --- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
>> +++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
>> @@ -35,6 +35,10 @@
>>   			};
>>   		};
>>
>> +		ahci: sata at 01c18000 {
>> +			status = "okay";
>> +		};
>> +
>>   		pinctrl at 01c20800 {
>>   			emac_power_pin_a1000: emac_power_pin at 0 {
>>   				allwinner,pins = "PH15";
>> diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
>> index b139ee6..6df237d8 100644
>> --- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
>> +++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
>> @@ -12,6 +12,7 @@
>>
>>   /dts-v1/;
>>   /include/ "sun4i-a10.dtsi"
>> +/include/ "sunxi-ahci-reg.dtsi"
>>
>>   / {
>>   	model = "Cubietech Cubieboard";
>> @@ -33,6 +34,11 @@
>>   			};
>>   		};
>>
>> +		ahci: sata at 01c18000 {
>> +			target-supply = <&reg_ahci_5v>;
>> +			status = "okay";
>> +		};
>> +
>>   		pinctrl at 01c20800 {
>>   			led_pins_cubieboard: led_pins at 0 {
>>   				allwinner,pins = "PH20", "PH21";
>> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
>> index 336dbec..454077a 100644
>> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
>> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
>> @@ -338,6 +338,14 @@
>>   			#size-cells = <0>;
>>   		};
>>
>> +		ahci: sata at 01c18000 {
>> +			compatible = "allwinner,sun4i-a10-ahci";
>> +			reg = <0x01c18000 0x1000>;
>> +			interrupts = <56>;
>> +			clocks = <&pll6 0>, <&ahb_gates 25>;
>> +			status = "disabled";
>> +		};
>> +
>>   		intc: interrupt-controller at 01c20400 {
>>   			compatible = "allwinner,sun4i-ic";
>>   			reg = <0x01c20400 0x400>;
>> diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
>> new file mode 100644
>> index 0000000..7072af1
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
>> @@ -0,0 +1,36 @@
>> +/*
>> + * sunxi boards sata target power supply common code
>
>
> Since IIRC we have pretty much the same needs for the USB, can't we
> just drop the SATA specific mention and use it as the common DTSI for
> the usual regulators?

On most boards with sata, there will also be 1 or 2 usb regulators,
so we need differently named regulator nodes for all 3 of ahci,
usb1 and usb2 vbus. On some boards how ever we may only need the
usb regulators. So if you look in my current personal sunxi-devel
tree you will see separate dtsi files for both ahci and usb regulators,
another advantage of having these separate is that the gpio controlling
the regulator can be pre-populated with the reference design gpio which
is used in most boards, so that the ahci specific code in the dts
becomes only the ahci: sata at ... node.

Regards,

Hans

[PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support

[Maxime Ripard]

On Sat, Feb 22, 2014 at 11:09:25AM +0100, Hans de Goede wrote:
> Hi Maxime,
> 
> On 02/21/2014 07:15 PM, Maxime Ripard wrote:
> >Hi Hans,
> >
> >On Wed, Feb 19, 2014 at 01:01:59PM +0100, Hans de Goede wrote:
> >>From: Oliver Schinagl <oliver@schinagl.nl>
> >>
> >>This patch adds sunxi sata support to A10 boards that have such a connector.
> >>Some boards also feature a regulator via a GPIO and support for this is also
> >>added.
> >>
> >>Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>  arch/arm/boot/dts/sun4i-a10-a1000.dts      |  4 ++++
> >>  arch/arm/boot/dts/sun4i-a10-cubieboard.dts |  6 +++++
> >>  arch/arm/boot/dts/sun4i-a10.dtsi           |  8 +++++++
> >>  arch/arm/boot/dts/sunxi-ahci-reg.dtsi      | 36 ++++++++++++++++++++++++++++++
> >>  4 files changed, 54 insertions(+)
> >>  create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> >>
> >>diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
> >>index cbd2e13..d6ec839 100644
> >>--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
> >>+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
> >>@@ -35,6 +35,10 @@
> >>  			};
> >>  		};
> >>
> >>+		ahci: sata at 01c18000 {
> >>+			status = "okay";
> >>+		};
> >>+
> >>  		pinctrl at 01c20800 {
> >>  			emac_power_pin_a1000: emac_power_pin at 0 {
> >>  				allwinner,pins = "PH15";
> >>diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> >>index b139ee6..6df237d8 100644
> >>--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> >>+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> >>@@ -12,6 +12,7 @@
> >>
> >>  /dts-v1/;
> >>  /include/ "sun4i-a10.dtsi"
> >>+/include/ "sunxi-ahci-reg.dtsi"
> >>
> >>  / {
> >>  	model = "Cubietech Cubieboard";
> >>@@ -33,6 +34,11 @@
> >>  			};
> >>  		};
> >>
> >>+		ahci: sata at 01c18000 {
> >>+			target-supply = <&reg_ahci_5v>;
> >>+			status = "okay";
> >>+		};
> >>+
> >>  		pinctrl at 01c20800 {
> >>  			led_pins_cubieboard: led_pins at 0 {
> >>  				allwinner,pins = "PH20", "PH21";
> >>diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> >>index 336dbec..454077a 100644
> >>--- a/arch/arm/boot/dts/sun4i-a10.dtsi
> >>+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> >>@@ -338,6 +338,14 @@
> >>  			#size-cells = <0>;
> >>  		};
> >>
> >>+		ahci: sata at 01c18000 {
> >>+			compatible = "allwinner,sun4i-a10-ahci";
> >>+			reg = <0x01c18000 0x1000>;
> >>+			interrupts = <56>;
> >>+			clocks = <&pll6 0>, <&ahb_gates 25>;
> >>+			status = "disabled";
> >>+		};
> >>+
> >>  		intc: interrupt-controller at 01c20400 {
> >>  			compatible = "allwinner,sun4i-ic";
> >>  			reg = <0x01c20400 0x400>;
> >>diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> >>new file mode 100644
> >>index 0000000..7072af1
> >>--- /dev/null
> >>+++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> >>@@ -0,0 +1,36 @@
> >>+/*
> >>+ * sunxi boards sata target power supply common code
> >
> >
> >Since IIRC we have pretty much the same needs for the USB, can't we
> >just drop the SATA specific mention and use it as the common DTSI for
> >the usual regulators?
> 
> On most boards with sata, there will also be 1 or 2 usb regulators,
> so we need differently named regulator nodes for all 3 of ahci,
> usb1 and usb2 vbus. On some boards how ever we may only need the
> usb regulators.

Yes, obviously...

> So if you look in my current personal sunxi-devel tree you will see
> separate dtsi files for both ahci and usb regulators,

And this is precisely what I don't understand. Why do you *need*
different DTSI files. If there's common regulators, that are used on
most boards, fine, create a common regulators files. But why do you
have to create a DTSI to define only one regulator.

> another advantage of having these separate is that the gpio controlling
> the regulator can be pre-populated with the reference design gpio which
> is used in most boards, so that the ahci specific code in the dts
> becomes only the ahci: sata at ... node.

I understand very well the advantages of what having a reference
regulators bring. What I don't understand is the benefits of having
"topics" regulators DTSI.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140222/216c9a7e/attachment.sig>

[PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support

[Hans de Goede]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

On 02/22/2014 06:15 PM, Maxime Ripard wrote:
> On Sat, Feb 22, 2014 at 11:09:25AM +0100, Hans de Goede wrote:
>> Hi Maxime,
>> 
>> On 02/21/2014 07:15 PM, Maxime Ripard wrote:
>>> Hi Hans,
>>> 
>>> On Wed, Feb 19, 2014 at 01:01:59PM +0100, Hans de Goede wrote:
>>>> From: Oliver Schinagl <oliver@schinagl.nl>
>>>> 
>>>> This patch adds sunxi sata support to A10 boards that have such a connector. Some boards also feature a regulator via a GPIO and support for this is also added.
>>>> 
>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- arch/arm/boot/dts/sun4i-a10-a1000.dts      |  4 ++++ arch/arm/boot/dts/sun4i-a10-cubieboard.dts |  6 +++++ arch/arm/boot/dts/sun4i-a10.dtsi           |  8 +++++++ arch/arm/boot/dts/sunxi-ahci-reg.dtsi      | 36 ++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+) create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi
>>>> 
>>>> diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts index cbd2e13..d6ec839 100644 --- a/arch/arm/boot/dts/sun4i-a10-a1000.dts +++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts @@ -35,6 +35,10 @@ }; };
>>>> 
>>>> +		ahci: sata at 01c18000 { +			status = "okay"; +		}; + pinctrl at 01c20800 { emac_power_pin_a1000: emac_power_pin at 0 { allwinner,pins = "PH15"; diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts index b139ee6..6df237d8 100644 --- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts +++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts @@ -12,6 +12,7 @@
>>>> 
>>>> /dts-v1/; /include/ "sun4i-a10.dtsi" +/include/ "sunxi-ahci-reg.dtsi"
>>>> 
>>>> / { model = "Cubietech Cubieboard"; @@ -33,6 +34,11 @@ }; };
>>>> 
>>>> +		ahci: sata at 01c18000 { +			target-supply = <&reg_ahci_5v>; +			status = "okay"; +		}; + pinctrl at 01c20800 { led_pins_cubieboard: led_pins at 0 { allwinner,pins = "PH20", "PH21"; diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi index 336dbec..454077a 100644 --- a/arch/arm/boot/dts/sun4i-a10.dtsi +++ b/arch/arm/boot/dts/sun4i-a10.dtsi @@ -338,6 +338,14 @@ #size-cells = <0>; };
>>>> 
>>>> +		ahci: sata at 01c18000 { +			compatible = "allwinner,sun4i-a10-ahci"; +			reg = <0x01c18000 0x1000>; +			interrupts = <56>; +			clocks = <&pll6 0>, <&ahb_gates 25>; +			status = "disabled"; +		}; + intc: interrupt-controller at 01c20400 { compatible = "allwinner,sun4i-ic"; reg = <0x01c20400 0x400>; diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi new file mode 100644 index 0000000..7072af1 --- /dev/null +++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi @@ -0,0 +1,36 @@ +/* + * sunxi boards sata target power supply common code
>>> 
>>> 
>>> Since IIRC we have pretty much the same needs for the USB, can't we just drop the SATA specific mention and use it as the common DTSI for the usual regulators?
>> 
>> On most boards with sata, there will also be 1 or 2 usb regulators, so we need differently named regulator nodes for all 3 of ahci, usb1 and usb2 vbus. On some boards how ever we may only need the usb regulators.
> 
> Yes, obviously...
> 
>> So if you look in my current personal sunxi-devel tree you will see separate dtsi files for both ahci and usb regulators,
> 
> And this is precisely what I don't understand. Why do you *need* different DTSI files. If there's common regulators, that are used on most boards, fine, create a common regulators files. But why do you have to create a DTSI to define only one regulator.
> 
>> another advantage of having these separate is that the gpio controlling the regulator can be pre-populated with the reference design gpio which is used in most boards, so that the ahci specific code in the dts becomes only the ahci: sata at ... node.
> 
> I understand very well the advantages of what having a reference regulators bring. What I don't understand is the benefits of having "topics" regulators DTSI.

Ok, so let me try to explain:

With topics regulator files, the ahci bits look something like this
for a board using the reference design gpio:

/include/ "sunxi-ahci-reg.dtsi"

...

		ahci: sata at 01c18000 {
			target-supply = <&reg_ahci_5v>;
			status = "okay";
		};

If we put all regulators in one file, then the ahci regulator cannot
be enabled (so it will have status = "disabled) by default since most
boards don't have it, so things would change into:

/include/ "sunxi-common-regulators.dtsi"

...

		ahci: sata at 01c18000 {
			target-supply = <&reg_ahci_5v>;
			status = "okay";
		};

...

        reg_ahci_5v: ahci-5v {
		status = "okay";
        };

Notice the addition of the 2nd node. This is why I ended up doing
2 separate dtsi files for the ahci and for the usb regulators.

To me saying:

/include/ "sunxi-ahci-reg.dtsi"

Makes it clear to the reader that the board has a ahci target-supply
regulator, so enabling it separately seems being overly verbose.

Of course of we change it to:

/include/ "sunxi-common-regulators.dtsi"

Then the verbosity / explicit enabling of various regulators becomes a
good thing, as it is not directly clear what the include does.

But if we do this, then for many boards we end up replacing:

/include/ "sunxi-ahci-reg.dtsi"
/include/ "sun4i-a10-usb-vbus-reg.dtsi"

With:

/include/ "sunxi-common-regulators.dtsi"

        reg_ahci_5v: ahci-5v {
		status = "okay";
        };

        reg_usb1_vbus: usb1-vbus {
		status = "okay";
        };

        reg_usb2_vbus: usb2-vbus {
		status = "okay";
        };

I prefer the shorter version, but I can completely understand if
you prefer the slightly more verbose version, this would also
get rid of having different usb regulator dtsi files for sun4i /
sun5i (as sun5i only has 1 usb host).

I hope this helps explain my reasoning, as said I'm fine with
either way, if you want to change over to a single file +
explicit enabling, let me know and I'll respin the ahci dts
patches.  Note I'm going on vacation for a week starting Monday,
so you likely won't get a new version until next weekend.

Regards,

Hans


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMI9jgACgkQF3VEtJrzE/tkEQCgm1V2Nga+RxsTELQUJxIl2Go3
4dkAnRK0CjK1YEapqN0anp2iltgp7smc
=fBXT
-----END PGP SIGNATURE-----

[PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support

[Maxime Ripard]

Hi Hans,

On Sat, Feb 22, 2014 at 08:10:54PM +0100, Hans de Goede wrote:
> >>> Since IIRC we have pretty much the same needs for the USB, can't
> >>> we just drop the SATA specific mention and use it as the common
> >>> DTSI for the usual regulators?
> >> 
> >> On most boards with sata, there will also be 1 or 2 usb
> >> regulators, so we need differently named regulator nodes for all
> >> 3 of ahci, usb1 and usb2 vbus. On some boards how ever we may
> >> only need the usb regulators.
> > 
> > Yes, obviously...
> > 
> >> So if you look in my current personal sunxi-devel tree you will
> >> see separate dtsi files for both ahci and usb regulators,
> > 
> > And this is precisely what I don't understand. Why do you *need*
> > different DTSI files. If there's common regulators, that are used
> > on most boards, fine, create a common regulators files. But why do
> > you have to create a DTSI to define only one regulator.
> > 
> >> another advantage of having these separate is that the gpio
> >> controlling the regulator can be pre-populated with the reference
> >> design gpio which is used in most boards, so that the ahci
> >> specific code in the dts becomes only the ahci: sata at ... node.
> > 
> > I understand very well the advantages of what having a reference
> > regulators bring. What I don't understand is the benefits of
> > having "topics" regulators DTSI.
> 
> Ok, so let me try to explain:
> 
> With topics regulator files, the ahci bits look something like this
> for a board using the reference design gpio:
> 
> /include/ "sunxi-ahci-reg.dtsi"
> 
> ...
> 
> 		ahci: sata at 01c18000 {
> 			target-supply = <&reg_ahci_5v>;
> 			status = "okay";
> 		};
> 
> If we put all regulators in one file, then the ahci regulator cannot
> be enabled (so it will have status = "disabled) by default since most
> boards don't have it, so things would change into:
> 
> /include/ "sunxi-common-regulators.dtsi"
> 
> ...
> 
> 		ahci: sata at 01c18000 {
> 			target-supply = <&reg_ahci_5v>;
> 			status = "okay";
> 		};
> 
> ...
> 
>         reg_ahci_5v: ahci-5v {
> 		status = "okay";
>         };
> 
> Notice the addition of the 2nd node. This is why I ended up doing
> 2 separate dtsi files for the ahci and for the usb regulators.
> 
> To me saying:
> 
> /include/ "sunxi-ahci-reg.dtsi"
> 
> Makes it clear to the reader that the board has a ahci target-supply
> regulator, so enabling it separately seems being overly verbose.
> 
> Of course of we change it to:
> 
> /include/ "sunxi-common-regulators.dtsi"
> 
> Then the verbosity / explicit enabling of various regulators becomes a
> good thing, as it is not directly clear what the include does.
> 
> But if we do this, then for many boards we end up replacing:
> 
> /include/ "sunxi-ahci-reg.dtsi"
> /include/ "sun4i-a10-usb-vbus-reg.dtsi"
> 
> With:
> 
> /include/ "sunxi-common-regulators.dtsi"
> 
>         reg_ahci_5v: ahci-5v {
> 		status = "okay";
>         };
> 
>         reg_usb1_vbus: usb1-vbus {
> 		status = "okay";
>         };
> 
>         reg_usb2_vbus: usb2-vbus {
> 		status = "okay";
>         };
> 
> I prefer the shorter version, but I can completely understand if
> you prefer the slightly more verbose version, this would also
> get rid of having different usb regulator dtsi files for sun4i /
> sun5i (as sun5i only has 1 usb host).
> 
> I hope this helps explain my reasoning, as said I'm fine with
> either way, if you want to change over to a single file +
> explicit enabling, let me know and I'll respin the ahci dts
> patches.  Note I'm going on vacation for a week starting Monday,
> so you likely won't get a new version until next weekend.

Yes, I strongly prefer the second case. That allows to have a
good-enough degree of factorisation, while not having anything
happening behind the scenes.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140303/b4d9317d/attachment.sig>

[PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support

[Hans de Goede]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

On 03/03/2014 10:33 AM, Maxime Ripard wrote:
> Hi Hans,
> 
> On Sat, Feb 22, 2014 at 08:10:54PM +0100, Hans de Goede wrote:
>>>>> Since IIRC we have pretty much the same needs for the USB, can't we just drop the SATA specific mention and use it as the common DTSI for the usual regulators?
>>>> 
>>>> On most boards with sata, there will also be 1 or 2 usb regulators, so we need differently named regulator nodes for all 3 of ahci, usb1 and usb2 vbus. On some boards how ever we may only need the usb regulators.
>>> 
>>> Yes, obviously...
>>> 
>>>> So if you look in my current personal sunxi-devel tree you will see separate dtsi files for both ahci and usb regulators,
>>> 
>>> And this is precisely what I don't understand. Why do you *need* different DTSI files. If there's common regulators, that are used on most boards, fine, create a common regulators files. But why do you have to create a DTSI to define only one regulator.
>>> 
>>>> another advantage of having these separate is that the gpio controlling the regulator can be pre-populated with the reference design gpio which is used in most boards, so that the ahci specific code in the dts becomes only the ahci: sata at ... node.
>>> 
>>> I understand very well the advantages of what having a reference regulators bring. What I don't understand is the benefits of having "topics" regulators DTSI.
>> 
>> Ok, so let me try to explain:
>> 
>> With topics regulator files, the ahci bits look something like this for a board using the reference design gpio:
>> 
>> /include/ "sunxi-ahci-reg.dtsi"
>> 
>> ...
>> 
>> ahci: sata at 01c18000 { target-supply = <&reg_ahci_5v>; status = "okay"; };
>> 
>> If we put all regulators in one file, then the ahci regulator cannot be enabled (so it will have status = "disabled) by default since most boards don't have it, so things would change into:
>> 
>> /include/ "sunxi-common-regulators.dtsi"
>> 
>> ...
>> 
>> ahci: sata at 01c18000 { target-supply = <&reg_ahci_5v>; status = "okay"; };
>> 
>> ...
>> 
>> reg_ahci_5v: ahci-5v { status = "okay"; };
>> 
>> Notice the addition of the 2nd node. This is why I ended up doing 2 separate dtsi files for the ahci and for the usb regulators.
>> 
>> To me saying:
>> 
>> /include/ "sunxi-ahci-reg.dtsi"
>> 
>> Makes it clear to the reader that the board has a ahci target-supply regulator, so enabling it separately seems being overly verbose.
>> 
>> Of course of we change it to:
>> 
>> /include/ "sunxi-common-regulators.dtsi"
>> 
>> Then the verbosity / explicit enabling of various regulators becomes a good thing, as it is not directly clear what the include does.
>> 
>> But if we do this, then for many boards we end up replacing:
>> 
>> /include/ "sunxi-ahci-reg.dtsi" /include/ "sun4i-a10-usb-vbus-reg.dtsi"
>> 
>> With:
>> 
>> /include/ "sunxi-common-regulators.dtsi"
>> 
>> reg_ahci_5v: ahci-5v { status = "okay"; };
>> 
>> reg_usb1_vbus: usb1-vbus { status = "okay"; };
>> 
>> reg_usb2_vbus: usb2-vbus { status = "okay"; };
>> 
>> I prefer the shorter version, but I can completely understand if you prefer the slightly more verbose version, this would also get rid of having different usb regulator dtsi files for sun4i / sun5i (as sun5i only has 1 usb host).
>> 
>> I hope this helps explain my reasoning, as said I'm fine with either way, if you want to change over to a single file + explicit enabling, let me know and I'll respin the ahci dts patches.  Note I'm going on vacation for a week starting Monday, so you likely won't get a new version until next weekend.
> 
> Yes, I strongly prefer the second case. That allows to have a good-enough degree of factorisation, while not having anything happening behind the scenes.

Good, note I've already assumed as much and send a new series
which already has moved over to 1 common regulators dtsi,
with all regulators disabled by default + explicit enabling
in the board dts files.

Regards,

Hans
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMUVHEACgkQF3VEtJrzE/vIyQCfVgZNpu7YEk/pf7QiE90C6Cuj
Dv8AoJq8fAEwTURGg5WF2FwMqZBPD1iS
=Vtd+
-----END PGP SIGNATURE-----

[PATCH v6 18/18] ARM: sun7i: dt: Add ahci / sata support

[Hans de Goede]

This patch adds sunxi sata support to A20 boards that have such a connector.
Some boards also feature a regulator via a GPIO and support for this is also
added.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts     |  6 ++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 18 ++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts |  6 ++++++
 arch/arm/boot/dts/sun7i-a20.dtsi                |  8 ++++++++
 4 files changed, 38 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index 7bf4935..07823c2 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -13,12 +13,18 @@
 
 /dts-v1/;
 /include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
 
 / {
 	model = "Cubietech Cubieboard2";
 	compatible = "cubietech,cubieboard2", "allwinner,sun7i-a20";
 
 	soc at 01c00000 {
+		ahci: sata at 01c18000 {
+			target-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
 			led_pins_cubieboard2: led_pins at 0 {
 				allwinner,pins = "PH20", "PH21";
diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index 025ce52..403bd2e 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -13,13 +13,26 @@
 
 /dts-v1/;
 /include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
 
 / {
 	model = "Cubietech Cubietruck";
 	compatible = "cubietech,cubietruck", "allwinner,sun7i-a20";
 
 	soc at 01c00000 {
+		ahci: sata at 01c18000 {
+			target-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
+			ahci_pwr_pin_cubietruck: ahci_pwr_pin at 1 {
+				allwinner,pins = "PH12";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			led_pins_cubietruck: led_pins at 0 {
 				allwinner,pins = "PH7", "PH11", "PH20", "PH21";
 				allwinner,function = "gpio_out";
@@ -90,4 +103,9 @@
 			gpios = <&pio 7 7 0>;
 		};
 	};
+
+	reg_ahci_5v: ahci-5v {
+		pinctrl-0 = <&ahci_pwr_pin_cubietruck>;
+		gpio = <&pio 7 12 0>;
+	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
index b02a796..d5c6799 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
@@ -13,12 +13,18 @@
 
 /dts-v1/;
 /include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
 
 / {
 	model = "Olimex A20-Olinuxino Micro";
 	compatible = "olimex,a20-olinuxino-micro", "allwinner,sun7i-a20";
 
 	soc at 01c00000 {
+		ahci: sata at 01c18000 {
+			target-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
 			led_pins_olinuxino: led_pins at 0 {
 				allwinner,pins = "PH2";
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index daaafd0..3385994 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -392,6 +392,14 @@
 			#size-cells = <0>;
 		};
 
+		ahci: sata at 01c18000 {
+			compatible = "allwinner,sun4i-a10-ahci";
+			reg = <0x01c18000 0x1000>;
+			interrupts = <0 56 4>;
+			clocks = <&pll6 0>, <&ahb_gates 25>;
+			status = "disabled";
+		};
+
 		pio: pinctrl at 01c20800 {
 			compatible = "allwinner,sun7i-a20-pinctrl";
 			reg = <0x01c20800 0x400>;
-- 
1.8.5.3

[PATCH v6 00/18] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver

[Tejun Heo]

Hello,

On Wed, Feb 19, 2014 at 01:01:42PM +0100, Hans de Goede wrote:
> Here is v6 of my patchset for adding ahci-sunxi support. This has been
> tested with Allwinner A10, Allwinner A20 and Freeware imx6x SoCs, including
> suspend / resume. Note that since my last revision the ahci_imx driver has
> also grown imx53 sata support, it would be good if some-one could test that
> with this series.

Stopping review here.  I really like where it's headed in general and
most review points, except for lack of devres usage, are rather
cosmetic.  I'm not completely decided yet whether we can defer devres
until later or should go for it in the first round.

Thanks.

-- 
tejun

[PATCH v6 00/18] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver

[Hans de Goede]

Hi,

On 02/19/2014 04:02 PM, Tejun Heo wrote:
> Hello,
> 
> On Wed, Feb 19, 2014 at 01:01:42PM +0100, Hans de Goede wrote:
>> Here is v6 of my patchset for adding ahci-sunxi support. This has been
>> tested with Allwinner A10, Allwinner A20 and Freeware imx6x SoCs, including
>> suspend / resume. Note that since my last revision the ahci_imx driver has
>> also grown imx53 sata support, it would be good if some-one could test that
>> with this series.
> 
> Stopping review here.  I really like where it's headed in general and
> most review points,

Thanks for the review. I'll respin the patchset taking the various remarks into
account. I hope to have a new version ready at the end of the coming weekend
at the latest.

> except for lack of devres usage, are rather
> cosmetic.  I'm not completely decided yet whether we can defer devres
> until later or should go for it in the first round.

The devres usage are really 2 different issues:

1) There is the issue that we're getting clocks by index (this is by design as
clk-names are not generic), but there is no devm_get_clk_by_index (or some
such), this is a shortcoming of the clk-core, which needs a separate patch
to fix. I would rather not delay this patch-set based on getting a patch
into another subsys.

Note that even with devm_get_clk_by_index we can unfortunately not get rid of
ahci_platform_put_resources() as in Roger's follow-up patches it gets used for
putting some runtime pm related resources too.

I guess this argues for simply turning ahci_platform_get_resources into a
devm_ahci_platform_get_resources doing its own devm handling, and then
making ahci_platform_put_resources a private function called by the devm
framework. If you agree I can do that for the next patch-set.

2) In some comments you also seem to want devm variants of enable / disable
resources, or at least have ahci_platform_put_resources do the disable
automatically. The problem is that most of the functions called here need to
be balanced, they increment / decrement usage counters in the clk / regulator
subsystems, so we cannot simply unconditional do an ahci_platform_disable_resources
in ahci_platform_put_resources

Doing the disable automatically requires tracking the enable state, and doing this
per resource, since the whole idea of having a separate ahci_platform_enable_resources
is that some drivers may want to override its behavior doing things in a different
order.

To ensure proper tracking we would then need to offer ahci_platform_enable_regulator,
etc. functions, and ensure that all drivers using the ahci_platform framework
always go through these, rather then calling directly into the relevant framework.

This is all doable, and I'm not against doing this but before spending a couple of
hours coding this all up, I would like to hear back from you whether you would like
to see this, or would rather keep things as is.

Regards,

Hans

[PATCH v6 00/18] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver

[Tejun Heo]

Hello, Hans.

On Wed, Feb 19, 2014 at 04:26:43PM +0100, Hans de Goede wrote:
> The devres usage are really 2 different issues:
> 
> 1) There is the issue that we're getting clocks by index (this is by design as
> clk-names are not generic), but there is no devm_get_clk_by_index (or some
> such), this is a shortcoming of the clk-core, which needs a separate patch
> to fix. I would rather not delay this patch-set based on getting a patch
> into another subsys.

Yes, that'd be my usual preference too.  The only reason I'm hesitant
is because nobody seems to be too interested in long term aspect of
the code base and the only leverage I have seems to be rejecting
drivers until things become right.

> Note that even with devm_get_clk_by_index we can unfortunately not get rid of
> ahci_platform_put_resources() as in Roger's follow-up patches it gets used for
> putting some runtime pm related resources too.
> 
> I guess this argues for simply turning ahci_platform_get_resources into a
> devm_ahci_platform_get_resources doing its own devm handling, and then
> making ahci_platform_put_resources a private function called by the devm
> framework. If you agree I can do that for the next patch-set.

Note that libata core already does something similar.  Please take a
look at ata_host_alloc() for details.  You don't really need to create
a separate devm_ prefixed variant.  Just making the function register
devres automatically should be enough.  If this is too much work, I'm
okay with deferring it until later if you promise to do it soonish.

> 2) In some comments you also seem to want devm variants of enable / disable
> resources, or at least have ahci_platform_put_resources do the disable
> automatically. The problem is that most of the functions called here need to
> be balanced, they increment / decrement usage counters in the clk / regulator
> subsystems, so we cannot simply unconditional do an ahci_platform_disable_resources
> in ahci_platform_put_resources
> 
> Doing the disable automatically requires tracking the enable state, and doing this
> per resource, since the whole idea of having a separate ahci_platform_enable_resources
> is that some drivers may want to override its behavior doing things in a different
> order.
> 
> To ensure proper tracking we would then need to offer ahci_platform_enable_regulator,
> etc. functions, and ensure that all drivers using the ahci_platform framework
> always go through these, rather then calling directly into the relevant framework.
> 
> This is all doable, and I'm not against doing this but before spending a couple of
> hours coding this all up, I would like to hear back from you whether you would like
> to see this, or would rather keep things as is.

Yes, in principle, I want every resource used by ahci_platform to be
devres managed.  I *think* devres should be flexible enough to handle
what you're describing (each devres resource can have data for state
tracking and devres resources can be looked up and modified, so the
different orders should be okay) but if not I'd be happy to help
extend it so that it can.

I'm not sure how many more ahci_platform drivers we'll get but the
rate seems pretty high in recent months, so I think it's worthwhile to
provide full devres coverage even if that complicates ahci_platform a
bit if it can simplify leaf drivers.  Again, if you commit to doing it
in the foreseeable future, I'm okay with proceeding as-is, but it
needs to happen one way or another.

Thanks.

-- 
tejun

[PATCH v6 00/18] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver

[Hans de Goede]

Hi,

On 02/19/2014 05:25 PM, Tejun Heo wrote:
> Hello, Hans.
> 
> On Wed, Feb 19, 2014 at 04:26:43PM +0100, Hans de Goede wrote:
>> The devres usage are really 2 different issues:
>>
>> 1) There is the issue that we're getting clocks by index (this is by design as
>> clk-names are not generic), but there is no devm_get_clk_by_index (or some
>> such), this is a shortcoming of the clk-core, which needs a separate patch
>> to fix. I would rather not delay this patch-set based on getting a patch
>> into another subsys.
> 
> Yes, that'd be my usual preference too.  The only reason I'm hesitant
> is because nobody seems to be too interested in long term aspect of
> the code base and the only leverage I have seems to be rejecting
> drivers until things become right.
> 
>> Note that even with devm_get_clk_by_index we can unfortunately not get rid of
>> ahci_platform_put_resources() as in Roger's follow-up patches it gets used for
>> putting some runtime pm related resources too.
>>
>> I guess this argues for simply turning ahci_platform_get_resources into a
>> devm_ahci_platform_get_resources doing its own devm handling, and then
>> making ahci_platform_put_resources a private function called by the devm
>> framework. If you agree I can do that for the next patch-set.
> 
> Note that libata core already does something similar.  Please take a
> look at ata_host_alloc() for details.  You don't really need to create
> a separate devm_ prefixed variant.  Just making the function register
> devres automatically should be enough.  If this is too much work, I'm
> okay with deferring it until later if you promise to do it soonish.
> 

Ok, I'll try to do this for the next revision.

>> 2) In some comments you also seem to want devm variants of enable / disable
>> resources, or at least have ahci_platform_put_resources do the disable
>> automatically. The problem is that most of the functions called here need to
>> be balanced, they increment / decrement usage counters in the clk / regulator
>> subsystems, so we cannot simply unconditional do an ahci_platform_disable_resources
>> in ahci_platform_put_resources
>>
>> Doing the disable automatically requires tracking the enable state, and doing this
>> per resource, since the whole idea of having a separate ahci_platform_enable_resources
>> is that some drivers may want to override its behavior doing things in a different
>> order.
>>
>> To ensure proper tracking we would then need to offer ahci_platform_enable_regulator,
>> etc. functions, and ensure that all drivers using the ahci_platform framework
>> always go through these, rather then calling directly into the relevant framework.
>>
>> This is all doable, and I'm not against doing this but before spending a couple of
>> hours coding this all up, I would like to hear back from you whether you would like
>> to see this, or would rather keep things as is.
> 
> Yes, in principle, I want every resource used by ahci_platform to be
> devres managed.  I *think* devres should be flexible enough to handle
> what you're describing (each devres resource can have data for state
> tracking and devres resources can be looked up and modified, so the
> different orders should be okay) but if not I'd be happy to help
> extend it so that it can.

Most of the resources are already devres managed (I use devm functions
to get them), the problem is not in freeing our reference to the resources,
the problem is that we've sequences like this:

devm_get_foo
enable_foo
disable_foo
(automatic release foo)

Where enable / disable can be done repeatedly (ie each suspend / resume).

>From your review comments, I take it that you want the final disable_foo
on driver release to happen automatically.

My preference for this would be to extend the devres tracking already present
in the relevant subsystems to keep track of the enable count done through a
specific reference, to allow automatic disable (if needed) on release.

But thinking more about this, I think that doing this automatically is a bad
idea, because then we fixate the shutdown sequence to a certain order (the
order in which we did the _get_foo for the resources) and the correct order may
be device specific.

So TL;DR: Yes to making things so that ahci_platform_put_resources gets done
automatically, no to automating the disable calls.

If I don't hear back from you, then I'll respin the patch-set assuming that
you agree to the above.

Regards,

Hans

[PATCH v6 00/18] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver

[Tejun Heo]

On Wed, Feb 19, 2014 at 06:18:09PM +0100, Hans de Goede wrote:
> Most of the resources are already devres managed (I use devm functions
> to get them), the problem is not in freeing our reference to the resources,
> the problem is that we've sequences like this:
> 
> devm_get_foo
> enable_foo
> disable_foo
> (automatic release foo)
> 
> Where enable / disable can be done repeatedly (ie each suspend / resume).
> 
> From your review comments, I take it that you want the final disable_foo
> on driver release to happen automatically.
> 
> My preference for this would be to extend the devres tracking already present
> in the relevant subsystems to keep track of the enable count done through a
> specific reference, to allow automatic disable (if needed) on release.
> 
> But thinking more about this, I think that doing this automatically is a bad
> idea, because then we fixate the shutdown sequence to a certain order (the
> order in which we did the _get_foo for the resources) and the correct order may
> be device specific.
> 
> So TL;DR: Yes to making things so that ahci_platform_put_resources gets done
> automatically, no to automating the disable calls.
> 
> If I don't hear back from you, then I'll respin the patch-set assuming that
> you agree to the above.

Yeah, sounds good enough to me.

Thanks.

-- 
tejun