[PATCH V1 0/3] ahci: enable ahci sata support on imx6q

[PATCH V1 0/3] ahci: enable ahci sata support on imx6q

[Richard Zhu]

v1: add imx6q specific ahci sata support to arch/arm/mach-imx/mach-imx6q.c

These patches is based on imx/dt branch of
"http://git.linaro.org/git-ro/people/shawnguo/linux-2.6.git"

[v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms
[v1 2/3] ata: ahci_platform: enable imx6q ahci sata support
[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[PATCH V1 0/3] ahci: enable ahci sata support on imx6q

[Richard Zhu]

v1: add imx6q specific ahci sata support to arch/arm/mach-imx/mach-imx6q.c

These patches is based on imx/dt branch of
"http://git.linaro.org/git-ro/people/shawnguo/linux-2.6.git"

[v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms
[v1 2/3] ata: ahci_platform: enable imx6q ahci sata support
[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms

[Richard Zhu]

Only imx6q has the ahci sata controller, enable
it on imx6q platforms.

Signed-off-by: Richard Zhu <r65037@freescale.com>
---
 arch/arm/boot/dts/imx6q-sabreauto.dts |    6 ++++++
 arch/arm/boot/dts/imx6q-sabrelite.dts |    6 ++++++
 arch/arm/boot/dts/imx6q-sabresd.dts   |    6 ++++++
 arch/arm/boot/dts/imx6q.dtsi          |    9 +++++++++
 4 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-sabreauto.dts b/arch/arm/boot/dts/imx6q-sabreauto.dts
index 09a7580..79643cc 100644
--- a/arch/arm/boot/dts/imx6q-sabreauto.dts
+++ b/arch/arm/boot/dts/imx6q-sabreauto.dts
@@ -18,6 +18,12 @@
 / {
 	model = "Freescale i.MX6 Quad SABRE Automotive Board";
 	compatible = "fsl,imx6q-sabreauto", "fsl,imx6q";
+
+	soc {
+		ahci@02200000 { /* AHCI SATA */
+			status = "okay";
+		};
+	}
 };
 
 &iomuxc {
diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 6a00066..dac40af 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -21,6 +21,12 @@
 		reg = <0x10000000 0x40000000>;
 	};
 
+	soc {
+		ahci@02200000 { /* AHCI SATA */
+			status = "okay";
+		};
+	}
+
 	regulators {
 		compatible = "simple-bus";
 
diff --git a/arch/arm/boot/dts/imx6q-sabresd.dts b/arch/arm/boot/dts/imx6q-sabresd.dts
index 0038228..ecae151 100644
--- a/arch/arm/boot/dts/imx6q-sabresd.dts
+++ b/arch/arm/boot/dts/imx6q-sabresd.dts
@@ -18,6 +18,12 @@
 / {
 	model = "Freescale i.MX6 Quad SABRE Smart Device Board";
 	compatible = "fsl,imx6q-sabresd", "fsl,imx6q";
+
+	soc {
+		ahci@02200000 { /* AHCI SATA */
+			status = "okay";
+		};
+	};
 };
 
 &iomuxc {
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index e7dd2c4..e13ff30 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -424,6 +424,15 @@
 			};
 		};
 
+		ahci@02200000 { /* AHCI SATA */
+			compatible = "snps,imx-ahci";
+			reg = <0x02200000 0x4000>;
+			interrupts = <0 39 0x04>;
+			clocks = <&clks 154>, <&clks 187>;
+			clock-names = "sata", "sata_ref_100m";
+			status = "disabled";
+		};
+
 		ipu2: ipu@02800000 {
 			#crtc-cells = <1>;
 			compatible = "fsl,imx6q-ipu";
-- 
1.7.5.4

[v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Richard Zhu]

imx6q contains the Synopsys AHCI SATA controller which shares
ahci_platform driver with other controllers.

This patch updates the DT compatible list for ahci_platform,
and enable the imx6q ahci sata support.

Signed-off-by: Richard Zhu <r65037@freescale.com>
---
 drivers/ata/ahci_platform.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 7a8a284..61f2142 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -327,6 +327,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
 
 static const struct of_device_id ahci_of_match[] = {
 	{ .compatible = "snps,spear-ahci", },
+	{ .compatible = "snps,imx-ahci", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ahci_of_match);
-- 
1.7.5.4

[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Richard Zhu]

Only the imx6q contains the ahci sata controller,
other imx6 SoCs don't have it.

Enable the ahci sata only on imx6q platforms

Signed-off-by: Richard Zhu <r65037@freescale.com>
---
 arch/arm/mach-imx/mach-imx6q.c |  179 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 178 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 045e5e3..68b0a45 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -10,6 +10,7 @@
  * http://www.gnu.org/copyleft/gpl.html
  */
 
+#include <linux/ahci_platform.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
@@ -23,6 +24,7 @@
 #include <linux/irqchip.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/opp.h>
@@ -39,6 +41,22 @@
 #include "cpuidle.h"
 #include "hardware.h"
 
+#define MX6Q_SATA_BASE_ADDR	0x02200000
+
+enum {
+	HOST_CAP = 0x00,
+	HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
+	HOST_CTL = 0x04, /* global host control */
+	HOST_RESET = (1 << 0),  /* reset controller; self-clear */
+	HOST_PORTS_IMPL	= 0x0c, /* bitmap of implemented ports */
+	HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
+	HOST_VERSIONR = 0xf8, /* host version register*/
+
+	PORT_SATA_SR = 0x128, /* Port0 SATA Status */
+	PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
+	PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */
+};
+
 static u32 chip_revision;
 
 int imx6q_revision(void)
@@ -162,12 +180,171 @@ static void __init imx6q_usb_init(void)
 	imx_anatop_usb_chrg_detect_disable();
 }
 
+/* imx ahci module initialization. */
+static int imx_sata_init(struct device *dev, void __iomem *addr)
+{
+	int ret = 0, iterations = 100;
+	struct clk *ahb_clk, *sata_clk, *sata_ref_clk;
+	struct regmap *gpr;
+
+	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+	if (IS_ERR(gpr)) {
+		pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
+		return PTR_ERR(gpr);
+	}
+
+	/* enable the clks */
+	sata_clk = devm_clk_get(dev, "sata");
+	if (IS_ERR(sata_clk)) {
+		dev_err(dev, "can't get sata clock.\n");
+		return PTR_ERR(sata_clk);
+	}
+	ret = clk_prepare_enable(sata_clk);
+	if (ret < 0) {
+		dev_err(dev, "can't prepare-enable sata clock\n");
+		clk_put(sata_clk);
+		return ret;
+	}
+	sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
+	if (IS_ERR(sata_ref_clk)) {
+		dev_err(dev, "can't get sata_ref clock.\n");
+		ret = PTR_ERR(sata_ref_clk);
+		goto release_sata_clk;
+	}
+	ret = clk_prepare_enable(sata_ref_clk);
+	if (ret < 0) {
+		dev_err(dev, "can't prepare-enable sata_ref clock\n");
+		clk_put(sata_ref_clk);
+		ret = PTR_ERR(sata_ref_clk);
+		goto release_sata_clk;
+	}
+
+	/* set PHY Paremeters, two steps to configure the GPR13,
+	 * one write for rest of parameters, mask of first write is 0x07fffffd,
+	 * and the other one write for setting the mpll_clk_off_b
+	 *.rx_eq_val_0(iomuxc_gpr13[26:24]),
+	 *.los_lvl(iomuxc_gpr13[23:19]),
+	 *.rx_dpll_mode_0(iomuxc_gpr13[18:16]),
+	 *.mpll_ss_en(iomuxc_gpr13[14]),
+	 *.tx_atten_0(iomuxc_gpr13[13:11]),
+	 *.tx_boost_0(iomuxc_gpr13[10:7]),
+	 *.tx_lvl(iomuxc_gpr13[6:2]),
+	 *.mpll_clk_off(iomuxc_gpr13[1]),
+	 *.tx_edgerate_0(iomuxc_gpr13[0]),
+	 */
+	regmap_update_bits(gpr, 0x34, 0x07fffffd, 0x0593e4c4);
+	regmap_update_bits(gpr, 0x34, 0x2, 0x2);
+
+	/*  reset hba */
+	writel(HOST_RESET, addr + HOST_CTL);
+	ret = 0;
+	while (readl(addr + HOST_VERSIONR) == 0) {
+		ret++;
+		usleep_range(1, 2);
+		if (ret > 100) {
+			dev_info(dev, "can't recover from reset hba!\n");
+			break;
+		}
+	}
+
+	/* get the ahb clock rate, and configure the TIMER1MS reg later */
+	ahb_clk = clk_get_sys(NULL, "ahb");
+	if (IS_ERR(ahb_clk)) {
+		dev_err(dev, "no ahb clock.\n");
+		ret = PTR_ERR(ahb_clk);
+		goto error;
+	}
+	ret = clk_get_rate(ahb_clk) / 1000;
+	clk_put(ahb_clk);
+	writel(ret, addr + HOST_TIMER1MS);
+
+	/*
+	 * configure CAP_SSS (support stagered spin up),
+	 * and implement the port0
+	 */
+	ret = readl(addr + HOST_CAP);
+	if (!(ret & HOST_CAP_SSS))
+		writel(ret |= HOST_CAP_SSS, addr + HOST_CAP);
+	ret = readl(addr + HOST_PORTS_IMPL);
+	if (!(ret & 0x1))
+		writel((ret | 0x1), addr + HOST_PORTS_IMPL);
+
+	/*
+	 * no device detected on the port, in order to save the power
+	 * consumption, enter into iddq mode(power down circuitry)
+	 * and release the clocks.
+	 * NOTE:in this case, the sata port can't be used anymore
+	 * without one system reboot.
+	 */
+	do {
+		if ((readl(addr + PORT_SATA_SR) & 0xF) == 0)
+			usleep_range(100, 200);
+		else
+			break;
+
+		if (iterations == 0) {
+			/*  enter into iddq mode, save power */
+			pr_info("no sata disk, enter into pddq mode.\n");
+			ret = readl(addr + PORT_PHY_CTL);
+			ret |= PORT_PHY_CTL_PDDQ_LOC;
+			writel(ret, addr + PORT_PHY_CTL);
+			ret = -ENODEV;
+			goto error;
+		}
+	} while (iterations-- > 0);
+
+	return 0;
+
+error:
+	regmap_update_bits(gpr, 0x34, 0x2, 0x0);
+	clk_disable_unprepare(sata_ref_clk);
+release_sata_clk:
+	clk_disable_unprepare(sata_clk);
+
+	return ret;
+}
+
+static void imx_sata_exit(struct device *dev)
+{
+	struct clk *sata_clk, *sata_ref_clk;
+	struct regmap *gpr;
+
+	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+	if (IS_ERR(gpr))
+		pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
+
+	regmap_update_bits(gpr, 0x34, 0x2, 0x0);
+
+	sata_clk = devm_clk_get(dev, "sata");
+	if (IS_ERR(sata_clk))
+		dev_err(dev, "can't get sata clock.\n");
+	else
+		clk_disable_unprepare(sata_clk);
+	sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
+	if (IS_ERR(sata_ref_clk))
+		dev_err(dev, "can't get sata_ref clock.\n");
+	else
+		clk_disable_unprepare(sata_ref_clk);
+}
+
+static struct ahci_platform_data imx_sata_pdata = {
+	.init = imx_sata_init,
+	.exit = imx_sata_exit,
+};
+
+static const struct of_dev_auxdata imx6q_auxdata_lookup[] __initconst = {
+	OF_DEV_AUXDATA("snps,imx-ahci", MX6Q_SATA_BASE_ADDR, "imx-ahci",
+			&imx_sata_pdata),
+	{ /* sentinel */ }
+};
+
 static void __init imx6q_init_machine(void)
 {
 	if (of_machine_is_compatible("fsl,imx6q-sabrelite"))
 		imx6q_sabrelite_init();
 
-	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+	of_platform_populate(NULL, of_default_bus_match_table,
+			imx6q_auxdata_lookup, NULL);
 
 	imx_anatop_init();
 	imx6q_pm_init();
-- 
1.7.5.4

Re: [v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Tejun Heo]

On Mon, Jun 17, 2013 at 2:52 AM, Richard Zhu
<richard.zhuhongxing@gmail.com> wrote:
> imx6q contains the Synopsys AHCI SATA controller which shares
> ahci_platform driver with other controllers.
>
> This patch updates the DT compatible list for ahci_platform,
> and enable the imx6q ahci sata support.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>

I suppose this better be routed with the rest of the series through
the arch tree?  If not, please let me know.

Acked-by: Tejun Heo <tj@kernel.org>

There probably will be a merge conflict but it's gonna be very
trivial, so no worries.

Thanks.

--
tejun

[v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Tejun Heo]

On Mon, Jun 17, 2013 at 2:52 AM, Richard Zhu
<richard.zhuhongxing@gmail.com> wrote:
> imx6q contains the Synopsys AHCI SATA controller which shares
> ahci_platform driver with other controllers.
>
> This patch updates the DT compatible list for ahci_platform,
> and enable the imx6q ahci sata support.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>

I suppose this better be routed with the rest of the series through
the arch tree?  If not, please let me know.

Acked-by: Tejun Heo <tj@kernel.org>

There probably will be a merge conflict but it's gonna be very
trivial, so no worries.

Thanks.

--
tejun

Re: [v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Shawn Guo]

On Mon, Jun 17, 2013 at 05:52:46PM +0800, Richard Zhu wrote:
> imx6q contains the Synopsys AHCI SATA controller which shares
> ahci_platform driver with other controllers.
> 
> This patch updates the DT compatible list for ahci_platform,
> and enable the imx6q ahci sata support.
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  drivers/ata/ahci_platform.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 7a8a284..61f2142 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -327,6 +327,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
>  
>  static const struct of_device_id ahci_of_match[] = {
>  	{ .compatible = "snps,spear-ahci", },
> +	{ .compatible = "snps,imx-ahci", },

I'm not sure it makes much sense to invent multiple platform specific
strings for an IP which is completely compatible between them.
Why don't we just have a generic compatible string like "snps,ahci"
for all those compatible platforms, and only add platform specific
string when there is incompatibility on particular platform to deal
with?

Added devicetree-discuss to get DT people's opinion.

Shawn

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ahci_of_match);
> -- 
> 1.7.5.4
> 

[v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Shawn Guo]

On Mon, Jun 17, 2013 at 05:52:46PM +0800, Richard Zhu wrote:
> imx6q contains the Synopsys AHCI SATA controller which shares
> ahci_platform driver with other controllers.
> 
> This patch updates the DT compatible list for ahci_platform,
> and enable the imx6q ahci sata support.
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  drivers/ata/ahci_platform.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 7a8a284..61f2142 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -327,6 +327,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
>  
>  static const struct of_device_id ahci_of_match[] = {
>  	{ .compatible = "snps,spear-ahci", },
> +	{ .compatible = "snps,imx-ahci", },

I'm not sure it makes much sense to invent multiple platform specific
strings for an IP which is completely compatible between them.
Why don't we just have a generic compatible string like "snps,ahci"
for all those compatible platforms, and only add platform specific
string when there is incompatibility on particular platform to deal
with?

Added devicetree-discuss to get DT people's opinion.

Shawn

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ahci_of_match);
> -- 
> 1.7.5.4
> 

Re: [v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms

[Shawn Guo]

On Mon, Jun 17, 2013 at 05:52:45PM +0800, Richard Zhu wrote:
> Only imx6q has the ahci sata controller, enable
> it on imx6q platforms.
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q-sabreauto.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q-sabrelite.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q-sabresd.dts   |    6 ++++++
>  arch/arm/boot/dts/imx6q.dtsi          |    9 +++++++++
>  4 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-sabreauto.dts b/arch/arm/boot/dts/imx6q-sabreauto.dts
> index 09a7580..79643cc 100644
> --- a/arch/arm/boot/dts/imx6q-sabreauto.dts
> +++ b/arch/arm/boot/dts/imx6q-sabreauto.dts
> @@ -18,6 +18,12 @@
>  / {
>  	model = "Freescale i.MX6 Quad SABRE Automotive Board";
>  	compatible = "fsl,imx6q-sabreauto", "fsl,imx6q";
> +
> +	soc {
> +		ahci@02200000 { /* AHCI SATA */
> +			status = "okay";
> +		};
> +	}
>  };
>  
>  &iomuxc {
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 6a00066..dac40af 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -21,6 +21,12 @@
>  		reg = <0x10000000 0x40000000>;
>  	};
>  
> +	soc {
> +		ahci@02200000 { /* AHCI SATA */
> +			status = "okay";
> +		};
> +	}
> +
>  	regulators {
>  		compatible = "simple-bus";
>  
> diff --git a/arch/arm/boot/dts/imx6q-sabresd.dts b/arch/arm/boot/dts/imx6q-sabresd.dts
> index 0038228..ecae151 100644
> --- a/arch/arm/boot/dts/imx6q-sabresd.dts
> +++ b/arch/arm/boot/dts/imx6q-sabresd.dts
> @@ -18,6 +18,12 @@
>  / {
>  	model = "Freescale i.MX6 Quad SABRE Smart Device Board";
>  	compatible = "fsl,imx6q-sabresd", "fsl,imx6q";
> +
> +	soc {
> +		ahci@02200000 { /* AHCI SATA */
> +			status = "okay";
> +		};
> +	};
>  };
>  
>  &iomuxc {
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index e7dd2c4..e13ff30 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -424,6 +424,15 @@
>  			};
>  		};
>  
> +		ahci@02200000 { /* AHCI SATA */

Add a label for the node like "ahci: ahci@02200000", you can refer to
the node simply using the label in board dts, just like every one else
is doing.

&ahci {
	status = "okay";
};

Shawn

> +			compatible = "snps,imx-ahci";
> +			reg = <0x02200000 0x4000>;
> +			interrupts = <0 39 0x04>;
> +			clocks = <&clks 154>, <&clks 187>;
> +			clock-names = "sata", "sata_ref_100m";
> +			status = "disabled";
> +		};
> +
>  		ipu2: ipu@02800000 {
>  			#crtc-cells = <1>;
>  			compatible = "fsl,imx6q-ipu";
> -- 
> 1.7.5.4
> 

[v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms

[Shawn Guo]

On Mon, Jun 17, 2013 at 05:52:45PM +0800, Richard Zhu wrote:
> Only imx6q has the ahci sata controller, enable
> it on imx6q platforms.
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q-sabreauto.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q-sabrelite.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q-sabresd.dts   |    6 ++++++
>  arch/arm/boot/dts/imx6q.dtsi          |    9 +++++++++
>  4 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-sabreauto.dts b/arch/arm/boot/dts/imx6q-sabreauto.dts
> index 09a7580..79643cc 100644
> --- a/arch/arm/boot/dts/imx6q-sabreauto.dts
> +++ b/arch/arm/boot/dts/imx6q-sabreauto.dts
> @@ -18,6 +18,12 @@
>  / {
>  	model = "Freescale i.MX6 Quad SABRE Automotive Board";
>  	compatible = "fsl,imx6q-sabreauto", "fsl,imx6q";
> +
> +	soc {
> +		ahci at 02200000 { /* AHCI SATA */
> +			status = "okay";
> +		};
> +	}
>  };
>  
>  &iomuxc {
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 6a00066..dac40af 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -21,6 +21,12 @@
>  		reg = <0x10000000 0x40000000>;
>  	};
>  
> +	soc {
> +		ahci at 02200000 { /* AHCI SATA */
> +			status = "okay";
> +		};
> +	}
> +
>  	regulators {
>  		compatible = "simple-bus";
>  
> diff --git a/arch/arm/boot/dts/imx6q-sabresd.dts b/arch/arm/boot/dts/imx6q-sabresd.dts
> index 0038228..ecae151 100644
> --- a/arch/arm/boot/dts/imx6q-sabresd.dts
> +++ b/arch/arm/boot/dts/imx6q-sabresd.dts
> @@ -18,6 +18,12 @@
>  / {
>  	model = "Freescale i.MX6 Quad SABRE Smart Device Board";
>  	compatible = "fsl,imx6q-sabresd", "fsl,imx6q";
> +
> +	soc {
> +		ahci at 02200000 { /* AHCI SATA */
> +			status = "okay";
> +		};
> +	};
>  };
>  
>  &iomuxc {
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index e7dd2c4..e13ff30 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -424,6 +424,15 @@
>  			};
>  		};
>  
> +		ahci at 02200000 { /* AHCI SATA */

Add a label for the node like "ahci: ahci at 02200000", you can refer to
the node simply using the label in board dts, just like every one else
is doing.

&ahci {
	status = "okay";
};

Shawn

> +			compatible = "snps,imx-ahci";
> +			reg = <0x02200000 0x4000>;
> +			interrupts = <0 39 0x04>;
> +			clocks = <&clks 154>, <&clks 187>;
> +			clock-names = "sata", "sata_ref_100m";
> +			status = "disabled";
> +		};
> +
>  		ipu2: ipu at 02800000 {
>  			#crtc-cells = <1>;
>  			compatible = "fsl,imx6q-ipu";
> -- 
> 1.7.5.4
> 

RE: [v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Zhu Richard-R65037]

Hi Tejun&Shawn:
Thanks for your comments.

Hi Tejun:
I looked through the for-next branch of the libata git-repo, there maybe a merge conflict.
"
static const struct of_device_id ahci_of_match[] = { 
        { .compatible = "snps,spear-ahci", },
        { .compatible = "snps,exynos5440-ahci", },
        {}, 
};
MODULE_DEVICE_TABLE(of, ahci_of_match);
"

How about to re-change the patch, based on for-next branch of the libata git-repo.

Hi Shawn:
About the one similar IP, multiple platform specific strings inventions.
Waiting for DT's people’s opinion.


Best Regards
Richard Zhu
________________________________________
From: Shawn Guo [shawn.guo@linaro.org]
Sent: Tuesday, June 18, 2013 10:01 AM
To: Richard Zhu
Cc: linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; linux-ide@vger.kernel.org; Zhu Richard-R65037; devicetree-discuss@lists.ozlabs.org
Subject: Re: [v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

On Mon, Jun 17, 2013 at 05:52:46PM +0800, Richard Zhu wrote:
> imx6q contains the Synopsys AHCI SATA controller which shares
> ahci_platform driver with other controllers.
>
> This patch updates the DT compatible list for ahci_platform,
> and enable the imx6q ahci sata support.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  drivers/ata/ahci_platform.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 7a8a284..61f2142 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -327,6 +327,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
>
>  static const struct of_device_id ahci_of_match[] = {
>       { .compatible = "snps,spear-ahci", },
> +     { .compatible = "snps,imx-ahci", },

I'm not sure it makes much sense to invent multiple platform specific
strings for an IP which is completely compatible between them.
Why don't we just have a generic compatible string like "snps,ahci"
for all those compatible platforms, and only add platform specific
string when there is incompatibility on particular platform to deal
with?

Added devicetree-discuss to get DT people's opinion.

Shawn

>       {},
>  };
>  MODULE_DEVICE_TABLE(of, ahci_of_match);
> --
> 1.7.5.4
>

[v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Zhu Richard-R65037]

Hi Tejun&Shawn:
Thanks for your comments.

Hi Tejun:
I looked through the for-next branch of the libata git-repo, there maybe a merge conflict.
"
static const struct of_device_id ahci_of_match[] = { 
        { .compatible = "snps,spear-ahci", },
        { .compatible = "snps,exynos5440-ahci", },
        {}, 
};
MODULE_DEVICE_TABLE(of, ahci_of_match);
"

How about to re-change the patch, based on for-next branch of the libata git-repo.

Hi Shawn:
About the one similar IP, multiple platform specific strings inventions.
Waiting for DT's people?s opinion.


Best Regards
Richard Zhu
________________________________________
From: Shawn Guo [shawn.guo at linaro.org]
Sent: Tuesday, June 18, 2013 10:01 AM
To: Richard Zhu
Cc: linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; linux-ide at vger.kernel.org; Zhu Richard-R65037; devicetree-discuss at lists.ozlabs.org
Subject: Re: [v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

On Mon, Jun 17, 2013 at 05:52:46PM +0800, Richard Zhu wrote:
> imx6q contains the Synopsys AHCI SATA controller which shares
> ahci_platform driver with other controllers.
>
> This patch updates the DT compatible list for ahci_platform,
> and enable the imx6q ahci sata support.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  drivers/ata/ahci_platform.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 7a8a284..61f2142 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -327,6 +327,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
>
>  static const struct of_device_id ahci_of_match[] = {
>       { .compatible = "snps,spear-ahci", },
> +     { .compatible = "snps,imx-ahci", },

I'm not sure it makes much sense to invent multiple platform specific
strings for an IP which is completely compatible between them.
Why don't we just have a generic compatible string like "snps,ahci"
for all those compatible platforms, and only add platform specific
string when there is incompatibility on particular platform to deal
with?

Added devicetree-discuss to get DT people's opinion.

Shawn

>       {},
>  };
>  MODULE_DEVICE_TABLE(of, ahci_of_match);
> --
> 1.7.5.4
>

RE: [v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms

[Zhu Richard-R65037]

Hi Shawn:
Thanks for yor comments.
Accepted.

Best Regards
Richard Zhu
________________________________________
From: Shawn Guo [shawn.guo@linaro.org]
Sent: Tuesday, June 18, 2013 10:06 AM
To: Richard Zhu
Cc: linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; linux-ide@vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms

On Mon, Jun 17, 2013 at 05:52:45PM +0800, Richard Zhu wrote:
> Only imx6q has the ahci sata controller, enable
> it on imx6q platforms.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q-sabreauto.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q-sabrelite.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q-sabresd.dts   |    6 ++++++
>  arch/arm/boot/dts/imx6q.dtsi          |    9 +++++++++
>  4 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6q-sabreauto.dts b/arch/arm/boot/dts/imx6q-sabreauto.dts
> index 09a7580..79643cc 100644
> --- a/arch/arm/boot/dts/imx6q-sabreauto.dts
> +++ b/arch/arm/boot/dts/imx6q-sabreauto.dts
> @@ -18,6 +18,12 @@
>  / {
>       model = "Freescale i.MX6 Quad SABRE Automotive Board";
>       compatible = "fsl,imx6q-sabreauto", "fsl,imx6q";
> +
> +     soc {
> +             ahci@02200000 { /* AHCI SATA */
> +                     status = "okay";
> +             };
> +     }
>  };
>
>  &iomuxc {
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 6a00066..dac40af 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -21,6 +21,12 @@
>               reg = <0x10000000 0x40000000>;
>       };
>
> +     soc {
> +             ahci@02200000 { /* AHCI SATA */
> +                     status = "okay";
> +             };
> +     }
> +
>       regulators {
>               compatible = "simple-bus";
>
> diff --git a/arch/arm/boot/dts/imx6q-sabresd.dts b/arch/arm/boot/dts/imx6q-sabresd.dts
> index 0038228..ecae151 100644
> --- a/arch/arm/boot/dts/imx6q-sabresd.dts
> +++ b/arch/arm/boot/dts/imx6q-sabresd.dts
> @@ -18,6 +18,12 @@
>  / {
>       model = "Freescale i.MX6 Quad SABRE Smart Device Board";
>       compatible = "fsl,imx6q-sabresd", "fsl,imx6q";
> +
> +     soc {
> +             ahci@02200000 { /* AHCI SATA */
> +                     status = "okay";
> +             };
> +     };
>  };
>
>  &iomuxc {
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index e7dd2c4..e13ff30 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -424,6 +424,15 @@
>                       };
>               };
>
> +             ahci@02200000 { /* AHCI SATA */

Add a label for the node like "ahci: ahci@02200000", you can refer to
the node simply using the label in board dts, just like every one else
is doing.

&ahci {
        status = "okay";
};

Shawn

> +                     compatible = "snps,imx-ahci";
> +                     reg = <0x02200000 0x4000>;
> +                     interrupts = <0 39 0x04>;
> +                     clocks = <&clks 154>, <&clks 187>;
> +                     clock-names = "sata", "sata_ref_100m";
> +                     status = "disabled";
> +             };
> +
>               ipu2: ipu@02800000 {
>                       #crtc-cells = <1>;
>                       compatible = "fsl,imx6q-ipu";
> --
> 1.7.5.4
>

[v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms

[Zhu Richard-R65037]

Hi Shawn:
Thanks for yor comments.
Accepted.

Best Regards
Richard Zhu
________________________________________
From: Shawn Guo [shawn.guo at linaro.org]
Sent: Tuesday, June 18, 2013 10:06 AM
To: Richard Zhu
Cc: linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; linux-ide at vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v1 1/3] ARM: dtsi: enable ahci sata on imx6q platforms

On Mon, Jun 17, 2013 at 05:52:45PM +0800, Richard Zhu wrote:
> Only imx6q has the ahci sata controller, enable
> it on imx6q platforms.
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q-sabreauto.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q-sabrelite.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q-sabresd.dts   |    6 ++++++
>  arch/arm/boot/dts/imx6q.dtsi          |    9 +++++++++
>  4 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6q-sabreauto.dts b/arch/arm/boot/dts/imx6q-sabreauto.dts
> index 09a7580..79643cc 100644
> --- a/arch/arm/boot/dts/imx6q-sabreauto.dts
> +++ b/arch/arm/boot/dts/imx6q-sabreauto.dts
> @@ -18,6 +18,12 @@
>  / {
>       model = "Freescale i.MX6 Quad SABRE Automotive Board";
>       compatible = "fsl,imx6q-sabreauto", "fsl,imx6q";
> +
> +     soc {
> +             ahci at 02200000 { /* AHCI SATA */
> +                     status = "okay";
> +             };
> +     }
>  };
>
>  &iomuxc {
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 6a00066..dac40af 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -21,6 +21,12 @@
>               reg = <0x10000000 0x40000000>;
>       };
>
> +     soc {
> +             ahci at 02200000 { /* AHCI SATA */
> +                     status = "okay";
> +             };
> +     }
> +
>       regulators {
>               compatible = "simple-bus";
>
> diff --git a/arch/arm/boot/dts/imx6q-sabresd.dts b/arch/arm/boot/dts/imx6q-sabresd.dts
> index 0038228..ecae151 100644
> --- a/arch/arm/boot/dts/imx6q-sabresd.dts
> +++ b/arch/arm/boot/dts/imx6q-sabresd.dts
> @@ -18,6 +18,12 @@
>  / {
>       model = "Freescale i.MX6 Quad SABRE Smart Device Board";
>       compatible = "fsl,imx6q-sabresd", "fsl,imx6q";
> +
> +     soc {
> +             ahci at 02200000 { /* AHCI SATA */
> +                     status = "okay";
> +             };
> +     };
>  };
>
>  &iomuxc {
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index e7dd2c4..e13ff30 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -424,6 +424,15 @@
>                       };
>               };
>
> +             ahci at 02200000 { /* AHCI SATA */

Add a label for the node like "ahci: ahci at 02200000", you can refer to
the node simply using the label in board dts, just like every one else
is doing.

&ahci {
        status = "okay";
};

Shawn

> +                     compatible = "snps,imx-ahci";
> +                     reg = <0x02200000 0x4000>;
> +                     interrupts = <0 39 0x04>;
> +                     clocks = <&clks 154>, <&clks 187>;
> +                     clock-names = "sata", "sata_ref_100m";
> +                     status = "disabled";
> +             };
> +
>               ipu2: ipu at 02800000 {
>                       #crtc-cells = <1>;
>                       compatible = "fsl,imx6q-ipu";
> --
> 1.7.5.4
>

Re: [v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Shawn Guo]

On Tue, Jun 18, 2013 at 02:19:52AM +0000, Zhu Richard-R65037 wrote:
> Hi Tejun&Shawn:
> Thanks for your comments.
> 
> Hi Tejun:
> I looked through the for-next branch of the libata git-repo, there maybe a merge conflict.
> "
> static const struct of_device_id ahci_of_match[] = { 
>         { .compatible = "snps,spear-ahci", },
>         { .compatible = "snps,exynos5440-ahci", },

This is exactly what I'm concerned about.  We will end up with a huge
match table with all different platform specific compatible strings in
there.  But they're all matching to the same programming model of the
same IP block.  That's not how device tree compatible table works.

>         {}, 
> };
> MODULE_DEVICE_TABLE(of, ahci_of_match);
> "
> 
> How about to re-change the patch, based on for-next branch of the libata git-repo.
> 
Yes, I think the driver part and platform part can go separately.

Shawn

[v1 2/3] ata: ahci_platform: enable imx6q ahci sata support

[Shawn Guo]

On Tue, Jun 18, 2013 at 02:19:52AM +0000, Zhu Richard-R65037 wrote:
> Hi Tejun&Shawn:
> Thanks for your comments.
> 
> Hi Tejun:
> I looked through the for-next branch of the libata git-repo, there maybe a merge conflict.
> "
> static const struct of_device_id ahci_of_match[] = { 
>         { .compatible = "snps,spear-ahci", },
>         { .compatible = "snps,exynos5440-ahci", },

This is exactly what I'm concerned about.  We will end up with a huge
match table with all different platform specific compatible strings in
there.  But they're all matching to the same programming model of the
same IP block.  That's not how device tree compatible table works.

>         {}, 
> };
> MODULE_DEVICE_TABLE(of, ahci_of_match);
> "
> 
> How about to re-change the patch, based on for-next branch of the libata git-repo.
> 
Yes, I think the driver part and platform part can go separately.

Shawn

Re: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Shawn Guo]

On Mon, Jun 17, 2013 at 05:52:47PM +0800, Richard Zhu wrote:
> Only the imx6q contains the ahci sata controller,
> other imx6 SoCs don't have it.
> 
> Enable the ahci sata only on imx6q platforms
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c |  179 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 178 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 045e5e3..68b0a45 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -10,6 +10,7 @@
>   * http://www.gnu.org/copyleft/gpl.html
>   */
>  
> +#include <linux/ahci_platform.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> @@ -23,6 +24,7 @@
>  #include <linux/irqchip.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>

Why do you need this header?

>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/opp.h>
> @@ -39,6 +41,22 @@
>  #include "cpuidle.h"
>  #include "hardware.h"
>  
> +#define MX6Q_SATA_BASE_ADDR	0x02200000
> +
> +enum {
> +	HOST_CAP = 0x00,
> +	HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> +	HOST_CTL = 0x04, /* global host control */
> +	HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> +	HOST_PORTS_IMPL	= 0x0c, /* bitmap of implemented ports */
> +	HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> +	HOST_VERSIONR = 0xf8, /* host version register*/
> +
> +	PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> +	PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> +	PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */
> +};
> +

This is all about IP stuff.  Some of them are just replication of
definitions found in drivers/ata/ahci.h. I do not understand why we
need them in platform.  It's a sign to me that we are doing something
in platform code that should really be done in the device driver, no?

>  static u32 chip_revision;
>  
>  int imx6q_revision(void)
> @@ -162,12 +180,171 @@ static void __init imx6q_usb_init(void)
>  	imx_anatop_usb_chrg_detect_disable();
>  }
>  
> +/* imx ahci module initialization. */
> +static int imx_sata_init(struct device *dev, void __iomem *addr)

This is an imx6q specific function, so maybe imx6q_sata_init() is a
better naming?

> +{
> +	int ret = 0, iterations = 100;
> +	struct clk *ahb_clk, *sata_clk, *sata_ref_clk;
> +	struct regmap *gpr;
> +
> +	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +	if (IS_ERR(gpr)) {
> +		pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
> +		return PTR_ERR(gpr);
> +	}
> +
> +	/* enable the clks */
> +	sata_clk = devm_clk_get(dev, "sata");
> +	if (IS_ERR(sata_clk)) {
> +		dev_err(dev, "can't get sata clock.\n");
> +		return PTR_ERR(sata_clk);
> +	}

I think ahci_platform.c is already managing this clock as hpriv->clk?

> +	ret = clk_prepare_enable(sata_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "can't prepare-enable sata clock\n");
> +		clk_put(sata_clk);
> +		return ret;
> +	}
> +	sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> +	if (IS_ERR(sata_ref_clk)) {
> +		dev_err(dev, "can't get sata_ref clock.\n");
> +		ret = PTR_ERR(sata_ref_clk);
> +		goto release_sata_clk;
> +	}
> +	ret = clk_prepare_enable(sata_ref_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "can't prepare-enable sata_ref clock\n");
> +		clk_put(sata_ref_clk);
> +		ret = PTR_ERR(sata_ref_clk);
> +		goto release_sata_clk;
> +	}

Doesn't ATA always need a PHY clock to function?  If so, can we
possibly manage this clock in ahci_platform driver as well as
hpriv->phy_clk?

> +
> +	/* set PHY Paremeters, two steps to configure the GPR13,
> +	 * one write for rest of parameters, mask of first write is 0x07fffffd,
> +	 * and the other one write for setting the mpll_clk_off_b
> +	 *.rx_eq_val_0(iomuxc_gpr13[26:24]),
> +	 *.los_lvl(iomuxc_gpr13[23:19]),
> +	 *.rx_dpll_mode_0(iomuxc_gpr13[18:16]),
> +	 *.mpll_ss_en(iomuxc_gpr13[14]),
> +	 *.tx_atten_0(iomuxc_gpr13[13:11]),
> +	 *.tx_boost_0(iomuxc_gpr13[10:7]),
> +	 *.tx_lvl(iomuxc_gpr13[6:2]),
> +	 *.mpll_clk_off(iomuxc_gpr13[1]),
> +	 *.tx_edgerate_0(iomuxc_gpr13[0]),
> +	 */

	/*
	 * multiple-lines comments
	 * ...
	 */

> +	regmap_update_bits(gpr, 0x34, 0x07fffffd, 0x0593e4c4);
> +	regmap_update_bits(gpr, 0x34, 0x2, 0x2);

Ok, this is platforms stuff and should be done in platform code.  Can
those macros defined in include/linux/mfd/syscon/imx6q-iomuxc-gpr.h be
useful?

> +
> +	/*  reset hba */
          ^^
Nit: one space is enough.

> +	writel(HOST_RESET, addr + HOST_CTL);
> +	ret = 0;
> +	while (readl(addr + HOST_VERSIONR) == 0) {
> +		ret++;
> +		usleep_range(1, 2);
> +		if (ret > 100) {
> +			dev_info(dev, "can't recover from reset hba!\n");
> +			break;
> +		}
> +	}

I failed to see why this can not be done in ahci_platform driver.

> +
> +	/* get the ahb clock rate, and configure the TIMER1MS reg later */
> +	ahb_clk = clk_get_sys(NULL, "ahb");
> +	if (IS_ERR(ahb_clk)) {
> +		dev_err(dev, "no ahb clock.\n");
> +		ret = PTR_ERR(ahb_clk);
> +		goto error;
> +	}
> +	ret = clk_get_rate(ahb_clk) / 1000;
> +	clk_put(ahb_clk);
> +	writel(ret, addr + HOST_TIMER1MS);

Why can not we define a clock source for TIMER1MS and get this work done
in ahci_platform driver?

> +
> +	/*
> +	 * configure CAP_SSS (support stagered spin up),
> +	 * and implement the port0
> +	 */
> +	ret = readl(addr + HOST_CAP);
> +	if (!(ret & HOST_CAP_SSS))
> +		writel(ret |= HOST_CAP_SSS, addr + HOST_CAP);
> +	ret = readl(addr + HOST_PORTS_IMPL);
> +	if (!(ret & 0x1))
> +		writel((ret | 0x1), addr + HOST_PORTS_IMPL);
> +

Same question: why can not it be done in ahci_platform driver?

> +	/*
> +	 * no device detected on the port, in order to save the power
> +	 * consumption, enter into iddq mode(power down circuitry)
> +	 * and release the clocks.
> +	 * NOTE:in this case, the sata port can't be used anymore
> +	 * without one system reboot.
> +	 */
> +	do {
> +		if ((readl(addr + PORT_SATA_SR) & 0xF) == 0)
> +			usleep_range(100, 200);
> +		else
> +			break;
> +
> +		if (iterations == 0) {
> +			/*  enter into iddq mode, save power */
> +			pr_info("no sata disk, enter into pddq mode.\n");
> +			ret = readl(addr + PORT_PHY_CTL);
> +			ret |= PORT_PHY_CTL_PDDQ_LOC;
> +			writel(ret, addr + PORT_PHY_CTL);
> +			ret = -ENODEV;
> +			goto error;
> +		}
> +	} while (iterations-- > 0);

Ditto

Shawn

> +
> +	return 0;
> +
> +error:
> +	regmap_update_bits(gpr, 0x34, 0x2, 0x0);
> +	clk_disable_unprepare(sata_ref_clk);
> +release_sata_clk:
> +	clk_disable_unprepare(sata_clk);
> +
> +	return ret;
> +}
> +
> +static void imx_sata_exit(struct device *dev)
> +{
> +	struct clk *sata_clk, *sata_ref_clk;
> +	struct regmap *gpr;
> +
> +	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +	if (IS_ERR(gpr))
> +		pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
> +
> +	regmap_update_bits(gpr, 0x34, 0x2, 0x0);
> +
> +	sata_clk = devm_clk_get(dev, "sata");
> +	if (IS_ERR(sata_clk))
> +		dev_err(dev, "can't get sata clock.\n");
> +	else
> +		clk_disable_unprepare(sata_clk);
> +	sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> +	if (IS_ERR(sata_ref_clk))
> +		dev_err(dev, "can't get sata_ref clock.\n");
> +	else
> +		clk_disable_unprepare(sata_ref_clk);
> +}
> +
> +static struct ahci_platform_data imx_sata_pdata = {
> +	.init = imx_sata_init,
> +	.exit = imx_sata_exit,
> +};
> +
> +static const struct of_dev_auxdata imx6q_auxdata_lookup[] __initconst = {
> +	OF_DEV_AUXDATA("snps,imx-ahci", MX6Q_SATA_BASE_ADDR, "imx-ahci",
> +			&imx_sata_pdata),
> +	{ /* sentinel */ }
> +};
> +
>  static void __init imx6q_init_machine(void)
>  {
>  	if (of_machine_is_compatible("fsl,imx6q-sabrelite"))
>  		imx6q_sabrelite_init();
>  
> -	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			imx6q_auxdata_lookup, NULL);
>  
>  	imx_anatop_init();
>  	imx6q_pm_init();
> -- 
> 1.7.5.4
> 

[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Shawn Guo]

On Mon, Jun 17, 2013 at 05:52:47PM +0800, Richard Zhu wrote:
> Only the imx6q contains the ahci sata controller,
> other imx6 SoCs don't have it.
> 
> Enable the ahci sata only on imx6q platforms
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c |  179 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 178 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 045e5e3..68b0a45 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -10,6 +10,7 @@
>   * http://www.gnu.org/copyleft/gpl.html
>   */
>  
> +#include <linux/ahci_platform.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> @@ -23,6 +24,7 @@
>  #include <linux/irqchip.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>

Why do you need this header?

>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/opp.h>
> @@ -39,6 +41,22 @@
>  #include "cpuidle.h"
>  #include "hardware.h"
>  
> +#define MX6Q_SATA_BASE_ADDR	0x02200000
> +
> +enum {
> +	HOST_CAP = 0x00,
> +	HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> +	HOST_CTL = 0x04, /* global host control */
> +	HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> +	HOST_PORTS_IMPL	= 0x0c, /* bitmap of implemented ports */
> +	HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> +	HOST_VERSIONR = 0xf8, /* host version register*/
> +
> +	PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> +	PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> +	PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */
> +};
> +

This is all about IP stuff.  Some of them are just replication of
definitions found in drivers/ata/ahci.h. I do not understand why we
need them in platform.  It's a sign to me that we are doing something
in platform code that should really be done in the device driver, no?

>  static u32 chip_revision;
>  
>  int imx6q_revision(void)
> @@ -162,12 +180,171 @@ static void __init imx6q_usb_init(void)
>  	imx_anatop_usb_chrg_detect_disable();
>  }
>  
> +/* imx ahci module initialization. */
> +static int imx_sata_init(struct device *dev, void __iomem *addr)

This is an imx6q specific function, so maybe imx6q_sata_init() is a
better naming?

> +{
> +	int ret = 0, iterations = 100;
> +	struct clk *ahb_clk, *sata_clk, *sata_ref_clk;
> +	struct regmap *gpr;
> +
> +	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +	if (IS_ERR(gpr)) {
> +		pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
> +		return PTR_ERR(gpr);
> +	}
> +
> +	/* enable the clks */
> +	sata_clk = devm_clk_get(dev, "sata");
> +	if (IS_ERR(sata_clk)) {
> +		dev_err(dev, "can't get sata clock.\n");
> +		return PTR_ERR(sata_clk);
> +	}

I think ahci_platform.c is already managing this clock as hpriv->clk?

> +	ret = clk_prepare_enable(sata_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "can't prepare-enable sata clock\n");
> +		clk_put(sata_clk);
> +		return ret;
> +	}
> +	sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> +	if (IS_ERR(sata_ref_clk)) {
> +		dev_err(dev, "can't get sata_ref clock.\n");
> +		ret = PTR_ERR(sata_ref_clk);
> +		goto release_sata_clk;
> +	}
> +	ret = clk_prepare_enable(sata_ref_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "can't prepare-enable sata_ref clock\n");
> +		clk_put(sata_ref_clk);
> +		ret = PTR_ERR(sata_ref_clk);
> +		goto release_sata_clk;
> +	}

Doesn't ATA always need a PHY clock to function?  If so, can we
possibly manage this clock in ahci_platform driver as well as
hpriv->phy_clk?

> +
> +	/* set PHY Paremeters, two steps to configure the GPR13,
> +	 * one write for rest of parameters, mask of first write is 0x07fffffd,
> +	 * and the other one write for setting the mpll_clk_off_b
> +	 *.rx_eq_val_0(iomuxc_gpr13[26:24]),
> +	 *.los_lvl(iomuxc_gpr13[23:19]),
> +	 *.rx_dpll_mode_0(iomuxc_gpr13[18:16]),
> +	 *.mpll_ss_en(iomuxc_gpr13[14]),
> +	 *.tx_atten_0(iomuxc_gpr13[13:11]),
> +	 *.tx_boost_0(iomuxc_gpr13[10:7]),
> +	 *.tx_lvl(iomuxc_gpr13[6:2]),
> +	 *.mpll_clk_off(iomuxc_gpr13[1]),
> +	 *.tx_edgerate_0(iomuxc_gpr13[0]),
> +	 */

	/*
	 * multiple-lines comments
	 * ...
	 */

> +	regmap_update_bits(gpr, 0x34, 0x07fffffd, 0x0593e4c4);
> +	regmap_update_bits(gpr, 0x34, 0x2, 0x2);

Ok, this is platforms stuff and should be done in platform code.  Can
those macros defined in include/linux/mfd/syscon/imx6q-iomuxc-gpr.h be
useful?

> +
> +	/*  reset hba */
          ^^
Nit: one space is enough.

> +	writel(HOST_RESET, addr + HOST_CTL);
> +	ret = 0;
> +	while (readl(addr + HOST_VERSIONR) == 0) {
> +		ret++;
> +		usleep_range(1, 2);
> +		if (ret > 100) {
> +			dev_info(dev, "can't recover from reset hba!\n");
> +			break;
> +		}
> +	}

I failed to see why this can not be done in ahci_platform driver.

> +
> +	/* get the ahb clock rate, and configure the TIMER1MS reg later */
> +	ahb_clk = clk_get_sys(NULL, "ahb");
> +	if (IS_ERR(ahb_clk)) {
> +		dev_err(dev, "no ahb clock.\n");
> +		ret = PTR_ERR(ahb_clk);
> +		goto error;
> +	}
> +	ret = clk_get_rate(ahb_clk) / 1000;
> +	clk_put(ahb_clk);
> +	writel(ret, addr + HOST_TIMER1MS);

Why can not we define a clock source for TIMER1MS and get this work done
in ahci_platform driver?

> +
> +	/*
> +	 * configure CAP_SSS (support stagered spin up),
> +	 * and implement the port0
> +	 */
> +	ret = readl(addr + HOST_CAP);
> +	if (!(ret & HOST_CAP_SSS))
> +		writel(ret |= HOST_CAP_SSS, addr + HOST_CAP);
> +	ret = readl(addr + HOST_PORTS_IMPL);
> +	if (!(ret & 0x1))
> +		writel((ret | 0x1), addr + HOST_PORTS_IMPL);
> +

Same question: why can not it be done in ahci_platform driver?

> +	/*
> +	 * no device detected on the port, in order to save the power
> +	 * consumption, enter into iddq mode(power down circuitry)
> +	 * and release the clocks.
> +	 * NOTE:in this case, the sata port can't be used anymore
> +	 * without one system reboot.
> +	 */
> +	do {
> +		if ((readl(addr + PORT_SATA_SR) & 0xF) == 0)
> +			usleep_range(100, 200);
> +		else
> +			break;
> +
> +		if (iterations == 0) {
> +			/*  enter into iddq mode, save power */
> +			pr_info("no sata disk, enter into pddq mode.\n");
> +			ret = readl(addr + PORT_PHY_CTL);
> +			ret |= PORT_PHY_CTL_PDDQ_LOC;
> +			writel(ret, addr + PORT_PHY_CTL);
> +			ret = -ENODEV;
> +			goto error;
> +		}
> +	} while (iterations-- > 0);

Ditto

Shawn

> +
> +	return 0;
> +
> +error:
> +	regmap_update_bits(gpr, 0x34, 0x2, 0x0);
> +	clk_disable_unprepare(sata_ref_clk);
> +release_sata_clk:
> +	clk_disable_unprepare(sata_clk);
> +
> +	return ret;
> +}
> +
> +static void imx_sata_exit(struct device *dev)
> +{
> +	struct clk *sata_clk, *sata_ref_clk;
> +	struct regmap *gpr;
> +
> +	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +	if (IS_ERR(gpr))
> +		pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
> +
> +	regmap_update_bits(gpr, 0x34, 0x2, 0x0);
> +
> +	sata_clk = devm_clk_get(dev, "sata");
> +	if (IS_ERR(sata_clk))
> +		dev_err(dev, "can't get sata clock.\n");
> +	else
> +		clk_disable_unprepare(sata_clk);
> +	sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> +	if (IS_ERR(sata_ref_clk))
> +		dev_err(dev, "can't get sata_ref clock.\n");
> +	else
> +		clk_disable_unprepare(sata_ref_clk);
> +}
> +
> +static struct ahci_platform_data imx_sata_pdata = {
> +	.init = imx_sata_init,
> +	.exit = imx_sata_exit,
> +};
> +
> +static const struct of_dev_auxdata imx6q_auxdata_lookup[] __initconst = {
> +	OF_DEV_AUXDATA("snps,imx-ahci", MX6Q_SATA_BASE_ADDR, "imx-ahci",
> +			&imx_sata_pdata),
> +	{ /* sentinel */ }
> +};
> +
>  static void __init imx6q_init_machine(void)
>  {
>  	if (of_machine_is_compatible("fsl,imx6q-sabrelite"))
>  		imx6q_sabrelite_init();
>  
> -	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			imx6q_auxdata_lookup, NULL);
>  
>  	imx_anatop_init();
>  	imx6q_pm_init();
> -- 
> 1.7.5.4
> 

RE: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Zhu Richard-R65037]

Hi Shawn:
Firstly, thanks for your review and comments.

Best Regards
Richard Zhu
________________________________________
From: Shawn Guo [shawn.guo@linaro.org]
Sent: Tuesday, June 18, 2013 11:03 AM
To: Richard Zhu
Cc: linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; linux-ide@vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

On Mon, Jun 17, 2013 at 05:52:47PM +0800, Richard Zhu wrote:
> Only the imx6q contains the ahci sata controller,
> other imx6 SoCs don't have it.
>
> Enable the ahci sata only on imx6q platforms
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c |  179 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 178 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 045e5e3..68b0a45 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -10,6 +10,7 @@
>   * http://www.gnu.org/copyleft/gpl.html
>   */
>
> +#include <linux/ahci_platform.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> @@ -23,6 +24,7 @@
>  #include <linux/irqchip.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>

Why do you need this header?
[Richard] mis-clean up it, when remove the codes of the gpio related device node.
Will remove it later.

>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/opp.h>
> @@ -39,6 +41,22 @@
>  #include "cpuidle.h"
>  #include "hardware.h"
>
> +#define MX6Q_SATA_BASE_ADDR  0x02200000
> +
> +enum {
> +     HOST_CAP = 0x00,
> +     HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> +     HOST_CTL = 0x04, /* global host control */
> +     HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> +     HOST_PORTS_IMPL = 0x0c, /* bitmap of implemented ports */
> +     HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> +     HOST_VERSIONR = 0xf8, /* host version register*/
> +
> +     PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> +     PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> +     PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */
> +};
> +

This is all about IP stuff.  Some of them are just replication of
definitions found in drivers/ata/ahci.h. I do not understand why we
need them in platform.  It's a sign to me that we are doing something
in platform code that should really be done in the device driver, no?

[Richard] Regarding to the AHCI SPEC, some bits of the ahci sata registers are
 specified to be RO, but they are RW in the design of fsl ahci ata in actually.
 So the configurations of those bits are mandatory required in the
 platform sata initialization. 
One HBA reset is better finished before start the configuration.
That's why those registers/bits are re-defined in the platform level.

For, e.x:
bit1 of the HOST_POSRTS_IMPL, HOST_CAP_SSS(bit27) of the HOST_CAP, are RO specified
in the AHCI SPEC. But they should be configured in the sata platform initialization.

BTW, HOST_TIMER1MS is defined by FSL, can't be used in the common codes.
regarding to the AHCI SPEC.
" Registers at offset A0h to FFh are vendor specific"

>  static u32 chip_revision;
>
>  int imx6q_revision(void)
> @@ -162,12 +180,171 @@ static void __init imx6q_usb_init(void)
>       imx_anatop_usb_chrg_detect_disable();
>  }
>
> +/* imx ahci module initialization. */
> +static int imx_sata_init(struct device *dev, void __iomem *addr)

This is an imx6q specific function, so maybe imx6q_sata_init() is a
better naming?
[Richard] Accepted.

> +{
> +     int ret = 0, iterations = 100;
> +     struct clk *ahb_clk, *sata_clk, *sata_ref_clk;
> +     struct regmap *gpr;
> +
> +     gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +     if (IS_ERR(gpr)) {
> +             pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
> +             return PTR_ERR(gpr);
> +     }
> +
> +     /* enable the clks */
> +     sata_clk = devm_clk_get(dev, "sata");
> +     if (IS_ERR(sata_clk)) {
> +             dev_err(dev, "can't get sata clock.\n");
> +             return PTR_ERR(sata_clk);
> +     }

I think ahci_platform.c is already managing this clock as hpriv->clk?
[Richard]Accepted.

> +     ret = clk_prepare_enable(sata_clk);
> +     if (ret < 0) {
> +             dev_err(dev, "can't prepare-enable sata clock\n");
> +             clk_put(sata_clk);
> +             return ret;
> +     }
> +     sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> +     if (IS_ERR(sata_ref_clk)) {
> +             dev_err(dev, "can't get sata_ref clock.\n");
> +             ret = PTR_ERR(sata_ref_clk);
> +             goto release_sata_clk;
> +     }
> +     ret = clk_prepare_enable(sata_ref_clk);
> +     if (ret < 0) {
> +             dev_err(dev, "can't prepare-enable sata_ref clock\n");
> +             clk_put(sata_ref_clk);
> +             ret = PTR_ERR(sata_ref_clk);
> +             goto release_sata_clk;
> +     }

Doesn't ATA always need a PHY clock to function?  If so, can we
possibly manage this clock in ahci_platform driver as well as
hpriv->phy_clk?
[Richard] No, it is not always one PHY clock to function.
this is defined by the IP design of the vendors.

> +
> +     /* set PHY Paremeters, two steps to configure the GPR13,
> +      * one write for rest of parameters, mask of first write is 0x07fffffd,
> +      * and the other one write for setting the mpll_clk_off_b
> +      *.rx_eq_val_0(iomuxc_gpr13[26:24]),
> +      *.los_lvl(iomuxc_gpr13[23:19]),
> +      *.rx_dpll_mode_0(iomuxc_gpr13[18:16]),
> +      *.mpll_ss_en(iomuxc_gpr13[14]),
> +      *.tx_atten_0(iomuxc_gpr13[13:11]),
> +      *.tx_boost_0(iomuxc_gpr13[10:7]),
> +      *.tx_lvl(iomuxc_gpr13[6:2]),
> +      *.mpll_clk_off(iomuxc_gpr13[1]),
> +      *.tx_edgerate_0(iomuxc_gpr13[0]),
> +      */

        /*
         * multiple-lines comments
         * ...
         */

> +     regmap_update_bits(gpr, 0x34, 0x07fffffd, 0x0593e4c4);
> +     regmap_update_bits(gpr, 0x34, 0x2, 0x2);

Ok, this is platforms stuff and should be done in platform code.  Can
those macros defined in include/linux/mfd/syscon/imx6q-iomuxc-gpr.h be
useful?
[Richard] Accepted.

> +
> +     /*  reset hba */
          ^^
Nit: one space is enough.
[Richard] Accepted.

> +     writel(HOST_RESET, addr + HOST_CTL);
> +     ret = 0;
> +     while (readl(addr + HOST_VERSIONR) == 0) {
> +             ret++;
> +             usleep_range(1, 2);
> +             if (ret > 100) {
> +                     dev_info(dev, "can't recover from reset hba!\n");
> +                     break;
> +             }
> +     }

I failed to see why this can not be done in ahci_platform driver.
[Richard] See the second comment listed above.

> +
> +     /* get the ahb clock rate, and configure the TIMER1MS reg later */
> +     ahb_clk = clk_get_sys(NULL, "ahb");
> +     if (IS_ERR(ahb_clk)) {
> +             dev_err(dev, "no ahb clock.\n");
> +             ret = PTR_ERR(ahb_clk);
> +             goto error;
> +     }
> +     ret = clk_get_rate(ahb_clk) / 1000;
> +     clk_put(ahb_clk);
> +     writel(ret, addr + HOST_TIMER1MS);

Why can not we define a clock source for TIMER1MS and get this work done
in ahci_platform driver?
[Richard]It's better don't do that, because that the HOST_TIMER1MS
 is special defined by FSL SATA AHCI IP.
BTW, regarding to the AHCI SPEC.
" Registers at offset A0h to FFh are vendor specific"
So, this register is not common, is better to configure it in the platform level.
How do you think about that?

> +
> +     /*
> +      * configure CAP_SSS (support stagered spin up),
> +      * and implement the port0
> +      */
> +     ret = readl(addr + HOST_CAP);
> +     if (!(ret & HOST_CAP_SSS))
> +             writel(ret |= HOST_CAP_SSS, addr + HOST_CAP);
> +     ret = readl(addr + HOST_PORTS_IMPL);
> +     if (!(ret & 0x1))
> +             writel((ret | 0x1), addr + HOST_PORTS_IMPL);
> +

Same question: why can not it be done in ahci_platform driver?
[Richard] See the second comment.

> +     /*
> +      * no device detected on the port, in order to save the power
> +      * consumption, enter into iddq mode(power down circuitry)
> +      * and release the clocks.
> +      * NOTE:in this case, the sata port can't be used anymore
> +      * without one system reboot.
> +      */
> +     do {
> +             if ((readl(addr + PORT_SATA_SR) & 0xF) == 0)
> +                     usleep_range(100, 200);
> +             else
> +                     break;
> +
> +             if (iterations == 0) {
> +                     /*  enter into iddq mode, save power */
> +                     pr_info("no sata disk, enter into pddq mode.\n");
> +                     ret = readl(addr + PORT_PHY_CTL);
> +                     ret |= PORT_PHY_CTL_PDDQ_LOC;
> +                     writel(ret, addr + PORT_PHY_CTL);
> +                     ret = -ENODEV;
> +                     goto error;
> +             }
> +     } while (iterations-- > 0);

Ditto
[Richard]In order to save power consumption when there is no device detected on the port, 
these codes is used to enter into pddq mode specified by FSL ahci sata IP. Not defined
to the common usage. Implement in the platform level.

Shawn

> +
> +     return 0;
> +
> +error:
> +     regmap_update_bits(gpr, 0x34, 0x2, 0x0);
> +     clk_disable_unprepare(sata_ref_clk);
> +release_sata_clk:
> +     clk_disable_unprepare(sata_clk);
> +
> +     return ret;
> +}
> +
> +static void imx_sata_exit(struct device *dev)
> +{
> +     struct clk *sata_clk, *sata_ref_clk;
> +     struct regmap *gpr;
> +
> +     gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +     if (IS_ERR(gpr))
> +             pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
> +
> +     regmap_update_bits(gpr, 0x34, 0x2, 0x0);
> +
> +     sata_clk = devm_clk_get(dev, "sata");
> +     if (IS_ERR(sata_clk))
> +             dev_err(dev, "can't get sata clock.\n");
> +     else
> +             clk_disable_unprepare(sata_clk);
> +     sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> +     if (IS_ERR(sata_ref_clk))
> +             dev_err(dev, "can't get sata_ref clock.\n");
> +     else
> +             clk_disable_unprepare(sata_ref_clk);
> +}
> +
> +static struct ahci_platform_data imx_sata_pdata = {
> +     .init = imx_sata_init,
> +     .exit = imx_sata_exit,
> +};
> +
> +static const struct of_dev_auxdata imx6q_auxdata_lookup[] __initconst = {
> +     OF_DEV_AUXDATA("snps,imx-ahci", MX6Q_SATA_BASE_ADDR, "imx-ahci",
> +                     &imx_sata_pdata),
> +     { /* sentinel */ }
> +};
> +
>  static void __init imx6q_init_machine(void)
>  {
>       if (of_machine_is_compatible("fsl,imx6q-sabrelite"))
>               imx6q_sabrelite_init();
>
> -     of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +     of_platform_populate(NULL, of_default_bus_match_table,
> +                     imx6q_auxdata_lookup, NULL);
>
>       imx_anatop_init();
>       imx6q_pm_init();
> --
> 1.7.5.4
>

[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Zhu Richard-R65037]

Hi Shawn:
Firstly, thanks for your review and comments.

Best Regards
Richard Zhu
________________________________________
From: Shawn Guo [shawn.guo at linaro.org]
Sent: Tuesday, June 18, 2013 11:03 AM
To: Richard Zhu
Cc: linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; linux-ide at vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

On Mon, Jun 17, 2013 at 05:52:47PM +0800, Richard Zhu wrote:
> Only the imx6q contains the ahci sata controller,
> other imx6 SoCs don't have it.
>
> Enable the ahci sata only on imx6q platforms
>
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c |  179 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 178 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 045e5e3..68b0a45 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -10,6 +10,7 @@
>   * http://www.gnu.org/copyleft/gpl.html
>   */
>
> +#include <linux/ahci_platform.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> @@ -23,6 +24,7 @@
>  #include <linux/irqchip.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>

Why do you need this header?
[Richard] mis-clean up it, when remove the codes of the gpio related device node.
Will remove it later.

>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/opp.h>
> @@ -39,6 +41,22 @@
>  #include "cpuidle.h"
>  #include "hardware.h"
>
> +#define MX6Q_SATA_BASE_ADDR  0x02200000
> +
> +enum {
> +     HOST_CAP = 0x00,
> +     HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> +     HOST_CTL = 0x04, /* global host control */
> +     HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> +     HOST_PORTS_IMPL = 0x0c, /* bitmap of implemented ports */
> +     HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> +     HOST_VERSIONR = 0xf8, /* host version register*/
> +
> +     PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> +     PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> +     PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */
> +};
> +

This is all about IP stuff.  Some of them are just replication of
definitions found in drivers/ata/ahci.h. I do not understand why we
need them in platform.  It's a sign to me that we are doing something
in platform code that should really be done in the device driver, no?

[Richard] Regarding to the AHCI SPEC, some bits of the ahci sata registers are
 specified to be RO, but they are RW in the design of fsl ahci ata in actually.
 So the configurations of those bits are mandatory required in the
 platform sata initialization. 
One HBA reset is better finished before start the configuration.
That's why those registers/bits are re-defined in the platform level.

For, e.x:
bit1 of the HOST_POSRTS_IMPL, HOST_CAP_SSS(bit27) of the HOST_CAP, are RO specified
in the AHCI SPEC. But they should be configured in the sata platform initialization.

BTW, HOST_TIMER1MS is defined by FSL, can't be used in the common codes.
regarding to the AHCI SPEC.
" Registers at offset A0h to FFh are vendor specific"

>  static u32 chip_revision;
>
>  int imx6q_revision(void)
> @@ -162,12 +180,171 @@ static void __init imx6q_usb_init(void)
>       imx_anatop_usb_chrg_detect_disable();
>  }
>
> +/* imx ahci module initialization. */
> +static int imx_sata_init(struct device *dev, void __iomem *addr)

This is an imx6q specific function, so maybe imx6q_sata_init() is a
better naming?
[Richard] Accepted.

> +{
> +     int ret = 0, iterations = 100;
> +     struct clk *ahb_clk, *sata_clk, *sata_ref_clk;
> +     struct regmap *gpr;
> +
> +     gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +     if (IS_ERR(gpr)) {
> +             pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
> +             return PTR_ERR(gpr);
> +     }
> +
> +     /* enable the clks */
> +     sata_clk = devm_clk_get(dev, "sata");
> +     if (IS_ERR(sata_clk)) {
> +             dev_err(dev, "can't get sata clock.\n");
> +             return PTR_ERR(sata_clk);
> +     }

I think ahci_platform.c is already managing this clock as hpriv->clk?
[Richard]Accepted.

> +     ret = clk_prepare_enable(sata_clk);
> +     if (ret < 0) {
> +             dev_err(dev, "can't prepare-enable sata clock\n");
> +             clk_put(sata_clk);
> +             return ret;
> +     }
> +     sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> +     if (IS_ERR(sata_ref_clk)) {
> +             dev_err(dev, "can't get sata_ref clock.\n");
> +             ret = PTR_ERR(sata_ref_clk);
> +             goto release_sata_clk;
> +     }
> +     ret = clk_prepare_enable(sata_ref_clk);
> +     if (ret < 0) {
> +             dev_err(dev, "can't prepare-enable sata_ref clock\n");
> +             clk_put(sata_ref_clk);
> +             ret = PTR_ERR(sata_ref_clk);
> +             goto release_sata_clk;
> +     }

Doesn't ATA always need a PHY clock to function?  If so, can we
possibly manage this clock in ahci_platform driver as well as
hpriv->phy_clk?
[Richard] No, it is not always one PHY clock to function.
this is defined by the IP design of the vendors.

> +
> +     /* set PHY Paremeters, two steps to configure the GPR13,
> +      * one write for rest of parameters, mask of first write is 0x07fffffd,
> +      * and the other one write for setting the mpll_clk_off_b
> +      *.rx_eq_val_0(iomuxc_gpr13[26:24]),
> +      *.los_lvl(iomuxc_gpr13[23:19]),
> +      *.rx_dpll_mode_0(iomuxc_gpr13[18:16]),
> +      *.mpll_ss_en(iomuxc_gpr13[14]),
> +      *.tx_atten_0(iomuxc_gpr13[13:11]),
> +      *.tx_boost_0(iomuxc_gpr13[10:7]),
> +      *.tx_lvl(iomuxc_gpr13[6:2]),
> +      *.mpll_clk_off(iomuxc_gpr13[1]),
> +      *.tx_edgerate_0(iomuxc_gpr13[0]),
> +      */

        /*
         * multiple-lines comments
         * ...
         */

> +     regmap_update_bits(gpr, 0x34, 0x07fffffd, 0x0593e4c4);
> +     regmap_update_bits(gpr, 0x34, 0x2, 0x2);

Ok, this is platforms stuff and should be done in platform code.  Can
those macros defined in include/linux/mfd/syscon/imx6q-iomuxc-gpr.h be
useful?
[Richard] Accepted.

> +
> +     /*  reset hba */
          ^^
Nit: one space is enough.
[Richard] Accepted.

> +     writel(HOST_RESET, addr + HOST_CTL);
> +     ret = 0;
> +     while (readl(addr + HOST_VERSIONR) == 0) {
> +             ret++;
> +             usleep_range(1, 2);
> +             if (ret > 100) {
> +                     dev_info(dev, "can't recover from reset hba!\n");
> +                     break;
> +             }
> +     }

I failed to see why this can not be done in ahci_platform driver.
[Richard] See the second comment listed above.

> +
> +     /* get the ahb clock rate, and configure the TIMER1MS reg later */
> +     ahb_clk = clk_get_sys(NULL, "ahb");
> +     if (IS_ERR(ahb_clk)) {
> +             dev_err(dev, "no ahb clock.\n");
> +             ret = PTR_ERR(ahb_clk);
> +             goto error;
> +     }
> +     ret = clk_get_rate(ahb_clk) / 1000;
> +     clk_put(ahb_clk);
> +     writel(ret, addr + HOST_TIMER1MS);

Why can not we define a clock source for TIMER1MS and get this work done
in ahci_platform driver?
[Richard]It's better don't do that, because that the HOST_TIMER1MS
 is special defined by FSL SATA AHCI IP.
BTW, regarding to the AHCI SPEC.
" Registers at offset A0h to FFh are vendor specific"
So, this register is not common, is better to configure it in the platform level.
How do you think about that?

> +
> +     /*
> +      * configure CAP_SSS (support stagered spin up),
> +      * and implement the port0
> +      */
> +     ret = readl(addr + HOST_CAP);
> +     if (!(ret & HOST_CAP_SSS))
> +             writel(ret |= HOST_CAP_SSS, addr + HOST_CAP);
> +     ret = readl(addr + HOST_PORTS_IMPL);
> +     if (!(ret & 0x1))
> +             writel((ret | 0x1), addr + HOST_PORTS_IMPL);
> +

Same question: why can not it be done in ahci_platform driver?
[Richard] See the second comment.

> +     /*
> +      * no device detected on the port, in order to save the power
> +      * consumption, enter into iddq mode(power down circuitry)
> +      * and release the clocks.
> +      * NOTE:in this case, the sata port can't be used anymore
> +      * without one system reboot.
> +      */
> +     do {
> +             if ((readl(addr + PORT_SATA_SR) & 0xF) == 0)
> +                     usleep_range(100, 200);
> +             else
> +                     break;
> +
> +             if (iterations == 0) {
> +                     /*  enter into iddq mode, save power */
> +                     pr_info("no sata disk, enter into pddq mode.\n");
> +                     ret = readl(addr + PORT_PHY_CTL);
> +                     ret |= PORT_PHY_CTL_PDDQ_LOC;
> +                     writel(ret, addr + PORT_PHY_CTL);
> +                     ret = -ENODEV;
> +                     goto error;
> +             }
> +     } while (iterations-- > 0);

Ditto
[Richard]In order to save power consumption when there is no device detected on the port, 
these codes is used to enter into pddq mode specified by FSL ahci sata IP. Not defined
to the common usage. Implement in the platform level.

Shawn

> +
> +     return 0;
> +
> +error:
> +     regmap_update_bits(gpr, 0x34, 0x2, 0x0);
> +     clk_disable_unprepare(sata_ref_clk);
> +release_sata_clk:
> +     clk_disable_unprepare(sata_clk);
> +
> +     return ret;
> +}
> +
> +static void imx_sata_exit(struct device *dev)
> +{
> +     struct clk *sata_clk, *sata_ref_clk;
> +     struct regmap *gpr;
> +
> +     gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +     if (IS_ERR(gpr))
> +             pr_err("failed to find fsl,imx6q-iomux-gpr regmap\n");
> +
> +     regmap_update_bits(gpr, 0x34, 0x2, 0x0);
> +
> +     sata_clk = devm_clk_get(dev, "sata");
> +     if (IS_ERR(sata_clk))
> +             dev_err(dev, "can't get sata clock.\n");
> +     else
> +             clk_disable_unprepare(sata_clk);
> +     sata_ref_clk = devm_clk_get(dev, "sata_ref_100m");
> +     if (IS_ERR(sata_ref_clk))
> +             dev_err(dev, "can't get sata_ref clock.\n");
> +     else
> +             clk_disable_unprepare(sata_ref_clk);
> +}
> +
> +static struct ahci_platform_data imx_sata_pdata = {
> +     .init = imx_sata_init,
> +     .exit = imx_sata_exit,
> +};
> +
> +static const struct of_dev_auxdata imx6q_auxdata_lookup[] __initconst = {
> +     OF_DEV_AUXDATA("snps,imx-ahci", MX6Q_SATA_BASE_ADDR, "imx-ahci",
> +                     &imx_sata_pdata),
> +     { /* sentinel */ }
> +};
> +
>  static void __init imx6q_init_machine(void)
>  {
>       if (of_machine_is_compatible("fsl,imx6q-sabrelite"))
>               imx6q_sabrelite_init();
>
> -     of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +     of_platform_populate(NULL, of_default_bus_match_table,
> +                     imx6q_auxdata_lookup, NULL);
>
>       imx_anatop_init();
>       imx6q_pm_init();
> --
> 1.7.5.4
>

Re: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Shawn Guo]

On Tue, Jun 18, 2013 at 05:00:09AM +0000, Zhu Richard-R65037 wrote:
> > +enum {
> > +     HOST_CAP = 0x00,
> > +     HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> > +     HOST_CTL = 0x04, /* global host control */
> > +     HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> > +     HOST_PORTS_IMPL = 0x0c, /* bitmap of implemented ports */
> > +     HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> > +     HOST_VERSIONR = 0xf8, /* host version register*/
> > +
> > +     PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> > +     PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> > +     PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */
> > +};
> > +
> 
> This is all about IP stuff.  Some of them are just replication of
> definitions found in drivers/ata/ahci.h. I do not understand why we
> need them in platform.  It's a sign to me that we are doing something
> in platform code that should really be done in the device driver, no?
> 
> [Richard] Regarding to the AHCI SPEC, some bits of the ahci sata registers are
>  specified to be RO, but they are RW in the design of fsl ahci ata in actually.
>  So the configurations of those bits are mandatory required in the
>  platform sata initialization. 
> One HBA reset is better finished before start the configuration.
> That's why those registers/bits are re-defined in the platform level.
> 
> For, e.x:
> bit1 of the HOST_POSRTS_IMPL, HOST_CAP_SSS(bit27) of the HOST_CAP, are RO specified
> in the AHCI SPEC. But they should be configured in the sata platform initialization.
> 
> BTW, HOST_TIMER1MS is defined by FSL, can't be used in the common codes.
> regarding to the AHCI SPEC.
> " Registers at offset A0h to FFh are vendor specific"

It sounds like to me that the generic ahci_platform driver does not
perfectly fit imx6q sata device.  You're pushing all those bits that
are not covered by the generic ahci_platform driver down to platform
code.  This is not the right thing to do, and platform is not the right
place to handle device/IP stuff.  I see two options you can go as below.

1. Create an imx6q sata specific compatible string and add code into
   generic ahci_platform driver to implement programming model for imx6q
   type of device.

2. The existing ahci_platform driver is not so useful for imx6q sata
   considering you have so many imx6q custom bits to add.  It might be
   more appropriate to create a custom ahci_platform driver for imx6q
   sata device with forking the generic one, just like what
   sata_highbank does.

I would go for option 2.

Shawn

[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Shawn Guo]

On Tue, Jun 18, 2013 at 05:00:09AM +0000, Zhu Richard-R65037 wrote:
> > +enum {
> > +     HOST_CAP = 0x00,
> > +     HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> > +     HOST_CTL = 0x04, /* global host control */
> > +     HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> > +     HOST_PORTS_IMPL = 0x0c, /* bitmap of implemented ports */
> > +     HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> > +     HOST_VERSIONR = 0xf8, /* host version register*/
> > +
> > +     PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> > +     PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> > +     PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */
> > +};
> > +
> 
> This is all about IP stuff.  Some of them are just replication of
> definitions found in drivers/ata/ahci.h. I do not understand why we
> need them in platform.  It's a sign to me that we are doing something
> in platform code that should really be done in the device driver, no?
> 
> [Richard] Regarding to the AHCI SPEC, some bits of the ahci sata registers are
>  specified to be RO, but they are RW in the design of fsl ahci ata in actually.
>  So the configurations of those bits are mandatory required in the
>  platform sata initialization. 
> One HBA reset is better finished before start the configuration.
> That's why those registers/bits are re-defined in the platform level.
> 
> For, e.x:
> bit1 of the HOST_POSRTS_IMPL, HOST_CAP_SSS(bit27) of the HOST_CAP, are RO specified
> in the AHCI SPEC. But they should be configured in the sata platform initialization.
> 
> BTW, HOST_TIMER1MS is defined by FSL, can't be used in the common codes.
> regarding to the AHCI SPEC.
> " Registers at offset A0h to FFh are vendor specific"

It sounds like to me that the generic ahci_platform driver does not
perfectly fit imx6q sata device.  You're pushing all those bits that
are not covered by the generic ahci_platform driver down to platform
code.  This is not the right thing to do, and platform is not the right
place to handle device/IP stuff.  I see two options you can go as below.

1. Create an imx6q sata specific compatible string and add code into
   generic ahci_platform driver to implement programming model for imx6q
   type of device.

2. The existing ahci_platform driver is not so useful for imx6q sata
   considering you have so many imx6q custom bits to add.  It might be
   more appropriate to create a custom ahci_platform driver for imx6q
   sata device with forking the generic one, just like what
   sata_highbank does.

I would go for option 2.

Shawn

Re: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Sascha Hauer]

On Tue, Jun 18, 2013 at 01:59:27PM +0800, Shawn Guo wrote:
> It sounds like to me that the generic ahci_platform driver does not
> perfectly fit imx6q sata device.  You're pushing all those bits that
> are not covered by the generic ahci_platform driver down to platform
> code.  This is not the right thing to do, and platform is not the right
> place to handle device/IP stuff.  I see two options you can go as below.
> 
> 1. Create an imx6q sata specific compatible string and add code into
>    generic ahci_platform driver to implement programming model for imx6q
>    type of device.
> 
> 2. The existing ahci_platform driver is not so useful for imx6q sata
>    considering you have so many imx6q custom bits to add.  It might be
>    more appropriate to create a custom ahci_platform driver for imx6q
>    sata device with forking the generic one, just like what
>    sata_highbank does.
> 
> I would go for option 2.

That's exactly what we have done some time ago. Unfortunately I never
came along to mainline this patch. See it attached, there may be bits to
copy or it may even be a better base for further work.


8<-------------------------------------------------

>From 51222e32870a37f50713284942b6df34ff40a43c Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Wed, 21 Nov 2012 16:29:00 +0100
Subject: [PATCH] ata: Add Freescale i.MX specific SATA driver

This adds an i.MX specific ahci glue driver. The i.MX SoCs need
some special handlings:

- Additional clocks
- An additional register (TIMER_1MS) which has to be configured
- Registers external to the AHCI core which have to be configured

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/ata/Kconfig                         |   8 +
 drivers/ata/Makefile                        |   1 +
 drivers/ata/ahci_platform.c                 |  10 -
 drivers/ata/sata_imx.c                      | 376 ++++++++++++++++++++++++++++
 include/linux/mfd/syscon/imx6q-iomuxc-gpr.h |   1 +
 5 files changed, 386 insertions(+), 10 deletions(-)
 create mode 100644 drivers/ata/sata_imx.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index a5a3ebc..7cef382 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -106,6 +106,14 @@ config SATA_FSL
 
 	  If unsure, say N.
 
+config SATA_IMX
+	tristate "Freescale i.MX SATA support"
+	help
+	  This option enables support for Freescale i.MX SATA controller.
+	  It can be found on i.MX53 and i.MX6.
+
+	  If unsure, say N.
+
 config SATA_INIC162X
 	tristate "Initio 162x SATA support"
 	depends on PCI
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index c04d0fd..c37ef58 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
 obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
 obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o
 obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
+obj-$(CONFIG_SATA_IMX)		+= sata_imx.o libahci.o
 obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
 obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
 obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 7a8a284..9d57774 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -29,7 +29,6 @@ static void ahci_host_stop(struct ata_host *host);
 
 enum ahci_type {
 	AHCI,		/* standard platform ahci */
-	IMX53_AHCI,	/* ahci on i.mx53 */
 	STRICT_AHCI,	/* delayed DMA engine start */
 };
 
@@ -38,9 +37,6 @@ static struct platform_device_id ahci_devtype[] = {
 		.name = "ahci",
 		.driver_data = AHCI,
 	}, {
-		.name = "imx53-ahci",
-		.driver_data = IMX53_AHCI,
-	}, {
 		.name = "strict-ahci",
 		.driver_data = STRICT_AHCI,
 	}, {
@@ -67,12 +63,6 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_platform_ops,
 	},
-	[IMX53_AHCI] = {
-		.flags		= AHCI_FLAG_COMMON,
-		.pio_mask	= ATA_PIO4,
-		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_platform_retry_srst_ops,
-	},
 	[STRICT_AHCI] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_DELAY_ENGINE),
 		.flags		= AHCI_FLAG_COMMON,
diff --git a/drivers/ata/sata_imx.c b/drivers/ata/sata_imx.c
new file mode 100644
index 0000000..bd8f348
--- /dev/null
+++ b/drivers/ata/sata_imx.c
@@ -0,0 +1,376 @@
+/*
+ * AHCI SATA platform driver
+ *
+ * Copyright 2012 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
+ *
+ * based on ahci platform driver
+ *
+ * Copyright 2004-2005  Red Hat, Inc.
+ *   Jeff Garzik <jgarzik@pobox.com>
+ * Copyright 2010  MontaVista Software, LLC.
+ *   Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * 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.
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/gfp.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/libata.h>
+#include <linux/ahci_platform.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
+
+#include "ahci.h"
+
+struct imx_ahci_priv {
+	struct ahci_host_priv ahci;
+	struct device *dev;
+	struct clk *clk_ahb;
+	struct clk *clk_phy;
+	struct regmap *gprmap;
+};
+
+struct imx_ahci_driver_data {
+	int (*init)(struct imx_ahci_priv *);
+};
+
+static const struct ata_port_info ahci_port_info = {
+	.flags		= AHCI_FLAG_COMMON,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_pmp_retry_srst_ops,
+};
+
+static struct scsi_host_template ahci_platform_sht = {
+	AHCI_SHT("ahci_platform"),
+};
+
+#define HOST_TIMER1MS 0xe0 /* Timer 1-ms */
+
+/*
+ * The i.MX AHCI core has a nonstandard register which has
+ * to be initialized with the number of ahb clk cycles during
+ * 1ms.
+ */
+static void imx_sata_init_1ms(struct imx_ahci_priv *imxpriv)
+{
+	struct ahci_host_priv *hpriv = &imxpriv->ahci;
+	unsigned long rate;
+	u32 val;
+
+	val = readl(hpriv->mmio + HOST_PORTS_IMPL);
+	if (!val & 0x1) {
+		val |= 0x1;
+		writel(val, hpriv->mmio + HOST_PORTS_IMPL);
+	}
+
+	rate = clk_get_rate(imxpriv->clk_ahb);
+	writel(rate / 1000, hpriv->mmio + HOST_TIMER1MS);
+}
+
+static void imx_ahci_clk_enable(struct imx_ahci_priv *imxpriv)
+{
+	clk_prepare_enable(imxpriv->clk_phy);
+	clk_prepare_enable(imxpriv->clk_ahb);
+	clk_prepare_enable(imxpriv->ahci.clk);
+}
+
+static void imx_ahci_clk_disable(struct imx_ahci_priv *imxpriv)
+{
+	clk_prepare_enable(imxpriv->clk_phy);
+	clk_prepare_enable(imxpriv->clk_ahb);
+	clk_prepare_enable(imxpriv->ahci.clk);
+}
+
+static int imx6q_sata_init(struct imx_ahci_priv *imxpriv)
+{
+	struct regmap *map;
+	int ret;
+	u32 val;
+
+	map = syscon_regmap_lookup_by_phandle(imxpriv->dev->of_node, "gprreg");
+	if (IS_ERR(map)) {
+		dev_err(imxpriv->dev, "requesting iomuxc gpr map failed with: %ld\n",
+				PTR_ERR(map));
+		return PTR_ERR(map);
+	}
+
+	val = 0x11 << IMX6Q_GPR13_SATA_PHY_2_OFF | IMX6Q_GPR13_SATA_PHY_4_9_16 |
+		IMX6Q_GPR13_SATA_SPEED_MASK | 0x3 << IMX6Q_GPR13_SATA_PHY_6_OFF |
+		IMX6Q_GPR13_SATA_PHY_7_SATA2I |	IMX6Q_GPR13_SATA_PHY_8_3_0_DB;
+
+	ret = regmap_update_bits(map, IOMUXC_GPR13, 0x07ffffff, val);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(map, IOMUXC_GPR13, 0x07ffffff, val | 2);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct imx_ahci_driver_data imx6q_driver_data = {
+	.init = imx6q_sata_init,
+};
+
+static struct imx_ahci_driver_data imx53_driver_data = {
+	/*
+	 * FIXME: The i.MX53 also needs SoC specific init code. This
+	 * is not yet implemented. The necessary fix currently is done
+	 * in U-Boot.
+	 */
+};
+
+static const struct of_device_id ahci_of_match[] = {
+	{ .compatible = "fsl,imx53-ahci", .data = &imx53_driver_data },
+	{ .compatible = "fsl,imx6q-ahci", .data = &imx6q_driver_data },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ahci_of_match);
+
+static int ahci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
+	struct ata_port_info pi = ahci_port_info;
+	const struct ata_port_info *ppi[] = { &ahci_port_info, NULL };
+	struct ahci_host_priv *hpriv;
+	struct imx_ahci_priv *imxpriv;
+	struct ata_host *host;
+	struct resource *mem;
+	const struct imx_ahci_driver_data *driver_data = NULL;
+	int irq, i, rc;
+
+	of_id = of_match_device(ahci_of_match, &pdev->dev);
+	if (of_id)
+		driver_data = of_id->data;
+
+	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;
+	}
+
+	imxpriv = devm_kzalloc(dev, sizeof(*imxpriv), GFP_KERNEL);
+	if (!imxpriv) {
+		dev_err(dev, "can't alloc ahci_host_priv\n");
+		return -ENOMEM;
+	}
+
+	imxpriv->dev = dev;
+
+	hpriv = &imxpriv->ahci;
+
+	hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem));
+	if (!hpriv->mmio) {
+		dev_err(dev, "can't map %pR\n", mem);
+		return -ENOMEM;
+	}
+
+	hpriv->clk = devm_clk_get(dev, "ipg");
+	if (IS_ERR(hpriv->clk)) {
+		rc = PTR_ERR(hpriv->clk);
+		dev_err(dev, "failed to get ipg clock: %d\n", rc);
+		return rc;
+	}
+
+	imxpriv->clk_ahb = devm_clk_get(dev, "ahb");
+	if (IS_ERR(imxpriv->clk_ahb)) {
+		rc = PTR_ERR(imxpriv->clk_ahb);
+		dev_err(dev, "failed to get ahb clock: %d\n", rc);
+		return rc;
+	}
+
+	imxpriv->clk_phy = devm_clk_get(dev, "per");
+	if (IS_ERR(imxpriv->clk_phy)) {
+		rc = PTR_ERR(imxpriv->clk_phy);
+		dev_err(dev, "failed to get ahb clock: %d\n", rc);
+		return rc;
+	}
+
+	if (driver_data && driver_data->init) {
+		rc = driver_data->init(imxpriv);
+		if (rc) {
+			dev_err(dev, "failed to init: %d\n", rc);
+			return rc;
+		}
+	}
+
+	imx_ahci_clk_enable(imxpriv);
+
+	imx_sata_init_1ms(imxpriv);
+
+	ahci_save_initial_config(dev, hpriv, 0, 0);
+
+	/* prepare host */
+	if (hpriv->cap & HOST_CAP_NCQ)
+		pi.flags |= ATA_FLAG_NCQ;
+
+	if (hpriv->cap & HOST_CAP_PMP)
+		pi.flags |= ATA_FLAG_PMP;
+
+	ahci_set_em_messages(hpriv, &pi);
+
+	host = ata_host_alloc_pinfo(dev, ppi, 1);
+	if (!host) {
+		rc = -ENOMEM;
+		goto err_clk;
+	}
+
+	host->private_data = hpriv;
+
+	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
+		host->flags |= ATA_HOST_PARALLEL_SCAN;
+
+	if (pi.flags & ATA_FLAG_EM)
+		ahci_reset_em(host);
+
+	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, "port 0x%x", 0x100 + ap->port_no * 0x80);
+
+		/* set enclosure management message type */
+		if (ap->flags & ATA_FLAG_EM)
+			ap->em_message_type = hpriv->em_msg_type;
+
+		/* disabled/not-implemented port */
+		if (!(hpriv->port_map & (1 << i)))
+			ap->ops = &ata_dummy_port_ops;
+	}
+
+	rc = ahci_reset_controller(host);
+	if (rc)
+		goto err_clk;
+
+	ahci_init_controller(host);
+	ahci_print_info(host, "platform");
+
+	rc = ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
+			       &ahci_platform_sht);
+	if (rc)
+		goto err_clk;
+
+	return 0;
+
+err_clk:
+	clk_disable_unprepare(hpriv->clk);
+
+	return rc;
+}
+
+static int ahci_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = container_of(hpriv, struct imx_ahci_priv, ahci);
+
+	ata_host_detach(host);
+
+	imx_ahci_clk_disable(imxpriv);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int ahci_suspend(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = container_of(hpriv, struct imx_ahci_priv, ahci);
+	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");
+		return -EIO;
+	}
+
+	/*
+	 * AHCI spec rev1.1 section 8.3.3:
+	 * Software must disable interrupts prior to requesting a
+	 * transition of the HBA to D3 state.
+	 */
+	ctl = readl(mmio + HOST_CTL);
+	ctl &= ~HOST_IRQ_EN;
+	writel(ctl, mmio + HOST_CTL);
+	readl(mmio + HOST_CTL); /* flush */
+
+	rc = ata_host_suspend(host, PMSG_SUSPEND);
+	if (rc)
+		return rc;
+
+	imx_ahci_clk_disable(imxpriv);
+
+	return 0;
+}
+
+static int ahci_resume(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = container_of(hpriv, struct imx_ahci_priv, ahci);
+	int rc;
+
+	imx_ahci_clk_enable(imxpriv);
+
+	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
+		rc = ahci_reset_controller(host);
+		if (rc)
+			goto disable_unprepare_clk;
+
+		ahci_init_controller(host);
+	}
+
+	ata_host_resume(host);
+
+	return 0;
+
+disable_unprepare_clk:
+		imx_ahci_clk_disable(imxpriv);
+
+	return rc;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
+
+static struct platform_driver imx_ahci_driver = {
+	.probe = ahci_probe,
+	.remove = ahci_remove,
+	.driver = {
+		.name = "imx-ahci",
+		.owner = THIS_MODULE,
+		.of_match_table = ahci_of_match,
+		.pm = &ahci_pm_ops,
+	},
+};
+
+module_platform_driver(imx_ahci_driver);
+
+MODULE_DESCRIPTION("i.MX AHCI SATA platform driver");
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:imx-ahci");
diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
index dab34a1..db43d59 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -296,6 +296,7 @@
 #define IMX6Q_GPR13_SATA_PHY_7_SATA2M		(0x12 << 19)
 #define IMX6Q_GPR13_SATA_PHY_7_SATA2X		(0x1a << 19)
 #define IMX6Q_GPR13_SATA_PHY_6_MASK		(0x7 << 16)
+#define IMX6Q_GPR13_SATA_PHY_6_OFF		16
 #define IMX6Q_GPR13_SATA_SPEED_MASK		BIT(15)
 #define IMX6Q_GPR13_SATA_SPEED_1P5G		0x0
 #define IMX6Q_GPR13_SATA_SPEED_3P0G		BIT(15)
-- 
1.8.3.1

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Sascha Hauer]

On Tue, Jun 18, 2013 at 01:59:27PM +0800, Shawn Guo wrote:
> It sounds like to me that the generic ahci_platform driver does not
> perfectly fit imx6q sata device.  You're pushing all those bits that
> are not covered by the generic ahci_platform driver down to platform
> code.  This is not the right thing to do, and platform is not the right
> place to handle device/IP stuff.  I see two options you can go as below.
> 
> 1. Create an imx6q sata specific compatible string and add code into
>    generic ahci_platform driver to implement programming model for imx6q
>    type of device.
> 
> 2. The existing ahci_platform driver is not so useful for imx6q sata
>    considering you have so many imx6q custom bits to add.  It might be
>    more appropriate to create a custom ahci_platform driver for imx6q
>    sata device with forking the generic one, just like what
>    sata_highbank does.
> 
> I would go for option 2.

That's exactly what we have done some time ago. Unfortunately I never
came along to mainline this patch. See it attached, there may be bits to
copy or it may even be a better base for further work.


8<-------------------------------------------------

>From 51222e32870a37f50713284942b6df34ff40a43c Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Wed, 21 Nov 2012 16:29:00 +0100
Subject: [PATCH] ata: Add Freescale i.MX specific SATA driver

This adds an i.MX specific ahci glue driver. The i.MX SoCs need
some special handlings:

- Additional clocks
- An additional register (TIMER_1MS) which has to be configured
- Registers external to the AHCI core which have to be configured

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/ata/Kconfig                         |   8 +
 drivers/ata/Makefile                        |   1 +
 drivers/ata/ahci_platform.c                 |  10 -
 drivers/ata/sata_imx.c                      | 376 ++++++++++++++++++++++++++++
 include/linux/mfd/syscon/imx6q-iomuxc-gpr.h |   1 +
 5 files changed, 386 insertions(+), 10 deletions(-)
 create mode 100644 drivers/ata/sata_imx.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index a5a3ebc..7cef382 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -106,6 +106,14 @@ config SATA_FSL
 
 	  If unsure, say N.
 
+config SATA_IMX
+	tristate "Freescale i.MX SATA support"
+	help
+	  This option enables support for Freescale i.MX SATA controller.
+	  It can be found on i.MX53 and i.MX6.
+
+	  If unsure, say N.
+
 config SATA_INIC162X
 	tristate "Initio 162x SATA support"
 	depends on PCI
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index c04d0fd..c37ef58 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
 obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
 obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o
 obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
+obj-$(CONFIG_SATA_IMX)		+= sata_imx.o libahci.o
 obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
 obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
 obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 7a8a284..9d57774 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -29,7 +29,6 @@ static void ahci_host_stop(struct ata_host *host);
 
 enum ahci_type {
 	AHCI,		/* standard platform ahci */
-	IMX53_AHCI,	/* ahci on i.mx53 */
 	STRICT_AHCI,	/* delayed DMA engine start */
 };
 
@@ -38,9 +37,6 @@ static struct platform_device_id ahci_devtype[] = {
 		.name = "ahci",
 		.driver_data = AHCI,
 	}, {
-		.name = "imx53-ahci",
-		.driver_data = IMX53_AHCI,
-	}, {
 		.name = "strict-ahci",
 		.driver_data = STRICT_AHCI,
 	}, {
@@ -67,12 +63,6 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_platform_ops,
 	},
-	[IMX53_AHCI] = {
-		.flags		= AHCI_FLAG_COMMON,
-		.pio_mask	= ATA_PIO4,
-		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_platform_retry_srst_ops,
-	},
 	[STRICT_AHCI] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_DELAY_ENGINE),
 		.flags		= AHCI_FLAG_COMMON,
diff --git a/drivers/ata/sata_imx.c b/drivers/ata/sata_imx.c
new file mode 100644
index 0000000..bd8f348
--- /dev/null
+++ b/drivers/ata/sata_imx.c
@@ -0,0 +1,376 @@
+/*
+ * AHCI SATA platform driver
+ *
+ * Copyright 2012 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
+ *
+ * based on ahci platform driver
+ *
+ * Copyright 2004-2005  Red Hat, Inc.
+ *   Jeff Garzik <jgarzik@pobox.com>
+ * Copyright 2010  MontaVista Software, LLC.
+ *   Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * 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.
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/gfp.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/libata.h>
+#include <linux/ahci_platform.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
+
+#include "ahci.h"
+
+struct imx_ahci_priv {
+	struct ahci_host_priv ahci;
+	struct device *dev;
+	struct clk *clk_ahb;
+	struct clk *clk_phy;
+	struct regmap *gprmap;
+};
+
+struct imx_ahci_driver_data {
+	int (*init)(struct imx_ahci_priv *);
+};
+
+static const struct ata_port_info ahci_port_info = {
+	.flags		= AHCI_FLAG_COMMON,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_pmp_retry_srst_ops,
+};
+
+static struct scsi_host_template ahci_platform_sht = {
+	AHCI_SHT("ahci_platform"),
+};
+
+#define HOST_TIMER1MS 0xe0 /* Timer 1-ms */
+
+/*
+ * The i.MX AHCI core has a nonstandard register which has
+ * to be initialized with the number of ahb clk cycles during
+ * 1ms.
+ */
+static void imx_sata_init_1ms(struct imx_ahci_priv *imxpriv)
+{
+	struct ahci_host_priv *hpriv = &imxpriv->ahci;
+	unsigned long rate;
+	u32 val;
+
+	val = readl(hpriv->mmio + HOST_PORTS_IMPL);
+	if (!val & 0x1) {
+		val |= 0x1;
+		writel(val, hpriv->mmio + HOST_PORTS_IMPL);
+	}
+
+	rate = clk_get_rate(imxpriv->clk_ahb);
+	writel(rate / 1000, hpriv->mmio + HOST_TIMER1MS);
+}
+
+static void imx_ahci_clk_enable(struct imx_ahci_priv *imxpriv)
+{
+	clk_prepare_enable(imxpriv->clk_phy);
+	clk_prepare_enable(imxpriv->clk_ahb);
+	clk_prepare_enable(imxpriv->ahci.clk);
+}
+
+static void imx_ahci_clk_disable(struct imx_ahci_priv *imxpriv)
+{
+	clk_prepare_enable(imxpriv->clk_phy);
+	clk_prepare_enable(imxpriv->clk_ahb);
+	clk_prepare_enable(imxpriv->ahci.clk);
+}
+
+static int imx6q_sata_init(struct imx_ahci_priv *imxpriv)
+{
+	struct regmap *map;
+	int ret;
+	u32 val;
+
+	map = syscon_regmap_lookup_by_phandle(imxpriv->dev->of_node, "gprreg");
+	if (IS_ERR(map)) {
+		dev_err(imxpriv->dev, "requesting iomuxc gpr map failed with: %ld\n",
+				PTR_ERR(map));
+		return PTR_ERR(map);
+	}
+
+	val = 0x11 << IMX6Q_GPR13_SATA_PHY_2_OFF | IMX6Q_GPR13_SATA_PHY_4_9_16 |
+		IMX6Q_GPR13_SATA_SPEED_MASK | 0x3 << IMX6Q_GPR13_SATA_PHY_6_OFF |
+		IMX6Q_GPR13_SATA_PHY_7_SATA2I |	IMX6Q_GPR13_SATA_PHY_8_3_0_DB;
+
+	ret = regmap_update_bits(map, IOMUXC_GPR13, 0x07ffffff, val);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(map, IOMUXC_GPR13, 0x07ffffff, val | 2);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct imx_ahci_driver_data imx6q_driver_data = {
+	.init = imx6q_sata_init,
+};
+
+static struct imx_ahci_driver_data imx53_driver_data = {
+	/*
+	 * FIXME: The i.MX53 also needs SoC specific init code. This
+	 * is not yet implemented. The necessary fix currently is done
+	 * in U-Boot.
+	 */
+};
+
+static const struct of_device_id ahci_of_match[] = {
+	{ .compatible = "fsl,imx53-ahci", .data = &imx53_driver_data },
+	{ .compatible = "fsl,imx6q-ahci", .data = &imx6q_driver_data },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ahci_of_match);
+
+static int ahci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
+	struct ata_port_info pi = ahci_port_info;
+	const struct ata_port_info *ppi[] = { &ahci_port_info, NULL };
+	struct ahci_host_priv *hpriv;
+	struct imx_ahci_priv *imxpriv;
+	struct ata_host *host;
+	struct resource *mem;
+	const struct imx_ahci_driver_data *driver_data = NULL;
+	int irq, i, rc;
+
+	of_id = of_match_device(ahci_of_match, &pdev->dev);
+	if (of_id)
+		driver_data = of_id->data;
+
+	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;
+	}
+
+	imxpriv = devm_kzalloc(dev, sizeof(*imxpriv), GFP_KERNEL);
+	if (!imxpriv) {
+		dev_err(dev, "can't alloc ahci_host_priv\n");
+		return -ENOMEM;
+	}
+
+	imxpriv->dev = dev;
+
+	hpriv = &imxpriv->ahci;
+
+	hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem));
+	if (!hpriv->mmio) {
+		dev_err(dev, "can't map %pR\n", mem);
+		return -ENOMEM;
+	}
+
+	hpriv->clk = devm_clk_get(dev, "ipg");
+	if (IS_ERR(hpriv->clk)) {
+		rc = PTR_ERR(hpriv->clk);
+		dev_err(dev, "failed to get ipg clock: %d\n", rc);
+		return rc;
+	}
+
+	imxpriv->clk_ahb = devm_clk_get(dev, "ahb");
+	if (IS_ERR(imxpriv->clk_ahb)) {
+		rc = PTR_ERR(imxpriv->clk_ahb);
+		dev_err(dev, "failed to get ahb clock: %d\n", rc);
+		return rc;
+	}
+
+	imxpriv->clk_phy = devm_clk_get(dev, "per");
+	if (IS_ERR(imxpriv->clk_phy)) {
+		rc = PTR_ERR(imxpriv->clk_phy);
+		dev_err(dev, "failed to get ahb clock: %d\n", rc);
+		return rc;
+	}
+
+	if (driver_data && driver_data->init) {
+		rc = driver_data->init(imxpriv);
+		if (rc) {
+			dev_err(dev, "failed to init: %d\n", rc);
+			return rc;
+		}
+	}
+
+	imx_ahci_clk_enable(imxpriv);
+
+	imx_sata_init_1ms(imxpriv);
+
+	ahci_save_initial_config(dev, hpriv, 0, 0);
+
+	/* prepare host */
+	if (hpriv->cap & HOST_CAP_NCQ)
+		pi.flags |= ATA_FLAG_NCQ;
+
+	if (hpriv->cap & HOST_CAP_PMP)
+		pi.flags |= ATA_FLAG_PMP;
+
+	ahci_set_em_messages(hpriv, &pi);
+
+	host = ata_host_alloc_pinfo(dev, ppi, 1);
+	if (!host) {
+		rc = -ENOMEM;
+		goto err_clk;
+	}
+
+	host->private_data = hpriv;
+
+	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
+		host->flags |= ATA_HOST_PARALLEL_SCAN;
+
+	if (pi.flags & ATA_FLAG_EM)
+		ahci_reset_em(host);
+
+	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, "port 0x%x", 0x100 + ap->port_no * 0x80);
+
+		/* set enclosure management message type */
+		if (ap->flags & ATA_FLAG_EM)
+			ap->em_message_type = hpriv->em_msg_type;
+
+		/* disabled/not-implemented port */
+		if (!(hpriv->port_map & (1 << i)))
+			ap->ops = &ata_dummy_port_ops;
+	}
+
+	rc = ahci_reset_controller(host);
+	if (rc)
+		goto err_clk;
+
+	ahci_init_controller(host);
+	ahci_print_info(host, "platform");
+
+	rc = ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
+			       &ahci_platform_sht);
+	if (rc)
+		goto err_clk;
+
+	return 0;
+
+err_clk:
+	clk_disable_unprepare(hpriv->clk);
+
+	return rc;
+}
+
+static int ahci_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = container_of(hpriv, struct imx_ahci_priv, ahci);
+
+	ata_host_detach(host);
+
+	imx_ahci_clk_disable(imxpriv);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int ahci_suspend(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = container_of(hpriv, struct imx_ahci_priv, ahci);
+	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");
+		return -EIO;
+	}
+
+	/*
+	 * AHCI spec rev1.1 section 8.3.3:
+	 * Software must disable interrupts prior to requesting a
+	 * transition of the HBA to D3 state.
+	 */
+	ctl = readl(mmio + HOST_CTL);
+	ctl &= ~HOST_IRQ_EN;
+	writel(ctl, mmio + HOST_CTL);
+	readl(mmio + HOST_CTL); /* flush */
+
+	rc = ata_host_suspend(host, PMSG_SUSPEND);
+	if (rc)
+		return rc;
+
+	imx_ahci_clk_disable(imxpriv);
+
+	return 0;
+}
+
+static int ahci_resume(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = container_of(hpriv, struct imx_ahci_priv, ahci);
+	int rc;
+
+	imx_ahci_clk_enable(imxpriv);
+
+	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
+		rc = ahci_reset_controller(host);
+		if (rc)
+			goto disable_unprepare_clk;
+
+		ahci_init_controller(host);
+	}
+
+	ata_host_resume(host);
+
+	return 0;
+
+disable_unprepare_clk:
+		imx_ahci_clk_disable(imxpriv);
+
+	return rc;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
+
+static struct platform_driver imx_ahci_driver = {
+	.probe = ahci_probe,
+	.remove = ahci_remove,
+	.driver = {
+		.name = "imx-ahci",
+		.owner = THIS_MODULE,
+		.of_match_table = ahci_of_match,
+		.pm = &ahci_pm_ops,
+	},
+};
+
+module_platform_driver(imx_ahci_driver);
+
+MODULE_DESCRIPTION("i.MX AHCI SATA platform driver");
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:imx-ahci");
diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
index dab34a1..db43d59 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -296,6 +296,7 @@
 #define IMX6Q_GPR13_SATA_PHY_7_SATA2M		(0x12 << 19)
 #define IMX6Q_GPR13_SATA_PHY_7_SATA2X		(0x1a << 19)
 #define IMX6Q_GPR13_SATA_PHY_6_MASK		(0x7 << 16)
+#define IMX6Q_GPR13_SATA_PHY_6_OFF		16
 #define IMX6Q_GPR13_SATA_SPEED_MASK		BIT(15)
 #define IMX6Q_GPR13_SATA_SPEED_1P5G		0x0
 #define IMX6Q_GPR13_SATA_SPEED_3P0G		BIT(15)
-- 
1.8.3.1

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Zhu Richard-R65037]

Hi Shawn:
It's sounds right, platform is not right place to handle device/IP stuff.
I would kick off to your option2. Thanks.

Best Regards
Richard Zhu

-----Original Message-----
From: Shawn Guo [mailto:shawn.guo@linaro.org] 
Sent: Tuesday, June 18, 2013 1:59 PM
To: Zhu Richard-R65037
Cc: Richard Zhu; linux-arm-kernel@lists.infradead.org; jgarzik@pobox.com; linux-ide@vger.kernel.org
Subject: Re: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

On Tue, Jun 18, 2013 at 05:00:09AM +0000, Zhu Richard-R65037 wrote:
> > +enum {
> > +     HOST_CAP = 0x00,
> > +     HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> > +     HOST_CTL = 0x04, /* global host control */
> > +     HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> > +     HOST_PORTS_IMPL = 0x0c, /* bitmap of implemented ports */
> > +     HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> > +     HOST_VERSIONR = 0xf8, /* host version register*/
> > +
> > +     PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> > +     PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> > +     PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */ };
> > +
> 
> This is all about IP stuff.  Some of them are just replication of 
> definitions found in drivers/ata/ahci.h. I do not understand why we 
> need them in platform.  It's a sign to me that we are doing something 
> in platform code that should really be done in the device driver, no?
> 
> [Richard] Regarding to the AHCI SPEC, some bits of the ahci sata 
> registers are  specified to be RO, but they are RW in the design of fsl ahci ata in actually.
>  So the configurations of those bits are mandatory required in the  
> platform sata initialization.
> One HBA reset is better finished before start the configuration.
> That's why those registers/bits are re-defined in the platform level.
> 
> For, e.x:
> bit1 of the HOST_POSRTS_IMPL, HOST_CAP_SSS(bit27) of the HOST_CAP, are 
> RO specified in the AHCI SPEC. But they should be configured in the sata platform initialization.
> 
> BTW, HOST_TIMER1MS is defined by FSL, can't be used in the common codes.
> regarding to the AHCI SPEC.
> " Registers at offset A0h to FFh are vendor specific"

It sounds like to me that the generic ahci_platform driver does not perfectly fit imx6q sata device.  You're pushing all those bits that are not covered by the generic ahci_platform driver down to platform code.  This is not the right thing to do, and platform is not the right place to handle device/IP stuff.  I see two options you can go as below.

1. Create an imx6q sata specific compatible string and add code into
   generic ahci_platform driver to implement programming model for imx6q
   type of device.

2. The existing ahci_platform driver is not so useful for imx6q sata
   considering you have so many imx6q custom bits to add.  It might be
   more appropriate to create a custom ahci_platform driver for imx6q
   sata device with forking the generic one, just like what
   sata_highbank does.

I would go for option 2.

Shawn

[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Zhu Richard-R65037]

Hi Shawn:
It's sounds right, platform is not right place to handle device/IP stuff.
I would kick off to your option2. Thanks.

Best Regards
Richard Zhu

-----Original Message-----
From: Shawn Guo [mailto:shawn.guo at linaro.org] 
Sent: Tuesday, June 18, 2013 1:59 PM
To: Zhu Richard-R65037
Cc: Richard Zhu; linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; linux-ide at vger.kernel.org
Subject: Re: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

On Tue, Jun 18, 2013 at 05:00:09AM +0000, Zhu Richard-R65037 wrote:
> > +enum {
> > +     HOST_CAP = 0x00,
> > +     HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> > +     HOST_CTL = 0x04, /* global host control */
> > +     HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> > +     HOST_PORTS_IMPL = 0x0c, /* bitmap of implemented ports */
> > +     HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> > +     HOST_VERSIONR = 0xf8, /* host version register*/
> > +
> > +     PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> > +     PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> > +     PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */ };
> > +
> 
> This is all about IP stuff.  Some of them are just replication of 
> definitions found in drivers/ata/ahci.h. I do not understand why we 
> need them in platform.  It's a sign to me that we are doing something 
> in platform code that should really be done in the device driver, no?
> 
> [Richard] Regarding to the AHCI SPEC, some bits of the ahci sata 
> registers are  specified to be RO, but they are RW in the design of fsl ahci ata in actually.
>  So the configurations of those bits are mandatory required in the  
> platform sata initialization.
> One HBA reset is better finished before start the configuration.
> That's why those registers/bits are re-defined in the platform level.
> 
> For, e.x:
> bit1 of the HOST_POSRTS_IMPL, HOST_CAP_SSS(bit27) of the HOST_CAP, are 
> RO specified in the AHCI SPEC. But they should be configured in the sata platform initialization.
> 
> BTW, HOST_TIMER1MS is defined by FSL, can't be used in the common codes.
> regarding to the AHCI SPEC.
> " Registers at offset A0h to FFh are vendor specific"

It sounds like to me that the generic ahci_platform driver does not perfectly fit imx6q sata device.  You're pushing all those bits that are not covered by the generic ahci_platform driver down to platform code.  This is not the right thing to do, and platform is not the right place to handle device/IP stuff.  I see two options you can go as below.

1. Create an imx6q sata specific compatible string and add code into
   generic ahci_platform driver to implement programming model for imx6q
   type of device.

2. The existing ahci_platform driver is not so useful for imx6q sata
   considering you have so many imx6q custom bits to add.  It might be
   more appropriate to create a custom ahci_platform driver for imx6q
   sata device with forking the generic one, just like what
   sata_highbank does.

I would go for option 2.

Shawn