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 <nagas...@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
>
