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

[Ezequiel Garcia]

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

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@xxxxxxxxxxxxxxxxxx>
> 
> +/**
> + * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/