Re: [PATCH] mtd: spi-nor: macronix: workaround for device id re-use

[Esben Haabendal <esben@geanix.com>]

"Michael Walle" <mwalle@kernel.org> writes:

>> >> + */
>> >> +static int
>> >> +mx25l3205d_late_init(struct spi_nor *nor)
>> >> +{
>> >> +	struct spi_nor_flash_parameter *params = nor->params;
>> >> +
>> >> +	/*                          DREAD  2READ  QREAD  4READ
>> >> +	 *                          1-1-2  1-2-2  1-1-4  1-4-4
>> >> +	 * Before SFDP parse          1      0      1      0
>> >> +	 * 3206e after SFDP parse     1      0      0      0
>> >> +	 * 3233f after SFDP parse     1      1      1      1
>> >> +	 * 3205d after this func      0      1      0      0
>> >> +	 */
>> >> +	if ((params->hwcaps.mask & SNOR_HWCAPS_READ_1_1_4) &&
>> >> +	    !(params->hwcaps.mask & SNOR_HWCAPS_READ_1_4_4)) {
>> >> +		/* Should be MX25L3205D */
>> >> +		params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_2;
>> >> +		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_1_2],
>> >> +					  0, 0, 0, 0);
>> >> +		params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_4;
>> >> +		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_1_4],
>> >> +					  0, 0, 0, 0);
>> >> +		params->hwcaps.mask |= SNOR_HWCAPS_READ_1_2_2;
>> >> +		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_2_2],
>> >> +					  0, 4, SPINOR_OP_READ_1_2_2,
>> >> +					  SNOR_PROTO_1_2_2);
>> >> +	}
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static const struct spi_nor_fixups mx25l3205d_fixups = {
>> >> +	.late_init = mx25l3205d_late_init,
>> >> +};
>> >> +
>> >>  static int
>> >>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>> >>  			    const struct sfdp_parameter_header *bfpt_header,
>> >> @@ -61,7 +118,8 @@ static const struct flash_info macronix_nor_parts[] = {
>> >>  		.id = SNOR_ID(0xc2, 0x20, 0x16),
>> >>  		.name = "mx25l3205d",
>> >>  		.size = SZ_4M,
>> >> -		.no_sfdp_flags = SECT_4K,
>> >> +		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>> >> +		.fixups = &mx25l3205d_fixups
>> >>  	}, {
>> >>  		.id = SNOR_ID(0xc2, 0x20, 0x17),
>> >>  		.name = "mx25l6405d",
>> >>
>>
>> If all support 1-1-2, (seems MX25L3205D doesn't), then we may have a
>> change to don't update the core.
>>
>> Frankly I don't care too much about what happens in the manufacturer
>> drivers, but I do care about the core and to not extend it with . This
>> patch is not too heavy to be unmaintainable and shows clear where the
>> problem is, we can keep this as well.
>
> It's a horrible hack. For example I'm working on a patch to clean up
> the spi_nor_set_read_settings() handling. So just throwing any code
> into vendor drivers doesn't make it any better in terms of
> maintainability. I'd need to touch all the code anyway. In fact it
> makes it even worse, because it looks like the manufacturer drivers
> are just a dumping ground for bad things. Thus, I'd really have it
> handled in a correct way inside the core.
>
> Also, this is not device specific. Let there be two different
> flashes with the same ID, but one support SFDP and one doesn't.
> Right now, you have to have any of the magic flags (dual, quad,
> etc) set to trigger an SFDP parsing. If the flash without SFDP
> doesn't support any of these, like in this case, we are screwed.
> Hence we might need such a flag also for other flashes.

Also, it is not very obvious that you can trigger SFDP parsing by
setting these dual/quad bits.  Having an explicit flag to cause this
behvaiour would be much better IMHO.

>> Other option that I'd like you to consider is whether we just remove
>> support for MX25L3205D, thus the entry altogether, and instead rely on
>> SFDP to set everything.
>
> Well, this will break boards with this flash :) And we don't know if
> there are any.