Re: [PATCH v2] mtd: micron-st: enable lock/unlock for mt25qu512a

From: SHUKLA Mamta Ramendra
Date: Thu Oct 05 2023 - 10:05:20 EST

Next message: kernel test robot: "drivers/scsi/mpi3mr/mpi3mr_transport.c:2365 mpi3mr_report_tgtdev_to_sas_transport() warn: inconsistent indenting"
Previous message: Jens Wiklander: "Re: [PATCH v3 07/17] firmware: arm_ffa: Implement the NOTIFICATION_INFO_GET interface"
Next in thread: Tudor Ambarus: "Re: [PATCH v2] mtd: micron-st: enable lock/unlock for mt25qu512a"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello Tudor,

Thanks for your Feedback.

I rebased the patch on spi-nor/next
[https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/commit/?h=spi-nor/next&id=8f407eda173f1d43466636314c7aa30405e4dd67]
to align with new flash_info format.

On 21.09.23 16:31, Tudor Ambarus wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> Hi, Shukla,
>
> Sorry, this email somehow got off my radar.
>
> On 8/22/23 13:16, SHUKLA Mamta Ramendra wrote:
>>
>> On 14.07.23 10:15, SHUKLA Mamta Ramendra wrote:
>>> Hello Tudor,
>>>
>>> On 13.07.23 05:43, Tudor Ambarus wrote:
>>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>>
>>>>
>>>> On 05.07.2023 18:49, Mamta Shukla wrote:
>>>>> mt25qu512a[1] supports locking/unlocking through BP bits in SR.
>>>>>
>>>>> Tested using mtd-utils- flash_lock/flash_unlock for MT25QU512ABB8E12.
>>>>>
>>>>> Link: https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_512_abb_0.pdf?rev=b259aadc3bea49ea8210a41c9ad58211
>>>>> Signed-off-by: Mamta Shukla <mamta.shukla@xxxxxxxxxxxxxxxxxxxx>
>>>>> ---
>>>>> Changes in v2:
>>>>> - add Link tag
>>>>> - fix chip part number mt25ql512a->mt25qu512a
>>>>>
>>>>> drivers/mtd/spi-nor/micron-st.c | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
>>>>> index 4b919756a205..08e94340ebaa 100644
>>>>> --- a/drivers/mtd/spi-nor/micron-st.c
>>>>> +++ b/drivers/mtd/spi-nor/micron-st.c
>>>>> @@ -229,6 +229,8 @@ static const struct flash_info st_nor_parts[] = {
>>>>> MFR_FLAGS(USE_FSR)
>>>>> },
>>>>> { "mt25qu512a", INFO6(0x20bb20, 0x104400, 64 * 1024, 1024)
>>>>> + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
>>>>> + SPI_NOR_BP3_SR_BIT6)
>>>>> NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>>>>> FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
>>>>> MFR_FLAGS(USE_FSR)
>>>>
>>>> Can you try the following instead? We try to use SFDP parsing whenever
>>>> possible.
>>>> { "mt25qu512a", INFO6(0x20bb20, 0x104400, 0, 0)
>>>> FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
>>>> SPI_NOR_BP3_SR_BIT6)
>>>> PARSE_SFDP
>>>> MFR_FLAGS(USE_FSR)
>>>
>>>
>>> I tested with SFDP Parsing Flag. It works fine.
>>>
>>> ---------------------------------------------------------------------------------
>>> [ 214.726090] ACPI: Host-directed Dynamic ACPI Table Load:
>>> [ 214.731482] ACPI: SSDT 0xFFFF892882D89800 0000EC (v02 ALASKA MT25QU
>>> 00001000 INTL 20190509)
>>> [ 214.766082] spi-nor spi-PRP0001:00: mt25qu512a (65536 Kbytes)
>>>
>>>
>>> cat /sys/bus/spi/devices/spi-PRP0001:00/spi-nor/jedec_id
>>> 20bb20104400
>>>
>>> cat /sys/bus/spi/devices/spi-PRP0001:00/spi-nor/manufacturer
>>> st
>>>
>>> cat /sys/bus/spi/devices/spi-PRP0001:00/spi-nor/partname
>>> mt25qu512a
>>>
>>> xxd -p /sys/bus/spi/devices/spi-PRP0001:00/spi-nor/sfdp
>>> 53464450060101ff00060110300000ff84000102800000ffffffffffffff
>>> ffffffffffffffffffffffffffffffffffffe520fbffffffff1f29eb276b
>>> 273b27bbffffffffffff27bbffff29eb0c2010d80f520000244a99008b8e
>>> 03e1ac0127387a757a75fbbdd55c4a0f82ff81bd3d36ffffffffffffffff
>>> ffffffffffffffffffe7ffff21dcffff
>>>
>>> md5sum /sys/bus/spi/devices/spi-PRP0001:00/spi-nor/sfdp
>>> 610efba1647e00ac6db18beb11e84c04
>>> /sys/bus/spi/devices/spi-PRP0001:00/spi-nor/sfdp
>>>
>>> dd if=/dev/urandom of=/tmp/qspi_write bs=1M count=1
>>> 1+0 records in
>>> 1+0 records out
>>> 1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0221391 s, 47.4 MB/s
>>>
>>> mtd_debug write /dev/mtd0 0 1048576 /tmp/qspi_write
>>> Copied 1048576 bytes from /tmp/qspi_write to address 0x00000000 in flash
>>>
>>> mtd_debug erase /dev/mtd0 0 1048576
>>> Erased 1048576 bytes from address 0x00000000 in flash
>>>
>>> mtd_debug read /dev/mtd0 0 1048576 /tmp/qspi_read
>>> Copied 1048576 bytes from address 0x00000000 in flash to /tmp/qspi_read
>>>
>>> hexdump /tmp/qspi_read
>>> 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
>>> *
>>> 0100000
>>>
>>> mtd_debug write /dev/mtd0 0 1048576 /tmp/qspi_write
>>> Copied 1048576 bytes from /tmp/qspi_write to address 0x00000000 in flash
>>>
>>> mtd_debug read /dev/mtd0 0 1048576 /tmp/qspi_read
>>> Copied 1048576 bytes from address 0x00000000 in flash to /tmp/qspi_read
>>>
>>> sha1sum /tmp/qspi_write /tmp/qspi_read
>>> 4fe4e71b11f44e9672bd49e2e32c0fd94da4feb6 /tmp/qspi_write
>>> 4fe4e71b11f44e9672bd49e2e32c0fd94da4feb6 /tmp/qspi_read
>>>
>>> ----------------------------------------------------------------------------------
>>>
>>> Shall I write a new commit for PARSE_SFDP Flag or update this commit as-
>>> "Modify mt25qu512a Flags" ?
>>
>>
>> Ping! Just to follow up on this.
>>
>
> Yeah, please make 2 commits so that we can cleanly revert the switching
> to SFDP, if ever needed.
>
> So the first commit will look like:
> diff --git a/drivers/mtd/spi-nor/micron-st.c
> b/drivers/mtd/spi-nor/micron-st.c
> index 4afcfc57c896..a8da1f18e335 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -405,9 +405,6 @@ static const struct flash_info st_nor_parts[] = {
> }, {
> .id = SNOR_ID(0x20, 0xbb, 0x20, 0x10, 0x44, 0x00),
> .name = "mt25qu512a",
> - .size = SZ_64M,
> - .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ |
> SPI_NOR_QUAD_READ,
> - .fixup_flags = SPI_NOR_4B_OPCODES,
> .mfr_flags = USE_FSR,
> }, {
> .id = SNOR_ID(0x20, 0xbb, 0x20),
>
> and the second one will add just the BP support, something like:
> diff --git a/drivers/mtd/spi-nor/micron-st.c
> b/drivers/mtd/spi-nor/micron-st.c
> index a8da1f18e335..fdafbfa0f936 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -405,6 +405,8 @@ static const struct flash_info st_nor_parts[] = {
> }, {
> .id = SNOR_ID(0x20, 0xbb, 0x20, 0x10, 0x44, 0x00),
> .name = "mt25qu512a",
> + .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> SPI_NOR_4BIT_BP |
> + SPI_NOR_BP3_SR_BIT6,
> .mfr_flags = USE_FSR,
> }, {
> .id = SNOR_ID(0x20, 0xbb, 0x20),
>
> Of course, I expect you to run again the mtd_debug tests and also verify
> the locking. Thanks!

I applied both changes as mentioned above i.e
1] Switch to SFDP and 2] Using BP Flags.

Case 1: Use BP Flags and Switch to SFDP
With both these changes, the lock/unlock doesn't work.

## x86-64-smarc-evk-uwd0j0007 # uname -r
6.6.0-rc2

# flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x4000000
Lock status: unlocked
Return code: 0
# flash_lock -l /dev/mtd0
flash_lock: error!: could not lock device: /dev/mtd0

error 5 (Input/output error)

I suspected this is because of miscalculation of BP bits, like the
possibility mentioned here:
https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/commit/?h=spi-nor/next&id=3ea3f0ac242c86c0275d347ab8c92bf1eb854b49

But further checked size, it is correct:

# mtd_debug info /dev/mtd0
mtd.type = MTD_NORFLASH
mtd.flags = MTD_CAP_NORFLASH
mtd.size = 67108864 (64M)
mtd.erasesize = 4096 (4K)
mtd.writesize = 1
mtd.oobsize = 0
regions = 0

And rest of read/write functions work as expected.

Any suggestions about this?

Case 2: Just added BP flags, rest of the size, mfr_flags, fixup flags
kept as it is.
Lock/unlock works.

## x86-64-smarc-evk-uwd0j0007 # uname -r
6.6.0-rc2

# flash_lock -i /dev/mtd0
Start: 0
Len: 0x4000000
Lock status: unlocked
Return code: 0

# flash_lock -l /dev/mtd0
# flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x4000000
Lock status: locked
Return code: 1

## x86-64-smarc-evk-uwd0j0007 # mtd_debug erase /dev/mtd0 0 1048576
[ 413.472411] spi-nor spi-PRP0001:00: Erase operation failed.
[ 413.478084] spi-nor spi-PRP0001:00: Attempted to modify a protected
sector.
MEMERASE: Input/output error

# flash_lock -u /dev/mtd0
# flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x4000000
Lock status: unlocked
Return code: 0

## x86-64-smarc-evk-uwd0j0007 # mtd_debug erase /dev/mtd0 0 1048576
Erased 1048576 bytes from address 0x00000000 in flash

Further I tested on stable 6.5.5 Kernel with old way of Flash Info
Format and which has forced PARSE_SFDP Flag, no issues with lock/unlock.

---
Mamta

Next message: kernel test robot: "drivers/scsi/mpi3mr/mpi3mr_transport.c:2365 mpi3mr_report_tgtdev_to_sas_transport() warn: inconsistent indenting"
Previous message: Jens Wiklander: "Re: [PATCH v3 07/17] firmware: arm_ffa: Implement the NOTIFICATION_INFO_GET interface"
Next in thread: Tudor Ambarus: "Re: [PATCH v2] mtd: micron-st: enable lock/unlock for mt25qu512a"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]