[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 at 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
>