diff mbox

[Uclinux-dist-devel,1/2] mtd: m25p80: Reworkprobing/JEDEC code

Message ID AANLkTiksYofs6hhWta6uFX2cla_g8pBu0Mtbtcbv-2tl@mail.gmail.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Barry Song June 21, 2010, 3:27 a.m. UTC 
On Mon, Jun 21, 2010 at 10:42 AM, Song, Barry <Barry.Song@analog.com> wrote:
>
>
>>-----Original Message-----
>>From: uclinux-dist-devel-bounces@blackfin.uclinux.org
>>[mailto:uclinux-dist-devel-bounces@blackfin.uclinux.org] On
>>Behalf Of Anton Vorontsov
>>Sent: Friday, June 18, 2010 9:32 PM
>>To: Barry Song
>>Cc: David Brownell; Artem Bityutskiy;
>>linux-kernel@vger.kernel.org; linuxppc-dev@ozlabs.org;
>>linux-mtd@lists.infradead.org;
>>uclinux-dist-devel@blackfin.uclinux.org; Andrew Morton
>>Subject: Re: [Uclinux-dist-devel] [PATCH 1/2] mtd: m25p80:
>>Reworkprobing/JEDEC code
>>
>>On Sat, Jun 12, 2010 at 02:27:12PM +0800, Barry Song wrote:
>>> On Wed, Aug 19, 2009 at 5:46 AM, Anton Vorontsov
>>> <avorontsov@ru.mvista.com> wrote:
>>> >
>>> > Previosly the driver always tried JEDEC probing, assuming
>>that non-JEDEC
>>> > chips will return '0'. But truly non-JEDEC chips (like
>>CAT25) won't do
>>> > that, their behaviour on RDID command is undefined, so the
>>driver should
>>> > not call jedec_probe() for these chips.
>>> >
>>> > Also, be less strict on error conditions, don't fail to
>>probe if JEDEC
>>> > found a chip that is different from what platform code
>>told, instead
>>> > just print some warnings and use an information obtained
>>via JEDEC. In
>>> This patch caused a problem:
>>> even though the external flash doesn't exist, it will still pass the
>>> probe() and be registerred into kernel and given the partition table.
>>> You may refer to this bug report:
>>>
>>http://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?ac
>>tion=TrackerItemEdit&tracker_item_id=5975&start=0
>>
>>Thanks for the report.
>>
>>There's little we can do about it. Platform code asked us
>>to register the device, and JEDEC probing of M25Pxx chips isn't
>>reliable (thanks to various vendors that make these JEDEC and
>>non-JEDEC variants), so the best thing we can do is to register
>>the chip anyway.
>>
>>OTOH, if the board pulls MISO line up, then the following patch
>>should help.
> Make sense with pullup to keep the value high while external device
> doesn't exist.
>>
>>If this won't work, we'll have to add some flag to the platform
>>data, i.e. to force JEDEC probing, and not trust platform data.
>
> How about we add a non_jedec flag in platform_data, if the flag is 1, we
> let the detection pass even though the ID is 0? Otherwise, we need a
> valid ID?
Here i mean:
>
>>
>>Not-yet-Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
>>---
>>
>>diff --git a/drivers/mtd/devices/m25p80.c
>>b/drivers/mtd/devices/m25p80.c
>>index 81e49a9..a307929 100644
>>--- a/drivers/mtd/devices/m25p80.c
>>+++ b/drivers/mtd/devices/m25p80.c
>>@@ -16,6 +16,7 @@
>>  */
>>
>> #include <linux/init.h>
>>+#include <linux/errno.h>
>> #include <linux/module.h>
>> #include <linux/device.h>
>> #include <linux/interrupt.h>
>>@@ -723,7 +724,7 @@ static const struct spi_device_id
>>*__devinit jedec_probe(struct spi_device *spi)
>>       if (tmp < 0) {
>>               DEBUG(MTD_DEBUG_LEVEL0, "%s: error %d reading
>>JEDEC ID\n",
>>                       dev_name(&spi->dev), tmp);
>>-              return NULL;
>>+              return ERR_PTR(tmp);
>>       }
>>       jedec = id[0];
>>       jedec = jedec << 8;
>>@@ -737,7 +738,7 @@ static const struct spi_device_id
>>*__devinit jedec_probe(struct spi_device *spi)
>>        * exist for non-JEDEC chips, but for compatibility
>>they return ID 0.
>>        */
>>       if (jedec == 0)
>>-              return NULL;
>>+              return ERR_PTR(-EEXIST);
>>
>>       ext_jedec = id[3] << 8 | id[4];
>>
>>@@ -749,7 +750,7 @@ static const struct spi_device_id
>>*__devinit jedec_probe(struct spi_device *spi)
>>                       return &m25p_ids[tmp];
>>               }
>>       }
>>-      return NULL;
>>+      return ERR_PTR(-ENODEV);
>> }
>>
>>
>>@@ -794,9 +795,11 @@ static int __devinit m25p_probe(struct
>>spi_device *spi)
>>               const struct spi_device_id *jid;
>>
>>               jid = jedec_probe(spi);
>>-              if (!jid) {
>>+              if (IS_ERR(jid) && PTR_ERR(jid) == -EEXIST) {
>>                       dev_info(&spi->dev, "non-JEDEC variant of %s\n",
>>                                id->name);
>>+              } else if (IS_ERR(jid)) {
>>+                      return PTR_ERR(jid);
>>               } else if (jid != id) {
>>                       /*
>>                        * JEDEC knows better, so overwrite
>>platform ID. We
>>_______________________________________________
>>Uclinux-dist-devel mailing list
>>Uclinux-dist-devel@blackfin.uclinux.org
>>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
>>
>

Comments

Anton Vorontsov June 21, 2010, 7:15 a.m. UTC | #1 
On Mon, Jun 21, 2010 at 11:27:31AM +0800, Barry Song wrote:
[...]
> > How about we add a non_jedec flag in platform_data, if the flag is 1, we
> > let the detection pass even though the ID is 0? Otherwise, we need a
> > valid ID?
> Here i mean:

This will break at least OF-enabled platforms (e.g. PowerPC),
they assume that the driver will success for non-JEDEC flashes.
OF platforms don't pass platform data, and even if they did,
device tree doesn't specify if the flash is JEDEC or non-JEDEC.

Which is why I think that, by default, the driver should
successfully register the flash even if JEDEC probe fails. So,
instead of checking for "!non_jedec", I would recommend
"force_jedec" check.

> Index: drivers/mtd/devices/m25p80.c
> ===================================================================
> --- drivers/mtd/devices/m25p80.c	(revision 8927)
> +++ drivers/mtd/devices/m25p80.c	(revision 8929)
> @@ -795,8 +795,13 @@
> 
>  		jid = jedec_probe(spi);
>  		if (!jid) {
> -			dev_info(&spi->dev, "non-JEDEC variant of %s\n",
> -				 id->name);
> +			if (!data->non_jedec) {
> +				dev_err(&spi->dev, "fail to detect%s\n",
> +						id->name);
> +				return -ENODEV;
> +			} else
> +				dev_info(&spi->dev, "non-JEDEC variant of %s\n",
> +						id->name);
>  		} else if (jid != id) {
Barry Song June 21, 2010, 7:22 a.m. UTC | #2 
On Mon, Jun 21, 2010 at 3:15 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Mon, Jun 21, 2010 at 11:27:31AM +0800, Barry Song wrote:
> [...]
>> > How about we add a non_jedec flag in platform_data, if the flag is 1, we
>> > let the detection pass even though the ID is 0? Otherwise, we need a
>> > valid ID?
>> Here i mean:
>
> This will break at least OF-enabled platforms (e.g. PowerPC),
> they assume that the driver will success for non-JEDEC flashes.
> OF platforms don't pass platform data, and even if they did,
> device tree doesn't specify if the flash is JEDEC or non-JEDEC.
>
> Which is why I think that, by default, the driver should
> successfully register the flash even if JEDEC probe fails. So,
> instead of checking for "!non_jedec", I would recommend
> "force_jedec" check.

Mike Frysinger suggested to use non_jedec since most devices are
standard jedec devices. Only if non_jedec=1, we let the detection pass
if ID is 0.

>
>> Index: drivers/mtd/devices/m25p80.c
>> ===================================================================
>> --- drivers/mtd/devices/m25p80.c      (revision 8927)
>> +++ drivers/mtd/devices/m25p80.c      (revision 8929)
>> @@ -795,8 +795,13 @@
>>
>>               jid = jedec_probe(spi);
>>               if (!jid) {
>> -                     dev_info(&spi->dev, "non-JEDEC variant of %s\n",
>> -                              id->name);
>> +                     if (!data->non_jedec) {
>> +                             dev_err(&spi->dev, "fail to detect%s\n",
>> +                                             id->name);
>> +                             return -ENODEV;
>> +                     } else
>> +                             dev_info(&spi->dev, "non-JEDEC variant of %s\n",
>> +                                             id->name);
>>               } else if (jid != id) {
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>
Anton Vorontsov June 21, 2010, 7:39 a.m. UTC | #3 
On Mon, Jun 21, 2010 at 03:22:48PM +0800, Barry Song wrote:
> On Mon, Jun 21, 2010 at 3:15 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Mon, Jun 21, 2010 at 11:27:31AM +0800, Barry Song wrote:
> > [...]
> >> > How about we add a non_jedec flag in platform_data, if the flag is 1, we
> >> > let the detection pass even though the ID is 0? Otherwise, we need a
> >> > valid ID?
> >> Here i mean:
> >
> > This will break at least OF-enabled platforms (e.g. PowerPC),
> > they assume that the driver will success for non-JEDEC flashes.
> > OF platforms don't pass platform data, and even if they did,
> > device tree doesn't specify if the flash is JEDEC or non-JEDEC.
> >
> > Which is why I think that, by default, the driver should
> > successfully register the flash even if JEDEC probe fails. So,
> > instead of checking for "!non_jedec", I would recommend
> > "force_jedec" check.
> 
> Mike Frysinger suggested to use non_jedec since most devices are
> standard jedec devices.

Well, on OF platforms most devices that I'm aware of are non-JEDEC.

> Only if non_jedec=1, we let the detection pass
> if ID is 0.

Then please #ifdef it with CONFIG_OF.

Thanks,

> >> Index: drivers/mtd/devices/m25p80.c
> >> ===================================================================
> >> --- drivers/mtd/devices/m25p80.c      (revision 8927)
> >> +++ drivers/mtd/devices/m25p80.c      (revision 8929)
> >> @@ -795,8 +795,13 @@
> >>
> >>               jid = jedec_probe(spi);
> >>               if (!jid) {
> >> -                     dev_info(&spi->dev, "non-JEDEC variant of %s\n",
> >> -                              id->name);
> >> +                     if (!data->non_jedec) {
> >> +                             dev_err(&spi->dev, "fail to detect%s\n",
> >> +                                             id->name);
> >> +                             return -ENODEV;
> >> +                     } else
> >> +                             dev_info(&spi->dev, "non-JEDEC variant of %s\n",
> >> +                                             id->name);
> >>               } else if (jid != id) {
> >
> > --
> > Anton Vorontsov
> > email: cbouatmailru@gmail.com
> > irc://irc.freenode.net/bd2
> >
Barry Song June 21, 2010, 10:31 a.m. UTC | #4 
On Mon, Jun 21, 2010 at 3:39 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Mon, Jun 21, 2010 at 03:22:48PM +0800, Barry Song wrote:
>> On Mon, Jun 21, 2010 at 3:15 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
>> > On Mon, Jun 21, 2010 at 11:27:31AM +0800, Barry Song wrote:
>> > [...]
>> >> > How about we add a non_jedec flag in platform_data, if the flag is 1, we
>> >> > let the detection pass even though the ID is 0? Otherwise, we need a
>> >> > valid ID?
>> >> Here i mean:
>> >
>> > This will break at least OF-enabled platforms (e.g. PowerPC),
>> > they assume that the driver will success for non-JEDEC flashes.
>> > OF platforms don't pass platform data, and even if they did,
>> > device tree doesn't specify if the flash is JEDEC or non-JEDEC.
>> >
>> > Which is why I think that, by default, the driver should
>> > successfully register the flash even if JEDEC probe fails. So,
>> > instead of checking for "!non_jedec", I would recommend
>> > "force_jedec" check.
>>
>> Mike Frysinger suggested to use non_jedec since most devices are
>> standard jedec devices.
>
> Well, on OF platforms most devices that I'm aware of are non-JEDEC.
>
>> Only if non_jedec=1, we let the detection pass
>> if ID is 0.
>
> Then please #ifdef it with CONFIG_OF.
I think the patch has nothing to do with platform. Here SPI Flash is a
peripherals, doesn't depend on any platform. Adding a CONFIG_OF
doesn't make sense very much.
If you think most devices are non-JEDEC, we can change non_JEDEC to
force_JEDEC as you said. But anyway, is that real that most devices
are non_JEDEC?  If not, I think we should change OF platform codes to
fit with this patch.

>
> Thanks,
>
>> >> Index: drivers/mtd/devices/m25p80.c
>> >> ===================================================================
>> >> --- drivers/mtd/devices/m25p80.c      (revision 8927)
>> >> +++ drivers/mtd/devices/m25p80.c      (revision 8929)
>> >> @@ -795,8 +795,13 @@
>> >>
>> >>               jid = jedec_probe(spi);
>> >>               if (!jid) {
>> >> -                     dev_info(&spi->dev, "non-JEDEC variant of %s\n",
>> >> -                              id->name);
>> >> +                     if (!data->non_jedec) {
>> >> +                             dev_err(&spi->dev, "fail to detect%s\n",
>> >> +                                             id->name);
>> >> +                             return -ENODEV;
>> >> +                     } else
>> >> +                             dev_info(&spi->dev, "non-JEDEC variant of %s\n",
>> >> +                                             id->name);
>> >>               } else if (jid != id) {
>> >
>> > --
>> > Anton Vorontsov
>> > email: cbouatmailru@gmail.com
>> > irc://irc.freenode.net/bd2
>> >
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>
diff mbox

Patch

Index: drivers/mtd/devices/m25p80.c
===================================================================
--- drivers/mtd/devices/m25p80.c	(revision 8927)
+++ drivers/mtd/devices/m25p80.c	(revision 8929)
@@ -795,8 +795,13 @@ 

 		jid = jedec_probe(spi);
 		if (!jid) {
-			dev_info(&spi->dev, "non-JEDEC variant of %s\n",
-				 id->name);
+			if (!data->non_jedec) {
+				dev_err(&spi->dev, "fail to detect%s\n",
+						id->name);
+				return -ENODEV;
+			} else
+				dev_info(&spi->dev, "non-JEDEC variant of %s\n",
+						id->name);
 		} else if (jid != id) {
 			/*
 			 * JEDEC knows better, so overwrite platform ID. We
Index: include/linux/spi/flash.h
===================================================================
--- include/linux/spi/flash.h	(revision 8927)
+++ include/linux/spi/flash.h	(revision 8929)
@@ -25,6 +25,11 @@ 

 	char		*type;

+	/*
+	 * For non-JEDEC, id will be 0. In this case, we can't be sure
+	 * whether the flash exists with runtime probing.
+	 */
+	int             non_jedec;
 	/* we'll likely add more ... use JEDEC IDs, etc */
 };