| Message ID | 1418511647-24736-1-git-send-email-vladimir_zapolskiy@mentor.com |
|---|---|
| State | Rejected |
| Headers | show |
On Sun, Dec 14, 2014 at 01:00:47AM +0200, Vladimir Zapolskiy wrote: > In attempt to spi_nor_scan() for an expected JEDEC compliant device by > reading RDID register don't return the first found non-JEDEC device > entry from spi_nor_ids[] table, if RDID is zero. > > First of all zeroes in RDID may be evidence for not correctly working > SPI, secondly empty RDID can not be used to select a particular JEDEC > non-compliant device correctly. > > The best possible solution is > * not to rely on spi_nor_read_id(), if expected device is non-JEDEC, > * not to substitute an expected JEDEC device with some arbitrary > chosen non-JEDEC device, if RDID is zero. > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> This patch doesn't apply to the latest tree. I think this commit probably already fixes your issue: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=09ffafb6977dc930770af2910edc3b469651131d commit 09ffafb6977dc930770af2910edc3b469651131d Author: Huang Shijie <shijie.huang@intel.com> Date: Thu Nov 6 07:34:01 2014 +0100 mtd: spi-nor: add id/id_len for flash_info{} > --- > drivers/mtd/spi-nor/spi-nor.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index c51ee52..119ace9 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -661,6 +661,12 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor) > > ext_jedec = id[3] << 8 | id[4]; > > + /* Non-JEDEC flash memory can not be detected correctly */ > + if (!jedec && !ext_jedec) { > + dev_err(nor->dev, "JEDEC compliant device is not found\n"); > + return ERR_PTR(-ENODEV); > + } > + > for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) { > info = (void *)spi_nor_ids[tmp].driver_data; > if (info->jedec_id == jedec) { Brian
Hi Brian, On 10.01.2015 00:28, Brian Norris wrote: > On Sun, Dec 14, 2014 at 01:00:47AM +0200, Vladimir Zapolskiy wrote: >> In attempt to spi_nor_scan() for an expected JEDEC compliant device by >> reading RDID register don't return the first found non-JEDEC device >> entry from spi_nor_ids[] table, if RDID is zero. >> >> First of all zeroes in RDID may be evidence for not correctly working >> SPI, secondly empty RDID can not be used to select a particular JEDEC >> non-compliant device correctly. >> >> The best possible solution is >> * not to rely on spi_nor_read_id(), if expected device is non-JEDEC, >> * not to substitute an expected JEDEC device with some arbitrary >> chosen non-JEDEC device, if RDID is zero. >> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > This patch doesn't apply to the latest tree. I think this commit > probably already fixes your issue: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=09ffafb6977dc930770af2910edc3b469651131d > > commit 09ffafb6977dc930770af2910edc3b469651131d > Author: Huang Shijie <shijie.huang@intel.com> > Date: Thu Nov 6 07:34:01 2014 +0100 > > mtd: spi-nor: add id/id_len for flash_info{} thank you for review. 09ffafb69 fixes my problem (and more), however regarding to my problem it does it in non-optimal way. If read JDID is zero, then there is no need to iterate over spi_nor_ids array to return ERR_PTR(-ENODEV). Assuming that zero JEDEC ID is a relatively rare situation, do you think it makes sense to add a preceding check for such case before entering the loop like it is done in my patch? If no, I'm fine, if yes, I'll rebase the change. >> --- >> drivers/mtd/spi-nor/spi-nor.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index c51ee52..119ace9 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -661,6 +661,12 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor) >> >> ext_jedec = id[3] << 8 | id[4]; >> >> + /* Non-JEDEC flash memory can not be detected correctly */ >> + if (!jedec && !ext_jedec) { >> + dev_err(nor->dev, "JEDEC compliant device is not found\n"); >> + return ERR_PTR(-ENODEV); >> + } >> + >> for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) { >> info = (void *)spi_nor_ids[tmp].driver_data; >> if (info->jedec_id == jedec) { > > Brian > -- With best wishes, Vladimir
On Sat, Jan 10, 2015 at 04:54:11PM +0200, Vladimir Zapolskiy wrote: > On 10.01.2015 00:28, Brian Norris wrote: > > On Sun, Dec 14, 2014 at 01:00:47AM +0200, Vladimir Zapolskiy wrote: > >> In attempt to spi_nor_scan() for an expected JEDEC compliant device by > >> reading RDID register don't return the first found non-JEDEC device > >> entry from spi_nor_ids[] table, if RDID is zero. > >> > >> First of all zeroes in RDID may be evidence for not correctly working > >> SPI, secondly empty RDID can not be used to select a particular JEDEC > >> non-compliant device correctly. > >> > >> The best possible solution is > >> * not to rely on spi_nor_read_id(), if expected device is non-JEDEC, > >> * not to substitute an expected JEDEC device with some arbitrary > >> chosen non-JEDEC device, if RDID is zero. > >> > >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > > > This patch doesn't apply to the latest tree. I think this commit > > probably already fixes your issue: > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=09ffafb6977dc930770af2910edc3b469651131d > > > > commit 09ffafb6977dc930770af2910edc3b469651131d > > Author: Huang Shijie <shijie.huang@intel.com> > > Date: Thu Nov 6 07:34:01 2014 +0100 > > > > mtd: spi-nor: add id/id_len for flash_info{} > > thank you for review. > > 09ffafb69 fixes my problem (and more), however regarding to my problem > it does it in non-optimal way. If read JDID is zero, then there is no > need to iterate over spi_nor_ids array to return ERR_PTR(-ENODEV). I'm not too worried about spinning a few extra cycles here. What makes this particular special case different than a system where we don't support the flash? We'll still likely have to scan through the whole array just to end up witih -ENODEV. > Assuming that zero JEDEC ID is a relatively rare situation, do you think > it makes sense to add a preceding check for such case before entering > the loop like it is done in my patch? As long as the current driver actually fails gracefully on a zero ID, I think we're OK without special case conditions. > If no, I'm fine, if yes, I'll rebase the change. Thanks, Brian
On 10.01.2015 20:29, Brian Norris wrote: > On Sat, Jan 10, 2015 at 04:54:11PM +0200, Vladimir Zapolskiy wrote: >> On 10.01.2015 00:28, Brian Norris wrote: >>> On Sun, Dec 14, 2014 at 01:00:47AM +0200, Vladimir Zapolskiy wrote: >>>> In attempt to spi_nor_scan() for an expected JEDEC compliant device by >>>> reading RDID register don't return the first found non-JEDEC device >>>> entry from spi_nor_ids[] table, if RDID is zero. >>>> >>>> First of all zeroes in RDID may be evidence for not correctly working >>>> SPI, secondly empty RDID can not be used to select a particular JEDEC >>>> non-compliant device correctly. >>>> >>>> The best possible solution is >>>> * not to rely on spi_nor_read_id(), if expected device is non-JEDEC, >>>> * not to substitute an expected JEDEC device with some arbitrary >>>> chosen non-JEDEC device, if RDID is zero. >>>> >>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >>> >>> This patch doesn't apply to the latest tree. I think this commit >>> probably already fixes your issue: >>> >>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=09ffafb6977dc930770af2910edc3b469651131d >>> >>> commit 09ffafb6977dc930770af2910edc3b469651131d >>> Author: Huang Shijie <shijie.huang@intel.com> >>> Date: Thu Nov 6 07:34:01 2014 +0100 >>> >>> mtd: spi-nor: add id/id_len for flash_info{} >> >> thank you for review. >> >> 09ffafb69 fixes my problem (and more), however regarding to my problem >> it does it in non-optimal way. If read JDID is zero, then there is no >> need to iterate over spi_nor_ids array to return ERR_PTR(-ENODEV). > > I'm not too worried about spinning a few extra cycles here. What makes > this particular special case different than a system where we don't > support the flash? We'll still likely have to scan through the whole > array just to end up witih -ENODEV. The difference is pretty visible, if JDID is zero, then it is known in advance that looking for a proper flash device from spi_nor_ids is fruitless and therefore can be omitted. >> Assuming that zero JEDEC ID is a relatively rare situation, do you think >> it makes sense to add a preceding check for such case before entering >> the loop like it is done in my patch? > > As long as the current driver actually fails gracefully on a zero ID, I > think we're OK without special case conditions. No objections, the optimization on error path may be considered as redundant. >> If no, I'm fine, if yes, I'll rebase the change. > -- With best wishes, Vladimir
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index c51ee52..119ace9 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -661,6 +661,12 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor) ext_jedec = id[3] << 8 | id[4]; + /* Non-JEDEC flash memory can not be detected correctly */ + if (!jedec && !ext_jedec) { + dev_err(nor->dev, "JEDEC compliant device is not found\n"); + return ERR_PTR(-ENODEV); + } + for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) { info = (void *)spi_nor_ids[tmp].driver_data; if (info->jedec_id == jedec) {
In attempt to spi_nor_scan() for an expected JEDEC compliant device by reading RDID register don't return the first found non-JEDEC device entry from spi_nor_ids[] table, if RDID is zero. First of all zeroes in RDID may be evidence for not correctly working SPI, secondly empty RDID can not be used to select a particular JEDEC non-compliant device correctly. The best possible solution is * not to rely on spi_nor_read_id(), if expected device is non-JEDEC, * not to substitute an expected JEDEC device with some arbitrary chosen non-JEDEC device, if RDID is zero. Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> --- drivers/mtd/spi-nor/spi-nor.c | 6 ++++++ 1 file changed, 6 insertions(+)