[PATCH RESEND] mtd: cfi: Support early CFI fixups

[Aaron Sierra]

----- Original Message -----
> From: "Boris Brezillon" <boris.brezillon at 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 at 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 at 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