diff mbox series

[RFC,25/27] mtd: nand: Add helpers to manage ECC engines and configurations

Message ID 20190221125806.28875-13-miquel.raynal@bootlin.com
State RFC
Headers show
Series None | expand

Commit Message

Miquel Raynal Feb. 21, 2019, 12:58 p.m. UTC
Add the logic in the NAND core to find the right ECC engine depending
on the NAND chip requirements and the user desires. Right now, the
choice may be made between (more will come):
* software Hamming
* software BCH
* on-die (SPI-NAND devices only)

Once the ECC engine has been found, the ECC engine must be
configured.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/core.c  | 107 +++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h |   4 ++
 2 files changed, 111 insertions(+)

Comments

Boris Brezillon Feb. 22, 2019, 2:44 p.m. UTC | #1
On Thu, 21 Feb 2019 13:58:04 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Add the logic in the NAND core to find the right ECC engine depending
> on the NAND chip requirements and the user desires. Right now, the
> choice may be made between (more will come):
> * software Hamming
> * software BCH
> * on-die (SPI-NAND devices only)
> 
> Once the ECC engine has been found, the ECC engine must be
> configured.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/core.c  | 107 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/nand.h |   4 ++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> index 872d46b5fc0f..9feb118c9f68 100644
> --- a/drivers/mtd/nand/core.c
> +++ b/drivers/mtd/nand/core.c
> @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len)
>  }
>  EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks);
>  
> +/**
> + * nanddev_find_ecc_engine() - Find a suitable ECC engine
> + * @nand: NAND device
> + */
> +static int nanddev_find_ecc_engine(struct nand_device *nand)

Can we pass the conf in argument instead of reading it from
nand->ecc.user_conf?

> +{
> +	bool is_spinand = mtd_type_is_spinand(&nand->mtd);

And here is the reason for the SPINAND type.

> +
> +	/* Read the user desires in terms of ECC engine/configuration */
> +	nand_ecc_read_user_conf(nand);
> +
> +	/* No ECC engine requestedn, let's return without error */
> +	if (nand->ecc.user_conf.mode == NAND_ECC_NONE)
> +		return 0;
> +
> +	/* Raw NAND default mode is hardware */
> +	if (!is_spinand && nand->ecc.user_conf.mode < 0)
> +		nand->ecc.user_conf.mode = NAND_ECC_HW;

We should let the raw NAND layer take this decision (actually, it's
even a raw NAND controller driver decision). Please complain if
user_conf.mode is invalid.
This way you won't need the SPINAND type you added in one of your
previous patch.

> +
> +	/* SPI-NAND default mode is on-die */
> +	if (is_spinand && nand->ecc.user_conf.mode < 0)
> +		nand->ecc.user_conf.mode = NAND_ECC_ON_DIE;
> +
> +	switch (nand->ecc.user_conf.mode) {
> +	case NAND_ECC_SOFT:
> +		nand->ecc.engine = nand_ecc_sw_get_engine(nand);
> +		break;
> +	case NAND_ECC_ON_DIE:
> +		if (is_spinand)
> +			nand->ecc.engine = spinand_ondie_ecc_get_engine();

So, maybe it's worth having the ondie ECC engine instance directly
embedded in nand_device instead of spinand, or maybe just a pointer, so
that you don't reserve extra space when the NAND device does not have
on-die ECC.

> +		else
> +			pr_err("On-die ECC engines for non SPI devices not supported yet\n");
> +		break;
> +	case NAND_ECC_HW:
> +		pr_err("Hardware ECC engines not supported yet\n");
> +		break;
> +	default:
> +		pr_err("Missing ECC engine property\n");
> +	}
> +
> +	if (!nand->ecc.engine)
> +		return  -EINVAL;
> +
> +	return 0;
> +}
Miquel Raynal Feb. 25, 2019, 4:01 p.m. UTC | #2
Hi Boris,

Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019
15:44:31 +0100:

> On Thu, 21 Feb 2019 13:58:04 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Add the logic in the NAND core to find the right ECC engine depending
> > on the NAND chip requirements and the user desires. Right now, the
> > choice may be made between (more will come):
> > * software Hamming
> > * software BCH
> > * on-die (SPI-NAND devices only)
> > 
> > Once the ECC engine has been found, the ECC engine must be
> > configured.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/core.c  | 107 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/nand.h |   4 ++
> >  2 files changed, 111 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> > index 872d46b5fc0f..9feb118c9f68 100644
> > --- a/drivers/mtd/nand/core.c
> > +++ b/drivers/mtd/nand/core.c
> > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len)
> >  }
> >  EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks);
> >  
> > +/**
> > + * nanddev_find_ecc_engine() - Find a suitable ECC engine
> > + * @nand: NAND device
> > + */
> > +static int nanddev_find_ecc_engine(struct nand_device *nand)  
> 
> Can we pass the conf in argument instead of reading it from
> nand->ecc.user_conf?
> 
> > +{
> > +	bool is_spinand = mtd_type_is_spinand(&nand->mtd);  
> 
> And here is the reason for the SPINAND type.
> 
> > +
> > +	/* Read the user desires in terms of ECC engine/configuration */
> > +	nand_ecc_read_user_conf(nand);
> > +
> > +	/* No ECC engine requestedn, let's return without error */
> > +	if (nand->ecc.user_conf.mode == NAND_ECC_NONE)
> > +		return 0;
> > +
> > +	/* Raw NAND default mode is hardware */
> > +	if (!is_spinand && nand->ecc.user_conf.mode < 0)
> > +		nand->ecc.user_conf.mode = NAND_ECC_HW;  
> 
> We should let the raw NAND layer take this decision (actually, it's
> even a raw NAND controller driver decision). Please complain if
> user_conf.mode is invalid.
> This way you won't need the SPINAND type you added in one of your
> previous patch.
> 
> > +
> > +	/* SPI-NAND default mode is on-die */
> > +	if (is_spinand && nand->ecc.user_conf.mode < 0)
> > +		nand->ecc.user_conf.mode = NAND_ECC_ON_DIE;
> > +
> > +	switch (nand->ecc.user_conf.mode) {
> > +	case NAND_ECC_SOFT:
> > +		nand->ecc.engine = nand_ecc_sw_get_engine(nand);
> > +		break;
> > +	case NAND_ECC_ON_DIE:
> > +		if (is_spinand)
> > +			nand->ecc.engine = spinand_ondie_ecc_get_engine();  
> 
> So, maybe it's worth having the ondie ECC engine instance directly
> embedded in nand_device instead of spinand, or maybe just a pointer, so
> that you don't reserve extra space when the NAND device does not have
> on-die ECC.
> 
> > +		else
> > +			pr_err("On-die ECC engines for non SPI devices not supported yet\n");
> > +		break;
> > +	case NAND_ECC_HW:
> > +		pr_err("Hardware ECC engines not supported yet\n");
> > +		break;
> > +	default:
> > +		pr_err("Missing ECC engine property\n");
> > +	}
> > +
> > +	if (!nand->ecc.engine)
> > +		return  -EINVAL;
> > +
> > +	return 0;
> > +}  
> 

I think this is the root patch were our ideas diverge. For me, each
'ECC engine' has a _get() helper and the NAND core decides which one to
call to retrieve the right engine. Can you please explain what was your
idea if this one does not fit?

Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the
user ECC mode and algo, so I cannot let the raw NAND core (or a raw
NAND controller driver) or the SPI NAND core decide which mode is the
default if not provided by the user.


Thanks,
Miquèl
Boris Brezillon Feb. 25, 2019, 4:34 p.m. UTC | #3
On Mon, 25 Feb 2019 17:01:18 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019
> 15:44:31 +0100:
> 
> > On Thu, 21 Feb 2019 13:58:04 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Add the logic in the NAND core to find the right ECC engine depending
> > > on the NAND chip requirements and the user desires. Right now, the
> > > choice may be made between (more will come):
> > > * software Hamming
> > > * software BCH
> > > * on-die (SPI-NAND devices only)
> > > 
> > > Once the ECC engine has been found, the ECC engine must be
> > > configured.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/mtd/nand/core.c  | 107 +++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mtd/nand.h |   4 ++
> > >  2 files changed, 111 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> > > index 872d46b5fc0f..9feb118c9f68 100644
> > > --- a/drivers/mtd/nand/core.c
> > > +++ b/drivers/mtd/nand/core.c
> > > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len)
> > >  }
> > >  EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks);
> > >  
> > > +/**
> > > + * nanddev_find_ecc_engine() - Find a suitable ECC engine
> > > + * @nand: NAND device
> > > + */
> > > +static int nanddev_find_ecc_engine(struct nand_device *nand)    
> > 
> > Can we pass the conf in argument instead of reading it from
> > nand->ecc.user_conf?
> >   
> > > +{
> > > +	bool is_spinand = mtd_type_is_spinand(&nand->mtd);    
> > 
> > And here is the reason for the SPINAND type.
> >   
> > > +
> > > +	/* Read the user desires in terms of ECC engine/configuration */
> > > +	nand_ecc_read_user_conf(nand);
> > > +
> > > +	/* No ECC engine requestedn, let's return without error */
> > > +	if (nand->ecc.user_conf.mode == NAND_ECC_NONE)
> > > +		return 0;
> > > +
> > > +	/* Raw NAND default mode is hardware */
> > > +	if (!is_spinand && nand->ecc.user_conf.mode < 0)
> > > +		nand->ecc.user_conf.mode = NAND_ECC_HW;    
> > 
> > We should let the raw NAND layer take this decision (actually, it's
> > even a raw NAND controller driver decision). Please complain if
> > user_conf.mode is invalid.
> > This way you won't need the SPINAND type you added in one of your
> > previous patch.
> >   
> > > +
> > > +	/* SPI-NAND default mode is on-die */
> > > +	if (is_spinand && nand->ecc.user_conf.mode < 0)
> > > +		nand->ecc.user_conf.mode = NAND_ECC_ON_DIE;
> > > +
> > > +	switch (nand->ecc.user_conf.mode) {
> > > +	case NAND_ECC_SOFT:
> > > +		nand->ecc.engine = nand_ecc_sw_get_engine(nand);
> > > +		break;
> > > +	case NAND_ECC_ON_DIE:
> > > +		if (is_spinand)
> > > +			nand->ecc.engine = spinand_ondie_ecc_get_engine();    
> > 
> > So, maybe it's worth having the ondie ECC engine instance directly
> > embedded in nand_device instead of spinand, or maybe just a pointer, so
> > that you don't reserve extra space when the NAND device does not have
> > on-die ECC.
> >   
> > > +		else
> > > +			pr_err("On-die ECC engines for non SPI devices not supported yet\n");
> > > +		break;
> > > +	case NAND_ECC_HW:
> > > +		pr_err("Hardware ECC engines not supported yet\n");
> > > +		break;
> > > +	default:
> > > +		pr_err("Missing ECC engine property\n");
> > > +	}
> > > +
> > > +	if (!nand->ecc.engine)
> > > +		return  -EINVAL;
> > > +
> > > +	return 0;
> > > +}    
> >   
> 
> I think this is the root patch were our ideas diverge. For me, each
> 'ECC engine' has a _get() helper and the NAND core decides which one to
> call to retrieve the right engine. Can you please explain what was your
> idea if this one does not fit?

Each class of ECC engine has its own way of getting a pointer to an ECC
engine instance:

- ondie: the engine is directly attached to the device and can be
  retrieved by accessing a nand_device field (nanddev->ondie_ecc?).
- sw ECC: you can create a new instance for each device and possibly
  pass the OOB layout you want to use (assuming you don't want to use
  the default one)
- HW-controller-side engine: refers to the controller device (parent
  node) or the device pointed by the ecc-engine DT prop (we'll probably
  need a way to pass this info when for non-DT platforms). For this one
  we'll add a nanddev_get_hw_ecc_engine() which will search for all
  registered HW ECC engines and try to match with some search keys (in
  case of DT, it's the ->of_node pointer address)

Clearly, for SW-based ECC, you'll need more than just the nanddev
object, as layouts can differ depending on the controller driver. So,
what I'm suggesting is to have a

	nand_create_sw_ecc_engine(algo, layout) 

funtion that returns this ECC engine instance which the driver will
then attach to the NAND device.

> 
> Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the
> user ECC mode and algo, so I cannot let the raw NAND core (or a raw
> NAND controller driver) or the SPI NAND core decide which mode is the
> default if not provided by the user.

Except this prop is optional in most cases, and the default value is
not always the same, which is why I think this ECC engine retrieval step
should be left to each sub-layer (and sometimes to the controller driver
behind it). Maybe you can provide helpers to help with that, but I
don't think taking this decision here, based on the bus type, is a good
idea. And I also don't like the idea of adding a new SPINAND type.
Boris Brezillon Feb. 25, 2019, 6:48 p.m. UTC | #4
On Mon, 25 Feb 2019 17:34:22 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Mon, 25 Feb 2019 17:01:18 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019
> > 15:44:31 +0100:
> >   
> > > On Thu, 21 Feb 2019 13:58:04 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > Add the logic in the NAND core to find the right ECC engine depending
> > > > on the NAND chip requirements and the user desires. Right now, the
> > > > choice may be made between (more will come):
> > > > * software Hamming
> > > > * software BCH
> > > > * on-die (SPI-NAND devices only)
> > > > 
> > > > Once the ECC engine has been found, the ECC engine must be
> > > > configured.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/mtd/nand/core.c  | 107 +++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/mtd/nand.h |   4 ++
> > > >  2 files changed, 111 insertions(+)
> > > > 
> > > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> > > > index 872d46b5fc0f..9feb118c9f68 100644
> > > > --- a/drivers/mtd/nand/core.c
> > > > +++ b/drivers/mtd/nand/core.c
> > > > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks);
> > > >  
> > > > +/**
> > > > + * nanddev_find_ecc_engine() - Find a suitable ECC engine
> > > > + * @nand: NAND device
> > > > + */
> > > > +static int nanddev_find_ecc_engine(struct nand_device *nand)      
> > > 
> > > Can we pass the conf in argument instead of reading it from
> > > nand->ecc.user_conf?
> > >     
> > > > +{
> > > > +	bool is_spinand = mtd_type_is_spinand(&nand->mtd);      
> > > 
> > > And here is the reason for the SPINAND type.
> > >     
> > > > +
> > > > +	/* Read the user desires in terms of ECC engine/configuration */
> > > > +	nand_ecc_read_user_conf(nand);
> > > > +
> > > > +	/* No ECC engine requestedn, let's return without error */
> > > > +	if (nand->ecc.user_conf.mode == NAND_ECC_NONE)
> > > > +		return 0;
> > > > +
> > > > +	/* Raw NAND default mode is hardware */
> > > > +	if (!is_spinand && nand->ecc.user_conf.mode < 0)
> > > > +		nand->ecc.user_conf.mode = NAND_ECC_HW;      
> > > 
> > > We should let the raw NAND layer take this decision (actually, it's
> > > even a raw NAND controller driver decision). Please complain if
> > > user_conf.mode is invalid.
> > > This way you won't need the SPINAND type you added in one of your
> > > previous patch.
> > >     
> > > > +
> > > > +	/* SPI-NAND default mode is on-die */
> > > > +	if (is_spinand && nand->ecc.user_conf.mode < 0)
> > > > +		nand->ecc.user_conf.mode = NAND_ECC_ON_DIE;
> > > > +
> > > > +	switch (nand->ecc.user_conf.mode) {
> > > > +	case NAND_ECC_SOFT:
> > > > +		nand->ecc.engine = nand_ecc_sw_get_engine(nand);
> > > > +		break;
> > > > +	case NAND_ECC_ON_DIE:
> > > > +		if (is_spinand)
> > > > +			nand->ecc.engine = spinand_ondie_ecc_get_engine();      
> > > 
> > > So, maybe it's worth having the ondie ECC engine instance directly
> > > embedded in nand_device instead of spinand, or maybe just a pointer, so
> > > that you don't reserve extra space when the NAND device does not have
> > > on-die ECC.
> > >     
> > > > +		else
> > > > +			pr_err("On-die ECC engines for non SPI devices not supported yet\n");
> > > > +		break;
> > > > +	case NAND_ECC_HW:
> > > > +		pr_err("Hardware ECC engines not supported yet\n");
> > > > +		break;
> > > > +	default:
> > > > +		pr_err("Missing ECC engine property\n");
> > > > +	}
> > > > +
> > > > +	if (!nand->ecc.engine)
> > > > +		return  -EINVAL;
> > > > +
> > > > +	return 0;
> > > > +}      
> > >     
> > 
> > I think this is the root patch were our ideas diverge. For me, each
> > 'ECC engine' has a _get() helper and the NAND core decides which one to
> > call to retrieve the right engine. Can you please explain what was your
> > idea if this one does not fit?  
> 
> Each class of ECC engine has its own way of getting a pointer to an ECC
> engine instance:
> 
> - ondie: the engine is directly attached to the device and can be
>   retrieved by accessing a nand_device field (nanddev->ondie_ecc?).
> - sw ECC: you can create a new instance for each device and possibly
>   pass the OOB layout you want to use (assuming you don't want to use
>   the default one)
> - HW-controller-side engine: refers to the controller device (parent
>   node) or the device pointed by the ecc-engine DT prop (we'll probably
>   need a way to pass this info when for non-DT platforms). For this one
>   we'll add a nanddev_get_hw_ecc_engine() which will search for all
>   registered HW ECC engines and try to match with some search keys (in
>   case of DT, it's the ->of_node pointer address)
> 
> Clearly, for SW-based ECC, you'll need more than just the nanddev
> object, as layouts can differ depending on the controller driver. So,
> what I'm suggesting is to have a
> 
> 	nand_create_sw_ecc_engine(algo, layout) 
> 
> funtion that returns this ECC engine instance which the driver will
> then attach to the NAND device.

I'm reconsidering what I said here. I guess having the layout set by
the driver before nand_ecc_sw_get_engine() is called could do the
trick. We just need to make sure the layout provided has enough space
to store ECC bytes at context creation time.

> 
> > 
> > Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the
> > user ECC mode and algo, so I cannot let the raw NAND core (or a raw
> > NAND controller driver) or the SPI NAND core decide which mode is the
> > default if not provided by the user.  
> 
> Except this prop is optional in most cases, and the default value is
> not always the same, which is why I think this ECC engine retrieval step
> should be left to each sub-layer (and sometimes to the controller driver
> behind it). Maybe you can provide helpers to help with that, but I
> don't think taking this decision here, based on the bus type, is a good
> idea. And I also don't like the idea of adding a new SPINAND type.

Hm, after thinking a bit more about it, maybe we could have something
in-between: let the controller driver or sub-layer specify what the
default values are (provider/mode and possibly algorithm if that makes
sense) so that this generic function can use these defaults when
nand->ecc.user_conf.{mode,algo} == UNKNOWN.
Miquel Raynal Feb. 26, 2019, 3:59 p.m. UTC | #5
Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 25 Feb
2019 19:48:03 +0100:

> > > Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the
> > > user ECC mode and algo, so I cannot let the raw NAND core (or a raw
> > > NAND controller driver) or the SPI NAND core decide which mode is the
> > > default if not provided by the user.    
> > 
> > Except this prop is optional in most cases, and the default value is
> > not always the same, which is why I think this ECC engine retrieval step
> > should be left to each sub-layer (and sometimes to the controller driver
> > behind it). Maybe you can provide helpers to help with that, but I
> > don't think taking this decision here, based on the bus type, is a good
> > idea. And I also don't like the idea of adding a new SPINAND type.  
> 
> Hm, after thinking a bit more about it, maybe we could have something
> in-between: let the controller driver or sub-layer specify what the
> default values are (provider/mode and possibly algorithm if that makes
> sense) so that this generic function can use these defaults when
> nand->ecc.user_conf.{mode,algo} == UNKNOWN.

Having a "default_provider" in struct nand_ecc could to the trick. It
would be filled by the controller drivers/sublayers before
nanddev_ecc_engine_init() is called.


Thanks,
Miquèl
Boris Brezillon Feb. 26, 2019, 4:04 p.m. UTC | #6
On Tue, 26 Feb 2019 16:59:23 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 25 Feb
> 2019 19:48:03 +0100:
> 
> > > > Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the
> > > > user ECC mode and algo, so I cannot let the raw NAND core (or a raw
> > > > NAND controller driver) or the SPI NAND core decide which mode is the
> > > > default if not provided by the user.      
> > > 
> > > Except this prop is optional in most cases, and the default value is
> > > not always the same, which is why I think this ECC engine retrieval step
> > > should be left to each sub-layer (and sometimes to the controller driver
> > > behind it). Maybe you can provide helpers to help with that, but I
> > > don't think taking this decision here, based on the bus type, is a good
> > > idea. And I also don't like the idea of adding a new SPINAND type.    
> > 
> > Hm, after thinking a bit more about it, maybe we could have something
> > in-between: let the controller driver or sub-layer specify what the
> > default values are (provider/mode and possibly algorithm if that makes
> > sense) so that this generic function can use these defaults when
> > nand->ecc.user_conf.{mode,algo} == UNKNOWN.  
> 
> Having a "default_provider" in struct nand_ecc could to the trick. It
> would be filled by the controller drivers/sublayers before
> nanddev_ecc_engine_init() is called.

You'll also need a default_algo, at least for SW ECC. Note that other
props, like strength, step-size or flags (like the maximize flag) can
be left undefined as well, so providing default vals for those ones (or
deriving them from ECC requirements) might make sense too.
Miquel Raynal Feb. 27, 2019, 2:07 p.m. UTC | #7
Hi Boris,

Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019
15:44:31 +0100:

> On Thu, 21 Feb 2019 13:58:04 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Add the logic in the NAND core to find the right ECC engine depending
> > on the NAND chip requirements and the user desires. Right now, the
> > choice may be made between (more will come):
> > * software Hamming
> > * software BCH
> > * on-die (SPI-NAND devices only)
> > 
> > Once the ECC engine has been found, the ECC engine must be
> > configured.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/core.c  | 107 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/nand.h |   4 ++
> >  2 files changed, 111 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> > index 872d46b5fc0f..9feb118c9f68 100644
> > --- a/drivers/mtd/nand/core.c
> > +++ b/drivers/mtd/nand/core.c
> > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len)
> >  }
> >  EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks);
> >  
> > +/**
> > + * nanddev_find_ecc_engine() - Find a suitable ECC engine
> > + * @nand: NAND device
> > + */
> > +static int nanddev_find_ecc_engine(struct nand_device *nand)  
> 
> Can we pass the conf in argument instead of reading it from
> nand->ecc.user_conf?

I just changed the root structures, there is a

       struct nand_ecc_conf defaults;

entry now in the nand_ecc_engine structure, which every layer/driver
can overwrite to inform the NAND core of a default value.

Later in this function, I check user_conf (which I might call
"desires" now that we have a "defaults" and a "requirements" entries),
but if the value is missing I fallback to the "defaults" one if filled.
Having all these structures being passed as parameters does not make
sense to me so I would prefer sticking to the single "nand" parameter.

> 
> > +{
> > +	bool is_spinand = mtd_type_is_spinand(&nand->mtd);  
> 
> And here is the reason for the SPINAND type.

This does not exist anymore as you suggested.

> 
> > +
> > +	/* Read the user desires in terms of ECC engine/configuration */
> > +	nand_ecc_read_user_conf(nand);
> > +
> > +	/* No ECC engine requestedn, let's return without error */
> > +	if (nand->ecc.user_conf.mode == NAND_ECC_NONE)
> > +		return 0;
> > +
> > +	/* Raw NAND default mode is hardware */
> > +	if (!is_spinand && nand->ecc.user_conf.mode < 0)
> > +		nand->ecc.user_conf.mode = NAND_ECC_HW;  
> 
> We should let the raw NAND layer take this decision (actually, it's
> even a raw NAND controller driver decision). Please complain if
> user_conf.mode is invalid.
> This way you won't need the SPINAND type you added in one of your
> previous patch.

See above for the solution I choose.

> 
> > +
> > +	/* SPI-NAND default mode is on-die */
> > +	if (is_spinand && nand->ecc.user_conf.mode < 0)
> > +		nand->ecc.user_conf.mode = NAND_ECC_ON_DIE;
> > +
> > +	switch (nand->ecc.user_conf.mode) {
> > +	case NAND_ECC_SOFT:
> > +		nand->ecc.engine = nand_ecc_sw_get_engine(nand);
> > +		break;
> > +	case NAND_ECC_ON_DIE:
> > +		if (is_spinand)
> > +			nand->ecc.engine = spinand_ondie_ecc_get_engine();  
> 
> So, maybe it's worth having the ondie ECC engine instance directly
> embedded in nand_device instead of spinand, or maybe just a pointer, so
> that you don't reserve extra space when the NAND device does not have
> on-die ECC.

It is now: the nand_ecc_engine structure features a *engine and a
*ondie_engine pointer.

The nand_ecc_get_ondie_engine(nand) helper just
returns nand->ecc.ondie_engine.

> 
> > +		else
> > +			pr_err("On-die ECC engines for non SPI devices not supported yet\n");
> > +		break;
> > +	case NAND_ECC_HW:
> > +		pr_err("Hardware ECC engines not supported yet\n");
> > +		break;
> > +	default:
> > +		pr_err("Missing ECC engine property\n");
> > +	}
> > +
> > +	if (!nand->ecc.engine)
> > +		return  -EINVAL;
> > +
> > +	return 0;
> > +}  
> 


Thanks,
Miquèl
Boris Brezillon Feb. 27, 2019, 2:30 p.m. UTC | #8
On Wed, 27 Feb 2019 15:07:13 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019
> 15:44:31 +0100:
> 
> > On Thu, 21 Feb 2019 13:58:04 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Add the logic in the NAND core to find the right ECC engine depending
> > > on the NAND chip requirements and the user desires. Right now, the
> > > choice may be made between (more will come):
> > > * software Hamming
> > > * software BCH
> > > * on-die (SPI-NAND devices only)
> > > 
> > > Once the ECC engine has been found, the ECC engine must be
> > > configured.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/mtd/nand/core.c  | 107 +++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mtd/nand.h |   4 ++
> > >  2 files changed, 111 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> > > index 872d46b5fc0f..9feb118c9f68 100644
> > > --- a/drivers/mtd/nand/core.c
> > > +++ b/drivers/mtd/nand/core.c
> > > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len)
> > >  }
> > >  EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks);
> > >  
> > > +/**
> > > + * nanddev_find_ecc_engine() - Find a suitable ECC engine
> > > + * @nand: NAND device
> > > + */
> > > +static int nanddev_find_ecc_engine(struct nand_device *nand)    
> > 
> > Can we pass the conf in argument instead of reading it from
> > nand->ecc.user_conf?  
> 
> I just changed the root structures, there is a
> 
>        struct nand_ecc_conf defaults;
> 
> entry now in the nand_ecc_engine structure, which every layer/driver
> can overwrite to inform the NAND core of a default value.
> 
> Later in this function, I check user_conf (which I might call
> "desires" now that we have a "defaults" and a "requirements" entries),
> but if the value is missing I fallback to the "defaults" one if filled.
> Having all these structures being passed as parameters does not make
> sense to me so I would prefer sticking to the single "nand" parameter.
> 
> >   
> > > +{
> > > +	bool is_spinand = mtd_type_is_spinand(&nand->mtd);    
> > 
> > And here is the reason for the SPINAND type.  
> 
> This does not exist anymore as you suggested.
> 
> >   
> > > +
> > > +	/* Read the user desires in terms of ECC engine/configuration */
> > > +	nand_ecc_read_user_conf(nand);
> > > +
> > > +	/* No ECC engine requestedn, let's return without error */
> > > +	if (nand->ecc.user_conf.mode == NAND_ECC_NONE)
> > > +		return 0;
> > > +
> > > +	/* Raw NAND default mode is hardware */
> > > +	if (!is_spinand && nand->ecc.user_conf.mode < 0)
> > > +		nand->ecc.user_conf.mode = NAND_ECC_HW;    
> > 
> > We should let the raw NAND layer take this decision (actually, it's
> > even a raw NAND controller driver decision). Please complain if
> > user_conf.mode is invalid.
> > This way you won't need the SPINAND type you added in one of your
> > previous patch.  
> 
> See above for the solution I choose.
> 
> >   
> > > +
> > > +	/* SPI-NAND default mode is on-die */
> > > +	if (is_spinand && nand->ecc.user_conf.mode < 0)
> > > +		nand->ecc.user_conf.mode = NAND_ECC_ON_DIE;
> > > +
> > > +	switch (nand->ecc.user_conf.mode) {
> > > +	case NAND_ECC_SOFT:
> > > +		nand->ecc.engine = nand_ecc_sw_get_engine(nand);
> > > +		break;
> > > +	case NAND_ECC_ON_DIE:
> > > +		if (is_spinand)
> > > +			nand->ecc.engine = spinand_ondie_ecc_get_engine();    
> > 
> > So, maybe it's worth having the ondie ECC engine instance directly
> > embedded in nand_device instead of spinand, or maybe just a pointer, so
> > that you don't reserve extra space when the NAND device does not have
> > on-die ECC.  
> 
> It is now: the nand_ecc_engine structure features a *engine and a
> *ondie_engine pointer.
> 
> The nand_ecc_get_ondie_engine(nand) helper just
> returns nand->ecc.ondie_engine.

Ack to all of this.
diff mbox series

Patch

diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
index 872d46b5fc0f..9feb118c9f68 100644
--- a/drivers/mtd/nand/core.c
+++ b/drivers/mtd/nand/core.c
@@ -207,6 +207,113 @@  int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len)
 }
 EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks);
 
+/**
+ * nanddev_find_ecc_engine() - Find a suitable ECC engine
+ * @nand: NAND device
+ */
+static int nanddev_find_ecc_engine(struct nand_device *nand)
+{
+	bool is_spinand = mtd_type_is_spinand(&nand->mtd);
+
+	/* Read the user desires in terms of ECC engine/configuration */
+	nand_ecc_read_user_conf(nand);
+
+	/* No ECC engine requestedn, let's return without error */
+	if (nand->ecc.user_conf.mode == NAND_ECC_NONE)
+		return 0;
+
+	/* Raw NAND default mode is hardware */
+	if (!is_spinand && nand->ecc.user_conf.mode < 0)
+		nand->ecc.user_conf.mode = NAND_ECC_HW;
+
+	/* SPI-NAND default mode is on-die */
+	if (is_spinand && nand->ecc.user_conf.mode < 0)
+		nand->ecc.user_conf.mode = NAND_ECC_ON_DIE;
+
+	switch (nand->ecc.user_conf.mode) {
+	case NAND_ECC_SOFT:
+		nand->ecc.engine = nand_ecc_sw_get_engine(nand);
+		break;
+	case NAND_ECC_ON_DIE:
+		if (is_spinand)
+			nand->ecc.engine = spinand_ondie_ecc_get_engine();
+		else
+			pr_err("On-die ECC engines for non SPI devices not supported yet\n");
+		break;
+	case NAND_ECC_HW:
+		pr_err("Hardware ECC engines not supported yet\n");
+		break;
+	default:
+		pr_err("Missing ECC engine property\n");
+	}
+
+	if (!nand->ecc.engine)
+		return  -EINVAL;
+
+	return 0;
+}
+
+/**
+ * nanddev_find_ecc_configuration() - Find a suitable ECC configuration
+ * @nand: NAND device
+ */
+static int nanddev_find_ecc_configuration(struct nand_device *nand)
+{
+	int ret;
+
+	if (!nand->ecc.engine)
+		return -ENOTSUPP;
+
+	ret = nand_ecc_init_ctx(nand);
+	if (ret)
+		return ret;
+
+	if (!nand_ecc_correction_is_enough(nand))
+		pr_warn("WARNING: %s: the ECC used on your system is too weak compared to the one required by the NAND chip\n",
+			nand->mtd.name);
+
+	return 0;
+}
+
+/**
+ * nanddev_ecc_engine_init() - Initialize an ECC engine for the chip
+ * @nand: NAND device
+ */
+int nanddev_ecc_engine_init(struct nand_device *nand)
+{
+	int ret;
+
+	/* Look for the ECC engine to use */
+	ret = nanddev_find_ecc_engine(nand);
+	if (ret) {
+		pr_err("No ECC engine found\n");
+		return ret;
+	}
+
+	/* No ECC engine requested */
+	if (!nand->ecc.engine)
+		return 0;
+
+	/* Configure the engine: balance user input and chip requirements */
+	ret = nanddev_find_ecc_configuration(nand);
+	if (ret) {
+		pr_err("No suitable ECC configuration\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * nanddev_ecc_engine_cleanup() - Cleanup ECC engine initializations
+ * @nand: NAND device
+ */
+void nanddev_ecc_engine_cleanup(struct nand_device *nand)
+{
+	if (nand->ecc.engine)
+		nand_ecc_cleanup_ctx(nand);
+}
+
 /**
  * nanddev_init() - Initialize a NAND device
  * @nand: NAND device
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 534c07fab9de..bf949f55c139 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -844,6 +844,10 @@  bool nanddev_isreserved(struct nand_device *nand, const struct nand_pos *pos);
 int nanddev_erase(struct nand_device *nand, const struct nand_pos *pos);
 int nanddev_markbad(struct nand_device *nand, const struct nand_pos *pos);
 
+/* ECC related functions */
+int nanddev_ecc_engine_init(struct nand_device *nand);
+void nanddev_ecc_engine_cleanup(struct nand_device *nand);
+
 /* BBT related functions */
 enum nand_bbt_block_status {
 	NAND_BBT_BLOCK_STATUS_UNKNOWN,