diff mbox series

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

Message ID 20230601061850.3907800-3-AVKrasnov@sberdevices.ru
State New
Headers
Series refactoring, fixes and updates for Meson NAND |

Commit Message

Arseniy Krasnov June 1, 2023, 6:18 a.m. UTC 
  This adds support of waiting for command completion in sofyware polling
mode. It is needed when ready/busy pin is not implemented in hardware.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 53 ++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 22 deletions(-)

Comments

Arseniy Krasnov June 1, 2023, 7:57 a.m. UTC | #1 
On 01.06.2023 09:18, Arseniy Krasnov wrote:
> This adds support of waiting for command completion in sofyware polling

                                                         ^^^ will fix it

> mode. It is needed when ready/busy pin is not implemented in hardware.
> 
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  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;
>  };
>  
>  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)
>  {
> -	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);
> +	} 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));
>  	} 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");
> +
>  	writel(0, nfc->reg_base + NFC_REG_CFG);
>  	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
>  	if (ret) {
Miquel Raynal June 1, 2023, 8:07 a.m. UTC | #2 
Hi Arseniy,

AVKrasnov@sberdevices.ru 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@sberdevices.ru>
> ---
>  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
Arseniy Krasnov June 1, 2023, 11:09 p.m. UTC | #3 
On 01.06.2023 11:07, Miquel Raynal wrote:
> Hi Arseniy,
> 
> AVKrasnov@sberdevices.ru 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.

Ack

> 
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>>  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".

Ack

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

Ack

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

I see. So i need to implement processing of this property here? And if it
is missed -> use software waiting. I think interesting thing will be that:

1) Even with support of this property here, I really don't know how to pass
   RB values to this controller - I just have define for RB command and that's
   it. I found that this property is an array of u32 - IIUC each element is
   RB pin per chip. May be i need to dive into the old vendor's driver to find
   how to use RB values (although this driver uses software waiting so I'm not
   sure that I'll find something in it).
2) I can't test RB mode - I don't have such device :( 

Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
in controller specific register for waiting (I guess Meson controller has something
like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
'nand-rb' property, but never use it.

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

You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?

Thanks, Arseniy

> 
>> +
>>  	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
Miquel Raynal June 5, 2023, 9:05 a.m. UTC | #4 
Hi Arseniy,

> >> @@ -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.  
> 
> I see. So i need to implement processing of this property here? And if it
> is missed -> use software waiting. I think interesting thing will be that:
> 
> 1) Even with support of this property here, I really don't know how to pass
>    RB values to this controller - I just have define for RB command and that's
>    it. I found that this property is an array of u32 - IIUC each element is
>    RB pin per chip. May be i need to dive into the old vendor's driver to find
>    how to use RB values (although this driver uses software waiting so I'm not
>    sure that I'll find something in it).

Liang, can you please give use the relevant information here? How do we
target RB0 and RB1? It seems like you use the CS as only information
like if the RB lines where hardwired internally to a CS. Can we invert
the lines with a specific configuration?

Arseniy, if the answer to my above question is no, then you should
expect the nand-rb and reg arrays to be identical. If they are not,
then you can return -EINVAL.

If the nand-rb property is missing, then fallback to software wait.

> 2) I can't test RB mode - I don't have such device :( 
> 
> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
> in controller specific register for waiting (I guess Meson controller has something
> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
> 'nand-rb' property, but never use it.

Yes, the logic around the second RB line (taking care of CS1/CS3) is
slightly broken or at least badly documented, and thus should not be
used.

> > In any case you'll need a dt-binding update which must be acked by
> > dt-binding maintainers.  
> 
> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?

Yes. In a dedicated patch. Something along the lines:

	nand-rb: true

inside the nand chip object should be fine. And flag the change as a
fix because we should have used and parsed this property since the
beginning.

Thanks,
Miquèl
Liang Yang June 5, 2023, 1:19 p.m. UTC | #5 
Hi Miquel and Arseniy,


On 2023/6/5 17:05, Miquel Raynal wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi Arseniy,
> 
>>>> @@ -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.
>>
>> I see. So i need to implement processing of this property here? And if it
>> is missed -> use software waiting. I think interesting thing will be that:
>>
>> 1) Even with support of this property here, I really don't know how to pass
>>     RB values to this controller - I just have define for RB command and that's
>>     it. I found that this property is an array of u32 - IIUC each element is
>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>     how to use RB values (although this driver uses software waiting so I'm not
>>     sure that I'll find something in it).
> 
> Liang, can you please give use the relevant information here? How do we
> target RB0 and RB1? It seems like you use the CS as only information
> like if the RB lines where hardwired internally to a CS. Can we invert
> the lines with a specific configuration?

Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
of different CEs need to be bound into one wire and connect with
NAND_RB0 if want to use controller polling rb. the current operating
CE of NAND is decided to "chip_select", of course controller internally 
has different nfc commands to regconize which Ce's RB signal is polling.

<&nand_pins> in dts/yaml should include the NAND_RB0 if hardware 
connects, or use software polling here.

@Arseniy, sorry, i don't travel all the informations yet. but why don't 
you use the new RB_INT command with irq that i provided in another 
thread. the new RB_INT command doesn't depend on the physical RB wires, 
it also send the READ status command(0x70) and wait for the irq wake up 
completion.

> Arseniy, if the answer to my above question is no, then you should
> expect the nand-rb and reg arrays to be identical. If they are not,
> then you can return -EINVAL.
> 
> If the nand-rb property is missing, then fallback to software wait.
> 
>> 2) I can't test RB mode - I don't have such device :(
>>
>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>> in controller specific register for waiting (I guess Meson controller has something
>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>> 'nand-rb' property, but never use it.
> 
> Yes, the logic around the second RB line (taking care of CS1/CS3) is
> slightly broken or at least badly documented, and thus should not be
> used.
> 
>>> In any case you'll need a dt-binding update which must be acked by
>>> dt-binding maintainers.
>>
>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?
> 
> Yes. In a dedicated patch. Something along the lines:
> 
>          nand-rb: true
> 
> inside the nand chip object should be fine. And flag the change as a
> fix because we should have used and parsed this property since the
> beginning.
> 
> Thanks,
> Miquèl
Liang Yang June 5, 2023, 1:30 p.m. UTC | #6 
On 2023/6/5 21:19, Liang Yang wrote:
> Hi Miquel and Arseniy,
> 
> 
> On 2023/6/5 17:05, Miquel Raynal wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hi Arseniy,
>>
>>>>> @@ -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.
>>>
>>> I see. So i need to implement processing of this property here? And 
>>> if it
>>> is missed -> use software waiting. I think interesting thing will be 
>>> that:
>>>
>>> 1) Even with support of this property here, I really don't know how 
>>> to pass
>>>     RB values to this controller - I just have define for RB command 
>>> and that's
>>>     it. I found that this property is an array of u32 - IIUC each 
>>> element is
>>>     RB pin per chip. May be i need to dive into the old vendor's 
>>> driver to find
>>>     how to use RB values (although this driver uses software waiting 
>>> so I'm not
>>>     sure that I'll find something in it).
>>
>> Liang, can you please give use the relevant information here? How do we
>> target RB0 and RB1? It seems like you use the CS as only information
>> like if the RB lines where hardwired internally to a CS. Can we invert
>> the lines with a specific configuration?
> 
> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
> of different CEs need to be bound into one wire and connect with
> NAND_RB0 if want to use controller polling rb. the current operating
> CE of NAND is decided to "chip_select", of course controller internally 
> has different nfc commands to regconize which Ce's RB signal is polling.
> 
> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware 
> connects, or use software polling here.
> 
> @Arseniy, sorry, i don't travel all the informations yet. but why don't 
> you use the new RB_INT command with irq that i provided in another 
> thread. the new RB_INT command doesn't depend on the physical RB wires, 
> it also send the READ status command(0x70) and wait for the irq wake up 
> completion.

Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) 
or new RB_INT(no physical RB wires). the new RB_INT command decides the 
RB0 or RB1 by the previous command with ce args.

> 
>> Arseniy, if the answer to my above question is no, then you should
>> expect the nand-rb and reg arrays to be identical. If they are not,
>> then you can return -EINVAL.
>>
>> If the nand-rb property is missing, then fallback to software wait.
>>
>>> 2) I can't test RB mode - I don't have such device :(
>>>
>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values 
>>> are used
>>> in controller specific register for waiting (I guess Meson controller 
>>> has something
>>> like that, but I don't have doc). While in marvell_nand.c it looks 
>>> like that they parse
>>> 'nand-rb' property, but never use it.
>>
>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>> slightly broken or at least badly documented, and thus should not be
>> used.
>>
>>>> In any case you'll need a dt-binding update which must be acked by
>>>> dt-binding maintainers.
>>>
>>> You mean to add this property desc to 
>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?
>>
>> Yes. In a dedicated patch. Something along the lines:
>>
>>          nand-rb: true
>>
>> inside the nand chip object should be fine. And flag the change as a
>> fix because we should have used and parsed this property since the
>> beginning.
>>
>> Thanks,
>> Miquèl
Arseniy Krasnov June 5, 2023, 4:58 p.m. UTC | #7 
On 05.06.2023 16:30, Liang Yang wrote:
> 
> 
> On 2023/6/5 21:19, Liang Yang wrote:
>> Hi Miquel and Arseniy,
>>
>>
>> On 2023/6/5 17:05, Miquel Raynal wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hi Arseniy,
>>>
>>>>>> @@ -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.
>>>>
>>>> I see. So i need to implement processing of this property here? And if it
>>>> is missed -> use software waiting. I think interesting thing will be that:
>>>>
>>>> 1) Even with support of this property here, I really don't know how to pass
>>>>     RB values to this controller - I just have define for RB command and that's
>>>>     it. I found that this property is an array of u32 - IIUC each element is
>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>>>     how to use RB values (although this driver uses software waiting so I'm not
>>>>     sure that I'll find something in it).
>>>
>>> Liang, can you please give use the relevant information here? How do we
>>> target RB0 and RB1? It seems like you use the CS as only information
>>> like if the RB lines where hardwired internally to a CS. Can we invert
>>> the lines with a specific configuration?
>>
>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
>> of different CEs need to be bound into one wire and connect with
>> NAND_RB0 if want to use controller polling rb. the current operating
>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
>>
>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
>>
>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.

Technically no problem! I can use new RB_INT instead of 'nand_soft_waitrdy()' as software fallback, and currently
implemented RB_INT as interrupt driven way. What do You think Miquel ?

> 
> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> 

So I can implement "nand-rb" in dts as boolean value - "false" or missing means use "no physical RB wires", "true" - means use "physical RB wires" ?

Thanks, Arseniy

>>
>>> Arseniy, if the answer to my above question is no, then you should
>>> expect the nand-rb and reg arrays to be identical. If they are not,
>>> then you can return -EINVAL.
>>>
>>> If the nand-rb property is missing, then fallback to software wait.
>>>
>>>> 2) I can't test RB mode - I don't have such device :(
>>>>
>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>>>> in controller specific register for waiting (I guess Meson controller has something
>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>>>> 'nand-rb' property, but never use it.
>>>
>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>>> slightly broken or at least badly documented, and thus should not be
>>> used.
>>>
>>>>> In any case you'll need a dt-binding update which must be acked by
>>>>> dt-binding maintainers.
>>>>
>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?
>>>
>>> Yes. In a dedicated patch. Something along the lines:
>>>
>>>          nand-rb: true
>>>
>>> inside the nand chip object should be fine. And flag the change as a
>>> fix because we should have used and parsed this property since the
>>> beginning.
>>>
>>> Thanks,
>>> Miquèl
Miquel Raynal June 6, 2023, 7:03 a.m. UTC | #8 
Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 19:58:02 +0300:

> On 05.06.2023 16:30, Liang Yang wrote:
> > 
> > 
> > On 2023/6/5 21:19, Liang Yang wrote:  
> >> Hi Miquel and Arseniy,
> >>
> >>
> >> On 2023/6/5 17:05, Miquel Raynal wrote:  
> >>> [ EXTERNAL EMAIL ]
> >>>
> >>> Hi Arseniy,
> >>>  
> >>>>>> @@ -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.  
> >>>>
> >>>> I see. So i need to implement processing of this property here? And if it
> >>>> is missed -> use software waiting. I think interesting thing will be that:
> >>>>
> >>>> 1) Even with support of this property here, I really don't know how to pass
> >>>>     RB values to this controller - I just have define for RB command and that's
> >>>>     it. I found that this property is an array of u32 - IIUC each element is
> >>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
> >>>>     how to use RB values (although this driver uses software waiting so I'm not
> >>>>     sure that I'll find something in it).  
> >>>
> >>> Liang, can you please give use the relevant information here? How do we
> >>> target RB0 and RB1? It seems like you use the CS as only information
> >>> like if the RB lines where hardwired internally to a CS. Can we invert
> >>> the lines with a specific configuration?  
> >>
> >> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
> >> of different CEs need to be bound into one wire and connect with
> >> NAND_RB0 if want to use controller polling rb. the current operating
> >> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
> >>
> >> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
> >>
> >> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.  
> 
> Technically no problem! I can use new RB_INT instead of 'nand_soft_waitrdy()' as software fallback, and currently
> implemented RB_INT as interrupt driven way. What do You think Miquel ?
> 
> > 
> > Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> >   
> 
> So I can implement "nand-rb" in dts as boolean value - "false" or missing means use "no physical RB wires", "true" - means use "physical RB wires" ?

As long as it works and does not contain any extremely strange READ0 or
READ_STATUS in the middle of nothing, I'm fine, take the simplest
approach which will work for all.

> 
> Thanks, Arseniy
> 
> >>  
> >>> Arseniy, if the answer to my above question is no, then you should
> >>> expect the nand-rb and reg arrays to be identical. If they are not,
> >>> then you can return -EINVAL.
> >>>
> >>> If the nand-rb property is missing, then fallback to software wait.
> >>>  
> >>>> 2) I can't test RB mode - I don't have such device :(
> >>>>
> >>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
> >>>> in controller specific register for waiting (I guess Meson controller has something
> >>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
> >>>> 'nand-rb' property, but never use it.  
> >>>
> >>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
> >>> slightly broken or at least badly documented, and thus should not be
> >>> used.
> >>>  
> >>>>> In any case you'll need a dt-binding update which must be acked by
> >>>>> dt-binding maintainers.  
> >>>>
> >>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?  
> >>>
> >>> Yes. In a dedicated patch. Something along the lines:
> >>>
> >>>          nand-rb: true
> >>>
> >>> inside the nand chip object should be fine. And flag the change as a
> >>> fix because we should have used and parsed this property since the
> >>> beginning.
> >>>
> >>> Thanks,
> >>> Miquèl  


Thanks,
Miquèl
Arseniy Krasnov June 6, 2023, 7:40 a.m. UTC | #9 
On 06.06.2023 10:03, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 19:58:02 +0300:
> 
>> On 05.06.2023 16:30, Liang Yang wrote:
>>>
>>>
>>> On 2023/6/5 21:19, Liang Yang wrote:  
>>>> Hi Miquel and Arseniy,
>>>>
>>>>
>>>> On 2023/6/5 17:05, Miquel Raynal wrote:  
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> Hi Arseniy,
>>>>>  
>>>>>>>> @@ -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.  
>>>>>>
>>>>>> I see. So i need to implement processing of this property here? And if it
>>>>>> is missed -> use software waiting. I think interesting thing will be that:
>>>>>>
>>>>>> 1) Even with support of this property here, I really don't know how to pass
>>>>>>     RB values to this controller - I just have define for RB command and that's
>>>>>>     it. I found that this property is an array of u32 - IIUC each element is
>>>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>>>>>     how to use RB values (although this driver uses software waiting so I'm not
>>>>>>     sure that I'll find something in it).  
>>>>>
>>>>> Liang, can you please give use the relevant information here? How do we
>>>>> target RB0 and RB1? It seems like you use the CS as only information
>>>>> like if the RB lines where hardwired internally to a CS. Can we invert
>>>>> the lines with a specific configuration?  
>>>>
>>>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
>>>> of different CEs need to be bound into one wire and connect with
>>>> NAND_RB0 if want to use controller polling rb. the current operating
>>>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
>>>>
>>>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
>>>>
>>>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.  
>>
>> Technically no problem! I can use new RB_INT instead of 'nand_soft_waitrdy()' as software fallback, and currently
>> implemented RB_INT as interrupt driven way. What do You think Miquel ?
>>
>>>
>>> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
>>>   
>>
>> So I can implement "nand-rb" in dts as boolean value - "false" or missing means use "no physical RB wires", "true" - means use "physical RB wires" ?
> 
> As long as it works and does not contain any extremely strange READ0 or
> READ_STATUS in the middle of nothing, I'm fine, take the simplest
> approach which will work for all.

"extremetely strange READ0" is method which uses STATUS, interrupt, READ0? This method was
described by Liang.

And You mean to use the following logic:
if ("nand-rb" == true)
    use RB_INT which requires wire
else
    use 'nand_soft_waitrdy()'

?

Thanks, Arseniy

> 
>>
>> Thanks, Arseniy
>>
>>>>  
>>>>> Arseniy, if the answer to my above question is no, then you should
>>>>> expect the nand-rb and reg arrays to be identical. If they are not,
>>>>> then you can return -EINVAL.
>>>>>
>>>>> If the nand-rb property is missing, then fallback to software wait.
>>>>>  
>>>>>> 2) I can't test RB mode - I don't have such device :(
>>>>>>
>>>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>>>>>> in controller specific register for waiting (I guess Meson controller has something
>>>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>>>>>> 'nand-rb' property, but never use it.  
>>>>>
>>>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>>>>> slightly broken or at least badly documented, and thus should not be
>>>>> used.
>>>>>  
>>>>>>> In any case you'll need a dt-binding update which must be acked by
>>>>>>> dt-binding maintainers.  
>>>>>>
>>>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?  
>>>>>
>>>>> Yes. In a dedicated patch. Something along the lines:
>>>>>
>>>>>          nand-rb: true
>>>>>
>>>>> inside the nand chip object should be fine. And flag the change as a
>>>>> fix because we should have used and parsed this property since the
>>>>> beginning.
>>>>>
>>>>> Thanks,
>>>>> Miquèl  
> 
> 
> Thanks,
> Miquèl
Miquel Raynal June 6, 2023, 7:55 a.m. UTC | #10 
Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 10:40:21 +0300:

> On 06.06.2023 10:03, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 19:58:02 +0300:
> >   
> >> On 05.06.2023 16:30, Liang Yang wrote:  
> >>>
> >>>
> >>> On 2023/6/5 21:19, Liang Yang wrote:    
> >>>> Hi Miquel and Arseniy,
> >>>>
> >>>>
> >>>> On 2023/6/5 17:05, Miquel Raynal wrote:    
> >>>>> [ EXTERNAL EMAIL ]
> >>>>>
> >>>>> Hi Arseniy,
> >>>>>    
> >>>>>>>> @@ -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.    
> >>>>>>
> >>>>>> I see. So i need to implement processing of this property here? And if it
> >>>>>> is missed -> use software waiting. I think interesting thing will be that:
> >>>>>>
> >>>>>> 1) Even with support of this property here, I really don't know how to pass
> >>>>>>     RB values to this controller - I just have define for RB command and that's
> >>>>>>     it. I found that this property is an array of u32 - IIUC each element is
> >>>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
> >>>>>>     how to use RB values (although this driver uses software waiting so I'm not
> >>>>>>     sure that I'll find something in it).    
> >>>>>
> >>>>> Liang, can you please give use the relevant information here? How do we
> >>>>> target RB0 and RB1? It seems like you use the CS as only information
> >>>>> like if the RB lines where hardwired internally to a CS. Can we invert
> >>>>> the lines with a specific configuration?    
> >>>>
> >>>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
> >>>> of different CEs need to be bound into one wire and connect with
> >>>> NAND_RB0 if want to use controller polling rb. the current operating
> >>>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
> >>>>
> >>>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
> >>>>
> >>>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.    
> >>
> >> Technically no problem! I can use new RB_INT instead of 'nand_soft_waitrdy()' as software fallback, and currently
> >> implemented RB_INT as interrupt driven way. What do You think Miquel ?
> >>  
> >>>
> >>> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> >>>     
> >>
> >> So I can implement "nand-rb" in dts as boolean value - "false" or missing means use "no physical RB wires", "true" - means use "physical RB wires" ?  
> > 
> > As long as it works and does not contain any extremely strange READ0 or
> > READ_STATUS in the middle of nothing, I'm fine, take the simplest
> > approach which will work for all.  
> 
> "extremetely strange READ0" is method which uses STATUS, interrupt, READ0? This method was
> described by Liang.

It needs to be very well contained in dedicated helpers and documented.
You choose what is easier for you (Liang's method or
nand_soft_waitrdy()), but I don't want to see spurious READ0 or
READ_STATUS calls inside read/write_page helpers like before.

> And You mean to use the following logic:
> if ("nand-rb" == true)
>     use RB_INT which requires wire
> else
>     use 'nand_soft_waitrdy()'
> 
> ?
> 
> Thanks, Arseniy
> 
> >   
> >>
> >> Thanks, Arseniy
> >>  
> >>>>    
> >>>>> Arseniy, if the answer to my above question is no, then you should
> >>>>> expect the nand-rb and reg arrays to be identical. If they are not,
> >>>>> then you can return -EINVAL.
> >>>>>
> >>>>> If the nand-rb property is missing, then fallback to software wait.
> >>>>>    
> >>>>>> 2) I can't test RB mode - I don't have such device :(
> >>>>>>
> >>>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
> >>>>>> in controller specific register for waiting (I guess Meson controller has something
> >>>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
> >>>>>> 'nand-rb' property, but never use it.    
> >>>>>
> >>>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
> >>>>> slightly broken or at least badly documented, and thus should not be
> >>>>> used.
> >>>>>    
> >>>>>>> In any case you'll need a dt-binding update which must be acked by
> >>>>>>> dt-binding maintainers.    
> >>>>>>
> >>>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?    
> >>>>>
> >>>>> Yes. In a dedicated patch. Something along the lines:
> >>>>>
> >>>>>          nand-rb: true
> >>>>>
> >>>>> inside the nand chip object should be fine. And flag the change as a
> >>>>> fix because we should have used and parsed this property since the
> >>>>> beginning.
> >>>>>
> >>>>> Thanks,
> >>>>> Miquèl    
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl
Arseniy Krasnov June 6, 2023, 11:49 a.m. UTC | #11 
On 05.06.2023 16:30, Liang Yang wrote:
> 
> 
> On 2023/6/5 21:19, Liang Yang wrote:
>> Hi Miquel and Arseniy,
>>
>>
>> On 2023/6/5 17:05, Miquel Raynal wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hi Arseniy,
>>>
>>>>>> @@ -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.
>>>>
>>>> I see. So i need to implement processing of this property here? And if it
>>>> is missed -> use software waiting. I think interesting thing will be that:
>>>>
>>>> 1) Even with support of this property here, I really don't know how to pass
>>>>     RB values to this controller - I just have define for RB command and that's
>>>>     it. I found that this property is an array of u32 - IIUC each element is
>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>>>     how to use RB values (although this driver uses software waiting so I'm not
>>>>     sure that I'll find something in it).
>>>
>>> Liang, can you please give use the relevant information here? How do we
>>> target RB0 and RB1? It seems like you use the CS as only information
>>> like if the RB lines where hardwired internally to a CS. Can we invert
>>> the lines with a specific configuration?
>>
>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
>> of different CEs need to be bound into one wire and connect with
>> NAND_RB0 if want to use controller polling rb. the current operating
>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
>>
>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
>>
>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.
> 
> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> 
>>
>>> Arseniy, if the answer to my above question is no, then you should
>>> expect the nand-rb and reg arrays to be identical. If they are not,
>>> then you can return -EINVAL.
>>>
>>> If the nand-rb property is missing, then fallback to software wait.
>>>
>>>> 2) I can't test RB mode - I don't have such device :(
>>>>
>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>>>> in controller specific register for waiting (I guess Meson controller has something
>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>>>> 'nand-rb' property, but never use it.
>>>
>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>>> slightly broken or at least badly documented, and thus should not be
>>> used.
>>>
>>>>> In any case you'll need a dt-binding update which must be acked by
>>>>> dt-binding maintainers.
>>>>
>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?
>>>
>>> Yes. In a dedicated patch. Something along the lines:
>>>
>>>          nand-rb: true
>>>
>>> inside the nand chip object should be fine. And flag the change as a
>>> fix because we should have used and parsed this property since the
>>> beginning.

Miquel,

Small remark, do we really need to add this 'nand-rb' to the chip object, as Liang said,
that there is only one RB wire (e.g. only one or nothing)? Maybe for Meson I can add it to the
meson controller structure?

Thanks, Arseniy

>>>
>>> Thanks,
>>> Miquèl
Arseniy Krasnov June 6, 2023, 12:10 p.m. UTC | #12 
On 06.06.2023 15:11, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 14:49:19 +0300:
> 
>> On 05.06.2023 16:30, Liang Yang wrote:
>>>
>>>
>>> On 2023/6/5 21:19, Liang Yang wrote:  
>>>> Hi Miquel and Arseniy,
>>>>
>>>>
>>>> On 2023/6/5 17:05, Miquel Raynal wrote:  
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> Hi Arseniy,
>>>>>  
>>>>>>>> @@ -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.  
>>>>>>
>>>>>> I see. So i need to implement processing of this property here? And if it
>>>>>> is missed -> use software waiting. I think interesting thing will be that:
>>>>>>
>>>>>> 1) Even with support of this property here, I really don't know how to pass
>>>>>>     RB values to this controller - I just have define for RB command and that's
>>>>>>     it. I found that this property is an array of u32 - IIUC each element is
>>>>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
>>>>>>     how to use RB values (although this driver uses software waiting so I'm not
>>>>>>     sure that I'll find something in it).  
>>>>>
>>>>> Liang, can you please give use the relevant information here? How do we
>>>>> target RB0 and RB1? It seems like you use the CS as only information
>>>>> like if the RB lines where hardwired internally to a CS. Can we invert
>>>>> the lines with a specific configuration?  
>>>>
>>>> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
>>>> of different CEs need to be bound into one wire and connect with
>>>> NAND_RB0 if want to use controller polling rb. the current operating
>>>> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
>>>>
>>>> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
>>>>
>>>> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.  
>>>
>>> Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
>>>   
>>>>  
>>>>> Arseniy, if the answer to my above question is no, then you should
>>>>> expect the nand-rb and reg arrays to be identical. If they are not,
>>>>> then you can return -EINVAL.
>>>>>
>>>>> If the nand-rb property is missing, then fallback to software wait.
>>>>>  
>>>>>> 2) I can't test RB mode - I don't have such device :(
>>>>>>
>>>>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
>>>>>> in controller specific register for waiting (I guess Meson controller has something
>>>>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
>>>>>> 'nand-rb' property, but never use it.  
>>>>>
>>>>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
>>>>> slightly broken or at least badly documented, and thus should not be
>>>>> used.
>>>>>  
>>>>>>> In any case you'll need a dt-binding update which must be acked by
>>>>>>> dt-binding maintainers.  
>>>>>>
>>>>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?  
>>>>>
>>>>> Yes. In a dedicated patch. Something along the lines:
>>>>>
>>>>>          nand-rb: true
>>>>>
>>>>> inside the nand chip object should be fine. And flag the change as a
>>>>> fix because we should have used and parsed this property since the
>>>>> beginning.  
>>
>> Miquel,
>>
>> Small remark, do we really need to add this 'nand-rb' to the chip object, as Liang said,
>> that there is only one RB wire (e.g. only one or nothing)? Maybe for Meson I can add it to the
>> meson controller structure?
> 
> You only need a boolean in the controller structure, I guess.

Got it!

Thanks, Arseniy

> 
>>
>> Thanks, Arseniy
>>
>>>>>
>>>>> Thanks,
>>>>> Miquèl  
> 
> 
> Thanks,
> Miquèl
Miquel Raynal June 6, 2023, 12:11 p.m. UTC | #13 
Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 14:49:19 +0300:

> On 05.06.2023 16:30, Liang Yang wrote:
> > 
> > 
> > On 2023/6/5 21:19, Liang Yang wrote:  
> >> Hi Miquel and Arseniy,
> >>
> >>
> >> On 2023/6/5 17:05, Miquel Raynal wrote:  
> >>> [ EXTERNAL EMAIL ]
> >>>
> >>> Hi Arseniy,
> >>>  
> >>>>>> @@ -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.  
> >>>>
> >>>> I see. So i need to implement processing of this property here? And if it
> >>>> is missed -> use software waiting. I think interesting thing will be that:
> >>>>
> >>>> 1) Even with support of this property here, I really don't know how to pass
> >>>>     RB values to this controller - I just have define for RB command and that's
> >>>>     it. I found that this property is an array of u32 - IIUC each element is
> >>>>     RB pin per chip. May be i need to dive into the old vendor's driver to find
> >>>>     how to use RB values (although this driver uses software waiting so I'm not
> >>>>     sure that I'll find something in it).  
> >>>
> >>> Liang, can you please give use the relevant information here? How do we
> >>> target RB0 and RB1? It seems like you use the CS as only information
> >>> like if the RB lines where hardwired internally to a CS. Can we invert
> >>> the lines with a specific configuration?  
> >>
> >> Controllor has only one external RB pinmux (NAND_RB0). all the RB pins
> >> of different CEs need to be bound into one wire and connect with
> >> NAND_RB0 if want to use controller polling rb. the current operating
> >> CE of NAND is decided to "chip_select", of course controller internally has different nfc commands to regconize which Ce's RB signal is polling.
> >>
> >> <&nand_pins> in dts/yaml should include the NAND_RB0 if hardware connects, or use software polling here.
> >>
> >> @Arseniy, sorry, i don't travel all the informations yet. but why don't you use the new RB_INT command with irq that i provided in another thread. the new RB_INT command doesn't depend on the physical RB wires, it also send the READ status command(0x70) and wait for the irq wake up completion.  
> > 
> > Use "nand-rb" in dts to decide old RB_INT(physical RB wires is needed) or new RB_INT(no physical RB wires). the new RB_INT command decides the RB0 or RB1 by the previous command with ce args.
> >   
> >>  
> >>> Arseniy, if the answer to my above question is no, then you should
> >>> expect the nand-rb and reg arrays to be identical. If they are not,
> >>> then you can return -EINVAL.
> >>>
> >>> If the nand-rb property is missing, then fallback to software wait.
> >>>  
> >>>> 2) I can't test RB mode - I don't have such device :(
> >>>>
> >>>> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are used
> >>>> in controller specific register for waiting (I guess Meson controller has something
> >>>> like that, but I don't have doc). While in marvell_nand.c it looks like that they parse
> >>>> 'nand-rb' property, but never use it.  
> >>>
> >>> Yes, the logic around the second RB line (taking care of CS1/CS3) is
> >>> slightly broken or at least badly documented, and thus should not be
> >>> used.
> >>>  
> >>>>> In any case you'll need a dt-binding update which must be acked by
> >>>>> dt-binding maintainers.  
> >>>>
> >>>> You mean to add this property desc to Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml ?  
> >>>
> >>> Yes. In a dedicated patch. Something along the lines:
> >>>
> >>>          nand-rb: true
> >>>
> >>> inside the nand chip object should be fine. And flag the change as a
> >>> fix because we should have used and parsed this property since the
> >>> beginning.  
> 
> Miquel,
> 
> Small remark, do we really need to add this 'nand-rb' to the chip object, as Liang said,
> that there is only one RB wire (e.g. only one or nothing)? Maybe for Meson I can add it to the
> meson controller structure?

You only need a boolean in the controller structure, I guess.

> 
> Thanks, Arseniy
> 
> >>>
> >>> Thanks,
> >>> Miquèl  


Thanks,
Miquèl
diff mbox series

Patch

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;
 };
 
 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)
 {
-	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);
+	} 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));
 	} 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");
+
 	writel(0, nfc->reg_base + NFC_REG_CFG);
 	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
 	if (ret) {