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

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.
Now, this driver implements ->setup_data_interface() method, so
nand_setup_data_interface() issues Set Features (0xEF) command to
each chip.  This can cause time-out error since denali_nand_reset()
did not check the chip existence.  If no chip there, the controller
will wait long for R/B# response, which we know never happens.
(The timeout error is correctly handled in this driver, so the
driver will be successfully probed anyway, but it will take longer
than needed.)

The Reset (0xFF) command also toggles the R/B# pin, and it sets
INTR__INT_ACT bit.  The driver should check this bit to see if the
chip has responded, then it can update denali->max_banks.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v3: None
Changes in v2:
  - Newly added

 drivers/mtd/nand/denali.c | 52 +++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

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.

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.


> Now, this driver implements ->setup_data_interface() method, so
> nand_setup_data_interface() issues Set Features (0xEF) command to
> each chip.  This can cause time-out error since denali_nand_reset()
> did not check the chip existence.  If no chip there, the controller
> will wait long for R/B# response, which we know never happens.
> (The timeout error is correctly handled in this driver, so the
> driver will be successfully probed anyway, but it will take longer
> than needed.)
> 
> The Reset (0xFF) command also toggles the R/B# pin, and it sets
> INTR__INT_ACT bit.  The driver should check this bit to see if the
> chip has responded, then it can update denali->max_banks.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v3: None
> Changes in v2:
>   - Newly added
> 
>  drivers/mtd/nand/denali.c | 52 +++++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 86b7c75..e4f2699 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -85,33 +85,6 @@ static void index_addr(struct denali_nand_info *denali,
>  	iowrite32(data, denali->flash_mem + 0x10);
>  }
>  
> -/* Reset the flash controller */
> -static uint16_t denali_nand_reset(struct denali_nand_info *denali)
> -{
> -	int i;
> -
> -	for (i = 0; i < denali->max_banks; i++)
> -		iowrite32(INTR__RST_COMP | INTR__TIME_OUT,
> -		denali->flash_reg + INTR_STATUS(i));
> -
> -	for (i = 0; i < denali->max_banks; i++) {
> -		iowrite32(1 << i, denali->flash_reg + DEVICE_RESET);
> -		while (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
> -			(INTR__RST_COMP | INTR__TIME_OUT)))
> -			cpu_relax();
> -		if (ioread32(denali->flash_reg + INTR_STATUS(i)) &
> -			INTR__TIME_OUT)
> -			dev_dbg(denali->dev,
> -			"NAND Reset operation timed out on bank %d\n", i);
> -	}
> -
> -	for (i = 0; i < denali->max_banks; i++)
> -		iowrite32(INTR__RST_COMP | INTR__TIME_OUT,
> -			  denali->flash_reg + INTR_STATUS(i));
> -
> -	return PASS;
> -}
> -
>  /*
>   * Use the configuration feature register to determine the maximum number of
>   * banks that the hardware supports.
> @@ -999,7 +972,28 @@ static int denali_setup_data_interface(struct mtd_info *mtd,
>  	return 0;
>  }
>  
> -/* Initialization code to bring the device up to a known good state */
> +static void denali_reset_banks(struct denali_nand_info *denali)
> +{
> +	int i;
> +
> +	denali_clear_irq_all(denali);
> +
> +	for (i = 0; i < denali->max_banks; i++) {
> +		iowrite32(1 << i, denali->flash_reg + DEVICE_RESET);
> +		while (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
> +			(INTR__RST_COMP | INTR__TIME_OUT)))
> +			cpu_relax();
> +		if (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
> +		      INTR__INT_ACT))
> +			break;
> +	}
> +
> +	dev_dbg(denali->dev, "%d chips connected\n", i);
> +	denali->max_banks = i;
> +
> +	denali_clear_irq_all(denali);
> +}
> +
>  static void denali_hw_init(struct denali_nand_info *denali)
>  {
>  	/*
> @@ -1019,7 +1013,7 @@ static void denali_hw_init(struct denali_nand_info *denali)
>  	denali->bbtskipbytes = ioread32(denali->flash_reg +
>  						SPARE_AREA_SKIP_BYTES);
>  	detect_max_banks(denali);
> -	denali_nand_reset(denali);
> +	denali_reset_banks(denali);
>  	iowrite32(0x0F, denali->flash_reg + RB_PIN_ENABLED);
>  	iowrite32(CHIP_EN_DONT_CARE__FLAG,
>  			denali->flash_reg + CHIP_ENABLE_DONT_CARE);

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