| Message ID | 20100424155812.14723.66554.stgit@shiryu.yomgui.biz |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2010-04-24 at 17:58 +0200, Guillaume LECERF wrote: > From: Uwe Kleine-König <Uwe.Kleine-Koenigi@digi.com> > > After the deleted block bootloc is only used once as follows: > > if (bootloc == 3 && something_else) { > ... > > So setting bootloc = 2 doesn't change anything. Taking that the warning is > wrong and missleading. > > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com> > Signed-off-by: Guillaume LECERF <glecerf@gmail.com> > Acked-by: Christopher Moore <moore@free.fr> Pushed this v5 series to l2-mtd-2.6 / dunno. But please, if you re-send the series, use consistent "mtd: cfi_cmdset_0002:" prefix for this patch. Then I will be able to do git-am without manual work. Now I have to tweak this patch because git am removes [MTD].
2010/4/29 Artem Bityutskiy <dedekind1@gmail.com>: > Pushed this v5 series to l2-mtd-2.6 / dunno. But please, if you re-send > the series, use consistent "mtd: cfi_cmdset_0002:" prefix for this > patch. Then I will be able to do git-am without manual work. Now I have > to tweak this patch because git am removes [MTD]. Thanks for pushing it. And I take your advice into consideration for my next series.
Hi. 2010/4/29 Artem Bityutskiy <dedekind1@gmail.com>: > Pushed this v5 series to l2-mtd-2.6 / dunno. But please, if you re-send > the series, use consistent "mtd: cfi_cmdset_0002:" prefix for this > patch. Then I will be able to do git-am without manual work. Now I have > to tweak this patch because git am removes [MTD]. David, could you please pick this series and push it to mtd-2.6.git before the merge window ? Thanks.
On Sat, 2010-04-24 at 17:58 +0200, Guillaume LECERF wrote: > From: Uwe Kleine-König <Uwe.Kleine-Koenigi@digi.com> > > After the deleted block bootloc is only used once as follows: > > if (bootloc == 3 && something_else) { > ... > > So setting bootloc = 2 doesn't change anything. Taking that the > warning is wrong and missleading. > > Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com> > Signed-off-by: Guillaume LECERF <glecerf@gmail.com> > Acked-by: Christopher Moore <moore@free.fr> The warning should be preserved for invalid values, or values we don't support. I don't think it's correct to just remove it. Looking at the latest datasheet, it looks like values of 0x04 or 0x05 are acceptable, meaning "Uniform, bottom WP protect" and "Uniform, top WP protect" respectively. Zero is acceptable only for pre-CFI 1.1 devices, and in that case it says "refer to device ID code for Top/Bottom boot version of AM29LV160 or AM29LV116". But perhaps we should just probe such devices using the JEDEC mode anyway.
Le 14/05/2010 02:19, David Woodhouse a écrit : > On Sat, 2010-04-24 at 17:58 +0200, Guillaume LECERF wrote: > >> From: Uwe Kleine-König<Uwe.Kleine-Koenigi@digi.com> >> >> After the deleted block bootloc is only used once as follows: >> >> if (bootloc == 3&& something_else) { >> ... >> >> So setting bootloc = 2 doesn't change anything. Taking that the >> warning is wrong and missleading. >> >> Signed-off-by: Uwe Kleine-König<Uwe.Kleine-Koenig@digi.com> >> Signed-off-by: Guillaume LECERF<glecerf@gmail.com> >> Acked-by: Christopher Moore<moore@free.fr> >> > The warning should be preserved for invalid values, or values we don't > support. I don't think it's correct to just remove it. > > But, David, the only effect of this code is to print an *erroneous* message. The message says "Assuming top" but sets bootloc to bottom (2) :( See this thread: http://thread.gmane.org/gmane.linux.drivers.mtd/22176 Remark: this would be even clearer if the bootloc values were macros instead of 2, 3, ... If you really want to keep the message then it must at least be changed to "Assuming bottom". Cheers, Chris
On Fri, 2010-05-14 at 08:17 +0200, Chris Moore wrote: > Le 14/05/2010 02:19, David Woodhouse a écrit : > > On Sat, 2010-04-24 at 17:58 +0200, Guillaume LECERF wrote: > > > >> From: Uwe Kleine-König<Uwe.Kleine-Koenigi@digi.com> > >> > >> After the deleted block bootloc is only used once as follows: > >> > >> if (bootloc == 3&& something_else) { > >> ... > >> > >> So setting bootloc = 2 doesn't change anything. Taking that the > >> warning is wrong and missleading. > >> > >> Signed-off-by: Uwe Kleine-König<Uwe.Kleine-Koenig@digi.com> > >> Signed-off-by: Guillaume LECERF<glecerf@gmail.com> > >> Acked-by: Christopher Moore<moore@free.fr> > >> > > The warning should be preserved for invalid values, or values we don't > > support. I don't think it's correct to just remove it. > > > > > > But, David, the only effect of this code is to print an *erroneous* message. > The message says "Assuming top" but sets bootloc to bottom (2) :( > See this thread: http://thread.gmane.org/gmane.linux.drivers.mtd/22176 > Remark: this would be even clearer if the bootloc values were macros > instead of 2, 3, ... > > If you really want to keep the message then it must at least be changed > to "Assuming bottom". http://git.infradead.org/mtd-2.6.git/commitdiff/412da2f6
2010/5/14 David Woodhouse <dwmw2@infradead.org>:
> http://git.infradead.org/mtd-2.6.git/commitdiff/412da2f6
Thanks David.
One thing I noted :
+ if ((bootloc < 2) || (bootloc > 5)) {
+ printk(KERN_WARNING "%s: CFI contains
unrecognised boot "
+ "bank location (%d). Assuming bottom.\n",
+ bootloc, map->name);
I think bootloc and map->name need to be swapped to match the printk
pattern order (%s first then %d).
On Fri, 2010-05-14 at 10:09 +0200, Guillaume LECERF wrote: > I think bootloc and map->name need to be swapped to match the printk > pattern order (%s first then %d). Doh. Fixed; thanks.
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index ea2a7f6..8da8655 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -391,11 +391,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) #endif bootloc = extp->TopBottom; - if ((bootloc != 2) && (bootloc != 3)) { - printk(KERN_WARNING "%s: CFI does not contain boot " - "bank location. Assuming top.\n", map->name); - bootloc = 2; - } if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) { printk(KERN_WARNING "%s: Swapping erase regions for broken CFI table.\n", map->name);