Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190630150230.7878-1-robdclark@gmail.com> <20190630150230.7878-5-robdclark@gmail.com>
 <75a2921d-bf1a-c4c1-6d9a-122474eface4@codeaurora.org> <CAF6AEGufiSU_sFZFdLH=KT5iCQGwccszURqAQCHd=dhuZafvZg@mail.gmail.com>
 <9fbf9226-578a-90aa-693d-9ea4fcda8281@codeaurora.org>
In-Reply-To: <9fbf9226-578a-90aa-693d-9ea4fcda8281@codeaurora.org>
From:   Rob Clark <robdclark@gmail.com>
Date:   Tue, 2 Jul 2019 06:53:05 -0700
Message-ID: <CAF6AEGuze-mnUQy36UvB-JCp3KUASgTa+0dh4i_AFdmYX0tzuw@mail.gmail.com>
Subject: Re: [PATCH 4/5] drm/msm/dsi: get the clocks into OFF state at init
To:     Jeffrey Hugo <jhugo@codeaurora.org>
Cc:     dri-devel <dri-devel@lists.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        freedreno <freedreno@lists.freedesktop.org>,
        aarch64-laptops@lists.linaro.org,
        linux-clk <linux-clk@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Rob Clark <robdclark@chromium.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Jordan Crouse <jcrouse@codeaurora.org>,
        Abhinav Kumar <abhinavk@codeaurora.org>,
        Sibi Sankar <sibis@codeaurora.org>,
        Mamta Shukla <mamtashukla555@gmail.com>,
        Chandan Uddaraju <chandanu@codeaurora.org>,
        Rajesh Yadav <ryadav@codeaurora.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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/