Re: [PATCH 04/10] drm/tidss: Move reset to the end of dispc_init()

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 06/11/2023 00:54, Laurent Pinchart wrote:
> On Thu, Nov 02, 2023 at 08:40:10AM +0200, Tomi Valkeinen wrote:
>> On 01/11/2023 15:57, Laurent Pinchart wrote:
>>> On Wed, Nov 01, 2023 at 11:17:41AM +0200, Tomi Valkeinen wrote:
>>>> We do a DSS reset in the middle of the dispc_init(). While that happens
>>>> to work now, we should really make sure that e..g the fclk, which is
>>>> acquired only later in the function, is enabled when doing a reset. This
>>>> will be handled in a later patch, but for now, let's move the
>>>> dispc_softreset() call to the end of dispc_init(), which is a sensible
>>>> place for it anyway.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>> But do I understand correctly that the device isn't powered up at this
>>> point ? That seems problematic.
>>
>> Indeed. It's fixed later in this series.
>>
>>> I'm also not sure why we need to reset the device at probe time.
>>
>> That's the usual place to do a reset, to make sure the HW is in a known
>> state, is it not? Where would you place it?
> 
> The first time the device is used, or possibly every time it is resumed
> ? It seems that you're resuming it at probe time for the only reason
> that you want to then reset it. Resuming it at probe could get entirely
> skipped.

As I mentioned in the earlier reply, I feel better resetting as early as 
possible. Otherwise the DSS may be running, in an unknown state, doing 
things.

It's hard to say if that would cause problems or not. The DSS IP would 
be using clocks that have not been set up or enabled by the driver, 
which might mean that the clock rates get changed underneath, caused by 
some other driver configuring its clocks. That might lead to DSS IP 
misbehaving. Whether that would cause any issues, hard to say.

Still, I'd much rather have the DSS IP in a known state.

We could reset it every time the DSS is resumed, and that would be safe, 
but it also feels pointless, as the DSS is in a known state already 
(presuming it was reset in probe).

  Tomi