| Message ID | 1480235616-34038-1-git-send-email-nagasure@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Naga, I have not finished to review the whole series yet but here some first comments: Le 27/11/2016 à 09:33, Naga Sureshkumar Relli a écrit : > This patch adds stripe support and it is needed for GQSPI parallel > configuration mode by: > > - Adding required parameters like stripe and shift to spi_nor > structure. > - Initializing all added parameters in spi_nor_scan() > - Updating read_sr() and read_fsr() for getting status from both > flashes > - Increasing page_size, sector_size, erase_size and toatal flash > size as and when required. > - Dividing address by 2 > - Updating spi->master->flags for qspi driver to change CS > > Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> > --- > Changes for v4: > - rename isparallel to stripe > Changes for v3: > - No change > Changes for v2: > - Splitted to separate MTD layer changes from SPI core changes > --- > drivers/mtd/spi-nor/spi-nor.c | 130 ++++++++++++++++++++++++++++++++---------- > include/linux/mtd/spi-nor.h | 2 + > 2 files changed, 103 insertions(+), 29 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index d0fc165..4252239 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -22,6 +22,7 @@ > #include <linux/of_platform.h> > #include <linux/spi/flash.h> > #include <linux/mtd/spi-nor.h> > +#include <linux/spi/spi.h> > > /* Define max times to check status register before we give up. */ > > @@ -89,15 +90,24 @@ static const struct flash_info *spi_nor_match_id(const char *name); > static int read_sr(struct spi_nor *nor) > { > int ret; > - u8 val; > + u8 val[2]; > > - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); > - if (ret < 0) { > - pr_err("error %d reading SR\n", (int) ret); > - return ret; > + if (nor->stripe) { > + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); > + if (ret < 0) { > + pr_err("error %d reading SR\n", (int) ret); > + return ret; > + } > + val[0] |= val[1]; Why '|' rather than '&' ? I guess because of the 'Write In Progress/Busy' bit: when called by spi_nor_sr_ready(), you want to be sure that this 'BUSY' bit is cleared on both memories. But what about when the Status Register is read for purpose other than checking the state of the 'BUSY' bit? What about SPI controllers supporting more than 2 memories in parallel? This solution might fit the ZynqMP controller but doesn't look so generic. > + } else { > + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); > + if (ret < 0) { > + pr_err("error %d reading SR\n", (int) ret); > + return ret; > + } > } > > - return val; > + return val[0]; > } > > /* > @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) > static int read_fsr(struct spi_nor *nor) > { > int ret; > - u8 val; > + u8 val[2]; > > - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); > - if (ret < 0) { > - pr_err("error %d reading FSR\n", ret); > - return ret; > + if (nor->stripe) { > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); > + if (ret < 0) { > + pr_err("error %d reading FSR\n", ret); > + return ret; > + } > + val[0] &= val[1]; Same comment here: why '&' rather than '|'? Surely because of the the 'READY' bit which should be set for both memories. > + } else { > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); > + if (ret < 0) { > + pr_err("error %d reading FSR\n", ret); > + return ret; > + } > } > > - return val; > + return val[0]; > } > > /* > @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct spi_nor *nor) > */ > static int erase_chip(struct spi_nor *nor) > { > + u32 ret; > + > dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> 10)); > > - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); > + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); > + if (ret) > + return ret; > + > + return ret; > + if (ret) return ret; else return ret; This chunk should be removed, it doesn't ease the patch review ;) > } > > static int spi_nor_lock_and_prep(struct spi_nor *nor, enum spi_nor_ops ops) > @@ -349,7 +375,7 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) > static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) > { > struct spi_nor *nor = mtd_to_spi_nor(mtd); > - u32 addr, len; > + u32 addr, len, offset; > uint32_t rem; > int ret; > > @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) > /* "sector"-at-a-time erase */ > } else { > while (len) { > + > write_enable(nor); > + offset = addr; > + if (nor->stripe) > + offset /= 2; I guess this should be /= 4 for controllers supporting 4 memories in parallel. Shouldn't you use nor->shift and define shift as an unsigned int instead of a bool? offset >>= nor->shift; Anyway, by tuning the address here in spi-nor.c rather than in the SPI controller driver, you impose a model to support parallel memories that might not be suited to other controllers. > > - ret = spi_nor_erase_sector(nor, addr); > + ret = spi_nor_erase_sector(nor, offset); > if (ret) > goto erase_err; > > @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) > bool use_top; > int ret; > > + ofs = ofs >> nor->shift; > + > status_old = read_sr(nor); > if (status_old < 0) > return status_old; > @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) > bool use_top; > int ret; > > + ofs = ofs >> nor->shift; > + > status_old = read_sr(nor); > if (status_old < 0) > return status_old; > @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > if (ret) > return ret; > > + ofs = ofs >> nor->shift; > + > ret = nor->flash_lock(nor, ofs, len); > > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); > @@ -724,6 +760,8 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > if (ret) > return ret; > > + ofs = ofs >> nor->shift; > + > ret = nor->flash_unlock(nor, ofs, len); > > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); > @@ -1018,6 +1056,9 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > u8 id[SPI_NOR_MAX_ID_LEN]; > const struct flash_info *info; > > + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | > + SPI_MASTER_DATA_STRIPE); > + > tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN); > if (tmp < 0) { > dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); > @@ -1041,6 +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, > { > struct spi_nor *nor = mtd_to_spi_nor(mtd); > int ret; > + u32 offset = from; > > dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > > @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, > return ret; > > while (len) { > - ret = nor->read(nor, from, len, buf); > + > + offset = from; > + > + if (nor->stripe) > + offset /= 2; > + > + ret = nor->read(nor, offset, len, buf); > if (ret == 0) { > /* We shouldn't see 0-length reads */ > ret = -EIO; > @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, > struct spi_nor *nor = mtd_to_spi_nor(mtd); > size_t page_offset, page_remain, i; > ssize_t ret; > + u32 offset; > > dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); > > @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, > /* the size of data remaining on the first page */ > page_remain = min_t(size_t, > nor->page_size - page_offset, len - i); > + offset = (to + i); > + > + if (nor->stripe) > + offset /= 2; > > write_enable(nor); > - ret = nor->write(nor, to + i, page_remain, buf + i); > + ret = nor->write(nor, (offset), page_remain, buf + i); > if (ret < 0) > goto write_err; > written = ret; > @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor *nor) > > int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > { > - const struct flash_info *info = NULL; > + struct flash_info *info = NULL; You should not remove the const and should not try to modify members of *info. > struct device *dev = nor->dev; > struct mtd_info *mtd = &nor->mtd; > struct device_node *np = spi_nor_get_flash_node(nor); > - int ret; > - int i; > + struct device_node *np_spi; > + int ret, i, xlnx_qspi_mode; > > ret = spi_nor_check(nor); > if (ret) > return ret; > > if (name) > - info = spi_nor_match_id(name); > + info = (struct flash_info *)spi_nor_match_id(name); > /* Try to auto-detect if chip name wasn't specified or not found */ > if (!info) > - info = spi_nor_read_id(nor); > + info = (struct flash_info *)spi_nor_read_id(nor); > if (IS_ERR_OR_NULL(info)) > return -ENOENT; > Both spi_nor_match_id() and spi_nor_read_id(), when they succeed, return a pointer to an entry of the spi_nor_ids[] array, which is located in a read-only memory area. Since your patch doesn't remove the const attribute of the spi_nor_ids[], I wonder whether it has been tested. I expect it not to work on most architecture. Anyway spi_nor_ids[] should remain const. Let's take the case of eXecution In Place (XIP) from an external memory: if spi_nor_ids[] is const, it can be read directly from this external (read-only) memory and we never need to copy the array in RAM, so we save some KB of RAM. This is just an example but I guess we can find other reasons to keep this array const. > @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > */ > dev_warn(dev, "found %s, expected %s\n", > jinfo->name, info->name); > - info = jinfo; > + info = (struct flash_info *)jinfo; > } > } > > @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > mtd->size = info->sector_size * info->n_sectors; > mtd->_erase = spi_nor_erase; > mtd->_read = spi_nor_read; > +#ifdef CONFIG_OF > + np_spi = of_get_next_parent(np); > + > + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", > + &xlnx_qspi_mode) < 0) { This really looks controller specific so should not be placed in the generic spi-nor.c file. > + nor->shift = 0; > + nor->stripe = 0; > + } else if (xlnx_qspi_mode == 2) { > + nor->shift = 1; > + info->sector_size <<= nor->shift; > + info->page_size <<= nor->shift; Just don't modify *info members! :) > + mtd->size <<= nor->shift; > + nor->stripe = 1; > + nor->spi->master->flags |= (SPI_MASTER_BOTH_CS | > + SPI_MASTER_DATA_STRIPE); > + } > +#else > + /* Default to single */ > + nor->shift = 0; > + nor->stripe = 0; > +#endif > > /* NOR protection support for STmicro/Micron chips and similar */ > if (JEDEC_MFR(info) == SNOR_MFR_MICRON || > @@ -1400,10 +1474,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > /* prefer "small sector" erase if possible */ > if (info->flags & SECT_4K) { > nor->erase_opcode = SPINOR_OP_BE_4K; > - mtd->erasesize = 4096; > + mtd->erasesize = 4096 << nor->shift; > } else if (info->flags & SECT_4K_PMC) { > nor->erase_opcode = SPINOR_OP_BE_4K_PMC; > - mtd->erasesize = 4096; > + mtd->erasesize = 4096 << nor->shift; > } else > #endif > { > @@ -1508,16 +1582,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > dev_info(dev, "%s (%lld Kbytes)\n", info->name, > (long long)mtd->size >> 10); > > - dev_dbg(dev, > - "mtd .name = %s, .size = 0x%llx (%lldMiB), " > + dev_dbg(dev, "mtd .name = %s, .size = 0x%llx (%lldMiB), " > ".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n", > mtd->name, (long long)mtd->size, (long long)(mtd->size >> 20), > mtd->erasesize, mtd->erasesize / 1024, mtd->numeraseregions); > > if (mtd->numeraseregions) > for (i = 0; i < mtd->numeraseregions; i++) > - dev_dbg(dev, > - "mtd.eraseregions[%d] = { .offset = 0x%llx, " > + dev_dbg(dev, "mtd.eraseregions[%d] = { .offset = 0x%llx, " > ".erasesize = 0x%.8x (%uKiB), " > ".numblocks = %d }\n", > i, (long long)mtd->eraseregions[i].offset, > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index 84f3ce5..673ec68 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -165,6 +165,8 @@ struct spi_nor { > u8 read_dummy; > u8 program_opcode; > enum read_mode flash_read; > + bool shift; > + bool stripe; > bool sst_write_second; > u32 flags; > u8 cmd_buf[SPI_NOR_MAX_CMD_SIZE]; > Best regards, Cyrille
Hi Cyrille, > I have not finished to review the whole series yet but here some first > comments: Thanks for reviewing these patch series. > > Le 27/11/2016 à 09:33, Naga Sureshkumar Relli a écrit : > > This patch adds stripe support and it is needed for GQSPI parallel > > configuration mode by: > > > > - Adding required parameters like stripe and shift to spi_nor > > structure. > > - Initializing all added parameters in spi_nor_scan() > > - Updating read_sr() and read_fsr() for getting status from both > > flashes > > - Increasing page_size, sector_size, erase_size and toatal flash > > size as and when required. > > - Dividing address by 2 > > - Updating spi->master->flags for qspi driver to change CS > > > > Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> > > --- > > Changes for v4: > > - rename isparallel to stripe > > Changes for v3: > > - No change > > Changes for v2: > > - Splitted to separate MTD layer changes from SPI core changes > > --- > > drivers/mtd/spi-nor/spi-nor.c | 130 > ++++++++++++++++++++++++++++++++---------- > > include/linux/mtd/spi-nor.h | 2 + > > 2 files changed, 103 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c > > b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..4252239 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -22,6 +22,7 @@ > > #include <linux/of_platform.h> > > #include <linux/spi/flash.h> > > #include <linux/mtd/spi-nor.h> > > +#include <linux/spi/spi.h> > > > > /* Define max times to check status register before we give up. */ > > > > @@ -89,15 +90,24 @@ static const struct flash_info > > *spi_nor_match_id(const char *name); static int read_sr(struct > > spi_nor *nor) { > > int ret; > > - u8 val; > > + u8 val[2]; > > > > - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); > > - if (ret < 0) { > > - pr_err("error %d reading SR\n", (int) ret); > > - return ret; > > + if (nor->stripe) { > > + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); > > + if (ret < 0) { > > + pr_err("error %d reading SR\n", (int) ret); > > + return ret; > > + } > > + val[0] |= val[1]; > Why '|' rather than '&' ? > I guess because of the 'Write In Progress/Busy' bit: when called by > spi_nor_sr_ready(), you want to be sure that this 'BUSY' bit is cleared on > both memories. > > But what about when the Status Register is read for purpose other than > checking the state of the 'BUSY' bit? > Yes you are correct, I will change this. > What about SPI controllers supporting more than 2 memories in parallel? > > This solution might fit the ZynqMP controller but doesn't look so generic. > > > + } else { > > + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); > > + if (ret < 0) { > > + pr_err("error %d reading SR\n", (int) ret); > > + return ret; > > + } > > } > > > > - return val; > > + return val[0]; > > } > > > > /* > > @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) static > > int read_fsr(struct spi_nor *nor) { > > int ret; > > - u8 val; > > + u8 val[2]; > > > > - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); > > - if (ret < 0) { > > - pr_err("error %d reading FSR\n", ret); > > - return ret; > > + if (nor->stripe) { > > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); > > + if (ret < 0) { > > + pr_err("error %d reading FSR\n", ret); > > + return ret; > > + } > > + val[0] &= val[1]; > Same comment here: why '&' rather than '|'? > Surely because of the the 'READY' bit which should be set for both memories. I will update this also. > > > + } else { > > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); > > + if (ret < 0) { > > + pr_err("error %d reading FSR\n", ret); > > + return ret; > > + } > > } > > > > - return val; > > + return val[0]; > > } > > > > /* > > @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct spi_nor > *nor) > > */ > > static int erase_chip(struct spi_nor *nor) { > > + u32 ret; > > + > > dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> 10)); > > > > - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); > > + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); > > + if (ret) > > + return ret; > > + > > + return ret; > > + > > if (ret) > return ret; > else > return ret; > > This chunk should be removed, it doesn't ease the patch review ;) Ok, I will remove. > > > } > > > > static int spi_nor_lock_and_prep(struct spi_nor *nor, enum > > spi_nor_ops ops) @@ -349,7 +375,7 @@ static int > > spi_nor_erase_sector(struct spi_nor *nor, u32 addr) static int > > spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { > > struct spi_nor *nor = mtd_to_spi_nor(mtd); > > - u32 addr, len; > > + u32 addr, len, offset; > > uint32_t rem; > > int ret; > > > > @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info *mtd, > struct erase_info *instr) > > /* "sector"-at-a-time erase */ > > } else { > > while (len) { > > + > > write_enable(nor); > > + offset = addr; > > + if (nor->stripe) > > + offset /= 2; > > I guess this should be /= 4 for controllers supporting 4 memories in parallel. > Shouldn't you use nor->shift and define shift as an unsigned int instead of a > bool? > offset >>= nor->shift; > Yes we can use this shift, I will update > Anyway, by tuning the address here in spi-nor.c rather than in the SPI > controller driver, you impose a model to support parallel memories that > might not be suited to other controllers. For this ZynqMP GQSPI controller parallel configuration, globally spi-nor should know about this stripe feature And based on that address has to change. As I mentioned in cover letter, this controller in parallel configuration will work with even addresses only. i.e. Before creating address format(m25p_addr2cmd) in mtd layer, spi-nor should change that address based on stripe option. I am updating this offset based on stripe option, and stripe option will update by reading dt property in nor_scan(). So the controller which doesn't support, then the stripe will be zero. Or Can you please suggest any other way? > > > > > - ret = spi_nor_erase_sector(nor, addr); > > + ret = spi_nor_erase_sector(nor, offset); > > if (ret) > > goto erase_err; > > > > @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, > uint64_t len) > > bool use_top; > > int ret; > > > > + ofs = ofs >> nor->shift; > > + > > status_old = read_sr(nor); > > if (status_old < 0) > > return status_old; > > @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, > uint64_t len) > > bool use_top; > > int ret; > > > > + ofs = ofs >> nor->shift; > > + > > status_old = read_sr(nor); > > if (status_old < 0) > > return status_old; > > @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t > ofs, uint64_t len) > > if (ret) > > return ret; > > > > + ofs = ofs >> nor->shift; > > + > > ret = nor->flash_lock(nor, ofs, len); > > > > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); @@ - > 724,6 +760,8 > > @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t > len) > > if (ret) > > return ret; > > > > + ofs = ofs >> nor->shift; > > + > > ret = nor->flash_unlock(nor, ofs, len); > > > > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); @@ - > 1018,6 +1056,9 > > @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > > u8 id[SPI_NOR_MAX_ID_LEN]; > > const struct flash_info *info; > > > > + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | > > + SPI_MASTER_DATA_STRIPE); > > + > > tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, > SPI_NOR_MAX_ID_LEN); > > if (tmp < 0) { > > dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); > @@ -1041,6 > > +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, > > size_t len, { > > struct spi_nor *nor = mtd_to_spi_nor(mtd); > > int ret; > > + u32 offset = from; > > > > dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > > > > @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info *mtd, > loff_t from, size_t len, > > return ret; > > > > while (len) { > > - ret = nor->read(nor, from, len, buf); > > + > > + offset = from; > > + > > + if (nor->stripe) > > + offset /= 2; > > + > > + ret = nor->read(nor, offset, len, buf); > > if (ret == 0) { > > /* We shouldn't see 0-length reads */ > > ret = -EIO; > > @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info *mtd, > loff_t to, size_t len, > > struct spi_nor *nor = mtd_to_spi_nor(mtd); > > size_t page_offset, page_remain, i; > > ssize_t ret; > > + u32 offset; > > > > dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); > > > > @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info *mtd, > loff_t to, size_t len, > > /* the size of data remaining on the first page */ > > page_remain = min_t(size_t, > > nor->page_size - page_offset, len - i); > > + offset = (to + i); > > + > > + if (nor->stripe) > > + offset /= 2; > > > > write_enable(nor); > > - ret = nor->write(nor, to + i, page_remain, buf + i); > > + ret = nor->write(nor, (offset), page_remain, buf + i); > > if (ret < 0) > > goto write_err; > > written = ret; > > @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor *nor) > > > > int spi_nor_scan(struct spi_nor *nor, const char *name, enum > > read_mode mode) { > > - const struct flash_info *info = NULL; > > + struct flash_info *info = NULL; > > You should not remove the const and should not try to modify members of > *info. > > > struct device *dev = nor->dev; > > struct mtd_info *mtd = &nor->mtd; > > struct device_node *np = spi_nor_get_flash_node(nor); > > - int ret; > > - int i; > > + struct device_node *np_spi; > > + int ret, i, xlnx_qspi_mode; > > > > ret = spi_nor_check(nor); > > if (ret) > > return ret; > > > > if (name) > > - info = spi_nor_match_id(name); > > + info = (struct flash_info *)spi_nor_match_id(name); > > /* Try to auto-detect if chip name wasn't specified or not found */ > > if (!info) > > - info = spi_nor_read_id(nor); > > + info = (struct flash_info *)spi_nor_read_id(nor); > > if (IS_ERR_OR_NULL(info)) > > return -ENOENT; > > > Both spi_nor_match_id() and spi_nor_read_id(), when they succeed, return > a pointer to an entry of the spi_nor_ids[] array, which is located in a read- > only memory area. > > Since your patch doesn't remove the const attribute of the spi_nor_ids[], I > wonder whether it has been tested. I expect it not to work on most > architecture. > > Anyway spi_nor_ids[] should remain const. Let's take the case of eXecution > In Place (XIP) from an external memory: if spi_nor_ids[] is const, it can be > read directly from this external (read-only) memory and we never need to > copy the array in RAM, so we save some KB of RAM. > This is just an example but I guess we can find other reasons to keep this > array const. > > > @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, enum read_mode mode) > > */ > > dev_warn(dev, "found %s, expected %s\n", > > jinfo->name, info->name); > > - info = jinfo; > > + info = (struct flash_info *)jinfo; > > } > > } > > > > @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, enum read_mode mode) > > mtd->size = info->sector_size * info->n_sectors; > > mtd->_erase = spi_nor_erase; > > mtd->_read = spi_nor_read; > > +#ifdef CONFIG_OF > > + np_spi = of_get_next_parent(np); > > + > > + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", > > + &xlnx_qspi_mode) < 0) { > This really looks controller specific so should not be placed in the generic spi- > nor.c file. Const is removed in info struct, because to update info members based parallel configuration. As I mentioned above, for this parallel configuration, mtd and spi-nor should know the details like mtd->size, info->sectors, sector_size and page_size. So during spi_nor_scan only mtd will update all these details, that's why I have added controller specific check to update those. Can you please suggest, any other way to let mtd and spi-nor to know about this configuration without touching the core layers? Please let me know, if I missed providing any required info. > > > + nor->shift = 0; > > + nor->stripe = 0; > > + } else if (xlnx_qspi_mode == 2) { > > + nor->shift = 1; > > + info->sector_size <<= nor->shift; > > + info->page_size <<= nor->shift; > Just don't modify *info members! :) > > > > + mtd->size <<= nor->shift; > > + nor->stripe = 1; > > + nor->spi->master->flags |= (SPI_MASTER_BOTH_CS | > > + SPI_MASTER_DATA_STRIPE); > > + } > > +#else > > + /* Default to single */ > > + nor->shift = 0; > > + nor->stripe = 0; > > +#endif > Best regards, > > Cyrille Thanks, Naga Sureshkumar Relli
Hi Naga, Le 30/11/2016 à 10:17, Naga Sureshkumar Relli a écrit : > Hi Cyrille, > >> I have not finished to review the whole series yet but here some first >> comments: > > Thanks for reviewing these patch series. > >> >> Le 27/11/2016 à 09:33, Naga Sureshkumar Relli a écrit : >>> This patch adds stripe support and it is needed for GQSPI parallel >>> configuration mode by: >>> >>> - Adding required parameters like stripe and shift to spi_nor >>> structure. >>> - Initializing all added parameters in spi_nor_scan() >>> - Updating read_sr() and read_fsr() for getting status from both >>> flashes >>> - Increasing page_size, sector_size, erase_size and toatal flash >>> size as and when required. >>> - Dividing address by 2 >>> - Updating spi->master->flags for qspi driver to change CS >>> >>> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> >>> --- >>> Changes for v4: >>> - rename isparallel to stripe >>> Changes for v3: >>> - No change >>> Changes for v2: >>> - Splitted to separate MTD layer changes from SPI core changes >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 130 >> ++++++++++++++++++++++++++++++++---------- >>> include/linux/mtd/spi-nor.h | 2 + >>> 2 files changed, 103 insertions(+), 29 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c >>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..4252239 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -22,6 +22,7 @@ >>> #include <linux/of_platform.h> >>> #include <linux/spi/flash.h> >>> #include <linux/mtd/spi-nor.h> >>> +#include <linux/spi/spi.h> >>> >>> /* Define max times to check status register before we give up. */ >>> >>> @@ -89,15 +90,24 @@ static const struct flash_info >>> *spi_nor_match_id(const char *name); static int read_sr(struct >>> spi_nor *nor) { >>> int ret; >>> - u8 val; >>> + u8 val[2]; >>> >>> - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); >>> - if (ret < 0) { >>> - pr_err("error %d reading SR\n", (int) ret); >>> - return ret; >>> + if (nor->stripe) { >>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); >>> + if (ret < 0) { >>> + pr_err("error %d reading SR\n", (int) ret); >>> + return ret; >>> + } >>> + val[0] |= val[1]; >> Why '|' rather than '&' ? >> I guess because of the 'Write In Progress/Busy' bit: when called by >> spi_nor_sr_ready(), you want to be sure that this 'BUSY' bit is cleared on >> both memories. >> >> But what about when the Status Register is read for purpose other than >> checking the state of the 'BUSY' bit? >> > Yes you are correct, I will change this. > >> What about SPI controllers supporting more than 2 memories in parallel? >> >> This solution might fit the ZynqMP controller but doesn't look so generic. >> >>> + } else { >>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); >>> + if (ret < 0) { >>> + pr_err("error %d reading SR\n", (int) ret); >>> + return ret; >>> + } >>> } >>> >>> - return val; >>> + return val[0]; >>> } >>> >>> /* >>> @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) static >>> int read_fsr(struct spi_nor *nor) { >>> int ret; >>> - u8 val; >>> + u8 val[2]; >>> >>> - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); >>> - if (ret < 0) { >>> - pr_err("error %d reading FSR\n", ret); >>> - return ret; >>> + if (nor->stripe) { >>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); >>> + if (ret < 0) { >>> + pr_err("error %d reading FSR\n", ret); >>> + return ret; >>> + } >>> + val[0] &= val[1]; >> Same comment here: why '&' rather than '|'? >> Surely because of the the 'READY' bit which should be set for both memories. > I will update this also. >> >>> + } else { >>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); >>> + if (ret < 0) { >>> + pr_err("error %d reading FSR\n", ret); >>> + return ret; >>> + } >>> } >>> >>> - return val; >>> + return val[0]; >>> } >>> >>> /* >>> @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct spi_nor >> *nor) >>> */ >>> static int erase_chip(struct spi_nor *nor) { >>> + u32 ret; >>> + >>> dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> 10)); >>> >>> - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); >>> + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); >>> + if (ret) >>> + return ret; >>> + >>> + return ret; >>> + >> >> if (ret) >> return ret; >> else >> return ret; >> >> This chunk should be removed, it doesn't ease the patch review ;) > Ok, I will remove. >> >>> } >>> >>> static int spi_nor_lock_and_prep(struct spi_nor *nor, enum >>> spi_nor_ops ops) @@ -349,7 +375,7 @@ static int >>> spi_nor_erase_sector(struct spi_nor *nor, u32 addr) static int >>> spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { >>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>> - u32 addr, len; >>> + u32 addr, len, offset; >>> uint32_t rem; >>> int ret; >>> >>> @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info *mtd, >> struct erase_info *instr) >>> /* "sector"-at-a-time erase */ >>> } else { >>> while (len) { >>> + >>> write_enable(nor); >>> + offset = addr; >>> + if (nor->stripe) >>> + offset /= 2; >> >> I guess this should be /= 4 for controllers supporting 4 memories in parallel. >> Shouldn't you use nor->shift and define shift as an unsigned int instead of a >> bool? >> offset >>= nor->shift; >> > Yes we can use this shift, I will update > >> Anyway, by tuning the address here in spi-nor.c rather than in the SPI >> controller driver, you impose a model to support parallel memories that >> might not be suited to other controllers. > > For this ZynqMP GQSPI controller parallel configuration, globally spi-nor should know about this stripe feature > And based on that address has to change. > As I mentioned in cover letter, this controller in parallel configuration will work with even addresses only. > i.e. Before creating address format(m25p_addr2cmd) in mtd layer, spi-nor should change that address based on stripe option. > > I am updating this offset based on stripe option, and stripe option will update by reading dt property in nor_scan(). > So the controller which doesn't support, then the stripe will be zero. > Or Can you please suggest any other way? > >> >>> >>> - ret = spi_nor_erase_sector(nor, addr); >>> + ret = spi_nor_erase_sector(nor, offset); >>> if (ret) >>> goto erase_err; >>> >>> @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >>> bool use_top; >>> int ret; >>> >>> + ofs = ofs >> nor->shift; >>> + >>> status_old = read_sr(nor); >>> if (status_old < 0) >>> return status_old; >>> @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >>> bool use_top; >>> int ret; >>> >>> + ofs = ofs >> nor->shift; >>> + >>> status_old = read_sr(nor); >>> if (status_old < 0) >>> return status_old; >>> @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t >> ofs, uint64_t len) >>> if (ret) >>> return ret; >>> >>> + ofs = ofs >> nor->shift; >>> + >>> ret = nor->flash_lock(nor, ofs, len); >>> >>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); @@ - >> 724,6 +760,8 >>> @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t >> len) >>> if (ret) >>> return ret; >>> >>> + ofs = ofs >> nor->shift; >>> + >>> ret = nor->flash_unlock(nor, ofs, len); >>> >>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); @@ - >> 1018,6 +1056,9 >>> @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) >>> u8 id[SPI_NOR_MAX_ID_LEN]; >>> const struct flash_info *info; >>> >>> + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | >>> + SPI_MASTER_DATA_STRIPE); >>> + >>> tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, >> SPI_NOR_MAX_ID_LEN); >>> if (tmp < 0) { >>> dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); >> @@ -1041,6 >>> +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, >>> size_t len, { >>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>> int ret; >>> + u32 offset = from; >>> >>> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); >>> >>> @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info *mtd, >> loff_t from, size_t len, >>> return ret; >>> >>> while (len) { >>> - ret = nor->read(nor, from, len, buf); >>> + >>> + offset = from; >>> + >>> + if (nor->stripe) >>> + offset /= 2; >>> + >>> + ret = nor->read(nor, offset, len, buf); >>> if (ret == 0) { >>> /* We shouldn't see 0-length reads */ >>> ret = -EIO; >>> @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info *mtd, >> loff_t to, size_t len, >>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>> size_t page_offset, page_remain, i; >>> ssize_t ret; >>> + u32 offset; >>> >>> dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); >>> >>> @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info *mtd, >> loff_t to, size_t len, >>> /* the size of data remaining on the first page */ >>> page_remain = min_t(size_t, >>> nor->page_size - page_offset, len - i); >>> + offset = (to + i); >>> + >>> + if (nor->stripe) >>> + offset /= 2; >>> >>> write_enable(nor); >>> - ret = nor->write(nor, to + i, page_remain, buf + i); >>> + ret = nor->write(nor, (offset), page_remain, buf + i); >>> if (ret < 0) >>> goto write_err; >>> written = ret; >>> @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor *nor) >>> >>> int spi_nor_scan(struct spi_nor *nor, const char *name, enum >>> read_mode mode) { >>> - const struct flash_info *info = NULL; >>> + struct flash_info *info = NULL; >> >> You should not remove the const and should not try to modify members of >> *info. >> >>> struct device *dev = nor->dev; >>> struct mtd_info *mtd = &nor->mtd; >>> struct device_node *np = spi_nor_get_flash_node(nor); >>> - int ret; >>> - int i; >>> + struct device_node *np_spi; >>> + int ret, i, xlnx_qspi_mode; >>> >>> ret = spi_nor_check(nor); >>> if (ret) >>> return ret; >>> >>> if (name) >>> - info = spi_nor_match_id(name); >>> + info = (struct flash_info *)spi_nor_match_id(name); >>> /* Try to auto-detect if chip name wasn't specified or not found */ >>> if (!info) >>> - info = spi_nor_read_id(nor); >>> + info = (struct flash_info *)spi_nor_read_id(nor); >>> if (IS_ERR_OR_NULL(info)) >>> return -ENOENT; >>> >> Both spi_nor_match_id() and spi_nor_read_id(), when they succeed, return >> a pointer to an entry of the spi_nor_ids[] array, which is located in a read- >> only memory area. >> >> Since your patch doesn't remove the const attribute of the spi_nor_ids[], I >> wonder whether it has been tested. I expect it not to work on most >> architecture. >> >> Anyway spi_nor_ids[] should remain const. Let's take the case of eXecution >> In Place (XIP) from an external memory: if spi_nor_ids[] is const, it can be >> read directly from this external (read-only) memory and we never need to >> copy the array in RAM, so we save some KB of RAM. >> This is just an example but I guess we can find other reasons to keep this >> array const. >> >>> @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, const char >> *name, enum read_mode mode) >>> */ >>> dev_warn(dev, "found %s, expected %s\n", >>> jinfo->name, info->name); >>> - info = jinfo; >>> + info = (struct flash_info *)jinfo; >>> } >>> } >>> >>> @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, const char >> *name, enum read_mode mode) >>> mtd->size = info->sector_size * info->n_sectors; >>> mtd->_erase = spi_nor_erase; >>> mtd->_read = spi_nor_read; >>> +#ifdef CONFIG_OF >>> + np_spi = of_get_next_parent(np); >>> + >>> + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", >>> + &xlnx_qspi_mode) < 0) { >> This really looks controller specific so should not be placed in the generic spi- >> nor.c file. > > Const is removed in info struct, because to update info members based parallel configuration. > As I mentioned above, for this parallel configuration, mtd and spi-nor should know the details like > mtd->size, info->sectors, sector_size and page_size. You can tune the values of nor->mtd.size, nor->mtd.erasesize, nor->page_size or whatever member of nor/nor.mtd as needed without ever modifying members of *info. If you modify *info then spi_nor_scan() is called a 2nd time to probe and configure SPI memories of the same part but connected to another controller, the values of the modified members in this *info would not be those expected. So *info and the spi_nor_ids[] array must remain const. The *info structure is not used outside spi_nor_scan(); none of spi_nor_read(), spi_nor_write() and spi_nor_erase() refers to *info hence every relevant value can be set only nor or nor->mtd members. Anyway, I think OR'ing or AND'ing values of memory registers depending on the relevant bit we want to check is not the right solution. If done in spi-nor.c, there would be a specific case for each memory register we read, each register bit would have to be handled differently. spi-nor.c tries to support as much memory parts as possible, it deals with many registers and bits: Status/Control registers, Quad Enable bits... If we start to OR or AND each of these register values to support the stripping mode, spi-nor will become really hard to maintain. I don't know whether it could be done with the xilinx controller but I thought about first configuring the two memories independently calling spi_nor_scan() twice; one call for each memory. Then the xilinx driver could register only one struct mtd_info, overriding mtd->_read() [and likely mtd->_write() and mtd->_erase() too] set by spi_nor_scan() with a xilinx driver custom implementation so this driver supports its controller stripping mode as it wants. Of course, this solution assumes that the SPI controller has one dedicated chip-select line for each memory and not a single chip-select line shared by both memories. The memories should be configured independently: you can't assume multiple instances of the very same memory part always return the exact same value when reading one of their register. Logical AND/OR is not a generic solution, IMHO. If the xilinx controller has only one shared chip-select line then let's see whether 2 GPIO lines could be used as independent chip-select lines. Best regards, Cyrille > So during spi_nor_scan only mtd will update all these details, that's why I have added controller specific check to update those. > > Can you please suggest, any other way to let mtd and spi-nor to know about this configuration without touching the core layers? > > Please let me know, if I missed providing any required info. > >> >>> + nor->shift = 0; >>> + nor->stripe = 0; >>> + } else if (xlnx_qspi_mode == 2) { >>> + nor->shift = 1; >>> + info->sector_size <<= nor->shift; >>> + info->page_size <<= nor->shift; >> Just don't modify *info members! :) >> >> >>> + mtd->size <<= nor->shift; >>> + nor->stripe = 1; >>> + nor->spi->master->flags |= (SPI_MASTER_BOTH_CS | >>> + SPI_MASTER_DATA_STRIPE); >>> + } >>> +#else >>> + /* Default to single */ >>> + nor->shift = 0; >>> + nor->stripe = 0; >>> +#endif >> Best regards, >> >> Cyrille > > Thanks, > Naga Sureshkumar Relli >
Hi Cyrille, > > Hi Cyrille, > > > >> I have not finished to review the whole series yet but here some > >> first > >> comments: > > > > Thanks for reviewing these patch series. > > > >> > >> Le 27/11/2016 à 09:33, Naga Sureshkumar Relli a écrit : > >>> This patch adds stripe support and it is needed for GQSPI parallel > >>> configuration mode by: > >>> > >>> - Adding required parameters like stripe and shift to spi_nor > >>> structure. > >>> - Initializing all added parameters in spi_nor_scan() > >>> - Updating read_sr() and read_fsr() for getting status from both > >>> flashes > >>> - Increasing page_size, sector_size, erase_size and toatal flash > >>> size as and when required. > >>> - Dividing address by 2 > >>> - Updating spi->master->flags for qspi driver to change CS > >>> > >>> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> > >>> --- > >>> Changes for v4: > >>> - rename isparallel to stripe > >>> Changes for v3: > >>> - No change > >>> Changes for v2: > >>> - Splitted to separate MTD layer changes from SPI core changes > >>> --- > >>> drivers/mtd/spi-nor/spi-nor.c | 130 > >> ++++++++++++++++++++++++++++++++---------- > >>> include/linux/mtd/spi-nor.h | 2 + > >>> 2 files changed, 103 insertions(+), 29 deletions(-) > >>> > >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c > >>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..4252239 100644 > >>> --- a/drivers/mtd/spi-nor/spi-nor.c > >>> +++ b/drivers/mtd/spi-nor/spi-nor.c > >>> @@ -22,6 +22,7 @@ > >>> #include <linux/of_platform.h> > >>> #include <linux/spi/flash.h> > >>> #include <linux/mtd/spi-nor.h> > >>> +#include <linux/spi/spi.h> > >>> > >>> /* Define max times to check status register before we give up. */ > >>> > >>> @@ -89,15 +90,24 @@ static const struct flash_info > >>> *spi_nor_match_id(const char *name); static int read_sr(struct > >>> spi_nor *nor) { > >>> int ret; > >>> - u8 val; > >>> + u8 val[2]; > >>> > >>> - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); > >>> - if (ret < 0) { > >>> - pr_err("error %d reading SR\n", (int) ret); > >>> - return ret; > >>> + if (nor->stripe) { > >>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); > >>> + if (ret < 0) { > >>> + pr_err("error %d reading SR\n", (int) ret); > >>> + return ret; > >>> + } > >>> + val[0] |= val[1]; > >> Why '|' rather than '&' ? > >> I guess because of the 'Write In Progress/Busy' bit: when called by > >> spi_nor_sr_ready(), you want to be sure that this 'BUSY' bit is > >> cleared on both memories. > >> > >> But what about when the Status Register is read for purpose other > >> than checking the state of the 'BUSY' bit? > >> > > Yes you are correct, I will change this. > > > >> What about SPI controllers supporting more than 2 memories in parallel? > >> > >> This solution might fit the ZynqMP controller but doesn't look so generic. > >> > >>> + } else { > >>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); > >>> + if (ret < 0) { > >>> + pr_err("error %d reading SR\n", (int) ret); > >>> + return ret; > >>> + } > >>> } > >>> > >>> - return val; > >>> + return val[0]; > >>> } > >>> > >>> /* > >>> @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) > >>> static int read_fsr(struct spi_nor *nor) { > >>> int ret; > >>> - u8 val; > >>> + u8 val[2]; > >>> > >>> - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); > >>> - if (ret < 0) { > >>> - pr_err("error %d reading FSR\n", ret); > >>> - return ret; > >>> + if (nor->stripe) { > >>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); > >>> + if (ret < 0) { > >>> + pr_err("error %d reading FSR\n", ret); > >>> + return ret; > >>> + } > >>> + val[0] &= val[1]; > >> Same comment here: why '&' rather than '|'? > >> Surely because of the the 'READY' bit which should be set for both > memories. > > I will update this also. > >> > >>> + } else { > >>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); > >>> + if (ret < 0) { > >>> + pr_err("error %d reading FSR\n", ret); > >>> + return ret; > >>> + } > >>> } > >>> > >>> - return val; > >>> + return val[0]; > >>> } > >>> > >>> /* > >>> @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct > >>> spi_nor > >> *nor) > >>> */ > >>> static int erase_chip(struct spi_nor *nor) { > >>> + u32 ret; > >>> + > >>> dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> 10)); > >>> > >>> - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); > >>> + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + return ret; > >>> + > >> > >> if (ret) > >> return ret; > >> else > >> return ret; > >> > >> This chunk should be removed, it doesn't ease the patch review ;) > > Ok, I will remove. > >> > >>> } > >>> > >>> static int spi_nor_lock_and_prep(struct spi_nor *nor, enum > >>> spi_nor_ops ops) @@ -349,7 +375,7 @@ static int > >>> spi_nor_erase_sector(struct spi_nor *nor, u32 addr) static int > >>> spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { > >>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>> - u32 addr, len; > >>> + u32 addr, len, offset; > >>> uint32_t rem; > >>> int ret; > >>> > >>> @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info *mtd, > >> struct erase_info *instr) > >>> /* "sector"-at-a-time erase */ > >>> } else { > >>> while (len) { > >>> + > >>> write_enable(nor); > >>> + offset = addr; > >>> + if (nor->stripe) > >>> + offset /= 2; > >> > >> I guess this should be /= 4 for controllers supporting 4 memories in > parallel. > >> Shouldn't you use nor->shift and define shift as an unsigned int > >> instead of a bool? > >> offset >>= nor->shift; > >> > > Yes we can use this shift, I will update > > > >> Anyway, by tuning the address here in spi-nor.c rather than in the > >> SPI controller driver, you impose a model to support parallel > >> memories that might not be suited to other controllers. > > > > For this ZynqMP GQSPI controller parallel configuration, globally > > spi-nor should know about this stripe feature And based on that address > has to change. > > As I mentioned in cover letter, this controller in parallel configuration will > work with even addresses only. > > i.e. Before creating address format(m25p_addr2cmd) in mtd layer, spi-nor > should change that address based on stripe option. > > > > I am updating this offset based on stripe option, and stripe option will > update by reading dt property in nor_scan(). > > So the controller which doesn't support, then the stripe will be zero. > > Or Can you please suggest any other way? > > > >> > >>> > >>> - ret = spi_nor_erase_sector(nor, addr); > >>> + ret = spi_nor_erase_sector(nor, offset); > >>> if (ret) > >>> goto erase_err; > >>> > >>> @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, loff_t > >>> ofs, > >> uint64_t len) > >>> bool use_top; > >>> int ret; > >>> > >>> + ofs = ofs >> nor->shift; > >>> + > >>> status_old = read_sr(nor); > >>> if (status_old < 0) > >>> return status_old; > >>> @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, > >>> loff_t ofs, > >> uint64_t len) > >>> bool use_top; > >>> int ret; > >>> > >>> + ofs = ofs >> nor->shift; > >>> + > >>> status_old = read_sr(nor); > >>> if (status_old < 0) > >>> return status_old; > >>> @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info *mtd, > >>> loff_t > >> ofs, uint64_t len) > >>> if (ret) > >>> return ret; > >>> > >>> + ofs = ofs >> nor->shift; > >>> + > >>> ret = nor->flash_lock(nor, ofs, len); > >>> > >>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); @@ - > >> 724,6 +760,8 > >>> @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, > >>> uint64_t > >> len) > >>> if (ret) > >>> return ret; > >>> > >>> + ofs = ofs >> nor->shift; > >>> + > >>> ret = nor->flash_unlock(nor, ofs, len); > >>> > >>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); @@ - > >> 1018,6 +1056,9 > >>> @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > >>> u8 id[SPI_NOR_MAX_ID_LEN]; > >>> const struct flash_info *info; > >>> > >>> + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | > >>> + SPI_MASTER_DATA_STRIPE); > >>> + > >>> tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, > >> SPI_NOR_MAX_ID_LEN); > >>> if (tmp < 0) { > >>> dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); > >> @@ -1041,6 > >>> +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t > >>> +from, > >>> size_t len, { > >>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>> int ret; > >>> + u32 offset = from; > >>> > >>> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > >>> > >>> @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info *mtd, > >> loff_t from, size_t len, > >>> return ret; > >>> > >>> while (len) { > >>> - ret = nor->read(nor, from, len, buf); > >>> + > >>> + offset = from; > >>> + > >>> + if (nor->stripe) > >>> + offset /= 2; > >>> + > >>> + ret = nor->read(nor, offset, len, buf); > >>> if (ret == 0) { > >>> /* We shouldn't see 0-length reads */ > >>> ret = -EIO; > >>> @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info *mtd, > >> loff_t to, size_t len, > >>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>> size_t page_offset, page_remain, i; > >>> ssize_t ret; > >>> + u32 offset; > >>> > >>> dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); > >>> > >>> @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info > >>> *mtd, > >> loff_t to, size_t len, > >>> /* the size of data remaining on the first page */ > >>> page_remain = min_t(size_t, > >>> nor->page_size - page_offset, len - i); > >>> + offset = (to + i); > >>> + > >>> + if (nor->stripe) > >>> + offset /= 2; > >>> > >>> write_enable(nor); > >>> - ret = nor->write(nor, to + i, page_remain, buf + i); > >>> + ret = nor->write(nor, (offset), page_remain, buf + i); > >>> if (ret < 0) > >>> goto write_err; > >>> written = ret; > >>> @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor > >>> *nor) > >>> > >>> int spi_nor_scan(struct spi_nor *nor, const char *name, enum > >>> read_mode mode) { > >>> - const struct flash_info *info = NULL; > >>> + struct flash_info *info = NULL; > >> > >> You should not remove the const and should not try to modify members > >> of *info. > >> > >>> struct device *dev = nor->dev; > >>> struct mtd_info *mtd = &nor->mtd; > >>> struct device_node *np = spi_nor_get_flash_node(nor); > >>> - int ret; > >>> - int i; > >>> + struct device_node *np_spi; > >>> + int ret, i, xlnx_qspi_mode; > >>> > >>> ret = spi_nor_check(nor); > >>> if (ret) > >>> return ret; > >>> > >>> if (name) > >>> - info = spi_nor_match_id(name); > >>> + info = (struct flash_info *)spi_nor_match_id(name); > >>> /* Try to auto-detect if chip name wasn't specified or not found */ > >>> if (!info) > >>> - info = spi_nor_read_id(nor); > >>> + info = (struct flash_info *)spi_nor_read_id(nor); > >>> if (IS_ERR_OR_NULL(info)) > >>> return -ENOENT; > >>> > >> Both spi_nor_match_id() and spi_nor_read_id(), when they succeed, > >> return a pointer to an entry of the spi_nor_ids[] array, which is > >> located in a read- only memory area. > >> > >> Since your patch doesn't remove the const attribute of the > >> spi_nor_ids[], I wonder whether it has been tested. I expect it not > >> to work on most architecture. > >> > >> Anyway spi_nor_ids[] should remain const. Let's take the case of > >> eXecution In Place (XIP) from an external memory: if spi_nor_ids[] is > >> const, it can be read directly from this external (read-only) memory > >> and we never need to copy the array in RAM, so we save some KB of > RAM. > >> This is just an example but I guess we can find other reasons to keep > >> this array const. > >> > >>> @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, const > >>> char > >> *name, enum read_mode mode) > >>> */ > >>> dev_warn(dev, "found %s, expected %s\n", > >>> jinfo->name, info->name); > >>> - info = jinfo; > >>> + info = (struct flash_info *)jinfo; > >>> } > >>> } > >>> > >>> @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, const > >>> char > >> *name, enum read_mode mode) > >>> mtd->size = info->sector_size * info->n_sectors; > >>> mtd->_erase = spi_nor_erase; > >>> mtd->_read = spi_nor_read; > >>> +#ifdef CONFIG_OF > >>> + np_spi = of_get_next_parent(np); > >>> + > >>> + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", > >>> + &xlnx_qspi_mode) < 0) { > >> This really looks controller specific so should not be placed in the > >> generic spi- nor.c file. > > > > Const is removed in info struct, because to update info members based > parallel configuration. > > As I mentioned above, for this parallel configuration, mtd and > > spi-nor should know the details like > > mtd->size, info->sectors, sector_size and page_size. > > You can tune the values of nor->mtd.size, nor->mtd.erasesize, nor- > >page_size or whatever member of nor/nor.mtd as needed without ever > modifying members of *info. > > If you modify *info then spi_nor_scan() is called a 2nd time to probe and > configure SPI memories of the same part but connected to another > controller, the values of the modified members in this *info would not be > those expected. > So *info and the spi_nor_ids[] array must remain const. > > The *info structure is not used outside spi_nor_scan(); none of > spi_nor_read(), > spi_nor_write() and spi_nor_erase() refers to *info hence every relevant > value can be set only nor or nor->mtd members. > > > Anyway, I think OR'ing or AND'ing values of memory registers depending on > the relevant bit we want to check is not the right solution. > If done in spi-nor.c, there would be a specific case for each memory register > we read, each register bit would have to be handled differently. > > spi-nor.c tries to support as much memory parts as possible, it deals with > many registers and bits: Status/Control registers, Quad Enable bits... > > If we start to OR or AND each of these register values to support the > stripping mode, spi-nor will become really hard to maintain. > > I don't know whether it could be done with the xilinx controller but I thought > about first configuring the two memories independently calling > spi_nor_scan() twice; one call for each memory. > > Then the xilinx driver could register only one struct mtd_info, overriding > mtd->_read() [and likely mtd->_write() and mtd->_erase() too] set by > spi_nor_scan() with a xilinx driver custom implementation so this driver > supports its controller stripping mode as it wants. > > Of course, this solution assumes that the SPI controller has one dedicated > chip-select line for each memory and not a single chip-select line shared by > both memories. The memories should be configured independently: you > can't assume multiple instances of the very same memory part always return > the exact same value when reading one of their register. Logical AND/OR is > not a generic solution, IMHO. > > If the xilinx controller has only one shared chip-select line then let's see > whether 2 GPIO lines could be used as independent chip-select lines. > > In parallel configuration, Physically we have two flashes but mtd will see as single flash memory (sum of both memories) If we call spi_nor_scan(), twice then read/write will override but nor->mtd.size, nor->mtd.erasesize, nor->page_size will remain same, I,e they will also override, they won't append. I tried calling spi_nor_scan() twice by some hacks but nor->mtd.size, nor->mtd.erasesize, nor->page_size are not changing Also the same issue we are getting for flash address, need to shift the address to work in this configuration. Also to tune nor->mtd.size, nor->mtd.erasesize, nor->page_size, we need to touch this spi-nor.c Please kindly suggest, if I am wrong. Thanks, Naga Sureshkumar Relli > Best regards, > > Cyrille This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Hi Naga, Le 05/12/2016 à 08:02, Naga Sureshkumar Relli a écrit : > Hi Cyrille, > >>> Hi Cyrille, >>> >>>> I have not finished to review the whole series yet but here some >>>> first >>>> comments: >>> >>> Thanks for reviewing these patch series. >>> >>>> >>>> Le 27/11/2016 à 09:33, Naga Sureshkumar Relli a écrit : >>>>> This patch adds stripe support and it is needed for GQSPI parallel >>>>> configuration mode by: >>>>> >>>>> - Adding required parameters like stripe and shift to spi_nor >>>>> structure. >>>>> - Initializing all added parameters in spi_nor_scan() >>>>> - Updating read_sr() and read_fsr() for getting status from both >>>>> flashes >>>>> - Increasing page_size, sector_size, erase_size and toatal flash >>>>> size as and when required. >>>>> - Dividing address by 2 >>>>> - Updating spi->master->flags for qspi driver to change CS >>>>> >>>>> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> >>>>> --- >>>>> Changes for v4: >>>>> - rename isparallel to stripe >>>>> Changes for v3: >>>>> - No change >>>>> Changes for v2: >>>>> - Splitted to separate MTD layer changes from SPI core changes >>>>> --- >>>>> drivers/mtd/spi-nor/spi-nor.c | 130 >>>> ++++++++++++++++++++++++++++++++---------- >>>>> include/linux/mtd/spi-nor.h | 2 + >>>>> 2 files changed, 103 insertions(+), 29 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c >>>>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..4252239 100644 >>>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>>> @@ -22,6 +22,7 @@ >>>>> #include <linux/of_platform.h> >>>>> #include <linux/spi/flash.h> >>>>> #include <linux/mtd/spi-nor.h> >>>>> +#include <linux/spi/spi.h> >>>>> >>>>> /* Define max times to check status register before we give up. */ >>>>> >>>>> @@ -89,15 +90,24 @@ static const struct flash_info >>>>> *spi_nor_match_id(const char *name); static int read_sr(struct >>>>> spi_nor *nor) { >>>>> int ret; >>>>> - u8 val; >>>>> + u8 val[2]; >>>>> >>>>> - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); >>>>> - if (ret < 0) { >>>>> - pr_err("error %d reading SR\n", (int) ret); >>>>> - return ret; >>>>> + if (nor->stripe) { >>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); >>>>> + if (ret < 0) { >>>>> + pr_err("error %d reading SR\n", (int) ret); >>>>> + return ret; >>>>> + } >>>>> + val[0] |= val[1]; >>>> Why '|' rather than '&' ? >>>> I guess because of the 'Write In Progress/Busy' bit: when called by >>>> spi_nor_sr_ready(), you want to be sure that this 'BUSY' bit is >>>> cleared on both memories. >>>> >>>> But what about when the Status Register is read for purpose other >>>> than checking the state of the 'BUSY' bit? >>>> >>> Yes you are correct, I will change this. >>> >>>> What about SPI controllers supporting more than 2 memories in parallel? >>>> >>>> This solution might fit the ZynqMP controller but doesn't look so generic. >>>> >>>>> + } else { >>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); >>>>> + if (ret < 0) { >>>>> + pr_err("error %d reading SR\n", (int) ret); >>>>> + return ret; >>>>> + } >>>>> } >>>>> >>>>> - return val; >>>>> + return val[0]; >>>>> } >>>>> >>>>> /* >>>>> @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) >>>>> static int read_fsr(struct spi_nor *nor) { >>>>> int ret; >>>>> - u8 val; >>>>> + u8 val[2]; >>>>> >>>>> - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); >>>>> - if (ret < 0) { >>>>> - pr_err("error %d reading FSR\n", ret); >>>>> - return ret; >>>>> + if (nor->stripe) { >>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); >>>>> + if (ret < 0) { >>>>> + pr_err("error %d reading FSR\n", ret); >>>>> + return ret; >>>>> + } >>>>> + val[0] &= val[1]; >>>> Same comment here: why '&' rather than '|'? >>>> Surely because of the the 'READY' bit which should be set for both >> memories. >>> I will update this also. >>>> >>>>> + } else { >>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); >>>>> + if (ret < 0) { >>>>> + pr_err("error %d reading FSR\n", ret); >>>>> + return ret; >>>>> + } >>>>> } >>>>> >>>>> - return val; >>>>> + return val[0]; >>>>> } >>>>> >>>>> /* >>>>> @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct >>>>> spi_nor >>>> *nor) >>>>> */ >>>>> static int erase_chip(struct spi_nor *nor) { >>>>> + u32 ret; >>>>> + >>>>> dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> 10)); >>>>> >>>>> - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); >>>>> + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return ret; >>>>> + >>>> >>>> if (ret) >>>> return ret; >>>> else >>>> return ret; >>>> >>>> This chunk should be removed, it doesn't ease the patch review ;) >>> Ok, I will remove. >>>> >>>>> } >>>>> >>>>> static int spi_nor_lock_and_prep(struct spi_nor *nor, enum >>>>> spi_nor_ops ops) @@ -349,7 +375,7 @@ static int >>>>> spi_nor_erase_sector(struct spi_nor *nor, u32 addr) static int >>>>> spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { >>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>>>> - u32 addr, len; >>>>> + u32 addr, len, offset; >>>>> uint32_t rem; >>>>> int ret; >>>>> >>>>> @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info *mtd, >>>> struct erase_info *instr) >>>>> /* "sector"-at-a-time erase */ >>>>> } else { >>>>> while (len) { >>>>> + >>>>> write_enable(nor); >>>>> + offset = addr; >>>>> + if (nor->stripe) >>>>> + offset /= 2; >>>> >>>> I guess this should be /= 4 for controllers supporting 4 memories in >> parallel. >>>> Shouldn't you use nor->shift and define shift as an unsigned int >>>> instead of a bool? >>>> offset >>= nor->shift; >>>> >>> Yes we can use this shift, I will update >>> >>>> Anyway, by tuning the address here in spi-nor.c rather than in the >>>> SPI controller driver, you impose a model to support parallel >>>> memories that might not be suited to other controllers. >>> >>> For this ZynqMP GQSPI controller parallel configuration, globally >>> spi-nor should know about this stripe feature And based on that address >> has to change. >>> As I mentioned in cover letter, this controller in parallel configuration will >> work with even addresses only. >>> i.e. Before creating address format(m25p_addr2cmd) in mtd layer, spi-nor >> should change that address based on stripe option. >>> >>> I am updating this offset based on stripe option, and stripe option will >> update by reading dt property in nor_scan(). >>> So the controller which doesn't support, then the stripe will be zero. >>> Or Can you please suggest any other way? >>> >>>> >>>>> >>>>> - ret = spi_nor_erase_sector(nor, addr); >>>>> + ret = spi_nor_erase_sector(nor, offset); >>>>> if (ret) >>>>> goto erase_err; >>>>> >>>>> @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, loff_t >>>>> ofs, >>>> uint64_t len) >>>>> bool use_top; >>>>> int ret; >>>>> >>>>> + ofs = ofs >> nor->shift; >>>>> + >>>>> status_old = read_sr(nor); >>>>> if (status_old < 0) >>>>> return status_old; >>>>> @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, >>>>> loff_t ofs, >>>> uint64_t len) >>>>> bool use_top; >>>>> int ret; >>>>> >>>>> + ofs = ofs >> nor->shift; >>>>> + >>>>> status_old = read_sr(nor); >>>>> if (status_old < 0) >>>>> return status_old; >>>>> @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info *mtd, >>>>> loff_t >>>> ofs, uint64_t len) >>>>> if (ret) >>>>> return ret; >>>>> >>>>> + ofs = ofs >> nor->shift; >>>>> + >>>>> ret = nor->flash_lock(nor, ofs, len); >>>>> >>>>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); @@ - >>>> 724,6 +760,8 >>>>> @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, >>>>> uint64_t >>>> len) >>>>> if (ret) >>>>> return ret; >>>>> >>>>> + ofs = ofs >> nor->shift; >>>>> + >>>>> ret = nor->flash_unlock(nor, ofs, len); >>>>> >>>>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); @@ - >>>> 1018,6 +1056,9 >>>>> @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) >>>>> u8 id[SPI_NOR_MAX_ID_LEN]; >>>>> const struct flash_info *info; >>>>> >>>>> + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | >>>>> + SPI_MASTER_DATA_STRIPE); >>>>> + >>>>> tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, >>>> SPI_NOR_MAX_ID_LEN); >>>>> if (tmp < 0) { >>>>> dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); >>>> @@ -1041,6 >>>>> +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t >>>>> +from, >>>>> size_t len, { >>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>>>> int ret; >>>>> + u32 offset = from; >>>>> >>>>> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); >>>>> >>>>> @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info *mtd, >>>> loff_t from, size_t len, >>>>> return ret; >>>>> >>>>> while (len) { >>>>> - ret = nor->read(nor, from, len, buf); >>>>> + >>>>> + offset = from; >>>>> + >>>>> + if (nor->stripe) >>>>> + offset /= 2; >>>>> + >>>>> + ret = nor->read(nor, offset, len, buf); >>>>> if (ret == 0) { >>>>> /* We shouldn't see 0-length reads */ >>>>> ret = -EIO; >>>>> @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info *mtd, >>>> loff_t to, size_t len, >>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>>>> size_t page_offset, page_remain, i; >>>>> ssize_t ret; >>>>> + u32 offset; >>>>> >>>>> dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); >>>>> >>>>> @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info >>>>> *mtd, >>>> loff_t to, size_t len, >>>>> /* the size of data remaining on the first page */ >>>>> page_remain = min_t(size_t, >>>>> nor->page_size - page_offset, len - i); >>>>> + offset = (to + i); >>>>> + >>>>> + if (nor->stripe) >>>>> + offset /= 2; >>>>> >>>>> write_enable(nor); >>>>> - ret = nor->write(nor, to + i, page_remain, buf + i); >>>>> + ret = nor->write(nor, (offset), page_remain, buf + i); >>>>> if (ret < 0) >>>>> goto write_err; >>>>> written = ret; >>>>> @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor >>>>> *nor) >>>>> >>>>> int spi_nor_scan(struct spi_nor *nor, const char *name, enum >>>>> read_mode mode) { >>>>> - const struct flash_info *info = NULL; >>>>> + struct flash_info *info = NULL; >>>> >>>> You should not remove the const and should not try to modify members >>>> of *info. >>>> >>>>> struct device *dev = nor->dev; >>>>> struct mtd_info *mtd = &nor->mtd; >>>>> struct device_node *np = spi_nor_get_flash_node(nor); >>>>> - int ret; >>>>> - int i; >>>>> + struct device_node *np_spi; >>>>> + int ret, i, xlnx_qspi_mode; >>>>> >>>>> ret = spi_nor_check(nor); >>>>> if (ret) >>>>> return ret; >>>>> >>>>> if (name) >>>>> - info = spi_nor_match_id(name); >>>>> + info = (struct flash_info *)spi_nor_match_id(name); >>>>> /* Try to auto-detect if chip name wasn't specified or not found */ >>>>> if (!info) >>>>> - info = spi_nor_read_id(nor); >>>>> + info = (struct flash_info *)spi_nor_read_id(nor); >>>>> if (IS_ERR_OR_NULL(info)) >>>>> return -ENOENT; >>>>> >>>> Both spi_nor_match_id() and spi_nor_read_id(), when they succeed, >>>> return a pointer to an entry of the spi_nor_ids[] array, which is >>>> located in a read- only memory area. >>>> >>>> Since your patch doesn't remove the const attribute of the >>>> spi_nor_ids[], I wonder whether it has been tested. I expect it not >>>> to work on most architecture. >>>> >>>> Anyway spi_nor_ids[] should remain const. Let's take the case of >>>> eXecution In Place (XIP) from an external memory: if spi_nor_ids[] is >>>> const, it can be read directly from this external (read-only) memory >>>> and we never need to copy the array in RAM, so we save some KB of >> RAM. >>>> This is just an example but I guess we can find other reasons to keep >>>> this array const. >>>> >>>>> @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, const >>>>> char >>>> *name, enum read_mode mode) >>>>> */ >>>>> dev_warn(dev, "found %s, expected %s\n", >>>>> jinfo->name, info->name); >>>>> - info = jinfo; >>>>> + info = (struct flash_info *)jinfo; >>>>> } >>>>> } >>>>> >>>>> @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, const >>>>> char >>>> *name, enum read_mode mode) >>>>> mtd->size = info->sector_size * info->n_sectors; >>>>> mtd->_erase = spi_nor_erase; >>>>> mtd->_read = spi_nor_read; >>>>> +#ifdef CONFIG_OF >>>>> + np_spi = of_get_next_parent(np); >>>>> + >>>>> + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", >>>>> + &xlnx_qspi_mode) < 0) { >>>> This really looks controller specific so should not be placed in the >>>> generic spi- nor.c file. >>> >>> Const is removed in info struct, because to update info members based >> parallel configuration. >>> As I mentioned above, for this parallel configuration, mtd and >>> spi-nor should know the details like >>> mtd->size, info->sectors, sector_size and page_size. >> >> You can tune the values of nor->mtd.size, nor->mtd.erasesize, nor- >>> page_size or whatever member of nor/nor.mtd as needed without ever >> modifying members of *info. >> >> If you modify *info then spi_nor_scan() is called a 2nd time to probe and >> configure SPI memories of the same part but connected to another >> controller, the values of the modified members in this *info would not be >> those expected. >> So *info and the spi_nor_ids[] array must remain const. >> >> The *info structure is not used outside spi_nor_scan(); none of >> spi_nor_read(), >> spi_nor_write() and spi_nor_erase() refers to *info hence every relevant >> value can be set only nor or nor->mtd members. >> >> >> Anyway, I think OR'ing or AND'ing values of memory registers depending on >> the relevant bit we want to check is not the right solution. >> If done in spi-nor.c, there would be a specific case for each memory register >> we read, each register bit would have to be handled differently. >> >> spi-nor.c tries to support as much memory parts as possible, it deals with >> many registers and bits: Status/Control registers, Quad Enable bits... >> >> If we start to OR or AND each of these register values to support the >> stripping mode, spi-nor will become really hard to maintain. >> >> I don't know whether it could be done with the xilinx controller but I thought >> about first configuring the two memories independently calling >> spi_nor_scan() twice; one call for each memory. >> >> Then the xilinx driver could register only one struct mtd_info, overriding >> mtd->_read() [and likely mtd->_write() and mtd->_erase() too] set by >> spi_nor_scan() with a xilinx driver custom implementation so this driver >> supports its controller stripping mode as it wants. >> >> Of course, this solution assumes that the SPI controller has one dedicated >> chip-select line for each memory and not a single chip-select line shared by >> both memories. The memories should be configured independently: you >> can't assume multiple instances of the very same memory part always return >> the exact same value when reading one of their register. Logical AND/OR is >> not a generic solution, IMHO. >> >> If the xilinx controller has only one shared chip-select line then let's see >> whether 2 GPIO lines could be used as independent chip-select lines. >> >> > In parallel configuration, Physically we have two flashes but mtd will see as single flash memory (sum of both memories) > If we call spi_nor_scan(), twice then read/write will override but nor->mtd.size, nor->mtd.erasesize, nor->page_size will remain same, I,e they will also override, they won't append. > I tried calling spi_nor_scan() twice by some hacks but nor->mtd.size, nor->mtd.erasesize, nor->page_size are not changing > Also the same issue we are getting for flash address, need to shift the address to work in this configuration. > Also to tune nor->mtd.size, nor->mtd.erasesize, nor->page_size, we need to touch this spi-nor.c > > Please kindly suggest, if I am wrong. > What I've been suggesting is: { struct spi_nor *nor1, *nor2; struct mtd_info *mtd; enum read_mode mode = SPI_NOR_QUAD; int err; [...] err = spi_nor_scan(nor1, NULL, mode); if (err) return err; /* or handle error properly. */ err = spi_nor_scan(nor2, NULL, mode); if (err) return err; mtd = &nor1->mtd; mtd->erasesize <<= 1; mtd->size <<= 1; mtd->writebufsize <<= 1; nor1->page_size <<= 1; /* tune all other relevant members of nor1/mtd. */ /* override relevant mtd hooks. */ mtd->_read = stripping_read; mtd->_erase = stripping_erase; mtd->_write = stripping_write; mtd->_lock = ...; mtd->_unlock = ...; mtd->_is_lock = ...; /* register a single mtd_info structure. */ err = mtd_device_register(mtd, NULL, 0); if (err) return err; [...] } Best regards, Cyrille > Thanks, > Naga Sureshkumar Relli > >> Best regards, >> >> Cyrille > > > This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately. > >
Hi Cyrille, > -----Original Message----- > From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] > Sent: Monday, December 05, 2016 6:34 PM > To: Naga Sureshkumar Relli <nagasure@xilinx.com>; broonie@kernel.org; > michal.simek@xilinx.com; Soren Brinkmann <sorenb@xilinx.com>; Harini > Katakam <harinik@xilinx.com>; Punnaiah Choudary Kalluri > <punnaia@xilinx.com> > Cc: linux-spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; linux-mtd@lists.infradead.org > Subject: Re: [LINUX RFC v4 3/4] mtd: spi-nor: add stripe support > > Hi Naga, > > Le 05/12/2016 à 08:02, Naga Sureshkumar Relli a écrit : > > Hi Cyrille, > > > >>> Hi Cyrille, > >>> > >>>> I have not finished to review the whole series yet but here some > >>>> first > >>>> comments: > >>> > >>> Thanks for reviewing these patch series. > >>> > >>>> > >>>> Le 27/11/2016 à 09:33, Naga Sureshkumar Relli a écrit : > >>>>> This patch adds stripe support and it is needed for GQSPI parallel > >>>>> configuration mode by: > >>>>> > >>>>> - Adding required parameters like stripe and shift to spi_nor > >>>>> structure. > >>>>> - Initializing all added parameters in spi_nor_scan() > >>>>> - Updating read_sr() and read_fsr() for getting status from both > >>>>> flashes > >>>>> - Increasing page_size, sector_size, erase_size and toatal flash > >>>>> size as and when required. > >>>>> - Dividing address by 2 > >>>>> - Updating spi->master->flags for qspi driver to change CS > >>>>> > >>>>> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> > >>>>> --- > >>>>> Changes for v4: > >>>>> - rename isparallel to stripe > >>>>> Changes for v3: > >>>>> - No change > >>>>> Changes for v2: > >>>>> - Splitted to separate MTD layer changes from SPI core changes > >>>>> --- > >>>>> drivers/mtd/spi-nor/spi-nor.c | 130 > >>>> ++++++++++++++++++++++++++++++++---------- > >>>>> include/linux/mtd/spi-nor.h | 2 + > >>>>> 2 files changed, 103 insertions(+), 29 deletions(-) > >>>>> > >>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c > >>>>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..4252239 100644 > >>>>> --- a/drivers/mtd/spi-nor/spi-nor.c > >>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c > >>>>> @@ -22,6 +22,7 @@ > >>>>> #include <linux/of_platform.h> > >>>>> #include <linux/spi/flash.h> > >>>>> #include <linux/mtd/spi-nor.h> > >>>>> +#include <linux/spi/spi.h> > >>>>> > >>>>> /* Define max times to check status register before we give up. > >>>>> */ > >>>>> > >>>>> @@ -89,15 +90,24 @@ static const struct flash_info > >>>>> *spi_nor_match_id(const char *name); static int read_sr(struct > >>>>> spi_nor *nor) { > >>>>> int ret; > >>>>> - u8 val; > >>>>> + u8 val[2]; > >>>>> > >>>>> - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); > >>>>> - if (ret < 0) { > >>>>> - pr_err("error %d reading SR\n", (int) ret); > >>>>> - return ret; > >>>>> + if (nor->stripe) { > >>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); > >>>>> + if (ret < 0) { > >>>>> + pr_err("error %d reading SR\n", (int) ret); > >>>>> + return ret; > >>>>> + } > >>>>> + val[0] |= val[1]; > >>>> Why '|' rather than '&' ? > >>>> I guess because of the 'Write In Progress/Busy' bit: when called by > >>>> spi_nor_sr_ready(), you want to be sure that this 'BUSY' bit is > >>>> cleared on both memories. > >>>> > >>>> But what about when the Status Register is read for purpose other > >>>> than checking the state of the 'BUSY' bit? > >>>> > >>> Yes you are correct, I will change this. > >>> > >>>> What about SPI controllers supporting more than 2 memories in > parallel? > >>>> > >>>> This solution might fit the ZynqMP controller but doesn't look so > generic. > >>>> > >>>>> + } else { > >>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); > >>>>> + if (ret < 0) { > >>>>> + pr_err("error %d reading SR\n", (int) ret); > >>>>> + return ret; > >>>>> + } > >>>>> } > >>>>> > >>>>> - return val; > >>>>> + return val[0]; > >>>>> } > >>>>> > >>>>> /* > >>>>> @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) > >>>>> static int read_fsr(struct spi_nor *nor) { > >>>>> int ret; > >>>>> - u8 val; > >>>>> + u8 val[2]; > >>>>> > >>>>> - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); > >>>>> - if (ret < 0) { > >>>>> - pr_err("error %d reading FSR\n", ret); > >>>>> - return ret; > >>>>> + if (nor->stripe) { > >>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); > >>>>> + if (ret < 0) { > >>>>> + pr_err("error %d reading FSR\n", ret); > >>>>> + return ret; > >>>>> + } > >>>>> + val[0] &= val[1]; > >>>> Same comment here: why '&' rather than '|'? > >>>> Surely because of the the 'READY' bit which should be set for both > >> memories. > >>> I will update this also. > >>>> > >>>>> + } else { > >>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); > >>>>> + if (ret < 0) { > >>>>> + pr_err("error %d reading FSR\n", ret); > >>>>> + return ret; > >>>>> + } > >>>>> } > >>>>> > >>>>> - return val; > >>>>> + return val[0]; > >>>>> } > >>>>> > >>>>> /* > >>>>> @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct > >>>>> spi_nor > >>>> *nor) > >>>>> */ > >>>>> static int erase_chip(struct spi_nor *nor) { > >>>>> + u32 ret; > >>>>> + > >>>>> dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> > >>>>> 10)); > >>>>> > >>>>> - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); > >>>>> + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); if > >>>>> + (ret) > >>>>> + return ret; > >>>>> + > >>>>> + return ret; > >>>>> + > >>>> > >>>> if (ret) > >>>> return ret; > >>>> else > >>>> return ret; > >>>> > >>>> This chunk should be removed, it doesn't ease the patch review ;) > >>> Ok, I will remove. > >>>> > >>>>> } > >>>>> > >>>>> static int spi_nor_lock_and_prep(struct spi_nor *nor, enum > >>>>> spi_nor_ops ops) @@ -349,7 +375,7 @@ static int > >>>>> spi_nor_erase_sector(struct spi_nor *nor, u32 addr) static int > >>>>> spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { > >>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>>>> - u32 addr, len; > >>>>> + u32 addr, len, offset; > >>>>> uint32_t rem; > >>>>> int ret; > >>>>> > >>>>> @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info > >>>>> *mtd, > >>>> struct erase_info *instr) > >>>>> /* "sector"-at-a-time erase */ > >>>>> } else { > >>>>> while (len) { > >>>>> + > >>>>> write_enable(nor); > >>>>> + offset = addr; > >>>>> + if (nor->stripe) > >>>>> + offset /= 2; > >>>> > >>>> I guess this should be /= 4 for controllers supporting 4 memories > >>>> in > >> parallel. > >>>> Shouldn't you use nor->shift and define shift as an unsigned int > >>>> instead of a bool? > >>>> offset >>= nor->shift; > >>>> > >>> Yes we can use this shift, I will update > >>> > >>>> Anyway, by tuning the address here in spi-nor.c rather than in the > >>>> SPI controller driver, you impose a model to support parallel > >>>> memories that might not be suited to other controllers. > >>> > >>> For this ZynqMP GQSPI controller parallel configuration, globally > >>> spi-nor should know about this stripe feature And based on that > >>> address > >> has to change. > >>> As I mentioned in cover letter, this controller in parallel > >>> configuration will > >> work with even addresses only. > >>> i.e. Before creating address format(m25p_addr2cmd) in mtd layer, > >>> spi-nor > >> should change that address based on stripe option. > >>> > >>> I am updating this offset based on stripe option, and stripe option > >>> will > >> update by reading dt property in nor_scan(). > >>> So the controller which doesn't support, then the stripe will be zero. > >>> Or Can you please suggest any other way? > >>> > >>>> > >>>>> > >>>>> - ret = spi_nor_erase_sector(nor, addr); > >>>>> + ret = spi_nor_erase_sector(nor, offset); > >>>>> if (ret) > >>>>> goto erase_err; > >>>>> > >>>>> @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, > >>>>> loff_t ofs, > >>>> uint64_t len) > >>>>> bool use_top; > >>>>> int ret; > >>>>> > >>>>> + ofs = ofs >> nor->shift; > >>>>> + > >>>>> status_old = read_sr(nor); > >>>>> if (status_old < 0) > >>>>> return status_old; > >>>>> @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, > >>>>> loff_t ofs, > >>>> uint64_t len) > >>>>> bool use_top; > >>>>> int ret; > >>>>> > >>>>> + ofs = ofs >> nor->shift; > >>>>> + > >>>>> status_old = read_sr(nor); > >>>>> if (status_old < 0) > >>>>> return status_old; > >>>>> @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info *mtd, > >>>>> loff_t > >>>> ofs, uint64_t len) > >>>>> if (ret) > >>>>> return ret; > >>>>> > >>>>> + ofs = ofs >> nor->shift; > >>>>> + > >>>>> ret = nor->flash_lock(nor, ofs, len); > >>>>> > >>>>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); @@ - > >>>> 724,6 +760,8 > >>>>> @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, > >>>>> uint64_t > >>>> len) > >>>>> if (ret) > >>>>> return ret; > >>>>> > >>>>> + ofs = ofs >> nor->shift; > >>>>> + > >>>>> ret = nor->flash_unlock(nor, ofs, len); > >>>>> > >>>>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); @@ - > >>>> 1018,6 +1056,9 > >>>>> @@ static const struct flash_info *spi_nor_read_id(struct spi_nor > *nor) > >>>>> u8 id[SPI_NOR_MAX_ID_LEN]; > >>>>> const struct flash_info *info; > >>>>> > >>>>> + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | > >>>>> + SPI_MASTER_DATA_STRIPE); > >>>>> + > >>>>> tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, > >>>> SPI_NOR_MAX_ID_LEN); > >>>>> if (tmp < 0) { > >>>>> dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); > >>>> @@ -1041,6 > >>>>> +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t > >>>>> +from, > >>>>> size_t len, { > >>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>>>> int ret; > >>>>> + u32 offset = from; > >>>>> > >>>>> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > >>>>> > >>>>> @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info > >>>>> *mtd, > >>>> loff_t from, size_t len, > >>>>> return ret; > >>>>> > >>>>> while (len) { > >>>>> - ret = nor->read(nor, from, len, buf); > >>>>> + > >>>>> + offset = from; > >>>>> + > >>>>> + if (nor->stripe) > >>>>> + offset /= 2; > >>>>> + > >>>>> + ret = nor->read(nor, offset, len, buf); > >>>>> if (ret == 0) { > >>>>> /* We shouldn't see 0-length reads */ > >>>>> ret = -EIO; > >>>>> @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info > >>>>> *mtd, > >>>> loff_t to, size_t len, > >>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>>>> size_t page_offset, page_remain, i; > >>>>> ssize_t ret; > >>>>> + u32 offset; > >>>>> > >>>>> dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); > >>>>> > >>>>> @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info > >>>>> *mtd, > >>>> loff_t to, size_t len, > >>>>> /* the size of data remaining on the first page */ > >>>>> page_remain = min_t(size_t, > >>>>> nor->page_size - page_offset, len - > >>>>> i); > >>>>> + offset = (to + i); > >>>>> + > >>>>> + if (nor->stripe) > >>>>> + offset /= 2; > >>>>> > >>>>> write_enable(nor); > >>>>> - ret = nor->write(nor, to + i, page_remain, buf + i); > >>>>> + ret = nor->write(nor, (offset), page_remain, buf + i); > >>>>> if (ret < 0) > >>>>> goto write_err; > >>>>> written = ret; > >>>>> @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor > >>>>> *nor) > >>>>> > >>>>> int spi_nor_scan(struct spi_nor *nor, const char *name, enum > >>>>> read_mode mode) { > >>>>> - const struct flash_info *info = NULL; > >>>>> + struct flash_info *info = NULL; > >>>> > >>>> You should not remove the const and should not try to modify > >>>> members of *info. > >>>> > >>>>> struct device *dev = nor->dev; > >>>>> struct mtd_info *mtd = &nor->mtd; > >>>>> struct device_node *np = spi_nor_get_flash_node(nor); > >>>>> - int ret; > >>>>> - int i; > >>>>> + struct device_node *np_spi; > >>>>> + int ret, i, xlnx_qspi_mode; > >>>>> > >>>>> ret = spi_nor_check(nor); > >>>>> if (ret) > >>>>> return ret; > >>>>> > >>>>> if (name) > >>>>> - info = spi_nor_match_id(name); > >>>>> + info = (struct flash_info *)spi_nor_match_id(name); > >>>>> /* Try to auto-detect if chip name wasn't specified or not found */ > >>>>> if (!info) > >>>>> - info = spi_nor_read_id(nor); > >>>>> + info = (struct flash_info *)spi_nor_read_id(nor); > >>>>> if (IS_ERR_OR_NULL(info)) > >>>>> return -ENOENT; > >>>>> > >>>> Both spi_nor_match_id() and spi_nor_read_id(), when they succeed, > >>>> return a pointer to an entry of the spi_nor_ids[] array, which is > >>>> located in a read- only memory area. > >>>> > >>>> Since your patch doesn't remove the const attribute of the > >>>> spi_nor_ids[], I wonder whether it has been tested. I expect it not > >>>> to work on most architecture. > >>>> > >>>> Anyway spi_nor_ids[] should remain const. Let's take the case of > >>>> eXecution In Place (XIP) from an external memory: if spi_nor_ids[] > >>>> is const, it can be read directly from this external (read-only) > >>>> memory and we never need to copy the array in RAM, so we save > some > >>>> KB of > >> RAM. > >>>> This is just an example but I guess we can find other reasons to > >>>> keep this array const. > >>>> > >>>>> @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, const > >>>>> char > >>>> *name, enum read_mode mode) > >>>>> */ > >>>>> dev_warn(dev, "found %s, expected %s\n", > >>>>> jinfo->name, info->name); > >>>>> - info = jinfo; > >>>>> + info = (struct flash_info *)jinfo; > >>>>> } > >>>>> } > >>>>> > >>>>> @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, const > >>>>> char > >>>> *name, enum read_mode mode) > >>>>> mtd->size = info->sector_size * info->n_sectors; > >>>>> mtd->_erase = spi_nor_erase; > >>>>> mtd->_read = spi_nor_read; > >>>>> +#ifdef CONFIG_OF > >>>>> + np_spi = of_get_next_parent(np); > >>>>> + > >>>>> + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", > >>>>> + &xlnx_qspi_mode) < 0) { > >>>> This really looks controller specific so should not be placed in > >>>> the generic spi- nor.c file. > >>> > >>> Const is removed in info struct, because to update info members > >>> based > >> parallel configuration. > >>> As I mentioned above, for this parallel configuration, mtd and > >>> spi-nor should know the details like > >>> mtd->size, info->sectors, sector_size and page_size. > >> > >> You can tune the values of nor->mtd.size, nor->mtd.erasesize, nor- > >>> page_size or whatever member of nor/nor.mtd as needed without ever > >> modifying members of *info. > >> > >> If you modify *info then spi_nor_scan() is called a 2nd time to probe > >> and configure SPI memories of the same part but connected to another > >> controller, the values of the modified members in this *info would > >> not be those expected. > >> So *info and the spi_nor_ids[] array must remain const. > >> > >> The *info structure is not used outside spi_nor_scan(); none of > >> spi_nor_read(), > >> spi_nor_write() and spi_nor_erase() refers to *info hence every > >> relevant value can be set only nor or nor->mtd members. > >> > >> > >> Anyway, I think OR'ing or AND'ing values of memory registers > >> depending on the relevant bit we want to check is not the right solution. > >> If done in spi-nor.c, there would be a specific case for each memory > >> register we read, each register bit would have to be handled differently. > >> > >> spi-nor.c tries to support as much memory parts as possible, it deals > >> with many registers and bits: Status/Control registers, Quad Enable bits... > >> > >> If we start to OR or AND each of these register values to support the > >> stripping mode, spi-nor will become really hard to maintain. > >> > >> I don't know whether it could be done with the xilinx controller but > >> I thought about first configuring the two memories independently > >> calling > >> spi_nor_scan() twice; one call for each memory. > >> > >> Then the xilinx driver could register only one struct mtd_info, > >> overriding > >> mtd->_read() [and likely mtd->_write() and mtd->_erase() too] set by > >> spi_nor_scan() with a xilinx driver custom implementation so this > >> driver supports its controller stripping mode as it wants. > >> > >> Of course, this solution assumes that the SPI controller has one > >> dedicated chip-select line for each memory and not a single > >> chip-select line shared by both memories. The memories should be > >> configured independently: you can't assume multiple instances of the > >> very same memory part always return the exact same value when reading > >> one of their register. Logical AND/OR is not a generic solution, IMHO. > >> > >> If the xilinx controller has only one shared chip-select line then > >> let's see whether 2 GPIO lines could be used as independent chip-select > lines. > >> > >> > > In parallel configuration, Physically we have two flashes but mtd will > > see as single flash memory (sum of both memories) If we call > spi_nor_scan(), twice then read/write will override but nor->mtd.size, nor- > >mtd.erasesize, nor->page_size will remain same, I,e they will also override, > they won't append. > > I tried calling spi_nor_scan() twice by some hacks but nor->mtd.size, > > nor->mtd.erasesize, nor->page_size are not changing Also the same issue > we are getting for flash address, need to shift the address to work in this > configuration. > > Also to tune nor->mtd.size, nor->mtd.erasesize, nor->page_size, we > > need to touch this spi-nor.c > > > > Please kindly suggest, if I am wrong. > > > > What I've been suggesting is: > > { > struct spi_nor *nor1, *nor2; > struct mtd_info *mtd; > enum read_mode mode = SPI_NOR_QUAD; > int err; > > [...] > > err = spi_nor_scan(nor1, NULL, mode); > if (err) > return err; /* or handle error properly. */ > > err = spi_nor_scan(nor2, NULL, mode); > if (err) > return err; > > mtd = &nor1->mtd; > mtd->erasesize <<= 1; > mtd->size <<= 1; > mtd->writebufsize <<= 1; > nor1->page_size <<= 1; > /* tune all other relevant members of nor1/mtd. */ > > /* override relevant mtd hooks. */ > mtd->_read = stripping_read; > mtd->_erase = stripping_erase; > mtd->_write = stripping_write; > mtd->_lock = ...; > mtd->_unlock = ...; > mtd->_is_lock = ...; > > /* register a single mtd_info structure. */ > err = mtd_device_register(mtd, NULL, 0); > if (err) > return err; > > [...] > } > It's really good for us to have our controller specific mtd hooks instead of changing the layer calls and thanks for this suggestion. But spi-zynqmp-gqspi.c is spi driver and all above mentioned parameters and function pointers are related to flash layer. So is it ok to update and change flash related stuff in our spi driver? > Best regards, > > Cyrille > Thanks, Naga Sureshkumar Relli This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Le 06/12/2016 à 07:54, Naga Sureshkumar Relli a écrit : > Hi Cyrille, > >> -----Original Message----- >> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] >> Sent: Monday, December 05, 2016 6:34 PM >> To: Naga Sureshkumar Relli <nagasure@xilinx.com>; broonie@kernel.org; >> michal.simek@xilinx.com; Soren Brinkmann <sorenb@xilinx.com>; Harini >> Katakam <harinik@xilinx.com>; Punnaiah Choudary Kalluri >> <punnaia@xilinx.com> >> Cc: linux-spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org; linux-mtd@lists.infradead.org >> Subject: Re: [LINUX RFC v4 3/4] mtd: spi-nor: add stripe support >> >> Hi Naga, >> >> Le 05/12/2016 à 08:02, Naga Sureshkumar Relli a écrit : >>> Hi Cyrille, >>> >>>>> Hi Cyrille, >>>>> >>>>>> I have not finished to review the whole series yet but here some >>>>>> first >>>>>> comments: >>>>> >>>>> Thanks for reviewing these patch series. >>>>> >>>>>> >>>>>> Le 27/11/2016 à 09:33, Naga Sureshkumar Relli a écrit : >>>>>>> This patch adds stripe support and it is needed for GQSPI parallel >>>>>>> configuration mode by: >>>>>>> >>>>>>> - Adding required parameters like stripe and shift to spi_nor >>>>>>> structure. >>>>>>> - Initializing all added parameters in spi_nor_scan() >>>>>>> - Updating read_sr() and read_fsr() for getting status from both >>>>>>> flashes >>>>>>> - Increasing page_size, sector_size, erase_size and toatal flash >>>>>>> size as and when required. >>>>>>> - Dividing address by 2 >>>>>>> - Updating spi->master->flags for qspi driver to change CS >>>>>>> >>>>>>> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> >>>>>>> --- >>>>>>> Changes for v4: >>>>>>> - rename isparallel to stripe >>>>>>> Changes for v3: >>>>>>> - No change >>>>>>> Changes for v2: >>>>>>> - Splitted to separate MTD layer changes from SPI core changes >>>>>>> --- >>>>>>> drivers/mtd/spi-nor/spi-nor.c | 130 >>>>>> ++++++++++++++++++++++++++++++++---------- >>>>>>> include/linux/mtd/spi-nor.h | 2 + >>>>>>> 2 files changed, 103 insertions(+), 29 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c >>>>>>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..4252239 100644 >>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>>>>> @@ -22,6 +22,7 @@ >>>>>>> #include <linux/of_platform.h> >>>>>>> #include <linux/spi/flash.h> >>>>>>> #include <linux/mtd/spi-nor.h> >>>>>>> +#include <linux/spi/spi.h> >>>>>>> >>>>>>> /* Define max times to check status register before we give up. >>>>>>> */ >>>>>>> >>>>>>> @@ -89,15 +90,24 @@ static const struct flash_info >>>>>>> *spi_nor_match_id(const char *name); static int read_sr(struct >>>>>>> spi_nor *nor) { >>>>>>> int ret; >>>>>>> - u8 val; >>>>>>> + u8 val[2]; >>>>>>> >>>>>>> - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); >>>>>>> - if (ret < 0) { >>>>>>> - pr_err("error %d reading SR\n", (int) ret); >>>>>>> - return ret; >>>>>>> + if (nor->stripe) { >>>>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); >>>>>>> + if (ret < 0) { >>>>>>> + pr_err("error %d reading SR\n", (int) ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + val[0] |= val[1]; >>>>>> Why '|' rather than '&' ? >>>>>> I guess because of the 'Write In Progress/Busy' bit: when called by >>>>>> spi_nor_sr_ready(), you want to be sure that this 'BUSY' bit is >>>>>> cleared on both memories. >>>>>> >>>>>> But what about when the Status Register is read for purpose other >>>>>> than checking the state of the 'BUSY' bit? >>>>>> >>>>> Yes you are correct, I will change this. >>>>> >>>>>> What about SPI controllers supporting more than 2 memories in >> parallel? >>>>>> >>>>>> This solution might fit the ZynqMP controller but doesn't look so >> generic. >>>>>> >>>>>>> + } else { >>>>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); >>>>>>> + if (ret < 0) { >>>>>>> + pr_err("error %d reading SR\n", (int) ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> } >>>>>>> >>>>>>> - return val; >>>>>>> + return val[0]; >>>>>>> } >>>>>>> >>>>>>> /* >>>>>>> @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) >>>>>>> static int read_fsr(struct spi_nor *nor) { >>>>>>> int ret; >>>>>>> - u8 val; >>>>>>> + u8 val[2]; >>>>>>> >>>>>>> - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); >>>>>>> - if (ret < 0) { >>>>>>> - pr_err("error %d reading FSR\n", ret); >>>>>>> - return ret; >>>>>>> + if (nor->stripe) { >>>>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); >>>>>>> + if (ret < 0) { >>>>>>> + pr_err("error %d reading FSR\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + val[0] &= val[1]; >>>>>> Same comment here: why '&' rather than '|'? >>>>>> Surely because of the the 'READY' bit which should be set for both >>>> memories. >>>>> I will update this also. >>>>>> >>>>>>> + } else { >>>>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); >>>>>>> + if (ret < 0) { >>>>>>> + pr_err("error %d reading FSR\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> } >>>>>>> >>>>>>> - return val; >>>>>>> + return val[0]; >>>>>>> } >>>>>>> >>>>>>> /* >>>>>>> @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct >>>>>>> spi_nor >>>>>> *nor) >>>>>>> */ >>>>>>> static int erase_chip(struct spi_nor *nor) { >>>>>>> + u32 ret; >>>>>>> + >>>>>>> dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> >>>>>>> 10)); >>>>>>> >>>>>>> - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); >>>>>>> + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); if >>>>>>> + (ret) >>>>>>> + return ret; >>>>>>> + >>>>>>> + return ret; >>>>>>> + >>>>>> >>>>>> if (ret) >>>>>> return ret; >>>>>> else >>>>>> return ret; >>>>>> >>>>>> This chunk should be removed, it doesn't ease the patch review ;) >>>>> Ok, I will remove. >>>>>> >>>>>>> } >>>>>>> >>>>>>> static int spi_nor_lock_and_prep(struct spi_nor *nor, enum >>>>>>> spi_nor_ops ops) @@ -349,7 +375,7 @@ static int >>>>>>> spi_nor_erase_sector(struct spi_nor *nor, u32 addr) static int >>>>>>> spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { >>>>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>>>>>> - u32 addr, len; >>>>>>> + u32 addr, len, offset; >>>>>>> uint32_t rem; >>>>>>> int ret; >>>>>>> >>>>>>> @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info >>>>>>> *mtd, >>>>>> struct erase_info *instr) >>>>>>> /* "sector"-at-a-time erase */ >>>>>>> } else { >>>>>>> while (len) { >>>>>>> + >>>>>>> write_enable(nor); >>>>>>> + offset = addr; >>>>>>> + if (nor->stripe) >>>>>>> + offset /= 2; >>>>>> >>>>>> I guess this should be /= 4 for controllers supporting 4 memories >>>>>> in >>>> parallel. >>>>>> Shouldn't you use nor->shift and define shift as an unsigned int >>>>>> instead of a bool? >>>>>> offset >>= nor->shift; >>>>>> >>>>> Yes we can use this shift, I will update >>>>> >>>>>> Anyway, by tuning the address here in spi-nor.c rather than in the >>>>>> SPI controller driver, you impose a model to support parallel >>>>>> memories that might not be suited to other controllers. >>>>> >>>>> For this ZynqMP GQSPI controller parallel configuration, globally >>>>> spi-nor should know about this stripe feature And based on that >>>>> address >>>> has to change. >>>>> As I mentioned in cover letter, this controller in parallel >>>>> configuration will >>>> work with even addresses only. >>>>> i.e. Before creating address format(m25p_addr2cmd) in mtd layer, >>>>> spi-nor >>>> should change that address based on stripe option. >>>>> >>>>> I am updating this offset based on stripe option, and stripe option >>>>> will >>>> update by reading dt property in nor_scan(). >>>>> So the controller which doesn't support, then the stripe will be zero. >>>>> Or Can you please suggest any other way? >>>>> >>>>>> >>>>>>> >>>>>>> - ret = spi_nor_erase_sector(nor, addr); >>>>>>> + ret = spi_nor_erase_sector(nor, offset); >>>>>>> if (ret) >>>>>>> goto erase_err; >>>>>>> >>>>>>> @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, >>>>>>> loff_t ofs, >>>>>> uint64_t len) >>>>>>> bool use_top; >>>>>>> int ret; >>>>>>> >>>>>>> + ofs = ofs >> nor->shift; >>>>>>> + >>>>>>> status_old = read_sr(nor); >>>>>>> if (status_old < 0) >>>>>>> return status_old; >>>>>>> @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, >>>>>>> loff_t ofs, >>>>>> uint64_t len) >>>>>>> bool use_top; >>>>>>> int ret; >>>>>>> >>>>>>> + ofs = ofs >> nor->shift; >>>>>>> + >>>>>>> status_old = read_sr(nor); >>>>>>> if (status_old < 0) >>>>>>> return status_old; >>>>>>> @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info *mtd, >>>>>>> loff_t >>>>>> ofs, uint64_t len) >>>>>>> if (ret) >>>>>>> return ret; >>>>>>> >>>>>>> + ofs = ofs >> nor->shift; >>>>>>> + >>>>>>> ret = nor->flash_lock(nor, ofs, len); >>>>>>> >>>>>>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); @@ - >>>>>> 724,6 +760,8 >>>>>>> @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, >>>>>>> uint64_t >>>>>> len) >>>>>>> if (ret) >>>>>>> return ret; >>>>>>> >>>>>>> + ofs = ofs >> nor->shift; >>>>>>> + >>>>>>> ret = nor->flash_unlock(nor, ofs, len); >>>>>>> >>>>>>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); @@ - >>>>>> 1018,6 +1056,9 >>>>>>> @@ static const struct flash_info *spi_nor_read_id(struct spi_nor >> *nor) >>>>>>> u8 id[SPI_NOR_MAX_ID_LEN]; >>>>>>> const struct flash_info *info; >>>>>>> >>>>>>> + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | >>>>>>> + SPI_MASTER_DATA_STRIPE); >>>>>>> + >>>>>>> tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, >>>>>> SPI_NOR_MAX_ID_LEN); >>>>>>> if (tmp < 0) { >>>>>>> dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); >>>>>> @@ -1041,6 >>>>>>> +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t >>>>>>> +from, >>>>>>> size_t len, { >>>>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>>>>>> int ret; >>>>>>> + u32 offset = from; >>>>>>> >>>>>>> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); >>>>>>> >>>>>>> @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info >>>>>>> *mtd, >>>>>> loff_t from, size_t len, >>>>>>> return ret; >>>>>>> >>>>>>> while (len) { >>>>>>> - ret = nor->read(nor, from, len, buf); >>>>>>> + >>>>>>> + offset = from; >>>>>>> + >>>>>>> + if (nor->stripe) >>>>>>> + offset /= 2; >>>>>>> + >>>>>>> + ret = nor->read(nor, offset, len, buf); >>>>>>> if (ret == 0) { >>>>>>> /* We shouldn't see 0-length reads */ >>>>>>> ret = -EIO; >>>>>>> @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info >>>>>>> *mtd, >>>>>> loff_t to, size_t len, >>>>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>>>>>> size_t page_offset, page_remain, i; >>>>>>> ssize_t ret; >>>>>>> + u32 offset; >>>>>>> >>>>>>> dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); >>>>>>> >>>>>>> @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info >>>>>>> *mtd, >>>>>> loff_t to, size_t len, >>>>>>> /* the size of data remaining on the first page */ >>>>>>> page_remain = min_t(size_t, >>>>>>> nor->page_size - page_offset, len - >>>>>>> i); >>>>>>> + offset = (to + i); >>>>>>> + >>>>>>> + if (nor->stripe) >>>>>>> + offset /= 2; >>>>>>> >>>>>>> write_enable(nor); >>>>>>> - ret = nor->write(nor, to + i, page_remain, buf + i); >>>>>>> + ret = nor->write(nor, (offset), page_remain, buf + i); >>>>>>> if (ret < 0) >>>>>>> goto write_err; >>>>>>> written = ret; >>>>>>> @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor >>>>>>> *nor) >>>>>>> >>>>>>> int spi_nor_scan(struct spi_nor *nor, const char *name, enum >>>>>>> read_mode mode) { >>>>>>> - const struct flash_info *info = NULL; >>>>>>> + struct flash_info *info = NULL; >>>>>> >>>>>> You should not remove the const and should not try to modify >>>>>> members of *info. >>>>>> >>>>>>> struct device *dev = nor->dev; >>>>>>> struct mtd_info *mtd = &nor->mtd; >>>>>>> struct device_node *np = spi_nor_get_flash_node(nor); >>>>>>> - int ret; >>>>>>> - int i; >>>>>>> + struct device_node *np_spi; >>>>>>> + int ret, i, xlnx_qspi_mode; >>>>>>> >>>>>>> ret = spi_nor_check(nor); >>>>>>> if (ret) >>>>>>> return ret; >>>>>>> >>>>>>> if (name) >>>>>>> - info = spi_nor_match_id(name); >>>>>>> + info = (struct flash_info *)spi_nor_match_id(name); >>>>>>> /* Try to auto-detect if chip name wasn't specified or not found */ >>>>>>> if (!info) >>>>>>> - info = spi_nor_read_id(nor); >>>>>>> + info = (struct flash_info *)spi_nor_read_id(nor); >>>>>>> if (IS_ERR_OR_NULL(info)) >>>>>>> return -ENOENT; >>>>>>> >>>>>> Both spi_nor_match_id() and spi_nor_read_id(), when they succeed, >>>>>> return a pointer to an entry of the spi_nor_ids[] array, which is >>>>>> located in a read- only memory area. >>>>>> >>>>>> Since your patch doesn't remove the const attribute of the >>>>>> spi_nor_ids[], I wonder whether it has been tested. I expect it not >>>>>> to work on most architecture. >>>>>> >>>>>> Anyway spi_nor_ids[] should remain const. Let's take the case of >>>>>> eXecution In Place (XIP) from an external memory: if spi_nor_ids[] >>>>>> is const, it can be read directly from this external (read-only) >>>>>> memory and we never need to copy the array in RAM, so we save >> some >>>>>> KB of >>>> RAM. >>>>>> This is just an example but I guess we can find other reasons to >>>>>> keep this array const. >>>>>> >>>>>>> @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, const >>>>>>> char >>>>>> *name, enum read_mode mode) >>>>>>> */ >>>>>>> dev_warn(dev, "found %s, expected %s\n", >>>>>>> jinfo->name, info->name); >>>>>>> - info = jinfo; >>>>>>> + info = (struct flash_info *)jinfo; >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, const >>>>>>> char >>>>>> *name, enum read_mode mode) >>>>>>> mtd->size = info->sector_size * info->n_sectors; >>>>>>> mtd->_erase = spi_nor_erase; >>>>>>> mtd->_read = spi_nor_read; >>>>>>> +#ifdef CONFIG_OF >>>>>>> + np_spi = of_get_next_parent(np); >>>>>>> + >>>>>>> + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", >>>>>>> + &xlnx_qspi_mode) < 0) { >>>>>> This really looks controller specific so should not be placed in >>>>>> the generic spi- nor.c file. >>>>> >>>>> Const is removed in info struct, because to update info members >>>>> based >>>> parallel configuration. >>>>> As I mentioned above, for this parallel configuration, mtd and >>>>> spi-nor should know the details like >>>>> mtd->size, info->sectors, sector_size and page_size. >>>> >>>> You can tune the values of nor->mtd.size, nor->mtd.erasesize, nor- >>>>> page_size or whatever member of nor/nor.mtd as needed without ever >>>> modifying members of *info. >>>> >>>> If you modify *info then spi_nor_scan() is called a 2nd time to probe >>>> and configure SPI memories of the same part but connected to another >>>> controller, the values of the modified members in this *info would >>>> not be those expected. >>>> So *info and the spi_nor_ids[] array must remain const. >>>> >>>> The *info structure is not used outside spi_nor_scan(); none of >>>> spi_nor_read(), >>>> spi_nor_write() and spi_nor_erase() refers to *info hence every >>>> relevant value can be set only nor or nor->mtd members. >>>> >>>> >>>> Anyway, I think OR'ing or AND'ing values of memory registers >>>> depending on the relevant bit we want to check is not the right solution. >>>> If done in spi-nor.c, there would be a specific case for each memory >>>> register we read, each register bit would have to be handled differently. >>>> >>>> spi-nor.c tries to support as much memory parts as possible, it deals >>>> with many registers and bits: Status/Control registers, Quad Enable bits... >>>> >>>> If we start to OR or AND each of these register values to support the >>>> stripping mode, spi-nor will become really hard to maintain. >>>> >>>> I don't know whether it could be done with the xilinx controller but >>>> I thought about first configuring the two memories independently >>>> calling >>>> spi_nor_scan() twice; one call for each memory. >>>> >>>> Then the xilinx driver could register only one struct mtd_info, >>>> overriding >>>> mtd->_read() [and likely mtd->_write() and mtd->_erase() too] set by >>>> spi_nor_scan() with a xilinx driver custom implementation so this >>>> driver supports its controller stripping mode as it wants. >>>> >>>> Of course, this solution assumes that the SPI controller has one >>>> dedicated chip-select line for each memory and not a single >>>> chip-select line shared by both memories. The memories should be >>>> configured independently: you can't assume multiple instances of the >>>> very same memory part always return the exact same value when reading >>>> one of their register. Logical AND/OR is not a generic solution, IMHO. >>>> >>>> If the xilinx controller has only one shared chip-select line then >>>> let's see whether 2 GPIO lines could be used as independent chip-select >> lines. >>>> >>>> >>> In parallel configuration, Physically we have two flashes but mtd will >>> see as single flash memory (sum of both memories) If we call >> spi_nor_scan(), twice then read/write will override but nor->mtd.size, nor- >>> mtd.erasesize, nor->page_size will remain same, I,e they will also override, >> they won't append. >>> I tried calling spi_nor_scan() twice by some hacks but nor->mtd.size, >>> nor->mtd.erasesize, nor->page_size are not changing Also the same issue >> we are getting for flash address, need to shift the address to work in this >> configuration. >>> Also to tune nor->mtd.size, nor->mtd.erasesize, nor->page_size, we >>> need to touch this spi-nor.c >>> >>> Please kindly suggest, if I am wrong. >>> >> >> What I've been suggesting is: >> >> { >> struct spi_nor *nor1, *nor2; >> struct mtd_info *mtd; >> enum read_mode mode = SPI_NOR_QUAD; >> int err; >> >> [...] >> >> err = spi_nor_scan(nor1, NULL, mode); >> if (err) >> return err; /* or handle error properly. */ >> >> err = spi_nor_scan(nor2, NULL, mode); >> if (err) >> return err; >> >> mtd = &nor1->mtd; >> mtd->erasesize <<= 1; >> mtd->size <<= 1; >> mtd->writebufsize <<= 1; >> nor1->page_size <<= 1; >> /* tune all other relevant members of nor1/mtd. */ >> >> /* override relevant mtd hooks. */ >> mtd->_read = stripping_read; >> mtd->_erase = stripping_erase; >> mtd->_write = stripping_write; >> mtd->_lock = ...; >> mtd->_unlock = ...; >> mtd->_is_lock = ...; >> >> /* register a single mtd_info structure. */ >> err = mtd_device_register(mtd, NULL, 0); >> if (err) >> return err; >> >> [...] >> } >> > > It's really good for us to have our controller specific mtd hooks instead of changing the layer calls and thanks for this suggestion. > But spi-zynqmp-gqspi.c is spi driver and all above mentioned parameters and function pointers are related to flash layer. > So is it ok to update and change flash related stuff in our spi driver? > Check with the SPI sub-system people, especially Mark Brown, but I don't think would be good to put too much mtd/spi-nor stuff in the SPI sub-system. Anyway, the solution I've proposed is not suited if you use m25p80.c as an adaptation layer between spi-nor.c and the SPI controller driver. Indeed, in that case, spi_nor_scan() is called from m25p_probe() in m25p80.c and I still think there would be too many side effects if we modified either spi-nor.c or m25p80.c the way you propose to compute logical operations on register values. This solution is not maintainable regarding the number memory registers we already manage. You might need to develop a new driver to substitute m25p80.c and make the link between spi-nor, mostly spi_nor_scan(), and you SPI controller driver. Honestly I have no real idea how you could proceed to add support to such a feature: accessing 2 SPI flashes at the time to perform stripping operations doesn't sound really reliable to me because I don't see how you plan to handle errors properly and also cover all the features provided by SPI flash memory and supported by spi-nor.c (sector/block protection, ...). >> Best regards, >> >> Cyrille >> > Thanks, > Naga Sureshkumar Relli > > > This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately. > >
Hi Mark and Cyrille, > -----Original Message----- > From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] > Sent: Tuesday, December 06, 2016 4:30 PM > To: Naga Sureshkumar Relli <nagasure@xilinx.com>; broonie@kernel.org; > michal.simek@xilinx.com; Soren Brinkmann <sorenb@xilinx.com>; Harini > Katakam <harinik@xilinx.com>; Punnaiah Choudary Kalluri > <punnaia@xilinx.com> > Cc: linux-spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; linux-mtd@lists.infradead.org > Subject: Re: [LINUX RFC v4 3/4] mtd: spi-nor: add stripe support > > Le 06/12/2016 à 07:54, Naga Sureshkumar Relli a écrit : > > Hi Cyrille, > > > >> -----Original Message----- > >> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] > >> Sent: Monday, December 05, 2016 6:34 PM > >> To: Naga Sureshkumar Relli <nagasure@xilinx.com>; broonie@kernel.org; > >> michal.simek@xilinx.com; Soren Brinkmann <sorenb@xilinx.com>; Harini > >> Katakam <harinik@xilinx.com>; Punnaiah Choudary Kalluri > >> <punnaia@xilinx.com> > >> Cc: linux-spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >> linux- kernel@vger.kernel.org; linux-mtd@lists.infradead.org > >> Subject: Re: [LINUX RFC v4 3/4] mtd: spi-nor: add stripe support > >> > >> Hi Naga, > >> > >> Le 05/12/2016 à 08:02, Naga Sureshkumar Relli a écrit : > >>> Hi Cyrille, > >>> > >>>>> Hi Cyrille, > >>>>> > >>>>>> I have not finished to review the whole series yet but here some > >>>>>> first > >>>>>> comments: > >>>>> > >>>>> Thanks for reviewing these patch series. > >>>>> > >>>>>> > >>>>>> Le 27/11/2016 à 09:33, Naga Sureshkumar Relli a écrit : > >>>>>>> This patch adds stripe support and it is needed for GQSPI > >>>>>>> parallel configuration mode by: > >>>>>>> > >>>>>>> - Adding required parameters like stripe and shift to spi_nor > >>>>>>> structure. > >>>>>>> - Initializing all added parameters in spi_nor_scan() > >>>>>>> - Updating read_sr() and read_fsr() for getting status from both > >>>>>>> flashes > >>>>>>> - Increasing page_size, sector_size, erase_size and toatal flash > >>>>>>> size as and when required. > >>>>>>> - Dividing address by 2 > >>>>>>> - Updating spi->master->flags for qspi driver to change CS > >>>>>>> > >>>>>>> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> > >>>>>>> --- > >>>>>>> Changes for v4: > >>>>>>> - rename isparallel to stripe > >>>>>>> Changes for v3: > >>>>>>> - No change > >>>>>>> Changes for v2: > >>>>>>> - Splitted to separate MTD layer changes from SPI core changes > >>>>>>> --- > >>>>>>> drivers/mtd/spi-nor/spi-nor.c | 130 > >>>>>> ++++++++++++++++++++++++++++++++---------- > >>>>>>> include/linux/mtd/spi-nor.h | 2 + > >>>>>>> 2 files changed, 103 insertions(+), 29 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c > >>>>>>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..4252239 100644 > >>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c > >>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c > >>>>>>> @@ -22,6 +22,7 @@ > >>>>>>> #include <linux/of_platform.h> > >>>>>>> #include <linux/spi/flash.h> > >>>>>>> #include <linux/mtd/spi-nor.h> > >>>>>>> +#include <linux/spi/spi.h> > >>>>>>> > >>>>>>> /* Define max times to check status register before we give up. > >>>>>>> */ > >>>>>>> > >>>>>>> @@ -89,15 +90,24 @@ static const struct flash_info > >>>>>>> *spi_nor_match_id(const char *name); static int read_sr(struct > >>>>>>> spi_nor *nor) { > >>>>>>> int ret; > >>>>>>> - u8 val; > >>>>>>> + u8 val[2]; > >>>>>>> > >>>>>>> - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); > >>>>>>> - if (ret < 0) { > >>>>>>> - pr_err("error %d reading SR\n", (int) ret); > >>>>>>> - return ret; > >>>>>>> + if (nor->stripe) { > >>>>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); > >>>>>>> + if (ret < 0) { > >>>>>>> + pr_err("error %d reading SR\n", (int) ret); > >>>>>>> + return ret; > >>>>>>> + } > >>>>>>> + val[0] |= val[1]; > >>>>>> Why '|' rather than '&' ? > >>>>>> I guess because of the 'Write In Progress/Busy' bit: when called > >>>>>> by spi_nor_sr_ready(), you want to be sure that this 'BUSY' bit > >>>>>> is cleared on both memories. > >>>>>> > >>>>>> But what about when the Status Register is read for purpose other > >>>>>> than checking the state of the 'BUSY' bit? > >>>>>> > >>>>> Yes you are correct, I will change this. > >>>>> > >>>>>> What about SPI controllers supporting more than 2 memories in > >> parallel? > >>>>>> > >>>>>> This solution might fit the ZynqMP controller but doesn't look so > >> generic. > >>>>>> > >>>>>>> + } else { > >>>>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); > >>>>>>> + if (ret < 0) { > >>>>>>> + pr_err("error %d reading SR\n", (int) ret); > >>>>>>> + return ret; > >>>>>>> + } > >>>>>>> } > >>>>>>> > >>>>>>> - return val; > >>>>>>> + return val[0]; > >>>>>>> } > >>>>>>> > >>>>>>> /* > >>>>>>> @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) > >>>>>>> static int read_fsr(struct spi_nor *nor) { > >>>>>>> int ret; > >>>>>>> - u8 val; > >>>>>>> + u8 val[2]; > >>>>>>> > >>>>>>> - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); > >>>>>>> - if (ret < 0) { > >>>>>>> - pr_err("error %d reading FSR\n", ret); > >>>>>>> - return ret; > >>>>>>> + if (nor->stripe) { > >>>>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); > >>>>>>> + if (ret < 0) { > >>>>>>> + pr_err("error %d reading FSR\n", ret); > >>>>>>> + return ret; > >>>>>>> + } > >>>>>>> + val[0] &= val[1]; > >>>>>> Same comment here: why '&' rather than '|'? > >>>>>> Surely because of the the 'READY' bit which should be set for > >>>>>> both > >>>> memories. > >>>>> I will update this also. > >>>>>> > >>>>>>> + } else { > >>>>>>> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); > >>>>>>> + if (ret < 0) { > >>>>>>> + pr_err("error %d reading FSR\n", ret); > >>>>>>> + return ret; > >>>>>>> + } > >>>>>>> } > >>>>>>> > >>>>>>> - return val; > >>>>>>> + return val[0]; > >>>>>>> } > >>>>>>> > >>>>>>> /* > >>>>>>> @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct > >>>>>>> spi_nor > >>>>>> *nor) > >>>>>>> */ > >>>>>>> static int erase_chip(struct spi_nor *nor) { > >>>>>>> + u32 ret; > >>>>>>> + > >>>>>>> dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> > >>>>>>> 10)); > >>>>>>> > >>>>>>> - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); > >>>>>>> + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); if > >>>>>>> + (ret) > >>>>>>> + return ret; > >>>>>>> + > >>>>>>> + return ret; > >>>>>>> + > >>>>>> > >>>>>> if (ret) > >>>>>> return ret; > >>>>>> else > >>>>>> return ret; > >>>>>> > >>>>>> This chunk should be removed, it doesn't ease the patch review ;) > >>>>> Ok, I will remove. > >>>>>> > >>>>>>> } > >>>>>>> > >>>>>>> static int spi_nor_lock_and_prep(struct spi_nor *nor, enum > >>>>>>> spi_nor_ops ops) @@ -349,7 +375,7 @@ static int > >>>>>>> spi_nor_erase_sector(struct spi_nor *nor, u32 addr) static int > >>>>>>> spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { > >>>>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>>>>>> - u32 addr, len; > >>>>>>> + u32 addr, len, offset; > >>>>>>> uint32_t rem; > >>>>>>> int ret; > >>>>>>> > >>>>>>> @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info > >>>>>>> *mtd, > >>>>>> struct erase_info *instr) > >>>>>>> /* "sector"-at-a-time erase */ > >>>>>>> } else { > >>>>>>> while (len) { > >>>>>>> + > >>>>>>> write_enable(nor); > >>>>>>> + offset = addr; > >>>>>>> + if (nor->stripe) > >>>>>>> + offset /= 2; > >>>>>> > >>>>>> I guess this should be /= 4 for controllers supporting 4 memories > >>>>>> in > >>>> parallel. > >>>>>> Shouldn't you use nor->shift and define shift as an unsigned int > >>>>>> instead of a bool? > >>>>>> offset >>= nor->shift; > >>>>>> > >>>>> Yes we can use this shift, I will update > >>>>> > >>>>>> Anyway, by tuning the address here in spi-nor.c rather than in > >>>>>> the SPI controller driver, you impose a model to support parallel > >>>>>> memories that might not be suited to other controllers. > >>>>> > >>>>> For this ZynqMP GQSPI controller parallel configuration, globally > >>>>> spi-nor should know about this stripe feature And based on that > >>>>> address > >>>> has to change. > >>>>> As I mentioned in cover letter, this controller in parallel > >>>>> configuration will > >>>> work with even addresses only. > >>>>> i.e. Before creating address format(m25p_addr2cmd) in mtd layer, > >>>>> spi-nor > >>>> should change that address based on stripe option. > >>>>> > >>>>> I am updating this offset based on stripe option, and stripe > >>>>> option will > >>>> update by reading dt property in nor_scan(). > >>>>> So the controller which doesn't support, then the stripe will be zero. > >>>>> Or Can you please suggest any other way? > >>>>> > >>>>>> > >>>>>>> > >>>>>>> - ret = spi_nor_erase_sector(nor, addr); > >>>>>>> + ret = spi_nor_erase_sector(nor, offset); > >>>>>>> if (ret) > >>>>>>> goto erase_err; > >>>>>>> > >>>>>>> @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, > >>>>>>> loff_t ofs, > >>>>>> uint64_t len) > >>>>>>> bool use_top; > >>>>>>> int ret; > >>>>>>> > >>>>>>> + ofs = ofs >> nor->shift; > >>>>>>> + > >>>>>>> status_old = read_sr(nor); > >>>>>>> if (status_old < 0) > >>>>>>> return status_old; > >>>>>>> @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, > >>>>>>> loff_t ofs, > >>>>>> uint64_t len) > >>>>>>> bool use_top; > >>>>>>> int ret; > >>>>>>> > >>>>>>> + ofs = ofs >> nor->shift; > >>>>>>> + > >>>>>>> status_old = read_sr(nor); > >>>>>>> if (status_old < 0) > >>>>>>> return status_old; > >>>>>>> @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info > >>>>>>> *mtd, loff_t > >>>>>> ofs, uint64_t len) > >>>>>>> if (ret) > >>>>>>> return ret; > >>>>>>> > >>>>>>> + ofs = ofs >> nor->shift; > >>>>>>> + > >>>>>>> ret = nor->flash_lock(nor, ofs, len); > >>>>>>> > >>>>>>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); @@ - > >>>>>> 724,6 +760,8 > >>>>>>> @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, > >>>>>>> uint64_t > >>>>>> len) > >>>>>>> if (ret) > >>>>>>> return ret; > >>>>>>> > >>>>>>> + ofs = ofs >> nor->shift; > >>>>>>> + > >>>>>>> ret = nor->flash_unlock(nor, ofs, len); > >>>>>>> > >>>>>>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); @@ - > >>>>>> 1018,6 +1056,9 > >>>>>>> @@ static const struct flash_info *spi_nor_read_id(struct > >>>>>>> spi_nor > >> *nor) > >>>>>>> u8 id[SPI_NOR_MAX_ID_LEN]; > >>>>>>> const struct flash_info *info; > >>>>>>> > >>>>>>> + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | > >>>>>>> + SPI_MASTER_DATA_STRIPE); > >>>>>>> + > >>>>>>> tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, > >>>>>> SPI_NOR_MAX_ID_LEN); > >>>>>>> if (tmp < 0) { > >>>>>>> dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); > >>>>>> @@ -1041,6 > >>>>>>> +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t > >>>>>>> +from, > >>>>>>> size_t len, { > >>>>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>>>>>> int ret; > >>>>>>> + u32 offset = from; > >>>>>>> > >>>>>>> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > >>>>>>> > >>>>>>> @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info > >>>>>>> *mtd, > >>>>>> loff_t from, size_t len, > >>>>>>> return ret; > >>>>>>> > >>>>>>> while (len) { > >>>>>>> - ret = nor->read(nor, from, len, buf); > >>>>>>> + > >>>>>>> + offset = from; > >>>>>>> + > >>>>>>> + if (nor->stripe) > >>>>>>> + offset /= 2; > >>>>>>> + > >>>>>>> + ret = nor->read(nor, offset, len, buf); > >>>>>>> if (ret == 0) { > >>>>>>> /* We shouldn't see 0-length reads */ > >>>>>>> ret = -EIO; > >>>>>>> @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info > >>>>>>> *mtd, > >>>>>> loff_t to, size_t len, > >>>>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd); > >>>>>>> size_t page_offset, page_remain, i; > >>>>>>> ssize_t ret; > >>>>>>> + u32 offset; > >>>>>>> > >>>>>>> dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); > >>>>>>> > >>>>>>> @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info > >>>>>>> *mtd, > >>>>>> loff_t to, size_t len, > >>>>>>> /* the size of data remaining on the first page */ > >>>>>>> page_remain = min_t(size_t, > >>>>>>> nor->page_size - page_offset, len > >>>>>>> - i); > >>>>>>> + offset = (to + i); > >>>>>>> + > >>>>>>> + if (nor->stripe) > >>>>>>> + offset /= 2; > >>>>>>> > >>>>>>> write_enable(nor); > >>>>>>> - ret = nor->write(nor, to + i, page_remain, buf + i); > >>>>>>> + ret = nor->write(nor, (offset), page_remain, buf + i); > >>>>>>> if (ret < 0) > >>>>>>> goto write_err; > >>>>>>> written = ret; > >>>>>>> @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor > >>>>>>> *nor) > >>>>>>> > >>>>>>> int spi_nor_scan(struct spi_nor *nor, const char *name, enum > >>>>>>> read_mode mode) { > >>>>>>> - const struct flash_info *info = NULL; > >>>>>>> + struct flash_info *info = NULL; > >>>>>> > >>>>>> You should not remove the const and should not try to modify > >>>>>> members of *info. > >>>>>> > >>>>>>> struct device *dev = nor->dev; > >>>>>>> struct mtd_info *mtd = &nor->mtd; > >>>>>>> struct device_node *np = spi_nor_get_flash_node(nor); > >>>>>>> - int ret; > >>>>>>> - int i; > >>>>>>> + struct device_node *np_spi; > >>>>>>> + int ret, i, xlnx_qspi_mode; > >>>>>>> > >>>>>>> ret = spi_nor_check(nor); > >>>>>>> if (ret) > >>>>>>> return ret; > >>>>>>> > >>>>>>> if (name) > >>>>>>> - info = spi_nor_match_id(name); > >>>>>>> + info = (struct flash_info *)spi_nor_match_id(name); > >>>>>>> /* Try to auto-detect if chip name wasn't specified or not found */ > >>>>>>> if (!info) > >>>>>>> - info = spi_nor_read_id(nor); > >>>>>>> + info = (struct flash_info *)spi_nor_read_id(nor); > >>>>>>> if (IS_ERR_OR_NULL(info)) > >>>>>>> return -ENOENT; > >>>>>>> > >>>>>> Both spi_nor_match_id() and spi_nor_read_id(), when they > succeed, > >>>>>> return a pointer to an entry of the spi_nor_ids[] array, which is > >>>>>> located in a read- only memory area. > >>>>>> > >>>>>> Since your patch doesn't remove the const attribute of the > >>>>>> spi_nor_ids[], I wonder whether it has been tested. I expect it > >>>>>> not to work on most architecture. > >>>>>> > >>>>>> Anyway spi_nor_ids[] should remain const. Let's take the case of > >>>>>> eXecution In Place (XIP) from an external memory: if > >>>>>> spi_nor_ids[] is const, it can be read directly from this > >>>>>> external (read-only) memory and we never need to copy the array > >>>>>> in RAM, so we save > >> some > >>>>>> KB of > >>>> RAM. > >>>>>> This is just an example but I guess we can find other reasons to > >>>>>> keep this array const. > >>>>>> > >>>>>>> @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, > >>>>>>> const char > >>>>>> *name, enum read_mode mode) > >>>>>>> */ > >>>>>>> dev_warn(dev, "found %s, expected %s\n", > >>>>>>> jinfo->name, info->name); > >>>>>>> - info = jinfo; > >>>>>>> + info = (struct flash_info *)jinfo; > >>>>>>> } > >>>>>>> } > >>>>>>> > >>>>>>> @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, > >>>>>>> const char > >>>>>> *name, enum read_mode mode) > >>>>>>> mtd->size = info->sector_size * info->n_sectors; > >>>>>>> mtd->_erase = spi_nor_erase; > >>>>>>> mtd->_read = spi_nor_read; > >>>>>>> +#ifdef CONFIG_OF > >>>>>>> + np_spi = of_get_next_parent(np); > >>>>>>> + > >>>>>>> + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", > >>>>>>> + &xlnx_qspi_mode) < 0) { > >>>>>> This really looks controller specific so should not be placed in > >>>>>> the generic spi- nor.c file. > >>>>> > >>>>> Const is removed in info struct, because to update info members > >>>>> based > >>>> parallel configuration. > >>>>> As I mentioned above, for this parallel configuration, mtd and > >>>>> spi-nor should know the details like > >>>>> mtd->size, info->sectors, sector_size and page_size. > >>>> > >>>> You can tune the values of nor->mtd.size, nor->mtd.erasesize, nor- > >>>>> page_size or whatever member of nor/nor.mtd as needed without > ever > >>>> modifying members of *info. > >>>> > >>>> If you modify *info then spi_nor_scan() is called a 2nd time to > >>>> probe and configure SPI memories of the same part but connected to > >>>> another controller, the values of the modified members in this > >>>> *info would not be those expected. > >>>> So *info and the spi_nor_ids[] array must remain const. > >>>> > >>>> The *info structure is not used outside spi_nor_scan(); none of > >>>> spi_nor_read(), > >>>> spi_nor_write() and spi_nor_erase() refers to *info hence every > >>>> relevant value can be set only nor or nor->mtd members. > >>>> > >>>> > >>>> Anyway, I think OR'ing or AND'ing values of memory registers > >>>> depending on the relevant bit we want to check is not the right > solution. > >>>> If done in spi-nor.c, there would be a specific case for each > >>>> memory register we read, each register bit would have to be handled > differently. > >>>> > >>>> spi-nor.c tries to support as much memory parts as possible, it > >>>> deals with many registers and bits: Status/Control registers, Quad > Enable bits... > >>>> > >>>> If we start to OR or AND each of these register values to support > >>>> the stripping mode, spi-nor will become really hard to maintain. > >>>> > >>>> I don't know whether it could be done with the xilinx controller > >>>> but I thought about first configuring the two memories > >>>> independently calling > >>>> spi_nor_scan() twice; one call for each memory. > >>>> > >>>> Then the xilinx driver could register only one struct mtd_info, > >>>> overriding > >>>> mtd->_read() [and likely mtd->_write() and mtd->_erase() too] set > >>>> mtd->by > >>>> spi_nor_scan() with a xilinx driver custom implementation so this > >>>> driver supports its controller stripping mode as it wants. > >>>> > >>>> Of course, this solution assumes that the SPI controller has one > >>>> dedicated chip-select line for each memory and not a single > >>>> chip-select line shared by both memories. The memories should be > >>>> configured independently: you can't assume multiple instances of > >>>> the very same memory part always return the exact same value when > >>>> reading one of their register. Logical AND/OR is not a generic solution, > IMHO. > >>>> > >>>> If the xilinx controller has only one shared chip-select line then > >>>> let's see whether 2 GPIO lines could be used as independent > >>>> chip-select > >> lines. > >>>> > >>>> > >>> In parallel configuration, Physically we have two flashes but mtd > >>> will see as single flash memory (sum of both memories) If we call > >> spi_nor_scan(), twice then read/write will override but > >> nor->mtd.size, nor- > >>> mtd.erasesize, nor->page_size will remain same, I,e they will also > >>> override, > >> they won't append. > >>> I tried calling spi_nor_scan() twice by some hacks but > >>> nor->mtd.size, > >>> nor->mtd.erasesize, nor->page_size are not changing Also the same > >>> nor->issue > >> we are getting for flash address, need to shift the address to work > >> in this configuration. > >>> Also to tune nor->mtd.size, nor->mtd.erasesize, nor->page_size, we > >>> need to touch this spi-nor.c > >>> > >>> Please kindly suggest, if I am wrong. > >>> > >> > >> What I've been suggesting is: > >> > >> { > >> struct spi_nor *nor1, *nor2; > >> struct mtd_info *mtd; > >> enum read_mode mode = SPI_NOR_QUAD; > >> int err; > >> > >> [...] > >> > >> err = spi_nor_scan(nor1, NULL, mode); > >> if (err) > >> return err; /* or handle error properly. */ > >> > >> err = spi_nor_scan(nor2, NULL, mode); > >> if (err) > >> return err; > >> > >> mtd = &nor1->mtd; > >> mtd->erasesize <<= 1; > >> mtd->size <<= 1; > >> mtd->writebufsize <<= 1; > >> nor1->page_size <<= 1; > >> /* tune all other relevant members of nor1/mtd. */ > >> > >> /* override relevant mtd hooks. */ > >> mtd->_read = stripping_read; > >> mtd->_erase = stripping_erase; > >> mtd->_write = stripping_write; > >> mtd->_lock = ...; > >> mtd->_unlock = ...; > >> mtd->_is_lock = ...; > >> > >> /* register a single mtd_info structure. */ > >> err = mtd_device_register(mtd, NULL, 0); > >> if (err) > >> return err; > >> > >> [...] > >> } > >> > > > > It's really good for us to have our controller specific mtd hooks instead of > changing the layer calls and thanks for this suggestion. > > But spi-zynqmp-gqspi.c is spi driver and all above mentioned parameters > and function pointers are related to flash layer. > > So is it ok to update and change flash related stuff in our spi driver? > > > > Check with the SPI sub-system people, especially Mark Brown, but I don't > think would be good to put too much mtd/spi-nor stuff in the SPI sub- > system. > > Anyway, the solution I've proposed is not suited if you use m25p80.c as an > adaptation layer between spi-nor.c and the SPI controller driver. > Indeed, in that case, spi_nor_scan() is called from m25p_probe() in m25p80.c > and I still think there would be too many side effects if we modified either > spi-nor.c or m25p80.c the way you propose to compute logical operations on > register values. This solution is not maintainable regarding the number > memory registers we already manage. > > You might need to develop a new driver to substitute m25p80.c and make > the link between spi-nor, mostly spi_nor_scan(), and you SPI controller > driver. > > Honestly I have no real idea how you could proceed to add support to such a > feature: accessing 2 SPI flashes at the time to perform stripping operations > doesn't sound really reliable to me because I don't see how you plan to > handle errors properly and also cover all the features provided by SPI flash > memory and supported by spi-nor.c (sector/block protection, ...). > > >> Best regards, > >> > >> Cyrille > >> > > Thanks, > > Naga Sureshkumar Relli Cyrille, thanks for info. Mark, Could you please provide a way to add this stripe support for GQSPI controller? We already sent patches to add this support by changing mtd layer, as we don't have any other way to do this Because of controller limitation. Cyrille provided some way to do this, but it involves spi-nor support in our spi driver. Thanks, Naga Sureshkumar Relli This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..4252239 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -22,6 +22,7 @@ #include <linux/of_platform.h> #include <linux/spi/flash.h> #include <linux/mtd/spi-nor.h> +#include <linux/spi/spi.h> /* Define max times to check status register before we give up. */ @@ -89,15 +90,24 @@ static const struct flash_info *spi_nor_match_id(const char *name); static int read_sr(struct spi_nor *nor) { int ret; - u8 val; + u8 val[2]; - ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1); - if (ret < 0) { - pr_err("error %d reading SR\n", (int) ret); - return ret; + if (nor->stripe) { + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 2); + if (ret < 0) { + pr_err("error %d reading SR\n", (int) ret); + return ret; + } + val[0] |= val[1]; + } else { + ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val[0], 1); + if (ret < 0) { + pr_err("error %d reading SR\n", (int) ret); + return ret; + } } - return val; + return val[0]; } /* @@ -108,15 +118,24 @@ static int read_sr(struct spi_nor *nor) static int read_fsr(struct spi_nor *nor) { int ret; - u8 val; + u8 val[2]; - ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1); - if (ret < 0) { - pr_err("error %d reading FSR\n", ret); - return ret; + if (nor->stripe) { + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 2); + if (ret < 0) { + pr_err("error %d reading FSR\n", ret); + return ret; + } + val[0] &= val[1]; + } else { + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val[0], 1); + if (ret < 0) { + pr_err("error %d reading FSR\n", ret); + return ret; + } } - return val; + return val[0]; } /* @@ -290,9 +309,16 @@ static int spi_nor_wait_till_ready(struct spi_nor *nor) */ static int erase_chip(struct spi_nor *nor) { + u32 ret; + dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> 10)); - return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); + ret = nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0); + if (ret) + return ret; + + return ret; + } static int spi_nor_lock_and_prep(struct spi_nor *nor, enum spi_nor_ops ops) @@ -349,7 +375,7 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { struct spi_nor *nor = mtd_to_spi_nor(mtd); - u32 addr, len; + u32 addr, len, offset; uint32_t rem; int ret; @@ -399,9 +425,13 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) /* "sector"-at-a-time erase */ } else { while (len) { + write_enable(nor); + offset = addr; + if (nor->stripe) + offset /= 2; - ret = spi_nor_erase_sector(nor, addr); + ret = spi_nor_erase_sector(nor, offset); if (ret) goto erase_err; @@ -525,6 +555,8 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) bool use_top; int ret; + ofs = ofs >> nor->shift; + status_old = read_sr(nor); if (status_old < 0) return status_old; @@ -610,6 +642,8 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) bool use_top; int ret; + ofs = ofs >> nor->shift; + status_old = read_sr(nor); if (status_old < 0) return status_old; @@ -709,6 +743,8 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) if (ret) return ret; + ofs = ofs >> nor->shift; + ret = nor->flash_lock(nor, ofs, len); spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); @@ -724,6 +760,8 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) if (ret) return ret; + ofs = ofs >> nor->shift; + ret = nor->flash_unlock(nor, ofs, len); spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); @@ -1018,6 +1056,9 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) u8 id[SPI_NOR_MAX_ID_LEN]; const struct flash_info *info; + nor->spi->master->flags &= ~(SPI_MASTER_BOTH_CS | + SPI_MASTER_DATA_STRIPE); + tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN); if (tmp < 0) { dev_dbg(nor->dev, "error %d reading JEDEC ID\n", tmp); @@ -1041,6 +1082,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, { struct spi_nor *nor = mtd_to_spi_nor(mtd); int ret; + u32 offset = from; dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); @@ -1049,7 +1091,13 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, return ret; while (len) { - ret = nor->read(nor, from, len, buf); + + offset = from; + + if (nor->stripe) + offset /= 2; + + ret = nor->read(nor, offset, len, buf); if (ret == 0) { /* We shouldn't see 0-length reads */ ret = -EIO; @@ -1161,6 +1209,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, struct spi_nor *nor = mtd_to_spi_nor(mtd); size_t page_offset, page_remain, i; ssize_t ret; + u32 offset; dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); @@ -1178,9 +1227,13 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, /* the size of data remaining on the first page */ page_remain = min_t(size_t, nor->page_size - page_offset, len - i); + offset = (to + i); + + if (nor->stripe) + offset /= 2; write_enable(nor); - ret = nor->write(nor, to + i, page_remain, buf + i); + ret = nor->write(nor, (offset), page_remain, buf + i); if (ret < 0) goto write_err; written = ret; @@ -1302,22 +1355,22 @@ static int spi_nor_check(struct spi_nor *nor) int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) { - const struct flash_info *info = NULL; + struct flash_info *info = NULL; struct device *dev = nor->dev; struct mtd_info *mtd = &nor->mtd; struct device_node *np = spi_nor_get_flash_node(nor); - int ret; - int i; + struct device_node *np_spi; + int ret, i, xlnx_qspi_mode; ret = spi_nor_check(nor); if (ret) return ret; if (name) - info = spi_nor_match_id(name); + info = (struct flash_info *)spi_nor_match_id(name); /* Try to auto-detect if chip name wasn't specified or not found */ if (!info) - info = spi_nor_read_id(nor); + info = (struct flash_info *)spi_nor_read_id(nor); if (IS_ERR_OR_NULL(info)) return -ENOENT; @@ -1341,7 +1394,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) */ dev_warn(dev, "found %s, expected %s\n", jinfo->name, info->name); - info = jinfo; + info = (struct flash_info *)jinfo; } } @@ -1370,6 +1423,27 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) mtd->size = info->sector_size * info->n_sectors; mtd->_erase = spi_nor_erase; mtd->_read = spi_nor_read; +#ifdef CONFIG_OF + np_spi = of_get_next_parent(np); + + if (of_property_read_u32(np_spi, "xlnx,qspi-mode", + &xlnx_qspi_mode) < 0) { + nor->shift = 0; + nor->stripe = 0; + } else if (xlnx_qspi_mode == 2) { + nor->shift = 1; + info->sector_size <<= nor->shift; + info->page_size <<= nor->shift; + mtd->size <<= nor->shift; + nor->stripe = 1; + nor->spi->master->flags |= (SPI_MASTER_BOTH_CS | + SPI_MASTER_DATA_STRIPE); + } +#else + /* Default to single */ + nor->shift = 0; + nor->stripe = 0; +#endif /* NOR protection support for STmicro/Micron chips and similar */ if (JEDEC_MFR(info) == SNOR_MFR_MICRON || @@ -1400,10 +1474,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) /* prefer "small sector" erase if possible */ if (info->flags & SECT_4K) { nor->erase_opcode = SPINOR_OP_BE_4K; - mtd->erasesize = 4096; + mtd->erasesize = 4096 << nor->shift; } else if (info->flags & SECT_4K_PMC) { nor->erase_opcode = SPINOR_OP_BE_4K_PMC; - mtd->erasesize = 4096; + mtd->erasesize = 4096 << nor->shift; } else #endif { @@ -1508,16 +1582,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) dev_info(dev, "%s (%lld Kbytes)\n", info->name, (long long)mtd->size >> 10); - dev_dbg(dev, - "mtd .name = %s, .size = 0x%llx (%lldMiB), " + dev_dbg(dev, "mtd .name = %s, .size = 0x%llx (%lldMiB), " ".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n", mtd->name, (long long)mtd->size, (long long)(mtd->size >> 20), mtd->erasesize, mtd->erasesize / 1024, mtd->numeraseregions); if (mtd->numeraseregions) for (i = 0; i < mtd->numeraseregions; i++) - dev_dbg(dev, - "mtd.eraseregions[%d] = { .offset = 0x%llx, " + dev_dbg(dev, "mtd.eraseregions[%d] = { .offset = 0x%llx, " ".erasesize = 0x%.8x (%uKiB), " ".numblocks = %d }\n", i, (long long)mtd->eraseregions[i].offset, diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 84f3ce5..673ec68 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -165,6 +165,8 @@ struct spi_nor { u8 read_dummy; u8 program_opcode; enum read_mode flash_read; + bool shift; + bool stripe; bool sst_write_second; u32 flags; u8 cmd_buf[SPI_NOR_MAX_CMD_SIZE];
This patch adds stripe support and it is needed for GQSPI parallel configuration mode by: - Adding required parameters like stripe and shift to spi_nor structure. - Initializing all added parameters in spi_nor_scan() - Updating read_sr() and read_fsr() for getting status from both flashes - Increasing page_size, sector_size, erase_size and toatal flash size as and when required. - Dividing address by 2 - Updating spi->master->flags for qspi driver to change CS Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> --- Changes for v4: - rename isparallel to stripe Changes for v3: - No change Changes for v2: - Splitted to separate MTD layer changes from SPI core changes --- drivers/mtd/spi-nor/spi-nor.c | 130 ++++++++++++++++++++++++++++++++---------- include/linux/mtd/spi-nor.h | 2 + 2 files changed, 103 insertions(+), 29 deletions(-)