Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in polling mode

[Miquel Raynal]

Hi Arseniy,

AVKrasnov@xxxxxxxxxxxxxx wrote on Thu, 1 Jun 2023 09:18:45 +0300:

> This adds support of waiting for command completion in sofyware polling

							software

> mode. It is needed when ready/busy pin is not implemented in hardware.

Please also use (here and in all your commits) the affirmative tense:

"Add support for "

instead of

"This adds support"

or

"This commit adds"

Also, this is not a fix but a feature, so it should be introduced after
all the fixes. This way I can take all the fixes first without
dependency.

> Signed-off-by: Arseniy Krasnov <AVKrasnov@xxxxxxxxxxxxxx>
> ---
>  drivers/mtd/nand/raw/meson_nand.c | 53 ++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index 9dd4a676497b..82a629025adc 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -179,6 +179,7 @@ struct meson_nfc {
>  	u32 info_bytes;
>  
>  	unsigned long assigned_cs;
> +	bool use_polling;

Very ambiguous wording. Polling is usually what you do to get the data.
Here you want a control signal so I would rename this flag with
something like "no_rb_pin".

Do you have a driver structure to represent the nand chip? Because
there is one RB per chip (even per die), not per controller.

>  };
>  
>  enum {
> @@ -392,32 +393,38 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>  	}
>  }
>  
> -static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms)
> +static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)

I would rather prefer keeping the controller pointer here. It's your
main structure here.

>  {
> -	u32 cmd, cfg;
> -	int ret = 0;
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>  
> -	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> -	meson_nfc_drain_cmd(nfc);
> -	meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
> +	if (nfc->use_polling) {
> +		return nand_soft_waitrdy(nand, timeout_ms);

You could simplify the diff by a lot by avoiding this extra tab
you added in the second part of the function, using:

	if (no_rb_pin)
		return nand_soft_waitrdy();

	...

> +	} else {
> +		u32 cmd, cfg;
> +		int ret = 0;
>  
> -	cfg = readl(nfc->reg_base + NFC_REG_CFG);
> -	cfg |= NFC_RB_IRQ_EN;
> -	writel(cfg, nfc->reg_base + NFC_REG_CFG);
> +		meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> +		meson_nfc_drain_cmd(nfc);
> +		meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT);
>  
> -	reinit_completion(&nfc->completion);
> +		cfg = readl(nfc->reg_base + NFC_REG_CFG);
> +		cfg |= NFC_RB_IRQ_EN;
> +		writel(cfg, nfc->reg_base + NFC_REG_CFG);
>  
> -	/* use the max erase time as the maximum clock for waiting R/B */
> -	cmd = NFC_CMD_RB | NFC_CMD_RB_INT
> -		| nfc->param.chip_select | nfc->timing.tbers_max;
> -	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +		reinit_completion(&nfc->completion);
>  
> -	ret = wait_for_completion_timeout(&nfc->completion,
> -					  msecs_to_jiffies(timeout_ms));
> -	if (ret == 0)
> -		ret = -1;
> +		/* use the max erase time as the maximum clock for waiting R/B */
> +		cmd = NFC_CMD_RB | NFC_CMD_RB_INT
> +			| nfc->param.chip_select | nfc->timing.tbers_max;
> +		writel(cmd, nfc->reg_base + NFC_REG_CMD);
>  
> -	return ret;
> +		ret = wait_for_completion_timeout(&nfc->completion,
> +						  msecs_to_jiffies(timeout_ms));
> +		if (ret == 0)
> +			return -ETIMEDOUT;
> +
> +		return 0;
> +	}
>  }
>  
>  static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
> @@ -623,7 +630,7 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>  	if (in) {
>  		nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
>  		writel(nfc->cmdfifo.rw.cmd1, nfc->reg_base + NFC_REG_CMD);
> -		meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tR_max));
> +		meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tR_max));

Let's avoid that.

>  	} else {
>  		meson_nfc_cmd_idle(nfc, nfc->timing.tadl);
>  	}
> @@ -669,7 +676,7 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>  
>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> -	meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tPROG_max));
> +	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
>  
>  	meson_nfc_dma_buffer_release(nand, data_len, info_len, DMA_TO_DEVICE);
>  
> @@ -952,7 +959,7 @@ static int meson_nfc_exec_op(struct nand_chip *nand,
>  			break;
>  
>  		case NAND_OP_WAITRDY_INSTR:
> -			meson_nfc_queue_rb(nfc, instr->ctx.waitrdy.timeout_ms);
> +			meson_nfc_queue_rb(nand, instr->ctx.waitrdy.timeout_ms);
>  			if (instr->delay_ns)
>  				meson_nfc_cmd_idle(nfc, delay_idle);
>  			break;
> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	nfc->use_polling = of_property_read_bool(dev->of_node, "polling");

This is a problem. You cannot add a polling property like that.

There is already a nand-rb property which is supposed to carry how are
wired the RB lines. I don't see any in-tree users of the compatibles, I
don't know how acceptable it is to consider using soft fallback when
this property is missing, otherwise take the values of the rb lines
provided in the DT and user hardware control, but I would definitely
prefer that.

In any case you'll need a dt-binding update which must be acked by
dt-binding maintainers.

> +
>  	writel(0, nfc->reg_base + NFC_REG_CFG);
>  	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
>  	if (ret) {


Thanks,
Miquèl