* [PATCH] mtd: spi-nor: don't return found by JEDEC ID a non-JEDEC flash
@ 2014-12-13 23:00 Vladimir Zapolskiy
2015-01-09 22:28 ` Brian Norris
0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Zapolskiy @ 2014-12-13 23:00 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, Huang Shijie, Rafał Miłecki
Cc: linux-mtd
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(+)
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) {
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: spi-nor: don't return found by JEDEC ID a non-JEDEC flash
2014-12-13 23:00 [PATCH] mtd: spi-nor: don't return found by JEDEC ID a non-JEDEC flash Vladimir Zapolskiy
@ 2015-01-09 22:28 ` Brian Norris
2015-01-10 14:54 ` Vladimir Zapolskiy
0 siblings, 1 reply; 5+ messages in thread
From: Brian Norris @ 2015-01-09 22:28 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Huang Shijie, Rafał Miłecki, David Woodhouse, linux-mtd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: spi-nor: don't return found by JEDEC ID a non-JEDEC flash
2015-01-09 22:28 ` Brian Norris
@ 2015-01-10 14:54 ` Vladimir Zapolskiy
2015-01-10 18:29 ` Brian Norris
0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Zapolskiy @ 2015-01-10 14:54 UTC (permalink / raw)
To: computersforpeace; +Cc: Huang Shijie, dwmw2, zajec5, linux-mtd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: spi-nor: don't return found by JEDEC ID a non-JEDEC flash
2015-01-10 14:54 ` Vladimir Zapolskiy
@ 2015-01-10 18:29 ` Brian Norris
2015-01-10 23:33 ` Vladimir Zapolskiy
0 siblings, 1 reply; 5+ messages in thread
From: Brian Norris @ 2015-01-10 18:29 UTC (permalink / raw)
To: Vladimir Zapolskiy; +Cc: Huang Shijie, dwmw2, zajec5, linux-mtd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: spi-nor: don't return found by JEDEC ID a non-JEDEC flash
2015-01-10 18:29 ` Brian Norris
@ 2015-01-10 23:33 ` Vladimir Zapolskiy
0 siblings, 0 replies; 5+ messages in thread
From: Vladimir Zapolskiy @ 2015-01-10 23:33 UTC (permalink / raw)
To: Brian Norris; +Cc: Huang Shijie, linux-mtd, zajec5, dwmw2
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-01-10 23:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-13 23:00 [PATCH] mtd: spi-nor: don't return found by JEDEC ID a non-JEDEC flash Vladimir Zapolskiy
2015-01-09 22:28 ` Brian Norris
2015-01-10 14:54 ` Vladimir Zapolskiy
2015-01-10 18:29 ` Brian Norris
2015-01-10 23:33 ` Vladimir Zapolskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.