| Message ID | 20231101-tidss-probe-v1-4-45149e0f9415@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Probe related fixes and cleanups | expand |
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; > } >
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
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.
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 --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; }
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(-)