[PATCH v3 0/3] phy: Add exynos-simple-phy driver

[PATCH v3 0/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

From: Rahul Sharma <Rahul.Sharma@samsung.com>

Hi All,

This series has been originally proposed by Tomasz Stanislawski. With
his permission, I have addressed the following review comments in V3.

Changelog:
v3:
	* Implement lazy-init of PHYs.
	* Use MACROs instead of numbers to represent phys.
	* Use regmap interface to access PMU registers.

It is based on "Next" branch in Kishon Vijay Abraham's tree at
https://git.kernel.org/cgit/linux/kernel/git/kishon/linux-phy.git/

/* Original Message from Tomasz Stanislawski <t.stanislaws@samsung.com> */

The Samsung SoCs from Exynos family are enhanced with a bunch of devices that
provide functionality of a physical layer for interfaces like USB, HDMI, SATA,
etc. They are controlled by a simple interface, often a single bit that enables
and/or resets the physical layer.

An IP driver should to control such a controller in an abstract manner.
Therefore, such 'enablers' were implemented as clocks in older versions of
Linux kernel.  With the dawn of PHY subsystems, PHYs become a natural way of
exporting the 'enabler' functionality to drivers.  However, there is an
unexpected consequence. Some of those 1-bit PHYs were implemented as separate
drivers.  This means that one has to create a struct device, struct phy, its
phy provider and 100-150 lines of driver code to basically set one bit.

The DP phy driver is a good example:
https://lkml.org/lkml/2013/7/18/53

And simple-phy RFC (shares only driver code but not other resources):
https://lkml.org/lkml/2013/10/21/313

To avoid waste of resources I propose to create all such 1-bit phys from Exynos
SoC using a single device, driver and phy provider.

This patchset contains a proposed solution.

All comment are welcome.

Hopefully in future the functionality introduced by this patch may be merged
into a larger Power Management Unit (PMU) gluer driver.  On Samsusng SoC , the
PMU part contains a number of register barely linked to power management (like
clock gating, clock dividers, CPU resetting, etc.).  It may be tempting to
create a hybrid driver that export clocks/phys/etc that are controlled by PMU
unit.

Alternative solutions might be:
* exporting a regmap to the IP driver and allow the driver to control the PHY layer
  like in the patch:
  http://thread.gmane.org/gmane.linux.kernel.samsung-soc/28617/focus=28648

* create a dedicated power domain for hdmiphy

Regards,
Tomasz Stanislawski

v2:
	* rename to exynos-simple-phy
	* fix usage of devm_ioremap()
	* add documentation for DT bindings
	* add patches to client drivers

v1: initial version

Tomasz Stanislawski (3):
  phy: Add exynos-simple-phy driver
  drm: exynos: hdmi: use hdmiphy as PHY
  s5p-tv: hdmi: use hdmiphy as PHY

 .../devicetree/bindings/phy/samsung-phy.txt        |   56 ++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   11 +-
 drivers/media/platform/s5p-tv/hdmi_drv.c           |   11 +-
 drivers/phy/Kconfig                                |    5 +
 drivers/phy/Makefile                               |    1 +
 drivers/phy/exynos-simple-phy.c                    |  189 ++++++++++++++++++++
 include/dt-bindings/phy/exynos-simple-phy.h        |   27 +++
 7 files changed, 290 insertions(+), 10 deletions(-)
 create mode 100644 drivers/phy/exynos-simple-phy.c
 create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h

-- 
1.7.9.5

[PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

From: Tomasz Stanislawski <t.stanislaws@samsung.com>

Add exynos-simple-phy driver to support a single register
PHY interfaces present on Exynos4 SoC.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>

---
 .../devicetree/bindings/phy/samsung-phy.txt        |   56 ++++++
 drivers/phy/Kconfig                                |    5 +
 drivers/phy/Makefile                               |    1 +
 drivers/phy/exynos-simple-phy.c                    |  189 ++++++++++++++++++++
 include/dt-bindings/phy/exynos-simple-phy.h        |   27 +++
 5 files changed, 278 insertions(+)
 create mode 100644 drivers/phy/exynos-simple-phy.c
 create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h

diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
index 2049261..12ad9cf 100644
--- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
+++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
@@ -161,3 +161,59 @@ Example:
 		usbdrdphy0 = &usb3_phy0;
 		usbdrdphy1 = &usb3_phy1;
 	};
+
+Samsung S5P/EXYNOS SoC series SIMPLE PHY
+-------------------------------------------------
+
+Required properties:
+- compatible : should be one of the listed compatibles:
+	- "samsung,exynos4210-simple-phy"
+	- "samsung,exynos4412-simple-phy"
+	- "samsung,exynos5250-simple-phy"
+	- "samsung,exynos5420-simple-phy"
+- samsung,pmureg-phandle - handle to syscon to control PMU registers
+- #phy-cells : from the generic phy bindings, must be 1;
+
+For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos4210
+are:
+  HDMI_PHY,
+  DAC_PHY,
+  ADC_PHY,
+  PCIE_PHY,
+  SATA_PHY.
+
+For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos4412
+are:
+  HDMI_PHY,
+  ADC_PHY.
+
+For "samsung,exynos5250-simple-phy" compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos5250
+are:
+  HDMI_PHY,
+  ADC_PHY,
+  SATA_PHY.
+
+For "samsung,exynos5420-simple-phy" compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos5420
+are:
+  HDMI_PHY,
+  ADC_PHY.
+
+Example:
+Simple PHY provider node:
+
+	simplephys: simple-phys@10040000 {
+		compatible = "samsung,exynos5250-simple-phy";
+		samsung,pmu-syscon = <&pmu_system_controller>;
+		#phy-cells = <1>;
+	};
+
+Other nodes accessing simple PHYs:
+
+	hdmi {
+		phys = <&simplephys HDMI_PHY>;
+		phy-names = "hdmiphy";
+	};
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 16a2f06..2a13e0d 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -178,4 +178,9 @@ config PHY_XGENE
 	help
 	  This option enables support for APM X-Gene SoC multi-purpose PHY.
 
+config EXYNOS_SIMPLE_PHY
+	tristate "Exynos Simple PHY driver"
+	help
+	  Support for 1-bit PHY controllers on SoCs from Exynos family.
+
 endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index b4f1d57..81b6efa 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -20,3 +20,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4X12_USB2)	+= phy-exynos4x12-usb2.o
 phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2)	+= phy-exynos5250-usb2.o
 obj-$(CONFIG_PHY_EXYNOS5_USBDRD)	+= phy-exynos5-usbdrd.o
 obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
+obj-$(CONFIG_EXYNOS_SIMPLE_PHY)		+= exynos-simple-phy.o
diff --git a/drivers/phy/exynos-simple-phy.c b/drivers/phy/exynos-simple-phy.c
new file mode 100644
index 0000000..792e9bc
--- /dev/null
+++ b/drivers/phy/exynos-simple-phy.c
@@ -0,0 +1,189 @@
+/*
+ * Exynos Simple PHY driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tomasz Stanislawski <t.stanislaws@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/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/phy/exynos-simple-phy.h>
+
+struct phy_driver_priv {
+	struct phy *phys[PHY_NR];
+	struct regmap *pmureg;
+	u32 offsets[PHY_NR];
+};
+
+struct phy_driver_data {
+	u32 index;
+	u32 offset;
+};
+
+struct phy_private {
+	struct regmap *pmureg;
+	u32 offset;
+};
+
+#define EXYNOS_PHY_ENABLE	(1 << 0)
+
+static int exynos_phy_power_on(struct phy *phy)
+{
+	struct phy_private *phy_private = phy_get_drvdata(phy);
+
+	return regmap_update_bits(phy_private->pmureg, phy_private->offset,
+		EXYNOS_PHY_ENABLE, 1);
+}
+
+static int exynos_phy_power_off(struct phy *phy)
+{
+	struct phy_private *phy_private = phy_get_drvdata(phy);
+
+	return regmap_update_bits(phy_private->pmureg, phy_private->offset,
+		EXYNOS_PHY_ENABLE, 0);
+}
+
+static struct phy_ops exynos_phy_ops = {
+	.power_on	= exynos_phy_power_on,
+	.power_off	= exynos_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static const struct phy_driver_data exynos4210_offsets[] = {
+	{ HDMI_PHY, 0x0700 },	/* HDMI_PHY */
+	{ DAC_PHY, 0x070C },	/* DAC_PHY */
+	{ ADC_PHY, 0x0718 },	/* ADC_PHY */
+	{ PCIE_PHY, 0x071C },	/* PCIE_PHY */
+	{ SATA_PHY, 0x0720 },	/* SATA_PHY */
+	{ INVALID, 0 },		/* End Mark */
+};
+
+static const struct phy_driver_data exynos4412_offsets[] = {
+	{ HDMI_PHY, 0x0700 },	/* HDMI_PHY */
+	{ ADC_PHY, 0x0718 },	/* ADC_PHY */
+	{ INVALID, 0 },		/* End Mark */
+};
+
+static const struct phy_driver_data exynos5250_offsets[] = {
+	{ HDMI_PHY, 0x0700 },	/* HDMI_PHY */
+	{ ADC_PHY, 0x0718 },	/* ADC_PHY */
+	{ SATA_PHY, 0x0724 },	/* SATA_PHY */
+	{ INVALID, 0 },		/* End Mark */
+};
+
+static const struct phy_driver_data exynos5420_offsets[] = {
+	{ HDMI_PHY, 0x0700 },	/* HDMI_PHY */
+	{ ADC_PHY, 0x0720 },	/* ADC_PHY */
+	{ INVALID, 0 },		/* End Mark */
+};
+
+static const struct of_device_id exynos_phy_of_match[] = {
+	{ .compatible = "samsung,exynos4210-simple-phy",
+	  .data = exynos4210_offsets},
+	{ .compatible = "samsung,exynos4412-simple-phy",
+	  .data = exynos4412_offsets},
+	{ .compatible = "samsung,exynos5250-simple-phy",
+	  .data = exynos5250_offsets},
+	{ .compatible = "samsung,exynos5420-simple-phy",
+	  .data = exynos5420_offsets},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, exynos_phy_of_match);
+
+static struct phy *exynos_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	struct phy_driver_priv *priv = dev_get_drvdata(dev);
+	struct phy_private *phy_private;
+	int index = args->args[0];
+
+	/* verify if index and corresponding offset are valid */
+	if (index >= PHY_NR || priv->offsets[index] == INVALID)
+		return ERR_PTR(-ENODEV);
+
+	/* return phy if already allocated */
+	if (!IS_ERR_OR_NULL(priv->phys[index]))
+		return priv->phys[index];
+
+	priv->phys[index] = devm_phy_create(dev, &exynos_phy_ops, NULL);
+	if (IS_ERR(priv->phys[index])) {
+		dev_err(dev, "failed to create PHY %d\n", index);
+		return priv->phys[index];
+	}
+
+	phy_private = devm_kzalloc(dev, sizeof(*phy_private), GFP_KERNEL);
+	if (!phy_private)
+		return ERR_PTR(-ENOMEM);
+
+	phy_private->pmureg = priv->pmureg;
+	phy_private->offset = priv->offsets[index];
+	phy_set_drvdata(priv->phys[index], phy_private);
+
+	return priv->phys[index];
+}
+
+static int exynos_phy_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *of_id = of_match_device(
+		of_match_ptr(exynos_phy_of_match), &pdev->dev);
+	const struct phy_driver_data *drv_data = of_id->data;
+	struct device *dev = &pdev->dev;
+	struct phy_driver_priv *priv;
+	struct phy_provider *phy_provider;
+	int i;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, priv);
+
+	priv->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
+			"samsung,pmu-syscon");
+	if (IS_ERR(priv->pmureg)) {
+		dev_err(dev, "Failed to map PMU register (via syscon)\n");
+		return PTR_ERR(priv->pmureg);
+	}
+
+	/* make all offsets invalid */
+	for (i = 0; i < PHY_NR; i++)
+		priv->offsets[i] = INVALID;
+
+	/* initialize offsets only if available in drv data */
+	for (i = 0; drv_data[i].index != INVALID; i++)
+		priv->offsets[drv_data[i].index] = drv_data[i].offset;
+
+	phy_provider = devm_of_phy_provider_register(dev, exynos_phy_xlate);
+	if (IS_ERR(phy_provider)) {
+		dev_err(dev, "failed to register PHY provider\n");
+		return PTR_ERR(phy_provider);
+	}
+
+	dev_info(dev, "probe success\n");
+
+	return 0;
+}
+
+static struct platform_driver exynos_phy_driver = {
+	.probe	= exynos_phy_probe,
+	.driver = {
+		.of_match_table	= exynos_phy_of_match,
+		.name  = "exynos-simple-phy",
+		.owner = THIS_MODULE,
+	}
+};
+module_platform_driver(exynos_phy_driver);
+
+MODULE_DESCRIPTION("Exynos Simple PHY driver");
+MODULE_AUTHOR("Tomasz Stanislawski <t.stanislaws@samsung.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/dt-bindings/phy/exynos-simple-phy.h b/include/dt-bindings/phy/exynos-simple-phy.h
new file mode 100644
index 0000000..30bb341
--- /dev/null
+++ b/include/dt-bindings/phy/exynos-simple-phy.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tomasz Stanislawski <t.stanislaws@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.
+ *
+ * Device Tree binding constants for Exynos Simple PHY driver
+ *
+ */
+
+#ifndef _DT_BINDINGS_PHY_EXYNOS_SIMPLE_PHY_H
+#define _DT_BINDINGS_PHY_EXYNOS_SIMPLE_PHY_H
+
+/* simeple phys */
+
+#define INVALID	(~1)
+
+#define HDMI_PHY	0
+#define DAC_PHY	1
+#define ADC_PHY	2
+#define PCIE_PHY	3
+#define SATA_PHY	4
+#define PHY_NR	5
+
+#endif
-- 
1.7.9.5

[PATCH v3 2/3] drm: exynos: hdmi: use hdmiphy as PHY

[Rahul Sharma]

From: Tomasz Stanislawski <t.stanislaws@samsung.com>

The HDMIPHY (physical interface) is controlled by a single
bit in a power controller's regiter. It was implemented
as clock. It was a simple but effective hack.

This patch makes HDMI driver to control HDMIPHY via PHY interface.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 9a6d652..ef1cdd0 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -36,6 +36,7 @@
 #include <linux/i2c.h>
 #include <linux/of_gpio.h>
 #include <linux/hdmi.h>
+#include <linux/phy/phy.h>
 
 #include <drm/exynos_drm.h>
 
@@ -74,8 +75,8 @@ struct hdmi_resources {
 	struct clk			*sclk_hdmi;
 	struct clk			*sclk_pixel;
 	struct clk			*sclk_hdmiphy;
-	struct clk			*hdmiphy;
 	struct clk			*mout_hdmi;
+	struct phy			*hdmiphy;
 	struct regulator_bulk_data	*regul_bulk;
 	int				regul_count;
 };
@@ -1854,7 +1855,7 @@ static void hdmi_poweron(struct exynos_drm_display *display)
 	if (regulator_bulk_enable(res->regul_count, res->regul_bulk))
 		DRM_DEBUG_KMS("failed to enable regulator bulk\n");
 
-	clk_prepare_enable(res->hdmiphy);
+	phy_power_on(res->hdmiphy);
 	clk_prepare_enable(res->hdmi);
 	clk_prepare_enable(res->sclk_hdmi);
 
@@ -1881,7 +1882,7 @@ static void hdmi_poweroff(struct exynos_drm_display *display)
 
 	clk_disable_unprepare(res->sclk_hdmi);
 	clk_disable_unprepare(res->hdmi);
-	clk_disable_unprepare(res->hdmiphy);
+	phy_power_off(res->hdmiphy);
 	regulator_bulk_disable(res->regul_count, res->regul_bulk);
 
 	pm_runtime_put_sync(hdata->dev);
@@ -1977,9 +1978,9 @@ static int hdmi_resources_init(struct hdmi_context *hdata)
 		DRM_ERROR("failed to get clock 'sclk_hdmiphy'\n");
 		goto fail;
 	}
-	res->hdmiphy = devm_clk_get(dev, "hdmiphy");
+	res->hdmiphy = devm_phy_get(dev, "hdmiphy");
 	if (IS_ERR(res->hdmiphy)) {
-		DRM_ERROR("failed to get clock 'hdmiphy'\n");
+		DRM_ERROR("failed to get phy 'hdmiphy'\n");
 		goto fail;
 	}
 	res->mout_hdmi = devm_clk_get(dev, "mout_hdmi");
-- 
1.7.9.5

[PATCH v3 3/3] s5p-tv: hdmi: use hdmiphy as PHY

[Rahul Sharma]

From: Tomasz Stanislawski <t.stanislaws@samsung.com>

The HDMIPHY (physical interface) is controlled by a single
bit in a power controller's regiter. It was implemented
as clock. It was a simple but effective hack.

This patch makes S5P-HDMI driver to control HDMIPHY via PHY interface.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
---
 drivers/media/platform/s5p-tv/hdmi_drv.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/hdmi_drv.c b/drivers/media/platform/s5p-tv/hdmi_drv.c
index 534722c..8013e52 100644
--- a/drivers/media/platform/s5p-tv/hdmi_drv.c
+++ b/drivers/media/platform/s5p-tv/hdmi_drv.c
@@ -32,6 +32,7 @@
 #include <linux/clk.h>
 #include <linux/regulator/consumer.h>
 #include <linux/v4l2-dv-timings.h>
+#include <linux/phy/phy.h>
 
 #include <media/s5p_hdmi.h>
 #include <media/v4l2-common.h>
@@ -66,7 +67,7 @@ struct hdmi_resources {
 	struct clk *sclk_hdmi;
 	struct clk *sclk_pixel;
 	struct clk *sclk_hdmiphy;
-	struct clk *hdmiphy;
+	struct phy *hdmiphy;
 	struct regulator_bulk_data *regul_bulk;
 	int regul_count;
 };
@@ -586,7 +587,7 @@ static int hdmi_resource_poweron(struct hdmi_resources *res)
 	if (ret < 0)
 		return ret;
 	/* power-on hdmi physical interface */
-	clk_enable(res->hdmiphy);
+	phy_power_on(res->hdmiphy);
 	/* use VPP as parent clock; HDMIPHY is not working yet */
 	clk_set_parent(res->sclk_hdmi, res->sclk_pixel);
 	/* turn clocks on */
@@ -600,7 +601,7 @@ static void hdmi_resource_poweroff(struct hdmi_resources *res)
 	/* turn clocks off */
 	clk_disable(res->sclk_hdmi);
 	/* power-off hdmiphy */
-	clk_disable(res->hdmiphy);
+	phy_power_off(res->hdmiphy);
 	/* turn HDMI power off */
 	regulator_bulk_disable(res->regul_count, res->regul_bulk);
 }
@@ -784,7 +785,7 @@ static void hdmi_resources_cleanup(struct hdmi_device *hdev)
 	/* kfree is NULL-safe */
 	kfree(res->regul_bulk);
 	if (!IS_ERR(res->hdmiphy))
-		clk_put(res->hdmiphy);
+		phy_put(res->hdmiphy);
 	if (!IS_ERR(res->sclk_hdmiphy))
 		clk_put(res->sclk_hdmiphy);
 	if (!IS_ERR(res->sclk_pixel))
@@ -835,7 +836,7 @@ static int hdmi_resources_init(struct hdmi_device *hdev)
 		dev_err(dev, "failed to get clock 'sclk_hdmiphy'\n");
 		goto fail;
 	}
-	res->hdmiphy = clk_get(dev, "hdmiphy");
+	res->hdmiphy = phy_get(dev, "hdmiphy");
 	if (IS_ERR(res->hdmiphy)) {
 		dev_err(dev, "failed to get clock 'hdmiphy'\n");
 		goto fail;
-- 
1.7.9.5

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Tomasz Figa]

Hi Rahul, Tomasz,

On 14.05.2014 21:17, Rahul Sharma wrote:
> From: Tomasz Stanislawski <t.stanislaws@samsung.com>
> 
> Add exynos-simple-phy driver to support a single register
> PHY interfaces present on Exynos4 SoC.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
> 
> ---
>  .../devicetree/bindings/phy/samsung-phy.txt        |   56 ++++++
>  drivers/phy/Kconfig                                |    5 +
>  drivers/phy/Makefile                               |    1 +
>  drivers/phy/exynos-simple-phy.c                    |  189 ++++++++++++++++++++
>  include/dt-bindings/phy/exynos-simple-phy.h        |   27 +++
>  5 files changed, 278 insertions(+)
>  create mode 100644 drivers/phy/exynos-simple-phy.c
>  create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h
> 
> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> index 2049261..12ad9cf 100644
> --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> @@ -161,3 +161,59 @@ Example:
>  		usbdrdphy0 = &usb3_phy0;
>  		usbdrdphy1 = &usb3_phy1;
>  	};
> +
> +Samsung S5P/EXYNOS SoC series SIMPLE PHY
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : should be one of the listed compatibles:
> +	- "samsung,exynos4210-simple-phy"
> +	- "samsung,exynos4412-simple-phy"
> +	- "samsung,exynos5250-simple-phy"
> +	- "samsung,exynos5420-simple-phy"
> +- samsung,pmureg-phandle - handle to syscon to control PMU registers
> +- #phy-cells : from the generic phy bindings, must be 1;
> +
> +For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in
> +the PHY specifier identifies the PHY and the supported phys for exynos4210
> +are:
> +  HDMI_PHY,
> +  DAC_PHY,
> +  ADC_PHY,
> +  PCIE_PHY,
> +  SATA_PHY.
> +
> +For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in
> +the PHY specifier identifies the PHY and the supported phys for exynos4412
> +are:
> +  HDMI_PHY,
> +  ADC_PHY.
> +
> +For "samsung,exynos5250-simple-phy" compatible PHYs the second cell in
> +the PHY specifier identifies the PHY and the supported phys for exynos5250
> +are:
> +  HDMI_PHY,
> +  ADC_PHY,
> +  SATA_PHY.
> +
> +For "samsung,exynos5420-simple-phy" compatible PHYs the second cell in
> +the PHY specifier identifies the PHY and the supported phys for exynos5420
> +are:
> +  HDMI_PHY,
> +  ADC_PHY.
> +
> +Example:
> +Simple PHY provider node:
> +
> +	simplephys: simple-phys@10040000 {
> +		compatible = "samsung,exynos5250-simple-phy";

Missing reg property or unnecessary @unit-address suffix in node name.

> +		samsung,pmu-syscon = <&pmu_system_controller>;
> +		#phy-cells = <1>;
> +	};

In general, the idea is quite good, but I think this should rather bind
to the main PMU node, since this is just a part of the PMU, not another
device in the system. This also means that the PMU node itself should be
the PHY provider.

Otherwise looks good.

Best regards,
Tomasz

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote:
[...]
> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
[...]
> +For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in
> +the PHY specifier identifies the PHY and the supported phys for exynos4210

I think the specifier is only the part after the phandle, so this should
probably be "... compatible PHYs the single cell specifier ..." or
something equivalent.

> +are:
> +  HDMI_PHY,
> +  DAC_PHY,
> +  ADC_PHY,
> +  PCIE_PHY,
> +  SATA_PHY.

I think you need to specify the literal values here as well, since the
binding must be fully self-contained. That is you can't rely on the DT
binding to be bundled with the exynos-simple-phy.h header.

> @@ -20,3 +20,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4X12_USB2)	+= phy-exynos4x12-usb2.o
>  phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2)	+= phy-exynos5250-usb2.o
>  obj-$(CONFIG_PHY_EXYNOS5_USBDRD)	+= phy-exynos5-usbdrd.o
>  obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
> +obj-$(CONFIG_EXYNOS_SIMPLE_PHY)		+= exynos-simple-phy.o

Perhaps this should be named phy-exynos-simple for consistency? Also it
may be a good idea to sort this alphabetically to reduce the potential
for conflicts.

> +static int exynos_phy_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *of_id = of_match_device(
> +		of_match_ptr(exynos_phy_of_match), &pdev->dev);

Why does this need of_match_ptr()?

> +	dev_info(dev, "probe success\n");

If at all this should be dev_dbg(). But in general the driver core will
already complain if the driver fails to probe, so there's in general no
need to mention when it probes successfully.

> diff --git a/include/dt-bindings/phy/exynos-simple-phy.h b/include/dt-bindings/phy/exynos-simple-phy.h
[...]
> +/* simeple phys */

s/simeple phys/simple PHYs/

Although on second thought that comment probably shouldn't be there in
the first place.

> +#define INVALID	(~1)

This doesn't belong in this header. The value should never be used by a
DT source file, should it?

> +#define HDMI_PHY	0
> +#define DAC_PHY	1
> +#define ADC_PHY	2
> +#define PCIE_PHY	3
> +#define SATA_PHY	4

Perhaps these should be namespaced somehow to avoid potential conflicts
with other PHY providers?

> +#define PHY_NR	5

I'm not sure that this belongs here either. It's not a value that will
ever appear in a DT source file.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

Thanks Tomasz,

On 15 May 2014 01:31, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Rahul, Tomasz,
[snip]
>> +     simplephys: simple-phys@10040000 {
>> +             compatible = "samsung,exynos5250-simple-phy";
>
> Missing reg property or unnecessary @unit-address suffix in node name.

I should have removed "@unit-address". I will change this.

>
>> +             samsung,pmu-syscon = <&pmu_system_controller>;
>> +             #phy-cells = <1>;
>> +     };
>
> In general, the idea is quite good, but I think this should rather bind
> to the main PMU node, since this is just a part of the PMU, not another
> device in the system. This also means that the PMU node itself should be
> the PHY provider.
>

Please correct me if I got you wrong. You want somthing like this:

pmu_system_controller: system-controller@10040000 {
         ...
          simple_phys: simple-phys {
                        compatible = "samsung,exynos5420-simple-phy";
                        ...
          };
};

Regards,
Rahul Sharma.

> Otherwise looks good.
>
> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

Hi Thierry,

On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote:
> [...]
>> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> [...]
>> +For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in
>> +the PHY specifier identifies the PHY and the supported phys for exynos4210
>
> I think the specifier is only the part after the phandle, so this should
> probably be "... compatible PHYs the single cell specifier ..." or
> something equivalent.
>

ok. I will rephrase this line.

>> +are:
>> +  HDMI_PHY,
>> +  DAC_PHY,
>> +  ADC_PHY,
>> +  PCIE_PHY,
>> +  SATA_PHY.
>
> I think you need to specify the literal values here as well, since the
> binding must be fully self-contained. That is you can't rely on the DT
> binding to be bundled with the exynos-simple-phy.h header.
>

Ok. I will add that.

>> @@ -20,3 +20,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4X12_USB2)       += phy-exynos4x12-usb2.o
>>  phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2)        += phy-exynos5250-usb2.o
>>  obj-$(CONFIG_PHY_EXYNOS5_USBDRD)     += phy-exynos5-usbdrd.o
>>  obj-$(CONFIG_PHY_XGENE)                      += phy-xgene.o
>> +obj-$(CONFIG_EXYNOS_SIMPLE_PHY)              += exynos-simple-phy.o
>
> Perhaps this should be named phy-exynos-simple for consistency? Also it
> may be a good idea to sort this alphabetically to reduce the potential
> for conflicts.

yea correct. I will use "phy-exynos-simple".
I will place this addition in alphabetical order in makefile.

>
>> +static int exynos_phy_probe(struct platform_device *pdev)
>> +{
>> +     const struct of_device_id *of_id = of_match_device(
>> +             of_match_ptr(exynos_phy_of_match), &pdev->dev);
>
> Why does this need of_match_ptr()?
>

yea correct. Not required.

>> +     dev_info(dev, "probe success\n");
>
> If at all this should be dev_dbg(). But in general the driver core will
> already complain if the driver fails to probe, so there's in general no
> need to mention when it probes successfully.
>

ok.

>> diff --git a/include/dt-bindings/phy/exynos-simple-phy.h b/include/dt-bindings/phy/exynos-simple-phy.h
> [...]
>> +/* simeple phys */
>
> s/simeple phys/simple PHYs/
>
> Although on second thought that comment probably shouldn't be there in
> the first place.
>
>> +#define INVALID      (~1)
>
> This doesn't belong in this header. The value should never be used by a
> DT source file, should it?

I will move this to driver file.
>
>> +#define HDMI_PHY     0
>> +#define DAC_PHY      1
>> +#define ADC_PHY      2
>> +#define PCIE_PHY     3
>> +#define SATA_PHY     4
>
> Perhaps these should be namespaced somehow to avoid potential conflicts
> with other PHY providers?

How about XXX_SIMPLE_PHY?

>
>> +#define PHY_NR       5
>
> I'm not sure that this belongs here either. It's not a value that will
> ever appear in a DT source file.

I want it to grow along with new additions in the phy list else
catastrophic. This will look unrelated in driver.

Regards,
Rahul Sharma.

>
> Thierry
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote:
> Hi Thierry,
> 
> On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote:
[...]
> >> +#define HDMI_PHY     0
> >> +#define DAC_PHY      1
> >> +#define ADC_PHY      2
> >> +#define PCIE_PHY     3
> >> +#define SATA_PHY     4
> >
> > Perhaps these should be namespaced somehow to avoid potential conflicts
> > with other PHY providers?
> 
> How about XXX_SIMPLE_PHY?

The indices are specific to the Exynos PHY device, so why not
PHY_EXYNOS_SIMPLE_XXX (or any variation thereof)?

> >> +#define PHY_NR       5
> >
> > I'm not sure that this belongs here either. It's not a value that will
> > ever appear in a DT source file.
> 
> I want it to grow along with new additions in the phy list else
> catastrophic. This will look unrelated in driver.

But this is in no way growing automatically as it is. Whoever adds a new
type of PHY will need to manually increment this define. Furthermore the
driver will need to be updated to cope with this anyway.

I think this is even a reason to have this only in the driver. Otherwise
you could have a situation where somebody upgrades the binding (and this
file along with it) but not update the driver with the necessary support.
But the driver will still pick up the PHY_NR change and will accept the
new PHY type as valid, even though it has no code to handle it. If you
have this in the driver, however, then as long as the driver has not yet
been updated it will reject requests for the new PHY type. And that's
exactly what it should be doing since it doesn't know how to handle it.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

On 15 May 2014 13:12, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote:
>> Hi Thierry,
>>
>> On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote:
> [...]
>> >> +#define HDMI_PHY     0
>> >> +#define DAC_PHY      1
>> >> +#define ADC_PHY      2
>> >> +#define PCIE_PHY     3
>> >> +#define SATA_PHY     4
>> >
>> > Perhaps these should be namespaced somehow to avoid potential conflicts
>> > with other PHY providers?
>>
>> How about XXX_SIMPLE_PHY?
>
> The indices are specific to the Exynos PHY device, so why not
> PHY_EXYNOS_SIMPLE_XXX (or any variation thereof)?

This looks ok. I will use that.

>
>> >> +#define PHY_NR       5
>> >
>> > I'm not sure that this belongs here either. It's not a value that will
>> > ever appear in a DT source file.
>>
>> I want it to grow along with new additions in the phy list else
>> catastrophic. This will look unrelated in driver.
>
> But this is in no way growing automatically as it is. Whoever adds a new
> type of PHY will need to manually increment this define. Furthermore the
> driver will need to be updated to cope with this anyway.

Not automatically. What I meant was If keeping it at end of the list, it is not
possible that somebody skip the updation of PHY_NR when adding a new phy
type.

If I leave a comment at the end of the list to update PHY_NR (after moving it
to driver), that also serves the purpose.

>
> I think this is even a reason to have this only in the driver. Otherwise
> you could have a situation where somebody upgrades the binding (and this
> file along with it) but not update the driver with the necessary support.
> But the driver will still pick up the PHY_NR change and will accept the
> new PHY type as valid, even though it has no code to handle it. If you
> have this in the driver, however, then as long as the driver has not yet
> been updated it will reject requests for the new PHY type. And that's
> exactly what it should be doing since it doesn't know how to handle it.

hmn...Ok.

Regards,
Rahul Sharma

>
> Thierry

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

On Thu, May 15, 2014 at 01:47:33PM +0530, Rahul Sharma wrote:
> On 15 May 2014 13:12, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote:
> >> On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote:
> >> > On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote:
[...]
> >> >> +#define PHY_NR       5
> >> >
> >> > I'm not sure that this belongs here either. It's not a value that will
> >> > ever appear in a DT source file.
> >>
> >> I want it to grow along with new additions in the phy list else
> >> catastrophic. This will look unrelated in driver.
> >
> > But this is in no way growing automatically as it is. Whoever adds a new
> > type of PHY will need to manually increment this define. Furthermore the
> > driver will need to be updated to cope with this anyway.
> 
> Not automatically. What I meant was If keeping it at end of the list, it is not
> possible that somebody skip the updation of PHY_NR when adding a new phy
> type.

It's perhaps not as likely, but still possible.

> If I leave a comment at the end of the list to update PHY_NR (after moving it
> to driver), that also serves the purpose.

I don't think this is needed either. Like I said earlier, since the
driver has an internal maximum number of PHYs that it supports the
maximum that can be specified in the DTS is irrelevant. If it doesn't
support a new one, then it will simply return an error. And I would
assume that if somebody added support for a new PHY type then they
probably wouldn't forget to update the driver since they're modifying
it anyway and testing will fail if they don't.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote:
> From: Tomasz Stanislawski <t.stanislaws@samsung.com>
> 
> Add exynos-simple-phy driver to support a single register
> PHY interfaces present on Exynos4 SoC.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
> 
> ---
>  .../devicetree/bindings/phy/samsung-phy.txt        |   56 ++++++
>  drivers/phy/Kconfig                                |    5 +
>  drivers/phy/Makefile                               |    1 +
>  drivers/phy/exynos-simple-phy.c                    |  189 ++++++++++++++++++++
>  include/dt-bindings/phy/exynos-simple-phy.h        |   27 +++
>  5 files changed, 278 insertions(+)
>  create mode 100644 drivers/phy/exynos-simple-phy.c
>  create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h

[...]

> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 16a2f06..2a13e0d 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -178,4 +178,9 @@ config PHY_XGENE
>  	help
>  	  This option enables support for APM X-Gene SoC multi-purpose PHY.
>  
> +config EXYNOS_SIMPLE_PHY
> +	tristate "Exynos Simple PHY driver"

Please limit this driver to EXYNOS platforms with:

	depends on ARCH_EXYNOS 

or (optionally)

	depends on ARCH_EXYNOS || COMPILE_TEST

Moreover the generic PHY dependency is missing.  It can be fixed with:

	select GENERIC_PHY

> +	help
> +	  Support for 1-bit PHY controllers on SoCs from Exynos family.
> +
>  endmenu

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

Hi,

On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
>
> Hi,
>
> On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote:
>> From: Tomasz Stanislawski <t.stanislaws@samsung.com>
>>
>> Add exynos-simple-phy driver to support a single register
>> PHY interfaces present on Exynos4 SoC.
>>
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>>
>> ---
>>  .../devicetree/bindings/phy/samsung-phy.txt        |   56 ++++++
>>  drivers/phy/Kconfig                                |    5 +
>>  drivers/phy/Makefile                               |    1 +
>>  drivers/phy/exynos-simple-phy.c                    |  189 ++++++++++++++++++++
>>  include/dt-bindings/phy/exynos-simple-phy.h        |   27 +++
>>  5 files changed, 278 insertions(+)
>>  create mode 100644 drivers/phy/exynos-simple-phy.c
>>  create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h
>
> [...]
>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 16a2f06..2a13e0d 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -178,4 +178,9 @@ config PHY_XGENE
>>       help
>>         This option enables support for APM X-Gene SoC multi-purpose PHY.
>>
>> +config EXYNOS_SIMPLE_PHY
>> +     tristate "Exynos Simple PHY driver"
>
> Please limit this driver to EXYNOS platforms with:
>
>         depends on ARCH_EXYNOS
>
> or (optionally)
>
>         depends on ARCH_EXYNOS || COMPILE_TEST
>
> Moreover the generic PHY dependency is missing.  It can be fixed with:
>
>         select GENERIC_PHY
>

Yea, I will fix this part.

Regards,
Rahul Sharma.

>> +     help
>> +       Support for 1-bit PHY controllers on SoCs from Exynos family.
>> +
>>  endmenu
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Kishon Vijay Abraham I]

On Thursday 15 May 2014 07:05 PM, Rahul Sharma wrote:
> Hi,
> 
> On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz
> <b.zolnierkie@samsung.com> wrote:
>>
>> Hi,
>>
>> On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote:
>>> From: Tomasz Stanislawski <t.stanislaws@samsung.com>
>>>
>>> Add exynos-simple-phy driver to support a single register
>>> PHY interfaces present on Exynos4 SoC.
>>>
>>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>>> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>>>
>>> ---
>>>  .../devicetree/bindings/phy/samsung-phy.txt        |   56 ++++++
>>>  drivers/phy/Kconfig                                |    5 +
>>>  drivers/phy/Makefile                               |    1 +
>>>  drivers/phy/exynos-simple-phy.c                    |  189 ++++++++++++++++++++
>>>  include/dt-bindings/phy/exynos-simple-phy.h        |   27 +++
>>>  5 files changed, 278 insertions(+)
>>>  create mode 100644 drivers/phy/exynos-simple-phy.c
>>>  create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h
>>
>> [...]
>>
>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>> index 16a2f06..2a13e0d 100644
>>> --- a/drivers/phy/Kconfig
>>> +++ b/drivers/phy/Kconfig
>>> @@ -178,4 +178,9 @@ config PHY_XGENE
>>>       help
>>>         This option enables support for APM X-Gene SoC multi-purpose PHY.
>>>
>>> +config EXYNOS_SIMPLE_PHY
>>> +     tristate "Exynos Simple PHY driver"
>>
>> Please limit this driver to EXYNOS platforms with:
>>
>>         depends on ARCH_EXYNOS
>>
>> or (optionally)
>>
>>         depends on ARCH_EXYNOS || COMPILE_TEST
>>
>> Moreover the generic PHY dependency is missing.  It can be fixed with:
>>
>>         select GENERIC_PHY
>>
> 
> Yea, I will fix this part.

While at that, change the header of the driver file to *2014*

Thanks
Kishon
> 
> Regards,
> Rahul Sharma.
> 
>>> +     help
>>> +       Support for 1-bit PHY controllers on SoCs from Exynos family.
>>> +
>>>  endmenu
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

On 15 May 2014 19:11, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
>
> On Thursday 15 May 2014 07:05 PM, Rahul Sharma wrote:
>> Hi,
>>
>> On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz
>> <b.zolnierkie@samsung.com> wrote:
>>>
>>> Hi,
>>>
>>> On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote:
>>>> From: Tomasz Stanislawski <t.stanislaws@samsung.com>
>>>>
>>>> Add exynos-simple-phy driver to support a single register
>>>> PHY interfaces present on Exynos4 SoC.
>>>>
>>>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>>>> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>>>>
>>>> ---
>>>>  .../devicetree/bindings/phy/samsung-phy.txt        |   56 ++++++
>>>>  drivers/phy/Kconfig                                |    5 +
>>>>  drivers/phy/Makefile                               |    1 +
>>>>  drivers/phy/exynos-simple-phy.c                    |  189 ++++++++++++++++++++
>>>>  include/dt-bindings/phy/exynos-simple-phy.h        |   27 +++
>>>>  5 files changed, 278 insertions(+)
>>>>  create mode 100644 drivers/phy/exynos-simple-phy.c
>>>>  create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>>> index 16a2f06..2a13e0d 100644
>>>> --- a/drivers/phy/Kconfig
>>>> +++ b/drivers/phy/Kconfig
>>>> @@ -178,4 +178,9 @@ config PHY_XGENE
>>>>       help
>>>>         This option enables support for APM X-Gene SoC multi-purpose PHY.
>>>>
>>>> +config EXYNOS_SIMPLE_PHY
>>>> +     tristate "Exynos Simple PHY driver"
>>>
>>> Please limit this driver to EXYNOS platforms with:
>>>
>>>         depends on ARCH_EXYNOS
>>>
>>> or (optionally)
>>>
>>>         depends on ARCH_EXYNOS || COMPILE_TEST
>>>
>>> Moreover the generic PHY dependency is missing.  It can be fixed with:
>>>
>>>         select GENERIC_PHY
>>>
>>
>> Yea, I will fix this part.
>
> While at that, change the header of the driver file to *2014*
>
Sure.

Thanks,
Rahul Sharma.

> Thanks
> Kishon
>>
>> Regards,
>> Rahul Sharma.
>>
>>>> +     help
>>>> +       Support for 1-bit PHY controllers on SoCs from Exynos family.
>>>> +
>>>>  endmenu
>>>
>>> Best regards,
>>> --
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Tomasz Figa]

On 15.05.2014 06:01, Rahul Sharma wrote:
> Thanks Tomasz,
> 
> On 15 May 2014 01:31, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Rahul, Tomasz,
> [snip]
>>> +     simplephys: simple-phys@10040000 {
>>> +             compatible = "samsung,exynos5250-simple-phy";
>>
>> Missing reg property or unnecessary @unit-address suffix in node name.
> 
> I should have removed "@unit-address". I will change this.
> 
>>
>>> +             samsung,pmu-syscon = <&pmu_system_controller>;
>>> +             #phy-cells = <1>;
>>> +     };
>>
>> In general, the idea is quite good, but I think this should rather bind
>> to the main PMU node, since this is just a part of the PMU, not another
>> device in the system. This also means that the PMU node itself should be
>> the PHY provider.
>>
> 
> Please correct me if I got you wrong. You want somthing like this:
> 
> pmu_system_controller: system-controller@10040000 {
>          ...
>           simple_phys: simple-phys {
>                         compatible = "samsung,exynos5420-simple-phy";
>                         ...
>           };
> };

Not exactly.

What I meant is that the PMU node itself should be the PHY provider, e.g.

pmu_system_controller: system-controller@10040000 {
	/* ... */
	#phy-cells = <1>;
};

and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.

I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.

So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.

Now, there is already ongoing effort to convert current freestanding PMU
configuration code in mach-exynos into a full-fledged PMU driver, but
not exactly in the same direction as I stated above. I'll try to
cooperate with Pankaj, who is responsible for this work to make this go
into the right track.

Best regards,
Tomasz

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 15.05.2014 06:01, Rahul Sharma wrote:
>> Thanks Tomasz,
>>
>> On 15 May 2014 01:31, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> Hi Rahul, Tomasz,
>> [snip]
>>>> +     simplephys: simple-phys@10040000 {
>>>> +             compatible = "samsung,exynos5250-simple-phy";
>>>
>>> Missing reg property or unnecessary @unit-address suffix in node name.
>>
>> I should have removed "@unit-address". I will change this.
>>
>>>
>>>> +             samsung,pmu-syscon = <&pmu_system_controller>;
>>>> +             #phy-cells = <1>;
>>>> +     };
>>>
>>> In general, the idea is quite good, but I think this should rather bind
>>> to the main PMU node, since this is just a part of the PMU, not another
>>> device in the system. This also means that the PMU node itself should be
>>> the PHY provider.
>>>
>>
>> Please correct me if I got you wrong. You want somthing like this:
>>
>> pmu_system_controller: system-controller@10040000 {
>>          ...
>>           simple_phys: simple-phys {
>>                         compatible = "samsung,exynos5420-simple-phy";
>>                         ...
>>           };
>> };
>
> Not exactly.
>
> What I meant is that the PMU node itself should be the PHY provider, e.g.
>
> pmu_system_controller: system-controller@10040000 {
>         /* ... */
>         #phy-cells = <1>;
> };
>
> and then the PMU node should instantiate the Exynos simple PHY driver,
> as this is a driver for a facility existing entirely inside of the PMU.
> Moreover, the driver should be rather called Exynos PMU PHY.
>
> I know this isn't really possible at the moment, but with device tree we
> must design things carefully, so it's better to take a bit more time and
> do things properly.
>
> So my opinion on this is that there should be a central Exynos PMU
> driver that claims the IO region and instantiates necessary subdrivers,
> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
> driver and more, similar to what is being done in drivers/mfd.

Hi Tomasz,



>
> Now, there is already ongoing effort to convert current freestanding PMU
> configuration code in mach-exynos into a full-fledged PMU driver, but
> not exactly in the same direction as I stated above. I'll try to
> cooperate with Pankaj, who is responsible for this work to make this go
> into the right track.
>
> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote:
> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 15.05.2014 06:01, Rahul Sharma wrote:
[snip]
>>>> the PHY provider.
>>>>
>>>
>>> Please correct me if I got you wrong. You want somthing like this:
>>>
>>> pmu_system_controller: system-controller@10040000 {
>>>          ...
>>>           simple_phys: simple-phys {
>>>                         compatible = "samsung,exynos5420-simple-phy";
>>>                         ...
>>>           };
>>> };
>>
>> Not exactly.
>>
>> What I meant is that the PMU node itself should be the PHY provider, e.g.
>>
>> pmu_system_controller: system-controller@10040000 {
>>         /* ... */
>>         #phy-cells = <1>;
>> };
>>
>> and then the PMU node should instantiate the Exynos simple PHY driver,
>> as this is a driver for a facility existing entirely inside of the PMU.
>> Moreover, the driver should be rather called Exynos PMU PHY.
>>
>> I know this isn't really possible at the moment, but with device tree we
>> must design things carefully, so it's better to take a bit more time and
>> do things properly.
>>
>> So my opinion on this is that there should be a central Exynos PMU
>> driver that claims the IO region and instantiates necessary subdrivers,
>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
>> driver and more, similar to what is being done in drivers/mfd.
>

Hi Tomasz,

These PHYs are not part of PMU as such. I am not sure if it is correct to
probe them as phy provider for all these phys. Only relation of these phys with
the PMU is 'enable/disable control'. Controlling this bit using regmap interface
still looks better to me.

IMHO Ideal method would be probing these PHYs independently and resolving
the necessary dependencies like syscon handle, clocks etc. This way we will
not be having any common phy provider for all these independent PHYs and it
would be clean to add each of these phy nodes in DT. Please see my original
comment below.

http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html

Regards,
Rahul Sharma

>
>>
>> Now, there is already ongoing effort to convert current freestanding PMU
>> configuration code in mach-exynos into a full-fledged PMU driver, but
>> not exactly in the same direction as I stated above. I'll try to
>> cooperate with Pankaj, who is responsible for this work to make this go
>> into the right track.
>>
>> Best regards,
>> Tomasz
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Tomasz Figa]

On 16.05.2014 12:35, Rahul Sharma wrote:
> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote:
>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> On 15.05.2014 06:01, Rahul Sharma wrote:
> [snip]
>>>>> the PHY provider.
>>>>>
>>>>
>>>> Please correct me if I got you wrong. You want somthing like this:
>>>>
>>>> pmu_system_controller: system-controller@10040000 {
>>>>          ...
>>>>           simple_phys: simple-phys {
>>>>                         compatible = "samsung,exynos5420-simple-phy";
>>>>                         ...
>>>>           };
>>>> };
>>>
>>> Not exactly.
>>>
>>> What I meant is that the PMU node itself should be the PHY provider, e.g.
>>>
>>> pmu_system_controller: system-controller@10040000 {
>>>         /* ... */
>>>         #phy-cells = <1>;
>>> };
>>>
>>> and then the PMU node should instantiate the Exynos simple PHY driver,
>>> as this is a driver for a facility existing entirely inside of the PMU.
>>> Moreover, the driver should be rather called Exynos PMU PHY.
>>>
>>> I know this isn't really possible at the moment, but with device tree we
>>> must design things carefully, so it's better to take a bit more time and
>>> do things properly.
>>>
>>> So my opinion on this is that there should be a central Exynos PMU
>>> driver that claims the IO region and instantiates necessary subdrivers,
>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
>>> driver and more, similar to what is being done in drivers/mfd.
>>
> 
> Hi Tomasz,
> 
> These PHYs are not part of PMU as such. I am not sure if it is correct to
> probe them as phy provider for all these phys. Only relation of these phys with
> the PMU is 'enable/disable control'.

Well, in reality what is implemented by this driver is not even a PHY,
just some kind of power controllers, which are contained entirely in the
PMU.

> Controlling this bit using regmap interface
> still looks better to me.

Well, when there is a choice between using regmap and not using regmap,
I'd rather choose the latter. Why would you want to introduce additional
abstraction layer if there is no need for such?

> 
> IMHO Ideal method would be probing these PHYs independently and resolving
> the necessary dependencies like syscon handle, clocks etc. This way we will
> not be having any common phy provider for all these independent PHYs and it
> would be clean to add each of these phy nodes in DT. Please see my original
> comment below.
> 
> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html

With the solution I proposed, you don't need any kind of dependencies
for those simple power controllers. They are just single bits that don't
need anything special to operate, except PMU clock running.

Best regards,
Tomasz

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote:
> On 16.05.2014 12:35, Rahul Sharma wrote:
>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote:
>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>> On 15.05.2014 06:01, Rahul Sharma wrote:
>> [snip]
>>>>>> the PHY provider.
>>>>>>
>>>>>
>>>>> Please correct me if I got you wrong. You want somthing like this:
>>>>>
>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>          ...
>>>>>           simple_phys: simple-phys {
>>>>>                         compatible = "samsung,exynos5420-simple-phy";
>>>>>                         ...
>>>>>           };
>>>>> };
>>>>
>>>> Not exactly.
>>>>
>>>> What I meant is that the PMU node itself should be the PHY provider, e.g.
>>>>
>>>> pmu_system_controller: system-controller@10040000 {
>>>>         /* ... */
>>>>         #phy-cells = <1>;
>>>> };
>>>>
>>>> and then the PMU node should instantiate the Exynos simple PHY driver,
>>>> as this is a driver for a facility existing entirely inside of the PMU.
>>>> Moreover, the driver should be rather called Exynos PMU PHY.
>>>>
>>>> I know this isn't really possible at the moment, but with device tree we
>>>> must design things carefully, so it's better to take a bit more time and
>>>> do things properly.
>>>>
>>>> So my opinion on this is that there should be a central Exynos PMU
>>>> driver that claims the IO region and instantiates necessary subdrivers,
>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
>>>> driver and more, similar to what is being done in drivers/mfd.
>>>
>>
>> Hi Tomasz,
>>
>> These PHYs are not part of PMU as such. I am not sure if it is correct to
>> probe them as phy provider for all these phys. Only relation of these phys with
>> the PMU is 'enable/disable control'.
>
> Well, in reality what is implemented by this driver is not even a PHY,
> just some kind of power controllers, which are contained entirely in the
> PMU.
>

I agree. Actually the role of generic phy framework for these 'simple' phys is
only that much.

>> Controlling this bit using regmap interface
>> still looks better to me.
>
> Well, when there is a choice between using regmap and not using regmap,
> I'd rather choose the latter. Why would you want to introduce additional
> abstraction layer if there is no need for such?
>
>>
>> IMHO Ideal method would be probing these PHYs independently and resolving
>> the necessary dependencies like syscon handle, clocks etc. This way we will
>> not be having any common phy provider for all these independent PHYs and it
>> would be clean to add each of these phy nodes in DT. Please see my original
>> comment below.
>>
>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
>
> With the solution I proposed, you don't need any kind of dependencies
> for those simple power controllers. They are just single bits that don't
> need anything special to operate, except PMU clock running.

In that case we can further trim it down and let the drivers use the regmap
interface to control this bit. Many drivers including HDMI, DP just need that
much functionality from the phy provider.

Regards,
Rahul Sharma

>
> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Tomasz Figa]

On 16.05.2014 16:30, Rahul Sharma wrote:
> On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote:
>> On 16.05.2014 12:35, Rahul Sharma wrote:
>>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote:
>>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>> On 15.05.2014 06:01, Rahul Sharma wrote:
>>> [snip]
>>>>>>> the PHY provider.
>>>>>>>
>>>>>>
>>>>>> Please correct me if I got you wrong. You want somthing like this:
>>>>>>
>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>          ...
>>>>>>           simple_phys: simple-phys {
>>>>>>                         compatible = "samsung,exynos5420-simple-phy";
>>>>>>                         ...
>>>>>>           };
>>>>>> };
>>>>>
>>>>> Not exactly.
>>>>>
>>>>> What I meant is that the PMU node itself should be the PHY provider, e.g.
>>>>>
>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>         /* ... */
>>>>>         #phy-cells = <1>;
>>>>> };
>>>>>
>>>>> and then the PMU node should instantiate the Exynos simple PHY driver,
>>>>> as this is a driver for a facility existing entirely inside of the PMU.
>>>>> Moreover, the driver should be rather called Exynos PMU PHY.
>>>>>
>>>>> I know this isn't really possible at the moment, but with device tree we
>>>>> must design things carefully, so it's better to take a bit more time and
>>>>> do things properly.
>>>>>
>>>>> So my opinion on this is that there should be a central Exynos PMU
>>>>> driver that claims the IO region and instantiates necessary subdrivers,
>>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
>>>>> driver and more, similar to what is being done in drivers/mfd.
>>>>
>>>
>>> Hi Tomasz,
>>>
>>> These PHYs are not part of PMU as such. I am not sure if it is correct to
>>> probe them as phy provider for all these phys. Only relation of these phys with
>>> the PMU is 'enable/disable control'.
>>
>> Well, in reality what is implemented by this driver is not even a PHY,
>> just some kind of power controllers, which are contained entirely in the
>> PMU.
>>
> 
> I agree. Actually the role of generic phy framework for these 'simple' phys is
> only that much.
> 
>>> Controlling this bit using regmap interface
>>> still looks better to me.
>>
>> Well, when there is a choice between using regmap and not using regmap,
>> I'd rather choose the latter. Why would you want to introduce additional
>> abstraction layer if there is no need for such?
>>
>>>
>>> IMHO Ideal method would be probing these PHYs independently and resolving
>>> the necessary dependencies like syscon handle, clocks etc. This way we will
>>> not be having any common phy provider for all these independent PHYs and it
>>> would be clean to add each of these phy nodes in DT. Please see my original
>>> comment below.
>>>
>>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
>>
>> With the solution I proposed, you don't need any kind of dependencies
>> for those simple power controllers. They are just single bits that don't
>> need anything special to operate, except PMU clock running.
> 
> In that case we can further trim it down and let the drivers use the regmap
> interface to control this bit. Many drivers including HDMI, DP just need that
> much functionality from the phy provider.

Well, this is what several drivers already do, like USB PHY (dedicated
IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
too) or will do, like I2C (for configuration of I2C mux on Exynos5).

At least this would be consistent with them and wouldn't be an API
abuse, so I'd be inclined to go this way more than introducing
abstractions like this patch does.

Best regards,
Tomasz

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

On 16 May 2014 20:19, Tomasz Figa <t.figa@samsung.com> wrote:
> On 16.05.2014 16:30, Rahul Sharma wrote:
>> On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote:
>>> On 16.05.2014 12:35, Rahul Sharma wrote:
>>>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote:
>>>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>>> On 15.05.2014 06:01, Rahul Sharma wrote:
>>>> [snip]
>>>>>>>> the PHY provider.
>>>>>>>>
>>>>>>>
>>>>>>> Please correct me if I got you wrong. You want somthing like this:
>>>>>>>
>>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>>          ...
>>>>>>>           simple_phys: simple-phys {
>>>>>>>                         compatible = "samsung,exynos5420-simple-phy";
>>>>>>>                         ...
>>>>>>>           };
>>>>>>> };
>>>>>>
>>>>>> Not exactly.
>>>>>>
>>>>>> What I meant is that the PMU node itself should be the PHY provider, e.g.
>>>>>>
>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>         /* ... */
>>>>>>         #phy-cells = <1>;
>>>>>> };
>>>>>>
>>>>>> and then the PMU node should instantiate the Exynos simple PHY driver,
>>>>>> as this is a driver for a facility existing entirely inside of the PMU.
>>>>>> Moreover, the driver should be rather called Exynos PMU PHY.
>>>>>>
>>>>>> I know this isn't really possible at the moment, but with device tree we
>>>>>> must design things carefully, so it's better to take a bit more time and
>>>>>> do things properly.
>>>>>>
>>>>>> So my opinion on this is that there should be a central Exynos PMU
>>>>>> driver that claims the IO region and instantiates necessary subdrivers,
>>>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
>>>>>> driver and more, similar to what is being done in drivers/mfd.
>>>>>
>>>>
>>>> Hi Tomasz,
>>>>
>>>> These PHYs are not part of PMU as such. I am not sure if it is correct to
>>>> probe them as phy provider for all these phys. Only relation of these phys with
>>>> the PMU is 'enable/disable control'.
>>>
>>> Well, in reality what is implemented by this driver is not even a PHY,
>>> just some kind of power controllers, which are contained entirely in the
>>> PMU.
>>>
>>
>> I agree. Actually the role of generic phy framework for these 'simple' phys is
>> only that much.
>>
>>>> Controlling this bit using regmap interface
>>>> still looks better to me.
>>>
>>> Well, when there is a choice between using regmap and not using regmap,
>>> I'd rather choose the latter. Why would you want to introduce additional
>>> abstraction layer if there is no need for such?
>>>
>>>>
>>>> IMHO Ideal method would be probing these PHYs independently and resolving
>>>> the necessary dependencies like syscon handle, clocks etc. This way we will
>>>> not be having any common phy provider for all these independent PHYs and it
>>>> would be clean to add each of these phy nodes in DT. Please see my original
>>>> comment below.
>>>>
>>>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
>>>
>>> With the solution I proposed, you don't need any kind of dependencies
>>> for those simple power controllers. They are just single bits that don't
>>> need anything special to operate, except PMU clock running.
>>
>> In that case we can further trim it down and let the drivers use the regmap
>> interface to control this bit. Many drivers including HDMI, DP just need that
>> much functionality from the phy provider.
>
> Well, this is what several drivers already do, like USB PHY (dedicated
> IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
> too) or will do, like I2C (for configuration of I2C mux on Exynos5).
>
> At least this would be consistent with them and wouldn't be an API
> abuse, so I'd be inclined to go this way more than introducing
> abstractions like this patch does.

Ok. I had already posted a patch for this at
http://www.spinics.net/lists/linux-samsung-soc/msg28049.html
I will revive that thread.

@Tomasz Stanislawski, Do you have different opinion here?

Regards,
Rahul Sharma.

>
> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Tomasz Figa]

On 19.05.2014 09:10, Rahul Sharma wrote:
> On 16 May 2014 20:19, Tomasz Figa <t.figa@samsung.com> wrote:
>> On 16.05.2014 16:30, Rahul Sharma wrote:
>>> On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote:
>>>> On 16.05.2014 12:35, Rahul Sharma wrote:
>>>>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote:
>>>>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>>>> On 15.05.2014 06:01, Rahul Sharma wrote:
>>>>> [snip]
>>>>>>>>> the PHY provider.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Please correct me if I got you wrong. You want somthing like this:
>>>>>>>>
>>>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>>>          ...
>>>>>>>>           simple_phys: simple-phys {
>>>>>>>>                         compatible = "samsung,exynos5420-simple-phy";
>>>>>>>>                         ...
>>>>>>>>           };
>>>>>>>> };
>>>>>>>
>>>>>>> Not exactly.
>>>>>>>
>>>>>>> What I meant is that the PMU node itself should be the PHY provider, e.g.
>>>>>>>
>>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>>         /* ... */
>>>>>>>         #phy-cells = <1>;
>>>>>>> };
>>>>>>>
>>>>>>> and then the PMU node should instantiate the Exynos simple PHY driver,
>>>>>>> as this is a driver for a facility existing entirely inside of the PMU.
>>>>>>> Moreover, the driver should be rather called Exynos PMU PHY.
>>>>>>>
>>>>>>> I know this isn't really possible at the moment, but with device tree we
>>>>>>> must design things carefully, so it's better to take a bit more time and
>>>>>>> do things properly.
>>>>>>>
>>>>>>> So my opinion on this is that there should be a central Exynos PMU
>>>>>>> driver that claims the IO region and instantiates necessary subdrivers,
>>>>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
>>>>>>> driver and more, similar to what is being done in drivers/mfd.
>>>>>>
>>>>>
>>>>> Hi Tomasz,
>>>>>
>>>>> These PHYs are not part of PMU as such. I am not sure if it is correct to
>>>>> probe them as phy provider for all these phys. Only relation of these phys with
>>>>> the PMU is 'enable/disable control'.
>>>>
>>>> Well, in reality what is implemented by this driver is not even a PHY,
>>>> just some kind of power controllers, which are contained entirely in the
>>>> PMU.
>>>>
>>>
>>> I agree. Actually the role of generic phy framework for these 'simple' phys is
>>> only that much.
>>>
>>>>> Controlling this bit using regmap interface
>>>>> still looks better to me.
>>>>
>>>> Well, when there is a choice between using regmap and not using regmap,
>>>> I'd rather choose the latter. Why would you want to introduce additional
>>>> abstraction layer if there is no need for such?
>>>>
>>>>>
>>>>> IMHO Ideal method would be probing these PHYs independently and resolving
>>>>> the necessary dependencies like syscon handle, clocks etc. This way we will
>>>>> not be having any common phy provider for all these independent PHYs and it
>>>>> would be clean to add each of these phy nodes in DT. Please see my original
>>>>> comment below.
>>>>>
>>>>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
>>>>
>>>> With the solution I proposed, you don't need any kind of dependencies
>>>> for those simple power controllers. They are just single bits that don't
>>>> need anything special to operate, except PMU clock running.
>>>
>>> In that case we can further trim it down and let the drivers use the regmap
>>> interface to control this bit. Many drivers including HDMI, DP just need that
>>> much functionality from the phy provider.
>>
>> Well, this is what several drivers already do, like USB PHY (dedicated
>> IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
>> too) or will do, like I2C (for configuration of I2C mux on Exynos5).
>>
>> At least this would be consistent with them and wouldn't be an API
>> abuse, so I'd be inclined to go this way more than introducing
>> abstractions like this patch does.
> 
> Ok. I had already posted a patch for this at
> http://www.spinics.net/lists/linux-samsung-soc/msg28049.html
> I will revive that thread.

Looks good to me.

> 
> @Tomasz Stanislawski, Do you have different opinion here?

I'm afraid Tomasz might not be very responsive during next few days, as
he is on a business trip. You might be able to reach him on our internal
communicator, though.

Best regards,
Tomasz

Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

On 19 May 2014 16:24, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 19.05.2014 09:10, Rahul Sharma wrote:
>> On 16 May 2014 20:19, Tomasz Figa <t.figa@samsung.com> wrote:
>>> On 16.05.2014 16:30, Rahul Sharma wrote:
>>>> On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote:
>>>>> On 16.05.2014 12:35, Rahul Sharma wrote:
>>>>>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote:
>>>>>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>>>>> On 15.05.2014 06:01, Rahul Sharma wrote:
>>>>>> [snip]
>>>>>>>>>> the PHY provider.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please correct me if I got you wrong. You want somthing like this:
>>>>>>>>>
>>>>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>>>>          ...
>>>>>>>>>           simple_phys: simple-phys {
>>>>>>>>>                         compatible = "samsung,exynos5420-simple-phy";
>>>>>>>>>                         ...
>>>>>>>>>           };
>>>>>>>>> };
>>>>>>>>
>>>>>>>> Not exactly.
>>>>>>>>
>>>>>>>> What I meant is that the PMU node itself should be the PHY provider, e.g.
>>>>>>>>
>>>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>>>         /* ... */
>>>>>>>>         #phy-cells = <1>;
>>>>>>>> };
>>>>>>>>
>>>>>>>> and then the PMU node should instantiate the Exynos simple PHY driver,
>>>>>>>> as this is a driver for a facility existing entirely inside of the PMU.
>>>>>>>> Moreover, the driver should be rather called Exynos PMU PHY.
>>>>>>>>
>>>>>>>> I know this isn't really possible at the moment, but with device tree we
>>>>>>>> must design things carefully, so it's better to take a bit more time and
>>>>>>>> do things properly.
>>>>>>>>
>>>>>>>> So my opinion on this is that there should be a central Exynos PMU
>>>>>>>> driver that claims the IO region and instantiates necessary subdrivers,
>>>>>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
>>>>>>>> driver and more, similar to what is being done in drivers/mfd.
>>>>>>>
>>>>>>
>>>>>> Hi Tomasz,
>>>>>>
>>>>>> These PHYs are not part of PMU as such. I am not sure if it is correct to
>>>>>> probe them as phy provider for all these phys. Only relation of these phys with
>>>>>> the PMU is 'enable/disable control'.
>>>>>
>>>>> Well, in reality what is implemented by this driver is not even a PHY,
>>>>> just some kind of power controllers, which are contained entirely in the
>>>>> PMU.
>>>>>
>>>>
>>>> I agree. Actually the role of generic phy framework for these 'simple' phys is
>>>> only that much.
>>>>
>>>>>> Controlling this bit using regmap interface
>>>>>> still looks better to me.
>>>>>
>>>>> Well, when there is a choice between using regmap and not using regmap,
>>>>> I'd rather choose the latter. Why would you want to introduce additional
>>>>> abstraction layer if there is no need for such?
>>>>>
>>>>>>
>>>>>> IMHO Ideal method would be probing these PHYs independently and resolving
>>>>>> the necessary dependencies like syscon handle, clocks etc. This way we will
>>>>>> not be having any common phy provider for all these independent PHYs and it
>>>>>> would be clean to add each of these phy nodes in DT. Please see my original
>>>>>> comment below.
>>>>>>
>>>>>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
>>>>>
>>>>> With the solution I proposed, you don't need any kind of dependencies
>>>>> for those simple power controllers. They are just single bits that don't
>>>>> need anything special to operate, except PMU clock running.
>>>>
>>>> In that case we can further trim it down and let the drivers use the regmap
>>>> interface to control this bit. Many drivers including HDMI, DP just need that
>>>> much functionality from the phy provider.
>>>
>>> Well, this is what several drivers already do, like USB PHY (dedicated
>>> IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
>>> too) or will do, like I2C (for configuration of I2C mux on Exynos5).
>>>
>>> At least this would be consistent with them and wouldn't be an API
>>> abuse, so I'd be inclined to go this way more than introducing
>>> abstractions like this patch does.
>>
>> Ok. I had already posted a patch for this at
>> http://www.spinics.net/lists/linux-samsung-soc/msg28049.html
>> I will revive that thread.
>
> Looks good to me.
>
>>
>> @Tomasz Stanislawski, Do you have different opinion here?
>
> I'm afraid Tomasz might not be very responsive during next few days, as
> he is on a business trip. You might be able to reach him on our internal
> communicator, though.

Thanks Tomasz,

I will contact him over communicator.

Regards.

>
> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel