[U-Boot] [PATCH] Revert "imx: mx7: implement reset_misc"

[U-Boot] [PATCH] Revert "imx: mx7: implement reset_misc"

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

Commit 9f8fa184fc1ac ("imx: mx7: implement reset_misc") says it needs to
to power down lcdif to make 'reset' can pass stress test.

However I see 100% of reset failures with this commit applied, so better
revert it for now.

This reverts commit 9f8fa184fc1acb6fe8e15e3bbbfcb916e6bc4cc1.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/cpu/armv7/mx7/soc.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm/cpu/armv7/mx7/soc.c b/arch/arm/cpu/armv7/mx7/soc.c
index dead1d3..b1d0e21 100644
--- a/arch/arm/cpu/armv7/mx7/soc.c
+++ b/arch/arm/cpu/armv7/mx7/soc.c
@@ -455,11 +455,3 @@ void s_init(void)
 
 	return;
 }
-
-void reset_misc(void)
-{
-#ifdef CONFIG_VIDEO_MXS
-	lcdif_power_down();
-#endif
-}
-
-- 
2.7.4

[U-Boot] [PATCH] Revert "imx: mx7: implement reset_misc"

[Fabio Estevam]

Hi Stefano and Peng,

On Tue, Feb 21, 2017 at 11:21 PM, Fabio Estevam <festevam@gmail.com> wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
>
> Commit 9f8fa184fc1ac ("imx: mx7: implement reset_misc") says it needs to
> to power down lcdif to make 'reset' can pass stress test.
>
> However I see 100% of reset failures with this commit applied, so better
> revert it for now.
>
> This reverts commit 9f8fa184fc1acb6fe8e15e3bbbfcb916e6bc4cc1.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

I realized that the reset failure always happen only when 'videomode'
variable is not present:

=> setenv videomode
=> saveenv
=> reset

Then reset hangs.

If I do the check like this, then it does not hang:

--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -135,6 +135,11 @@ void lcdif_power_down(void)
 {
        struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE;
        int timeout = 1000000;
+       char *penv;
+
+       penv = getenv("videomode");
+       if (!penv)
+               return;

        writel(panel.frameAdrs, &regs->hw_lcdif_cur_buf_reg);
        writel(panel.frameAdrs, &regs->hw_lcdif_next_buf_reg);

What do you think of such proposal instead of doing the revert?

[U-Boot] [PATCH] Revert "imx: mx7: implement reset_misc"

[Peng Fan]

Hi Fabio,

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: Wednesday, February 22, 2017 5:34 PM
> To: Stefano Babic <sbabic@denx.de>
> Cc: Tom Rini <trini@konsulko.com>; U-Boot-Denx <u-boot@lists.denx.de>;
> Peng Fan <peng.fan@nxp.com>; Fabio Estevam <fabio.estevam@nxp.com>
> Subject: Re: [PATCH] Revert "imx: mx7: implement reset_misc"
> 
> Hi Stefano and Peng,
> 
> On Tue, Feb 21, 2017 at 11:21 PM, Fabio Estevam <festevam@gmail.com>
> wrote:
> > From: Fabio Estevam <fabio.estevam@nxp.com>
> >
> > Commit 9f8fa184fc1ac ("imx: mx7: implement reset_misc") says it needs
> > to to power down lcdif to make 'reset' can pass stress test.
> >
> > However I see 100% of reset failures with this commit applied, so
> > better revert it for now.
> >
> > This reverts commit 9f8fa184fc1acb6fe8e15e3bbbfcb916e6bc4cc1.
> >
> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> 
> I realized that the reset failure always happen only when 'videomode'
> variable is not present:
> 
> => setenv videomode
> => saveenv
> => reset
> 
> Then reset hangs.
> 
> If I do the check like this, then it does not hang:
> 
> --- a/drivers/video/mxsfb.c
> +++ b/drivers/video/mxsfb.c
> @@ -135,6 +135,11 @@ void lcdif_power_down(void)  {
>         struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE;
>         int timeout = 1000000;
> +       char *penv;
> +
> +       penv = getenv("videomode");
> +       if (!penv)
> +               return;
> 
>         writel(panel.frameAdrs, &regs->hw_lcdif_cur_buf_reg);
>         writel(panel.frameAdrs, &regs->hw_lcdif_next_buf_reg);
> 
> What do you think of such proposal instead of doing the revert?

If no videomode set, that means lcd will not be initialized.  So there is no need to power down the lcdif, agree with your new approach.
Because when lcdif is initialized, there is also a risk that system hang if lcdif is not powered down, so no revert.

Regards,
Peng.

[U-Boot] [PATCH] Revert "imx: mx7: implement reset_misc"

[Stefano Babic]

On 22/02/2017 10:33, Fabio Estevam wrote:
> Hi Stefano and Peng,
> 
> On Tue, Feb 21, 2017 at 11:21 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> From: Fabio Estevam <fabio.estevam@nxp.com>
>>
>> Commit 9f8fa184fc1ac ("imx: mx7: implement reset_misc") says it needs to
>> to power down lcdif to make 'reset' can pass stress test.
>>
>> However I see 100% of reset failures with this commit applied, so better
>> revert it for now.
>>
>> This reverts commit 9f8fa184fc1acb6fe8e15e3bbbfcb916e6bc4cc1.
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> 
> I realized that the reset failure always happen only when 'videomode'
> variable is not present:
> 

Ouch ! Wrong access ?

> => setenv videomode
> => saveenv
> => reset
> 
> Then reset hangs.
> 
> If I do the check like this, then it does not hang:
> 
> --- a/drivers/video/mxsfb.c
> +++ b/drivers/video/mxsfb.c
> @@ -135,6 +135,11 @@ void lcdif_power_down(void)
>  {
>         struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE;
>         int timeout = 1000000;
> +       char *penv;
> +
> +       penv = getenv("videomode");
> +       if (!penv)
> +               return;
> 
>         writel(panel.frameAdrs, &regs->hw_lcdif_cur_buf_reg);
>         writel(panel.frameAdrs, &regs->hw_lcdif_next_buf_reg);
> 
> What do you think of such proposal instead of doing the revert?
> 

Fully agree !

Best regards,
Stefano


-- 
Meet DENX at the Embedded World Trade Show
14 Mar - 16 Mar 2017, Nuremberg Trade Fair Centre, Hall 4, Booth 581
--
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] Revert "imx: mx7: implement reset_misc"

[Peng Fan]

On Wed, Feb 22, 2017 at 06:33:32AM -0300, Fabio Estevam wrote:
>Hi Stefano and Peng,
>
>On Tue, Feb 21, 2017 at 11:21 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> From: Fabio Estevam <fabio.estevam@nxp.com>
>>
>> Commit 9f8fa184fc1ac ("imx: mx7: implement reset_misc") says it needs to
>> to power down lcdif to make 'reset' can pass stress test.
>>
>> However I see 100% of reset failures with this commit applied, so better
>> revert it for now.
>>
>> This reverts commit 9f8fa184fc1acb6fe8e15e3bbbfcb916e6bc4cc1.
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
>
>I realized that the reset failure always happen only when 'videomode'
>variable is not present:
>
>=> setenv videomode
>=> saveenv
>=> reset
>
>Then reset hangs.
>
>If I do the check like this, then it does not hang:
>
>--- a/drivers/video/mxsfb.c
>+++ b/drivers/video/mxsfb.c
>@@ -135,6 +135,11 @@ void lcdif_power_down(void)
> {
>        struct mxs_lcdif_regs *regs = (struct mxs_lcdif_regs *)MXS_LCDIF_BASE;
>        int timeout = 1000000;
>+       char *penv;
>+
>+       penv = getenv("videomode");
>+       if (!penv)
>+               return;
>
>        writel(panel.frameAdrs, &regs->hw_lcdif_cur_buf_reg);
>        writel(panel.frameAdrs, &regs->hw_lcdif_next_buf_reg);
>
>What do you think of such proposal instead of doing the revert?

Sorry for repost, my outlook seems work abnormally.

No revert. When videomode is set, lcdif will be initialized, then
when reset, need to power down lcdif to avoid reset failure.

If no videomode, lcdif will not be initialized, no need to powerdown.

Agree with your new approach.

Regards,
Peng.

>_______________________________________________
>U-Boot mailing list
>U-Boot at lists.denx.de
>http://lists.denx.de/mailman/listinfo/u-boot