Re: [coreboot] mcp55 patch

[Carl-Daniel Hailfinger]

On 09.08.2008 23:19, ron minnich wrote:
> This patch reconciles mcp55 to the new pci changes.
>
> Gets much further. Adds usb debug code.
>
> Last change of the day.
>
> Add stage1.h to the files for standard defines used by all stage1 functions.
>
> Modify all functions to use the new v3 pci operations calling conventions. 
> Uncomment delayx and let it take a full int.
>
> Add the USB debug support function. 
> This is compiling pretty well for me save for the missing hypertransport function.
>
> Signed-off-by: Ronald G. Minnich

It would be great if you could split the big patch in a few smaller
ones, each doing one thing. Suggested splitting mentioned in the review
below.
The Ack is for all small patches (or, if unavoidable, for the big one).

Acked-by: Carl-Daniel Hailfinger
> Index: southbridge/nvidia/mcp55/stage1.c
>
> --- southbridge/nvidia/mcp55/stage1.c	(revision 726)
> +++ southbridge/nvidia/mcp55/stage1.c	(working copy)
> @@ -26,8 +26,7 @@
>  #include <cpu.h>
>  #include <amd/k8/k8.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)
> +#include "stage1.h"
>
> #warning fix disgusting define of MCP55_NUM it is mainboard dependent
>  #define MCP55_NUM 1
> @@ -139,13 +138,12 @@
>
> }
> -static void delayx(u8 value) {
> -#if 1
> +static void delayx(unsigned int value) {
> int i;
>  for(i=0;i<0x8000;i++) {
> outb(value, 0x80);
> inb(0x80);
> }
>
> -#endif
>  }

delayx() as a separate patch, please. It's useful and we may want to do
the same in v2.
Or we could kill it altogether and use udelay() from
arch/x86/udelay_io.c. It's the same code anyway.
> static void mcp55_early_pcie_setup(unsigned busnx, unsigned devnx, unsigned anactrl_io_base, unsigned pci_e_x)
> @@ -154,11 +152,10 @@
>    u32 pll_ctrl;
>    u32 dword;
>    int i;
> u32 dev;
> //	dev = PCI_BDF(busnx, devnx+1, 1);
> dword = pci_read_config32(busnx, (devnx+1)<<3 | 1, 0xe4);
> u32 bdf = PCI_BDF(busnx, devnx+1, 1);
> dword = pci_read_config32(bdf, 0xe4);
> dword |= 0x3f0; // disable it at first
> pci_write_config32(busnx,  (devnx+1)<<3 | 1, 0xe4, dword);
>
> pci_write_config32(bdf, 0xe4, dword);
>
> for(i=0; i<3; i++) {
> tgio_ctrl = inl(anactrl_io_base + 0xcc);
>
> @@ -180,9 +177,9 @@
>  //	wait 100us
>    delayx(1);
> dword = pci_read_config32(busnx,  (devnx+1)<<3 | 1, 0xe4);
> dword = pci_read_config32(bdf, 0xe4);
> dword &= ~(0x3f0); // enable
> pci_write_config32(busnx,  (devnx+1)<<3 | 1, 0xe4, dword);
> pci_write_config32(bdf, 0xe4, dword);
>
> //	need to wait 100ms
>    delayx(1000);
> @@ -403,8 +400,9 @@
>    	busnx = ht_c_index * HT_CHAIN_BUSN_D;
>    	for(devnx=0;devnx<0x20;devnx++) {
>    		u32 id;
> 	//			dev = PCI_BDF(busnx, devnx, 0);
> 	id = pci_read_config32(busnx, devnx<<3, PCI_VENDOR_ID);
> 	u32 bdf;
> 	bdf = PCI_BDF(busnx, devnx, 0);
> 	id = pci_read_config32(bdf, PCI_VENDOR_ID);
> if(id == 0x036910de) {
> 	busn[mcp55_num] = busnx;
> 	devn[mcp55_num] = devnx;
> Index: southbridge/nvidia/mcp55/stage1_usbdebug.c
>
> --- southbridge/nvidia/mcp55/stage1_usbdebug.c	(revision 0)
> +++ southbridge/nvidia/mcp55/stage1_usbdebug.c	(revision 0)
> @@ -0,0 +1,55 @@
> +/*
> 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
> */
>
> +/* This should probably be a config variable. */
> +#if HT_CHAIN_END_UNITID_BASE != 0x20
> #define MCP55_DEVN_BASE	HT_CHAIN_END_UNITID_BASE
>
> +#else
> #define MCP55_DEVN_BASE	HT_CHAIN_UNITID_BASE
>
> +#endif
>
> +#define EHCI_BAR_INDEX	0x10
> +#define EHCI_BAR	0xFEF00000
> +#define EHCI_DEBUG_OFFSET	0x98
>
> +#include "pci.h"
> +#include "stage1.h"
> +static void set_debug_port(unsigned port)
> +{
> u32 dword;
> u32 bdf = PCI_BDF(0, MCP55_DEVN_BASE+2, 1);
> dword = pci_read_config32(bdf, 0x74);
> dword &= ~(0xf<<12);
> dword |= (port<<12);
> pci_write_config32(bdf, 0x74, dword);
>
> +}
>
> +static void mcp55_enable_usbdebug_direct(unsigned port)
> +{
> u32 bdf = PCI_BDF(0, MCP55_DEVN_BASE+2, 1);
> set_debug_port(port);
> pci_write_config32(bdf, EHCI_BAR_INDEX, EHCI_BAR);
> pci_write_config8(bdf, 0x04, 0x2); // mem space enabe

Typo: enable
> +}

USB debug as a separate patch, please.
> Index: southbridge/nvidia/mcp55/stage1.h
>
> --- southbridge/nvidia/mcp55/stage1.h	(revision 0)
> +++ southbridge/nvidia/mcp55/stage1.h	(revision 0)
> @@ -0,0 +1,24 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Ronald G. Minnich
> 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
> */
>
> +#define pci_read_config32(bdf, where) pci_cf8_conf1.read32(bdf, where)
> +#define pci_write_config32(bdf, where, what) pci_cf8_conf1.write32(bdf, where, what)
> +#define pci_read_config16(bdf, where) pci_cf8_conf1.read32(bdf, where)
> +#define pci_write_config16(bdf, where, what) pci_cf8_conf1.write32(bdf, where, what)
> +#define pci_locate_device(vid, did, slot) pci_cf8_conf1.find(vid, did, slot)
> Index: southbridge/nvidia/mcp55/stage1_smbus.c
>
> --- southbridge/nvidia/mcp55/stage1_smbus.c	(revision 728)
> +++ southbridge/nvidia/mcp55/stage1_smbus.c	(working copy)
> @@ -5,6 +5,7 @@
> Written by Yinghai Lu  for Tyan Computer.
> Copyright (C) 2006,2007 AMD
> Written by Yinghai Lu  for AMD.
> Copyright (C) 2008 Ronald G. Minnich (for v3)
> 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
>
> @@ -26,10 +27,7 @@
>  #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 "stage1.h"
>  #include "mcp55_smbus.h"
>
> #define SMBUS0_IO_BASE	0x1000
> @@ -38,20 +36,17 @@
>
> static void enable_smbus(void)
>  {
> u32 dev;
> dev = pci_locate_device(PCI_ID(0x10de, 0x0368), 0);
>
> -#if 0
> if (dev == PCI_DEV_INVALID) {
> u32 bdf;
> if (!pci_locate_device(0x10de, 0x0368, &bdf)) {

In theory, the device model should allow us to call enable_smbus with
the right device as argument.
> die("SMBUS controller not found\r\n");
>
> }
> printk(BIOS_DEBUG, "SMBus controller enabled\r\n");
>
> -#endif
> printk(BIOS_DEBUG, "SMBus controller enabled\n");
> /* set smbus iobase */
> pci_write_config32(0, dev, 0x20, SMBUS0_IO_BASE | 1);
> pci_write_config32(0, dev, 0x24, SMBUS1_IO_BASE | 1);
> pci_write_config32(bdf, 0x20, SMBUS0_IO_BASE | 1);
> pci_write_config32(bdf, 0x24, SMBUS1_IO_BASE | 1);
> /* Set smbus iospace enable */
> pci_write_config16(0, dev, 0x4, 0x01);
> pci_write_config16(bdf, 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);

Regards,
Carl-Daniel