diff mbox series

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

Message ID 20190630150230.7878-5-robdclark@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show
Series drm+clk+genpd: support for bootloader enabled display | expand

Commit Message

Rob Clark June 30, 2019, 3:01 p.m. UTC 
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(-)

Comments

Jeffrey Hugo July 1, 2019, 6:37 p.m. UTC | #1 
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?
Rob Clark July 1, 2019, 6:58 p.m. UTC | #2 
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
Jeffrey Hugo July 1, 2019, 7:07 p.m. UTC | #3 
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?
Rob Clark July 1, 2019, 7:34 p.m. UTC | #4 
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
Rob Clark July 2, 2019, 1:53 p.m. UTC | #5 
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/
diff mbox series

Patch

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;
 }