On 12.08.2008 05:55, ron minnich wrote:
I can not say I really like this idea at all.
We have this ops_pci thing in the device, which I left in there probably by mistake in 2007.
And for what reason? so we can do one pci operation.
AFAICS the infrastructure is generic to be able to set HT subsystem IDs etc. No idea whether anything besides PCI has subsystem IDs.
And on top of that, we need to set up an additional set of ops vectors with only one member:
Do you want to move the vector directly into struct device? Will this really make stuff easier?
+void mcp55_pci_dev_set_subsystem(struct device *dev, unsigned int vendor,
unsigned int device)+{
pci_write_config32(dev, PCI_CB_SUBSYSTEM_VENDOR_ID,((device & 0xffff) << 16) | (vendor & 0xffff));+}
+/** MCP55 specific device operation for PCI devices. */ +struct pci_operations mcp55_pci_dev_ops_pci = {
.set_subsystem = mcp55_pci_dev_set_subsystem,+};
And what's that thing do anyway? Nothing special. It sets one register, the subsystem register, which most parts can handle in a generic way anyway.
We already have two incompatible subsystem setting functions in the tree. The generic PCI one does not work on MCP55 and vice versa.
And, we can do it in phase6 as I started to do -- all it requires is that we add subsystem vendor and device id to the generic device. Given that the generic device already has some things in it that are pci-centric, I think we can afford two more u16s :-)
OK, then do it in phase6.
Finally, we can do all this setting of the values of subsystem vendor and device id at compile time (or I thought we could) so why not do it then? Then, when we want to view the tree, we can see more information.
I still have trouble following the twisted maze called dtc.
This patch is just bringing back the v2 way of doing things, and I would really hope we can avoid that.
Can you take a look at my proposal to use the dtc to set this stuff and let me know if that might work instead? I really don't want to do this the v2 way.
If you really want this mcp55_pci_dev_set_subsystem, let's just put it in the generic device and kill the pci_ops *. It's simpler.
Hm. New iteration/compromise follows. It also addresses the comments you made in a private mail. My primary goal for this patch was to fix bugs and point out the places where it can be changed to fit your suggestions.
Regards, Carl-Daniel
Move mcp55 subsystem ID setting to the v3 model. This fixes a genuine bug in the MCP55 code.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/mcp55.h =================================================================== --- corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/mcp55.h (Revision 749) +++ corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/mcp55.h (Arbeitskopie) @@ -22,6 +22,10 @@ #ifndef MCP55_H #define MCP55_H
+#define PCI_MCP55_SUBSYSTEM_VENDOR_ID 0x40
+void mcp55_pci_dev_set_subsystem(struct device *dev, unsigned int vendor, + unsigned int device); +struct pci_operations mcp55_pci_dev_ops_pci;
#endif /* MCP55_H */ Index: corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/ide.c =================================================================== --- corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/ide.c (Revision 749) +++ corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/ide.c (Arbeitskopie) @@ -68,12 +68,6 @@ #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 }
struct device_operations mcp55_ide = { @@ -86,6 +80,6 @@ .phase4_set_resources = pci_dev_set_resources, .phase5_enable_resources = pci_dev_enable_resources, .phase6_init = ide_init, - .ops_pci = &pci_dev_ops_pci, + .ops_pci = &mcp55_pci_dev_ops_pci, };
Index: corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/usb2.c =================================================================== --- corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/usb2.c (Revision 749) +++ corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/usb2.c (Arbeitskopie) @@ -44,11 +44,6 @@ dword = pci_read_config32(dev, 0xf8); dword |= 40; pci_write_config32(dev, 0xf8, dword); -#warning mange set subsystem in mcp55 usb2 -#if 0 - pci_write_config32(dev, 0x40, - ((device & 0xffff) << 16) | (vendor & 0xffff)); -#endif }
static void usb2_set_resources(struct device *dev) @@ -84,5 +79,5 @@ .phase4_set_resources = usb2_set_resources, .phase5_enable_resources = pci_dev_enable_resources, .phase6_init = usb2_init, - .ops_pci = &pci_dev_ops_pci, + .ops_pci = &mcp55_pci_dev_ops_pci, }; Index: corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/sata.c =================================================================== --- corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/sata.c (Revision 749) +++ corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/sata.c (Arbeitskopie) @@ -67,13 +67,6 @@ dword = pci_read_config32(dev, 0xf8); dword |= 2; pci_write_config32(dev, 0xf8, dword); - - -#warning finish set subsystem in mcp55 sata -#if 0 - pci_write_config32(dev, 0x40, - ((device & 0xffff) << 16) | (vendor & 0xffff)); -#endif }
struct device_operations mcp55_sata = { @@ -86,5 +79,5 @@ .phase4_set_resources = pci_dev_set_resources, .phase5_enable_resources = pci_dev_enable_resources, .phase6_init = sata_init, - .ops_pci = &pci_dev_ops_pci, + .ops_pci = &mcp55_pci_dev_ops_pci, }; Index: corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/usb.c =================================================================== --- corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/usb.c (Revision 749) +++ corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/usb.c (Arbeitskopie) @@ -34,11 +34,6 @@
static void usb_init(struct device * dev) { -#warning handle subsystem set in mcp55 usb -#if 0 - pci_write_config32(dev, 0x40, - ((device & 0xffff) << 16) | (vendor & 0xffff)); -#endif }
struct device_operations mcp55_usb = { @@ -51,5 +46,5 @@ .phase4_set_resources = pci_dev_set_resources, .phase5_enable_resources = pci_dev_enable_resources, .phase6_init = usb_init, - .ops_pci = &pci_dev_ops_pci, + .ops_pci = &mcp55_pci_dev_ops_pci, }; Index: corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/mcp55.c =================================================================== --- corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/mcp55.c (Revision 749) +++ corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/mcp55.c (Arbeitskopie) @@ -248,6 +248,18 @@
}
+void mcp55_pci_dev_set_subsystem(struct device *dev, unsigned int vendor, + unsigned int device) +{ + pci_write_config32(dev, PCI_MCP55_SUBSYSTEM_VENDOR_ID, + ((device & 0xffff) << 16) | (vendor & 0xffff)); +} + +/** MCP55 specific device operation for PCI devices. */ +struct pci_operations mcp55_pci_dev_ops_pci = { + .set_subsystem = mcp55_pci_dev_set_subsystem, +}; + struct device_operations nvidia_ops = { .id = {.type = DEVICE_ID_PCI, {.pci = {.vendor = PCI_VENDOR_ID_NVIDIA, Index: corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/smbus.c =================================================================== --- corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/smbus.c (Revision 749) +++ corebootv3-mcp55_subsystem/southbridge/nvidia/mcp55/smbus.c (Arbeitskopie) @@ -127,24 +127,19 @@ if (res) pm_base = res->base; #endif -#warning finish subsystem set in mcp55 smbus -#if 0 - pci_write_config32(dev, 0x40, - ((device & 0xffff) << 16) | (vendor & 0xffff)); -#endif }
struct device_operations mcp55_smbus = { .id = {.type = DEVICE_ID_PCI, {.pci = {.vendor = PCI_VENDOR_ID_NVIDIA, .device = PCI_DEVICE_ID_NVIDIA_MCP55_SM2}}}, - .constructor = default_device_constructor, - .phase3_scan = scan_static_bus, + .constructor = default_device_constructor, + .phase3_scan = scan_static_bus, .phase4_read_resources = mcp55_sm_read_resources, .phase4_set_resources = pci_dev_set_resources, .phase5_enable_resources = pci_dev_enable_resources, - .phase6_init = mcp55_sm_init, - .ops_pci = &pci_dev_ops_pci, - .ops_smbus_bus = &lops_smbus_bus, + .phase6_init = mcp55_sm_init, + .ops_pci = &mcp55_pci_dev_ops_pci, + .ops_smbus_bus = &lops_smbus_bus, };
Index: corebootv3-mcp55_subsystem/device/pci_device.c =================================================================== --- corebootv3-mcp55_subsystem/device/pci_device.c (Revision 749) +++ corebootv3-mcp55_subsystem/device/pci_device.c (Arbeitskopie) @@ -620,26 +620,47 @@ printk(BIOS_SPEW, "Adding RAM resource (%lld bytes)\n", res->size); }
-void pci_dev_enable_resources(struct device *dev) +void pci_dev_set_subsystem_wrapper(struct device *dev) { const struct pci_operations *ops; - u16 command; + u16 vendor = 0; + u16 device = 0;
+#warning Per-device subsystem ID has to be set here, but for that we have to call this function with the matching dts struct. + +#ifdef HAVE_MAINBOARD_PCI_SUBSYSTEM_ID + /* If there's no explicit subsystem ID for this device and the device + * and the device is onboard, use the board defaults. */ + if (dev->on_mainboard) { + if (!vendor) + vendor = mainboard_pci_subsystem_vendor; + if (!device) + device = mainboard_pci_subsystem_device; + } +#endif /* Set the subsystem vendor and device ID for mainboard devices. */ ops = ops_pci(dev);
-#ifdef HAVE_MAINBOARD_PCI_SUBSYSTEM_ID - if (dev->on_mainboard && ops && ops->set_subsystem) { + /* If either vendor or device is zero, we leave it as is. */ + if (ops && ops->set_subsystem && vendor && device) { printk(BIOS_DEBUG, "%s: Setting subsystem VID/DID to %02x/%02x\n", - dev_path(dev), mainboard_pci_subsystem_vendor, - mainboard_pci_subsystem_device); + dev_path(dev), vendor, device);
- ops->set_subsystem(dev, mainboard_pci_subsystem_vendor, - mainboard_pci_subsystem_device); + ops->set_subsystem(dev, vendor, device); + } else { + printk(BIOS_DEBUG, "%s: Not setting subsystem VID/DID\n", + dev_path(dev)); } -#endif + +}
+void pci_dev_enable_resources(struct device *dev) +{ + u16 command; + + pci_dev_set_subsystem_wrapper(dev); + command = pci_read_config16(dev, PCI_COMMAND); command |= dev->command; command |= (PCI_COMMAND_PARITY + PCI_COMMAND_SERR); // Error check.