| Submitter | Scott |
|---|---|
| Date | 2011-06-18 04:26:05 |
| Message ID | <7685E708643544B196304D179DC1CF87@asusp67> |
| Download | mbox | patch |
| Permalink | /patch/3161/ |
| State | New |
| Headers | show |
Comments
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
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
Patch
diff -d -u -r coreboot-c045b4c\src\mainboard\asrock\e350m1\devicetree.cb coreboot-pcie-fix\src\mainboard\asrock\e350m1\devicetree.cb --- coreboot-c045b4c\src\mainboard\asrock\e350m1\devicetree.cb Thu Jun 16 10:20:56 2011 +++ coreboot-pcie-fix\src\mainboard\asrock\e350m1\devicetree.cb Fri Jun 17 21:11:52 2011 @@ -99,11 +99,19 @@ end #LPC device pci 14.4 on end # PCI 0x4384 device pci 14.5 on end # USB 2 - device pci 15.0 off end # PCIe PortA - device pci 15.1 off end # PCIe PortB + device pci 15.0 on end # PCIe PortA + device pci 15.1 on end # PCIe PortB device pci 15.2 off end # PCIe PortC device pci 15.3 off end # PCIe PortD - register "gpp_configuration" = "0" #4:0:0:0 (really need to disable all 4 somehow) + + # gpp_configuration options + #0000: PortA lanes[3:0] + #0001: N/A + #0010: PortA lanes[1:0], PortB lanes[3:2] + #0011: PortA lanes[1:0], PortB lane2, PortC lane3 + #0100: PortA lane0, PortB lane1, PortC lane2, PortD lane3. + register "gpp_configuration" = "4" + register "boot_switch_sata_ide" = "0" # 0: boot from SATA. 1: IDE end #southbridge/amd/cimx_wrapper/sb800 # end # device pci 18.0 diff -d -u -r coreboot-c045b4c\src\southbridge\amd\cimx_wrapper\sb800\late.c coreboot-pcie-fix\src\southbridge\amd\cimx_wrapper\sb800\late.c --- coreboot-c045b4c\src\southbridge\amd\cimx_wrapper\sb800\late.c Thu Jun 16 10:20:56 2011 +++ coreboot-pcie-fix\src\southbridge\amd\cimx_wrapper\sb800\late.c Fri Jun 17 21:16:51 2011 @@ -413,16 +413,13 @@ break; case (0x15 << 3) | 0: /* 0:15:0 PCIe PortA */ - 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; + } /* * GPP_CFGMODE_X4000: PortA Lanes[3:0] @@ -430,22 +427,16 @@ * GPP_CFGMODE_X2110: PortA Lanes[1:0], PortB Lane2, PortC Lane3 * GPP_CFGMODE_X1111: PortA Lanes0, PortB Lane1, PortC Lane2, PortD Lane3 */ - if (sb_config->GppLinkConfig != sb_chip->gpp_configuration) { - sb_config->GppLinkConfig = sb_chip->gpp_configuration; - } - - sbPcieGppEarlyInit(sb_config); + sb_config->GppLinkConfig = sb_chip->gpp_configuration; + sb_config->StdHeader.Func = SB_BEFORE_PCI_INIT; + AmdSbDispatcher(sb_config); break; + } default: break; } - /* Special setting ABCFG registers before PCI emulation. */ - abSpecialSetBeforePciEnum(sb_config); - usbDesertPll(sb_config); - //sb_config->StdHeader.Func = SB_BEFORE_PCI_INIT; - //AmdSbDispatcher(sb_config); } struct chip_operations southbridge_amd_cimx_wrapper_sb800_ops = {