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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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?

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.

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

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