* [PATCH V6 0/2] Exynos5250 SATA Support
@ 2014-01-10 7:00 ` Yuvaraj Kumar C D
0 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar C D @ 2014-01-10 7:00 UTC (permalink / raw)
To: kishon, kgene.kim, linux-kernel, linux-arm-kernel, devicetree, linux-doc
Cc: mark.rutland, jg1.han, b.zolnierkie, joshi, t.figa, swarren,
grant.likely, christoffer.dall, linux-ide, Yuvaraj Kumar C D
This patch series enable the SATA support on Exynos5250 based boards.
It incorporates the generic phy framework to deal with sata phy.
This patch depends on the below patches
[1]. drivers: phy: add generic PHY framework
by Kishon Vijay Abraham I<kishon@ti.com>
[2]. ata: ahci_platform: Manage SATA PHY
by Roger Quadros <rogerq@ti.com>
[3].ARM: dts: Add pmu sysreg node to exynos5250 and
exynos5420 dtsi files
by Leela Krishna Amudala <l.krishna@samsung.com>
[4]. i2c: s3c2410 : Add polling mode support
by Vasanth Ananthan <vasanth.a@samsung.com>
Changes from V5:
1.Rebased on latest generic PHY framework for-next tree.
2.Minor nits such as indentations.
Changes from V4:
1.Made Exynos5250 SATA PHY driver by default selects
CONFIG_I2C and CONFIG_I2C_S3C2410, as SATA PHY driver
depends on I2C.
2.Used the new phandle "sata_phy_i2c" in the DT entry.
3.struct i2c_driver sataphy_i2c_driver made static which
was earlier global type.
Changes from V3:
1.Moved the devicetree binding documentation
from /bindings/ata/ to bindings/phy/ .
2.Moved devm_phy_create call before to
the devm_phy_provider_register.
Changes from V2:
1.Removed of_match_table
2.Moved to syscon interface for PMU handling.
Changes from V1:
1. Dropped the patch
ahci: exynos: add ahci sata support on Exynos platform
2.Adapt to latest generic PHY framework available in
git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next
Yuvaraj Kumar C D (2):
PHY: Exynos: Add Exynos5250 SATA PHY driver
ARM: dts: Enable ahci sata and sata phy
.../devicetree/bindings/ata/exynos-sata-phy.txt | 14 --
.../devicetree/bindings/ata/exynos-sata.txt | 25 +-
.../devicetree/bindings/phy/samsung-phy.txt | 36 +++
arch/arm/boot/dts/exynos5250-arndale.dts | 11 +
arch/arm/boot/dts/exynos5250-smdk5250.dts | 8 +-
arch/arm/boot/dts/exynos5250.dtsi | 17 +-
drivers/phy/Kconfig | 13 ++
drivers/phy/Makefile | 1 +
drivers/phy/phy-exynos5250-sata-i2c.c | 40 ++++
drivers/phy/phy-exynos5250-sata.c | 238 ++++++++++++++++++++
10 files changed, 370 insertions(+), 33 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/ata/exynos-sata-phy.txt
create mode 100644 drivers/phy/phy-exynos5250-sata-i2c.c
create mode 100644 drivers/phy/phy-exynos5250-sata.c
--
1.7.9.5
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V6 0/2] Exynos5250 SATA Support
@ 2014-01-10 7:00 ` Yuvaraj Kumar C D
0 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar C D @ 2014-01-10 7:00 UTC (permalink / raw)
To: linux-arm-kernel
This patch series enable the SATA support on Exynos5250 based boards.
It incorporates the generic phy framework to deal with sata phy.
This patch depends on the below patches
[1]. drivers: phy: add generic PHY framework
by Kishon Vijay Abraham I<kishon@ti.com>
[2]. ata: ahci_platform: Manage SATA PHY
by Roger Quadros <rogerq@ti.com>
[3].ARM: dts: Add pmu sysreg node to exynos5250 and
exynos5420 dtsi files
by Leela Krishna Amudala <l.krishna@samsung.com>
[4]. i2c: s3c2410 : Add polling mode support
by Vasanth Ananthan <vasanth.a@samsung.com>
Changes from V5:
1.Rebased on latest generic PHY framework for-next tree.
2.Minor nits such as indentations.
Changes from V4:
1.Made Exynos5250 SATA PHY driver by default selects
CONFIG_I2C and CONFIG_I2C_S3C2410, as SATA PHY driver
depends on I2C.
2.Used the new phandle "sata_phy_i2c" in the DT entry.
3.struct i2c_driver sataphy_i2c_driver made static which
was earlier global type.
Changes from V3:
1.Moved the devicetree binding documentation
from /bindings/ata/ to bindings/phy/ .
2.Moved devm_phy_create call before to
the devm_phy_provider_register.
Changes from V2:
1.Removed of_match_table
2.Moved to syscon interface for PMU handling.
Changes from V1:
1. Dropped the patch
ahci: exynos: add ahci sata support on Exynos platform
2.Adapt to latest generic PHY framework available in
git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next
Yuvaraj Kumar C D (2):
PHY: Exynos: Add Exynos5250 SATA PHY driver
ARM: dts: Enable ahci sata and sata phy
.../devicetree/bindings/ata/exynos-sata-phy.txt | 14 --
.../devicetree/bindings/ata/exynos-sata.txt | 25 +-
.../devicetree/bindings/phy/samsung-phy.txt | 36 +++
arch/arm/boot/dts/exynos5250-arndale.dts | 11 +
arch/arm/boot/dts/exynos5250-smdk5250.dts | 8 +-
arch/arm/boot/dts/exynos5250.dtsi | 17 +-
drivers/phy/Kconfig | 13 ++
drivers/phy/Makefile | 1 +
drivers/phy/phy-exynos5250-sata-i2c.c | 40 ++++
drivers/phy/phy-exynos5250-sata.c | 238 ++++++++++++++++++++
10 files changed, 370 insertions(+), 33 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/ata/exynos-sata-phy.txt
create mode 100644 drivers/phy/phy-exynos5250-sata-i2c.c
create mode 100644 drivers/phy/phy-exynos5250-sata.c
--
1.7.9.5
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
2014-01-10 7:00 ` Yuvaraj Kumar C D
@ 2014-01-10 7:00 ` Yuvaraj Kumar C D
-1 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar C D @ 2014-01-10 7:00 UTC (permalink / raw)
To: kishon, kgene.kim, linux-kernel, linux-arm-kernel, devicetree, linux-doc
Cc: mark.rutland, jg1.han, b.zolnierkie, joshi, t.figa, swarren,
grant.likely, christoffer.dall, linux-ide, Yuvaraj Kumar C D,
Girish K S, Vasanth Ananthan
This patch adds the SATA PHY driver for Exynos5250.Exynos5250 SATA
PHY comprises of CMU and TRSV blocks which are of I2C register Map.
So this patch also adds a i2c client driver, which is used configure
the CMU and TRSV block of exynos5250 SATA PHY.
This patch incorporates the generic PHY framework to deal with SATA
PHY.
This patch depends on the below patches
[1].drivers: phy: add generic PHY framework
by Kishon Vijay Abraham I<kishon@ti.com>
[2].ata: ahci_platform: Manage SATA PHY
by Roger Quadros <rogerq@ti.com>
Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
Signed-off-by: Girish K S <ks.giri@samsung.com>
Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
---
Changes from V5:
1.Rebased on latest generic PHY framework for-next tree.
2.Minor nits such as indentations.
Changes from V4:
1.Made Exynos5250 SATA PHY driver by default selects
CONFIG_I2C and CONFIG_I2C_S3C2410, as SATA PHY driver
depends on I2C.
2.struct i2c_driver sataphy_i2c_driver made static which
was earlier global type.
3.Renamed the files to phy-exynos5250-sata.c and
phy-exynos5250-sata-i2c.c and CONFIG_EXYNOS5250_SATA_PHY
to CONFIG_PHY_EXYNOS5250_SATA.
Changes from V3:
1.Moved devm_phy_create before to devm_phy_provider_register.
Changes from V2:
1.Removed of_match_table
2.Moved to syscon interface for PMU handling.
Changes from V1:
1.Adapted to latest version of Generic PHY framework
2.Removed exynos_sata_i2c_remove function.
drivers/phy/Kconfig | 13 ++
drivers/phy/Makefile | 1 +
drivers/phy/phy-exynos5250-sata-i2c.c | 40 ++++++
drivers/phy/phy-exynos5250-sata.c | 238 +++++++++++++++++++++++++++++++++
4 files changed, 292 insertions(+)
create mode 100644 drivers/phy/phy-exynos5250-sata-i2c.c
create mode 100644 drivers/phy/phy-exynos5250-sata.c
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index d0611b8..df79150 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -57,4 +57,17 @@ config PHY_EXYNOS_DP_VIDEO
help
Support for Display Port PHY found on Samsung EXYNOS SoCs.
+config PHY_EXYNOS5250_SATA
+ tristate "Exynos5250 Sata SerDes/PHY driver"
+ depends on SOC_EXYNOS5250
+ select GENERIC_PHY
+ select I2C
+ select I2C_S3C2410
+ select MFD_SYSCON
+ help
+ Enable this to support SATA SerDes/Phy found on Samsung's
+ Exynos5250 based SoCs.This SerDes/Phy supports SATA 1.5 Gb/s,
+ SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds. It supports one SATA host
+ port to accept one SATA device.
+
endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 4e4adc9..8ccf8c3 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o
obj-$(CONFIG_PHY_MVEBU_SATA) += phy-mvebu-sata.o
obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o
obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
+obj-$(CONFIG_PHY_EXYNOS5250_SATA) += phy-exynos5250-sata.o phy-exynos5250-sata-i2c.o
diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c b/drivers/phy/phy-exynos5250-sata-i2c.c
new file mode 100644
index 0000000..206e337
--- /dev/null
+++ b/drivers/phy/phy-exynos5250-sata-i2c.c
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co.Ltd
+ * Author:
+ * Yuvaraj C D <yuvaraj.cd@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+static int exynos_sata_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *i2c_id)
+{
+ return 0;
+}
+
+static const struct i2c_device_id sataphy_i2c_device_match[] = {
+ { "exynos-sataphy-i2c", 0 },
+};
+
+static struct i2c_driver sataphy_i2c_driver = {
+ .probe = exynos_sata_i2c_probe,
+ .id_table = sataphy_i2c_device_match,
+ .driver = {
+ .name = "exynos-sataphy-i2c",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init exynos5250_phy_i2c_init(void)
+{
+ return i2c_add_driver(&sataphy_i2c_driver);
+}
+module_init(exynos5250_phy_i2c_init);
diff --git a/drivers/phy/phy-exynos5250-sata.c b/drivers/phy/phy-exynos5250-sata.c
new file mode 100644
index 0000000..6e5ff8d
--- /dev/null
+++ b/drivers/phy/phy-exynos5250-sata.c
@@ -0,0 +1,238 @@
+/*
+ * Samsung SATA SerDes(PHY) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Authors: Girish K S <ks.giri@samsung.com>
+ * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
+ *
+ * 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/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+#include <linux/mfd/syscon.h>
+
+#define EXYNOS5_SATA_RESET 0x4
+#define RESET_CMN_RST_N (1 << 1)
+#define LINK_RESET 0xF0000
+#define EXYNOS5_SATA_MODE0 0x10
+#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
+#define SATA_SPD_GEN3 (2 << 0)
+#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 EXYNOS_SATA_PHY_EN (1 << 0)
+#define SATAPHY_CONTROL_OFFSET 0x0724
+
+struct exynos_sata_phy {
+ struct phy *phy;
+ struct clk *phyclk;
+ void __iomem *regs;
+ void __iomem *pmureg;
+ struct i2c_client *client;
+};
+
+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 ((readl(base + reg) & checkbit) == status)
+ return true;
+ }
+
+ return false;
+}
+
+static int exynos_sata_phy_power_on(struct phy *phy)
+{
+ struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
+
+ regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
+ EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
+
+ return 0;
+}
+
+static int exynos_sata_phy_power_off(struct phy *phy)
+{
+ struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
+
+ regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
+ EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
+
+ return 0;
+}
+
+static int exynos_sata_phy_init(struct phy *phy)
+{
+ u32 val = 0;
+ int ret = 0;
+ u8 buf[] = { 0x3A, 0x0B };
+ struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
+
+ regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
+ EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
+
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val |= 0xFF;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val |= LINK_RESET;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val |= RESET_CMN_RST_N;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
+ val &= ~PHCTRLM_REF_RATE;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
+
+ /* High speed enable for Gen3 */
+ val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
+ val |= PHCTRLM_HIGH_SPEED;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
+ val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
+ val |= SATA_SPD_GEN3;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
+
+ ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
+ if (ret < 0)
+ return -ENXIO;
+
+ /* release cmu reset */
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val &= ~RESET_CMN_RST_N;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val |= RESET_CMN_RST_N;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ return (wait_for_reg_status(sata_phy->regs, EXYNOS5_SATA_PHSATA_STATM,
+ PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
+
+}
+
+static struct phy_ops exynos_sata_phy_ops = {
+ .init = exynos_sata_phy_init,
+ .power_on = exynos_sata_phy_power_on,
+ .power_off = exynos_sata_phy_power_off,
+ .owner = THIS_MODULE,
+};
+
+static int exynos_sata_phy_probe(struct platform_device *pdev)
+{
+ struct exynos_sata_phy *sata_phy;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct phy_provider *phy_provider;
+ struct device_node *node;
+ int ret = 0;
+
+ sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
+ if (!sata_phy)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ sata_phy->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(sata_phy->regs))
+ return PTR_ERR(sata_phy->regs);
+
+ sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
+ "samsung,syscon-phandle");
+ if (!sata_phy->pmureg) {
+ dev_err(dev, "syscon regmap lookup failed.\n");
+ return PTR_ERR(sata_phy->pmureg);
+ }
+
+ node = of_parse_phandle(dev->of_node,
+ "samsung,exynos-sataphy-i2c-phandle", 0);
+ if (!node)
+ return -ENODEV;
+
+ sata_phy->client = of_find_i2c_device_by_node(node);
+ if (!sata_phy->client)
+ return -EPROBE_DEFER;
+
+ dev_set_drvdata(dev, sata_phy);
+
+ sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
+ if (IS_ERR(sata_phy->phyclk)) {
+ dev_err(dev, "failed to get clk for PHY\n");
+ return PTR_ERR(sata_phy->phyclk);
+ }
+
+ ret = clk_prepare_enable(sata_phy->phyclk);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable source clk\n");
+ return ret;
+ }
+
+ sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
+ if (IS_ERR(sata_phy->phy)) {
+ clk_disable_unprepare(sata_phy->phyclk);
+ dev_err(dev, "failed to create PHY\n");
+ return PTR_ERR(sata_phy->phy);
+ }
+
+ phy_set_drvdata(sata_phy->phy, sata_phy);
+
+ phy_provider = devm_of_phy_provider_register(dev,
+ of_phy_simple_xlate);
+ if (IS_ERR(phy_provider)) {
+ clk_disable_unprepare(sata_phy->phyclk);
+ return PTR_ERR(phy_provider);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id exynos_sata_phy_of_match[] = {
+ { .compatible = "samsung,exynos5250-sata-phy" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
+
+static struct platform_driver exynos_sata_phy_driver = {
+ .probe = exynos_sata_phy_probe,
+ .driver = {
+ .of_match_table = exynos_sata_phy_of_match,
+ .name = "samsung,sata-phy",
+ .owner = THIS_MODULE,
+ }
+};
+module_platform_driver(exynos_sata_phy_driver);
+
+MODULE_DESCRIPTION("Samsung SerDes PHY driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Girish K S <ks.giri@samsung.com>");
+MODULE_AUTHOR("Yuvaraj C D <yuvaraj.cd@samsung.com>");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
@ 2014-01-10 7:00 ` Yuvaraj Kumar C D
0 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar C D @ 2014-01-10 7:00 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds the SATA PHY driver for Exynos5250.Exynos5250 SATA
PHY comprises of CMU and TRSV blocks which are of I2C register Map.
So this patch also adds a i2c client driver, which is used configure
the CMU and TRSV block of exynos5250 SATA PHY.
This patch incorporates the generic PHY framework to deal with SATA
PHY.
This patch depends on the below patches
[1].drivers: phy: add generic PHY framework
by Kishon Vijay Abraham I<kishon@ti.com>
[2].ata: ahci_platform: Manage SATA PHY
by Roger Quadros <rogerq@ti.com>
Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
Signed-off-by: Girish K S <ks.giri@samsung.com>
Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
---
Changes from V5:
1.Rebased on latest generic PHY framework for-next tree.
2.Minor nits such as indentations.
Changes from V4:
1.Made Exynos5250 SATA PHY driver by default selects
CONFIG_I2C and CONFIG_I2C_S3C2410, as SATA PHY driver
depends on I2C.
2.struct i2c_driver sataphy_i2c_driver made static which
was earlier global type.
3.Renamed the files to phy-exynos5250-sata.c and
phy-exynos5250-sata-i2c.c and CONFIG_EXYNOS5250_SATA_PHY
to CONFIG_PHY_EXYNOS5250_SATA.
Changes from V3:
1.Moved devm_phy_create before to devm_phy_provider_register.
Changes from V2:
1.Removed of_match_table
2.Moved to syscon interface for PMU handling.
Changes from V1:
1.Adapted to latest version of Generic PHY framework
2.Removed exynos_sata_i2c_remove function.
drivers/phy/Kconfig | 13 ++
drivers/phy/Makefile | 1 +
drivers/phy/phy-exynos5250-sata-i2c.c | 40 ++++++
drivers/phy/phy-exynos5250-sata.c | 238 +++++++++++++++++++++++++++++++++
4 files changed, 292 insertions(+)
create mode 100644 drivers/phy/phy-exynos5250-sata-i2c.c
create mode 100644 drivers/phy/phy-exynos5250-sata.c
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index d0611b8..df79150 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -57,4 +57,17 @@ config PHY_EXYNOS_DP_VIDEO
help
Support for Display Port PHY found on Samsung EXYNOS SoCs.
+config PHY_EXYNOS5250_SATA
+ tristate "Exynos5250 Sata SerDes/PHY driver"
+ depends on SOC_EXYNOS5250
+ select GENERIC_PHY
+ select I2C
+ select I2C_S3C2410
+ select MFD_SYSCON
+ help
+ Enable this to support SATA SerDes/Phy found on Samsung's
+ Exynos5250 based SoCs.This SerDes/Phy supports SATA 1.5 Gb/s,
+ SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds. It supports one SATA host
+ port to accept one SATA device.
+
endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 4e4adc9..8ccf8c3 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o
obj-$(CONFIG_PHY_MVEBU_SATA) += phy-mvebu-sata.o
obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o
obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
+obj-$(CONFIG_PHY_EXYNOS5250_SATA) += phy-exynos5250-sata.o phy-exynos5250-sata-i2c.o
diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c b/drivers/phy/phy-exynos5250-sata-i2c.c
new file mode 100644
index 0000000..206e337
--- /dev/null
+++ b/drivers/phy/phy-exynos5250-sata-i2c.c
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co.Ltd
+ * Author:
+ * Yuvaraj C D <yuvaraj.cd@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+static int exynos_sata_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *i2c_id)
+{
+ return 0;
+}
+
+static const struct i2c_device_id sataphy_i2c_device_match[] = {
+ { "exynos-sataphy-i2c", 0 },
+};
+
+static struct i2c_driver sataphy_i2c_driver = {
+ .probe = exynos_sata_i2c_probe,
+ .id_table = sataphy_i2c_device_match,
+ .driver = {
+ .name = "exynos-sataphy-i2c",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init exynos5250_phy_i2c_init(void)
+{
+ return i2c_add_driver(&sataphy_i2c_driver);
+}
+module_init(exynos5250_phy_i2c_init);
diff --git a/drivers/phy/phy-exynos5250-sata.c b/drivers/phy/phy-exynos5250-sata.c
new file mode 100644
index 0000000..6e5ff8d
--- /dev/null
+++ b/drivers/phy/phy-exynos5250-sata.c
@@ -0,0 +1,238 @@
+/*
+ * Samsung SATA SerDes(PHY) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Authors: Girish K S <ks.giri@samsung.com>
+ * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
+ *
+ * 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/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+#include <linux/mfd/syscon.h>
+
+#define EXYNOS5_SATA_RESET 0x4
+#define RESET_CMN_RST_N (1 << 1)
+#define LINK_RESET 0xF0000
+#define EXYNOS5_SATA_MODE0 0x10
+#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
+#define SATA_SPD_GEN3 (2 << 0)
+#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 EXYNOS_SATA_PHY_EN (1 << 0)
+#define SATAPHY_CONTROL_OFFSET 0x0724
+
+struct exynos_sata_phy {
+ struct phy *phy;
+ struct clk *phyclk;
+ void __iomem *regs;
+ void __iomem *pmureg;
+ struct i2c_client *client;
+};
+
+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 ((readl(base + reg) & checkbit) == status)
+ return true;
+ }
+
+ return false;
+}
+
+static int exynos_sata_phy_power_on(struct phy *phy)
+{
+ struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
+
+ regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
+ EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
+
+ return 0;
+}
+
+static int exynos_sata_phy_power_off(struct phy *phy)
+{
+ struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
+
+ regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
+ EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
+
+ return 0;
+}
+
+static int exynos_sata_phy_init(struct phy *phy)
+{
+ u32 val = 0;
+ int ret = 0;
+ u8 buf[] = { 0x3A, 0x0B };
+ struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
+
+ regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
+ EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
+
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val |= 0xFF;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val |= LINK_RESET;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val |= RESET_CMN_RST_N;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
+ val &= ~PHCTRLM_REF_RATE;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
+
+ /* High speed enable for Gen3 */
+ val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
+ val |= PHCTRLM_HIGH_SPEED;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
+ val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
+ val |= SATA_SPD_GEN3;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
+
+ ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
+ if (ret < 0)
+ return -ENXIO;
+
+ /* release cmu reset */
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val &= ~RESET_CMN_RST_N;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
+ val |= RESET_CMN_RST_N;
+ writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
+
+ return (wait_for_reg_status(sata_phy->regs, EXYNOS5_SATA_PHSATA_STATM,
+ PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
+
+}
+
+static struct phy_ops exynos_sata_phy_ops = {
+ .init = exynos_sata_phy_init,
+ .power_on = exynos_sata_phy_power_on,
+ .power_off = exynos_sata_phy_power_off,
+ .owner = THIS_MODULE,
+};
+
+static int exynos_sata_phy_probe(struct platform_device *pdev)
+{
+ struct exynos_sata_phy *sata_phy;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct phy_provider *phy_provider;
+ struct device_node *node;
+ int ret = 0;
+
+ sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
+ if (!sata_phy)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ sata_phy->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(sata_phy->regs))
+ return PTR_ERR(sata_phy->regs);
+
+ sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
+ "samsung,syscon-phandle");
+ if (!sata_phy->pmureg) {
+ dev_err(dev, "syscon regmap lookup failed.\n");
+ return PTR_ERR(sata_phy->pmureg);
+ }
+
+ node = of_parse_phandle(dev->of_node,
+ "samsung,exynos-sataphy-i2c-phandle", 0);
+ if (!node)
+ return -ENODEV;
+
+ sata_phy->client = of_find_i2c_device_by_node(node);
+ if (!sata_phy->client)
+ return -EPROBE_DEFER;
+
+ dev_set_drvdata(dev, sata_phy);
+
+ sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
+ if (IS_ERR(sata_phy->phyclk)) {
+ dev_err(dev, "failed to get clk for PHY\n");
+ return PTR_ERR(sata_phy->phyclk);
+ }
+
+ ret = clk_prepare_enable(sata_phy->phyclk);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable source clk\n");
+ return ret;
+ }
+
+ sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
+ if (IS_ERR(sata_phy->phy)) {
+ clk_disable_unprepare(sata_phy->phyclk);
+ dev_err(dev, "failed to create PHY\n");
+ return PTR_ERR(sata_phy->phy);
+ }
+
+ phy_set_drvdata(sata_phy->phy, sata_phy);
+
+ phy_provider = devm_of_phy_provider_register(dev,
+ of_phy_simple_xlate);
+ if (IS_ERR(phy_provider)) {
+ clk_disable_unprepare(sata_phy->phyclk);
+ return PTR_ERR(phy_provider);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id exynos_sata_phy_of_match[] = {
+ { .compatible = "samsung,exynos5250-sata-phy" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
+
+static struct platform_driver exynos_sata_phy_driver = {
+ .probe = exynos_sata_phy_probe,
+ .driver = {
+ .of_match_table = exynos_sata_phy_of_match,
+ .name = "samsung,sata-phy",
+ .owner = THIS_MODULE,
+ }
+};
+module_platform_driver(exynos_sata_phy_driver);
+
+MODULE_DESCRIPTION("Samsung SerDes PHY driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Girish K S <ks.giri@samsung.com>");
+MODULE_AUTHOR("Yuvaraj C D <yuvaraj.cd@samsung.com>");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V6 2/2] ARM: dts: Enable ahci sata and sata phy
2014-01-10 7:00 ` Yuvaraj Kumar C D
@ 2014-01-10 7:00 ` Yuvaraj Kumar C D
-1 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar C D @ 2014-01-10 7:00 UTC (permalink / raw)
To: kishon, kgene.kim, linux-kernel, linux-arm-kernel, devicetree, linux-doc
Cc: mark.rutland, jg1.han, b.zolnierkie, joshi, t.figa, swarren,
grant.likely, christoffer.dall, linux-ide, Yuvaraj Kumar C D
This patch adds dt entry for ahci sata controller and its
corresponding phy controller.phy node has been added w.r.t
new generic phy framework.
Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
---
Changes since V4:
1.Used the new phandle "sata_phy_i2c" in the DT entry.
2.Updated binding document.
Changes since V3:
1.Moved the binding info to the /bindings/phy/
Changes since V2:
1.Used syscon interface to PMU handling.
2.Changed "sata-phy-i2c" to "exynos-sataphy-i2c".
Changes since V1:
1.Minor changes to node name convention.
2.Updated binding document.
.../devicetree/bindings/ata/exynos-sata-phy.txt | 14 --------
.../devicetree/bindings/ata/exynos-sata.txt | 25 +++++++++-----
.../devicetree/bindings/phy/samsung-phy.txt | 36 ++++++++++++++++++++
arch/arm/boot/dts/exynos5250-arndale.dts | 11 ++++++
arch/arm/boot/dts/exynos5250-smdk5250.dts | 8 ++---
arch/arm/boot/dts/exynos5250.dtsi | 17 ++++++---
6 files changed, 78 insertions(+), 33 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/ata/exynos-sata-phy.txt
diff --git a/Documentation/devicetree/bindings/ata/exynos-sata-phy.txt b/Documentation/devicetree/bindings/ata/exynos-sata-phy.txt
deleted file mode 100644
index 37824fa..0000000
--- a/Documentation/devicetree/bindings/ata/exynos-sata-phy.txt
+++ /dev/null
@@ -1,14 +0,0 @@
-* Samsung SATA PHY Controller
-
-SATA PHY nodes are defined to describe on-chip SATA Physical layer controllers.
-Each SATA PHY controller should have its own node.
-
-Required properties:
-- compatible : compatible list, contains "samsung,exynos5-sata-phy"
-- reg : <registers mapping>
-
-Example:
- sata@ffe07000 {
- compatible = "samsung,exynos5-sata-phy";
- reg = <0xffe07000 0x1000>;
- };
diff --git a/Documentation/devicetree/bindings/ata/exynos-sata.txt b/Documentation/devicetree/bindings/ata/exynos-sata.txt
index 0849f10..b2adb1f 100644
--- a/Documentation/devicetree/bindings/ata/exynos-sata.txt
+++ b/Documentation/devicetree/bindings/ata/exynos-sata.txt
@@ -4,14 +4,21 @@ SATA nodes are defined to describe on-chip Serial ATA controllers.
Each SATA controller should have its own node.
Required properties:
-- compatible : compatible list, contains "samsung,exynos5-sata"
-- interrupts : <interrupt mapping for SATA IRQ>
-- reg : <registers mapping>
-- samsung,sata-freq : <frequency in MHz>
+- compatible : compatible list, contains "samsung,exynos5-sata"
+- interrupts : <interrupt mapping for SATA IRQ>
+- reg : <registers mapping>
+- samsung,sata-freq : <frequency in MHz>
+- phys : as mentioned in phy-bindings.txt
+- phy-names : as mentioned in phy-bindings.txt
Example:
- sata@ffe08000 {
- compatible = "samsung,exynos5-sata";
- reg = <0xffe08000 0x1000>;
- interrupts = <115>;
- };
+ sata@122f0000 {
+ compatible = "snps,dwc-ahci";
+ samsung,sata-freq = <66>;
+ reg = <0x122f0000 0x1ff>;
+ interrupts = <0 115 0>;
+ clocks = <&clock 277>, <&clock 143>;
+ clock-names = "sata", "sclk_sata";
+ phys = <&sata_phy>;
+ phy-names = "sata-phy";
+ };
diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
index c0fccaa..a937f75 100644
--- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
+++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
@@ -20,3 +20,39 @@ Required properties:
- compatible : should be "samsung,exynos5250-dp-video-phy";
- reg : offset and length of the Display Port PHY register set;
- #phy-cells : from the generic PHY bindings, must be 0;
+
+Samsung SATA PHY Controller
+---------------------------
+
+SATA PHY nodes are defined to describe on-chip SATA Physical layer controllers.
+Each SATA PHY controller should have its own node.
+
+Required properties:
+- compatible : compatible list, contains "samsung,exynos5250-sata-phy"
+- reg : offset and length of the SATA PHY register set;
+- #phy-cells : from the generic phy bindings;
+
+Example:
+ sata_phy: sata-phy@12170000 {
+ compatible = "samsung,exynos5250-sata-phy";
+ reg = <0x12170000 0x1ff>;
+ clocks = <&clock 287>;
+ clock-names = "sata_phyctrl";
+ #phy-cells = <0>;
+ samsung,exynos-sataphy-i2c-phandle = <&sata_phy_i2c>;
+ samsung,syscon-phandle = <&pmu_syscon>;
+ };
+
+Device-Tree bindings for sataphy i2c client driver
+--------------------------------------------------
+
+Required properties:
+compatible: Should be "samsung,exynos-sataphy-i2c"
+- reg: I2C address of the sataphy i2c device.
+
+Example:
+
+ sata_phy_i2c:sata-phy@38 {
+ compatible = "samsung,exynos-sataphy-i2c";
+ reg = <0x38>;
+ };
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index b42e658..d9c9eaf 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -370,6 +370,17 @@
};
};
+ i2c@121D0000 {
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <40000>;
+ samsung,i2c-slave-addr = <0x38>;
+
+ sata_phy_i2c:sata-phy@38 {
+ compatible = "samsung,exynos-sataphy-i2c";
+ reg = <0x38>;
+ };
+ };
+
mmc_0: mmc@12200000 {
status = "okay";
num-slots = <1>;
diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 3e69837..e52eed8 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -96,16 +96,12 @@
samsung,i2c-slave-addr = <0x38>;
status = "okay";
- sata-phy {
- compatible = "samsung,sata-phy";
+ sata_phy_i2c:sata-phy@38 {
+ compatible = "samsung,exynos-sataphy-i2c";
reg = <0x38>;
};
};
- sata@122F0000 {
- samsung,sata-freq = <66>;
- };
-
i2c@12C80000 {
samsung,i2c-sda-delay = <100>;
samsung,i2c-max-bus-freq = <66000>;
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index dbb4a47..550d59e 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -46,6 +46,7 @@
i2c6 = &i2c_6;
i2c7 = &i2c_7;
i2c8 = &i2c_8;
+ i2c9 = &i2c_9;
pinctrl0 = &pinctrl_0;
pinctrl1 = &pinctrl_1;
pinctrl2 = &pinctrl_2;
@@ -231,16 +232,24 @@
};
sata@122F0000 {
- compatible = "samsung,exynos5-sata-ahci";
+ compatible = "snps,dwc-ahci";
+ samsung,sata-freq = <66>;
reg = <0x122F0000 0x1ff>;
interrupts = <0 115 0>;
clocks = <&clock 277>, <&clock 143>;
clock-names = "sata", "sclk_sata";
+ phys = <&sata_phy>;
+ phy-names = "sata-phy";
};
- sata-phy@12170000 {
- compatible = "samsung,exynos5-sata-phy";
+ sata_phy: sata-phy@12170000 {
+ compatible = "samsung,exynos5250-sata-phy";
reg = <0x12170000 0x1ff>;
+ clocks = <&clock 287>;
+ clock-names = "sata_phyctrl";
+ #phy-cells = <0>;
+ samsung,exynos-sataphy-i2c-phandle = <&sata_phy_i2c>;
+ samsung,syscon-phandle = <&pmu_syscon>;
};
i2c_0: i2c@12C60000 {
@@ -358,7 +367,7 @@
status = "disabled";
};
- i2c@121D0000 {
+ i2c_9: i2c@121D0000 {
compatible = "samsung,exynos5-sata-phy-i2c";
reg = <0x121D0000 0x100>;
#address-cells = <1>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V6 2/2] ARM: dts: Enable ahci sata and sata phy
@ 2014-01-10 7:00 ` Yuvaraj Kumar C D
0 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar C D @ 2014-01-10 7:00 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds dt entry for ahci sata controller and its
corresponding phy controller.phy node has been added w.r.t
new generic phy framework.
Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
---
Changes since V4:
1.Used the new phandle "sata_phy_i2c" in the DT entry.
2.Updated binding document.
Changes since V3:
1.Moved the binding info to the /bindings/phy/
Changes since V2:
1.Used syscon interface to PMU handling.
2.Changed "sata-phy-i2c" to "exynos-sataphy-i2c".
Changes since V1:
1.Minor changes to node name convention.
2.Updated binding document.
.../devicetree/bindings/ata/exynos-sata-phy.txt | 14 --------
.../devicetree/bindings/ata/exynos-sata.txt | 25 +++++++++-----
.../devicetree/bindings/phy/samsung-phy.txt | 36 ++++++++++++++++++++
arch/arm/boot/dts/exynos5250-arndale.dts | 11 ++++++
arch/arm/boot/dts/exynos5250-smdk5250.dts | 8 ++---
arch/arm/boot/dts/exynos5250.dtsi | 17 ++++++---
6 files changed, 78 insertions(+), 33 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/ata/exynos-sata-phy.txt
diff --git a/Documentation/devicetree/bindings/ata/exynos-sata-phy.txt b/Documentation/devicetree/bindings/ata/exynos-sata-phy.txt
deleted file mode 100644
index 37824fa..0000000
--- a/Documentation/devicetree/bindings/ata/exynos-sata-phy.txt
+++ /dev/null
@@ -1,14 +0,0 @@
-* Samsung SATA PHY Controller
-
-SATA PHY nodes are defined to describe on-chip SATA Physical layer controllers.
-Each SATA PHY controller should have its own node.
-
-Required properties:
-- compatible : compatible list, contains "samsung,exynos5-sata-phy"
-- reg : <registers mapping>
-
-Example:
- sata at ffe07000 {
- compatible = "samsung,exynos5-sata-phy";
- reg = <0xffe07000 0x1000>;
- };
diff --git a/Documentation/devicetree/bindings/ata/exynos-sata.txt b/Documentation/devicetree/bindings/ata/exynos-sata.txt
index 0849f10..b2adb1f 100644
--- a/Documentation/devicetree/bindings/ata/exynos-sata.txt
+++ b/Documentation/devicetree/bindings/ata/exynos-sata.txt
@@ -4,14 +4,21 @@ SATA nodes are defined to describe on-chip Serial ATA controllers.
Each SATA controller should have its own node.
Required properties:
-- compatible : compatible list, contains "samsung,exynos5-sata"
-- interrupts : <interrupt mapping for SATA IRQ>
-- reg : <registers mapping>
-- samsung,sata-freq : <frequency in MHz>
+- compatible : compatible list, contains "samsung,exynos5-sata"
+- interrupts : <interrupt mapping for SATA IRQ>
+- reg : <registers mapping>
+- samsung,sata-freq : <frequency in MHz>
+- phys : as mentioned in phy-bindings.txt
+- phy-names : as mentioned in phy-bindings.txt
Example:
- sata at ffe08000 {
- compatible = "samsung,exynos5-sata";
- reg = <0xffe08000 0x1000>;
- interrupts = <115>;
- };
+ sata at 122f0000 {
+ compatible = "snps,dwc-ahci";
+ samsung,sata-freq = <66>;
+ reg = <0x122f0000 0x1ff>;
+ interrupts = <0 115 0>;
+ clocks = <&clock 277>, <&clock 143>;
+ clock-names = "sata", "sclk_sata";
+ phys = <&sata_phy>;
+ phy-names = "sata-phy";
+ };
diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
index c0fccaa..a937f75 100644
--- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
+++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
@@ -20,3 +20,39 @@ Required properties:
- compatible : should be "samsung,exynos5250-dp-video-phy";
- reg : offset and length of the Display Port PHY register set;
- #phy-cells : from the generic PHY bindings, must be 0;
+
+Samsung SATA PHY Controller
+---------------------------
+
+SATA PHY nodes are defined to describe on-chip SATA Physical layer controllers.
+Each SATA PHY controller should have its own node.
+
+Required properties:
+- compatible : compatible list, contains "samsung,exynos5250-sata-phy"
+- reg : offset and length of the SATA PHY register set;
+- #phy-cells : from the generic phy bindings;
+
+Example:
+ sata_phy: sata-phy at 12170000 {
+ compatible = "samsung,exynos5250-sata-phy";
+ reg = <0x12170000 0x1ff>;
+ clocks = <&clock 287>;
+ clock-names = "sata_phyctrl";
+ #phy-cells = <0>;
+ samsung,exynos-sataphy-i2c-phandle = <&sata_phy_i2c>;
+ samsung,syscon-phandle = <&pmu_syscon>;
+ };
+
+Device-Tree bindings for sataphy i2c client driver
+--------------------------------------------------
+
+Required properties:
+compatible: Should be "samsung,exynos-sataphy-i2c"
+- reg: I2C address of the sataphy i2c device.
+
+Example:
+
+ sata_phy_i2c:sata-phy at 38 {
+ compatible = "samsung,exynos-sataphy-i2c";
+ reg = <0x38>;
+ };
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index b42e658..d9c9eaf 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -370,6 +370,17 @@
};
};
+ i2c at 121D0000 {
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <40000>;
+ samsung,i2c-slave-addr = <0x38>;
+
+ sata_phy_i2c:sata-phy at 38 {
+ compatible = "samsung,exynos-sataphy-i2c";
+ reg = <0x38>;
+ };
+ };
+
mmc_0: mmc at 12200000 {
status = "okay";
num-slots = <1>;
diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 3e69837..e52eed8 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -96,16 +96,12 @@
samsung,i2c-slave-addr = <0x38>;
status = "okay";
- sata-phy {
- compatible = "samsung,sata-phy";
+ sata_phy_i2c:sata-phy at 38 {
+ compatible = "samsung,exynos-sataphy-i2c";
reg = <0x38>;
};
};
- sata at 122F0000 {
- samsung,sata-freq = <66>;
- };
-
i2c at 12C80000 {
samsung,i2c-sda-delay = <100>;
samsung,i2c-max-bus-freq = <66000>;
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index dbb4a47..550d59e 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -46,6 +46,7 @@
i2c6 = &i2c_6;
i2c7 = &i2c_7;
i2c8 = &i2c_8;
+ i2c9 = &i2c_9;
pinctrl0 = &pinctrl_0;
pinctrl1 = &pinctrl_1;
pinctrl2 = &pinctrl_2;
@@ -231,16 +232,24 @@
};
sata at 122F0000 {
- compatible = "samsung,exynos5-sata-ahci";
+ compatible = "snps,dwc-ahci";
+ samsung,sata-freq = <66>;
reg = <0x122F0000 0x1ff>;
interrupts = <0 115 0>;
clocks = <&clock 277>, <&clock 143>;
clock-names = "sata", "sclk_sata";
+ phys = <&sata_phy>;
+ phy-names = "sata-phy";
};
- sata-phy at 12170000 {
- compatible = "samsung,exynos5-sata-phy";
+ sata_phy: sata-phy at 12170000 {
+ compatible = "samsung,exynos5250-sata-phy";
reg = <0x12170000 0x1ff>;
+ clocks = <&clock 287>;
+ clock-names = "sata_phyctrl";
+ #phy-cells = <0>;
+ samsung,exynos-sataphy-i2c-phandle = <&sata_phy_i2c>;
+ samsung,syscon-phandle = <&pmu_syscon>;
};
i2c_0: i2c at 12C60000 {
@@ -358,7 +367,7 @@
status = "disabled";
};
- i2c at 121D0000 {
+ i2c_9: i2c at 121D0000 {
compatible = "samsung,exynos5-sata-phy-i2c";
reg = <0x121D0000 0x100>;
#address-cells = <1>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
2014-01-10 7:00 ` Yuvaraj Kumar C D
@ 2014-01-10 15:05 ` Tomasz Figa
-1 siblings, 0 replies; 14+ messages in thread
From: Tomasz Figa @ 2014-01-10 15:05 UTC (permalink / raw)
To: Yuvaraj Kumar C D, kishon, kgene.kim, linux-kernel,
linux-arm-kernel, devicetree, linux-doc, Wolfram Sang
Cc: mark.rutland, jg1.han, b.zolnierkie, joshi, swarren,
grant.likely, christoffer.dall, linux-ide, Yuvaraj Kumar C D,
Girish K S, Vasanth Ananthan
Hi Yuvaraj,
In general this version looks pretty good, but I have some questions inline.
On 10.01.2014 08:00, Yuvaraj Kumar C D wrote:
[snip]
> diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c b/drivers/phy/phy-exynos5250-sata-i2c.c
> new file mode 100644
> index 0000000..206e337
> --- /dev/null
> +++ b/drivers/phy/phy-exynos5250-sata-i2c.c
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
> + * Author:
> + * Yuvaraj C D <yuvaraj.cd@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +static int exynos_sata_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *i2c_id)
> +{
> + return 0;
> +}
> +
> +static const struct i2c_device_id sataphy_i2c_device_match[] = {
> + { "exynos-sataphy-i2c", 0 },
> +};
> +
> +static struct i2c_driver sataphy_i2c_driver = {
> + .probe = exynos_sata_i2c_probe,
> + .id_table = sataphy_i2c_device_match,
> + .driver = {
> + .name = "exynos-sataphy-i2c",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init exynos5250_phy_i2c_init(void)
> +{
> + return i2c_add_driver(&sataphy_i2c_driver);
> +}
> +module_init(exynos5250_phy_i2c_init);
Hmm, is this driver even necessary now?
Wolfram, would it be possible to use an i2c_client without a driver
bound to it?
> diff --git a/drivers/phy/phy-exynos5250-sata.c b/drivers/phy/phy-exynos5250-sata.c
> new file mode 100644
> index 0000000..6e5ff8d
> --- /dev/null
> +++ b/drivers/phy/phy-exynos5250-sata.c
> @@ -0,0 +1,238 @@
> +/*
> + * Samsung SATA SerDes(PHY) driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Authors: Girish K S <ks.giri@samsung.com>
> + * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
> + *
> + * 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define EXYNOS5_SATA_RESET 0x4
> +#define RESET_CMN_RST_N (1 << 1)
> +#define LINK_RESET 0xF0000
nit: Lowercase is preferred in hexadecimal notation.
+ all other occurrences in this file.
> +#define EXYNOS5_SATA_MODE0 0x10
> +#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
> +#define SATA_SPD_GEN3 (2 << 0)
> +#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 EXYNOS_SATA_PHY_EN (1 << 0)
> +#define SATAPHY_CONTROL_OFFSET 0x0724
> +
> +struct exynos_sata_phy {
> + struct phy *phy;
> + struct clk *phyclk;
> + void __iomem *regs;
> + void __iomem *pmureg;
> + struct i2c_client *client;
> +};
> +
> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32 checkbit,
> + u32 status)
> +{
> + unsigned long timeout = jiffies + usecs_to_jiffies(1000);
nit: It would be better to define the timeout using a macro to not use
magic numbers.
> +
> + while (time_before(jiffies, timeout)) {
> + if ((readl(base + reg) & checkbit) == status)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int exynos_sata_phy_power_on(struct phy *phy)
> +{
> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
> +
> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
regmap_update_bits can return an error. Wouldn't it be better to return
it as return value of this function instead of returning 0 all the time?
As a side effect, this would make the function smaller by two lines.
> +
> + return 0;
> +}
> +
> +static int exynos_sata_phy_power_off(struct phy *phy)
> +{
> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
> +
> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
> + EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
Same here.
> +
> + return 0;
> +}
> +
> +static int exynos_sata_phy_init(struct phy *phy)
> +{
> + u32 val = 0;
> + int ret = 0;
> + u8 buf[] = { 0x3A, 0x0B };
> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
> +
> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
regmap_update_bits returns an error code.
> +
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val |= 0xFF;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val |= LINK_RESET;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val |= RESET_CMN_RST_N;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
> + val &= ~PHCTRLM_REF_RATE;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
> +
> + /* High speed enable for Gen3 */
> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
> + val |= PHCTRLM_HIGH_SPEED;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
> + val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
> + val |= SATA_SPD_GEN3;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
> +
> + ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
> + if (ret < 0)
> + return -ENXIO;
Wouldn't it be better to return the same error code as i2c_master_send
returned?
> +
> + /* release cmu reset */
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val &= ~RESET_CMN_RST_N;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val |= RESET_CMN_RST_N;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + return (wait_for_reg_status(sata_phy->regs, EXYNOS5_SATA_PHSATA_STATM,
> + PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
> +
nit: Stray blank line.
Also it might be more readable after making wait_for_reg_status() return
an integer error code (0 and e.g. -EFAULT) and rewriting the last line to:
ret = wait_for_reg_status(sata_phy->regs,
EXYNOS5_SATA_PHSATA_STATM,
PHSTATM_PLL_LOCKED, 1);
if (ret < 0)
dev_err(&sata_phy->client->dev,
"PHY PLL locking failed\n");
return ret;
By the way, isn't this initialization really needed whenever the PHY is
powered on?
> +}
> +
> +static struct phy_ops exynos_sata_phy_ops = {
> + .init = exynos_sata_phy_init,
> + .power_on = exynos_sata_phy_power_on,
> + .power_off = exynos_sata_phy_power_off,
> + .owner = THIS_MODULE,
> +};
> +
> +static int exynos_sata_phy_probe(struct platform_device *pdev)
> +{
> + struct exynos_sata_phy *sata_phy;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct phy_provider *phy_provider;
> + struct device_node *node;
> + int ret = 0;
> +
> + sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
> + if (!sata_phy)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + sata_phy->regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(sata_phy->regs))
> + return PTR_ERR(sata_phy->regs);
> +
> + sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "samsung,syscon-phandle");
pmureg is defined as (void __iomem *) in struct exynos_sata_phy, but
syscon_regmap_lookup_by_phandle() returns (struct regmap *). Moreover it
does not return NULL on error, but rather ERR_PTR(). Please correct this.
> + if (!sata_phy->pmureg) {
> + dev_err(dev, "syscon regmap lookup failed.\n");
> + return PTR_ERR(sata_phy->pmureg);
> + }
> +
> + node = of_parse_phandle(dev->of_node,
> + "samsung,exynos-sataphy-i2c-phandle", 0);
> + if (!node)
> + return -ENODEV;
An error here means that a required DT property was not specified or was
specified incorrectly. IMHO -EINVAL would be better here.
> +
> + sata_phy->client = of_find_i2c_device_by_node(node);
> + if (!sata_phy->client)
> + return -EPROBE_DEFER;
> +
> + dev_set_drvdata(dev, sata_phy);
> +
> + sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
> + if (IS_ERR(sata_phy->phyclk)) {
> + dev_err(dev, "failed to get clk for PHY\n");
> + return PTR_ERR(sata_phy->phyclk);
> + }
> +
> + ret = clk_prepare_enable(sata_phy->phyclk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable source clk\n");
> + return ret;
> + }
> +
> + sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
> + if (IS_ERR(sata_phy->phy)) {
> + clk_disable_unprepare(sata_phy->phyclk);
> + dev_err(dev, "failed to create PHY\n");
> + return PTR_ERR(sata_phy->phy);
> + }
> +
> + phy_set_drvdata(sata_phy->phy, sata_phy);
> +
> + phy_provider = devm_of_phy_provider_register(dev,
> + of_phy_simple_xlate);
> + if (IS_ERR(phy_provider)) {
> + clk_disable_unprepare(sata_phy->phyclk);
> + return PTR_ERR(phy_provider);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id exynos_sata_phy_of_match[] = {
> + { .compatible = "samsung,exynos5250-sata-phy" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
> +
> +static struct platform_driver exynos_sata_phy_driver = {
> + .probe = exynos_sata_phy_probe,
If this driver can be compiled as module, don't you also need remove?
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
@ 2014-01-10 15:05 ` Tomasz Figa
0 siblings, 0 replies; 14+ messages in thread
From: Tomasz Figa @ 2014-01-10 15:05 UTC (permalink / raw)
To: linux-arm-kernel
Hi Yuvaraj,
In general this version looks pretty good, but I have some questions inline.
On 10.01.2014 08:00, Yuvaraj Kumar C D wrote:
[snip]
> diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c b/drivers/phy/phy-exynos5250-sata-i2c.c
> new file mode 100644
> index 0000000..206e337
> --- /dev/null
> +++ b/drivers/phy/phy-exynos5250-sata-i2c.c
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
> + * Author:
> + * Yuvaraj C D <yuvaraj.cd@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +static int exynos_sata_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *i2c_id)
> +{
> + return 0;
> +}
> +
> +static const struct i2c_device_id sataphy_i2c_device_match[] = {
> + { "exynos-sataphy-i2c", 0 },
> +};
> +
> +static struct i2c_driver sataphy_i2c_driver = {
> + .probe = exynos_sata_i2c_probe,
> + .id_table = sataphy_i2c_device_match,
> + .driver = {
> + .name = "exynos-sataphy-i2c",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init exynos5250_phy_i2c_init(void)
> +{
> + return i2c_add_driver(&sataphy_i2c_driver);
> +}
> +module_init(exynos5250_phy_i2c_init);
Hmm, is this driver even necessary now?
Wolfram, would it be possible to use an i2c_client without a driver
bound to it?
> diff --git a/drivers/phy/phy-exynos5250-sata.c b/drivers/phy/phy-exynos5250-sata.c
> new file mode 100644
> index 0000000..6e5ff8d
> --- /dev/null
> +++ b/drivers/phy/phy-exynos5250-sata.c
> @@ -0,0 +1,238 @@
> +/*
> + * Samsung SATA SerDes(PHY) driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Authors: Girish K S <ks.giri@samsung.com>
> + * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
> + *
> + * 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define EXYNOS5_SATA_RESET 0x4
> +#define RESET_CMN_RST_N (1 << 1)
> +#define LINK_RESET 0xF0000
nit: Lowercase is preferred in hexadecimal notation.
+ all other occurrences in this file.
> +#define EXYNOS5_SATA_MODE0 0x10
> +#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
> +#define SATA_SPD_GEN3 (2 << 0)
> +#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 EXYNOS_SATA_PHY_EN (1 << 0)
> +#define SATAPHY_CONTROL_OFFSET 0x0724
> +
> +struct exynos_sata_phy {
> + struct phy *phy;
> + struct clk *phyclk;
> + void __iomem *regs;
> + void __iomem *pmureg;
> + struct i2c_client *client;
> +};
> +
> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32 checkbit,
> + u32 status)
> +{
> + unsigned long timeout = jiffies + usecs_to_jiffies(1000);
nit: It would be better to define the timeout using a macro to not use
magic numbers.
> +
> + while (time_before(jiffies, timeout)) {
> + if ((readl(base + reg) & checkbit) == status)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int exynos_sata_phy_power_on(struct phy *phy)
> +{
> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
> +
> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
regmap_update_bits can return an error. Wouldn't it be better to return
it as return value of this function instead of returning 0 all the time?
As a side effect, this would make the function smaller by two lines.
> +
> + return 0;
> +}
> +
> +static int exynos_sata_phy_power_off(struct phy *phy)
> +{
> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
> +
> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
> + EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
Same here.
> +
> + return 0;
> +}
> +
> +static int exynos_sata_phy_init(struct phy *phy)
> +{
> + u32 val = 0;
> + int ret = 0;
> + u8 buf[] = { 0x3A, 0x0B };
> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
> +
> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
regmap_update_bits returns an error code.
> +
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val |= 0xFF;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val |= LINK_RESET;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val |= RESET_CMN_RST_N;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
> + val &= ~PHCTRLM_REF_RATE;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
> +
> + /* High speed enable for Gen3 */
> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
> + val |= PHCTRLM_HIGH_SPEED;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
> + val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
> + val |= SATA_SPD_GEN3;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
> +
> + ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
> + if (ret < 0)
> + return -ENXIO;
Wouldn't it be better to return the same error code as i2c_master_send
returned?
> +
> + /* release cmu reset */
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val &= ~RESET_CMN_RST_N;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
> + val |= RESET_CMN_RST_N;
> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
> +
> + return (wait_for_reg_status(sata_phy->regs, EXYNOS5_SATA_PHSATA_STATM,
> + PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
> +
nit: Stray blank line.
Also it might be more readable after making wait_for_reg_status() return
an integer error code (0 and e.g. -EFAULT) and rewriting the last line to:
ret = wait_for_reg_status(sata_phy->regs,
EXYNOS5_SATA_PHSATA_STATM,
PHSTATM_PLL_LOCKED, 1);
if (ret < 0)
dev_err(&sata_phy->client->dev,
"PHY PLL locking failed\n");
return ret;
By the way, isn't this initialization really needed whenever the PHY is
powered on?
> +}
> +
> +static struct phy_ops exynos_sata_phy_ops = {
> + .init = exynos_sata_phy_init,
> + .power_on = exynos_sata_phy_power_on,
> + .power_off = exynos_sata_phy_power_off,
> + .owner = THIS_MODULE,
> +};
> +
> +static int exynos_sata_phy_probe(struct platform_device *pdev)
> +{
> + struct exynos_sata_phy *sata_phy;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct phy_provider *phy_provider;
> + struct device_node *node;
> + int ret = 0;
> +
> + sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
> + if (!sata_phy)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + sata_phy->regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(sata_phy->regs))
> + return PTR_ERR(sata_phy->regs);
> +
> + sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "samsung,syscon-phandle");
pmureg is defined as (void __iomem *) in struct exynos_sata_phy, but
syscon_regmap_lookup_by_phandle() returns (struct regmap *). Moreover it
does not return NULL on error, but rather ERR_PTR(). Please correct this.
> + if (!sata_phy->pmureg) {
> + dev_err(dev, "syscon regmap lookup failed.\n");
> + return PTR_ERR(sata_phy->pmureg);
> + }
> +
> + node = of_parse_phandle(dev->of_node,
> + "samsung,exynos-sataphy-i2c-phandle", 0);
> + if (!node)
> + return -ENODEV;
An error here means that a required DT property was not specified or was
specified incorrectly. IMHO -EINVAL would be better here.
> +
> + sata_phy->client = of_find_i2c_device_by_node(node);
> + if (!sata_phy->client)
> + return -EPROBE_DEFER;
> +
> + dev_set_drvdata(dev, sata_phy);
> +
> + sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
> + if (IS_ERR(sata_phy->phyclk)) {
> + dev_err(dev, "failed to get clk for PHY\n");
> + return PTR_ERR(sata_phy->phyclk);
> + }
> +
> + ret = clk_prepare_enable(sata_phy->phyclk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable source clk\n");
> + return ret;
> + }
> +
> + sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
> + if (IS_ERR(sata_phy->phy)) {
> + clk_disable_unprepare(sata_phy->phyclk);
> + dev_err(dev, "failed to create PHY\n");
> + return PTR_ERR(sata_phy->phy);
> + }
> +
> + phy_set_drvdata(sata_phy->phy, sata_phy);
> +
> + phy_provider = devm_of_phy_provider_register(dev,
> + of_phy_simple_xlate);
> + if (IS_ERR(phy_provider)) {
> + clk_disable_unprepare(sata_phy->phyclk);
> + return PTR_ERR(phy_provider);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id exynos_sata_phy_of_match[] = {
> + { .compatible = "samsung,exynos5250-sata-phy" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
> +
> +static struct platform_driver exynos_sata_phy_driver = {
> + .probe = exynos_sata_phy_probe,
If this driver can be compiled as module, don't you also need remove?
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
2014-01-10 15:05 ` Tomasz Figa
(?)
@ 2014-01-17 11:50 ` Yuvaraj Kumar
-1 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar @ 2014-01-17 11:50 UTC (permalink / raw)
To: Tomasz Figa
Cc: Mark Rutland, devicetree, kgene.kim, Yuvaraj Kumar C D,
Stephen Warren, linux-doc, Girish K S, Wolfram Sang,
Vasanth Ananthan, linux-kernel, Kishon Vijay Abraham I,
linux-ide, Grant Likely, Bartlomiej Zolnierkiewicz,
Christoffer Dall, Jingoo Han, sunil joshi, linux-arm-kernel
Sorry for the late reply.
On Fri, Jan 10, 2014 at 8:35 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yuvaraj,
>
> In general this version looks pretty good, but I have some questions inline.
Please find my comments inline.
>
> On 10.01.2014 08:00, Yuvaraj Kumar C D wrote:
> [snip]
>
>> diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c
>> b/drivers/phy/phy-exynos5250-sata-i2c.c
>> new file mode 100644
>> index 0000000..206e337
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata-i2c.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
>> + * Author:
>> + * Yuvaraj C D <yuvaraj.cd@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License as published by
>> the
>> + * Free Software Foundation; either version 2 of the License, or (at
>> your
>> + * option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +static int exynos_sata_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *i2c_id)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id sataphy_i2c_device_match[] = {
>> + { "exynos-sataphy-i2c", 0 },
>> +};
>> +
>> +static struct i2c_driver sataphy_i2c_driver = {
>> + .probe = exynos_sata_i2c_probe,
>> + .id_table = sataphy_i2c_device_match,
>> + .driver = {
>> + .name = "exynos-sataphy-i2c",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +static int __init exynos5250_phy_i2c_init(void)
>> +{
>> + return i2c_add_driver(&sataphy_i2c_driver);
>> +}
>> +module_init(exynos5250_phy_i2c_init);
>
>
> Hmm, is this driver even necessary now?
Yes.It will not necessary now with of_find_i2c_device_by_node.I will
remove phy-exynos5250-sata-i2c.c
>
> Wolfram, would it be possible to use an i2c_client without a driver bound to
> it?
>
>
>> diff --git a/drivers/phy/phy-exynos5250-sata.c
>> b/drivers/phy/phy-exynos5250-sata.c
>> new file mode 100644
>> index 0000000..6e5ff8d
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * Samsung SATA SerDes(PHY) driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Authors: Girish K S <ks.giri@samsung.com>
>> + * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> + *
>> + * 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/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define EXYNOS5_SATA_RESET 0x4
>> +#define RESET_CMN_RST_N (1 << 1)
>> +#define LINK_RESET 0xF0000
>
>
> nit: Lowercase is preferred in hexadecimal notation.
> + all other occurrences in this file.
Ok.
>
>
>> +#define EXYNOS5_SATA_MODE0 0x10
>> +#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
>> +#define SATA_SPD_GEN3 (2 << 0)
>> +#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 EXYNOS_SATA_PHY_EN (1 << 0)
>> +#define SATAPHY_CONTROL_OFFSET 0x0724
>> +
>> +struct exynos_sata_phy {
>> + struct phy *phy;
>> + struct clk *phyclk;
>> + void __iomem *regs;
>> + void __iomem *pmureg;
>> + struct i2c_client *client;
>> +};
>> +
>> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
>> checkbit,
>> + u32 status)
>> +{
>> + unsigned long timeout = jiffies + usecs_to_jiffies(1000);
>
>
> nit: It would be better to define the timeout using a macro to not use magic
> numbers.
Ok
>
>
>> +
>> + while (time_before(jiffies, timeout)) {
>> + if ((readl(base + reg) & checkbit) == status)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int exynos_sata_phy_power_on(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits can return an error. Wouldn't it be better to return it
> as return value of this function instead of returning 0 all the time? As a
> side effect, this would make the function smaller by two lines.
Yes.Will incorporate in next version.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_power_off(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
>
>
> Same here.
Yes.Will incorporate in next version.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_init(struct phy *phy)
>> +{
>> + u32 val = 0;
>> + int ret = 0;
>> + u8 buf[] = { 0x3A, 0x0B };
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits returns an error code.
Yes.Will incorporate in next version.
>
>
>> +
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= 0xFF;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= LINK_RESET;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val &= ~PHCTRLM_REF_RATE;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + /* High speed enable for Gen3 */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val |= PHCTRLM_HIGH_SPEED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> + val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
>> + val |= SATA_SPD_GEN3;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
>> +
>> + ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
>> + if (ret < 0)
>> + return -ENXIO;
>
>
> Wouldn't it be better to return the same error code as i2c_master_send
> returned?
Yes.Will incorporate in next version.
>
>
>> +
>> + /* release cmu reset */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val &= ~RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + return (wait_for_reg_status(sata_phy->regs,
>> EXYNOS5_SATA_PHSATA_STATM,
>> + PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
>> +
>
>
> nit: Stray blank line.
Ok.will remove
>
> Also it might be more readable after making wait_for_reg_status() return an
> integer error code (0 and e.g. -EFAULT) and rewriting the last line to:
>
> ret = wait_for_reg_status(sata_phy->regs,
> EXYNOS5_SATA_PHSATA_STATM,
> PHSTATM_PLL_LOCKED, 1);
> if (ret < 0)
> dev_err(&sata_phy->client->dev,
> "PHY PLL locking failed\n");
>
> return ret;
>
Ok.
> By the way, isn't this initialization really needed whenever the PHY is
> powered on?
In Exynos5250, we do require to power on the phy before writing
anything into phy controller register space.
As you know,phy_ops have separate callbacks power_on and init .So I
just used those to separate these functionalities.
>
>
>> +}
>> +
>> +static struct phy_ops exynos_sata_phy_ops = {
>> + .init = exynos_sata_phy_init,
>> + .power_on = exynos_sata_phy_power_on,
>> + .power_off = exynos_sata_phy_power_off,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int exynos_sata_phy_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_sata_phy *sata_phy;
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct phy_provider *phy_provider;
>> + struct device_node *node;
>> + int ret = 0;
>> +
>> + sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
>> + if (!sata_phy)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + sata_phy->regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sata_phy->regs))
>> + return PTR_ERR(sata_phy->regs);
>> +
>> + sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-phandle");
>
>
> pmureg is defined as (void __iomem *) in struct exynos_sata_phy, but
> syscon_regmap_lookup_by_phandle() returns (struct regmap *). Moreover it
> does not return NULL on error, but rather ERR_PTR(). Please correct this.
Ohh!my mistake.Will correct in next version.
>
>
>> + if (!sata_phy->pmureg) {
>> + dev_err(dev, "syscon regmap lookup failed.\n");
>> + return PTR_ERR(sata_phy->pmureg);
>> + }
>> +
>> + node = of_parse_phandle(dev->of_node,
>> + "samsung,exynos-sataphy-i2c-phandle", 0);
>> + if (!node)
>> + return -ENODEV;
>
>
> An error here means that a required DT property was not specified or was
> specified incorrectly. IMHO -EINVAL would be better here.
ok
>
>
>> +
>> + sata_phy->client = of_find_i2c_device_by_node(node);
>> + if (!sata_phy->client)
>> + return -EPROBE_DEFER;
>> +
>> + dev_set_drvdata(dev, sata_phy);
>> +
>> + sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
>> + if (IS_ERR(sata_phy->phyclk)) {
>> + dev_err(dev, "failed to get clk for PHY\n");
>> + return PTR_ERR(sata_phy->phyclk);
>> + }
>> +
>> + ret = clk_prepare_enable(sata_phy->phyclk);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable source clk\n");
>> + return ret;
>> + }
>> +
>> + sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
>> + if (IS_ERR(sata_phy->phy)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + dev_err(dev, "failed to create PHY\n");
>> + return PTR_ERR(sata_phy->phy);
>> + }
>> +
>> + phy_set_drvdata(sata_phy->phy, sata_phy);
>> +
>> + phy_provider = devm_of_phy_provider_register(dev,
>> + of_phy_simple_xlate);
>> + if (IS_ERR(phy_provider)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + return PTR_ERR(phy_provider);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id exynos_sata_phy_of_match[] = {
>> + { .compatible = "samsung,exynos5250-sata-phy" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
>> +
>> +static struct platform_driver exynos_sata_phy_driver = {
>> + .probe = exynos_sata_phy_probe,
>
>
> If this driver can be compiled as module, don't you also need remove?
Will add in next version.
>
> Best regards,
> Tomasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
@ 2014-01-17 11:50 ` Yuvaraj Kumar
0 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar @ 2014-01-17 11:50 UTC (permalink / raw)
To: Tomasz Figa
Cc: Kishon Vijay Abraham I, kgene.kim, linux-kernel,
linux-arm-kernel, devicetree, linux-doc, Wolfram Sang,
Mark Rutland, Jingoo Han, Bartlomiej Zolnierkiewicz, sunil joshi,
Stephen Warren, Grant Likely, Christoffer Dall, linux-ide,
Yuvaraj Kumar C D, Girish K S, Vasanth Ananthan
Sorry for the late reply.
On Fri, Jan 10, 2014 at 8:35 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yuvaraj,
>
> In general this version looks pretty good, but I have some questions inline.
Please find my comments inline.
>
> On 10.01.2014 08:00, Yuvaraj Kumar C D wrote:
> [snip]
>
>> diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c
>> b/drivers/phy/phy-exynos5250-sata-i2c.c
>> new file mode 100644
>> index 0000000..206e337
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata-i2c.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
>> + * Author:
>> + * Yuvaraj C D <yuvaraj.cd@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License as published by
>> the
>> + * Free Software Foundation; either version 2 of the License, or (at
>> your
>> + * option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +static int exynos_sata_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *i2c_id)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id sataphy_i2c_device_match[] = {
>> + { "exynos-sataphy-i2c", 0 },
>> +};
>> +
>> +static struct i2c_driver sataphy_i2c_driver = {
>> + .probe = exynos_sata_i2c_probe,
>> + .id_table = sataphy_i2c_device_match,
>> + .driver = {
>> + .name = "exynos-sataphy-i2c",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +static int __init exynos5250_phy_i2c_init(void)
>> +{
>> + return i2c_add_driver(&sataphy_i2c_driver);
>> +}
>> +module_init(exynos5250_phy_i2c_init);
>
>
> Hmm, is this driver even necessary now?
Yes.It will not necessary now with of_find_i2c_device_by_node.I will
remove phy-exynos5250-sata-i2c.c
>
> Wolfram, would it be possible to use an i2c_client without a driver bound to
> it?
>
>
>> diff --git a/drivers/phy/phy-exynos5250-sata.c
>> b/drivers/phy/phy-exynos5250-sata.c
>> new file mode 100644
>> index 0000000..6e5ff8d
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * Samsung SATA SerDes(PHY) driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Authors: Girish K S <ks.giri@samsung.com>
>> + * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> + *
>> + * 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/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define EXYNOS5_SATA_RESET 0x4
>> +#define RESET_CMN_RST_N (1 << 1)
>> +#define LINK_RESET 0xF0000
>
>
> nit: Lowercase is preferred in hexadecimal notation.
> + all other occurrences in this file.
Ok.
>
>
>> +#define EXYNOS5_SATA_MODE0 0x10
>> +#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
>> +#define SATA_SPD_GEN3 (2 << 0)
>> +#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 EXYNOS_SATA_PHY_EN (1 << 0)
>> +#define SATAPHY_CONTROL_OFFSET 0x0724
>> +
>> +struct exynos_sata_phy {
>> + struct phy *phy;
>> + struct clk *phyclk;
>> + void __iomem *regs;
>> + void __iomem *pmureg;
>> + struct i2c_client *client;
>> +};
>> +
>> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
>> checkbit,
>> + u32 status)
>> +{
>> + unsigned long timeout = jiffies + usecs_to_jiffies(1000);
>
>
> nit: It would be better to define the timeout using a macro to not use magic
> numbers.
Ok
>
>
>> +
>> + while (time_before(jiffies, timeout)) {
>> + if ((readl(base + reg) & checkbit) == status)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int exynos_sata_phy_power_on(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits can return an error. Wouldn't it be better to return it
> as return value of this function instead of returning 0 all the time? As a
> side effect, this would make the function smaller by two lines.
Yes.Will incorporate in next version.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_power_off(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
>
>
> Same here.
Yes.Will incorporate in next version.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_init(struct phy *phy)
>> +{
>> + u32 val = 0;
>> + int ret = 0;
>> + u8 buf[] = { 0x3A, 0x0B };
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits returns an error code.
Yes.Will incorporate in next version.
>
>
>> +
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= 0xFF;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= LINK_RESET;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val &= ~PHCTRLM_REF_RATE;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + /* High speed enable for Gen3 */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val |= PHCTRLM_HIGH_SPEED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> + val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
>> + val |= SATA_SPD_GEN3;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
>> +
>> + ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
>> + if (ret < 0)
>> + return -ENXIO;
>
>
> Wouldn't it be better to return the same error code as i2c_master_send
> returned?
Yes.Will incorporate in next version.
>
>
>> +
>> + /* release cmu reset */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val &= ~RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + return (wait_for_reg_status(sata_phy->regs,
>> EXYNOS5_SATA_PHSATA_STATM,
>> + PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
>> +
>
>
> nit: Stray blank line.
Ok.will remove
>
> Also it might be more readable after making wait_for_reg_status() return an
> integer error code (0 and e.g. -EFAULT) and rewriting the last line to:
>
> ret = wait_for_reg_status(sata_phy->regs,
> EXYNOS5_SATA_PHSATA_STATM,
> PHSTATM_PLL_LOCKED, 1);
> if (ret < 0)
> dev_err(&sata_phy->client->dev,
> "PHY PLL locking failed\n");
>
> return ret;
>
Ok.
> By the way, isn't this initialization really needed whenever the PHY is
> powered on?
In Exynos5250, we do require to power on the phy before writing
anything into phy controller register space.
As you know,phy_ops have separate callbacks power_on and init .So I
just used those to separate these functionalities.
>
>
>> +}
>> +
>> +static struct phy_ops exynos_sata_phy_ops = {
>> + .init = exynos_sata_phy_init,
>> + .power_on = exynos_sata_phy_power_on,
>> + .power_off = exynos_sata_phy_power_off,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int exynos_sata_phy_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_sata_phy *sata_phy;
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct phy_provider *phy_provider;
>> + struct device_node *node;
>> + int ret = 0;
>> +
>> + sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
>> + if (!sata_phy)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + sata_phy->regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sata_phy->regs))
>> + return PTR_ERR(sata_phy->regs);
>> +
>> + sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-phandle");
>
>
> pmureg is defined as (void __iomem *) in struct exynos_sata_phy, but
> syscon_regmap_lookup_by_phandle() returns (struct regmap *). Moreover it
> does not return NULL on error, but rather ERR_PTR(). Please correct this.
Ohh!my mistake.Will correct in next version.
>
>
>> + if (!sata_phy->pmureg) {
>> + dev_err(dev, "syscon regmap lookup failed.\n");
>> + return PTR_ERR(sata_phy->pmureg);
>> + }
>> +
>> + node = of_parse_phandle(dev->of_node,
>> + "samsung,exynos-sataphy-i2c-phandle", 0);
>> + if (!node)
>> + return -ENODEV;
>
>
> An error here means that a required DT property was not specified or was
> specified incorrectly. IMHO -EINVAL would be better here.
ok
>
>
>> +
>> + sata_phy->client = of_find_i2c_device_by_node(node);
>> + if (!sata_phy->client)
>> + return -EPROBE_DEFER;
>> +
>> + dev_set_drvdata(dev, sata_phy);
>> +
>> + sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
>> + if (IS_ERR(sata_phy->phyclk)) {
>> + dev_err(dev, "failed to get clk for PHY\n");
>> + return PTR_ERR(sata_phy->phyclk);
>> + }
>> +
>> + ret = clk_prepare_enable(sata_phy->phyclk);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable source clk\n");
>> + return ret;
>> + }
>> +
>> + sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
>> + if (IS_ERR(sata_phy->phy)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + dev_err(dev, "failed to create PHY\n");
>> + return PTR_ERR(sata_phy->phy);
>> + }
>> +
>> + phy_set_drvdata(sata_phy->phy, sata_phy);
>> +
>> + phy_provider = devm_of_phy_provider_register(dev,
>> + of_phy_simple_xlate);
>> + if (IS_ERR(phy_provider)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + return PTR_ERR(phy_provider);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id exynos_sata_phy_of_match[] = {
>> + { .compatible = "samsung,exynos5250-sata-phy" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
>> +
>> +static struct platform_driver exynos_sata_phy_driver = {
>> + .probe = exynos_sata_phy_probe,
>
>
> If this driver can be compiled as module, don't you also need remove?
Will add in next version.
>
> Best regards,
> Tomasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
@ 2014-01-17 11:50 ` Yuvaraj Kumar
0 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar @ 2014-01-17 11:50 UTC (permalink / raw)
To: linux-arm-kernel
Sorry for the late reply.
On Fri, Jan 10, 2014 at 8:35 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yuvaraj,
>
> In general this version looks pretty good, but I have some questions inline.
Please find my comments inline.
>
> On 10.01.2014 08:00, Yuvaraj Kumar C D wrote:
> [snip]
>
>> diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c
>> b/drivers/phy/phy-exynos5250-sata-i2c.c
>> new file mode 100644
>> index 0000000..206e337
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata-i2c.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
>> + * Author:
>> + * Yuvaraj C D <yuvaraj.cd@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License as published by
>> the
>> + * Free Software Foundation; either version 2 of the License, or (at
>> your
>> + * option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +static int exynos_sata_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *i2c_id)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id sataphy_i2c_device_match[] = {
>> + { "exynos-sataphy-i2c", 0 },
>> +};
>> +
>> +static struct i2c_driver sataphy_i2c_driver = {
>> + .probe = exynos_sata_i2c_probe,
>> + .id_table = sataphy_i2c_device_match,
>> + .driver = {
>> + .name = "exynos-sataphy-i2c",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +static int __init exynos5250_phy_i2c_init(void)
>> +{
>> + return i2c_add_driver(&sataphy_i2c_driver);
>> +}
>> +module_init(exynos5250_phy_i2c_init);
>
>
> Hmm, is this driver even necessary now?
Yes.It will not necessary now with of_find_i2c_device_by_node.I will
remove phy-exynos5250-sata-i2c.c
>
> Wolfram, would it be possible to use an i2c_client without a driver bound to
> it?
>
>
>> diff --git a/drivers/phy/phy-exynos5250-sata.c
>> b/drivers/phy/phy-exynos5250-sata.c
>> new file mode 100644
>> index 0000000..6e5ff8d
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * Samsung SATA SerDes(PHY) driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Authors: Girish K S <ks.giri@samsung.com>
>> + * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> + *
>> + * 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/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define EXYNOS5_SATA_RESET 0x4
>> +#define RESET_CMN_RST_N (1 << 1)
>> +#define LINK_RESET 0xF0000
>
>
> nit: Lowercase is preferred in hexadecimal notation.
> + all other occurrences in this file.
Ok.
>
>
>> +#define EXYNOS5_SATA_MODE0 0x10
>> +#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
>> +#define SATA_SPD_GEN3 (2 << 0)
>> +#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 EXYNOS_SATA_PHY_EN (1 << 0)
>> +#define SATAPHY_CONTROL_OFFSET 0x0724
>> +
>> +struct exynos_sata_phy {
>> + struct phy *phy;
>> + struct clk *phyclk;
>> + void __iomem *regs;
>> + void __iomem *pmureg;
>> + struct i2c_client *client;
>> +};
>> +
>> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
>> checkbit,
>> + u32 status)
>> +{
>> + unsigned long timeout = jiffies + usecs_to_jiffies(1000);
>
>
> nit: It would be better to define the timeout using a macro to not use magic
> numbers.
Ok
>
>
>> +
>> + while (time_before(jiffies, timeout)) {
>> + if ((readl(base + reg) & checkbit) == status)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int exynos_sata_phy_power_on(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits can return an error. Wouldn't it be better to return it
> as return value of this function instead of returning 0 all the time? As a
> side effect, this would make the function smaller by two lines.
Yes.Will incorporate in next version.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_power_off(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
>
>
> Same here.
Yes.Will incorporate in next version.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_init(struct phy *phy)
>> +{
>> + u32 val = 0;
>> + int ret = 0;
>> + u8 buf[] = { 0x3A, 0x0B };
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits returns an error code.
Yes.Will incorporate in next version.
>
>
>> +
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= 0xFF;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= LINK_RESET;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val &= ~PHCTRLM_REF_RATE;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + /* High speed enable for Gen3 */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val |= PHCTRLM_HIGH_SPEED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> + val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
>> + val |= SATA_SPD_GEN3;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
>> +
>> + ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
>> + if (ret < 0)
>> + return -ENXIO;
>
>
> Wouldn't it be better to return the same error code as i2c_master_send
> returned?
Yes.Will incorporate in next version.
>
>
>> +
>> + /* release cmu reset */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val &= ~RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + return (wait_for_reg_status(sata_phy->regs,
>> EXYNOS5_SATA_PHSATA_STATM,
>> + PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
>> +
>
>
> nit: Stray blank line.
Ok.will remove
>
> Also it might be more readable after making wait_for_reg_status() return an
> integer error code (0 and e.g. -EFAULT) and rewriting the last line to:
>
> ret = wait_for_reg_status(sata_phy->regs,
> EXYNOS5_SATA_PHSATA_STATM,
> PHSTATM_PLL_LOCKED, 1);
> if (ret < 0)
> dev_err(&sata_phy->client->dev,
> "PHY PLL locking failed\n");
>
> return ret;
>
Ok.
> By the way, isn't this initialization really needed whenever the PHY is
> powered on?
In Exynos5250, we do require to power on the phy before writing
anything into phy controller register space.
As you know,phy_ops have separate callbacks power_on and init .So I
just used those to separate these functionalities.
>
>
>> +}
>> +
>> +static struct phy_ops exynos_sata_phy_ops = {
>> + .init = exynos_sata_phy_init,
>> + .power_on = exynos_sata_phy_power_on,
>> + .power_off = exynos_sata_phy_power_off,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int exynos_sata_phy_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_sata_phy *sata_phy;
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct phy_provider *phy_provider;
>> + struct device_node *node;
>> + int ret = 0;
>> +
>> + sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
>> + if (!sata_phy)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + sata_phy->regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sata_phy->regs))
>> + return PTR_ERR(sata_phy->regs);
>> +
>> + sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-phandle");
>
>
> pmureg is defined as (void __iomem *) in struct exynos_sata_phy, but
> syscon_regmap_lookup_by_phandle() returns (struct regmap *). Moreover it
> does not return NULL on error, but rather ERR_PTR(). Please correct this.
Ohh!my mistake.Will correct in next version.
>
>
>> + if (!sata_phy->pmureg) {
>> + dev_err(dev, "syscon regmap lookup failed.\n");
>> + return PTR_ERR(sata_phy->pmureg);
>> + }
>> +
>> + node = of_parse_phandle(dev->of_node,
>> + "samsung,exynos-sataphy-i2c-phandle", 0);
>> + if (!node)
>> + return -ENODEV;
>
>
> An error here means that a required DT property was not specified or was
> specified incorrectly. IMHO -EINVAL would be better here.
ok
>
>
>> +
>> + sata_phy->client = of_find_i2c_device_by_node(node);
>> + if (!sata_phy->client)
>> + return -EPROBE_DEFER;
>> +
>> + dev_set_drvdata(dev, sata_phy);
>> +
>> + sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
>> + if (IS_ERR(sata_phy->phyclk)) {
>> + dev_err(dev, "failed to get clk for PHY\n");
>> + return PTR_ERR(sata_phy->phyclk);
>> + }
>> +
>> + ret = clk_prepare_enable(sata_phy->phyclk);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable source clk\n");
>> + return ret;
>> + }
>> +
>> + sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
>> + if (IS_ERR(sata_phy->phy)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + dev_err(dev, "failed to create PHY\n");
>> + return PTR_ERR(sata_phy->phy);
>> + }
>> +
>> + phy_set_drvdata(sata_phy->phy, sata_phy);
>> +
>> + phy_provider = devm_of_phy_provider_register(dev,
>> + of_phy_simple_xlate);
>> + if (IS_ERR(phy_provider)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + return PTR_ERR(phy_provider);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id exynos_sata_phy_of_match[] = {
>> + { .compatible = "samsung,exynos5250-sata-phy" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
>> +
>> +static struct platform_driver exynos_sata_phy_driver = {
>> + .probe = exynos_sata_phy_probe,
>
>
> If this driver can be compiled as module, don't you also need remove?
Will add in next version.
>
> Best regards,
> Tomasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
2014-01-10 15:05 ` Tomasz Figa
(?)
@ 2014-01-27 13:38 ` Yuvaraj Kumar
-1 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar @ 2014-01-27 13:38 UTC (permalink / raw)
To: Tomasz Figa
Cc: Kishon Vijay Abraham I, kgene.kim, linux-kernel,
linux-arm-kernel, devicetree, linux-doc, Wolfram Sang,
Mark Rutland, Jingoo Han, Bartlomiej Zolnierkiewicz, sunil joshi,
Stephen Warren, Grant Likely, Christoffer Dall, linux-ide,
Yuvaraj Kumar C D, Girish K S, Vasanth Ananthan
On Fri, Jan 10, 2014 at 8:35 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yuvaraj,
>
> In general this version looks pretty good, but I have some questions inline.
>
> On 10.01.2014 08:00, Yuvaraj Kumar C D wrote:
> [snip]
>
>> diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c
>> b/drivers/phy/phy-exynos5250-sata-i2c.c
>> new file mode 100644
>> index 0000000..206e337
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata-i2c.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
>> + * Author:
>> + * Yuvaraj C D <yuvaraj.cd@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License as published by
>> the
>> + * Free Software Foundation; either version 2 of the License, or (at
>> your
>> + * option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +static int exynos_sata_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *i2c_id)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id sataphy_i2c_device_match[] = {
>> + { "exynos-sataphy-i2c", 0 },
>> +};
>> +
>> +static struct i2c_driver sataphy_i2c_driver = {
>> + .probe = exynos_sata_i2c_probe,
>> + .id_table = sataphy_i2c_device_match,
>> + .driver = {
>> + .name = "exynos-sataphy-i2c",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +static int __init exynos5250_phy_i2c_init(void)
>> +{
>> + return i2c_add_driver(&sataphy_i2c_driver);
>> +}
>> +module_init(exynos5250_phy_i2c_init);
>
>
> Hmm, is this driver even necessary now?
>
> Wolfram, would it be possible to use an i2c_client without a driver bound to
> it?
>
>
>> diff --git a/drivers/phy/phy-exynos5250-sata.c
>> b/drivers/phy/phy-exynos5250-sata.c
>> new file mode 100644
>> index 0000000..6e5ff8d
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * Samsung SATA SerDes(PHY) driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Authors: Girish K S <ks.giri@samsung.com>
>> + * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> + *
>> + * 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/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define EXYNOS5_SATA_RESET 0x4
>> +#define RESET_CMN_RST_N (1 << 1)
>> +#define LINK_RESET 0xF0000
>
>
> nit: Lowercase is preferred in hexadecimal notation.
> + all other occurrences in this file.
>
>
>> +#define EXYNOS5_SATA_MODE0 0x10
>> +#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
>> +#define SATA_SPD_GEN3 (2 << 0)
>> +#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 EXYNOS_SATA_PHY_EN (1 << 0)
>> +#define SATAPHY_CONTROL_OFFSET 0x0724
>> +
>> +struct exynos_sata_phy {
>> + struct phy *phy;
>> + struct clk *phyclk;
>> + void __iomem *regs;
>> + void __iomem *pmureg;
>> + struct i2c_client *client;
>> +};
>> +
>> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
>> checkbit,
>> + u32 status)
>> +{
>> + unsigned long timeout = jiffies + usecs_to_jiffies(1000);
>
>
> nit: It would be better to define the timeout using a macro to not use magic
> numbers.
>
>
>> +
>> + while (time_before(jiffies, timeout)) {
>> + if ((readl(base + reg) & checkbit) == status)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int exynos_sata_phy_power_on(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits can return an error. Wouldn't it be better to return it
> as return value of this function instead of returning 0 all the time? As a
> side effect, this would make the function smaller by two lines.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_power_off(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
>
>
> Same here.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_init(struct phy *phy)
>> +{
>> + u32 val = 0;
>> + int ret = 0;
>> + u8 buf[] = { 0x3A, 0x0B };
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits returns an error code.
>
>
>> +
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= 0xFF;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= LINK_RESET;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val &= ~PHCTRLM_REF_RATE;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + /* High speed enable for Gen3 */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val |= PHCTRLM_HIGH_SPEED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> + val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
>> + val |= SATA_SPD_GEN3;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
>> +
>> + ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
>> + if (ret < 0)
>> + return -ENXIO;
>
>
> Wouldn't it be better to return the same error code as i2c_master_send
> returned?
>
>
>> +
>> + /* release cmu reset */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val &= ~RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + return (wait_for_reg_status(sata_phy->regs,
>> EXYNOS5_SATA_PHSATA_STATM,
>> + PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
>> +
>
>
> nit: Stray blank line.
>
> Also it might be more readable after making wait_for_reg_status() return an
> integer error code (0 and e.g. -EFAULT) and rewriting the last line to:
>
> ret = wait_for_reg_status(sata_phy->regs,
> EXYNOS5_SATA_PHSATA_STATM,
> PHSTATM_PLL_LOCKED, 1);
> if (ret < 0)
> dev_err(&sata_phy->client->dev,
> "PHY PLL locking failed\n");
>
> return ret;
>
> By the way, isn't this initialization really needed whenever the PHY is
> powered on?
>
>
>> +}
>> +
>> +static struct phy_ops exynos_sata_phy_ops = {
>> + .init = exynos_sata_phy_init,
>> + .power_on = exynos_sata_phy_power_on,
>> + .power_off = exynos_sata_phy_power_off,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int exynos_sata_phy_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_sata_phy *sata_phy;
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct phy_provider *phy_provider;
>> + struct device_node *node;
>> + int ret = 0;
>> +
>> + sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
>> + if (!sata_phy)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + sata_phy->regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sata_phy->regs))
>> + return PTR_ERR(sata_phy->regs);
>> +
>> + sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-phandle");
>
>
> pmureg is defined as (void __iomem *) in struct exynos_sata_phy, but
> syscon_regmap_lookup_by_phandle() returns (struct regmap *). Moreover it
> does not return NULL on error, but rather ERR_PTR(). Please correct this.
>
>
>> + if (!sata_phy->pmureg) {
>> + dev_err(dev, "syscon regmap lookup failed.\n");
>> + return PTR_ERR(sata_phy->pmureg);
>> + }
>> +
>> + node = of_parse_phandle(dev->of_node,
>> + "samsung,exynos-sataphy-i2c-phandle", 0);
>> + if (!node)
>> + return -ENODEV;
>
>
> An error here means that a required DT property was not specified or was
> specified incorrectly. IMHO -EINVAL would be better here.
>
>
>> +
>> + sata_phy->client = of_find_i2c_device_by_node(node);
>> + if (!sata_phy->client)
>> + return -EPROBE_DEFER;
>> +
>> + dev_set_drvdata(dev, sata_phy);
>> +
>> + sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
>> + if (IS_ERR(sata_phy->phyclk)) {
>> + dev_err(dev, "failed to get clk for PHY\n");
>> + return PTR_ERR(sata_phy->phyclk);
>> + }
>> +
>> + ret = clk_prepare_enable(sata_phy->phyclk);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable source clk\n");
>> + return ret;
>> + }
>> +
>> + sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
>> + if (IS_ERR(sata_phy->phy)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + dev_err(dev, "failed to create PHY\n");
>> + return PTR_ERR(sata_phy->phy);
>> + }
>> +
>> + phy_set_drvdata(sata_phy->phy, sata_phy);
>> +
>> + phy_provider = devm_of_phy_provider_register(dev,
>> + of_phy_simple_xlate);
>> + if (IS_ERR(phy_provider)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + return PTR_ERR(phy_provider);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id exynos_sata_phy_of_match[] = {
>> + { .compatible = "samsung,exynos5250-sata-phy" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
>> +
>> +static struct platform_driver exynos_sata_phy_driver = {
>> + .probe = exynos_sata_phy_probe,
>
>
> If this driver can be compiled as module, don't you also need remove?
I tried this and found as such there is no resources hold by this
driver to handle in remove case.
This driver is used by ahci_platform driver and when load
phy-exynos5250-sata,driver usage count is 1.
so if we unload ahci_platform driver first and then
phy-exynos5250-sata.ko it cleanely unload this module.
>
> Best regards,
> Tomasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
@ 2014-01-27 13:38 ` Yuvaraj Kumar
0 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar @ 2014-01-27 13:38 UTC (permalink / raw)
To: Tomasz Figa
Cc: Kishon Vijay Abraham I, kgene.kim, linux-kernel,
linux-arm-kernel, devicetree, linux-doc, Wolfram Sang,
Mark Rutland, Jingoo Han, Bartlomiej Zolnierkiewicz, sunil joshi,
Stephen Warren, Grant Likely, Christoffer Dall, linux-ide,
Yuvaraj Kumar C D, Girish K S, Vasanth Ananthan
On Fri, Jan 10, 2014 at 8:35 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yuvaraj,
>
> In general this version looks pretty good, but I have some questions inline.
>
> On 10.01.2014 08:00, Yuvaraj Kumar C D wrote:
> [snip]
>
>> diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c
>> b/drivers/phy/phy-exynos5250-sata-i2c.c
>> new file mode 100644
>> index 0000000..206e337
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata-i2c.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
>> + * Author:
>> + * Yuvaraj C D <yuvaraj.cd@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License as published by
>> the
>> + * Free Software Foundation; either version 2 of the License, or (at
>> your
>> + * option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +static int exynos_sata_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *i2c_id)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id sataphy_i2c_device_match[] = {
>> + { "exynos-sataphy-i2c", 0 },
>> +};
>> +
>> +static struct i2c_driver sataphy_i2c_driver = {
>> + .probe = exynos_sata_i2c_probe,
>> + .id_table = sataphy_i2c_device_match,
>> + .driver = {
>> + .name = "exynos-sataphy-i2c",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +static int __init exynos5250_phy_i2c_init(void)
>> +{
>> + return i2c_add_driver(&sataphy_i2c_driver);
>> +}
>> +module_init(exynos5250_phy_i2c_init);
>
>
> Hmm, is this driver even necessary now?
>
> Wolfram, would it be possible to use an i2c_client without a driver bound to
> it?
>
>
>> diff --git a/drivers/phy/phy-exynos5250-sata.c
>> b/drivers/phy/phy-exynos5250-sata.c
>> new file mode 100644
>> index 0000000..6e5ff8d
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * Samsung SATA SerDes(PHY) driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Authors: Girish K S <ks.giri@samsung.com>
>> + * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> + *
>> + * 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/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define EXYNOS5_SATA_RESET 0x4
>> +#define RESET_CMN_RST_N (1 << 1)
>> +#define LINK_RESET 0xF0000
>
>
> nit: Lowercase is preferred in hexadecimal notation.
> + all other occurrences in this file.
>
>
>> +#define EXYNOS5_SATA_MODE0 0x10
>> +#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
>> +#define SATA_SPD_GEN3 (2 << 0)
>> +#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 EXYNOS_SATA_PHY_EN (1 << 0)
>> +#define SATAPHY_CONTROL_OFFSET 0x0724
>> +
>> +struct exynos_sata_phy {
>> + struct phy *phy;
>> + struct clk *phyclk;
>> + void __iomem *regs;
>> + void __iomem *pmureg;
>> + struct i2c_client *client;
>> +};
>> +
>> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
>> checkbit,
>> + u32 status)
>> +{
>> + unsigned long timeout = jiffies + usecs_to_jiffies(1000);
>
>
> nit: It would be better to define the timeout using a macro to not use magic
> numbers.
>
>
>> +
>> + while (time_before(jiffies, timeout)) {
>> + if ((readl(base + reg) & checkbit) == status)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int exynos_sata_phy_power_on(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits can return an error. Wouldn't it be better to return it
> as return value of this function instead of returning 0 all the time? As a
> side effect, this would make the function smaller by two lines.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_power_off(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
>
>
> Same here.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_init(struct phy *phy)
>> +{
>> + u32 val = 0;
>> + int ret = 0;
>> + u8 buf[] = { 0x3A, 0x0B };
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits returns an error code.
>
>
>> +
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= 0xFF;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= LINK_RESET;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val &= ~PHCTRLM_REF_RATE;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + /* High speed enable for Gen3 */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val |= PHCTRLM_HIGH_SPEED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> + val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
>> + val |= SATA_SPD_GEN3;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
>> +
>> + ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
>> + if (ret < 0)
>> + return -ENXIO;
>
>
> Wouldn't it be better to return the same error code as i2c_master_send
> returned?
>
>
>> +
>> + /* release cmu reset */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val &= ~RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + return (wait_for_reg_status(sata_phy->regs,
>> EXYNOS5_SATA_PHSATA_STATM,
>> + PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
>> +
>
>
> nit: Stray blank line.
>
> Also it might be more readable after making wait_for_reg_status() return an
> integer error code (0 and e.g. -EFAULT) and rewriting the last line to:
>
> ret = wait_for_reg_status(sata_phy->regs,
> EXYNOS5_SATA_PHSATA_STATM,
> PHSTATM_PLL_LOCKED, 1);
> if (ret < 0)
> dev_err(&sata_phy->client->dev,
> "PHY PLL locking failed\n");
>
> return ret;
>
> By the way, isn't this initialization really needed whenever the PHY is
> powered on?
>
>
>> +}
>> +
>> +static struct phy_ops exynos_sata_phy_ops = {
>> + .init = exynos_sata_phy_init,
>> + .power_on = exynos_sata_phy_power_on,
>> + .power_off = exynos_sata_phy_power_off,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int exynos_sata_phy_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_sata_phy *sata_phy;
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct phy_provider *phy_provider;
>> + struct device_node *node;
>> + int ret = 0;
>> +
>> + sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
>> + if (!sata_phy)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + sata_phy->regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sata_phy->regs))
>> + return PTR_ERR(sata_phy->regs);
>> +
>> + sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-phandle");
>
>
> pmureg is defined as (void __iomem *) in struct exynos_sata_phy, but
> syscon_regmap_lookup_by_phandle() returns (struct regmap *). Moreover it
> does not return NULL on error, but rather ERR_PTR(). Please correct this.
>
>
>> + if (!sata_phy->pmureg) {
>> + dev_err(dev, "syscon regmap lookup failed.\n");
>> + return PTR_ERR(sata_phy->pmureg);
>> + }
>> +
>> + node = of_parse_phandle(dev->of_node,
>> + "samsung,exynos-sataphy-i2c-phandle", 0);
>> + if (!node)
>> + return -ENODEV;
>
>
> An error here means that a required DT property was not specified or was
> specified incorrectly. IMHO -EINVAL would be better here.
>
>
>> +
>> + sata_phy->client = of_find_i2c_device_by_node(node);
>> + if (!sata_phy->client)
>> + return -EPROBE_DEFER;
>> +
>> + dev_set_drvdata(dev, sata_phy);
>> +
>> + sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
>> + if (IS_ERR(sata_phy->phyclk)) {
>> + dev_err(dev, "failed to get clk for PHY\n");
>> + return PTR_ERR(sata_phy->phyclk);
>> + }
>> +
>> + ret = clk_prepare_enable(sata_phy->phyclk);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable source clk\n");
>> + return ret;
>> + }
>> +
>> + sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
>> + if (IS_ERR(sata_phy->phy)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + dev_err(dev, "failed to create PHY\n");
>> + return PTR_ERR(sata_phy->phy);
>> + }
>> +
>> + phy_set_drvdata(sata_phy->phy, sata_phy);
>> +
>> + phy_provider = devm_of_phy_provider_register(dev,
>> + of_phy_simple_xlate);
>> + if (IS_ERR(phy_provider)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + return PTR_ERR(phy_provider);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id exynos_sata_phy_of_match[] = {
>> + { .compatible = "samsung,exynos5250-sata-phy" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
>> +
>> +static struct platform_driver exynos_sata_phy_driver = {
>> + .probe = exynos_sata_phy_probe,
>
>
> If this driver can be compiled as module, don't you also need remove?
I tried this and found as such there is no resources hold by this
driver to handle in remove case.
This driver is used by ahci_platform driver and when load
phy-exynos5250-sata,driver usage count is 1.
so if we unload ahci_platform driver first and then
phy-exynos5250-sata.ko it cleanely unload this module.
>
> Best regards,
> Tomasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver
@ 2014-01-27 13:38 ` Yuvaraj Kumar
0 siblings, 0 replies; 14+ messages in thread
From: Yuvaraj Kumar @ 2014-01-27 13:38 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jan 10, 2014 at 8:35 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Yuvaraj,
>
> In general this version looks pretty good, but I have some questions inline.
>
> On 10.01.2014 08:00, Yuvaraj Kumar C D wrote:
> [snip]
>
>> diff --git a/drivers/phy/phy-exynos5250-sata-i2c.c
>> b/drivers/phy/phy-exynos5250-sata-i2c.c
>> new file mode 100644
>> index 0000000..206e337
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata-i2c.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
>> + * Author:
>> + * Yuvaraj C D <yuvaraj.cd@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> it
>> + * under the terms of the GNU General Public License as published by
>> the
>> + * Free Software Foundation; either version 2 of the License, or (at
>> your
>> + * option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +static int exynos_sata_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *i2c_id)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id sataphy_i2c_device_match[] = {
>> + { "exynos-sataphy-i2c", 0 },
>> +};
>> +
>> +static struct i2c_driver sataphy_i2c_driver = {
>> + .probe = exynos_sata_i2c_probe,
>> + .id_table = sataphy_i2c_device_match,
>> + .driver = {
>> + .name = "exynos-sataphy-i2c",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +static int __init exynos5250_phy_i2c_init(void)
>> +{
>> + return i2c_add_driver(&sataphy_i2c_driver);
>> +}
>> +module_init(exynos5250_phy_i2c_init);
>
>
> Hmm, is this driver even necessary now?
>
> Wolfram, would it be possible to use an i2c_client without a driver bound to
> it?
>
>
>> diff --git a/drivers/phy/phy-exynos5250-sata.c
>> b/drivers/phy/phy-exynos5250-sata.c
>> new file mode 100644
>> index 0000000..6e5ff8d
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5250-sata.c
>> @@ -0,0 +1,238 @@
>> +/*
>> + * Samsung SATA SerDes(PHY) driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Authors: Girish K S <ks.giri@samsung.com>
>> + * Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> + *
>> + * 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/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define EXYNOS5_SATA_RESET 0x4
>> +#define RESET_CMN_RST_N (1 << 1)
>> +#define LINK_RESET 0xF0000
>
>
> nit: Lowercase is preferred in hexadecimal notation.
> + all other occurrences in this file.
>
>
>> +#define EXYNOS5_SATA_MODE0 0x10
>> +#define EXYNOS5_SATAPHY_PMU_ENABLE (1 << 0)
>> +#define SATA_SPD_GEN3 (2 << 0)
>> +#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 EXYNOS_SATA_PHY_EN (1 << 0)
>> +#define SATAPHY_CONTROL_OFFSET 0x0724
>> +
>> +struct exynos_sata_phy {
>> + struct phy *phy;
>> + struct clk *phyclk;
>> + void __iomem *regs;
>> + void __iomem *pmureg;
>> + struct i2c_client *client;
>> +};
>> +
>> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
>> checkbit,
>> + u32 status)
>> +{
>> + unsigned long timeout = jiffies + usecs_to_jiffies(1000);
>
>
> nit: It would be better to define the timeout using a macro to not use magic
> numbers.
>
>
>> +
>> + while (time_before(jiffies, timeout)) {
>> + if ((readl(base + reg) & checkbit) == status)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int exynos_sata_phy_power_on(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits can return an error. Wouldn't it be better to return it
> as return value of this function instead of returning 0 all the time? As a
> side effect, this would make the function smaller by two lines.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_power_off(struct phy *phy)
>> +{
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, ~EXYNOS_SATA_PHY_EN);
>
>
> Same here.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_sata_phy_init(struct phy *phy)
>> +{
>> + u32 val = 0;
>> + int ret = 0;
>> + u8 buf[] = { 0x3A, 0x0B };
>> + struct exynos_sata_phy *sata_phy = phy_get_drvdata(phy);
>> +
>> + regmap_update_bits(sata_phy->pmureg, SATAPHY_CONTROL_OFFSET,
>> + EXYNOS5_SATAPHY_PMU_ENABLE, EXYNOS_SATA_PHY_EN);
>
>
> regmap_update_bits returns an error code.
>
>
>> +
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= 0xFF;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= LINK_RESET;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val &= ~PHCTRLM_REF_RATE;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + /* High speed enable for Gen3 */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> + val |= PHCTRLM_HIGH_SPEED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_PHSATA_CTRLM);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> + val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_CTRL0);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_MODE0);
>> + val |= SATA_SPD_GEN3;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_MODE0);
>> +
>> + ret = i2c_master_send(sata_phy->client, buf, sizeof(buf));
>> + if (ret < 0)
>> + return -ENXIO;
>
>
> Wouldn't it be better to return the same error code as i2c_master_send
> returned?
>
>
>> +
>> + /* release cmu reset */
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val &= ~RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + val = readl(sata_phy->regs + EXYNOS5_SATA_RESET);
>> + val |= RESET_CMN_RST_N;
>> + writel(val, sata_phy->regs + EXYNOS5_SATA_RESET);
>> +
>> + return (wait_for_reg_status(sata_phy->regs,
>> EXYNOS5_SATA_PHSATA_STATM,
>> + PHSTATM_PLL_LOCKED, 1)) ? 0 : -EINVAL;
>> +
>
>
> nit: Stray blank line.
>
> Also it might be more readable after making wait_for_reg_status() return an
> integer error code (0 and e.g. -EFAULT) and rewriting the last line to:
>
> ret = wait_for_reg_status(sata_phy->regs,
> EXYNOS5_SATA_PHSATA_STATM,
> PHSTATM_PLL_LOCKED, 1);
> if (ret < 0)
> dev_err(&sata_phy->client->dev,
> "PHY PLL locking failed\n");
>
> return ret;
>
> By the way, isn't this initialization really needed whenever the PHY is
> powered on?
>
>
>> +}
>> +
>> +static struct phy_ops exynos_sata_phy_ops = {
>> + .init = exynos_sata_phy_init,
>> + .power_on = exynos_sata_phy_power_on,
>> + .power_off = exynos_sata_phy_power_off,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int exynos_sata_phy_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_sata_phy *sata_phy;
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct phy_provider *phy_provider;
>> + struct device_node *node;
>> + int ret = 0;
>> +
>> + sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
>> + if (!sata_phy)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + sata_phy->regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sata_phy->regs))
>> + return PTR_ERR(sata_phy->regs);
>> +
>> + sata_phy->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "samsung,syscon-phandle");
>
>
> pmureg is defined as (void __iomem *) in struct exynos_sata_phy, but
> syscon_regmap_lookup_by_phandle() returns (struct regmap *). Moreover it
> does not return NULL on error, but rather ERR_PTR(). Please correct this.
>
>
>> + if (!sata_phy->pmureg) {
>> + dev_err(dev, "syscon regmap lookup failed.\n");
>> + return PTR_ERR(sata_phy->pmureg);
>> + }
>> +
>> + node = of_parse_phandle(dev->of_node,
>> + "samsung,exynos-sataphy-i2c-phandle", 0);
>> + if (!node)
>> + return -ENODEV;
>
>
> An error here means that a required DT property was not specified or was
> specified incorrectly. IMHO -EINVAL would be better here.
>
>
>> +
>> + sata_phy->client = of_find_i2c_device_by_node(node);
>> + if (!sata_phy->client)
>> + return -EPROBE_DEFER;
>> +
>> + dev_set_drvdata(dev, sata_phy);
>> +
>> + sata_phy->phyclk = devm_clk_get(dev, "sata_phyctrl");
>> + if (IS_ERR(sata_phy->phyclk)) {
>> + dev_err(dev, "failed to get clk for PHY\n");
>> + return PTR_ERR(sata_phy->phyclk);
>> + }
>> +
>> + ret = clk_prepare_enable(sata_phy->phyclk);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable source clk\n");
>> + return ret;
>> + }
>> +
>> + sata_phy->phy = devm_phy_create(dev, &exynos_sata_phy_ops, NULL);
>> + if (IS_ERR(sata_phy->phy)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + dev_err(dev, "failed to create PHY\n");
>> + return PTR_ERR(sata_phy->phy);
>> + }
>> +
>> + phy_set_drvdata(sata_phy->phy, sata_phy);
>> +
>> + phy_provider = devm_of_phy_provider_register(dev,
>> + of_phy_simple_xlate);
>> + if (IS_ERR(phy_provider)) {
>> + clk_disable_unprepare(sata_phy->phyclk);
>> + return PTR_ERR(phy_provider);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id exynos_sata_phy_of_match[] = {
>> + { .compatible = "samsung,exynos5250-sata-phy" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_sata_phy_of_match);
>> +
>> +static struct platform_driver exynos_sata_phy_driver = {
>> + .probe = exynos_sata_phy_probe,
>
>
> If this driver can be compiled as module, don't you also need remove?
I tried this and found as such there is no resources hold by this
driver to handle in remove case.
This driver is used by ahci_platform driver and when load
phy-exynos5250-sata,driver usage count is 1.
so if we unload ahci_platform driver first and then
phy-exynos5250-sata.ko it cleanely unload this module.
>
> Best regards,
> Tomasz
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-01-27 13:38 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-10 7:00 [PATCH V6 0/2] Exynos5250 SATA Support Yuvaraj Kumar C D
2014-01-10 7:00 ` Yuvaraj Kumar C D
2014-01-10 7:00 ` [PATCH V6 1/2] PHY: Exynos: Add Exynos5250 SATA PHY driver Yuvaraj Kumar C D
2014-01-10 7:00 ` Yuvaraj Kumar C D
2014-01-10 15:05 ` Tomasz Figa
2014-01-10 15:05 ` Tomasz Figa
2014-01-17 11:50 ` Yuvaraj Kumar
2014-01-17 11:50 ` Yuvaraj Kumar
2014-01-17 11:50 ` Yuvaraj Kumar
2014-01-27 13:38 ` Yuvaraj Kumar
2014-01-27 13:38 ` Yuvaraj Kumar
2014-01-27 13:38 ` Yuvaraj Kumar
2014-01-10 7:00 ` [PATCH V6 2/2] ARM: dts: Enable ahci sata and sata phy Yuvaraj Kumar C D
2014-01-10 7:00 ` Yuvaraj Kumar C D
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.