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 <[email protected]>
---
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
On Fri, 27 Apr 2018 16:18:05 +1000
NeilBrown <[email protected]> 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 <[email protected]>
> ---
> 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:
On Wed, May 09 2018, Boris Brezillon wrote:
> On Fri, 27 Apr 2018 16:18:05 +1000
> NeilBrown <[email protected]> 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 <[email protected]>
>> ---
>> 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:
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 <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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