Messages in this thread | | | Date | Mon, 12 May 2008 19:54:35 +0200 | From | Pierre Ossman <> | Subject | Re: [PATCH] MMC/SD host driver for Ricoh Bay1Controllers |
| |
Big thanks to Andrew for helping out with the basic review here. :)
On Sun, 11 May 2008 10:50:55 +0200 Sascha Sommer <saschasommer@freenet.de> wrote:
> On Freitag, 2. Mai 2008, Andrew Morton wrote: > > > > Will this code work correctly on big-endian machines? > > > > Most likely not and there might be other big-endian related problems in the > driver. I don't know of a case where this hardware can be found on big > endian. >
That's not a really good reason to just ignore the problem though. Endian safe code is generally more bug free and more easily understood because it doesn't use arch specific shortcuts. I won't NAK the patch because of this, but I do prefer endian safe code even if there is no hardware built that needs it right now. It's always good to practice writing such code anyway. :)
> > > > hm. I don't really know how the kernel gets down into here, but I wonder > > if this driver (and, I bet, lots of similar ones) should be doing > > flush_dcache_page() after modifying the page. If this page can be file > > pagecache or user memory then "yes". Unless it's done elsewhere for us. > >
Andrew, could you elaborate a bit on this (couldn't find anything in the docs)? AFAIK dcache is a VFS thing, so I don't see how low level devices should have to deal with it. The VFS should be well aware of what pages have been submitted for data transfers.
> > > > Is the dependency on YENTA correct? > > > > Yes. At least noone who wants to enable this driver will forget the yenta > driver then. >
But is there a technical dependency? We shouldn't be introducing artificial dependencies just because of how Ricoh is currently bundling things.
The actual review:
> +#include <linux/mmc/mmc.h> > +#include <linux/mmc/sd.h> > +#include <linux/mmc/sdio.h>
Including these three in a host driver is always an error. The driver should be operating at a lower level than the stuff these describe.
> + /* wait for command completion */ > + if (opcode) { > + for (loop = 0; loop < CMD_TIMEOUT; loop++) { > + status = sdricoh_readl(host, R21C_STATUS); > + sdricoh_writel(host, R2E4_STATUS_RESP, status); > + if (status & STATUS_CMD_FINISHED) > + break; > + }
You might want to have a look at making this a bit more compliant with the spec. A MMC command can take 64 clock cycles plus the transfer time for the command and response. So you need to figure out how long a readl takes (as that is your source of delays) and adjust CMD_TIMEOUT depending on current clock frequency. You can submit a patch for this later though.
> + dev_dbg(dev, "mmc_cmd opcode=%i arg=0x%x => %i (queries=%i)\n", > + opcode, arg, result, loop);
The core should be giving you the same output.
> + if (result == 0) { > + /* EXT_CSD are filtered so this should be save */ > + if (opcode == SD_SEND_IF_COND) { > + if (host->mode != MODE_SDHC) { > + dev_info(dev, "switching to SDHC mode\n"); > + host->mode = MODE_SDHC; > + } > + } > + > + /* switch to SD mode if APP_CMDs are supported */ > + if (opcode == MMC_APP_CMD) { > + if (host->mode == MODE_MMC) { > + dev_info(dev, "switching to SD mode\n"); > + host->mode = MODE_SD; > + } > + } > + }
What's the point of this? Looking at the rest of the code, it seems to be mostly that you haven't properly mapped up the 4-bit bus control (which is used on MMC cards as well these days).
> +static int sdricoh_blockio(struct sdricoh_host *host, int read, > + unsigned int *buf) > +{ > + int i; > + /* wait until the data is available */ > + if (read) { > + if (sdricoh_query_status(host, STATUS_READY_TO_READ, > + TRANSFER_TIMEOUT)) > + return 0; > + sdricoh_writel(host, R21C_STATUS, 0x18); > + /* read data */ > + for (i = 0; i < 512 / 4; i++) > + buf[i] = sdricoh_readl(host, R230_DATA);
Is the controller only capable of 512 byte transfers? I seriously hope not, but if that's the case then you still need to make sure you fail transfers that aren't a multiple of 512.
> + /* wait until the tranfer is finished */ > + for (i = 0; i < BUSY_TIMEOUT; i++) { > + status = sdricoh_readl(host, R21C_STATUS); > + sdricoh_writel(host, R2E4_STATUS_RESP, status); > + if (!(status & STATUS_BUSY)) > + break; > + }
The card can be busy for minutes with some operations, so this is probably wrong.
> + dev_dbg(dev, "=============================\n"); > + dev_dbg(dev, "sdricoh_request opcode=%i\n", cmd->opcode);
More redundant debug output.
> + /* we cannot handle all commands that require a block transfer > + therefore do some ugly special handling here > + */
What kind of analysis have you done here? It's likely that the controller just needs some special handling, as there is so far only a single known controller (wbsd) that looks at opcodes.
> + if (cmd->data) { > + switch (cmd->opcode) { > + /* working commands */ > + case MMC_READ_SINGLE_BLOCK: > + case MMC_READ_MULTIPLE_BLOCK: > + case MMC_WRITE_BLOCK: > + case MMC_WRITE_MULTIPLE_BLOCK: > + break;
In any case, you should do what the hardware does and put a list of the actual integers the hardware looks at. Using the defines implies that it is those specific opcodes that are a problem (which likely isn't the case as opcodes are context dependent).
> + case SD_APP_SEND_SCR: /* required for SDHC */ > + cmd->error = sdricoh_mmc_cmd(host, cmd->opcode, > + cmd->arg); > + mmc_request_done(mmc, mrq); > + return;
What are you doing here?
> + > + /* read/write commands seem to require this */ > + if (data) { > + cmd->error = sdricoh_busy(host); > + if (cmd->error)
mmc_block should make sure this isn't a problem, so if you need this then we have a bug somewhere. Still, you should be waiting for the busy signal to go away at the end of commands, not compensating for it in the subsequent one.
> + } > + dev_dbg(dev, "resp[0]=0x%x\n", cmd->resp[0]); > + dev_dbg(dev, "resp[1]=0x%x\n", cmd->resp[1]); > + dev_dbg(dev, "resp[2]=0x%x\n", cmd->resp[2]); > + dev_dbg(dev, "resp[2]=0x%x\n", cmd->resp[3]); > + } else { > + cmd->resp[0] = sdricoh_readl(host, R20C_RESP); > + dev_dbg(dev, "resp[0]=0x%x\n", cmd->resp[0]); > + }
More redundant output. There's a few others, but you get the picture...
> + /* yet another workaround */ > + /* without the extra command SD cards do not work at all */ > + if (cmd->opcode == MMC_SELECT_CARD) { > + if (host->mode != MODE_MMC) { > + sdricoh_mmc_cmd(host, MMC_APP_CMD, cmd->arg); > + sdricoh_mmc_cmd(host, 0x46, 0x02); > + } else { > + sdricoh_writel(host, R228_POWER, 0xc0e0); > + sdricoh_writel(host, R224_MODE, 0x2000301); > + } > + } > +
This is probably caused by the fact that you're messing with the SCR earlier. You're sending a faked ACMD6 (SET_BUS_WIDTH) to the card.
> + buf = kmap(page) + data->sg->offset + (512 * i);
You cannot just assume that the block size is 512 bytes.
> +MODULE_PARM_DESC(switchlocked, "Switch the cards locked status." > + "Use this when unlocked cards are shown readonly (default 0)");
This doesn't seem to be used anywhere in the code.
You've done a great job figuring out how this controller works, but there is still a few more steps that need to be taken. The opcode specific workarounds need to go, so a bit more testing and probing is necessary. For example bit 0x40 of the mode register looks like a prime candidate for bus width control.
Rgds -- -- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org rdesktop, core developer http://www.rdesktop.org
| |