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

login
register
mail settings
Submitter Anton Vorontsov
Date June 21, 2010, 11:20 a.m.
Message ID <20100621112049.GA9273@oksana.dev.rtsoft.ru>
Download mbox | patch
Permalink /patch/56303/
State Accepted
Commit f7b000904a848b64c36e3b4d0715744aaf345767
Headers show

Comments

Anton Vorontsov - June 21, 2010, 11:20 a.m.
On Mon, Jun 21, 2010 at 06:31:44PM +0800, Barry Song wrote:
> 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.

With OF we don't place non-existent devices into the device
tree (or we mark them with status = "not-ok/disabled/absent"
property).

> 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?

Why would this matter? We have to support both.

> If not, I think we should change OF platform codes to
> fit with this patch.

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.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
Mike Frysinger - June 21, 2010, 4:34 p.m.
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
Anton Vorontsov - June 21, 2010, 4:47 p.m.
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'.
Mike Frysinger - June 21, 2010, 4:54 p.m.
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
Barry Song - June 22, 2010, 6:37 a.m.
On Mon, Jun 21, 2010 at 7:20 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Mon, Jun 21, 2010 at 06:31:44PM +0800, Barry Song wrote:
>> 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.
>
> With OF we don't place non-existent devices into the device
> tree (or we mark them with status = "not-ok/disabled/absent"
> property).
>
>> 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?
>
> Why would this matter? We have to support both.
>
>> If not, I think we should change OF platform codes to
>> fit with this patch.
>
> 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.
>
> Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
> ---
>
> 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) },
> +
>        { "m45pe10", INFO(0x204011,  0, 64 * 1024,    2, 0) },
>        { "m45pe80", INFO(0x204014,  0, 64 * 1024,   16, 0) },
>        { "m45pe16", INFO(0x204015,  0, 64 * 1024,   32, 0) },
> @@ -795,8 +805,7 @@ static int __devinit m25p_probe(struct spi_device *spi)
>
>                jid = jedec_probe(spi);
>                if (!jid) {
> -                       dev_info(&spi->dev, "non-JEDEC variant of %s\n",
> -                                id->name);
> +                       return -ENODEV;
The patch looks good to me. Only problem is NULL is also returned by
spi_write_then_read() fail:
static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi)
{
        int                     tmp;
        u8                      code = OPCODE_RDID;
        u8                      id[5];
        u32                     jedec;
        u16                     ext_jedec;
        struct flash_info       *info;

        /* JEDEC also defines an optional "extended device information"
         * string for after vendor-specific data, after the three bytes
         * we use here.  Supporting some chips might require using it.
         */
        tmp = spi_write_then_read(spi, &code, 1, id, 5);
        if (tmp < 0) {
                DEBUG(MTD_DEBUG_LEVEL0, "%s: error %d reading JEDEC ID\n",
                        dev_name(&spi->dev), tmp);
                return NULL;
        }
...
}

Here much better for -EIO (return tmp)?

>                } else if (jid != id) {
>                        /*
>                         * JEDEC knows better, so overwrite platform ID. We
>
Anton Vorontsov - June 22, 2010, 4:55 p.m.
On Tue, Jun 22, 2010 at 02:37:52PM +0800, Barry Song wrote:
[...]
> >                jid = jedec_probe(spi);
> >                if (!jid) {
> > -                       dev_info(&spi->dev, "non-JEDEC variant of %s\n",
> > -                                id->name);
> > +                       return -ENODEV;
> The patch looks good to me. Only problem is NULL is also returned by
> spi_write_then_read() fail:
[...]
> Here much better for -EIO (return tmp)?

Agreed. Though, this is not a regression, and I guess desires
its own patch. 

Here are two patches, one for 2.6.35 (minimal changes to fix
the JEDEC problem), another for 2.6.36.

Patch

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) },
+
 	{ "m45pe10", INFO(0x204011,  0, 64 * 1024,    2, 0) },
 	{ "m45pe80", INFO(0x204014,  0, 64 * 1024,   16, 0) },
 	{ "m45pe16", INFO(0x204015,  0, 64 * 1024,   32, 0) },
@@ -795,8 +805,7 @@  static int __devinit m25p_probe(struct spi_device *spi)
 
 		jid = jedec_probe(spi);
 		if (!jid) {
-			dev_info(&spi->dev, "non-JEDEC variant of %s\n",
-				 id->name);
+			return -ENODEV;
 		} else if (jid != id) {
 			/*
 			 * JEDEC knows better, so overwrite platform ID. We