[v5 3/3] sata: imx: add ahci sata support on imx platforms
Zhu Richard-R65037
r65037 at freescale.com
Fri Jul 12 03:42:13 EDT 2013
Hi Shawn:
Thanks for your comments.
Best Regards
Richard Zhu
-----Original Message-----
From: Shawn Guo [mailto:shawn.guo at linaro.org]
Sent: Friday, July 12, 2013 3:02 PM
To: Richard Zhu
Cc: linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; tj at kernel.org; rob.herring at calxeda.com; s.hauer at pengutronix.de; linux-ide at vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v5 3/3] sata: imx: add ahci sata support on imx platforms
On Thu, Jul 11, 2013 at 05:10:31PM +0800, Richard Zhu wrote:
> From: Richard Zhu <r65037 at freescale.com>
>
> imx6q contains one Synopsys AHCI SATA controller, But it can't shares
> ahci_platform driver with other controllers.
> Because there are some misalignments of the generic AHCI controller.
> The bits definitions of the HBA registers, the Vendor Specific
> registers, the AHCI PHY clock and the AHCI signals adjustment
> window(GPR13 Reg).
> - CAP_SSS(bit20) of the HOST_CAP is writable, default value is '0',
> should be configured to be '1'
> - bit0 (only one AHCI SATA port on imx6q) of the HOST_PORTS_IMPL
> should be set to be '1'.(default 0)
> - One Vendor Specific register HOST_TIMER1MS(offset:0xe0) should be
> configured regarding to the frequency of AHB bus clock.
> - Configurations of the AHCI PHY clock, and the signal parameters of
> the GPR13
>
> Setup its own ahci sata driver, contained the imx6q specific
> initialized codes, re-use the generic ahci_platform drier, and
s/drier/driver
[Richard] One typo error, would be changed.
> keep the generic ahci_platform driver clean as much as possible.
>
> Signed-off-by: Richard Zhu <r65037 at freescale.com>
> ---
> drivers/ata/Kconfig | 9 ++
> drivers/ata/Makefile | 1 +
> drivers/ata/sata_imx.c | 233
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 243 insertions(+), 0 deletions(-) create mode
> 100644 drivers/ata/sata_imx.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index
> a5a3ebc..275dc2c 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -97,6 +97,15 @@ config SATA_AHCI_PLATFORM
>
> If unsure, say N.
>
> +config SATA_IMX
> + tristate "Freescale i.MX AHCI SATA support"
> + depends on SATA_AHCI_PLATFORM
> + help
> + This option enables support for the Freescale i.MX SoC's
> + onboard AHCI SATA.
> +
> + If unsure, say N.
> +
> config SATA_FSL
> tristate "Freescale 3.0Gbps SATA support"
> depends on FSL_SOC
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile index
> c04d0fd..04b1c6c 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o
> obj-$(CONFIG_SATA_SIL24) += sata_sil24.o
> obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o
> obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o
> +obj-$(CONFIG_SATA_IMX) += sata_imx.o
>
> # SFF w/ custom DMA
> obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
> diff --git a/drivers/ata/sata_imx.c b/drivers/ata/sata_imx.c new file
> mode 100644 index 0000000..de6330e
> --- /dev/null
> +++ b/drivers/ata/sata_imx.c
> @@ -0,0 +1,233 @@
> +/*
> + * Freescale IMX AHCI SATA platform driver
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * based on the AHCI SATA platform driver by Jeff Garzik and Anton
> +Vorontsov
> + *
> + * This program is free software; you can redistribute it and/or
> +modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> +WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> +or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> +License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> +along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> +#include "ahci.h"
> +
> +enum {
> + HOST_TIMER1MS = 0xe0, /* Timer 1-ms */ };
> +
> +struct imx_ahci_priv {
> + struct platform_device *imx_ahci_pdev;
> + struct clk *sata_ref_clk;
> + struct regmap *gpr;
> +};
> +
> +/* imx6q ahci module initialization. */ static int
> +imx6q_sata_init(struct device *dev, void __iomem *mmio) {
> + int ret = 0;
> + unsigned int rc;
The variable name is confusing. "rc" generally means return code.
[Richard] Accepted. Would be changed.
> + struct clk *ahb_clk;
> + struct device *p_dev = dev->parent;
> + struct imx_ahci_priv *imxpriv = dev_get_drvdata(p_dev);
I would go dev_get_drvdata(dev->parent), since dev->parent is even more readable than p_dev.
[Richard] Accepted.
> +
> + imxpriv->gpr =
> + syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (IS_ERR(imxpriv->gpr)) {
> + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n");
> + return PTR_ERR(imxpriv->gpr);
> + }
Move it into imx_ahci_probe().
[Richard] The codes "> + syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr"); " is imx6q specific.
I think it's better to place these codes in the imx6q_sata_init(). Otherwise, the probe maybe broken here, when imx53
sata use sata_imx driver in future.
> +
> + imxpriv->sata_ref_clk = devm_clk_get(dev, "sata_ref");
> + if (IS_ERR(imxpriv->sata_ref_clk)) {
> + dev_err(dev, "can't get sata_ref clock.\n");
> + return PTR_ERR(imxpriv->sata_ref_clk);
> + }
Ditto
[Richard] Accepted.
> +
> + ret = clk_prepare_enable(imxpriv->sata_ref_clk);
> + if (ret < 0) {
> + dev_err(dev, "prepare-enable sata_ref clock err:%d\n", ret);
> + return ret;
> + }
> +
> + /*
> + * 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_en.
> + */
> + regmap_update_bits(imxpriv->gpr, 0x34, IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK
> + | IMX6Q_GPR13_SATA_SPD_MODE_MASK
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK
> + | IMX6Q_GPR13_SATA_TX_BOOST_MASK
> + | IMX6Q_GPR13_SATA_TX_LVL_MASK
> + | IMX6Q_GPR13_SATA_TX_EDGE_RATE
> + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB
> + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M
> + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F
> + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G
> + | IMX6Q_GPR13_SATA_MPLL_SS_EN
> + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16
> + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB
> + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V);
> + regmap_update_bits(imxpriv->gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> + usleep_range(100, 200);
> +
> + /*
> + * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
> + * and IP vendor specific register HOST_TIMER1MS.
> + *
> + * Configure CAP_SSS (support stagered spin up).
> + * Implement the port0.
> + * Get the ahb clock rate, and configure the TIMER1MS register.
> + */
> + rc = readl(mmio + HOST_CAP);
> + if (!(rc & HOST_CAP_SSS)) {
> + rc |= HOST_CAP_SSS;
> + writel(rc, mmio + HOST_CAP);
> + }
> + rc = readl(mmio + HOST_PORTS_IMPL);
> + if (!(rc & 0x1)) {
> + rc |= 0x1;
> + writel(rc, mmio + HOST_PORTS_IMPL);
> + }
> +
> + ahb_clk = devm_clk_get(dev, "ahb");
If you need to get ahb clock, please specify it in your DTS node. Right now, you only have the following two clocks in the DTS.
clocks = <&clks 154>, <&clks 187>;
clock-names = "sata", "sata_ref";
You code happens to work because of the following call in clk-imx6q.c.
clk_register_clkdev(clk[ahb], "ahb", NULL);
But you shouldn't reply on that. With all users moved to looking up clocks from device tree, we would remove it from clock driver.
Also, please have a clk pointer in imx_ahci_priv and call devm_clk_get() in imx_ahci_probe().
[Richard] Accepted. Would add "ahb" to sata related dts changes.
> + if (IS_ERR(ahb_clk)) {
> + dev_err(dev, "no ahb clock.\n");
> + clk_disable_unprepare(imxpriv->sata_ref_clk);
> + devm_clk_put(dev, imxpriv->sata_ref_clk);
> + return PTR_ERR(ahb_clk);
> + }
> + rc = clk_get_rate(ahb_clk) / 1000;
> + writel(rc, mmio + HOST_TIMER1MS);
> + devm_clk_put(dev, ahb_clk);
> +
> + return 0;
> +}
> +
> +static void imx6q_sata_exit(struct device *dev) {
> + struct device *p_dev = dev->parent;
> + struct imx_ahci_priv *imxpriv = dev_get_drvdata(p_dev);
dev_get_drvdata(dev->parent);
[Richard] Accepted.
> +
> + regmap_update_bits(imxpriv->gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
> + !IMX6Q_GPR13_SATA_MPLL_CLK_EN);
> + clk_disable_unprepare(imxpriv->sata_ref_clk);
> +}
> +
> +static struct ahci_platform_data imx6q_sata_pdata = {
> + .init = imx6q_sata_init,
> + .exit = imx6q_sata_exit,
> +};
Put a blank line here.
> +static const struct of_device_id imx_ahci_of_match[] = {
> + { .compatible = "fsl,imx6q-ahci", .data = &imx6q_sata_pdata},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, imx_ahci_of_match);
> +
> +static void imx_ahci_platform_release(struct device *dev) {
> + return;
> +}
Is this empty function mandatory?
[Richard] Introduce in the previous patch-set, It can be removed in the current version.
> +
> +static int imx_ahci_probe(struct platform_device *pdev) {
> + struct device *dev = &pdev->dev;
> + struct resource *mem, *irq, res[2];
> + const struct of_device_id *of_id;
> + const struct ahci_platform_data *pdata = NULL;
> + struct imx_ahci_priv *imxpriv;
> + int ret;
> +
I would drop this blank line and move the following two line above "int ret;".
[Richard] Accepted.
> + struct device *imx_dev;
> + struct platform_device *imx_ahci_pdev;
These two variable names are confusing. I would name them ahci_dev and ahci_pdev, as they are all about generic "ahci" rather than imx specific one.
[Richard] Accepted.
> +
> + imx_ahci_pdev = platform_device_alloc("ahci", -1);
> + if (!imx_ahci_pdev)
> + return -ENODEV;
> +
> + imx_dev = &imx_ahci_pdev->dev;
> + imx_dev->release = imx_ahci_platform_release;
> + imx_dev->parent = dev;
> +
> + imxpriv = devm_kzalloc(dev, sizeof(*imxpriv), GFP_KERNEL);
> + if (!imxpriv) {
> + dev_err(dev, "can't alloc ahci_host_priv\n");
> + return -ENOMEM;
> + }
> +
> + imxpriv->imx_ahci_pdev = imx_ahci_pdev;
So imx_ahci_pdev in imx_ahci_priv is also confusing.
[Richard] imx_ahci_pdev would be replaced by ahci_pdev.
> + platform_set_drvdata(pdev, imxpriv);
> +
> + of_id = of_match_device(imx_ahci_of_match, dev);
> + if (of_id)
> + pdata = of_id->data;
> + else
> + return -EINVAL;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!mem || !irq) {
> + dev_err(dev, "no mmio/irq resource\n");
> + return -EINVAL;
> + }
> +
> + res[0] = *mem;
> + res[1] = *irq;
> +
> + imx_dev->coherent_dma_mask = DMA_BIT_MASK(32);
> + imx_dev->dma_mask = &imx_dev->coherent_dma_mask;
> + imx_dev->of_node = dev->of_node;
> +
> + platform_device_add_resources(imx_ahci_pdev, res, 2);
> + platform_device_add_data(imx_ahci_pdev, pdata, sizeof(*pdata));
> +
> + ret = platform_device_add(imx_ahci_pdev);
> +
> + return ret;
Can just be return platform_device_add(imx_ahci_pdev);
[Richard] Accepted.
Shawn
> +}
> +
> +static int imx_ahci_remove(struct platform_device *pdev) {
> + struct imx_ahci_priv *imxpriv = platform_get_drvdata(pdev);
> + struct platform_device *imx_ahci_pdev = imxpriv->imx_ahci_pdev;
> +
> + platform_device_unregister(imx_ahci_pdev);
> + return 0;
> +}
> +
> +static struct platform_driver imx_ahci_driver = {
> + .probe = imx_ahci_probe,
> + .remove = imx_ahci_remove,
> + .driver = {
> + .name = "sata-imx",
> + .owner = THIS_MODULE,
> + .of_match_table = imx_ahci_of_match,
> + },
> +};
> +module_platform_driver(imx_ahci_driver);
> +
> +MODULE_DESCRIPTION("Freescale i.MX AHCI SATA platform driver");
> +MODULE_AUTHOR("Richard Zhu <Hong-Xing.Zhu at freescale.com>");
> +MODULE_LICENSE("GPL"); MODULE_ALIAS("sata:imx");
> --
> 1.7.5.4
>
More information about the linux-arm-kernel
mailing list