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

[Barry Song]

On Mon, Jun 21, 2010 at 7:20 PM, Anton Vorontsov <cbouatmailru@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx>
> ---
>
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/