| Message ID | 2051977971.384388.1524012814487.JavaMail.zimbra@xes-inc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND] mtd: cfi: Support early CFI fixups | expand |
Hello Aaron, On Tue, 17 Apr 2018 19:53:34 -0500 (CDT) Aaron Sierra <asierra@xes-inc.com> wrote: > Some CFI devices need fixups that affect the number of chips detected, > but the current fixup infrastructure (struct cfi_fixup and cfi_fixup()) > does not cover this situation. > > Introduce struct cfi_early_fixup and cfi_early_fixup() to fill the void > along with the fixup for S70GL02GS. > > Without early fixups (top half of device cannot be written or erased): > ff0000000.nor-boot: Found 1 x16 devices at 0x0 in 16-bit bank. <snip> > Amd/Fujitsu Extended Query Table at 0x0040 > Amd/Fujitsu Extended Query version 1.5. > number of CFI chips: 1 > > With early fixups (entire device can be written and erased): > Bad S70GL02GS CFI data; adjust to detect 2 chips > ff0000000.nor-boot: Found 1 x16 devices at 0x0 in 16-bit bank. <snip> > ff0000000.nor-boot: Found 1 x16 devices at 0x8000000 in 16-bit bank > Amd/Fujitsu Extended Query Table at 0x0040 > Amd/Fujitsu Extended Query version 1.5. > number of CFI chips: 2 > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > --- > > * Resent due to mailing list bounce > * Compiled under 4.16. Tested under 4.1 and 3.12 (e5500 PowerPC) > > drivers/mtd/chips/cfi_probe.c | 18 ++++++++++++++++++ > drivers/mtd/chips/cfi_util.c | 13 +++++++++++++ > include/linux/mtd/cfi.h | 10 ++++++++++ > 3 files changed, 41 insertions(+) > > diff --git a/drivers/mtd/chips/cfi_probe.c b/drivers/mtd/chips/cfi_probe.c > index e8d0164..8b60aff 100644 > --- a/drivers/mtd/chips/cfi_probe.c > +++ b/drivers/mtd/chips/cfi_probe.c > @@ -151,6 +151,22 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base, > return 1; > } > > +static void fixup_s70gl02gs_chips(struct cfi_private *cfi) > +{ > + /* > + * S70GL02GS flash reports a single 256 MiB chip, but is really made up > + * of two 128 MiB chips with 1024 sectors each. > + */ > + cfi->cfiq->DevSize = 27; > + cfi->cfiq->EraseRegionInfo[0] = 0x20003ff; > + pr_warn("Bad S70GL02GS CFI data; adjust to detect 2 chips\n"); > +} > + > +static struct cfi_early_fixup cfi_early_fixup_table[] = { > + { CFI_MFR_AMD, 0x4801, fixup_s70gl02gs_chips }, > + { 0, 0, NULL }, > +}; > + Can you move these changes to a separate patch: one patch adding the early fixup infra, and another one making use of it for S70GL02GS. > static int __xipram cfi_chip_setup(struct map_info *map, > struct cfi_private *cfi) > { > @@ -235,6 +251,8 @@ static int __xipram cfi_chip_setup(struct map_info *map, > cfi_qry_mode_off(base, map, cfi); > xip_allowed(base, map); > > + cfi_early_fixup(cfi, cfi_early_fixup_table); > + > printk(KERN_INFO "%s: Found %d x%d devices at 0x%x in %d-bit bank. Manufacturer ID %#08x Chip ID %#08x\n", > map->name, cfi->interleave, cfi->device_type*8, base, > map->bankwidth*8, cfi->mfr, cfi->id); > diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c > index 6f16552..e25d858 100644 > --- a/drivers/mtd/chips/cfi_util.c > +++ b/drivers/mtd/chips/cfi_util.c > @@ -333,6 +333,19 @@ __xipram cfi_read_pri(struct map_info *map, __u16 adr, __u16 size, const char* n > > EXPORT_SYMBOL(cfi_read_pri); > > +void cfi_early_fixup(struct cfi_private *cfi, struct cfi_early_fixup *fixups) > +{ > + struct cfi_early_fixup *f; > + > + for (f = fixups; f->fixup; f++) { > + if (((f->mfr == CFI_MFR_ANY) || (f->mfr == cfi->mfr)) && > + ((f->id == CFI_ID_ANY) || (f->id == cfi->id))) { > + f->fixup(cfi); > + } > + } > +} > +EXPORT_SYMBOL(cfi_early_fixup); > + > void cfi_fixup(struct mtd_info *mtd, struct cfi_fixup *fixups) > { > struct map_info *map = mtd->priv; > diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h > index 9b57a9b..7d304db 100644 > --- a/include/linux/mtd/cfi.h > +++ b/include/linux/mtd/cfi.h > @@ -353,6 +353,15 @@ void __xipram cfi_qry_mode_off(uint32_t base, struct map_info *map, > > struct cfi_extquery *cfi_read_pri(struct map_info *map, uint16_t adr, uint16_t size, > const char* name); > + > +/* CFI fixup that can occur immediately after reading */ ^ "after reading the ID" ? > +struct cfi_early_fixup { > + uint16_t mfr; > + uint16_t id; > + void (*fixup)(struct cfi_private *cfi); > +}; > + > +/* CFI fixup that can only occur after MTD device exists */ > struct cfi_fixup { > uint16_t mfr; > uint16_t id; > @@ -380,6 +389,7 @@ struct cfi_fixup { > #define CFI_MFR_TOSHIBA 0x0098 > #define CFI_MFR_WINBOND 0x00DA > > +void cfi_early_fixup(struct cfi_private *cfi, struct cfi_early_fixup *fixups); Is cfi_early_fixup() intended to be used outside of cfi_probe.c? If that's not the case, I'd recommend to keep the struct and function def in cfi_probe.c. > void cfi_fixup(struct mtd_info *mtd, struct cfi_fixup* fixups); > > typedef int (*varsize_frob_t)(struct map_info *map, struct flchip *chip, Thanks, Boris
----- Original Message ----- > From: "Boris Brezillon" <boris.brezillon@bootlin.com> > Sent: Monday, April 30, 2018 5:39:56 AM > Subject: Re: [PATCH RESEND] mtd: cfi: Support early CFI fixups Boris, Thanks for your review. > Hello Aaron, > > On Tue, 17 Apr 2018 19:53:34 -0500 (CDT) > Aaron Sierra <asierra@xes-inc.com> wrote: > >> Some CFI devices need fixups that affect the number of chips detected, >> but the current fixup infrastructure (struct cfi_fixup and cfi_fixup()) >> does not cover this situation. >> >> Introduce struct cfi_early_fixup and cfi_early_fixup() to fill the void >> along with the fixup for S70GL02GS. >> >> Without early fixups (top half of device cannot be written or erased): >> ff0000000.nor-boot: Found 1 x16 devices at 0x0 in 16-bit bank. <snip> >> Amd/Fujitsu Extended Query Table at 0x0040 >> Amd/Fujitsu Extended Query version 1.5. >> number of CFI chips: 1 >> >> With early fixups (entire device can be written and erased): >> Bad S70GL02GS CFI data; adjust to detect 2 chips >> ff0000000.nor-boot: Found 1 x16 devices at 0x0 in 16-bit bank. <snip> >> ff0000000.nor-boot: Found 1 x16 devices at 0x8000000 in 16-bit bank >> Amd/Fujitsu Extended Query Table at 0x0040 >> Amd/Fujitsu Extended Query version 1.5. >> number of CFI chips: 2 >> >> Signed-off-by: Aaron Sierra <asierra@xes-inc.com> >> --- >> >> * Resent due to mailing list bounce >> * Compiled under 4.16. Tested under 4.1 and 3.12 (e5500 PowerPC) >> >> drivers/mtd/chips/cfi_probe.c | 18 ++++++++++++++++++ >> drivers/mtd/chips/cfi_util.c | 13 +++++++++++++ >> include/linux/mtd/cfi.h | 10 ++++++++++ >> 3 files changed, 41 insertions(+) >> >> diff --git a/drivers/mtd/chips/cfi_probe.c b/drivers/mtd/chips/cfi_probe.c >> index e8d0164..8b60aff 100644 >> --- a/drivers/mtd/chips/cfi_probe.c >> +++ b/drivers/mtd/chips/cfi_probe.c >> @@ -151,6 +151,22 @@ static int __xipram cfi_probe_chip(struct map_info *map, >> __u32 base, >> return 1; >> } >> >> +static void fixup_s70gl02gs_chips(struct cfi_private *cfi) >> +{ >> + /* >> + * S70GL02GS flash reports a single 256 MiB chip, but is really made up >> + * of two 128 MiB chips with 1024 sectors each. >> + */ >> + cfi->cfiq->DevSize = 27; >> + cfi->cfiq->EraseRegionInfo[0] = 0x20003ff; >> + pr_warn("Bad S70GL02GS CFI data; adjust to detect 2 chips\n"); >> +} >> + >> +static struct cfi_early_fixup cfi_early_fixup_table[] = { >> + { CFI_MFR_AMD, 0x4801, fixup_s70gl02gs_chips }, >> + { 0, 0, NULL }, >> +}; >> + > > Can you move these changes to a separate patch: one patch adding the > early fixup infra, and another one making use of it for S70GL02GS. Sure thing. [snip] >> --- a/include/linux/mtd/cfi.h >> +++ b/include/linux/mtd/cfi.h >> @@ -353,6 +353,15 @@ void __xipram cfi_qry_mode_off(uint32_t base, struct >> map_info *map, >> >> struct cfi_extquery *cfi_read_pri(struct map_info *map, uint16_t adr, uint16_t >> size, >> const char* name); >> + >> +/* CFI fixup that can occur immediately after reading */ > > ^ "after reading the ID" ? After reading the CFI structure out of the device, not just the ID. These fixups apply to the just-read, byte-swapped, in-memory representation of the CFI structure. Is more clarification necessary? >> +struct cfi_early_fixup { >> + uint16_t mfr; >> + uint16_t id; >> + void (*fixup)(struct cfi_private *cfi); >> +}; >> + >> +/* CFI fixup that can only occur after MTD device exists */ >> struct cfi_fixup { >> uint16_t mfr; >> uint16_t id; >> @@ -380,6 +389,7 @@ struct cfi_fixup { >> #define CFI_MFR_TOSHIBA 0x0098 >> #define CFI_MFR_WINBOND 0x00DA >> >> +void cfi_early_fixup(struct cfi_private *cfi, struct cfi_early_fixup *fixups); > > Is cfi_early_fixup() intended to be used outside of cfi_probe.c? If > that's not the case, I'd recommend to keep the struct and function def > in cfi_probe.c. It is not, but since this is adding a second fixup mechanism for CFI, I thought there may be value in defining both of them in close proximity to each other. That way anyone looking for available mechanisms would find both instead of only one or the other. Does that value outweigh it only being used in one file? -Aaron >> void cfi_fixup(struct mtd_info *mtd, struct cfi_fixup* fixups); >> >> typedef int (*varsize_frob_t)(struct map_info *map, struct flchip *chip, > > Thanks, > > Boris
On Mon, 30 Apr 2018 09:38:45 -0500 (CDT) Aaron Sierra <asierra@xes-inc.com> wrote: > >> --- a/include/linux/mtd/cfi.h > >> +++ b/include/linux/mtd/cfi.h > >> @@ -353,6 +353,15 @@ void __xipram cfi_qry_mode_off(uint32_t base, struct > >> map_info *map, > >> > >> struct cfi_extquery *cfi_read_pri(struct map_info *map, uint16_t adr, uint16_t > >> size, > >> const char* name); > >> + > >> +/* CFI fixup that can occur immediately after reading */ > > > > ^ "after reading the ID" ? > > After reading the CFI structure out of the device, not just the ID. These > fixups apply to the just-read, byte-swapped, in-memory representation of > the CFI structure. Is more clarification necessary? Yep, you should say after reading what. "after reading the CFI structure out of the device" sounds good. > > >> +struct cfi_early_fixup { > >> + uint16_t mfr; > >> + uint16_t id; > >> + void (*fixup)(struct cfi_private *cfi); > >> +}; > >> + > >> +/* CFI fixup that can only occur after MTD device exists */ > >> struct cfi_fixup { > >> uint16_t mfr; > >> uint16_t id; > >> @@ -380,6 +389,7 @@ struct cfi_fixup { > >> #define CFI_MFR_TOSHIBA 0x0098 > >> #define CFI_MFR_WINBOND 0x00DA > >> > >> +void cfi_early_fixup(struct cfi_private *cfi, struct cfi_early_fixup *fixups); > > > > Is cfi_early_fixup() intended to be used outside of cfi_probe.c? If > > that's not the case, I'd recommend to keep the struct and function def > > in cfi_probe.c. > > It is not, but since this is adding a second fixup mechanism for CFI, I > thought there may be value in defining both of them in close proximity to > each other. That way anyone looking for available mechanisms would find > both instead of only one or the other. Does that value outweigh it only > being used in one file? Well, we try to avoid exposing symbols when they're only used from the file itself. If someone ever needs a similar mechanism in a different file (cfi_jedec.c for example), we'll consider exposing it in cfi_util.c/cfi.h, but that's not the case yet, so let's keep it private to cfi_probe.c for now.
diff --git a/drivers/mtd/chips/cfi_probe.c b/drivers/mtd/chips/cfi_probe.c index e8d0164..8b60aff 100644 --- a/drivers/mtd/chips/cfi_probe.c +++ b/drivers/mtd/chips/cfi_probe.c @@ -151,6 +151,22 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base, return 1; } +static void fixup_s70gl02gs_chips(struct cfi_private *cfi) +{ + /* + * S70GL02GS flash reports a single 256 MiB chip, but is really made up + * of two 128 MiB chips with 1024 sectors each. + */ + cfi->cfiq->DevSize = 27; + cfi->cfiq->EraseRegionInfo[0] = 0x20003ff; + pr_warn("Bad S70GL02GS CFI data; adjust to detect 2 chips\n"); +} + +static struct cfi_early_fixup cfi_early_fixup_table[] = { + { CFI_MFR_AMD, 0x4801, fixup_s70gl02gs_chips }, + { 0, 0, NULL }, +}; + static int __xipram cfi_chip_setup(struct map_info *map, struct cfi_private *cfi) { @@ -235,6 +251,8 @@ static int __xipram cfi_chip_setup(struct map_info *map, cfi_qry_mode_off(base, map, cfi); xip_allowed(base, map); + cfi_early_fixup(cfi, cfi_early_fixup_table); + printk(KERN_INFO "%s: Found %d x%d devices at 0x%x in %d-bit bank. Manufacturer ID %#08x Chip ID %#08x\n", map->name, cfi->interleave, cfi->device_type*8, base, map->bankwidth*8, cfi->mfr, cfi->id); diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c index 6f16552..e25d858 100644 --- a/drivers/mtd/chips/cfi_util.c +++ b/drivers/mtd/chips/cfi_util.c @@ -333,6 +333,19 @@ __xipram cfi_read_pri(struct map_info *map, __u16 adr, __u16 size, const char* n EXPORT_SYMBOL(cfi_read_pri); +void cfi_early_fixup(struct cfi_private *cfi, struct cfi_early_fixup *fixups) +{ + struct cfi_early_fixup *f; + + for (f = fixups; f->fixup; f++) { + if (((f->mfr == CFI_MFR_ANY) || (f->mfr == cfi->mfr)) && + ((f->id == CFI_ID_ANY) || (f->id == cfi->id))) { + f->fixup(cfi); + } + } +} +EXPORT_SYMBOL(cfi_early_fixup); + void cfi_fixup(struct mtd_info *mtd, struct cfi_fixup *fixups) { struct map_info *map = mtd->priv; diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h index 9b57a9b..7d304db 100644 --- a/include/linux/mtd/cfi.h +++ b/include/linux/mtd/cfi.h @@ -353,6 +353,15 @@ void __xipram cfi_qry_mode_off(uint32_t base, struct map_info *map, struct cfi_extquery *cfi_read_pri(struct map_info *map, uint16_t adr, uint16_t size, const char* name); + +/* CFI fixup that can occur immediately after reading */ +struct cfi_early_fixup { + uint16_t mfr; + uint16_t id; + void (*fixup)(struct cfi_private *cfi); +}; + +/* CFI fixup that can only occur after MTD device exists */ struct cfi_fixup { uint16_t mfr; uint16_t id; @@ -380,6 +389,7 @@ struct cfi_fixup { #define CFI_MFR_TOSHIBA 0x0098 #define CFI_MFR_WINBOND 0x00DA +void cfi_early_fixup(struct cfi_private *cfi, struct cfi_early_fixup *fixups); void cfi_fixup(struct mtd_info *mtd, struct cfi_fixup* fixups); typedef int (*varsize_frob_t)(struct map_info *map, struct flchip *chip,
Some CFI devices need fixups that affect the number of chips detected, but the current fixup infrastructure (struct cfi_fixup and cfi_fixup()) does not cover this situation. Introduce struct cfi_early_fixup and cfi_early_fixup() to fill the void along with the fixup for S70GL02GS. Without early fixups (top half of device cannot be written or erased): ff0000000.nor-boot: Found 1 x16 devices at 0x0 in 16-bit bank. <snip> Amd/Fujitsu Extended Query Table at 0x0040 Amd/Fujitsu Extended Query version 1.5. number of CFI chips: 1 With early fixups (entire device can be written and erased): Bad S70GL02GS CFI data; adjust to detect 2 chips ff0000000.nor-boot: Found 1 x16 devices at 0x0 in 16-bit bank. <snip> ff0000000.nor-boot: Found 1 x16 devices at 0x8000000 in 16-bit bank Amd/Fujitsu Extended Query Table at 0x0040 Amd/Fujitsu Extended Query version 1.5. number of CFI chips: 2 Signed-off-by: Aaron Sierra <asierra@xes-inc.com> --- * Resent due to mailing list bounce * Compiled under 4.16. Tested under 4.1 and 3.12 (e5500 PowerPC) drivers/mtd/chips/cfi_probe.c | 18 ++++++++++++++++++ drivers/mtd/chips/cfi_util.c | 13 +++++++++++++ include/linux/mtd/cfi.h | 10 ++++++++++ 3 files changed, 41 insertions(+)