Re: [PATCH v3 03/23] mtd: spi-nor: Refactor Read Status/Write Status support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello Michael,

>> There will probably be breakages on older chips. These cannot be
>> guessed because they are not properly listed in manufacturer
>> fixups (yet). If we want a cleanup/simplification, we will have to cope
>> with this risk.
>>
>> I hope the diff stats will motivate people to have a look and report
>> their testing.
>>
>> I will also eagerly monitor Sashiko's output which will probably be
>> very useful to catch niche weird cases where these changes might break.
>
>
> Honestly, I'm still not sold at the sr1 and sr2 split. But since
> spi-nor is know for it's code churn. I could just rework that later
> :)

:'-)

> Still I had some questions in the old thread:
> I don't get why there should be any endianness problems?
> Just the low level read and write would have to figure out how to
> read and write the status registers.

Yes, that's the point, with an array there is no endianness problem,
even low level helpers do not need to know memory ordering. Endianness
is *always* a pain, it is most of the time wrongly understood, I hate
endianness topics :)

[...]

>> +int spi_nor_read_sr2(struct spi_nor *nor, u8 *sr2)
>> +{
>> +	struct spi_nor_flash_parameter *params = nor->params;
>> +	int ret;
>> +
>> +	if (params->opcodes.read_sr2) {
>> +		ret = spi_nor_read_sr_ll(nor, params->opcodes.read_sr2, sr2, 1);
>> +		if (ret)
>> +			return ret;
>> +	} else if (spi_nor_get_protocol_width(nor->read_proto) == 4 ||
>> +		   spi_nor_get_protocol_width(nor->write_proto) == 4) {
>> +		/* Make sure the QE bit is persistently kept */
>> +		*sr2 = params->qe_mask[1];
>
> Mh. So before there was at least a check if a quad_enable cb is set.
> Is this now implicit? because qe_mask[1] is zero if there is no
> quad_enable? That should at *least* get some comment.

You are right, this is missing the st_micron case. I will re-add the
check.

> I tried to actually get the original use case, but all I've found
> is Tudors commit 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit
> on lock()/unlock()"). There wasn't much of a discussion on the ML,
> and I couldn't find where Boris was suggesting it.
>
> So before we cannot get rid of it anymore, we should just handle it
> in the one caller which actually uses it: spi_nor_sr_{un,}lock().
> I've checked the current spi_nor_write_sr_and_check() and the only
> actual user is the swp.c. atmel.c is doing global write protection
> on/off, which IIRC only uses SR1.

Yes.

> So let's move that weird handling there. Then chances are, we can
> get rid of it as we know what flashes do locking because that is not
> discoverable at runtime. It's just tedious work to check all the
> datasheets. But as soon as we now put that into a more generic path,
> we can't do anything about it anymore.

I understand the concern, which is right. I just added a
"read_sr2_careful" helper for the swp.c purpose.

>> +	} else {
>> +		*sr2 = 0;
>
> I'd expect an -EINVAL or -EIO here.

There is an actual case that is possible where we do not want to use
quad mode on a BFPT_DWORD15_QER_SR2_BIT1_NO_RD chip. I believe we should
keep = 0 here.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/