Re: [PATCH 1/6] Unified AVR32/AT91 MCI Driver

[Nicolas Ferre <nicolas.ferre@atmel.com>]

Hi,

First of all thank you for this splitting effort. It is definitively the
good way of presenting things and ease reading a lot.

Rob Emanuele :
> Unification of the atmel-mci driver to support the AT91 processors MCI
> interface.  The atmel-mci driver currently supports the AVR32 and this
> patch adds AT91 support.
> 
> To use this new driver on a at91 the platform driver for your board
> needs to updated.  See the following patch for an example of how to do
> that.
> 
> Please read the whole set, try it out, and comment.

Ok, I try to make my comments. I try to collect your ideas and sometimes
submit modified patches. Comment them back if the modifications I make
to your code seams no so good to you.

> ---
>  drivers/mmc/host/Kconfig     |   16 ++++++++++++----
>  drivers/mmc/host/atmel-mci.c |   33 +++++++++++++++++++++++++++++----
>  2 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index b4cf691..6e2f52b 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -124,6 +124,12 @@ config MMC_AU1X
> 
>  	  If unsure, say N.
> 
> +choice
> +	prompt "Atmel MMC Driver"
> +	default MMC_ATMELMCI
> +	help
> +	  Choose which driver to use for the Atmel MCI Silicon

I suggest :

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index b4cf691..e779049 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -124,6 +124,12 @@ config MMC_AU1X

 	  If unsure, say N.

+choice
+	prompt "Atmel SD/MMC Driver"
+	default MMC_ATMELMCI if AVR32
+	help
+	  Choose which driver to use for the Atmel MCI Silicon
+
 config MMC_AT91
 	tristate "AT91 SD/MMC Card Interface support"
 	depends on ARCH_AT91

It will keep present default configuration for both AVR32 and AT91.

[..]

> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index cf6a100..497dd51 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -2,11 +2,13 @@
>   * Atmel MultiMedia Card Interface driver
>   *
>   * Copyright (C) 2004-2008 Atmel Corporation
> + * Copyright (C) 2009 Rob Emanuele

I think you can add your copyright on a file that you create or copy and
modify from another file.

>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> +

Not needed. Can be the subject of a "presentation" only patch.

>  #include <linux/blkdev.h>
>  #include <linux/clk.h>
>  #include <linux/debugfs.h>
> @@ -30,11 +32,14 @@
>  #include <asm/io.h>
>  #include <asm/unaligned.h>
> 
> +#include <mach/cpu.h>
>  #include <mach/board.h>
> 
>  #include "atmel-mci-regs.h"
> 
> -#define ATMCI_DATA_ERROR_FLAGS	(MCI_DCRCE | MCI_DTOE | MCI_OVRE | MCI_UNRE)
> +#define ATMCI_DATA_ERROR_FLAGS	(MCI_RINDE | MCI_RDIRE | MCI_RCRCE  \
> +				 | MCI_RENDE | MCI_RTOE | MCI_DCRCE \
> +				 | MCI_DTOE | MCI_OVRE | MCI_UNRE)

We will have to consider what Haavard said: some are not data errors.
But indeed, if you experienced errors not beeing addressed, we have to
deal with them.

>  #define ATMCI_DMA_THRESHOLD	16
> 
>  enum {
> @@ -208,6 +213,18 @@ struct atmel_mci_slot {
>  	set_bit(event, &host->pending_events)
> 
>  /*
> + * Enable or disable features/registers based on
> + * whether the processor supports them
> + */
> +static bool mci_has_rwproof(void)
> +{
> +	if (cpu_is_at91sam9261() || cpu_is_at91rm9200())
> +		return false;
> +	else
> +		return true;
> +}
> +
> +/*

Ok for this.

>   * The debugfs stuff below is mostly optimized away when
>   * CONFIG_DEBUG_FS is not set.
>   */
> @@ -274,8 +291,12 @@ static void atmci_show_status_reg(struct seq_file *s,
>  		[3]	= "BLKE",
>  		[4]	= "DTIP",
>  		[5]	= "NOTBUSY",
> +		[6]	= "ENDRX",
> +		[7]	= "ENDTX",
>  		[8]	= "SDIOIRQA",
>  		[9]	= "SDIOIRQB",
> +		[14]	= "RXBUFF",
> +		[15]	= "TXBUFE",
>  		[16]	= "RINDE",
>  		[17]	= "RDIRE",
>  		[18]	= "RCRCE",

I add some MCI2 bits:

--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -276,8 +276,13 @@ static void atmci_show_status_reg(struct seq_file *s,
 		[3]	= "BLKE",
 		[4]	= "DTIP",
 		[5]	= "NOTBUSY",
+		[6]	= "ENDRX",
+		[7]	= "ENDTX",
 		[8]	= "SDIOIRQA",
 		[9]	= "SDIOIRQB",
+		[12]	= "SDIOWAIT",
+		[14]	= "RXBUFF",
+		[15]	= "TXBUFE",
 		[16]	= "RINDE",
 		[17]	= "RDIRE",
 		[18]	= "RCRCE",
@@ -285,6 +290,11 @@ static void atmci_show_status_reg(struct seq_file *s,
 		[20]	= "RTOE",
 		[21]	= "DCRCE",
 		[22]	= "DTOE",
+		[23]	= "CSTOE",
+		[24]	= "BLKOVRE",
+		[25]	= "DMADONE",
+		[26]	= "FIFOEMPTY",
+		[27]	= "XFRDONE",
 		[30]	= "OVRE",
 		[31]	= "UNRE",
 	};


> @@ -847,13 +868,15 @@ static void atmci_set_ios(struct mmc_host *mmc,
> struct mmc_ios *ios)
>  			clkdiv = 255;
>  		}
> 
> +		host->mode_reg = MCI_MR_CLKDIV(clkdiv);
> +
>  		/*
>  		 * WRPROOF and RDPROOF prevent overruns/underruns by
>  		 * stopping the clock when the FIFO is full/empty.
>  		 * This state is not expected to last for long.
>  		 */
> -		host->mode_reg = MCI_MR_CLKDIV(clkdiv) | MCI_MR_WRPROOF
> -					| MCI_MR_RDPROOF;
> +		if (mci_has_rwproof())
> +			host->mode_reg |= (MCI_MR_WRPROOF | MCI_MR_RDPROOF);
> 
>  		if (list_empty(&host->queue))
>  			mci_writel(host, MR, host->mode_reg);

Ok for this.

> @@ -1642,8 +1665,10 @@ static int __init atmci_probe(struct
> platform_device *pdev)
>  			nr_slots++;
>  	}
> 
> -	if (!nr_slots)
> +	if (!nr_slots) {
> +		printk(KERN_ERR "Atmel MCI controller init failed.  atmci_init_slot
> error or no slots with bus_width > 0.\n");
>  		goto err_init_slot;
> +	}
> 
>  	dev_info(&pdev->dev,
>  			"Atmel MCI controller at 0x%08lx irq %d, %u slots\n",

Ok (maybe in a different patch)

Best regards,
-- 
Nicolas Ferre