Message-ID: <846439be-77db-7311-3bb3-3647dc6d0622@quicinc.com>
Date:   Mon, 27 Feb 2023 12:05:53 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [PATCH] drm/msm/dp: check core_initialized flag at both
 host_init() and host_deinit()
Content-Language: en-US
To:     Stephen Boyd <swboyd@chromium.org>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>, <agross@kernel.org>,
        <airlied@gmail.com>, <andersson@kernel.org>, <daniel@ffwll.ch>,
        <dianders@chromium.org>, <dmitry.baryshkov@linaro.org>,
        <robdclark@gmail.com>, <sean@poorly.run>, <vkoul@kernel.org>
CC:     <quic_sbillaka@quicinc.com>, <linux-arm-msm@vger.kernel.org>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        <freedreno@lists.freedesktop.org>
References: <1677263398-13801-1-git-send-email-quic_khsieh@quicinc.com>
 <CAE-0n514WpXDhE17DzqF9X7DYg_3DYuYSGzhvx-=eXVJpgq3Yg@mail.gmail.com>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAE-0n514WpXDhE17DzqF9X7DYg_3DYuYSGzhvx-=eXVJpgq3Yg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Stephen

On 2/27/2023 11:53 AM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2023-02-24 10:29:58)
>> There is a reboot/suspend test case where system suspend is forced during
>> system booting up. Since host_init() of external DP is executed at hpd
> 
> dp_display_host_init()?
> 
>> thread context, this test case may created a scenario that host_deinit()
> 
> dp_display_host_deinit()?
> 
ack for both.
>> from pm_suspend() run before host_init() if hpd thread has no chance to
>> run during booting up while suspend request command was issued.
>> At this scenario system will crash at aux register access at host_deinit()
>> since aux clock had not yet been enabled by host_init().  Therefore we
> 
> The aux clk is enabled in dp_power_clk_enable() right? Can you clarify?
> 

Yes, thats right. Its mapped to core_*** in the dts hence the name goes 
to the DP_CORE_PM case in the dp_power_clk_enable()

3092 				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
3093 					 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
3094 					 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
3095 					 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
3096 					 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
3097 				clock-names = "core_iface", "core_aux"

>> have to ensure aux clock enabled by checking core_initialized flag before
>> access aux registers at pm_suspend.
> 
> I'd much more like to get rid of 'core_initialized'. What is preventing
> us from enabling the power (i.e. dp_power_init()), or at least enough
> clks and pm runtime state, during probe? That would fix this problem and
> also clean things up. As I understand, the device is half initialized in
> probe and half initialized in the kthread. If we put all power
> management into the runtime PM ops and synced that state during probe so
> that runtime PM state matched device probe state we could make runtime
> PM be the only suspend function and then push the power state tracking
> into the device core.
> 

You are correct. https://patchwork.freedesktop.org/patch/523879/ will be 
doing that. Its still in review.

>>
>> Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt received")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> 
> The code looks OK to me, so
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> 
> once the commit text is cleaned up to indicate the proper function
> names.

Thanks , will fix all those.