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
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
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
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
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
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
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
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.
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
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.
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
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.
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
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.
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
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.
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
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.
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
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
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
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
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
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
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
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_EXYNOSor (optionally)
depends on ARCH_EXYNOS || COMPILE_TESTMoreover the generic PHY dependency is missing. It can be fixed with:
select GENERIC_PHY
Yea, I will fix this part.
Regards, Rahul Sharma.
helpSupport 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
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_EXYNOSor (optionally)
depends on ARCH_EXYNOS || COMPILE_TESTMoreover the generic PHY dependency is missing. It can be fixed with:
select GENERIC_PHYYea, I will fix this part.
While at that, change the header of the driver file to *2014*
Thanks Kishon
Regards, Rahul Sharma.
helpSupport 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
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_EXYNOSor (optionally)
depends on ARCH_EXYNOS || COMPILE_TESTMoreover the generic PHY dependency is missing. It can be fixed with:
select GENERIC_PHYYea, 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.
helpSupport 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
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");
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;
dri-devel@lists.freedesktop.org
-
Bartlomiej Zolnierkiewicz -
Kishon Vijay Abraham I -
Rahul Sharma -
Thierry Reding -
Tomasz Figa -
Tomasz Figa