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.