Patchwork [1/2] mtd: m25p80: Rework probing/JEDEC code

login
register
mail settings
Submitter Anton Vorontsov
Date Aug. 18, 2009, 9:46 p.m.
Message ID <20090818214622.GA22651@oksana.dev.rtsoft.ru>
Download mbox | patch
Permalink /patch/31594/
State Accepted
Commit 18c6182bae0acca220ed6611f741034d563cd19f
Headers show

Comments

Anton Vorontsov - Aug. 18, 2009, 9:46 p.m.
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
that case we should not trust partitions any longer, but they might be
still useful (i.e. they could protect some parts of the chip).

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mtd/devices/m25p80.c |   69 ++++++++++++++++++++++++-----------------
 1 files changed, 40 insertions(+), 29 deletions(-)
Barry Song - June 12, 2010, 6:27 a.m.
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/?action=TrackerItemEdit&tracker_item_id=5975&start=0

> that case we should not trust partitions any longer, but they might be
> still useful (i.e. they could protect some parts of the chip).
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/mtd/devices/m25p80.c |   69 ++++++++++++++++++++++++-----------------
>  1 files changed, 40 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 0d74b38..b75e319 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -581,6 +581,14 @@ static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi)
>        jedec = jedec << 8;
>        jedec |= id[2];
>
> +       /*
> +        * Some chips (like Numonyx M25P80) have JEDEC and non-JEDEC variants,
> +        * which depend on technology process. Officially RDID command doesn't
> +        * exist for non-JEDEC chips, but for compatibility they return ID 0.
> +        */
> +       if (jedec == 0)
> +               return NULL;
> +
>        ext_jedec = id[3] << 8 | id[4];
>
>        for (tmp = 0; tmp < ARRAY_SIZE(m25p_ids) - 1; tmp++) {
> @@ -602,7 +610,7 @@ static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi)
>  */
>  static int __devinit m25p_probe(struct spi_device *spi)
>  {
> -       const struct spi_device_id      *id;
> +       const struct spi_device_id      *id = spi_get_device_id(spi);
>        struct flash_platform_data      *data;
>        struct m25p                     *flash;
>        struct flash_info               *info;
> @@ -615,41 +623,44 @@ static int __devinit m25p_probe(struct spi_device *spi)
>         */
>        data = spi->dev.platform_data;
>        if (data && data->type) {
> +               const struct spi_device_id *plat_id;
> +
>                for (i = 0; i < ARRAY_SIZE(m25p_ids) - 1; i++) {
> -                       id = &m25p_ids[i];
> -                       info = (void *)m25p_ids[i].driver_data;
> -                       if (strcmp(data->type, id->name))
> +                       plat_id = &m25p_ids[i];
> +                       if (strcmp(data->type, plat_id->name))
>                                continue;
>                        break;
>                }
>
> -               /* unrecognized chip? */
> -               if (i == ARRAY_SIZE(m25p_ids) - 1) {
> -                       DEBUG(MTD_DEBUG_LEVEL0, "%s: unrecognized id %s\n",
> -                                       dev_name(&spi->dev), data->type);
> -                       info = NULL;
> -
> -               /* recognized; is that chip really what's there? */
> -               } else if (info->jedec_id) {
> -                       id = jedec_probe(spi);
> -
> -                       if (id != &m25p_ids[i]) {
> -                               dev_warn(&spi->dev, "found %s, expected %s\n",
> -                                               id ? id->name : "UNKNOWN",
> -                                               m25p_ids[i].name);
> -                               info = NULL;
> -                       }
> -               }
> -       } else {
> -               id = jedec_probe(spi);
> -               if (!id)
> -                       id = spi_get_device_id(spi);
> -
> -               info = (void *)id->driver_data;
> +               if (plat_id)
> +                       id = plat_id;
> +               else
> +                       dev_warn(&spi->dev, "unrecognized id %s\n", data->type);
>        }
>
> -       if (!info)
> -               return -ENODEV;
> +       info = (void *)id->driver_data;
> +
> +       if (info->jedec_id) {
> +               const struct spi_device_id *jid;
> +
> +               jid = jedec_probe(spi);
> +               if (!jid) {
> +                       dev_info(&spi->dev, "non-JEDEC variant of %s\n",
> +                                id->name);
> +               } else if (jid != id) {
> +                       /*
> +                        * JEDEC knows better, so overwrite platform ID. We
> +                        * can't trust partitions any longer, but we'll let
> +                        * mtd apply them anyway, since some partitions may be
> +                        * marked read-only, and we don't want to lose that
> +                        * information, even if it's not 100% accurate.
> +                        */
> +                       dev_warn(&spi->dev, "found %s, expected %s\n",
> +                                jid->name, id->name);
> +                       id = jid;
> +                       info = (void *)jid->driver_data;
> +               }
> +       }
>
>        flash = kzalloc(sizeof *flash, GFP_KERNEL);
>        if (!flash)
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Patch

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 0d74b38..b75e319 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -581,6 +581,14 @@  static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi)
 	jedec = jedec << 8;
 	jedec |= id[2];
 
+	/*
+	 * Some chips (like Numonyx M25P80) have JEDEC and non-JEDEC variants,
+	 * which depend on technology process. Officially RDID command doesn't
+	 * exist for non-JEDEC chips, but for compatibility they return ID 0.
+	 */
+	if (jedec == 0)
+		return NULL;
+
 	ext_jedec = id[3] << 8 | id[4];
 
 	for (tmp = 0; tmp < ARRAY_SIZE(m25p_ids) - 1; tmp++) {
@@ -602,7 +610,7 @@  static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi)
  */
 static int __devinit m25p_probe(struct spi_device *spi)
 {
-	const struct spi_device_id	*id;
+	const struct spi_device_id	*id = spi_get_device_id(spi);
 	struct flash_platform_data	*data;
 	struct m25p			*flash;
 	struct flash_info		*info;
@@ -615,41 +623,44 @@  static int __devinit m25p_probe(struct spi_device *spi)
 	 */
 	data = spi->dev.platform_data;
 	if (data && data->type) {
+		const struct spi_device_id *plat_id;
+
 		for (i = 0; i < ARRAY_SIZE(m25p_ids) - 1; i++) {
-			id = &m25p_ids[i];
-			info = (void *)m25p_ids[i].driver_data;
-			if (strcmp(data->type, id->name))
+			plat_id = &m25p_ids[i];
+			if (strcmp(data->type, plat_id->name))
 				continue;
 			break;
 		}
 
-		/* unrecognized chip? */
-		if (i == ARRAY_SIZE(m25p_ids) - 1) {
-			DEBUG(MTD_DEBUG_LEVEL0, "%s: unrecognized id %s\n",
-					dev_name(&spi->dev), data->type);
-			info = NULL;
-
-		/* recognized; is that chip really what's there? */
-		} else if (info->jedec_id) {
-			id = jedec_probe(spi);
-
-			if (id != &m25p_ids[i]) {
-				dev_warn(&spi->dev, "found %s, expected %s\n",
-						id ? id->name : "UNKNOWN",
-						m25p_ids[i].name);
-				info = NULL;
-			}
-		}
-	} else {
-		id = jedec_probe(spi);
-		if (!id)
-			id = spi_get_device_id(spi);
-
-		info = (void *)id->driver_data;
+		if (plat_id)
+			id = plat_id;
+		else
+			dev_warn(&spi->dev, "unrecognized id %s\n", data->type);
 	}
 
-	if (!info)
-		return -ENODEV;
+	info = (void *)id->driver_data;
+
+	if (info->jedec_id) {
+		const struct spi_device_id *jid;
+
+		jid = jedec_probe(spi);
+		if (!jid) {
+			dev_info(&spi->dev, "non-JEDEC variant of %s\n",
+				 id->name);
+		} else if (jid != id) {
+			/*
+			 * JEDEC knows better, so overwrite platform ID. We
+			 * can't trust partitions any longer, but we'll let
+			 * mtd apply them anyway, since some partitions may be
+			 * marked read-only, and we don't want to lose that
+			 * information, even if it's not 100% accurate.
+			 */
+			dev_warn(&spi->dev, "found %s, expected %s\n",
+				 jid->name, id->name);
+			id = jid;
+			info = (void *)jid->driver_data;
+		}
+	}
 
 	flash = kzalloc(sizeof *flash, GFP_KERNEL);
 	if (!flash)