Hi Shawn: Thanks for your comments. Best Regards Richard Zhu -----Original Message----- From: Shawn Guo [mailto:shawn.guo@xxxxxxxxxx] Sent: Friday, July 12, 2013 3:02 PM To: Richard Zhu Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; jgarzik@xxxxxxxxx; tj@xxxxxxxxxx; rob.herring@xxxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; linux-ide@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxx>"); > +MODULE_LICENSE("GPL"); MODULE_ALIAS("sata:imx"); > -- > 1.7.5.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
