Re: [PATCH v2] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami <ikegami.t@gmail.com>]

Hi,

On 2022/03/15 18:50, Miquel Raynal wrote:
> Hello,
>
> vigneshr@ti.com wrote on Tue, 15 Mar 2022 11:41:52 +0530:
>
>> Hi Miquel,
>>
>> On 06/03/22 9:02 pm, Tokunori Ikegami wrote:
>>> The regression issue has been caused on S29GL064N and reported it.
>>> Also the change mentioned is to use chip_good() for buffered write.
>>> So disable the change on S29GL064N and use chip_ready() as before.
>>>
>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>> Cc: Richard Weinberger <richard@nod.at>
>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>> Cc: linux-mtd@lists.infradead.org
>>> Cc: stable@vger.kernel.org
>>> Link: https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>> Could you please pull this in for v5.18-rc1 as this is a regression
>> being tracked for a while?
> I was about to apply the patch but reviewing it quickly shown a number
> of issues. I can still pull the patch for -rc1 if all this is
> corrected rather quickly but otherwise it will go through Fixes later
> on.
>
> - I just don't understand the commit log, please rephrase it entirely.
>    You can include the link to the bug report as well inside the commit
>    text, not as part of a Link tag, because the Link tag is here to
>    refer to the mailing list discussion and is added automatically.
>    Something like:
>
> 	As pointed out by this bug report [1], <what> is now broken on
> 	S29GL064N. The reason is that <explain the problem>. One way to
> 	solve the issue is to <explain why I should choose this
> 	solution>.
>
> 	[1] <the link here>
Thanks. Just fixed by the version 3 patches and sent them.
> - The patch itself should be split into at least three parts:
>    * The ID definition (should not be part of the fix).
>    * The chip_check/chip_ready/chip_good_for_write code moves/renaming
>      independently of any functinal change as a preparation patch.
>    * The actual fix within one of the above functions.
This is also fixed by the version patches.
>
> Aside from these, I don't quite get what the __xipram keyword is
> supposed to carry. We return a boolean expression stored in an int
> provided by the map_word_equal() macro which does return a simple int
> declared on the stack. It's not stored anywhere else than regular RAM,
> or am I missing something?

The __xipram keyword was implemented by the commit 02b15e343aee for the 
chip_ready and chip_good, etc.
Also the definision is implemneted in include/linux/mtd/xip.h as below.

/*
  * Function that are modifying the flash state away from array mode must
  * obviously not be running from flash.  The __xipram is therefore marking
  * those functions so they get relocated to ram.
  */
#ifdef CONFIG_XIP_KERNEL
#define __xipram noinline __section(".xiptext")
#endif

The changes for the fix just follows the implementation as same I think.
By the way it was added to use the __xipram keyword for 
cfi_use_chip_ready_for_write() also.

Regards,
Ikegami

>
>> Acked-by: Vignesh Raghavendra <vigneshr@ti.com>
>>
>> [...]
>>
>> Regards
>> Vignesh
>>
> Thanks,
> Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/