Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 13 Jun 2017 17:04:11 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 2017-06-13 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > Le Tue, 13 Jun 2017 14:03:52 +0900,
> > Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
> >  
> >> This patch series intends to solve various problems.
> >>
> >> [1] The driver just retrieves the OOB area as-is
> >>     whereas the controller uses syndrome page layout.
> >> [2] ONFi devices are not working
> >> [3] It can not read Bad Block Marker
> >>
> >> Outstanding changes are:
> >>  - Fix raw/oob callbacks for syndrome page layout
> >>  - Implement setup_data_interface() callback
> >>  - Fix/implement more commands for ONFi devices
> >>  - Allow to skip the driver internal bounce buffer
> >>  - Support PIO in case DMA is not supported
> >>  - Switch from ->cmdfunc over to ->cmd_ctrl
> >>
> >> 18 patches were merged by v2.
> >> 11 patches were merged by v3.
> >> 2 patches were merged by v4.
> >> 5 patches were merged by v5.
> >> Here is the rest of the series.
> >>
> >> v1: https://lkml.org/lkml/2016/11/26/144
> >> v2: https://lkml.org/lkml/2017/3/22/804
> >> v3: https://lkml.org/lkml/2017/3/30/90
> >> v4: https://lkml.org/lkml/2017/6/5/1005
> >>
> >> Masahiro Yamada (18):
> >>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
> >>   mtd: nand: denali: remove unneeded find_valid_banks()
> >>   mtd: nand: denali: handle timing parameters by setup_data_interface()
> >>   mtd: nand: denali: rework interrupt handling
> >>   mtd: nand: denali: fix NAND_CMD_STATUS handling
> >>   mtd: nand: denali: fix NAND_CMD_PARAM handling  
> >
> > AFAICT, patch 5 and 6 are unneeded...
> >  
> >>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc  
> >
> > ... because you're anyway switching to ->cmd_ctrl() in patch 7, which
> > fixes the problem you were addressing in patch 5 and 6.
> >
> > Please squash those 3 patches into a single one and adjust your commit
> > message accordingly (explaining that it fixes STATUS and PARAM handling).
> > See below if you need an example.  
> 
> Squashing 3 patches is OK, but I did not get your additional implementation.
> 
> 
> > BTW, I also implemented ->read/write_buf_word() since the core may one
> > day call these functions, and the default implementations used by the
> > core when these hooks are NULL are not appropriate in your case.  
> 
> I implemented
> 
> denali_read_buf()
> denali_write_buf()
> denali_read_buf16()
> denali_write_buf16()
> 
> in 13/18 in a bit different way:
> http://patchwork.ozlabs.org/patch/774961/

Your implementation is better (I didn't know if a single
iowrite32(MODE_11 | BANK(denali->flash_bank) | 2, denali->flash_mem)
was enough or if we needed to call it each time we read/write a
byte/word).

> 
> 
> If you like, I can modify 13/18 so that
> denali_read/write_byte() is implemented based on denali_read/write_buf().

You can. Anyway, I'd prefer to have ->read/write_buf/byte/word()
implemented in patch 7, even if you actually start using
->read/write_buf() in patch 13.

> 
> 
> 
> 
> 
> > --->8---  
> > From 136727ba7b453ca1567c711037230aa6ec0f124a Mon Sep 17 00:00:00 2001
> > From: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Date: Tue, 13 Jun 2017 14:03:57 +0900
> > Subject: [PATCH] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
> >
> > The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().
> >
> > Besides, we see /* TODO: Read OOB data */ comment line.
> >
> > It would be possible to add more commands along with the current
> > implementation, but having ->cmd_ctrl() seems a better approach from
> > the discussion with Boris [1].
> >
> > Rely on the default ->cmdfunc() from the framework and implement the
> > driver's own ->cmd_ctrl(). We are also implementing  
> > ->read/write_buf/byte/word().  
> >
> > Also add ->write_byte(), which is needed for write direction commands.
> >
> > Then, we can drop nand_onfi_get_set_features_notsupp from this driver.
> >
> > This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
> > NAND_CMD_STATUS was just faked by the implementation, and the only valid
> > bit returned in this case was the WP bit.
> > NAND_CMD_PARAM was completely broken: not only the command sent on the
> > bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM, but the driver was
> > only reading 8 bytes of data, while the parameter page is contains
> > several hundreds of bytes.  
> 
> Probably "... page contains" instead of "... page is contains".

Yep. You can also reword the commit message if you want.