Re: [coreboot] patch: stage 2 ide device for mcp55

[Carl-Daniel Hailfinger]

On 11.08.2008 06:42, ron minnich wrote:
> attached. This was pretty trivial. I expect to be able to do the next
> few devices and take at most 15 mins apiece for them.
>
> First I intend, since Peter keeps asking, to modify dtc so we can have
> ide.dtc and not just ide.
>
> This actually compiles and builds and the dtc works and ... well it's nice.
>
> ron
>
> Here is the first stage2 device. It was really easy to bring over.
>
> For Peter and others, the next thing I'll do is make it so dtc can
> use 'ide.dtc' and still get the name right.
>
> Stand by.
> For those of you who want to see how to bring other chips across, 
> watch this space: this is a tutorial of sorts.
>
> So far, it's going well. 
> Signed-off-by: Ronald G. Minnich

A few minor comments, feel free to commit anyway.
Acked-by: Carl-Daniel Hailfinger
> Index: southbridge/nvidia/mcp55/ide.c
>
> --- southbridge/nvidia/mcp55/ide.c	(revision 0)
> +++ southbridge/nvidia/mcp55/ide.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 <types.h>
> +#include <lib.h>
> +#include <console.h>
> +#include <device/pci.h>
> +#include <msr.h>
> +#include <legacy.h>
> +#include <device/pci_ids.h>
> +#include <statictree.h>
> +#include <config.h>
> +#include "mcp55.h"
>
> +static void ide_init(struct device *dev)
> +{
> struct southbridge_nvidia_mcp55_ide_config *conf =
>    (struct southbridge_nvidia_mcp55_ide_config *)dev->device_configuration;
> /* Enable ide devices so the linux ide driver will work */
> u32 dword;
> u16 word;
> u8 byte;
> word = pci_read_config16(dev, 0x50);
> /* Ensure prefetch is disabled */
> word &= ~((1 << 15) | (1 << 13));
> if (conf->ide1_enable) {
> /* Enable secondary ide interface */
> word |= (1<<0);
> printk(BIOS_DEBUG, "IDE1 \t");

Can you move the comment into the printk? Same value for readers of the
code, great improvement for the logs.
> }
> if (conf->ide0_enable) {
> /* Enable primary ide interface */
> word |= (1<<1);
> printk(BIOS_DEBUG, "IDE0\n");

Same here.
> }
> word |= (1<<12);
> word |= (1<<14);
> pci_write_config16(dev, 0x50, word);
> byte = 0x20 ; // Latency: 64-->32
> pci_write_config8(dev, 0xd, byte);
> dword = pci_read_config32(dev, 0xf8);
> dword |= 12;
> pci_write_config32(dev, 0xf8, dword);
>
> +#ifdef CONFIG_PCI_ROM_RUN
> pci_dev_init(dev);
>
> +#endif
>
> +#warning set subsystem id on mcp55 ide
> +#if 0
> pci_write_config32(dev, 0x40,
> ((device & 0xffff) << 16) | (vendor & 0xffff));
>
> +#endif

Maybe move the hunk above into an extra function so we can use the
generic subsystem ID infrastructure?
> +}
>
> +struct device_operations mcp55_ide = {
> .id = {.type = DEVICE_ID_PCI,
> {.pci = {.vendor = PCI_VENDOR_ID_NVIDIA,
> 	      .device = PCI_DEVICE_ID_NVIDIA_MCP55_IDE}}},
> .constructor		 = default_device_constructor,
> .phase3_scan		 = 0,
> .phase4_read_resources	 = pci_dev_read_resources,
> .phase4_set_resources	 = pci_dev_set_resources,
> .phase5_enable_resources = pci_dev_enable_resources,
> .phase6_init		 = ide_init,
> .ops_pci		 = &pci_dev_ops_pci,
>
> +};

Regards,
Carl-Daniel