[U-Boot] [PATCH 1/3] Monahan/PXA3xx: integrate Linux NAND controller driver

[Scott Wood]

On Thu, Dec 24, 2009 at 04:33:49AM +0100, Marcel Ziswiler wrote:
> +/* XXX U-BOOT XXX */
> +#define __U_BOOT__
> +
> +#ifndef __U_BOOT__

I don't think we need this.

In fact, it could make it harder to spot conflicts when merging new versions
of the file from linux, since patch could find the context it's looking for
in the middle of an #ifndef __U_BOOT__ section when the change really needs
to be applied to the other side of the #else.

Plus, it makes it harder to read the code.  Let's share what we reasonably
can, and get rid of the rest.

> +#undef DEBUG

Don't undef it, just don't define it.  Maybe someone wants to turn debugging
on system-wide.

> +#ifdef DEBUG
> +#define DEBUGF(fmt, args...) printf(fmt, ##args)
> +#else /* DEBUG */
> +#define DEBUGF(x...)
> +#endif /* DEBUG */

We've already got MTDDEBUG.

> +#define CONFIG_MTD_NAND_PXA3xx_BUILTIN

This is marked deprecated in Linux; perhaps we should take the preferred
approach of platform-specified data here as well?

> +#define CONFIG_MTD_NAND_PXA3xx_POLLING

I don't see this upstream.

> +#ifdef CONFIG_NAND_SPL
> +/**
> + * memset - Fill a region of memory with the given value
> + * @s: Pointer to the start of the area.
> + * @c: The byte to fill the area with
> + * @count: The size of the area.
> + *
> + * Do not use memset() to access IO space, use memset_io() instead.
> + */
> +void *memset(void *s, int c, size_t count)
> +{
> +	char *xs = (char *) s;
> +
> +	while (count--)
> +		*xs++ = c;
> +
> +	return s;
> +}
> +
> +/**
> + * memcpy - Copy one area of memory to another
> + * @dest: Where to copy to
> + * @src: Where to copy from
> + * @count: The size of the area.
> + *
> + * You should not use this function to access IO space, use memcpy_toio()
> + * or memcpy_fromio() instead.
> + */
> +void *memcpy(void *dest, const void *src, size_t count)
> +{
> +	char *tmp = (char *) dest, *s = (char *) src;
> +
> +	while (count--)
> +		*tmp++ = *s++;
> +
> +	return dest;
> +}
> +#endif /* CONFIG_NAND_SPL */
> +
> +void msleep(int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++)
> +		udelay(1000);
> +}

Can we arrange to pull in the relevant functions from the rest of u-boot
rather than duplicate them?

Or try to avoid the need in the NAND loader -- for example, msleep is only
used in handling NAND_CMD_RESET, which you shouldn't need.

In fact, I'm not sure how this driver can be used at all with NAND_SPL.  The
normal nand_spl/nand_boot.c uses cmd_ctrl.  More complicated controllers
that can't use cmd_ctrl generally need their own nand_spl driver (such as
nand_spl/nand_boot_fsl_elbc.c).  If you have a particularly large NAND boot
region, you might be able to create a nand_spl driver that uses this driver,
but typically space is too tight.

> +/* macros for registers read/write */
> +#ifdef __U_BOOT__
> +#define nand_writel(info, off, val)     \
> +	((*(volatile u32 *) ((info)->mmio_base + (off))) = (val))
> +
> +static void nand_writesl(volatile u32 *dest, u8 *src, int size)
> +{
> +	int i;
> +	u32 *source = (u32 *)src;
> +
> +	for (i = 0; i < size; i++)
> +		*dest = *(source++);
> +}
> +
> +#define nand_readl(info, off)           \
> +	(*(volatile u32 *)((info)->mmio_base + (off)))
> +
> +static void nand_readsl(volatile u32 *source, u8 *dst, int size)
> +{
> +	u32 *dest = (u32 *)dst;
> +	int i;
> +
> +	for (i = 0; i < size; i++)
> +		*(dest++) = *source;
> +}
> +#else /* __U_BOOT__ */
> +#define nand_writel(info, off, val)	\
> +	__raw_writel((val), (info)->mmio_base + (off))
> +
> +#define nand_readl(info, off)		\
> +	__raw_readl((info)->mmio_base + (off))
> +#endif /* __U_BOOT__ */

You can (and should) use __raw_writel/__raw_readl in U-Boot as well.

There is also a prototype for__raw_writesl/__raw_readsl, but the
implementation seems to be missing -- perhaps you could add it generically,
rather than just in one driver.

-Scott