On Fri, Nov 8, 2019 at 10:42 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Rob Clark (2019-11-08 08:54:23)
> > On Thu, Nov 7, 2019 at 10:35 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> > >
> > > Quoting Rob Clark (2019-11-07 18:06:19)
> > > > On Thu, Nov 7, 2019 at 1:06 PM Stephen Boyd <sboyd@xxxxxxxxxx> 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 :-/)
From 0a31adb5994d5df4c3393687b4c6084005502247 Mon Sep 17 00:00:00 2001
From: Rob Clark <robdclark@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
---
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
