Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <a86b93c4-a507-47c7-bdce-4aed8dfcf929@linaro.org>
Date:   Thu, 5 Oct 2023 15:59:03 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] mtd: micron-st: enable lock/unlock for mt25qu512a
Content-Language: en-US
To:     SHUKLA Mamta Ramendra <mamta.shukla@leica-geosystems.com>,
        "pratyush@kernel.org" <pratyush@kernel.org>,
        "michael@walle.cc" <michael@walle.cc>,
        "miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
        "richard@nod.at" <richard@nod.at>,
        "vigneshr@ti.com" <vigneshr@ti.com>,
        "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     GEO-CHHER-bsp-development 
        <bsp-development.geo@leica-geosystems.com>
References: <20230705154942.3936658-1-mamta.shukla@leica-geosystems.com>
 <19800e51-a871-be9f-9eb5-5829237e2613@linaro.org>
 <a8271289-0611-4e37-cf37-0be19a85656b@leica-geosystems.com>
 <084ed945-7674-280f-5866-9238473a294d@leica-geosystems.com>
 <17989610-d069-40e2-9b4d-7ca6bdf2497e@linaro.org>
 <a24d7b3b-4077-9dbe-49ae-96f595b42223@leica-geosystems.com>
 <2d7271b3-dd60-44bb-9700-f6a5295ea873@linaro.org>
 <372046cb-9135-9a6c-fdb7-307a42f55b30@leica-geosystems.com>
From:   Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <372046cb-9135-9a6c-fdb7-307a42f55b30@leica-geosystems.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 10/5/23 15:21, SHUKLA Mamta Ramendra wrote:
> 
> 
> On 05.10.23 15:51, Tudor Ambarus wrote:
>> [Some people who received this message don't often get email from tudor.ambarus@linaro.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>
>>
>> On 10/5/23 10:21, SHUKLA Mamta Ramendra wrote:
>>
>> cut
>>
>>>>
>>>> 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.
>>
>> would you please detail what exact definitions you used in case 2? Send
>> us the diff please.
> Case 2: Adding Flags for BP
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c 
> b/drivers/mtd/spi-nor/micron-st.c
> index 4afcfc57c896..6c8cabbead2e 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,
>                  .size = SZ_64M,
>                  .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | 
> SPI_NOR_QUAD_READ,
>                  .fixup_flags = SPI_NOR_4B_OPCODES,
> 
> 

Thanks. Nothing obvious on a first look. I looked at the sfdp dump, it
looks like 4BAIT table is missing, so you'll probably need:
	.fixup_flags = SPI_NOR_4B_OPCODES,

I don't see how this could affect BP, but it is worth to test
incremental changes and find out what misses.

After you test the above, I'd like you to extend the patch with:
	.size = SZ_64M,
Check if it works and send us the output of mtd_debug info /dev/mtd0 here.

Also you could enable dev_dbg to see where you get -EIO. Probably when
reading the SR back. Also you can use debugfs to check what is set in
the working scenario and what params are different in the non-working
case. See drivers/mtd/spi-nor/debugfs.c

Cheers,
ta


> -----------------------------------------------------------------
> Case 1: BP Flags and removed size, and no_sfdp so by default expecting 
> to read SFDP
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c 
> b/drivers/mtd/spi-nor/micron-st.c
> index 6c8cabbead2e..4feb03ee2d13 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -407,9 +407,6 @@ static const struct flash_info st_nor_parts[] = {
>                  .name = "mt25qu512a",
>                  .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | 
> SPI_NOR_4BIT_BP |
>                          SPI_NOR_BP3_SR_BIT6,
> -               .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,
>          }, {
> 
> 
> 
>>
>> Cheers,
>> ta
>>
>>> 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