Re: [PATCH v2] axs_nand - add driver for NAND controller used on Synopsys AXS dev boards

From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
To: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
Cc: linux-mtd@lists.infradead.org,
	Vineet Gupta <Vineet.Gupta1@synopsys.com>,
	Brian Norris <computersforpeace@gmail.com>,
	Grant Likely <grant.likely@linaro.org>,
	David Woodhouse <dwmw2@infradead.org>,
	Francois Bedard <Francois.Bedard@synopsys.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v2] axs_nand - add driver for NAND controller used on Synopsys AXS dev boards
Date: Fri, 4 Apr 2014 11:09:34 -0300	[thread overview]
Message-ID: <20140404140933.GA25772@arch.cereza> (raw)
In-Reply-To: <1396597089-1081-1-git-send-email-abrodkin@synopsys.com>

On Apr 04, Alexey Brodkin wrote:
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 

Maybe it would be nice adding some driver description here, so the commit
log actually says something useful about the commit.

[..]
> Reviewed-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> +/**
> + * axs_flag_wait_and_reset - Waits until requested flag in INT_STATUS register
> + *              is set by HW and resets it by writing "1" in INT_CLR_STATUS.
> + * @host:	Pointer to private data structure.
> + * @flag:	Bit/flag offset in INT_STATUS register
> + */
> +static void axs_flag_wait_and_reset(struct axs_nand_host *host, int flag)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < AXS_FLAG_WAIT_DELAY * 100; i++) {
> +		unsigned int status = reg_get(host, INT_STATUS);
> +
> +		if (status & (1 << flag)) {
> +			reg_set(host, INT_CLR_STATUS, 1 << flag);
> +			return;
> +		}
> +
> +		udelay(10);
> +	}
> +
> +	/*
> +	 * Since we cannot report this problem any further than
> +	 * axs_nand_{write|read}_buf() letting user know there's a problem.
> +	 */
> +	dev_err(host->dev, "Waited too long (%d s.) for flag/bit %d\n",
> +		AXS_FLAG_WAIT_DELAY, flag);
> +}

Hm... I'm not sure the above is really true.

The NAND core uses the replaceable chip->waitfunc callback to check the
status of issued commands. See for instance:

static int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
                              int page)
{
        int status = 0;
        const uint8_t *buf = chip->oob_poi;
        int length = mtd->oobsize;

        chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
        chip->write_buf(mtd, buf, length);
        /* Send command to program the OOB data */
        chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);

        status = chip->waitfunc(mtd, chip);

        return status & NAND_STATUS_FAIL ? -EIO : 0;
}

On the other side, if you are clearing the flags in axs_flag_wait_and_reset()
it might be a bit hard for you to get this right.

IOW, I'm not saying you *must* do this, but instead suggesting that you take
a look at waitfunc() and see if it helps report a proper error in the
read/write path.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com