[Skiboot] [PATCH v2] pci: Avoid hot reset after post-fundamental reset

[Gavin Shan]

On Wed, Oct 12, 2016 at 10:19:40AM +1100, Russell Currey wrote:
>On Wed, 2016-10-12 at 10:04 +1100, Gavin Shan wrote:
>> On Tue, Oct 11, 2016 at 04:04:07PM +1100, Russell Currey wrote:
>> > 
>> > In the PCI post-fundamental reset code, a hot reset is performed at the
>> > end.  This is causing issues at boot time as a reset signal is being sent
>> > downstream before the links are up, which is causing issues on adapters
>> > behind switches.  No errors result in skiboot, but the adapters are not
>> > usable in Linux as a result.
>> > 
>> > This patch fixes some adapters not being configurable in Linux on some
>> > systems.  The issue was not present in skiboot 5.2.x.
>> > 
>> > Cc: stable # 5.3.x
>> > Signed-off-by: Russell Currey <ruscur at russell.cc>
>> 
>> It seems one pointed missed from last comments: firenze_pci_slot_freset()
>> need similar thing to skip hot reset. With this fixed:
>
>I had a look at this and I'm not sure it's the same.  In the case of
>firenze_pci_slot_freset(), it only goes to hreset if pfreset isn't defined, and
>I don't know if that would be problematic.  In the case that pfreset isn't
>defined, would you suggest doing nothing at the end of firenze_pci_slot_freset()
>instead?
>

The platform backend only handles normal PCI slot (not PHB3 slots).
We don't have pfreset for PCI normal slots. What you need is to
replace below code. firenze_pci_slot_perst(), which relies on
PLX switch internal registers to issue freset, needs similiar
thing.

Without the changes, we could issue a hot reset after a power
cycle on the target slot and it can prevent some adapters from
being run and running. So we need issue the requested reset
type (fundamental vs hot) and the request is usually coming
from EEH subsystem (arch/powerpc/kernel/eeh.c::eeh_set_dev_freset()).

#define FIRENZE_PCI_SLOT_HRESET                 0x00000200
#define   FIRENZE_PCI_SLOT_HRESET_START         0x00000201

                /* PHB3 slot supports post fundamental reset, we switch
                 * to that. For normal PCI slot, we switch to hot reset
                 * instead.
                 */
                if (slot->ops.pfreset) {
                        prlog(PR_DEBUG, "%016llx FRESET: Switch to PFRESET\n",
                              slot->id);
                        pci_slot_set_state(slot,
                                FIRENZE_PCI_SLOT_PFRESET_START);
                        return slot->ops.pfreset(slot);
                } else if (slot->ops.hreset) {
                        prlog(PR_DEBUG, "%016llx FRESET: Switch to HRESET\n",
                              slot->id);
                        pci_slot_set_state(slot,
                                FIRENZE_PCI_SLOT_HRESET_START);
                        return slot->ops.hreset(slot);
                }

                pci_slot_set_state(slot, FIRENZE_PCI_SLOT_NORMAL);

With:

#define FIRENZE_PCI_SLOT_LINK		0x00000100
#define   FIRENZE_PCI_SLOT_LINK_START	0x00000101

		pci_slot_set_state(slot, FIRENZE_PCI_SLOT_LINK_START);
		return slot->ops.poll_link(slot);


On top of your patch, I will post followup patch to remove pfreset() as
it becomes useless.

Thanks,
Gavin

>> > 
>> > ---
>> > hw/p7ioc-phb.c | 4 ++--
>> > hw/phb3.c      | 4 ++--
>> > hw/phb4.c      | 8 --------
>> > 3 files changed, 4 insertions(+), 12 deletions(-)
>> > 
>> > diff --git a/hw/p7ioc-phb.c b/hw/p7ioc-phb.c
>> > index d2a18a3..1f1b362 100644
>> > --- a/hw/p7ioc-phb.c
>> > +++ b/hw/p7ioc-phb.c
>> > @@ -2234,8 +2234,8 @@ static int64_t p7ioc_freset(struct pci_slot *slot)
>> > 			return slot->ops.pfreset(slot);
>> > 		}
>> > 
>> > -		pci_slot_set_state(slot, P7IOC_SLOT_HRESET_START);
>> > -		return slot->ops.hreset(slot);
>> > +		pci_slot_set_state(slot, P7IOC_SLOT_LINK_START);
>> > +		return slot->ops.poll_link(slot);
>> > 	default:
>> > 		PHBERR(p, "FRESET: Unexpected slot state %08x\n",
>> > 		       slot->state);
>> > diff --git a/hw/phb3.c b/hw/phb3.c
>> > index d0b5010..817137b 100644
>> > --- a/hw/phb3.c
>> > +++ b/hw/phb3.c
>> > @@ -2240,8 +2240,8 @@ static int64_t phb3_pfreset(struct pci_slot *slot)
>> > 		/* CAPP FPGA requires 1s to flash before polling link */
>> > 		return pci_slot_set_sm_timeout(slot, secs_to_tb(1));
>> > 	case PHB3_SLOT_PFRESET_DEASSERT_DELAY:
>> > -		pci_slot_set_state(slot, PHB3_SLOT_HRESET_START);
>> > -		return slot->ops.hreset(slot);
>> > +		pci_slot_set_state(slot, PHB3_SLOT_LINK_START);
>> > +		return slot->ops.poll_link(slot);
>> > 	default:
>> > 		PHBERR(p, "Unexpected slot state %08x\n", slot->state);
>> > 	}
>> > diff --git a/hw/phb4.c b/hw/phb4.c
>> > index 385ce8c..efb6a5f 100644
>> > --- a/hw/phb4.c
>> > +++ b/hw/phb4.c
>> > @@ -1951,16 +1951,8 @@ static int64_t phb4_pfreset(struct pci_slot *slot)
>> > 		/* CAPP FPGA requires 1s to flash before polling link */
>> > 		return pci_slot_set_sm_timeout(slot, secs_to_tb(1));
>> > 	case PHB4_SLOT_PFRESET_DEASSERT_DELAY:
>> > -#if 0 	/* PHB3 does a Hreset here. It's unnecessary I think and it's
>> > -	 * causing problems with the simulator croc model so don't do
>> > -	 * it until I figure out Gavin's reasons
>> > -	 */
>> > -		pci_slot_set_state(slot, PHB4_SLOT_HRESET_START);
>> > -		return slot->ops.hreset(slot);
>> > -#else
>> > 		pci_slot_set_state(slot, PHB4_SLOT_LINK_START);
>> > 		return slot->ops.poll_link(slot);
>> > -#endif
>> > 	default:
>> > 		PHBERR(p, "Unexpected slot state %08x\n", slot->state);
>> > 	}
>> > -- 
>> > 2.10.0
>> > 
>> > _______________________________________________
>> > Skiboot mailing list
>> > Skiboot at lists.ozlabs.org
>> > https://lists.ozlabs.org/listinfo/skiboot
>