RE: [PATCH V7] mtd: spi-nor: winbond: add support for W25Q512NW-IM

[Sajida Bhanu (Temp) (QUIC)]

Hi Tudor,

Thanks for the review.

Please find the inline comments.

Thanks,
Sajida
-----Original Message-----
From: Tudor.Ambarus@xxxxxxxxxxxxx <Tudor.Ambarus@xxxxxxxxxxxxx> 
Sent: Friday, February 25, 2022 10:49 PM
To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@xxxxxxxxxxx>; dianders@xxxxxxxxxxxx; michael@xxxxxxxx; p.yadav@xxxxxx; miquel.raynal@xxxxxxxxxxx; richard@xxxxxx; vigneshr@xxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Cc: Sahitya Tummala (QUIC) <quic_stummala@xxxxxxxxxxx>; Veerabhadrarao Badiganti (QUIC) <quic_vbadigan@xxxxxxxxxxx>; Ram Prakash Gupta (QUIC) <quic_rampraka@xxxxxxxxxxx>; Pradeep Pragallapati (QUIC) <quic_pragalla@xxxxxxxxxxx>; Sarthak Garg (QUIC) <quic_sartgarg@xxxxxxxxxxx>
Subject: Re: [PATCH V7] mtd: spi-nor: winbond: add support for W25Q512NW-IM

Hi,

On 2/11/22 13:24, Shaik Sajida Bhanu wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> Add support for winbond W25Q512NW-IM chip.
> 
> Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@xxxxxxxxxxx>
> Reviewed-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> ---
> localhost / # cat 
> /sys/bus/platform/devices/soc\@0/88dc000.spi/spi_master/spi16/
> spi16.0/spi-nor/jedec_id
> ef8020
> 
> localhost / # cat 
> /sys/bus/platform/devices/soc\@0/88dc000.spi/spi_master/spi16/
> spi16.0/spi-nor/manufacturer
> winbond
> 
> localhost / # cat 
> /sys/bus/platform/devices/soc\@0/88dc000.spi/spi_master/spi16/
> spi16.0/spi-nor/partname
> w25q512nwm
> 
> localhost / # hexdump 
> /sys/bus/platform/devices/soc\@0/88dc000.spi/spi_master/sp
> i16/spi16.0/spi-nor/sfdp
> 0000000 4653 5044 0106 ff01 0600 1001 0080 ff00
> 0000010 0084 0201 00d0 ff00 ffff ffff ffff ffff
> 0000020 6800 6c65 6f6c 7720 726f 646c ffff ffff
> 0000030 ffff ffff ffff ffff ffff ffff ffff ffff
> *
> 0000080 20e5 fffb ffff 1fff eb44 6b08 3b08 bb42
> 0000090 fffe ffff ffff 0000 ffff eb40 200c 520f
> 00000a0 d810 0000 0233 00a6 e781 d914 63e9 3376
> 00000b0 757a 757a bdf7 5cd5 f719 ff5d 70e9 a5f9
> 00000c0 ffff ffff ffff ffff ffff ffff ffff ffff
> 00000d0 0aff fff0 ff21 ffdc
> 00000d8
> 
> localhost / # md5sum 
> /sys/bus/platform/devices/soc\@0/88dc000.spi/spi_master/spi
> 16/spi16.0/spi-nor/sfdp
> 106d89d6c049110bc94c01517cb4ce24 
> /sys/bus/platform/devices/soc@0/88dc000.spi/
> spi_master/spi16/spi16.0/spi-nor/sfdp
> 
> Changes since V1:
>         - Added space before name of the flash part as suggested by Doug.
> 
> Changes since V2:
>         - Updated chip name as w25q512nwm as suggested by Doug.
> 
> Changes since V3:
>         - Updated flash_info flags passing according to below patch.
> 
> Changes since V4:
>         - Added OTP support for SPI card as suggested by Michael Walle.
>         - Updated SFDP flags passing as suggested by Pratyush Yadav.
> 
> Changes since V5:
>         - Reordered flags passing info for spi nor as suggested by
>           Michael Walle.
>         - Added SFDP dump info in commit as suggested by Michael Walle.
> 
> Changes since V6:
>         - Updated commit subject and added md5sum in commit as suggested
>           by Michael Walle.
>         - Reordered flags passing info for spi nor which has missed in
>           V5 patch as suggested by Michael Walle.
> ---
>  drivers/mtd/spi-nor/winbond.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/winbond.c 
> b/drivers/mtd/spi-nor/winbond.c index 675f32c..6fde2ac 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -124,6 +124,10 @@ static const struct flash_info winbond_parts[] = {
>         { "w25m512jv", INFO(0xef7119, 0, 64 * 1024, 1024)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ |
>                               SPI_NOR_DUAL_READ) },
> +       { "w25q512nwm", INFO(0xef8020, 0, 64 * 1024, 1024)
> +               PARSE_SFDP
> +               FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)

This flash has 3BP support, was locking tested? I would like you to describe in the commit message how the flash was tested, and add in the comment sections proofs on how it was tested.

https://www.winbond.com/resource-files/W25Q512NW%20RevB%2007192021.pdf

Cheers,
Ta

> I have tried to validate lock/unlock feature on w25q512nwm card. But looks like the lock is not working as expected, so when we do lock the whole card and tried to write, write was successful without any errors.
   Similarly I also tried to erase lock protected area, it was successful without any issues.
   Below are the experiments I tried.

  localhost ~ # flash_lock -i /dev/mtd0
  Device: /dev/mtd0
  Start: 0
  Len: 0x4000000
  Lock status: locked
  Return code: 1
  localhost ~ # mtd_debug erase /dev/mtd0 0x400000 4096
  Erased 4096 bytes from address 0x00400000 in flash
  localhost ~ # mtd_debug read /dev/mtd0 0x400000 4096 temp
  Copied 4096 bytes from address 0x00400000 in flash to temp
  localhost ~ # mtd_debug write /dev/mtd0 0x400000 4096 temp
  Copied 4096 bytes from temp to address 0x00400000 in flash
  localhost ~ # flash_lock -u /dev/mtd0
  localhost ~ # flash_lock -i /dev/mtd0
  Device: /dev/mtd0
  Start: 0
  Len: 0x4000000
  Lock status: unlocked
  Return code: 0
  localhost ~ # mtd_debug erase /dev/mtd0 0x400000 4096
  Erased 4096 bytes from address 0x00400000 in flash
  localhost ~ # mtd_debug read /dev/mtd0 0x400000 4096 temp
  Copied 4096 bytes from address 0x00400000 in flash to temp
  localhost ~ # mtd_debug write /dev/mtd0 0x400000 4096 temp
  Copied 4096 bytes from temp to address 0x00400000 in flash
  localhost ~ #

  Could you please let me know is this expected behavior in this specific card.

  Thanks,
  Sajida

> +               OTP_INFO(256, 3, 0x1000, 0x1000) },
>         { "w25q512jvq", INFO(0xef4020, 0, 64 * 1024, 1024)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
>                               SPI_NOR_QUAD_READ) },
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
> member of Code Aurora Forum, hosted by The Linux Foundation
>