Note discussion in the patch.
Not that I want another long-running discussion which results in nothing being done!
Just "noting for the log"
ron
On 11.08.2008 07:37, ron minnich wrote:
Note discussion in the patch.
Not that I want another long-running discussion which results in nothing being done!
Just "noting for the log"
ron
pcie device for mcp55.
This compiles.
Note this is not as convenient in one way as v2. In v2, due to the use of linker sets, we simply get all 6 devices just by compiling.
In v3, in the mainboard dts, we will need a separate dts node for each device, viz: pci@0,a{}; pci@0,b{};
etc.
This strongly argues for Uwe's request to put multiple device dts nodes in one file. I just don't know how to do that yet; suggestions welcome.
How about multi-level dts instead of the current two-level model? An intermediate level could aggregate device-level dts. The mainboard dts would include intermediate-level dts unless low-level stuff was needed.
Note that it *is* clearer from the point of view of seeing exactly what is being built in in one place: the dts. In v2, the magic of linker scripts added considerable confusion, and we got complaints about that, so we are trying to avoid such magic in v3.
Point taken.
The overall question of which is better? We'll see.
Note that in an earlier version of v3, we had this same capabilty (albeit done without linker sets), and again we removed it as people preferred to reduce the magic.
Could you dig up the revision? I'd like to take a look. Thanks.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
With the comments below addressed: Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: southbridge/nvidia/mcp55/pcie.c
--- southbridge/nvidia/mcp55/pcie.c (revision 0) +++ southbridge/nvidia/mcp55/pcie.c (revision 0) @@ -0,0 +1,117 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2004 Tyan Computer
- Written by Yinghai Lu yhlu@tyan.com for Tyan Computer.
- Copyright (C) 2006,2007 AMD
- Written by Yinghai Lu yinghai.lu@amd.com 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 pcie_init(struct device *dev) +{
- /* Enable pci error detecting */
- u32 dword;
- /* System error enable */
- dword = pci_read_config32(dev, 0x04);
- dword |= (1<<8); /* System error enable */
- dword |= (1<<30); /* Clear possible errors */
- pci_write_config32(dev, 0x04, dword);
+}
+struct device_operations mcp55_pcia = {
Please change the name to mcp55_pcie_a. Especially a few lines below, where you have a mcp55_pcie, the naming gets really confusing really fast.
- .id = {.type = DEVICE_ID_PCI,
{.pci = {.vendor = PCI_VENDOR_ID_NVIDIA,.device = PCI_DEVICE_ID_NVIDIA_MCP55_PCIE_A}}},- .constructor = default_device_constructor,
- .reset_bus = pci_bus_reset,
- .phase3_scan = pci_scan_bridge,
- .phase4_read_resources = pci_dev_read_resources,
- .phase4_set_resources = pci_dev_set_resources,
- .phase5_enable_resources = pci_bus_enable_resources,
- .phase6_init = pcie_init,
- .ops_pci = &pci_dev_ops_pci,
+};
+struct device_operations mcp55_pcib_c = {
mcp55_pcie_bc
- .id = {.type = DEVICE_ID_PCI,
{.pci = {.vendor = PCI_VENDOR_ID_NVIDIA,.device = PCI_DEVICE_ID_NVIDIA_MCP55_PCIE_B_C}}},- .constructor = default_device_constructor,
- .reset_bus = pci_bus_reset,
- .phase3_scan = pci_scan_bridge,
- .phase4_read_resources = pci_dev_read_resources,
- .phase4_set_resources = pci_dev_set_resources,
- .phase5_enable_resources = pci_bus_enable_resources,
- .phase6_init = pcie_init,
- .ops_pci = &pci_dev_ops_pci,
+};
+struct device_operations mcp55_pcid = {
mcp55_pcie_d
- .id = {.type = DEVICE_ID_PCI,
{.pci = {.vendor = PCI_VENDOR_ID_NVIDIA,.device = PCI_DEVICE_ID_NVIDIA_MCP55_PCIE_D}}},- .constructor = default_device_constructor,
- .reset_bus = pci_bus_reset,
- .phase3_scan = pci_scan_bridge,
- .phase4_read_resources = pci_dev_read_resources,
- .phase4_set_resources = pci_dev_set_resources,
- .phase5_enable_resources = pci_bus_enable_resources,
- .phase6_init = pcie_init,
- .ops_pci = &pci_dev_ops_pci,
+};
+struct device_operations mcp55_pcie = {
mcp55_pcie_e. Right now the name looks like it is the only PCIe device operations. Bad.
- .id = {.type = DEVICE_ID_PCI,
{.pci = {.vendor = PCI_VENDOR_ID_NVIDIA,.device = PCI_DEVICE_ID_NVIDIA_MCP55_PCIE_D}}},- .constructor = default_device_constructor,
- .reset_bus = pci_bus_reset,
- .phase3_scan = pci_scan_bridge,
- .phase4_read_resources = pci_dev_read_resources,
- .phase4_set_resources = pci_dev_set_resources,
- .phase5_enable_resources = pci_bus_enable_resources,
- .phase6_init = pcie_init,
- .ops_pci = &pci_dev_ops_pci,
+};
+struct device_operations mcp55_pcif = {
mcp55_pcie_f
- .id = {.type = DEVICE_ID_PCI,
{.pci = {.vendor = PCI_VENDOR_ID_NVIDIA,.device = PCI_DEVICE_ID_NVIDIA_MCP55_PCIE_F}}},- .constructor = default_device_constructor,
- .reset_bus = pci_bus_reset,
- .phase3_scan = pci_scan_bridge,
- .phase4_read_resources = pci_dev_read_resources,
- .phase4_set_resources = pci_dev_set_resources,
- .phase5_enable_resources = pci_bus_enable_resources,
- .phase6_init = pcie_init,
- .ops_pci = &pci_dev_ops_pci,
+};
Regards, Carl-Daniel
Committed revision 743.
comments:
On Mon, Aug 11, 2008 at 3:53 AM, Carl-Daniel Hailfinger
How about multi-level dts instead of the current two-level model? An intermediate level could aggregate device-level dts. The mainboard dts would include intermediate-level dts unless low-level stuff was needed.
Horrors. Let's not go there right now. This has already been the subject of multi-month discussions. Keep it simple and regular.
Note that it *is* clearer from the point of view of seeing exactly what is being built in in one place: the dts. In v2, the magic of linker scripts added considerable confusion, and we got complaints about that, so we are trying to avoid such magic in v3.
Point taken.
Let's never forget: we need to think about it from the point of view of non-coreboot-experts, i.e. people who are using the tool, and might have to change it, but don't wnat to make a living at it.
That should be our target: people who don't do this for a living and don't want to -- they just want a quick build and an easy set of changes. One issue that came up on OLPC was that a number of people found v2 hard to figure out, and they did not have time to plumb its depths. I don't blame them one bit.
I think u-boot does a great job that way. You can walk into that code and find your way around. It's not nearly as capable as v2 but i can tell you -- there are some big PPC machines running u-boot, not coreboot, and the choice was made by people who looked at both systems.
I think v2 veered off the path to some degree, in recent years, as more and more complexity was added in via cpp tricks, linker sets and complex code paths. In some sense, I have been trying to reduce trickiness and increase transparency with v3. I hope it works out.
I have taken a lot of inspiration from how Plan 9 kernel build process works and plan 9 init and setup works; it's far simpler than (e.g.) Linux.
Could you dig up the revision? I'd like to take a look. Thanks.
I don't think I can :-) I did that in the very early going at FOSDEM 2007 and we killed it about a year later IIRC. It was documented in early versions of the newboot.lyx document.
With the comments below addressed:
renamed per your suggestions.
ron
On 11.08.2008 18:29, ron minnich wrote:
Committed revision 743.
comments:
On Mon, Aug 11, 2008 at 3:53 AM, Carl-Daniel Hailfinger
How about multi-level dts instead of the current two-level model? An intermediate level could aggregate device-level dts. The mainboard dts would include intermediate-level dts unless low-level stuff was needed.
Horrors. Let's not go there right now. This has already been the subject of multi-month discussions. Keep it simple and regular.
I missed those discussions. It seems I should be happy I missed them.
Note that it *is* clearer from the point of view of seeing exactly what is being built in in one place: the dts. In v2, the magic of linker scripts added considerable confusion, and we got complaints about that, so we are trying to avoid such magic in v3.
Point taken.
Let's never forget: we need to think about it from the point of view of non-coreboot-experts, i.e. people who are using the tool, and might have to change it, but don't wnat to make a living at it.
If anything, they are most likely to use automated tools (to be written) for dts creation. But the important thing is that people should not be forced to understand magic multilevel include stuff.
That should be our target: people who don't do this for a living and don't want to -- they just want a quick build and an easy set of changes. One issue that came up on OLPC was that a number of people found v2 hard to figure out, and they did not have time to plumb its depths. I don't blame them one bit.
I think u-boot does a great job that way. You can walk into that code and find your way around. It's not nearly as capable as v2 but i can tell you -- there are some big PPC machines running u-boot, not coreboot, and the choice was made by people who looked at both systems.
I have not looked at U-Boot code in depth, but coreboot v3 code was pretty readable and the structure was intelligible some weeks ago. I strive to keep it that way.
I think v2 veered off the path to some degree, in recent years, as more and more complexity was added in via cpp tricks, linker sets and complex code paths. In some sense, I have been trying to reduce trickiness and increase transparency with v3. I hope it works out.
I'll keep an eye on it as well.
Could you dig up the revision? I'd like to take a look. Thanks.
I don't think I can :-) I did that in the very early going at FOSDEM 2007 and we killed it about a year later IIRC. It was documented in early versions of the newboot.lyx document.
I'll read through that once I have time.
With the comments below addressed:
renamed per your suggestions.
Thanks.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
ron minnich