| Message ID | 1372139717-14885-4-git-send-email-shangw@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 2013-06-25 at 13:55 +0800, Gavin Shan wrote: > * don't touch the other command bits > */ > - eeh_ops->read_config(dn, PCI_COMMAND, 4, &cmd); > - if (edev->config_space[1] & PCI_COMMAND_PARITY) > - cmd |= PCI_COMMAND_PARITY; > - else > - cmd &= ~PCI_COMMAND_PARITY; > - if (edev->config_space[1] & PCI_COMMAND_SERR) > - cmd |= PCI_COMMAND_SERR; > - else > - cmd &= ~PCI_COMMAND_SERR; > - eeh_ops->write_config(dn, PCI_COMMAND, 4, cmd); > + if (pdev) { > + eeh_ops->write_config(dn, PCI_COMMAND, 4, > + edev->config_space[1]); > + } else { That needs a much better comment. Why are you doing that instead of what's below ? In fact there is more to restore in a bridge right ? (windows etc...). Do you do that ? Should we just have a different function to restore a device vs. a bridge ? I also don't see a need to do thing differently between phyp and powernv. Bridges inside partitions would suffer the same fate in both cases. Ben. > + eeh_ops->read_config(dn, PCI_COMMAND, 4, &cmd); > + if (edev->config_space[1] & PCI_COMMAND_PARITY) > + cmd |= PCI_COMMAND_PARITY; > + else > + cmd &= ~PCI_COMMAND_PARITY; > + if (edev->config_space[1] & PCI_COMMAND_SERR) > + cmd |= PCI_COMMAND_SERR; > + else > + cmd &= ~PCI_COMMAND_SERR; > + eeh_ops->write_config(dn, PCI_COMMAND, 4, cmd); > + } > +
On Tue, Jun 25, 2013 at 04:06:24PM +1000, Benjamin Herrenschmidt wrote: >On Tue, 2013-06-25 at 13:55 +0800, Gavin Shan wrote: >> * don't touch the other command bits >> */ >> - eeh_ops->read_config(dn, PCI_COMMAND, 4, &cmd); >> - if (edev->config_space[1] & PCI_COMMAND_PARITY) >> - cmd |= PCI_COMMAND_PARITY; >> - else >> - cmd &= ~PCI_COMMAND_PARITY; >> - if (edev->config_space[1] & PCI_COMMAND_SERR) >> - cmd |= PCI_COMMAND_SERR; >> - else >> - cmd &= ~PCI_COMMAND_SERR; >> - eeh_ops->write_config(dn, PCI_COMMAND, 4, cmd); >> + if (pdev) { >> + eeh_ops->write_config(dn, PCI_COMMAND, 4, >> + edev->config_space[1]); >> + } else { > >That needs a much better comment. Why are you doing that instead >of what's below ? In fact there is more to restore in a bridge >right ? (windows etc...). Do you do that ? Should we just have a >different function to restore a device vs. a bridge ? > Yeah, We should have one separate function to do that for bridge. I'll do that in next revision. >I also don't see a need to do thing differently between phyp and >powernv. Bridges inside partitions would suffer the same fate in >both cases. > If we just have complete reset for fenced PHB, we need restore it from the cache (edev->config_space[1]) instead of reading that from hardware. Fenced PHB is the special case on PowerNV :-) Thanks, Gavin
On Tue, 2013-06-25 at 15:47 +0800, Gavin Shan wrote: > If we just have complete reset for fenced PHB, we need restore it > from the cache (edev->config_space[1]) instead of reading that from > hardware. Fenced PHB is the special case on PowerNV :-) Well not really... In general we can also end up doing a hard reset under pHyp, and bridges can lose their state as well, which means they need to be restored from cache. We don't see the real PHB, but we might see the bridges if we have a PE that contains a bridge, for example, a PCIe card with a switch on it. If we hard reset that (because the driver requested it) or if pHyp did a reset due to a fence behind the scene, that bridge *will* have lost its state and will need to be reconfigured too... or is RTAS doing it all ? Cheers, Ben.
On Tue, Jun 25, 2013 at 05:57:44PM +1000, Benjamin Herrenschmidt wrote: >On Tue, 2013-06-25 at 15:47 +0800, Gavin Shan wrote: >> If we just have complete reset for fenced PHB, we need restore it >> from the cache (edev->config_space[1]) instead of reading that from >> hardware. Fenced PHB is the special case on PowerNV :-) > >Well not really... > >In general we can also end up doing a hard reset under pHyp, and bridges >can lose their state as well, which means they need to be restored from >cache. > >We don't see the real PHB, but we might see the bridges if we have a PE >that contains a bridge, for example, a PCIe card with a switch on it. > >If we hard reset that (because the driver requested it) or if pHyp did a >reset due to a fence behind the scene, that bridge *will* have lost its >state and will need to be reconfigured too... or is RTAS doing it all ? > Ok. So that would be job of eeh_ops->configure_bridge(). On pSeries, it should have done with that. Thanks, Gavin
diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c index 55943fc..db83ada 100644 --- a/arch/powerpc/kernel/eeh_pe.c +++ b/arch/powerpc/kernel/eeh_pe.c @@ -22,6 +22,7 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ +#include <linux/delay.h> #include <linux/export.h> #include <linux/gfp.h> #include <linux/init.h> @@ -567,6 +568,88 @@ void eeh_pe_state_clear(struct eeh_pe *pe, int state) eeh_pe_traverse(pe, __eeh_pe_state_clear, &state); } +/* + * Some PCI bridges (e.g. PLX bridges) have primary/secondary + * buses assigned explicitly by firmware, and we probably have + * lost that after reset. So we have to delay the check until + * the PCI-CFG registers have been restored for the parent + * bridge. + * + * Don't use normal PCI-CFG accessors, which probably has been + * blocked on normal path during the stage. So we need utilize + * eeh operations, which is always permitted. + */ +static void eeh_bridge_check_link(struct device_node *dn, + struct pci_dev *pdev) +{ + int cap; + uint32_t val; + int timeout = 0; + + /* + * We only check root port and downstream ports of + * PCIe switches + */ + if (!pci_is_pcie(pdev) || + (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT && + pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM)) + return; + + pr_debug("%s: Check PCIe link for %s ...\n", + __func__, pci_name(pdev)); + + /* Check slot status */ + cap = pdev->pcie_cap; + eeh_ops->read_config(dn, cap + PCI_EXP_SLTSTA, 2, &val); + if (!(val & PCI_EXP_SLTSTA_PDS)) { + pr_debug(" No card in the slot (0x%04x) !\n", val); + return; + } + + /* Check power status if we have the capability */ + eeh_ops->read_config(dn, cap + PCI_EXP_SLTCAP, 2, &val); + if (val & PCI_EXP_SLTCAP_PCP) { + eeh_ops->read_config(dn, cap + PCI_EXP_SLTCTL, 2, &val); + if (val & PCI_EXP_SLTCTL_PCC) { + pr_debug(" In power-off state, power it on ...\n"); + val &= ~(PCI_EXP_SLTCTL_PCC | PCI_EXP_SLTCTL_PIC); + val |= (0x0100 & PCI_EXP_SLTCTL_PIC); + eeh_ops->write_config(dn, cap + PCI_EXP_SLTCTL, 2, val); + msleep(2 * 1000); + } + } + + /* Enable link */ + eeh_ops->read_config(dn, cap + PCI_EXP_LNKCTL, 2, &val); + val &= ~PCI_EXP_LNKCTL_LD; + eeh_ops->write_config(dn, cap + PCI_EXP_LNKCTL, 2, val); + + /* Check link */ + eeh_ops->read_config(dn, cap + PCI_EXP_LNKCAP, 4, &val); + if (!(val & PCI_EXP_LNKCAP_DLLLARC)) { + pr_debug(" No link reporting capability (0x%08x) \n", val); + msleep(1000); + return; + } + + /* Wait the link is up until timeout (5s) */ + timeout = 0; + while (timeout < 5000) { + msleep(20); + timeout += 20; + + eeh_ops->read_config(dn, cap + PCI_EXP_LNKSTA, 2, &val); + if (val & PCI_EXP_LNKSTA_DLLLA) + break; + } + + if (val & PCI_EXP_LNKSTA_DLLLA) + pr_debug(" Link up (%s)\n", + (val & PCI_EXP_LNKSTA_CLS_2_5GB) ? "2.5GB" : "5GB"); + else + pr_debug(" Link not ready (0x%04x)\n", val); +} + /** * eeh_restore_one_device_bars - Restore the Base Address Registers for one device * @data: EEH device @@ -580,9 +663,17 @@ static void *eeh_restore_one_device_bars(void *data, void *flag) { int i; u32 cmd; + struct pci_dev *pdev = NULL; struct eeh_dev *edev = (struct eeh_dev *)data; struct device_node *dn = eeh_dev_to_of_node(edev); + /* Trace the PCI bridge */ + if (eeh_probe_mode_dev()) { + pdev = eeh_dev_to_pci_dev(edev); + if (pdev->hdr_type != PCI_HEADER_TYPE_BRIDGE) + pdev = NULL; + } + for (i = 4; i < 10; i++) eeh_ops->write_config(dn, i*4, 4, edev->config_space[i]); /* 12 == Expansion ROM Address */ @@ -603,16 +694,25 @@ static void *eeh_restore_one_device_bars(void *data, void *flag) * Restore PERR & SERR bits, some devices require it, * don't touch the other command bits */ - eeh_ops->read_config(dn, PCI_COMMAND, 4, &cmd); - if (edev->config_space[1] & PCI_COMMAND_PARITY) - cmd |= PCI_COMMAND_PARITY; - else - cmd &= ~PCI_COMMAND_PARITY; - if (edev->config_space[1] & PCI_COMMAND_SERR) - cmd |= PCI_COMMAND_SERR; - else - cmd &= ~PCI_COMMAND_SERR; - eeh_ops->write_config(dn, PCI_COMMAND, 4, cmd); + if (pdev) { + eeh_ops->write_config(dn, PCI_COMMAND, 4, + edev->config_space[1]); + } else { + eeh_ops->read_config(dn, PCI_COMMAND, 4, &cmd); + if (edev->config_space[1] & PCI_COMMAND_PARITY) + cmd |= PCI_COMMAND_PARITY; + else + cmd &= ~PCI_COMMAND_PARITY; + if (edev->config_space[1] & PCI_COMMAND_SERR) + cmd |= PCI_COMMAND_SERR; + else + cmd &= ~PCI_COMMAND_SERR; + eeh_ops->write_config(dn, PCI_COMMAND, 4, cmd); + } + + /* Check the PCIe link for bridge */ + if (pdev) + eeh_bridge_check_link(dn, pdev); return NULL; }
After reset (e.g. complete reset) in order to bring the fenced PHB back, the PCIe link might not be ready yet. The patch intends to make sure the PCIe link is ready before accessing its subordinate PCI devices. The patch also fixes that wrong values restored to PCI_COMMAND register for PCI bridges. Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com> --- arch/powerpc/kernel/eeh_pe.c | 120 ++++++++++++++++++++++++++++++++++++++---- 1 files changed, 110 insertions(+), 10 deletions(-)