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

[Michael Walle]

> >> + */
> >> +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.

> 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.

-michael

Attachment: signature.asc
Description: PGP signature