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

[Punnaiah Choudary Kalluri]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe@xxxxxxxxxxxxxxxxxxxx]
> Sent: Tuesday, October 25, 2016 10:16 AM
> To: Punnaiah Choudary Kalluri <punnaia@xxxxxxxxxx>
> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>;
> mark.rutland@xxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx;
> michal.simek@xxxxxxxxxx; ezequiel.garcia@xxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; rob@xxxxxxxxxxx; galak@xxxxxxxxxxxxxx;
> computersforpeace@xxxxxxxxx; dwmw2@xxxxxxxxxxxxx
> Subject: Re: [v7, 1/3] nand: pl353: Add basic driver for arm pl353 smc nand
> interface
>
> 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.
>

Sure.

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

I didn't say that I am forwarding the patch working with 4.8. I said
We made changes to driver compatible for 4.6 and working on driver rework,
Most of your comments also part of that.

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

See above.

> Please cc me on future series.

Ok.

Regards,
Punnaiah

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


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.