Re: [PATCH v7 03/12] drm/bridge: cdns-dsi: Fix phy de-init and flag it so

[Aradhya Bhatia <aradhya.bhatia@linux.dev>]

Hi,

On 1/14/25 20:50, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/01/2025 16:44, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 1/14/25 18:00, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 14/01/2025 07:56, Aradhya Bhatia wrote:
>>>> From: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>
>>>> The driver code doesn't have a Phy de-initialization path as yet,
>>>> and so
>>>> it does not clear the phy_initialized flag while suspending. This is a
>>>> problem because after resume the driver looks at this flag to determine
>>>> if a Phy re-initialization is required or not. It is in fact required
>>>> because the hardware is resuming from a suspend, but the driver does
>>>> not
>>>> carry out any re-initialization causing the D-Phy to not work at all.
>>>>
>>>> Call the counterparts of phy_init() and phy_power_on(), that are
>>>> phy_exit() and phy_power_off(), from _bridge_disable(), and clear the
>>>> flags so that the Phy can be initialized again when required.
>>>>
>>>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>>>> ---
>>>>    drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> index 056583e81155..039c5eb7fb66 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct
>>>> drm_bridge *bridge)
>>>>        if (dsi->platform_ops && dsi->platform_ops->disable)
>>>>            dsi->platform_ops->disable(dsi);
>>>>    +    dsi->phy_initialized = false;
>>>> +    dsi->link_initialized = false;
>>>> +    phy_power_off(dsi->dphy);
>>>> +    phy_exit(dsi->dphy);
>>>> +
>>>
>>> The phy related lines are counterparts to what's done in
>>> cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(),
>>>
>>> But is the phy_initialized even needed? phy_initialized() is called from
>>> cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the
>>> call in cdns_dsi_bridge_enable() be always skipped, as
>>> cdns_dsi_bridge_pre_enable() already set phy_initialized?
>>
>> Yes, that is how the behavior has been. The initialization calls inside
>> the _bridge_enable() end-up getting skipped.
>>
>> My first thought after reading your comments was to remove the init
>> calls entirely from the _bridge_pre_enable(), and drop the
>> phy_initialized flag too, and let _bridge_enable() only handle the init.
> 
> Isn't that the wrong way around? If currently bridge_pre_enable enables
> the phy, your suggestion above would change that. I would think keeping
> the init calls in bridge_pre_enable, and drop from bridge_enable.
> 
>> The _bridge_enable() will anyway get renamed to _bridge_pre_enable(),
>> while the existing _bridge_pre_enable() will get dropped, by the last
>> patch of this series.
> 
> Ok, but you can't do a fix that'll only be right after some future patch
> does more changes =).

Yeah, that would be wrong. =)

> 
>> But since this patch is intended as a fix, it will get applied to
>> previous versions while that last patch of the series won't... and then
> 
> Speaking of which, I think you should cc: stable for the ones that
> should be applied to earlier kernels. And it would be good to have all
> such patches first in the series, to decrease any dependencies.

Will do!

> 
>> we may end up having init calls only from _bridge_enable() for the older
>> versions.
>> Also, given all the fixes in the series, there is a possibility that an
>> older-version of the driver might become functional (except for the
>> color shift issue).
>>
>> My question then is, would it be a cause for concern if all the init
>> calls are handled from the _bridge_enable() only?
> 
> I'm not sure I follow here. Don't we want the init calls to happen in
> the pre_enable phase, both before and after the sequence change (patch 12)?
> 

It is, now. For that brief period, I was considering to keep them only
in _bridge_enable().

> But generally speaking, yes, it's good to keep fixes simple, and do
> cleanups later on top. Keeping that in mind, maybe this current patch is
> fine as it is. Although... if the init is done in pre_enable, shouldn't
> the deinit be done in post_disable?

Yes, I will move the deinit to _bridge_post_disable().


So, if we keep the fix limited to deinit in _bridge_post_disable(), then
the cleanup would involve dropping the init calls from _bridge_enable().
And then the patch-12 would do 3 things -

	1. Drop older _bridge_pre_enable()
	2. Rename old _bridge_enable() to _bridge_pre_enable()
	3. Since the _old_ _bridge_enable() has the calls dropped in the
	   cleanup patch, add those calls again in the _new_
	   _bridge_pre_enable() (which are really the same function
	   bodies).

Do you think we can instead skip the cleanup patch, as well as #3 from
patch-12?

Fun fact: We already have patch-4 which fixes the order of init calls in
_bridge_enable()! =)

> 
>> (one of the potential concerns detailed below)
>>
>>>
>>> Same question for cdns_dsi_init_link(), although that's also called from
>>> cdns_dsi_transfer(), so we probably need dsi->link_initialized.
>>>
>>
>> Don't you think we'd need the phy to also be initialized for the DCS
>> command to work?
> 
> I'm sure we do. But the driver doesn't do that currently, does it? Which
> I did find a bit odd, but I'm not familiar with the HW.
> 
> However, my comment was related to calling cdns_dsi_init_link() in both
> cdns_dsi_bridge_enable and cdns_dsi_bridge_pre_enable functions. In this
> case the call in the cdns_dsi_bridge_enable function is a no-op, similar
> to calling cdns_dsi_hs_init().
> 
> But, if changed, that's also a cleanup, so maybe better keep away from
> fix patches.
> 
>> Usually, since DSI is among the initial bridges to get pre_enabled, the
>> Link and Phy are both initialized by the time cdns_dsi_transfer() is
>> called. So, even if cdns_dsi_transfer() doesn't call for
>> cdns_dsi_hs_init(), it is able to work fine.
>>
>> If DCS commands do indeed require the cdns_dsi_hs_init(), then shifting
>> it to _bridge_enable() (like I suggested above) would be problematic
>> without fixing it here.
> 
> I don't know what how the HW works, but we definitely need PHY to send
> DCS commands. But we don't necessarily need HS mode, LP works fine
> (usually). It's just not clear to me what exactly cdns_dsi_hs_init() and
> cdns_dsi_init_link() do. What is "link"? Looks like cdns_dsi_init_link
> is doing some PHY stuff, which is kind of strange thing to do, as
> phy_init() and phy_power_on() are only done later.

That is where my confusion is too. A quick look into the TRM didn't
help me with distinctions either.

> 
> In any case, yes, the cdns_dsi_transfer() has to make sure we have LP/HS
> working. So indeed it might mean calling both functions. This is,
> however, perhaps a different topic, best left out of this series.
> 

Alright. Since it is decided to keep the init calls in
_bridge_pre_enable(), cdns_dsi_transfer() is not going to be affected
any more than it already is, and we won't be breaking anything new.

I guess there can be some trial and error done to find whether
cdns_dsi_transfer() is really dependent on cdns_dsi_hs_init() -
but yes, let's keep that out of this series' scope.


Regards
Aradhya