On Sat, 14 Jul 2007 15:08:06 -0700
David Brownell <[EMAIL PROTECTED]> wrote:
> --- g26.orig/drivers/mmc/host/Kconfig 2007-07-14 14:47:11.000000000 -0700
> +++ g26/drivers/mmc/host/Kconfig 2007-07-14 14:47:57.000000000 -0700
> @@ -100,3 +100,16 @@ config MMC_TIFM_SD
> To compile this driver as a module, choose M here: the
> module will be called tifm_sd.
>
> +config MMC_SPI
> + tristate "MMC/SD over SPI"
> + depends on MMC && SPI_MASTER && !HIGHMEM && EXPERIMENTAL
> + select CRC7
> + select CRC_ITU_T
> + help
> + Some systems accss MMC/SD cards using a SPI controller instead of
> + using a "native" MMC/SD controller. This has a disadvantage of
> + being relatively high overhead, but a compensating advantage of
> + working on many systems without dedicated MMC/SD controllers.
> +
> + If unsure, or if your system has no SPI master driver, say N.
> +
It is customary to have "(Experimental)" in the description aswell.
> +
> +static int
> +mmc_spi_readbytes(struct mmc_spi_host *host, unsigned len)
> +{
> + int status;
> +
> + if (len > sizeof *host->data) {
> + WARN_ON(1);
> + return -EIO;
> + }
sizeof without parenthesis? Heathen!
> +
> +/*
> + * Note that for SPI, cmd->resp[0] is not the same data as "native" protocol
> + * hosts return! The low byte holds R1_SPI bits. The next byte may hold
> + * R2_SPI bits ... for SEND_STATUS, or after data read errors.
> + *
> + * cmd->resp[1] holds any four-byte response, for R3 (READ_OCR) and on
> + * newer cards R7 (IF_COND).
> + */
> +
> +static char *maptype(struct mmc_command *cmd)
> +{
> + switch (mmc_spi_resp_type(cmd)) {
> + case MMC_RSP_SPI_R1: return "R1";
> + case MMC_RSP_SPI_R1B: return "R1B";
> + case MMC_RSP_SPI_R2: return "R2";
> + case MMC_RSP_SPI_R3: return "R3/R7";
> + default: return "?";
> + }
> +}
> +
I'm trying to move all generic debugging code into the core, so I would prefer
if you would have a look at extending that to fit your needs. That way everyone
wins.
> +
> + /* Status byte: the entire seven-bit R1 response. */
> + if (cmd->resp[0] != 0) {
> + if (R1_SPI_COM_CRC & cmd->resp[0]) {
> + cmd->error = MMC_ERR_BADCRC;
> + value = -EILSEQ;
> + } else if ((R1_SPI_PARAMETER | R1_SPI_ADDRESS
> + | R1_SPI_ILLEGAL_COMMAND)
> + & cmd->resp[0]) {
> + cmd->error = MMC_ERR_INVALID;
> + value = -EINVAL;
> + } else if ((R1_SPI_ERASE_SEQ | R1_SPI_ERASE_RESET)
> + & cmd->resp[0]) {
> + cmd->error = MMC_ERR_FAILED;
> + value = -EINVAL;
> + } /* else R1_SPI_IDLE, "it's resetting" */
> +
> + if (value < 0)
> + goto fail;
> + }
I'd still preferred that you'd didn't try to decode this here. And your code
demostrates why; CRC error and illegal command both signal problems with the
previous command, not the current one, so this code would be failing the wrong
request.
> +
> + switch (mmc_spi_resp_type(cmd)) {
> +
> + /* SPI R1B == R1 + busy; STOP_TRANSMISSION (for multiblock reads)
> + * and less-common stuff like various erase operations.
> + */
> + case MMC_RSP_SPI_R1B:
for (i = 0;i < mmc_spi_resp_size(cmd);i++)
cmd->resp[i/4] |= *cp++ << (24 - (i % 4) * 8);
if (cmd->flags & MMC_SPI_RSP_BUSY)
mmc_spi_wait_unbusy();
That's the whole point of hacking up the response types into components, so
that the host driver doesn't have to care.
You can keep your design if you'd like, but I really think this is cleaner and
more layered.
> +
> + /* We can handle most commands (except block reads) in one full
> + * duplex I/O operation before either starting the next transfer
> + * (data block or command) or else deselecting the card.
> + *
> + * First, write 7 bytes:
> + * - an all-ones byte to ensure the card is ready
> + * - opcode byte (plus start and transmission bits)
> + * - four bytes of big-endian argument
> + * - crc7 (plus end bit) ... always computed, it's cheap
> + *
I thought your argument against always using crc7 was that it wasn't cheap?
Colour me confused.
> + if (status == -EILSEQ)
> + data->error = MMC_ERR_BADCRC;
> + else if (status == -ETIMEDOUT)
> + data->error = MMC_ERR_TIMEOUT;
> + else if (status == -EINVAL)
> + data->error = MMC_ERR_INVALID;
> + else if (data->error == MMC_ERR_NONE)
> + data->error = MMC_ERR_FAILED;
> + break;
> + }
At least we agree on error codes. This is precisely the change I have pending
in my patch set to remove the MMC_ERR_* mess. :)
There are a few outstanding issues, but they are really minor so this could
very soon get into my -mm branch. I need a MAINTAINERS entry aswell.
I'd like to see those REVISIT things attended to before I push the stuff to
Linus though.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
