[PATCH v2 0/4] ARM: sunxi: Add ahci-sunxi driver for Allwinner SoCs sata

[PATCH v2 0/4] ARM: sunxi: Add ahci-sunxi driver for Allwinner SoCs sata

[Hans de Goede]

Hi Tejun et al,

Here is the second version of the ahci-sunxi driver. Since Oliver is a bit low
on time atm, I've taken over the efforts to upstream this for now.

v1 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 . This was rightfully frowned upon, so we've gone
looking for a better / cleaner solution.

The option of turning ahci_platform.c into a generic re-usable library to be
shared between different ahci platform drivers was considered and rejected.

Almost all functionality is already shared through libahci / ata-core,
ahci_platform.c really is just a few lines of glue-code generalized to be
shared for the simply ahci platform device case.

Modifying ahci_platform.c to be generic enough to cleanly handle more
complex platform ahci devices, results in much more complex code, which likely
will still need to be modified each time a new exotic platform device shows
up. There simply is not enough common code to share there, and making it
generic enough would grow the code to be larger then simply having stand-alone
platform drivers using libahci directly.

IOW I believe that the "last mile" of glue code  simply cannot be efficiently
generalized.

So I've decided avoiding the ahci_imx.c ugliness is best done by refactoring
ahci_sunxi.c into a stand-alone platform driver, like ie sata_highbank.c.

To put this in numbers, ahci_platform.c is 353 lines, ahci_sunxi.c using
the ahci_imx.c approach (which was using ahci_platform.c as a lib) was 305
lines + 12 new lines in ahci_platform.c. ahci_sunxi.c as a standalone driver
is 351 lines. And the old ahci_sunxi.c was missing proper suspend / resume
support.

Please review and if there are no remark add patch 1 + 2 to your tree for
Linus. The dts bits will be merged through Maxime Ripard's tree.

Thanks & Regards,

Hans

[PATCH v2 0/4] ARM: sunxi: Add ahci-sunxi driver for Allwinner SoCs sata

[Hans de Goede]

Hi Tejun et al,

Here is the second version of the ahci-sunxi driver. Since Oliver is a bit low
on time atm, I've taken over the efforts to upstream this for now.

v1 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 . This was rightfully frowned upon, so we've gone
looking for a better / cleaner solution.

The option of turning ahci_platform.c into a generic re-usable library to be
shared between different ahci platform drivers was considered and rejected.

Almost all functionality is already shared through libahci / ata-core,
ahci_platform.c really is just a few lines of glue-code generalized to be
shared for the simply ahci platform device case.

Modifying ahci_platform.c to be generic enough to cleanly handle more
complex platform ahci devices, results in much more complex code, which likely
will still need to be modified each time a new exotic platform device shows
up. There simply is not enough common code to share there, and making it
generic enough would grow the code to be larger then simply having stand-alone
platform drivers using libahci directly.

IOW I believe that the "last mile" of glue code  simply cannot be efficiently
generalized.

So I've decided avoiding the ahci_imx.c ugliness is best done by refactoring
ahci_sunxi.c into a stand-alone platform driver, like ie sata_highbank.c.

To put this in numbers, ahci_platform.c is 353 lines, ahci_sunxi.c using
the ahci_imx.c approach (which was using ahci_platform.c as a lib) was 305
lines + 12 new lines in ahci_platform.c. ahci_sunxi.c as a standalone driver
is 351 lines. And the old ahci_sunxi.c was missing proper suspend / resume
support.

Please review and if there are no remark add patch 1 + 2 to your tree for
Linus. The dts bits will be merged through Maxime Ripard's tree.

Thanks & Regards,

Hans

[PATCH v2 1/4] libahci: Add a pre ahci_start_engine hook

[Hans de Goede]

From: Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>

Allwinner A10 and A20 ARM SoCs have an AHCI sata controller which need 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 adds a pre ahci_start_engine hook for use by the Allwinner AHCI
driver (and potentially other drivers in the future).

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/ata/ahci.h    | 2 ++
 drivers/ata/libahci.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efd..1bba479 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 pre ahci_start_engine hook */
+	void			(*pre_start_engine)(struct ata_port *ap);
 };
 
 extern int ahci_ignore_sss;
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c482f8c..780e9df 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -568,8 +568,12 @@ static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
 void ahci_start_engine(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	u32 tmp;
 
+	if (hpriv->pre_start_engine)
+		hpriv->pre_start_engine(ap);
+
 	/* start DMA */
 	tmp = readl(port_mmio + PORT_CMD);
 	tmp |= PORT_CMD_START;
-- 
1.8.4.2

[PATCH v2 1/4] libahci: Add a pre ahci_start_engine hook

[Hans de Goede]

From: Oliver Schinagl <oliver@schinagl.nl>

Allwinner A10 and A20 ARM SoCs have an AHCI sata controller which need 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 adds a pre ahci_start_engine hook 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.h    | 2 ++
 drivers/ata/libahci.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efd..1bba479 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 pre ahci_start_engine hook */
+	void			(*pre_start_engine)(struct ata_port *ap);
 };
 
 extern int ahci_ignore_sss;
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c482f8c..780e9df 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -568,8 +568,12 @@ static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
 void ahci_start_engine(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	u32 tmp;
 
+	if (hpriv->pre_start_engine)
+		hpriv->pre_start_engine(ap);
+
 	/* start DMA */
 	tmp = readl(port_mmio + PORT_CMD);
 	tmp |= PORT_CMD_START;
-- 
1.8.4.2

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

From: Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>

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

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 .

Given that almost all functionality already is shared through libahci /
ata-core, and that ahci_platform.c cannot cleanly handle somewhat more complex
platform specific ahci cases, such as the sunxi case, it was refactored into
a stand-alone platform driver by Hans de Goede.

Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/ata/ahci-sunxi.txt         |  24 ++
 drivers/ata/Kconfig                                |   9 +
 drivers/ata/Makefile                               |   1 +
 drivers/ata/ahci_sunxi.c                           | 349 +++++++++++++++++++++
 4 files changed, 383 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-sunxi.txt
 create mode 100644 drivers/ata/ahci_sunxi.c

diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
new file mode 100644
index 0000000..0792fa5
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
@@ -0,0 +1,24 @@
+Allwinner SUNXI AHCI SATA Controller
+
+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 "allwinner,sun4i-a10-ahci"
+- reg		: <registers mapping>
+- interrupts	: <interrupt mapping for AHCI IRQ>
+- clocks	: clocks for ACHI
+- clock-names	: clock names for AHCI
+
+Optional properties:
+- pwr-supply	: regulator to control the power supply GPIO
+
+Example:
+	ahci@01c18000 {
+		compatible = "allwinner,sun4i-a10-ahci";
+		reg = <0x01c18000 0x1000>;
+		interrupts = <0 56 1>;
+		clocks = <&ahb_gates 25>, <&pll6 0>;
+		clock-names = "ahb_sata", "pll6_sata";
+		pwr-supply = <&reg_ahci_5v>;
+	};
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 4e73772..3c80bbe 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
+	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..956abc3 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 libahci.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..22d3972
--- /dev/null
+++ b/drivers/ata/ahci_sunxi.c
@@ -0,0 +1,349 @@
+/*
+ * Allwinner sunxi AHCI SATA platform driver
+ * Copyright 2013 Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
+ * Copyright 2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+ *
+ * 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-0TFLnhJekD6UEPyfVivIlAC/G2K4zDHf@public.gmane.org>
+ *
+ * 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/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
+
+struct sunxi_ahci {
+	struct ahci_host_priv hpriv;
+	struct regulator *pwr;
+	struct clk *sata_clk;
+	struct clk *ahb_clk;
+};
+
+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 sunxi_ahci_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 = 0x100000;
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
+	} while (--timeout && (reg_val != 0x2));
+	if (!timeout) {
+		dev_err(dev, "PHY power up failed.\n");
+		return -EIO;
+	}
+
+	sunxi_setbits(reg_base + AHCI_PHYCS2R, (0x1 << 24));
+
+	timeout = 0x100000;
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS2R, 0x1, 24);
+	} while (--timeout && reg_val);
+	if (!timeout) {
+		dev_err(dev, "PHY calibration failed.\n");
+		return -EIO;
+	}
+	mdelay(15);
+
+	writel(0x7, reg_base + AHCI_RWCR);
+
+	return 0;
+}
+
+void sunxi_ahci_pre_start_engine(struct ata_port *ap)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+
+	/* Setup DMA before DMA start */
+	sunxi_clrsetbits(hpriv->mmio + AHCI_P0DMACR, 0x0000ff00, 0x00004400);
+}
+
+static int sunxi_ahci_enable_clks(struct sunxi_ahci *ahci)
+{
+	int ret;
+
+	ret = clk_prepare_enable(ahci->sata_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(ahci->ahb_clk);
+	if (ret)
+		clk_disable_unprepare(ahci->sata_clk);
+
+	return ret;
+}
+
+static void sunxi_ahci_disable_clks(struct sunxi_ahci *ahci)
+{
+	clk_disable_unprepare(ahci->ahb_clk);
+	clk_disable_unprepare(ahci->sata_clk);
+}
+
+static void sunxi_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct sunxi_ahci *ahci = hpriv->plat_data;
+
+	if (!IS_ERR(ahci->pwr))
+		regulator_disable(ahci->pwr);
+
+	sunxi_ahci_disable_clks(ahci);
+}
+
+static struct ata_port_operations sunxi_ahci_platform_ops = {
+	.inherits	= &ahci_ops,
+	.host_stop	= sunxi_ahci_host_stop,
+};
+
+static const struct ata_port_info sunxiahci_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	= &sunxi_ahci_platform_ops,
+};
+
+static struct scsi_host_template sunxi_ahci_platform_sht = {
+	AHCI_SHT("sunxi_ahci"),
+};
+
+static int sunxi_ahci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct ata_port_info *ppi[] = { &sunxiahci_port_info, NULL };
+	struct sunxi_ahci *ahci;
+	struct ata_host *host;
+	int ret;
+
+	ahci = devm_kzalloc(&pdev->dev, sizeof(*ahci), GFP_KERNEL);
+	if (!ahci)
+		return -ENOMEM;
+
+	ahci->pwr = devm_regulator_get_optional(dev, "pwr");
+	if (IS_ERR(ahci->pwr) && PTR_ERR(ahci->pwr) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	host = ata_host_alloc_pinfo(dev, ppi, 1);
+	if (!host)
+		return -ENOMEM;
+
+	host->private_data = &ahci->hpriv;
+	host->flags |= ATA_HOST_PARALLEL_SCAN;
+
+	ahci->hpriv.flags = (unsigned long)ppi[0]->private_data;
+	ahci->hpriv.plat_data = ahci;
+	ahci->hpriv.pre_start_engine = sunxi_ahci_pre_start_engine;
+	ahci->hpriv.mmio = devm_ioremap_resource(dev,
+			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
+	if (IS_ERR(ahci->hpriv.mmio))
+		return PTR_ERR(ahci->hpriv.mmio);
+
+	ahci->ahb_clk = devm_clk_get(&pdev->dev, "ahb_sata");
+	if (IS_ERR(ahci->ahb_clk))
+		return PTR_ERR(ahci->ahb_clk);
+
+	ahci->sata_clk = devm_clk_get(&pdev->dev, "pll6_sata");
+	if (IS_ERR(ahci->sata_clk))
+		return PTR_ERR(ahci->sata_clk);
+
+	ret = sunxi_ahci_enable_clks(ahci);
+	if (ret)
+		return ret;
+
+	if (!IS_ERR(ahci->pwr)) {
+		ret = regulator_enable(ahci->pwr);
+		if (ret) {
+			sunxi_ahci_disable_clks(ahci);
+			return ret;
+		}
+	}
+
+	ret = sunxi_ahci_phy_init(dev, ahci->hpriv.mmio);
+	if (ret) {
+		sunxi_ahci_host_stop(host);
+		return ret;
+	}
+
+	ahci_save_initial_config(dev, &ahci->hpriv, 0, 0);
+
+	ret = ahci_reset_controller(host);
+	if (ret) {
+		sunxi_ahci_host_stop(host);
+		return ret;
+	}
+
+	ahci_init_controller(host);
+	ahci_print_info(host, "sunxi");
+
+	ret = ata_host_activate(host, platform_get_irq(pdev, 0),
+				ahci_interrupt, 0, &sunxi_ahci_platform_sht);
+	if (ret)
+		sunxi_ahci_host_stop(host);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int sunxi_ahci_susp(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct sunxi_ahci *ahci = hpriv->plat_data;
+	int ret;
+
+	/*
+	 * AHCI spec rev1.1 section 8.3.3:
+	 * Software must disable interrupts prior to requesting a
+	 * transition of the HBA to D3 state.
+	 */
+	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
+
+	ret = ata_host_suspend(host, PMSG_SUSPEND);
+	if (ret)
+		return ret;
+
+	sunxi_ahci_disable_clks(ahci);
+
+	return 0;
+}
+
+static int sunxi_ahci_resume(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct sunxi_ahci *ahci = hpriv->plat_data;
+	int ret;
+
+	ret = sunxi_ahci_enable_clks(ahci);
+	if (ret)
+		return ret;
+
+	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
+		ret = ahci_reset_controller(host);
+		if (ret)
+			return ret;
+
+		ahci_init_controller(host);
+	}
+
+	ata_host_resume(host);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(sunxi_ahci_pmo, sunxi_ahci_susp, sunxi_ahci_resume);
+
+static const struct of_device_id sunxi_ahci_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-ahci" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, sunxi_ahci_of_match);
+
+static struct platform_driver sunxi_ahci_driver = {
+	.probe = sunxi_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "sunxi-ahci",
+		.owner = THIS_MODULE,
+		.of_match_table = sunxi_ahci_of_match,
+		.pm = &sunxi_ahci_pmo,
+	},
+};
+module_platform_driver(sunxi_ahci_driver);
+
+MODULE_DESCRIPTION("Allwinner sunxi AHCI SATA platform driver");
+MODULE_AUTHOR("Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>");
+MODULE_LICENSE("GPL");
-- 
1.8.4.2

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

From: Oliver Schinagl <oliver@schinagl.nl>

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

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 .

Given that almost all functionality already is shared through libahci /
ata-core, and that ahci_platform.c cannot cleanly handle somewhat more complex
platform specific ahci cases, such as the sunxi case, it was refactored into
a stand-alone platform driver by Hans de Goede.

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

diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
new file mode 100644
index 0000000..0792fa5
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
@@ -0,0 +1,24 @@
+Allwinner SUNXI AHCI SATA Controller
+
+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 "allwinner,sun4i-a10-ahci"
+- reg		: <registers mapping>
+- interrupts	: <interrupt mapping for AHCI IRQ>
+- clocks	: clocks for ACHI
+- clock-names	: clock names for AHCI
+
+Optional properties:
+- pwr-supply	: regulator to control the power supply GPIO
+
+Example:
+	ahci at 01c18000 {
+		compatible = "allwinner,sun4i-a10-ahci";
+		reg = <0x01c18000 0x1000>;
+		interrupts = <0 56 1>;
+		clocks = <&ahb_gates 25>, <&pll6 0>;
+		clock-names = "ahb_sata", "pll6_sata";
+		pwr-supply = <&reg_ahci_5v>;
+	};
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 4e73772..3c80bbe 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
+	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..956abc3 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 libahci.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..22d3972
--- /dev/null
+++ b/drivers/ata/ahci_sunxi.c
@@ -0,0 +1,349 @@
+/*
+ * 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/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
+
+struct sunxi_ahci {
+	struct ahci_host_priv hpriv;
+	struct regulator *pwr;
+	struct clk *sata_clk;
+	struct clk *ahb_clk;
+};
+
+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 sunxi_ahci_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 = 0x100000;
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
+	} while (--timeout && (reg_val != 0x2));
+	if (!timeout) {
+		dev_err(dev, "PHY power up failed.\n");
+		return -EIO;
+	}
+
+	sunxi_setbits(reg_base + AHCI_PHYCS2R, (0x1 << 24));
+
+	timeout = 0x100000;
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS2R, 0x1, 24);
+	} while (--timeout && reg_val);
+	if (!timeout) {
+		dev_err(dev, "PHY calibration failed.\n");
+		return -EIO;
+	}
+	mdelay(15);
+
+	writel(0x7, reg_base + AHCI_RWCR);
+
+	return 0;
+}
+
+void sunxi_ahci_pre_start_engine(struct ata_port *ap)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+
+	/* Setup DMA before DMA start */
+	sunxi_clrsetbits(hpriv->mmio + AHCI_P0DMACR, 0x0000ff00, 0x00004400);
+}
+
+static int sunxi_ahci_enable_clks(struct sunxi_ahci *ahci)
+{
+	int ret;
+
+	ret = clk_prepare_enable(ahci->sata_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(ahci->ahb_clk);
+	if (ret)
+		clk_disable_unprepare(ahci->sata_clk);
+
+	return ret;
+}
+
+static void sunxi_ahci_disable_clks(struct sunxi_ahci *ahci)
+{
+	clk_disable_unprepare(ahci->ahb_clk);
+	clk_disable_unprepare(ahci->sata_clk);
+}
+
+static void sunxi_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct sunxi_ahci *ahci = hpriv->plat_data;
+
+	if (!IS_ERR(ahci->pwr))
+		regulator_disable(ahci->pwr);
+
+	sunxi_ahci_disable_clks(ahci);
+}
+
+static struct ata_port_operations sunxi_ahci_platform_ops = {
+	.inherits	= &ahci_ops,
+	.host_stop	= sunxi_ahci_host_stop,
+};
+
+static const struct ata_port_info sunxiahci_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	= &sunxi_ahci_platform_ops,
+};
+
+static struct scsi_host_template sunxi_ahci_platform_sht = {
+	AHCI_SHT("sunxi_ahci"),
+};
+
+static int sunxi_ahci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct ata_port_info *ppi[] = { &sunxiahci_port_info, NULL };
+	struct sunxi_ahci *ahci;
+	struct ata_host *host;
+	int ret;
+
+	ahci = devm_kzalloc(&pdev->dev, sizeof(*ahci), GFP_KERNEL);
+	if (!ahci)
+		return -ENOMEM;
+
+	ahci->pwr = devm_regulator_get_optional(dev, "pwr");
+	if (IS_ERR(ahci->pwr) && PTR_ERR(ahci->pwr) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	host = ata_host_alloc_pinfo(dev, ppi, 1);
+	if (!host)
+		return -ENOMEM;
+
+	host->private_data = &ahci->hpriv;
+	host->flags |= ATA_HOST_PARALLEL_SCAN;
+
+	ahci->hpriv.flags = (unsigned long)ppi[0]->private_data;
+	ahci->hpriv.plat_data = ahci;
+	ahci->hpriv.pre_start_engine = sunxi_ahci_pre_start_engine;
+	ahci->hpriv.mmio = devm_ioremap_resource(dev,
+			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
+	if (IS_ERR(ahci->hpriv.mmio))
+		return PTR_ERR(ahci->hpriv.mmio);
+
+	ahci->ahb_clk = devm_clk_get(&pdev->dev, "ahb_sata");
+	if (IS_ERR(ahci->ahb_clk))
+		return PTR_ERR(ahci->ahb_clk);
+
+	ahci->sata_clk = devm_clk_get(&pdev->dev, "pll6_sata");
+	if (IS_ERR(ahci->sata_clk))
+		return PTR_ERR(ahci->sata_clk);
+
+	ret = sunxi_ahci_enable_clks(ahci);
+	if (ret)
+		return ret;
+
+	if (!IS_ERR(ahci->pwr)) {
+		ret = regulator_enable(ahci->pwr);
+		if (ret) {
+			sunxi_ahci_disable_clks(ahci);
+			return ret;
+		}
+	}
+
+	ret = sunxi_ahci_phy_init(dev, ahci->hpriv.mmio);
+	if (ret) {
+		sunxi_ahci_host_stop(host);
+		return ret;
+	}
+
+	ahci_save_initial_config(dev, &ahci->hpriv, 0, 0);
+
+	ret = ahci_reset_controller(host);
+	if (ret) {
+		sunxi_ahci_host_stop(host);
+		return ret;
+	}
+
+	ahci_init_controller(host);
+	ahci_print_info(host, "sunxi");
+
+	ret = ata_host_activate(host, platform_get_irq(pdev, 0),
+				ahci_interrupt, 0, &sunxi_ahci_platform_sht);
+	if (ret)
+		sunxi_ahci_host_stop(host);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int sunxi_ahci_susp(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct sunxi_ahci *ahci = hpriv->plat_data;
+	int ret;
+
+	/*
+	 * AHCI spec rev1.1 section 8.3.3:
+	 * Software must disable interrupts prior to requesting a
+	 * transition of the HBA to D3 state.
+	 */
+	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
+
+	ret = ata_host_suspend(host, PMSG_SUSPEND);
+	if (ret)
+		return ret;
+
+	sunxi_ahci_disable_clks(ahci);
+
+	return 0;
+}
+
+static int sunxi_ahci_resume(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct sunxi_ahci *ahci = hpriv->plat_data;
+	int ret;
+
+	ret = sunxi_ahci_enable_clks(ahci);
+	if (ret)
+		return ret;
+
+	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
+		ret = ahci_reset_controller(host);
+		if (ret)
+			return ret;
+
+		ahci_init_controller(host);
+	}
+
+	ata_host_resume(host);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(sunxi_ahci_pmo, sunxi_ahci_susp, sunxi_ahci_resume);
+
+static const struct of_device_id sunxi_ahci_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-ahci" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, sunxi_ahci_of_match);
+
+static struct platform_driver sunxi_ahci_driver = {
+	.probe = sunxi_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "sunxi-ahci",
+		.owner = THIS_MODULE,
+		.of_match_table = sunxi_ahci_of_match,
+		.pm = &sunxi_ahci_pmo,
+	},
+};
+module_platform_driver(sunxi_ahci_driver);
+
+MODULE_DESCRIPTION("Allwinner sunxi AHCI SATA platform driver");
+MODULE_AUTHOR("Olliver Schinagl <oliver@schinagl.nl>");
+MODULE_LICENSE("GPL");
-- 
1.8.4.2

[PATCH v2 3/4] ARM: sun4i: dts: Add ahci / sata support

[Hans de Goede]

From: Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>

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-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/sun4i-a10-a1000.dts      |  4 ++++
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 27 +++++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10.dtsi           |  9 +++++++++
 3 files changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
index aef8207..fd6d512 100644
--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
@@ -48,6 +48,10 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
 			mmc0_cd_pin_a1000: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
index f50fb2b..53ac453 100644
--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
@@ -51,7 +51,19 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			pwr-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
+			ahci_pwr_pin_cubieboard: ahci_pwr_pin@0 {
+				allwinner,pins = "PB8";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			mmc0_cd_pin_cubieboard: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
 				allwinner,function = "gpio_in";
@@ -102,4 +114,19 @@
 			linux,default-trigger = "heartbeat";
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+		pinctrl-names = "default";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_cubieboard>;
+			gpio = <&pio 1 8 0>;
+			enable-active-high;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 4736dd2..731b491 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -331,6 +331,15 @@
 			status = "disabled";
 		};
 
+		sata: ahci@01c18000 {
+			compatible = "allwinner,sun4i-a10-ahci";
+			reg = <0x01c18000 0x1000>;
+			interrupts = <56>;
+			clocks = <&ahb_gates 25>, <&pll6 0>;
+			clock-names = "ahb_sata", "pll6_sata";
+			status = "disabled";
+		};
+
 		intc: interrupt-controller@01c20400 {
 			compatible = "allwinner,sun4i-ic";
 			reg = <0x01c20400 0x400>;
-- 
1.8.4.2

[PATCH v2 3/4] ARM: sun4i: dts: 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 | 27 +++++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10.dtsi           |  9 +++++++++
 3 files changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
index aef8207..fd6d512 100644
--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
@@ -48,6 +48,10 @@
 			status = "okay";
 		};
 
+		sata: ahci at 01c18000 {
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
 			mmc0_cd_pin_a1000: mmc0_cd_pin at 0 {
 				allwinner,pins = "PH1";
diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
index f50fb2b..53ac453 100644
--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
@@ -51,7 +51,19 @@
 			status = "okay";
 		};
 
+		sata: ahci at 01c18000 {
+			pwr-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
+			ahci_pwr_pin_cubieboard: ahci_pwr_pin at 0 {
+				allwinner,pins = "PB8";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			mmc0_cd_pin_cubieboard: mmc0_cd_pin at 0 {
 				allwinner,pins = "PH1";
 				allwinner,function = "gpio_in";
@@ -102,4 +114,19 @@
 			linux,default-trigger = "heartbeat";
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+		pinctrl-names = "default";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_cubieboard>;
+			gpio = <&pio 1 8 0>;
+			enable-active-high;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 4736dd2..731b491 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -331,6 +331,15 @@
 			status = "disabled";
 		};
 
+		sata: ahci at 01c18000 {
+			compatible = "allwinner,sun4i-a10-ahci";
+			reg = <0x01c18000 0x1000>;
+			interrupts = <56>;
+			clocks = <&ahb_gates 25>, <&pll6 0>;
+			clock-names = "ahb_sata", "pll6_sata";
+			status = "disabled";
+		};
+
 		intc: interrupt-controller at 01c20400 {
 			compatible = "allwinner,sun4i-ic";
 			reg = <0x01c20400 0x400>;
-- 
1.8.4.2

[PATCH v2 4/4] ARM: sun7i: dts: 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-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts     | 27 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 27 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 26 ++++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20.dtsi                |  9 +++++++++
 4 files changed, 89 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index 48777cd..a26711c 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -28,7 +28,19 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			pwr-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
+			ahci_pwr_pin_cubieboard2: ahci_pwr_pin@0 {
+				allwinner,pins = "PB8";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			mmc0_cd_pin_cubieboard2: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
 				allwinner,function = "gpio_in";
@@ -86,4 +98,19 @@
 			gpios = <&pio 7 20 0>;
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+		pinctrl-names = "default";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_cubieboard2>;
+			gpio = <&pio 1 8 0>;
+			enable-active-high;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index 2684f27..a5f3418 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -28,6 +28,11 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			pwr-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
 			mmc0_cd_pin_cubietruck: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
@@ -36,6 +41,13 @@
 				allwinner,pull = <0>;
 			};
 
+			ahci_pwr_pin_cubietruck: ahci_pwr_pin@0 {
+				allwinner,pins = "PH12";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			led_pins_cubietruck: led_pins@0 {
 				allwinner,pins = "PH7", "PH11", "PH20", "PH21";
 				allwinner,function = "gpio_out";
@@ -84,4 +96,19 @@
 			gpios = <&pio 7 7 0>;
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+		pinctrl-names = "default";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_cubietruck>;
+			gpio = <&pio 7 12 0>;
+			enable-active-high;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
index bf6f6c8..20b1000 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
@@ -37,7 +37,19 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			pwr-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
+			ahci_pwr_pin_olinuxinom: ahci_pwr_pin@0 {
+				allwinner,pins = "PB8";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			mmc0_cd_pin_olinuxinom: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
 				allwinner,function = "gpio_in";
@@ -116,4 +128,18 @@
 			default-state = "on";
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_olinuxinom>;
+			gpio = <&pio 1 8 0>;
+			enable-active-high;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index c9c123a..3242a29 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -448,6 +448,15 @@
 			};
 		};
 
+		sata: ahci@01c18000 {
+			compatible = "allwinner,sun4i-a10-ahci";
+			reg = <0x01c18000 0x1000>;
+			interrupts = <0 56 1>;
+			clocks = <&ahb_gates 25>, <&pll6 0>;
+			clock-names = "ahb_sata", "pll6_sata";
+			status = "disabled";
+		};
+
 		timer@01c20c00 {
 			compatible = "allwinner,sun4i-timer";
 			reg = <0x01c20c00 0x90>;
-- 
1.8.4.2

[PATCH v2 4/4] ARM: sun7i: dts: 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     | 27 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 27 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 26 ++++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20.dtsi                |  9 +++++++++
 4 files changed, 89 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index 48777cd..a26711c 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -28,7 +28,19 @@
 			status = "okay";
 		};
 
+		sata: ahci at 01c18000 {
+			pwr-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
+			ahci_pwr_pin_cubieboard2: ahci_pwr_pin at 0 {
+				allwinner,pins = "PB8";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			mmc0_cd_pin_cubieboard2: mmc0_cd_pin at 0 {
 				allwinner,pins = "PH1";
 				allwinner,function = "gpio_in";
@@ -86,4 +98,19 @@
 			gpios = <&pio 7 20 0>;
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+		pinctrl-names = "default";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_cubieboard2>;
+			gpio = <&pio 1 8 0>;
+			enable-active-high;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index 2684f27..a5f3418 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -28,6 +28,11 @@
 			status = "okay";
 		};
 
+		sata: ahci at 01c18000 {
+			pwr-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
 			mmc0_cd_pin_cubietruck: mmc0_cd_pin at 0 {
 				allwinner,pins = "PH1";
@@ -36,6 +41,13 @@
 				allwinner,pull = <0>;
 			};
 
+			ahci_pwr_pin_cubietruck: ahci_pwr_pin at 0 {
+				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";
@@ -84,4 +96,19 @@
 			gpios = <&pio 7 7 0>;
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+		pinctrl-names = "default";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_cubietruck>;
+			gpio = <&pio 7 12 0>;
+			enable-active-high;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
index bf6f6c8..20b1000 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
@@ -37,7 +37,19 @@
 			status = "okay";
 		};
 
+		sata: ahci at 01c18000 {
+			pwr-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl at 01c20800 {
+			ahci_pwr_pin_olinuxinom: ahci_pwr_pin at 0 {
+				allwinner,pins = "PB8";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			mmc0_cd_pin_olinuxinom: mmc0_cd_pin at 0 {
 				allwinner,pins = "PH1";
 				allwinner,function = "gpio_in";
@@ -116,4 +128,18 @@
 			default-state = "on";
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_olinuxinom>;
+			gpio = <&pio 1 8 0>;
+			enable-active-high;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index c9c123a..3242a29 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -448,6 +448,15 @@
 			};
 		};
 
+		sata: ahci at 01c18000 {
+			compatible = "allwinner,sun4i-a10-ahci";
+			reg = <0x01c18000 0x1000>;
+			interrupts = <0 56 1>;
+			clocks = <&ahb_gates 25>, <&pll6 0>;
+			clock-names = "ahb_sata", "pll6_sata";
+			status = "disabled";
+		};
+
 		timer at 01c20c00 {
 			compatible = "allwinner,sun4i-timer";
 			reg = <0x01c20c00 0x90>;
-- 
1.8.4.2

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Arnd Bergmann]

On Saturday 04 January 2014 10:14:36 Hans de Goede wrote:
> diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
> new file mode 100644
> index 0000000..0792fa5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
> @@ -0,0 +1,24 @@
> +Allwinner SUNXI AHCI SATA Controller
> +
> +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 "allwinner,sun4i-a10-ahci"
> +- reg		: <registers mapping>
> +- interrupts	: <interrupt mapping for AHCI IRQ>
> +- clocks	: clocks for ACHI
> +- clock-names	: clock names for AHCI

The binding needs to specify the required names for the clocks.

> +Optional properties:
> +- pwr-supply	: regulator to control the power supply GPIO
> +
> +Example:
> +	ahci@01c18000 {
> +		compatible = "allwinner,sun4i-a10-ahci";
> +		reg = <0x01c18000 0x1000>;
> +		interrupts = <0 56 1>;
> +		clocks = <&ahb_gates 25>, <&pll6 0>;
> +		clock-names = "ahb_sata", "pll6_sata";

"pll6_sata" doesn't sound particularly generic. The name should
reflect what the clock is used for, not what drives it.

Also, please send the binding as a separate patch with Cc to
the devicetree-discuss mailing list.

> +
> +static int sunxi_ahci_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);

This function should probably be in a separate phy driver. I would
very much hope that we can minimize the required code in an AHCI
driver and move code from this new file into the ahci-platform
driver. The clock, regulator and phy setup can all be optional
properties of the generic driver, and then there shouldn't
be much left that is sunxi specific.

> +static int sunxi_ahci_susp(struct device *dev)
> +{
> +	struct ata_host *host = dev_get_drvdata(dev);
> +	struct ahci_host_priv *hpriv = host->private_data;
> +	struct sunxi_ahci *ahci = hpriv->plat_data;
> +	int ret;
> +
> +	/*
> +	 * AHCI spec rev1.1 section 8.3.3:
> +	 * Software must disable interrupts prior to requesting a
> +	 * transition of the HBA to D3 state.
> +	 */
> +	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
> +
> +	ret = ata_host_suspend(host, PMSG_SUSPEND);
> +	if (ret)
> +		return ret;
> +
> +	sunxi_ahci_disable_clks(ahci);
> +
> +	return 0;
> +}

The only thing in here that seems sunxi-specific is the irq disabling
part. Can't you do this instead by calling disable_irq() and make
the function completely generic?

	Arnd

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Arnd Bergmann]

On Saturday 04 January 2014 10:14:36 Hans de Goede wrote:
> diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
> new file mode 100644
> index 0000000..0792fa5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
> @@ -0,0 +1,24 @@
> +Allwinner SUNXI AHCI SATA Controller
> +
> +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 "allwinner,sun4i-a10-ahci"
> +- reg		: <registers mapping>
> +- interrupts	: <interrupt mapping for AHCI IRQ>
> +- clocks	: clocks for ACHI
> +- clock-names	: clock names for AHCI

The binding needs to specify the required names for the clocks.

> +Optional properties:
> +- pwr-supply	: regulator to control the power supply GPIO
> +
> +Example:
> +	ahci at 01c18000 {
> +		compatible = "allwinner,sun4i-a10-ahci";
> +		reg = <0x01c18000 0x1000>;
> +		interrupts = <0 56 1>;
> +		clocks = <&ahb_gates 25>, <&pll6 0>;
> +		clock-names = "ahb_sata", "pll6_sata";

"pll6_sata" doesn't sound particularly generic. The name should
reflect what the clock is used for, not what drives it.

Also, please send the binding as a separate patch with Cc to
the devicetree-discuss mailing list.

> +
> +static int sunxi_ahci_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);

This function should probably be in a separate phy driver. I would
very much hope that we can minimize the required code in an AHCI
driver and move code from this new file into the ahci-platform
driver. The clock, regulator and phy setup can all be optional
properties of the generic driver, and then there shouldn't
be much left that is sunxi specific.

> +static int sunxi_ahci_susp(struct device *dev)
> +{
> +	struct ata_host *host = dev_get_drvdata(dev);
> +	struct ahci_host_priv *hpriv = host->private_data;
> +	struct sunxi_ahci *ahci = hpriv->plat_data;
> +	int ret;
> +
> +	/*
> +	 * AHCI spec rev1.1 section 8.3.3:
> +	 * Software must disable interrupts prior to requesting a
> +	 * transition of the HBA to D3 state.
> +	 */
> +	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
> +
> +	ret = ata_host_suspend(host, PMSG_SUSPEND);
> +	if (ret)
> +		return ret;
> +
> +	sunxi_ahci_disable_clks(ahci);
> +
> +	return 0;
> +}

The only thing in here that seems sunxi-specific is the irq disabling
part. Can't you do this instead by calling disable_irq() and make
the function completely generic?

	Arnd

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Arnd Bergmann]

On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
> > +Required properties:
> > +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
> > +- reg                : <registers mapping>
> > +- interrupts : <interrupt mapping for AHCI IRQ>
> > +- clocks     : clocks for ACHI
> > +- clock-names        : clock names for AHCI
> 
> The binding needs to specify the required names for the clocks.

fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
I would strongly suggest using the same names here, and documenting
them in the ahci binding as optional.

	Arnd

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Arnd Bergmann]

On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
> > +Required properties:
> > +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
> > +- reg                : <registers mapping>
> > +- interrupts : <interrupt mapping for AHCI IRQ>
> > +- clocks     : clocks for ACHI
> > +- clock-names        : clock names for AHCI
> 
> The binding needs to specify the required names for the clocks.

fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
I would strongly suggest using the same names here, and documenting
them in the ahci binding as optional.

	Arnd

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

Hi,

On 01/04/2014 10:39 PM, Arnd Bergmann wrote:
> On Saturday 04 January 2014 10:14:36 Hans de Goede wrote:
>> diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
>> new file mode 100644
>> index 0000000..0792fa5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
>> @@ -0,0 +1,24 @@
>> +Allwinner SUNXI AHCI SATA Controller
>> +
>> +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 "allwinner,sun4i-a10-ahci"
>> +- reg		: <registers mapping>
>> +- interrupts	: <interrupt mapping for AHCI IRQ>
>> +- clocks	: clocks for ACHI
>> +- clock-names	: clock names for AHCI
>
> The binding needs to specify the required names for the clocks.

Will fix.

>
>> +Optional properties:
>> +- pwr-supply	: regulator to control the power supply GPIO
>> +
>> +Example:
>> +	ahci@01c18000 {
>> +		compatible = "allwinner,sun4i-a10-ahci";
>> +		reg = <0x01c18000 0x1000>;
>> +		interrupts = <0 56 1>;
>> +		clocks = <&ahb_gates 25>, <&pll6 0>;
>> +		clock-names = "ahb_sata", "pll6_sata";
>
> "pll6_sata" doesn't sound particularly generic. The name should
> reflect what the clock is used for, not what drives it.

Will change to ahb + sata_ref.

>
> Also, please send the binding as a separate patch with Cc to
> the devicetree-discuss mailing list.

Hmm, this contradicts what others are saying who have requested for
the binding docs to be part of the same commit as the driver (with
various other drivers). Note that I've cc-ed devicetree already.

>
>> +
>> +static int sunxi_ahci_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);
>
> This function should probably be in a separate phy driver.  I would
> very much hope that we can minimize the required code in an AHCI
> driver and move code from this new file into the ahci-platform
> driver. The clock, regulator and phy setup can all be optional
> properties of the generic driver, and then there shouldn't
> be much left that is sunxi specific.

Writing a phy driver, and extending ahci-platform to use that
was my original plan. But the phy really is part of the
ahci ip-block here, and not a separate ip-block. Its registers
are smack in the middle of the io-range for the ahci function.

Also note that sunxi_ahci_pre_start_engine is rather sunxi
specific. Needing to put that in a generic ahci_platform driver
and only activating it for sunxi socs would only serve to
prove my point that at some point it is simply easier and
better to write a non generic platform glue driver when dealing
with exotic ahci ip blocks.

If we end up putting all sort of if soca do foo else if socb
do bar, else do nothing magic in ahci_platform.c I think we're
over generalizing. If something nicely fits as a generic
platform dev, by all means we should use a generic platform
driver, but that just won't work cleanly here.

Actually I've just completed writing a phy driver for
the usbphy in the same soc, and a patch to extend ehci-platform
to take an optional clk and phy. So I completely agree with the
basic idea, it just does not seem doable cleanly in this case,
hence my choice to write a separate platform driver. For the
usb stuff (wip) see:
https://github.com/jwrdegoede/linux-sunxi/commit/4655dd01936f42d8a75da08a00af439e0a34eaf7
https://github.com/jwrdegoede/linux-sunxi/commit/bcb674859d015ff9e082829dbd5cf239b8b53d4a

As said this is exactly what your asking me to do for sunxi-ahci,
but the difference is that for sunxi-ehci it works nicely, and
for sunxi-ahci it just doesn't work cleanly.



>
>> +static int sunxi_ahci_susp(struct device *dev)
>> +{
>> +	struct ata_host *host = dev_get_drvdata(dev);
>> +	struct ahci_host_priv *hpriv = host->private_data;
>> +	struct sunxi_ahci *ahci = hpriv->plat_data;
>> +	int ret;
>> +
>> +	/*
>> +	 * AHCI spec rev1.1 section 8.3.3:
>> +	 * Software must disable interrupts prior to requesting a
>> +	 * transition of the HBA to D3 state.
>> +	 */
>> +	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
>> +
>> +	ret = ata_host_suspend(host, PMSG_SUSPEND);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sunxi_ahci_disable_clks(ahci);
>> +
>> +	return 0;
>> +}
>
> The only thing in here that seems sunxi-specific is the irq disabling
> part. Can't you do this instead by calling disable_irq() and make
> the function completely generic?

The irq disabling part actually is not sunxi specific, ahci_platform.c
has it too.

Regards,

Hans

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

Hi,

On 01/04/2014 10:39 PM, Arnd Bergmann wrote:
> On Saturday 04 January 2014 10:14:36 Hans de Goede wrote:
>> diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
>> new file mode 100644
>> index 0000000..0792fa5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt
>> @@ -0,0 +1,24 @@
>> +Allwinner SUNXI AHCI SATA Controller
>> +
>> +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 "allwinner,sun4i-a10-ahci"
>> +- reg		: <registers mapping>
>> +- interrupts	: <interrupt mapping for AHCI IRQ>
>> +- clocks	: clocks for ACHI
>> +- clock-names	: clock names for AHCI
>
> The binding needs to specify the required names for the clocks.

Will fix.

>
>> +Optional properties:
>> +- pwr-supply	: regulator to control the power supply GPIO
>> +
>> +Example:
>> +	ahci at 01c18000 {
>> +		compatible = "allwinner,sun4i-a10-ahci";
>> +		reg = <0x01c18000 0x1000>;
>> +		interrupts = <0 56 1>;
>> +		clocks = <&ahb_gates 25>, <&pll6 0>;
>> +		clock-names = "ahb_sata", "pll6_sata";
>
> "pll6_sata" doesn't sound particularly generic. The name should
> reflect what the clock is used for, not what drives it.

Will change to ahb + sata_ref.

>
> Also, please send the binding as a separate patch with Cc to
> the devicetree-discuss mailing list.

Hmm, this contradicts what others are saying who have requested for
the binding docs to be part of the same commit as the driver (with
various other drivers). Note that I've cc-ed devicetree already.

>
>> +
>> +static int sunxi_ahci_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);
>
> This function should probably be in a separate phy driver.  I would
> very much hope that we can minimize the required code in an AHCI
> driver and move code from this new file into the ahci-platform
> driver. The clock, regulator and phy setup can all be optional
> properties of the generic driver, and then there shouldn't
> be much left that is sunxi specific.

Writing a phy driver, and extending ahci-platform to use that
was my original plan. But the phy really is part of the
ahci ip-block here, and not a separate ip-block. Its registers
are smack in the middle of the io-range for the ahci function.

Also note that sunxi_ahci_pre_start_engine is rather sunxi
specific. Needing to put that in a generic ahci_platform driver
and only activating it for sunxi socs would only serve to
prove my point that at some point it is simply easier and
better to write a non generic platform glue driver when dealing
with exotic ahci ip blocks.

If we end up putting all sort of if soca do foo else if socb
do bar, else do nothing magic in ahci_platform.c I think we're
over generalizing. If something nicely fits as a generic
platform dev, by all means we should use a generic platform
driver, but that just won't work cleanly here.

Actually I've just completed writing a phy driver for
the usbphy in the same soc, and a patch to extend ehci-platform
to take an optional clk and phy. So I completely agree with the
basic idea, it just does not seem doable cleanly in this case,
hence my choice to write a separate platform driver. For the
usb stuff (wip) see:
https://github.com/jwrdegoede/linux-sunxi/commit/4655dd01936f42d8a75da08a00af439e0a34eaf7
https://github.com/jwrdegoede/linux-sunxi/commit/bcb674859d015ff9e082829dbd5cf239b8b53d4a

As said this is exactly what your asking me to do for sunxi-ahci,
but the difference is that for sunxi-ehci it works nicely, and
for sunxi-ahci it just doesn't work cleanly.



>
>> +static int sunxi_ahci_susp(struct device *dev)
>> +{
>> +	struct ata_host *host = dev_get_drvdata(dev);
>> +	struct ahci_host_priv *hpriv = host->private_data;
>> +	struct sunxi_ahci *ahci = hpriv->plat_data;
>> +	int ret;
>> +
>> +	/*
>> +	 * AHCI spec rev1.1 section 8.3.3:
>> +	 * Software must disable interrupts prior to requesting a
>> +	 * transition of the HBA to D3 state.
>> +	 */
>> +	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
>> +
>> +	ret = ata_host_suspend(host, PMSG_SUSPEND);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sunxi_ahci_disable_clks(ahci);
>> +
>> +	return 0;
>> +}
>
> The only thing in here that seems sunxi-specific is the irq disabling
> part. Can't you do this instead by calling disable_irq() and make
> the function completely generic?

The irq disabling part actually is not sunxi specific, ahci_platform.c
has it too.

Regards,

Hans

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Arnd Bergmann]

On Sunday 05 January 2014, Hans de Goede wrote:
> >
> > Also, please send the binding as a separate patch with Cc to
> > the devicetree-discuss mailing list.
> 
> Hmm, this contradicts what others are saying who have requested for
> the binding docs to be part of the same commit as the driver (with
> various other drivers). Note that I've cc-ed devicetree already.

Probably my fault then, I've been away for some time and may misremember
what we discussed. Maybe someone else can clarify.

> >> +static int sunxi_ahci_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);
> >
> > This function should probably be in a separate phy driver.  I would
> > very much hope that we can minimize the required code in an AHCI
> > driver and move code from this new file into the ahci-platform
> > driver. The clock, regulator and phy setup can all be optional
> > properties of the generic driver, and then there shouldn't
> > be much left that is sunxi specific.
> 
> Writing a phy driver, and extending ahci-platform to use that
> was my original plan. But the phy really is part of the
> ahci ip-block here, and not a separate ip-block. Its registers
> are smack in the middle of the io-range for the ahci function.

I see. I wonder if the register layout is common with some other
implementation then. If it's part of the AHCI block, it's probably
not an Allwinner invention but comes from whoever implemented the
AHCI.

> Also note that sunxi_ahci_pre_start_engine is rather sunxi
> specific. Needing to put that in a generic ahci_platform driver
> and only activating it for sunxi socs would only serve to
> prove my point that at some point it is simply easier and
> better to write a non generic platform glue driver when dealing
> with exotic ahci ip blocks.
> 
> If we end up putting all sort of if soca do foo else if socb
> do bar, else do nothing magic in ahci_platform.c I think we're
> over generalizing. If something nicely fits as a generic
> platform dev, by all means we should use a generic platform
> driver, but that just won't work cleanly here.

Yes, but there may be some middle ground. I still think it would
be worthwhile to make the clock handling part of the common
ahci (or ahci-platform) driver and reuse that, since it seems
to be needed on a number of implementations. IIRC there is already
some inheritence model in libata that can be used to define
variations of drivers and have common parts done only once.

> Actually I've just completed writing a phy driver for
> the usbphy in the same soc, and a patch to extend ehci-platform
> to take an optional clk and phy. So I completely agree with the
> basic idea, it just does not seem doable cleanly in this case,
> hence my choice to write a separate platform driver. For the
> usb stuff (wip) see:
> https://github.com/jwrdegoede/linux-sunxi/commit/4655dd01936f42d8a75da08a00af439e0a34eaf7
> https://github.com/jwrdegoede/linux-sunxi/commit/bcb674859d015ff9e082829dbd5cf239b8b53d4a

Ah, very nice!

> >> +static int sunxi_ahci_susp(struct device *dev)
> >> +{
> >> +	struct ata_host *host = dev_get_drvdata(dev);
> >> +	struct ahci_host_priv *hpriv = host->private_data;
> >> +	struct sunxi_ahci *ahci = hpriv->plat_data;
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * AHCI spec rev1.1 section 8.3.3:
> >> +	 * Software must disable interrupts prior to requesting a
> >> +	 * transition of the HBA to D3 state.
> >> +	 */
> >> +	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
> >> +
> >> +	ret = ata_host_suspend(host, PMSG_SUSPEND);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	sunxi_ahci_disable_clks(ahci);
> >> +
> >> +	return 0;
> >> +}
> >
> > The only thing in here that seems sunxi-specific is the irq disabling
> > part. Can't you do this instead by calling disable_irq() and make
> > the function completely generic?
> 
> The irq disabling part actually is not sunxi specific, ahci_platform.c
> has it too.

Ok. Can this function be shared in some way then, e.g. by exporting the
ahci-platform implementation?

	Arnd

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Arnd Bergmann]

On Sunday 05 January 2014, Hans de Goede wrote:
> >
> > Also, please send the binding as a separate patch with Cc to
> > the devicetree-discuss mailing list.
> 
> Hmm, this contradicts what others are saying who have requested for
> the binding docs to be part of the same commit as the driver (with
> various other drivers). Note that I've cc-ed devicetree already.

Probably my fault then, I've been away for some time and may misremember
what we discussed. Maybe someone else can clarify.

> >> +static int sunxi_ahci_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);
> >
> > This function should probably be in a separate phy driver.  I would
> > very much hope that we can minimize the required code in an AHCI
> > driver and move code from this new file into the ahci-platform
> > driver. The clock, regulator and phy setup can all be optional
> > properties of the generic driver, and then there shouldn't
> > be much left that is sunxi specific.
> 
> Writing a phy driver, and extending ahci-platform to use that
> was my original plan. But the phy really is part of the
> ahci ip-block here, and not a separate ip-block. Its registers
> are smack in the middle of the io-range for the ahci function.

I see. I wonder if the register layout is common with some other
implementation then. If it's part of the AHCI block, it's probably
not an Allwinner invention but comes from whoever implemented the
AHCI.

> Also note that sunxi_ahci_pre_start_engine is rather sunxi
> specific. Needing to put that in a generic ahci_platform driver
> and only activating it for sunxi socs would only serve to
> prove my point that at some point it is simply easier and
> better to write a non generic platform glue driver when dealing
> with exotic ahci ip blocks.
> 
> If we end up putting all sort of if soca do foo else if socb
> do bar, else do nothing magic in ahci_platform.c I think we're
> over generalizing. If something nicely fits as a generic
> platform dev, by all means we should use a generic platform
> driver, but that just won't work cleanly here.

Yes, but there may be some middle ground. I still think it would
be worthwhile to make the clock handling part of the common
ahci (or ahci-platform) driver and reuse that, since it seems
to be needed on a number of implementations. IIRC there is already
some inheritence model in libata that can be used to define
variations of drivers and have common parts done only once.

> Actually I've just completed writing a phy driver for
> the usbphy in the same soc, and a patch to extend ehci-platform
> to take an optional clk and phy. So I completely agree with the
> basic idea, it just does not seem doable cleanly in this case,
> hence my choice to write a separate platform driver. For the
> usb stuff (wip) see:
> https://github.com/jwrdegoede/linux-sunxi/commit/4655dd01936f42d8a75da08a00af439e0a34eaf7
> https://github.com/jwrdegoede/linux-sunxi/commit/bcb674859d015ff9e082829dbd5cf239b8b53d4a

Ah, very nice!

> >> +static int sunxi_ahci_susp(struct device *dev)
> >> +{
> >> +	struct ata_host *host = dev_get_drvdata(dev);
> >> +	struct ahci_host_priv *hpriv = host->private_data;
> >> +	struct sunxi_ahci *ahci = hpriv->plat_data;
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * AHCI spec rev1.1 section 8.3.3:
> >> +	 * Software must disable interrupts prior to requesting a
> >> +	 * transition of the HBA to D3 state.
> >> +	 */
> >> +	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
> >> +
> >> +	ret = ata_host_suspend(host, PMSG_SUSPEND);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	sunxi_ahci_disable_clks(ahci);
> >> +
> >> +	return 0;
> >> +}
> >
> > The only thing in here that seems sunxi-specific is the irq disabling
> > part. Can't you do this instead by calling disable_irq() and make
> > the function completely generic?
> 
> The irq disabling part actually is not sunxi specific, ahci_platform.c
> has it too.

Ok. Can this function be shared in some way then, e.g. by exporting the
ahci-platform implementation?

	Arnd

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Olliver Schinagl]

On 01/04/14 22:47, Arnd Bergmann wrote:
> On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
>>> +Required properties:
>>> +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
>>> +- reg                : <registers mapping>
>>> +- interrupts : <interrupt mapping for AHCI IRQ>
>>> +- clocks     : clocks for ACHI
>>> +- clock-names        : clock names for AHCI
>> The binding needs to specify the required names for the clocks.
> fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
> I would strongly suggest using the same names here, and documenting
> them in the ahci binding as optional.
This is my fault, and you just reminded me I should have fixed that from 
the previous comments. It just slipped my mind and I'm sorry for that!

Hans, I'll go over the original commit thread and pick up all changes 
and send them as a patch to you Monday.

Oliver
>
> 	Arnd

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Olliver Schinagl]

On 01/04/14 22:47, Arnd Bergmann wrote:
> On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
>>> +Required properties:
>>> +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
>>> +- reg                : <registers mapping>
>>> +- interrupts : <interrupt mapping for AHCI IRQ>
>>> +- clocks     : clocks for ACHI
>>> +- clock-names        : clock names for AHCI
>> The binding needs to specify the required names for the clocks.
> fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
> I would strongly suggest using the same names here, and documenting
> them in the ahci binding as optional.
This is my fault, and you just reminded me I should have fixed that from 
the previous comments. It just slipped my mind and I'm sorry for that!

Hans, I'll go over the original commit thread and pick up all changes 
and send them as a patch to you Monday.

Oliver
>
> 	Arnd

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

Hi,

On 01/05/2014 01:42 PM, Olliver Schinagl wrote:
> On 01/04/14 22:47, Arnd Bergmann wrote:
>> On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
>>>> +Required properties:
>>>> +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
>>>> +- reg                : <registers mapping>
>>>> +- interrupts : <interrupt mapping for AHCI IRQ>
>>>> +- clocks     : clocks for ACHI
>>>> +- clock-names        : clock names for AHCI
>>> The binding needs to specify the required names for the clocks.
>> fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
>> I would strongly suggest using the same names here, and documenting
>> them in the ahci binding as optional.
> This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
>
> Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.

Thanks, sounds good. Then I won't touch the code till I get your patch (way too busy with usb now anyways).

Regards,

Hans

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

Hi,

On 01/05/2014 01:42 PM, Olliver Schinagl wrote:
> On 01/04/14 22:47, Arnd Bergmann wrote:
>> On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
>>>> +Required properties:
>>>> +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
>>>> +- reg                : <registers mapping>
>>>> +- interrupts : <interrupt mapping for AHCI IRQ>
>>>> +- clocks     : clocks for ACHI
>>>> +- clock-names        : clock names for AHCI
>>> The binding needs to specify the required names for the clocks.
>> fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
>> I would strongly suggest using the same names here, and documenting
>> them in the ahci binding as optional.
> This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
>
> Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.

Thanks, sounds good. Then I won't touch the code till I get your patch (way too busy with usb now anyways).

Regards,

Hans

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

Hi,

On 01/05/2014 12:35 PM, Arnd Bergmann wrote:
> On Sunday 05 January 2014, Hans de Goede wrote:

<snip>

>>>> +static int sunxi_ahci_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);
>>>
>>> This function should probably be in a separate phy driver.  I would
>>> very much hope that we can minimize the required code in an AHCI
>>> driver and move code from this new file into the ahci-platform
>>> driver. The clock, regulator and phy setup can all be optional
>>> properties of the generic driver, and then there shouldn't
>>> be much left that is sunxi specific.
>>
>> Writing a phy driver, and extending ahci-platform to use that
>> was my original plan. But the phy really is part of the
>> ahci ip-block here, and not a separate ip-block. Its registers
>> are smack in the middle of the io-range for the ahci function.
>
> I see. I wonder if the register layout is common with some other
> implementation then. If it's part of the AHCI block, it's probably
> not an Allwinner invention but comes from whoever implemented the
> AHCI.

Right, but so far we've been unable to find anything quite like it.

>> Also note that sunxi_ahci_pre_start_engine is rather sunxi
>> specific. Needing to put that in a generic ahci_platform driver
>> and only activating it for sunxi socs would only serve to
>> prove my point that at some point it is simply easier and
>> better to write a non generic platform glue driver when dealing
>> with exotic ahci ip blocks.
>>
>> If we end up putting all sort of if soca do foo else if socb
>> do bar, else do nothing magic in ahci_platform.c I think we're
>> over generalizing. If something nicely fits as a generic
>> platform dev, by all means we should use a generic platform
>> driver, but that just won't work cleanly here.
>
> Yes, but there may be some middle ground. I still think it would
> be worthwhile to make the clock handling part of the common
> ahci (or ahci-platform) driver and reuse that, since it seems
> to be needed on a number of implementations. IIRC there is already
> some inheritence model in libata that can be used to define
> variations of drivers and have common parts done only once.

Ok, thinking more about this I think I have a solution which
should be acceptable. I'll do a v3 the coming days.

Regards,

Hans

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

Hi,

On 01/05/2014 12:35 PM, Arnd Bergmann wrote:
> On Sunday 05 January 2014, Hans de Goede wrote:

<snip>

>>>> +static int sunxi_ahci_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);
>>>
>>> This function should probably be in a separate phy driver.  I would
>>> very much hope that we can minimize the required code in an AHCI
>>> driver and move code from this new file into the ahci-platform
>>> driver. The clock, regulator and phy setup can all be optional
>>> properties of the generic driver, and then there shouldn't
>>> be much left that is sunxi specific.
>>
>> Writing a phy driver, and extending ahci-platform to use that
>> was my original plan. But the phy really is part of the
>> ahci ip-block here, and not a separate ip-block. Its registers
>> are smack in the middle of the io-range for the ahci function.
>
> I see. I wonder if the register layout is common with some other
> implementation then. If it's part of the AHCI block, it's probably
> not an Allwinner invention but comes from whoever implemented the
> AHCI.

Right, but so far we've been unable to find anything quite like it.

>> Also note that sunxi_ahci_pre_start_engine is rather sunxi
>> specific. Needing to put that in a generic ahci_platform driver
>> and only activating it for sunxi socs would only serve to
>> prove my point that at some point it is simply easier and
>> better to write a non generic platform glue driver when dealing
>> with exotic ahci ip blocks.
>>
>> If we end up putting all sort of if soca do foo else if socb
>> do bar, else do nothing magic in ahci_platform.c I think we're
>> over generalizing. If something nicely fits as a generic
>> platform dev, by all means we should use a generic platform
>> driver, but that just won't work cleanly here.
>
> Yes, but there may be some middle ground. I still think it would
> be worthwhile to make the clock handling part of the common
> ahci (or ahci-platform) driver and reuse that, since it seems
> to be needed on a number of implementations. IIRC there is already
> some inheritence model in libata that can be used to define
> variations of drivers and have common parts done only once.

Ok, thinking more about this I think I have a solution which
should be acceptable. I'll do a v3 the coming days.

Regards,

Hans

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

Hi,

On 01/05/2014 01:42 PM, Olliver Schinagl wrote:
> On 01/04/14 22:47, Arnd Bergmann wrote:
>> On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
>>>> +Required properties:
>>>> +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
>>>> +- reg                : <registers mapping>
>>>> +- interrupts : <interrupt mapping for AHCI IRQ>
>>>> +- clocks     : clocks for ACHI
>>>> +- clock-names        : clock names for AHCI
>>> The binding needs to specify the required names for the clocks.
>> fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
>> I would strongly suggest using the same names here, and documenting
>> them in the ahci binding as optional.
> This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
>
> Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.

As I just mentioned to Arnd I've a better idea to be able to re-use most of
the ahci_platform.c code without copy-pasting it. So I'm going to do a v3
soonish. Can you please just make a mail with summary of the requested
changes and send that to me, or do this after I've send v3 ?

Thanks,

Hans

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Hans de Goede]

Hi,

On 01/05/2014 01:42 PM, Olliver Schinagl wrote:
> On 01/04/14 22:47, Arnd Bergmann wrote:
>> On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
>>>> +Required properties:
>>>> +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
>>>> +- reg                : <registers mapping>
>>>> +- interrupts : <interrupt mapping for AHCI IRQ>
>>>> +- clocks     : clocks for ACHI
>>>> +- clock-names        : clock names for AHCI
>>> The binding needs to specify the required names for the clocks.
>> fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
>> I would strongly suggest using the same names here, and documenting
>> them in the ahci binding as optional.
> This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
>
> Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.

As I just mentioned to Arnd I've a better idea to be able to re-use most of
the ahci_platform.c code without copy-pasting it. So I'm going to do a v3
soonish. Can you please just make a mail with summary of the requested
changes and send that to me, or do this after I've send v3 ?

Thanks,

Hans

Re: [PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Olliver Schinagl]

On 01/05/14 14:32, Hans de Goede wrote:
> Hi,
>
> On 01/05/2014 01:42 PM, Olliver Schinagl wrote:
>> On 01/04/14 22:47, Arnd Bergmann wrote:
>>> On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
>>>>> +Required properties:
>>>>> +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
>>>>> +- reg                : <registers mapping>
>>>>> +- interrupts : <interrupt mapping for AHCI IRQ>
>>>>> +- clocks     : clocks for ACHI
>>>>> +- clock-names        : clock names for AHCI
>>>> The binding needs to specify the required names for the clocks.
>>> fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
>>> I would strongly suggest using the same names here, and documenting
>>> them in the ahci binding as optional.
>> This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
>>
>> Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.
> As I just mentioned to Arnd I've a better idea to be able to re-use most of
> the ahci_platform.c code without copy-pasting it. So I'm going to do a v3
> soonish. Can you please just make a mail with summary of the requested
> changes and send that to me, or do this after I've send v3 ?
if memory serves, it was mostly about the dt documentation and binding 
names, i'll work on it first thing tomorrow morning ;)
>
> Thanks,
>
> Hans

[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

[Olliver Schinagl]

On 01/05/14 14:32, Hans de Goede wrote:
> Hi,
>
> On 01/05/2014 01:42 PM, Olliver Schinagl wrote:
>> On 01/04/14 22:47, Arnd Bergmann wrote:
>>> On Saturday 04 January 2014 22:39:50 Arnd Bergmann wrote:
>>>>> +Required properties:
>>>>> +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci"
>>>>> +- reg                : <registers mapping>
>>>>> +- interrupts : <interrupt mapping for AHCI IRQ>
>>>>> +- clocks     : clocks for ACHI
>>>>> +- clock-names        : clock names for AHCI
>>>> The binding needs to specify the required names for the clocks.
>>> fwiw, the imx driver uses "ahb" and "sata_ref" as the clock names.
>>> I would strongly suggest using the same names here, and documenting
>>> them in the ahci binding as optional.
>> This is my fault, and you just reminded me I should have fixed that from the previous comments. It just slipped my mind and I'm sorry for that!
>>
>> Hans, I'll go over the original commit thread and pick up all changes and send them as a patch to you Monday.
> As I just mentioned to Arnd I've a better idea to be able to re-use most of
> the ahci_platform.c code without copy-pasting it. So I'm going to do a v3
> soonish. Can you please just make a mail with summary of the requested
> changes and send that to me, or do this after I've send v3 ?
if memory serves, it was mostly about the dt documentation and binding 
names, i'll work on it first thing tomorrow morning ;)
>
> Thanks,
>
> Hans

Re: [PATCH v2 1/4] libahci: Add a pre ahci_start_engine hook

[Tejun Heo]

Hello,

On Sat, Jan 04, 2014 at 10:14:35AM +0100, Hans de Goede wrote:
> @@ -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 pre ahci_start_engine hook */
> +	void			(*pre_start_engine)(struct ata_port *ap);

I'd much prefer if the callback overrides start_engine itself rather
than adding a hook inside start_engine.  This is a bit too specific.

Thanks.

-- 
tejun

[PATCH v2 1/4] libahci: Add a pre ahci_start_engine hook

[Tejun Heo]

Hello,

On Sat, Jan 04, 2014 at 10:14:35AM +0100, Hans de Goede wrote:
> @@ -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 pre ahci_start_engine hook */
> +	void			(*pre_start_engine)(struct ata_port *ap);

I'd much prefer if the callback overrides start_engine itself rather
than adding a hook inside start_engine.  This is a bit too specific.

Thanks.

-- 
tejun