lkml.org 
[lkml]   [2008]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2008-05-12 19:57    [W:0.095 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site