[PATCH] fsl_ifc_nand: Added random output enable cmd

[PATCH] fsl_ifc_nand: Added random output enable cmd

[Matt Weber]

This patch adds random output enable command support in IFC nand
controller driver. This command implements change read
column (05h-E0h).

Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
---
 drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 4e9e5fd..230919f 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
 
 		/*
 		 * although currently it's 8 bytes for READID, we always read
-		 * the maximum 256 bytes(for PARAM)
+		 * the maximum 8192 bytes(for PARAM) supported by IFC controller
+		 * as extended page may be available for some NAND devices.
 		 */
-		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
-		ifc_nand_ctrl->read_bytes = 256;
+		ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole page */
+		ifc_nand_ctrl->read_bytes = 8192; /* Maximum supported page by IFC */
 
 		set_addr(mtd, 0, 0, 0);
 		fsl_ifc_run_command(mtd);
@@ -530,6 +531,29 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
 		fsl_ifc_run_command(mtd);
 		return;
 
+	case NAND_CMD_RNDOUT: {
+		__le16 Tccs = 0;
+		chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
+					: (Tccs = chip->jedec_params.t_ccs);
+		ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
+				(IFC_FIR_OP_CA0 << IFC_NAND_FIR0_OP1_SHIFT) |
+				(IFC_FIR_OP_CMD1 << IFC_NAND_FIR0_OP2_SHIFT) |
+				(IFC_FIR_OP_NWAIT << IFC_NAND_FIR0_OP3_SHIFT),
+				&ifc->ifc_nand.nand_fir0);
+
+		ifc_out32((NAND_CMD_RNDOUT << IFC_NAND_FCR0_CMD0_SHIFT) |
+				(NAND_CMD_RNDOUTSTART << IFC_NAND_FCR0_CMD1_SHIFT),
+				&ifc->ifc_nand.nand_fcr0);
+
+		/* Wait for minimum change column set-up time. But it does not harm
+		 * to wait more time, so calculated based on 333.3 MHz input IFC clock
+		 */
+		ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc->ifc_nand.ncfgr);
+		set_addr(mtd, column, 0, 0);
+		fsl_ifc_run_command(mtd);
+		return;
+	}
+
 	default:
 		dev_err(priv->dev, "%s: error, unsupported command 0x%x.\n",
 					__func__, command);
-- 
1.9.1

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Boris Brezillon]

On Tue,  6 Sep 2016 14:13:17 -0500
Matt Weber <matthew.weber@rockwellcollins.com> wrote:

> This patch adds random output enable command support in IFC nand
> controller driver. This command implements change read
> column (05h-E0h).
> 
> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
> ---
>  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 4e9e5fd..230919f 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>  
>  		/*
>  		 * although currently it's 8 bytes for READID, we always read
> -		 * the maximum 256 bytes(for PARAM)
> +		 * the maximum 8192 bytes(for PARAM) supported by IFC controller
> +		 * as extended page may be available for some NAND devices.
>  		 */
> -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> -		ifc_nand_ctrl->read_bytes = 256;
> +		ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole page */
> +		ifc_nand_ctrl->read_bytes = 8192; /* Maximum supported page by IFC */

And this exactly why letting drivers implement their own ->cmdfunc()
method is a bad idea.
->cmdfunc() does not provide enough information to guess how much data
should be read or written. Actually it's not supposed to be used this
way, but drivers usually abuse it.

I know you're just adding support for a new feature here, and I don't
blame you, but this kind of things make the whole NAND framework
impossible to maintain.

>  
>  		set_addr(mtd, 0, 0, 0);
>  		fsl_ifc_run_command(mtd);
> @@ -530,6 +531,29 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>  		fsl_ifc_run_command(mtd);
>  		return;
>  
> +	case NAND_CMD_RNDOUT: {
> +		__le16 Tccs = 0;
> +		chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
> +					: (Tccs = chip->jedec_params.t_ccs);
> +		ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> +				(IFC_FIR_OP_CA0 << IFC_NAND_FIR0_OP1_SHIFT) |
> +				(IFC_FIR_OP_CMD1 << IFC_NAND_FIR0_OP2_SHIFT) |
> +				(IFC_FIR_OP_NWAIT << IFC_NAND_FIR0_OP3_SHIFT),
> +				&ifc->ifc_nand.nand_fir0);
> +
> +		ifc_out32((NAND_CMD_RNDOUT << IFC_NAND_FCR0_CMD0_SHIFT) |
> +				(NAND_CMD_RNDOUTSTART << IFC_NAND_FCR0_CMD1_SHIFT),
> +				&ifc->ifc_nand.nand_fcr0);
> +
> +		/* Wait for minimum change column set-up time. But it does not harm
> +		 * to wait more time, so calculated based on 333.3 MHz input IFC clock
> +		 */

Can't you know the clk rate at runtime instead of basing your
calculation on the hypothetic clk rate?

> +		ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc->ifc_nand.ncfgr);

Why is it done in ->cmdfunc()? I mean, this timing parameter should be
set for all operations, and applied as soon as ->select_chip() is
called.

> +		set_addr(mtd, column, 0, 0);
> +		fsl_ifc_run_command(mtd);
> +		return;
> +	}
> +
>  	default:
>  		dev_err(priv->dev, "%s: error, unsupported command 0x%x.\n",
>  					__func__, command);

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Ronak Desai]

On Tue, Sep 6, 2016 at 2:50 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
>
> On Tue,  6 Sep 2016 14:13:17 -0500
> Matt Weber <matthew.weber@rockwellcollins.com> wrote:
>
> > This patch adds random output enable command support in IFC nand
> > controller driver. This command implements change read
> > column (05h-E0h).
> >
> > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> > Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
> > ---
> >  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> > index 4e9e5fd..230919f 100644
> > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
> >
> >               /*
> >                * although currently it's 8 bytes for READID, we always read
> > -              * the maximum 256 bytes(for PARAM)
> > +              * the maximum 8192 bytes(for PARAM) supported by IFC controller
> > +              * as extended page may be available for some NAND devices.
> >                */
> > -             ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> > -             ifc_nand_ctrl->read_bytes = 256;
> > +             ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole page */
> > +             ifc_nand_ctrl->read_bytes = 8192; /* Maximum supported page by IFC */
>
> And this exactly why letting drivers implement their own ->cmdfunc()
> method is a bad idea.
> ->cmdfunc() does not provide enough information to guess how much data
> should be read or written. Actually it's not supposed to be used this
> way, but drivers usually abuse it.
>
> I know you're just adding support for a new feature here, and I don't
> blame you, but this kind of things make the whole NAND framework
> impossible to maintain.
>
In nand_flash_detect_ext_param_page() function, nand base code
sends NAND_CMD_PARAM and after that sends NAND_CMD_RNDOUT
to skip the ONFI param pages. Now, NAND_CMD_PARAM actually does
not do any actual reading and just requests to change the column pointer
we need to read the extended page somewhere and thus I ended up with
the above change.
>
> >
> >               set_addr(mtd, 0, 0, 0);
> >               fsl_ifc_run_command(mtd);
> > @@ -530,6 +531,29 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
> >               fsl_ifc_run_command(mtd);
> >               return;
> >
> > +     case NAND_CMD_RNDOUT: {
> > +             __le16 Tccs = 0;
> > +             chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
> > +                                     : (Tccs = chip->jedec_params.t_ccs);
> > +             ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> > +                             (IFC_FIR_OP_CA0 << IFC_NAND_FIR0_OP1_SHIFT) |
> > +                             (IFC_FIR_OP_CMD1 << IFC_NAND_FIR0_OP2_SHIFT) |
> > +                             (IFC_FIR_OP_NWAIT << IFC_NAND_FIR0_OP3_SHIFT),
> > +                             &ifc->ifc_nand.nand_fir0);
> > +
> > +             ifc_out32((NAND_CMD_RNDOUT << IFC_NAND_FCR0_CMD0_SHIFT) |
> > +                             (NAND_CMD_RNDOUTSTART << IFC_NAND_FCR0_CMD1_SHIFT),
> > +                             &ifc->ifc_nand.nand_fcr0);
> > +
> > +             /* Wait for minimum change column set-up time. But it does not harm
> > +              * to wait more time, so calculated based on 333.3 MHz input IFC clock
> > +              */
>
> Can't you know the clk rate at runtime instead of basing your
> calculation on the hypothetic clk rate?


IFC input clock is divide by 2 platform clock and for T, P series
and Layerscape series of processors where IFC is used, maximum
platform clock is 600 MHz so IFC input clock becomes 300 MHz maximum.
To avoid floating operation, I chose next possible frequency of
333.33 MHz which gives me period of 3 ns. There was no easy way to find
the input IFC clock without knowing platform clock which requires RCW values
. So, to avoid that I selected this route where we will end up waiting few more
cycles then required but it does not harm. For example, my input IFC clock is
266.66 Mhz and my NAND part tells me to wait minimum 200 ns so I have to
wait for  54 cycles while with this calculation I will be waiting for
66 cycles which
 I think won't matter as the data-sheet specifies minimum change
column set-up time.
>
>
> > +             ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc->ifc_nand.ncfgr);
>
> Why is it done in ->cmdfunc()? I mean, this timing parameter should be
> set for all operations, and applied as soon as ->select_chip() is
> called.
>
I am using NCFGR[NUM_WAIT] (Number of IFC input clock cycles)
 to wait for Tccs time and which gets filled by the base driver when
probing for features. So, I am using tccs value read from NAND device
to calculate wait cycles with maximum possible IFC input clock.

> > +             set_addr(mtd, column, 0, 0);
> > +             fsl_ifc_run_command(mtd);
> > +             return;
> > +     }
> > +
> >       default:
> >               dev_err(priv->dev, "%s: error, unsupported command 0x%x.\n",
> >                                       __func__, command);
>

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Matthew Weber]

Apologize for top post.

Dipen seems to have moved on.  Adding new CC(s).

Cc: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
Cc: Steve Wood  <oss@buserror.net>

On Tue, Sep 6, 2016 at 3:37 PM, Ronak Desai
<ronak.desai@rockwellcollins.com> wrote:
> On Tue, Sep 6, 2016 at 2:50 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
>>
>> On Tue,  6 Sep 2016 14:13:17 -0500
>> Matt Weber <matthew.weber@rockwellcollins.com> wrote:
>>
>> > This patch adds random output enable command support in IFC nand
>> > controller driver. This command implements change read
>> > column (05h-E0h).
>> >
>> > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
>> > Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
>> > ---
>> >  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
>> >  1 file changed, 27 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
>> > index 4e9e5fd..230919f 100644
>> > --- a/drivers/mtd/nand/fsl_ifc_nand.c
>> > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
>> > @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>> >
>> >               /*
>> >                * although currently it's 8 bytes for READID, we always read
>> > -              * the maximum 256 bytes(for PARAM)
>> > +              * the maximum 8192 bytes(for PARAM) supported by IFC controller
>> > +              * as extended page may be available for some NAND devices.
>> >                */
>> > -             ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
>> > -             ifc_nand_ctrl->read_bytes = 256;
>> > +             ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole page */
>> > +             ifc_nand_ctrl->read_bytes = 8192; /* Maximum supported page by IFC */
>>
>> And this exactly why letting drivers implement their own ->cmdfunc()
>> method is a bad idea.
>> ->cmdfunc() does not provide enough information to guess how much data
>> should be read or written. Actually it's not supposed to be used this
>> way, but drivers usually abuse it.
>>
>> I know you're just adding support for a new feature here, and I don't
>> blame you, but this kind of things make the whole NAND framework
>> impossible to maintain.
>>
> In nand_flash_detect_ext_param_page() function, nand base code
> sends NAND_CMD_PARAM and after that sends NAND_CMD_RNDOUT
> to skip the ONFI param pages. Now, NAND_CMD_PARAM actually does
> not do any actual reading and just requests to change the column pointer
> we need to read the extended page somewhere and thus I ended up with
> the above change.
>>
>> >
>> >               set_addr(mtd, 0, 0, 0);
>> >               fsl_ifc_run_command(mtd);
>> > @@ -530,6 +531,29 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>> >               fsl_ifc_run_command(mtd);
>> >               return;
>> >
>> > +     case NAND_CMD_RNDOUT: {
>> > +             __le16 Tccs = 0;
>> > +             chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
>> > +                                     : (Tccs = chip->jedec_params.t_ccs);
>> > +             ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
>> > +                             (IFC_FIR_OP_CA0 << IFC_NAND_FIR0_OP1_SHIFT) |
>> > +                             (IFC_FIR_OP_CMD1 << IFC_NAND_FIR0_OP2_SHIFT) |
>> > +                             (IFC_FIR_OP_NWAIT << IFC_NAND_FIR0_OP3_SHIFT),
>> > +                             &ifc->ifc_nand.nand_fir0);
>> > +
>> > +             ifc_out32((NAND_CMD_RNDOUT << IFC_NAND_FCR0_CMD0_SHIFT) |
>> > +                             (NAND_CMD_RNDOUTSTART << IFC_NAND_FCR0_CMD1_SHIFT),
>> > +                             &ifc->ifc_nand.nand_fcr0);
>> > +
>> > +             /* Wait for minimum change column set-up time. But it does not harm
>> > +              * to wait more time, so calculated based on 333.3 MHz input IFC clock
>> > +              */
>>
>> Can't you know the clk rate at runtime instead of basing your
>> calculation on the hypothetic clk rate?
>
>
> IFC input clock is divide by 2 platform clock and for T, P series
> and Layerscape series of processors where IFC is used, maximum
> platform clock is 600 MHz so IFC input clock becomes 300 MHz maximum.
> To avoid floating operation, I chose next possible frequency of
> 333.33 MHz which gives me period of 3 ns. There was no easy way to find
> the input IFC clock without knowing platform clock which requires RCW values
> . So, to avoid that I selected this route where we will end up waiting few more
> cycles then required but it does not harm. For example, my input IFC clock is
> 266.66 Mhz and my NAND part tells me to wait minimum 200 ns so I have to
> wait for  54 cycles while with this calculation I will be waiting for
> 66 cycles which
>  I think won't matter as the data-sheet specifies minimum change
> column set-up time.
>>
>>
>> > +             ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc->ifc_nand.ncfgr);
>>
>> Why is it done in ->cmdfunc()? I mean, this timing parameter should be
>> set for all operations, and applied as soon as ->select_chip() is
>> called.
>>
> I am using NCFGR[NUM_WAIT] (Number of IFC input clock cycles)
>  to wait for Tccs time and which gets filled by the base driver when
> probing for features. So, I am using tccs value read from NAND device
> to calculate wait cycles with maximum possible IFC input clock.
>
>> > +             set_addr(mtd, column, 0, 0);
>> > +             fsl_ifc_run_command(mtd);
>> > +             return;
>> > +     }
>> > +
>> >       default:
>> >               dev_err(priv->dev, "%s: error, unsupported command 0x%x.\n",
>> >                                       __func__, command);
>>



-- 
Matthew L Weber / Pr Software Engineer
Airborne Information Systems / Security Systems and Software / Secure Platforms
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
www.rockwellcollins.com

Note: Any Export License Required Information and License Restricted
Third Party Intellectual Property (TPIP) content must be encrypted and
sent to matthew.weber@corp.rockwellcollins.com.

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Scott Wood]

On Tue, 2016-09-06 at 21:50 +0200, Boris Brezillon wrote:
> On Tue,  6 Sep 2016 14:13:17 -0500
> Matt Weber <matthew.weber@rockwellcollins.com> wrote:
> 
> > 
> > This patch adds random output enable command support in IFC nand
> > controller driver. This command implements change read
> > column (05h-E0h).
> > 
> > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> > Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
> > ---
> >  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c
> > b/drivers/mtd/nand/fsl_ifc_nand.c
> > index 4e9e5fd..230919f 100644
> > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> > unsigned int command,
> >  
> >  		/*
> >  		 * although currently it's 8 bytes for READID, we always
> > read
> > -		 * the maximum 256 bytes(for PARAM)
> > +		 * the maximum 8192 bytes(for PARAM) supported by IFC
> > controller
> > +		 * as extended page may be available for some NAND
> > devices.
> >  		 */
> > -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> > -		ifc_nand_ctrl->read_bytes = 256;
> > +		ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole
> > page */
> > +		ifc_nand_ctrl->read_bytes = 8192; /* Maximum supported
> > page by IFC */

Are you sure that reading 8192 bytes will work if the configured page size is
smaller?

> And this exactly why letting drivers implement their own ->cmdfunc()
> method is a bad idea.
> ->cmdfunc() does not provide enough information to guess how much data
> should be read or written. Actually it's not supposed to be used this
> way, but drivers usually abuse it.

It would be even uglier if we used the standard nand_command_lp() and had to
abuse cmd_ctrl() instead. :-P

The current NAND driver interface is too low level for controllers that expose
higher level interfaces.  If we can't/shouldn't replace cmdfunc() then we need
to replace the things that call cmdfunc().  Yes, we probably should have
pushed for a better interface back then rather than just "making it work"...

> I know you're just adding support for a new feature here, and I don't
> blame you, but this kind of things make the whole NAND framework
> impossible to maintain.
> 
> > 
> >  
> >  		set_addr(mtd, 0, 0, 0);
> >  		fsl_ifc_run_command(mtd);
> > @@ -530,6 +531,29 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> > unsigned int command,
> >  		fsl_ifc_run_command(mtd);
> >  		return;
> >  
> > +	case NAND_CMD_RNDOUT: {
> > +		__le16 Tccs = 0;
> > +		chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
> > +					: (Tccs = chip-
> > >jedec_params.t_ccs);

Please don't put assignments inside a conditional.  An if-statement would work
just fine here.

> > +		ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> > +				(IFC_FIR_OP_CA0 <<
> > IFC_NAND_FIR0_OP1_SHIFT) |
> > +				(IFC_FIR_OP_CMD1 <<
> > IFC_NAND_FIR0_OP2_SHIFT) |
> > +				(IFC_FIR_OP_NWAIT <<
> > IFC_NAND_FIR0_OP3_SHIFT),
> > +				&ifc->ifc_nand.nand_fir0);
> > +
> > +		ifc_out32((NAND_CMD_RNDOUT << IFC_NAND_FCR0_CMD0_SHIFT) |
> > +				(NAND_CMD_RNDOUTSTART <<
> > IFC_NAND_FCR0_CMD1_SHIFT),
> > +				&ifc->ifc_nand.nand_fcr0);
> > +
> > +		/* Wait for minimum change column set-up time. But it
> > does not harm
> > +		 * to wait more time, so calculated based on 333.3 MHz
> > input IFC clock
> > +		 */
> Can't you know the clk rate at runtime instead of basing your
> calculation on the hypothetic clk rate?

If we really need to know the clock rate, we could add a clocks property to
the IFC node.  But we haven't needed to deal with clocking anywhere else in
this driver, so I'm not too happy about introducing it for this.  In
particular, I don't think we should be sending a real RNDOUT command at the
device here -- it breaks the model of self-contained operations.  The read
command is over and the chipselect has been dropped (do all ONFI chips support
"CE don't care"?).  If the new offset (plus size to be read) fits within the
page buffer, we could just adjust ifc_nand_ctrl->index to the desired
location.

OTOH, if we need to read parameter data beyond the size of the page buffer,
then we'd need to send another read command (possibly from read_buf)... or we
can do the sane thing and introduce an interface function to read a certain
number of parameter bytes from a certain offset.  Which we should do anyway if
we want to move in the direction of a better interface with less cmdfunc
abuse.

> 
> > +		ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc-
> > >ifc_nand.ncfgr);
> Why is it done in ->cmdfunc()? I mean, this timing parameter should be
> set for all operations, and applied as soon as ->select_chip() is
> called.

select_chip() is a no-op[1].  IFC normally handles the delays internally when
running a command sequence.

-Scott

[1]  In any case, this is the time between RNDOUT and reading the data, not
the time between asserting the chipselect and issuing a command -- and I don't
see other drivers doing a delay in select_chip().

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Boris Brezillon]

Hi Scott,

On Wed, 07 Sep 2016 01:03:53 -0500
Scott Wood <oss@buserror.net> wrote:

> On Tue, 2016-09-06 at 21:50 +0200, Boris Brezillon wrote:
> > On Tue,  6 Sep 2016 14:13:17 -0500
> > Matt Weber <matthew.weber@rockwellcollins.com> wrote:
> >   
> > > 
> > > This patch adds random output enable command support in IFC nand
> > > controller driver. This command implements change read
> > > column (05h-E0h).
> > > 
> > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> > > Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
> > > ---
> > >  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
> > >  1 file changed, 27 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c
> > > b/drivers/mtd/nand/fsl_ifc_nand.c
> > > index 4e9e5fd..230919f 100644
> > > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > > @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> > > unsigned int command,
> > >  
> > >  		/*
> > >  		 * although currently it's 8 bytes for READID, we always
> > > read
> > > -		 * the maximum 256 bytes(for PARAM)
> > > +		 * the maximum 8192 bytes(for PARAM) supported by IFC
> > > controller
> > > +		 * as extended page may be available for some NAND
> > > devices.
> > >  		 */
> > > -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> > > -		ifc_nand_ctrl->read_bytes = 256;
> > > +		ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole
> > > page */
> > > +		ifc_nand_ctrl->read_bytes = 8192; /* Maximum supported
> > > page by IFC */  
> 
> Are you sure that reading 8192 bytes will work if the configured page size is
> smaller?

I'm pretty sure it will, but you will either get garbage or have the
same content duplicated in your buffer.

> 
> > And this exactly why letting drivers implement their own ->cmdfunc()
> > method is a bad idea.  
> > ->cmdfunc() does not provide enough information to guess how much data  
> > should be read or written. Actually it's not supposed to be used this
> > way, but drivers usually abuse it.  
> 
> It would be even uglier if we used the standard nand_command_lp() and had to
> abuse cmd_ctrl() instead. :-P

Well, I can't say, I haven't read the whole driver. But I've been said
so many times that ->cmd_ctrl() was not a good solution and in the end
it appeared to be untrue (you don't necessarily have to issue the CMD
and ADDR cycles right away, you can cache them and send the whole
sequence once the command parameter is CMD_NONE).

Anyway, the problem is not really whether ->cmd_ctrl() should be used
instead of ->cmdfunc() here. The thing is, you're doing the I/Os in a
place where you don't know how many bytes should be retrieved.

If we stick to the ->cmdfunc() + ->{read,write}_buf() model (which is
definitely inappropriate for most controllers), the data transfer should
be done in ->{read,write}_buf() (where you have the size information).

Actually, the PXA driver is doing pretty much the same thing as you do,
and I remember that they had to tweak the size they were reading in
->cmdfunc() because the core decided to read more data at some point.

> 
> The current NAND driver interface is too low level for controllers that expose
> higher level interfaces.

That's for sure.

> If we can't/shouldn't replace cmdfunc() then we need
> to replace the things that call cmdfunc().  Yes, we probably should have
> pushed for a better interface back then rather than just "making it work"...

No, let's not add more random hacks. I was just complaining because I
see more and more drivers implementing their own ->cmdfunc() and adding
this kind of hacks.
Most of them can implement ->cmd_ctrl() and rely on the default
nand_command_lp(), but I agree that some controllers do not fit in this
model.

For these ones, I was planning to provide a ->send_op(chip, nand_op)
method, where nand_op would contain both the CMD, ADDR cycles and
the data transfer.

> 
> > I know you're just adding support for a new feature here, and I don't
> > blame you, but this kind of things make the whole NAND framework
> > impossible to maintain.
> >   
> > > 
> > >  
> > >  		set_addr(mtd, 0, 0, 0);
> > >  		fsl_ifc_run_command(mtd);
> > > @@ -530,6 +531,29 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> > > unsigned int command,
> > >  		fsl_ifc_run_command(mtd);
> > >  		return;
> > >  
> > > +	case NAND_CMD_RNDOUT: {
> > > +		__le16 Tccs = 0;
> > > +		chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
> > > +					: (Tccs = chip-  
> > > >jedec_params.t_ccs);  
> 
> Please don't put assignments inside a conditional.  An if-statement would work
> just fine here.
> 
> > > +		ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> > > +				(IFC_FIR_OP_CA0 <<
> > > IFC_NAND_FIR0_OP1_SHIFT) |
> > > +				(IFC_FIR_OP_CMD1 <<
> > > IFC_NAND_FIR0_OP2_SHIFT) |
> > > +				(IFC_FIR_OP_NWAIT <<
> > > IFC_NAND_FIR0_OP3_SHIFT),
> > > +				&ifc->ifc_nand.nand_fir0);
> > > +
> > > +		ifc_out32((NAND_CMD_RNDOUT << IFC_NAND_FCR0_CMD0_SHIFT) |
> > > +				(NAND_CMD_RNDOUTSTART <<
> > > IFC_NAND_FCR0_CMD1_SHIFT),
> > > +				&ifc->ifc_nand.nand_fcr0);
> > > +
> > > +		/* Wait for minimum change column set-up time. But it
> > > does not harm
> > > +		 * to wait more time, so calculated based on 333.3 MHz
> > > input IFC clock
> > > +		 */  
> > Can't you know the clk rate at runtime instead of basing your
> > calculation on the hypothetic clk rate?  
> 
> If we really need to know the clock rate, we could add a clocks property to
> the IFC node.  But we haven't needed to deal with clocking anywhere else in
> this driver, so I'm not too happy about introducing it for this.

Will the base clk always be running at the same rate on all designs? To
me, it sounds a bit risky to do this assumption, but maybe I'm wrong.

> In
> particular, I don't think we should be sending a real RNDOUT command at the
> device here -- it breaks the model of self-contained operations.  The read
> command is over and the chipselect has been dropped (do all ONFI chips support
> "CE don't care"?).  If the new offset (plus size to be read) fits within the
> page buffer, we could just adjust ifc_nand_ctrl->index to the desired
> location.

It should be supported (releasing the CE after the PARAM command and
then sending a RNDOUT), but who knows what NAND vendors decide to
implement.

> 
> OTOH, if we need to read parameter data beyond the size of the page buffer,
> then we'd need to send another read command (possibly from read_buf)... or we
> can do the sane thing and introduce an interface function to read a certain
> number of parameter bytes from a certain offset.

No please, do not add any complex logic in read_buf() (like resending a
NAND cmd from there), it will just make the whole thing worst.

> Which we should do anyway if
> we want to move in the direction of a better interface with less cmdfunc
> abuse.

Not really. Ideally, ->read_buf() should just launch the I/O transfer
(same for ->write_buf()), nothing more. It shouldn't have to test which
command was sent before and adapt its behavior.
Now, I know that some controllers are not able to dissociate the
CMD/ADDR cycles from the DATA transfers, which is why a new interface
is needed.

> 
> >   
> > > +		ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc-  
> > > >ifc_nand.ncfgr);  
> > Why is it done in ->cmdfunc()? I mean, this timing parameter should be
> > set for all operations, and applied as soon as ->select_chip() is
> > called.  
> 
> select_chip() is a no-op[1].

Maybe it is in your implementation, but this is where you should have
all the CHIP specific settings (switching from one NAND chip to another
requires adapting the timing, setting a new CS line, ...).
The what the sunxi_nand driver is doing if you want an example where
the driver is tweaking the timing config at chip selection time.

> IFC normally handles the delays internally when
> running a command sequence.

Same for a lot of controllers out there, but that doesn't prevent you
from configuring the timings (on the controller side) when a chip is
selected.

Note that ->select_chip() is not implying that you assert the CE line,
it's here to inform your controller driver that the core wants to
interact with a different NAND chip. What's done in there is completely
controller dependent.

> 
> -Scott
> 
> [1]  In any case, this is the time between RNDOUT and reading the data, not
> the time between asserting the chipselect and issuing a command -- and I don't
> see other drivers doing a delay in select_chip().

It's not about applying the delay in ->select_chip() it's about
configuring the chip specific timings on the controller side. I guess
the first byte of the ncfgr register is always encoding the tCCS
timing, not something different depending on the command that is sent.
If that's the case, then you don't need to reconfigure the register
each time you send the RNDOUT command, only once, when you activate a
chip.

But again, I don't know this controller, so I might be wrong.

Regards,

Boris

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Ronak Desai]

On Wed, Sep 7, 2016 at 2:22 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Scott,
>
> On Wed, 07 Sep 2016 01:03:53 -0500
> Scott Wood <oss@buserror.net> wrote:
>
>> On Tue, 2016-09-06 at 21:50 +0200, Boris Brezillon wrote:
>> > On Tue,  6 Sep 2016 14:13:17 -0500
>> > Matt Weber <matthew.weber@rockwellcollins.com> wrote:
>> >
>> > >
>> > > This patch adds random output enable command support in IFC nand
>> > > controller driver. This command implements change read
>> > > column (05h-E0h).
>> > >
>> > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
>> > > Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
>> > > ---
>> > >  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
>> > >  1 file changed, 27 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c
>> > > b/drivers/mtd/nand/fsl_ifc_nand.c
>> > > index 4e9e5fd..230919f 100644
>> > > --- a/drivers/mtd/nand/fsl_ifc_nand.c
>> > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
>> > > @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
>> > > unsigned int command,
>> > >
>> > >           /*
>> > >            * although currently it's 8 bytes for READID, we always
>> > > read
>> > > -          * the maximum 256 bytes(for PARAM)
>> > > +          * the maximum 8192 bytes(for PARAM) supported by IFC
>> > > controller
>> > > +          * as extended page may be available for some NAND
>> > > devices.
>> > >            */
>> > > -         ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
>> > > -         ifc_nand_ctrl->read_bytes = 256;
>> > > +         ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole
>> > > page */
>> > > +         ifc_nand_ctrl->read_bytes = 8192; /* Maximum supported
>> > > page by IFC */
>>
>> Are you sure that reading 8192 bytes will work if the configured page size is
>> smaller?
>
> I'm pretty sure it will, but you will either get garbage or have the
> same content duplicated in your buffer.
>
Yes, it works fine. I have tested it with 2 KB page size NAND device.
>>
>> > And this exactly why letting drivers implement their own ->cmdfunc()
>> > method is a bad idea.
>> > ->cmdfunc() does not provide enough information to guess how much data
>> > should be read or written. Actually it's not supposed to be used this
>> > way, but drivers usually abuse it.
>>
>> It would be even uglier if we used the standard nand_command_lp() and had to
>> abuse cmd_ctrl() instead. :-P
>
> Well, I can't say, I haven't read the whole driver. But I've been said
> so many times that ->cmd_ctrl() was not a good solution and in the end
> it appeared to be untrue (you don't necessarily have to issue the CMD
> and ADDR cycles right away, you can cache them and send the whole
> sequence once the command parameter is CMD_NONE).
>
> Anyway, the problem is not really whether ->cmd_ctrl() should be used
> instead of ->cmdfunc() here. The thing is, you're doing the I/Os in a
> place where you don't know how many bytes should be retrieved.
>
> If we stick to the ->cmdfunc() + ->{read,write}_buf() model (which is
> definitely inappropriate for most controllers), the data transfer should
> be done in ->{read,write}_buf() (where you have the size information).
>
> Actually, the PXA driver is doing pretty much the same thing as you do,
> and I remember that they had to tweak the size they were reading in
> ->cmdfunc() because the core decided to read more data at some point.
>
>>
>> The current NAND driver interface is too low level for controllers that expose
>> higher level interfaces.
>
> That's for sure.
>
>> If we can't/shouldn't replace cmdfunc() then we need
>> to replace the things that call cmdfunc().  Yes, we probably should have
>> pushed for a better interface back then rather than just "making it work"...
>
> No, let's not add more random hacks. I was just complaining because I
> see more and more drivers implementing their own ->cmdfunc() and adding
> this kind of hacks.
> Most of them can implement ->cmd_ctrl() and rely on the default
> nand_command_lp(), but I agree that some controllers do not fit in this
> model.
>
> For these ones, I was planning to provide a ->send_op(chip, nand_op)
> method, where nand_op would contain both the CMD, ADDR cycles and
> the data transfer.
>
>>
>> > I know you're just adding support for a new feature here, and I don't
>> > blame you, but this kind of things make the whole NAND framework
>> > impossible to maintain.
>> >
>> > >
>> > >
>> > >           set_addr(mtd, 0, 0, 0);
>> > >           fsl_ifc_run_command(mtd);
>> > > @@ -530,6 +531,29 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
>> > > unsigned int command,
>> > >           fsl_ifc_run_command(mtd);
>> > >           return;
>> > >
>> > > + case NAND_CMD_RNDOUT: {
>> > > +         __le16 Tccs = 0;
>> > > +         chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
>> > > +                                 : (Tccs = chip-
>> > > >jedec_params.t_ccs);
>>
>> Please don't put assignments inside a conditional.  An if-statement would work
>> just fine here.
>>
>> > > +         ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
>> > > +                         (IFC_FIR_OP_CA0 <<
>> > > IFC_NAND_FIR0_OP1_SHIFT) |
>> > > +                         (IFC_FIR_OP_CMD1 <<
>> > > IFC_NAND_FIR0_OP2_SHIFT) |
>> > > +                         (IFC_FIR_OP_NWAIT <<
>> > > IFC_NAND_FIR0_OP3_SHIFT),
>> > > +                         &ifc->ifc_nand.nand_fir0);
>> > > +
>> > > +         ifc_out32((NAND_CMD_RNDOUT << IFC_NAND_FCR0_CMD0_SHIFT) |
>> > > +                         (NAND_CMD_RNDOUTSTART <<
>> > > IFC_NAND_FCR0_CMD1_SHIFT),
>> > > +                         &ifc->ifc_nand.nand_fcr0);
>> > > +
>> > > +         /* Wait for minimum change column set-up time. But it
>> > > does not harm
>> > > +          * to wait more time, so calculated based on 333.3 MHz
>> > > input IFC clock
>> > > +          */
>> > Can't you know the clk rate at runtime instead of basing your
>> > calculation on the hypothetic clk rate?
>>
>> If we really need to know the clock rate, we could add a clocks property to
>> the IFC node.  But we haven't needed to deal with clocking anywhere else in
>> this driver, so I'm not too happy about introducing it for this.
>
> Will the base clk always be running at the same rate on all designs? To
> me, it sounds a bit risky to do this assumption, but maybe I'm wrong.
>
IFC input clock is divide by 2 platform clock which is fixed in T, P series
and Layerscape series of processors where IFC is used, maximum
platform clock is 600 MHz so IFC input clock becomes 300 MHz maximum.
To avoid floating operation, I chose next possible frequency of
333.33 MHz which gives me period of 3 ns. There was no easy way to find
the input IFC clock without knowing platform clock which requires RCW values.
So, to avoid that I selected this route where we will end up waiting few more
cycles then required but it does not harm. For example, my input IFC clock is
266.66 Mhz and my NAND part tells me to wait minimum 200 ns so I have to
wait for  54 cycles while with this calculation I will be waiting for 66 cycles
which I think won't matter as the data-sheet specifies minimum change column
set-up time.

>> In
>> particular, I don't think we should be sending a real RNDOUT command at the
>> device here -- it breaks the model of self-contained operations.  The read
>> command is over and the chipselect has been dropped (do all ONFI chips support
>> "CE don't care"?).  If the new offset (plus size to be read) fits within the
>> page buffer, we could just adjust ifc_nand_ctrl->index to the desired
>> location.
>
> It should be supported (releasing the CE after the PARAM command and
> then sending a RNDOUT), but who knows what NAND vendors decide to
> implement.
>
Initially I though of just adjusting the index as we don't do any read operation
again and already we read the required bytes.
So, what should be the final take on this ? Should I just update the index and
avoid sending the change column command at all? Or should I keep the existing
implementation ?
>>
>> OTOH, if we need to read parameter data beyond the size of the page buffer,
>> then we'd need to send another read command (possibly from read_buf)... or we
>> can do the sane thing and introduce an interface function to read a certain
>> number of parameter bytes from a certain offset.
>
> No please, do not add any complex logic in read_buf() (like resending a
> NAND cmd from there), it will just make the whole thing worst.
>
>> Which we should do anyway if
>> we want to move in the direction of a better interface with less cmdfunc
>> abuse.
>
> Not really. Ideally, ->read_buf() should just launch the I/O transfer
> (same for ->write_buf()), nothing more. It shouldn't have to test which
> command was sent before and adapt its behavior.
> Now, I know that some controllers are not able to dissociate the
> CMD/ADDR cycles from the DATA transfers, which is why a new interface
> is needed.
>
>>
>> >
>> > > +         ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc-
>> > > >ifc_nand.ncfgr);
>> > Why is it done in ->cmdfunc()? I mean, this timing parameter should be
>> > set for all operations, and applied as soon as ->select_chip() is
>> > called.
>>
>> select_chip() is a no-op[1].
>
> Maybe it is in your implementation, but this is where you should have
> all the CHIP specific settings (switching from one NAND chip to another
> requires adapting the timing, setting a new CS line, ...).
> The what the sunxi_nand driver is doing if you want an example where
> the driver is tweaking the timing config at chip selection time.
>
>> IFC normally handles the delays internally when
>> running a command sequence.
>
> Same for a lot of controllers out there, but that doesn't prevent you
> from configuring the timings (on the controller side) when a chip is
> selected.
>
> Note that ->select_chip() is not implying that you assert the CE line,
> it's here to inform your controller driver that the core wants to
> interact with a different NAND chip. What's done in there is completely
> controller dependent.
>
>>
>> -Scott
>>
>> [1]  In any case, this is the time between RNDOUT and reading the data, not
>> the time between asserting the chipselect and issuing a command -- and I don't
>> see other drivers doing a delay in select_chip().
>
> It's not about applying the delay in ->select_chip() it's about
> configuring the chip specific timings on the controller side. I guess
> the first byte of the ncfgr register is always encoding the tCCS
> timing, not something different depending on the command that is sent.
> If that's the case, then you don't need to reconfigure the register
> each time you send the RNDOUT command, only once, when you activate a
> chip.
>
> But again, I don't know this controller, so I might be wrong.

NCFGR may be used in future to wait for custom number of clock cycles, so I
don't think it's a good idea to to configure tCCS time into it only
once during init.
>
> Regards,
>
> Boris
>
Best Regards,
Ronak

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Boris Brezillon]

On Wed, 7 Sep 2016 09:50:50 -0500
Ronak Desai <ronak.desai@rockwellcollins.com> wrote:

> On Wed, Sep 7, 2016 at 2:22 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Scott,
> >
> > On Wed, 07 Sep 2016 01:03:53 -0500
> > Scott Wood <oss@buserror.net> wrote:
> >  
> >> On Tue, 2016-09-06 at 21:50 +0200, Boris Brezillon wrote:  
> >> > On Tue,  6 Sep 2016 14:13:17 -0500
> >> > Matt Weber <matthew.weber@rockwellcollins.com> wrote:
> >> >  
> >> > >
> >> > > This patch adds random output enable command support in IFC nand
> >> > > controller driver. This command implements change read
> >> > > column (05h-E0h).
> >> > >
> >> > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> >> > > Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
> >> > > ---
> >> > >  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
> >> > >  1 file changed, 27 insertions(+), 3 deletions(-)
> >> > >
> >> > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c
> >> > > b/drivers/mtd/nand/fsl_ifc_nand.c
> >> > > index 4e9e5fd..230919f 100644
> >> > > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> >> > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> >> > > @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> >> > > unsigned int command,
> >> > >
> >> > >           /*
> >> > >            * although currently it's 8 bytes for READID, we always
> >> > > read
> >> > > -          * the maximum 256 bytes(for PARAM)
> >> > > +          * the maximum 8192 bytes(for PARAM) supported by IFC
> >> > > controller
> >> > > +          * as extended page may be available for some NAND
> >> > > devices.
> >> > >            */
> >> > > -         ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> >> > > -         ifc_nand_ctrl->read_bytes = 256;
> >> > > +         ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole
> >> > > page */
> >> > > +         ifc_nand_ctrl->read_bytes = 8192; /* Maximum supported
> >> > > page by IFC */  
> >>
> >> Are you sure that reading 8192 bytes will work if the configured page size is
> >> smaller?  
> >
> > I'm pretty sure it will, but you will either get garbage or have the
> > same content duplicated in your buffer.
> >  
> Yes, it works fine. I have tested it with 2 KB page size NAND device.
> >>  
> >> > And this exactly why letting drivers implement their own ->cmdfunc()
> >> > method is a bad idea.  
> >> > ->cmdfunc() does not provide enough information to guess how much data  
> >> > should be read or written. Actually it's not supposed to be used this
> >> > way, but drivers usually abuse it.  
> >>
> >> It would be even uglier if we used the standard nand_command_lp() and had to
> >> abuse cmd_ctrl() instead. :-P  
> >
> > Well, I can't say, I haven't read the whole driver. But I've been said
> > so many times that ->cmd_ctrl() was not a good solution and in the end
> > it appeared to be untrue (you don't necessarily have to issue the CMD
> > and ADDR cycles right away, you can cache them and send the whole
> > sequence once the command parameter is CMD_NONE).
> >
> > Anyway, the problem is not really whether ->cmd_ctrl() should be used
> > instead of ->cmdfunc() here. The thing is, you're doing the I/Os in a
> > place where you don't know how many bytes should be retrieved.
> >
> > If we stick to the ->cmdfunc() + ->{read,write}_buf() model (which is
> > definitely inappropriate for most controllers), the data transfer should
> > be done in ->{read,write}_buf() (where you have the size information).
> >
> > Actually, the PXA driver is doing pretty much the same thing as you do,
> > and I remember that they had to tweak the size they were reading in  
> > ->cmdfunc() because the core decided to read more data at some point.  
> >  
> >>
> >> The current NAND driver interface is too low level for controllers that expose
> >> higher level interfaces.  
> >
> > That's for sure.
> >  
> >> If we can't/shouldn't replace cmdfunc() then we need
> >> to replace the things that call cmdfunc().  Yes, we probably should have
> >> pushed for a better interface back then rather than just "making it work"...  
> >
> > No, let's not add more random hacks. I was just complaining because I
> > see more and more drivers implementing their own ->cmdfunc() and adding
> > this kind of hacks.
> > Most of them can implement ->cmd_ctrl() and rely on the default
> > nand_command_lp(), but I agree that some controllers do not fit in this
> > model.
> >
> > For these ones, I was planning to provide a ->send_op(chip, nand_op)
> > method, where nand_op would contain both the CMD, ADDR cycles and
> > the data transfer.
> >  
> >>  
> >> > I know you're just adding support for a new feature here, and I don't
> >> > blame you, but this kind of things make the whole NAND framework
> >> > impossible to maintain.
> >> >  
> >> > >
> >> > >
> >> > >           set_addr(mtd, 0, 0, 0);
> >> > >           fsl_ifc_run_command(mtd);
> >> > > @@ -530,6 +531,29 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> >> > > unsigned int command,
> >> > >           fsl_ifc_run_command(mtd);
> >> > >           return;
> >> > >
> >> > > + case NAND_CMD_RNDOUT: {
> >> > > +         __le16 Tccs = 0;
> >> > > +         chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
> >> > > +                                 : (Tccs = chip-  
> >> > > >jedec_params.t_ccs);  
> >>
> >> Please don't put assignments inside a conditional.  An if-statement would work
> >> just fine here.
> >>  
> >> > > +         ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> >> > > +                         (IFC_FIR_OP_CA0 <<
> >> > > IFC_NAND_FIR0_OP1_SHIFT) |
> >> > > +                         (IFC_FIR_OP_CMD1 <<
> >> > > IFC_NAND_FIR0_OP2_SHIFT) |
> >> > > +                         (IFC_FIR_OP_NWAIT <<
> >> > > IFC_NAND_FIR0_OP3_SHIFT),
> >> > > +                         &ifc->ifc_nand.nand_fir0);
> >> > > +
> >> > > +         ifc_out32((NAND_CMD_RNDOUT << IFC_NAND_FCR0_CMD0_SHIFT) |
> >> > > +                         (NAND_CMD_RNDOUTSTART <<
> >> > > IFC_NAND_FCR0_CMD1_SHIFT),
> >> > > +                         &ifc->ifc_nand.nand_fcr0);
> >> > > +
> >> > > +         /* Wait for minimum change column set-up time. But it
> >> > > does not harm
> >> > > +          * to wait more time, so calculated based on 333.3 MHz
> >> > > input IFC clock
> >> > > +          */  
> >> > Can't you know the clk rate at runtime instead of basing your
> >> > calculation on the hypothetic clk rate?  
> >>
> >> If we really need to know the clock rate, we could add a clocks property to
> >> the IFC node.  But we haven't needed to deal with clocking anywhere else in
> >> this driver, so I'm not too happy about introducing it for this.  
> >
> > Will the base clk always be running at the same rate on all designs? To
> > me, it sounds a bit risky to do this assumption, but maybe I'm wrong.
> >  
> IFC input clock is divide by 2 platform clock which is fixed in T, P series
> and Layerscape series of processors where IFC is used, maximum
> platform clock is 600 MHz so IFC input clock becomes 300 MHz maximum.
> To avoid floating operation, I chose next possible frequency of
> 333.33 MHz which gives me period of 3 ns. There was no easy way to find
> the input IFC clock without knowing platform clock which requires RCW values.
> So, to avoid that I selected this route where we will end up waiting few more
> cycles then required but it does not harm. For example, my input IFC clock is
> 266.66 Mhz and my NAND part tells me to wait minimum 200 ns so I have to
> wait for  54 cycles while with this calculation I will be waiting for 66 cycles
> which I think won't matter as the data-sheet specifies minimum change column
> set-up time.

Ok, let's forget this aspect for now. It just feels weird for someone
who is working on ARM platforms to see code that is making assumptions
on the parent clk rate.
We usually have most clocks exposed through the clk framework, and
query the rate of the clock driving the IP before calculating
sub-timings. 

> 
> >> In
> >> particular, I don't think we should be sending a real RNDOUT command at the
> >> device here -- it breaks the model of self-contained operations.  The read
> >> command is over and the chipselect has been dropped (do all ONFI chips support
> >> "CE don't care"?).  If the new offset (plus size to be read) fits within the
> >> page buffer, we could just adjust ifc_nand_ctrl->index to the desired
> >> location.  
> >
> > It should be supported (releasing the CE after the PARAM command and
> > then sending a RNDOUT), but who knows what NAND vendors decide to
> > implement.
> >  
> Initially I though of just adjusting the index as we don't do any read operation
> again and already we read the required bytes.
> So, what should be the final take on this ? Should I just update the index and
> avoid sending the change column command at all? Or should I keep the existing
> implementation ?

Not sure what you mean. Do you mean updating the index of your internal
buffer?

> >>
> >> OTOH, if we need to read parameter data beyond the size of the page buffer,
> >> then we'd need to send another read command (possibly from read_buf)... or we
> >> can do the sane thing and introduce an interface function to read a certain
> >> number of parameter bytes from a certain offset.  
> >
> > No please, do not add any complex logic in read_buf() (like resending a
> > NAND cmd from there), it will just make the whole thing worst.
> >  
> >> Which we should do anyway if
> >> we want to move in the direction of a better interface with less cmdfunc
> >> abuse.  
> >
> > Not really. Ideally, ->read_buf() should just launch the I/O transfer
> > (same for ->write_buf()), nothing more. It shouldn't have to test which
> > command was sent before and adapt its behavior.
> > Now, I know that some controllers are not able to dissociate the
> > CMD/ADDR cycles from the DATA transfers, which is why a new interface
> > is needed.
> >  
> >>  
> >> >  
> >> > > +         ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc-  
> >> > > >ifc_nand.ncfgr);  
> >> > Why is it done in ->cmdfunc()? I mean, this timing parameter should be
> >> > set for all operations, and applied as soon as ->select_chip() is
> >> > called.  
> >>
> >> select_chip() is a no-op[1].  
> >
> > Maybe it is in your implementation, but this is where you should have
> > all the CHIP specific settings (switching from one NAND chip to another
> > requires adapting the timing, setting a new CS line, ...).
> > The what the sunxi_nand driver is doing if you want an example where
> > the driver is tweaking the timing config at chip selection time.
> >  
> >> IFC normally handles the delays internally when
> >> running a command sequence.  
> >
> > Same for a lot of controllers out there, but that doesn't prevent you
> > from configuring the timings (on the controller side) when a chip is
> > selected.
> >
> > Note that ->select_chip() is not implying that you assert the CE line,
> > it's here to inform your controller driver that the core wants to
> > interact with a different NAND chip. What's done in there is completely
> > controller dependent.
> >  
> >>
> >> -Scott
> >>
> >> [1]  In any case, this is the time between RNDOUT and reading the data, not
> >> the time between asserting the chipselect and issuing a command -- and I don't
> >> see other drivers doing a delay in select_chip().  
> >
> > It's not about applying the delay in ->select_chip() it's about
> > configuring the chip specific timings on the controller side. I guess
> > the first byte of the ncfgr register is always encoding the tCCS
> > timing, not something different depending on the command that is sent.
> > If that's the case, then you don't need to reconfigure the register
> > each time you send the RNDOUT command, only once, when you activate a
> > chip.
> >
> > But again, I don't know this controller, so I might be wrong.  
> 
> NCFGR may be used in future to wait for custom number of clock cycles, so I
> don't think it's a good idea to to configure tCCS time into it only
> once during init.

I'm just curious, could you detail what are the fields in NCFGR?
If this is a generic field saying "wait X cycle after the last CMD
cycles has been sent before doing I/Os", then yes, putting tCCS in
there is wrong.
But it's usually not how timings are configured. Most of the time it's
a subtle combination of tNFCX = max(tX, tY, tZ). And this configuration
can be done at chip selection time, and not every time you send a
command.

Anyway, I can't say much without knowing what this field describe.

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Scott Wood]

On Wed, 2016-09-07 at 18:15 +0200, Boris Brezillon wrote:
> On Wed, 7 Sep 2016 09:50:50 -0500
> Ronak Desai <ronak.desai@rockwellcollins.com> wrote:
> 
> > IFC input clock is divide by 2 platform clock which is fixed in T, P
> > series
> > and Layerscape series of processors where IFC is used, maximum
> > platform clock is 600 MHz so IFC input clock becomes 300 MHz maximum.
> > To avoid floating operation, I chose next possible frequency of
> > 333.33 MHz which gives me period of 3 ns. There was no easy way to find
> > the input IFC clock without knowing platform clock which requires RCW
> > values.
> > So, to avoid that I selected this route where we will end up waiting few
> > more
> > cycles then required but it does not harm. For example, my input IFC clock
> > is
> > 266.66 Mhz and my NAND part tells me to wait minimum 200 ns so I have to
> > wait for  54 cycles while with this calculation I will be waiting for 66
> > cycles
> > which I think won't matter as the data-sheet specifies minimum change
> > column
> > set-up time.
> Ok, let's forget this aspect for now. It just feels weird for someone
> who is working on ARM platforms to see code that is making assumptions
> on the parent clk rate.
> We usually have most clocks exposed through the clk framework, and
> query the rate of the clock driving the IP before calculating
> sub-timings. 

I don't like seeing these assumptions hardcoded either, and we do have a clock
driver that can supply this information.  We just need to add an appropriate
clocks property to the IFC node (though the driver should keep working at
least as well as it currently does on existing device trees).

Prabhakar, do you know of a FIR operation other than NWAIT that would be
suitable here if we do go the route of sending a real RNDOUT command?

> > > > In
> > > > particular, I don't think we should be sending a real RNDOUT command
> > > > at the
> > > > device here -- it breaks the model of self-contained operations.  The
> > > > read
> > > > command is over and the chipselect has been dropped (do all ONFI chips
> > > > support
> > > > "CE don't care"?).  If the new offset (plus size to be read) fits
> > > > within the
> > > > page buffer, we could just adjust ifc_nand_ctrl->index to the desired
> > > > location.  
> > > It should be supported (releasing the CE after the PARAM command and
> > > then sending a RNDOUT), but who knows what NAND vendors decide to
> > > implement.
> > >  
> > Initially I though of just adjusting the index as we don't do any read
> > operation
> > again and already we read the required bytes.
> > So, what should be the final take on this ? Should I just update the index
> > and
> > avoid sending the change column command at all? Or should I keep the
> > existing
> > implementation ?
> Not sure what you mean. Do you mean updating the index of your internal
> buffer?

Yes, the offset within the SRAM buffer that the next read_byte()/read_buf()
will access.

-Scott

RE: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Prabhakar Kushwaha]

Hi Scott,

Sorry for very late reply.

> -----Original Message-----
> From: Scott Wood [mailto:oss@buserror.net]
> Sent: Thursday, September 08, 2016 5:01 AM
> To: Boris Brezillon <boris.brezillon@free-electrons.com>; Ronak Desai
> <ronak.desai@rockwellcollins.com>
> Cc: Matt Weber <matthew.weber@rockwellcollins.com>; Prabhakar Kushwaha
> <prabhakar.kushwaha@nxp.com>; linux-mtd@lists.infradead.org; Ezequiel
> Garcia <ezequiel@vanguardiasur.com.ar>
> Subject: Re: [PATCH] fsl_ifc_nand: Added random output enable cmd
> 
> On Wed, 2016-09-07 at 18:15 +0200, Boris Brezillon wrote:
> > On Wed, 7 Sep 2016 09:50:50 -0500
> > Ronak Desai <ronak.desai@rockwellcollins.com> wrote:
> >
> > > IFC input clock is divide by 2 platform clock which is fixed in T, P
> > > series
> > > and Layerscape series of processors where IFC is used, maximum
> > > platform clock is 600 MHz so IFC input clock becomes 300 MHz maximum.
> > > To avoid floating operation, I chose next possible frequency of
> > > 333.33 MHz which gives me period of 3 ns. There was no easy way to find
> > > the input IFC clock without knowing platform clock which requires RCW
> > > values.
> > > So, to avoid that I selected this route where we will end up waiting few
> > > more
> > > cycles then required but it does not harm. For example, my input IFC clock
> > > is
> > > 266.66 Mhz and my NAND part tells me to wait minimum 200 ns so I have to
> > > wait for  54 cycles while with this calculation I will be waiting for 66
> > > cycles
> > > which I think won't matter as the data-sheet specifies minimum change
> > > column
> > > set-up time.
> > Ok, let's forget this aspect for now. It just feels weird for someone
> > who is working on ARM platforms to see code that is making assumptions
> > on the parent clk rate.
> > We usually have most clocks exposed through the clk framework, and
> > query the rate of the clock driving the IP before calculating
> > sub-timings.
> 
> I don't like seeing these assumptions hardcoded either, and we do have a clock
> driver that can supply this information.  We just need to add an appropriate
> clocks property to the IFC node (though the driver should keep working at
> least as well as it currently does on existing device trees).
> 
> Prabhakar, do you know of a FIR operation other than NWAIT that would be
> suitable here if we do go the route of sending a real RNDOUT command?

There is no FIR operation other than NWAIT which can be used. 
Same has been confirmed from design team. 

--prabhakar

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Scott Wood]

On Wed, 2016-09-07 at 09:22 +0200, Boris Brezillon wrote:
> Hi Scott,
> 
> On Wed, 07 Sep 2016 01:03:53 -0500
> Scott Wood <oss@buserror.net> wrote:
> 
> > 
> > On Tue, 2016-09-06 at 21:50 +0200, Boris Brezillon wrote:
> > > 
> > > On Tue,  6 Sep 2016 14:13:17 -0500
> > > Matt Weber <matthew.weber@rockwellcollins.com> wrote:
> > >   
> > > > 
> > > > 
> > > > This patch adds random output enable command support in IFC nand
> > > > controller driver. This command implements change read
> > > > column (05h-E0h).
> > > > 
> > > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> > > > Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
> > > > ---
> > > >  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
> > > >  1 file changed, 27 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c
> > > > b/drivers/mtd/nand/fsl_ifc_nand.c
> > > > index 4e9e5fd..230919f 100644
> > > > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > > > @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info
> > > > *mtd,
> > > > unsigned int command,
> > > >  
> > > >  		/*
> > > >  		 * although currently it's 8 bytes for READID, we
> > > > always
> > > > read
> > > > -		 * the maximum 256 bytes(for PARAM)
> > > > +		 * the maximum 8192 bytes(for PARAM) supported by IFC
> > > > controller
> > > > +		 * as extended page may be available for some NAND
> > > > devices.
> > > >  		 */
> > > > -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> > > > -		ifc_nand_ctrl->read_bytes = 256;
> > > > +		ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole
> > > > page */
> > > > +		ifc_nand_ctrl->read_bytes = 8192; /* Maximum
> > > > supported
> > > > page by IFC */  
> > Are you sure that reading 8192 bytes will work if the configured page size
> > is
> > smaller?
> I'm pretty sure it will, but you will either get garbage or have the
> same content duplicated in your buffer.

Or the controller might ignore the upper bits in the byte count, or exhibit
other undefined behavior.  The manual doesn't explicitly address it but
telling the controller to load data into a region of the buffer that has no
SRAM backing it up seems like a bad thing.  Even if it seems to "work fine"
when tested. :-P

Plus, if we properly limit this based on the buffer configuration, we'd get an
error message if we try to read beyond the end of the buffer, rather than
silently getting garbage.  Another option is to temporarily configure the
buffer for 8K pages when reading param data.

> > > And this exactly why letting drivers implement their own ->cmdfunc()
> > > method is a bad idea.  
> > > ->cmdfunc() does not provide enough information to guess how much data  
> > > should be read or written. Actually it's not supposed to be used this
> > > way, but drivers usually abuse it.  
> > It would be even uglier if we used the standard nand_command_lp() and had
> > to
> > abuse cmd_ctrl() instead. :-P
> Well, I can't say, I haven't read the whole driver. But I've been said
> so many times that ->cmd_ctrl() was not a good solution and in the end
> it appeared to be untrue (you don't necessarily have to issue the CMD
> and ADDR cycles right away, you can cache them and send the whole
> sequence once the command parameter is CMD_NONE).
> 
> Anyway, the problem is not really whether ->cmd_ctrl() should be used
> instead of ->cmdfunc() here. The thing is, you're doing the I/Os in a
> place where you don't know how many bytes should be retrieved.

Right.  Normally there's a reasonable upper bound (read the whole page, and
don't allow subpage writes) but it's not obvious with param data.

> > The current NAND driver interface is too low level for controllers that
> > expose
> > higher level interfaces.
> That's for sure.
> 
> > 
> > If we can't/shouldn't replace cmdfunc() then we need
> > to replace the things that call cmdfunc().  Yes, we probably should have
> > pushed for a better interface back then rather than just "making it
> > work"...
> No, let's not add more random hacks.

I'm not suggesting "random hacks".  I'm suggesting a higher level interface
based on complete NAND operations.

>  I was just complaining because I
> see more and more drivers implementing their own ->cmdfunc() and adding
> this kind of hacks.
> Most of them can implement ->cmd_ctrl() and rely on the default
> nand_command_lp(), but I agree that some controllers do not fit in this
> model.
> 
> For these ones, I was planning to provide a ->send_op(chip, nand_op)
> method, where nand_op would contain both the CMD, ADDR cycles and
> the data transfer.

I was thinking more along the lines of specific operations (similar to
read_page but without the preceding cmdfunc)...  A generic "send_op" might
work, though I'm curious about the details of how nand_op gets encoded, and am
worried that it might still result in NAND drivers interpreting specific
commands rather than passing things through in a generic way (and then things
can break if common code sends something new).

> > > Can't you know the clk rate at runtime instead of basing your
> > > calculation on the hypothetic clk rate?  
> > If we really need to know the clock rate, we could add a clocks property
> > to
> > the IFC node.  But we haven't needed to deal with clocking anywhere else
> > in
> > this driver, so I'm not too happy about introducing it for this.
> Will the base clk always be running at the same rate on all designs? To
> me, it sounds a bit risky to do this assumption, but maybe I'm wrong.

There are timing registers that the bootloader is supposed to set
appropriately.

> > In
> > particular, I don't think we should be sending a real RNDOUT command at
> > the
> > device here -- it breaks the model of self-contained operations.  The read
> > command is over and the chipselect has been dropped (do all ONFI chips
> > support
> > "CE don't care"?).  If the new offset (plus size to be read) fits within
> > the
> > page buffer, we could just adjust ifc_nand_ctrl->index to the desired
> > location.
> It should be supported (releasing the CE after the PARAM command and
> then sending a RNDOUT), but who knows what NAND vendors decide to
> implement.
> 
> > 
> > 
> > OTOH, if we need to read parameter data beyond the size of the page
> > buffer,
> > then we'd need to send another read command (possibly from read_buf)... or
> > we
> > can do the sane thing and introduce an interface function to read a
> > certain
> > number of parameter bytes from a certain offset.
> No please, do not add any complex logic in read_buf() (like resending a
> NAND cmd from there), it will just make the whole thing worst.

I agree.  I was pointing out where the road we're currently on leads, to
demonstrate why we need to do something different.

> > 
> > Which we should do anyway if
> > we want to move in the direction of a better interface with less cmdfunc
> > abuse.
> Not really. Ideally, ->read_buf() should just launch the I/O transfer
> (same for ->write_buf()), nothing more. It shouldn't have to test which
> command was sent before and adapt its behavior.

"Which we should do anyway" refers to "or we can do the sane thing and
introduce an interface function...", not to complicating read_buf.

> Now, I know that some controllers are not able to dissociate the
> CMD/ADDR cycles from the DATA transfers, which is why a new interface
> is needed.

In theory we could separate them but not without dropping CE.  Some NAND data
sheets I have indicate that support for that is an optional feature, and we do
have some older NAND chips being used by this driver.  In any case we'd be
fighting against the way the controller was intended to be used, and we'd be
adding more CPU overhead to wake up after the command has been issued but
before the transfer has begun.

> > > > +		ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc-  
> > > > > 
> > > > > ifc_nand.ncfgr);  
> > > Why is it done in ->cmdfunc()? I mean, this timing parameter should be
> > > set for all operations, and applied as soon as ->select_chip() is
> > > called.  
> > select_chip() is a no-op[1].
> Maybe it is in your implementation, but this is where you should have
> all the CHIP specific settings (switching from one NAND chip to another
> requires adapting the timing, setting a new CS line, ...).
> The what the sunxi_nand driver is doing if you want an example where
> the driver is tweaking the timing config at chip selection time.
>
> > IFC normally handles the delays internally when
> > running a command sequence.
> Same for a lot of controllers out there, but that doesn't prevent you
> from configuring the timings (on the controller side) when a chip is
> selected.
> 
> Note that ->select_chip() is not implying that you assert the CE line,
> it's here to inform your controller driver that the core wants to
> interact with a different NAND chip. What's done in there is completely
> controller dependent.
[snip]
> It's not about applying the delay in ->select_chip() it's about
> configuring the chip specific timings on the controller side. I guess
> the first byte of the ncfgr register is always encoding the tCCS
> timing, not something different depending on the command that is sent.
> If that's the case, then you don't need to reconfigure the register
> each time you send the RNDOUT command, only once, when you activate a
> chip.
> 
> But again, I don't know this controller, so I might be wrong.

This controller has separate timing registers for each chipselect.  Each
chipselect maps to a struct mtd_info.  We don't support multiple chips per
mtd_info.

NCFGR[NUM_WAIT] is an exception to this -- it's an arbitrary delay that can be
requested via the FIR NWAIT opcode (this patch is the the first usage of NWAIT
in the driver).  It doesn't have any meaning outside of a particular FIR
sequence that uses NWAIT.

I suppose we could use select_chip() to write the NAND_CSEL register instead
of doing it in fsl_ifc_run_command()...  This driver was patterned after the
eLBC driver, where writing the register that sets the chipselect also triggers
the operation to start.

-Scott

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Boris Brezillon]

On Wed, 07 Sep 2016 18:18:59 -0500
Scott Wood <oss@buserror.net> wrote:

> On Wed, 2016-09-07 at 09:22 +0200, Boris Brezillon wrote:
> > Hi Scott,
> > 
> > On Wed, 07 Sep 2016 01:03:53 -0500
> > Scott Wood <oss@buserror.net> wrote:
> >   
> > > 
> > > On Tue, 2016-09-06 at 21:50 +0200, Boris Brezillon wrote:  
> > > > 
> > > > On Tue,  6 Sep 2016 14:13:17 -0500
> > > > Matt Weber <matthew.weber@rockwellcollins.com> wrote:
> > > >     
> > > > > 
> > > > > 
> > > > > This patch adds random output enable command support in IFC nand
> > > > > controller driver. This command implements change read
> > > > > column (05h-E0h).
> > > > > 
> > > > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> > > > > Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
> > > > > ---
> > > > >  drivers/mtd/nand/fsl_ifc_nand.c | 30 +++++++++++++++++++++++++++---
> > > > >  1 file changed, 27 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c
> > > > > b/drivers/mtd/nand/fsl_ifc_nand.c
> > > > > index 4e9e5fd..230919f 100644
> > > > > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > > > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > > > > @@ -387,10 +387,11 @@ static void fsl_ifc_cmdfunc(struct mtd_info
> > > > > *mtd,
> > > > > unsigned int command,
> > > > >  
> > > > >  		/*
> > > > >  		 * although currently it's 8 bytes for READID, we
> > > > > always
> > > > > read
> > > > > -		 * the maximum 256 bytes(for PARAM)
> > > > > +		 * the maximum 8192 bytes(for PARAM) supported by IFC
> > > > > controller
> > > > > +		 * as extended page may be available for some NAND
> > > > > devices.
> > > > >  		 */
> > > > > -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> > > > > -		ifc_nand_ctrl->read_bytes = 256;
> > > > > +		ifc_out32(0, &ifc->ifc_nand.nand_fbcr); /* Read whole
> > > > > page */
> > > > > +		ifc_nand_ctrl->read_bytes = 8192; /* Maximum
> > > > > supported
> > > > > page by IFC */    
> > > Are you sure that reading 8192 bytes will work if the configured page size
> > > is
> > > smaller?  
> > I'm pretty sure it will, but you will either get garbage or have the
> > same content duplicated in your buffer.  
> 
> Or the controller might ignore the upper bits in the byte count, or exhibit
> other undefined behavior.  The manual doesn't explicitly address it but
> telling the controller to load data into a region of the buffer that has no
> SRAM backing it up seems like a bad thing.  Even if it seems to "work fine"
> when tested. :-P

I think there was a misunderstanding here. I was talking about the NAND
chip behavior, not the NAND controller.

> 
> Plus, if we properly limit this based on the buffer configuration, we'd get an
> error message if we try to read beyond the end of the buffer, rather than
> silently getting garbage.  Another option is to temporarily configure the
> buffer for 8K pages when reading param data.
> 
> > > > And this exactly why letting drivers implement their own ->cmdfunc()
> > > > method is a bad idea.    
> > > > ->cmdfunc() does not provide enough information to guess how much data    
> > > > should be read or written. Actually it's not supposed to be used this
> > > > way, but drivers usually abuse it.    
> > > It would be even uglier if we used the standard nand_command_lp() and had
> > > to
> > > abuse cmd_ctrl() instead. :-P  
> > Well, I can't say, I haven't read the whole driver. But I've been said
> > so many times that ->cmd_ctrl() was not a good solution and in the end
> > it appeared to be untrue (you don't necessarily have to issue the CMD
> > and ADDR cycles right away, you can cache them and send the whole
> > sequence once the command parameter is CMD_NONE).
> > 
> > Anyway, the problem is not really whether ->cmd_ctrl() should be used
> > instead of ->cmdfunc() here. The thing is, you're doing the I/Os in a
> > place where you don't know how many bytes should be retrieved.  
> 
> Right.  Normally there's a reasonable upper bound (read the whole page, and
> don't allow subpage writes) but it's not obvious with param data.

And PARAM is not this only problem. I'm working with MLC NANDs which
implement private commands where you are asked to read/write a random
amount of bytes. If you read or write more, it won't necessarily work.

> 
> > > The current NAND driver interface is too low level for controllers that
> > > expose
> > > higher level interfaces.  
> > That's for sure.
> >   
> > > 
> > > If we can't/shouldn't replace cmdfunc() then we need
> > > to replace the things that call cmdfunc().  Yes, we probably should have
> > > pushed for a better interface back then rather than just "making it
> > > work"...  
> > No, let's not add more random hacks.  
> 
> I'm not suggesting "random hacks".  I'm suggesting a higher level interface
> based on complete NAND operations.

Okay, then we are on the same page.

> 
> >  I was just complaining because I
> > see more and more drivers implementing their own ->cmdfunc() and adding
> > this kind of hacks.
> > Most of them can implement ->cmd_ctrl() and rely on the default
> > nand_command_lp(), but I agree that some controllers do not fit in this
> > model.
> > 
> > For these ones, I was planning to provide a ->send_op(chip, nand_op)
> > method, where nand_op would contain both the CMD, ADDR cycles and
> > the data transfer.  
> 
> I was thinking more along the lines of specific operations (similar to
> read_page but without the preceding cmdfunc)...

I'm curious. What did you have in mind? Adding a new ->do_xxx() each
time a NAND needs a new CMD/ADDR/DATA sequence? So we'll start with
->read_param_page(), which is rather generic. But what about all those
private commands that are not generic at all. I don't think it's
reasonable to ask the NAND controller to implement all these methods.

> A generic "send_op" might
> work, though I'm curious about the details of how nand_op gets encoded, and am
> worried that it might still result in NAND drivers interpreting specific
> commands rather than passing things through in a generic way (and then things
> can break if common code sends something new).

If drivers end up doing that, this means we failed providing an
interface that is suitable for everyone.
But, from what I've seen in other drivers, it's usually just about
setting the first and second cmd cyles, specifying the address cycles
(and the number of cycles) and then the amount of data to transfer +
the direction.

There are some controllers that only understand high level commands,
and for these ones we are just screwed (the conversion from raw
commands to high-level ones is inevitable). But most controllers
support both modes, and for management operations like retrieving the
param page or setting onfi params, we don't really care about
optimizations that could be done in the controller logic when using the
high-level commands.

Where we really care is in the read_page/write_page path, and this path
already provides dedicated callbacks. This only thing that is missing
is a way to tell the core that ->cmdfunc() should not be called.
Actually, I proposed this solution to Marc (adding a flagging to
chip->options to tell the core not to send the READ_START commands), and
it seems to work.

Now, for the 'administration' operations, where we do not care about
performances, I keep thinking that implementing ->cmd_ctrl() and relying
on the default nand_command{,_lp}() implementations (or, if we introduce
this ->send_op() method, directly relying on ->send_op()) is saner.

> 
> > > > Can't you know the clk rate at runtime instead of basing your
> > > > calculation on the hypothetic clk rate?    
> > > If we really need to know the clock rate, we could add a clocks property
> > > to
> > > the IFC node.  But we haven't needed to deal with clocking anywhere else
> > > in
> > > this driver, so I'm not too happy about introducing it for this.  
> > Will the base clk always be running at the same rate on all designs? To
> > me, it sounds a bit risky to do this assumption, but maybe I'm wrong.  
> 
> There are timing registers that the bootloader is supposed to set
> appropriately.

Okay. So you don't want to adapt these timings dynamically. I
understand.

> 
> > > In
> > > particular, I don't think we should be sending a real RNDOUT command at
> > > the
> > > device here -- it breaks the model of self-contained operations.  The read
> > > command is over and the chipselect has been dropped (do all ONFI chips
> > > support
> > > "CE don't care"?).  If the new offset (plus size to be read) fits within
> > > the
> > > page buffer, we could just adjust ifc_nand_ctrl->index to the desired
> > > location.  
> > It should be supported (releasing the CE after the PARAM command and
> > then sending a RNDOUT), but who knows what NAND vendors decide to
> > implement.
> >   
> > > 
> > > 
> > > OTOH, if we need to read parameter data beyond the size of the page
> > > buffer,
> > > then we'd need to send another read command (possibly from read_buf)... or
> > > we
> > > can do the sane thing and introduce an interface function to read a
> > > certain
> > > number of parameter bytes from a certain offset.  
> > No please, do not add any complex logic in read_buf() (like resending a
> > NAND cmd from there), it will just make the whole thing worst.  
> 
> I agree.  I was pointing out where the road we're currently on leads, to
> demonstrate why we need to do something different.
> 
> > > 
> > > Which we should do anyway if
> > > we want to move in the direction of a better interface with less cmdfunc
> > > abuse.  
> > Not really. Ideally, ->read_buf() should just launch the I/O transfer
> > (same for ->write_buf()), nothing more. It shouldn't have to test which
> > command was sent before and adapt its behavior.  
> 
> "Which we should do anyway" refers to "or we can do the sane thing and
> introduce an interface function...", not to complicating read_buf.
> 
> > Now, I know that some controllers are not able to dissociate the
> > CMD/ADDR cycles from the DATA transfers, which is why a new interface
> > is needed.  
> 
> In theory we could separate them but not without dropping CE.  Some NAND data
> sheets I have indicate that support for that is an optional feature, and we do
> have some older NAND chips being used by this driver.  In any case we'd be
> fighting against the way the controller was intended to be used, and we'd be
> adding more CPU overhead to wake up after the command has been issued but
> before the transfer has begun.

Well, as I said above, it's not about getting best perfs for this kind
of operations. It's only happening once at init time. As I said, the
whole NAND framework has become unmaintainable for this very reason.
Say one solder a new NAND on a new board. It's not guaranteed to work,
because the controller might not support some of important functions in
its ->cmdfunc() implementation.
And each time we want to add a new NAND operation, we have to go over
all drivers and check if it's supported. That's really a pain.

I'd prefer to have all drivers implement a generic method to send any
kind of CMD/ADDR/DATA sequence, and then have a few methods for
operations where perfs really matters (read/write_page()).

> 
> > > > > +		ifc_out32((0xFF & (le16_to_cpu(Tccs)/3)), &ifc-    
> > > > > > 
> > > > > > ifc_nand.ncfgr);    
> > > > Why is it done in ->cmdfunc()? I mean, this timing parameter should be
> > > > set for all operations, and applied as soon as ->select_chip() is
> > > > called.    
> > > select_chip() is a no-op[1].  
> > Maybe it is in your implementation, but this is where you should have
> > all the CHIP specific settings (switching from one NAND chip to another
> > requires adapting the timing, setting a new CS line, ...).
> > The what the sunxi_nand driver is doing if you want an example where
> > the driver is tweaking the timing config at chip selection time.
> >  
> > > IFC normally handles the delays internally when
> > > running a command sequence.  
> > Same for a lot of controllers out there, but that doesn't prevent you
> > from configuring the timings (on the controller side) when a chip is
> > selected.
> > 
> > Note that ->select_chip() is not implying that you assert the CE line,
> > it's here to inform your controller driver that the core wants to
> > interact with a different NAND chip. What's done in there is completely
> > controller dependent.  
> [snip]
> > It's not about applying the delay in ->select_chip() it's about
> > configuring the chip specific timings on the controller side. I guess
> > the first byte of the ncfgr register is always encoding the tCCS
> > timing, not something different depending on the command that is sent.
> > If that's the case, then you don't need to reconfigure the register
> > each time you send the RNDOUT command, only once, when you activate a
> > chip.
> > 
> > But again, I don't know this controller, so I might be wrong.  
> 
> This controller has separate timing registers for each chipselect.  Each
> chipselect maps to a struct mtd_info.  We don't support multiple chips per
> mtd_info.
> 
> NCFGR[NUM_WAIT] is an exception to this -- it's an arbitrary delay that can be
> requested via the FIR NWAIT opcode (this patch is the the first usage of NWAIT
> in the driver).  It doesn't have any meaning outside of a particular FIR
> sequence that uses NWAIT.

Okay, that answers my question. Thanks.

> 
> I suppose we could use select_chip() to write the NAND_CSEL register instead
> of doing it in fsl_ifc_run_command()...  This driver was patterned after the
> eLBC driver, where writing the register that sets the chipselect also triggers
> the operation to start.

That's fine. Just keep the existing logic for now.

Regards,

Boris

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Scott Wood]

On Thu, 2016-09-08 at 07:57 +0200, Boris Brezillon wrote:
> On Wed, 07 Sep 2016 18:18:59 -0500
> Scott Wood <oss@buserror.net> wrote:
> 
> > 
> > On Wed, 2016-09-07 at 09:22 +0200, Boris Brezillon wrote:
> > > 
> > > 
> > >  I was just complaining because I
> > > see more and more drivers implementing their own ->cmdfunc() and adding
> > > this kind of hacks.
> > > Most of them can implement ->cmd_ctrl() and rely on the default
> > > nand_command_lp(), but I agree that some controllers do not fit in this
> > > model.
> > > 
> > > For these ones, I was planning to provide a ->send_op(chip, nand_op)
> > > method, where nand_op would contain both the CMD, ADDR cycles and
> > > the data transfer.  
> > I was thinking more along the lines of specific operations (similar to
> > read_page but without the preceding cmdfunc)...
> I'm curious. What did you have in mind? Adding a new ->do_xxx() each
> time a NAND needs a new CMD/ADDR/DATA sequence? So we'll start with
> ->read_param_page(), which is rather generic. But what about all those
> private commands that are not generic at all. I don't think it's
> reasonable to ask the NAND controller to implement all these methods.

That's basically what I had in mind, and yes, it is a problem if we need to
support private commands.

> > A generic "send_op" might
> > work, though I'm curious about the details of how nand_op gets encoded,
> > and am
> > worried that it might still result in NAND drivers interpreting specific
> > commands rather than passing things through in a generic way (and then
> > things
> > can break if common code sends something new).
> If drivers end up doing that, this means we failed providing an
> interface that is suitable for everyone.
> But, from what I've seen in other drivers, it's usually just about
> setting the first and second cmd cyles, specifying the address cycles
> (and the number of cycles) and then the amount of data to transfer +
> the direction.

Timing is one area that could be problematic.  This patch is an example of a
situation where different timing was used for a particular command.  It seems
that RNDOUT doesn't use the R/B pin -- how would a generic send_op
implementation know whether it can use R/B to determine when to start the I/O
transfer?

> There are some controllers that only understand high level commands,
> and for these ones we are just screwed (the conversion from raw
> commands to high-level ones is inevitable).

A mixture of the approaches could help here.  Have replaceable do_xxx() that
cover the basic operations (with the default implementations calling send_op) 
and if a driver can't support send_op (or the current API) then private
commands and new features just won't work with that controller (but it would
be more obvious what does/doesn't work than sending an unrecognized command to
cmdfunc).

> > > 
> > > Now, I know that some controllers are not able to dissociate the
> > > CMD/ADDR cycles from the DATA transfers, which is why a new interface
> > > is needed.  
> > In theory we could separate them but not without dropping CE.  Some NAND
> > data
> > sheets I have indicate that support for that is an optional feature, and
> > we do
> > have some older NAND chips being used by this driver.  In any case we'd be
> > fighting against the way the controller was intended to be used, and we'd
> > be
> > adding more CPU overhead to wake up after the command has been issued but
> > before the transfer has begun.
> Well, as I said above, it's not about getting best perfs for this kind
> of operations. It's only happening once at init time.

Right, I was thinking of the impact on other operations if we rework the
driver to separate commands from I/O.  Removing the cmdfunc call from page
reads would help make that a special case without complicating the general
flow.

>  As I said, the
> whole NAND framework has become unmaintainable for this very reason.
> Say one solder a new NAND on a new board. It's not guaranteed to work,
> because the controller might not support some of important functions in
> its ->cmdfunc() implementation.
> And each time we want to add a new NAND operation, we have to go over
> all drivers and check if it's supported. That's really a pain.
> 
> I'd prefer to have all drivers implement a generic method to send any
> kind of CMD/ADDR/DATA sequence, and then have a few methods for
> operations where perfs really matters (read/write_page()).

We could avoid special cases in read_buf() by having read_page() not call it,
but the command sending is (currently) separate and thus would still need a
special case to delay read commands until read_page().

A bigger problem with separating the command from the I/O is the R/B pin.  If
the NAND chip asserts R/B when another (non-NAND) device's chipselect is
asserted, then it can corrupt that chip's activity.  We'd be undoing the fix
in commit 476459a6cf46d20e ("mtd: eLBC NAND: use recommended command
sequences").

IFC seems to be better about how it shares pins, but it's not clear that it's
completely OK in all configurations.

-Scott

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Boris Brezillon]

On Thu, 08 Sep 2016 22:00:10 -0500
Scott Wood <oss@buserror.net> wrote:

> On Thu, 2016-09-08 at 07:57 +0200, Boris Brezillon wrote:
> > On Wed, 07 Sep 2016 18:18:59 -0500
> > Scott Wood <oss@buserror.net> wrote:
> >   
> > > 
> > > On Wed, 2016-09-07 at 09:22 +0200, Boris Brezillon wrote:  
> > > > 
> > > > 
> > > >  I was just complaining because I
> > > > see more and more drivers implementing their own ->cmdfunc() and adding
> > > > this kind of hacks.
> > > > Most of them can implement ->cmd_ctrl() and rely on the default
> > > > nand_command_lp(), but I agree that some controllers do not fit in this
> > > > model.
> > > > 
> > > > For these ones, I was planning to provide a ->send_op(chip, nand_op)
> > > > method, where nand_op would contain both the CMD, ADDR cycles and
> > > > the data transfer.    
> > > I was thinking more along the lines of specific operations (similar to
> > > read_page but without the preceding cmdfunc)...  
> > I'm curious. What did you have in mind? Adding a new ->do_xxx() each
> > time a NAND needs a new CMD/ADDR/DATA sequence? So we'll start with  
> > ->read_param_page(), which is rather generic. But what about all those  
> > private commands that are not generic at all. I don't think it's
> > reasonable to ask the NAND controller to implement all these methods.  
> 
> That's basically what I had in mind, and yes, it is a problem if we need to
> support private commands.

Hm, then I'd really like to avoid that.

> 
> > > A generic "send_op" might
> > > work, though I'm curious about the details of how nand_op gets encoded,
> > > and am
> > > worried that it might still result in NAND drivers interpreting specific
> > > commands rather than passing things through in a generic way (and then
> > > things
> > > can break if common code sends something new).  
> > If drivers end up doing that, this means we failed providing an
> > interface that is suitable for everyone.
> > But, from what I've seen in other drivers, it's usually just about
> > setting the first and second cmd cyles, specifying the address cycles
> > (and the number of cycles) and then the amount of data to transfer +
> > the direction.  
> 
> Timing is one area that could be problematic.  This patch is an example of a
> situation where different timing was used for a particular command.  It seems
> that RNDOUT doesn't use the R/B pin -- how would a generic send_op
> implementation know whether it can use R/B to determine when to start the I/O
> transfer?

Well, the plan was to specify in the nand operation whether it should
wait for the chip to be ready or not.

Now, for the timings thing, I was assuming that all timings were
configured at chip selection time (or earlier, if you hardcode the
timings in your bootloader for example). But if that's needed, the core
can extract the timings information for us and pass them to the nand_op
structure.

> 
> > There are some controllers that only understand high level commands,
> > and for these ones we are just screwed (the conversion from raw
> > commands to high-level ones is inevitable).  
> 
> A mixture of the approaches could help here.  Have replaceable do_xxx() that
> cover the basic operations (with the default implementations calling send_op) 
> and if a driver can't support send_op (or the current API) then private
> commands and new features just won't work with that controller (but it would
> be more obvious what does/doesn't work than sending an unrecognized command to
> cmdfunc).

Well, I don't close the door to the ->do_xxx() methods, but I'd really
like consider this option as a last resort, so let's first see if we can
make all controllers fit in the ->send_op() model.

For the other problem you're mentionning (->cmdfunc() silently ignores
unsupported command), having new methods return -ENOTSUPP would
definitely help identify controller drivers that don't support random
command sequences.

> 
> > > > 
> > > > Now, I know that some controllers are not able to dissociate the
> > > > CMD/ADDR cycles from the DATA transfers, which is why a new interface
> > > > is needed.    
> > > In theory we could separate them but not without dropping CE.  Some NAND
> > > data
> > > sheets I have indicate that support for that is an optional feature, and
> > > we do
> > > have some older NAND chips being used by this driver.  In any case we'd be
> > > fighting against the way the controller was intended to be used, and we'd
> > > be
> > > adding more CPU overhead to wake up after the command has been issued but
> > > before the transfer has begun.  
> > Well, as I said above, it's not about getting best perfs for this kind
> > of operations. It's only happening once at init time.  
> 
> Right, I was thinking of the impact on other operations if we rework the
> driver to separate commands from I/O.  Removing the cmdfunc call from page
> reads would help make that a special case without complicating the general
> flow.

Yep, that's the plan. Actually, the plan is to explicitly ask the core
to not send the command when the controller already takes care of that.

> 
> >  As I said, the
> > whole NAND framework has become unmaintainable for this very reason.
> > Say one solder a new NAND on a new board. It's not guaranteed to work,
> > because the controller might not support some of important functions in
> > its ->cmdfunc() implementation.
> > And each time we want to add a new NAND operation, we have to go over
> > all drivers and check if it's supported. That's really a pain.
> > 
> > I'd prefer to have all drivers implement a generic method to send any
> > kind of CMD/ADDR/DATA sequence, and then have a few methods for
> > operations where perfs really matters (read/write_page()).  
> 
> We could avoid special cases in read_buf() by having read_page() not call it,
> but the command sending is (currently) separate and thus would still need a
> special case to delay read commands until read_page().

That's why I'm proposing to let ->read_page() issue the command by
itself.

> 
> A bigger problem with separating the command from the I/O is the R/B pin.  If
> the NAND chip asserts R/B when another (non-NAND) device's chipselect is
> asserted, then it can corrupt that chip's activity.  We'd be undoing the fix
> in commit 476459a6cf46d20e ("mtd: eLBC NAND: use recommended command
> sequences").

The ->send_op() method we're discussing would solve this problem, right?

> 
> IFC seems to be better about how it shares pins, but it's not clear that it's
> completely OK in all configurations.

This tends to confirm that we need to provide a solution to ask the
controller driver to send the whole operation, instead of doing it in 2
steps (the CMD+ADDR cycles and then the data).


Regards,

Boris

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Scott Wood]

On Fri, 2016-09-09 at 09:40 +0200, Boris Brezillon wrote:
> > > 


> On Thu, 08 Sep 2016 22:00:10 -0500
> Scott Wood <oss@buserror.net> wrote:
> 
> > 
> > On Thu, 2016-09-08 at 07:57 +0200, Boris Brezillon wrote:
> > > 
> > > On Wed, 07 Sep 2016 18:18:59 -0500
> > > Scott Wood <oss@buserror.net> wrote:
> > > 
> > > > 
> > > > A generic "send_op" might
> > > > work, though I'm curious about the details of how nand_op gets
> > > > encoded,
> > > > and am
> > > > worried that it might still result in NAND drivers interpreting
> > > > specific
> > > > commands rather than passing things through in a generic way (and then
> > > > things
> > > > can break if common code sends something new).  
> > > If drivers end up doing that, this means we failed providing an
> > > interface that is suitable for everyone.
> > > But, from what I've seen in other drivers, it's usually just about
> > > setting the first and second cmd cyles, specifying the address cycles
> > > (and the number of cycles) and then the amount of data to transfer +
> > > the direction.  
> > Timing is one area that could be problematic.  This patch is an example of
> > a
> > situation where different timing was used for a particular command.  It
> > seems
> > that RNDOUT doesn't use the R/B pin -- how would a generic send_op
> > implementation know whether it can use R/B to determine when to start the
> > I/O
> > transfer?
> Well, the plan was to specify in the nand operation whether it should
> wait for the chip to be ready or not.

OK.  Is there a more detailed description of send_op ready yet?

> > A bigger problem with separating the command from the I/O is the R/B pin.
> >  If
> > the NAND chip asserts R/B when another (non-NAND) device's chipselect is
> > asserted, then it can corrupt that chip's activity.  We'd be undoing the
> > fix
> > in commit 476459a6cf46d20e ("mtd: eLBC NAND: use recommended command
> > sequences").
> The ->send_op() method we're discussing would solve this problem, right?

Yes.

-Scott

Re: [PATCH] fsl_ifc_nand: Added random output enable cmd

[Boris Brezillon]

On Fri, 09 Sep 2016 13:30:41 -0500
Scott Wood <oss@buserror.net> wrote:

> On Fri, 2016-09-09 at 09:40 +0200, Boris Brezillon wrote:
> > > >   
> 
> 
> > On Thu, 08 Sep 2016 22:00:10 -0500
> > Scott Wood <oss@buserror.net> wrote:
> >   
> > > 
> > > On Thu, 2016-09-08 at 07:57 +0200, Boris Brezillon wrote:  
> > > > 
> > > > On Wed, 07 Sep 2016 18:18:59 -0500
> > > > Scott Wood <oss@buserror.net> wrote:
> > > >   
> > > > > 
> > > > > A generic "send_op" might
> > > > > work, though I'm curious about the details of how nand_op gets
> > > > > encoded,
> > > > > and am
> > > > > worried that it might still result in NAND drivers interpreting
> > > > > specific
> > > > > commands rather than passing things through in a generic way (and then
> > > > > things
> > > > > can break if common code sends something new).    
> > > > If drivers end up doing that, this means we failed providing an
> > > > interface that is suitable for everyone.
> > > > But, from what I've seen in other drivers, it's usually just about
> > > > setting the first and second cmd cyles, specifying the address cycles
> > > > (and the number of cycles) and then the amount of data to transfer +
> > > > the direction.    
> > > Timing is one area that could be problematic.  This patch is an example of
> > > a
> > > situation where different timing was used for a particular command.  It
> > > seems
> > > that RNDOUT doesn't use the R/B pin -- how would a generic send_op
> > > implementation know whether it can use R/B to determine when to start the
> > > I/O
> > > transfer?  
> > Well, the plan was to specify in the nand operation whether it should
> > wait for the chip to be ready or not.  
> 
> OK.  Is there a more detailed description of send_op ready yet?

I started something more than a year ago [1], but did not find the time
to finish the implementation. The plan was to rework the entire NAND
framework, not only the way we send NAND operations/commands, so this
branch contains much more than just the ->send_op() concept. But you
can see the nand_operation struct here [2].

Now, I realize that this attempt to rework the whole framework at once
was not doable, but I can probably provide a PoC introducing the
->send_op() method and nand_operation struct, and implement
->send_op() in the sunxi driver.

This being said, my main concern is the introduction of yet another way
to do things. The NAND framework is already quite complex, and if we
introduce this ->send_op() method, I'd like it to completely replace
the ->cmdfunc() instead of keeping both around until all
implementations decide to switch to the new approach.

Note that a default ->send_op() based around ->cmd_ctrl()
and ->{read/write}_buf() can easily be implemented.

[1]https://github.com/bbrezillon/linux-sunxi/tree/nand-core-rework-v2
[2]https://github.com/bbrezillon/linux-sunxi/blob/nand-core-rework-v2/include/linux/mtd/nand2.h#L41