| 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 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>
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.
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.
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 >
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
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);