| Message ID | 20240123171633.246057-7-clamor95@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
| Series | Add T114 video support | expand |
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. Thierry
пт, 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. > Thierry
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
пт, 19 квіт. 2024 р. о 19:46 Thierry Reding <thierry.reding@gmail.com> пише: > > 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(); > You are correct. I assume This patch may be dropped entirely. Thanks. > Thierry
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); + if (tegra_display_probe(priv, (void *)plat->base)) { debug("%s: Failed to probe display driver\n", __func__); return -1;