Hi Gerald
Sorry for late feedback, please find my comments inlined.
> -----Original Message-----
> From: u-boot-boun...@lists.denx.de [mailto:u-boot-boun...@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 <gera...@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. khar...@nexus-tech.net
> + * Copyright (C) 2010 Gérald Kerma <gera...@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.ke...@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 . .
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
