mtd: spi-nor: sst: Factor out common write operation to `sst_nor_write_data()`

Writing to the Flash in `sst_nor_write()` is a 3-step process:
first an optional one-byte write to get 2-byte-aligned, then the
bulk of the data is written out in vendor-specific 2-byte writes.
Finally, if there's a byte left over, another one-byte write.
This was implemented 3 times in the body of `sst_nor_write()`.
To reduce code duplication, factor out these sub-steps to their
own function.

Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---

Notes:
    RFC: I'm thinking of removing SPINOR_OP_BP in favor of
    SPINOR_OP_PP (they have the same value). SPINOR_OP_PP
    is the "standard" name for the elementary unit-sized
    (1 byte, in the case of NOR) write operation. I find it
    confusing to have two names for the same operation,
    so in a followup I plan to remove the vendor-specific
    name in favor of the standard one.

 drivers/mtd/spi-nor/sst.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

Hi,

On Wed, Jul 10 2024, Csókás, Bence wrote:

> Writing to the Flash in `sst_nor_write()` is a 3-step process:
> first an optional one-byte write to get 2-byte-aligned, then the
> bulk of the data is written out in vendor-specific 2-byte writes.
> Finally, if there's a byte left over, another one-byte write.
> This was implemented 3 times in the body of `sst_nor_write()`.
> To reduce code duplication, factor out these sub-steps to their
> own function.
>
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
> ---
>
> Notes:
>     RFC: I'm thinking of removing SPINOR_OP_BP in favor of
>     SPINOR_OP_PP (they have the same value). SPINOR_OP_PP
>     is the "standard" name for the elementary unit-sized
>     (1 byte, in the case of NOR) write operation. I find it
>     confusing to have two names for the same operation,
>     so in a followup I plan to remove the vendor-specific
>     name in favor of the standard one.

Even though the operations have the same opcode, I see them as different
operations. One is a byte program: it can only write one byte at a time.
The other is a page program: it can write up to one page (256 bytes
usually) at a time.

So I would actually find it more confusing if you use page program in a
situation where the operation is actually a byte program, and attempting
to program the whole page will fail.

>
>  drivers/mtd/spi-nor/sst.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 180b7390690c..fec71689e644 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -167,6 +167,21 @@ static const struct flash_info sst_nor_parts[] = {
>  	}
>  };
>  
> +static int sst_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> +			const u_char *buf)
> +{
> +	u8 op = (len == 1) ? SPINOR_OP_BP : SPINOR_OP_AAI_WP;

Hmm, this looks a bit hacky. But the whole sst_nor_writa() is kinda
hacky anyway so it is fine I guess... LGTM otherwise.

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

Not directly related to this patch, but when reviewing this patch I
noticed another small improvement you can make. since you are already
looking at this code. (to be clear, this is not needed for this patch to
get merged, this can be a follow up patch).

sst_nor_write() looks like:

	/* Write out most of the data here. */
	for (; actual < len - 1; actual += 2) {
        	[...]
	}
	nor->sst_write_second = false;

	ret = spi_nor_write_disable(nor);
	if (ret)
		goto out;

	ret = spi_nor_wait_till_ready(nor);
	if (ret)
		goto out;

	/* Write out trailing byte if it exists. */
	if (actual != len) {
		ret = spi_nor_write_enable(nor);
                [...]

		ret = spi_nor_write_data(nor, to, 1, buf + actual);
                [...]

		ret = spi_nor_wait_till_ready(nor);
		if (ret)
			goto out;


		ret = spi_nor_write_disable(nor);
	}

Here, we do a write disable. Then if a one-byte write is needed, do a
write enable again, write the data and write disable.

Do we really need to toggle write enable between these? If not, it can
be simplified to only do the write disable after all bytes have been
written.

[...]

Hi!

On 7/10/24 15:04, Pratyush Yadav wrote:
>> Notes:
>>      RFC: I'm thinking of removing SPINOR_OP_BP in favor of
>>      SPINOR_OP_PP (they have the same value). SPINOR_OP_PP
>>      is the "standard" name for the elementary unit-sized
>>      (1 byte, in the case of NOR) write operation. I find it
>>      confusing to have two names for the same operation,
>>      so in a followup I plan to remove the vendor-specific
>>      name in favor of the standard one.
> 
> Even though the operations have the same opcode, I see them as different
> operations. One is a byte program: it can only write one byte at a time.
> The other is a page program: it can write up to one page (256 bytes
> usually) at a time.
> 
> So I would actually find it more confusing if you use page program in a
> situation where the operation is actually a byte program, and attempting
> to program the whole page will fail.

Yes, SST engineers took some _unconventional_ steps when designing this 
family... However, there are no 256 byte pages in these chips. You 
either program it one byte at a time, or as a sequence of two byte 
values. So, in my eyes, that makes it a Flash where the page size is 1 
byte, and the vendor-specific write is something extra added on (and 
mind you, that's not a page program either, you just feed it an 
*arbitrary* even number of bytes, there really are no pages here at all, 
only erase sectors).

> Not directly related to this patch, but when reviewing this patch I
> noticed another small improvement you can make. [...]
> Here, we do a write disable. Then if a one-byte write is needed, do a
> write enable again, write the data and write disable.
> 
> Do we really need to toggle write enable between these? If not, it can
> be simplified to only do the write disable after all bytes have been
> written.

Honestly, I'm not sure, I was too afraid to touch that part. However, 
from the datasheet of SST25VF040B I presume that if we did not toggle 
it, then the Flash chip would interpret the 0x02 opcode and its argument 
as another 2 bytes of data to write at the end. Byte Program takes 
exactly 1 argument, so it can be followed by another command, but AAI WP 
goes on until ~CS goes high.

 > Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

Thanks!

Bence

On Wed, Jul 10 2024, Csókás Bence wrote:

> Hi!
>
> On 7/10/24 15:04, Pratyush Yadav wrote:
>>> Notes:
>>>      RFC: I'm thinking of removing SPINOR_OP_BP in favor of
>>>      SPINOR_OP_PP (they have the same value). SPINOR_OP_PP
>>>      is the "standard" name for the elementary unit-sized
>>>      (1 byte, in the case of NOR) write operation. I find it
>>>      confusing to have two names for the same operation,
>>>      so in a followup I plan to remove the vendor-specific
>>>      name in favor of the standard one.
>> Even though the operations have the same opcode, I see them as different
>> operations. One is a byte program: it can only write one byte at a time.
>> The other is a page program: it can write up to one page (256 bytes
>> usually) at a time.
>> So I would actually find it more confusing if you use page program in a
>> situation where the operation is actually a byte program, and attempting
>> to program the whole page will fail.
>
> Yes, SST engineers took some _unconventional_ steps when designing this
> family... However, there are no 256 byte pages in these chips. You either
> program it one byte at a time, or as a sequence of two byte values. So, in my
> eyes, that makes it a Flash where the page size is 1 byte, and the
> vendor-specific write is something extra added on (and mind you, that's not a
> page program either, you just feed it an *arbitrary* even number of bytes, there
> really are no pages here at all, only erase sectors).

Exactly. Since there are no pages, calling the operation "Page Program"
would be a misnomer, no? Byte Program is a fitting name IMO.

Beyond cosmetic reasons, do you see any need for changing this?
Otherwise, I'd rather avoid the churn on something that is in the gray
zone anyway.

>
>> Not directly related to this patch, but when reviewing this patch I
>> noticed another small improvement you can make. [...]
>> Here, we do a write disable. Then if a one-byte write is needed, do a
>> write enable again, write the data and write disable.
>> Do we really need to toggle write enable between these? If not, it can
>> be simplified to only do the write disable after all bytes have been
>> written.
>
> Honestly, I'm not sure, I was too afraid to touch that part. However, from the
> datasheet of SST25VF040B I presume that if we did not toggle it, then the Flash
> chip would interpret the 0x02 opcode and its argument as another 2 bytes of data
> to write at the end. Byte Program takes exactly 1 argument, so it can be
> followed by another command, but AAI WP goes on until ~CS goes high.

I see. Then let's _not_ fix what isn't broken!

Hi,

On Wed, Jul 10 2024, Csókás, Bence wrote:

> Writing to the Flash in `sst_nor_write()` is a 3-step process:
> first an optional one-byte write to get 2-byte-aligned, then the
> bulk of the data is written out in vendor-specific 2-byte writes.
> Finally, if there's a byte left over, another one-byte write.
> This was implemented 3 times in the body of `sst_nor_write()`.
> To reduce code duplication, factor out these sub-steps to their
> own function.
>
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>

Applied to spi-nor/next, thanks!

FYI, I spotted a couple small issues and fixed them up when applying.
See below...

> ---
>
> Notes:
>     RFC: I'm thinking of removing SPINOR_OP_BP in favor of
>     SPINOR_OP_PP (they have the same value). SPINOR_OP_PP
>     is the "standard" name for the elementary unit-sized
>     (1 byte, in the case of NOR) write operation. I find it
>     confusing to have two names for the same operation,
>     so in a followup I plan to remove the vendor-specific
>     name in favor of the standard one.
>
>  drivers/mtd/spi-nor/sst.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 180b7390690c..fec71689e644 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -167,6 +167,21 @@ static const struct flash_info sst_nor_parts[] = {
>  	}
>  };
>  
> +static int sst_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> +			const u_char *buf)

Whitespace issue, checkpatch complains on this. It should be aligned
with the opening parenthesis above.

> +{
> +	u8 op = (len == 1) ? SPINOR_OP_BP : SPINOR_OP_AAI_WP;
> +	int ret;
> +
> +	nor->program_opcode = op;
> +	ret = spi_nor_write_data(nor, to, 1, buf);
> +	if (ret < 0)
> +		return ret;
> +	WARN(ret != len, "While writing %i byte written %i bytes\n", len, ret);

I get a build warning because of incorrect format specifier. Should use
%zu since len is size_t.

> +
> +	return spi_nor_wait_till_ready(nor);
> +}
> +
>  static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  			 size_t *retlen, const u_char *buf)
>  {
[...]