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 <[email protected]>
---
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;
}
--
2.34.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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
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;
> }
>
--
Regards,
Laurent Pinchart
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 <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
>
> 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
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 <[email protected]>
> >
> > Reviewed-by: Laurent Pinchart <[email protected]>
> >
> > 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.
--
Regards,
Laurent Pinchart
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 <[email protected]>
>>>
>>> Reviewed-by: Laurent Pinchart <[email protected]>
>>>
>>> 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