Date: Wed, 4 Apr 2012 10:14:21 GMT From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 209055 for review Message-ID: <201204041014.q34AELok085615@skunkworks.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://p4web.freebsd.org/@@209055?ac=10 Change 209055 by rwatson@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), };
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201204041014.q34AELok085615>