[U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
This patch adds MMC/SD support to the S3C2410 SoC code in u-boot
Signed-off-by: Harald Welte laforge@openmoko.org
Index: u-boot/cpu/arm920t/s3c24x0/Makefile =================================================================== --- u-boot.orig/cpu/arm920t/s3c24x0/Makefile 2007-02-16 23:42:19.000000000 +0100 +++ u-boot/cpu/arm920t/s3c24x0/Makefile 2007-02-16 23:42:19.000000000 +0100 @@ -26,7 +26,7 @@ LIB = $(obj)lib$(SOC).a
COBJS = i2c.o interrupts.o serial.o speed.o \ - usb_ohci.o nand_read.o nand.o cmd_s3c2410.o + usb_ohci.o nand_read.o nand.o mmc.o cmd_s3c2410.o
SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(SOBJS) $(COBJS)) Index: u-boot/cpu/arm920t/s3c24x0/mmc.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot/cpu/arm920t/s3c24x0/mmc.c 2007-02-16 23:43:28.000000000 +0100 @@ -0,0 +1,504 @@ +/* + * u-boot S3C2410 MMC/SD card driver + * (C) Copyright 2006 by OpenMoko, Inc. + * Author: Harald Welte laforge@openmoko.org + * + * based on u-boot pxa MMC driver and linux/drivers/mmc/s3c2410mci.c + * (C) 2005-2005 Thomas Kleffel + * + * 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 <mmc.h> +#include <asm/errno.h> +#include <asm/io.h> +#include <s3c2410.h> +#include <part.h> + +#ifdef CONFIG_MMC + +#define CONFIG_MMC_WIDE + +//#define MMC_DEBUG + +#ifdef MMC_DEBUG +#define DEBUGP printf +#else +#define DEBUGP(x, args ...) do { } while(0) +#endif + +static S3C2410_SDI *sdi; + +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 mmc_csd_t mmc_csd; +static int mmc_ready = 0; +static int wide = 0; + + +#define CMD_F_RESP 0x01 +#define CMD_F_RESP_LONG 0x02 + +static u_int32_t * +/****************************************************/ +mmc_cmd(ushort cmd, ulong arg, ushort flags) +/****************************************************/ +{ + static u_int32_t resp[5]; + ulong status; + int i; + + u_int32_t ccon, csta; + u_int32_t csta_rdy_bit = S3C2410_SDICMDSTAT_CMDSENT; + + memset(resp, 0, sizeof(resp)); + + DEBUGP("mmc_cmd CMD%d arg=0x%08x flags=%x\n", cmd, arg, flags); + + sdi->SDICSTA = 0xffffffff; + sdi->SDIDSTA = 0xffffffff; + sdi->SDIFSTA = 0xffffffff; + + sdi->SDICARG = arg; + + ccon = cmd & S3C2410_SDICMDCON_INDEX; + ccon |= S3C2410_SDICMDCON_SENDERHOST|S3C2410_SDICMDCON_CMDSTART; + + if (flags & CMD_F_RESP) { + ccon |= S3C2410_SDICMDCON_WAITRSP; + csta_rdy_bit = S3C2410_SDICMDSTAT_RSPFIN; /* 1 << 9 */ + } + + if (flags & CMD_F_RESP_LONG) + ccon |= S3C2410_SDICMDCON_LONGRSP; + + sdi->SDICCON = ccon; + + while (1) { + csta = sdi->SDICSTA; + if (csta & csta_rdy_bit) + break; + if (csta & S3C2410_SDICMDSTAT_CMDTIMEOUT) { + printf("===============> MMC CMD Timeout\n"); + sdi->SDICSTA |= S3C2410_SDICMDSTAT_CMDTIMEOUT; + break; + } + } + + DEBUGP("final MMC CMD status 0x%x\n", csta); + + sdi->SDICSTA |= csta_rdy_bit; + + if (flags & CMD_F_RESP) { + resp[0] = sdi->SDIRSP0; + resp[1] = sdi->SDIRSP1; + resp[2] = sdi->SDIRSP2; + resp[3] = sdi->SDIRSP3; + } + + return resp; +} + +#define FIFO_FILL(host) ((host->SDIFSTA & S3C2410_SDIFSTA_COUNTMASK) >> 2) + +static int +/****************************************************/ +mmc_block_read(uchar *dst, ulong src, ulong len) +/****************************************************/ +{ + u_int32_t dcon, fifo; + u_int32_t *dst_u32 = (u_int32_t *)dst; + u_int32_t *resp; + + if (len == 0) { + return 0; + } + + DEBUGP("mmc_block_rd dst %lx src %lx len %d\n", (ulong)dst, src, len); + + /* set block len */ + resp = mmc_cmd(MMC_CMD_SET_BLOCKLEN, len, CMD_F_RESP); + sdi->SDIBSIZE = len; + + //sdi->SDIPRE = 0xff; + + /* setup data */ + dcon = (len >> 9) & S3C2410_SDIDCON_BLKNUM_MASK; + dcon |= S3C2410_SDIDCON_BLOCKMODE; + dcon |= S3C2410_SDIDCON_RXAFTERCMD|S3C2410_SDIDCON_XFER_RXSTART; + if (wide) + dcon |= S3C2410_SDIDCON_WIDEBUS; + sdi->SDIDCON = dcon; + + /* send read command */ + resp = mmc_cmd(MMC_CMD_READ_BLOCK, src, CMD_F_RESP); + + while (len > 0) { + u_int32_t sdidsta = sdi->SDIDSTA; + fifo = FIFO_FILL(sdi); + if (sdidsta & (S3C2410_SDIDSTA_FIFOFAIL| + S3C2410_SDIDSTA_CRCFAIL| + S3C2410_SDIDSTA_RXCRCFAIL| + S3C2410_SDIDSTA_DATATIMEOUT)) { + printf("mmc_block_read: err SDIDSTA=0x%08x\n", sdidsta); + return -EIO; + } + + while (fifo--) { + //DEBUGP("dst_u32 = 0x%08x\n", dst_u32); + *(dst_u32++) = sdi->SDIDAT; + if (len >= 4) + len -= 4; + else { + len = 0; + break; + } + } + } + + DEBUGP("waiting for SDIDSTA (currently 0x%08x\n", sdi->SDIDSTA); + while (!(sdi->SDIDSTA & (1 << 4))) {} + DEBUGP("done waiting for SDIDSTA (currently 0x%08x\n", sdi->SDIDSTA); + + sdi->SDIDCON = 0; + + if (!(sdi->SDIDSTA & S3C2410_SDIDSTA_XFERFINISH)) + DEBUGP("mmc_block_read; transfer not finished!\n"); + + return 0; +} + +static int +/****************************************************/ +mmc_block_write(ulong dst, uchar *src, int len) +/****************************************************/ +{ + printf("MMC block write not yet supported on S3C2410!\n"); + return -1; +} + + +int +/****************************************************/ +mmc_read(ulong src, uchar *dst, int size) +/****************************************************/ +{ + ulong end, part_start, part_end, part_len, aligned_start, aligned_end; + ulong mmc_block_size, mmc_block_address; + + if (size == 0) { + return 0; + } + + if (!mmc_ready) { + printf("Please initialize the MMC first\n"); + return -1; + } + + mmc_block_size = MMC_BLOCK_SIZE; + mmc_block_address = ~(mmc_block_size - 1); + + src -= CFG_MMC_BASE; + end = src + size; + part_start = ~mmc_block_address & src; + part_end = ~mmc_block_address & end; + aligned_start = mmc_block_address & src; + aligned_end = mmc_block_address & end; + + /* all block aligned accesses */ + DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if (part_start) { + part_len = mmc_block_size - part_start; + DEBUGP("ps src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if ((mmc_block_read(mmc_buf, aligned_start, mmc_block_size)) < 0) { + return -1; + } + memcpy(dst, mmc_buf+part_start, part_len); + dst += part_len; + src += part_len; + } + DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + for (; src < aligned_end; src += mmc_block_size, dst += mmc_block_size) { + DEBUGP("al src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if ((mmc_block_read((uchar *)(dst), src, mmc_block_size)) < 0) { + return -1; + } + } + DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if (part_end && src < end) { + DEBUGP("pe src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if ((mmc_block_read(mmc_buf, aligned_end, mmc_block_size)) < 0) { + return -1; + } + memcpy(dst, mmc_buf, part_end); + } + return 0; +} + +int +/****************************************************/ +mmc_write(uchar *src, ulong dst, int size) +/****************************************************/ +{ + ulong end, part_start, part_end, part_len, aligned_start, aligned_end; + ulong mmc_block_size, mmc_block_address; + + if (size == 0) { + return 0; + } + + if (!mmc_ready) { + printf("Please initialize the MMC first\n"); + return -1; + } + + mmc_block_size = MMC_BLOCK_SIZE; + mmc_block_address = ~(mmc_block_size - 1); + + dst -= CFG_MMC_BASE; + end = dst + size; + part_start = ~mmc_block_address & dst; + part_end = ~mmc_block_address & end; + aligned_start = mmc_block_address & dst; + aligned_end = mmc_block_address & end; + + /* all block aligned accesses */ + DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if (part_start) { + part_len = mmc_block_size - part_start; + DEBUGP("ps src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + (ulong)src, dst, end, part_start, part_end, aligned_start, aligned_end); + if ((mmc_block_read(mmc_buf, aligned_start, mmc_block_size)) < 0) { + return -1; + } + memcpy(mmc_buf+part_start, src, part_len); + if ((mmc_block_write(aligned_start, mmc_buf, mmc_block_size)) < 0) { + return -1; + } + dst += part_len; + src += part_len; + } + DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + for (; dst < aligned_end; src += mmc_block_size, dst += mmc_block_size) { + DEBUGP("al src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if ((mmc_block_write(dst, (uchar *)src, mmc_block_size)) < 0) { + return -1; + } + } + DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if (part_end && dst < end) { + DEBUGP("pe src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n", + src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end); + if ((mmc_block_read(mmc_buf, aligned_end, mmc_block_size)) < 0) { + return -1; + } + memcpy(mmc_buf, src, part_end); + if ((mmc_block_write(aligned_end, mmc_buf, mmc_block_size)) < 0) { + return -1; + } + } + return 0; +} + +ulong +/****************************************************/ +mmc_bread(int dev_num, ulong blknr, ulong blkcnt, ulong *dst) +/****************************************************/ +{ + int mmc_block_size = MMC_BLOCK_SIZE; + ulong src = blknr * mmc_block_size + CFG_MMC_BASE; + + mmc_read(src, (uchar *)dst, blkcnt*mmc_block_size); + return blkcnt; +} + +static u_int16_t rca = MMC_DEFAULT_RCA; + +static u_int32_t mmc_size(const struct 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; +} + +int +/****************************************************/ +mmc_init(int verbose) +/****************************************************/ +{ + int retries, rc = -ENODEV; + int is_sd = 0; + u_int32_t *resp; + S3C24X0_CLOCK_POWER * const clk_power = S3C24X0_GetBase_CLOCK_POWER(); + + sdi = S3C2410_GetBase_SDI(); + + DEBUGP("mmc_init(PCLK=%u)\n", get_PCLK()); + + clk_power->CLKCON |= (1 << 9); + + /* S3C2410 has some bug that prevents reliable operation at higher speed */ + //sdi->SDIPRE = 0x3e; /* SDCLK = PCLK/2 / (SDIPRE+1) = 396kHz */ + sdi->SDIPRE = 0x02; /* SDCLK = PCLK/2 / (SDIPRE+1) = 396kHz */ + sdi->SDIBSIZE = 512; + sdi->SDIDTIMER = 0xffff; + sdi->SDIIMSK = 0x0; + sdi->SDICON = S3C2410_SDICON_FIFORESET|S3C2440_SDICON_MMCCLOCK; + udelay(125000); /* FIXME: 74 SDCLK cycles */ + + mmc_csd.c_size = 0; + + /* reset */ + retries = 10; + resp = mmc_cmd(MMC_CMD_RESET, 0, 0); + + printf("trying to detect SD Card...\n"); + while (retries--) { + int i; + udelay(100000); + resp = mmc_cmd(55, 0x00000000, CMD_F_RESP); + resp = mmc_cmd(41, 0x00300000, CMD_F_RESP); + + if (resp[0] & (1 << 31)) { + is_sd = 1; + break; + } + } + + if (retries == 0 && !is_sd) { + retries = 10; + printf("failed to detect SD Card, trying MMC\n"); + resp = mmc_cmd(MMC_CMD_SEND_OP_COND, 0x00ffc000, CMD_F_RESP); + while (retries-- && resp && !(resp[4] & 0x80)) { + DEBUGP("resp %x %x\n", resp[0], resp[1]); + udelay(50); + resp = mmc_cmd(1, 0x00ffff00, CMD_F_RESP); + } + } + + /* try to get card id */ + resp = mmc_cmd(MMC_CMD_ALL_SEND_CID, 0, CMD_F_RESP|CMD_F_RESP_LONG); + if (resp) { + /* TODO configure mmc driver depending on card attributes */ + mmc_cid_t *cid = (mmc_cid_t *)resp; + if (verbose) { + 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); + } + /* fill in device description */ + mmc_dev.if_type = IF_TYPE_MMC; + mmc_dev.part_type = PART_TYPE_DOS; + mmc_dev.dev = 0; + mmc_dev.lun = 0; + mmc_dev.type = 0; + /* FIXME fill in the correct size (is set to 32MByte) */ + mmc_dev.blksz = 512; + mmc_dev.lba = 0x10000; + sprintf(mmc_dev.vendor,"Man %02x%02x%02x Snr %02x%02x%02x", + cid->id[0], cid->id[1], cid->id[2], + cid->sn[0], cid->sn[1], cid->sn[2]); + sprintf(mmc_dev.product,"%s",cid->name); + sprintf(mmc_dev.revision,"%x %x",cid->hwrev, cid->fwrev); + mmc_dev.removable = 0; + mmc_dev.block_read = mmc_bread; + + /* MMC exists, get CSD too */ + resp = mmc_cmd(MMC_CMD_SET_RCA, MMC_DEFAULT_RCA, CMD_F_RESP); + if (is_sd) + rca = resp[0] >> 16; + + resp = mmc_cmd(MMC_CMD_SEND_CSD, rca<<16, CMD_F_RESP|CMD_F_RESP_LONG); + if (resp) { + mmc_csd_t *csd = (mmc_csd_t *)resp; + memcpy(&mmc_csd, csd, sizeof(csd)); + rc = 0; + mmc_ready = 1; + /* FIXME add verbose printout for csd */ + printf("READ_BL_LEN=%u, C_SIZE_MULT=%u, C_SIZE=%u\n", + csd->read_bl_len, csd->c_size_mult1, csd->c_size); + printf("size = %u\n", mmc_size(csd)); + } + } + + resp = mmc_cmd(MMC_CMD_SELECT_CARD, rca<<16, CMD_F_RESP); + +#ifdef CONFIG_MMC_WIDE + if (is_sd) { + resp = mmc_cmd(55, rca<<16, CMD_F_RESP); + resp = mmc_cmd(6, 0x02, CMD_F_RESP); + wide = 1; + } +#endif + + fat_register_device(&mmc_dev,1); /* partitions start counting with 1 */ + + return rc; +} + +int +mmc_ident(block_dev_desc_t *dev) +{ + return 0; +} + +int +mmc2info(ulong addr) +{ + /* FIXME hard codes to 32 MB device */ + if (addr >= CFG_MMC_BASE && addr < CFG_MMC_BASE + 0x02000000) { + return 1; + } + return 0; +} + +#endif /* CONFIG_MMC */ Index: u-boot/include/asm-arm/arch-s3c24x0/mmc.h =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot/include/asm-arm/arch-s3c24x0/mmc.h 2007-02-16 23:42:19.000000000 +0100 @@ -0,0 +1,112 @@ +/* + * linux/drivers/mmc/mmc_pxa.h + * + * Author: Vladimir Shebordaev, Igor Oblakov + * Copyright: MontaVista Software Inc. + * + * $Id: mmc_pxa.h,v 0.3.1.6 2002/09/25 19:25:48 ted Exp ted $ + * + * 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 __MMC_PXA_P_H__ +#define __MMC_PXA_P_H__ + +#include <asm/arch/regs-sdi.h> + +#define MMC_DEFAULT_RCA (1<<16) + +#define MMC_BLOCK_SIZE 512 +#define MMC_CMD_RESET 0 +#define MMC_CMD_SEND_OP_COND 1 +#define MMC_CMD_ALL_SEND_CID 2 +#define MMC_CMD_SET_RCA 3 +#define MMC_CMD_SELECT_CARD 7 +#define MMC_CMD_SEND_CSD 9 +#define MMC_CMD_SEND_CID 10 +#define MMC_CMD_SEND_STATUS 13 +#define MMC_CMD_SET_BLOCKLEN 16 +#define MMC_CMD_READ_BLOCK 17 +#define MMC_CMD_RD_BLK_MULTI 18 +#define MMC_CMD_WRITE_BLOCK 24 + +#define MMC_MAX_BLOCK_SIZE 512 + +#define MMC_R1_IDLE_STATE 0x01 +#define MMC_R1_ERASE_STATE 0x02 +#define MMC_R1_ILLEGAL_CMD 0x04 +#define MMC_R1_COM_CRC_ERR 0x08 +#define MMC_R1_ERASE_SEQ_ERR 0x01 +#define MMC_R1_ADDR_ERR 0x02 +#define MMC_R1_PARAM_ERR 0x04 + +#define MMC_R1B_WP_ERASE_SKIP 0x0002 +#define MMC_R1B_ERR 0x0004 +#define MMC_R1B_CC_ERR 0x0008 +#define MMC_R1B_CARD_ECC_ERR 0x0010 +#define MMC_R1B_WP_VIOLATION 0x0020 +#define MMC_R1B_ERASE_PARAM 0x0040 +#define MMC_R1B_OOR 0x0080 +#define MMC_R1B_IDLE_STATE 0x0100 +#define MMC_R1B_ERASE_RESET 0x0200 +#define MMC_R1B_ILLEGAL_CMD 0x0400 +#define MMC_R1B_COM_CRC_ERR 0x0800 +#define MMC_R1B_ERASE_SEQ_ERR 0x1000 +#define MMC_R1B_ADDR_ERR 0x2000 +#define MMC_R1B_PARAM_ERR 0x4000 + +typedef struct mmc_cid +{ +/* FIXME: BYTE_ORDER */ + uchar year:4, + month:4; + uchar sn[3]; + uchar fwrev:4, + hwrev:4; + uchar name[6]; + uchar id[3]; +} mmc_cid_t; + +typedef struct mmc_csd +{ + uchar ecc:2, + file_format:2, + tmp_write_protect:1, + perm_write_protect:1, + copy:1, + file_format_grp:1; + uint64_t content_prot_app:1, + rsvd3:4, + write_bl_partial:1, + write_bl_len:4, + r2w_factor:3, + default_ecc:2, + wp_grp_enable:1, + wp_grp_size:5, + erase_grp_mult:5, + erase_grp_size:5, + c_size_mult1:3, + vdd_w_curr_max:3, + vdd_w_curr_min:3, + vdd_r_curr_max:3, + vdd_r_curr_min:3, + c_size:12, + rsvd2:2, + dsr_imp:1, + read_blk_misalign:1, + write_blk_misalign:1, + read_bl_partial:1; + + ushort read_bl_len:4, + ccc:12; + uchar tran_speed; + uchar nsac; + uchar taac; + uchar rsvd1:2, + spec_vers:4, + csd_structure:2; +} mmc_csd_t; + + +#endif /* __MMC_PXA_P_H__ */ Index: u-boot/include/asm-arm/arch-s3c24x0/regs-sdi.h =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot/include/asm-arm/arch-s3c24x0/regs-sdi.h 2007-02-16 23:42:19.000000000 +0100 @@ -0,0 +1,110 @@ +/* linux/include/asm/arch-s3c2410/regs-sdi.h + * + * Copyright (c) 2004 Simtec Electronics linux@simtec.co.uk + * http://www.simtec.co.uk/products/SWLINUX/ + * + * 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. + * + * S3C2410 MMC/SDIO register definitions + * + * Changelog: + * 18-Aug-2004 Ben Dooks Created initial file + * 29-Nov-2004 Koen Martens Added some missing defines, fixed duplicates + * 29-Nov-2004 Ben Dooks Updated Koen's patch +*/ + +#ifndef __ASM_ARM_REGS_SDI +#define __ASM_ARM_REGS_SDI "regs-sdi.h" + +#define S3C2440_SDICON_SDRESET (1<<8) +#define S3C2440_SDICON_MMCCLOCK (1<<5) +#define S3C2410_SDICON_BYTEORDER (1<<4) +#define S3C2410_SDICON_SDIOIRQ (1<<3) +#define S3C2410_SDICON_RWAITEN (1<<2) +#define S3C2410_SDICON_FIFORESET (1<<1) +#define S3C2410_SDICON_CLOCKTYPE (1<<0) + +#define S3C2410_SDICMDCON_ABORT (1<<12) +#define S3C2410_SDICMDCON_WITHDATA (1<<11) +#define S3C2410_SDICMDCON_LONGRSP (1<<10) +#define S3C2410_SDICMDCON_WAITRSP (1<<9) +#define S3C2410_SDICMDCON_CMDSTART (1<<8) +#define S3C2410_SDICMDCON_SENDERHOST (1<<6) +#define S3C2410_SDICMDCON_INDEX (0x3f) + +#define S3C2410_SDICMDSTAT_CRCFAIL (1<<12) +#define S3C2410_SDICMDSTAT_CMDSENT (1<<11) +#define S3C2410_SDICMDSTAT_CMDTIMEOUT (1<<10) +#define S3C2410_SDICMDSTAT_RSPFIN (1<<9) +#define S3C2410_SDICMDSTAT_XFERING (1<<8) +#define S3C2410_SDICMDSTAT_INDEX (0xff) + +#define S3C2440_SDIDCON_DS_BYTE (0<<22) +#define S3C2440_SDIDCON_DS_HALFWORD (1<<22) +#define S3C2440_SDIDCON_DS_WORD (2<<22) +#define S3C2410_SDIDCON_IRQPERIOD (1<<21) +#define S3C2410_SDIDCON_TXAFTERRESP (1<<20) +#define S3C2410_SDIDCON_RXAFTERCMD (1<<19) +#define S3C2410_SDIDCON_BUSYAFTERCMD (1<<18) +#define S3C2410_SDIDCON_BLOCKMODE (1<<17) +#define S3C2410_SDIDCON_WIDEBUS (1<<16) +#define S3C2410_SDIDCON_DMAEN (1<<15) +#define S3C2410_SDIDCON_STOP (1<<14) +#define S3C2440_SDIDCON_DATSTART (1<<14) +#define S3C2410_SDIDCON_DATMODE (3<<12) +#define S3C2410_SDIDCON_BLKNUM (0x7ff) + +/* constants for S3C2410_SDIDCON_DATMODE */ +#define S3C2410_SDIDCON_XFER_READY (0<<12) +#define S3C2410_SDIDCON_XFER_CHKSTART (1<<12) +#define S3C2410_SDIDCON_XFER_RXSTART (2<<12) +#define S3C2410_SDIDCON_XFER_TXSTART (3<<12) + +#define S3C2410_SDIDCON_BLKNUM_MASK (0xFFF) +#define S3C2410_SDIDCNT_BLKNUM_SHIFT (12) + +#define S3C2410_SDIDSTA_RDYWAITREQ (1<<10) +#define S3C2410_SDIDSTA_SDIOIRQDETECT (1<<9) +#define S3C2410_SDIDSTA_FIFOFAIL (1<<8) /* reserved on 2440 */ +#define S3C2410_SDIDSTA_CRCFAIL (1<<7) +#define S3C2410_SDIDSTA_RXCRCFAIL (1<<6) +#define S3C2410_SDIDSTA_DATATIMEOUT (1<<5) +#define S3C2410_SDIDSTA_XFERFINISH (1<<4) +#define S3C2410_SDIDSTA_BUSYFINISH (1<<3) +#define S3C2410_SDIDSTA_SBITERR (1<<2) /* reserved on 2410a/2440 */ +#define S3C2410_SDIDSTA_TXDATAON (1<<1) +#define S3C2410_SDIDSTA_RXDATAON (1<<0) + +#define S3C2440_SDIFSTA_FIFORESET (1<<16) +#define S3C2440_SDIFSTA_FIFOFAIL (3<<14) /* 3 is correct (2 bits) */ +#define S3C2410_SDIFSTA_TFDET (1<<13) +#define S3C2410_SDIFSTA_RFDET (1<<12) +#define S3C2410_SDIFSTA_TFHALF (1<<11) +#define S3C2410_SDIFSTA_TFEMPTY (1<<10) +#define S3C2410_SDIFSTA_RFLAST (1<<9) +#define S3C2410_SDIFSTA_RFFULL (1<<8) +#define S3C2410_SDIFSTA_RFHALF (1<<7) +#define S3C2410_SDIFSTA_COUNTMASK (0x7f) + +#define S3C2410_SDIIMSK_RESPONSECRC (1<<17) +#define S3C2410_SDIIMSK_CMDSENT (1<<16) +#define S3C2410_SDIIMSK_CMDTIMEOUT (1<<15) +#define S3C2410_SDIIMSK_RESPONSEND (1<<14) +#define S3C2410_SDIIMSK_READWAIT (1<<13) +#define S3C2410_SDIIMSK_SDIOIRQ (1<<12) +#define S3C2410_SDIIMSK_FIFOFAIL (1<<11) +#define S3C2410_SDIIMSK_CRCSTATUS (1<<10) +#define S3C2410_SDIIMSK_DATACRC (1<<9) +#define S3C2410_SDIIMSK_DATATIMEOUT (1<<8) +#define S3C2410_SDIIMSK_DATAFINISH (1<<7) +#define S3C2410_SDIIMSK_BUSYFINISH (1<<6) +#define S3C2410_SDIIMSK_SBITERR (1<<5) /* reserved 2440/2410a */ +#define S3C2410_SDIIMSK_TXFIFOHALF (1<<4) +#define S3C2410_SDIIMSK_TXFIFOEMPTY (1<<3) +#define S3C2410_SDIIMSK_RXFIFOLAST (1<<2) +#define S3C2410_SDIIMSK_RXFIFOFULL (1<<1) +#define S3C2410_SDIIMSK_RXFIFOHALF (1<<0) + +#endif /* __ASM_ARM_REGS_SDI */ Index: u-boot/include/s3c24x0.h =================================================================== --- u-boot.orig/include/s3c24x0.h 2007-02-16 23:42:02.000000000 +0100 +++ u-boot/include/s3c24x0.h 2007-02-16 23:42:19.000000000 +0100 @@ -637,13 +637,7 @@ S3C24X0_REG32 SDIDCNT; S3C24X0_REG32 SDIDSTA; S3C24X0_REG32 SDIFSTA; -#ifdef __BIG_ENDIAN - S3C24X0_REG8 res[3]; - S3C24X0_REG8 SDIDAT; -#else - S3C24X0_REG8 SDIDAT; - S3C24X0_REG8 res[3]; -#endif + S3C24X0_REG32 SDIDAT; S3C24X0_REG32 SDIIMSK; } /*__attribute__((__packed__))*/ S3C2410_SDI;
@@ -1123,11 +1117,7 @@ #define rSDIDatCnt (*(volatile unsigned *)0x5A000030) #define rSDIDatSta (*(volatile unsigned *)0x5A000034) #define rSDIFSTA (*(volatile unsigned *)0x5A000038) -#ifdef __BIG_ENDIAN -#define rSDIDAT (*(volatile unsigned char *)0x5A00003F) -#else -#define rSDIDAT (*(volatile unsigned char *)0x5A00003C) -#endif +#define rSDIDAT (*(volatile unsigned *)0x5A00003C) #define rSDIIntMsk (*(volatile unsigned *)0x5A000040)
#endif
On 2/17/07, Harald Welte laforge@openmoko.org wrote:
This patch adds MMC/SD support to the S3C2410 SoC code in u-boot
/* FIXME fill in the correct size (is set to 32MByte) */mmc_dev.blksz = 512;mmc_dev.lba = 0x10000;
So...what happens if the card really isn't exactly 32 MB?
Index: u-boot/include/asm-arm/arch-s3c24x0/mmc.h
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot/include/asm-arm/arch-s3c24x0/mmc.h 2007-02-16 23:42:19.000000000 +0100 @@ -0,0 +1,112 @@ +/*
- linux/drivers/mmc/mmc_pxa.h
This looks like it's been copied from pxa with few modifications ;)
I wonder if we could perhaps create a common header file for the mmc protocol definitions instead of duplicating them all over the place? This file consists entirely of constants and data structures from the MMC spec, and isn't really arch-specific at all.
I have a patch that adds support for the mmc controller on the AT32AP7000 chip, and which adds the exact same definitions all over again to the AVR32 include directory. The same driver should work on several AT91 chips, which means that we need to add them to the at91rm9200 and at91sam926x directories too.
Or we could just move everything to a common file and be done with it...
Haavard
On Sat, Feb 17, 2007 at 12:48:31AM +0100, Haavard Skinnemoen wrote:
On 2/17/07, Harald Welte laforge@openmoko.org wrote:
This patch adds MMC/SD support to the S3C2410 SoC code in u-boot
/* FIXME fill in the correct size (is set to 32MByte) */mmc_dev.blksz = 512;mmc_dev.lba = 0x10000;So...what happens if the card really isn't exactly 32 MB?
nothing really. I've used up to 1GB cards so far. Just the size indication printed is wrong. Yes, this is ugly. I should either remove printing the size, or implement it correctly.
This looks like it's been copied from pxa with few modifications ;)
indeed.
I wonder if we could perhaps create a common header file for the mmc protocol definitions instead of duplicating them all over the place? This file consists entirely of constants and data structures from the MMC spec, and isn't really arch-specific at all.
I have a patch that adds support for the mmc controller on the AT32AP7000 chip, and which adds the exact same definitions all over again to the AVR32 include directory. The same driver should work on several AT91 chips, which means that we need to add them to the at91rm9200 and at91sam926x directories too.
Or we could just move everything to a common file and be done with it...
will do so in the next version of the patch.
In message 20070217111834.GD11991@prithivi.gnumonks.org you wrote:
nothing really. I've used up to 1GB cards so far. Just the size indication printed is wrong. Yes, this is ugly. I should either remove printing the size, or implement it correctly.
Obviously "implement it correctly" is the correct thing to do :-)
Best regards,
Wolfgang Denk
-----Original Message----- From: u-boot-users-bounces@lists.sourceforge.net [mailto:u-boot-users-bounces@lists.sourceforge.net] On Behalf Of Harald Welte Sent: 17 February 2007 11:19 To: Haavard Skinnemoen
will do so in the next version of the patch.
--
Harald
Did you ever re-submit?
Regards
Peter
Here are some comments after a quick review.
Cheers, g.
On 2/16/07, Harald Welte laforge@openmoko.org wrote:
This patch adds MMC/SD support to the S3C2410 SoC code in u-boot
Signed-off-by: Harald Welte laforge@openmoko.org
Index: u-boot/cpu/arm920t/s3c24x0/Makefile
--- u-boot.orig/cpu/arm920t/s3c24x0/Makefile 2007-02-16 23:42:19.000000000 +0100 +++ u-boot/cpu/arm920t/s3c24x0/Makefile 2007-02-16 23:42:19.000000000 +0100 @@ -26,7 +26,7 @@ LIB = $(obj)lib$(SOC).a
COBJS = i2c.o interrupts.o serial.o speed.o \
usb_ohci.o nand_read.o nand.o cmd_s3c2410.o
usb_ohci.o nand_read.o nand.o mmc.o cmd_s3c2410.oSRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(SOBJS) $(COBJS)) Index: u-boot/cpu/arm920t/s3c24x0/mmc.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot/cpu/arm920t/s3c24x0/mmc.c 2007-02-16 23:43:28.000000000 +0100
+//#define MMC_DEBUG
C++ style comment; bad! :-)
+#ifdef MMC_DEBUG +#define DEBUGP printf +#else +#define DEBUGP(x, args ...) do { } while(0) +#endif
Should really use the debug/debugX macros from common.h. To enable them for a single file, just add '#define DEBUG' at the top of the file before any #include statements.
I know this is done a lot in u-boot; but I think it should be avoided as much as possible; especially in new code.
+static S3C2410_SDI *sdi;
+extern int +fat_register_device(block_dev_desc_t *dev_desc, int part_no);
External prototype in a .c file; bad practice. Include the correct header instead. If the prototype doesn't exist in a header included by the function implementation, then write a patch to add it.
Doing it this way means that the compiler cannot verify that your interface and implementation match.
+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 mmc_csd_t mmc_csd; +static int mmc_ready = 0; +static int wide = 0;
+#define CMD_F_RESP 0x01 +#define CMD_F_RESP_LONG 0x02
+static u_int32_t * +/****************************************************/ +mmc_cmd(ushort cmd, ulong arg, ushort flags) +/****************************************************/
I'm not so fond of the style putting a comment line between the function name and the return type. I think they should be together. What does everyone else think?
+{
static u_int32_t resp[5];ulong status;int i;u_int32_t ccon, csta;u_int32_t csta_rdy_bit = S3C2410_SDICMDSTAT_CMDSENT;memset(resp, 0, sizeof(resp));DEBUGP("mmc_cmd CMD%d arg=0x%08x flags=%x\n", cmd, arg, flags);sdi->SDICSTA = 0xffffffff;sdi->SDIDSTA = 0xffffffff;sdi->SDIFSTA = 0xffffffff;sdi->SDICARG = arg;ccon = cmd & S3C2410_SDICMDCON_INDEX;ccon |= S3C2410_SDICMDCON_SENDERHOST|S3C2410_SDICMDCON_CMDSTART;if (flags & CMD_F_RESP) {ccon |= S3C2410_SDICMDCON_WAITRSP;csta_rdy_bit = S3C2410_SDICMDSTAT_RSPFIN; /* 1 << 9 */}if (flags & CMD_F_RESP_LONG)ccon |= S3C2410_SDICMDCON_LONGRSP;sdi->SDICCON = ccon;while (1) {csta = sdi->SDICSTA;if (csta & csta_rdy_bit)break;if (csta & S3C2410_SDICMDSTAT_CMDTIMEOUT) {printf("===============> MMC CMD Timeout\n");sdi->SDICSTA |= S3C2410_SDICMDSTAT_CMDTIMEOUT;break;}}DEBUGP("final MMC CMD status 0x%x\n", csta);sdi->SDICSTA |= csta_rdy_bit;if (flags & CMD_F_RESP) {resp[0] = sdi->SDIRSP0;resp[1] = sdi->SDIRSP1;resp[2] = sdi->SDIRSP2;resp[3] = sdi->SDIRSP3;}return resp;+}
+#define FIFO_FILL(host) ((host->SDIFSTA & S3C2410_SDIFSTA_COUNTMASK) >> 2)
+static int +/****************************************************/ +mmc_block_read(uchar *dst, ulong src, ulong len) +/****************************************************/ +{
u_int32_t dcon, fifo;u_int32_t *dst_u32 = (u_int32_t *)dst;u_int32_t *resp;if (len == 0) {return 0;}
braces not necessary on single line conditionals
DEBUGP("mmc_block_rd dst %lx src %lx len %d\n", (ulong)dst, src, len);/* set block len */resp = mmc_cmd(MMC_CMD_SET_BLOCKLEN, len, CMD_F_RESP);sdi->SDIBSIZE = len;//sdi->SDIPRE = 0xff;/* setup data */dcon = (len >> 9) & S3C2410_SDIDCON_BLKNUM_MASK;dcon |= S3C2410_SDIDCON_BLOCKMODE;dcon |= S3C2410_SDIDCON_RXAFTERCMD|S3C2410_SDIDCON_XFER_RXSTART;if (wide)dcon |= S3C2410_SDIDCON_WIDEBUS;sdi->SDIDCON = dcon;/* send read command */resp = mmc_cmd(MMC_CMD_READ_BLOCK, src, CMD_F_RESP);while (len > 0) {u_int32_t sdidsta = sdi->SDIDSTA;fifo = FIFO_FILL(sdi);if (sdidsta & (S3C2410_SDIDSTA_FIFOFAIL|S3C2410_SDIDSTA_CRCFAIL|S3C2410_SDIDSTA_RXCRCFAIL|S3C2410_SDIDSTA_DATATIMEOUT)) {printf("mmc_block_read: err SDIDSTA=0x%08x\n", sdidsta);return -EIO;}while (fifo--) {//DEBUGP("dst_u32 = 0x%08x\n", dst_u32);*(dst_u32++) = sdi->SDIDAT;if (len >= 4)len -= 4;else {len = 0;break;}
Seems a little verbose; what about something like this? for ( ; fifo && len > 0; fifo--, len += 4) *(dst_u32++) = sdi->SDIDAT;
}}DEBUGP("waiting for SDIDSTA (currently 0x%08x\n", sdi->SDIDSTA);while (!(sdi->SDIDSTA & (1 << 4))) {}DEBUGP("done waiting for SDIDSTA (currently 0x%08x\n", sdi->SDIDSTA);
Can/should this ever time out? Should (1 << 4) be a #defined constant?
+int +/****************************************************/ +mmc_read(ulong src, uchar *dst, int size) +/****************************************************/ +{
ulong end, part_start, part_end, part_len, aligned_start, aligned_end;ulong mmc_block_size, mmc_block_address;if (size == 0) {return 0;}if (!mmc_ready) {printf("Please initialize the MMC first\n");return -1;}mmc_block_size = MMC_BLOCK_SIZE;mmc_block_address = ~(mmc_block_size - 1);src -= CFG_MMC_BASE;end = src + size;part_start = ~mmc_block_address & src;part_end = ~mmc_block_address & end;aligned_start = mmc_block_address & src;aligned_end = mmc_block_address & end;/* all block aligned accesses */DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lx aend %lx\n",src, (ulong)dst, end, part_start, part_end, aligned_start, aligned_end);
This long debug statement appears a lot; should it be a macro?
Index: u-boot/include/asm-arm/arch-s3c24x0/mmc.h
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot/include/asm-arm/arch-s3c24x0/mmc.h 2007-02-16 23:42:19.000000000 +0100
+typedef struct mmc_cid +{ +/* FIXME: BYTE_ORDER */
- uchar year:4,
- month:4;
- uchar sn[3];
- uchar fwrev:4,
- hwrev:4;
- uchar name[6];
- uchar id[3];
+} mmc_cid_t;
- Inconsistent line spacing - Bit fields are non-portable (but that might not matter for this code)
+typedef struct mmc_csd +{
uchar ecc:2,file_format:2,tmp_write_protect:1,perm_write_protect:1,copy:1,file_format_grp:1;uint64_t content_prot_app:1,rsvd3:4,write_bl_partial:1,write_bl_len:4,r2w_factor:3,default_ecc:2,wp_grp_enable:1,wp_grp_size:5,erase_grp_mult:5,erase_grp_size:5,c_size_mult1:3,vdd_w_curr_max:3,vdd_w_curr_min:3,vdd_r_curr_max:3,vdd_r_curr_min:3,c_size:12,rsvd2:2,dsr_imp:1,read_blk_misalign:1,write_blk_misalign:1,read_bl_partial:1;ushort read_bl_len:4,ccc:12;uchar tran_speed;uchar nsac;uchar taac;uchar rsvd1:2,spec_vers:4,csd_structure:2;+} mmc_csd_t;
Ditto here
+#endif /* __MMC_PXA_P_H__ */ Index: u-boot/include/s2c24x0.h =================================================================== --- u-boot.orig/include/s3c24x0.h 2007-02-16 23:42:02.000000000 +0100 +++ u-boot/include/s3c24x0.h 2007-02-16 23:42:19.000000000 +0100 @@ -637,13 +637,7 @@ S3C24X0_REG32 SDIDCNT; S3C24X0_REG32 SDIDSTA; S3C24X0_REG32 SDIFSTA; -#ifdef __BIG_ENDIAN
S3C24X0_REG8 res[3];S3C24X0_REG8 SDIDAT;-#else
S3C24X0_REG8 SDIDAT;S3C24X0_REG8 res[3];-#endif
Is this a bug fix? If so, it should go in another patch with an appropriate description
S3C24X0_REG32 SDIDAT; S3C24X0_REG32 SDIIMSK;} /*__attribute__((__packed__))*/ S3C2410_SDI;
@@ -1123,11 +1117,7 @@ #define rSDIDatCnt (*(volatile unsigned *)0x5A000030) #define rSDIDatSta (*(volatile unsigned *)0x5A000034) #define rSDIFSTA (*(volatile unsigned *)0x5A000038) -#ifdef __BIG_ENDIAN -#define rSDIDAT (*(volatile unsigned char *)0x5A00003F) -#else -#define rSDIDAT (*(volatile unsigned char *)0x5A00003C) -#endif +#define rSDIDAT (*(volatile unsigned *)0x5A00003C)
Ditto
Cheers, g.
On Saturday 17 February 2007 03:31, Grant Likely wrote:
Here are some comments after a quick review.
And some more...
<snip>
+static u_int32_t * +/****************************************************/ +mmc_cmd(ushort cmd, ulong arg, ushort flags) +/****************************************************/
I'm not so fond of the style putting a comment line between the function name and the return type. I think they should be together. What does everyone else think?
ACK. Very confusing this way.
Best regards, Stefan
On Sat, Feb 17, 2007 at 08:20:22AM +0100, Stefan Roese wrote:
<snip> > > +static u_int32_t * > > +/****************************************************/ > > +mmc_cmd(ushort cmd, ulong arg, ushort flags) > > +/****************************************************/ > > I'm not so fond of the style putting a comment line between the > function name and the return type. I think they should be together. > What does everyone else think?
ACK. Very confusing this way.
Ok. This is actually a remnescent from the original mmc code that I used as an example/basis. I'll remove it from my patch, thanks.
Cheers,
On Fri, Feb 16, 2007 at 07:31:25PM -0700, Grant Likely wrote:
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot/cpu/arm920t/s3c24x0/mmc.c 2007-02-16 23:43:28.000000000 +0100
+//#define MMC_DEBUG
C++ style comment; bad! :-)
well, I was a long-time proponent of this, too. But it's now officially part of the C99 standard. So there's not really much [technical] argument you can still make about this :(
+#ifdef MMC_DEBUG +#define DEBUGP printf +#else +#define DEBUGP(x, args ...) do { } while(0) +#endif
Should really use the debug/debugX macros from common.h. To enable them for a single file, just add '#define DEBUG' at the top of the file before any #include statements.
I know this is done a lot in u-boot; but I think it should be avoided as much as possible; especially in new code.
ok, no problem, have changed in my tree
+static S3C2410_SDI *sdi;
+extern int +fat_register_device(block_dev_desc_t *dev_desc, int part_no);
External prototype in a .c file; bad practice.
A very common one in u-boot. I'm just trying to fit in ;) Using '#include <fat.h>' now.
Include the correct header instead. If the prototype doesn't exist in a header included by the function implementation, then write a patch to add it.
will do.
+/****************************************************/ +mmc_cmd(ushort cmd, ulong arg, ushort flags) +/****************************************************/
I'm not so fond of the style putting a comment line between the function name and the return type. I think they should be together. What does everyone else think?
I've removed this already in my tree.
braces not necessary on single line conditionals
removed them in my tree.
while (fifo--) {//DEBUGP("dst_u32 = 0x%08x\n", dst_u32);*(dst_u32++) = sdi->SDIDAT;if (len >= 4)len -= 4;else {len = 0;break;}Seems a little verbose; what about something like this? for ( ; fifo && len > 0; fifo--, len += 4) *(dst_u32++) = sdi->SDIDAT;
I guess this si a question of taste/preferecne. I personally think the explicit variant (minus the //indented DEBUGP statement) is more readable than a for loop with no initial statement and ',' style multiple commands in each loop.
Can/should this ever time out?
probably yes, if you do something strange to the hardware...
Should (1 << 4) be a #defined constant?
probably, too.
/* all block aligned accesses */DEBUGP("src %lx dst %lx end %lx pstart %lx pend %lx astart %lxaend %lx\n",
src, (ulong)dst, end, part_start, part_end, aligned_start,aligned_end);
This long debug statement appears a lot; should it be a macro?
that's (like much of the stuff) just a copy of something that is already in u-boot. So one of the things while adopting it from pxa to s3c2410 was: keep it as close as possible to the original. It seems like that one was a bad example ;)
+typedef struct mmc_cid +{ +/* FIXME: BYTE_ORDER */
- uchar year:4,
- month:4;
- uchar sn[3];
- uchar fwrev:4,
- hwrev:4;
- uchar name[6];
- uchar id[3];
+} mmc_cid_t;
- Inconsistent line spacing
will fix that.
- Bit fields are non-portable (but that might not matter for this code)
yes. S3C2410 supports big-endian mode, but I doubt that anyone is using it, especially so in combination with u-boot.
Ditto here
sine those are actual MMC protocol definitions, it might be worth having one common definition. Much the same goes for the MMC read/write code in general. But I don't really know too many SD/MMC controllers to decide on the level of the API, etc.
Index: u-boot/include/s2c24x0.h
--- u-boot.orig/include/s3c24x0.h 2007-02-16 23:42:02.000000000 +0100 +++ u-boot/include/s3c24x0.h 2007-02-16 23:42:19.000000000 +0100 @@ -637,13 +637,7 @@ S3C24X0_REG32 SDIDCNT; S3C24X0_REG32 SDIDSTA; S3C24X0_REG32 SDIFSTA; -#ifdef __BIG_ENDIAN
S3C24X0_REG8 res[3];S3C24X0_REG8 SDIDAT;-#else
S3C24X0_REG8 SDIDAT;S3C24X0_REG8 res[3];-#endif
Is this a bug fix? If so, it should go in another patch with an appropriate description
well, you can access those registers as byte or as long register. If you use the long address, it will be the same addres independent of the endianness.
There was no existing user of those regs in u-boot. Since my code uses long accesses to use it, there's no point in having them defined as byte registers.
So it's not really a bugfix, just adopting the existing unused definitions to the needs of my mmc driver.
@@ -1123,11 +1117,7 @@ #define rSDIDatCnt (*(volatile unsigned *)0x5A000030) #define rSDIDatSta (*(volatile unsigned *)0x5A000034) #define rSDIFSTA (*(volatile unsigned *)0x5A000038) -#ifdef __BIG_ENDIAN -#define rSDIDAT (*(volatile unsigned char *)0x5A00003F) -#else -#define rSDIDAT (*(volatile unsigned char *)0x5A00003C) -#endif +#define rSDIDAT (*(volatile unsigned *)0x5A00003C)
Ditto
ditto ;)
In message 20070217111634.GC11991@prithivi.gnumonks.org you wrote:
C++ style comment; bad! :-)
well, I was a long-time proponent of this, too. But it's now officially part of the C99 standard. So there's not really much [technical] argument you can still make about this :(
Not technical, but stylish. It's forbidden in U-Boot code, so please fix it.
Seems a little verbose; what about something like this? for ( ; fifo && len > 0; fifo--, len += 4) *(dst_u32++) = sdi->SDIDAT;
I guess this si a question of taste/preferecne. I personally think the explicit variant (minus the //indented DEBUGP statement) is more readable than a for loop with no initial statement and ',' style multiple commands in each loop.
IMO the shorter version is much easier to read, so please change this, too.
Can/should this ever time out?
probably yes, if you do something strange to the hardware...
Should (1 << 4) be a #defined constant?
probably, too.
Then please change these, too.
This long debug statement appears a lot; should it be a macro?
that's (like much of the stuff) just a copy of something that is already in u-boot. So one of the things while adopting it from pxa to s3c2410 was: keep it as close as possible to the original. It seems like that one was a bad example ;)
If you decide to submit a second patch to also clean up the pxa code this is just the more appreciated. If you see ways to improve the code, please always do. Do not follow bad examples just because there are so many of them around.
Best regards,
Wolfgang Denk
participants (6)
-
Grant Likely -
Haavard Skinnemoen -
Harald Welte -
Peter Pearse -
Stefan Roese -
Wolfgang Denk