Re: [PATCH v3 26/37] mtd: nand: denali: fix bank reset function

[Masahiro Yamada <yamada.masahiro@socionext.com>]

Hi Boris,


2017-03-31 1:16 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 30 Mar 2017 15:46:12 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> The function denali_nand_reset() is called during the driver probe,
>> and polls the INTR__RST_COMP and INTR__TIME_OUT bits.  However,
>> INTR__RST_COMP is set anyway even if no NAND device is connected to
>> that bank.
>>
>> This can be a problem for ONFi devices.  The nand_scan_ident()
>> iterates over maxchips, and calls nand_reset() for each chip.
>
> Actually, maxchips is a bad name. What should be passed in argument to
> nand_scan_ident() is not the maximum number of CS-line the controller
> has, it's the expected number of CS-lines provided by a chip.
>
> If you're using DT, this information should be retrieved from the DT. If
> you look at this binding doc [1] you'll see that each NAND chip has a
> reg property encoding the CS line. When a chip exposes more than one
> CS-line, the reg property should contain 2 entries describing which
> controller-side CS lines are connected to the chip CS-lines.


Actually, the Denali driver is much older than
commit 2d472aba15ff169 ("mtd: nand: document the NAND
controller/NAND chip DT representation").

So, I guess it is allowed to use the old binding,
then you were kind to merge my

commit 63757d463ea683b469c1976032054d46cecdef09
Author: Masahiro Yamada <yamada.masahiro@socionext.com>
Date:   Thu Mar 23 05:07:18 2017 +0900

    mtd: nand: denali: call nand_set_flash_node() to set DT node




Having said that, I have to admit the current implementation
(not my fault) is not nice in a long run.

In order to switch to the new binding,
I have to de-couple the controller and chips
(like you did for sunxi_nand)

I am taking a close look for how much efforts are needed
(because I am guessing you will recommend me to do this work  :-)  )





> For non-DT cases, this should be exposed by some other means (for
> example pdata, but I'm not sure it works well with PCI where everything
> is discoverable).
>
> So normally, you shouldn't have a timeout, or something is wrong with
> the DT/board description.
>
> Note that you might have different NAND models connected to the same
> NAND controller. If you call nand_scan_ident() only once and pass
> controllers->max_cs_lines to it, you will only have one chip detected,
> which is not what you expect.


The Denali IP actually supports multiple chip selects,
but has only a single set of parameter registers.

For example,
DEVICE_MAIN_AREA_SIZE must match to a chip's mtd->writesize.

If denali_select_chip() updates all parameter registers every time,
it would be theoretically possible to use different models.

(And, looks like sunxi_select_chip does this.)





-- 
Best Regards
Masahiro Yamada