[PATCH] [MMC] Minimise protocol awareness in Au1x00 driver

[PATCH] [MMC] Minimise protocol awareness in Au1x00 driver

[Pierre Ossman]

The Au1x00 MMC/SD driver currently contains switch statements based on
protocol opcodes, not on desired behaviour.

Unfortunately the AMD specification is not detailed enough to determine
how the controller will behave for the response settings. For now, it will
have to suffice to warn when we have an unknown response type.

Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
---

 drivers/mmc/au1xmmc.c |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/au1xmmc.c b/drivers/mmc/au1xmmc.c
index aaf0463..23cdcac 100644
--- a/drivers/mmc/au1xmmc.c
+++ b/drivers/mmc/au1xmmc.c
@@ -207,26 +207,33 @@ static int au1xmmc_send_command(struct a
 	case MMC_RSP_R3:
 		mmccmd |= SD_CMD_RT_3;
 		break;
+	default:
+		printk(KERN_ERR "%s: Unsupported response type (%x).\n",
+			mmc_hostname(host->mmc), cmd->flags);
+		return MMC_ERR_INVALID;
 	}
 
-	switch(cmd->opcode) {
-	case MMC_READ_SINGLE_BLOCK:
-	case SD_APP_SEND_SCR:
-		mmccmd |= SD_CMD_CT_2;
-		break;
-	case MMC_READ_MULTIPLE_BLOCK:
-		mmccmd |= SD_CMD_CT_4;
-		break;
-	case MMC_WRITE_BLOCK:
-		mmccmd |= SD_CMD_CT_1;
-		break;
-
-	case MMC_WRITE_MULTIPLE_BLOCK:
-		mmccmd |= SD_CMD_CT_3;
-		break;
-	case MMC_STOP_TRANSMISSION:
+	if (host->mrq->data && (host->mrq->data->stop == cmd))
 		mmccmd |= SD_CMD_CT_7;
-		break;
+	else if (!cmd->data)
+		mmccmd |= SD_CMD_CT_0;
+	else {
+		if (cmd->data->stop) {
+			if (cmd->data->flags & MMC_DATA_WRITE)
+				mmccmd |= SD_CMD_CT_3;
+			else
+				mmccmd |= SD_CMD_CT_4;
+		} else if (cmd->data->blocks == 1) {
+			if (cmd->data->flags & MMC_DATA_WRITE)
+				mmccmd |= SD_CMD_CT_1;
+			else
+				mmccmd |= SD_CMD_CT_2;
+		} else {
+			printk(KERN_ERR "%s: Multi-block transfer without "
+				"a stop command is not supported.\n",
+				mmc_hostname(host->mmc));
+			return MMC_ERR_INVALID;
+		}
 	}
 
 	au_writel(cmd->arg, HOST_CMDARG(host));

Re: [PATCH] [MMC] Minimise protocol awareness in Au1x00 driver

[Pierre Ossman]

Pierre Ossman wrote:
> The Au1x00 MMC/SD driver currently contains switch statements based on
> protocol opcodes, not on desired behaviour.
>
> Unfortunately the AMD specification is not detailed enough to determine
> how the controller will behave for the response settings. For now, it will
> have to suffice to warn when we have an unknown response type.
>
> Signed-off-by: Pierre Ossman <drzeus@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.

Rgds
Pierre

Re: Minimise protocol awareness in Au1x00 driver

[Jordan Crouse]

> 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>

Re: Minimise protocol awareness in Au1x00 driver

[Pierre Ossman]

Jordan Crouse wrote:
>> 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.
>
>   

Quite correct. A test for the opcode and an error if it's "wrong" would
be the safest approach. Usually these types of command types are there
to kick the controller's state machine in the correct direction. So it
probably doesn't care what the command is (CMD12 happens to be the only
valid command for this transition ATM). Only those who know the
hardware's internal workings can answer this. So we either get in touch
with one of these people or we create a test case to determine this.

> 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).
>
>   

Yes. The hardware should be an interface to the MMC/SD bus. No more.
Anything else will eventually create a maintenance nightmare.

> 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.
>
>   

I do not think we should cater to the laziness of hardware manufacturers
(even if we happen to work for said manufacturer). The hardware has no
business caring about the opcodes, only their semantics. If the spec.
keeps referring to specific opcodes then we have to try and see past
those through guessing and testing. I am convinced that the hardware has
no requirements on the SD spec on a protocol layer. Only the
specification suffers from such a limitation. Let's try and overcome
that limitation in the driver.

What it boils down to is that layers and abstractions are there for a
reason. Let's try and not to break them because of short gains since it
usually ends up costing us in the long run.

Rgds
Pierre