| Message ID | 1423309988-11793-2-git-send-email-inki.dae@samsung.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
On 07/02/15 12:53, Inki Dae wrote: > This patch fixes the issue that the try to get a phy object is failed > to enable mipi phy. > > System and power management unit registers should be controlled by > syscon framework. So this patch removes existing phy framework based > codes and adds syscon support instead. However, we should support > legacy device tree binding so consider the legacy binding for compatibility. > > In addition, we need to remove below device node and relevant properties, > mipi_phy: video-phy@10020710 { > compatible = "samsung,s5pv210-mipi-video-phy"; > reg = <0x10020710 8>; > #phy-cells = <1>; > }; > > Now camera device node uses mipi_phy node relevant properties like below, > camera { > ... > csis_0: csis@11880000 { > ... > phys = <&mipi_phy 0>; > phy-names = "csis"; > ... > }; > csis_1: csis@11890000 { > ... > phys = <&mipi_phy 2>; > phy-names = "csis"; > ... > }; > ... > }; > > With above, we will find below message while booting, > can't request region for resource [mem 0x10020710-0x10020717] I'm afraid this approach won't work because MIPI DSI Master and MIPI CSI Slave devices share a control bit in the register and it seems impossible to ensure proper locking with current regmap/syscon API. I have submitted patches to fix this issue [1] and they should be already available in linux-next and can be found on linux-samsung-soc ML: [PATCH 1/2] phy: exynos-video-mipi: Fix regression by adding support for PMU regmap [PATCH 2/2] ARM: dts: Add syscon phandle to the video-phy node for Exynos4 The other issue with your approach is that we are moving the PMU details to the MIPI DSIM driver and similar changes would need to be done in the MIPI CSIS driver. Instead I just added syscon support to the PHY layer, it's not perfect but fixes the issue for both DSI and CSI and doesn't strip the PHY layer which could potentially be useful.
On 2015년 02월 09일 19:57, Sylwester Nawrocki wrote: > On 07/02/15 12:53, Inki Dae wrote: >> This patch fixes the issue that the try to get a phy object is failed >> to enable mipi phy. >> >> System and power management unit registers should be controlled by >> syscon framework. So this patch removes existing phy framework based >> codes and adds syscon support instead. However, we should support >> legacy device tree binding so consider the legacy binding for compatibility. >> >> In addition, we need to remove below device node and relevant properties, >> mipi_phy: video-phy@10020710 { >> compatible = "samsung,s5pv210-mipi-video-phy"; >> reg = <0x10020710 8>; >> #phy-cells = <1>; >> }; >> >> Now camera device node uses mipi_phy node relevant properties like below, >> camera { >> ... >> csis_0: csis@11880000 { >> ... >> phys = <&mipi_phy 0>; >> phy-names = "csis"; >> ... >> }; >> csis_1: csis@11890000 { >> ... >> phys = <&mipi_phy 2>; >> phy-names = "csis"; >> ... >> }; >> ... >> }; >> >> With above, we will find below message while booting, >> can't request region for resource [mem 0x10020710-0x10020717] > > I'm afraid this approach won't work because MIPI DSI Master and MIPI CSI > Slave devices share a control bit in the register and it seems impossible > to ensure proper locking with current regmap/syscon API. > > I have submitted patches to fix this issue [1] and they should be already > available in linux-next and can be found on linux-samsung-soc ML: > > [PATCH 1/2] phy: exynos-video-mipi: Fix regression by adding support for PMU regmap > [PATCH 2/2] ARM: dts: Add syscon phandle to the video-phy node for Exynos4 > > The other issue with your approach is that we are moving the PMU details > to the MIPI DSIM driver and similar changes would need to be done in > the MIPI CSIS driver. > > Instead I just added syscon support to the PHY layer, it's not perfect > but fixes the issue for both DSI and CSI and doesn't strip the PHY layer > which could potentially be useful. Ah, Right. I didn't check your patch set. Your way is a better idea than my one. With this, we don't need to change device drivers, MIPI DSI and CSI. Then, what is the meaning that it's not perfect? Thanks, Inki Dae > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/02/15 13:17, Inki Dae wrote: >> Instead I just added syscon support to the PHY layer, it's not perfect >> > but fixes the issue for both DSI and CSI and doesn't strip the PHY layer >> > which could potentially be useful. > > Ah, Right. I didn't check your patch set. Your way is a better idea than > my one. With this, we don't need to change device drivers, MIPI DSI and CSI. > > Then, what is the meaning that it's not perfect? What I didn't like is that there are at least 3 mutexes on the phy_power_on/ phy_power_off path (PHY core, PHY driver, regmap) and there is another level of indirection after introducing the regmap. I guess it's nothing serious though. And BTW the syscon could be converted to use spinlock rather than a mutex, since in our case behind the PMU syscon is always a memory mapped region.
diff --git a/Documentation/devicetree/bindings/video/exynos_dsim.txt b/Documentation/devicetree/bindings/video/exynos_dsim.txt index ca2b4aa..dec3b55 100644 --- a/Documentation/devicetree/bindings/video/exynos_dsim.txt +++ b/Documentation/devicetree/bindings/video/exynos_dsim.txt @@ -11,15 +11,18 @@ Required properties: - clocks: list of clock specifiers, must contain an entry for each required entry in clock-names - clock-names: should include "bus_clk"and "pll_clk" entries - - phys: list of phy specifiers, must contain an entry for each required - entry in phy-names - - phy-names: should include "dsim" entry + - samsung,pmureg: handle to syscon used to control the PMU registers - vddcore-supply: MIPI DSIM Core voltage supply (e.g. 1.1V) - vddio-supply: MIPI DSIM I/O and PLL voltage supply (e.g. 1.8V) - samsung,pll-clock-frequency: specifies frequency of the "pll_clk" clock - #address-cells, #size-cells: should be set respectively to <1> and <0> according to DSI host bindings (see MIPI DSI bindings [1]) +Deprecated properties for MIPI DSI Master: + - phys: list of phy specifiers, must contain an entry for each required + entry in phy-names (Deprecated) + - phy-names: should include "dsim" entry (Deprecated) + Optional properties: - samsung,power-domain: a phandle to DSIM power domain node diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 05fe93d..38b025e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -23,6 +23,8 @@ #include <linux/phy/phy.h> #include <linux/regulator/consumer.h> #include <linux/component.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> #include <video/mipi_display.h> #include <video/videomode.h> @@ -263,6 +265,8 @@ struct exynos_dsi_transfer { struct exynos_dsi_driver_data { unsigned int plltmr_reg; + unsigned int mipi_phy_offset; + unsigned int mipi_phy_en_shift; unsigned int has_freqband:1; unsigned int has_clklane_stop:1; }; @@ -277,6 +281,7 @@ struct exynos_dsi { void __iomem *reg_base; struct phy *phy; + struct regmap *pmureg; struct clk *pll_clk; struct clk *bus_clk; struct regulator_bulk_data supplies[2]; @@ -313,21 +318,29 @@ static struct exynos_dsi_driver_data exynos3_dsi_driver_data = { .plltmr_reg = 0x50, .has_freqband = 1, .has_clklane_stop = 1, + .mipi_phy_offset = 0x710, + .mipi_phy_en_shift = 0, }; static struct exynos_dsi_driver_data exynos4_dsi_driver_data = { .plltmr_reg = 0x50, .has_freqband = 1, .has_clklane_stop = 1, + .mipi_phy_offset = 0x710, + .mipi_phy_en_shift = 0, }; static struct exynos_dsi_driver_data exynos4415_dsi_driver_data = { .plltmr_reg = 0x58, .has_clklane_stop = 1, + .mipi_phy_offset = 0x710, + .mipi_phy_en_shift = 0, }; static struct exynos_dsi_driver_data exynos5_dsi_driver_data = { .plltmr_reg = 0x58, + .mipi_phy_offset = 0x714, + .mipi_phy_en_shift = 0, }; static struct of_device_id exynos_dsi_of_match[] = { @@ -1294,6 +1307,7 @@ static const struct mipi_dsi_host_ops exynos_dsi_ops = { static int exynos_dsi_poweron(struct exynos_dsi *dsi) { + struct exynos_dsi_driver_data *driver_data = dsi->driver_data; int ret; ret = regulator_bulk_enable(ARRAY_SIZE(dsi->supplies), dsi->supplies); @@ -1314,7 +1328,14 @@ static int exynos_dsi_poweron(struct exynos_dsi *dsi) goto err_pll_clk; } - ret = phy_power_on(dsi->phy); + if (dsi->phy) { + ret = phy_power_on(dsi->phy); + } else { + ret = regmap_update_bits(dsi->pmureg, + driver_data->mipi_phy_offset, + 0x1 << driver_data->mipi_phy_en_shift, + 0x1 << driver_data->mipi_phy_en_shift); + } if (ret < 0) { dev_err(dsi->dev, "cannot enable phy %d\n", ret); goto err_phy; @@ -1334,6 +1355,7 @@ err_bus_clk: static void exynos_dsi_poweroff(struct exynos_dsi *dsi) { + struct exynos_dsi_driver_data *driver_data = dsi->driver_data; int ret; usleep_range(10000, 20000); @@ -1348,7 +1370,14 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi) dsi->state &= ~DSIM_STATE_CMD_LPM; - phy_power_off(dsi->phy); + if (dsi->phy) { + phy_power_off(dsi->phy); + } else { + regmap_update_bits(dsi->pmureg, + driver_data->mipi_phy_offset, + 0x0 << driver_data->mipi_phy_en_shift, + 0x0 << driver_data->mipi_phy_en_shift); + } clk_disable_unprepare(dsi->pll_clk); clk_disable_unprepare(dsi->bus_clk); @@ -1742,13 +1771,30 @@ static int exynos_dsi_probe(struct platform_device *pdev) goto err_del_component; } + /* + * Consider legacy device tree binding. + * phy framework isn't used anymore. (Deprecated) + */ dsi->phy = devm_phy_get(dev, "dsim"); - if (IS_ERR(dsi->phy)) { - dev_info(dev, "failed to get dsim phy\n"); + if (!IS_ERR(dsi->phy)) + goto skip_syscon; + + if (PTR_ERR(dsi->phy) == -EPROBE_DEFER) { ret = PTR_ERR(dsi->phy); goto err_del_component; } + dsi->phy = NULL; + + dsi->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, + "samsung,pmureg"); + if (IS_ERR(dsi->pmureg)) { + dev_warn(dev, "failed to get pmu register.\n"); + ret = PTR_ERR(dsi->pmureg); + goto err_del_component; + } + +skip_syscon: dsi->irq = platform_get_irq(pdev, 0); if (dsi->irq < 0) { dev_err(dev, "failed to request dsi irq resource\n");
This patch fixes the issue that the try to get a phy object is failed to enable mipi phy. System and power management unit registers should be controlled by syscon framework. So this patch removes existing phy framework based codes and adds syscon support instead. However, we should support legacy device tree binding so consider the legacy binding for compatibility. In addition, we need to remove below device node and relevant properties, mipi_phy: video-phy@10020710 { compatible = "samsung,s5pv210-mipi-video-phy"; reg = <0x10020710 8>; #phy-cells = <1>; }; Now camera device node uses mipi_phy node relevant properties like below, camera { ... csis_0: csis@11880000 { ... phys = <&mipi_phy 0>; phy-names = "csis"; ... }; csis_1: csis@11890000 { ... phys = <&mipi_phy 2>; phy-names = "csis"; ... }; ... }; With above, we will find below message while booting, can't request region for resource [mem 0x10020710-0x10020717] Signed-off-by: Inki Dae <inki.dae@samsung.com> --- .../devicetree/bindings/video/exynos_dsim.txt | 9 ++-- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 54 ++++++++++++++++++-- 2 files changed, 56 insertions(+), 7 deletions(-)