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

[Vignesh Raghavendra <vigneshr@xxxxxx>]

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