Re: [PATCH 5/9] mtd: rawnand: bcm47xx: Implement the exec_op() interface

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:36 +0200:

> Implement the exec_op() interface so we can get rid of the convoluted
> cmdfunc() implementation.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> This is based on my understanding of how this controller works, and I
> think it covers all the use cases covered by the custom cmdfunc()
> implementation. I might be wrong of course, so it'd be great to have
> someone test on real HW.
> ---
>  .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    |   1 +
>  .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 150 ++++++++++++++++++
>  2 files changed, 151 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> index 201b9baa52a0..00d0974b73cb 100644
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> +++ b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> @@ -10,6 +10,7 @@
>  #include <linux/mtd/rawnand.h>
>  
>  struct bcm47xxnflash {
> +	struct nand_controller base;
>  	struct bcma_drv_cc *cc;
>  
>  	struct nand_chip nand_chip;
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> index fbb7acebc8f7..184f78b3d45a 100644
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> +++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> @@ -382,6 +382,153 @@ static void bcm47xxnflash_ops_bcm4706_write_buf(struct nand_chip *nand_chip,
>  	pr_err("Invalid command for buf write: 0x%X\n", b47n->curr_command);
>  }
>  
> +static int
> +bcm47xxnflash_ops_bcm4706_exec_cmd_addr(struct nand_chip *chip,
> +					const struct nand_subop *subop)
> +{
> +	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
> +	u32 nctl = 0, col = 0, row = 0, ncols = 0, nrows = 0;
> +	unsigned int i, j;
> +
> +	for (i = 0; i < subop->ninstrs; i++) {
> +		const struct nand_op_instr *instr = &subop->instrs[i];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			if (WARN_ON_ONCE((nctl & NCTL_CMD0) &&
> +					 (nctl & NCTL_CMD1W)))
> +				return -EINVAL;
> +			else if (nctl & NCTL_CMD0)
> +				nctl |= NCTL_CMD1W |
> +					((u32)instr->ctx.cmd.opcode << 8);
> +			else
> +				nctl |= NCTL_CMD0 | instr->ctx.cmd.opcode;
> +			break;
> +		case NAND_OP_ADDR_INSTR:
> +			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
> +				u32 addr = instr->ctx.addr.addrs[j];
> +
> +				if (i < 2) {

Don't you mean j here?              ^

> +					col |= addr << i * 8;

I'm not sure this will work, addr is 32-bit and col as well, I bet you
won't end up with what you expect.

> +					nctl |= NCTL_COL;
> +					ncols++;
> +				} else {
> +					row |= addr << (i - 2) * 8;
> +					nctl |= NCTL_ROW;
> +					nrows++;
> +				}
> +			}
> +			break;
> +		default:
> +			WARN_ON_ONCE(1);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Keep the CS line asserted if there's something else to execute. */
> +	if (!subop->is_last)
> +		nctl |= NCTL_CSA;
> +
> +	bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_CONF,
> +			CONF_MAGIC_BIT |
> +			CONF_COL_BYTES(ncols) |
> +			CONF_ROW_BYTES(nrows));
> +	return bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, nctl);
> +}
> +
> +static int
> +bcm47xxnflash_ops_bcm4706_exec_waitrdy(struct nand_chip *chip,
> +				       const struct nand_subop *subop)
> +{
> +	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
> +	const struct nand_op_instr *instr = &subop->instrs[0];
> +	unsigned long timeout_jiffies = jiffies;
> +
> +	if (WARN_ON(subop->ninstrs != 1 ||
> +		    instr->type != NAND_OP_DATA_IN_INSTR))
> +		return -EINVAL;

Same remark as for the atmel migration, I doubt all these checks are
useful as long as we use the "official" parser to call these helpers. I
would rather prefer to drop them all.

> +
> +	timeout_jiffies += msecs_to_jiffies(instr->ctx.waitrdy.timeout_ms) + 1;
> +	do {
> +		if (bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_CTL) & NCTL_READY)
> +			return 0;
> +
> +		usleep_range(10, 100);
> +	} while (time_before(jiffies, timeout_jiffies));
> +
> +	return -ETIMEDOUT;
> +}



Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/