On Mon, Aug 14, 2017 at 04:26:10PM +1000, Jonathan Matthew wrote:
> On 14/08/17 01:40, Christian Weisgerber wrote:
> > Jonathan Matthew:
> >
> > > Better version that actually preserves the port command register state
> > > across
> > > resets, rather than throwing it away and replacing it with garbage:
> >
> > This appears to break ahci on the OverDrive 1000:
> >
> > ...
> > ahci0 at simplebus0: AHCI 1.3
> > scsibus0 at ahci0: 32 targets
> > ahci0: stopping the port, softreset slot 31 was still active.
> > ahci0: failed to reset port during timeout handling, disabling it
> > ...
> > bootfile: sd0a:/bsd
> > boot device: lookup sd0a:/bsd failed
> > root device:
> >
> > (There's still a long pause after the "ahci0 at simplebus0" line, too.)
>
> The problem here is that the portreset process is reentrant in some cases,
> and when the first portreset finishes, it sets the command register to 0
> since the second one cleared ap->ap_saved_cmd when it finished. Not
> clearing ap_saved_cmd makes it work again, but that's a bit nasty. I'll see
> if I can figure out how to untangle this so it doesn't have to be reentrant.
> I'm sure this has caused problems before.
Easier than I thought. The updated diff below works on the overdrive 1000
and everything else I can test at the moment. Port multipliers still work,
even.
If I don't hear of any more breakage, I'll commit this over the weekend.
> The delay there is happening because one of the port multiplier probe
> commands is timing out. I'm not sure whose fault this is yet.
It looks like the dwc ahci implementation isn't behaving properly here.
We run the probe twice for reasons I can't quite remember, but I don't see that
in any other ahci drivers, so maybe I can fix it in ours, which would halve the
time we spend on it.
diff --git sys/dev/ic/ahci.c sys/dev/ic/ahci.c
index 9057d44ff78..1b398596706 100644
--- sys/dev/ic/ahci.c
+++ sys/dev/ic/ahci.c
@@ -72,6 +72,7 @@ void ahci_enable_interrupts(struct ahci_port
*);
int ahci_init(struct ahci_softc *);
int ahci_port_alloc(struct ahci_softc *, u_int);
+void ahci_port_detect(struct ahci_softc *, u_int);
void ahci_port_free(struct ahci_softc *, u_int);
int ahci_port_init(struct ahci_softc *, u_int);
@@ -80,6 +81,10 @@ int ahci_port_stop(struct ahci_port *, int);
int ahci_port_clo(struct ahci_port *);
int ahci_port_softreset(struct ahci_port *);
int ahci_port_portreset(struct ahci_port *, int);
+void ahci_port_portreset_start(struct ahci_port *);
+int ahci_port_portreset_poll(struct ahci_port *);
+void ahci_port_portreset_wait(struct ahci_port *);
+int ahci_port_portreset_finish(struct ahci_port *, int);
int ahci_port_signature(struct ahci_port *);
int ahci_pmp_port_softreset(struct ahci_port *, int);
int ahci_pmp_port_portreset(struct ahci_port *, int);
@@ -173,7 +178,7 @@ ahci_attach(struct ahci_softc *sc)
{
struct atascsi_attach_args aaa;
u_int32_t pi;
- int i;
+ int i, j, done;
if (sc->sc_port_start == NULL)
sc->sc_port_start = ahci_default_port_start;
@@ -268,6 +273,37 @@ noccc:
if (ahci_port_alloc(sc, i) == ENOMEM)
goto freeports;
+
+ if (sc->sc_ports[i] != NULL)
+ ahci_port_portreset_start(sc->sc_ports[i]);
+ }
+
+ /*
+ * Poll for device detection until all ports report a device, or one
+ * second has elapsed.
+ */
+ for (i = 0; i < 1000; i++) {
+ done = 1;
+ for (j = 0; j < AHCI_MAX_PORTS; j++) {
+ if (sc->sc_ports[j] == NULL)
+ continue;
+
+ if (ahci_port_portreset_poll(sc->sc_ports[j]))
+ done = 0;
+ }
+
+ if (done)
+ break;
+
+ delay(1000);
+ }
+
+ /*
+ * Finish device detection on all ports that initialized.
+ */
+ for (i = 0; i < AHCI_MAX_PORTS; i++) {
+ if (sc->sc_ports[i] != NULL)
+ ahci_port_detect(sc, i);
}
memset(&aaa, 0, sizeof(aaa));
@@ -446,7 +482,6 @@ ahci_port_alloc(struct ahci_softc *sc, u_int port)
u_int32_t cmd;
struct ahci_cmd_hdr *hdr;
struct ahci_cmd_table *table;
- const char *speed;
int i, rc = ENOMEM;
ap = malloc(sizeof(*ap), M_DEVBUF, M_NOWAIT | M_ZERO);
@@ -594,10 +629,25 @@ nomem:
/* Wait for ICC change to complete */
ahci_pwait_clr(ap, AHCI_PREG_CMD, AHCI_PREG_CMD_ICC, 1);
+ rc = 0;
- /* Reset port */
- rc = ahci_port_portreset(ap, 1);
+freeport:
+ if (rc != 0)
+ ahci_port_free(sc, port);
+reterr:
+ return (rc);
+}
+
+void
+ahci_port_detect(struct ahci_softc *sc, u_int port)
+{
+ struct ahci_port *ap;
+ const char *speed;
+ int rc;
+
+ ap = sc->sc_ports[port];
+ rc = ahci_port_portreset_finish(ap, 1);
switch (rc) {
case ENODEV:
switch (ahci_pread(ap, AHCI_PREG_SSTS) & AHCI_PREG_SSTS_DET) {
@@ -667,12 +717,9 @@ nomem:
ahci_write(sc, AHCI_REG_IS, 1 << port);
ahci_enable_interrupts(ap);
-
freeport:
if (rc != 0)
ahci_port_free(sc, port);
-reterr:
- return (rc);
}
void
@@ -1372,25 +1419,12 @@ err:
}
/* AHCI port reset, Section 10.4.2 */
-int
-ahci_port_portreset(struct ahci_port *ap, int pmp)
-{
- u_int32_t cmd, r;
- int rc, s, retries = 0;
-
- s = splbio();
- DPRINTF(AHCI_D_VERBOSE, "%s: port reset\n", PORTNAME(ap));
- /* Save previous command register state */
- cmd = ahci_pread(ap, AHCI_PREG_CMD) & ~AHCI_PREG_CMD_ICC;
-
- /* Clear ST, ignoring failure */
- ahci_port_stop(ap, 0);
+void
+ahci_port_comreset(struct ahci_port *ap)
+{
+ u_int32_t r;
- /* Perform device detection */
- ahci_pwrite(ap, AHCI_PREG_SCTL, 0);
-retry:
- delay(10000);
r = AHCI_PREG_SCTL_IPM_DISABLED | AHCI_PREG_SCTL_DET_INIT;
if ((ap->ap_sc->sc_dev.dv_cfdata->cf_flags & 0x01) != 0) {
DPRINTF(AHCI_D_VERBOSE, "%s: forcing GEN1\n", PORTNAME(ap));
@@ -1403,10 +1437,58 @@ retry:
r |= AHCI_PREG_SCTL_DET_NONE;
ahci_pwrite(ap, AHCI_PREG_SCTL, r);
delay(10000);
+}
+
+void
+ahci_port_portreset_start(struct ahci_port *ap)
+{
+ int s;
+
+ s = splbio();
+ DPRINTF(AHCI_D_VERBOSE, "%s: port reset\n", PORTNAME(ap));
+
+ /* Save previous command register state */
+ ap->ap_saved_cmd = ahci_pread(ap, AHCI_PREG_CMD) & ~AHCI_PREG_CMD_ICC;
+
+ /* Clear ST, ignoring failure */
+ ahci_port_stop(ap, 0);
+
+ /* Perform device detection */
+ ahci_pwrite(ap, AHCI_PREG_SCTL, 0);
+ delay(10000);
+ ahci_port_comreset(ap);
+ splx(s);
+}
+
+int
+ahci_port_portreset_poll(struct ahci_port *ap)
+{
+ if ((ahci_pread(ap, AHCI_PREG_SSTS) & AHCI_PREG_SSTS_DET) !=
+ AHCI_PREG_SSTS_DET_DEV)
+ return (EAGAIN);
+ return (0);
+}
+
+void
+ahci_port_portreset_wait(struct ahci_port *ap)
+{
+ int i;
+
+ for (i = 0; i < 1000; i++) {
+ if (ahci_port_portreset_poll(ap) == 0)
+ break;
+ delay(1000);
+ }
+}
- /* Wait for device to be detected and communications established */
- if (ahci_pwait_eq(ap, AHCI_PREG_SSTS, AHCI_PREG_SSTS_DET,
- AHCI_PREG_SSTS_DET_DEV, 1)) {
+int
+ahci_port_portreset_finish(struct ahci_port *ap, int pmp)
+{
+ int rc, s, retries = 0;
+
+ s = splbio();
+retry:
+ if (ahci_port_portreset_poll(ap)) {
rc = ENODEV;
if (ahci_pread(ap, AHCI_PREG_SSTS) & AHCI_PREG_SSTS_DET) {
/* this may be a port multiplier with no device
@@ -1427,6 +1509,8 @@ retry:
*/
if (retries == 0) {
retries = 1;
+ ahci_port_comreset(ap);
+ ahci_port_portreset_wait(ap);
goto retry;
}
rc = EBUSY;
@@ -1436,20 +1520,34 @@ retry:
}
if (pmp != 0) {
- if (ahci_port_detect_pmp(ap) != 0) {
- rc = EBUSY;
+ if (ahci_port_detect_pmp(ap)) {
+ /* reset again without pmp support */
+ pmp = 0;
+ retries = 0;
+ ahci_port_comreset(ap);
+ ahci_port_portreset_wait(ap);
+ goto retry;
}
}
err:
/* Restore preserved port state */
- ahci_pwrite(ap, AHCI_PREG_CMD, cmd);
+ ahci_pwrite(ap, AHCI_PREG_CMD, ap->ap_saved_cmd);
+ ap->ap_saved_cmd = 0;
splx(s);
return (rc);
}
int
+ahci_port_portreset(struct ahci_port *ap, int pmp)
+{
+ ahci_port_portreset_start(ap);
+ ahci_port_portreset_wait(ap);
+ return (ahci_port_portreset_finish(ap, pmp));
+}
+
+int
ahci_port_detect_pmp(struct ahci_port *ap)
{
int count, pmp_rc, rc;
@@ -1642,7 +1740,7 @@ ahci_port_detect_pmp(struct ahci_port *ap)
ahci_enable_interrupts(ap);
- ahci_port_portreset(ap, 0);
+ rc = pmp_rc;
}
return (rc);
diff --git sys/dev/ic/ahcivar.h sys/dev/ic/ahcivar.h
index dee05adef22..27c866a496b 100644
--- sys/dev/ic/ahcivar.h
+++ sys/dev/ic/ahcivar.h
@@ -97,6 +97,7 @@ struct ahci_port {
u_int32_t ap_err_saved_sactive;
u_int32_t ap_err_saved_active;
u_int32_t ap_err_saved_active_cnt;
+ u_int32_t ap_saved_cmd;
u_int8_t *ap_err_scratch;
