| Message ID | 20220315165607.390070-4-ikegami.t@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N | expand |
Hi Tokunori, ikegami.t@gmail.com wrote on Wed, 16 Mar 2022 01:56:07 +0900: > As pointed out by this bug report [1], the buffered write is now broken on , buffered writes are now broken > S29GL064N. The reason is that changed the buffered write to use chip_good > instead of chip_ready. "This issue comes from a rework which switched from using chip_good() to chip_ready(), because <explain the difference here>." [please note I am just trying to understand what the root cause is, please rephrase if I'm wrong]. > One way to solve the issue is to revert the change > partially to use chip_ready for S29GL064N since the way of least surprise. s/since the way of least surprise// > > [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/ > > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") > Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> > Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > 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 I think you can get rid of all the above Cc: tags and just copy all 3 of us + the mailing list when sending your v4. > Cc: stable@vger.kernel.org > --- Please also include a Fixes/stable tag in the patch before (2/3) to explain that both patches are required in order to fix the issue and the current patch alone won't apply. You should mention that with a nice comment below the three dashes ("---") in patch 2/3 as well. > drivers/mtd/chips/cfi_cmdset_0002.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index 8f3f0309dc03..fa11db066c99 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -867,10 +867,20 @@ static int __xipram chip_good(struct map_info *map, struct flchip *chip, > return chip_check(map, chip, addr, &expected); > } > > +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map) > +{ > + struct cfi_private *cfi = map->fldrv_priv; > + > + return cfi->mfr == CFI_MFR_AMD && cfi->id == S29GL064N_MN12; > +} > + > static int __xipram chip_good_for_write(struct map_info *map, > struct flchip *chip, unsigned long addr, > map_word expected) > { > + if (cfi_use_chip_ready_for_write(map)) > + return chip_ready(map, chip, addr); > + > return chip_good(map, chip, addr, expected); > } > This is much more understandable. Vignesh, perhaps it would be better to provide a way for manufacturers to overload certain callbacks instead of applying quirks like this in the code. But that will come in a second time of course. Thanks, Miquèl
Hi, On 2022/03/16 3:51, Miquel Raynal wrote: > Hi Tokunori, > > ikegami.t@gmail.com wrote on Wed, 16 Mar 2022 01:56:07 +0900: > >> As pointed out by this bug report [1], the buffered write is now broken on > , buffered writes are now broken > >> S29GL064N. The reason is that changed the buffered write to use chip_good >> instead of chip_ready. > "This issue comes from a rework which switched from using chip_good() > to chip_ready(), because <explain the difference here>." > > [please note I am just trying to understand what the root cause is, > please rephrase if I'm wrong]. Fixed by the version 4 patches. > >> One way to solve the issue is to revert the change >> partially to use chip_ready for S29GL064N since the way of least surprise. > s/since the way of least surprise// Fixed by the version 4 patches. > > >> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/ >> >> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") >> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> >> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> 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 > I think you can get rid of all the above Cc: tags and just copy all 3 > of us + the mailing list when sending your v4. Fixed by the version 4 patches. > >> Cc: stable@vger.kernel.org >> --- > Please also include a Fixes/stable tag in the patch before (2/3) to explain > that both patches are required in order to fix the issue and the current patch alone won't apply. > > You should mention that with a nice comment below the three dashes ("---") in patch 2/3 as well. > >> drivers/mtd/chips/cfi_cmdset_0002.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c >> index 8f3f0309dc03..fa11db066c99 100644 >> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >> @@ -867,10 +867,20 @@ static int __xipram chip_good(struct map_info *map, struct flchip *chip, >> return chip_check(map, chip, addr, &expected); >> } >> >> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map) >> +{ >> + struct cfi_private *cfi = map->fldrv_priv; >> + >> + return cfi->mfr == CFI_MFR_AMD && cfi->id == S29GL064N_MN12; >> +} >> + >> static int __xipram chip_good_for_write(struct map_info *map, >> struct flchip *chip, unsigned long addr, >> map_word expected) >> { >> + if (cfi_use_chip_ready_for_write(map)) >> + return chip_ready(map, chip, addr); >> + >> return chip_good(map, chip, addr, expected); >> } >> > This is much more understandable. > > Vignesh, perhaps it would be better to provide a way for manufacturers > to overload certain callbacks instead of applying quirks like this in > the code. But that will come in a second time of course. > > > Thanks, > Miquèl
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 8f3f0309dc03..fa11db066c99 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -867,10 +867,20 @@ static int __xipram chip_good(struct map_info *map, struct flchip *chip, return chip_check(map, chip, addr, &expected); } +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map) +{ + struct cfi_private *cfi = map->fldrv_priv; + + return cfi->mfr == CFI_MFR_AMD && cfi->id == S29GL064N_MN12; +} + static int __xipram chip_good_for_write(struct map_info *map, struct flchip *chip, unsigned long addr, map_word expected) { + if (cfi_use_chip_ready_for_write(map)) + return chip_ready(map, chip, addr); + return chip_good(map, chip, addr, expected); }
As pointed out by this bug report [1], the buffered write is now broken on S29GL064N. The reason is that changed the buffered write to use chip_good instead of chip_ready. One way to solve the issue is to revert the change partially to use chip_ready for S29GL064N since the way of least surprise. [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/ Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> 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 --- drivers/mtd/chips/cfi_cmdset_0002.c | 10 ++++++++++ 1 file changed, 10 insertions(+)