* Alex G. <mr.nuke...@gmail.com> [110225 18:48]: > Find attached the version with the colloquial verbiage abridged. > > Alex
> Remove all occurences of outb(*, 0x80), and replace them with > post_code(). > Create post_codes.h to store a central place for post codes. > Replace common post_codes with macros defined in post_codes.h. > > Signed-off-by: Alexandru Gagniuc <mr.nuke...@gmail.com> Hi Alexandru, thanks a lot, nice patch! I have a few comments below though. > > Index: src/southbridge/via/vt8231/early_smbus.c > =================================================================== > --- src/southbridge/via/vt8231/early_smbus.c (revision 6380) > +++ src/southbridge/via/vt8231/early_smbus.c (working copy) > @@ -1,3 +1,5 @@ > +#include <console/console.h> > + > #define SMBUS_IO_BASE 0x5000 > > #define SMBHSTSTAT 0x0 > @@ -54,7 +56,7 @@ > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } This should not be an outb to 0x80 at all, as it does not reflect a "post_code" debug message but rather a delay. I suggest that you either use an inb(0x80) or an outb to port 0xeb or something instead. > Index: src/southbridge/via/vt8235/early_smbus.c > =================================================================== > --- src/southbridge/via/vt8235/early_smbus.c (revision 6380) > +++ src/southbridge/via/vt8235/early_smbus.c (working copy) > @@ -1,4 +1,5 @@ > #define SMBUS_IO_BASE 0xf00 > +#include <console/console.h> > > #define SMBHSTSTAT 0x0 > #define SMBSLVSTAT 0x1 > @@ -52,7 +53,7 @@ > /* let clocks and the like settle */ > /* as yet arbitrary count - 1000 is too little 5000 works */ > for(i = 0 ; i < 5000 ; i++) > - outb(0x80,0x80); > + post_code(POST_SMBUS_DELAY); same here. > /* > * The VT1211 serial port needs 48 mhz clock, on power up it is getting > @@ -75,7 +76,7 @@ > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } and here. > + post_code( P80_CHIPSET_INIT ); No spaces required left and right. > Index: src/southbridge/amd/sb600/smbus.c > =================================================================== > --- src/southbridge/amd/sb600/smbus.c (revision 6380) > +++ src/southbridge/amd/sb600/smbus.c (working copy) > @@ -18,10 +18,11 @@ > */ > > #include "smbus.h" > +#include <console/console.h> > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } No use of post_code here (inb or 0xeb instead) > Index: src/southbridge/amd/sb700/smbus.c > =================================================================== > --- src/southbridge/amd/sb700/smbus.c (revision 6380) > +++ src/southbridge/amd/sb700/smbus.c (working copy) > @@ -21,10 +21,11 @@ > #define _SB700_SMBUS_C_ > > #include "smbus.h" > +#include <console/console.h> > > static inline void smbus_delay(void) > { > - outb(inb(0x80), 0x80); > + post_code(POST_SMBUS_DELAY); > } > > static int smbus_wait_until_ready(u32 smbus_io_base) Same here. > Index: src/southbridge/amd/sb800/smbus.c > =================================================================== > --- src/southbridge/amd/sb800/smbus.c (revision 6380) > +++ src/southbridge/amd/sb800/smbus.c (working copy) > @@ -21,10 +21,11 @@ > #define _SB800_SMBUS_C_ > > #include "smbus.h" > +#include <console/console.h> > > static inline void smbus_delay(void) > { > - outb(inb(0x80), 0x80); > + post_code(POST_SMBUS_DELAY); > } Same here. > Index: src/southbridge/amd/cimx_wrapper/sb800/smbus.c > =================================================================== > --- src/southbridge/amd/cimx_wrapper/sb800/smbus.c (revision 6380) > +++ src/southbridge/amd/cimx_wrapper/sb800/smbus.c (working copy) > @@ -20,10 +20,11 @@ > > #include <arch/io.h> > #include "smbus.h" > +#include <console/console.h> > > static inline void smbus_delay(void) > { > - outb(inb(0x80), 0x80); > + post_code(POST_SMBUS_DELAY); > } Same here. > Index: src/southbridge/amd/amd8111/amd8111_smbus.h > =================================================================== > --- src/southbridge/amd/amd8111/amd8111_smbus.h (revision 6380) > +++ src/southbridge/amd/amd8111/amd8111_smbus.h (working copy) > @@ -12,7 +13,7 @@ > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } Same here. > --- src/southbridge/broadcom/bcm5785/smbus.h (revision 6380) > +++ src/southbridge/broadcom/bcm5785/smbus.h (working copy) > @@ -42,7 +43,7 @@ > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } Same here. > static int smbus_wait_until_ready(unsigned smbus_io_base) > Index: src/southbridge/nvidia/ck804/smbus.h > =================================================================== > --- src/southbridge/nvidia/ck804/smbus.h (revision 6380) > +++ src/southbridge/nvidia/ck804/smbus.h (working copy) > @@ -19,6 +19,7 @@ > */ > > #include <device/smbus_def.h> > +#include <console/console.h> > > #define SMBHSTSTAT 0x1 > #define SMBHSTPRTCL 0x0 > @@ -35,7 +36,7 @@ > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY);; > } Same here. > Index: src/southbridge/nvidia/mcp55/smbus.h > =================================================================== > --- src/southbridge/nvidia/mcp55/smbus.h (revision 6380) > +++ src/southbridge/nvidia/mcp55/smbus.h (working copy) > @@ -34,10 +34,11 @@ > * Longer than this is just painful when a timeout condition occurs. > */ > #define SMBUS_TIMEOUT (100*1000*10) > +#include <console/console.h> > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } Same here. > Index: src/southbridge/intel/i82371eb/smbus.h > =================================================================== > --- src/southbridge/intel/i82371eb/smbus.h (revision 6380) > +++ src/southbridge/intel/i82371eb/smbus.h (working copy) > @@ -1,5 +1,6 @@ > #include <device/smbus_def.h> > #include "i82371eb.h" > +#include <console/console.h> > > #define SMBHST_STATUS 0x0 > #define SMBHST_CTL 0x2 > @@ -15,12 +16,12 @@ > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > - outb(0x80, 0x80); > - outb(0x80, 0x80); > - outb(0x80, 0x80); > - outb(0x80, 0x80); > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > + post_code(POST_SMBUS_DELAY); > + post_code(POST_SMBUS_DELAY); > + post_code(POST_SMBUS_DELAY); > + post_code(POST_SMBUS_DELAY); > + post_code(POST_SMBUS_DELAY); > } Same here. > Index: src/southbridge/intel/i82801ax/smbus.h > =================================================================== > --- src/southbridge/intel/i82801ax/smbus.h (revision 6380) > +++ src/southbridge/intel/i82801ax/smbus.h (working copy) > @@ -19,6 +19,7 @@ > */ > > #include <device/smbus_def.h> > +#include <console/console.h> > #include "i82801ax.h" > > int do_smbus_read_byte(u16 smbus_io_base, u8 device, u8 address); Is this needed? No other changes happen in this file? > Index: src/southbridge/intel/i82801bx/smbus.h > =================================================================== > --- src/southbridge/intel/i82801bx/smbus.h (revision 6380) > +++ src/southbridge/intel/i82801bx/smbus.h (working copy) > @@ -19,6 +19,7 @@ > */ > > #include <device/smbus_def.h> > +#include <console/console.h> > > static void smbus_delay(void) > { Is this needed? No other changes happen in this file? > Index: src/southbridge/intel/i82801cx/early_smbus.c > =================================================================== > --- src/southbridge/intel/i82801cx/early_smbus.c (revision 6380) > +++ src/southbridge/intel/i82801cx/early_smbus.c (working copy) > @@ -1,5 +1,6 @@ > #include <device/pci_ids.h> > #include "i82801cx.h" > +#include <console/console.h> > > static void enable_smbus(void) > { > @@ -21,7 +22,7 @@ > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } > > // See http://www.coreboot.org/pipermail/linuxbios/2004-September/009077.html No use of post_code here (inb or 0xeb instead) > Index: src/southbridge/intel/i82801dx/early_smbus.c > =================================================================== > --- src/southbridge/intel/i82801dx/early_smbus.c (revision 6380) > +++ src/southbridge/intel/i82801dx/early_smbus.c (working copy) > @@ -19,6 +19,7 @@ > */ > > #include "i82801dx.h" > +#include <console/console.h> > > #define SMBHSTSTAT 0x0 > #define SMBHSTCTL 0x2 > @@ -56,7 +57,7 @@ > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } > > static int smbus_wait_until_active(void) Same here. > Index: src/southbridge/intel/i82801ex/smbus.h > =================================================================== > --- src/southbridge/intel/i82801ex/smbus.h (revision 6380) > +++ src/southbridge/intel/i82801ex/smbus.h (working copy) > @@ -1,4 +1,5 @@ > #include <device/smbus_def.h> > +#include <console/console.h> > > #define SMBHSTSTAT 0x0 > #define SMBHSTCTL 0x2 > @@ -17,7 +18,7 @@ > > static void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } > > static int smbus_wait_until_ready(unsigned smbus_io_base) Same here. > Index: src/southbridge/intel/i3100/smbus.h > =================================================================== > --- src/southbridge/intel/i3100/smbus.h (revision 6380) > +++ src/southbridge/intel/i3100/smbus.h (working copy) > @@ -20,6 +20,7 @@ > /* This code is based on src/southbridge/intel/esb6300/esb6300_smbus.h */ > > #include <device/smbus_def.h> > +#include <console/console.h> > > #define SMBHSTSTAT 0x0 > #define SMBHSTCTL 0x2 > @@ -38,7 +39,7 @@ > > static void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } > > static int smbus_wait_until_ready(u32 smbus_io_base) Same here. > Index: src/southbridge/sis/sis966/early_smbus.c > =================================================================== > --- src/southbridge/sis/sis966/early_smbus.c (revision 6380) > +++ src/southbridge/sis/sis966/early_smbus.c (working copy) > @@ -20,12 +20,13 @@ > */ > > #include "smbus.h" > +#include <console/console.h> > > #define SMBUS0_IO_BASE 0x8D0 > > static inline void smbus_delay(void) > { > - outb(0x80, 0x80); > + post_code(POST_SMBUS_DELAY); > } Same here. > Index: src/include/console/console.h > =================================================================== > --- src/include/console/console.h (revision 6380) > +++ src/include/console/console.h (working copy) > @@ -22,6 +22,7 @@ > > #include <stdint.h> > #include <console/loglevel.h> > +#include <console/post_codes.h> > > #ifndef __PRE_RAM__ > void console_tx_byte(unsigned char byte); Is this needed? No other changes happen in this file? > Index: src/include/console/post_codes.h > =================================================================== > --- src/include/console/post_codes.h (revision 0) > +++ src/include/console/post_codes.h (revision 0) > @@ -0,0 +1,182 @@ > +/* > + * This file is part of the coreboot project. > + * > + * Copyright (C) 2011 Alexandru Gagniuc <mr.nuke...@gmail.com> > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 3 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +/** > + * @file post_codes.h > + * > + * This aims to be a central point for POST codes used throughout coreboot. > + * All POST codes should be declared here as macros, and post_code() should > + * be used with the macros instead of hardcoded values. This allows us to > + * quicly reference POST codes when nothing is working > + * > + * The format for a POST code macro is > + * #define POST_WHAT_WE_COMMUNICATE_IS_HAPPENING_WHEN_THIS_CODE_IS_POSTED > + * Lets's keep it at POST_* instead of POST_CODE_* > + * > + * DOCUMENTATION: > + * Please document any and all post codes using Doxygen style comments. We > + * want to be able to generate a verbose enough documentation that is useful > + * during debugging. Failure to do so will result in your patch being > rejected > + * without any explanation or effort on part of the maintainers. > + * > + */ > +#ifndef POST_CODES_H > +#define POST_CODES_H > + > +/** > + * \brief Entry into 'crt0.s'. reset code jumps to here > + * > + * First instruction that gets executed after the reset vector jumps. > + */ > +#define POST_ENTRY_CRT0_S 0x01 > + > +/** > + * \brief Entry into protected mode > + * > + * Preparing to enter protected mode. This is POSTed right before changing to > + * protected mode. > + */ > +#define POST_ENTER_PROTECTED_MODE 0x10 > + > +/** > + * \brief Start copying coreboot to RAM with decompression if compressed > + * > + * POSTed before ramstage is about to be loaded into memory > + */ > +#define POST_PREPARE_RAMSTAGE 0x11 > + > +/** > + * \brief Copy/decompression finished; jumping to RAM > + * > + * This is called after ramstage is loaded in memory, and before > + * the code jumps there. This represents the end of romstage. > + */ > +#define POST_RAMSTAGE_IS_PREPARED 0x12 > + > + > +/** > + * \brief Entry into c_start > + * > + * c_start.S is the first code executing in ramstage. > + */ > +#define POST_ENTRY_C_START 0x13 > + > +/** > + * \brief Delay the SMBUS > + * > + * This is used mostly by SMBUS code to insert a delay > + * FIXME: This shouldn't be used by the SMBUS code. > + * If you feel you need to use this, ask us about "smbus refactoring" > + */ > +#define POST_SMBUS_DELAY 0x80 > + > +/** > + * \brief Entry into coreboot in hardwaremain (RAM) > + * > + * This is the first call in hardwaremain.c. If this code is POSTed, then > + * ramstage has succesfully loaded. > + */ > +#define POST_ENTRY_RAMSTAGE 0x80 > + > +/** > + * \brief Console is initialized > + * > + * The console is initialized and is ready for usage > + */ > +#define POST_CONSOLE_READY 0x39 > + > +/** > + * \brief Console boot message succeeded > + * > + * First console message has been succesfully sent through the console > backend > + * driver. > + */ > +#define POST_CONSOLE_BOOT_MSG 0x40 > + > +/** > + * \brief Devices have been enumerated > + * > + * Bus scan, and device enumeration has completed. > + */ > +#define POST_DEVICE_ENUMERATION_COMPLETE 0x66 > + > +/** > + * \brief Devices have been configured > + * > + * Device confgration has completed. > + */ > +#define POST_DEVICE_CONFIGURATION_COMPLETE 0x88 > + > +/** > + * \brief Devices have been enabled > + * > + * Devices have been enabled. > + */ > +#define POST_DEVICES_ENABLED 0x89 > + > +/** > + * \brief UNUSED - Entry into elf boot > + * > + * This POST code is unused. The intent of its author is unknown, but is kept > + * here for reference. > + */ > +#define POST_ENTER_ELF_BOOT 0xf8 > +/** > + * \brief Jumping to payload > + * > + * Called right before jumping to a payload. If the boot sequence stops with > + * this code, chances are the payload freezes. > + */ > +#define POST_JUMPING_TO_PAYLOAD 0xf3 > +/** > + * \brief Not supposed to get here > + * > + * A function that should not have returned, returned > + * > + * Check the console output for details. > + */ > +#define POST_DEAD_CODE 0xee > + > +/** > + * \brief Pre call to hardwaremain() > + * > + * POSTed right before hardwaremain is called from c_start.S > + * TODO: Change this code to a lower number > + */ > +#define POST_PRE_HARDWAREMAIN 0xfe > + > +/** > + * \brief Elfload fail or die() called > + * > + * Coreboot was not able to load the payload, no payload was detected > + * or die() was called. > + * \n > + * If this code appears before entering ramstage, then most likely > + * ramstage is corrupted, and reflashing of the ROM chip is needed. > + * \n > + * If this code appears after ramstage, there is a problem with the payload > + * If the payload was built out-of-tree, check that it was compiled as > + * a coreboot payload > + * \n > + * Check the console output to see exactly where the failure occured. > + */ > +#define POST_DIE 0xff > + > + > +#endif /* THE_ALMIGHTY_POST_CODES_H */ Nice. I like this. Can we put this in one file together with src/include/cpu/amd/geode_post_code.h src/include/cpu/x86/post_code.h ? > Index: src/cpu/intel/car/cache_as_ram.inc > =================================================================== > --- src/cpu/intel/car/cache_as_ram.inc (revision 6380) > +++ src/cpu/intel/car/cache_as_ram.inc (working copy) > @@ -24,6 +24,7 @@ > #include <cpu/x86/stack.h> > #include <cpu/x86/mtrr.h> > #include <cpu/x86/lapic_def.h> > +#include <console/post_codes.h> > > #define CacheSize CONFIG_DCACHE_RAM_SIZE > #define CacheBase (0xd0000 - CacheSize) > @@ -296,7 +297,7 @@ > movl $0x4000, %edx > movb %ah, %al > .testx1: > - outb %al, $0x80 > + post_code(%al) I think this will not work. Since post_code looks like this: #define post_code(value) \ movb $value, %al; \ outb %al, $0x80 it will end up being: movb $%al, %al // invalid outb %al, $0x80 I think we should refrain from putting random values out on post_code here anyways. Either put in a defined value or drop it alltogether. > Index: src/boot/selfboot.c > =================================================================== > --- src/boot/selfboot.c (revision 6380) > +++ src/boot/selfboot.c (working copy) > @@ -553,7 +553,7 @@ > boot_successful(); > > printk(BIOS_DEBUG, "Jumping to boot code at %x\n", entry); > - post_code(0xfe); > + post_code(POST_PRE_HARDWAREMAIN); > > /* Jump to kernel */ > jmp_to_elf_entry((void*)entry, bounce_buffer, bounce_size); POST_PRE_HARDWAREMAIN sounds like a bad name for something that jumps to a (linux) kernel ? > Index: src/northbridge/via/cx700/early_serial.c > =================================================================== > --- src/northbridge/via/cx700/early_serial.c (revision 6380) > +++ src/northbridge/via/cx700/early_serial.c (working copy) > @@ -47,7 +47,7 @@ > > static void enable_cx700_serial(void) > { > - outb(6, 0x80); > + post_code(0x06); > > // WTH? > outb(0x03, 0x22); > @@ -98,5 +98,5 @@ > // should be done. Dump a char for fun. > cx700_writesiobyte(0x3f8, 48); > > - outb(7, 0x80); > + post_code(0x07); > } Maybe drop (one of) them? > Index: src/northbridge/via/cx700/early_smbus.c > =================================================================== > --- src/northbridge/via/cx700/early_smbus.c (revision 6380) > +++ src/northbridge/via/cx700/early_smbus.c (working copy) > @@ -17,6 +17,8 @@ > * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > */ > > +#include <console/console.h> > + > // other bioses use this, too: > #define SMBUS_IO_BASE 0x0500 > > @@ -44,7 +46,7 @@ > #define I2C_TRANS_CMD 0x40 > #define CLOCK_SLAVE_ADDRESS 0x69 > > -#define SMBUS_DELAY() outb(0x80, 0x80) > +#define SMBUS_DELAY() post_code(POST_SMBUS_DELAY) > > /* Debugging macros. */ > #if CONFIG_DEBUG_SMBUS use different delay mechanism here (inb or 0xeb) > Index: src/northbridge/via/vx800/early_serial.c > =================================================================== > --- src/northbridge/via/vx800/early_serial.c (revision 6380) > +++ src/northbridge/via/vx800/early_serial.c (working copy) > @@ -55,7 +55,7 @@ > > void enable_vx800_serial(void) > { > - outb(6, 0x80); > + post_code(0x06); > outb(0x03, 0x22); > > //pci_write_config8(PCI_DEV(0,17,0),0xb4,0x7e); > @@ -97,5 +97,5 @@ > vx800_writesiobyte(0x3f9, 0xf); > // should be done. Dump a char for fun. > vx800_writesiobyte(0x3f8, 48); > - outb(7, 0x80); > + post_code(0x07); > } Maybe drop (one of) them? > Index: src/northbridge/via/vx800/early_smbus.c > =================================================================== > --- src/northbridge/via/vx800/early_smbus.c (revision 6380) > +++ src/northbridge/via/vx800/early_smbus.c (working copy) > @@ -47,7 +47,7 @@ > #define I2C_TRANS_CMD 0x40 > #define CLOCK_SLAVE_ADDRESS 0x69 > > -#define SMBUS_DELAY() outb(0x80, 0x80) > +#define SMBUS_DELAY() post_code(POST_SMBUS_DELAY) > > #ifdef CONFIG_DEBUG_SMBUS > #define PRINT_DEBUG(x) print_debug(x) use different delay mechanism here (inb or 0xeb) > Index: src/northbridge/amd/gx2/raminit.c > =================================================================== > --- src/northbridge/amd/gx2/raminit.c (revision 6380) > +++ src/northbridge/amd/gx2/raminit.c (working copy) > @@ -576,7 +576,7 @@ > > /* wait 200 SDCLKs */ > for (i = 0; i < 200; i++) > - outb(0xaa, 0x80); > + post_code(0xaa); > > /* load RDSYNC */ > msr = rdmsr(MC_CF_RDSYNC); use different delay mechanism here (inb or 0xeb) > Index: src/arch/x86/init/prologue.inc > =================================================================== > --- src/arch/x86/init/prologue.inc (revision 6380) > +++ src/arch/x86/init/prologue.inc (working copy) > @@ -19,11 +19,12 @@ > > #include <cpu/x86/post_code.h> > #include <cpu/x86/stack.h> > +#include <console/post_codes.h> > > .section ".rom.data", "a", @progbits > .section ".rom.text", "ax", @progbits > > /* This is the entry code. The code in the .reset section jumps here. */ > > - post_code(0x01) > + post_code(POST_ENTRY_CRT0_S) better name would be nice. I hope we can drop the name crt0.S some time, completely. Stefan -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot