[PATCH v2 4/4] mtd: rawnand: brcmnand: exec_op implementation

[dregan at mail.com]

Hi Miquèl,

>
> Hello,
> 
> dregan at mail.com wrote on Thu, 12 Oct 2023 02:42:37 +0200:
> 

...

> > +
> > +int brcmnand_exec_op_is_status(const struct nand_operation *op)
> > +{
> > +	if ((op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> > +		(op->instrs[0].ctx.cmd.opcode == NAND_CMD_STATUS) &&
> > +		(op->ninstrs > 1) &&
> > +		(op->instrs[1].type == NAND_OP_DATA_IN_INSTR))
> 
> You need to start checking if op->ninstrs == 2, no?

Will fix.

> 
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +int brcmnand_exec_op_is_reset(const struct nand_operation *op)
> > +{
> > +	if ((op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> > +		(op->instrs[0].ctx.cmd.opcode == NAND_CMD_RESET))
> 
> Please check ninstrs as well (first).

Will do.

> 
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int brcmnand_exec_op(struct nand_chip *chip,
> > +			    const struct nand_operation *op,
> > +			    bool check_only)
> > +{
> > +	struct brcmnand_host *host = nand_get_controller_data(chip);
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	const struct nand_op_instr *instr = &op->instrs[0];
> > +	u8 *status;
> > +	unsigned int i;
> > +	int ret = 0;
> > +
> > +	if (check_only)
> > +		return 0;
> > +
> > +	if (brcmnand_exec_op_is_status(op)) {
> > +		status = op->instrs[1].ctx.data.buf.in;
> > +		*status = brcmnand_status(host);
> > +
> > +		return 0;
> > +	}
> > +
> > +	if (op->deassert_wp)
> > +		brcmnand_wp(mtd, 0);
> > +
> > +	for (i = 0; i < op->ninstrs; i++) {
> > +		instr = &op->instrs[i];
> > +
> > +		if (instr->type == NAND_OP_WAITRDY_INSTR) {
> > +			ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> 
> Why don't you handle this in brcmnand_exec_instr() ? It would look much
> nicer. Even if this does not involve any additional "cycle" on the NAND
> bus, it still is an instruction of the current operation.

Good idea.

> 
> > +			if (ret)
> > +				break;
> > +		} else { /* otherwise pass to low level implementation */
> > +			ret = brcmnand_exec_instr(host, instr, i == (op->ninstrs - 1));
> 
> What's the point of this i == op->ninstrs - 1 check?
> 

It's to let the controller know we are issuing the last command
in a series of commands. I may need to rework this as to moving
waitrdy into brcmnand_exec_instr


> > +			if (ret)
> > +				break;
> > +		}
> > +	}
> > +
> > +	if (op->deassert_wp)
> > +		brcmnand_wp(mtd, 1);
> > +
> > +	if (brcmnand_exec_op_is_reset(op)) {
> > +		brcmnand_status(host);
> > +		brcmnand_wp(mtd, 1);
> 
> This should be done right after the status check, no?
> 
> Anyway, I'm not sure I understand this. Why do you read the status and
> assert WP?

I read the status to fill the cached status, and reset the WP to
it's default state.

...

> Thanks,
> Miquèl
>

Thanks!

-Dave