From owner-p4-projects@FreeBSD.ORG Tue Apr 10 22:32:50 2012 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id F109C1065676; Tue, 10 Apr 2012 22:32:49 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id B30F01065670; Tue, 10 Apr 2012 22:32:49 +0000 (UTC) (envelope-from jonwoodruff@gmail.com) Received: from mail-wi0-f178.google.com (mail-wi0-f178.google.com [209.85.212.178]) by mx1.freebsd.org (Postfix) with ESMTP id 120C78FC0A; Tue, 10 Apr 2012 22:32:48 +0000 (UTC) Received: by wibhq7 with SMTP id hq7so239338wib.13 for ; Tue, 10 Apr 2012 15:32:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=zeuIxLGvTS12qAJ/ZdpG/7q4hoQ4f5VqQ5SOsLuXsQc=; b=iSugYjO5QYgv7ho1fgNjyVEthphn1FT3SzlK3zOiSPyPCwsltHhexE3rt0BJHNTfG8 Zvd4mVYtE3dM1JsYSr/dXtxoOXyDsTlzjji+VpTlfViC7TskwRS0PBcq1eHe57e2ZlTZ Lx5SEUreU7JQrIBz5/QJSgFT5m2LuMt4eKKXIo3URdQmOzXKF3eeVC5R3FVVXtFchyv5 KWiqVZt2qmJbkxxN5KWJkUf7cOPiQ31BtpBoK3GukyVip9QezONO2OlAd2M46B54I4Tu ulZ2Y5lNfU+ZOwL8m5SajgwnYm9JgnksJJPj4Kui6MU0EZned/dAXL8neOz4IU4Waf1B b26g== MIME-Version: 1.0 Received: by 10.180.92.71 with SMTP id ck7mr10685012wib.21.1334097165663; Tue, 10 Apr 2012 15:32:45 -0700 (PDT) Received: by 10.216.85.10 with HTTP; Tue, 10 Apr 2012 15:32:45 -0700 (PDT) In-Reply-To: <201204102200.q3AM0KjU068586@skunkworks.freebsd.org> References: <201204102200.q3AM0KjU068586@skunkworks.freebsd.org> Date: Tue, 10 Apr 2012 23:32:45 +0100 Message-ID: From: Jonathan Woodruff To: Robert Watson Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.5 Cc: Perforce Change Reviews Subject: Re: PERFORCE change 209388 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Apr 2012 22:32:50 -0000 Yay! On Tue, Apr 10, 2012 at 11:00 PM, Robert Watson 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, > >