[PATCH] outb(* 0x80) to post_code()

[Alex G.]

Applies to fresh svn.......................[OK]
abuild run......................................[OK]

See patch.

Alex

[Alex G.]

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

---
Oops, forgot to include that.

[Alex G.]

Find attached the version with the colloquial verbiage abridged.

Alex

[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

[Patrick Georgi]

Am 25.02.2011 22:10, schrieb Stefan Reinauer:
> > > 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.

He asked on #coreboot about this, and I told him to do it this way (or 
not change those outbs at all), as long as the smbus_* functions that 
are the same stay the same (for future refactorings, see Infrastructure 
Projects).
Of course, inb 0x80 or outb 0xeb would be nicer (as that doesn't spam 
POST), but IMHO we had this for so long and it didn't seriously hurt, 
and can survive some more.

patrick

[Stefan Reinauer]

* Patrick Georgi  [110225 22:37]:
> Am 25.02.2011 22:10, schrieb Stefan Reinauer:
> > > > 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.
>
> He asked on #coreboot about this, and I told him to do it this way
> (or not change those outbs at all), as long as the smbus_* functions
> that are the same stay the same (for future refactorings, see
> Infrastructure Projects).
> Of course, inb 0x80 or outb 0xeb would be nicer (as that doesn't
> spam POST), but IMHO we had this for so long and it didn't seriously
> hurt, and can survive some more.

Hm. If we touch it now, why not fix it so we don't have to care anymore?
Some smbus drivers (like the ICH7 one) are using inb already, and it
works fine.

[Patrick Georgi]

Am 25.02.2011 22:45, schrieb Stefan Reinauer:
> Hm. If we touch it now, why not fix it so we don't have to care anymore?
> Some smbus drivers (like the ICH7 one) are using inb already, and it
> works fine.

I didn't want to block this task because of that other one, it's great 
that Alexandru takes care of it.
Of course, I'd love to see smbus* fixed (or even better, refactored and 
fixed)

Patrick

[Stefan Reinauer]

* Patrick Georgi  [110225 22:55]:
> Am 25.02.2011 22:45, schrieb Stefan Reinauer:
> > Hm. If we touch it now, why not fix it so we don't have to care anymore?
> > Some smbus drivers (like the ICH7 one) are using inb already, and it
> > works fine.
>
> I didn't want to block this task because of that other one, it's
> great that Alexandru takes care of it.
> Of course, I'd love to see smbus* fixed (or even better, refactored
> and fixed)

refactored... should we move the delay function to a common place?

[Alex G.]

Hi Stefan and Patrick. I Just saw your emails.

inb(0x80) or post_code(POST_SMBUS_DELAY):
make up your minds :)

Extra #include post_codes:
*mrnuke starts chopping
> Can we put this in one file together with
>   src/include/cpu/amd/geode_post_code.h

Looks interesting. Looking into that. Using this will totally obsolete
documentation/POSTCODES, which I used as the basis. If you prefer to use
these codes, say "green".
> src/include/cpu/x86/post_code.h

No. This would ruin the behavior of post_code() in console.c, which also
outputs to console if the option is selected.

On 02/25/2011 11:59 PM, Stefan Reinauer wrote:
> refactored... should we move the delay function to a common place?

I can move it. Just which file ? Also, wouldn't this make a single patch
too hard to swallow?

Alex

[Stefan Reinauer]

* Alex G.  [110225 23:26]:
> Hi Stefan and Patrick. I Just saw your emails.
>
> inb(0x80) or post_code(POST_SMBUS_DELAY):
> make up your minds :)

The second one seems wrong. It's not a post_code, but a delay that
happens to print some garbage on a post card. Changing that into
post_code() silently pretends that this is done on purpose. Not good, in
my opinion.
> > Can we put this in one file together with
> >   src/include/cpu/amd/geode_post_code.h
>
> Looks interesting. Looking into that. Using this will totally obsolete
> documentation/POSTCODES, which I used as the basis. If you prefer to use
> these codes, say "green".
> > src/include/cpu/x86/post_code.h
>
> No. This would ruin the behavior of post_code() in console.c, which also
> outputs to console if the option is selected.

Why? Just add #ifdef ASSEMBLY around it. Then it won't be visible in
console.c (or console.h for that matter)
> On 02/25/2011 11:59 PM, Stefan Reinauer wrote:
> > refactored... should we move the delay function to a common place?
>
> I can move it. Just which file ? Also, wouldn't this make a single patch
> too hard to swallow?

Yes. I think you should remove all changes to smbus* files from your
patch and then we can look at the issue separately.

[Alex G.]

Signed-off-by Alexandru Gagniuc 
---
On 02/26/2011 02:58 AM, Stefan Reinauer wrote:
> > > Can we put this in one file together with

[...]
> > > src/include/cpu/x86/post_code.h
> >
> > No. This would ruin the behavior of post_code() in console.c, which also
> > outputs to console if the option is selected.
>
> Why? Just add #ifdef ASSEMBLY around it. Then it won't be visible in
> console.c (or console.h for that matter)

Aaah. The obscure quirks. done.
> > On 02/25/2011 11:59 PM, Stefan Reinauer wrote:
> > > refactored... should we move the delay function to a common place?
> >
> > I can move it. Just which file ? Also, wouldn't this make a single patch
> > too hard to swallow?
>
> Yes. I think you should remove all changes to smbus* files from your
> patch and then we can look at the issue separately.

Been there. Done that.

[Stefan Reinauer]

* Alex G.  [110226 02:35]:
> 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,350 @@
> +/*
> 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/.
> */

Due to the GPLv2 only nature of many source code files, we can not allow
GPLv3 or even GPLv3 or later code to be committed to the repository.
Please make this GPLv2 if possible.

Stefan

[Alex G.]

On 03/01/2011 11:09 PM, Stefan Reinauer wrote:
> Alex G.  [110226 02:35]:
> > 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,350 @@
> > +/*
> > 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/.
> > */
>
> Due to the GPLv2 only nature of many source code files, we can not allow
> GPLv3 or even GPLv3 or later code to be committed to the repository.
> Please make this GPLv2 if possible.

You may, if you wish, change the license to GPLv2+.

Signed-off-by: Alexandru Gagniuc

[Alex G.]

On 03/02/2011 10:55 AM, Alex G. wrote:
> On 03/01/2011 11:09 PM, Stefan Reinauer wrote:
> > Due to the GPLv2 only nature of many source code files, we can not allow
> > GPLv3 or even GPLv3 or later code to be committed to the repository.
> > Please make this GPLv2 if possible.
>
> You may, if you wish, change the license to GPLv2+.

Nevermind that. I'll resubmit the patch. I mess it, I fix it :)

Alex

[Alex G.]

Signed-off-by: Alexandru Gagniuc 
---