On Mon, Jun 20, 2011 at 3:43 PM, Scott Duplichan <[email protected]> wrote: > Marc Jones wrote: > > ]Hi Scott, > ] > ]On Fri, Jun 17, 2011 at 10:26 PM, Scott Duplichan <[email protected]> wrote: > ]> The attached patch allows the asrock e350m1 onboard nic to work. > ]> 1) Update the asrock e350m1 devicetree.cb to match the hardware. > ]> 2) Change the way the sb800 cimx wrapper code works. The original > ]> cimx code calls sb800 cimx function sbBeforePciInit() once. When > ]> ported to coreboot, the gpp component of this function was called > ]> once for each gpp port, as the gpp port's enable/disable state > ]> became known. A 05/15/2011 change makes the early gpp code run > ]> only once, triggered by processing the 4th gpp port. This method > ]> is not general enough because the 4th gpp port is not enabled on > ]> all boards. With the current change, the early gpp code runs when > ]> the first gpp port is processed. If any gpp ports are enabled, the > ]> first must be enabled. Tested with Win7 and linux on asrock e350m1. > ]> This change will also affect amd inagua, and has not been tested > ]> on that board. > ]> > ]> Signed-off-by: Scott Duplichan <[email protected]> > ] > ]- sb_config->PORTCONFIG[0].PortCfg.PortPresent = dev->enabled; > ]- return; > ]- case (0x15 << 3) | 1: /* 0:15:1 PCIe PortB */ > ]- sb_config->PORTCONFIG[1].PortCfg.PortPresent = dev->enabled; > ]- return; > ]- case (0x15 << 3) | 2: /* 0:15:2 PCIe PortC */ > ]- sb_config->PORTCONFIG[2].PortCfg.PortPresent = dev->enabled; > ]- return; > ]- case (0x15 << 3) | 3: /* 0:15:3 PCIe PortD */ > ]- sb_config->PORTCONFIG[3].PortCfg.PortPresent = dev->enabled; > ]+ { > ]+ device_t device; > ]+ for (device = dev; device; device = device->next) { > ]+ if (dev->path.type != DEVICE_PATH_PCI) continue; > ]+ if ((device->path.pci.devfn & ~7) != > PCI_DEVFN(0x15,0)) ]break; > ]+ sb_config->PORTCONFIG[device->path.pci.devfn & > ]3].PortCfg.PortPresent = device->enabled; > ]+ } > ] > ]The allocator is going to loop through and call this function for each > ]device in the devicetree.cb. Is there a reason to change this to a > ]loop here? It looks like the real fix is moving the SB_BEFORE_PCI_INIT > ]call to the last device, and to not run for each device. Did I miss > ]something more subtle in this patch? > ] > ]Marc > > Hello Marc, > > I wanted to call the standard cimx entry point only once, the way it is done > in the reference BIOS. This is difficult to do with coreboot, because all > the portPresent values need to be known before making the call. All of the > dev->enable values are not available in parallel from coreboot. The previous > code captured the dev->enable values for functions 0, 1, and 2 in the local > sb_config struct, then returned. When function 3 was processed all 4 values > were ready. I think that is similar to what you suggest. The difference is > that the prevoius code called only the sbPcieGppEarlyInit part of > SB_BEFORE_PCI_INIT. > > I think your suggestion works for boards that use the sb800 option for 4 x1 > ports, as asrock e350m1 does. But what about a board that uses one of the > other sb800 pcie options such as single x4 port? My concern was that > functions 1, 2, and 3 might not even be visible. Even if they are visible, > the person writing devicetree might not choose to include them since they > are not present in a booted system. Because function 0 is visible in all > cases, I thought that is the best place to trigger the cimx call. Because > function 0 is processed first, getting the dev->enable values is not so > easy. The new code scans the devicetree.cb nodes following device 0x15 > function 0. Scanning stops on the first pci node not for device 0x15, so > the loop will run a maximum of 3 times. That lets the code gather the needed > dev->enable values.
Hi Scott, I see what you were thinking about the visible ports. I was thinking that the devicetree.db would have the IDs, they would just be turned off. I agree that this way works too and may be more clear for a developer. i added Kerry to see if he has an opinion. Marc -- http://se-eng.com -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
