diff mbox series

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

Message ID 20231101-tidss-probe-v1-4-45149e0f9415@ideasonboard.com
State New
Headers
Series drm/tidss: Probe related fixes and cleanups |

Commit Message

Tomi Valkeinen Nov. 1, 2023, 9:17 a.m. UTC 
  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>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart Nov. 1, 2023, 1:57 p.m. UTC | #1 
Hi Tomi,

Thank you for the patch.

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.

I'm also not sure why we need to reset the device at probe time.

> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index ad7999434299..9430625e2d62 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2777,10 +2777,6 @@ int dispc_init(struct tidss_device *tidss)
>  			return r;
>  	}
>  
> -	/* K2G display controller does not support soft reset */
> -	if (feat->subrev != DISPC_K2G)
> -		dispc_softreset(dispc);
> -
>  	for (i = 0; i < dispc->feat->num_vps; i++) {
>  		u32 gamma_size = dispc->feat->vp_feat.color.gamma_size;
>  		u32 *gamma_table;
> @@ -2831,5 +2827,9 @@ int dispc_init(struct tidss_device *tidss)
>  
>  	tidss->dispc = dispc;
>  
> +	/* K2G display controller does not support soft reset */
> +	if (feat->subrev != DISPC_K2G)
> +		dispc_softreset(dispc);
> +
>  	return 0;
>  }
>
Tomi Valkeinen Nov. 2, 2023, 6:40 a.m. UTC | #2 
On 01/11/2023 15:57, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> 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?

  Tomi
Laurent Pinchart Nov. 5, 2023, 10:54 p.m. UTC | #3 
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.
Tomi Valkeinen Nov. 6, 2023, 11:56 a.m. UTC | #4 
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
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index ad7999434299..9430625e2d62 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2777,10 +2777,6 @@  int dispc_init(struct tidss_device *tidss)
 			return r;
 	}
 
-	/* K2G display controller does not support soft reset */
-	if (feat->subrev != DISPC_K2G)
-		dispc_softreset(dispc);
-
 	for (i = 0; i < dispc->feat->num_vps; i++) {
 		u32 gamma_size = dispc->feat->vp_feat.color.gamma_size;
 		u32 *gamma_table;
@@ -2831,5 +2827,9 @@  int dispc_init(struct tidss_device *tidss)
 
 	tidss->dispc = dispc;
 
+	/* K2G display controller does not support soft reset */
+	if (feat->subrev != DISPC_K2G)
+		dispc_softreset(dispc);
+
 	return 0;
 }