Re: [U-Boot] [PATCH v1] add mv_sdio driver for sheevaplug

Hi Gerald Sorry for late feedback, please find my comments inlined.

-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Gérald Kerma Sent: Sunday, August 29, 2010 4:07 AM To: u-boot@lists.denx.de Subject: [U-Boot] [PATCH v1] add mv_sdio driver for sheevaplug

Add mvsdio driver to Kirkwood SoC
Add SDIO support to SHEEVAPLUG
Fix environments for SHEEVAPLUG

You should break this patch into small pieces as per functionalities like- 1. Kirkwood: add more timer functions 2. Kirkwood: pre-requisite for SDIO driver support 3. mmc : Add SDIO driver for Marvell SoCs (Kirkwood) 4. Sheevaplug: Add MMC/SDIO support 5. Sheevaplug: Fix environments for <specify problem> 6. Guruplug: Add MMC/SDIO support (as Clint suggested- Optional)

Signed-off-by: Gérald Kerma geraker@gmail.com

Changes in v1:

• Fix errors from SD/SDHC detect
• Minor fixes in boot env

arch/arm/cpu/arm926ejs/kirkwood/timer.c | 23 + arch/arm/include/asm/arch-kirkwood/kirkwood.h | 1 + drivers/mmc/Makefile | 1 + drivers/mmc/mv_sdio.c | 747 +++++++++++++++++++++++++ drivers/mmc/mv_sdio.h | 296 ++++++++++ include/configs/sheevaplug.h | 65 ++- 6 files changed, 1124 insertions(+), 9 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/kirkwood/timer.c b/arch/arm/cpu/arm926ejs/kirkwood/timer.c index 2ec6a93..c8f932b 100644 --- a/arch/arm/cpu/arm926ejs/kirkwood/timer.c +++ b/arch/arm/cpu/arm926ejs/kirkwood/timer.c @@ -166,3 +166,26 @@ int timer_init(void)

return 0; }

+/*

• • This function is derived from PowerPC code (read timebase as long

long).

• • On ARM it just returns the timer value.
• */

+unsigned long long get_ticks(void) +{

• return get_timer(0);

+}

This is not needed, you can directly use get_timer(0) wherever needed

+/*

• • This function is derived from PowerPC code (timebase clock frequency).
• • On ARM it returns the number of timer ticks per second.
• */

+ulong get_tbclk (void) +{

• ulong tbclk;
• tbclk = CONFIG_SYS_HZ;
• return tbclk;

+}

Ditto,

diff --git a/arch/arm/include/asm/arch-kirkwood/kirkwood.h b/arch/arm/include/asm/arch-kirkwood/kirkwood.h index 0104418..4f9fe7e 100644 --- a/arch/arm/include/asm/arch-kirkwood/kirkwood.h +++ b/arch/arm/include/asm/arch-kirkwood/kirkwood.h @@ -60,6 +60,7 @@ #define KW_EGIGA0_BASE (KW_REGISTER(0x72000)) #define KW_EGIGA1_BASE (KW_REGISTER(0x76000)) #define KW_SATA_BASE (KW_REGISTER(0x80000)) +#define KW_SDIO_BASE (KW_REGISTER(0x90000))

/* Kirkwood Sata controller has two ports */ #define KW_SATA_PORT0_OFFSET 0x2000 diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile index 8dfd8a3..f1e7db6 100644 --- a/drivers/mmc/Makefile +++ b/drivers/mmc/Makefile @@ -31,6 +31,7 @@ COBJS-$(CONFIG_BFIN_SDH) += bfin_sdh.o COBJS-$(CONFIG_OMAP3_MMC) += omap3_mmc.o COBJS-$(CONFIG_FSL_ESDHC) += fsl_esdhc.o COBJS-$(CONFIG_MXC_MMC) += mxcmmc.o +COBJS-$(CONFIG_MV_SDIO) += mv_sdio.o

Please maintain order here

COBJS-$(CONFIG_PXA_MMC) += pxa_mmc.o COBJS-$(CONFIG_S5P_MMC) += s5p_mmc.o

diff --git a/drivers/mmc/mv_sdio.c b/drivers/mmc/mv_sdio.c new file mode 100644 index 0000000..aa8df10 --- /dev/null +++ b/drivers/mmc/mv_sdio.c @@ -0,0 +1,747 @@ +/*

• • (C) Copyright 2003
• • Kyle Harris, Nexus Technologies, Inc. kharris@nexus-tech.net
• • Copyright (C) 2010 Gérald Kerma geraker@gmail.com
• • See file CREDITS for list of people who contributed to this
• • project.
• • This program is free software; you can redistribute it and/or
• • modify it under the terms of the GNU General Public License as
• • published by the Free Software Foundation; either version 2 of
• • the License, or (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program; if not, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston,
• • MA 02111-1307 USA
• */

+#include <config.h> +#include <common.h> +#include <malloc.h> +#include <mmc.h> +#include "mv_sdio.h" +#include <asm/errno.h> +#include <part.h> +#include <asm/io.h>

Please use #ifdef CONFIG_KIRKWOOD here since driver name is generic and not Kirkwood specific

+#include <asm/arch/kirkwood.h>

+#ifdef CONFIG_MMC

+#define DRIVER_NAME "mv-sdio"

+//#define DEBUG

+//static int maxfreq = MVSD_CLOCKRATE_MAX; +//static int nodma;

Remove dead code, please post clean code

+static int is_sdhc;

+extern int fat_register_device(block_dev_desc_t *dev_desc, int part_no);

+static block_dev_desc_t mmc_dev;

+block_dev_desc_t * mmc_get_dev(int dev) +{

• return ((block_dev_desc_t *)&mmc_dev);

+}

+/*

• • FIXME needs to read cid and csd info to determine block size
• • and other parameters
• */

+static uchar mmc_buf[MMC_BLOCK_SIZE]; +static mv_mmc_csd_t mv_mmc_csd; +static int mmc_ready = 0;

+/* MMC_DEFAULT_RCA should probably be just 1, but this may break other code

• that expects it to be shifted. */

+static u_int16_t rca = 0;

+static u_int32_t mv_mmc_size(const struct mv_mmc_csd *csd) +{

• u_int32_t block_len, mult, blocknr;
• block_len = csd->read_bl_len << 12;
• mult = csd->c_size_mult1 << 8;
• blocknr = (csd->c_size+1) * mult;
• return blocknr * block_len;

+}

+static int isprint (unsigned char ch) +{

• if (ch >= 32 && ch < 127)

• return (1);
  

• return (0);

+}

+static int toprint(char *dst, char c) +{

• if (isprint(c)) {

• *dst = c;
  

• return 1;
  

• }
• return sprintf(dst,"\x%02x", c);

+}

+static void print_mmc_cid(mv_mmc_cid_t *cid) +{

• printf("MMC found. Card desciption is:\n");
• printf("Manufacturer ID = %02x%02x%02x\n",

• cid->id[0], cid->id[1], cid->id[2]);
  

• printf("HW/FW Revision = %x %x\n",cid->hwrev, cid->fwrev);
• cid->hwrev = cid->fwrev = 0; /* null terminate string */
• printf("Product Name = %s\n",cid->name);
• printf("Serial Number = %02x%02x%02x\n",

• cid->sn[0], cid->sn[1], cid->sn[2]);
  

• printf("Month = %d\n",cid->month);
• printf("Year = %d\n",1997 + cid->year);

+}

+static void print_sd_cid(mv_sd_cid_t *cid) +{

• int len;
• char tbuf[64];
• printf("SD%s found. Card desciption is:\n", is_sdhc?"HC":"");
• len = 0;
• len += toprint(&tbuf[len], cid->oid_0);
• len += toprint(&tbuf[len], cid->oid_1);
• tbuf[len] = 0;
• printf("Manufacturer: 0x%02x, OEM "%s"\n",

•    cid->mid, tbuf);
  

• len = 0;
• len += toprint(&tbuf[len], cid->pnm_0);
• len += toprint(&tbuf[len], cid->pnm_1);
• len += toprint(&tbuf[len], cid->pnm_2);
• len += toprint(&tbuf[len], cid->pnm_3);
• len += toprint(&tbuf[len], cid->pnm_4);
• tbuf[len] = 0;
• printf("Product name: "%s", revision %d.%d\n",

• tbuf,
  

•    cid->prv >> 4, cid->prv & 15);
  

• printf("Serial number: %u\n",

•    cid->psn_0 << 24 | cid->psn_1 << 16 | cid->psn_2 << 8 |
  

•    cid->psn_3);
  

• printf("Manufacturing date: %d/%d\n",

•    cid->mdt_1 & 15,
  

•    2000+((cid->mdt_0 & 15) << 4)+((cid->mdt_1 & 0xf0) >> 4));
  

• printf("CRC: 0x%02x, b0 = %d\n",

•    cid->crc >> 1, cid->crc & 1);
  

+}

+static void mvsdio_set_clock(unsigned int clock) +{

• unsigned int m;
• m = MVSDMMC_BASE_FAST_CLOCK/(2*clock) - 1;
• debug("mvsdio_set_clock: dividor = 0x%x clock=%d\n",

•       m, clock);
  

• SDIO_REG_WRITE32(SDIO_CLK_DIV, m & 0x7ff);
• if (isprint(1))
• udelay(10*1000);

+}

+/****************************************************/ +static ulong * mv_mmc_cmd(ulong cmd, ulong arg, ushort xfermode, ushort resptype, ushort waittype) +/****************************************************/ +{

• static ulong resp[4];
• ushort done ;
• int err = 0 ;
• ulong curr, start, diff, hz;
• ushort response[8], resp_indx = 0;
• debug("mv_mmc_cmd %x, arg: %x,xfer: %x,resp: %x, wait : %x\n"
• , (unsigned int)cmd, (unsigned int)arg, xfermode, resptype,

waittype);

• //clear status

Please no c++ style of comments

• SDIO_REG_WRITE16(SDIO_NOR_INTR_STATUS, 0xffff);
• SDIO_REG_WRITE16(SDIO_ERR_INTR_STATUS, 0xffff);

Please use reads/writes everywhere

...snip...

diff --git a/drivers/mmc/mv_sdio.h b/drivers/mmc/mv_sdio.h new file mode 100644 index 0000000..bb7d39c --- /dev/null +++ b/drivers/mmc/mv_sdio.h @@ -0,0 +1,296 @@ +/*

• • Copyright (C) 2008 Marvell Semiconductors, All Rights Reserved.
• • Copyright (C) 2010 Gérald Kerma gerald.kerma@gk2.net
• • 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.
• */

+#ifndef _MVSDIO_INCLUDE +#define _MVSDIO_INCLUDE

+#define SDIO_REG(x) (KW_SDIO_BASE + (x))

+#define SDIO_REG_WRITE32(offset,value) writel(value,SDIO_REG(offset)) +#define SDIO_REG_READ32(offset) readl(SDIO_REG(offset))

+#define SDIO_REG_WRITE16(offset,value) writew(value,SDIO_REG(offset)) +#define SDIO_REG_READ16(offset) readw(SDIO_REG(offset))

It makes more sense to use readl/writel macros directly, the used macros increases code size.

+#define MVSDMMC_DMA_SIZE 65536 +#define MVSDMMC_CMD_TIMEOUT 2 /* 100 usec*/

+/*

• • Clock rates
• */

+#define MVSD_CLOCKRATE_MAX 50000000 +#define MVSD_BASE_DIV_MAX 0x7ff

+#define CONFIG_SYS_MMC_CLK_PP 25000000

+/*

• • The base MMC clock rate
• */

+#define MVSDMMC_CLOCKRATE_MIN 100000 +#define MVSDMMC_CLOCKRATE_MAX MVSD_CLOCKRATE_MAX +#define MVSDMMC_BASE_FAST_CLOCK CONFIG_SYS_TCLK

+/*

• • SDIO register
• */

Please use c-struct for register definations

+#define SDIO_SYS_ADDR_LOW 0x000 +#define SDIO_SYS_ADDR_HI 0x004 +#define SDIO_BLK_SIZE 0x008 +#define SDIO_BLK_COUNT 0x00c +#define SDIO_ARG_LOW 0x010 +#define SDIO_ARG_HI 0x014 +#define SDIO_XFER_MODE 0x018 +#define SDIO_CMD 0x01c +#define SDIO_RSP(i) (0x020 + ((i)<<2)) +#define SDIO_RSP0 0x020 +#define SDIO_RSP1 0x024 +#define SDIO_RSP2 0x028 +#define SDIO_RSP3 0x02c +#define SDIO_RSP4 0x030 +#define SDIO_RSP5 0x034 +#define SDIO_RSP6 0x038 +#define SDIO_RSP7 0x03c +#define SDIO_BUF_DATA_PORT 0x040 +#define SDIO_RSVED 0x044

+#define SDIO_PRESENT_STATE0 0x048 +#define SDIO_PRESENT_STATE1 0x04c +#define SDIO_HOST_CTRL 0x050 +#define SDIO_BLK_GAP_CTRL 0x054 +#define SDIO_CLK_CTRL 0x058 +#define SDIO_SW_RESET 0x05c +#define SDIO_NOR_INTR_STATUS 0x060 +#define SDIO_ERR_INTR_STATUS 0x064 +#define SDIO_NOR_STATUS_EN 0x068 +#define SDIO_ERR_STATUS_EN 0x06c +#define SDIO_NOR_INTR_EN 0x070 +#define SDIO_ERR_INTR_EN 0x074 +#define SDIO_AUTOCMD12_ERR_STATUS 0x078 +#define SDIO_CURR_BYTE_LEFT 0x07c +#define SDIO_CURR_BLK_LEFT 0x080 +#define SDIO_AUTOCMD12_ARG_LOW 0x084 +#define SDIO_AUTOCMD12_ARG_HI 0x088 +#define SDIO_AUTOCMD12_INDEX 0x08c +#define SDIO_AUTO_RSP(i) (0x090 + ((i)<<2)) +#define SDIO_AUTO_RSP0 0x090 +#define SDIO_AUTO_RSP1 0x094 +#define SDIO_AUTO_RSP2 0x098 +#define SDIO_CLK_DIV 0x128

Overall, the code looks too bulky Please try to make use of CONFIG_GENERIC_MMC, check if you can use mmc.c and reduce mv_mmc.c code size and complications. Ref: omap4_sdp4430 board

Regards.. Prafulla . .