| Message ID | 20220822130513.119029-3-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: DSI fixes | expand |
Hi Tomi, Thank you for the patch. On Mon, Aug 22, 2022 at 04:05:10PM +0300, Tomi Valkeinen wrote: > From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > > Improve the DSI shutdown procedure by clearing various bits that were > set while enabling the DSI output. There has been no clear issues caused > by these, but it's safer to ensure that the features are disabled at the > start of the next DSI enable. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > index 7f2be490fcf8..6a10a35f1122 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > @@ -441,9 +441,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi, > > static void rcar_mipi_dsi_shutdown(struct rcar_mipi_dsi *dsi) > { > + /* Disable VCLKEN */ > + rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN); > + > + /* Disable DOT clock */ > + rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN); I think you can write 0 to those two registers, this will also be safer. With this, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I think there's a bug in rcar_mipi_dsi_startup() related to this by the way, the function only uses rcar_mipi_dsi_set() to set bits, so if the DSI format is modified between two starts, bad things will happen. > + > rcar_mipi_dsi_clr(dsi, PHYSETUP, PHYSETUP_RSTZ); > rcar_mipi_dsi_clr(dsi, PHYSETUP, PHYSETUP_SHUTDOWNZ); > > + /* CFGCLK disable */ > + rcar_mipi_dsi_clr(dsi, CFGCLKSET, CFGCLKSET_CKEN); > + > + /* LPCLK disable */ > + rcar_mipi_dsi_clr(dsi, LPCLKSET, LPCLKSET_CKEN); > + > dev_dbg(dsi->dev, "DSI device is shutdown\n"); > } >
On 22/08/2022 16:20, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Aug 22, 2022 at 04:05:10PM +0300, Tomi Valkeinen wrote: >> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> >> Improve the DSI shutdown procedure by clearing various bits that were >> set while enabling the DSI output. There has been no clear issues caused >> by these, but it's safer to ensure that the features are disabled at the >> start of the next DSI enable. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c >> index 7f2be490fcf8..6a10a35f1122 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c >> @@ -441,9 +441,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi, >> >> static void rcar_mipi_dsi_shutdown(struct rcar_mipi_dsi *dsi) >> { >> + /* Disable VCLKEN */ >> + rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN); >> + >> + /* Disable DOT clock */ >> + rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN); > > I think you can write 0 to those two registers, this will also be safer. > With this, VCLKEN has only the single VCLKEN_CKEN bit and the rest of the bits are reserved with default value of 0, however VCLKSET has other fields and the default value of those fields is not 0. Why do you think it's safer to set the whole register to 0? Isn't it better to just do what we want to do, which makes the purpose clear and, I think, is safer as we don't touch bits we don't know about? > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I think there's a bug in rcar_mipi_dsi_startup() related to this by the > way, the function only uses rcar_mipi_dsi_set() to set bits, so if the > DSI format is modified between two starts, bad things will happen. Oh, that's bad. rcar_mipi_dsi_set() is not a very good function as it's easy to misuse it like that. I'll make a fix for that. Tomi
On Mon, Aug 22, 2022 at 04:49:02PM +0300, Tomi Valkeinen wrote: > On 22/08/2022 16:20, Laurent Pinchart wrote: > > Hi Tomi, > > > > Thank you for the patch. > > > > On Mon, Aug 22, 2022 at 04:05:10PM +0300, Tomi Valkeinen wrote: > >> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >> > >> Improve the DSI shutdown procedure by clearing various bits that were > >> set while enabling the DSI output. There has been no clear issues caused > >> by these, but it's safer to ensure that the features are disabled at the > >> start of the next DSI enable. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >> --- > >> drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 12 ++++++++++++ > >> 1 file changed, 12 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >> index 7f2be490fcf8..6a10a35f1122 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >> @@ -441,9 +441,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi, > >> > >> static void rcar_mipi_dsi_shutdown(struct rcar_mipi_dsi *dsi) > >> { > >> + /* Disable VCLKEN */ > >> + rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN); > >> + > >> + /* Disable DOT clock */ > >> + rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN); > > > > I think you can write 0 to those two registers, this will also be safer. > > With this, > > VCLKEN has only the single VCLKEN_CKEN bit and the rest of the bits are > reserved with default value of 0, however VCLKSET has other fields and > the default value of those fields is not 0. But the two fields whose default value isn't 0 are set in the startup() function (albeit incorrectly as discussed below), so it should be fine. > Why do you think it's safer to set the whole register to 0? Isn't it > better to just do what we want to do, which makes the purpose clear and, > I think, is safer as we don't touch bits we don't know about? Because it will ensure that we don't get surprises when we later restart the device, such as mentioned below :-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > I think there's a bug in rcar_mipi_dsi_startup() related to this by the > > way, the function only uses rcar_mipi_dsi_set() to set bits, so if the > > DSI format is modified between two starts, bad things will happen. > > Oh, that's bad. rcar_mipi_dsi_set() is not a very good function as it's > easy to misuse it like that. I'll make a fix for that.
On 22/08/2022 17:05, Laurent Pinchart wrote: > On Mon, Aug 22, 2022 at 04:49:02PM +0300, Tomi Valkeinen wrote: >> On 22/08/2022 16:20, Laurent Pinchart wrote: >>> Hi Tomi, >>> >>> Thank you for the patch. >>> >>> On Mon, Aug 22, 2022 at 04:05:10PM +0300, Tomi Valkeinen wrote: >>>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >>>> >>>> Improve the DSI shutdown procedure by clearing various bits that were >>>> set while enabling the DSI output. There has been no clear issues caused >>>> by these, but it's safer to ensure that the features are disabled at the >>>> start of the next DSI enable. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >>>> --- >>>> drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c >>>> index 7f2be490fcf8..6a10a35f1122 100644 >>>> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c >>>> @@ -441,9 +441,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi, >>>> >>>> static void rcar_mipi_dsi_shutdown(struct rcar_mipi_dsi *dsi) >>>> { >>>> + /* Disable VCLKEN */ >>>> + rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN); >>>> + >>>> + /* Disable DOT clock */ >>>> + rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN); >>> >>> I think you can write 0 to those two registers, this will also be safer. >>> With this, >> >> VCLKEN has only the single VCLKEN_CKEN bit and the rest of the bits are >> reserved with default value of 0, however VCLKSET has other fields and >> the default value of those fields is not 0. > > But the two fields whose default value isn't 0 are set in the startup() > function (albeit incorrectly as discussed below), so it should be fine. That is true. But I'd rather write 0 to VCLKEN in the startup, before writing the configuration. >> Why do you think it's safer to set the whole register to 0? Isn't it >> better to just do what we want to do, which makes the purpose clear and, >> I think, is safer as we don't touch bits we don't know about? > > Because it will ensure that we don't get surprises when we later restart > the device, such as mentioned below :-) Well, but that's a bug in the startup code. I don't think the shutdown code should do things to make startup work better if the startup does something wrong. Nevertheless, while I slightly disagree, I'm fine with writing zero there in shutdown. Tomi
On Mon, Aug 22, 2022 at 05:19:56PM +0300, Tomi Valkeinen wrote: > On 22/08/2022 17:05, Laurent Pinchart wrote: > > On Mon, Aug 22, 2022 at 04:49:02PM +0300, Tomi Valkeinen wrote: > >> On 22/08/2022 16:20, Laurent Pinchart wrote: > >>> Hi Tomi, > >>> > >>> Thank you for the patch. > >>> > >>> On Mon, Aug 22, 2022 at 04:05:10PM +0300, Tomi Valkeinen wrote: > >>>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >>>> > >>>> Improve the DSI shutdown procedure by clearing various bits that were > >>>> set while enabling the DSI output. There has been no clear issues caused > >>>> by these, but it's safer to ensure that the features are disabled at the > >>>> start of the next DSI enable. > >>>> > >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >>>> --- > >>>> drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 12 ++++++++++++ > >>>> 1 file changed, 12 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >>>> index 7f2be490fcf8..6a10a35f1122 100644 > >>>> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >>>> @@ -441,9 +441,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi, > >>>> > >>>> static void rcar_mipi_dsi_shutdown(struct rcar_mipi_dsi *dsi) > >>>> { > >>>> + /* Disable VCLKEN */ > >>>> + rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN); > >>>> + > >>>> + /* Disable DOT clock */ > >>>> + rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN); > >>> > >>> I think you can write 0 to those two registers, this will also be safer. > >>> With this, > >> > >> VCLKEN has only the single VCLKEN_CKEN bit and the rest of the bits are > >> reserved with default value of 0, however VCLKSET has other fields and > >> the default value of those fields is not 0. > > > > But the two fields whose default value isn't 0 are set in the startup() > > function (albeit incorrectly as discussed below), so it should be fine. > > That is true. But I'd rather write 0 to VCLKEN in the startup, before > writing the configuration. You can do both :-) > >> Why do you think it's safer to set the whole register to 0? Isn't it > >> better to just do what we want to do, which makes the purpose clear and, > >> I think, is safer as we don't touch bits we don't know about? > > > > Because it will ensure that we don't get surprises when we later restart > > the device, such as mentioned below :-) > > Well, but that's a bug in the startup code. I don't think the shutdown > code should do things to make startup work better if the startup does > something wrong. Nevertheless, while I slightly disagree, I'm fine with > writing zero there in shutdown. I agree it needs to be fixed at start() time, but I think it's also good practice to put the device in a fully known state after shutdown, at least when it's easy to do so. It would also save an unnecessary read access to the register.
diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c index 7f2be490fcf8..6a10a35f1122 100644 --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c @@ -441,9 +441,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi, static void rcar_mipi_dsi_shutdown(struct rcar_mipi_dsi *dsi) { + /* Disable VCLKEN */ + rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN); + + /* Disable DOT clock */ + rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN); + rcar_mipi_dsi_clr(dsi, PHYSETUP, PHYSETUP_RSTZ); rcar_mipi_dsi_clr(dsi, PHYSETUP, PHYSETUP_SHUTDOWNZ); + /* CFGCLK disable */ + rcar_mipi_dsi_clr(dsi, CFGCLKSET, CFGCLKSET_CKEN); + + /* LPCLK disable */ + rcar_mipi_dsi_clr(dsi, LPCLKSET, LPCLKSET_CKEN); + dev_dbg(dsi->dev, "DSI device is shutdown\n"); }