Re: [PATCH] drm/msm/dp: check core_initialized flag at both host_init() and host_deinit()

[Abhinav Kumar]

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@xxxxxxxxxxx>
>
> The code looks OK to me, so
>
> Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
>
> once the commit text is cleaned up to indicate the proper function
> names.

Thanks , will fix all those.