Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

From: Vignesh Raghavendra
Date: Thu Apr 30 2020 - 04:22:26 EST


Hi Mason,

On 29/04/20 1:01 pm, masonccyang@xxxxxxxxxxx wrote:
>
> Hi Pratyush,
>
>
>>>>> On Tue, 21 Apr 2020 14:39:42 +0800
>>>>> Mason Yang <masonccyang@xxxxxxxxxxx> wrote:
[...]
>>>
> https://lore.kernel.org/linux-mtd/20200424184410.8578-1-p.yadav@xxxxxx/
>>>
>>>
>>> Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI
> profile
>>> 1.0,
>>> and it comply with BFPT DWORD-19, octal mode enable sequences by write
> CFG
>>> Reg2
>>> with instruction 0x72. Therefore, I can't apply your patches.
>>
>> I didn't mean apply my patches directly. I meant more along the lines of
>
>> edit your patches to work on top of my series. It should be as easy as
>> adding your flash's fixup hooks and its octal DTR enable hook, but if my
>
>> series is missing something you need (like complete Profile 1.0 parsing,
>
>> which I left out because I wanted to be conservative and didn't see any
>> immediate use-case for us), let me know, and we can work together to
>> address it.
>
> yes,sure!
> let's work together to upstream the Octal 8D-8D-8D driver to mainline.
>
> The main concern is where and how to enable xSPI octal mode?
>
> Vignesh don't agree to enable it in fixup hooks and that's why I patched
> it to spi_nor_late_init_params() and confirmed the device support xSPI
> Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
>

My suggestion was to use SFDP wherever possible.. E.g: it is possible to
get opcode extension type from BFPT...

But using BFPT DWORD-19 is not correct for switching to 8D-8D-8D mode:

Per JESD216D.01 Bits 22:20 of 19th DWORD of BFPT:

Octal Enable Requirements:

This field describes whether the device contains a Octal Enable bit used
to enable 1-1-8 and 1-
8-8 octal read or octal program operations.

So, this cannot be used for enabling 8D-8D-8D mode... Flashes that only
support 1S-1S-1S and 8D-8D-8D will set this field to 0.

There is a separate table to enable 8D mode called
"Command Sequences to Change to Octal DDR (8D-8D-8D) mode". But if flash
does not have the table or has bad data, fixup hook is the only way...

If mx25* supports above table, please build on top of Pratyush's series
to add support for parsing this table. Otherwise, macronix would have to
use a fixup hook too...

> I can't apply your patches to enable xSPI Octal mode for mx25uw51245g
> because your patches set up Octal protocol first and then using Octal
> protocol to write Configuration Register 2(CFG Reg2). I think driver
> should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make sure
> write CFG Reg 2 is success and then setup Octa protocol in the last.
>
> As JESD216F description on BFPT DOWRD 19th, only two way to enable
> xSPI Octal mode;

Where is JESD216F? Latest I can find is JESD216D.01

> one is by two instruction: issue instruction 06h(WREN) and then E8h.
> the other is issue instruction 06h, then issue instruction 72h (Write
> CFG Reg2), address 0h and data 02h (8D-8D-8D).
>
> Let our patches comply with this. you may refer to my patches
> [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D
> mode

As I pointed out earlier using above DWORDS seems wrong for 8D-8D-8D,
they can be used for 1-1-8 and 1-
8-8

>
> /* Octal mode enable sequences. */
> switch (bfpt.dwords[BFPT_DWORD(19)] &
> BFPT_DWORD19_OCTAL_SEQ_MASK) {
> case BFPT_DWORD19_TWO_INST:
> + ----> to patch here.
> break;
> case BFPT_DWORD19_CFG_REG2:
> params->xspi_enable =
> spi_nor_cfg_reg2_octal_enable;
> break;
> default:
> break;
> }
>
>
>>
>>> I quickly went through your patches but can't reply them in each your
>>> patches.
>>>
>>> i.e,.
>>> 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
>>>
>>> - u8 opcode;
>>> + u16 opcode;
>>>
>>> big/little Endian issue, right?

Is the big/little Endian issue a quirk of the flash or controller? If
its controller specific then it needs to handled in controller driver.

If this is a flash quirk, please point to the waveforms in the flash
datasheet...

>>> why not just u8 ext_opcode;
>>> No any impact for exist code and actually only xSPI device use
> extension
>>> command.
>>
>> Boris already explained the reasoning behind it.
>
> yup, I got his point and please make sure CPU data access.
>
> i.e,.
> Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
>
> and your patch,
> + ext = spi_nor_get_cmd_ext(nor, op);
> + op->cmd.opcode = (op->cmd.opcode << 8) |
> ext;
> + op->cmd.nbytes = 2;
>
> I think maybe using u8 opcode[2] could avoid endianness.
>
> Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
> please check his comments on
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1573808288-19365-3-git-send-email-masonccyang@xxxxxxxxxxx/
>
>
>
> Let's open this discussion and maybe Vighesh and Tudor could have some
> comments on it.
> thanks a lot.
>

Sorry , but others clearly see having single variable to store cmd +
extension is beneficial here. So, I take back my suggestion.

Regards
Vignesh