LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Rafał Miłecki" <zajec5@gmail.com>
To: Viet Nga Dao <vndao@altera.com>
Cc: Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] mtd:spi-nor: Add Altera EPCQ Driver
Date: Tue, 10 Mar 2015 07:55:08 +0100	[thread overview]
Message-ID: <CACna6rzPRKFN+RzoF3TnNrPMy=rci25wb4NFPHZXBu9P61MjLQ@mail.gmail.com> (raw)
In-Reply-To: <CAN1oZWw2RUf7jSDGqk5UUrT3DDR2MHD6ORGQ1OLXQyES6+3Cjw@mail.gmail.com>

On 10 March 2015 at 07:11, Viet Nga Dao <vndao@altera.com> wrote:
> On Mon, Mar 9, 2015 at 2:31 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 11 February 2015 at 05:53, Viet Nga Dao <vndao@altera.com> wrote:
>>>  /* NOTE: double check command sets and memory organization when you add
>>>   * more nor chips.  This current list focusses on newer chips, which
>>>   * have been converging on command sets which including JEDEC ID.
>>> @@ -637,6 +691,17 @@ static const struct spi_device_id spi_nor_ids[] = {
>>>      { "cat25c09", CAT25_INFO( 128, 8, 32, 2, SPI_NOR_NO_ERASE |
>>> SPI_NOR_NO_FR) },
>>>      { "cat25c17", CAT25_INFO( 256, 8, 32, 2, SPI_NOR_NO_ERASE |
>>> SPI_NOR_NO_FR) },
>>>      { "cat25128", CAT25_INFO(2048, 8, 64, 2, SPI_NOR_NO_ERASE |
>>> SPI_NOR_NO_FR) },
>>> +
>>> +    /* Altera EPCQ/EPCS Flashes */
>>> +    { "epcq16"  , EPCQ_INFO(2, 0x15, 0x10000, 32,   0x100) },
>>> +    { "epcq32"  , EPCQ_INFO(2, 0x16, 0x10000, 64,   0x100) },
>>> +    { "epcq64"  , EPCQ_INFO(2, 0x17, 0x10000, 128,  0x100) },
>>> +    { "epcq128" , EPCQ_INFO(2, 0x18, 0x10000, 256,  0x100) },
>>> +    { "epcq256" , EPCQ_INFO(2, 0x19, 0x10000, 512,  0x100) },
>>> +    { "epcq512" , EPCQ_INFO(2, 0x20, 0x10000, 1024, 0x100) },
>>> +    { "epcs16"  , EPCQ_INFO(1, 0x14, 0x10000, 32,   0x100) },
>>> +    { "epcs64"  , EPCQ_INFO(1, 0x16, 0x10000, 128,  0x100) },
>>> +    { "epcs128" , EPCQ_INFO(1, 0x18, 0x40000, 256,  0x100) },
>>>      { },
>>>  };
>>>
>>> @@ -666,6 +731,14 @@ static const struct spi_device_id
>>> *spi_nor_read_id(struct spi_nor *nor)
>>>          if (info->jedec_id == jedec) {
>>>              if (info->ext_id == 0 || info->ext_id == ext_jedec)
>>>                  return &spi_nor_ids[tmp];
>>> +
>>> +        /* altera epcq which is non jedec device
>>> +         * use id[4] as opcode id to differentiate
>>> +         * epcs and epcq devices
>>> +         */
>>> +        } else if (info->altera_flash_opcode_id == id[4] &&
>>> +              info->ext_id == id[3]) {
>>> +                return &spi_nor_ids[tmp];
>>
>> This is the part I don't like. I think it's fishy, and that this check
>> may result in false positives. Looks too generic.
>>
>> Also the logic of your behavior there seems unclear to me. On the one
>> hand you don't have JEDEC, so you provide chip name using DT. But in
>> place above you stop trusting DT info and you try to (kind of)
>> auto-detect used chip anyway.
>>
>> I guess we should finally think about some more generic way of passing
>> flash info.
>
> Actually, i just want fo follow the way current spi-nor doing as much
> as possible. Like to read the device id and compare with info table.
> Like double checking from both dtb and the device id. Since the
> flashes i support do not have JEDEC id but only extended id. But the
> problem is that some of them have the same extended id, for example
> epcs64 and epcq32). That is why in my driver, i have to decode 1st
> byte of ext id to differentiate epcs and ecpq.

I see your point and it makes sense, I just think it shouldn't be part
of spi-nor. By adding chip specific code to spi-nor we'll end with
hacky code and possible false chips identifications. I can really
easily imagine some random chip having the same id[3] and id[4] as one
of Altera flashes.

Moreover your patch has not working support for epcs16 and epcs64.
They don't support 0x9f opcode (SPINOR_OP_RDID), so you would need to
add support for 0xab ("Read silicon ID") to the spi-nor.

It's the same problem I have with Broadcom's "w25q128" that doesn't
support 0x9f opcode but a 0x90 with 16b reply. You may see my tiny
bcm53xxspiflash.c driver:
http://git.openwrt.org/?p=openwrt.git;a=blob;f=target/linux/bcm53xx/files/drivers/mtd/spi-nor/bcm53xxspiflash.c;h=f192f4e59b71a2444833b5c62dd2239d28f9435d;hb=d105c51a428a72a9af42759c472df9960c496d67

So I'm afraid that if spi-nor gets support for:
1) 0xab opcode
2) 0x90 opcode
3) Some uncommon replies for 0x9f opcode (like Altera ones)
it will quickly get hacky & buggy.

So what about:
1) Using 0x9f and 0xab in altera_epcq.c
2) Finding chip name in altera_epcq.c
3) Adding Altera chip names & all sizes to spi-nor.c
4) Just passing a chip name to spi-nor.c

It's something I do in bcm53xxspiflash.c. I detect w25q128 using some
specific 0x90 opcode and just pass a chip name to the spi-nor.

-- 
Rafał

  reply	other threads:[~2015-03-10  6:55 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-11  4:53 Viet Nga Dao
2015-02-17  1:33 ` Viet Nga Dao
2015-02-23  1:30   ` Viet Nga Dao
2015-02-24  3:59     ` Brian Norris
2015-03-09  1:42       ` Viet Nga Dao
2015-03-13  2:42     ` Viet Nga Dao
2015-03-09  6:31 ` Rafał Miłecki
2015-03-10  6:11   ` Viet Nga Dao
2015-03-10  6:55     ` Rafał Miłecki [this message]
     [not found]       ` <A8F913A698B3864C898FDDF9BF921BC9E8C00F@PG-ITEXCH01.altera.priv.altera.com>
2015-03-10  8:09         ` FW: " Viet Nga Dao
2015-03-11  8:41           ` Viet Nga Dao
2015-03-13  5:45             ` Rafał Miłecki
2015-03-13  6:47               ` Viet Nga Dao
2015-03-13  7:38 ` Brian Norris
2015-03-13  8:25   ` Viet Nga Dao
2015-03-16  8:46   ` Viet Nga Dao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACna6rzPRKFN+RzoF3TnNrPMy=rci25wb4NFPHZXBu9P61MjLQ@mail.gmail.com' \
    --to=zajec5@gmail.com \
    --cc=computersforpeace@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dwmw2@infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=vndao@altera.com \
    --subject='Re: [PATCH v2] mtd:spi-nor: Add Altera EPCQ Driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).