Patchwork don't print too early on mcp55-based boards

login
register
about
Submitter Ward Vandewege
Date 2010-11-01 16:03:38
Message ID <20101101160338.GA28294@countzero.vandewege.net>
Download mbox | patch
Permalink /patch/2222/
State Accepted
Headers show

Comments

Ward Vandewege - 2010-11-01 16:03:38
See attached. Perhaps we should also print a post code if the SMBus
controller can't be found - suggestions for a value?

Thanks,
Ward.
Peter Stuge - 2010-11-01 22:01:41
Ward Vandewege wrote:
> See attached. Perhaps we should also print a post code if the SMBus
> controller can't be found - suggestions for a value?

0x5B ?


> We can't print this early.
> 
> This patch fixes a hang on 
> 
>   supermicro/h8dme
>   supermicro/h8dmr
>   supermicro/h8dmr_fam10
> 
> and possibly on other mcp55-based boards.
> 
> Signed-off-by: Ward Vandewege <ward@gnu.org>

Acked-by: Peter Stuge <peter@stuge.se>
Uwe Hermann - 2010-11-02 22:20:43
On Mon, Nov 01, 2010 at 11:01:41PM +0100, Peter Stuge wrote:
> Ward Vandewege wrote:
> > See attached. Perhaps we should also print a post code if the SMBus
> > controller can't be found - suggestions for a value?
> 
> 0x5B ?
> 
> 
> > We can't print this early.
> > 
> > This patch fixes a hang on 
> > 
> >   supermicro/h8dme
> >   supermicro/h8dmr
> >   supermicro/h8dmr_fam10
> > 
> > and possibly on other mcp55-based boards.
> > 
> > Signed-off-by: Ward Vandewege <ward@gnu.org>
> 
> Acked-by: Peter Stuge <peter@stuge.se>

I don't object to the patch, and we should probably fix this in all
other southbridges, I think the same problem applies there.

But: the die() call itself also does a printk(), so that'll still hang
if the error path is chosen (at that point it probably doesn't matter
much, though, as we die anyway).

I also agree that die() should have a POST code, preferrably something
easy to remember. It already has a commented-out "//post_code(0xff);".
Not sure why it's disabled, but I think it should be something other
than 0xff, that's a bit too "special" for my taste.

We have "0xee: Not supposed to get here" as per documentation/POSTCODES,
so maybe we can use 0xdd ("d" as in die), if that's not already used elsewhere.


Uwe.
Ward Vandewege - 2010-11-03 16:56:04
On Tue, Nov 02, 2010 at 11:20:43PM +0100, Uwe Hermann wrote:
> I don't object to the patch, and we should probably fix this in all
> other southbridges, I think the same problem applies there.
> 
> But: the die() call itself also does a printk(), so that'll still hang
> if the error path is chosen (at that point it probably doesn't matter
> much, though, as we die anyway).

Right, I think it does not matter. If the die happens when printk is already
functional, great, if not it will hang there which is fine.

> I also agree that die() should have a POST code, preferrably something
> easy to remember. It already has a commented-out "//post_code(0xff);".
> Not sure why it's disabled, but I think it should be something other
> than 0xff, that's a bit too "special" for my taste.
> 
> We have "0xee: Not supposed to get here" as per documentation/POSTCODES,
> so maybe we can use 0xdd ("d" as in die), if that's not already used elsewhere.

So, thinking about this a little more, I'm not sure adding a post code to
'die' is a good idea. The problem with doing that is that it would clobber
any previous post codes, which might be a better indicator for what's going
wrong.

Perhaps a good way to deal with fatal runtime error conditions would be

  a) set a unique post code
  b) call die

in the assumption that die does not clobber the post code.

What do you think?

Thanks,
Ward.
Stefan Reinauer - 2010-11-03 19:48:54
Sent from my mobile phone

On 03.11.2010, at 09:56, Ward Vandewege <ward@gnu.org> wrote:

> On Tue, Nov 02, 2010 at 11:20:43PM +0100, Uwe Hermann wrote:
>> I don't object to the patch, and we should probably fix this in all
>> other southbridges, I think the same problem applies there.
>> 
>> But: the die() call itself also does a printk(), so that'll still hang
>> if the error path is chosen (at that point it probably doesn't matter
>> much, though, as we die anyway).
> 
> Right, I think it does not matter. If the die happens when printk is already
> functional, great, if not it will hang there which is fine.
> 
>> I also agree that die() should have a POST code, preferrably something
>> easy to remember. It already has a commented-out "//post_code(0xff);".
>> Not sure why it's disabled, but I think it should be something other
>> than 0xff, that's a bit too "special" for my taste.
>> 
>> We have "0xee: Not supposed to get here" as per documentation/POSTCODES,
>> so maybe we can use 0xdd ("d" as in die), if that's not already used elsewhere.
> 
> So, thinking about this a little more, I'm not sure adding a post code to
> 'die' is a good idea. The problem with doing that is that it would clobber
> any previous post codes, which might be a better indicator for what's going
> wrong.
> 
> Perhaps a good way to deal with fatal runtime error conditions would be
> 
>  a) set a unique post code
>  b) call die
> 
> in the assumption that die does not clobber the post code.
> 

We could add a post code to the parameters of the die() function.

> What do you think?
> 
> Thanks,
> Ward.
> 
> -- 
> Ward Vandewege <ward@fsf.org>
> Free Software Foundation - Senior Systems Administrator
> 
> -- 
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
>
Peter Stuge - 2010-11-04 07:46:31
Ward Vandewege wrote:
> I'm not sure adding a post code to 'die' is a good idea. The
> problem with doing that is that it would clobber any previous post
> codes, which might be a better indicator for what's going wrong.

I think it is important to have a special POST code for use in die()
to positively identify that that is really where the code has hung.


Stefan Reinauer wrote:
> We could add a post code to the parameters of the die() function.

I think this is a good idea, and I would like to suggest that die()
does the following:

void die(u8 post) {
  while(1) {
    post_code(0xdd);
    mdelay(700);
    post_code(post);
    mdelay(700);
  }
}

There is no printk() which is sad. I think it's better to only use
POST codes if that is the only reliable output method, but I think
it's a lot better if the print functions can just be made to not
hang if called before they can actually output something. That would
need a bit more work though, so for now I'd like to suggest the
above.


//Peter
Ward Vandewege - 2010-11-08 17:42:25
On Mon, Nov 01, 2010 at 11:01:41PM +0100, Peter Stuge wrote:
> Ward Vandewege wrote:
> > See attached. Perhaps we should also print a post code if the SMBus
> > controller can't be found - suggestions for a value?
> 
> 0x5B ?

Let's do that as part of the die modification.

> > We can't print this early.
> > 
> > This patch fixes a hang on 
> > 
> >   supermicro/h8dme
> >   supermicro/h8dmr
> >   supermicro/h8dmr_fam10
> > 
> > and possibly on other mcp55-based boards.
> > 
> > Signed-off-by: Ward Vandewege <ward@gnu.org>
> 
> Acked-by: Peter Stuge <peter@stuge.se>

r6048.

Thanks,
Ward.

Patch


We can't print this early.

This patch fixes a hang on 

  supermicro/h8dme
  supermicro/h8dmr
  supermicro/h8dmr_fam10

and possibly on other mcp55-based boards.

Signed-off-by: Ward Vandewege <ward@gnu.org>

Index: src/southbridge/nvidia/mcp55/mcp55_early_smbus.c
===================================================================
--- src/southbridge/nvidia/mcp55/mcp55_early_smbus.c	(revision 6011)
+++ src/southbridge/nvidia/mcp55/mcp55_early_smbus.c	(working copy)
@@ -32,11 +32,8 @@ 
 	device_t dev;
 	dev = pci_locate_device(PCI_ID(0x10de, 0x0368), 0);
 
-	if (dev == PCI_DEV_INVALID) {
-		printk(BIOS_WARNING, "SMBUS controller not found\n");
-	} else {
-		printk(BIOS_DEBUG, "SMBus controller enabled\n");
-	}
+	if (dev == PCI_DEV_INVALID)
+		die("SMBus controller not found\n");
 
 	/* set smbus iobase */
 	pci_write_config32(dev, 0x20, SMBUS0_IO_BASE | 1);