Re: [PATCH v16 00/15] mtd: spi-nor: add xSPI Octal DTR support

From: Vignesh Raghavendra <vigneshr@ti.com>
To: "Tudor.Ambarus@microchip.com" <Tudor.Ambarus@microchip.com>,
	"Yadav, Pratyush" <p.yadav@ti.com>
Cc: "miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
	"richard@nod.at" <richard@nod.at>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Nori, Sekhar" <nsekhar@ti.com>,
	"boris.brezillon@collabora.com" <boris.brezillon@collabora.com>
Subject: Re: [PATCH v16 00/15] mtd: spi-nor: add xSPI Octal DTR support
Date: Thu, 29 Oct 2020 11:56:01 +0530	[thread overview]
Message-ID: <b7a18f2a-edd6-25e1-2f30-1a4427e67018@ti.com> (raw)
In-Reply-To: <d9d96eda-4cb6-fe68-7469-e73dc7ba2fda@microchip.com>

On 10/28/20 8:51 PM, Tudor.Ambarus@microchip.com wrote:
> On 10/28/20 2:49 PM, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi Tudor,
>>
>> On 28/10/20 07:53AM, Tudor.Ambarus@microchip.com wrote:
>>> Hi, Pratyush,
>>>
>>> On 10/5/20 6:31 PM, Pratyush Yadav wrote:
>>>> Tested on Micron MT35X and S28HS flashes for Octal DTR.
>>>
>>> Do these flashes define the "Command Sequences to Change to
>>> Octal DDR (8D-8D-8D) mode" table? Can't we use that table
>>> instead of defining our own octal dtr enable functions?
>>
>> The Micron flash does not have this table. The Cypress flash does. The
>> problem is that one of the samples of the Cypress flash I tested on had
>> incorrect data in that table which meant the sequence would fail. The
>> newer samples of the flash have the correct data.
> 
> Can we differentiate the Cypress flashes? Do you remember what
> was the incorrect data?
> 
>>
>> I don't know how many of those faulty flashes are out there in the wild.
>> IMO, to be on the safe side spi_nor_cypress_octal_dtr_enable() needs to
>> be implemented. So from the point of view of this series there is no
>> need to parse the Octal DDR enable table.
> 
> Meh, we cover manufacturer's mistakes. On the long run, our aim should be
> to follow the SFDP standard and if a flash implements it wrong, to either
> fix it via a fixup hook (if the fix is minimal), or to skip the faulty
> table.
> 
> Regarding "Command Sequences to Change to Octal DDR (8D-8D-8D) mode"
> table. Have you looked over
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-4-git-send-email-masonccyang@mxic.com.tw/
> ?
> Is there a standard way to determine the offsets of opcode, addr and
> data in the cmd seq?
> 

There is no standard way of doing this and I recommend against it.
Each cmd seq has 0 to 7 Bytes. So the sequence maybe:
1 cmd byte-3 addr bytes- 3 data bytes 
or 
1 cmd byte-0 address bytes-6 data bytes 

We could just assume first byte to be command and rest to be data bytes always,
but one problem maybe that controller may not support data length to be so long
when address phase is absent. Eg.: Cadence OSPI controller supports only upto 8 bytes
length transfers in absence of address phase but other controllers may
limit the length further?

One more drawback of using "Command Sequences to Change to Octal DDR (8D-8D-8D) 
mode" table is that it not only switches flash to 8D mode 
but also configures flash to be in:

- 50 ohm I/O driver strength (Driver Type 0, mandatory for xSPI devices) 

- 20 dummy cycles for Read Fast commands

- Operation at 100MHz (or higher, if supported)

Note that 20 dummy cycles may not be enough for flashes to 
operate at 166/200MHz or higher and thus requiring flash specific fixups.

So, I am beginning to doubt if parsing "Command Sequences to Change to Octal DDR (8D-8D-8D) mode"
table is worth it. Not to mention we still need flash specific code to "verify" 
that mode switch is indeed successful.

Regards
Vignesh