From: Pierre Ossman <drzeus-mmc-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
To: David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
Mikael Starvik <mikael.starvik-VrBV9hrLPhE@public.gmane.org>,
Hans-Peter Nilsson
<hans-peter.nilsson-VrBV9hrLPhE@public.gmane.org>,
Mike Lavender
<mike-UTnDXsALFwNjMdQLN6DIHgC/G2K4zDHf@public.gmane.org>
Subject: Re: [patch 4/4] MMC-over-SPI host driver
Date: Wed, 13 Jun 2007 21:32:07 +0200 [thread overview]
Message-ID: <46704637.2090309@drzeus.cx> (raw)
In-Reply-To: <200706101308.07910.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
David Brownell wrote:
> +#include <linux/device.h>
> +#include <linux/blkdev.h>
> +#include <linux/dma-mapping.h>
> +
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +
This header file is a warning sign. Hopefully it's just the mapping that needs
it, so it should go away in the next revision.
> +
> +
> +#define CRC_GO_IDLE_STATE 0x95 /* constant CRC for GO_IDLE */
Isn't this just optimisation now that we have CRC calculation? And if so, is the
speed gain worth the complexity?
> +
> +/* The unit for these timeouts is milliseconds. See mmc_spi_scanbyte. */
> +#define MINI_TIMEOUT 1
> +#define READ_TIMEOUT 100
> +#define WRITE_TIMEOUT 250
> +
Shouldn't these be set in the request by the core?
> +
> + /* Specs say to write ones most of the time, even when the card
> + * has no need to read its input data; and many cards won't care.
> + * This is our source of those ones.
> + */
> + void *ones;
If this is just read, can't it be a global const?
> +
> + if (cmd->opcode == MMC_STOP_TRANSMISSION) {
> + /*
> + * We can't tell whether we read block data or the
> + * command reply, so to cope with trash data during
> + * the latency, we just read in 14 bytes (8 would be
> + * enough according to the MMC spec; SD doesn't say)
> + * after the command and fake a clean reply. We could
> + * avoid this if we saved what the card sent us while
> + * we sent the command, and treat it like a normal
> + * response if we didn't get a SPI_TOKEN_SINGLE.
> + */
> + (void) mmc_spi_readbytes(host, host->command.buf,
> + sizeof host->command.buf);
> + (void) mmc_spi_readbytes(host, host->command.buf,
> + sizeof host->command.buf);
> + value = 0;
Didn't you tweak the block driver to not send MMC_STOP_TRANSMISSION?
> +
> + if (!host->app_cmd
> + && cmd->error == MMC_ERR_NONE
> + && cmd->opcode == MMC_APP_CMD) {
> + host->app_cmd = 1;
> + cmd->resp[0] |= R1_APP_CMD;
> + }
Why does the host need to keep track of app command state?
> +
> + /*
> + * If this was part of a successful request with a stop-part,
> + * our caller signals the request as done.
> + */
> + if (status == 0 && mrq->stop == NULL)
> + mmc_request_done(host->mmc, mrq);
> + return status;
> +}
I assume the comment should have read "without".
> +
> +static inline int resp2status(u8 write_resp)
> +{
> + switch (SPI_MMC_RESPONSE_CODE(write_resp)) {
> + case SPI_RESPONSE_ACCEPTED:
> + return 0;
> + case SPI_RESPONSE_CRC_ERR:
> + /* host shall then issue MMC_STOP_TRANSMISSION */
> + return -ECRC;
What's with the made-up errno?
We should check what other parts of the kernel uses for CRC errors.
> +
> + if (data->flags & MMC_DATA_READ) {
> + direction = DMA_FROM_DEVICE;
> + multiple = (cmd->opcode == MMC_READ_MULTIPLE_BLOCK);
> +
multiple = (cmd->data->blocks > 1)
> + } else {
> + direction = DMA_TO_DEVICE;
> + multiple = (cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK);
> + }
same thing here
> +
> + /* allow pio too, with kmap handling any highmem */
> + kmap_addr = kmap(sg->page);
> + if (direction == DMA_TO_DEVICE)
> + t->tx_buf = kmap_addr + sg->offset;
> + else
> + t->rx_buf = kmap_addr + sg->offset;
> +
If you want highmem, you also need to be prepared for clustered sg entries. kmap
only maps a single page, and a clustered sg might be larger than that.
> +
> + /* NOTE if caller issues SET_BLOCK_COUNT before a multiblock write,
> + * this STOP_TRAN logic isn't needed except when we stop early for
> + * errors. Currently, mmc_block doesn't issue that request.
> + */
But will it hurt? Other commands might behave like that.
> +
> + if (mrq->data && (mrq->data->blksz > MMC_SPI_BLOCKSIZE)) {
> + mrq->cmd->error = MMC_ERR_INVALID;
> + return -EINVAL;
> + }
> +
If you've set your parameters correct, then this shouldn't be possible. So do a
BUG_ON() if you want to have a test.
> +
> +/*
> + * RESET is started when cmd->opcode == MMC_GO_IDLE_STATE. This can't
> + * be just a standard protocol operation.
> + *
> + * We expect the MMC core to be responsible for "very hard" resets like
> + * power cycling the card; there's no accessible reset signal.
> + */
Why can't you do this on POWER_ON? I do not like it looking at opcodes, even if
this one isn't likely to change.
> + /*
> + * Do a burst with chipselect deactivated. We need to do this
> + * to meet the requirement of 74 clock cycles with chipselect
> + * high before CMD0. (Section 6.4.1, in "Simplified Physical
> + * Layer Specification 2.0".) Some cards are particularly
> + * needy of this (e.g. Viking "SD256") while most others don't
> + * seem to care. Note that it's not enough to deactivate
> + * chipselect without toggling the clock. Beware of the hack:
> + * we "know" that mmc_spi_readbytes uses the host->status
> + * spi_transfer.
> + *
> + * Note that this is one of two places MMC/SD plays games with
> + * the SPI protocol. The other is that when chipselect is
> + * released while the card returns BUSY status, the clock must
> + * issue several cycles with chipselect high before the card
> + * will stop driving its output.
> + */
Sounds like this signaling can be done with the chip select ios already present
in the mmc layer. Then we won't need voodoo in the host driver.
> +
> + /* issue software reset */
> + cmd->arg = 0;
> + status = mmc_spi_command_send(host, mrq, CRC_GO_IDLE_STATE, cmd, 0);
> + if (status < 0) {
> + /* Maybe:
> + * - there's no card present
> + * - the card isn't seated correctly (bad contacts)
> + * - it didn't leave MMC/SD mode
> + * - there's other confusion in the card state
> + *
> + * Power cycling the card ought to help a lot.
> + * At any rate, let's try again.
> + */
> + status = mmc_spi_command_send(host, mrq,
> + CRC_GO_IDLE_STATE, cmd, 0);
> + if (status < 0)
> + dev_dbg(&host->spi->dev,
> + "can't initialize; no card%s?\n",
> + could_invert_cs
> + ? ""
> + : " or chip-select error");
> + }
No, no, no... This is not the kind of behaviour you want to hide down in the
host driver. This is a decision the core should make. Don't put the layer cake
in the blender.
> +
> + /* MMC core and layered drivers *MUST* issue SPI-aware commands */
> + cmd = mrq->stop;
> + if (cmd && !mmc_spi_resp_type(cmd)) {
> + dev_dbg(&host->spi->dev, "bogus STOP command\n");
> + dump_stack();
> + cmd->error = MMC_ERR_INVALID;
> + goto fail;
> + }
> +
This seems like something that should just be in debug builds.
> +
> +static int mmc_spi_get_ro(struct mmc_host *mmc)
> +{
> + struct mmc_spi_host *host = mmc_priv(mmc);
> +
> + if (host->pdata && host->pdata->get_ro)
> + return host->pdata->get_ro(mmc->parent);
> + /* board doesn't support read only detection; assume writeable */
> + return 0;
> +}
> +
A printk() might be nice so the user knows why the switch isn't being respected.
> +
> + /* SanDisk and Hitachi MMC docs both show clock timing diagrams
> + * with clock starting low (CPOL=0) and sampling on leading edge
> + * (CPHA=0); clock is measured between rising edges. Sandisk SD
> + * docs show clock starting high (CPOL=1) and sampling on trailing
> + * edge (CPHA=1), measuring between falling edges.
> + *
> + * Docs are very explicit that sampling is on the rising edge, so
> + * the difference between SPI_MODE_0 and SPI_MODE_3 may not matter.
> + */
Sure you haven't looked at high-speed versus legacy clocking?
> +
> + if (spi->master->cdev.dev->dma_mask) {
> + struct device *dev = spi->master->cdev.dev;
> +
> + host->dma_dev = dev;
> + host->dma = dma_map_single(dev, host,
> + sizeof *host, DMA_BIDIRECTIONAL);
> + host->ones_dma = dma_map_single(dev, ones,
> + MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
> + host->data_dma = dma_map_single(dev, host->data,
> + sizeof *host->data, DMA_BIDIRECTIONAL);
> +
This might be wasteful if the system has an iommu.
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 DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
next prev parent reply other threads:[~2007-06-13 19:32 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-06-10 19:57 [patch 0/4] MMC-over-SPI for 2.6.22-rc4-mm David Brownell
[not found] ` <200706101257.45278.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-10 20:05 ` [patch 1/4] MMC-over-SPI header updates David Brownell
[not found] ` <200706101305.53151.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-12 17:22 ` Pierre Ossman
[not found] ` <466ED661.1010407-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-12 18:06 ` David Brownell
[not found] ` <200706121106.42067.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-13 8:25 ` Pierre Ossman
[not found] ` <466FAA0C.9020102-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-13 21:33 ` David Brownell
[not found] ` <200706131433.21238.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-15 17:53 ` Pierre Ossman
[not found] ` <4672D202.3000901-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-16 18:50 ` David Brownell
[not found] ` <200706161150.58273.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-20 15:52 ` Pierre Ossman
[not found] ` <46794D3B.1060009-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-22 20:08 ` David Brownell
2007-06-10 20:06 ` [patch 2/4] MMC-over-SPI card driver update David Brownell
[not found] ` <200706101306.39081.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-12 17:28 ` Pierre Ossman
2007-06-10 20:07 ` [patch 3/4] MMC-over-SPI core updates David Brownell
[not found] ` <200706101307.11394.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-13 8:12 ` Pierre Ossman
[not found] ` <466FA700.2080009-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-14 0:53 ` David Brownell
[not found] ` <200706131753.47453.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-15 18:03 ` Pierre Ossman
[not found] ` <4672D474.4060707-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-16 21:16 ` David Brownell
[not found] ` <200706161416.17802.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-20 15:56 ` Pierre Ossman
[not found] ` <46794E1B.6010907-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-22 20:42 ` David Brownell
2007-06-10 20:08 ` [patch 4/4] MMC-over-SPI host driver David Brownell
[not found] ` <200706101308.07910.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-13 19:32 ` Pierre Ossman [this message]
[not found] ` <46704637.2090309-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-14 4:05 ` David Brownell
[not found] ` <200706132105.51711.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-15 18:26 ` Pierre Ossman
[not found] ` <4672D9C5.9080707-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-17 20:29 ` David Brownell
[not found] ` <200706171329.12709.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-20 16:20 ` Pierre Ossman
[not found] ` <467953E0.8050408-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-20 17:05 ` David Brownell
[not found] ` <20070620170511.EC564F31CB-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org>
2007-06-20 17:51 ` Pierre Ossman
[not found] ` <4679691C.2060803-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>
2007-06-22 21:18 ` David Brownell
[not found] ` <200706221418.44214.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2007-06-30 18:46 ` Pierre Ossman
2007-07-12 18:14 ` David Brownell
2007-07-12 17:52 ` David Brownell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=46704637.2090309@drzeus.cx \
--to=drzeus-mmc-p3sgcrwkh8cezlla646fqq@public.gmane.org \
--cc=david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org \
--cc=hans-peter.nilsson-VrBV9hrLPhE@public.gmane.org \
--cc=mikael.starvik-VrBV9hrLPhE@public.gmane.org \
--cc=mike-UTnDXsALFwNjMdQLN6DIHgC/G2K4zDHf@public.gmane.org \
--cc=spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).