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 scott@notabs.org
Hi Scott,
On Fri, Jun 17, 2011 at 10:26 PM, Scott Duplichan scott@notabs.org wrote:
The attached patch allows the asrock e350m1 onboard nic to work.
- Update the asrock e350m1 devicetree.cb to match the hardware.
- 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 scott@notabs.org
- 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
Hi Scott,
Please send this patch and the others you might have in the pipeline to the new gerrit system for review.
Thanks, Cristi
Marc Jones wrote:
]Hi Scott, ] ]On Fri, Jun 17, 2011 at 10:26 PM, Scott Duplichan scott@notabs.org 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 scott@notabs.org ] ]- 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.
Thanks, Scott
On Mon, Jun 20, 2011 at 3:43 PM, Scott Duplichan scott@notabs.org wrote:
Marc Jones wrote:
]Hi Scott, ] ]On Fri, Jun 17, 2011 at 10:26 PM, Scott Duplichan scott@notabs.org 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 scott@notabs.org ] ]- 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
-
Cristian Măgherușan-Stanciu -
Marc Jones -
Scott Duplichan