[RFC PATCH 0/5] Generic PHY driver for Exynos SoCs MIPI CSI-2/DSIM DPHYs

[RFC PATCH 0/5] Generic PHY driver for Exynos SoCs MIPI CSI-2/DSIM DPHYs

[Sylwester Nawrocki]

Hi,

The following is a simple driver for the Samsung S5P/Exynos SoCs MIPI CSI-2
receiver and MIPI DSI transmitter DPHYs, using the generic PHY framework [1].
Previously the MIPI CSIS and MIPI DSIM used a platform callback to control
the PHY power enable and reset bits. The callback can be dropped now and
those drivers don't depend any more on any platform code.

Any comments are welcome.

Thanks,
Sylwester

[1] https://lkml.org/lkml/2013/6/13/97

Sylwester Nawrocki (5):
  phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  ARM: dts: Add MIPI PHY node to exynos4.dtsi
  video: exynos_dsi: Use generic PHY driver
  exynos4-is: Use generic MIPI CSIS PHY driver
  ARM: Samsung: Remove MIPI PHY setup code

 .../bindings/phy/exynos-video-mipi-phy.txt         |   16 ++
 arch/arm/boot/dts/exynos4.dtsi                     |   12 ++
 arch/arm/mach-exynos/include/mach/regs-pmu.h       |    5 -
 arch/arm/mach-s5pv210/include/mach/regs-clock.h    |    4 -
 arch/arm/plat-samsung/Makefile                     |    1 -
 arch/arm/plat-samsung/setup-mipiphy.c              |   60 -------
 drivers/media/platform/exynos4-is/mipi-csis.c      |   11 +-
 drivers/phy/Kconfig                                |   10 ++
 drivers/phy/Makefile                               |    3 +-
 drivers/phy/exynos_video_mipi_phy.c                |  166 ++++++++++++++++++++
 drivers/video/display/source-exynos_dsi.c          |   36 ++---
 include/linux/platform_data/mipi-csis.h            |    9 --
 include/video/exynos_dsi.h                         |    5 -
 13 files changed, 226 insertions(+), 112 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
 delete mode 100644 arch/arm/plat-samsung/setup-mipiphy.c
 create mode 100644 drivers/phy/exynos_video_mipi_phy.c

--
1.7.9.5

[RFC PATCH 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Sylwester Nawrocki]

Add a PHY provider driver for the Samsung S5P/Exynos SoC MIPI CSI-2
receiver and MIPI DSI transmitter DPHYs.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../bindings/phy/exynos-video-mipi-phy.txt         |   16 ++
 drivers/phy/Kconfig                                |   10 ++
 drivers/phy/Makefile                               |    3 +-
 drivers/phy/exynos_video_mipi_phy.c                |  166 ++++++++++++++++++++
 4 files changed, 194 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
 create mode 100644 drivers/phy/exynos_video_mipi_phy.c

diff --git a/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
new file mode 100644
index 0000000..32311c89
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
@@ -0,0 +1,16 @@
+Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY
+-------------------------------------------------
+
+Required properties:
+- compatible : "samsung,<soc_name>-video-phy", currently most SoCs can claim
+  compatibility with the S5PV210 MIPI CSIS/DSIM PHY and thus should use
+  "samsung,s5pv210-video-phy";
+- reg : offset and length of the MIPI DPHY register set;
+- #phy-cells : from the generic phy bindings, must be 1;
+
+For "samsung,s5pv210-video-phy" compatible DPHYs the second cell in the PHY
+specifier identifies the DPHY and its meaning is as follows:
+  0 - MIPI CSIS 0,
+  1 - MIPI DSIM 0,
+  2 - MIPI CSIS 1,
+  3 - MIPI DSIM 1.
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 0764a54..d234e99 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -11,3 +11,13 @@ menuconfig GENERIC_PHY
 	  devices present in the kernel. This layer will have the generic
 	  API by which phy drivers can create PHY using the phy framework and
 	  phy users can obtain reference to the PHY.
+
+if GENERIC_PHY
+
+config EXYNOS_VIDEO_MIPI_PHY
+	bool "S5P/EXYNOS MIPI CSI-2/DSI PHY driver"
+	depends on OF
+	help
+	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung
+	  S5P and EXYNOS SoCs.
+endif
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 9e9560f..b16f2c1 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -2,4 +2,5 @@
 # Makefile for the phy drivers.
 #
 
-obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
+obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+obj-$(CONFIG_EXYNOS_VIDEO_MIPI_PHY)	+= exynos_video_mipi_phy.o
diff --git a/drivers/phy/exynos_video_mipi_phy.c b/drivers/phy/exynos_video_mipi_phy.c
new file mode 100644
index 0000000..8d4976f
--- /dev/null
+++ b/drivers/phy/exynos_video_mipi_phy.c
@@ -0,0 +1,166 @@
+/*
+ * Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Sylwester Nawrocki <s.nawrocki@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/delay.h>
+#include <linux/io.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/spinlock.h>
+
+/* MIPI_PHYn_CONTROL register bit definitions */
+#define EXYNOS_MIPI_PHY_ENABLE		(1 << 0)
+#define EXYNOS_MIPI_PHY_SRESETN		(1 << 1)
+#define EXYNOS_MIPI_PHY_MRESETN		(1 << 2)
+#define EXYNOS_MIPI_PHY_RESET_MASK	(3 << 1)
+
+#define EXYNOS_MAX_VIDEO_PHYS		4
+
+struct exynos_video_phy {
+	spinlock_t slock;
+	struct phy *phys[EXYNOS_MAX_VIDEO_PHYS];
+	void __iomem *regs;
+};
+
+/*
+ * The @id argument specifies MIPI CSIS or DSIM PHY as follows:
+ *  0 - MIPI CSIS 0
+ *  1 - MIPI DSIM 0
+ *  2 - MIPI CSIS 1
+ *  3 - MIPI DSIM 1
+ */
+static int set_phy_state(struct exynos_video_phy *state,
+					unsigned int id, int on)
+{
+	void __iomem *addr = id < 2 ? state->regs : state->regs + 4;
+	unsigned long flags;
+	u32 reg, reset;
+
+	pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
+		 __func__, id, on, (u32)addr, (u32)state->regs);
+
+	if (WARN_ON(id > EXYNOS_MAX_VIDEO_PHYS))
+		return -EINVAL;
+
+	if (id & 1)
+		reset = EXYNOS_MIPI_PHY_MRESETN;
+	else
+		reset = EXYNOS_MIPI_PHY_SRESETN;
+
+	spin_lock_irqsave(&state->slock, flags);
+
+	reg = readl(addr);
+	if (on)
+		reg |= reset;
+	else
+		reg &= ~reset;
+	writel(reg, addr);
+
+	if (on)
+		reg |= EXYNOS_MIPI_PHY_ENABLE;
+	else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
+		reg &= ~EXYNOS_MIPI_PHY_ENABLE;
+
+	writel(reg, addr);
+
+	spin_unlock_irqrestore(&state->slock, flags);
+	return 0;
+}
+
+static int exynos_video_phy_power_on(struct phy *phy)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
+	return set_phy_state(state, phy->id, 1);
+}
+
+static int exynos_video_phy_power_off(struct phy *phy)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
+	return set_phy_state(state, phy->id, 0);
+}
+
+static struct phy *exynos_video_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(dev);
+
+	if (WARN_ON(args->args[0] > EXYNOS_MAX_VIDEO_PHYS))
+		return NULL;
+
+	return state->phys[args->args[0]];
+}
+
+static struct phy_ops exynos_video_phy_ops = {
+	.power_on	= exynos_video_phy_power_on,
+	.power_off	= exynos_video_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int exynos_video_phy_probe(struct platform_device *pdev)
+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource res;
+	struct phy_provider *phy_provider;
+	int ret, i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	ret = of_address_to_resource(dev->of_node, 0, &res);
+	if (ret < 0)
+		return ret;
+
+	state->regs = devm_ioremap_resource(dev, &res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);
+
+	phy_provider = devm_of_phy_provider_register(dev, THIS_MODULE,
+					    exynos_video_phy_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	for (i = 0; i < EXYNOS_MAX_VIDEO_PHYS; i++) {
+		state->phys[i] = devm_phy_create(dev, i, &exynos_video_phy_ops,
+									state);
+		if (IS_ERR(state->phys[i])) {
+			dev_err(dev, "failed to create PHY %d\n", i);
+			return PTR_ERR(state->phys[i]);
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id exynos_video_phy_of_match[] = {
+	{ .compatible = "samsung,s5pv210-video-phy" },
+	{ },
+};
+
+static struct platform_driver exynos_video_phy_driver = {
+	.probe	= exynos_video_phy_probe,
+	.driver = {
+		.of_match_table	= exynos_video_phy_of_match,
+		.name  = "exynos-video-phy",
+		.owner = THIS_MODULE,
+	}
+};
+module_platform_driver(exynos_video_phy_driver);
+
+MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC MIPI CSI-2/DSI DPHY driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
-- 
1.7.9.5

[RFC PATCH 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi

[Sylwester Nawrocki]

Add PHY provider node for the MIPI CSIS and MIPI DSIM PHYs.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4.dtsi |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index d505ece..4b7ce52 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -120,12 +120,20 @@
 		reg = <0x10010000 0x400>;
 	};
 
+	mipi_phy: video-phy {
+		compatible = "samsung,s5pv210-video-phy";
+		reg = <0x10020710 8>;
+		#phy-cells = <1>;
+	};
+
 	dsi_0: dsi@11C80000 {
 		compatible = "samsung,exynos4210-mipi-dsi";
 		reg = <0x11C80000 0x10000>;
 		interrupts = <0 79 0>;
 		samsung,phy-type = <0>;
 		samsung,power-domain = <&pd_lcd0>;
+		phys = <&mipi_phy 1>;
+		phy-names = "dsim";
 		clocks = <&clock 286>, <&clock 143>;
 		clock-names = "bus_clk", "pll_clk";
 		status = "disabled";
@@ -181,6 +189,8 @@
 			interrupts = <0 78 0>;
 			bus-width = <4>;
 			samsung,power-domain = <&pd_cam>;
+			phys = <&mipi_phy 0>;
+			phy-names = "csis";
 			status = "disabled";
 		};
 
@@ -190,6 +200,8 @@
 			interrupts = <0 80 0>;
 			bus-width = <2>;
 			samsung,power-domain = <&pd_cam>;
+			phys = <&mipi_phy 2>;
+			phy-names = "csis";
 			status = "disabled";
 		};
 	};
-- 
1.7.9.5

[RFC PATCH 3/5] video: exynos_dsi: Use generic PHY driver

[Sylwester Nawrocki]

Use the generic PHY API instead of the platform callback to control
the MIPI DSIM DPHY.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/video/display/source-exynos_dsi.c |   36 +++++++++--------------------
 include/video/exynos_dsi.h                |    5 ----
 2 files changed, 11 insertions(+), 30 deletions(-)

diff --git a/drivers/video/display/source-exynos_dsi.c b/drivers/video/display/source-exynos_dsi.c
index 145d57b..dfab790 100644
--- a/drivers/video/display/source-exynos_dsi.c
+++ b/drivers/video/display/source-exynos_dsi.c
@@ -24,6 +24,7 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
@@ -219,6 +220,7 @@ struct exynos_dsi {
 	bool enabled;
 
 	struct platform_device *pdev;
+	struct phy *phy;
 	struct device *dev;
 	struct resource *res;
 	struct clk *pll_clk;
@@ -816,6 +818,7 @@ again:
 
 static bool exynos_dsi_transfer_finish(struct exynos_dsi *dsi)
 {
+	static unsigned long j;
 	struct exynos_dsi_transfer *xfer;
 	unsigned long flags;
 	bool start = true;
@@ -824,7 +827,8 @@ static bool exynos_dsi_transfer_finish(struct exynos_dsi *dsi)
 
 	if (list_empty(&dsi->transfer_list)) {
 		spin_unlock_irqrestore(&dsi->transfer_lock, flags);
-		dev_warn(dsi->dev, "unexpected TX/RX interrupt\n");
+		if (printk_timed_ratelimit(&j, 500))
+			dev_warn(dsi->dev, "unexpected TX/RX interrupt\n");
 		return false;
 	}
 
@@ -994,8 +998,7 @@ static int exynos_dsi_enable(struct video_source *src)
 	clk_prepare_enable(dsi->bus_clk);
 	clk_prepare_enable(dsi->pll_clk);
 
-	if (dsi->pd->phy_enable)
-		dsi->pd->phy_enable(dsi->pdev, true);
+	phy_power_on(dsi->phy);
 
 	exynos_dsi_reset(dsi);
 	exynos_dsi_init_link(dsi);
@@ -1019,8 +1022,7 @@ static int exynos_dsi_disable(struct video_source *src)
 
 	exynos_dsi_disable_clock(dsi);
 
-	if (dsi->pd->phy_enable)
-		dsi->pd->phy_enable(dsi->pdev, false);
+	phy_power_off(dsi->phy);
 
 	clk_disable_unprepare(dsi->pll_clk);
 	clk_disable_unprepare(dsi->bus_clk);
@@ -1099,12 +1101,6 @@ static const struct dsi_video_source_ops exynos_dsi_ops = {
  * Device Tree
  */
 
-static int (* const of_phy_enables[])(struct platform_device *, bool) = {
-#ifdef CONFIG_S5P_SETUP_MIPIPHY
-	[0] = s5p_dsim_phy_enable,
-#endif
-};
-
 static struct exynos_dsi_platform_data *exynos_dsi_parse_dt(
 						struct platform_device *pdev)
 {
@@ -1112,7 +1108,6 @@ static struct exynos_dsi_platform_data *exynos_dsi_parse_dt(
 	struct exynos_dsi_platform_data *dsi_pd;
 	struct device *dev = &pdev->dev;
 	const __be32 *prop_data;
-	u32 val;
 
 	dsi_pd = kzalloc(sizeof(*dsi_pd), GFP_KERNEL);
 	if (!dsi_pd) {
@@ -1120,19 +1115,6 @@ static struct exynos_dsi_platform_data *exynos_dsi_parse_dt(
 		return NULL;
 	}
 
-	prop_data = of_get_property(node, "samsung,phy-type", NULL);
-	if (!prop_data) {
-		dev_err(dev, "failed to get phy-type property\n");
-		goto err_free_pd;
-	}
-
-	val = be32_to_cpu(*prop_data);
-	if (val >= ARRAY_SIZE(of_phy_enables) || !of_phy_enables[val]) {
-		dev_err(dev, "Invalid phy-type %u\n", val);
-		goto err_free_pd;
-	}
-	dsi_pd->phy_enable = of_phy_enables[val];
-
 	prop_data = of_get_property(node, "samsung,pll-stable-time", NULL);
 	if (!prop_data) {
 		dev_err(dev, "failed to get pll-stable-time property\n");
@@ -1254,6 +1236,10 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	dsi->phy = devm_phy_get(&pdev->dev, "dsim");
+	if (IS_ERR(dsi->phy))
+		return PTR_ERR(dsi->phy);
+
 	platform_set_drvdata(pdev, dsi);
 
 	dsi->irq = platform_get_irq(pdev, 0);
diff --git a/include/video/exynos_dsi.h b/include/video/exynos_dsi.h
index 95e1568..5c062c7 100644
--- a/include/video/exynos_dsi.h
+++ b/include/video/exynos_dsi.h
@@ -25,9 +25,6 @@
  */
 struct exynos_dsi_platform_data {
 	unsigned int enabled;
-
-	int (*phy_enable)(struct platform_device *pdev, bool on);
-
 	unsigned int pll_stable_time;
 	unsigned long pll_clk_rate;
 	unsigned long esc_clk_rate;
@@ -36,6 +33,4 @@ struct exynos_dsi_platform_data {
 	unsigned short rx_timeout;
 };
 
-int s5p_dsim_phy_enable(struct platform_device *pdev, bool on);
-
 #endif /* _EXYNOS_MIPI_DSIM_H */
-- 
1.7.9.5

[RFC PATCH 4/5] exynos4-is: Use generic MIPI CSIS PHY driver

[Sylwester Nawrocki]

Use the generic PHY API instead of the platform callback to control
the MIPI CSIS DPHY.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/exynos4-is/mipi-csis.c |   11 +++++++++--
 include/linux/platform_data/mipi-csis.h       |    9 ---------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index 0fe80e3..88d0d61 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -20,6 +20,7 @@
 #include <linux/memory.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_data/mipi-csis.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -184,6 +185,7 @@ struct csis_drvdata {
  * @sd: v4l2_subdev associated with CSIS device instance
  * @index: the hardware instance index
  * @pdev: CSIS platform device
+ * @phy: pointer to the CSIS generic PHY
  * @regs: mmaped I/O registers memory
  * @supplies: CSIS regulator supplies
  * @clock: CSIS clocks
@@ -207,6 +209,7 @@ struct csis_state {
 	struct v4l2_subdev sd;
 	u8 index;
 	struct platform_device *pdev;
+	struct phy *phy;
 	void __iomem *regs;
 	struct regulator_bulk_data supplies[CSIS_NUM_SUPPLIES];
 	struct clk *clock[NUM_CSIS_CLOCKS];
@@ -861,6 +864,10 @@ static int s5pcsis_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	state->phy = devm_phy_get(dev, "csis");
+	if (IS_ERR(state->phy))
+		return PTR_ERR(state->phy);
+
 	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	state->regs = devm_ioremap_resource(dev, mem_res);
 	if (IS_ERR(state->regs))
@@ -946,7 +953,7 @@ static int s5pcsis_pm_suspend(struct device *dev, bool runtime)
 	mutex_lock(&state->lock);
 	if (state->flags & ST_POWERED) {
 		s5pcsis_stop_stream(state);
-		ret = s5p_csis_phy_enable(state->index, false);
+		ret = phy_power_off(state->phy);
 		if (ret)
 			goto unlock;
 		ret = regulator_bulk_disable(CSIS_NUM_SUPPLIES,
@@ -982,7 +989,7 @@ static int s5pcsis_pm_resume(struct device *dev, bool runtime)
 					    state->supplies);
 		if (ret)
 			goto unlock;
-		ret = s5p_csis_phy_enable(state->index, true);
+		ret = phy_power_on(state->phy);
 		if (!ret) {
 			state->flags |= ST_POWERED;
 		} else {
diff --git a/include/linux/platform_data/mipi-csis.h b/include/linux/platform_data/mipi-csis.h
index bf34e17..c2fd902 100644
--- a/include/linux/platform_data/mipi-csis.h
+++ b/include/linux/platform_data/mipi-csis.h
@@ -25,13 +25,4 @@ struct s5p_platform_mipi_csis {
 	u8 hs_settle;
 };
 
-/**
- * s5p_csis_phy_enable - global MIPI-CSI receiver D-PHY control
- * @id:     MIPI-CSIS harware instance index (0...1)
- * @on:     true to enable D-PHY and deassert its reset
- *          false to disable D-PHY
- * @return: 0 on success, or negative error code on failure
- */
-int s5p_csis_phy_enable(int id, bool on);
-
 #endif /* __PLAT_SAMSUNG_MIPI_CSIS_H_ */
-- 
1.7.9.5

[RFC PATCH 5/5] ARM: Samsung: Remove MIPI PHY setup code

[Sylwester Nawrocki]

Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
DPHYs so we can remove now unused code at arch/arm/plat-samsung.
In case there is any board file for S5PV210 platforms using MIPI
CSIS/DSIM (not any upstream currently) it should use the generic
PHY API to bind the PHYs to respective PHY consumer drivers.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/include/mach/regs-pmu.h    |    5 --
 arch/arm/mach-s5pv210/include/mach/regs-clock.h |    4 --
 arch/arm/plat-samsung/Makefile                  |    1 -
 arch/arm/plat-samsung/setup-mipiphy.c           |   60 -----------------------
 4 files changed, 70 deletions(-)
 delete mode 100644 arch/arm/plat-samsung/setup-mipiphy.c

diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h
index cf40b86..3820e39 100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -44,11 +44,6 @@
 #define S5P_DAC_PHY_CONTROL			S5P_PMUREG(0x070C)
 #define S5P_DAC_PHY_ENABLE			(1 << 0)
 
-#define S5P_MIPI_DPHY_CONTROL(n)		S5P_PMUREG(0x0710 + (n) * 4)
-#define S5P_MIPI_DPHY_ENABLE			(1 << 0)
-#define S5P_MIPI_DPHY_SRESETN			(1 << 1)
-#define S5P_MIPI_DPHY_MRESETN			(1 << 2)
-
 #define S5P_INFORM0				S5P_PMUREG(0x0800)
 #define S5P_INFORM1				S5P_PMUREG(0x0804)
 #define S5P_INFORM2				S5P_PMUREG(0x0808)
diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
index 032de66..e345584 100644
--- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
+++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
@@ -147,10 +147,6 @@
 #define S5P_HDMI_PHY_CONTROL	S5P_CLKREG(0xE804)
 #define S5P_USB_PHY_CONTROL	S5P_CLKREG(0xE80C)
 #define S5P_DAC_PHY_CONTROL	S5P_CLKREG(0xE810)
-#define S5P_MIPI_DPHY_CONTROL(x) S5P_CLKREG(0xE814)
-#define S5P_MIPI_DPHY_ENABLE	(1 << 0)
-#define S5P_MIPI_DPHY_SRESETN	(1 << 1)
-#define S5P_MIPI_DPHY_MRESETN	(1 << 2)
 
 #define S5P_INFORM0		S5P_CLKREG(0xF000)
 #define S5P_INFORM1		S5P_CLKREG(0xF004)
diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
index a23c460..5a15fae 100644
--- a/arch/arm/plat-samsung/Makefile
+++ b/arch/arm/plat-samsung/Makefile
@@ -41,7 +41,6 @@ obj-$(CONFIG_S5P_DEV_UART)	+= s5p-dev-uart.o
 obj-$(CONFIG_SAMSUNG_DEV_BACKLIGHT)	+= dev-backlight.o
 
 obj-$(CONFIG_S3C_SETUP_CAMIF)	+= setup-camif.o
-obj-$(CONFIG_S5P_SETUP_MIPIPHY)	+= setup-mipiphy.o
 
 # DMA support
 
diff --git a/arch/arm/plat-samsung/setup-mipiphy.c b/arch/arm/plat-samsung/setup-mipiphy.c
deleted file mode 100644
index 66df315..0000000
--- a/arch/arm/plat-samsung/setup-mipiphy.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
- * Copyright (C) 2011 Samsung Electronics Co., Ltd.
- *
- * S5P - Helper functions for MIPI-CSIS and MIPI-DSIM D-PHY control
- *
- * 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/export.h>
-#include <linux/kernel.h>
-#include <linux/platform_device.h>
-#include <linux/io.h>
-#include <linux/spinlock.h>
-#include <mach/regs-clock.h>
-
-static int __s5p_mipi_phy_control(int id, bool on, u32 reset)
-{
-	static DEFINE_SPINLOCK(lock);
-	void __iomem *addr;
-	unsigned long flags;
-	u32 cfg;
-
-	id = max(0, id);
-	if (id > 1)
-		return -EINVAL;
-
-	addr = S5P_MIPI_DPHY_CONTROL(id);
-
-	spin_lock_irqsave(&lock, flags);
-
-	cfg = __raw_readl(addr);
-	cfg = on ? (cfg | reset) : (cfg & ~reset);
-	__raw_writel(cfg, addr);
-
-	if (on) {
-		cfg |= S5P_MIPI_DPHY_ENABLE;
-	} else if (!(cfg & (S5P_MIPI_DPHY_SRESETN |
-			    S5P_MIPI_DPHY_MRESETN) & ~reset)) {
-		cfg &= ~S5P_MIPI_DPHY_ENABLE;
-	}
-
-	__raw_writel(cfg, addr);
-	spin_unlock_irqrestore(&lock, flags);
-
-	return 0;
-}
-
-int s5p_csis_phy_enable(int id, bool on)
-{
-	return __s5p_mipi_phy_control(id, on, S5P_MIPI_DPHY_SRESETN);
-}
-EXPORT_SYMBOL(s5p_csis_phy_enable);
-
-int s5p_dsim_phy_enable(struct platform_device *pdev, bool on)
-{
-	return __s5p_mipi_phy_control(pdev->id, on, S5P_MIPI_DPHY_MRESETN);
-}
-EXPORT_SYMBOL(s5p_dsim_phy_enable);
-- 
1.7.9.5

Re: [RFC PATCH 5/5] ARM: Samsung: Remove MIPI PHY setup code

[Kukjin Kim]

On 06/15/13 02:45, Sylwester Nawrocki wrote:
> Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
> DPHYs so we can remove now unused code at arch/arm/plat-samsung.

If so, sounds good :)

> In case there is any board file for S5PV210 platforms using MIPI
> CSIS/DSIM (not any upstream currently) it should use the generic
> PHY API to bind the PHYs to respective PHY consumer drivers.

To be honest, I didn't test this on boards but if the working is fine, 
please go ahead without RFC.

Thanks,
- Kukjin

Re: [RFC PATCH 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Tomasz Figa]

Hi Sylwester,

Looks good, but I added some nitpicks inline.

On Friday 14 of June 2013 19:45:47 Sylwester Nawrocki wrote:
> Add a PHY provider driver for the Samsung S5P/Exynos SoC MIPI CSI-2
> receiver and MIPI DSI transmitter DPHYs.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../bindings/phy/exynos-video-mipi-phy.txt         |   16 ++
>  drivers/phy/Kconfig                                |   10 ++
>  drivers/phy/Makefile                               |    3 +-
>  drivers/phy/exynos_video_mipi_phy.c                |  166
> ++++++++++++++++++++ 4 files changed, 194 insertions(+), 1 deletion(-)
>  create mode 100644
> Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt create
> mode 100644 drivers/phy/exynos_video_mipi_phy.c
> 
> diff --git
> a/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
> b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt new
> file mode 100644
> index 0000000..32311c89
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
> @@ -0,0 +1,16 @@
> +Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : "samsung,<soc_name>-video-phy", currently most SoCs can

I don't like this <soc_name> here. It sounds like any SoC name can be put 
here. IMHO just listing all supported compatible values should be enough.

> claim +  compatibility with the S5PV210 MIPI CSIS/DSIM PHY and thus
> should use +  "samsung,s5pv210-video-phy";
> +- reg : offset and length of the MIPI DPHY register set;
> +- #phy-cells : from the generic phy bindings, must be 1;
> +
> +For "samsung,s5pv210-video-phy" compatible DPHYs the second cell in the
> PHY +specifier identifies the DPHY and its meaning is as follows:
> +  0 - MIPI CSIS 0,
> +  1 - MIPI DSIM 0,
> +  2 - MIPI CSIS 1,
> +  3 - MIPI DSIM 1.
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 0764a54..d234e99 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -11,3 +11,13 @@ menuconfig GENERIC_PHY
>  	  devices present in the kernel. This layer will have the generic
>  	  API by which phy drivers can create PHY using the phy framework 
and
>  	  phy users can obtain reference to the PHY.
> +
> +if GENERIC_PHY
> +
> +config EXYNOS_VIDEO_MIPI_PHY
> +	bool "S5P/EXYNOS MIPI CSI-2/DSI PHY driver"
> +	depends on OF

Hmm. Is this driver designed only for OF-enabled boards?

> +	help
> +	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung
> +	  S5P and EXYNOS SoCs.
> +endif
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 9e9560f..b16f2c1 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -2,4 +2,5 @@
>  # Makefile for the phy drivers.
>  #
> 
> -obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
> +obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
> +obj-$(CONFIG_EXYNOS_VIDEO_MIPI_PHY)	+= exynos_video_mipi_phy.o
> diff --git a/drivers/phy/exynos_video_mipi_phy.c
> b/drivers/phy/exynos_video_mipi_phy.c new file mode 100644
> index 0000000..8d4976f
> --- /dev/null
> +++ b/drivers/phy/exynos_video_mipi_phy.c
> @@ -0,0 +1,166 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Sylwester Nawrocki <s.nawrocki@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/delay.h>
> +#include <linux/io.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/spinlock.h>
> +
> +/* MIPI_PHYn_CONTROL register bit definitions */
> +#define EXYNOS_MIPI_PHY_ENABLE		(1 << 0)
> +#define EXYNOS_MIPI_PHY_SRESETN		(1 << 1)
> +#define EXYNOS_MIPI_PHY_MRESETN		(1 << 2)
> +#define EXYNOS_MIPI_PHY_RESET_MASK	(3 << 1)
> +
> +#define EXYNOS_MAX_VIDEO_PHYS		4
> +
> +struct exynos_video_phy {
> +	spinlock_t slock;
> +	struct phy *phys[EXYNOS_MAX_VIDEO_PHYS];
> +	void __iomem *regs;
> +};
> +
> +/*
> + * The @id argument specifies MIPI CSIS or DSIM PHY as follows:
> + *  0 - MIPI CSIS 0
> + *  1 - MIPI DSIM 0
> + *  2 - MIPI CSIS 1
> + *  3 - MIPI DSIM 1
> + */
> +static int set_phy_state(struct exynos_video_phy *state,
> +					unsigned int id, int on)
> +{
> +	void __iomem *addr = id < 2 ? state->regs : state->regs + 4;

I don't find this statement too readable. What about:

	void __iomem *addr = state->regs;

and below:

	/* CSIS 1 and DSIM 1 PHYs have separate register */
	if (id >= 2)
		addr += 4;

> +	unsigned long flags;
> +	u32 reg, reset;
> +
> +	pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
> +		 __func__, id, on, (u32)addr, (u32)state->regs);
> +
> +	if (WARN_ON(id > EXYNOS_MAX_VIDEO_PHYS))
> +		return -EINVAL;
> +
> +	if (id & 1)

Nice trick ;), but not very readable. What about creating an enum of PHYs 
and using those defined values here:

	if (id == PHY_DSI0 || id == PHY_DSI1)

> +		reset = EXYNOS_MIPI_PHY_MRESETN;
> +	else
> +		reset = EXYNOS_MIPI_PHY_SRESETN;
> +
> +	spin_lock_irqsave(&state->slock, flags);
> +
> +	reg = readl(addr);
> +	if (on)
> +		reg |= reset;
> +	else
> +		reg &= ~reset;
> +	writel(reg, addr);
> +
> +	if (on)
> +		reg |= EXYNOS_MIPI_PHY_ENABLE;

I believe this is a kind of reference counting, but a comment here would 
be nice.

> +	else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
> +		reg &= ~EXYNOS_MIPI_PHY_ENABLE;
> +
> +	writel(reg, addr);
> +
> +	spin_unlock_irqrestore(&state->slock, flags);
> +	return 0;
> +}
> +
> +static int exynos_video_phy_power_on(struct phy *phy)
> +{
> +	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
> +	return set_phy_state(state, phy->id, 1);
> +}
> +
> +static int exynos_video_phy_power_off(struct phy *phy)
> +{
> +	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
> +	return set_phy_state(state, phy->id, 0);
> +}
> +
> +static struct phy *exynos_video_phy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct exynos_video_phy *state = dev_get_drvdata(dev);
> +
> +	if (WARN_ON(args->args[0] > EXYNOS_MAX_VIDEO_PHYS))
> +		return NULL;
> +
> +	return state->phys[args->args[0]];
> +}
> +
> +static struct phy_ops exynos_video_phy_ops = {
> +	.power_on	= exynos_video_phy_power_on,
> +	.power_off	= exynos_video_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int exynos_video_phy_probe(struct platform_device *pdev)
> +{
> +	struct exynos_video_phy *state;
> +	struct device *dev = &pdev->dev;
> +	struct resource res;
> +	struct phy_provider *phy_provider;
> +	int ret, i;
> +
> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	ret = of_address_to_resource(dev->of_node, 0, &res);
> +	if (ret < 0)
> +		return ret;

You can use platform_get_resource() here to get a resource generated for 
you by of_platform_populate().

In addition you don't need to check the pointer returned by 
platform_get_resource() because it is checked in devm_ioremap_resource().

> +
> +	state->regs = devm_ioremap_resource(dev, &res);
> +	if (IS_ERR(state->regs))
> +		return PTR_ERR(state->regs);
> +
> +	dev_set_drvdata(dev, state);
> +
> +	phy_provider = devm_of_phy_provider_register(dev, THIS_MODULE,
> +					    exynos_video_phy_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	for (i = 0; i < EXYNOS_MAX_VIDEO_PHYS; i++) {
> +		state->phys[i] = devm_phy_create(dev, i, 
&exynos_video_phy_ops,
> +									
state);
> +		if (IS_ERR(state->phys[i])) {
> +			dev_err(dev, "failed to create PHY %d\n", i);
> +			return PTR_ERR(state->phys[i]);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id exynos_video_phy_of_match[] = {
> +	{ .compatible = "samsung,s5pv210-video-phy" },
> +	{ },
> +};

IMHO a MODULE_DEVICE_TABLE should be added here.

Best regards,
Tomasz

> +
> +static struct platform_driver exynos_video_phy_driver = {
> +	.probe	= exynos_video_phy_probe,
> +	.driver = {
> +		.of_match_table	= exynos_video_phy_of_match,
> +		.name  = "exynos-video-phy",
> +		.owner = THIS_MODULE,
> +	}
> +};
> +module_platform_driver(exynos_video_phy_driver);
> +
> +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC MIPI CSI-2/DSI DPHY
> driver"); +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");

Re: [RFC PATCH 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi

[Tomasz Figa]

On Friday 14 of June 2013 19:45:48 Sylwester Nawrocki wrote:
> Add PHY provider node for the MIPI CSIS and MIPI DSIM PHYs.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4.dtsi |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4.dtsi
> b/arch/arm/boot/dts/exynos4.dtsi index d505ece..4b7ce52 100644
> --- a/arch/arm/boot/dts/exynos4.dtsi
> +++ b/arch/arm/boot/dts/exynos4.dtsi
> @@ -120,12 +120,20 @@
>  		reg = <0x10010000 0x400>;
>  	};
> 
> +	mipi_phy: video-phy {

nit: video-phy@10020710

Best regards,
Tomasz

> +		compatible = "samsung,s5pv210-video-phy";
> +		reg = <0x10020710 8>;
> +		#phy-cells = <1>;
> +	};
> +
>  	dsi_0: dsi@11C80000 {
>  		compatible = "samsung,exynos4210-mipi-dsi";
>  		reg = <0x11C80000 0x10000>;
>  		interrupts = <0 79 0>;
>  		samsung,phy-type = <0>;
>  		samsung,power-domain = <&pd_lcd0>;
> +		phys = <&mipi_phy 1>;
> +		phy-names = "dsim";
>  		clocks = <&clock 286>, <&clock 143>;
>  		clock-names = "bus_clk", "pll_clk";
>  		status = "disabled";
> @@ -181,6 +189,8 @@
>  			interrupts = <0 78 0>;
>  			bus-width = <4>;
>  			samsung,power-domain = <&pd_cam>;
> +			phys = <&mipi_phy 0>;
> +			phy-names = "csis";
>  			status = "disabled";
>  		};
> 
> @@ -190,6 +200,8 @@
>  			interrupts = <0 80 0>;
>  			bus-width = <2>;
>  			samsung,power-domain = <&pd_cam>;
> +			phys = <&mipi_phy 2>;
> +			phy-names = "csis";
>  			status = "disabled";
>  		};
>  	};

Re: [RFC PATCH 3/5] video: exynos_dsi: Use generic PHY driver

[Tomasz Figa]

On Friday 14 of June 2013 19:45:49 Sylwester Nawrocki wrote:
> Use the generic PHY API instead of the platform callback to control
> the MIPI DSIM DPHY.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/video/display/source-exynos_dsi.c |   36
> +++++++++-------------------- include/video/exynos_dsi.h               
> |    5 ----
>  2 files changed, 11 insertions(+), 30 deletions(-)

Yes, this is what I was really missing a lot while developing this driver.

Definitely looks good! It's a shame we don't have this driver in mainline 
yet ;) ,

Best regards,
Tomasz

> diff --git a/drivers/video/display/source-exynos_dsi.c
> b/drivers/video/display/source-exynos_dsi.c index 145d57b..dfab790
> 100644
> --- a/drivers/video/display/source-exynos_dsi.c
> +++ b/drivers/video/display/source-exynos_dsi.c
> @@ -24,6 +24,7 @@
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> @@ -219,6 +220,7 @@ struct exynos_dsi {
>  	bool enabled;
> 
>  	struct platform_device *pdev;
> +	struct phy *phy;
>  	struct device *dev;
>  	struct resource *res;
>  	struct clk *pll_clk;
> @@ -816,6 +818,7 @@ again:
> 
>  static bool exynos_dsi_transfer_finish(struct exynos_dsi *dsi)
>  {
> +	static unsigned long j;
>  	struct exynos_dsi_transfer *xfer;
>  	unsigned long flags;
>  	bool start = true;
> @@ -824,7 +827,8 @@ static bool exynos_dsi_transfer_finish(struct
> exynos_dsi *dsi)
> 
>  	if (list_empty(&dsi->transfer_list)) {
>  		spin_unlock_irqrestore(&dsi->transfer_lock, flags);
> -		dev_warn(dsi->dev, "unexpected TX/RX interrupt\n");
> +		if (printk_timed_ratelimit(&j, 500))
> +			dev_warn(dsi->dev, "unexpected TX/RX 
interrupt\n");
>  		return false;
>  	}
> 
> @@ -994,8 +998,7 @@ static int exynos_dsi_enable(struct video_source
> *src) clk_prepare_enable(dsi->bus_clk);
>  	clk_prepare_enable(dsi->pll_clk);
> 
> -	if (dsi->pd->phy_enable)
> -		dsi->pd->phy_enable(dsi->pdev, true);
> +	phy_power_on(dsi->phy);
> 
>  	exynos_dsi_reset(dsi);
>  	exynos_dsi_init_link(dsi);
> @@ -1019,8 +1022,7 @@ static int exynos_dsi_disable(struct video_source
> *src)
> 
>  	exynos_dsi_disable_clock(dsi);
> 
> -	if (dsi->pd->phy_enable)
> -		dsi->pd->phy_enable(dsi->pdev, false);
> +	phy_power_off(dsi->phy);
> 
>  	clk_disable_unprepare(dsi->pll_clk);
>  	clk_disable_unprepare(dsi->bus_clk);
> @@ -1099,12 +1101,6 @@ static const struct dsi_video_source_ops
> exynos_dsi_ops = { * Device Tree
>   */
> 
> -static int (* const of_phy_enables[])(struct platform_device *, bool) =
> { -#ifdef CONFIG_S5P_SETUP_MIPIPHY
> -	[0] = s5p_dsim_phy_enable,
> -#endif
> -};
> -
>  static struct exynos_dsi_platform_data *exynos_dsi_parse_dt(
>  						struct platform_device 
*pdev)
>  {
> @@ -1112,7 +1108,6 @@ static struct exynos_dsi_platform_data
> *exynos_dsi_parse_dt( struct exynos_dsi_platform_data *dsi_pd;
>  	struct device *dev = &pdev->dev;
>  	const __be32 *prop_data;
> -	u32 val;
> 
>  	dsi_pd = kzalloc(sizeof(*dsi_pd), GFP_KERNEL);
>  	if (!dsi_pd) {
> @@ -1120,19 +1115,6 @@ static struct exynos_dsi_platform_data
> *exynos_dsi_parse_dt( return NULL;
>  	}
> 
> -	prop_data = of_get_property(node, "samsung,phy-type", NULL);
> -	if (!prop_data) {
> -		dev_err(dev, "failed to get phy-type property\n");
> -		goto err_free_pd;
> -	}
> -
> -	val = be32_to_cpu(*prop_data);
> -	if (val >= ARRAY_SIZE(of_phy_enables) || !of_phy_enables[val]) {
> -		dev_err(dev, "Invalid phy-type %u\n", val);
> -		goto err_free_pd;
> -	}
> -	dsi_pd->phy_enable = of_phy_enables[val];
> -
>  	prop_data = of_get_property(node, "samsung,pll-stable-time", 
NULL);
>  	if (!prop_data) {
>  		dev_err(dev, "failed to get pll-stable-time property\n");
> @@ -1254,6 +1236,10 @@ static int exynos_dsi_probe(struct
> platform_device *pdev) return -ENOMEM;
>  	}
> 
> +	dsi->phy = devm_phy_get(&pdev->dev, "dsim");
> +	if (IS_ERR(dsi->phy))
> +		return PTR_ERR(dsi->phy);
> +
>  	platform_set_drvdata(pdev, dsi);
> 
>  	dsi->irq = platform_get_irq(pdev, 0);
> diff --git a/include/video/exynos_dsi.h b/include/video/exynos_dsi.h
> index 95e1568..5c062c7 100644
> --- a/include/video/exynos_dsi.h
> +++ b/include/video/exynos_dsi.h
> @@ -25,9 +25,6 @@
>   */
>  struct exynos_dsi_platform_data {
>  	unsigned int enabled;
> -
> -	int (*phy_enable)(struct platform_device *pdev, bool on);
> -
>  	unsigned int pll_stable_time;
>  	unsigned long pll_clk_rate;
>  	unsigned long esc_clk_rate;
> @@ -36,6 +33,4 @@ struct exynos_dsi_platform_data {
>  	unsigned short rx_timeout;
>  };
> 
> -int s5p_dsim_phy_enable(struct platform_device *pdev, bool on);
> -
>  #endif /* _EXYNOS_MIPI_DSIM_H */

Re: [RFC PATCH 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Sylwester Nawrocki]

Hi Tomasz,

On 06/16/2013 11:11 PM, Tomasz Figa wrote:
> Hi Sylwester,
> 
> Looks good, but I added some nitpicks inline.
> 
> On Friday 14 of June 2013 19:45:47 Sylwester Nawrocki wrote:
>> Add a PHY provider driver for the Samsung S5P/Exynos SoC MIPI CSI-2
>> receiver and MIPI DSI transmitter DPHYs.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  .../bindings/phy/exynos-video-mipi-phy.txt         |   16 ++
>>  drivers/phy/Kconfig                                |   10 ++
>>  drivers/phy/Makefile                               |    3 +-
>>  drivers/phy/exynos_video_mipi_phy.c                |  166
>> ++++++++++++++++++++ 4 files changed, 194 insertions(+), 1 deletion(-)
>>  create mode 100644
>> Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt create
>> mode 100644 drivers/phy/exynos_video_mipi_phy.c
>>
>> diff --git
>> a/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
>> b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt new
>> file mode 100644
>> index 0000000..32311c89
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
>> @@ -0,0 +1,16 @@
>> +Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY
>> +-------------------------------------------------
>> +
>> +Required properties:
>> +- compatible : "samsung,<soc_name>-video-phy", currently most SoCs can
> 
> I don't like this <soc_name> here. It sounds like any SoC name can be put 
> here. IMHO just listing all supported compatible values should be enough.

Hmm, OK, I'll simply put there the one compatible string supported now.

>> claim +  compatibility with the S5PV210 MIPI CSIS/DSIM PHY and thus
>> should use +  "samsung,s5pv210-video-phy";
>> +- reg : offset and length of the MIPI DPHY register set;
>> +- #phy-cells : from the generic phy bindings, must be 1;
>> +
>> +For "samsung,s5pv210-video-phy" compatible DPHYs the second cell in the
>> PHY +specifier identifies the DPHY and its meaning is as follows:
>> +  0 - MIPI CSIS 0,
>> +  1 - MIPI DSIM 0,
>> +  2 - MIPI CSIS 1,
>> +  3 - MIPI DSIM 1.
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 0764a54..d234e99 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -11,3 +11,13 @@ menuconfig GENERIC_PHY
>>  	  devices present in the kernel. This layer will have the generic
>>  	  API by which phy drivers can create PHY using the phy framework 
> and
>>  	  phy users can obtain reference to the PHY.
>> +
>> +if GENERIC_PHY
>> +
>> +config EXYNOS_VIDEO_MIPI_PHY
>> +	bool "S5P/EXYNOS MIPI CSI-2/DSI PHY driver"
>> +	depends on OF
> 
> Hmm. Is this driver designed only for OF-enabled boards?

Yes, there seems currently to be no users of MIPI CSIS/DSIM in the mainline 
kernel among the non-dt platforms, so I initially focused on DT only. I will
rework this driver to make it usable on non-dt platforms, but I currently 
have not way to fully test it. I believe S5PV210 will get migrated to device
tree sooner than anyone needs the functionality this driver provides on 
non-dt, and S5PC100 seems to be forgotten anyway.

>> +	help
>> +	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung
>> +	  S5P and EXYNOS SoCs.
>> +endif
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 9e9560f..b16f2c1 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -2,4 +2,5 @@
>>  # Makefile for the phy drivers.
>>  #
>>
>> -obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
>> +obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
>> +obj-$(CONFIG_EXYNOS_VIDEO_MIPI_PHY)	+= exynos_video_mipi_phy.o
>> diff --git a/drivers/phy/exynos_video_mipi_phy.c
>> b/drivers/phy/exynos_video_mipi_phy.c new file mode 100644
>> index 0000000..8d4976f
>> --- /dev/null
>> +++ b/drivers/phy/exynos_video_mipi_phy.c
>> @@ -0,0 +1,166 @@
>> +/*
>> + * Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Author: Sylwester Nawrocki <s.nawrocki@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/delay.h>
>> +#include <linux/io.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/spinlock.h>
>> +
>> +/* MIPI_PHYn_CONTROL register bit definitions */
>> +#define EXYNOS_MIPI_PHY_ENABLE		(1 << 0)
>> +#define EXYNOS_MIPI_PHY_SRESETN		(1 << 1)
>> +#define EXYNOS_MIPI_PHY_MRESETN		(1 << 2)
>> +#define EXYNOS_MIPI_PHY_RESET_MASK	(3 << 1)
>> +
>> +#define EXYNOS_MAX_VIDEO_PHYS		4
>> +
>> +struct exynos_video_phy {
>> +	spinlock_t slock;
>> +	struct phy *phys[EXYNOS_MAX_VIDEO_PHYS];
>> +	void __iomem *regs;
>> +};
>> +
>> +/*
>> + * The @id argument specifies MIPI CSIS or DSIM PHY as follows:
>> + *  0 - MIPI CSIS 0
>> + *  1 - MIPI DSIM 0
>> + *  2 - MIPI CSIS 1
>> + *  3 - MIPI DSIM 1
>> + */
>> +static int set_phy_state(struct exynos_video_phy *state,
>> +					unsigned int id, int on)
>> +{
>> +	void __iomem *addr = id < 2 ? state->regs : state->regs + 4;
> 
> I don't find this statement too readable. What about:
> 
> 	void __iomem *addr = state->regs;
> 
> and below:
> 
> 	/* CSIS 1 and DSIM 1 PHYs have separate register */
> 	if (id >= 2)
> 		addr += 4;

OK, thanks for the suggestion. I've addressed this in v2.

>> +	unsigned long flags;
>> +	u32 reg, reset;
>> +
>> +	pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
>> +		 __func__, id, on, (u32)addr, (u32)state->regs);
>> +
>> +	if (WARN_ON(id > EXYNOS_MAX_VIDEO_PHYS))
>> +		return -EINVAL;
>> +
>> +	if (id & 1)
> 
> Nice trick ;), but not very readable. What about creating an enum of PHYs 
> and using those defined values here:
> 
> 	if (id == PHY_DSI0 || id == PHY_DSI1)

Yeah, good point. Fixed.

>> +		reset = EXYNOS_MIPI_PHY_MRESETN;
>> +	else
>> +		reset = EXYNOS_MIPI_PHY_SRESETN;
>> +
>> +	spin_lock_irqsave(&state->slock, flags);
>> +
>> +	reg = readl(addr);
>> +	if (on)
>> +		reg |= reset;
>> +	else
>> +		reg &= ~reset;
>> +	writel(reg, addr);
>> +
>> +	if (on)
>> +		reg |= EXYNOS_MIPI_PHY_ENABLE;
> 
> I believe this is a kind of reference counting, but a comment here would 
> be nice.

Indeed, I've added a comment.

>> +	else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
>> +		reg &= ~EXYNOS_MIPI_PHY_ENABLE;
>> +
>> +	writel(reg, addr);
>> +
>> +	spin_unlock_irqrestore(&state->slock, flags);
>> +	return 0;
>> +}
>> +
>> +static int exynos_video_phy_power_on(struct phy *phy)
>> +{
>> +	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
>> +	return set_phy_state(state, phy->id, 1);
>> +}
>> +
>> +static int exynos_video_phy_power_off(struct phy *phy)
>> +{
>> +	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
>> +	return set_phy_state(state, phy->id, 0);
>> +}
>> +
>> +static struct phy *exynos_video_phy_xlate(struct device *dev,
>> +					struct of_phandle_args *args)
>> +{
>> +	struct exynos_video_phy *state = dev_get_drvdata(dev);
>> +
>> +	if (WARN_ON(args->args[0] > EXYNOS_MAX_VIDEO_PHYS))
>> +		return NULL;
>> +
>> +	return state->phys[args->args[0]];
>> +}
>> +
>> +static struct phy_ops exynos_video_phy_ops = {
>> +	.power_on	= exynos_video_phy_power_on,
>> +	.power_off	= exynos_video_phy_power_off,
>> +	.owner		= THIS_MODULE,
>> +};
>> +
>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>> +{
>> +	struct exynos_video_phy *state;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource res;
>> +	struct phy_provider *phy_provider;
>> +	int ret, i;
>> +
>> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>> +	if (!state)
>> +		return -ENOMEM;
>> +
>> +	ret = of_address_to_resource(dev->of_node, 0, &res);
>> +	if (ret < 0)
>> +		return ret;
> 
> You can use platform_get_resource() here to get a resource generated for 
> you by of_platform_populate().
> 
> In addition you don't need to check the pointer returned by 
> platform_get_resource() because it is checked in devm_ioremap_resource().

Fixed.

>> +
>> +	state->regs = devm_ioremap_resource(dev, &res);
>> +	if (IS_ERR(state->regs))
>> +		return PTR_ERR(state->regs);
>> +
>> +	dev_set_drvdata(dev, state);
>> +
>> +	phy_provider = devm_of_phy_provider_register(dev, THIS_MODULE,
>> +					    exynos_video_phy_xlate);
>> +	if (IS_ERR(phy_provider))
>> +		return PTR_ERR(phy_provider);
>> +
>> +	for (i = 0; i < EXYNOS_MAX_VIDEO_PHYS; i++) {
>> +		state->phys[i] = devm_phy_create(dev, i, 
> &exynos_video_phy_ops,
>> +									
> state);
>> +		if (IS_ERR(state->phys[i])) {
>> +			dev_err(dev, "failed to create PHY %d\n", i);
>> +			return PTR_ERR(state->phys[i]);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id exynos_video_phy_of_match[] = {
>> +	{ .compatible = "samsung,s5pv210-video-phy" },
>> +	{ },
>> +};
> 
> IMHO a MODULE_DEVICE_TABLE should be added here.

True, added after modifying the Kconfig entry to allow this driver 
to be built as a module.

Regards,
Sylwester

Re: [RFC PATCH 3/5] video: exynos_dsi: Use generic PHY driver

[Sylwester Nawrocki]

On 06/16/2013 11:15 PM, Tomasz Figa wrote:
> On Friday 14 of June 2013 19:45:49 Sylwester Nawrocki wrote:
>> > Use the generic PHY API instead of the platform callback to control
>> > the MIPI DSIM DPHY.
>> > 
>> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> > ---
>> >  drivers/video/display/source-exynos_dsi.c |   36
>> > +++++++++-------------------- include/video/exynos_dsi.h               
>> > |    5 ----
>> >  2 files changed, 11 insertions(+), 30 deletions(-)
>
> Yes, this is what I was really missing a lot while developing this driver.
> 
> Definitely looks good! It's a shame we don't have this driver in mainline 
> yet ;)

Yes, I should have mentioned in the cover letter this patch depends
on modified version of this [1] patch set of yours. I'll drop this
patch and will update the driver staying in mainline now, but I won't
be able to test it, on a non-dt platform.

I guess even some pre-eliminary display (panel) API would be helpful.
The CDF development seems to have been stalled for some time. I wonder
if we could first have something that works for limited set of devices
and be extending it gradually, rather than living with zero support
for displays on DT based ARM platforms.

[1] http://www.spinics.net/lists/linux-fbdev/msg09689.html

Regards,
Sylwester

Re: [RFC PATCH 3/5] video: exynos_dsi: Use generic PHY driver

[Tomasz Figa]

On Wednesday 19 of June 2013 19:10:52 Sylwester Nawrocki wrote:
> On 06/16/2013 11:15 PM, Tomasz Figa wrote:
> > On Friday 14 of June 2013 19:45:49 Sylwester Nawrocki wrote:
> >> > Use the generic PHY API instead of the platform callback to control
> >> > the MIPI DSIM DPHY.
> >> > 
> >> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> > ---
> >> > 
> >> >  drivers/video/display/source-exynos_dsi.c |   36
> >> > 
> >> > +++++++++-------------------- include/video/exynos_dsi.h
> >> > 
> >> > |    5 ----
> >> >  
> >> >  2 files changed, 11 insertions(+), 30 deletions(-)
> > 
> > Yes, this is what I was really missing a lot while developing this
> > driver.
> > 
> > Definitely looks good! It's a shame we don't have this driver in
> > mainline yet ;)
> 
> Yes, I should have mentioned in the cover letter this patch depends
> on modified version of this [1] patch set of yours. I'll drop this
> patch and will update the driver staying in mainline now, but I won't
> be able to test it, on a non-dt platform.
> 
> I guess even some pre-eliminary display (panel) API would be helpful.
> The CDF development seems to have been stalled for some time. I wonder
> if we could first have something that works for limited set of devices
> and be extending it gradually, rather than living with zero support
> for displays on DT based ARM platforms.

Well, the problem is that once we define a binding for displays, we will 
have to keep support for this binding even if we decide to change 
something.

But as I discussed with Laurent and Alexandre at LinuxCon Japan, we should 
be able to reuse V4L2 bindings for our purposes, so someone just needs to 
code a proof of concept implementation that doesn't necessarily provide 
full functionality yet, but allows to make something work. Probably based 
on already posted RFC versions of CDF.

CCed Laurent and Alexandre, as they might be able to shed even more light 
on this.

Best regards,
Tomasz

> 
> [1] http://www.spinics.net/lists/linux-fbdev/msg09689.html
> 
> Regards,
> Sylwester

Re: [RFC PATCH 5/5] ARM: Samsung: Remove MIPI PHY setup code

[Sylwester Nawrocki]

On 06/16/2013 10:52 PM, Kukjin Kim wrote:
> On 06/15/13 02:45, Sylwester Nawrocki wrote:
>> > Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
>> > DPHYs so we can remove now unused code at arch/arm/plat-samsung.
>
> If so, sounds good :)
> 
>> > In case there is any board file for S5PV210 platforms using MIPI
>> > CSIS/DSIM (not any upstream currently) it should use the generic
>> > PHY API to bind the PHYs to respective PHY consumer drivers.
>
> To be honest, I didn't test this on boards but if the working is fine, 
> please go ahead without RFC.

Thanks for review. I've tested it on Exynos4412 based board, and will
check also on Exynos4210 TRATS before posting the final version.
It seems to work fine, I just won't be able to test on any non-dt
platform (s5pv210), and there is currently no users of MIPI CSIS/DSIM
on s5pv210. Moreover this series depends on the generic PHY API,
perhaps it can be merged for 3.11.

[1] http://www.spinics.net/lists/arm-kernel/msg251232.html

Regards,
Sylwester