[coreboot] [PATCH] asrock e350m1: configure sb800 gpp ports to support onboard pcie nic

[Marc Jones]

On Mon, Jun 20, 2011 at 3:43 PM, Scott Duplichan <scott at notabs.org> wrote:
> Marc Jones wrote:
>
> ]Hi Scott,
> ]
> ]On Fri, Jun 17, 2011 at 10:26 PM, Scott Duplichan <scott at 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 at 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




-- 
http://se-eng.com