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

Chin Liang See
2014-03-07T15:50:36Z

Hi Masahiro,

On Fri, 2014-03-07 at 21:58 +0900, Masahiro Yamada wrote:
> 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".

Hmmm... based on past few email exchange, I am wonder there are few
different version of Denali. For this section, its located at chapter 7:
Data DMA. Wonder you can facilitate which part if not correct. From
there, I can provide what being explain in the datasheet.

>
> 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?"
>

Yup, if you see this driver, all transaction is being done using DMA
function. Its working as some of SOCFPGA users are using that code.

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

Can you share your code so we can check the differences. Its really
strange.

>
> > #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
>

I have no much objection when come to naming. FYI, I just pluck this
name from datasheet. I will changed that with file name change.

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

Yup, actually waiting more comments from you before sending out next
revision. :)

>
> > /* 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.
>

I can change that.

>
> >/* 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.
>

I can fix that. Hmmm seems checkpatch dun catch this.

>
>
> > > > > 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/
>

Great, I will patch them.