Re: [v7, 1/3] nand: pl353: Add basic driver for arm pl353 smc nand interface

[Jason Gunthorpe <jgunthorpe@obsidianresearch.com>]

On Tue, Oct 25, 2016 at 03:58:49AM +0000, Punnaiah Choudary Kalluri wrote:

> > I have hacked the v7 patchset enough to work on 4.8 and hacked it some
> > more to work on my hardware. The driver appears to be in no better
> > shape than the 3.12 out-of-tree Xilinx release I was using previously..
>
> The driver in Xilinx tree works with 4.6 kernel and adopted the
> required

I mean, the driver from the 3.12 Xilinx tree that I last looked
at years ago. This is at v7 now and is still needs lots of work, I did
some fixing, including making parts of it work on 4.8.
You can copy it out of the patch I sent you.

> Changes (may release in 1-2 weeks). Still it need some rework, now a days
> I am seeing many requests from different people for this driver and some of
> Them are using different version of IP as you know this IP has four variants and
> Xilinx is using the pl353 variant.

Well, I am using Xilinx Zynq hardware and care about making that
configuration actually work. This is the last driver I require to use
Zynq with mainline.

It clearly needs more work than just forward porting to 4.8, so please
let me know if you are able to tackle this.

> Let's wait for the next series of patches and Get the patches tested
> with others as well along with the review comments.

You now have 10 review comments from me, please respond to all of them in
your v8 patchset - no sense in continuing to drag this out.

Please cc me on future series.

Jason

> Regards,
> Punnaiah
> > I've attached the ugly, ugly patch I made, it may save you some time
> > when preparing v8.
> > 
> > Commentary:
> >  1) nand_chip already includes mtd_info, no reason for a second one in
> >     the pl353_nand_info struct. The standard accessors mtd_to_nand/etc
> >     should be used instead of priv
> >  2) I hacked out all the ECC stuff from the driver since I don't use
> >     it and it was broken.. Obviously some has to come back, but fixing
> >     other things on this list first will make that much easier and better..
> >  3) pl353_nand_write_page_swecc/pl353_nand_read_page_swecc are pure
> >     outdated copies of the core routines and should not exist in a
> >     driver. This needs to be fixed so nand_scan_tail sets them.
> >     This seems to be a side effect of #9 ?
> >  4) The hacky stuff to detect 2 vs 3 address cycle NAND doesn't work,
> >     and doesn't work without ONFI (see patch for possible fix). The
> >     driver should probably use the same scheme the core code uses..
> >     But this is all a problem because of #10
> >  5) The driver assumes alignment of the iomap, needs to use + not |
> >     when constructing the address. (yes, this blows up on my system)
> >  6) Driver unconditionally sets timing to ONFI mode 0 (slow!)
> >     Maybe timing should be common code driven by DT..
> >  7) Driver unconditionally selects a BBT format, and an ECC layout
> >     (neither match what my system uses, but I guess that is a core mtd
> >     issue these days :/)
> >  8) Driver unconditionally forces a chip-delay (wrong for my
> >     system) maybe this should be common code driven by DT..
> >  9) This buisness with pl353_nand_ecc_init and the
> >     special functions to do PL353_NAND_LAST_TRANSFER_LENGTH stuff
> >     is just horrid. I'd expect that is a big NAK.
> > 
> >     The driver needs to implement read_buf *properly* so the core
> >     routines can be used instead of trying to 'fix' the call sites
> >     to do the PL353_NAND_LAST_TRANSFER_LENGTH stuff.
> >  10) pl353_nand_cmd_function is a wonky copy of nand_command_lp,
> > maybe
> >      this driver should use cmd_ctrl, or the core code should be
> >      refactored some more..