[PATCH 1/5] clk: inherit clocks enabled by bootloader

[PATCH 1/5] clk: inherit clocks enabled by bootloader

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

The goal here is to support inheriting a display setup by bootloader,
although there may also be some non-display related use-cases.

Rough idea is to add a flag for clks and power domains that might
already be enabled when kernel starts, and which should not be
disabled at late_initcall if the kernel thinks they are "unused".

If bootloader is enabling display, and kernel is using efifb before
real display driver is loaded (potentially from kernel module after
userspace starts, in a typical distro kernel), we don't want to kill
the clocks and power domains that are used by the display before
userspace starts.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/clk/clk.c                | 48 ++++++++++++++++++++++++++++++++
 drivers/clk/qcom/common.c        | 25 +++++++++++++++++
 drivers/clk/qcom/dispcc-sdm845.c | 22 ++++++++-------
 drivers/clk/qcom/gcc-sdm845.c    |  3 +-
 include/linux/clk-provider.h     | 10 +++++++
 5 files changed, 97 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index aa51756fd4d6..14460e87f508 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -66,6 +66,7 @@ struct clk_core {
 	unsigned long		flags;
 	bool			orphan;
 	bool			rpm_enabled;
+	bool			inherit_enabled; /* clock was enabled by bootloader */
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
 	unsigned int		protect_count;
@@ -1166,6 +1167,9 @@ static void clk_unprepare_unused_subtree(struct clk_core *core)
 	hlist_for_each_entry(child, &core->children, child_node)
 		clk_unprepare_unused_subtree(child);
 
+	if (core->inherit_enabled)
+		return;
+
 	if (core->prepare_count)
 		return;
 
@@ -1197,6 +1201,9 @@ static void clk_disable_unused_subtree(struct clk_core *core)
 	hlist_for_each_entry(child, &core->children, child_node)
 		clk_disable_unused_subtree(child);
 
+	if (core->inherit_enabled)
+		return;
+
 	if (core->flags & CLK_OPS_PARENT_ENABLE)
 		clk_core_prepare_enable(core->parent);
 
@@ -1270,6 +1277,45 @@ static int clk_disable_unused(void)
 }
 late_initcall_sync(clk_disable_unused);
 
+/* Ignore CLK_INHERIT_BOOTLOADER clocks enabled by bootloader.  This
+ * gives a debug knob to disable inheriting clks from bootloader, so
+ * that drivers that used to work, when loaded as a module, thanks
+ * to disabling "unused" clocks at late_initcall(), can continue to
+ * work.
+ *
+ * The proper solution is to fix the drivers.
+ */
+static bool clk_ignore_inherited;
+static int __init clk_ignore_inherited_setup(char *__unused)
+{
+	clk_ignore_inherited = true;
+	return 1;
+}
+__setup("clk_ignore_inherited", clk_ignore_inherited_setup);
+
+/* clock and it's parents are already prepared/enabled from bootloader,
+ * so simply record the fact.
+ */
+static void __clk_inherit_enabled(struct clk_core *core)
+{
+	unsigned long parent_rate = 0;
+	core->inherit_enabled = true;
+	if (core->parent) {
+		__clk_inherit_enabled(core->parent);
+		parent_rate = core->parent->rate;
+	}
+	if (core->ops->recalc_rate)
+		core->rate = core->ops->recalc_rate(core->hw, parent_rate);
+}
+
+void clk_inherit_enabled(struct clk *clk)
+{
+	if (clk_ignore_inherited)
+		return;
+	__clk_inherit_enabled(clk->core);
+}
+EXPORT_SYMBOL_GPL(clk_inherit_enabled);
+
 static int clk_core_determine_round_nolock(struct clk_core *core,
 					   struct clk_rate_request *req)
 {
@@ -3302,6 +3348,8 @@ static int __clk_core_init(struct clk_core *core)
 		 * are enabled during init but might not have a parent yet.
 		 */
 		if (parent) {
+			if (orphan->inherit_enabled)
+				__clk_inherit_enabled(parent);
 			/* update the clk tree topology */
 			__clk_set_parent_before(orphan, parent);
 			__clk_set_parent_after(orphan, parent, NULL);
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index a6b2f86112d8..0d542eeef9aa 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -290,6 +290,31 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
+	/*
+	 * Check which of clocks that we inherit state from bootloader
+	 * are enabled, and fixup enable/prepare state (as well as that
+	 * of it's parents).
+	 */
+	for (i = 0; i < num_clks; i++) {
+		struct clk_hw *hw;
+
+		if (!rclks[i])
+			continue;
+
+		hw = &rclks[i]->hw;
+
+		if (!(hw->init->flags & CLK_INHERIT_BOOTLOADER))
+			continue;
+
+		if (!clk_is_enabled_regmap(hw))
+			continue;
+
+		dev_dbg(dev, "%s is enabled from bootloader!\n",
+			hw->init->name);
+
+		clk_inherit_enabled(hw->clk);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
diff --git a/drivers/clk/qcom/dispcc-sdm845.c b/drivers/clk/qcom/dispcc-sdm845.c
index 0cc4909b5dbe..40d7e0ab4340 100644
--- a/drivers/clk/qcom/dispcc-sdm845.c
+++ b/drivers/clk/qcom/dispcc-sdm845.c
@@ -263,6 +263,7 @@ static struct clk_branch disp_cc_mdss_ahb_clk = {
 		.enable_mask = BIT(0),
 		.hw.init = &(struct clk_init_data){
 			.name = "disp_cc_mdss_ahb_clk",
+			.flags = CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -276,6 +277,7 @@ static struct clk_branch disp_cc_mdss_axi_clk = {
 		.enable_mask = BIT(0),
 		.hw.init = &(struct clk_init_data){
 			.name = "disp_cc_mdss_axi_clk",
+			.flags = CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -294,7 +296,7 @@ static struct clk_branch disp_cc_mdss_byte0_clk = {
 				"disp_cc_mdss_byte0_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -330,7 +332,7 @@ static struct clk_branch disp_cc_mdss_byte0_intf_clk = {
 				"disp_cc_mdss_byte0_div_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -349,7 +351,7 @@ static struct clk_branch disp_cc_mdss_byte1_clk = {
 				"disp_cc_mdss_byte1_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -385,7 +387,7 @@ static struct clk_branch disp_cc_mdss_byte1_intf_clk = {
 				"disp_cc_mdss_byte1_div_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -403,7 +405,7 @@ static struct clk_branch disp_cc_mdss_esc0_clk = {
 				"disp_cc_mdss_esc0_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -421,7 +423,7 @@ static struct clk_branch disp_cc_mdss_esc1_clk = {
 				"disp_cc_mdss_esc1_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -439,7 +441,7 @@ static struct clk_branch disp_cc_mdss_mdp_clk = {
 				"disp_cc_mdss_mdp_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -475,7 +477,7 @@ static struct clk_branch disp_cc_mdss_pclk0_clk = {
 				"disp_cc_mdss_pclk0_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -494,7 +496,7 @@ static struct clk_branch disp_cc_mdss_pclk1_clk = {
 				"disp_cc_mdss_pclk1_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -561,7 +563,7 @@ static struct clk_branch disp_cc_mdss_vsync_clk = {
 				"disp_cc_mdss_vsync_clk_src",
 			},
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT,
+			.flags = CLK_SET_RATE_PARENT | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
index 7131dcf9b060..fe2498be7bc7 100644
--- a/drivers/clk/qcom/gcc-sdm845.c
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -1314,7 +1314,7 @@ static struct clk_branch gcc_disp_ahb_clk = {
 		.enable_mask = BIT(0),
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_disp_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
+			.flags = CLK_IS_CRITICAL | CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
@@ -1328,6 +1328,7 @@ static struct clk_branch gcc_disp_axi_clk = {
 		.enable_mask = BIT(0),
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_disp_axi_clk",
+			.flags = CLK_INHERIT_BOOTLOADER,
 			.ops = &clk_branch2_ops,
 		},
 	},
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index bb6118f79784..41b951c8b92b 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -34,6 +34,7 @@
 #define CLK_OPS_PARENT_ENABLE	BIT(12)
 /* duty cycle call may be forwarded to the parent clock */
 #define CLK_DUTY_CYCLE_PARENT	BIT(13)
+#define CLK_INHERIT_BOOTLOADER	BIT(14) /* clk may be enabled from bootloader */
 
 struct clk;
 struct clk_hw;
@@ -349,6 +350,15 @@ void clk_hw_unregister_fixed_rate(struct clk_hw *hw);
 
 void of_fixed_clk_setup(struct device_node *np);
 
+/**
+ * clk_inherit_enabled - update the enable/prepare count of a clock and it's
+ * parents for clock enabled by bootloader.
+ *
+ * Intended to be used by clock drivers to inform the clk core of a clock
+ * that is already running.
+ */
+void clk_inherit_enabled(struct clk *clk);
+
 /**
  * struct clk_gate - gating clock
  *
-- 
2.20.1

Re: [Freedreno] [PATCH 1/5] clk: inherit clocks enabled by bootloader

[Jeffrey Hugo]

On Sun, Jun 30, 2019 at 9:02 AM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> The goal here is to support inheriting a display setup by bootloader,
> although there may also be some non-display related use-cases.
>
> Rough idea is to add a flag for clks and power domains that might
> already be enabled when kernel starts, and which should not be
> disabled at late_initcall if the kernel thinks they are "unused".
>
> If bootloader is enabling display, and kernel is using efifb before
> real display driver is loaded (potentially from kernel module after
> userspace starts, in a typical distro kernel), we don't want to kill
> the clocks and power domains that are used by the display before
> userspace starts.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Seems sane to me.  I'm curious what Stephen Boyd thinks.
I'll try to give it a spin on one of the 835 laptops.

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

Re: [PATCH 1/5] clk: inherit clocks enabled by bootloader

[Eric Anholt]

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

Rob Clark <robdclark@gmail.com> writes:

> From: Rob Clark <robdclark@chromium.org>
>
> The goal here is to support inheriting a display setup by bootloader,
> although there may also be some non-display related use-cases.
>
> Rough idea is to add a flag for clks and power domains that might
> already be enabled when kernel starts, and which should not be
> disabled at late_initcall if the kernel thinks they are "unused".
>
> If bootloader is enabling display, and kernel is using efifb before
> real display driver is loaded (potentially from kernel module after
> userspace starts, in a typical distro kernel), we don't want to kill
> the clocks and power domains that are used by the display before
> userspace starts.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Raspberry Pi is carrying downstream hacks to do similar stuff, and it
would be great to see CCF finally support this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/5] clk: inherit clocks enabled by bootloader

[Rob Clark]

On Mon, Jul 1, 2019 at 11:25 AM Eric Anholt <eric@anholt.net> wrote:
>
> Rob Clark <robdclark@gmail.com> writes:
>
> > From: Rob Clark <robdclark@chromium.org>
> >
> > The goal here is to support inheriting a display setup by bootloader,
> > although there may also be some non-display related use-cases.
> >
> > Rough idea is to add a flag for clks and power domains that might
> > already be enabled when kernel starts, and which should not be
> > disabled at late_initcall if the kernel thinks they are "unused".
> >
> > If bootloader is enabling display, and kernel is using efifb before
> > real display driver is loaded (potentially from kernel module after
> > userspace starts, in a typical distro kernel), we don't want to kill
> > the clocks and power domains that are used by the display before
> > userspace starts.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
>
> Raspberry Pi is carrying downstream hacks to do similar stuff, and it
> would be great to see CCF finally support this.

yeah, both this and the multiple-possible-panel thing are a big source
of downstream hacks on basically every android device too.. :-/

it certainly would be nice to have upstream solutions for these
problems to give downstream hacks a reason not to exist

BR,
-R

[PATCH 2/5] genpd/gdsc: inherit display powerdomain from bootloader

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Mark power domains that may be enabled by bootloader, and which should
not be disabled until a driver takes them over.

This keeps efifb alive until the real driver can be probed.  In a distro
kernel, the driver will most likely built as a module, and not probed
until we get to userspace (after late_initcall)

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/base/power/domain.c      | 10 ++++++++++
 drivers/clk/qcom/dispcc-sdm845.c |  2 +-
 drivers/clk/qcom/gdsc.c          |  5 +++++
 drivers/clk/qcom/gdsc.h          |  1 +
 include/linux/pm_domain.h        |  4 ++++
 5 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 33c30c1e6a30..513a8655fbd7 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -537,6 +537,16 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 			not_suspended++;
 	}
 
+	/*
+	 * If the power domain is enabled by the bootloader (for example
+	 * display enabled by bootloader), but no devices attached yet
+	 * (perhaps because driver built as kernel module), then do not
+	 * suspend.
+	 */
+	if ((genpd->flags & GENPD_FLAG_INHERIT_BL) &&
+		list_empty(&genpd->dev_list))
+		not_suspended++;
+
 	if (not_suspended > 1 || (not_suspended == 1 && !one_dev_on))
 		return -EBUSY;
 
diff --git a/drivers/clk/qcom/dispcc-sdm845.c b/drivers/clk/qcom/dispcc-sdm845.c
index 40d7e0ab4340..1d9365ac2315 100644
--- a/drivers/clk/qcom/dispcc-sdm845.c
+++ b/drivers/clk/qcom/dispcc-sdm845.c
@@ -575,7 +575,7 @@ static struct gdsc mdss_gdsc = {
 		.name = "mdss_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = HW_CTRL | POLL_CFG_GDSCR,
+	.flags = HW_CTRL | POLL_CFG_GDSCR | INHERIT_BL,
 };
 
 static struct clk_regmap *disp_cc_sdm845_clocks[] = {
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a250f59708d8..4639fbeb9a7f 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -331,6 +331,11 @@ static int gdsc_init(struct gdsc *sc)
 	if ((sc->flags & VOTABLE) && on)
 		gdsc_enable(&sc->pd);
 
+	if ((sc->flags & INHERIT_BL) && on) {
+		pr_debug("gdsc: %s is enabled from bootloader!\n", sc->pd.name);
+		sc->pd.flags |= GENPD_FLAG_INHERIT_BL;
+	}
+
 	/* If ALWAYS_ON GDSCs are not ON, turn them ON */
 	if (sc->flags & ALWAYS_ON) {
 		if (!on)
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 64cdc8cf0d4d..c6fe56247399 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -49,6 +49,7 @@ struct gdsc {
 #define AON_RESET	BIT(4)
 #define POLL_CFG_GDSCR	BIT(5)
 #define ALWAYS_ON	BIT(6)
+#define INHERIT_BL	BIT(7)
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 91d9bf497071..5e421afcf6f3 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -55,6 +55,9 @@
  *
  * GENPD_FLAG_RPM_ALWAYS_ON:	Instructs genpd to always keep the PM domain
  *				powered on except for system suspend.
+ *
+ * GENPD_FLAG_INHERIT_BL:	The bootloader has already enabled this power
+ * 				domain.
  */
 #define GENPD_FLAG_PM_CLK	 (1U << 0)
 #define GENPD_FLAG_IRQ_SAFE	 (1U << 1)
@@ -62,6 +65,7 @@
 #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
 #define GENPD_FLAG_CPU_DOMAIN	 (1U << 4)
 #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
+#define GENPD_FLAG_INHERIT_BL	 (1U << 6)
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
-- 
2.20.1

Re: [Freedreno] [PATCH 2/5] genpd/gdsc: inherit display powerdomain from bootloader

[Jeffrey Hugo]

On Sun, Jun 30, 2019 at 9:02 AM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Mark power domains that may be enabled by bootloader, and which should
> not be disabled until a driver takes them over.
>
> This keeps efifb alive until the real driver can be probed.  In a distro
> kernel, the driver will most likely built as a module, and not probed
> until we get to userspace (after late_initcall)
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

[PATCH 3/5] drm/msm/dsi: split clk rate setting and enable

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Prep work for the following patch.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
 drivers/gpu/drm/msm/dsi/dsi_cfg.c  |  3 +++
 drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c | 38 ++++++++++++++++++++++--------
 4 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 4dd2a9a79257..c4e3c4cf96c5 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -178,6 +178,8 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
 int msm_dsi_host_init(struct msm_dsi *msm_dsi);
 int msm_dsi_runtime_suspend(struct device *dev);
 int msm_dsi_runtime_resume(struct device *dev);
+int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host);
+int dsi_link_clk_set_rate_v2(struct msm_dsi_host *msm_host);
 int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
 int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
 void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 9ddf16380289..35e272c27780 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -129,6 +129,7 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
 };
 
 const static struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
+	.link_clk_set_rate = dsi_link_clk_set_rate_v2,
 	.link_clk_enable = dsi_link_clk_enable_v2,
 	.link_clk_disable = dsi_link_clk_disable_v2,
 	.clk_init_ver = dsi_clk_init_v2,
@@ -140,6 +141,7 @@ const static struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
 };
 
 const static struct msm_dsi_host_cfg_ops msm_dsi_6g_host_ops = {
+	.link_clk_set_rate = dsi_link_clk_set_rate_6g,
 	.link_clk_enable = dsi_link_clk_enable_6g,
 	.link_clk_disable = dsi_link_clk_disable_6g,
 	.clk_init_ver = NULL,
@@ -151,6 +153,7 @@ const static struct msm_dsi_host_cfg_ops msm_dsi_6g_host_ops = {
 };
 
 const static struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_host_ops = {
+	.link_clk_set_rate = dsi_link_clk_set_rate_6g,
 	.link_clk_enable = dsi_link_clk_enable_6g,
 	.link_clk_disable = dsi_link_clk_disable_6g,
 	.clk_init_ver = dsi_clk_init_6g_v2,
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
index a6a3d2bad263..7c1bc174537d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
@@ -33,6 +33,7 @@ struct msm_dsi_config {
 };
 
 struct msm_dsi_host_cfg_ops {
+	int (*link_clk_set_rate)(struct msm_dsi_host *msm_host);
 	int (*link_clk_enable)(struct msm_dsi_host *msm_host);
 	void (*link_clk_disable)(struct msm_dsi_host *msm_host);
 	int (*clk_init_ver)(struct msm_dsi_host *msm_host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index d03212ef4853..87119d0afb91 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -503,7 +503,7 @@ int msm_dsi_runtime_resume(struct device *dev)
 	return dsi_bus_clk_enable(msm_host);
 }
 
-int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host)
+int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
 {
 	int ret;
 
@@ -513,13 +513,13 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host)
 	ret = clk_set_rate(msm_host->byte_clk, msm_host->byte_clk_rate);
 	if (ret) {
 		pr_err("%s: Failed to set rate byte clk, %d\n", __func__, ret);
-		goto error;
+		return ret;
 	}
 
 	ret = clk_set_rate(msm_host->pixel_clk, msm_host->pixel_clk_rate);
 	if (ret) {
 		pr_err("%s: Failed to set rate pixel clk, %d\n", __func__, ret);
-		goto error;
+		return ret;
 	}
 
 	if (msm_host->byte_intf_clk) {
@@ -528,10 +528,18 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host)
 		if (ret) {
 			pr_err("%s: Failed to set rate byte intf clk, %d\n",
 			       __func__, ret);
-			goto error;
+			return ret;
 		}
 	}
 
+	return 0;
+}
+
+
+int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host)
+{
+	int ret;
+
 	ret = clk_prepare_enable(msm_host->esc_clk);
 	if (ret) {
 		pr_err("%s: Failed to enable dsi esc clk\n", __func__);
@@ -571,7 +579,7 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host)
 	return ret;
 }
 
-int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host)
+int dsi_link_clk_set_rate_v2(struct msm_dsi_host *msm_host)
 {
 	int ret;
 
@@ -582,27 +590,34 @@ int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host)
 	ret = clk_set_rate(msm_host->byte_clk, msm_host->byte_clk_rate);
 	if (ret) {
 		pr_err("%s: Failed to set rate byte clk, %d\n", __func__, ret);
-		goto error;
+		return ret;
 	}
 
 	ret = clk_set_rate(msm_host->esc_clk, msm_host->esc_clk_rate);
 	if (ret) {
 		pr_err("%s: Failed to set rate esc clk, %d\n", __func__, ret);
-		goto error;
+		return ret;
 	}
 
 	ret = clk_set_rate(msm_host->src_clk, msm_host->src_clk_rate);
 	if (ret) {
 		pr_err("%s: Failed to set rate src clk, %d\n", __func__, ret);
-		goto error;
+		return ret;
 	}
 
 	ret = clk_set_rate(msm_host->pixel_clk, msm_host->pixel_clk_rate);
 	if (ret) {
 		pr_err("%s: Failed to set rate pixel clk, %d\n", __func__, ret);
-		goto error;
+		return ret;
 	}
 
+	return 0;
+}
+
+int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host)
+{
+	int ret;
+
 	ret = clk_prepare_enable(msm_host->byte_clk);
 	if (ret) {
 		pr_err("%s: Failed to enable dsi byte clk\n", __func__);
@@ -1997,6 +2012,7 @@ int msm_dsi_host_xfer_prepare(struct mipi_dsi_host *host,
 	 * mdp clock need to be enabled to receive dsi interrupt
 	 */
 	pm_runtime_get_sync(&msm_host->pdev->dev);
+	cfg_hnd->ops->link_clk_set_rate(msm_host);
 	cfg_hnd->ops->link_clk_enable(msm_host);
 
 	/* TODO: vote for bus bandwidth */
@@ -2345,7 +2361,9 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
 	}
 
 	pm_runtime_get_sync(&msm_host->pdev->dev);
-	ret = cfg_hnd->ops->link_clk_enable(msm_host);
+	ret = cfg_hnd->ops->link_clk_set_rate(msm_host);
+	if (!ret)
+		ret = cfg_hnd->ops->link_clk_enable(msm_host);
 	if (ret) {
 		pr_err("%s: failed to enable link clocks. ret=%d\n",
 		       __func__, ret);
-- 
2.20.1

Re: [Freedreno] [PATCH 3/5] drm/msm/dsi: split clk rate setting and enable

[Jeffrey Hugo]

On Sun, Jun 30, 2019 at 9:03 AM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Prep work for the following patch.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

[PATCH 4/5] drm/msm/dsi: get the clocks into OFF state at init

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Do an extra enable/disable cycle at init, to get the clks into disabled
state in case bootloader left them enabled.

In case they were already enabled, the clk_prepare_enable() has no real
effect, other than getting the enable_count/prepare_count into the right
state so that we can disable clocks in the correct order.  This way we
avoid having stuck clocks when we later want to do a modeset and set the
clock rates.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c         | 18 +++++++++++++++---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c |  1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 87119d0afb91..d6e81f330db4 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -215,8 +215,6 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(
 		goto put_gdsc;
 	}
 
-	pm_runtime_get_sync(dev);
-
 	ret = regulator_enable(gdsc_reg);
 	if (ret) {
 		pr_err("%s: unable to enable gdsc\n", __func__);
@@ -243,7 +241,6 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(
 	clk_disable_unprepare(ahb_clk);
 disable_gdsc:
 	regulator_disable(gdsc_reg);
-	pm_runtime_put_sync(dev);
 put_gdsc:
 	regulator_put(gdsc_reg);
 exit:
@@ -390,6 +387,8 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host)
 				__func__, cfg->bus_clk_names[i], ret);
 			goto exit;
 		}
+
+		clk_prepare_enable(msm_host->bus_clks[i]);
 	}
 
 	/* get link and source clocks */
@@ -436,6 +435,16 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host)
 
 	if (cfg_hnd->ops->clk_init_ver)
 		ret = cfg_hnd->ops->clk_init_ver(msm_host);
+
+	/*
+	 * Do an extra enable/disable sequence initially to ensure the
+	 * clocks are actually off, if left enabled by bootloader..
+	 */
+	ret = cfg_hnd->ops->link_clk_enable(msm_host);
+	if (!ret)
+		cfg_hnd->ops->link_clk_disable(msm_host);
+	ret = 0;
+
 exit:
 	return ret;
 }
@@ -1855,6 +1864,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 	}
 
 	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
 
 	msm_host->cfg_hnd = dsi_get_config(msm_host);
 	if (!msm_host->cfg_hnd) {
@@ -1885,6 +1895,8 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 		goto fail;
 	}
 
+	pm_runtime_put_sync(&pdev->dev);
+
 	msm_host->rx_buf = devm_kzalloc(&pdev->dev, SZ_4K, GFP_KERNEL);
 	if (!msm_host->rx_buf) {
 		ret = -ENOMEM;
diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
index aabab6311043..d0172d8db882 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
@@ -354,6 +354,7 @@ static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm *pll)
 	if (rc)
 		pr_err("DSI PLL(%d) lock failed, status=0x%08x\n",
 		       pll->id, status);
+rc = 0; // HACK, this will fail if PLL already running..
 
 	return rc;
 }
-- 
2.20.1

Re: [PATCH 4/5] drm/msm/dsi: get the clocks into OFF state at init

[Jeffrey Hugo]

On 6/30/2019 9:01 AM, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Do an extra enable/disable cycle at init, to get the clks into disabled
> state in case bootloader left them enabled.
> 
> In case they were already enabled, the clk_prepare_enable() has no real
> effect, other than getting the enable_count/prepare_count into the right
> state so that we can disable clocks in the correct order.  This way we
> avoid having stuck clocks when we later want to do a modeset and set the
> clock rates.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c         | 18 +++++++++++++++---
>   drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c |  1 +
>   2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> index aabab6311043..d0172d8db882 100644
> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> @@ -354,6 +354,7 @@ static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm *pll)
>   	if (rc)
>   		pr_err("DSI PLL(%d) lock failed, status=0x%08x\n",
>   		       pll->id, status);
> +rc = 0; // HACK, this will fail if PLL already running..

Umm, why?  Is this intentional?


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 4/5] drm/msm/dsi: get the clocks into OFF state at init

[Rob Clark]

On Mon, Jul 1, 2019 at 11:37 AM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 6/30/2019 9:01 AM, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Do an extra enable/disable cycle at init, to get the clks into disabled
> > state in case bootloader left them enabled.
> >
> > In case they were already enabled, the clk_prepare_enable() has no real
> > effect, other than getting the enable_count/prepare_count into the right
> > state so that we can disable clocks in the correct order.  This way we
> > avoid having stuck clocks when we later want to do a modeset and set the
> > clock rates.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/dsi/dsi_host.c         | 18 +++++++++++++++---
> >   drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c |  1 +
> >   2 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > index aabab6311043..d0172d8db882 100644
> > --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > @@ -354,6 +354,7 @@ static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm *pll)
> >       if (rc)
> >               pr_err("DSI PLL(%d) lock failed, status=0x%08x\n",
> >                      pll->id, status);
> > +rc = 0; // HACK, this will fail if PLL already running..
>
> Umm, why?  Is this intentional?
>

I need to sort out a proper solution for this.. but PLL lock will fail
if the clk is already running (which, in that case, is fine since it
is already running and locked), which will cause the clk_enable to
fail..

I guess there is some way that I can check that clk is already running
and skip this check..

BR,
-R

Re: [PATCH 4/5] drm/msm/dsi: get the clocks into OFF state at init

[Jeffrey Hugo]

On 7/1/2019 12:58 PM, Rob Clark wrote:
> On Mon, Jul 1, 2019 at 11:37 AM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>>
>> On 6/30/2019 9:01 AM, Rob Clark wrote:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Do an extra enable/disable cycle at init, to get the clks into disabled
>>> state in case bootloader left them enabled.
>>>
>>> In case they were already enabled, the clk_prepare_enable() has no real
>>> effect, other than getting the enable_count/prepare_count into the right
>>> state so that we can disable clocks in the correct order.  This way we
>>> avoid having stuck clocks when we later want to do a modeset and set the
>>> clock rates.
>>>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
>>>    drivers/gpu/drm/msm/dsi/dsi_host.c         | 18 +++++++++++++++---
>>>    drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c |  1 +
>>>    2 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>> index aabab6311043..d0172d8db882 100644
>>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>> @@ -354,6 +354,7 @@ static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm *pll)
>>>        if (rc)
>>>                pr_err("DSI PLL(%d) lock failed, status=0x%08x\n",
>>>                       pll->id, status);
>>> +rc = 0; // HACK, this will fail if PLL already running..
>>
>> Umm, why?  Is this intentional?
>>
> 
> I need to sort out a proper solution for this.. but PLL lock will fail
> if the clk is already running (which, in that case, is fine since it
> is already running and locked), which will cause the clk_enable to
> fail..
> 
> I guess there is some way that I can check that clk is already running
> and skip this check..


I'm sorry, but this makes no sense to me.  What clock are we talking 
about here?

If the pll is locked, the the lock check should just drop through.  If 
the pll cannot lock, you have an issue.  I'm confused as to how any of 
the downstream clocks can actually be running if the pll isn't locked.

I feel like we are not yet on the same page about what situation you 
seem to be in.  Can you describe in exacting detail?

Re: [PATCH 4/5] drm/msm/dsi: get the clocks into OFF state at init

[Rob Clark]

On Mon, Jul 1, 2019 at 12:07 PM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 7/1/2019 12:58 PM, Rob Clark wrote:
> > On Mon, Jul 1, 2019 at 11:37 AM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> >>
> >> On 6/30/2019 9:01 AM, Rob Clark wrote:
> >>> From: Rob Clark <robdclark@chromium.org>
> >>>
> >>> Do an extra enable/disable cycle at init, to get the clks into disabled
> >>> state in case bootloader left them enabled.
> >>>
> >>> In case they were already enabled, the clk_prepare_enable() has no real
> >>> effect, other than getting the enable_count/prepare_count into the right
> >>> state so that we can disable clocks in the correct order.  This way we
> >>> avoid having stuck clocks when we later want to do a modeset and set the
> >>> clock rates.
> >>>
> >>> Signed-off-by: Rob Clark <robdclark@chromium.org>
> >>> ---
> >>>    drivers/gpu/drm/msm/dsi/dsi_host.c         | 18 +++++++++++++++---
> >>>    drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c |  1 +
> >>>    2 files changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>> index aabab6311043..d0172d8db882 100644
> >>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>> @@ -354,6 +354,7 @@ static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm *pll)
> >>>        if (rc)
> >>>                pr_err("DSI PLL(%d) lock failed, status=0x%08x\n",
> >>>                       pll->id, status);
> >>> +rc = 0; // HACK, this will fail if PLL already running..
> >>
> >> Umm, why?  Is this intentional?
> >>
> >
> > I need to sort out a proper solution for this.. but PLL lock will fail
> > if the clk is already running (which, in that case, is fine since it
> > is already running and locked), which will cause the clk_enable to
> > fail..
> >
> > I guess there is some way that I can check that clk is already running
> > and skip this check..
>
>
> I'm sorry, but this makes no sense to me.  What clock are we talking
> about here?
>
> If the pll is locked, the the lock check should just drop through.  If
> the pll cannot lock, you have an issue.  I'm confused as to how any of
> the downstream clocks can actually be running if the pll isn't locked.
>
> I feel like we are not yet on the same page about what situation you
> seem to be in.  Can you describe in exacting detail?

yeah, I'd expect the lock bit to still be set (since the display is
obviously running at that point)..  but I didn't really debug it yet,
I just hacked that in so the clk_enable didn't fail, so that we could
get correct enable/prepare_counts in order to do the
clk_disable_unprepare()..

BR,
-R

Re: [PATCH 4/5] drm/msm/dsi: get the clocks into OFF state at init

[Rob Clark]

On Mon, Jul 1, 2019 at 12:07 PM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 7/1/2019 12:58 PM, Rob Clark wrote:
> > On Mon, Jul 1, 2019 at 11:37 AM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> >>
> >> On 6/30/2019 9:01 AM, Rob Clark wrote:
> >>> From: Rob Clark <robdclark@chromium.org>
> >>>
> >>> Do an extra enable/disable cycle at init, to get the clks into disabled
> >>> state in case bootloader left them enabled.
> >>>
> >>> In case they were already enabled, the clk_prepare_enable() has no real
> >>> effect, other than getting the enable_count/prepare_count into the right
> >>> state so that we can disable clocks in the correct order.  This way we
> >>> avoid having stuck clocks when we later want to do a modeset and set the
> >>> clock rates.
> >>>
> >>> Signed-off-by: Rob Clark <robdclark@chromium.org>
> >>> ---
> >>>    drivers/gpu/drm/msm/dsi/dsi_host.c         | 18 +++++++++++++++---
> >>>    drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c |  1 +
> >>>    2 files changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>> index aabab6311043..d0172d8db882 100644
> >>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>> @@ -354,6 +354,7 @@ static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm *pll)
> >>>        if (rc)
> >>>                pr_err("DSI PLL(%d) lock failed, status=0x%08x\n",
> >>>                       pll->id, status);
> >>> +rc = 0; // HACK, this will fail if PLL already running..
> >>
> >> Umm, why?  Is this intentional?
> >>
> >
> > I need to sort out a proper solution for this.. but PLL lock will fail
> > if the clk is already running (which, in that case, is fine since it
> > is already running and locked), which will cause the clk_enable to
> > fail..
> >
> > I guess there is some way that I can check that clk is already running
> > and skip this check..
>
>
> I'm sorry, but this makes no sense to me.  What clock are we talking
> about here?
>
> If the pll is locked, the the lock check should just drop through.  If
> the pll cannot lock, you have an issue.  I'm confused as to how any of
> the downstream clocks can actually be running if the pll isn't locked.
>
> I feel like we are not yet on the same page about what situation you
> seem to be in.  Can you describe in exacting detail?
>

So, I went back to check some of the kernel logs, and actually the
case where we were hitting the PLL lock fail was -EPROBE_DEFER cases,
so what was happening is the enable/disable cycle would succeed the
first time, but then we'd -EPROBE_DEFER.  Then after a suspend/resume
cycle, we'd try again, but this time pll's were reset to power on
state, and we weren't setting rate.

With the other patchset[1] I sent over the weekend, this should no
longer be a problem so I can drop the hack.

BR,
-R

[1] https://patchwork.freedesktop.org/series/63000/

[PATCH 5/5] drm/bridge: ti-sn65dsi86: support booloader enabled display

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Request the enable gpio ASIS to avoid disabling bridge during probe, if
already enabled.  And if already enabled, defer enabling runpm until
attach to avoid cutting off the power to the bridge.

Once we get to attach, we know panel and drm driver are probed
successfully, so at this point it i s safe to enable runpm and reset the
bridge.  If we do it earlier, we kill efifb (in the case that panel or
drm driver do not probe successfully, giving the user no way to see what
is going on.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 7a046bcdd81b..8bdc33576992 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -257,6 +257,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
 						   .node = NULL,
 						 };
 
+	if (gpiod_get_value(pdata->enable_gpio)) {
+		pm_runtime_enable(pdata->dev);
+		ti_sn_bridge_resume(pdata->dev);
+		ti_sn_bridge_suspend(pdata->dev);
+	}
+
 	ret = drm_connector_init(bridge->dev, &pdata->connector,
 				 &ti_sn_bridge_connector_funcs,
 				 DRM_MODE_CONNECTOR_eDP);
@@ -813,7 +819,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 	dev_set_drvdata(&client->dev, pdata);
 
 	pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",
-					    GPIOD_OUT_LOW);
+					    GPIOD_ASIS);
 	if (IS_ERR(pdata->enable_gpio)) {
 		DRM_ERROR("failed to get enable gpio from DT\n");
 		ret = PTR_ERR(pdata->enable_gpio);
@@ -843,7 +849,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	pm_runtime_enable(pdata->dev);
+	if (!gpiod_get_value(pdata->enable_gpio)) {
+		pm_runtime_enable(pdata->dev);
+	}
 
 	i2c_set_clientdata(client, pdata);
 
-- 
2.20.1

Re: [PATCH 5/5] drm/bridge: ti-sn65dsi86: support booloader enabled display

[Jeffrey Hugo]

On 6/30/2019 9:01 AM, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Request the enable gpio ASIS to avoid disabling bridge during probe, if
> already enabled.  And if already enabled, defer enabling runpm until
> attach to avoid cutting off the power to the bridge.
> 
> Once we get to attach, we know panel and drm driver are probed
> successfully, so at this point it i s safe to enable runpm and reset the

is?

> bridge.  If we do it earlier, we kill efifb (in the case that panel or
> drm driver do not probe successfully, giving the user no way to see what
> is going on.

Where should the missing ")" be?

Re: [PATCH 5/5] drm/bridge: ti-sn65dsi86: support booloader enabled display

[Laurent Pinchart]

Hi Rob,

Thank you for the patch.

On Sun, Jun 30, 2019 at 08:01:43AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Request the enable gpio ASIS to avoid disabling bridge during probe, if
> already enabled.  And if already enabled, defer enabling runpm until
> attach to avoid cutting off the power to the bridge.
> 
> Once we get to attach, we know panel and drm driver are probed
> successfully, so at this point it i s safe to enable runpm and reset the
> bridge.  If we do it earlier, we kill efifb (in the case that panel or
> drm driver do not probe successfully, giving the user no way to see what
> is going on.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 7a046bcdd81b..8bdc33576992 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -257,6 +257,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
>  						   .node = NULL,
>  						 };
>  
> +	if (gpiod_get_value(pdata->enable_gpio)) {
> +		pm_runtime_enable(pdata->dev);

Does this need to be balanced with a pm_runtime_disable() call ? Bridges
can be attached and detached at runtime when reloading the display
controller drivers, so you need to ensure that detach/re-attach cycles
work.

> +		ti_sn_bridge_resume(pdata->dev);
> +		ti_sn_bridge_suspend(pdata->dev);
> +	}
> +
>  	ret = drm_connector_init(bridge->dev, &pdata->connector,
>  				 &ti_sn_bridge_connector_funcs,
>  				 DRM_MODE_CONNECTOR_eDP);
> @@ -813,7 +819,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  	dev_set_drvdata(&client->dev, pdata);
>  
>  	pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",
> -					    GPIOD_OUT_LOW);
> +					    GPIOD_ASIS);
>  	if (IS_ERR(pdata->enable_gpio)) {
>  		DRM_ERROR("failed to get enable gpio from DT\n");
>  		ret = PTR_ERR(pdata->enable_gpio);
> @@ -843,7 +849,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
>  
> -	pm_runtime_enable(pdata->dev);
> +	if (!gpiod_get_value(pdata->enable_gpio)) {
> +		pm_runtime_enable(pdata->dev);
> +	}

If I understand the issue correctly, this is part of an effort to avoid
disabling a potentially display output until we get as close as possible
to display handover, right ? Is there a drawback in always enabling
runtime PM when the bridge is attached instead of at probe time ? I
think we need to come up with a set of rules for bridge driver authors,
otherwise we'll end up with incompatible expectations of bridge drivers
and display controller drivers.

>  
>  	i2c_set_clientdata(client, pdata);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] drm/bridge: ti-sn65dsi86: support booloader enabled display

[Rob Clark]

On Tue, Jul 2, 2019 at 8:20 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Sun, Jun 30, 2019 at 08:01:43AM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Request the enable gpio ASIS to avoid disabling bridge during probe, if
> > already enabled.  And if already enabled, defer enabling runpm until
> > attach to avoid cutting off the power to the bridge.
> >
> > Once we get to attach, we know panel and drm driver are probed
> > successfully, so at this point it i s safe to enable runpm and reset the
> > bridge.  If we do it earlier, we kill efifb (in the case that panel or
> > drm driver do not probe successfully, giving the user no way to see what
> > is going on.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 7a046bcdd81b..8bdc33576992 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -257,6 +257,12 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
> >                                                  .node = NULL,
> >                                                };
> >
> > +     if (gpiod_get_value(pdata->enable_gpio)) {
> > +             pm_runtime_enable(pdata->dev);
>
> Does this need to be balanced with a pm_runtime_disable() call ? Bridges
> can be attached and detached at runtime when reloading the display
> controller drivers, so you need to ensure that detach/re-attach cycles
> work.

It should only be a problem if things don't get shut down properly in
the detach/unload path.

> > +             ti_sn_bridge_resume(pdata->dev);
> > +             ti_sn_bridge_suspend(pdata->dev);
> > +     }
> > +
> >       ret = drm_connector_init(bridge->dev, &pdata->connector,
> >                                &ti_sn_bridge_connector_funcs,
> >                                DRM_MODE_CONNECTOR_eDP);
> > @@ -813,7 +819,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >       dev_set_drvdata(&client->dev, pdata);
> >
> >       pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",
> > -                                         GPIOD_OUT_LOW);
> > +                                         GPIOD_ASIS);
> >       if (IS_ERR(pdata->enable_gpio)) {
> >               DRM_ERROR("failed to get enable gpio from DT\n");
> >               ret = PTR_ERR(pdata->enable_gpio);
> > @@ -843,7 +849,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >       if (ret)
> >               return ret;
> >
> > -     pm_runtime_enable(pdata->dev);
> > +     if (!gpiod_get_value(pdata->enable_gpio)) {
> > +             pm_runtime_enable(pdata->dev);
> > +     }
>
> If I understand the issue correctly, this is part of an effort to avoid
> disabling a potentially display output until we get as close as possible
> to display handover, right ? Is there a drawback in always enabling
> runtime PM when the bridge is attached instead of at probe time ? I
> think we need to come up with a set of rules for bridge driver authors,
> otherwise we'll end up with incompatible expectations of bridge drivers
> and display controller drivers.

That would simplify things slightly.. but perhaps w/ the slight
downside, if things booted with clk running or regulator enabled, but
the panel not actually enabled, then you wouldn't shut things down
until attach.

I'm also about to send a patch that adds debugfs to dump status
registers (and a related fix that I found from that).. which will need
to do a runpm get/put, and could potentially happen before attach (ie.
if bridge driver is probed but drm driver is not).

Maybe those are edge cases not worth worrying about.

I suppose also it is possible that some bridge driver would want to
read out a hw revision register in probe to see if it is a version of
hw that it supports.  But fortunately that is not a problem with this
particular bridge.

In the end, I suspect the first time you bring up some platform with
display running, you are going to have some patches spanning clk /
bridge / display / etc, no matter what we do ;-)

BR,
-R

> >
> >       i2c_set_clientdata(client, pdata);
> >
>
> --
> Regards,
>
> Laurent Pinchart