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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Mon, 27 Apr 2020 19:18:11 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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?              ^
> 

Nice catch! Indeed, it should be j.

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

Well, assuming I use j that's really what I want. addr is an u32 to
allow for a shift greater than 8, but the value has be extracted
from the instr->ctx.addr.addrs array which is an u8 array, thus
making addr <= 0xff.

> 
> > +					nctl |= NCTL_COL;
> > +					ncols++;
> > +				} else {
> > +					row |= addr << (i - 2) * 8;

And it's j here as well.

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

Agreed.

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