PERFORCE change 209055 for review
Robert Watson
rwatson at FreeBSD.org
Wed Apr 4 10:14:22 UTC 2012
http://p4web.freebsd.org/@@209055?ac=10
Change 209055 by rwatson at rwatson_svr_ctsrd_mipsbuild on 2012/04/04 10:13:58
Improve convergence of Altera SD Card IP Core with hardware as it
becomes more accessible, as well as fixing a number of driver bugs
and issues:
- Properly initialise softc condition variable; destroy lock and
condition variable on device detach (detach path unexercised as
yet).
- Have the driver name controllers "altera_sdcardc%d" so that they
don't collide with disk instances, which are "altera_sdcard%d".
It would be interesting to try this with multiple controllers to
make sure this works as desired.
- Introduce a new state, ALTERA_SDCARD_STATE_BADCARD to replace
the softc flag ALTERA_SDCARD_FLAG_BADCARD. We now have a number
of reasons we might transition a controller into the bad card
state, not least unsupported CSD versions or unlikely disk sizes
(e.g., all zeroes). This eliminates a number of XXX comments
elsewhere in the driver about how to handle bad cards.
- Extract and cache the CSD structure version in the softc.
Restructure CSD processing code so that it can (a) return a
failure and (b) in principle (although not yet in practice)
handle multiple CSD versions more smoothly. Announce card
insert, as well as card probe failures, more vocally.
- Fix logic bugs in the handling of CSD fields -- I misread the
SD Card physical layer specification for how certain fields are
composed to calculate the actual disk size.
Affected files ...
.. //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard.c#3 edit
.. //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard.h#5 edit
.. //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard_disk.c#3 edit
.. //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard_io.c#5 edit
.. //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard_nexus.c#2 edit
Differences ...
==== //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard.c#3 (text+ko) ====
@@ -76,14 +76,15 @@
{
ALTERA_SDCARD_LOCK_INIT(sc);
+ ALTERA_SDCARD_CONDVAR_INIT(sc);
sc->as_disk = NULL;
bioq_init(&sc->as_bioq);
sc->as_currentbio = NULL;
sc->as_state = ALTERA_SDCARD_STATE_NOCARD;
- sc->as_taskqueue = taskqueue_create("altera_sdcard taskq", M_WAITOK,
+ sc->as_taskqueue = taskqueue_create("altera_sdcardc taskq", M_WAITOK,
taskqueue_thread_enqueue, &sc->as_taskqueue);
taskqueue_start_threads(&sc->as_taskqueue, 1, PI_DISK,
- "altera_sdcard%d taskqueue", sc->as_unit);
+ "altera_sdcardc%d taskqueue", sc->as_unit);
TIMEOUT_TASK_INIT(sc->as_taskqueue, &sc->as_task, 0,
altera_sdcard_task, sc);
@@ -135,6 +136,8 @@
*/
taskqueue_free(sc->as_taskqueue);
sc->as_taskqueue = NULL;
+ ALTERA_SDCARD_CONDVAR_DESTROY(sc);
+ ALTERA_SDCARD_LOCK_DESTROY(sc);
}
/*
@@ -180,15 +183,19 @@
return;
/*
- * Read the CSD -- it may contain values that force the setting of
- * ALTERA_SDCARD_FLAG_BADCARD, in which case we suppress detection of
- * the card. For example, if there is an unrecognised CSD structure
- * version, or if the IP Core doesn't support the returned sector
- * size.
+ * Read the CSD -- it may contain values that the driver can't handle,
+ * either because of an unsupported version/feature, or because the
+ * card is misbehaving. This triggers a transition to
+ * ALTERA_SDCARD_STATE_BADCARD. We rely on the CSD read to print a
+ * banner about how the card is problematic, since it has more
+ * information. The bad card state allows us to print that banner
+ * once rather than each time we notice the card is there, and still
+ * bad.
*/
- altera_sdcard_read_csd(sc);
- if (sc->as_flags & ALTERA_SDCARD_FLAG_BADCARD)
+ if (altera_sdcard_read_csd(sc) != 0) {
+ sc->as_state = ALTERA_SDCARD_STATE_BADCARD;
return;
+ }
/*
* Process card insertion and upgrade to the IDLE state.
@@ -198,6 +205,28 @@
}
static void
+altera_sdcard_task_badcard(struct altera_sdcard_softc *sc)
+{
+
+ ALTERA_SDCARD_LOCK_ASSERT(sc);
+
+ /*
+ * Handle device driver detach.
+ */
+ if (sc->as_flags & ALTERA_SDCARD_FLAG_DETACHREQ) {
+ sc->as_state = ALTERA_SDCARD_STATE_DETACHED;
+ return;
+ }
+
+ /*
+ * Handle safe card removal -- no teardown is required, just a state
+ * transition.
+ */
+ if (!(altera_sdcard_read_asr(sc) & ALTERA_SDCARD_ASR_CARDPRESENT))
+ sc->as_state = ALTERA_SDCARD_STATE_NOCARD;
+}
+
+static void
altera_sdcard_task_idle(struct altera_sdcard_softc *sc)
{
@@ -213,9 +242,6 @@
/*
* Handle safe card removal.
- *
- * XXXRW: In the future, we may want to handle the run-time setting of
- * ALTERA_SDCARD_FLAG_BADCARD.
*/
if (!(altera_sdcard_read_asr(sc) & ALTERA_SDCARD_ASR_CARDPRESENT)) {
altera_sdcard_disk_remove(sc);
@@ -235,9 +261,6 @@
/*
* Check for unexpected card removal during an I/O.
- *
- * XXXRW: In the future, we may want to handle the run-time setting of
- * ALTERA_SDCARD_FLAG_BADCARD.
*/
if (!(asr & ALTERA_SDCARD_ASR_CARDPRESENT)) {
altera_sdcard_disk_remove(sc);
@@ -285,10 +308,11 @@
/*
* Reschedule based on new state. Or not, if detaching the device
- * driver.
+ * driver. Treat a bad card as though it were no card at all.
*/
switch (sc->as_state) {
case ALTERA_SDCARD_STATE_NOCARD:
+ case ALTERA_SDCARD_STATE_BADCARD:
interval = ALTERA_SDCARD_TIMEOUT_NOCARD;
break;
@@ -328,6 +352,10 @@
altera_sdcard_task_nocard(sc);
break;
+ case ALTERA_SDCARD_STATE_BADCARD:
+ altera_sdcard_task_badcard(sc);
+ break;
+
case ALTERA_SDCARD_STATE_IDLE:
altera_sdcard_task_idle(sc);
break;
==== //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard.h#5 (text+ko) ====
@@ -55,6 +55,7 @@
* Infrequently changing fields cached from the SD Card IP Core.
*/
struct altera_sdcard_csd as_csd;
+ uint8_t as_csd_structure; /* CSD version. */
uint64_t as_mediasize;
};
@@ -66,7 +67,8 @@
#define ALTERA_SDCARD_UNLOCK(sc) mtx_unlock(&(sc)->as_lock)
#define ALTERA_SDCARD_CONDVAR_DESTROY(sc) cv_destroy(&(sc)->as_condvar)
-#define ALTERA_SDCARD_CONDVAR_INIT(sc) cv_init(&(sc)->as_condvar)
+#define ALTERA_SDCARD_CONDVAR_INIT(sc) cv_init(&(sc)->as_condvar, \
+ "altera_sdcard_detach_wait")
#define ALTERA_SDCARD_CONDVAR_SIGNAL(dc) cv_signal(&(sc)->as_condvar)
#define ALTERA_SDCARD_CONDVAR_WAIT(sc) cv_wait(&(sc)->as_condvar, \
&(sc)->as_lock)
@@ -75,9 +77,10 @@
* States an instance can be in at any given moment.
*/
#define ALTERA_SDCARD_STATE_NOCARD 1 /* No card inserted. */
-#define ALTERA_SDCARD_STATE_IDLE 2 /* Card present but idle. */
-#define ALTERA_SDCARD_STATE_IO 3 /* Card in I/O currently. */
-#define ALTERA_SDCARD_STATE_DETACHED 4 /* Driver is detaching. */
+#define ALTERA_SDCARD_STATE_BADCARD 2 /* Card bad/not supported. */
+#define ALTERA_SDCARD_STATE_IDLE 3 /* Card present but idle. */
+#define ALTERA_SDCARD_STATE_IO 4 /* Card in I/O currently. */
+#define ALTERA_SDCARD_STATE_DETACHED 5 /* Driver is detaching. */
/*
* Different timeout intervals based on state. When just looking for a card
@@ -92,7 +95,6 @@
* Driver status flags.
*/
#define ALTERA_SDCARD_FLAG_DETACHREQ 0x00000001 /* Detach requested. */
-#define ALTERA_SDCARD_FLAG_BADCARD 0x00000002 /* Unsupported card. */
/*
* Functions for performing low-level register and memory I/O to/from the SD
@@ -100,7 +102,7 @@
* hardware interface.
*/
uint16_t altera_sdcard_read_asr(struct altera_sdcard_softc *sc);
-void altera_sdcard_read_csd(struct altera_sdcard_softc *sc);
+int altera_sdcard_read_csd(struct altera_sdcard_softc *sc);
void altera_sdcard_io_complete(struct altera_sdcard_softc *sc,
uint16_t asr);
==== //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard_disk.c#3 (text+ko) ====
@@ -86,6 +86,12 @@
biofinish(bp, NULL, ENXIO);
break;
+ case ALTERA_SDCARD_STATE_BADCARD:
+ device_printf(sc->as_dev, "%s: unexpected I/O on BADCARD",
+ __func__);
+ biofinish(bp, NULL, ENXIO);
+ break;
+
case ALTERA_SDCARD_STATE_DETACHED:
device_printf(sc->as_dev, "%s: unexpected I/O on DETACHED",
__func__);
@@ -110,6 +116,8 @@
altera_sdcard_disk_insert(struct altera_sdcard_softc *sc)
{
struct disk *disk;
+ uint64_t size;
+ char scale;
ALTERA_SDCARD_LOCK_ASSERT(sc);
@@ -135,6 +143,24 @@
disk->d_maxsize = ALTERA_SDCARD_SECTORSIZE;
sc->as_disk = disk;
disk_create(disk, DISK_VERSION);
+
+ /*
+ * Print a pretty-ish card insertion string. We could stand to
+ * decorate this further, e.g., with card vendor information.
+ */
+ size = sc->as_mediasize;
+ if (size > 1024*1024*1024) {
+ scale = 'G';
+ size /= 1024*1024*1024;
+ } else if (size > 1024*1024) {
+ scale = 'M';
+ size /= 1024*1024;
+ } else {
+ scale = 'K';
+ size /= 1024;
+ }
+ device_printf(sc->as_dev, "%ju%c SD Card inserted\n", (uintmax_t)size,
+ scale);
}
void
==== //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard_io.c#5 (text+ko) ====
@@ -68,44 +68,14 @@
return (le16toh(bus_read_2(sc->as_res, ALTERA_SDCARD_OFF_ASR)));
}
-void
-altera_sdcard_read_csd(struct altera_sdcard_softc *sc)
+static int
+altera_sdcard_process_csd0(struct altera_sdcard_softc *sc)
{
uint64_t c_size, c_size_mult, read_bl_len;
- uint8_t csd_structure, byte0, byte1, byte2;
+ uint8_t byte0, byte1, byte2;
ALTERA_SDCARD_LOCK_ASSERT(sc);
- /*
- * XXXRW: Assume for now that when the SD Card IP Core negotiates
- * voltage/speed/etc, it must use the CSD register, and therefore
- * populates the SD Card IP Core's cache of the register value. This
- * means that we can read it without issuing further SD Card commands.
- * If this assumption proves false, we will (a) get back garbage and
- * (b) need to add additional states in the driver state machine in
- * order to query card properties before I/O can start.
- *
- * XXXRW: Treating this as an array of bytes, so no byte swapping --
- * is that a safe assumption?
- */
- bus_read_region_1(sc->as_res, ALTERA_SDCARD_OFF_CSD,
- sc->as_csd.csd_data, sizeof(sc->as_csd));
-
- /*
- * Interpret the loaded CSD, extracting certain fields and copying
- * them into the softc for easy software access.
- *
- * Currently, we support only CSD Version 1.0. If we detect a newer
- * version, suppress card detection.
- */
- csd_structure = sc->as_csd.csd_data[ALTERA_SDCARD_CSD_STRUCTURE_BYTE];
- csd_structure &= ALTERA_SDCARD_CSD_STRUCTURE_MASK;
- csd_structure >>= ALTERA_SDCARD_CSD_STRUCTURE_RSHIFT;
- if (csd_structure != 0) {
- sc->as_flags |= ALTERA_SDCARD_FLAG_BADCARD;
- return;
- }
-
/*-
* Compute card capacity per SD Card interface description as follows:
*
@@ -114,8 +84,8 @@
* Where:
*
* BLOCKNR = (C_SIZE + 1) * MULT
- * MULT = C_SIZE_MULT << 8
- * BLOCK_LEN = READ_BL_LEN << 12
+ * MULT = 2^(C_SIZE_MULT+2)
+ * BLOCK_LEN = 2^READ_BL_LEN
*/
read_bl_len = sc->as_csd.csd_data[ALTERA_SDCARD_CSD_READ_BL_LEN_BYTE];
read_bl_len &= ALTERA_SDCARD_CSD_READ_BL_LEN_MASK;
@@ -144,12 +114,73 @@
(byte1 << ALTERA_SDCARD_CSD_C_SIZE_MULT_LSHIFT1);
/*
- * If we're just getting back zero's, mark the card as bad.
+ * If we're just getting back zero's, mark the card as bad, even
+ * though it could just mean a Very Small Disk Indeed.
+ */
+ if (c_size == 0 && c_size_mult == 0 && read_bl_len == 0) {
+ device_printf(sc->as_dev, "Ignored zero-size card\n");
+ return (ENXIO);
+ }
+ sc->as_mediasize = (c_size + 1) * (1 << (c_size_mult + 2)) *
+ (1 << read_bl_len);
+ return (0);
+}
+
+int
+altera_sdcard_read_csd(struct altera_sdcard_softc *sc)
+{
+ uint8_t csd_structure;
+ int error;
+
+ ALTERA_SDCARD_LOCK_ASSERT(sc);
+
+ /*
+ * XXXRW: Assume for now that when the SD Card IP Core negotiates
+ * voltage/speed/etc, it must use the CSD register, and therefore
+ * populates the SD Card IP Core's cache of the register value. This
+ * means that we can read it without issuing further SD Card commands.
+ * If this assumption proves false, we will (a) get back garbage and
+ * (b) need to add additional states in the driver state machine in
+ * order to query card properties before I/O can start.
+ *
+ * XXXRW: Treating this as an array of bytes, so no byte swapping --
+ * is that a safe assumption?
+ */
+ bus_read_region_1(sc->as_res, ALTERA_SDCARD_OFF_CSD,
+ sc->as_csd.csd_data, sizeof(sc->as_csd));
+
+ /*
+ * Interpret the loaded CSD, extracting certain fields and copying
+ * them into the softc for easy software access.
+ *
+ * Currently, we support only CSD Version 1.0. If we detect a newer
+ * version, suppress card detection.
+ */
+ csd_structure = sc->as_csd.csd_data[ALTERA_SDCARD_CSD_STRUCTURE_BYTE];
+ csd_structure &= ALTERA_SDCARD_CSD_STRUCTURE_MASK;
+ csd_structure >>= ALTERA_SDCARD_CSD_STRUCTURE_RSHIFT;
+ sc->as_csd_structure = csd_structure;
+
+ /*
+ * Interpret the CSD field based on its version. Extract fields,
+ * especially mediasize.
+ *
+ * XXXRW: Desirable to support further CSD versions here.
*/
- sc->as_mediasize = (c_size + 1) * (c_size_mult << 8) *
- (read_bl_len << 12);
- if (sc->as_mediasize == 0)
- sc->as_flags |= ALTERA_SDCARD_FLAG_BADCARD;
+ switch (sc->as_csd_structure) {
+ case 0:
+ error = altera_sdcard_process_csd0(sc);
+ if (error)
+ return (error);
+ break;
+
+ default:
+ device_printf(sc->as_dev,
+ "Ignored disk with unsupported CSD structure (%d)\n",
+ sc->as_csd_structure);
+ return (ENXIO);
+ }
+ return (0);
}
#if 0
==== //depot/projects/ctsrd/beribsd/src/sys/dev/altera/sdcard/altera_sdcard_nexus.c#2 (text+ko) ====
@@ -105,7 +105,7 @@
};
static driver_t altera_sdcard_nexus_driver = {
- "altera_sdcard",
+ "altera_sdcardc",
altera_sdcard_nexus_methods,
sizeof(struct altera_sdcard_softc),
};
More information about the p4-projects
mailing list