All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
@ 2007-02-16 23:02 Harald Welte
  2007-02-16 23:48 ` Haavard Skinnemoen
  2007-02-17  2:31 ` Grant Likely
  0 siblings, 2 replies; 10+ messages in thread
From: Harald Welte @ 2007-02-16 23:02 UTC (permalink / raw)
  To: u-boot

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@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
-- 
- Harald Welte <laforge@openmoko.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-16 23:02 [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support Harald Welte
@ 2007-02-16 23:48 ` Haavard Skinnemoen
  2007-02-17 11:18   ` Harald Welte
  2007-02-17  2:31 ` Grant Likely
  1 sibling, 1 reply; 10+ messages in thread
From: Haavard Skinnemoen @ 2007-02-16 23:48 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-16 23:02 [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support Harald Welte
  2007-02-16 23:48 ` Haavard Skinnemoen
@ 2007-02-17  2:31 ` Grant Likely
  2007-02-17  7:20   ` Stefan Roese
  2007-02-17 11:16   ` Harald Welte
  1 sibling, 2 replies; 10+ messages in thread
From: Grant Likely @ 2007-02-17  2:31 UTC (permalink / raw)
  To: u-boot

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.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
> +
> +//#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.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-17  2:31 ` Grant Likely
@ 2007-02-17  7:20   ` Stefan Roese
  2007-02-17 10:23     ` Harald Welte
  2007-02-17 11:16   ` Harald Welte
  1 sibling, 1 reply; 10+ messages in thread
From: Stefan Roese @ 2007-02-17  7:20 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-17  7:20   ` Stefan Roese
@ 2007-02-17 10:23     ` Harald Welte
  0 siblings, 0 replies; 10+ messages in thread
From: Harald Welte @ 2007-02-17 10:23 UTC (permalink / raw)
  To: u-boot

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.
-- 
- Harald Welte <laforge@openmoko.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-17  2:31 ` Grant Likely
  2007-02-17  7:20   ` Stefan Roese
@ 2007-02-17 11:16   ` Harald Welte
  2007-02-17 14:59     ` Wolfgang Denk
  1 sibling, 1 reply; 10+ messages in thread
From: Harald Welte @ 2007-02-17 11:16 UTC (permalink / raw)
  To: u-boot

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

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

-- 
- Harald Welte <laforge@openmoko.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-16 23:48 ` Haavard Skinnemoen
@ 2007-02-17 11:18   ` Harald Welte
  2007-02-17 15:00     ` Wolfgang Denk
  2008-01-28 14:17     ` Peter Pearse
  0 siblings, 2 replies; 10+ messages in thread
From: Harald Welte @ 2007-02-17 11:18 UTC (permalink / raw)
  To: u-boot

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.

-- 
- Harald Welte <laforge@openmoko.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-17 11:16   ` Harald Welte
@ 2007-02-17 14:59     ` Wolfgang Denk
  0 siblings, 0 replies; 10+ messages in thread
From: Wolfgang Denk @ 2007-02-17 14:59 UTC (permalink / raw)
  To: u-boot

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

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
They're usually so busy thinking about what  happens  next  that  the
only  time they ever find out what is happening now is when they come
to look back on it.                 - Terry Pratchett, _Wyrd Sisters_

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-17 11:18   ` Harald Welte
@ 2007-02-17 15:00     ` Wolfgang Denk
  2008-01-28 14:17     ` Peter Pearse
  1 sibling, 0 replies; 10+ messages in thread
From: Wolfgang Denk @ 2007-02-17 15:00 UTC (permalink / raw)
  To: u-boot

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

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
For those who like this sort of thing, this is the sort of thing they
like.                                               - Abraham Lincoln

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
  2007-02-17 11:18   ` Harald Welte
  2007-02-17 15:00     ` Wolfgang Denk
@ 2008-01-28 14:17     ` Peter Pearse
  1 sibling, 0 replies; 10+ messages in thread
From: Peter Pearse @ 2008-01-28 14:17 UTC (permalink / raw)
  To: u-boot

 

> -----Original Message-----
> From: u-boot-users-bounces at lists.sourceforge.net 
> [mailto:u-boot-users-bounces at 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

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2008-01-28 14:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-16 23:02 [U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support Harald Welte
2007-02-16 23:48 ` Haavard Skinnemoen
2007-02-17 11:18   ` Harald Welte
2007-02-17 15:00     ` Wolfgang Denk
2008-01-28 14:17     ` Peter Pearse
2007-02-17  2:31 ` Grant Likely
2007-02-17  7:20   ` Stefan Roese
2007-02-17 10:23     ` Harald Welte
2007-02-17 11:16   ` Harald Welte
2007-02-17 14:59     ` Wolfgang Denk

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.