Message ID | 20190722055621.23526-5-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:17 +0200: "mtd: spinand: enable parameter page support" > From: Shivamurthy Shastri <sshivamurthy@micron.com> > > Some of the SPI NAND devices has parameter page, which is similar to - have a > ONFI table. regular raw NAND ONFI tables. > > But, it may not be self sufficient to propagate all the required As it may not be > parameters. Fixup function has been added in struct manufacturer to , a fixup is being added in the manufacturer structure > accommodate this. The fixup function sentence should be dropped from the commit message, see below. > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > --- > drivers/mtd/nand/spi/core.c | 134 ++++++++++++++++++++++++++++++++++++ > include/linux/mtd/spinand.h | 3 + > 2 files changed, 137 insertions(+) > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index 89f6beefb01c..7ae76dab9141 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct spinand_device *spinand, u8 lock) > return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock); > } > > +/** > + * spinand_read_param_page_op - Read parameter page operation Again, the name in the doc does not fit the function you describe > + * @spinand: the spinand SPI-NAND chip Shiva, there are way too much typos and shortcuts in your series. Please be more careful otherwise we can't focus on the technical aspects. I am not a native English speaker at all but please, plain English is not C code. We talk SPI-NAND and not spinand, we say structure and not struct, acronyms are uppercase, etc. > + * @page: page number where parameter page tables can be found ^ the > + * @buf: buffer used to store the parameter page > + * @len: length of the buffer > + * > + * Read parameter page the > + * > + * Returns 0 on success, a negative error code otherwise. > + */ > +static int spinand_parameter_page_read(struct spinand_device *spinand, > + u8 page, void *buf, unsigned int len) > +{ > + struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page); > + struct spi_mem_op pread_cache_op = > + SPINAND_PAGE_READ_FROM_CACHE_OP(false, > + 0, > + 1, > + buf, > + len); That's ok if you cross the 80 characters boundary here. You may put "0, 1," on the first line and "buf, len);" on the second. > + u8 feature; > + u8 status; > + int ret; > + > + if (len && !buf) > + return -EINVAL; > + > + ret = spinand_read_reg_op(spinand, REG_CFG, > + &feature); > + if (ret) > + return ret; > + > + /* CFG_OTP_ENABLE is used to enable parameter page access */ > + feature |= CFG_OTP_ENABLE; > + > + spinand_write_reg_op(spinand, REG_CFG, feature); > + > + ret = spi_mem_exec_op(spinand->spimem, &pread_op); > + if (ret) > + return ret; > + > + ret = spinand_wait(spinand, &status); > + if (ret < 0) > + return ret; > + > + ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op); > + if (ret) > + return ret; > + > + ret = spinand_read_reg_op(spinand, REG_CFG, > + &feature); > + if (ret) > + return ret; > + > + feature &= ~CFG_OTP_ENABLE; > + > + spinand_write_reg_op(spinand, REG_CFG, feature); > + > + return 0; > +} > + Add the kernel doc please Change the below function so that it returns 1 if the page was detected, 0 if it did not, an negative error code otherwise. > +static int spinand_param_page_detect(struct spinand_device *spinand) > +{ > + struct mtd_info *mtd = spinand_to_mtd(spinand); > + struct nand_memory_organization *memorg; > + struct nand_onfi_params *p; > + struct nand_device *base = spinand_to_nand(spinand); > + int i, ret; > + > + memorg = nanddev_get_memorg(base); > + > + /* Allocate buffer to hold parameter page */ > + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL); > + if (!p) > + return -ENOMEM; > + > + ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * 3); > + if (ret) { > + ret = 0; No, you should return the error in case of error. You will later handle the fact that there is no parameter page. > + goto free_param_page; > + } > + > + for (i = 0; i < 3; i++) { > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) == ^ If you force the parameter page to be 254 bytes long it means you limit yourself to ONFI standard. That's not a problem, but then you should mention it in the function name. > + le16_to_cpu(p->crc)) { > + if (i) > + memcpy(p, &p[i], sizeof(*p)); > + break; > + } > + } > + > + if (i == 3) { > + const void *srcbufs[3] = {p, p + 1, p + 2}; > + > + pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n"); > + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p, > + sizeof(*p)); > + > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) != > + le16_to_cpu(p->crc)) { > + pr_err("ONFI parameter recovery failed, aborting\n"); > + goto free_param_page; > + } > + } The whole for-loop and the if (i==3) condition is exactly the same as for raw NANDs and must be extracted in a generic function: 1/ extract the function from nand/raw/nand_onfi.c and put it in nand/onfi.c. 2/ then use it in this patch. > + > + parse_onfi_params(memorg, p); > + > + mtd->writesize = memorg->pagesize; > + mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize; > + mtd->oobsize = memorg->oobsize; This will be handled by nanddev_init, should be removed. > + > + /* Manufacturers may interpret the parameter page differently */ > + if (spinand->manufacturer->ops->fixup_param_page) > + spinand->manufacturer->ops->fixup_param_page(spinand, p); The whole "manufacturer fixup" should be done separately. > + > + /* Identification done, free the full parameter page and exit */ > + ret = 1; > + > +free_param_page: > + kfree(p); > + > + return ret; > +} > + > static int spinand_check_ecc_status(struct spinand_device *spinand, u8 status) > { > struct nand_device *nand = spinand_to_nand(spinand); > @@ -911,6 +1036,15 @@ static int spinand_detect(struct spinand_device *spinand) > return ret; > } > > + if (!spinand->base.memorg.pagesize) { > + ret = spinand_param_page_detect(spinand); > + if (ret <= 0) { > + dev_err(dev, "no parameter page for %*phN\n", Not sure at this stage dev will give something meaningful. Anyway I don't think we should scream at the user if his NAND is not an ONFI one so please return an error only if ret < 0. If ret == 0 or ret == 1, don't warn the user. > + SPINAND_MAX_ID_LEN, spinand->id.data); > + return -ENODEV; > + } > + } > + > if (nand->memorg.ntargets > 1 && !spinand->select_target) { > dev_err(dev, > "SPI NANDs with more than one die must implement ->select_target()\n"); > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index 4ea558bd3c46..fea820a20bc9 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -15,6 +15,7 @@ > #include <linux/mtd/nand.h> > #include <linux/spi/spi.h> > #include <linux/spi/spi-mem.h> > +#include <linux/mtd/onfi.h> > > /** > * Standard SPI NAND flash operations > @@ -209,6 +210,8 @@ struct spinand_manufacturer_ops { > int (*detect)(struct spinand_device *spinand); > int (*init)(struct spinand_device *spinand); > void (*cleanup)(struct spinand_device *spinand); > + void (*fixup_param_page)(struct spinand_device *spinand, > + struct nand_onfi_params *p); Please do this in a separate patch. > }; > > /** Thanks, Miquèl
Hi Miquel, > > Hi Shiva, > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200: > > "mtd: spinand: enable parameter page support" > > > From: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > Some of the SPI NAND devices has parameter page, which is similar to > - have a > > ONFI table. > regular raw NAND ONFI tables. > > > > > But, it may not be self sufficient to propagate all the required > As it may not be > > parameters. Fixup function has been added in struct manufacturer to > , a fixup is being added in the manufacturer structure > > accommodate this. > > The fixup function sentence should be dropped from the commit message, > see below. Okay, I will create separate patch for fixup function. > > > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > > --- > > drivers/mtd/nand/spi/core.c | 134 > ++++++++++++++++++++++++++++++++++++ > > include/linux/mtd/spinand.h | 3 + > > 2 files changed, 137 insertions(+) > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > index 89f6beefb01c..7ae76dab9141 100644 > > --- a/drivers/mtd/nand/spi/core.c > > +++ b/drivers/mtd/nand/spi/core.c > > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct > spinand_device *spinand, u8 lock) > > return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock); > > } > > > > +/** > > + * spinand_read_param_page_op - Read parameter page operation > > Again, the name in the doc does not fit the function you describe > > > + * @spinand: the spinand > SPI-NAND chip > > Shiva, there are way too much typos and shortcuts in your series. > Please be more careful otherwise we can't focus on the technical > aspects. I am not a native English speaker at all but please, plain > English is not C code. We talk SPI-NAND and not spinand, we say > structure and not struct, acronyms are uppercase, etc. > Sorry for the inconvenience caused, I will take care from next time. > > + * @page: page number where parameter page tables can be found > ^ the > > + * @buf: buffer used to store the parameter page > > + * @len: length of the buffer > > + * > > + * Read parameter page > the > > + * > > + * Returns 0 on success, a negative error code otherwise. > > + */ > > +static int spinand_parameter_page_read(struct spinand_device *spinand, > > + u8 page, void *buf, unsigned int len) > > +{ > > + struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page); > > + struct spi_mem_op pread_cache_op = > > + > SPINAND_PAGE_READ_FROM_CACHE_OP(false, > > + 0, > > + 1, > > + buf, > > + len); > > That's ok if you cross the 80 characters boundary here. You may put "0, > 1," on the first line and "buf, len);" on the second. > > > + u8 feature; > > + u8 status; > > + int ret; > > + > > + if (len && !buf) > > + return -EINVAL; > > + > > + ret = spinand_read_reg_op(spinand, REG_CFG, > > + &feature); > > + if (ret) > > + return ret; > > + > > + /* CFG_OTP_ENABLE is used to enable parameter page access */ > > + feature |= CFG_OTP_ENABLE; > > + > > + spinand_write_reg_op(spinand, REG_CFG, feature); > > + > > + ret = spi_mem_exec_op(spinand->spimem, &pread_op); > > + if (ret) > > + return ret; > > + > > + ret = spinand_wait(spinand, &status); > > + if (ret < 0) > > + return ret; > > + > > + ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op); > > + if (ret) > > + return ret; > > + > > + ret = spinand_read_reg_op(spinand, REG_CFG, > > + &feature); > > + if (ret) > > + return ret; > > + > > + feature &= ~CFG_OTP_ENABLE; > > + > > + spinand_write_reg_op(spinand, REG_CFG, feature); > > + > > + return 0; > > +} > > + > Add the kernel doc please > > Change the below function so that it returns 1 if the page was > detected, 0 if it did not, an negative error code otherwise. > > > +static int spinand_param_page_detect(struct spinand_device *spinand) > > +{ > > + struct mtd_info *mtd = spinand_to_mtd(spinand); > > + struct nand_memory_organization *memorg; > > + struct nand_onfi_params *p; > > + struct nand_device *base = spinand_to_nand(spinand); > > + int i, ret; > > + > > + memorg = nanddev_get_memorg(base); > > + > > + /* Allocate buffer to hold parameter page */ > > + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL); > > + if (!p) > > + return -ENOMEM; > > + > > + ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * > 3); > > + if (ret) { > > + ret = 0; > > No, you should return the error in case of error. You will later handle > the fact that there is no parameter page. okay. > > > + goto free_param_page; > > + } > > + > > + for (i = 0; i < 3; i++) { > > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) == > ^ > If you force the parameter page to be 254 bytes long it means you limit > yourself to ONFI standard. That's not a problem, but then you should > mention it in the function name. okay, I will mention in kernel doc. > > > + le16_to_cpu(p->crc)) { > > + if (i) > > + memcpy(p, &p[i], sizeof(*p)); > > + break; > > + } > > + } > > + > > + if (i == 3) { > > + const void *srcbufs[3] = {p, p + 1, p + 2}; > > + > > + pr_warn("Could not find a valid ONFI parameter page, trying > bit-wise majority to recover it\n"); > > + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p, > > + sizeof(*p)); > > + > > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) != > > + le16_to_cpu(p->crc)) { > > + pr_err("ONFI parameter recovery failed, > aborting\n"); > > + goto free_param_page; > > + } > > + } > > The whole for-loop and the if (i==3) condition is exactly the same as > for raw NANDs and must be extracted in a generic function: > 1/ extract the function from nand/raw/nand_onfi.c and put it in > nand/onfi.c. > 2/ then use it in this patch. I have done this intentionally, because in raw NAND case there is function "nand_read_data_op" called inside for-loop. I don't think just for if (i == 3) it is necessary to create new function. Let me know if you have different opinion. > > > + > > + parse_onfi_params(memorg, p); > > + > > + mtd->writesize = memorg->pagesize; > > + mtd->erasesize = memorg->pages_per_eraseblock * memorg- > >pagesize; > > + mtd->oobsize = memorg->oobsize; > > This will be handled by nanddev_init, should be removed. > > > + > > + /* Manufacturers may interpret the parameter page differently */ > > + if (spinand->manufacturer->ops->fixup_param_page) > > + spinand->manufacturer->ops->fixup_param_page(spinand, > p); > > The whole "manufacturer fixup" should be done separately. > > > + > > + /* Identification done, free the full parameter page and exit */ > > + ret = 1; > > + > > +free_param_page: > > + kfree(p); > > + > > + return ret; > > +} > > + > > static int spinand_check_ecc_status(struct spinand_device *spinand, u8 > status) > > { > > struct nand_device *nand = spinand_to_nand(spinand); > > @@ -911,6 +1036,15 @@ static int spinand_detect(struct spinand_device > *spinand) > > return ret; > > } > > > > + if (!spinand->base.memorg.pagesize) { > > + ret = spinand_param_page_detect(spinand); > > + if (ret <= 0) { > > + dev_err(dev, "no parameter page for %*phN\n", > > Not sure at this stage dev will give something meaningful. Anyway I > don't think we should scream at the user if his NAND is not an ONFI one > so please return an error only if ret < 0. If ret == 0 or ret == 1, > don't warn the user. I will do it as per your suggestion. > > > + SPINAND_MAX_ID_LEN, spinand->id.data); > > + return -ENODEV; > > + } > > + } > > + > > if (nand->memorg.ntargets > 1 && !spinand->select_target) { > > dev_err(dev, > > "SPI NANDs with more than one die must implement > ->select_target()\n"); > > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > > index 4ea558bd3c46..fea820a20bc9 100644 > > --- a/include/linux/mtd/spinand.h > > +++ b/include/linux/mtd/spinand.h > > @@ -15,6 +15,7 @@ > > #include <linux/mtd/nand.h> > > #include <linux/spi/spi.h> > > #include <linux/spi/spi-mem.h> > > +#include <linux/mtd/onfi.h> > > > > /** > > * Standard SPI NAND flash operations > > @@ -209,6 +210,8 @@ struct spinand_manufacturer_ops { > > int (*detect)(struct spinand_device *spinand); > > int (*init)(struct spinand_device *spinand); > > void (*cleanup)(struct spinand_device *spinand); > > + void (*fixup_param_page)(struct spinand_device *spinand, > > + struct nand_onfi_params *p); > > Please do this in a separate patch. > > > }; > > > > /** > > Thanks, > Miquèl
Hi Boris, Another question for you :) "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on Mon, 19 Aug 2019 08:51:52 +0000: > Hi Miquel, > > > > > Hi Shiva, > > > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200: > > > > "mtd: spinand: enable parameter page support" > > > > > From: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > > > Some of the SPI NAND devices has parameter page, which is similar to > > - have a > > > ONFI table. > > regular raw NAND ONFI tables. > > > > > > > > But, it may not be self sufficient to propagate all the required > > As it may not be > > > parameters. Fixup function has been added in struct manufacturer to > > , a fixup is being added in the manufacturer structure > > > accommodate this. > > > > The fixup function sentence should be dropped from the commit message, > > see below. > > Okay, I will create separate patch for fixup function. > > > > > > > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > > > --- > > > drivers/mtd/nand/spi/core.c | 134 > > ++++++++++++++++++++++++++++++++++++ > > > include/linux/mtd/spinand.h | 3 + > > > 2 files changed, 137 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > > index 89f6beefb01c..7ae76dab9141 100644 > > > --- a/drivers/mtd/nand/spi/core.c > > > +++ b/drivers/mtd/nand/spi/core.c > > > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct > > spinand_device *spinand, u8 lock) > > > return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock); > > > } > > > > > > +/** > > > + * spinand_read_param_page_op - Read parameter page operation > > > > Again, the name in the doc does not fit the function you describe > > > > > + * @spinand: the spinand > > SPI-NAND chip > > > > Shiva, there are way too much typos and shortcuts in your series. > > Please be more careful otherwise we can't focus on the technical > > aspects. I am not a native English speaker at all but please, plain > > English is not C code. We talk SPI-NAND and not spinand, we say > > structure and not struct, acronyms are uppercase, etc. > > > > Sorry for the inconvenience caused, I will take care from next time. > > > > + * @page: page number where parameter page tables can be found > > ^ the > > > + * @buf: buffer used to store the parameter page > > > + * @len: length of the buffer > > > + * > > > + * Read parameter page > > the > > > + * > > > + * Returns 0 on success, a negative error code otherwise. > > > + */ > > > +static int spinand_parameter_page_read(struct spinand_device *spinand, > > > + u8 page, void *buf, unsigned int len) > > > +{ > > > + struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page); > > > + struct spi_mem_op pread_cache_op = > > > + > > SPINAND_PAGE_READ_FROM_CACHE_OP(false, > > > + 0, > > > + 1, > > > + buf, > > > + len); > > > > That's ok if you cross the 80 characters boundary here. You may put "0, > > 1," on the first line and "buf, len);" on the second. > > > > > + u8 feature; > > > + u8 status; > > > + int ret; > > > + > > > + if (len && !buf) > > > + return -EINVAL; > > > + > > > + ret = spinand_read_reg_op(spinand, REG_CFG, > > > + &feature); > > > + if (ret) > > > + return ret; > > > + > > > + /* CFG_OTP_ENABLE is used to enable parameter page access */ > > > + feature |= CFG_OTP_ENABLE; > > > + > > > + spinand_write_reg_op(spinand, REG_CFG, feature); > > > + > > > + ret = spi_mem_exec_op(spinand->spimem, &pread_op); > > > + if (ret) > > > + return ret; > > > + > > > + ret = spinand_wait(spinand, &status); > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op); > > > + if (ret) > > > + return ret; > > > + > > > + ret = spinand_read_reg_op(spinand, REG_CFG, > > > + &feature); > > > + if (ret) > > > + return ret; > > > + > > > + feature &= ~CFG_OTP_ENABLE; > > > + > > > + spinand_write_reg_op(spinand, REG_CFG, feature); > > > + > > > + return 0; > > > +} > > > + > > Add the kernel doc please > > > > Change the below function so that it returns 1 if the page was > > detected, 0 if it did not, an negative error code otherwise. > > > > > +static int spinand_param_page_detect(struct spinand_device *spinand) > > > +{ > > > + struct mtd_info *mtd = spinand_to_mtd(spinand); > > > + struct nand_memory_organization *memorg; > > > + struct nand_onfi_params *p; > > > + struct nand_device *base = spinand_to_nand(spinand); > > > + int i, ret; > > > + > > > + memorg = nanddev_get_memorg(base); > > > + > > > + /* Allocate buffer to hold parameter page */ > > > + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL); > > > + if (!p) > > > + return -ENOMEM; > > > + > > > + ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * > > 3); > > > + if (ret) { > > > + ret = 0; > > > > No, you should return the error in case of error. You will later handle > > the fact that there is no parameter page. > > okay. > > > > > > + goto free_param_page; > > > + } > > > + > > > + for (i = 0; i < 3; i++) { > > > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) == > > ^ > > If you force the parameter page to be 254 bytes long it means you limit > > yourself to ONFI standard. That's not a problem, but then you should > > mention it in the function name. > > okay, I will mention in kernel doc. > > > > > > + le16_to_cpu(p->crc)) { > > > + if (i) > > > + memcpy(p, &p[i], sizeof(*p)); > > > + break; > > > + } > > > + } > > > + > > > + if (i == 3) { > > > + const void *srcbufs[3] = {p, p + 1, p + 2}; > > > + > > > + pr_warn("Could not find a valid ONFI parameter page, trying > > bit-wise majority to recover it\n"); > > > + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p, > > > + sizeof(*p)); > > > + > > > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) != > > > + le16_to_cpu(p->crc)) { > > > + pr_err("ONFI parameter recovery failed, > > aborting\n"); > > > + goto free_param_page; > > > + } > > > + } > > > > The whole for-loop and the if (i==3) condition is exactly the same as > > for raw NANDs and must be extracted in a generic function: > > 1/ extract the function from nand/raw/nand_onfi.c and put it in > > nand/onfi.c. > > 2/ then use it in this patch. > > I have done this intentionally, because in raw NAND case there is function > "nand_read_data_op" called inside for-loop. I don't think just for if (i == 3) > it is necessary to create new function. > > Let me know if you have different opinion. I don't have a strong opinion on that. Boris what do you think? Shall we duplicate the code? It's not just about the if condition, it's the whole for loop which is very similar. Thanks, Miquèl
Hello Miquel & Boris, Just a gentle reminder that I'd like some feedback. Thanks, Shiva > > Hi Boris, > > Another question for you :) > > "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote > on > Mon, 19 Aug 2019 08:51:52 +0000: > > > Hi Miquel, > > > > > > > > Hi Shiva, > > > > > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200: > > > > > > "mtd: spinand: enable parameter page support" > > > > > > > From: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > > > > > Some of the SPI NAND devices has parameter page, which is similar to > > > - have a > > > > ONFI table. > > > regular raw NAND ONFI tables. > > > > > > > > > > > But, it may not be self sufficient to propagate all the required > > > As it may not be > > > > parameters. Fixup function has been added in struct manufacturer to > > > , a fixup is being added in the manufacturer structure > > > > accommodate this. > > > > > > The fixup function sentence should be dropped from the commit > message, > > > see below. > > > > Okay, I will create separate patch for fixup function. > > > > > > > > > > > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > --- > > > > drivers/mtd/nand/spi/core.c | 134 > > > ++++++++++++++++++++++++++++++++++++ > > > > include/linux/mtd/spinand.h | 3 + > > > > 2 files changed, 137 insertions(+) > > > > > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > > > index 89f6beefb01c..7ae76dab9141 100644 > > > > --- a/drivers/mtd/nand/spi/core.c > > > > +++ b/drivers/mtd/nand/spi/core.c > > > > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct > > > spinand_device *spinand, u8 lock) > > > > return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock); > > > > } > > > > > > > > +/** > > > > + * spinand_read_param_page_op - Read parameter page operation > > > > > > Again, the name in the doc does not fit the function you describe > > > > > > > + * @spinand: the spinand > > > SPI-NAND chip > > > > > > Shiva, there are way too much typos and shortcuts in your series. > > > Please be more careful otherwise we can't focus on the technical > > > aspects. I am not a native English speaker at all but please, plain > > > English is not C code. We talk SPI-NAND and not spinand, we say > > > structure and not struct, acronyms are uppercase, etc. > > > > > > > Sorry for the inconvenience caused, I will take care from next time. > > > > > > + * @page: page number where parameter page tables can be found > > > ^ the > > > > + * @buf: buffer used to store the parameter page > > > > + * @len: length of the buffer > > > > + * > > > > + * Read parameter page > > > the > > > > + * > > > > + * Returns 0 on success, a negative error code otherwise. > > > > + */ > > > > +static int spinand_parameter_page_read(struct spinand_device > *spinand, > > > > + u8 page, void *buf, unsigned int len) > > > > +{ > > > > + struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page); > > > > + struct spi_mem_op pread_cache_op = > > > > + > > > SPINAND_PAGE_READ_FROM_CACHE_OP(false, > > > > + 0, > > > > + 1, > > > > + buf, > > > > + len); > > > > > > That's ok if you cross the 80 characters boundary here. You may put "0, > > > 1," on the first line and "buf, len);" on the second. > > > > > > > + u8 feature; > > > > + u8 status; > > > > + int ret; > > > > + > > > > + if (len && !buf) > > > > + return -EINVAL; > > > > + > > > > + ret = spinand_read_reg_op(spinand, REG_CFG, > > > > + &feature); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* CFG_OTP_ENABLE is used to enable parameter page access */ > > > > + feature |= CFG_OTP_ENABLE; > > > > + > > > > + spinand_write_reg_op(spinand, REG_CFG, feature); > > > > + > > > > + ret = spi_mem_exec_op(spinand->spimem, &pread_op); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = spinand_wait(spinand, &status); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = spinand_read_reg_op(spinand, REG_CFG, > > > > + &feature); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + feature &= ~CFG_OTP_ENABLE; > > > > + > > > > + spinand_write_reg_op(spinand, REG_CFG, feature); > > > > + > > > > + return 0; > > > > +} > > > > + > > > Add the kernel doc please > > > > > > Change the below function so that it returns 1 if the page was > > > detected, 0 if it did not, an negative error code otherwise. > > > > > > > +static int spinand_param_page_detect(struct spinand_device > *spinand) > > > > +{ > > > > + struct mtd_info *mtd = spinand_to_mtd(spinand); > > > > + struct nand_memory_organization *memorg; > > > > + struct nand_onfi_params *p; > > > > + struct nand_device *base = spinand_to_nand(spinand); > > > > + int i, ret; > > > > + > > > > + memorg = nanddev_get_memorg(base); > > > > + > > > > + /* Allocate buffer to hold parameter page */ > > > > + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL); > > > > + if (!p) > > > > + return -ENOMEM; > > > > + > > > > + ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * > > > 3); > > > > + if (ret) { > > > > + ret = 0; > > > > > > No, you should return the error in case of error. You will later handle > > > the fact that there is no parameter page. > > > > okay. > > > > > > > > > + goto free_param_page; > > > > + } > > > > + > > > > + for (i = 0; i < 3; i++) { > > > > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) == > > > ^ > > > If you force the parameter page to be 254 bytes long it means you limit > > > yourself to ONFI standard. That's not a problem, but then you should > > > mention it in the function name. > > > > okay, I will mention in kernel doc. > > > > > > > > > + le16_to_cpu(p->crc)) { > > > > + if (i) > > > > + memcpy(p, &p[i], sizeof(*p)); > > > > + break; > > > > + } > > > > + } > > > > + > > > > + if (i == 3) { > > > > + const void *srcbufs[3] = {p, p + 1, p + 2}; > > > > + > > > > + pr_warn("Could not find a valid ONFI parameter page, trying > > > bit-wise majority to recover it\n"); > > > > + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p, > > > > + sizeof(*p)); > > > > + > > > > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) != > > > > + le16_to_cpu(p->crc)) { > > > > + pr_err("ONFI parameter recovery failed, > > > aborting\n"); > > > > + goto free_param_page; > > > > + } > > > > + } > > > > > > The whole for-loop and the if (i==3) condition is exactly the same as > > > for raw NANDs and must be extracted in a generic function: > > > 1/ extract the function from nand/raw/nand_onfi.c and put it in > > > nand/onfi.c. > > > 2/ then use it in this patch. > > > > I have done this intentionally, because in raw NAND case there is function > > "nand_read_data_op" called inside for-loop. I don't think just for if (i == 3) > > it is necessary to create new function. > > > > Let me know if you have different opinion. > > I don't have a strong opinion on that. Boris what do you think? Shall > we duplicate the code? It's not just about the if condition, it's the > whole for loop which is very similar. > > > Thanks, > Miquèl
On Mon, 19 Aug 2019 11:21:18 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > Another question for you :) > > "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@micron.com> wrote on > Mon, 19 Aug 2019 08:51:52 +0000: > > > Hi Miquel, > > > > > > > > Hi Shiva, > > > > > > shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200: > > > > > > "mtd: spinand: enable parameter page support" > > > > > > > From: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > > > > > Some of the SPI NAND devices has parameter page, which is similar to > > > - have a > > > > ONFI table. > > > regular raw NAND ONFI tables. > > > > > > > > > > > But, it may not be self sufficient to propagate all the required > > > As it may not be > > > > parameters. Fixup function has been added in struct manufacturer to > > > , a fixup is being added in the manufacturer structure > > > > accommodate this. > > > > > > The fixup function sentence should be dropped from the commit message, > > > see below. > > > > Okay, I will create separate patch for fixup function. > > > > > > > > > > > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com> > > > > --- > > > > drivers/mtd/nand/spi/core.c | 134 > > > ++++++++++++++++++++++++++++++++++++ > > > > include/linux/mtd/spinand.h | 3 + > > > > 2 files changed, 137 insertions(+) > > > > > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > > > index 89f6beefb01c..7ae76dab9141 100644 > > > > --- a/drivers/mtd/nand/spi/core.c > > > > +++ b/drivers/mtd/nand/spi/core.c > > > > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct > > > spinand_device *spinand, u8 lock) > > > > return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock); > > > > } > > > > > > > > +/** > > > > + * spinand_read_param_page_op - Read parameter page operation > > > > > > Again, the name in the doc does not fit the function you describe > > > > > > > + * @spinand: the spinand > > > SPI-NAND chip > > > > > > Shiva, there are way too much typos and shortcuts in your series. > > > Please be more careful otherwise we can't focus on the technical > > > aspects. I am not a native English speaker at all but please, plain > > > English is not C code. We talk SPI-NAND and not spinand, we say > > > structure and not struct, acronyms are uppercase, etc. > > > > > > > Sorry for the inconvenience caused, I will take care from next time. > > > > > > + * @page: page number where parameter page tables can be found > > > ^ the > > > > + * @buf: buffer used to store the parameter page > > > > + * @len: length of the buffer > > > > + * > > > > + * Read parameter page > > > the > > > > + * > > > > + * Returns 0 on success, a negative error code otherwise. > > > > + */ > > > > +static int spinand_parameter_page_read(struct spinand_device *spinand, > > > > + u8 page, void *buf, unsigned int len) > > > > +{ > > > > + struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page); > > > > + struct spi_mem_op pread_cache_op = > > > > + > > > SPINAND_PAGE_READ_FROM_CACHE_OP(false, > > > > + 0, > > > > + 1, > > > > + buf, > > > > + len); > > > > > > That's ok if you cross the 80 characters boundary here. You may put "0, > > > 1," on the first line and "buf, len);" on the second. > > > > > > > + u8 feature; > > > > + u8 status; > > > > + int ret; > > > > + > > > > + if (len && !buf) > > > > + return -EINVAL; > > > > + > > > > + ret = spinand_read_reg_op(spinand, REG_CFG, > > > > + &feature); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* CFG_OTP_ENABLE is used to enable parameter page access */ > > > > + feature |= CFG_OTP_ENABLE; > > > > + > > > > + spinand_write_reg_op(spinand, REG_CFG, feature); > > > > + > > > > + ret = spi_mem_exec_op(spinand->spimem, &pread_op); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = spinand_wait(spinand, &status); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = spinand_read_reg_op(spinand, REG_CFG, > > > > + &feature); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + feature &= ~CFG_OTP_ENABLE; > > > > + > > > > + spinand_write_reg_op(spinand, REG_CFG, feature); > > > > + > > > > + return 0; > > > > +} > > > > + > > > Add the kernel doc please > > > > > > Change the below function so that it returns 1 if the page was > > > detected, 0 if it did not, an negative error code otherwise. > > > > > > > +static int spinand_param_page_detect(struct spinand_device *spinand) > > > > +{ > > > > + struct mtd_info *mtd = spinand_to_mtd(spinand); > > > > + struct nand_memory_organization *memorg; > > > > + struct nand_onfi_params *p; > > > > + struct nand_device *base = spinand_to_nand(spinand); > > > > + int i, ret; > > > > + > > > > + memorg = nanddev_get_memorg(base); > > > > + > > > > + /* Allocate buffer to hold parameter page */ > > > > + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL); > > > > + if (!p) > > > > + return -ENOMEM; > > > > + > > > > + ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * > > > 3); > > > > + if (ret) { > > > > + ret = 0; > > > > > > No, you should return the error in case of error. You will later handle > > > the fact that there is no parameter page. > > > > okay. > > > > > > > > > + goto free_param_page; > > > > + } > > > > + > > > > + for (i = 0; i < 3; i++) { > > > > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) == > > > ^ > > > If you force the parameter page to be 254 bytes long it means you limit > > > yourself to ONFI standard. That's not a problem, but then you should > > > mention it in the function name. > > > > okay, I will mention in kernel doc. > > > > > > > > > + le16_to_cpu(p->crc)) { > > > > + if (i) > > > > + memcpy(p, &p[i], sizeof(*p)); > > > > + break; > > > > + } > > > > + } > > > > + > > > > + if (i == 3) { > > > > + const void *srcbufs[3] = {p, p + 1, p + 2}; > > > > + > > > > + pr_warn("Could not find a valid ONFI parameter page, trying > > > bit-wise majority to recover it\n"); > > > > + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p, > > > > + sizeof(*p)); > > > > + > > > > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) != > > > > + le16_to_cpu(p->crc)) { > > > > + pr_err("ONFI parameter recovery failed, > > > aborting\n"); > > > > + goto free_param_page; > > > > + } > > > > + } > > > > > > The whole for-loop and the if (i==3) condition is exactly the same as > > > for raw NANDs and must be extracted in a generic function: > > > 1/ extract the function from nand/raw/nand_onfi.c and put it in > > > nand/onfi.c. > > > 2/ then use it in this patch. Sounds like a good idea indeed. > > > > I have done this intentionally, because in raw NAND case there is function > > "nand_read_data_op" called inside for-loop. I don't think just for if (i == 3) > > it is necessary to create new function. Well, reading more than just a single param page in a single step shouldn't hurt. You can let the caller choose the number of page it wants to read (can be more than 3) and pass this information to the helper function. You should probably call nand_bit_wise_majority() from this helper too. > > > > Let me know if you have different opinion. > > I don't have a strong opinion on that. Boris what do you think? Shall > we duplicate the code? It's not just about the if condition, it's the > whole for loop which is very similar. I think Miquel is right, we shouldn't duplicate this logic.
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index 89f6beefb01c..7ae76dab9141 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -400,6 +400,131 @@ static int spinand_lock_block(struct spinand_device *spinand, u8 lock) return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock); } +/** + * spinand_read_param_page_op - Read parameter page operation + * @spinand: the spinand + * @page: page number where parameter page tables can be found + * @buf: buffer used to store the parameter page + * @len: length of the buffer + * + * Read parameter page + * + * Returns 0 on success, a negative error code otherwise. + */ +static int spinand_parameter_page_read(struct spinand_device *spinand, + u8 page, void *buf, unsigned int len) +{ + struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page); + struct spi_mem_op pread_cache_op = + SPINAND_PAGE_READ_FROM_CACHE_OP(false, + 0, + 1, + buf, + len); + u8 feature; + u8 status; + int ret; + + if (len && !buf) + return -EINVAL; + + ret = spinand_read_reg_op(spinand, REG_CFG, + &feature); + if (ret) + return ret; + + /* CFG_OTP_ENABLE is used to enable parameter page access */ + feature |= CFG_OTP_ENABLE; + + spinand_write_reg_op(spinand, REG_CFG, feature); + + ret = spi_mem_exec_op(spinand->spimem, &pread_op); + if (ret) + return ret; + + ret = spinand_wait(spinand, &status); + if (ret < 0) + return ret; + + ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op); + if (ret) + return ret; + + ret = spinand_read_reg_op(spinand, REG_CFG, + &feature); + if (ret) + return ret; + + feature &= ~CFG_OTP_ENABLE; + + spinand_write_reg_op(spinand, REG_CFG, feature); + + return 0; +} + +static int spinand_param_page_detect(struct spinand_device *spinand) +{ + struct mtd_info *mtd = spinand_to_mtd(spinand); + struct nand_memory_organization *memorg; + struct nand_onfi_params *p; + struct nand_device *base = spinand_to_nand(spinand); + int i, ret; + + memorg = nanddev_get_memorg(base); + + /* Allocate buffer to hold parameter page */ + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL); + if (!p) + return -ENOMEM; + + ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * 3); + if (ret) { + ret = 0; + goto free_param_page; + } + + for (i = 0; i < 3; i++) { + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) == + le16_to_cpu(p->crc)) { + if (i) + memcpy(p, &p[i], sizeof(*p)); + break; + } + } + + if (i == 3) { + const void *srcbufs[3] = {p, p + 1, p + 2}; + + pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n"); + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p, + sizeof(*p)); + + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) != + le16_to_cpu(p->crc)) { + pr_err("ONFI parameter recovery failed, aborting\n"); + goto free_param_page; + } + } + + parse_onfi_params(memorg, p); + + mtd->writesize = memorg->pagesize; + mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize; + mtd->oobsize = memorg->oobsize; + + /* Manufacturers may interpret the parameter page differently */ + if (spinand->manufacturer->ops->fixup_param_page) + spinand->manufacturer->ops->fixup_param_page(spinand, p); + + /* Identification done, free the full parameter page and exit */ + ret = 1; + +free_param_page: + kfree(p); + + return ret; +} + static int spinand_check_ecc_status(struct spinand_device *spinand, u8 status) { struct nand_device *nand = spinand_to_nand(spinand); @@ -911,6 +1036,15 @@ static int spinand_detect(struct spinand_device *spinand) return ret; } + if (!spinand->base.memorg.pagesize) { + ret = spinand_param_page_detect(spinand); + if (ret <= 0) { + dev_err(dev, "no parameter page for %*phN\n", + SPINAND_MAX_ID_LEN, spinand->id.data); + return -ENODEV; + } + } + if (nand->memorg.ntargets > 1 && !spinand->select_target) { dev_err(dev, "SPI NANDs with more than one die must implement ->select_target()\n"); diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h index 4ea558bd3c46..fea820a20bc9 100644 --- a/include/linux/mtd/spinand.h +++ b/include/linux/mtd/spinand.h @@ -15,6 +15,7 @@ #include <linux/mtd/nand.h> #include <linux/spi/spi.h> #include <linux/spi/spi-mem.h> +#include <linux/mtd/onfi.h> /** * Standard SPI NAND flash operations @@ -209,6 +210,8 @@ struct spinand_manufacturer_ops { int (*detect)(struct spinand_device *spinand); int (*init)(struct spinand_device *spinand); void (*cleanup)(struct spinand_device *spinand); + void (*fixup_param_page)(struct spinand_device *spinand, + struct nand_onfi_params *p); }; /**