[LINUX PATCH v8 1/2] Documentation: nand: pl353: Add documentation for controller and driver

[Naga Sureshkumar Relli]

Hi Miquel,

> -----Original Message-----
> From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
> Sent: Thursday, March 22, 2018 1:01 PM
> To: Naga Sureshkumar Relli <nagasure at xilinx.com>
> Cc: nagasureshkumarrelli at gmail.com; boris.brezillon at bootlin.com;
> richard at nod.at; dwmw2 at infradead.org; computersforpeace at gmail.com;
> marek.vasut at gmail.com; linux-mtd at lists.infradead.org; Michal Simek
> <michals at xilinx.com>; Punnaiah Choudary Kalluri <punnaia at xilinx.com>
> Subject: Re: [LINUX PATCH v8 1/2] Documentation: nand: pl353: Add
> documentation for controller and driver
> 
> Hi Naga,
> 
> I removed linux-kernel from the cc: list (linux-mtd is enough).
> 
> > > > +Since the controller expects that the chipselect bit should be
> > > > +cleared for the
> > >
> > >                                          ^chip select   ^ would? is?
> > Will correct in next patch.
> > >
> > > > +last data transfer i.e last 4 data bytes, the existing nandbase
> > > > +page
> > >
> > > What is nandbase?
> > It is just nand page read/write, I will correct it,
> > >
> > > > +read/write routines for soft ecc and ecc none modes will not work.
> > > > +So, inorder
> > >
> > > s/ecc/ECC/                                                   in order^
> > >
> > > > +to make this driver work, it always updates the ecc mode as HW
> > > > +ECC and can
> > >
> > > s/ecc/ECC/
> > I will update.
> > >
> > > > +implemented the page read/write functions for supporting the SW ECC.
> > >
> > > s/can implemented/implements/?
> > Will correct in next patch.
> >
> > >
> > > I don't understand this paragraph, can you explain it please? I am
> > > not sure to understand the limitation nor how you address it.
> > There is a limitation in SMC, that we must set ECC LAST on last data
> > phase access, to tell ECC block not to expect any data further.
> > Ex:  When number of ECC STEP are 4, then till 3 we will write to flash using
> SMC with HW ECC enabled.
> > And for the last ECC STEP, we will subtract 4bytes from page size, and will
> initiate a transfer.
> > And the remaining 4 as one more transfer with ECC_LAST bit set in NAND
> > Data phase register to notify ECC block not to expect any more data. The last
> block should be align with end of 512 byte block.
> > Because of this limitation, we are not using core routines.
> 
> This is way more understandable, thanks. Can you adapt this explanation into
> the documentation?
Ok, I will add in next series.
> 
> Otherwise I am still not convinced that you need special handlers for SW ECC nor
> raw page accessors as they don't toggle the ECC/ECC_LAST bit set, so please try
> to remove them from both the documentation and the code.
Sure, I will update.
> 
> > >
> > > > +
> > > > +HW ECC mode:
> > > > +	Upto 2K page size is supported and beyond that it retuns
> > > > +-ENOSUPPORT error. If the flsh has ONDIE ecc controller then the
> > >
> > >     ^ -ENOTSUPP              ^flash   ^on-die ECC
> > >
> > Will correct in next patch.
> > > > +priority has given to the ONDIE ecc controller. Also the current
> > >
> > >             ^ is given?      ^on-die ECC
> > >
> > Will correct in next patch.
> > > > +implementation has support for upto 64 byte oob area
> > >
> > >                                   ^up to 64 bytes of OOB data.
> > >
> > Will correct in next patch.
> > > > +
> > > > +SW ECC mode:
> > > > +	It supports all the pgae sizes. But since, zynq soc bootrom uses
> > >
> > >                             ^ page                 ^Zync SOC
> > >
> > Will correct in next patch.
> > > > +HW ECC for the devices that have pgae size <=2K so, to avoid any
> > > > +ecc related
> > >
> > >                                     ^ page <= 2K,               ECC^
> > >
> > Will correct in next patch.
> > > > +issues during boot, prefer HW ECC over SW ECC.
> > >
> > > I suppose this means that if no ECC mode is given ie. no
> > > nand-ecc-mode in the DT, the driver will use HW ECC by default, right?
> > Yes.
> 
> Then can you reword to make it clearer? I think you can drop the
> "issues during boot" argument and stick to something simple like "When
> the kernel is not aware of the ECC mode, use HW ECC by default.
Ok, I will update

Thanks,
Naga Sureshkumar Relli.
> 
> > >
> > > > +
> > > > +For devicetree binding information please refer the below dt binding
> > > > +file Documentation/devicetree/bindings/memory-controllers/pl353-
> smc.txt.
> > >
> > > This file does not exist in my tree.
> > Actually this driver has dependency with SMC driver.
> > Recently I sent one more patch series, there we will find all these.
> > This is for drivers/memory/
> > Since SMC controller has two interfaces, Nand and Nor.
> > So we have two drivers, one main driver is SMC and the second is NAND driver.
> > https://www.spinics.net/lists/kernel/msg2748832.html
> > https://www.spinics.net/lists/kernel/msg2748834.html
> > https://www.spinics.net/lists/kernel/msg2748840.html
> 
> Ok, thanks for pointing the links.
> 
> >
> > >
> > >
> > > Thanks for contributing this driver,
> > > Miquèl
> > >
> > > --
> > > Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel
> > > engineering https://bootlin.com
> >
> > Thanks,
> > Naga Sureshkumar Relli.
> 
> 
> 
> --
> Miquel Raynal, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com