[U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support
Grant Likely
grant.likely at secretlab.ca
Sat Feb 17 03:31:25 CET 2007
Here are some comments after a quick review.
Cheers,
g.
On 2/16/07, Harald Welte <laforge at openmoko.org> wrote:
> This patch adds MMC/SD support to the S3C2410 SoC code in u-boot
>
> Signed-off-by: Harald Welte <laforge at 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
More information about the U-Boot
mailing list