Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[Boris Brezillon]

On Wed, 30 Nov 2016 17:02:16 +0900
Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:

> Hi.
> 
> 2016-11-28 0:04 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:
> > +Andy
> >
> > Hi Masahiro,
> >
> > On Sun, 27 Nov 2016 03:05:46 +0900
> > Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
> >  
> >> As I said in the 1st round series, I am tackling on this driver
> >> to use it for my SoCs.
> >>
> >> The previous series was just cosmetic things, but this series
> >> includes *real* changes.
> >>
> >> After some more cleanups, I will start to add changes that
> >> are really necessary.
> >> One of the biggest problems I want to solve is a bunch of
> >> hard-coded parameters that prevent me from using this driver for
> >> my SoCs.
> >>
> >> I will introduce capability flags that are associated with DT
> >> compatible and make platform-dependent parameters overridable.
> >>
> >> I still have lots of reworks to get done (so probably 3rd round
> >> series will come), but I hope it is getting better and
> >> I am showing a big picture now.
> >>  
> >
> > Thanks for posting this 2nd round of patches, I know have a clearer
> > view of what you're trying to achieve.
> > Could you be a bit more specific about the remaining rework (your 3rd
> > round)?  
> 
> 
> [1]
> I want to remove
> get_samsung_nand_para()
> get_onfi_nand_para()
> 
> The driver should not hard-code timing parameters of Samsung specific
> chips.  For ONFI, it is duplicating effort of the core framework.

Definitely.

> 
> I am thinking if it would be possible to implement
> chip->setup_data_interface() in order to set up
> timings in a generic way.

Indeed, and that'd be really cool to have this driver converted to this
new interface.

> 
> [2]
> Remove driver-internal bounce buffer.
> The current Denali driver allocate DMA_BIDIRECTIONAL buffer
> to use it as a driver-internal bounce buffer.
> 
> The hardware transfer page data into the bounce buffer,
> then CPU copies from the bounce buffer to a given buf (and oob_poi).
> This is not efficient.
> 
> So, I want to set NAND_USE_BOUNCE_BUFFER flag
> and do dma_map_single directly for a given buffer.

Sounds good. Be careful though, when you use the generic bounce buffer
interface you might have to clear the page cache info (->pagebuf = -1).

> 
> [3]
> Fix raw and oob callbacks.
> 
> I asked in another thread,
> the current driver just puts the physically accessed OOB data
> into oob_poi, which is not a collection of ECC data.
> Raw write/read() are wrong as well.

That's all good things too.

> 
> After fixing those, enable BBT scan by removing the following flag:
>     /* skip the scan for now until we have OOB read and write support */
>     chip->options |= NAND_SKIP_BBTSCAN;
> 

Hm, here you have a problem. The layout you described replaces BBMs by
payload data, thus preventing the BBM scan approach (or at least, it
won't work with factory BBMs).

Some drivers/controllers have an extra 'switch BBM/data bytes' step to
restore the BBM at the correct place before flushing the data to the
NAND or after reading a page, but I'm not sure this is the case here.

> 
> 
> > Also, if you don't mind, I'd like to have reviews and testing from intel
> > users before applying the series. Can you Cc Andy (and possibly other
> > intel maintainers) for the next round.  
> 
> Sure.
> 
> Anyway, this series already missed the pull-req for 4.10-rc1,
> we have plenty of time until 4.11-rc1.
> 
> Review/test from Intel engineers are very appreciated
> because I have no access to their boards.
> 
>