patch: add in smbus support for mcp55

[ron minnich]

This patch adds in a bit more support, this is the mcp55 smbus.

This also illustrates the need to figure out the pci stuff in a better
way. It works for now, but we need to do better.

But we'll figure out it once we have a full port under our belts, and
we can come up with something that works right. We're still
getting over the v2 practice of including .c files :-) [Which, lest we
forget, was the best way to do things given that we did NOT have
cache as ram -- I intend no criticism at all!]

Signed-off-by: Ronald G. Minnich

[Carl-Daniel Hailfinger]

On 06.08.2008 02:23, ron minnich wrote:
> This patch adds in a bit more support, this is the mcp55 smbus.
>
> This also illustrates the need to figure out the pci stuff in a better
> way. It works for now, but we need to do better.
>
> But we'll figure out it once we have a full port under our belts, and
> we can come up with something that works right. We're still
> getting over the v2 practice of including .c files :-) [Which, lest we
> forget, was the best way to do things given that we did NOT have
> cache as ram -- I intend no criticism at all!]
>
> Signed-off-by: Ronald G. Minnich

A few comments below. In general, using printk more often would be nice
since we're not constrained by romcc anymore. IMO we also should settle
for a delay mechanism which doesn't clobber POST codes.
> Index: southbridge/nvidia/mcp55/mcp55_smbus.h
>
> --- southbridge/nvidia/mcp55/mcp55_smbus.h	(revision 0)
> +++ southbridge/nvidia/mcp55/mcp55_smbus.h	(revision 0)
> @@ -0,0 +1,194 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2004 Tyan Computer
> Written by Yinghai Lu  for Tyan Computer.
> Copyright (C) 2006,2007 AMD
> Written by Yinghai Lu  for AMD.
> 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 2 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, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <device/smbus_def.h>
>
> +#define SMBHSTSTAT	0x1
> +#define SMBHSTPRTCL	0x0
> +#define SMBHSTCMD	0x3
> +#define SMBXMITADD	0x2
> +#define SMBHSTDAT0	0x4
> +#define SMBHSTDAT1	0x5
>
> +/* Between 1-10 seconds, We should never timeout normally
> Longer than this is just painful when a timeout condition occurs.
> */
>
> +#define SMBUS_TIMEOUT	(100*1000*10)
>
> +static inline void smbus_delay(void)
> +{
> outb(0x80, 0x80);
>
> +}

Can we use inb(0x80) or outb(0x80,0x84) instead? That way, we won't mess
us POST code logging.
> +static int smbus_wait_until_ready(unsigned smbus_io_base)

Can we use explicit length types like u16 instead of unsigned? That
applies to almost all functions.
> +{
> unsigned long loops;
> loops = SMBUS_TIMEOUT;
> do {
> unsigned char val;
> smbus_delay();
> val = inb(smbus_io_base + SMBHSTSTAT);
> val &= 0x1f;
> if (val == 0) {
> 	return 0;
> }
> outb(val,smbus_io_base + SMBHSTSTAT);
> } while(--loops);
> return -2;

The -2 is a magic value. A #define would be appreciated.
> +}
>
> +static int smbus_wait_until_done(unsigned smbus_io_base)

u16
> +{
> unsigned long loops;
> loops = SMBUS_TIMEOUT;
> do {
> unsigned char val;
> smbus_delay();
> val = inb(smbus_io_base + SMBHSTSTAT);
> if ( (val & 0xff) != 0) {
> 	return 0;
> }
> } while(--loops);
> return -3;

Magic value.
> +}

Missing empty line between functions.
> +static int do_smbus_recv_byte(unsigned smbus_io_base, unsigned device)

u16/u32
> +{
> unsigned char global_status_register;
> unsigned char byte;
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1)|1 , smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* byte data recv */
> outb(0x05, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;

Magic value.
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */

Do we want to lose a check here or do we want to check loosely?
> /* read results of transaction */
> byte = inb(smbus_io_base + SMBHSTCMD);
> if (global_status_register != 0x80) { // lose check, otherwise it should be 0
> return -1;

Magic value.
> }
> return byte;
>
> +}

Missing empty line between functions.
> +static int do_smbus_send_byte(unsigned smbus_io_base, unsigned device, unsigned char val)

u16/u32
> +{
> unsigned global_status_register;
> outb(val, smbus_io_base + SMBHSTDAT0);
> smbus_delay();
> /* set the command... */
> outb(val, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* set up for a byte data write */
> outb(0x04, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;

Magic value.
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */;
> if (global_status_register != 0x80) {
> return -1;

Magic value.
> }
> return 0;
>
> +}

Missing empty line between functions.
> +static int do_smbus_read_byte(unsigned smbus_io_base, unsigned device, unsigned address)

u16/u32/u8
> +{
> unsigned char global_status_register;
> unsigned char byte;
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1)|1 , smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* set the command/address... */
> outb(address & 0xff, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* byte data read */
> outb(0x07, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */
> /* read results of transaction */
> byte = inb(smbus_io_base + SMBHSTDAT0);
> if (global_status_register != 0x80) { // lose check, otherwise it should be 0
> return -1;
> }
> return byte;
>
> +}

Two empty lines.
> +static int do_smbus_write_byte(unsigned smbus_io_base, unsigned device, unsigned address, unsigned char val)

u16/u32/u8
> +{
> unsigned global_status_register;
> outb(val, smbus_io_base + SMBHSTDAT0);
> smbus_delay();
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBXMITADD);
> smbus_delay();
> outb(address & 0xff, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* set up for a byte data write */
> outb(0x06, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */;
> if (global_status_register != 0x80) {
> return -1;
> }
> return 0;
>
> +}
> Index: southbridge/nvidia/mcp55/stage1_smbus.c
>
> --- southbridge/nvidia/mcp55/stage1_smbus.c	(revision 0)
> +++ southbridge/nvidia/mcp55/stage1_smbus.c	(revision 0)
> @@ -0,0 +1,93 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2004 Tyan Computer
> Written by Yinghai Lu  for Tyan Computer.
> Copyright (C) 2006,2007 AMD
> Written by Yinghai Lu  for AMD.
> 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 2 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, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <console.h>
> +#include <io.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include "mcp55.h"
> +#define pci_read_config32(bus, dev, where) pci_cf8_conf1.read32(NULL, bus, dev, where)
> +#define pci_write_config32(bus, dev, where, what) pci_cf8_conf1.write32(NULL, bus, dev, where, what)
> +#define pci_read_config16(bus, dev, where) pci_cf8_conf1.read32(NULL, bus, dev, where)
> +#define pci_write_config16(bus, dev, where, what) pci_cf8_conf1.write32(NULL, bus, dev, where, what)
> +#include "mcp55_smbus.h"
>
> +#define SMBUS0_IO_BASE	0x1000
> +#define SMBUS1_IO_BASE	(0x1000+(1<<8))
> +/*SIZE 0x40 */
>
> +static void enable_smbus(void)
> +{
> u32 dev;
> dev = pci_locate_device(PCI_ID(0x10de, 0x0368), 0);
>
> +#if 0
> if (dev == PCI_DEV_INVALID) {
> die("SMBUS controller not found\r\n");
> }
> print_debug("SMBus controller enabled\r\n");

printk
> +#endif
> /* set smbus iobase */
> pci_write_config32(0, dev, 0x20, SMBUS0_IO_BASE | 1);
> pci_write_config32(0, dev, 0x24, SMBUS1_IO_BASE | 1);
> /* Set smbus iospace enable */
> pci_write_config16(0, dev, 0x4, 0x01);
> /* clear any lingering errors, so the transaction will run */
> outb(inb(SMBUS0_IO_BASE + SMBHSTSTAT), SMBUS0_IO_BASE + SMBHSTSTAT);
> outb(inb(SMBUS1_IO_BASE + SMBHSTSTAT), SMBUS1_IO_BASE + SMBHSTSTAT);
>
> +}
>
> +static int smbus_recv_byte(unsigned device)

u32?
> +{
> return do_smbus_recv_byte(SMBUS0_IO_BASE, device);
>
> +}

Missing empty line between functions.
> +static int smbus_send_byte(unsigned device, unsigned char val)

u32?
> +{
> return do_smbus_send_byte(SMBUS0_IO_BASE, device, val);
>
> +}

Missing empty line between functions.
> +static int smbus_read_byte(unsigned device, unsigned address)

u32?
> +{
> return do_smbus_read_byte(SMBUS0_IO_BASE, device, address);
>
> +}

Missing empty line between functions.
> +static int smbus_write_byte(unsigned device, unsigned address, unsigned char val)

u32/u16?
> +{
> return do_smbus_write_byte(SMBUS0_IO_BASE, device, address, val);
>
> +}
>
> +static int smbusx_recv_byte(unsigned smb_index, unsigned device)

u8/u32?
> +{
> return do_smbus_recv_byte(SMBUS0_IO_BASE + (smb_index<<8), device);
>
> +}

Missing empty line between functions.
> +static int smbusx_send_byte(unsigned smb_index, unsigned device, unsigned char val)
> +{
> return do_smbus_send_byte(SMBUS0_IO_BASE + (smb_index<<8), device, val);
>
> +}

Missing empty line between functions.
> +static int smbusx_read_byte(unsigned smb_index, unsigned device, unsigned address)
> +{
> return do_smbus_read_byte(SMBUS0_IO_BASE + (smb_index<<8), device, address);
>
> +}

Missing empty line between functions.
> +static int smbusx_write_byte(unsigned smb_index, unsigned device, unsigned address, unsigned char val)
> +{
> return do_smbus_write_byte(SMBUS0_IO_BASE + (smb_index<<8), device, address, val);
>
> +}
> Index: mainboard/gigabyte/m57sli/Makefile
>
> --- mainboard/gigabyte/m57sli/Makefile	(revision 719)
> +++ mainboard/gigabyte/m57sli/Makefile	(working copy)
> @@ -20,7 +20,8 @@
>  ##
>
> STAGE0_MAINBOARD_OBJ := $(obj)/mainboard/$(MAINBOARDDIR)/stage1.o \
> 	$(obj)/mainboard/$(MAINBOARDDIR)/option_table.c
> 	$(obj)/mainboard/$(MAINBOARDDIR)/option_table.c \
> 	$(obj)/southbridge/nvidia/mcp55/stage1_smbus.o
>
> INITRAM_SRC =      $(src)/mainboard/$(MAINBOARDDIR)/initram.c

Regards,
Carl-Daniel

[ron minnich]

Try 2. I don't want to mess with the magic values yet. They're all
over v2. For the start I want to get this in and we can do the beauty
treatment later. All other concerns are addressed.

ron

[Peter Stuge]

On Wed, Aug 06, 2008 at 04:16:32PM -0700, ron minnich wrote:
> SMBUS support for mcp55 and v3.
>
> Signed-off-by: Ronald G. Minnich

Acked-by: Peter Stuge
> Index: southbridge/nvidia/mcp55/mcp55_smbus.h
>
> --- southbridge/nvidia/mcp55/mcp55_smbus.h	(revision 0)
> +++ southbridge/nvidia/mcp55/mcp55_smbus.h	(revision 0)
> @@ -0,0 +1,197 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2004 Tyan Computer
> Written by Yinghai Lu  for Tyan Computer.
> Copyright (C) 2006,2007 AMD
> Written by Yinghai Lu  for AMD.
> 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 2 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, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <device/smbus_def.h>
>
> +#define SMBHSTSTAT	0x1
> +#define SMBHSTPRTCL	0x0
> +#define SMBHSTCMD	0x3
> +#define SMBXMITADD	0x2
> +#define SMBHSTDAT0	0x4
> +#define SMBHSTDAT1	0x5
>
> +/* Between 1-10 seconds, We should never timeout normally
> Longer than this is just painful when a timeout condition occurs.
> */
>
> +#define SMBUS_TIMEOUT	(100*1000*10)
>
> +static inline void smbus_delay(void)
> +{
> /* let's hope this is not optimized out */
> (void) intb(0x80);
>
> +}
>
> +static int smbus_wait_until_ready(u16 smbus_io_base)
> +{
> unsigned long loops;
> loops = SMBUS_TIMEOUT;
> do {
> unsigned char val;
> smbus_delay();
> val = inb(smbus_io_base + SMBHSTSTAT);
> val &= 0x1f;
> if (val == 0) {
> 	return 0;
> }
> outb(val,smbus_io_base + SMBHSTSTAT);
> } while(--loops);
> return -2;
>
> +}
>
> +static int smbus_wait_until_done(u16 smbus_io_base)
> +{
> unsigned long loops;
> loops = SMBUS_TIMEOUT;
> do {
> unsigned char val;
> smbus_delay();
> val = inb(smbus_io_base + SMBHSTSTAT);
> if ( (val & 0xff) != 0) {
> 	return 0;
> }
> } while(--loops);
> return -3;
>
> +}
>
> +static int do_smbus_recv_byte(u16 smbus_io_base, unsigned u8)
> +{
> u8 global_status_register;
> u8 byte;
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1)|1 , smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* byte data recv */
> outb(0x05, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */
> /* read results of transaction */
> byte = inb(smbus_io_base + SMBHSTCMD);
> if (global_status_register != 0x80) { // loose check, otherwise it should be 0
> return -1;
> }
> return byte;
>
> +}
>
> +static int do_smbus_send_byte(u16 smbus_io_base, u8 device, u8 val)
> +{
> u8 global_status_register;
> outb(val, smbus_io_base + SMBHSTDAT0);
> smbus_delay();
> /* set the command... */
> outb(val, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* set up for a byte data write */
> outb(0x04, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */;
> if (global_status_register != 0x80) {
> return -1;
> }
> return 0;
>
> +}
>
> +static int do_smbus_read_byte(u16 smbus_io_base, u8 device, u8 address)
> +{
> u8 global_status_register;
> u8 byte;
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1)|1 , smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* set the command/address... */
> outb(address & 0xff, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* byte data read */
> outb(0x07, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */
> /* read results of transaction */
> byte = inb(smbus_io_base + SMBHSTDAT0);
> if (global_status_register != 0x80) { // lose check, otherwise it should be 0
> return -1;
> }
> return byte;
>
> +}
>
> +static int do_smbus_write_byte(u16 smbus_io_base, u8 device, u8 address, u8 val)
> +{
> u8 global_status_register;
> outb(val, smbus_io_base + SMBHSTDAT0);
> smbus_delay();
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBXMITADD);
> smbus_delay();
> outb(address & 0xff, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* set up for a byte data write */
> outb(0x06, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */;
> if (global_status_register != 0x80) {
> return -1;
> }
> return 0;
>
> +}
> Index: southbridge/nvidia/mcp55/stage1_smbus.c
>
> --- southbridge/nvidia/mcp55/stage1_smbus.c	(revision 0)
> +++ southbridge/nvidia/mcp55/stage1_smbus.c	(revision 0)
> @@ -0,0 +1,99 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2004 Tyan Computer
> Written by Yinghai Lu  for Tyan Computer.
> Copyright (C) 2006,2007 AMD
> Written by Yinghai Lu  for AMD.
> 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 2 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, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <console.h>
> +#include <io.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include "mcp55.h"
> +#define pci_read_config32(bus, dev, where) pci_cf8_conf1.read32(NULL, bus, dev, where)
> +#define pci_write_config32(bus, dev, where, what) pci_cf8_conf1.write32(NULL, bus, dev, where, what)
> +#define pci_read_config16(bus, dev, where) pci_cf8_conf1.read32(NULL, bus, dev, where)
> +#define pci_write_config16(bus, dev, where, what) pci_cf8_conf1.write32(NULL, bus, dev, where, what)
> +#include "mcp55_smbus.h"
>
> +#define SMBUS0_IO_BASE	0x1000
> +#define SMBUS1_IO_BASE	(0x1000+(1<<8))
> +/*SIZE 0x40 */
>
> +static void enable_smbus(void)
> +{
> u32 dev;
> dev = pci_locate_device(PCI_ID(0x10de, 0x0368), 0);
>
> +#if 0
> if (dev == PCI_DEV_INVALID) {
> die("SMBUS controller not found\r\n");
> }
> printk(BIOS_DEBUG, "SMBus controller enabled\r\n");
>
> +#endif
> /* set smbus iobase */
> pci_write_config32(0, dev, 0x20, SMBUS0_IO_BASE | 1);
> pci_write_config32(0, dev, 0x24, SMBUS1_IO_BASE | 1);
> /* Set smbus iospace enable */
> pci_write_config16(0, dev, 0x4, 0x01);
> /* clear any lingering errors, so the transaction will run */
> outb(inb(SMBUS0_IO_BASE + SMBHSTSTAT), SMBUS0_IO_BASE + SMBHSTSTAT);
> outb(inb(SMBUS1_IO_BASE + SMBHSTSTAT), SMBUS1_IO_BASE + SMBHSTSTAT);
>
> +}
>
> +static u8 smbus_recv_byte(u8 device)
> +{
> return do_smbus_recv_byte(SMBUS0_IO_BASE, device);
>
> +}
>
> +static u8 smbus_send_byte(u8 device,u8 val)
> +{
> return do_smbus_send_byte(SMBUS0_IO_BASE, device, val);
>
> +}
>
> +static u8 smbus_read_byte(u8 device, u8 address)
> +{
> return do_smbus_read_byte(SMBUS0_IO_BASE, device, address);
>
> +}
>
> +static u8 smbus_write_byte(u8 device, u8 address, u8 char val)
> +{
> return do_smbus_write_byte(SMBUS0_IO_BASE, device, address, val);
>
> +}
>
> +static u8 smbusx_recv_byte(u8 smb_index, u8 device)
> +{
> return do_smbus_recv_byte(SMBUS0_IO_BASE + (smb_index<<8), device);
>
> +}
>
> +static u8 smbusx_send_byte(u8 smb_index, u8 device, u8 char val)
> +{
> return do_smbus_send_byte(SMBUS0_IO_BASE + (smb_index<<8), device, val);
>
> +}
>
> +static u8 smbusx_read_byte(u8 smb_index, u8 device, u8 address)
> +{
> return do_smbus_read_byte(SMBUS0_IO_BASE + (smb_index<<8), device, address);
>
> +}
>
> +static u8 smbusx_write_byte(u8 smb_index, u8 device, u8 address, u8 char val)
> +{
> return do_smbus_write_byte(SMBUS0_IO_BASE + (smb_index<<8), device, address, val);
>
> +}
> Index: mainboard/gigabyte/m57sli/Makefile
>
> --- mainboard/gigabyte/m57sli/Makefile	(revision 719)
> +++ mainboard/gigabyte/m57sli/Makefile	(working copy)
> @@ -20,7 +20,8 @@
>  ##
>
> STAGE0_MAINBOARD_OBJ := $(obj)/mainboard/$(MAINBOARDDIR)/stage1.o \
> 	$(obj)/mainboard/$(MAINBOARDDIR)/option_table.c
> 	$(obj)/mainboard/$(MAINBOARDDIR)/option_table.c \
> 	$(obj)/southbridge/nvidia/mcp55/stage1_smbus.o
>
> INITRAM_SRC =      $(src)/mainboard/$(MAINBOARDDIR)/initram.c

[Carl-Daniel Hailfinger]

On 07.08.2008 01:16, ron minnich wrote:
> Try 2. I don't want to mess with the magic values yet. They're all
> over v2. For the start I want to get this in and we can do the beauty
> treatment later. All other concerns are addressed.
>
> SMBUS support for mcp55 and v3.
>
> Signed-off-by: Ronald G. Minnich

A few minor bugs killing compilation, if you fix them, it is
Acked-by: Carl-Daniel Hailfinger
> Index: southbridge/nvidia/mcp55/mcp55_smbus.h
>
> --- southbridge/nvidia/mcp55/mcp55_smbus.h	(revision 0)
> +++ southbridge/nvidia/mcp55/mcp55_smbus.h	(revision 0)
> @@ -0,0 +1,197 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2004 Tyan Computer
> Written by Yinghai Lu  for Tyan Computer.
> Copyright (C) 2006,2007 AMD
> Written by Yinghai Lu  for AMD.
> 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 2 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, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <device/smbus_def.h>
>
> +#define SMBHSTSTAT	0x1
> +#define SMBHSTPRTCL	0x0
> +#define SMBHSTCMD	0x3
> +#define SMBXMITADD	0x2
> +#define SMBHSTDAT0	0x4
> +#define SMBHSTDAT1	0x5
>
> +/* Between 1-10 seconds, We should never timeout normally
> Longer than this is just painful when a timeout condition occurs.
> */
>
> +#define SMBUS_TIMEOUT	(100*1000*10)
>
> +static inline void smbus_delay(void)
> +{
> /* let's hope this is not optimized out */
> (void) intb(0x80);

inb, not intb
> +}
>
> +static int smbus_wait_until_ready(u16 smbus_io_base)
> +{
> unsigned long loops;
> loops = SMBUS_TIMEOUT;
> do {
> unsigned char val;
> smbus_delay();
> val = inb(smbus_io_base + SMBHSTSTAT);
> val &= 0x1f;
> if (val == 0) {
> 	return 0;
> }
> outb(val,smbus_io_base + SMBHSTSTAT);
> } while(--loops);
> return -2;
>
> +}
>
> +static int smbus_wait_until_done(u16 smbus_io_base)
> +{
> unsigned long loops;
> loops = SMBUS_TIMEOUT;
> do {
> unsigned char val;
> smbus_delay();
> val = inb(smbus_io_base + SMBHSTSTAT);
> if ( (val & 0xff) != 0) {
> 	return 0;
> }
> } while(--loops);
> return -3;
>
> +}
>
> +static int do_smbus_recv_byte(u16 smbus_io_base, unsigned u8)

I'm pretty sure this was meant to be "u8 some_variable_name" and not
"unsigned u8".
> +{
> u8 global_status_register;
> u8 byte;
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1)|1 , smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* byte data recv */
> outb(0x05, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */
> /* read results of transaction */
> byte = inb(smbus_io_base + SMBHSTCMD);
> if (global_status_register != 0x80) { // loose check, otherwise it should be 0
> return -1;
> }
> return byte;
>
> +}
>
> +static int do_smbus_send_byte(u16 smbus_io_base, u8 device, u8 val)
> +{
> u8 global_status_register;
> outb(val, smbus_io_base + SMBHSTDAT0);
> smbus_delay();
> /* set the command... */
> outb(val, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* set up for a byte data write */
> outb(0x04, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */;
> if (global_status_register != 0x80) {
> return -1;
> }
> return 0;
>
> +}
>
> +static int do_smbus_read_byte(u16 smbus_io_base, u8 device, u8 address)
> +{
> u8 global_status_register;
> u8 byte;
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1)|1 , smbus_io_base + SMBXMITADD);
> smbus_delay();
> /* set the command/address... */
> outb(address & 0xff, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* byte data read */
> outb(0x07, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */
> /* read results of transaction */
> byte = inb(smbus_io_base + SMBHSTDAT0);
> if (global_status_register != 0x80) { // lose check, otherwise it should be 0
> return -1;
> }
> return byte;
>
> +}
>
> +static int do_smbus_write_byte(u16 smbus_io_base, u8 device, u8 address, u8 val)
> +{
> u8 global_status_register;
> outb(val, smbus_io_base + SMBHSTDAT0);
> smbus_delay();
> /* set the device I'm talking too */
> outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBXMITADD);
> smbus_delay();
> outb(address & 0xff, smbus_io_base + SMBHSTCMD);
> smbus_delay();
> /* set up for a byte data write */
> outb(0x06, smbus_io_base + SMBHSTPRTCL);
> smbus_delay();
> /* poll for transaction completion */
> if (smbus_wait_until_done(smbus_io_base) < 0) {
> return -3;
> }
> global_status_register = inb(smbus_io_base + SMBHSTSTAT) & 0x80; /* lose check */;
> if (global_status_register != 0x80) {
> return -1;
> }
> return 0;
>
> +}
> Index: southbridge/nvidia/mcp55/stage1_smbus.c
>
> --- southbridge/nvidia/mcp55/stage1_smbus.c	(revision 0)
> +++ southbridge/nvidia/mcp55/stage1_smbus.c	(revision 0)
> @@ -0,0 +1,99 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2004 Tyan Computer
> Written by Yinghai Lu  for Tyan Computer.
> Copyright (C) 2006,2007 AMD
> Written by Yinghai Lu  for AMD.
> 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 2 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, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <console.h>
> +#include <io.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include "mcp55.h"
> +#define pci_read_config32(bus, dev, where) pci_cf8_conf1.read32(NULL, bus, dev, where)
> +#define pci_write_config32(bus, dev, where, what) pci_cf8_conf1.write32(NULL, bus, dev, where, what)
> +#define pci_read_config16(bus, dev, where) pci_cf8_conf1.read32(NULL, bus, dev, where)
> +#define pci_write_config16(bus, dev, where, what) pci_cf8_conf1.write32(NULL, bus, dev, where, what)
> +#include "mcp55_smbus.h"
>
> +#define SMBUS0_IO_BASE	0x1000
> +#define SMBUS1_IO_BASE	(0x1000+(1<<8))
> +/*SIZE 0x40 */
>
> +static void enable_smbus(void)
> +{
> u32 dev;
> dev = pci_locate_device(PCI_ID(0x10de, 0x0368), 0);
>
> +#if 0
> if (dev == PCI_DEV_INVALID) {
> die("SMBUS controller not found\r\n");
> }
> printk(BIOS_DEBUG, "SMBus controller enabled\r\n");
>
> +#endif
> /* set smbus iobase */
> pci_write_config32(0, dev, 0x20, SMBUS0_IO_BASE | 1);
> pci_write_config32(0, dev, 0x24, SMBUS1_IO_BASE | 1);
> /* Set smbus iospace enable */
> pci_write_config16(0, dev, 0x4, 0x01);
> /* clear any lingering errors, so the transaction will run */
> outb(inb(SMBUS0_IO_BASE + SMBHSTSTAT), SMBUS0_IO_BASE + SMBHSTSTAT);
> outb(inb(SMBUS1_IO_BASE + SMBHSTSTAT), SMBUS1_IO_BASE + SMBHSTSTAT);
>
> +}
>
> +static u8 smbus_recv_byte(u8 device)
> +{
> return do_smbus_recv_byte(SMBUS0_IO_BASE, device);
>
> +}
>
> +static u8 smbus_send_byte(u8 device,u8 val)
> +{
> return do_smbus_send_byte(SMBUS0_IO_BASE, device, val);
>
> +}
>
> +static u8 smbus_read_byte(u8 device, u8 address)
> +{
> return do_smbus_read_byte(SMBUS0_IO_BASE, device, address);
>
> +}
>
> +static u8 smbus_write_byte(u8 device, u8 address, u8 char val)

"u8 char val" should be "u8 val"
> +{
> return do_smbus_write_byte(SMBUS0_IO_BASE, device, address, val);
>
> +}
>
> +static u8 smbusx_recv_byte(u8 smb_index, u8 device)
> +{
> return do_smbus_recv_byte(SMBUS0_IO_BASE + (smb_index<<8), device);
>
> +}
>
> +static u8 smbusx_send_byte(u8 smb_index, u8 device, u8 char val)

u8 char val...
> +{
> return do_smbus_send_byte(SMBUS0_IO_BASE + (smb_index<<8), device, val);
>
> +}
>
> +static u8 smbusx_read_byte(u8 smb_index, u8 device, u8 address)
> +{
> return do_smbus_read_byte(SMBUS0_IO_BASE + (smb_index<<8), device, address);
>
> +}
>
> +static u8 smbusx_write_byte(u8 smb_index, u8 device, u8 address, u8 char val)

u8 char val
> +{
> return do_smbus_write_byte(SMBUS0_IO_BASE + (smb_index<<8), device, address, val);
>
> +}
> Index: mainboard/gigabyte/m57sli/Makefile
>
> --- mainboard/gigabyte/m57sli/Makefile	(revision 719)
> +++ mainboard/gigabyte/m57sli/Makefile	(working copy)
> @@ -20,7 +20,8 @@
>  ##
>
> STAGE0_MAINBOARD_OBJ := $(obj)/mainboard/$(MAINBOARDDIR)/stage1.o \
> 	$(obj)/mainboard/$(MAINBOARDDIR)/option_table.c
> 	$(obj)/mainboard/$(MAINBOARDDIR)/option_table.c \
> 	$(obj)/southbridge/nvidia/mcp55/stage1_smbus.o
>
> INITRAM_SRC =      $(src)/mainboard/$(MAINBOARDDIR)/initram.c

Regards,
Carl-Daniel

[ron minnich]

oh, sorry, will fix those typos later. Geesh, I am on 1/2 rations for
brains today!

ron

[ron minnich]

Committed revision 728.

with your corrections.

I really kind of need a go/no-go on the PCI stuff I just posted. I am fine with
either an ACK OR a "no, but here is the right patch, what do you think?"

A code-less NAK would be less okay :-)

Anybody?

thanks

ron

[Peter Stuge]

On Sat, Aug 09, 2008 at 01:47:48PM -0700, ron minnich wrote:
> Committed revision 728.
>
> with your corrections.

cool.
> I really kind of need a go/no-go on the PCI stuff I just posted. I
> am fine with either an ACK OR a "no, but here is the right patch,
> what do you think?"
>
> A code-less NAK would be less okay :-)

Hence not naking.

I'm basically asking if we can and want to access some sort of tree
this early on?

//Peter

[ron minnich]

On Sat, Aug 9, 2008 at 1:57 PM, Peter Stuge  wrote:
.
> I'm basically asking if we can and want to access some sort of tree
> this early on?

I don't see a need. Early in stage1 you are doing very little with a
very small number of devices, and you pretty much know what and where
they are. I just don't see the need to add the huge amount of code it
would add. I see the beauty of it, but sometimes things can get
overdesigned. This is one such case.

Consider that on the m57sli, which is a pretty complicated platform, we
touch
0:18.x and 0:19.x
usb config space
smbus config space

All of which are known to be there and known as to what they are. I
think we should be very careful to clutter up stage 1-- it's supposed
to be very short and limited and finished quickly.

Bootstraps are always kind of ugly, and stage1 is the ugly bootstrap.
Thanks

ron