[PATCH 1/2] mtd: m25p80: Reworkprobing/JEDEC code
in [Kernel]
|
From: Barry Song on 20 Jun 2010 23:30 On Mon, Jun 21, 2010 at 10:42 AM, Song, Barry <Barry.Song(a)analog.com> wrote: > > >>-----Original Message----- >>From: uclinux-dist-devel-bounces(a)blackfin.uclinux.org >>[mailto:uclinux-dist-devel-bounces(a)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(a)vger.kernel.org; linuxppc-dev(a)ozlabs.org; >>linux-mtd(a)lists.infradead.org; >>uclinux-dist-devel(a)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(a)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: 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 */ }; > >> >>Not-yet-Signed-off-by: Anton Vorontsov <cbouatmailru(a)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(a)blackfin.uclinux.org >>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Barry Song on 21 Jun 2010 03:30 On Mon, Jun 21, 2010 at 3:15 PM, Anton Vorontsov <cbouatmailru(a)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(a)gmail.com > irc://irc.freenode.net/bd2 >
From: Barry Song on 21 Jun 2010 06:40 On Mon, Jun 21, 2010 at 3:39 PM, Anton Vorontsov <cbouatmailru(a)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(a)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(a)gmail.com >> > irc://irc.freenode.net/bd2 >> > > > -- > Anton Vorontsov > email: cbouatmailru(a)gmail.com > irc://irc.freenode.net/bd2 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mike Frysinger on 21 Jun 2010 12:40 On Mon, Jun 21, 2010 at 07:20, Anton Vorontsov wrote: > You can't easily change OF. It's like "let's change ACPI tables > or BIOS in these PCs". Doable, but involves things like reflashing. > And we usually have to support old BIOSes as well. > > OTOH, I see (git grep m25p arch/powerpc/boot/dts/) that in > mainline kernel only MPC8569 board has a correct m25p > node, and it is STMicro variant (it is JEDEC capable). > > As we don't really have to support out of tree code, I'd > just go with this patch, assuming that we have to change > device tree for boards with non-JEDEC flashes. It's > effectively the same thing as platform data flag, except > that it works automatically for OF platforms. > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 81e49a9..a610ca9 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -680,6 +680,16 @@ static const struct spi_device_id m25p_ids[] = { > { "m25p64", INFO(0x202017, 0, 64 * 1024, 128, 0) }, > { "m25p128", INFO(0x202018, 0, 256 * 1024, 64, 0) }, > > + { "m25p05-nonjedec", INFO(0, 0, 32 * 1024, 2, 0) }, > + { "m25p10-nonjedec", INFO(0, 0, 32 * 1024, 4, 0) }, > + { "m25p20-nonjedec", INFO(0, 0, 64 * 1024, 4, 0) }, > + { "m25p40-nonjedec", INFO(0, 0, 64 * 1024, 8, 0) }, > + { "m25p80-nonjedec", INFO(0, 0, 64 * 1024, 16, 0) }, > + { "m25p16-nonjedec", INFO(0, 0, 64 * 1024, 32, 0) }, > + { "m25p32-nonjedec", INFO(0, 0, 64 * 1024, 64, 0) }, > + { "m25p64-nonjedec", INFO(0, 0, 64 * 1024, 128, 0) }, > + { "m25p128-nonjedec", INFO(0, 0, 256 * 1024, 64, 0) }, > + are you picking the m25p because its flash geometry matches whatever you're using, or because you have some weird variant of the m25p that has JEDEC commands removed ? -mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mike Frysinger on 21 Jun 2010 13:00 On Mon, Jun 21, 2010 at 12:47, Anton Vorontsov wrote: > On Mon, Jun 21, 2010 at 12:34:05PM -0400, Mike Frysinger wrote: >> On Mon, Jun 21, 2010 at 07:20, Anton Vorontsov wrote: >> > You can't easily change OF. It's like "let's change ACPI tables >> > or BIOS in these PCs". Doable, but involves things like reflashing. >> > And we usually have to support old BIOSes as well. >> > >> > OTOH, I see (git grep m25p arch/powerpc/boot/dts/) that in >> > mainline kernel only MPC8569 board has a correct m25p >> > node, and it is STMicro variant (it is JEDEC capable). >> > >> > As we don't really have to support out of tree code, I'd >> > just go with this patch, assuming that we have to change >> > device tree for boards with non-JEDEC flashes. It's >> > effectively the same thing as platform data flag, except >> > that it works automatically for OF platforms. >> > >> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >> > index 81e49a9..a610ca9 100644 >> > --- a/drivers/mtd/devices/m25p80.c >> > +++ b/drivers/mtd/devices/m25p80.c >> > @@ -680,6 +680,16 @@ static const struct spi_device_id m25p_ids[] = { >> > { "m25p64", INFO(0x202017, 0, 64 * 1024, 128, 0) }, >> > { "m25p128", INFO(0x202018, 0, 256 * 1024, 64, 0) }, >> > >> > + { "m25p05-nonjedec", INFO(0, 0, 32 * 1024, 2, 0) }, >> > + { "m25p10-nonjedec", INFO(0, 0, 32 * 1024, 4, 0) }, >> > + { "m25p20-nonjedec", INFO(0, 0, 64 * 1024, 4, 0) }, >> > + { "m25p40-nonjedec", INFO(0, 0, 64 * 1024, 8, 0) }, >> > + { "m25p80-nonjedec", INFO(0, 0, 64 * 1024, 16, 0) }, >> > + { "m25p16-nonjedec", INFO(0, 0, 64 * 1024, 32, 0) }, >> > + { "m25p32-nonjedec", INFO(0, 0, 64 * 1024, 64, 0) }, >> > + { "m25p64-nonjedec", INFO(0, 0, 64 * 1024, 128, 0) }, >> > + { "m25p128-nonjedec", INFO(0, 0, 256 * 1024, 64, 0) }, >> > + >> >> are you picking the m25p because its flash geometry matches whatever >> you're using, or because you have some weird variant of the m25p that >> has JEDEC commands removed ? > > The latter. It's Numonyx M25Pxx flashes, see > http://www.numonyx.com/Documents/Datasheets/M25P80.pdf > > The RDID instruction is available only for parts made with 110 > nm Technology identified with Process letter '4'. lovely. i guess this patch is the way to go to satisfy everyone's requirements. i'm also of the mindset that a mtd should not be created if the SPI flash isnt there simply because the resources say it might be. -mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 Prev: [PATCH 2/3] SCSI: Support Type C RAID controller Next: <<<<let's discuss>>>> |