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 3/4] MMC-over-SPI core updates
Date: Wed, 13 Jun 2007 10:12:48 +0200 [thread overview]
Message-ID: <466FA700.2080009@drzeus.cx> (raw)
In-Reply-To: <200706101307.11394.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.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/
next prev parent reply other threads:[~2007-06-13 8:12 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 [this message]
[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
[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=466FA700.2080009@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).