[LinuxBIOS] [Patch] AMD cs5536 uart disable

[Marc Jones]

The UART disable code was causing a hang and was worked around with a 
return that skipped the disable code. This patch removes the return and 
fixes the UART disable code.

The problem was that the disable code was ORing bits into the Legacy_IO 
MSR causing issues with the LPC SIOs init code that would manifest as a 
hang because the IO would not be decoded correctly. ANDing to clear the 
bits fixes the issue.

Signed-off-by: Marc Jones

[ron minnich]

Should I test this first or were you able to test?

thanks

ron

[Marc Jones]

ron minnich wrote:
> Should I test this first or were you able to test?
>
> thanks
>
> ron

Ron,
I tested it and it works for me. I don't mind if you test it too. :)
Marc

[ron minnich]

Acked-by: Ronald G. Minnich

On 5/30/07, Marc Jones  wrote:
> The UART disable code was causing a hang and was worked around with a
> return that skipped the disable code. This patch removes the return and
> fixes the UART disable code.
>
> The problem was that the disable code was ORing bits into the Legacy_IO
> MSR causing issues with the LPC SIOs init code that would manifest as a
> hang because the IO would not be decoded correctly. ANDing to clear the
> bits fixes the issue.
>
> Signed-off-by: Marc Jones
> Index: cs5536.c
>
> --- cs5536.c    (revision 2705)
> +++ cs5536.c    (working copy)
> @@ -316,9 +316,6 @@
>     } else {
>             /* Reset and disable COM1 */
>           printk_err("Not disabling COM1 due to a bug ...\n");
>           /* for now, don't do this! */
>           return;
>           msr = rdmsr(MDD_UART1_CONF);
>           msr.lo = 1;     // reset
>           wrmsr(MDD_UART1_CONF, msr);
>
> @@ -327,7 +324,7 @@
>             /* Disable the IRQ */
>             msr = rdmsr(MDD_LEG_IO);
>           msr.lo |= ~(0xF << 16);
>           msr.lo &= ~(0xF << 16);
>           wrmsr(MDD_LEG_IO, msr);
>   }
>
> @@ -391,7 +388,7 @@
>             /* Disable the IRQ */
>             msr = rdmsr(MDD_LEG_IO);
>           msr.lo |= ~(0xF << 20);
>           msr.lo &= ~(0xF << 20);
>           wrmsr(MDD_LEG_IO, msr);
>   }
>
> }

[Uwe Hermann]

On Thu, May 31, 2007 at 07:14:00AM -0700, ron minnich wrote:
> Acked-by: Ronald G. Minnich

Yep, looks good. Committed in r2706. Thanks for the fix Marc!

Two minor issues, though:

- Please run 'svn diff' at the top-level directory (easier to see
   which files shall be patched).

- Please put the explanations and sign-off into the patch file itself
   (not in the email body). That makes it easier to handle things when
   you have 20 patches in your $HOME and you don't want to switch
   back to the mail-client all the time to check the sign-off/explanations...

Thanks, Uwe.

[LinuxBIOS] AMD cs5536 uart disable

[Jordan Crouse]

On 03/06/07 01:59 +0200, Uwe Hermann wrote:
> Please run 'svn diff' at the top-level directory (easier to see
> which files shall be patched).

Not sure what you mean by that - are you looking for the diffstat?

Jordan

---
Jordan Crouse
Senior Linux Engineer
Advanced Micro Devices, Inc.
<www.amd.com/embeddedprocessors>

[LinuxBIOS] AMD cs5536 uart disable

[Uwe Hermann]

On Sun, Jun 03, 2007 at 09:44:00PM -0600, Jordan Crouse wrote:
> On 03/06/07 01:59 +0200, Uwe Hermann wrote:
> > Please run 'svn diff' at the top-level directory (easier to see
> > which files shall be patched).
>
> Not sure what you mean by that - are you looking for the diffstat?

Ah, no, sorry for being unclear.

Running 'svn diff > foo.patch' in a subdirectory, e.g. in
src/southbridge/amd/cs5536, will produce a patch with entries in the
following format:

Index: cs5536.c
===================================================================
--- cs5536.c    (revision 2705)
+++ cs5536.c    (working copy)

Running 'svn diff > foo.patch' at the top-level directory of the svn
working copy will produce entries such as the following:

Index: src/southbridge/amd/cs5536/cs5536.c
===================================================================
--- src/southbridge/amd/cs5536/cs5536.c (revision 2708)
+++ src/southbridge/amd/cs5536/cs5536.c (working copy)

I.e., the "full" relative path to the file being modified is included
in the patch, so you can 'patch -p0 < ~/foo.patch' at the top-level to
apply the patch.

The first version is harder to deal with, as you have to know in which
directory the file 'cs5536.c' resides, cd into that directory and run
'patch -p0 < ~/foo.patch' there.

Not too hard for cs5536.c, but imagine a patch which changes a file
which can be located just about anywhere (e.g. Config.lb). We'd have to
guess or ask the patch submitter to which file the patch must be applied.

Hence -> 'svn diff' at the top-level directory is easier to handle for us.

Thanks, Uwe.

[LinuxBIOS] AMD cs5536 uart disable

[Jordan Crouse]

On 04/06/07 08:48 +0200, Uwe Hermann wrote:
> I.e., the "full" relative path to the file being modified is included
> in the patch, so you can 'patch -p0 < ~/foo.patch' at the top-level to
> apply the patch.

-p0?  Is that seriously the standard here?  Can I ask why?
> Hence -> 'svn diff' at the top-level directory is easier to handle for us.

I thought Marc used quilt - which should do the right thing in this regard.
I'll follow up with him.

Jordan

[LinuxBIOS] AMD cs5536 uart disable

[Stefan Reinauer]

* Jordan Crouse  [070604 16:44]:
> On 04/06/07 08:48 +0200, Uwe Hermann wrote:
> > I.e., the "full" relative path to the file being modified is included
> > in the patch, so you can 'patch -p0 < ~/foo.patch' at the top-level to
> > apply the patch.
>
> -p0?  Is that seriously the standard here?  Can I ask why?

Purely convenience of the patch reviewers / people doing the check-ins.
Then you don't have to check manually which directory the patch goes
into. Don't worry about it. We will figure this out either way. It just
might make our life easier (thus speeding up check-ins)

Stefan

[LinuxBIOS] AMD cs5536 uart disable

[Segher Boessenkool]

> > > I.e., the "full" relative path to the file being modified is included
> > > in the patch, so you can 'patch -p0 < ~/foo.patch' at the top-level 
> > > to
> > > apply the patch.
> >
> > -p0?  Is that seriously the standard here?  Can I ask why?
>
> Purely convenience of the patch reviewers / people doing the check-ins.
> Then you don't have to check manually which directory the patch goes
> into. Don't worry about it. We will figure this out either way. It just
> might make our life easier (thus speeding up check-ins)

I'm sure you mean you want -p1 patches, instead?

Segher

[LinuxBIOS] AMD cs5536 uart disable

[Peter Stuge]

On Mon, Jun 04, 2007 at 05:27:47PM +0200, Segher Boessenkool wrote:
> > > -p0?  Is that seriously the standard here?  Can I ask why?
> >
> > Purely convenience of the patch reviewers / people doing the check-ins.
>
> I'm sure you mean you want -p1 patches, instead?

Are -p0 and -p1 equivalent if the patch has a relative filename as
opposed to an absolute? This is only academia anyway.

Please send patches generated from the root of the svn tree; the
LinuxBIOSv2 or v3 subdirectory.

//Peter

[LinuxBIOS] AMD cs5536 uart disable

[Segher Boessenkool]

> > > > -p0?  Is that seriously the standard here?  Can I ask why?
> > >
> > > Purely convenience of the patch reviewers / people doing the 
> > > check-ins.
> >
> > I'm sure you mean you want -p1 patches, instead?
>
> Are -p0 and -p1 equivalent if the patch has a relative filename as
> opposed to an absolute? This is only academia anyway.

No, they are never equivalent; -p1 strips everything up
to and including the first slash from every pathname,
-p0 doesn't.  You *always* need to use one of -p0 and
-p1 fwiw.
> Please send patches generated from the root of the svn tree; the
> LinuxBIOSv2 or v3 subdirectory.

Sounds advice.  For some reason "svn diff" seems to
generate -p0 style patches; every sane tool in the world
uses -p1.  Oh who cares, either style is trivial to
deal with.

Segher

[LinuxBIOS] AMD cs5536 uart disable

[Uwe Hermann]

On Mon, Jun 04, 2007 at 09:23:51PM +0200, Segher Boessenkool wrote:
> Sounds advice.  For some reason "svn diff" seems to
> generate -p0 style patches; every sane tool in the world
> uses -p1.

Why? What's so special about -p1? Why should -p0 be worse?
Just curios...

Uwe.

[LinuxBIOS] AMD cs5536 uart disable

[Uwe Hermann]

On Mon, Jun 04, 2007 at 08:44:36AM -0600, Jordan Crouse wrote:
> On 04/06/07 08:48 +0200, Uwe Hermann wrote:
> > I.e., the "full" relative path to the file being modified is included
> > in the patch, so you can 'patch -p0 < ~/foo.patch' at the top-level to
> > apply the patch.
>
> -p0?  Is that seriously the standard here?  Can I ask why?

No standard (or rather: not really), it's just what you get when you do
'svn diff > foo.patch' at the top-level.

Either -p0 or -p1 is fine, I don't care.

Uwe.