| Message ID | 1380609183-21430-2-git-send-email-yuvaraj.cd@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Yuvaraj, On 1 October 2013 12:03, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote: > Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible > ahci_platform driver is not sufficient to handle the AHCI PHY and PHY > clock related initialization. > +err_out: > + platform_set_drvdata(pdev, NULL); This is not necessary. Driver core clears it upon detach or failure. > + platform_device_put(ahci_pdev); > + return ret; > + } > + > + return 0; > +} > + > +static int exynos_ahci_remove(struct platform_device *pdev) > +{ > + struct exynos_ahci_priv *priv = platform_get_drvdata(pdev); > + struct platform_device *ahci_pdev = priv->ahci_pdev; > + > + platform_device_unregister(ahci_pdev); > + platform_set_drvdata(pdev, NULL); ditto
On Tuesday 01 October 2013 12:03 PM, Yuvaraj Kumar C D wrote: > Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible > ahci_platform driver is not sufficient to handle the AHCI PHY and PHY > clock related initialization. > > This patch adds exynos specific ahci sata driver,contained the exynos > specific initialized codes, re-use the generic ahci_platform driver, and > keep the generic ahci_platform driver clean as much as possible. > > This patch depends on the below patch > [1].drivers: phy: add generic PHY framework > by Kishon Vijay Abraham I<kishon@ti.com> > > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> > --- > drivers/ata/Kconfig | 9 ++ > drivers/ata/Makefile | 1 + > drivers/ata/ahci_exynos.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 236 insertions(+) > create mode 100644 drivers/ata/ahci_exynos.c > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig > index 4e73772..99b2392 100644 > --- a/drivers/ata/Kconfig > +++ b/drivers/ata/Kconfig > @@ -106,6 +106,15 @@ config AHCI_IMX > > If unsure, say N. > > +config AHCI_EXYNOS > + tristate "Samsung Exynos AHCI SATA support" > + depends on SATA_AHCI_PLATFORM Can we select GENERIC_PHY here? > + help > + This option enables support for the Samsung's Exynos SoC's > + onboard AHCI SATA. > + > + If unsure, say N. > + > config SATA_FSL > tristate "Freescale 3.0Gbps SATA support" > depends on FSL_SOC > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile > index 46518c6..0e1f420f 100644 > --- a/drivers/ata/Makefile > +++ b/drivers/ata/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_SATA_SIL24) += sata_sil24.o > obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o > obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o > obj-$(CONFIG_AHCI_IMX) += ahci_imx.o > +obj-$(CONFIG_AHCI_EXYNOS) += ahci_exynos.o > > # SFF w/ custom DMA > obj-$(CONFIG_PDC_ADMA) += pdc_adma.o > diff --git a/drivers/ata/ahci_exynos.c b/drivers/ata/ahci_exynos.c > new file mode 100644 > index 0000000..7f0af00 > --- /dev/null > +++ b/drivers/ata/ahci_exynos.c > @@ -0,0 +1,226 @@ > +/* > + * Samsung AHCI SATA platform driver > + * Copyright 2013 Samsung Electronics Co., Ltd. > + * > + * 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/i2c.h> > +#include <linux/io.h> > +#include <linux/ahci_platform.h> > +#include <linux/of_device.h> > +#include <linux/phy/phy.h> > +#include "ahci.h" > + > +#define MHZ (1000 * 1000) > + > +struct exynos_ahci_priv { > + struct platform_device *ahci_pdev; > + struct clk *sclk; > + unsigned int freq; > + struct phy *phy; > +}; > + > +static int exynos_sata_init(struct device *dev, void __iomem *mmio) > +{ > + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); > + int ret; > + > + priv->phy = devm_phy_get(dev , "sata-phy"); ^^ spurious space here phy_get should be called from probe. > + if (IS_ERR(priv->phy)) > + return PTR_ERR(priv->phy); > + > + ret = phy_init(priv->phy); > + if (ret < 0) { > + dev_err(dev, "failed to init SATA PHY\n"); single tab is sufficient here and below. > + return ret; > + } > + > + ret = clk_prepare_enable(priv->sclk); > + if (ret < 0) { > + dev_err(dev, "failed to enable source clk\n"); > + return ret; > + } > + > + ret = clk_set_rate(priv->sclk, priv->freq * MHZ); > + if (ret < 0) { > + dev_err(dev, "failed to set clk frequency\n"); > + clk_disable_unprepare(priv->sclk); > + return ret; > + } > + > + return 0; > +} > + > +static void exynos_sata_exit(struct device *dev) > +{ > + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); > + if (!IS_ERR(priv->sclk)) This error check is not needed. You fail probe if you dont get clock anyways. > + clk_disable_unprepare(priv->sclk); > +} > + > +static int exynos_sata_suspend(struct device *dev) > +{ > + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); > + > + if (!IS_ERR(priv->sclk)) ditto.. > + clk_disable_unprepare(priv->sclk); > + phy_power_off(priv->phy); > + return 0; > +} > + > +static int exynos_sata_resume(struct device *dev) > +{ > + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); > + phy_power_on(priv->phy); > + exynos_sata_init(dev, NULL); > + return 0; > +} > + > +static struct ahci_platform_data exynos_sata_pdata = { > + .init = exynos_sata_init, Why is exynos_sata_init given as callback and also in exynos_sata_resume? > + .exit = exynos_sata_exit, > + .suspend = exynos_sata_suspend, > + .resume = exynos_sata_resume, Can't we use this in runtime_suspend/runtime_resume or suspend/resume pm ops? > +}; > + > +static const struct of_device_id exynos_ahci_of_match[] = { > + { .compatible = "samsung,exynos5250-sata-ahci", > + .data = &exynos_sata_pdata}, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, exynos_ahci_of_match); > + > +static int exynos_sata_parse_dt(struct device_node *np, > + struct exynos_ahci_priv *priv) > +{ > + if (!np) > + return -EINVAL; > + return of_property_read_u32(np, "samsung,sata-freq", > + &priv->freq); > +} > + > +static int exynos_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 exynos_ahci_priv *priv; > + struct device *ahci_dev; > + struct platform_device *ahci_pdev; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) { > + dev_err(dev, "can't alloc ahci_host_priv\n"); error message not needed. kzalloc will give a dump if it fails. > + return -ENOMEM; > + } > + > + ahci_pdev = platform_device_alloc("ahci", -1); Should be using PLATFORM_DEVID_AUTO unless it is absolutely necessary. > + if (!ahci_pdev) > + return -ENODEV; > + > + ahci_dev = &ahci_pdev->dev; > + ahci_dev->parent = dev; > + > + ret = exynos_sata_parse_dt(dev->of_node, priv); > + if (ret < 0) { > + dev_err(dev, "failed to parse device tree\n"); > + goto err_out; > + } > + > + priv->sclk = devm_clk_get(dev, "sclk_sata"); > + if (IS_ERR(priv->sclk)) { > + dev_err(dev, "failed to get sclk_sata\n"); > + ret = PTR_ERR(priv->sclk); > + goto err_out; > + } > + priv->ahci_pdev = ahci_pdev; > + platform_set_drvdata(pdev, priv); > + > + of_id = of_match_device(exynos_ahci_of_match, dev); > + if (of_id) { > + pdata = of_id->data; > + } else { > + ret = -EINVAL; > + goto err_out; > + } > + > + 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"); > + ret = -ENOMEM; > + goto err_out; > + } > + > + res[0] = *mem; > + res[1] = *irq; > + > + ahci_dev->coherent_dma_mask = DMA_BIT_MASK(32); > + ahci_dev->dma_mask = &ahci_dev->coherent_dma_mask; > + ahci_dev->of_node = dev->of_node; > + > + ret = platform_device_add_resources(ahci_pdev, res, 2); > + if (ret) > + goto err_out; > + > + ret = platform_device_add_data(ahci_pdev, pdata, sizeof(*pdata)); > + if (ret) > + goto err_out; > + > + ret = platform_device_add(ahci_pdev); > + if (ret) { > + > + > +err_out: > + platform_set_drvdata(pdev, NULL); > + platform_device_put(ahci_pdev); > + return ret; Move this to the end of this function and then goto err_out for better readability. > + } > + > + return 0; > +} > + > +static int exynos_ahci_remove(struct platform_device *pdev) > +{ > + struct exynos_ahci_priv *priv = platform_get_drvdata(pdev); > + struct platform_device *ahci_pdev = priv->ahci_pdev; > + > + platform_device_unregister(ahci_pdev); > + platform_set_drvdata(pdev, NULL); > + > + return 0; > +} > + > +static struct platform_driver exynos_ahci_driver = { > + .probe = exynos_ahci_probe, > + .remove = exynos_ahci_remove, > + .driver = { > + .name = "sata-exynos", > + .owner = THIS_MODULE, > + .of_match_table = exynos_ahci_of_match, > + }, > +}; > +module_platform_driver(exynos_ahci_driver); > + > +MODULE_DESCRIPTION("Samsung Exynos AHCI SATA platform driver"); > +MODULE_AUTHOR("Yuvaraj C D <yuvaraj.cd@samsung.com>"); > +MODULE_LICENSE("GPL"); GPL v2? Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tuesday, October 01, 2013 12:03:01 PM Yuvaraj Kumar C D wrote: > Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible > ahci_platform driver is not sufficient to handle the AHCI PHY and PHY > clock related initialization. > > This patch adds exynos specific ahci sata driver,contained the exynos > specific initialized codes, re-use the generic ahci_platform driver, and > keep the generic ahci_platform driver clean as much as possible. > > This patch depends on the below patch > [1].drivers: phy: add generic PHY framework > by Kishon Vijay Abraham I<kishon@ti.com> > > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> > --- > drivers/ata/Kconfig | 9 ++ > drivers/ata/Makefile | 1 + > drivers/ata/ahci_exynos.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 236 insertions(+) > create mode 100644 drivers/ata/ahci_exynos.c > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig > index 4e73772..99b2392 100644 > --- a/drivers/ata/Kconfig > +++ b/drivers/ata/Kconfig > @@ -106,6 +106,15 @@ config AHCI_IMX > > If unsure, say N. > > +config AHCI_EXYNOS > + tristate "Samsung Exynos AHCI SATA support" > + depends on SATA_AHCI_PLATFORM This should also depend on SOC_EXYNOS5250. > + help > + This option enables support for the Samsung's Exynos SoC's > + onboard AHCI SATA. > + > + If unsure, say N. > + > config SATA_FSL > tristate "Freescale 3.0Gbps SATA support" > depends on FSL_SOC > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile > index 46518c6..0e1f420f 100644 > --- a/drivers/ata/Makefile > +++ b/drivers/ata/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_SATA_SIL24) += sata_sil24.o > obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o > obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o > obj-$(CONFIG_AHCI_IMX) += ahci_imx.o > +obj-$(CONFIG_AHCI_EXYNOS) += ahci_exynos.o > > # SFF w/ custom DMA > obj-$(CONFIG_PDC_ADMA) += pdc_adma.o > diff --git a/drivers/ata/ahci_exynos.c b/drivers/ata/ahci_exynos.c > new file mode 100644 > index 0000000..7f0af00 > --- /dev/null > +++ b/drivers/ata/ahci_exynos.c > @@ -0,0 +1,226 @@ > +/* > + * Samsung AHCI SATA platform driver > + * Copyright 2013 Samsung Electronics Co., Ltd. > + * > + * 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/i2c.h> This include doesn't seem to be needed. > +#include <linux/io.h> > +#include <linux/ahci_platform.h> > +#include <linux/of_device.h> > +#include <linux/phy/phy.h> > +#include "ahci.h" > + > +#define MHZ (1000 * 1000) > + > +struct exynos_ahci_priv { > + struct platform_device *ahci_pdev; > + struct clk *sclk; > + unsigned int freq; > + struct phy *phy; > +}; > + > +static int exynos_sata_init(struct device *dev, void __iomem *mmio) > +{ > + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); > + int ret; > + > + priv->phy = devm_phy_get(dev , "sata-phy"); > + if (IS_ERR(priv->phy)) > + return PTR_ERR(priv->phy); This should happen in ->probe (exynos_sata_init() is also called in exynos_sata_resume() so the above code will be executed on every resume operation which is incorrect). Also please take a look at the following patch: https://lkml.org/lkml/2013/9/19/173 it adds PHY support to ahci_platform driver, maybe it can be used for your needs as well. > + ret = phy_init(priv->phy); > + if (ret < 0) { > + dev_err(dev, "failed to init SATA PHY\n"); > + return ret; > + } > + > + ret = clk_prepare_enable(priv->sclk); > + if (ret < 0) { > + dev_err(dev, "failed to enable source clk\n"); > + return ret; > + } > + > + ret = clk_set_rate(priv->sclk, priv->freq * MHZ); > + if (ret < 0) { > + dev_err(dev, "failed to set clk frequency\n"); > + clk_disable_unprepare(priv->sclk); > + return ret; > + } > + > + return 0; > +} > + > +static void exynos_sata_exit(struct device *dev) > +{ > + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); > + if (!IS_ERR(priv->sclk)) > + clk_disable_unprepare(priv->sclk); > +} > + > +static int exynos_sata_suspend(struct device *dev) > +{ > + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); > + > + if (!IS_ERR(priv->sclk)) > + clk_disable_unprepare(priv->sclk); > + phy_power_off(priv->phy); > + return 0; > +} > + > +static int exynos_sata_resume(struct device *dev) > +{ > + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); > + phy_power_on(priv->phy); > + exynos_sata_init(dev, NULL); > + return 0; It should be: return exynos_sata_init(dev, NULL); Also please fix exynos_sata_suspend() to call exynos_sata_exit(). [...] Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, October 03, 2013 8:32 PM, Bartlomiej Zolnierkiewicz wrote: > On Tuesday, October 01, 2013 12:03:01 PM Yuvaraj Kumar C D wrote: > > Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible > > ahci_platform driver is not sufficient to handle the AHCI PHY and PHY > > clock related initialization. > > > > This patch adds exynos specific ahci sata driver,contained the exynos > > specific initialized codes, re-use the generic ahci_platform driver, and > > keep the generic ahci_platform driver clean as much as possible. > > > > This patch depends on the below patch > > [1].drivers: phy: add generic PHY framework > > by Kishon Vijay Abraham I<kishon@ti.com> > > > > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> > > --- > > drivers/ata/Kconfig | 9 ++ > > drivers/ata/Makefile | 1 + > > drivers/ata/ahci_exynos.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 236 insertions(+) > > create mode 100644 drivers/ata/ahci_exynos.c > > [.....] > > + priv->phy = devm_phy_get(dev , "sata-phy"); > > + if (IS_ERR(priv->phy)) > > + return PTR_ERR(priv->phy); [.....] > Also please take a look at the following patch: > > https://lkml.org/lkml/2013/9/19/173 > > it adds PHY support to ahci_platform driver, maybe it can be used > for your needs as well. I also agree with Bartlomiej Zolnierkiewicz's opinion. 'ahci_exynos.c' just calls PHY API, without any additional control for Exynos AHCI IP. Best regards, Jingoo Han > > > + ret = phy_init(priv->phy); > > + if (ret < 0) { > > + dev_err(dev, "failed to init SATA PHY\n"); > > + return ret; > > + } > > + -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 4, 2013 at 6:03 AM, Jingoo Han <jg1.han@samsung.com> wrote: > On Thursday, October 03, 2013 8:32 PM, Bartlomiej Zolnierkiewicz wrote: >> On Tuesday, October 01, 2013 12:03:01 PM Yuvaraj Kumar C D wrote: >> > Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible >> > ahci_platform driver is not sufficient to handle the AHCI PHY and PHY >> > clock related initialization. >> > >> > This patch adds exynos specific ahci sata driver,contained the exynos >> > specific initialized codes, re-use the generic ahci_platform driver, and >> > keep the generic ahci_platform driver clean as much as possible. >> > >> > This patch depends on the below patch >> > [1].drivers: phy: add generic PHY framework >> > by Kishon Vijay Abraham I<kishon@ti.com> >> > >> > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >> > --- >> > drivers/ata/Kconfig | 9 ++ >> > drivers/ata/Makefile | 1 + >> > drivers/ata/ahci_exynos.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 236 insertions(+) >> > create mode 100644 drivers/ata/ahci_exynos.c >> > > > > [.....] > >> > + priv->phy = devm_phy_get(dev , "sata-phy"); >> > + if (IS_ERR(priv->phy)) >> > + return PTR_ERR(priv->phy); > > [.....] > >> Also please take a look at the following patch: >> >> https://lkml.org/lkml/2013/9/19/173 >> >> it adds PHY support to ahci_platform driver, maybe it can be used >> for your needs as well. > > I also agree with Bartlomiej Zolnierkiewicz's opinion. > 'ahci_exynos.c' just calls PHY API, without any additional control > for Exynos AHCI IP. In addition to PHY handling,it also deals with the special clock sclk_sata which is not dealt in ahci_platform.c(certainly exynos specific). Morever there is a wrapper driver to handle the platform specific things for the sata.Please refer the patch[1] [1]ata: ti_sata: Add Texas Instruments SATA Wrapper driver https://lkml.org/lkml/2013/9/19/166 [2]ahci_imx: add ahci sata support on imx platforms I think, if we have platform specific driver like ahci_xxx.c , it would be better to handle the sata PHY in ahci_xxx.c so that we can retain and re-use the ahci_platform.c as it is. Further comments will be much appreciated. > > Best regards, > Jingoo Han > >> >> > + ret = phy_init(priv->phy); >> > + if (ret < 0) { >> > + dev_err(dev, "failed to init SATA PHY\n"); >> > + return ret; >> > + } >> > + > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10/08/2013 02:44 PM, Yuvaraj Kumar wrote: > On Fri, Oct 4, 2013 at 6:03 AM, Jingoo Han <jg1.han@samsung.com> wrote: >> On Thursday, October 03, 2013 8:32 PM, Bartlomiej Zolnierkiewicz wrote: >>> On Tuesday, October 01, 2013 12:03:01 PM Yuvaraj Kumar C D wrote: >>>> Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible >>>> ahci_platform driver is not sufficient to handle the AHCI PHY and PHY >>>> clock related initialization. >>>> >>>> This patch adds exynos specific ahci sata driver,contained the exynos >>>> specific initialized codes, re-use the generic ahci_platform driver, and >>>> keep the generic ahci_platform driver clean as much as possible. >>>> >>>> This patch depends on the below patch >>>> [1].drivers: phy: add generic PHY framework >>>> by Kishon Vijay Abraham I<kishon@ti.com> >>>> >>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> >>>> --- >>>> drivers/ata/Kconfig | 9 ++ >>>> drivers/ata/Makefile | 1 + >>>> drivers/ata/ahci_exynos.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 236 insertions(+) >>>> create mode 100644 drivers/ata/ahci_exynos.c >>>> >> >> >> [.....] >> >>>> + priv->phy = devm_phy_get(dev , "sata-phy"); >>>> + if (IS_ERR(priv->phy)) >>>> + return PTR_ERR(priv->phy); >> >> [.....] >> >>> Also please take a look at the following patch: >>> >>> https://lkml.org/lkml/2013/9/19/173 >>> >>> it adds PHY support to ahci_platform driver, maybe it can be used >>> for your needs as well. >> >> I also agree with Bartlomiej Zolnierkiewicz's opinion. >> 'ahci_exynos.c' just calls PHY API, without any additional control >> for Exynos AHCI IP. > In addition to PHY handling,it also deals with the special clock > sclk_sata which is not dealt in ahci_platform.c(certainly exynos > specific). > > Morever there is a wrapper driver to handle the platform specific > things for the sata.Please refer the patch[1] > [1]ata: ti_sata: Add Texas Instruments SATA Wrapper driver > https://lkml.org/lkml/2013/9/19/166 This driver doesn't do anything at the moment since the OMAP hwmod layer takes care of enabling the relevant clocks. So I was planning to drop it for now. > > [2]ahci_imx: add ahci sata support on imx platforms > > I think, if we have platform specific driver like ahci_xxx.c , it > would be better to handle the sata PHY in ahci_xxx.c so that we can > retain and re-use the ahci_platform.c as it is. > The Generic PHY framework [3] has been merged into Greg's usb-next branch. The operations there are pretty generic and IMO the PHY can be handled in the ahci_platform driver. [3] Generic PHY framework https://lkml.org/lkml/2013/9/27/581 cheers, -roger > Further comments will be much appreciated. > >> >> Best regards, >> Jingoo Han >> >>> >>>> + ret = phy_init(priv->phy); >>>> + if (ret < 0) { >>>> + dev_err(dev, "failed to init SATA PHY\n"); >>>> + return ret; >>>> + } >>>> + >> -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, October 08, 2013 8:45 PM, Yuvaraj Kumar wrote: > On Fri, Oct 4, 2013 at 6:03 AM, Jingoo Han <jg1.han@samsung.com> wrote: > > On Thursday, October 03, 2013 8:32 PM, Bartlomiej Zolnierkiewicz wrote: > >> On Tuesday, October 01, 2013 12:03:01 PM Yuvaraj Kumar C D wrote: > >> > Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible > >> > ahci_platform driver is not sufficient to handle the AHCI PHY and PHY > >> > clock related initialization. > >> > > >> > This patch adds exynos specific ahci sata driver,contained the exynos > >> > specific initialized codes, re-use the generic ahci_platform driver, and > >> > keep the generic ahci_platform driver clean as much as possible. > >> > > >> > This patch depends on the below patch > >> > [1].drivers: phy: add generic PHY framework > >> > by Kishon Vijay Abraham I<kishon@ti.com> > >> > > >> > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> > >> > --- > >> > drivers/ata/Kconfig | 9 ++ > >> > drivers/ata/Makefile | 1 + > >> > drivers/ata/ahci_exynos.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ > >> > 3 files changed, 236 insertions(+) > >> > create mode 100644 drivers/ata/ahci_exynos.c > >> > > > > > > > [.....] > > > >> > + priv->phy = devm_phy_get(dev , "sata-phy"); > >> > + if (IS_ERR(priv->phy)) > >> > + return PTR_ERR(priv->phy); > > > > [.....] > > > >> Also please take a look at the following patch: > >> > >> https://lkml.org/lkml/2013/9/19/173 > >> > >> it adds PHY support to ahci_platform driver, maybe it can be used > >> for your needs as well. > > > > I also agree with Bartlomiej Zolnierkiewicz's opinion. > > 'ahci_exynos.c' just calls PHY API, without any additional control > > for Exynos AHCI IP. > In addition to PHY handling,it also deals with the special clock > sclk_sata which is not dealt in ahci_platform.c(certainly exynos > specific). Handling the special clock 'sclk_sata' is another issue. Please, look at that other 'sclk_*'s are handled. Also, if possible, please add 'the code handling the special clock' to 'ahci_platform.c'. > > Morever there is a wrapper driver to handle the platform specific > things for the sata.Please refer the patch[1] > [1]ata: ti_sata: Add Texas Instruments SATA Wrapper driver > https://lkml.org/lkml/2013/9/19/166 As Roger Quadros mentioned, 'ti_sata' driver will be dropped. > [2]ahci_imx: add ahci sata support on imx platforms > 'ahci_imx' is necessary, because 'ahci_imx' controls some specific registers. However, 'ahci_exynos.c' does not control any registers. > I think, if we have platform specific driver like ahci_xxx.c , it > would be better to handle the sata PHY in ahci_xxx.c so that we can > retain and re-use the ahci_platform.c as it is. I think that the platform specific driver like ahci_xxx.c is not necessary, because 'ahci_exynos.c' does not control any special registers. Best regards, Jingoo Han > > Further comments will be much appreciated. > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Friday 11 of October 2013 15:49:04 Jingoo Han wrote: > On Tuesday, October 08, 2013 8:45 PM, Yuvaraj Kumar wrote: > > On Fri, Oct 4, 2013 at 6:03 AM, Jingoo Han <jg1.han@samsung.com> wrote: > > > On Thursday, October 03, 2013 8:32 PM, Bartlomiej Zolnierkiewicz wrote: > > >> On Tuesday, October 01, 2013 12:03:01 PM Yuvaraj Kumar C D wrote: > > >> > Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible > > >> > ahci_platform driver is not sufficient to handle the AHCI PHY and PHY > > >> > clock related initialization. > > >> > > > >> > This patch adds exynos specific ahci sata driver,contained the exynos > > >> > specific initialized codes, re-use the generic ahci_platform driver, and > > >> > keep the generic ahci_platform driver clean as much as possible. > > >> > > > >> > This patch depends on the below patch > > >> > [1].drivers: phy: add generic PHY framework > > >> > by Kishon Vijay Abraham I<kishon@ti.com> > > >> > > > >> > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> > > >> > --- > > >> > drivers/ata/Kconfig | 9 ++ > > >> > drivers/ata/Makefile | 1 + > > >> > drivers/ata/ahci_exynos.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ > > >> > 3 files changed, 236 insertions(+) > > >> > create mode 100644 drivers/ata/ahci_exynos.c > > >> > > > > > > > > > > [.....] > > > > > >> > + priv->phy = devm_phy_get(dev , "sata-phy"); > > >> > + if (IS_ERR(priv->phy)) > > >> > + return PTR_ERR(priv->phy); > > > > > > [.....] > > > > > >> Also please take a look at the following patch: > > >> > > >> https://lkml.org/lkml/2013/9/19/173 > > >> > > >> it adds PHY support to ahci_platform driver, maybe it can be used > > >> for your needs as well. > > > > > > I also agree with Bartlomiej Zolnierkiewicz's opinion. > > > 'ahci_exynos.c' just calls PHY API, without any additional control > > > for Exynos AHCI IP. > > In addition to PHY handling,it also deals with the special clock > > sclk_sata which is not dealt in ahci_platform.c(certainly exynos > > specific). > > Handling the special clock 'sclk_sata' is another issue. > Please, look at that other 'sclk_*'s are handled. > Also, if possible, please add 'the code handling the special clock' > to 'ahci_platform.c'. > > > > > Morever there is a wrapper driver to handle the platform specific > > things for the sata.Please refer the patch[1] > > [1]ata: ti_sata: Add Texas Instruments SATA Wrapper driver > > https://lkml.org/lkml/2013/9/19/166 > > As Roger Quadros mentioned, 'ti_sata' driver will be dropped. > > > [2]ahci_imx: add ahci sata support on imx platforms > > > > 'ahci_imx' is necessary, because 'ahci_imx' controls some > specific registers. > > However, 'ahci_exynos.c' does not control any registers. > > > I think, if we have platform specific driver like ahci_xxx.c , it > > would be better to handle the sata PHY in ahci_xxx.c so that we can > > retain and re-use the ahci_platform.c as it is. > > I think that the platform specific driver like ahci_xxx.c is not > necessary, because 'ahci_exynos.c' does not control any special > registers. My big +1 to all the JIngoo's points above. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index 4e73772..99b2392 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -106,6 +106,15 @@ config AHCI_IMX If unsure, say N. +config AHCI_EXYNOS + tristate "Samsung Exynos AHCI SATA support" + depends on SATA_AHCI_PLATFORM + help + This option enables support for the Samsung's Exynos SoC's + onboard AHCI SATA. + + If unsure, say N. + config SATA_FSL tristate "Freescale 3.0Gbps SATA support" depends on FSL_SOC diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile index 46518c6..0e1f420f 100644 --- a/drivers/ata/Makefile +++ b/drivers/ata/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_SATA_SIL24) += sata_sil24.o obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o obj-$(CONFIG_AHCI_IMX) += ahci_imx.o +obj-$(CONFIG_AHCI_EXYNOS) += ahci_exynos.o # SFF w/ custom DMA obj-$(CONFIG_PDC_ADMA) += pdc_adma.o diff --git a/drivers/ata/ahci_exynos.c b/drivers/ata/ahci_exynos.c new file mode 100644 index 0000000..7f0af00 --- /dev/null +++ b/drivers/ata/ahci_exynos.c @@ -0,0 +1,226 @@ +/* + * Samsung AHCI SATA platform driver + * Copyright 2013 Samsung Electronics Co., Ltd. + * + * 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/i2c.h> +#include <linux/io.h> +#include <linux/ahci_platform.h> +#include <linux/of_device.h> +#include <linux/phy/phy.h> +#include "ahci.h" + +#define MHZ (1000 * 1000) + +struct exynos_ahci_priv { + struct platform_device *ahci_pdev; + struct clk *sclk; + unsigned int freq; + struct phy *phy; +}; + +static int exynos_sata_init(struct device *dev, void __iomem *mmio) +{ + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); + int ret; + + priv->phy = devm_phy_get(dev , "sata-phy"); + if (IS_ERR(priv->phy)) + return PTR_ERR(priv->phy); + + ret = phy_init(priv->phy); + if (ret < 0) { + dev_err(dev, "failed to init SATA PHY\n"); + return ret; + } + + ret = clk_prepare_enable(priv->sclk); + if (ret < 0) { + dev_err(dev, "failed to enable source clk\n"); + return ret; + } + + ret = clk_set_rate(priv->sclk, priv->freq * MHZ); + if (ret < 0) { + dev_err(dev, "failed to set clk frequency\n"); + clk_disable_unprepare(priv->sclk); + return ret; + } + + return 0; +} + +static void exynos_sata_exit(struct device *dev) +{ + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); + if (!IS_ERR(priv->sclk)) + clk_disable_unprepare(priv->sclk); +} + +static int exynos_sata_suspend(struct device *dev) +{ + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); + + if (!IS_ERR(priv->sclk)) + clk_disable_unprepare(priv->sclk); + phy_power_off(priv->phy); + return 0; +} + +static int exynos_sata_resume(struct device *dev) +{ + struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent); + phy_power_on(priv->phy); + exynos_sata_init(dev, NULL); + return 0; +} + +static struct ahci_platform_data exynos_sata_pdata = { + .init = exynos_sata_init, + .exit = exynos_sata_exit, + .suspend = exynos_sata_suspend, + .resume = exynos_sata_resume, +}; + +static const struct of_device_id exynos_ahci_of_match[] = { + { .compatible = "samsung,exynos5250-sata-ahci", + .data = &exynos_sata_pdata}, + {}, +}; +MODULE_DEVICE_TABLE(of, exynos_ahci_of_match); + +static int exynos_sata_parse_dt(struct device_node *np, + struct exynos_ahci_priv *priv) +{ + if (!np) + return -EINVAL; + return of_property_read_u32(np, "samsung,sata-freq", + &priv->freq); +} + +static int exynos_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 exynos_ahci_priv *priv; + struct device *ahci_dev; + struct platform_device *ahci_pdev; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) { + dev_err(dev, "can't alloc ahci_host_priv\n"); + return -ENOMEM; + } + + ahci_pdev = platform_device_alloc("ahci", -1); + if (!ahci_pdev) + return -ENODEV; + + ahci_dev = &ahci_pdev->dev; + ahci_dev->parent = dev; + + ret = exynos_sata_parse_dt(dev->of_node, priv); + if (ret < 0) { + dev_err(dev, "failed to parse device tree\n"); + goto err_out; + } + + priv->sclk = devm_clk_get(dev, "sclk_sata"); + if (IS_ERR(priv->sclk)) { + dev_err(dev, "failed to get sclk_sata\n"); + ret = PTR_ERR(priv->sclk); + goto err_out; + } + priv->ahci_pdev = ahci_pdev; + platform_set_drvdata(pdev, priv); + + of_id = of_match_device(exynos_ahci_of_match, dev); + if (of_id) { + pdata = of_id->data; + } else { + ret = -EINVAL; + goto err_out; + } + + 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"); + ret = -ENOMEM; + goto err_out; + } + + res[0] = *mem; + res[1] = *irq; + + ahci_dev->coherent_dma_mask = DMA_BIT_MASK(32); + ahci_dev->dma_mask = &ahci_dev->coherent_dma_mask; + ahci_dev->of_node = dev->of_node; + + ret = platform_device_add_resources(ahci_pdev, res, 2); + if (ret) + goto err_out; + + ret = platform_device_add_data(ahci_pdev, pdata, sizeof(*pdata)); + if (ret) + goto err_out; + + ret = platform_device_add(ahci_pdev); + if (ret) { + + +err_out: + platform_set_drvdata(pdev, NULL); + platform_device_put(ahci_pdev); + return ret; + } + + return 0; +} + +static int exynos_ahci_remove(struct platform_device *pdev) +{ + struct exynos_ahci_priv *priv = platform_get_drvdata(pdev); + struct platform_device *ahci_pdev = priv->ahci_pdev; + + platform_device_unregister(ahci_pdev); + platform_set_drvdata(pdev, NULL); + + return 0; +} + +static struct platform_driver exynos_ahci_driver = { + .probe = exynos_ahci_probe, + .remove = exynos_ahci_remove, + .driver = { + .name = "sata-exynos", + .owner = THIS_MODULE, + .of_match_table = exynos_ahci_of_match, + }, +}; +module_platform_driver(exynos_ahci_driver); + +MODULE_DESCRIPTION("Samsung Exynos AHCI SATA platform driver"); +MODULE_AUTHOR("Yuvaraj C D <yuvaraj.cd@samsung.com>"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("ahci:exynos");
Exynos5250 contains one Synopsys AHCI SATA controller.The avalaible ahci_platform driver is not sufficient to handle the AHCI PHY and PHY clock related initialization. This patch adds exynos specific ahci sata driver,contained the exynos specific initialized codes, re-use the generic ahci_platform driver, and keep the generic ahci_platform driver clean as much as possible. This patch depends on the below patch [1].drivers: phy: add generic PHY framework by Kishon Vijay Abraham I<kishon@ti.com> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com> --- drivers/ata/Kconfig | 9 ++ drivers/ata/Makefile | 1 + drivers/ata/ahci_exynos.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 236 insertions(+) create mode 100644 drivers/ata/ahci_exynos.c