From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pierre Ossman Subject: Re: [patch 3/4] MMC-over-SPI core updates Date: Wed, 13 Jun 2007 10:12:48 +0200 Message-ID: <466FA700.2080009@drzeus.cx> References: <200706101257.45278.david-b@pacbell.net> <200706101307.11394.david-b@pacbell.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, Mikael Starvik , Hans-Peter Nilsson , Mike Lavender To: David Brownell Return-path: In-Reply-To: <200706101307.11394.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: spi-devel-general-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: spi-devel-general-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: linux-spi.vger.kernel.org David Brownell wrote: > @@ -498,6 +505,19 @@ void mmc_detect_change(struct mmc_host * > EXPORT_SYMBOL(mmc_detect_change); > > > +static int mmc_spi_fixup(struct mmc_host *host, u32 *ocrp) > +{ > + int err; > + > + if (!mmc_host_is_spi(host)) > + return MMC_ERR_NONE; > + > + err = mmc_spi_read_ocr(host, ocrp); > + if (err == MMC_ERR_NONE); > + err = mmc_spi_set_crc(host); > + return err; > +} > + > void mmc_rescan(struct work_struct *work) > { > struct mmc_host *host = I'd prefer if this is in mmc_rescan(). It's so little code that all this does is hide the init sequence. > @@ -519,11 +539,13 @@ void mmc_rescan(struct work_struct *work > mmc_power_up(host); > mmc_go_idle(host); > > - mmc_send_if_cond(host, host->ocr_avail); > + if (!mmc_host_is_spi(host)) > + mmc_send_if_cond(host, host->ocr_avail); > If you have a look at the SD physical spec (the public one), you'll see that if_cond is needed on SPI aswell. I know you don't have cards for testing, but let's code according to spec and try to find people who can test all parts. > err = mmc_send_app_op_cond(host, 0, &ocr); > if (err == MMC_ERR_NONE) { > - if (mmc_attach_sd(host, ocr)) > + err = mmc_spi_fixup(host, &ocr); > + if (err != MMC_ERR_NONE || mmc_attach_sd(host, ocr)) > mmc_power_off(host); > } else { > /* Looking at the example flow charts in the specs, this is not the proper way. I've spent the morning staring at both specs in parallel, and here's how I think it should be done: [CMD8 for SD] CMD58 [with HCS bit for MMC] ACMD51, OK -> SD CMD1, OK -> MMC The SD spec states that both ACMD51 and CMD1 is legal, but nothing about why both are supported. It also states that CMD1 has a bit indicating SDHC support (as if CMD8 isn't enough), but ACMD51 doesn't. But the flow charts show ACMD51, not CMD1. :/ The next step, in the bus handlers, will have to get the OCR on their own. SD states that the OCR is valid in the initial CMD58 (before CMD1/ACMD51), but MMC states that it is not. So for the sake of consistency, let both paths reissue a CMD58 for the OCR. > --- pxa.orig/drivers/mmc/core/mmc_ops.c 2007-06-10 13:00:21.000000000 -0700 > +++ pxa/drivers/mmc/core/mmc_ops.c 2007-06-10 13:00:34.000000000 -0700 > @@ -31,6 +31,10 @@ static int _mmc_select_card(struct mmc_h > > BUG_ON(!host); > > + /* SPI doesn't multiplex; "select" is meaningless */ > + if (mmc_host_is_spi(host)) > + return MMC_ERR_NONE; > + > memset(&cmd, 0, sizeof(struct mmc_command)); > > cmd.opcode = MMC_SELECT_CARD; I do not like this. Unsupported commands shouldn't be issued. We should not have voodoo in here that makes them a no-op. That just obscures things. Feel free to put a check in there if you feel the risk is too great. > > -int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd) > +struct mmc_cxd { > + struct mmc_card *card; /* optional */ > + void *buf; > + unsigned len; > + u32 opcode; > + u32 arg; > + unsigned flags; > +}; > + This seems overly complex. You can do this with arguments as most of that struct is superfluous: - card can be specified at all times if you reorg the init a bit, allowing you to drop the host argument. - arg and flags are the same for all three commands, so those can be dropped. > @@ -230,6 +251,85 @@ int mmc_send_ext_csd(struct mmc_card *ca > return MMC_ERR_NONE; > } > > +int mmc_spi_read_ocr(struct mmc_host *host, u32 *ocrp) > +{ How about moving these further down so that the mmc_send_cxd_data() users are right below it? Also, the theme I've tried to have here is to take a card pointer whenever you operate on a specific card, and a host pointer for bus wide things. It adds a bit of readability. > --- pxa.orig/drivers/mmc/core/mmc.c 2007-06-10 13:00:21.000000000 -0700 > +++ pxa/drivers/mmc/core/mmc.c 2007-06-10 13:00:34.000000000 -0700 > @@ -320,9 +320,15 @@ static int mmc_init_card(struct mmc_host > /* > * Fetch CID from card. > */ > - err = mmc_all_send_cid(host, cid); > - if (err != MMC_ERR_NONE) > - goto err; > + if (mmc_host_is_spi(host)) { > + err = mmc_spi_send_cid(host, cid); > + if (err != MMC_ERR_NONE) > + goto err; > + } else { > + err = mmc_all_send_cid(host, cid); > + if (err != MMC_ERR_NONE) > + goto err; > + } > > if (oldcard) { > if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0) This is entirely subjective, but wouldn't this be cleaner: if (mmc_host_is_spi(host)) err = mmc_spi_send_cid(host, cid); else err = mmc_all_send_cid(host, cid); if (err != MMC_ERR_NONE) goto err; It's fewer lines at least, which should make Linus happy. ;) > @@ -401,13 +411,15 @@ static int mmc_sd_init_card(struct mmc_h > } > > /* > - * Set card RCA. > + * For native busses: set card RCA and leave open drain mode. > */ While your in the processes of fixing my crappy comments, this command actually gets, not sends the RCA (SD is a bit backwards). :) > @@ -455,9 +467,11 @@ static int mmc_sd_init_card(struct mmc_h > /* > * Fetch switch information from card. > */ > - err = mmc_read_switch(card); > - if (err != MMC_ERR_NONE) > - goto free_card; > + if (!mmc_host_is_spi(host)) { > + err = mmc_read_switch(card); > + if (err != MMC_ERR_NONE) > + goto free_card; > + } > } > > /* Don't you want high-speed on your SPI bus? ;) Add a FIXME here if you don't have the cards to test this. Or implement it according to spec and wait for bug reports. > --- pxa.orig/drivers/mmc/core/sd_ops.c 2007-06-06 16:47:58.000000000 -0700 > +++ pxa/drivers/mmc/core/sd_ops.c 2007-06-10 13:00:34.000000000 -0700 > @@ -89,10 +89,10 @@ int mmc_app_cmd(struct mmc_host *host, s > > if (card) { > cmd.arg = card->rca << 16; > - cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; > + cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC; > } else { > cmd.arg = 0; > - cmd.flags = MMC_RSP_R1 | MMC_CMD_BCR; > + cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_BCR; > } > > err = mmc_wait_for_cmd(host, &cmd, 0); You also need to fix the check further down as SPI doesn't have a bit indicating if it toggled into ACMD mode. > @@ -175,6 +179,9 @@ int mmc_send_if_cond(struct mmc_host *ho > int err; > static const u8 test_pattern = 0xAA; > > + if (mmc_host_is_spi(host)) > + return MMC_ERR_FAILED; > + > /* > * To support SD 2.0 cards, we must always invoke SD_SEND_IF_COND > * before SD_APP_OP_COND. This command will harmlessly fail for See above, we need this for SDHC. 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/