| Message ID | 20190722055621.23526-7-sshivamurthy@micron.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | Introduce generic ONFI support | expand |
Hi Shiva, shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:19 +0200: > From: Shivamurthy Shastri <sshivamurthy@micron.com> > I am not sure the "turn implemenatation generic" title describes what you do. > Driver is redesigned using parameter page to support Micron SPI NAND > flashes. Redesigned is perhaps a bit too much. " > The reason why spinand_select_op_variant globalized is that the Micron > driver no longer calling spinand_match_and_init. > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > --- > drivers/mtd/nand/spi/core.c | 2 +- > drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++---------- > include/linux/mtd/spinand.h | 4 +++ > 3 files changed, 49 insertions(+), 18 deletions(-) > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index 7ae76dab9141..aae715d388b7 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -920,7 +920,7 @@ static void spinand_manufacturer_cleanup(struct spinand_device *spinand) > return spinand->manufacturer->ops->cleanup(spinand); > } > > -static const struct spi_mem_op * > +const struct spi_mem_op * > spinand_select_op_variant(struct spinand_device *spinand, > const struct spinand_op_variants *variants) > { > diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c > index 95bc5264ebc1..6fde93ec23a1 100644 > --- a/drivers/mtd/nand/spi/micron.c > +++ b/drivers/mtd/nand/spi/micron.c > @@ -90,22 +90,10 @@ static int micron_ecc_get_status(struct spinand_device *spinand, > return -EINVAL; > } > > -static const struct spinand_info micron_spinand_table[] = { > - SPINAND_INFO("MT29F2G01ABAGD", 0x24, > - NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1), > - NAND_ECCREQ(8, 512), > - SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > - &write_cache_variants, > - &update_cache_variants), > - 0, > - SPINAND_ECCINFO(µn_ooblayout_ops, > - micron_ecc_get_status)), > -}; > - I don't know if it is wise to drop this structure. > static int micron_spinand_detect(struct spinand_device *spinand) > { > + const struct spi_mem_op *op; > u8 *id = spinand->id.data; > - int ret; > > /* > * Micron SPI NAND read ID need a dummy byte, > @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct spinand_device *spinand) > if (id[1] != SPINAND_MFR_MICRON) > return 0; > > - ret = spinand_match_and_init(spinand, micron_spinand_table, > - ARRAY_SIZE(micron_spinand_table), id[2]); I am not sure this is the right solution. I would keep this call and overwrite what you need to overwrite with the fixup hook. > - if (ret) > - return ret; > + spinand->flags = 0; > + spinand->eccinfo.get_status = micron_ecc_get_status; > + spinand->eccinfo.ooblayout = µn_ooblayout_ops; > + > + op = spinand_select_op_variant(spinand, > + &read_cache_variants); > + if (!op) > + return -ENOTSUPP; > + > + spinand->op_templates.read_cache = op; > + > + op = spinand_select_op_variant(spinand, > + &write_cache_variants); > + if (!op) > + return -ENOTSUPP; > + > + spinand->op_templates.write_cache = op; > + > + op = spinand_select_op_variant(spinand, > + &update_cache_variants); > + spinand->op_templates.update_cache = op; > > return 1; > } > > +static void micron_fixup_param_page(struct spinand_device *spinand, > + struct nand_onfi_params *p) > +{ > + /* > + * As per Micron datasheets vendor[83] is defined as > + * die_select_feature > + */ > + if (p->vendor[83] && !p->interleaved_bits) > + spinand->base.memorg.planes_per_lun = 1 << p->vendor[0]; > + > + spinand->base.memorg.ntargets = p->lun_count; > + spinand->base.memorg.luns_per_target = 1; > + > + /* > + * As per Micron datasheets, > + * vendor[82] is ECC maximum correctability > + */ > + spinand->base.eccreq.strength = p->vendor[82]; > + spinand->base.eccreq.step_size = 512; > +} > + > static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = { > .detect = micron_spinand_detect, > + .fixup_param_page = micron_fixup_param_page, > }; > > const struct spinand_manufacturer micron_spinand_manufacturer = { > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index fea820a20bc9..ddb2194273a8 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -461,4 +461,8 @@ int spinand_match_and_init(struct spinand_device *dev, > int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val); > int spinand_select_target(struct spinand_device *spinand, unsigned int target); > > +const struct spi_mem_op * > +spinand_select_op_variant(struct spinand_device *spinand, > + const struct spinand_op_variants *variants); > + > #endif /* __LINUX_MTD_SPINAND_H */ Thanks, Miquèl
Hi Miquel, > > Hi Shiva, > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:19 +0200: > > > From: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > I am not sure the "turn implemenatation generic" title describes what > you do. > > > Driver is redesigned using parameter page to support Micron SPI NAND > > flashes. > > Redesigned is perhaps a bit too much. > > " > > The reason why spinand_select_op_variant globalized is that the Micron > > driver no longer calling spinand_match_and_init. > > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > > --- > > drivers/mtd/nand/spi/core.c | 2 +- > > drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++------- > --- > > include/linux/mtd/spinand.h | 4 +++ > > 3 files changed, 49 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > index 7ae76dab9141..aae715d388b7 100644 > > --- a/drivers/mtd/nand/spi/core.c > > +++ b/drivers/mtd/nand/spi/core.c > > @@ -920,7 +920,7 @@ static void spinand_manufacturer_cleanup(struct > spinand_device *spinand) > > return spinand->manufacturer->ops->cleanup(spinand); > > } > > > > -static const struct spi_mem_op * > > +const struct spi_mem_op * > > spinand_select_op_variant(struct spinand_device *spinand, > > const struct spinand_op_variants *variants) > > { > > diff --git a/drivers/mtd/nand/spi/micron.c > b/drivers/mtd/nand/spi/micron.c > > index 95bc5264ebc1..6fde93ec23a1 100644 > > --- a/drivers/mtd/nand/spi/micron.c > > +++ b/drivers/mtd/nand/spi/micron.c > > @@ -90,22 +90,10 @@ static int micron_ecc_get_status(struct > spinand_device *spinand, > > return -EINVAL; > > } > > > > -static const struct spinand_info micron_spinand_table[] = { > > - SPINAND_INFO("MT29F2G01ABAGD", 0x24, > > - NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1), > > - NAND_ECCREQ(8, 512), > > - SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > > - &write_cache_variants, > > - &update_cache_variants), > > - 0, > > - SPINAND_ECCINFO(µn_ooblayout_ops, > > - micron_ecc_get_status)), > > -}; > > - > > I don't know if it is wise to drop this structure. > > > static int micron_spinand_detect(struct spinand_device *spinand) > > { > > + const struct spi_mem_op *op; > > u8 *id = spinand->id.data; > > - int ret; > > > > /* > > * Micron SPI NAND read ID need a dummy byte, > > @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct > spinand_device *spinand) > > if (id[1] != SPINAND_MFR_MICRON) > > return 0; > > > > - ret = spinand_match_and_init(spinand, micron_spinand_table, > > - ARRAY_SIZE(micron_spinand_table), > id[2]); > > I am not sure this is the right solution. I would keep this call and > overwrite what you need to overwrite with the fixup hook. > Then, I will have dummy structure like below. static const struct spinand_info micron_spinand_table[] = { SPINAND_INFO(NULL, 0, NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0), NAND_ECCREQ(0, 0), SPINAND_INFO_OP_VARIANTS(&read_cache_variants, &write_cache_variants, &update_cache_variants), 0, SPINAND_ECCINFO(µn_ooblayout_ops, micron_ecc_get_status)), }; Let me know if you are thinking for different approach. > > - if (ret) > > - return ret; > > + spinand->flags = 0; > > + spinand->eccinfo.get_status = micron_ecc_get_status; > > + spinand->eccinfo.ooblayout = µn_ooblayout_ops; > > + > > + op = spinand_select_op_variant(spinand, > > + &read_cache_variants); > > + if (!op) > > + return -ENOTSUPP; > > + > > + spinand->op_templates.read_cache = op; > > + > > + op = spinand_select_op_variant(spinand, > > + &write_cache_variants); > > + if (!op) > > + return -ENOTSUPP; > > + > > + spinand->op_templates.write_cache = op; > > + > > + op = spinand_select_op_variant(spinand, > > + &update_cache_variants); > > + spinand->op_templates.update_cache = op; > > > > return 1; > > } > > > > +static void micron_fixup_param_page(struct spinand_device *spinand, > > + struct nand_onfi_params *p) > > +{ > > + /* > > + * As per Micron datasheets vendor[83] is defined as > > + * die_select_feature > > + */ > > + if (p->vendor[83] && !p->interleaved_bits) > > + spinand->base.memorg.planes_per_lun = 1 << p- > >vendor[0]; > > + > > + spinand->base.memorg.ntargets = p->lun_count; > > + spinand->base.memorg.luns_per_target = 1; > > + > > + /* > > + * As per Micron datasheets, > > + * vendor[82] is ECC maximum correctability > > + */ > > + spinand->base.eccreq.strength = p->vendor[82]; > > + spinand->base.eccreq.step_size = 512; > > +} > > + > > static const struct spinand_manufacturer_ops > micron_spinand_manuf_ops = { > > .detect = micron_spinand_detect, > > + .fixup_param_page = micron_fixup_param_page, > > }; > > > > const struct spinand_manufacturer micron_spinand_manufacturer = { > > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > > index fea820a20bc9..ddb2194273a8 100644 > > --- a/include/linux/mtd/spinand.h > > +++ b/include/linux/mtd/spinand.h > > @@ -461,4 +461,8 @@ int spinand_match_and_init(struct spinand_device > *dev, > > int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val); > > int spinand_select_target(struct spinand_device *spinand, unsigned int > target); > > > > +const struct spi_mem_op * > > +spinand_select_op_variant(struct spinand_device *spinand, > > + const struct spinand_op_variants *variants); > > + > > #endif /* __LINUX_MTD_SPINAND_H */ > > > > > Thanks, > Miquèl
Hi Boris, I need your opinion on the question below. "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on Mon, 19 Aug 2019 09:03:38 +0000: > Hi Miquel, > > > > > Hi Shiva, > > > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:19 +0200: > > > > > From: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > > > > I am not sure the "turn implemenatation generic" title describes what > > you do. > > > > > Driver is redesigned using parameter page to support Micron SPI NAND > > > flashes. > > > > Redesigned is perhaps a bit too much. > > > > " > > > The reason why spinand_select_op_variant globalized is that the Micron > > > driver no longer calling spinand_match_and_init. > > > > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > > > --- > > > drivers/mtd/nand/spi/core.c | 2 +- > > > drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++------- > > --- > > > include/linux/mtd/spinand.h | 4 +++ > > > 3 files changed, 49 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > > index 7ae76dab9141..aae715d388b7 100644 > > > --- a/drivers/mtd/nand/spi/core.c > > > +++ b/drivers/mtd/nand/spi/core.c > > > @@ -920,7 +920,7 @@ static void spinand_manufacturer_cleanup(struct > > spinand_device *spinand) > > > return spinand->manufacturer->ops->cleanup(spinand); > > > } > > > > > > -static const struct spi_mem_op * > > > +const struct spi_mem_op * > > > spinand_select_op_variant(struct spinand_device *spinand, > > > const struct spinand_op_variants *variants) > > > { > > > diff --git a/drivers/mtd/nand/spi/micron.c > > b/drivers/mtd/nand/spi/micron.c > > > index 95bc5264ebc1..6fde93ec23a1 100644 > > > --- a/drivers/mtd/nand/spi/micron.c > > > +++ b/drivers/mtd/nand/spi/micron.c > > > @@ -90,22 +90,10 @@ static int micron_ecc_get_status(struct > > spinand_device *spinand, > > > return -EINVAL; > > > } > > > > > > -static const struct spinand_info micron_spinand_table[] = { > > > - SPINAND_INFO("MT29F2G01ABAGD", 0x24, > > > - NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1), > > > - NAND_ECCREQ(8, 512), > > > - SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > > > - &write_cache_variants, > > > - &update_cache_variants), > > > - 0, > > > - SPINAND_ECCINFO(µn_ooblayout_ops, > > > - micron_ecc_get_status)), > > > -}; > > > - > > > > I don't know if it is wise to drop this structure. > > > > > static int micron_spinand_detect(struct spinand_device *spinand) > > > { > > > + const struct spi_mem_op *op; > > > u8 *id = spinand->id.data; > > > - int ret; > > > > > > /* > > > * Micron SPI NAND read ID need a dummy byte, > > > @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct > > spinand_device *spinand) > > > if (id[1] != SPINAND_MFR_MICRON) > > > return 0; > > > > > > - ret = spinand_match_and_init(spinand, micron_spinand_table, > > > - ARRAY_SIZE(micron_spinand_table), > > id[2]); > > > > I am not sure this is the right solution. I would keep this call and > > overwrite what you need to overwrite with the fixup hook. > > > > Then, I will have dummy structure like below. > > static const struct spinand_info micron_spinand_table[] = { > SPINAND_INFO(NULL, 0, > NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0), > NAND_ECCREQ(0, 0), > SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > &write_cache_variants, > &update_cache_variants), > 0, > SPINAND_ECCINFO(µn_ooblayout_ops, > micron_ecc_get_status)), > }; > > Let me know if you are thinking for different approach. > Thanks, Miquèl
Hello Miquel & Boris, Just a gentle reminder that I'd like some feedback. Thanks, Shiva > > Hi Boris, > > I need your opinion on the question below. > > "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote > on > Mon, 19 Aug 2019 09:03:38 +0000: > > > Hi Miquel, > > > > > > > > Hi Shiva, > > > > > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:19 +0200: > > > > > > > From: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > > > > > > > I am not sure the "turn implemenatation generic" title describes what > > > you do. > > > > > > > Driver is redesigned using parameter page to support Micron SPI NAND > > > > flashes. > > > > > > Redesigned is perhaps a bit too much. > > > > > > " > > > > The reason why spinand_select_op_variant globalized is that the > Micron > > > > driver no longer calling spinand_match_and_init. > > > > > > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > --- > > > > drivers/mtd/nand/spi/core.c | 2 +- > > > > drivers/mtd/nand/spi/micron.c | 61 +++++++++++++++++++++++++-- > ----- > > > --- > > > > include/linux/mtd/spinand.h | 4 +++ > > > > 3 files changed, 49 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > > > index 7ae76dab9141..aae715d388b7 100644 > > > > --- a/drivers/mtd/nand/spi/core.c > > > > +++ b/drivers/mtd/nand/spi/core.c > > > > @@ -920,7 +920,7 @@ static void > spinand_manufacturer_cleanup(struct > > > spinand_device *spinand) > > > > return spinand->manufacturer->ops->cleanup(spinand); > > > > } > > > > > > > > -static const struct spi_mem_op * > > > > +const struct spi_mem_op * > > > > spinand_select_op_variant(struct spinand_device *spinand, > > > > const struct spinand_op_variants *variants) > > > > { > > > > diff --git a/drivers/mtd/nand/spi/micron.c > > > b/drivers/mtd/nand/spi/micron.c > > > > index 95bc5264ebc1..6fde93ec23a1 100644 > > > > --- a/drivers/mtd/nand/spi/micron.c > > > > +++ b/drivers/mtd/nand/spi/micron.c > > > > @@ -90,22 +90,10 @@ static int micron_ecc_get_status(struct > > > spinand_device *spinand, > > > > return -EINVAL; > > > > } > > > > > > > > -static const struct spinand_info micron_spinand_table[] = { > > > > - SPINAND_INFO("MT29F2G01ABAGD", 0x24, > > > > - NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1), > > > > - NAND_ECCREQ(8, 512), > > > > - SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > > > > - &write_cache_variants, > > > > - &update_cache_variants), > > > > - 0, > > > > - SPINAND_ECCINFO(µn_ooblayout_ops, > > > > - micron_ecc_get_status)), > > > > -}; > > > > - > > > > > > I don't know if it is wise to drop this structure. > > > > > > > static int micron_spinand_detect(struct spinand_device *spinand) > > > > { > > > > + const struct spi_mem_op *op; > > > > u8 *id = spinand->id.data; > > > > - int ret; > > > > > > > > /* > > > > * Micron SPI NAND read ID need a dummy byte, > > > > @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct > > > spinand_device *spinand) > > > > if (id[1] != SPINAND_MFR_MICRON) > > > > return 0; > > > > > > > > - ret = spinand_match_and_init(spinand, micron_spinand_table, > > > > - ARRAY_SIZE(micron_spinand_table), > > > id[2]); > > > > > > I am not sure this is the right solution. I would keep this call and > > > overwrite what you need to overwrite with the fixup hook. > > > > > > > Then, I will have dummy structure like below. > > > > static const struct spinand_info micron_spinand_table[] = { > > SPINAND_INFO(NULL, 0, > > NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0), > > NAND_ECCREQ(0, 0), > > SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > > &write_cache_variants, > > &update_cache_variants), > > 0, > > SPINAND_ECCINFO(µn_ooblayout_ops, > > micron_ecc_get_status)), > > }; > > > > Let me know if you are thinking for different approach. > > > > Thanks, > Miquèl
On Mon, 19 Aug 2019 09:03:38 +0000 "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote: > > > > > static int micron_spinand_detect(struct spinand_device *spinand) > > > { > > > + const struct spi_mem_op *op; > > > u8 *id = spinand->id.data; > > > - int ret; > > > > > > /* > > > * Micron SPI NAND read ID need a dummy byte, > > > @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct > > spinand_device *spinand) > > > if (id[1] != SPINAND_MFR_MICRON) > > > return 0; > > > > > > - ret = spinand_match_and_init(spinand, micron_spinand_table, > > > - ARRAY_SIZE(micron_spinand_table), > > id[2]); > > > > I am not sure this is the right solution. I would keep this call and > > overwrite what you need to overwrite with the fixup hook. > > I'm definitely not comfortable with this whole "rely on ONFi param-page" thing. Vendors have proven to get it wrong from time to time, so before we do that, I'd like to make sure all currently supported Micron NANDs (looks like we only support MT29F2G01ABAGD, so that shouldn't be hard) expose the right thing there. For instance, are we sure the ECC layout is always the same, and if not, do we have a reliable way to extract that? > > Then, I will have dummy structure like below. > > static const struct spinand_info micron_spinand_table[] = { > SPINAND_INFO(NULL, 0, > NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0), > NAND_ECCREQ(0, 0), > SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > &write_cache_variants, > &update_cache_variants), > 0, > SPINAND_ECCINFO(µn_ooblayout_ops, > micron_ecc_get_status)), > }; > > Let me know if you are thinking for different approach. Exposing dummy entries is useless. If you're entirely sure all Micron SPI NANDs have a valid ONFi param page, then no need to use the ID-based detection. But as I said above, I feel param-page-based detection is going to be as messy as SFDP-based detection is for SPI NORs. Vendors tend to make mistakes which we have to fix to make things work. ID-based detection is much more reliable in this regard, as long as we don't have ID collisions :P. Plus, it looks like only a few manufacturers decided to use ONFi param pages to expose SPI NAND info (AFAICT, only Micron and Macronix do that), which is not surprising since the ONFi param page has been created to describe parallel NANDs not SPI NANDs (if you look closely enough, you'll notice that some fields are meaningless for SPI NANDs).
Hi Boris, Thank you for the review. > > On Mon, 19 Aug 2019 09:03:38 +0000 > "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote: > > > > > > > > static int micron_spinand_detect(struct spinand_device *spinand) > > > > { > > > > + const struct spi_mem_op *op; > > > > u8 *id = spinand->id.data; > > > > - int ret; > > > > > > > > /* > > > > * Micron SPI NAND read ID need a dummy byte, > > > > @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct > > > spinand_device *spinand) > > > > if (id[1] != SPINAND_MFR_MICRON) > > > > return 0; > > > > > > > > - ret = spinand_match_and_init(spinand, micron_spinand_table, > > > > - ARRAY_SIZE(micron_spinand_table), > > > id[2]); > > > > > > I am not sure this is the right solution. I would keep this call and > > > overwrite what you need to overwrite with the fixup hook. > > > > > I'm definitely not comfortable with this whole "rely on ONFi > param-page" thing. Vendors have proven to get it wrong from time to > time, so before we do that, I'd like to make sure all currently > supported Micron NANDs (looks like we only support MT29F2G01ABAGD, so > that shouldn't be hard) expose the right thing there. For instance, are > we sure the ECC layout is always the same, and if not, do we have a > reliable way to extract that? > > > > > Then, I will have dummy structure like below. > > > > static const struct spinand_info micron_spinand_table[] = { > > SPINAND_INFO(NULL, 0, > > NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0), > > NAND_ECCREQ(0, 0), > > SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > > &write_cache_variants, > > &update_cache_variants), > > 0, > > SPINAND_ECCINFO(µn_ooblayout_ops, > > micron_ecc_get_status)), > > }; > > > > > Let me know if you are thinking for different approach. > > Exposing dummy entries is useless. If you're entirely sure all Micron > SPI NANDs have a valid ONFi param page, then no need to use the > ID-based detection. But as I said above, I feel param-page-based > detection is going to be as messy as SFDP-based detection is for SPI > NORs. Vendors tend to make mistakes which we have to fix to make > things work. ID-based detection is much more reliable in this regard, > as long as we don't have ID collisions :P. > Plus, it looks like only a few manufacturers decided to use ONFi param > pages to expose SPI NAND info (AFAICT, only Micron and Macronix do > that), which is not surprising since the ONFi param page has been > created to describe parallel NANDs not SPI NANDs (if you look closely > enough, you'll notice that some fields are meaningless for SPI NANDs). Okay, I will send new patches with ID-based detection for the new devices. Thanks, Shiva
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index 7ae76dab9141..aae715d388b7 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -920,7 +920,7 @@ static void spinand_manufacturer_cleanup(struct spinand_device *spinand) return spinand->manufacturer->ops->cleanup(spinand); } -static const struct spi_mem_op * +const struct spi_mem_op * spinand_select_op_variant(struct spinand_device *spinand, const struct spinand_op_variants *variants) { diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c index 95bc5264ebc1..6fde93ec23a1 100644 --- a/drivers/mtd/nand/spi/micron.c +++ b/drivers/mtd/nand/spi/micron.c @@ -90,22 +90,10 @@ static int micron_ecc_get_status(struct spinand_device *spinand, return -EINVAL; } -static const struct spinand_info micron_spinand_table[] = { - SPINAND_INFO("MT29F2G01ABAGD", 0x24, - NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1), - NAND_ECCREQ(8, 512), - SPINAND_INFO_OP_VARIANTS(&read_cache_variants, - &write_cache_variants, - &update_cache_variants), - 0, - SPINAND_ECCINFO(µn_ooblayout_ops, - micron_ecc_get_status)), -}; - static int micron_spinand_detect(struct spinand_device *spinand) { + const struct spi_mem_op *op; u8 *id = spinand->id.data; - int ret; /* * Micron SPI NAND read ID need a dummy byte, @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct spinand_device *spinand) if (id[1] != SPINAND_MFR_MICRON) return 0; - ret = spinand_match_and_init(spinand, micron_spinand_table, - ARRAY_SIZE(micron_spinand_table), id[2]); - if (ret) - return ret; + spinand->flags = 0; + spinand->eccinfo.get_status = micron_ecc_get_status; + spinand->eccinfo.ooblayout = µn_ooblayout_ops; + + op = spinand_select_op_variant(spinand, + &read_cache_variants); + if (!op) + return -ENOTSUPP; + + spinand->op_templates.read_cache = op; + + op = spinand_select_op_variant(spinand, + &write_cache_variants); + if (!op) + return -ENOTSUPP; + + spinand->op_templates.write_cache = op; + + op = spinand_select_op_variant(spinand, + &update_cache_variants); + spinand->op_templates.update_cache = op; return 1; } +static void micron_fixup_param_page(struct spinand_device *spinand, + struct nand_onfi_params *p) +{ + /* + * As per Micron datasheets vendor[83] is defined as + * die_select_feature + */ + if (p->vendor[83] && !p->interleaved_bits) + spinand->base.memorg.planes_per_lun = 1 << p->vendor[0]; + + spinand->base.memorg.ntargets = p->lun_count; + spinand->base.memorg.luns_per_target = 1; + + /* + * As per Micron datasheets, + * vendor[82] is ECC maximum correctability + */ + spinand->base.eccreq.strength = p->vendor[82]; + spinand->base.eccreq.step_size = 512; +} + static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = { .detect = micron_spinand_detect, + .fixup_param_page = micron_fixup_param_page, }; const struct spinand_manufacturer micron_spinand_manufacturer = { diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h index fea820a20bc9..ddb2194273a8 100644 --- a/include/linux/mtd/spinand.h +++ b/include/linux/mtd/spinand.h @@ -461,4 +461,8 @@ int spinand_match_and_init(struct spinand_device *dev, int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val); int spinand_select_target(struct spinand_device *spinand, unsigned int target); +const struct spi_mem_op * +spinand_select_op_variant(struct spinand_device *spinand, + const struct spinand_op_variants *variants); + #endif /* __LINUX_MTD_SPINAND_H */