From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chin Liang See <clsee@altera.com>
Date: Fri, 7 Mar 2014 09:50:36 -0600
Subject: [U-Boot] [PATCH v4 1/2] nand/denali: Adding Denali NAND driver
	support
In-Reply-To: <20140307215806.6EE0.AA925319@jp.panasonic.com>
References: <1394042827-4912-1-git-send-email-clsee@altera.com>
	<20140307215806.6EE0.AA925319@jp.panasonic.com>
Message-ID: <1394207436.2749.18.camel@clsee-VirtualBox.altera.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.