Date: Tue, 10 Apr 2012 23:32:45 +0100 From: Jonathan Woodruff <jonwoodruff@gmail.com> To: Robert Watson <rwatson@freebsd.org> Cc: Perforce Change Reviews <perforce@freebsd.org> Subject: Re: PERFORCE change 209388 for review Message-ID: <CA%2BoXuiKdqcWFCFNmqjDsqUnZKq2y3=LL5ymzG3abQUq=FaExWg@mail.gmail.com> In-Reply-To: <201204102200.q3AM0KjU068586@skunkworks.freebsd.org> References: <201204102200.q3AM0KjU068586@skunkworks.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Yay! On Tue, Apr 10, 2012 at 11:00 PM, Robert Watson <rwatson@freebsd.org> wrote: > http://p4web.freebsd.org/@@209388?ac=10 > > Change 209388 by rwatson@rwatson_svr_ctsrd_mipsbuild on 2012/04/10 > 22:00:07 > > Modify the Altera SD Card IP core driver to ignore the presence of > ALTERA_SDCARD_RR1_COMMANDCRCFAILED in the RR1 register when a read > error is reported by the core. In my (limited) tests, the error > bit does not reflect data corruption in the I/O. With this change > I am able to mount and use UFS file systems reliably on 2GB CSD > structure 0 SD cards from FreeBSD/BERI. > > Affected files ... > > .. > //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard.h#8 > edit > .. > //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard_io.c#10 > edit > > Differences ... > > ==== > //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard.h#8 > (text+ko) ==== > > @@ -115,7 +115,6 @@ > */ > uint16_t altera_sdcard_read_asr(struct altera_sdcard_softc *sc); > int altera_sdcard_read_csd(struct altera_sdcard_softc *sc); > -uint16_t altera_sdcard_read_rr1(struct altera_sdcard_softc *sc); > > int altera_sdcard_io_complete(struct altera_sdcard_softc *sc, > uint16_t asr); > @@ -185,14 +184,28 @@ > #define ALTERA_SDCARD_ASR_CMDDATAERROR 0x0020 > > /* > - * The Altera IP Core provides a 16-bit "Response R1" regster (RR1) beyond > - * those described in the SD Card specification that holds additional > - * information on the most recently completed command sent to the unit. > + * The Altera IP Core claims to provide a 16-bit "Response R1" register > (RR1) > + * to provide more detailed error reporting when a read or write fails. > * > * XXXRW: The specification claims that this field is 16-bit, but then > - * proceeds to define values as though it is 32-bit. Which piece of the > - * documentation is correct? > + * proceeds to define values as though it is 32-bit. In practice, 16-bit > + * seems more likely as the register is not 32-bit aligned. > + */ > +#define ALTERA_SDCARD_RR1_INITPROCRUNNING 0x0100 > +#define ALTERA_SDCARD_RR1_ERASEINTERRUPTED 0x0200 > +#define ALTERA_SDCARD_RR1_ILLEGALCOMMAND 0x0400 > +#define ALTERA_SDCARD_RR1_COMMANDCRCFAILED 0x0800 > +#define ALTERA_SDCARD_RR1_ADDRESSMISALIGNED 0x1000 > +#define ALTERA_SDCARD_RR1_ADDRBLOCKRANGE 0x2000 > + > +/* > + * Not all RR1 values are "errors" per se -- check only for the ones that > are > + * when performing error handling. > */ > +#define ALTERA_SDCARD_RR1_ERRORMASK > \ > + (ALTERA_SDCARD_RR1_ERASEINTERRUPTED | > ALTERA_SDCARD_RR1_ILLEGALCOMMAND | \ > + ALTERA_SDCARD_RR1_COMMANDCRCFAILED | > ALTERA_SDCARD_RR1_ADDRESSMISALIGNED |\ > + ALTERA_SDCARD_RR1_ADDRBLOCKRANGE) > > /* > * Although SD Cards may have various sector sizes, the Altera IP Core > > ==== > //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard_io.c#10 > (text+ko) ==== > > @@ -188,7 +188,7 @@ > * register, but all bits it identifies are >16 bit. Most likely, RR1 is a > * 32-bit register? > */ > -uint16_t > +static uint16_t > altera_sdcard_read_rr1(struct altera_sdcard_softc *sc) > { > > @@ -287,7 +287,7 @@ > altera_sdcard_io_complete(struct altera_sdcard_softc *sc, uint16_t asr) > { > struct bio *bp; > - uint16_t rr1; > + uint16_t rr1, mask; > int error; > > ALTERA_SDCARD_LOCK_ASSERT(sc); > @@ -298,12 +298,42 @@ > > bp = sc->as_currentbio; > > - /* > - * Handle I/O retries if an error is returned by the device. > + /*- > + * Handle I/O retries if an error is returned by the device. > Various > + * quirks handled in the process: > + * > + * 1. ALTERA_SDCARD_ASR_CMDDATAERROR is ignored for BIO_WRITE. > + * 2. ALTERA_SDCARD_RR1_COMMANDCRCFAILED is ignored for BIO_READ. > */ > - if ((asr & ALTERA_SDCARD_ASR_CMDTIMEOUT) || (bp->bio_cmd == > BIO_READ > - && (asr & ALTERA_SDCARD_ASR_CMDDATAERROR))) { > - rr1 = altera_sdcard_read_rr1(sc); > + error = 0; > + rr1 = altera_sdcard_read_rr1(sc); > + switch (bp->bio_cmd) { > + case BIO_READ: > + mask = ALTERA_SDCARD_RR1_ERRORMASK; > + mask &= ~ALTERA_SDCARD_RR1_COMMANDCRCFAILED; > + if (asr & ALTERA_SDCARD_ASR_CMDTIMEOUT) > + error = EIO; > + else if ((asr & ALTERA_SDCARD_ASR_CMDDATAERROR) && > + (rr1 & mask)) > + error = EIO; > + else > + error = 0; > + break; > + > + case BIO_WRITE: > + if (asr & ALTERA_SDCARD_ASR_CMDTIMEOUT) > + error = EIO; > + else > + error = 0; > + break; > + > + default: > + break; > + } > + if (error) { > + /* > + * This attempt experienced an error; possibly retry. > + */ > sc->as_retriesleft--; > if (sc->as_retriesleft != 0) { > sc->as_flags |= ALTERA_SDCARD_FLAG_IOERROR; > @@ -316,8 +346,10 @@ > (bp->bio_cmd == BIO_WRITE ? "BIO_WRITE" : "unknown"), > bp->bio_pblkno, bp->bio_bcount, asr, rr1); > sc->as_flags &= ~ALTERA_SDCARD_FLAG_IOERROR; > - error = EIO; > } else { > + /* > + * Successful I/O completion path. > + */ > if (sc->as_flags & ALTERA_SDCARD_FLAG_IOERROR) { > device_printf(sc->as_dev, "%s: %s operation block > %ju" > " length %ju succeeded after %d retries\n", > @@ -327,10 +359,6 @@ > ALTERA_SDCARD_RETRY_LIMIT - sc->as_retriesleft); > sc->as_flags &= ~ALTERA_SDCARD_FLAG_IOERROR; > } > - > - /* > - * Successful I/O completion path. > - */ > switch (bp->bio_cmd) { > case BIO_READ: > altera_sdcard_read_rxtx_buffer(sc, bp->bio_data, > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CA%2BoXuiKdqcWFCFNmqjDsqUnZKq2y3=LL5ymzG3abQUq=FaExWg>