[U-Boot] [PATCH v4 1/2] nand/denali: Adding Denali NAND driver support

[Masahiro Yamada]

Hello Chin,


> +/* setups the HW to perform the data DMA */
> +static void denali_setup_dma_sequence(int op)
> +{
> +	const int page_count = 1;
> +	uint32_t mode;
> +	uint32_t addr = (uint32_t)denali.buf.dma_buf;
> +
> +	flush_dcache_range(addr, addr + sizeof(denali.buf.dma_buf));
> +
> +	mode = MODE_10 | BANK(denali.flash_bank);
> +
> +	/* DMA is a four step process */
> +
> +	/* 1. setup transfer type and # of pages */
> +	index_addr(mode | denali.page, 0x2000 | op | page_count);
> +
> +	/* 2. set memory high address bits 23:8 */
> +	index_addr(mode | ((uint32_t)(addr >> 16) << 8), 0x2200);
> +
> +	/* 3. set memory low address bits 23:8 */
> +	index_addr(mode | ((uint32_t)addr << 8), 0x2300);
> +
> +	/* 4.  interrupt when complete, burst len = 64 bytes*/
> +	index_addr(mode | 0x14000, 0x2400);
> +}


This function would not work on my board.
Besides, it is completely different from the description
written in  "Chapter 7. Data DMA" of 
"Denali NAND Flash Memory Controller User's Guide".

It looks really weird to me.
So I asked some questions to Cadence (Denali, before M&A).

Accoding to the anwers from Cadence,
in conclusion, I think this function seems wrong.

Please let me confirm once again:
"Did this function really work on your board?"

I had to re-write this function to run on my board.


> #define DENALI_DATA_OFFSET_ADDRESS	0x10

This is a bad name.
I can't understand at all what this macro stands for.
Besides, you only defined data register, not control register.
This is weird.

There exist two registers: control and data
for indexed-addressing of Denali NAND controller.

Denali document shows
Table 8.1
Register      |     Offset Address
----------------------------------
Control        |    0x0
Data            |    0x10


How about this?
#define INDEX_CTRL_REG     0x0
#define INDEX_DATA_REG    0x10

And move definition from denali_nand.c to denal_nand.h.



> /* setups the HW to perform the data DMA */
> static void denali_setup_dma_sequence(int op)
> {

Please rename "denali_setup_dma_sequence"
to "denali_setup_dma".

Linux uses the latter.
No reason to change it.


>/* Common DMA function */
>static uint32_t denali_dma_configuration(uint32_t ops, bool raw_xfer,
>	uint32_t irq_mask, int oob_required)

Fix indentaion, please.



> > > > It's "denali.c" in Linux -- why "denali_nand.c" here?
> > > 
> > > 
> > > 
> > > It seems all the existing U-Boot nand driver is using this naming
> > > standard where <platform>_nand.
> > 
> > Not all -- there's omap_gpmc.c, omap_elm.c, nomadik.c, ndfc.c, etc.
> > 
> > A lot of them have the _nand.c suffix in Linux, too.  Personally, I
> > think it's redundant.
> 
> 
> Sure, I can change to denali.c

Agreed.



> > > Why are you using raw I/O accessors?  The Linux driver doesn't do this.
> > 
> > Add ioread32/iowrite32 to arch/arm/include/asm/io.h
> > and use them?
> > 
> 
> 
> Changed all to use standard writel and readl.

In my understanding, ioread32/iowrite32 is the same as
readl/writel  at least memory mapped architectures such ARM.

Because Linux Kernel' denali.c uses  ioread32/iowrite32,
I thought using them is helpful for easy diffing.


I've posted my feedback.
I hope it is helpful for you.
http://patchwork.ozlabs.org/patch/327943/



Best Regards
Masahiro Yamada