Hello everyone,
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
Changelog:
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 | 24 +++
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 | 154 ++++++++++++++++++++
6 files changed, 196 insertions(+), 10 deletions(-)
create mode 100644 drivers/phy/exynos-simple-phy.c
--
1.7.9.5
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Add exynos-simple-phy driver to support a single register
PHY interfaces present on Exynos4 SoC.
Signed-off-by: Tomasz Stanislawski <[email protected]>
---
.../devicetree/bindings/phy/samsung-phy.txt | 24 +++
drivers/phy/Kconfig | 5 +
drivers/phy/Makefile | 1 +
drivers/phy/exynos-simple-phy.c | 154 ++++++++++++++++++++
4 files changed, 184 insertions(+)
create mode 100644 drivers/phy/exynos-simple-phy.c
diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
index b422e38..f97c4c3 100644
--- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
+++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
@@ -114,3 +114,27 @@ Example:
compatible = "samsung,exynos-sataphy-i2c";
reg = <0x38>;
};
+
+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"
+- reg : offset and length of the register set;
+- #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 its meaning is as follows:
+ 0 - HDMI PHY,
+ 1 - DAC PHY,
+ 2 - ADC PHY,
+ 3 - PCIE PHY.
+ 4 - SATA PHY.
+
+For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in
+the PHY specifier identifies the PHY and its meaning is as follows:
+ 0 - HDMI PHY,
+ 1 - ADC PHY,
+
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 3bb05f1..65ab783 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -166,4 +166,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 2faf78e..88c5b60 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_PHY_EXYNOS4210_USB2) += phy-exynos4210-usb2.o
obj-$(CONFIG_PHY_EXYNOS4X12_USB2) += phy-exynos4x12-usb2.o
obj-$(CONFIG_PHY_EXYNOS5250_USB2) += phy-exynos5250-usb2.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..57ad338
--- /dev/null
+++ b/drivers/phy/exynos-simple-phy.c
@@ -0,0 +1,154 @@
+/*
+ * Exynos Simple PHY driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tomasz Stanislawski <[email protected]>
+ *
+ * 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/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+
+#define EXYNOS_PHY_ENABLE (1 << 0)
+
+static int exynos_phy_power_on(struct phy *phy)
+{
+ void __iomem *reg = phy_get_drvdata(phy);
+ u32 val;
+
+ val = readl(reg);
+ val |= EXYNOS_PHY_ENABLE;
+ writel(val, reg);
+
+ return 0;
+}
+
+static int exynos_phy_power_off(struct phy *phy)
+{
+ void __iomem *reg = phy_get_drvdata(phy);
+ u32 val;
+
+ val = readl(reg);
+ val &= ~EXYNOS_PHY_ENABLE;
+ writel(val, reg);
+
+ return 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 u32 exynos4210_offsets[] = {
+ 0x0700, /* HDMI_PHY */
+ 0x070C, /* DAC_PHY */
+ 0x0718, /* ADC_PHY */
+ 0x071C, /* PCIE_PHY */
+ 0x0720, /* SATA_PHY */
+ ~0, /* end mark */
+};
+
+static const u32 exynos4412_offsets[] = {
+ 0x0700, /* HDMI_PHY */
+ 0x0718, /* ADC_PHY */
+ ~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},
+ { },
+};
+MODULE_DEVICE_TABLE(of, exynos_phy_of_match);
+
+static struct phy *exynos_phy_xlate(struct device *dev,
+ struct of_phandle_args *args)
+{
+ struct phy **phys = dev_get_drvdata(dev);
+ int index = args->args[0];
+ int i;
+
+ /* verify if index is valid */
+ for (i = 0; i <= index; ++i)
+ if (!phys[i])
+ return ERR_PTR(-ENODEV);
+
+ return 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 u32 *offsets = of_id->data;
+ int count;
+ struct device *dev = &pdev->dev;
+ struct phy **phys;
+ struct resource *res;
+ void __iomem *regs;
+ int i;
+ struct phy_provider *phy_provider;
+
+ /* count number of phys to create */
+ for (count = 0; offsets[count] != ~0; ++count)
+ ;
+
+ phys = devm_kzalloc(dev, (count + 1) * sizeof(phys[0]), GFP_KERNEL);
+ if (!phys)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, phys);
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ regs = devm_ioremap(dev, res->start, res->end - res->start);
+ if (!regs) {
+ dev_err(dev, "failed to ioremap registers\n");
+ return -EFAULT;
+ }
+
+ /* NOTE: last entry in phys[] is NULL */
+ for (i = 0; i < count; ++i) {
+ phys[i] = devm_phy_create(dev, &exynos_phy_ops, NULL);
+ if (IS_ERR(phys[i])) {
+ dev_err(dev, "failed to create PHY %d\n", i);
+ return PTR_ERR(phys[i]);
+ }
+ phy_set_drvdata(phys[i], regs + offsets[i]);
+ }
+
+ 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, "added %d phys\n", count);
+
+ 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 <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5
Hi Tomasz,
On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
> Add exynos-simple-phy driver to support a single register
> PHY interfaces present on Exynos4 SoC.
>
> Signed-off-by: Tomasz Stanislawski <[email protected]>
> ---
> .../devicetree/bindings/phy/samsung-phy.txt | 24 +++
> drivers/phy/Kconfig | 5 +
> drivers/phy/Makefile | 1 +
> drivers/phy/exynos-simple-phy.c | 154 ++++++++++++++++++++
> 4 files changed, 184 insertions(+)
> create mode 100644 drivers/phy/exynos-simple-phy.c
>
> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> index b422e38..f97c4c3 100644
> --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> @@ -114,3 +114,27 @@ Example:
> compatible = "samsung,exynos-sataphy-i2c";
> reg = <0x38>;
> };
> +
> +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"
> +- reg : offset and length of the register set;
> +- #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 its meaning is as follows:
> + 0 - HDMI PHY,
> + 1 - DAC PHY,
> + 2 - ADC PHY,
> + 3 - PCIE PHY.
> + 4 - SATA PHY.
> +
> +For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in
> +the PHY specifier identifies the PHY and its meaning is as follows:
> + 0 - HDMI PHY,
> + 1 - ADC PHY,
What about using preprocessor macros?
> +
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 3bb05f1..65ab783 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -166,4 +166,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 2faf78e..88c5b60 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_PHY_EXYNOS4210_USB2) += phy-exynos4210-usb2.o
> obj-$(CONFIG_PHY_EXYNOS4X12_USB2) += phy-exynos4x12-usb2.o
> obj-$(CONFIG_PHY_EXYNOS5250_USB2) += phy-exynos5250-usb2.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..57ad338
> --- /dev/null
> +++ b/drivers/phy/exynos-simple-phy.c
> @@ -0,0 +1,154 @@
> +/*
> + * Exynos Simple PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Tomasz Stanislawski <[email protected]>
> + *
> + * 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/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +
> +#define EXYNOS_PHY_ENABLE (1 << 0)
> +
> +static int exynos_phy_power_on(struct phy *phy)
> +{
> + void __iomem *reg = phy_get_drvdata(phy);
> + u32 val;
> +
> + val = readl(reg);
> + val |= EXYNOS_PHY_ENABLE;
> + writel(val, reg);
> +
> + return 0;
> +}
> +
> +static int exynos_phy_power_off(struct phy *phy)
> +{
> + void __iomem *reg = phy_get_drvdata(phy);
> + u32 val;
> +
> + val = readl(reg);
> + val &= ~EXYNOS_PHY_ENABLE;
> + writel(val, reg);
> +
> + return 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 u32 exynos4210_offsets[] = {
> + 0x0700, /* HDMI_PHY */
> + 0x070C, /* DAC_PHY */
> + 0x0718, /* ADC_PHY */
> + 0x071C, /* PCIE_PHY */
> + 0x0720, /* SATA_PHY */
> + ~0, /* end mark */
> +};
> +
> +static const u32 exynos4412_offsets[] = {
> + 0x0700, /* HDMI_PHY */
> + 0x0718, /* ADC_PHY */
> + ~0, /* end mark */
> +};
Why have you selected only these registers?
According to specs Exynos 4210 has 9 and 4412 has 7 control registers
with 'phy-enable' functionality.
I guess MIPI would require little more work as it has also reset bits,
but it will be still better than separate driver.
> +
> +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},
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_phy_of_match);
> +
> +static struct phy *exynos_phy_xlate(struct device *dev,
> + struct of_phandle_args *args)
> +{
> + struct phy **phys = dev_get_drvdata(dev);
> + int index = args->args[0];
> + int i;
> +
> + /* verify if index is valid */
> + for (i = 0; i <= index; ++i)
> + if (!phys[i])
> + return ERR_PTR(-ENODEV);
> +
> + return 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 u32 *offsets = of_id->data;
> + int count;
> + struct device *dev = &pdev->dev;
> + struct phy **phys;
> + struct resource *res;
> + void __iomem *regs;
> + int i;
> + struct phy_provider *phy_provider;
> +
> + /* count number of phys to create */
> + for (count = 0; offsets[count] != ~0; ++count)
> + ;
count = ARRAY_SIZE(offsets) - 1;
> +
> + phys = devm_kzalloc(dev, (count + 1) * sizeof(phys[0]), GFP_KERNEL);
> + if (!phys)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, phys);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + regs = devm_ioremap(dev, res->start, res->end - res->start);
> + if (!regs) {
> + dev_err(dev, "failed to ioremap registers\n");
> + return -EFAULT;
> + }
Why not devm_ioremap_resource? If not, resource_size function calculates
length of resource correctly.
Anyway I like the idea of implementing multiple phys in one driver.
The only drawback I see is that some phys will be created even there are
no consumers for them. To avoid such situation you can try to use
lazy approach - create phy only if there is request for it,
exynos_phy_xlate callback should allow this.
Regards
Andrzej
> +
> + /* NOTE: last entry in phys[] is NULL */
> + for (i = 0; i < count; ++i) {
> + phys[i] = devm_phy_create(dev, &exynos_phy_ops, NULL);
> + if (IS_ERR(phys[i])) {
> + dev_err(dev, "failed to create PHY %d\n", i);
> + return PTR_ERR(phys[i]);
> + }
> + phy_set_drvdata(phys[i], regs + offsets[i]);
> + }
> +
> + 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, "added %d phys\n", count);
> +
> + 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 <[email protected]>");
> +MODULE_LICENSE("GPL v2");
>
Hi Tomasz,
On 9 April 2014 14:07, Andrzej Hajda <[email protected]> wrote:
> Hi Tomasz,
>
> On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
>> Add exynos-simple-phy driver to support a single register
>> PHY interfaces present on Exynos4 SoC.
>>
>> Signed-off-by: Tomasz Stanislawski <[email protected]>
>> ---
>> .../devicetree/bindings/phy/samsung-phy.txt | 24 +++
>> drivers/phy/Kconfig | 5 +
>> drivers/phy/Makefile | 1 +
>> drivers/phy/exynos-simple-phy.c | 154 ++++++++++++++++++++
>> 4 files changed, 184 insertions(+)
>> create mode 100644 drivers/phy/exynos-simple-phy.c
>>
>> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> index b422e38..f97c4c3 100644
>> --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> @@ -114,3 +114,27 @@ Example:
>> compatible = "samsung,exynos-sataphy-i2c";
>> reg = <0x38>;
>> };
>> +
>> +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"
>> +- reg : offset and length of the register set;
>> +- #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 its meaning is as follows:
>> + 0 - HDMI PHY,
>> + 1 - DAC PHY,
>> + 2 - ADC PHY,
>> + 3 - PCIE PHY.
>> + 4 - SATA PHY.
>> +
>> +For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in
>> +the PHY specifier identifies the PHY and its meaning is as follows:
>> + 0 - HDMI PHY,
>> + 1 - ADC PHY,
>
> What about using preprocessor macros?
>
>> +
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 3bb05f1..65ab783 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -166,4 +166,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 2faf78e..88c5b60 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -18,3 +18,4 @@ obj-$(CONFIG_PHY_EXYNOS4210_USB2) += phy-exynos4210-usb2.o
>> obj-$(CONFIG_PHY_EXYNOS4X12_USB2) += phy-exynos4x12-usb2.o
>> obj-$(CONFIG_PHY_EXYNOS5250_USB2) += phy-exynos5250-usb2.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..57ad338
>> --- /dev/null
>> +++ b/drivers/phy/exynos-simple-phy.c
>> @@ -0,0 +1,154 @@
>> +/*
>> + * Exynos Simple PHY driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Author: Tomasz Stanislawski <[email protected]>
>> + *
>> + * 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/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/phy/phy.h>
>> +
>> +#define EXYNOS_PHY_ENABLE (1 << 0)
>> +
>> +static int exynos_phy_power_on(struct phy *phy)
>> +{
>> + void __iomem *reg = phy_get_drvdata(phy);
>> + u32 val;
>> +
>> + val = readl(reg);
>> + val |= EXYNOS_PHY_ENABLE;
>> + writel(val, reg);
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_phy_power_off(struct phy *phy)
>> +{
>> + void __iomem *reg = phy_get_drvdata(phy);
>> + u32 val;
>> +
>> + val = readl(reg);
>> + val &= ~EXYNOS_PHY_ENABLE;
>> + writel(val, reg);
>> +
>> + return 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 u32 exynos4210_offsets[] = {
>> + 0x0700, /* HDMI_PHY */
>> + 0x070C, /* DAC_PHY */
>> + 0x0718, /* ADC_PHY */
>> + 0x071C, /* PCIE_PHY */
>> + 0x0720, /* SATA_PHY */
>> + ~0, /* end mark */
>> +};
>> +
>> +static const u32 exynos4412_offsets[] = {
>> + 0x0700, /* HDMI_PHY */
>> + 0x0718, /* ADC_PHY */
>> + ~0, /* end mark */
>> +};
>
> Why have you selected only these registers?
> According to specs Exynos 4210 has 9 and 4412 has 7 control registers
> with 'phy-enable' functionality.
> I guess MIPI would require little more work as it has also reset bits,
> but it will be still better than separate driver.
>
>> +
>> +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},
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_phy_of_match);
>> +
>> +static struct phy *exynos_phy_xlate(struct device *dev,
>> + struct of_phandle_args *args)
>> +{
>> + struct phy **phys = dev_get_drvdata(dev);
>> + int index = args->args[0];
>> + int i;
>> +
>> + /* verify if index is valid */
>> + for (i = 0; i <= index; ++i)
>> + if (!phys[i])
>> + return ERR_PTR(-ENODEV);
>> +
>> + return 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 u32 *offsets = of_id->data;
>> + int count;
>> + struct device *dev = &pdev->dev;
>> + struct phy **phys;
>> + struct resource *res;
>> + void __iomem *regs;
>> + int i;
>> + struct phy_provider *phy_provider;
>> +
>> + /* count number of phys to create */
>> + for (count = 0; offsets[count] != ~0; ++count)
>> + ;
>
> count = ARRAY_SIZE(offsets) - 1;
>
>> +
>> + phys = devm_kzalloc(dev, (count + 1) * sizeof(phys[0]), GFP_KERNEL);
>> + if (!phys)
>> + return -ENOMEM;
>> +
>> + dev_set_drvdata(dev, phys);
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + regs = devm_ioremap(dev, res->start, res->end - res->start);
>> + if (!regs) {
>> + dev_err(dev, "failed to ioremap registers\n");
>> + return -EFAULT;
>> + }
>
> Why not devm_ioremap_resource? If not, resource_size function calculates
> length of resource correctly.
>
> Anyway I like the idea of implementing multiple phys in one driver.
> The only drawback I see is that some phys will be created even there are
> no consumers for them. To avoid such situation you can try to use
> lazy approach - create phy only if there is request for it,
> exynos_phy_xlate callback should allow this.
>
> Regards
> Andrzej
>
Idea looks good. How about keeping compatible which is independent
of SoC, something like "samsung,exynos-simple-phy" and provide Reg
and Bit through phy provider node. This way we can avoid SoC specific
hardcoding in phy driver and don't need to look into dt bindings for
each new SoC.
We can use syscon interface to access PMU bits like USB phy.
PMU is already registered as system controller
Regards,
Rahul Sharma.
>> +
>> + /* NOTE: last entry in phys[] is NULL */
>> + for (i = 0; i < count; ++i) {
>> + phys[i] = devm_phy_create(dev, &exynos_phy_ops, NULL);
>> + if (IS_ERR(phys[i])) {
>> + dev_err(dev, "failed to create PHY %d\n", i);
>> + return PTR_ERR(phys[i]);
>> + }
>> + phy_set_drvdata(phys[i], regs + offsets[i]);
>> + }
>> +
>> + 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, "added %d phys\n", count);
>> +
>> + 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 <[email protected]>");
>> +MODULE_LICENSE("GPL v2");
>>
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On 09/04/14 11:12, Rahul Sharma wrote:
> Idea looks good. How about keeping compatible which is independent
> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
> and Bit through phy provider node. This way we can avoid SoC specific
> hardcoding in phy driver and don't need to look into dt bindings for
> each new SoC.
I believe it is a not recommended approach.
> We can use syscon interface to access PMU bits like USB phy.
> PMU is already registered as system controller
Yes, that sounds good. This way we could avoid overlapping memory
mapped register regions specified in 'reg' properties in the device
tree.
--
Thanks,
Sylwester
Hi Tomasz,
On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
> 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 power controller register has also bits to control HDMI clock
divider ratio. I guess current drivers do not change it, but how do you
want to implement access to it if some HDMI driver in the future will
need to change ratio. I guess in case of clk it would be easier.
Regards
Andrzej
Hi Andrzej,
On 9 April 2014 16:00, Andrzej Hajda <[email protected]> wrote:
> Hi Tomasz,
>
> On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
>> 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 power controller register has also bits to control HDMI clock
> divider ratio. I guess current drivers do not change it, but how do you
> want to implement access to it if some HDMI driver in the future will
> need to change ratio. I guess in case of clk it would be easier.
If it is really required to change this divider, it should be registered as
a clock provider in clock driver exposing single divider clock.
Regards,
Rahul Sharma
>
> Regards
> Andrzej
>
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Andrzej,
This issue could be solved by exporting a regmap from PMU driver
or Exynos clock provider for the usage by exynos-simple-phy.
The operations on PHYs from exynos-simple-phy provider would
be chained to PMU driver and protected by a spinlock in the regmap.
Luckily, the divider is not used as far as I know.
Regards,
Tomasz Stanislawski
On 04/09/2014 12:30 PM, Andrzej Hajda wrote:
> Hi Tomasz,
>
> On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
>> 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 power controller register has also bits to control HDMI clock
> divider ratio. I guess current drivers do not change it, but how do you
> want to implement access to it if some HDMI driver in the future will
> need to change ratio. I guess in case of clk it would be easier.
>
> Regards
> Andrzej
>
>
Hi Rahul,
On 04/09/2014 11:12 AM, Rahul Sharma wrote:
> Hi Tomasz,
>
> On 9 April 2014 14:07, Andrzej Hajda <[email protected]> wrote:
>> Hi Tomasz,
>>
>> On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
>>> Add exynos-simple-phy driver to support a single register
>>> PHY interfaces present on Exynos4 SoC.
>>>
>>> Signed-off-by: Tomasz Stanislawski <[email protected]>
[snip]
>>> +
>>> + regs = devm_ioremap(dev, res->start, res->end - res->start);
>>> + if (!regs) {
>>> + dev_err(dev, "failed to ioremap registers\n");
>>> + return -EFAULT;
>>> + }
>>
>> Why not devm_ioremap_resource? If not, resource_size function calculates
>> length of resource correctly.
>>
>> Anyway I like the idea of implementing multiple phys in one driver.
>> The only drawback I see is that some phys will be created even there are
>> no consumers for them. To avoid such situation you can try to use
>> lazy approach - create phy only if there is request for it,
>> exynos_phy_xlate callback should allow this.
>>
>> Regards
>> Andrzej
>>
>
> Idea looks good. How about keeping compatible which is independent
> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
> and Bit through phy provider node. This way we can avoid SoC specific
> hardcoding in phy driver and don't need to look into dt bindings for
> each new SoC.
A very nice idea BUT there is a very strong pressure from DT guys
to avoid adding any bit fields/offsets/masks in DT nodes.
Hopefully, as long as driver name starts with "exynos-" prefix
one can hide SoCs specific tricks deep inside driver code.
The idea behind this driver was not to create a generic phy for 1-bit
devices but rather to hide SoC-specific issues from client drivers
like DRM-HDMI.
>
> We can use syscon interface to access PMU bits like USB phy.
> PMU is already registered as system controller
>
Ok. I will try to use it in PATCHv3.
> Regards,
> Rahul Sharma.
>
Regards,
Tomasz Stanislawski
Hello Andrzej,
On 09.04.2014 10:37, Andrzej Hajda wrote:
>> +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 u32 *offsets = of_id->data;
>> + int count;
>> + struct device *dev = &pdev->dev;
>> + struct phy **phys;
>> + struct resource *res;
>> + void __iomem *regs;
>> + int i;
>> + struct phy_provider *phy_provider;
>> +
>> + /* count number of phys to create */
>> + for (count = 0; offsets[count] != ~0; ++count)
>> + ;
>
> count = ARRAY_SIZE(offsets) - 1;
u32 *offsets is not an array.
Regards,
Andreas
Hi Tomasz,
I have tested your patches for exynos5250 and 5420. Works fine. Are
you planning to post v3? If you want I can share hand with you for v3.
Regards,
Rahul Sharma
On 9 April 2014 17:17, Andreas Oberritter <[email protected]> wrote:
> Hello Andrzej,
>
> On 09.04.2014 10:37, Andrzej Hajda wrote:
>>> +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 u32 *offsets = of_id->data;
>>> + int count;
>>> + struct device *dev = &pdev->dev;
>>> + struct phy **phys;
>>> + struct resource *res;
>>> + void __iomem *regs;
>>> + int i;
>>> + struct phy_provider *phy_provider;
>>> +
>>> + /* count number of phys to create */
>>> + for (count = 0; offsets[count] != ~0; ++count)
>>> + ;
>>
>> count = ARRAY_SIZE(offsets) - 1;
>
> u32 *offsets is not an array.
>
> Regards,
> Andreas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rahul,
I will prepare we v3 version.
Do you want me to add your patches for exynos5?50 to the patchset?
Regards,
Tomasz Stanislawski
On 04/30/2014 08:37 AM, Rahul Sharma wrote:
> Hi Tomasz,
>
> I have tested your patches for exynos5250 and 5420. Works fine. Are
> you planning to post v3? If you want I can share hand with you for v3.
>
> Regards,
> Rahul Sharma
>
> On 9 April 2014 17:17, Andreas Oberritter <[email protected]> wrote:
>> Hello Andrzej,
>>
>> On 09.04.2014 10:37, Andrzej Hajda wrote:
>>>> +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 u32 *offsets = of_id->data;
>>>> + int count;
>>>> + struct device *dev = &pdev->dev;
>>>> + struct phy **phys;
>>>> + struct resource *res;
>>>> + void __iomem *regs;
>>>> + int i;
>>>> + struct phy_provider *phy_provider;
>>>> +
>>>> + /* count number of phys to create */
>>>> + for (count = 0; offsets[count] != ~0; ++count)
>>>> + ;
>>>
>>> count = ARRAY_SIZE(offsets) - 1;
>>
>> u32 *offsets is not an array.
>>
>> Regards,
>> Andreas
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Sure (5250, 5420). I will wait for the same to update DT patches, if any.
Regards,
Rahul Sharma.
On 30 April 2014 14:02, Tomasz Stanislawski <[email protected]> wrote:
> Hi Rahul,
> I will prepare we v3 version.
> Do you want me to add your patches for exynos5?50 to the patchset?
> Regards,
> Tomasz Stanislawski
>
> On 04/30/2014 08:37 AM, Rahul Sharma wrote:
>> Hi Tomasz,
>>
>> I have tested your patches for exynos5250 and 5420. Works fine. Are
>> you planning to post v3? If you want I can share hand with you for v3.
>>
>> Regards,
>> Rahul Sharma
>>
>> On 9 April 2014 17:17, Andreas Oberritter <[email protected]> wrote:
>>> Hello Andrzej,
>>>
>>> On 09.04.2014 10:37, Andrzej Hajda wrote:
>>>>> +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 u32 *offsets = of_id->data;
>>>>> + int count;
>>>>> + struct device *dev = &pdev->dev;
>>>>> + struct phy **phys;
>>>>> + struct resource *res;
>>>>> + void __iomem *regs;
>>>>> + int i;
>>>>> + struct phy_provider *phy_provider;
>>>>> +
>>>>> + /* count number of phys to create */
>>>>> + for (count = 0; offsets[count] != ~0; ++count)
>>>>> + ;
>>>>
>>>> count = ARRAY_SIZE(offsets) - 1;
>>>
>>> u32 *offsets is not an array.
>>>
>>> Regards,
>>> Andreas
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>