Re: [PATCH v4 5/5] clk: qcom: Add Global Clock controller (GCC) driver for SC7180

[Rob Clark <robdclark@chromium.org>]

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

On Fri, Nov 8, 2019 at 10:42 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Rob Clark (2019-11-08 08:54:23)
> > On Thu, Nov 7, 2019 at 10:35 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Rob Clark (2019-11-07 18:06:19)
> > > > On Thu, Nov 7, 2019 at 1:06 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > >
> > > > >
> > > > > NULL is a valid clk pointer returned by clk_get(). What is the display
> > > > > driver doing that makes it consider NULL an error?
> > > > >
> > > >
> > > > do we not have an iface clk?  I think the driver assumes we should
> > > > have one, rather than it being an optional thing.. we could ofc change
> > > > that
> > >
> > > I think some sort of AHB clk is always enabled so the plan is to just
> > > hand back NULL to the caller when they call clk_get() on it and nobody
> > > should be the wiser when calling clk APIs with a NULL iface clk. The
> > > common clk APIs typically just return 0 and move along. Of course, we'll
> > > also turn the clk on in the clk driver so that hardware can function
> > > properly, but we don't need to expose it as a clk object and all that
> > > stuff if we're literally just slamming a bit somewhere and never looking
> > > back.
> > >
> > > But it sounds like we can't return NULL for this clk for some reason? I
> > > haven't tried to track it down yet but I think Matthias has found it
> > > causes some sort of problem in the display driver.
> > >
> >
> > ok, I guess we can change the dpu code to allow NULL..  but what would
> > the return be, for example on a different SoC where we do have an
> > iface clk, but the clk driver isn't enabled?  Would that also return
> > NULL?  I guess it would be nice to differentiate between those cases..
> >
>
> So the scenario is DT describes the clk
>
>  dpu_node {
>      clocks = <&cc AHB_CLK>;
>      clock-names = "iface";
>  }
>
> but the &cc node has a driver that doesn't probe?
>
> I believe in this scenario we return -EPROBE_DEFER because we assume we
> should wait for the clk driver to probe and provide the iface clk. See
> of_clk_get_hw_from_clkspec() and how it looks through a list of clk
> providers and tries to match the &cc phandle to some provider.
>
> Once the driver probes, the match will happen and we'll be able to look
> up the clk in the provider with __of_clk_get_hw_from_provider(). If
> the clk provider decides that there isn't a clk object, it will return
> NULL and then eventually clk_hw_create_clk() will turn the NULL return
> value into a NULL pointer to return from clk_get().
>

ok, that was the scenario I was worried about (since unclk'd register
access tends to be insta-reboot and hard to debug)..  so I think it
should be ok to make dpu just ignore NULL clks.

From a quick look, I think something like the attached (untested).
(Sorry, I'd just paste it inline but gmail somehow eats all the
whitespace when I do that :-/)

[-- Attachment #2: 0001-drm-msm-dpu-ignore-NULL-clocks.patch --]
[-- Type: text/x-patch, Size: 2061 bytes --]

From 0a31adb5994d5df4c3393687b4c6084005502247 Mon Sep 17 00:00:00 2001
From: Rob Clark <robdclark@chromium.org>
Date: Fri, 8 Nov 2019 11:38:46 -0800
Subject: [PATCH] drm/msm/dpu: ignore NULL clocks

This isn't an error.  Also the clk APIs handle the NULL case, so we can
just delete the check.

Change-Id: Ib2725a44a0ab070e44e0c3da5eac9189992a4517
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 26 ++++++---------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
index 27fbeb504362..ec1437b0ef75 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
@@ -93,19 +93,12 @@ int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable)
 			DEV_DBG("%pS->%s: enable '%s'\n",
 				__builtin_return_address(0), __func__,
 				clk_arry[i].clk_name);
-			if (clk_arry[i].clk) {
-				rc = clk_prepare_enable(clk_arry[i].clk);
-				if (rc)
-					DEV_ERR("%pS->%s: %s en fail. rc=%d\n",
-						__builtin_return_address(0),
-						__func__,
-						clk_arry[i].clk_name, rc);
-			} else {
-				DEV_ERR("%pS->%s: '%s' is not available\n",
-					__builtin_return_address(0), __func__,
-					clk_arry[i].clk_name);
-				rc = -EPERM;
-			}
+			rc = clk_prepare_enable(clk_arry[i].clk);
+			if (rc)
+				DEV_ERR("%pS->%s: %s en fail. rc=%d\n",
+					__builtin_return_address(0),
+					__func__,
+					clk_arry[i].clk_name, rc);
 
 			if (rc && i) {
 				msm_dss_enable_clk(&clk_arry[i - 1],
@@ -119,12 +112,7 @@ int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable)
 				__builtin_return_address(0), __func__,
 				clk_arry[i].clk_name);
 
-			if (clk_arry[i].clk)
-				clk_disable_unprepare(clk_arry[i].clk);
-			else
-				DEV_ERR("%pS->%s: '%s' is not available\n",
-					__builtin_return_address(0), __func__,
-					clk_arry[i].clk_name);
+			clk_disable_unprepare(clk_arry[i].clk);
 		}
 	}
 
-- 
2.24.0.432.g9d3f5f5b63-goog