Re: [coreboot] [PATCH] outb(* 0x80) to post_code()

[Stefan Reinauer]

* Alex G.  [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

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
> 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