[PATCH 0/2] Adding SATA PHY framework and PHY controller driver

[PATCH 0/2] Adding SATA PHY framework and PHY controller driver

[Vasanth Ananthan]

Adding SATA PHY framework. The framework acts as an interface
between the SATA controller and the PHY controller.

The framework can be used by any platform with device tree support.
It primarily serves to de-couple the SATA controller driver and the
PHY controller driver.

Vasanth Ananthan (2):
  driver: ata: SATA PHY framework
  driver: ata: add new Exynos5250 SATA PHY controller driver

 arch/arm/mach-exynos/include/mach/regs-pmu.h  |    3 +
 arch/arm/mach-exynos/include/mach/regs-sata.h |   29 +++
 drivers/ata/Kconfig                           |   10 +
 drivers/ata/Makefile                          |    1 +
 drivers/ata/sata_exynos_phy.c                 |  265 +++++++++++++++++++++++++
 drivers/ata/sata_phy.c                        |  145 ++++++++++++++
 drivers/ata/sata_phy.h                        |   29 +++
 7 files changed, 482 insertions(+)
 create mode 100644 arch/arm/mach-exynos/include/mach/regs-sata.h
 create mode 100644 drivers/ata/sata_exynos_phy.c
 create mode 100644 drivers/ata/sata_phy.c
 create mode 100644 drivers/ata/sata_phy.h

-- 
1.7.9.5

[PATCH 1/2] driver: ata: SATA PHY framework

[Vasanth Ananthan]

This patch adds SATA PHY framework APIs. The framework acts as an
interface between the SATA controller and the PHY controller.
The SATA PHY controller driver registers the PHY  with the framework
through the sata_add_phy call, passing its device_node. The SATA controller
requests for an appropriate PHY controller by passing the device_node of
the PHY, the SATA controller node is mapped to in the device tree, to
sata_get_phy call. After getting the PHY, SATA controllers initializes
corresponding PHY controller through sata_init_phy call.

Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
---
 drivers/ata/Kconfig    |   10 ++++
 drivers/ata/Makefile   |    1 +
 drivers/ata/sata_phy.c |  146 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/sata_phy.h |   29 ++++++++++
 4 files changed, 186 insertions(+)
 create mode 100644 drivers/ata/sata_phy.c
 create mode 100644 drivers/ata/sata_phy.h

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 996d16c..99c1e1b 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -96,6 +96,16 @@ config SATA_AHCI_PLATFORM
 
 	  If unsure, say N.
 
+config SATA_PHY
+	bool "SATA PHY Framework"
+	default n
+	help
+	  This option enables the SATA PHY utility framework APIs.
+	  The framework acts as an interface between the SATA
+	  controller and the PHY controller. The PHY controller
+	  registers itself with the framework through the APIs provided and the SATA
+	  controller finds and requests for an appropriate PHY controller.
+
 config SATA_FSL
 	tristate "Freescale 3.0Gbps SATA support"
 	depends on FSL_SOC
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 85e3de4..3d2d128 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_PHY)		+= sata_phy.o
 
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
diff --git a/drivers/ata/sata_phy.c b/drivers/ata/sata_phy.c
new file mode 100644
index 0000000..0f56878
--- /dev/null
+++ b/drivers/ata/sata_phy.c
@@ -0,0 +1,146 @@
+/*
+ * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
+ *              http://www.samsung.com
+ *
+ * SATA PHY framework.
+ *
+ * This file provides a set of functions/interfaces for establishing
+ * communication between SATA controller and the PHY controller. A
+ * PHY controller driver registers call backs for its initialization and
+ * shutdown. The SATA controller driver finds the appropriate PHYs for
+ * its implemented ports and initialize/shutdown PHYs through the
+ * call backs provided.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/kernel.h>
+#include <linux/export.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include "sata_phy.h"
+
+static LIST_HEAD(phy_list);
+static DEFINE_SPINLOCK(phy_lock);
+
+struct sata_phy *sata_get_phy(struct device_node *phy_np)
+{
+	struct sata_phy *phy;
+	unsigned long flag;
+
+	spin_lock_irqsave(&phy_lock, flag);
+
+	if (list_empty(&phy_list)) {
+		spin_unlock_irqrestore(&phy_lock, flag);
+		return ERR_PTR(-ENODEV);
+	}
+
+	list_for_each_entry(phy, &phy_list, head) {
+		if (phy->dev->of_node == phy_np) {
+			if (phy->status == IN_USE) {
+				pr_info(KERN_INFO
+					"PHY already in use\n");
+				spin_unlock_irqrestore(&phy_lock, flag);
+				return ERR_PTR(-EBUSY);
+			}
+
+			get_device(phy->dev);
+			phy->status = IN_USE;
+			spin_unlock_irqrestore(&phy_lock, flag);
+			return phy;
+		}
+	}
+
+	spin_unlock_irqrestore(&phy_lock, flag);
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL(sata_get_phy);
+
+int sata_add_phy(struct sata_phy *sataphy)
+{
+	unsigned long flag;
+	unsigned int ret = -EINVAL;
+	struct sata_phy *phy;
+
+	if (!sataphy)
+		return ret;
+
+	spin_lock_irqsave(&phy_lock, flag);
+
+	list_for_each_entry(phy, &phy_list, head) {
+		if (phy->dev->of_node == sataphy->dev->of_node) {
+			dev_err(sataphy->dev, "PHY already exists in the list\n");
+			goto out;
+		}
+	}
+
+	sataphy->status = NOT_IN_USE;
+	list_add_tail(&sataphy->head, &phy_list);
+	ret = 0;
+
+ out:
+	spin_unlock_irqrestore(&phy_lock, flag);
+	return ret;
+}
+EXPORT_SYMBOL(sata_add_phy);
+
+void sata_remove_phy(struct sata_phy *sataphy)
+{
+	unsigned long flag;
+	struct sata_phy *phy;
+
+	if (!sataphy)
+		return;
+
+	if (sataphy->status == IN_USE) {
+		pr_info(KERN_INFO
+			"PHY in use, cannot be removed\n");
+		return;
+	}
+
+	spin_lock_irqsave(&phy_lock, flag);
+
+	list_for_each_entry(phy, &phy_list, head) {
+		if (phy->dev->of_node == sataphy->dev->of_node)
+			list_del(&phy->head);
+	}
+
+	spin_unlock_irqrestore(&phy_lock, flag);
+}
+EXPORT_SYMBOL(sata_remove_phy);
+
+void sata_put_phy(struct sata_phy *sataphy)
+{
+	unsigned long flag;
+
+	if (!sataphy)
+		return;
+
+	spin_lock_irqsave(&phy_lock, flag);
+
+	put_device(sataphy->dev);
+	sataphy->status = NOT_IN_USE;
+
+	spin_unlock_irqrestore(&phy_lock, flag);
+}
+EXPORT_SYMBOL(sata_put_phy);
+
+int sata_init_phy(struct sata_phy *sataphy)
+{
+	if (sataphy && sataphy->init)
+		return sataphy->init(sataphy);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(sata_init_phy);
+
+void sata_shutdown_phy(struct sata_phy *sataphy)
+{
+	if (sataphy && sataphy->shutdown)
+		sataphy->shutdown(sataphy);
+}
+EXPORT_SYMBOL(sata_shutdown_phy);
diff --git a/drivers/ata/sata_phy.h b/drivers/ata/sata_phy.h
new file mode 100644
index 0000000..03ae2f9
--- /dev/null
+++ b/drivers/ata/sata_phy.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
+ *              http://www.samsung.com
+ *
+ * SATA utility framework definitions.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#define IN_USE		1
+#define NOT_IN_USE	0
+
+struct sata_phy {
+	int (*init) (struct sata_phy *);
+	int (*shutdown) (struct sata_phy *);
+	struct device *dev;
+	void *priv_data;
+	struct list_head head;
+	unsigned char status;
+};
+
+struct sata_phy *sata_get_phy(struct device_node *);
+int sata_add_phy(struct sata_phy *);
+void sata_remove_phy(struct sata_phy *);
+void sata_put_phy(struct sata_phy *);
+int sata_init_phy(struct sata_phy *);
+void sata_shutdown_phy(struct sata_phy *);
-- 
1.7.9.5

[PATCH 2/2] driver: ata: add new Exynos5250 SATA PHY controller driver

[Vasanth Ananthan]

Adding platform driver and I2C client driver for SATA PHY controller
for Samsung Exynos5250.

The PHY controller in Exynos5250 has both the APB interface
and I2C client interface, hence it requires both a platform driver
and an I2C client driver. The PHY controller's primary charecteristics
are handled through the APB interface, facilitated by the platform driver
and the 40 bit interface should be enabled through the I2C interface,
facilitated by the I2C client driver.

Further, this PHY controller driver uses PHY framework introduced by this
patchset. The driver registers its initialization/shutdown call back to the
framework and corresponding port this PHY controller is assciated with
gets this PHY and initializes it.

Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
---
 arch/arm/mach-exynos/include/mach/regs-pmu.h  |    3 +
 arch/arm/mach-exynos/include/mach/regs-sata.h |   29 +++
 drivers/ata/Makefile                          |    2 +-
 drivers/ata/sata_exynos_phy.c                 |  265 +++++++++++++++++++++++++
 4 files changed, 298 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-exynos/include/mach/regs-sata.h
 create mode 100644 drivers/ata/sata_exynos_phy.c

diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h
index 3f30aa1..fd813d9 100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -369,4 +369,7 @@
 
 #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
 
+/* Only for EXYNOS5250 */
+#define EXYNOS5_SATA_PHY_CONTROL				S5P_PMUREG(0x0724)
+
 #endif /* __ASM_ARCH_REGS_PMU_H */
diff --git a/arch/arm/mach-exynos/include/mach/regs-sata.h b/arch/arm/mach-exynos/include/mach/regs-sata.h
new file mode 100644
index 0000000..80dd564
--- /dev/null
+++ b/arch/arm/mach-exynos/include/mach/regs-sata.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
+ *              http://www.samsung.com
+ *
+ * EXYNOS - SATA PHY controller definition
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#define EXYNOS5_SATA_RESET		0x4
+#define RESET_CMN_RST_N			(1 << 1)
+#define LINK_RESET			0xF0000
+
+#define EXYNOS5_SATA_MODE0		0x10
+
+#define EXYNOS5_SATA_CTRL0		0x14
+#define CTRL0_P0_PHY_CALIBRATED_SEL	(1 << 9)
+#define CTRL0_P0_PHY_CALIBRATED		(1 << 8)
+
+#define EXYNOS5_SATA_PHSATA_CTRLM	0xE0
+#define PHCTRLM_REF_RATE		(1 << 1)
+#define PHCTRLM_HIGH_SPEED		(1 << 0)
+
+#define EXYNOS5_SATA_PHSATA_STATM	0xF0
+#define PHSTATM_PLL_LOCKED		(1 << 0)
+
+#define SATA_PHY_CON_RESET              0xF003F
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 3d2d128..7add682 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -10,7 +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_PHY)		+= sata_phy.o
+obj-$(CONFIG_SATA_PHY)		+= sata_phy.o sata_exynos_phy.o
 
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
diff --git a/drivers/ata/sata_exynos_phy.c b/drivers/ata/sata_exynos_phy.c
new file mode 100644
index 0000000..f48fd2f
--- /dev/null
+++ b/drivers/ata/sata_exynos_phy.c
@@ -0,0 +1,265 @@
+/*
+ * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
+ *              http://www.samsung.com
+ *
+ * EXYNOS - SATA PHY controller driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/ahci_platform.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+
+#include <plat/cpu.h>
+
+#include <mach/irqs.h>
+#include <mach/map.h>
+#include <mach/regs-pmu.h>
+#include <mach/regs-sata.h>
+
+#include "sata_phy.h"
+
+#define	SATA_TIME_LIMIT		1000
+
+static struct i2c_client *i2c_client;
+
+static struct i2c_driver sataphy_i2c_driver;
+
+struct exynos_sata_phy {
+	void __iomem *mmio;
+	struct resource *mem;
+	struct clk *clk;
+	struct sata_phy phy;
+};
+
+static bool sata_is_reg(void __iomem *base, u32 reg, u32 checkbit, u32 status)
+{
+	if ((readl(base + reg) & checkbit) == status)
+		return true;
+	else
+		return false;
+}
+
+static bool wait_for_reg_status(void __iomem *base, u32 reg, u32 checkbit,
+				u32 status)
+{
+	unsigned long timeout = jiffies + usecs_to_jiffies(1000);
+
+	while (time_before(jiffies, timeout)) {
+		if (sata_is_reg(base, reg, checkbit, status))
+			return true;
+	}
+
+	return false;
+}
+
+static int sataphy_init(struct sata_phy *phy)
+{
+	int ret;
+	u32 val;
+
+	/* Values to be written to enable 40 bits interface */
+	u8 buf[] = { 0x3A, 0x0B };
+
+	struct exynos_sata_phy *sata_phy;
+
+	if (!i2c_client)
+		return -EPROBE_DEFER;
+
+	sata_phy = container_of(phy, struct exynos_sata_phy, phy);
+
+	ret = clk_enable(sata_phy->clk);
+	if (ret < 0) {
+		dev_err(phy->dev, "failed to enable source clk\n");
+		return ret;
+	}
+
+	if (sata_is_reg(sata_phy->mmio , EXYNOS5_SATA_CTRL0,
+		CTRL0_P0_PHY_CALIBRATED, CTRL0_P0_PHY_CALIBRATED))
+		return 0;
+
+	writel(S5P_PMU_SATA_PHY_CONTROL_EN, EXYNOS5_SATA_PHY_CONTROL);
+
+	val = 0;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val |= 0xFF;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val |= LINK_RESET;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val |= RESET_CMN_RST_N;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
+	val &= ~PHCTRLM_REF_RATE;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
+
+	/* High speed enable for Gen3 */
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
+	val |= PHCTRLM_HIGH_SPEED;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_CTRL0);
+	val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_CTRL0);
+
+	writel(0x2, sata_phy->mmio + EXYNOS5_SATA_MODE0);
+
+	ret = i2c_master_send(i2c_client, buf, sizeof(buf));
+	if (ret < 0)
+		return -ENXIO;
+
+	/* release cmu reset */
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val &= ~RESET_CMN_RST_N;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val |= RESET_CMN_RST_N;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	if (wait_for_reg_status(sata_phy->mmio, EXYNOS5_SATA_PHSATA_STATM,
+				PHSTATM_PLL_LOCKED, 1)) {
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static int sataphy_shutdown(struct sata_phy *phy)
+{
+
+	struct exynos_sata_phy *sata_phy;
+
+	sata_phy = container_of(phy, struct exynos_sata_phy, phy);
+	clk_disable(sata_phy->clk);
+
+	return 0;
+}
+
+static int __init sata_i2c_probe(struct i2c_client *client,
+			  const struct i2c_device_id *i2c_id)
+{
+	i2c_client = client;
+	return 0;
+}
+
+static int __init sata_phy_probe(struct platform_device *pdev)
+{
+	struct exynos_sata_phy *sataphy;
+	struct device *dev = &pdev->dev;
+	int ret = 0;
+
+	sataphy = devm_kzalloc(dev, sizeof(struct exynos_sata_phy), GFP_KERNEL);
+	if (!sataphy) {
+		dev_err(dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	sataphy->mmio = of_iomap(dev->of_node, 0);
+	if (!sataphy->mmio) {
+		dev_err(dev, "failed to remap IO\n");
+		return -EADDRNOTAVAIL;
+	}
+
+	sataphy->clk = devm_clk_get(dev, "sata-phy");
+	if (IS_ERR(sataphy->clk)) {
+		dev_err(dev, "failed to get clk for PHY\n");
+		ret = PTR_ERR(sataphy->clk);
+		goto err_iomap;
+	}
+
+	sataphy->phy.init = sataphy_init;
+	sataphy->phy.shutdown = sataphy_shutdown;
+	sataphy->phy.dev = dev;
+
+	ret = sata_add_phy(&sataphy->phy);
+	if (ret < 0) {
+		dev_err(dev, "PHY not registered with framework\n");
+		goto err_iomap;
+	}
+
+	ret = i2c_add_driver(&sataphy_i2c_driver);
+	if (ret < 0)
+		goto err_phy;
+
+	platform_set_drvdata(pdev, sataphy);
+
+	return ret;
+
+ err_phy:
+	sata_remove_phy(&sataphy->phy);
+
+ err_iomap:
+	iounmap(sataphy->mmio);
+
+	return ret;
+}
+
+static int sata_phy_remove(struct platform_device *pdev)
+{
+	struct exynos_sata_phy *sataphy;
+
+	sataphy = platform_get_drvdata(pdev);
+	iounmap(sataphy->mmio);
+	i2c_del_driver(&sataphy_i2c_driver);
+	sata_remove_phy(&sataphy->phy);
+
+	return 0;
+}
+
+static const struct of_device_id sata_phy_of_match[] = {
+	{ .compatible = "samsung,exynos5250-sata-phy", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, sata_phy_of_match);
+
+static const struct i2c_device_id phy_i2c_device_match[] = {
+	{ "sata-phy", 0 },
+};
+
+MODULE_DEVICE_TABLE(of, phy_i2c_device_match);
+
+static struct platform_driver sata_phy_driver = {
+	.probe = sata_phy_probe,
+	.remove = sata_phy_remove,
+	.driver = {
+		   .name = "sata-phy",
+		   .owner = THIS_MODULE,
+		   .of_match_table = sata_phy_of_match,
+	},
+};
+
+static struct i2c_driver sataphy_i2c_driver = {
+	.driver = {
+		   .name = "sata-phy-i2c",
+		   .owner = THIS_MODULE,
+		   .of_match_table = (void *)phy_i2c_device_match,
+	},
+	.probe = sata_i2c_probe,
+	.id_table = phy_i2c_device_match,
+};
+
+module_platform_driver(sata_phy_driver);
+
+MODULE_DESCRIPTION("EXYNOS SATA PHY DRIVER");
+MODULE_AUTHOR("Vasanth Ananthan, <vasanth.a@samsung.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

RE: [PATCH 2/2] driver: ata: add new Exynos5250 SATA PHY controller driver

[Kukjin Kim]

Vasanth Ananthan wrote:
> 
> Adding platform driver and I2C client driver for SATA PHY controller
> for Samsung Exynos5250.
> 
Well, I think, if required, you need to implement that via DT...

> The PHY controller in Exynos5250 has both the APB interface
> and I2C client interface, hence it requires both a platform driver
> and an I2C client driver. The PHY controller's primary charecteristics
> are handled through the APB interface, facilitated by the platform driver
> and the 40 bit interface should be enabled through the I2C interface,
> facilitated by the I2C client driver.
> 
> Further, this PHY controller driver uses PHY framework introduced by this
> patchset. The driver registers its initialization/shutdown call back to
the
> framework and corresponding port this PHY controller is assciated with
> gets this PHY and initializes it.
> 
> Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> ---
>  arch/arm/mach-exynos/include/mach/regs-pmu.h  |    3 +
>  arch/arm/mach-exynos/include/mach/regs-sata.h |   29 +++
>  drivers/ata/Makefile                          |    2 +-
>  drivers/ata/sata_exynos_phy.c                 |  265
> +++++++++++++++++++++++++
>  4 files changed, 298 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-exynos/include/mach/regs-sata.h

If this header file is used only for sata driver, this can be moved into
drivers/ata/.

>  create mode 100644 drivers/ata/sata_exynos_phy.c
> 
> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> index 3f30aa1..fd813d9 100644
> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> @@ -369,4 +369,7 @@
> 
>  #define EXYNOS5_OPTION_USE_RETENTION				(1 <<
> 4)
> 
> +/* Only for EXYNOS5250 */
> +#define EXYNOS5_SATA_PHY_CONTROL
> 	S5P_PMUREG(0x0724)

This should be handled into the driver. Please don't make a dependency with
SoC for your driver.

[...]

> -obj-$(CONFIG_SATA_PHY)		+= sata_phy.o
> +obj-$(CONFIG_SATA_PHY)		+= sata_phy.o sata_exynos_phy.o

Do you _really_ want to build the sata_exynos_phy.c under CONFIG_SATA_PHY? I
don't think so.

[...]

Thanks.

- Kukjin

Re: [PATCH 0/2] Adding SATA PHY framework and PHY controller driver

[Tomasz Figa]

Hi,

On Tuesday 29 of January 2013 20:49:16 Vasanth Ananthan wrote:
> Adding SATA PHY framework. The framework acts as an interface
> between the SATA controller and the PHY controller.
> 
> The framework can be used by any platform with device tree support.
> It primarily serves to de-couple the SATA controller driver and the
> PHY controller driver.
> 
> Vasanth Ananthan (2):
>   driver: ata: SATA PHY framework
>   driver: ata: add new Exynos5250 SATA PHY controller driver
> 
>  arch/arm/mach-exynos/include/mach/regs-pmu.h  |    3 +
>  arch/arm/mach-exynos/include/mach/regs-sata.h |   29 +++
>  drivers/ata/Kconfig                           |   10 +
>  drivers/ata/Makefile                          |    1 +
>  drivers/ata/sata_exynos_phy.c                 |  265
> +++++++++++++++++++++++++ drivers/ata/sata_phy.c                       
> |  145 ++++++++++++++ drivers/ata/sata_phy.h                        |  
> 29 +++
>  7 files changed, 482 insertions(+)
>  create mode 100644 arch/arm/mach-exynos/include/mach/regs-sata.h
>  create mode 100644 drivers/ata/sata_exynos_phy.c
>  create mode 100644 drivers/ata/sata_phy.c
>  create mode 100644 drivers/ata/sata_phy.h

It seems like you haven't addressed any of the comments received last time 
you posted this series...

Best regards,
Tomasz

Re: [PATCH 2/2] driver: ata: add new Exynos5250 SATA PHY controller driver

[Tomasz Figa]

Hi Vasanth,

Please see my comments inline. They are basically related to the same 
issues as I pointed in previous version of this patch.

On Tuesday 29 of January 2013 20:49:18 Vasanth Ananthan wrote:
> Adding platform driver and I2C client driver for SATA PHY controller
> for Samsung Exynos5250.
> 
> The PHY controller in Exynos5250 has both the APB interface
> and I2C client interface, hence it requires both a platform driver
> and an I2C client driver. The PHY controller's primary charecteristics
> are handled through the APB interface, facilitated by the platform
> driver and the 40 bit interface should be enabled through the I2C
> interface, facilitated by the I2C client driver.
> 
> Further, this PHY controller driver uses PHY framework introduced by
> this patchset. The driver registers its initialization/shutdown call
> back to the framework and corresponding port this PHY controller is
> assciated with gets this PHY and initializes it.
> 
> Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> ---
>  arch/arm/mach-exynos/include/mach/regs-pmu.h  |    3 +
>  arch/arm/mach-exynos/include/mach/regs-sata.h |   29 +++
>  drivers/ata/Makefile                          |    2 +-
>  drivers/ata/sata_exynos_phy.c                 |  265
> +++++++++++++++++++++++++ 4 files changed, 298 insertions(+), 1
> deletion(-)
>  create mode 100644 arch/arm/mach-exynos/include/mach/regs-sata.h
>  create mode 100644 drivers/ata/sata_exynos_phy.c
> 
> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 3f30aa1..fd813d9
> 100644
> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> @@ -369,4 +369,7 @@
> 
>  #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
> 
> +/* Only for EXYNOS5250 */
> +#define EXYNOS5_SATA_PHY_CONTROL				
S5P_PMUREG(0x0724)
> +
>  #endif /* __ASM_ARCH_REGS_PMU_H */
> diff --git a/arch/arm/mach-exynos/include/mach/regs-sata.h
> b/arch/arm/mach-exynos/include/mach/regs-sata.h new file mode 100644
> index 0000000..80dd564
> --- /dev/null
> +++ b/arch/arm/mach-exynos/include/mach/regs-sata.h
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
> + *              http://www.samsung.com
> + *
> + * EXYNOS - SATA PHY controller definition
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as +
> * published by the Free Software Foundation.
> +*/
> +
> +#define EXYNOS5_SATA_RESET		0x4
> +#define RESET_CMN_RST_N			(1 << 1)
> +#define LINK_RESET			0xF0000
> +
> +#define EXYNOS5_SATA_MODE0		0x10
> +
> +#define EXYNOS5_SATA_CTRL0		0x14
> +#define CTRL0_P0_PHY_CALIBRATED_SEL	(1 << 9)
> +#define CTRL0_P0_PHY_CALIBRATED		(1 << 8)
> +
> +#define EXYNOS5_SATA_PHSATA_CTRLM	0xE0
> +#define PHCTRLM_REF_RATE		(1 << 1)
> +#define PHCTRLM_HIGH_SPEED		(1 << 0)
> +
> +#define EXYNOS5_SATA_PHSATA_STATM	0xF0
> +#define PHSTATM_PLL_LOCKED		(1 << 0)
> +
> +#define SATA_PHY_CON_RESET              0xF003F
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 3d2d128..7add682 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -10,7 +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_PHY)		+= sata_phy.o
> +obj-$(CONFIG_SATA_PHY)		+= sata_phy.o sata_exynos_phy.o
> 
>  # SFF w/ custom DMA
>  obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
> diff --git a/drivers/ata/sata_exynos_phy.c
> b/drivers/ata/sata_exynos_phy.c new file mode 100644
> index 0000000..f48fd2f
> --- /dev/null
> +++ b/drivers/ata/sata_exynos_phy.c
> @@ -0,0 +1,265 @@
> +/*
> + * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
> + *              http://www.samsung.com
> + *
> + * EXYNOS - SATA PHY controller driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as +
> * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +
> +#include <plat/cpu.h>
> +
> +#include <mach/irqs.h>
> +#include <mach/map.h>
> +#include <mach/regs-pmu.h>
> +#include <mach/regs-sata.h>
> +
> +#include "sata_phy.h"
> +
> +#define	SATA_TIME_LIMIT		1000
> +
> +static struct i2c_client *i2c_client;
> +
> +static struct i2c_driver sataphy_i2c_driver;
> +
> +struct exynos_sata_phy {
> +	void __iomem *mmio;
> +	struct resource *mem;
> +	struct clk *clk;
> +	struct sata_phy phy;
> +};
> +
> +static bool sata_is_reg(void __iomem *base, u32 reg, u32 checkbit, u32
> status) +{
> +	if ((readl(base + reg) & checkbit) == status)
> +		return true;
> +	else
> +		return false;
> +}
> +
> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
> checkbit, +				u32 status)
> +{
> +	unsigned long timeout = jiffies + usecs_to_jiffies(1000);
> +
> +	while (time_before(jiffies, timeout)) {
> +		if (sata_is_reg(base, reg, checkbit, status))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int sataphy_init(struct sata_phy *phy)
> +{
> +	int ret;
> +	u32 val;
> +
> +	/* Values to be written to enable 40 bits interface */
> +	u8 buf[] = { 0x3A, 0x0B };
> +
> +	struct exynos_sata_phy *sata_phy;
> +
> +	if (!i2c_client)
> +		return -EPROBE_DEFER;

What probe are you deferring here? SATA PHY has been already probed if 
something can call this function.

> +
> +	sata_phy = container_of(phy, struct exynos_sata_phy, phy);
> +
> +	ret = clk_enable(sata_phy->clk);
> +	if (ret < 0) {
> +		dev_err(phy->dev, "failed to enable source clk\n");
> +		return ret;
> +	}
> +
> +	if (sata_is_reg(sata_phy->mmio , EXYNOS5_SATA_CTRL0,
> +		CTRL0_P0_PHY_CALIBRATED, CTRL0_P0_PHY_CALIBRATED))
> +		return 0;
> +
> +	writel(S5P_PMU_SATA_PHY_CONTROL_EN, EXYNOS5_SATA_PHY_CONTROL);
> +
> +	val = 0;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val |= 0xFF;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val |= LINK_RESET;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val |= RESET_CMN_RST_N;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> +	val &= ~PHCTRLM_REF_RATE;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> +
> +	/* High speed enable for Gen3 */
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> +	val |= PHCTRLM_HIGH_SPEED;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_CTRL0);
> +	val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_CTRL0);
> +
> +	writel(0x2, sata_phy->mmio + EXYNOS5_SATA_MODE0);
> +
> +	ret = i2c_master_send(i2c_client, buf, sizeof(buf));
> +	if (ret < 0)
> +		return -ENXIO;
> +
> +	/* release cmu reset */
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val &= ~RESET_CMN_RST_N;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val |= RESET_CMN_RST_N;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	if (wait_for_reg_status(sata_phy->mmio, EXYNOS5_SATA_PHSATA_STATM,
> +				PHSTATM_PLL_LOCKED, 1)) {
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int sataphy_shutdown(struct sata_phy *phy)
> +{
> +
> +	struct exynos_sata_phy *sata_phy;
> +
> +	sata_phy = container_of(phy, struct exynos_sata_phy, phy);
> +	clk_disable(sata_phy->clk);
> +
> +	return 0;
> +}
> +
> +static int __init sata_i2c_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *i2c_id)
> +{
> +	i2c_client = client;
> +	return 0;
> +}
> +
> +static int __init sata_phy_probe(struct platform_device *pdev)
> +{
> +	struct exynos_sata_phy *sataphy;
> +	struct device *dev = &pdev->dev;
> +	int ret = 0;
> +
> +	sataphy = devm_kzalloc(dev, sizeof(struct exynos_sata_phy),
> GFP_KERNEL); +	if (!sataphy) {
> +		dev_err(dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	sataphy->mmio = of_iomap(dev->of_node, 0);
> +	if (!sataphy->mmio) {
> +		dev_err(dev, "failed to remap IO\n");
> +		return -EADDRNOTAVAIL;
> +	}

Wouldn't it be better to use platform_get_resource and 
devm_request_and_ioremap here.

> +
> +	sataphy->clk = devm_clk_get(dev, "sata-phy");
> +	if (IS_ERR(sataphy->clk)) {
> +		dev_err(dev, "failed to get clk for PHY\n");
> +		ret = PTR_ERR(sataphy->clk);
> +		goto err_iomap;
> +	}
> +
> +	sataphy->phy.init = sataphy_init;
> +	sataphy->phy.shutdown = sataphy_shutdown;
> +	sataphy->phy.dev = dev;
> +
> +	ret = sata_add_phy(&sataphy->phy);
> +	if (ret < 0) {
> +		dev_err(dev, "PHY not registered with framework\n");
> +		goto err_iomap;
> +	}
> +
> +	ret = i2c_add_driver(&sataphy_i2c_driver);
> +	if (ret < 0)
> +		goto err_phy;

This is obviously incorrect, as I already pointed in previous version of 
this patch.

You are registering a SATA PHY, without making sure that the driver is 
fully initialized.

Some kind of solution would be to register both platform driver and i2c 
driver in module init function and defer probe of platform device until 
i2c device gets registered.

However this doesn't solve another problem. How can this driver support 
cases when there are multiple SATA PHYs?

> +
> +	platform_set_drvdata(pdev, sataphy);

This should be done before calling sata_add_phy. Basically calling 
sata_add_phy means that your driver is fully initialized and users can 
instantly call your callbacks safely.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

Re: [PATCH 2/2] driver: ata: add new Exynos5250 SATA PHY controller driver

[Vasanth Ananthan]

Hi Tomasz,

On Tue, Feb 5, 2013 at 7:53 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>
> Hi Vasanth,
>
> Please see my comments inline. They are basically related to the same
> issues as I pointed in previous version of this patch.
>
> On Tuesday 29 of January 2013 20:49:18 Vasanth Ananthan wrote:
> > Adding platform driver and I2C client driver for SATA PHY controller
> > for Samsung Exynos5250.
> >
> > The PHY controller in Exynos5250 has both the APB interface
> > and I2C client interface, hence it requires both a platform driver
> > and an I2C client driver. The PHY controller's primary charecteristics
> > are handled through the APB interface, facilitated by the platform
> > driver and the 40 bit interface should be enabled through the I2C
> > interface, facilitated by the I2C client driver.
> >
> > Further, this PHY controller driver uses PHY framework introduced by
> > this patchset. The driver registers its initialization/shutdown call
> > back to the framework and corresponding port this PHY controller is
> > assciated with gets this PHY and initializes it.
> >
> > Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> > ---
> >  arch/arm/mach-exynos/include/mach/regs-pmu.h  |    3 +
> >  arch/arm/mach-exynos/include/mach/regs-sata.h |   29 +++
> >  drivers/ata/Makefile                          |    2 +-
> >  drivers/ata/sata_exynos_phy.c                 |  265
> > +++++++++++++++++++++++++ 4 files changed, 298 insertions(+), 1
> > deletion(-)
> >  create mode 100644 arch/arm/mach-exynos/include/mach/regs-sata.h
> >  create mode 100644 drivers/ata/sata_exynos_phy.c
> >
> > diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> > b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 3f30aa1..fd813d9
> > 100644
> > --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> > +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> > @@ -369,4 +369,7 @@
> >
> >  #define EXYNOS5_OPTION_USE_RETENTION                         (1 << 4)
> >
> > +/* Only for EXYNOS5250 */
> > +#define EXYNOS5_SATA_PHY_CONTROL
> S5P_PMUREG(0x0724)
> > +
> >  #endif /* __ASM_ARCH_REGS_PMU_H */
> > diff --git a/arch/arm/mach-exynos/include/mach/regs-sata.h
> > b/arch/arm/mach-exynos/include/mach/regs-sata.h new file mode 100644
> > index 0000000..80dd564
> > --- /dev/null
> > +++ b/arch/arm/mach-exynos/include/mach/regs-sata.h
> > @@ -0,0 +1,29 @@
> > +/*
> > + * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
> > + *              http://www.samsung.com
> > + *
> > + * EXYNOS - SATA PHY controller definition
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as +
> > * published by the Free Software Foundation.
> > +*/
> > +
> > +#define EXYNOS5_SATA_RESET           0x4
> > +#define RESET_CMN_RST_N                      (1 << 1)
> > +#define LINK_RESET                   0xF0000
> > +
> > +#define EXYNOS5_SATA_MODE0           0x10
> > +
> > +#define EXYNOS5_SATA_CTRL0           0x14
> > +#define CTRL0_P0_PHY_CALIBRATED_SEL  (1 << 9)
> > +#define CTRL0_P0_PHY_CALIBRATED              (1 << 8)
> > +
> > +#define EXYNOS5_SATA_PHSATA_CTRLM    0xE0
> > +#define PHCTRLM_REF_RATE             (1 << 1)
> > +#define PHCTRLM_HIGH_SPEED           (1 << 0)
> > +
> > +#define EXYNOS5_SATA_PHSATA_STATM    0xF0
> > +#define PHSTATM_PLL_LOCKED           (1 << 0)
> > +
> > +#define SATA_PHY_CON_RESET              0xF003F
> > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> > index 3d2d128..7add682 100644
> > --- a/drivers/ata/Makefile
> > +++ b/drivers/ata/Makefile
> > @@ -10,7 +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_PHY)               += sata_phy.o
> > +obj-$(CONFIG_SATA_PHY)               += sata_phy.o sata_exynos_phy.o
> >
> >  # SFF w/ custom DMA
> >  obj-$(CONFIG_PDC_ADMA)               += pdc_adma.o
> > diff --git a/drivers/ata/sata_exynos_phy.c
> > b/drivers/ata/sata_exynos_phy.c new file mode 100644
> > index 0000000..f48fd2f
> > --- /dev/null
> > +++ b/drivers/ata/sata_exynos_phy.c
> > @@ -0,0 +1,265 @@
> > +/*
> > + * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
> > + *              http://www.samsung.com
> > + *
> > + * EXYNOS - SATA PHY controller driver
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as +
> > * published by the Free Software Foundation.
> > +*/
> > +
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/ahci_platform.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/list.h>
> > +#include <linux/io.h>
> > +#include <linux/of_address.h>
> > +
> > +#include <plat/cpu.h>
> > +
> > +#include <mach/irqs.h>
> > +#include <mach/map.h>
> > +#include <mach/regs-pmu.h>
> > +#include <mach/regs-sata.h>
> > +
> > +#include "sata_phy.h"
> > +
> > +#define      SATA_TIME_LIMIT         1000
> > +
> > +static struct i2c_client *i2c_client;
> > +
> > +static struct i2c_driver sataphy_i2c_driver;
> > +
> > +struct exynos_sata_phy {
> > +     void __iomem *mmio;
> > +     struct resource *mem;
> > +     struct clk *clk;
> > +     struct sata_phy phy;
> > +};
> > +
> > +static bool sata_is_reg(void __iomem *base, u32 reg, u32 checkbit, u32
> > status) +{
> > +     if ((readl(base + reg) & checkbit) == status)
> > +             return true;
> > +     else
> > +             return false;
> > +}
> > +
> > +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
> > checkbit, +                           u32 status)
> > +{
> > +     unsigned long timeout = jiffies + usecs_to_jiffies(1000);
> > +
> > +     while (time_before(jiffies, timeout)) {
> > +             if (sata_is_reg(base, reg, checkbit, status))
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static int sataphy_init(struct sata_phy *phy)
> > +{
> > +     int ret;
> > +     u32 val;
> > +
> > +     /* Values to be written to enable 40 bits interface */
> > +     u8 buf[] = { 0x3A, 0x0B };
> > +
> > +     struct exynos_sata_phy *sata_phy;
> > +
> > +     if (!i2c_client)
> > +             return -EPROBE_DEFER;
>
> What probe are you deferring here? SATA PHY has been already probed if
> something can call this function.

This would be called from the SATA controller driver via the
framework, the return value
would propagate to the SATA ctrl driver probe, and defer it, if the
i2c client is not initialized.

>
>
> > +
> > +     sata_phy = container_of(phy, struct exynos_sata_phy, phy);
> > +
> > +     ret = clk_enable(sata_phy->clk);
> > +     if (ret < 0) {
> > +             dev_err(phy->dev, "failed to enable source clk\n");
> > +             return ret;
> > +     }
> > +
> > +     if (sata_is_reg(sata_phy->mmio , EXYNOS5_SATA_CTRL0,
> > +             CTRL0_P0_PHY_CALIBRATED, CTRL0_P0_PHY_CALIBRATED))
> > +             return 0;
> > +
> > +     writel(S5P_PMU_SATA_PHY_CONTROL_EN, EXYNOS5_SATA_PHY_CONTROL);
> > +
> > +     val = 0;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +
> > +     val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +     val |= 0xFF;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +
> > +     val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +     val |= LINK_RESET;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +
> > +     val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +     val |= RESET_CMN_RST_N;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +
> > +     val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> > +     val &= ~PHCTRLM_REF_RATE;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> > +
> > +     /* High speed enable for Gen3 */
> > +     val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> > +     val |= PHCTRLM_HIGH_SPEED;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> > +
> > +     val = readl(sata_phy->mmio + EXYNOS5_SATA_CTRL0);
> > +     val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_CTRL0);
> > +
> > +     writel(0x2, sata_phy->mmio + EXYNOS5_SATA_MODE0);
> > +
> > +     ret = i2c_master_send(i2c_client, buf, sizeof(buf));
> > +     if (ret < 0)
> > +             return -ENXIO;
> > +
> > +     /* release cmu reset */
> > +     val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +     val &= ~RESET_CMN_RST_N;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +
> > +     val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +     val |= RESET_CMN_RST_N;
> > +     writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> > +
> > +     if (wait_for_reg_status(sata_phy->mmio, EXYNOS5_SATA_PHSATA_STATM,
> > +                             PHSTATM_PLL_LOCKED, 1)) {
> > +             return 0;
> > +     }
> > +     return -EINVAL;
> > +}
> > +
> > +static int sataphy_shutdown(struct sata_phy *phy)
> > +{
> > +
> > +     struct exynos_sata_phy *sata_phy;
> > +
> > +     sata_phy = container_of(phy, struct exynos_sata_phy, phy);
> > +     clk_disable(sata_phy->clk);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __init sata_i2c_probe(struct i2c_client *client,
> > +                       const struct i2c_device_id *i2c_id)
> > +{
> > +     i2c_client = client;
> > +     return 0;
> > +}
> > +
> > +static int __init sata_phy_probe(struct platform_device *pdev)
> > +{
> > +     struct exynos_sata_phy *sataphy;
> > +     struct device *dev = &pdev->dev;
> > +     int ret = 0;
> > +
> > +     sataphy = devm_kzalloc(dev, sizeof(struct exynos_sata_phy),
> > GFP_KERNEL); +        if (!sataphy) {
> > +             dev_err(dev, "failed to allocate memory\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     sataphy->mmio = of_iomap(dev->of_node, 0);
> > +     if (!sataphy->mmio) {
> > +             dev_err(dev, "failed to remap IO\n");
> > +             return -EADDRNOTAVAIL;
> > +     }
>
> Wouldn't it be better to use platform_get_resource and
> devm_request_and_ioremap here.

Since this driver is entirely DT based I thought of_iomap would be
more appropriate.Whats your view?

>
> > +
> > +     sataphy->clk = devm_clk_get(dev, "sata-phy");
> > +     if (IS_ERR(sataphy->clk)) {
> > +             dev_err(dev, "failed to get clk for PHY\n");
> > +             ret = PTR_ERR(sataphy->clk);
> > +             goto err_iomap;
> > +     }
> > +
> > +     sataphy->phy.init = sataphy_init;
> > +     sataphy->phy.shutdown = sataphy_shutdown;
> > +     sataphy->phy.dev = dev;
> > +
> > +     ret = sata_add_phy(&sataphy->phy);
> > +     if (ret < 0) {
> > +             dev_err(dev, "PHY not registered with framework\n");
> > +             goto err_iomap;
> > +     }
> > +
> > +     ret = i2c_add_driver(&sataphy_i2c_driver);
> > +     if (ret < 0)
> > +             goto err_phy;
>
> This is obviously incorrect, as I already pointed in previous version of
> this patch.
>
> You are registering a SATA PHY, without making sure that the driver is
> fully initialized.
>
> Some kind of solution would be to register both platform driver and i2c
> driver in module init function and defer probe of platform device until
> i2c device gets registered.
>

Though the driver is not fully initialized, in callback, I am checking
whether the
i2c client is initialized and returning -EPROBEDEFER if otherwise. In
such cases,
the SATA driver probe is deferred and by the time its probed again,
the PHY initialization
would have completed, if not the PHY would never be initialized and
the SATA controller
abandons this PHY.

What do you suggest?


> However this doesn't solve another problem. How can this driver support
> cases when there are multiple SATA PHYs?
>
> > +
> > +     platform_set_drvdata(pdev, sataphy);
>
> This should be done before calling sata_add_phy. Basically calling
> sata_add_phy means that your driver is fully initialized and users can
> instantly call your callbacks safely.
>
> Best regards,
> --
> Tomasz Figa
> Samsung Poland R&D Center
> SW Solution Development, Linux Platform
>
> --
> 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




--
Regards,

Vasanth K A