Re: [PATCH v4 3/5] powerpc/mpc5121: shared DIU framebuffer support

On Thu, 15 Dec 2011 17:27:53 +0000
Tabi Timur-B04825 [off-list ref] wrote:

On Fri, Jul 23, 2010 at 9:00 AM, Anatolij Gustschin [off-list ref] wrote:

quoted

@@ -1471,7 +1476,9 @@ static int __devinit fsl_diu_probe(struct of_device *ofdev,

               goto error2;
       }

-       out_be32(&dr.diu_reg->diu_mode, 0);             /* disable DIU anyway*/
+       diu_mode = in_be32(&dr.diu_reg->diu_mode);
+       if (diu_mode != MFB_MODE1)
+               out_be32(&dr.diu_reg->diu_mode, 0);     /* disable DIU */

Anatolij,

I know this patch is old, but I just noticed something odd about it
that I need your help with.

In the above snippet, you test for != MFB_MODE1.  My understanding is
that U-boot only supports modes 0 and 1, never modes 2 or 3.  So
diu_mode can only ever be 0 or 1.  That means that that the above code
is equivalent to:

diu_mode = in_be32(&dr.diu_reg->diu_mode);
if (diu_mode = 0)
        out_be32(&dr.diu_reg->diu_mode, 0);     /* disable DIU */

which is silly, because now we're writing 0 to diu_mode only if it's already 0.

Am I missing something?

the intention of the above code snippet was:
not to disable DIU if it is in mode 1 (displaying splash screen)
and to disable DIU if it is in modes 2, 3 or 4 for some reason.

We cannot guarantee that the DIU is not in these modes. Even
if U-Boot didn't set these modes there is still a possibility
that such mode is configured. E.g. I've seen U-Boot binary
standalone applications for other display controllers initializing
the display controller.

But you are right. With this snippet, if the DIU is already
disabled, there will be not needed mode register access. So
the code should better look like:

diu_mode = in_be32(&dr.diu_reg->diu_mode);
if (diu_mode && diu_mode != MFB_MODE1)
	out_be32(&dr.diu_reg->diu_mode, 0);

Anatolij