Re: [PATCH v6 06/18] video: tegra20: dc: add reset support

["Thierry Reding" <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3548 bytes --]

On Fri Apr 19, 2024 at 6:37 PM CEST, Svyatoslav Ryhel wrote:
> пт, 19 квіт. 2024 р. о 19:05 Thierry Reding <thierry.reding@gmail.com> пише:
> >
> > On Tue Jan 23, 2024 at 6:16 PM CET, Svyatoslav Ryhel wrote:
> > > Implement reset use to discard any changes which could have been
> > > applied to DC before and can interfere with current configuration.
> > >
> > > Tested-by: Agneli <poczt@protonmail.ch> # Toshiba AC100 T20
> > > Tested-by: Robert Eckelmann <longnoserob@gmail.com> # ASUS TF101
> > > Tested-by: Andreas Westman Dorcsak <hedmoo@yahoo.com> # ASUS Grouper E1565
> > > Tested-by: Ion Agorria <ion@agorria.com> # HTC One X
> > > Tested-by: Svyatoslav Ryhel <clamor95@gmail.com> # Nvidia Tegratab T114
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  drivers/video/tegra20/tegra-dc.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/video/tegra20/tegra-dc.c b/drivers/video/tegra20/tegra-dc.c
> > > index 56a23b3c97..35abb6fe46 100644
> > > --- a/drivers/video/tegra20/tegra-dc.c
> > > +++ b/drivers/video/tegra20/tegra-dc.c
> > > @@ -10,7 +10,9 @@
> > >  #include <panel.h>
> > >  #include <part.h>
> > >  #include <pwm.h>
> > > +#include <reset.h>
> > >  #include <video.h>
> > > +#include <linux/delay.h>
> > >  #include <asm/cache.h>
> > >  #include <asm/global_data.h>
> > >  #include <asm/system.h>
> > > @@ -342,6 +344,7 @@ static int tegra_lcd_probe(struct udevice *dev)
> > >       struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> > >       struct video_priv *uc_priv = dev_get_uclass_priv(dev);
> > >       struct tegra_lcd_priv *priv = dev_get_priv(dev);
> > > +     struct reset_ctl reset_ctl;
> > >       int ret;
> > >
> > >       /* Initialize the Tegra display controller */
> > > @@ -349,6 +352,20 @@ static int tegra_lcd_probe(struct udevice *dev)
> > >       funcmux_select(PERIPH_ID_DISP1, FUNCMUX_DEFAULT);
> > >  #endif
> > >
> > > +     ret = reset_get_by_name(dev, "dc", &reset_ctl);
> > > +     if (ret) {
> > > +             log_err("reset_get_by_name() failed: %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     clock_disable(priv->dc_clk[0]);
> > > +
> > > +     /* Reset everything set before */
> > > +     reset_assert(&reset_ctl);
> > > +     mdelay(4);
> > > +     reset_deassert(&reset_ctl);
> > > +     mdelay(4);
> >
> > Are you sure this works as intended? It's been a long time since I
> > worked on this, but I seem to recall that most of these resets are
> > actually synchronous, so in order for them to do what they're supposed
> > to the clock needs to be kept running.
> >
> > The Linux driver certainly does this differently.
>
> You have point, but I have tried to mostly adapt Linux tegra dc driver,
> which has same logic in probe. Maybe I have not understood it properly.
> Testing on T20, T30 and T114 passed without issues so far.

Maybe look again. What it does is (basically):

	clock_enable();
	mdelay(4);
	reset_assert();
	mdelay(4);
	clock_disable();

That should ensure that it's completely reset at that point. Now before
any subsequent register accesses happen it will do this:

	clock_enable();
	reset_deassert();

to take it out of reset again. Perhaps that's something you want to keep
doing in probe() in U-Boot. In that case maybe you want something like
this instead:

	clock_enable();
	mdelay(4);
	reset_assert();
	mdelay(4);
	reset_deassert();

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]