From: "Jordan Crouse" <jordan.crouse@amd.com>
To: "Pierre Ossman" <drzeus-list@drzeus.cx>
Cc: rmk+lkml@arm.linux.org.uk, linux-kernel@vger.kernel.org
Subject: Re: Minimise protocol awareness in Au1x00 driver
Date: Fri, 6 Jan 2006 18:01:59 -0700 [thread overview]
Message-ID: <20060107010159.GC17575@cosmic.amd.com> (raw)
In-Reply-To: <43BF00A4.8070606@drzeus.cx>
> Whilst doing this I also noticed how horribly broken this driver is with
> regard to sending the stop command. Instead of sending the requested
> command it sends a hard coded opcode!! Jordan, please fix this ASAP.
I recognize that this is a bad thing, but bear with me for a second while
I digress.
The current thinking, as far as I can tell is that that the drivers need to
be aware that that data->stop opcode may be something other then CMD12.
If that is true, then I'm worried about this snippet from your patch:
+ if (host->mrq->data && (host->mrq->data->stop == cmd))
mmccmd |= SD_CMD_CT_7;
- break;
+ else if (!cmd->data)
+ mmccmd |= SD_CMD_CT_0;
Because, as the AMD specification states that CT_7 means (page 420 of
the AU1200 data book):
* Terminate transfer of a multiple block write or read. Use when
* doing a STOP_TRANSMISSION (CMD12) command.
The reason why I was so protocol heavy in the original version of this
driver was because the spec is very specific in this regard. CT_7 *means*
a CMD12, CT_3 *mean* a CMD25, so on and so forth. Your code does an
excellent job of removing these dependencies, but it opens up the door
for scary behavior if the command opcode behind the command type is ever
changed.
That said, I recognize that my decision to hard code the stop command
was a stupid one (it was done for speed reasons - if you assume that a CMD12
always stops the transaction, then why bother parsing the cmd structure)?
So let me be blunt - why are we trying to be so generic? Is it because
we want to keep the door open for future versions of the SD specification
that may change things up (which is an admirable goal, I admit).
If that is the case however, then perhaps we need to have some sort of
version control mechanism in place - since the AU1200 SD controller clearly
states that:
* The SD controllers comply with version 1.1 of the SD card specification.
* References in this section are to that version of the specification.
So, it is perfectly reasonable for the SD controller to make assumptions,
like say that that stop is *always* CMD12, etc. And if we allow that
to be the case, then perhaps we should insert some checking into the
subsystem that will check the supported SD version (if it should ever
change in the future), and not ask a driver to do anything it cannot.
Anyway, let me ruminate on your patch for a day or so, and I'll see if
I can scratch something together to fix the stop opcode problem.
Regards,
Jordan
--
Jordan Crouse
Senior Linux Engineer
AMD - Personal Connectivity Solutions Group
<www.amd.com/embeddedprocessors>
next prev parent reply other threads:[~2006-01-07 0:54 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-01-06 23:40 [PATCH] [MMC] Minimise protocol awareness in Au1x00 driver Pierre Ossman
2006-01-06 23:43 ` Pierre Ossman
2006-01-07 1:01 ` Jordan Crouse [this message]
2006-01-07 1:09 ` Pierre Ossman
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=20060107010159.GC17575@cosmic.amd.com \
--to=jordan.crouse@amd.com \
--cc=drzeus-list@drzeus.cx \
--cc=linux-kernel@vger.kernel.org \
--cc=rmk+lkml@arm.linux.org.uk \
/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