Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <58cff9e3-2488-5119-40ba-070ae930906d@ideasonboard.com>
Date:   Wed, 30 Nov 2022 10:56:12 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] drm/bridge: cdns-dsi: Fix issue with phy init
Content-Language: en-US
To:     Rahul T R <r-ravikumar@ti.com>, dri-devel@lists.freedesktop.org
Cc:     andrzej.hajda@intel.com, neil.armstrong@linaro.org,
        robert.foss@linaro.org, Laurent.pinchart@ideasonboard.com,
        jonas@kwiboo.se, jernej.skrabec@gmail.com, airlied@gmail.com,
        daniel@ffwll.ch, p.zabel@pengutronix.de,
        linux-kernel@vger.kernel.org, jpawar@cadence.com,
        sjakhade@cadence.com
References: <20221115083918.23192-1-r-ravikumar@ti.com>
From:   Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
In-Reply-To: <20221115083918.23192-1-r-ravikumar@ti.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 15/11/2022 10:39, Rahul T R wrote:
> Phy is not being initialized after suspend resume. Fix this by setting
> phy_initialized flag to false in suspend callback
> 
> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> ---
>   drivers/gpu/drm/bridge/cdns-dsi.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
> index 20bece84ff8c..1a988f53424a 100644
> --- a/drivers/gpu/drm/bridge/cdns-dsi.c
> +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> @@ -1187,6 +1187,7 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
>   	clk_disable_unprepare(dsi->dsi_p_clk);
>   	reset_control_assert(dsi->dsi_p_rst);
>   	dsi->link_initialized = false;
> +	dsi->phy_initialized = false;
>   	return 0;
>   }
>   

I'm not familiar with the IP, but the code related to enable/disable 
looks a bit odd.

Why does cdns_dsi_bridge_enable() do:
	cdns_dsi_hs_init(dsi);
	cdns_dsi_init_link(dsi);
but then in cdns_dsi_bridge_pre_enable():
	cdns_dsi_init_link(dsi);
	cdns_dsi_hs_init(dsi);

Doesn't the order matter? Why are the same functions called in both places?

cdns_dsi_hs_init() seems to do enabling, like locking the PLL. But 
there's no counterpart, hs_uninit(). I see cdns_dsi_bridge_disable() 
doing some clearing of the registers, so perhaps that's where the 
disabling happens. But cdns_dsi_hs_init() is called from the pre-enable, 
and post-disable doesn't do anything else but pm_runtime_put().

More or less the same comments apply to cdns_dsi_init_link(), but it's a 
bit worse as it's also called in cdns_dsi_transfer(), and then there's 
no uninit counterpart that I can see.

Well, maybe both functions are only doing configuration, not enabling 
this as such, and so it's fine to just turn off the IP without any 
uninit step. If that's the case, then probably this patch is fine.

  Tomi