Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933853AbcLARBa (ORCPT ); Thu, 1 Dec 2016 12:01:30 -0500 Received: from eusmtp01.atmel.com ([212.144.249.242]:27693 "EHLO eusmtp01.atmel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759968AbcLARB0 (ORCPT ); Thu, 1 Dec 2016 12:01:26 -0500 Subject: Re: [LINUX RFC v4 3/4] mtd: spi-nor: add stripe support To: Naga Sureshkumar Relli , "broonie@kernel.org" , "michal.simek@xilinx.com" , Soren Brinkmann , "Harini Katakam" , Punnaiah Choudary Kalluri References: <1480235616-34038-1-git-send-email-nagasure@xilinx.com> <68ca0f19-e534-3361-11f0-6566626380fe@atmel.com> From: Cyrille Pitchen CC: "linux-spi@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-mtd@lists.infradead.org" Message-ID: <504cd632-022b-3e6b-8c50-563a585e1a08@atmel.com> Date: Thu, 1 Dec 2016 18:01:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.145.133.18] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15380 Lines: 464 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 >>> --- >>> 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 >>> #include >>> #include >>> +#include >>> >>> /* 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 >