Messages in this thread | | | Date | Sun, 31 Dec 2006 13:32:18 +0100 | From | Pierre Ossman <> | Subject | Re: [RFC] MTD driver for MMC cards |
| |
Arnd Bergmann wrote: > This is an experiment on how an SD/MMC card could be used in the MTD layer. > I don't currently have a system set up to test this, so this driver is > completely _untested_ and therefore you should consider it _broken_. > > You can get similar functionality by using the mmc_block driver together > with block2mtd, so you may wonder what the point of another driver is. > IMHO, there are two separate advantages from using a special driver: > > * better use of low-level interfaces: the MTD driver can detect the > erase block size of the card and erase sectors in advance instead of > blocking in the write path. The MTD file systems also expect the > underlying interface to be synchronous, so there is little point > in using extra kernel threads to operate on the card in the background. >
I'm a complete MTD noob, but what uses does the MTD layer have besides JFFS2. If it's none, than this advantage isn't that big of a deal.
> * It becomes possible to use MMC cards with jffs2 even with CONFIG_BLOCK > disabled, which can save a significant amount of kernel memory on > small machines that have an MMC slot but no other block device. >
From what I've heard, JFFS2 is close to unusuable on the sizes of modern SD/MMC cards. So I'd like to see some more use cases before I'm ready to let this in.
> I still want to be sure that I'm on the right track with this driver > and did not make a conceptual mistake. >
I can comment it from a MMC perspective, but the MTD stuff I will have to assume is correct.
> @@ -616,6 +616,8 @@ static void mmc_decode_csd(struct mmc_ca > csd->r2w_factor = UNSTUFF_BITS(resp, 26, 3); > csd->write_blkbits = UNSTUFF_BITS(resp, 22, 4); > csd->write_partial = UNSTUFF_BITS(resp, 21, 1); > + csd->erase_blksize = (UNSTUFF_BITS(resp, 37, 5) + 1) * > + (UNSTUFF_BITS(resp, 42, 5) + 1); > } else { > /* > * We only understand CSD structure v1.1 and v1.2.
NAK. SD uses another format for erase blocks. See the simplified physical spec.
> +/* > + * transfer a block to/from the card. The block needs to be aligned > + * to mtd->writesize. If we want to implement an mtd_writev method, > + * this needs to use stream operations with an appropriate stop > + * command as well. > + */ > +static int mmc_mtd_transfer_low(struct mmc_card *card, loff_t off, size_t len, > + size_t *retlen, u_char *buf, int write) > +{ > + struct scatterlist sg; > + struct mmc_data data = { > + .blksz = 1 << card->csd.read_blkbits, > + .blocks = len >> card->csd.read_blkbits,
First of all, you cannot assume that read_blkbits is a valid block size when doing writes.
Secondly, the cards default in a block size of 512 bytes, so you need to tell the card your desired block size during probe.
> + .flags = write ? MMC_DATA_WRITE : MMC_DATA_READ, > + .sg = &sg, > + .sg_len = 1, > + }; > + struct mmc_command cmd = { > + .arg = off, > + .data = &data, > + .flags = MMC_RSP_R1 | MMC_CMD_ADTC, > + .opcode = write ? MMC_WRITE_BLOCK : MMC_READ_SINGLE_BLOCK,
You set .blocks above, so I have to assume it can be more than 1. So you need to change the opcodes accordingly.
> + }; > + struct mmc_request mrq = { > + .cmd = &cmd, > + .data = &data, > + };
And it also means you need a stop command.
> + > + /* copied from the block driver, don't understand why this is needed */
Now this gives me a bad feeling. Have you read any spec about the MMC protocol or are you just winging it?
It is needed because the card goes into programming state after a write, where it is very unresponsive to other commands.
> + > + ret = mmc_card_claim_host(card); > + if (ret) { > + dev_warn(&card->dev, "%s: mmc_card_claim_host returned %d\n", > + __FUNCTION__, ret); > + ret = -EIO; > + goto error; > + }
mmc_card_claim_host() is currently very stupid in that it requires you to call mmc_card_release_host() on error. I intend to fix that some time in the future.
> +/* > + * Initialize an mmc card. We create a new MTD device for each > + * MMC card we find. The operations are rather straightforward, > + * so we don't even need our own data structure to contain the > + * mtd_info. > + */ > +static int mmc_mtd_probe(struct mmc_card *card) > +{ > + struct mtd_info *mtd; > + int ret; > + > + if (!(card->csd.cmdclass & CCC_ERASE)) > + return -ENODEV; > +
You should probably check for CCC_BLOCK_READ here.
And your driver needs to check if the card support writes (both by mmc_card_readonly() and CCC_BLOCK_WRITE).
Rgds -- -- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |