Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 7 Oct 2019 10:45:01 +0200
From:   Miquel Raynal <miquel.raynal@bootlin.com>
To:     Mason Yang <masonccyang@mxic.com.tw>
Cc:     richard@nod.at, marek.vasut@gmail.com, dwmw2@infradead.org,
        bbrezillon@kernel.org, computersforpeace@gmail.com,
        vigneshr@ti.com, marcel.ziswiler@toradex.com, juliensu@mxic.com.tw,
        linux-kernel@vger.kernel.org, frieder.schrempf@kontron.de,
        gregkh@linuxfoundation.org, linux-mtd@lists.infradead.org,
        tglx@linutronix.de
Subject: Re: [PATCH RFC 3/3] mtd: rawnand: Add support Macronix power down
 mode
Message-ID: <20191007104501.1b4ed8ed@xps13>
In-Reply-To: <1568793387-25199-3-git-send-email-masonccyang@mxic.com.tw>
References: <1568793387-25199-1-git-send-email-masonccyang@mxic.com.tw>
        <1568793387-25199-3-git-send-email-masonccyang@mxic.com.tw>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mason,

Mason Yang <masonccyang@mxic.com.tw> wrote on Wed, 18 Sep 2019 15:56:26
+0800:

> Macronix AD series support using power down command to
> enter a minimum power consumption state.
> 
> MTD default _suspend/_resume function replacement by
> manufacturer postponed initialization.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  drivers/mtd/nand/raw/nand_macronix.c | 78 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c
> index 991c636..99a7b2e 100644
> --- a/drivers/mtd/nand/raw/nand_macronix.c
> +++ b/drivers/mtd/nand/raw/nand_macronix.c
> @@ -15,6 +15,8 @@
>  #define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38
>  #define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0
>  
> +#define NAND_CMD_POWER_DOWN 0xB9
> +
>  struct nand_onfi_vendor_macronix {
>  	u8 reserved;
>  	u8 reliability_func;
> @@ -78,6 +80,12 @@ static void macronix_nand_onfi_init(struct nand_chip *chip)
>  		"MX30UF4G28AC",
>  };
>  
> +static const char * const nand_power_down[] = {
> +		"MX30LF1G28AD",
> +		"MX30LF2G28AD",
> +		"MX30LF4G28AD",
> +};
> +
>  static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip)
>  {
>  	unsigned int i;
> @@ -144,8 +152,64 @@ static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	return ret;
>  }
>  
> +int nand_power_down_op(struct nand_chip *chip)
> +{
> +	int ret;
> +
> +	if (nand_has_exec_op(chip)) {
> +		struct nand_op_instr instrs[] = {
> +			NAND_OP_CMD(NAND_CMD_POWER_DOWN, 0),
> +		};
> +
> +		struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
> +
> +		ret = nand_exec_op(chip, &op);
> +		if (ret)
> +			return ret;
> +
> +	} else {
> +		chip->legacy.cmdfunc(chip, NAND_CMD_POWER_DOWN, -1, -1);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxic_nand_suspend(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	mutex_lock(&chip->lock);
> +
> +	nand_select_target(chip, 0);
> +	nand_power_down_op(chip);
> +	nand_deselect_target(chip);
> +
> +	chip->suspend = 1;
> +	mutex_unlock(&chip->lock);
> +
> +	return 0;
> +}
> +
> +static void mxic_nand_resume(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	mutex_lock(&chip->lock);
> +	// toggle #CS pin to resume NAND device

C++ style comments are forbidden in code.

> +	nand_select_target(chip, 0);

On several NAND controllers there is no way to act on the CS line
without actually writing bytes to the NAND chip. So basically this
is very likely to not work.

> +	ndelay(20);

Is this delay known somewhere? Is this purely experimental?

> +	nand_deselect_target(chip);
> +
> +	if (chip->suspend)
> +		chip->suspended = 0;
> +	else
> +		pr_err("%s call for a chip which is not in suspended state\n",
> +		       __func__);
> +	mutex_unlock(&chip->lock);
> +}
> +
>  /*
> - * Macronix AC series support using SET/GET_FEATURES to change
> + * Macronix AC and AD series support using SET/GET_FEATURES to change
>   * Block Protection and Unprotection.
>   *
>   * MTD call-back function replacement by manufacturer postponed
> @@ -163,6 +227,18 @@ static void macronix_nand_post_init(struct nand_chip *chip)
>  		}
>  	}
>  
> +	for (i = 0; i < ARRAY_SIZE(nand_power_down); i++) {
> +		if (!strcmp(nand_power_down[i], chip->parameters.model)) {
> +			blockprotected = 1;
> +			break;
> +		}
> +	}
> +
> +	if (i < ARRAY_SIZE(nand_power_down)) {
> +		mtd->_suspend = mxic_nand_suspend;
> +		mtd->_resume = mxic_nand_resume;
> +	}
> +
>  	if (blockprotected && chip->parameters.supports_set_get_features) {
>  		bitmap_set(chip->parameters.set_feature_list,
>  			   ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1);

Thanks,
Miquèl