[PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.

[PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]


Hi,
 I've labeled this an RFC because I'm really not sure about removing the
 error path from spi_nor_write() -- maybe that really matters.  But on
 my hardware, performing multiple small spi writes to the flash seems
 to work.

 The spi driver is drivers/staging/mt7621-spi.  Possibly this needs to
 use DMA instead of a FIFO (assuming the hardware can) - or maybe
 drivers/spi/spi-mt65xx.c can be made to work on this hardware, though
 that is for an ARM SOC and mt7621 is a MIPS SOC.

 I note that openwrt has similar patches:
  target/linux/generic/pending-4.14/450-mtd-spi-nor-allow-NOR-driver-to-write-fewer-bytes-th.patch

 They also change the spi driver to do a short write, rather
 than change m25p80 to request a short write.

 Is there something horribly wrong with this?

Thanks,
NeilBrown

-----------------------8<------------------------

m25p80 honours max_message_size and max_transfer_size
for reads, but not for writes.
I have a driver that has a max message size of 36 bytes
(command, address, 32 bytes data, all places in a FIFO
in the controller).
This requires m25p80_write() to honour the size limits.
For that to work, spi-nor needs to quietly accept partial
writes.

With this, I can successfully re-flash my device.

Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/mtd/devices/m25p80.c  | 3 ++-
 drivers/mtd/spi-nor/spi-nor.c | 7 -------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index a4e18f6aaa33..7ded13507604 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -117,7 +117,8 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 
 	t[data_idx].tx_buf = buf;
 	t[data_idx].tx_nbits = data_nbits;
-	t[data_idx].len = len;
+	t[data_idx].len = min3(len, spi_max_transfer_size(spi),
+			       spi_max_message_size(spi) - cmd_sz);
 	spi_message_add_tail(&t[data_idx], &m);
 
 	ret = spi_sync(spi, &m);
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 42ae9a1529bb..cfa15f2801ad 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1445,13 +1445,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 			goto write_err;
 		*retlen += written;
 		i += written;
-		if (written != page_remain) {
-			dev_err(nor->dev,
-				"While writing %zu bytes written %zd bytes\n",
-				page_remain, written);
-			ret = -EIO;
-			goto write_err;
-		}
 	}
 
 write_err:
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]


Hi,
 I've labeled this an RFC because I'm really not sure about removing the
 error path from spi_nor_write() -- maybe that really matters.  But on
 my hardware, performing multiple small spi writes to the flash seems
 to work.

 The spi driver is drivers/staging/mt7621-spi.  Possibly this needs to
 use DMA instead of a FIFO (assuming the hardware can) - or maybe
 drivers/spi/spi-mt65xx.c can be made to work on this hardware, though
 that is for an ARM SOC and mt7621 is a MIPS SOC.

 I note that openwrt has similar patches:
  target/linux/generic/pending-4.14/450-mtd-spi-nor-allow-NOR-driver-to-write-fewer-bytes-th.patch

 They also change the spi driver to do a short write, rather
 than change m25p80 to request a short write.

 Is there something horribly wrong with this?

Thanks,
NeilBrown

-----------------------8<------------------------

m25p80 honours max_message_size and max_transfer_size
for reads, but not for writes.
I have a driver that has a max message size of 36 bytes
(command, address, 32 bytes data, all places in a FIFO
in the controller).
This requires m25p80_write() to honour the size limits.
For that to work, spi-nor needs to quietly accept partial
writes.

With this, I can successfully re-flash my device.

Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/mtd/devices/m25p80.c  | 3 ++-
 drivers/mtd/spi-nor/spi-nor.c | 7 -------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index a4e18f6aaa33..7ded13507604 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -117,7 +117,8 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 
 	t[data_idx].tx_buf = buf;
 	t[data_idx].tx_nbits = data_nbits;
-	t[data_idx].len = len;
+	t[data_idx].len = min3(len, spi_max_transfer_size(spi),
+			       spi_max_message_size(spi) - cmd_sz);
 	spi_message_add_tail(&t[data_idx], &m);
 
 	ret = spi_sync(spi, &m);
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 42ae9a1529bb..cfa15f2801ad 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1445,13 +1445,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 			goto write_err;
 		*retlen += written;
 		i += written;
-		if (written != page_remain) {
-			dev_err(nor->dev,
-				"While writing %zu bytes written %zd bytes\n",
-				page_remain, written);
-			ret = -EIO;
-			goto write_err;
-		}
 	}
 
 write_err:
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.

[Boris Brezillon]

On Fri, 27 Apr 2018 16:18:05 +1000
NeilBrown <neil@brown.name> wrote:

> Hi,
>  I've labeled this an RFC because I'm really not sure about removing the
>  error path from spi_nor_write() -- maybe that really matters.  But on
>  my hardware, performing multiple small spi writes to the flash seems
>  to work.
> 
>  The spi driver is drivers/staging/mt7621-spi.  Possibly this needs to
>  use DMA instead of a FIFO (assuming the hardware can) - or maybe
>  drivers/spi/spi-mt65xx.c can be made to work on this hardware, though
>  that is for an ARM SOC and mt7621 is a MIPS SOC.
> 
>  I note that openwrt has similar patches:
>   target/linux/generic/pending-4.14/450-mtd-spi-nor-allow-NOR-driver-to-write-fewer-bytes-th.patch
> 
>  They also change the spi driver to do a short write, rather
>  than change m25p80 to request a short write.
> 
>  Is there something horribly wrong with this?

Marek, any opinion on this patch?

> 
> Thanks,
> NeilBrown
> 
> -----------------------8<------------------------
> 
> m25p80 honours max_message_size and max_transfer_size
> for reads, but not for writes.
> I have a driver that has a max message size of 36 bytes
> (command, address, 32 bytes data, all places in a FIFO
> in the controller).
> This requires m25p80_write() to honour the size limits.
> For that to work, spi-nor needs to quietly accept partial
> writes.
> 
> With this, I can successfully re-flash my device.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  drivers/mtd/devices/m25p80.c  | 3 ++-
>  drivers/mtd/spi-nor/spi-nor.c | 7 -------
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index a4e18f6aaa33..7ded13507604 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -117,7 +117,8 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  
>  	t[data_idx].tx_buf = buf;
>  	t[data_idx].tx_nbits = data_nbits;
> -	t[data_idx].len = len;
> +	t[data_idx].len = min3(len, spi_max_transfer_size(spi),
> +			       spi_max_message_size(spi) - cmd_sz);
>  	spi_message_add_tail(&t[data_idx], &m);
>  
>  	ret = spi_sync(spi, &m);
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 42ae9a1529bb..cfa15f2801ad 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1445,13 +1445,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  			goto write_err;
>  		*retlen += written;
>  		i += written;
> -		if (written != page_remain) {
> -			dev_err(nor->dev,
> -				"While writing %zu bytes written %zd bytes\n",
> -				page_remain, written);
> -			ret = -EIO;
> -			goto write_err;
> -		}
>  	}
>  
>  write_err:

Re: [PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3718 bytes --]

On Wed, May 09 2018, Boris Brezillon wrote:

> On Fri, 27 Apr 2018 16:18:05 +1000
> NeilBrown <neil@brown.name> wrote:
>
>> Hi,
>>  I've labeled this an RFC because I'm really not sure about removing the
>>  error path from spi_nor_write() -- maybe that really matters.  But on
>>  my hardware, performing multiple small spi writes to the flash seems
>>  to work.
>> 
>>  The spi driver is drivers/staging/mt7621-spi.  Possibly this needs to
>>  use DMA instead of a FIFO (assuming the hardware can) - or maybe
>>  drivers/spi/spi-mt65xx.c can be made to work on this hardware, though
>>  that is for an ARM SOC and mt7621 is a MIPS SOC.
>> 
>>  I note that openwrt has similar patches:
>>   target/linux/generic/pending-4.14/450-mtd-spi-nor-allow-NOR-driver-to-write-fewer-bytes-th.patch
>> 
>>  They also change the spi driver to do a short write, rather
>>  than change m25p80 to request a short write.
>> 
>>  Is there something horribly wrong with this?
>
> Marek, any opinion on this patch?
>

Hi,
 thanks for following up.
 I have since found that I don't need this patch, though maybe others
 still do(??).
 My hardware can only send 36 bytes and receive 32 in a single
 transaction.  However I can run a sequence of transactions
 to process a whole message no matter how large that message is.  As
 long as I keep chip-select asserted, all the slave device sees is that
 the clock period isn't quite constant, and the slave shouldn't care
 much about that.
 When reading from flash, I found that handling large messages with
 multiple hardware transactions was 50% faster than breaking the
 read down into lots of 32 byte messages.

 So, I won't object if this patch is forgotten.  Thanks for
 your time anyway.

NeilBrown

>> 
>> Thanks,
>> NeilBrown
>> 
>> -----------------------8<------------------------
>> 
>> m25p80 honours max_message_size and max_transfer_size
>> for reads, but not for writes.
>> I have a driver that has a max message size of 36 bytes
>> (command, address, 32 bytes data, all places in a FIFO
>> in the controller).
>> This requires m25p80_write() to honour the size limits.
>> For that to work, spi-nor needs to quietly accept partial
>> writes.
>> 
>> With this, I can successfully re-flash my device.
>> 
>> Signed-off-by: NeilBrown <neil@brown.name>
>> ---
>>  drivers/mtd/devices/m25p80.c  | 3 ++-
>>  drivers/mtd/spi-nor/spi-nor.c | 7 -------
>>  2 files changed, 2 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index a4e18f6aaa33..7ded13507604 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -117,7 +117,8 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>>  
>>  	t[data_idx].tx_buf = buf;
>>  	t[data_idx].tx_nbits = data_nbits;
>> -	t[data_idx].len = len;
>> +	t[data_idx].len = min3(len, spi_max_transfer_size(spi),
>> +			       spi_max_message_size(spi) - cmd_sz);
>>  	spi_message_add_tail(&t[data_idx], &m);
>>  
>>  	ret = spi_sync(spi, &m);
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 42ae9a1529bb..cfa15f2801ad 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1445,13 +1445,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>  			goto write_err;
>>  		*retlen += written;
>>  		i += written;
>> -		if (written != page_remain) {
>> -			dev_err(nor->dev,
>> -				"While writing %zu bytes written %zd bytes\n",
>> -				page_remain, written);
>> -			ret = -EIO;
>> -			goto write_err;
>> -		}
>>  	}
>>  
>>  write_err:

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.

[Marek Vasut]

On 05/10/2018 12:28 AM, NeilBrown wrote:
> On Wed, May 09 2018, Boris Brezillon wrote:
> 
>> On Fri, 27 Apr 2018 16:18:05 +1000
>> NeilBrown <neil@brown.name> wrote:
>>
>>> Hi,
>>>  I've labeled this an RFC because I'm really not sure about removing the
>>>  error path from spi_nor_write() -- maybe that really matters.  But on
>>>  my hardware, performing multiple small spi writes to the flash seems
>>>  to work.
>>>
>>>  The spi driver is drivers/staging/mt7621-spi.  Possibly this needs to
>>>  use DMA instead of a FIFO (assuming the hardware can) - or maybe
>>>  drivers/spi/spi-mt65xx.c can be made to work on this hardware, though
>>>  that is for an ARM SOC and mt7621 is a MIPS SOC.
>>>
>>>  I note that openwrt has similar patches:
>>>   target/linux/generic/pending-4.14/450-mtd-spi-nor-allow-NOR-driver-to-write-fewer-bytes-th.patch
>>>
>>>  They also change the spi driver to do a short write, rather
>>>  than change m25p80 to request a short write.
>>>
>>>  Is there something horribly wrong with this?
>>
>> Marek, any opinion on this patch?
>>
> 
> Hi,
>  thanks for following up.
>  I have since found that I don't need this patch, though maybe others
>  still do(??).
>  My hardware can only send 36 bytes and receive 32 in a single
>  transaction.  However I can run a sequence of transactions
>  to process a whole message no matter how large that message is.  As
>  long as I keep chip-select asserted, all the slave device sees is that
>  the clock period isn't quite constant, and the slave shouldn't care
>  much about that.
>  When reading from flash, I found that handling large messages with
>  multiple hardware transactions was 50% faster than breaking the
>  read down into lots of 32 byte messages.
> 
>  So, I won't object if this patch is forgotten.  Thanks for
>  your time anyway.

Nice, which hardware is that ?

-- 
Best regards,
Marek Vasut

Re: [PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

On Thu, May 10 2018, Marek Vasut wrote:

> On 05/10/2018 12:28 AM, NeilBrown wrote:
>> On Wed, May 09 2018, Boris Brezillon wrote:
>> 
>>> On Fri, 27 Apr 2018 16:18:05 +1000
>>> NeilBrown <neil@brown.name> wrote:
>>>
>>>> Hi,
>>>>  I've labeled this an RFC because I'm really not sure about removing the
>>>>  error path from spi_nor_write() -- maybe that really matters.  But on
>>>>  my hardware, performing multiple small spi writes to the flash seems
>>>>  to work.
>>>>
>>>>  The spi driver is drivers/staging/mt7621-spi.  Possibly this needs to
>>>>  use DMA instead of a FIFO (assuming the hardware can) - or maybe
>>>>  drivers/spi/spi-mt65xx.c can be made to work on this hardware, though
>>>>  that is for an ARM SOC and mt7621 is a MIPS SOC.
>>>>
>>>>  I note that openwrt has similar patches:
>>>>   target/linux/generic/pending-4.14/450-mtd-spi-nor-allow-NOR-driver-to-write-fewer-bytes-th.patch
>>>>
>>>>  They also change the spi driver to do a short write, rather
>>>>  than change m25p80 to request a short write.
>>>>
>>>>  Is there something horribly wrong with this?
>>>
>>> Marek, any opinion on this patch?
>>>
>> 
>> Hi,
>>  thanks for following up.
>>  I have since found that I don't need this patch, though maybe others
>>  still do(??).
>>  My hardware can only send 36 bytes and receive 32 in a single
>>  transaction.  However I can run a sequence of transactions
>>  to process a whole message no matter how large that message is.  As
>>  long as I keep chip-select asserted, all the slave device sees is that
>>  the clock period isn't quite constant, and the slave shouldn't care
>>  much about that.
>>  When reading from flash, I found that handling large messages with
>>  multiple hardware transactions was 50% faster than breaking the
>>  read down into lots of 32 byte messages.
>> 
>>  So, I won't object if this patch is forgotten.  Thanks for
>>  your time anyway.
>
> Nice, which hardware is that ?

Mediatek MT7621 SOC (particularly in the gnubee.org NAS platform).

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.

[Marek Vasut]

On 05/10/2018 01:57 PM, NeilBrown wrote:
> On Thu, May 10 2018, Marek Vasut wrote:
> 
>> On 05/10/2018 12:28 AM, NeilBrown wrote:
>>> On Wed, May 09 2018, Boris Brezillon wrote:
>>>
>>>> On Fri, 27 Apr 2018 16:18:05 +1000
>>>> NeilBrown <neil@brown.name> wrote:
>>>>
>>>>> Hi,
>>>>>  I've labeled this an RFC because I'm really not sure about removing the
>>>>>  error path from spi_nor_write() -- maybe that really matters.  But on
>>>>>  my hardware, performing multiple small spi writes to the flash seems
>>>>>  to work.
>>>>>
>>>>>  The spi driver is drivers/staging/mt7621-spi.  Possibly this needs to
>>>>>  use DMA instead of a FIFO (assuming the hardware can) - or maybe
>>>>>  drivers/spi/spi-mt65xx.c can be made to work on this hardware, though
>>>>>  that is for an ARM SOC and mt7621 is a MIPS SOC.
>>>>>
>>>>>  I note that openwrt has similar patches:
>>>>>   target/linux/generic/pending-4.14/450-mtd-spi-nor-allow-NOR-driver-to-write-fewer-bytes-th.patch
>>>>>
>>>>>  They also change the spi driver to do a short write, rather
>>>>>  than change m25p80 to request a short write.
>>>>>
>>>>>  Is there something horribly wrong with this?
>>>>
>>>> Marek, any opinion on this patch?
>>>>
>>>
>>> Hi,
>>>  thanks for following up.
>>>  I have since found that I don't need this patch, though maybe others
>>>  still do(??).
>>>  My hardware can only send 36 bytes and receive 32 in a single
>>>  transaction.  However I can run a sequence of transactions
>>>  to process a whole message no matter how large that message is.  As
>>>  long as I keep chip-select asserted, all the slave device sees is that
>>>  the clock period isn't quite constant, and the slave shouldn't care
>>>  much about that.
>>>  When reading from flash, I found that handling large messages with
>>>  multiple hardware transactions was 50% faster than breaking the
>>>  read down into lots of 32 byte messages.
>>>
>>>  So, I won't object if this patch is forgotten.  Thanks for
>>>  your time anyway.
>>
>> Nice, which hardware is that ?
> 
> Mediatek MT7621 SOC (particularly in the gnubee.org NAS platform).

On nice, a mips, good to see someone still cares about mips :)

-- 
Best regards,
Marek Vasut