| Message ID | 20190322054523.29670-1-oohall@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hw/npu2: Merge reset functions | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (b392d785eb49630b9f00fef8d17944ed82b2c1fe) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | fail | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Fri, Mar 22, 2019 at 04:45:23PM +1100, Oliver O'Halloran wrote: > static int64_t npu2_freset(struct pci_slot *slot __unused) > { >- return OPAL_SUCCESS; >+ return npu2_reset("Freset"); > } IIRC, there actually is a reason npu2_freset() doesn't do anything. Alistair, do you recall?
Le 22/03/2019 à 22:15, Reza Arbab a écrit : > On Fri, Mar 22, 2019 at 04:45:23PM +1100, Oliver O'Halloran wrote: >> static int64_t npu2_freset(struct pci_slot *slot __unused) >> { >> - return OPAL_SUCCESS; >> + return npu2_reset("Freset"); >> } > > IIRC, there actually is a reason npu2_freset() doesn't do anything. > > Alistair, do you recall? > I'm certainly not trying to fill-in for Alistair ;-) but freset is called when the PCI slots are init when skiboot starts, typically to kick-in link training. Except for nvlink. So here we'd be resetting the NTL instead of doing nothing. Any chance it could have consequences? Fred
On Monday, 25 March 2019 11:12:49 AM AEDT Frederic Barrat wrote: > Le 22/03/2019 à 22:15, Reza Arbab a écrit : > > On Fri, Mar 22, 2019 at 04:45:23PM +1100, Oliver O'Halloran wrote: > >> static int64_t npu2_freset(struct pci_slot *slot __unused) > >> { > >> - return OPAL_SUCCESS; > >> + return npu2_reset("Freset"); > >> } > > > > IIRC, there actually is a reason npu2_freset() doesn't do anything. > > > > Alistair, do you recall? > > I'm certainly not trying to fill-in for Alistair ;-) but freset is > called when the PCI slots are init when skiboot starts, typically to > kick-in link training. Except for nvlink. So here we'd be resetting the > NTL instead of doing nothing. Any chance it could have consequences? I don't recall there being any reason why freset doesn't do anything other than it doesn't need to do anything. The NVLink resets were originally used for resetting the NVLink HW to a POR state so the driver could retrain links, etc. in the case of a GPU reset. For coherent memory we required some work arounds to fence the NVLink as well once the link was brought down. Calling this from freset() shouldn't cause any issues during skiboot start, although I am not sure what the argument for doing it is either as unlike PCIe (and perhaps CAPI?) we can't do nvlink training from Skiboot. - Alistair > Fred
diff --git a/hw/npu2.c b/hw/npu2.c index 4ecc91350e82..a630536ca418 100644 --- a/hw/npu2.c +++ b/hw/npu2.c @@ -1200,14 +1200,14 @@ static int64_t npu2_get_power_state(struct pci_slot *slot __unused, uint8_t *val return OPAL_SUCCESS; } -static int64_t npu2_hreset(struct pci_slot *slot __unused) +static int64_t npu2_reset(const char *reset_type) { struct npu2 *p; int i; struct npu2_dev *ndev; p = phb_to_npu2_nvlink(slot->phb); - NPU2INF(p, "Hreset PHB state\n"); + NPU2INF(p, "%s PHB state\n", reset_type); for (i = 0; i < p->total_devices; i++) { ndev = &p->devices[i]; @@ -1219,28 +1219,19 @@ static int64_t npu2_hreset(struct pci_slot *slot __unused) return purge_l2_l3_caches(); } +static int64_t npu2_hreset(struct pci_slot *slot __unused) +{ + return npu2_reset("Hreset"); +} + static int64_t npu2_freset(struct pci_slot *slot __unused) { - return OPAL_SUCCESS; + return npu2_reset("Freset"); } static int64_t npu2_creset(struct pci_slot *slot) { - struct npu2 *p; - int i; - struct npu2_dev *ndev; - - p = phb_to_npu2_nvlink(slot->phb); - NPU2INF(p, "Creset PHB state\n"); - - for (i = 0; i < p->total_devices; i++) { - ndev = &p->devices[i]; - if (ndev) { - NPU2DEVINF(ndev, "Resetting device\n"); - reset_ntl(ndev); - } - } - return OPAL_SUCCESS; + return npu2_reset("Creset"); } static struct pci_slot *npu2_slot_create(struct phb *phb)
Seems like we should be doing the same cache purge operation in the CRESET case, and we don't even implement FRESET. Cc: Reza Arbab <arbab@linux.ibm.com> Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- hw/npu2.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-)