[U-Boot-Users] [PATCH] Add S3C2410 MMC/SD support

[Harald Welte]

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