Message ID | 87efj1kw9u.fsf@notabene.neil.brown.name |
---|---|
State | Rejected |
Delegated to: | Cyrille Pitchen |
Headers | show |
Series | [PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes. | expand |
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:
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:
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 ?
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
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 :)
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:
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(-)